Attachment 349701 Details for Bug 189590 – Patch

[patch] Patch

bug-189590-20180913140606.patch (text/plain), 17.85 KB, created by Chris Dumez on 2018-09-13 14:06:07 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-09-13 14:06:07 PDT

Size: 17.85 KB

patch

obsolete

>Subversion Revision: 235987
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 0b2a95fcc4a2b4dcf5c277d5ccce145d4e4fdc1d..cacb2571d1bb404ff88da61d5159873943b0cf03 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,26 @@
>+2018-09-13  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression(PSON): setting window.opener to null allows process swapping in cases that are not web-compatible
>+        https://bugs.webkit.org/show_bug.cgi?id=189590
>+        <rdar://problem/44422725>
>+
>+        Reviewed by Geoffrey Garen.
>+
>+        Set a flag on the navigation action to indicate if the page was opened via window.open() without 'noopener'.
>+
>+        Test: http/tests/navigation/window-open-cross-origin-then-navigated-back-same-origin.html
>+
>+        * loader/FrameLoader.cpp:
>+        (WebCore::FrameLoader::loadURL):
>+        * loader/NavigationAction.h:
>+        (WebCore::NavigationAction::openedViaWindowOpenWithOpener const):
>+        (WebCore::NavigationAction::setOpenedViaWindowOpenWithOpener):
>+        * page/DOMWindow.cpp:
>+        (WebCore::DOMWindow::createWindow):
>+        * page/Page.h:
>+        (WebCore::Page::openedViaWindowOpenWithOpener const):
>+        (WebCore::Page::setOpenedViaWindowOpenWithOpener):
>+
> 2018-09-13  Fujii Hironori  <Hironori.Fujii@sony.com>
> 
>         Remove the workaround for friend class LazyNeverDestroyed<X> statements for MSVC
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 47d57ff4f4fdf4b791e6b2d3f893fc80596020b5..8823906738e92d8e49f8cce53ad074b4c3bd38da 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,36 @@
>+2018-09-13  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression(PSON): setting window.opener to null allows process swapping in cases that are not web-compatible
>+        https://bugs.webkit.org/show_bug.cgi?id=189590
>+        <rdar://problem/44422725>
>+
>+        Reviewed by Geoffrey Garen.
>+
>+        If script calls window.open() without 'noopener' and the newly navigated window gets navigated cross-site,
>+        we are currently unable to process-swap because the opener has a WindowProxy handle to this new Window and
>+        may interact with it (which we currently do not support cross-process). We were dealing with this by not
>+        process-swapping if window.opener is not null. This works most of the time but is not sufficient because the
>+        opener may get nulled out, while the opener still has a valid WindowProxy handle to its openee.
>+
>+        Therefore, we now also check for a flag indicating if the frame was opened via window.open() without
>+        'nooopener'. We still need to check if the browsing context has an opener for browsing context created
>+        via <a target="_blank"> for example (the opener does not have a handle to the new window but the openee
>+        has access to its opener).
>+
>+        * Shared/NavigationActionData.cpp:
>+        (WebKit::NavigationActionData::encode const):
>+        (WebKit::NavigationActionData::decode):
>+        * Shared/NavigationActionData.h:
>+        * UIProcess/API/APINavigation.h:
>+        (API::Navigation::openedViaWindowOpenWithOpener const):
>+        (API::Navigation::setOpenedViaWindowOpenWithOpener):
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::decidePolicyForNavigationAction):
>+        * UIProcess/WebProcessPool.cpp:
>+        (WebKit::WebProcessPool::processForNavigationInternal):
>+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
>+        (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
>+
> 2018-09-13  Chris Dumez  <cdumez@apple.com>
> 
>         Add release logging to help debug PSON issues
>diff --git a/Source/WebCore/loader/FrameLoader.cpp b/Source/WebCore/loader/FrameLoader.cpp
>index 6ab68551f8584d3faddbbfb8198c8d5d5a2f0857..20a1f784f0d04a2ec1962cee503e0847f8225a06 100644
>--- a/Source/WebCore/loader/FrameLoader.cpp
>+++ b/Source/WebCore/loader/FrameLoader.cpp
>@@ -1364,6 +1364,8 @@ void FrameLoader::loadURL(FrameLoadRequest&& frameLoadRequest, const String& ref
> 
>     NavigationAction action { frameLoadRequest.requester(), request, frameLoadRequest.initiatedByMainFrame(), newLoadType, isFormSubmission, event, frameLoadRequest.shouldOpenExternalURLsPolicy(), frameLoadRequest.downloadAttribute() };
>     action.setIsCrossOriginWindowOpenNavigation(frameLoadRequest.isCrossOriginWindowOpenNavigation());
>+    if (m_frame.page() && m_frame.page()->openedViaWindowOpenWithOpener())
>+        action.setOpenedViaWindowOpenWithOpener();
>     action.setHasOpenedFrames(!m_openedFrames.isEmpty());
>     if (auto* opener = this->opener()) {
>         auto pageID = opener->loader().client().pageID();
>diff --git a/Source/WebCore/loader/NavigationAction.h b/Source/WebCore/loader/NavigationAction.h
>index 740d51394eb78b9d1375421df3f0cb41f306b12a..95bce229bc81579825e89bff288a230ab3bf814a 100644
>--- a/Source/WebCore/loader/NavigationAction.h
>+++ b/Source/WebCore/loader/NavigationAction.h
>@@ -126,6 +126,9 @@ public:
>     bool hasOpenedFrames() const { return m_hasOpenedFrames; }
>     void setHasOpenedFrames(bool value) { m_hasOpenedFrames = value; }
> 
>+    bool openedViaWindowOpenWithOpener() const { return m_openedViaWindowOpenWithOpener; }
>+    void setOpenedViaWindowOpenWithOpener() { m_openedViaWindowOpenWithOpener = true; }
>+
>     void setTargetBackForwardItem(HistoryItem&);
>     const std::optional<BackForwardItemIdentifier>& targetBackForwardItemIdentifier() const { return m_targetBackForwardItemIdentifier; }
> 
>@@ -144,6 +147,7 @@ private:
>     bool m_treatAsSameOriginNavigation;
>     bool m_isCrossOriginWindowOpenNavigation { false };
>     bool m_hasOpenedFrames { false };
>+    bool m_openedViaWindowOpenWithOpener { false };
>     std::optional<PageIDAndFrameIDPair> m_opener;
>     std::optional<BackForwardItemIdentifier> m_targetBackForwardItemIdentifier;
> };
>diff --git a/Source/WebCore/page/DOMWindow.cpp b/Source/WebCore/page/DOMWindow.cpp
>index 04fc30549b26b3361abe16ead94e44929a386d0c..fb68507db181f96720b2d981804c3445756faf3e 100644
>--- a/Source/WebCore/page/DOMWindow.cpp
>+++ b/Source/WebCore/page/DOMWindow.cpp
>@@ -2264,8 +2264,10 @@ ExceptionOr<RefPtr<Frame>> DOMWindow::createWindow(const String& urlString, cons
>     if (!newFrame)
>         return RefPtr<Frame> { nullptr };
> 
>-    if (!windowFeatures.noopener)
>+    if (!windowFeatures.noopener) {
>         newFrame->loader().setOpener(&openerFrame);
>+        newFrame->page()->setOpenedViaWindowOpenWithOpener();
>+    }
>     newFrame->page()->setOpenedByDOM();
> 
>     if (newFrame->document()->domWindow()->isInsecureScriptAccess(activeWindow, completedURL))
>diff --git a/Source/WebCore/page/Page.h b/Source/WebCore/page/Page.h
>index 6c1d432f9da25ced82be358b9919786cbf9f62c0..24b41de3d61fa16818821ed30cd7e7b94f680d71 100644
>--- a/Source/WebCore/page/Page.h
>+++ b/Source/WebCore/page/Page.h
>@@ -202,6 +202,9 @@ public:
>     bool openedByDOM() const;
>     void setOpenedByDOM();
> 
>+    bool openedViaWindowOpenWithOpener() const { return m_openedViaWindowOpenWithOpener; }
>+    void setOpenedViaWindowOpenWithOpener() { m_openedViaWindowOpenWithOpener = true; }
>+
>     WEBCORE_EXPORT void goToItem(HistoryItem&, FrameLoadType, ShouldTreatAsContinuingLoad);
> 
>     WEBCORE_EXPORT void setGroupName(const String&);
>@@ -750,6 +753,7 @@ private:
>     int m_subframeCount { 0 };
>     String m_groupName;
>     bool m_openedByDOM { false };
>+    bool m_openedViaWindowOpenWithOpener { false };
> 
>     bool m_tabKeyCyclesThroughElements { true };
>     bool m_defersLoading { false };
>diff --git a/Source/WebKit/Shared/NavigationActionData.cpp b/Source/WebKit/Shared/NavigationActionData.cpp
>index 7f52f41b4701ad16c763f9f4b40fe743abe7f348..95496dab0c71fc508975ee7e0f36c00c659c8d66 100644
>--- a/Source/WebKit/Shared/NavigationActionData.cpp
>+++ b/Source/WebKit/Shared/NavigationActionData.cpp
>@@ -48,6 +48,7 @@ void NavigationActionData::encode(IPC::Encoder& encoder) const
>     encoder << treatAsSameOriginNavigation;
>     encoder << isCrossOriginWindowOpenNavigation;
>     encoder << hasOpenedFrames;
>+    encoder << openedViaWindowOpenWithOpener;
>     encoder << opener;
>     encoder << targetBackForwardItemIdentifier;
> }
>@@ -113,6 +114,11 @@ std::optional<NavigationActionData> NavigationActionData::decode(IPC::Decoder& d
>     if (!hasOpenedFrames)
>         return std::nullopt;
> 
>+    std::optional<bool> openedViaWindowOpenWithOpener;
>+    decoder >> openedViaWindowOpenWithOpener;
>+    if (!openedViaWindowOpenWithOpener)
>+        return std::nullopt;
>+
>     std::optional<std::optional<std::pair<uint64_t, uint64_t>>> opener;
>     decoder >> opener;
>     if (!opener)
>@@ -125,7 +131,7 @@ std::optional<NavigationActionData> NavigationActionData::decode(IPC::Decoder& d
>         
>     return {{ WTFMove(navigationType), WTFMove(modifiers), WTFMove(mouseButton), WTFMove(syntheticClickType), WTFMove(*userGestureTokenIdentifier),
>         WTFMove(*canHandleRequest), WTFMove(shouldOpenExternalURLsPolicy), WTFMove(*downloadAttribute), WTFMove(clickLocationInRootViewCoordinates),
>-        WTFMove(*isRedirect), *treatAsSameOriginNavigation, *isCrossOriginWindowOpenNavigation, *hasOpenedFrames, WTFMove(*opener), WTFMove(*targetBackForwardItemIdentifier) }};
>+        WTFMove(*isRedirect), *treatAsSameOriginNavigation, *isCrossOriginWindowOpenNavigation, *hasOpenedFrames, *openedViaWindowOpenWithOpener, WTFMove(*opener), WTFMove(*targetBackForwardItemIdentifier) }};
> }
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/Shared/NavigationActionData.h b/Source/WebKit/Shared/NavigationActionData.h
>index 09db575641f6bc2636b76bc28c75090e3401a046..79008d845619af6122754a31f8db4b4a964a1f9f 100644
>--- a/Source/WebKit/Shared/NavigationActionData.h
>+++ b/Source/WebKit/Shared/NavigationActionData.h
>@@ -54,6 +54,7 @@ struct NavigationActionData {
>     bool treatAsSameOriginNavigation { false };
>     bool isCrossOriginWindowOpenNavigation { false };
>     bool hasOpenedFrames { false };
>+    bool openedViaWindowOpenWithOpener { false };
>     std::optional<std::pair<uint64_t, uint64_t>> opener;
>     std::optional<WebCore::BackForwardItemIdentifier> targetBackForwardItemIdentifier;
> };
>diff --git a/Source/WebKit/UIProcess/API/APINavigation.h b/Source/WebKit/UIProcess/API/APINavigation.h
>index 0f95fc5f9b21d25e610fa80f23cc0405d5c53f32..abd85c73eba69991713f8822cf316553255a7bbc 100644
>--- a/Source/WebKit/UIProcess/API/APINavigation.h
>+++ b/Source/WebKit/UIProcess/API/APINavigation.h
>@@ -94,6 +94,9 @@ public:
>     void setHasOpenedFrames(bool value) { m_hasOpenedFrames = value; }
>     bool hasOpenedFrames() const { return m_hasOpenedFrames; }
> 
>+    bool openedViaWindowOpenWithOpener() const { return m_openedViaWindowOpenWithOpener; }
>+    void setOpenedViaWindowOpenWithOpener() { m_openedViaWindowOpenWithOpener = true; }
>+
>     void setOpener(const std::optional<std::pair<uint64_t, uint64_t>>& opener) { m_opener = opener; }
>     const std::optional<std::pair<uint64_t, uint64_t>>& opener() const { return m_opener; }
> 
>@@ -121,6 +124,7 @@ private:
>     bool m_treatAsSameOriginNavigation { false };
>     bool m_isCrossOriginWindowOpenNavigation { false };
>     bool m_hasOpenedFrames { false };
>+    bool m_openedViaWindowOpenWithOpener { false };
>     std::optional<std::pair<uint64_t, uint64_t>> m_opener;
> };
> 
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 7eca8fdfb6776c3055352c820061b7e8c1d9a068..21f01c3ba245dcbf32f7409e00393f7cd9cc8951 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -4020,6 +4020,8 @@ void WebPageProxy::decidePolicyForNavigationAction(uint64_t frameID, const WebCo
>     navigation->setTreatAsSameOriginNavigation(navigationActionData.treatAsSameOriginNavigation);
>     navigation->setIsCrossOriginWindowOpenNavigation(navigationActionData.isCrossOriginWindowOpenNavigation);
>     navigation->setHasOpenedFrames(navigationActionData.hasOpenedFrames);
>+    if (navigationActionData.openedViaWindowOpenWithOpener)
>+        navigation->setOpenedViaWindowOpenWithOpener();
>     navigation->setOpener(navigationActionData.opener);
> 
> #if ENABLE(CONTENT_FILTERING)
>diff --git a/Source/WebKit/UIProcess/WebProcessPool.cpp b/Source/WebKit/UIProcess/WebProcessPool.cpp
>index 95341a40d0f7c6f87017e7f9fcf82322b126af12..5133ff664ffec0f00cac881b6987d5600ed38368 100644
>--- a/Source/WebKit/UIProcess/WebProcessPool.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessPool.cpp
>@@ -2153,6 +2153,13 @@ Ref<WebProcessProxy> WebProcessPool::processForNavigationInternal(WebPageProxy&
>         return createNewWebProcess(page.websiteDataStore());
>     }
> 
>+    // FIXME: We should support process swap when a window has been opened via window.open() without 'noopener'.
>+    // The issue is that the opener has a handle to the WindowProxy.
>+    if (navigation.openedViaWindowOpenWithOpener()) {
>+        reason = "Browsing context been opened via window.open() without 'noopener'"_s;
>+        return page.process();
>+    }
>+
>     // FIXME: We should support process swap when a window has an opener.
>     if (navigation.opener()) {
>         reason = "Browsing context has an opener"_s;
>diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp b/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
>index 3503072025501bf56b0ebffbab6c25c184902a19..5b1f37a79ecc2c830c335b6d1a049f3626744506 100644
>--- a/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
>+++ b/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
>@@ -868,6 +868,7 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const Navigat
>     navigationActionData.treatAsSameOriginNavigation = navigationAction.treatAsSameOriginNavigation();
>     navigationActionData.isCrossOriginWindowOpenNavigation = navigationAction.isCrossOriginWindowOpenNavigation();
>     navigationActionData.hasOpenedFrames = navigationAction.hasOpenedFrames();
>+    navigationActionData.openedViaWindowOpenWithOpener = navigationAction.openedViaWindowOpenWithOpener();
>     navigationActionData.opener = navigationAction.opener();
>     navigationActionData.targetBackForwardItemIdentifier = navigationAction.targetBackForwardItemIdentifier();
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index ecd04e17b43e04d52014251a52d87c6cd5947417..5d676ed0630be913767d6e0dd7b253e669713594 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2018-09-13  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression(PSON): setting window.opener to null allows process swapping in cases that are not web-compatible
>+        https://bugs.webkit.org/show_bug.cgi?id=189590
>+        <rdar://problem/44422725>
>+
>+        Reviewed by Geoffrey Garen.
>+
>+        Add layout test coverage.
>+
>+        * http/tests/navigation/resources/navigate-helper.html: Added.
>+        * http/tests/navigation/window-open-cross-origin-then-navigated-back-same-origin-expected.txt: Added.
>+        * http/tests/navigation/window-open-cross-origin-then-navigated-back-same-origin.html: Added.
>+
> 2018-09-13  Dean Jackson  <dino@grorg.org>
> 
>         https://bugs.webkit.org/show_bug.cgi?id=189594
>diff --git a/LayoutTests/http/tests/navigation/resources/navigate-helper.html b/LayoutTests/http/tests/navigation/resources/navigate-helper.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..dfc9e6ecdbb0c389eaf0e708cca38c41863bb76a
>--- /dev/null
>+++ b/LayoutTests/http/tests/navigation/resources/navigate-helper.html
>@@ -0,0 +1,12 @@
>+<!DOCTYPE html>
>+<html>
>+<body>
>+<script>
>+onmessage = (msg) => {
>+    setTimeout(() => {
>+        window.location = msg.data.navigate;
>+    }, 0);
>+};
>+</script>
>+</body>
>+</html>
>diff --git a/LayoutTests/http/tests/navigation/window-open-cross-origin-then-navigated-back-same-origin-expected.txt b/LayoutTests/http/tests/navigation/window-open-cross-origin-then-navigated-back-same-origin-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..509ab3ffc533e916aae0758a81c1aa685eaf2f5b
>--- /dev/null
>+++ b/LayoutTests/http/tests/navigation/window-open-cross-origin-then-navigated-back-same-origin-expected.txt
>@@ -0,0 +1,10 @@
>+Tests that process swap on navigation does not break navigating back cross-origin a cross-origin window.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS w.location.href is "http://127.0.0.1:8000/navigation/resources/otherpage.html"
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/http/tests/navigation/window-open-cross-origin-then-navigated-back-same-origin.html b/LayoutTests/http/tests/navigation/window-open-cross-origin-then-navigated-back-same-origin.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..ebcbea8d4c550e7688e47def57431552d3da7511
>--- /dev/null
>+++ b/LayoutTests/http/tests/navigation/window-open-cross-origin-then-navigated-back-same-origin.html
>@@ -0,0 +1,49 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<script src="/js-test-resources/js-test.js"></script>
>+</head>
>+<body>
>+<script>
>+description("Tests that process swap on navigation does not break navigating back cross-origin a cross-origin window.");
>+jsTestIsAsync = true;
>+
>+if (window.testRunner)
>+    testRunner.setCanOpenWindows();
>+
>+function wasNavigatedSameOrigin()
>+{
>+    shouldBeEqualToString("w.location.href", "http://127.0.0.1:8000/navigation/resources/otherpage.html");
>+    finishJSTest();
>+}
>+
>+function wasNavigatedCrossOrigin()
>+{
>+    // Navigate back same origin.
>+    w.postMessage({ navigate: "http://127.0.0.1:8000/navigation/resources/otherpage.html" }, "*");
>+    handle = setInterval(() => {
>+        try {
>+            w.name;
>+            clearInterval(handle);
>+            wasNavigatedSameOrigin();
>+        } catch(e) {
>+        }
>+    }, 10);
>+}
>+
>+onload = function() {
>+    w = window.open();
>+    w.opener = null;
>+    w.location = "http://localhost:8000/navigation/resources/navigate-helper.html";
>+    handle = setInterval(() => {
>+        try {
>+            w.name;
>+        } catch(e) {
>+            clearInterval(handle);
>+            wasNavigatedCrossOrigin();
>+        }
>+    }, 10);
>+}
>+</script>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 189590: 349680 | 349695 | 349698 | 349701