Experiment: target=_blank on anchors should imply rel=noopener

https://bugs.webkit.org/show_bug.cgi?id=190481 Reviewed by Alex Christensen. Source/WebCore: As an experiment, try and make it so that target=_blank on anchors implies `rel=noopener` for improved security. WebContent can then request an opener relationship by using `rel=opener` instead. This change was discussed at: - whatwg/html#4078 We want to attempt this change is STP to see if it is Web-compatible. Preliminary testing seems to indicate that OAuth workflows still work. * html/HTMLAnchorElement.cpp: (WebCore::HTMLAnchorElement::parseAttribute): (WebCore::HTMLAnchorElement::handleClick): (WebCore::HTMLAnchorElement::effectiveTarget const): * html/HTMLAnchorElement.h: * page/RuntimeEnabledFeatures.h: (WebCore::RuntimeEnabledFeatures::setBlankAnchorTargetImpliesNoOpenerEnabled): (WebCore::RuntimeEnabledFeatures::blankAnchorTargetImpliesNoOpenerEnabled const): Source/WebKit: * Shared/WebPreferences.yaml: Tools: Add API test coverage to make sure we can now swap process when target=_blank is specified on an anchor but rel=noopener is not. * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm: LayoutTests: Update existing tests to reflect behavior change. * TestExpectations: * http/tests/navigation/no-referrer-reset.html: * http/tests/security/resources/referrer-policy-redirect-link.html: * http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson.html: * http/tests/security/xss-DENIED-script-inject-into-inactive-window2.html: * http/tests/security/xssAuditor/link-opens-new-window.html: git-svn-id: http://svn.webkit.org/repository/webkit/trunk@237144 268f45cc-cd09-0410-ab3c-d52691b4dbfc
paul99 · Oct 15, 2018 · 17638e7 · 17638e7
1 parent 0c0d330
commit 17638e7
Show file tree

Hide file tree

Showing 18 changed files with 199 additions and 7 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,19 @@
+2018-10-15  Chris Dumez  <[email protected]>
+
+        Experiment: target=_blank on anchors should imply rel=noopener
+        https://bugs.webkit.org/show_bug.cgi?id=190481
+
+        Reviewed by Alex Christensen.
+
+        Update existing tests to reflect behavior change.
+
+        * TestExpectations:
+        * http/tests/navigation/no-referrer-reset.html:
+        * http/tests/security/resources/referrer-policy-redirect-link.html:
+        * http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson.html:
+        * http/tests/security/xss-DENIED-script-inject-into-inactive-window2.html:
+        * http/tests/security/xssAuditor/link-opens-new-window.html:
+
 2018-10-15  Andy Estes  <[email protected]>
 
         [Apple Pay] New shipping methods are ignored when updating after the shippingaddresschange event

diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
@@ -2865,6 +2865,13 @@ webkit.org/b/185308 legacy-animation-engine/animations/combo-transform-translate
 # This newly imported test crashes in debug and flakily times out.
 webkit.org/b/189917 imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/document-write/contentType.window.html [ Skip ]
 
+# These tests started to time out or fail because of our experiment to have target=_blank on anchors imply rel=noopener (https://bugs.webkit.org/show_bug.cgi?id=190481).
+imported/w3c/web-platform-tests/html/browsers/windows/auxiliary-browsing-contexts/opener-closed.html [ Skip ]
+imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/002.html [ Failure ]
+imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_escaping-2.html [ Skip ]
+imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe_sandbox_popups_nonescaping-2.html [ Skip ]
+imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/choose-_blank-003.html [ Failure ]
+
 fast/gradients/conic-repeating.html [ Skip ]
 fast/gradients/conic.html [ Skip ]
 fast/gradients/conic-off-center.html [ Skip ]

