|
|
Created:
4 years, 7 months ago by meacer Modified:
4 years, 6 months ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org, Charlie Reis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBlock webpages from navigating to view-source URLs.
This CL blocks navigations to view-source URLs initiated by a page via window.location, window.open, A tag, etc. It still allows user initiated navigations such as directly entering the URL, clicking "view page source" or "open in new tab" in the context menu.
BUG=247151, 606619
Committed: https://crrev.com/ce6b6603637ee045041ccb49359fbae617d84ba5
Cr-Commit-Position: refs/heads/master@{#397505}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Fix ChildProcessSecurityPolicy and tests #
Total comments: 12
Patch Set 3 : Fix unit test #
Total comments: 2
Patch Set 4 : Add layout and browser tests #
Total comments: 27
Patch Set 5 : creis comments #Patch Set 6 : Minor comment fix #Patch Set 7 : Delete layout tests #Messages
Total messages: 29 (11 generated)
Description was changed from ========== Block webpages from navigating to view-source URLs BUG=606619 ========== to ========== Block webpages from navigating to view-source URLs BUG=606619 ==========
Description was changed from ========== Block webpages from navigating to view-source URLs BUG=606619 ========== to ========== Block webpages from navigating to view-source URLs. This CL blocks navigations to view-source URLs initiated by a page via window.location, window.open, A tag, etc. It still allows user initiated navigations such as directly entering the URL, clicking "view page source" or "open in new tab" in the context menu. BUG=606619 ==========
meacer@chromium.org changed reviewers: + creis@chromium.org
Charlie, can you PTAL? Thanks.
Description was changed from ========== Block webpages from navigating to view-source URLs. This CL blocks navigations to view-source URLs initiated by a page via window.location, window.open, A tag, etc. It still allows user initiated navigations such as directly entering the URL, clicking "view page source" or "open in new tab" in the context menu. BUG=606619 ========== to ========== Block webpages from navigating to view-source URLs. This CL blocks navigations to view-source URLs initiated by a page via window.location, window.open, A tag, etc. It still allows user initiated navigations such as directly entering the URL, clicking "view page source" or "open in new tab" in the context menu. BUG=247151,606619 ==========
Thanks for putting something together. Since this is a web-visible change, I think we should put up an Intent on blink-dev about it. (Shouldn't be a problem, since we're apparently becoming more consistent with other browsers by doing this.) We'll definitely want tests for this-- it looks like Firefox had some issues with their fix in https://bugzilla.mozilla.org/show_bug.cgi?id=1172165. I'd recommend getting some tests early, so that we can be confident in whatever approach we choose. The current fix doesn't seem sufficient, unless there's some additional uses of that display-isolated registry I'm missing. The use in SecurityOrigin is in the renderer process, so it could be bypassed by a renderer exploit. (I'm also not sure if it will prevent linking to view-source or not.) I think there are other ways to prevent links to certain schemes, like not registering them as web safe schemes. This one might be different because it's a pseudo-scheme, though. Maybe we need to update ChildProcessSecurityPolicyImpl::CanRequestURL? There's a few references to kViewSourceScheme in that class, so we should look closely at them. I'm not sure if we can prevent the process from asking for the URL, though, since the view-source page might end up in a web page after a browser-initiated navigation. https://codereview.chromium.org/1917073002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1917073002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1331: WebSecurityPolicy::registerURLSchemeAsDisplayIsolated(view_source_scheme); This seems to equate to SecurityOrigin::canDisplay, which has the following comment: // Returns true if |document| can display content from the given URL (e.g., // in an iframe or as an image). For example, web sites generally cannot // display content from the user's files system. Does it also prevent linking or navigating to such content? I don't see the code for that.
Thanks Charlie, modified some tests but will look into adding more. > The current fix doesn't seem sufficient, unless there's some additional uses of > that display-isolated registry I'm missing. The use in SecurityOrigin is in the > renderer process, so it could be bypassed by a renderer exploit. (I'm also not > sure if it will prevent linking to view-source or not.) Is fixing child process security policy sufficient for this? Or is there other places I should look into? > > I think there are other ways to prevent links to certain schemes, like not > registering them as web safe schemes. This one might be different because it's > a pseudo-scheme, though. > > Maybe we need to update ChildProcessSecurityPolicyImpl::CanRequestURL? There's > a few references to kViewSourceScheme in that class, so we should look closely > at them. I'm not sure if we can prevent the process from asking for the URL, > though, since the view-source page might end up in a web page after a > browser-initiated navigation. view-source isn't registered as a web safe scheme, instead there were some special cases for it in ChildProcessSecurityPolicy. I removed those cases. https://codereview.chromium.org/1917073002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1917073002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1331: WebSecurityPolicy::registerURLSchemeAsDisplayIsolated(view_source_scheme); On 2016/04/27 21:47:03, Charlie Reis wrote: > This seems to equate to SecurityOrigin::canDisplay, which has the following > comment: > // Returns true if |document| can display content from the given URL (e.g., > // in an iframe or as an image). For example, web sites generally cannot > // display content from the user's files system. > > Does it also prevent linking or navigating to such content? I don't see the > code for that. Yes, it also prevents linking and navigation. I think |canDisplay| is a misnomer and we should probably consider renaming it and updating its documentation.
Sorry for the delay reviewing this-- I got hit with an unusually large number of reviews over the last several days. On 2016/05/12 01:14:16, Mustafa Emre Acer wrote: > Thanks Charlie, modified some tests but will look into adding more. Great. I'm guessing we'll also want a browser test for a failed location.href="view-source:http://foo.com", in addition to the unit tests (where we can't verify the renderer process's behavior). > > The current fix doesn't seem sufficient, unless there's some additional uses > of > > that display-isolated registry I'm missing. The use in SecurityOrigin is in > the > > renderer process, so it could be bypassed by a renderer exploit. (I'm also > not > > sure if it will prevent linking to view-source or not.) > > Is fixing child process security policy sufficient for this? Or is there other > places I should look into? I'm not sure. That should prevent them from working, but it might just get them rewritten to about:blank rather than failing gracefully, so we might need a bit more. Hopefully on a renderer-initiated navigation, it would behave more like what we see on location.href = "chrome://settings", which prints this in the console: Not allowed to load local resource: chrome://settings/ Do we get that with this patch? > > I think there are other ways to prevent links to certain schemes, like not > > registering them as web safe schemes. This one might be different because > it's > > a pseudo-scheme, though. > > > > Maybe we need to update ChildProcessSecurityPolicyImpl::CanRequestURL? > There's > > a few references to kViewSourceScheme in that class, so we should look closely > > at them. I'm not sure if we can prevent the process from asking for the URL, > > though, since the view-source page might end up in a web page after a > > browser-initiated navigation. > > view-source isn't registered as a web safe scheme, instead there were some > special cases for it in ChildProcessSecurityPolicy. I removed those cases. Ok, that sounds worthwhile. https://codereview.chromium.org/1917073002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1917073002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1331: WebSecurityPolicy::registerURLSchemeAsDisplayIsolated(view_source_scheme); On 2016/05/12 01:14:16, Mustafa Emre Acer wrote: > On 2016/04/27 21:47:03, Charlie Reis wrote: > > This seems to equate to SecurityOrigin::canDisplay, which has the following > > comment: > > // Returns true if |document| can display content from the given URL > (e.g., > > // in an iframe or as an image). For example, web sites generally cannot > > // display content from the user's files system. > > > > Does it also prevent linking or navigating to such content? I don't see the > > code for that. > > Yes, it also prevents linking and navigation. I think |canDisplay| is a misnomer > and we should probably consider renaming it and updating its documentation. Ah. Yes, that would be a good idea. Do you know which code prevents linking and navigation for those schemes? I couldn't find it. (I can ask dcheng@ if not.) https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:774: IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, WebUIViewSource) { Seems like a nice test. Was this behavior at risk in one of your attempts, or is this more of a general sanity check that we can do browser-initiated view-source for WebUI? https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:417: cont.LoadURL(kGURL, Referrer(), ui::PAGE_TRANSITION_LINK, std::string()); 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. https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:421: // view-source URL. Yeah, this would be concerning if it happened for renderer-initiated navigations. We expect it for browser-initiated ones, though. https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:431: EXPECT_EQ(base::ASCIIToUTF16("about:blank"), contents()->GetTitle()); I'm curious what happened here. Why did it get rewritten to about:blank? That shouldn't happen for browser-initiated navigations.
> Sorry for the delay reviewing this-- I got hit with an unusually large number of > reviews over the last several days. No worries, I see you have 67 incoming reviews, that's a lot! :) > Great. I'm guessing we'll also want a browser test for a failed > location.href="view-source:http://foo.com", in addition to the unit tests (where > we can't verify the renderer process's behavior). Yes, I'll add more tests as a new patchset, but I'm sending these comments up front in case they might give more ideas about what to test. > I'm not sure. That should prevent them from working, but it might just get them > rewritten to about:blank rather than failing gracefully, so we might need a bit > more. Hopefully on a renderer-initiated navigation, it would behave more like > what we see on location.href = "chrome://settings", which prints this in the > console: > Not allowed to load local resource: chrome://settings/ > > Do we get that with this patch? Yes, for location.href we get "Not allowed to load local resource" message. For window.open, we get a new tab with about:blank URL. This is the same behavior as chrome:// and chrome-devtools:// schemes. > Ah. Yes, that would be a good idea. Do you know which code prevents linking > and navigation for those schemes? I couldn't find it. (I can ask dcheng@ if > not.) I believe SecurityOrigin::canDisplay check in FrameLoader::prepareRequestForThisFrame prevents the linking and prints "not allowed ..." message in console. Navigation is prevented by RenderProcessHost::FilterURL which rewrites view-source to about:blank after checking ChildProcessSecurityPolicyImpl::CanRequestURL. https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:774: IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, WebUIViewSource) { On 2016/05/16 22:22:18, Charlie Reis (slow to reply) wrote: > Seems like a nice test. Was this behavior at risk in one of your attempts, or > is this more of a general sanity check that we can do browser-initiated > view-source for WebUI? I didn't run into this case in my attempts, but I got the idea from WebContentsImplTest.ContentInitiatedViewSource test. I fixed the other test now, but it seemed fragile because it manually constructed the URL to be navigated to. https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:417: cont.LoadURL(kGURL, Referrer(), ui::PAGE_TRANSITION_LINK, std::string()); On 2016/05/16 22:22:18, Charlie Reis (slow to reply) wrote: > 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. 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. https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:421: // view-source URL. On 2016/05/16 22:22:18, Charlie Reis (slow to reply) wrote: > Yeah, this would be concerning if it happened for renderer-initiated > navigations. We expect it for browser-initiated ones, though. Now that the test is testing a browser initiated navigation, removed the TODO. https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:431: EXPECT_EQ(base::ASCIIToUTF16("about:blank"), contents()->GetTitle()); On 2016/05/16 22:22:18, Charlie Reis (slow to reply) wrote: > I'm curious what happened here. Why did it get rewritten to about:blank? That > shouldn't happen for browser-initiated navigations. 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.
On 2016/05/19 01:13:55, Mustafa Emre Acer wrote: > > Sorry for the delay reviewing this-- I got hit with an unusually large number > of > > reviews over the last several days. > > No worries, I see you have 67 incoming reviews, that's a lot! :) > Thankfully, only the top 15 or so are active. :) Still, it's keeping my hands full. > > I'm not sure. That should prevent them from working, but it might just get > them > > rewritten to about:blank rather than failing gracefully, so we might need a > bit > > more. Hopefully on a renderer-initiated navigation, it would behave more like > > what we see on location.href = "chrome://settings", which prints this in the > > console: > > Not allowed to load local resource: chrome://settings/ > > > > Do we get that with this patch? > > Yes, for location.href we get "Not allowed to load local resource" message. For > window.open, we get a new tab with about:blank URL. This is the same behavior as > > chrome:// and chrome-devtools:// schemes. Great! I think we're on the right track, then. Should just be a matter of getting the tests in order. > > Ah. Yes, that would be a good idea. Do you know which code prevents linking > > and navigation for those schemes? I couldn't find it. (I can ask dcheng@ if > > not.) > > I believe SecurityOrigin::canDisplay check in > FrameLoader::prepareRequestForThisFrame > prevents the linking and prints "not allowed ..." message in console. Navigation > is > prevented by RenderProcessHost::FilterURL which rewrites view-source to > about:blank > after checking ChildProcessSecurityPolicyImpl::CanRequestURL. Thanks-- that looks right. https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:417: cont.LoadURL(kGURL, Referrer(), ui::PAGE_TRANSITION_LINK, std::string()); On 2016/05/19 01:13:54, Mustafa Emre Acer wrote: > On 2016/05/16 22:22:18, Charlie Reis (slow to reply) wrote: > > 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. > > This test is problematic in that it uses LoadURL instead of LoadURLWithParams. > NavigationControllerImpl::CreateNavigationEntry rewrites > http://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. 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. https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:421: // view-source URL. On 2016/05/19 01:13:54, Mustafa Emre Acer wrote: > On 2016/05/16 22:22:18, Charlie Reis (slow to reply) wrote: > > Yeah, this would be concerning if it happened for renderer-initiated > > navigations. We expect it for browser-initiated ones, though. > > Now that the test is testing a browser initiated navigation, removed the TODO. I'm confused-- I thought you were making this a test of renderer-initiated navigations. https://codereview.chromium.org/1917073002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/1917073002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:424: load_params.is_renderer_initiated = false; Why is this set to false? The test name and comment above say this is a test of content-initiated navigations, in which case it should be true (and should be blocked below).
Thanks Charlie, added some more tests. https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:417: cont.LoadURL(kGURL, Referrer(), ui::PAGE_TRANSITION_LINK, std::string()); On 2016/05/23 22:32:16, Charlie Reis (slow to reply) wrote: > On 2016/05/19 01:13:54, Mustafa Emre Acer wrote: > > On 2016/05/16 22:22:18, Charlie Reis (slow to reply) wrote: > > > 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. > > > > This test is problematic in that it uses LoadURL instead of LoadURLWithParams. > > NavigationControllerImpl::CreateNavigationEntry rewrites > > http://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. > > 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. Added multiple browser tests and a layout test. https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:421: // view-source URL. On 2016/05/23 22:32:16, Charlie Reis (slow to reply) wrote: > On 2016/05/19 01:13:54, Mustafa Emre Acer wrote: > > On 2016/05/16 22:22:18, Charlie Reis (slow to reply) wrote: > > > Yeah, this would be concerning if it happened for renderer-initiated > > > navigations. We expect it for browser-initiated ones, though. > > > > Now that the test is testing a browser initiated navigation, removed the TODO. > > I'm confused-- I thought you were making this a test of renderer-initiated > navigations. 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. https://codereview.chromium.org/1917073002/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/1917073002/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:424: load_params.is_renderer_initiated = false; On 2016/05/23 22:32:16, Charlie Reis (slow to reply) wrote: > Why is this set to false? The test name and comment above say this is a test of > content-initiated navigations, in which case it should be true (and should be > blocked below). Ah, I changed this to test browser-initiated navigations but forgot to change the description and the test name. Content initiated navigations are being tested in browsertests. https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html (right): https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html:18: }, 1000); This is ugly as it uses setTimeout with a non-zero value, but I can't find a better way to check that a navigation doesn't complete. Happy to change to something nicer.
creis@chromium.org changed reviewers: + japhet@chromium.org
Thanks! content/ LGTM with nits. +japhet@ for layout test. https://codereview.chromium.org/1917073002/diff/60001/content/browser/browser... File content/browser/browser_side_navigation_browsertest.cc (right): https://codereview.chromium.org/1917073002/diff/60001/content/browser/browser... content/browser/browser_side_navigation_browsertest.cc:241: } Thanks for proactively testing the PlzNavigate case! Can you add a renderer-initiated test that shows it stays blocked? See RendererInitiatedCrossSiteNavigation for a model. https://codereview.chromium.org/1917073002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/1917073002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:779: // Displayed view-source URLs don't include the scheme of the effective URL nit: Just for HTTP, right? https://codereview.chromium.org/1917073002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:783: shell()->web_contents()->GetTitle()); Maybe we can also check that we're in view-source mode? EXPECT_TRUE(shell()->web_contents()->GetController().GetLastCommittedEntry()->IsViewSourceMode()); Similar in the tests below. https://codereview.chromium.org/1917073002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:799: // EXPECT_EQ("", static_cast<WebContentsImpl*>(new_shell->web_contents()) nit: Remove. https://codereview.chromium.org/1917073002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:800: EXPECT_EQ("", new_shell->web_contents()->GetURL().spec()); Can we use a ConsoleObserverDelegate on shell() in this test as well? https://codereview.chromium.org/1917073002/diff/60001/content/public/test/con... File content/public/test/content_browser_test_utils.cc (right): https://codereview.chromium.org/1917073002/diff/60001/content/public/test/con... content/public/test/content_browser_test_utils.cc:136: message_(""), nit: I think this is unnecessary, right? https://codereview.chromium.org/1917073002/diff/60001/content/public/test/con... File content/public/test/content_browser_test_utils.h (right): https://codereview.chromium.org/1917073002/diff/60001/content/public/test/con... content/public/test/content_browser_test_utils.h:108: ~ConsoleObserverDelegate() override; nit: No blank line above. https://codereview.chromium.org/1917073002/diff/60001/content/public/test/con... content/public/test/content_browser_test_utils.h:110: bool AddMessageToConsole(WebContents* source, nit: // WebContentsDelegate overrides. https://codereview.chromium.org/1917073002/diff/60001/content/public/test/con... content/public/test/content_browser_test_utils.h:116: std::string message() { return message_; } nit: This is now a public interface, so it needs comments on these methods. Something like "Returns the most recent message sent to the console." https://codereview.chromium.org/1917073002/diff/60001/content/public/test/con... content/public/test/content_browser_test_utils.h:118: void Wait(); // Wait for the next message sent to the console. https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html (right): https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html:18: }, 1000); On 2016/05/23 23:57:47, Mustafa Emre Acer wrote: > This is ugly as it uses setTimeout with a non-zero value, but I can't find a > better way to check that a navigation doesn't complete. Happy to change to > something nicer. Yeah, we shouldn't do this. Maybe there's an error event we can listen for? Nate might know.
https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html (right): https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html:9: testRunner.notifyDone(); Nit: if (window.testRunner) https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html:18: }, 1000); On 2016/05/24 22:54:17, Charlie Reis wrote: > On 2016/05/23 23:57:47, Mustafa Emre Acer wrote: > > This is ugly as it uses setTimeout with a non-zero value, but I can't find a > > better way to check that a navigation doesn't complete. Happy to change to > > something nicer. > > Yeah, we shouldn't do this. Maybe there's an error event we can listen for? > Nate might know. This is a bit tougher because it's view-source. Normally I'd say to navigate to pass-and-notify-done.html, but its script won't run if it's view-source'ed. :) Instead of using waitUntilDone/notifyDone, you could use testRunner.queueLoad(). The test won't complete while a queued load hasn't completed, so you could have this page just display PASS, and try to navigationt to view-source:fail.html or similar. It might work to just drop the waitUntilDone/notifyDone calls, too, I can't remember if setting window.location before the load event is guaranteed to block test completion.
https://codereview.chromium.org/1917073002/diff/60001/content/browser/browser... File content/browser/browser_side_navigation_browsertest.cc (right): https://codereview.chromium.org/1917073002/diff/60001/content/browser/browser... content/browser/browser_side_navigation_browsertest.cc:241: } On 2016/05/24 22:54:16, Charlie Reis wrote: > Thanks for proactively testing the PlzNavigate case! Can you add a > renderer-initiated test that shows it stays blocked? See > RendererInitiatedCrossSiteNavigation for a model. Done. https://codereview.chromium.org/1917073002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/1917073002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:779: // Displayed view-source URLs don't include the scheme of the effective URL On 2016/05/24 22:54:16, Charlie Reis wrote: > nit: Just for HTTP, right? Done. https://codereview.chromium.org/1917073002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:783: shell()->web_contents()->GetTitle()); On 2016/05/24 22:54:16, Charlie Reis wrote: > Maybe we can also check that we're in view-source mode? > > EXPECT_TRUE(shell()->web_contents()->GetController().GetLastCommittedEntry()->IsViewSourceMode()); > > Similar in the tests below. Done, except for window.open case where there is no committed navigation (checking for null there). https://codereview.chromium.org/1917073002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:799: // EXPECT_EQ("", static_cast<WebContentsImpl*>(new_shell->web_contents()) On 2016/05/24 22:54:16, Charlie Reis wrote: > nit: Remove. Done. https://codereview.chromium.org/1917073002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:800: EXPECT_EQ("", new_shell->web_contents()->GetURL().spec()); On 2016/05/24 22:54:16, Charlie Reis wrote: > Can we use a ConsoleObserverDelegate on shell() in this test as well? Unfortunately no, because the console message gets logged in the new shell. So we need ConsoleObserverDelegate use the new shell, but by the time new_shell_observer.GetShell() returns, it's too late for observing the message. https://codereview.chromium.org/1917073002/diff/60001/content/public/test/con... File content/public/test/content_browser_test_utils.cc (right): https://codereview.chromium.org/1917073002/diff/60001/content/public/test/con... content/public/test/content_browser_test_utils.cc:136: message_(""), On 2016/05/24 22:54:16, Charlie Reis wrote: > nit: I think this is unnecessary, right? Copypasta :) https://codereview.chromium.org/1917073002/diff/60001/content/public/test/con... File content/public/test/content_browser_test_utils.h (right): https://codereview.chromium.org/1917073002/diff/60001/content/public/test/con... content/public/test/content_browser_test_utils.h:108: ~ConsoleObserverDelegate() override; On 2016/05/24 22:54:17, Charlie Reis wrote: > nit: No blank line above. Done. https://codereview.chromium.org/1917073002/diff/60001/content/public/test/con... content/public/test/content_browser_test_utils.h:110: bool AddMessageToConsole(WebContents* source, On 2016/05/24 22:54:17, Charlie Reis wrote: > nit: // WebContentsDelegate overrides. Done. https://codereview.chromium.org/1917073002/diff/60001/content/public/test/con... content/public/test/content_browser_test_utils.h:116: std::string message() { return message_; } On 2016/05/24 22:54:17, Charlie Reis wrote: > nit: This is now a public interface, so it needs comments on these methods. > Something like "Returns the most recent message sent to the console." Done. https://codereview.chromium.org/1917073002/diff/60001/content/public/test/con... content/public/test/content_browser_test_utils.h:118: void Wait(); On 2016/05/24 22:54:17, Charlie Reis wrote: > // Wait for the next message sent to the console. Done. https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html (right): https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html:18: }, 1000); On 2016/05/27 23:57:32, Nate Chapin wrote: > On 2016/05/24 22:54:17, Charlie Reis wrote: > > On 2016/05/23 23:57:47, Mustafa Emre Acer wrote: > > > This is ugly as it uses setTimeout with a non-zero value, but I can't find a > > > better way to check that a navigation doesn't complete. Happy to change to > > > something nicer. > > > > Yeah, we shouldn't do this. Maybe there's an error event we can listen for? > > Nate might know. > > This is a bit tougher because it's view-source. Normally I'd say to navigate to > pass-and-notify-done.html, but its script won't run if it's view-source'ed. :) > > Instead of using waitUntilDone/notifyDone, you could use testRunner.queueLoad(). > The test won't complete while a queued load hasn't completed, so you could have > this page just display PASS, and try to navigationt to view-source:fail.html or > similar. > > It might work to just drop the waitUntilDone/notifyDone calls, too, I can't > remember if setting window.location before the load event is guaranteed to block > test completion. Looks like queueLoad triggers a browser initiated navigation, so it's not helping here. Unfortunately removing waitUntilDone/notifyDone doesn't work either, the test finishes before the console message is printed (hence the setTimeout in my original case). Do we have something that does queueContentInitiatedLoad()? Should we introduce one?
Thanks for the updates! LGTM, and I think it might be safe to land without the layout test, given the other extensive test coverage. https://codereview.chromium.org/1917073002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_browsertest.cc (right): https://codereview.chromium.org/1917073002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_browsertest.cc:800: EXPECT_EQ("", new_shell->web_contents()->GetURL().spec()); On 2016/05/31 23:57:54, Mustafa Emre Acer wrote: > On 2016/05/24 22:54:16, Charlie Reis wrote: > > Can we use a ConsoleObserverDelegate on shell() in this test as well? > > Unfortunately no, because the console message gets logged in the new shell. So > we need ConsoleObserverDelegate use the new shell, but by the time > new_shell_observer.GetShell() returns, it's too late for observing the message. Acknowledged. https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html (right): https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html:18: }, 1000); On 2016/05/31 23:57:54, Mustafa Emre Acer wrote: > On 2016/05/27 23:57:32, Nate Chapin wrote: > > On 2016/05/24 22:54:17, Charlie Reis wrote: > > > On 2016/05/23 23:57:47, Mustafa Emre Acer wrote: > > > > This is ugly as it uses setTimeout with a non-zero value, but I can't find > a > > > > better way to check that a navigation doesn't complete. Happy to change to > > > > something nicer. > > > > > > Yeah, we shouldn't do this. Maybe there's an error event we can listen for? > > > > Nate might know. > > > > This is a bit tougher because it's view-source. Normally I'd say to navigate > to > > pass-and-notify-done.html, but its script won't run if it's view-source'ed. :) > > > > Instead of using waitUntilDone/notifyDone, you could use > testRunner.queueLoad(). > > The test won't complete while a queued load hasn't completed, so you could > have > > this page just display PASS, and try to navigationt to view-source:fail.html > or > > similar. > > > > It might work to just drop the waitUntilDone/notifyDone calls, too, I can't > > remember if setting window.location before the load event is guaranteed to > block > > test completion. > > Looks like queueLoad triggers a browser initiated navigation, so it's not > helping here. > > Unfortunately removing waitUntilDone/notifyDone doesn't work either, the test > finishes before the console message is printed (hence the setTimeout in my > original case). > > Do we have something that does queueContentInitiatedLoad()? Should we introduce > one? Do we actually need a layout test for this? You've already added browser_test coverage for renderer-initiated navigations, which should cover the same thing. I'd suggest proceeding without it unless there's a reason I'm missing.
On 2016/06/01 22:59:51, Charlie Reis wrote: > Thanks for the updates! LGTM, and I think it might be safe to land without the > layout test, given the other extensive test coverage. > > https://codereview.chromium.org/1917073002/diff/60001/content/browser/web_con... > File content/browser/web_contents/web_contents_impl_browsertest.cc (right): > > https://codereview.chromium.org/1917073002/diff/60001/content/browser/web_con... > content/browser/web_contents/web_contents_impl_browsertest.cc:800: EXPECT_EQ("", > new_shell->web_contents()->GetURL().spec()); > On 2016/05/31 23:57:54, Mustafa Emre Acer wrote: > > On 2016/05/24 22:54:16, Charlie Reis wrote: > > > Can we use a ConsoleObserverDelegate on shell() in this test as well? > > > > Unfortunately no, because the console message gets logged in the new shell. So > > we need ConsoleObserverDelegate use the new shell, but by the time > > new_shell_observer.GetShell() returns, it's too late for observing the > message. > > Acknowledged. > > https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html > (right): > > https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html:18: > }, 1000); > On 2016/05/31 23:57:54, Mustafa Emre Acer wrote: > > On 2016/05/27 23:57:32, Nate Chapin wrote: > > > On 2016/05/24 22:54:17, Charlie Reis wrote: > > > > On 2016/05/23 23:57:47, Mustafa Emre Acer wrote: > > > > > This is ugly as it uses setTimeout with a non-zero value, but I can't > find > > a > > > > > better way to check that a navigation doesn't complete. Happy to change > to > > > > > something nicer. > > > > > > > > Yeah, we shouldn't do this. Maybe there's an error event we can listen > for? > > > > > > Nate might know. > > > > > > This is a bit tougher because it's view-source. Normally I'd say to navigate > > to > > > pass-and-notify-done.html, but its script won't run if it's view-source'ed. > :) > > > > > > Instead of using waitUntilDone/notifyDone, you could use > > testRunner.queueLoad(). > > > The test won't complete while a queued load hasn't completed, so you could > > have > > > this page just display PASS, and try to navigationt to view-source:fail.html > > or > > > similar. > > > > > > It might work to just drop the waitUntilDone/notifyDone calls, too, I can't > > > remember if setting window.location before the load event is guaranteed to > > block > > > test completion. > > > > Looks like queueLoad triggers a browser initiated navigation, so it's not > > helping here. > > > > Unfortunately removing waitUntilDone/notifyDone doesn't work either, the test > > finishes before the console message is printed (hence the setTimeout in my > > original case). > > > > Do we have something that does queueContentInitiatedLoad()? Should we > introduce > > one? > > Do we actually need a layout test for this? You've already added browser_test > coverage for renderer-initiated navigations, which should cover the same thing. > I'd suggest proceeding without it unless there's a reason I'm missing. I'd say that if you want a blink test, it's probably easier to do it as a WebFrameTest at this point. But I don't think that needs to block landing the fix.
On 2016/06/01 23:09:54, Nate Chapin wrote: > On 2016/06/01 22:59:51, Charlie Reis wrote: > > Thanks for the updates! LGTM, and I think it might be safe to land without > the > > layout test, given the other extensive test coverage. > > > > > https://codereview.chromium.org/1917073002/diff/60001/content/browser/web_con... > > File content/browser/web_contents/web_contents_impl_browsertest.cc (right): > > > > > https://codereview.chromium.org/1917073002/diff/60001/content/browser/web_con... > > content/browser/web_contents/web_contents_impl_browsertest.cc:800: > EXPECT_EQ("", > > new_shell->web_contents()->GetURL().spec()); > > On 2016/05/31 23:57:54, Mustafa Emre Acer wrote: > > > On 2016/05/24 22:54:16, Charlie Reis wrote: > > > > Can we use a ConsoleObserverDelegate on shell() in this test as well? > > > > > > Unfortunately no, because the console message gets logged in the new shell. > So > > > we need ConsoleObserverDelegate use the new shell, but by the time > > > new_shell_observer.GetShell() returns, it's too late for observing the > > message. > > > > Acknowledged. > > > > > https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/Layo... > > File > > > third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html > > (right): > > > > > https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html:18: > > }, 1000); > > On 2016/05/31 23:57:54, Mustafa Emre Acer wrote: > > > On 2016/05/27 23:57:32, Nate Chapin wrote: > > > > On 2016/05/24 22:54:17, Charlie Reis wrote: > > > > > On 2016/05/23 23:57:47, Mustafa Emre Acer wrote: > > > > > > This is ugly as it uses setTimeout with a non-zero value, but I can't > > find > > > a > > > > > > better way to check that a navigation doesn't complete. Happy to > change > > to > > > > > > something nicer. > > > > > > > > > > Yeah, we shouldn't do this. Maybe there's an error event we can listen > > for? > > > > > > > > Nate might know. > > > > > > > > This is a bit tougher because it's view-source. Normally I'd say to > navigate > > > to > > > > pass-and-notify-done.html, but its script won't run if it's > view-source'ed. > > :) > > > > > > > > Instead of using waitUntilDone/notifyDone, you could use > > > testRunner.queueLoad(). > > > > The test won't complete while a queued load hasn't completed, so you could > > > have > > > > this page just display PASS, and try to navigationt to > view-source:fail.html > > > or > > > > similar. > > > > > > > > It might work to just drop the waitUntilDone/notifyDone calls, too, I > can't > > > > remember if setting window.location before the load event is guaranteed to > > > block > > > > test completion. > > > > > > Looks like queueLoad triggers a browser initiated navigation, so it's not > > > helping here. > > > > > > Unfortunately removing waitUntilDone/notifyDone doesn't work either, the > test > > > finishes before the console message is printed (hence the setTimeout in my > > > original case). > > > > > > Do we have something that does queueContentInitiatedLoad()? Should we > > introduce > > > one? > > > > Do we actually need a layout test for this? You've already added browser_test > > coverage for renderer-initiated navigations, which should cover the same > thing. > > I'd suggest proceeding without it unless there's a reason I'm missing. > > I'd say that if you want a blink test, it's probably easier to do it as a > WebFrameTest at this point. But I don't think that needs to block landing the > fix. The layout test was the first test I wrote for this CL, but given that there are now other tests I agree it's not needed anymore. I'll land the CL without it.
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/1917073002/#ps120001 (title: "Delete layout tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917073002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917073002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by meacer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917073002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917073002/120001
Message was sent while issue was closed.
Description was changed from ========== Block webpages from navigating to view-source URLs. This CL blocks navigations to view-source URLs initiated by a page via window.location, window.open, A tag, etc. It still allows user initiated navigations such as directly entering the URL, clicking "view page source" or "open in new tab" in the context menu. BUG=247151,606619 ========== to ========== Block webpages from navigating to view-source URLs. This CL blocks navigations to view-source URLs initiated by a page via window.location, window.open, A tag, etc. It still allows user initiated navigations such as directly entering the URL, clicking "view page source" or "open in new tab" in the context menu. BUG=247151,606619 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Block webpages from navigating to view-source URLs. This CL blocks navigations to view-source URLs initiated by a page via window.location, window.open, A tag, etc. It still allows user initiated navigations such as directly entering the URL, clicking "view page source" or "open in new tab" in the context menu. BUG=247151,606619 ========== to ========== Block webpages from navigating to view-source URLs. This CL blocks navigations to view-source URLs initiated by a page via window.location, window.open, A tag, etc. It still allows user initiated navigations such as directly entering the URL, clicking "view page source" or "open in new tab" in the context menu. BUG=247151,606619 Committed: https://crrev.com/ce6b6603637ee045041ccb49359fbae617d84ba5 Cr-Commit-Position: refs/heads/master@{#397505} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ce6b6603637ee045041ccb49359fbae617d84ba5 Cr-Commit-Position: refs/heads/master@{#397505} |