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

chrome/browser/pdf/pdf_extension_test.cc

Index: chrome/browser/pdf/pdf_extension_test.cc
diff --git a/chrome/browser/pdf/pdf_extension_test.cc b/chrome/browser/pdf/pdf_extension_test.cc
index 3ddc059fda093f979b65af8ef5980e6e922f2263..14b6033ff44d847a26d7bd30b268fb926e21f697 100644
--- a/chrome/browser/pdf/pdf_extension_test.cc
+++ b/chrome/browser/pdf/pdf_extension_test.cc
@@ -32,6 +32,7 @@
 #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"
@@ -99,14 +100,7 @@ class PDFExtensionTest : public ExtensionApiTest,
     // 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;
@@ -139,6 +133,21 @@ class PDFExtensionTest : public ExtensionApiTest,
     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).
@@ -377,6 +386,27 @@ IN_PROC_BROWSER_TEST_F(PDFExtensionTest, EnsureSameOriginRepliesAllowed) {
                            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);