Migrate handler layer to HTTP types (PR 12)

[prk-Jr]

Summary

• Move the PR 12 handler boundary to http request/response types so core handlers no longer depend on Fastly request/response APIs.
• Keep Fastly isolated to the adapter entry/exit path and the still-unmigrated integration/provider boundary, which keeps PR 13 scope separate.
• Lock the migrated surface with tests and migration guards so later changes do not accidentally reintroduce handler-layer Fastly types.

Changes

File	Change
crates/trusted-server-adapter-fastly/src/main.rs	Move the conversion boundary to the adapter entry/exit path and route core handlers with HTTP-native requests and responses.
crates/trusted-server-core/src/auction/endpoints.rs	Migrate the /auction handler to http types and rebuild a Fastly request only at the provider-facing AuctionContext boundary.
crates/trusted-server-core/src/auction/formats.rs	Consume HTTP requests directly and return HTTP auction responses.
crates/trusted-server-core/src/auction/orchestrator.rs	Keep provider parsing Fastly-based while making the provider response conversion explicit in the orchestrator.
crates/trusted-server-core/src/compat.rs	Add the minimal borrowed HTTP-to-Fastly request bridge needed for the remaining provider boundary.
crates/trusted-server-core/src/geo.rs	Make response header injection operate on HTTP responses.
crates/trusted-server-core/src/integrations/google_tag_manager.rs	Adapt the integration boundary so the shared proxy helper can stay HTTP-native while the integration trait remains Fastly-based.
crates/trusted-server-core/src/integrations/gpt.rs	Adapt the integration boundary so the shared proxy helper can stay HTTP-native while the integration trait remains Fastly-based.
crates/trusted-server-core/src/integrations/testlight.rs	Adapt the integration boundary so the shared proxy helper can stay HTTP-native while the integration trait remains Fastly-based.
crates/trusted-server-core/src/migration_guards.rs	Extend the guard to cover the handler modules migrated in PR 12.
crates/trusted-server-core/src/proxy.rs	Migrate first-party proxy/click/sign/rebuild handlers and response rewriting to HTTP request/response types.
crates/trusted-server-core/src/publisher.rs	Migrate publisher handlers to HTTP types and use the platform HTTP client for publisher-origin fetches.
crates/trusted-server-core/src/request_signing/endpoints.rs	Migrate request-signing and admin handlers to HTTP request/response types.

Closes

Closes #493

Test plan

• cargo test --workspace
• cargo clippy --workspace --all-targets --all-features -- -D warnings
• cargo fmt --all -- --check
• JS tests: cd crates/js/lib && npx vitest run (not run; no JS/TS files changed)
• JS format: cd crates/js/lib && npm run format
• Docs format: cd docs && npm run format
• WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
• Manual testing via fastly compute serve
• Other: verified remaining Fastly usage is limited to the adapter boundary and the still-unmigrated integration/provider boundary

Checklist

• Changes follow CLAUDE.md conventions
• No unwrap() in production code — use expect("should ...")
• Uses tracing macros (not println!)
• New code has tests
• No secrets or credentials committed

[aram356]

Summary

Second-round review. The PR addresses all four blocking findings and both questions from the prior review — unbounded body → String in sign/rebuild, silent JSON serialization fallback, geo lookup before auth, silent EdgeBody::Stream drop (now test-pinned in compat.rs), TODO(PR13) marker for the integration trait, and the body-drop comment on to_fastly_request_ref. CI is green.

Two new blocking issues — public POST endpoints /verify-signature and /auction still accept unbounded bodies — are the only security-material gaps in this round. The rest are follow-ups and polish.

Blocking

🔧 wrench

• Unbounded body on /verify-signature (crates/trusted-server-core/src/request_signing/endpoints.rs:89) — public endpoint materializes the full request into memory with no cap. See inline.
• Unbounded body on /auction (crates/trusted-server-core/src/auction/endpoints.rs:43) — public endpoint, same pattern. See inline.

Non-blocking

🤔 thinking

