fix(background): bind what the delivery layer swallows, redact what it stringifies

[ost-ptk]

Closes WALLET-1393.

No failure at the background delivery layer is invisible in the log any more, and no error path stringifies a whole action.

1. deliverViaOrigin binds its rejection

deliver-via-origin.ts returned a silent 0 for both "the tabs.query broadcast blew up" and "no active same-origin tab was open". PR #1427 added a consumer that picks the banner copy from that 0, so the ambiguity became load-bearing. The rejection is now logged with origin + action.type; the return value is unchanged.

This fallback has three production consumers — sdk-response-to-tab.ts (×2) and cancel-requests.ts (×2) — so all four delivery paths gain the diagnostic.

2. The primary delivery catch binds too — and says what came of it

The catch around tabs.sendMessage dispatched a sagaError but never logged the cause, collapsing Could not establish connection, Extension context invalidated, DataCloneError and Safari-specific rejections into one indistinguishable outcome.

Binding the cause alone would have left a second ambiguity in place: a signature recovered through another same-origin tab logged identically to one that was destroyed, because the outcome lived only in the dispatched banner, which no support reader sees. The log therefore carries delivered alongside requestId / tabId / type, and splits severity on it — warn when the fallback recovered, error when the response was lost. That is the same warn/error split this file already applies to benign versus lost duplicate responses sixty lines above.

3. Error paths no longer serialize a whole action

Three sites in background/index.ts threw Error('… ' + JSON.stringify(action)). These messages are handed to sendError(error), which returns them across the boundary — to dispatchToMainStore for the redux branch and into the dapp's SDK for the sdk branch — and an action payload can carry signatureHex / encryptedMessage. Not reachable today for a signature-bearing action per the parity test, so this is defense in depth.

