YNU-325: add session keys revocation mechanism#419
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a private RPC method Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK as SDK/Signer
participant RPCRouter as RPCRouter
participant DB as Database
participant Cache as SessionCache
Client->>SDK: createRevokeSessionKeyMessage(session_key)
SDK->>Client: signed RPC payload (revoke_session_key)
Client->>RPCRouter: revoke_session_key { session_key }
RPCRouter->>RPCRouter: validate params & resolve signer (verifySigner → signerAddress)
alt permission denied
RPCRouter-->>Client: RPC Error (permission denied)
else permitted
RPCRouter->>DB: BEGIN TX
RPCRouter->>DB: SELECT session row (verify owner & active)
DB-->>RPCRouter: session row
RPCRouter->>DB: UPDATE sessions SET expires_at = now()
DB-->>RPCRouter: OK
RPCRouter->>Cache: Evict(session_key)
Cache-->>RPCRouter: Evicted
RPCRouter->>DB: COMMIT
RPCRouter-->>Client: Success { session_key }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
Summary of ChangesHello @dimast-x, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances security by introducing a mechanism for users to revoke their active session keys. This new functionality provides immediate control over account access, allowing users to invalidate compromised or unwanted sessions and improving the overall security posture of the system. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Code Review
This pull request introduces functionality to revoke session keys, a valuable addition for security and session management. The implementation is robust and accompanied by a thorough test suite that covers various scenarios and edge cases. My review includes a critical fix for handling requests with multiple signatures to prevent non-deterministic behavior, a refactoring suggestion to improve database query efficiency, and a minor naming correction in the tests to enhance clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
clearnode/rpc_router.go(1 hunks)clearnode/rpc_router_private.go(2 hunks)clearnode/rpc_router_private_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
clearnode/rpc_router_private.go (6)
clearnode/session_key.go (5)
SessionKey(18-30)SessionKey(32-34)GetWalletBySessionKey(131-143)GetSessionKeyIfActive(187-199)AppNameClearnode(14-14)clearnode/rpc_router.go (1)
RPCRouter(16-29)clearnode/rpc_node.go (1)
RPCContext(252-266)clearnode/log.go (1)
LoggerFromContext(95-100)clearnode/pkg/rpc/payload.go (1)
Params(177-177)clearnode/rpc.go (1)
RPCErrorf(147-151)
clearnode/rpc_router_private_test.go (3)
clearnode/signer.go (2)
Signer(23-25)Allowance(17-20)clearnode/testing/client.go (2)
Signer(26-28)Allowance(513-516)clearnode/session_key.go (5)
AddSessionKey(62-123)AppNameClearnode(14-14)GetWalletBySessionKey(131-143)SessionKey(18-30)SessionKey(32-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
43b65dc to
5daff51
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
clearnode/testing/client.go (1)
394-410: Fix timestamp value in test client TypedData message.The review comment correctly identified an issue. The test client uses
big.NewInt(3600)(line 409 of clearnode/testing/client.go), which is a duration value, not a Unix timestamp.Documentation explicitly states that ExpiresAt should be "when the authentication expires (Unix timestamp)", and other parts of the codebase use
uint64(time.Now().Add(1 * time.Hour).Unix())to generate proper future timestamps. The value3600represents ~60 seconds after epoch (January 1, 1970), not a valid future expiration time.Update line 409 to use an absolute timestamp like the rest of the codebase:
"expires_at": big.NewInt(int64(time.Now().Add(1 * time.Hour).Unix())),clearnode/docs/API.md (1)
46-64: Fix duplicate “application” field in auth_request example.JSON shows “application” twice (name and address). Keep only the name, or rename the address to a different key if needed. Suggest removing the second line.
"req": [1, "auth_request", { "address": "0x1234567890abcdef...", "session_key": "0x9876543210fedcba...", // If specified, enables delegation to this key "application": "Example App", // Application name for analytics (defaults to "clearnode" if not provided) @@ - "expires_at": 1762417328, // Session expiration timestamp - "application": "0xApp1234567890abcdef..." // Application public address + "expires_at": 1762417328 // Session expiration timestamp }, 1619123456789],
♻️ Duplicate comments (2)
clearnode/rpc_router_private_test.go (1)
2380-2496: Duplicate AC labels in subtest names.Multiple tests are prefixed “AC4”. Please give unique IDs (e.g., AC4, AC5, AC6) to reduce confusion in logs.
clearnode/rpc_router_private.go (1)
826-834: Critical: Non-deterministic session key selection when multiple session keys sign the requestYou confirmed in previous review discussions that "if clearnode session key signature is present among other sigs (in case there are multiple), the request should be processed." However, the current implementation breaks on the first matching signer due to Go's random map iteration order. If a request is signed by both a clearnode session key and another app's session key, the non-clearnode key may be selected first, causing the permission check at lines 863-866 to reject a valid clearnode authorization.
Apply this diff to scan all candidate signers and prioritize clearnode keys:
- // Determine the active session key (if any) used to sign this request var activeSessionKeyAddress *string + var clearnodeKeyAddress *string + for signer := range rpcSignersMap { if GetWalletBySessionKey(signer) == c.UserID { - s := signer - activeSessionKeyAddress = &s - break + // Check if this is a clearnode session key + if sessionKey, err := GetSessionKeyIfActive(r.DB, signer); err == nil && sessionKey.Application == AppNameClearnode { + s := signer + clearnodeKeyAddress = &s + break // Clearnode key found, use it + } + // Store first non-clearnode key as fallback + if activeSessionKeyAddress == nil { + s := signer + activeSessionKeyAddress = &s + } } } + + // Prefer clearnode key if found + if clearnodeKeyAddress != nil { + activeSessionKeyAddress = clearnodeKeyAddress + }
🧹 Nitpick comments (8)
clearnode/rpc_router_private_test.go (1)
2219-2354: Reduce boilerplate with a small helper.You repeat wallet/session-key creation and AddSessionKey across subtests. Consider extracting a helper like newTestSessionKey(db, wallet, app, scope, allowances, ttl) to cut duplication and speed iteration.
Also applies to: 2479-2520
clearnode/rpc_router_auth.go (1)
13-20: Expire → expires_at wiring looks correct.End-to-end rename and use (including EIP-712 recovery) are consistent. Optionally, validate ExpiresAt is in the future and within a sane max TTL to reject stale/long-lived sessions early.
Also applies to: 48-55, 56-65, 200-204, 233-236
sdk/src/rpc/types/request.ts (1)
104-114: Requests surface additions are correct; add params mapping for stronger typing.Please add RevokeSessionKey to RPCRequestParamsByMethod (and optionally export a params alias) to keep method→params typing complete.
export type GetSessionKeysRequestParams = GetSessionKeysRequest['params']; + +// Optional ergonomic alias +export type RevokeSessionKeyRequestParams = RevokeSessionKeyRequest['params']; @@ export type RPCRequest = @@ | GetSessionKeysRequest | RevokeSessionKeyRequest @@ export type RPCRequestParamsByMethod = { @@ [RPCMethod.GetSessionKeys]: GetSessionKeysRequestParams; + [RPCMethod.RevokeSessionKey]: RevokeSessionKeyRequest['params']; [RPCMethod.CreateAppSession]: CreateAppSessionRequestParams;Also applies to: 477-488, 260-276
clearnode/pkg/rpc/api.go (1)
234-248: Add Method constant for revoke_session_key for parity with other endpoints.For consistency with other RPC methods, define a Method constant.
const ( @@ CloseAppSessionMethod Method = "close_app_session" + // RevokeSessionKeyMethod revokes a session key (auth required). + RevokeSessionKeyMethod Method = "revoke_session_key" )Also applies to: 328-339, 62-111
clearnode/docs/API.md (1)
496-547: Revoke Session Key docs — clear and complete.Nice coverage of permissions, responses, and errors. Consider noting idempotency (revoking an already-expired/non-existent key returns the documented error).
integration/tests/session_key.test.ts (2)
614-619: Avoid duplicate authentication in nested beforeEach.Outer suite already authenticates; this inner re-auth adds overhead and may rotate keys. Consider removing it unless you need a fresh policy per sub-suite.
662-672: Strengthen the post-revocation assertion.Instead of a generic throw, assert on the error RPC response (method === 'error' and message includes the expected phrase) to avoid false positives. Example:
const res = await aliceAppWS.sendAndWaitForResponse( transferMsg2, (data) => parseAnyRPCResponse(data).requestId === JSON.parse(transferMsg2).req[0], 5000 ); const parsed = parseAnyRPCResponse(res); expect(parsed.method).toBe(RPCMethod.Error); expect((parsed.params as any).error).toMatch(/session key|expired|insufficient permissions/i);clearnode/rpc_router_private.go (1)
838-852: Consider combining fetch and ownership validation into a single queryThe target session key is first fetched by address, then separately checked for ownership and expiration. These validations can be combined into a single database query by including
wallet_addressin the WHERE clause, reducing overhead and simplifying the logic.Apply this diff to combine the checks:
- // First, get the target session key from DB (without expiration check) var targetSessionKey SessionKey - err := tx.Where("address = ?", params.SessionKey).First(&targetSessionKey).Error + err := tx.Where("address = ? AND wallet_address = ?", params.SessionKey, c.UserID).First(&targetSessionKey).Error if err != nil { return RPCErrorf("operation denied: provided address is not a session key of the session wallet") } // Check if the session key is already expired if isExpired(targetSessionKey.ExpiresAt) { return RPCErrorf("operation denied: provided address is not a session key of the session wallet") } - - // Verify ownership: the session key must belong to the authenticated user's wallet - if targetSessionKey.WalletAddress != c.UserID { - return RPCErrorf("operation denied: provided address is not a session key of the session wallet") - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
clearnode/README.md(1 hunks)clearnode/auth.go(6 hunks)clearnode/docs/API.md(3 hunks)clearnode/docs/Clearnode.protocol.md(1 hunks)clearnode/docs/Entities.md(1 hunks)clearnode/pkg/rpc/api.go(2 hunks)clearnode/pkg/rpc/client.go(3 hunks)clearnode/pkg/rpc/client_internal_test.go(1 hunks)clearnode/pkg/rpc/client_manual_test.go(1 hunks)clearnode/rpc_router.go(1 hunks)clearnode/rpc_router_auth.go(4 hunks)clearnode/rpc_router_private.go(2 hunks)clearnode/rpc_router_private_test.go(1 hunks)clearnode/signer.go(3 hunks)clearnode/signer_test.go(2 hunks)clearnode/testing/client.go(2 hunks)erc7824-docs/docs/quick_start/connect_to_the_clearnode.md(6 hunks)examples/cerebro/clearnet/auth.go(3 hunks)examples/cerebro/clearnet/clearnode_client.go(0 hunks)integration/common/auth.ts(2 hunks)integration/common/testHelpers.ts(2 hunks)integration/tests/clearnode_auth.test.ts(2 hunks)integration/tests/session_key.test.ts(3 hunks)sdk/src/rpc/api.ts(1 hunks)sdk/src/rpc/types/index.ts(3 hunks)sdk/src/rpc/types/request.ts(3 hunks)sdk/src/rpc/types/response.ts(3 hunks)sdk/test/unit/rpc/api.test.ts(2 hunks)
💤 Files with no reviewable changes (1)
- examples/cerebro/clearnet/clearnode_client.go
🧰 Additional context used
🧬 Code graph analysis (8)
sdk/src/rpc/types/response.ts (1)
sdk/src/rpc/types/index.ts (1)
GenericRPCMessage(25-29)
examples/cerebro/clearnet/auth.go (3)
clearnode/pkg/sign/sign.go (2)
Address(29-34)PublicKey(23-26)clearnode/session_key.go (2)
SessionKey(18-30)SessionKey(32-34)clearnode/pkg/rpc/api.go (1)
Allowance(570-575)
clearnode/pkg/rpc/api.go (1)
clearnode/session_key.go (2)
SessionKey(18-30)SessionKey(32-34)
sdk/src/rpc/types/request.ts (1)
sdk/src/rpc/types/index.ts (1)
GenericRPCMessage(25-29)
clearnode/auth.go (2)
clearnode/session_key.go (2)
SessionKey(18-30)SessionKey(32-34)clearnode/pkg/rpc/api.go (1)
Allowance(570-575)
clearnode/rpc_router_private_test.go (3)
clearnode/signer.go (2)
Signer(23-25)Allowance(17-20)clearnode/pkg/rpc/api.go (3)
Allowance(570-575)Asset(558-567)RevokeSessionKeyResponse(335-338)clearnode/session_key.go (5)
AddSessionKey(62-123)AppNameClearnode(14-14)GetWalletBySessionKey(131-143)SessionKey(18-30)SessionKey(32-34)
sdk/src/rpc/api.ts (3)
sdk/src/rpc/types/index.ts (4)
PartialEIP712AuthMessage(136-141)EIP712AuthDomain(155-157)MessageSigner(93-93)RPCData(19-19)clearnode/testing/client.go (1)
RPCData(39-44)clearnode/pkg/rpc/error.go (1)
Error(48-50)
clearnode/rpc_router_private.go (7)
clearnode/session_key.go (5)
SessionKey(18-30)SessionKey(32-34)GetWalletBySessionKey(131-143)GetSessionKeyIfActive(187-199)AppNameClearnode(14-14)clearnode/rpc_router.go (1)
RPCRouter(16-30)clearnode/rpc_node.go (1)
RPCContext(252-266)clearnode/log.go (1)
LoggerFromContext(95-100)clearnode/pkg/rpc/payload.go (1)
Params(177-177)clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/pkg/rpc/api.go (1)
RevokeSessionKeyResponse(335-338)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test (Integration) / Test Integration
🔇 Additional comments (38)
clearnode/signer_test.go (1)
35-35: Field rename is fully implemented and consistent across the codebase.Verification confirms the "expire" to "expires_at" rename has been completely and consistently applied:
- EIP-712 type definition in
clearnode/signer.go(line 103)- Message payload construction in
RecoverAddressFromEip712Signature(line 119)- Test files (
signer_test.go,testing/client.go)- RPC client and API types (
pkg/rpc/client.go,pkg/rpc/api.go)- Auth router and related handlers
- Example code
No orphaned references to the old field name exist in EIP-712 contexts.
clearnode/README.md (1)
91-91: LGTM! Documentation updated to reflect field rename.The Policy struct documentation correctly reflects the rename from
expiretoexpires_at, maintaining consistency with the codebase changes.integration/common/auth.ts (2)
20-20: LGTM! Field renamed correctly.The
expires_atfield correctly uses a Unix timestamp (current time + 1 hour).
30-30: LGTM! EIP-712 message signer updated consistently.The field rename is correctly applied to the typed data construction.
examples/cerebro/clearnet/auth.go (3)
6-6: LGTM! Import added for timestamp calculation.The
timeimport is correctly added to support theExpiresAtfield calculation.
17-17: LGTM! Struct field renamed consistently.The field rename from
ExpiretoExpiresAtwith the JSON tagexpires_ataligns with the API surface changes across the codebase.
27-31: LGTM! Request construction updated correctly.The
ExpiresAtfield is calculated correctly as a Unix timestamp (1 hour from now) and all field mappings are consistent with the renamed API.clearnode/signer.go (1)
89-119: LGTM! EIP-712 signing data updated consistently.The parameter rename from
expiretoexpiresAtand corresponding updates to the TypedData schema (line 103) and message payload (line 119) are correct and maintain the uint64 type throughout.clearnode/pkg/rpc/client.go (2)
514-514: LGTM! Documentation updated.The comment correctly references
ExpiresAtfield instead of the oldExpirefield.
1461-1492: LGTM! EIP-712 challenge signing updated correctly.The TypedData schema (line 1472) and message payload (line 1489) correctly use
expires_atwith the value fromreq.ExpiresAt. The big.Int conversion is appropriate for EIP-712 typed data.integration/common/testHelpers.ts (2)
61-73: LGTM! Helper function updated correctly.The field rename from
expiretoexpires_at(line 65) is correct, and the timestamp calculation properly computes 1 hour from now.
79-93: LGTM! Multi-asset helper function updated correctly.The field rename from
expiretoexpires_at(line 89) is consistent with the other helper function and correctly uses an absolute Unix timestamp.clearnode/docs/Clearnode.protocol.md (1)
171-171: LGTM! Protocol documentation updated.The field name change from
expiretoexpires_atin the auth_request payload documentation is correct and aligns with the API changes across the codebase.clearnode/pkg/rpc/client_manual_test.go (1)
69-69: LGTM! Field rename is consistent with API changes.The update from
ExpiretoExpiresAtaligns with the broader field renaming across the codebase.clearnode/rpc_router.go (1)
85-85: LGTM! New revoke endpoint registered correctly.The
revoke_session_keyendpoint is appropriately placed in the private group, ensuring authentication is required before revocation operations.clearnode/pkg/rpc/client_internal_test.go (1)
139-139: LGTM! Test updated to match API field rename.The change from
ExpiretoExpiresAtis consistent with the API surface updates across the codebase.sdk/src/rpc/types/response.ts (3)
111-120: LGTM! New response type is well-structured.The
RevokeSessionKeyResponseinterface follows the established pattern and correctly returns the revoked session key address.
404-407: LGTM! Type alias follows convention.The
RevokeSessionKeyResponseParamstype alias is consistent with other response parameter types in the file.
529-529: LGTM! Union type properly extended.The
RevokeSessionKeyResponseis correctly added to theRPCResponseunion type.sdk/test/unit/rpc/api.test.ts (2)
56-56: LGTM! Field renamed and type updated correctly.The change from
expire: '0'(string) toexpires_at: 0n(bigint) is correct and consistent with TypeScript type definitions.
72-72: LGTM! Serialization output is correct.The bigint value
0ncorrectly serializes to the number0in the JSON output.integration/tests/clearnode_auth.test.ts (2)
32-32: LGTM! Expiration field renamed with correct type.The field is correctly renamed to
expires_atand usesBigInt()for the timestamp calculation.
42-42: LGTM! EIP-712 signer updated consistently.The
expires_atfield reference in the EIP-712 message signer is consistent with theauthRequestParamsdefinition.clearnode/docs/Entities.md (1)
84-86: LGTM! Field names improved for clarity.The rename from
Expire/ExpiresAttoSessionKeyExpiresAt/ChallengeExpiresAtmakes the distinction between session key expiration and challenge expiration more explicit and clear.erc7824-docs/docs/quick_start/connect_to_the_clearnode.md (5)
286-286: LGTM! Documentation example updated correctly.The field rename from
expiretoexpires_atin the authentication request example is correct and consistent with the API changes.
316-316: LGTM! EIP-712 message structure updated.The
expires_atfield reference in the EIP-712 message signer is consistent with the updated API.
375-375: LGTM! Manual challenge handling examples updated.Both occurrences of the
expires_atfield in the manual challenge handling examples are correct and consistent.Also applies to: 402-402
493-493: LGTM! EIP-712 type definition updated.The
expires_atfield in the Policy type definition correctly reflects the new field name withuint64type.
511-511: LGTM! Example message data updated.The
expires_atfield in the example EIP-712 message correctly shows a numeric timestamp value.clearnode/rpc_router_private_test.go (5)
2211-2261: Revocation happy-path looks solid.Self-revoke via session key with DB expiry and cache eviction assertions is correct and valuable coverage.
2263-2326: Clearnode session key privileged revoke — good coverage.Covers privileged revocation and verifies non-regression of key A.
2328-2379: Non-clearnode cannot revoke other keys — correct negative case.Error assertion is precise; DB and cache checks are appropriate.
2497-2539: Wallet-initiated revoke — good additional case.Covers wallet-signed revocation path and DB expiry assertion.
2541-2560: Missing param validation — good guardrail.Clear error surface for missing session_key param.
sdk/src/rpc/types/index.ts (1)
136-141: Consistent expires_at rename and new RPC method — LGTM.Types align with server payloads and docs; adding RPCMethod.RevokeSessionKey is correct.
Also applies to: 162-170, 180-193
sdk/src/rpc/api.ts (1)
720-745: New createRevokeSessionKeyMessage — LGTM.Matches existing request builders and signs payload correctly.
clearnode/pkg/rpc/api.go (1)
328-339: Public API types for revocation — LGTM.Request/response structs and JSON tags look correct.
clearnode/auth.go (1)
17-26: LGTM! Clear and consistent field naming refactorThe renaming of
Expire/ExpiresAttoSessionKeyExpiresAtandChallengeExpiresAtimproves code clarity by explicitly distinguishing between the two different expiration timestamps. All usages have been updated consistently throughout the file with no logic changes.Also applies to: 80-80, 89-99, 151-151, 166-166, 296-296
512325e to
2e86dfa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/src/rpc/types/request.ts (1)
508-532: Add revoke_session_key entry to RPCRequestParamsByMethod.The new
RevokeSessionKeyRequestis in the union, butRPCRequestParamsByMethodstill lacks a mapping forRPCMethod.RevokeSessionKey. Any code indexing that helper (e.g.,RPCRequestParamsByMethod[RPCMethod.RevokeSessionKey]) now fails to type-check because the property is missing. Please add the corresponding entry so the new RPC stays type-safe.[RPCMethod.GetUserTag]: GetUserTagRequestParams; [RPCMethod.GetSessionKeys]: GetSessionKeysRequestParams; + [RPCMethod.RevokeSessionKey]: RevokeSessionKeyRequest['params']; [RPCMethod.CreateAppSession]: CreateAppSessionRequestParams;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
clearnode/docs/API.md(3 hunks)clearnode/rpc_router_private.go(2 hunks)clearnode/rpc_router_private_test.go(1 hunks)integration/tests/session_key.test.ts(3 hunks)sdk/src/rpc/api.ts(1 hunks)sdk/src/rpc/parse/misc.ts(3 hunks)sdk/src/rpc/types/index.ts(3 hunks)sdk/src/rpc/types/request.ts(3 hunks)sdk/src/rpc/types/response.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/src/rpc/types/index.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 419
File: clearnode/rpc_router_private.go:827-834
Timestamp: 2025-11-07T14:03:35.354Z
Learning: For the `revoke_session_key` RPC endpoint in clearnode/rpc_router_private.go: The design intentionally allows non-deterministic behavior when multiple session key signatures are present. Client-side code must ensure correct signatures are provided. The server keeps the RPC protocol flexible rather than implementing deterministic signature selection logic.
📚 Learning: 2025-11-07T14:03:35.354Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 419
File: clearnode/rpc_router_private.go:827-834
Timestamp: 2025-11-07T14:03:35.354Z
Learning: For the `revoke_session_key` RPC endpoint in clearnode/rpc_router_private.go: The design intentionally allows non-deterministic behavior when multiple session key signatures are present. Client-side code must ensure correct signatures are provided. The server keeps the RPC protocol flexible rather than implementing deterministic signature selection logic.
Applied to files:
sdk/src/rpc/types/response.tsclearnode/rpc_router_private.goclearnode/rpc_router_private_test.gosdk/src/rpc/types/request.tssdk/src/rpc/parse/misc.tssdk/src/rpc/api.tsclearnode/docs/API.md
🧬 Code graph analysis (7)
sdk/src/rpc/types/response.ts (3)
clearnode/pkg/rpc/api.go (1)
RevokeSessionKeyResponse(335-338)sdk/src/rpc/types/index.ts (1)
GenericRPCMessage(25-29)clearnode/pkg/sign/sign.go (1)
Address(29-34)
clearnode/rpc_router_private.go (7)
clearnode/session_key.go (5)
SessionKey(18-30)SessionKey(32-34)GetWalletBySessionKey(131-143)GetSessionKeyIfActive(187-199)AppNameClearnode(14-14)clearnode/rpc_router.go (1)
RPCRouter(16-30)clearnode/rpc_node.go (1)
RPCContext(252-266)clearnode/log.go (1)
LoggerFromContext(95-100)clearnode/pkg/rpc/payload.go (1)
Params(177-177)clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/pkg/rpc/api.go (1)
RevokeSessionKeyResponse(335-338)
clearnode/rpc_router_private_test.go (3)
clearnode/signer.go (2)
Signer(23-25)Allowance(17-20)clearnode/pkg/rpc/api.go (2)
Allowance(570-575)RevokeSessionKeyResponse(335-338)clearnode/session_key.go (5)
AddSessionKey(62-123)AppNameClearnode(14-14)GetWalletBySessionKey(131-143)SessionKey(18-30)SessionKey(32-34)
sdk/src/rpc/types/request.ts (3)
clearnode/pkg/rpc/api.go (1)
RevokeSessionKeyRequest(329-332)sdk/src/rpc/types/index.ts (1)
GenericRPCMessage(25-29)clearnode/pkg/sign/sign.go (1)
Address(29-34)
sdk/src/rpc/parse/misc.ts (2)
sdk/src/rpc/parse/common.ts (1)
addressSchema(27-32)sdk/src/rpc/types/response.ts (1)
RevokeSessionKeyResponseParams(407-407)
sdk/src/rpc/api.ts (3)
sdk/src/rpc/types/index.ts (3)
MessageSigner(93-93)RequestID(10-10)Timestamp(13-13)sdk/src/rpc/utils.ts (2)
generateRequestId(18-20)getCurrentTimestamp(9-11)sdk/src/rpc/nitrolite.ts (1)
NitroliteRPC(17-157)
integration/tests/session_key.test.ts (3)
integration/common/testHelpers.ts (1)
authenticateAppWithAllowances(54-74)sdk/src/rpc/api.ts (2)
createTransferMessage(691-718)createRevokeSessionKeyMessage(729-744)sdk/src/rpc/parse/parse.ts (1)
parseAnyRPCResponse(12-40)
🪛 Gitleaks (8.28.0)
clearnode/docs/API.md
[high] 511-511: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 522-522: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
2e86dfa to
2bf521f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/src/rpc/types/request.ts (1)
507-532: Missing type mapping entry for RevokeSessionKey.The
RPCRequestParamsByMethodtype map (lines 507-532) is missing an entry for the newRevokeSessionKeymethod. This breaks type safety when accessing request parameters by method.Add the missing entry to the type map:
export type RPCRequestParamsByMethod = { [RPCMethod.AuthChallenge]: AuthChallengeRequestParams; [RPCMethod.AuthVerify]: AuthVerifyRequestParams; [RPCMethod.AuthRequest]: AuthRequestParams; [RPCMethod.GetConfig]: GetConfigRequestParams; [RPCMethod.GetLedgerBalances]: GetLedgerBalancesRequestParams; [RPCMethod.GetLedgerEntries]: GetLedgerEntriesRequestParams; [RPCMethod.GetLedgerTransactions]: GetLedgerTransactionsRequestParams; [RPCMethod.GetUserTag]: GetUserTagRequestParams; [RPCMethod.GetSessionKeys]: GetSessionKeysRequestParams; + [RPCMethod.RevokeSessionKey]: RevokeSessionKeyRequest['params']; [RPCMethod.CreateAppSession]: CreateAppSessionRequestParams;Note: You'll also need to export a
RevokeSessionKeyRequestParamstype alias (similar to other methods around line 355) and use it in the map instead of the inlineRevokeSessionKeyRequest['params'].
🧹 Nitpick comments (2)
clearnode/docs/API.md (2)
20-20: Minor documentation inconsistency in API endpoints table.The table description for
revoke_session_keystates "by setting its expiration to now," but the detailed section description (line 498) more accurately states "immediately invalidating it." Consider harmonizing the language for consistency.-| `revoke_session_key` | Revokes a session key by setting its expiration to now | Private | +| `revoke_session_key` | Revokes a session key by immediately invalidating it | Private |
548-549: Misplaced content: Notes belong to get_session_keys, not revoke_session_key.Lines 548–549 contain notes about "available allowance for an asset" and the "Special case" for clearnode session keys with root access. These notes are contextually part of the
get_session_keyssection documentation (should appear near line 494) and do not apply to therevoke_session_keyoperation. This appears to be residual content that shifted during the addition of the new section.Consider moving these lines back to the end of the
get_session_keys→ Notes: section to maintain logical organization:### Get Session Keys [... existing get_session_keys documentation ...] **Notes:** - Only active (non-expired) session keys are returned - If a session key has an empty spending cap array, all operations using that key will be denied (no spending allowed) - The `used_allowance` is calculated by summing all debit entries in the ledger that were made using this session key - Available allowance for an asset = `allowance - used_allowance` - **Special case**: Session keys with `application` set to `"clearnode"` have root access and are exempt from spending allowance limits and application validation. This facilitates backwards compatibility, and will be deprecated after a migration period for developers elapses. ### Revoke Session Key [... revoke_session_key documentation ...]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
clearnode/docs/API.md(3 hunks)clearnode/rpc_router_private.go(2 hunks)clearnode/rpc_router_private_test.go(1 hunks)integration/tests/session_key.test.ts(3 hunks)sdk/src/rpc/api.ts(1 hunks)sdk/src/rpc/parse/misc.ts(3 hunks)sdk/src/rpc/types/index.ts(3 hunks)sdk/src/rpc/types/request.ts(3 hunks)sdk/src/rpc/types/response.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- sdk/src/rpc/types/index.ts
- clearnode/rpc_router_private.go
- sdk/src/rpc/api.ts
- clearnode/rpc_router_private_test.go
- sdk/src/rpc/parse/misc.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 419
File: clearnode/rpc_router_private.go:827-834
Timestamp: 2025-11-07T14:03:35.354Z
Learning: For the `revoke_session_key` RPC endpoint in clearnode/rpc_router_private.go: The design intentionally allows non-deterministic behavior when multiple session key signatures are present. Client-side code must ensure correct signatures are provided. The server keeps the RPC protocol flexible rather than implementing deterministic signature selection logic.
📚 Learning: 2025-11-07T14:03:35.354Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 419
File: clearnode/rpc_router_private.go:827-834
Timestamp: 2025-11-07T14:03:35.354Z
Learning: For the `revoke_session_key` RPC endpoint in clearnode/rpc_router_private.go: The design intentionally allows non-deterministic behavior when multiple session key signatures are present. Client-side code must ensure correct signatures are provided. The server keeps the RPC protocol flexible rather than implementing deterministic signature selection logic.
Applied to files:
sdk/src/rpc/types/request.tssdk/src/rpc/types/response.tsclearnode/docs/API.md
🧬 Code graph analysis (3)
sdk/src/rpc/types/request.ts (2)
clearnode/pkg/rpc/api.go (1)
RevokeSessionKeyRequest(329-332)sdk/src/rpc/types/index.ts (1)
GenericRPCMessage(25-29)
sdk/src/rpc/types/response.ts (3)
clearnode/pkg/rpc/api.go (1)
RevokeSessionKeyResponse(335-338)sdk/src/rpc/types/index.ts (1)
GenericRPCMessage(25-29)clearnode/pkg/sign/sign.go (1)
Address(29-34)
integration/tests/session_key.test.ts (3)
integration/common/testHelpers.ts (1)
authenticateAppWithAllowances(54-74)sdk/src/rpc/api.ts (2)
createTransferMessage(691-718)createRevokeSessionKeyMessage(729-744)sdk/src/rpc/parse/parse.ts (1)
parseAnyRPCResponse(12-40)
🪛 Gitleaks (8.28.0)
clearnode/docs/API.md
[high] 511-511: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 522-522: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
clearnode/docs/API.md (1)
496-547: Comprehensive and well‑structured revoke_session_key documentation.The new section provides clear permission rules, realistic request/response examples, well-documented parameters, explicit error cases, and practical notes. The permission hierarchy (wallet → session key self-revocation → clearnode session key authority) is clearly articulated. This aligns well with the intentional non-deterministic behavior noted in the learnings: clients must provide correct signatures, while the server keeps the protocol flexible.
sdk/src/rpc/types/request.ts (2)
104-113: LGTM! Well-structured request interface.The
RevokeSessionKeyRequestinterface is correctly defined with proper typing and documentation. Thesession_keyparameter uses snake_case, which aligns with the backend JSON format.
272-272: LGTM! Consistent naming improvement.The renaming from
expiretoexpires_atimproves clarity and aligns with the broader renaming effort mentioned in the PR summary.sdk/src/rpc/types/response.ts (3)
111-120: LGTM! Response interface follows established conventions.The
RevokeSessionKeyResponseinterface is correctly defined and follows the camelCase naming convention used throughout the response types (consistent withsessionKeys,challengeMessage, etc.).
404-407: LGTM! Standard type alias.The
RevokeSessionKeyResponseParamstype alias follows the established pattern for extracting response parameters.
529-529: LGTM! Union type correctly extended.The
RPCResponseunion type is properly updated to includeRevokeSessionKeyResponse.
2bf521f to
366f27a
Compare
nksazonov
left a comment
There was a problem hiding this comment.
Some interesting comments. No critical things, on the other hand.
| await authenticateAppWithAllowances(aliceAppWS, aliceAppIdentity, ASSET_SYMBOL, spendingCapAmount.toString()); | ||
| }); | ||
|
|
||
| it('should reject transfer after session key is revoked', async () => { |
There was a problem hiding this comment.
I think that we should also test that a revoked session key can not create app session nor submit an app state.
Moreover, we I think we should test different scenarios of session key revokation (the same as in go).
There was a problem hiding this comment.
Could you also add separate different test cases that will try revoking session key, akin to what and how you are already testing in go?
e5f5060 to
b3c365d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clearnode/docs/API.md (1)
46-64: Auth request example has duplicate "application" key; keep a single field.Only one
application(string) exists in public types. Remove the second occurrence (the address). If an app address is needed, introduce a distinct field (e.g.,application_address) consistently across server, SDK, and docs.Apply this diff:
@@ "application": "Example App", // Application name for analytics (defaults to "clearnode" if not provided) @@ - "expires_at": 1762417328, // Session expiration timestamp - "application": "0xApp1234567890abcdef..." // Application public address + "expires_at": 1762417328 // Session expiration timestamp
🧹 Nitpick comments (6)
sdk/test/unit/rpc/api.test.ts (1)
51-60: Duplicate "application" key shadows the first value; fix payload and expectation.Object literals with duplicate keys keep only the last. Here,
application: 'test-app'is overwritten byapplication: clientAddress, and the expectation mirrors this. Keep a single, intended field (types only expose one string field).Apply this diff:
@@ const authRequest: AuthRequestParams = { address: clientAddress, session_key: clientAddress, - application: 'test-app', allowances: [], - expires_at: 0n, + application: 'test-app', + expires_at: 0n, scope: '', - application: clientAddress, }; @@ { address: clientAddress, session_key: clientAddress, - application: 'test-app', allowances: [], - expires_at: 0, + application: 'test-app', + expires_at: 0, scope: '', - application: clientAddress, },Also applies to: 63-75
clearnode/docs/API.md (2)
498-551: Revoke Session Key section reads well; mirror exact error strings and add note on signer selection.
- Ensure error messages match server strings exactly (they currently do).
- Add a note that when multiple signatures are present, any valid one may be accepted; clients must include the intended key’s signature. Based on learnings.
I can PR a one-liner note right under “Permission Rules”.
513-527: Neutralize example tokens/signatures to avoid false-positive secret scans.Static analyzers flagged “Generic API Key” on placeholders. Use clearly fake values like
0xdead...deador wrap examples with a note “example only”.I can batch-replace example sigs/JWTs to safer placeholders.
sdk/src/rpc/types/request.ts (1)
477-503: Add RPCRequestParamsByMethod entry for RevokeSessionKey.The union includes
RevokeSessionKeyRequest, but method→params map lacks it. Add for parity and better typing.Apply this diff:
@@ export type RPCRequest = | GetSessionKeysRequest - | RevokeSessionKeyRequest + | RevokeSessionKeyRequest @@ export type RPCRequestParamsByMethod = { [RPCMethod.GetSessionKeys]: GetSessionKeysRequestParams; + [RPCMethod.RevokeSessionKey]: RevokeSessionKeyRequest['params'];Also applies to: 507-532
clearnode/pkg/rpc/api.go (1)
328-339: Expose a Method constant for revoke_session_key for consistency.Other endpoints have
Methodconstants. Add one for"revoke_session_key"to keep parity and aid client code.Apply this diff near other method constants:
@@ // GetSessionKeysMethod returns session keys with allowances (auth required). GetSessionKeysMethod Method = "get_session_keys" + // RevokeSessionKeyMethod revokes an active session key (auth required). + RevokeSessionKeyMethod Method = "revoke_session_key"integration/tests/session_key.test.ts (1)
614-682: Consider expanding test coverage for revocation scenariosThe current test validates that transfers fail after revocation, which is good. However, as noted in a previous comment, it would be valuable to also test:
- Revoked session key cannot create app sessions
- Revoked session key cannot submit app state
- Different revocation scenarios (self-revocation vs. clearnode key revoking another key)
- Other edge cases that may be covered in the Go tests (
clearnode/rpc_router_private_test.go)These additional test cases would provide more comprehensive coverage of the revocation mechanism and ensure consistency between the integration tests and backend test scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
clearnode/README.md(1 hunks)clearnode/auth.go(6 hunks)clearnode/docs/API.md(3 hunks)clearnode/docs/Clearnode.protocol.md(1 hunks)clearnode/docs/Entities.md(1 hunks)clearnode/pkg/rpc/api.go(2 hunks)clearnode/pkg/rpc/client.go(3 hunks)clearnode/pkg/rpc/client_internal_test.go(1 hunks)clearnode/pkg/rpc/client_manual_test.go(1 hunks)clearnode/rpc_router.go(1 hunks)clearnode/rpc_router_auth.go(4 hunks)clearnode/rpc_router_private.go(5 hunks)clearnode/rpc_router_private_test.go(1 hunks)clearnode/signer.go(3 hunks)clearnode/signer_test.go(2 hunks)clearnode/testing/client.go(2 hunks)erc7824-docs/docs/quick_start/connect_to_the_clearnode.md(6 hunks)examples/cerebro/clearnet/auth.go(3 hunks)examples/cerebro/clearnet/clearnode_client.go(0 hunks)integration/common/auth.ts(2 hunks)integration/common/testHelpers.ts(2 hunks)integration/tests/clearnode_auth.test.ts(2 hunks)integration/tests/session_key.test.ts(3 hunks)sdk/src/rpc/api.ts(1 hunks)sdk/src/rpc/parse/misc.ts(3 hunks)sdk/src/rpc/types/index.ts(3 hunks)sdk/src/rpc/types/request.ts(3 hunks)sdk/src/rpc/types/response.ts(3 hunks)sdk/test/unit/rpc/api.test.ts(2 hunks)
💤 Files with no reviewable changes (1)
- examples/cerebro/clearnet/clearnode_client.go
🚧 Files skipped from review as they are similar to previous changes (12)
- integration/common/testHelpers.ts
- clearnode/pkg/rpc/client_manual_test.go
- clearnode/docs/Clearnode.protocol.md
- erc7824-docs/docs/quick_start/connect_to_the_clearnode.md
- clearnode/README.md
- integration/tests/clearnode_auth.test.ts
- sdk/src/rpc/parse/misc.ts
- clearnode/docs/Entities.md
- clearnode/signer_test.go
- clearnode/testing/client.go
- sdk/src/rpc/types/index.ts
- clearnode/signer.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 419
File: clearnode/rpc_router_private.go:827-834
Timestamp: 2025-11-07T14:03:35.354Z
Learning: For the `revoke_session_key` RPC endpoint in clearnode/rpc_router_private.go: The design intentionally allows non-deterministic behavior when multiple session key signatures are present. Client-side code must ensure correct signatures are provided. The server keeps the RPC protocol flexible rather than implementing deterministic signature selection logic.
📚 Learning: 2025-11-07T14:03:35.354Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 419
File: clearnode/rpc_router_private.go:827-834
Timestamp: 2025-11-07T14:03:35.354Z
Learning: For the `revoke_session_key` RPC endpoint in clearnode/rpc_router_private.go: The design intentionally allows non-deterministic behavior when multiple session key signatures are present. Client-side code must ensure correct signatures are provided. The server keeps the RPC protocol flexible rather than implementing deterministic signature selection logic.
Applied to files:
clearnode/rpc_router.gosdk/src/rpc/types/response.tssdk/src/rpc/types/request.tsclearnode/pkg/rpc/api.goclearnode/rpc_router_auth.goclearnode/rpc_router_private.goclearnode/rpc_router_private_test.gointegration/tests/session_key.test.tssdk/src/rpc/api.tsexamples/cerebro/clearnet/auth.goclearnode/docs/API.md
🧬 Code graph analysis (9)
sdk/src/rpc/types/response.ts (2)
clearnode/pkg/rpc/api.go (1)
RevokeSessionKeyResponse(335-338)sdk/src/rpc/types/index.ts (1)
GenericRPCMessage(25-29)
sdk/src/rpc/types/request.ts (3)
clearnode/pkg/rpc/api.go (1)
RevokeSessionKeyRequest(329-332)sdk/src/rpc/types/index.ts (1)
GenericRPCMessage(25-29)clearnode/pkg/sign/sign.go (1)
Address(29-34)
clearnode/pkg/rpc/api.go (1)
clearnode/session_key.go (2)
SessionKey(18-30)SessionKey(32-34)
clearnode/rpc_router_private.go (9)
clearnode/session_key.go (5)
SessionKey(18-30)SessionKey(32-34)GetSessionKeyIfActive(187-199)AppNameClearnode(14-14)GetWalletBySessionKey(131-143)clearnode/rpc_router.go (1)
RPCRouter(16-30)clearnode/rpc_node.go (1)
RPCContext(252-266)clearnode/log.go (1)
LoggerFromContext(95-100)clearnode/pkg/rpc/payload.go (1)
Params(177-177)clearnode/pkg/rpc/error.go (2)
Error(48-50)Errorf(71-75)clearnode/rpc.go (2)
RPCErrorf(147-151)RPCMessage(11-16)clearnode/pkg/rpc/api.go (1)
RevokeSessionKeyResponse(335-338)clearnode/signer.go (1)
RecoverAddress(62-80)
clearnode/rpc_router_private_test.go (3)
clearnode/signer.go (2)
Signer(23-25)Allowance(17-20)clearnode/pkg/rpc/api.go (3)
Allowance(570-575)Asset(558-567)RevokeSessionKeyResponse(335-338)clearnode/session_key.go (5)
AddSessionKey(62-123)GetWalletBySessionKey(131-143)SessionKey(18-30)SessionKey(32-34)AppNameClearnode(14-14)
integration/tests/session_key.test.ts (3)
integration/common/testHelpers.ts (1)
authenticateAppWithAllowances(54-74)sdk/src/rpc/api.ts (2)
createTransferMessage(691-718)createRevokeSessionKeyMessage(729-744)sdk/src/rpc/parse/parse.ts (1)
parseAnyRPCResponse(12-40)
sdk/src/rpc/api.ts (3)
sdk/src/rpc/types/index.ts (3)
MessageSigner(93-93)RequestID(10-10)Timestamp(13-13)sdk/src/rpc/utils.ts (2)
generateRequestId(18-20)getCurrentTimestamp(9-11)sdk/src/rpc/nitrolite.ts (1)
NitroliteRPC(17-157)
clearnode/auth.go (2)
clearnode/session_key.go (2)
SessionKey(18-30)SessionKey(32-34)clearnode/pkg/rpc/api.go (1)
Allowance(570-575)
examples/cerebro/clearnet/auth.go (3)
clearnode/pkg/sign/sign.go (2)
Address(29-34)PublicKey(23-26)clearnode/session_key.go (2)
SessionKey(18-30)SessionKey(32-34)clearnode/pkg/rpc/api.go (1)
Allowance(570-575)
🪛 Gitleaks (8.28.0)
clearnode/docs/API.md
[high] 513-513: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 524-524: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
🔇 Additional comments (14)
examples/cerebro/clearnet/auth.go (2)
6-6: LGTM! Consistent with API surface update.The
timeimport and the field rename fromExpiretoExpiresAt(with corresponding JSON tag update) are correct and consistent with the project-wide API update mentioned in the PR summary.Also applies to: 17-17
26-32: LGTM! Authentication parameters correctly updated.The authentication request construction is correct:
- The
ExpiresAtcalculation properly converts a 1-hour expiration to a Unix timestamp- Field renames (
Application,ExpiresAt) align with the updated RPC API- Hardcoded duration and empty allowances are acceptable simplifications for example code
sdk/test/unit/rpc/api.test.ts (1)
56-57: BigInt JSON encoding policy differs across tests; confirm intended rule.Here
expires_at: 0nbecomes number0, whileresize_amountis stringified. If the policy is “all bigints → strings in JSON”, this test should expect"0". Ifexpires_atis an exception (safe 64-bit seconds), document that and keep the mixed behavior.Do we standardize JSON bigints as strings everywhere except
expires_at? If yes, please note it in sdk/src/rpc/api.ts and docs/API.md; if not, I can patch the serializer/tests to makeexpires_ata string as well.Also applies to: 72-73
clearnode/pkg/rpc/client.go (1)
1468-1491: EIP-712 payload update to expires_at looks correct.TypedData field and message map align with
uint64semantics and Go API rename.sdk/src/rpc/types/request.ts (2)
104-114: New RevokeSessionKeyRequest type looks good.Typed shape matches server:
{ session_key: Address }.
271-276: Public type uses bigint for expires_at — consistent with tests.No issues.
clearnode/pkg/rpc/api.go (1)
244-248: AuthRequestRequest: ExpiresAt rename is consistent and clear.No issues.
clearnode/rpc_router_private_test.go (1)
2246-2581: Great coverage for revoke_session_key; two small improvements.
- Prefer typed params over map for clarity.
- Parallel subtests + global caches can flake; verify cache is concurrency-safe and reset by cleanup.
Apply this diff to use the public request type (where convenient):
- params := map[string]interface{}{ - "session_key": sessionKeyAAddr, - } + params := rpc.RevokeSessionKeyRequest{SessionKey: sessionKeyAAddr}And check cache safety:
clearnode/auth.go (1)
17-27: LGTM! Clear naming improvementThe field renames from
Expire/ExpiresAttoSessionKeyExpiresAt/ChallengeExpiresAtsignificantly improve code clarity by explicitly distinguishing between session key expiration (external unix timestamp) and challenge expiration (internal TTL). All usages throughout the file are consistently updated.clearnode/rpc_router_private.go (4)
27-29: LGTM!The
RevokeSessionKeyParamsstruct follows the established pattern and naming conventions used by other parameter structs in the file.
262-268: LGTM! Simplified signer detectionThe refactored approach using
signerAddressfromverifySigneris cleaner than the previous signer map iteration. The logic correctly identifies session key usage by comparingsignerAddresswithfromWallet.Also applies to: 318-320
820-828: Note: Error message distinction may need documentation updateThe implementation correctly distinguishes between a non-existent session key (line 823) and an expired one (line 827) with different error messages. A previous reviewer (nksazonov) noted that the acceptance criteria define these as having the same error message and suggested updating
docs/API.mdto reflect this distinction if intentional.The current distinction seems reasonable from a user perspective, but please ensure the API documentation is aligned with this behavior.
Based on learnings
917-933: LGTM! Well-designed enhancementThe modification to return the signer address (before wallet mapping) is a clean enhancement that provides callers with the information needed to distinguish between wallet and session key signatures without changing the validation logic. Both call sites correctly handle the new return value.
sdk/src/rpc/api.ts (1)
729-744: No action required—AI summary was incorrectThe search confirms only a single declaration of
createRevokeSessionKeyMessageexists at line 729. The AI-generated summary's claim of duplicate exports was false. The code is correct as-is.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
integration/tests/session_key.test.ts (2)
638-654: Consider extracting the revocation flow to a helper function.The revocation sequence (create message → send → parse → validate) is duplicated across all three test cases. Extracting this to a helper would improve maintainability.
Example helper function to add to
testHelpers.ts:export async function revokeSessionKey( ws: TestWebSocket, identity: Identity, sessionKeyAddress: Address ): Promise<void> { const revokeMsg = await createRevokeSessionKeyMessage( identity.messageWalletSigner, sessionKeyAddress ); const revokeResponse = await ws.sendAndWaitForResponse( revokeMsg, (data: string) => { const parsed = parseAnyRPCResponse(data); return parsed.method === RPCMethod.RevokeSessionKey; }, 5000 ); const parsedRevoke = parseAnyRPCResponse(revokeResponse); expect(parsedRevoke.method).toBe(RPCMethod.RevokeSessionKey); expect((parsedRevoke.params as any).sessionKey).toBe(sessionKeyAddress); }Then use it in tests:
-const revokeMsg = await createRevokeSessionKeyMessage( - aliceAppIdentity.messageWalletSigner, - aliceAppIdentity.sessionKeyAddress -); - -const revokeResponse = await aliceAppWS.sendAndWaitForResponse( - revokeMsg, - (data: string) => { - const parsed = parseAnyRPCResponse(data); - return parsed.method === RPCMethod.RevokeSessionKey; - }, - 5000 -); - -const parsedRevoke = parseAnyRPCResponse(revokeResponse); -expect(parsedRevoke.method).toBe(RPCMethod.RevokeSessionKey); -expect((parsedRevoke.params as any).sessionKey).toBe(aliceAppIdentity.sessionKeyAddress); +await revokeSessionKey(aliceAppWS, aliceAppIdentity, aliceAppIdentity.sessionKeyAddress);Also applies to: 685-701, 732-748
662-680: Strengthen error assertions with specific patterns.The failure assertions use generic
.rejects.toThrow()without specifying expected error messages or patterns. Other tests in this file use specific patterns (e.g., lines 131, 208, 288). This would make the tests more precise and easier to debug when they fail.Apply these diffs to add specific error patterns:
For the transfer rejection test (lines 662-680):
) - ).rejects.toThrow(); + ).rejects.toThrow(/session key.*revoked|authentication.*failed/i);For the app session creation rejection test (lines 704-714):
) - ).rejects.toThrow(); + ).rejects.toThrow(/session key.*revoked|authentication.*failed/i);For the app state submission rejection test (lines 765-775):
) - ).rejects.toThrow(); + ).rejects.toThrow(/session key.*revoked|authentication.*failed/i);Also applies to: 704-714, 765-775
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration/tests/session_key.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 419
File: clearnode/rpc_router_private.go:827-834
Timestamp: 2025-11-07T14:03:35.354Z
Learning: For the `revoke_session_key` RPC endpoint in clearnode/rpc_router_private.go: The design intentionally allows non-deterministic behavior when multiple session key signatures are present. Client-side code must ensure correct signatures are provided. The server keeps the RPC protocol flexible rather than implementing deterministic signature selection logic.
📚 Learning: 2025-11-07T14:03:35.354Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 419
File: clearnode/rpc_router_private.go:827-834
Timestamp: 2025-11-07T14:03:35.354Z
Learning: For the `revoke_session_key` RPC endpoint in clearnode/rpc_router_private.go: The design intentionally allows non-deterministic behavior when multiple session key signatures are present. Client-side code must ensure correct signatures are provided. The server keeps the RPC protocol flexible rather than implementing deterministic signature selection logic.
Applied to files:
integration/tests/session_key.test.ts
🧬 Code graph analysis (1)
integration/tests/session_key.test.ts (4)
integration/common/testHelpers.ts (2)
authenticateAppWithAllowances(54-74)createTestAppSession(98-151)sdk/src/rpc/api.ts (2)
createTransferMessage(691-718)createRevokeSessionKeyMessage(729-744)sdk/src/rpc/parse/parse.ts (1)
parseAnyRPCResponse(12-40)integration/common/testAppSessionHelpers.ts (1)
submitAppStateUpdate_v04(53-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test (Integration) / Test Integration
🔇 Additional comments (4)
integration/tests/session_key.test.ts (4)
7-14: LGTM!The new imports support the session key revocation test suite and are correctly sourced from the
@erc7824/nitrolitepackage.
28-28: LGTM!The suite rename to "Session Keys" appropriately reflects the expanded scope that now includes both spending caps and revocation tests.
614-618: LGTM!The test suite structure and setup follow established patterns from the existing spending cap tests.
620-776: Comprehensive revocation test coverage.The three test cases thoroughly validate that revoked session keys are rejected across all key operations: transfers, app session creation, and app state submissions. This addresses the test scenarios suggested in the previous review comments.
Based on learnings.
| await authenticateAppWithAllowances(aliceAppWS, aliceAppIdentity, ASSET_SYMBOL, spendingCapAmount.toString()); | ||
| }); | ||
|
|
||
| it('should reject transfer after session key is revoked', async () => { |
There was a problem hiding this comment.
Could you also add separate different test cases that will try revoking session key, akin to what and how you are already testing in go?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
integration/tests/session_key.test.ts (1)
643-836: Consider extracting duplicate error-handling predicate logic.The tests at lines 643-688, 732-767, 769-803, and 805-836 all contain nearly identical predicate functions for handling RPC responses and errors. This pattern could be extracted into a reusable helper function to reduce duplication and improve maintainability.
Example helper:
function createRPCPredicate(expectedMethod: RPCMethod, requestMessage: string) { return (data: string) => { const parsed = parseAnyRPCResponse(data); const requestId = JSON.parse(requestMessage).req[0]; if (parsed.requestId === requestId) { if (parsed.method === RPCMethod.Error) { const errorMsg = (parsed.params as any)?.error || 'Unknown error'; throw new Error(`RPC Error: ${errorMsg}`); } return parsed.method === expectedMethod; } return false; }; }Then usage would simplify to:
await aliceAppWS.sendAndWaitForResponse( transferMsg, createRPCPredicate(RPCMethod.Transfer, transferMsg), 5000 )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration/tests/session_key.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 419
File: clearnode/rpc_router_private.go:827-834
Timestamp: 2025-11-07T14:03:35.354Z
Learning: For the `revoke_session_key` RPC endpoint in clearnode/rpc_router_private.go: The design intentionally allows non-deterministic behavior when multiple session key signatures are present. Client-side code must ensure correct signatures are provided. The server keeps the RPC protocol flexible rather than implementing deterministic signature selection logic.
📚 Learning: 2025-11-07T14:03:35.354Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 419
File: clearnode/rpc_router_private.go:827-834
Timestamp: 2025-11-07T14:03:35.354Z
Learning: For the `revoke_session_key` RPC endpoint in clearnode/rpc_router_private.go: The design intentionally allows non-deterministic behavior when multiple session key signatures are present. Client-side code must ensure correct signatures are provided. The server keeps the RPC protocol flexible rather than implementing deterministic signature selection logic.
Applied to files:
integration/tests/session_key.test.ts
🧬 Code graph analysis (1)
integration/tests/session_key.test.ts (7)
integration/common/ws.ts (1)
TestWebSocket(9-139)integration/common/identity.ts (1)
Identity(7-33)integration/common/testHelpers.ts (2)
authenticateAppWithAllowances(54-74)createTestAppSession(98-151)integration/common/setup.ts (1)
CONFIG(4-44)sdk/src/rpc/api.ts (2)
createRevokeSessionKeyMessage(729-744)createTransferMessage(691-718)sdk/src/rpc/parse/parse.ts (1)
parseAnyRPCResponse(12-40)integration/common/testAppSessionHelpers.ts (1)
submitAppStateUpdate_v04(53-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
integration/tests/session_key.test.ts (5)
7-14: LGTM!The import additions are appropriate for the new revocation test scenarios. The functions enable message creation, response parsing, and method type checking needed for comprehensive revocation testing.
28-33: LGTM!The helper function correctly generates random 32-byte private keys using
crypto.getRandomValues, which is appropriate for testing scenarios.
35-35: LGTM!The test suite rename appropriately reflects the expanded scope to include both spending caps and revocation functionality.
621-641: LGTM!The test setup properly creates both regular and clearnode session keys for testing different revocation scenarios, with appropriate cleanup in the afterEach hook.
838-994: LGTM! Comprehensive post-revocation verification.These tests effectively verify that various operations (transfer, app session creation, app state submission) fail after a session key is revoked, addressing the requirements from the previous review feedback. The end-to-end flows provide good coverage of the revocation mechanism's impact on subsequent operations.
904f8f3 to
1df9637
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration/tests/session_key.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 419
File: clearnode/rpc_router_private.go:827-834
Timestamp: 2025-11-07T14:03:35.354Z
Learning: For the `revoke_session_key` RPC endpoint in clearnode/rpc_router_private.go: The design intentionally allows non-deterministic behavior when multiple session key signatures are present. Client-side code must ensure correct signatures are provided. The server keeps the RPC protocol flexible rather than implementing deterministic signature selection logic.
📚 Learning: 2025-11-07T14:03:35.354Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 419
File: clearnode/rpc_router_private.go:827-834
Timestamp: 2025-11-07T14:03:35.354Z
Learning: For the `revoke_session_key` RPC endpoint in clearnode/rpc_router_private.go: The design intentionally allows non-deterministic behavior when multiple session key signatures are present. Client-side code must ensure correct signatures are provided. The server keeps the RPC protocol flexible rather than implementing deterministic signature selection logic.
Applied to files:
integration/tests/session_key.test.ts
🧬 Code graph analysis (1)
integration/tests/session_key.test.ts (6)
integration/common/ws.ts (1)
TestWebSocket(9-139)integration/common/identity.ts (1)
Identity(7-33)integration/common/testHelpers.ts (2)
authenticateAppWithAllowances(54-74)createTestAppSession(98-151)sdk/src/rpc/api.ts (2)
createRevokeSessionKeyMessage(729-744)createTransferMessage(691-718)sdk/src/rpc/parse/parse.ts (1)
parseAnyRPCResponse(12-40)integration/common/testAppSessionHelpers.ts (1)
submitAppStateUpdate_v04(53-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
clearnode/rpc_router_private.go (2)
790-865: Well-structured revocation handler with sound permission model.The implementation correctly enforces the permission hierarchy:
- Wallet owner can revoke any session key
- Session keys can revoke themselves
- Only clearnode session keys can revoke other keys
The transaction ensures atomicity, cache invalidation prevents stale data, and logging provides a complete audit trail.
Consider adding hex address validation for
params.SessionKeybefore the database query to fail fast on malformed input:if params.SessionKey == "" { c.Fail(nil, "session_key parameter is required") return } + + if !common.IsHexAddress(params.SessionKey) { + c.Fail(nil, "invalid session_key format") + return + }
825-838: Permission logic is correct; consider enhanced documentation.The permission checks correctly implement the authorization model. The code is reasonably clear, though more detailed comments could help future maintainers understand the three authorization paths (wallet-signed, self-revocation, clearnode delegation).
Consider expanding the inline comment to document all three authorization cases:
if activeSessionKeyAddress != nil { - // If trying to revoke a different session key (not self-revocation) + // Session key authorization: allow self-revocation, otherwise require clearnode key + // - Self-revocation: session key can revoke itself + // - Cross-revocation: only clearnode application keys can revoke other keys if *activeSessionKeyAddress != params.SessionKey {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clearnode/rpc_router_private.go(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 419
File: clearnode/rpc_router_private.go:827-834
Timestamp: 2025-11-07T14:03:35.354Z
Learning: For the `revoke_session_key` RPC endpoint in clearnode/rpc_router_private.go: The design intentionally allows non-deterministic behavior when multiple session key signatures are present. Client-side code must ensure correct signatures are provided. The server keeps the RPC protocol flexible rather than implementing deterministic signature selection logic.
📚 Learning: 2025-11-07T14:03:35.354Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 419
File: clearnode/rpc_router_private.go:827-834
Timestamp: 2025-11-07T14:03:35.354Z
Learning: For the `revoke_session_key` RPC endpoint in clearnode/rpc_router_private.go: The design intentionally allows non-deterministic behavior when multiple session key signatures are present. Client-side code must ensure correct signatures are provided. The server keeps the RPC protocol flexible rather than implementing deterministic signature selection logic.
Applied to files:
clearnode/rpc_router_private.go
🧬 Code graph analysis (1)
clearnode/rpc_router_private.go (6)
clearnode/session_key.go (7)
SessionKey(18-30)SessionKey(32-34)GetActiveSessionKeyForWallet(202-214)GetSessionKeyIfActive(187-199)AppNameClearnode(14-14)RevokeSessionKeyFromDB(299-307)GetWalletBySessionKey(131-143)clearnode/rpc_node.go (1)
RPCContext(252-266)clearnode/log.go (1)
LoggerFromContext(95-100)clearnode/rpc.go (2)
RPCErrorf(147-151)RPCMessage(11-16)clearnode/pkg/rpc/api.go (1)
RevokeSessionKeyResponse(335-338)clearnode/signer.go (1)
RecoverAddress(62-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
clearnode/rpc_router_private.go (3)
27-29: LGTM: Parameter struct follows established patterns.The
RevokeSessionKeyParamsstruct is cleanly defined and consistent with other parameter structures in the codebase.
262-268: LGTM: Cleaner signer resolution improves determinism.The change to capture
signerAddressfromverifySignerand use it directly eliminates the previous non-deterministic map iteration. This makes the code more predictable while keeping it simple.Also applies to: 318-320
909-925: LGTM: Clean signature extension enables better signer tracking.The function now returns the actual signer address, which allows callers to distinguish between wallet-signed and session-key-signed requests without additional lookups. The implementation correctly preserves the original signer before wallet mapping.
Summary by CodeRabbit
New Features
API Changes
Documentation
Tests