|
|
Created:
3 years, 10 months ago by arthursonzogni Modified:
3 years, 9 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, nasko, alexmos Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: add support for BLOCK_REQUEST during redirects
This CL adds support for blocking requests in NavigationRequest during
redirects. It also fixes an issue in NavigationHandle, without
PlzNavigate activated, where the NavigationHandle would not be properly
recognized at error page commit if the navigation was blocked by a
NavigationThrottle during a redirect.
BUG=685074, 695421
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2698623006
Cr-Commit-Position: refs/heads/master@{#457387}
Committed: https://chromium.googlesource.com/chromium/src/+/5b5c309859412def817a518a7dddbf413cd892e9
Patch Set 1 #Patch Set 2 : Rebase + fix problem without PlzNavigate. #Patch Set 3 : Rebase. #Patch Set 4 : Take CL from: https://codereview.chromium.org/2729433003/ #
Total comments: 10
Patch Set 5 : Nit. #Patch Set 6 : Abandon support of BLOCK_REQUEST on redirect without PlzNavigate. #
Messages
Total messages: 59 (44 generated)
Description was changed from ========== Bugfix in NavigationRequest: returning BLOCK_REQUEST on redirect. There is currently no support for returning BLOCK_REQUEST in NavigationThrottle::WillRedirectRequest(). This CL fixes it and add a tests. This will be used in crrev.com/2655463006 BUG=685074 ========== to ========== Bugfix in NavigationRequest: returning BLOCK_REQUEST on redirect. There is currently no support for returning BLOCK_REQUEST in NavigationThrottle::WillRedirectRequest(). This CL fixes it and add a tests. This will be used in crrev.com/2655463006 BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
arthursonzogni@chromium.org changed reviewers: + clamy@chromium.org
Hi Camille, Please, could you take a look? Thanks!
Description was changed from ========== Bugfix in NavigationRequest: returning BLOCK_REQUEST on redirect. There is currently no support for returning BLOCK_REQUEST in NavigationThrottle::WillRedirectRequest(). This CL fixes it and add a tests. This will be used in crrev.com/2655463006 BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Bugfix in NavigationRequest: return BLOCK_REQUEST on redirect. There is currently no support for returning BLOCK_REQUEST in NavigationThrottle::WillRedirectRequest(). This CL fixes it and add a tests. This will be used in crrev.com/2655463006 BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by arthursonzogni@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...)
The CQ bit was checked by arthursonzogni@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...
Patchset #2 (id:20001) has been deleted
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by arthursonzogni@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...
Hi Camille, I fix the problem that occurs WITHOUT PlzNavigate. It is perfectly summarized by this TODO here: // TODO(mkwst): If we cancel the main frame request with anything other than // 'net::ERR_ABORTED', we'll trigger some special behavior that might not be // desirable here (non-POSTs will reload the page, while POST has some logic // around reloading to avoid duplicating actions server-side). For the // moment, only child frame navigations should be blocked. If we need to // block main frame navigations in the future, we'll need to carefully // consider the right thing to do here. Once the navigation was blocked, it was reloaded by blink. The observer observed the second navigation without any NavigationThrottle. To make it work, I prevented the Observer to observe more than 2 navigations. One thing that is weird is that with PlzNavigate, this reload mechanism doesn't exists, maybe it is a problem. I am also a little concerned with the reason evoked by mkwst, since the request to the redirecting url has already been done and will be done again(without PlzNavigate)
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...)
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by arthursonzogni@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...)
The CQ bit was checked by arthursonzogni@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...)
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by arthursonzogni@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...
Patchset #3 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
So, I was telling you silliness. The second navigation was not due to a reload but it was due to a mismatch in TakeNavigationHandleForCommit because blink doesn't know about the redirect when the redirect is blocked (it happens before the redirect is sent to blink). For reference, maybe a fix for this problem: https://codereview.chromium.org/2712123002/ Are you okay with this CL? Since I don't allow the observer to observe more than 1 navigation, the test pass.
Description was changed from ========== Bugfix in NavigationRequest: return BLOCK_REQUEST on redirect. There is currently no support for returning BLOCK_REQUEST in NavigationThrottle::WillRedirectRequest(). This CL fixes it and add a tests. This will be used in crrev.com/2655463006 BUG=685074 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: add support for BLOCK_REQUEST during redirects This CL adds support for blocking requests in NavigationRequest during redirects. It also fixes an issue in NavigationHandle, without PlzNavigate activated, where the NavigationHandle would not be properly recognized at error page commit if the navigation was blocked by a NavigationThrottle during a redirect. BUG=685074, 695421 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
arthursonzogni@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org
The CQ bit was checked by arthursonzogni@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...
Hi Charlie, I come back. When I was OOO, Camille tries to solve the same problem we had in https://codereview.chromium.org/2712123002/ Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
One concern below about the difference between the modes. I wonder if we can find the right behavior for both modes to converge on. https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:330: url_ = redirect_chain_.back(); Does this mean that if we're redirecting from A to B and B fails, we'll say that A failed? (Looks like yes, according to the test. I might be ok with that.) https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:755: NOTREACHED(); Unrelated: NOTREACHED is compiled out of release builds. Is it safe to act like PROCEED if some bug makes us get here with BLOCK_RESPONSE, or should we handle this differently? https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:1081: // to commit an error page to the post-redirect URL. The result was that the Did you mean pre-redirect URL? https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:1112: finished_navigation = {kRedirectingUrl}; I'm concerned that we have a difference in behavior based on which mode we're in. Ideally we'd decide what the right behavior is and make both modes act that way. After our discussion about the difference between blocked and failed navigations (where blocked navigations are more associated with the previous page and their error page stays in its process, vs failed navigations that might succeed on reload and end up with an error page in the destination process), I wonder if kRedirectingUrl is the right thing for both modes? Otherwise, a malicious redirect URL might be able to get an error page into a process of its choice. Would it make sense for PlzNavigate to do kRedirectingUrl here, or are there reasons that's worse?
Thanks Charlie! Some answers below: https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:330: url_ = redirect_chain_.back(); On 2017/03/09 17:39:03, Charlie Reis (slow) wrote: > Does this mean that if we're redirecting from A to B and B fails, we'll say that > A failed? (Looks like yes, according to the test. I might be ok with that.) Yes unfortunately. https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:755: NOTREACHED(); On 2017/03/09 17:39:03, Charlie Reis (slow) wrote: > Unrelated: NOTREACHED is compiled out of release builds. Is it safe to act like > PROCEED if some bug makes us get here with BLOCK_RESPONSE, or should we handle > this differently? It is impossible to have NavigationThrottle::BLOCK_RESPONSE in WillRedirectRequest() because it must only be returned in WillProcessResponse(). The NOTREACHED() is here to prevent developers to misuse it. So, I don't know what it means to have BLOCK_RESPONSE here, I can't say if it is safe or not. We probably can have separate enums in order to statically prevent this case or as you said using something that isn't stripped out in release mode. I think the comments in the enum and the NOTREACHED() are probably enough to prevent developers from using it. Let us know if you think we should do something. https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:1081: // to commit an error page to the post-redirect URL. The result was that the On 2017/03/09 17:39:03, Charlie Reis (slow) wrote: > Did you mean pre-redirect URL? Yes thanks! https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:1112: finished_navigation = {kRedirectingUrl}; On 2017/03/09 17:39:03, Charlie Reis (slow) wrote: > I'm concerned that we have a difference in behavior based on which mode we're > in. Ideally we'd decide what the right behavior is and make both modes act that > way. > > After our discussion about the difference between blocked and failed navigations > (where blocked navigations are more associated with the previous page and their > error page stays in its process, vs failed navigations that might succeed on > reload and end up with an error page in the destination process), I wonder if > kRedirectingUrl is the right thing for both modes? Otherwise, a malicious > redirect URL might be able to get an error page into a process of its choice. > > Would it make sense for PlzNavigate to do kRedirectingUrl here, or are there > reasons that's worse? The page is blocked on the post-redirect URL. So the right behavior is the PlzNavigate one, but fixing the problem that occurs without PlzNavigate looks very hard. What Camille did to mitigate it is dirty and we commit an error page to the pre-redirect URL, but that the best solution I know. If I understand, you suggest to do the wrong behavior with PlzNavigate too (committing an error-page to the pre-redirect URL). Why not, but I don't understand the benefits apart from the coherence in both mode. Do you know what an attacker could do with an error page in the wrong process? Whatever happens, when Nasko's patch: https://codereview.chromium.org/2738643002/ will be landed, the blocking error page will always be the process of its parent, no matter what is the url. Isn't it a problem too?
https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:755: NOTREACHED(); On 2017/03/10 11:55:28, arthursonzogni wrote: > On 2017/03/09 17:39:03, Charlie Reis (slow) wrote: > > Unrelated: NOTREACHED is compiled out of release builds. Is it safe to act > like > > PROCEED if some bug makes us get here with BLOCK_RESPONSE, or should we handle > > this differently? > > It is impossible to have NavigationThrottle::BLOCK_RESPONSE in > WillRedirectRequest() because it must only be returned in WillProcessResponse(). > The NOTREACHED() is here to prevent developers to misuse it. > > So, I don't know what it means to have BLOCK_RESPONSE here, I can't say if it is > safe or not. > We probably can have separate enums in order to statically prevent this case or > as you said using something that isn't stripped out in release mode. I think the > comments in the enum and the NOTREACHED() are probably enough to prevent > developers from using it. Let us know if you think we should do something. My concern was that a poorly written NavigationThrottle might return BLOCK_RESPONSE here, and because there's no early return or change to state_, we'll act like it said PROCEED. Developers would only realize the problem if they happened to write a test for that case. That said, it sounds like their code would be buggy in that case regardless, and the NOTREACHED is a reasonable attempt to help them catch bugs. It's only a concerning bug if a NavigationThrottle depends on BLOCK_RESPONSE for some security decision, in which case the developers should have noticed that it doesn't work here. I guess I'm ok with leaving it as is. https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:1112: finished_navigation = {kRedirectingUrl}; On 2017/03/10 11:55:28, arthursonzogni wrote: > On 2017/03/09 17:39:03, Charlie Reis (slow) wrote: > > I'm concerned that we have a difference in behavior based on which mode we're > > in. Ideally we'd decide what the right behavior is and make both modes act > that > > way. > > > > After our discussion about the difference between blocked and failed > navigations > > (where blocked navigations are more associated with the previous page and > their > > error page stays in its process, vs failed navigations that might succeed on > > reload and end up with an error page in the destination process), I wonder if > > kRedirectingUrl is the right thing for both modes? Otherwise, a malicious > > redirect URL might be able to get an error page into a process of its choice. > > > > Would it make sense for PlzNavigate to do kRedirectingUrl here, or are there > > reasons that's worse? > > The page is blocked on the post-redirect URL. So the right behavior is the > PlzNavigate one, but fixing the problem that occurs without PlzNavigate looks > very hard. What Camille did to mitigate it is dirty and we commit an error page > to the pre-redirect URL, but that the best solution I know. > > If I understand, you suggest to do the wrong behavior with PlzNavigate too > (committing an error-page to the pre-redirect URL). Why not, but I don't > understand the benefits apart from the coherence in both mode. My argument there was that the pre-redirect URL isn't necessarily the wrong one. Yes, Chrome today reports the post-redirect URL, but there's unfortunate aspects to that for blocked URLs. A malicious page can try to navigate to an off-limits page, and even though the navigation "fails", that URL still ends up in the user's session history. The user might later reload it, restore it, etc, and we have to be extra careful that malicious page can't circumvent having its navigation blocked this way. By using the pre-redirect URL, we would avoid those problems. On the flip side, though, the user experience is quite different, and might make it look like the pre-redirect URL was the one being blocked (which isn't true). It's a nontrivial change to make, and Nasko, Alex, and I have some thoughts about how to do it down the road. Given that, let's forget I suggested standardizing on the pre-redirect CL for now. :) Instead, let's try to get both modes to report the post-redirect CL, which is a minimal change from Chrome's behavior before this CL. On your earlier version of this CL (https://codereview.chromium.org/2712123002/), I wondered if there was a way to send ResourceMsg_ReceivedRedirect in the blocked case so that the renderer did the right thing. Chatting with Nasko, it seems like we don't send that IPC at the moment because we don't make it down to AsyncResourceHandler if the redirect was blocked. Maybe the blocking logic can send that IPC manually, since we know we won't do it from AsyncResourceHandler? It's a bit of a layering violation / hack, but we'd only need it until PlzNavigate ships, and it would hopefully get us consistent behavior across modes. > > Do you know what an attacker could do with an error page in the wrong process? > Whatever happens, when Nasko's patch: > https://codereview.chromium.org/2738643002/ will be landed, the blocking error > page will always be the process of its parent, no matter what is the url. Isn't > it a problem too?
On 2017/03/13 20:48:02, Charlie Reis (slow) wrote: > https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... > File content/browser/frame_host/navigation_handle_impl.cc (right): > > https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... > content/browser/frame_host/navigation_handle_impl.cc:755: NOTREACHED(); > On 2017/03/10 11:55:28, arthursonzogni wrote: > > On 2017/03/09 17:39:03, Charlie Reis (slow) wrote: > > > Unrelated: NOTREACHED is compiled out of release builds. Is it safe to act > > like > > > PROCEED if some bug makes us get here with BLOCK_RESPONSE, or should we > handle > > > this differently? > > > > It is impossible to have NavigationThrottle::BLOCK_RESPONSE in > > WillRedirectRequest() because it must only be returned in > WillProcessResponse(). > > The NOTREACHED() is here to prevent developers to misuse it. > > > > So, I don't know what it means to have BLOCK_RESPONSE here, I can't say if it > is > > safe or not. > > We probably can have separate enums in order to statically prevent this case > or > > as you said using something that isn't stripped out in release mode. I think > the > > comments in the enum and the NOTREACHED() are probably enough to prevent > > developers from using it. Let us know if you think we should do something. > > My concern was that a poorly written NavigationThrottle might return > BLOCK_RESPONSE here, and because there's no early return or change to state_, > we'll act like it said PROCEED. Developers would only realize the problem if > they happened to write a test for that case. > > That said, it sounds like their code would be buggy in that case regardless, and > the NOTREACHED is a reasonable attempt to help them catch bugs. It's only a > concerning bug if a NavigationThrottle depends on BLOCK_RESPONSE for some > security decision, in which case the developers should have noticed that it > doesn't work here. I guess I'm ok with leaving it as is. > > https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... > File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): > > https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... > content/browser/frame_host/navigation_handle_impl_browsertest.cc:1112: > finished_navigation = {kRedirectingUrl}; > On 2017/03/10 11:55:28, arthursonzogni wrote: > > On 2017/03/09 17:39:03, Charlie Reis (slow) wrote: > > > I'm concerned that we have a difference in behavior based on which mode > we're > > > in. Ideally we'd decide what the right behavior is and make both modes act > > that > > > way. > > > > > > After our discussion about the difference between blocked and failed > > navigations > > > (where blocked navigations are more associated with the previous page and > > their > > > error page stays in its process, vs failed navigations that might succeed on > > > reload and end up with an error page in the destination process), I wonder > if > > > kRedirectingUrl is the right thing for both modes? Otherwise, a malicious > > > redirect URL might be able to get an error page into a process of its > choice. > > > > > > Would it make sense for PlzNavigate to do kRedirectingUrl here, or are there > > > reasons that's worse? > > > > The page is blocked on the post-redirect URL. So the right behavior is the > > PlzNavigate one, but fixing the problem that occurs without PlzNavigate looks > > very hard. What Camille did to mitigate it is dirty and we commit an error > page > > to the pre-redirect URL, but that the best solution I know. > > > > If I understand, you suggest to do the wrong behavior with PlzNavigate too > > (committing an error-page to the pre-redirect URL). Why not, but I don't > > understand the benefits apart from the coherence in both mode. > > My argument there was that the pre-redirect URL isn't necessarily the wrong one. > Yes, Chrome today reports the post-redirect URL, but there's unfortunate > aspects to that for blocked URLs. A malicious page can try to navigate to an > off-limits page, and even though the navigation "fails", that URL still ends up > in the user's session history. The user might later reload it, restore it, etc, > and we have to be extra careful that malicious page can't circumvent having its > navigation blocked this way. > > By using the pre-redirect URL, we would avoid those problems. On the flip side, > though, the user experience is quite different, and might make it look like the > pre-redirect URL was the one being blocked (which isn't true). It's a > nontrivial change to make, and Nasko, Alex, and I have some thoughts about how > to do it down the road. > > Given that, let's forget I suggested standardizing on the pre-redirect CL for > now. :) Instead, let's try to get both modes to report the post-redirect CL, > which is a minimal change from Chrome's behavior before this CL. > > On your earlier version of this CL > (https://codereview.chromium.org/2712123002/), I wondered if there was a way to > send ResourceMsg_ReceivedRedirect in the blocked case so that the renderer did > the right thing. Chatting with Nasko, it seems like we don't send that IPC at > the moment because we don't make it down to AsyncResourceHandler if the redirect > was blocked. Maybe the blocking logic can send that IPC manually, since we know > we won't do it from AsyncResourceHandler? It's a bit of a layering violation / > hack, but we'd only need it until PlzNavigate ships, and it would hopefully get > us consistent behavior across modes. Just chiming in. We could try to send the IPC, but I'm not sure about the side effects that this may have. First, we are going to go through a certain number of checks in Blink, do we want to that? Second, the renderer process would normally reply back to the browser process to proceed with the redirect or not. If it should proceed, the AsyncResourceHandler will resume the request. However here, I'm not sure we can guarantee that the whole loader chain will be gone by the time the IPC arrives in the browser process and that we won't accidentally resume the navigation. > > > > > Do you know what an attacker could do with an error page in the wrong process? > > Whatever happens, when Nasko's patch: > > https://codereview.chromium.org/2738643002/ will be landed, the blocking error > > page will always be the process of its parent, no matter what is the url. > Isn't > > it a problem too?
On 2017/03/14 16:20:38, clamy wrote: > https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... > > content/browser/frame_host/navigation_handle_impl_browsertest.cc:1112: > > finished_navigation = {kRedirectingUrl}; > > On 2017/03/10 11:55:28, arthursonzogni wrote: > > > On 2017/03/09 17:39:03, Charlie Reis (slow) wrote: > > > > I'm concerned that we have a difference in behavior based on which mode > > we're > > > > in. Ideally we'd decide what the right behavior is and make both modes > act > > > that > > > > way. > > > > > > > > After our discussion about the difference between blocked and failed > > > navigations > > > > (where blocked navigations are more associated with the previous page and > > > their > > > > error page stays in its process, vs failed navigations that might succeed > on > > > > reload and end up with an error page in the destination process), I wonder > > if > > > > kRedirectingUrl is the right thing for both modes? Otherwise, a malicious > > > > redirect URL might be able to get an error page into a process of its > > choice. > > > > > > > > Would it make sense for PlzNavigate to do kRedirectingUrl here, or are > there > > > > reasons that's worse? > > > > > > The page is blocked on the post-redirect URL. So the right behavior is the > > > PlzNavigate one, but fixing the problem that occurs without PlzNavigate > looks > > > very hard. What Camille did to mitigate it is dirty and we commit an error > > page > > > to the pre-redirect URL, but that the best solution I know. > > > > > > If I understand, you suggest to do the wrong behavior with PlzNavigate too > > > (committing an error-page to the pre-redirect URL). Why not, but I don't > > > understand the benefits apart from the coherence in both mode. > > > > My argument there was that the pre-redirect URL isn't necessarily the wrong > one. > > Yes, Chrome today reports the post-redirect URL, but there's unfortunate > > aspects to that for blocked URLs. A malicious page can try to navigate to an > > off-limits page, and even though the navigation "fails", that URL still ends > up > > in the user's session history. The user might later reload it, restore it, > etc, > > and we have to be extra careful that malicious page can't circumvent having > its > > navigation blocked this way. > > > > By using the pre-redirect URL, we would avoid those problems. On the flip > side, > > though, the user experience is quite different, and might make it look like > the > > pre-redirect URL was the one being blocked (which isn't true). It's a > > nontrivial change to make, and Nasko, Alex, and I have some thoughts about how > > to do it down the road. > > > > Given that, let's forget I suggested standardizing on the pre-redirect CL for > > now. :) Instead, let's try to get both modes to report the post-redirect CL, > > which is a minimal change from Chrome's behavior before this CL. > > > > On your earlier version of this CL > > (https://codereview.chromium.org/2712123002/), I wondered if there was a way > to > > send ResourceMsg_ReceivedRedirect in the blocked case so that the renderer did > > the right thing. Chatting with Nasko, it seems like we don't send that IPC at > > the moment because we don't make it down to AsyncResourceHandler if the > redirect > > was blocked. Maybe the blocking logic can send that IPC manually, since we > know > > we won't do it from AsyncResourceHandler? It's a bit of a layering violation > / > > hack, but we'd only need it until PlzNavigate ships, and it would hopefully > get > > us consistent behavior across modes. > > Just chiming in. We could try to send the IPC, but I'm not sure about the side > effects that this may have. First, we are going to go through a certain number > of checks in Blink, do we want to that? Second, the renderer process would > normally reply back to the browser process to proceed with the redirect or not. > If it should proceed, the AsyncResourceHandler will resume the request. However > here, I'm not sure we can guarantee that the whole loader chain will be gone by > the time the IPC arrives in the browser process and that we won't accidentally > resume the navigation. What if we sent a new IPC just to add the redirect into the chain? Or is there another way for the renderer to infer that the redirect occurred? Something like sending the URL in ResourceMsg_RequestComplete?
On 2017/03/14 16:28:09, Charlie Reis (slow) wrote: > On 2017/03/14 16:20:38, clamy wrote: > > > https://codereview.chromium.org/2698623006/diff/140001/content/browser/frame_... > > > content/browser/frame_host/navigation_handle_impl_browsertest.cc:1112: > > > finished_navigation = {kRedirectingUrl}; > > > On 2017/03/10 11:55:28, arthursonzogni wrote: > > > > On 2017/03/09 17:39:03, Charlie Reis (slow) wrote: > > > > > I'm concerned that we have a difference in behavior based on which mode > > > we're > > > > > in. Ideally we'd decide what the right behavior is and make both modes > > act > > > > that > > > > > way. > > > > > > > > > > After our discussion about the difference between blocked and failed > > > > navigations > > > > > (where blocked navigations are more associated with the previous page > and > > > > their > > > > > error page stays in its process, vs failed navigations that might > succeed > > on > > > > > reload and end up with an error page in the destination process), I > wonder > > > if > > > > > kRedirectingUrl is the right thing for both modes? Otherwise, a > malicious > > > > > redirect URL might be able to get an error page into a process of its > > > choice. > > > > > > > > > > Would it make sense for PlzNavigate to do kRedirectingUrl here, or are > > there > > > > > reasons that's worse? > > > > > > > > The page is blocked on the post-redirect URL. So the right behavior is the > > > > PlzNavigate one, but fixing the problem that occurs without PlzNavigate > > looks > > > > very hard. What Camille did to mitigate it is dirty and we commit an error > > > page > > > > to the pre-redirect URL, but that the best solution I know. > > > > > > > > If I understand, you suggest to do the wrong behavior with PlzNavigate too > > > > (committing an error-page to the pre-redirect URL). Why not, but I don't > > > > understand the benefits apart from the coherence in both mode. > > > > > > My argument there was that the pre-redirect URL isn't necessarily the wrong > > one. > > > Yes, Chrome today reports the post-redirect URL, but there's unfortunate > > > aspects to that for blocked URLs. A malicious page can try to navigate to > an > > > off-limits page, and even though the navigation "fails", that URL still ends > > up > > > in the user's session history. The user might later reload it, restore it, > > etc, > > > and we have to be extra careful that malicious page can't circumvent having > > its > > > navigation blocked this way. > > > > > > By using the pre-redirect URL, we would avoid those problems. On the flip > > side, > > > though, the user experience is quite different, and might make it look like > > the > > > pre-redirect URL was the one being blocked (which isn't true). It's a > > > nontrivial change to make, and Nasko, Alex, and I have some thoughts about > how > > > to do it down the road. > > > > > > Given that, let's forget I suggested standardizing on the pre-redirect CL > for > > > now. :) Instead, let's try to get both modes to report the post-redirect > CL, > > > which is a minimal change from Chrome's behavior before this CL. > > > > > > On your earlier version of this CL > > > (https://codereview.chromium.org/2712123002/), I wondered if there was a way > > to > > > send ResourceMsg_ReceivedRedirect in the blocked case so that the renderer > did > > > the right thing. Chatting with Nasko, it seems like we don't send that IPC > at > > > the moment because we don't make it down to AsyncResourceHandler if the > > redirect > > > was blocked. Maybe the blocking logic can send that IPC manually, since we > > know > > > we won't do it from AsyncResourceHandler? It's a bit of a layering > violation > > / > > > hack, but we'd only need it until PlzNavigate ships, and it would hopefully > > get > > > us consistent behavior across modes. > > > > Just chiming in. We could try to send the IPC, but I'm not sure about the side > > effects that this may have. First, we are going to go through a certain number > > of checks in Blink, do we want to that? Second, the renderer process would > > normally reply back to the browser process to proceed with the redirect or > not. > > If it should proceed, the AsyncResourceHandler will resume the request. > However > > here, I'm not sure we can guarantee that the whole loader chain will be gone > by > > the time the IPC arrives in the browser process and that we won't accidentally > > resume the navigation. > > What if we sent a new IPC just to add the redirect into the chain? Or is there > another way for the renderer to infer that the redirect occurred? Something > like sending the URL in ResourceMsg_RequestComplete? A new IPC could work. I imagine we could send the final url in ResourceMsg_RequestComplete and fake a redirect there (we fake redirects in PlzNavigate at some point so this could work).
Patchset #6 (id:180001) has been deleted
The CQ bit was checked by arthursonzogni@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...
Thanks for the reviews! We tried overriding blink url by using ResourceMsg_RequestComplete. It was a promising way, but it turns out that it still require us to change a lot of thing. I think we could simply drop the support of BLOCK_REQUEST on redirect without PlzNavigate for the moment. That's what I did in the last patch. Please let me know what do you think about it. N.B. BLOCK_REQUEST on redirect will be useful for enforcing 'frame-src' and 'form-action" CSP on the browser-side with PlzNavigate.
On 2017/03/15 16:45:13, arthursonzogni wrote: > Thanks for the reviews! > > We tried overriding blink url by using ResourceMsg_RequestComplete. It was a > promising way, but it turns out that it still require us to change a lot of > thing. > > I think we could simply drop the support of BLOCK_REQUEST on redirect without > PlzNavigate for the moment. That's what I did in the last patch. Please let me > know what do you think about it. > > N.B. BLOCK_REQUEST on redirect will be useful for enforcing 'frame-src' and > 'form-action" CSP on the browser-side with PlzNavigate. Ok. I'm disappointed we won't get to use this browser-side enforcement of CSP in non-PlzNavigate, but I agree that isn't blocking for the time being. LGTM and we can revisit if there proves to be a reason for it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/15 17:44:49, Charlie Reis (slow) wrote: > Ok. I'm disappointed we won't get to use this browser-side enforcement of CSP > in non-PlzNavigate, but I agree that isn't blocking for the time being. LGTM > and we can revisit if there proves to be a reason for it. Yes, making BLOCK_REQUEST working on redirect without PlzNavigate is probably the only thing that prevents us to make the "frame-src" enforcement on the browser-side without PlzNavigate. It would be very useful for fixing the last issues between frame-src and site isolation. Thanks for the review and yes, this should be revisited soon.
The CQ bit was checked by arthursonzogni@chromium.org
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": 200001, "attempt_start_ts": 1489657839331550, "parent_rev": "e8014f980b6380e690b2e386ac8f39d228419e2f", "commit_rev": "5b5c309859412def817a518a7dddbf413cd892e9"}
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: add support for BLOCK_REQUEST during redirects This CL adds support for blocking requests in NavigationRequest during redirects. It also fixes an issue in NavigationHandle, without PlzNavigate activated, where the NavigationHandle would not be properly recognized at error page commit if the navigation was blocked by a NavigationThrottle during a redirect. BUG=685074, 695421 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: add support for BLOCK_REQUEST during redirects This CL adds support for blocking requests in NavigationRequest during redirects. It also fixes an issue in NavigationHandle, without PlzNavigate activated, where the NavigationHandle would not be properly recognized at error page commit if the navigation was blocked by a NavigationThrottle during a redirect. BUG=685074, 695421 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2698623006 Cr-Commit-Position: refs/heads/master@{#457387} Committed: https://chromium.googlesource.com/chromium/src/+/5b5c309859412def817a518a7ddd... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:200001) as https://chromium.googlesource.com/chromium/src/+/5b5c309859412def817a518a7ddd... |