Closed Bug 1260931 Opened 8 years ago Closed 8 years ago

Add 1st party isolation pref and OriginAttribute.

Categories

(Core :: DOM: Security, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: huseby, Assigned: allstars.chh)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [tor], [domsecurity-active][ETA 9/12][tor 13742])

Attachments

(6 files, 28 obsolete files)

1.22 KB, patch
smaug
: review+
Details | Diff | Splinter Review
12.38 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
16.99 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
2.70 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
20.84 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
9.92 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
This bug is to add the 1st party isolation pref and the firstPartyURI origin attributes.

The origin attribute should only be present and passed along when the 1st party isolation pref is set to true.
Whiteboard: [tor] → [tor], [domsecurity-active]
Attached patch Bug_1260931.patch (obsolete) — — Splinter Review
WIP patch to add pref and origin attributes and unit tests.
Assignee: huseby → allstars.chh
Whiteboard: [tor], [domsecurity-active] → [tor], [domsecurity-active],[OA]
Status: ASSIGNED → NEW
Yoshi,

This bug is a prerequisite for a lot of the isolation tests from the tor browser.  I'd like to take over this bug and land the patch ASAP.

--dave
Flags: needinfo?(allstars.chh)
Assignee: allstars.chh → huseby
Status: NEW → ASSIGNED
feel free to :)
Flags: needinfo?(allstars.chh)
Attached patch Bug_1260931.patch (obsolete) — — Splinter Review
rebased and updated patch.
Attachment #8736525 - Attachment is obsolete: true
(In reply to Dave Huseby [:huseby] from comment #4)
> Created attachment 8762690 [details] [diff] [review]
> Bug_1260931.patch
> 
> rebased and updated patch.

I notice this attribute is called `firstPartyUri`. In Tor Browser, we have generally called it a `firstPartyHost`, because it is only the domain and not the full URI of the main content page. Also, the Tor Browser policy has been to not isolate subdomains, so that, for example, "www.google.com", "mail.google.com" and "google.com" are all together in the same "silo". I suppose `firstParty` might be a good name as well.
(In reply to Arthur Edelstein from comment #6)
> (In reply to Dave Huseby [:huseby] from comment #4)
> > Created attachment 8762690 [details] [diff] [review]
> > Bug_1260931.patch
> > 
> > rebased and updated patch.
> 
> I notice this attribute is called `firstPartyUri`. In Tor Browser, we have
> generally called it a `firstPartyHost`, because it is only the domain and
> not the full URI of the main content page. Also, the Tor Browser policy has
> been to not isolate subdomains, so that, for example, "www.google.com",
> "mail.google.com" and "google.com" are all together in the same "silo". I
> suppose `firstParty` might be a good name as well.

Ok thanks!  I'll change the name while I can.  And yes, the plan is to use the nsIURI domain accessor to just get the main domain.  I'll look at the Tor ThirdPartyUtils patch to see how you handle subdomains and add that.
dumping notes in here.  i will be using the nsIEffectiveTLDService::GetBaseDomain to get the correct value to assign to the origin attribute.

https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/nsIEffectiveTLDService.idl#39
Summary: Add 1st party isolation pref and origin attribute. → Add 1st party isolation pref and OriginAttribute.
Priority: -- → P1
Whiteboard: [tor], [domsecurity-active],[OA] → [tor], [domsecurity-active]
This is the latest WIP patch for adding and populating the firstPartyDomain origin attribute.  After running many experiments, I think this is the bare minimum change to get this origin attribute populated correctly when doing a top-level load.

The difficulty in this patch comes from properly detecting a first party load that is an actual web page and not some resource or some other kind of page (e.g. about:).

I'm 90% certain this isn't the only place the origin attribute will need to get set but this is good enough for us to move ahead on merging the tor isolation tests.  I'm happy I was able to distill this change down to just mostly the 10 line change in nsDocShell::InternalLoad.
Attachment #8762690 - Attachment is obsolete: true
Attachment #8773922 - Flags: feedback?(jonas)
Comment on attachment 8773922 [details] [diff] [review]
updated WIP to add and populate firstPartyDomain origin attribute.

>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>@@ -9843,16 +9844,52 @@ nsDocShell::InternalLoad(nsIURI* aURI,
>   if (NS_FAILED(rv) || NS_CP_REJECTED(shouldLoad)) {
>     if (NS_SUCCEEDED(rv) && shouldLoad == nsIContentPolicy::REJECT_TYPE) {
>       return NS_ERROR_CONTENT_BLOCKED_SHOW_ALT;
>     }
> 
>     return NS_ERROR_CONTENT_BLOCKED;
>   }
> 
>+  // XXXhuseby: sandboxing to first party domain starts here.  we want to only
>+  // set the firstPartyDomain origin attribute when the first party isolation
>+  // pref is set AND the disallow inherit owner flag is false AND the first
>+  // party boolean is true.
>+  //
>+  // when that happens we can be sure that aURI contains the URI for the top-
>+  // level, first party page we are loading.  we need to use the effective tld
>+  // service to extract the base domain from the URI to use as the value for
>+  // the origin attribute.
>+  //
>+  // the other complication is that we only have the loading principal here and
>+  // there's no way to directly set an origin attribute value on an existing
>+  // principal.  so we create a new loading principal with the modified origin
>+  // attributes.
>+  if (Preferences::GetBool("privacy.firstparty.isolate", false) &&
>+      (aFlags & LOAD_FLAGS_DISALLOW_INHERIT_OWNER) == false &&
>+      aFirstParty && aURI && loadingPrincipal)
>+  {

* How is aFirstParty set?  If we attempt to load a foo.com frame in a top level foo.com document, will aFirstParty be true for the frame since it is the same domain as the top level document?  I'm wondering if aFirstParty is the right thing to check here, or if you should check contentType == TYPE_DOCUMENT.

* Is the firstparty attribute only set if the pref is on?  We may consider setting it regardless of the pref and checking/using it for security decisions only when the pref is set.  This way, if users turn the pref on and off, they can still site data that is already stored on their browser.  Ex: without the pref set, we still mark things with the firstparty domain origin attribute; When the user turns on first party isolation, they can still benefit from data previously stored while the pref was disabled.  Something to consider.

* What happens if loadingPrincipal isn't set (i.e. no owner and no referrer)?  There are cases like this, and Christoph is trying to eliminate them, starting with bug 1181370.

>+    // get the base domain from the URI we're loading.
>+    nsCOMPtr<nsIEffectiveTLDService> tldService =
>+      do_GetService(NS_EFFECTIVETLDSERVICE_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    
>+    nsAutoCString baseDomain;
>+    rv = tldService->GetBaseDomain(aURI, 0, baseDomain);
>+    if (NS_SUCCEEDED(rv)) {
>+#if defined(DEBUG)
>+      printf_stderr("############ Setting firstPartyDomain = %s\n", baseDomain.Data());
>+#endif
>+      // populate the origin attributes in the loading principal.
>+      PrincipalOriginAttributes attrs = BasePrincipal::Cast(loadingPrincipal)->OriginAttributesRef();
>+      CopyUTF8toUTF16(baseDomain, attrs.mFirstPartyDomain);
>+      loadingPrincipal = BasePrincipal::CreateCodebasePrincipal(aURI, attrs);

How do you know that aURI is the same as the loadingPrincipal already defined (which came from the owner or referrer, rather than the aURI being loaded right now)?  I'm not sure if this really sets the firstparty attribute, since the loadingPrincipal here is not used in the rest of InternalLoad.  Part of the confusion here is that this is not loadInfo->loadingPrincipal.  That is set in nsDocShell::DoURILoad().  (I actually tried to rename this loadingPrincipal to requestingPrincipal to avoid confusion but that work hasn't landed yet - bug 1264137).

>+    }
>+  }
>+
(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #10)
> * How is aFirstParty set?  If we attempt to load a foo.com frame in a top
> level foo.com document, will aFirstParty be true for the frame since it is
> the same domain as the top level document?  I'm wondering if aFirstParty is
> the right thing to check here, or if you should check contentType ==
> TYPE_DOCUMENT.

Checking contentType == TYPE_DOCUMENT might be the correct thing to do here.  I do know that aFirstParty is true for the top level document and false for a loaded frame even if the frame is the same domain as the first party domain.  I have a test page at https://blog.linuxprogrammer.org/test.html which frames https://blog.linuxprogrammer.org/test2.html which frames http://w3schools.org that I used to confirm this.

> * Is the firstparty attribute only set if the pref is on?  We may consider
> setting it regardless of the pref and checking/using it for security
> decisions only when the pref is set.  This way, if users turn the pref on
> and off, they can still site data that is already stored on their browser. 
> Ex: without the pref set, we still mark things with the firstparty domain
> origin attribute; When the user turns on first party isolation, they can
> still benefit from data previously stored while the pref was disabled. 
> Something to consider.

The firstPartyDomain attribute is only set if the pref is set to true.  The problem with setting it always is that it breaks the web.  This double key's cookies and everything else.  We can't have it on by default.  The only way we could set it when the pref is off is if we set a default value like "globalcontext" or something.  But then that's identical to not setting it.

> * What happens if loadingPrincipal isn't set (i.e. no owner and no
> referrer)?  There are cases like this, and Christoph is trying to eliminate
> them, starting with bug 1181370.

Hrm...I explicitly check for a loadingPrincipal and nothing happens if it isn't there.  The only times I've seen it be null is when we're loading resource: and about: URI's.  There are probably corner cases I'm missing and it is good that we're having this discussion.  I'll look at 1181370 and talk to Christoph about it.

> How do you know that aURI is the same as the loadingPrincipal already
> defined (which came from the owner or referrer, rather than the aURI being
> loaded right now)?  I'm not sure if this really sets the firstparty
> attribute, since the loadingPrincipal here is not used in the rest of
> InternalLoad.  Part of the confusion here is that this is not
> loadInfo->loadingPrincipal.  That is set in nsDocShell::DoURILoad().  (I
> actually tried to rename this loadingPrincipal to requestingPrincipal to
> avoid confusion but that work hasn't landed yet - bug 1264137).

ooooh, good catch.  so what you're saying is I need to set the loadInfo->loadingPrincipal on the docshell?
Flags: needinfo?(tanvi)
(In reply to Dave Huseby [:huseby] from comment #11)
> (In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #10)
> > * How is aFirstParty set?  If we attempt to load a foo.com frame in a top
> > level foo.com document, will aFirstParty be true for the frame since it is
> > the same domain as the top level document?  I'm wondering if aFirstParty is
> > the right thing to check here, or if you should check contentType ==
> > TYPE_DOCUMENT.
> 
> Checking contentType == TYPE_DOCUMENT might be the correct thing to do here.
> I do know that aFirstParty is true for the top level document and false for
> a loaded frame even if the frame is the same domain as the first party
> domain.  I have a test page at https://blog.linuxprogrammer.org/test.html
> which frames https://blog.linuxprogrammer.org/test2.html which frames
> http://w3schools.org that I used to confirm this.

I would dig into the code to see how aFirstParty is set and what it is used for.  I would also add an assert locally to make sure.  something like...
Assert(!(contentType==TYPE_DOCUMENT && !aFirstParty) && !(contentType!=TYPE_DOCUMENT && aFirstParty))
You may be able to simplify that ;)


> > How do you know that aURI is the same as the loadingPrincipal already
> > defined (which came from the owner or referrer, rather than the aURI being
> > loaded right now)?  I'm not sure if this really sets the firstparty
> > attribute, since the loadingPrincipal here is not used in the rest of
> > InternalLoad.  Part of the confusion here is that this is not
> > loadInfo->loadingPrincipal.  That is set in nsDocShell::DoURILoad().  (I
> > actually tried to rename this loadingPrincipal to requestingPrincipal to
> > avoid confusion but that work hasn't landed yet - bug 1264137).
> 
> ooooh, good catch.  so what you're saying is I need to set the
> loadInfo->loadingPrincipal on the docshell?

You may need to make your changes in DoURILoad() using the principals and originAttributes there instead.
Flags: needinfo?(tanvi)
http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10754

You should set the first party domain attribute around here instead.

Note that for TYPE_DOCUMENT loads, we don't have a loadingPrincipal (and we don't want one by design).  The loadInfo for the channel sets the OriginAttributes by looking at the docshell associated with the OuterWindow - http://searchfox.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#236.  So what you need to do is set the firstPartyDomain OriginAttribute on the docshell before the LoadInfo is created.  And then the channel should know about it via loadInfo->GetOriginAttributes().
Comment on attachment 8773922 [details] [diff] [review]
updated WIP to add and populate firstPartyDomain origin attribute.

Review of attachment 8773922 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDocShell.cpp
@@ +9880,5 @@
> +#endif
> +      // populate the origin attributes in the loading principal.
> +      PrincipalOriginAttributes attrs = BasePrincipal::Cast(loadingPrincipal)->OriginAttributesRef();
> +      CopyUTF8toUTF16(baseDomain, attrs.mFirstPartyDomain);
> +      loadingPrincipal = BasePrincipal::CreateCodebasePrincipal(aURI, attrs);

I don't think changing the loadingPrincipal will change what cookies are used for the request. The cookie code currently just looks at the nsILoadContext (which is implemented by nsDocShell) in order to get the OriginAttributes.

Have you tested that this actually changes which cookie jar is used for any requests?
Attachment #8773922 - Flags: feedback?(jonas) → feedback-
Alright, investigating where the aFirstParty flag comes from gave some interesting results.  

The first caller is nsDocShell::LoadURIWithOptions() which always calls LoadURI with the first party boolean set to true:

https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#4789

the second case is nsDocShell::ForceRefreshURI which always calls LoadURI with the first party boolean set to true:

https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#6774

the third case is nsSHistory::InitiateLoad which always calls it with it set to false:

https://dxr.mozilla.org/mozilla-central/source/docshell/shistory/nsSHistory.cpp#1784

the fourth is nsFrameLoader::ReallyStartLoadingInternal which calls it with it set to false:

https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#491

the fifth is nsLocation::SetURI which calls it with it set to true:

https://dxr.mozilla.org/mozilla-central/source/dom/base/nsLocation.cpp#273

the sixth is ClientNavigateRunnable::Navigate which calls it with it set to true:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerWindowClient.cpp#483

the last one is nsWindowWatcher::OpenWindowInternal which calls it with it set to true:

https://dxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/nsWindowWatcher.cpp#1059

so i'm not sure if it is a reliable indicator of a top-level load anymore.  hrm...that's what i was afraid of.  maybe all i need to do is look for the DOCUMENT content policy value.
Flags: needinfo?(tanvi)
Tanvi,

The assert failure is coming from nsFrameLoader::ReallyStartLoadingInternal calling nsDocShell::LoadURI with first party set to false.  But this is during startup, the assert failure happens when we load "about:blank".
So now the next test is to see if testing for (aContentPolicyType == nsIContentPolicy::TYPE_DOCUMENT) is enough to spot top level, first party loads.
Attached patch updated WIP (obsolete) — — Splinter Review
Jonas,

I moved the code to DoURILoad, just before the load info is created after Tanvi pointed out that I was setting the origin attribute in the incorrect place.

I also investigated the aFirstParty flag more and realized that it isn't a reliable indicator for all first party loads because we get some cases where the content policy is TYPE_DOCUMENT but first party is false (e.g. loading 'about:blank').  So I changed the test for when to set the origin attribute to just be the test for TYPE_DOCUMENT.

Running a debug build indicates that this patch works.  I'm seeing the base domain being set and it being propagated through the Inherit* functions in the origin attribute classes.

Do you see anything else that is wrong with this?

--dave
Attachment #8773922 - Attachment is obsolete: true
Attachment #8773983 - Flags: feedback?(jonas)
The problem is that Necko *only* looks at the nsILoadContext to determine which OriginAttributes to use. So even though you are probably propagating the OriginAttributes into the various principals and nsILoadInfo objects involved, that doesn't matter at all to Necko.

I feel confident that if you actually test which cookies are hitting the wire, you'll see that no separation is happening.
I don't think the approach in the try patch is a good idea. The problem is that you're keeping state in docshell and trying to update it any time a navigation happens. This is *really* hard to do correctly since state management during navigation is really complex.

In particular when the user clicks a link to start a navigation, we fire off the network request for the new HTML document while still showing and running the current document in the docshell.

So for a short period of time, the docshell is managing two documents. One which is currently being displayed, and one which we are downloading/parsing until it gets to the state where we have pixels to show.

During this time we might even download sub-resources of the new document since we don't switch to display the new document until we hit the <body> tag.

The other problem is that there are multiple different code paths that are hit when the user navigate back/forward. Some of which don't actually fire off any network requests. And this is true even when the user navigates between different domains.

In short, the OriginAttributes in the docshell should stay from the point where the docshell has been "fully created", until it is deleted. Trying to keep the OriginAttributes in sync during navigations will lead to insanity.
(In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #21)
> I don't think the approach in the try patch is a good idea. The problem is
> that you're keeping state in docshell and trying to update it any time a
> navigation happens. This is *really* hard to do correctly since state
> management during navigation is really complex.
> 
> In particular when the user clicks a link to start a navigation, we fire off
> the network request for the new HTML document while still showing and
> running the current document in the docshell.

Isn't this handled by the PrincipalOriginAttributes::InheritFromDocShellToDoc and NeckoOriginAttributes::InheritFromDocShellToNecko?

What do you see as the right way to do this?  I'm thinking that our only option is to track down all of the code paths and make sure the origin attribute gets set up correctly.  But as you said, that "will lead to insanity."  I'm hoping there is a central place or two where we can just add the origin attribute and the inherit will take care of the rest.
Flags: needinfo?(jonas)
> Isn't this handled by the
> PrincipalOriginAttributes::InheritFromDocShellToDoc and
> NeckoOriginAttributes::InheritFromDocShellToNecko?

I'm not sure which part you are hoping is handled by these functions. All they do is to modify OriginAttributes structs. The problem you are having is that the cookie code is currently looking at the struct in the DocShell. And updating the struct in the DocShell such that it always contains the correct value is more or less impossible.

> What do you see as the right way to do this?  I'm thinking that our only
> option is to track down all of the code paths and make sure the origin
> attribute gets set up correctly.  But as you said, that "will lead to
> insanity."  I'm hoping there is a central place or two where we can just add
> the origin attribute and the inherit will take care of the rest.

The right way to do this is to make the cookie code not look at the OriginAttributes in the nsILoadContext (i.e. the docshell), but rather look at the OriginAttributes in the nsILoadInfo.
Flags: needinfo?(jonas)
Attached patch updated WIP (obsolete) — — Splinter Review
I changed the script security manager to propagate the loadInfo origin attributes for docs and sub-docs only when the pref is enabled. the pref gate is necessary until we fix the loadInfo/loadingContext problem.  This WIP should be good enough for now.  I think the isolation tests will highlight all of the ways this isn't complete and we can fix it as we go.
Attachment #8773983 - Attachment is obsolete: true
Attachment #8776196 - Flags: feedback?(jonas)
Comment on attachment 8776196 [details] [diff] [review]
updated WIP

Review of attachment 8776196 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good in general, minus a couple of issues below.

Obviously this will only fully work once we switch the cookie code to use the LoadInfo OAs, rather than the LoadContext OAs.

::: caps/nsScriptSecurityManager.cpp
@@ +460,5 @@
> +      // make sure we pull origin attributes from loadInfo if isolation is
> +      // enabled and we have a valid loadInfo
> +      if (loadInfo) {
> +        if (Preferences::GetBool("privacy.firstparty.isolate", false)) {
> +          attrs.InheritFromNecko(loadInfo->GetOriginAttributes());

We should make this change independent of the "privacy.firstparty.isolate" pref. I.e. this is just a bug in our OA support which should be fixed, independent of if first-party isolation is enabled or not.

Also, this should be done *instead of* inheriting from the loadContext, not in addition to.

You'll likely want to do this in a separate bug.

::: docshell/base/nsDocShell.cpp
@@ +10860,1 @@
>    rv = loadInfo->SetOriginAttributes(neckoAttrs);

We need to also update these attributes in case of a cross-origin redirect.

Note that for each redirect, a new nsIChannel is created, and given a new nsILoadInfo. We should only update the OAs in the new loadInfo.

I would assume that we have a redirect listener already somewhere. So you can probably hook into that. Most likely [1] is where you'll want to hook in. However I'm *really* not sure how this part of document loading works, so you should verify that.

[1] https://dxr.mozilla.org/mozilla-central/source/uriloader/base/nsDocLoader.cpp#1366
Attachment #8776196 - Flags: feedback?(jonas)
Depends on: 1264231
Attached patch updated WIP (obsolete) — — Splinter Review
this is an updated WIP patch for filling in the firstPartyDomain origin attribute.  i updated to also update the loadInfo during a redirect. this patch doesn't work very well, the inheritence of the firstPartyDomain origin attribute doesn't work, but I suspect that's because we're still switching over to loadInfo being the source of truth.  anyway, this doesn't crash and it's good enough for us to move on to uplifting the tor isolation tests and use those as our guide to fix this up.
Attachment #8776196 - Attachment is obsolete: true
Attachment #8777585 - Flags: review?(bobbyholley)
Attachment #8777585 - Flags: feedback?(allstars.chh)
Attachment #8777585 - Flags: review?(bobbyholley) → review?(bzbarsky)
Comment on attachment 8777585 [details] [diff] [review]
updated WIP

r- because this needs a commit message explaining what the patch is changing and why and so forth.

That would incidentally make it possible to actually review this, instead of trying to reverse-engineer it....
Attachment #8777585 - Flags: review?(bzbarsky) → review-
Is my understanding correct that the goal of this feature is compartmentalizing 3rd-party loads based on the domain of the top level document?

E.g. a youtube iframe wouldn't get the same cookies as browsing to youtube.com via the url bar?
(In reply to The 8472 from comment #28)
> Is my understanding correct that the goal of this feature is
> compartmentalizing 3rd-party loads based on the domain of the top level
> document?
> 
> E.g. a youtube iframe wouldn't get the same cookies as browsing to
> youtube.com via the url bar?

A youtube iframe wouldn't get the same cookies as browsing to youtube.com via the url bar if that iframe's top level document's origin is not youtube.com.
Excellent, I have been looking for something like this for a long time. OriginAttributes documentation is non-existent, so writing an addon to do the same looks like a difficult task.

From a usability perspective the feature a bit inflexible though. The domain scoping is too strict and too lax at the same time. E.g. putting gmail and google search into the same silo as mentioned in an earlier comment does not make sense to me. Google mail does not need to know about my search habits.
But conversely there are silos that span multiple top level domains, e.g. the stack exchange network.

How difficult would it be for rule-based addons like noscript or µMatrix to hook into the first-party decision process?
Comment on attachment 8777585 [details] [diff] [review]
updated WIP

Review of attachment 8777585 [details] [diff] [review]:
-----------------------------------------------------------------

Adding firstPartyDomain into OriginAttributes looks okay.

However I think you didn't address Jonas' comment 21 yet, the origin attributes of the docshell should stay the same.

I don't know enough for the redirect part so I can't comment on that
Attachment #8777585 - Flags: feedback?(allstars.chh)
Removing my needinfo from comment 15 since Jonas discussed with Dave.  Please reflag me if you there are additional questions.
Flags: needinfo?(tanvi)
Whiteboard: [tor], [domsecurity-active] → [tor], [domsecurity-active][ETA 9/12]
Per discussion in the Tor integration meeting, Yoshi is taking over this bug.
Assignee: huseby → allstars.chh
Right now we have some problems to consider.

1. Should the top-level document have the firstPartyDomain origin attribute set?
Yes: but we will lose data-jars.
No: after turn on the prefs, tracking site could still track users by the old cookies.

2. if a webpage iframes itself, for example, a.com has a inner iframe which is also from a.com. Should we still isolate them?

See discussions from
https://public.etherpad-mozilla.org/p/firstPartyIsolation
Attached patch Part 1: add firstPartyDomain (obsolete) — — Splinter Review
Attachment #8777585 - Attachment is obsolete: true
I still have problems when running tests, and the reason is I need to turn on the pref before running the test, otherwise the firstPartyDomain won't be appended into top-level document. 

Same reason for the inner iframe, it cannot acquire the firstPartyDomain from its parent document through its docshell.
kanru suggested me to use browser test then I can load the top-level document after the pref is on. And it did work.
Attachment #8780478 - Attachment is obsolete: true
Attached patch Part 1: add firstPartyDomain (obsolete) — — Splinter Review
added commit message.
Attachment #8780476 - Attachment is obsolete: true
updated test with iframes, and also added commit message.
Attachment #8780490 - Attachment is obsolete: true
Comment on attachment 8781504 [details] [diff] [review]
Part 1: add firstPartyDomain

Review of attachment 8781504 [details] [diff] [review]:
-----------------------------------------------------------------

(I found bz is taking PTO now, so redirect the r? to smaug)
Hi Smaug,

We'd like to add another origin attribute called 'firstPartyDomain', the idea is from Tor's 'First party isolation'
https://www.torproject.org/projects/torbrowser/design/#identifier-linkability

I've added some notes in the commit message for the detail.

could you review this for me ?

Thanks
Attachment #8781504 - Flags: review?(bugs)
Comment on attachment 8780477 [details] [diff] [review]
Part 2: add pref privacy.firstparty.isolate

Review of attachment 8780477 [details] [diff] [review]:
-----------------------------------------------------------------

Pref, it is default off in Firefox.

When we turn on the pref, all tabs shold be reloaded again since we should update the principal,
however IMO it should be a seperate bug if you agree.

Thanks
Attachment #8780477 - Flags: review?(bugs)
Comment on attachment 8780477 [details] [diff] [review]
Part 2: add pref privacy.firstparty.isolate

I don't yet know what this is for, but perhaps the other patch reveals that.
Attachment #8780477 - Flags: review?(bugs) → review+
ok, I don't yet quite understand what this is about, so need to read (and understand) tor documentation.
Comment on attachment 8781504 [details] [diff] [review]
Part 1: add firstPartyDomain

though, this doesn't really use the new OA property, so I guess it is part 3 which is the tricky one.
Attachment #8781504 - Flags: review?(bugs) → review+
Comment on attachment 8781505 [details] [diff] [review]
Part 3: propagate the firstPartyDomain attribute.

Review of attachment 8781505 [details] [diff] [review]:
-----------------------------------------------------------------

(Oops, I just realized I didn't send r? yesterday because originally I've flagged r? to Jonas but he doesn't accept a r? now)
Hi Smaug

I've added some notes in the commit message, but not sure why it doesn't show up in spliter review.

So the point is, I didn't set firstPartyDomain on the top-level docshell, but on the top-level document.
And the reason I didn't do this on docshell is we don't want the origin attributes of the docshell to change all the time user navigates.
See Jonas' comment 21.

But I did this on the top-level document, although users might lose cookies, but it's the idea of this feature, and we don't want users are still being tracked by the old cookies.

And then when this top-level document creates any iframe, it will propagate the firstPartyDomain to the child docshell. which in turn will pass the attribute to the child document, and so on.

I also have another test for cookie, but it depends on the bug 1291652, so I didn't attach here. And the reason is that given the top-level docshell doesn't have firstPartyDomain, so neither does the loadContext, but loadInfo does, so right now the cookie-jar of the top-level is not isolated.

One problem for my patch is that now we could have mismatch of firstPartyDomain between loadContext and loadInfo, the reason is mentioned above.
But I am not sure if this is a problem, because Jonas mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1209162#c12
The docshell should only provide enough information to create the origin attributes of the document, that means firstPartyDomain doesn't act like appId/mozbrowser flag, should be always inherited to the child docshell/document.

Also I am not sure about SerializedLoadContext, either. I thought Bug 1125916 would have replaced SerializedLoadContext but it didn't, so for the IPC part we still use the information from LoadContext, not sure if this the problem we need to look into first.

Anyway, I'd like to request for your suggestions on this, feel free to provide any suggestion if you think it doesn't make sense.
Thanks
Attachment #8781505 - Flags: review?(bugs)
(In reply to Yoshi Huang[:allstars.chh] from comment #46)
 
> I've added some notes in the commit message, but not sure why it doesn't
> show up in spliter review.
FWIW, I never use splinter. I use "Details" link for reviewing whenever possible.
Oh, another question, how does this interact with A -> B -> A redirects?
Comment on attachment 8781505 [details] [diff] [review]
Part 3: propagate the firstPartyDomain attribute.

Review of attachment 8781505 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/moz.build
@@ +11,5 @@
>      'dirprovider',
>      'downloads',
>      'extensions',
>      'feeds',
> +    'firstpartyisolation',

In https://bugzilla.mozilla.org/show_bug.cgi?id=1289319#c16, Tanvi suggested we could put it in browser/components/originattributes, so next time I'll rename the folder.
Attached patch Part 3: NeckoParent change (obsolete) — — Splinter Review
Because GetValidatedAppInfo only compares top-level OriginAttributes, so for iframe in the content side, it will lose the firstPartyDomain attributes after IPC.
I moved the check of  "if (!UsingNeckoIPCSecurity())" to the front base on the patch from bug 1029352, should help the code easier to read.

I guess it should also be reviewed be a Necko peer so I split it into a seperate part, but I'll let smaug review this first.
Attachment #8783406 - Flags: review?(bugs)
This is the update version of previous Part 3 patch, I added cookie test and redirect test this time.
Attachment #8781505 - Attachment is obsolete: true
Attachment #8781505 - Flags: review?(bugs)
Attachment #8783409 - Flags: review?(bugs)
It may take still couple of days before I get to this, since I don't yet quite understand what the feature is supposed to do.
Comment on attachment 8783406 [details] [diff] [review]
Part 3: NeckoParent change

>+    if (aSerialized.mOriginAttributes.mAppId != tabContext.OriginAttributesRef().mAppId) {
>+      continue;
>+    }
I don't understand what this change has to do with this bug.

>+    if (aSerialized.mOriginAttributes.mInIsolatedMozBrowser != tabContext.OriginAttributesRef().mInIsolatedMozBrowser) {
>+      continue;
>+    }
or this


Could we limit the changes in this bug to the feature this bug is adding. (or perhaps I'm misunderstand some changes)
Attachment #8783406 - Flags: review?(bugs) → review-
Comment on attachment 8783409 [details] [diff] [review]
Part 4: propagate the firstPartyDomain attribute.

>From c6bef5c64413660b23529d882d15ac630243d274 Mon Sep 17 00:00:00 2001
>From: Yoshi Huang <allstars.chh@mozilla.com>
>Date: Fri, 12 Aug 2016 18:38:34 +0800
>Subject: Bug 1260931 - Part 4: propagate the firstPartyDomain attribute.
>
>Given that the firstPartyDomain is extracted from the URL bar. So when
>the top-level docshell is created, it does *NOT* have the
>firstPartyDomain origin attribute. This is also because we don't want to
>change the origin attributes of the top-level docshell when user
>navigates to other page.
>See Jonas' comment from https://bugzilla.mozilla.org/show_bug.cgi?id=1260931#c21
>
>However we do want the top-level document has the firstPartyDomain set
>(although users may lose data-jars after they turn on the pref), so we
>compute the firstPartyDomain value when the load is for
>nsIContentPolicy::TYPE_DOCUMENT.
>
>And when the document tries to create an iframe, it will propagate the
>firstPartyDomain to its child docshell.
>---
> .../components/firstpartyisolation/test/test.html  | 12 ++++
> browser/components/moz.build                       |  1 +
> browser/components/originattributes/moz.build      |  8 +++
> .../components/originattributes/test/browser.ini   | 13 ++++
> .../test/browser_firstPartyIsolation.js            | 82 ++++++++++++++++++++++
> browser/components/originattributes/test/head.js   |  0
> browser/components/originattributes/test/test.html | 12 ++++
> browser/components/originattributes/test/test.js   |  1 +
> .../originattributes/test/test.js^headers^         |  1 +
> .../originattributes/test/test_firstParty.html     | 15 ++++
> .../test/test_firstParty_cookie.html               | 12 ++++
> .../test/test_firstParty_html_redirect.html        |  9 +++
> .../test/test_firstParty_http_redirect.html        |  9 +++
> .../test_firstParty_http_redirect.html^headers^    |  2 +
> caps/nsScriptSecurityManager.cpp                   | 12 ++++
> dom/base/nsFrameLoader.cpp                         |  8 +++
> 16 files changed, 197 insertions(+)
> create mode 100644 browser/components/firstpartyisolation/test/test.html
> create mode 100644 browser/components/originattributes/moz.build
> create mode 100644 browser/components/originattributes/test/browser.ini
> create mode 100644 browser/components/originattributes/test/browser_firstPartyIsolation.js
> create mode 100644 browser/components/originattributes/test/head.js
> create mode 100644 browser/components/originattributes/test/test.html
> create mode 100644 browser/components/originattributes/test/test.js
> create mode 100644 browser/components/originattributes/test/test.js^headers^
> create mode 100644 browser/components/originattributes/test/test_firstParty.html
> create mode 100644 browser/components/originattributes/test/test_firstParty_cookie.html
> create mode 100644 browser/components/originattributes/test/test_firstParty_html_redirect.html
> create mode 100644 browser/components/originattributes/test/test_firstParty_http_redirect.html
> create mode 100644 browser/components/originattributes/test/test_firstParty_http_redirect.html^headers^
>
>diff --git a/browser/components/firstpartyisolation/test/test.html b/browser/components/firstpartyisolation/test/test.html
>new file mode 100644
>index 0000000..3eb8293
>--- /dev/null
>+++ b/browser/components/firstpartyisolation/test/test.html
>@@ -0,0 +1,12 @@
>+<!DOCTYPE HTML>
>+<html>
>+<head>
>+  <meta charset="utf-8">
>+  <title>Test for Bug 1260931</title>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>+  <script src="test.js"></script>
>+</head>
>+<body>
>+  Hello World.
>+</body>
>+</html>
>diff --git a/browser/components/moz.build b/browser/components/moz.build
>index 818235c..824b26f 100644
>--- a/browser/components/moz.build
>+++ b/browser/components/moz.build
>@@ -9,16 +9,17 @@ DIRS += [
>     'contextualidentity',
>     'customizableui',
>     'dirprovider',
>     'downloads',
>     'extensions',
>     'feeds',
>     'migration',
>     'newtab',
>+    'originattributes',
>     'places',
>     'preferences',
>     'privatebrowsing',
>     'search',
>     'sessionstore',
>     'shell',
>     'selfsupport',
>     'syncedtabs',
>diff --git a/browser/components/originattributes/moz.build b/browser/components/originattributes/moz.build
>new file mode 100644
>index 0000000..180b081
>--- /dev/null
>+++ b/browser/components/originattributes/moz.build
>@@ -0,0 +1,8 @@
>+# -*- Mode: python; indent-tabs-mode: nil; tab-width: 40 -*-
>+# vim: set filetype=python:
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this
>+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
>+
>+BROWSER_CHROME_MANIFESTS += ['test/browser.ini']
>+
>diff --git a/browser/components/originattributes/test/browser.ini b/browser/components/originattributes/test/browser.ini
>new file mode 100644
>index 0000000..c2784a2
>--- /dev/null
>+++ b/browser/components/originattributes/test/browser.ini
>@@ -0,0 +1,13 @@
>+[DEFAULT]
>+
>+support-files =
>+  test.js
>+  test.js^headers^
>+  test.html
>+  test_firstParty.html
>+  test_firstParty_cookie.html
>+  test_firstParty_html_redirect.html
>+  test_firstParty_http_redirect.html
>+  test_firstParty_http_redirect.html^headers^
>+
>+[browser_firstPartyIsolation.js]
>diff --git a/browser/components/originattributes/test/browser_firstPartyIsolation.js b/browser/components/originattributes/test/browser_firstPartyIsolation.js
>new file mode 100644
>index 0000000..ed207e3
>--- /dev/null
>+++ b/browser/components/originattributes/test/browser_firstPartyIsolation.js
>@@ -0,0 +1,82 @@
>+const BASE_URL = "http://mochi.test:8888/browser/browser/components/originattributes/test/";
>+const BASE_DOMAIN = "mochi.test";
>+
>+add_task(function* setup() {
>+  Services.prefs.setBoolPref("privacy.firstparty.isolate", true);
>+  registerCleanupFunction(function () {
>+    Services.prefs.clearUserPref("privacy.firstparty.isolate");
>+  });
>+});
>+
>+add_task(function* test() {
>+  let tab = gBrowser.addTab(BASE_URL + "test_firstParty.html");
>+  yield BrowserTestUtils.browserLoaded(tab.linkedBrowser, true, function (url) {
>+    return url == BASE_URL + "test_firstParty.html";
>+  });
>+
>+  yield ContentTask.spawn(tab.linkedBrowser, { firstPartyDomain: BASE_DOMAIN }, function* (attrs) {
>+    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,
>+                 attrs.firstPartyDomain, "The document should have firstPartyDomain");
>+
>+    for (let i = 1; i < 4; i++) {
>+      let iframe = content.document.getElementById("iframe" + i);
>+      info("iframe principal: " + iframe.contentDocument.nodePrincipal.origin);
>+      Assert.equal(iframe.frameLoader.docShell.getOriginAttributes().firstPartyDomain,
>+                   attrs.firstPartyDomain, "iframe's docshell should have firstPartyDomain");
>+      Assert.equal(iframe.contentDocument.nodePrincipal.originAttributes.firstPartyDomain,
>+                   attrs.firstPartyDomain, "iframe should have firstPartyDomain");
>+    }
>+  });
>+
>+  gBrowser.removeTab(tab);
>+});
>+
>+add_task(function* test() {
>+  let tab = gBrowser.addTab(BASE_URL + "test_firstParty_cookie.html");
>+  yield BrowserTestUtils.browserLoaded(tab.linkedBrowser, true);
>+
>+  let iter = Services.cookies.enumerator;
>+  while (iter.hasMoreElements()) {
>+    let cookie = iter.getNext().QueryInterface(Ci.nsICookie2);
>+    Assert.equal(cookie.name, "test", "Cookie name should be test");
>+    Assert.equal(cookie.value, "foo", "Cookie value should be foo");
>+    Assert.equal(cookie.originAttributes.firstPartyDomain, BASE_DOMAIN, "Cookie's origin attributes should be " + BASE_DOMAIN);
>+  }
>+
>+  gBrowser.removeTab(tab);
>+});
>+
>+add_task(function* redirect_test() {
>+  let tab = gBrowser.addTab(BASE_URL + "test_firstParty_http_redirect.html");
>+  yield BrowserTestUtils.browserLoaded(tab.linkedBrowser);
>+  yield ContentTask.spawn(tab.linkedBrowser, { firstPartyDomain: "example.com" }, function* (attrs) {
>+    info("document principal: " + content.document.nodePrincipal.origin);
>+    info("document uri: " + content.document.documentURI);
>+
>+    Assert.equal(content.document.documentURI, "http://example.com/",
>+                 "The page should have been redirected to http://example.com");
>+    Assert.equal(content.document.nodePrincipal.originAttributes.firstPartyDomain,
>+                 attrs.firstPartyDomain, "The document should have firstPartyDomain");
>+  });
>+
>+  // Since this is a HTML redirect, we wait until the final page is loaded.
>+  let tab2 = gBrowser.addTab(BASE_URL + "test_firstParty_html_redirect.html");
>+  yield BrowserTestUtils.browserLoaded(tab2.linkedBrowser, false, function(url) {
>+    return url == "http://example.com/";
>+  });
>+
>+  yield ContentTask.spawn(tab2.linkedBrowser, { firstPartyDomain: "example.com" }, function* (attrs) {
>+    info("2nd tab document principal: " + content.document.nodePrincipal.origin);
>+    info("2nd tab document uri: " + content.document.documentURI);
>+    Assert.equal(content.document.documentURI, "http://example.com/",
>+                 "The page should have been redirected to http://example.com");
>+    Assert.equal(content.document.nodePrincipal.originAttributes.firstPartyDomain,
>+                 attrs.firstPartyDomain, "The document should have firstPartyDomain");
>+  });
>+
>+  gBrowser.removeTab(tab);
>+  gBrowser.removeTab(tab2);
>+});
>diff --git a/browser/components/originattributes/test/head.js b/browser/components/originattributes/test/head.js
>new file mode 100644
>index 0000000..e69de29
>diff --git a/browser/components/originattributes/test/test.html b/browser/components/originattributes/test/test.html
>new file mode 100644
>index 0000000..3eb8293
>--- /dev/null
>+++ b/browser/components/originattributes/test/test.html
>@@ -0,0 +1,12 @@
>+<!DOCTYPE HTML>
>+<html>
>+<head>
>+  <meta charset="utf-8">
>+  <title>Test for Bug 1260931</title>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>+  <script src="test.js"></script>
>+</head>
>+<body>
>+  Hello World.
>+</body>
>+</html>
>diff --git a/browser/components/originattributes/test/test.js b/browser/components/originattributes/test/test.js
>new file mode 100644
>index 0000000..d290af9
>--- /dev/null
>+++ b/browser/components/originattributes/test/test.js
>@@ -0,0 +1 @@
>+var i = 1;
>diff --git a/browser/components/originattributes/test/test.js^headers^ b/browser/components/originattributes/test/test.js^headers^
>new file mode 100644
>index 0000000..881f5bf
>--- /dev/null
>+++ b/browser/components/originattributes/test/test.js^headers^
>@@ -0,0 +1 @@
>+Set-Cookie: test=foo
>diff --git a/browser/components/originattributes/test/test_firstParty.html b/browser/components/originattributes/test/test_firstParty.html
>new file mode 100644
>index 0000000..a90e01c
>--- /dev/null
>+++ b/browser/components/originattributes/test/test_firstParty.html
>@@ -0,0 +1,15 @@
>+<!DOCTYPE HTML>
>+<html>
>+<head>
>+  <meta charset="utf-8"/>
>+  <title>Test for Bug 1260931</title>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>+</head>
>+<body>
>+  <div>
>+    <iframe id="iframe1" src="http://example.com"></iframe>
>+    <iframe id="iframe2" sandbox="" src="http://example.com"></iframe>
>+    <iframe id="iframe3" sandbox="allow-same-origin" src="http://example.com"></iframe>
>+  </div>
>+</body>
>+</html>
>diff --git a/browser/components/originattributes/test/test_firstParty_cookie.html b/browser/components/originattributes/test/test_firstParty_cookie.html
>new file mode 100644
>index 0000000..766f206
>--- /dev/null
>+++ b/browser/components/originattributes/test/test_firstParty_cookie.html
>@@ -0,0 +1,12 @@
>+<!DOCTYPE HTML>
>+<html>
>+<head>
>+  <meta charset="utf-8">
>+  <title>Test for Bug 1260931</title>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>+</head>
>+<body>
>+  Hello World.
>+  <iframe id="iframe1" src="test.html"></iframe>
>+</body>
>+</html>
>diff --git a/browser/components/originattributes/test/test_firstParty_html_redirect.html b/browser/components/originattributes/test/test_firstParty_html_redirect.html
>new file mode 100644
>index 0000000..3c52d4f
>--- /dev/null
>+++ b/browser/components/originattributes/test/test_firstParty_html_redirect.html
>@@ -0,0 +1,9 @@
>+<!DOCTYPE HTML>
>+<html>
>+<head>
>+  <meta charset="utf8" http-equiv="refresh" content="0; url=http://example.com/"/>
>+  <title>Test for Bug 1260931</title>
>+</head>
>+<body>
>+</body>
>+</html>
>diff --git a/browser/components/originattributes/test/test_firstParty_http_redirect.html b/browser/components/originattributes/test/test_firstParty_http_redirect.html
>new file mode 100644
>index 0000000..7b794a0
>--- /dev/null
>+++ b/browser/components/originattributes/test/test_firstParty_http_redirect.html
>@@ -0,0 +1,9 @@
>+<!DOCTYPE HTML>
>+<html>
>+<head>
>+  <meta charset="utf-8"/>
>+  <title>Test for Bug 1260931</title>
>+</head>
>+<body>
>+</body>
>+</html>
>diff --git a/browser/components/originattributes/test/test_firstParty_http_redirect.html^headers^ b/browser/components/originattributes/test/test_firstParty_http_redirect.html^headers^
>new file mode 100644
>index 0000000..08f7fa8
>--- /dev/null
>+++ b/browser/components/originattributes/test/test_firstParty_http_redirect.html^headers^
>@@ -0,0 +1,2 @@
>+HTTP 302 Found
>+Location: http://example.com
>diff --git a/caps/nsScriptSecurityManager.cpp b/caps/nsScriptSecurityManager.cpp
>index 709c9e8..9726583 100644
>--- a/caps/nsScriptSecurityManager.cpp


>+++ b/caps/nsScriptSecurityManager.cpp
>@@ -448,16 +448,28 @@ nsScriptSecurityManager::GetChannelURIPrincipal(nsIChannel* aChannel,
>     if (nsIContentPolicy::TYPE_DOCUMENT == contentPolicyType ||
>         nsIContentPolicy::TYPE_SUBDOCUMENT == contentPolicyType) {
>       // If it's document or sub-document, inherit originAttributes from
>       // the document.
>       if (loadContext) {
>         DocShellOriginAttributes docShellAttrs;
>         loadContext->GetOriginAttributes(docShellAttrs);
>         attrs.InheritFromDocShellToDoc(docShellAttrs, uri);
>+
>+        // When the pref is on, we also compute the firstParyDomain attribute
>+        // if it's top-level document,
>+        if (Preferences::GetBool("privacy.firstparty.isolate", false) &&
>+            nsIContentPolicy::TYPE_DOCUMENT == contentPolicyType) {
Could we please use bool var cache for the pref.

>   DocShellOriginAttributes attrs;
>   if (docShell->ItemType() == mDocShell->ItemType()) {
>     attrs = nsDocShell::Cast(docShell)->GetOriginAttributes();
>   }
> 
>+  // Inherit origin attributes from parent document. For example,
>+  // firstPartyDomain is computed from top-level document, it doesn't exist in
>+  // the top-level docshell.
>+  if (parentType == nsIDocShellTreeItem::typeContent) {
>+    PrincipalOriginAttributes poa = BasePrincipal::Cast(doc->NodePrincipal())->OriginAttributesRef();
>+    attrs.InheritFromDocToChildDocShell(poa);
>+  }
I think we should add some MOZ_ASSERTs here that only firstPartyDomain might be different between
nsDocShell::Cast(docShell)->GetOriginAttributes(); and BasePrincipal::Cast(doc->NodePrincipal())->OriginAttributesRef();

 
So, the idea is that all the loads will have first party, or none? So, it wouldn't be possible to have per tab settings for this?
I guess that is fine initially.


I didn't yet review the tests. Would like to see those assertions in place and hoping our code passes with them.
Attachment #8783409 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] (vacation Aug 25-28) from comment #54)
> Could we limit the changes in this bug to the feature this bug is adding.
> (or perhaps I'm misunderstand some changes)
Okay, will do.
I just need to pass mFirstPartyDomain properly, I'll merge the needed code into Part 4.


(In reply to Olli Pettay [:smaug] (vacation Aug 25-28) from comment #55)

> >+  // Inherit origin attributes from parent document. For example,
> >+  // firstPartyDomain is computed from top-level document, it doesn't exist in
> >+  // the top-level docshell.
> >+  if (parentType == nsIDocShellTreeItem::typeContent) {
> >+    PrincipalOriginAttributes poa = BasePrincipal::Cast(doc->NodePrincipal())->OriginAttributesRef();
> >+    attrs.InheritFromDocToChildDocShell(poa);
> >+  }
> I think we should add some MOZ_ASSERTs here that only firstPartyDomain might
> be different between
> nsDocShell::Cast(docShell)->GetOriginAttributes(); and
> BasePrincipal::Cast(doc->NodePrincipal())->OriginAttributesRef();
> 
Okay, will do.
Given that the mFirstPartyDomain will only be computed when the pref is on,
so inside nsFrameLoader, I'll put those MOZ_ASSERT inside #ifdef DEBUG,
if you think adding '#ifdef DEBUG' is unneccesary, I'll remove it. :)

>  
> So, the idea is that all the loads will have first party, or none?
If the pref is on, all the load will have firstPartyDomain attribute.

> So, it
> wouldn't be possible to have per tab settings for this?
> I guess that is fine initially.
> 
Right now it's *NOT* a per tab settings, it should affect the whole browser.
added MOZ_ASSERT,and BoolVarCache.
See the interdiff in 
https://gist.github.com/allstarschh/063a229329cbef7375ee60cd7d75ad2f

I also added one postMessage test this time,
https://gist.github.com/allstarschh/9b033bd15ab97bf329dcdd0cab5207ae
Attachment #8783406 - Attachment is obsolete: true
Attachment #8783409 - Attachment is obsolete: true
Attachment #8784259 - Flags: review?(bugs)
MOZ_ASSERT is always #ifdef DEBUG. 
One needs to use MOZ_RELEASE_ASSERT or similar to assert in non-debug builds.
Comment on attachment 8784259 [details] [diff] [review]
Part 3: propagate the firstPartyDomain attribute. v2

(In reply to Olli Pettay [:smaug] (vacation Aug 25-28) from comment #58)
> MOZ_ASSERT is always #ifdef DEBUG. 
> One needs to use MOZ_RELEASE_ASSERT or similar to assert in non-debug builds.

Oh, I mean put 'read pref and cache it' part below inside #ifdef DEBUG, although we have cached it.

>+  // Inherit origin attributes from parent document. For example,
>+  // firstPartyDomain is computed from top-level document, it doesn't exist in
>+  // the top-level docshell.
>+  if (parentType == nsIDocShellTreeItem::typeContent) {
>+    MOZ_ASSERT(attrs.mFirstPartyDomain.IsEmpty(),
>+               "Top-level DocShellAttributes shouldn't have FirstPartyDomain attribute.");
>+    PrincipalOriginAttributes poa = BasePrincipal::Cast(doc->NodePrincipal())->OriginAttributesRef();
>+    attrs.InheritFromDocToChildDocShell(poa);
>+
>+#ifdef DEBUG
>+    // Cache the privacy.firstparty.isolate pref.
>+    static bool sFirstPartyIsolation = false;
>+    static bool sCachedFirstPartyPref = false;
>+    if (!sCachedFirstPartyPref) {
>+      sCachedFirstPartyPref = true;
>+      Preferences::AddBoolVarCache(&sFirstPartyIsolation, "privacy.firstparty.isolate");
>+    }
>+
>+    // When the pref is on, top-level document and child-docshell should have
>+    // firstParyDomain attribute.
>+    if (sFirstPartyIsolation) {
>+      MOZ_ASSERT(!poa.mFirstPartyDomain.IsEmpty(), "Top-level document should have FirstPartyDomain attribute.");
>+      MOZ_ASSERT(!attrs.mFirstPartyDomain.IsEmpty(), "Child DocShell should have FirstPartyDomain attribute.");
>+    }
>+#endif /* DEBUG */
>+  }
>+
Comment on attachment 8784259 [details] [diff] [review]
Part 3: propagate the firstPartyDomain attribute. v2

Review of attachment 8784259 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/originattributes/test/browser_firstPartyIsolation.js
@@ +90,5 @@
> +  yield ContentTask.spawn(tab.linkedBrowser, { firstPartyDomain: BASE_DOMAIN }, function* (attrs) {
> +    info("document principal: " + content.document.nodePrincipal.origin);
> +    let value = content.document.getElementById("message").textContent;
> +    info("value="+value);
> +    Assert.equal(value, "OK");

The postMessage part just added still has some intermittent failure, I'll keep working on it.
However the rest part should be okay.
Comment on attachment 8784259 [details] [diff] [review]
Part 3: propagate the firstPartyDomain attribute. v2

Review of attachment 8784259 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/firstpartyisolation/test/test.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

You probably forgot to git rm this file.
Comment on attachment 8784259 [details] [diff] [review]
Part 3: propagate the firstPartyDomain attribute. v2

Review of attachment 8784259 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/nsScriptSecurityManager.cpp
@@ +470,5 @@
> +          NS_ENSURE_TRUE(tldService, NS_ERROR_FAILURE);
> +
> +          nsAutoCString baseDomain;
> +          tldService->GetBaseDomain(uri, 0, baseDomain);
> +          attrs.mFirstPartyDomain = NS_ConvertUTF8toUTF16(baseDomain);

This does not look correct to me.

We should be setting the correct OriginAttributes in nsILoadInfo.

If we do that then GetChannelURIPrincipal should be able to simply use the OriginAttributes from the nsILoadInfo (using PrincipalOriginAttributes::InheritFromNecko...).

That should always work. I.e. we should not need to check for nsIContentPolicy::TYPE_(SUB)DOCUMENT == contentPolicyType, and we should not need to check for loadContext.
Attachment #8784259 - Flags: review-
(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #62)
> Comment on attachment 8784259 [details] [diff] [review]
> Part 3: propagate the firstPartyDomain attribute. v2
> 
> Review of attachment 8784259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: caps/nsScriptSecurityManager.cpp
> @@ +470,5 @@
> > +          NS_ENSURE_TRUE(tldService, NS_ERROR_FAILURE);
> > +
> > +          nsAutoCString baseDomain;
> > +          tldService->GetBaseDomain(uri, 0, baseDomain);
> > +          attrs.mFirstPartyDomain = NS_ConvertUTF8toUTF16(baseDomain);
> 
> This does not look correct to me.
> 
> We should be setting the correct OriginAttributes in nsILoadInfo.
> 
> If we do that then GetChannelURIPrincipal should be able to simply use the
> OriginAttributes from the nsILoadInfo (using
> PrincipalOriginAttributes::InheritFromNecko...).
> 
> That should always work. I.e. we should not need to check for
> nsIContentPolicy::TYPE_(SUB)DOCUMENT == contentPolicyType, and we should not
> need to check for loadContext.

Thanks Jonas, this is a very helpful comment.
the loadInfo is created from nsDocShell::DoURILoad, and we override the OA of the loadInfo in 

http://searchfox.org/mozilla-central/rev/44f6964ba95b8ddd8ebf70c55b34cd2323afeef4/docshell/base/nsDocShell.cpp#10821

So I'll replace the docshell->necko origin attributes into another inheriting function

NeckoOriginAttributes::InheritFromDocShellToNecko(DocShellOriginAttributes& aAttrs, bool aIsDocument, nsIURI* aURI)

when aIsDocument is true, it means this is a top-level document load, and we will compute the firstPartyDomain value accordingly if the pref is on.

(Or do you have a better naming suggestion?)

And now I should fix http-redirect part, 
will also add another 

NeckoOriginAttributes::InheritNeckoWhenRedirect(NeckoOriginAttributes& aAttrs, nsIURI* aURI)
to re-compute the firstPartyDomain.

That should make the code more cleaner.
(In reply to Jonathan Hao [:jhao] from comment #61)
> Comment on attachment 8784259 [details] [diff] [review]
> Part 3: propagate the firstPartyDomain attribute. v2
> 
> Review of attachment 8784259 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/firstpartyisolation/test/test.html
> @@ +1,1 @@
> > +<!DOCTYPE HTML>
> 
> You probably forgot to git rm this file.

Thanks, will fix this.
Will also rebase on bug 1291652.
(In reply to Yoshi Huang[:allstars.chh] from comment #63)
> Thanks Jonas, this is a very helpful comment.
> the loadInfo is created from nsDocShell::DoURILoad, and we override the OA
> of the loadInfo in 
> 
> http://searchfox.org/mozilla-central/rev/
> 44f6964ba95b8ddd8ebf70c55b34cd2323afeef4/docshell/base/nsDocShell.cpp#10821
> 
> So I'll replace the docshell->necko origin attributes into another
> inheriting function
> 
> NeckoOriginAttributes::InheritFromDocShellToNecko(DocShellOriginAttributes&
> aAttrs, bool aIsDocument, nsIURI* aURI)
> 
> when aIsDocument is true, it means this is a top-level document load, and we
> will compute the firstPartyDomain value accordingly if the pref is on.

If it's only true for top-level document loads, then you should probably call it aIsToplevelDocument.

However note that Huseby had already written code to do this in some bug. I thought it was this bug but I'm not sure. Please talk to him since it seems like there's duplication of effort happening here.

> And now I should fix http-redirect part, 
> will also add another 
> 
> NeckoOriginAttributes::InheritNeckoWhenRedirect(NeckoOriginAttributes&
> aAttrs, nsIURI* aURI)
> to re-compute the firstPartyDomain.

Can't you call NeckoOriginAttributes::InheritFromDocShellToNecko here too? Why is a separate function needed?
(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #65)
> (In reply to Yoshi Huang[:allstars.chh] from comment #63)
> > Thanks Jonas, this is a very helpful comment.
> > the loadInfo is created from nsDocShell::DoURILoad, and we override the OA
> > of the loadInfo in 
> > 
> > http://searchfox.org/mozilla-central/rev/
> > 44f6964ba95b8ddd8ebf70c55b34cd2323afeef4/docshell/base/nsDocShell.cpp#10821
> > 
> > So I'll replace the docshell->necko origin attributes into another
> > inheriting function
> > 
> > NeckoOriginAttributes::InheritFromDocShellToNecko(DocShellOriginAttributes&
> > aAttrs, bool aIsDocument, nsIURI* aURI)
> > 
> > when aIsDocument is true, it means this is a top-level document load, and we
> > will compute the firstPartyDomain value accordingly if the pref is on.
> 
> If it's only true for top-level document loads, then you should probably
> call it aIsToplevelDocument.
> 
Okay, will rename it.

> However note that Huseby had already written code to do this in some bug. I
> thought it was this bug but I'm not sure. Please talk to him since it seems
> like there's duplication of effort happening here.
Yeah, I took over this bug.

> > And now I should fix http-redirect part, 
> > will also add another 
> > 
> > NeckoOriginAttributes::InheritNeckoWhenRedirect(NeckoOriginAttributes&
> > aAttrs, nsIURI* aURI)
> > to re-compute the firstPartyDomain.
> 
> Can't you call NeckoOriginAttributes::InheritFromDocShellToNecko here too?
> Why is a separate function needed?
I thought you comment 25 for the redirect is referring to 
nsDocShell::OnRedirectStateChange
http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#

So far It looks the newChannel is not created in nsDocShell::DoURILoad, so the mFirstPartyDomain is still from the oldChannel.

I'll check the code in more detail how newChannel is created and try to reuse NeckoOriginAttributes::InheritFromDocShellToNecko if possible.
Still need another patch for HTTP redirect.
Attachment #8784259 - Attachment is obsolete: true
Attachment #8784259 - Flags: review?(bugs)
Attachment #8785250 - Flags: review?(bugs)
Attached patch Part 5: tests. (obsolete) — — Splinter Review
split tests into another part.
removed some compiler warning
Attachment #8785250 - Attachment is obsolete: true
Attachment #8785250 - Flags: review?(bugs)
Attachment #8785826 - Flags: review?(bugs)
Attached patch Part 5: tests. (obsolete) — — Splinter Review
updated cookie test and removed browser_dummy.js
Attachment #8785252 - Attachment is obsolete: true
Attachment #8785828 - Flags: review?(bugs)
Comment on attachment 8785826 [details] [diff] [review]
Part 3: propagate the firstPartyDomain attribute. v3

>@@ -75,16 +76,22 @@ DocShellOriginAttributes::InheritFromDocToChildDocShell(const PrincipalOriginAtt
> 
>   // TODO:
>   // Bug 1225353 - DocShell/NeckoOriginAttributes should inherit
>   // mSignedPkg accordingly by mSignedPkgInBrowser
>   mSignedPkg = aAttrs.mSignedPkg;
> 
>   mPrivateBrowsingId = aAttrs.mPrivateBrowsingId;
>   mFirstPartyDomain = aAttrs.mFirstPartyDomain;
>+
>+  bool isFirstPartyEnabled = IsFirstPartyEnabled();
>+  if (isFirstPartyEnabled) {
>+    MOZ_ASSERT(!aAttrs.mFirstPartyDomain.IsEmpty(), "document should have FirstPartyDomain attribute.");
>+    MOZ_ASSERT(!mFirstPartyDomain.IsEmpty(), "Child DocShell should have FirstPartyDomain attribute.");
>+  }
So this is not what I asked for. I think we should assert that frameloader doesn't override any other OA than mFirstPartyDomain, since everything else should be got from 
nsDocShell::Cast(docShell)->GetOriginAttributes(); there. So I'm worried about the attrs.InheritFromDocToChildDocShell(poa); call there.




>+  // When the pref is on, we also compute the firstParyDomain attribute
firstPartyDomain
>+  // Inheriting OriginAttributes from a docshell when loading a top-level
>+  // document.
>+  void InheritFromDocShellToNecko(const DocShellOriginAttributes& aAttrs, const bool aIsTopLevelDocument, nsIURI* aURI);
overlong line

>+++ b/caps/nsScriptSecurityManager.cpp
>@@ -429,47 +429,21 @@ nsScriptSecurityManager::GetChannelURIPrincipal(nsIChannel* aChannel,
>     NS_PRECONDITION(aChannel, "Must have channel!");
> 
>     // Get the principal from the URI.  Make sure this does the same thing
>     // as nsDocument::Reset and XULDocument::StartDocumentLoad.
>     nsCOMPtr<nsIURI> uri;
>     nsresult rv = NS_GetFinalChannelURI(aChannel, getter_AddRefs(uri));
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>-    nsCOMPtr<nsILoadContext> loadContext;
>-    NS_QueryNotificationCallbacks(aChannel, loadContext);
>-
>     nsCOMPtr<nsILoadInfo> loadInfo;
>     aChannel->GetLoadInfo(getter_AddRefs(loadInfo));
>-    nsContentPolicyType contentPolicyType = nsIContentPolicy::TYPE_INVALID;
>-    if (loadInfo) {
>-      contentPolicyType = loadInfo->GetExternalContentPolicyType();
>-    }
> 
>     PrincipalOriginAttributes attrs;
>-    if (nsIContentPolicy::TYPE_DOCUMENT == contentPolicyType ||
>-        nsIContentPolicy::TYPE_SUBDOCUMENT == contentPolicyType) {
>-      // If it's document or sub-document, inherit originAttributes from
>-      // the document.
>-      if (loadContext) {
>-        DocShellOriginAttributes docShellAttrs;
>-        loadContext->GetOriginAttributes(docShellAttrs);
>-        attrs.InheritFromDocShellToDoc(docShellAttrs, uri);
>-      }
>-    } else {
>-      // Inherit origin attributes from loading principal if any.
>-      nsCOMPtr<nsIPrincipal> loadingPrincipal;
>-      if (loadInfo) {
>-        loadInfo->GetLoadingPrincipal(getter_AddRefs(loadingPrincipal));
>-      }
>-      if (loadingPrincipal) {
>-        attrs = BasePrincipal::Cast(loadingPrincipal)->OriginAttributesRef();
>-      }
>-    }
>-
>+    attrs.InheritFromNecko(loadInfo->GetOriginAttributes());

Hmm, so this is re-architecting Bug 1211590
Could you explain me why this doesn't break anything in case the initial about:blank in docshell starts some load. I think we still have the issue that
OA of docshell can be set after docshell has got the about:blank.
Though, hmm, is that broken anyhow, if somehow we end up getting principal from that about:blank and pass it to loadinfo and then load some non-documents, like images or scripts.
But please test private browsing handling, and explain why the change is ok. We've had so many issues with pb recently.
Is this ok for document loads because nsDocShell::DoURILoad has the OA handling?


I'd like to see a new patch with more assertions in InheritFromDocToChildDocShell, and then some explanation for nsScriptSecurityManager::GetChannelURIPrincipal
Attachment #8785826 - Flags: review?(bugs) → review-
Comment on attachment 8785827 [details] [diff] [review]
Part 4: update OriginAttributes of LoadInfo when HTTP redirect.

Why do we need this? Why ->Clone() doesn't do the right thing?

What is the behavior we want on redirects? Per this patch we let redirected load to break out from '1st party isolation'. Is that expected? Isn't that at least unclear to the user.


Please explain
Attachment #8785827 - Flags: review?(bugs) → review-
Comment on attachment 8785828 [details] [diff] [review]
Part 5: tests.

I think I can't review this yet since I don't understand what behavior we're trying to get for redirects. Especially interested redirects of iframes and such.
Attachment #8785828 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #72)
> >+
> >+  bool isFirstPartyEnabled = IsFirstPartyEnabled();
> >+  if (isFirstPartyEnabled) {
> >+    MOZ_ASSERT(!aAttrs.mFirstPartyDomain.IsEmpty(), "document should have FirstPartyDomain attribute.");
> >+    MOZ_ASSERT(!mFirstPartyDomain.IsEmpty(), "Child DocShell should have FirstPartyDomain attribute.");
> >+  }
> So this is not what I asked for. I think we should assert that frameloader
> doesn't override any other OA than mFirstPartyDomain, since everything else
> should be got from 
> nsDocShell::Cast(docShell)->GetOriginAttributes(); there. So I'm worried
> about the attrs.InheritFromDocToChildDocShell(poa); call there.
> 
Okay, I'll update the assert,
Right now the firstPartyDomain is only computed at top-level document, all the sub-resources the document uses will get the firstParyDomain value through InheritFromXXX functions, that means child docshell and child document (and parent document) will have the same firstPartyDomain value, whereas top-level docshell's firstPartyDomain is "". So I'll add the ASSERT when the mIsTopLevelContent is true.

if (mIsTopLevelContent) {
  MOZ_ASSERT(nsDocShell::Cast(docShell)->GetOriginAttributes().mFirstPartyDomain.IsEmpty());
}

but your comment "since everything else should be got from nsDocShell::Cast(docShell)->GetOriginAttributes();",
could be incorrect (in content side),

From Jonas' comments, 
https://bugzilla.mozilla.org/show_bug.cgi?id=1165466#c86
<quote>* Inheriting OriginAttributes from document to child docshell when an <iframe> is created.</quote>
Note here he said "from (parent) document" instead of parent docshell.

https://bugzilla.mozilla.org/show_bug.cgi?id=1209162#c5
<quote>
I think we generally should inherit from document to docshell.
One reason for this is that if the document doesn't have mSignedPkg set (due to not being loaded from the package), then any <iframe> that it creates should get a docshell without a mSignedPkg. No matter what the parent docshell has as mSignedPkg.
</quote>
(I know mSignedPkg is not related to this bug here, but the idea could still applies here, document could have different origin attributes than its docshell, so document could override some origin attributes that got from docshell, this shouldn't be applied to appId, nor isIsolatedMozBrowser, nor mPrivateBrowsingId, where these attributes should always be progated through the docshell hierachy. but it could be applied for firstParyDomain attribute.)

More detail for this could be found in https://bugzilla.mozilla.org/show_bug.cgi?id=1227861#c74

I know you have some comments for the inheriting from chrome docshell to content docshell in 
https://bugzilla.mozilla.org/show_bug.cgi?id=1227861#c66

That could also be a problem, but generally in content side I think we could inherit from the parent *document*.


> >+++ b/caps/nsScriptSecurityManager.cpp
> >@@ -429,47 +429,21 @@ nsScriptSecurityManager::GetChannelURIPrincipal(nsIChannel* aChannel,
> >     NS_PRECONDITION(aChannel, "Must have channel!");
> > 
> >     // Get the principal from the URI.  Make sure this does the same thing
> >     // as nsDocument::Reset and XULDocument::StartDocumentLoad.
> >     nsCOMPtr<nsIURI> uri;
> >     nsresult rv = NS_GetFinalChannelURI(aChannel, getter_AddRefs(uri));
> >     NS_ENSURE_SUCCESS(rv, rv);
> > 
> >-    nsCOMPtr<nsILoadContext> loadContext;
> >-    NS_QueryNotificationCallbacks(aChannel, loadContext);
> >-
> >     nsCOMPtr<nsILoadInfo> loadInfo;
> >     aChannel->GetLoadInfo(getter_AddRefs(loadInfo));
> >-    nsContentPolicyType contentPolicyType = nsIContentPolicy::TYPE_INVALID;
> >-    if (loadInfo) {
> >-      contentPolicyType = loadInfo->GetExternalContentPolicyType();
> >-    }
> > 
> >     PrincipalOriginAttributes attrs;
> >-    if (nsIContentPolicy::TYPE_DOCUMENT == contentPolicyType ||
> >-        nsIContentPolicy::TYPE_SUBDOCUMENT == contentPolicyType) {
> >-      // If it's document or sub-document, inherit originAttributes from
> >-      // the document.
> >-      if (loadContext) {
> >-        DocShellOriginAttributes docShellAttrs;
> >-        loadContext->GetOriginAttributes(docShellAttrs);
> >-        attrs.InheritFromDocShellToDoc(docShellAttrs, uri);
> >-      }
> >-    } else {
> >-      // Inherit origin attributes from loading principal if any.
> >-      nsCOMPtr<nsIPrincipal> loadingPrincipal;
> >-      if (loadInfo) {
> >-        loadInfo->GetLoadingPrincipal(getter_AddRefs(loadingPrincipal));
> >-      }
> >-      if (loadingPrincipal) {
> >-        attrs = BasePrincipal::Cast(loadingPrincipal)->OriginAttributesRef();
> >-      }
> >-    }
> >-
> >+    attrs.InheritFromNecko(loadInfo->GetOriginAttributes());
> 
> Hmm, so this is re-architecting Bug 1211590
And Bug 1125916

> Could you explain me why this doesn't break anything in case the initial
> about:blank in docshell starts some load. 
Bug 1125916 set the origin attributes on the loadInfo, base on the attributes that passed from docShell.
So that's what Jonas meant in comment 62.

> I think we still have the issue
> that
> OA of docshell can be set after docshell has got the about:blank.
> Though, hmm, is that broken anyhow, if somehow we end up getting principal
> from that about:blank and pass it to loadinfo and then load some
> non-documents, like images or scripts.
I am not sure if I understand your points correctly, but I didn't update the origin attributes of any *docshell* after it has loaded a document, also FYI bug 1273058 is trying to add assert as you mentioned.

For the top-level docshell (created in nsWebBrowser), I didn't set firstPartyDomain on it.
For the child docshell (iframe), it will inherit the firstPartyDomain from the parent document.
And the principal of top-level document, will be created from GetChannelURIPrincipal.

So the step is as follows:
1. When we create a new tab, it loads about:blank, and nsDocShell::DoURILoad will create a new LoadInfo, and the loadInfo will inherit the origin attributes from the docshell by calling InheritFromDocShellToNecko.

2. Then GetChannelURIPrincipal will calculate the principal of the document, base on the origin attributes from the loadInfo the created from (1). (Originally for the top-level load, the origin attributes is from the docshell, which is also from (1))

3. Now in the same tab we navigate to https://www.mozilla.org, 
Again the same docshell from 1 will call nsDocShell::DoURILoad, which will create a new LoadInfo, and loadInfo will inherit the same origin attributes from the same docshell in (1), and this time InheritFromDocShellToNecko will calculate the firstPartyDomain on the NeckoOrginAttributes.

4. The document principal of https://www.mozilla.org will be created in GetChannelURIPrincipal, which will inherit the OriginAttributes from the loadInfo created in (3).
 
> But please test private browsing handling, and explain why the change is ok.
> We've had so many issues with pb recently.
I know the PB has some problems. But I've run the try and the result is okay.
I'd like to make it clear that PB and FirstPartyIsolation are two different origin attributes and should be different engineering tasks, hence if you think there's PB problem, we should discuss seperately.

> Is this ok for document loads because nsDocShell::DoURILoad has the OA
> handling?
> 
Yes, base on the work on Bug 1125916.

> 
> I'd like to see a new patch with more assertions in
> InheritFromDocToChildDocShell, and then some explanation for
> nsScriptSecurityManager::GetChannelURIPrincipal
Will do, 

Thanks
(In reply to Olli Pettay [:smaug] from comment #73)
> Comment on attachment 8785827 [details] [diff] [review]
> Part 4: update OriginAttributes of LoadInfo when HTTP redirect.
> 
> Why do we need this? Why ->Clone() doesn't do the right thing?
> 
> What is the behavior we want on redirects? Per this patch we let redirected
> load to break out from '1st party isolation'. Is that expected? Isn't that
> at least unclear to the user.
> 
> 
> Please explain

In the old channel, said https://www.mozilla.org, its firstPartyDomain should be 'mozilla.org', now if there's a Cross-Origin HTTP redirect, and redirect the page to http://www.google.com, the new Channel will also have the firstPartyDomain as "mozilla.org", which should be wrong, as the top-level document now is http://www.google.com

So the behavior we want is, users navigates to https://www.mozilla.org, then it is redirected to a Cross-Origin page, https://www.google.com, we want the firstPartyDomain of the top-level document is 'google.com'.
So it would share the same data-jar if user just navigates to https://www.google.com without redirect.
This is the problem Jonas mentioned in Comment 25,
and he suggested me to use to same function InheritFromDocShellToNecko in Comment 65
(In reply to Yoshi Huang[:allstars.chh] from comment #76)
> In the old channel, said https://www.mozilla.org, its firstPartyDomain
> should be 'mozilla.org', now if there's a Cross-Origin HTTP redirect, and
> redirect the page to http://www.google.com, the new Channel will also have
> the firstPartyDomain as "mozilla.org", which should be wrong, as the
> top-level document now is http://www.google.com

What about other than top level redirects?
added assert.
Attachment #8786276 - Attachment is obsolete: true
only update attrs if it's top-level document load.
Attachment #8785827 - Attachment is obsolete: true
Attached patch Part 5: tests (obsolete) — — Splinter Review
added window.open test
Attachment #8785828 - Attachment is obsolete: true
Attached patch Part 5: tests (obsolete) — — Splinter Review
Attachment #8786709 - Attachment is obsolete: true
Comment on attachment 8786708 [details] [diff] [review]
Part 4: update OriginAttributes of LoadInfo when HTTP redirect. v2

>+    if (isTopLevelDoc) {
>+      nsCOMPtr<nsILoadContext> loadContext;
>+      NS_QueryNotificationCallbacks(this, loadContext);
>+      DocShellOriginAttributes docShellAttrs;
>+      if (loadContext) {
>+        loadContext->GetOriginAttributes(docShellAttrs);
>+      }
>+      MOZ_ASSERT(docShellAttrs.mFirstPartyDomain.IsEmpty(),
>+                 "top-level docshell shouldn't have firstPartyDomain attribute.");
>+
>+      NeckoOriginAttributes attrs = newLoadInfo->GetOriginAttributes();
>+      attrs.InheritFromDocShellToNecko(docShellAttrs, true, newURI);
Why do we need to inherit docshell OAs? Don't we effectively just want to keep the existing OAs + change mFirstPartyDomain?

(Though, whether attrs and docShellAttrs can be different here is unclear to me)

Could you explain and ask review again, or simplify this by just overriding firstPartyDomain.
Attachment #8786708 - Flags: review?(bugs) → review-
Comment on attachment 8786707 [details] [diff] [review]
Part 3: propagate the firstPartyDomain attribute. v5


>+++ b/docshell/base/nsDocShell.cpp
>@@ -10813,17 +10813,19 @@ nsDocShell::DoURILoad(nsIURI* aURI,
>                    securityFlags) :
>       new LoadInfo(loadingPrincipal, triggeringPrincipal, loadingNode,
>                    securityFlags, aContentPolicyType);
> 
>   // We have to do this in case our OriginAttributes are different from the
>   // OriginAttributes of the parent document. Or in case there isn't a
>   // parent document.
>   NeckoOriginAttributes neckoAttrs;
>-  neckoAttrs.InheritFromDocShellToNecko(GetOriginAttributes());
>+  bool isTopLevelDoc = aContentPolicyType == nsIContentPolicy::TYPE_DOCUMENT &&
>+                       mItemType == typeContent;
>+  neckoAttrs.InheritFromDocShellToNecko(GetOriginAttributes(), isTopLevelDoc, aURI);
One thing I wasn't thinking before... since we have still mozbrowser support in tree.
Don't we want to give the top level docshell underneath mozbrowser a new firstparty domain, if its parent docshell is also typeContent?

>+  // Inherit origin attributes from parent document. For example,
>+  // firstPartyDomain is computed from top-level document, it doesn't exist in
>+  // the top-level docshell.
>+  if (parentType == nsIDocShellTreeItem::typeContent) {
also here. I think we want to inherit from parent docshell only if OwnerIsMozBrowserOrAppFrame returns false (but parent is still typeContent)


Sorry that this takes couple of iterations.
Attachment #8786707 - Flags: review?(bugs) → review-
Comment on attachment 8786712 [details] [diff] [review]
Part 5: tests


>+++ b/browser/components/originattributes/test/browser/window.html
>@@ -0,0 +1,12 @@
>+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN">
>+<html>
>+  <head>
>+    <meta charset="utf8">
>+    <title>Page creating a popup</title>
>+  </head>
>+  <body>
>+    <script type="text/javascript">
>+       window.open("data:text/plain,test", "testwin").document.body.innerHTML = "<iframe id='iframe1' src='data:text/plain,test2'></iframe>";
Sorry if I wasn't clear, but I asked for window.open(); (no parameter, especially no first param) That is different since at least in Gecko it just creates to initial about:blank but doesn't load anything. And that document is the tricky one to deal with.
So you'd need to do something like
var w = window.open();
w.document.body.innerHTML = "<iframe id='iframe1' src='data:text/plain,test2'></iframe>";

So could you keep this exiting window.open test and add another one for window.open().
Comment on attachment 8786712 [details] [diff] [review]
Part 5: tests


>+++ b/browser/components/originattributes/test/browser/window.html
>@@ -0,0 +1,12 @@
>+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN">
>+<html>
>+  <head>
>+    <meta charset="utf8">
>+    <title>Page creating a popup</title>
>+  </head>
>+  <body>
>+    <script type="text/javascript">
>+       window.open("data:text/plain,test", "testwin").document.body.innerHTML = "<iframe id='iframe1' src='data:text/plain,test2'></iframe>";
Sorry if I wasn't clear, but I asked for window.open(); (no parameter, especially no first param) That is different since at least in Gecko it just creates to initial about:blank but doesn't load anything. And that document is the tricky one to deal with.
So you'd need to do something like
var w = window.open();
w.document.body.innerHTML = "<iframe id='iframe1' src='data:text/plain,test2'></iframe>";

So could you keep this exiting window.open test and add another one for window.open().
With that r+
Attachment #8786712 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #84)
> Comment on attachment 8786708 [details] [diff] [review]
> Part 4: update OriginAttributes of LoadInfo when HTTP redirect. v2
> 
> >+    if (isTopLevelDoc) {
> >+      nsCOMPtr<nsILoadContext> loadContext;
> >+      NS_QueryNotificationCallbacks(this, loadContext);
> >+      DocShellOriginAttributes docShellAttrs;
> >+      if (loadContext) {
> >+        loadContext->GetOriginAttributes(docShellAttrs);
> >+      }
> >+      MOZ_ASSERT(docShellAttrs.mFirstPartyDomain.IsEmpty(),
> >+                 "top-level docshell shouldn't have firstPartyDomain attribute.");
> >+
> >+      NeckoOriginAttributes attrs = newLoadInfo->GetOriginAttributes();
> >+      attrs.InheritFromDocShellToNecko(docShellAttrs, true, newURI);
> Why do we need to inherit docshell OAs? Don't we effectively just want to
> keep the existing OAs + change mFirstPartyDomain?
> 
Jonas mentioned in somewhere that we should use those Inherit functions to propagate origin attributes, otherwise it will be messy if we only compute some attribute on our own.
So you cannot convert DocShellOriginAttributes to NeckoOriginAttributes directly, and vice vesa, you have to use those Inherit functions.
(Sorry I cannot find the comment mentioning this now)

Also since this will be a top-level load, so I'd like to use the same behavior in nsDocShell::DoURILoad to get the same NeckoOriginAttributes, and I try to make all the details inside those Inherit functions.

Originally I try to have a function called NeckOriginAttributes::InheritFromNeckoOnRedirect 
but Jonas suggested me to use the same one (InheritFromDocShellToNecko) in Comment 65.

> (Though, whether attrs and docShellAttrs can be different here is unclear to
> me)
>
It could different as far as firstPartyDomain is concerned, that's why we need to use Inherit functions.
So all DocShellOriginAttributes -> NeckoOriginAttributes will behave the same.
 
> Could you explain and ask review again, or simplify this by just overriding
> firstPartyDomain.

I think using Inherit function is better, unless you have more strong reason to just compute the firtPartyDomain alone here.
(In reply to Olli Pettay [:smaug] from comment #87)
> >+    <script type="text/javascript">
> >+       window.open("data:text/plain,test", "testwin").document.body.innerHTML = "<iframe id='iframe1' src='data:text/plain,test2'></iframe>";
> Sorry if I wasn't clear,
No, it's my fault.

> but I asked for window.open(); (no parameter,
> especially no first param) That is different since at least in Gecko it just
> creates to initial about:blank but doesn't load anything. And that document
> is the tricky one to deal with.
> So you'd need to do something like
> var w = window.open();
> w.document.body.innerHTML = "<iframe id='iframe1'
> src='data:text/plain,test2'></iframe>";
> 
I found in my original test in non-e10s mode, the iframe will be removed by the "data:text/plain,test", and causes the iframe1 to be null.
So window.open() is better. I'll replace it.

Thanks
Comment on attachment 8786707 [details] [diff] [review]
Part 3: propagate the firstPartyDomain attribute. v5

Review of attachment 8786707 [details] [diff] [review]:
-----------------------------------------------------------------

> >   // We have to do this in case our OriginAttributes are different from the
> >   // OriginAttributes of the parent document. Or in case there isn't a
> >   // parent document.
> >   NeckoOriginAttributes neckoAttrs;
> >-  neckoAttrs.InheritFromDocShellToNecko(GetOriginAttributes());
> >+  bool isTopLevelDoc = aContentPolicyType == nsIContentPolicy::TYPE_DOCUMENT &&
> >+                       mItemType == typeContent;
> >+  neckoAttrs.InheritFromDocShellToNecko(GetOriginAttributes(), isTopLevelDoc, aURI);
> One thing I wasn't thinking before... since we have still mozbrowser support
> in tree.
> Don't we want to give the top level docshell underneath mozbrowser a new
> firstparty domain, if its parent docshell is also typeContent?
>
Okay, I'll add the mozbrowser check.

I also found that some documents are loaded by SystemPrincipal thus there's no OriginAttributes there.
Will add another !nsContentUtils::IsSystemPrincipal(doc->NodePrincipal()) check here.


Thanks for your prompt review.

::: dom/base/nsFrameLoader.cpp
@@ +2117,5 @@
> +    MOZ_ASSERT(attrs.mUserContextId == poa.mUserContextId,
> +              "docshell and docuemnt should have the same userContextId attribute.");
> +    MOZ_ASSERT(attrs.mInIsolatedMozBrowser == poa.mInIsolatedMozBrowser,
> +              "docshell and docuemnt should have the same inIsolatedMozBrowser attribute.");
> +    MOZ_ASSERT(attrs.mAddonId.Equals(poa.mAddonId),

I'll remove the mAddonId assert here, I found some tests failed because of this.
(In reply to Yoshi Huang[:allstars.chh] from comment #88)
> I think using Inherit function is better, unless you have more strong reason
> to just compute the firtPartyDomain alone here.
ok, sounds fine. Ask re-review.
(In reply to Yoshi Huang[:allstars.chh] from comment #90)
> ::: dom/base/nsFrameLoader.cpp
> @@ +2117,5 @@
> > +    MOZ_ASSERT(attrs.mUserContextId == poa.mUserContextId,
> > +              "docshell and docuemnt should have the same userContextId attribute.");
> > +    MOZ_ASSERT(attrs.mInIsolatedMozBrowser == poa.mInIsolatedMozBrowser,
> > +              "docshell and docuemnt should have the same inIsolatedMozBrowser attribute.");
> > +    MOZ_ASSERT(attrs.mAddonId.Equals(poa.mAddonId),
> 
> I'll remove the mAddonId assert here, I found some tests failed because of
> this.
Hmm, why do they fail. Is this again about the bizarre addonId propagation model.
bug 1278804
only inherit from document if !MozBrowserOrApp() and !SystemPrincipal()
removed mAddonId assert.
Attachment #8786707 - Attachment is obsolete: true
Comment on attachment 8786708 [details] [diff] [review]
Part 4: update OriginAttributes of LoadInfo when HTTP redirect. v2

Review of attachment 8786708 [details] [diff] [review]:
-----------------------------------------------------------------

r? again per comment 92.
Attachment #8786708 - Flags: review- → review?(bugs)
Attached patch Part 5: tests v2 (obsolete) — — Splinter Review
window.open()
Attachment #8786712 - Attachment is obsolete: true
Attachment #8787161 - Flags: review+
(In reply to Olli Pettay [:smaug] from comment #93)
> (In reply to Yoshi Huang[:allstars.chh] from comment #90)
> > ::: dom/base/nsFrameLoader.cpp
> > @@ +2117,5 @@
> > > +    MOZ_ASSERT(attrs.mUserContextId == poa.mUserContextId,
> > > +              "docshell and docuemnt should have the same userContextId attribute.");
> > > +    MOZ_ASSERT(attrs.mInIsolatedMozBrowser == poa.mInIsolatedMozBrowser,
> > > +              "docshell and docuemnt should have the same inIsolatedMozBrowser attribute.");
> > > +    MOZ_ASSERT(attrs.mAddonId.Equals(poa.mAddonId),
> > 
> > I'll remove the mAddonId assert here, I found some tests failed because of
> > this.
> Hmm, why do they fail. Is this again about the bizarre addonId propagation
> model.
> bug 1278804

The parent docshell doesn't have to addonId, however the document does.
But my patch should still be fine as InheritFromDocToChildDocShell doesn't override addonId, so addonId is still progated to the child docshell.
Comment on attachment 8786708 [details] [diff] [review]
Part 4: update OriginAttributes of LoadInfo when HTTP redirect. v2

I wish we could also here assert that only mFirstPartyDomain differs between 
docShellAttrs and attrs. (though, addonid is broken so I don't know how it should work).
Do you think you could add similar assertion here as what nsFrameLoader gets.
Attachment #8786708 - Flags: review?(bugs) → review+
Attachment #8787153 - Flags: review?(bugs) → review+
Comment on attachment 8787153 [details] [diff] [review]
Part 3: propagate the firstPartyDomain attribute. v6

Review of attachment 8787153 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/nsScriptSecurityManager.cpp
@@ +445,2 @@
>      PrincipalOriginAttributes attrs;
> +    attrs.InheritFromNecko(loadInfo->GetOriginAttributes());

for addons, loadInfo could be null.
So I'll add 
if (loadInfo) {
  attrs.InheritFromNecko(loadInfo->GetOriginAttributes());
}
add
if (loadInfo)
Attachment #8787153 - Attachment is obsolete: true
Attachment #8787473 - Flags: review+
Attached patch Part 1: add firstPartyDomain v2 — — Splinter Review
forgot to add mFirstPartyDomain in SetFromGeneric
Attachment #8781504 - Attachment is obsolete: true
Attachment #8787474 - Flags: review+
Attached patch Part 5: tests v3 (obsolete) — — Splinter Review
rebase
Attachment #8787161 - Attachment is obsolete: true
Attachment #8787475 - Flags: review+
add MOZ_ASSERT as smaug requested.
Attachment #8786708 - Attachment is obsolete: true
Attachment #8787477 - Flags: review+
Attached patch Part 5: tests v3 — — Splinter Review
added wrong file.
Attachment #8787475 - Attachment is obsolete: true
Attachment #8787523 - Flags: review+
No longer blocks: meta_tor
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/075d612841fb
Backed out changeset 57a1ca19b38c 
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fa7c4d8a5bc
Backed out changeset 51b6694efc8f 
https://hg.mozilla.org/integration/mozilla-inbound/rev/39cff1d988fd
Backed out changeset 10da0eca7bbb 
https://hg.mozilla.org/integration/mozilla-inbound/rev/be65e87da9e3
Backed out changeset 2de0e171f15b 
https://hg.mozilla.org/integration/mozilla-inbound/rev/86e1a437021b
Backed out changeset dd200883aa79 for permafailing test_child_docshell.html on Android debug. r=backout
Backed out for permafailing test_child_docshell.html on Android debug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f63dddb267503a9b751f833ac37f5efc974949e
https://hg.mozilla.org/integration/mozilla-inbound/rev/987712421446356c6d599bd92ddcd307b6f17656
https://hg.mozilla.org/integration/mozilla-inbound/rev/19290c4c824e564fdcb332254b8e6538539ad94c
https://hg.mozilla.org/integration/mozilla-inbound/rev/be9f845f66e87e3cef1c98601c3a281fa9a33fd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/293c0ebc1e091fad7cb6d161ddc795fff126b2a9

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=57a1ca19b38c948ae15b3977c4f719f175e8d467
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=35185447&repo=mozilla-inbound

08:46:07     INFO -  293 INFO TEST-START | dom/ipc/tests/test_child_docshell.html
08:51:24     INFO -  294 INFO TEST-PASS | dom/ipc/tests/test_child_docshell.html | The frame script is running in a real distinct child process
08:51:24     INFO -  295 INFO TEST-PASS | dom/ipc/tests/test_child_docshell.html | docshell.chromeEventHandler has nsIDOMEventTarget interface
08:51:24     INFO -  296 INFO TEST-UNEXPECTED-FAIL | dom/ipc/tests/test_child_docshell.html | Test timed out.
08:51:24     INFO -      reportError@SimpleTest/TestRunner.js:114:7
08:51:24     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:135:7
Flags: needinfo?(allstars.chh)
https://public-artifacts.taskcluster.net/Ki5uTobdTIObuve7XddTmQ/0/public/test_info//logcat-emulator-5554.log

09-02 01:46:58.157  2497  2497 F MOZ_Assert: Assertion failure: attrs.mInIsolatedMozBrowser == poa.mInIsolatedMozBrowser (docshell and document should have the same inIsolatedMozBrowser attribute.), at /home/worker/workspace/build/src/dom/base/nsFrameLoader.cpp:2126

It looks mozbrowser flag  has some problem here.
Checking
Flags: needinfo?(allstars.chh)
I found that it's docshell has mInIsolatedMozBrowser = true;

So this assert happens when Owner is *not* MozBrowserFrame, but the parent docshell has mInIsolatedMozBrowser flag is true.

if(!OwnerIsMozBrowserOrAppFrame()) {
  MOZ_ASSERT(!attrs.mInIsolatedMozBrowser); 
  // assertion triggers when running dom/ipc/tests/test_child_docshell.html
}
Comment on attachment 8788202 [details] [diff] [review]
Part 6: pass origin attributes to nsIWebBrowser

>+%{C++
>+#include "mozilla/BasePrincipal.h"
>+%}
>+
>+[ref] native const_OriginAttributesRef(const mozilla::DocShellOriginAttributes);
>+

It would be nicer if BasePrincipal.h wasn't included here.
Just forward declare DocShellOriginAttributes in side mozilla namespace and then 



With that, r+
Attachment #8788202 - Flags: review?(bugs) → review+
forward declaration

Thanks smaug again for the review and comments. :)
Attachment #8788202 - Attachment is obsolete: true
Attachment #8788230 - Flags: review+
please file those bugs about domain usage and about:/data: urls
(or maybe you did file them already) and make them block this one.
Pushed by yhuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/935ffd53f193
Part 1: add firstPartyDomain. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/99bb7090b830
Part 2: add pref privacy.firstparty.isolate. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9afda2804fd
Part 3: Propagate firstPartyDomain. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9efc0a2bb306
Part 4: update OriginAttributes when http redirect. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7189dc44fca0
part 5: tests. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f4f5b4e3d8a
Part 6: pass origin attributes to nsIWebBrowser. r=smaug
Oops, I was in a hurry and it turned out I pushed the old version of patches.
Sorry for the mistake. :(
Flags: needinfo?(allstars.chh)
Pushed by yhuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e160879d4d
Part 1: add firstPartyDomain. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d173cefba1e1
Part 2: add pref privacy.firstparty.isolate
https://hg.mozilla.org/integration/mozilla-inbound/rev/36b63f0e067a
Part 3: Propagate firstPartyDomain. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/44328c5df64d
Part 4: update OriginAttributes when http redirect. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a2c681ec347
part 5: tests. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2a98312df81
Part 6: pass origin attributes to nsIWebBrowser. r=smaug
(In reply to Olli Pettay [:smaug] from comment #114)
> please file those bugs about domain usage and about:/data: urls
> (or maybe you did file them already) and make them block this one.

Hi Smaug,
I've checked with tor source. They do use domain, as Arthur mentioned in comment 6.
So I think http://foo.com and https://foo.com will share the same cookie.

First they will call mozIThirdPartyUtil.getFirstPartyIsolationURI or getFirstPartyURI to get the top-level document URI. Then they use this URI to get the domain from mozIThirdPartyUtil.getFirstPartyHostForIsolation.

https://gitweb.torproject.org/tor-browser.git/tree/netwerk/base/mozIThirdPartyUtil.idl?h=tor-browser-45.3.0esr-6.5-1#n245

Also noted for the about: (I guess also data: ) URLs, they use the prefix '--NoFirstPartyHost-', then append the scheme and file name.
Comment on attachment 8787473 [details] [diff] [review]
Part 3: propagate the firstPartyDomain attribute. v7

Review of attachment 8787473 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsFrameLoader.cpp
@@ +2111,5 @@
> +    PrincipalOriginAttributes poa = BasePrincipal::Cast(doc->NodePrincipal())->OriginAttributesRef();
> +
> +    // Assert on the firstPartyDomain from top-level docshell should be empty
> +    if (mIsTopLevelContent) {
> +      MOZ_ASSERT(attrs.mFirstPartyDomain.IsEmpty(),

This should have used MOZ_ASSERT_IF.

Not a big deal since most likely the optimizer will kill the dead code in non-debug builds. But for future reference that's the right thing to do in situations like this.
Comment on attachment 8787473 [details] [diff] [review]
Part 3: propagate the firstPartyDomain attribute. v7

Review of attachment 8787473 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsFrameLoader.cpp
@@ +2095,5 @@
>  
>    DocShellOriginAttributes attrs;
>    if (docShell->ItemType() == mDocShell->ItemType()) {
>      attrs = nsDocShell::Cast(docShell)->GetOriginAttributes();
>    }

It's a bit confusing that this code will first inherit from the parent docshell, and then, in some cases, also inherit from the parent document.

As far as I can tell, there are two situations involved.

If the parent document uses a system principal, we should not inherit from the parent at all. Rather the parent should be calling docshell::SetOriginAttributes.

For other docshells we should inherit from the parent document always.

Are there any situations where this will do the wrong thing? If so, what situation would that be?

The other nice thing is that this can be simplified by relying on that the system principal always has the default OAs. If we rely on that we can always inherit from the parent document since if the parent document uses the system principal, it'll simply mean that we inherit the default OAs.
if one opens for example about:addons in a tab with non-default user context id, what UCI should be used in content iframes about:addons use? I think the non-default UCI, and not the default UCI from system principal.
So, I'd say we do want to inherit from docshell, which contains the right UCI.
Comment on attachment 8787477 [details] [diff] [review]
Part 4: update OriginAttributes of LoadInfo when HTTP redirect. v3

Review of attachment 8787477 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2926,5 @@
> +                "docshell and necko should have the same inIsolatedMozBrowser attribute.");
> +      MOZ_ASSERT(docShellAttrs.mPrivateBrowsingId == attrs.mPrivateBrowsingId,
> +                 "docshell and necko should have the same privateBrowsingId attribute.");
> +
> +      attrs.InheritFromDocShellToNecko(docShellAttrs, true, newURI);

I think ideally we'd always call this.

I.e. rather than 

if (isTopLevelDoc) {
  ... updateAttrs ...
}

do

attrs.InheritFromDocShellToNecko(docShellAttrs, isTopLevelDoc, newURI);



Not really important with the current inheriting rules since the newURI argument only matters when isTopLevelDoc is true. But might matter in the future.

At the very least leave a comment in the InheritFromDocShellToNecko function saying that it won't always be called on redirect unless this code is changed.
I think it's simpler that we don't do anything "automatic". If about:addons the inner iframe to use the user context of the top-level tab, it can forward the <xul:browser userContextId> attribute itself.

What the right behavior is in that scenario seems like a case-by-case basis decision anyway. And the current setup wouldn't even work for the attributes that we do have since the firstPartyDomain wouldn't treat the iframe as a top-level iframe.

So my vote is to keep it simple for now.

That said, I'm trying to transition myself out, so the decision really lies with you and bz.
(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #124)
> I.e. rather than 
> 
> if (isTopLevelDoc) {
>   ... updateAttrs ...
> }
> 
> do
> 
> attrs.InheritFromDocShellToNecko(docShellAttrs, isTopLevelDoc, newURI);
>  
> Not really important with the current inheriting rules since the newURI
> argument only matters when isTopLevelDoc is true. But might matter in the
> future.
> 
> At the very least leave a comment in the InheritFromDocShellToNecko function
> saying that it won't always be called on redirect unless this code is
> changed.

Hi Jonas

The if (isTopLevelDoc) {..} is used to get the docshell attributes, which is needed by InheritFromDocShellToNecko, as you requested to reuse it here.

My original intention for adding the 'if()' is not to re-compute the OA if it isn't for top-level redirect.
(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #126)
> So my vote is to keep it simple for now.

Also, I generally prefer explicit APIs, rather than ones that do things automatically. Automatic ones tend to often be harder to use, including for front-end developers, unless they do *exactly* the right thing.

Anyhow, up to bz and you.
(In reply to Yoshi Huang[:allstars.chh] from comment #127)
> My original intention for adding the 'if()' is not to re-compute the OA if
> it isn't for top-level redirect.

For non-top-level redirects the OA computation is really cheap, so not really important to optimize out.

But I'm also fine with leaving a comment at the top of the InheritFromDocShellToNecko function indicating that if the uri argument is used also for non-top-level callers, then HttpBaseChannel::SetupReplacementChannel needs to be updated.
This doesn't seem to entirely work yet.

Enabling privacy.firstparty.isolate on nightly 20160907030427 causes some stackoverflow.com features to fail and it throws a "SecurityError: The operation is insecure." on the console.
(In reply to The 8472 from comment #131)
> This doesn't seem to entirely work yet.
> 
> Enabling privacy.firstparty.isolate on nightly 20160907030427 causes some
> stackoverflow.com features to fail and it throws a "SecurityError: The
> operation is insecure." on the console.

Sorry I downloaded https://ftp.mozilla.org/pub/firefox/nightly/2016/09/2016-09-07-03-04-27-mozilla-central/firefox-51.0a1.en-US.linux-x86_64.tar.bz2, (linux build) and enabled the pref, but didn't run into any problem.

If you have more detail STR and log please file a new bug for it.
Depends on: 1301274
Depends on: 1300182
Depends on: 1301768
Depends on: 1301778
Whiteboard: [tor], [domsecurity-active][ETA 9/12] → [tor], [domsecurity-active][ETA 9/12][tor 13742]
Depends on: 1470156
You need to log in before you can comment on or make changes to this bug.