Some tests to verify forbidden frame navigation time out

https://bugs.webkit.org/show_bug.cgi?id=173657

Patch by Frederic Wang <fwang@igalia.com> on 2017-06-27
Reviewed by Chris Dumez.

LayoutTests/imported/w3c:

* web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-2-expected.txt: Update the text expectation to PASS.
* web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation_by_user_activation_without_user_gesture-expected.txt: Ditto.
* web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_ancestor-1-expected.txt: Ditto.
* web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3-expected.txt: Add the security error until bug 173162 is fixed.
* web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3-expected.txt: Ditto.

Source/WebCore:

Currently some tests try and perform a forbidden frame navigation and verify the
corresponding console error. However, WebKit does not raise any exception for such error so
the tests have to wait until the timeout limit to complete, which makes execution slow.
This patch modifies the setters of window.location for which such error may happen in order
to raise an exception so the tests behave as expected.

No new tests, already covered by existing tests.

* page/Location.cpp: Adjust Location::setLocation to return a security exception and pass it
to the callers.
(WebCore::Location::setHref): Adjust function to possibly return an exception.
(WebCore::Location::setProtocol): Ditto.
(WebCore::Location::setHost): Ditto.
(WebCore::Location::setHostname): Ditto.
(WebCore::Location::setPort): Ditto.
(WebCore::Location::setPathname): Ditto.
(WebCore::Location::setSearch): Ditto.
(WebCore::Location::setHash): Ditto.
(WebCore::Location::assign): Ditto.
(WebCore::Location::setLocation): FrameLoader::findFrameForNavigation is really only used
to verify whether navigating m_frame is permitted so it is more simple and clearer to do it
directly. When navigation is not permitted, this function now raises a security exception.
* page/Location.h: Modify some setters to return an ExceptionOr<void>.
* page/Location.idl: Allow some setters to raise an exception.

LayoutTests:

* fast/frames/sandboxed-iframe-navigation-top-denied-expected.txt: Add the security error.
* http/tests/security/frameNavigation/inactive-function-in-popup-navigate-child.html: Adjust
the test to catch and dump the exception and complete immediately.
* http/tests/security/frameNavigation/inactive-function-in-popup-navigate-child-expected.txt:
Add the dumped security error exception.

Canonical link: https://commits.webkit.org/190712@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@218835 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog

@@ -1,3 +1,16 @@
+2017-06-27  Frederic Wang  <fwang@igalia.com>
+
+        Some tests to verify forbidden frame navigation time out
+        https://bugs.webkit.org/show_bug.cgi?id=173657
+
+        Reviewed by Chris Dumez.
+
+        * fast/frames/sandboxed-iframe-navigation-top-denied-expected.txt: Add the security error.
+        * http/tests/security/frameNavigation/inactive-function-in-popup-navigate-child.html: Adjust
+        the test to catch and dump the exception and complete immediately.
+        * http/tests/security/frameNavigation/inactive-function-in-popup-navigate-child-expected.txt:
+        Add the dumped security error exception.
+
 2017-06-27  Youenn Fablet  <youenn@apple.com>
 
         LayoutTest webrtc/datachannel/multiple-connections.html is a flaky timeout

LayoutTests/fast/frames/sandboxed-iframe-navigation-top-denied-expected.txt

@@ -1,5 +1,6 @@
 CONSOLE MESSAGE: Unsafe JavaScript attempt to initiate navigation for frame with URL 'navigate-top-to-fail.html'. The frame attempting navigation of the top-level window is sandboxed, but the 'allow-top-navigation' flag is not set.
 
+CONSOLE MESSAGE: SecurityError (DOM Exception 18): The operation is insecure.
 This test verifies that a sandboxed IFrame cannot navigate the top-level frame without allow-top-navigation. This test passes if the navigation does not occur.
 
 

LayoutTests/http/tests/security/frameNavigation/inactive-function-in-popup-navigate-child-expected.txt

@@ -2,3 +2,4 @@ This test passes if it doesn't alert "FAIL".
 ready
 iframe-with-inner-frame-on-foreign-domain-LOADED
 Attempting navigation...
+SecurityError (DOM Exception 18): The operation is insecure.

LayoutTests/http/tests/security/frameNavigation/inactive-function-in-popup-navigate-child.html

@@ -30,14 +30,14 @@
 
     if (e.data = "iframe-with-inner-frame-on-foreign-domain-LOADED") {
         log("Attempting navigation...");
-        window.savedFunction();
-        setTimeout(function() {
-            // Unfortunately, there's no way to receive positive confirmation
-            // that the navigation failed, so we just complete the test
-            // asynchronously.
+        try {
+            window.savedFunction();
             if (window.testRunner)
                 testRunner.notifyDone();
-        }, 0);
+        } catch(e) {
+            log(e);
+            testRunner.notifyDone();
+        }
         return;
     }
 

