Fix for "chrome://" links in PDFs.

chrome/browser/pdf/pdf_extension_test.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <vector>
 
 #include "base/base_paths.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/hash.h"
@@ skipping 14 matching lines @@
 #include "chrome/common/chrome_content_client.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/browser_plugin_guest_manager.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_manager.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/plugin_service.h"
+#include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/manifest_handlers/mime_types_handler.h"
 #include "extensions/test/result_catcher.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "url/gurl.h"
 
 const int kNumberLoadTestParts = 10;
@@ skipping 47 matching lines @@
   void RunTestsInFile(std::string filename, std::string pdf_filename) {
     extensions::ResultCatcher catcher;
 
     GURL url(embedded_test_server()->GetURL("/pdf/" + pdf_filename));
 
     // It should be good enough to just navigate to the URL. But loading up the
     // BrowserPluginGuest seems to happen asynchronously as there was flakiness
     // being seen due to the BrowserPluginGuest not being available yet (see
     // crbug.com/498077). So instead use |LoadPdf| which ensures that the PDF is
     // loaded before continuing.
-    ASSERT_TRUE(LoadPdf(url));
-
-    content::WebContents* contents =
-        browser()->tab_strip_model()->GetActiveWebContents();
-    content::BrowserPluginGuestManager* guest_manager =
-        contents->GetBrowserContext()->GetGuestManager();
-    content::WebContents* guest_contents =
-        guest_manager->GetFullPageGuest(contents);
+    content::WebContents* guest_contents = LoadPdfGetGuestContents(url);
     ASSERT_TRUE(guest_contents);
 
     base::FilePath test_data_dir;
     PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);
     test_data_dir = test_data_dir.Append(FILE_PATH_LITERAL("pdf"));
     base::FilePath test_util_path = test_data_dir.AppendASCII("test_util.js");
     std::string test_util_js;
     ASSERT_TRUE(base::ReadFileToString(test_util_path, &test_util_js));
 
     base::FilePath test_file_path = test_data_dir.AppendASCII(filename);
@@ skipping 12 matching lines @@
   // fails. If it doesn't finish loading the test will hang. This is done from
   // outside of the BrowserPlugin guest to ensure the PDFScriptingAPI works
   // correctly from there.
   bool LoadPdf(const GURL& url) {
     ui_test_utils::NavigateToURL(browser(), url);
     content::WebContents* web_contents =
         browser()->tab_strip_model()->GetActiveWebContents();
     return pdf_extension_test_util::EnsurePDFHasLoaded(web_contents);
   }
 
+  // Same as |LoadPdf|, but also returns a pointer to the guest WebContents for
+  // the loaded PDF. Returns nullptr if the load fails.
+  content::WebContents* LoadPdfGetGuestContents(const GURL& url) {
+    if (!LoadPdf(url))
+      return nullptr;
+
+    content::WebContents* contents =
+        browser()->tab_strip_model()->GetActiveWebContents();
+    content::BrowserPluginGuestManager* guest_manager =
+        contents->GetBrowserContext()->GetGuestManager();
+    content::WebContents* guest_contents =
+        guest_manager->GetFullPageGuest(contents);
+    return guest_contents;
+  }
+
   // Load all the PDFs contained in chrome/test/data/<dir_name>. This only runs
   // the test if base::Hash(filename) mod kNumberLoadTestParts == k in order
   // to shard the files evenly across values of k in [0, kNumberLoadTestParts).
   void LoadAllPdfsTest(const std::string& dir_name, int k) {
     base::FilePath test_data_dir;
     ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir));
     base::FileEnumerator file_enumerator(test_data_dir.AppendASCII(dir_name),
                                          false, base::FileEnumerator::FILES,
                                          FILE_PATH_LITERAL("*.pdf"));
 
