Fix text fragment for user activation

For security reasons, text fragments must only be activated when
navigated with a user gesture. However, browser initiated navigations
(e.g. user typing in the omnibox, bookmarks) don't have the user gesture
bit set despite being initiated by the user (see discussion in
https://crrev.com/c/2132673 for details). Because of this limitation,
text fragment code explicitly checked if the navigation was browser
initiated, assuming that such navigations are always user activated.

However, history navigations are a special case. They're intentionally
considered to be browser initiated, even if they originate from renderer
script (e.g. `history.back()`). This meant that our check above would
allow script to use the history API to activate a text fragment without
a user gesture.

This CL explicitly forbids activating a text fragment if the navigation
is of history type. This is a trivial change (in terms of UX) because a
history navigation will restore the scroll position to where the user
left off so the text fragment scroll is already clobbered. This change
prevents a transient scroll that will be undone.

Note: we had an explicit test for this case that failed to catch the
failure. The reason was that the test was checking that the fragment
wasn't activated by checking that the scroll offset after a navigation
is 0. However, the text fragment's scroll would be clobbered (assuming
by history scroll restoration) so this check would erroneously pass. We
fix it in this CL by using a scroll listener so that we can tell a
scroll occurred even if it is later restored.

Bug: 1042986
Change-Id: Ia0ad9a8adcda2250603e6a7dd2b386193be2a6e6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2135407Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Nick Burris <nburris@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758413}

content/browser/frame_host/navigation_request.h

@@ -903,7 +903,9 @@ class CONTENT_EXPORT NavigationRequest
   // be set in CreatedNavigationRequest.
   // Note: |browser_initiated_| and |common_params_| may be mutated by
   // ContentBrowserClient::OverrideNavigationParams at StartNavigation time
-  // (i.e. before we actually kick off the navigation).
+  // (i.e. before we actually kick off the navigation). |browser_initiated|
+  // will always be true for history navigations, even if they began in the
+  // renderer using the history API.
   mojom::CommonNavigationParamsPtr common_params_;
   mojom::BeginNavigationParamsPtr begin_params_;
   mojom::CommitNavigationParamsPtr commit_params_;

content/browser/navigation_browsertest.cc

@@ -2023,7 +2023,8 @@ IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest, EnabledOnUserNavigation) {
   WaitForPageLoad(main_contents);
   frame_observer.WaitForScrollOffsetAtTop(
       /*expected_scroll_offset_at_top=*/false);
-  EXPECT_FALSE(main_contents->GetMainFrame()->GetView()->IsScrollOffsetAtTop());
+  RunUntilInputProcessed(GetWidgetHost());
+  EXPECT_EQ(true, EvalJs(main_contents, "did_scroll;"));
 }
 
 IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest,
@@ -2039,7 +2040,8 @@ IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest,
   WaitForPageLoad(main_contents);
   frame_observer.WaitForScrollOffsetAtTop(
       /*expected_scroll_offset_at_top=*/false);
-  EXPECT_FALSE(main_contents->GetMainFrame()->GetView()->IsScrollOffsetAtTop());
+  RunUntilInputProcessed(GetWidgetHost());
+  EXPECT_EQ(true, EvalJs(main_contents, "did_scroll;"));
 }
 
 IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest,
@@ -2064,7 +2066,8 @@ IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest,
   WaitForPageLoad(main_contents);
   frame_observer.WaitForScrollOffsetAtTop(
       /*expected_scroll_offset_at_top=*/false);
-  EXPECT_FALSE(main_contents->GetMainFrame()->GetView()->IsScrollOffsetAtTop());
+  RunUntilInputProcessed(GetWidgetHost());
+  EXPECT_EQ(true, EvalJs(main_contents, "did_scroll;"));
 }
 
 IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest,
@@ -2091,7 +2094,7 @@ IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest,
       FROM_HERE, run_loop.QuitClosure(), TestTimeouts::tiny_timeout());
   run_loop.Run();
   RunUntilInputProcessed(GetWidgetHost());
-  EXPECT_TRUE(main_contents->GetMainFrame()->GetView()->IsScrollOffsetAtTop());
+  EXPECT_EQ(false, EvalJs(main_contents, "did_scroll;"));
 }
 
 IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest,
@@ -2127,7 +2130,13 @@ IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest,
       FROM_HERE, run_loop.QuitClosure(), TestTimeouts::tiny_timeout());
   run_loop.Run();
   RunUntilInputProcessed(GetWidgetHost());
