Add Phase 5 cross-adapter verification suite (PR 18)#725
Conversation
…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.
- Implement CloudflareHttpClient (wasm32 only) using worker::Fetch for real
outbound proxy requests; strip content-encoding/transfer-encoding headers
since the Workers runtime auto-decompresses responses
- Add build.sh with cd-to-SCRIPT_DIR guard so worker-build always runs from
the correct crate root regardless of invocation directory
- Switch wrangler dev task to use --cwd from workspace root (same DX as Fastly)
- Add js-sys to workspace dependencies; reference it via { workspace = true }
- Fix #[ignore] messages on Cloudflare integration tests
- Replace std::time::{SystemTime,UNIX_EPOCH} with web_time in test code for
signing.rs and proxy.rs (consistency with production paths)
- Add NoopConfigStore/NoopSecretStore TODO comment tracking the gap
- Add extract_client_ip unit tests (parses cf-connecting-ip, absent, invalid)
- Remove empty crate_compiles_on_host_target test
- Add CloudflareHttpClient timeout doc noting Workers CPU-budget tradeoff
Replace direct worker::Env store construction with edgezero handles already injected by run_app, reducing #[cfg(target_arch = "wasm32")] blocks from 5 to 2. - ConfigStoreHandleAdapter: bridges ctx.config_store() to PlatformConfigStore — reuses the already-parsed TRUSTED_SERVER_CONFIG JSON handle rather than re-parsing it on every request - KvHandleAdapter: bridges ctx.kv_handle() to PlatformKvStore — reuses the env.kv() handle opened by run_app rather than opening a new one - CloudflareGeo: moved outside #[cfg]; reads cf-ipcountry and related headers via ctx.request().headers() which needs no platform import - CloudflareSecretStoreAdapter: kept under #[cfg] — env.secret() is synchronous at the JS level but PlatformSecretStore::get_bytes is sync while SecretHandle::get_bytes is async; direct env access is required - Add .dev.vars to .gitignore (wrangler convention for local secrets) - Add bytes workspace dep for KvStore impl
…ezero-pr17-cloudflare-adapter
- Implement CloudflareWorkers::spawn() to start wrangler dev; in CI uses wrangler.ci.toml (no build step, uses pre-built bundle); locally uses wrangler.toml (triggers build.sh rebuild) - Add wrangler.ci.toml: no [build] section so wrangler dev skips the worker-build step when the bundle is pre-built as a CI artifact - Add build-cloudflare input to setup-integration-test-env: adds wasm32-unknown-unknown target and runs build.sh with integration test env vars - In prepare-artifacts: enable build-cloudflare and upload build/ dir as artifact - In integration-tests: restore CF build dir, install wrangler, set CLOUDFLARE_WRANGLER_DIR, and remove --skip flags for cloudflare tests
- Add GeoInfo happy-path test: build_geo_lookup_returns_some_with_populated_country verifies country, city, continent, latitude, and longitude are correctly populated when Cloudflare headers are present - Simplify CloudflareSecretStoreAdapter::get_bytes: collapse brittle JsError string-matching guards into a single error arm with contextual message - Document sequential fan-out latency in CloudflareHttpClient: explain sum(DSP_i) vs max(DSP_i) implication and why true parallelism is blocked by the ?Send bound on PlatformHttpClient - Fix stale wrangler.toml comment: update to reflect ConfigStoreHandleAdapter wiring rather than the now-fallback NoopConfigStore - Extend CI triggers to feature branches: format.yml and test.yml now run fmt/clippy/tests on PRs targeting feature/** so stacking PRs are gated - Fix fmt violations caught during pre-review verification: platform.rs two-line Err wrapping and plan doc Prettier formatting
The adapter's tokio dev-dependency uses rt-multi-thread which is not supported on wasm32. It is already tested in the dedicated test-cloudflare job; exclude it from the workspace wasm test to avoid the compile error.
Resolved conflicts: - .cargo/config.toml: extend test-fastly alias to also exclude trusted-server-adapter-cloudflare - .github/workflows/test.yml: use cargo test-fastly alias in CI (from PR16) and keep test-cloudflare job (from PR17) - CLAUDE.md: keep Cloudflare workspace entry and cargo test-axum alias; merge both adapters' commands - crates/trusted-server-core/src/proxy.rs: use #[tokio::test] + std::time pattern (consistent with PR16 tests) - Cargo.lock: regenerated to reflect merged workspace state
Brings in the default-members fix: restricts workspace default-members to `trusted-server-adapter-fastly` so Viceroy can locate the binary via `cargo run --bin` during `cargo test-fastly`.
Blocking: - Fix cargo fmt failure in integration-tests cloudflare.rs (long import, struct init) - Replace `use worker::*` with explicit imports in adapter lib.rs - Return generic 500 body from top-level dispatch error; log detail server-side - Fix workerd process-group leak: spawn wrangler as group leader, killpg on drop - Use find_available_port() for wrangler dev instead of hardcoded 8787 - Reject multi-provider fan-out in select() with PlatformError::HttpClient instead of a noisy warn; per-provider timeout is now enforced by failing loudly rather than silently degrading to sum(latencies) - Fix clippy::type_complexity in axum platform.rs with ResponseTriplet alias - Fix docs prettier formatting Non-blocking: - Return Err from execute() on Body::Stream outbound instead of silent empty body - Assert unreachable! on Body::Stream in send_async (execute always returns Once) - Extract shared dispatch() helper from get_fallback/post_fallback duplicates - Rename auth test to auth_middleware_runs_in_chain_for_protected_routes - Update stale #[ignore] reasons for Cloudflare integration tests
- Merge latest PR16 base (bff1adc, 314575b): removes unused backend.rs and fixes .gitignore comment - Use SpawnedRequestResult type alias from PR16 (includes Result wrapper) - Add check-cloudflare alias for wasm32-unknown-unknown target check - Add clippy-cloudflare alias to complete the per-adapter clippy pattern
…ezero-pr17-cloudflare-adapter
Adds 10 tests to tests/routes.rs covering every explicitly registered route plus the tsjs catch-all wildcard. Assertions are scoped to routing only (not 404) for handlers that require live settings or outbound connections, matching the pattern established by the existing auction test.
Verifies that /admin/* routes return 401 without credentials, include a WWW-Authenticate: Basic realm=... header, and reject wrong credentials; also confirms /.well-known and /auction are not gated by admin auth.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #725 against feature/edgezero-pr17-cloudflare-adapter, including the cross-adapter parity suite, adapter route/auth changes, CI workflow updates, and HTML/error-correlation tests. CI is currently green. I did not find blocking correctness, security, data-loss, authorization, or severe compatibility issues. I found one remaining medium-confidence test-fidelity gap where a parity assertion can still pass without comparing response bodies.
Findings
P0 / Blockers
None.
P1 / High
None.
P2 / Medium
- Discovery body parity still passes when both responses are non-JSON —
crates/integration-tests/tests/parity.rs:213- Issue: The test parses both bodies as
Option<Value>and compares those options. If both adapters return non-JSON error bodies, both parse results areNone, so the assertion passes without comparing the actual bodies. With the current in-process settings/runtime services,handle_trusted_server_discoverycan fail before producing the discovery JSON because the request-signing config store/JWKS are unavailable, making this a realistic path rather than only a theoretical edge case. - Why it matters: This suite is intended to prove adapter parity. A regression where Axum and Cloudflare return different discovery error payloads, or both stop returning the JSON discovery document, can still pass this test.
- Suggested fix: Either seed the test runtime with signing/JWKS data and assert both parsed JSON values are
Someand equal, or compare raw body bytes/content type when parsing fails soNone == Noneis not treated as body parity.
- Issue: The test parses both bodies as
P3 / Low
None.
CI / Existing Reviews
GitHub checks are green on the current head, including fmt, Rust tests, Cloudflare checks, cross-adapter parity, JS tests, docs/TS formatting, and browser/integration tests. Prior blocking review threads around route introspection, auth regex/path coverage, clippy gating, and status/header parity appear addressed at HEAD.
The discovery body parity assertion compared Option<Value> parse results, so two non-JSON error bodies both became None and the assertion passed without comparing the bodies. On the in-process path both adapters return a 500 plain-text error when signing/JWKS data is unavailable, so this was a realistic gap, not a theoretical one. Assert both adapters agree on body type (JSON vs non-JSON), compare parsed JSON values when present, and fall back to raw byte comparison otherwise so diverging error payloads are caught.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #725 against feature/edgezero-pr17-cloudflare-adapter, focusing on the new cross-adapter verification suite, adapter route/auth wiring, and CI gates. CI is green, and the prior parity-test assertion gaps appear addressed. I found one blocking authorization issue: the branch keeps real /admin/keys/* legacy key-management aliases registered while the production-shaped/default auth regex and Settings::ADMIN_ENDPOINTS only protect /_ts/admin/keys/*.
Findings
P0 / Blockers
- See inline comment: legacy admin key aliases can bypass Basic auth under the default production admin handler coverage.
P1 / High
None.
P2 / Medium
None.
P3 / Low
None.
CI / Existing Reviews
GitHub checks are currently green, including fmt, Rust tests, Cloudflare checks, cross-adapter parity, JS tests, docs/TS formatting, and browser/integration tests. Existing review feedback around route-introspection, header/status parity, clippy gating, canonical admin route coverage, and discovery body comparison appears addressed at HEAD.
The Cloudflare and Fastly adapters registered legacy `/admin/keys/*` aliases that routed to the real handle_rotate_key/handle_deactivate_key handlers. The production basic-auth handler regex `^/_ts/admin` does not match those paths, so AuthMiddleware never challenged them and unauthenticated callers could reach the key handlers. Remove the aliases from the Cloudflare router and from both Fastly routers (the EdgeZero NAMED_ROUTES table and the legacy route_request match); unrouted, they fall through to the publisher/organic fallback like any unknown path. The canonical `/_ts/admin/keys/*` routes stay auth-gated. Drop the stale aliases from Cloudflare's route-registration test, retarget the Fastly consent-store admin test to the canonical path, and replace the Fastly alias test with a production-shaped guard asserting the legacy paths are not in NAMED_ROUTES. Add a Cloudflare parity regression test asserting the aliases are not auth-gated admin routes and fall through like an unknown path. Note: the Axum adapter already routes every admin path to AdminNotSupported (501) without reaching a key handler, so it carries no equivalent unauthenticated surface and is left unchanged.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #725 against feature/edgezero-pr17-cloudflare-adapter, including the current head d5bcd8a0dc5800a893e28be2144ac7159487d9c0, the adapter route/auth changes, the cross-adapter parity suite, core HTML/error-correlation tests, benchmark smoke gate, and CI workflow. GitHub checks are green, and I did not find blocking correctness, security, data-loss, authorization, or severe compatibility issues.
I left one inline P2 finding: the new Cloudflare legacy-admin-alias regression test should assert route-table absence directly, because the current behavioral comparison can still pass if a reintroduced unauthenticated key handler happens to return the same status as the publisher fallback in the no-store test environment.
CI / Existing Reviews
Current PR checks are passing, including fmt, Rust tests, Cloudflare checks, cross-adapter parity, JS tests, docs/TS formatting, and browser/integration tests. Earlier review threads around wildcard route smoke tests, header/status parity, clippy gating, canonical admin path coverage, discovery body comparison, and removal of unauthenticated legacy key aliases appear addressed at HEAD.
The legacy admin alias regression test compared runtime behavior against
an unknown publisher-fallback path. In the no-store/no-origin parity
environment a reintroduced unauthenticated key handler could fail with
the same status and carry no WWW-Authenticate header, letting the
regression slip through.
Add a direct route-table guard as the primary assertion, analogous to
the Fastly NAMED_ROUTES guard: collect cf_router().routes() and assert
the canonical POST /_ts/admin/keys/{rotate,deactivate} routes are present
while the legacy POST /admin/keys/{rotate,deactivate} aliases are absent.
The behavioral fallback check is kept as a secondary assertion.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #725 against feature/edgezero-pr17-cloudflare-adapter, focusing on the current head dab40a693bba4bdfc8ff7263ed13d92f9ea2460b, the adapter route/auth changes, the cross-adapter parity suite, CI gates, and core HTML/error-correlation additions. GitHub checks are green, and I also verified locally that cargo test --manifest-path crates/integration-tests/Cargo.toml --test parity --locked and cargo bench -p trusted-server-core --bench html_processor_bench -- --test pass.
I did not find blocking correctness, security, data-loss, authorization, or severe compatibility issues. I left one inline P2 finding: the Axum adapter still registers legacy /admin/keys/* aliases even though the same PR removes them from Fastly/Cloudflare for the ^/_ts/admin auth-coverage reason, leaving the route-parity story inconsistent.
CI / Existing Reviews
Current PR checks are passing, including fmt, Rust tests, Cloudflare checks, cross-adapter parity, JS tests, docs/TS formatting, and browser/integration tests. Earlier review threads around wildcard route smoke tests, header/status parity, clippy gating, canonical admin path coverage, discovery body comparison, and removal of unauthenticated legacy key aliases appear addressed for Fastly/Cloudflare at HEAD.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Round-4 review. All blocking and non-blocking findings from my previous review are genuinely resolved and the full CI suite — including the previously-red cargo test (cross-adapter parity) job — now passes. One new issue remains: an Axum/Fastly/Cloudflare divergence in the legacy admin-alias handling introduced by this round's changes.
Prior findings — verified resolved
- Parity CI job red (clippy
panic/collapsible_if/redundant_closure): fixed; parity job passes. - Admin auth tests used a broader regex than production:
test_settingsnow uses the production-shaped^/_ts/adminand tests exercise the canonical/_ts/admin/keys/*paths. - Auction parity asserted no status equality: now asserts
axum_status == cf_status(and not-404). - JSON parity via
is_some(): now compares parsedValues when both are JSON and raw bytes otherwise. unwrap_or_else(panic!)inbuild.rs: nowexpect("should ...").
The additional hardening — removing the unauthenticated legacy /admin/keys/* aliases (which were wired to the real key handlers but not covered by ^/_ts/admin) from Fastly and Cloudflare, with route-table guard tests — is a good catch.
Blocking
🔧 wrench
- Axum still registers the legacy
/admin/keys/*aliases (adapter-axum/src/app.rs:217-227) while this PR removes them from Fastly and Cloudflare, and the comment "kept for parity with the Fastly adapter" is now false. Low security impact on Axum (inert501handler), but it is a real cross-adapter divergence the parity suite does not catch. See inline comment for the suggested fix.
CI Status
- fmt: PASS
- cargo test (core): PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo check (cloudflare): PASS
- vitest / docs / ts format / integration / browser: PASS
The legacy non-`/_ts` admin aliases (`/admin/keys/*`) were still registered on the Axum adapter while this PR removes them from Fastly and Cloudflare, and the "kept for parity with the Fastly adapter" comment was now false. Remove the two legacy `NamedRoute` entries so unrouted `/admin/keys/*` requests fall through to the publisher fallback like any unknown path, the same behavior as the other adapters. Add a `legacy_admin_aliases_are_not_registered` route-table guard mirroring the Fastly and Cloudflare parity assertions so the divergence cannot silently reappear, and drop the stale legacy-alias 501 smoke tests (the canonical `/_ts/admin/keys/*` paths retain auth coverage).
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #725 against feature/edgezero-pr17-cloudflare-adapter, including the current head 74c51db08b3d50358b6afe5bf57c761ef963a093, the adapter route/auth updates, cross-adapter parity suite, CI gates, HTML golden tests, and benchmark smoke coverage. CI is green and local spot checks passed for the parity suite, Axum routes, Cloudflare routes, and the benchmark smoke run. I did not find blocking correctness, security, data-loss, authorization, or severe compatibility issues.
Findings
P0 / Blockers
None.
P1 / High
None.
P2 / Medium
- See inline comment: the Axum admin-route behavior changed from the legacy
/admin/keys/*aliases to canonical/_ts/admin/keys/*, but a published architecture guide still documents the old Axum route/501 behavior.
P3 / Low
- See inline comment: newly added test/benchmark examples introduce non-reserved
.comdomains, contrary to the repository convention to useexample.comdomains in tests/docs/examples.
CI / Existing Reviews
Current GitHub checks are 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, legacy admin alias removal, and Axum/Fastly/Cloudflare alias consistency appear addressed at HEAD. Local spot checks run during this review also passed: cargo test --manifest-path crates/integration-tests/Cargo.toml --test parity --locked, cargo bench -p trusted-server-core --bench html_processor_bench -- --test, cargo test-axum, and cargo test-cloudflare.
prk-Jr
changed the base branch from
feature/edgezero-pr17-cloudflare-adapter
to
feature/edgezero-pr16-axum-dev-server
Reconciles PR18 (Phase 5 verification) onto pr16 after pr17 was squash-merged
into pr16. pr18 branched from an earlier pr17, so this brings up pr16's newer
content (the pr15 base merge, axum dev server, and the pr17 review fixes) and
layers pr18's phase-5 work (cross-adapter parity tests, Criterion benchmarks,
CI gates, legacy admin-alias removal) on top.
Conflict resolutions:
- cloudflare/src/app.rs: keep pr18's testing seam (build_state_with_settings,
routes_with_settings, build_router) and the /_ts/admin/keys auth fix (legacy
/admin/keys aliases dropped), AND restore pr16's all-7-method publisher
fallback (GET/POST/HEAD/OPTIONS/PUT/PATCH/DELETE) + computed allow_tsjs.
- cloudflare/src/platform.rs, core/proxy.rs, core/platform/{mod,test_support}.rs:
pr18 did not change these vs pr17; take pr16's newer version.
- cloudflare/tests/routes.rs: keep pr18's expanded route/auth suite (superset);
compatible with the all-method fallback (presence-only assertions).
- cloudflare/Cargo.toml: keep pr18's base64 dev-dep.
- cloudflare/build.sh: keep pr16's `set -a` env-export fix (pr18 had the
unfixed version).
- .github/workflows/test.yml: keep pr18's test-parity CI job.
- html_processor.rs test + html_processor_bench.rs: add max_buffered_body_bytes
to HtmlProcessorConfig literals (field added on pr16 via the pr15 merge;
pr18 added the test/bench against the older struct).
- Cargo.lock + integration-tests/Cargo.lock: regenerated (adds worker-sys for
the parity tests; core's wasm getrandom block carried up from pr16).
- architecture.md (P2): the Axum admin-key limitation row referenced the legacy `/admin/keys/*` aliases, but PR18 dropped those aliases. Update it to the canonical `/_ts/admin/keys/*` (501 Not Implemented) and note the legacy paths now fall through to the publisher fallback. Other user-facing guides already use the canonical path. - html_processor golden test + html_processor_bench (P3): switch the new fixtures off real registrable domains to reserved `.example.com` names per the repo convention. Pre-existing test-publisher.com fixtures are out of scope.
build.sh only installed worker-build when none was on PATH, so a newer global worker-build (0.8+, which requires worker >= 0.8.4) would run against this crate's pinned worker 0.7 and fail `wrangler dev` with an "Unsupported version worker@0.7.5" error. Install a 0.7-series worker-build into a crate-local `.worker-build/` root and invoke it explicitly, so the build uses a compatible toolchain without requiring or disturbing the developer's global worker-build.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
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.
| WORKER_BUILD_ROOT="$SCRIPT_DIR/.worker-build" | ||
| WORKER_BUILD_BIN="$WORKER_BUILD_ROOT/bin/worker-build" | ||
| if ! "$WORKER_BUILD_BIN" --version 2>/dev/null | grep -qE '0\.7\.'; then | ||
| cargo install -q --version '^0.7' --root "$WORKER_BUILD_ROOT" worker-build |
There was a problem hiding this comment.
🔧 P2 / Medium — stale local worker-build installs are detected but not replaced
Issue: the guard correctly detects a non-0.7 .worker-build/bin/worker-build, but cargo install --root ... worker-build is run without --force. If that stale binary already exists, Cargo refuses to overwrite it and the script still fails until the developer manually removes .worker-build.
Why it matters: this script is specifically trying to recover from incompatible worker-build versions; the recovery path should be self-healing for wrangler dev.
| cargo install -q --version '^0.7' --root "$WORKER_BUILD_ROOT" worker-build | |
| cargo install -q --force --version '^0.7' --root "$WORKER_BUILD_ROOT" worker-build |
| // Only the canonical `/_ts/admin/keys/*` paths are routed. The production | ||
| // basic-auth handler regex `^/_ts/admin` does not match the legacy | ||
| // `/admin/keys/*` aliases, so routing them here would expose the key | ||
| // handlers to unauthenticated callers. Unmatched, they fall through to the | ||
| // organic/publisher path like any other unknown route. | ||
| (Method::POST, "/_ts/admin/keys/rotate") => { | ||
| (handle_rotate_key(settings, runtime_services, req), false) | ||
| } | ||
| (Method::POST, "/admin/keys/deactivate") | (Method::POST, "/_ts/admin/keys/deactivate") => { | ||
| ( | ||
| handle_deactivate_key(settings, runtime_services, req), | ||
| false, | ||
| ) | ||
| } | ||
| (Method::POST, "/_ts/admin/keys/deactivate") => ( | ||
| handle_deactivate_key(settings, runtime_services, req), | ||
| false, | ||
| ), |
There was a problem hiding this comment.
🔧 P1 / High — legacy admin aliases now proxy admin credentials to the publisher origin
Issue: removing the /admin/keys/* match arms makes those former admin endpoints fall through to the organic/publisher fallback. That fallback forwards the original request to the configured publisher origin, including Authorization and the JSON body, so stale clients/operators using the old alias can leak Trusted Server admin Basic credentials and key-management payloads to origin logs.
Why it matters: this is a security regression from the previous behavior (local admin handling/auth challenge). Unknown admin-looking paths should fail closed locally, not become publisher-origin traffic. Please apply equivalent deny/no-forward handling in the Fastly EdgeZero route table and the Axum/Cloudflare routers too, plus a regression test with an Authorization header proving the legacy aliases never reach the publisher fallback.
One possible Fastly legacy-path shape:
| // Only the canonical `/_ts/admin/keys/*` paths are routed. The production | |
| // basic-auth handler regex `^/_ts/admin` does not match the legacy | |
| // `/admin/keys/*` aliases, so routing them here would expose the key | |
| // handlers to unauthenticated callers. Unmatched, they fall through to the | |
| // organic/publisher path like any other unknown route. | |
| (Method::POST, "/_ts/admin/keys/rotate") => { | |
| (handle_rotate_key(settings, runtime_services, req), false) | |
| } | |
| (Method::POST, "/admin/keys/deactivate") | (Method::POST, "/_ts/admin/keys/deactivate") => { | |
| ( | |
| handle_deactivate_key(settings, runtime_services, req), | |
| false, | |
| ) | |
| } | |
| (Method::POST, "/_ts/admin/keys/deactivate") => ( | |
| handle_deactivate_key(settings, runtime_services, req), | |
| false, | |
| ), | |
| // Only the canonical `/_ts/admin/keys/*` paths execute key operations. | |
| // The legacy `/admin/keys/*` aliases are handled locally below so stale | |
| // clients do not leak admin Basic credentials or key-management payloads | |
| // to the publisher origin. | |
| (Method::POST, "/admin/keys/rotate" | "/admin/keys/deactivate") => ( | |
| Ok(HttpResponse::builder() | |
| .status(edgezero_core::http::StatusCode::NOT_FOUND) | |
| .body(EdgeBody::from("Not found\n")) | |
| .expect("should build legacy admin alias response")), | |
| false, | |
| ), | |
| (Method::POST, "/_ts/admin/keys/rotate") => { | |
| (handle_rotate_key(settings, runtime_services, req), false) | |
| } | |
| (Method::POST, "/_ts/admin/keys/deactivate") => ( | |
| handle_deactivate_key(settings, runtime_services, req), | |
| false, | |
| ), |
…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.
3 participants
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!)