Skip to content

Migrate utility layer to HTTP types (PR 11)#623

Open
prk-Jr wants to merge 3 commits intofeature/edgezero-pr10-abstract-logging-initializationfrom
feature/edgezero-pr11-utility-layer-migration-v2
Open

Migrate utility layer to HTTP types (PR 11)#623
prk-Jr wants to merge 3 commits intofeature/edgezero-pr10-abstract-logging-initializationfrom
feature/edgezero-pr11-utility-layer-migration-v2

Conversation

@prk-Jr
Copy link
Copy Markdown
Collaborator

@prk-Jr prk-Jr commented Apr 8, 2026

Summary

  • Migrate the PR11 utility layer off direct fastly::Request/fastly::Response usage so core helpers can operate on http::{Request, Response} and edgezero_core::Body.
  • Add a temporary compat bridge at Fastly boundaries so handlers and integrations can keep working while later migration PRs move the remaining call stack.
  • Lock in the migration with focused compat tests and a guard test that prevents the migrated utility modules from drifting back to Fastly request/response types.

Changes

File Change
Cargo.toml Add the workspace mime dependency used by migrated HTTP response helpers.
Cargo.lock Record the new mime dependency.
crates/trusted-server-adapter-fastly/src/main.rs Route forwarded-header sanitization and basic-auth response conversion through the new compat boundary.
crates/trusted-server-core/Cargo.toml Add the core crate's mime workspace dependency.
crates/trusted-server-core/src/auction/endpoints.rs Convert auction utility calls to use the HTTP request compat bridge for synthetic ID and consent handling.
crates/trusted-server-core/src/auction/formats.rs Bridge Fastly requests into migrated synthetic ID generation helpers.
crates/trusted-server-core/src/auth.rs Migrate basic-auth enforcement to http::Request/Response and update tests to HTTP builders.
crates/trusted-server-core/src/compat.rs Add Fastly↔HTTP request/response conversions plus temporary Fastly boundary shims for headers, cookies, and synthetic cookie handling.
crates/trusted-server-core/src/consent/extraction.rs Migrate consent signal extraction to http::Request<EdgeBody>.
crates/trusted-server-core/src/consent/mod.rs Move consent pipeline input types and tests onto HTTP request types.
crates/trusted-server-core/src/cookies.rs Migrate cookie parsing/forwarding and synthetic cookie response helpers to HTTP request/response types.
crates/trusted-server-core/src/http_util.rs Migrate request/response helpers to HTTP types, preserve duplicate headers, and keep request-info logic on ClientInfo.
crates/trusted-server-core/src/integrations/lockr.rs Use Fastly compat shims for header/cookie forwarding at the integration boundary.
crates/trusted-server-core/src/integrations/permutive.rs Use Fastly compat shims for custom-header forwarding at the integration boundary.
crates/trusted-server-core/src/integrations/prebid.rs Bridge request-info construction and cookie forwarding through compat conversions.
crates/trusted-server-core/src/integrations/registry.rs Use compat conversions for synthetic ID generation and synthetic cookie response handling.
crates/trusted-server-core/src/integrations/testlight.rs Bridge the migrated synthetic ID lookup into the Fastly integration path.
crates/trusted-server-core/src/lib.rs Export the compat module and add migration guard tests.
crates/trusted-server-core/src/migration_guards.rs Add a regression test preventing migrated utility modules from reintroducing direct Fastly request/response types.
crates/trusted-server-core/src/proxy.rs Bridge proxy synthetic ID reads through the migrated HTTP utility layer.
crates/trusted-server-core/src/publisher.rs Route TSJS serving, request-info extraction, consent handling, and synthetic cookie writes through compat conversions.
crates/trusted-server-core/src/request_signing/endpoints.rs Switch JSON content-type constants to mime::APPLICATION_JSON.
crates/trusted-server-core/src/synthetic.rs Migrate synthetic ID helpers and tests to http::Request<EdgeBody>.

Closes

Closes #492

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
  • 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: focused Rust verification with cargo test --package trusted-server-core compat -- --nocapture, cargo test --package trusted-server-core http_util -- --nocapture, cargo test --package trusted-server-core request_signing -- --nocapture, and cargo test --package trusted-server-core migration_guards -- --nocapture
  • Other: local cd crates/js/lib && npx vitest run currently fails before test execution with ERR_REQUIRE_ESM in html-encoding-sniffer -> @exodus/bytes/encoding-lite.js; leaving CI to capture the current JS environment issue.

Hardening note

