Fixes from the security review (issue #6):
1. **Unpredictable approval IDs** — replaced sequential `nextApprovalId++` with `crypto.randomUUID()`
2. **Origin validation** — derive RPC origin from trusted `sender.origin` / `sender.tab.url` instead of trusting `msg.origin` from content script
3. **Popup message sender validation** — approval/response messages now verified to come from extension URL only
4. **eth_sign danger warning** — prominent red warning for raw hash signing requests
5. **Secret zeroing** — best-effort clearing of decrypted secrets after use
6. **Password min length 8→12** — ⚠️ NOTE: this is a policy change not specified in RULES.md, flagged for sneak to approve or revert
7. **TODO comments** for password plaintext over runtime.sendMessage (future improvement)
`make check` passes (after `yarn install`).
Closes #6
sneak
was assigned by clawbot2026-02-27 20:38:42 +01:00
✅ No RULES.md / linter / Makefile changes ✅crypto.randomUUID() — correct fix for predictable approval IDs ✅ Origin derivation from sender.origin / sender.tab.url — proper defense against content script origin spoofing ✅ Popup sender validation using runtime.getURL() — prevents external pages from sending approval responses ✅ eth_sign warning — appropriate, this is a dangerous method ✅ Secret zeroing — best-effort is the right approach for JS ✅make check passes
⚠️Flag: Password minimum changed 8→12 (commits in addWallet.js and importKey.js). RULES.md does not specify a minimum. This is a security policy decision for sneak to approve or revert.
## Code Review
**LGTM with one flag:**
✅ No RULES.md / linter / Makefile changes
✅ `crypto.randomUUID()` — correct fix for predictable approval IDs
✅ Origin derivation from `sender.origin` / `sender.tab.url` — proper defense against content script origin spoofing
✅ Popup sender validation using `runtime.getURL()` — prevents external pages from sending approval responses
✅ eth_sign warning — appropriate, this is a dangerous method
✅ Secret zeroing — best-effort is the right approach for JS
✅ `make check` passes
⚠️ **Flag**: Password minimum changed 8→12 (commits in addWallet.js and importKey.js). RULES.md does not specify a minimum. This is a security policy decision for sneak to approve or revert.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes from the security review (issue #6):
nextApprovalId++withcrypto.randomUUID()sender.origin/sender.tab.urlinstead of trustingmsg.originfrom content scriptmake checkpasses (afteryarn install).Closes #6
Code Review
LGTM with one flag:
✅ No RULES.md / linter / Makefile changes
✅
crypto.randomUUID()— correct fix for predictable approval IDs✅ Origin derivation from
sender.origin/sender.tab.url— proper defense against content script origin spoofing✅ Popup sender validation using
runtime.getURL()— prevents external pages from sending approval responses✅ eth_sign warning — appropriate, this is a dangerous method
✅ Secret zeroing — best-effort is the right approach for JS
✅
make checkpasses⚠️ Flag: Password minimum changed 8→12 (commits in addWallet.js and importKey.js). RULES.md does not specify a minimum. This is a security policy decision for sneak to approve or revert.