search

lukeed / worktop

The next generation web framework for Cloudflare Workers
MIT License
1.65k stars 42 forks source link

`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 1 year ago

0x221A commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.