- Revision
- 231935
- Author
- [email protected]
- Date
- 2018-05-17 17:52:28 -0700 (Thu, 17 May 2018)
Log Message
RenderLayer::scrollRectToVisible() should not propagate a subframe's scroll to its cross-origin parent
https://bugs.webkit.org/show_bug.cgi?id=185664
<rdar://problem/36185260>
Reviewed by Simon Fraser.
Source/WebCore:
RenderLayer::scrollRectToVisible() should not propagate a subframe's scroll to its
cross-origin parent. There was logic in FrameLoader::scrollToFragmentWithParentBoundary()
to temporarily set the 'safeToPropagateScrollToParent' flag to false on the cross-origin
ancestor frame during the call to FrameView::scrollToFragment(). This would correctly
prevent RenderLayer::scrollRectToVisible() to propagate the scroll to the cross-origin
ancestor frame when scrollRectToVisible() is called synchronously. However,
scrollRectToVisible() can get called asynchronously in case of a dirty layout, as part
of the post layout tasks.
To address the issue, we get rid of the safeToPropagateScrollToParent flag on FrameView
and instead update FrameView::safeToPropagateScrollToParent() to do the cross-origin
check. FrameView::safeToPropagateScrollToParent() is called by RenderLayer::scrollRectToVisible()
and this is a lot more robust than relying on a flag which gets temporarily set.
Test: http/tests/navigation/fragment-navigation-cross-origin-subframe-no-scrolling-parent.html
* dom/Document.cpp:
* dom/Document.h:
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::scrollToFragmentWithParentBoundary):
* page/FrameView.cpp:
(WebCore::FrameView::FrameView):
(WebCore::FrameView::reset):
(WebCore::FrameView::safeToPropagateScrollToParent const):
* page/FrameView.h:
LayoutTests:
Add layout test coverage.
* http/tests/navigation/fragment-navigation-cross-origin-subframe-no-scrolling-parent-expected.txt: Added.
* http/tests/navigation/fragment-navigation-cross-origin-subframe-no-scrolling-parent.html: Added.
* http/tests/navigation/resources/clear-fragment.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (231934 => 231935)
--- trunk/LayoutTests/ChangeLog 2018-05-18 00:17:46 UTC (rev 231934)
+++ trunk/LayoutTests/ChangeLog 2018-05-18 00:52:28 UTC (rev 231935)
@@ -1,3 +1,17 @@
+2018-05-17 Chris Dumez <[email protected]>
+
+ RenderLayer::scrollRectToVisible() should not propagate a subframe's scroll to its cross-origin parent
+ https://bugs.webkit.org/show_bug.cgi?id=185664
+ <rdar://problem/36185260>
+
+ Reviewed by Simon Fraser.
+
+ Add layout test coverage.
+
+ * http/tests/navigation/fragment-navigation-cross-origin-subframe-no-scrolling-parent-expected.txt: Added.
+ * http/tests/navigation/fragment-navigation-cross-origin-subframe-no-scrolling-parent.html: Added.
+ * http/tests/navigation/resources/clear-fragment.html: Added.
+
2018-05-17 Ryan Haddad <[email protected]>
Unreviewed, rolling out r231899.
Added: trunk/LayoutTests/http/tests/navigation/fragment-navigation-cross-origin-subframe-no-scrolling-parent-expected.txt (0 => 231935)
--- trunk/LayoutTests/http/tests/navigation/fragment-navigation-cross-origin-subframe-no-scrolling-parent-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/navigation/fragment-navigation-cross-origin-subframe-no-scrolling-parent-expected.txt 2018-05-18 00:52:28 UTC (rev 231935)
@@ -0,0 +1,10 @@
+Tests that a fragment navigation in a cross-origin subframe does not scroll its parent.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS window.scrollY is 0
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/http/tests/navigation/fragment-navigation-cross-origin-subframe-no-scrolling-parent.html (0 => 231935)
--- trunk/LayoutTests/http/tests/navigation/fragment-navigation-cross-origin-subframe-no-scrolling-parent.html (rev 0)
+++ trunk/LayoutTests/http/tests/navigation/fragment-navigation-cross-origin-subframe-no-scrolling-parent.html 2018-05-18 00:52:28 UTC (rev 231935)
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<script>
+description("Tests that a fragment navigation in a cross-origin subframe does not scroll its parent.");
+jsTestIsAsync = true;
+
+_onload_ = () => {
+ setTimeout(function() {
+ shouldBe("window.scrollY", "0");
+ finishJSTest();
+ }, 0);
+}
+</script>
+<iframe src="" style="position: relative; top: 800px;"></iframe>
+</body>
+</html>
Added: trunk/LayoutTests/http/tests/navigation/resources/clear-fragment.html (0 => 231935)
--- trunk/LayoutTests/http/tests/navigation/resources/clear-fragment.html (rev 0)
+++ trunk/LayoutTests/http/tests/navigation/resources/clear-fragment.html 2018-05-18 00:52:28 UTC (rev 231935)
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script>
+document.location.hash = '';
+</script>
+<div id="test">TEST</div>
+<script>
+test.offsetHeight;
+</script>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (231934 => 231935)
--- trunk/Source/WebCore/ChangeLog 2018-05-18 00:17:46 UTC (rev 231934)
+++ trunk/Source/WebCore/ChangeLog 2018-05-18 00:52:28 UTC (rev 231935)
@@ -1,3 +1,37 @@
+2018-05-17 Chris Dumez <[email protected]>
+
+ RenderLayer::scrollRectToVisible() should not propagate a subframe's scroll to its cross-origin parent
+ https://bugs.webkit.org/show_bug.cgi?id=185664
+ <rdar://problem/36185260>
+
+ Reviewed by Simon Fraser.
+
+ RenderLayer::scrollRectToVisible() should not propagate a subframe's scroll to its
+ cross-origin parent. There was logic in FrameLoader::scrollToFragmentWithParentBoundary()
+ to temporarily set the 'safeToPropagateScrollToParent' flag to false on the cross-origin
+ ancestor frame during the call to FrameView::scrollToFragment(). This would correctly
+ prevent RenderLayer::scrollRectToVisible() to propagate the scroll to the cross-origin
+ ancestor frame when scrollRectToVisible() is called synchronously. However,
+ scrollRectToVisible() can get called asynchronously in case of a dirty layout, as part
+ of the post layout tasks.
+
+ To address the issue, we get rid of the safeToPropagateScrollToParent flag on FrameView
+ and instead update FrameView::safeToPropagateScrollToParent() to do the cross-origin
+ check. FrameView::safeToPropagateScrollToParent() is called by RenderLayer::scrollRectToVisible()
+ and this is a lot more robust than relying on a flag which gets temporarily set.
+
+ Test: http/tests/navigation/fragment-navigation-cross-origin-subframe-no-scrolling-parent.html
+
+ * dom/Document.cpp:
+ * dom/Document.h:
+ * loader/FrameLoader.cpp:
+ (WebCore::FrameLoader::scrollToFragmentWithParentBoundary):
+ * page/FrameView.cpp:
+ (WebCore::FrameView::FrameView):
+ (WebCore::FrameView::reset):
+ (WebCore::FrameView::safeToPropagateScrollToParent const):
+ * page/FrameView.h:
+
2018-05-17 Don Olmstead <[email protected]>
[Curl] Enable HTTP/2 Multiplexing
Modified: trunk/Source/WebCore/dom/Document.cpp (231934 => 231935)
--- trunk/Source/WebCore/dom/Document.cpp 2018-05-18 00:17:46 UTC (rev 231934)
+++ trunk/Source/WebCore/dom/Document.cpp 2018-05-18 00:52:28 UTC (rev 231935)
@@ -3264,23 +3264,6 @@
return false;
}
-Frame* Document::findUnsafeParentScrollPropagationBoundary()
-{
- Frame* currentFrame = m_frame;
- if (!currentFrame)
- return nullptr;
-
- Frame* ancestorFrame = currentFrame->tree().parent();
-
- while (ancestorFrame) {
- if (!ancestorFrame->document()->securityOrigin().canAccess(securityOrigin()))
- return currentFrame;
- currentFrame = ancestorFrame;
- ancestorFrame = ancestorFrame->tree().parent();
- }
- return nullptr;
-}
-
void Document::didRemoveAllPendingStylesheet()
{
if (auto* parser = scriptableDocumentParser())
Modified: trunk/Source/WebCore/dom/Document.h (231934 => 231935)
--- trunk/Source/WebCore/dom/Document.h 2018-05-18 00:17:46 UTC (rev 231934)
+++ trunk/Source/WebCore/dom/Document.h 2018-05-18 00:52:28 UTC (rev 231935)
@@ -675,7 +675,6 @@
SocketProvider* socketProvider() final;
bool canNavigate(Frame* targetFrame);
- Frame* findUnsafeParentScrollPropagationBoundary();
bool usesStyleBasedEditability() const;
void setHasElementUsingStyleBasedEditability();
Modified: trunk/Source/WebCore/loader/FrameLoader.cpp (231934 => 231935)
--- trunk/Source/WebCore/loader/FrameLoader.cpp 2018-05-18 00:17:46 UTC (rev 231934)
+++ trunk/Source/WebCore/loader/FrameLoader.cpp 2018-05-18 00:52:28 UTC (rev 231935)
@@ -3028,17 +3028,8 @@
if (!view)
return;
- // Leaking scroll position to a cross-origin ancestor would permit the so-called "framesniffing" attack.
- RefPtr<Frame> boundaryFrame(url.hasFragmentIdentifier() ? m_frame.document()->findUnsafeParentScrollPropagationBoundary() : 0);
-
- if (boundaryFrame)
- boundaryFrame->view()->setSafeToPropagateScrollToParent(false);
-
if (isSameDocumentReload(isNewNavigation, m_loadType) || itemAllowsScrollRestoration(history().currentItem()))
view->scrollToFragment(url);
-
- if (boundaryFrame)
- boundaryFrame->view()->setSafeToPropagateScrollToParent(true);
}
bool FrameLoader::shouldClose()
Modified: trunk/Source/WebCore/page/FrameView.cpp (231934 => 231935)
--- trunk/Source/WebCore/page/FrameView.cpp 2018-05-18 00:17:46 UTC (rev 231934)
+++ trunk/Source/WebCore/page/FrameView.cpp 2018-05-18 00:52:28 UTC (rev 231935)
@@ -179,7 +179,6 @@
, m_overflowStatusDirty(true)
, m_wasScrolledByUser(false)
, m_inProgrammaticScroll(false)
- , m_safeToPropagateScrollToParent(true)
, m_delayedScrollEventTimer(*this, &FrameView::sendScrollEvent)
, m_selectionRevealModeForFocusedElement(SelectionRevealMode::DoNotReveal)
, m_delayedScrollToFocusedElementTimer(*this, &FrameView::scrollToFocusedElementTimerFired)
@@ -264,7 +263,6 @@
m_updateEmbeddedObjectsTimer.stop();
m_firstLayoutCallbackPending = false;
m_wasScrolledByUser = false;
- m_safeToPropagateScrollToParent = true;
m_delayedScrollEventTimer.stop();
m_shouldScrollToFocusedElement = false;
m_delayedScrollToFocusedElementTimer.stop();
@@ -3059,6 +3057,23 @@
return true;
}
+bool FrameView::safeToPropagateScrollToParent() const
+{
+ auto* document = frame().document();
+ if (!document)
+ return false;
+
+ auto* parentFrame = frame().tree().parent();
+ if (!parentFrame)
+ return false;
+
+ auto* parentDocument = parentFrame->document();
+ if (!parentDocument)
+ return false;
+
+ return document->securityOrigin().canAccess(parentDocument->securityOrigin());
+}
+
void FrameView::scrollToAnchor()
{
RefPtr<ContainerNode> anchorNode = m_maintainScrollPositionAnchor;
Modified: trunk/Source/WebCore/page/FrameView.h (231934 => 231935)
--- trunk/Source/WebCore/page/FrameView.h 2018-05-18 00:17:46 UTC (rev 231934)
+++ trunk/Source/WebCore/page/FrameView.h 2018-05-18 00:52:28 UTC (rev 231935)
@@ -337,8 +337,7 @@
WEBCORE_EXPORT bool wasScrolledByUser() const;
WEBCORE_EXPORT void setWasScrolledByUser(bool);
- bool safeToPropagateScrollToParent() const { return m_safeToPropagateScrollToParent; }
- void setSafeToPropagateScrollToParent(bool isSafe) { m_safeToPropagateScrollToParent = isSafe; }
+ bool safeToPropagateScrollToParent() const;
void addEmbeddedObjectToUpdate(RenderEmbeddedObject&);
void removeEmbeddedObjectToUpdate(RenderEmbeddedObject&);
@@ -823,7 +822,6 @@
bool m_wasScrolledByUser;
bool m_inProgrammaticScroll;
- bool m_safeToPropagateScrollToParent;
Timer m_delayedScrollEventTimer;
bool m_shouldScrollToFocusedElement { false };
SelectionRevealMode m_selectionRevealModeForFocusedElement;
