|
|
DescriptionChange NavigationEntry to use virtual URL in error pages for blocked navigations
Error pages today commit with the original URL in the NavigationEntry. This means
that on history navigations/reload, the original URL will be used. However, when
navigations are blocked, we shouldn't be allowing the blocked URL to be set as
the NavigationEntry URL. There has been a long standing desire to change this
aspect of error pages and this CL is a small step in that direction.
The goal of this CL is to classify error pages due to blocked navigations and
change the URL of the NavigationEntry to be a safe one - "about:blank". To
preserve the existing UX, though, the originally blocked URL is set as the
virtual URL on the NavigationEntry. This allows history navigations/reload to
end up at about:blank, which is much safer behavior than allowing the load
to the blocked URL.
BUG=723796
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2889003002
Cr-Commit-Position: refs/heads/master@{#474535}
Committed: https://chromium.googlesource.com/chromium/src/+/5ff4eeaefbc41f51d02887e0c42bb6e72ba292a0
Patch Set 1 #Patch Set 2 : Speculative implementation of blocked navigations. #Patch Set 3 : Fix failing PlzNavigate test, added more error codes. #Patch Set 4 : Add a test for reload. #
Total comments: 22
Patch Set 5 : Address review comments. #Patch Set 6 : Remove some error codes and add a TODO. #
Total comments: 1
Patch Set 7 : Remove reload test since it isn't accessible cross-origin. #
Total comments: 3
Patch Set 8 : Address review comments. #Patch Set 9 : Add back reload test (PlzNavigate only) and address review comments. #
Total comments: 2
Patch Set 10 : Rebase. #Patch Set 11 : Improve the extensions check. #Patch Set 12 : rebase #
Messages
Total messages: 54 (37 generated)
Description was changed from ========== Disallow data: URL navigations in top frame. BUG=723796 ========== to ========== Disallow data: URL navigations in top frame. BUG=723796 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Disallow data: URL navigations in top frame. BUG=723796 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== 01234567890123456789012345678901234567890123456789012345678901234567890123456789 Change NavigationEntry to use virtual URL in error pages for blocked navigations Error pages today commit with the original URL in the NavigationEntry. This means that on history navigations/reload, the original URL will be used. However, when navigations are blocked, we shouldn't be allowing the blocked URL to be set as the NavigationEntry URL. There has been a long standing desire to change this aspect of error pages and this CL is a small step in that direction. The goal of this CL is to classify error pages due to blocked navigations and change the URL of the NavigationEntry to be a safe one - "about:blank". To preserve the existing UX, though, the originally blocked URL is set as the virtual URL on the NavigationEntry. This allows history navigations/reload to end up at about:blank, which is much safer behavior than allowing the load to the blocked URL. BUG=723796 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== 01234567890123456789012345678901234567890123456789012345678901234567890123456789 Change NavigationEntry to use virtual URL in error pages for blocked navigations Error pages today commit with the original URL in the NavigationEntry. This means that on history navigations/reload, the original URL will be used. However, when navigations are blocked, we shouldn't be allowing the blocked URL to be set as the NavigationEntry URL. There has been a long standing desire to change this aspect of error pages and this CL is a small step in that direction. The goal of this CL is to classify error pages due to blocked navigations and change the URL of the NavigationEntry to be a safe one - "about:blank". To preserve the existing UX, though, the originally blocked URL is set as the virtual URL on the NavigationEntry. This allows history navigations/reload to end up at about:blank, which is much safer behavior than allowing the load to the blocked URL. BUG=723796 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Change NavigationEntry to use virtual URL in error pages for blocked navigations Error pages today commit with the original URL in the NavigationEntry. This means that on history navigations/reload, the original URL will be used. However, when navigations are blocked, we shouldn't be allowing the blocked URL to be set as the NavigationEntry URL. There has been a long standing desire to change this aspect of error pages and this CL is a small step in that direction. The goal of this CL is to classify error pages due to blocked navigations and change the URL of the NavigationEntry to be a safe one - "about:blank". To preserve the existing UX, though, the originally blocked URL is set as the virtual URL on the NavigationEntry. This allows history navigations/reload to end up at about:blank, which is much safer behavior than allowing the load to the blocked URL. BUG=723796 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
nasko@chromium.org changed reviewers: + creis@chromium.org
Hey Charlie, Can you review this CL for me? It implements the solution we discussed offline to ensure we don't use the blocked URL on NavigationEntry, but about:blank. Thanks in advance! Nasko
Thanks! I'm very excited to get better protection against blocked navigations. A few thoughts and questions below. https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... File content/browser/frame_host/data_url_navigation_browsertest.cc (right): https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/data_url_navigation_browsertest.cc:960: // The window.open() should have resulted in an error page. nit: Add "The blocked URL should be in the virtual URL, not the actual URL." https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:166: switch (error_code) { I'm nervous about hand picking these out of net_error_list.h. Feels easy to miss things (especially if error codes get added later, like ERR_BLOCKED_BY_XSS_AUDITOR) or go too far (as we found with ERR_INVALID_URL causing test failures). Can we get someone from the network team to offer a suggestion? (Is blocked vs failed even a well-defined distinction between errors? I hope so.) https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:174: case net::ERR_NETWORK_ACCESS_DENIED: // Does this mean blocked? I would probably remove this. Network access denied doesn't sound like the URL was the problem. If network access was later granted, I would expect these URLs to keep working. In general, we should only be doing this change for cases that are definitely dangerous. Otherwise it's a functional regression if some temporary error prevents you from getting back to the pages you had open before. https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:175: case net::ERR_INVALID_URL: Should we remove this to get the test to pass, or investigate why the test is hitting it? https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:908: return false; If we later extend this fix to subframes, we'll need to be careful about this case, which happens on the first navigation of a subframe and doesn't reach the code below. https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:961: // PageState with a new one. Sounds good. Slight rephrase: ... To avoid loading the blocked URL on back/forward/reload navigations, do not store it in the FrameNavigationEntry's URL or PageState. Instead, make it visible to the user as the virtual URL. Store a safe URL (about:blank) as the one to load if the entry is revisited. https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:964: DCHECK(params.url_is_unreachable); Heh, I have some mixed feelings here. We generally don't want a renderer IPC to crash the browser, but in a debug build it's probably reasonable to catch bugs this way. I'm mainly curious if this is going to get hit in practice-- hopefully not, but I'm not sure how close the code for these lines up. Let's keep it and see what happens. https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:967: if (frame_entry) nit: Needs braces. https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6834: VerifyBlockedErrorPageURL_History) { Just curious, how is this different from the earlier test?
https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... File content/browser/frame_host/data_url_navigation_browsertest.cc (right): https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/data_url_navigation_browsertest.cc:960: // The window.open() should have resulted in an error page. On 2017/05/18 22:41:46, Charlie Reis wrote: > nit: Add "The blocked URL should be in the virtual URL, not the actual URL." Done. https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:166: switch (error_code) { On 2017/05/18 22:41:46, Charlie Reis wrote: > I'm nervous about hand picking these out of net_error_list.h. Feels easy to > miss things (especially if error codes get added later, like > ERR_BLOCKED_BY_XSS_AUDITOR) or go too far (as we found with ERR_INVALID_URL > causing test failures). > > Can we get someone from the network team to offer a suggestion? (Is blocked vs > failed even a well-defined distinction between errors? I hope so.) I don't think that such a distinction exists actually, but definitely will ping someone from the network team to discuss it with. The goal of this was to start with something and drive discussion forward :). https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:174: case net::ERR_NETWORK_ACCESS_DENIED: // Does this mean blocked? On 2017/05/18 22:41:46, Charlie Reis wrote: > I would probably remove this. Network access denied doesn't sound like the URL > was the problem. If network access was later granted, I would expect these URLs > to keep working. > > In general, we should only be doing this change for cases that are definitely > dangerous. Otherwise it's a functional regression if some temporary error > prevents you from getting back to the pages you had open before. My goal was to pick the errors that aren't transient, but will always result in the same error. This specific error was on the fence, hence the comment. Removed. https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:175: case net::ERR_INVALID_URL: On 2017/05/18 22:41:46, Charlie Reis wrote: > Should we remove this to get the test to pass, or investigate why the test is > hitting it? I don't think the test is doing what it is expecting to do. Let's discuss what the goals of the test are and we can decide then. I think the failure while related to this change, might be a bit unrelated. https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:908: return false; On 2017/05/18 22:41:46, Charlie Reis wrote: > If we later extend this fix to subframes, we'll need to be careful about this > case, which happens on the first navigation of a subframe and doesn't reach the > code below. Noted! Should we add a TODO/Note? https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:961: // PageState with a new one. On 2017/05/18 22:41:46, Charlie Reis wrote: > Sounds good. Slight rephrase: > > ... To avoid loading the blocked URL on back/forward/reload navigations, do not > store it in the FrameNavigationEntry's URL or PageState. Instead, make it > visible to the user as the virtual URL. Store a safe URL (about:blank) as the > one to load if the entry is revisited. Done. https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:964: DCHECK(params.url_is_unreachable); On 2017/05/18 22:41:46, Charlie Reis wrote: > Heh, I have some mixed feelings here. > > We generally don't want a renderer IPC to crash the browser, but in a debug > build it's probably reasonable to catch bugs this way. > > I'm mainly curious if this is going to get hit in practice-- hopefully not, but > I'm not sure how close the code for these lines up. > > Let's keep it and see what happens. I wanted it mainly for discovering potential issues in test coverage that we have. I'm ok removing it, but we also should not be shipping DCHECKs to users :). https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:967: if (frame_entry) On 2017/05/18 22:41:46, Charlie Reis wrote: > nit: Needs braces. Done. https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6834: VerifyBlockedErrorPageURL_History) { On 2017/05/18 22:41:46, Charlie Reis wrote: > Just curious, how is this different from the earlier test? The goal is to use a navigation different than the redirect-to-data-url one, so we get more generic coverage of "blocked navigations" behavior. I will look around for other such cases once we have better idea of which error codes will be considered "blocked".
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Great. Next step is investigating the test and chatting with net folks. If we keep the switch statement, let's be sure to include a comment with what criteria we used to pick those values. https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:175: case net::ERR_INVALID_URL: On 2017/05/18 22:55:11, nasko wrote: > On 2017/05/18 22:41:46, Charlie Reis wrote: > > Should we remove this to get the test to pass, or investigate why the test is > > hitting it? > > I don't think the test is doing what it is expecting to do. Let's discuss what > the goals of the test are and we can decide then. I think the failure while > related to this change, might be a bit unrelated. Agreed-- let's look more closely at it tomorrow. https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:908: return false; On 2017/05/18 22:55:11, nasko wrote: > On 2017/05/18 22:41:46, Charlie Reis wrote: > > If we later extend this fix to subframes, we'll need to be careful about this > > case, which happens on the first navigation of a subframe and doesn't reach > the > > code below. > > Noted! Should we add a TODO/Note? Yes, maybe down on the comment you're adding for the block below, about doing something similar for subframe navigations (including AUTO_SUBFRAME). https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6834: VerifyBlockedErrorPageURL_History) { On 2017/05/18 22:55:11, nasko wrote: > On 2017/05/18 22:41:46, Charlie Reis wrote: > > Just curious, how is this different from the earlier test? > > The goal is to use a navigation different than the redirect-to-data-url one, so > we get more generic coverage of "blocked navigations" behavior. I will look > around for other such cases once we have better idea of which error codes will > be considered "blocked". Acknowledged.
The CQ bit was checked by creis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2889003002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:908: return false; On 2017/05/18 23:15:51, Charlie Reis wrote: > On 2017/05/18 22:55:11, nasko wrote: > > On 2017/05/18 22:41:46, Charlie Reis wrote: > > > If we later extend this fix to subframes, we'll need to be careful about > this > > > case, which happens on the first navigation of a subframe and doesn't reach > > the > > > code below. > > > > Noted! Should we add a TODO/Note? > > Yes, maybe down on the comment you're adding for the block below, about doing > something similar for subframe navigations (including AUTO_SUBFRAME). Done.
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hey Charlie, Can you review this CL for sign off? It should be ready to go. Thanks in advance! Nasko
Great! Basically looks good, but I have a concern about the removed reload test. If we aren't preventing the load on reloads, we should update the CL description and comments. (Is it safe to ignore that case?) https://codereview.chromium.org/2889003002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl_browsertest.cc (right): https://codereview.chromium.org/2889003002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl_browsertest.cc:6876: VerifyBlockedErrorPageURL_Reload) { Why did you need to remove this test? I thought we wanted to continue blocking on reload. https://codereview.chromium.org/2889003002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2889003002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:165: bool IsBlockedNavigation(net::Error error_code) { Can we include a comment here with some context? Something about how this is an imperfect way to infer whether a navigation was blocked or not, and these error codes are just the ones that are most clearly blocked cases? We could probably include a TODO to more formally identify this as well. https://codereview.chromium.org/2889003002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:954: // navigatios, including AUTO_SUBFRAME. nit: navigations
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Added the reload test in PlzNavigate mode only, as reloads there behave much more sanely. https://codereview.chromium.org/2889003002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2889003002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_controller_impl.cc:165: bool IsBlockedNavigation(net::Error error_code) { On 2017/05/24 16:42:11, Charlie Reis (overloaded) wrote: > Can we include a comment here with some context? Something about how this is an > imperfect way to infer whether a navigation was blocked or not, and these error > codes are just the ones that are most clearly blocked cases? We could probably > include a TODO to more formally identify this as well. Done.
nasko@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Hey Devlin, Would you mind stamping chrome/browser/extensions/navigation_observer.cc for OWNERS, since we discussed this fix offline? Thanks in advance! Nasko
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
chrome/browser/extensions/navigation_observer.cc lgtm!
The extension change makes me wonder whether other code will break when we alter GetURL() as well... I think I'm ok with that risk, though, given that the tests are green and the value that we'll get from this. This LGTM once we adjust the code below. https://codereview.chromium.org/2889003002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/navigation_observer.cc (right): https://codereview.chromium.org/2889003002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/navigation_observer.cc:76: const GURL& url = nav_entry->GetVirtualURL().is_empty() This condition makes me nervous. If GetURL is an extension URL and GetVirtualURL is not, then we're potentially breaking the code below. I know most component extensions won't be disabled, but I'm not 100% sure that no extension that can be disabled has a non-extension virtual URL. Maybe we should only use the virtual URL (instead of GetURL) if the virtual URL is an extension and GetURL() is about:blank, to limit to this case? And/or your idea of checking if nav_entry has an error in GetPageType?
Patchset #11 (id:200001) has been deleted
The CQ bit was checked by nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2889003002/#ps220001 (title: "Improve the extensions check.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2889003002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/navigation_observer.cc (right): https://codereview.chromium.org/2889003002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/navigation_observer.cc:76: const GURL& url = nav_entry->GetVirtualURL().is_empty() On 2017/05/24 21:50:52, Charlie Reis (overloaded) wrote: > This condition makes me nervous. If GetURL is an extension URL and > GetVirtualURL is not, then we're potentially breaking the code below. I know > most component extensions won't be disabled, but I'm not 100% sure that no > extension that can be disabled has a non-extension virtual URL. > > Maybe we should only use the virtual URL (instead of GetURL) if the virtual URL > is an extension and GetURL() is about:blank, to limit to this case? > > And/or your idea of checking if nav_entry has an error in GetPageType? Fixed by combining all 3 checks.
Thanks, LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for content/browser/browser_side_navigation_browsertest.cc: While running git apply --index -3 -p1; error: patch failed: content/browser/browser_side_navigation_browsertest.cc:322 Falling back to three-way merge... Applied patch to 'content/browser/browser_side_navigation_browsertest.cc' with conflicts. U content/browser/browser_side_navigation_browsertest.cc Patch: content/browser/browser_side_navigation_browsertest.cc Index: content/browser/browser_side_navigation_browsertest.cc diff --git a/content/browser/browser_side_navigation_browsertest.cc b/content/browser/browser_side_navigation_browsertest.cc index dbe2ca69b8b20d05cb6203e9696a7703f0ea9f96..5e493112f75db3e8fe17449abad592e71c1508ec 100644 --- a/content/browser/browser_side_navigation_browsertest.cc +++ b/content/browser/browser_side_navigation_browsertest.cc @@ -322,4 +322,45 @@ IN_PROC_BROWSER_TEST_F(BrowserSideNavigationBrowserTest, SanitizeReferrer) { EXPECT_EQ(kInsecureUrl, shell()->web_contents()->GetLastCommittedURL()); } +// Test case to verify that redirects to data: URLs are properly disallowed, +// even when invoked through a reload. +// See https://crbug.com/723796. +// +// Note: This is PlzNavigate specific test, as the behavior of reloads in the +// non-PlzNavigate path differs. The WebURLRequest for the reload is generated +// based on Blink's state instead of the history state in the browser process, +// which ends up loading the originally blocked URL. With PlzNavigate, the +// reload uses the NavigationEntry state to create a navigation and commit it. +IN_PROC_BROWSER_TEST_F(BrowserSideNavigationBrowserTest, + VerifyBlockedErrorPageURL_Reload) { + NavigationControllerImpl& controller = static_cast<NavigationControllerImpl&>( + shell()->web_contents()->GetController()); + + GURL start_url(embedded_test_server()->GetURL("/title1.html")); + EXPECT_TRUE(NavigateToURL(shell(), start_url)); + EXPECT_EQ(0, controller.GetLastCommittedEntryIndex()); + + // Navigate to an URL, which redirects to a data: URL, since it is an + // unsafe redirect and will result in a blocked navigation and error page. + GURL redirect_to_blank_url( + embedded_test_server()->GetURL("/server-redirect?data:text/html,Hello!")); + EXPECT_FALSE(NavigateToURL(shell(), redirect_to_blank_url)); + EXPECT_EQ(1, controller.GetLastCommittedEntryIndex()); + EXPECT_EQ(PAGE_TYPE_ERROR, controller.GetLastCommittedEntry()->GetPageType()); + + TestNavigationObserver reload_observer(shell()->web_contents()); + EXPECT_TRUE(ExecuteScript(shell(), "location.reload()")); + reload_observer.Wait(); + + // The expectation is that about:blank was loaded and the virtual URL is set + // to the URL that was blocked. + EXPECT_EQ(1, controller.GetLastCommittedEntryIndex()); + EXPECT_FALSE( + controller.GetLastCommittedEntry()->GetURL().SchemeIs(url::kDataScheme)); + EXPECT_TRUE(controller.GetLastCommittedEntry()->GetVirtualURL().SchemeIs( + url::kDataScheme)); + EXPECT_EQ(url::kAboutBlankURL, + controller.GetLastCommittedEntry()->GetURL().spec()); +} + } // namespace content
The CQ bit was checked by jam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2889003002/#ps240001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 240001, "attempt_start_ts": 1495673699608740, "parent_rev": "8921b24098caf2612b3e30694ce4250781c2cdca", "commit_rev": "5ff4eeaefbc41f51d02887e0c42bb6e72ba292a0"}
Message was sent while issue was closed.
Description was changed from ========== Change NavigationEntry to use virtual URL in error pages for blocked navigations Error pages today commit with the original URL in the NavigationEntry. This means that on history navigations/reload, the original URL will be used. However, when navigations are blocked, we shouldn't be allowing the blocked URL to be set as the NavigationEntry URL. There has been a long standing desire to change this aspect of error pages and this CL is a small step in that direction. The goal of this CL is to classify error pages due to blocked navigations and change the URL of the NavigationEntry to be a safe one - "about:blank". To preserve the existing UX, though, the originally blocked URL is set as the virtual URL on the NavigationEntry. This allows history navigations/reload to end up at about:blank, which is much safer behavior than allowing the load to the blocked URL. BUG=723796 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Change NavigationEntry to use virtual URL in error pages for blocked navigations Error pages today commit with the original URL in the NavigationEntry. This means that on history navigations/reload, the original URL will be used. However, when navigations are blocked, we shouldn't be allowing the blocked URL to be set as the NavigationEntry URL. There has been a long standing desire to change this aspect of error pages and this CL is a small step in that direction. The goal of this CL is to classify error pages due to blocked navigations and change the URL of the NavigationEntry to be a safe one - "about:blank". To preserve the existing UX, though, the originally blocked URL is set as the virtual URL on the NavigationEntry. This allows history navigations/reload to end up at about:blank, which is much safer behavior than allowing the load to the blocked URL. BUG=723796 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2889003002 Cr-Commit-Position: refs/heads/master@{#474535} Committed: https://chromium.googlesource.com/chromium/src/+/5ff4eeaefbc41f51d02887e0c42b... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001) as https://chromium.googlesource.com/chromium/src/+/5ff4eeaefbc41f51d02887e0c42b... |