feat: add structured error codes to verifier (#46) #70
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. 📝 WalkthroughWalkthroughThe pull request implements structured error codes for signature verification, replacing generic error messages with specific codes (E001–E006). Changes include updating the OpenAPI schema with a new error response structure, adding error handling logic and nonce tracking to the verifier, and documenting error codes in the README. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
The test on line 417 even expects this to be OK, but from an HTTP semantics perspective, returning 200 OK when Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: verifier/src/main.rs
Line: 248:259
Comment:
When signature recovery fails, the function returns `StatusCode::OK` (200). This is semantically incorrect. A failed verification is an error condition and should return an appropriate error status code like `StatusCode::BAD_REQUEST` (400) or maintain consistency with other error cases.
The test on line 417 even expects this to be OK, but from an HTTP semantics perspective, returning 200 OK when `is_valid: false` is misleading. Clients typically use HTTP status codes to determine success/failure before parsing the response body.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
verifier/src/main.rs (1)
234-259: Use consistent error status code for recovery failures.Line 251 returns
StatusCode::OKfor recovery failures (E003), while other verification errors returnStatusCode::BAD_REQUEST. This is semantically incorrect—a failed verification should not return HTTP 200.Consider returning
BAD_REQUESTorFORBIDDENconsistently for all verification errors.🔧 Proposed fix
Err(e) => { println!("Verification failed: {}", e); ( - StatusCode::OK, + StatusCode::BAD_REQUEST, Json(VerifyResponse { is_valid: false, recovered_address: None, error: Some(VerifyError::recovery_failed(&e.to_string())), }), ) }
🤖 Fix all issues with AI agents
In @verifier/src/main.rs:
- Around line 101-107: The E004/address_mismatch path is never used: either add
an expected address to the request and validate it in the signature flow, or
remove the unused error. To implement validation, add an expected_address field
to the PaymentContext struct, update any deserialization/constructors, then in
verify_signature (the function that recovers the signer) compare the recovered
address to PaymentContext.expected_address and return the
Error::address_mismatch(recovered, expected) when they differ; ensure unit tests
and any README references to E004 remain consistent. Alternatively, if you
choose to drop E004, remove the address_mismatch constructor, delete E004 from
the error enum/definitions and update README/docs/tests to eliminate the
now-unused error code.
🧹 Nitpick comments (1)
verifier/src/main.rs (1)
424-431: Consider adding test coverage for E005.The test for E005 (invalid message) is empty with a comment noting it's hard to trigger. While the current code structure makes it difficult to reach this error path, consider adding a test that directly calls the error constructor or mocks the scenario.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
gateway/openapi.yamlverifier/Cargo.tomlverifier/README.mdverifier/src/main.rs
🔇 Additional comments (7)
verifier/README.md (1)
50-91: LGTM! Clear and comprehensive error documentation.The error codes reference is well-structured and provides clear guidance for debugging. The table format, JSON example, and test references make this easy to follow.
gateway/openapi.yaml (1)
109-176: LGTM! Well-documented structured error response.The OpenAPI spec correctly documents the structured error response with all six error codes. The examples provide clear documentation for API consumers.
verifier/Cargo.toml (1)
14-14: Dependency added correctly.The
lazy_staticcrate is appropriate for creating the globalNONCE_TRACKER. Version 1.4 is stable and widely used.verifier/src/main.rs (4)
134-160: LGTM! Proper empty signature and nonce reuse detection.The empty signature check and nonce reuse detection are implemented correctly. The lock scope ensures thread-safe insertion and checking.
193-209: LGTM! Proper typed data parsing with error handling.The
TypedDataparsing correctly returns E005 error for invalid message format.
211-232: LGTM! Detailed signature parsing with error categorization.The signature parsing correctly categorizes different malformation errors (invalid hex, wrong length) and returns appropriate E002 errors.
433-511: LGTM! Comprehensive nonce reuse test.The E006 test correctly verifies that the first request succeeds and the second request with the same nonce fails with the appropriate error code.
|
@Diksha78-bot Might take a look at rust and the bugs greptile found |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
verifier/src/main.rs (4)
171-180: Fix TypedData parse variable typo.
serde_json::from_value(typed_data)references a non-existent variable and won’t compile.🐛 Proposed fix
- let typed_data: TypedData = match serde_json::from_value(typed_data) { + let typed_data: TypedData = match serde_json::from_value(typed_data_json) {
147-169: Nonce reuse detection never runs (E006 can’t fire).
NONCE_TRACKERis defined and cleared in tests, but the verification flow never checks or inserts nonces. Add an atomic insert after a successful recovery and keep a nonce copy before it’s moved into JSON.✅ Proposed fix
- // Reconstruct Typed Data (Domain, Types, Value) + // Reconstruct Typed Data (Domain, Types, Value) + let nonce = payload.context.nonce.clone(); @@ - let value = serde_json::json!({ + let value = serde_json::json!({ "recipient": payload.context.recipient, "token": payload.context.token, "amount": payload.context.amount, - "nonce": payload.context.nonce + "nonce": nonce.clone() }); @@ - Ok(address) => { + Ok(address) => { + let mut tracker = NONCE_TRACKER.lock().unwrap(); + if !tracker.insert(nonce.clone()) { + return ( + StatusCode::BAD_REQUEST, + res_headers, + Json(VerifyResponse { + is_valid: false, + recovered_address: None, + error: Some(VerifyError::nonce_reused()), + }), + ); + } + drop(tracker); let recovered_addr = format!("{:?}", address); println!("Signature valid! Recovered: {}", recovered_addr); (Also applies to: 221-233
236-248: Use a non-200 status for recovery failures.
Returning200 OKfor E003 makes error handling ambiguous and conflicts with the documented error responses.✅ Proposed fix
- ( - StatusCode::OK, + ( + StatusCode::FORBIDDEN, res_headers, // Header added Json(VerifyResponse { is_valid: false, recovered_address: None,
320-323:VerifyErrordoesn’t implementPartialEq.
assert_eq!(response.error, None)won’t compile; useis_none()instead.✅ Proposed fix
- assert_eq!(response.error, None); + assert!(response.error.is_none());
🤖 Fix all issues with AI agents
In `@verifier/src/main.rs`:
- Around line 76-83: The VerifyError::missing_signature() error is effectively
unreachable because signature absence is currently handled as E002 or extractor
errors; add an explicit guard for empty/missing signatures (prefer checking the
X-402-Signature header first) before any parsing logic so you return
VerifyError::missing_signature() when the header is absent or blank; update the
code paths that parse headers (including the other occurrence around the earlier
parse block referenced at lines ~197-199) to check the header value, return
missing_signature() immediately on missing/empty, and only proceed to
parsing/validation afterwards so E001 is triggered correctly.
- Around line 340-347: The tests call verify_signature with the wrong tuple
binding and then use a shadowed response variable; fix each test to match the
function's return type (StatusCode, HeaderMap, Json<VerifyResponse>) by
destructuring as let (status, _headers, Json(response)) =
verify_signature(HeaderMap::new(), Json(req)).await; (replace any
Json(_response) and subsequent use of response) so status, response, and
response.error are the actual values to assert against (apply same change to the
other failing test locations).
♻️ Duplicate comments (3)
verifier/src/main.rs (3)
69-74: Update downstream deserialization for structurederror.
VerifyResponse.erroris now a structured object; gateway still expects a string, which will break JSON decoding and error handling.
101-107: E004address_mismatchremains unused.
There’s no expected-address comparison in the flow, so this path is dead and docs/tests will drift. Either implement address validation or remove E004.
373-380: E005 test is empty and doesn’t assert anything.
Either implement an invalid-message case or mark it astodo!/unimplemented!so it can’t silently pass.
| impl VerifyError { | ||
| fn missing_signature() -> Self { | ||
| Self { | ||
| code: E001.to_string(), | ||
| message: "Missing signature".to_string(), | ||
| details: Some("No X-402-Signature header provided".to_string()), | ||
| } | ||
| } |
There was a problem hiding this comment.
Return E001 for missing signatures (currently never triggered).
Empty/missing signatures fall into E002 or extractor errors, so E001 is effectively unreachable. Add a guard (and optionally prefer X-402-Signature) before parsing.
✅ Proposed fix
- // Parse Signature (E001/E002: Malformed signature)
- let signature = match Signature::from_str(&payload.signature) {
+ // Parse Signature (E001/E002: Missing/Malformed signature)
+ let signature_str = if let Some(h) = headers
+ .get("X-402-Signature")
+ .and_then(|v| v.to_str().ok())
+ .filter(|s| !s.trim().is_empty())
+ {
+ h
+ } else {
+ payload.signature.trim()
+ };
+
+ if signature_str.is_empty() {
+ return (
+ StatusCode::BAD_REQUEST,
+ res_headers,
+ Json(VerifyResponse {
+ is_valid: false,
+ recovered_address: None,
+ error: Some(VerifyError::missing_signature()),
+ }),
+ );
+ }
+
+ let signature = match Signature::from_str(signature_str) {Also applies to: 197-199
🤖 Prompt for AI Agents
In `@verifier/src/main.rs` around lines 76 - 83, The
VerifyError::missing_signature() error is effectively unreachable because
signature absence is currently handled as E002 or extractor errors; add an
explicit guard for empty/missing signatures (prefer checking the X-402-Signature
header first) before any parsing logic so you return
VerifyError::missing_signature() when the header is absent or blank; update the
code paths that parse headers (including the other occurrence around the earlier
parse block referenced at lines ~197-199) to check the header value, return
missing_signature() immediately on missing/empty, and only proceed to
parsing/validation afterwards so E001 is triggered correctly.
| let (status, _headers, Json(_response)) = | ||
| verify_signature(HeaderMap::new(), Json(req)).await; | ||
| assert_eq!(status, StatusCode::BAD_REQUEST); | ||
| assert!(!response.is_valid); | ||
| let error = response.error.unwrap(); | ||
| assert_eq!(error.code, E002); | ||
| assert_eq!(error.message, "Malformed signature"); | ||
| } |
There was a problem hiding this comment.
Fix test calls to verify_signature and response binding.
The function returns (StatusCode, HeaderMap, Json<VerifyResponse>), but several tests call it with the wrong arity and also bind _response while using response.
✅ Proposed fix
- let (status, _headers, Json(_response)) =
- verify_signature(HeaderMap::new(), Json(req)).await;
+ let (status, _headers, Json(response)) =
+ verify_signature(HeaderMap::new(), Json(req)).await;
@@
- let (status, Json(response)) = verify_signature(Json(req)).await;
+ let (status, _headers, Json(response)) =
+ verify_signature(HeaderMap::new(), Json(req)).await;
@@
- let (status1, Json(response1)) = verify_signature(Json(req1)).await;
+ let (status1, _headers, Json(response1)) =
+ verify_signature(HeaderMap::new(), Json(req1)).await;
@@
- let (status2, Json(response2)) = verify_signature(Json(req2)).await;
+ let (status2, _headers, Json(response2)) =
+ verify_signature(HeaderMap::new(), Json(req2)).await;Also applies to: 364-365, 438-439, 454-455
🤖 Prompt for AI Agents
In `@verifier/src/main.rs` around lines 340 - 347, The tests call verify_signature
with the wrong tuple binding and then use a shadowed response variable; fix each
test to match the function's return type (StatusCode, HeaderMap,
Json<VerifyResponse>) by destructuring as let (status, _headers, Json(response))
= verify_signature(HeaderMap::new(), Json(req)).await; (replace any
Json(_response) and subsequent use of response) so status, response, and
response.error are the actual values to assert against (apply same change to the
other failing test locations).
|
@Diksha78-bot any updates? |
2 participants
Implements structured error responses with error codes (E001-E006) for the verifier service.
Closes #46
Greptile Overview
Greptile Summary
This PR adds structured error codes (E001-E006) to the verifier service, providing better error diagnostics and implementing nonce reuse detection for replay attack prevention.
What Changed
VerifyErrorstruct withcode,message, anddetailsfields replacing plain string errorsNONCE_TRACKERusinglazy_staticto detect and prevent nonce reuseCritical Issues Found
🔴 Breaking Change - Gateway Integration Broken
The
VerifyResponse.errorfield changed fromstringtoVerifyErrorobject, but the gateway atgateway/main.go:44-48still expects a string. This will cause the gateway to fail parsing verifier responses, breaking the integration between services.🔴 Memory Leak - NONCE_TRACKER Never Cleaned
The global
NONCE_TRACKERHashSet accumulates nonces forever without cleanup. In production, this will cause unbounded memory growth until the service crashes. Every verification request adds a nonce that's never removed.🔴 Logic Error - Nonce Inserted Before Validation
The nonce is marked as "used" BEFORE signature validation completes (line 159, before line 212-259). If validation fails due to malformed signature or recovery errors, the nonce is already consumed, preventing legitimate retries. This also allows attackers to exhaust the nonce space with invalid requests.
Additional Issues
is_validandrecovered_addressfields)Confidence Score: 1/5
Important Files Changed
File Analysis
Sequence Diagram
sequenceDiagram participant Client participant Gateway participant Verifier participant NonceTracker Client->>Gateway: POST /api/ai/summarize<br/>X-402-Signature: 0x...<br/>X-402-Nonce: uuid Gateway->>Gateway: Create PaymentContext<br/>(recipient, token, amount, nonce, chainId) Gateway->>Verifier: POST /verify<br/>{context, signature} Note over Verifier: Check empty signature (E001) alt Signature is empty Verifier->>Gateway: 400 BAD_REQUEST<br/>{is_valid: false, error: E001} Gateway->>Client: 403 Forbidden end Note over Verifier: Check nonce reuse (E006) Verifier->>NonceTracker: Check if nonce exists alt Nonce already used Verifier->>Gateway: 400 BAD_REQUEST<br/>{is_valid: false, error: E006} Gateway->>Client: 403 Forbidden else Nonce is new Verifier->>NonceTracker: Insert nonce (BEFORE validation!) end Note over Verifier: Parse signature (E002) alt Signature malformed Verifier->>Gateway: 400 BAD_REQUEST<br/>{is_valid: false, error: E002} Gateway->>Client: 403 Forbidden end Note over Verifier: Construct EIP-712 TypedData (E005) alt TypedData construction fails Verifier->>Gateway: 400 BAD_REQUEST<br/>{is_valid: false, error: E005} Gateway->>Client: 403 Forbidden end Note over Verifier: Recover address from signature (E003) alt Recovery fails Verifier->>Gateway: 200 OK (!)<br/>{is_valid: false, error: E003} Gateway->>Client: 403 Forbidden else Recovery succeeds Verifier->>Gateway: 200 OK<br/>{is_valid: true, recovered_address: "0x..."} Gateway->>Gateway: Generate receipt Gateway->>Client: 200 OK<br/>{result: "summary", receipt: {...}} end Note over NonceTracker: Nonce never removed<br/>Memory leak!(2/5) Greptile learns from your feedback when you react with thumbs up/down!
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.