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

perf(server/index.ts): do not set cookies to image proxy so CDNs can cache images #3332

Merged
merged 1 commit into from Feb 12, 2023

Conversation

lunks
Copy link
Contributor

@lunks lunks commented Feb 11, 2023

This is only a problem if CSRF protection is enabled.

Description

CDNs like Cloudflare bypass their cache if cookies are set in the response.
clearCookies middleware removes the header before imageproxy serves the image.

I initially considered creating a middlewares collection and removing CSRF protection from the list if it is present for the image proxy, but removing the header is a much simpler solution.

Screenshot (if UI-related)

To-Dos

  • Successful build yarn build
  • Translation keys yarn i18n:extract
  • Database migration (if required)

Issues Fixed or Closed

none

@sct
Copy link
Owner

sct commented Feb 11, 2023

Curious why you are having cookies sent with your image proxy. I seem unable to reproduce. My images served through Cloudflare are returning hits for the cache.

@lunks
Copy link
Contributor Author

lunks commented Feb 11, 2023

Curious why you are having cookies sent with your image proxy. I seem unable to reproduce. My images served through Cloudflare are returning hits for the cache.

If you enable CSRF, it gets set on every request. Cloudflare works fine if I disable CSRF, but I want the extra security the CSRF token provides.

@sct
Copy link
Owner

sct commented Feb 11, 2023

Ah right CSRF. I see. Got it.

…cache images

CDNs such as Cloudflare bypass their cache if cookies are set in the response.
clearCookies
middleware removes the header before imageproxy serves the image.
@lunks lunks force-pushed the do-not-set-cookies-on-image-proxy branch from c5d167c to 516a255 Compare February 11, 2023 16:40
@sct sct merged commit 966639d into sct:develop Feb 12, 2023
@sct
Copy link
Owner

sct commented Feb 12, 2023

@all-contributors please add @lunks for code

@allcontributors
Copy link
Contributor

@sct

I've put up a pull request to add @lunks! 🎉

@sct
Copy link
Owner

sct commented Feb 12, 2023

@holopin-bot @lunks lets get a badge here!

@holopin-bot
Copy link

holopin-bot bot commented Feb 12, 2023

Congratulations @lunks, you just earned a badge! Here it is: https://holopin.io/claim/cle1a6vy7009508mh4kay1a0z

This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account.
Or if you're new to Holopin, you can simply sign up with GitHub, which will do the trick!

@lunks
Copy link
Contributor Author

lunks commented Feb 13, 2023

Thank you for merging my PR and for the awesome project :)

@github-actions
Copy link

🎉 This PR is included in version 1.33.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

lenaxia pushed a commit to lenaxia/overseerr-oidc that referenced this pull request Jan 3, 2024
… images (sct#3332)

CDNs such as Cloudflare bypass their cache if cookies are set in the response.
clearCookies
middleware removes the header before imageproxy serves the image.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants