YNU-486: do not create new session key on auth if exists#428
YNU-486: do not create new session key on auth if exists#428
Conversation
Summary of ChangesHello @philanton, 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 enhances the session key management during the authentication process by implementing a pre-check for existing session keys. The primary goal is to ensure that session keys are only added if they don't already exist or are not valid, thereby improving the efficiency and robustness of the authentication mechanism. 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
|
|
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 session-key existence pre-check, propagates session key expiration into JWT generation, updates cache commit ordering, and introduces a test-mode RPC to clear the session-key cache; also updates SDK/types/parsers, examples, integration helpers, and docs for the new cleanup RPC and session-key flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as RPC Client / WS
participant Router as RPCRouter
participant Auth as AuthManager
participant Cache as Session Cache
participant DB as Database
Client->>Router: Auth sig-verify (wallet, signer, challenge)
Router->>Auth: handleAuthSigVerify(...)
Auth->>Cache: CheckSessionKeyExists(wallet, signer) %%{bgColor:"#E8F6EF", title:"cache/db pre-check"}%%
alt exists & valid
Cache-->>Auth: true
note right of Auth: skip AddSessionKey
else exists but expired / wrong owner
Cache-->>Auth: error
Auth-->>Router: return error
else not found
Cache-->>Auth: false
Auth->>DB: AddSessionKey(wallet, signer, expiresAt)
DB-->>Auth: commit success
Auth->>Cache: update cache post-commit
end
Auth->>Auth: GenerateJWT(..., sessionKeyExpiresAt) %%{bgColor:"#FFF4E0", title:"jwt uses session expiry"}%%
Auth-->>Router: JWT / error
Router-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-11-07T14:03:35.354ZApplied to files:
🧬 Code graph analysis (1)clearnode/session_key.go (2)
⏰ 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)
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Code Review
The pull request introduces a check to see if a session key already exists before adding a new one during authentication. While the intent is good, the implementation has a critical flaw: it only checks an in-memory cache, which can be out of sync with the database, leading to potential race conditions and database errors. Additionally, the logic incorrectly handles expired session keys, preventing users from re-authenticating. My review includes suggestions to make the check database-aware and to correct the logic for expired keys.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
clearnode/session_key.go (1)
134-134: Clarify the error message for expired keys.The error message "session key already exists but is expired" might confuse users about why they cannot re-register an expired key. Consider a clearer message like "session key address cannot be reused after expiration" to indicate this is an intentional security restriction.
clearnode/rpc_router_auth.go (1)
233-244: Improve logging clarity; TOCTOU is client-side responsibility per design.Per the codebase design (and retrieved learning on
revoke_session_key), concurrent requests with identical session key signatures are client-side errors. The DB unique constraint onAddress(session_key.go:20) ensures final state correctness. However, improve observability:
Log message clarity (line 235):
"failed to check existing session key"is misleading.CheckSessionKeyExistsvalidates expiration and wallet ownership. Revise to:"session key validation failed".Idempotent case logging (line 239): When
existsis true, add debug logging:logger.Debug("session key already exists and is valid")for operational visibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clearnode/rpc_router_auth.go(1 hunks)clearnode/session_key.go(1 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/session_key.goclearnode/rpc_router_auth.go
🧬 Code graph analysis (2)
clearnode/session_key.go (1)
clearnode/rpc.go (1)
RPCErrorf(147-151)
clearnode/rpc_router_auth.go (2)
clearnode/session_key.go (4)
CheckSessionKeyExists(126-140)SessionKey(18-30)SessionKey(32-34)AddSessionKey(62-123)clearnode/auth.go (1)
Policy(47-54)
⏰ 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: 2
♻️ Duplicate comments (1)
clearnode/rpc_router_auth.go (1)
233-244: Critical: Cache-only check causes reliability and logic issues.The call to
CheckSessionKeyExistsis missing the database handle and only checks the in-memory cache, which can lead to incorrect results and subsequent database constraint violations. This was already flagged in a previous review.Additionally, there's a logic flaw:
CheckSessionKeyExistsreturns an error when a session key exists but is expired ("session key already exists but is expired"). This error propagates and blocks authentication, preventing creation of a new session key for an expired one. The intended behavior should allow creating a new session key when the old one is expired.Recommended fix:
- Pass the database handle to
CheckSessionKeyExistsand query the database, not just the cache- Modify the logic to allow session key creation when the existing key is expired:
exists, err := CheckSessionKeyExists(challenge.Address, challenge.SessionKey) if err != nil { - logger.Error("failed to check existing session key", "error", err, "sessionKey", challenge.SessionKey) - return nil, nil, err + // Check if error is due to expiration - if so, allow creation of new key + if strings.Contains(err.Error(), "expired") { + exists = false + } else { + logger.Error("failed to check existing session key", "error", err, "sessionKey", challenge.SessionKey) + return nil, nil, err + } } if !exists { if err := AddSessionKey(r.DB, challenge.Address, challenge.SessionKey, challenge.Application, challenge.Scope, challenge.Allowances, claims.Policy.ExpiresAt); err != nil {Or better yet, modify
CheckSessionKeyExiststo return(false, nil)for expired keys instead of an error, treating them as effectively non-existent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/cerebro/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
clearnode/auth.go(1 hunks)clearnode/auth_test.go(2 hunks)clearnode/rpc_router_auth.go(2 hunks)examples/cerebro/clearnet/auth.go(2 hunks)examples/cerebro/go.mod(2 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_auth.goexamples/cerebro/clearnet/auth.go
🧬 Code graph analysis (3)
clearnode/auth.go (1)
clearnode/session_key.go (2)
SessionKey(18-30)SessionKey(32-34)
clearnode/rpc_router_auth.go (2)
clearnode/auth.go (2)
AuthManager(30-40)Policy(47-54)clearnode/session_key.go (4)
SessionKey(18-30)SessionKey(32-34)CheckSessionKeyExists(126-140)AddSessionKey(62-123)
clearnode/auth_test.go (1)
clearnode/pkg/rpc/api.go (1)
Allowance(570-575)
⏰ 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)
examples/cerebro/clearnet/auth.go (1)
6-6: LGTM! Usingmath.MaxUint64for unlimited expiration is clear.The change from time-based expiration to
math.MaxUint64as a sentinel value for "no expiration" is appropriate for example code and aligns well with the updated JWT generation flow.Also applies to: 12-14, 35-35
clearnode/auth_test.go (1)
86-86: LGTM! Test updates correctly match the new signature.The tests properly pass the
sessionKeyExpiresAtparameter with a reasonable 1-hour expiration, aligning with the updatedGenerateJWTmethod signature.Also applies to: 117-117
clearnode/rpc_router_auth.go (1)
221-221: LGTM! Correctly passes session key expiration.The call to
GenerateJWTnow correctly passeschallenge.SessionKeyExpiresAt, aligning with the updated method signature.
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 (7)
clearnode/chart/templates/helpers/_common.tpl(1 hunks)clearnode/config.go(4 hunks)clearnode/pkg/rpc/api.go(1 hunks)clearnode/pkg/rpc/client.go(1 hunks)clearnode/pkg/rpc/client_test.go(1 hunks)clearnode/rpc_router.go(2 hunks)docker-compose.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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.goclearnode/pkg/rpc/api.goclearnode/pkg/rpc/client.go
🧬 Code graph analysis (3)
clearnode/rpc_router.go (3)
clearnode/pkg/rpc/node.go (1)
Node(33-53)clearnode/rpc_node.go (1)
RPCContext(252-266)clearnode/config.go (2)
Config(26-33)ModeTest(16-16)
clearnode/pkg/rpc/client_test.go (4)
clearnode/pkg/rpc/api.go (1)
CleanupSessionKeyCacheMethod(112-112)clearnode/pkg/rpc/payload.go (2)
Params(177-177)NewPayload(53-64)clearnode/pkg/rpc/mock_dialer_test.go (1)
MockNotificationPublisher(16-16)clearnode/pkg/rpc/message.go (2)
Response(88-94)NewResponse(111-116)
clearnode/pkg/rpc/client.go (2)
clearnode/signer.go (1)
Signature(14-14)clearnode/pkg/rpc/api.go (1)
CleanupSessionKeyCacheMethod(112-112)
⏰ 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 (10)
clearnode/chart/templates/helpers/_common.tpl (1)
74-75: LGTM: Production mode set by default.The addition of CLEARNODE_MODE with a production default aligns with the new mode handling in config.go and provides clear runtime behavior.
clearnode/config.go (2)
12-17: LGTM: Clean mode type declaration.The Mode type with production and test constants provides type safety and clear semantics for the application's runtime mode.
51-57: LGTM: Robust mode configuration with proper validation.The implementation correctly:
- Defaults to production mode when unset
- Validates only allowed values (production/test)
- Logs the chosen mode for observability
- Uses Fatal for invalid configuration (appropriate for startup validation)
docker-compose.yml (1)
164-164: LGTM: Test mode enabled for local development.Setting CLEARNODE_MODE to test in docker-compose enables test-mode RPC endpoints for local development and testing, which is appropriate for this environment.
clearnode/pkg/rpc/api.go (1)
111-112: LGTM: Test-mode cleanup method constant added.The new CleanupSessionKeyCacheMethod constant follows the existing naming convention and is clearly documented as test-mode only.
clearnode/pkg/rpc/client_test.go (1)
539-553: LGTM: Well-structured test for cleanup RPC method.The test follows the established pattern in this file:
- Uses parallel execution
- Registers a mock handler that returns success
- Validates both error absence and empty signature response
- Provides good coverage for the happy path
clearnode/rpc_router.go (3)
78-80: LGTM: Test-mode group properly configured.The test_mode route group correctly applies TestModeMiddleware to gate test-only endpoints and registers the cleanup handler.
247-254: LGTM: Test-mode middleware properly gates test endpoints.The middleware correctly checks the configuration mode and fails requests with a clear error message when not in test mode, preventing accidental use of test-only functionality in production.
256-259: LGTM: Simple and effective cache cleanup handler.The handler clears the session key cache and returns success. The implementation is straightforward and appropriate for a test-mode utility endpoint.
clearnode/pkg/rpc/client.go (1)
1265-1283: LGTM: Client method follows established patterns.The CleanupSessionKeyCache method:
- Has clear documentation indicating test-mode only usage
- Follows the same pattern as other simple RPC methods (e.g., Ping)
- Properly handles errors and returns signatures
- Provides a helpful usage example
ee5d815 to
174bead
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
clearnode/pkg/rpc/client.go (1)
1285-1306: Excellent refactoring!The refactored
callmethod improves code organization by extracting payload preparation and request construction into dedicated helper methods (PreparePayloadandNewRequest). This makes the flow clearer, reduces duplication, and improves maintainability. Error handling is properly maintained at each step.clearnode/config.go (2)
12-17: Add documentation comments for the public Mode type.The
Modetype and its constants are public but lack documentation comments. Consider adding godoc comments to explain their purpose and usage.Apply this diff to add documentation:
+// Mode represents the application runtime mode type Mode string const ( + // ModeProduction is the default production runtime mode ModeProduction Mode = "production" + // ModeTest enables test-specific features and behaviors ModeTest Mode = "test" )
27-27: Mode field is properly accessible within the package; adding a getter is optional.The
modefield is correctly accessed atrpc_router.go:248viar.Config.mode, which is valid since bothconfig.goandrpc_router.goare in the samemainpackage. This follows the existing pattern used for other Config fields likeassetsandmsgExpiryTime.A getter method is not required for functionality but would improve encapsulation as a style enhancement if desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
clearnode/chart/templates/helpers/_common.tpl(1 hunks)clearnode/config.go(4 hunks)clearnode/pkg/rpc/api.go(1 hunks)clearnode/pkg/rpc/client.go(1 hunks)clearnode/pkg/rpc/client_test.go(1 hunks)clearnode/rpc_router.go(2 hunks)docker-compose.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- clearnode/chart/templates/helpers/_common.tpl
- clearnode/rpc_router.go
- clearnode/pkg/rpc/api.go
- docker-compose.yml
- clearnode/pkg/rpc/client_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 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/pkg/rpc/client.go
🧬 Code graph analysis (1)
clearnode/pkg/rpc/client.go (3)
clearnode/testing/client.go (1)
Client(149-160)clearnode/signer.go (1)
Signature(14-14)clearnode/pkg/rpc/api.go (1)
CleanupSessionKeyCacheMethod(112-112)
⏰ 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/pkg/rpc/client.go (1)
1265-1283: LGTM! Clean test utility implementation.The new
CleanupSessionKeyCachemethod follows the established pattern for simple RPC methods and is appropriately documented as test-mode only. The implementation correctly delegates toc.callwith no request parameters and returns the signatures.clearnode/config.go (2)
51-57: LGTM! Mode loading and validation logic is sound.The implementation correctly:
- Defaults to production mode when
CLEARNODE_MODEis unset (secure default)- Validates that only allowed modes are accepted
- Uses
logger.Fatalfor configuration errors (appropriate for startup failures)- Provides observability with an info log
107-107: LGTM! Mode field assignment is correct.The mode is properly stored in the Config struct during initialization.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
clearnode/docs/API.md (2)
30-30: Consider moving endpoint documentation to "Utility Methods" section for better discoverability.The new
cleanup_session_key_cacheendpoint is documented under "Error Handling" (line 1824), but it's a utility endpoint similar to Ping. Placing it in the "Utility Methods" section (around line 1501) would make it easier for users to locate test-mode utilities.- Move the entire "### Cleanup Session Key Cache" section (lines 1824-1844) to after the "Ping" section and before "Balance Updates" in the Utility Methods section. - Update line numbers accordingly for documentation flow.Also applies to: 1824-1844
1824-1844: Expand endpoint documentation with purpose and usage context.The documentation is minimal and doesn't clearly explain when or why this endpoint should be used. Consider adding a brief description of the cache cleanup purpose and any side effects.
### Cleanup Session Key Cache - Cleans up the session key cache for everyone, which contains information about session keys. + Clears the local session key cache. This is a test-mode utility endpoint that resets cached session key information + on the server. Use this endpoint during testing to ensure session key state is fresh. Note: This endpoint only + functions when the Clearnode instance is running in TEST mode.integration/tests/resize_channel.test.ts (1)
119-123: Consider refactoring all tests to use thegetLedgerBalanceshelper.While the refactoring in this test is good, the remaining tests (starting at lines 126, 195, etc.) still use the manual RPC approach at lines 142-148, 187-192, 211-216, and 257-262. For consistency and maintainability, consider applying the same refactoring pattern across all test cases in this file.
sdk/src/rpc/parse/parse.ts (1)
89-90: Add missing semicolon for consistency.Line 90 is missing a trailing semicolon, which is inconsistent with the coding style used throughout this file (see lines 60, 63, 66, 69, 72, etc.).
Apply this diff to add the semicolon:
/** Parses `get_session_keys` response */ -export const parseGetSessionKeysResponse = (raw: string) => _parseSpecificRPCResponse(raw, RPCMethod.GetSessionKeys) +export const parseGetSessionKeysResponse = (raw: string) => _parseSpecificRPCResponse(raw, RPCMethod.GetSessionKeys);sdk/src/rpc/types/request.ts (1)
318-324: LGTM! Consider enhancing documentation.The interface definition is correct and follows the established pattern for RPC methods with empty parameters.
Given the PR context mentions this is for test-mode cache cleanup, consider adding a
@remarkstag to clarify the intended use case (e.g., testing/debugging scenarios).integration/tests/session_key.test.ts (2)
78-81: Consider localizing multi-asset authentication to the test suite that needs it.The main
beforeEachnow grants both USDC and ETH allowances to all tests in this file, but only the "Multi-asset spending caps" suite actually uses ETH. Most other tests only exercise USDC, and some (like "Float amounts" and "Session key revocation") explicitly re-authenticate with their own allowances.For better test isolation and clarity, consider moving the multi-asset authentication to the "Multi-asset spending caps" suite's own
beforeEachand keeping the mainbeforeEachwith single-asset (USDC-only) authentication.- await authenticateAppWithMultiAssetAllowances(aliceAppWS, aliceAppIdentity, [ - { asset: ASSET_SYMBOL, amount: spendingCapAmount.toString()}, - { asset: ETH_ASSET_SYMBOL, amount: ETH_CAP.toString() }, - ]); + await authenticateAppWithAllowances( + aliceAppWS, + aliceAppIdentity, + ASSET_SYMBOL, + spendingCapAmount.toString() + );Then add to the "Multi-asset spending caps" suite's
beforeEach:beforeEach(async () => { // Re-authenticate with multi-asset allowances for this suite await authenticateAppWithMultiAssetAllowances(aliceAppWS, aliceAppIdentity, [ { asset: ASSET_SYMBOL, amount: spendingCapAmount.toString()}, { asset: ETH_ASSET_SYMBOL, amount: ETH_CAP.toString() }, ]); // Create WETH channel for Alice to have ETH in ledger await aliceClient.createAndWaitForChannel(aliceWS, { tokenAddress: CONFIG.ADDRESSES.WETH_TOKEN_ADDRESS, amount: toRaw(BigInt(10), 18), // 10 WETH }); });
450-594: Consider breaking down this complex test into smaller, focused tests.This test validates multiple scenarios across two sessions and two assets in a single test case with seven distinct steps. While the logic is correct, the complexity makes it harder to:
- Understand what specific behavior is being tested at each step
- Debug failures (a failure at step 5 requires understanding steps 1-4)
- Maintain when requirements change
Consider splitting into separate tests such as:
- "should allow USDC deposits up to cap across sessions"
- "should allow ETH deposits up to cap across sessions"
- "should reject USDC deposits exceeding cap"
- "should reject ETH deposits exceeding cap"
This would make each test more focused and easier to understand, though it may result in some test setup duplication.
integration/tests/nitrorpc_v04.test.ts (1)
23-23: LGTM! Clear and well-sized allowance.The new constant provides a descriptive name and adequate allowance (5x the app session deposit) for session key operations across multiple tests.
Optionally, consider adding a brief comment explaining the constant:
+// Session key allowance for authentication - sized to support multiple app session operations const SKAllowanceAmount = BigInt(500);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
clearnode/docs/API.md(48 hunks)integration/common/databaseUtils.ts(2 hunks)integration/common/ws.ts(2 hunks)integration/tests/lifecycle_nitrorpc_v02.test.ts(2 hunks)integration/tests/nitrorpc_v04.test.ts(2 hunks)integration/tests/resize_channel.test.ts(2 hunks)integration/tests/session_key.test.ts(7 hunks)sdk/src/rpc/README.md(2 hunks)sdk/src/rpc/api.ts(2 hunks)sdk/src/rpc/parse/index.ts(1 hunks)sdk/src/rpc/parse/parse.ts(2 hunks)sdk/src/rpc/types/index.ts(2 hunks)sdk/src/rpc/types/request.ts(4 hunks)sdk/src/rpc/types/response.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-05T10:19:06.155Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Applied to files:
integration/tests/lifecycle_nitrorpc_v02.test.tsintegration/tests/resize_channel.test.tsclearnode/docs/API.md
📚 Learning: 2025-11-04T19:14:48.960Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:542-553
Timestamp: 2025-11-04T19:14:48.960Z
Learning: In clearnode/custody.go's handleClosed function, when unlocking escrow funds on channel close, the code intentionally passes the channelEscrowAccountBalance (which may be negative) directly to ledger.Record for the wallet account. When the escrow balance is negative (e.g., resize confirmation received for a channel not in resizing state), this causes a debit to the unified wallet account, which is the expected behavior to equalize accounting. Temporary negative unified balances in this scenario are acceptable.
Applied to files:
integration/tests/lifecycle_nitrorpc_v02.test.tsintegration/tests/resize_channel.test.ts
📚 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/docs/API.mdsdk/src/rpc/api.ts
📚 Learning: 2025-11-04T10:39:28.297Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:425-427
Timestamp: 2025-11-04T10:39:28.297Z
Learning: In clearnode/custody.go's handleResized function, when a resize event is received for a channel with status ChannelStatusOpen (not ChannelStatusResizing), the code intentionally logs an error but continues processing the resize operation normally. This is the expected behavior.
Applied to files:
clearnode/docs/API.md
🧬 Code graph analysis (9)
integration/tests/nitrorpc_v04.test.ts (1)
integration/common/testHelpers.ts (1)
authenticateAppWithAllowances(54-74)
sdk/src/rpc/parse/index.ts (1)
sdk/src/rpc/parse/common.ts (1)
noop(52-54)
sdk/src/rpc/types/index.ts (2)
clearnode/testing/client.go (1)
RPCData(39-44)clearnode/pkg/sign/sign.go (1)
Address(29-34)
integration/common/databaseUtils.ts (3)
integration/common/ws.ts (2)
TestWebSocket(9-139)getCleanupSessionKeyCachePredicate(284-288)integration/common/setup.ts (1)
CONFIG(4-44)sdk/src/rpc/api.ts (2)
createECDSAMessageSigner(839-850)createCleanupSessionKeyCacheMessage(752-766)
sdk/src/rpc/types/request.ts (1)
sdk/src/rpc/types/index.ts (1)
GenericRPCMessage(25-29)
integration/tests/resize_channel.test.ts (3)
integration/common/testHelpers.ts (1)
getLedgerBalances(158-173)integration/common/setup.ts (1)
CONFIG(4-44)sdk/src/rpc/api.ts (1)
createResizeChannelMessage(579-594)
sdk/src/rpc/types/response.ts (1)
sdk/src/rpc/types/index.ts (1)
GenericRPCMessage(25-29)
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 (2)
integration/common/testHelpers.ts (3)
authenticateAppWithMultiAssetAllowances(79-93)toRaw(21-23)createTestAppSession(98-151)integration/common/setup.ts (1)
CONFIG(4-44)
⏰ 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 (22)
clearnode/docs/API.md (2)
32-38: LGTM!The new "Legenda" section clearly documents the three access levels and their meanings. This is helpful context for understanding which endpoints require authentication.
5-30: LGTM!The endpoint table update is clear and consistent. The new endpoint is properly positioned alphabetically and labeled with the "Test" access level.
integration/tests/lifecycle_nitrorpc_v02.test.ts (2)
209-209: LGTM! Test description improved.Specifying "Alice" in the test description improves clarity and aligns with the test implementation.
271-271: LGTM! Test description improved.Specifying "Bob" in the test description improves clarity and maintains consistency with the Alice test description change.
integration/tests/resize_channel.test.ts (3)
64-68: Good refactoring to use the helper function.The refactoring to use
getLedgerBalancesimproves code maintainability and reduces duplication. The addition of asset symbol verification at line 67 is also a good defensive check.However, note that the
String()conversion has been removed from the amount assertion (line 68 vs. line 147 in the unchanged test). Verify that theamountfield inRPCBalanceis already typed as a string to ensure type compatibility.
70-71: Verify the "custody balance" terminology aligns with the method semantics.The variable has been renamed from
preResizeAccountBalancetopreResizeCustodyBalance, but the method being called is stillclient.getAccountBalance(). Ensure that "custody balance" accurately describes what this method returns to avoid confusion in the test.
73-78: Addition offunds_destinationfield looks correct.The
funds_destinationfield is appropriately set toidentity.walletAddressand aligns with the resize channel API requirements shown in other tests.integration/common/ws.ts (2)
242-246: LGTM!The new predicate helper follows the established pattern and correctly uses the
GetSessionKeysRPC method.
284-288: LGTM!The new predicate helper is consistent with existing patterns and correctly detects the
CleanupSessionKeyCacheRPC method.sdk/src/rpc/README.md (1)
20-22: LGTM!Documentation correctly reflects the new RPC functions added to the API.
sdk/src/rpc/parse/index.ts (1)
21-21: LGTM!Correctly maps the
CleanupSessionKeyCachemethod to thenoopparser, consistent with other parameter-less methods likePingandPong.sdk/src/rpc/types/response.ts (1)
348-354: LGTM!The new response types for
CleanupSessionKeyCachefollow the established patterns and are properly integrated into the type system.Also applies to: 517-520, 557-558
sdk/src/rpc/types/index.ts (1)
111-111: LGTM!The additions are well-structured:
- The
MultiMessageVerifiertype correctly defines the signature for multi-party verification- The
CleanupSessionKeyCacheenum member properly extends the RPC method set- Minor formatting change to
SingleMessageVerifierimproves readabilityAlso applies to: 121-125, 199-199
sdk/src/rpc/parse/parse.ts (1)
151-153: LGTM!The parser correctly follows the established pattern for handling
CleanupSessionKeyCacheresponses.integration/common/databaseUtils.ts (1)
50-65: Random signer usage in test mode is correct—no changes needed.The verification confirms the
cleanupSessionKeyCacheendpoint is properly gated to test mode. The endpoint is registered intestModeGroupwithTestModeMiddleware(clearnode/rpc_router.go:80), and the handler implementation doesn't validate signatures by design (clearnode/rpc_router.go:256-259). Using a random signer in the test code is appropriate for this test-mode-only utility endpoint, as documented in the API comments and client methods (clearnode/pkg/rpc/api.go:111-112, clearnode/pkg/rpc/client.go:1265-1266).sdk/src/rpc/types/request.ts (3)
481-484: LGTM!The params type alias correctly extracts the parameters from the request interface, maintaining consistency with the established pattern throughout the file.
515-516: LGTM!The new request type is correctly added to the
RPCRequestunion, enabling type-safe handling of cleanup requests across the RPC system.
546-546: LGTM!The mapping correctly associates the cleanup method with its parameter type, completing the type-safe integration of the new RPC method.
integration/tests/session_key.test.ts (3)
33-34: LGTM! Clear ETH test constants.The ETH asset symbol and spending cap constants are well-defined and consistent with the existing USDC constants pattern.
444-447: LGTM! Proper ETH channel setup.The WETH channel creation correctly uses 18 decimals and provides sufficient balance (10 WETH) for the multi-asset spending cap tests (which only need 2 ETH).
603-603: Re-authentication is intentional for test isolation.This line re-authenticates with single-asset allowances, overriding the multi-asset allowances set in the main
beforeEach. This ensures the revocation tests have a predictable, controlled authentication state.Note: If the multi-asset authentication is moved to the "Multi-asset spending caps" suite per the earlier suggestion, this line would become redundant and could be removed.
integration/tests/nitrorpc_v04.test.ts (1)
75-75: Re-authentication will succeed but won't update the allowance amount.The session key existence check allows re-authentication to succeed without error. However, when an existing session key is detected, the new allowance amount is not stored—the original 500 allowance from line 75 is retained. For this specific test, this may not cause failures since the v0.2 app session uses its own deposit amount. Verify whether the test expectations align with this behavior or if the allowance should be explicitly reset between authentications.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
clearnode/session_key.go (2)
129-159: Inconsistent cache management and expired key handling prevents re-authentication.This function has two design issues:
Lazy purge inconsistency: Unlike
GetWalletBySessionKey(line 173), which removes expired entries from the cache when found, this function returns an error without cleanup, causing expired keys to accumulate in the cache indefinitely.Expired keys block re-authentication: When an expired session key is found for the same wallet,
ErrSessionKeyExistsAndExpiredis returned. Inrpc_router_auth.go, this error preventsAddSessionKeyfrom being called, blocking users from re-authenticating with the same session key address. SinceAddSessionKeydeletes by wallet+application before inserting, it would successfully replace the expired key if reached.Based on the past review discussion where dimast-x confirmed this is important, consider one of these solutions:
Option 1 (Recommended): Treat expired keys for the same wallet as non-existent, allowing replacement:
func CheckSessionKeyExists(db *gorm.DB, walletAddress, sessionKeyAddress string) (bool, error) { if w, ok := sessionKeyCache.Load(sessionKeyAddress); ok { - entry := w.(sessionKeyCacheEntry) + entry, ok := w.(sessionKeyCacheEntry) + if !ok { + sessionKeyCache.Delete(sessionKeyAddress) + return false, nil + } if entry.wallet == walletAddress { if !isExpired(entry.expiresAt) { return true, nil } - - return false, ErrSessionKeyExistsAndExpired + // Expired key for same wallet - allow replacement + sessionKeyCache.Delete(sessionKeyAddress) // Lazy purge + return false, nil } return false, ErrSignedUsedForAnotherWallet } - // Not in cache - check DB for expired keys with this address - // This handles keys that expired before server restart var existingKey SessionKey if err := db.Where("address = ?", sessionKeyAddress).First(&existingKey).Error; err == nil { - if isExpired(existingKey.ExpiresAt) { - return false, ErrSessionKeyExistsAndExpired - } - // Non-expired key found - should have been in cache but wasn't - // Return error to prevent conflict if existingKey.WalletAddress == walletAddress { + if isExpired(existingKey.ExpiresAt) { + // Expired key for same wallet - allow replacement + return false, nil + } return true, nil } return false, ErrSignedUsedForAnotherWallet } return false, nil }Option 2: Keep
ErrSessionKeyExistsAndExpiredfor monitoring but handle it specially in the caller (see my comment inclearnode/rpc_router_auth.go).
132-132: Add safety check for type assertion.The unchecked type assertion
entry := w.(sessionKeyCacheEntry)will panic if the cache contains an unexpected type. Use the two-value form for safety.Apply this diff:
if w, ok := sessionKeyCache.Load(sessionKeyAddress); ok { - entry := w.(sessionKeyCacheEntry) + entry, ok := w.(sessionKeyCacheEntry) + if !ok { + sessionKeyCache.Delete(sessionKeyAddress) // Remove invalid entry + return false, nil + } if entry.wallet == walletAddress {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clearnode/rpc_router_auth.go(2 hunks)clearnode/session_key.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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_auth.goclearnode/session_key.go
🧬 Code graph analysis (2)
clearnode/rpc_router_auth.go (2)
clearnode/auth.go (2)
AuthManager(30-40)Policy(47-54)clearnode/session_key.go (4)
SessionKey(23-35)SessionKey(37-39)CheckSessionKeyExists(130-159)AddSessionKey(67-127)
clearnode/session_key.go (2)
clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/pkg/rpc/error.go (1)
Error(48-50)
⏰ 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 (4)
clearnode/rpc_router_auth.go (1)
221-221: LGTM!The addition of
challenge.SessionKeyExpiresAtto theGenerateJWTcall correctly propagates session key expiration into the JWT claims, aligning with the updated signature.clearnode/session_key.go (3)
17-20: LGTM!The error variables are well-defined with clear, descriptive messages.
67-76: LGTM!Moving expiration validation outside the transaction with UTC conversion and early validation is a good practice that reduces unnecessary transaction overhead.
78-79: LGTM!The
deletedAddressestracking correctly implements the pattern of collecting changes during the transaction and applying cache updates only after successful commit, ensuring cache-DB consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
clearnode/session_key.go (3)
132-132: Add safety check for type assertion.The unchecked type assertion will panic if the cache contains an unexpected type. Use the two-value form to handle this safely.
Apply this diff:
if w, ok := sessionKeyCache.Load(sessionKeyAddress); ok { - entry := w.(sessionKeyCacheEntry) + entry, ok := w.(sessionKeyCacheEntry) + if !ok { + // Invalid cache entry, remove it and continue to DB check + sessionKeyCache.Delete(sessionKeyAddress) + // Fall through to DB check below + } else { if entry.wallet == walletAddress { if !isExpired(entry.expiresAt) { return true, nil } return false, ErrSessionKeyExistsAndExpired } return false, ErrSignedUsedForAnotherWallet + } }
131-141: Expired keys should allow re-creation, not block it.Returning
ErrSessionKeyExistsAndExpired(line 138) prevents users from re-authenticating after their session key expires. The expected behavior is to allow the expired key to be replaced with a new one.Additionally, unlike
GetWalletBySessionKey(line 173), this function doesn't perform lazy purge of expired cache entries, causing them to accumulate indefinitely.Apply this diff to fix both issues:
if w, ok := sessionKeyCache.Load(sessionKeyAddress); ok { entry, ok := w.(sessionKeyCacheEntry) if !ok { sessionKeyCache.Delete(sessionKeyAddress) } else if entry.wallet == walletAddress { if !isExpired(entry.expiresAt) { return true, nil } - - return false, ErrSessionKeyExistsAndExpired + // Expired key found - perform lazy purge and allow re-creation + sessionKeyCache.Delete(sessionKeyAddress) + // Fall through to DB check to handle the expired key properly } else { return false, ErrSignedUsedForAnotherWallet } }
143-159: Handle expired DB keys and check for DB errors properly.Two issues with the DB fallback logic:
Expired key handling (lines 147-149): Returning
ErrSessionKeyExistsAndExpiredblocks re-authentication. The key should be deleted from the DB and the function should returnfalse, nilto allowAddSessionKeyto create a new one.Silent error swallowing (line 146): The code only handles the success case (
err == nil). If the query fails with an error other thangorm.ErrRecordNotFound, it silently falls through to returnfalse, nil, masking database connectivity or other issues.Apply this diff:
+ "errors" + "gorm.io/gorm"// Not in cache - check DB for expired keys with this address // This handles keys that expired before server restart var existingKey SessionKey - if err := db.Where("address = ?", sessionKeyAddress).First(&existingKey).Error; err == nil { + err := db.Where("address = ?", sessionKeyAddress).First(&existingKey).Error + if err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { + return false, nil // Key does not exist, safe to create + } + return false, fmt.Errorf("failed to check session key in database: %w", err) + } + + // Key exists in DB + if existingKey.WalletAddress != walletAddress { + return false, ErrSignerUsedForAnotherWallet + } + if isExpired(existingKey.ExpiresAt) { - return false, ErrSessionKeyExistsAndExpired + // Expired key for this wallet - delete it and allow re-creation + if err := db.Delete(&existingKey).Error; err != nil { + return false, fmt.Errorf("failed to delete expired session key: %w", err) } - // Non-expired key found - should have been in cache but wasn't - // Return error to prevent conflict - if existingKey.WalletAddress == walletAddress { - return true, nil - } - return false, ErrSignerUsedForAnotherWallet + return false, nil // Allow re-creation } - return false, nil + // Non-expired key exists for this wallet + return true, nilNote: This requires importing
"errors"package.Based on learnings and past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clearnode/session_key.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/session_key.go
🧬 Code graph analysis (1)
clearnode/session_key.go (2)
clearnode/rpc.go (1)
RPCErrorf(147-151)clearnode/pkg/rpc/error.go (1)
Error(48-50)
⏰ 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: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
clearnode/session_key.go (2)
68-76: LGTM: Early expiration validation.Moving the expiration validation before the transaction and ensuring UTC conversion is a good practice that prevents unnecessary DB operations for invalid inputs.
78-127: LGTM: Proper cache-transaction synchronization.The pattern of collecting
deletedAddressesduring the transaction and updating the cache only after successful commit ensures cache-DB consistency.
85fa6db to
5f77830
Compare
Summary by CodeRabbit
Bug Fixes
Behavior Changes
New Features
Documentation
Tests