feat(config): request_body_limit_bytes defaults to 0 = no cap; uniform 413 surface - #853
Conversation
…m 413 surface The reference LLM proxy ships with no request-body cap (its request-size guard defaults to off), and providers accept larger requests than any fixed gateway default — Anthropic takes 32 MB — so the 10 MiB default rejected requests the upstream would have served. - Default 0 = cap disabled: DefaultBodyLimit::disable() (axum would otherwise fall back to its built-in 2 MiB), Content-Length pre-check skipped, manual to_bytes reads widened. The duplicate-Content-Length rejection stays on — smuggling hygiene, not a size limit. - Configured caps behave exactly as before, and the 413 surface is now uniform on the chunked path: five bare Json handlers + the two Bytes handlers map through the shared rejection helpers, multipart keeps axum's 413 classification instead of folding it into 400, and /mcp + /a2a discriminate the length-limit error into the standard envelope. - e2e: harness gets a dedicated requestBodyLimitBytes override (pins 10 MiB for the existing 413 suite); new suite proves a 12 MiB body reaches the upstream on the new default.
📝 WalkthroughWalkthroughRequest-body limits now default to unlimited when configured as ChangesRequest body limit handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ProxyMiddleware
participant AxumBodyExtractor
participant ProxyError
Client->>ProxyMiddleware: Send request body
ProxyMiddleware->>AxumBodyExtractor: Apply configured body cap
AxumBodyExtractor->>ProxyError: Report length-limit rejection
ProxyError-->>Client: Return enveloped 413 response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…ore #849 Audit follow-ups on the unlimited-default change: - The passthrough tunnel's manual to_bytes still used the raw limit, so the new 0 default rejected EVERY passthrough body with '413 request body exceeds 0-byte limit' (reproduced by the audit). It now goes through body_read_cap and discriminates the length-limit error from transport faults — a real cap hit is the enveloped 413, a read failure is a 400, matching /mcp and /a2a. Regression tests for both, plus chunked-oversize coverage for /mcp and /a2a. - Restores Cargo.toml strip = 'debuginfo' and the docker-image.yml symbol-table guard from #849: the previous commit was assembled with 'git reset --soft origin/main' after the remote ref had advanced past the working tree's base, so its tree silently reverted that commit. No intended change there.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aisix-core/src/config.rs (1)
376-401: 🔒 Security & Privacy | 🔵 TrivialDefault request-body cap now "unlimited" — confirm this is called out to operators.
default_body_limit()changing from 10 MiB to0(disabled) is a deliberate design choice matching upstream LLM proxy behavior, and it's documented thoroughly in the doc comment. Worth flagging explicitly for release notes: any operator upgrading without settingrequest_body_limit_byteswill silently go from a 10 MiB cap to no cap at all on JSON/multipart/passthrough/MCP/A2A bodies. Axum's ownDefaultBodyLimit::disable()docs recommend pairing this with an independent limit (e.g.tower_http::limit) "if you're accepting data from untrusted remotes" — worth considering whether a reverse-proxy/LB in front of the gateway is expected to be the actual backstop, and documenting that assumption.
Note that if you’re accepting data from untrusted remotes it is recommend to add your own limit such as tower_http::limit.🤖 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 `@crates/aisix-core/src/config.rs` around lines 376 - 401, Update operator-facing release notes/documentation for ProxyConfig::default_body_limit to explicitly state that the default request_body_limit_bytes is now unlimited instead of 10 MiB across all supported request types. Document that deployments accepting untrusted remotes must provide an independent backstop, such as a reverse-proxy/LB limit or tower_http::limit, rather than relying on the disabled default.
🤖 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 `@crates/aisix-core/src/config.rs`:
- Around line 376-401: Update operator-facing release notes/documentation for
ProxyConfig::default_body_limit to explicitly state that the default
request_body_limit_bytes is now unlimited instead of 10 MiB across all supported
request types. Document that deployments accepting untrusted remotes must
provide an independent backstop, such as a reverse-proxy/LB limit or
tower_http::limit, rather than relying on the disabled default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42a7362b-12c4-4b2d-b131-54b27a873b86
📒 Files selected for processing (16)
config.example.yamlconfig.managed.yamlcrates/aisix-core/src/config.rscrates/aisix-proxy/src/a2a.rscrates/aisix-proxy/src/audio.rscrates/aisix-proxy/src/completions.rscrates/aisix-proxy/src/error.rscrates/aisix-proxy/src/images.rscrates/aisix-proxy/src/jobs.rscrates/aisix-proxy/src/lib.rscrates/aisix-proxy/src/mcp.rscrates/aisix-proxy/src/passthrough.rscrates/aisix-proxy/src/rerank.rscrates/aisix-proxy/src/responses.rstests/e2e/src/cases/body-edges-e2e.test.tstests/e2e/src/harness/app.ts
|
Addressed the release-note/backstop point in the paired docs PR (api7/docs#1987): the reference page now carries an explicit upgrade note (earlier releases defaulted to 10 MiB) and tells operators accepting untrusted clients directly to either set a value or enforce the limit at the LB/ingress in front. The PR description's Behaviour changes section covers the same for release notes. |
1 participant
Problem
proxy.request_body_limit_bytesdefaulted to 10 MiB. The reference LLM proxy ships with no request-body cap at all (its request-size guard defaults to off), and providers accept larger requests than any fixed gateway default — Anthropic's Messages API takes requests up to 32 MB. A 10 MiB gateway default therefore rejects requests the upstream would have served (base64 PDFs and images hit this in practice), and a client that works against the provider directly breaks the moment it goes through the gateway.Two adjacent defects in the same family, found while aligning:
0was accepted but meant "reject every request with a body" —DefaultBodyLimit::max(0)plus adeclared > 0check — instead of "no cap". There was no way to express "unlimited" at all (omitting the layer entirely would still leave axum's built-in 2 MiB extractor default).Json<Value>handlers (completions, images, rerank, responses, audio/speech) and the two raw-Byteshandlers (batches, fine-tuning) leaked axum's stocktext/plainrejection instead of the OpenAI envelope; the multipart handlers (audio transcriptions/translations, files upload) folded axum's correctly-classified 413 into a generic 400;/mcpand/a2areturned a bare 400 for an over-cap body.Change
request_body_limit_bytes→0= no cap, matching the reference proxy's out-of-box behaviour.0now installsDefaultBodyLimit::disable()(not merely a skipped layer — axum would otherwise fall back to its built-in 2 MiB), skips the Content-Length pre-check, and widens the manualto_bytesreads (/mcp,/a2a,/passthrough) accordingly. (The passthrough site was missed in the first cut — every passthrough POST got a 413 on the new default; caught by the independent audit, fixed with regression tests.) The duplicate-Content-Length rejection (request-smuggling hygiene) keeps firing regardless of the cap setting.proxy_error_from_json_rejection,Byteshandlers through a newproxy_error_from_bytes_rejection, multipart handlers preserve axum's 413 classification viaproxy_error_from_multipart, and/mcp+/a2adiscriminate the length-limit error into the standard 413 envelope.Behaviour changes
request_body_limit_bytesin their config no longer cap request bodies (previously 10 MiB). The shippedconfig.example.yaml/config.managed.yamlpreviously carried the value explicitly, so file-based deployments that started from them keep whatever their file says; the compiled default only applies when the key is absent. Operators who want a cap set the value — everything about configured behaviour is unchanged.text/plain), multipart over-limit uploads get 413 (previously 400), and/mcp//a2aover-limit bodies get the enveloped 413 (previously a bare 400). Malformed JSON on the five converted handlers now also gets the enveloped 400 instead of axum's stock rejection.Tests
zero_limit_admits_bodies_over_axums_builtin_cap(a 2.5 MiB body — over axum's built-in 2 MiB — reaches the handler on both the declared-length and chunked paths),zero_limit_still_rejects_duplicate_content_length,chunked_oversize_on_v1_completions_returns_openai_envelope,chunked_oversize_on_v1_batches_returns_openai_envelope,chunked_oversize_multipart_returns_413_envelope. The three envelope/multipart tests fail against the previous handlers (verified)./passthrough,/mcpand/a2a(the length-limit discrimination path), and/passthroughno longer mislabels transport faults asRequestTooLarge.body edges: unlimited defaultsuite spawns the gateway withrequest_body_limit_bytes: 0and drives a 12 MiB request through to the mock upstream; the existing 413 suite keeps its 10 MiB subject via a new dedicated harness override (the harness pins the old value so every existing with-cap assertion still tests what it always tested).Summary by CodeRabbit
New Features
0.Bug Fixes
Tests