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 WebSocket RPC stack: connection primitives and hub, a WebsocketNode server with routing/groups/middleware and signed responses, a Context middleware framework with SafeStorage, a mock signer, dialer context refinements, and comprehensive unit/integration tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant HTTPServer as WebsocketNode
participant Conn as WebsocketConnection
participant Hub as ConnectionHub
Client->>HTTPServer: HTTP Upgrade -> WebSocket
HTTPServer->>Conn: NewWebsocketConnection(...) and register
HTTPServer->>Hub: Add(conn)
Note over Conn: readMessages / writeMessages goroutines
Client-->>Conn: send JSON request
Conn-->>HTTPServer: enqueue raw request (RawRequests)
HTTPServer->>HTTPServer: build Context, run middleware/handler
alt auth changes
HTTPServer->>Hub: Reauthenticate(connID, newUserID)
HTTPServer->>HTTPServer: OnAuthenticated hook
end
HTTPServer->>Conn: Write signed response
HTTPServer->>Hub: Publish(userID, notification)
Hub->>Conn: deliver notification(s)
Conn->>Hub: Remove(connID) on close
HTTPServer->>HTTPServer: OnDisconnect hook
sequenceDiagram
autonumber
participant Context
participant Middleware01 as Middleware[0]
participant Middleware02 as Middleware[1]
participant Handler
Context->>Middleware01: invoke
Middleware01-->>Context: call Next()
Context->>Middleware02: invoke
Middleware02-->>Context: call Next()
Context->>Handler: invoke final handler
alt success
Handler-->>Context: Succeed(method, params)
Context->>Context: prepareRawResponse (sign & marshal)
else failure
Handler-->>Context: Fail(err, fallback)
Context->>Context: prepareRawResponse (error payload)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)clearnode/pkg/rpc/connection.go (2)
clearnode/pkg/rpc/node.go (7)
⏰ 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). (4)
🔇 Additional comments (13)
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 @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 delivers a foundational WebSocket-based RPC node, designed for robust and scalable communication. It establishes a structured approach to handling client connections, processing remote procedure calls, and managing user authentication. The implementation emphasizes modularity through middleware and handler groups, offering a flexible framework for extending RPC functionalities while ensuring message authenticity via cryptographic signing. 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 a comprehensive RPC node implementation for WebSocket communication. The changes include connection management, a hub for tracking connections, a routing system with support for middleware and handler groups, and event handling for connection lifecycle. The code is well-structured, but there are several important issues to address. These include a critical bug in context propagation that could lead to resource leaks, a high-severity security flaw in WebSocket origin checking, and several concurrency and resource management improvements in the connection handling logic. The tests are thorough but could be improved by avoiding global state modification and flaky time.Sleep calls.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
clearnode/pkg/rpc/connection.go(1 hunks)clearnode/pkg/rpc/connection_test.go(1 hunks)clearnode/pkg/rpc/node.go(1 hunks)clearnode/pkg/rpc/node_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
clearnode/pkg/rpc/connection_test.go (2)
clearnode/pkg/log/noop_logger.go (1)
NewNoopLogger(12-14)clearnode/pkg/rpc/connection.go (2)
Connection(15-36)NewConnectionHub(213-218)
clearnode/pkg/rpc/node.go (5)
clearnode/pkg/rpc/connection.go (3)
ConnectionHub(202-209)NewConnection(39-55)Connection(15-36)clearnode/pkg/rpc/message.go (3)
Request(20-26)Response(88-94)NewErrorResponse(161-165)clearnode/pkg/rpc/error.go (2)
Error(49-51)Errorf(72-76)clearnode/pkg/rpc/payload.go (3)
Params(177-177)NewPayload(53-64)Payload(21-41)clearnode/nitrolite/signature.go (1)
Sign(57-78)
clearnode/pkg/rpc/node_test.go (5)
clearnode/pkg/sign/eth_signer.go (1)
NewEthereumSigner(86-96)clearnode/pkg/log/zap_logger.go (1)
NewZapLogger(34-79)clearnode/pkg/rpc/node.go (3)
NewNode(62-85)SendResponseFunc(224-224)Context(228-244)clearnode/pkg/rpc/payload.go (3)
NewParams(205-219)Params(177-177)NewPayload(53-64)clearnode/pkg/rpc/message.go (2)
Response(88-94)NewRequest(42-47)
clearnode/pkg/rpc/connection.go (1)
clearnode/pkg/rpc/node.go (1)
Context(228-244)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clearnode/pkg/rpc/dialer.go (1)
128-135: ProtectclosureErrfrom concurrent writes.
childHandleClosureruns from multiple goroutines, mutatingclosureErrwithout synchronization. That’s a data race and violates the concurrency requirements.Apply this diff to guard the assignment:
- // Track the first error that causes closure - var closureErr error + // Track the first error that causes closure + var ( + closureErr error + closureErrMu sync.Mutex + ) childHandleClosure := func(err error) { - if err != nil && closureErr == nil { - closureErr = err + if err != nil { + closureErrMu.Lock() + if closureErr == nil { + closureErr = err + } + closureErrMu.Unlock() }
♻️ Duplicate comments (4)
clearnode/pkg/rpc/connection_test.go (1)
59-62: Do not mutate package globals in tests.Tweaking
defaultResponseWriteDurationin-place can leak into other tests (especially if any ever callt.Parallel()), which was already called out earlier. Please make the timeout configurable on the connection under test instead of patching the global.clearnode/pkg/rpc/node_test.go (1)
291-292: Replace fixed sleep with deterministic message wait.
time.Sleep(100 * time.Millisecond)is the same flakiness hotspot previously flagged; it can race under slower CI. Please remove the sleep and wait for the nextreceive(t)(or a signal) to ensure the notification arrived.clearnode/pkg/rpc/connection.go (1)
91-112: WaitGroup count must cover all three goroutines.
ServelaunchesreadMessages,writeMessages, andwaitForConnClose, all of which invokechildHandleClosure. Withwg.Add(2)the counter goes negative, triggeringsync: negative WaitGroup counter. Bump the count to three before starting the goroutines.wg := &sync.WaitGroup{} - wg.Add(2) + wg.Add(3)clearnode/pkg/rpc/node.go (1)
83-88: Do not disable WebSocket origin checks.
CheckOriginreturningtrueopens the server to cross-site WebSocket hijacking. Replace this with an allowlist (or configurable validator) so only trusted origins are accepted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
clearnode/pkg/rpc/connection.go(1 hunks)clearnode/pkg/rpc/connection_hub.go(1 hunks)clearnode/pkg/rpc/connection_test.go(1 hunks)clearnode/pkg/rpc/context.go(1 hunks)clearnode/pkg/rpc/dialer.go(3 hunks)clearnode/pkg/rpc/node.go(1 hunks)clearnode/pkg/rpc/node_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
clearnode/pkg/rpc/connection_hub.go (1)
clearnode/pkg/rpc/connection.go (1)
Connection(16-29)
clearnode/pkg/rpc/connection.go (1)
clearnode/pkg/rpc/context.go (1)
Context(22-38)
clearnode/pkg/rpc/connection_test.go (3)
clearnode/pkg/log/noop_logger.go (1)
NewNoopLogger(12-14)clearnode/pkg/rpc/connection.go (1)
WebsocketConnection(33-56)clearnode/pkg/rpc/connection_hub.go (1)
NewConnectionHub(21-26)
clearnode/pkg/rpc/context.go (3)
clearnode/pkg/rpc/payload.go (3)
Params(177-177)NewPayload(53-64)Payload(21-41)clearnode/pkg/rpc/message.go (3)
Request(20-26)Response(88-94)NewErrorResponse(161-165)clearnode/pkg/rpc/error.go (2)
Error(49-51)Errorf(72-76)
clearnode/pkg/rpc/dialer.go (2)
clearnode/pkg/rpc/error.go (2)
Errorf(72-76)ErrDialingWebsocket(32-32)clearnode/pkg/log/context.go (1)
FromContext(35-40)
clearnode/pkg/rpc/node_test.go (6)
clearnode/pkg/sign/eth_signer.go (1)
NewEthereumSigner(86-96)clearnode/pkg/log/zap_logger.go (1)
NewZapLogger(34-79)clearnode/pkg/rpc/node.go (1)
NewWebsocketNode(81-104)clearnode/pkg/rpc/context.go (2)
SendResponseFunc(18-18)Context(22-38)clearnode/pkg/rpc/payload.go (3)
NewParams(205-219)Params(177-177)NewPayload(53-64)clearnode/pkg/rpc/message.go (2)
Response(88-94)NewRequest(42-47)
clearnode/pkg/rpc/node.go (5)
clearnode/pkg/rpc/context.go (4)
Handler(14-14)SendResponseFunc(18-18)Context(22-38)NewSafeStorage(150-154)clearnode/pkg/rpc/payload.go (2)
Params(177-177)NewPayload(53-64)clearnode/pkg/rpc/connection_hub.go (2)
ConnectionHub(10-17)NewConnectionHub(21-26)clearnode/pkg/rpc/message.go (2)
Request(20-26)NewErrorResponse(161-165)clearnode/pkg/rpc/connection.go (2)
NewWebsocketConnection(59-75)Connection(16-29)
🪛 Gitleaks (8.28.0)
clearnode/pkg/rpc/node_test.go
[high] 22-22: 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: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
clearnode/pkg/rpc/node_test.go (1)
6-8: Remove the static private key from source control.Even though the test is commented out, the literal Ethereum private key is still present in the repo and continues to trigger gitleaks. Please drop the hardcoded key and generate an ephemeral one at runtime (e.g., with
crypto.GenerateKey, hex-encode it, and prefix with0x) before constructing the signer.
🧹 Nitpick comments (2)
clearnode/pkg/rpc/connection.go (1)
14-21: Document the rationale for buffer size defaults.The default buffer sizes of 10 are not explained. Consider adding comments that justify these values (e.g., "balances memory usage with throughput for typical workloads" or "allows up to 10 concurrent messages before blocking").
Apply this diff to add explanatory comments:
var ( // defaultWsConnWriteTimeout is the default maximum duration to wait for a write to complete. defaultWsConnWriteTimeout = 5 * time.Second - // defaultWsConnProcessBufferSize is the default size of the buffer for processing incoming messages. + // defaultWsConnProcessBufferSize is the default size of the buffer for processing incoming messages. + // A size of 10 allows the connection to queue up to 10 inbound messages before blocking the reader. defaultWsConnProcessBufferSize = 10 - // defaultWsConnWriteBufferSize is the default size of the buffer for outgoing messages. + // defaultWsConnWriteBufferSize is the default size of the buffer for outgoing messages. + // A size of 10 allows the connection to queue up to 10 outbound messages before blocking the sender. defaultWsConnWriteBufferSize = 10 )clearnode/pkg/rpc/node.go (1)
119-129: Document default buffer size rationale.The default WebSocket buffer sizes (1024 bytes for read/write) are set without explanation. Adding comments to justify these values improves maintainability and helps operators decide if they need custom values for their workload.
Apply this diff:
if config.WsUpgraderReadBufferSize <= 0 { + // 1024 bytes is sufficient for typical JSON-RPC messages config.WsUpgraderReadBufferSize = 1024 } if config.WsUpgraderWriteBufferSize <= 0 { + // 1024 bytes is sufficient for typical JSON-RPC messages config.WsUpgraderWriteBufferSize = 1024 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
clearnode/pkg/rpc/connection.go(1 hunks)clearnode/pkg/rpc/connection_hub.go(1 hunks)clearnode/pkg/rpc/connection_hub_test.go(1 hunks)clearnode/pkg/rpc/connection_test.go(1 hunks)clearnode/pkg/rpc/context.go(1 hunks)clearnode/pkg/rpc/context_internal_test.go(1 hunks)clearnode/pkg/rpc/dialer.go(3 hunks)clearnode/pkg/rpc/node.go(1 hunks)clearnode/pkg/rpc/node_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
clearnode/pkg/rpc/context_internal_test.go (4)
clearnode/pkg/rpc/context.go (3)
Context(22-38)Handler(14-14)NewSafeStorage(150-154)clearnode/pkg/rpc/message.go (2)
Request(20-26)Response(88-94)clearnode/pkg/rpc/payload.go (2)
Payload(21-41)Params(177-177)clearnode/pkg/rpc/error.go (2)
Errorf(72-76)Error(49-51)
clearnode/pkg/rpc/connection.go (1)
clearnode/pkg/log/noop_logger.go (1)
NewNoopLogger(12-14)
clearnode/pkg/rpc/context.go (4)
clearnode/pkg/rpc/payload.go (3)
Params(177-177)NewPayload(53-64)Payload(21-41)clearnode/pkg/rpc/message.go (3)
Request(20-26)Response(88-94)NewErrorResponse(161-165)clearnode/pkg/rpc/error.go (2)
Error(49-51)Errorf(72-76)clearnode/nitrolite/signature.go (1)
Sign(57-78)
clearnode/pkg/rpc/connection_test.go (1)
clearnode/pkg/rpc/connection.go (2)
WebsocketConnectionConfig(78-88)NewWebsocketConnection(91-126)
clearnode/pkg/rpc/dialer.go (2)
clearnode/pkg/rpc/error.go (2)
Errorf(72-76)ErrDialingWebsocket(32-32)clearnode/pkg/log/context.go (1)
FromContext(35-40)
clearnode/pkg/rpc/node.go (5)
clearnode/pkg/rpc/context.go (4)
Handler(14-14)SendResponseFunc(18-18)Context(22-38)NewSafeStorage(150-154)clearnode/pkg/rpc/payload.go (2)
Params(177-177)NewPayload(53-64)clearnode/pkg/rpc/connection_hub.go (2)
ConnectionHub(10-17)NewConnectionHub(21-26)clearnode/pkg/rpc/message.go (2)
Request(20-26)NewErrorResponse(161-165)clearnode/pkg/rpc/connection.go (3)
WebsocketConnectionConfig(78-88)NewWebsocketConnection(91-126)Connection(23-37)
clearnode/pkg/rpc/connection_hub_test.go (1)
clearnode/pkg/rpc/connection_hub.go (1)
NewConnectionHub(21-26)
clearnode/pkg/rpc/connection_hub.go (1)
clearnode/pkg/rpc/connection.go (1)
Connection(23-37)
🪛 Gitleaks (8.28.0)
clearnode/pkg/rpc/node_test.go
[high] 6-6: 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 (4)
clearnode/pkg/rpc/connection.go (2)
128-184: LGTM! Serve lifecycle is well-implemented.The
Servemethod properly:
- Guards against concurrent runs with mutex
- Tracks all 3 goroutines in the WaitGroup
- Protects
closureErrwith a dedicated mutex to prevent races- Coordinates shutdown via context cancellation
- Closes the WebSocket connection after all goroutines complete
This addresses multiple past review comments comprehensively.
210-227: LGTM! WriteRawResponse handles timeout correctly.The implementation properly:
- Uses
time.NewTimerwithdefer Stop()to avoid timer leaks- Employs a buffered
closeConnCh(size 1) with non-blocking send to prevent deadlocks- Returns false on timeout to signal the caller
This addresses past review comments about timer leaks and deadlock prevention.
clearnode/pkg/rpc/node.go (2)
255-263: LGTM! Context properly inherits parent lifecycle.The
Contextis now initialized withparentCtx(line 256), ensuring that handler execution is cancelled when the connection closes. This addresses the critical past review comment about disconnected lifecycles and prevents resource leaks.
407-412: LGTM! Nested group IDs prevent collisions.Line 409 uses
fmt.Sprintf("%s.%s", hg.groupId, name)to compose nested group IDs, which ensures that groups with identical names under different parents don't collide. This addresses the past review comment about nested group ID conflicts.
| func NewWebsocketNode(config WebsocketNodeConfig) (*WebsocketNode, error) { | ||
| if config.Signer == nil { | ||
| return nil, fmt.Errorf("signer cannot be nil") | ||
| } | ||
| if config.Logger == nil { | ||
| return nil, fmt.Errorf("logger cannot be nil") | ||
| } | ||
| config.Logger = config.Logger.WithName("rpc-node") | ||
|
|
||
| if config.OnConnectHandler == nil { | ||
| config.OnConnectHandler = func(send SendResponseFunc) {} | ||
| } | ||
| if config.OnDisconnectHandler == nil { | ||
| config.OnDisconnectHandler = func(userID string) {} | ||
| } | ||
| if config.OnMessageSentHandler == nil { | ||
| config.OnMessageSentHandler = func([]byte) {} | ||
| } | ||
| if config.OnAuthenticatedHandler == nil { | ||
| config.OnAuthenticatedHandler = func(userID string, send SendResponseFunc) {} | ||
| } | ||
| if config.WsUpgraderReadBufferSize <= 0 { | ||
| config.WsUpgraderReadBufferSize = 1024 | ||
| } | ||
| if config.WsUpgraderWriteBufferSize <= 0 { | ||
| config.WsUpgraderWriteBufferSize = 1024 | ||
| } | ||
| if config.WsUpgraderCheckOrigin == nil { | ||
| config.WsUpgraderCheckOrigin = func(r *http.Request) bool { | ||
| return true // Allow all origins by default | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical security vulnerability: CheckOrigin allows all origins.
Line 127 sets CheckOrigin to always return true, which permits WebSocket connections from any origin. This exposes the server to Cross-Site WebSocket Hijacking (CSWH) attacks where malicious sites can establish connections on behalf of authenticated users.
Solution: Implement strict origin validation against an allowlist:
if config.WsUpgraderCheckOrigin == nil {
- config.WsUpgraderCheckOrigin = func(r *http.Request) bool {
- return true // Allow all origins by default
- }
+ // SECURITY: Default to rejecting all origins. Operators must explicitly configure allowed origins.
+ config.WsUpgraderCheckOrigin = func(r *http.Request) bool {
+ return false
+ }
}Alternatively, if you need to allow all origins for development purposes, add a configuration flag and log a warning:
if config.WsUpgraderCheckOrigin == nil {
config.Logger.Warn("CheckOrigin not configured - rejecting all cross-origin connections for security")
config.WsUpgraderCheckOrigin = func(r *http.Request) bool {
return false
}
}🤖 Prompt for AI Agents
In clearnode/pkg/rpc/node.go around lines 98 to 129, the default
WsUpgraderCheckOrigin currently returns true (allowing all origins) which
creates a security risk; replace this permissive default with strict origin
validation by checking the request Origin header against a configured allowlist
(e.g., config.AllowedOrigins or config.OriginAllowlist) and only returning true
when the origin is present in that list, and if you need an opt-in development
mode allow-all behavior expose a boolean flag like config.AllowAllOrigins and
when that flag is true log a clear warning via config.Logger.Warn before
permitting all origins; ensure nil-checks and empty-list behavior default to
rejecting cross-origin requests.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
clearnode/pkg/rpc/node_test.go (1)
391-395: Replace fixed sleep with proper synchronization.The bare
time.Sleep(100 * time.Millisecond)at line 393 makes the test timing-dependent and potentially flaky. Consider using a synchronization mechanism like a channel or wait condition to ensure the disconnect has been processed before assertions.For example, you could add a disconnect notification channel:
// Disconnect cancel() - time.Sleep(100 * time.Millisecond) // Allow some time for the disconnect to be processed + + // Wait for disconnect to be processed + select { + case <-time.After(1 * time.Second): + t.Fatal("disconnect not processed in time") + default: + // Poll for disconnect completion + for i := 0; i < 20; i++ { + eventMu.RLock() + disconnected := onDisconnectCount == 1 + eventMu.RUnlock() + if disconnected { + break + } + time.Sleep(50 * time.Millisecond) + } + }Or better, use a channel to signal when disconnect is complete in
OnDisconnectHandler.clearnode/pkg/rpc/context_internal_test.go (1)
178-203: Fix concurrency test synchronization.The test spawns goroutines without waiting for them to complete (lines 190-202), and calls
assertfrom those goroutines with*testing.T, which is unsafe. The test may exit before the goroutines finish, and failures in goroutines won't be properly reported.Apply this diff to add proper synchronization:
+import ( + "sync" + // ... other imports +) + func TestSafeStorage(t *testing.T) { storage := NewSafeStorage() key := "testKey" value := "testValue" storage.Set(key, value) retrievedValue, ok := storage.Get(key) require.True(t, ok, "Key should exist in storage") require.Equal(t, value, retrievedValue, "Retrieved value should match the set value") // Test concurrent access + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() for range 100 { storage.Set(key, value) } }() go func() { + defer wg.Done() for range 100 { - value, ok := storage.Get(key) - assert.True(t, ok, "Key should exist in storage") - assert.Equal(t, value, value, "Value should match the set value") + _, ok := storage.Get(key) + if !ok { + t.Error("Key should exist in storage") + } } }() + + wg.Wait() }Note: Also fix line 200 where
assert.Equal(t, value, value, ...)comparesvaluewith itself instead ofretrievedValue.clearnode/pkg/rpc/node.go (1)
125-129: Critical security vulnerability remains unfixed.This issue has been flagged multiple times in previous reviews: the
CheckOriginfunction defaults to accepting all origins, which exposes the server to Cross-Site WebSocket Hijacking (CSWH) attacks. In production, malicious sites can establish authenticated connections on behalf of users.Per previous review recommendations, either:
- Default to rejecting all origins and require explicit configuration:
if config.WsUpgraderCheckOrigin == nil { + config.Logger.Warn("CheckOrigin not configured - rejecting all cross-origin connections for security") config.WsUpgraderCheckOrigin = func(r *http.Request) bool { - return true // Allow all origins by default + return false } }
- Or require the operator to provide an origin allowlist in the config and validate against it.
🧹 Nitpick comments (2)
clearnode/pkg/rpc/node.go (2)
306-314: Consider returning errors instead of panicking.While panics are acceptable for catching programming errors during setup, returning errors would provide more flexibility for dynamic handler registration or testing scenarios where handlers might be loaded from configuration.
If you want to maintain the current panic behavior for the convenience of the happy path, you could add error-returning variants (e.g.,
HandleE,UseE) for use cases that need graceful error handling:func (wn *WebsocketNode) HandleE(method string, handler Handler) error { if method == "" { return fmt.Errorf("Websocket method cannot be empty") } if handler == nil { return fmt.Errorf("Websocket handler cannot be nil for method %s", method) } wn.handlerChain[method] = []Handler{handler} return nil } func (wn *WebsocketNode) Handle(method string, handler Handler) { if err := wn.HandleE(method, handler); err != nil { panic(err) } }
298-301: Consider documenting handler registration lifecycle or adding synchronization.The
handlerChainandroutesmaps are accessed without mutex protection. This is safe if all handler registration (Handle,Use,NewGroup) completes beforeServeHTTPis called, which appears to be the intended design pattern. However, if handlers can be registered at runtime while connections are active, concurrent map access could cause data races or panics.Either:
- Document that all handler registration must complete before starting the server
- Or protect the maps with an
sync.RWMutexto allow safe runtime registrationFor the documentation approach, add a comment to
NewWebsocketNode:// NewWebsocketNode creates a new RPC node instance with the provided configuration. // The signer is used to sign all outgoing messages, ensuring message authenticity. // // IMPORTANT: All handler registration (Handle, Use, NewGroup) must complete before // calling ServeHTTP. The node does not support dynamic handler registration after // the server has started accepting connections. func NewWebsocketNode(config WebsocketNodeConfig) (*WebsocketNode, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
clearnode/pkg/rpc/api.go(1 hunks)clearnode/pkg/rpc/client_internal_test.go(3 hunks)clearnode/pkg/rpc/context_internal_test.go(1 hunks)clearnode/pkg/rpc/dialer.go(5 hunks)clearnode/pkg/rpc/error.go(0 hunks)clearnode/pkg/rpc/message.go(2 hunks)clearnode/pkg/rpc/node.go(1 hunks)clearnode/pkg/rpc/node_test.go(1 hunks)clearnode/pkg/sign/mock_signer.go(1 hunks)clearnode/pkg/sign/mock_signer_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- clearnode/pkg/rpc/error.go
🧰 Additional context used
🧬 Code graph analysis (8)
clearnode/pkg/rpc/client_internal_test.go (1)
clearnode/pkg/sign/mock_signer.go (1)
NewMockSigner(18-20)
clearnode/pkg/sign/mock_signer.go (1)
clearnode/pkg/sign/sign.go (2)
PublicKey(22-25)Address(28-33)
clearnode/pkg/rpc/node_test.go (9)
clearnode/pkg/rpc/node.go (2)
WebsocketNodeConfig(66-94)NewWebsocketNode(98-147)clearnode/pkg/rpc/error.go (2)
Error(48-50)NewErrorParams(106-108)clearnode/pkg/sign/mock_signer.go (1)
NewMockSigner(18-20)clearnode/pkg/log/noop_logger.go (1)
NewNoopLogger(12-14)clearnode/pkg/rpc/context.go (3)
SendResponseFunc(18-18)NewSafeStorage(150-154)Context(22-38)clearnode/pkg/rpc/payload.go (3)
NewParams(205-219)Params(177-177)NewPayload(53-64)clearnode/pkg/rpc/api.go (2)
ErrorMethod(65-65)Method(57-57)clearnode/pkg/rpc/message.go (2)
Request(20-26)NewRequest(42-47)clearnode/pkg/rpc/dialer.go (2)
DefaultWebsocketDialerConfig(61-66)NewWebsocketDialer(83-89)
clearnode/pkg/rpc/message.go (2)
clearnode/pkg/rpc/payload.go (1)
NewPayload(53-64)clearnode/pkg/rpc/api.go (2)
ErrorMethod(65-65)Method(57-57)
clearnode/pkg/rpc/node.go (6)
clearnode/pkg/rpc/context.go (4)
Handler(14-14)SendResponseFunc(18-18)Context(22-38)NewSafeStorage(150-154)clearnode/pkg/rpc/payload.go (2)
Params(177-177)NewPayload(53-64)clearnode/pkg/rpc/connection_hub.go (2)
ConnectionHub(10-17)NewConnectionHub(21-26)clearnode/pkg/rpc/message.go (2)
Request(20-26)NewErrorResponse(161-165)clearnode/pkg/rpc/api.go (3)
PingMethod(61-61)Method(57-57)PongMethod(63-63)clearnode/pkg/rpc/connection.go (3)
WebsocketConnectionConfig(78-88)NewWebsocketConnection(91-126)Connection(23-37)
clearnode/pkg/rpc/dialer.go (5)
clearnode/pkg/rpc/error.go (2)
Errorf(71-75)ErrDialingWebsocket(31-31)clearnode/pkg/log/context.go (1)
FromContext(35-40)clearnode/pkg/rpc/message.go (1)
Response(88-94)clearnode/pkg/rpc/payload.go (1)
NewPayload(53-64)clearnode/pkg/rpc/api.go (3)
PingMethod(61-61)Method(57-57)PongMethod(63-63)
clearnode/pkg/rpc/context_internal_test.go (5)
clearnode/pkg/rpc/context.go (3)
Context(22-38)Handler(14-14)NewSafeStorage(150-154)clearnode/pkg/rpc/message.go (2)
Request(20-26)Response(88-94)clearnode/pkg/rpc/payload.go (2)
Payload(21-41)Params(177-177)clearnode/pkg/rpc/error.go (2)
Errorf(71-75)Error(48-50)clearnode/pkg/sign/mock_signer.go (1)
NewMockSigner(18-20)
clearnode/pkg/sign/mock_signer_test.go (2)
clearnode/pkg/sign/mock_signer.go (3)
NewMockSigner(18-20)NewMockPublicKey(46-48)NewMockAddress(69-71)clearnode/pkg/sign/sign.go (2)
PublicKey(22-25)Address(28-33)
⏰ 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: Test (Integration) / Test Integration
- GitHub Check: Test (Clearnode) / Test Clearnode
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
clearnode/pkg/rpc/message.go (2)
161-165: LGTM! Consistent use of ErrorMethod constant.The switch to
ErrorMethod.String()aligns with the newly introduced constant and ensures uniform error method identification across the RPC layer.
194-200: LGTM! Defensive guard improves robustness.The method check at lines 195-197 correctly prevents attempting to extract errors from non-error responses, making the error detection more explicit and type-safe.
clearnode/pkg/rpc/api.go (1)
64-65: LGTM! Clean constant addition.The
ErrorMethodconstant is well-placed and provides a centralized identifier for error responses, improving consistency across the RPC API.clearnode/pkg/rpc/client_internal_test.go (1)
123-143: LGTM! Good refactoring to centralized mock signer.The switch to
sign.NewMockSigner("signer1")improves code reuse and maintainability by using the shared mock implementation. The updated assertion at lines 141-142 correctly verifies the deterministic signature format.clearnode/pkg/sign/mock_signer_test.go (1)
1-62: LGTM! Comprehensive test coverage for mock types.The test suite thoroughly validates the mock signer, public key, and address implementations with clear assertions for Sign, Address, Bytes, String, and Equals methods.
clearnode/pkg/sign/mock_signer.go (1)
1-81: LGTM! Clean mock implementation for testing.The mock signer, public key, and address implementations provide predictable, deterministic behavior perfect for testing. The Sign method's suffix pattern (
-signed-by-{id}) makes signatures easily verifiable in tests. Documentation is clear and interface compliance guards are properly included.clearnode/pkg/rpc/dialer.go (1)
106-169: LGTM! Improved context lifecycle management.The refactoring introduces proper parent-child context separation:
parentCtxfor the initial connection and logger derivation (line 106, 147)childCtxfor goroutine lifecycle control (line 123, 145, 153-155)- Thread-safe closure error handling with mutex (lines 128-140)
- Clean resource cleanup via WaitGroup (lines 158-166)
This ensures proper cancellation propagation and prevents resource leaks.
clearnode/pkg/rpc/node.go (5)
153-203: LGTM! Well-structured connection lifecycle management.The ServeHTTP implementation correctly:
- Upgrades the HTTP connection to WebSocket with proper error handling
- Registers the connection with the hub before processing
- Uses a parent context with cancellation to coordinate goroutine shutdown
- Employs a WaitGroup to ensure both goroutines complete before cleanup
- Executes cleanup in a defer to handle disconnection events reliably
The handleClosure pattern ensures that if either goroutine exits (whether normally or due to error), the other will be cancelled and both will complete before the connection is closed.
259-267: Context lifecycle correctly fixed.The Context is now properly initialized with
parentCtxinstead ofcontext.Background(), which was a critical issue raised in previous reviews. This ensures that handler execution is correctly tied to the connection's lifecycle—if the connection closes, the context will be cancelled and handlers can respond appropriately.
277-281: LGTM! Proper re-authentication handling.The re-authentication logic correctly detects when a handler changes the user's identity (e.g., during login) and properly:
- Updates the connection hub's auth mapping via
Reauthenticate- Notifies the application via
OnAuthenticatedHandlerwith a freshSendResponseFuncThis enables proper routing of notifications to the newly authenticated user.
418-424: Nested group ID collision properly fixed.The
NewGroupmethod now correctly constructs the child group's ID by incorporating the parent'sgroupIdusingfmt.Sprintf("%s.%s", hg.groupId, name)(line 420). This prevents different parent groups from overwriting each other's handler chains when creating subgroups with the same name, which was a major issue flagged in previous reviews.
18-18: defaultNodeErrorMessage is used in context.go and its tests
The constant is referenced inclearnode/pkg/rpc/context.goandcontext_internal_test.go; the removal suggestion is incorrect.Likely an incorrect or invalid review comment.
Summary by CodeRabbit
New Features
Tests
Documentation