From 2da11f3c79d2ebcb94d1aabf60f196fd615f5c60 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Tue, 25 Aug 2026 12:39:26 +0530 Subject: [PATCH 1/3] Allow template caching behind edge auth --- .../src/middleware.rs | 7 +- .../src/middleware.rs | 7 +- .../src/middleware.rs | 7 +- .../src/middleware.rs | 7 +- crates/trusted-server-core/src/auth.rs | 268 +++++++++++++++--- crates/trusted-server-core/src/publisher.rs | 138 ++++++++- 6 files changed, 391 insertions(+), 43 deletions(-) diff --git a/crates/trusted-server-adapter-axum/src/middleware.rs b/crates/trusted-server-adapter-axum/src/middleware.rs index 9f00f7614..974a23e46 100644 --- a/crates/trusted-server-adapter-axum/src/middleware.rs +++ b/crates/trusted-server-adapter-axum/src/middleware.rs @@ -104,8 +104,11 @@ impl AuthMiddleware { #[async_trait(?Send)] impl Middleware for AuthMiddleware { - async fn handle(&self, ctx: RequestContext, next: Next<'_>) -> Result { - match enforce_basic_auth(&self.settings, ctx.request()) { + async fn handle(&self, mut ctx: RequestContext, next: Next<'_>) -> Result { + // Takes the request mutably because `enforce_basic_auth` marks requests + // whose credential it consumed itself; the shared template cache gate + // reads that marker later. + match enforce_basic_auth(&self.settings, ctx.request_mut()) { Ok(Some(response)) => return Ok(response), Ok(None) => {} Err(report) => { diff --git a/crates/trusted-server-adapter-cloudflare/src/middleware.rs b/crates/trusted-server-adapter-cloudflare/src/middleware.rs index f2b3374c4..1e5f79ef3 100644 --- a/crates/trusted-server-adapter-cloudflare/src/middleware.rs +++ b/crates/trusted-server-adapter-cloudflare/src/middleware.rs @@ -112,8 +112,11 @@ impl AuthMiddleware { #[async_trait(?Send)] impl Middleware for AuthMiddleware { - async fn handle(&self, ctx: RequestContext, next: Next<'_>) -> Result { - match enforce_basic_auth(&self.settings, ctx.request()) { + async fn handle(&self, mut ctx: RequestContext, next: Next<'_>) -> Result { + // Takes the request mutably because `enforce_basic_auth` marks requests + // whose credential it consumed itself; the shared template cache gate + // reads that marker later. + match enforce_basic_auth(&self.settings, ctx.request_mut()) { Ok(Some(response)) => return Ok(response), Ok(None) => {} Err(report) => { diff --git a/crates/trusted-server-adapter-fastly/src/middleware.rs b/crates/trusted-server-adapter-fastly/src/middleware.rs index 8329c46b1..7b2bb636e 100644 --- a/crates/trusted-server-adapter-fastly/src/middleware.rs +++ b/crates/trusted-server-adapter-fastly/src/middleware.rs @@ -127,8 +127,11 @@ impl AuthMiddleware { #[async_trait(?Send)] impl Middleware for AuthMiddleware { - async fn handle(&self, ctx: RequestContext, next: Next<'_>) -> Result { - match enforce_basic_auth(&self.settings, ctx.request()) { + async fn handle(&self, mut ctx: RequestContext, next: Next<'_>) -> Result { + // Takes the request mutably because `enforce_basic_auth` marks requests + // whose credential it consumed itself; the shared template cache gate + // reads that marker later. + match enforce_basic_auth(&self.settings, ctx.request_mut()) { Ok(Some(response)) => return Ok(response), Ok(None) => {} Err(report) => { diff --git a/crates/trusted-server-adapter-spin/src/middleware.rs b/crates/trusted-server-adapter-spin/src/middleware.rs index d3005d510..9ee6d2b50 100644 --- a/crates/trusted-server-adapter-spin/src/middleware.rs +++ b/crates/trusted-server-adapter-spin/src/middleware.rs @@ -105,8 +105,11 @@ impl AuthMiddleware { #[async_trait(?Send)] impl Middleware for AuthMiddleware { - async fn handle(&self, ctx: RequestContext, next: Next<'_>) -> Result { - match enforce_basic_auth(&self.settings, ctx.request()) { + async fn handle(&self, mut ctx: RequestContext, next: Next<'_>) -> Result { + // Takes the request mutably because `enforce_basic_auth` marks requests + // whose credential it consumed itself; the shared template cache gate + // reads that marker later. + match enforce_basic_auth(&self.settings, ctx.request_mut()) { Ok(Some(response)) => return Ok(response), Ok(None) => {} Err(report) => { diff --git a/crates/trusted-server-core/src/auth.rs b/crates/trusted-server-core/src/auth.rs index 6c92d042d..68e590cd7 100644 --- a/crates/trusted-server-core/src/auth.rs +++ b/crates/trusted-server-core/src/auth.rs @@ -11,6 +11,46 @@ use crate::settings::Settings; const BASIC_AUTH_REALM: &str = r#"Basic realm="Trusted Server""#; +/// Marker recording that this request's `Authorization` header was consumed and +/// validated by a Trusted Server handler at the edge. +/// +/// The shared template cache refuses every request carrying `Authorization`, +/// because an authorized response must never become a reader-neutral template. +/// That rule exists for credentials bound for the publisher origin, whose +/// response content TS cannot reason about. +/// +/// A credential this edge terminated is a different case. [`enforce_basic_auth`] +/// runs as middleware ahead of routing, so a request that reaches a handler for a +/// gated path has necessarily already satisfied that same handler. Every reader +/// able to look up a template stored from such a request has authenticated +/// against the same credential, so reuse is not a cross-reader disclosure. +/// +/// Absence of this marker on a request that still carries `Authorization` means +/// the credential is pass-through, and the template cache continues to refuse it. +/// +/// # Invariants +/// +/// The private field makes [`enforce_basic_auth`] the only code that can produce +/// this marker. It grants shared-template eligibility to a request that would +/// otherwise be refused, so being unforgeable outside this module is the whole +/// point: a caller cannot assert "already authenticated" without having actually +/// checked. [`enforce_basic_auth`] also clears any inherited marker before it +/// decides, so the value can never outlive the check that produced it. +#[derive(Debug, Clone)] +pub(crate) struct EdgeTerminatedAuthorization(()); + +impl EdgeTerminatedAuthorization { + /// Builds the marker without performing a credential check. + /// + /// Test-only. Production code obtains this marker exclusively by passing + /// [`enforce_basic_auth`], which is what makes it meaningful. + #[cfg(test)] + #[must_use] + pub(crate) const fn for_test() -> Self { + Self(()) + } +} + /// Enforces HTTP Basic authentication for configured handler paths. /// /// Returns `Ok(None)` when the request does not target a protected handler or @@ -24,22 +64,49 @@ const BASIC_AUTH_REALM: &str = r#"Basic realm="Trusted Server""#; /// the reserved admin namespace fail closed if no handler matches, providing /// defense in depth for malformed and parameterized paths. /// +/// # Request mutation +/// +/// Takes `req` mutably because it owns [`EdgeTerminatedAuthorization`]. Any +/// inherited marker is cleared on entry, and a fresh one is inserted only on the +/// success path, so the marker present after this call always describes this +/// call's own decision. Nothing else about the request is touched — in +/// particular the `Authorization` header is left in place and still reaches the +/// publisher origin. +/// +/// That last point is a stated assumption: a credential this edge terminates is +/// treated as reader-neutral, which holds unless the origin *also* authenticates +/// on the same header. An origin that does so declares `Vary: Authorization`, +/// which the template-cache store refuses as an uncovered `Vary` name. An origin +/// that varies on `Authorization` without declaring it would defeat any HTTP +/// cache, and is out of scope here. +/// /// # Errors /// /// Returns an error when handler configuration is invalid, such as an /// un-compilable path regex. pub fn enforce_basic_auth( settings: &Settings, - req: &Request, + req: &mut Request, ) -> Result>, Report> { - let path = req.uri().path(); - let Some(handler) = settings.handler_for_path(path)? else { - if Settings::is_admin_path(path) { - return Err(Report::new(TrustedServerError::Configuration { - message: format!("Admin path `{path}` has no configured handler"), - })); - } - return Ok(None); + // Cleared before any early return so no inherited marker can survive a call + // that did not itself validate a credential. Without this, a request marked + // upstream and then routed to an unprotected path would keep an assertion + // nothing checked. + req.extensions_mut().remove::(); + + // Scoped so the path borrow ends before the successful branch marks the + // request. `handler` borrows `settings`, not `req`. + let handler = { + let path = req.uri().path(); + let Some(handler) = settings.handler_for_path(path)? else { + if Settings::is_admin_path(path) { + return Err(Report::new(TrustedServerError::Configuration { + message: format!("Admin path `{path}` has no configured handler"), + })); + } + return Ok(None); + }; + handler }; let Some((username, password)) = extract_credentials(req) else { @@ -59,6 +126,9 @@ pub fn enforce_basic_auth( .ct_eq(&Sha256::digest(password.as_bytes())); if bool::from(username_match & password_match) { + // Record that TS itself consumed this credential, so the shared template + // cache can distinguish it from a credential meant for the origin. + req.extensions_mut().insert(EdgeTerminatedAuthorization(())); Ok(None) } else { log::warn!("Basic auth failed for path: {}", req.uri().path()); @@ -67,10 +137,11 @@ pub fn enforce_basic_auth( } fn extract_credentials(req: &Request) -> Option<(String, String)> { - let header_value = req - .headers() - .get(header::AUTHORIZATION) - .and_then(|value| value.to_str().ok())?; + let mut header_values = req.headers().get_all(header::AUTHORIZATION).iter(); + let header_value = header_values.next()?.to_str().ok()?; + if header_values.next().is_some() { + return None; + } let mut parts = header_value.splitn(2, ' '); let scheme = parts.next()?.trim(); @@ -134,9 +205,9 @@ mod tests { let settings = create_test_settings(); for path in ["/_ts/admin%2Fec", "/_ts/admin%2fec"] { - let req = build_request(Method::GET, &format!("https://example.com{path}")); + let mut req = build_request(Method::GET, &format!("https://example.com{path}")); - let response = enforce_basic_auth(&settings, &req) + let response = enforce_basic_auth(&settings, &mut req) .expect("should evaluate auth") .unwrap_or_else(|| panic!("should challenge {path}")); @@ -148,13 +219,142 @@ mod tests { } } + #[test] + fn valid_credentials_mark_the_request_as_edge_terminated() { + let settings = create_test_settings(); + let mut req = build_request(Method::GET, "https://example.com/secure"); + let encoded = STANDARD.encode("user:pass"); + set_authorization(&mut req, &format!("Basic {encoded}")); + + assert!( + enforce_basic_auth(&settings, &mut req) + .expect("should evaluate auth") + .is_none(), + "valid credentials should be admitted" + ); + assert!( + req.extensions() + .get::() + .is_some(), + "a credential this edge consumed should be marked so the template cache can share it" + ); + } + + #[test] + fn repeated_authorization_values_are_rejected_without_a_marker() { + let settings = create_test_settings(); + let mut req = build_request(Method::GET, "https://example.com/secure"); + let encoded = STANDARD.encode("user:pass"); + set_authorization(&mut req, &format!("Basic {encoded}")); + req.headers_mut().append( + header::AUTHORIZATION, + HeaderValue::from_static("Bearer publisher-origin-credential"), + ); + + let response = enforce_basic_auth(&settings, &mut req) + .expect("should evaluate auth") + .expect("should challenge an ambiguous credential"); + assert_eq!(response.status(), StatusCode::UNAUTHORIZED); + assert!( + req.extensions() + .get::() + .is_none(), + "a repeated authorization field must remain pass-through rather than being marked safe" + ); + } + + #[test] + fn an_inherited_marker_is_cleared_on_an_unprotected_path() { + // The marker asserts "this edge already checked a credential". A request + // routed to a path no handler protects was never checked here, so a marker + // it arrived with must not survive to grant shared-template eligibility. + let settings = create_test_settings(); + let mut req = build_request(Method::GET, "https://example.com/open"); + set_authorization(&mut req, "Basic dXNlcjpwYXNz"); + req.extensions_mut() + .insert(EdgeTerminatedAuthorization::for_test()); + + assert!( + enforce_basic_auth(&settings, &mut req) + .expect("should evaluate auth") + .is_none(), + "an unprotected path should not challenge" + ); + assert!( + req.extensions() + .get::() + .is_none(), + "a marker no credential check produced must not survive this call" + ); + } + + #[test] + fn an_inherited_marker_is_cleared_when_credentials_are_rejected() { + let settings = create_test_settings(); + let mut req = build_request(Method::GET, "https://example.com/secure"); + let encoded = STANDARD.encode("user:wrong-pass"); + set_authorization(&mut req, &format!("Basic {encoded}")); + req.extensions_mut() + .insert(EdgeTerminatedAuthorization::for_test()); + + let response = enforce_basic_auth(&settings, &mut req) + .expect("should evaluate auth") + .expect("should challenge"); + assert_eq!(response.status(), StatusCode::UNAUTHORIZED); + assert!( + req.extensions() + .get::() + .is_none(), + "a failed check must strip an inherited marker rather than honour it" + ); + } + + #[test] + fn a_non_protected_path_leaves_authorization_unmarked() { + let settings = create_test_settings(); + let mut req = build_request(Method::GET, "https://example.com/open"); + set_authorization(&mut req, "Basic dXNlcjpwYXNz"); + + assert!( + enforce_basic_auth(&settings, &mut req) + .expect("should evaluate auth") + .is_none(), + "an unprotected path should not challenge" + ); + assert!( + req.extensions() + .get::() + .is_none(), + "a credential no handler consumed is pass-through and must stay disqualifying" + ); + } + + #[test] + fn rejected_credentials_leave_the_request_unmarked() { + let settings = create_test_settings(); + let mut req = build_request(Method::GET, "https://example.com/secure"); + let encoded = STANDARD.encode("user:wrong-pass"); + set_authorization(&mut req, &format!("Basic {encoded}")); + + let response = enforce_basic_auth(&settings, &mut req) + .expect("should evaluate auth") + .expect("should challenge"); + assert_eq!(response.status(), StatusCode::UNAUTHORIZED); + assert!( + req.extensions() + .get::() + .is_none(), + "a failed credential must never be marked as terminated" + ); + } + #[test] fn no_challenge_for_non_protected_path() { let settings = create_test_settings(); - let req = build_request(Method::GET, "https://example.com/open"); + let mut req = build_request(Method::GET, "https://example.com/open"); assert!( - enforce_basic_auth(&settings, &req) + enforce_basic_auth(&settings, &mut req) .expect("should evaluate auth") .is_none() ); @@ -163,9 +363,9 @@ mod tests { #[test] fn challenge_when_missing_credentials() { let settings = create_test_settings(); - let req = build_request(Method::GET, "https://example.com/secure"); + let mut req = build_request(Method::GET, "https://example.com/secure"); - let response = enforce_basic_auth(&settings, &req) + let response = enforce_basic_auth(&settings, &mut req) .expect("should evaluate auth") .expect("should challenge"); assert_eq!(response.status(), StatusCode::UNAUTHORIZED); @@ -184,7 +384,7 @@ mod tests { set_authorization(&mut req, &format!("Basic {token}")); assert!( - enforce_basic_auth(&settings, &req) + enforce_basic_auth(&settings, &mut req) .expect("should evaluate auth") .is_none() ); @@ -197,7 +397,7 @@ mod tests { let token = STANDARD.encode("wrong:wrong"); set_authorization(&mut req, &format!("Basic {token}")); - let response = enforce_basic_auth(&settings, &req) + let response = enforce_basic_auth(&settings, &mut req) .expect("should evaluate auth") .expect("should challenge"); assert_eq!(response.status(), StatusCode::UNAUTHORIZED); @@ -211,7 +411,7 @@ mod tests { let token = STANDARD.encode("wrong-user:pass"); set_authorization(&mut req, &format!("Basic {token}")); - let response = enforce_basic_auth(&settings, &req) + let response = enforce_basic_auth(&settings, &mut req) .expect("should evaluate auth") .expect("should challenge"); assert_eq!( @@ -228,7 +428,7 @@ mod tests { let token = STANDARD.encode("user:wrong-pass"); set_authorization(&mut req, &format!("Basic {token}")); - let response = enforce_basic_auth(&settings, &req) + let response = enforce_basic_auth(&settings, &mut req) .expect("should evaluate auth") .expect("should challenge"); assert_eq!( @@ -244,7 +444,7 @@ mod tests { let mut req = build_request(Method::GET, "https://example.com/secure"); set_authorization(&mut req, "Bearer token"); - let response = enforce_basic_auth(&settings, &req) + let response = enforce_basic_auth(&settings, &mut req) .expect("should evaluate auth") .expect("should challenge"); assert_eq!(response.status(), StatusCode::UNAUTHORIZED); @@ -269,7 +469,7 @@ mod tests { set_authorization(&mut req, &format!("Basic {token}")); assert!( - enforce_basic_auth(&settings, &req) + enforce_basic_auth(&settings, &mut req) .expect("should evaluate auth") .is_none(), "should allow admin path with correct credentials" @@ -283,7 +483,7 @@ mod tests { let token = STANDARD.encode("admin:wrong"); set_authorization(&mut req, &format!("Basic {token}")); - let response = enforce_basic_auth(&settings, &req) + let response = enforce_basic_auth(&settings, &mut req) .expect("should evaluate auth") .expect("should challenge admin path with wrong credentials"); assert_eq!(response.status(), StatusCode::UNAUTHORIZED); @@ -312,9 +512,9 @@ mod tests { "https://example.com/_ts/page-bids?path=/article", "https://example.com/_ts/api/v1/identify", ] { - let req = build_request(Method::GET, path); + let mut req = build_request(Method::GET, path); - let response = enforce_basic_auth(&settings, &req) + let response = enforce_basic_auth(&settings, &mut req) .expect("should evaluate auth") .unwrap_or_else(|| panic!("should challenge {path} under a `^/_ts` handler")); assert_eq!( @@ -328,9 +528,9 @@ mod tests { #[test] fn challenge_admin_path_with_missing_credentials() { let settings = create_test_settings(); - let req = build_request(Method::POST, "https://example.com/_ts/admin/keys/rotate"); + let mut req = build_request(Method::POST, "https://example.com/_ts/admin/keys/rotate"); - let response = enforce_basic_auth(&settings, &req) + let response = enforce_basic_auth(&settings, &mut req) .expect("should evaluate auth") .expect("should challenge admin path with missing credentials"); assert_eq!(response.status(), StatusCode::UNAUTHORIZED); @@ -354,12 +554,12 @@ mod tests { let settings: Settings = toml::from_str(&config).expect("should deserialize settings without finalization"); let ec_id = format!("{}.abc123", "a".repeat(64)); - let req = build_request( + let mut req = build_request( Method::GET, &format!("https://example.com/_ts/admin/ec/{ec_id}"), ); - let error = enforce_basic_auth(&settings, &req) + let error = enforce_basic_auth(&settings, &mut req) .expect_err("should fail closed without a matching admin handler"); assert!( error.to_string().contains("no configured handler"), @@ -375,10 +575,10 @@ mod tests { ); let settings: Settings = toml::from_str(&config).expect("should deserialize settings without finalization"); - let req = build_request(Method::GET, "https://example.com/_ts/administrator"); + let mut req = build_request(Method::GET, "https://example.com/_ts/administrator"); assert!( - enforce_basic_auth(&settings, &req) + enforce_basic_auth(&settings, &mut req) .expect("should evaluate auth") .is_none(), "should not classify a similar prefix as the admin namespace" diff --git a/crates/trusted-server-core/src/publisher.rs b/crates/trusted-server-core/src/publisher.rs index b0d63b82a..3cf6a76af 100644 --- a/crates/trusted-server-core/src/publisher.rs +++ b/crates/trusted-server-core/src/publisher.rs @@ -4343,7 +4343,22 @@ pub async fn handle_publisher_request( // Recorded before the request is consumed by the origin send: the template cache gate // below needs it, and an authorized response must never become a shared // template. - let request_had_authorization = req.headers().contains_key(header::AUTHORIZATION); + // + // A credential this edge already terminated is exempt. Basic auth runs as + // middleware ahead of routing, so reaching here on a gated path means the same + // handler already validated the request; every reader that can look the template + // up has satisfied that same credential. Without the marker the credential is + // pass-through to the origin and still disqualifies. See + // [`crate::auth::EdgeTerminatedAuthorization`]. + let authorization_value_count = req.headers().get_all(header::AUTHORIZATION).iter().count(); + let request_had_authorization = match authorization_value_count { + 0 => false, + 1 => req + .extensions() + .get::() + .is_none(), + _ => true, + }; let request_had_cookie = req.headers().contains_key(header::COOKIE); // Whether carrying a cookie is itself disqualifying. Computed once and used for both // the lookup and the store, so the two cannot drift apart. @@ -8973,6 +8988,127 @@ mod tests { ); } + #[tokio::test] + async fn a_pass_through_authorization_never_reaches_the_shared_template() { + let stub = Arc::new(StubHttpClient::new()); + let cache = Arc::new(MemoryTemplateCache::default()); + let settings = Arc::new(settings_with_mode("esi")); + let services = services(Arc::clone(&stub), Arc::clone(&cache)); + queue_shareable_html(&stub); + + let mut request = navigation_request(); + request.headers_mut().insert( + header::AUTHORIZATION, + HeaderValue::from_static("Basic dXNlcjpwYXNz"), + ); + + let response = run(&settings, &services, request).await; + assert_eq!( + response + .headers() + .get(HEADER_X_TS_TEMPLATE_CACHE) + .and_then(|value| value.to_str().ok()), + Some("bypass-request"), + "a credential TS did not terminate is bound for the origin, so its response must never be shared" + ); + assert_eq!( + cache.lookups.lock().expect("should lock lookups").len(), + 0, + "an unterminated authorization must bypass before the lookup, not after it" + ); + } + + #[tokio::test] + async fn repeated_authorization_never_reaches_the_shared_template() { + let stub = Arc::new(StubHttpClient::new()); + let cache = Arc::new(MemoryTemplateCache::default()); + let settings = Arc::new(settings_with_mode("esi")); + let services = services(Arc::clone(&stub), Arc::clone(&cache)); + queue_shareable_html(&stub); + + let mut request = navigation_request(); + request.headers_mut().insert( + header::AUTHORIZATION, + HeaderValue::from_static("Basic dXNlcjpwYXNz"), + ); + request.headers_mut().append( + header::AUTHORIZATION, + HeaderValue::from_static("Bearer publisher-origin-credential"), + ); + request + .extensions_mut() + .insert(crate::auth::EdgeTerminatedAuthorization::for_test()); + + let response = run(&settings, &services, request).await; + assert_eq!( + response + .headers() + .get(HEADER_X_TS_TEMPLATE_CACHE) + .and_then(|value| value.to_str().ok()), + Some("bypass-request"), + "an ambiguous authorization must remain bound for the origin even if a marker is present" + ); + assert_eq!( + cache.lookups.lock().expect("should lock lookups").len(), + 0, + "repeated authorization must bypass before the shared template lookup" + ); + } + + #[tokio::test] + async fn an_edge_terminated_authorization_still_shares_its_template() { + let stub = Arc::new(StubHttpClient::new()); + let cache = Arc::new(MemoryTemplateCache::default()); + let settings = Arc::new(settings_with_mode("esi")); + let services = services(Arc::clone(&stub), Arc::clone(&cache)); + + // One origin response for two requests: the warm read is asserted by the + // fixture running dry, exactly as in the unauthenticated case. + queue_shareable_html(&stub); + + let authorized_navigation = || { + let mut request = navigation_request(); + request.headers_mut().insert( + header::AUTHORIZATION, + HeaderValue::from_static("Basic dXNlcjpwYXNz"), + ); + request + .extensions_mut() + .insert(crate::auth::EdgeTerminatedAuthorization::for_test()); + request + }; + + let cold = run(&settings, &services, authorized_navigation()).await; + assert_eq!( + cold.headers() + .get(HEADER_X_TS_TEMPLATE_CACHE) + .and_then(|value| value.to_str().ok()), + Some("miss-stored"), + "a credential this edge terminated must be allowed to fill the shared template" + ); + let first = body_of(cold).await; + + let warm = run(&settings, &services, authorized_navigation()).await; + assert_eq!( + warm.headers() + .get(HEADER_X_TS_TEMPLATE_CACHE) + .and_then(|value| value.to_str().ok()), + Some("hit"), + "the second gated request must read the template the first one stored" + ); + let second = body_of(warm).await; + + assert_eq!( + stub.recorded_request_uris().len(), + 1, + "the warm gated request must not fetch the origin" + ); + assert_eq!( + second, first, + "the gated warm response must be byte-identical to the stored template" + ); + } + #[tokio::test] async fn only_the_cold_miss_uses_the_platform_assembler() { let stub = Arc::new(StubHttpClient::new()); From 368f03d95f8d4737fb1f0f99d2e36e6867e97875 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Fri, 28 Aug 2026 16:01:56 +0530 Subject: [PATCH 2/3] docs: plan PR 1070 review remediation --- ...08-28-pr-1070-review-remediation-design.md | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-28-pr-1070-review-remediation-design.md diff --git a/docs/superpowers/specs/2026-08-28-pr-1070-review-remediation-design.md b/docs/superpowers/specs/2026-08-28-pr-1070-review-remediation-design.md new file mode 100644 index 000000000..f3bafc53c --- /dev/null +++ b/docs/superpowers/specs/2026-08-28-pr-1070-review-remediation-design.md @@ -0,0 +1,39 @@ +# PR 1070 Review Remediation Design + +## Goal + +Address every review finding on PR 1070 while preserving its intended behavior: a request carrying exactly the Basic credential validated by Trusted Server may use the shared ESI template cache, while pass-through, repeated, or subsequently replaced authorization values must bypass it. + +## Authorization marker + +`EdgeTerminatedAuthorization` will store a SHA-256 digest of the exact raw `Authorization` header value accepted by `enforce_basic_auth`. The raw credential will not be duplicated in request extensions. The marker will expose a crate-private predicate that returns true only when the request still has exactly one authorization value and its digest matches the validated value. + +The publisher cache gate will use that predicate. No authorization value is eligible, one matching marked value is eligible, and every other case disqualifies template sharing. This keeps the safety check next to the cache decision and remains correct if DataDome or another later request filter replaces or appends `Authorization`. + +## Configuration invariant + +`CreativeOpportunitiesConfig::validate_runtime` will reject `Authorization` in `template_cache_vary`, case-insensitively, alongside the existing `Cookie` rejection. This ensures an origin response declaring `Vary: Authorization` is refused instead of incorporating credential bytes into shared-template key material. + +## Cleanup + +The cache-gate boolean and helper parameters will be renamed from `request_had_authorization` to `authorization_disqualifies`. The marker documentation and publisher comment will be shortened, the unnecessary handler scoping block will be removed, and the repeated mutable-request comments will be removed from the Fastly, Axum, Cloudflare, and Spin middleware copies. + +## Documentation + +The configuration guide will distinguish pass-through and repeated authorization values from one unchanged, edge-validated Basic credential. It will state that Trusted Server forwards the credential, that an origin depending on it must return `Vary: Authorization` (which prevents template storage), and that whole-site staging gates should use an alias-proof raw-path expression such as `^/` rather than a decoded-path assumption. + +## Tests + +Tests will be added or updated for: + +- digest insertion after successful Basic authentication; +- cache bypass after the validated authorization value is replaced; +- repeated and pass-through authorization values; +- case-insensitive rejection of `Authorization` in `template_cache_vary`; +- a Fastly adapter dispatch path that performs Basic auth, crosses the router boundary, stores a cold ESI template, and hits it on the warm request. + +The adapter seam test will use test-local platform implementations and a router composed with the real Fastly `AuthMiddleware`, then invoke the real publisher handler. It will not add production dependency-injection fields solely for testing. + +## Verification and review resolution + +Focused tests will run after each behavior change. Final verification will include repository formatting, Fastly/Axum/Cloudflare/Spin adapter tests, and the corresponding target-specific clippy aliases. After the fixes are committed and pushed, each inline GitHub thread will receive a concise technical reply and be resolved. From 0c47533cd920879ae49554618f7e35a8659ffa70 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Fri, 28 Aug 2026 16:33:30 +0530 Subject: [PATCH 3/3] fix: address PR 1070 review feedback --- .../src/middleware.rs | 3 - .../src/middleware.rs | 3 - .../trusted-server-adapter-fastly/src/app.rs | 307 +++++++++++++++++- .../src/middleware.rs | 3 - .../src/middleware.rs | 3 - crates/trusted-server-core/src/auth.rs | 109 ++++--- .../src/creative_opportunities.rs | 31 +- crates/trusted-server-core/src/publisher.rs | 88 +++-- docs/guide/configuration.md | 35 +- ...08-28-pr-1070-review-remediation-design.md | 39 --- 10 files changed, 477 insertions(+), 144 deletions(-) delete mode 100644 docs/superpowers/specs/2026-08-28-pr-1070-review-remediation-design.md diff --git a/crates/trusted-server-adapter-axum/src/middleware.rs b/crates/trusted-server-adapter-axum/src/middleware.rs index 974a23e46..fd11d7728 100644 --- a/crates/trusted-server-adapter-axum/src/middleware.rs +++ b/crates/trusted-server-adapter-axum/src/middleware.rs @@ -105,9 +105,6 @@ impl AuthMiddleware { #[async_trait(?Send)] impl Middleware for AuthMiddleware { async fn handle(&self, mut ctx: RequestContext, next: Next<'_>) -> Result { - // Takes the request mutably because `enforce_basic_auth` marks requests - // whose credential it consumed itself; the shared template cache gate - // reads that marker later. match enforce_basic_auth(&self.settings, ctx.request_mut()) { Ok(Some(response)) => return Ok(response), Ok(None) => {} diff --git a/crates/trusted-server-adapter-cloudflare/src/middleware.rs b/crates/trusted-server-adapter-cloudflare/src/middleware.rs index 1e5f79ef3..14efed56a 100644 --- a/crates/trusted-server-adapter-cloudflare/src/middleware.rs +++ b/crates/trusted-server-adapter-cloudflare/src/middleware.rs @@ -113,9 +113,6 @@ impl AuthMiddleware { #[async_trait(?Send)] impl Middleware for AuthMiddleware { async fn handle(&self, mut ctx: RequestContext, next: Next<'_>) -> Result { - // Takes the request mutably because `enforce_basic_auth` marks requests - // whose credential it consumed itself; the shared template cache gate - // reads that marker later. match enforce_basic_auth(&self.settings, ctx.request_mut()) { Ok(Some(response)) => return Ok(response), Ok(None) => {} diff --git a/crates/trusted-server-adapter-fastly/src/app.rs b/crates/trusted-server-adapter-fastly/src/app.rs index 5e78a87ed..06a0a155f 100644 --- a/crates/trusted-server-adapter-fastly/src/app.rs +++ b/crates/trusted-server-adapter-fastly/src/app.rs @@ -1300,12 +1300,16 @@ impl Hooks for TrustedServerApp { #[cfg(test)] mod tests { - use std::sync::Arc; + use std::collections::HashMap; + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::{Arc, Mutex}; + use std::time::Duration; use super::{ - AppState, NAMED_ROUTES, NamedRouteHandler, PAGE_BIDS_LEGACY_PATH, PAGE_BIDS_PATH, - TrustedServerApp, build_per_request_services, build_state_from_settings, - startup_error_router, + AppState, AuctionDispatch, EcContext, EdgeCacheHeader, HandlerFuture, NAMED_ROUTES, + NamedRouteHandler, PAGE_BIDS_LEGACY_PATH, PAGE_BIDS_PATH, TrustedServerApp, + build_per_request_services, build_state_from_settings, handle_publisher_request, + publisher_response_into_streaming_response, startup_error_router, }; use base64::Engine as _; use bytes::Bytes; @@ -1316,7 +1320,6 @@ mod tests { use edgezero_core::params::PathParams; use edgezero_core::router::RouterService; use std::net::{IpAddr, Ipv4Addr}; - use std::sync::Mutex; use error_stack::Report; use futures::executor::block_on; @@ -1331,7 +1334,9 @@ mod tests { use trusted_server_core::platform::{ ClientInfo, PlatformBackend, PlatformBackendSpec, PlatformError, PlatformHttpClient, PlatformHttpRequest, PlatformKvStore, PlatformPendingRequest, PlatformResponse, - PlatformSelectResult, RuntimeServices, + PlatformSelectResult, PlatformTemplateCache, PlatformTemplateCacheReservation, + RuntimeServices, TemplateCacheError, TemplateCacheKey, TemplateCacheLookup, + TemplateCacheMiss, TemplateCacheReservation, TemplateEntry, TemplateMetadata, }; use trusted_server_core::settings::Settings; @@ -2611,6 +2616,296 @@ mod tests { .build() } + #[derive(Default)] + struct DispatchTemplateCache { + entries: Arc>>, + } + + struct DispatchTemplateReservation { + entries: Arc>>, + key: TemplateCacheKey, + } + + impl PlatformTemplateCacheReservation for DispatchTemplateReservation { + fn insert( + self: Box, + metadata: &TemplateMetadata, + body: Vec, + _max_age: Duration, + ) -> Result<(), TemplateCacheError> { + self.entries.lock().expect("should lock entries").insert( + self.key.to_cache_key(), + TemplateEntry { + metadata: metadata.clone(), + body, + }, + ); + Ok(()) + } + + fn cancel(self: Box) -> Result<(), TemplateCacheError> { + Ok(()) + } + } + + #[async_trait::async_trait(?Send)] + impl PlatformTemplateCache for DispatchTemplateCache { + async fn lookup_or_reserve( + &self, + key: &TemplateCacheKey, + ) -> Result { + if let Some(entry) = self + .entries + .lock() + .expect("should lock entries") + .get(&key.to_cache_key()) + .cloned() + { + return Ok(TemplateCacheLookup::Hit(entry)); + } + Ok(TemplateCacheLookup::Reserved( + TemplateCacheReservation::new(Box::new(DispatchTemplateReservation { + entries: Arc::clone(&self.entries), + key: key.clone(), + })), + )) + } + + async fn get(&self, key: &TemplateCacheKey) -> Result { + self.entries + .lock() + .expect("should lock entries") + .get(&key.to_cache_key()) + .cloned() + .ok_or(TemplateCacheMiss::NotFound) + } + + async fn put( + &self, + key: &TemplateCacheKey, + metadata: &TemplateMetadata, + body: Vec, + _max_age: Duration, + ) -> Result<(), TemplateCacheError> { + self.entries.lock().expect("should lock entries").insert( + key.to_cache_key(), + TemplateEntry { + metadata: metadata.clone(), + body, + }, + ); + Ok(()) + } + + async fn purge_url(&self, key: &TemplateCacheKey) -> Result<(), TemplateCacheError> { + self.entries + .lock() + .expect("should lock entries") + .remove(&key.to_cache_key()); + Ok(()) + } + + async fn purge_all(&self) -> Result<(), TemplateCacheError> { + self.entries.lock().expect("should lock entries").clear(); + Ok(()) + } + } + + #[derive(Default)] + struct DispatchOriginClient { + calls: AtomicUsize, + } + + #[async_trait::async_trait(?Send)] + impl PlatformHttpClient for DispatchOriginClient { + async fn send( + &self, + _request: PlatformHttpRequest, + ) -> Result> { + self.calls.fetch_add(1, Ordering::Relaxed); + let response = edgezero_core::http::response_builder() + .status(StatusCode::OK) + .header(header::CONTENT_TYPE, "text/html; charset=utf-8") + .header(header::CACHE_CONTROL, "public, max-age=300") + .body(Body::from( + b"origin".as_ref(), + )) + .map_err(|_| Report::new(PlatformError::HttpClient))?; + Ok(PlatformResponse::new(response)) + } + + async fn send_async( + &self, + _request: PlatformHttpRequest, + ) -> Result> { + Err(Report::new(PlatformError::Unsupported)) + } + + async fn select( + &self, + _pending_requests: Vec, + ) -> Result> { + Err(Report::new(PlatformError::Unsupported)) + } + } + + #[test] + fn dispatch_edge_authenticated_esi_request_stores_then_hits_template() { + let settings = Arc::new( + Settings::from_toml( + r#" + [[handlers]] + path = "^/secure" + username = "user" + password = "pass" + + [[handlers]] + path = "^/_ts/admin" + username = "admin" + password = "admin-pass" + + [publisher] + domain = "test-publisher.com" + cookie_domain = ".test-publisher.com" + origin_url = "https://origin.test-publisher.com" + proxy_secret = "unit-test-proxy-secret" + + [ec] + passphrase = "test-secret-key-32-bytes-minimum" + + [auction] + enabled = true + providers = [] + + [creative_opportunities] + gam_network_id = "99999" + assembly_mode = "esi" + + [[creative_opportunities.slot]] + id = "test-slot" + page_patterns = ["/secure/article"] + formats = [{ width = 728, height = 90 }] + "#, + ) + .expect("should parse dispatch cache settings"), + ); + let cache = Arc::new(DispatchTemplateCache::default()); + let origin = Arc::new(DispatchOriginClient::default()); + let services = RuntimeServices::builder() + .config_store(Arc::new(crate::platform::FastlyPlatformConfigStore)) + .secret_store(Arc::new(crate::platform::FastlyPlatformSecretStore)) + .kv_store(Arc::new(NoopKvStore) as Arc) + .template_cache(Arc::clone(&cache) as Arc) + .template_assembler(Arc::new(crate::esi_assembly::FastlyTemplateAssembler)) + .backend(Arc::new(FixedBackend)) + .http_client(Arc::clone(&origin) as Arc) + .geo(Arc::new(crate::platform::FastlyPlatformGeo)) + .client_info(ClientInfo::default()) + .build(); + let registry = Arc::new( + IntegrationRegistry::new(&settings).expect("should build integration registry"), + ); + let orchestrator = Arc::new( + trusted_server_core::auction::build_orchestrator(&settings) + .expect("should build auction orchestrator"), + ); + + let handler = { + let settings = Arc::clone(&settings); + let services = services.clone(); + let registry = Arc::clone(®istry); + let orchestrator = Arc::clone(&orchestrator); + move |ctx: RequestContext| { + let settings = Arc::clone(&settings); + let services = services.clone(); + let registry = Arc::clone(®istry); + let orchestrator = Arc::clone(&orchestrator); + Box::pin(async move { + let request = ctx.into_request(); + let method = request.method().clone(); + let mut ec_context = + match EcContext::read_from_request(&settings, &request, &services) { + Ok(context) => context, + Err(report) => return Ok(super::http_error(&report)), + }; + let response = match handle_publisher_request( + &settings, + &services, + None, + &mut ec_context, + AuctionDispatch { + orchestrator: &orchestrator, + slots: settings.creative_opportunity_slots(), + registry: None, + }, + request, + EdgeCacheHeader::SurrogateControl, + ) + .await + { + Ok(response) => response, + Err(report) => return Ok(super::http_error(&report)), + }; + match publisher_response_into_streaming_response( + response, + &method, + Arc::clone(&settings), + ®istry, + orchestrator, + services, + ) + .await + { + Ok(response) => Ok(response), + Err(report) => Ok(super::http_error(&report)), + } + }) as HandlerFuture + } + }; + let router = RouterService::builder() + .middleware(crate::middleware::AuthMiddleware::new(Arc::clone( + &settings, + ))) + .route("/secure/article", Method::GET, handler) + .build(); + let authorized_request = || { + request_builder() + .method(Method::GET) + .uri("https://test-publisher.com/secure/article") + .header(header::HOST, "test-publisher.com") + .header(header::AUTHORIZATION, "Basic dXNlcjpwYXNz") + .header("sec-fetch-dest", "document") + .header("sec-fetch-mode", "navigate") + .body(Body::empty()) + .expect("should build authorized navigation") + }; + + let cold = route(&router, authorized_request()); + assert_eq!( + cold.headers() + .get("x-ts-template-cache") + .and_then(|value| value.to_str().ok()), + Some("miss-stored") + ); + block_on(cold.into_body().into_bytes_bounded(1024 * 1024)) + .expect("should drain cold response"); + + let warm = route(&router, authorized_request()); + assert_eq!( + warm.headers() + .get("x-ts-template-cache") + .and_then(|value| value.to_str().ok()), + Some("hit") + ); + block_on(warm.into_body().into_bytes_bounded(1024 * 1024)) + .expect("should drain warm response"); + assert_eq!( + origin.calls.load(Ordering::Relaxed), + 1, + "the warm dispatch must not fetch the publisher origin" + ); + } + #[test] fn dispatch_asset_fallback_streams_origin_body_without_buffering() { // Regression guard for the EdgeZero asset streaming cutover: a successful diff --git a/crates/trusted-server-adapter-fastly/src/middleware.rs b/crates/trusted-server-adapter-fastly/src/middleware.rs index 7b2bb636e..283f16255 100644 --- a/crates/trusted-server-adapter-fastly/src/middleware.rs +++ b/crates/trusted-server-adapter-fastly/src/middleware.rs @@ -128,9 +128,6 @@ impl AuthMiddleware { #[async_trait(?Send)] impl Middleware for AuthMiddleware { async fn handle(&self, mut ctx: RequestContext, next: Next<'_>) -> Result { - // Takes the request mutably because `enforce_basic_auth` marks requests - // whose credential it consumed itself; the shared template cache gate - // reads that marker later. match enforce_basic_auth(&self.settings, ctx.request_mut()) { Ok(Some(response)) => return Ok(response), Ok(None) => {} diff --git a/crates/trusted-server-adapter-spin/src/middleware.rs b/crates/trusted-server-adapter-spin/src/middleware.rs index 9ee6d2b50..d7a09987a 100644 --- a/crates/trusted-server-adapter-spin/src/middleware.rs +++ b/crates/trusted-server-adapter-spin/src/middleware.rs @@ -106,9 +106,6 @@ impl AuthMiddleware { #[async_trait(?Send)] impl Middleware for AuthMiddleware { async fn handle(&self, mut ctx: RequestContext, next: Next<'_>) -> Result { - // Takes the request mutably because `enforce_basic_auth` marks requests - // whose credential it consumed itself; the shared template cache gate - // reads that marker later. match enforce_basic_auth(&self.settings, ctx.request_mut()) { Ok(Some(response)) => return Ok(response), Ok(None) => {} diff --git a/crates/trusted-server-core/src/auth.rs b/crates/trusted-server-core/src/auth.rs index 68e590cd7..f5e45bbd3 100644 --- a/crates/trusted-server-core/src/auth.rs +++ b/crates/trusted-server-core/src/auth.rs @@ -11,43 +11,35 @@ use crate::settings::Settings; const BASIC_AUTH_REALM: &str = r#"Basic realm="Trusted Server""#; -/// Marker recording that this request's `Authorization` header was consumed and -/// validated by a Trusted Server handler at the edge. +/// Marks the single `Authorization` value Trusted Server validated. /// -/// The shared template cache refuses every request carrying `Authorization`, -/// because an authorized response must never become a reader-neutral template. -/// That rule exists for credentials bound for the publisher origin, whose -/// response content TS cannot reason about. -/// -/// A credential this edge terminated is a different case. [`enforce_basic_auth`] -/// runs as middleware ahead of routing, so a request that reaches a handler for a -/// gated path has necessarily already satisfied that same handler. Every reader -/// able to look up a template stored from such a request has authenticated -/// against the same credential, so reuse is not a cross-reader disclosure. -/// -/// Absence of this marker on a request that still carries `Authorization` means -/// the credential is pass-through, and the template cache continues to refuse it. -/// -/// # Invariants -/// -/// The private field makes [`enforce_basic_auth`] the only code that can produce -/// this marker. It grants shared-template eligibility to a request that would -/// otherwise be refused, so being unforgeable outside this module is the whole -/// point: a caller cannot assert "already authenticated" without having actually -/// checked. [`enforce_basic_auth`] also clears any inherited marker before it -/// decides, so the value can never outlive the check that produced it. +/// The shared template cache may exempt this value from its normal authorization +/// bypass. [`enforce_basic_auth`] clears any existing marker before checking and +/// inserts a digest-bound marker only after successful authentication. #[derive(Debug, Clone)] -pub(crate) struct EdgeTerminatedAuthorization(()); +pub(crate) struct EdgeTerminatedAuthorization([u8; 32]); impl EdgeTerminatedAuthorization { + fn digest(value: &[u8]) -> [u8; 32] { + Sha256::digest(value).into() + } + + pub(crate) fn matches(&self, headers: &http::HeaderMap) -> bool { + let mut values = headers.get_all(header::AUTHORIZATION).iter(); + let Some(value) = values.next() else { + return false; + }; + values.next().is_none() && self.0 == Self::digest(value.as_bytes()) + } + /// Builds the marker without performing a credential check. /// /// Test-only. Production code obtains this marker exclusively by passing /// [`enforce_basic_auth`], which is what makes it meaningful. #[cfg(test)] #[must_use] - pub(crate) const fn for_test() -> Self { - Self(()) + pub(crate) fn for_test(value: &str) -> Self { + Self(Self::digest(value.as_bytes())) } } @@ -94,22 +86,17 @@ pub fn enforce_basic_auth( // nothing checked. req.extensions_mut().remove::(); - // Scoped so the path borrow ends before the successful branch marks the - // request. `handler` borrows `settings`, not `req`. - let handler = { - let path = req.uri().path(); - let Some(handler) = settings.handler_for_path(path)? else { - if Settings::is_admin_path(path) { - return Err(Report::new(TrustedServerError::Configuration { - message: format!("Admin path `{path}` has no configured handler"), - })); - } - return Ok(None); - }; - handler + let path = req.uri().path(); + let Some(handler) = settings.handler_for_path(path)? else { + if Settings::is_admin_path(path) { + return Err(Report::new(TrustedServerError::Configuration { + message: format!("Admin path `{path}` has no configured handler"), + })); + } + return Ok(None); }; - let Some((username, password)) = extract_credentials(req) else { + let Some((username, password, authorization_digest)) = extract_credentials(req) else { return Ok(Some(unauthorized_response())); }; @@ -128,7 +115,8 @@ pub fn enforce_basic_auth( if bool::from(username_match & password_match) { // Record that TS itself consumed this credential, so the shared template // cache can distinguish it from a credential meant for the origin. - req.extensions_mut().insert(EdgeTerminatedAuthorization(())); + req.extensions_mut() + .insert(EdgeTerminatedAuthorization(authorization_digest)); Ok(None) } else { log::warn!("Basic auth failed for path: {}", req.uri().path()); @@ -136,12 +124,14 @@ pub fn enforce_basic_auth( } } -fn extract_credentials(req: &Request) -> Option<(String, String)> { +fn extract_credentials(req: &Request) -> Option<(String, String, [u8; 32])> { let mut header_values = req.headers().get_all(header::AUTHORIZATION).iter(); - let header_value = header_values.next()?.to_str().ok()?; + let header_value = header_values.next()?; if header_values.next().is_some() { return None; } + let authorization_digest = EdgeTerminatedAuthorization::digest(header_value.as_bytes()); + let header_value = header_value.to_str().ok()?; let mut parts = header_value.splitn(2, ' '); let scheme = parts.next()?.trim(); @@ -161,7 +151,7 @@ fn extract_credentials(req: &Request) -> Option<(String, String)> { let username = credentials_parts.next()?.to_owned(); let password = credentials_parts.next()?.to_owned(); - Some((username, password)) + Some((username, password, authorization_digest)) } fn unauthorized_response() -> Response { @@ -232,11 +222,26 @@ mod tests { .is_none(), "valid credentials should be admitted" ); + let marker = req + .extensions() + .get::() + .expect("should mark a credential this edge consumed"); assert!( - req.extensions() - .get::() - .is_some(), - "a credential this edge consumed should be marked so the template cache can share it" + marker.matches(req.headers()), + "the marker should match the unchanged validated authorization" + ); + + req.headers_mut().insert( + header::AUTHORIZATION, + HeaderValue::from_static("Bearer publisher-origin-credential"), + ); + let marker = req + .extensions() + .get::() + .expect("should retain the marker after an unrelated mutation"); + assert!( + !marker.matches(req.headers()), + "the marker must not match a replacement authorization value" ); } @@ -272,7 +277,7 @@ mod tests { let mut req = build_request(Method::GET, "https://example.com/open"); set_authorization(&mut req, "Basic dXNlcjpwYXNz"); req.extensions_mut() - .insert(EdgeTerminatedAuthorization::for_test()); + .insert(EdgeTerminatedAuthorization::for_test("Basic dXNlcjpwYXNz")); assert!( enforce_basic_auth(&settings, &mut req) @@ -295,7 +300,9 @@ mod tests { let encoded = STANDARD.encode("user:wrong-pass"); set_authorization(&mut req, &format!("Basic {encoded}")); req.extensions_mut() - .insert(EdgeTerminatedAuthorization::for_test()); + .insert(EdgeTerminatedAuthorization::for_test( + "Basic dXNlcjp3cm9uZy1wYXNz", + )); let response = enforce_basic_auth(&settings, &mut req) .expect("should evaluate auth") diff --git a/crates/trusted-server-core/src/creative_opportunities.rs b/crates/trusted-server-core/src/creative_opportunities.rs index e83fef9cb..ab272e4da 100644 --- a/crates/trusted-server-core/src/creative_opportunities.rs +++ b/crates/trusted-server-core/src/creative_opportunities.rs @@ -313,10 +313,10 @@ pub struct CreativeOpportunitiesConfig { /// /// **Unset or empty means no operator-stated header is covered, so any origin /// `Vary` other than structurally covered `Accept-Encoding` disqualifies the - /// response.** `Cookie` may never be configured: a per-cookie object violates the - /// reader-neutral template contract. This fail-closed default prevents a deployment - /// that has not stated what its origin varies on from gaining a shared cache by - /// omission. + /// response.** `Cookie` and `Authorization` may never be configured: their values + /// are not reader-neutral template dimensions. This fail-closed default prevents a + /// deployment that has not stated what its origin varies on from gaining a shared + /// cache by omission. /// /// Spike-only. Same `Option` + `skip_serializing_if` reasoning as `assembly_mode`. #[serde(default, skip_serializing_if = "Option::is_none")] @@ -479,6 +479,15 @@ impl CreativeOpportunitiesConfig { .to_string(), ); } + if names + .iter() + .any(|name| name.eq_ignore_ascii_case("authorization")) + { + return Err( + "template_cache_vary must not include Authorization; shared templates are keyed on the edge-terminated credential decision, not the header value" + .to_string(), + ); + } } // A network ID is required only when a slot renders the default @@ -2107,6 +2116,20 @@ mod tests { .validate_runtime() .expect_err("per-cookie templates violate the reader-neutral shared-template contract"); assert!(err.contains("Cookie"), "unexpected error: {err}"); + + for name in ["Authorization", "aUtHoRiZaTiOn"] { + let authorization_key: CreativeOpportunitiesConfig = toml::from_str(&format!( + r#" + gam_network_id = "99999" + template_cache_vary = ["{name}"] + "#, + )) + .expect("shape should deserialize before runtime validation"); + let err = authorization_key + .validate_runtime() + .expect_err("authorization must not enter shared-template cache keys"); + assert!(err.contains("Authorization"), "unexpected error: {err}"); + } } #[test] diff --git a/crates/trusted-server-core/src/publisher.rs b/crates/trusted-server-core/src/publisher.rs index 3cf6a76af..40bb7ea27 100644 --- a/crates/trusted-server-core/src/publisher.rs +++ b/crates/trusted-server-core/src/publisher.rs @@ -4340,23 +4340,16 @@ pub async fn handle_publisher_request( } ); - // Recorded before the request is consumed by the origin send: the template cache gate - // below needs it, and an authorized response must never become a shared - // template. - // - // A credential this edge already terminated is exempt. Basic auth runs as - // middleware ahead of routing, so reaching here on a gated path means the same - // handler already validated the request; every reader that can look the template - // up has satisfied that same credential. Without the marker the credential is - // pass-through to the origin and still disqualifies. See - // [`crate::auth::EdgeTerminatedAuthorization`]. + // Capture cache eligibility before the origin send consumes the request. One + // unchanged marked Authorization value passed edge auth; unmarked, replaced, + // or repeated values remain pass-through and bypass sharing. let authorization_value_count = req.headers().get_all(header::AUTHORIZATION).iter().count(); - let request_had_authorization = match authorization_value_count { + let authorization_disqualifies = match authorization_value_count { 0 => false, 1 => req .extensions() .get::() - .is_none(), + .is_none_or(|marker| !marker.matches(req.headers())), _ => true, }; let request_had_cookie = req.headers().contains_key(header::COOKIE); @@ -4404,7 +4397,7 @@ pub async fn handle_publisher_request( let request_can_use_shared_template = method_is_cacheable && matches!(assembly_mode, AssemblyMode::Esi) && !request_host.is_empty() - && !request_had_authorization + && !authorization_disqualifies && !cookie_disqualifies && !request_requires_origin && reader_supports_assembly; @@ -4650,7 +4643,7 @@ pub async fn handle_publisher_request( let mut template_cache_key = template_cache_reservation.and_then(|reservation| { match template_cache_ttl( assembly_mode, - request_had_authorization, + authorization_disqualifies, cookie_disqualifies, response.status(), &gate_content_type, @@ -5737,7 +5730,7 @@ impl TemplateCachePolicy { #[cfg(test)] pub(crate) fn template_cache_bypass_reason( mode: AssemblyMode, - request_had_authorization: bool, + authorization_disqualifies: bool, cookie_disqualifies: bool, status: StatusCode, content_type: &str, @@ -5747,7 +5740,7 @@ pub(crate) fn template_cache_bypass_reason( let policy = TemplateCachePolicy::for_test(key_vary, Duration::from_secs(60)); template_cache_ttl( mode, - request_had_authorization, + authorization_disqualifies, cookie_disqualifies, status, content_type, @@ -6003,7 +5996,7 @@ fn replayable_policy_headers( fn template_cache_ttl( mode: AssemblyMode, - request_had_authorization: bool, + authorization_disqualifies: bool, cookie_disqualifies: bool, status: StatusCode, content_type: &str, @@ -6013,7 +6006,7 @@ fn template_cache_ttl( if matches!(mode, AssemblyMode::Inline) { return Err(TemplateCacheBypassReason::InlineMode); } - if request_had_authorization { + if authorization_disqualifies { return Err(TemplateCacheBypassReason::AuthorizedRequest); } if cookie_disqualifies { @@ -9037,7 +9030,9 @@ mod tests { ); request .extensions_mut() - .insert(crate::auth::EdgeTerminatedAuthorization::for_test()); + .insert(crate::auth::EdgeTerminatedAuthorization::for_test( + "Basic dXNlcjpwYXNz", + )); let response = run(&settings, &services, request).await; assert_eq!( @@ -9055,6 +9050,55 @@ mod tests { ); } + #[tokio::test] + async fn a_replaced_edge_terminated_authorization_bypasses_the_shared_template() { + let stub = Arc::new(StubHttpClient::new()); + let cache = Arc::new(MemoryTemplateCache::default()); + let mut settings = settings_with_mode("esi"); + settings.handlers[0] = toml::from_str( + r#" + path = "^/article" + username = "user" + password = "pass" + "#, + ) + .expect("should parse article auth handler"); + let settings = Arc::new(settings); + let services = services(Arc::clone(&stub), Arc::clone(&cache)); + queue_shareable_html(&stub); + + let mut request = navigation_request(); + request.headers_mut().insert( + header::AUTHORIZATION, + HeaderValue::from_static("Basic dXNlcjpwYXNz"), + ); + assert!( + crate::auth::enforce_basic_auth(&settings, &mut request) + .expect("should evaluate auth") + .is_none(), + "the original credential should pass edge auth" + ); + request.headers_mut().insert( + header::AUTHORIZATION, + HeaderValue::from_static("Bearer publisher-origin-credential"), + ); + + let response = run(&settings, &services, request).await; + assert_eq!( + response + .headers() + .get(HEADER_X_TS_TEMPLATE_CACHE) + .and_then(|value| value.to_str().ok()), + Some("bypass-request"), + "a replacement credential was not validated at the edge and must bypass sharing" + ); + assert_eq!( + cache.lookups.lock().expect("should lock lookups").len(), + 0, + "a replaced authorization must bypass before the shared template lookup" + ); + } + #[tokio::test] async fn an_edge_terminated_authorization_still_shares_its_template() { let stub = Arc::new(StubHttpClient::new()); @@ -9072,9 +9116,9 @@ mod tests { header::AUTHORIZATION, HeaderValue::from_static("Basic dXNlcjpwYXNz"), ); - request - .extensions_mut() - .insert(crate::auth::EdgeTerminatedAuthorization::for_test()); + request.extensions_mut().insert( + crate::auth::EdgeTerminatedAuthorization::for_test("Basic dXNlcjpwYXNz"), + ); request }; diff --git a/docs/guide/configuration.md b/docs/guide/configuration.md index 33afb0feb..068518aee 100644 --- a/docs/guide/configuration.md +++ b/docs/guide/configuration.md @@ -753,6 +753,11 @@ from ID-shaped samples: a pattern such as matcher (`^/_ts/admin`, or `^/_ts/admin/ec/` alongside the other admin patterns). +Handler expressions match the raw URI path, while a publisher origin may decode +percent-encoded aliases before routing. For a whole-site staging gate, use +`path = "^/"`; do not rely on a decoded-path prefix such as `^/secure` to protect +equivalent origin paths. + Startup also fails when any handler — admin or not — uses a placeholder or well-known weak password (`changeme`, `password`, `admin`, or a `replace-with-…` template value). Handler selection is first-match-wins, so a @@ -1706,9 +1711,10 @@ The cache fails closed. A template is stored only for a `GET` with a processable `200 text/html` origin response, a supported content encoding, and explicit positive shared freshness. `private`, `no-store`, `no-cache`, exhausted or malformed freshness, `Set-Cookie`, `Vary: *`, `Vary: Cookie`, uncovered `Vary` -names, response-bound CSP nonces, authorization, diagnostics sessions, range or -conditional requests, positive or malformed request `max-age`, `min-fresh`, and -unsupported CDN-specific cache policy fields all bypass the template cache. Fastly +names, response-bound CSP nonces, pass-through or ambiguous authorization, +diagnostics sessions, range or conditional requests, positive or malformed +request `max-age`, `min-fresh`, and unsupported CDN-specific cache policy fields +all bypass the template cache. Fastly `Surrogate-Control` is the narrow exception: the template cache accepts exactly one positive `max-age` plus optional valid `stale-while-revalidate` and `stale-if-error` delta-seconds. Restrictive, duplicated, malformed, or unknown directives fail @@ -1725,6 +1731,14 @@ response and runs a new per-reader auction. Explicit `no-cache`, `no-store`, positive or malformed request `max-age`, range, and conditional requests still bypass the template cache. Check `X-TS-Template-Cache: hit` to verify template reuse. +Authorization has one narrow exception. A request carrying exactly the same +single Basic credential that Trusted Server just validated at the edge may share +a template; pass-through, repeated, appended, or replaced values still bypass. +Trusted Server does not remove the validated header, so it remains forwarded to +the publisher origin. If the origin uses that credential to select response +content, it must declare `Vary: Authorization`; that response is deliberately not +stored as a shared template. + `template_cache_vary` is necessary because lookup occurs before the origin can return `Vary`. Presence, empty values, repeated raw field values, host/scheme, origin identity, complete template-shaping settings, TSJS content, and schema @@ -1733,13 +1747,14 @@ not: the stored template is decoded identity and the assembled result is encoded for each reader with `Vary: Accept-Encoding`. This assumes the origin's `Accept-Encoding` variants differ only by HTTP content coding, as normal compression negotiation does. Do not enable ESI for an origin that changes the -document's meaning based on `Accept-Encoding`. Never put `Cookie` in -`template_cache_vary`; startup rejects it because a per-cookie object is not a -reader-neutral template. With `origin_is_cookie_independent = false` (the safe -default), all cookie-bearing requests bypass. With it set to `true`, an origin -`Vary: Cookie` still overrides the assertion and refuses storage. -Every other name the origin emits in `Vary` must appear in the configured list; -an uncovered name safely refuses template storage. +document's meaning based on `Accept-Encoding`. Never put `Cookie` or +`Authorization` in `template_cache_vary`; startup rejects both because raw cookie +or credential values are not reader-neutral template dimensions. With +`origin_is_cookie_independent = false` (the safe default), all cookie-bearing +requests bypass. With it set to `true`, an origin `Vary: Cookie` still overrides +the assertion and refuses storage. Every other name the origin emits in `Vary` +must appear in the configured list; an uncovered name safely refuses template +storage. For a canary, inspect `X-TS-Template-Cache`. Its bounded values are `hit`, `miss-stored`, `miss-store-error`, `miss-reserved`, `bypass-request`, diff --git a/docs/superpowers/specs/2026-08-28-pr-1070-review-remediation-design.md b/docs/superpowers/specs/2026-08-28-pr-1070-review-remediation-design.md deleted file mode 100644 index f3bafc53c..000000000 --- a/docs/superpowers/specs/2026-08-28-pr-1070-review-remediation-design.md +++ /dev/null @@ -1,39 +0,0 @@ -# PR 1070 Review Remediation Design - -## Goal - -Address every review finding on PR 1070 while preserving its intended behavior: a request carrying exactly the Basic credential validated by Trusted Server may use the shared ESI template cache, while pass-through, repeated, or subsequently replaced authorization values must bypass it. - -## Authorization marker - -`EdgeTerminatedAuthorization` will store a SHA-256 digest of the exact raw `Authorization` header value accepted by `enforce_basic_auth`. The raw credential will not be duplicated in request extensions. The marker will expose a crate-private predicate that returns true only when the request still has exactly one authorization value and its digest matches the validated value. - -The publisher cache gate will use that predicate. No authorization value is eligible, one matching marked value is eligible, and every other case disqualifies template sharing. This keeps the safety check next to the cache decision and remains correct if DataDome or another later request filter replaces or appends `Authorization`. - -## Configuration invariant - -`CreativeOpportunitiesConfig::validate_runtime` will reject `Authorization` in `template_cache_vary`, case-insensitively, alongside the existing `Cookie` rejection. This ensures an origin response declaring `Vary: Authorization` is refused instead of incorporating credential bytes into shared-template key material. - -## Cleanup - -The cache-gate boolean and helper parameters will be renamed from `request_had_authorization` to `authorization_disqualifies`. The marker documentation and publisher comment will be shortened, the unnecessary handler scoping block will be removed, and the repeated mutable-request comments will be removed from the Fastly, Axum, Cloudflare, and Spin middleware copies. - -## Documentation - -The configuration guide will distinguish pass-through and repeated authorization values from one unchanged, edge-validated Basic credential. It will state that Trusted Server forwards the credential, that an origin depending on it must return `Vary: Authorization` (which prevents template storage), and that whole-site staging gates should use an alias-proof raw-path expression such as `^/` rather than a decoded-path assumption. - -## Tests - -Tests will be added or updated for: - -- digest insertion after successful Basic authentication; -- cache bypass after the validated authorization value is replaced; -- repeated and pass-through authorization values; -- case-insensitive rejection of `Authorization` in `template_cache_vary`; -- a Fastly adapter dispatch path that performs Basic auth, crosses the router boundary, stores a cold ESI template, and hits it on the warm request. - -The adapter seam test will use test-local platform implementations and a router composed with the real Fastly `AuthMiddleware`, then invoke the real publisher handler. It will not add production dependency-injection fields solely for testing. - -## Verification and review resolution - -Focused tests will run after each behavior change. Final verification will include repository formatting, Fastly/Axum/Cloudflare/Spin adapter tests, and the corresponding target-specific clippy aliases. After the fixes are committed and pushed, each inline GitHub thread will receive a concise technical reply and be resolved.