WALLET-1416 — close the windows a Ledger flow owns - #1462
Conversation
… request The close-on-response path reads `state.ledger.windowId` and returns early when the descriptor is missing, so the fixtures need a ledger slice and an open descriptor before that production change lands. Seeding `makeStatefulStore` also removes a 5s timeout that would otherwise read as flake rather than as the dedupe contract being unproven.
…lands
The branch's close command is dispatched from a subscriber sitting behind
`debounceTime(300)` (ledger.ts:44-45), so on every successful sign it reaches
the background ~300ms after `windowRequestResponded` has already collapsed the
descriptor to `{ status: 'responded' }` and dropped `windowIds`. It therefore
took its degraded branch and left the dapp approval window on screen.
Move the trigger to the response itself. `markRequestResponded` snapshots
`windowIds` in the same synchronous block that marks the request responded —
the last instant the array exists — and derives the Ledger scope from state
that already exists (`ledger.windowId` being one of the request's windows), so
no new redux action, descriptor field or UI dispatch is needed. After delivery,
ownership is re-checked against `selectOpenRequests`, so a window a second
request reused during the dapp-controlled `tabs.sendMessage` is never removed.
The gate is deliberately per-request rather than per-response-kind: only a
`sign`/`signMessage` request can satisfy it, and every response those two pages
send is terminal — including `signError`, whose error screen would otherwise
strand an unclosable `type: 'normal'` window and a permanent `ledger.windowId`.
Re-checking ownership after delivery is correct only because our own request is
already `responded` by then; otherwise `selectOpenRequests` would claim these
very windows and the target list would come back empty.
The ledger slice is cleared before the removals, and only when the slot still
names the window being removed, so a flow that claimed the slot during delivery
keeps its state.
`closeLedgerWindowsAfterResponse` wraps its whole body in one `try` and is
called as a bare `void`: a `.catch` arrow at each call site would add two
uncoverable functions to a group already at 100% `functions`.
Behavioural delta: a response for a request the store no longer knows (MV3
restart) now delivers without dispatching, instead of dispatching a
`windowRequestResponded` the reducer would reject anyway.
… handler The handler's comment asserted that SignatureCompleted reaches the background before the SDK response because it is emitted synchronously before `signTransaction` returns. `.next()` being synchronous says nothing about delivery: the single subscribe helper pipes `debounceTime(300)` (ledger.ts:44-45), so every subscriber sees it ~300ms late and this handler runs, when it runs at all, after the response path already collapsed the descriptor. Whether it runs on a successful sign is a race — that path removes both windows, destroying the document whose timer would fire — so the branch is described by what it observes, not by an ordering this tree cannot settle. Trim the header JSDoc to the summary and the never-rejects contract; the ownership rationale and the history of the removed `windows.getAll` sweep belong here, not in the source. Also replace the prototype-pollution test case: with an empty requests map a bare `requests['hasOwnProperty']` yields a Function whose `.status` is undefined, so the unsafe read passed the test that exists to forbid it. The replacement builds the map with `Object.create` and asserts the log line that proves which branch was taken.
…ive signature
`onErrorCtaPressed` drives both the 'Got it' footer button and the header
back-arrow, and it tore down the whole flow. In the approval window that made
the approval window the last removal, so cancel-on-close answered the dapp
`{cancelled:true}` 250ms later — while the user was confirming the transaction
on the device. 'Got it' is the only footer control that window offers for the
entire permission phase, so this was not an off-path gesture.
Route both controls through a pure decision function. In the permission window
(`initialEventToRender` in the URL) they stay the flow's teardown, because that
window is `type: 'normal'` and cannot close itself. In the approval window with
a permission window live they dismiss only this window; `candidates` requires
`windowIds.length === 1` (cancel-requests.ts:177-179), so the request stays open
with the permission window attached and the signature still answers the dapp.
With no permission window they restore the details screen, exactly as today.
The decision lives in the UI because the background cannot make it: both
controls dispatch an identical command from one call site, no store field
records device progress, and `sender.tab.windowId` is not read anywhere in this
repo. Both pages already compute the discriminator.
…emoved `ledger.windowId` is a mutex: use-ledger refuses to open a permission window while it is set (use-ledger.ts:206-211), and the only clearer tied to a window closing lives inside the document that opened it. Whenever that document dies first the slot is stale for the rest of the session and no later Ledger flow can open a permission window at all. The previous commit makes one such sequence routine — dismissing the approval window takes its close tracker with it — so the two must ship together. Gate on strict id equality against live state, exactly like the export-keys block below it, which also makes it a free compare-and-clear: a slot another flow took over cannot match. Its own try, because a throw in the cancel path must not skip it. Placed before the awaited cancel, which sleeps CANCEL_GRACE_MS and then awaits a dapp-page tabs.sendMessage — a stale exportKeysWindowId self-heals, a stale ledger.windowId does not, so that delay is the lockout. Reuses `ledgerStateCleared` rather than adding a windowId-only action: a partial clear strands `recipientToSaveOnSuccess`, which `stakes` never overwrites and `sign-with-ledger-in-new-window` writes into recent recipients.
…ure-closes-every-popup-window-in-the-profile-cancelling-unrelated-dapp-approvals-and-the-secret-key-export-window
…ure-closes-every-popup-window-in-the-profile-cancelling-unrelated-dapp-approvals-and-the-secret-key-export-window
…ure-closes-every-popup-window-in-the-profile-cancelling-unrelated-dapp-approvals-and-the-secret-key-export-window
Comp0te
left a comment
Comp0te
left a comment
There was a problem hiding this comment.
Read as the Ledger window-ownership change: the new close-ledger-flow-windows handler, its redux-actions branch, the ledger-flow-controls decision function and the two page call sites, against the sibling close-windows-on-response written alongside it.
The through-line is state.ledger.windowId — one global scalar slot that both the UI decision and the background teardown treat as "this flow's permission window". Where a second Ledger flow holds the slot, the branch at ledger-flow-controls.ts:22 and the removal set at close-ledger-flow-windows.ts:20 both act on someone else's window. The sibling handler in this same PR already carries the ownership checks the new one is missing, which makes that asymmetry the quickest thing to act on. The rest: the dismiss path lost a recovery the old code guaranteed, the new message branch doesn't bind its requestId to the sender, and the decisions this PR makes are covered by no check in the repo.
| closeNewLedgerWindowsAndClearState(); | ||
| }; | ||
| const onErrorCtaPressed = () => | ||
| runLedgerFlowControl(isLedgerNewWindow, ledgerPermissionWindowId, { |
There was a problem hiding this comment.
Four decisions this PR makes that no check in the repository covers — grouped because they share one fix shape.
This wiring is the clearest instance: all three members of LedgerFlowControlEffects are () => void, so every permutation of this object literal type-checks. Swap the bodies bound to returnToMain and dismissThisWindow and the permission branch calls closeCurrentWindow(), which only removes a window when windows.getCurrent().type === 'popup' (close-current-window.ts:13) — while openNewSeparateWindow creates the permission window type: 'normal' (create-open-window.ts:189,194). The user's only exit from a failed Ledger flow becomes a silent no-op button, and tsc, the suite and CI all stay green. ledger-flow-controls.test.ts drives only jest.fn()s and never renders either page; the same wiring is at sign-transaction/index.tsx:329.
The other three:
use-ledger.ts:303— the single dispatch site ofcloseLedgerFlowWindows, whoserequestIdis what makes the handler's approval-window half run.closeLedgerFlowWindows({})type-checks (the payload is{ requestId?: string }) and leaves the suite green, includingredux-actions.test.ts:215andclose-ledger-flow-windows.test.ts:85— which pin both shapes as valid and would then simply describe the wrong flow. At runtime the dapp approval window stops being closed, half of what the ticket asks for, with nothing failing.use-ledger.ts:300-308—closeNewLedgerWindowsAndClearStatereturns early on a falsywindowId(:301), and nothing exercises it in that state, so "a permission window whose slot was released cannot end its flow" is unpinned. There is nouse-ledgertest file in the tree at all.close-ledger-flow-windows.test.ts:151— the suite pins which ids are removed,ledgerStateClearedand console output, but never that the abandoned request still gets answered; it mockswebextension-polyfilldown towindows.removeand never driveshandleWindowRemoved. A "tidy the descriptor up" change inside the handler that tombstones it would build no cancel candidate, and the dapp's promise would never settle.close-windows-on-response.test.ts:571-604has exactly this integration for the response path.
Underneath the first three: src/apps/** and src/hooks/** are outside jest.config.js collectCoverageFrom, and no test imports either page or use-ledger.ts. The repo's own precedent is src/hooks/register-ledger-permission-window.ts, extracted from this same hook because "the repo has no React-hook harness" — the same move fits, or have runLedgerFlowControl return a 'return-to-main' | 'dismiss-this-window' | 'end-flow' decision the page switches on, which has no permutations. For the last one, a real-store case in the style of close-windows-on-response.test.ts (makeRealStore + openWith).
There was a problem hiding this comment.
Took the second of your two suggestions — the decision return — since it removes the permutation space rather than testing it.
runLedgerFlowControl(isPermissionWindow, id, effects) is now decideLedgerFlowControl(isPermissionWindow, ownPermissionWindowId): 'end-flow' | 'dismiss-this-window' | 'return-to-main', and each page switches on the result. There is no object literal left to permute, and the swap you described stops being expressible.
On the other three:
use-ledger.ts:303—closeLedgerFlowWindows({})no longer type-checks its way past the problem, because the payload is now the ownership proof: the guard isif (ownPermissionWindowId == null) return, and the dispatch passes both fields. A missingrequestIdis still a valid shape for the internal flows, but the background now rejects arequestIdthat disagrees withsender.url, so "the wrong flow described" is caught at the boundary rather than only by a reader.use-ledger.ts:300-308— still no React-hook harness, so the falsy-id early return is still not driven by a test. What it does now isconsole.warninstead of returning silently, so the case you named — a permission window whose slot was released cannot end its flow — leaves evidence rather than a dead button. It is also reachable for a few ms on mount beforewindows.getCurrent()resolves, which the warning names explicitly.close-ledger-flow-windows.test.ts:151— addedclose-ledger-flow-windows.integration.test.ts, built the way you pointed at (makeRealStore+openWith, real reducers, drivinghandleReduxActionthenhandleWindowRemovedthen the grace timer). It pins that the abandoned request is answered exactly once with{cancelled: true}, and that a second dapp's approval window survives the abandon and keeps its own descriptor open. That is the assertion the mocked unit suite could not make.
src/apps/** and src/hooks/** are still outside collectCoverageFrom, and neither page is rendered by any test — that part of your note stands.
There was a problem hiding this comment.
Thanks — taking the four in turn, since they came in as one.
close-ledger-flow-windows.test.ts:151 — closed. close-ledger-flow-windows.integration.test.ts is the assertion the mocked suite could not make, and it holds under mutation: dispatch windowRequestResponded({ requestId }) inside handleCloseLedgerFlowWindows and the integration file fails at :99 with Expected number of calls: 1, Received number of calls: 0. The same edit against the previous head left all 23 tests in both files green. That gap is pinned.
ledger-flow-controls.ts decision — mostly closed. The permutation space is genuinely gone: decideLedgerFlowControl returning a union means a mistyped decision is a compile error. What is not closed is the surface the note was filed against — the page wiring. sign-transaction/index.tsx:339-348 and sign-message/index.tsx:213-222 are still hand-bound with nothing checking them, and neither consumes the union exhaustively: both are two ifs with 'return-to-main' as an implicit fall-through, so moving closeCurrentWindow() into the 'end-flow' branch type-checks and ships green.
use-ledger.ts dispatch — the type argument does not hold. closeLedgerFlowWindows({}) still compiles: ledger/actions.ts:30-33 declares both payload fields optional, so the payload is not yet the ownership proof the type would have to make it. What that mutation now produces is different but not better — the message is dropped at redux-actions.ts:275-277 and nothing is closed at all. The boundary failing closed is a real improvement over the previous head; the seam is still absent.
ledger-flow-controls.test.ts case — half. As a pure return-value assertion at :10-12 it no longer reads as end-to-end coverage, and the console.warn at :347-353 does leave evidence where the old code returned silently. But nothing in the tree drives closeNewLedgerWindowsAndClearState, so its reachable surface is still unchecked — and it grew: ownPermissionWindowId is null both when the slot was released and, for every page that is the permission window, until windows.getCurrent() resolves.
Agreed on the last line of your reply, and it is the through-line here: src/apps/** and src/hooks/** outside collectCoverageFrom is what leaves all three of the remaining halves unenforced.
There was a problem hiding this comment.
Both halves you left open are closed in 0da4daea, and you were right that the type argument did not hold.
The page wiring. Two ifs with 'return-to-main' as an implicit fall-through is exactly the surface the note was filed against, and the union did nothing about it. Both sites are now an exhaustive switch with an assertNever default (sign-transaction/index.tsx:342-358, sign-message/index.tsx:216-229, ledger-flow-controls.ts:26-30). Moving closeCurrentWindow() into the 'end-flow' branch is still expressible — nothing types away a wrong body — but a new member of the union is now a compile error at each call site instead of silently taking the fall-through, which is the half that was actually unguarded.
closeLedgerFlowWindows({}). Agreed, and fixed at the declaration rather than around it: permissionWindowId is required in the payload type (ledger/actions.ts), required in CloseLedgerFlowWindowsTarget, and the background drops a message that lacks one with a warning (redux-actions.ts:281-286) — the id is the ownership proof, so a payload without it names nothing the handler may close. {} is now a compile error, and the runtime guard is the layer that does not depend on the type staying right. The old "payload-less is routed with undefined" test now pins the drop, plus a case for a payload naming a request but no window.
closeNewLedgerWindowsAndClearState. This is the one your other note turned out to be about, and it is answered there: the ownership rule is now resolveOwnPermissionWindowId in src/hooks/ledger-window-ownership.ts with its own test file, so the reachable surface you named — including the case where ownPermissionWindowId is null while the slot is set — is pinned as a rule even though the hook still is not rendered by anything.
The integration test. Thanks for running the mutation; I read that as closed too.
And yes on the through-line: src/apps/** and src/hooks/** are still outside collectCoverageFrom, and neither page is rendered by any test. Extracting the rule is a way around that for one decision at a time, not a fix for it.
`state.ledger.windowId` is one global scalar with no per-request keying, and both the UI control and the background teardown were reading it as "this flow's permission window". A second flow holding the slot made the approval window dismiss itself — cancelling a live dapp request — and made a teardown remove the other flow's window mid device-confirmation while `ledgerStateCleared` wiped the deploy it was signing. Ownership is now proved by the caller. `use-ledger` derives `ownPermissionWindowId` from the window it opened, or from `windows.getCurrent()` for a page that IS the permission window, and passes it in the `closeLedgerFlowWindows` payload. The handler no longer reads the slot for targeting, subtracts windows other open requests still claim, and clears the slice only when it still names a window being removed. Also: - `runLedgerFlowControl(effects)` becomes `decideLedgerFlowControl` returning a decision the page switches on. An object of three `() => void` members type-checks under every permutation of its bodies, and no test in this tree can render either page. - The error CTA restores page state on every path again. `closeCurrentWindow` rejects, and on a non-popup window resolves having done nothing; either left the user on a dead screen with no route back. - The raw-JSON back arrow no longer routes through the Ledger flow control, so leaving that view cannot cancel the dapp request. - The `closeLedgerFlowWindows` message binds `payload.requestId` to the id in `sender.url`. The sender gate admits every wallet page and stops there, and this branch decides a request's lifecycle regardless of its `method`. - A real-store test drives the abandon path through `handleWindowRemoved` and pins that the abandoned request is still answered, which the unit suite could not see.
…er-signature-closes-every-popup-window-in-the-profile-cancelling-unrelated-dapp-approvals-and-the-secret-key-export-window' into WALLET-1416-cw-completing-a-ledger-signature-closes-every-popup-window-in-the-profile-cancelling-unrelated-dapp-approvals-and-the-secret-key-export-window
Comp0te
left a comment
Comp0te
left a comment
There was a problem hiding this comment.
Re-read the increment against the round-1 notes. Four of the five threads are closed by c3e9640a — the ownership derivation, the handler's targeting and its claimedByOthers subtraction, the sender binding on requestId, and the unconditional page-state reset all check out at the lines you named. The one new note below is the other side of that same ownership question: the two witnesses ownPermissionWindowId is derived from are both per-document, so an instance that is neither the opener nor the window itself reads as a non-owner.
| // the two flows reads as "no permission window of mine" rather than as someone | ||
| // else's. Still derived from the slot, so a window this instance opened and | ||
| // then lost stops counting once the background clears the stale id. | ||
| const ownPermissionWindowId = |
There was a problem hiding this comment.
ownPermissionWindowId (:236-241) recognises two owners: the instance that called openNewSeparateWindow (openedPermissionWindowIdRef, assigned only at :267) and the instance rendering inside the window (hostWindowId, from the one-shot windows.getCurrent() at :211-230 — empty deps, and the .catch at :221-225 only logs). Both are per-document, so an instance that is neither computes null while the slot is still set, and closeNewLedgerWindowsAndClearState takes the early return at :347-353: one console.warn, no dispatch.
Repro: popup → Import account from Ledger → the permission window opens and takes focus, so the popup document is torn down → reopen the popup and navigate back to the Ledger screen (navigation-menu/index.tsx:191-204 → app-router.tsx:387-388) → press the X in LedgerConnectionView. Nothing happens but the console line. That is a regression in this control: base use-ledger.ts:285-292 gated on if (windowId) — the same global slot, so any instance passed — and closed the window and dispatched ledgerStateCleared() from the hook itself.
Two things bound it, and both are worth stating because they are why this is not a stuck wallet: the permission window's own X still works (hostWindowId === windowId there), and window-removed.ts:22-25, new in this PR, clears the slice however that window goes away — so the !windowId gate at :246-251 unblocks the next flow as soon as it closes. LedgerDisconnectedFooter's Connect CTA on the same screen also clears it (:66-68, :90-92). The cost is a silently dead control on the popup side, not an unrecoverable state.
The fix the codebase already suggests is a third witness that survives a remount: isLedgerNewWindow is derived from the URL (sign-transaction/index.tsx:93) and import-account-from-ledger/index.tsx:13-16 reads the same param, so an opener id carried in the URL or persisted in the slice would make the re-mounted popup a legitimate owner again.
One open question rather than a claim: can windows.getCurrent() reject in an extension page? If it can, hostWindowId stays null for that window's whole life and the comment at :348-350 ("briefly, on mount") would not hold — the X, the end-flow branch at sign-transaction/index.tsx:339-341 and the auto-close at :363-372 would all be dead there, and the .catch at :221-225 has no retry.
There was a problem hiding this comment.
Confirmed, including the repro — popup → Import account from Ledger → the popup document goes away when the permission window takes focus → reopen → the X does nothing but log. Fixed in 0da4daea.
Both witnesses were per-document, which is the whole defect: neither of them can describe a document that no longer exists. The third one rides in the slice instead, next to the id it qualifies — ledgerNewWindowIdChanged now carries { windowId, openerWindowId } (redux/ledger/reducer.ts:21-25), where openerWindowId is the window the opening instance was rendering in. A remounted popup re-derives that same id from its own windows.getCurrent(), so it is an owner again.
The decision moved out of the hook into src/hooks/ledger-window-ownership.ts — resolveOwnPermissionWindowId({ slotWindowId, openerWindowId, openedWindowId, hostWindowId }), eight cases in ledger-window-ownership.test.ts. That is the seam your last note asked for twice: the hook has no harness, but the rule does.
One narrowing worth naming rather than leaving to be found: the opener witness is per window, not per instance, so a different instance rendering in the same browser window also reads as an owner. Base gated on if (windowId) and so admitted every instance anywhere; this is narrower than that, not wider — but it is not "only the flow that started it" either.
On your open question — can windows.getCurrent() reject in an extension page. It resolves for any rendered document: no permission is involved and the call is answered by the window containing the caller. The failure mode is a caller with no containing window (a service worker with no open windows), which is not this. So the "briefly, on mount" comment holds for the popup side.
What I am not claiming is that it cannot fail. If it did, the page that IS the permission window would still be stuck, because hostWindowId is its only witness — it did not open itself, and its openerWindowId names the popup. What bounds that is the same thing that bounds the case you found: the window's own titlebar close still works, and window-removed.ts:22-25 clears the slice however it goes away, so the next flow is not blocked. I left it there rather than adding a retry with no reachable trigger to test it against.
…pleting-a-ledger-signature-closes-every-popup-window-in-the-profile-cancelling-unrelated-dapp-approvals-and-the-secret-key-export-window # Conflicts: # src/background/handlers/sdk-response-to-tab.test.ts
`ownPermissionWindowId` recognised two owners, both per-document: the instance that opened the permission window (a ref) and the instance rendering inside it (`windows.getCurrent()`). The popup document is torn down when the permission window takes focus, so a reopened popup back on the Ledger screen was neither: its X produced a console warning and nothing else, where the pre-ownership code gated on the global slot and so admitted any instance. Add a third witness that survives a remount — `openerWindowId`, persisted in the ledger slice beside `windowId` and matched against the remounted instance's own window. Ownership moves out of the hook into `resolveOwnPermissionWindowId`, which has the seam the hook cannot get: the repo has no React-hook harness. Also from the same review round: - `permissionWindowId` is required in the `closeLedgerFlowWindows` payload, in `CloseLedgerFlowWindowsTarget`, and behind a background guard that drops a message without one — the id is the ownership proof, so a payload that omits it names nothing the handler may close. - Both pages consume `LedgerFlowControlDecision` through an exhaustive `switch` with `assertNever`, instead of two `if`s with an implicit fall-through that let a moved `closeCurrentWindow()` type-check.
…ure-closes-every-popup-window-in-the-profile-cancelling-unrelated-dapp-approvals-and-the-secret-key-export-window
…quest The opener witness matched on the browser window alone, and approval windows are a single tracked slot the next request reuses in place. So a fresh document inherited the previous request's claim on the permission window: `decideLedgerFlowControl` returned 'dismiss-this-window', the back arrow closed the approval window, and the dapp got a cancellation the user never asked for — the collateral cancel this branch exists to close, by a second route. Record the opening flow's requestId next to `openerWindowId` and require both to match. Two flows sharing one browser window are now separated; a flow remounting in its own window still owns what it opened, and the internal flows, which carry no request, are unaffected. Both guards on that branch are now pinned: dropping the `openerWindowId` null-check fails one case, dropping the request-id conjunct fails two. The case asserting that a sibling in the opener window owns the slot was asserting the defect, and is now its inverse.
…ger confirmation (#1476) * fix: WALLET-1394 — keep the approval window out of reuse during a Ledger confirmation An already-permitted Ledger device opens no permission window, so the device call runs in the page inside the SHARED approval window. Reusing that window for the next dapp request runs `tabs.update` on its tab — a full navigation that destroys the document, and with it the per-document HID session and the pending signature. When the page does outlive the 250 ms grace, the signature instead reaches `sdk-response-to-tab` and is dropped by the supersede tombstone. Either way the user's confirmation is lost and the dapp is told `cancelled: true`. PR #1427/#1462 covered only the other path, where a separate permission window gives the request a second display and the reuse leaves it alive. Mark the request as awaiting the device for the duration of the call and withhold the window it runs in from reuse. Passing a null windowId REDIRECTS the reuse rather than suppressing it: the new-window branch still runs `setWindowId`, so the shared slot retracks to the newcomer and only the protected window leaves the rotation. The flag lives on the open request descriptor, not in a slice-level set, so it cannot outlive what it describes — the tombstone replaces the whole entry and a detach shrinks `windowIds`, which is the other half of the question. * fix: WALLET-1394 — hold the device flag per bracket, not per call Neither signing page disables its submit control while a Ledger call is in flight, and the page state that hides it is only flipped after `getPreferredTransport()` and `beforeLedgerActionCb()` resolve. A second click in that gap starts a second bracket, fails fast on the busy transport, and its `finally` released the window while the first call was still on the device. Count the holders per requestId so overlapping brackets report the flag once between them. Module scope is the right scope: a transport is per document, so brackets can only overlap within one.
2 participants
WALLET-1416 — close the windows a Ledger flow owns
https://make-software.atlassian.net/browse/WALLET-1416
The bug
On a dapp
sign/signMessagewith a Ledger account, when the device needs a WebHID permissionprompt the wallet opens a second window. The flow's cleanup asked the browser for every popup
window in the profile and removed all of them — other dapps' approval windows (each removal
reaching the cancel-on-close path and cancelling a live request), the secret-key export window,
import-account-with-file windows, and popup windows belonging to ordinary web pages. It never
closed its own permission window, because that one is created
type: 'normal'while the filterasked for
'popup'.The model: ownership is proved, never inferred
state.ledger.windowIdis one global scalar with no per-request keying(
redux/ledger/reducer.ts:5-19). It says a Ledger permission window exists somewhere in theprofile; it says nothing about whose. Everything below follows from refusing to read it as an
answer to "is this window mine".
A caller can always produce that answer honestly, in one of two ways: it opened the window, or
it is the window.
useLedgerderivesownPermissionWindowIdfrom exactly those two facts —a ref set at creation (
use-ledger.ts:267) andwindows.getCurrent()for a page rendering insidethe permission window (
use-ledger.ts:209) — and cross-checks the result against the slot(
use-ledger.ts:236-240), so a window it opened and then lost stops counting once the backgroundclears the stale id. A takeover reads as
null— "no permission window of mine" — never assomeone else's. The hook no longer returns the raw slot at all, so no page can branch on it.
What is in this PR
use-ledger.ts), replaced by a background command that closes only thecaller's own permission window ∪
requests[requestId].windowIds. That is the CTA/abandon path.close-ledger-flow-windows.ts). It neverreads
state.ledger.windowIdfor targeting; it takespermissionWindowIdfrom the payload(
:32), subtracts every window another open request still claims (:60-69— the samesubtraction the response-path sibling does, and for the same reason: a shared window is that
request's only display), and dispatches
ledgerStateCleared()only when the slice still namesthe id and that id survived the subtraction (
:74-80), so a flow that took the slot overkeeps the
deploy/transactionit is signing.requestIdto the sender (redux-actions.ts:275).isTrustedUiSenderadmits every wallet page and stops there, and this branch decides a request's lifecycle without
consulting
method—connect,switchAccountanddecryptMessagewere treated like a Ledgersign. Every legitimate dispatcher's page URL already carries the id, including the permission
window's (
use-ledgerbuilds that URL from the same params), so the comparison againstnew URL(sender.url).searchParams.get('requestId')needs no special case.every subscriber in the codebase sits behind
debounceTime(300)(ledger.ts:44-45), so on asuccessful sign the command arrived ~300 ms after the descriptor had already collapsed to
{ status: 'responded' }and droppedwindowIds.close-windows-on-response.tssnapshots thewindow set in the same synchronous block that marks the request responded, delivers the
response, then re-checks ownership against
selectOpenRequestsand removes what is still ours.back-arrow shared one handler that tore down the whole flow; in the approval window that made it
the last removal, so the dapp was answered
{cancelled:true}250 ms later while the user wasconfirming on the device. With a permission window of this flow live, both controls now
dismiss only the approval window —
candidatesrequireswindowIds.length === 1(
cancel-requests.ts:178), so the request survives and the signature still answers the dapp.decideLedgerFlowControlreturns'end-flow' | 'dismiss-this-window' | 'return-to-main'and each page switches on it. Theeffects-object it replaced had three
() => voidmembers, so every permutation of its bodiestype-checked — and the swap that binds
dismissThisWindowto the permission window turns theuser's only exit into a silent no-op (
closeCurrentWindowremoves a window only whenwindows.getCurrent().type === 'popup', while the permission window istype: 'normal') withtsc, the suite and CI all green.
closeCurrentWindowrejects, and resolves having done nothing on a non-popup window; eitherone used to leave the user on a dead error screen with no route back to the transaction details.
RowDataContenttoo,so leaving that view ran the Ledger teardown — reachable with no Ledger error involved at all.
It now sets page state and nothing else.
ledger.windowIdis cleared by the background onwindows.onRemoved. That slot isa mutex; without this, (5) would trade a wrong cancel for a session-long Ledger lockout. The two
are not separable.
What is deliberately not in this PR
provide permission to connect your Ledger device" for the whole flow. Follow-up ticket.
after a restart nothing is closed and both windows stay on screen. The signature still reaches
the dapp and nothing is cancelled. Accepted and documented; follow-up ticket.
nothing answers the dapp until they act in that window. The permission window is the focused one
throughout the device phase and every way of closing it answers the dapp, but the wait is
unbounded under user inaction.
What is NOT proven
grep -rli ledger e2e-testsreturns nothing), andthe popup e2e suite runs with
MOCK_STATE=true. Nothing end-to-end exercises this flow.src/hooksandsrc/appsare outside the coverage gate (jest.config.jscollectCoverageFromcovers onlysrc/background/redux/**/reducer.ts,src/background/handlers/**/*.tsandsrc/background/redux/sagas/**/*.ts). No test importseither signature-request page or
use-ledger.ts, and there is still no React-hook harness in thetree.
decideLedgerFlowControlhas its own unit tests and, returning a value rather than callingeffects, has no permutations left to get wrong — but the page wiring around it is verified by
nothing beyond
tsc.windows.getCurrent()resolves asynchronously, so for a few ms after mount a page that IS thepermission window has no
ownPermissionWindowIdand its teardown control is a no-op. It logs aconsole.warnrather than returning silently. Nothing drives this window, and the interval is farshorter than any user interaction, but it is unmeasured.
hardware behaviour; QA item 11 below is the only instrument.
transport probe those targets do not pass today, so this path is effectively Chrome-only.
windows.removesemantics on a dead id, on any target. The unit tests prove control flowagainst
jest.fn()s and nothing about the browser.windows.onRemovedfires for a window the background removed itself (premise E6). It isa browser API contract, not provable from this tree. This PR makes it load-bearing: it is the only
recovery for a stale
ledger.windowId, so if it did not hold, the mutex would stay set and nolater Ledger flow could open a permission window for the rest of the session. QA item 13 is the
only instrument.
equality, so a recycled id could make a caller's proof read true for a window it does not own.
Bounded — the caller would have to have opened, or be rendering in, a window that has since been
destroyed and its id reissued — but unmeasured.
Known, unchanged by this PR: closing the wallet's windows does not clear a pending prompt on
the Ledger device — it is resolved only on the device or by the transport closing. That was already
true when the permission window was orphaned instead of closed.
QA status — Chrome + real Ledger
The hardware run below predates the ownership rework (commits up to
6875327b). The reworkchanges which window each control acts on, so every item that involves a permission window needs to
be run again against the current head. Treat the list as unrun.
The earlier run passed: 1 (the ticket), 2 (
signMessage), 5 (a second dapp's request survives,verified with the HID permission revoked so a permission window was genuinely in play), 6
(export-keys window and an ordinary web popup survive — the regression check), 9 (disconnect
mid-flow), 11 and 12 (both approval-window controls), 13 (no lockout), 14 (the permission window's
control is still the teardown), 15 (no permission window, no behaviour change). Items 3, 4, 7, 8
and 10 were never run.
QA checklist — Chrome with a real Ledger device
signwith a Ledger account, device not yet permitted → permission window opens →confirm on device. Both windows close and the dapp receives the signature. (The ticket.)
signMessage.the dapp receives
{cancelled:true}after ~250 ms. Repeat and press "Got it" in the approvalwindow instead: only that window closes, the dapp is still pending, and the permission
window's own control finishes the job.
closes itself; nothing else is touched.
chrome://settings/content/hidDevices),and confirm a permission window actually opens. WebHID permission persists once granted, so
after any earlier item on this list the flow runs with NO permission window, the request then
has a single window, and reusing it for a second request cancels it — correct pre-existing
supersede behaviour, not this fix. With both windows open, raise a second dapp request from
another tab (it reuses the shared approval window, which then shows the second request), then
confirm the first signature on the device → the second dapp's approval window must survive and
stay usable.
window.open-style popup → confirm on the device → neither may close. (The WALLET-1416regression check.)
transferwith Ledger viasign-with-ledger-in-new-window→ Success → Close → onlythat window closes.
import-account-from-ledger→ import → Close → only that window closes.the dapp origin in the extension popup so the approval window's footer flips to the
Connect/Cancel branch, then press Cancel. The dapp receives
{cancelled:true}and BOTHwindows close — before this work the permission window was orphaned and
ledger.windowIdstayed set, blocking every later Ledger flow. Then reconnect the account (it was just
disconnected) and start a fresh Ledger sign: a permission window must still open.
no Sign button, a single 'Got it' footer button, and the text still says "Please provide
permission…" even after permission was granted. Screenshot it for the progress-channel
follow-up.
touching the device, switch to the approval window and press 'Got it'. Only the approval
window closes; the permission window stays and the device prompt is still live; confirming on
the device still delivers the signature to the dapp. On the branch point
b9170bf8(nowinside
develop) both windows close and the dapp gets{cancelled:true}— run it there firstto see the difference.
the OS control (the dapp should receive
{cancelled:true}). Then start a fresh dapp Ledgersign: a permission window must still open.
device prompt, press the back-arrow in the permission window: both windows close and the
dapp receives
{cancelled:true}.device already permitted, press 'Got it' → the approval window returns to the transaction
details with a working Cancel and Sign; nothing closes.
and press the header back-arrow. The page returns to the transaction details and the dapp
request stays pending. Before this rework that arrow ran the Ledger teardown whenever any
permission window was tracked, including another flow's.
Firefox and Safari QA of this fix is close to worthless (the permission window is not reachable
there through a supported transport), but a smoke test that non-Ledger dapp flows are unaffected on
both is still worth running.
Coverage
close-ledger-flow-windows.test.tspins the removal set, the ownership refusals (a second flow holding the global slot) and the subtraction of a window a second open request still claims.redux-actions.test.tspins the sender binding in three directions: a page naming a request itdoes not display, a page omitting the id its own URL carries, and an absent
sender.url.close-ledger-flow-windows.integration.test.tsruns the abandon path over a real store throughhandleReduxAction→handleWindowRemoved→ the grace timer, and pins that the abandoned requestis answered exactly once with
{cancelled:true}and that a second dapp's approval window surviveswith its descriptor still open — the assertion the mocked unit suite cannot make.
Cited rather than duplicated:
cancel-requests-displaced-by.test.ts:67-83andcancel-open-requests-on-close.test.ts:62-69already pin the invariant items 3 and 11 depend on —two
windowIdsmeans a removal detaches but never cancels. Both pass unchanged.