always add Vary: Accept-Encoding for compressible content-types

david-crespo · david-crespo · commit 76e00b24234d · 2025-11-18T17:14:58.000-06:00
diff --git a/dropshot/src/compression.rs b/dropshot/src/compression.rs
@@ -97,6 +97,41 @@ pub fn accepts_gzip_encoding(headers: &HeaderMap<HeaderValue>) -> bool {
     false
 }
 
+/// Checks if a content type is compressible.
+/// This is used to determine if the Vary: Accept-Encoding header should be added,
+/// even if compression doesn't occur for this particular request.
+pub fn is_compressible_content_type(
+    response_headers: &HeaderMap<HeaderValue>,
+) -> bool {
+    // Only compress when we know the content type
+    let Some(content_type) = response_headers.get(http::header::CONTENT_TYPE)
+    else {
+        return false;
+    };
+    let Ok(ct_str) = content_type.to_str() else {
+        return false;
+    };
+
+    let ct_lower = ct_str.to_ascii_lowercase();
+
+    // SSE streams prioritize latency over compression
+    if ct_lower.starts_with("text/event-stream") {
+        return false;
+    }
+
+    let is_compressible = ct_lower.starts_with("application/json")
+        || ct_lower.starts_with("text/")
+        || ct_lower.starts_with("application/xml")
+        || ct_lower.starts_with("application/javascript")
+        || ct_lower.starts_with("application/x-javascript");
+
+    // RFC 6839 structured syntax suffixes (+json, +xml)
+    let has_compressible_suffix =
+        ct_lower.contains("+json") || ct_lower.contains("+xml");
+
+    is_compressible || has_compressible_suffix
+}
+
 /// Determines if a response should be compressed with gzip.
 pub fn should_compress_response(
     request_method: &http::Method,
@@ -151,33 +186,9 @@ pub fn should_compress_response(
         }
     }
 
-    // Only compress when we know the content type
-    let Some(content_type) = response_headers.get(http::header::CONTENT_TYPE)
-    else {
-        return false;
-    };
-    let Ok(ct_str) = content_type.to_str() else {
-        return false;
-    };
-
-    let ct_lower = ct_str.to_ascii_lowercase();
-
-    // SSE streams prioritize latency over compression
-    if ct_lower.starts_with("text/event-stream") {
-        return false;
-    }
-
-    let is_compressible = ct_lower.starts_with("application/json")
-        || ct_lower.starts_with("text/")
-        || ct_lower.starts_with("application/xml")
-        || ct_lower.starts_with("application/javascript")
-        || ct_lower.starts_with("application/x-javascript");
-
-    // RFC 6839 structured syntax suffixes (+json, +xml)
-    let has_compressible_suffix =
-        ct_lower.contains("+json") || ct_lower.contains("+xml");
-
-    is_compressible || has_compressible_suffix
+    // technically redundant with check outside of the call, but kept here
+    // because it's logically part of "should compress?" question
+    is_compressible_content_type(response_headers)
 }
 
 /// Minimum size in bytes for a response to be compressed.
@@ -222,18 +233,7 @@ pub fn apply_gzip_compression(response: Response<Body>) -> Response<Body> {
 
     // Vary header is critical for caching - prevents serving compressed
     // responses to clients that don't accept gzip
-    let vary_has_accept_encoding = parts
-        .headers
-        .get_all(http::header::VARY)
-        .iter()
-        .any(header_value_contains_accept_encoding);
-
-    if !vary_has_accept_encoding {
-        parts.headers.append(
-            http::header::VARY,
-            HeaderValue::from_static("Accept-Encoding"),
-        );
-    }
+    add_vary_header(&mut parts.headers);
 
     // because we're streaming, we can't handle ranges and we don't know the
     // length of the response ahead of time
@@ -250,6 +250,25 @@ fn header_value_contains_accept_encoding(value: &HeaderValue) -> bool {
     })
 }
 
