Bug 96247 – Improve console error messages when 'document.domain' blocks cross-origin script access.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

From discussion in bug #64079:

> What would you like to see added? I'm happy to make more changes now while I'm already rebasing tests. :)

One corner case that would be good to cover is when document.domain is set.  There are some counter-intuitive cases there.  For example, http://www.example.com/foo might not be able to access http://www.example.com/bar if bar sets document.domain to example.com.

The even stranger case is when http://example.com/foo can't talk with http://www.example.com/bar even after bar sets document.domain to example.com.  The rule is that either the both need to have set document.domain or neither of them can have set it.

[Mike West]

Created attachment 163181 [details]
Patch

[Mike West]

Hey Adam! Mind taking a look at this? I'm a bit worried that we're going to end up replicating more and more of SecurityOrigin into this function, but for the moment, I think it makes sense to keep it here.

I expect there to be more tests to rebaseline, but I'm going to let the bots tell me which ones... my computer at home is slooooow. :)

Also, I'm not sure if the ASSERT is appropriate, but it seemed reasonable to add. WDYT?

Thanks!

[Adam Barth]

Comment on attachment 163181 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163181&action=review

> Source/WebCore/page/DOMWindow.cpp:1772
> +    // 'document.domain' errors.

Is there a way to make this code more readable?  It looks like a big wall of complicated logic.  :(

By the way, you don't need to use makeString any more.  + is fast these days.

[Adam Barth]

> Also, I'm not sure if the ASSERT is appropriate, but it seemed reasonable to add. WDYT?

The ASSERT is probably a good idea.  It would have caught a recent bug, actually.

[Adam Barth]

> I'm a bit worried that we're going to end up replicating more and more of SecurityOrigin into this function, but for the moment, I think it makes sense to keep it here.

Yeah.  This is something to keep an eye on.  There's an argument that says we should generate the error message at the same time we do the access check instead of re-checking everything...

[Mike West]

Created attachment 163195 [details]
Patch

[Mike West]

(In reply to comment #3)
> (From update of attachment 163181 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163181&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:1772
> > +    // 'document.domain' errors.
> 
> Is there a way to make this code more readable?  It looks like a big wall of complicated logic.  :(

Slightly restructured, with named variables for the origins. Is that any better?

> By the way, you don't need to use makeString any more.  + is fast these days.

Done.

[Adam Barth]

Comment on attachment 163195 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163195&action=review

Much nicer.  Thanks.

> Source/WebCore/page/DOMWindow.cpp:1773
> +    SecurityOrigin* requestorOrigin = activeWindow->document()->securityOrigin();
> +    SecurityOrigin* requesteeOrigin = document()->securityOrigin();

We usually call these "active" and "target"

[Mike West]

(In reply to comment #8)
> > Source/WebCore/page/DOMWindow.cpp:1773
> > +    SecurityOrigin* requestorOrigin = activeWindow->document()->securityOrigin();
> > +    SecurityOrigin* requesteeOrigin = document()->securityOrigin();
> 
> We usually call these "active" and "target"

Ah, much better. Thanks!

Let's see how the bot run goes. There are quite a few more tests that use document.domain; I expect I'll need to rebaseline some of them before landing this.

[WebKit Review Bot]

Comment on attachment 163195 [details]
Patch

Attachment 163195 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13818276

New failing tests:
http/tests/security/dataURL/xss-DENIED-to-data-url-sub-frame-2-level.html
http/tests/security/view-source-no-javascript-url.html
http/tests/security/dataURL/xss-DENIED-to-data-url-in-foreign-domain-subframe.html
http/tests/security/dataURL/xss-DENIED-from-javascript-url-window-open.html
http/tests/security/dataURL/xss-DENIED-to-data-url-sub-frame.html
http/tests/security/dataURL/xss-DENIED-from-data-url-in-foreign-domain-subframe.html
http/tests/security/dataURL/xss-DENIED-to-data-url-window-open.html
http/tests/security/dataURL/xss-DENIED-from-data-url-in-foreign-domain-window-open.html
http/tests/security/dataURL/xss-DENIED-to-data-url-in-foreign-domain-subframe-location-change.html
http/tests/security/dataURL/xss-DENIED-to-data-url-sub-frame-uppercase.html
http/tests/security/inactive-document-with-empty-security-origin.html
http/tests/security/window-named-proto.html
http/tests/security/dataURL/xss-DENIED-from-data-url-sub-frame.html
http/tests/security/cross-frame-access-protocol.html
http/tests/security/dataURL/xss-DENIED-to-data-url-in-foreign-domain-window-open.html
http/tests/security/dataURL/xss-DENIED-from-data-url-sub-frame-2-level.html

[Mike West]

Cool. I'll rebaseline these tests sometime today and ask for CQ once that's done.

[Mike West]

Created attachment 163385 [details]
Rebaseline.

[Mike West]

(In reply to comment #12)
> Created an attachment (id=163385) [details]
> Rebaseline.

Alright. Let's see if that covers things.

[Mike West]

Comment on attachment 163385 [details]
Rebaseline.

Bots seem happy with the rebaseline. CQ?

[Adam Barth]

Comment on attachment 163385 [details]
Rebaseline.

Sure.  I bet we're going to need to rebaseline the non-Chromium ports as well.

[WebKit Review Bot]

Comment on attachment 163385 [details]
Rebaseline.

Clearing flags on attachment: 163385

Committed r128208: <http://trac.webkit.org/changeset/128208>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mike West]

(In reply to comment #15)
> (From update of attachment 163385 [details])
> Sure.  I bet we're going to need to rebaseline the non-Chromium ports as well.

I'd like to do that; I'd like to do it before I land changes, really. I don't suppose there's a EFL or GTK bot that would give me test results?

[Adam Barth]

http://build.webkit.org/TestFailures/garden-o-matic.html?platform=gtk and http://build.webkit.org/TestFailures/garden-o-matic.html?platform=qt might be helpful in point you in the right direction.

[Mike West]

*** Bug 14820 has been marked as a duplicate of this bug. ***