Each site reports identifiers only. meta.requestId is deliberately kept on the SDK one: it is the correlation key every other log at this layer uses, it costs nothing in exposure (the error rejects the originating dapp's own promise, so the receiver already knows the value), and isSDKMethod guarantees it is a string. Unknown redux action: <TYPE> stays the signal for a missing entry in the forwarding allow-list and reads better than the stringified blob did.

4. The same sweep in the content-script relay

handleSdkMessage already applies this rule ten lines above the offender — its no-port branch logs type + requestId only, with a SECURITY comment noting these envelopes carry signatureHex / encryptedMessage — and then its default branch threw with the whole envelope stringified, as did emitSdkEvent's.

This one matters more than the background half it mirrors: a content script's console is the dapp page's console, while the background's is the service worker's. Same reachability (the default fires when a new response type is added and not listed in the switch), so likewise defense in depth. detail: JSON.stringify(message.payload) is untouched — that is the page-event delivery mechanism, not an error path.

Tests

Assertions added to the five existing cases that already drive these branches (fallback emit THROWS ×2, delivery rejects ×3), plus one new case pinning that the delivery-failure log carries identifiers and no signature material.

The payload guard needed care to be worth anything: JSON.stringify cannot see an Error's message (non-enumerable), so asserting on JSON.stringify(mock.calls) vetted nothing about the Error argument — the one part of the log whose text this code does not control. It now goes through a serializer that unwraps Errors, verified against two deliberate mutations: the action passed as an extra log argument, and the action embedded in new Error(JSON.stringify(action)). The naive version caught only the first.

deliver-via-origin.ts reaches 100% coverage — the new catch body was its only uncovered branch. Both unknown-message-errors.ts modules added in the review round are at 100%. handlers/ sits at 96.9% statements / 90.3% branches against its 95/85 floor. Full ci-check green: 88 suites, 817 tests.

Review round

Two comments, both about what holds this change in place rather than about the change itself. Addressed in 3988fc7.

The five redacted throw sites had nothing testing them. Nothing in src/ imports background/index.ts, src/content/ has no index.test.ts (sdk-channel.test.ts's require('./index') only inspects the listeners init() registers), and neither file is inside collectCoverageFrom — so any of the five could go back to JSON.stringify(action) with a green ci-check. The constructions move into two modules, one per side, each taking the whole envelope and extracting type itself, so the redaction has a single tested seam instead of a convention every call site has to keep:

• src/background/handlers/unknown-message-errors.ts — in coverage scope already, via handlers/**/*.ts
• src/content/unknown-message-errors.ts — added to collectCoverageFrom as one explicit entry rather than widening to src/content/**, which would fail the global 100 gate; this module is pure and meets it

Same reasoning as windows.onRemoved → window-removed.ts: the body lives in a handler so it can be tested.

The §2 guard fed itself a benign fixture. It rejected with new Error('no listener / tab gone'), but webextension-polyfill@0.12.0 serialises a rejecting listener's Error.message into __mozWebExtensionPolyfillReject__ and rebuilds it as new Error(reply.message) on the sender side (browser-polyfill.js:1105-1148), and handleSdkMessage is async — so its throw takes exactly that path, and the §4 redaction is the only thing keeping payload text out of the §2 log. Nothing connected the two files. The fixture is now built by the real content-script constructor instead of a literal, which couples them:

function deliveryRejection(): Error {
  return unknownSdkMessageError(makeMessage().action);
}

Two more gaps in the same assertions. The warn branch — the one that fires whenever another same-origin tab is open, i.e. the common outcome — had no no-payload check at all; and type was matched as expect.any(String) at all six new sites, pinning nothing to action.type.

Each fix verified by mutation: reverting the content-script redaction fails 4 tests (3 of them in the background suite), leaking the action into the warn log fails 1, a literal type fails 6, reverting either background message fails 2.

Deliberately not included

• sdk-response-to-tab.ts:22-30 (recoverDappOrigin) keeps its unbound catch. It guards new URL() on an extension page URL that isTrustedUiSender has already validated, and its null return is not swallowed — it leads to the "no same-origin fallback available" sagaError, which is surfaced. The failure has a visible consequence, so binding it would add noise, not information.
• open-onboarding-flow.ts:71 is a storage.local read on the onboarding path, not the delivery layer.
• The three expect.any(String) in the duplicate-classifier tests (:245, :266, :316) keep their loose matcher — same weakness, but they predate this PR and are not part of its diff.
• WALLET-1386 touches sdk-response-to-tab.ts as well but changes delivery behaviour (cross-checking tabId against the request descriptor) rather than logging. It lands separately, after this.

[Comp0te]

Read against WALLET-1393's acceptance criterion — no failure at the background delivery layer invisible in the log, no error path stringifying a whole action. The binding and the redactions themselves look right; both comments below are about what holds them in place. The new assertions pin the shape of the identifier object but not the two properties this change exists to create — that no payload text reaches these lines, and that method identifies which call was lost — and the five redacted throw sites live in the two files the coverage config doesn't reach.

[Comp0te]

Three of the assertions added around these two log branches can't fail for the property they're written to protect. Same shape, one pass to fix.

1 — this line's third argument is the one piece of text the background doesn't control, and the test that checks it feeds itself a benign fixture. sdk-response-to-tab.test.ts:528-544 asserts loggedText(outerConsoleError) has no deadbeef in it, with the rejection hardcoded to new Error('no listener / tab gone') at :530. Under the pinned webextension-polyfill@0.12.0 a content-script listener's thrown message is relayed verbatim into this promise, so the redaction you added at content/index.ts:78 is the only thing keeping signatureHex out of this line — and nothing pins the two files together. Swap :530 for a content-script-shaped rejection and the assertion already there does the job: new Error('Content: handleOnMessage unknown sdk message: ' + JSON.stringify(makeMessage().action)) takes the file to 1 failed / 18 passed, with signatureHex":"deadbeef" in this console.error. As it stands, reverting content/index.ts:78 to JSON.stringify(message) breaks nothing here.

2 — the warn branch at :212-216 has no redaction check at all. loggedText(outerConsoleWarn) appears nowhere in the file; the three not.toContain('deadbeef') checks at :537, :566 and :591 are all against outerConsoleError. The warn branch's only assertion (:426-435) matches type: expect.any(String). Change it to log { ...identifiers, type: JSON.stringify(action) } and the suite stays green at 19/19 — and this is the branch that fires whenever tabs.sendMessage rejects while another same-origin tab is open, i.e. the common fallback outcome rather than the rare one. expect(loggedText(outerConsoleWarn)).not.toContain('deadbeef') in the test at :411 closes it.

3 — type is matched as expect.any(String) at every site, so nothing pins it to action.type. :431, :479, :513, :541, :563, :588. Replace action?.type with the literal 'sdk' here at :209 and at deliver-via-origin.ts:24 and the file is still green at 19/19. method is what separates a lost signResponse from a benign connectResponse(false) once the line is in a support log — the same distinction the severity split at :211-223 exists to make. One concrete assertion per branch, e.g. type: sdkMethod.signResponse.type, is enough.

[ost-ptk]

All three confirmed and fixed in 3988fc7. Each one reproduces exactly as described — I ran the mutations you named before touching anything, and again afterwards.

1 — the benign fixture. Your reproduction is right, and the mechanism is worth pinning down: webextension-polyfill@0.12.0 serialises a rejecting listener's Error.message into __mozWebExtensionPolyfillReject__ and rebuilds it as new Error(reply.message) on the sender side (browser-polyfill.js:1105-1148), and handleSdkMessage is async, so its throw takes exactly that path. The content-script redaction really is the only thing keeping payload text out of this line.

I didn't take the literal you suggested, though — a hardcoded 'Content: handleOnMessage unknown sdk message: ' + JSON.stringify(...) string proves the assertion can fail, but it still wouldn't couple the files: reverting content/index.ts wouldn't change a literal in this test. So the fixture is now built by the real constructor:

function deliveryRejection(): Error {
  return unknownSdkMessageError(makeMessage().action);
}

That only became possible because of your second comment — see below. Reverting the content-script redaction now fails 4 tests, 3 of them in this background suite.

2 — the warn branch. Correct, loggedText(outerConsoleWarn) appeared nowhere. Added at the site you pointed to, with a note that this is the branch that fires whenever another same-origin tab is open. Verified: { ...identifiers, type: JSON.stringify(action) } on the warn branch alone went from 19/19 green to 1 failed.

3 — expect.any(String). All six sites (:431, :479, :513, :541, :563, :588) now assert type: sdkMethod.signResponse.type. Verified: replacing action?.type with a literal in both sdk-response-to-tab.ts and deliver-via-origin.ts fails 6.

Not changed: the three expect.any(String) in the duplicate-classifier tests (:245, :266, :316). Same weakness, but they predate this PR and you didn't list them — happy to pin them here if you'd rather not leave three vacuous assertions next to six fixed ones.

[Comp0te]

This throw site and the four others redacted here — :298, :310-312, and content/index.ts:78, :119 — are exercised by no test, so the convention this PR establishes has nothing holding it in place.

Nothing in src/ imports src/background/index.ts. src/content/ has no index.test.ts, though its siblings sdk.ts, sdk-channel.ts and sdk-method.ts all have one — sdk-channel.test.ts:114 does require('./index'), but only inspects the listeners init() registers, so neither content-script throw executes. And jest.config.js:42-51 scopes collectCoverageFrom to redux/**/reducer.ts, handlers/**/*.ts and redux/sagas/**/*.ts, so the coverage gate reports nothing about either file; e2e-tests/fixtures.ts collects console lines only when they start with [CSP] or [SvgIcon]. Grepping the tree for all five message strings returns just the five source lines. Any one of them can go back to JSON.stringify(action) with a fully green ci-check.

The two content-script sites are the ones I'd least like to see regress, for the reason your own comment at content/index.ts:72-76 gives: that console is the dapp page's console.

The answer is already named in this file at :183-184 — "The body lives in a handler so it can be tested — this entry point is imported by no test." Moving these throw constructions into a small exported helper under src/background/handlers/ puts them inside collectCoverageFrom (jest.config.js:44, with its own threshold group at :66-71) and lets a unit test assert the message carries action.type and not the serialized action.

[ost-ptk]

Confirmed on every point — nothing in src/ imports background/index.ts, sdk-channel.test.ts:114's require('./index') never reaches either content-script throw, and grepping the five message strings returned only the five source lines. Fixed in 3988fc7, taking the route you named.

Two modules, one per side, both constructing from the whole envelope rather than a pre-extracted type — that way the redaction has a single tested seam instead of a convention each call site has to keep:

• src/background/handlers/unknown-message-errors.ts — unknownSdkMessageError / unknownReduxActionError / unknownMessageError
• src/content/unknown-message-errors.ts — unknownSdkMessageError / unknownSdkEventError

The background one lands in collectCoverageFrom via handlers/**/*.ts as you described. For the content one I added a single explicit entry to collectCoverageFrom rather than widening to src/content/** — the rest of that directory would fail the global 100 gate, but this module is pure and sits at 100/100/100/100.

Both sides now have a unit test asserting the message carries the type (and requestId for the SDK one) and not the payload. Verified by mutation: reverting either background message to JSON.stringify(action) fails 2; reverting the content one fails 4, because the delivery-failure test in the other thread now uses unknownSdkMessageError as its rejection fixture. Your two comments turned out to be the same fix.

ci-check green: 88 suites, 817 tests.