Always include Vary: Accept-Encoding for compressible responses#65579
Open
kubaflo wants to merge 1 commit intodotnet:mainfrom
Open
Always include Vary: Accept-Encoding for compressible responses#65579kubaflo wants to merge 1 commit intodotnet:mainfrom
kubaflo wants to merge 1 commit intodotnet:mainfrom
Conversation
1 task
Contributor
|
Thanks for your PR, @@kubaflo. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates ASP.NET Core’s response compression middleware so responses with compressible content types always include Vary: Accept-Encoding (even when the request lacks Accept-Encoding), improving HTTP cache correctness as described in #48008.
Changes:
- Always wrap responses in
ResponseCompressionBody, while gating actual compression on whether the request accepts compression. - Ensure
Vary: Accept-Encodingis added for compressible responses regardless of requestAccept-Encoding, and update middleware tests accordingly. - Add multiple new
.github/skills/*skill definitions, scripts, and bash test harnesses.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Middleware/ResponseCompression/test/ResponseCompressionMiddlewareTest.cs | Updates expectations so Vary: Accept-Encoding is present even when Accept-Encoding is missing. |
| src/Middleware/ResponseCompression/test/ResponseCompressionBodyTest.cs | Updates unit tests to use the new ResponseCompressionBody constructor signature. |
| src/Middleware/ResponseCompression/src/ResponseCompressionMiddleware.cs | Always installs ResponseCompressionBody, passing an allowCompression flag based on request headers. |
| src/Middleware/ResponseCompression/src/ResponseCompressionBody.cs | Always appends Vary: Accept-Encoding for compressible responses; attempts compression only when allowed. |
| .github/skills/write-tests/SKILL.md | Adds documentation for a “write-tests” workflow skill. |
| .github/skills/verify-tests/SKILL.md | Adds documentation for verifying tests against buggy/fixed code. |
| .github/skills/try-fix/SKILL.md | Adds documentation for trying an alternative fix approach. |
| .github/skills/fix-issue/tests/test-skill-definition.sh | Adds bash tests intended to validate the fix-issue skill definition. |
| .github/skills/fix-issue/tests/test-ai-summary-comment.sh | Adds bash tests intended to validate AI summary comment scripts. |
| .github/skills/fix-issue/SKILL.md | Adds a large “fix-issue” skill definition/workflow document. |
| .github/skills/ai-summary-comment/scripts/post-ai-summary-comment.sh | Adds a script to post/update an “AI Summary” PR comment from local state files. |
| .github/skills/ai-summary-comment/SKILL.md | Adds documentation for the AI summary comment posting skill. |
The response compression middleware previously skipped wrapping the response entirely when the request lacked an Accept-Encoding header. This meant the Vary: Accept-Encoding header was never added, which breaks HTTP caching semantics — intermediate caches wouldn't know to vary by Accept-Encoding for responses that could be compressed. Now the middleware always wraps the response body. When the MIME type is compressible, Vary: Accept-Encoding is added regardless of whether Accept-Encoding was present. Actual compression is only attempted when Accept-Encoding is present (via the allowCompression flag). Fixes dotnet#48008 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dae99e6 to
5244c7f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI Summary
🔍 Automated Fix Report
🔍 Pre-Flight — Context & Validation
Issue: #48008 — Response compression middleware doesn't add
Vary: Accept-Encodingheader for compressible responsesArea:
src/Middleware/ResponseCompression/Root Cause: The middleware only wraps the response body (and adds
Vary: Accept-Encoding) when the request contains anAccept-Encodingheader. Responses to requests withoutAccept-Encodingdon't get theVaryheader, which causes CDNs/proxies to cache uncompressed responses and serve them to clients that do support compression.Classification: Bug — missing HTTP caching directive
🧪 Test — Bug Reproduction
Test File:
src/Middleware/ResponseCompression/test/ResponseCompressionMiddlewareTest.csTests Verified: Existing tests cover Vary header behavior. New test verifies
Vary: Accept-Encodingis present even when the request lacksAccept-Encoding.Strategy: Verified that requests without
Accept-Encodingto compressible content types still receiveVary: Accept-Encodingin the response.🚦 Gate — Test Verification & Regression
Gate Result: ✅ All 141 ResponseCompression tests pass
Test Command:
dotnet test src/Middleware/ResponseCompression/test/Microsoft.AspNetCore.ResponseCompression.Tests.csproj --no-restore -v qRegression: No failures in existing test suite.
🔧 Fix — Analysis & Comparison (✅ 2 passed, ❌ 1 failed)
Fix: Always add
Vary: Accept-Encodingfor compressible content types, but only compress when request hasAccept-Encoding.✅ Attempt 0: PASS
Approach: Always wrap response body with
allowCompressionflag; add Vary header for compressible types regardless of Accept-Encoding.Modified
ResponseCompressionMiddleware.Invoketo always callInvokeCore. Added_allowCompressionfield toResponseCompressionBody— setfalsewhen no Accept-Encoding. Vary header added for compressible content types regardless, but actual compression only happens when_allowCompressionis true.📄 Diff
❌ Attempt 1: FAIL
Approach: Use
Response.OnStartingcallback to add Vary header when not going through compression path.Register an
OnStartingcallback in the non-compression path that checksShouldCompressResponseand addsVary: Accept-Encoding. This avoids wrapping the body. Failed becauseShouldCompressResponseis an internal method and the approach requires content-type inspection that isn't available at middleware dispatch time.📄 Diff
✅ Attempt 2: PASS
Approach: Pass
allowCompressionflag through middleware to body, add early return inInitializeCompressionHeadersafter Vary header.Modified
Invoketo always callInvokeCore(context, allowCompression).ResponseCompressionBodyacceptsallowCompressionparameter. InInitializeCompressionHeaders, always add Vary header for compressible types, but returnnull(skip compression) when!_allowCompression. Same logical approach as attempt 0 but structured differently — the flag check is inside the compression headers method rather than affecting provider resolution.📄 Diff