Skip to content

Conversation

@paulcrussell
Copy link

πŸ”— Linked issue

#3077

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

The build process produces compressed public assets based on nitro option 'compressPublicAssets' if those assets are present then it means the response may 'Vary' based on Accept-Encoding request header.

Currently the 'Vary' based on Accept-Encoding request header is included only if the Accept-Encoding request header exists. This introduces a problem that the Vary header can get omitted if the client sends a request without Accept-Encoding header. This would cause the server to respond with a uncompressed file that would then be cached in shared caches and served to everyone.

Shifting the logic so that the header is always sent if the server is optionally configured to return uncompressed, gz or br public assets removes the possibility for a client to poison the cache for all.

Resolves #3077

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@paulcrussell paulcrussell requested a review from pi0 as a code owner July 3, 2025 16:45
@paulcrussell paulcrussell force-pushed the fix/content-negotiation branch from fb0e772 to cb2599c Compare July 4, 2025 12:28
@paulcrussell
Copy link
Author

@pi0 I've updated the commit to fix the failing tests.

appendResponseHeader(event, "Vary", "Accept-Encoding");
}

for (const encoding of encodings) {
Copy link
Member

Choose a reason for hiding this comment

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

We are already iterating on encoding variations, on match, we can alternatively also append the header instead of additional loop

Copy link
Author

Choose a reason for hiding this comment

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

Hi @pi0 the fix here is to change the dependency of the Vary response header.
current: dependent on the request accept-encoding request header
proposed: dependent on whether the application has the capability to Vary the response

So it needs to iterate all possible encodings (i.e. gz or .br) and validate if at least one encoding is enabled. In this case the server is capable of Varying the response so the Vary header can be sent.

Copy link
Member

Choose a reason for hiding this comment

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

Please let me double check this, we can land fix in next releases.

@pi0 pi0 changed the title fix(content-negotiation) respect encoding file presence for Vary header fix(static): always add vary: accept-encoding for compressed assets Jul 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants