Attachment 343634 Details for Bug 186090 – Patch and layout test

[patch] Patch and layout test

bug-186090-20180626133053.patch (text/plain), 12.58 KB, created by Daniel Bates on 2018-06-26 13:30:54 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Daniel Bates

Created: 2018-06-26 13:30:54 PDT

Size: 12.58 KB

patch

obsolete

>Subversion Revision: 233157
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 6a39cc4a468cf955f8af65edb4b13db27ee9d1c6..66ddbce5abc36c895d0d0364ce7c56be8e51f177 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,21 @@
>+2018-06-26  Daniel Bates  <dabates@apple.com>
>+
>+        REGRESSION (r231479): Unable to buy Odeon cinema tickets in STP (bogus 'X-Frame-Options' to 'SAMEORIGIN')
>+        https://bugs.webkit.org/show_bug.cgi?id=186090
>+        <rdar://problem/40692595>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Fix up Content Security Policy logic for checking the frame ancestors now that we
>+        exclude the frame that initiated the load request.
>+
>+        Test: http/tests/security/XFrameOptions/cross-origin-iframe-post-form-to-parent-same-origin-x-frame-options-page-allow.html
>+
>+        * page/csp/ContentSecurityPolicy.cpp:
>+        (WebCore::ContentSecurityPolicy::allowFrameAncestors const): 
>+        * page/csp/ContentSecurityPolicyDirectiveList.cpp:
>+        (WebCore::checkFrameAncestors):
>+
> 2018-06-25  Youenn Fablet  <youenn@apple.com>
> 
>         imported/w3c/web-platform-tests/fetch/nosniff/importscripts.html is crashing on debug builds
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index b5e2a49b559729d9fa5cde8c94c0a0b49111ec20..7ef4866afd174eafd5d7088e61abdde815677d69 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,29 @@
>+2018-06-26  Daniel Bates  <dabates@apple.com>
>+
>+        REGRESSION (r231479): Unable to buy Odeon cinema tickets in STP (bogus 'X-Frame-Options' to 'SAMEORIGIN')
>+        https://bugs.webkit.org/show_bug.cgi?id=186090
>+        <rdar://problem/40692595>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Fixes an issue where a page P delivered with "X-Frame-Options: SAMEORIGIN" loaded in a sub-
>+        frame would be blocked if we were redirected to it in response to the cross-origin POST
>+        request regardless of whether P is same-origin with its parent document.
>+
>+        * NetworkProcess/NetworkResourceLoader.cpp:
>+        (WebKit::NetworkResourceLoader::shouldInterruptLoadForXFrameOptions): Compare the origin
>+        of the top frame's document as opposed to the source origin. The latter represents the
>+        origin of the document that initiated the navigation, which can be cross-origin, and
>+        should not be considered when applying "X-Frame-Options: SAMEORIGIN". This check exists
>+        as a performance optimization to avoid traversing over all frame ancestors only to find
>+        out that the innermost frame (the one that made this request) is cross-origin with the
>+        top-most frame.
>+        * NetworkProcess/NetworkResourceLoader.h:
>+        * WebProcess/Network/WebLoaderStrategy.cpp:
>+        (WebKit::WebLoaderStrategy::scheduleLoadFromNetworkProcess): Exclude the origin of the
>+        frame that is making the load request from the list of ancestor origins. This makes the
>+        X-Frame-Options algorithm in WebKit2 match the logic we do in FrameLoader::shouldInterruptLoadForXFrameOptions().
>+
> 2018-06-23  Brian Burg  <bburg@apple.com>
> 
>         [Mac] Web Automation: include correct key code with synthesized NSEvents used for keystrokes
>diff --git a/Source/WebCore/page/csp/ContentSecurityPolicy.cpp b/Source/WebCore/page/csp/ContentSecurityPolicy.cpp
>index 6d226ea6a3c9b76794f36eb5c0653c362dd0fce8..b136371aa0d043ee00b201d9f0ca476f81645a92 100644
>--- a/Source/WebCore/page/csp/ContentSecurityPolicy.cpp
>+++ b/Source/WebCore/page/csp/ContentSecurityPolicy.cpp
>@@ -493,8 +493,7 @@ bool ContentSecurityPolicy::allowFrameAncestors(const Vector<RefPtr<SecurityOrig
> {
>     if (overrideContentSecurityPolicy)
>         return true;
>-    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!ancestorOrigins.isEmpty());
>-    bool isTopLevelFrame = ancestorOrigins.size() == 1;
>+    bool isTopLevelFrame = ancestorOrigins.isEmpty();
>     if (isTopLevelFrame)
>         return true;
>     String sourceURL;
>diff --git a/Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp b/Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp
>index 6423cec1028492a29a6f380bfe91a922dd400a64..5c1bf7d47e6d90ea9cd1607528c69465538634ac 100644
>--- a/Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp
>+++ b/Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp
>@@ -97,10 +97,9 @@ static inline bool checkFrameAncestors(ContentSecurityPolicySourceListDirective*
>     if (!directive)
>         return true;
>     bool didReceiveRedirectResponse = false;
>-    auto end = ancestorOrigins.end();
>-    for (auto it = ancestorOrigins.begin() + 1; it != end; ++it) {
>-        URL origin = urlFromOrigin(*(*it));
>-        if (!origin.isValid() || !directive->allows(origin, didReceiveRedirectResponse, ContentSecurityPolicySourceListDirective::ShouldAllowEmptyURLIfSourceListIsNotNone::No))
>+    for (auto& origin : ancestorOrigins) {
>+        URL originURL = urlFromOrigin(*origin);
>+        if (!originURL.isValid() || !directive->allows(originURL, didReceiveRedirectResponse, ContentSecurityPolicySourceListDirective::ShouldAllowEmptyURLIfSourceListIsNotNone::No))
>             return false;
>     }
>     return true;
>diff --git a/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp b/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>index c2a4b9537b437cf3303aa49ccec66a279feae99a..904d0b188e5f4df5ee8f6b20dc68f6d431860ceb 100644
>--- a/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>@@ -374,7 +374,8 @@ bool NetworkResourceLoader::shouldInterruptLoadForXFrameOptions(const String& xF
>         return true;
>     case XFrameOptionsSameOrigin: {
>         auto origin = SecurityOrigin::create(url);
>-        if (!origin->isSameSchemeHostPort(*m_parameters.sourceOrigin))
>+        auto topFrameOrigin = m_parameters.frameAncestorOrigins.last();
>+        if (!origin->isSameSchemeHostPort(*topFrameOrigin))
>             return true;
>         for (auto& ancestorOrigin : m_parameters.frameAncestorOrigins) {
>             if (!origin->isSameSchemeHostPort(*ancestorOrigin))
>diff --git a/Source/WebKit/NetworkProcess/NetworkResourceLoader.h b/Source/WebKit/NetworkProcess/NetworkResourceLoader.h
>index 7925710d3eddc83fea64b9d53df2d96c6b1402ef..931f65e238d000f925266e61321ca0de83acf2a0 100644
>--- a/Source/WebKit/NetworkProcess/NetworkResourceLoader.h
>+++ b/Source/WebKit/NetworkProcess/NetworkResourceLoader.h
>@@ -113,7 +113,7 @@ public:
>     void convertToDownload(DownloadID, const WebCore::ResourceRequest&, const WebCore::ResourceResponse&);
> 
>     bool isMainResource() const { return m_parameters.request.requester() == WebCore::ResourceRequest::Requester::Main; }
>-    bool isMainFrameLoad() const { return isMainResource() && m_parameters.frameAncestorOrigins.size() == 1; }
>+    bool isMainFrameLoad() const { return isMainResource() && m_parameters.frameAncestorOrigins.isEmpty(); }
> 
>     bool isAlwaysOnLoggingAllowed() const;
> 
>diff --git a/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp b/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp
>index 15cb99b7a713c8513bc1b71f3028f6b855048db8..199bd4daab87c91349ceccc324d918702ba6e354 100644
>--- a/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp
>+++ b/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp
>@@ -331,7 +331,7 @@ void WebLoaderStrategy::scheduleLoadFromNetworkProcess(ResourceLoader& resourceL
> 
>     if (resourceLoader.options().mode == FetchOptions::Mode::Navigate) {
>         Vector<RefPtr<SecurityOrigin>> frameAncestorOrigins;
>-        for (auto* frame = resourceLoader.frame(); frame; frame = frame->tree().parent())
>+        for (auto* frame = resourceLoader.frame()->tree().parent(); frame; frame = frame->tree().parent())
>             frameAncestorOrigins.append(makeRefPtr(frame->document()->securityOrigin()));
>         loadParameters.frameAncestorOrigins = WTFMove(frameAncestorOrigins);
>     }
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index e4f31c7de77b5151243b38ea55d975869c5da1f6..7997919ec11e2028db8a2fe0d1be01c86352d352 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,19 @@
>+2018-06-26  Daniel Bates  <dabates@apple.com>
>+
>+        REGRESSION (r231479): Unable to buy Odeon cinema tickets in STP (bogus 'X-Frame-Options' to 'SAMEORIGIN')
>+        https://bugs.webkit.org/show_bug.cgi?id=186090
>+        <rdar://problem/40692595>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add a test to ensure that we allow a same-origin page with "X-Frame-Options: SAMEORIGIN" to
>+        load as a result of a redirected cross-origin POST request.
>+
>+        * http/tests/security/XFrameOptions/cross-origin-iframe-post-form-to-parent-same-origin-x-frame-options-page-allow-expected.txt: Added.
>+        * http/tests/security/XFrameOptions/cross-origin-iframe-post-form-to-parent-same-origin-x-frame-options-page-allow.html: Added.
>+        * http/tests/security/XFrameOptions/resources/post-form-to-x-frame-options-parent-same-origin-allow.html: Added.
>+        * http/tests/security/XFrameOptions/resources/x-frame-options-parent-same-origin-allow.cgi:
>+
> 2018-06-25  Youenn Fablet  <youenn@apple.com>
> 
>         imported/w3c/web-platform-tests/fetch/nosniff/importscripts.html is crashing on debug builds
>diff --git a/LayoutTests/http/tests/security/XFrameOptions/cross-origin-iframe-post-form-to-parent-same-origin-x-frame-options-page-allow-expected.txt b/LayoutTests/http/tests/security/XFrameOptions/cross-origin-iframe-post-form-to-parent-same-origin-x-frame-options-page-allow-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..e35e4d350db33f6b4eb9fc073857478baaffc452
>--- /dev/null
>+++ b/LayoutTests/http/tests/security/XFrameOptions/cross-origin-iframe-post-form-to-parent-same-origin-x-frame-options-page-allow-expected.txt
>@@ -0,0 +1,8 @@
>+There should be content in the iframe below
>+
>+
>+
>+--------
>+Frame: ''
>+--------
>+PASS: This should show up as the parent is in the same origin.
>diff --git a/LayoutTests/http/tests/security/XFrameOptions/cross-origin-iframe-post-form-to-parent-same-origin-x-frame-options-page-allow.html b/LayoutTests/http/tests/security/XFrameOptions/cross-origin-iframe-post-form-to-parent-same-origin-x-frame-options-page-allow.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..c965b980aa5a21b13023b084502c40dfc35f22cc
>--- /dev/null
>+++ b/LayoutTests/http/tests/security/XFrameOptions/cross-origin-iframe-post-form-to-parent-same-origin-x-frame-options-page-allow.html
>@@ -0,0 +1,10 @@
>+<script>
>+    if (window.testRunner) {
>+        testRunner.dumpAsText();
>+        testRunner.dumpChildFramesAsText();
>+        testRunner.waitUntilDone();
>+    }
>+</script>
>+
>+<p>There should be content in the iframe below</p>
>+<iframe style="width:500px; height:500px" src="http://localhost:8000/security/XFrameOptions/resources/post-form-to-x-frame-options-parent-same-origin-allow.html"></iframe>
>diff --git a/LayoutTests/http/tests/security/XFrameOptions/resources/post-form-to-x-frame-options-parent-same-origin-allow.html b/LayoutTests/http/tests/security/XFrameOptions/resources/post-form-to-x-frame-options-parent-same-origin-allow.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..a27e5a72ce87baf5dc285eaead773a5ea189a118
>--- /dev/null
>+++ b/LayoutTests/http/tests/security/XFrameOptions/resources/post-form-to-x-frame-options-parent-same-origin-allow.html
>@@ -0,0 +1,13 @@
>+<!DOCTYPE html>
>+<html>
>+<body>
>+<p>Posting form...</p>
>+<form action="http://127.0.0.1:8000/security/XFrameOptions/resources/x-frame-options-parent-same-origin-allow.cgi" method="POST">
>+    <input type="hidden" name="notifyDone" value="1">
>+    <input type="submit" name="Submit">
>+</form>
>+<script>
>+document.forms[0].submit();
>+</script>
>+</body>
>+</html>
>diff --git a/LayoutTests/http/tests/security/XFrameOptions/resources/x-frame-options-parent-same-origin-allow.cgi b/LayoutTests/http/tests/security/XFrameOptions/resources/x-frame-options-parent-same-origin-allow.cgi
>index 1906530fab13def08852bb68e58eed110f5de5d3..94cfb303a85898646bf959e15ec7c3d37b1ce828 100755
>--- a/LayoutTests/http/tests/security/XFrameOptions/resources/x-frame-options-parent-same-origin-allow.cgi
>+++ b/LayoutTests/http/tests/security/XFrameOptions/resources/x-frame-options-parent-same-origin-allow.cgi
>@@ -1,8 +1,18 @@
> #!/usr/bin/perl -wT
> use strict;
>+use CGI;
>+
>+my $cgi = new CGI;
> 
> print "Content-Type: text/html\n";
> print "Cache-Control: no-cache, no-store\n";
> print "X-FRAME-OPTIONS: sameorigin\n\n";
> 
> print "<p>PASS: This should show up as the parent is in the same origin.</p>\n";
>+
>+if ($cgi->param("notifyDone")) {
>+    print "<script>\n";
>+    print "if (window.testRunner)\n";
>+    print "    testRunner.notifyDone();\n";
>+    print "</script>\n";
>+}

Actions: View | Formatted Diff | Diff

Attachments on bug 186090: 343562 | 343595 | 343634