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

Manually rewrite cookies for container requests #687

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

eritbh
Copy link
Member

@eritbh eritbh commented Dec 1, 2022

Fixes #98.

Works around Firefox's lack of support for "incognito": "split" by manually rewriting the Cookie header of outgoing requests to match the tab that initiated the request. This lets Toolbox work properly with Firefox private windows and containers.

This is just a workaround for the request cookie behavior; notably, it doesn't split cache storage on the background page between cookie stores. Having different users logged in to different contexts will result in conflicting information being written back and forth in the cache. That will need more thought, and I think in an ideal world we would split the cache by logged-in user and allow multiple cookie stores with the same logged-in user to share the same cache, rather than naively splitting it based only on cookie store. There are probably other aspects of Toolbox that will need more thought for multi-user support, too.

This introduces two new permissions to the Firefox manifest only: webRequest and webRequestBlocking, which are required for the workaround.

@creesch
Copy link
Member

creesch commented Dec 4, 2022

That will need more thought, and I think in an ideal world we would split the cache by logged-in user and allow multiple cookie stores with the same logged-in user to share the same cache, rather than naively splitting it based only on cookie store.

Yup, this was also on my mind when I did the work for manifest v3.
There is a bit of an extra challenge involved with clearing the cache here. For chrome, I just decided on having two caches, on for regular session and one for incognito sessions. That works because we know those are the only two, but gets a bit more complex with per user caching.

@creesch
Copy link
Member

creesch commented Dec 15, 2022

https://github.com/toolbox-team/reddit-moderator-toolbox/blob/master/extension/data/background/handlers/cache.js#L9

This is what we have now. Looking at the code in this PR we might be able to leverage the cookieID as a prefix to store cache. Assuming the ID is always the same from related tabs (I assume it is).

We still need to implement a method that cleans up the cache after a while, but it gets us halfway there.

@eritbh
Copy link
Member Author

eritbh commented Feb 25, 2023

Now that I'm looking at this again, there are security concerns with this solution. A malicious webpage could make a request to Reddit setting our cookie store header set and use that to send Reddit requests using credentials from other containers. This would require knowledge of the cookie store IDs ahead of time, but I would want to look into ways to mitigate this before releasing.

Previously any request could have the cookie store ID header set, resulting in Toolbox sending cookies that the originator isn't supposed to have access to. By instead generating a random ID for the header value and storing the cookie store ID in an object keyed by this random value, we can ensure that only Toolbox requests will have their `Cookie` headers rewritten.
@eritbh
Copy link
Member Author

eritbh commented Apr 20, 2023

9d7e8a3 fixes the security concern by storing the cookieStoreId associated with a request in a metadata map rather than directly in a header; a randomly generated "request ID" is used to key into the map and retrieve the cookie store ID to use for cookie rewriting. Entries in the map are deleted after use so they can't be reused by other requests.

Because of Chrome mv3 shenanigans, using local state here instead of browser.storage.local (like we do for notifications) isn't technically correct, but this logic will only ever be used in Firefox anyway, and even in Chrome it seems incredibly unlikely that the worker will die and be brought back up between initiating an outgoing request and receiving the same request in webRequest.onBeforeSendHeaders.

@creesch
Copy link
Member

creesch commented Apr 21, 2023

Removing this bit of logic should probably also be part of this PR https://github.com/toolbox-team/reddit-moderator-toolbox/blob/master/extension/data/init.js#L145-L149

At least, I assume it also works with incognito windows.

@eritbh eritbh added enhancement New feature or request core: background labels Apr 21, 2023
@creesch
Copy link
Member

creesch commented May 19, 2023

@eritbh eritbh marked this pull request as draft May 24, 2023 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core: background enhancement New feature or request
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

Firefox container storage weirdness
2 participants