Add Phase 5 cross-adapter verification suite (PR 18)#725
Merged
Conversation
Brings in PR11 review-finding fixes (compat shim additions, testlight integration changes, migration guard comments). Conflicts resolved by keeping PR12's http-native handler layer throughout — all compat shim insertions from PR11 are superseded by the http type migration.
Conflicts resolved by keeping PR13's http-native integration layer throughout — all compat shim insertions from PR12 are superseded by this branch's completed integration type migration.
- Add collect_body_bounded helper with INTEGRATION_MAX_BODY_BYTES (256 KiB) cap to prevent unbounded memory use on streaming bodies - Replace all into_bytes() calls (panic on Body::Stream) with collect_body or collect_body_bounded across integrations and auction endpoint - Make AuctionProvider::parse_response async so implementations can safely drain response bodies without panicking on the Stream variant - Add .await to both parse_response call sites in the orchestrator - Cap inbound request bodies in lockr, permutive, datadome, and didomi proxy handlers using collect_body_bounded before forwarding upstream - Fix lockr Origin header: replace expect() with a warn-and-skip fallback so an invalid user-supplied origin cannot crash the edge worker - Add PayloadSizeError::StreamRead variant to google_tag_manager and return 502 (not 413) when a stream transport error occurs while reading the body - Remove extra blank line before closing brace in google_tag_manager impl block
…n-provider-type-migration' into feature/edgezero-pr14-entry-point-dual-path
Resolved five conflicts: - Deleted compat.rs (PR15 goal: remove Fastly from core) - integrations/mod.rs: kept both PR15's ensure_integration_backend_with_timeout / predict_backend_name_for_url and PR14's INTEGRATION_MAX_BODY_BYTES constant - integrations/adserver_mock.rs, aps.rs, prebid.rs: merged collect_body import (PR14) with ensure_integration_backend_with_timeout / predict_backend_name_for_url imports (PR15) into single use statements
Move trusted-server-adapter-axum from workspace exclude to members list. Remove the global `target = "wasm32-wasip1"` build override from .cargo/config.toml (which forced the axum crate out of the workspace) and pass --target wasm32-wasip1 explicitly only for Fastly CI commands. Delete the now-redundant crate-local .cargo/config.toml. Update CI test-rust job to exclude the axum crate and pass the explicit target; test-axum job runs from the workspace root with -p flag. Add .edgezero/ to .gitignore to exclude the local KV store file.
Register AxumDevServer alongside FastlyViceroy in RUNTIME_ENVIRONMENTS so the full framework x runtime scenario matrix (WordPress, Next.js) runs against both platforms. AxumDevServer spawns the native trusted-server-axum binary (no WASM or Viceroy), binds to the fixed port 8787 (baked into axum.toml at compile time), and polls for any HTTP response as readiness (root returns 403 in test env). Binary path defaults to target/debug/trusted-server-axum, overridable via AXUM_BINARY_PATH. Settings are baked in at build time via TRUSTED_SERVER__* env vars, same as Fastly. The integration-tests.sh script now builds both the WASM and the native Axum binary with test-specific overrides (origin=127.0.0.1:8888). Add test_wordpress_axum and test_nextjs_axum individual test functions. Ignore .edgezero/ at workspace root (local KV store from the dev server).
- README: update Quick Start and Development commands for both runtimes - getting-started: add Axum as Option A (no Fastly CLI needed) - architecture: add trusted-server-adapter-axum to Core Components; add Runtime Targets table - testing: fix cargo test commands; add Axum adapter section; split Local Server Testing into Axum vs Fastly; restore clippy step in CI/CD workflow example alongside new test-axum job
The integration test matrix includes AxumDevServer which requires the native trusted-server-axum binary. Add build-axum step to the shared setup action, package the binary alongside the WASM artifact, and pass AXUM_BINARY_PATH to the integration test run step.
…ator wasm32-unknown-unknown (Cloudflare Workers) does not support std::time::Instant — it panics at runtime. web_time::Instant is a zero-cost drop-in on native and JS-backed on WASM.
Implements the Cloudflare Workers adapter following the same pattern as trusted-server-adapter-axum: TrustedServerApp implements the Hooks trait, platform services use noop stubs on native (CI-compilable), and the #[event(fetch)] entry point delegates to edgezero_adapter_cloudflare::run_app. Also adds UnavailableHttpClient to trusted-server-core platform module, parallel to the existing UnavailableKvStore.
CI: test-cloudflare job checks native host compile, wasm32-unknown-unknown compile (with cloudflare feature), and runs host-target unit tests. CLAUDE.md: add cloudflare crate to workspace layout and build commands.
…un_app The rev (38198f9) of edgezero used in this workspace requires worker 0.7 (not 0.6) and run_app() takes a manifest_src: &str as first argument. Updated Cargo.toml and lib.rs accordingly.
ChristianPavilonis
requested changes
Jun 18, 2026
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Collaborator
There was a problem hiding this comment.
Review Summary
🔧 Requesting changes. I found one high-impact credential-leak regression caused by letting the removed legacy admin aliases fall through to the publisher proxy, plus one Cloudflare local-build robustness issue. CI is green, but these should be fixed before merge.
I used CREG-style :wrench: findings inline and included GitHub suggestions where they are directly applicable.
Address PR review findings on the dual-path EdgeZero entry point. - Run the integration request-filter pipeline (DataDome protection) on the EdgeZero dispatch path, mirroring legacy ordering: after auth, before route match. Respond short-circuits keep EcFinalizeState; response effects thread out via extensions and apply after EC finalization in edgezero_main. Add dispatch regression tests via a test-utils-gated registry constructor. - Reject publisher.max_buffered_body_bytes = 0 at config validation. - Make the buffered publisher resolver method-aware so HEAD and bodiless statuses keep origin Content-Length instead of a misleading 0. - Bound cumulative decoded HTML input so a placeholder-emitting rewriter cannot grow the Wasm heap behind the output cap. - Add an EdgeZero-enabled Viceroy fixture and integration-tests-edgezero CI job running the EC lifecycle suite against the EdgeZero entry point. - Document publisher.max_buffered_body_bytes and the edgezero_enabled runtime flag in the configuration guide.
…t-dual-path' into feature/edgezero-pr15-remove-fastly-core # Conflicts: # crates/trusted-server-adapter-fastly/src/app.rs # crates/trusted-server-adapter-fastly/src/main.rs
…tly-core' into feature/edgezero-pr16-axum-dev-server # Conflicts: # crates/trusted-server-adapter-fastly/src/app.rs # crates/trusted-server-adapter-fastly/src/main.rs # crates/trusted-server-core/Cargo.toml
…erver' into feature/edgezero-pr18-phase5-verification
The legacy `/admin/keys/*` aliases were dropped from the route tables, which let them fall through to the publisher fallback. That fallback forwards the request — including the `Authorization` header and key-management body — to the publisher origin, leaking Trusted Server admin Basic credentials to origin logs. Fail closed locally instead: register the aliases to a 404 deny handler on every router (Fastly legacy `route_request`, Fastly EdgeZero, Axum, Cloudflare). The production basic-auth regex `^/_ts/admin` does not match these paths, so they are not auth-gated; denying them locally keeps admin credentials and key payloads off the wire. Canonical `/_ts/admin/keys/*` handling is unchanged. Adds regression tests on every router that POST a legacy alias with an `Authorization` header and assert a local 404 (not a publisher-proxy 5xx).
The Cloudflare local-build guard detects a non-0.7 `.worker-build/bin/worker-build` but reinstalled without `--force`, so `cargo install` refused to overwrite the existing binary and the script kept failing until the developer manually removed `.worker-build`. Add `--force` so the recovery path self-heals for wrangler dev.
- Satisfy clippy on the VICEROY_CONFIG_PATH override in the Viceroy environment helper: backtick `EdgeZero` in the doc comment and collapse the nested `if` into a let-chain. - Update the cross-adapter parity guard for the legacy `/admin/keys/*` aliases: they are now registered to a local 404 deny (not unrouted), so assert the aliases ARE registered and that a POST returns 404 with no auth challenge, rather than falling through to the publisher fallback.
aram356
approved these changes
Jun 22, 2026
aram356
left a comment
aram356
left a comment
Collaborator
There was a problem hiding this comment.
Summary
Round-5 review. My two prior REQUEST_CHANGES rounds are fully resolved, and the round-4 legacy-alias finding was addressed correctly — including a credential-leak vector that the naive "drop the routes" approach would have introduced.
Resolution of the round-4 finding
Dropping the legacy /admin/keys/* aliases entirely would let them fall through to the publisher fallback, which proxies the request — including the Authorization header and key-management body — to the publisher origin, leaking admin Basic credentials. The fix (commit 70380c29) instead registers the aliases to a local 404 deny handler on all four router paths (Axum, Cloudflare, Fastly route_request, Fastly EdgeZero NAMED_ROUTES):
- Handler returns
404 / "Not found\n" / text/plainand never reads the body orAuthorizationheader — fails closed. - Comments corrected on every adapter (the prior "parity with Fastly" claim is gone).
- Regression test on each router (
legacy_admin_aliases_denied_locally_not_proxied_to_publisher) POSTs a legacy alias with anAuthorization: Basic …header + key body and asserts a local404(not a proxy5xx). - Cross-adapter parity guard updated (
2c575925) to assert the aliases are registered to the local deny and return404with no auth challenge.
Verified: all four router paths, the handler implementations (no unwrap/panic/expect), the per-router tests, and the clippy let-chain fix in the Viceroy env helper. Full CI is green, including the new EdgeZero integration-tests job.
Non-blocking
🌱 seedling
- No single cross-adapter parity test asserts Axum == Cloudflare ==
404for the legacy alias the wayauction_*_paritydoes; each adapter verifies404independently (cf_legacy_…is Cloudflare-only). Behavior is pinned per-adapter, so this is low value — noting it only because it would match the suite's spirit. - Only
POSTlegacy aliases are denied; aGET /admin/keys/*carrying credentials would still reach the publisher fallback. The realistic stale-client threat isPOST(canonical routes arePOST-only), so this is acceptable as-is.
CI Status
- fmt: PASS
- cargo test (core): PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo check (cloudflare): PASS
- integration tests (incl. EdgeZero entry point): PASS
- vitest / docs / ts format: PASS
aram356
reviewed
Jun 22, 2026
aram356
left a comment
aram356
left a comment
Collaborator
There was a problem hiding this comment.
Inline notes for the two non-blocking 🌱 observations from my approving review above.
ChristianPavilonis
previously requested changes
Jun 22, 2026
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Collaborator
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #725 against feature/edgezero-pr16-axum-dev-server, focusing on the current head 2c5759250535ec5b2a75341613479f82f84f574c, the cross-adapter parity suite, adapter route/auth changes, legacy admin-alias handling, CI gates, and core HTML/benchmark additions. CI is green, and most prior review feedback appears addressed. I found one blocking security gap in the legacy admin-alias deny fix: only POST is denied, while other methods on the same known sensitive legacy paths can still fall through to the publisher fallback and forward Authorization.
Findings
P0 / Blockers
None.
P1 / High
- See inline comment: legacy
/admin/keys/*aliases are denied only for POST, leaving other methods on those sensitive paths able to reach the publisher fallback.
P2 / Medium
- See inline comment: the architecture guide now describes the legacy alias behavior incorrectly after the local-deny change.
P3 / Low
None.
CI / Existing Reviews
GitHub checks are currently passing, including fmt, Rust tests, Cloudflare checks, cross-adapter parity, JS tests, docs/TS formatting, browser tests, and integration tests. Existing review threads around wildcard route false positives, header/status parity, clippy gating, canonical admin auth coverage, discovery body comparison, and POST legacy-alias denial appear addressed at HEAD; the remaining issue is the method coverage of that denial.
Resolve conflicts keeping this branch's full fastly-removal architecture while preserving main's independent improvements: - Cargo.toml / core Cargo.toml: keep tower (axum dep); drop the residual fastly dependency from core (this branch removes it entirely via the EcKvStore platform trait + adapter-local rate limiter, superseding main's allowlist approach). - migration_guards.rs: keep the no-fastly guards; drop main's deferred-EC fastly allowlist tests. - fastly app.rs / main.rs: keep this branch's fastly-in-adapter wiring (ec_kv, rate_limiter, use_finalize_kv, core buffer_publisher_response) and adopt main's full ClientInfo capture into request extensions, which fixes the EdgeZero bot-protection metadata loss for integration request filters (DataDome). - edge_cookie.rs / storage/kv_store.rs: take main's identifier-free consent and EC log messages. - proxy.rs: take main's SVG image-optimizer tests (superset of this branch's). - auction/orchestrator.rs: keep the sequential-fan-out rejection test.
Blocking: - Force manual redirect handling on the Cloudflare fetch path (RequestRedirect::Manual). The Workers runtime defaults to Follow and auto-chases 3xx to any host, bypassing core's per-hop allowed_domains enforcement in proxy_with_redirects (SSRF). Surfaces the 3xx back to core unfollowed, matching the Axum adapter. - Return a fixed 501 from /admin/keys/rotate and /admin/keys/deactivate. Cloudflare's config/secret stores reject writes, so the real handlers surfaced an opaque 500; 501 makes the unsupported-runtime contract explicit, matching Axum. Non-blocking parity/safety: - Cap the buffered upstream response body at 10 MiB (Content-Length pre-check + post-buffer check), mirroring the Fastly adapter, to avoid OOMing the isolate on a large/hostile origin. - Reject image_optimizer and stream_response in the fetch path rather than silently dropping the behavior, honoring the PlatformHttpRequest contract. - Strip hop-by-hop response headers (and Connection-listed tokens) before forwarding, matching the Axum adapter's is_hop_by_hop_response_header. - Derive the S3 SigV4 signing time from the wasm-safe web_time clock so it no longer calls std::time::SystemTime::now(), which panics on wasm32-unknown-unknown (latent once asset proxying lands on Cloudflare). Tests: - Add routes_with_settings/build_router seam to the Cloudflare adapter and assert admin routes return 501 and the tsjs catch-all returns < 500, matching the Axum route-test coverage that would have caught the admin status divergence.
The build script only installed worker-build when none was on PATH, so a
globally installed worker-build 0.8.x was used as-is and rejected the worker
0.7 crate pinned in Cargo.toml ("Unsupported version worker@0.7.x, expected at
least worker@0.8.4"), failing `wrangler dev`. Check the installed major.minor
and reinstall ^0.7 when it is absent or mismatched.
ChristianPavilonis
dismissed
their stale review
Superseded by follow-up approval review; duplicate findings are carried forward there.
ChristianPavilonis
approved these changes
Jun 22, 2026
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
Collaborator
There was a problem hiding this comment.
Review Summary
😃👍 Approving per maintainer direction. CI is green on the current head, and the remaining items below are follow-up hardening / test-fidelity comments. I used CREG-style markers and included suggestion blocks where the fix is mechanical.
CI / Existing Reviews
Latest checks are passing. I dismissed my previous CHANGES_REQUESTED review because the relevant findings are superseded here.
Resolve conflicts from main's crate rename (js→trusted-server-js, openrtb→trusted-server-openrtb, integration-tests→trusted-server-integration-tests), toolchain bump (1.95.0), and the edgezero #269 repin. Key resolutions: - Keep the branch's pinned edgezero rev 38198f9 and fastly 0.11.12. That rev's Body::into_bytes returns Bytes (not Option) and the Fastly 0.11 TLS getters return Option directly, so main's #269 glue (.into_bytes().unwrap_or_default(), .ok().flatten() on TLS getters, oneshot().expect()/?) is reverted to the branch's API shape. Forward repin to fastly 0.12 is deferred to its own PR. - Adopt toolchain 1.95.0 but keep both wasm targets (Cloudflare needs wasm32-unknown-unknown). - Take the platform-neutral EC KV rewrite (ec/kv.rs) over main's Fastly-coupled version; main's changes there were cosmetic. - Carry main's new features onto the branch: tester-cookie endpoints (/_ts/set-tester, /_ts/clear-tester) wired into the Fastly router, and the osano integration. - Fix the Cloudflare adapter's trusted-server-js path after the rename, drop the leftover renamed-away crate dirs, and restore repo-wide node_modules/dist ignores (the old crates/js/.gitignore was removed by the rename). All adapters build, lint, and test green (fastly+core, axum, cloudflare).
…ezero-pr18-phase5-verification # Conflicts: # .github/workflows/integration-tests.yml
…ezero-pr18-phase5-verification # Conflicts: # crates/trusted-server-integration-tests/Cargo.lock
prk-Jr
changed the base branch from
feature/edgezero-pr16-axum-dev-server
to
main
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
paritytest binary incrates/integration-teststhat drives the Axum and Cloudflare adapters with identical requests in-process, proving behavioral equivalence before cutoverChanges
crates/trusted-server-adapter-cloudflare/tests/routes.rscrates/trusted-server-adapter-axum/tests/routes.rscrates/trusted-server-adapter-cloudflare/Cargo.tomlbase64dev-dependencycrates/trusted-server-adapter-axum/Cargo.tomlbase64dev-dependencycrates/integration-tests/Cargo.toml[[test]] paritybinary + adapter path deps + edgezero git depscrates/integration-tests/tests/parity.rscrates/trusted-server-core/src/platform/http.rsPlatformResponse::backend_nameunit tests (error-correlation, pre-EdgeZero #213)crates/trusted-server-core/src/html_processor.rscrates/trusted-server-core/Cargo.toml[[bench]] html_processor_benchentrycrates/trusted-server-core/benches/html_processor_bench.rs.github/workflows/test.ymltest-parityCI job + benchmark smoke step intest-axumjobdocs/superpowers/plans/2026-05-20-pr18-phase5-verification.mdCloses
Closes #499
Test plan
cargo fmt --all -- --checkcargo clippy-axumcargo test-axum(16 tests pass)cargo test-cloudflare(22 tests pass)cargo test --manifest-path crates/integration-tests/Cargo.toml --test parity(8 tests pass)cargo test --lib -p trusted-server-core(956 tests pass)cargo bench -p trusted-server-core --bench html_processor_bench -- --test(smoke passes)cargo test-fastly(Viceroy-based — not run locally; covered by existing CI job)Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)