feat(gateway): make request body size configurable via env#186
feat(gateway): make request body size configurable via env#186PranavAgarkar07 wants to merge 10 commits into
Conversation
Add dedicated duplicate-nonce error kind for nonce_already_used (was incorrectly mapped to 'expired'). Sanitize raw gateway JSON blobs in debug detail to show clean error codes instead. Closes AnkanMisra#165
Covers all HTTP status -> error kind mappings (400/402/403/408/409/429/ 502/504/5xx), gateway error codes (nonce_already_used, invalid_signature, upstream_unavailable, verification_unavailable, verifier_timeout, etc.), wallet error patterns (rejection codes 4001/ACTION_REJECTED, wrong chain messages, missing wallet, network errors), and detail sanitization.
Replace hardcoded 10MB limit with MAX_REQUEST_BODY_MB env var parsed by getMaxBodySize() helper in config.go. Falls back to 10MB when unset or invalid. Also fixes a P1 regression from the original implementation: the 413 error response key was mistakenly changed from 'max_size' to 'max_size_mb' with a different format. The original 'max_size' key with '%dMB' string format is restored, now driven by the configurable value. Closes AnkanMisra#116
|
@PranavAgarkar07 is attempting to deploy a commit to the ankanmisra's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughReplaces hardcoded 10MB limits with a configurable ChangesRequest Body Size Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gateway/config.go`:
- Around line 99-102: The getMaxBodySize function must guard against
non-positive values returned by getEnvAsInt (e.g., MAX_REQUEST_BODY_MB=0 or
negative) by clamping them to the default used elsewhere (like
getPositiveTimeout); update getMaxBodySize to treat mb <= 0 as the default 10
(or use the same default constant if present) before converting to bytes so
http.MaxBytesReader never receives a zero/negative limit and accidentally
rejects valid requests.
In `@gateway/main.go`:
- Around line 419-425: Replace the hardcoded const maxBodySize = 10*1024*1024 in
gateway/cache.go and any Content-Length checks or http.MaxBytesReader uses with
the dynamic value from getMaxBodySize(); update the cache middleware (e.g., the
Cache middleware/handler function) to call maxSize := getMaxBodySize(), use
http.MaxBytesReader(c.Writer, c.Request.Body, maxSize), compare Content-Length
against maxSize, and when returning 413 include the same formatted max_size
string as in gateway/main.go (fmt.Sprintf("%dMB", maxSize/1024/1024)) so
CACHE_ENABLED respects MAX_REQUEST_BODY_MB.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 169325ae-7afd-47d8-8070-93b825677bde
📒 Files selected for processing (6)
.env.examplegateway/config.gogateway/main.goweb/package.jsonweb/src/lib/errors.test.tsweb/src/lib/errors.ts
…y-size # Conflicts: # web/package.json # web/src/lib/errors.test.ts
…e.go Co-authored-by: codex <codex@users.noreply.github.com>
|
Resolved merge conflicts with main (web/package.json scripts moved to bun:test, web/src/lib/errors.test.ts import changed from vitest to bun:test). Also addressed the two CodeRabbit findings: Finding 1 (config.go): Added non-positive guard to getMaxBodySize — treats MAX_REQUEST_BODY_MB=0 or negative as the default 10MB with a logged warning, matching getPositiveTimeout's pattern. Finding 2 (cache.go): Replaced hardcoded const maxBodySize = 1010241024 with getMaxBodySize() and dynamic fmt.Sprintf("%dMB", maxSize/1024/1024) for the 413 response. CACHE_ENABLED now honors MAX_REQUEST_BODY_MB. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
vercel logs: |
…-size bun install was needed to sync web/bun.lock with web/package.json after vitest was added as a devDependency during merge conflict resolution.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37dffe69fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| # Request Body Size Configuration | ||
| # Maximum request body size in MB (default: 10) | ||
| MAX_REQUEST_BODY_MB=10 |
There was a problem hiding this comment.
This introduces a new operator-facing gateway setting, but only .env.example mentions it; the main README configuration table, gateway/README.md optional-variable table, and .env.production.example still omit it. In production/setup contexts users will not discover the new limit knob or know its default/unit, so please keep those config references aligned with this new env var.
Useful? React with 👍 / 👎.
…s, tests - Clamp MAX_REQUEST_BODY_MB to 10240 before converting to bytes to prevent int64 overflow (P3 finding) - Remove unused vitest devDependency from web/package.json (P2 finding) - Add MAX_REQUEST_BODY_MB to gateway/README.md env table and .env.production.example (P2 finding) - Add TestGetMaxBodySize regression tests covering default, custom, zero, negative, overflow-clamped, and non-numeric paths (P2 finding) Co-authored-by: codex <codex@users.noreply.github.com>
|
Addressed all 4 Codex findings in a0484b1: P2 — Remove unused Vitest dependency: Removed vitest from web/package.json devDependencies and regenerated web/bun.lock. The test:unit script uses bun test, not vitest. P2 — Document MAX_REQUEST_BODY_MB: Added to gateway/README.md optional-variable table (default: 10, clamped to 10240) and .env.production.example as a commented-out entry. P2 — Add regression coverage: Added TestGetMaxBodySize with 6 sub-tests covering: default (unset), custom value (25MB), zero fallback, negative fallback, overflow clamp (99999→10240), and non-numeric fallback. P3 — Clamp MAX_REQUEST_BODY_MB before multiplying: Added maxBodySizeMBMax = 10240 constant and clamp in getMaxBodySize(). Values exceeding 10240 MB are logged and clamped to prevent int64 overflow. |
|
@codex review |
|
@PranavAgarkar07 which issue does it solve, please mention it in the description |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0484b101d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Done — added |
… churn - Reduce maxBodySizeMBMax from 10240 to 100 to prevent memory-exhaustion risk from io.ReadAll buffering (Codex P2 finding) - Revert web/bun.lock to origin/main to drop unrelated transitive dep bumps from this gateway-only PR (Codex P2 finding) Co-authored-by: codex <codex@users.noreply.github.com>
|
Addressed both open Codex findings from the second review (commit P2 — Cap configurable request bodies below memory-exhaustion size
P2 — Revert unrelated web lockfile updates
Ran |
|
@codex review the pr |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
AnkanMisra
added
enhancement
AnkanMisra
left a comment
AnkanMisra
left a comment
There was a problem hiding this comment.
@PranavAgarkar07 Final Changes wrt to pr
|
|
||
| # Request Body Size Configuration | ||
| # Maximum request body size in MB (default: 10) | ||
| MAX_REQUEST_BODY_MB=10 |
| func TestGetMaxBodySize(t *testing.T) { | ||
| t.Run("default when unset", func(t *testing.T) { | ||
| t.Setenv("MAX_REQUEST_BODY_MB", "") | ||
| if got := getMaxBodySize(); got != 10*1024*1024 { | ||
| t.Fatalf("expected 10MB default, got %d", got) | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
Add route-level regression coverage for configured 413 responses
These added tests only validate getMaxBodySize() parsing, but they never exercise handleSummarize or CacheMiddleware with MAX_REQUEST_BODY_MB set to a smaller value and an oversized request body
That leaves both the cached and uncached 413 responses unprotected, so a future drift in the actual rejection path or max_size payload would still pass CI even though this PR changes a public error contract there
|
Addressed review feedback - added TestRequestBodyLimit_EnforcedAtRouteLevel with 5 sub-tests covering body size enforcement at the route level (below/at/over limit with env-driven config). Debug test removed. Commit 66adb49. All tests pass. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gateway/config_test.go (1)
531-620: 🏗️ Heavy liftConsider adding integration tests for the actual handlers.
The test validates the enforcement pattern and 413 response format, which is valuable. However, it uses a simplified mock handler rather than exercising
handleSummarizeorCacheMiddlewaredirectly.Testing the actual handlers would provide stronger protection against:
- Changes to the 413 response format in production code
- Removal of body-limit enforcement from specific handlers
- Differences between cached and uncached error paths
💡 Potential approach
Add integration tests that:
- Set up a test Gin engine with the actual
CacheMiddleware(cache disabled for simplicity)- Call
handleSummarizewithMAX_REQUEST_BODY_MB=1and a 2MB payload- Assert 413 response with
"max_size": "1MB"- Optionally, test the cached path by enabling cache and verifying both cache miss and cache hit scenarios
This would complement the current pattern-validation test by ensuring the actual handlers maintain the contract.
Based on learnings: Changes to gateway status codes or response bodies require matching tests in the actual handlers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/config_test.go` around lines 531 - 620, The current TestRequestBodyLimit_EnforcedAtRouteLevel test validates the body limit enforcement pattern using a mock handler but does not exercise the actual production handlers like handleSummarize or CacheMiddleware. Add a new integration test function that sets up a test Gin engine with the actual CacheMiddleware and calls handleSummarize directly with MAX_REQUEST_BODY_MB environment variable set to a specific limit (e.g., 1MB) and a payload exceeding that limit, then verify the response returns status 413 with the correct max_size format in the response body. This ensures the actual handlers maintain the expected contract and response format, rather than just testing the enforcement pattern in isolation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@gateway/config_test.go`:
- Around line 531-620: The current TestRequestBodyLimit_EnforcedAtRouteLevel
test validates the body limit enforcement pattern using a mock handler but does
not exercise the actual production handlers like handleSummarize or
CacheMiddleware. Add a new integration test function that sets up a test Gin
engine with the actual CacheMiddleware and calls handleSummarize directly with
MAX_REQUEST_BODY_MB environment variable set to a specific limit (e.g., 1MB) and
a payload exceeding that limit, then verify the response returns status 413 with
the correct max_size format in the response body. This ensures the actual
handlers maintain the expected contract and response format, rather than just
testing the enforcement pattern in isolation.
2 participants
Summary
Replaces the hardcoded
const maxBodySize = 10 * 1024 * 1024ingateway/main.gowith a configurable value driven by theMAX_REQUEST_BODY_MBenvironment variable. A new helpergetMaxBodySize()ingateway/config.gohandles parsing and falls back to 10 MB when the variable is unset or invalid.Also fixes a P1 issue introduced when the original hardcoded constant was replaced: the 413 error response JSON key drifted from
"max_size"to"max_size_mb"with a type change (string → int). Restored the original response format so clients parsing the error do not break.Related Issue
Closes #116
Type Of Change
Affected Areas
gateway/)verifier/)web/)Contributor Checklist
Verification
Notes For Reviewers
The
getMaxBodySize()helper lives inconfig.goalongside the other typed-helper functions (getEnvAsDuration,getEnvAsBool, etc.) and follows the same pattern: log a warning and return the default when parsing fails.Summary by CodeRabbit
New Features
Documentation
Tests