feat(be): cap sessions per identity - #4267
Conversation
|
✅ No security or compliance issues detected. Reviewed everything up to 246e75e. Security Overview
Detected Code Changes
|
cb6d44c to
5569415
Compare
5569415 to
63b4539
Compare
63b4539 to
10823ce
Compare
Five hundred stored records per identity, expired ones included, because nothing observes a session dying: a session expires with no write anywhere, so no counter can follow the live set. An expired record holds its slot until something reclaims it, and since it is the first thing reclaimed, a held slot is never taken from a session in use. Reaching the cap does not fail a sign-in. Reclaiming walks the identity's rows and takes dead sessions first, then the live ones by `last_used + (last_used − created_at)` — how recently used, extended by how long the session stayed in service. The extension is what separates an app in weekly use from one opened once and abandoned yesterday, which recency alone gets backwards, since the abandoned one was touched more recently. Note what the cap does not do: it puts no bound on a flood of sign-ins, because standing rises with use. What stands in the way is the ceremony — creating a session needs an access method — and a stronger bound is left for whoever finds this one too weak. Reclaiming runs before the new record is admitted, and admission is granted against what that pass counted rather than against the anchor's counter, so the stored set never sits above the cap. It reads every row rather than a prefix, because a truncated scan would undercount and the undercount would become the counter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
10823ce to
a60eb4a
Compare
# Conflicts: # src/internet_identity/src/storage/tests.rs
# Conflicts: # src/internet_identity/src/storage/tests.rs
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 # Conflicts: # src/internet_identity/src/storage.rs
There was a problem hiding this comment.
🟡 Changes recommended
The new session-count update logic can silently clamp on counter underflow (writing an incorrect session_count), and one newly added test is currently duplicated/misaligned with its stated intent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Implements a per-identity cap on stored session records (including expired records) to prevent unbounded growth of session data across many apps/browsers. The cap is enforced by reclaiming (expired first, then least-demonstrated-use) down to a watermark rather than refusing new sign-ins, and by validating against a recount when the stored counter indicates the cap is reached.
Changes:
- Add
MAX_SESSIONS_PER_ANCHOR(500) andSESSIONS_WATERMARK_PER_ANCHOR(450), plus a reclaim pass that prunes expired and then reclaims to the watermark. - Update write validation to compute session deltas from list diffs and only recount stored sessions when needed at/over the cap, updating
anchor.session_countaccordingly. - Expand storage tests to cover cap behavior under many sign-ins, counter drift correction, and cross-list reclaim semantics (also widening test “generation” to
u16for longer sequences).
File summaries
| File | Description |
|---|---|
| src/internet_identity/src/storage.rs | Adds session cap constants, reclaim logic, and session-count validation/update during account-state writes. |
| src/internet_identity/src/storage/tests.rs | Updates session test helpers (u16 generation) and adds/adjusts tests for session-cap enforcement and reclaim behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 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
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
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
The session cap moves in with the counters and the tracked-default eviction it sits beside. `create_session` no longer reaches across the identity to make room for itself: it writes the origin it signs in at, and the gate — which already reads the stored count to enforce the cap — reclaims when a write would take the identity past it. Reclaim is gated on a write that grows the stored count, which is what makes this safe on the hot paths. A delegation refresh and a revocation cannot grow it, so neither reaches the scan; nor does a sign-in that only replaces the session this browser already held at this origin, which is the case Mario raised on this PR: at the cap it used to reclaim fifty records to make room it did not need. Victims are selected from what is stored, so the session a write is creating is never given up by the pass that made room for it. The selection returns ids and one sweep applies them, the same sweep that takes the sessions of a browser the registry gave up — both rules mean "these sessions go, wherever they are". `now` reaches the gate from the endpoint, the way the rest of storage takes its time: read once per request in `main.rs`, passed down. `TEST_NOW` names the clock for the writes whose timing is not what a test is about. The counter is computed in `i64` and recounted whichever end it fails to fit, rather than saturating at `u32`. An over-count already recounted; an under-count clamped to zero and stayed there, and a counter under the truth is the one that lets the lists past the cap. Neither state is reachable today — every session write derives the count from the lists it changed — so this is about what the code says rather than a bug it fixes. `sync_session_index` no longer returns a delta nobody reads. Tests: `the_session_cap_reclaims_to_the_watermark` now reaches the cap and asserts it clears to the watermark, which is what its name always claimed and what the test it duplicated never checked; `an_under_counting_anchor_is_corrected_rather_than_clamped` covers the other direction of the drift. `reclaiming_spans_every_list_and_takes_the_expired_ones_first` signs in at a third origin, because a sign-in at either list under test prunes that list's expired record on the way past and leaves the identity holding what it started with. 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
# Conflicts: # src/internet_identity/src/storage/tests.rs
# Conflicts: # src/internet_identity/src/storage/tests.rs
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>
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 than `created_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_refreshed` is 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.com/claude-code) https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Design: #4224. Overview: #4230. "This browser used this app 3 minutes ago" against "5 weeks ago" is what makes a session list worth reading, and what lets someone spot a session they do not recognise **still being used** rather than merely still existing. **Every refresh stamps**, and three consumers make coarsening it unattractive: the session cap orders live sessions on this field, so inside a coarsening interval every session would look equally idle; the browser registry cap orders on the same signal, where an hour is long enough to drop a browser in use; and a user-facing list that can be an hour stale does not answer the question it exists to answer. **What the stamps cost, stated separately, because they are not the same write.** `last_refreshed_ns` and the reference's `last_used` ride free: the list blob is rewritten either way, so they change bytes already in flight. The browser registry's `last_used` does not — `stamp_browser_use` is a `read` plus a `write` of the whole anchor, which serialises every passkey, OpenID credential, recovery key and verified email, pre-checks the email-recovery binding against its reverse index, and re-syncs five reverse indices by diffing vectors, all to move one `u64`. So this PR adds **one anchor read-modify-write per five minutes of app use, per live session**, where before there was one per sign-in. That is accepted here rather than argued away: the registry cap orders on this field, and a browser in use dropping out of the list is worse than the write. The coarsening argument above is about `last_refreshed_ns` and the user-facing list, and does not apply to the device field. The same write carries three stamps: | Field | Lives on | Drives | | ----- | -------- | ------ | | `last_used` | the account reference | account eviction (#4235) | | `last_refreshed` | the session record | the session cap (#4267) and the user-facing list | | `last_used` | the device record | the registry cap (#4242) and the settings list | Stamping the device is the only reason refresh touches the anchor: it authenticates by session chain and never runs `check_authorization`. What that write buys is a use signal a sign-in stamp cannot give the browser list. Stable writes scale with `1/T` alongside the calls, so lowering the app-delegation TTL multiplies both. Tests: `session_refresh_stamp_tests` (8), including a stamp for a session that is gone being refused rather than reporting success, and a refresh advancing the device's `last_used` while leaving its enrolment timestamp alone. PocketIC drives two refreshes a minute apart and reads the device back off `identity_info`. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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_counton 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.