-  EXPECT_TRUE(main_contents->GetMainFrame()->GetView()->IsScrollOffsetAtTop());
+
+  // Note: we use a scroll handler in the page to check whether any scrolls
+  // happened at all, rather than checking the current scroll offset. This is
+  // to ensure that if the offset is reset back to the top for other reasons
+  // (e.g. history restoration) we still fail this test. See
+  // https://crbug.com/1042986 for why this matters.
+  EXPECT_EQ(false, EvalJs(main_contents, "did_scroll;"));
 }
 
 IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest,
@@ -2142,11 +2151,13 @@ IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest,
 
   WaitForPageLoad(main_contents);
   frame_observer.WaitForScrollOffsetAtTop(false);
-  EXPECT_FALSE(main_contents->GetMainFrame()->GetView()->IsScrollOffsetAtTop());
 
-  // Scroll the page back to top.
+  // Scroll the page back to top. Make sure we reset the |did_scroll| variable
+  // we'll use below to ensure the same-document navigation invokes the text
+  // fragment.
   EXPECT_TRUE(ExecuteScript(main_contents, "window.scrollTo(0, 0)"));
   frame_observer.WaitForScrollOffsetAtTop(true);
+  EXPECT_TRUE(ExecJs(main_contents, "did_scroll = false;"));
 
   // Perform a same-document browser initiated navigation
   GURL same_doc_url(embedded_test_server()->GetURL(
@@ -2156,7 +2167,8 @@ IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest,
   WaitForPageLoad(main_contents);
   frame_observer.WaitForScrollOffsetAtTop(
       /*expected_scroll_offset_at_top=*/false);
-  EXPECT_FALSE(main_contents->GetMainFrame()->GetView()->IsScrollOffsetAtTop());
+  RunUntilInputProcessed(GetWidgetHost());
+  EXPECT_EQ(true, EvalJs(main_contents, "did_scroll;"));
 }
 
 IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest,
@@ -2184,7 +2196,7 @@ IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest,
       FROM_HERE, run_loop.QuitClosure(), TestTimeouts::tiny_timeout());
   run_loop.Run();
   RunUntilInputProcessed(GetWidgetHost());
-  EXPECT_TRUE(main_contents->GetMainFrame()->GetView()->IsScrollOffsetAtTop());
+  EXPECT_EQ(false, EvalJs(main_contents, "did_scroll;"));
 }
 
 IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest, EnabledByDocumentPolicy) {
@@ -2211,6 +2223,12 @@ IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest, EnabledByDocumentPolicy) {
       "Content-Type: text/html; charset=utf-8\r\n"
       "Document-Policy: no-force-load-at-top\r\n"
       "\r\n"
+      "<script>"
+      "  let did_scroll = false;"
+      "  window.addEventListener('scroll', () => {"
+      "    did_scroll = true;"
+      "  });"
+      "</script>"
       "<p style='position: absolute; top: 10000px;'>Some text</p>");
   response.Done();
 
@@ -2221,7 +2239,8 @@ IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest, EnabledByDocumentPolicy) {
   WaitForPageLoad(main_contents);
   frame_observer.WaitForScrollOffsetAtTop(
       /*expected_scroll_offset_at_top=*/false);
-  EXPECT_FALSE(main_contents->GetMainFrame()->GetView()->IsScrollOffsetAtTop());
+  RunUntilInputProcessed(GetWidgetHost());
+  EXPECT_EQ(true, EvalJs(main_contents, "did_scroll;"));
 }
 
 IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest,
@@ -2248,6 +2267,12 @@ IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest,
       "Content-Type: text/html; charset=utf-8\r\n"
       "Document-Policy: force-load-at-top\r\n"
       "\r\n"
+      "<script>"
+      "  let did_scroll = false;"
+      "  window.addEventListener('scroll', () => {"
+      "    did_scroll = true;"
+      "  });"
+      "</script>"
       "<p style='position: absolute; top: 10000px;'>Some text</p>");
   response.Done();
 
@@ -2262,7 +2287,7 @@ IN_PROC_BROWSER_TEST_F(TextFragmentAnchorBrowserTest,
       FROM_HERE, run_loop.QuitClosure(), TestTimeouts::tiny_timeout());
   run_loop.Run();
   RunUntilInputProcessed(GetWidgetHost());
-  EXPECT_TRUE(main_contents->GetMainFrame()->GetView()->IsScrollOffsetAtTop());
+  EXPECT_EQ(false, EvalJs(main_contents, "did_scroll;"));
 }
 
 // Regression test for https://crbug.com/996044

content/test/data/scrollable_page_with_content.html

 <html>
   <head>
     <meta name="viewport" content="width=device-width, minimum-scale=1.0">