This PR does not add any new config-derived regex or pattern compilation paths. Basic auth still surfaces invalid enabled handler regex configuration as an error rather than panicking, covered by auth::tests::returns_error_for_invalid_handler_regex_without_panicking alongside the existing settings startup validation tests.

Checklist

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

@prk-Jr prk-Jr self-assigned this Apr 8, 2026
@prk-Jr prk-Jr linked an issue Apr 8, 2026 that may be closed by this pull request
Copy link
Copy Markdown
Collaborator

@ChristianPavilonis ChristianPavilonis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR cleanly migrates utility-layer functions (auth, cookies, synthetic, http_util, consent/extraction, consent/mod) from fastly::Request/fastly::Response to http::Request/http::Response with EdgeBody. The compat bridge pattern is sound and well-tested.

Two duplicate-header-dropping bugs need fixing before merge — see inline comments.

Blocking

🔧 wrench

  • to_fastly_request drops duplicate headers: Uses set_header instead of append_header, losing multi-valued headers during conversion (compat.rs:65)
  • copy_custom_headers drops duplicate X-headers: Uses insert instead of append, a behavioral regression from the old Fastly-based version (http_util.rs:22)

Non-blocking

🤔 thinking

  • Migration guard strip_line_comments is naive about string literals: A Rust string literal like "fastly::Request" in a test assertion would trigger a false positive. Currently does not happen, but the approach is fragile. Consider adding a brief comment documenting the limitation. (migration_guards.rs)

🌱 seedling

  • copy_custom_headers in http_util.rs has no callers outside tests: The integrations (lockr, permutive) now call compat::copy_fastly_custom_headers instead. Worth noting for PR 15 cleanup.

⛏ nitpick

  • Dead "X-" check in copy_fastly_custom_headers: The check for name_str.starts_with("X-") in compat.rs:144 is dead code since Fastly normalizes header names to lowercase. The migrated copy_custom_headers in http_util.rs only checks "x-". Not a correctness issue, but the asymmetry may confuse readers.
  • Temporary compat functions well-documented with removal targets: Every public function in compat.rs has a # PR 15 removal target annotation — great practice for tracking temporary code.

👍 praise

  • Thorough compat test coverage: 11 focused tests covering round-trip conversions, duplicate header preservation, header sanitization, cookie forwarding with consent stripping, and synthetic cookie lifecycle. Excellent scaffolding for temporary bridge code.
  • Migration guard test is a clever regression barrier: The include_str! approach to scan source files for banned Fastly types provides a compile-time-like guarantee without proc macros.

📝 note

  • mime dependency is appropriate: Replaces fastly::mime::APPLICATION_JSON references. Well-maintained, zero-dependency crate suitable for the use case.

CI Status

  • All checks: PASS

Comment thread crates/trusted-server-core/src/compat.rs Outdated
Comment thread crates/trusted-server-core/src/http_util.rs Outdated
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Migrates the utility layer (auth, cookies, synthetic, http_util, consent) off direct fastly::Request/fastly::Response to http::{Request, Response}<EdgeBody> with a temporary compat bridge. Well-structured migration with good test coverage and clear lifecycle annotations. Two header-handling bugs need fixing before merge.

Blocking

🔧 wrench

  • copy_custom_headers drops duplicate headers: uses insert instead of append in http_util.rs:22. The compat shim preserves duplicates; this migrated version doesn't. Will become a production regression when the compat layer is removed in PR 15.
  • to_fastly_request drops duplicate headers: uses set_header instead of append_header in compat.rs:65. Inconsistent with to_fastly_response which correctly uses append_header on line 109.

Non-blocking

🤔 thinking

  • Migration guard doesn't handle block comments: strip_line_comments in migration_guards.rs only strips // line comments. A /* fastly::Request */ block comment would cause a false positive, and a real regression hidden inside a block comment wouldn't be caught. Acceptable for a guard test (false positives are safe), just noting the limitation.

🌱 seedling

  • Add a to_fastly_request duplicate-header round-trip test: after fixing the append_header issue, a round-trip test (http::Requestfastly::Requesthttp::Request) verifying duplicate header preservation would prevent future regressions.

⛏ nitpick

  • Redundant case check in copy_fastly_custom_headers: compat.rs:144 checks both "x-" and "X-" but Fastly's HeaderName is already case-normalized to lowercase. Not worth changing in temporary code.

CI Status

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

Comment thread crates/trusted-server-core/src/http_util.rs Outdated
Comment thread crates/trusted-server-core/src/compat.rs Outdated
Comment thread crates/trusted-server-core/src/compat.rs Outdated
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Prior blocking findings (duplicate-header drops in to_fastly_request and copy_custom_headers) are both fixed in 2817761, with round-trip regression tests added. CI is green. Approving; non-blocking observations below.

