Bug 96247 - Improve console error messages when 'document.domain' blocks cross-origin script access.
Summary: Improve console error messages when 'document.domain' blocks cross-origin scr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords: WebExposed
: 14820 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-09-10 01:42 PDT by Mike West
Modified: 2013-03-27 22:55 PDT (History)
4 users (show)

See Also:


Attachments
Patch (10.81 KB, patch)
2012-09-10 12:27 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (11.19 KB, patch)
2012-09-10 13:33 PDT, Mike West
no flags Details | Formatted Diff | Diff
Rebaseline. (63.82 KB, patch)
2012-09-11 09:35 PDT, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2012-09-10 01:42:23 PDT
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.
Comment 1 Mike West 2012-09-10 12:27:25 PDT
Created attachment 163181 [details]
Patch
Comment 2 Mike West 2012-09-10 12:30:41 PDT
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!
Comment 3 Adam Barth 2012-09-10 12:42:18 PDT
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.
Comment 4 Adam Barth 2012-09-10 12:43:04 PDT
> 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.
Comment 5 Adam Barth 2012-09-10 12:46:02 PDT
> 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...
Comment 6 Mike West 2012-09-10 13:33:07 PDT
Created attachment 163195 [details]
Patch
Comment 7 Mike West 2012-09-10 13:34:01 PDT
(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.
Comment 8 Adam Barth 2012-09-10 13:39:48 PDT
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"
Comment 9 Mike West 2012-09-10 13:50:40 PDT
(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.
Comment 10 WebKit Review Bot 2012-09-11 01:56:33 PDT
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
Comment 11 Mike West 2012-09-11 01:59:58 PDT
Cool. I'll rebaseline these tests sometime today and ask for CQ once that's done.
Comment 12 Mike West 2012-09-11 09:35:18 PDT
Created attachment 163385 [details]
Rebaseline.
Comment 13 Mike West 2012-09-11 09:35:51 PDT
(In reply to comment #12)
> Created an attachment (id=163385) [details]
> Rebaseline.

Alright. Let's see if that covers things.
Comment 14 Mike West 2012-09-11 10:03:23 PDT
Comment on attachment 163385 [details]
Rebaseline.

Bots seem happy with the rebaseline. CQ?
Comment 15 Adam Barth 2012-09-11 10:45:09 PDT
Comment on attachment 163385 [details]
Rebaseline.

Sure.  I bet we're going to need to rebaseline the non-Chromium ports as well.
Comment 16 WebKit Review Bot 2012-09-11 11:12:27 PDT
Comment on attachment 163385 [details]
Rebaseline.

Clearing flags on attachment: 163385

Committed r128208: <http://trac.webkit.org/changeset/128208>
Comment 17 WebKit Review Bot 2012-09-11 11:12:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Mike West 2012-09-11 11:29:53 PDT
(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?
Comment 20 Mike West 2013-03-27 22:55:37 PDT
*** Bug 14820 has been marked as a duplicate of this bug. ***