Repository navigation
fix(security): the seed must not sit in the DOM, and the guard must cover more than fetch - #7
Merged
Merged
Conversation
… transports and secrets the guard missed Three defects, all of which let the exact string the wallet displays reach somewhere it should not. 1. A confirmed reveal put the seed in the DOM for the rest of the session with nothing over it but `filter: blur(6px)`. Dev tools, the accessibility tree, screen readers and extensions read straight through a paint instruction, and every unrelated re-render — a balance change, a claim — put it back. SPEC 6.6 friction requirement 4 rules this out by name. `mayRenderSeed` now requires the press as well as the confirmation, `isUnblurred` is gone, and the panel renders a press-and-hold button where the value used to sit. `getSeed` is called on exactly one branch. 2. The secret list was seeds only, and matched case-sensitively. `normalizeSeed()` upper-cases and `Kei.start()` never writes the normalized form back, so after a lower-case import storage holds one string while `kei.seed` returns another — and the private key, which spends the wallet just as completely, was never on the list at all. `walletSecrets()` now records both seed forms and the derived private key, and `hasSecret` is case-insensitive. 3. Only `fetch` was wrapped. `guardKnownTransports` now also covers `XMLHttpRequest` (url, headers, body), `navigator.sendBeacon`, `WebSocket` (url and frames), `EventSource`, and `navigator.clipboard.writeText`. A body it cannot decode blocks rather than passes. The header comment and SPEC 1.2 no longer call this a tripwire: `Image.src`, CSS `url()`, form actions, `<a ping>`, service workers, WebRTC and dynamic `import()` all leave without passing through it, and the CSP is what actually bounds exfiltration. What is here catches a regression in this app or the SDK, which is a real and different job. The duplicated secret list exists only because `containsSecret` / `scrub` / `registerSecret` are not re-exported by the `kei-transaction` umbrella (keicoin-org/kei-transaction#138).
…op the hold on focus loss
Follow-up to the previous commit, which stopped at the network transports.
SPEC §6.6 names logs, error messages, analytics, URLs and query strings
alongside network requests, and the guard covered none of those.
Newly covered, each with a negative test that fails without its guard:
`window.open`, `navigator.clipboard.write` (ClipboardItem) and
`document.execCommand('copy' | 'cut')`, `history.pushState` and
`replaceState`, `window.postMessage` plus `MessagePort` and `Worker`,
`console.*`, and `document.cookie` — the one store that leaves the machine
without anybody writing the code that sends it.
`describe()` flattens an arbitrary payload — a postMessage body, a console
argument, a history state — into scannable text, bounded in depth and node
count, cycle-safe, and returning null (which blocks) for anything it cannot
read synchronously.
The module header now carries an explicit not-covered list rather than
implying the covered one is complete: markup that fetches, `location.href`
(a non-configurable accessor), WebRTC, dynamic `import()`, a service
worker's own requests, uncaught exceptions the browser prints itself,
`localStorage` (where the seed lives by design), and anything in another
realm or holding a pre-install reference. SPEC §1.2 and the §6 invariant
table say the same.
Two more holes:
- A press ends on mouseup, mouseleave or touchend, none of which fire when
the window loses focus or the tab is hidden. Alt-tabbing mid-press left
the seed sitting in the DOM — the exact state friction requirement 4
exists to prevent. `App` now releases the hold on `blur` and
`visibilitychange`.
- `keyPairFromSeed` is async and the derivation was fire-and-forget, so the
private key reached the list a tick after the seed. `connectWallet` now
awaits it, and records `kei.seed` itself so the list holds the normalized
form the wallet displays rather than only the raw stored one. Stated
honestly in the test: the invariant also held by accident before, because
`Kei.start()` awaits enough internally. The await makes it not an accident.
`activeSecrets` now ignores anything under 16 characters, mirroring
`registerSecret` in `@keicoin/core`: a short entry would match nearly every
request and turn a leak check into an outage.
`tests/wallet-secrets.test.ts` now asserts through
`createSeedGuardedFetch(…, walletSecrets)` — the real list feeding the real
comparison — instead of reimplementing `hasSecret` locally, where a test
would have passed whatever the check happened to do. Verified failing
against the pre-rescue behaviour.
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.
Rescued from an uncommitted worktree, then finished. Three defects, all of which let the exact string the wallet displays reach somewhere it should not, plus the sinks the guard never looked at.
The wallet is "the exit door — what makes 'players own their items' true rather than rhetorical." A wallet that leaks a seed destroys that claim, so the emphasis below is on what is measured, and on saying plainly what is not covered.
1. A confirmed reveal left the seed in the DOM for the session
mayRenderSeedreturned true onconfirmedalone, and the value was hidden withfilter: blur(6px). Dev tools, the accessibility tree, screen readers and extensions read straight through a paint instruction, and every unrelated re-render — a balance change, a claim — put it back. SPEC §6.6 friction requirement 4 rules this out by name.mayRenderSeednow requiresconfirmed && held.isUnblurredand the blur CSS are gone; the panel renders a press-and-hold button where the value used to sit, andgetSeedis called on exactly one branch. The old test asserted the defect (expect(panel.textContent).toContain(seed)for a confirmed, unheld state) and now asserts the opposite.Also: a press ends on
mouseup,mouseleaveortouchend, none of which fire when the window loses focus or the tab is hidden. Alt-tabbing mid-press left the seed in the DOM.Appnow releases the hold onblurandvisibilitychange.2. The secret list was seeds only, and matched case-sensitively
normalizeSeed()upper-cases andKei.start()never writes the normalized form back, so after a lower-case import storage holds one string whilekei.seed— what the wallet displays, and what a leak would carry — is another. A case-sensitiveincludesover the stored form was blind to the displayed form. This is the most likely real leak, not an exotic one. The private key, which spends the wallet just as completely, was never on the list at all.walletSecrets()now records the stored seed, the normalized formkei.seedreturns, and the derived private key;hasSecretis case-insensitive.activeSecretsignores anything under 16 characters, mirroringregisterSecret— a short entry would match nearly every request and turn a leak check into an outage.keyPairFromSeedis async and the derivation was fire-and-forget, so the key reached the list a tick after the seed.connectWalletnow awaits it. Stated honestly in the test: that invariant also held by accident before, becauseKei.start()happens to await enough internally. The await makes it not an accident.3. Only
fetchwas wrappedCovered — each with a negative test that fails without its guard
fetch,XMLHttpRequest(url, headers, body),navigator.sendBeacon,WebSocket(url and frames),EventSource(url),window.opennavigator.clipboard.writeTextand.write,document.execCommand('copy' | 'cut')history.pushState,history.replaceStatewindow.postMessage,MessagePort,Workerconsole.logand siblingsdocument.cookie— the one store that leaves on its ownA body or payload the guard cannot read synchronously blocks rather than passes: one it cannot read is one it cannot clear.
Not covered — stated so the table above is not read as "all of them"
Image.src,<script src>,<link href>, CSSurl(),<a ping>,<form action>. No interception point exists.default-src 'none',img-src 'self' data:andform-action 'none'are what close these.location.href/location.assign/location.replace— non-configurable accessors onLocation, so they cannot be wrapped. Nothing in this app assigns them.RTCDataChannel.send) and dynamicimport().console.*.localStorage— where the seed lives by design (kei:seed:*). Guarding it would guard the wallet against working.The guard is not the control. The CSP in SPEC §1.2 is:
script-src 'self'with no inline script leaves an injected payload nothing to run from. This file catches a regression in this app or the SDK, which is a real and different job from stopping an attacker. The file is no longer named or described as a "tripwire", and SPEC §1.2 and the §6 invariant table now say the same.The durable fix belongs in kei-transaction
This wallet maintains a duplicate of a list
@keicoin/corealready keeps.registerSecretcovers both the seed and the private key in three case variants, andcontainsSecretqueries it — but none ofcontainsSecret/scrub/registerSecretis re-exported by thekei-transactionumbrella (kei-transaction#138), and this wallet depends on the umbrella only. So the weaker local copy existed because the strong one was not importable.The two defects in §2 above are exactly the two ways cronoh#138 predicted the copy would be weaker. That is direct evidence the problem is not theoretical. Once cronoh#138 ships,
hasSecretand most ofwalletSecrets()should be deleted in favour ofcontainsSecret. The local check is fixed correctly here rather than weakened to match.The UTF-8 finding is already fixed on master
An audit reported
src/ui/panels/wallet-summary.tsas invalid UTF-8 — bare0x97/0x85/0x95bytes rendering five strings as U+FFFD, including every truncated address. Verified withiconv/odrather than by eye:ef3bab0(the master the audit saw): invalid, 5 suspicious bytes.6eefae3onward: valid UTF-8, 0 suspicious bytes.6eefae3is now an ancestor oforigin/master(f245221), so this landed separately while this branch was in progress. Nothing to do here, and no source file in this branch is invalid UTF-8.Verification
npm run typecheck,vitest run(53 tests, 7 files) andnpm run buildall pass. There is no CI workflow in this repo, so these were run locally — nothing here was confirmed by a green check.Every channel claim was checked by reverting the corresponding guard and confirming the test fails: 16 of 16 channel tests fail against fetch-only behaviour, and the lower-case-import and private-key tests fail against the pre-rescue list and comparison.