diff --git a/LayoutTests/http/tests/navigation/anchor-blank-target-implies-rel-noopener-expected.txt b/LayoutTests/http/tests/navigation/anchor-blank-target-implies-rel-noopener-expected.txt
@@ -0,0 +1,4 @@
+CONSOLE MESSAGE: line 6: PASS: New window should not have an opener
+Tests that a new window opened via target=_blank does not have an opener
+
+
diff --git a/LayoutTests/http/tests/navigation/anchor-blank-target-implies-rel-noopener.html b/LayoutTests/http/tests/navigation/anchor-blank-target-implies-rel-noopener.html
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>Tests that a new window opened via target=_blank does not have an opener</p>
+<a id="testAnchor" href="resources/anchor-blank-target-implies-rel-noopener-win.html" target="_blank"></a>
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.waitUntilDone();
+    testRunner.setCanOpenWindows();
+}
+
+onload = function() {
+    setTimeout(() => {
+        testAnchor.click();
+    }, 0);
+}
+</script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/navigation/no-referrer-reset.html b/LayoutTests/http/tests/navigation/no-referrer-reset.html
@@ -4,7 +4,7 @@
 2. Click a rel="noreferrer" link: referrer is null, but window.opener remains set since the link was not opened with target="_blank".<br/>
 3. Click a link without rel="noreferrer": referrer is sent, but window.opener is still set.
 <br/>
-<a id="link" href="resources/no-referrer-reset-helper.php" target="_blank">Start reset test</a>
+<a id="link" href="resources/no-referrer-reset-helper.php" target="_blank" rel="opener">Start reset test</a>
 <script>
     window.name = "consoleWindow";
     window.noreferrerStepDone = false;

diff --git a/...utTests/http/tests/navigation/resources/anchor-blank-target-implies-rel-noopener-win.html b/...utTests/http/tests/navigation/resources/anchor-blank-target-implies-rel-noopener-win.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script>
+    if (!window.opener)
+        console.log("PASS: New window should not have an opener");
+    else
+        console.log("FAIL: New window should not have an opener");
+
+    if (window.testRunner)
+        testRunner.notifyDone();
+</script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/security/resources/referrer-policy-redirect-link.html b/LayoutTests/http/tests/security/resources/referrer-policy-redirect-link.html
@@ -21,7 +21,7 @@
 </script>
 </head>
 <body>
-<a id="link" target="_blank" href="https://127.0.0.1:8443/resources/redirect.php?url=http://127.0.0.1:8000/security/resources/referrer-policy-postmessage.php">If not running in DumpRenderTree, click this link</a>
+<a id="link" target="_blank" href="https://127.0.0.1:8443/resources/redirect.php?url=http://127.0.0.1:8000/security/resources/referrer-policy-postmessage.php" rel="opener">If not running in DumpRenderTree, click this link</a>
 <div id="log"></div>
 </body>
 </html>
diff --git a/LayoutTests/http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson.html b/LayoutTests/http/tests/security/xss-DENIED-script-inject-into-inactive-window2-pson.html
@@ -35,6 +35,7 @@
     // Case: Initial load
     var link = document.createElement("a");
     link.target = "_blank";
+    link.rel = "opener";
     link.href = "?actually-attack";
     link.click(); // Open a new window.
 }

diff --git a/LayoutTests/http/tests/security/xss-DENIED-script-inject-into-inactive-window2.html b/LayoutTests/http/tests/security/xss-DENIED-script-inject-into-inactive-window2.html
@@ -35,6 +35,7 @@
     // Case: Initial load
     var link = document.createElement("a");
     link.target = "_blank";
+    link.rel = "opener";
     link.href = "?actually-attack";
     link.click(); // Open a new window.
 }

diff --git a/LayoutTests/http/tests/security/xssAuditor/link-opens-new-window.html b/LayoutTests/http/tests/security/xssAuditor/link-opens-new-window.html
@@ -19,6 +19,6 @@
 </script>
 </head>
 <body>