LayoutTests/imported/w3c/ChangeLog

@@ -1,3 +1,16 @@
+2017-06-27  Frederic Wang  <fwang@igalia.com>
+
+        Some tests to verify forbidden frame navigation time out
+        https://bugs.webkit.org/show_bug.cgi?id=173657
+
+        Reviewed by Chris Dumez.
+
+        * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-2-expected.txt: Update the text expectation to PASS.
+        * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation_by_user_activation_without_user_gesture-expected.txt: Ditto.
+        * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_ancestor-1-expected.txt: Ditto.
+        * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3-expected.txt: Add the security error until bug 173162 is fixed.
+        * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3-expected.txt: Ditto.
+
 2017-06-23  Youenn Fablet  <youenn@apple.com>
 
         Set getUserMedia permission to true by default on WTR

LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation-2-expected.txt

@@ -2,7 +2,5 @@ CONSOLE MESSAGE: line 7: Unsafe JavaScript attempt to initiate navigation for fr
 
 
 
-Harness Error (TIMEOUT), message = null
-
-TIMEOUT Frames without `allow-top-navigation` should not be able to navigate the top frame. Test timed out
+PASS Frames without `allow-top-navigation` should not be able to navigate the top frame. 
 

LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_allow_top_navigation_by_user_activation_without_user_gesture-expected.txt

@@ -5,5 +5,5 @@ This tests that an iframe in sandbox with 'allow-top-navigation-by-user-activati
 
 
 
-FAIL The sandboxed iframe should post a message saying the test was in the state of 'PASS'. assert_equals: The message should say 'PASS' instead of 'FAIL' expected "PASS" but got "FAIL"
+PASS The sandboxed iframe should post a message saying the test was in the state of 'PASS'. 
 

LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_navigate_ancestor-1-expected.txt

@@ -2,7 +2,5 @@ CONSOLE MESSAGE: line 6: Unsafe JavaScript attempt to initiate navigation for fr
 
 
 
-Harness Error (TIMEOUT), message = null
-
-NOTRUN Check that sandboxed iframe can not navigate their ancestors 
+PASS Check that sandboxed iframe can not navigate their ancestors 
 

LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-3-expected.txt

@@ -1,5 +1,6 @@
 CONSOLE MESSAGE: line 15: Unsafe JavaScript attempt to initiate navigation for frame with URL 'about:blank' from frame with URL 'http://localhost:8800/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_helper-3.html'. The frame attempting navigation is sandboxed, and is therefore disallowed from navigating its ancestors.
 
+CONSOLE MESSAGE: line 15: SecurityError (DOM Exception 18): The operation is insecure.
 
 
 Harness Error (TIMEOUT), message = null

LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-3-expected.txt

@@ -1,5 +1,6 @@
 CONSOLE MESSAGE: line 15: Unsafe JavaScript attempt to initiate navigation for frame with URL 'about:blank' from frame with URL 'http://localhost:8800/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_helper-3.html'. The frame attempting navigation is sandboxed, and is therefore disallowed from navigating its ancestors.
 
+CONSOLE MESSAGE: line 15: SecurityError (DOM Exception 18): The operation is insecure.
 
 
 Harness Error (TIMEOUT), message = null

Source/WebCore/ChangeLog

@@ -1,3 +1,35 @@
+2017-06-27  Frederic Wang  <fwang@igalia.com>
+
+        Some tests to verify forbidden frame navigation time out
+        https://bugs.webkit.org/show_bug.cgi?id=173657
+
+        Reviewed by Chris Dumez.
+
+        Currently some tests try and perform a forbidden frame navigation and verify the
+        corresponding console error. However, WebKit does not raise any exception for such error so
+        the tests have to wait until the timeout limit to complete, which makes execution slow.
+        This patch modifies the setters of window.location for which such error may happen in order
+        to raise an exception so the tests behave as expected.
+
+        No new tests, already covered by existing tests.
+
+        * page/Location.cpp: Adjust Location::setLocation to return a security exception and pass it
+        to the callers.
+        (WebCore::Location::setHref): Adjust function to possibly return an exception.
+        (WebCore::Location::setProtocol): Ditto.
+        (WebCore::Location::setHost): Ditto.
+        (WebCore::Location::setHostname): Ditto.
+        (WebCore::Location::setPort): Ditto.
+        (WebCore::Location::setPathname): Ditto.
+        (WebCore::Location::setSearch): Ditto.
+        (WebCore::Location::setHash): Ditto.
+        (WebCore::Location::assign): Ditto.
+        (WebCore::Location::setLocation): FrameLoader::findFrameForNavigation is really only used
+        to verify whether navigating m_frame is permitted so it is more simple and clearer to do it
+        directly. When navigation is not permitted, this function now raises a security exception.
+        * page/Location.h: Modify some setters to return an ExceptionOr<void>.
+        * page/Location.idl: Allow some setters to raise an exception.
+
 2017-06-26  Fujii Hironori  <Hironori.Fujii@sony.com>
 
         [GTK] Layout Test webrtc/video.html issues "stack smashing detected"

