Block webpages from navigating to view-source URLs

content/browser/web_contents/web_contents_impl_unittest.cc

Index: content/browser/web_contents/web_contents_impl_unittest.cc
diff --git a/content/browser/web_contents/web_contents_impl_unittest.cc b/content/browser/web_contents/web_contents_impl_unittest.cc
index e6f350002cd63cb14e5454802dedf05dd167cf2b..48cf512a05d84a6925d09c67c91b06f8b3e61689 100644
--- a/content/browser/web_contents/web_contents_impl_unittest.cc
+++ b/content/browser/web_contents/web_contents_impl_unittest.cc
@@ -405,8 +405,8 @@ TEST_F(WebContentsImplTest, UseTitleFromPendingEntryIfSet) {
   EXPECT_EQ(title, contents()->GetTitle());
 }
 
-// Test view source mode for a webui page.
-TEST_F(WebContentsImplTest, NTPViewSource) {
+// A page shouldn't be able to open view-source for a webui page.
+TEST_F(WebContentsImplTest, ContentInitiatedViewSource) {
   NavigationControllerImpl& cont =
       static_cast<NavigationControllerImpl&>(controller());
   const char kUrl[] = "view-source:chrome://blah";
@@ -414,10 +414,11 @@ TEST_F(WebContentsImplTest, NTPViewSource) {
 
   process()->sink().ClearMessages();
 
-  cont.LoadURL(
-      kGURL, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
+  cont.LoadURL(kGURL, Referrer(), ui::PAGE_TRANSITION_LINK, std::string());

[Charlie Reis, 2016/05/16 22:22:18]

The transition type doesn't make this a renderer-initiated navigation. 
Generally all calls to LoadURL are considered browser-initiated, though you
might be able to fake it by calling LoadURLWithParams and setting
is_renderer_initiated to true on the LoadURLParams.

This is probably why the EnableViewSourceMode message is still received below.

[meacer, 2016/05/19 01:13:54]

This test is problematic in that it uses LoadURL instead of LoadURLWithParams.
NavigationControllerImpl::CreateNavigationEntry rewrites
view-source:http://example.com to http://example.com, but it's not called by
LoadURL, so the loaded URL ends up getting filtered.

To fix, I've changed it to use LoadURLWithParams, and am now manually setting
NavigateParams to use chrome://blah instead of view-source:chrome://blah. But
that kind of defeats the purpose of the test. I'm inclined to delete it and make
it a full browser test instead, if you agree.

[Charlie Reis, 2016/05/23 22:32:16]

I definitely think it's worth testing both the browser-initiated and
renderer-initiated cases as browser tests, since those are much more
representative of what actually happens.

I'm ok with either keeping or deleting this unit test once those exist.

[meacer, 2016/05/23 23:57:47]

Added multiple browser tests and a layout test.

   int entry_id = cont.GetPendingEntry()->GetUniqueID();
   // Did we get the expected message?
+  // TODO(meacer): This probably shouldn't be true if we can't navigate to the
+  //               view-source URL.

[Charlie Reis, 2016/05/16 22:22:18]

Yeah, this would be concerning if it happened for renderer-initiated
navigations.  We expect it for browser-initiated ones, though.

[meacer, 2016/05/19 01:13:54]

Now that the test is testing a browser initiated navigation, removed the TODO.

[Charlie Reis, 2016/05/23 22:32:16]

I'm confused-- I thought you were making this a test of renderer-initiated
navigations.

[meacer, 2016/05/23 23:57:47]

Sorry for the confusion, I decided to only check browser initiated navigations
here. Though as I said earlier, I think browser tests cover both browser and
content initiated navigations well enough that we can now remove this test
altogether.

   EXPECT_TRUE(process()->sink().GetFirstMessageMatching(
       FrameMsg_EnableViewSourceMode::ID));
 
@@ -426,8 +427,8 @@ TEST_F(WebContentsImplTest, NTPViewSource) {
                      ui::PAGE_TRANSITION_TYPED);
   contents()->GetMainFrame()->PrepareForCommit();
   contents()->GetMainFrame()->SendNavigateWithParams(&params);
-  // Also check title and url.
-  EXPECT_EQ(base::ASCIIToUTF16(kUrl), contents()->GetTitle());
+  // Child processes shouldn't be able to load view-source URLs.
+  EXPECT_EQ(base::ASCIIToUTF16("about:blank"), contents()->GetTitle());

[Charlie Reis, 2016/05/16 22:22:18]

I'm curious what happened here.  Why did it get rewritten to about:blank?  That
shouldn't happen for browser-initiated navigations.

[meacer, 2016/05/19 01:13:54]

As I mentioned in the comment above, view-source:chrome://blah URL wasn't
properly being rewritten to chrome://blah, and was getting filtered later on.

 }
 
 // Test to ensure UpdateMaxPageID is working properly.