Bug 153598

<rdar://problem/24391483>

Created attachment 275361 [details] Patch

Comment on attachment 275361 [details] Patch Attachment 275361 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1078052 New failing tests: http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr.html fast/workers/worker-inherits-csp-blocks-xhr.html

Created attachment 275362 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 275361 [details] Patch Attachment 275361 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1078057 New failing tests: http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr.html fast/workers/worker-inherits-csp-blocks-xhr.html

Created attachment 275363 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 275361 [details] Patch Attachment 275361 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1078054 New failing tests: http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr.html fast/workers/worker-inherits-csp-blocks-xhr.html

Created attachment 275365 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5

Comment on attachment 275361 [details] Patch Attachment 275361 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1078061 New failing tests: http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr.html fast/workers/worker-inherits-csp-blocks-xhr.html

Created attachment 275366 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 275361 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275361&action=review This looks great! You need to correct the test expectations, but otherwise this is ready to go. > Source/WebCore/xml/XMLHttpRequest.cpp:-500 > - // FIXME: Convert this to check the isolated world's Content Security Policy once webkit.org/b/104520 is solved. Is this comment about the isolated world's CSP still relevant? > Source/WebCore/xml/XMLHttpRequest.cpp:570 > + // FIXME: Should this be throwing an exception? I know this is just moved code, but I don't understand this question. Is this question about throwing DOM exceptions? We don't use C++ exception handling. > LayoutTests/ChangeLog:21 > + - Added an additional call to XMLHttpRequest.send() to make existing test work with the changes. Looks like you missed "worker-inherits-csp-blocks-xhr.html", which was used to seeing error code 18, but now gets 19 (per your change): -PASS threw exception Error: SecurityError: DOM Exception 18. +FAIL should throw 18. Threw exception Error: NetworkError: DOM Exception 19. > LayoutTests/ChangeLog:22 > + It looks like "http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr.html" is also unhappy, though it's not clear to me if this is an expected outcome of your change or not.

Created attachment 275413 [details] Patch