-<a id="anchorLink" href="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?test=/security/xssAuditor/link-opens-new-window.html&notifyDone=1&q=<script>alert(String.fromCharCode(0x58,0x53,0x53))</script>" target="_blank">Click me</a>
+<a id="anchorLink" href="http://localhost:8000/security/xssAuditor/resources/echo-intertag.pl?test=/security/xssAuditor/link-opens-new-window.html&notifyDone=1&q=<script>alert(String.fromCharCode(0x58,0x53,0x53))</script>" target="_blank" rel="opener">Click me</a>
 </body>
 </html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,28 @@
+2018-10-15  Chris Dumez  <[email protected]>
+
+        Experiment: target=_blank on anchors should imply rel=noopener
+        https://bugs.webkit.org/show_bug.cgi?id=190481
+
+        Reviewed by Alex Christensen.
+
+        As an experiment, try and make it so that target=_blank on anchors implies `rel=noopener` for improved security.
+        WebContent can then request an opener relationship by using `rel=opener` instead.
+
+        This change was discussed at:
+        - https://github.com/whatwg/html/issues/4078
+
+        We want to attempt this change is STP to see if it is Web-compatible. Preliminary testing seems to indicate
+        that OAuth workflows still work.
+
+        * html/HTMLAnchorElement.cpp:
+        (WebCore::HTMLAnchorElement::parseAttribute):
+        (WebCore::HTMLAnchorElement::handleClick):
+        (WebCore::HTMLAnchorElement::effectiveTarget const):
+        * html/HTMLAnchorElement.h:
+        * page/RuntimeEnabledFeatures.h:
+        (WebCore::RuntimeEnabledFeatures::setBlankAnchorTargetImpliesNoOpenerEnabled):
+        (WebCore::RuntimeEnabledFeatures::blankAnchorTargetImpliesNoOpenerEnabled const):
+
 2018-10-15  Andy Estes  <[email protected]>
 
         [Apple Pay] New shipping methods are ignored when updating after the shippingaddresschange event

diff --git a/Source/WebCore/html/HTMLAnchorElement.cpp b/Source/WebCore/html/HTMLAnchorElement.cpp
@@ -250,12 +250,15 @@ void HTMLAnchorElement::parseAttribute(const QualifiedName& name, const AtomicSt
         // Update HTMLAnchorElement::relList() if more rel attributes values are supported.
         static NeverDestroyed<AtomicString> noReferrer("noreferrer", AtomicString::ConstructFromLiteral);
         static NeverDestroyed<AtomicString> noOpener("noopener", AtomicString::ConstructFromLiteral);
+        static NeverDestroyed<AtomicString> opener("opener", AtomicString::ConstructFromLiteral);
         const bool shouldFoldCase = true;
         SpaceSplitString relValue(value, shouldFoldCase);
         if (relValue.contains(noReferrer))
             m_linkRelations.add(Relation::NoReferrer);
         if (relValue.contains(noOpener))
             m_linkRelations.add(Relation::NoOpener);
+        if (relValue.contains(opener))
+            m_linkRelations.add(Relation::Opener);
         if (m_relList)
             m_relList->associatedAttributeValueChanged(value);
     }
@@ -427,12 +430,28 @@ void HTMLAnchorElement::handleClick(Event& event)
 #endif
 
     ShouldSendReferrer shouldSendReferrer = hasRel(Relation::NoReferrer) ? NeverSendReferrer : MaybeSendReferrer;
