fix: remove insecure Electron flags (nodeIntegration, contextIsolation) - #874
Open
raul-oliveira wants to merge 2 commits into
Open
fix: remove insecure Electron flags (nodeIntegration, contextIsolation)#874raul-oliveira wants to merge 2 commits into
raul-oliveira wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughElectron renderer access is moved behind a preload-exposed ChangesElectron contextBridge migration
Sequence DiagramsequenceDiagram
participant Renderer as Renderer
participant electronAPI as window.electronAPI
participant preload as preload.js
participant Main as Electron Main Process
Renderer->>electronAPI: send / openExternal / Sentry call
electronAPI->>preload: validate and normalize input
preload->>Main: ipcRenderer.send(...) or shell.openExternal(...)
Main-->>preload: IPC event or external action result
preload-->>Renderer: bridged listener or return value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
public/preload.js (1)
57-70: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
restoreBuffersonly restoresUint8Array; other typed arrays and special objects are silently mangled.
value instanceof Uint8Arraycorrectly catchesBufferandUint8Array, but any otherTypedArray(e.g.Int8Array,Float32Array),Date,Map, orSetfalls into the generic object branch, whereObject.entries(...)reconstructs an empty/plain object and drops the original type. There is also no cycle guard, so a circular payload would recurse infinitely. This is currently safe for the Buffer-only Ledger payloads, but it’s a latent trap if payload shapes change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/preload.js` around lines 57 - 70, The restoreBuffers function currently only handles Uint8Array correctly, causing other TypedArray instances (Int8Array, Float32Array, etc.), Date, Map, and Set objects to be silently converted to plain objects when they fall into the generic object branch. Additionally, there is no protection against circular references which could cause infinite recursion. To fix this, extend the restoreBuffers function to explicitly check for and properly restore other TypedArray types by converting them using Buffer.from or their respective constructors, add specific checks for Date, Map, and Set to preserve their types, and implement cycle detection using a WeakSet parameter to track visited objects and prevent infinite recursion on circular payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/preload.js`:
- Line 92: The openExternal function exposed at line 92 lacks protocol
validation, creating a security vulnerability where compromised renderers could
execute arbitrary file or custom scheme handlers. Add protocol validation to the
openExternal function to restrict URLs to http: and https: protocols only,
rejecting or throwing an error for any other protocols like file: or custom
schemes before calling shell.openExternal.
- Around line 13-16: The Sentry.init() call at the preload stage initializes
with the live DSN before user consent is obtained, which violates privacy
requirements. Additionally, process.env.npm_package_version is undefined in
packaged Electron builds. Modify the Sentry.init() call to initialize with an
empty string for the dsn parameter instead of constants.SENTRY_DSN, and replace
process.env.npm_package_version with constants.VERSION. The real DSN should be
configured later in the renderer process after the user consent check is
performed via the sentrySetEnabled() mechanism.
---
Nitpick comments:
In `@public/preload.js`:
- Around line 57-70: The restoreBuffers function currently only handles
Uint8Array correctly, causing other TypedArray instances (Int8Array,
Float32Array, etc.), Date, Map, and Set objects to be silently converted to
plain objects when they fall into the generic object branch. Additionally, there
is no protection against circular references which could cause infinite
recursion. To fix this, extend the restoreBuffers function to explicitly check
for and properly restore other TypedArray types by converting them using
Buffer.from or their respective constructors, add specific checks for Date, Map,
and Set to preserve their types, and implement cycle detection using a WeakSet
parameter to track visited objects and prevent infinite recursion on circular
payloads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b0b5cb3-cd7e-4ce7-8d2f-959307bf628d
📒 Files selected for processing (6)
lavamoat/webpack/policy-override.jsonpublic/electron.jspublic/preload.jssrc/constants.jssrc/utils/helpers.jssrc/utils/wallet.js
Comment on lines
13
to
16
| Sentry.init({ | ||
| dsn: constants.SENTRY_DSN, | ||
| release: process.env.npm_package_version | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\b(isSentryAllowed|updateSentryState|initSentry|sentrySetEnabled)\b' src public
rg -nP 'SENTRY_DSN|export const VERSION' src/constants.js
rg -nP 'npm_package_version' public srcRepository: HathorNetwork/hathor-wallet
Length of output: 1617
🏁 Script executed:
cat -n public/preload.js | head -100Repository: HathorNetwork/hathor-wallet
Length of output: 4256
Sentry initializes with the live DSN at preload startup, before any user consent gate.
The top-level Sentry.init at lines 13–16 runs immediately when preload loads, capturing the real constants.SENTRY_DSN. While the renderer can later disable it via sentrySetEnabled(), this only affects future errors. Any errors during preload execution—including early renderer initialization before consent is checked—are captured without user consent.
Additionally, process.env.npm_package_version is only populated during npm script execution. In packaged Electron builds, it's undefined. Use constants.VERSION instead, which is already imported and properly sourced from package.json.
Initialize with the DSN disabled ('') and let the renderer enable it only after consent:
Suggested change
-Sentry.init({
- dsn: constants.SENTRY_DSN,
- release: process.env.npm_package_version
-})
+// Start disabled; the renderer enables it via sentrySetEnabled(dsn) only
+// after the user consents (see wallet.updateSentryState).
+Sentry.init({
+ dsn: '',
+ release: constants.VERSION
+})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@public/preload.js` around lines 13 - 16, The Sentry.init() call at the
preload stage initializes with the live DSN before user consent is obtained,
which violates privacy requirements. Additionally,
process.env.npm_package_version is undefined in packaged Electron builds. Modify
the Sentry.init() call to initialize with an empty string for the dsn parameter
instead of constants.SENTRY_DSN, and replace process.env.npm_package_version
with constants.VERSION. The real DSN should be configured later in the renderer
process after the user consent check is performed via the sentrySetEnabled()
mechanism.
raul-oliveira
force-pushed
the
raul-oliveira/fix/electron-insecure-flags
branch
from
June 24, 2026 01:27
c432d6c to
9292f39
Compare
The renderer ran with nodeIntegration enabled and contextIsolation disabled so it could reach the main process via window.require. Replace that with a contextBridge API exposed from preload.js, which lets us turn the two insecure flags off. - electron.js: nodeIntegration:false, contextIsolation:true (sandbox kept false so the preload can still use Node to set up the bridge) - preload.js: expose `window.electronAPI` with a whitelisted IPC surface (send/on/removeAllListeners), openExternal, and Sentry. Buffers are restored when crossing the bridge so the Ledger main-process code in public/ledger.js stays unchanged - constants/helpers/wallet: use window.electronAPI instead of window.require - Sentry now runs in the preload (@sentry/electron, outside LavaMoat) and is reached through the bridge; @sentry/browser is kept only as the browser/dev fallback - lavamoat policy-override: grant setTimeout/clearTimeout to lodash. Lodash reads setTimeout off the captured global object; with nodeIntegration on it used Node's global (which bypassed LavaMoat), and disabling it exposed the missing grant Validated on a physical Ledger Nano S: getVersion, getPublicKeyData, checkAddress, and a full sendTx + getSignatures flow (transaction sent successfully) all work through the bridge.
raul-oliveira
force-pushed
the
raul-oliveira/fix/electron-insecure-flags
branch
from
June 24, 2026 13:50
9292f39 to
ec185a0
Compare
pedroferreira1
left a comment
pedroferreira1
left a comment
Member
There was a problem hiding this comment.
Review by claudin:
P1 — send() throws for every non-Buffer payload; Ledger send hangs forever
public/preload.js:81 — ipcRenderer.send(channel, ...args.map(restoreBuffers)). Array.prototype.map passes (element, index, array), so the array index lands in restoreBuffers's seen parameter, replacing the WeakSet default. Any top-level argument that's a plain object or array hits seen.has(value) → TypeError: seen.has is not a function. I ran the exact function from the diff: ledger:getSignatures (number array, src/utils/ledger.js:184), ledger:sendTokens and ledger:verifyManyTokenSignatures (token-map objects) all throw; only Buffer/primitive/no-arg channels survive.
User-visible impact, traced through the screens:
- HTR send: sendTx (a single Buffer) crosses fine, the Ledger prompts, the user approves on device — then handleTxSent calls getSignatures (src/screens/SendTokens.js:170), which is async and called without await/.catch (SendTokens.js:293), so the TypeError becomes a swallowed rejection and the app hangs forever on the footer-less, data-backdrop="static" "Validate outputs on Ledger" modal. Restart required; transaction never broadcast.
- Custom-token send: ledger.sendTokens throws synchronously before the flow starts → generic "Unhandled error" modal.
This contradicts the PR's "getSignatures ✅ transaction broadcast successfully" testing claim — no code path in commit ec185a0 can complete that flow, so the device testing must predate the seen parameter. Ask for a re-test on hardware after the fix.
The right fix is to delete restoreBuffers entirely, not repair it. The function can't achieve its stated purpose: ipcRenderer.send re-serializes with the Structured Clone Algorithm (Electron 27 docs), so Buffers created in the preload arrive in the main process as Uint8Array anyway — exactly what main received before this PR. Verifiers confirmed public/ledger.js is already Uint8Array-safe (data.length/data.slice at ledger.js:339-340; @ledgerhq/hw-transport's Buffer.concat accepts Uint8Array). ipcRenderer.send(channel, ...args) is the whole fix. While in there, make the SendTokens.js:293 wrapper .catch → handleSendError so a future rejection surfaces instead of hanging that modal.
P1 — Sentry consent is broken both ways in Electron
Two sides of the same migration, both verified against the pinned @sentry/electron 3.0.7 sources:
1. Opt-out is a silent no-op. sentrySetEnabled: (dsn) => Sentry.init({ dsn, ... }) (preload.js:109) relies on re-init, but the renderer SDK has a one-shot guard (window.__SENTRY__RENDERER_INIT__ in renderer/sdk.js) — after the unconditional startup init with the real DSN (preload.js:13), every later Sentry.init returns early. So updateSentryState → initSentry('') never disables anything; users who decline error reporting keep an active SDK whose events forward via IPC to the main process (also unconditionally init'd, electron.js:17). The "Empty dsn disables it" comment codifies a contract the SDK doesn't honor. (Partially pre-existing at base, but this PR makes sentrySetEnabled the only Electron path, so this is the place to fix it.)
2. Opt-in users lose all reporting. With contextIsolation: true, the preload SDK's global handlers live in the isolated world and never see main-world uncaught errors (at base, the shared window meant auto-capture worked). The browser SDK is never initialized in Electron (wallet.js:461 returns early via the bridge), and ModalUnhandledError.js:87 shows the manual "Send error report" button only when !isSentryAllowed() — so consented users have no path at all for app errors to reach Sentry. This is a real regression introduced by the contextIsolation flip.
Suggested shape: make the bridge API a boolean (sentrySetEnabled(enabled)) with the DSN kept inside the preload (also removes the renderer-supplied-DSN surface); gate captures by flipping the live client (getClient().getOptions().enabled = enabled — checked before the EventToMain processor runs; beforeSend would be too late), ideally starting disabled until consent is read; and capture main-world errors explicitly (e.g. ErrorWrapper's handlers → sentryCapture when consented). Note the related pre-existing bug that becomes load-bearing: ErrorWrapper.js:46 does setError(error) instead of setError(newError) and never passes the error to the modal, so the one surviving report path sends new Error(undefined) with no stack — worth fixing in passing.
P2 � LavaMoat grant keyed to cypress>lodash is a production time bomb
policy-override.json:324 keys the Send-Tokens crash fix to "cypress>lodash". lodash is a phantom dependency � imported by ~36 src files but declared nowhere in package.json; @lavamoat/aa canonicalizes it through cypress (a devDependency) only because it prefers the shortest direct-dep name. The policy regenerates on every production build, so if cypress is removed/upgraded or hoisting shifts, the key silently stops matching (no build error, only a verbosity-gated debug log), the setTimeout grant vanishes � the generated policy can only detect define for lodash � and the ft.setTimeout is not a function crash returns in packaged builds only, invisible to dev/e2e. Fix: add lodash@4.17.21 to dependencies, re-key the override as "lodash", regenerate policy.json.
Minor
- Release tag: both Sentry.init calls in preload use process.env.npm_package_version, which is undefined in packaged builds. Use constants.versionNumber (already required; matches electron.js). Note for the CodeRabbit thread: its suggested constants.VERSION does not exist in public/constants.js, and its "init with dsn: '' then enable later" patch would break enabling due to the same one-shot init guard � so don't apply it as written.
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.
Acceptance Criteria
BrowserWindow:nodeIntegration: true→falseandcontextIsolation: false→true(the warnings Electron itself raises).sandboxis keptfalseso the preload can still use Node to set up the bridge — enabling full sandboxing is a follow-up.window.require('electron')access with acontextBridgeAPI (window.electronAPI) exposed frompreload.js, with a whitelisted IPC surface (send/on/removeAllListeners) plusopenExternal.public/ledger.js) and is reached over the IPC bridge; binary payloads are restored toBufferwhen crossing the bridge sopublic/ledger.jsdoes not change.window.require('@sentry/electron')to the preload (reached via the bridge, outside the LavaMoat-governed bundle);@sentry/browseris kept only as the browser/dev fallback.TypeError: ft.setTimeout is not a function):lodashreadssetTimeoutoff the captured global object and relied on Node'sglobal(which bypassed LavaMoat) — disablingnodeIntegrationexposed the missing grant. Fixed with alavamoat/webpack/policy-override.jsongrant.Security Checklist
@sentry/browserwas already a production dependency (previously the browser/dev fallback).Testing
Validated on a physical Ledger Nano S (Hathor app):
getVersion,getPublicKeyData(xpub import)checkAddresssendTx+getSignatures(the binary path through the bridge); transaction broadcast successfullyThe Electron
nodeIntegration/contextIsolationsecurity warnings are gone from the console; only the unrelated CSP warning remains.Summary by CodeRabbit
Security
New Features
Bug Fixes