fix: render a hostile token symbol as text, and put a floor under the CSP (closes #307) #327

Merged

clawbot merged 1 commits from issue-307-escape-html into next 2026-08-20 13:47:29 +02:00

Conversation 1 Commits 1 Files Changed 24 +706 -97

clawbot commented 2026-08-20 13:35:20 +02:00

Collaborator

Copy Link

Closes #307.

The defect

A token's symbol is whatever its symbol() returns, the block explorer passes it through unfiltered, and balanceLine() interpolated it into an innerHTML string. A token with the 1,000 holders the spam filter asks for, airdropped to the victim, could paint a full-viewport cross-origin iframe over the wallet's own UI — on the screens where the user is used to typing their password.

What changed

escapeHtml moved to a new DOM-free src/shared/html.js as a pure string replace over &, <, >, " and ', re-exported from helpers.js so no caller moved. The implementation it replaces round-tripped through a detached element's textContent, which escapes neither quote character — and it was already used inside data-copy="...", so quote escaping was load-bearing before this change, not after.

Every interpolation into an innerHTML string across src/popup/views/ was audited, not just the reported one. Also unescaped, and now fixed:

• the transaction lists' direction label in all three lists — the explorer's own method name for a contract call, attacker-chosen for an attacker's contract;
• the wallet name and the ENS name in the Home wallet list (renderAddressHtml escaped its copy of the ENS name; this list did not);
• the URL in the explorer link's href, plus the blockie data: URI in two views;
• the confirmation screen's warning line, which carries only fixed strings today but is one wiring change from src/shared/etherscanLabels.js, which builds one out of scraped explorer markup;
• the bundled-token-list attributes in addToken.js and settingsAddToken.js, which were using an ad-hoc .replace(/"/g, """).

Explorer URLs are now built by one explorerUrl(kind, value) helper that percent-encodes the path segment, so a from/to out of explorer JSON cannot re-point the link elsewhere in the explorer. What stays bare is markup fragments this code just built, loop indices and locally computed numbers; the rule and its reason are stated at the top of helpers.js so the grep stays readable.

CSP. Both manifests now declare default-src 'self' with frame-src 'none'. default-src 'self' on its own breaks the popup, so the policy is written out:

directive	why it is not 'self'
style-src 'self' 'unsafe-inline'	index.html and the view helpers set presentation through style="..." attributes. Chrome enforces style-src on attributes, and Firefox has never implemented style-src-attr, so there is no narrower spelling that works on both. Script stays under script-src, which does not allow 'unsafe-inline'.
img-src 'self' data:	blockies are data: PNGs assigned to img.src.
connect-src 'self' https: http:	the RPC endpoint is user-configurable and a local node over http://127.0.0.1 is supported — the Firefox e2e suite runs on exactly that.
frame-src 'none', form-action 'none', base-uri 'none'	named rather than inherited: the last two do not fall back to default-src at all.

tests/manifest.test.js now pins the whole directive set exactly, in both directions; README.md carries the reasoning.

Length cap. Displayed symbols are capped at 12 characters in src/shared/symbolDisplay.js — the bound lookupTokenInfo() already applied to a symbol read straight off a contract, while the explorer path had none. The longest symbol in the 512-entry bundled list is MSYRUPUSDP at 10, so no real token is truncated. The module states that it is a layout bound and not the security control. isSpoofedSymbol is untouched.

Verification

make check green: 39 suites, 811 tests; lint ran in the container (#11 [lint 1/1] RUN make lint, not CACHED), prettier clean.

make test-e2e (Chrome) 55/55, including the new browser test:

# stub token symbol() now returns: "<iframe id=\"pwn\" src=\"https://dapp.e2e.test/\" style=\"position:fixed;left:0;top:0;width:360px;height:600px;z-index:99999\"></iframe>"
# address-detail iframes = 0
# iframes in the popup DOM = 0 | #pwn present = false | symbol = "<iframe id=…"
ok 44 - a token whose symbol() returns markup renders as text (#307)

make test-e2e-firefox 8/8 steps, which is what proves the CSP change does not break the popup in the other engine — the loopback http:// dApp and the http://127.0.0.1 stub node both still reachable under connect-src. ok 2 - the popup compiles WebAssembly under the shipped CSP (#182) still passes in Chrome, so libsodium is still on the WASM backend.

Verified failing first, four ways:

1. Restricting escapeHtml to & < > — the escape the old textContent round trip actually performed — fails 5 unit tests, among them keeps a quote-bearing token id inside its attribute.
2. Removing the length cap fails 3.
3. Dropping default-src from manifest/chrome.json fails 2 manifest tests.
4. Removing both the escape and the cap and running the whole Chrome suite fails the new browser test with the attack reproduced verbatim — Playwright reported <iframe id="pwn" src="https://dapp.e2e.test/"></iframe> from <div class="mt-3">…</div> subtree intercepts pointer events on the Back button. Note that frame-src 'none' stops such a frame loading but not existing, which is why the test asserts the iframe count and the literal rendered text separately, and why the count is taken on the address screen before anything is clicked.

Not in this PR

https://git.eeqj.de/sneak/AutistMask/issues/322 and https://git.eeqj.de/sneak/AutistMask/issues/323 are on the same screens and are deliberately untouched.

Closes https://git.eeqj.de/sneak/AutistMask/issues/307. ## The defect A token's symbol is whatever its `symbol()` returns, the block explorer passes it through unfiltered, and `balanceLine()` interpolated it into an `innerHTML` string. A token with the 1,000 holders the spam filter asks for, airdropped to the victim, could paint a full-viewport cross-origin iframe over the wallet's own UI — on the screens where the user is used to typing their password. ## What changed **`escapeHtml`** moved to a new DOM-free `src/shared/html.js` as a pure string replace over `&amp;`, `&lt;`, `&gt;`, `"` and `'`, re-exported from `helpers.js` so no caller moved. The implementation it replaces round-tripped through a detached element's `textContent`, which escapes neither quote character — and it was already used inside `data-copy="..."`, so quote escaping was load-bearing before this change, not after. **Every interpolation into an `innerHTML` string across `src/popup/views/` was audited**, not just the reported one. Also unescaped, and now fixed: - the transaction lists' direction label in all three lists — the explorer's own method name for a contract call, attacker-chosen for an attacker's contract; - the wallet name and the ENS name in the Home wallet list (`renderAddressHtml` escaped its copy of the ENS name; this list did not); - the URL in the explorer link's `href`, plus the blockie `data:` URI in two views; - the confirmation screen's warning line, which carries only fixed strings today but is one wiring change from `src/shared/etherscanLabels.js`, which builds one out of scraped explorer markup; - the bundled-token-list attributes in `addToken.js` and `settingsAddToken.js`, which were using an ad-hoc `.replace(/"/g, "&amp;quot;")`. Explorer URLs are now built by one `explorerUrl(kind, value)` helper that percent-encodes the path segment, so a `from`/`to` out of explorer JSON cannot re-point the link elsewhere in the explorer. What stays bare is markup fragments this code just built, loop indices and locally computed numbers; the rule and its reason are stated at the top of `helpers.js` so the grep stays readable. **CSP.** Both manifests now declare `default-src 'self'` with `frame-src 'none'`. `default-src 'self'` on its own breaks the popup, so the policy is written out: | directive | why it is not `'self'` | | --- | --- | | `style-src 'self' 'unsafe-inline'` | `index.html` and the view helpers set presentation through `style="..."` attributes. Chrome enforces `style-src` on attributes, and Firefox has never implemented `style-src-attr`, so there is no narrower spelling that works on both. Script stays under `script-src`, which does not allow `'unsafe-inline'`. | | `img-src 'self' data:` | blockies are `data:` PNGs assigned to `img.src`. | | `connect-src 'self' https: http:` | the RPC endpoint is user-configurable and a local node over `http://127.0.0.1` is supported — the Firefox e2e suite runs on exactly that. | | `frame-src 'none'`, `form-action 'none'`, `base-uri 'none'` | named rather than inherited: the last two do not fall back to `default-src` at all. | `tests/manifest.test.js` now pins the whole directive set exactly, in both directions; README.md carries the reasoning. **Length cap.** Displayed symbols are capped at 12 characters in `src/shared/symbolDisplay.js` — the bound `lookupTokenInfo()` already applied to a symbol read straight off a contract, while the explorer path had none. The longest symbol in the 512-entry bundled list is `MSYRUPUSDP` at 10, so no real token is truncated. The module states that it is a layout bound and not the security control. `isSpoofedSymbol` is untouched. ## Verification `make check` **green**: 39 suites, 811 tests; lint ran in the container (`#11 [lint 1/1] RUN make lint`, not `CACHED`), prettier clean. `make test-e2e` (Chrome) **55/55**, including the new browser test: ``` # stub token symbol() now returns: "<iframe id=\"pwn\" src=\"https://dapp.e2e.test/\" style=\"position:fixed;left:0;top:0;width:360px;height:600px;z-index:99999\"></iframe>" # address-detail iframes = 0 # iframes in the popup DOM = 0 | #pwn present = false | symbol = "<iframe id=…" ok 44 - a token whose symbol() returns markup renders as text (#307) ``` `make test-e2e-firefox` **8/8 steps**, which is what proves the CSP change does not break the popup in the other engine — the loopback `http://` dApp and the `http://127.0.0.1` stub node both still reachable under `connect-src`. `ok 2 - the popup compiles WebAssembly under the shipped CSP (#182)` still passes in Chrome, so libsodium is still on the WASM backend. **Verified failing first, four ways:** 1. Restricting `escapeHtml` to `&amp; &lt; &gt;` — the escape the old `textContent` round trip actually performed — fails 5 unit tests, among them `keeps a quote-bearing token id inside its attribute`. 2. Removing the length cap fails 3. 3. Dropping `default-src` from `manifest/chrome.json` fails 2 manifest tests. 4. Removing **both** the escape and the cap and running the whole Chrome suite fails the new browser test with the attack reproduced verbatim — Playwright reported `<iframe id="pwn" src="https://dapp.e2e.test/"></iframe> from <div class="mt-3">…</div> subtree intercepts pointer events` on the Back button. Note that `frame-src 'none'` stops such a frame *loading* but not *existing*, which is why the test asserts the iframe count and the literal rendered text separately, and why the count is taken on the address screen before anything is clicked. ## Not in this PR `https://git.eeqj.de/sneak/AutistMask/issues/322` and `https://git.eeqj.de/sneak/AutistMask/issues/323` are on the same screens and are deliberately untouched.

clawbot added 1 commit 2026-08-20 13:35:20 +02:00

fix: render a hostile token symbol as text, and put a floor under the CSP (closes #307) ... ada41bf5e1

A token's symbol is whatever its symbol() returns, the block explorer passes it
through unfiltered, and balanceLine() interpolated it into an innerHTML string.
A token with the 1,000 holders the spam filter asks for, airdropped to the
victim, could therefore paint a full-viewport cross-origin iframe over the
wallet's own UI, on the screens where the user is used to typing their password.

escapeHtml moves to the new src/shared/html.js as a pure string replace over &,
<, >, " and '. The implementation it replaces round-tripped through a detached
element's textContent, which escapes neither quote character, and it was already
in use inside data-copy="..." and would have been inside href="...". Being pure
also makes it testable without a DOM shim.

Every interpolation into an innerHTML string across src/popup/views/ was audited
rather than only the reported one. Also unescaped: the transaction lists'
direction label (the explorer's method name, attacker-chosen for an attacker's
contract), the wallet name and ENS name in the Home wallet list, the URL in the
explorer link's href, the blockie data: URI, and the confirmation screen's
warning line, which carries only fixed strings today but is one wiring change
from carrying scraped explorer text. Explorer URLs are now built by one helper
that percent-encodes the path segment, so a from/to out of explorer JSON cannot
re-point the link. Where a value is a markup fragment this code just built, or a
loop index, or a locally computed number, it stays bare; the rule and the reason
are stated at the top of helpers.js.

Both manifests now declare default-src 'self' with frame-src 'none'. Four
directives had to stay looser than 'self' and none of them generalises:
style-src needs 'unsafe-inline' because the popup sets presentation through
style="..." attributes and Firefox has never implemented style-src-attr; img-src
needs data: for the blockies; connect-src needs https: and http: because the RPC
endpoint is user-configurable and a local node over http://127.0.0.1 is a
supported configuration. frame-src, form-action and base-uri are named rather
than inherited, because the last two do not fall back to default-src at all.
tests/manifest.test.js now pins the whole directive set exactly, in both
directions, and README.md carries the reasoning.

Displayed symbols are capped at 12 characters, the bound lookupTokenInfo()
already applied to a symbol read straight off a contract; the explorer path had
none. The cap is a layout bound and is documented as not being the security
control. isSpoofedSymbol() is untouched: it answers whether a symbol collides
with a known ticker, which is a different question, and repurposing it here
would have been the wrong control.

Verified failing first, four ways. Restricting escapeHtml to & < > (the escape
the old textContent round trip actually performed) fails 5 unit tests including
the data-copy attribute break-out. Removing the length cap fails 3. Dropping
default-src from manifest/chrome.json fails 2. Removing both the escape and the
cap and running the full Chrome suite fails the new browser test with the
attack reproduced: an <iframe id="pwn"> in the popup DOM, intercepting pointer
events over the Back button.

clawbot added the needs-review label 2026-08-20 13:35:23 +02:00

clawbot self-assigned this 2026-08-20 13:35:24 +02:00

clawbot referenced this pull request 2026-08-20 13:35:37 +02:00

harden: a hostile ERC-20 symbol renders as live HTML in the popup — cross-origin iframe over the wallet UI #307

clawbot commented 2026-08-20 13:46:48 +02:00

Author

Collaborator

Copy Link

PASS. Independently re-derived the ${ audit over src/popup/views/, re-ran everything, and reproduced two of the four mutations; nothing blocking.

Evidence the checks ran: make check green here (39 suites / 811 tests; lint stage #11 [lint 1/1] RUN make lint ... DONE 5.1s, not CACHED; prettier clean). make test-e2e 55/55 including ok 44 and ok 2 - the popup compiles WebAssembly under the shipped CSP (#182). make test-e2e-firefox 8/8, with no manifest-CSP parse error in the Firefox console output. No CSP violation appeared in either browser log, and that is asserted rather than merely unobserved: tests/e2e/harness.js:114 records every page console.error and run.js:3548-3551 fails the run on one, so a Chrome "Refused to apply inline style" would have failed the suite. Mutations reproduced independently: cutting escapeHtml to & < > fails 5 tests including keeps a quote-bearing token id inside its attribute; reverting only balanceLine's interpolation to ${symbol} gives not ok 44 ... the address screen contains 2 iframe(s), taken before any click, so the browser test is not vacuous. Both reverted, tree clean, no containers left, e2e image tags as found. explorerUrl() probed with ", >, javascript:, ../, newline, ?/#, ' and an astral character: all neutralised, and the ' that encodeURIComponent leaves alone is caught by the escape into href="...".

Non-blocking findings, in descending order:

1. src/shared/networks.js:47 — explorerLink(network, type, value) is an exported, un-encoded twin of the new explorerUrl(). It is dead code and was dead before this PR, so it is not a regression, but this PR's stated remediation is "one helper that percent-encodes the path segment" and this leaves a second exported builder that does not. Acceptable: delete it, or make it delegate to the encoding one.
2. src/popup/views/addressToken.js:202 — ${tokenDecimals} is the one value-shaped interpolation left bare, against the rule this PR itself writes at the top of helpers.js. Traced every writer: balances.js:75 parseInt, lookupTokenInfo Number(), settingsAddToken.js:43,104 parseInt, bundled list numeric, and no dApp-writable path into trackedTokens. Safe today; it is only the greppable rule that does not hold.
3. src/popup/views/home.js:267 interpolates addrTotal bare while deleteAddress.js:96 escapes the identical formatAddressTotal() output. Both safe, inconsistent with each other.
4. displaySymbol() slices at a UTF-16 boundary, so an astral-plane symbol truncates onto a lone surrogate — verified: 20 emoji in gives "\ud83d…" as the 12th/11th units, which renders U+FFFD. Cosmetic, escaping unaffected.

Opinion on the three loosenings, all acceptable to ship:

• style-src 'unsafe-inline' — the Firefox claim is accurate (style-src-attr was never implemented there, nor 'unsafe-hashes'), so no narrower spelling works on both engines while the 39 static style="..." attributes in src/popup/index.html and the view helpers remain. Note for the record that the 57 .style.foo = assignments are CSSOM and CSP-exempt, so they are not what forces this — only the 39 attributes are, and moving those to classes would remove the loosening entirely. Residual risk is small: with the escaping in place nothing can inject a style attribute, and the CSS exfiltration channels are shut anyway by img-src 'self' data: and font-src falling back to default-src 'self'. Worth a follow-up issue; I did not file one.
• img-src data: — no injection route. The blockie URI is makeBlockie() over a hex address and is now escaped into src="..."; data: appears in no script-fetching directive, and SVG in <img> does not script.
• connect-src https: http: — the weakest of the three and it cannot be narrowed: a static manifest cannot know a user-configured RPC endpoint. It gives back nothing on the #307 attack, since injected markup cannot run script under script-src 'self' and form-action 'none' plus img-src close the non-script channels. Two things worth knowing: ws:/wss: are not matched by http:/https: and so fall back to default-src 'self', i.e. blocked — harmless, because the wallet only builds JsonRpcProvider over fetch (balances.js:28) and settings.js validates an endpoint by fetching it. The real residual cost is supply-chain, a malicious bundled dependency exfiltrating to any origin, which is pre-existing and not a regression here.

Confirmed and not repeated: all six DoD items; the walletDefectHtml reasoning (only the frozen DEFECTS constants reach it, and the module already says so); isSpoofedSymbol untouched and nothing relying on the 12-char cap for safety; both manifests byte-identical and pinned exactly in both directions; CI green on ada41bf; merges clean into next and main; single commit ending (closes #307) with TODO.md in it, author and committer both clawbot; no Claude/Anthropic reference or attribution trailer anywhere in the diff or the commit message; no scope creep; make fmt clean. The disclosed one-off raw node -e left no residue in the committed tree.

PASS. Independently re-derived the `${` audit over `src/popup/views/`, re-ran everything, and reproduced two of the four mutations; nothing blocking. Evidence the checks ran: `make check` green here (39 suites / 811 tests; lint stage `#11 [lint 1/1] RUN make lint ... DONE 5.1s`, not `CACHED`; prettier clean). `make test-e2e` 55/55 including `ok 44` and `ok 2 - the popup compiles WebAssembly under the shipped CSP (#182)`. `make test-e2e-firefox` 8/8, with no manifest-CSP parse error in the Firefox console output. No CSP violation appeared in either browser log, and that is asserted rather than merely unobserved: `tests/e2e/harness.js:114` records every page `console.error` and `run.js:3548-3551` fails the run on one, so a Chrome "Refused to apply inline style" would have failed the suite. Mutations reproduced independently: cutting `escapeHtml` to `&amp; &lt; &gt;` fails 5 tests including `keeps a quote-bearing token id inside its attribute`; reverting only `balanceLine`'s interpolation to `${symbol}` gives `not ok 44 ... the address screen contains 2 iframe(s)`, taken before any click, so the browser test is not vacuous. Both reverted, tree clean, no containers left, e2e image tags as found. `explorerUrl()` probed with `"`, `&gt;`, `javascript:`, `../`, newline, `?`/`#`, `'` and an astral character: all neutralised, and the `'` that `encodeURIComponent` leaves alone is caught by the escape into `href="..."`. Non-blocking findings, in descending order: 1. `src/shared/networks.js:47` — `explorerLink(network, type, value)` is an exported, **un**-encoded twin of the new `explorerUrl()`. It is dead code and was dead before this PR, so it is not a regression, but this PR's stated remediation is "one helper that percent-encodes the path segment" and this leaves a second exported builder that does not. Acceptable: delete it, or make it delegate to the encoding one. 2. `src/popup/views/addressToken.js:202` — `${tokenDecimals}` is the one value-shaped interpolation left bare, against the rule this PR itself writes at the top of `helpers.js`. Traced every writer: `balances.js:75` `parseInt`, `lookupTokenInfo` `Number()`, `settingsAddToken.js:43,104` `parseInt`, bundled list numeric, and no dApp-writable path into `trackedTokens`. Safe today; it is only the greppable rule that does not hold. 3. `src/popup/views/home.js:267` interpolates `addrTotal` bare while `deleteAddress.js:96` escapes the identical `formatAddressTotal()` output. Both safe, inconsistent with each other. 4. `displaySymbol()` slices at a UTF-16 boundary, so an astral-plane symbol truncates onto a lone surrogate — verified: 20 emoji in gives `"\ud83d…"` as the 12th/11th units, which renders U+FFFD. Cosmetic, escaping unaffected. Opinion on the three loosenings, all **acceptable to ship**: - `style-src 'unsafe-inline'` — the Firefox claim is accurate (`style-src-attr` was never implemented there, nor `'unsafe-hashes'`), so no narrower spelling works on both engines while the 39 static `style="..."` attributes in `src/popup/index.html` and the view helpers remain. Note for the record that the 57 `.style.foo =` assignments are CSSOM and CSP-exempt, so they are not what forces this — only the 39 attributes are, and moving those to classes would remove the loosening entirely. Residual risk is small: with the escaping in place nothing can inject a `style` attribute, and the CSS exfiltration channels are shut anyway by `img-src 'self' data:` and `font-src` falling back to `default-src 'self'`. Worth a follow-up issue; I did not file one. - `img-src data:` — no injection route. The blockie URI is `makeBlockie()` over a hex address and is now escaped into `src="..."`; `data:` appears in no script-fetching directive, and SVG in `&lt;img&gt;` does not script. - `connect-src https: http:` — the weakest of the three and it cannot be narrowed: a static manifest cannot know a user-configured RPC endpoint. It gives back nothing on the #307 attack, since injected markup cannot run script under `script-src 'self'` and `form-action 'none'` plus `img-src` close the non-script channels. Two things worth knowing: `ws:`/`wss:` are not matched by `http:`/`https:` and so fall back to `default-src 'self'`, i.e. blocked — harmless, because the wallet only builds `JsonRpcProvider` over fetch (`balances.js:28`) and `settings.js` validates an endpoint by fetching it. The real residual cost is supply-chain, a malicious bundled dependency exfiltrating to any origin, which is pre-existing and not a regression here. Confirmed and not repeated: all six DoD items; the `walletDefectHtml` reasoning (only the frozen `DEFECTS` constants reach it, and the module already says so); `isSpoofedSymbol` untouched and nothing relying on the 12-char cap for safety; both manifests byte-identical and pinned exactly in both directions; CI green on `ada41bf`; merges clean into `next` and `main`; single commit ending ` (closes #307)` with `TODO.md` in it, author and committer both `clawbot`; no Claude/Anthropic reference or attribution trailer anywhere in the diff or the commit message; no scope creep; `make fmt` clean. The disclosed one-off raw `node -e` left no residue in the committed tree.

clawbot merged commit c8c2af0c6b into next 2026-08-20 13:47:29 +02:00

clawbot referenced this pull request 2026-08-20 13:47:43 +02:00

harden: move the 39 static style attributes onto classes and drop 'unsafe-inline' from style-src #328

clawbot referenced this pull request 2026-08-20 13:47:53 +02:00

chore: escaping hygiene left after #307 — a dead un-encoded explorer helper, two inconsistent bare interpolations, a UTF-16 slice #329

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

merge-ready

needs-checks

needs-rebase

needs-review

needs-rework

No Label needs-review

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clawbot sneak

No Assignees clawbot

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/AutistMask#327