-    auto newFrameOpenerPolicy = hasRel(Relation::NoOpener) ? std::make_optional(NewFrameOpenerPolicy::Suppress) : std::nullopt;
-    frame->loader().urlSelected(completedURL, target(), &event, LockHistory::No, LockBackForwardList::No, shouldSendReferrer, document().shouldOpenExternalURLsPolicyToPropagate(), newFrameOpenerPolicy, downloadAttribute, systemPreviewInfo);
+
+    auto effectiveTarget = this->effectiveTarget();
+    std::optional<NewFrameOpenerPolicy> newFrameOpenerPolicy;
+    if (hasRel(Relation::Opener))
+        newFrameOpenerPolicy = NewFrameOpenerPolicy::Allow;
+    else if (hasRel(Relation::NoOpener) || (RuntimeEnabledFeatures::sharedFeatures().blankAnchorTargetImpliesNoOpenerEnabled() && equalIgnoringASCIICase(effectiveTarget, "_blank")))
+        newFrameOpenerPolicy = NewFrameOpenerPolicy::Suppress;
+
+    frame->loader().urlSelected(completedURL, effectiveTarget, &event, LockHistory::No, LockBackForwardList::No, shouldSendReferrer, document().shouldOpenExternalURLsPolicyToPropagate(), newFrameOpenerPolicy, downloadAttribute, systemPreviewInfo);
 
     sendPings(completedURL);
 }
 