• platform_response_to_fastly still duplicated (crates/trusted-server-core/src/auction/orchestrator.rs:19-40 vs crates/trusted-server-core/src/compat.rs:119-138) — the prior review flagged this. Now consistent on append_header and documented as PR 15 removal, which is an improvement, but platform_resp.response is already http::Response<EdgeBody> and can be passed to compat::to_fastly_response directly. See inline.
• Admin endpoints also lack body-size caps (crates/trusted-server-core/src/request_signing/endpoints.rs:170, 270) — basic-auth-protected so attack surface is narrow, but defence-in-depth is cheap once RequestTooLarge exists. See inline.
• EdgeBody::Stream silent drop in release (crates/trusted-server-core/src/compat.rs:74-77, 132-135 and crates/trusted-server-core/src/auction/orchestrator.rs:29-32) — tests at compat.rs:584-629 pin the drop-to-empty behaviour, and debug_assert! catches it in debug, but in release a future streaming backend would silently return a 200 with an empty body. Acceptable transient debt given PR 15 removal scope — please carry this into the PR 15 plan so streaming isn't silently lost when the boundary moves.

♻️ refactor

• No 413 regression tests for the new size-capped endpoints (crates/trusted-server-core/src/proxy.rs:884-891 and :1007-1014) — the 65536-byte cap is a security control with no negative test. GTM has the pattern worth copying (crates/trusted-server-core/src/integrations/google_tag_manager.rs:1100-1137). See inline.

📝 note

• response_headers validation uses .expect() in the hot path (crates/trusted-server-adapter-fastly/src/main.rs:249-255). Correct today because Settings::prepare_runtime validates at load (crates/trusted-server-core/src/settings.rs:486-498), but route_request keeps a defensive fallback for the handler regex with the same "validated at load time" invariant (main.rs:132-138). Consider matching that pattern — warn-skip instead of panic — for symmetry and one less panic site in the wasm binary.

CI Status

• browser integration tests: PASS
• integration tests: PASS
• prepare integration artifacts: PASS

[aram356]

Summary

Round 3 review. The PR cleanly addresses every blocking finding from the prior two rounds: /verify-signature and /auction now enforce body-size caps (4 KiB and 64 KiB), platform_response_to_fastly was deleted from auction/orchestrator.rs (both call sites use crate::compat::to_fastly_response directly), and /admin/keys/{rotate,deactivate} got matching 4 KiB caps. The expect() in finalize_response (main.rs:251-253) is now safe because Settings::prepare_runtime validates response_headers at startup (settings.rs:486-498).

One new blocking finding: to_fastly_request_ref (introduced in PR 12) silently collapses multi-value headers, which is a latent correctness regression in the provider path. See inline.

CI is green; local cargo test --workspace (814 passed), cargo clippy --workspace --all-targets --all-features -- -D warnings, and cargo fmt --all -- --check all pass.

Blocking

🔧 wrench

• to_fastly_request_ref clobbers multi-value headers: see inline at compat.rs:90.

Non-blocking

🤔 thinking

• VERIFY_MAX_BODY_BYTES = 4096 may be tight for legitimate signed payloads (request_signing/endpoints.rs:78). After base64 signature (~88 bytes for Ed25519), kid, JSON wrapper, and quoting/escaping, the usable payload budget is ~3 KiB. Plausibly fine for typical OpenRTB-shaped payloads but can clash with publishers signing larger blobs (e.g. embedded events). Consider a brief // safety: comment explaining the chosen limit, or making it config-derived alongside the existing GTM max_beacon_body_size pattern.

📝 note

• Body-size caps fire after the body is already materialized into memory (compat.rs:40-43). compat::from_fastly_request calls req.take_body_bytes() at the adapter entry, so the entire body is buffered before any handler-level cap runs. The new caps protect parse allocations but not the receive allocation. This matches the compat shim's semantics today and is a PR-15 concern, not PR-12 — worth pinning in the PR 15 plan that streaming-aware caps need to land with the boundary removal so the protection isn't quietly downgraded when the shim is replaced.

♻️ refactor

• No 413 regression tests for /verify-signature, /auction, /admin/keys/{rotate,deactivate} (request_signing/endpoints.rs:93,183,292, auction/endpoints.rs:45). This commit added sign_rejects_oversized_body and rebuild_rejects_oversized_body (proxy.rs:2358-2395) — exactly what the previous review asked for. The same pattern wasn't applied to the four endpoints whose caps are introduced in this commit. Without negative tests, a future commit that drops any cap won't be caught. Same shape as the existing proxy tests — five lines per endpoint.

