refactor: encapsulate CORS preflight and response injection logic into reusable functions; improve test coverage

Author: das-Abroxas

api/src/s3/cors.rs

@@ -196,10 +196,6 @@ fn matched_allowed_headers(rule: &BucketCorsRule, requested_headers: &[String])
         return rule.allowed_headers.clone();
     }
 
-    if rule.allowed_headers.iter().any(|header| header == "*") {
-        return requested_headers.to_vec();
-    }
-
     requested_headers.to_vec()
 }
 
@@ -438,4 +434,73 @@ mod tests {
 
         assert!(matched.is_none());
     }
+
+    fn matched_rule() -> MatchedCorsRule {
+        MatchedCorsRule {
+            allow_origin: "https://example.org".to_string(),
+            allow_methods: vec!["GET".to_string(), "PUT".to_string()],
+            allow_headers: vec!["content-type".to_string(), "x-test".to_string()],
+            expose_headers: vec!["etag".to_string()],
+            max_age_seconds: Some(60),
+        }
+    }
+
+    #[test]
+    fn builds_preflight_success_response_with_expected_headers() {
+        let response = build_preflight_response(matched_rule());
+
+        assert_eq!(response.status(), StatusCode::NO_CONTENT);
+        assert_eq!(
+            response.headers()["access-control-allow-origin"],
+            "https://example.org"
+        );
+        assert_eq!(
+            response.headers()["access-control-allow-methods"],
+            "GET, PUT"
+        );
+        assert_eq!(
+            response.headers()["access-control-allow-headers"],
+            "content-type, x-test"
+        );
+        assert_eq!(response.headers()["access-control-max-age"], "60");
+        assert_eq!(
+            response.headers()[VARY],
+            "Origin, Access-Control-Request-Method, Access-Control-Request-Headers"
+        );
+    }
+
+    #[test]
+    fn builds_preflight_failure_response_without_allow_headers() {
+        let response = build_preflight_forbidden_response();
+
+        assert_eq!(response.status(), StatusCode::FORBIDDEN);
+        assert!(
+            response
+                .headers()
+                .get("access-control-allow-origin")
+                .is_none()
+        );
+        assert_eq!(
+            response.headers()[VARY],
+            "Origin, Access-Control-Request-Method, Access-Control-Request-Headers"
+        );
+    }
+
+    #[test]
+    fn injects_actual_cors_headers_and_preserves_existing_vary_entries() {
+        let mut response = HttpResponse::new(s3s::Body::default());
+        *response.status_mut() = StatusCode::OK;
+        response
+            .headers_mut()
+            .insert(VARY, HeaderValue::from_static("Accept-Encoding"));
+
+        inject_actual_cors_headers(&mut response, matched_rule());
+
+        assert_eq!(
+            response.headers()["access-control-allow-origin"],
+            "https://example.org"
+        );
+        assert_eq!(response.headers()["access-control-expose-headers"], "etag");
+        assert_eq!(response.headers()[VARY], "Accept-Encoding, Origin");
+    }
 }

api/src/s3/s3_server.rs

@@ -187,28 +187,24 @@ impl Service<Request<Incoming>> for WrappingService {
                 }
             };
 
-            if let (Some(origin), Some(requested_method), Some(config)) = (
-                origin.as_deref(),
-                preflight_requested_method.as_deref(),
-                cors_configuration.as_ref(),
-            )
-                && method == Method::OPTIONS
+            if let Some(config) = cors_configuration.as_ref()
+                && let Some(response) = handle_cors_preflight(
+                    config,
+                    &method,
+                    origin.as_deref(),
+                    preflight_requested_method.as_deref(),
+                    &requested_headers,
+                )
             {
-                let response = match match_preflight_rule(config, origin, requested_method, &requested_headers) {
-                    Some(matched_rule) => build_preflight_response(matched_rule),
-                    None => build_preflight_forbidden_response()
-                };
                 emit_request_completed(&span, "s3", response.status().as_u16(), started);
                 return Ok(response);
             }
 
             let result = shared.call(s3s_request).instrument(span.clone()).await;
             match result {
                 Ok(mut response) => {
-                    if let (Some(origin), Some(config)) = (origin.as_deref(), cors_configuration.as_ref()) {
-                        if let Some(matched_rule) = match_actual_rule(config, origin, &method) {
-                            inject_actual_cors_headers(&mut response, matched_rule);
-                        }
+                    if let Some(config) = cors_configuration.as_ref() {
+                        apply_cors_to_response(&mut response, config, origin.as_deref(), &method);
                     }
                     emit_request_completed(&span, "s3", response.status().as_u16(), started);
                     Ok(response)
@@ -250,6 +246,40 @@ fn virtual_hosted_bucket(host: &str, domain: &str) -> Option<String> {
     (!prefix.is_empty()).then(|| prefix.to_owned())
 }
 
+fn handle_cors_preflight(
+    config: &BucketCorsConfiguration,
+    method: &Method,
+    origin: Option<&str>,
+    requested_method: Option<&str>,
+    requested_headers: &[String],
+) -> Option<HttpResponse> {
+    if *method != Method::OPTIONS {
+        return None;
+    }
+    let origin = origin?;
+    let requested_method = requested_method?;
+
+    Some(
+        match match_preflight_rule(config, origin, requested_method, requested_headers) {
+            Some(matched_rule) => build_preflight_response(matched_rule),
+            None => build_preflight_forbidden_response(),
+        },
+    )
+}
+
+fn apply_cors_to_response(
+    response: &mut HttpResponse,
+    config: &BucketCorsConfiguration,
+    origin: Option<&str>,
+    method: &Method,
+) {
+    if let Some(origin) = origin
+        && let Some(matched_rule) = match_actual_rule(config, origin, method)
+    {
+        inject_actual_cors_headers(response, matched_rule);
+    }
+}
+
 async fn load_bucket_cors_config(
     driver_ctx: Arc<DriverContext>,
     bucket: Option<String>,
@@ -272,16 +302,6 @@ async fn load_bucket_cors_config(
 mod tests {
     use super::*;
 
-    fn matched_rule() -> super::super::cors::MatchedCorsRule {
-        super::super::cors::MatchedCorsRule {
-            allow_origin: "https://example.org".to_string(),
-            allow_methods: vec!["GET".to_string(), "PUT".to_string()],
-            allow_headers: vec!["content-type".to_string(), "x-test".to_string()],
-            expose_headers: vec!["etag".to_string()],
-            max_age_seconds: Some(60),
-        }
-    }
-
     #[test]
     fn extracts_bucket_name_from_path_style_request() {
         assert_eq!(