diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index 9e4fa2b4..8c24c46a 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -4,6 +4,7 @@ use super::ProbeRegistration; use super::api_description::ApiDescription; use super::body::Body; +use super::compression::NoCompression; use super::compression::add_vary_header; use super::compression::apply_gzip_compression; use super::compression::is_compressible_content_type; @@ -993,15 +994,6 @@ async fn http_request_handle( if matches!(server.config.compression, CompressionConfig::Gzip) && 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 (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, @@ -1009,7 +1001,18 @@ async fn http_request_handle( response.headers(), response.extensions(), ) { + // Add Vary: Accept-Encoding and compress. The Vary header is added + // here so caches know they must not serve a compressed response to + // a client that didn't accept it. + add_vary_header(response.headers_mut()); response = apply_gzip_compression(response); + } else if response.extensions().get::().is_none() { + // Add Vary: Accept-Encoding for compressible responses that are + // skipped this time (e.g. body too small, wrong method, partial + // content) but that could be compressed for a different request. + // Omit Vary when NoCompression is set because that response will + // never be compressed regardless of Accept-Encoding. + add_vary_header(response.headers_mut()); } } diff --git a/dropshot/tests/integration-tests/gzip.rs b/dropshot/tests/integration-tests/gzip.rs index cecf7c4e..9f95c6da 100644 --- a/dropshot/tests/integration-tests/gzip.rs +++ b/dropshot/tests/integration-tests/gzip.rs @@ -524,6 +524,15 @@ async fn test_compression_disabled_with_extension() { assert_eq!(response.headers().get(header::CONTENT_ENCODING), None); + // A NoCompression response will never be compressed regardless of + // Accept-Encoding, so it must not carry Vary: Accept-Encoding (which would + // cause caches to split entries on a header the body doesn't depend on). + assert_eq!( + response.headers().get(header::VARY), + None, + "NoCompression response should omit Vary: Accept-Encoding" + ); + get_response_bytes(&mut response).await; testctx.teardown().await; @@ -540,6 +549,20 @@ async fn test_no_compression_below_size_threshold() { assert_eq!(response.headers().get(header::CONTENT_ENCODING), None); + // Unlike the NoCompression case, a compressible response skipped only + // because it's below the size threshold must still carry Vary: a different + // request to the same resource could be compressed. + let vary = response + .headers() + .get(header::VARY) + .expect("compressible-but-skipped response should keep Vary") + .to_str() + .unwrap(); + assert!( + vary.to_lowercase().contains("accept-encoding"), + "Vary should include Accept-Encoding, got: {vary}" + ); + testctx.teardown().await; }