Migrate utility layer to HTTP types (PR 11)#623
Migrate utility layer to HTTP types (PR 11)#623prk-Jr wants to merge 3 commits intofeature/edgezero-pr10-abstract-logging-initializationfrom
Conversation
ChristianPavilonis
left a comment
There was a problem hiding this comment.
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_requestdrops duplicate headers: Usesset_headerinstead ofappend_header, losing multi-valued headers during conversion (compat.rs:65)copy_custom_headersdrops duplicate X-headers: Usesinsertinstead ofappend, a behavioral regression from the old Fastly-based version (http_util.rs:22)
Non-blocking
🤔 thinking
- Migration guard
strip_line_commentsis 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_headersin http_util.rs has no callers outside tests: The integrations (lockr, permutive) now callcompat::copy_fastly_custom_headersinstead. Worth noting for PR 15 cleanup.
⛏ nitpick
- Dead
"X-"check incopy_fastly_custom_headers: The check forname_str.starts_with("X-")in compat.rs:144 is dead code since Fastly normalizes header names to lowercase. The migratedcopy_custom_headersin 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.rshas a# PR 15 removal targetannotation — 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
mimedependency is appropriate: Replacesfastly::mime::APPLICATION_JSONreferences. Well-maintained, zero-dependency crate suitable for the use case.
CI Status
- All checks: PASS
aram356
left a comment
There was a problem hiding this comment.
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_headersdrops duplicate headers: usesinsertinstead ofappendinhttp_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_requestdrops duplicate headers: usesset_headerinstead ofappend_headerincompat.rs:65. Inconsistent withto_fastly_responsewhich correctly usesappend_headeron line 109.
Non-blocking
🤔 thinking
- Migration guard doesn't handle block comments:
strip_line_commentsinmigration_guards.rsonly 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_requestduplicate-header round-trip test: after fixing theappend_headerissue, a round-trip test (http::Request→fastly::Request→http::Request) verifying duplicate header preservation would prevent future regressions.
⛏ nitpick
- Redundant case check in
copy_fastly_custom_headers:compat.rs:144checks both"x-"and"X-"but Fastly'sHeaderNameis already case-normalized to lowercase. Not worth changing in temporary code.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
aram356
left a comment
There was a problem hiding this comment.
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_refsilently 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 tofrom_fastly_headers_refor adding adebug_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:50buildshttp_reqonce, butauction/formats.rs:93(convert_tsjs_to_auction_request) re-converts the same underlyingfastly::Requestagain viafrom_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_headerusesinsert, notappend(crates/trusted-server-core/src/cookies.rs:167,175,184): fine for HTTP/1 whereCookieis 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 usefastly::Requestby 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-"thoughfastly::Requestnormalizes 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
aram356
left a comment
There was a problem hiding this comment.
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_ref → from_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_requestcalls.parse().expect(...)on every bridged request.http::Uriis stricter than Fastly's internalurl::Url, so a URL Fastly accepts buthttp::Urirejects panics the entire edge handler before auth can run. Previouslyenforce_basic_authusedreq.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) andto_openrtb(prebid.rs:713) both convert the samecontext.requestin the same auction flow — pull up once and thread through.
♻️ refactor
- Three compat functions ship without callers:
from_fastly_request,to_fastly_request, andfrom_fastly_responseare only referenced from their own tests.CLAUDE.mdsays "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_headersget-then-remove:remove_headeris idempotent; theget_headerguard exists only for the debug log. (compat.rs:129-136) - ⛏
forward_cookie_headerpanics onHeaderValue::from_str: fires only on already-validated input, but atry_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"); |
There was a problem hiding this comment.
🔧 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_request → enforce_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:
- Return a 400 from the edge instead of panicking when the URL can't be represented as
http::Uri. - Build the
http::Requestwith a placeholder URI (e.g.,/) when the source URL fails to parse, and let callers that actually need the URI read it from the originalfastly::Request. - At minimum, convert the
expectinto 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); |
There was a problem hiding this comment.
🤔 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> { |
There was a problem hiding this comment.
♻️ refactor — from_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); |
There was a problem hiding this comment.
📌 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() { |
There was a problem hiding this comment.
🌱 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"), |
There was a problem hiding this comment.
⛏ 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.
Summary
fastly::Request/fastly::Responseusage so core helpers can operate onhttp::{Request, Response}andedgezero_core::Body.compatbridge at Fastly boundaries so handlers and integrations can keep working while later migration PRs move the remaining call stack.Changes
Cargo.tomlmimedependency used by migrated HTTP response helpers.Cargo.lockmimedependency.crates/trusted-server-adapter-fastly/src/main.rscrates/trusted-server-core/Cargo.tomlmimeworkspace dependency.crates/trusted-server-core/src/auction/endpoints.rscrates/trusted-server-core/src/auction/formats.rscrates/trusted-server-core/src/auth.rshttp::Request/Responseand update tests to HTTP builders.crates/trusted-server-core/src/compat.rscrates/trusted-server-core/src/consent/extraction.rshttp::Request<EdgeBody>.crates/trusted-server-core/src/consent/mod.rscrates/trusted-server-core/src/cookies.rscrates/trusted-server-core/src/http_util.rsClientInfo.crates/trusted-server-core/src/integrations/lockr.rscrates/trusted-server-core/src/integrations/permutive.rscrates/trusted-server-core/src/integrations/prebid.rscrates/trusted-server-core/src/integrations/registry.rscrates/trusted-server-core/src/integrations/testlight.rscrates/trusted-server-core/src/lib.rscrates/trusted-server-core/src/migration_guards.rscrates/trusted-server-core/src/proxy.rscrates/trusted-server-core/src/publisher.rscrates/trusted-server-core/src/request_signing/endpoints.rsmime::APPLICATION_JSON.crates/trusted-server-core/src/synthetic.rshttp::Request<EdgeBody>.Closes
Closes #492
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo 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, andcargo test --package trusted-server-core migration_guards -- --nocapturecd crates/js/lib && npx vitest runcurrently fails before test execution withERR_REQUIRE_ESMinhtml-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_panickingalongside the existing settings startup validation tests.Checklist
unwrap()in production code — useexpect("should ...")println!)