-
Notifications
You must be signed in to change notification settings - Fork 38
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
Firefox container storage weirdness #98
Comments
Probably related to |
Yes, is caused by that, background always using the default container. Here's the PoC for toolbox. Set up listeners to update the container we're using and the headers of the requests if needed. let storeId = '';
browser.webRequest.onBeforeSendHeaders.addListener(
async (args) => {
if (!args.requestHeaders) return args;
let source = args.requestHeaders.find(value => value.name === 'tmp-source');
if (!source || source.value !== 'extension') return args;
let cookies = await browser.cookies.getAll({
domain: 'reddit.com',
storeId: storeId
});
let cookiesMap = cookies.map(value => `${value.name}=${value.value}`);
const requestHeaders =
args.requestHeaders.filter((header) => {
return header.name !== 'tmp-source' &&
header.name !== 'Cookie';
})
.concat({
name: 'Cookie',
value: cookiesMap.join(';'),
});
args.requestHeaders = requestHeaders;
return args;
},
{ urls: [`https://*.reddit.com/*`] },
['blocking', 'requestHeaders'],
);
browser.tabs.onActivated.addListener(async (arg) => {
let tab = await browser.tabs.get(arg.tabId);
storeId = tab.cookieStoreId;
}); The only difference when making a request is to include our custom header in async function makeRequest() {
const fetchOptions: RequestInit = {
credentials: 'include', // required for cookies to be sent
redirect: 'error', // prevents strange reddit API shenanigans
method: 'GET',
cache: 'no-store',
headers: [
['tmp-source', 'extension']
]
};
let url = 'https://old.reddit.com/subreddits/mine/moderator.json';
let res = await fetch(url, fetchOptions);
} Now, this won't play nice with the cache, I quickly saw that |
Thanks a ton for putting together that proof-of-concept, that's super helpful and it looks like a really solid start! I think we'd welcome a PR for this issue based on what you just posted. This is a bit of pre-emptive code review nitpicking which we can probably discuss more in a PR, but my initial thought is that using the most recent async function makeRequest({
// existing request options...
}, sender) {
const fetchOptions = {
headers: [
['x-toolbox-tmp-cookiestore', sender.tab.cookieStoreId],
],
// other headers...
};
// perform the request...
}
// this line is modified to forward the message sender to makeRequest
messageHandlers.set('tb-request', (requestOptions, sender) => makeRequest(requestOptions, sender).then(...)
browser.webRequest.onBeforeSendHeaders.addListener(async args => {
if (!args.requestHeaders) {
return args;
}
const storeId = args.requestHeaders.find(header => header.name === 'x-toolbox-tmp-cookiestore')?.value;
if (!storeId) {
return args;
}
// fetch cookies from that store, attach to request...
}); The only other concern I have is whether this works in Chrome as well as Firefox, or if we'll need to make this a browser-specific behavior. I'm not sure how Chrome MV3 would impact this solution if applicable. |
The first thing that pops in my mind are notifications, that show up as long as a reddit tab is open.
Based on this post Firefox will still support |
Yeah, notifications could get more complicated; we can definitely come up with some way to differentiate the container a notification is coming from for the user. We also need to think about notification click handlers: When a notification is clicked, it triggers opening a new tab and usually sends an API request to mark the user's inbox as unread; the handling for both is naive. The mark read API request we can fix by storing the sender ID in notification metadata and replacing I tend to be sensitive to this sort of thing because I work with virtual desktops in Windows a lot, and poor handling of stuff like this regularly causes me to get switched to an entirely different desktop so some application can open a tab in a Firefox window I'm not using. So if we need to redo this part of the notifications system, I want to make sure we consider our options and get the UX right.
It should be fine to make this conditional on Let me know if you're interested in putting together a PR for this, or if you'd rather I own the development for it myself. @creesch and I talked on Discord, and we're interested in getting a fix for this issue into the upcoming 6.0 release. |
The easiest solution is probably to include the logged in username in the notification. It doesn't rule out people having the same user logged into multiple containers, but to be frank those people can't be helped anyway :P
I was confused for a moment... why do we still use jquery ajax there and not fetch? Other than that it does seem like we should indeed store the tab ID or something like that. However, I am not seeing right away how we can differentiate the container context in which the url should be opened. https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/notifications/create Another point of concern is our other global handling of things like here: https://github.com/toolbox-team/reddit-moderator-toolbox/blob/master/extension/data/background/handlers/globalmessage.js Currently, with the Chrome implementation we can use the naive approach, but also here we'd need a way to differentiate what tabs to send an event to. The tl;dr is that Firefox's implementation of background tabs is utterly FUBAR and doesn't allow for actual privacy minded usage unless we go out of our to make it so. (see also [this other discussion on Bugzilla I had about two years ago) I am fairly sure we can get API handling to work based on the mentioned approach, but I am not so confident other mechanisms have a workaround as well. |
We determined on Discord that Caching will be a bit more complicated to make per-container; we can convert the single cache object to a the solution can never be simple, can it :V |
@pe1uca Is this something you want to work on a PR for, or should I take it from here? Just want to make sure I'm not stepping on your toes if I start work on this. |
Sorry for the late response, this is something I wouldn't be able to do in a timely manner, I mean I don't know the code very well and it'll be a while for me to learn it 😅 |
no worries! I'll see what I can come up with. Thanks again for pointing us in the right direction! |
from /u/durinthal on discord:
The text was updated successfully, but these errors were encountered: