fix: WALLET-1364 — tie an approval request to the windows displaying it - #1427
Conversation
…ership Collapse the parallel requests/pendingRequests maps into one discriminated union and give every open request the set of window ids displaying it. A request can be shown by more than one window at a time (the Ledger permission window carries the same requestId), so ownership has to be a set, not a single id. windowRequestOpened no longer overwrites an existing entry: requestId is page-generated and therefore dapp-controlled, so a repeat must neither resurrect a responded tombstone nor clobber a live descriptor. PopupState.windowManagement becomes Pick<..., 'windowId'> so a future slice field is excluded from the replica broadcast by default instead of included.
…yed it cancelRequestsDisplacedBy becomes the single cancellation trigger: a window stopped displaying requests. Only requests whose windowIds was exactly that one window are cancelled, so a request the Ledger permission window still shows survives, and a request that never had the window can never cancel itself. failRequestOnWindowError covers the case where no window will ever exist: it marks the request responded and delivers the method-correct cancel, instead of leaving the dapp promise pending until its own 30-minute timeout. Error handling in the delivery loop: bind and log the error with identifiers only (never the SDK action, which carries signatureHex/encryptedMessage), use allSettled so one failed delivery cannot abort the batch, and raise the sagaError banner only when the same-origin fallback delivered nothing.
cancelOpenRequestsForClosedWindow becomes a thin wrapper over the shared routine, and the windows.onRemoved listener now runs for EVERY removed window instead of only the tracked one — the handler decides, by window ownership, whether anything should be cancelled. Closing the main approval window while the Ledger permission window is still up therefore leaves the request alive. The listener is also rejection-safe: a failure in the cancel path can no longer skip the exportKeysWindowIdCleared dispatch that follows it.
createOpenWindow now resolves { window, reused } so the caller can tell a reused
window from a fresh one — that distinction is what drives cancellation.
Two latent bugs in the same function: tabs.update omitted the tab id and so
targeted "the active tab of the current window", which lands correctly only
because the focus call is awaited first; and setWindowId ran even for
isNewWindow, letting the import-account flows retarget the shared approval slot
so their close could cancel an unrelated dapp approval.
Move the trigger out of the six SDK method branches and into openWindow, the only place that knows whether a window was reused. The supersede runs before the incoming request is attached and snapshots its candidates synchronously, so the incoming request — which has no window yet — can never be among them. That makes the old "must call cancel before dispatching the open" invariant structural instead of a matter of statement order. A windows.create rejection now cancels the incoming request and tells the dapp rather than leaving its promise pending. A resolved window with no id, and a window that closes during the reuse round-trips, are both recovered the same way instead of stranding the request with no window and no reaper. open-window.supersede.test.ts pins the ordering against the real reducer and the real cancel module: swapping the two blocks makes it fail.
The Ledger permission window displays a second copy of the approval page carrying the same requestId, and device confirmation takes far longer than any grace period. Registering that window against the request is what stops the main approval window's reuse or close from discarding an approval the user is in the middle of confirming — after which the genuine signature was silently dropped by the response dedup. windowRequestWindowAttached is dispatched from the UI, so it has to be in the background forwarding allow-list; without it dispatchToMainStore throws "Unknown redux action" at runtime with nothing to catch it at build time. The parity guard now asserts that membership directly.
The dedup drop was completely silent. A dropped cancellation is harmless; a dropped signature means the user approved and the response vanished with no trace anywhere in the system, which is what made this bug class undebuggable. Log identifiers only — requestId, tabId and the action type. The action payload carries signatureHex/encryptedMessage and must never reach the console. Adds the missing 'open'-status case to the dedup tests: no test had ever used the status every live request has when the user approves, so widening the condition to a null check would have passed the whole suite while dropping real signatures.
Attaching a window is the only way a request gains a display, which makes it the only way a request can be made permanently uncancellable: cancellation fires only for a request whose windowIds was exactly the window that went away, so an id nothing will ever raise windows.onRemoved for keeps the set oversized forever. The request then survives every close and every reuse and hangs its dapp's promise until the dapp's own timeout. Two callers attach, and only one was guarded. openWindow ran a liveness probe inline; the Ledger hook dispatches through dispatchToMainStore — a runtime.sendMessage round trip, by far the wider race of the two — and was forwarded blindly. The permission window closing during that hop left the request bound to a dead id. Both now go through attachWindowToRequest, which dispatches and then confirms the window exists, repairing the miss with exactly what onRemoved would have run. windowRequestWindowAttached therefore leaves the blind forwarding set and gets a dedicated branch in handleReduxAction, mirroring resetVault: a UI page crosses a message boundary to reach it, so the payload is validated rather than trusted. That also puts the guarantee in background code, where this repo's conventions allow it to be tested — the hook itself cannot be. The same check covers an invented windowId from a compromised extension page, which could otherwise pin windowIds above one and disable the request's cancel path for good.
Comp0te
left a comment
Comp0te
left a comment
There was a problem hiding this comment.
Reviewed the window-lifecycle rework: the windowIds model, the onRemoved and supersede cancellation paths, attachWindowToRequest, and the new discriminated union in the windowManagement slice.
The through-line of the comments below is that the new model makes "a window stopped displaying this request" the thing that decides a request's fate, while several paths still decide it from status alone, or register a window through a channel that can fail without saying so. A second, smaller cluster is about error surfaces the rework narrowed — a few places now log less, or stop short of telling the user, than the code they replace.
One note that has no line to sit on: the description opens with P0 + P1 items 1–8, but items 3 (export-keys window failure feedback) and 4 (CSP_NONCE build reproducibility) are not in this change. src/background/redux/sagas/export-keys-window-saga.ts still has the bare console.error with its "there is no popup left to show it in" comment, and webpack.config.js:71 is still crypto.randomBytes(16) with the unconditional DefinePlugin entry at :212 — gh pr diff --name-only lists neither file. Worth narrowing the body to the items actually implemented.
| void (async () => { | ||
| const store = await getExistingMainStoreSingletonOrInit(); | ||
|
|
||
| try { |
There was a problem hiding this comment.
Separate point from the .catch comment above: nothing tests any of this listener.
Grepping onRemoved across every test file at this head returns four hits — redux-actions.parity.test.ts:146,150, attach-window-to-request.test.ts:46, open-window.test.ts:219 — and all four are inside prose comments. No test file imports the background entry point, so the rewritten listener's shape, this try/catch, and the removal of the tracked-id guard are all unverified. cancel-open-requests-on-close.test.ts drives the handler directly and is indifferent to every one of them.
Concretely: reverting this listener to the previous async form — undoing both the guard removal and the try/catch — leaves the suite green. The two things that re-breaks are each the subject of an explicit ticket item.
Suggest extracting the body into an exported handleWindowRemoved(store, removedWindowId) and unit-testing it in the style of cancel-open-requests-on-close.test.ts.
| // second window carrying the same requestId (`src/hooks/use-ledger.ts`), so a | ||
| // request can outlive the loss of any single window. Cancellation is driven by | ||
| // "this window displays it no longer", never by a timer. | ||
| export type Request = |
There was a problem hiding this comment.
The 'open' variant permits windowIds: [], which is simultaneously the legitimate transient state (registered, window not yet resolved) and the terminal stranded one (nothing will ever display this).
The consequence is that the stranded case has no exit. cancelRequestsDisplacedBy filters on windowIds.length === 1 (cancel-requests.ts:137-140), so an empty set can never be cancelled by any window event; the only escape is failRequestOnWindowError, and grepping it at this head shows it is reachable only from openWindow's two failure branches (open-window.ts:57 and :97, via failIncomingRequest). There is no reaper — once a request reaches {status: 'open', windowIds: []} outside openWindow's control, it stays there for the life of the background context.
This union was introduced precisely to make illegal states unrepresentable, and this is the one illegal-ish state it still represents. It is also where two other paths flagged in this review land.
Suggest encoding the displayed case as non-empty — windowIds: readonly [number, ...number[]] on a 'displayed' member, with a separate {status: 'awaiting-window'} for the gap between windowRequestOpened and the first attach — so the stranded case becomes visible to selectOpenRequests consumers instead of indistinguishable from a healthy one.
| // opened it. Registering it is what keeps the request alive when the | ||
| // shared approval window is reused or closed while the user is still | ||
| // confirming on the device. | ||
| const requestId = askPermissionUrlData.params?.requestId; |
There was a problem hiding this comment.
Distinct from the error-handling point on the line below: this requestId read — the only thing in the codebase that ever puts a second entry in windowIds, i.e. the UI half of the P0 fix — has no test at all.
At this head, grepping use-ledger/useLedger across src returns 13 files and none of them is a test that imports or exercises the hook; grepping windowRequestWindowAttached returns 11, of which the only non-background hit is this file. Deleting lines 190-195 leaves the suite green.
There is a cheaper way to break it than deletion, which is what makes the gap worth closing: rename the requestId key at sign-transaction/index.tsx:294-295 or sign-message/index.tsx:189-190. askPermissionUrlData.params is Record<string, string> (:39), so the lookup here silently yields undefined, tsc does not catch it, and the permission window is never registered. The background half is well covered (cancel-requests-displaced-by.test.ts:63-80, reducer.test.ts:587); nothing proves the Ledger flow ever produces the second entry.
The repo has no React-hook harness, so suggest either extracting the requestId → dispatch decision into a testable unit, or asserting at the page level that params.requestId is threaded. Typing params as { requestId?: string; … } would additionally make the rename case a compile error.
`cancelRequests` re-filtered its snapshot against status alone, so a request that GAINED a window during the 250 ms grace was still marked responded and still had a cancel delivered to its dapp. That is the P0 this model exists to close, reached from inside it: request R is displayed by shared approval window W; the user triggers Ledger, so `openNewSeparateWindow` resolves and the attach begins its `runtime.sendMessage` round trip; dapp B reuses W inside that gap, so `cancelRequestsDisplacedBy` snapshots R as a candidate and detaches W; the Ledger attach then lands and R is genuinely displayed again, with the user confirming on the device. 250 ms later R was cancelled and the device signature dropped by the dedup. Re-check against the CURRENT descriptor instead: still open, and displayed by nothing but the window that went away.
…unexplained probe rejection Two ways the attach path could cancel a live approval it had no business touching. `windowRequestWindowAttached.match(action)` is RTK's `isAction(action) && action.type === type` — it does not validate the payload. Reading `action.payload.requestId` before `attachWindowToRequest`'s own shape guard runs turned a payload-less message into a TypeError out of `handleReduxAction`, which the router reports as a generic sendError. Read it defensively so the existing guard produces its intended "ignoring malformed attach" instead. The liveness probe's `.catch` did not bind its error and treated ANY rejection as "the window is gone", running the full repair. A transient extension-context error or a Safari window-type quirk would then cancel an approval still on screen and tell the dapp it was cancelled. Narrowing on the error's text would just move the problem — the wording differs per browser and such a guard stops matching silently — so confirm against `windows.getAll()` instead, and fail closed (no repair) when even that is unavailable: an uncancellable request hangs until its own timeout, which is recoverable, while a wrongly cancelled one destroys a signature the user already approved.
The two-arm `.then(onFulfilled, onRejected)` form is deliberate — the recovery must not catch itself — but it left the success arm covered by nothing, and there is no `unhandledrejection` handler anywhere in `src/`. `attachWindowToRequest` is what is genuinely unprotected there. If it throws, the window opened but was never attached: `windowIds` stays empty, and per the candidate filter a request with an empty set can never be cancelled by any window event. The dapp then hangs for its full 30-minute timeout with nothing logged.
`windows.onRemoved` now fires for ANY window, not only the tracked approval one, and this dispatch was unconditional. The candidate filter ran four lines above, but the dispatch was not gated on its result — it fired even when no open request held the window and the reducer returned the identical state object. Redux invokes subscribers per dispatch regardless of what the reducer returned, and the store subscriber does no state-change comparison: every one of these was a `popupStateUpdated` broadcast to every replica plus a full storage.local re-write of vault cipher, keys, settings, contacts, app events, trusted wasm and CSPR-name expirations. Before this listener changed, an unrelated window close produced zero dispatches.
…e truth when nothing was delivered
Two places where the rework stopped short of telling the user.
`cancelRequests` suppressed the banner whenever `deliverViaOrigin` reported a
delivery, and the comment justifying it reasons entirely from the supersede path
("this fires while the user is already looking at the NEXT approval screen").
But the routine is shared: the close path reaches it too, and there the user is
not looking at a replacement screen. `deliverViaOrigin` is also a weaker signal
than its name suggests — it counts same-origin sends to active tabs that did not
throw, which is not proof the tab owning the pending dapp promise received
anything. Gate the suppression on the source instead.
`failRequestOnWindowError` discarded `deliverViaOrigin`'s return value and
dispatched its banner BEFORE attempting delivery. When both routes fail the
request is tombstoned, the dapp received nothing and will hang, and the user was
told it was cancelled — with `sdk-response-to-tab` dropping anything that
arrives later, so it is unrecoverable. Dispatch after the attempt and say which
of the two happened.
Registering the Ledger permission window is the UI half of the window-ownership
fix — skip it and `windowIds` stays `[approvalWindow]`, so the next dapp request
reusing that window cancels this one while the user is confirming on the device.
It could be skipped three ways, none of which left anything in any log:
- `if (requestId)` does nothing when the param is absent. That is correct for
the import-account flow, which legitimately passes `params: {}` — but `params`
was `Record<string, string>`, so a signing page arriving without a `requestId`
was indistinguishable from it at runtime.
- the effect body was a bare async IIFE with no `try/catch`; `openNewSeparateWindow`
is an awaited call that can reject, and a rejection skipped everything below it.
- `dispatchToMainStore` discarded the error and logged only the action type, and
a send to a sleeping or restarting MV3 service worker is a routine failure.
Spell the params out as a named type so renaming `requestId` at a call site is a
compile error, extract the decision into `registerLedgerPermissionWindow` so it
has a seam to test (the repo has no React-hook harness, and deleting those three
lines left the suite green), branch explicitly on the flow, and give
`dispatchToMainStore` an error-bearing log. The effect's catch logs the error's
NAME only: `url` embeds the plaintext `signMessage` message.
Nothing tested the `windows.onRemoved` listener. Grepping `onRemoved` across every test file returned four hits, all inside prose comments; no test imports the background entry point, so the rewritten listener's shape, its error handling and the removal of the old tracked-id guard were all unverified. Reverting the listener to its previous form left the suite green. Its error handling was also narrower than it looked. The `try/catch` enclosed only `cancelOpenRequestsForClosedWindow`; both the store init above it and the export-keys branch below were outside, discarded by `void`, with no `unhandledrejection` handler anywhere in `src/`. What was never logged is that the export-keys cleanup got SKIPPED when the cancel threw — the tracked window id stays set, and the next export-keys open focuses a dead id. Move the body into `handleWindowRemoved`, give the two concerns independent try/catches, guarantee it never rejects, and cover it. A mutation probe confirms the two error-handling tests fail without the fix.
`handleReduxAction` had no sender gate, and the new `windowRequestWindowAttached` branch put a lifecycle-authority decision on it. The asymmetry is deliberate elsewhere in the router: `handleSdkResponseToTab` and `handleLegacyImport` are both pulled OUT of the generic loop specifically to gate on `sender`, each with a comment naming a content-script-world compromise as the reason — and `@bringweb3/chrome-extension-kit` runs in exactly that world. Both directions are reachable from there. A live-but-unrelated `windowId` gives a set that never shrinks to empty, i.e. a request nothing can cancel; a dead one attached in the gap before the real window makes `windowIds` exactly `[dead]`, which the cancel path then selects. Not dapp-reachable today — the page-world relay pins SDK_REQUEST_TYPES — but that allowlist is precisely the defence-in-depth the sibling handlers' comments say they do not rely on. Thread `sender` through and gate the attach branch. Gating the whole forwarding set is a behaviour change for ~60 actions and stays a separate ticket.
The liveness probe asked only "does a window with this id exist" — no
`{ populate: true }`, no check that the window belongs to this extension, no
check that it displays this request. Any live browser window passed. With
`windowIds: [W, F]` where `F` is an unrelated window, closing the real approval
window `W` is filtered out (the set has two entries), leaving `[F]` — so the
request's fate now hangs on a window the user may never close.
Populate the window and require its first tab's URL to start with
`runtime.getURL('')`. The checks are deliberately asymmetric: a URL that is
provably not ours is a verdict and undoes the attach, while anything
inconclusive is not. On the reuse path `tabs.update` resolves when the
navigation STARTS, so a legitimate window can be probed before its URL settles;
repairing on that would cancel a live approval, which is the failure this whole
model exists to prevent. Same reason a requestId mismatch only warns.
Three places where the slice's types said less than the code relied on. `windowIds: number[]` was mutable and `selectOpenRequests` hands entries out with a shallow spread, so the store's own array left by reference — and `configureStore` runs with `immutableCheck: false`, so nothing caught an in-place mutation at runtime either. Since `cancelRequestsDisplacedBy` decides a request's fate purely from that array, a `push` on a selector result would silently rewrite the decision with no action dispatched. `readonly` throughout. `selectOpenRequests` opted out of checked narrowing with a type predicate. TypeScript verifies only that the asserted type is assignable to the parameter type, not that the predicate body agrees — writing `=== 'responded'` compiled identically and everything downstream stayed typed as open descriptors. Narrow on the discriminant instead; a probe confirms the flipped comparison is now a compile error. `requests: Record<string, Request>` with `strict` but no `noUncheckedIndexedAccess` types every lookup as `Request`, never `Request | undefined` — which made the reducer's existence guards dead code as far as the compiler is concerned, including the P2 fix "a repeated dapp-controlled requestId must not overwrite a live request nor resurrect a tombstone". `Partial<Record<…>>` models the lookup honestly; a probe confirms removing the guard is now a compile error.
`CancelSource` constrained a parameter with only two reachable values and left unconstrained the field where sources actually land. `'open-window-failed'` was in the union but no call site produced it AS a `CancelSource` — `cancelRequests` is module-private and its only caller passes one of the other two; the literal appears only inside `failRequestOnWindowError`'s `sagaError`. Meanwhile `SagaError.source` was still `string`, which is what WALLET-1364 #18 asked to have made a union, and another live source, `'sdk-response-to-tab'`, was not in the union at all. Move a `SagaErrorSource` covering all fourteen producers into `app-events/types.ts` and type `SagaError.source` with it; `CancelSource` becomes an `Extract` of the two window-driven ones. The union immediately caught three invented sources in the app-events reducer tests.
The comment claimed the map "stays proportional to in-flight requests, not to the lifetime total" and that "an MV3 service-worker restart wipes it". Neither holds on all three build targets: `windowRequestResponded` writes a tombstone and nothing anywhere deleted a key, and both `manifest.v2.json` and `manifest.v2.safari.json` declare `"persistent": true`, so the Firefox and Safari builds have a background page that is never torn down. On those two the map grew by one permanent entry per request, keyed by a dapp-supplied string, for the whole browser session — and that comment was also the sole justification given for keeping the tombstone at all. State the real bound and add FIFO eviction above a cap that a dedup could plausibly need. Open requests are never evicted.
`requestId` is page-generated, i.e. dapp-controlled. The slice reducer refuses to overwrite a live request or resurrect a tombstone — but that no-op was invisible to the caller, and all six method branches called `openWindow` regardless. Two shapes followed. A dapp replaying a finished id hits the tombstone, the window still opens fully, `deployPayloadReceived` still populates `jsonById` so the screen renders normally — and the user's approval is then silently dropped by the dedup. A dapp reusing a LIVE id under another method keeps the first descriptor, so `buildCancelResponse` later builds the wrong response type. Refuse both before anything is dispatched, and do it once for the whole set of approval methods rather than in each branch, so a seventh flow cannot forget it. `OpenApprovalWindowProps.requestId` becomes required in the same spirit: there is no non-approval caller, and every branch that saves a request from being stranded was gated on the field being present.
The `!isNewWindow` clause added to `createOpenWindow` made three of `useWindowManager`'s inputs dead. Both consumers of its `openWindow` pass `isNewWindow: true`, which forces `id = null` — so the reuse block and `clearWindowId` are unreachable, `setWindowId` is unreachable through the new guard, and the `useSelector(selectWindowId)` feeds nothing. `knip` cannot flag any of it: these are object properties, not exports. The live edge is downstream. `windowIdChanged` and `windowIdCleared` remained in `FORWARDED_ACTION_TYPES` with no legitimate UI dispatcher left — the background dispatches its own — so any extension UI page could `runtime.sendMessage` a `windowIdChanged(<arbitrary id>)` and retarget the shared approval-window slot the request lifecycle depends on. The parity test reasons carefully about exactly this hazard for `windowRequestWindowAttached`; these two now meet the same background-only criterion, so move them to EXCLUSIONS with the reason recorded.
This rejection arm is the sole cause-bearing diagnostic on the "no approval window could be opened" path — the hard-to-reproduce failure the rest of this work adds `failRequestOnWindowError` to handle. The reason for withholding the raw error is right: a `signMessage` window URL embeds the user's plaintext message as a query param, and a rejection's text can echo the URL it failed on. But the mitigation dropped the whole diagnostic rather than redacting it: `.name` on a `windows.create` / `windows.update` / `tabs.update` rejection is the string "Error" in the realistic cases and `undefined` for a non-`Error` rejection, so the message — the only field that says WHY — was gone. Cut everything from the first `?` (where any secret would be), cap the length, and add `windowApp`. The secret stays out and the cause stays in.
Unrelated to the request-lifecycle work in this PR — no commit here touches package.json or the lockfile. The branch was green on this same lockfile on 2026-07-29; `npm audit` queries the live registry, so two advisories published since then turned it red: - GHSA-rgw5-rvv9-x895 — brace-expansion, DoS via unbounded intermediate arrays, which bypasses the CVE-2026-14257 mitigation. Fixed in 1.1.18. - GHSA-7p8r-x3mc-p8w7 — fast-uri, host confusion via a backslash authority introducer. Fixed in 3.1.5. Both packages were already pinned through `overrides`, and both now have a patched release reachable from the existing range, so this is a version bump rather than a new allowlist entry. That also retires the ACCEPTED entry for GHSA-mh99-v99m-4gvg: it reasoned that "no patched 1.x/2.x/3.x exists", which the 1.1.18 maintenance release makes false, and the gate itself now reports the entry as stale. Removing it keeps the allowlist to advisories that are genuinely unfixable here.
|
Thanks for the depth here — this is one review rather than 26, so one reply. I checked every comment against the branch before touching anything. None was a false positive, including the claims that needed independent verification: 24 of the 26 are fixed here (15 commits, The one that mattered
Resolution map
Where I did not take the suggestion as written
Two implementation notes worth flagging back
Verification
One bug was caught by its own test while implementing: the tombstone eviction used The red CI on this PR was unrelated — two advisories published after the branch last ran green ( |
Comp0te
left a comment
Comp0te
left a comment
There was a problem hiding this comment.
Second pass over the push that answers the first round — the incremental diff 8e0e1747..d923973c plus the state it leaves behind. Most of the earlier comments are closed and I've resolved those threads.
What's left is two ways a request can reach the lifecycle model with nothing registered in it, two new failure paths that log but leave the user or the request stranded, and one banner arm that no test pins.
| // anything is dispatched. | ||
| if ( | ||
| APPROVAL_REQUEST_TYPES.has(action.type) && | ||
| selectRequestStatus(store.getState(), action.meta.requestId) != null |
There was a problem hiding this comment.
This guard and the reducer's "register once" guard read the same dapp-controlled key off a plain object with different operators. selectRequestStatus is requests[requestId]?.status; the reducer at windowManagement/reducer.ts:51 is state.requests[requestId] != null.
For requestId ∈ __proto__, toString, constructor, valueOf, hasOwnProperty the two disagree. The first sees undefined and lets the request through; the second sees an inherited Object.prototype member, concludes the id is already registered, and returns state unchanged — so nothing is registered. handleSdkMethod then calls openWindow anyway, attachWindowToRequest's dispatch is likewise a no-op, and failRequestOnWindowError returns early because there is no descriptor.
The result is a live sign / signMessage / connect / decryptMessage approval window sitting outside the entire lifecycle model this PR builds — not cancellable on close, not cancellable on supersede, not deduped, not recoverable on window-open failure. That is the stranded state the change exists to eliminate, reachable by picking one of five string literals, with no race involved. isSDKMethod (src/content/sdk-method.ts:129) only checks typeof requestId === 'string', so nothing upstream rejects them.
The divergence on its own: node -e "const m = {}; console.log(m['__proto__']?.status, m['__proto__'] != null)" → undefined true.
Suggested: validate action.meta.requestId against a UUID shape at the handleSdkMethod entry, and build the map with Object.create(null) or read it via Object.prototype.hasOwnProperty.call.
| // when the navigation STARTS, so a legitimate window can be probed before its | ||
| // URL settles — repairing on that would cancel a live approval, the exact | ||
| // failure this model exists to prevent. | ||
| void windows.get(windowId, { populate: true }).then( |
There was a problem hiding this comment.
This is void windows.get(windowId, { populate: true }).then(onFulfilled, onRejected) — a two-arm .then with no trailing .catch. A throw inside the fulfilled arm becomes an unhandled rejection.
runtime.getURL('') at :74 is a live candidate: it is precisely the API that fails on an invalidated extension context, which the comment a few lines below cites as the reason the rejected arm must not trust itself.
This is the same shape you repaired one file over in this push — open-window.ts:132, void chain.catch(...). The inner windows.getAll() chain here has a .catch; the outer one does not. Nothing logs, nothing repairs, and the attach stands with a windowId whose liveness was never established — this file's own header calls that the permanently-uncancellable state. There is no unhandledrejection handler anywhere in src/, so in an MV3 service worker it leaves no trace at all.
| (delivered > 0 | ||
| ? '; delivered via same-origin fallback' | ||
| : '; not delivered') | ||
| delivered > 0 |
There was a problem hiding this comment.
The delivered === 0 arms of both ternaries are covered — fail-request-on-window-error.test.ts:80-100 asserts stringContaining('could not be told') and cancel-requests-displaced-by.test.ts:161-182 asserts stringContaining('not delivered'), both with deliverViaOrigin mocked to zero.
The other arm has no assertion anywhere. '…recovered via the page' here at :151 and '…the request was cancelled' at :234 appear nowhere outside the source; the only other hit is a test title at cancel-requests-displaced-by.test.ts:184, which pins suppression rather than text.
That gap matters because saga-error-banner.tsx:87 renders {error.source}: {error.message} verbatim, and the two arms say opposite things about whether the dapp was told. Collapsing either ternary to its pessimistic constant leaves the whole suite green and makes the banner tell the user the site may still be waiting when it was in fact informed — the direction these commits set out to fix.
Suggested: add a delivered > 0 case to each, asserting stringContaining('recovered via the page') and stringContaining('the request was cancelled').
| if (w.id) { | ||
| dispatchToMainStore(ledgerNewWindowIdChanged(w.id)); | ||
| triggeredRef.current = true; | ||
| if (w.id == null) { |
There was a problem hiding this comment.
Both new failure paths — this w.id == null early return and the effect's .catch at :237-246 — log and return without calling setLedgerEventStatusToRender or setting triggeredRef (set only at :228, on the success path). For the openNewSeparateWindow rejection the comment at :238-239 names, nothing in the dependency array at :247-253 has changed at that point, so the effect will not re-run and the status stays LedgerPermissionRequired.
To be fair to what's already there: that status is in the ledger-error map (errors.ts:22-26), so the page does render an error view with a working "Got it" CTA (ledger-footer.tsx:44-48). The problem is that the error is the wrong one. It tells the user to grant permission in a window that was never opened and never will be, with no signal that the open failed and no retry path — the only exit is the CTA that abandons the approval.
Suggested: set a distinct rendered error status in both branches so the screen reflects "the permission window could not be opened" rather than "waiting for permission".
| // the FIFO cap below: the descriptor is dropped as before, and the oldest | ||
| // tombstones are evicted once there are more than a dedup could plausibly | ||
| // need. Open requests are never evicted. | ||
| windowRequestResponded: ( |
There was a problem hiding this comment.
windowRequestOpened (:51-53) and windowRequestWindowAttached (:78-84) both refuse to act on a missing or wrong-status entry. windowRequestResponded here is an unguarded upsert, so it writes a tombstone for a requestId that has no descriptor at all — reachable from sdk-response-to-tab.ts:117,134 for any id the extension UI forwards.
The union models ∅ → open → responded; the reducer permits ∅ → responded. A concrete route: an MV3 service-worker restart wipes requests between registration and the UI's response, and the response then tombstones an id the store has never seen. That entry consumes a slot in the new 50-entry cap, and until it is evicted the Duplicate requestId throw at sdk-methods.ts:68-73 will reject that id — selectRequestStatus returns 'open' | 'responded' | undefined, so a tombstone trips it just as a live entry does.
Bounded, to be clear: the FIFO cap at :147-156 evicts it after 50 further responses, and generateRequestId is crypto.randomUUID, so this is not a collision path. The point is the asymmetry itself — two of the three cases make the illegal transition unrepresentable and the third does not.
Suggested: guard the upsert the way its two siblings are guarded — only tombstone a requestId that is currently 'open'.
`requestId` is dapp-controlled and `requests` is a plain object, so `requests[requestId]` can read an INHERITED `Object.prototype` member. Two readers then disagreed about the same key: `requests[id] != null` in the reducer saw the inherited member and concluded "already registered", while `selectRequestStatus`'s `requests[id]?.status` saw `undefined` and concluded "fresh". Picking one of five string literals — `__proto__`, `toString`, `constructor`, `valueOf`, `hasOwnProperty` — therefore produced an approval window for a request the store never registered: not cancellable on close, not on supersede, not deduped, and not recoverable by `failRequestOnWindowError`, which returns early with no descriptor to find. No race involved, and nothing upstream rejects them — `isSDKMethod` only checks `typeof requestId === 'string'`. Every read now goes through `getRequest`, which answers both questions the same way. Four of the five then behave like any other id. `__proto__` is different and cannot be stored at all: building the next map copies entries by assignment, and assigning `__proto__` sets the object's PROTOTYPE instead of adding an entry — the descriptor would be inherited by every later lookup. So it is refused, in the reducer and again at the SDK entry so the dapp gets a definite error instead of a window nothing can close.
`windowRequestOpened` and `windowRequestWindowAttached` both refuse to act on a missing or wrong-status entry. `windowRequestResponded` wrote unconditionally, so the union modelled ∅ → open → responded while the reducer permitted ∅ → responded. Reachable whenever the UI forwards a response for an id the store no longer holds — an MV3 service-worker restart between registration and the response. The orphan tombstone then consumes a slot in the 50-entry cap, and until it is evicted the SDK entry guard rejects that id as a duplicate, since `selectRequestStatus` cannot tell a tombstone from a live entry. Bounded either way, and not a collision path — `generateRequestId` is `crypto.randomUUID`. The point is the asymmetry: two of the three transitions made the illegal one unrepresentable and the third did not.
…ay when a window carries no requestId The probe is a two-arm `.then(onFulfilled, onRejected)` with no trailing `.catch`, so a throw in the fulfilled arm has nothing to catch it — the same shape repaired one file over in `open-window.ts`, where the inner chain had a `.catch` and the outer one did not. `runtime.getURL` is the live candidate: failing on an invalidated extension context is precisely what the rejected arm's own comment says it must not trust. Nothing would log, nothing would repair, and the attach would stand with a windowId whose liveness was never established — this file's own permanently-uncancellable state, with no `unhandledrejection` handler anywhere in `src/` to leave a trace. The asymmetry between "provably not ours" (repair) and "inconclusive" (leave it alone) is deliberate and stays. But a URL carrying no `requestId` param produced no signal at all, which left the check establishing "one of our windows" and silently saying nothing about "the window showing THIS request". Both diagnostics now warn; neither repairs, for the same reuse-round-trip reason as the mismatch branch.
Only the `delivered === 0` arms were asserted. `'…recovered via the page'` and `'…the request was cancelled'` appeared nowhere outside the source, so collapsing either ternary to its pessimistic constant left the whole suite green — while `saga-error-banner.tsx` renders the message verbatim and the two arms say opposite things about whether the dapp was told. Verified by doing exactly that: collapsing both fails these two tests and nothing else.
Both new failure paths — the `w.id == null` early return and the effect's `.catch` — logged and returned without touching the rendered status, so the screen kept showing `LedgerPermissionRequired`. That status IS in the error map, so an error view with a working CTA did render — but the wrong error: it tells the user to grant permission in a window that was never opened and never will be. Nothing in the effect's dependency array changes on that path, so it does not re-run, and the only exit was the CTA that abandons the approval. Add a distinct `PermissionWindowFailed` status and set it in both branches, so the screen says the window could not be opened and what to do about it. The Ledger error map holds plain strings rather than i18n keys, so this needs no catalog round trip.
|
Second round: all five actionable comments fixed, and an answer to the question on
|
| Comment | Fix |
|---|---|
attach-window-to-request.ts:60 — two-arm .then with no trailing .catch |
063737e4. Correct, and I had just repaired the same shape one file over — the inner getAll chain had a .catch and the outer one did not. runtime.getURL is indeed the live candidate. |
reducer.ts:136 — unguarded tombstone upsert |
4984ca04. Guarded on 'open' like its two siblings, so ∅ → responded is no longer representable. The eviction tests now drive the real ∅ → open → responded path rather than tombstoning ids that were never registered — which is also what let the old shape through unnoticed. |
cancel-requests.ts:150 — the delivered > 0 arms are unpinned |
bd016c0e. Both now assert the text. Verified by collapsing both ternaries to their pessimistic constants: exactly these two tests fail and nothing else. |
use-ledger.ts:210 — the wrong error is rendered |
15f8311f. New PermissionWindowFailed status set in both branches, so the screen says the window could not be opened rather than telling the user to grant permission in a window that will never exist. The Ledger error map holds plain strings rather than i18n keys, so no catalog round trip. |
attach-window-to-request.ts:50 — the gap is intentional, but it was too quiet
You are right that the check establishes "one of our windows", not "the window showing this request", and right that the mismatch branch is the only thing that would distinguish them.
That is deliberate: a requestId mismatch during the reuse round trip is expected, not anomalous, so treating it as a verdict would cancel live approvals. But "no requestId param at all" producing no signal was not deliberate — it left the distinction silently unobservable. Both diagnostics now warn (063737e4); neither repairs.
Every approval window is opened with requestId in its search params, so a window of ours without one is genuinely anomalous and worth a line.
Still not filed
The two follow-up tickets from the first round — the Request union refactor and gating the whole FORWARDED_ACTION_TYPES — still need Jira keys.
… saga into SagaErrorSource (#1437) * fix(app-events): admit the export-keys saga into SagaErrorSource #1427 narrowed SagaError.source from a bare string to a closed union; #1428 was green against a base that predated it and landed a producer the union does not list. Neither PR's CI could see the other — GitHub does not re-run a PR when its base moves — and git found no textual conflict, since the two touch different files. release/2.7.0 has been failing tsc since #1428 landed, which also blocks #1432 and #1433. 'openExportKeysWindowSaga' is a real producer: it is the ERROR_SOURCE constant that export-keys-window-saga.ts dispatches both of its sagaError payloads with. The invented 'sagaA'/'sagaB' test fixtures become real sources too — a test asserting the union's own behaviour has no business inventing members it forbids. * refactor(app-events): close the other half of the SagaErrorSource contract dismissSagaErrorsBySource still took PayloadAction<string> while SagaError.source is a closed union, so a producer could retract errors under a spelling no producer can ever write — exactly the drift the union was introduced to make impossible, left in place on the retraction side. The only production caller already passes the saga's ERROR_SOURCE constant, so this narrows nothing that exists. It does remove one test: "no-op for an unknown source" asserted a state that is no longer representable; the reachable case — a real producer with nothing on screen — replaces it.
2 participants
Ticket: https://make-software.atlassian.net/browse/WALLET-1364 (P0, and P1 items 1, 2, 5–8)
The bug
A request's approval UI was not tied to any window, and "the old request is gone" was inferred from a 250 ms wall-clock timer.
cancelRequestspicked victims bystatus === 'open'alone, andPendingRequestDescriptorwas{tabId, origin, method}— no window link at all.The Ledger permission flow opens a separate window carrying a fully functional signing page with the same
requestId, and device confirmation takes seconds to minutes. So when a second dapp sent any request:signResponse({ cancelled: true });signResponse({ signatureHex }), which hit the dedup guard, saw status'responded', and was dropped with no log at all;The same defect existed on the close path: closing the tracked approval window cancelled a request the Ledger window was still displaying.
The fix
A request stays open while at least one window still displays it. Cancellation is triggered by "window W stopped displaying requests" — never by a timer, never by "a new request arrived".
requestsandpendingRequestscollapse into one discriminated union, and each open request carrieswindowIds: number[].windows.onRemoved(now for any window, not only the tracked one) and the resolution ofopenWindowwithreused: true.openWindow, so the old "call cancel before dispatching the open" invariant is structural instead of a matter of statement order.Also fixed, all in the same paths:
windows.createrejection now cancels the incoming request and tells the dapp, instead of leaving its promise pending for the 30-minute timeout; likewise a resolved window with no id, and a window that closes during the reuse round-trips.signatureHex/encryptedMessage.tabs.updatetargets the resolved tab id instead of "the active tab of the current window".isNewWindow(the import-account flows) no longer retargets the shared approval slot.sagaErrorbanner is suppressed when the same-origin fallback actually delivered, so the user no longer reads an untranslated red error on top of the signing screen for a different transaction.PopupState.windowManagementbecomesPick<…, 'windowId'>— fail-closed, so a future slice field is excluded from the replica broadcast by default.Review round — 15 follow-up commits
All 26 review comments were verified against the branch; none was a false positive. 24 are fixed here, one was already closed by #1430's
Pick/broadcast-shape test, and two are taken partially on purpose (see Deliberately partial below).The one that mattered most:
cancelRequestsre-filtered its snapshot on status alone after the grace, so a request that regained a window inside those 250 ms was still cancelled. That is this PR's own P0, reached from inside the fix — the Ledger attach crosses aruntime.sendMessageround trip and routinely lands after the candidates were snapshotted. It now re-checks the current descriptor.The rest, by cluster:
requestIdindistinguishable from the import flow, notry/catcharound the effect,dispatchToMainStoreswallowing the cause) — all three closed, the decision extracted into a tested unit, andparamsgiven a named type so renamingrequestIdis a compile error; a throw inopenWindow's success arm was caught by nothing; theonRemovedlistener had no test at all and is now an exportedhandleWindowRemovedwith one.failRequestOnWindowErrortold the user "cancelled" without checking whether the dapp had been told;openWindowloggederror.name(always the string "Error") instead of a redacted message; a dropped duplicate no longer conflates "a cancel raced a cancel" with "a signature was destroyed".storage.localrewrite; the tombstone map was unbounded on the two MV2 targets whose background page never restarts; a replayed or reusedrequestIdopened a fully functional approval screen for a request that could never be answered.windowIdswas mutable and handed out by reference;selectOpenRequestsused an unchecked type predicate; the reducer's existence guards were dead code to the compiler;SagaError.sourcewas stillstring. Each is now enforced — probes confirm that reverting any of them is a compile error.sendergate;.match()does not validate a payload, so a payload-less message threw out of the handler; the liveness probe accepted any live browser window and treated any rejection as "gone";windowIdChanged/windowIdClearedwere still forwardable with no legitimate UI dispatcher.Deliberately partial
Requestunion.{status: 'open', windowIds: []}is both the legitimate transient state and the terminal stranded one. The suggested'awaiting-window'member with a non-empty tuple is a refactor through the reducer, selectors, both cancel paths, fixtures and every slice test. Here the entry paths are closed instead (requiredrequestId, the post-open.catch, the duplicate guard), so the stranded state is reachable only underopenWindow's control, whose failure branches both lead tofailRequestOnWindowError. The union itself wants its own ticket.windowRequestWindowAttachedbranch. Gating the wholeFORWARDED_ACTION_TYPESis a behaviour change for ~60 actions and needs its own verification pass.Verification
npm run ci-checkpasses: 66 suites / 473 tests,knipclean, coverage floors held.Mutation probes were run to prove the new tests are load-bearing rather than decorative:
openWindowmakesopen-window.supersede.test.tsfail;=== 'responded'to!= nullfails exactly the new'still open'case, and nothing else;handleWindowRemoved's two independent try/catches fails exactly its two error-handling tests;selectOpenRequests, or dropping the reducer's null guard, are now compile errors — neither was before.One bug was caught by its own test during this round: the tombstone eviction used
slice(0, length - CAP), and a negative end slices from the back, so it evicted almost everything until the map was full.Manual QA — cannot be automated
signfrom dapp A, reach device confirmation, then trigger any request from dapp B. Expected: the Ledger window stays functional and dapp A resolves with its signature. Note the repro requires the permission-prompt path — with an already-paired device the confirmation happens inside the main window.Known follow-ups — not filed yet, not caused by this PR
closeNewLedgerWindowsAndClearStatecloses all popup windows, including the main approval window, so after a supersede dapp B is force-cancelled when the Ledger flow finishes. Pre-existing and unchanged by this PR (B died before it too), but it wants its own ticket and its own QA.Requestunion refactor and full forwarding-set gating described under Deliberately partial.