Disallow guest main frame navigations in PDF viewer extension.

Cross-origin navigations inside the guest's main frame of the
PDF extension can cause the IsSameOrigin CHECK in
MimeHandlerViewGuest::DidFinishNavigation to fail. They should be
disallowed.

Bug: 394513280
Change-Id: I9489facab8f35396419acca19d14a385ae02fafb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6354237
Reviewed-by: Andy Phan <andyphan@chromium.org>
Reviewed-by: Kevin McNee <mcnee@chromium.org>
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1434896}

chrome/browser/pdf/chrome_pdf_stream_delegate.cc

@@ -226,6 +226,32 @@ void ChromePdfStreamDelegate::OnPdfEmbedderSandboxed(
   pdf_viewer_stream_manager->DeleteUnclaimedStreamInfo(frame_tree_node_id);
 }
 
+bool ChromePdfStreamDelegate::ShouldAllowPdfExtensionFrameNavigation(
+    content::NavigationHandle* navigation_handle) {
+  // If PdfOopif is enabled, or if this is an about:blank navigation, allow it.
+  if (chrome_pdf::features::IsOopifPdfEnabled() ||
+      navigation_handle->GetURL().IsAboutBlank()) {
+    return true;
+  }
+
+  // Verify this is a guest, otherwise allow the navigation to proceed.
+  auto* guest =
+      extensions::MimeHandlerViewGuest::FromNavigationHandle(navigation_handle);
+  if (!guest) {
+    return true;
+  }
+
+  // Since this is the PDF delegate, don't suppress navigations for other stream
+  // types (should they exist).
+  base::WeakPtr<extensions::StreamContainer> stream = guest->GetStreamWeakPtr();
+  if (!stream || stream->extension_id() != extension_misc::kPdfExtensionId) {
+    return true;
+  }
+
+  return url::IsSameOriginWith(stream->handler_url(),
+                               navigation_handle->GetURL());
+}
+
 bool ChromePdfStreamDelegate::ShouldAllowPdfFrameNavigation(
     content::NavigationHandle* navigation_handle) {
   // Blocks any non-setup navigations in the PDF extension frame and the PDF

chrome/browser/pdf/chrome_pdf_stream_delegate.h

@@ -24,6 +24,8 @@ class ChromePdfStreamDelegate : public pdf::PdfStreamDelegate {
       content::FrameTreeNodeId frame_tree_node_id) override;
   bool ShouldAllowPdfFrameNavigation(
       content::NavigationHandle* navigation_handle) override;
+  bool ShouldAllowPdfExtensionFrameNavigation(
+      content::NavigationHandle* navigation_handle) override;
 };
 
 #endif  // CHROME_BROWSER_PDF_CHROME_PDF_STREAM_DELEGATE_H_

chrome/browser/pdf/pdf_extension_test.cc

@@ -259,6 +259,32 @@ IN_PROC_BROWSER_TEST_P(PDFExtensionTest, PdfExtensionLoaded) {
   }
 }
 
+// The pdf extension's frame should not allow cross-origin navigations. They
+// should be blocked, but not crash.
+// https://crbug.com/394513280
+IN_PROC_BROWSER_TEST_P(PDFExtensionTest,
+                       NoCrossOriginNavigationFromPdfExtensionFrame) {
+  if (UseOopif()) {
+    // This test only applies to MimeHandlerViewGuest PDF.
+    GTEST_SKIP();
+  }
+
+  const GURL main_url(embedded_test_server()->GetURL("/pdf/test.pdf"));
+  content::RenderFrameHost* extension_host = LoadPdfGetExtensionHost(main_url);
+  ASSERT_TRUE(extension_host);
+
+  content::TestFrameNavigationObserver frame_nav_observer(extension_host);
+  ASSERT_TRUE(content::ExecJs(extension_host,
+                              "window.location = 'https://example.com';"));
+  frame_nav_observer.Wait();
+
+  EXPECT_EQ(net::ERR_BLOCKED_BY_CLIENT,
+            frame_nav_observer.last_net_error_code());
+  const GURL extension_url(
+      "chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/index.html");
+  EXPECT_EQ(extension_url, extension_host->GetLastCommittedURL());
+}
+
 // Helper class to allow pausing the asynchronous attachment of an inner
 // WebContents between MimeHandlerViewAttachHelper's AttachToOuterWebContents()
 // and ResumeAttachOrDestroy().  This corresponds to the point where the inner

components/pdf/browser/fake_pdf_stream_delegate.cc

