Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
arpadgabor opened this issue Sep 13, 2023 · 4 comments

Comments

@arpadgabor
Copy link

arpadgabor commented Sep 13, 2023

Environment

  • Linux WSL2
  • Node: v18.16.1
  • @nuxt/image: 1.0.0-rc.1
  • IPX: 1.3.0

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.

ipx/src/sources/http.ts

Lines 58 to 65 in b82adb7

let maxAge = options.maxAge;
const _cacheControl = response.headers.get("cache-control");
if (_cacheControl) {
const m = _cacheControl.match(/max-age=(\d+)/);
if (m && m[1]) {
maxAge = Number.parseInt(m[1]);
}
}

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:

  • Respect whatever maxAge was set by the user of the module, or
  • Also account for s-maxage

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
Copy link

flapili commented Oct 15, 2023

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
Copy link
Author

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

@flapili
Copy link

flapili commented Oct 15, 2023

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

@pi0
Copy link
Member

pi0 commented Oct 17, 2023

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 4690342 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.

@pi0 pi0 closed this as completed Oct 17, 2023
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

No branches or pull requests

3 participants