Require the entry document to have the same origin as the open()d document

Require the entry document to have the same origin as the open()d document

This implements step 3) of https://html.spec.whatwg.org/#dom-document-open

BUG=579493
R=philipj@opera.com,mkwst@chromium.org

Committed: https://crrev.com/5ff185ee69963f7e749130dce11205021b9f1671
Cr-Commit-Position: refs/heads/master@{#372330}

[jochen (gone - plz use gerrit), 2016-01-20 13:46:46]

see also https://github.com/whatwg/html/issues/536

[philipj_slow, 2016-01-20 13:59:48]

OK, so the spec incantation is "Change the document's address to the address of
the responsible document specified by the entry settings object."

However, Document::open will also copy ownerDocument->cookieURL() and
ownerDocument->securityOrigin(). I'm not sure what the cookie URL even is, but
AFAICT the origin is actually set in
https://html.spec.whatwg.org/multipage/webappapis.html#set-up-a-browsing-cont...
and that isn't using the entry settings object.

BTW, maybe rename ownerDocument in Document.cpp to match the new meaning, or
pass in two documents if that is in fact necessary?

[Mike West, 2016-01-20 15:01:02]

https://codereview.chromium.org/1611523002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1611523002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/TestExpectations:1482: crbug.com/579493
http/tests/security/xss-DENIED-xsl-document-securityOrigin.xml [ Timeout ]
Firefox has pretty different behavior than we do for this test. I'd really like
to just align with their behavior if possible, though I honestly don't
understand what the test is doing or why. :/

[jochen (gone - plz use gerrit), 2016-01-22 09:18:50]

On 2016/01/20 at 13:59:48, philipj wrote:
> OK, so the spec incantation is "Change the document's address to the address
of the responsible document specified by the entry settings object."
> 
> However, Document::open will also copy ownerDocument->cookieURL() and
ownerDocument->securityOrigin(). I'm not sure what the cookie URL even is, but
AFAICT the origin is actually set in
https://html.spec.whatwg.org/multipage/webappapis.html#set-up-a-browsing-cont...
and that isn't using the entry settings object.


in any case, it can't be the current value, because "one level up the stack" is
nowhere defined in html or ecmascript.

> 
> BTW, maybe rename ownerDocument in Document.cpp to match the new meaning, or
pass in two documents if that is in fact necessary?

let's wait for the whatwg issue to be resolved

[philipj_slow, 2016-01-25 15:10:20]

On 2016/01/22 09:18:50, jochen wrote:
> On 2016/01/20 at 13:59:48, philipj wrote:
> > OK, so the spec incantation is "Change the document's address to the address
> of the responsible document specified by the entry settings object."
> > 
> > However, Document::open will also copy ownerDocument->cookieURL() and
> ownerDocument->securityOrigin(). I'm not sure what the cookie URL even is, but
> AFAICT the origin is actually set in
>
https://html.spec.whatwg.org/multipage/webappapis.html#set-up-a-browsing-cont...
> and that isn't using the entry settings object.
> 
> 
> in any case, it can't be the current value, because "one level up the stack"
is
> nowhere defined in html or ecmascript.
> 
> > 
> > BTW, maybe rename ownerDocument in Document.cpp to match the new meaning, or
> pass in two documents if that is in fact necessary?
> 
> let's wait for the whatwg issue to be resolved

Don't be afraid to ping as much as needed, because spec issues tend to sit if
nobody feels ownership for an area and nobody knows exactly what to do.

[jochen (gone - plz use gerrit), 2016-01-29 08:01:37]

ptal

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/HTMLDocument/document-open-return-value.html
(left):

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/HTMLDocument/document-open-return-value.html:17:
<iframe src="data:text/html,FAILURE"></iframe>
data: will fail the canAccess() check. This test is about document.open()
returning a non-void result, so just changing the origin to about:blank makes
the test test what it's supposed to test again

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/parser/tokenizer-close-during-document-write.html
(left):

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/parser/tokenizer-close-during-document-write.html:5:
<iframe src="data:text/html,"></iframe>
same here

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/aboutBlank/security-context-grandchildren-lexical-expected.txt
(right):

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/aboutBlank/security-context-grandchildren-lexical-expected.txt:14:
document.URL =
http://127.0.0.1:8000/security/aboutBlank/security-context-grandchildren-lexi...
these are from callingDOMWindow -> enteredDOMWindow changes

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2862: if (enteredDocument &&
!securityOrigin()->canAccess(enteredDocument->securityOrigin())) {
without this, the ASSERT(m_parser) below will fail if open() refuses to open()

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.idl (right):

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.idl:116: [CallWith=FirstWindow,
CustomElementCallbacks, RaisesException] void write(DOMString... text);
write() uses the window to call open() internally, so I had to update it here as
well

[philipj_slow, 2016-01-29 08:39:18]

These changes LGTM, but I'm not confident I understand all of the security
implications. In addition to you and Mike, do we have someone who could
sanity-check this? Or if you're sure this is safe, I'll take your word for it.

Can you also link to the relevant spec sections in the commit message and
possible also the code itself?

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/HTMLDocument/document-open-return-value.html
(left):

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/HTMLDocument/document-open-return-value.html:17:
<iframe src="data:text/html,FAILURE"></iframe>
On 2016/01/29 08:01:37, jochen wrote:
> data: will fail the canAccess() check. This test is about document.open()
> returning a non-void result, so just changing the origin to about:blank makes
> the test test what it's supposed to test again

I guess that data: URLs in an <iframe> already have this behavior in Firefox? A
certain risk of regressions here, can you also check IE/Edge?

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:476: void open(Document*
enteredDocument, ExceptionState&);
Can you document what the enteredDocument is? The further away from bindings we
get the less obvious this probably is. Calling it entryDocument would strengthen
the association with "entry settings object" but instead be inconsistent with
the terminology in bindings.

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.idl (right):

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.idl:116: [CallWith=FirstWindow,
CustomElementCallbacks, RaisesException] void write(DOMString... text);
On 2016/01/29 08:01:37, jochen wrote:
> write() uses the window to call open() internally, so I had to update it here
as
> well

FirstWindow isn't documented in https://www.chromium.org/blink/webidl, is a
Window that holds the responsible document of the entry settings object?

[jochen (gone - plz use gerrit), 2016-01-29 09:37:17]

Description was changed from

==========
The result of document.open() should copy its properties from the entry doc

Before, it used the calling context's document, which is, however, not
specified.

BUG=579493
R=philipj@opera.com,mkwst@chromium.org
==========

to

==========
Require the entry document to have the same origin as the open()d document

This implements step 3) of https://html.spec.whatwg.org/#dom-document-open

BUG=579493
R=philipj@opera.com,mkwst@chromium.org
==========

[jochen (gone - plz use gerrit), 2016-01-29 09:40:25]

not sure who but Mike knows about this stuff :-/

I don't have security concerns, however, as we just tighten the implementation,
so the only thing that could happen is that we break sites that didn't work in
Firefox anyways.

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/HTMLDocument/document-open-return-value.html
(left):

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/HTMLDocument/document-open-return-value.html:17:
<iframe src="data:text/html,FAILURE"></iframe>
On 2016/01/29 at 08:39:18, philipj_UTC7 wrote:
> On 2016/01/29 08:01:37, jochen wrote:
> > data: will fail the canAccess() check. This test is about document.open()
> > returning a non-void result, so just changing the origin to about:blank
makes
> > the test test what it's supposed to test again
> 
> I guess that data: URLs in an <iframe> already have this behavior in Firefox?
A certain risk of regressions here, can you also check IE/Edge?

no, data: is treated differently in chrome and firefox. Firefox treats data:
like about:blank (i.e. its origin is an alias to the containing document's
origin). In Chrome, data: is treated as a different origin. This test used to
only pass because of flags passed to layout tests to disable those security
checks. The test would fail when run in Chrome.

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:476: void open(Document*
enteredDocument, ExceptionState&);
On 2016/01/29 at 08:39:18, philipj_UTC7 wrote:
> Can you document what the enteredDocument is? The further away from bindings
we get the less obvious this probably is. Calling it entryDocument would
strengthen the association with "entry settings object" but instead be
inconsistent with the terminology in bindings.

added a comment.

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.idl (right):

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.idl:116: [CallWith=FirstWindow,
CustomElementCallbacks, RaisesException] void write(DOMString... text);
On 2016/01/29 at 08:39:18, philipj_UTC7 wrote:
> On 2016/01/29 08:01:37, jochen wrote:
> > write() uses the window to call open() internally, so I had to update it
here as
> > well
> 
> FirstWindow isn't documented in https://www.chromium.org/blink/webidl, is a
Window that holds the responsible document of the entry settings object?

done.

I didn't document ActiveWindow, as I'll delete this soon.

[philipj_slow, 2016-01-29 10:21:12]

Even LGTMer, but I'll leave the final say to Mike.

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/HTMLDocument/document-open-return-value.html
(left):

https://codereview.chromium.org/1611523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/HTMLDocument/document-open-return-value.html:17:
<iframe src="data:text/html,FAILURE"></iframe>
On 2016/01/29 09:40:24, jochen wrote:
> On 2016/01/29 at 08:39:18, philipj_UTC7 wrote:
> > On 2016/01/29 08:01:37, jochen wrote:
> > > data: will fail the canAccess() check. This test is about document.open()
> > > returning a non-void result, so just changing the origin to about:blank
> makes
> > > the test test what it's supposed to test again
> > 
> > I guess that data: URLs in an <iframe> already have this behavior in
Firefox?
> A certain risk of regressions here, can you also check IE/Edge?
> 
> no, data: is treated differently in chrome and firefox. Firefox treats data:
> like about:blank (i.e. its origin is an alias to the containing document's
> origin). In Chrome, data: is treated as a different origin. This test used to
> only pass because of flags passed to layout tests to disable those security
> checks. The test would fail when run in Chrome.

Oh right, so this won't break sites that try to document.open their data:
iframes. Good.

[Mike West, 2016-01-29 10:28:30]

> > I guess that data: URLs in an <iframe> already have this behavior in
Firefox? A certain risk of regressions here, can you also check IE/Edge?
> 
> no, data: is treated differently in chrome and firefox. Firefox treats data:
like about:blank (i.e. its origin is an alias to the containing document's
origin). In Chrome, data: is treated as a different origin. This test used to
only pass because of flags passed to layout tests to disable those security
checks. The test would fail when run in Chrome.

It's marginally more complicated: `data:` and `about:blank` in Firefox inherit
the origin of the document responsible for the navigation. That is, if
`crossorigin.com` nested in `example.com` navigates
`window.top.frames[2].contentWindow.location = 'data:whatever'`, that frame will
be `crossorigin.com`. This is what the spec says we should do, and it's not
crazy. It's just hard to do correctly in Blink, and there doesn't appear to be
much value to outweigh the risk that we'd screw it up.

[Mike West, 2016-01-29 10:32:27]

https://codereview.chromium.org/1611523002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/HTMLDocument/document-open-return-value.html
(right):

https://codereview.chromium.org/1611523002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/HTMLDocument/document-open-return-value.html:17:
<iframe></iframe>
If you wanted to keep `FAILURE` visible in the failure case, you could use
`srcdoc="FAILURE"`.

https://codereview.chromium.org/1611523002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/aboutBlank/security-context-grandchildren-lexical-expected.txt
(right):

https://codereview.chromium.org/1611523002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/aboutBlank/security-context-grandchildren-lexical-expected.txt:15:
document.baseURI =
http://127.0.0.1:8000/security/aboutBlank/security-context-grandchildren-lexi...
Why did these change, here and in the other aboutBlank tests?

[Mike West, 2016-01-29 10:33:05]

On 2016/01/29 at 10:32:27, Mike West wrote:
>
https://codereview.chromium.org/1611523002/diff/60001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/fast/dom/HTMLDocument/document-open-return-value.html
(right):
> 
>
https://codereview.chromium.org/1611523002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/fast/dom/HTMLDocument/document-open-return-value.html:17:
<iframe></iframe>
> If you wanted to keep `FAILURE` visible in the failure case, you could use
`srcdoc="FAILURE"`.
> 
>
https://codereview.chromium.org/1611523002/diff/60001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/http/tests/security/aboutBlank/security-context-grandchildren-lexical-expected.txt
(right):
> 
>
https://codereview.chromium.org/1611523002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/security/aboutBlank/security-context-grandchildren-lexical-expected.txt:15:
document.baseURI =
http://127.0.0.1:8000/security/aboutBlank/security-context-grandchildren-lexi...
> Why did these change, here and in the other aboutBlank tests?

Nevermind: "these are from callingDOMWindow -> enteredDOMWindow changes". LGTM,
thanks.

[jochen (gone - plz use gerrit), 2016-01-29 11:23:40]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-01-29 11:23:40]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1611523002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-01-29 11:24:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1611523002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1611523002/60001

[commit-bot: I haz the power, 2016-01-29 11:29:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-29 11:30:37]

Description was changed from

==========
Require the entry document to have the same origin as the open()d document

This implements step 3) of https://html.spec.whatwg.org/#dom-document-open

BUG=579493
R=philipj@opera.com,mkwst@chromium.org
==========

to

==========
Require the entry document to have the same origin as the open()d document

This implements step 3) of https://html.spec.whatwg.org/#dom-document-open

BUG=579493
R=philipj@opera.com,mkwst@chromium.org

Committed: https://crrev.com/5ff185ee69963f7e749130dce11205021b9f1671
Cr-Commit-Position: refs/heads/master@{#372330}
==========

[commit-bot: I haz the power, 2016-01-29 11:30:38]

Patchset 4 (id:??) landed as
https://crrev.com/5ff185ee69963f7e749130dce11205021b9f1671
Cr-Commit-Position: refs/heads/master@{#372330}