Skip to content

Conversation

@ByteBaker
Copy link
Contributor

@ByteBaker ByteBaker commented Oct 20, 2025

User description

The compression middleware was appending a separate Vary: accept-encoding header instead of combining it with existing CORS Vary headers. This caused Suricata alerts and caching inconsistencies.

Now all Vary values are collected and combined into a single header with case-insensitive deduplication.

Fixes #8847


PR Type

Bug fix, Tests


Description

  • Combine duplicate Vary headers into one

  • Deduplicate values case-insensitively

  • Preserve existing CORS Vary values

  • Add tests for header consolidation


Diagram Walkthrough

flowchart LR
  Encoder["encoding::update_head"]
  HeadersOld["Multiple Vary headers"]
  Collect["Collect + split + trim"]
  Dedup["Case-insensitive check for accept-encoding"]
  Combine["Join into single Vary header"]
  Tests["Unit tests for scenarios"]

  Encoder -- "reads existing Vary" --> HeadersOld
  HeadersOld -- "values extracted" --> Collect
  Collect -- "ensure accept-encoding present" --> Dedup
  Dedup -- "remove old, insert single" --> Combine
  Encoder -- "validated by" --> Tests
Loading

File Walkthrough

Relevant files
Bug fix
encoder.rs
Consolidate and deduplicate Vary header handling                 

src/handler/http/router/middlewares/encoding/encoder.rs

  • Replace appending Vary with consolidation logic
  • Collect, split, trim, and deduplicate Vary values
  • Ensure single combined Vary header is inserted
  • Add tests for multiple/CORS/duplicate cases
+78/-3   

@github-actions github-actions bot added the ☢️ Bug Something isn't working label Oct 20, 2025
@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Remove stale Content-Length on encode

When setting Content-Encoding, remove any existing Content-Length to prevent stale
lengths after compression changes. This avoids clients misinterpreting the body due
to mismatched length.

src/handler/http/router/middlewares/encoding/encoder.rs [282-283]

 head.headers_mut()
     .insert(header::CONTENT_ENCODING, encoding.to_header_value());
+// Content length may no longer be valid once encoding is applied
+head.headers_mut().remove(header::CONTENT_LENGTH);
Suggestion importance[1-10]: 8

__

Why: Removing Content-Length when setting Content-Encoding prevents mismatched length after compression, which can break clients; it's a high-impact correctness fix and aligns with typical HTTP handling.

Medium
Deduplicate and validate Vary header

Deduplicate all Vary tokens case-insensitively, not just accept-encoding, to avoid
repeated values when upstream set duplicates. Also skip inserting an invalid Vary
header when the combined value fails to parse, instead leaving existing headers
intact.

src/handler/http/router/middlewares/encoding/encoder.rs [286-309]

 let mut vary_values: Vec<String> = head
     .headers()
     .get_all(header::VARY)
     .filter_map(|v| v.to_str().ok())
     .flat_map(|s| s.split(','))
     .map(|v| v.trim())
     .filter(|v| !v.is_empty())
     .map(str::to_string)
     .collect();
 
