Title: [231935] trunk
Revision
231935
Author
cdu...@apple.com
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  <cdu...@apple.com>
+
+        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  <ryanhad...@apple.com>
 
         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  <cdu...@apple.com>
+
+        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  <don.olmst...@sony.com>
 
         [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;
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to