fastify / fastify-compress

Fastify compression utils
MIT License
199 stars 46 forks source link

Accept-Encoding should not be added to 'Vary' for non-compressable Content-Types #279

Closed rasander closed 5 months ago

rasander commented 7 months ago

Prerequisites

Fastify version

4.25.2

Plugin version

6.5.0

Node.js version

20.x

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

Mac OS X and Linux Ubuntu

Description

"Accept-Encoding" is always added to Vary for both compressable and non-compressable content-types. This should only be added if content-type is compressable.

This means, that caching is not working properly in nginx or CDN, because some caching applications (in my case Akamai CDN), will only cache if Content-Encoding is matching Accept-Encoding (in case Accept-Encoding is included in Vary). For non-compressable content-types these will not match.

Steps to Reproduce

$ curl -I "http://localhost:3000/-/media/test.gif" -X GET -H "Accept-Encoding: br"
HTTP/1.1 200 OK
cache-control: public, max-age=604800
content-type: image/gif
vary: accept-encoding
content-length: 2971239
Date: Mon, 05 Feb 2024 14:50:33 GMT
Connection: keep-alive
Keep-Alive: timeout=72

$ curl -I "http://localhost:3000/-/media/test.gif" -X GET 
HTTP/1.1 200 OK
cache-control: public, max-age=604800
content-type: image/gif
vary: accept-encoding
content-length: 2971239
Date: Mon, 05 Feb 2024 14:50:47 GMT
Connection: keep-alive
Keep-Alive: timeout=72

Expected Behavior

$ curl -I "http://localhost:3000/-/media/test.gif" -X GET -H "Accept-Encoding: br"
HTTP/1.1 200 OK
cache-control: public, max-age=604800
content-type: image/gif
content-length: 2971239
Date: Mon, 05 Feb 2024 14:50:33 GMT
Connection: keep-alive
Keep-Alive: timeout=72

$ curl -I "http://localhost:3000/-/media/test.gif" -X GET 
HTTP/1.1 200 OK
cache-control: public, max-age=604800
content-type: image/gif
content-length: 2971239
Date: Mon, 05 Feb 2024 14:50:47 GMT
Connection: keep-alive
Keep-Alive: timeout=72
mcollina commented 7 months ago

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

Fdawgs commented 6 months ago

"Accept-Encoding" is always added to Vary for both compressable and non-compressable content-types. This should only be added if content-type is compressable.

@rasander Do you have a link to a spec that states this?

stanleyxu2005 commented 5 months ago

@rasander Could you confirm if my understanding is correct?

  1. Wherever response.headers['content-encoding'] is defined (unless the value is identity, then the header vary should contain accept-encoding
  2. vice versa

Open question: if only one accept-encoding in request header, should vary header be set in response?

I have provided a PR, but I'm not so certain, if the changes in tests are all valid. @mcollina @Fdawgs

stanleyxu2005 commented 5 months ago

"Accept-Encoding" is always added to Vary for both compressable and non-compressable content-types. This should only be added if content-type is compressable.

@rasander Do you have a link to a spec that states this?

https://www.fastly.com/blog/best-practices-using-vary-header See the section How to Use Vary to Solve Problems