Non-blocking

🤔 thinking

  • from_fastly_request_ref silently drops body (crates/trusted-server-core/src/compat.rs:54): docs note the body is empty, but the name doesn't signal it. Footgun if a future caller passes a POST expecting body access. Consider renaming to from_fastly_headers_ref or adding a debug_assert! when the source request has a non-empty body. All current callers only read headers, so not a bug today.
  • Double conversion on the auction hot path: auction/endpoints.rs:50 builds http_req once, but auction/formats.rs:93 (convert_tsjs_to_auction_request) re-converts the same underlying fastly::Request again via from_fastly_request_ref. Each conversion iterates all headers — two copies per auction request. Acceptable for a temporary bridge (PR 15 removes it), but worth a comment referencing PR 15 so the duplication doesn't get frozen.
  • forward_cookie_header uses insert, not append (crates/trusted-server-core/src/cookies.rs:167,175,184): fine for HTTP/1 where Cookie is a single semicolon-joined value, but HTTP/2 (RFC 7540 §8.1.2.5) allows splitting across multiple headers and only the first would be forwarded. Matches pre-migration behavior and Fastly concatenates HTTP/2 cookie headers upstream, so likely not a live bug — worth documenting the assumption.

🌱 seedling

  • Migration guard scope is "utility modules only" (crates/trusted-server-core/src/migration_guards.rs:27-37): guards 6 files. Handler/integration files (publisher.rs, proxy.rs, registry.rs, integrations/*.rs, auction/*.rs) still use fastly::Request by design. PR 12–14 should extend the banned-patterns list as files are migrated.

⛏ nitpick

  • Dead case check in copy_fastly_custom_headers (crates/trusted-server-core/src/compat.rs:144): checks both "x-" and "X-" though fastly::Request normalizes to lowercase. Not worth a commit in temporary code — trim on the next touch.

CI Status

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

Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

PR introduces compat.rs as a temporary Fastly↔http bridge and migrates six utility modules (auth, cookies, synthetic, http_util, consent/extraction, consent/mod) off fastly::Request/fastly::Response. Prior approvals (2026-04-14) predate the latest commit ae402ff (2026-04-15), which renamed from_fastly_request_reffrom_fastly_headers_ref, dropped a redundant case check in copy_fastly_custom_headers, and switched forward_cookie_header to get_all + append. The round-2 fixes for duplicate-header preservation are clean, but a few concerns remain — notably a new runtime-panic surface at the edge and unused compat helpers that arrived before their callers.

Blocking

🔧 wrench

  • New edge-wide panic surface on URL parsing: compat::build_http_request calls .parse().expect(...) on every bridged request. http::Uri is stricter than Fastly's internal url::Url, so a URL Fastly accepts but http::Uri rejects panics the entire edge handler before auth can run. Previously enforce_basic_auth used req.get_path() with no re-parse. (crates/trusted-server-core/src/compat.rs:14-31)

Non-blocking

🤔 thinking

  • Redundant compat conversion on the prebid hot path: request_bids (prebid.rs:1012) and to_openrtb (prebid.rs:713) both convert the same context.request in the same auction flow — pull up once and thread through.

♻️ refactor

  • Three compat functions ship without callers: from_fastly_request, to_fastly_request, and from_fastly_response are only referenced from their own tests. CLAUDE.md says "Don't design for hypothetical future requirements. No half-finished implementations either." Ship in the PR that uses them. (crates/trusted-server-core/src/compat.rs:40, 61, 90)

🌱 seedling / 📌 out of scope / ⛏ nitpick

  • 📌 Redundant conversion at the auction boundary: acknowledged by TODO at auction/formats.rs:93-95; accepted cost of incremental migration.
  • 🌱 sanitize_fastly_forwarded_headers get-then-remove: remove_header is idempotent; the get_header guard exists only for the debug log. (compat.rs:129-136)
  • forward_cookie_header panics on HeaderValue::from_str: fires only on already-validated input, but a try_from + skip keeps failure local to the function. (cookies.rs:156-186)

CI Status

  • fmt: PASS
  • clippy: PASS
  • rust tests: PASS (841/841)
  • browser integration / integration / artifacts (GitHub Actions): PASS

let uri: http::Uri = req
.get_url_str()
.parse()
.expect("should parse fastly request URL as URI");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 wrench — New edge-wide runtime panic surface on URL parsing.

build_http_request calls .parse().expect("should parse fastly request URL as URI") on every request that passes through the compat bridge. Fastly's internal URL handling (backed by url::Url) is more permissive than http::Uri (IPv6 zone identifiers, some host-character edge cases, etc.), so a URL Fastly accepts but http::Uri rejects now panics the entire edge handler.

This runs on every request via route_requestenforce_basic_auth (adapter-fastly/src/main.rs:124) before auth can even execute. Previously enforce_basic_auth used req.get_path() which does not re-parse, so this is a net regression in robustness at the very first step of request handling. Most callers don't actually use the parsed URI (they only read headers), so the blast radius is larger than the value delivered.

Fix options:

  1. Return a 400 from the edge instead of panicking when the URL can't be represented as http::Uri.
  2. Build the http::Request with a placeholder URI (e.g., /) when the source URL fails to parse, and let callers that actually need the URI read it from the original fastly::Request.
  3. At minimum, convert the expect into a checked path so malformed URLs produce a logged error + 400, not a panic.
let uri: http::Uri = match req.get_url_str().parse() {
    Ok(uri) => uri,
    Err(_) => http::Uri::from_static("/"),
};

{
if request_signing_config.enabled {
let request_info = RequestInfo::from_request(context.request, context.client_info);
let http_req = compat::from_fastly_headers_ref(context.request);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 thinking — Redundant compat conversion on the prebid hot path.

request_bids calls compat::from_fastly_headers_ref(context.request) here, and to_openrtb calls it again at prebid.rs:713 for the same request in the same auction flow. Each conversion iterates every header and re-parses the URL. Pull the conversion up once in request_bids and thread the http::Request (or the derived RequestInfo) into to_openrtb via a parameter.

Fix:

let http_req = compat::from_fastly_headers_ref(context.request);
let request_info = RequestInfo::from_request(&http_req, context.client_info);
// ... pass request_info into to_openrtb and drop the second conversion at line 713

/// # Panics
///
/// Panics if the Fastly request URL cannot be parsed as an `http::Uri`.
pub fn from_fastly_request(mut req: fastly::Request) -> http::Request<EdgeBody> {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ refactorfrom_fastly_request, to_fastly_request, and from_fastly_response are unused in this PR.

from_fastly_request (line 40), to_fastly_request (line 61), and from_fastly_response (line 90) are only referenced from their own unit tests — a crate-wide grep returns zero production callers in this PR. The module doc says "scaffolding created in PR 11 and scheduled for deletion in PR 15," but CLAUDE.md explicitly calls out: "Don't design for hypothetical future requirements. No half-finished implementations either."

Both to_fastly_request and to_fastly_response also silently drop the body on EdgeBody::Stream (lines 74-76, 118-120) with only a warn! log — that's a latent bug that's easier to justify one way or another when paired with a real caller.

Recommendation: ship these helpers in the PR that actually uses them (PR 12/13/14). Keep only from_fastly_headers_ref, to_fastly_response, sanitize_fastly_forwarded_headers, copy_fastly_custom_headers, forward_fastly_cookie_header, set_fastly_synthetic_cookie, and expire_fastly_synthetic_cookie — the functions with production callers in this PR.

let fresh_id = generate_synthetic_id(settings, services, req).change_context(
// TODO(PR 15): Remove this conversion once the auction hot path is migrated to http::Request.
// endpoints.rs already converts this, but we do it again here as a temporary bridge.
let http_req = compat::from_fastly_headers_ref(req);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📌 out of scope — Duplicate conversion acknowledged in the TODO.

endpoints.rs:50 already converts the same request before calling this path, but convert_tsjs_to_auction_request accepts &fastly::Request and re-converts here. Accepted cost of incremental migration — worth a follow-up when the function signature can take &http::Request<EdgeBody>.

/// # PR 15 removal target
pub fn sanitize_fastly_forwarded_headers(req: &mut fastly::Request) {
for &name in SPOOFABLE_FORWARDED_HEADERS {
if req.get_header(name).is_some() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌱 seedling — Redundant get_header before remove_header.

remove_header is idempotent, so the get_header(name).is_some() check exists only to emit the debug log. That's fine, but worth noting in case this module is later consolidated with the http-native sanitize_forwarded_headers — they should behave the same way.

to.headers_mut().append(
header::COOKIE,
http::HeaderValue::from_str(&stripped)
.expect("should build stripped Cookie header value"),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick — Consider replacing expect with a graceful drop.

The HeaderValue::from_str(&stripped).expect(...) panic fires only if strip_cookies produces an invalid header value from an already-valid one, which shouldn't happen in practice — but a try_from + skip-on-error keeps failure local to this forwarding function instead of taking down the request. Same applies to the panic documented in the function-level # Panics doc at lines 152-155.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Utility layer type migration + compat adapter

3 participants