HTML String load cannot be prevented by responding 'Cancel' asynchronously in decideP...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 23 Apr 2018 20:49:57 +0000 (20:49 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 23 Apr 2018 20:49:57 +0000 (20:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=184848
<rdar://problem/39145306>

Reviewed by Brady Eidson.

Source/WebCore:

When calling loadHTMLString on a WebView, we end up doing a load for 'about:blank'
with substitute data. In such case, we want to do a regular asynchronous policy
delegate check, there is no reason we need it to be synchronous. Update our check
to make sure we only do a synchronous policy check for initial 'about:blank' loads
that do not have substitute data.

* loader/PolicyChecker.cpp:
(WebCore::PolicyChecker::checkNavigationPolicy):

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm:
(-[DecidePolicyForNavigationActionController webView:decidePolicyForNavigationAction:decisionHandler:]):
(TEST):

LayoutTests:

Update layout tests that wrongly expected 'about:blank' to load synchronously even
when it is not the initial empty document of an iframe. I have checked that our
behavior is now consistent with Chrome.

* fast/events/beforeunload-alert-user-interaction2.html:
* http/tests/security/cross-origin-reified-window-location-setting-expected.txt:
* http/tests/security/cross-origin-reified-window-location-setting.html:
* webarchive/loading/javascript-url-iframe-crash-expected.txt:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@230919 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/events/beforeunload-alert-user-interaction2.html
LayoutTests/http/tests/security/cross-origin-reified-window-location-setting-expected.txt
LayoutTests/http/tests/security/cross-origin-reified-window-location-setting.html
LayoutTests/platform/wk2/webarchive/loading/javascript-url-iframe-crash-expected.txt [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/PolicyChecker.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm

index d4c1e6f..a7cc961 100644 (file)
@@ -1,3 +1,20 @@
+2018-04-23  Chris Dumez  <cdumez@apple.com>
+
+        HTML String load cannot be prevented by responding 'Cancel' asynchronously in decidePolicyForNavigationAction
+        https://bugs.webkit.org/show_bug.cgi?id=184848
+        <rdar://problem/39145306>
+
+        Reviewed by Brady Eidson.
+
+        Update layout tests that wrongly expected 'about:blank' to load synchronously even
+        when it is not the initial empty document of an iframe. I have checked that our
+        behavior is now consistent with Chrome.
+
+        * fast/events/beforeunload-alert-user-interaction2.html:
+        * http/tests/security/cross-origin-reified-window-location-setting-expected.txt:
+        * http/tests/security/cross-origin-reified-window-location-setting.html:
+        * webarchive/loading/javascript-url-iframe-crash-expected.txt:
+
 2018-04-23  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         [Extra zoom mode] 100vw is roughly half of the viewport width in extra zoom mode
index 9969a12..184d8b5 100644 (file)
@@ -16,8 +16,8 @@ onload = function() {
     const testInput = document.getElementById("testInput");
     UIHelper.activateAt(testInput.offsetLeft + 5, testInput.offsetTop + 5).then(function() {
         setTimeout(function() {
+            testFrame.onload = finishJSTest;
             testFrame.src = "about:blank";
-            setTimeout(finishJSTest, 0);
         }, 0);
     });
 };
index b6ba68c..18792da 100644 (file)
@@ -3,7 +3,9 @@ Tests that window.location can be set cross-origin even if the window object is
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
+PASS crossOriginWindow.location.href threw exception SecurityError: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a cross-origin frame. Protocols, domains, and ports must match..
 PASS crossOriginWindow.location = 'about:blank' did not throw exception.
+PASS crossOriginWindow.location.href threw exception SecurityError: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a cross-origin frame. Protocols, domains, and ports must match..
 PASS crossOriginWindow.location.href is "about:blank"
 PASS successfullyParsed is true
 
index db280f9..d62003b 100644 (file)
@@ -1,7 +1,7 @@
 <!DOCTYPE html>
 <html>
 <head>
-<script src="../../resources/js-test-pre.js"></script>
+<script src="/js-test-resources/js-test.js"></script>
 </head>
 <body onload="runTest()">
 <iframe id="crossOriginFrame" src="http://localhost:8000/security/resources/reify-window.html"></iframe>
@@ -11,15 +11,19 @@ jsTestIsAsync = true;
 
 function runTest()
 {
-    crossOriginWindow = crossOriginFrame.window;
+    crossOriginWindow = document.getElementById("crossOriginFrame").contentWindow;
+    shouldThrow("crossOriginWindow.location.href");
     shouldNotThrow("crossOriginWindow.location = 'about:blank'");
-
-    setTimeout(function() {
-        shouldBeEqualToString("crossOriginWindow.location.href", "about:blank");
-        finishJSTest();
-    }, 0);
+    shouldThrow("crossOriginWindow.location.href");
+    handle = setInterval(function() {
+        try {
+            crossOriginWindow.location.href;
+            shouldBeEqualToString("crossOriginWindow.location.href", "about:blank");
+            clearInterval(handle);
+            finishJSTest();
+        } catch(e) { }
+    }, 5);
 }
 </script>
 </body>
-<script src="../../resources/js-test-post.js"></script>
 </html>
diff --git a/LayoutTests/platform/wk2/webarchive/loading/javascript-url-iframe-crash-expected.txt b/LayoutTests/platform/wk2/webarchive/loading/javascript-url-iframe-crash-expected.txt
new file mode 100644 (file)
index 0000000..aafec5d
--- /dev/null
@@ -0,0 +1,15 @@
+main frame - didStartProvisionalLoadForFrame
+main frame - didCommitLoadForFrame
+main frame - willPerformClientRedirectToURL: resources/javascript-url-iframe-crash.webarchive 
+main frame - didFinishDocumentLoadForFrame
+main frame - didFinishLoadForFrame
+main frame - didStartProvisionalLoadForFrame
+main frame - didCancelClientRedirectForFrame
+main frame - didCommitLoadForFrame
+frame "<!--framePath //<!--frame0-->-->" - didFinishDocumentLoadForFrame
+frame "<!--framePath //<!--frame0-->-->" - didHandleOnloadEventsForFrame
+main frame - didFinishDocumentLoadForFrame
+main frame - didHandleOnloadEventsForFrame
+main frame - didFinishLoadForFrame
+Loading this webarchive with a "non-empty javascript URL iframe" should not crash.
+
index d196e99..c29e10c 100644 (file)
@@ -1,3 +1,20 @@
+2018-04-23  Chris Dumez  <cdumez@apple.com>
+
+        HTML String load cannot be prevented by responding 'Cancel' asynchronously in decidePolicyForNavigationAction
+        https://bugs.webkit.org/show_bug.cgi?id=184848
+        <rdar://problem/39145306>
+
+        Reviewed by Brady Eidson.
+
+        When calling loadHTMLString on a WebView, we end up doing a load for 'about:blank'
+        with substitute data. In such case, we want to do a regular asynchronous policy
+        delegate check, there is no reason we need it to be synchronous. Update our check
+        to make sure we only do a synchronous policy check for initial 'about:blank' loads
+        that do not have substitute data.
+
+        * loader/PolicyChecker.cpp:
+        (WebCore::PolicyChecker::checkNavigationPolicy):
+
 2018-04-23  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         [Extra zoom mode] 100vw is roughly half of the viewport width in extra zoom mode
index fdeda3f..fc24194 100644 (file)
@@ -123,7 +123,8 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didRec
 
     loader->setLastCheckedRequest(ResourceRequest(request));
 
-    if (request.url().isBlankURL())
+    // Initial 'about:blank' load needs to happen synchronously so the policy check needs to be synchronous in this case.
+    if (!m_frame.loader().stateMachine().committedFirstRealDocumentLoad() && request.url().isBlankURL() && !substituteData.isValid())
         policyDecisionMode = PolicyDecisionMode::Synchronous;
 
 #if USE(QUICK_LOOK)
index 586ce66..bf1dcb1 100644 (file)
@@ -1,3 +1,17 @@
+2018-04-23  Chris Dumez  <cdumez@apple.com>
+
+        HTML String load cannot be prevented by responding 'Cancel' asynchronously in decidePolicyForNavigationAction
+        https://bugs.webkit.org/show_bug.cgi?id=184848
+        <rdar://problem/39145306>
+
+        Reviewed by Brady Eidson.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm:
+        (-[DecidePolicyForNavigationActionController webView:decidePolicyForNavigationAction:decisionHandler:]):
+        (TEST):
+
 2018-04-23  Michael Catanzaro  <mcatanzaro@igalia.com>
 
         [GTK][WPE] TestSSL fails due to additional TLS errors returned
index 31116aa..8a11127 100644 (file)
@@ -35,6 +35,7 @@
 
 #if WK_API_ENABLED
 
+static bool shouldCancelNavigation;
 static bool createdWebView;
 static bool decidedPolicy;
 static bool finishedNavigation;
@@ -51,6 +52,16 @@ static NSString *secondURL = @"data:text/html,Second";
 
 - (void)webView:(WKWebView *)webView decidePolicyForNavigationAction:(WKNavigationAction *)navigationAction decisionHandler:(void (^)(WKNavigationActionPolicy))decisionHandler
 {
+    if (shouldCancelNavigation) {
+        int64_t deferredWaitTime = 100 * NSEC_PER_MSEC;
+        dispatch_time_t when = dispatch_time(DISPATCH_TIME_NOW, deferredWaitTime);
+        dispatch_after(when, dispatch_get_main_queue(), ^{
+            decisionHandler(WKNavigationActionPolicyCancel);
+            decidedPolicy = true;
+        });
+        return;
+    }
+
     decisionHandler(webView == newWebView.get() ? WKNavigationActionPolicyCancel : WKNavigationActionPolicyAllow);
 
     action = navigationAction;
@@ -285,7 +296,7 @@ TEST(WebKit, DecidePolicyForNavigationActionForTargetedHyperlink)
     action = nullptr;
 }
 
-TEST(WebKit, DecidePolicyForNavigationActionForLoadHTMLString)
+TEST(WebKit, DecidePolicyForNavigationActionForLoadHTMLStringAllow)
 {
     auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);
 
@@ -296,9 +307,31 @@ TEST(WebKit, DecidePolicyForNavigationActionForLoadHTMLString)
     [webView setNavigationDelegate:controller.get()];
     [webView setUIDelegate:controller.get()];
 
+    finishedNavigation = false;
     decidedPolicy = false;
     [webView loadHTMLString:@"TEST" baseURL:[NSURL URLWithString:@"about:blank"]];
-    TestWebKitAPI::Util::run(&decidedPolicy);
+    TestWebKitAPI::Util::run(&finishedNavigation);
+    EXPECT_TRUE(decidedPolicy);
+}
+
+TEST(WebKit, DecidePolicyForNavigationActionForLoadHTMLStringDeny)
+{
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);
+
+    auto window = adoptNS([[NSWindow alloc] initWithContentRect:[webView frame] styleMask:NSWindowStyleMaskBorderless backing:NSBackingStoreBuffered defer:YES]);
+    [[window contentView] addSubview:webView.get()];
+
+    auto controller = adoptNS([[DecidePolicyForNavigationActionController alloc] init]);
+    [webView setNavigationDelegate:controller.get()];
+    [webView setUIDelegate:controller.get()];
+
+    shouldCancelNavigation = true;
+    finishedNavigation = false;
+    decidedPolicy = false;
+    [webView loadHTMLString:@"TEST" baseURL:[NSURL URLWithString:@"about:blank"]];
+    TestWebKitAPI::Util::sleep(0.5);
+    EXPECT_FALSE(finishedNavigation);
+    shouldCancelNavigation = false;
 }
 
 TEST(WebKit, DecidePolicyForNavigationActionForTargetedWindowOpen)