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

No Limit on Cache Size #922

Open
tehryanx opened this issue May 5, 2023 · 3 comments
Open

No Limit on Cache Size #922

tehryanx opened this issue May 5, 2023 · 3 comments

Comments

@tehryanx
Copy link

tehryanx commented May 5, 2023

Using data-turbo-preload, it's possible to overload the cache and make the browser unstable. This turns a simple link injection bug into a full browser DOS. Below are screenshots where I've loaded a 30mb and a 50mb file into cache using nothing but a link tag. Note how the memory footprint balloons.

image

image

Providing a configuration option to limit the size of the cache could mitigate this.

@seanpdoyle
Copy link
Contributor

@tehryanx thank you for opening this issue!

I'm surprised that this is possible. The SnapshotClass has had a size property since the project was ported from Turbolinks. Calls to put and get on that call invoke touch(), which them invokes trim().

Recently, the original implementation has been split into a new MemoryStore cache, which still accepts a size, and has a default size of 10 entries.

@afcapel @kevinmcconnell since you two have recently implemented the new caching mechanisms, this is definitely worth investigating further.

@afcapel
Copy link
Collaborator

afcapel commented Oct 23, 2023

@seanpdoyle I don't think this issue is related to the disk cache additions, since it predates the PR being merged.

The normal Turbo cache is an in memory cache, so preloading a very big page will load it into memory. That's the expected behaviour. You can fix this removing the preloading or opting out of caching.

Agreed that now that we have a disk cache it'd be interesting to explore if we can limit the cache byte size, instead of limiting the number entries, but I suspect it will be a bit difficult to nail down in a general way.

@seanpdoyle
Copy link
Contributor

@afcapel that makes a lot of sense.

I don't think this issue is related to the disk cache additions

I'm sorry, I didn't mean to imply that you had introduced it, only that your fresh context on that area of the codebase might help to inform a solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants