feat(web): add automatic LOCK setting and Vault Format 2 - #150
Conversation
miso-develop
left a comment
There was a problem hiding this comment.
Review disposition: REWORK_REQUIRED
Independent Review completed against Issue #141 / Decision #138 at exact head 7816dab6f6b8bdef9ac062d8a3dfe201999cbd05.
Blocking finding — unsaved automatic-LOCK draft leaks across Device connection boundaries
web/src/auto-lock-settings.ts stores the editable form as controller-local draft + dirty state. Once the user edits the form, dirty becomes true, and render() intentionally stops loading canonical values while dirty:
if (!dirty && snapshot?.autoLock.known) draft = autoLockDraftFromCanonical(snapshot.autoLock.days);However, the controller is created once for the page lifetime and neither dirty nor draft is cleared when its Device/canonical context disappears or changes. web/src/main.ts reuses that same controller across disconnect/reconnect; disconnectDevice() clears management, serialSession, and snapshot, then the next Device connection supplies a new snapshot to the same controller.
Reproducible path:
- Connect Device A while active/unlocked/Format-2-writable. Suppose A's canonical automatic-LOCK setting is disabled.
- Enable the setting in the form (for example 1 day) but do not Save. The controller now has
dirty=trueand an A-derived draft. - Disconnect Device A. The form becomes non-writable, but its
dirtyflag and draft remain in memory. - Connect Device B while active/unlocked/Format-2-writable. Suppose B's canonical setting is 31 days.
render(snapshotB)skipsautoLockDraftFromCanonical(snapshotB.autoLock.days)becausedirtyis still true. The form therefore continues to show A's unsaved draft rather than B's effective canonical setting.- Since B is writable and
dirty=true, Save becomes enabled. Pressing Save writes A's stale draft throughsetAutoLockDays()to Device B.
The reverse case is security-significant: an unsaved disabled/null draft created for Device A can be carried into Device B and disable B's existing automatic-LOCK policy with a single Save.
This violates Issue #141's requirement to show the currently effective configured state/value and the reconnect/refresh behavior expected by its Acceptance Criteria. It also permits a security-setting edit to cross Device/canonical ownership context without an explicit new edit for that Device.
Required completion condition
The editable automatic-LOCK draft must be bound to the canonical/Device context it came from. When that context is lost or replaced (at minimum disconnect and different Device/canonical identity), stale dirty state must not suppress loading the newly connected Device's canonical setting, and stale Save must not be possible. Add regression coverage using the actual controller/form lifecycle for a dirty Device-A draft -> disconnect/context loss -> Device-B snapshot with a different canonical setting, proving Device B renders its own setting and cannot save A's stale draft.
The exact reset strategy is implementation-defined; same-Device UX may preserve an unsaved edit only if it cannot cross a changed canonical/Device context or mask authoritative state after reconciliation.
Other review results
No additional blocking findings were identified in the reviewed head. The Vault Format 1/2 framing and distinct AAD domains, explicit Format-2 capability gate, F1->F2 same-VMK/vault_id generation advance, format-aware reconciliation, no ordinary F2->F1 downgrade, Recovery Package v1 F1/F2 handling, null/1..31 setting representation, authenticated canonical mutation path, and immediate-LOCK response handling are consistent with Issue #141 / Decision #138. Adjacent Device PR #152 uses compatible PT2/AAD2 framing and advertises [1,2] without overlapping this Web PR.
Exact-head Foundation #742 and Security #745 are independently confirmed green.
miso-develop
left a comment
There was a problem hiding this comment.
Implementation rework at exact head c09cb6b6809fbffe92f5076d9e262db40005ad77 addresses the stale automatic-LOCK draft context leak. Durable handoff is being recorded on Issue #141. Implementation Agent does not merge.
miso-develop
left a comment
There was a problem hiding this comment.
Review disposition: READY_FOR_INTEGRATION
Independent re-review completed for Issue #141 / PR #150 at exact head c09cb6b6809fbffe92f5076d9e262db40005ad77.
The previous blocking finding from Issue #141 comment 5701293265 is resolved.
Previous blocker resolution
web/src/auto-lock-settings.ts now binds the editable draft to a canonical context identity composed of Device ID, vault ID, generation, Vault format, registration ID/epoch, and browser ownership. When that identity changes — including snapshot -> null on disconnect/context loss — the controller clears dirty, clears stale status flags, and reloads the authoritative canonical automatic-LOCK state.
This closes both stale-draft directions:
- Device A disabled -> unsaved enabled draft -> disconnect -> Device B configured for 31 days renders B's enabled/31 state; stale Save is disabled and cannot call the save handler.
- Device A enabled -> unsaved disabled/null draft -> disconnect -> Device B configured for 31 days cannot have its active policy disabled by the stale A draft.
The new Chrome DOM regression exercises both paths using the production controller and also verifies that an unsaved draft survives a rerender only while the canonical context identity is unchanged.
Saving lifecycle
A legitimate save may advance canonical generation while saving is active. The context-change path intentionally does not clear saving; it reloads the newly authoritative canonical draft while controls remain disabled by saving/busy state. When the save promise resolves, the normal completion path clears saving and renders the committed state. No stale Save window is introduced by the generation change.
Regression / scope review
The rework from previously reviewed head 7816dab6f6b8bdef9ac062d8a3dfe201999cbd05 is exactly three files:
web/src/auto-lock-settings.tsweb/tests/browser/auto-lock-context-smoke.tsweb/tests/browser/combined-smoke.ts
No Vault Format 1/2 codec/crypto, capability-gating, canonical transaction/reconciliation, Recovery Package, or secret-persistence implementation changed in the rework. The previously reviewed semantics therefore remain intact, and exact-head regression CI is green.
Independent CI / integration-state verification
At exact head c09cb6b6809fbffe92f5076d9e262db40005ad77:
- Foundation #746: SUCCESS
- Web: 32 test files / 228 tests passed
- Linux Chrome production smoke: SUCCESS, including the automatic-LOCK context stage through
combined-smoke.ts - TypeScript/Vite production build: SUCCESS
- Windows Chrome job: SUCCESS
- firmware/build/package baseline and lock checks: SUCCESS
- Security #749: SUCCESS
Current main remains d99d4824990b6ebd7ddb670ee31aed4aeb4fc3f8, identical to the PR base. PR #150 is open, non-draft, unmerged, and mergeable. There are no unresolved inline review threads.
Findings
Blocking: none.
Non-blocking: none.
Disposition: READY_FOR_INTEGRATION. Review does not merge.
Implements #141.
Scope
supported_vault_formatsexplicitly containing2setAutoLockDays()through the existing authenticated encrypted canonical generation transactionCompatibility / security behavior
vault_idand VMK, advances generation by one, uses a fresh nonce, and reuses the existing transaction journal / reconciliation pathauto_lock_daysis not persisted as a separate plaintext settingReview rework
Resolved the blocking
REWORK_REQUIREDfinding recorded in Issue #141 comment5701293265and the PR review at prior head7816dab6f6b8bdef9ac062d8a3dfe201999cbd05.The automatic-LOCK editable draft is now bound to canonical context identity: Device ID, vault ID/generation/format, registration identity/epoch, and browser ownership. A context loss or replacement clears
dirtyand reloads authoritative canonical state, so an unsaved Device-A draft cannot cross into Device B or survive a changed canonical identity.Same-context rerenders may preserve the unsaved local draft. The controller intentionally does not clear the active
savingstate merely because a legitimate save advances canonical generation.This rework does not modify Vault Format 1/2 codec/crypto, capability gating, canonical transaction/reconciliation, Recovery Package, or secret persistence paths.
Regression coverage
Added/updated tests cover:
[1,2]supportLOCKEDafter applying an already-expired shorter lifetimeExact-head verification
Exact PR head:
c09cb6b6809fbffe92f5076d9e262db40005ad77Coordination
d99d4824990b6ebd7ddb670ee31aed4aeb4fc3f8; branch has no base divergenceweb/src/auto-lock-settings.tsweb/tests/browser/auto-lock-context-smoke.tsweb/tests/browser/combined-smoke.tsState
READY_FOR_REVIEWImplementation Agent must not merge this PR.