Migrate handler layer to HTTP types (PR 12)#624
Migrate handler layer to HTTP types (PR 12)#624prk-Jr wants to merge 12 commits intofeature/edgezero-pr11-utility-layer-migration-v2from
Conversation
prk-Jr
changed the title
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Clean migration of the handler layer from fastly::Request/fastly::Response to http::Request<EdgeBody>/http::Response<EdgeBody>. The conversion boundary is pushed to the adapter entry/exit and the still-unmigrated integration trait boundary. Well-tested, CI passes.
Non-blocking
♻️ refactor
- Duplicate error-to-response functions:
http_error_response(new, returnsHttpResponse) andto_error_response(existing, returnsFastlyResponse) implement identical logic with different return types. Expected migration scaffolding — track for cleanup in PR 15 when Fastly types are fully removed. (main.rs:255-267/error.rs:10)
CI Status
- integration tests: PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Solid mechanical migration of the handler layer from Fastly request/response types to http crate types. The conversion boundary is cleanly pushed to the adapter entry point, with proper compat shims for the still-unmigrated integration/provider boundary.
Non-blocking
🤔 thinking
platform_response_to_fastlyduplicated into orchestrator.rs: Copy-pasted fromproxy.rsrather than shared viacompat.rs. Two separate HTTP→Fastly response paths risk divergence (this copy usesset_headerwhich drops multi-value headers;compat::to_fastly_responsedoesn't). (orchestrator.rs:15)- sync→async
handle_publisher_request: Public API signature change — only caller usesblock_onso it works, but worth documenting. (publisher.rs:305)
♻️ refactor
Cursor::new(body.into_bytes())repeated 4 times: Could extract a smallbody_as_readerhelper to centralize body materialization. (proxy.rs:175,publisher.rs:228,246,263)
🌱 seedling
DEFAULT_PUBLISHER_FIRST_BYTE_TIMEOUThardcoded at 15s: Good to make it explicit, but may need to be configurable per deployment. (publisher.rs:22)
📝 note
- Integration proxy double-conversion: GTM, GPT, testlight each wrap
proxy_requestwithfrom_fastly_request/to_fastly_response, copying bodies twice. Expected for the PR 13 boundary.
⛏ nitpick
- Inconsistent test type alias style:
proxy.rstests useHttpMethod/HttpStatusCodeprefixes;publisher.rstests don't. The unprefixed style is cleaner.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Mechanically sound migration of the handler layer from fastly::{Request,Response} to http::{Request,Response}<EdgeBody>. The adapter entry/exit is now the only fastly conversion site for handlers, and the migration guard test is extended to lock this in. Overall high quality — a handful of concrete concerns to address before merge, mostly around silent failure modes that the type migration makes easier to miss.
Blocking
🔧 wrench
- Silent
EdgeBody::Streamdrop —compat.rs:132-134andauction/orchestrator.rs:29-32log a warning and drop the body on theStreamvariant. No test pins this behavior;debug_assert!only fires in debug. If a streaming body ever reaches this boundary, the client gets an empty 200. - Unbounded request body →
String—proxy.rs:889andproxy.rs:1003doString::from_utf8(req.into_body().into_bytes().to_vec())with no size cap. OOM/DoS risk on hostile POST. - Silent JSON serialization fallback —
proxy.rs:1156returns{}onserde_json::to_stringfailure instead of propagating an error. - Geo lookup moved before auth —
main.rs:95-110runs the geo lookup on every request, including ones that will immediately 401. Confirm this is intentional.
❓ question
- Integration trait still fastly-based —
main.rs:182-196pays a fullhttp → fastly → httpround-trip per integration request. Deferred to PR 13? If so, please leave a// TODO(PR13)marker. to_fastly_request_refdrops body —auction/endpoints.rs:90call site has no comment explaining why the empty body is safe. Please add one line so this invariant doesn't regress.
Non-blocking
🤔 thinking
- Multi-valued
Set-Cookiesurvival throughrebuild_response_with_body(proxy.rs:138) — add a test, also considermem::taketo avoid the header clone. - Invalid
settings.response_headersare silently warn-skipped at request time (main.rs:250-262); CLAUDE.md prefers startup-time validation.
♻️ refactor
migration_guards.rsuses substring matching on"fastly::Request"— prefer a\bfastly::(Request|Response|http::(Method|StatusCode))\bregex to avoid false positives on future string literals or doc comments.
🌱 seedling
- End-to-end streaming (
EdgeBody::Stream) is effectively unreachable today. Worth a follow-up to preserve streaming semantics through publisher fetch → rewrite → response once PR 13 lands.
📝 note
publisher.rs:325vspublisher.rs:361— inconsistentservices.client_info()vsservices.client_info.client_ipaccess idiom.
👍 praise
- Tight adapter boundary in
main.rs:89-120: onefrom_fastly_request/to_fastly_responsepair framingroute_request. Very clean. insert_geo_headerwarn-and-continue is the right shape for derived geo data.- Migration guard extended to cover the handler layer — exactly the right regression gate for this refactor.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS
- js tests: PASS
(All three GitHub Actions checks — browser integration tests, integration tests, prepare integration artifacts — are green.)
publisher.rs:325vspublisher.rs:361— inconsistentservices.client_info()(method) vsservices.client_info.client_ip(field) access idiom. Please pick one.
Brings in PR11 review-finding fixes (compat shim additions, testlight integration changes, migration guard comments). Conflicts resolved by keeping PR12's http-native handler layer throughout — all compat shim insertions from PR11 are superseded by the http type migration.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
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_fastlystill duplicated (crates/trusted-server-core/src/auction/orchestrator.rs:19-40vscrates/trusted-server-core/src/compat.rs:119-138) — the prior review flagged this. Now consistent onappend_headerand documented as PR 15 removal, which is an improvement, butplatform_resp.responseis alreadyhttp::Response<EdgeBody>and can be passed tocompat::to_fastly_responsedirectly. 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 onceRequestTooLargeexists. See inline. EdgeBody::Streamsilent drop in release (crates/trusted-server-core/src/compat.rs:74-77, 132-135andcrates/trusted-server-core/src/auction/orchestrator.rs:29-32) — tests atcompat.rs:584-629pin the drop-to-empty behaviour, anddebug_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-891and: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_headersvalidation uses.expect()in the hot path (crates/trusted-server-adapter-fastly/src/main.rs:249-255). Correct today becauseSettings::prepare_runtimevalidates at load (crates/trusted-server-core/src/settings.rs:486-498), butroute_requestkeeps 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
…tion - Add VERIFY_MAX_BODY_BYTES (4096) cap to handle_verify_signature - Add AUCTION_MAX_BODY_BYTES (65536) cap to handle_auction - Add ADMIN_MAX_BODY_BYTES (4096) cap to handle_rotate_key and handle_deactivate_key - Delete platform_response_to_fastly from orchestrator; both call sites now use crate::compat::to_fastly_response directly - Add sign_rejects_oversized_body and rebuild_rejects_oversized_body regression tests asserting 413 on oversized POST bodies
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
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_refclobbers multi-value headers: see inline atcompat.rs:90.
Non-blocking
🤔 thinking
VERIFY_MAX_BODY_BYTES = 4096may 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 usablepayloadbudget 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 GTMmax_beacon_body_sizepattern.
📝 note
- Body-size caps fire after the body is already materialized into memory (
compat.rs:40-43).compat::from_fastly_requestcallsreq.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 addedsign_rejects_oversized_bodyandrebuild_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 atproxy.rs:884,proxy.rs:1007,auction/endpoints.rs:45, and three times inrequest_signing/endpoints.rs(93, 183, 292). A small helper (fn enforce_max_body_size(bytes: &[u8], limit: usize, what: &'static str) -> Result<(), Report<TrustedServerError>>) inhttp_utilwould 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
…feature/edgezero-pr12-handler-layer-types
…feature/edgezero-pr12-handler-layer-types
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
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
# Panicsdoc 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_requestusesunwrap_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 withexpect("should parse Fastly-validated URL").
♻️ refactor
65536literal 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 smallenforce_max_body_size(bytes, limit, what)helper inhttp_utilwould centralize the message format and remove ~50 lines.
🌱 seedling
DEFAULT_PUBLISHER_FIRST_BYTE_TIMEOUT = 15sstill 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_fastlyis now the only diverging conversion (compat.rs:245-264). ReturnsErron streaming bodies instead ofto_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
| /// | ||
| /// # Panics | ||
| /// | ||
| /// Panics if the Fastly request URL cannot be parsed as an `http::Uri`. |
There was a problem hiding this comment.
🤔 thinking — # Panics doc says "Panics if the Fastly request URL cannot be parsed as an http::Uri," but build_http_request at compat.rs:21 has unwrap_or_else(|_| http::Uri::from_static("/")) — URL parsing never panics; it silently falls back to /. The actual panic site is builder.body(body).expect(...) at compat.rs:33-35. Same doc claim repeats at compat.rs:56-58 for from_fastly_headers_ref.
Fix: Either correct the docs to reflect the real panic condition (header construction failure), or change the URI fallback to propagate the error so doc and code agree.
| req: Request<EdgeBody>, | ||
| ) -> Result<Response<EdgeBody>, Report<TrustedServerError>> { | ||
| let body = req.into_body().into_bytes(); | ||
| if body.len() > VERIFY_MAX_BODY_BYTES { |
There was a problem hiding this comment.
🔧 wrench — No 413 regression test for /verify-signature. Round-3 review explicitly asked for these as ♻️; the proxy endpoints got sign_rejects_oversized_body/rebuild_rejects_oversized_body (proxy.rs:2549-2585) but the four endpoints whose caps were introduced in this PR still have no negative tests. Without them, a future commit that drops any cap won't be caught.
Fix (mirror the proxy.rs pattern):
#[test]
fn verify_signature_rejects_oversized_body() {
let settings = crate::test_support::tests::create_test_settings();
let oversized = "x".repeat(VERIFY_MAX_BODY_BYTES + 1);
let req = build_request(
Method::POST,
"https://test.com/verify-signature",
Some(&oversized),
);
let err = handle_verify_signature(&settings, &noop_services(), req)
.expect_err("should reject oversized body");
assert_eq!(
err.current_context().status_code(),
StatusCode::PAYLOAD_TOO_LARGE,
);
}Apply the same pattern at the rotate (line 254) and deactivate (line 381) cap checks.
| let body: AdRequest = serde_json::from_slice(&req.take_body_bytes()).change_context( | ||
| TrustedServerError::Auction { | ||
| let body_bytes = body.into_bytes(); | ||
| if body_bytes.len() > AUCTION_MAX_BODY_BYTES { |
There was a problem hiding this comment.
🔧 wrench — No 413 regression test for /auction, and the file has no #[cfg(test)] module at all. Round-3 review asked for these. Without a negative test, a future commit that drops the 64 KiB cap won't be caught.
Fix — add at the bottom of the file:
#[cfg(test)]
mod tests {
use super::*;
use crate::auction::build_orchestrator;
use crate::platform::test_support::noop_services;
use crate::test_support::tests::create_test_settings;
use edgezero_core::body::Body as EdgeBody;
use http::{Method, Request as HttpRequest, StatusCode};
#[tokio::test]
async fn auction_rejects_oversized_body() {
let settings = create_test_settings();
let orchestrator = build_orchestrator(&settings).expect("should build orchestrator");
let oversized = vec![b'x'; AUCTION_MAX_BODY_BYTES + 1];
let req = HttpRequest::builder()
.method(Method::POST)
.uri("https://test.com/auction")
.body(EdgeBody::from(oversized))
.expect("should build request");
let err = handle_auction(&settings, &orchestrator, &noop_services(), req)
.await
.expect_err("should reject oversized body");
assert_eq!(
err.current_context().status_code(),
StatusCode::PAYLOAD_TOO_LARGE,
);
}
}|
|
||
| let payload = if method == Method::POST { | ||
| let body_bytes = req.into_body().into_bytes(); | ||
| if body_bytes.len() > 65536 { |
There was a problem hiding this comment.
♻️ refactor — 65536 literal duplicated four times in this file (proxy.rs:884, :887, :1007, :1010). The other handlers in this PR introduced named constants (AUCTION_MAX_BODY_BYTES, VERIFY_MAX_BODY_BYTES, ADMIN_MAX_BODY_BYTES); proxy is the only one using bare numerics.
Fix:
const SIGN_MAX_BODY_BYTES: usize = 65536;
const REBUILD_MAX_BODY_BYTES: usize = 65536;Then reference the constants in the cap check and the format string.
| ), | ||
| })); | ||
| } | ||
| let body = String::from_utf8(body_bytes.to_vec()).change_context( |
There was a problem hiding this comment.
♻️ refactor — String::from_utf8(body_bytes.to_vec()) allocates twice: body_bytes is already bytes::Bytes, and .to_vec() copies into a fresh Vec before String::from_utf8 consumes it. Same pattern at proxy.rs:1015.
Fix (one pass over the buffer):
let body = std::str::from_utf8(&body_bytes)
.change_context(TrustedServerError::InvalidUtf8 {
message: "first-party sign request body should be valid UTF-8".to_string(),
})?;
serde_json::from_str::<ProxySignReq>(body).change_context(...)?Minor — feel free to defer.
…feature/edgezero-pr12-handler-layer-types
3 participants
Summary
httprequest/response types so core handlers no longer depend on Fastly request/response APIs.Changes
crates/trusted-server-adapter-fastly/src/main.rscrates/trusted-server-core/src/auction/endpoints.rs/auctionhandler tohttptypes and rebuild a Fastly request only at the provider-facingAuctionContextboundary.crates/trusted-server-core/src/auction/formats.rscrates/trusted-server-core/src/auction/orchestrator.rscrates/trusted-server-core/src/compat.rscrates/trusted-server-core/src/geo.rscrates/trusted-server-core/src/integrations/google_tag_manager.rscrates/trusted-server-core/src/integrations/gpt.rscrates/trusted-server-core/src/integrations/testlight.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.rsCloses
Closes #493
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest run(not run; no JS/TS files changed)cd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)