Source/WebCore/page/Location.cpp

@@ -150,11 +150,11 @@ String Location::hash() const
     return fragmentIdentifier.isEmpty() ? emptyString() : "#" + fragmentIdentifier;
 }
 
-void Location::setHref(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& url)
+ExceptionOr<void> Location::setHref(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& url)
 {
     if (!m_frame)
-        return;
-    setLocation(activeWindow, firstWindow, url);
+        return { };
+    return setLocation(activeWindow, firstWindow, url);
 }
 
 ExceptionOr<void> Location::setProtocol(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& protocol)
@@ -164,63 +164,62 @@ ExceptionOr<void> Location::setProtocol(DOMWindow& activeWindow, DOMWindow& firs
     URL url = m_frame->document()->url();
     if (!url.setProtocol(protocol))
         return Exception { SYNTAX_ERR };
-    setLocation(activeWindow, firstWindow, url.string());
-    return { };
+    return setLocation(activeWindow, firstWindow, url.string());
 }
 
-void Location::setHost(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& host)
+ExceptionOr<void> Location::setHost(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& host)
 {
     if (!m_frame)
-        return;
+        return { };
     URL url = m_frame->document()->url();
     url.setHostAndPort(host);
-    setLocation(activeWindow, firstWindow, url.string());
+    return setLocation(activeWindow, firstWindow, url.string());
 }
 
-void Location::setHostname(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& hostname)
+ExceptionOr<void> Location::setHostname(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& hostname)
 {
     if (!m_frame)
-        return;
+        return { };
     URL url = m_frame->document()->url();
     url.setHost(hostname);
-    setLocation(activeWindow, firstWindow, url.string());
+    return setLocation(activeWindow, firstWindow, url.string());
 }
 
-void Location::setPort(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& portString)
+ExceptionOr<void> Location::setPort(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& portString)
 {
     if (!m_frame)
-        return;
+        return { };
     URL url = m_frame->document()->url();
     int port = portString.toInt();
     if (port < 0 || port > 0xFFFF || portString.isEmpty())
         url.removePort();
     else
         url.setPort(port);
-    setLocation(activeWindow, firstWindow, url.string());
+    return setLocation(activeWindow, firstWindow, url.string());
 }
 
-void Location::setPathname(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& pathname)
+ExceptionOr<void> Location::setPathname(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& pathname)
 {
     if (!m_frame)
-        return;
+        return { };
     URL url = m_frame->document()->url();
     url.setPath(pathname);
-    setLocation(activeWindow, firstWindow, url.string());
+    return setLocation(activeWindow, firstWindow, url.string());
 }
 
-void Location::setSearch(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& search)
+ExceptionOr<void> Location::setSearch(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& search)
 {
     if (!m_frame)
-        return;
+        return { };
     URL url = m_frame->document()->url();
     url.setQuery(search);
-    setLocation(activeWindow, firstWindow, url.string());
+    return setLocation(activeWindow, firstWindow, url.string());
 }
 
-void Location::setHash(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& hash)
+ExceptionOr<void> Location::setHash(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& hash)
 {
     if (!m_frame)
-        return;
+        return { };
     ASSERT(m_frame->document());
     auto url = m_frame->document()->url();
     auto oldFragmentIdentifier = url.fragmentIdentifier();
@@ -232,15 +231,15 @@ void Location::setHash(DOMWindow& activeWindow, DOMWindow& firstWindow, const St
     // comparing fragments post-canonicalization, and so this handles the 
     // cases where fragment identifiers are ignored or invalid. 
     if (equalIgnoringNullity(oldFragmentIdentifier, url.fragmentIdentifier()))
-        return;
-    setLocation(activeWindow, firstWindow, url.string());
+        return { };
+    return setLocation(activeWindow, firstWindow, url.string());
 }
 
-void Location::assign(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& url)
+ExceptionOr<void> Location::assign(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& url)
 {
     if (!m_frame)
-        return;
-    setLocation(activeWindow, firstWindow, url);
+        return { };
+    return setLocation(activeWindow, firstWindow, url);
 }
 
 void Location::replace(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& url)
@@ -280,15 +279,15 @@ void Location::reload(DOMWindow& activeWindow)
     m_frame->navigationScheduler().scheduleRefresh(activeDocument);
 }
 
