|
|
Created:
4 years, 7 months ago by dominickn Modified:
4 years, 7 months ago CC:
chrome-apps-syd-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, Charlie Reis, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPersist prompt/block download limiter state on renderer-initiated loads.
The download request limiter will reset its state to allow a download
when it receives a notification of a pending navigation. This ensures
that the newly loaded page is always allowed to download one file. The
reset does not occur if the current state allows or blocks all downloads
AND the navigation is to the same host.
This means that a site can avoid prompting the user for multiple
downloads as follows:
1. trigger a download (moves state from ALLOW_ONE to PROMPT)
2. change window.location.href, creating a navigation and
resetting the limiter state to ALLOW_ONE
Sites can repeatedly loop and trigger downloads in a new pop-up window,
or use meta refresh tags to "ping-pong" between pages, downloading a
new file on each page. Because the limiter is in the PROMPT state when
the pending navigation arrives, the state is reset and the next download
is always permitted.
This CL closes the loophole by making the limiter maintain state
following a renderer-initiated navigation, unless the state is allowing
one or all downloads. This ensures that navigations which are not
triggered by the user cannot be used to reset the limiter to allowing
downloads if the state is either blocking downloads or prompting the
user to permit downloads. Both carpet bomb attacks described above
are mitigated in the CL.
A new IsRendererInitiated method is added to
content::NavigationHandle to enable this functionality. The limiter now
overrides WebContentsObserver::DidStartNavigation so it can access
the handle.
BUG=559515
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/b38c9f6740d73bd4e40bc197c3983739bbeec0ba
Cr-Commit-Position: refs/heads/master@{#394721}
Patch Set 1 #Patch Set 2 : Fix tests #Patch Set 3 : Restore non-PlzNavigate code #Patch Set 4 : Implement DidFinishNavigation #
Total comments: 22
Patch Set 5 : Addressing reviewer comments #
Total comments: 5
Patch Set 6 : Address nit #
Total comments: 8
Patch Set 7 : Address nit #Patch Set 8 : Adding subframe limiter test #Messages
Total messages: 31 (13 generated)
Description was changed from ========== Persist prompt/block download limiter state on renderer-initiated loads. The download request limiter will reset its state to allow a download when it receives a notification of a pending navigation. This ensures that the newly loaded page is always allowed to download one file. The reset does not occur if the current state allows or blocks all downloads AND the navigation is to the same host. This means that a site can avoid prompting the user for multiple downloads as follows: 1. trigger a download (moves state from ALLOW_ONE to PROMPT) 2. change window.location.href, creating a navigation and resetting the limiter state to ALLOW_ONE Sites can repeatedly loop and trigger downloads in a new pop-up window, or use meta refresh tags to "ping-pong" between pages, downloading a new file one each page. Because the limiter is in the PROMPT state when the pending navigation arrives, the state is reset and the next download is always permitted. This CL closes the loophole by making the limiter maintain state following a renderer-initiated navigation, unless the state is allowing one or all downloads. This ensures that navigations which are not triggered by the user cannot be used to reset the limiter to allowing downloads if the state is either blocking downloads or prompting the user to permit downloads. A new IsRendererInitiated method is added to content::NavigationHandle to enable this functionality. The limiter is switched from overriding WebContentsObserver::DidNavigateMainFrame to WebContentsObserver::DidStartNavigation. BUG=559515 ========== to ========== Persist prompt/block download limiter state on renderer-initiated loads. The download request limiter will reset its state to allow a download when it receives a notification of a pending navigation. This ensures that the newly loaded page is always allowed to download one file. The reset does not occur if the current state allows or blocks all downloads AND the navigation is to the same host. This means that a site can avoid prompting the user for multiple downloads as follows: 1. trigger a download (moves state from ALLOW_ONE to PROMPT) 2. change window.location.href, creating a navigation and resetting the limiter state to ALLOW_ONE Sites can repeatedly loop and trigger downloads in a new pop-up window, or use meta refresh tags to "ping-pong" between pages, downloading a new file one each page. Because the limiter is in the PROMPT state when the pending navigation arrives, the state is reset and the next download is always permitted. This CL closes the loophole by making the limiter maintain state following a renderer-initiated navigation, unless the state is allowing one or all downloads. This ensures that navigations which are not triggered by the user cannot be used to reset the limiter to allowing downloads if the state is either blocking downloads or prompting the user to permit downloads. A new IsRendererInitiated method is added to content::NavigationHandle to enable this functionality. The limiter is switched from overriding WebContentsObserver::DidNavigateMainFrame to WebContentsObserver::DidStartNavigation. BUG=559515 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Persist prompt/block download limiter state on renderer-initiated loads. The download request limiter will reset its state to allow a download when it receives a notification of a pending navigation. This ensures that the newly loaded page is always allowed to download one file. The reset does not occur if the current state allows or blocks all downloads AND the navigation is to the same host. This means that a site can avoid prompting the user for multiple downloads as follows: 1. trigger a download (moves state from ALLOW_ONE to PROMPT) 2. change window.location.href, creating a navigation and resetting the limiter state to ALLOW_ONE Sites can repeatedly loop and trigger downloads in a new pop-up window, or use meta refresh tags to "ping-pong" between pages, downloading a new file one each page. Because the limiter is in the PROMPT state when the pending navigation arrives, the state is reset and the next download is always permitted. This CL closes the loophole by making the limiter maintain state following a renderer-initiated navigation, unless the state is allowing one or all downloads. This ensures that navigations which are not triggered by the user cannot be used to reset the limiter to allowing downloads if the state is either blocking downloads or prompting the user to permit downloads. A new IsRendererInitiated method is added to content::NavigationHandle to enable this functionality. The limiter is switched from overriding WebContentsObserver::DidNavigateMainFrame to WebContentsObserver::DidStartNavigation. BUG=559515 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Persist prompt/block download limiter state on renderer-initiated loads. The download request limiter will reset its state to allow a download when it receives a notification of a pending navigation. This ensures that the newly loaded page is always allowed to download one file. The reset does not occur if the current state allows or blocks all downloads AND the navigation is to the same host. This means that a site can avoid prompting the user for multiple downloads as follows: 1. trigger a download (moves state from ALLOW_ONE to PROMPT) 2. change window.location.href, creating a navigation and resetting the limiter state to ALLOW_ONE Sites can repeatedly loop and trigger downloads in a new pop-up window, or use meta refresh tags to "ping-pong" between pages, downloading a new file one each page. Because the limiter is in the PROMPT state when the pending navigation arrives, the state is reset and the next download is always permitted. This CL closes the loophole by making the limiter maintain state following a renderer-initiated navigation, unless the state is allowing one or all downloads. This ensures that navigations which are not triggered by the user cannot be used to reset the limiter to allowing downloads if the state is either blocking downloads or prompting the user to permit downloads. A new IsRendererInitiated method is added to content::NavigationHandle to enable this functionality. The limiter is switched from overriding WebContentsObserver::DidNavigateMainFrame to WebContentsObserver::DidStartNavigation. BUG=559515 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
dominickn@chromium.org changed reviewers: + asanka@chromium.org, nasko@chromium.org
dominickn@chromium.org changed reviewers: + clamy@chromium.org - nasko@chromium.org
dominickn@chromium.org changed reviewers: + creis@chromium.org
Take 2, this time plumbing in is_renderer_initiated onto NavigationHandle. asanka: PTAL at the download request limiter clamy, creis: PTAL at content/ and the implementation of DidStart/FinishNavigation in the download request limiter. It is intended that this CL be a no-op change when browser-side navigation is disabled, whilst listening for pending navs and renderer initiated loads is enabled when browser-side navigation is enabled. Also, please let me know if I've adequately plumbed is_renderer_initiated through onto NavigationHandle. It seems to work correctly in my testing, but if you'd prefer it done a different way (e.g. the boolean is reset in NavigationEntrys, but I didn't implement that) let me know. Thanks!
Thanks for rewriting! Some thoughts below. On 2016/05/11 03:01:27, dominickn wrote: > Take 2, this time plumbing in is_renderer_initiated onto NavigationHandle. > > asanka: PTAL at the download request limiter > > clamy, creis: PTAL at content/ and the implementation of > DidStart/FinishNavigation in the download request limiter. It is intended that > this CL be a no-op change when browser-side navigation is disabled, whilst > listening for pending navs and renderer initiated loads is enabled when > browser-side navigation is enabled. I'm confused by this comment-- why would it be specific to PlzNavigate? NavigationHandles are used in default Chrome as well. > Also, please let me know if I've adequately plumbed is_renderer_initiated > through onto NavigationHandle. It seems to work correctly in my testing, but if > you'd prefer it done a different way (e.g. the boolean is reset in > NavigationEntrys, but I didn't implement that) let me know. Thanks! Camille, NavigationHandle goes away at commit, right? That should make it unnecessary to clear the value the way we do in NavigationEntry. The plumbing generally looks good, with one caveat about code that I suspect might be dead. https://codereview.chromium.org/1964863002/diff/60001/chrome/browser/download... File chrome/browser/download/download_request_limiter.cc (right): https://codereview.chromium.org/1964863002/diff/60001/chrome/browser/download... chrome/browser/download/download_request_limiter.cc:77: if (!navigation_handle->IsInMainFrame()) Sanity check: Do we limit downloads in subframes somewhere else? https://codereview.chromium.org/1964863002/diff/60001/chrome/browser/download... chrome/browser/download/download_request_limiter.cc:84: // content::IsBrowserSideNavigationEnabled() returns true). This doesn't sound right. We should know IsRendererInitiated whether we're in PlzNavigate or not. (NavigatorImpl::DidStartProvisionalLoad should be setting it in the default mode.) https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:128: // entries. See https://crbug.com/495161. Yikes, looks like most of this was copied from update_entry_id_for_transfer, which has a stale comment. I've recently fixed https://crbug.com/495161 and we don't use a pending entry during transfer. (I'll have to see if I can remove update_entry_id_for_transfer now.) I'm wondering if this is dead code as well. I'll try to find time to investigate if I can, but let me know if you're aware of a case this is needed for. https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_browsertest.cc:91: bool is_renderer_initiated() { return is_renderer_initiated_; } Please actually check this in some of the tests. Hopefully we can observe it in both states below. https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:178: bool is_renderer_initiated = false; Seems like we should default to true, with a comment about why. Reasoning: It's safer to assume a navigation is renderer-initiated unless we know otherwise, since browser-initiated navigations are generally more powerful (e.g., can go to chrome:// URLs). Browser-initiated navigations should always have a pending NavigationEntry, so we should be able to detect if that's the case here. (Can you add a test that would have failed with the way this is currently written? I bet it would classify a subframe navigation as browser-initiated.) https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1064: bool is_renderer_initiated = false; As before, better assume true unless we know otherwise. https://codereview.chromium.org/1964863002/diff/60001/content/public/browser/... File content/public/browser/navigation_handle.cc (right): https://codereview.chromium.org/1964863002/diff/60001/content/public/browser/... content/public/browser/navigation_handle.cc:32: false, // is_renderer_initiated Would it cause any problems to make this default to true? I'm not sure what impact that has on existing tests. https://codereview.chromium.org/1964863002/diff/60001/content/public/browser/... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1964863002/diff/60001/content/public/browser/... content/public/browser/navigation_handle.h:52: // * <meta http-equiv="refresh"> tag nit: Let's be a bit more explicit that these are just two of many examples, since I've noticed many people don't know the difference between e.g. and i.e. :) Something like the the comment on IsSynchronousNavigation could work here: "...by the renderer process. Examples of renderer-initiated navigations include: * a link click * a change to window.location.href * <meta http-equiv="refresh"> tag"
Thanks! A few comments and one question. My understanding from the CL description is that you want to ensure that navigations that were not triggered by the user cannot reset the state of the download limiter. However, some renderer-initiated navigations are actually triggered by the user, for example link clicks or form submits. Is it ok if they don't reset the state of the download limiter? https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:128: // entries. See https://crbug.com/495161. On 2016/05/11 21:50:03, Charlie Reis wrote: > Yikes, looks like most of this was copied from update_entry_id_for_transfer, > which has a stale comment. I've recently fixed https://crbug.com/495161 and we > don't use a pending entry during transfer. (I'll have to see if I can remove > update_entry_id_for_transfer now.) > > I'm wondering if this is dead code as well. I'll try to find time to > investigate if I can, but let me know if you're aware of a case this is needed > for. Considering how the NavigationHandle is used (ie from start to stop), there should be no reason to change is_renderer_initiated ever over the course of the navigation. In transfer navigations we transfer the actual NavigationHandle, so there's no need to update is_renderer_initiated. https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:269: bool is_renderer_initiated_; I don't see a good reason why this should change over the course of the navigation. This should be const. https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_request.h:152: void CreateNavigationHandle(bool is_renderer_initiated, Navigation request already has a boolean browser_initiated_ (ie !is_renderer_initiated). Please use it instead of adding a new parameter to this function. https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1064: bool is_renderer_initiated = false; On 2016/05/11 21:50:04, Charlie Reis wrote: > As before, better assume true unless we know otherwise. In this case is_renderer_initiated _really_ has to be set to true. Otherwise you will instantiate the NavigationHandle for synchronous navigation below (line 1097) with is_renderer_initiated false all the time. Considering that this case corresponds to fragment navigations or push state, this is wrong.
Description was changed from ========== Persist prompt/block download limiter state on renderer-initiated loads. The download request limiter will reset its state to allow a download when it receives a notification of a pending navigation. This ensures that the newly loaded page is always allowed to download one file. The reset does not occur if the current state allows or blocks all downloads AND the navigation is to the same host. This means that a site can avoid prompting the user for multiple downloads as follows: 1. trigger a download (moves state from ALLOW_ONE to PROMPT) 2. change window.location.href, creating a navigation and resetting the limiter state to ALLOW_ONE Sites can repeatedly loop and trigger downloads in a new pop-up window, or use meta refresh tags to "ping-pong" between pages, downloading a new file one each page. Because the limiter is in the PROMPT state when the pending navigation arrives, the state is reset and the next download is always permitted. This CL closes the loophole by making the limiter maintain state following a renderer-initiated navigation, unless the state is allowing one or all downloads. This ensures that navigations which are not triggered by the user cannot be used to reset the limiter to allowing downloads if the state is either blocking downloads or prompting the user to permit downloads. A new IsRendererInitiated method is added to content::NavigationHandle to enable this functionality. The limiter is switched from overriding WebContentsObserver::DidNavigateMainFrame to WebContentsObserver::DidStartNavigation. BUG=559515 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Persist prompt/block download limiter state on renderer-initiated loads. The download request limiter will reset its state to allow a download when it receives a notification of a pending navigation. This ensures that the newly loaded page is always allowed to download one file. The reset does not occur if the current state allows or blocks all downloads AND the navigation is to the same host. This means that a site can avoid prompting the user for multiple downloads as follows: 1. trigger a download (moves state from ALLOW_ONE to PROMPT) 2. change window.location.href, creating a navigation and resetting the limiter state to ALLOW_ONE Sites can repeatedly loop and trigger downloads in a new pop-up window, or use meta refresh tags to "ping-pong" between pages, downloading a new file one each page. Because the limiter is in the PROMPT state when the pending navigation arrives, the state is reset and the next download is always permitted. This CL closes the loophole by making the limiter maintain state following a renderer-initiated navigation, unless the state is allowing one or all downloads. This ensures that navigations which are not triggered by the user cannot be used to reset the limiter to allowing downloads if the state is either blocking downloads or prompting the user to permit downloads. A new IsRendererInitiated method is added to content::NavigationHandle to enable this functionality, with the boolean propagated from content::NavigationRequest and content::NavigationEntryImpl. The limiter is switched from overriding WebContentsObserver::DidNavigateMainFrame to WebContentsObserver::DidStartNavigation to use handle. BUG=559515 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Thanks for the feedback, I was definitely stumbling around in unfamiliar territory with this CL! On 2016/05/12 04:04:44, clamy wrote: > Thanks! A few comments and one question. My understanding from the CL > description is that you want to ensure that navigations that were not triggered > by the user cannot reset the state of the download limiter. However, some > renderer-initiated navigations are actually triggered by the user, for example > link clicks or form submits. Is it ok if they don't reset the state of the > download limiter? Uh oh. We definitely want user-triggered navigations to reset the state back to ALLOW_ONE if the state at navigation is PROMPT. Every time the user navigates, they should be allowed a download, unless they've explicitly blocked or allowed the site. This should be the only broken case here. Can we use the PageTransition or some other mechanism to tell these cases apart? Otherwise, we might not be able to use the renderer-initiated check here. :( https://codereview.chromium.org/1964863002/diff/60001/chrome/browser/download... File chrome/browser/download/download_request_limiter.cc (right): https://codereview.chromium.org/1964863002/diff/60001/chrome/browser/download... chrome/browser/download/download_request_limiter.cc:77: if (!navigation_handle->IsInMainFrame()) On 2016/05/11 21:50:03, Charlie Reis wrote: > Sanity check: Do we limit downloads in subframes somewhere else? There is one limiter per WebContents, and that limits subframe downloads too. The MainFrame check here ensures that navigations in subframes cannot reset the limiter: only eligible main frame navigations do. https://codereview.chromium.org/1964863002/diff/60001/chrome/browser/download... chrome/browser/download/download_request_limiter.cc:84: // content::IsBrowserSideNavigationEnabled() returns true). On 2016/05/11 21:50:03, Charlie Reis wrote: > This doesn't sound right. We should know IsRendererInitiated whether we're in > PlzNavigate or not. (NavigatorImpl::DidStartProvisionalLoad should be setting > it in the default mode.) This comment was added because I couldn't get the limiter unit test that checked the renderer initiated behaviour to pass unless I ran with --enable-browser-side-navigation. I've since figured out that I need an explicit commit to get the appropriate WebContentsObserver callback to fire, which I've now added. All the browser side navigation references have been removed. https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:128: // entries. See https://crbug.com/495161. On 2016/05/11 21:50:03, Charlie Reis wrote: > Yikes, looks like most of this was copied from update_entry_id_for_transfer, > which has a stale comment. I've recently fixed https://crbug.com/495161 and we > don't use a pending entry during transfer. (I'll have to see if I can remove > update_entry_id_for_transfer now.) > > I'm wondering if this is dead code as well. I'll try to find time to > investigate if I can, but let me know if you're aware of a case this is needed > for. I did just copy this from the update_entry_id_for_transfer; I don't really know enough about the navigation process to say whether or not this case is needed. Based on these comments I've removed the method and its caller. https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:269: bool is_renderer_initiated_; On 2016/05/12 04:04:44, clamy wrote: > I don't see a good reason why this should change over the course of the > navigation. This should be const. Done. https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_browsertest.cc:91: bool is_renderer_initiated() { return is_renderer_initiated_; } On 2016/05/11 21:50:03, Charlie Reis wrote: > Please actually check this in some of the tests. Hopefully we can observe it in > both states below. Done. https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_request.h:152: void CreateNavigationHandle(bool is_renderer_initiated, On 2016/05/12 04:04:44, clamy wrote: > Navigation request already has a boolean browser_initiated_ (ie > !is_renderer_initiated). Please use it instead of adding a new parameter to this > function. Done. https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:178: bool is_renderer_initiated = false; On 2016/05/11 21:50:03, Charlie Reis wrote: > Seems like we should default to true, with a comment about why. Reasoning: > > It's safer to assume a navigation is renderer-initiated unless we know > otherwise, since browser-initiated navigations are generally more powerful > (e.g., can go to chrome:// URLs). Browser-initiated navigations should always > have a pending NavigationEntry, so we should be able to detect if that's the > case here. > > (Can you add a test that would have failed with the way this is currently > written? I bet it would classify a subframe navigation as browser-initiated.) Makes sense, done. The test I added with subframe navigations does indeed fail with this set to false by default. :) https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1964863002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1064: bool is_renderer_initiated = false; On 2016/05/11 21:50:04, Charlie Reis wrote: > As before, better assume true unless we know otherwise. Done. https://codereview.chromium.org/1964863002/diff/60001/content/public/browser/... File content/public/browser/navigation_handle.cc (right): https://codereview.chromium.org/1964863002/diff/60001/content/public/browser/... content/public/browser/navigation_handle.cc:32: false, // is_renderer_initiated On 2016/05/11 21:50:04, Charlie Reis wrote: > Would it cause any problems to make this default to true? I'm not sure what > impact that has on existing tests. Happy to switch the default - I used false because it mirrored what callers passed by default into NavigationEntryImpl. https://codereview.chromium.org/1964863002/diff/60001/content/public/browser/... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1964863002/diff/60001/content/public/browser/... content/public/browser/navigation_handle.h:52: // * <meta http-equiv="refresh"> tag On 2016/05/11 21:50:04, Charlie Reis wrote: > nit: Let's be a bit more explicit that these are just two of many examples, > since I've noticed many people don't know the difference between e.g. and i.e. > :) > > Something like the the comment on IsSynchronousNavigation could work here: > > "...by the renderer process. Examples of renderer-initiated navigations > include: > * a link click > * a change to window.location.href > * <meta http-equiv="refresh"> tag" Done.
Description was changed from ========== Persist prompt/block download limiter state on renderer-initiated loads. The download request limiter will reset its state to allow a download when it receives a notification of a pending navigation. This ensures that the newly loaded page is always allowed to download one file. The reset does not occur if the current state allows or blocks all downloads AND the navigation is to the same host. This means that a site can avoid prompting the user for multiple downloads as follows: 1. trigger a download (moves state from ALLOW_ONE to PROMPT) 2. change window.location.href, creating a navigation and resetting the limiter state to ALLOW_ONE Sites can repeatedly loop and trigger downloads in a new pop-up window, or use meta refresh tags to "ping-pong" between pages, downloading a new file one each page. Because the limiter is in the PROMPT state when the pending navigation arrives, the state is reset and the next download is always permitted. This CL closes the loophole by making the limiter maintain state following a renderer-initiated navigation, unless the state is allowing one or all downloads. This ensures that navigations which are not triggered by the user cannot be used to reset the limiter to allowing downloads if the state is either blocking downloads or prompting the user to permit downloads. A new IsRendererInitiated method is added to content::NavigationHandle to enable this functionality, with the boolean propagated from content::NavigationRequest and content::NavigationEntryImpl. The limiter is switched from overriding WebContentsObserver::DidNavigateMainFrame to WebContentsObserver::DidStartNavigation to use handle. BUG=559515 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Persist prompt/block download limiter state on renderer-initiated loads. The download request limiter will reset its state to allow a download when it receives a notification of a pending navigation. This ensures that the newly loaded page is always allowed to download one file. The reset does not occur if the current state allows or blocks all downloads AND the navigation is to the same host. This means that a site can avoid prompting the user for multiple downloads as follows: 1. trigger a download (moves state from ALLOW_ONE to PROMPT) 2. change window.location.href, creating a navigation and resetting the limiter state to ALLOW_ONE Sites can repeatedly loop and trigger downloads in a new pop-up window, or use meta refresh tags to "ping-pong" between pages, downloading a new file on each page. Because the limiter is in the PROMPT state when the pending navigation arrives, the state is reset and the next download is always permitted. This CL closes the loophole by making the limiter maintain state following a renderer-initiated navigation, unless the state is allowing one or all downloads. This ensures that navigations which are not triggered by the user cannot be used to reset the limiter to allowing downloads if the state is either blocking downloads or prompting the user to permit downloads. A new IsRendererInitiated method is added to content::NavigationHandle to enable this functionality, with the boolean propagated from content::NavigationRequest and content::NavigationEntryImpl. The limiter is switched from overriding WebContentsObserver::DidNavigateMainFrame to WebContentsObserver::DidStartNavigation to use handle. BUG=559515 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Persist prompt/block download limiter state on renderer-initiated loads. The download request limiter will reset its state to allow a download when it receives a notification of a pending navigation. This ensures that the newly loaded page is always allowed to download one file. The reset does not occur if the current state allows or blocks all downloads AND the navigation is to the same host. This means that a site can avoid prompting the user for multiple downloads as follows: 1. trigger a download (moves state from ALLOW_ONE to PROMPT) 2. change window.location.href, creating a navigation and resetting the limiter state to ALLOW_ONE Sites can repeatedly loop and trigger downloads in a new pop-up window, or use meta refresh tags to "ping-pong" between pages, downloading a new file on each page. Because the limiter is in the PROMPT state when the pending navigation arrives, the state is reset and the next download is always permitted. This CL closes the loophole by making the limiter maintain state following a renderer-initiated navigation, unless the state is allowing one or all downloads. This ensures that navigations which are not triggered by the user cannot be used to reset the limiter to allowing downloads if the state is either blocking downloads or prompting the user to permit downloads. A new IsRendererInitiated method is added to content::NavigationHandle to enable this functionality, with the boolean propagated from content::NavigationRequest and content::NavigationEntryImpl. The limiter is switched from overriding WebContentsObserver::DidNavigateMainFrame to WebContentsObserver::DidStartNavigation to use handle. BUG=559515 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Persist prompt/block download limiter state on renderer-initiated loads. The download request limiter will reset its state to allow a download when it receives a notification of a pending navigation. This ensures that the newly loaded page is always allowed to download one file. The reset does not occur if the current state allows or blocks all downloads AND the navigation is to the same host. This means that a site can avoid prompting the user for multiple downloads as follows: 1. trigger a download (moves state from ALLOW_ONE to PROMPT) 2. change window.location.href, creating a navigation and resetting the limiter state to ALLOW_ONE Sites can repeatedly loop and trigger downloads in a new pop-up window, or use meta refresh tags to "ping-pong" between pages, downloading a new file on each page. Because the limiter is in the PROMPT state when the pending navigation arrives, the state is reset and the next download is always permitted. This CL closes the loophole by making the limiter maintain state following a renderer-initiated navigation, unless the state is allowing one or all downloads. This ensures that navigations which are not triggered by the user cannot be used to reset the limiter to allowing downloads if the state is either blocking downloads or prompting the user to permit downloads. A new IsRendererInitiated method is added to content::NavigationHandle to enable this functionality. The limiter now overrides WebContentsObserver::DidStartNavigation so it can access the handle. BUG=559515 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
You could check NavigationHandle::HasUserGesture, which should be true on a user renderer-initiated navigation to filter between non user-initiated and user initiated renderer navigations. IIRC this would be false on browser-initiated navigations so you may still need the renderer-initiated check as well to ensure that browser-initiated navigations do reset the download limiter (we generally assume that browser-initiated navigations are user initiated).
On 2016/05/12 04:52:55, clamy wrote: > You could check NavigationHandle::HasUserGesture, which should be true on a user > renderer-initiated navigation to filter between non user-initiated and user > initiated renderer navigations. IIRC this would be false on browser-initiated > navigations so you may still need the renderer-initiated check as well to ensure > that browser-initiated navigations do reset the download limiter (we generally > assume that browser-initiated navigations are user initiated). Thanks! Having thought about this some more, link clicks and form submits (and other user-initiated navigations) trigger DidGetUserInteraction, which will reset the limiter anyway. So the behaviour in #ps5 should be sufficient without needing to further look at user gestures from the NavigationHandle. @asanka, do you agree with this? @clamy, creis: otherwise, let me know if there's anything else in the CL that you'd like me to do on the content/ side, or if it looks okay now with the additional tests and comments addressed. Thanks!
Thanks! content/ LGTM, with one question you or asanka@ might be able to answer. https://codereview.chromium.org/1964863002/diff/80001/chrome/browser/download... File chrome/browser/download/download_request_limiter.cc (right): https://codereview.chromium.org/1964863002/diff/80001/chrome/browser/download... chrome/browser/download/download_request_limiter.cc:78: // refresh can't be abused to avoid the limiter. Might mention where renderer-initiated navigations with user gestures reset the state, in case that question comes up again. https://codereview.chromium.org/1964863002/diff/80001/chrome/browser/download... chrome/browser/download/download_request_limiter.cc:90: navigation_handle->GetURL().host() == initial_page_host_) { Host comparisons always catch my eye, since there's a long list of corner cases. :) Is this something that could be gamed if the site navigated to about:blank (which is considered same-origin) or a data URL? I suppose that would only be an attack for renderer-initiated navigations, though. (Also, I suppose this is just a move from below, so I won't ask you to fix it in this CL.)
Thanks! @asanka: ping https://codereview.chromium.org/1964863002/diff/80001/chrome/browser/download... File chrome/browser/download/download_request_limiter.cc (right): https://codereview.chromium.org/1964863002/diff/80001/chrome/browser/download... chrome/browser/download/download_request_limiter.cc:78: // refresh can't be abused to avoid the limiter. On 2016/05/13 00:36:12, Charlie Reis (slow to reply) wrote: > Might mention where renderer-initiated navigations with user gestures reset the > state, in case that question comes up again. Done. https://codereview.chromium.org/1964863002/diff/80001/chrome/browser/download... chrome/browser/download/download_request_limiter.cc:90: navigation_handle->GetURL().host() == initial_page_host_) { On 2016/05/13 00:36:12, Charlie Reis (slow to reply) wrote: > Host comparisons always catch my eye, since there's a long list of corner cases. > :) Is this something that could be gamed if the site navigated to about:blank > (which is considered same-origin) or a data URL? I suppose that would only be > an attack for renderer-initiated navigations, though. > > (Also, I suppose this is just a move from below, so I won't ask you to fix it in > this CL.) I think the fact that a prompt/block state is persisted for renderer-initiated navigations should account for sites which navigate to data URLs or about:blank. asanka@, do you have anything to add here?
Thanks! Lgtm with one nit. https://codereview.chromium.org/1964863002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1964863002/diff/100001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:179: // otherwise. Browser navigations are generally more powerful, and they should nit: "navigations are more powerful" sounds a bit weird to me. How about "have more privileges" or "have priority over renderer-initiated navigations"?
Description was changed from ========== Persist prompt/block download limiter state on renderer-initiated loads. The download request limiter will reset its state to allow a download when it receives a notification of a pending navigation. This ensures that the newly loaded page is always allowed to download one file. The reset does not occur if the current state allows or blocks all downloads AND the navigation is to the same host. This means that a site can avoid prompting the user for multiple downloads as follows: 1. trigger a download (moves state from ALLOW_ONE to PROMPT) 2. change window.location.href, creating a navigation and resetting the limiter state to ALLOW_ONE Sites can repeatedly loop and trigger downloads in a new pop-up window, or use meta refresh tags to "ping-pong" between pages, downloading a new file on each page. Because the limiter is in the PROMPT state when the pending navigation arrives, the state is reset and the next download is always permitted. This CL closes the loophole by making the limiter maintain state following a renderer-initiated navigation, unless the state is allowing one or all downloads. This ensures that navigations which are not triggered by the user cannot be used to reset the limiter to allowing downloads if the state is either blocking downloads or prompting the user to permit downloads. A new IsRendererInitiated method is added to content::NavigationHandle to enable this functionality. The limiter now overrides WebContentsObserver::DidStartNavigation so it can access the handle. BUG=559515 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Persist prompt/block download limiter state on renderer-initiated loads. The download request limiter will reset its state to allow a download when it receives a notification of a pending navigation. This ensures that the newly loaded page is always allowed to download one file. The reset does not occur if the current state allows or blocks all downloads AND the navigation is to the same host. This means that a site can avoid prompting the user for multiple downloads as follows: 1. trigger a download (moves state from ALLOW_ONE to PROMPT) 2. change window.location.href, creating a navigation and resetting the limiter state to ALLOW_ONE Sites can repeatedly loop and trigger downloads in a new pop-up window, or use meta refresh tags to "ping-pong" between pages, downloading a new file on each page. Because the limiter is in the PROMPT state when the pending navigation arrives, the state is reset and the next download is always permitted. This CL closes the loophole by making the limiter maintain state following a renderer-initiated navigation, unless the state is allowing one or all downloads. This ensures that navigations which are not triggered by the user cannot be used to reset the limiter to allowing downloads if the state is either blocking downloads or prompting the user to permit downloads. Both carpet bomb attacks described above are mitigated in the CL. A new IsRendererInitiated method is added to content::NavigationHandle to enable this functionality. The limiter now overrides WebContentsObserver::DidStartNavigation so it can access the handle. BUG=559515 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
lgtm % nits and test suggestion. Sorry about the delay. https://codereview.chromium.org/1964863002/diff/80001/chrome/browser/download... File chrome/browser/download/download_request_limiter.cc (right): https://codereview.chromium.org/1964863002/diff/80001/chrome/browser/download... chrome/browser/download/download_request_limiter.cc:90: navigation_handle->GetURL().host() == initial_page_host_) { On 2016/05/17 at 01:31:48, dominickn wrote: > On 2016/05/13 00:36:12, Charlie Reis (slow to reply) wrote: > > Host comparisons always catch my eye, since there's a long list of corner cases. > > :) Is this something that could be gamed if the site navigated to about:blank > > (which is considered same-origin) or a data URL? I suppose that would only be > > an attack for renderer-initiated navigations, though. > > > > (Also, I suppose this is just a move from below, so I won't ask you to fix it in > > this CL.) > > I think the fact that a prompt/block state is persisted for renderer-initiated navigations should account for sites which navigate to data URLs or about:blank. asanka@, do you have anything to add here? I believe this is safe. If the navigation was renderer intiated, then the status_ is ALLOW_ALL_DOWNLOADS, in which case resetting the tab download state will only hurt the attacker (i.e. they'd be better off not navigating). Often overlooked navigation targets like data: and blob: URLs (is the latter even possible?) should yield an empty host() which should cause the above condition to fail. https://codereview.chromium.org/1964863002/diff/100001/chrome/browser/downloa... File chrome/browser/download/download_request_limiter.cc (right): https://codereview.chromium.org/1964863002/diff/100001/chrome/browser/downloa... chrome/browser/download/download_request_limiter.cc:88: // User has either allowed all downloads or canceled all downloads. Only nit: s/canceled/blocked/ https://codereview.chromium.org/1964863002/diff/100001/chrome/browser/downloa... chrome/browser/download/download_request_limiter.cc:241: HostContentSettingsMap* content_settings = GetContentSettings(contents); Nit: return early if !content_settings https://codereview.chromium.org/1964863002/diff/100001/chrome/browser/downloa... File chrome/browser/download/download_request_limiter_unittest.cc (right): https://codereview.chromium.org/1964863002/diff/100001/chrome/browser/downloa... chrome/browser/download/download_request_limiter_unittest.cc:355: TEST_F(DownloadRequestLimiterTest, DownloadRequestLimiter_RendererInitiated) { Could you add a test for navigation in an iframe?
Thanks! https://codereview.chromium.org/1964863002/diff/100001/chrome/browser/downloa... File chrome/browser/download/download_request_limiter.cc (right): https://codereview.chromium.org/1964863002/diff/100001/chrome/browser/downloa... chrome/browser/download/download_request_limiter.cc:88: // User has either allowed all downloads or canceled all downloads. Only On 2016/05/19 02:18:30, asanka wrote: > nit: s/canceled/blocked/ Done. https://codereview.chromium.org/1964863002/diff/100001/chrome/browser/downloa... chrome/browser/download/download_request_limiter.cc:241: HostContentSettingsMap* content_settings = GetContentSettings(contents); On 2016/05/19 02:18:30, asanka wrote: > Nit: return early if !content_settings Done. https://codereview.chromium.org/1964863002/diff/100001/chrome/browser/downloa... File chrome/browser/download/download_request_limiter_unittest.cc (right): https://codereview.chromium.org/1964863002/diff/100001/chrome/browser/downloa... chrome/browser/download/download_request_limiter_unittest.cc:355: TEST_F(DownloadRequestLimiterTest, DownloadRequestLimiter_RendererInitiated) { On 2016/05/19 02:18:30, asanka wrote: > Could you add a test for navigation in an iframe? Done. https://codereview.chromium.org/1964863002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1964863002/diff/100001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:179: // otherwise. Browser navigations are generally more powerful, and they should On 2016/05/18 13:53:36, clamy wrote: > nit: "navigations are more powerful" sounds a bit weird to me. How about "have > more privileges" or "have priority over renderer-initiated navigations"? Done.
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, clamy@chromium.org, asanka@chromium.org Link to the patchset: https://codereview.chromium.org/1964863002/#ps140001 (title: "Adding subframe limiter test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1964863002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1964863002/140001
Message was sent while issue was closed.
Description was changed from ========== Persist prompt/block download limiter state on renderer-initiated loads. The download request limiter will reset its state to allow a download when it receives a notification of a pending navigation. This ensures that the newly loaded page is always allowed to download one file. The reset does not occur if the current state allows or blocks all downloads AND the navigation is to the same host. This means that a site can avoid prompting the user for multiple downloads as follows: 1. trigger a download (moves state from ALLOW_ONE to PROMPT) 2. change window.location.href, creating a navigation and resetting the limiter state to ALLOW_ONE Sites can repeatedly loop and trigger downloads in a new pop-up window, or use meta refresh tags to "ping-pong" between pages, downloading a new file on each page. Because the limiter is in the PROMPT state when the pending navigation arrives, the state is reset and the next download is always permitted. This CL closes the loophole by making the limiter maintain state following a renderer-initiated navigation, unless the state is allowing one or all downloads. This ensures that navigations which are not triggered by the user cannot be used to reset the limiter to allowing downloads if the state is either blocking downloads or prompting the user to permit downloads. Both carpet bomb attacks described above are mitigated in the CL. A new IsRendererInitiated method is added to content::NavigationHandle to enable this functionality. The limiter now overrides WebContentsObserver::DidStartNavigation so it can access the handle. BUG=559515 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Persist prompt/block download limiter state on renderer-initiated loads. The download request limiter will reset its state to allow a download when it receives a notification of a pending navigation. This ensures that the newly loaded page is always allowed to download one file. The reset does not occur if the current state allows or blocks all downloads AND the navigation is to the same host. This means that a site can avoid prompting the user for multiple downloads as follows: 1. trigger a download (moves state from ALLOW_ONE to PROMPT) 2. change window.location.href, creating a navigation and resetting the limiter state to ALLOW_ONE Sites can repeatedly loop and trigger downloads in a new pop-up window, or use meta refresh tags to "ping-pong" between pages, downloading a new file on each page. Because the limiter is in the PROMPT state when the pending navigation arrives, the state is reset and the next download is always permitted. This CL closes the loophole by making the limiter maintain state following a renderer-initiated navigation, unless the state is allowing one or all downloads. This ensures that navigations which are not triggered by the user cannot be used to reset the limiter to allowing downloads if the state is either blocking downloads or prompting the user to permit downloads. Both carpet bomb attacks described above are mitigated in the CL. A new IsRendererInitiated method is added to content::NavigationHandle to enable this functionality. The limiter now overrides WebContentsObserver::DidStartNavigation so it can access the handle. BUG=559515 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Persist prompt/block download limiter state on renderer-initiated loads. The download request limiter will reset its state to allow a download when it receives a notification of a pending navigation. This ensures that the newly loaded page is always allowed to download one file. The reset does not occur if the current state allows or blocks all downloads AND the navigation is to the same host. This means that a site can avoid prompting the user for multiple downloads as follows: 1. trigger a download (moves state from ALLOW_ONE to PROMPT) 2. change window.location.href, creating a navigation and resetting the limiter state to ALLOW_ONE Sites can repeatedly loop and trigger downloads in a new pop-up window, or use meta refresh tags to "ping-pong" between pages, downloading a new file on each page. Because the limiter is in the PROMPT state when the pending navigation arrives, the state is reset and the next download is always permitted. This CL closes the loophole by making the limiter maintain state following a renderer-initiated navigation, unless the state is allowing one or all downloads. This ensures that navigations which are not triggered by the user cannot be used to reset the limiter to allowing downloads if the state is either blocking downloads or prompting the user to permit downloads. Both carpet bomb attacks described above are mitigated in the CL. A new IsRendererInitiated method is added to content::NavigationHandle to enable this functionality. The limiter now overrides WebContentsObserver::DidStartNavigation so it can access the handle. BUG=559515 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Persist prompt/block download limiter state on renderer-initiated loads. The download request limiter will reset its state to allow a download when it receives a notification of a pending navigation. This ensures that the newly loaded page is always allowed to download one file. The reset does not occur if the current state allows or blocks all downloads AND the navigation is to the same host. This means that a site can avoid prompting the user for multiple downloads as follows: 1. trigger a download (moves state from ALLOW_ONE to PROMPT) 2. change window.location.href, creating a navigation and resetting the limiter state to ALLOW_ONE Sites can repeatedly loop and trigger downloads in a new pop-up window, or use meta refresh tags to "ping-pong" between pages, downloading a new file on each page. Because the limiter is in the PROMPT state when the pending navigation arrives, the state is reset and the next download is always permitted. This CL closes the loophole by making the limiter maintain state following a renderer-initiated navigation, unless the state is allowing one or all downloads. This ensures that navigations which are not triggered by the user cannot be used to reset the limiter to allowing downloads if the state is either blocking downloads or prompting the user to permit downloads. Both carpet bomb attacks described above are mitigated in the CL. A new IsRendererInitiated method is added to content::NavigationHandle to enable this functionality. The limiter now overrides WebContentsObserver::DidStartNavigation so it can access the handle. BUG=559515 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/b38c9f6740d73bd4e40bc197c3983739bbeec0ba Cr-Commit-Position: refs/heads/master@{#394721} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b38c9f6740d73bd4e40bc197c3983739bbeec0ba Cr-Commit-Position: refs/heads/master@{#394721}
Message was sent while issue was closed.
One thing I forgot to ask about this CL: Does it correctly deal with workflows like the following (assume no iframes): a) search for something. b) click on a search result, get taken to a different host c) click on link that initiates a download via <a download> d) hit "back" e) goto b I'm assuming the 'renderer initiated' flag is true for all the navigations except d) ? Does d count as a navigation and would it reset the tab download state? Also, what if it was something like this: a) go to some link farm b) click on one link, get taken to a different host c) click on link that initiates a download via <a download> d) click on 'back to link farm' link e) goto b In this case, what would cause the tab download state to be reset?
Message was sent while issue was closed.
On 2016/05/19 14:05:33, asanka wrote: > One thing I forgot to ask about this CL: > > Does it correctly deal with workflows like the following (assume no iframes): > > a) search for something. > b) click on a search result, get taken to a different host > c) click on link that initiates a download via <a download> > d) hit "back" > e) goto b > > I'm assuming the 'renderer initiated' flag is true for all the navigations > except d) ? Does d count as a navigation and would it reset the tab download > state? > > Also, what if it was something like this: > > a) go to some link farm > b) click on one link, get taken to a different host > c) click on link that initiates a download via <a download> > d) click on 'back to link farm' link > e) goto b > > In this case, what would cause the tab download state to be reset? In these cases, clicking on a link triggers TabDownloadState::DidGetUserInteraction, which resets the limiter. Any link click by the user should reset the limiter
Message was sent while issue was closed.
On 2016/05/19 at 14:42:42, dominickn wrote: > On 2016/05/19 14:05:33, asanka wrote: > > One thing I forgot to ask about this CL: > > > > Does it correctly deal with workflows like the following (assume no iframes): > > > > a) search for something. > > b) click on a search result, get taken to a different host > > c) click on link that initiates a download via <a download> > > d) hit "back" > > e) goto b > > > > I'm assuming the 'renderer initiated' flag is true for all the navigations > > except d) ? Does d count as a navigation and would it reset the tab download > > state? > > > > Also, what if it was something like this: > > > > a) go to some link farm > > b) click on one link, get taken to a different host > > c) click on link that initiates a download via <a download> > > d) click on 'back to link farm' link > > e) goto b > > > > In this case, what would cause the tab download state to be reset? > > In these cases, clicking on a link triggers TabDownloadState::DidGetUserInteraction, which resets the limiter. Any link click by the user should reset the limiter Ah. Excellent. It might be worth pointing that out in the handling of Did{Start,Finish}Navigation that even if the per-tab state is preserved by the handler, it may already have been reset by the DidGetUserInteraction handler if any user interactions were involved. In particular, someone trying to figure out what happens during a link click may (as I did) overlook that the logic is split between the three handlers.
Message was sent while issue was closed.
On 2016/05/19 15:19:28, asanka wrote: > On 2016/05/19 at 14:42:42, dominickn wrote: > > On 2016/05/19 14:05:33, asanka wrote: > > > One thing I forgot to ask about this CL: > > > > > > Does it correctly deal with workflows like the following (assume no > iframes): > > > > > > a) search for something. > > > b) click on a search result, get taken to a different host > > > c) click on link that initiates a download via <a download> > > > d) hit "back" > > > e) goto b > > > > > > I'm assuming the 'renderer initiated' flag is true for all the navigations > > > except d) ? Does d count as a navigation and would it reset the tab download > > > state? > > > > > > Also, what if it was something like this: > > > > > > a) go to some link farm > > > b) click on one link, get taken to a different host > > > c) click on link that initiates a download via <a download> > > > d) click on 'back to link farm' link > > > e) goto b > > > > > > In this case, what would cause the tab download state to be reset? > > > > In these cases, clicking on a link triggers > TabDownloadState::DidGetUserInteraction, which resets the limiter. Any link > click by the user should reset the limiter > > Ah. Excellent. It might be worth pointing that out in the handling of > Did{Start,Finish}Navigation that even if the per-tab state is preserved by the > handler, it may already have been reset by the DidGetUserInteraction handler if > any user interactions were involved. In particular, someone trying to figure out > what happens during a link click may (as I did) overlook that the logic is split > between the three handlers. Yep, there's a comment (in the large block of comments) in DidStartNavigation referring to DidGetUserInteraction. Next change I make to the limiter I'll beef it up a little. :) |