MF3-M05: allow wallet-only revocation of session keys#809
Conversation
MF3-M05: SubmitSessionKeyState required session_key_sig on every submit, including revocation (expires_at <= now). This let a lost, unavailable, or malicious delegate veto revocation of its own delegation — the user had to wait for the prior expires_at to pass while the key stayed usable. Revocation now validates user_sig alone; activation, extension, and rotation (expires_at > now) still require both user_sig and session_key_sig. A session_key_sig present on the revocation path is ignored. - pkg/app, pkg/core: add ValidateAppSessionKeyStateUserSigV1 / ValidateChannelSessionKeyStateUserSigV1; refactor the both-signature validators to reuse them. - nitronode handlers: branch on expires_at to pick the validator; capture now once and reuse it for the in-tx cap/version logic. - migration: drop the session_key_sig non-empty CHECK constraints added in 20260508000000. Active rows stay co-signed via app code, and owner/auth lookups filter expires_at > now, so revocation rows are never trusted. - sdk/go, sdk/ts: add wallet-only RevokeChannelSessionKey / RevokeAppSessionKey (revokeChannelSessionKey / revokeSessionKey) helpers. - docs/api.yaml: document session_key_sig as optional for revocation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements wallet-only session-key revocation. When ChangesSession-key wallet-only revocation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/api.yaml (1)
389-397:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMark
session_key_sigoptional in the schema too.The prose now allows wallet-only revocation, but both type definitions still model
session_key_sigas required. Anything generated or validated fromdocs/api.yamlwill keep rejecting valid revocation requests that omit the field.Suggested fix
- name: session_key_sig type: string + optional: true description: > Session-key holder's EIP-191 signature over the same payload as user_sig — abi.encode(SESSION_KEY_AUTH_TYPEHASH, session_key, metadata_hash) — proving possession of the key being registered. Required when expires_at is in the future (registration, update, re-activation) to prevent registration of keys the submitter does not control. Optional for revocation (expires_at at or before now): a wallet may revoke unilaterally with user_sig alone so a lost or uncooperative session key cannot block revocation.- name: session_key_sig type: string + optional: true description: Session-key holder's signature over the same packed state (which already binds user_address) proving possession of the key being registered. Required when expires_at is in the future (registration, update, re-activation) to prevent registration of keys the submitter does not control. Optional for revocation (expires_at at or before now); a wallet may revoke unilaterally with user_sig alone so a lost or uncooperative session key cannot block revocation.As per coding guidelines, "docs/api.yaml: API definition and canonical list of all v1 RPC methods, types, and request/response schemas is located in
docs/api.yaml."Also applies to: 427-429
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api.yaml` around lines 389 - 397, The schema currently marks session_key_sig as required; update the docs/api.yaml type/request schemas that include the session_key_sig field so it is optional (remove it from any "required" lists or change its required flag), since revocation requests where expires_at is at-or-before now must be allowed without session_key_sig; apply the same change to the other occurrence(s) of session_key_sig in the file (the second block referenced around the later occurrence) and ensure the field description still documents that session_key_sig is required for future-dated registrations/updates/reactivations but optional for revocation when expires_at <= now.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api.yaml`:
- Around line 674-676: The field description for the ChannelSessionKey schema
(`field_name: state`, `type: channel_session_key_state`) incorrectly says "past
value" whereas the server allows expires_at == now; update the description text
to say "at or before now" (e.g., "Set `expires_at` to at or before now to revoke
(user_sig only); future to (re-)activate (both signatures).") and make the same
change for the other identical occurrence of this field description elsewhere in
the file so both places match the server contract.
In `@nitronode/api/app_session_v1/submit_session_key_state.go`:
- Around line 114-120: The revocation branch currently validates only the user
signature but then persists the unchanged coreState, allowing an unvalidated
session_key_sig to be stored; update the branch in submit_session_key_state.go
(where ValidateAppSessionKeyStateUserSigV1(coreState) is called) to clear the
SessionKeySig field on coreState (set to zero value) before returning/persisting
so revocation rows never retain client-provided session_key_sig and stored state
matches the public contract.
In `@nitronode/api/channel_v1/submit_session_key_state.go`:
- Around line 97-103: The revoke branch currently ignores but does not clear
coreState.SessionKeySig, allowing an unverified signature to be persisted;
update the revocation path in submit_session_key_state.go so that after calling
ValidateChannelSessionKeyStateUserSigV1(coreState) you explicitly zero/clear
coreState.SessionKeySig before any persistence, and add a regression assertion
in the revoke unit test to confirm the stored/returned state has an empty
SessionKeySig; reference the coreState struct, its SessionKeySig field, the
ValidateChannelSessionKeyStateUserSigV1 check, and the code path that persists
coreState to locate where to clear the field and where to assert in tests.
In `@sdk/go/channel.go`:
- Around line 1101-1103: RevokeChannelSessionKey's time check uses nanosecond
precision (state.ExpiresAt.After(time.Now())) but signing uses Unix seconds, so
change the guard to compare Unix-second timestamps to match
SignChannelSessionKeyState / SignChannelSessionKeyOwnership: compare
state.ExpiresAt.Unix() against time.Now().Unix() (e.g., require
state.ExpiresAt.Unix() <= time.Now().Unix()) so values that serialize to the
current Unix second are not incorrectly rejected.
---
Outside diff comments:
In `@docs/api.yaml`:
- Around line 389-397: The schema currently marks session_key_sig as required;
update the docs/api.yaml type/request schemas that include the session_key_sig
field so it is optional (remove it from any "required" lists or change its
required flag), since revocation requests where expires_at is at-or-before now
must be allowed without session_key_sig; apply the same change to the other
occurrence(s) of session_key_sig in the file (the second block referenced around
the later occurrence) and ensure the field description still documents that
session_key_sig is required for future-dated registrations/updates/reactivations
but optional for revocation when expires_at <= now.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ce11d05-5b5b-49bb-a8ed-132ddd84b48a
⛔ Files ignored due to path filters (1)
sdk/ts/test/unit/__snapshots__/public-api-drift.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
docs/api.yamlnitronode/api/app_session_v1/submit_session_key_state.gonitronode/api/app_session_v1/submit_session_key_state_test.gonitronode/api/channel_v1/submit_session_key_state.gonitronode/api/channel_v1/submit_session_key_state_test.gonitronode/config/migrations/postgres/20260602000000_session_key_user_only_revocation.sqlpkg/app/session_key_v1.gopkg/app/session_key_v1_test.gopkg/core/session_key.gopkg/core/session_key_test.gosdk/go/app_session.gosdk/go/channel.gosdk/ts/src/client.ts
There was a problem hiding this comment.
Good direction overall: future-dated registration and re-activation still require both signatures, and the new user-only revoke path addresses the lost or uncooperative session-key case.
I would keep this open for now. The remaining blocker is that an expired first submit can reserve an unowned session_key before any delegate signature is required. I left inline notes on both handler paths.
Also worth picking up the existing notes on clearing SessionKeySig before storing revocation rows, marking session_key_sig optional in the API schema, and aligning the Go helper time check to Unix seconds.
| c.Fail(rpc.Errorf("invalid_session_key_state: %v", err), "") | ||
| return | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Good direction on splitting active vs revoke validation, but I would block on one ownership edge case here. A wallet can submit version=1 with expires_at <= now for any arbitrary session_key; this branch only checks user_sig, then LockSessionKeyState creates the permanent (session_key, kind) owner row and the later latestVersion + 1 check accepts it.
Could we only allow the user-only path after the lock proves latestVersion > 0 for this wallet? For first submit, reject the expired state or require both signatures before the pointer row can be created.
There was a problem hiding this comment.
Yeah, good catch — this was the real hole. Fixed in bf08526.
I went with rejecting a revoke at version == 1 before LockSessionKeyState runs, rather than gating on latestVersion > 0 after the lock. The reason: the seed row is written inside the same tx, so a version >= 2 revoke on an unowned key would seed, then fail the latestVersion + 1 check and roll the seed back with the tx. The only revoke that can actually commit a claim for a key with no prior delegation is version == 1 (passes 0 + 1), so blocking that is enough — and it never even seeds. A legitimate revoke is always version >= 2 since registration at v1 is future-dated and requires both sigs.
| c.Fail(rpc.Errorf("invalid_session_key_state: %v", err), "") | ||
| return | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Same issue applies here for app session keys. A first submit with expires_at <= now can reserve an unowned session_key under the caller because the user-only branch runs before the lock proves an existing delegation.
Please gate wallet-only revocation on latestVersion > 0. First submits should either be future-dated with both signatures or rejected.
There was a problem hiding this comment.
Same fix here — bf08526. Reject version == 1 revoke before the lock, same reasoning as the channel handler (the seed only persists if the tx commits, and only a v1 revoke commits without a prior delegation).
| * | ||
| * @param state - The channel session key state to revoke (version = latest + 1, expires_at <= now) | ||
| */ | ||
| async revokeChannelSessionKey(state: ChannelSessionKeyStateV1): Promise<void> { |
There was a problem hiding this comment.
This helper signs and clears the signature fields itself, but the input type still requires callers to provide user_sig and session_key_sig. That means TS callers need dummy values for the wallet-only revoke path.
Could we use a dedicated revoke input type, or Omit<ChannelSessionKeyStateV1, 'user_sig' | 'session_key_sig'>, and apply the same shape to revokeSessionKey?
There was a problem hiding this comment.
Yep, makes sense — switched both revokeChannelSessionKey and revokeSessionKey to Omit<…, 'user_sig' | 'session_key_sig'> so callers don't pass dummy sigs. bf08526
| // } | ||
| // err := client.RevokeAppSessionKey(ctx, state) | ||
| func (c *Client) RevokeAppSessionKey(ctx context.Context, state app.AppSessionKeyStateV1) error { | ||
| if state.ExpiresAt.After(time.Now()) { |
There was a problem hiding this comment.
Same precision issue applies to the app helper. This guard uses full time.Time precision, but PackAppSessionKeyStateV1 signs ExpiresAt.Unix(), so a value inside the current Unix second can be rejected locally even though the server treats it as expires_at <= now.
Please compare Unix seconds here too, matching the channel helper fix.
There was a problem hiding this comment.
Fixed in bf08526 — comparing ExpiresAt.Unix() here too, matching the channel helper.
…on rows
Address PR review on wallet-only session-key revocation:
- Reject a revoke at version 1 in both submit handlers before LockSessionKeyState
runs. A version-1 revoke has no prior delegation, so it would otherwise let a
wallet seed a permanent (session_key, kind) ownership claim for a key it never
proved possession of. A legitimate revoke is always version >= 2.
- Clear SessionKeySig on the revocation path before persisting so stored revoke
rows never retain unverified client input.
- Compare Unix seconds in RevokeChannelSessionKey / RevokeAppSessionKey guards to
match the precision the metadata hash is signed at, so a value inside the
current Unix second is not rejected locally.
- Use Omit<..., 'user_sig' | 'session_key_sig'> for the TS revoke method inputs
so callers no longer pass dummy signature fields.
- Fix docs/api.yaml field descriptions ("past value" -> "at or before now") and
correct the stale seed-row permanence comment, which claimed the seed survives
aborted transactions.
Adds regression tests for the version-1 rejection and the cleared SessionKeySig.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ihsraham
left a comment
ihsraham
left a comment
There was a problem hiding this comment.
Approving for M-05. The latest commit closes the revoke-veto path and the first-submit ownership issue: v1 revokes are rejected before the pointer lock, v2+ first-submit revokes fail the version check in the transaction, and revoke rows are canonicalized by clearing SessionKeySig.
Only non-blocking follow-up I would consider is a DB-backed regression for the v2+ first-submit rollback case, since the current tests already cover the v1 pre-lock rejection.
|
After reading this PR, I have some thoughts:
What the PR does nowRevocation ( Industry analogues
The closest analogue is OAuth RFC 7009. A session key is a delegated access token that runs code — exactly what RFC 7009 covers. The core argument for EITHERA session key self-revoking is strictly a reduction of authority. It can never escalate privilege. This makes it unconditionally safe. Practical use cases the current design blocks:
Security consideration on session-key-initiated revocationCould an attacker who steals a session key cause harm by self-revoking it?
The one non-obvious risk is a racing revocation: an attacker steals the key and immediately self-revokes it before the owner notices. But this is a minor DoS at most — the owner can re-register at My personal recommendation: accept EITHER
|
3 participants
MF3-M05: wallet-only session-key revocation
Problem
SubmitSessionKeyState()(both app-session and channel paths) treated a submittedexpires_atnot afternowas a revocation, but still required and validatedsession_key_sigbefore storing. A user could not unilaterally revoke a delegated key: if the session key was lost, unavailable, compromised, or held by a delegate who refused to sign the revocation payload, the user had to wait until the existingexpires_atpassed while the key stayed usable. Requiringsession_key_sigis correct when activating or extending authority, but on revocation it handed the delegate a veto.Fix
Split the signature requirement by intent:
expires_at > now(activation / extension / rotation) — still requires bothuser_sigandsession_key_sig, validated as before.expires_at <= now(revocation) — requires onlyuser_sig. Anysession_key_sigpresent is ignored.The packed payload binds
user_address,session_key,version, andexpires_at, so a wallet-only revocation is authorized for exactly that key/version and cannot be replayed. Revocation still flows throughLockSessionKeyState(ownership binding) and the monotonic version check.Changes
pkg/app,pkg/core— newValidateAppSessionKeyStateUserSigV1/ValidateChannelSessionKeyStateUserSigV1; the both-signature validators now reuse them.expires_atto pick the validator.nowis captured once and reused for the in-transaction cap/version logic so the active/inactive decision is consistent across validation and storage.20260602000000— drops the*_session_key_sig_present_chkCHECK constraints added in20260508000000(they rejected an emptysession_key_sig). The ownership guarantee is preserved in application code: active rows are still co-signed, and owner/auth lookups filterexpires_at > now, so revocation rows with an empty signature are never trusted as a key's owner. Down re-adds the constraints asNOT VALID.RevokeChannelSessionKey/RevokeAppSessionKey(Go),revokeChannelSessionKey/revokeSessionKey(TS). Updated the docstrings that previously stated wallet-only revocation was unsupported.docs/api.yaml—session_key_sigdocumented as optional on the revocation path for both methods.Tests
session_key_sigis ignored on revoke; an invaliduser_sigis still rejected on revoke. Existing both-signature, cap, and reactivation tests unchanged and passing.Verification
go build ./...,go vet(affected packages), Go tests for pkg/app, pkg/core, both handlers, sdk/go — pass.Note for reviewers
Migration is timestamped
20260602000000. If it should slot before another unreleased migration, say so and I'll rename.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
RevokeChannelSessionKey,RevokeAppSessionKey(Go SDK) andrevokeChannelSessionKey,revokeSessionKey(TypeScript SDK) enable simplified revocation without requiring session-key holder signatures.Documentation