Repository navigation
feat(axum): add safe diagnostics and trusted proxy metadata - #409
ChristianPavilonis wants to merge 12 commits into
Conversation
66a05e2 to
9fa1666
Compare
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
This PR adds peer-gated incoming-proxy trust, checked native ingress metadata, correlated native request records, work snapshots and an optional observer, plus fixed native routing/JSON error text. The lifecycle machinery is solid, but I'm requesting changes for:
- a trust-walk behavior that lets an untrusted client collapse its identity to the trusted proxy's address;
- a duplicate-Host gap in the new "checked" host;
- a regression that drops the post-commit source-failure category from logs;
- four new hosting tests that can't pass on macOS;
- two open questions on how far the error redaction is meant to reach.
Findings
Blocking
- 🔧 Untrusted client collapses to the trusted proxy's identity (
crates/edgezero-adapter-axum/src/trusted_proxy.rs:417): a client-controlled malformed/oversized prefix invalidates the whole family, andeffective_clientbecomes the proxy IP with sourceDirect. - 🔧 Duplicate
Host→ first value is the checkedDirecthost (crates/edgezero-adapter-axum/src/trusted_proxy.rs:369): inconsistent withinbound_origin. - 🔧 Post-commit body-source failure category no longer logged (
crates/edgezero-adapter-axum/src/response.rs:516):NativeRequestRecordhas no source-failure field. - 🔧 Four new proxy/TLS hosting tests always fail on macOS; the relay error is swallowed (
crates/edgezero-adapter-axum/tests/fixtures/native_proxy.rs:268). - ❓ Path/query/form/
Validated*errors still echo request input on the managed path (crates/edgezero-core/src/context.rs:666): intended exemption from the spec's safe-errors row? - ❓ Guide says unmanaged JSON errors keep detail;
into_core_requestredacts them (docs/guide/axum-diagnostics.md:427): which is intended?
Non-blocking (inline)
- 🤔 Stripping rule too narrow (
X_Forwarded_For,X-Real-IP, ...) and too broad (oauth2-proxy/EnvoyX-Forwarded-*identity headers):trusted_proxy.rs:459 - 🤔 New exact
getrandom/ipnetpins downgrade the app-demo lock and pull in the wasip3 RC + a second wasm-tools tree:Cargo.toml:66 - ♻️
#[non_exhaustive]on the public diagnostics/trust types:diagnostics.rs:197 - ♻️ Public naming/surface (
CheckedHostSource,from_ingress,CheckedProxyMetadata):ingress.rs:331 - 🤔 Exposed request IDs reveal process request volume:
diagnostics.rs:36 - 🤔 Logger-panic isolation incomplete on the terminal path:
response_egress.rs:1234 - ♻️ New per-request
route.clone()on every adapter:app.rs:348 - 🤔 Sink/observer blocking scope understated:
axum-diagnostics.md:434 - ⛏ Duplicate/mislabeled identity-exhaustion logs and inconsistent
event=format:diagnostics.rs:349 - ⛏
strip_forwarding_headersis O(n²):trusted_proxy.rs:464 - ⛏
failure=not_foundambiguity:diagnostics.rs:703 - ⛏ 405 negative assertion can't fail:
app.rs:2611 - ⛏ Blank family vs blank CIDR asymmetry:
trusted_proxy.rs:83 - ⛏ Unsorted
Cargo.lockentry:Cargo.lock:739 - ⛏ IPv6 whole-family guard unreachable:
trusted_proxy.rs:151 - ⛏ Entropy failure cached forever:
diagnostics.rs:273 - ⛏ Process/stack language and stale facts in docs:
axum-diagnostics.md:439
Non-blocking (not pinned to a diff line)
- 🤔 Port race in
production_hosting.rsgrows, and this suite now runs in CI.address()(tests/production_hosting.rs:67-72) still binds127.0.0.1:0, drops the listener, and hands the port to a child process. This PR raises the number ofaddress()calls from 25 to 43, many inside loops. The 12 newfailed()checks end withconnect_timeout(addr).expect_err("failed startup never accepts connections")(line 249) on a port nothing holds, so a parallel test can be handed the same port and make that assertion fail spuriously. Measured locally: 1 of 3 parallel runs failedboth_signals_stop_idle_processeswithstartup bind: listener: address unavailable; serial runs don't hit it. The harness runs this suite with--features axum,test-utils --all-targets --include-ignored(crates/edgezero-test-harness/src/runners/ci/platforms.rs:144-161), so the flake can turn CI red. Suggestion: keep a process-wideMutex<HashSet<u16>>inaddress()so a port is never handed out twice, or retry spawn on that specific bind error, or run this binary with--test-threads=1. - 🤔 Test gaps behind documented claims.
- The guide and
adapters/axum.mdsay invalid settings fail startup in development and production, but nothing tests thatcapture_dev_environment(dev_server.rs:1054-1094) rejects invalid or non-UnicodeTRUSTED_PROXY_CIDRS/FORWARDING_HEADER_FAMILY/LOGGING__REQUEST_RECORDS. The end-to-end invalid-settings tests only run"diagnostics"mode; loop them over["diagnostics", "bare-dev"]likeinvalid_json_limits_fail_before_application_hooks_in_dev_and_production. AxumDevServer::with_trusted_proxy_policy,with_request_recordsandwith_diagnosticsare untested, andAxumRunOptions::with_trusted_proxy_policyis only checked as a stored value.- In core, the
FallbackBodyExceeded/FallbackReadTimedOutclassification (router.rs:795-810), the pre-dispatchfixed_outcomebranch, and theNormalizedIngressHeadSummarybyte formula have no colocated tests. - The IPv6 branches in the proxy fixture (
native_proxy.rs:179-182, 234-237) are unreachable because only IPv4 sockets are used, so IPv6 peers and quoted IPv6 nodes have unit coverage only.
- The guide and
- ⛏ A backwards terminal instant still reaches terminal-time callbacks. In
ResponseEgressAttempt::publish_report(response_egress.rs:1207-1236), whenrequest_start <= observed_at < egress_started_atunder an injected clock, the core report becomesUnspecifiedwith zero elapsed, butcomplete_at(&report, observed_at)still forwards the instant, and nativerecord()computesSome(duration). That contradicts the guide's "a backwards clock produces absent duration and a safe clock-fault category". PassOption<MonotonicInstant>(None on regression), or have the native side check the outcome. - ⛏ Cancellation during zero-length stream validation is misrecorded. If the future is dropped during
validate_empty_stream().await(response.rs:623-668), the attempt'sDropin theInitialstate reportsConversionError. The native record then shows the selected status (e.g. 200) plusconversion_erroralthough no response was sent, contradicting "no-response paths have no invented status". Report a cancellation disposition fromInitial, or document the edge case. - ♻️ Simplifications.
- The
ConnectionScopecheck (diagnostics.rs:341-346) can't trigger in production, becauseConnectionActivityis only minted by the same session; make it adebug_assert!and drop the variant. set_failure(Option<_>)is only called withSome(service.rs:539-541), andprepare_egress_response(.., selected: Option<&SelectedStatus>)always getsSomein production.set_ingress_error_kind(service.rs:523) is overwritten by the envelope's identical classification on the Response branch; it's only needed for Abort.- Every request allocates
route.pattern().to_owned()and builds a record even when request records are off and there's no observer (diagnostics.rs:539-561); skip construction in that case, or storeArc<str>/Option<RouteMetadata>.
- The
- 🌱 The
into_core_requestbypass. RawX-Forwarded-Hoststill drivesForwardedHostthere (request.rs:44-56), so anyone embedding the low-level converter in their own Axum router is spoofable by default. Documented as a bypass, but inserting a no-trustnormalizeresult (direct host only) would be cheap hardening. - 🌱 Pre-service rejections are invisible. Head-deadline failures (
header_deadline.rs:198-201) and hyper parser 400/431 responses produce no record and no count. That matches the spec's parsed-service boundary, but the guide should tell operators. - 🌱 TLS fixture upkeep.
tests/fixtures/tls/README.mdhas no regeneration commands, and since the CA key isn't committed (good), renewal means a new CA plus leaf. Nothing warns before the 2036-10-03 expiry; a-checkend-style assertion would. ConsiderClientConfig::builder_with_provider(aws_lc_rs::default_provider().into())innative_proxy.rs:284/302, so that feature unification enabling a second provider can't makebuilder()panic.
📌 Out of Scope
connection::tests::pending_chunked_forward_preserves_original_deadlines_over_tcp(connection.rs:1290, not touched by this PR) allows 5 ms on a 200 ms budget. It failed one parallelcargo test --workspace --all-targetsrun locally (handler terminal took 208.6ms, expected 200ms + at most 5 ms) and passed 10 of 10 isolated runs. Worth tracking separately alongside the "native default-parallel deadline misses" noted in the PR body.
CI Status
- fmt: PASS
- clippy (
--workspace --all-targets --all-features -D warnings): PASS - tests (
--workspace --all-targets): PASS on rerun (2,795 passed, 1 ignored); the first run hit the out-of-scopeconnection.rstiming flake above cargo test -p edgezero-adapter-axum --all-targets --all-features(macOS): FAIL. 4 proxy/TLS tests fail deterministically (inline atnative_proxy.rs:268), and 1 of 3 parallel runs also hit the port race- feature check (
fastly cloudflare spin): PASS - spin
wasm32-wasip2check: PASS - hosted checks at
5bbff2b: 29/29 green
| ), | ||
| None => return metadata, | ||
| }; | ||
| let Ok(parsed) = parse_result else { |
There was a problem hiding this comment.
🔧 An untrusted client can collapse its identity to the trusted proxy's address
The whole selected family is validated, including the client-written left prefix, before the trust walk. On Malformed (and on UnresolvedChain, lines 423-437) effective_client keeps the peer value from line 381 and client_source stays Direct.
Scenario, using the guide's own Cloudflare → LB topology: the visitor sends X-Forwarded-For: x. EdgeZero receives x, 203.0.113.7, 198.51.100.10 from the trusted LB 10.20.0.8. The family becomes Malformed, so effective_client() == 10.20.0.8 with source Direct. The same happens with 63+ entries (the 64-slot cap) or a client-added Forwarded element carrying proto=ftp or an invalid host.
Impact: the visitor evades per-client IP bans and rate limits keyed on effective_client, and passes any application allowlist that covers the proxy subnet. client_source() == Direct cannot distinguish "visitor connected directly" from "trusted proxy sent a chain we rejected". The guide's example (let client_boundary = metadata.effective_client();) checks neither source nor result. whole_family_validation_includes_unselected_prefix_and_by_nodes currently pins this behavior.
nginx real_ip_recursive avoids this by walking right to left and never parsing past the first untrusted hop, so a client-controlled prefix cannot invalidate the trusted suffix.
Fix (preferred): split each list into elements, walk right to left validating only trusted hops plus the boundary node, and ignore everything to the left. For Forwarded, keep the structural/quoting parse over the whole field (needed to find element boundaries), but do not let invalid values left of the boundary invalidate the assertion.
Fix (minimum): when the peer is trusted but no boundary is accepted, never report the proxy as the client:
// after the UntrustedPeer early return
metadata.effective_client = None;
metadata.client_source = CheckedHostSource::Unavailable;
// ...set them again only when the walk accepts a boundarydirect_peer() still exposes the proxy socket. Either way, the spec's "malformed whole-set fallback" row needs updating.
| ) -> CheckedProxyMetadata { | ||
| let direct_authority = match parts.uri.authority() { | ||
| Some(authority) => parse_authority(authority.as_str().as_bytes()).ok(), | ||
| None => parts |
There was a problem hiding this comment.
🔧 Duplicate Host headers: the first value becomes the checked Direct host
parts.headers.get("host") returns the first of several values. Neither hyper nor validate_normalized_head rejects duplicate Host. With Host: a.example followed by Host: b.example, effective_host() is a.example with source Direct, and ForwardedHost returns it. This also applies on trusted paths when the final Forwarded element has no host=. A front proxy that routes on the last or its own Host disagrees with the app. RFC 9112 §3.2 says such a request must be rejected with 400.
It also contradicts the existing origin witness: request.rs:133-137 (inbound_origin) already treats duplicate Host as unavailable.
Fix:
None => {
let mut hosts = parts.headers.get_all(HOST).iter();
match (hosts.next(), hosts.next()) {
(Some(value), None) => parse_authority(value.as_bytes()).ok(),
_ => None,
}
}Please add a unit test with two Host lines.
| "response-egress Axum body source failed after commit: {}", | ||
| response_egress_source_error_category(&error) | ||
| ); | ||
| Poll::Ready(Some(Err(_error))) => { |
There was a problem hiding this comment.
🔧 Post-commit body-source failure category is no longer logged anywhere
The base logged response-egress Axum body source failed after commit: {category} at WARN with the typed category (bad_gateway_transport, declared_length_exceeded, ...). This line deletes it on the grounds that "the native terminal owner records this once", but NativeRequestRecord has no source-failure field and record() (diagnostics.rs:524-552) never reads report.source_failure, which is populated at lines 488-490.
Scenario: an upstream stream fails mid-body. Before: a WARN naming the category. Now: outcome=source_error failure=none. With request_records=false, nothing at all, and observers can't see the cause either. The spec explicitly permits typed categories ("Log typed categories, not Display/Debug of error sources"), and ResponseEgressSourceFailure::category() is &'static str. The precommit zero-length path still logs its own WARN (line 656), so the "recorded once" rationale is applied inconsistently.
Fix: add a field before the record shape freezes:
pub source_failure: Option<&'static str>,
// in record():
source_failure: report.and_then(|r| r.source_failure).map(|f| f.category()),and print source_failure={} in SafeRecord. Add an assertion to managed_source_and_deadline_failures_keep_selected_200_prefix_accounting_and_one_record.
| { | ||
| let (head, body_prefix) = read_head(client).await?; | ||
| let rewritten = rewrite(&head, peer, assertions)?; | ||
| let socket = TcpSocket::new_v4()?; |
There was a problem hiding this comment.
🔧 The four new proxy/TLS hosting tests always fail on macOS, and the cause is swallowed
relay() binds the outbound socket to 127.0.0.2/127.0.0.3. On Linux all of 127/8 is loopback, but macOS lo0 only has 127.0.0.1, so the bind fails with EADDRNOTAVAIL (verified: Can't assign requested address). let _relayed = relay(...) (lines 95 and 99) discards the error, the client reads an empty response, and the test panics with only response head or UnexpectedEof.
Affected: native_source_bound_single_proxy_and_mapped_cidr_match_only_owned_peers, native_two_proxy_chain_stops_at_untrusted_hop_and_gateway_replaces_prefix, native_selected_families_conflicts_malformed_sets_gaps_and_exhaustion, native_verified_tls_ingress_overwrites_public_assertions_and_rejects_wrong_name. The suite is gated on unix, so it runs on darwin. cargo test -p edgezero-adapter-axum --all-features fails 4/32 on every macOS run (5 serial and parallel runs locally), while Linux CI stays green.
Fix (any one), plus surfacing the relay error:
- Probe-bind the source in
LocalProxy::startand panic with an actionable message, e.g.needs loopback alias 127.0.0.2 (sudo ifconfig lo0 alias 127.0.0.2 up). - Gate these four tests on
target_os = "linux". - At minimum, propagate the error out of the relay task (eprintln or channel) so the panic names the bind failure.
| serde_json::from_slice(bytes.as_ref()) | ||
| .map_err(|err| EdgeError::bad_request(format!("invalid JSON payload: {err}"))) | ||
| serde_json::from_slice(bytes.as_ref()).map_err(|err| { | ||
| if self.body_policy.is_some() { |
There was a problem hiding this comment.
❓ Managed-native framework errors still echo request input outside JSON
Only json_within got the fixed-text treatment. On the same managed native path these framework call sites still put raw input into default public errors:
context.rs:599:invalid form payload: {err}context.rs:825:invalid path parameters: {err}, e.g./items/PATH_SENTINEL→invalid type: string "PATH_SENTINEL", expected u32context.rs:854:invalid query string: {err}(e.g.unknown variant `X`)extractor.rs:82,100,331,407,483,524:Validated*→EdgeError::validation(err.to_string()). validator 0.20+ includes the rejectedvaluein params, so{"token":"PAYLOAD_SENTINEL"}renders... "value": String("PAYLOAD_SENTINEL").
wire_message returns the message verbatim for BadRequest/Validation. The spec says "Framework call sites must use safe fixed messages where they create those categories", and its "Safe errors and records" acceptance row says "Also test query/path parameters ... and payloads". The current tests cover a route-path sentinel and a Json<u64> sentinel only.
Is this an intentional exemption? If so, please record it in the spec and guide. If not, apply the same managed gate with fixed text (invalid path parameters, invalid query string, invalid form payload, validation failed) and add sentinel tests for each.
| cidrs: Option<&str>, | ||
| family: Option<&str>, | ||
| ) -> Result<Self, ProxyTrustConfigError> { | ||
| let selected_family = family |
There was a problem hiding this comment.
⛏ An empty family setting fails startup, but an empty CIDR list means "no trust"
EDGEZERO__ADAPTER__FORWARDING_HEADER_FAMILY= (set but blank) arrives as Some(""), trims to "", and is rejected, while EDGEZERO__ADAPTER__TRUSTED_PROXY_CIDRS= is accepted as no trust. Templated deployments (Helm, compose, systemd env files) commonly render unused variables as empty, so the same template fails for one variable and not the other.
Suggestion: treat a blank family as unset (and still reject a nonempty CIDR list without a family), or document the asymmetry in the settings table.
| "httparse", | ||
| "hyper", | ||
| "hyper-util", | ||
| "libc", |
There was a problem hiding this comment.
⛏ The committed lock has an unsorted dependency list
"libc", "ipnet" is out of order. Any cargo invocation without --locked rewrites the file; a plain local cargo test here immediately showed M Cargo.lock with exactly this swap. CI uses --locked, which compares semantically, so it passes there. Please commit cargo's normalized output.
| normalized.push(network); | ||
| } | ||
| } | ||
| if covers_family(&normalized, false) || covers_family(&normalized, true) { |
There was a problem hiding this comment.
⛏ The IPv6 "whole family" guard can never fire
Every IPv6 network with prefix < 96 that contains ::ffff:0:0 is rejected by the mapped-range rule, and every /96+ mapped network is projected to IPv4 (lines 305-310). So the normalized IPv6 set can never cover ::ffff:0:0, and covers_family(&normalized, true) is always false. ::/1 is rejected only by the mapped rule. Meanwhile 8000::/1,4000::/2,2000::/3 (all routable unicast IPv6, i.e. effectively trust-all for real clients) is accepted.
Suggestion: either drop the branch and document that the trust-all guard is best-effort, or define "trust-all" for IPv6 as covering 2000::/3 and test the combined-coverage case.
| requests: AtomicU64, | ||
| } | ||
|
|
||
| static PROCESS_NONCE: OnceLock<Result<[u8; 16], IdentityFault>> = OnceLock::new(); |
There was a problem hiding this comment.
⛏ One entropy failure poisons every later runner in the process
PROCESS_NONCE: OnceLock<Result<..>> caches Err permanently. A single transient getrandom failure at the first NativeSession::new makes every later session in the process fail, including in-process restarts and embedded hosts.
Suggestion: cache only Ok (retry on Err), or document that an entropy failure is process-fatal for native hosting.
| native accounting, but panic-abort, SIGKILL, and process failure have no completion or log-flush | ||
| promise. There is no retry, durable delivery guarantee, or added shutdown flush budget. | ||
|
|
||
| Deployment acceptance remains downstream. Parser tests and local socket/TLS fixtures do not |
There was a problem hiding this comment.
⛏ Process and stack language in user-facing and committed docs
- "Recheck the accepted prerequisite stack ..." (439-443) means nothing to a guide reader. Rewrite it as plain operator guidance: validate the deployed image and proxy configuration before relying on these properties.
- The plan record (
docs/superpowers/plans/2026-10-06-axum-diagnostics-trusted-proxy.md) has stale facts: rustls 0.23.40 (lines 291/365/375; the workspace pins=0.23.46), Rust 1.95.0 (347/375;.tool-versionsis 1.98.1), "No package version changed" (375; see the app-demo lock comment), unchecked gates (18-20, 227-229), and evidence under/tmp/edgezero-impl-397/. Should it be refreshed, or explicitly marked as a frozen historical record? - The spec (line 12) commits a personal worktree path,
/home/pav/worktrees/.... - The moved comment
Per spec 6.3.1.atcrates/edgezero-core/src/error.rs:406is a process reference; describe the constraint instead.
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Reviewed against the stack base issue-393-buffered-body-limit-spec (31 files). Local gates on 1.98.1: fmt, clippy (-D warnings) and cargo test --workspace --all-targets (2796 passed, 0 failed) all pass.
Approving. The trust policy design is solid. Inline comments are non-blocking questions and suggestions.
🤔 Dev-only TLS stack build cost (root Cargo.toml, axum dev-deps): rustls/tokio-rustls with aws_lc_rs pulls in aws-lc-sys, which needs a C toolchain and a lot of build disk. It is test-only, so the ring provider may be a lighter choice for CI.
📝 Committed test TLS key (crates/edgezero-adapter-axum/tests/fixtures/tls/server-key.der): the README marks it as a public test asset, but secret scanners will probably flag it. Consider an allowlist entry.
📌 Raw forwarding headers are now always stripped (service.rs, strip_forwarding_headers after normalize): with the default no-trust policy, managed native apps no longer see X-Forwarded-* or Forwarded. The docs cover this; a CHANGELOG or migration note would help upgraders.
| #[inline] | ||
| async fn from_request(ctx: &RequestContext) -> Result<Self, EdgeError> { | ||
| if let Some(checked) = ctx.extensions().get::<CheckedEffectiveHost>() { | ||
| return Ok(Self(checked.authority().unwrap_or("localhost").to_owned())); |
There was a problem hiding this comment.
🤔 On the managed path, an Unavailable checked host gives handlers the literal "localhost", so they can't tell "no trustworthy host" from a real localhost. Is this intended? If so, consider documenting the sentinel and pointing callers to CheckedEffectiveHost; otherwise returning an error may be safer.
| metadata.scheme_source = source; | ||
| } | ||
| metadata.forwarding_result = if unresolved { | ||
| ForwardingResult::UnresolvedChain |
There was a problem hiding this comment.
🤔 For UnresolvedChain (every hop trusted, or an obfuscated node), the trusted proxy's forwarded host and proto are still installed as effective values. That seems defensible because the direct peer is trusted, but could the spec or docs state it explicitly?
| clippy::arithmetic_side_effects, | ||
| reason = "the maximum endpoint returns before end + 1; IPv4 endpoints fit u128 and the IPv6 maximum is handled explicitly" | ||
| )] | ||
| fn covers_family(networks: &[IpNet], ipv6: bool) -> bool { |
There was a problem hiding this comment.
😃 Strong design: trust nothing by default, decide from the real socket peer, refuse family-wide trust (including IPv4-mapped IPv6), cap input size and network count up front, and keep operator values out of parse errors.
| handle: NativeDiagnosticsHandle, | ||
| request_records: bool, | ||
| ) -> anyhow::Result<Self> { | ||
| let nonce = *PROCESS_NONCE.get_or_init(|| nonce_from(getrandom::fill)); |
There was a problem hiding this comment.
🤔 The OnceLock caches the Result, so one early getrandom failure makes every later NativeSession::new fail for the life of the process. Cache only a successful nonce, or note that permanent failure is intended.
| /// Sharing this handle explicitly aggregates counters across managed runners. | ||
| #[derive(Clone, Default)] | ||
| pub struct NativeDiagnosticsHandle { | ||
| counters: Arc<Mutex<NativeDiagnosticsSnapshot>>, |
There was a problem hiding this comment.
🤔 Every request locks this global Mutex (plus per-connection state) in release. That could become a contention point under high concurrency; atomic counters would avoid it. Fine as a follow-up.
| .is_none() | ||
| ); | ||
| for (key, value) in [ | ||
| ("EDGEZERO__ADAPTER__TRUSTED_PROXY_CIDRS", "127.0.0.2"), |
There was a problem hiding this comment.
⛏ This case fails only because no forwarding family is set; a bare IP is otherwise valid (TrustedProxyPolicy::new accepts it). Add a family, or comment that the expected failure is the missing family, so the test checks what it appears to.
| let bytes = self.buffered_bytes(max, true).await?; | ||
| serde_json::from_slice(bytes.as_ref()) | ||
| .map_err(|err| EdgeError::bad_request(format!("invalid JSON payload: {err}"))) | ||
| serde_json::from_slice(bytes.as_ref()).map_err(|err| { |
There was a problem hiding this comment.
😃 Nice: managed JSON errors use fixed text instead of echoing the payload, unmanaged paths keep the detail, and a test covers both.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Reviewed head 31f46bde against stack base d6f74308. Since the previous review at 5bbff2bf, the only PR-specific change is the received-head preparer call in service.rs; every file carrying the earlier blocking findings is unchanged, and those threads have no replies. Requesting changes until they are addressed or answered.
Findings
Blocking
- 🔧 Earlier blocking findings remain open at this head: the trust walk collapsing a client to the trusted proxy's address on a malformed prefix (
trusted_proxy.rs:417), duplicateHostselecting the first value (trusted_proxy.rs:369), the dropped post-commit body-source failure category (response.rs:516), and the two open questions on redaction scope (context.rs:666,axum-diagnostics.md:427). Please fix or reply on each thread. - 🔧 Proxy/TLS hosting tests fail on macOS (reproduced):
lo0has only127.0.0.1, sorelay()cannot bind127.0.0.2/127.0.0.3, and the bind error is swallowed.cargo test -p edgezero-adapter-axum --all-features --test production_hosting -- native_source_bound native_two_proxy native_verified_tlsgives 0 passed / 3 failed, with misleading messages (response headatproduction_hosting.rs:390, TLSUnexpectedEofat:871). CI passes only because it runs on Linux. Suggestion: skip or gate these tests when the extra loopback addresses are not routable, and surface the relay error.
Non-blocking (inline)
- 🤔 Guide pipeline omits the received-head preparer, which sees unchecked forwarding headers:
service.rs:408
CI Status
- Hosted checks at
31f46bde: 29/29 pass - New
received_head_preparation_precedes_stripping_but_not_head_validation: PASS locally - Targeted proxy/TLS hosting tests on macOS: FAIL (3/3), as above. Local toolchain was rustc 1.99.1 rather than the pinned 1.98.1; the failure is an address-bind issue, not toolchain-dependent.
- Full workspace gates not rerun for this head.
| .await; | ||
| } | ||
| }; | ||
| app.prepare_received_head(&parts.headers, &mut parts.extensions); |
There was a problem hiding this comment.
🤔 The guide says application code runs after stripping, but the preparer runs before the trust check
prepare_received_head runs before normalize and strip_forwarding_headers, so the callback sees client-forgeable Forwarded / X-Forwarded-* even when the peer is untrusted. That is useful when the app authenticates its own credential first (as the new test does), and the rustdoc warns about it. However, the guide's flowchart and the "Remove both raw forwarding families" step (docs/guide/axum-diagnostics.md:89-98) still show application code running only after stripping. An app author reading the guide could trust X-Forwarded-Host inside a preparer without a credential check.
Suggestion: add the preparer step between head validation and proxy-policy matching in the diagram, and state that it receives unchecked headers and must authenticate before retaining any forwarded value.
Current review readiness — 2026-11-10
Ready for review at the publication head below. The required workspace and test aggregate have now passed in run42916832735; all 29 latest checks pass. 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:
5bbff2bf6d6d853b470f76e7293b7ecfa55476f5. 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:
3b52b804e5570564cc95cdaaf4b9efc6307875ae.Checked forwarding trusts the actual socket peer, validates the selected family and preserves original normalized-head accounting before stripping assertions and entering application callbacks. Diagnostics retain the existing terminal owner/time and redact raw values. Independent source review found no unresolved correctness defect.
The CLI-only diagnostics gate is fixed, with 57 CLI-only tests passing. Native ingress trust is now under
edgezero_adapter_axum::trusted_proxy, leaving the current outbound legacy-API guard unchanged. Parent namespace proof passes 370 Axum unit, 28 contract and 32 production-hosting tests plus axum-only strict Clippy. Nested demo resolution matches existing exact workspace getrandom0.4.2/ipnet2.12.0 requirements; no manifest pin was relaxed.This layer does not strip a future protected-CloudFront credential or prove a Geo reader identity.
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
Changes
edgezero-adapter-axumedgezero-core.2/.3relays, verified test TLS with wrong-name control, 32 production-hosting cases, callback/panic/privacy/counter checks and defensive fallback testsThe approved JSON extension retains HTTP 400 and unmanaged serde detail. Custom renderers still own their response, but native framework JSON-deserialization errors now reach them as
invalid JSON payload. Application-authored EdgeErrors are not globally sanitized. Managed raw-forwarding-header removal is documented as a compatibility change; the low-level converter remains a trust-policy bypass.Dependency changes are four direct Axum edges to existing locked packages. Rustls and Tokio Rustls are dev-only. The published TLS private key is an explicitly public test asset, never a deployment credential.
Closes
Closes #397.
Stack and acceptance
Part of #391. This PR remains draft until the required integration and acceptance gates are satisfied.
Stacked above #407 using
gh stack, at base3308041b910b648af746c15e809ee86b96adc2e9. The user authorized implementation on this open stack. That exception does not satisfy accepted #275/#393 integration or authorize bypassing #394/#395 upload experiments. Accepted-stack, final-image and deployed-ingress acceptance remain deferred.Test plan
Local checks passed for implementation commit
66a05e2:cargo test --workspace --all-targets— 1,938 passed, one existing ignored generated-workspace compile smokecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkandgit diff --checkcargo check --workspace --all-targets --features "fastly cloudflare spin"wasm32-wasip1, Cloudflarewasm32-unknown-unknown, Spinwasm32-wasip2cargo test -p edgezero-adapter-axum --all-targets --all-features— 255 library, 13 contract and 32 hosting casescargo test -p edgezero-core— 731 library tests plus integrations/doctests; 13 existing ignored doc examplesexamples/app-demoworkspacedocs/superpowers/plans/2026-10-06-axum-diagnostics-trusted-proxy.mdrecords commands, timings, historical failures, source fingerprints, all 22 specification rows and proof boundaries. Final runtime-input manifest SHA256:ac40934b17bb6c521951f15090e2accf3059abcb4d386c6da74a69d95de8c9d7.Local wire tests directly prove owned proxy peers, consumer agreement, verified TLS, selected 200 plus failure after an observed head/prefix, hook/cardinality preservation and graceful/grace-forced work release. Missing-peer, exhaustive parser/identity bounds, precise injected timing and defensive fallback branches remain service/unit evidence. Environment-derived secret/config error privacy is tested; declared-store/backend and actual outbound-origin experiments are not claimed. Explicit diagnostic stop fixtures do not prove the convenience second-signal-plus-native-snapshot combination. The existing supported convenience second-signal test is retained.
Independent source reviews found and resolved lost Abort failure classification and formatting drift. A maintenance-only duplicated IPv6 bracket split remains unchanged; node and authority port grammars differ. Docs installation reports seven existing audit vulnerabilities, two moderate and five high; no docs package changes were made.
Checklist
CLAUDE.md; shared Cargo/global formatting centrally scheduledTS cutover follow-up, November 11
Native hosting invokes application head preparation after structural validation and before stripping forwarding fields. Protected Geo paths also capture facts before credential sanitation; invalid heads still skip application preparation.
Current head:
3708ffa418778b8ec3499daad132b1a3f9c7962f.Local combined-RC qualification at
36f8e9c9c2c93661aaa73f2ca956cec3bc9e4c92: workspace tests 2,947 passed / 6 ignored; all-feature Axum 568 passed, including 36 production-hosting, 6 AWS startup and 4 celld startup cases; core doctests 19 passed / 13 ignored; strict workspace Clippy, formatting, feature compilation and Spin WASM check passed under Rust 1.98.1. Narrow TS consumer proof under Node 24.20.0: all four real proxy tests and typed prepared config/secrets/router startup passed. This is not final TS-stack/image/deployment qualification. Fresh hosted checks are still required.