Firefox container storage weirdness

[eritbh]

from /u/durinthal on discord:

[creesch]

Probably related to

https://bugzilla.mozilla.org/show_bug.cgi?id=1380812#c7

[pe1uca]

Yes, is caused by that, background always using the default container.
This can be solved with webRequest.onBeforeSendHeaders as described here

Here's the PoC for toolbox.

Set up listeners to update the container we're using and the headers of the requests if needed.
The custom header tmp-source was included since otherwise this would update all requests, not only the ones from toolbox.

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 fetchOptions as a flag to replace the headers

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 window._TBCore is used as well as localStorage, so those need to be updated to take in mind the cookieStoreId.

[eritbh]

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 tabs.onActivated to determine what cookie store to use for requests is probably a bit fragile. In some situations, requests may be made by tabs that aren't the currently active one. I'd probably make use of the sender parameter of the runtime.onMessage callback to get the cookie store of the tab directly, and store it as the value of the temporary header so the onBeforeSendHeaders handler has direct access to it. I'd also pick a slightly more unique header name to minimize the chances of colliding with other extensions, just to be safe. Here's what applying those tweaks might look like:

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.

[pe1uca]

In some situations, requests may be made by tabs that aren't the currently active one

The first thing that pops in my mind are notifications, that show up as long as a reddit tab is open.
I'm guessing this will also need to have in mind how to let the user know where some messages originated, based on the docs the icon, color and name of the container could be easily used.

---

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.

Based on this post Firefox will still support WebRequest with MV3, AFAIK chromium based browsers will definitely drop it in favor of declarativeNetRequest.
Taking a look at the documentation is not possible to dynamically set cookies, unless we can generate the json file on the fly, which I'm guessing is not possible.
So, yes, unless declarativeNetRequest adds support to dynamically set cookies, this will be a browser-specific implementation.

[eritbh]

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.

https://github.com/toolbox-team/reddit-moderator-toolbox/blob/master/extension/data/background/handlers/notifications.js#L134-L148

The mark read API request we can fix by storing the sender ID in notification metadata and replacing $.ajax with makeRequest() and passing the tab along as the sender. Opening a new tab while ensuring it's in the same container/incognito state as the tab that generated the notification will be a bit trickier. The simplest way I can think of would be to send a message back to the tab and have the content script open the new tab itself, but that would mean the tab might open in a window that's not the most recently focused. We should identify other potential solutions - does the tabs API let us create a tab in a specific container in an arbitrary window? Is that cross-platform? What about incognito, which has to be its own window and not a tab?

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.

So, yes, unless declarativeNetRequest adds support to dynamically set cookies, this will be a browser-specific implementation.

It should be fine to make this conditional on browser.webRequest existing if Chrome is just removing it outright. That shouldn't be a huge deal, since Chrome already supports "incognito": "split" and the background page is unique per-context already; this issue doesn't exist there right now.

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.

[creesch]

we can definitely come up with some way to differentiate the container a notification is coming from for the user.

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

$.ajax

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.

[eritbh]

We determined on Discord that tb-global can be handled by querying for tabs within the current cookieStoreId rather than querying all tabs; that's used for updating things like toolbar counters which are tied to the current account. We'll do the same with in-page notifications to ensure notifications for one container don't show up in another container (native notifications will be unaffected since they're displayed outside the tab context, we'll find another way to differentiate those).

Caching will be a bit more complicated to make per-container; we can convert the single cache object to a Map or object with cookieStoreIds as keys so each one can have its own cache. The cache is also stored via TBStorage though, so we'll need to make sure that if we change the structure we store, things will still work on first run with the old cache layout being read from storage. We'll also need to double check that the cache isn't recorded in settings backups and stuff like that, since we don't want to share cookie store IDs beyond the browser.

the solution can never be simple, can it :V

[eritbh]

@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.

[pe1uca]

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 😅

[eritbh]

no worries! I'll see what I can come up with. Thanks again for pointing us in the right direction!