-void Location::setLocation(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& url)
+ExceptionOr<void> Location::setLocation(DOMWindow& activeWindow, DOMWindow& firstWindow, const String& url)
 {
     ASSERT(m_frame);
-    auto* targetFrame = m_frame->loader().findFrameForNavigation({ }, activeWindow.document());
-    if (!targetFrame)
-        return;
-    ASSERT(targetFrame->document());
-    ASSERT(targetFrame->document()->domWindow());
-    targetFrame->document()->domWindow()->setLocation(activeWindow, firstWindow, url);
+    if (!activeWindow.document()->canNavigate(m_frame))
+        return Exception { SECURITY_ERR };
+    ASSERT(m_frame->document());
+    ASSERT(m_frame->document()->domWindow());
+    m_frame->document()->domWindow()->setLocation(activeWindow, firstWindow, url);
+    return { };
 }
 
 } // namespace WebCore

Source/WebCore/page/Location.h

@@ -43,26 +43,26 @@ class Location : public ScriptWrappable, public RefCounted<Location>, public DOM
 public:
     static Ref<Location> create(Frame* frame) { return adoptRef(*new Location(frame)); }
 
-    void setHref(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
+    ExceptionOr<void> setHref(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
     String href() const;
 
-    void assign(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
+    ExceptionOr<void> assign(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
     void replace(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
     void reload(DOMWindow& activeWindow);
 
     ExceptionOr<void> setProtocol(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
     String protocol() const;
-    void setHost(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
+    ExceptionOr<void> setHost(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
     String host() const;
-    void setHostname(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
+    ExceptionOr<void> setHostname(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
     String hostname() const;
-    void setPort(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
+    ExceptionOr<void> setPort(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
     String port() const;
-    void setPathname(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
+    ExceptionOr<void> setPathname(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
     String pathname() const;
-    void setSearch(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
+    ExceptionOr<void> setSearch(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
     String search() const;
-    void setHash(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
+    ExceptionOr<void> setHash(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
     String hash() const;
     String origin() const;
 
@@ -73,7 +73,7 @@ class Location : public ScriptWrappable, public RefCounted<Location>, public DOM
 private:
     explicit Location(Frame*);
 
-    void setLocation(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
+    ExceptionOr<void> setLocation(DOMWindow& activeWindow, DOMWindow& firstWindow, const String&);
 
     const URL& url() const;
 };

Source/WebCore/page/Location.idl

@@ -42,20 +42,20 @@
     IsImmutablePrototypeExoticObject,
     Unforgeable,
 ] interface Location {
-    [SetterCallWith=ActiveWindow&FirstWindow, DoNotCheckSecurityOnSetter] stringifier attribute USVString href;
+    [SetterCallWith=ActiveWindow&FirstWindow, SetterMayThrowException, DoNotCheckSecurityOnSetter] stringifier attribute USVString href;
 
-    [CallWith=ActiveWindow&FirstWindow, ForwardDeclareInHeader] void assign(USVString url);
+    [CallWith=ActiveWindow&FirstWindow, MayThrowException, ForwardDeclareInHeader] void assign(USVString url);
     [DoNotCheckSecurity, CallWith=ActiveWindow&FirstWindow, ForwardDeclareInHeader] void replace(USVString url);
     [CallWith=ActiveWindow, ForwardDeclareInHeader] void reload();
 
     // URI decomposition attributes
     [SetterCallWith=ActiveWindow&FirstWindow, SetterMayThrowException] attribute USVString protocol;
-    [SetterCallWith=ActiveWindow&FirstWindow] attribute USVString host;
-    [SetterCallWith=ActiveWindow&FirstWindow] attribute USVString hostname;
-    [SetterCallWith=ActiveWindow&FirstWindow] attribute USVString port;
-    [SetterCallWith=ActiveWindow&FirstWindow] attribute USVString pathname;
-    [SetterCallWith=ActiveWindow&FirstWindow] attribute USVString search;
-    [SetterCallWith=ActiveWindow&FirstWindow] attribute USVString hash;
+    [SetterCallWith=ActiveWindow&FirstWindow, SetterMayThrowException] attribute USVString host;
+    [SetterCallWith=ActiveWindow&FirstWindow, SetterMayThrowException] attribute USVString hostname;
+    [SetterCallWith=ActiveWindow&FirstWindow, SetterMayThrowException] attribute USVString port;
+    [SetterCallWith=ActiveWindow&FirstWindow, SetterMayThrowException] attribute USVString pathname;
+    [SetterCallWith=ActiveWindow&FirstWindow, SetterMayThrowException] attribute USVString search;
+    [SetterCallWith=ActiveWindow&FirstWindow, SetterMayThrowException] attribute USVString hash;
 
     readonly attribute USVString origin;