Summary: | Improve console error messages when 'document.domain' blocks cross-origin script access. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike West <mkwst> | ||||||||
Component: | WebCore Misc. | Assignee: | Mike West <mkwst> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, bhagyalaxmi.dash, dglazkov, webkit.review.bot | ||||||||
Priority: | P2 | Keywords: | WebExposed | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Mike West
2012-09-10 01:42:23 PDT
Created attachment 163181 [details]
Patch
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 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. > 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.
> 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...
Created attachment 163195 [details]
Patch
(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 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" (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 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 Cool. I'll rebaseline these tests sometime today and ask for CQ once that's done. Created attachment 163385 [details]
Rebaseline.
(In reply to comment #12) > Created an attachment (id=163385) [details] > Rebaseline. Alright. Let's see if that covers things. Comment on attachment 163385 [details]
Rebaseline.
Bots seem happy with the rebaseline. CQ?
Comment on attachment 163385 [details]
Rebaseline.
Sure. I bet we're going to need to rebaseline the non-Chromium ports as well.
Comment on attachment 163385 [details] Rebaseline. Clearing flags on attachment: 163385 Committed r128208: <http://trac.webkit.org/changeset/128208> All reviewed patches have been landed. Closing bug. (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? 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. *** Bug 14820 has been marked as a duplicate of this bug. *** |