+// Falls back to using <base> element's target if the anchor does not have one.
+String HTMLAnchorElement::effectiveTarget() const
+{
+    auto effectiveTarget = target();
+    if (effectiveTarget.isEmpty())
+        effectiveTarget = document().baseTarget();
+    return effectiveTarget;
+}
+
 HTMLAnchorElement::EventType HTMLAnchorElement::eventType(Event& event)
 {
     if (!is<MouseEvent>(event))

diff --git a/Source/WebCore/html/HTMLAnchorElement.h b/Source/WebCore/html/HTMLAnchorElement.h
@@ -37,6 +37,7 @@ class DOMTokenList;
 enum class Relation {
     NoReferrer = 1 << 0,
     NoOpener = 1 << 1,
+    Opener = 1 << 2,
 };
 
 class HTMLAnchorElement : public HTMLElement, public URLUtils<HTMLAnchorElement> {
@@ -90,6 +91,8 @@ class HTMLAnchorElement : public HTMLElement, public URLUtils<HTMLAnchorElement>
     int tabIndex() const final;
     bool draggable() const final;
 
+    String effectiveTarget() const;
+
     void sendPings(const URL& destinationURL);
 
     void handleClick(Event&);

diff --git a/Source/WebCore/page/RuntimeEnabledFeatures.h b/Source/WebCore/page/RuntimeEnabledFeatures.h
@@ -43,6 +43,9 @@ namespace WebCore {
 class RuntimeEnabledFeatures {
     WTF_MAKE_NONCOPYABLE(RuntimeEnabledFeatures);
 public:
+    void setBlankAnchorTargetImpliesNoOpenerEnabled(bool isEnabled) { m_blankAnchorTargetImpliesNoOpenerEnabled = isEnabled; }
+    bool blankAnchorTargetImpliesNoOpenerEnabled() const { return m_blankAnchorTargetImpliesNoOpenerEnabled; }
+
     void setDisplayContentsEnabled(bool isEnabled) { m_isDisplayContentsEnabled = isEnabled; }
     bool displayContentsEnabled() const { return m_isDisplayContentsEnabled; }
 
@@ -300,6 +303,7 @@ class RuntimeEnabledFeatures {
     // Never instantiate.
     RuntimeEnabledFeatures();
 
+    bool m_blankAnchorTargetImpliesNoOpenerEnabled { true };
     bool m_areModernMediaControlsEnabled { false };
     bool m_isLinkPreloadEnabled { true };
     bool m_isLinkPrefetchEnabled { false };

diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
@@ -1,3 +1,12 @@
+2018-10-15  Chris Dumez  <[email protected]>
+
+        Experiment: target=_blank on anchors should imply rel=noopener
+        https://bugs.webkit.org/show_bug.cgi?id=190481
+
+        Reviewed by Alex Christensen.
+
+        * Shared/WebPreferences.yaml:
+
 2018-10-15  Alex Christensen  <[email protected]>
 
         Remove unused parameters from FrameLoaderClient::createFrame

diff --git a/Source/WebKit/Shared/WebPreferences.yaml b/Source/WebKit/Shared/WebPreferences.yaml
@@ -1,3 +1,11 @@
+BlankAnchorTargetImpliesNoOpenerEnabled:
+   type: bool
+   defaultValue: true
+   webcoreBinding: RuntimeEnabledFeatures
+   humanReadableName: "Blank anchor target implies rel=noopener"
+   humanReadableDescription: "target=_blank on anchor elements implies rel=noopener"
+   category: experimental
+
 JavaScriptEnabled:
   type: bool
   defaultValue: true

diff --git a/Tools/ChangeLog b/Tools/ChangeLog
@@ -1,3 +1,15 @@
+2018-10-15  Chris Dumez  <[email protected]>
+
+        Experiment: target=_blank on anchors should imply rel=noopener
+        https://bugs.webkit.org/show_bug.cgi?id=190481
+
+        Reviewed by Alex Christensen.
+
+        Add API test coverage to make sure we can now swap process when target=_blank
+        is specified on an anchor but rel=noopener is not.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2018-10-15  Wenson Hsieh  <[email protected]>
 
         [iOS] Can't select text after dismissing the keyboard when changing focus

diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
@@ -280,7 +280,16 @@ function log(msg)
 </script>
 )PSONRESOURCE";
 
-static const char* targetBlankCrossSiteWithOpenerTestBytes = R"PSONRESOURCE(
+static const char* targetBlankCrossSiteWithExplicitOpenerTestBytes = R"PSONRESOURCE(
+<a id="testLink" target="_blank" href="pson://www.apple.com/main.html" rel="opener">Link</a>
+<script>
+window.onload = function() {
+    testLink.click();
+}
+</script>
+)PSONRESOURCE";
+
+static const char* targetBlankCrossSiteWithImplicitNoOpenerTestBytes = R"PSONRESOURCE(
 <a id="testLink" target="_blank" href="pson://www.apple.com/main.html">Link</a>
 <script>
 window.onload = function() {
@@ -693,7 +702,7 @@ function log(msg)
     auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
     [webViewConfiguration setProcessPool:processPool.get()];
     auto handler = adoptNS([[PSONScheme alloc] init]);
-    [handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:targetBlankCrossSiteWithOpenerTestBytes];
+    [handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:targetBlankCrossSiteWithExplicitOpenerTestBytes];
     [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
 
     auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
@@ -724,6 +733,46 @@ function log(msg)
     EXPECT_EQ(pid1, pid2);
 }
 
+TEST(ProcessSwap, CrossSiteBlankTargetImplicitNoOpener)
+{
+    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+    processPoolConfiguration.get().processSwapsOnNavigation = YES;
+    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    [webViewConfiguration setProcessPool:processPool.get()];
+    auto handler = adoptNS([[PSONScheme alloc] init]);
+    [handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:targetBlankCrossSiteWithImplicitNoOpenerTestBytes];
+    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+    auto navigationDelegate = adoptNS([[PSONNavigationDelegate alloc] init]);
+    [webView setNavigationDelegate:navigationDelegate.get()];
+    auto uiDelegate = adoptNS([[PSONUIDelegate alloc] initWithNavigationDelegate:navigationDelegate.get()]);
+    [webView setUIDelegate:uiDelegate.get()];
+
+    numberOfDecidePolicyCalls = 0;
+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    TestWebKitAPI::Util::run(&didCreateWebView);
+    didCreateWebView = false;
+
+    TestWebKitAPI::Util::run(&done);
+
+    EXPECT_EQ(3, numberOfDecidePolicyCalls);
+
+    auto pid1 = [webView _webProcessIdentifier];
+    EXPECT_TRUE(!!pid1);
+    auto pid2 = [createdWebView _webProcessIdentifier];
+    EXPECT_TRUE(!!pid2);
+
+    EXPECT_NE(pid1, pid2);
+}
+
 TEST(ProcessSwap, CrossSiteBlankTargetNoOpener)
 {
     auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);