Block webpages from navigating to view-source URLs

content/browser/web_contents/web_contents_impl_unittest.cc

 // Copyright (c) 2012 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 <stdint.h>
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/macros.h"
@@ skipping 387 matching lines @@
   controller().LoadURL(
       kGURL, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
 
   NavigationEntry* entry = controller().GetVisibleEntry();
   ASSERT_EQ(kGURL, entry->GetURL());
   entry->SetTitle(title);
 
   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";
   const GURL kGURL(kUrl);
 
   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));
 
   FrameHostMsg_DidCommitProvisionalLoad_Params params;
   InitNavigateParams(&params, 0, entry_id, true, kGURL,
                      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.
 TEST_F(WebContentsImplTest, UpdateMaxPageID) {
   SiteInstance* instance1 = contents()->GetSiteInstance();
   scoped_refptr<SiteInstance> instance2(SiteInstance::Create(nullptr));
 
   // Starts at -1.
   EXPECT_EQ(-1, contents()->GetMaxPageID());
   EXPECT_EQ(-1, contents()->GetMaxPageIDForSiteInstance(instance1));
@@ skipping 3012 matching lines @@
   // An automatic navigation.
   contents()->GetMainFrame()->SendNavigateWithModificationCallback(
       2, 0, true, GURL(url::kAboutBlankURL), base::Bind(SetAsNonUserGesture));
 
   EXPECT_EQ(1u, dialog_manager.reset_count());
 
   contents()->SetJavaScriptDialogManagerForTesting(nullptr);
 }
 
 }  // namespace content