Repository navigation
Add configurable Axum buffered JSON body limits - #407
ChristianPavilonis wants to merge 2 commits into
Conversation
dhruv8sh
left a comment
There was a problem hiding this comment.
PR Review
Summary
This adds a configurable 2 MiB ceiling on buffered JSON bodies for Axum, with a 413 status through a new EdgeError::PayloadTooLarge. The runtime logic is sound:
- The size check runs before each chunk is appended, so streamed bodies are never buffered past the cap.
- Overflow handling is sticky, and the source is dropped exactly once.
- The JSON classification is fixed at ingress, and
Json/ValidatedJsonare capped regardless of Content-Type, so the "only capped when JSON" rule can't be bypassed.
I found no runtime bugs. I'm requesting changes for two blocking questions about integration with #275 (the open PR this branch depends on). Both should be settled before this merges.
😃 Praise
- 😃 Careful overflow handling in the body reader, strict and leak-free config validation, and thorough end-to-end boundary tests (see inline).
Findings
Blocking
- ❓ The JSON limit's config surface will clash with #275's
AxumIngressConfig(crates/edgezero-adapter-axum/src/run_options.rs:21, inline) - ❓ The #275 baseline audit is stale, and the docs are still marked provisional (
docs/superpowers/plans/2026-10-06-axum-buffered-json-body-limit.md:25, inline). The same provisional status appears at plan:4and:34, atdocs/superpowers/specs/2026-10-06-axum-buffered-json-body-limit-design.md:4, and atdocs/superpowers/specs/2026-08-22-inbound-body-design.md:28.
Non-blocking
- 🤔 Carrying the policy inside public
Bodyforces a breaking API change (crates/edgezero-core/src/body.rs:42, inline) - 🤔
Body::with_buffered_json_policyis public but only adapters need it (crates/edgezero-core/src/body.rs:124, inline) - 🤔 The JSON classifier reads only the first Content-Type header (
crates/edgezero-adapter-axum/src/request.rs:44, inline) - ♻️
capture_dev_environmentduplicates the key check infrom_env(crates/edgezero-adapter-axum/src/dev_server.rs:741, inline) - 📝 Status-code changes the guide doesn't spell out (
docs/guide/handlers.md:111, inline) - 🌱 Reject early on a declared Content-Length (
crates/edgezero-core/src/context.rs:713): JSON reads could fail immediately when the declaredContent-Lengthis over the effective cap, instead of reading up to the cap first. Not needed now. - ⛏ Machine-specific paths in the new docs (inline on the plan
:21and the design spec:12). The plan also cites/tmp/393-final-*.logfiles as evidence at:362-373; suggest replacing those with commands plus results, or a CI run link. - 📝
Bodygrows by one word, which is why#[expect(clippy::large_enum_variant)]was added incrates/edgezero-core/src/outbound.rsandcrates/edgezero-adapter-fastly/src/outbound.rs. No action needed, and it goes away if theBodyrefactor suggestion is adopted.
📌 Out of Scope
docs/superpowers/specs/2026-10-05-axum-production-startup-lifecycle-design.md:10contains the same/home/pav/...worktree path. It comes from #406, so it should be fixed there.
Dependency on #275
I think #275 should block the merge and the final review of this PR, but not this review. #275 has moved past the audited 950cb81 and now adds its own Axum ingress config plus a large dev_server.rs rework. That affects where this config should live and will cause rebase conflicts. The core body.rs/context.rs logic this PR relies on hasn't changed there. Before merge: rebase onto the accepted #275, redo the audit, and settle the config-naming question.
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS (1,861 passed). One
edgezero-macrostrybuild failure was environmental (build files vanished mid-build) and passed 9/9 on an isolated rerun. - No GitHub checks ran on this PR, likely because its base isn't
main.
| pub(crate) const DEFAULT: Self = | ||
| Self(NonZeroUsize::new(0x0020_0000).expect("positive default")); | ||
|
|
||
| pub(crate) fn from_config(env: &EnvConfig) -> anyhow::Result<Self> { |
There was a problem hiding this comment.
❓ The JSON limit's config surface will clash with #275's AxumIngressConfig
Since this plan's audit, #275 (now 596f9f4) added crates/edgezero-adapter-axum/src/ingress_config.rs. It reads ingress limits from EDGEZERO__ADAPTER__INGRESS__* using the same digits-only validation, and it reworks dev_server.rs (+800/−43 lines).
This PR adds a separate JsonBodyLimit, passed through run_options, dev_server and service, under a different name: EDGEZERO__ADAPTER__JSON_BODY_LIMIT_BYTES. Env var names are a public contract, so it's worth settling now.
Question: should this live in AxumIngressConfig, for example as EDGEZERO__ADAPTER__INGRESS__MAX_JSON_BODY_BYTES reusing its setting() validator? If it stays separate, please document why the naming differs. The same pattern is at line 304.
| | HEAD / parent hosting integration | `3ab22096fd117f7506cf437b5ba128e0789e059c` | | ||
| | Parent branch | `issue-392-396-production-hosting-spec` | | ||
| | Inherited #275 revision | `5527b58e2a7cb6307d5c7a8926cd5dfc6f4f8b4a` | | ||
| | GitHub #275 on 2026-10-06 | Open, head `950cb81d406d0dc269baf94314547329a4c1cef0`; not treated as accepted or integrated | |
There was a problem hiding this comment.
❓ The #275 baseline audit is stale, and the docs are still marked provisional
This says #275's head is 950cb81 and that body.rs, http.rs, context.rs and request.rs are unchanged against it. #275 is now 11 commits ahead (596f9f4). In the files this PR edits, the new commits only add lint expectations and switch tests to ConfigValue (request.rs, service.rs). But they also add ingress_config.rs and the large dev_server.rs rework, so the integration story here no longer holds.
"Deferred gate 1" (:34) is still unticked, and the provisional status is still at :4, at the design spec :4, and at the inbound-body spec addendum :28.
Fix: re-pin to the current #275 head, redo the audit after rebasing, and update or remove the provisional status lines.
| /// } | ||
| /// } | ||
| /// ``` | ||
| #[non_exhaustive] |
There was a problem hiding this comment.
🤔 Carrying the policy inside public Body forces a breaking API change
Downstream code that matches every Body variant now fails to compile (CHANGELOG.md:13 admits this), and every adapter's outbound and response code had to add into_content() normalization. The stated reason is that the policy survives extensions_mut().clear(). But trusted code can already strip it with into_content(), so the tamper resistance gained is small.
Alternative: keep it in a core-private place, such as a field on RequestContext filled from a crate-private request extension at construction. Body would stay exhaustive, and the extra large_enum_variant expectations in the outbound modules would go away. This is non-blocking if the spec owner accepts the trade-off.
| /// Original policy/provenance win on reattachment. Never polls or copies content. | ||
| #[must_use] | ||
| #[inline] | ||
| pub fn with_buffered_json_policy(self, ceiling: NonZeroUsize, ingress_is_json: bool) -> Self { |
There was a problem hiding this comment.
🤔 Public, but only adapters need it
Applications can use this to attach arbitrary ceilings, and it becomes a stable API commitment. Consider #[doc(hidden)] or moving it to an adapter-facing module.
| outbound_transport: Option<Arc<reqwest::Client>>, | ||
| json_body_limit: JsonBodyLimit, | ||
| ) -> Result<CoreRequest, String> { | ||
| let ingress_is_json = parts |
There was a problem hiding this comment.
🤔 The JSON classifier reads only the first Content-Type header
A request with text/plain followed by application/json, or with application/json; broken, is classified as non-JSON and skips the generic-read ceiling. Impact is low, since JSON extraction is capped regardless and generic reads still use the caller's cap. A version that only tightens behavior:
let ingress_is_json = parts.headers.get_all(CONTENT_TYPE).iter()
.filter_map(|v| v.to_str().ok())
.any(|raw| is_json_media_type(raw)
|| raw.trim_start().to_ascii_lowercase().starts_with("application/json"));The same applies at line 90.
|
|
||
| | Item | Inspected state | | ||
| | --- | --- | | ||
| | Worktree | `/home/pav/worktrees/issue-393-buffered-body-limit-spec` | |
There was a problem hiding this comment.
⛏ Machine-specific path: /home/pav/worktrees/... shouldn't be in committed docs; suggest dropping the worktree lines. The same goes for the /tmp/393-final-*.log evidence at :362-373; replace those with commands plus results, or a CI run link.
|
|
||
| Stacked base: `issue-392-396-production-hosting-spec` at `3ab22096fd117f7506cf437b5ba128e0789e059c`. This includes the initial #392/#396 implementation and the #275 integration revision recorded in its [startup/lifecycle spec](2026-10-05-axum-production-startup-lifecycle-design.md). It is not the latest or accepted #275 head. | ||
|
|
||
| Worktree: `/home/pav/worktrees/issue-393-buffered-body-limit-spec`. |
There was a problem hiding this comment.
⛏ Machine-specific path: please drop the /home/pav/... worktree reference.
| .len() | ||
| .checked_add(chunk.len()) | ||
| .ok_or_else(|| policy.error("request body size accounting overflow"))?; | ||
| if next_len > policy.cap { |
There was a problem hiding this comment.
😃 Careful overflow handling
The size check runs before each append, and the 413/400 choice comes from the policy rather than from matching message text (covered by managed_source_bad_request_is_not_translated_by_message). A deadline wins over an overflow on a tie, and the source is dropped exactly once and never polled again.
| pub(crate) const DEFAULT: Self = | ||
| Self(NonZeroUsize::new(0x0020_0000).expect("positive default")); | ||
|
|
||
| pub(crate) fn from_config(env: &EnvConfig) -> anyhow::Result<Self> { |
There was a problem hiding this comment.
😃 Strict, leak-free config validation: signs, whitespace, suffixes and fractions are rejected, nothing from the input value leaks into diagnostics, and the end-to-end tests confirm invalid values fail before any app hook runs in both dev and production.
| log | ||
| } | ||
|
|
||
| fn exact_json(bytes: usize) -> Vec<u8> { |
There was a problem hiding this comment.
😃 Thorough end-to-end tests: over the real network, they hit the exact boundary (at the limit and one byte over) with both fixed-length and chunked uploads. They cover the standalone and embedded hosting paths and several media types, and they verify that taken streams stay uncapped.
Summary
Stacked on #406, which is based on #275. This implementation was explicitly authorized on the current hosting base at
3ab2209, without waiting for accepted #275 integration. No parent code was merged or rebased. Keep this PR draft until the later prerequisite rebase/re-audit and remaining acceptance work are complete.Changes
body.rs,lib.rsBody, exhaustiveBodyContentraw handoff, and external API fixturesPayloadTooLarge, sticky initial errors, and stateless cached rechecksBodyContentwithout changing provider policiesBreaking API:
BodyaddsManagedand becomes#[non_exhaustive]. Downstream exhaustive raw-Body matches must migrate to accessors orBody::into_contentand exhaustiveBodyContent, or add a raw-Body wildcard.Closes
Closes #393.
Part of #391. This PR remains draft until accepted-prerequisite integration and the remaining acceptance work below are complete.
Test plan
cargo test --workspace --all-targetscargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcargo check --workspace --all-targets --features "fastly cloudflare spin"cargo check --all-targetsfor Fastly, Spin, and Cloudflare, plus core WASM checkcargo test -p edgezero-core --all-targets, 724 unit and 2 integration testscargo test -p edgezero-core --doc, 2 regular and 4 compile-fail tests; 13 existing examples ignoredcargo test -p edgezero-adapter-axum --all-targets --all-features, 192 unit, 13 contract, 16 subprocess teststests::json_body_limit_wire_content_length_and_chunkedexecuted separately and reportedokgit diff --checkFinal Rust commands checked one 25-file source fingerprint before and after execution. The plan records commands, counts, elapsed times, and local log paths. Local
/tmpartifacts are not durable CI evidence.Remaining gaps include dedicated above-8-MiB reconstruction, actual router app-state replacement dispatch, and router-only override wire cases. Media variants were tested at the configured 64-byte boundary rather than repeated for every default-size framing. The router-only test awaits outer cancellation, not an explicit inner blocking-worker join.
Fastly all-feature native tests could not link provider host ABI imports; portable Fastly tests passed 94. The standalone Viceroy version differs from the pinned version, and wasmtime/wasm-bindgen-test-runner were unavailable, so no provider-runtime certification is claimed. One inherited storage subprocess
selected-greetingassertion failed transiently during development, then passed individually and in subsequent full suites. It is unresolved, not classified as a reproduced baseline failure. Final native/workspace runs used no exclusions.Checklist
CLAUDE.mdconventions{id}syntaxedgezero_core