Repository navigation
feat: make config production-ready with env vars and dynamic nonces - #12
Conversation
|
Warning Rate limit exceeded@AnkanMisra has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 11 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
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. 📝 WalkthroughWalkthroughThis pull request adds environment-driven configuration for payment token, amount, recipient address, and verifier URL; replaces a static nonce with UUID-based nonces; updates AI/OpenRouter usage and response parsing; and bumps prerequisite tooling versions (Go 1.24+, Node 20+). Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gateway/main.go (2)
109-115: Handle JSON decode error to avoid silent verification failures.If the verifier returns malformed JSON or an unexpected response, the decode error is silently ignored.
verifyResp.IsValiddefaults tofalse, which may return a misleading "Invalid Signature" error to the client instead of indicating a service issue.🔎 Proposed fix
var verifyResp VerifyResponse -json.NewDecoder(resp.Body).Decode(&verifyResp) +if err := json.NewDecoder(resp.Body).Decode(&verifyResp); err != nil { + c.JSON(500, gin.H{"error": "Failed to parse verification response"}) + return +}
179-188: Unsafe type assertions can cause runtime panics.The chained type assertions on lines 183-185 will panic if the AI provider returns an unexpected response structure. This could bring down the service.
🔎 Proposed fix with defensive checks
var result map[string]interface{} -json.NewDecoder(resp.Body).Decode(&result) +if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { + return "", fmt.Errorf("failed to parse AI response: %w", err) +} -if choices, ok := result["choices"].([]interface{}); ok && len(choices) > 0 { - choice := choices[0].(map[string]interface{}) - message := choice["message"].(map[string]interface{}) - return message["content"].(string), nil +choices, ok := result["choices"].([]interface{}) +if !ok || len(choices) == 0 { + return "", fmt.Errorf("invalid response from AI provider") +} +choice, ok := choices[0].(map[string]interface{}) +if !ok { + return "", fmt.Errorf("invalid response from AI provider") +} +message, ok := choice["message"].(map[string]interface{}) +if !ok { + return "", fmt.Errorf("invalid response from AI provider") +} +content, ok := message["content"].(string) +if !ok { + return "", fmt.Errorf("invalid response from AI provider") } +return content, nil
🧹 Nitpick comments (1)
.env.example (1)
12-22: Inline comments may cause parsing issues in some environments.Some
.envparsers do not support inline comments (e.g.,#dummy). The comment text may be included as part of the value, causing configuration errors.Consider moving comments to separate lines or removing them:
🔎 Proposed fix
-# Recipient address (derived from private key, or set explicitly) -RECIPIENT_ADDRESS=0x2cAF48b4BA1C58721a85dFADa5aC01C2DFa62219 #dummy +# Recipient address (derived from private key, or set explicitly; example value below) +RECIPIENT_ADDRESS=0x2cAF48b4BA1C58721a85dFADa5aC01C2DFa62219 -# Token Configuration (optional - defaults shown) -USDC_TOKEN_ADDRESS=0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913 #dummy -PAYMENT_AMOUNT=0.001 +# Token Configuration (optional - example defaults shown) +USDC_TOKEN_ADDRESS=0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913 +PAYMENT_AMOUNT=0.001Note: The Gitleaks warning on line 18 is a false positive—this is the Base USDC contract address, not an API key.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gateway/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
.env.example.gitignoreCONTRIBUTING.mdREADME.mdgateway/go.modgateway/main.gosrc/config.ts
🧰 Additional context used
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 13-13: [UnorderedKey] The RECIPIENT_ADDRESS key should go before the SERVER_WALLET_PRIVATE_KEY key
(UnorderedKey)
[warning] 13-13: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 15-15: [UnorderedKey] The CHAIN_ID key should go before the RECIPIENT_ADDRESS key
(UnorderedKey)
[warning] 18-18: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 19-19: [UnorderedKey] The PAYMENT_AMOUNT key should go before the USDC_TOKEN_ADDRESS key
(UnorderedKey)
🪛 Gitleaks (8.30.0)
.env.example
[high] 18-18: 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). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (8)
gateway/go.mod (1)
8-8: LGTM!The
github.com/google/uuiddependency is a well-maintained library for UUID generation and is appropriate for generating unique nonces.CONTRIBUTING.md (1)
7-7: LGTM!Prerequisites are consistent with the project's
go.modand align with the README updates..gitignore (1)
45-45: LGTM!Adding
issues/to gitignore is appropriate if this directory is used for local issue tracking or temporary files.gateway/main.go (2)
98-102: LGTM on environment configuration and nonce generation.The dynamic
VERIFIER_URLwith a sensible default and UUID-based nonce generation are good improvements for production readiness.Also applies to: 138-138
143-150: LGTM on configurable recipient address.The fallback with a warning log is appropriate for backward compatibility while encouraging proper configuration.
src/config.ts (1)
15-17: LGTM!Environment-driven configuration with sensible defaults is a good pattern for production readiness. The fallback values ensure the application works out of the box for development.
README.md (2)
201-208: LGTM!Clear documentation of required and optional environment variables with defaults. This helps users understand the configuration options available.
216-235: LGTM!Adding the environment configuration step to Docker deployment is essential for production readiness.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (1)
-
gateway/main.go, line 86-89 (link)logic: hardcoded payment values bypass the new environment variable configuration. The PR adds
PAYMENT_AMOUNTandCHAIN_IDto.env.example, but these aren't read here.Add helper functions similar to
getRecipientAddress()for amount and chain ID.
8 files reviewed, 3 comments
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @AnkanMisra. * #12 (comment) The following files were modified: * `gateway/main.go`
📝 Add docstrings to `confif/prod-fixes`
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/main.go (1)
86-92: Critical: Hardcoded verification values mismatch payment context.The verification context uses hardcoded
Amount: "0.001"andChainID: 8453, whilecreatePaymentContext()(line 76) correctly usesgetPaymentAmount()andgetChainID(). This inconsistency will cause all valid signatures to fail verification when environment variables differ from these hardcoded defaults.🔎 Proposed fix
// 2. Verify Payment (Call Rust Service) context := PaymentContext{ Recipient: getRecipientAddress(), Token: "USDC", - Amount: "0.001", + Amount: getPaymentAmount(), Nonce: nonce, - ChainID: 8453, + ChainID: getChainID(), }
♻️ Duplicate comments (1)
gateway/main.go (1)
138-147: LGTM! But nonce replay attack remains unaddressed.The function correctly uses environment-driven helpers for recipient, amount, and chain ID, and generates dynamic nonces with UUID.
However, the critical security issue flagged in the previous review remains unresolved: nonces are generated but never validated or stored. This allows replay attacks where valid signatures can be reused indefinitely. The user acknowledged this as "redis issue 1" but no implementation has been added yet.
Nonce validation must be implemented before production deployment. Each nonce should be stored when generated and consumed (deleted) after first use to prevent replay.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gateway/main.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (6)
gateway/main.go (6)
10-10: LGTM!The new imports are appropriately used:
strconvfor parsingCHAIN_IDfrom environment variables andgoogle/uuidfor dynamic nonce generation.Also applies to: 14-14
100-104: LGTM!The verifier URL is properly configured from the
VERIFIER_URLenvironment variable with a sensible localhost default for development.
112-115: LGTM!Proper error handling for verification response decoding prevents silent failures and provides clear error feedback.
149-158: LGTM!The function properly reads from environment variables with appropriate fallback and warning logging.
171-182: LGTM!The function correctly reads and parses the chain ID from environment variables with proper validation, error logging, and sensible defaults.
184-237: LGTM! Excellent error handling improvements.The AI response parsing now includes comprehensive type assertions at each nested level (choices array → choice object → message object → content string), preventing panics from malformed responses and providing clear error messages. This is a significant reliability improvement over the previous implementation.
There was a problem hiding this comment.
Additional Comments (1)
-
gateway/main.go, line 86-92 (link)logic: verification context uses hardcoded values (
"0.001"and8453) instead of the dynamic env vars used increatePaymentContext(). This will always fail signature verification since the client signs one context but the server verifies against a different one.
8 files reviewed, 2 comments
Summary by CodeRabbit
Documentation
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Issue:
Greptile Summary
This PR makes the payment system production-ready by adding environment variable configuration for recipient address, token contract address, and payment amounts. Dynamic UUID-based nonces replace the previous hardcoded nonce.
Key changes:
RECIPIENT_ADDRESS,USDC_TOKEN_ADDRESS,PAYMENT_AMOUNT,VERIFIER_URLhub.lumenfield.work/google/uuidCritical issue found:
gateway/main.go:86-92- Verification context uses hardcoded values"0.001"and8453instead of callinggetPaymentAmount()andgetChainID(), causing all signature verifications to fail when env vars differ from defaultsConfidence Score: 2/5
createPaymentContext(), the verification logic inhandleSummarize()uses hardcoded values instead of reading from env vars. This mismatch means the client signs one context (with env var values) but the server verifies against a different context (with hardcoded values), causing guaranteed verification failures. The nonce replay vulnerability mentioned in previous threads remains unaddressed.gateway/main.gorequires immediate attention - the verification context construction must use the same helper functions as payment context creationImportant Files Changed
Sequence Diagram
sequenceDiagram participant Client participant Gateway participant Verifier participant OpenRouter Client->>Gateway: POST /api/ai/summarize (no signature) Gateway->>Gateway: createPaymentContext()<br/>(generate UUID nonce, read env vars) Gateway-->>Client: 402 Payment Required<br/>(paymentContext with nonce) Client->>Client: Sign paymentContext with EIP-712 Client->>Gateway: POST /api/ai/summarize<br/>(X-402-Signature, X-402-Nonce) Gateway->>Gateway: Reconstruct PaymentContext<br/>(with nonce from header) Gateway->>Verifier: POST /verify<br/>(context, signature) Verifier->>Verifier: Verify ECDSA signature<br/>recover signer address Verifier-->>Gateway: VerifyResponse<br/>(isValid, recoveredAddress) alt Signature Invalid Gateway-->>Client: 403 Forbidden<br/>(Invalid Signature) else Signature Valid Gateway->>OpenRouter: POST /api/v1/chat/completions<br/>(summarize request) OpenRouter-->>Gateway: AI summary response Gateway-->>Client: 200 OK<br/>(summary result) end