Changeset 231935 in webkit

Timestamp:

May 17, 2018 5:52:28 PM (6 years ago)

Author:

Chris Dumez

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/tests/navigation/fragment-navigation-cross-origin-subframe-no-scrolling-parent-expected.txt (added)
• LayoutTests/http/tests/navigation/fragment-navigation-cross-origin-subframe-no-scrolling-parent.html (added)
• LayoutTests/http/tests/navigation/resources/clear-fragment.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/Document.cpp (modified) (1 diff)
• Source/WebCore/dom/Document.h (modified) (1 diff)
• Source/WebCore/loader/FrameLoader.cpp (modified) (1 diff)
• Source/WebCore/page/FrameView.cpp (modified) (3 diffs)
• Source/WebCore/page/FrameView.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2018-05-17  Chris Dumez  <cdumez@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  <ryanhaddad@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2018-05-17  Chris Dumez  <cdumez@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.olmstead@sony.com>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -3265 +3265 @@
 }
 
-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()
 {

trunk/Source/WebCore/dom/Document.h

@@ -676 +676 @@
 
     bool canNavigate(Frame* targetFrame);
-    Frame* findUnsafeParentScrollPropagationBoundary();
 
     bool usesStyleBasedEditability() const;

trunk/Source/WebCore/loader/FrameLoader.cpp

@@ -3029 +3029 @@
         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);
 }
 

trunk/Source/WebCore/page/FrameView.cpp

@@ -180 +180 @@
     , m_wasScrolledByUser(false)
     , m_inProgrammaticScroll(false)
-    , m_safeToPropagateScrollToParent(true)
     , m_delayedScrollEventTimer(*this, &FrameView::sendScrollEvent)
     , m_selectionRevealModeForFocusedElement(SelectionRevealMode::DoNotReveal)
@@ -265 +264 @@
     m_firstLayoutCallbackPending = false;
     m_wasScrolledByUser = false;
-    m_safeToPropagateScrollToParent = true;
     m_delayedScrollEventTimer.stop();
     m_shouldScrollToFocusedElement = false;
@@ -3058 +3056 @@
         return false;
     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());
 }
 

trunk/Source/WebCore/page/FrameView.h

@@ -338 +338 @@
     WEBCORE_EXPORT void setWasScrolledByUser(bool);
 
-    bool safeToPropagateScrollToParent() const { return m_safeToPropagateScrollToParent; }
-    void setSafeToPropagateScrollToParent(bool isSafe) { m_safeToPropagateScrollToParent = isSafe; }
+    bool safeToPropagateScrollToParent() const;
 
     void addEmbeddedObjectToUpdate(RenderEmbeddedObject&);
@@ -824 +823 @@
     bool m_wasScrolledByUser;
     bool m_inProgrammaticScroll;
-    bool m_safeToPropagateScrollToParent;
     Timer m_delayedScrollEventTimer;
     bool m_shouldScrollToFocusedElement { false };