+    <script>
+      let did_scroll = false;
+      window.addEventListener('scroll', () => {
+        did_scroll = true;
+      });
+    </script>
     <style>
       p {
         position: absolute;

content/test/data/target_text_link.html

@@ -5,4 +5,4 @@
   <body>
     <a id="link" href="/scrollable_page_with_content.html#:~:text=text">link</a>
   </body>
-</html>>
+</html>

third_party/blink/renderer/core/loader/document_loader.h

@@ -294,6 +294,9 @@ class CORE_EXPORT DocumentLoader : public GarbageCollected<DocumentLoader>,
 
   bool HadTransientActivation() const { return had_transient_activation_; }
 
+  // Whether the navigation originated from the browser process. Note: history
+  // navigation is always considered to be browser initiated, even if the
+  // navigation was started using the history API in the renderer.
   bool IsBrowserInitiated() const { return is_browser_initiated_; }
 
   bool IsSameOriginNavigation() const { return is_same_origin_navigation_; }

third_party/blink/renderer/core/page/scrolling/text_fragment_anchor.cc

@@ -61,12 +61,25 @@ bool ParseTextDirective(const String& fragment,
 bool CheckSecurityRestrictions(LocalFrame& frame,
                                bool same_document_navigation) {
   // This algorithm checks the security restrictions detailed in
-  // https://wicg.github.io/ScrollToTextFragment/#should-allow-text-fragment
-
-  // We only allow text fragment anchors for user or browser initiated
-  // navigations, i.e. no script navigations.
-  if (!(frame.Loader().GetDocumentLoader()->HadTransientActivation() ||
-        frame.Loader().GetDocumentLoader()->IsBrowserInitiated())) {
+  // https://wicg.github.io/ScrollToTextFragment/#should-allow-a-text-fragment
+  // TODO(bokan): These are really only relevant for observable actions like
+  // scrolling. We should consider allowing highlighting regardless of these
+  // conditions. See the TODO in the relevant spec section:
+  // https://wicg.github.io/ScrollToTextFragment/#restricting-the-text-fragment
+
+  // History navigation is special because it's considered to be browser
+  // initiated even if the navigation originated via use of the history API
+  // within the renderer. We avoid creating a text fragment for history
+  // navigations since history scroll restoration should take precedence but
+  // it'd be bad if we ever got here for a history navigation since the check
+  // below would pass even if the user took no action.
+  SECURITY_CHECK(frame.Loader().GetDocumentLoader()->GetNavigationType() !=
+                 kWebNavigationTypeBackForward);
+
+  // We only allow text fragment anchors for user navigations, e.g. link
+  // clicks, omnibox navigations, no script navigations.
+  if (!frame.Loader().GetDocumentLoader()->HadTransientActivation() &&
+      !frame.Loader().GetDocumentLoader()->IsBrowserInitiated()) {
     return false;
   }
 
@@ -104,6 +117,17 @@ TextFragmentAnchor* TextFragmentAnchor::TryCreateFragmentDirective(
   if (!frame.GetDocument()->GetFragmentDirective())
     return nullptr;
 
+  // Avoid invoking the text fragment for history or reload navigations as
+  // they'll be clobbered by scroll restoration; this prevents a transient
+  // scroll as well as user gesture issues; see https://crbug.com/1042986 for
+  // details.
+  auto navigation_type =
+      frame.Loader().GetDocumentLoader()->GetNavigationType();
+  if (navigation_type == kWebNavigationTypeBackForward ||
+      navigation_type == kWebNavigationTypeReload) {
+    return nullptr;
+  }
+
   if (!CheckSecurityRestrictions(frame, same_document_navigation))
     return nullptr;
 

third_party/blink/renderer/core/page/scrolling/text_fragment_anchor_test.cc

@@ -1611,7 +1611,11 @@ TEST_F(TextFragmentAnchorTest, TextDirectiveInSvg) {
 }
 
 // Ensure we restore the text highlight on page reload
-TEST_F(TextFragmentAnchorTest, HighlightOnReload) {
+// TODO(bokan): This test is disabled as this functionality was suppressed in
+// https://crrev.com/c/2135407; it would be better addressed by providing a
+// highlight-only function. See the TODO in
+// https://wicg.github.io/ScrollToTextFragment/#restricting-the-text-fragment
+TEST_F(TextFragmentAnchorTest, DISABLED_HighlightOnReload) {
   SimRequest request("https://example.com/test.html#:~:text=test", "text/html");
   LoadURL("https://example.com/test.html#:~:text=test");
   const String& html = R"HTML(