@@ skipping 218 matching lines @@
       "</body></html>";
   TestGetSelectedTextReply(GURL(data_url), false);
 }
 
 // Ensure same-origin replies do work for getSelectedText.
 IN_PROC_BROWSER_TEST_F(PDFExtensionTest, EnsureSameOriginRepliesAllowed) {
   TestGetSelectedTextReply(embedded_test_server()->GetURL("/pdf/test.pdf"),
                            true);
 }
 
+// This test ensures that link permissions are enforced properly in PDFs.
+IN_PROC_BROWSER_TEST_F(PDFExtensionTest, LinkPermissions) {
+  GURL test_pdf_url(embedded_test_server()->GetURL("/pdf/test.pdf"));
+  content::WebContents* guest_contents = LoadPdfGetGuestContents(test_pdf_url);
+  ASSERT_TRUE(guest_contents);
+
+  // chrome://favicon links should be allowed for PDFs, while chrome://settings
+  // links should not.
+  GURL valid_link_url("chrome://favicon/https://www.google.ca/");
+  GURL invalid_link_url("chrome://settings");
+
+  GURL unfiltered_valid_link_url(valid_link_url);
+  content::RenderProcessHost* rph = guest_contents->GetRenderProcessHost();
+  rph->FilterURL(true, &valid_link_url);
+  rph->FilterURL(true, &invalid_link_url);

[Charlie Reis, 2015/09/29 22:15:53]

Hmm, is there no way to attempt to request the resource instead?  This is better
than nothing, but it doesn't guarantee that we're actually calling FilterURL in
the cases that matter.

[paulmeyer, 2015/09/30 14:21:31]

The issue behind this bug is that context menus allowed link URLs to go through
that shouldn't. This only happened because of a problem within FilterURL, so I
am testing that.

I originally created a PDF with these links on it, and was going to open context
menus on them both and check the links that the menus picked up, but it turned
out that writing the test that way was really finicky, and the context menus
often didn't act as if they'd been opened on links even if they were opened in
exactly the right spot. I feel like a test like that would also be really
fragile in the future, because it relies upon the links being at set coordinates
that could change easily if the layout of the pdf viewer ever changed.

I think this test should be sufficient for testing against the regression of
this particular bug, and is also very reliable.

[Charlie Reis, 2015/09/30 20:39:17]

Acknowledged.

+
+  // Invalid link URLs should be changed to "about:blank" when filtered.
+  ASSERT_EQ(valid_link_url, unfiltered_valid_link_url);

[Charlie Reis, 2015/09/29 22:15:53]

nit: Use EXPECT_EQ for test expectations (so that the test keeps running) and
ASSERT_EQ for mandatory preconditions (if the test can't proceed sensibly).

[paulmeyer, 2015/09/30 14:21:31]

Done.

+  ASSERT_EQ(invalid_link_url, GURL("about:blank"));

[Charlie Reis, 2015/09/29 22:15:53]

nit: The arguments should be (expected, actual), so flip the order.

[paulmeyer, 2015/09/30 14:21:31]

Done.

+}
+
 class MaterialPDFExtensionTest : public PDFExtensionTest {
   void SetUpCommandLine(base::CommandLine* command_line) override {
     command_line->AppendSwitch(switches::kEnablePdfMaterialUI);
   }
 };
 
 IN_PROC_BROWSER_TEST_F(MaterialPDFExtensionTest, Basic) {
   RunTestsInFile("basic_test_material.js", "test.pdf");
 }
 
@@ skipping 31 matching lines @@
   RunTestsInFile("toolbar_manager_test.js", "test.pdf");
 }
 
 IN_PROC_BROWSER_TEST_F(MaterialPDFExtensionTest, Title) {
   RunTestsInFile("title_test.js", "test-title.pdf");
 }
 
 IN_PROC_BROWSER_TEST_F(MaterialPDFExtensionTest, WhitespaceTitle) {
   RunTestsInFile("whitespace_title_test.js", "test-whitespace-title.pdf");
 }