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

isCacheable allowed cache by default if the cache-control header is empty may leading to a security flaw in ssg content #176

Open
0x221A opened this issue May 7, 2023 · 2 comments

Comments

@0x221A
Copy link

0x221A commented May 7, 2023

This problem appeared in @sveltejs/adapter-cloudflare@2.2.2 that let Cache.save handle whether to cache the response or not.

The scenario that leads to the problem is when Cache-Control is empty and the user is logged in without any problem all the content is generated and cached to the CF server. Then another tries to access directly to the dashboard without any authorization CF server response cached content of another user directly to them.

I think that we should not allow cache by default and let the developer decide which response to cache to avoid this issue.

@psytrx
Copy link

psytrx commented May 9, 2023

We've had a somewhat related incident caused by weird caching behavior that made me look closer at isCacheable from worktop, and I had the same gut feeling about it.

(Sadly, it wasn't the cause for us because we're on 2.2.1 in prod. Still trying to pinpoint our cause.)

If I'm not mistaken, requests without a cache-control header are okay to be cached, depending on multiple conditions. There's a few exceptions in the HTTP spec, mainly regarding expires, authorization, pragma and vary headers. But I haven't read the full spec and would love someone more knowledgeable to chime in. Worktop at least handles the vary case.

A more defensive approach to caching sounds reasonable to me.

@0x221A
Copy link
Author

0x221A commented May 12, 2023

Without a cache-control header, it is okay to cache but has a tradeoff between performance and security. In this case, I would not cache by default since we can set the cache-control header in the _headers file. And I think that most of the frontend frameworks that have CF adapter already include a _headers file like svelte-kit has done in .svelte-kit/cloudflare/_headers.

@0x221A 0x221A changed the title isCacheable allowed cache by default if it was empty may leading to a security flaw in ssg content isCacheable allowed cache by default if the cache-control header is empty may leading to a security flaw in ssg content May 18, 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

2 participants