-// Add accept-encoding if not already present
-if !vary_values
-    .iter()
-    .any(|v| v.eq_ignore_ascii_case("accept-encoding"))
-{
+// Ensure "accept-encoding" is present (case-insensitive)
+if !vary_values.iter().any(|v| v.eq_ignore_ascii_case("accept-encoding")) {
     vary_values.push("accept-encoding".to_string());
 }
 
-// Remove all existing Vary headers and insert a single combined one
-head.headers_mut().remove(header::VARY);
-let combined = vary_values.join(", ");
-if let Ok(header_value) = HeaderValue::from_str(&combined) {
+// Deduplicate tokens case-insensitively while preserving original casing of first occurrence
+let mut seen = std::collections::HashSet::new();
+let mut deduped: Vec<String> = Vec::new();
+for v in vary_values.into_iter() {
+    let key = v.to_ascii_lowercase();
+    if seen.insert(key) {
+        deduped.push(v);
+    }
+}
+
+// Build combined header and only replace if valid
+let combined = deduped.join(", ");
+if HeaderValue::from_str(&combined).is_ok() {
+    head.headers_mut().remove(header::VARY);
+    // safe to unwrap because checked above
+    let header_value = HeaderValue::from_str(&combined).unwrap();
     head.headers_mut().insert(header::VARY, header_value);
 }
Suggestion importance[1-10]: 7

__

Why: Correctly identifies potential duplicate Vary tokens and proposes case-insensitive deduplication plus safer replacement only when valid; this improves robustness and standards compliance, though it's an enhancement rather than fixing a critical bug.

Medium
General
Normalize Vary header whitespace

Normalize spacing in the combined Vary header by joining with a single comma and
space and trimming each token to meet RFC formatting, ensuring consistent downstream
parsing. This prevents accidental duplicates caused by differing whitespace.

src/handler/http/router/middlewares/encoding/encoder.rs [307-309]

+let combined = vary_values
+    .into_iter()
+    .map(|s| s.trim().to_string())
+    .filter(|s| !s.is_empty())
+    .collect::<Vec<_>>()
+    .join(", ");
 if let Ok(header_value) = HeaderValue::from_str(&combined) {
     head.headers_mut().insert(header::VARY, header_value);
 }
Suggestion importance[1-10]: 5

__

Why: Normalizing whitespace is already largely handled by trimming and joining; this is a minor readability/consistency improvement with limited impact on functionality.

Low

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Summary

This PR fixes the duplicate Vary header issue by modifying the update_head function in the compression middleware. Previously, the middleware simply appended accept-encoding to the Vary header, resulting in multiple Vary headers when CORS middleware had already set one.

Key changes:

  • Collects all existing Vary header values and parses comma-separated values
  • Performs case-insensitive deduplication to avoid duplicate accept-encoding entries
  • Removes all existing Vary headers and inserts a single combined header
  • Adds three comprehensive test cases covering CORS headers, deduplication, and combining multiple values

Impact:

  • Resolves Suricata alerts triggered by duplicate headers
  • Ensures consistent caching behavior across reverse proxies
  • Maintains RFC 9110 compliance while following best practices

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The implementation is sound with proper parsing, case-insensitive deduplication, and error handling. Three new comprehensive tests validate the fix for CORS headers, deduplication, and combining multiple values. The change is localized to the update_head function with no breaking changes to the API.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
src/handler/http/router/middlewares/encoding/encoder.rs 5/5 Fixes duplicate Vary headers by combining all values into single header with case-insensitive deduplication; includes comprehensive tests

Sequence Diagram

sequenceDiagram
    participant Client
    participant CompressMiddleware
    participant CORSMiddleware
    participant Encoder
    participant ResponseHead

    Client->>CompressMiddleware: HTTP Request with Accept-Encoding
    CompressMiddleware->>CORSMiddleware: Forward Request
    CORSMiddleware->>ResponseHead: Set Vary: Origin, Access-Control-Request-Method, Access-Control-Request-Headers
    CORSMiddleware-->>CompressMiddleware: Response with CORS headers
    CompressMiddleware->>Encoder: Encode response body
    Encoder->>Encoder: update_head(encoding, head)
    Note over Encoder: Collect existing Vary headers
    Encoder->>ResponseHead: Get all Vary header values
    ResponseHead-->>Encoder: ["Origin, Access-Control-Request-Method, Access-Control-Request-Headers"]
    Note over Encoder: Split by comma, trim, deduplicate
    Encoder->>Encoder: Check if "accept-encoding" exists (case-insensitive)
    Encoder->>Encoder: Add "accept-encoding" to list
    Encoder->>ResponseHead: Remove all Vary headers
    Encoder->>ResponseHead: Insert combined Vary: Origin, Access-Control-Request-Method, Access-Control-Request-Headers, accept-encoding
    Encoder-->>CompressMiddleware: Encoded response with single Vary header
    CompressMiddleware-->>Client: Compressed response
Loading

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@testdino-playwright-reporter
Copy link

⚠️ Test Run Unstable


Author: ByteBaker | Branch: fix/double-vary-header | Commit: 477f446

Testdino Test Results

Status Total Passed Failed Skipped Flaky Pass Rate Duration
All tests passed 364 340 0 19 5 93% 4m 40s

View Detailed Results

The compression middleware was appending a separate Vary: accept-encoding
header instead of combining it with existing CORS Vary headers. This
caused Suricata alerts and caching inconsistencies.

Now all Vary values are collected and combined into a single header
with case-insensitive deduplication.

Fixes #8847
@ByteBaker ByteBaker force-pushed the fix/double-vary-header branch from 477f446 to 81da0b1 Compare October 21, 2025 06:58
// Remove all existing Vary headers and insert a single combined one
head.headers_mut().remove(header::VARY);
let combined = vary_values.join(", ");
if let Ok(header_value) = HeaderValue::from_str(&combined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a log here in case from_str fails?

@hengfeiyang hengfeiyang marked this pull request as draft November 3, 2025 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

☢️ Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HTTP header VARY doubled

3 participants