feat(fe): hold a browser key and rotate it on every sign-in - #4271
Merged
Conversation
sea-snake
marked this pull request as ready for review
August 22, 2026 18:16
|
✅ No security or compliance issues detected. Reviewed everything up to 80c7e09. Security Overview
Detected Code Changes
|
sea-snake
force-pushed
the
fe/browser-key-store
branch
from
August 22, 2026 18:45
928d1be to
836aef3
Compare
sea-snake
force-pushed
the
fe/browser-key-store
branch
from
August 22, 2026 19:07
836aef3 to
0938c39
Compare
sea-snake
force-pushed
the
fe/browser-key-store
branch
from
August 22, 2026 19:25
0938c39 to
2e2b753
Compare
The canister identifies a browser by a key it proves possession of, so the frontend has to hold one. A non-extractable P-256 keypair in IndexedDB, per identity, and a successor generated alongside it: the sign-in signs with the current key and announces the successor, and the successor is promoted only once the canister confirms the sign-in. Promoting after confirmation rather than before is what survives a lost response: the browser still holds the key the canister has, so its next attempt presents the same one rather than a successor the canister never saw. Sign-ins are serialised with a web lock, because two at once would leave whichever wrote last holding a key the canister never accepted. Where the Web Locks API is missing the calls run unserialised, which is the accepted cost of not blocking sign-in on it. Nothing imports this yet; the sign-in that uses it lands two PRs up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sea-snake
force-pushed
the
fe/browser-key-store
branch
from
August 22, 2026 19:49
2e2b753 to
f7b8adf
Compare
MRmarioruci
reviewed
Sep 2, 2026
MRmarioruci
reviewed
Sep 3, 2026
MRmarioruci
reviewed
Sep 3, 2026
sea-snake
removed this pull request from stack #4277
September 9, 2026 20:32
sea-snake
added this pull request to stack #4324
September 9, 2026 20:34
sea-snake
removed this pull request from stack #4324
September 9, 2026 20:35
sea-snake
added this pull request to stack #4323
September 9, 2026 20:38
sea-snake
removed this pull request from stack #4323
September 9, 2026 20:42
sea-snake
added this pull request to stack #4326
September 9, 2026 20:43
MRmarioruci
reviewed
Sep 10, 2026
MRmarioruci
reviewed
Sep 10, 2026
MRmarioruci
reviewed
Sep 10, 2026
MRmarioruci
reviewed
Sep 10, 2026
…ld to `accept` was a callback the caller had to remember to fire, and forgetting it compiles. The record then keeps the key the canister has just retired, so every later sign-in pays a stale-key recovery round-trip; `currentBrowserId` stays absent, which costs the devices page its marker and `forgetIdentity` its canister call; and a browser that changed what it reports goes unnoticed, because there is no stored description to compare. `withBrowserProof` takes `browserIdOf` instead. The caller still supplies the id — it only exists in the canister's reply — but a required argument is something `tsc` refuses to let it omit, and the rotation happens here, once, beside the id it is written with. The store's own tests split along the line that now matters: `signIn` is a call the canister answered, `attempt` one it did not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
sea-snake
removed this pull request from stack #4326
September 10, 2026 12:55
sea-snake
added this pull request to stack #4328
September 10, 2026 12:55
MRmarioruci
approved these changes
Sep 10, 2026
`announced` is an adjective with no noun, leaving the doc comment to supply the word. With the noun in the name the comment says why the key is retained instead of what it is, and the rotation function's summary states what it returns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Lost-response and storage-failure paths can desynchronize local browser records from the canister and prevent correct recovery.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds per-identity browser keys that rotate after confirmed sign-ins.
Changes:
- Stores and rotates non-extractable P-256 key pairs.
- Handles stale keys, browser-description changes, and concurrent sign-ins.
- Adds comprehensive browser-key tests.
File summaries
| File | Description |
|---|---|
browser-key.store.ts |
Implements browser-key persistence, proofs, rotation, and recovery. |
browser-key.store.test.ts |
Tests signing, rotation, recovery, descriptions, and locking. |
Review details
Suppressed comments (2)
src/frontend/src/lib/stores/browser-key.store.ts:160
- A brand-new identity returns
undefinedhere, and the changed-description branch below also createsfreshwithoutdescription. Consequently, if that registration is accepted but its response is lost, IndexedDB contains the keys but not the description the canister registered. A description change before recovery then reuses/advances that entry and finally records the newer description locally, while the canister keeps the older one. Create the initial record here and persistdescriptionalongside every fresh key pair before callingsignIn.
if (
stored?.description === undefined ||
sameDescription(stored.description, description)
src/frontend/src/lib/stores/browser-key.store.ts:289
- Promoting a real successor drops the existing
browserIdand registereddescription. If this retry fails, the incomplete record remains; a later changed browser description is therefore not detected and the old canister entry is advanced instead of registering a new browser. Preserve those fields whenannouncedSuccessorexists, while keeping the fresh-key fallback free of the old browser ID.
const promoted: BrowserKeyRecord = {
keyPair: stored?.announcedSuccessor ?? (await generate()),
};
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
sea-snake
added a commit
that referenced
this pull request
Sep 10, 2026
…4273) Design: #4224. Overview: #4230. Depends on #4272 for the record store, #4271 for the browser key, and #4270 for the browser description — this is what gives all three their production caller. The canister side is complete after #4265, but nothing hands an app a session yet. This is the JSON-RPC method that does. **`ii_session_delegation`** is what an app calls instead of `icrc34_delegation` when it wants a session rather than a long-lived delegation. It answers with the session chain, extended to the app's own key, and the app mints five-minute delegations from it with no further browser involvement. The `ii_` prefix is deliberate: an RPC method travels over a transport shared with other providers and has to say which one it belongs to, which is why every II-specific RPC method carries it. **`targets` on the chain is restricted to the II canister.** That is a developer guardrail, not a defence against a thief — a thief holding the chain can refresh with it either way. Revocability is the protection that matters, and saying otherwise in a security table would be dishonest. **The consent duration is honoured**, and an SSO organization's cap still binds it: `valid_for` carries the lifetime the user chose, and an SSO identity sends a duration even when the user picked none, matching the ICRC-34 path. **The request carries only a session public key and an optional derivation origin.** An app cannot ask for an access level or a lifetime, because both are the user's to decide at consent. `icrc25_permissions` now reports `ii_session_delegation` as a granted scope so an app can discover the method. `icrc34_delegation` itself is unchanged. Tests: `sessionDelegation.test.ts` (5) covers a request for another method being ignored and params carrying no session key being rejected. The success path has no unit test here, which is worth knowing rather than glossing: the harness does not stub the authentication store, so `createSession` never completes in it, and neither this path's SSO clamp nor the ICRC-34 one is covered. #4248 stubs enough to drive the handler to an answer and exercises the `targets` restriction and the held-session path. Review `createSession` by reading it. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Design: #4224. Overview: #4230. Nothing imports this until the sign-in in #4273.
The canister identifies a browser by a key it proves possession of (#4264), so the frontend has to hold one, and rotate it, so a key captured from one sign-in cannot claim that browser at the next.
A non-extractable P-256 keypair in IndexedDB, per identity, plus a successor generated alongside it. A sign-in signs with the current key and announces the successor; the successor is promoted only once the canister has confirmed the sign-in.
Promoting after confirmation rather than before is what survives a lost response. If the reply never arrives, the browser still holds the key the canister has, so its next attempt presents the same one rather than a successor the canister never saw — which the canister would treat as an unknown browser and enrol as a second entry for the same machine.
Sign-ins are serialised with a web lock, because two at once would leave whichever wrote last holding a key the canister never accepted. Where the Web Locks API is missing the calls run unserialised: not blocking sign-in on it is the deliberate trade, and the window is one concurrent sign-in on the same device.
A browser that no longer matches what it registered as signs in as a new one. The description it reported (#4270) is stored beside the key pair, and compared before anything is sent. A registered entry keeps the description it was created with — the canister ignores what a sign-in reports once an entry is being advanced — so a browser whose brand, system, form factor or model has changed would otherwise keep rotating an entry describing something it no longer is. Where it differs, this presents a fresh key pair, which no entry holds and which therefore registers under its own.
Deciding here rather than being told keeps that a purely local operation: nothing has been sent when the comparison happens, so the entry left behind is untouched and a retry hits the same state and takes the same branch. The description is written with the key pair and never on its own, so the comparison is always against what the canister was actually sent.
The private key never leaves IndexedDB and never appears in a delegation chain.
Tests:
browser-key.store.test.ts(21), including the successor promoted only after the call succeeds, a lost response leaving the current key in place, concurrent sign-ins serialised, the store working with the lock API absent, and a changed description signing in on a fresh key pair while an unchanged one keeps rotating.