Phase 3 pre-bump: explicit hardened webPreferences at every window#980
Closed
notluquis wants to merge 8 commits into
Closed
Phase 3 pre-bump: explicit hardened webPreferences at every window#980notluquis wants to merge 8 commits into
notluquis wants to merge 8 commits into
Conversation
electron-builder --dir fails when env_installer/jlab_server.tar.gz is absent, since package.json build.extraResources lists it as a literal path. The real tarball comes from conda pack, which is too heavy for CI. Packaged-build e2e never uses the bundled environment because it sources Python from JLAB_TEST_PYTHON_PATH, so a minimal stub tarball satisfies the extraResources copy. The script generates the tar at CI time; the tar stays gitignored and is not committed. Note: AI-assisted (Claude Code). Manually verified: node scripts/stub-env-tar.js writes a 425-byte gzip tarball, tar -tzf lists STUB_FOR_E2E; mutation check (renamed marker) fails the 2 listing tests confirming they are not tautological; yarn lint:check exit 0; yarn test:unit 433/433 green incl 3 new; git check-ignore confirms the tar is gitignored and only the two source files are staged. yarn build fails on a pre-existing @types/esrecurse type error in shared node_modules, reproduced on the clean baseline, unrelated to this change.
…nce lock Packaged-build Playwright cannot drive the app because app.relaunch() plus requestSingleInstanceLock tear down or hand off the process Playwright is monitoring, so launch hangs or the window never appears. Gate both behaviors behind a JLAB_DESKTOP_E2E_MODE=1 env var, matching the existing JLAB_DESKTOP_* convention. When set, the two relaunch sites log and skip, and the lock is treated as already held without registering the second-instance handoff. The non-e2e path keeps its existing resolve and reject behavior unchanged. Note: AI-assisted (Claude Code). Manually verified: tsc typecheck clean (scoped typeRoots to avoid an unrelated parent-worktree @types/esrecurse that breaks tsc 4.2 on every checkout including clean origin/master), preload webpack bundle + copyassets clean, lint:check exit 0, test:unit 438 passed including 8 new isE2EMode cases. Mutation: flipping === '1' to !== '1' fails all 8 isE2EMode tests, then reverted. e2e 5/5 pass (default path, env var unset) reproducibly.
…ll views Pin contextIsolation:true, nodeIntegration:false, sandbox:true at every BrowserView/BrowserWindow construction site instead of relying on Electron defaults. Closes the nodeIntegration/contextIsolation advisory and prevents a silent regression when Electron 42 changes the defaults. A shared hardenedWebPreferences helper keeps the policy in one place. connect.ts is refactored so the flags are always set (previously webPreferences was undefined unless incognito/partition was passed), which also hardens the remote-server connection window. The 12 preloads only use contextBridge/ipcRenderer, both available under sandbox:true, so this is non-breaking. Note: AI-assisted (Claude Code). Manually verified: eslint clean, prettier clean on src, 430 unit tests pass, and the e2e suite (5 passed including the env-backed New-notebook flow) confirms the views load under sandbox:true in real runtime. tsc build fails only on an environmental @types/esrecurse parse error from the nested worktree seeing the parent's eslint-10 types, unrelated to this change.
# Conflicts: # src/main/utils.ts
62 tasks
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged js/file-system-race (high) on the test: existsSync + statSync + readFileSync on the same path is a time-of-check-to-time-of-use race. Read the file once and assert on the buffer instead (readFileSync throws if missing, length covers the size check, the first two bytes are the gzip magic). Same assertions, one access, no race. Note: AI-assisted (Claude Code). Manually verified: the 3 stub-tar tests pass, eslint and prettier clean, and no existsSync/statSync remain on the path so the CodeQL check-then-use pattern is gone.
Both shipped ahead of their consumer (a packaged-build e2e that is not in the tree yet) and the e2e-mode toggle put test branches in production code. The JLAB_DESKTOP_E2E_MODE opt-out around requestSingleInstanceLock was redundant: the single-instance lock is Chromium's ProcessSingleton keyed on the userData directory, so the unique --user-data-dir each e2e already launches with gives every run its own lock. Disabling the lock by env var in a shipped build is a footgun with no benefit. The two app.relaunch() sites it also guarded are user-triggered (update bundled env, RestartApp); a launch smoke never reaches them. Revert all three to native behavior. scripts/stub-env-tar.js had no consumer either (no workflow or package script runs electron-builder --dir against it); its only caller was its own unit test, whose tarball read was the CodeQL js/file-system-race finding. Removing the script removes that finding at the source. Both pieces will return with the packaged-build e2e in Phase 3, where a real failing launch proves them necessary. The webPreferences hardening in this PR is unaffected. Note: AI-assisted (Claude Code). Manually verified: requestSingleInstanceLock userData keying against Electron PR #33559 + ProcessSingleton docs and the VS Code smoke-test pattern (fresh user-data-dir per run, no test-mode var); both relaunch sites are user-triggered (app.ts); grep confirms no remaining isE2EMode/JLAB_DESKTOP_E2E_MODE references and no consumer of stub-env-tar; tsc --noEmit clean, 430 unit tests pass, coverage 58% over the 48% floor, eslint clean.
notluquis
changed the title
Collaborator
Author
|
Closing this. After dropping the e2e-mode opt-out and the stub-env-tar from this PR, the only remaining change was adding explicit contextIsolation/nodeIntegration/sandbox at the window construction sites via a hardenedWebPreferences helper. Checked the defaults against the Electron security tutorial: nodeIntegration is false by default since 5.0, contextIsolation true since 12.0, and sandbox true since 20.0. At Electron 27 (and 42) all three secure values already apply, and the security checklist is satisfied by the defaults rather than requiring the flags to be written out. master already relies on the defaults at every one of these sites and is green, so the explicit declaration here does not change runtime behavior. Rather than land a helper that only restates the defaults (and that a static analyzer like Electronegativity cannot resolve through a function call), I would rather rely on the defaults like master does. If defense-in-depth explicit flags are wanted later on the remote-content surfaces (labview, the connect window), that is a small focused change we can do on its own with that rationale. Note: AI-assisted (Claude Code). Manually verified the default values against the Electron docs and that there are no open CodeQL alerts for these sites. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #951 Phase 3 pre-bump PRs. De-risks the Electron 42 bump and is safe at Electron 27.
Explicit webPreferences (contextIsolation, nodeIntegration, sandbox) at every BrowserView/BrowserWindow construction site via a shared hardenedWebPreferences helper, instead of relying on Electron defaults. Closes the nodeIntegration/contextIsolation advisory and prevents a silent regression when Electron 42 changes the defaults. sandbox:true is verified non-breaking: the 12 preloads only use contextBridge/ipcRenderer.
This PR originally also carried a JLAB_DESKTOP_E2E_MODE opt-out and a stub-env-tar script. Both were dropped: the single-instance lock is keyed on the userData dir so a unique --user-data-dir per e2e already isolates it (no opt-out needed), the relaunch sites are user-triggered, and neither piece had a consumer yet. They will return with the packaged-build e2e in a later phase, where a real launch proves them necessary. The CodeQL file-system-race finding went away with the stub-tar test that contained it.
Note: AI-assisted (Claude Code). Manually verified: tsc --noEmit clean, unit suite green, coverage over the floor, eslint clean; confirmed the single-instance handoff works natively by launching two instances against one userData dir (the second logs "Handling request in the main instance." and exits).