|
|
Created:
4 years, 2 months ago by ananta Modified:
4 years, 1 month ago CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, Randy Smith (Not in Mondays), darin-cc_chromium.org, loading-reviews_chromium.org, mmenke Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: Fix the FindInPageControllerTest.SearchWithinSpecialURL browser test.
This test was initiating a navigation to the chrome\test\data folder and performing a
number of find requests on the page returned. With PlzNavigate the request to navigate
to chrome\test\data folder would get blocked in ResourceLoader::OnReceivedRedirect because
the process id (-1) did not have access to the url being navigated to. In the non PlzNavigate
case this is not an issue because the child process is given access to the URL during
navigation.
Proposed fix for PlzNavigate is to not do the ChildProcessSecurityPolicy check in
the ResourceLoader::OnReceivedRedirect function and instead do this check on the UI thread
in the NavigationRequest::OnRedirectChecksComplete function. Additionally we also grant
access to the URL in the NavigatorImpl::RequestNavigation function.
The above test is now enabled for Windows.
BUG=175711
Disabled due to crbug.com/175711
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/7bb43d67968cfd1f9d56e0dbd1ce1dbb2fefe46f
Cr-Commit-Position: refs/heads/master@{#427279}
Patch Set 1 #
Total comments: 8
Patch Set 2 : The URL security checks for PlzNavigate should be handled in ResourceLoader for non frame resources… #
Total comments: 9
Patch Set 3 : Update comments #Patch Set 4 : Don't check for URL access in NavigationRequest::OnRedirectChecksComplete() for browser initiated n… #Patch Set 5 : Revert changes to navigator_impl.cc #
Total comments: 4
Patch Set 6 : Set source_site_instance in cases where it is null and a source contents is available and use it fo… #Patch Set 7 : Perform access checks on the URL only for renderer initiated navigations. #Patch Set 8 : Revert changes to browser_navigator.cc #
Total comments: 3
Patch Set 9 : Use FilterURL to validate access from the renderer for a URL being redirected. #
Total comments: 6
Patch Set 10 : Address creis review comments #
Total comments: 2
Patch Set 11 : Address comments #
Messages
Total messages: 50 (27 generated)
Description was changed from ========== PlzNavigate: Fix the FindInPageControllerTest.SearchWithinSpecialURL browser test. This test was initiating a navigation to the chrome\test\data folder and performing a number of find requests on the page returned. With PlzNavigate the request to navigate to chrome\test\data folder would get blocked in ResourceLoader::OnReceivedRedirect because the process id (-1) did not have access to the url being navigated to. In the non PlzNavigate case this is not an issue because the child process is given access to the URL during navigation. Proposed fix for PlzNavigate is to not do the ChildProcessSecurityPolicy check in the ResourceLoader::OnReceivedRedirect function and instead do this check on the UI thread in the NavigationRequest::OnRedirectChecksComplete function. Additionally we also grant access to the URL in the NavigatorImpl::RequestNavigation function. The above test is now enabled for Windows. BUG=175711 Disabled due to crbug.com/175711 ========== to ========== PlzNavigate: Fix the FindInPageControllerTest.SearchWithinSpecialURL browser test. This test was initiating a navigation to the chrome\test\data folder and performing a number of find requests on the page returned. With PlzNavigate the request to navigate to chrome\test\data folder would get blocked in ResourceLoader::OnReceivedRedirect because the process id (-1) did not have access to the url being navigated to. In the non PlzNavigate case this is not an issue because the child process is given access to the URL during navigation. Proposed fix for PlzNavigate is to not do the ChildProcessSecurityPolicy check in the ResourceLoader::OnReceivedRedirect function and instead do this check on the UI thread in the NavigationRequest::OnRedirectChecksComplete function. Additionally we also grant access to the URL in the NavigatorImpl::RequestNavigation function. The above test is now enabled for Windows. BUG=175711 Disabled due to crbug.com/175711 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by ananta@chromium.org to run a CQ dry run
ananta@chromium.org changed reviewers: + jam@chromium.org
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: This issue passed the CQ dry run.
ananta@chromium.org changed reviewers: + nasko@chromium.org
+nasko
oops, sorry I forgot to hit publish :( https://codereview.chromium.org/2436253002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/1/content/browser/loader/reso... content/browser/loader/resource_loader.cc:262: if (!IsBrowserSideNavigationEnabled()) { NavigationRequest handles only frame navigations. for subresources, we still need to do this check. so this should be: if (!IsBrowserSideNavigationEnabled() || info->GetChildID() != -1 || !IsResourceTypeFrame(info->GetResourceType()))
https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:520: !ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL( it seems a bit odd to be checking process ID of speculative processes with PlzNavigate. https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:1141: ChildProcessSecurityPolicyImpl::GetInstance()->GrantRequestURL( GrantRequestURL should only be called for browser-initiated loads. if we call it here, that means that an exploited renderer would say a navigation occurred to a scheme it doesn't have access to and then this codepath would allow it.
https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:520: !ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL( On 2016/10/21 20:28:20, jam wrote: > it seems a bit odd to be checking process ID of speculative processes with > PlzNavigate. > nasko suggested using the siteinstance id for this. The GrantRequestURL function validates the id passed in with the expectation that the id would be registered. https://cs.chromium.org/chromium/src/content/browser/child_process_security_p.... https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:1141: ChildProcessSecurityPolicyImpl::GetInstance()->GrantRequestURL( On 2016/10/21 20:28:21, jam wrote: > GrantRequestURL should only be called for browser-initiated loads. if we call it > here, that means that an exploited renderer would say a navigation occurred to a > scheme it doesn't have access to and then this codepath would allow it. This function appears to be only called for top level navigations. These include navigations originating in the browser and those via the RenderFrameImpl::OpenURL code path. nasko please confirm if this is the case.
https://codereview.chromium.org/2436253002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/1/content/browser/loader/reso... content/browser/loader/resource_loader.cc:262: if (!IsBrowserSideNavigationEnabled()) { On 2016/10/21 20:22:38, jam wrote: > NavigationRequest handles only frame navigations. for subresources, we still > need to do this check. so this should be: > > if (!IsBrowserSideNavigationEnabled() || info->GetChildID() != -1 || > !IsResourceTypeFrame(info->GetResourceType())) Thanks done.
nasko@chromium.org changed reviewers: + creis@chromium.org
Adding creis@, as I don't know the history of why the IO thread check on redirects exist and he might. https://codereview.chromium.org/2436253002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2436253002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:1141: ChildProcessSecurityPolicyImpl::GetInstance()->GrantRequestURL( Why are we doing this grant here? The process is either allowed or not. https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:262: if (!IsBrowserSideNavigationEnabled() || info->GetChildID() != -1 || Why check the process ID here? The check should be PlzNav and Frame navigation, right?
https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:520: !ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL( On 2016/10/21 21:08:29, ananta wrote: > On 2016/10/21 20:28:20, jam wrote: > > it seems a bit odd to be checking process ID of speculative processes with > > PlzNavigate. > > > > nasko suggested using the siteinstance id for this. The GrantRequestURL function > validates the id passed in with the expectation that the id would be registered. > > https://cs.chromium.org/chromium/src/content/browser/child_process_security_p.... > ah, in that case I think he meant doing navigation_handle_->GetSiteInstance()->GetProcess()->GetID() https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2436253002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:1141: ChildProcessSecurityPolicyImpl::GetInstance()->GrantRequestURL( On 2016/10/21 21:08:29, ananta wrote: > On 2016/10/21 20:28:21, jam wrote: > > GrantRequestURL should only be called for browser-initiated loads. if we call > it > > here, that means that an exploited renderer would say a navigation occurred to > a > > scheme it doesn't have access to and then this codepath would allow it. > > This function appears to be only called for top level navigations. These include > navigations originating in the browser and those via the > RenderFrameImpl::OpenURL code path. nasko please confirm if this is the case. (from our chat, so Nasko can clarify) on the NTP, clicking the icons led to this code path. is the NTP treated differently?
https://codereview.chromium.org/2436253002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2436253002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:1141: ChildProcessSecurityPolicyImpl::GetInstance()->GrantRequestURL( On 2016/10/21 21:21:07, nasko (slow) wrote: > Why are we doing this grant here? The process is either allowed or not. If we don't grant access here(e.g. file://), it fails in the redirect phase in navigationrequest https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:262: if (!IsBrowserSideNavigationEnabled() || info->GetChildID() != -1 || On 2016/10/21 21:21:07, nasko (slow) wrote: > Why check the process ID here? The check should be PlzNav and Frame navigation, > right? For non plznavigate we want the check to happen
The CQ bit was checked by ananta@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...
https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:262: if (!IsBrowserSideNavigationEnabled() || info->GetChildID() != -1 || On 2016/10/21 21:53:01, ananta wrote: > On 2016/10/21 21:21:07, nasko (slow) wrote: > > Why check the process ID here? The check should be PlzNav and Frame > navigation, > > right? > > For non plznavigate we want the check to happen Yes, that is already decided by "!IsBrowserSideNavigationEnabled()". What happens if we have a bug in the non-PlzNavigate case where the child process id is indeed -1? We will skip the check and default to success. This is bad from security perspective, isn't it?
https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:262: if (!IsBrowserSideNavigationEnabled() || info->GetChildID() != -1 || On 2016/10/21 22:47:49, nasko (slow) wrote: > On 2016/10/21 21:53:01, ananta wrote: > > On 2016/10/21 21:21:07, nasko (slow) wrote: > > > Why check the process ID here? The check should be PlzNav and Frame > > navigation, > > > right? > > > > For non plznavigate we want the check to happen > > Yes, that is already decided by "!IsBrowserSideNavigationEnabled()". What > happens if we have a bug in the non-PlzNavigate case where the child process id > is indeed -1? We will skip the check and default to success. This is bad from > security perspective, isn't it? Agreed. We should not have the child ID check here, allowing us to skip the CanRequestURL check. I also don't understand why the !IsResourceTypeFrame exception was added. CanRequestURL is not solely about navigation requests. It's about all requests. https://codereview.chromium.org/2436253002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:522: (!browser_initiated_ && nit: Would be clearer to move this complex condition out into a renderer_cant_request bool. https://codereview.chromium.org/2436253002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:524: frame_tree_node()->current_frame_host()->GetSiteInstance()-> This looks incorrect to me in the case that another tab/frame initiates the navigation in this tab (e.g., if a normal tab were targeting a tab with an extension URL in it). Is there a way to know the initiator of the navigation here? (And does this bug already exist in the current code?)
https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:262: if (!IsBrowserSideNavigationEnabled() || info->GetChildID() != -1 || On 2016/10/21 23:01:43, Charlie Reis wrote: > On 2016/10/21 22:47:49, nasko (slow) wrote: > > On 2016/10/21 21:53:01, ananta wrote: > > > On 2016/10/21 21:21:07, nasko (slow) wrote: > > > > Why check the process ID here? The check should be PlzNav and Frame > > > navigation, > > > > right? > > > > > > For non plznavigate we want the check to happen > > > > Yes, that is already decided by "!IsBrowserSideNavigationEnabled()". What > > happens if we have a bug in the non-PlzNavigate case where the child process > id > > is indeed -1? We will skip the check and default to success. This is bad from > > security perspective, isn't it? > > Agreed. We should not have the child ID check here, allowing us to skip the > CanRequestURL check. > > I also don't understand why the !IsResourceTypeFrame exception was added. > CanRequestURL is not solely about navigation requests. It's about all requests. I removed the GetChildID check. The !IsBrowserSideNavigation() enabled check will ensure that this works correctly for the non plznavigate case. The frame checks have been added here with the expectation that this will get handled eventually in NavigationRequest::OnRedirectChecksComplete() function. I verified that the function does get called for redirects on the main and sub frames. https://codereview.chromium.org/2436253002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:522: (!browser_initiated_ && On 2016/10/21 23:01:43, Charlie Reis wrote: > nit: Would be clearer to move this complex condition out into a > renderer_cant_request bool. Done. https://codereview.chromium.org/2436253002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:524: frame_tree_node()->current_frame_host()->GetSiteInstance()-> On 2016/10/21 23:01:44, Charlie Reis wrote: > This looks incorrect to me in the case that another tab/frame initiates the > navigation in this tab (e.g., if a normal tab were targeting a tab with an > extension URL in it). > > Is there a way to know the initiator of the navigation here? (And does this bug > already exist in the current code?) Added a check for both source and target site instances.
The CQ bit was checked by ananta@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ananta@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...
https://codereview.chromium.org/2436253002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:521: if (!browser_initiated_ && source_site_instance()) { Verified that the browser_initiated_ field is set to false for renderer initiated navigations like opening a link in a current tab/new tab/window via a script. Normal link clicks also have the browser_initiated_ flag set to false.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2436253002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:521: if (!browser_initiated_ && source_site_instance()) { actually, we should probably do this before throttles run? i.e. line 337 above where there's a TODO? And maybe instead of just calling CanRequestURL, we call FilterURL since that does more checks thatn just CPSP?
The CQ bit was checked by ananta@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...
https://codereview.chromium.org/2436253002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:521: if (!browser_initiated_ && source_site_instance()) { On 2016/10/24 16:59:16, jam wrote: > actually, we should probably do this before throttles run? i.e. line 337 above > where there's a TODO? And maybe instead of just calling CanRequestURL, we call > FilterURL since that does more checks thatn just CPSP? Done
Thanks! A few more questions to see if I understand the new checks. https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:262: if (!IsBrowserSideNavigationEnabled() || info->GetChildID() != -1 || On 2016/10/22 00:20:42, ananta wrote: > On 2016/10/21 23:01:43, Charlie Reis wrote: > > On 2016/10/21 22:47:49, nasko (slow) wrote: > > > On 2016/10/21 21:53:01, ananta wrote: > > > > On 2016/10/21 21:21:07, nasko (slow) wrote: > > > > > Why check the process ID here? The check should be PlzNav and Frame > > > > navigation, > > > > > right? > > > > > > > > For non plznavigate we want the check to happen > > > > > > Yes, that is already decided by "!IsBrowserSideNavigationEnabled()". What > > > happens if we have a bug in the non-PlzNavigate case where the child process > > id > > > is indeed -1? We will skip the check and default to success. This is bad > from > > > security perspective, isn't it? > > > > Agreed. We should not have the child ID check here, allowing us to skip the > > CanRequestURL check. > > > > I also don't understand why the !IsResourceTypeFrame exception was added. > > CanRequestURL is not solely about navigation requests. It's about all > requests. > > I removed the GetChildID check. The !IsBrowserSideNavigation() enabled check > will ensure that this works correctly for the non plznavigate case. The frame > checks have been added here with the expectation that this will get handled > eventually in NavigationRequest::OnRedirectChecksComplete() function. I verified > that the function does get called for redirects on the main and sub frames. I see. I was misreading it before-- the non-PlzNavigate case won't care about the extra checks. (I wonder if there's a way to make this code easier to read, like having a "check_handled_elsewhere" bool set to IsBrowserSideNavigationEnabled() && IsResourceTypeFrame(info->GetResourceType()). I suppose it's not a big deal, though.) https://codereview.chromium.org/2436253002/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:339: // browser initiated requests. I don't think it matters if the target (or rather, the current SiteInstance) has access, does it? Suppose the source is an extension A that opens a new tab to a web page B. The web page B is not allowed to go to another (non-web-accessible) extension URL (in A), but A is allowed to send the tab to another extension URL (in A). Note that this assumes we'll end up with a process swap for the redirect. Or am I thinking about this the wrong way? What's a case where we should block the navigation because the current SiteInstance isn't allowed to navigate there? (e.g., Is there a case where the navigation would stay in the current SiteInstance but that SiteInstance isn't allowed to request it? Seems unlikely to me.) https://codereview.chromium.org/2436253002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:346: if (url != common_params_.url) { This is making an assumption that FilterURL will only ever rewrite to about:blank. That's true today, but maybe this should also be checking whether |url| became about:blank (in addition to whether they differ)? https://codereview.chromium.org/2436253002/diff/160001/content/browser/loader... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/160001/content/browser/loader... content/browser/loader/resource_loader.cc:261: // NavigationRequest::OnRedirectChecksComplete() function. Should this comment say OnRequestRedirected instead of OnRedirectChecksComplete? (Looks like the call to FilterURL is there, which presumably is the one that calls CanRequestURL.)
https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/20001/content/browser/loader/... content/browser/loader/resource_loader.cc:262: if (!IsBrowserSideNavigationEnabled() || info->GetChildID() != -1 || On 2016/10/24 22:04:36, Charlie Reis wrote: > On 2016/10/22 00:20:42, ananta wrote: > > On 2016/10/21 23:01:43, Charlie Reis wrote: > > > On 2016/10/21 22:47:49, nasko (slow) wrote: > > > > On 2016/10/21 21:53:01, ananta wrote: > > > > > On 2016/10/21 21:21:07, nasko (slow) wrote: > > > > > > Why check the process ID here? The check should be PlzNav and Frame > > > > > navigation, > > > > > > right? > > > > > > > > > > For non plznavigate we want the check to happen > > > > > > > > Yes, that is already decided by "!IsBrowserSideNavigationEnabled()". What > > > > happens if we have a bug in the non-PlzNavigate case where the child > process > > > id > > > > is indeed -1? We will skip the check and default to success. This is bad > > from > > > > security perspective, isn't it? > > > > > > Agreed. We should not have the child ID check here, allowing us to skip the > > > CanRequestURL check. > > > > > > I also don't understand why the !IsResourceTypeFrame exception was added. > > > CanRequestURL is not solely about navigation requests. It's about all > > requests. > > > > I removed the GetChildID check. The !IsBrowserSideNavigation() enabled check > > will ensure that this works correctly for the non plznavigate case. The frame > > checks have been added here with the expectation that this will get handled > > eventually in NavigationRequest::OnRedirectChecksComplete() function. I > verified > > that the function does get called for redirects on the main and sub frames. > > I see. I was misreading it before-- the non-PlzNavigate case won't care about > the extra checks. > > (I wonder if there's a way to make this code easier to read, like having a > "check_handled_elsewhere" bool set to IsBrowserSideNavigationEnabled() && > IsResourceTypeFrame(info->GetResourceType()). I suppose it's not a big deal, > though.) Thanks. Done https://codereview.chromium.org/2436253002/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:339: // browser initiated requests. On 2016/10/24 22:04:36, Charlie Reis wrote: > I don't think it matters if the target (or rather, the current SiteInstance) has > access, does it? Suppose the source is an extension A that opens a new tab to a > web page B. The web page B is not allowed to go to another (non-web-accessible) > extension URL (in A), but A is allowed to send the tab to another extension URL > (in A). Note that this assumes we'll end up with a process swap for the > redirect. > > Or am I thinking about this the wrong way? What's a case where we should block > the navigation because the current SiteInstance isn't allowed to navigate there? > > > (e.g., Is there a case where the navigation would stay in the current > SiteInstance but that SiteInstance isn't allowed to request it? Seems unlikely > to me.) Thanks. Removed the check for the target. https://codereview.chromium.org/2436253002/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:346: if (url != common_params_.url) { On 2016/10/24 22:04:36, Charlie Reis wrote: > This is making an assumption that FilterURL will only ever rewrite to > about:blank. That's true today, but maybe this should also be checking whether > |url| became about:blank (in addition to whether they differ)? Thanks. Done. https://codereview.chromium.org/2436253002/diff/160001/content/browser/loader... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2436253002/diff/160001/content/browser/loader... content/browser/loader/resource_loader.cc:261: // NavigationRequest::OnRedirectChecksComplete() function. On 2016/10/24 22:04:36, Charlie Reis wrote: > Should this comment say OnRequestRedirected instead of OnRedirectChecksComplete? > (Looks like the call to FilterURL is there, which presumably is the one that > calls CanRequestURL.) Yes. Thanks for pointing this out.
The CQ bit was checked by ananta@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks, and sorry for all the corner cases to consider! LGTM.
lgtm with nit https://codereview.chromium.org/2436253002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:347: NavigationRequest::OnRedirectChecksComplete(NavigationThrottle::CANCEL); why not just call frame_tree_node_->ResetNavigationRequest(false); directly? that way OnRedirectChecksComplete is only called for the scenario that its name implies
https://codereview.chromium.org/2436253002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2436253002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:347: NavigationRequest::OnRedirectChecksComplete(NavigationThrottle::CANCEL); On 2016/10/25 00:17:08, jam wrote: > why not just call frame_tree_node_->ResetNavigationRequest(false); directly? > that way OnRedirectChecksComplete is only called for the scenario that its name > implies Done.
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/2436253002/#ps200001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Fix the FindInPageControllerTest.SearchWithinSpecialURL browser test. This test was initiating a navigation to the chrome\test\data folder and performing a number of find requests on the page returned. With PlzNavigate the request to navigate to chrome\test\data folder would get blocked in ResourceLoader::OnReceivedRedirect because the process id (-1) did not have access to the url being navigated to. In the non PlzNavigate case this is not an issue because the child process is given access to the URL during navigation. Proposed fix for PlzNavigate is to not do the ChildProcessSecurityPolicy check in the ResourceLoader::OnReceivedRedirect function and instead do this check on the UI thread in the NavigationRequest::OnRedirectChecksComplete function. Additionally we also grant access to the URL in the NavigatorImpl::RequestNavigation function. The above test is now enabled for Windows. BUG=175711 Disabled due to crbug.com/175711 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: Fix the FindInPageControllerTest.SearchWithinSpecialURL browser test. This test was initiating a navigation to the chrome\test\data folder and performing a number of find requests on the page returned. With PlzNavigate the request to navigate to chrome\test\data folder would get blocked in ResourceLoader::OnReceivedRedirect because the process id (-1) did not have access to the url being navigated to. In the non PlzNavigate case this is not an issue because the child process is given access to the URL during navigation. Proposed fix for PlzNavigate is to not do the ChildProcessSecurityPolicy check in the ResourceLoader::OnReceivedRedirect function and instead do this check on the UI thread in the NavigationRequest::OnRedirectChecksComplete function. Additionally we also grant access to the URL in the NavigatorImpl::RequestNavigation function. The above test is now enabled for Windows. BUG=175711 Disabled due to crbug.com/175711 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/7bb43d67968cfd1f9d56e0dbd1ce1dbb2fefe46f Cr-Commit-Position: refs/heads/master@{#427279} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/7bb43d67968cfd1f9d56e0dbd1ce1dbb2fefe46f Cr-Commit-Position: refs/heads/master@{#427279} |