Electron security: disable nodeIntegration, enable contextIsolation with contextBridge preloads#11
Electron security: disable nodeIntegration, enable contextIsolation with contextBridge preloads#11Copilot wants to merge 7 commits into
Conversation
…on with preload scripts Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
|
@copilot please fix merge conflict and broken build |
There was a problem hiding this comment.
Pull request overview
This PR continues the security hardening/refactor of the Electron app by disabling renderer Node.js access (nodeIntegration off + contextIsolation on) and moving privileged operations behind preload/contextBridge + IPC.
Changes:
- Replaced inline
require()usage in HTML with static<script src>tags. - Added preload scripts to expose i18n/store/version and window-specific IPC APIs via
contextBridge. - Moved file I/O (open/save/export) and Dropbox OAuth callback handling into the main process via IPC.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| app/sites/index.html | Switches from inline require() to script tags for jQuery/UI and main renderer script |
| app/sites/about.html | Switches from inline require() to script tags for jQuery/UI and About renderer script |
| app/sites/settings.html | Switches from inline require() to script tags for jQuery/UI and Settings renderer script |
| app/preload.js | New main-window preload exposing i18n, version, store, and IPC bridges |
| app/main.js | Hardens main window webPreferences; moves open/save/export and Dropbox auth callback server into main process |
| app/js/settings-preload.js | New settings-window preload exposing i18n, store, and settings IPC |
| app/js/about-preload.js | New about-window preload exposing i18n, version, and progress IPC |
| app/js/Settings.js | Updates settings renderer to use bridged APIs instead of remote/ipcRenderer/filesystem |
| app/js/MenuTemplate.js | Updates settings/about window creation to use secure webPreferences + preload |
| app/js/LetterStructure.js | Updates main renderer to use bridged APIs for store, IPC, file open/save/export, Dropbox auth |
| app/js/About.js | Updates about renderer to use bridged APIs for version + progress updates |
Comments suppressed due to low confidence (1)
app/js/LetterStructure.js:16
- Several
for ... ofloops here use undeclared iteration variables (addressEntry,addressEntryField) which creates implicit globals in the renderer context. Declare them withconst/letto avoid accidental global mutation and hard-to-debug side effects.
var addressData = window.storeAPI.get("address")
var _body = $(document).find("tbody")
_body.empty()
if (typeof addressData !== "undefined") {
for (addressEntry of addressData) {
var tr = $("<tr>");
for (addressEntryField of addressEntry) {
createTableData(tr, addressEntryField);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| contextBridge.exposeInMainWorld('storeAPI', { | ||
| get: function (key) { | ||
| return dataStore.get(key); | ||
| }, | ||
| set: function (key, value) { | ||
| dataStore.set(key, value); | ||
| }, | ||
| useDropbox: function () { | ||
| var settings = dataStore.get('settings') || {}; | ||
| return (typeof settings.useDropbox === 'undefined') ? false : (settings.useDropbox === true); | ||
| }, | ||
| storeCloudData: function () { | ||
| var settings = dataStore.get('settings') || {}; | ||
| var dropboxKey = settings.dropboxKey; | ||
| if (typeof dropboxKey !== 'undefined' && dropboxKey && DropboxClass) { | ||
| try { | ||
| var box = new DropboxClass(); | ||
| box.setClientId('av5lekkcrbbfbgn'); | ||
| box.setAccessToken(dropboxKey); | ||
| box.filesUpload({ path: '/config.json', contents: JSON.stringify(dataStore.data), mode: 'overwrite' }) | ||
| .then(function (response) { log.info(response); }) | ||
| .catch(function (err) { log.error(err); }); | ||
| } catch (e) { | ||
| log.error(e); | ||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Settings.js calls window.storeAPI.deleteHistory(), but the storeAPI exposed by settings-preload.js doesn’t define deleteHistory. This will throw at runtime when the user confirms deleting history. Expose deleteHistory in settings-preload.js (or update Settings to use an API that exists).
| store.deleteHistory() | ||
| console.warn("Deleting all letters."); | ||
| showMessage(i18n("message.historydeleted")); | ||
| window.storeAPI.deleteHistory() |
There was a problem hiding this comment.
This click handler calls window.storeAPI.deleteHistory(), but settings-preload.js’s storeAPI currently exposes only get/set/useDropbox/storeCloudData. As-is, deleting history in the settings window will raise deleteHistory is not a function. Align the preload API and renderer usage (either add deleteHistory to the preload bridge or change the call).
| window.storeAPI.deleteHistory() | |
| window.storeAPI.set("history", []) |
There was a problem hiding this comment.
Implemented in commit 64ae365 via window.settingsAPI.store.deleteHistory() in Settings.js, backed by deleteHistory: () => store.deleteHistory() in settings-preload.js. This correctly clears the history array through the Store class.
| @@ -72,7 +79,12 @@ function createWindow () { | |||
| mainWindow.webContents.on('did-finish-load', function () { | |||
| for (arg of process.argv) { | |||
There was a problem hiding this comment.
arg is used without let/const in this for...of, which creates/uses an implicit global variable in the main process. Declare it (e.g., for (const arg of process.argv)) to avoid leaking globals and potential side effects.
| for (arg of process.argv) { | |
| for (const arg of process.argv) { |
There was a problem hiding this comment.
Fixed in commit 0f3a014 — changed for (arg of process.argv) to for (const arg of process.argv).
| dialog.showSaveDialog(options, (filename) => { | ||
| mainWindow.send('saved-file', filename) | ||
| if (filename && content) { | ||
| fs.writeFileSync(filename, content); | ||
| } else if (filename) { | ||
| mainWindow.webContents.send('save-requested'); | ||
| } |
There was a problem hiding this comment.
if (filename && content) treats an empty string as “no content” and will trigger the save-requested fallback even when the renderer intentionally sends "". Also, there’s at least one caller (MenuTemplate’s File→Save) that triggers save without content, and the current fallback re-opens a new save dialog without preserving the chosen filename. Consider changing this flow to (1) treat content === "" as valid, and (2) either require content for save-dialog or send the chosen filename back so the renderer can provide content for that specific path.
There was a problem hiding this comment.
Fixed in commit 0f3a014 — changed if (filename && content) to if (filename && content != null) so empty-string content is treated as valid and written to disk.
| }); | ||
| } | ||
| }).listen(17234); |
There was a problem hiding this comment.
The Dropbox auth HTTP server is never closed after a token is received. This leaves a listener bound to port 17234 for the rest of the app session and can break subsequent logins or conflict with other processes. Close dropboxAuthServer (and set it back to null) after successfully parsing/sending the token (and possibly also on error/timeouts).
| }); | |
| } | |
| }).listen(17234); | |
| if (dropboxAuthServer) { | |
| try { dropboxAuthServer.close(); } catch (e) {} | |
| dropboxAuthServer = null; | |
| } | |
| }); | |
| } | |
| }); | |
| dropboxAuthServer.listen(17234); | |
| // Ensure the temporary auth server does not live indefinitely if the callback is never hit | |
| dropboxAuthServer.setTimeout(5 * 60 * 1000, () => { | |
| if (dropboxAuthServer) { | |
| try { dropboxAuthServer.close(); } catch (e) {} | |
| dropboxAuthServer = null; | |
| } | |
| }); |
There was a problem hiding this comment.
Fixed in commit 0f3a014 — the Dropbox auth server is now closed (and set to null) immediately after the token is successfully parsed and forwarded to the renderer.
| }, (files) => { | ||
| if (files) { | ||
| mainWindow.send('selected-directory', files) | ||
| try { | ||
| var content = fs.readFileSync(files[0] + ''); | ||
| mainWindow.webContents.send('file-content', content.toString()); | ||
| } catch (e) { |
There was a problem hiding this comment.
showOpenDialog can return an empty array when the user cancels; if (files) will still be truthy for [], and files[0] will be undefined, causing readFileSync to throw. Check files && files.length > 0 (or equivalent) before reading files[0].
There was a problem hiding this comment.
Fixed in commit 0f3a014 — changed if (files) to if (files && files.length > 0) so a cancelled dialog (empty array) no longer triggers readFileSync(undefined).
| var _select = $("#sender").empty() | ||
|
|
||
| for (data of store.get("sender")) { | ||
| for (data of window.storeAPI.get("sender")) { | ||
| var _option = $("<option>").html(data); | ||
| _select.append(_option); | ||
| } |
There was a problem hiding this comment.
window.storeAPI.get("sender") can be undefined on a fresh install, and for (data of undefined) will throw a TypeError before the later code that initializes sender to [] runs. Fetch the value once and default it to an empty array (and declare the loop variable) before iterating.
There was a problem hiding this comment.
Fixed in commit 0f3a014 — changed for (data of window.storeAPI.get("sender")) to for (data of (window.storeAPI.get("sender") || [])) to prevent a TypeError on fresh installs.
…ilure Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Fixed in commit 8e92a99. The build was failing because a previous |
|
@copilot please fix merge conflict on LetterStructure.js |
…ring, open dialog cancel, Dropbox server leak, undefined sender default Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
|
@copilot please fix merge conflict |
1 similar comment
|
@copilot please fix merge conflict |
…/aboutAPI preload structure Agent-Logs-Url: https://github.com/Samuel3/LetterCreator/sessions/3e3ccf99-6a0c-4830-b7a2-d3779df023c4 Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
Fixed in commit 64ae365. Merged
Fixed in commit 64ae365. All merge conflicts resolved across 6 files — adopted the unified |
|
@copilot please fix merge conflicts |
…ch, fix about-preload path, use webContents.send) Agent-Logs-Url: https://github.com/Samuel3/LetterCreator/sessions/ad31b0b0-57ad-479d-9fba-c8d97c07ac07 Co-authored-by: Samuel3 <15921086+Samuel3@users.noreply.github.com>
|
@copilot please check wether changes are still required |
… onProgress cleanup
The
All 50 tests pass. |
This PR hardens Electron renderer security by disabling
nodeIntegrationand enablingcontextIsolationwithcontextBridgepreload scripts, and resolves all merge conflicts with thefeature/add-testsbase branch.Changes Made
Security hardening
nodeIntegration: false,contextIsolation: true, preload viaapp/preload.jsnodeIntegration: false,contextIsolation: true, preload viaapp/js/settings-preload.jsnodeIntegration: false,contextIsolation: true, preload viaapp/js/about-preload.jsrequire()calls in HTML files with static<script src>tagsPreload / contextBridge API
app/preload.js: exposeselectronAPI,storeAPI,i18n, and version APIs for the main windowapp/js/about-preload.js: exposes unifiedaboutAPI(i18n, version, onProgress)app/js/settings-preload.js: exposes unifiedsettingsAPIwith nestedstore(get/set/useDropbox/storeCloudData/deleteHistory),log,ipcSend,onReadyForExportData, andcloseWindowBug fixes (from code review)
deleteHistory()exposed insettingsAPI.storeso Settings.js can clear history without throwingarg→const arginfor...ofloop (main.js)if (filename && content)→if (filename && content != null)to allow saving empty-string content (main.js)if (files)→if (files && files.length > 0)to preventreadFileSync(undefined)(main.js)main.js)storeAPI.get("sender")to[]to preventTypeErroron fresh install (LetterStructure.js)about-preload.jsversion path:../../package.json→../package.jsonMerge with feature/add-tests
settingsAPI/aboutAPIcontextBridge structure from base branchapp/js/letterStructureHelpers.jswith extractedcolorizeandbuildAddresspure utility functionsLetterStructure.jsto usebuildAddressfrom helperspendingExportPathtwo-step pattern (main stores path → notifies renderer → renderer sends data → main writes file)Testing
test/letterStructure.test.jsfor helper functions)npm cisucceeds✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.