stoically / temporary-containers

Firefox Add-on that lets you open automatically managed disposable containers
https://addons.mozilla.org/firefox/addon/temporary-containers/
MIT License
846 stars 57 forks source link

Per-domain exclusions not excluding sites from isolation #607

Closed JustEffy closed 1 year ago

JustEffy commented 1 year ago

temporary_containers_preferences_2022-10-18_19.03.00.json.txt TCDebug.log

Actual behavior / Steps to reproduce

.. Make new profile, install Temporary Containers. Turn on automatic mode, set all Global Isolation options to domain+subdomain. Make per-domain rules for lowerclapton.nhs.uk and .lowerclapton.nhs.uk, excluding practice365.co.uk and .practice365.co.uk from isolation in each rule. Visit lowerclapton.nhs.uk, click on any of the practice365.co.uk links, and instead of opening in the same temp container they open in a new one.

Expected behavior

.. "Domain Patterns added here will not get isolated if encountered in a Navigation or Mouse Click Isolation." I was expecting to click through the practice365 links in the same temp container as the originating site, once I'd set up the exclusion rules.

In any case, thank you for TC, it's really good!

stoically commented 1 year ago

Unfortunately I can't reach lowerclapton.nhs.uk from here. There's no DNS entry available. Maybe it's Geo-locked?

However, it seems you found indeed a bug where the exclusion pattern does not properly take into account mouse clicks. If you configure the mouse clicks to Never for the per-domain isolation rules, it'll work as expected. Generally that's no issue, since navigation isolation always takes precedence over mouse click isolation – so if you have navigation isolation configured, configuring the same mouse click isolation rules has no effect. It only gets relevant if you configure different isolation behaviors.

JustEffy commented 1 year ago

Ah, sorry, that's my bad! Should have been www.lowerclapton.nhs.uk . I'll try setting mouse clicks to Never and report back.

JustEffy commented 1 year ago

temporary_containers_preferences_2022-10-19_17.27.51.json.txt TCDebug2.log Still seeing the same behaviour here

stoically commented 1 year ago

Turns out they're doing funky double-redirects. You need to add *.lowerclapton.nhs.uk as exclusion pattern to the *.lowerclapton.nhs.uk per-domain isolation rule, then it works.

JustEffy commented 1 year ago

That works... I'm trying to get my head around what's happening so I can write rules correctly in future.

So, the redirect back from practice365 is triggering the .lowerclapton rule (because... shenanigans? Is it still in the context of lowerclapton when it triggers?), but* when the rule's actually being tested it's treating practice365 as the source and therefore not being allowed by the Global Different from Tab Domain & Subdomains rules that would allow lowerclapton pages to load other lowerclapton pages without triggering isolation?

And thank you for TC and for your help!

stoically commented 1 year ago

Glad it works for you and thanks!

This looks like a race condition around the active tab domain. Looking at the logs, without the additional exclude:

[webRequestOnBeforeRequest] incoming request" {"requestId":"211","url":"https://practice365.co.uk/f84003/health-information/book-an-appointment/" [shouldIsolateNavigation] not isolating because excluded domain pattern matches" "https://practice365.co.uk/f84003/health-information/book-an-appointment/" "practice365.co.uk" [webRequestOnBeforeRequest] incoming request" {"requestId":"211","url":"https://www.lowerclapton.nhs.uk/health-information/book-an-appointment/ [shouldIsolateNavigation] found pattern" "*.lowerclapton.nhs.uk" {"action":"global"} [checkIsolationPreferenceAgainstUrl]" "notsamedomain" "practice365.co.uk" "www.lowerclapton.nhs.uk" [checkIsolationPreferenceAgainstUrl] isolating based on "notsamedomain"

It seems that at the point of comparing the domains, the tab updated to the practice365.co.uk domain – or, maybe, there's a bug because Firefox's request id stays the same and TC might do some naive caching there. Could be both I guess.

With the additional exclude it's

[shouldIsolateNavigation] not isolating because excluded domain pattern matches" "https://www.lowerclapton.nhs.uk/health-information/book-an-appointment/" "*.lowerclapton.nhs.uk

because it compares the full incoming request URL against the exclude patterns, not the active tab domain.

Overall the TC request/isolation handling could use some long overdue improvements, especially #397 would be nice. But, oh well, priorities.