Repository navigation
Add configurable Axum buffered JSON body limits - #407
ChristianPavilonis wants to merge 6 commits into
Conversation
dhruv8sh
left a comment
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.
There was a problem hiding this comment.
REVIEWER: should-fix — residual acceptance evidence, not another buffering implementation
Source-only follow-up at 8a2a0b7d7d268c6e2b7269dc49e02c47c5f04b8a, kept in this existing audit thread to avoid duplicating review findings.
PR #275 already supplies lazy conversion, the bounded pre-append collector, typed 413, cache/poison handling and ingress/egress ownership. The remaining #393 behavior here is the configurable Axum ceiling. Please retain that shared path rather than reintroducing eager conversion or a second collector.
Two remaining acceptance requests:
-
Interrupted/erroring managed-JSON reads through the actual Axum service. The added
managed_json_overflow_drops_source_once_stops_polling_and_keeps_error_egress(service.rs:967) verifies overflow. The source-error and partial-cancellation cases cited at plan line 400 are core-context tests. Please add or identify an adapter-service test that starts a managed JSON drain, then injects a source error or cancels the pending service/drain, and checks exactly-once source release, no further polling, and the existing error/egress mapping where a response is produced. For a cancelled operation, do not require a response that the caller has abandoned. The issue's interrupted-read acceptance should be tied to an executable service-path proof, not inferred solely from the shared collector. -
Complete the documented override/reconstruction matrix.
production_hosting.rs:435-443proves an ordinary 9 MiB override, but not reconstruction above the portable 8 MiB fallback. The router-only wire test atdev_server.rs:1918uses the default;json_limit_development_capture_and_explicit_router_builderchecks configuration without exercising an overridden router-only listener. Please exercise initial and cached reconstruction with a ceiling above 8 MiB (preserving the ceiling/provenance), and the router-only configured exact-cap/+1 wire boundary through both constructors. Plan line 406 already records these as remaining gaps; attach current-head test names/results or explicitly resolve their acceptance status.
I checked all 23 existing inline threads and their replies before posting. The obsolete Body::Managed/BodyContent and 400-status documentation issue is already covered by the existing documentation thread; no duplicate finding is needed.
These are missing-evidence requests from source inspection, not newly reproduced runtime defects. I did not rerun the suite for this follow-up and am not claiming approval or closing the existing findings.
| /// } | ||
| /// } | ||
| /// ``` | ||
| #[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.
3308041 to
e867c12
Compare
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
This adds a configurable Axum ceiling, 2 MiB by default, for framework-managed JSON buffering. The limit is carried as a private policy on Body and re-applied through into_request. The core mechanism and config validation are careful, but four things need to change:
- The new dev-server boundary test is flaky.
- A cached body re-entering pre-dispatch skips the
InputGuard. - The docs and the PR description describe an API that this PR does not ship.
- The CHANGELOG doesn't flag the breaking changes that do ship.
Findings
Blocking
- 🔧 New boundary test is flaky (
dev_server.rs:1965): the 413 races a connection reset. It failed 6/30 runs in isolation and 3/20 in the full suite. - 🔧 Cached re-entry bypasses
InputGuard(context.rs:781): fallback outcomes differ from base. - 🔧 Docs and PR description describe an API that isn't shipped, and the wrong overflow status (
inbound-body-design.md:22). The other locations:- Spec
2026-10-06-axum-buffered-json-body-limit-design.md:- L49, L58, L166, L294, L301–303 and L333–334 state the non-exhaustive
Body/BodyContent/ adapter-migration design as fact. The note at L18–25 says that design was replaced. - L80, L81, L144 and L322 claim overflow returns 400.
- L256 says there is no request-body-too-large variant.
- L49, L58, L166, L294, L301–303 and L333–334 state the non-exhaustive
- Plan
2026-10-06-axum-buffered-json-body-limit.mdhas no supersession note:- L52, L67–111, L134–153 and L356 specify the Managed/BodyContent design.
- L114 marks all four adapters' body consumers as aligned.
- L149, L153, L175, L191 and L193 claim overflow returns 400.
- L398 cites
managed_content_accessors_and_manual_collection_ignore_ingress_ceiling, Body/BodyContent rustdoc fixtures and outbound/response/framing regressions. None of them exist. - The L362–385 evidence was recorded on
3ab2209with "25 changed inputs", not on this 22-file head.
- PR description:
- These parts describe changes that are not in the diff: the Changes-table rows for
body.rs/lib.rs, core outbound/response preparation and "All four adapters", plus the Breaking API paragraph. The description itself also says the obsolete carrier enum is not restored. - The status lines contradict each other: "Ready for review" vs "Keep this PR draft"; "No parent code was merged" vs the merge commits on the branch; "Closes #393" vs spec L387 ("does not close #393").
- The checked "Public source breaks documented" item refers to a break that didn't happen, while the real breaks are undocumented.
- These parts describe changes that are not in the diff: the Changes-table rows for
- Spec
- 🔧 CHANGELOG misses the real breaking changes, and the rustdoc is stale (
CHANGELOG.md:7).
Non-blocking
- 🤔
with_buffered_json_policyis public, unvalidated, and only the first call takes effect (body.rs:76). - 🤔 Rewrapping the body falls back to the looser 8 MiB default (
context.rs:355). - 🤔 JSON classification misses valid media types (
request.rs:156). - ♻️ The policy field grows every
Bodyfrom 40 to 64 bytes (body.rs:27). - ⛏ Blanket
arbitrary_source_item_orderingexpects (body.rs:67,context.rs:338,context.rs:1184). - ⛏ Hex literal for 2 MiB (
run_options.rs:20). - 🌱 No drain before close after an early 413. When the declared
Content-Lengthexceeds the ceiling, the server answers and closes with the body unread, so ordinary clients (reqwest here) often getECONNRESETinstead of the 413. The flaky test above shows it. The docs disclaim client receipt, which is fine for this PR. A later change could read and discard a bounded amount (a few KiB, or a short timeout) before closing, so clients actually see the 413. - 🌱
into_core_requesthas no way to override the limit (request.rs:54).
Earlier review threads
None of the 12 threads from the 2026-10-16 review has a reply. These still apply at this head; please answer them on their threads:
- ❓ JSON limit env-var naming vs
AxumIngressConfig(run_options.rs:22) - ❓ Stale #275 baseline audit and provisional status (
plan:25) - 🤔 Only the first
Content-Typeheader is classified (request.rs:65) - ♻️ Duplicate key check in the dev environment capture (
dev_server.rs:917). It now sharesconfig_path, but still matches the JSON path with its own literal instead offixed_setting. - ⛏ Machine-specific
/home/pav/...paths (spec:12,plan:21)
CI Status
-
fmt: PASS
-
clippy: PASS
-
tests: FAIL on the first run with two failures:
dev_server::tests::router_only_default_json_boundary_for_both_constructors- the pre-existing
connection::tests::pending_chunked_forward_preserves_original_deadlines_over_tcp, which has a 5 ms tolerance and failed 1/20 runs on this head vs 0/20 on base
PASS on rerun.
-
cargo check --workspace --all-targets --features "fastly cloudflare spin": PASS -
cargo check -p edgezero-adapter-spin --target wasm32-wasip2 --features spin: PASS
| .body(body) | ||
| .send() | ||
| .await | ||
| .expect("prebound listener request"); |
There was a problem hiding this comment.
🔧 This boundary test is flaky: the 413 case races a connection reset
The over-limit request declares Content-Length: 2097153. The server rejects it from the declared length without reading the body, sends 413, and closes the connection with the body unread. reqwest is still writing, gets ECONNRESET (hyper::Error(BodyWrite, ConnectionReset)), and this expect panics.
On macOS it failed 6 of 30 runs in isolation and 3 of 20 runs inside the full edgezero-adapter-axum --lib suite. It also broke my first cargo test --workspace --all-targets.
The new axum.md section already says rejection "does not promise client receipt". The upload helper in tests/production_hosting.rs handles this case: it reads the response independently of the writer and tolerates a reset only when the status is 413.
Fix: send the over-limit case over a raw socket the same way, for example through tokio::task::spawn_blocking(move || post_raw(addr, body)):
fn post_raw(addr: SocketAddr, body: Vec<u8>) -> String {
use std::io::{Read as _, Write as _};
let mut socket = std::net::TcpStream::connect(addr).expect("connect");
let mut writer = socket.try_clone().expect("writer");
let head = format!(
"POST /json HTTP/1.1\r\nHost: test\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n",
body.len()
);
let sending = std::thread::spawn(move || {
writer.write_all(head.as_bytes())?;
writer.write_all(&body)
});
let mut response = String::new();
let read = socket.read_to_string(&mut response);
let sent = sending.join().expect("writer thread");
if read.is_err() || sent.is_err() {
assert!(response.starts_with("HTTP/1.1 413"), "{response}");
}
response
}| let (body_policy, transfer) = raw_body.into_transfer(); | ||
| let body = match transfer { | ||
| BodyTransfer::Initial(body) => BodyState::Initial(guard_input(body, Rc::clone(&guard))), | ||
| BodyTransfer::Cached(bytes) => BodyState::Cached(bytes), |
There was a problem hiding this comment.
🔧 A cached body re-entering pre-dispatch skips the InputGuard
On base, a reconstructed buffered body entering new_pre_dispatch became BodyState::Initial(guard_input(..)); a Once body passes straight through guard_input. The first buffered read then ran:
guard.check(), which covers the deadline and the declared length vs the fallback max;prepare_protected_body→guard.observe(len)→complete = true.
With a policy attached, this arm now stores BodyState::Cached directly. The Cached arm of buffered_bytes, and therefore transform_pre_dispatch_body, only runs check_cached_body, and complete is never set.
I probed the same flow with an unmanaged body and a managed one, under ReadBodyBeforeFallback admission:
| Scenario | Base / unmanaged | Managed |
|---|---|---|
10 cached bytes, fallback max 4, hook body_bytes(1000) |
413, fixed outcome Exceeded |
Ok(10), no fixed outcome |
| hook reads before the deadline, deadline passes, fallback discard | Complete |
TimedOut |
transform_body expands 4 → 16 bytes, fallback max 8, then discard |
Complete |
Exceeded (the output is counted as raw input) |
This is only reachable when app code re-dispatches a reconstructed request through begin_ingress; Axum ingress always starts Initial. But managed_routed_reconstruction_restores_initial_and_cache_after_head_mutation exercises exactly this path, so it is a supported flow.
Fix: keep the cached state, but account it against the guard once, in the Cached arm of buffered_bytes:
BodyState::Cached(bytes) => {
if let Some(guard) = &self.input_guard
&& !guard.complete.get()
{
if let Err(error) = guard.check().and_then(|()| guard.observe(bytes.len())) {
let stored = StoredError::capture(&error);
let returned = stored.to_edge_error();
*state = BodyState::Poisoned(stored);
return Err(returned);
}
guard.complete.set(true);
}
let result = check_cached_body(&bytes, max);
*state = BodyState::Cached(bytes);
return result;
}Please add a regression test that covers the three rows above.
| it for captured JSON/+json ingress. Explicit caller caps may tighten it, and ordinary Form keeps | ||
| its 1 MiB cap. Taken streams, fallback discard, forwarding, and manual collection remain excluded. | ||
|
|
||
| Public `Body` is non-exhaustive with an opaque, nonrecursive policy carrier. Private initial/cache |
There was a problem hiding this comment.
🔧 This addendum describes an API that isn't shipped, and the wrong overflow status
The addendum says public Body "is non-exhaustive" and that "Exhaustive BodyContent exposes raw buffered/streamed ownership to transports". At this head:
Bodyis still a struct with a privateingress_bufferfield (body.rs:25).- Nothing under
crates/has#[non_exhaustive]onBody, aManagedvariant,BodyContentorBody::into_content. - The Fastly, Cloudflare and Spin adapters are untouched.
Line 16 says portable overflow returns 400. Base already returns a typed 413: see drain_body in context.rs, Body::into_bytes_bounded, and base's json_within rustdoc ("Returns 413 on overflow").
The same outdated design runs through the spec, the plan and the PR description; the line list is in the review body.
Fix: rewrite this addendum to describe what actually shipped:
- a private ingress policy/provenance field on the existing
Bodystruct; into_partsstrips the policy;- the typed 413 is unchanged from base.
|
|
||
| ### Axum buffered JSON limits and body API | ||
|
|
||
| Axum now defaults framework-managed JSON buffering to 2 MiB, exactly 2,097,152 bytes. |
There was a problem hiding this comment.
🔧 Breaking changes aren't flagged, and "representation unchanged" is inaccurate
For existing Axum apps, this entry changes behavior that the CHANGELOG normally marks with - **Breaking:** (see "Policy-Aware Axum Servers"):
-
The Axum
Json<T>/ValidatedJson<T>default drops from 8 MiB to 2 MiB. -
Explicit caps above the ceiling are now silently lowered to it and return 413:
- always:
json_within(N)andValidatedJsonWithin<T, N>; - on
application/json/+jsonrequests:body_bytes(N),form_within(N), the Form family, and pre-dispatchbody_bytes/transform_body(max_input, ..).
"Explicit application caps may tighten the ceiling" hides that clamp.
- always:
-
New public method:
Body::with_buffered_json_policy. -
Body'sDebugoutput gains aningress_bufferfield, andBodygrows from 40 to 64 bytes, so "representation remain unchanged" doesn't hold.
Please also update the rustdoc that now describes the wrong behavior:
DEFAULT_INBOUND_JSON_BYTES("accepted by JSON extractors (8 MiB)",context.rs:32)body_bytes("under the caller's byte cap",context.rs:343)json_within("Buffers at mostmaxbytes",context.rs:654)
All three should mention the native ceiling.
| /// Reattaching a policy preserves the original ceiling and classification. | ||
| #[must_use] | ||
| #[inline] | ||
| pub fn with_buffered_json_policy( |
There was a problem hiding this comment.
🤔 A public, unvalidated setter where only the first call takes effect
This method is pub only so that the Axum crate can call it, but any application can call it too:
with_buffered_json_policy(NonZeroUsize::MAX, ..)skipsJsonBodyLimit's<= isize::MAXcheck and makes plainJsoneffectively unbounded. A probe gotjson_body_default() == 18446744073709551615. On base, ordinaryJsonwas always capped at 8 MiB.- On a body that already has a policy, the call is a silent no-op. A probe that tried to tighten 4 → 1 still got 4.
- It works on bodies from every adapter, not only Axum.
The earlier thread on this method is no longer attached to a current line and was not answered.
Suggestion:
- mark it
#[doc(hidden)], asresponse_egress_framing.rsdoes; - reject ceilings above
isize::MAX; - either make a repeated call an error, or name the method so first-call-wins is visible.
| Some(origin) | ||
| } | ||
|
|
||
| fn is_json_media_type(raw: &str) -> bool { |
There was a problem hiding this comment.
🤔 JSON classification misses valid JSON media types
mime 0.3 rejects some values that RFC 9110 allows, so they are classified as non-JSON. Generic reads then skip the ceiling: body_bytes, form_within, the Form family, and pre-dispatch body_bytes/transform_body. I verified these with mime 0.3.17 and this exact function:
| Content-Type | classified as JSON |
|---|---|
application/json ;charset=utf-8 (RFC 9110 §5.6.6 allows whitespace before ;) |
false |
application/json; charset |
false |
application/json, |
false |
application/json+xml |
true (a false positive, but harmless) |
This adds to the open thread at line 65, which notes that only the first Content-Type value is read. The JSON extractors are unaffected, because they always apply the cap.
Suggestion: classify on the trimmed media type before ;, across every Content-Type value, and fail closed. That also removes the need for the new mime workspace dependency:
fn is_json_media_type(raw: &str) -> bool {
let essence = raw.split_once(';').map_or(raw, |(essence, _)| essence).trim();
essence.split_once('/').is_some_and(|(kind, subtype)| {
kind.eq_ignore_ascii_case("application")
&& (subtype.eq_ignore_ascii_case("json")
|| subtype
.rsplit_once('+')
.is_some_and(|(_, suffix)| suffix.eq_ignore_ascii_case("json")))
})
}The caller would use parts.headers.get_all(CONTENT_TYPE).iter().filter_map(|value| value.to_str().ok()).any(is_json_media_type).
With this, application/json; broken would be classified as JSON. Update that test case; failing closed is the safer direction.
| /// A buffered or locally streamed HTTP body with optional response-egress ownership. | ||
| pub struct Body { | ||
| egress_owner: Option<Box<OutboundScopeOwner>>, | ||
| ingress_buffer: Option<IngressBuffer>, |
There was a problem hiding this comment.
♻️ Request-only metadata grows every Body by 60%
Measured with size_of on 64-bit:
| Type | Base | This PR |
|---|---|---|
Body |
40 | 64 |
Response |
152 | 176 |
OutboundCollectedSlot |
200 | 224 |
Response and outbound bodies carry the field too, because attach_egress_owner/bind_outbound_scope keep it. This growth is what required the new clippy::large_enum_variant expect on the public OutboundCollectedSlot in outbound/batch.rs.
Option<IngressBuffer> takes 24 bytes because the policy is wrapped in a second enum.
Suggestion: flatten it:
#[derive(Clone, Copy, Debug)]
struct IngressPolicy {
cached: bool,
ceiling: NonZeroUsize,
ingress_is_json: bool,
}
// field: ingress_policy: Option<IngressPolicy> // 16 bytes via the NonZeroUsize nicheAlternatively, use Option<Box<..>> (8 bytes), since it is set once per request. Flattening also removes one of the three parallel Cached/Initial enums (IngressBuffer, BodyTransfer, BodyState).
| source: Option<Revocable<BodyStream>>, | ||
| } | ||
|
|
||
| #[expect( |
There was a problem hiding this comment.
⛏ Blanket ordering suppression on whole impl blocks
arbitrary_source_item_ordering is deny-level in this workspace. These #[expect]s cover the whole impl Body here, impl RequestContext (context.rs:338) and the test module (context.rs:1184). They exist only because the new functions were inserted out of order, and they turn the lint off for every future edit to the two largest impls.
Suggestion:
- In
impl Body: putfrom_transferafterfrom_stream,into_transferafterinto_stream, andwith_buffered_json_policylast. - In
RequestContext: putbuffered_bytesafterbody_kind, andjson_body_defaultbeforejson_within. - Then drop the expects.
|
|
||
| impl JsonBodyLimit { | ||
| pub(crate) const DEFAULT: Self = | ||
| Self(NonZeroUsize::new(0x0020_0000).expect("positive default")); |
There was a problem hiding this comment.
⛏ 0x0020_0000 reads less clearly than 2 * 1024 * 1024, which is how DEFAULT_INBOUND_JSON_BYTES / DEFAULT_INBOUND_FORM_BYTES are written (context.rs:33-35). The same applies to the 0x0020_0000/0x0020_0001 literals in the tests here, in dev_server.rs and in production_hosting.rs.
| axum_body, | ||
| None, | ||
| HttpClient::with_client(client), | ||
| JsonBodyLimit::DEFAULT, |
There was a problem hiding this comment.
🌱 The public converter has no way to choose the limit
into_core_request attaches JsonBodyLimit::DEFAULT unconditionally. Because with_buffered_json_policy keeps only the first policy, a caller that embeds EdgeZero in its own Axum stack can't raise the limit without stripping the body. Stripping it then falls back to 8 MiB (see the comment on context.rs:355).
Consider adding an into_core_request_with_json_limit(request, limit_bytes) variant later.
Current review readiness — 2026-11-10
Ready for review at the publication head below. The current-head required test aggregate passed in run42916835491; additional pending runs are not a blanket review prerequisite. Reviewers:
aram356,prk-Jr,dhruv8sh.Review transitions are independent of TS work and the combined integration audit; #462 remains draft. This current status supersedes historical draft-only prerequisites below without claiming unexecuted deployment/manual acceptance complete. All three reviewers are requested. Stack465 is the current grouping; #453 is excluded.
Current review-stack checkpoint
Publication head:
8a2a0b7d7d268c6e2b7269dc49e02c47c5f04b8a. This cleanup preserves the existing feature unit and PR identity. The additional change is the completed generated-native build correction: image-only code now follows the scaffold's existing Fastly-example selection, with no request-policy change. Its focused generator/native proof is retained; fresh per-branch hosted checks are not assumed green.Review order: #275 → #406 → #407 → #409 → #414 → #422 → #439 → #454. Draft integration checkpoint: #462. New SDK/group/codec/phase development and unrelated TS work are not prerequisites or additions to this PR. Earlier notes below retain their original tested revisions and limits; they are not a whole-tree pass for this new head.
Earlier implementation and proof notes
RC refresh checkpoint, 2026-11-10
Current head:
4856d950bc724eb7d3d28df783d7cf4b23ff90e5.Current #275 Body/BodyPayload, exact outbound egress owner and immutable initial/cached JSON policy are retained. This does not restore the obsolete carrier enum. Internal reconstruction preserves policy and provenance even after mutable headers/extensions change; raw transfer retains the exact owner.
Focused core/Axum, cancellation/forwarding/reconstruction and exact-owner tests, scoped Clippy and independent correctness inspection pass. The lower CI fixes are merged and both fixture locks now declare the added MIME dependency. No dependency version change belongs to this layer.
Draft pending fresh hosted checks and the combined RC audit. Two unchanged native default-parallel deadline misses remain recorded and unclassified; serial passes and untouched-foundation parallel passes do not clear them. No timing bound was relaxed. No deployment, main merge, security-alert dismissal or review-thread resolution occurred. Historical design/review context follows.
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