WebKit Bugzilla
Attachment 349695 Details for
Bug 189590
: Regression(PSON): setting window.opener to null allows process swapping in cases that are not web-compatible
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-189590-20180913134924.patch (text/plain), 17.82 KB, created by
Chris Dumez
on 2018-09-13 13:49:25 PDT
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Chris Dumez
Created:
2018-09-13 13:49:25 PDT
Size:
17.82 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..a4e53ba290f1c1bae6c32a0a5c08d19646c0902f 100644 >--- a/Source/WebKit/ChangeLog >+++ b/Source/WebKit/ChangeLog >@@ -1,3 +1,34 @@ >+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, I replaced the window.opener check with a check for a flag indicating if the frame was opened via >+ window.open() without 'nooopener'. >+ >+ * 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..ec609511336bbc8ba6578c1ba2582e014430e60c 100644 >--- a/Source/WebKit/UIProcess/WebProcessPool.cpp >+++ b/Source/WebKit/UIProcess/WebProcessPool.cpp >@@ -2153,11 +2153,13 @@ Ref<WebProcessProxy> WebProcessPool::processForNavigationInternal(WebPageProxy& > return createNewWebProcess(page.websiteDataStore()); > } > >- // FIXME: We should support process swap when a window has an opener. >- if (navigation.opener()) { >- reason = "Browsing context has an opener"_s; >+ // 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(); > } >+ ASSERT(!navigation.opener); > > // FIXME: We should support process swap when a window has opened other windows via window.open. > if (navigation.hasOpenedFrames()) { >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>
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=349695">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 189590
:
349680
|
349695
|
349698
|
349701