Resolve PR 628 round-1 review findings

prk-Jr · prk-Jr · commit 1973b5dce5d3 · 2026-05-28T10:35:21.000+05:30
Blocking:
- Default max_buffered_body_bytes to Some(16 MiB) so EdgeZero flag-flip
  fails-safe; operators must opt out for pages larger than 16 MiB

Non-blocking:
- Rename response_was_finalized_by_middleware to take_finalize_sentinel so
  the side-effecting header removal is visible in the name
- Inline apply_entry_point_finalize at its single call site; remove the
  thin wrapper function; update the test to call resolve_geo_for_response
  and apply_finalize_headers directly
- Extract build_state_from_settings(settings) so build_state() and the
  test-only app_state_for_settings() share a single constructor path
- BoundedWriter: use std::io::Error::other() for consistency
- Convert named_routes() -&gt; [NamedRoute; 9] to const NAMED_ROUTES: &amp;[NamedRoute];
  removes the size literal coupling and avoids a stack copy on every call
diff --git a/crates/trusted-server-adapter-fastly/src/app.rs b/crates/trusted-server-adapter-fastly/src/app.rs
@@ -98,14 +98,15 @@ pub(crate) struct AppState {
 /// Returns an error when settings, the auction orchestrator, or the integration
 /// registry fail to initialise.
 pub(crate) fn build_state() -> Result<Arc<AppState>, Report<TrustedServerError>> {
-    let settings = get_settings()?;
+    build_state_from_settings(get_settings()?)
+}
 
+pub(crate) fn build_state_from_settings(
+    settings: Settings,
+) -> Result<Arc<AppState>, Report<TrustedServerError>> {
     let orchestrator = build_orchestrator(&settings)?;
-
     let registry = IntegrationRegistry::new(&settings)?;
-
     let kv_store = Arc::new(UnavailableKvStore) as Arc<dyn PlatformKvStore>;
-
     Ok(Arc::new(AppState {
         settings: Arc::new(settings),
         orchestrator: Arc::new(orchestrator),
@@ -318,55 +319,53 @@ struct NamedRoute {
     handler: NamedRouteHandler,
 }
 
-fn named_routes() -> [NamedRoute; 9] {
-    [
-        NamedRoute {
-            path: "/.well-known/trusted-server.json",
-            primary_methods: &[Method::GET],
-            handler: NamedRouteHandler::TrustedServerDiscovery,
-        },
-        NamedRoute {
-            path: "/verify-signature",
-            primary_methods: &[Method::POST],
-            handler: NamedRouteHandler::VerifySignature,
-        },
-        NamedRoute {
-            path: "/admin/keys/rotate",
-            primary_methods: &[Method::POST],
-            handler: NamedRouteHandler::RotateKey,
-        },
-        NamedRoute {
-            path: "/admin/keys/deactivate",
-            primary_methods: &[Method::POST],
-            handler: NamedRouteHandler::DeactivateKey,
-        },
-        NamedRoute {
-            path: "/auction",
-            primary_methods: &[Method::POST],
-            handler: NamedRouteHandler::Auction,
-        },
-        NamedRoute {
-            path: "/first-party/proxy",
-            primary_methods: &[Method::GET],
-            handler: NamedRouteHandler::FirstPartyProxy,
-        },
-        NamedRoute {
-            path: "/first-party/click",
-            primary_methods: &[Method::GET],
-            handler: NamedRouteHandler::FirstPartyClick,
-        },
-        NamedRoute {
-            path: "/first-party/sign",
-            primary_methods: &[Method::GET, Method::POST],
-            handler: NamedRouteHandler::FirstPartySign,
-        },
-        NamedRoute {
-            path: "/first-party/proxy-rebuild",
-            primary_methods: &[Method::POST],
-            handler: NamedRouteHandler::FirstPartyProxyRebuild,
-        },
-    ]
-}
+const NAMED_ROUTES: &[NamedRoute] = &[
+    NamedRoute {
+        path: "/.well-known/trusted-server.json",
+        primary_methods: &[Method::GET],
+        handler: NamedRouteHandler::TrustedServerDiscovery,
+    },
+    NamedRoute {
+        path: "/verify-signature",
+        primary_methods: &[Method::POST],
+        handler: NamedRouteHandler::VerifySignature,
+    },
+    NamedRoute {
+        path: "/admin/keys/rotate",
+        primary_methods: &[Method::POST],
+        handler: NamedRouteHandler::RotateKey,
+    },
+    NamedRoute {
+        path: "/admin/keys/deactivate",
+        primary_methods: &[Method::POST],
+        handler: NamedRouteHandler::DeactivateKey,
+    },
+    NamedRoute {
+        path: "/auction",
+        primary_methods: &[Method::POST],
+        handler: NamedRouteHandler::Auction,
+    },
+    NamedRoute {
+        path: "/first-party/proxy",
+        primary_methods: &[Method::GET],
+        handler: NamedRouteHandler::FirstPartyProxy,
+    },
+    NamedRoute {
+        path: "/first-party/click",
+        primary_methods: &[Method::GET],
+        handler: NamedRouteHandler::FirstPartyClick,
+    },
+    NamedRoute {
+        path: "/first-party/sign",
+        primary_methods: &[Method::GET, Method::POST],
+        handler: NamedRouteHandler::FirstPartySign,
+    },
+    NamedRoute {
+        path: "/first-party/proxy-rebuild",
+        primary_methods: &[Method::POST],
+        handler: NamedRouteHandler::FirstPartyProxyRebuild,
+    },
+];
 
 fn named_route_handler(
     state: Arc<AppState>,
@@ -463,7 +462,7 @@ impl TrustedServerApp {
         // named route is registered from this single table, then every
         // non-primary publisher fallback method is registered from the same
         // row. Adding a named route now requires editing only this table.
-        for route in named_routes() {
+        for route in NAMED_ROUTES {
             for method in route.primary_methods {
                 router = router.route(
                     route.path,
@@ -510,7 +509,7 @@ impl Hooks for TrustedServerApp {
 
 #[cfg(test)]
 mod tests {
-    use super::{startup_error_router, AppState, TrustedServerApp};
+    use super::{build_state_from_settings, startup_error_router, AppState, TrustedServerApp};
 
     use std::sync::Arc;
 
@@ -520,11 +519,8 @@ mod tests {
     use error_stack::Report;
     use futures::executor::block_on;
     use serde_json::json;
-    use trusted_server_core::auction::build_orchestrator;
     use trusted_server_core::constants::HEADER_X_GEO_INFO_AVAILABLE;
     use trusted_server_core::error::TrustedServerError;
-    use trusted_server_core::integrations::IntegrationRegistry;
-    use trusted_server_core::platform::PlatformKvStore;
     use trusted_server_core::settings::Settings;
 
     fn settings_with_missing_consent_store() -> Settings {
@@ -566,17 +562,7 @@ mod tests {
     }
 
     fn app_state_for_settings(settings: Settings) -> Arc<AppState> {
-        let orchestrator =
-            build_orchestrator(&settings).expect("should build auction orchestrator");
-        let registry = IntegrationRegistry::new(&settings).expect("should create registry");
-        let kv_store = Arc::new(crate::platform::UnavailableKvStore) as Arc<dyn PlatformKvStore>;
-
-        Arc::new(AppState {
-            settings: Arc::new(settings),
-            orchestrator: Arc::new(orchestrator),
-            registry: Arc::new(registry),
-            kv_store,
-        })
+        build_state_from_settings(settings).expect("should build app state from settings")
     }
 
     fn empty_request(method: Method, uri: &str) -> edgezero_core::http::Request {
diff --git a/crates/trusted-server-adapter-fastly/src/main.rs b/crates/trusted-server-adapter-fastly/src/main.rs
@@ -1,5 +1,5 @@
 use std::io::Write;
-use std::net::IpAddr;
+
 use std::sync::Arc;
 
 use edgezero_adapter_fastly::FastlyConfigStore;
@@ -215,20 +215,21 @@ fn edgezero_main(mut req: FastlyRequest, config_store: ConfigStoreHandle) {
             }
         };
 
-    if !response_was_finalized_by_middleware(&mut response) {
+    if !take_finalize_sentinel(&mut response) {
         // Apply finalize headers at the entry point so that router-level
         // 405/404 responses for unregistered HTTP methods (e.g. TRACE, WebDAV
         // verbs) carry TS/geo headers. Middleware-finalized responses are
         // skipped here to avoid a second settings read and geo lookup on the
         // normal registered-route path.
         match get_settings() {
             Ok(settings) => {
-                apply_entry_point_finalize(&settings, client_ip, &mut response, |client_ip| {
+                let geo_info = resolve_geo_for_response(&response, client_ip, |client_ip| {
                     FastlyPlatformGeo.lookup(client_ip).unwrap_or_else(|e| {
                         log::warn!("entry-point geo lookup failed: {e}");
                         None
                     })
-                })
+                });
+                apply_finalize_headers(&settings, geo_info.as_ref(), &mut response);
             }
             Err(e) => {
                 log::warn!("entry-point finalize skipped: failed to reload settings: {e:?}");
@@ -239,29 +240,17 @@ fn edgezero_main(mut req: FastlyRequest, config_store: ConfigStoreHandle) {
     compat::to_fastly_response(response).send_to_client();
 }
 
-fn response_was_finalized_by_middleware(response: &mut HttpResponse) -> bool {
+fn take_finalize_sentinel(response: &mut HttpResponse) -> bool {
     response
         .headers_mut()
         .remove(HEADER_X_TS_FINALIZED)
         .is_some()
 }
 
-fn apply_entry_point_finalize<F>(
-    settings: &Settings,
-    client_ip: Option<IpAddr>,
-    response: &mut HttpResponse,
-    lookup_geo: F,
-) where
-    F: FnOnce(Option<IpAddr>) -> Option<GeoInfo>,
-{
-    let geo_info = resolve_geo_for_response(response, client_ip, lookup_geo);
-    apply_finalize_headers(settings, geo_info.as_ref(), response);
-}
-
 /// Handles a request using the original Fastly-native entry point.
 ///
 /// Preserves identical semantics to the pre-PR14 `main()`. Called whenever
-/// the EdgeZero flag is disabled or cannot be read/parsed as enabled — that
+/// the `EdgeZero` flag is disabled or cannot be read/parsed as enabled — that
 /// includes config-store open failures, key-read errors, missing keys, and
 /// any value other than the accepted `"true"` / `"1"` forms.
 ///
@@ -552,8 +541,7 @@ impl BoundedWriter {
 impl Write for BoundedWriter {
     fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
         if self.inner.len() + buf.len() > self.limit {
-            return Err(std::io::Error::new(
-                std::io::ErrorKind::Other,
+            return Err(std::io::Error::other(
                 "publisher body exceeded maximum buffered size",
             ));
         }
@@ -704,14 +692,14 @@ mod tests {
     }
 
     #[test]
-    fn response_was_finalized_by_middleware_strips_sentinel() {
+    fn take_finalize_sentinel_strips_sentinel() {
         let mut response = HttpResponse::new(EdgeBody::empty());
         response
             .headers_mut()
             .insert("x-ts-finalized", HeaderValue::from_static("1"));
 
         assert!(
-            response_was_finalized_by_middleware(&mut response),
+            take_finalize_sentinel(&mut response),
             "should detect middleware-finalized responses"
         );
         assert!(
@@ -729,9 +717,10 @@ mod tests {
             .body(EdgeBody::empty())
             .expect("should build response");
 
-        apply_entry_point_finalize(&settings, None, &mut response, |_| {
+        let geo_info = resolve_geo_for_response(&response, None, |_| {
             panic!("should skip entry-point geo lookup for 401 responses");
         });
+        apply_finalize_headers(&settings, geo_info.as_ref(), &mut response);
 
         assert_eq!(
             response
diff --git a/crates/trusted-server-core/src/http_util.rs b/crates/trusted-server-core/src/http_util.rs
@@ -169,7 +169,7 @@ fn normalize_scheme(value: &str) -> Option<String> {
 /// 1. `ClientInfo` TLS fields populated at the adapter entry point (most reliable)
 /// 2. Forwarded header (RFC 7239)
 /// 3. X-Forwarded-Proto header
-/// 4. Fastly-SSL header (trusted on EdgeZero path; can be spoofed on legacy path)
+/// 4. Fastly-SSL header (trusted on `EdgeZero` path; can be spoofed on legacy path)
 /// 5. Default to HTTP
 fn detect_request_scheme(
     req: &Request<EdgeBody>,
@@ -210,7 +210,7 @@ fn detect_request_scheme(
         }
     }
 
-    // 4. Check Fastly-SSL header. On the EdgeZero path this is injected from
+    // 4. Check Fastly-SSL header. On the `EdgeZero` path this is injected from
     //    authoritative Fastly TLS metadata after spoofable headers are stripped,
     //    so it is reliable. On direct or legacy paths it can be spoofed by clients.
     if let Some(ssl) = req.headers().get("fastly-ssl") {
diff --git a/crates/trusted-server-core/src/settings.rs b/crates/trusted-server-core/src/settings.rs
@@ -28,14 +28,18 @@ pub struct Publisher {
     /// Keep this secret stable to allow existing links to decode.
     #[validate(custom(function = validate_redacted_not_empty))]
     pub proxy_secret: Redacted<String>,
-    /// Maximum number of bytes buffered when the EdgeZero publisher fallback processes
-    /// a streaming response. When `None` (the default), buffering is unbounded and limited
-    /// only by the Wasm heap. Set to a byte limit to cap per-request allocation and return
-    /// a 500 when the processed body exceeds the cap.
-    #[serde(default)]
+    /// Maximum number of bytes buffered when the `EdgeZero` publisher fallback processes
+    /// a streaming response. Defaults to 16 MiB — a conservative cap that prevents
+    /// Wasm-heap OOM at flag-flip. Set explicitly to a larger value or `null` (unbounded)
+    /// when the deployment serves publisher pages larger than 16 MiB.
+    #[serde(default = "default_max_buffered_body_bytes")]
     pub max_buffered_body_bytes: Option<usize>,
 }
 
+fn default_max_buffered_body_bytes() -> Option<usize> {
+    Some(16 * 1024 * 1024)
+}
+
 impl Publisher {
     /// Known placeholder values that must not be used in production.
     pub const PROXY_SECRET_PLACEHOLDERS: &[&str] = &["change-me-proxy-secret", "proxy-secret"];
