Skip to content

Add configurable Axum buffered JSON body limits - #407

Draft
ChristianPavilonis wants to merge 2 commits into
issue-392-396-production-hosting-specfrom
issue-393-buffered-body-limit-spec
Draft

ChristianPavilonis wants to merge 2 commits into
issue-392-396-production-hosting-specfrom
issue-393-buffered-body-limit-spec

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Bound framework-managed Axum JSON buffering to a configurable 2 MiB default, exactly 2,097,152 bytes, with typed HTTP 413 on overflow. Keep taken streams, forwarding, manual collection, and other providers' defaults unchanged.
  • Preserve the original ceiling, JSON ingress classification, and cached provenance through request reconstruction, independent of mutable headers and extensions.
  • Reuse admission, lazy conversion, the existing collector/cache/poison state, and response lifecycle. Add strict startup validation and real HTTP/1 boundary tests.

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

Crate / File Change
Core body.rs, lib.rs Opaque policy/cache carrier, non-exhaustive public Body, exhaustive BodyContent raw handoff, and external API fixtures
Core context, extractors, errors Effective native/caller caps, typed PayloadTooLarge, sticky initial errors, and stateless cached rechecks
Core outbound and response preparation Strip ingress metadata at transport boundaries while preserving logical content and byte allocations
All four adapters' outbound/response consumers Migrate transport branching to exhaustive BodyContent without changing provider policies
Axum run options and development server Validated positive limit, fallible builders, strict decimal environment parsing, and safe OS-string development capture
Axum request/service Carry the selected limit through existing startup and attach immutable policy lazily after admission
Core, direct-service, and native HTTP/1 tests Reconstruction, cancellation/drop-once, deadline tie 408, usable Content-Length/chunked 413, overrides above 8 MiB, unrestricted taken streams, startup failures, and router-only defaults
Guides, changelog, spec and plan Configuration, approved breaking API migration, verification evidence, and remaining gaps

Breaking API: Body adds Managed and becomes #[non_exhaustive]. Downstream exhaustive raw-Body matches must migrate to accessors or Body::into_content and exhaustive BodyContent, 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-targets
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check
  • cargo check --workspace --all-targets --features "fastly cloudflare spin"
  • SDK-target cargo check --all-targets for Fastly, Spin, and Cloudflare, plus core WASM check
  • All 56 native/WASM adapter feature-matrix combinations
  • cargo test -p edgezero-core --all-targets, 724 unit and 2 integration tests
  • cargo test -p edgezero-core --doc, 2 regular and 4 compile-fail tests; 13 existing examples ignored
  • cargo test -p edgezero-adapter-axum --all-targets --all-features, 192 unit, 13 contract, 16 subprocess tests
  • Qualified tests::json_body_limit_wire_content_length_and_chunked executed separately and reported ok
  • Docs install, formatting, lint, build, outbound-contract guard, and internal Markdown/link/shell checks
  • git diff --check
  • Provider runtime suites, not implied by successful WASM compilation
  • Accepted feat: implement portable outbound HTTP and request lifecycle contracts #275 integration/re-audit and affected acceptance reruns
  • Excluded app-demo workspace and manual CLI serve, no entrypoint/generator source changes in this PR

Final Rust commands checked one 25-file source fingerprint before and after execution. The plan records commands, counts, elapsed times, and local log paths. Local /tmp artifacts 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-greeting assertion 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

  • Changes follow CLAUDE.md conventions
  • No Tokio dependency or blanket Send bound added to portable core
  • Route parameters use {id} syntax
  • HTTP types imported from edgezero_core
  • Existing registry/store wiring retained
  • New behavior has focused tests and independent read-only verification
  • Public source breaks documented
  • No secrets or credentials committed

@ChristianPavilonis ChristianPavilonis self-assigned this Oct 6, 2026
@ChristianPavilonis
ChristianPavilonis added this pull request to stack #408 October 6, 2026 21:02

@dhruv8sh dhruv8sh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/ValidatedJson are 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 :4 and :34, at docs/superpowers/specs/2026-10-06-axum-buffered-json-body-limit-design.md:4, and at docs/superpowers/specs/2026-08-22-inbound-body-design.md:28.

Non-blocking

  • 🤔 Carrying the policy inside public Body forces a breaking API change (crates/edgezero-core/src/body.rs:42, inline)
  • 🤔 Body::with_buffered_json_policy is 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_environment duplicates the key check in from_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 declared Content-Length is 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 :21 and the design spec :12). The plan also cites /tmp/393-final-*.log files as evidence at :362-373; suggest replacing those with commands plus results, or a CI run link.
  • 📝 Body grows by one word, which is why #[expect(clippy::large_enum_variant)] was added in crates/edgezero-core/src/outbound.rs and crates/edgezero-adapter-fastly/src/outbound.rs. No action needed, and it goes away if the Body refactor suggestion is adopted.

📌 Out of Scope

  • docs/superpowers/specs/2026-10-05-axum-production-startup-lifecycle-design.md:10 contains 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-macros trybuild 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> {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ 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 |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ 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]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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` |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏ 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`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏ 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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😃 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> {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😃 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> {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😃 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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Axum: enforce configurable buffered request-body limits

2 participants