Provide an option for first-party isolation in Private Browsing Mode

NEW
Assigned to

Status

()

enhancement
P2
normal
2 years ago
5 months ago

People

(Reporter: arthur, Assigned: tjr)

Tracking

(Blocks 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tor])

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
I would like to propose adding a pref to enable first-party isolation in Private Browsing mode. First-party isolation (currently behind the privacy.firstparty.isolate pref) prevents third party from tracking users across websites (aka supercookies).

According to a recent survey by DuckDuckGo (https://duckduckgo.com/download/Private_Browsing.pdf):
"""
  • 41.0 ±2.5% believe that Private Browsing “Prevents websites from tracking me.”
  • 39.1 ±2.6% believe that Private Browsing “Prevents ads from tracking me.”
"""

Including first-party isolation in PBM would help to meet these expectations without having to block content directly.

This functionality could be behind a pref, e.g. "privacy.firstparty.isolate.pbm". Perhaps one day the pref could be enabled by default.

The existing implementation of first-party isolation (using OriginAttributes) means that I think the necessary code changes are relatively small.
(Reporter)

Comment 1

2 years ago
Oops, the second sentence should be "First-party isolation ... prevents third parties from tracking users across websites by storing stateful information in the browser (aka supercookies)."
(Reporter)

Comment 2

2 years ago
I want to clarify my explanation. The pref I am proposing would be a boolean, "privacy.firstparty.isolate.pbm", with the following effect:

* When "privacy.firstparty.isolate.pbm" is false, Firefox behaves normally.
* When "privacy.firstparty.isolate" is true, first-party isolation is enforced in Private Browsing Windows only.

The pref would be false by default.

Comment 3

2 years ago
(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #2)
> I want to clarify my explanation. The pref I am proposing would be a
> boolean, "privacy.firstparty.isolate.pbm", with the following effect:
> 
> * When "privacy.firstparty.isolate.pbm" is false, Firefox behaves normally.
> * When "privacy.firstparty.isolate" is true, first-party isolation is
> enforced in Private Browsing Windows only.
> 
> The pref would be false by default.

What do you mean Firefox behaves "normally"? Why would "privacy.firstparty.isolate" is true be PB mode only - how do I then use FPI in normal browsing mode?

Isn't it easier to just have:
privacy.firstparty.isolate = applies to normal browsing mode
privacy.firstparty.isolate.pbm = applies to PB mode

Thus allowing 4 combinations: all on, all off, normal on and PB off, normal off and PB on
(Reporter)

Comment 4

2 years ago
(In reply to Simon Mainey from comment #3)

I'm sorry, I made a typo and I wish I could edit comment 2. I meant to write:

* When "privacy.firstparty.isolate.pbm" is false, Firefox behaves normally.
* When "privacy.firstparty.isolate.pbm" is true, first-party isolation is enforced in Private Browsing windows only.

"private.firstparty.isolate" would continue to apply as a global mode.

Comment 5

2 years ago
OK, that makes sense now.

"private.firstparty.isolate" is not global (it excludes PB Mode which does not currently use OriginAttributes, but I know what you mean) hence this ticket.

But from your comment 4 I see that *.pbm applies only to PB Mode windows, and as per current FPI pref, that only applies to normal windows. So exactly what we wanted, right - as per comment 3?
See Also: → FirstPartyIsolation
Whiteboard: [tor]

Comment 6

2 years ago
Can we rename the ticket to "Provide an option to restrict first-party isolation to Private Browsing Mode" - thanks

Comment 7

2 years ago
(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from Bug 1404017)
> Another alternative would be to convert privacy.resistFingerprinting to an
> integer. Then the integer could have three values:
> 
> 0: Fingerprinting Resistance off
> 1: Fingerprinting Resistance on in all windows
> 2: Fingerprinting Resistance on in Private-Browsing windows only

+1 for this but with privacy.firstparty.isolate
Marking P3 for PBM backlog.

Also, AFAIK the only private-browsing-mode config option we have so far is privacy.trackingprotection.pbmode.enabled

We should stick to a consistent naming scheme for private-browsing-mode configs, so I propose:

privacy.firstparty.isolate.pbmode.enabled
Priority: -- → P3
(Assignee)

Comment 9

a year ago
(In reply to Luke Crouch [:groovecoder] from comment #8)
> Marking P3 for PBM backlog.
> 
> Also, AFAIK the only private-browsing-mode config option we have so far is
> privacy.trackingprotection.pbmode.enabled
> 
> We should stick to a consistent naming scheme for private-browsing-mode
> configs, so I propose:
> 
> privacy.firstparty.isolate.pbmode.enabled

Is there a way to turn tracking protection mode outside of pbmode? FPI and AntiFingerpriting both currently exist as boolean prefs for the whole browser. Making them ints expose 3 states, while making a new pref makes it four:

Disabled Everywhere
Disabled in Normal Browsing, Enabled in PBM
Enable in Normal Browsing, Disabled in PBM
Enabled Everywhere

I don't have a strong preference, just noting the state expansion implicit in making it a new pref.
For TP, privacy.trackingprotection.enabled=true overrides privacy.trackingprotection.pbmode.enabled.

So we have 3 states with the 2 prefs:

Disabled everywhere: enabled = false; pbmode.enabled = false
Disabled in Normal Browsing, Enabled in PBM: enabled = false; pbmode.enabled = true (this is the default state)
Enabled everywhere: enabled = true; pbmode.enabled = true|false (the global overrides the pbmode)
See Also: → 1312655
(Assignee)

Updated

a year ago
Blocks: 554303
(Assignee)

Updated

a year ago
Blocks: 901614
(Assignee)

Updated

a year ago
Priority: P3 → P2
(Assignee)

Updated

a year ago
Blocks: 1449727
Comment hidden (spam)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Assignee: nobody → tom
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

a year ago
mozreview-review
Comment on attachment 8963461 [details]
Bug 1397624 Refactor browser_firstPartyIsolation.js so it will be able to test bth FPI prefs

https://reviewboard.mozilla.org/r/232392/#review238026


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/originattributes/test/browser/browser_firstPartyIsolation.js:135
(Diff revision 3)
>    await BrowserTestUtils.browserLoaded(tab.linkedBrowser, true, function(url) {
>      return url == BASE_URL + "dummy.html";
>    });
>  
> -  await ContentTask.spawn(tab.linkedBrowser, {}, async function() {
> +  await ContentTask.spawn(tab.linkedBrowser, {shouldBeIsolated}, async function(attrs) {
> +    let assertFunc = function(b) { return attrs.shouldBeIsolated ? b : !b; };

Error: 'assertfunc' is assigned a value but never used. [eslint: no-unused-vars]

::: browser/components/originattributes/test/browser/browser_firstPartyIsolation.js:308
(Diff revision 3)
> +  await openWindow_test(gBrowser, true);
> +  await window_open_redirect_test(gBrowser, true);
> +  await window_open_iframe_test(gBrowser, true);
> +  await form_test(gBrowser, true);
> +  await window_open_form_test(gBrowser, true);
>  });

Error: Newline required at end of file but not found. [eslint: eol-last]

Comment 21

a year ago
mozreview-review
Comment on attachment 8963721 [details]
Bug 1397624 Add tests for privacy.firstparty.isolate.pbmode.enabled

https://reviewboard.mozilla.org/r/232618/#review238028


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/originattributes/test/browser/browser_firstPartyIsolation.js:135
(Diff revision 1)
>    await BrowserTestUtils.browserLoaded(tab.linkedBrowser, true, function(url) {
>      return url == BASE_URL + "dummy.html";
>    });
>  
>    await ContentTask.spawn(tab.linkedBrowser, {shouldBeIsolated}, async function(attrs) {
>      let assertFunc = function(b) { return attrs.shouldBeIsolated ? b : !b; };

Error: 'assertfunc' is assigned a value but never used. [eslint: no-unused-vars]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (off-topic)
Comment hidden (off-topic)

Comment 29

a year ago
mozreview-review
Comment on attachment 8963460 [details]
Bug 1397624 Add a new privacy.firstparty.isolate.pbmode.enabled pref to enable FPI in PBM only

https://reviewboard.mozilla.org/r/232390/#review242102

Overall, this looks good, but I have a few questions about what happens to old data / old principals when FPI is first turned on.  Also, I'm not a peer reviewer for most of this, so we will need secondary review.

::: docshell/base/nsDocShell.cpp:3218
(Diff revision 4)
>    // When the first party isolation is on, the top-level docShell may not have
>    // the firstPartyDomain in its originAttributes, but its document will have
>    // it. So we get the firstPartyDomain from the nodePrincipal of the document
>    // before we compare the originAttributes.
> -  if (OriginAttributes::IsFirstPartyEnabled()) {
> +  // [[Tanvi: I don't know if this assert is necessary, but I'm being cautious.]]
> +  MOZ_ASSERT(targetOA.mPrivateBrowsingId == accessingOA.mPrivateBrowsingId);

Not necessary, but couldn't hurt.  Could catch issues if pbmode origin attribute isn't getting set appropriately.

::: js/xpconnect/wrappers/AccessCheck.cpp:66
(Diff revision 4)
>  AccessCheck::subsumesConsideringDomain(JSCompartment* a, JSCompartment* b)
>  {
> -    MOZ_ASSERT(OriginAttributes::IsFirstPartyEnabled());
>      nsIPrincipal* aprin = GetCompartmentPrincipal(a);
>      nsIPrincipal* bprin = GetCompartmentPrincipal(b);
> +    // [[Tanvi: I know from testing that sometimes we will compare two principles that

What do you mean?  See my next comment.

::: js/xpconnect/wrappers/AccessCheck.cpp:78
(Diff revision 4)
>                                                    JSCompartment* b)
>  {
> -    MOZ_ASSERT(!OriginAttributes::IsFirstPartyEnabled());
>      nsIPrincipal* aprin = GetCompartmentPrincipal(a);
>      nsIPrincipal* bprin = GetCompartmentPrincipal(b);
> +    // [[Tanvi: Necessary?]]

From my understanding, when you enable First Party Isolation, you start fresh, both becasue none of your old cookies make any sense anymore and because your old cookies get wiped.  Is that correct?  Looking at the rest of this diff, it seems like the old principals/data may still be in use.  Once FPI is enabled, don't all principals have to have a FPI Origin Attribute set?

If so, if you set FPI in regular mode, and compare 2 principals, both will have FPI enabled.

And if you set FPI only in private mode, you are presumably only comparing principals in private mode, so again FPI will be enabled in both.

However, this check doesn't hurt, so I would keep it.

::: js/xpconnect/wrappers/WrapperFactory.cpp:143
(Diff revision 4)
>      // compartment boundary. In that case, we only want to preserve waivers
>      // in transactions between same-origin compartments.
>      JSCompartment* oldCompartment = js::GetObjectCompartment(originalObj);
>      JSCompartment* newCompartment = js::GetContextCompartment(cx);
> +
> +    nsIPrincipal* oldprin = GetCompartmentPrincipal(oldCompartment);

Although the changes in this file look fine from the surface, I'm not an appropriate reviewer for this file.

::: js/xpconnect/wrappers/WrapperFactory.cpp:465
(Diff revision 4)
>      bool targetIsChrome = AccessCheck::isChrome(target);
> -    bool originSubsumesTarget = OriginAttributes::IsFirstPartyEnabled() ?
> +
> +    nsIPrincipal* originprin = GetCompartmentPrincipal(origin);
> +    nsIPrincipal* targetprin = GetCompartmentPrincipal(target);
> +
> +    // This ensures we do a deep comparison even if only one of the principles as FPI enabled.

s/principles as/principals has/

Comment 30

a year ago
mozreview-review
Comment on attachment 8963720 [details]
Bug 1397624 Remove privacy.firstparty.isolate.restrict_opener_access and act as if it were always enabled

https://reviewboard.mozilla.org/r/232616/#review242104

Are you sure you want to remove this pref?  Did it help with single sign on or alleviate any breakage?

Few comments/questions in the review, but again we will need secondary review.

::: browser/components/originattributes/test/browser/browser_windowOpenerRestriction.js:39
(Diff revision 2)
>      // Insert a cookie into this iframe.
>      childFrame.contentDocument.cookie = obj.cookieStr;
>  
>      // Open the tab here and focus on it.
>      let openedPath = obj.page;
> -    if (!obj.isPrefEnabled) {
> +    if (!obj.shouldRestrictAccess) {

Is there any case where shouldRestrictAccess would be false, since you are removing privacy.firstparty.isolate.restrict_opener_access and setting it to always be true.

::: browser/components/originattributes/test/browser/browser_windowOpenerRestriction.js:40
(Diff revision 2)
>      childFrame.contentDocument.cookie = obj.cookieStr;
>  
>      // Open the tab here and focus on it.
>      let openedPath = obj.page;
> -    if (!obj.isPrefEnabled) {
> +    if (!obj.shouldRestrictAccess) {
>        // If the pref is not enabled, we pass the cookie value through the query string

update comment to remove reference to "pref"

::: browser/components/originattributes/test/browser/file_windowOpenerRestrictionTarget.html:10
(Diff revision 2)
> -    // If the query string is given, we are expecting the window.opener can be accessed
> -    // across different first party domains, so we will match the cookie value.
> +    // The access of window.opener should be treated as cross-origin.
> +    // Therefore, it should fail.
> -    // Otherwise, the access of window.opener should be treated as cross-origin.
> -    // Therefore, it should fail at this setting.
>      let openerRestriction = true;
> -    let cookieValue;
> +    document.title = "fail";

cookieValue still needs to be defined.

I'm not clear what this is testing now.

Comment 31

a year ago
mozreview-review
Comment on attachment 8963461 [details]
Bug 1397624 Refactor browser_firstPartyIsolation.js so it will be able to test bth FPI prefs

https://reviewboard.mozilla.org/r/232392/#review242110

r+ with the fix to error messages.

::: browser/components/originattributes/test/browser/browser_firstPartyIsolation.js:19
(Diff revision 4)
> -  await ContentTask.spawn(tab.linkedBrowser, { firstPartyDomain: BASE_DOMAIN }, async function(attrs) {
> +  await ContentTask.spawn(tab.linkedBrowser, { shouldBeIsolated, firstPartyDomain: BASE_DOMAIN }, async function(attrs) {
> +    let assertFunc = function(b) { return attrs.shouldBeIsolated ? b : !b; };
>      info("document principal: " + content.document.nodePrincipal.origin);
> -    Assert.equal(docShell.getOriginAttributes().firstPartyDomain, "",
> -                 "top-level docShell shouldn't have firstPartyDomain attribute.");
> -    Assert.equal(content.document.nodePrincipal.originAttributes.firstPartyDomain,
> +    Assert.ok(docShell.getOriginAttributes().firstPartyDomain == "",
> +                 "top-level docShell shouldn't have firstPartyDomain attribute under any circumstance.");
> +    Assert.ok(assertFunc(content.document.nodePrincipal.originAttributes.firstPartyDomain ==

The error messages need to be updated, since with the updated code, in some cases, you may not want isolation.

Assume shouldBeIsolated is false.  We call assertFunc from line 15.  Assume the input b = false (firstPartyDomain attributes aren't set properly).

Then we will be returning !b, which is true.  In this case, the document should not have firstPartyDomain.  Hence, the error message needs to be updated.

To help fix this in a way where the error messages make sense, you could put actual variable values into the error messages.

::: browser/components/originattributes/test/browser/browser_firstPartyIsolation.js:135
(Diff revision 4)
>    await BrowserTestUtils.browserLoaded(tab.linkedBrowser, true, function(url) {
>      return url == BASE_URL + "dummy.html";
>    });
>  
> -  await ContentTask.spawn(tab.linkedBrowser, {}, async function() {
> +  await ContentTask.spawn(tab.linkedBrowser, {shouldBeIsolated}, async function(attrs) {
> +    let assertFunc = function(b) { return attrs.shouldBeIsolated ? b : !b; };

As reviewbot says, this assertFunc isn't used. In this case, whether we shouldBeIsolated or not, postmessage should work, right?  So maybe this test doesn't need changing?

::: browser/components/originattributes/test/browser/browser_firstPartyIsolation.js:224
(Diff revision 4)
> +    Assert.ok(assertFunc(docShell.getOriginAttributes().firstPartyDomain == attrs.firstPartyDomain),
>                   "window.open() should have firstPartyDomain attribute");
>  
>      // The document is http://example.com/browser/browser/components/originattributes/test/browser/test_firstParty.html
>      // so the firstPartyDomain will be overriden to 'example.com'.
> -    Assert.equal(content.document.nodePrincipal.originAttributes.firstPartyDomain,
> +    Assert.ok(assertFunc(content.document.nodePrincipal.originAttributes.firstPartyDomain ==

This and the following 2 error messages could be more descriptive to mention example.com by name.
Attachment #8963461 - Flags: review?(tanvi) → review+

Comment 32

a year ago
mozreview-review
Comment on attachment 8963721 [details]
Bug 1397624 Add tests for privacy.firstparty.isolate.pbmode.enabled

https://reviewboard.mozilla.org/r/232618/#review242114

::: browser/components/originattributes/test/browser/browser_firstPartyIsolation.js:35
(Diff revision 3)
>    });
>  
>    browser.removeTab(tab);
>  }
>  
>  /**

Add comment about private mode.

::: browser/components/originattributes/test/browser/browser_firstPartyIsolation.js:134
(Diff revision 3)
>    // So we wait until dummy.html is loaded
>    await BrowserTestUtils.browserLoaded(tab.linkedBrowser, true, function(url) {
>      return url == BASE_URL + "dummy.html";
>    });
>  
> -  await ContentTask.spawn(tab.linkedBrowser, {shouldBeIsolated}, async function(attrs) {
> +  await ContentTask.spawn(tab.linkedBrowser, {}, async function() {

looks like you cleaned up the issues in the previous patch with postmessage here.

::: browser/components/originattributes/test/browser/browser_firstPartyIsolation.js:327
(Diff revision 3)
> +  await window_open_redirect_test(gBrowser, false);
> +  await window_open_iframe_test(gBrowser, false);
> +  await form_test(gBrowser, false);
> +  await window_open_form_test(gBrowser, false);
> +
> +  Services.cookies.removeAll();

Do you need to call this before line 317 as well, when you switch from global isolation to no global isolation?  Why or why not?
Attachment #8963721 - Flags: review?(tanvi) → review+
(Assignee)

Comment 33

a year ago
mozreview-review
Comment on attachment 8963720 [details]
Bug 1397624 Remove privacy.firstparty.isolate.restrict_opener_access and act as if it were always enabled

https://reviewboard.mozilla.org/r/232616/#review242270

::: browser/components/originattributes/test/browser/browser_windowOpenerRestriction.js:39
(Diff revision 2)
>      // Insert a cookie into this iframe.
>      childFrame.contentDocument.cookie = obj.cookieStr;
>  
>      // Open the tab here and focus on it.
>      let openedPath = obj.page;
> -    if (!obj.isPrefEnabled) {
> +    if (!obj.shouldRestrictAccess) {

Yes. When FPI is disabled, we do not restrict operner access.

::: browser/components/originattributes/test/browser/browser_windowOpenerRestriction.js:40
(Diff revision 2)
>      childFrame.contentDocument.cookie = obj.cookieStr;
>  
>      // Open the tab here and focus on it.
>      let openedPath = obj.page;
> -    if (!obj.isPrefEnabled) {
> +    if (!obj.shouldRestrictAccess) {
>        // If the pref is not enabled, we pass the cookie value through the query string

Now, 'pref' refers to privacy.firstparty.isolate so I think this is sill accurate.

::: browser/components/originattributes/test/browser/file_windowOpenerRestrictionTarget.html:10
(Diff revision 2)
> -    // If the query string is given, we are expecting the window.opener can be accessed
> -    // across different first party domains, so we will match the cookie value.
> +    // The access of window.opener should be treated as cross-origin.
> +    // Therefore, it should fail.
> -    // Otherwise, the access of window.opener should be treated as cross-origin.
> -    // Therefore, it should fail at this setting.
>      let openerRestriction = true;
> -    let cookieValue;
> +    document.title = "fail";

Hm, I'm not sure why this file is so wrong.  Maybe I didn't roll a commit? In any event, I left some stuff out that I fixed.
(Assignee)

Comment 34

a year ago
mozreview-review-reply
Comment on attachment 8963721 [details]
Bug 1397624 Add tests for privacy.firstparty.isolate.pbmode.enabled

https://reviewboard.mozilla.org/r/232618/#review242114

> Do you need to call this before line 317 as well, when you switch from global isolation to no global isolation?  Why or why not?

I do; it's called in resetup.
(Assignee)

Comment 35

a year ago
mozreview-review-reply
Comment on attachment 8963460 [details]
Bug 1397624 Add a new privacy.firstparty.isolate.pbmode.enabled pref to enable FPI in PBM only

https://reviewboard.mozilla.org/r/232390/#review242102

> From my understanding, when you enable First Party Isolation, you start fresh, both becasue none of your old cookies make any sense anymore and because your old cookies get wiped.  Is that correct?  Looking at the rest of this diff, it seems like the old principals/data may still be in use.  Once FPI is enabled, don't all principals have to have a FPI Origin Attribute set?
> 
> If so, if you set FPI in regular mode, and compare 2 principals, both will have FPI enabled.
> 
> And if you set FPI only in private mode, you are presumably only comparing principals in private mode, so again FPI will be enabled in both.
> 
> However, this check doesn't hurt, so I would keep it.

No; It's my understanding tha FPI doesn't wipe any cookies. When you first enable FPI, you basically switch how you query the cookie jar, so there is nothing to be found when you start querying. When you turn FPI off, you go back to querying the old way, and you'll get all your original cookies. When you enable FPI again, you start querying int he FPI way, and you start getting all the cookies you had accumulated during the first time you had FPI enabled.

> you are presumably only comparing principals in private mode, so again FPI will be enabled in both.

I believe a comparison occurs between a principal from non-private mode and a principal from private mode. And therefore FPI may only be enabled in one of those principals.
(Assignee)

Comment 36

a year ago
I addressed all of the comments except those stemming from the discussion in Comment 35, which I think we need to sort out.

For rebasing purposes, I addressed the comments on the third patch (like fixing the error messages) in the fourth patch.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Flags: needinfo?(tanvi)

Comment 41

a year ago
mozreview-review
Comment on attachment 8963461 [details]
Bug 1397624 Refactor browser_firstPartyIsolation.js so it will be able to test bth FPI prefs

https://reviewboard.mozilla.org/r/232392/#review242296


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/originattributes/test/browser/browser_firstPartyIsolation.js:135
(Diff revision 5)
>    await BrowserTestUtils.browserLoaded(tab.linkedBrowser, true, function(url) {
>      return url == BASE_URL + "dummy.html";
>    });
>  
> -  await ContentTask.spawn(tab.linkedBrowser, {}, async function() {
> +  await ContentTask.spawn(tab.linkedBrowser, {shouldBeIsolated}, async function(attrs) {
> +    let assertFunc = function(b) { return attrs.shouldBeIsolated ? b : !b; };

Error: 'assertfunc' is assigned a value but never used. [eslint: no-unused-vars]

::: browser/components/originattributes/test/browser/browser_firstPartyIsolation.js:308
(Diff revision 5)
> +  await openWindow_test(gBrowser, true);
> +  await window_open_redirect_test(gBrowser, true);
> +  await window_open_iframe_test(gBrowser, true);
> +  await form_test(gBrowser, true);
> +  await window_open_form_test(gBrowser, true);
>  });

Error: Newline required at end of file but not found. [eslint: eol-last]

Comment 42

a year ago
mozreview-review-reply
Comment on attachment 8963460 [details]
Bug 1397624 Add a new privacy.firstparty.isolate.pbmode.enabled pref to enable FPI in PBM only

https://reviewboard.mozilla.org/r/232390/#review242102

> No; It's my understanding tha FPI doesn't wipe any cookies. When you first enable FPI, you basically switch how you query the cookie jar, so there is nothing to be found when you start querying. When you turn FPI off, you go back to querying the old way, and you'll get all your original cookies. When you enable FPI again, you start querying int he FPI way, and you start getting all the cookies you had accumulated during the first time you had FPI enabled.
> 
> > you are presumably only comparing principals in private mode, so again FPI will be enabled in both.
> 
> I believe a comparison occurs between a principal from non-private mode and a principal from private mode. And therefore FPI may only be enabled in one of those principals.

The only time I can think of where we could compare two principals for equality from non-private mode and private mode and want the result to be true even when all origin attributes (ex: pbmode and fpi) doesn't match is for permissions and password manager.  I'm not sure if/how we share permissions across regular mode and private mode.  And I'm not sure how we fill or save passwords in private mode.

Anyway, I would say to err on the safer side (include the asserts, return false).  Do a few manual tests on passwords and permissions in regular/private mode when FPI is set to true in private mode.  And then catch any edge cases we haven't thought of in nightly when returning false causes an issue or an assert fails.

> s/principles as/principals has/

"principals has" instead of "principals as"
Comment on attachment 8963460 [details]
Bug 1397624 Add a new privacy.firstparty.isolate.pbmode.enabled pref to enable FPI in PBM only

Please get a second peer reviewer for this change, particularly js/xpconnect/wrappers/WrapperFactory.cpp.
Flags: needinfo?(tanvi)
Attachment #8963460 - Flags: review?(tanvi) → review+

Comment 44

a year ago
mozreview-review-reply
Comment on attachment 8963720 [details]
Bug 1397624 Remove privacy.firstparty.isolate.restrict_opener_access and act as if it were always enabled

https://reviewboard.mozilla.org/r/232616/#review242270

> Now, 'pref' refers to privacy.firstparty.isolate so I think this is sill accurate.

Okay, then please be more specific.  "If the pref privacy.firstparty.isolate is not enabled,..."

Comment 45

a year ago
mozreview-review
Comment on attachment 8963720 [details]
Bug 1397624 Remove privacy.firstparty.isolate.restrict_opener_access and act as if it were always enabled

https://reviewboard.mozilla.org/r/232616/#review244360

::: browser/components/originattributes/test/browser/browser_windowOpenerRestriction.js:2
(Diff revision 3)
>  /**
>   * Bug 1339336 - A test case for testing pref 'privacy.firstparty.isolate.restrict_opener_access'

Update the comment

::: browser/components/originattributes/test/browser/file_windowOpenerRestrictionTarget.html:13
(Diff revision 3)
> -    // Therefore, it should fail at this setting.
>      let openerRestriction = true;
>      let cookieValue;
>      if (window.location.search.length > 0) {
>        cookieValue = window.location.search.substr(1);
>        openerRestriction = false;

Why do we set the openerRestriction to false here? Do we need to bring back the previous comment about when the query string is given?

This test needs a little more explanation.
Comment on attachment 8963720 [details]
Bug 1397624 Remove privacy.firstparty.isolate.restrict_opener_access and act as if it were always enabled

Hi Tom, please respond to the comments and then reflag.  Thanks!
Attachment #8963720 - Flags: review?(tanvi)

Comment 47

7 months ago
I am a bit curious now: A few days before with Firefox 62.0.3 and permanent private browsing mode I tested the functionality of first party isolation via the same embedded YouTube video on different websites by changing its volume and checking if the setting is remembered across websites with privacy.firstparty.isolate set to false and true.

If privacy.firstparty.isolate was set to false the volume was remembered across different websites while I could never reproduce this with privacy.firstparty.isolate set to true (the volume was always on its default setting).


Then I did read somewhere that first party isolation does not work with private browsing and found this bug report. But my observations conflict with this. Did Firefox get some other changes meanwhile that improved this behavior? Or does first party isolation just work partially with the current Firefox implementation or is it eventually even just more aggressive than in non - private browsing?
(Assignee)

Comment 48

7 months ago
(In reply to sworddragon2 from comment #47)
> Then I did read somewhere that first party isolation does not work with
> private browsing and found this bug report. But my observations conflict
> with this. Did Firefox get some other changes meanwhile that improved this
> behavior? Or does first party isolation just work partially with the current
> Firefox implementation or is it eventually even just more aggressive than in
> non - private browsing?

FPI should behave the same way between Private Browsing and not. Private Browsing has no effect on FPI (right now). (It will have no existing state when you open it; and it wipe your cookies when you close a private window of course.) You see a strange behavior with this; feel free to file a new bug and cc me.
(Assignee)

Updated

5 months ago
See Also: → 1450398
(Assignee)

Updated

5 months ago
See Also: → 1404017
You need to log in before you can comment on or make changes to this bug.