Remove the assertion of FirstPartyDomain should be empty in HTTP redirect

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: allstars.chh, Assigned: allstars.chh)

Tracking

(Blocks 1 bug)

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [tor][domsecurity-active])

Attachments

(2 attachments, 7 obsolete attachments)

5.53 KB, patch
Details | Diff | Splinter Review
12.35 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
In http://searchfox.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#2955

We assert that the top-level docshell won't have firstPartyDomain set in origin attributes.

However imagine we open foo.html, the foo.html will call window.open("bar.html")
We will have a new window with the top-level docshell has firstPartyDomain set.
Posted patch Patch. (obsolete) — Splinter Review
Hi smaug
See Comment 0 for the problem,
As you mentioned we should have some assert for the firstPartyDomain in https://bugzilla.mozilla.org/show_bug.cgi?id=1260931#c98

I'd like to remove the assert of firstPartyDomain in this bug.

Thanks
Attachment #8808069 - Flags: review?(bugs)
Is it expected behavior that new top level window/tab shares the firstPartyDomain? If so, only when noopener isn't used?
I think we should keep the assertion but perhaps change it so that it is checked only when the window doesn't have an opener.

Would be good to have tests also for <a href="foo.html" target="_blank">.
And also <a href="http://some.other.domain/foo.html" target="_blank">

And for the cases when noopener is being used.
Comment on attachment 8808069 [details] [diff] [review]
Patch.

So I think we need more assertions, not less, to ensure we behave as expected.
Just need to get the assertions right.
Attachment #8808069 - Flags: review?(bugs) → review-
Posted patch Part 1: change the assert. v2 (obsolete) — Splinter Review
compare docshellAttrs.mFirstPartyDomain with loadInfo->TriggeringPrincipal().mFirstPartyDomain.
Attachment #8808069 - Attachment is obsolete: true
Attachment #8808555 - Flags: review?(bugs)
Comment on attachment 8808555 [details] [diff] [review]
Part 1: change the assert. v2

It is still unclear why we actually want to propagate the firstPartyDomain also to 3rd party top level pages in new tabs/windows.
That feels like being inconsistent with "First party isolation means that all identifier sources and browser state are scoped (isolated) using the URL bar domain."
But that is somewhat separate issue from this bug.
Attachment #8808555 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #5)
> Comment on attachment 8808555 [details] [diff] [review]
> Patch. v2
> 
> It is still unclear why we actually want to propagate the firstPartyDomain
> also to 3rd party top level pages in new tabs/windows.

I thought previously you mentioned window.open should also inherit the firstPartyDomain attributes?
that's why the test in
http://searchfox.org/mozilla-central/source/browser/components/originattributes/test/browser/browser_firstPartyIsolation.js#149
from https://bugzilla.mozilla.org/show_bug.cgi?id=1260931#c86

> That feels like being inconsistent with "First party isolation means that
> all identifier sources and browser state are scoped (isolated) using the URL
> bar domain."
Note that I made the the page opened by window.open() will be redirected to a 'same-domain' page on purpose.
If I change the test to redirect to a different domain webpage, say http://example.com,
then the top-level document will override the firstPartyDomain from the top-level docshell (created by window.open)

So the document (and the iframes it creates) will have 'example.com' as firstPartyDomain.
and the top-level docshell will still have 'mochi.test' as firstPartyDomain. 

It is still isolated by the URL bar domain.

I can add this into my test if you think we should.

Or there's something I misunderstood, specially the window.open part, feel free to comment, I'll file another bug and fix it. :)