@@ -53,4 +53,9 @@ bool FakePdfStreamDelegate::ShouldAllowPdfFrameNavigation(
   return should_allow_pdf_frame_navigation_;
 }
 
+bool FakePdfStreamDelegate::ShouldAllowPdfExtensionFrameNavigation(
+    content::NavigationHandle* navigation_handle) {
+  return should_allow_pdf_extension_frame_navigation_;
+}
+
 }  // namespace pdf

components/pdf/browser/fake_pdf_stream_delegate.h

@@ -31,6 +31,8 @@ class FakePdfStreamDelegate : public PdfStreamDelegate {
       content::FrameTreeNodeId frame_tree_node_id) override;
   bool ShouldAllowPdfFrameNavigation(
       content::NavigationHandle* navigation_handle) override;
+  bool ShouldAllowPdfExtensionFrameNavigation(
+      content::NavigationHandle* navigation_handle) override;
 
   void clear_stream_info() { stream_info_.reset(); }
 
@@ -38,8 +40,13 @@ class FakePdfStreamDelegate : public PdfStreamDelegate {
     should_allow_pdf_frame_navigation_ = should_allow;
   }
 
+  void set_should_allow_pdf_extension_frame_navigation(bool should_allow) {
+    should_allow_pdf_extension_frame_navigation_ = should_allow;
+  }
+
  private:
   bool should_allow_pdf_frame_navigation_ = true;
+  bool should_allow_pdf_extension_frame_navigation_ = true;
   std::optional<StreamInfo> stream_info_;
 };
 

components/pdf/browser/pdf_navigation_throttle.cc

@@ -79,10 +79,13 @@ PdfNavigationThrottle::WillStartRequest() {
   // Intercepts navigations to a PDF stream URL in a PDF content frame and
   // re-navigates to the original PDF URL.
 
-  // Skip main frame navigations, as the main frame should never be navigating
+  // The main frame may contain the PDF extension for non-PdfOopif cases; it
-  // to the stream URL.
+  // should never be navigated away from the extension.
   if (navigation_handle()->IsInMainFrame()) {
-    return PROCEED;
+    return stream_delegate_->ShouldAllowPdfExtensionFrameNavigation(
+               navigation_handle())
+               ? PROCEED
+               : BLOCK_REQUEST;
   }
 
   // Skip unless navigating to the stream URL.

components/pdf/browser/pdf_navigation_throttle_unittest.cc

@@ -142,6 +142,15 @@ TEST_F(PdfNavigationThrottleTest,
             navigation_throttle->WillStartRequest().action());
 }
 
+TEST_F(PdfNavigationThrottleTest,
+       WillStartRequestShouldAllowPdfExtensionFrameNavigationFalse) {
+  stream_delegate_->clear_stream_info();
+  stream_delegate_->set_should_allow_pdf_extension_frame_navigation(false);
+  auto navigation_throttle = CreateNavigationThrottle(stream_url(), main_rfh());
+  EXPECT_EQ(content::NavigationThrottle::BLOCK_REQUEST,
+            navigation_throttle->WillStartRequest().action());
+}
+
 TEST_F(PdfNavigationThrottleTest, WillStartRequestOtherUrl) {
   auto navigation_throttle = CreateNavigationThrottle(
       GURL("https://example.test"), CreateChildFrame());

components/pdf/browser/pdf_stream_delegate.h

@@ -74,6 +74,12 @@ class PdfStreamDelegate {
   // if they are not related to PDF viewer setup.
   virtual bool ShouldAllowPdfFrameNavigation(
       content::NavigationHandle* navigation_handle) = 0;
+
+  // Determines whether navigation attempts in the PDF extension frame should be
+  // allowed. If MimeHandlerGuestView is in use, don't allow navigations away
+  // from the extension's origin in the guest's main frame.
+  virtual bool ShouldAllowPdfExtensionFrameNavigation(
+      content::NavigationHandle* navigation_handle) = 0;
 };
 
 }  // namespace pdf

extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc

@@ -542,7 +542,8 @@ void MimeHandlerViewGuest::DidFinishNavigation(
   guest_view::GuestView<MimeHandlerViewGuest>::DidFinishNavigation(
       navigation_handle);
 
-  if (!IsObservedNavigationWithinGuest(navigation_handle)) {
+  if (!IsObservedNavigationWithinGuest(navigation_handle) ||
+      !navigation_handle->HasCommitted()) {
     return;
   }