(In reply to comment #11) > Comment on attachment 275361 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=275361&action=review > > This looks great! You need to correct the test expectations, but otherwise > this is ready to go. > > > Source/WebCore/xml/XMLHttpRequest.cpp:-500 > > - // FIXME: Convert this to check the isolated world's Content Security Policy once webkit.org/b/104520 is solved. > > Is this comment about the isolated world's CSP still relevant? I'm unfamiliar with the isolated world CSP. The bug is still open though. > > Source/WebCore/xml/XMLHttpRequest.cpp:570 > > + // FIXME: Should this be throwing an exception? > > I know this is just moved code, but I don't understand this question. Is > this question about throwing DOM exceptions? We don't use C++ exception > handling. Don't know. This might very well be the related bug: https://bugs.webkit.org/show_bug.cgi?id=97654 And as the discussion there indicates, certain security-sensitive information should not be exposed to JavaScript. > > LayoutTests/ChangeLog:21 > > + - Added an additional call to XMLHttpRequest.send() to make existing test work with the changes. > > Looks like you missed "worker-inherits-csp-blocks-xhr.html", which was used > to seeing error code 18, but now gets 19 (per your change): > > -PASS threw exception Error: SecurityError: DOM Exception 18. > +FAIL should throw 18. Threw exception Error: NetworkError: DOM Exception 19. Saw this one too now. I'll obsolete my current patch and fix this one. > > LayoutTests/ChangeLog:22 > > + > > It looks like > "http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr.html" is > also unhappy, though it's not clear to me if this is an expected outcome of > your change or not. Fixed.

Created attachment 275417 [details] Patch

Comment on attachment 275417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275417&action=review > Source/WebCore/xml/XMLHttpRequest.cpp:-500 > - // FIXME: Convert this to check the isolated world's Content Security Policy once webkit.org/b/104520 is solved. I think you should keep this comment, just move it to your new location. > Source/WebCore/xml/XMLHttpRequest.cpp:570 > + // FIXME: Should this be throwing an exception? I think you should get rid of this comment, or reference the Bug 97654 here.

Comment on attachment 275417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275417&action=review > Source/WebCore/ChangeLog:15 > + - Moved the CSP check from open to initSend and changed the error type from Security to Network. open => XMLHttpRequest::open() initSend => XMLHttpRequest::initSend() (By using this syntax it makes it clear to a reader that you are referring to member functions of XMLHttpRequest). This sentence is just a summary of the code changes made in the patch. The purpose of a ChangeLog is to explain "why" a change(s) were made unless such a change is mechanical and obvious. Although bug 153598, comment 0 describes the motivation for the changes in this patch we should repeat the reasoning in the ChangeLog message for convenience. In particular, we should explain that the reason we are moving the CSP check and changing the exception type is to conform to section connect-src of the Content Security Policy Level 2 spec. We should also reference the spec and its date. >> Source/WebCore/xml/XMLHttpRequest.cpp:570 >> + // FIXME: Should this be throwing an exception? > > I think you should get rid of this comment, or reference the Bug 97654 here. Please remove this FIXME comment. The CSP Level 2/3 spec. explicitly tells us that we should throw this exception. > LayoutTests/ChangeLog:28 > + - Added an additional call to XMLHttpRequest.send() to make existing tests work with the changes. Also entered the expected outcome since the debug output now says .send instead of .open. Nit: It is an unwritten convention that we wrap ChangeLog content to about 100 characters per line. > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-open-allowed.html:2 > +<!DOCTYPE html> > +<html> This test is unnecessary given that we already perform an identical test as part of file LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html. Please remove this file and its expected result. > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked-expected.txt:3 > +Pass > +Pass This output is ambiguous. We should emit a more meaningful message when a sub-test passes/fails to help a person understand the significance of the output, especially should one or both of these sub-tests fail in the future, without requiring a person to read LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html. Although a person will ultimately need to read LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html should one or more of the sub-test fail in the future in order to diagnose the failure, providing output that is more than just "Pass"/"Fail" will help expedite such diagnosis by giving them some context of cause of the failure. > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:8 > +<script> > +if (window.testRunner) > + testRunner.dumpAsText(); > +</script> I suggest that we write this test using the functionality provided by js-test-pre.js/js-test-post.js to simplify the code in this test and make the output of this test consistent with the output of other text-only tests. Among the simplifications we can make is the replace this <script> with <script src="/js-test-resources/js-test-pre.js"></script> because js-test-pre.js calls testRunner.dumpAsText() for us. > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:11 > +<pre id="console"></pre> If we use js-test-pre.js then we can remove this element as it will create an element with this id for us. > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:16 > +function log(msg) > +{ > + document.getElementById("console").appendChild(document.createTextNode(msg + "\n")); > +} If we use js-test-pre.js then we can remove this function. > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:28 > +var xhr, > + handler = function() {}; > +try { > + xhr = new XMLHttpRequest; > + xhr.open("GET", "http://localhost:8000/xmlhttprequest/resources/get.txt", true); > + xhr.onreadystatechange = handler; > + xhr.withCredentials = true; > + log("Pass"); > +} catch(e) { > + log("Fail"); > +} I do not see the need for the xhr.onreadystatechange hander (which is just the empty function). I also do not see the need to set the withCredentials flag given the purpose of this test and would remove the setting of this flag. Applying these changes together and using shouldNotThrow() from js-test-pre.js we can simplify this code to read: description("This tests that a Content Security Policy violation for an XHR is triggered when calling XMLHttpRequest.send()."); var xhr = new XMLHttpRequest; shouldNotThrow('xhr.open("GET", "http://localhost:8000/xmlhttprequest/resources/get.txt", true)'); // Asynchronous request Notice that using shouldNotThrow() will also help make the test output less ambiguous. > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:34 > +try { > + xhr.send(); > + log("Fail"); > +} catch(e) { > + log("Pass"); > +} Using shouldThrow() from js-test-pre.js we can simplify this code to read: shouldThrow("xhr.send()", DOMException.NETWORK_ERR); Obviously this also has the benefit of ensuring that the exception thrown is a DOMException.NETWORK_ERR. > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:35 > +</script> After this line we will need to add: <script src="/js-test-resources/js-test-post.js"></script>

Comment on attachment 275417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275417&action=review >> LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:11 >> +<pre id="console"></pre> > > If we use js-test-pre.js then we can remove this element as it will create an element with this id for us. One nit: If you need the console output to be below other items in the test (e.g., if position matters or you are trying to retain click targets) you might want to include a specific "console" element so you can control where the output goes. >> LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:16 >> +} > > If we use js-test-pre.js then we can remove this function. Yes! Good point.

