Change error pages to use a new chrome-error:// scheme. [chromium/src : master]
Alex Moshchuk (Gerrit)
Alex Moshchuk uploaded patch set #6 to this change.
Change error pages to use a new chrome-error:// scheme.
This CL changes error pages to use chrome-error://chromewebdata/ instead
of data:text/html,chromewebdata.
This has the benefit that error pages won't inherit CSP from their
creator. Previously, when a page defined a CSP and then ended up
loading an error page in a subframe, the error page might've been
broken by the parent's CSP, since, for example, it needed to execute
inline scripts. This could result in the error page not showing up
correctly and false CSP reports being sent.
Bug: 703801
Change-Id: I45fa0aa157523450c33c6464f96414ff742e8604
---
M chrome/browser/extensions/window_open_apitest.cc
M chrome/test/chromedriver/chrome/navigation_tracker.cc
M chrome/test/chromedriver/test/run_py_tests.py
M chrome/test/chromedriver/window_commands.cc
M content/common/url_schemes.cc
M content/public/common/url_constants.cc
M content/public/common/url_constants.h
M third_party/WebKit/LayoutTests/inspector-protocol/page/frameNavigatedToUnreachableUrl-expected.txt
M third_party/WebKit/LayoutTests/inspector/console/console-context-selector-expected.txt
M third_party/WebKit/Source/web/tests/WebFrameTest.cpp
10 files changed, 22 insertions(+), 18 deletions(-)
To view, visit change 580169. To unsubscribe, visit settings.
Alex Moshchuk (Gerrit)
Alex Moshchuk uploaded patch set #8 to this change.
Change error pages to use a new chrome-error:// scheme.
This CL changes error pages to use chrome-error://chromewebdata/
instead of data:text/html,chromewebdata.
This has the benefit that error pages won't inherit CSP from their
parent/opener. Previously, when a page defined a CSP and then ended
up loading an error page in a subframe, the error page might've been
broken by the parent's CSP, since, for example, it needs to execute
inline scripts. This could result in the error page not showing up
correctly and/or false CSP reports being sent.
The new scheme is marked as secure and as requiring an opaque origin
to match previous behavior. Web pages are not allowed to directly
navigate to the error URL anymore. In the future, it's possible to
further utilize the host/path portion of the URL to identify different
kinds of error pages.
Bug: 703801
Change-Id: I45fa0aa157523450c33c6464f96414ff742e8604
---
M chrome/browser/chrome_navigation_browsertest.cc
M chrome/browser/extensions/window_open_apitest.cc
M chrome/test/chromedriver/chrome/navigation_tracker.cc
M chrome/test/chromedriver/test/run_py_tests.py
M chrome/test/chromedriver/window_commands.cc
A chrome/test/data/page_with_csp_and_error_iframe.html
M content/browser/child_process_security_policy_impl.cc
M content/browser/child_process_security_policy_unittest.cc
M content/common/url_schemes.cc
M content/public/common/url_constants.cc
M content/public/common/url_constants.h
M third_party/WebKit/LayoutTests/inspector-protocol/page/frameNavigatedToUnreachableUrl-expected.txt
M third_party/WebKit/LayoutTests/inspector/console/console-context-selector-expected.txt
M third_party/WebKit/Source/web/tests/WebFrameTest.cpp
14 files changed, 150 insertions(+), 18 deletions(-)
To view, visit change 580169. To unsubscribe, visit settings.
Alex Moshchuk (Gerrit)
Alex Moshchuk uploaded patch set #10 to this change.
Change error pages to use a new chrome-error:// scheme.
This CL changes error pages to use chrome-error://chromewebdata/
instead of data:text/html,chromewebdata.
This has the benefit that error pages won't inherit CSP from their
parent/opener. Previously, when a page defined a CSP and then ended
up loading an error page in a subframe, the error page might've been
broken by the parent's CSP, since, for example, it needs to execute
inline scripts. This could result in the error page not showing up
correctly and/or false CSP reports being sent.
The new scheme is marked as secure and as requiring an opaque origin
to match previous behavior. Web pages are not allowed to directly
navigate to the error URL anymore. In the future, it's possible to
further utilize the host/path portion of the URL to identify different
kinds of error pages.
Bug: 703801
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I45fa0aa157523450c33c6464f96414ff742e8604
---
M chrome/browser/chrome_navigation_browsertest.cc
M chrome/browser/extensions/window_open_apitest.cc
M chrome/test/chromedriver/chrome/navigation_tracker.cc
M chrome/test/chromedriver/test/run_py_tests.py
M chrome/test/chromedriver/window_commands.cc
A chrome/test/data/empty_with_404.html
A chrome/test/data/empty_with_404.html.mock-http-headers
A chrome/test/data/page_with_csp_and_error_iframe.html
M content/browser/child_process_security_policy_impl.cc
M content/browser/child_process_security_policy_unittest.cc
M content/browser/frame_host/render_frame_host_impl.cc
M content/common/url_schemes.cc
M content/public/common/url_constants.cc
M content/public/common/url_constants.h
M third_party/WebKit/LayoutTests/inspector-protocol/page/frameNavigatedToUnreachableUrl-expected.txt
M third_party/WebKit/LayoutTests/inspector/console/console-context-selector-expected.txt
M third_party/WebKit/Source/web/tests/WebFrameTest.cpp
17 files changed, 137 insertions(+), 18 deletions(-)
To view, visit change 580169. To unsubscribe, or for help writing mail filters, visit settings.
Alex Moshchuk (Gerrit)
Alex Moshchuk uploaded patch set #11 to this change.
Change error pages to use a new chrome-error:// scheme.
This CL changes error pages to use chrome-error://chromewebdata/
instead of data:text/html,chromewebdata.
This has the benefit that error pages won't inherit CSP from their
parent/opener. Previously, when a page defined a CSP and then ended
up loading an error page in a subframe, the error page might've been
broken by the parent's CSP, since, for example, it needs to execute
inline scripts. This could result in the error page not showing up
correctly and/or false CSP reports being sent.
The new scheme is marked as secure and as requiring an opaque origin
to match previous behavior.
Web pages used to be able to directly load the error URL, which just
showed up as "chromewebdata". With this change, navigating to the
error URL would bring up the external protocol dialog instead, so this
CL prevents renderers from directly navigating to or redirecting to
error URLs.
Alex Moshchuk (Gerrit)
Alex Moshchuk would like Nasko Oskov, Mike West and Charlie Reis to review this change.
Alex Moshchuk (Gerrit)
Nasko, Charlie, Mike: can you please review this? This is my first crack at renaming the error page URL to use chrome-error://chromewebdata/, and I figured the more eyes I can get on this, the better. Are there any other places where I should register chrome-error://?
5 comments:
File chrome/browser/chrome_navigation_browsertest.cc:
Patch Set #11, Line 521:
EmptyDocumentWithErrorCode) {This whole scenario turned out not to be a problem after all -- the Android Webview tests were failing due to URL mismatch in DidFinishLoad and not due to this. But I had the test already written, so thought I might as well keep it to increase test coverage for it beyond Android Webview as we keep refactoring error pages. This logic currently lives in Blink in RenderFrameImpl::RunScriptsAtDocumentReady [1], and ideally the detection should move to the browser process if we still even care about it, but it's not something I wanted to do in this CL.
File content/browser/child_process_security_policy_impl.cc:
Patch Set #11, Line 681:
if (scheme == kErrorScheme)This drops redirects to the error page URL for PlzNavigate (this is checked by NavigationRequest::OnRequestRedirected). For non-PlzNavigate, sadly, this isn't enough, as that path goes through ResourceLoader::OnReceivedRedirect, which checks CanRequestURL instead of CanRedirectToURL. We could make it work on the old path by checking CanRedirectToURL instead of/in addition to CanRequestURL, but I figured it wasn't worth the effort, given that PlzNavigate is ready to ship.
File content/browser/child_process_security_policy_unittest.cc:
Patch Set #11, Line 211:
EXPECT_FALSE(p->CanCommitURL(kRendererID, GURL(kUnreachableWebDataURL)));We get this for free since we don't grant special access to the error scheme or origin in CPSP.
File content/browser/frame_host/render_frame_host_impl.cc:
Patch Set #11, Line 2289:
if (validated_params.url.SchemeIs(kErrorScheme))It'd be nice to make CanRequestURL return false for the error page URL, and it was what I tried initially to cancel the navigation if a renderer sets "location='chrome-error://chromewebdata'", but that didn't work out.
The reason is that this causes RPHI::FilterURL to rewrite the error URL to "about:blank" in a lot of places (DidStart, DidCommit in the redirect chain, DidFinishLoad), and it turned out that some of those places actually care about the error URL being preserved. For example, Android Webview monitors the URL in WCO::DidFinishLoad and has explicit checks against the error page URL for it [1].
I wanted to do this as it just seems saner to not let renderers directly navigate to chrome-error://, plus not doing this causes the external protocol handler to come up. For now, I've replaced this with a narrowed check here in OnBeginNavigation, which works for PlzNavigate. If we wanted to support the same without PlzNavigate, we could modify ResourceDispatcherHostImpl::ShouldServiceRequest to drop it, but again I'm not sure it's worth the trouble at this point.
[1] See https://cs.chromium.org/chromium/src/android_webview/browser/aw_contents_statics.cc?l=67&rcl=eb478a08ad6f0944e1ddf17e25948e6686abb300 and
https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/android_webview/AwWebContentsObserver.java?l=44&rcl=eb478a08ad6f0944e1ddf17e25948e6686abb300
File content/public/common/url_constants.cc:
Patch Set #11, Line 64:
const char kUnreachableWebDataURL[] = "chrome-error://chromewebdata/";Originally, I had this without the trailing slash, which cause issues with fragment navigations on the error page. This is apparently a thing and exercised by ErrorPageAutoReloadTest.IgnoresSameDocumentNavigation and DNSError_DoReloadAfterSameDocumentNavigation. Having the trailing slash allows the fragment navigation to happen (apparently due to intricacies deep in the URL parsing code), which I don't think is that bad if we ever want to add more structure to chrome-error URLs. It also makes more sense anyway.
To view, visit change 580169. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
The bits I can stamp LGTM. Thanks for putting this together!
2 comments:
Patch Set #11, Line 681:
if (scheme == kErrorScheme)This drops redirects to the error page URL for PlzNavigate (this is checked
SGTM, but I'll defer to folks who own that code for their opinion. It will be lovely when all the non-PlzNavigate code is ripped out of the codebase to avoid this kind of confusion.
File content/public/common/url_constants.cc:
Patch Set #11, Line 64:
const char kUnreachableWebDataURL[] = "chrome-error://chromewebdata/";Originally, I had this without the trailing slash, which cause issues with
This seems fine to me. We shouldn't rely on having the same behavior as `data:` here, because `data:`'s fragment navigation behavior is absurd. :)
To view, visit change 580169. To unsubscribe, or for help writing mail filters, visit settings.
Nasko Oskov (Gerrit)
Super exciting change, thanks for working on it! Mostly nits and simple questions.
5 comments:
File chrome/browser/chrome_navigation_browsertest.cc:
Patch Set #11, Line 482:
domAutomationController.sendDo we need to explicitly wrap these in domAutomationController.send() calls? Most content/ tests don't, so I wonder if it is just syntax sugar we have in content/ that helps eliminate it.
Patch Set #11, Line 521:
EmptyDocumentWithErrorCode) {This whole scenario turned out not to be a problem after all -- the Android
Thanks for the test. However, it doesn't seem to check that the content of the error page is indeed there. How does it fail if something doesn't work correctly?
File chrome/test/data/empty_with_404.html.mock-http-headers:
Patch Set #11, Line 2:
Content-Type: text/htmlShould this also include Content-Length: 0?
File content/browser/frame_host/render_frame_host_impl.cc:
Patch Set #11, Line 2289:
if (validated_params.url.SchemeIs(kErrorScheme))It'd be nice to make CanRequestURL return false for the error page URL, and
Should we also consider this a bad message and terminate the renderer process?
File content/public/common/url_constants.h:
Patch Set #11, Line 21:
CONTENT_EXPORT extern const char kErrorScheme[];nit: kChromeErrorScheme?
To view, visit change 580169. To unsubscribe, or for help writing mail filters, visit settings.
Charlie Reis (Gerrit)
A few questions to make sure I'm following, but mostly seems good.
4 comments:
Patch Set #11, Line 12:
This has the benefit that error pages won't inherit CSP from theirI'm curious, which part of the new approach prevents this inheritance?
File chrome/browser/chrome_navigation_browsertest.cc:
Patch Set #11, Line 451:
IN_PROC_BROWSER_TEST_F(ChromeNavigationBrowserTest,Is there a reason these tests live in the chrome/ layer? Most of the implementation seems to be within content.
File content/browser/child_process_security_policy_unittest.cc:
Patch Set #11, Line 211:
EXPECT_FALSE(p->CanCommitURL(kRendererID, GURL(kUnreachableWebDataURL)));We get this for free since we don't grant special access to the error schem
I thought that we did end up committing the error URL in normal processes, right? Won't that be a problem if CanCommitURL returns false for it?
File content/common/url_schemes.cc:
Patch Set #11, Line 70:
for (auto& scheme : schemes.csp_bypassing_schemes)Does this scheme bypass CSP for some other reason?
To view, visit change 580169. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
(Actually LGTMing this time)
Patch set 11:Code-Review +1
2 comments:
Patch Set #11, Line 12:
This has the benefit that error pages won't inherit CSP from theirI'm curious, which part of the new approach prevents this inheritance?
The existing code in Blink enumerates the schemes which inherit a policy from their parent: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.cpp?rcl=2dab99eb614d76c0446b46538515f13a77319408&l=5590.
File content/common/url_schemes.cc:
Patch Set #11, Line 70:
for (auto& scheme : schemes.csp_bypassing_schemes)Does this scheme bypass CSP for some other reason?
That's a good question. I wonder what our current behavior is for `frame-src 'none'` when committing the existing error page? It's probably not correct. :) I'm a little wary of adding another scheme to this list, as I think it's already a bit less restrictive than it should be.
Rather than poke at it in this patch, I'd suggest adding a `TODO(mkwst)`, and I'll try to put together reasonable behavior.
To view, visit change 580169. To unsubscribe, or for help writing mail filters, visit settings.
Alex Moshchuk (Gerrit)
Thanks all for the reviews! Some responses below.
8 comments:
Is there a reason these tests live in the chrome/ layer? Most of the imple
Yes, content_browsertests don't appear to actually load full error pages, but rather leave the contents blank. When I tried the test below from content/, is_error() was false, and the location.href of the committed document was "http://invalid.foo" instead of chrome-error://chromewebdata/. The url_is_unreachable in DidCommit was false (despite the correct 404 status code); I didn't dig much further into why, but maybe because of https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?l=2515&rcl=10820afc6a5b6462e5382ee518c74dcf763669be and/or https://cs.chromium.org/chromium/src/chrome/renderer/net/net_error_helper.cc?l=226&rcl=10820afc6a5b6462e5382ee518c74dcf763669be being in chrome/.
Patch Set #11, Line 482:
teScriptAndExtractString(
Do we need to explicitly wrap these in domAutomationController.send() calls
I thought ExecuteScript doesn't require this, but ExecuteScriptAndExtract* do: https://cs.chromium.org/chromium/src/content/public/test/browser_test_utils.h?l=245&rcl=f552ae48cd2725ebcb2b5811e74699e376b32bab
AFAICT the tests in content/ that extract return values also use the domAutomationController.send right now; if there's a way to get away without it, I'd like to know. :) I think that's something that Nick is simplifying in his ExecuteScript refactor though?Patch Set #11, Line 521:
IN_PROC_BROWSER_TEST_F(ChromeNavigationBrowserTest,
Thanks for the test. However, it doesn't seem to check that the content of
Yes, that's a fair point. If the error page doesn't load, we won't get PAGE_TYPE_ERROR for the last committed entry's PageType, as well as last_navigation_succeeded(), but I agree it's worth also checking that the error page text actually showed up for this test. One problem with checking it is that the text appears to be loaded asynchronously by scripts running on the error page after DidFinishLoad. I was hesitant to add a polling check for it, but did it now, as I didn't see a better way.
File chrome/test/data/empty_with_404.html.mock-http-headers:
Patch Set #11, Line 2:
Content-Type: text/htmlShould this also include Content-Length: 0?
Patch Set #11, Line 211:
EXPECT_FALSE(p->CanCommitURL(kRendererID, GURL(kUnreachableWebDataURL)));
I thought that we did end up committing the error URL in normal processes,
I think today the committed URL (params.url in DidCommit) is the URL of the page that failed to load, not kUnreachableWebDataURL, so that should be fine. This check will only matter if someone explicitly tries to navigate to kUnreachableWebDataURL, like in one of my tests. I think params.base_url, as well as one of the redirect entries has kUnreachableWebDataURL though, and some places check that, e.g. [1]. Interestingly, the URL we pass to DidStartProvisionalLoad and DidFinishLoad are indeed kUnreachableWebDataURL when we are loading the error page. It's a mess :/, and we will probably need to revisit this if we want to make error pages commit with the new error scheme, rather than with the URL that failed to load.
File content/browser/frame_host/render_frame_host_impl.cc:
Patch Set #11, Line 2289:
if (validated_params.url.SchemeIs(kChromeErrorScheme))
Should we also consider this a bad message and terminate the renderer proce
I considered this - I think in the common case though, this is probably not a sign of an exploited renderer, but rather someone typing in "location='chrome-error://chromewebdata'" in DevTools, after finding out that this is what "location.href" returns on an error page. I didn't think this was worth killing the process for, but if you feel it's worth it, I can add it.
File content/common/url_schemes.cc:
Patch Set #11, Line 70:
// TODO(mkwst): Investigate whether chrome-error should be included in
That's a good question. I wonder what our current behavior is for `frame-sr
frame-src: 'none' makes an error page in a subframe not load without PlzNavigate, but it appears to load with PlzNavigate, whether or not chrome-error:// is included in csp_bypassing_schemes. Though with PlzNav, the error page contents is "Requests to the server have been blocked by an extension," which is probably not right. :/
I followed Mike's suggestion and added a TODO to deal with this in a followup.
File content/public/common/url_constants.h:
Patch Set #11, Line 21:
CONTENT_EXPORT extern const char kChromeUIScheme[]; // Used for WebUIs.nit: kChromeErrorScheme?
Done
To view, visit change 580169. To unsubscribe, or for help writing mail filters, visit settings.
Charlie Reis (Gerrit)
Thanks-- almost set, but one more question. There's another set of registrations in RenderThreadImpl::RegisterSchemes(), and I'm wondering if we need to do either of the following?
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated
WebSecurityPolicy::RegisterURLSchemeAsNotAllowingJavascriptURLs
4 comments:
Yes, content_browsertests don't appear to actually load full error pages, b
Ah, thanks. Maybe it's worth mentioning in the test comment that it's here because error pages are only fully defined in chrome/.
File content/browser/child_process_security_policy_unittest.cc:
Patch Set #11, Line 211:
EXPECT_FALSE(p->CanCommitURL(kRendererID, GURL(kUnreachableWebDataURL)));
I think today the committed URL (params.url in DidCommit) is the URL of the
I see. Ok, we can revisit later if it becomes a problem.
File content/browser/frame_host/render_frame_host_impl.cc:
Patch Set #11, Line 2289:
if (validated_params.url.SchemeIs(kChromeErrorScheme))
I considered this - I think in the common case though, this is probably not
Yeah, given that the renderer doesn't block pages from doing it, we probably shouldn't kill the renderer if it happens.
File content/common/url_schemes.cc:
Patch Set #11, Line 70:
// TODO(mkwst): Investigate whether chrome-error should be included in
frame-src: 'none' makes an error page in a subframe not load without PlzNav
Ack
To view, visit change 580169. To unsubscribe, or for help writing mail filters, visit settings.
Alex Moshchuk (Gerrit)
Patch Set 13:
(4 comments)
Thanks-- almost set, but one more question. There's another set of registrations in RenderThreadImpl::RegisterSchemes(), and I'm wondering if we need to do either of the following?
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated
WebSecurityPolicy::RegisterURLSchemeAsNotAllowingJavascriptURLs
Oh, good find! I think RegisterURLSchemeAsDisplayIsolated makes sense here - I think this prevents a regular web page from embedding chrome-error:// in an iframe or image. Previously that was allowed and in fact opened the external protocol handler. I added this and also added some test coverage for it to NavigationToErrorURLIsDisallowed. I'll also see what trybots think about it.
I'm not certain about RegisterURLSchemeAsNotAllowingJavascriptURLs. This "protects privileged pages against bookmarklets and other javascript manipulations," but since chrome-error:// URLs aren't really privileged, we probably don't need this? (It's also plausible that some existing bookmarklets might run on errors, and this would break them.)
I'm curious what Mike has to say about both of these though, as he probably understands the context for both of these scheme lists much better. :)
1 comment:
Ah, thanks. Maybe it's worth mentioning in the test comment that it's here
Done
To view, visit change 580169. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
Patch Set 13:
(1 comment)
Patch Set 13:
(4 comments)
Thanks-- almost set, but one more question. There's another set of registrations in RenderThreadImpl::RegisterSchemes(), and I'm wondering if we need to do either of the following?
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated
WebSecurityPolicy::RegisterURLSchemeAsNotAllowingJavascriptURLsOh, good find! I think RegisterURLSchemeAsDisplayIsolated makes sense here - I think this prevents a regular web page from embedding chrome-error:// in an iframe or image. Previously that was allowed and in fact opened the external protocol handler. I added this and also added some test coverage for it to NavigationToErrorURLIsDisallowed. I'll also see what trybots think about it.
I agree. If we can lock this down, that seems like a win.
I'm not certain about RegisterURLSchemeAsNotAllowingJavascriptURLs. This "protects privileged pages against bookmarklets and other javascript manipulations," but since chrome-error:// URLs aren't really privileged, we probably don't need this? (It's also plausible that some existing bookmarklets might run on errors, and this would break them.)
I'd recommend erring on the side of caution, and blocking bookmarklets from executing on error pages. I think we generally do a good job of not introducing interesting features on these pages (the most powerful thing I can think of is opting-into extended error reporting on interstitials, and I can imagine moving those to `chrome-error:`?), but if we consider them part of Chrome, we should prevent them from being modified.
Charlie Reis (Gerrit)
Patch Set 14:
Patch Set 13:
(1 comment)
Patch Set 13:
(4 comments)
Thanks-- almost set, but one more question. There's another set of registrations in RenderThreadImpl::RegisterSchemes(), and I'm wondering if we need to do either of the following?
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated
WebSecurityPolicy::RegisterURLSchemeAsNotAllowingJavascriptURLsOh, good find! I think RegisterURLSchemeAsDisplayIsolated makes sense here - I think this prevents a regular web page from embedding chrome-error:// in an iframe or image. Previously that was allowed and in fact opened the external protocol handler. I added this and also added some test coverage for it to NavigationToErrorURLIsDisallowed. I'll also see what trybots think about it.
I agree. If we can lock this down, that seems like a win.
Hmm, any idea why the try jobs are failing? I would hope we could include this restriction on iframes, etc.
> I'm not certain about RegisterURLSchemeAsNotAllowingJavascriptURLs. This "protects privileged pages against bookmarklets and other javascript manipulations," but since chrome-error:// URLs aren't really privileged, we probably don't need this? (It's also plausible that some existing bookmarklets might run on errors, and this would break them.)I'd recommend erring on the side of caution, and blocking bookmarklets from executing on error pages. I think we generally do a good job of not introducing interesting features on these pages (the most powerful thing I can think of is opting-into extended error reporting on interstitials, and I can imagine moving those to `chrome-error:`?), but if we consider them part of Chrome, we should prevent them from being modified.
I agree. It's probably not critical to prevent JS URLs, but it's more consistent with UI surfaces and probably worth locking down. Let's do it.
Otherwise, looks good once we get the try bots happy.
Alex Moshchuk (Gerrit)
Patch Set 14:
Patch Set 14:
Patch Set 13:
(1 comment)
Patch Set 13:
(4 comments)
Thanks-- almost set, but one more question. There's another set of registrations in RenderThreadImpl::RegisterSchemes(), and I'm wondering if we need to do either of the following?
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated
WebSecurityPolicy::RegisterURLSchemeAsNotAllowingJavascriptURLsOh, good find! I think RegisterURLSchemeAsDisplayIsolated makes sense here - I think this prevents a regular web page from embedding chrome-error:// in an iframe or image. Previously that was allowed and in fact opened the external protocol handler. I added this and also added some test coverage for it to NavigationToErrorURLIsDisallowed. I'll also see what trybots think about it.
I agree. If we can lock this down, that seems like a win.
Hmm, any idea why the try jobs are failing? I would hope we could include this restriction on iframes, etc.
So the problem is with the two tests that try to do location.href="#" while on an error page. This is now failing with a "Not allowed to load local resource", because SecurityOrigin::CanDisplay() now returns false for "chrome-error://chromewebdata/#" while at chrome-error://chromewebdata/. :( This is because we've made chrome-error:// a unique origin, so the protocol_ in CanDisplay is empty, and it fails the protocol check after getting into the ShouldTreatURLSchemeAsDisplayIsolated clause.
I guess some options are:
1. Assume we can't do same-document navigations on error pages anymore and fix these tests.
2. Don't use a unique origin for chrome-error:// and remove it from no_access_schemes.
3. Don't use ShouldTreatURLSchemeAsDisplayIsolated.
WDYT? 1 will prevent error pages from doing fragment navigations, which seems risky given https://crbug.com/557541 and https://codereview.chromium.org/1516623002, which suggest that existing error pages might actually need to do this.
I'm not certain about RegisterURLSchemeAsNotAllowingJavascriptURLs. This "protects privileged pages against bookmarklets and other javascript manipulations," but since chrome-error:// URLs aren't really privileged, we probably don't need this? (It's also plausible that some existing bookmarklets might run on errors, and this would break them.)
I'd recommend erring on the side of caution, and blocking bookmarklets from executing on error pages. I think we generally do a good job of not introducing interesting features on these pages (the most powerful thing I can think of is opting-into extended error reporting on interstitials, and I can imagine moving those to `chrome-error:`?), but if we consider them part of Chrome, we should prevent them from being modified.
I agree. It's probably not critical to prevent JS URLs, but it's more consistent with UI surfaces and probably worth locking down. Let's do it.
Done!
Charlie Reis (Gerrit)
Patch Set 15:
Patch Set 14:
Patch Set 14:
Patch Set 13:
(1 comment)
Patch Set 13:
(4 comments)
Thanks-- almost set, but one more question. There's another set of registrations in RenderThreadImpl::RegisterSchemes(), and I'm wondering if we need to do either of the following?
WebSecurityPolicy::RegisterURLSchemeAsDisplayIsolated
WebSecurityPolicy::RegisterURLSchemeAsNotAllowingJavascriptURLsOh, good find! I think RegisterURLSchemeAsDisplayIsolated makes sense here - I think this prevents a regular web page from embedding chrome-error:// in an iframe or image. Previously that was allowed and in fact opened the external protocol handler. I added this and also added some test coverage for it to NavigationToErrorURLIsDisallowed. I'll also see what trybots think about it.
I agree. If we can lock this down, that seems like a win.
Hmm, any idea why the try jobs are failing? I would hope we could include this restriction on iframes, etc.
So the problem is with the two tests that try to do location.href="#" while on an error page. This is now failing with a "Not allowed to load local resource", because SecurityOrigin::CanDisplay() now returns false for "chrome-error://chromewebdata/#" while at chrome-error://chromewebdata/. :( This is because we've made chrome-error:// a unique origin, so the protocol_ in CanDisplay is empty, and it fails the protocol check after getting into the ShouldTreatURLSchemeAsDisplayIsolated clause.
I guess some options are:
1. Assume we can't do same-document navigations on error pages anymore and fix these tests.
2. Don't use a unique origin for chrome-error:// and remove it from no_access_schemes.
3. Don't use ShouldTreatURLSchemeAsDisplayIsolated.WDYT? 1 will prevent error pages from doing fragment navigations, which seems risky given https://crbug.com/557541 and https://codereview.chromium.org/1516623002, which suggest that existing error pages might actually need to do this.
This is a tough call. I don't know of the case that error pages need to do same-document navigations, but I can see that that bug seems to imply it's needed. I'm super hesitant to drop the unique origin aspect (since we have a unique origin for them today with data URLs). That sounds like we can't mark it as display isolated?
Would there be some other way to prevent web pages from iframing error page URLs directly? (If not, maybe we have to live with that.)
> > > I'm not certain about RegisterURLSchemeAsNotAllowingJavascriptURLs. This "protects privileged pages against bookmarklets and other javascript manipulations," but since chrome-error:// URLs aren't really privileged, we probably don't need this? (It's also plausible that some existing bookmarklets might run on errors, and this would break them.)
> >
> > I'd recommend erring on the side of caution, and blocking bookmarklets from executing on error pages. I think we generally do a good job of not introducing interesting features on these pages (the most powerful thing I can think of is opting-into extended error reporting on interstitials, and I can imagine moving those to `chrome-error:`?), but if we consider them part of Chrome, we should prevent them from being modified.
>
> I agree. It's probably not critical to prevent JS URLs, but it's more consistent with UI surfaces and probably worth locking down. Let's do it.Done!
To view, visit change 580169. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
I guess some options are:
1. Assume we can't do same-document navigations on error pages anymore and fix these tests.
2. Don't use a unique origin for chrome-error:// and remove it from no_access_schemes.
3. Don't use ShouldTreatURLSchemeAsDisplayIsolated.WDYT? 1 will prevent error pages from doing fragment navigations, which seems risky given https://crbug.com/557541 and https://codereview.chromium.org/1516623002, which suggest that existing error pages might actually need to do this.
I'd suggest that we try #1. The comments on https://bugs.chromium.org/p/chromium/issues/detail?id=557541#c14 seem to indicate that same-page fragment navigation is unexpected, and since we've revamped our error page style since then, it's worth trying to lock things down even with this restriction as a side-effect.
#2 is something I'd like to avoid doing without a lot more thought about what it would mean for every error page to have the same origin.
#3 is an option, but if we can get away with #1, I'd prefer it. If we ship #1 and it explodes, #3 seems like a reasonable outcome.
Alex Moshchuk (Gerrit)
Alex Moshchuk uploaded patch set #18 to this change.
Change error pages to use a new chrome-error:// scheme.
This CL changes error pages to use chrome-error://chromewebdata/
instead of data:text/html,chromewebdata.
This has the benefit that error pages won't inherit CSP from their
parent/opener. Previously, when a page defined a CSP and then ended
up loading an error page in a subframe, the error page might've been
broken by the parent's CSP, since, for example, it needs to execute
inline scripts. This could result in the error page not showing up
correctly and/or false CSP reports being sent.
The new scheme is marked as secure and as requiring an opaque origin
to match previous behavior.
Web pages used to be able to directly load the error URL, which just
showed up as "chromewebdata". With this change, navigating to the
error URL would bring up the external protocol dialog instead, so this
CL prevents renderers from directly navigating to or redirecting to
error URLs.
Additionally, chrome-error:// is registered as a display-isolated
scheme, so that regular web pages can't embed the error URL in an
iframe or image, and as a scheme that does not allow javascript URL
manipulation, which is consistent with other pages considered to be
part of Chrome. If either of these new restrictions ends up being
problematic, we should revisit them in
RenderThreadImpl::RegisterSchemes().
In the future, it's possible to further utilize the host/path portion
of the URL to identify different kinds of error pages.
Bug: 703801
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I45fa0aa157523450c33c6464f96414ff742e8604
---
M chrome/browser/chrome_navigation_browsertest.cc
M chrome/browser/extensions/window_open_apitest.cc
M chrome/browser/net/errorpage_browsertest.cc
M chrome/test/chromedriver/chrome/navigation_tracker.cc
M chrome/test/chromedriver/test/run_py_tests.py
M chrome/test/chromedriver/window_commands.cc
A chrome/test/data/empty_with_404.html
A chrome/test/data/empty_with_404.html.mock-http-headers
A chrome/test/data/page_with_csp_and_error_iframe.html
M content/browser/child_process_security_policy_impl.cc
M content/browser/child_process_security_policy_unittest.cc
M content/browser/frame_host/render_frame_host_impl.cc
M content/common/url_schemes.cc
M content/public/common/url_constants.cc
M content/public/common/url_constants.h
M content/renderer/render_thread_impl.cc
M third_party/WebKit/LayoutTests/inspector-protocol/page/frameNavigatedToUnreachableUrl-expected.txt
M third_party/WebKit/LayoutTests/inspector/console/console-context-selector-expected.txt
M third_party/WebKit/Source/core/exported/WebFrameTest.cpp
19 files changed, 182 insertions(+), 28 deletions(-)
To view, visit change 580169. To unsubscribe, or for help writing mail filters, visit settings.
Alex Moshchuk (Gerrit)
Patch Set 15:
I guess some options are:
1. Assume we can't do same-document navigations on error pages anymore and fix these tests.
2. Don't use a unique origin for chrome-error:// and remove it from no_access_schemes.
3. Don't use ShouldTreatURLSchemeAsDisplayIsolated.WDYT? 1 will prevent error pages from doing fragment navigations, which seems risky given https://crbug.com/557541 and https://codereview.chromium.org/1516623002, which suggest that existing error pages might actually need to do this.
I'd suggest that we try #1. The comments on https://bugs.chromium.org/p/chromium/issues/detail?id=557541#c14 seem to indicate that same-page fragment navigation is unexpected, and since we've revamped our error page style since then, it's worth trying to lock things down even with this restriction as a side-effect.
#2 is something I'd like to avoid doing without a lot more thought about what it would mean for every error page to have the same origin.
#3 is an option, but if we can get away with #1, I'd prefer it. If we ship #1 and it explodes, #3 seems like a reasonable outcome.
Thanks for the feedback! I agree that #1 is most desirable, so let's try it and fall back to #3 if we end up breaking any error page behavior. I looked at the net error code a bit more and couldn't find any use of same-document navigations.
The errorpage test fixes are pretty easy -- just a modification to the way the test waits for the hash navigation: previously, it relied on TestNavigationObserver catching DidFinishLoad, which doesn't happen anymore. Instead of this, I just use content::WaitForLoadStop, which should return right away in the expected case where there's no navigation to wait for. The auto-reload behavior, etc that these tests are checking doesn't change, as I think this ignored fragment navigations even when they were allowed.
+mmenke: are you ok with these changes to errorpage_browsertest.cc? Do you know any other context about fragment navigations on error pages, other than what's in https://crbug.com/557541?
Matt Menke (Gerrit)
Patch Set 15:
I guess some options are:
1. Assume we can't do same-document navigations on error pages anymore and fix these tests.
2. Don't use a unique origin for chrome-error:// and remove it from no_access_schemes.
3. Don't use ShouldTreatURLSchemeAsDisplayIsolated.WDYT? 1 will prevent error pages from doing fragment navigations, which seems risky given https://crbug.com/557541 and https://codereview.chromium.org/1516623002, which suggest that existing error pages might actually need to do this.
I'd suggest that we try #1. The comments on https://bugs.chromium.org/p/chromium/issues/detail?id=557541#c14 seem to indicate that same-page fragment navigation is unexpected, and since we've revamped our error page style since then, it's worth trying to lock things down even with this restriction as a side-effect.
#2 is something I'd like to avoid doing without a lot more thought about what it would mean for every error page to have the same origin.
#3 is an option, but if we can get away with #1, I'd prefer it. If we ship #1 and it explodes, #3 seems like a reasonable outcome.
We've run into crashes before because some things are apparently doing same page navigations with error pages. I'm not sure what things are, but at one point, the error page had a racy crash bug in those cases, and we did indeed get crash reports about it (Though no user reports about the crashes).
Nasko Oskov (Gerrit)
LGTM!
Patch set 18:Code-Review +1
2 comments:
File chrome/browser/chrome_navigation_browsertest.cc:
Patch Set #18, Line 452:
chrome/minor nit: s/chrome\//at this layer/ to avoid duplication of "chrome/".
File content/browser/frame_host/render_frame_host_impl.cc:
Patch Set #11, Line 2289:
GetProcess()->FilterURL(false, &validated_params.url);Yeah, given that the renderer doesn't block pages from doing it, we probably shouldn't kill the rend […]
Ack
To view, visit change 580169. To unsubscribe, or for help writing mail filters, visit settings.
Charlie Reis (Gerrit)
Sounds like there's a chance #1 might break things, but we don't have examples and we're not sure. If you're willing to keep an eye out for any bugs that arise, I'm ok with giving it a try, since it's certainly the most desirable option for proceeding. LGTM if you want to give that a try.
To view, visit change 580169. To unsubscribe, or for help writing mail filters, visit settings.
Alex Moshchuk (Gerrit)
Thanks everyone for the reviews! Adding OWNERS:
rdevlin.cronin@: for chrome/browser/extensions/window_open_apitest.cc
mmenke@: for errorpage_browsertest.cc
johnchen@: for chrome/test/chromedriver/
Sounds like there's a chance #1 might break things, but we don't have examples and we're not sure. If you're willing to keep an eye out for any bugs that arise, I'm ok with giving it a try, since it's certainly the most desirable option for proceeding. LGTM if you want to give that a try.
Yes, I vote for giving #1 a try. I'll watch out for any bugs and fall back to #3 if needed (which should be easy).
John Chen (Gerrit)
ChromeDriver changes LGTM.
Shuotao: Could you PTAL? Thanks.
Devlin (Gerrit)
extensions lgtm!
To view, visit change 580169. To unsubscribe, or for help writing mail filters, visit settings.
John Chen (Gerrit)
Just thought of an issue: ChromeDriver needs to be backward compatible with older versions of Chrome, so in the two ChromeDriver C++ files, it is necessary to compare the URL against both the old and new string.
Shuotao Gao (Gerrit)
ChromeDriver LGTM
To view, visit change 580169. To unsubscribe, or for help writing mail filters, visit settings.
Alex Moshchuk (Gerrit)
Patch Set 18:
Just thought of an issue: ChromeDriver needs to be backward compatible with older versions of Chrome, so in the two ChromeDriver C++ files, it is necessary to compare the URL against both the old and new string.
Thanks for the feedback. I've changed ChromeDriver to track both URLs in PS19 - please take a look.
Matt Menke (Gerrit)
Patch set 19:Code-Review +1
John Chen (Gerrit)
ChromeDriver changes LGTM. Thanks!
Alex Moshchuk (Gerrit)
Patch set 19:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
Reviewed-on: https://chromium-review.googlesource.com/580169
Reviewed-by: Matt Menke <mme...@chromium.org>
Reviewed-by: Nasko Oskov <na...@chromium.org>
Reviewed-by: Charlie Reis (OOO Aug 17-24) <cr...@chromium.org>
Reviewed-by: Devlin <rdevlin...@chromium.org>
Reviewed-by: Shuotao Gao <st...@chromium.org>
Reviewed-by: Mike West <mk...@chromium.org>
Commit-Queue: Alex Moshchuk <ale...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494809}
---
M chrome/browser/chrome_navigation_browsertest.cc
M chrome/browser/extensions/window_open_apitest.cc
M chrome/browser/net/errorpage_browsertest.cc
M chrome/test/chromedriver/chrome/navigation_tracker.cc
M chrome/test/chromedriver/test/run_py_tests.py
M chrome/test/chromedriver/window_commands.cc
A chrome/test/data/empty_with_404.html
A chrome/test/data/empty_with_404.html.mock-http-headers
A chrome/test/data/page_with_csp_and_error_iframe.html
M content/browser/child_process_security_policy_impl.cc
M content/browser/child_process_security_policy_unittest.cc
M content/browser/frame_host/render_frame_host_impl.cc
M content/common/url_schemes.cc
M content/public/common/url_constants.cc
M content/public/common/url_constants.h
M content/renderer/render_thread_impl.cc
M third_party/WebKit/LayoutTests/inspector-protocol/page/frameNavigatedToUnreachableUrl-expected.txt
M third_party/WebKit/LayoutTests/inspector/console/console-context-selector-expected.txt
M third_party/WebKit/Source/core/exported/WebFrameTest.cpp
19 files changed, 198 insertions(+), 30 deletions(-)
Jules Acer
> Alex Moshchuk uploaded patch set #6 to this change.
> View ChangeChange error pages to use a new chrome-error:// scheme.
>
> This CL changes error pages to use chrome-error://chromewebdata/ instead
> of data:text/html,chromewebdata.
>
> This has the benefit that error pages won't inherit CSP from their
> loading an error page in a subframe, the error page might've been
> inline scripts. This could result in the error page not showing up
>
> Bug: 703801
> Change-Id: I45fa0aa157523450c33c6464f96414ff742e8604
> ---
> M chrome/browser/extensions/window_open_apitest.cc
> M chrome/test/chromedriver/chrome/navigation_tracker.cc
> M chrome/test/chromedriver/test/run_py_tests.py
> M chrome/test/chromedriver/window_commands.cc
> M content/public/common/url_constants.cc
> M content/public/common/url_constants.h
> M third_party/WebKit/LayoutTests/inspector/console/console-context-selector-expected.txt
> M third_party/WebKit/Source/web/tests/WebFrameTest.cpp
> 10 files changed, 22 insertions(+), 18 deletions(-)
>
>
> To view, visit change 580169. To unsubscribe, visit settings.
>
>
>
>
> Gerrit-Project: chromium/src
>
> Gerrit-Branch: master
>
> Gerrit-MessageType: newpatchset
>
> Gerrit-Change-Id: I45fa0aa157523450c33c6464f96414ff742e8604
>
> Gerrit-Change-Number: 580169
>
> Gerrit-PatchSet: 6
> Gerrit-Owner: Alex Moshchuk <ale...@chromium.org>
>
> Gerrit-Reviewer: Alex Moshchuk <ale...@chromium.org>
>
>
> Gerrit-CC: John Abd-El-Malek <j...@chromium.org>
>
> Gerrit-CC: Nasko Oskov <na...@chromium.org>
>
> Gerrit-CC: chromium...@chromium.org
>
> Gerrit-CC: devtools...@chromium.org
