fix: the restored viewStack can contain views the popup may not reopen onto, giving a dead-end screen #224
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
state.viewStackis persisted and restored verbatim insrc/shared/state.js.RESTORABLE_VIEWScorrectly stops the popup opening ONTO a non-restorable view, but nothing filters those views out of the restored stack itself.Consequence: a secret-bearing screen such as
export-privkey— or the newshow-phrase— can sit in the restored stack. The popup opens on a permitted view, the user presses Back, and lands on an empty dead-end screen whose content was deliberately never re-rendered. Onshow-phrasethere is no Back control either, so it is not merely ugly.To be clear about what this is and is not: no secret leaks. The views render empty because their content is never restored. This is a navigation defect, not a disclosure one.
General to every non-restorable view rather than specific to any screen, which is why it was left out of #215 — fixing it there would have changed
export-privkeynavigation as a side effect of a feature PR. Found by the independent review of that PR.Implementation requirements
RESTORABLE_VIEWSinloadState(), so a restored stack contains only views the popup is willing to render.Definition of done
TODO.mdupdated in the same commit.make checkpasses.Implemented in #266.
loadState()now truncates the stored stack at the first entry outsideRESTORABLE_VIEWS(that entry and everything above it), rather than splicing it out: the result is always a prefix of what was stored, so every surviving entry keeps exactly the Back target it had. Splicing would silently re-point the entry above the hole. If truncation leaves nothing and the restored view is a restorable non-root view,mainis put beneath it so Back always has somewhere to go.Filtered on load, not on save: only a load-side filter repairs the stacks already in storage (including ones written when a view was still in the set), and the live in-session stack is legitimate while the popup is open.
saveState()is unchanged.Verified: the new cases in
tests/state.test.js(non-restorable view at top, middle, bottom, plus an unknown view name and the restore-onto-maincase) were run against unmodifiednextfirst and failed 6 of 6; they pass with the fix.make checkgreen on the rebased branch — 585 tests in 25 suites,test-verify-build18 cases, prettier clean — and green again inside the container viascript/cibuildwithdocker build --no-cacheon this image, where theRUN make checklayer executed rather than reportingCACHED.src/popup/restorableViews.jsis untouched.Narrowing the second DoD item. "Pressing Back after reopening never lands on an unrendered screen" is not achievable by filtering the stack, and the independent review of #266 proved why:
goBack()re-renders nothing, so Back lands on a blank template for ordinary restorable views too — no non-restorable view required. That is a wider, pre-existing defect and is now #268.This issue's item 2 therefore reads: pressing Back after reopening never lands on a view the popup declined to restore. The general blank-on-Back case belongs to #268 and this unit should not grow to cover it.