1315602 - Remove the assertion of FirstPartyDomain should be empty in HTTP redirect

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

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

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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 3

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: change the assert. v2

compare docshellAttrs.mFirstPartyDomain with loadInfo->TriggeringPrincipal().mFirstPartyDomain.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(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

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: test on iframe in window.open.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1.1 : assert when firstPartyDomain is not empty.

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

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3 : only assert when firstPartyDomain is not empty. v2

> 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

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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?

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3 : only assert when firstPartyDomain is not empty. v3

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: remove the assert on firstPartyDomain v2

see explanation on comment 16.

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: UseNeckoOriginAttributes::InheritOnHttpRedirect

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

Comment 19

[Arthur Edelstein [:arthur]]

(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?

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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.

Comment 21

[Arthur Edelstein [:arthur]]

(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.

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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.

Comment 23

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 24

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8813070 [details] [diff] [review]
UseNeckoOriginAttributes::InheritOnHttpRedirect

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

See comment 18 if you like this idea.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 26

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8813070 [details] [diff] [review]
UseNeckoOriginAttributes::InheritOnHttpRedirect

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

See comment 18 if you like this idea.

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

Merged into one.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66d8dfee23ca
Remove the assertion of FirstPartyDomain should be empty in HTTP redirect. r=smaug

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/66d8dfee23ca