Thanks
(In reply to Yoshi Huang[:allstars.chh] from comment #6)

> Note that I made the the page opened by window.open() will be redirected to
> a 'same-domain' page on purpose.
> If I change the test to redirect to a different domain webpage, say
> http://example.com,
> then the top-level document will override the firstPartyDomain from the
> top-level docshell (created by window.open)
I see. I guess it doesn't matter which firstPartyDomain the new docshell gets
Would be good to test that iframes in 3rd party window.open'ed document get the firstPartyDomain from top level document, and not from the docshell. But perhaps there is a test for that already.
Attachment #8808555 - Attachment description: Patch. v2 → Part 1: change the assert. v2
In my Part 1 patch I noticed it will cause assert in some website.

say foo.com/ has a <form action="foo.com/login.html" method="POST">,
then after keying the userId/password, the page will be redirected to foo.cc/....

In this case the triggeringPrincipal will be the top-level document foo.com, (with firstPartyDomain is foo.com), however the docshell doesn't have firstPartyDomain and cause the assert.

So I changed the assert only when the firstPartyDomain of the docshell is not empty, and added some comments,

smaug, will you review this again?

Thanks
Attachment #8808958 - Flags: review?(bugs)
Comment on attachment 8808958 [details] [diff] [review]
Part 1.1 : assert when firstPartyDomain is not empty.

So why does this assert work if you do the form posting thingie in a window.open'ed document?
Attachment #8808958 - Flags: review?(bugs) → review-
Attachment #8808934 - Flags: review?(bugs) → review+
> So why does this assert work if you do the form posting thingie in a
> window.open'ed document?
Sorry for not explaining ealier, this is not related to window.open.
It's just form submittion.

In that case loadInfo->TriggeringPrincipal is the document, and it will have firstPartyDomain, whereas loadContext is from the top-level docshell hence no firstPartyDomain.

I've added a test this time, should be more clear this time.

Thanks
Attachment #8808958 - Attachment is obsolete: true
Attachment #8809288 - Flags: review?(bugs)
Attachment #8809288 - Attachment description: Part 3 : only assert when firstPartyDomain is not empty. → Part 3 : only assert when firstPartyDomain is not empty. v2
Comment on attachment 8809288 [details] [diff] [review]
Part 3 : only assert when firstPartyDomain is not empty. v2

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

I found this still doesn't work in the window.open case
STR:
1. I press like button from some webpage foo.com
2. It pops up a facebook login webpage in a new window ( docshell with firstPartyDomain foo.com)
3. Login
4. Then the page will be redirected, in this case TriggeringPrincipal will be facebook.com, and causes the assert.

Perhaps compare it with TriggeringPrincipal() is not a good idea.

Is there any way we can know from LoadInfo that the loadContext(docShell) is created by window.open() from some page?
Attachment #8809288 - Flags: review?(bugs)
Comment on attachment 8810334 [details] [diff] [review]
Part 3 : only assert when firstPartyDomain is not empty. v3

Because of bug 1300671, wouldn't the assertion fire when using data: or about:.

Should we perhaps change mFirstPartyDomain handling so that new top level content docshells get always empty mFirstPartyDomain?
Attachment #8810334 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] (vacation Nov 19-26) from comment #15)
> Comment on attachment 8810334 [details] [diff] [review]
> Part 3 : only assert when firstPartyDomain is not empty. v3
> 
> Because of bug 1300671, wouldn't the assertion fire when using data: or
> about:.
> 
Yeah, you're right.

> Should we perhaps change mFirstPartyDomain handling so that new top level
> content docshells get always empty mFirstPartyDomain?
But that seems to make things more complicated as far as OriginAttributes is concerned.

For the container project, baku made some change so the window.open in a container tab will inherit the userContextId, see bug 1249224.
Things might be more complicated if we enable userContextId and firstPartyIsolation together.

Back to the problem I am trying to fix in this bug,
is that except firstPartyDomain, other origin attributes should be the same with NeckoOriginAttributes, per your comments from https://bugzilla.mozilla.org/show_bug.cgi?id=1260931#c98.

And in nsFrameLoader part, I've added assert mFirstPartyDomain should be empty in the top-level docshell 
http://searchfox.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#2129

In this bug I found in the HTTP redirect case, the assertion of firstPartyDomain will be empty is wrong, as I've provided several tests here, also it might not related to loadInfo nor loadInfo->TriggeringPrincipal() in different cases.

So I'll go back to comment 1 to remove the assert.

Thanks
see explanation on comment 16.
Attachment #8810334 - Attachment is obsolete: true
Flags: needinfo?(bugs)
Hi smaug
I've added a method called InheritOnHttpRedirect.
Jonas' original idea is to reuse InheritFromDocShellToNecko, but it will require to get the attributes from LoadContext first, and we might need to assert those values except firstParrtyDomain should be equal.
If you think this is better, we could use this, and remove those MOZ_ASSERT accordingly.

Thanks
(In reply to Olli Pettay [:smaug] (vacation Nov 19-26) from comment #7)
> (In reply to Yoshi Huang[:allstars.chh] from comment #6)
> 
> > Note that I made the the page opened by window.open() will be redirected to
> > a 'same-domain' page on purpose.
> > If I change the test to redirect to a different domain webpage, say
> > http://example.com,
> > then the top-level document will override the firstPartyDomain from the
> > top-level docshell (created by window.open)
> I see. I guess it doesn't matter which firstPartyDomain the new docshell gets

I agree there is no effect if the docshell overrides the window's firstPartyDomain, but it still sounds dangerously confusing to me to assign the firstPartyDomain from the opener to the new window. Why do we want to do that?
Flags: needinfo?(allstars.chh)
(In reply to Arthur Edelstein [:arthuredelstein] from comment #19)
> I agree there is no effect if the docshell overrides the window's
> firstPartyDomain,
Not sure what you mean here, but the docshell created in window.open doesn't override anything, it keeps the OA from its opener.

> but it still sounds dangerously confusing to me to assign
> the firstPartyDomain from the opener to the new window. Why do we want to do
> that?
Right now it's our bahavior in Origin Attributes,
calling window.open from a container tab will result a same container tab, not a normal tab.
calling window.open from a private browsing window will also result a PB window.

Same for the firstPartyIsolation,
see http://searchfox.org/mozilla-central/source/browser/components/originattributes/test/browser/browser_firstPartyIsolation.js#149

Do you also think we should always empty the firstPartyDomain in the top-level window as smaug suggested in comment 15?
If so I'd file another bug for this, as the code here is from the HTTP redirect (HttpBaseChannel::SetupReplacementChannel), and in some cases it's not related to window.open.
Flags: needinfo?(allstars.chh)
(In reply to Yoshi Huang[:allstars.chh] from comment #20)
> (In reply to Arthur Edelstein [:arthuredelstein] from comment #19)
> > I agree there is no effect if the docshell overrides the window's
> > firstPartyDomain,
> Not sure what you mean here, but the docshell created in window.open doesn't
> override anything, it keeps the OA from its opener.

I misunderstood your earlier comment. This behavior seems incorrect then. A new window or tab that has a URL bar visible should have a firstPartyDomain corresponding to the URL bar domain (with the possible exception of about: or data: URLs), not to the domain of the opener.

> Do you also think we should always empty the firstPartyDomain in the
> top-level window as smaug suggested in comment 15?

I think a top-level document should have the firstPartyDomain from the domain in the URL bar. I'm not sure if that is equivalent to the top-level window firstPartyDomain. But in any case it makes sense to me that containers and firstPartyDomain should have different behaviors for window.open.

> If so I'd file another bug for this, as the code here is from the HTTP
> redirect (HttpBaseChannel::SetupReplacementChannel), and in some cases it's
> not related to window.open.

I didn't understand that. It would be great if you can file another bug if you think we need one.

We have been discussing redirects in bug 1319839 and bug 1309800. Are these relevant to your work here? I am leaning toward the approach in bug 1319839 comment 5.
Flags: needinfo?(allstars.chh)
(In reply to Arthur Edelstein [:arthuredelstein] from comment #21)
> (In reply to Yoshi Huang[:allstars.chh] from comment #20)
> > (In reply to Arthur Edelstein [:arthuredelstein] from comment #19)
> > > I agree there is no effect if the docshell overrides the window's
> > > firstPartyDomain,
> > Not sure what you mean here, but the docshell created in window.open doesn't
> > override anything, it keeps the OA from its opener.
> 
> I misunderstood your earlier comment. This behavior seems incorrect then. A
> new window or tab that has a URL bar visible should have a firstPartyDomain
> corresponding to the URL bar domain (with the possible exception of about:
> or data: URLs), not to the domain of the opener.
> 
I think you misunderstood the docshell/document part.
When the docshell (window, or tab) is created, it doesn't load any document yet, so it doesn't know the firstPartyDomain yet, hence the value(firstPartyDomain) is empty in docshell.

And we can't change the firstPartyDomain after the docshell loads some document either, because it might have some security problem, for example change a private window to a normal window will cause security problems.
http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#14325

We create the firstPartyDomain on the top-level document, and when we create the document, we could decide that if we want to propagate all origin attributes from the docshell to the document, in the firstPartyIsolation case, the mFirstPartyDomain is decided by the documentURI, not from the docshell.
http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#14325

> 
> I'm not sure if that is equivalent to the top-level
> window firstPartyDomain. 
No, as explained above.

> But in any case it makes sense to me that
> containers and firstPartyDomain should have different behaviors for
> window.open.
> 
> We have been discussing redirects in bug 1319839 and bug 1309800. Are these
> relevant to your work here?
Those two bugs, their OA is got from the top-level document. However in this bug we are discussing the docshell. Should be different problems.
Flags: needinfo?(allstars.chh)
Summary: top-level docshell could have firstPartyDomain set. → Remove the assertion of FirstPartyDomain should be empty in HTTP redirect
Comment on attachment 8812722 [details] [diff] [review]
Part 3: remove the assert on firstPartyDomain v2

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

I've filed another bug Bug 1321158 to discuss the cases in window.open.

So back to this bug I'd like to remove the assertion.
Attachment #8812722 - Flags: review?(bugs)
Flags: needinfo?(bugs)
Comment on attachment 8813070 [details] [diff] [review]
UseNeckoOriginAttributes::InheritOnHttpRedirect

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

See comment 18 if you like this idea.
Attachment #8813070 - Flags: feedback?(bugs)
Comment on attachment 8812722 [details] [diff] [review]
Part 3: remove the assert on firstPartyDomain v2

I still think we should fix bug 1300671 and then have better assertions here.
But fine for now.
Attachment #8812722 - Flags: review?(bugs) → review+
Comment on attachment 8813070 [details] [diff] [review]
UseNeckoOriginAttributes::InheritOnHttpRedirect

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

See comment 18 if you like this idea.
Attachment #8813070 - Flags: feedback?(bugs)
Posted patch Patch.Splinter Review
Merged into one.
Attachment #8808555 - Attachment is obsolete: true
Attachment #8808934 - Attachment is obsolete: true
Attachment #8812722 - Attachment is obsolete: true
Attachment #8816029 - Flags: review+
Pushed by yhuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66d8dfee23ca
Remove the assertion of FirstPartyDomain should be empty in HTTP redirect. r=smaug
https://hg.mozilla.org/mozilla-central/rev/66d8dfee23ca
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.