• Body-size-cap pattern duplicated 6× across handlers. if body.len() > MAX { return Err(Report::new(RequestTooLarge { format!("X payload {} exceeds limit of {}", body.len(), MAX) })); } appears at proxy.rs:884, proxy.rs:1007, auction/endpoints.rs:45, and three times in request_signing/endpoints.rs (93, 183, 292). A small helper (fn enforce_max_body_size(bytes: &[u8], limit: usize, what: &'static str) -> Result<(), Report<TrustedServerError>>) in http_util would drop ~50 lines and centralize the message format. Defer if you'd rather land in a follow-up.

CI Status

• browser integration tests: PASS
• integration tests: PASS
• prepare integration artifacts: PASS

[aram356]

Summary

Round-4 review. Round-3's blocking finding (to_fastly_request_ref clobbering multi-value headers) is resolved at compat.rs:95 with a regression test at compat.rs:478. CI is green.

Two new concerns this round: (1) the round-3 ♻️ ask for 413 regression tests on the four endpoints whose caps were introduced in this PR is still unaddressed — only the proxy endpoints got sign_rejects_oversized_body/rebuild_rejects_oversized_body. (2) Two doc-vs-code mismatches in compat.rs — the # Panics blocks describe a panic site that doesn't exist (URL parsing has a silent fallback to /), while the real panic site (header construction) is undocumented.

Blocking

🔧 wrench

• No 413 regression tests for /verify-signature, /admin/keys/{rotate,deactivate} (request_signing/endpoints.rs:103, :254, :381). Round-3 explicitly asked. See inline.
• No 413 regression test for /auction; file has no #[cfg(test)] module (auction/endpoints.rs:45). See inline.

Non-blocking

🤔 thinking

• # Panics doc claims URL-parse panic that cannot occur (compat.rs:42-44, compat.rs:56-58). Doc/code mismatch. See inline.
• Silent URL fallback to / masks malformed URIs (compat.rs:18-21). build_http_request uses unwrap_or_else(|_| http::Uri::from_static("/")). Realistically unreachable given Fastly pre-validation, but produces silent misrouting (catch-all hits the publisher origin at main.rs:215) if it ever fires. Either document why the fallback is reachable-but-safe, or assert the invariant with expect("should parse Fastly-validated URL").

♻️ refactor

• 65536 literal duplicated 4× in proxy.rs (proxy.rs:884, :887, :1007, :1010) — the only handler in this PR not using named constants. See inline.
• String::from_utf8(body_bytes.to_vec()) allocates twice (proxy.rs:892, :1015). See inline.
• Body-size cap pattern duplicated 6× (carry-over from round 3). Same if body.len() > MAX { return Err(RequestTooLarge { format!(...) }) } shape at proxy.rs:884, proxy.rs:1007, auction/endpoints.rs:45, and three times in request_signing/endpoints.rs (103, 254, 381). A small enforce_max_body_size(bytes, limit, what) helper in http_util would centralize the message format and remove ~50 lines.

🌱 seedling

• DEFAULT_PUBLISHER_FIRST_BYTE_TIMEOUT = 15s still hardcoded (publisher.rs:21). Carry-over from round 1 — worth a config knob during PR-15 boundary cleanup.
• Body-size caps still fire after req.take_body_bytes() materializes (compat.rs:46). Carry-over from round 3 — caps protect parse allocations but not the receive allocation. PR-15 streaming-aware shim should land alongside boundary removal so the protection isn't quietly downgraded.

📝 note

• platform_response_to_fastly is now the only diverging conversion (compat.rs:245-264). Returns Err on streaming bodies instead of to_fastly_response's silent-drop. The asymmetry is correct for orchestrator use, but worth pinning in the PR-15 plan so streaming closes consistently.

CI Status

• browser integration tests: PASS
• integration tests: PASS
• prepare integration artifacts: PASS

[aram356]

Summary

Re-review of the handler-layer HTTP-types migration. The migration is structurally clean, but two concerns block merge: (1) the publisher streaming dispatch added in #562 is regressed — PublisherResponse::Stream now buffers the entire pipeline output instead of writing chunks via stream_to_client(), and (2) cargo test --workspace fails on the current head because the debug_assert! in compat::to_fastly_response (introduced in PR 11 and inherited here) contradicts the test that exercises the documented silent-drop fallback. CI did not catch (2) because PR 624's base branch causes test.yml and format.yml to skip.

Blocking

🔧 wrench

• Streaming regression on the publisher fallback path. Before this PR, Ok(PublisherResponse::Stream { … }) in adapter-fastly/src/main.rs called response.stream_to_client() and wrote chunks via stream_publisher_body. After this PR, resolve_publisher_response materializes the entire pipeline output into a Vec<u8>, sets Content-Length, replaces the body, and the adapter sends via compat::to_fastly_response(response).send_to_client(). The whole publisher response is now buffered in WASM memory.

  Observable: the PublisherResponse::Stream enum variant becomes equivalent to PublisherResponse::Buffered until streaming is restored. TTFB is delayed by the full pipeline pass, and WASM memory pressure scales with body size.

  Structural cause: compat::to_fastly_response cannot move an EdgeBody::Stream into a fastly::StreamingBody — there is no streaming bridge across the compat shim. Two reasonable directions:

  • Preferred: Keep the streaming dispatch in the adapter. Have route_request return Result<HandlerOutcome, …> where HandlerOutcome::Streaming(…) carries the unbuffered body+params, and main() opens stream_to_client() on the converted Fastly response and calls stream_publisher_body against it. Honest "no behavior regressions" version of PR 12.
  • Alternative: Block the streaming arm and route everything through BufferedProcessed until PR 15 introduces a streaming body bridge — but explicitly call this out as a known regression in the PR description and add a tracking issue.

  No test today asserts streaming behavior. The publisher tests (stream_publisher_body_*) all use Vec<u8> outputs and don't distinguish streamed-to-client from buffered-then-sent. So the regression is entirely silent.

• cargo test --workspace fails: compat::tests::to_fastly_response_with_streaming_body_produces_empty_body panics on a debug_assert! that contradicts the test.

  In crates/trusted-server-core/src/compat.rs (line ~138), the function asserts matches!(&body, EdgeBody::Once(_)). The test below it (line ~676) constructs an EdgeBody::Stream and asserts the body comes out empty — so the test panics in cargo test debug builds.

  Both the debug_assert! and the test were introduced in PR 11 (commit 76df6f8), so PR 624 inherits the contradiction rather than introducing it — but it materializes here because (a) the PR's test plan checkbox [x] cargo test --workspace does not pass on the current head, and (b) when this stack lands on main and a downstream PR opens, the standard test.yml workflow will fail. The PR 11 review can fix it upstream, or this PR can apply the fix.

  Fix (preferred — the warn-and-empty fallback is documented behavior):

  pub fn to_fastly_response(resp: http::Response<EdgeBody>) -> fastly::Response {
      // … build headers …
      match body {
          EdgeBody::Once(bytes) => {
              if !bytes.is_empty() {
                  fastly_resp.set_body(bytes.to_vec());
              }
          }
          EdgeBody::Stream(_) => {
              log::warn!("streaming body in compat::to_fastly_response; body will be empty");
          }
      }
      fastly_resp
  }

  Alternative: keep the debug_assert! and gate the test on #[cfg(not(debug_assertions))]. But the warn-log already covers misuse and the documented fallback path should be exercised by tests.

Non-blocking

🤔 thinking

• Silent body drop on EdgeBody::Stream in compat. Both to_fastly_request and to_fastly_response log a warning and emit an empty body when handed a streaming EdgeBody. Combined with the streaming-dispatch removal above, this means a future caller that tries to stream through the compat boundary will silently see truncated bodies. Promote the misuse to a Result return (the Fastly adapter's edge_request_to_fastly already does this), or add a comment listing the audited non-streaming call sites so the constraint is verifiable. Not in PR 12's diff but the streaming-regression fix will reintroduce the constraint.

♻️ refactor

• bytes.to_vec() copies the full body across the compat boundary. compat.rs:144-146 and compat.rs:81-84 memcpy the body. For large publisher responses this is unavoidable O(N) at the shim. Worth a one-line comment that this cost goes away with PR 15 so future readers don't try to optimize inside the shim. (Not in PR 12's diff.)

🌱 seedling

• Sync between route_request admin routes and Settings::ADMIN_ENDPOINTS is comment-only. main.rs:166 carries a "Keep in sync with Settings::ADMIN_ENDPOINTS" comment. A test that compares the constant against the routes registered in route_request would prevent future drift. Follow-up issue.

📝 note

• CI workflow gate gap. .github/workflows/test.yml and .github/workflows/format.yml trigger only on pull_request: branches: [main]. PR 624 targets the PR 11 feature branch, so cargo test, fmt, and clippy never ran on this PR. Only integration-tests.yml ran (and passed). This is how the failing compat test stayed invisible. Out of scope for PR 12 to fix — but the consequence (a regression that will only surface when this lands on main and a downstream PR opens) is in scope. Either drop the branches: [main] filter for the EdgeZero stack PRs, or workflow_dispatch the test workflow against each stack head.

CI Status

• fmt: PASS (locally)
• clippy: PASS (locally)
• rust tests: FAIL locally (compat::tests::to_fastly_response_with_streaming_body_produces_empty_body); not run on CI for this PR
• integration tests: PASS (CI)
• browser integration tests: PASS (CI)

[aram356]

🤔 thinking — 401 responses no longer carry geo headers — silent behavior change.

Previously geo_info was looked up before routing and applied via finalize_response to every path including auth early-returns, so 401s had geo headers. Now geo_info = if status == UNAUTHORIZED { None } else { … } skips the lookup on 401.

This may be intentional (avoid leaking geo to unauthenticated callers) but it's a silent change. Add a one-line comment to make the rationale explicit:

// Skip geo lookup for 401s so geo headers are not exposed to unauthenticated
// callers. Authenticated routes do their own lookups for consent context.
let geo_info = if response.status() == StatusCode::UNAUTHORIZED { … };

[aram356]

📝 note — TODO(PR13) is captured. The bridge through compat::to_fastly_request / compat::from_fastly_response is correct for the scope of this PR. No action here — confirming the deferred work is tracked.

[aram356]

🤔 thinking — Geo lookup is now duplicated.

The handler does its own lookup here for the consent context, and the adapter does another lookup at response-finalization time for header injection (main.rs finalize_response). Lookup is cheap (in-memory on Fastly) so this is more of a 📌 out of scope note — a follow-up could thread the same Option<GeoInfo> through both. Same pattern in auction/endpoints.rs:75-81.

[aram356]

📝 note — fastly::geo::Geo import remains in core after this migration. The geo_from_fastly field-mapping helper still lives here, so crates/trusted-server-core retains a Fastly type leak after PR 12. Predates this PR and is acknowledged out of scope, but worth tracking for PR 13 / PR 15 since geo.rs is one of the migrated files.

[aram356]

♻️ refactor — Dedup the match Result { Ok(r) => r.status(), Err(e) => e.current_context().status_code() } block.

It appears twice in this file (auction + publisher cases). A small helper at the top of the test module would clean this up:

fn status_of(
    result: &Result<HttpResponse, Report<TrustedServerError>>,
) -> StatusCode {
    match result {
        Ok(resp) => resp.status(),
        Err(e) => e.current_context().status_code(),
    }
}

Not a blocker — duplication is small.

[aram356]

🔧 wrench — Streaming regression: this arm now buffers the full pipeline output instead of streaming chunks to the client.

Before this PR, the adapter handled Ok(PublisherResponse::Stream { … }) by calling response.stream_to_client() and writing chunks via stream_publisher_body against the StreamingBody — the streaming pipeline added in #562 (commit 5ff3e676) emitted bytes as the upstream body was processed.

After this PR, resolve_publisher_response materializes the entire pipeline output into a Vec<u8>, sets Content-Length, replaces the body, and the adapter sends via compat::to_fastly_response(response).send_to_client(). The whole publisher response is now buffered in WASM memory:

• PublisherResponse::Stream becomes observably equivalent to PublisherResponse::Buffered — the variant no longer means "stream".
• TTFB is delayed by the full pipeline pass rather than starting at first chunk.
• WASM memory pressure scales with body size for every publisher response.

Structural cause: compat::to_fastly_response cannot move an EdgeBody::Stream into a fastly::StreamingBody — there is no streaming bridge across the compat shim, so the only way to honor the variant is to streaming-dispatch from the adapter itself before going through compat.

Fix (preferred) — keep the streaming dispatch in the adapter:

// Have route_request return Result<HandlerOutcome, …> where HandlerOutcome
// distinguishes streamed vs buffered responses, then in main():
match outcome {
    HandlerOutcome::Streaming { mut response, body, params } => {
        finalize_response(&settings, geo_info.as_ref(), &mut response);
        let mut fastly_resp = compat::to_fastly_response_skeleton(response);
        let mut sb = fastly_resp.stream_to_client();
        if let Err(e) = stream_publisher_body(body, &mut sb, &params, &settings, &integration_registry) {
            log::error!("Streaming processing failed: {e:?}");
            drop(sb);
        } else if let Err(e) = sb.finish() {
            log::error!("Failed to finish streaming body: {e}");
        }
    }
    HandlerOutcome::Buffered(response) => {
        compat::to_fastly_response(response).send_to_client();
    }
}

Alternative — fold this arm into BufferedProcessed until PR 15 introduces a streaming body bridge, and call it out as a known regression in the PR description with a tracking issue.

No test today asserts streaming behavior — the publisher tests (stream_publisher_body_*) all use Vec<u8> outputs and don't distinguish streamed-to-client from buffered-then-sent. So the regression is entirely silent until you measure TTFB or memory.