+/// Adds the Vary: Accept-Encoding header to a response if not already present.
+/// This is critical for correct caching behavior with intermediate caches.
+pub fn add_vary_header(headers: &mut HeaderMap<HeaderValue>) {
+    let vary_values = headers.get_all(http::header::VARY);
+
+    // can't and shouldn't add anything if we already have "*"
+    // https://datatracker.ietf.org/doc/html/rfc7231#section-7.1.4
+    if vary_values.iter().any(|v| v == "*") {
+        return;
+    }
+
+    if !vary_values.iter().any(header_value_contains_accept_encoding) {
+        headers.append(
+            http::header::VARY,
+            HeaderValue::from_static("Accept-Encoding"),
+        );
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs
@@ -3,7 +3,9 @@
 
 use super::api_description::ApiDescription;
 use super::body::Body;
+use super::compression::add_vary_header;
 use super::compression::apply_gzip_compression;
+use super::compression::is_compressible_content_type;
 use super::compression::should_compress_response;
 use super::config::{ConfigDropshot, ConfigTls};
 #[cfg(feature = "usdt-probes")]
@@ -990,15 +992,29 @@ async fn http_request_handle<C: ServerContext>(
     };
 
     if server.config.compression
-        && should_compress_response(
+        && is_compressible_content_type(response.headers())
+    {
+        // Add Vary: Accept-Encoding header for all compressible content
+        // types. This needs to be there even if the response ends up not being
+        // compressed because it tells caches how to handle the possibility that
+        // responses could be compressed or not.
+        //
+        // This tells caches (like browsers and CDNs) that the response content
+        // depends on the value of the Accept-Encoding header. Without this, a
+        // cache might mistakenly serve a compressed response to a client that
+        // cannot decompress it, or serve an uncompressed response to a client
+        // that could have benefited from compression.
+        add_vary_header(response.headers_mut());
+
+        if should_compress_response(
             &method,
             &request_headers,
             response.status(),
             response.headers(),
             response.extensions(),
-        )
-    {
-        response = apply_gzip_compression(response);
+        ) {
+            response = apply_gzip_compression(response);
+        }
     }
 
     response.headers_mut().insert(
diff --git a/dropshot/tests/integration-tests/gzip.rs b/dropshot/tests/integration-tests/gzip.rs
@@ -690,3 +690,43 @@ async fn test_compression_config_enabled() {
 
     testctx.teardown().await;
 }
+
+#[tokio::test]
+async fn test_vary_header_on_non_gzip_requests() {
+    // Test that Vary: Accept-Encoding is present even when client doesn't
+    // accept gzip. This is critical for correct caching behavior.
+    //
+    // Without this header, the following sequence causes incorrect behavior:
+    // 1. Client A (no gzip) requests resource → cache stores uncompressed
+    //    response without Vary header
+    // 2. Client B (gzip support) requests same resource → cache serves the
+    //    uncompressed version, even though Client B could handle compression
+    let testctx = test_setup("vary_header_no_gzip", api());
+    let client = &testctx.client_testctx;
+
+    // Request WITHOUT Accept-Encoding: gzip for a compressible resource
+    let uri = client.url("/large-response");
+    let response = make_plain_request_response(client, &uri).await;
+
+    // Should NOT be compressed
+    assert!(
+        response.headers().get(header::CONTENT_ENCODING).is_none(),
+        "Response should not be compressed"
+    );
+
+    // Should still have Vary: Accept-Encoding header
+    assert!(
+        response.headers().contains_key(header::VARY),
+        "Response should have Vary header even without gzip encoding"
+    );
+
+    let vary_value =
+        response.headers().get(header::VARY).unwrap().to_str().unwrap();
+    assert!(
+        vary_value.to_lowercase().contains("accept-encoding"),
+        "Vary header should include Accept-Encoding, got: {}",
+        vary_value
+    );
+
+    testctx.teardown().await;
+}
