feat(be): store a revocable session on the account reference - #4266
Merged
Conversation
This was referenced Aug 22, 2026
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 f626fdd. Security Overview
Detected Code ChangesThe diff is too large to display a summary of code changes. |
sea-snake
force-pushed
the
feat/session-create
branch
from
August 22, 2026 18:44
84285d2 to
026f8c0
Compare
sea-snake
force-pushed
the
feat/session-create
branch
2 times, most recently
from
August 22, 2026 19:25
4175495 to
ead89cc
Compare
A session is a record on the account reference — created at, expires at, last refreshed, the browser that made it, and whether the user consented to queries only. Putting it there rather than in its own map means it inherits the caps that already bound references, and revoking, expiring and evicting reuse machinery that exists. Its identity is `H(salt, "session", account_seed, created_at, device_id)`, every field length-prefixed. Building on the account's own seed rather than on the numbers behind it is what makes a session survive anything that leaves the account's principal unchanged — naming a default account is exactly that. Only `last_refreshed` is mutable, which is why it is the one field the seed does not take: a mutable input would change the session's principal every time it was stamped. A ceremony from a browser that already holds a session at this account replaces it, so a copy of the old chain stops working at the user's next sign-in rather than at its expiry. Expired records on the row go in the same write. The writers here have no caller yet — the ceremony that calls them is two PRs up — so they carry `allow(dead_code)` until it arrives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sea-snake
force-pushed
the
feat/session-create
branch
from
August 22, 2026 19:49
ead89cc to
5a34d25
Compare
# Conflicts: # src/internet_identity/src/storage/tests.rs
create_session clamps it rather than trusting the caller, so every path that makes a session gets the same range. The floor is ten minutes: an app delegation lasts five and an active application replaces it a little early, so a bound near that would end sessions plainly in use. The ceiling is the life this session was actually granted — a bound it could never reach would say something about the session that is not true. Asking for nothing still stores nothing, which is the same session anyone gets today.
…wise Absent used to mean unbounded, which made the feature opt-in and left the default sign-in lasting thirty days whether or not anybody came back. Seven days of nobody touching an application now ends it, under the same thirty-day cap. Raised then lowered rather than clamped in one call: clamp panics when its floor exceeds its ceiling, which a session granted less than ten minutes would do, and a trap is a poor answer to a short session. Such a session is bounded by its own life instead.
MRmarioruci
reviewed
Sep 1, 2026
The session unindexing and the reordered write path meet here. Both fallible steps — the counters and the session count — run before the first removal, so a refusal leaves the row, its sessions and its index entries as they were.
…ication A session's principal is derived through the application row, and applying the deltas is what removes it. Unindexing afterwards found nothing to derive the keys from and left every entry behind, which the eviction test caught.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The critical stale-origin eviction issue and unresolved moderate implementation and test issues must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds persistent, revocable account sessions with browser ownership, principal indexing, expiration, and session counts.
Changes:
- Implements session creation, replacement, pruning, revocation, and seed derivation.
- Persists session identifiers, handles, and browser/identity counters.
- Exposes browser session counts through the v2 API.
File summaries
| File | Summary |
|---|---|
src/internet_identity/src/verified_emails/remove.rs |
Updates anchor test fixtures. |
src/internet_identity/src/storage/tests.rs |
Adds session tests; moderate: per-browser removal test unintentionally replaces a session. |
src/internet_identity/src/storage/storable/session_record.rs |
Persists session IDs. |
src/internet_identity/src/storage/storable/session_id.rs |
Defines the stable session ID type. |
src/internet_identity/src/storage/storable/session_handle.rs |
Adds stable principal lookup handles. |
src/internet_identity/src/storage/storable/browser.rs |
Persists per-browser session counts. |
src/internet_identity/src/storage/storable/anchor.rs |
Persists identity session counts; nit: documentation incorrectly calls stored sessions “live.” |
src/internet_identity/src/storage/storable.rs |
Registers the new stable types. |
src/internet_identity/src/storage/anchor/tests.rs |
Updates anchor fixtures. |
src/internet_identity/src/storage/anchor.rs |
Tracks browser and identity session counts. |
src/internet_identity/src/storage/account/tests.rs |
Adapts tests to stored anchors. |
src/internet_identity/src/storage/account.rs |
Defines session records and expiration behavior. |
src/internet_identity/src/storage.rs |
Implements session lifecycle and indexing; critical: full-state writes prevent stale-origin eviction; moderate: common sign-ins scan all account lists and principal-index lifecycle coverage is missing. |
src/internet_identity/src/main.rs |
Returns browser session counts. |
src/internet_identity/src/email_recovery/remove.rs |
Updates anchor test fixtures. |
src/internet_identity/src/delegation.rs |
Derives session identity seeds; nit: PR description conflicts with the implemented seed inputs. |
src/internet_identity/src/account_management.rs |
Updates account tests for stored anchors. |
src/internet_identity/internet_identity.did |
Extends the Candid browser record. |
src/internet_identity_interface/src/internet_identity/types/api_v2.rs |
Extends BrowserInfo. |
src/internet_identity_interface/src/internet_identity/types.rs |
Defines SessionId. |
src/frontend/src/lib/generated/internet_identity_types.d.ts |
Regenerates frontend types. |
src/frontend/src/lib/generated/internet_identity_idl.js |
Regenerates frontend IDL bindings. |
Review details
Suppressed comments (2)
src/internet_identity/src/delegation.rs:150
- This seed binds
session_id, but the PR description still specifiescreated_atandbrowser_idas seed inputs. The linked design update supports the implementation, so the PR description should be corrected; otherwise reviewers and follow-up implementations have two incompatible session-identity contracts.
let mut blob: Vec<u8> = vec![];
push_field(&mut blob, salt);
push_field(&mut blob, SESSION_SEED_PREFIX.as_bytes());
push_field(&mut blob, account_seed);
push_field(&mut blob, &session_id.to_be_bytes());
src/internet_identity/src/storage/storable/anchor.rs:45
- This counter includes expired records until a later write prunes them, so calling it “live sessions” contradicts both the following explanation and the public/browser documentation that defines it as stored records. Describe it as stored sessions to avoid implementing the later cap against the wrong population.
/// Live sessions this anchor holds, as a trigger for the session cap rather than a
/// source of truth: expiry removes a session with no write to observe, so this can
/// over-count until a reclaim pass prunes and corrects it.
- Files reviewed: 20/22 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ # Conflicts: # src/internet_identity/src/storage.rs # src/internet_identity/src/storage/tests.rs
…here Carries the merge's resolution — `BrowserInfo` holds both the description and the session count — and this branch's own part of it: `CreateSessionParams` names a `browser_description` where it named a `browser_name`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
MRmarioruci
reviewed
Sep 9, 2026
MRmarioruci
reviewed
Sep 9, 2026
MRmarioruci
reviewed
Sep 9, 2026
MRmarioruci
reviewed
Sep 9, 2026
MRmarioruci
reviewed
Sep 9, 2026
MRmarioruci
reviewed
Sep 9, 2026
MRmarioruci
reviewed
Sep 9, 2026
MRmarioruci
reviewed
Sep 9, 2026
`create_session` handed the gate the whole of `account_state`, which made every origin the identity holds an origin this write was changing — and an origin a write is changing is never a candidate for its own eviction. So the one write that creates tracked defaults was the one write that could never evict them, and nothing else on a sign-in-only identity ever runs that pass. The sign-in now writes the origin it signs in at. The sessions of a browser the registry gave up are a consequence of the write rather than something the caller reaches across the identity to do, so the gate derives them from the registry the write carries and sweeps them in the same write. It adds only the origins that hold such a session, and only looks for them when a browser was actually given up — a registry below `MAX_BROWSERS` cannot have dropped one, so it answers without reading the stored anchor and an ordinary sign-in reads the one origin it names. Also from the same review: - The anchor's session counter counts stored records, not live ones, which is what the rest of its own comment says. - `a_browser_the_registry_gave_up_leaves_no_count_behind` signed in twice from one browser at one origin, so the second replaced the first and the browser held one session where the test says two. The second session moves to another origin, and the setup is asserted. Tests: `a_sign_in_evicts_the_stale_defaults_too`, which fails without the first change because eviction never runs on that path — 501 lists where the watermark plus the new one is 451; and `replacing_a_session_takes_its_principal_out_of_the_index`, for the one session-principal index transition nothing observed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
MRmarioruci
approved these changes
Sep 9, 2026
The session record's accessors are renamed; the tie-breaker stays the session id this branch introduced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
Main's retirement of the discrepancy counter arrives alongside the session index this branch adds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
# Conflicts: # src/internet_identity/src/main.rs # src/internet_identity/src/storage/anchor.rs # src/internet_identity/src/storage/storable.rs # src/internet_identity/src/storage/tests.rs
…id out loud `SessionRecordKey` is neither a record nor a stable-structure key — it is where one session lives, so `SessionLocator`, beside `StorableAccountLocator`. Its `account()` becomes `account_key()`, because the sibling on the session handle returns a principal from a method of the same name. `CreateSessionParams` takes the `VerifiedBrowserKeys` the verifier produces instead of two bare public keys: `create_session` registers a registry entry from them, and a caller that assembled them could have skipped the proof. `account_state` skipped a reference list whose application number resolves to nothing, silently. That state is illegal and skipping is still the right answer — refusing would block every sweep for the identity, including the origins that do resolve — but it is logged now. The storable's `session_id` becomes `id` (the type already says session), the `session_count` doc drops the consumer it named, the two `crate::delegation` imports become one, and the note above the id allocation stops claiming that nothing downstream of it can refuse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
MRmarioruci
approved these changes
Sep 9, 2026
`account_principal_of` never got the caller its note promised. The sign-in ceremony derives that principal itself, in `sessions.rs`, because storage's version is keyed by an application number while the ceremony has an origin — so the signature was the wrong shape for the one caller it was written for, and the logic was rewritten a layer up instead. Storage keeps `account_principals`, the batch form the index path actually uses. The test that reached for the singular one now reads the principal off the session handle `create_session` wrote, which is the value it was crossing against the index anyway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
sea-snake
added a commit
that referenced
this pull request
Sep 9, 2026
Design: #4224, on the storage of #4222. Overview: #4230. The schema only — #4266 is what writes a session. An app delegation is unrevocable for as long as it is valid, which is up to 30 days: the client holds a self-contained canister-signed artifact whose verification never consults the canister again. The fix needs somewhere to put a long-lived, revocable record, and #4222 gives a capped, evictable per-account store to hang it on. A session is `(created_at, valid_till, last_refreshed, browser_id, read_only)` on the account reference. Nothing else. Putting it there rather than in a map of its own means it inherits the per-anchor caps that already bound references, so revoking, expiring and evicting reuse machinery that exists. - The field is `Option`, so references written under the previous schema decode unchanged, and an empty list is not stored. - Only `last_refreshed` is mutable, which is why it is the one field that will not feed the session's seed: a mutable input would change the session's principal every time it was stamped. - **A list carries no eviction exemption for holding a session.** The list is what makes an app visible in settings, so sparing it would leave the user access they cannot see, and a session nobody can find is a session nobody can revoke. What an eviction costs is a ceremony, not an account: the account is computed, so it returns at the identical principal on the next sign-in. - `reclaim_order` ranks dead sessions first, then live ones on `last_used + (last_used - created_at)` — recency extended by how long the session stayed in service, so an app in weekly use outranks one opened once yesterday. #4267 is what enforces a cap with it. `AccountReference::new` replaces the struct literals, so adding the field did not spread across every construction site. Tests: `session_tests` (11), including a reference written before sessions existed decoding with none, a list holding a session being evictable like any other, and a flood of unused sessions failing to displace a used one. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
sea-snake
added a commit
that referenced
this pull request
Sep 9, 2026
Design: #4224. Overview: #4230. #4266 stores sessions; this bounds them. Sessions are per (identity, application, account, browser), so without a bound an identity's stored set grows with every app it ever signs in to. **Five hundred stored records per identity, expired ones included.** Counting what is stored rather than what is live is what makes the cap enforceable: a session expires with no write anywhere, so no counter can follow the live set — something would have to decrement at the moment of expiry, and nothing runs then. An expired record holds its slot until something reclaims it, and because it is the first thing reclaimed, a held slot is never taken from a session in use. **Reaching the cap reclaims rather than refusing.** The user is trying to sign in, and the only thing that could refuse them is internal bookkeeping. Reclaiming walks the identity's lists and takes dead sessions first, then live ones by `last_used + (last_used - created_at)`: how recently the session was used, extended by how long it stayed in service. The extension is the point, because recency alone gets the common case backwards — an app opened once and abandoned yesterday was touched more recently than an app in weekly use last opened three days ago. It needs no constant of its own, since the span it adds is bounded by the session's own 30-day lifetime. **What the cap does not do.** It puts no bound on a flood of sign-ins: standing rises with use, so a party that can provoke sign-ins and keep refreshing them can outrank sessions an identity has left idle. What stands in the way is the ceremony — creating a session needs an access method, so a flood costs one authenticated sign-in per session. A stronger bound is deliberately not decided here. **Enforced against a recount, not a counter.** A `session_count` on the anchor decides whether to run at all, so a sign-in below the cap reads nothing extra. At the cap, the pass counts what the lists actually hold and admits the sign-in against that number. Every path that removes a session decrements the counter, but it is one number maintained by seven call sites, and the recount is what makes a missed decrement cost an extra pass rather than a slot past the cap. It reads every list rather than a bounded prefix, because a truncated scan would undercount and the undercount would become the counter. Tests: `session_creation_tests` (5), including 620 consecutive sign-ins never exceeding the cap, a drifted counter being corrected rather than denying a sign-in, and reclaiming across two lists taking only the sessions it selected and never their namesakes in another list. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
sea-snake
added a commit
that referenced
this pull request
Sep 10, 2026
Design: #4224. Overview: #4230. This is the caller #4266 and #4264 were waiting for, so it removes their `#[allow(dead_code)]` annotations. **`prepare_account_session` / `get_account_session`.** The first creates a session and signs its delegation to the II frontend's key; the second witnesses it. A separate pair from `prepare_account_delegation`, not an option on it: both mint, but one proves a live session and identifies the account by its principal while the other proves an access method and names the anchor outright. Merging them would mean one method with two authorizers and two argument shapes, and would drag the frontend's internal surface into the public API. **Everything that can refuse does so before anything is written**, cheapest first. The account check leads — an account the identity does not hold is the one failure a caller can provoke, and returning it after the writes would leave a browser registered for a sign-in that never happened — so a request that was never going to succeed does not pay for two P-256 verifications on the way to being told so. Nothing is revealed by that order: `check_authz_and_record_activity` above is the auth guard. The browser proof follows, using the verifier from #4264, and `create_session` takes the `VerifiedBrowserKeys` it produces rather than two byte strings. **The session credential is scoped to Internet Identity.** It exists to mint app delegations, which is an update call on this canister and nothing else, so it is signed with `targets = [id()]` and can be presented nowhere else. Leaving that to the II frontend would have left it to the party holding the session key, who can decline to add it. `permissions` stays absent for the same reason it is set on app delegations: minting is an update call, so a read-only session that could not make one could not sign in to an app at all — read-only travels on the app delegation instead. **`get_account_session` tells three failures apart.** No stored session for that id is `NoSuchSession`; a session that is present with no signature for the asked-for key and expiration is `NoSuchDelegation`, because the remedy is to ask with the parameters that were signed rather than to sign in again; a seed that will not derive is the salt being unset, which is `InternalCanisterError`. An over-long origin is that too, rather than a rejected message the caller cannot read as a response. **The request carries what the browser is, not a label for it.** `browser_description` holds the tokens the registry stores (#4242), and each token a client writes for itself — an unrecognised brand, an unrecognised system, and the hardware model — is bounded at 64 bytes. The named variants carry no text, so a description of nothing but those is within the limit whatever it says. Refused rather than truncated: a cut-off token would put a value in the record that no parser ever produced. **Every later failure traps rather than returning.** On the IC, returning an error commits state and only a trap rolls the message back, so once the browser registration is written a failure has to trap or a caller could be told "no" and still have a browser enrolled. **`valid_for`** is the lifetime the user chose at consent, clamped by the canister to between 10 minutes and 30 days. Every ceremony creates, so it always applies: the replacement's expiry is measured from the ceremony that made it, and no session is renewed in place. Tests: `integration/sessions.rs` (18) drives the real ceremony — creating and verifying a session, a request for another identity refused, the registry cap dropping the least recently used and ending its sessions, the key proof's rejections at the endpoint, rotation keeping the entry, a retired key returning as a new browser, and two browsers each keeping the description it registered with. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
sea-snake
added a commit
that referenced
this pull request
Sep 10, 2026
Design: #4224. Overview: #4230. Depends on #4268 for the ceremony that creates a session. This is the PR the feature turns on: after it, a delegation an app holds lasts five minutes and the session behind it can be revoked. **`app_prepare_delegation` / `app_get_delegation`.** An app holds a session chain and asks for a delegation with it. What it gets is capped at five minutes and the cap is not requestable, so revoking the session ends access within one delegation lifetime. **Finding the session from the call.** The index mapping a session's rooted principal to `{account_principal, session_id}` is written by #4266; this adds the two accessors that read it and the caller that needs them. The call names nothing and attaches nothing: `caller()` is looked up, the account principal resolves through the principal index from #4238, and the record is read from the list. A hit is itself the proof that the caller is that session, since only the holder of its key can arrive as that principal. The account is named by principal rather than by locator on purpose: materialising a default account changes its locator and leaves its principal alone, so naming an account touches one index entry instead of every session on it. **The entry names the session by its id.** A browser keeps its id across sign-ins, so an index entry keyed on the browser and outliving its session would authenticate its holder as whatever that browser created next — a revoked chain coming back to life. `session_id` is allocated per session and never reissued, so an entry can only ever resolve to the one session it was written for. Every path that destroys a session drops its index entry in the same write; the id is what makes a stale entry fail closed regardless. **The `get` re-derives the five-minute ceiling** rather than trusting the `expiration` it is handed, because longer-lived delegations exist over the same account seed. An account's own principal is absent from the session index, so an app delegation cannot mint its own replacement. Tests: `integration/sessions.rs` (11), including a refresh refused once the session has expired, a session replaced by a new ceremony invalidating the previous chain, and the account principal minting nothing itself. Index accessor coverage lands in `account_principal_index_tests` and `session_creation_tests`. --------- 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. The writers have no caller until the ceremony in #4268, which removes the
#[allow(dead_code)]annotations this adds — each at the PR that first calls the function it sits on. The per-identity cap arrives in #4267.A session record on the account reference: created at, expires at, last refreshed, the browser that made it, and whether the user consented to queries only.
Its identity.
session_seed = H(salt, "session", account_seed, session_id), every field length-prefixed. The id rather thancreated_at, which is not unique within a round. Building on the account's own seed rather than on the identity, application and account numbers is what makes a session survive anything that leaves the account's principal unchanged — naming a default account is exactly that, since it gains a number and a name while still deriving from the identity it was conjured from. Had the numbers been inputs, naming an account would have signed the user out of every app using it.Only
last_refreshedis mutable, which is why the seed does not take it: a mutable input would change the session's principal every time it was stamped.Replacement, not reuse. A ceremony from a browser that already holds a session at this account deletes it and mints a new one, so a copy of the old chain stops working at the user's next sign-in rather than at its expiry. Expired records on the list go in the same write, which is what keeps the design free of any sweep.
Read hardest: the seed derivation, since it decides what survives an account being renamed.
Tests:
session_creation_tests(23), including a second browser getting its own session, the same browser's being replaced, expired records pruned on write, an account the identity does not hold refused, and the seed being unchanged by naming a default account.🤖 Generated with Claude Code
https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