unjs / ipx

🖼️ High performance, secure and easy-to-use image optimizer.
MIT License
1.21k stars 58 forks source link

IPX ignores `s-maxage` when `max-age` is present in the HTTP source `cache-control` header #166

Closed arpadgabor closed 9 months ago

arpadgabor commented 10 months ago

Environment

Reproduction

I can pinpoint the location with the bug so it should be enough, please let me know if you need more.

Describe the bug

This code in the http source only accounts for the 1st max-age occurence.

https://github.com/unjs/ipx/blob/b82adb795e7d376f62d3bbd938b165b34c519085/src/sources/http.ts#L58-L65

Thing is, it seems CloudFront will include both a max-age and s-maxage in the response from a cached object. An excerpt from my object response headers:

content-type: image/jpeg
content-length: 4061118
date: Wed, 13 Sep 2023 06:14:30 GMT
last-modified: Tue, 12 Sep 2023 21:30:57 GMT
etag: "7353ae45c7f4d79dc16b3b1513dcd00f"
x-amz-server-side-encryption: AES256
+ cache-control: public,max-age=0,s-maxage=31536000,must-revalidate
accept-ranges: bytes
server: AmazonS3
+ x-cache: Hit from cloudfront

The regex in the source code ignores the s-maxage, as can be noticed in this playground.

MDN notes that:

The s-maxage response directive also indicates how long the response is fresh for (similar to max-age) — but it is specific to shared caches, and they will ignore max-age when it is present.

All in all, I think IPX should do either one of two things:

Currently, I am unable to cache /_ipx/* images in Nuxt 3 because all images are sent back using the max-age=0 that is inherited from the HTTP source and completely ignores what I had set (I spent way too much time debugging this).

Additional context

I might be able to submit a PR for this fix, but let me know what you think would be the best approach (honestly I think a combination of both but prioritize the maxAge set by the developer in the IPX options).

Logs

No response

flapili commented 9 months ago

I have exactly the same issue ? did you get it working without a lot of hack in source code directly ? if not I'll try to make a RP

arpadgabor commented 9 months ago

@flapili nope, I patched ipx to fix it in the meantime. Hopefully we can get this fix in the v2 roadmap.

flapili commented 9 months ago

@Atinux and others maintainers does that is a big breaking change in your opinion ?

pi0 commented 9 months ago

Hi, dear @flapili, and thanks for explaining your issue.

It is not a breaking change but it is not something I think we could have enabled by default. s-maxage cache-control directive instructs CDN proxies how long they can keep a resource cached. IPX is not a reverse caching proxy to do so. So if reverse proxy explicitly requests max-age is zero, it should be respected and checked every time. s-max-age could be used if for example IPX caches the HTTP response (we might in the future)

For a quick solution, i have added a new option for http.ignoreCacheControl (can be configured for nuxt image v1 stable) via https://github.com/unjs/ipx/commit/4690342e26612a95b792141241ac2f18effc2cf0 this allows you to opt-out from infering cache-control header altogether and you can explicitly override maxAge.

We could also support an option like http.useSMaxAge but since it is not same semantics for directive, i consider it unsafe to be included but still open to support via another opt-in configuration if you really think would be needed in your case.