feat(be): record that a session is still in use - #4244
Conversation
|
✅ No security or compliance issues detected. Reviewed everything up to 5363e32. Security Overview
Detected Code Changes
|
7d6a294 to
0b043da
Compare
0b043da to
72c91a1
Compare
72c91a1 to
d8bd528
Compare
d8bd528 to
474d22b
Compare
474d22b to
77d5897
Compare
77d5897 to
78a570f
Compare
78a570f to
b7b407e
Compare
b7b407e to
2676560
Compare
2676560 to
41a2e22
Compare
41a2e22 to
752fe5a
Compare
752fe5a to
0502a3f
Compare
0502a3f to
5c573c2
Compare
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
…resh-stamps # Conflicts: # src/internet_identity/src/sessions.rs # src/internet_identity/src/storage/anchor.rs
`record_session_use` returned `Ok(false)` for three different kinds of "what you named is not there", and `?` at the call site caught only the `Err` — so an app delegation was minted for a session no list holds, which is the one outcome the design exists to prevent. It returns `Result<(), StorageError>` now, with `SessionNotFound`, and the case stops being ignorable because `?` handles it. Nobody made a decision from the bool, so it should not have been one. The refusals also move above the stamp. Returning `Err` on the IC commits whatever was written before it — only a trap rolls back — so a seed that will not derive left the session recorded as used and the caller told the call failed. `stamp_browser_use`'s doc still claimed a return value it does not have, and a saving no caller could make from a value it never receives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
The stamp in `app_prepare_delegation` flattened every `StorageError` into `InternalCanisterError`, including the not-found one it gained when `record_session_use` stopped returning a bool. That case is a session revoked between `authorize_session` and the stamp — a race, and the same answer the caller would have got a moment earlier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate test-coverage gaps remain unresolved.
Review details
Suppressed comments (2)
src/internet_identity/src/sessions.rs:383
- The new update-path side effect is only covered by direct
Storageunit tests. The existing PocketIC session tests exerciseapp_prepare_delegationand browser listing separately, but no end-to-end test refreshes a session and then verifies the updatedBrowserInfo.last_usedthroughidentity_info; this is the boundary where wiring the stamp or persisting the modified anchor could regress. Please add the described PocketIC regression test.
storage_borrow_mut(|storage| storage.record_session_use(&locator, now)).map_err(
|err| match err {
StorageError::SessionNotFound { .. } => AppSessionError::NoSuchSession,
other => AppSessionError::InternalCanisterError(other.to_string()),
},
)?;
src/internet_identity/src/storage/tests.rs:6585
- This test does not exercise the unregistered-browser case named here:
storage_with_session()callscreate_session, which registers a browser throughresolve_browser, just like the registered-browser helper. As written, it is only another registered-browser refresh test and would not catch a regression in the intended no-registry-entry behavior. Remove the browser entry (or construct a session without one) before callingrecord_session_use.
fn a_refresh_for_a_device_the_anchor_never_registered_still_stamps_the_session() {
let (mut storage, anchor_number, key) = storage_with_session();
storage.record_session_use(&key, 9_000).unwrap();
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
`a_refresh_for_a_device_the_anchor_never_registered_still_stamps_the_session` built its session with `storage_with_session`, which calls `create_session` and so registers a browser — leaving it a duplicate of the registered-browser refresh above it, under a name claiming the opposite. The state it named is unreachable. A browser is given up only at `MAX_BROWSERS` in `resolve_browser`; the write gate sweeps that browser's sessions in the same write, and `sync_session_index` takes their index entries with them. The session is gone too, so a mint stops at `NoSuchSession` in `find_caller_session` long before the stamp is reached. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVi99RYo2jyi2kCurgovNJ
|
Both suppressed findings from the Copilot review, answered — they land differently.
Constructing the orphan, as suggested, would pin a state that cannot occur. A browser is given up only at That also settles 🤖 Generated with Claude Code |
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_nsand the reference'slast_usedride free: the list blob is rewritten either way, so they change bytes already in flight. The browser registry'slast_useddoes not —stamp_browser_useis areadplus awriteof 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 oneu64. 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 aboutlast_refreshed_nsand the user-facing list, and does not apply to the device field.The same write carries three stamps:
last_usedlast_refreshedlast_usedStamping 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/Talongside 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'slast_usedwhile leaving its enrolment timestamp alone. PocketIC drives two refreshes a minute apart and reads the device back offidentity_info.