Opened 5 years ago

Closed 5 years ago

#10819 closed enhancement (fixed)

Create preference for DOM storage isolation and image cache isolation

Reported by: mikeperry Owned by: mikeperry
Priority: Medium Milestone:
Component: Firefox Patch Issues Version:
Severity: Keywords: tbb-mozilla-merge, interview, MikePerry201406R
Cc: gk, mcs, brade, arthuredelstein@… Actual Points:
Parent ID: #10820 Points:
Reviewer: Sponsor:

Description

In #6564, we created a patch to isolate DOM storage to first party domain. It could use a pref to control if it is enabled, and ideally also have an option to control if it only applies to private browsing mode windows (though that could be a separate ticket if it is substantial).

Our patch for the isolation is here:
https://gitweb.torproject.org/tor-browser.git/commitdiff/1b3c110a29ae11b50ce2bf56d5954773262e67c0

Child Tickets

Attachments (2)

0001-Add-a-pref-privacy.thirdparty.isolate-to-allow-the-a.patch (49.6 KB) - added by arthuredelstein 5 years ago.
0001-Add-a-pref-privacy.thirdparty.isolate-to-allow-the-a.2.patch (50.3 KB) - added by arthuredelstein 5 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 5 years ago by mikeperry

Parent ID: #10820

comment:2 Changed 5 years ago by gk

Cc: gk added

comment:3 Changed 5 years ago by mcs

Cc: mcs brade added

comment:4 Changed 5 years ago by arthuredelstein

Cc: arthuredelstein@… added
Summary: Create preference for DOM storage isolationCreate preference for DOM storage isolation and image cache isolation

comment:5 Changed 5 years ago by arthuredelstein

The pref will also cover image cache isolation, whose TBB patch is here:
https://gitweb.torproject.org/tor-browser.git/commitdiff/af2e4eb80cfb723066816185bfd68883c0e7e623

comment:6 Changed 5 years ago by mikeperry

Keywords: MikePerry201404R added

comment:7 Changed 5 years ago by mikeperry

Keywords: MikePerry201405R added; MikePerry201404R removed

comment:8 Changed 5 years ago by mcs

Mike asked Kathy Brade and me to review this patch (he is reviewing it as well). Here are our comments:

What is the effect on the other subsystems? If first party isolation is disabled, we probably want everything to work just like it does in Firefox. For imageLoader, it looks like values generated by GetCacheKey() will still be different than in Firefox. For DOM storage, it looks like using a NULL URI will return everything to the original Firefox behavior. But please verify if you have not already done so.

Unless is needs to be called from elsewhere, IsFirstPartyIsolationActive() should be a private method.

IsFirstPartyIsolationActive() should avoid getting the channel and checking for private browsing mode if the pref value is 0 or 2.

ThirdPartyUtil::GetFirstPartyIsolationURI() should check the return value from IsFirstPartyIsolationActive. Also, please initialize isolationActive when it is declared. Note that if IsFirstPartyIsolationActive() is converted to a private method that returns a bool, these issues disappear.

Renaming firstPartyURI to firstPartyIsolationURI everywhere makes for a large diff but it is a reasonable thing to do, especially if these new changes are collapsed with the original image loader and DOM storage patches.

comment:9 in reply to:  8 Changed 5 years ago by arthuredelstein

Replying to mcs:

Mike asked Kathy Brade and me to review this patch (he is reviewing it as well).

Thanks!

What is the effect on the other subsystems? If first party isolation is disabled, we probably want everything to work just like it does in Firefox. For imageLoader, it looks like values generated by GetCacheKey() will still be different than in Firefox.

That's true. I've changed GetCacheKey so that it returns the bare image URI spec, just as expected in standard Firefox.

For DOM storage, it looks like using a NULL URI will return everything to the original Firefox behavior. But please verify if you have not already done so.

Yes, I checked again and I believe nullptr URI behavior is the same as standard Firefox.

Unless is needs to be called from elsewhere, IsFirstPartyIsolationActive() should be a private method.

Fixed.

IsFirstPartyIsolationActive() should avoid getting the channel and checking for private browsing mode if the pref value is 0 or 2.

Fixed.

ThirdPartyUtil::GetFirstPartyIsolationURI() should check the return value from IsFirstPartyIsolationActive. Also, please initialize isolationActive when it is declared. Note that if IsFirstPartyIsolationActive() is converted to a private method that returns a bool, these issues disappear.

Done.

(I posted a new version of the patch.)

comment:10 Changed 5 years ago by mikeperry

Keywords: MikePerry201406R added; MikePerry201405R removed

comment:11 Changed 5 years ago by mikeperry

Resolution: fixed
Status: newclosed

Ok, this has been merged for 4.0-alpha-1. It should also appear in the next nightly.

Thanks a lot, Arthur+mcs!

Note: See TracTickets for help on using tickets.