Address S3 and Image Optimizer review findings

Author: ChristianPavilonis

crates/trusted-server-adapter-fastly/src/main.rs

@@ -16,7 +16,7 @@ use trusted_server_core::integrations::IntegrationRegistry;
 use trusted_server_core::platform::RuntimeServices;
 use trusted_server_core::proxy::{
     handle_asset_proxy_request, handle_first_party_click, handle_first_party_proxy,
-    handle_first_party_proxy_rebuild, handle_first_party_proxy_sign,
+    handle_first_party_proxy_rebuild, handle_first_party_proxy_sign, AssetProxyCachePolicy,
 };
 use trusted_server_core::publisher::{
     handle_publisher_request, handle_tsjs_dynamic, stream_publisher_body, PublisherResponse,
@@ -207,7 +207,7 @@ async fn route_request(
     let path = req.get_path().to_string();
     let method = req.get_method().clone();
 
-    let mut preserve_no_store_private = false;
+    let mut asset_cache_policy = AssetProxyCachePolicy::OriginControlled;
 
     // Match known routes and handle them
     let result = match (method, path.as_str()) {
@@ -280,10 +280,9 @@ async fn route_request(
                 );
                 match handle_asset_proxy_request(settings, runtime_services, req, asset_route).await
                 {
-                    Ok(response) => {
-                        preserve_no_store_private = response.get_header_str(header::CACHE_CONTROL)
-                            == Some("no-store, private");
-                        Ok(response)
+                    Ok(asset_response) => {
+                        asset_cache_policy = asset_response.cache_policy();
+                        Ok(asset_response.into_response())
                     }
                     Err(e) => Err(e),
                 }
@@ -351,9 +350,7 @@ async fn route_request(
     let mut response = result.unwrap_or_else(|e| to_error_response(&e));
 
     finalize_response(settings, geo_info.as_ref(), &mut response);
-    if preserve_no_store_private {
-        response.set_header(header::CACHE_CONTROL, "no-store, private");
-    }
+    asset_cache_policy.apply_after_route_finalization(&mut response);
 
     Some(response)
 }

crates/trusted-server-core/src/asset_image_optimizer.rs

@@ -75,7 +75,14 @@ pub(crate) fn options_for_asset_request(
     let profile_params = profile_set
         .profiles
         .get(selected_profile)
-        .expect("should select only configured image optimizer profiles");
+        .ok_or_else(|| {
+            Report::new(TrustedServerError::Configuration {
+                message: format!(
+                    "image_optimizer.profile_sets `{}` selected profile `{selected_profile}` is not defined",
+                    route_config.profile_set
+                ),
+            })
+        })?;
     params.merge_from(parse_param_string(profile_params)?);
 
     apply_aspect_ratio_override(profile_set, selected_profile, query, &mut params);
@@ -362,7 +369,8 @@ fn parse_aspect_ratio_value(value: &str) -> Option<(u32, u32)> {
 mod tests {
     use super::*;
     use crate::settings::{
-        ImageOptimizerAspectRatioConfig, ImageOptimizerCropOffsetsConfig, ImageOptimizerProfileSet,
+        AssetImageOptimizerConfig, ImageOptimizerAspectRatioConfig,
+        ImageOptimizerCropOffsetsConfig, ImageOptimizerProfileSet, ImageOptimizerSettings,
     };
 
     fn profile_set() -> ImageOptimizerProfileSet {
@@ -392,6 +400,34 @@ mod tests {
         }
     }
 
+    #[test]
+    fn options_for_asset_request_returns_error_when_selected_profile_is_missing() {
+        let mut settings = crate::test_support::tests::create_test_settings();
+        let mut profile_set = profile_set();
+        profile_set.profiles.remove("default");
+        settings.image_optimizer = ImageOptimizerSettings {
+            profile_sets: std::collections::HashMap::from([(
+                "default_images".to_string(),
+                profile_set,
+            )]),
+        };
+        let mut route = ProxyAssetRoute::new("/.images/", "https://assets.example.com");
+        route.image_optimizer = Some(AssetImageOptimizerConfig {
+            enabled: true,
+            region: "us_east".to_string(),
+            profile_set: "default_images".to_string(),
+            origin_query: None,
+        });
+
+        let err = options_for_asset_request(&settings, &route, "")
+            .expect_err("should return a configuration error for broken profile sets");
+
+        assert!(
+            format!("{err:?}").contains("selected profile `default` is not defined"),
+            "should describe the missing selected profile without panicking: {err:?}"
+        );
+    }
+
     #[test]
     fn profile_conversion_adds_aspect_ratio_and_smart_crop() {
         let set = profile_set();

crates/trusted-server-core/src/proxy.rs

@@ -22,6 +22,7 @@ use crate::error::TrustedServerError;
 use crate::platform::{
     PlatformBackendSpec, PlatformHttpRequest, PlatformResponse, RuntimeServices, StoreName,
 };
+use crate::redacted::Redacted;
 use crate::s3_sigv4::{self, S3Credentials};
 use crate::settings::{
     AssetOriginAuth, OriginQueryPolicy, ProxyAssetRoute, S3SigV4AuthConfig, Settings,
@@ -69,6 +70,66 @@ const ASSET_PROXY_FORWARD_HEADERS: [header::HeaderName; 11] = [
 const ASSET_PROXY_STRIP_RESPONSE_HEADERS: [&str; 3] =
     ["set-cookie", "strict-transport-security", "clear-site-data"];
 
+/// Cache-control value used when asset proxy responses must not be stored.
+pub const ASSET_NO_STORE_PRIVATE_CACHE_CONTROL: &str = "no-store, private";
+
+/// Cache policy metadata emitted by the asset proxy handler.
+///
+/// The Fastly router finalizes standard response headers after the asset handler
+/// returns. This typed policy lets the router reapply protected cache directives
+/// without depending on an exact header string set by the handler.
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub enum AssetProxyCachePolicy {
+    /// Leave origin/cache-control headers under normal response finalization.
+    OriginControlled,
+    /// Reapply `Cache-Control: no-store, private` after standard finalization.
+    NoStorePrivate,
+}
+
+impl AssetProxyCachePolicy {
+    /// Apply protected cache headers after route-level response finalization.
+    pub fn apply_after_route_finalization(self, response: &mut Response) {
+        if self == Self::NoStorePrivate {
+            apply_no_store_cache_control(response);
+        }
+    }
+}
+
+/// Asset proxy response plus metadata needed by the outer router.
+pub struct AssetProxyResponse {
+    response: Response,
+    cache_policy: AssetProxyCachePolicy,
+}
+
+impl AssetProxyResponse {
+    fn new(response: Response, cache_policy: AssetProxyCachePolicy) -> Self {
+        Self {
+            response,
+            cache_policy,
+        }
+    }
+
+    fn origin_controlled(response: Response) -> Self {
+        Self::new(response, AssetProxyCachePolicy::OriginControlled)
+    }
+
+    fn no_store_private(response: Response) -> Self {
+        Self::new(response, AssetProxyCachePolicy::NoStorePrivate)
+    }
+
+    /// Return cache policy metadata for router finalization.
+    #[must_use]
+    pub fn cache_policy(&self) -> AssetProxyCachePolicy {
+        self.cache_policy
+    }
+
+    /// Consume this wrapper and return the Fastly response.
+    #[must_use]
+    pub fn into_response(self) -> Response {
+        self.response
+    }
+}
+
 #[derive(Clone, Debug, Eq, Hash, PartialEq)]
 struct S3CredentialsCacheKey {
     secret_store: String,
@@ -684,8 +745,8 @@ fn load_s3_credentials(
         .transpose()?;
     let credentials = Arc::new(S3Credentials {
         access_key_id,
-        secret_access_key,
-        session_token,
+        secret_access_key: Redacted::new(secret_access_key),
+        session_token: session_token.map(Redacted::new),
     });
 
     let mut cache = S3_CREDENTIALS_CACHE
@@ -792,7 +853,7 @@ fn strip_asset_proxy_response_headers(response: &mut Response) {
 fn apply_no_store_cache_control(response: &mut Response) {
     response.set_header(
         header::CACHE_CONTROL,
-        HeaderValue::from_static("no-store, private"),
+        HeaderValue::from_static(ASSET_NO_STORE_PRIVATE_CACHE_CONTROL),
     );
 }
 
@@ -813,7 +874,7 @@ async fn preflight_s3_origin_for_image_optimizer(
     request_method: &Method,
     unsigned_headers: &http::HeaderMap,
     backend_name: &str,
-) -> Result<Option<Response>, Report<TrustedServerError>> {
+) -> Result<Option<AssetProxyResponse>, Report<TrustedServerError>> {
     let Some(auth @ AssetOriginAuth::S3SigV4(_)) = route.auth.as_ref() else {
         return Ok(None);
     };
@@ -841,7 +902,7 @@ async fn preflight_s3_origin_for_image_optimizer(
         let mut response = head_response;
         strip_asset_proxy_response_headers(&mut response);
         apply_no_store_cache_control(&mut response);
-        return Ok(Some(response));
+        return Ok(Some(AssetProxyResponse::no_store_private(response)));
     }
 
     let mut get_headers = unsigned_headers.clone();
@@ -858,7 +919,7 @@ async fn preflight_s3_origin_for_image_optimizer(
     strip_asset_proxy_response_headers(&mut response);
     apply_no_store_cache_control(&mut response);
 
-    Ok(Some(response))
+    Ok(Some(AssetProxyResponse::no_store_private(response)))
 }
 
 /// Proxy a configured first-party asset path to its matched asset origin.
@@ -882,7 +943,7 @@ pub async fn handle_asset_proxy_request(
     services: &RuntimeServices,
     req: Request,
     route: &ProxyAssetRoute,
-) -> Result<Response, Report<TrustedServerError>> {
+) -> Result<AssetProxyResponse, Report<TrustedServerError>> {
     let incoming_query = req.get_query_str().unwrap_or("");
     let mut target_url = build_asset_proxy_target_url(route, req.get_path(), incoming_query)?;
     let image_optimizer =
@@ -967,7 +1028,7 @@ pub async fn handle_asset_proxy_request(
     let mut response = platform_response_to_fastly_asset(platform_resp).await?;
     strip_asset_proxy_response_headers(&mut response);
 
-    Ok(response)
+    Ok(AssetProxyResponse::origin_controlled(response))
 }
 
 /// Upserts the `ts-ec` query parameter on a URL, replacing any existing value.
@@ -1731,8 +1792,8 @@ mod tests {
         clear_s3_credentials_cache_for_tests, handle_asset_proxy_request, handle_first_party_click,
         handle_first_party_proxy, handle_first_party_proxy_rebuild, handle_first_party_proxy_sign,
         is_host_allowed, proxy_request, rebuild_response_with_body,
-        reconstruct_and_validate_signed_target, redirect_is_permitted, ProxyRequestConfig,
-        SUPPORTED_ENCODINGS,
+        reconstruct_and_validate_signed_target, redirect_is_permitted, AssetProxyCachePolicy,
+        ProxyRequestConfig, SUPPORTED_ENCODINGS,
     };
     use crate::constants::{HEADER_ACCEPT, HEADER_X_FORWARDED_FOR};
     use crate::creative;
@@ -2761,7 +2822,8 @@ mod tests {
         let route = ProxyAssetRoute::new("/.images/", "https://assets.example.com:8443");
         let response = handle_asset_proxy_request(&settings, &services, req, &route)
             .await
-            .expect("should proxy asset request");
+            .expect("should proxy asset request")
+            .into_response();
         assert_eq!(response.get_status(), StatusCode::OK);
 
         let all_headers = stub.recorded_request_headers();
@@ -2852,7 +2914,8 @@ mod tests {
         let route = ProxyAssetRoute::new("/.images/", "https://assets.example.com");
         let response = handle_asset_proxy_request(&settings, &services, req, &route)
             .await
-            .expect("should proxy asset request");
+            .expect("should proxy asset request")
+            .into_response();
 
         assert!(
             response.get_header(header::SET_COOKIE).is_none(),
@@ -3152,7 +3215,8 @@ mod tests {
 
         let mut response = handle_asset_proxy_request(&settings, &services, req, &route)
             .await
-            .expect("should proxy optimized S3 asset request");
+            .expect("should proxy optimized S3 asset request")
+            .into_response();
 
         assert_eq!(response.get_status(), StatusCode::OK);
         assert_eq!(response.take_body_str(), "optimized");
@@ -3213,9 +3277,15 @@ mod tests {
         );
         let route = test_s3_image_optimizer_route();
 
-        let mut response = handle_asset_proxy_request(&settings, &services, req, &route)
+        let asset_response = handle_asset_proxy_request(&settings, &services, req, &route)
             .await
             .expect("should return raw S3 error response");
+        assert_eq!(
+            asset_response.cache_policy(),
+            AssetProxyCachePolicy::NoStorePrivate,
+            "should carry a typed no-store policy for router finalization"
+        );
+        let mut response = asset_response.into_response();
 
         assert_eq!(response.get_status(), StatusCode::NOT_FOUND);
         assert_eq!(
@@ -3271,7 +3341,8 @@ mod tests {
 
         let mut response = handle_asset_proxy_request(&settings, &services, req, &route)
             .await
-            .expect("should proxy debug S3 asset request");
+            .expect("should proxy debug S3 asset request")
+            .into_response();
 
         assert_eq!(response.take_body_str(), "raw");
         assert_eq!(
@@ -3336,7 +3407,8 @@ mod tests {
 
         let mut response = handle_asset_proxy_request(&settings, &services, req, &route)
             .await
-            .expect("should proxy a streaming asset response");
+            .expect("should proxy a streaming asset response")
+            .into_response();
 
         assert_eq!(response.take_body_str(), "chunk");
     }