Repository navigation
fix(wallet): harden browser-signing bridge (security review) - #383
Merged
Merged
Conversation
…token, 0700, url scrub, body cap) Targeted hardening of the browser-wallet signing bridge per security review; all existing behavior and tests preserved. - Verify signer: reject a wallet result whose from differs from the connected/expected signer, both in the bridge (handleResult) and in browserSend (assertResultSigner); plumb from through sessionClient.waitForTxResult. On accountsChanged the page re-POSTs /api/connected so the daemon stays coherent, and the misleading CLI-will-verify-the-sender copy now matches the enforced behavior. - Host-header validation: reject Host != 127.0.0.1:<port>/localhost:<port> with 403 before route dispatch (validated against the port captured at start() so long-polls flushed during teardown still pass). - Constant-time token compare via length-guarded timingSafeEqual. - Create ~/.genlayer and keystores dir with mode 0o700 + chmodSync. - Scrub the hash token from the URL via history.replaceState. - Cap request bodies at 64KB (413 PayloadTooLargeError). - Warn against ssh -g / GatewayPorts yes / public-interface binding. Existing no-CORS, origin-fail-closed, and token-on-every-route checks are unchanged.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
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
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.
Summary
Targeted hardening of the browser-wallet signing bridge per a security review. The bridge is otherwise sound — this keeps all existing behavior and tests intact and only closes the flagged gaps. Existing no-CORS, origin-fail-closed, and token-on-every-route checks are unchanged.
Changes
fromdiffers from the connected/expected signer, both in the bridge (browserBridge.tshandleResult) and inbrowserSend.ts(assertResultSigner);fromis plumbed throughsessionClient.tswaitForTxResult. OnaccountsChanged, the page re-POSTs/api/connectedso the daemon stays coherent (aborts on disconnect), and the misleading "The CLI will verify the sender" copy now matches the enforced behavior.crypto.timingSafeEqual.Host!=127.0.0.1:<port>/localhost:<port>with 403 before route dispatch. Validated against the port captured atstart()so a long-poll flushed during teardown still passes.~/.genlayerperms. Config + keystores dirs created{recursive:true, mode:0o700}andchmodSync(dir, 0o700).history.replaceStateafter reading the hash token.ssh -g/GatewayPorts yes/ binding a public interface where port-forwarding is suggested.Tests
fromresult is rejected with a clear signer-mismatch error (account-switch case).Hostheader is rejected (403) while loopback (127.0.0.1/localhost) is served.configFileManagerandsessionClienttests for the 0700 perms and thewaitForTxResult{txHash, from}shape.Verification
npm run buildclean.npx vitest run→ 749 passed (72 files); +2 new hardening tests.tsc --noEmit→ 32 errors, identical to the pre-existing baseline (zero new; none in touched files).