Thanks for the review, Dan! (In reply to comment #16) > Comment on attachment 275417 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=275417&action=review > > > Source/WebCore/ChangeLog:15 > > + - Moved the CSP check from open to initSend and changed the error type from Security to Network. > > open => XMLHttpRequest::open() > initSend => XMLHttpRequest::initSend() > > (By using this syntax it makes it clear to a reader that you are referring > to member functions of XMLHttpRequest). > > This sentence is just a summary of the code changes made in the patch. The > purpose of a ChangeLog is to explain "why" a change(s) were made unless such > a change is mechanical and obvious. Although bug 153598, comment 0 describes > the motivation for the changes in this patch we should repeat the reasoning > in the ChangeLog message for convenience. In particular, we should explain > that the reason we are moving the CSP check and changing the exception type > is to conform to section connect-src of the Content Security Policy Level 2 > spec. We should also reference the spec and its date. OK, I'll do that in the upcoming patch. > >> Source/WebCore/xml/XMLHttpRequest.cpp:570 > >> + // FIXME: Should this be throwing an exception? > > > > I think you should get rid of this comment, or reference the Bug 97654 here. > > Please remove this FIXME comment. The CSP Level 2/3 spec. explicitly tells > us that we should throw this exception. New patch removes the FIXME and just mentions Bug 97654. > > LayoutTests/ChangeLog:28 > > + - Added an additional call to XMLHttpRequest.send() to make existing tests work with the changes. Also entered the expected outcome since the debug output now says .send instead of .open. > > Nit: It is an unwritten convention that we wrap ChangeLog content to about > 100 characters per line. OK, I'll fix that. > > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-open-allowed.html:2 > > +<!DOCTYPE html> > > +<html> > > This test is unnecessary given that we already perform an identical test as > part of file > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src- > xmlhttprequest-send-blocked.html. Please remove this file and its expected > result. Will do. > > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked-expected.txt:3 > > +Pass > > +Pass > > This output is ambiguous. We should emit a more meaningful message when a > sub-test passes/fails to help a person understand the significance of the > output, especially should one or both of these sub-tests fail in the future, > without requiring a person to read > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src- > xmlhttprequest-send-blocked.html. Although a person will ultimately need to > read > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src- > xmlhttprequest-send-blocked.html should one or more of the sub-test fail in > the future in order to diagnose the failure, providing output that is more > than just "Pass"/"Fail" will help expedite such diagnosis by giving them > some context of cause of the failure. I will add explicit Pass comments. > > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:8 > > +<script> > > +if (window.testRunner) > > + testRunner.dumpAsText(); > > +</script> > > I suggest that we write this test using the functionality provided by > js-test-pre.js/js-test-post.js to simplify the code in this test and make > the output of this test consistent with the output of other text-only tests. > Among the simplifications we can make is the replace this <script> with > <script src="/js-test-resources/js-test-pre.js"></script> because > js-test-pre.js calls testRunner.dumpAsText() for us. OK, will do. > > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:11 > > +<pre id="console"></pre> > > If we use js-test-pre.js then we can remove this element as it will create > an element with this id for us. > > > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:16 > > +function log(msg) > > +{ > > + document.getElementById("console").appendChild(document.createTextNode(msg + "\n")); > > +} > > If we use js-test-pre.js then we can remove this function. > > > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:28 > > +var xhr, > > + handler = function() {}; > > +try { > > + xhr = new XMLHttpRequest; > > + xhr.open("GET", "http://localhost:8000/xmlhttprequest/resources/get.txt", true); > > + xhr.onreadystatechange = handler; > > + xhr.withCredentials = true; > > + log("Pass"); > > +} catch(e) { > > + log("Fail"); > > +} > > I do not see the need for the xhr.onreadystatechange hander (which is just > the empty function). I also do not see the need to set the withCredentials > flag given the purpose of this test and would remove the setting of this > flag. Applying these changes together and using shouldNotThrow() from > js-test-pre.js we can simplify this code to read: > > description("This tests that a Content Security Policy violation for an XHR > is triggered when calling XMLHttpRequest.send()."); > > var xhr = new XMLHttpRequest; > shouldNotThrow('xhr.open("GET", > "http://localhost:8000/xmlhttprequest/resources/get.txt", true)'); // > Asynchronous request > > Notice that using shouldNotThrow() will also help make the test output less > ambiguous. > > > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:34 > > +try { > > + xhr.send(); > > + log("Fail"); > > +} catch(e) { > > + log("Pass"); > > +} > > Using shouldThrow() from js-test-pre.js we can simplify this code to read: > > shouldThrow("xhr.send()", DOMException.NETWORK_ERR); This did not work. DOMException.NETWORK_ERR is just the number 19 whereas the CSP violation throws "Error: NetworkError: DOM Exception 19". > Obviously this also has the benefit of ensuring that the exception thrown is > a DOMException.NETWORK_ERR. I made sure to test that even though I couldn't get shouldThrow working. > > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:35 > > +</script> > > After this line we will need to add: <script > src="/js-test-resources/js-test-post.js"></script> New patch coming up.

Created attachment 275431 [details] Patch

Comment on attachment 275431 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275431&action=review > Source/WebCore/xml/XMLHttpRequest.cpp:571 > + // Regarding a more descriptive exception: https://bugs.webkit.org/show_bug.cgi?id=97654 I do not find this comment helpful and I disagree with the need to fix bug #97654 and expose the reason for the security violation in the exception itself. Please remove this comment. For completeness, I propose that we fix bug #114317 and provide source file, line and column numbers in console messages for CSP violations. > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:21 > +try { > + xhr.send(); > +} catch (e) { > + const expectedError = "NetworkError: DOM Exception 19"; > + if(e.message == expectedError) > + debug("PASS xhr.send() threw " + e.message); > + else > + debug("FAIL Expected error: " + expectedError + ", actual: " + e.message); > +} Have you tried: shouldThrow("xhr.send()", "'Error: NetworkError: DOM Exception 19'"); ?

Created attachment 275555 [details] Patch

Created attachment 275557 [details] Patch

shouldThrow("xhr.send()", "'Error: NetworkError: DOM Exception 19'"); … fixed it. Thanks!

Comment on attachment 275557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275557&action=review > LayoutTests/http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr-expected.txt:11 > +CONSOLE MESSAGE: XMLHttpRequest cannot load http://localhost:8000/security/isolatedWorld/resources/cross-origin-xhr.txt. Origin http://127.0.0.1:8000 is not allowed by Access-Control-Allow-Origin. > Tests that isolated worlds can have XHRs that the page's CSP wouldn't allow. > > On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". > > > XHR from main world > -PASS: XHR.open threw an exception. > +PASS: XHR.send threw an exception. > XHR from isolated world > -PASS: XHR.open did not throw an exception. > +PASS: XHR.send did not throw an exception. I did not expect to see a CORS warning in this test based on its description and the names of its sub tests "XHR from main world" and "XHR from isolated world". For your consideration I suggest that we make the sub-test "XHR from isolated world" perform a same-origin load and add two new sub-tests that perform a cross-origin XHR from the main world and isolated world, respectively. You may find it beneficial to make use of CGI script <http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/xmlhttprequest/resources/access-control-basic-allow.cgi> to test a cross-origin XHR load without causing a CORS warning. One example of a test that makes use of this CGI script can be seen at <http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/xmlhttprequest/access-control-basic-allow.html?rev=199022>.

Created attachment 275629 [details] Patch

Comment on attachment 275629 [details] Patch Dan, I mark this for another review since I made semi-significant changes to the structure of the test you and I discussed.

Comment on attachment 275629 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275629&action=review > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:13 > +shouldThrow("xhr.send()", "'Error: NetworkError: DOM Exception 19'"); I don't think this is the intent of the spec. Chrome and Firefox both do not do this. A fatal network error would not throw an exception here with an asynchronous xhr, but rather it would asynchronously call onerror. See https://bugs.webkit.org/show_bug.cgi?id=146706 for another example of something blocking asynchronous xhr and wanting to look like a network error.

Comment on attachment 275629 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275629&action=review >> LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-send-blocked.html:13 >> +shouldThrow("xhr.send()", "'Error: NetworkError: DOM Exception 19'"); > > I don't think this is the intent of the spec. Chrome and Firefox both do not do this. A fatal network error would not throw an exception here with an asynchronous xhr, but rather it would asynchronously call onerror. See https://bugs.webkit.org/show_bug.cgi?id=146706 for another example of something blocking asynchronous xhr and wanting to look like a network error. I would suggest a test like this, which passes in Firefox and I think is the intended behavior: if (window.testRunner) window.testRunner.waitUntilDone() var xhr = new XMLHttpRequest; xhr.onerror = function () { debug("onerror was called asynchronously as intended"); if (window.testRunner) testRunner.notifyDone(); }; shouldNotThrow('xhr.open("GET", "http://localhost:8000/xmlhttprequest/resources/get.txt", true)'); // Asynchronous request shouldNotThrow("xhr.send()", "'Error: NetworkError: DOM Exception 19'");

OK, I've done some testing with the code change in place. Here's where we stand: 1. We do not fire an error event for the CSP block. 2. We throw an exception (NetworkError with the code change) for both asynchronous and synchronous requests. We likely want to address both of these issues but the CSP level 2 spec and tests are currently ambiguous in the following ways: 1. http://www.w3.org/TR/CSP2/#directive-connect-src says we should block on any .send() that violates connect-src and act as if there was "a fatal network error." 2. http://www.w3.org/TR/CSP2/#connect-src-usage provides an example of a request that should fail based on .open(). 3. Safari Technology Preview is already passing the connect-src tests which suggests they accept blocking already on .open() and without firing an event: - http://w3c-test.org/content-security-policy/blink-contrib/connect-src-xmlhttprequest-blocked.sub.html - http://w3c-test.org/content-security-policy/blink-contrib/connect-src-xmlhttprequest-redirect-to-blocked.sub.html I suggest we land this patch to at least move the CSP block from .open to .send. Then we work with W3C WebAppSec to make the spec clear and implement accordingly. I've filed https://bugs.webkit.org/show_bug.cgi?id=156322 to track what we currently believe is the correct behavior for error events and exceptions.

Comment on attachment 275629 [details] Patch Requesting new review.

(In reply to comment #29) > OK, I've done some testing with the code change in place. Here's where we > stand: > 1. We do not fire an error event for the CSP block. I think we knew this. See Bug 153150 and 153155.

Comment on attachment 275629 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275629&action=review > Source/WebCore/xml/XMLHttpRequest.cpp:571 > + ec = NETWORK_ERR; I suggest that we add a FIXME comment above this line to explain that we should only throw a exception for a synchronous XHR, that we should be dispatching a DOM error event, and reference bug #156322. Maybe something like: FIXME: We should only throw an exception for a synchronous request and should dispatch a DOM error event so as to make this CSP violation indistinguishable from a XHR network error. See <https://bugs.webkit.org/show_bug.cgi?id=156322>. > LayoutTests/ChangeLog:18 > + Tests that XMLHttpRequest.send() is blocked if the URL voilates the connect-src directive in CSP. Nit: voilates => violates > LayoutTests/ChangeLog:30 > + Simplified the structure with unique funcion names to avoid confusion and race conditions. Nit: funcion => function > LayoutTests/http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr.html:24 > +var tests = { > + mainWorldTest : function() { > xhr(true); > }, > - function() { > - debug('XHR from isolated world'); > + isolatedWorldSameOriginTest : function() { > runTestInWorld(1, 'xhr', 'false'); > }, > -]; > -var currentTest = 0; > + isolatedWorldCrossOriginTest : function() { > + runTestInWorld(2, 'xhrCrossOrigin', 'false'); > + } > +}; We are underutilizing this dictionary. > LayoutTests/http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr.html:59 > switch (message.type) { > - case 'test-done': > - currentTest++; > - if (currentTest == tests.length) > + case 'test-main-world-xhr-done': > + if (!mainWorldTestDone) { > + mainWorldTestDone = true; > + debug("PASS Main world, same-origin XHR threw an exception."); > + tests.isolatedWorldSameOriginTest(); > + } > + break; > + case 'test-isolated-world-xhr-done': > + if (!isolatedWorldSameOriginTestDone) { > + isolatedWorldSameOriginTestDone = true; > + debug("PASS Isolated world, same-origin XHR did not throw an exception."); > + tests.isolatedWorldCrossOriginTest(); > + } > + break; > + case 'test-isolated-world-xhr-cross-origin-done': > + if (!isolatedWorldCrossOriginTestDone) { > + isolatedWorldCrossOriginTestDone = true; > + debug("PASS Main world, cross-origin XHR did not throw an exception."); > finishJSTest(); > - else > - tests[currentTest](); > + } > break; > - case 'debug': > - debug(message.message); > + case 'fail': > + testFailed('FAIL ' + message.message); > break; How did you come to the decision to make these changes as opposed to making use of the existing structure to add new sub-tests? I suggest that we make use of the existing test machinery because it seems generic (*), teach runTestInWorld() (**) how to pass arbitrary arguments to a function and teach xhr() to take another argument to determine whether it should make a same-origin or cross-origin request. (*) It seems sufficiently generic because it runs sub-tests that are defined as functions and each test function can window.postMessage() at least two types of messages "debug" and "fail" to print an arbitrary message and signal a test failure, respectively. (**) Maybe something like (I renamed runTestInWorld to invokeInWorld to better convey its purpose): function invokeInWorld(worldId, aFunction) { var script = aFunction.toString() + "; " + aFunction.name + "(" + Array.prototype.slice.call(arguments, 2).join(", ") + ");"; testRunner.evaluateScriptInIsolatedWorld(worldId, script); }

(In reply to comment #32) > (**) Maybe something like (I renamed runTestInWorld to invokeInWorld to > better convey its purpose): > > function invokeInWorld(worldId, aFunction) > { > var script = aFunction.toString() + "; " + aFunction.name + "(" + > Array.prototype.slice.call(arguments, 2).join(", ") + ");"; > testRunner.evaluateScriptInIsolatedWorld(worldId, script); > } Even better, we should make use of JSON.stringify() to preserve the original argument values: function invokeInWorld(worldId, aFunction) { var script = aFunction.toString() + "; " + aFunction.name + "(" + Array.prototype.slice.call(arguments, 2).map(JSON.stringify).join(", ") + ");"; testRunner.evaluateScriptInIsolatedWorld(worldId, script); }

(In reply to comment #32) > Comment on attachment 275629 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=275629&action=review > > > Source/WebCore/xml/XMLHttpRequest.cpp:571 > > + ec = NETWORK_ERR; > > I suggest that we add a FIXME comment above this line to explain that we > should only throw a exception for a synchronous XHR, that we should be > dispatching a DOM error event, and reference bug #156322. Maybe something > like: > > FIXME: We should only throw an exception for a synchronous request and > should dispatch a DOM error event so as to make this CSP violation > indistinguishable from a XHR network error. See > <https://bugs.webkit.org/show_bug.cgi?id=156322>. > > > LayoutTests/ChangeLog:18 > > + Tests that XMLHttpRequest.send() is blocked if the URL voilates the connect-src directive in CSP. > > Nit: voilates => violates > > > LayoutTests/ChangeLog:30 > > + Simplified the structure with unique funcion names to avoid confusion and race conditions. > > Nit: funcion => function > > > LayoutTests/http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr.html:24 > > +var tests = { > > + mainWorldTest : function() { > > xhr(true); > > }, > > - function() { > > - debug('XHR from isolated world'); > > + isolatedWorldSameOriginTest : function() { > > runTestInWorld(1, 'xhr', 'false'); > > }, > > -]; > > -var currentTest = 0; > > + isolatedWorldCrossOriginTest : function() { > > + runTestInWorld(2, 'xhrCrossOrigin', 'false'); > > + } > > +}; > > We are underutilizing this dictionary. > > > LayoutTests/http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr.html:59 > > switch (message.type) { > > - case 'test-done': > > - currentTest++; > > - if (currentTest == tests.length) > > + case 'test-main-world-xhr-done': > > + if (!mainWorldTestDone) { > > + mainWorldTestDone = true; > > + debug("PASS Main world, same-origin XHR threw an exception."); > > + tests.isolatedWorldSameOriginTest(); > > + } > > + break; > > + case 'test-isolated-world-xhr-done': > > + if (!isolatedWorldSameOriginTestDone) { > > + isolatedWorldSameOriginTestDone = true; > > + debug("PASS Isolated world, same-origin XHR did not throw an exception."); > > + tests.isolatedWorldCrossOriginTest(); > > + } > > + break; > > + case 'test-isolated-world-xhr-cross-origin-done': > > + if (!isolatedWorldCrossOriginTestDone) { > > + isolatedWorldCrossOriginTestDone = true; > > + debug("PASS Main world, cross-origin XHR did not throw an exception."); > > finishJSTest(); > > - else > > - tests[currentTest](); > > + } > > break; > > - case 'debug': > > - debug(message.message); > > + case 'fail': > > + testFailed('FAIL ' + message.message); > > break; > > How did you come to the decision to make these changes as opposed to making > use of the existing structure to add new sub-tests? I briefly mentioned the reason in the change log: "Simplified the structure with unique funcion names to avoid confusion and race conditions." Once I added a third test case I ran in to race conditions and it was hard to understand which case was running and in what state. It made it a lot easier to debug once I made the messages posted between the worlds specific. To me the test setup is much more clear now while still extendable. My impression is that the previous test with just one main world request and one (accidental?) cross-origin, isolated world request didn't make enough use of the generic rig to expose its bugs or complexity. > I suggest that we make > use of the existing test machinery because it seems generic (*), teach > runTestInWorld() (**) how to pass arbitrary arguments to a function and > teach xhr() to take another argument to determine whether it should make a > same-origin or cross-origin request. > > (*) It seems sufficiently generic because it runs sub-tests that are defined > as functions and each test function can window.postMessage() at least two > types of messages "debug" and "fail" to print an arbitrary message and > signal a test failure, respectively. I can add the debug message type back in. I removed it since I didn't use it. > > (**) Maybe something like (I renamed runTestInWorld to invokeInWorld to > better convey its purpose): > > function invokeInWorld(worldId, aFunction) > { > var script = aFunction.toString() + "; " + aFunction.name + "(" + > Array.prototype.slice.call(arguments, 2).join(", ") + ");"; > testRunner.evaluateScriptInIsolatedWorld(worldId, script); > }

Created attachment 275928 [details] Patch

Thanks for the fruitful discussion, Dan! I have incorporated what we decided in the test you had comments on. Please review.

Created attachment 275933 [details] Patch

Comment on attachment 275933 [details] Patch Forgot to add the FIXME comment Dan requested. Fixed now.

Comment on attachment 275933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275933&action=review I think you'll need to call setPendingActivity, m_timeoutTimer.stop(); and m_networkErrorTimer.startOneShot(0); if m_async is true, then add a test that calls abort immediately. This patch is a step in the right direction, and I would be ok with this landing and the async onerror call fixed in a follow up patch, but I would really prefer we not ship with this "throwing when sending async xhr" behavior. > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-blocked.html:14 > +shouldThrow("xhr.send()", "'Error: NetworkError: DOM Exception 19'"); This shouldNotThrow if it's asynchronous.

Comment on attachment 275933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275933&action=review > Source/WebCore/xml/XMLHttpRequest.cpp:570 > + // FIXME: We should only throw an exception for a synchronous request and should dispatch a DOM error event so as to make this CSP violation indistinguishable from a XHR network error. See <https://bugs.webkit.org/show_bug.cgi?id=156322>. Although most people have widescreen displays nowadays this line is long enough to wrap on my 27" Apple LED display unless I size my editor window to take up the majority of my screen. Please break this comment across multiple lines where each line is a C++-style comment. I suggest that we make the length of each comment line roughly the length of the comment on 569 for symmetry. I also suggest that we move this comment about throwing an exception to be above line 572 so that it is closer to the line that causes us to throw an exception. Something like: // FIXME: Convert this to check the isolated world's Content Security Policy once webkit.org/b/104520 is solved. if (!scriptExecutionContext()->contentSecurityPolicy()->allowConnectToSource(m_url, scriptExecutionContext()->shouldBypassMainWorldContentSecurityPolicy())) { // FIXME: We should only throw an exception for a synchronous request and should dispatch a DOM error event // so as to make this CSP violation indistinguishable from a XHR network error. See <https://bugs.webkit.org/show_bug.cgi?id=156322>. ec = NETWORK_ERR; return false; } > LayoutTests/http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr.html:83 > + if (!aNamedFunction.name) { > + throw new Error("invokeInWorld requires a named function."); > + } I would assert this invariant using console.assert() as opposed to throwing an Error object: console.assert(aNamedFunction.name); The reason I suggest this approach is that throwing an Error object can be caught using a try/catch block and it is unclear how a person would make use of such an exception to recover. Although this test does not contain code to catch such a throw exception and this function is not API it seems like good programming practice to design code that is easy to use correctly and hard to use incorrectly. By using console.assert() instead of throwing an Error we coerce a person to fix the call site that passes the bad input (the correct approach) and hard for them to do any other workaround (say, catch an exception and try to recover - a bad approach). > LayoutTests/http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr.html:84 > + var slice = Function.prototype.call.bind([].slice); I do not see the benefit of doing such gymnastics to cache a reference to the function Array.prototype.slice instead of using "Array.prototype.slice.call(arguments, 2)" directly (&). This code is significantly harder to reason about than (&) because it involves at least two levels of indirection and an allocation of an array (&&) using array literal syntax for the purpose of getting a reference to the method Array.prototype.slice(). If the purpose of this line is to reduce the length of the line below (85) then a better approach is to cache the passed arguments in a local variable, say argumentToPass, that is equal to Array.prototype.slice.call(arguments, 2). Then write line 85 in terms of argumentToPass: ... var argumentToPass = Array.prototype.slice.call(arguments, 2); var script = aNamedFunction.toString() + "; " + aNamedFunction.name + "(" + argumentToPass.map(JSON.stringify).join(", ") + ");"; ... (&&) I suspect that JavaScriptCore is smart enough to avoid allocating anything for this empty array. > LayoutTests/http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr.html:98 > + var url = (isSameOrigin ? "http://127.0.0.1:8000/" : "http://localhost:8000/") + "xmlhttprequest/resources/access-control-basic-allow.cgi", Nit: " (double quotes) => ' (single quotes) (For consistency with the quoting style used throughout this file). > LayoutTests/http/tests/security/isolatedWorld/bypass-main-world-csp-for-xhr.html:100 > + xhr = new XMLHttpRequest(), > + asyncCallDone = false, finallyClauseDone = false; We tend to follow the WebKit Code Style Guidelines for JavaScript code. It follows that we prefer one variable initialization per line instead of using the comma operator to sequence multiple initializations: var url = (isSameOrigin ? 'http://127.0.0.1:8000/' : 'http://localhost:8000/') + 'xmlhttprequest/resources/access-control-basic-allow.cgi'; var xhr = new XMLHttpRequest; var asyncCallDone = false; var finallyClauseDone = false;

Created attachment 275966 [details] Patch

Comment on attachment 275966 [details] Patch Alex, the new patch fixes the error event bug for asynchronous requests. Tests updated and expanded accordingly. Dan, I believe I have addressed the additional comments you had on the test code. Thanks both of you for all your comments! Please review.

Comment on attachment 275966 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275966&action=review > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-blocked-expected.txt:16 > +PASS An error event was received for the asynchronous call. > +PASS An error event was received for the aborted asynchronous call. This might be flaky because the order of these events is not defined. It might be nicer to create the second asynchronous call after receiving the error of the first.

Comment on attachment 275966 [details] Patch Clearing flags on attachment: 275966 Committed r199221: <http://trac.webkit.org/changeset/199221>

All reviewed patches have been landed. Closing bug.