YNU-918: treat past expires_at as session-key revocation#775
Conversation
…vocation Submit handlers for channels.v1.submit_session_key_state and app_sessions.v1.submit_session_key_state previously rejected any state whose expires_at was not strictly in the future. There was no first-class way for a user to deactivate a session key before its natural expiry. Accept past expires_at as a revocation: the same monotonic version sequence is preserved, and the auth path (GetAppSessionKeyOwner / ValidateChannelSessionKeyForAsset) already filters expires_at > now, so a past timestamp deactivates the key immediately. A later submit with the next version and a future expires_at re-activates the same session key address. Reject expires_at < 0 instead of expires_at <= now. Defense-in-depth: the metadata-hash packer casts int64 -> uint64, which would wrap a negative unix timestamp to a huge future value and silently desynchronize the user-signed payload from the value persisted in the database (the DB filter is still the source of truth, but the divergence is undesirable). Update CountSessionKeysForUser to JOIN both history tables and only count rows where expires_at > now, so a revoke frees the per-user cap slot. A single now is bound for both kind branches for internal consistency. Emit an Info log "session key revoked" / "channel session key revoked" when the submission deactivates the key, distinct from the existing "successfully stored" log. docs/api.yaml, pkg/rpc/types.go, and the Go SDK Submit* doc comments are refreshed to describe revoke and re-activation semantics.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR treats past ChangesSession Key Revocation Semantics
Sequence Diagram 🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested labels
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 |
nksazonov
left a comment
nksazonov
left a comment
There was a problem hiding this comment.
Good job — the revocation approach is clean and the DB-level cap accounting is well-tested.
Revocation requires the compromised key's private key. Both handlers require session_key_sig unconditionally, even on the revocation path (app_session_v1/submit_session_key_state.go:95, channel_v1/submit_session_key_state.go:78). This means a user cannot revoke a lost or compromised session key using their wallet signature alone — the compromised key must co-sign the revocation. Worth documenting explicitly; if wallet-only revocation is a safety requirement, a separate code path (accepting user_sig alone when expires_at <= now) would be needed.
No handler-level test for re-activation. The PR description documents "a later submit with version+1 and a future expires_at re-activates the same session key address," but no handler test exercises this round-trip (active → revoke → re-activate). The DB test covers slot-freeing only. Silent regressions in the re-activation path wouldn't be caught.
…ests - CountSessionKeysForUser now bounds-checks the int64 sum before casting to uint32 and returns an explicit error if the total ever exceeds math.MaxUint32. Defense against silent wrap that would let the cap comparison pass incorrectly under the soft-cap concurrency race noted in TODO(MF-H01-followup). - Add TestSubmitSessionKeyState_RevokeExistingActiveKey and TestSubmitSessionKeyState_ReactivateAfterRevoke for both the app-session and channel handlers. These cover the typical revocation path (latestVersion > 0, past expires_at) and the re-activation round-trip (latestVersion > 0, future expires_at) — the prior tests only exercised the new-key-with-past-expiry path. Both new tests assert CountSessionKeysForUser is not called, locking in the short-circuit on the latestVersion > 0 branch. - Document explicitly in docs/api.yaml and the Go SDK Submit* doc comments that session_key_sig is required on every submit, including the revoke path. Wallet-only revocation for a lost or compromised key is not supported by this method and is tracked as a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks for the review @nksazonov, addressed:
Also tracking the I4 SDK |
|
I would keep this open for one closure issue.
One way to close this is to run the cap check whenever the submitted state is active and the previous latest state was inactive or absent, not only when Minor/non-blocking cleanup: the TS example app still revokes by clearing |
Extend LockSessionKeyState to also return the latest history expires_at so submit handlers can tell rotation from reactivation. The cap check now fires whenever the submit moves the slot from inactive to active (new key or revoked → active), closing the revoke→register-new→reactivate bypass that the prior `latestVersion == 0` gate allowed. Docs and example app updated to describe submit-as-revoke (set past expires_at, keep assets) under the new semantics.
|
Yep, you're right about the bypass — fixed in f6e7f34. Extended So new-key registration, and reactivation from a revoked state, both go through the cap; rotation against a still-active key and revoke submits skip it as before. New tests on both the app-session and channel handler — On the cleanup: also fixed in the same commit.
|
ihsraham
left a comment
ihsraham
left a comment
There was a problem hiding this comment.
Approving. The closure blocker is fixed: reactivation now re-checks the active-key cap, and the new tests cover both below-cap and at-cap reactivation for app-session and channel keys.
Non-blocking cleanup: sdk/ts/examples/example-app/src/components/WalletDashboard.tsx still has one Auto Sign disable path that revokes by clearing assets while keeping the old future expires_at. The table/list revoke path and docs were updated correctly, so I would not block closure on this, but it is worth fixing so the example app does not report a successful on-chain revoke while leaving the latest key state active.
handleDisableAutoSign still cleared assets while keeping the original future expires_at, so under the submit-as-revoke semantics it reported a successful on-chain revoke while leaving the latest state active. Mirror the table/list revoke fix: keep assets, set expires_at to now-1s.
|
Yep, good catch — missed the |
3 participants
Summary
channels.v1.submit_session_key_stateandapp_sessions.v1.submit_session_key_stateaccept a pastexpires_atas a revocation, preserving the monotonic version sequence. The auth path already filtersexpires_at > now, so the key deactivates immediately; a later submit with the next version and a futureexpires_atre-activates the same session key address.expires_at < 0instead ofexpires_at <= now. Defense-in-depth againstint64 → uint64wrap in the metadata-hash packer, which would otherwise silently desynchronize the user-signed payload from the persisted row.CountSessionKeysForUserJOINs both history tables and only counts rows withexpires_at > now, so a revoke frees the per-user cap slot. A singlenowis bound for both kind branches. Theint64sum is bounds-checked before theuint32cast.session key revoked/channel session key revoked) when a submission deactivates the key, distinct from the existingsuccessfully storedlog.docs/api.yaml,pkg/rpc/types.go, and the Go SDKSubmit*doc comments to describe revoke and re-activation semantics, including the explicit constraint thatsession_key_sigis required on every submit (including revocation).Follow-ups (out of scope for this PR)
session_key_sigon every submit, so a user cannot revoke a key whose private material they no longer control. A wallet-only flow is a separate code path (asymmetric auth, replay-across-wallets threat model, version monotonicity when the key is unrecoverable) and warrants its own ticket.Revoke*SessionKeyStatehelpers for the Go SDK and the TypeScript SDK, hiding the past-timestamp construction, version bump, and dual signing behind a first-class API. Also touches the TS SDK public-API drift snapshots, so worth a dedicated PR.Test plan
go build ./...go vet ./...go test ./nitronode/api/app_session_v1/... ./nitronode/api/channel_v1/... ./nitronode/store/database/...🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
expires_attimestampDocumentation
Behavior Changes