Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(75)

Issue 1964863002: Persist prompt/block download limiter state on renderer-initiated loads. (Closed)

Created:
4 years, 7 months ago by dominickn
Modified:
4 years, 7 months ago
Reviewers:
asanka, clamy, Charlie Reis
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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+367 lines, -140 lines) Patch
M chrome/browser/download/download_request_limiter.h View 1 2 3 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/browser/download/download_request_limiter.cc View 1 2 3 4 5 6 7 14 chunks +110 lines, -127 lines 0 comments Download
M chrome/browser/download/download_request_limiter_unittest.cc View 1 2 3 4 5 6 7 4 chunks +135 lines, -1 line 0 comments Download
M content/browser/frame_host/navigation_handle_impl.h View 1 2 3 4 5 4 chunks +4 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigation_handle_impl.cc View 1 2 3 4 5 3 chunks +10 lines, -2 lines 0 comments Download
M content/browser/frame_host/navigation_handle_impl_browsertest.cc View 1 2 3 4 6 chunks +72 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigation_handle_impl_unittest.cc View 1 2 3 4 1 chunk +3 lines, -3 lines 0 comments Download
M content/browser/frame_host/navigation_request.cc View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/frame_host/navigator_impl.cc View 1 2 3 4 5 6 7 1 chunk +12 lines, -2 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 5 6 7 4 chunks +7 lines, -2 lines 0 comments Download
M content/public/browser/navigation_handle.h View 1 2 3 4 5 1 chunk +8 lines, -0 lines 0 comments Download
M content/public/browser/navigation_handle.cc View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 31 (13 generated)
dominickn
Take 2, this time plumbing in is_renderer_initiated onto NavigationHandle. asanka: PTAL at the download request ...
4 years, 7 months ago (2016-05-11 03:01:27 UTC) #6
Charlie Reis
Thanks for rewriting! Some thoughts below. On 2016/05/11 03:01:27, dominickn wrote: > Take 2, this ...
4 years, 7 months ago (2016-05-11 21:50:04 UTC) #7
clamy
Thanks! A few comments and one question. My understanding from the CL description is that ...
4 years, 7 months ago (2016-05-12 04:04:44 UTC) #8
dominickn
Thanks for the feedback, I was definitely stumbling around in unfamiliar territory with this CL! ...
4 years, 7 months ago (2016-05-12 04:22:58 UTC) #10
clamy
You could check NavigationHandle::HasUserGesture, which should be true on a user renderer-initiated navigation to filter ...
4 years, 7 months ago (2016-05-12 04:52:55 UTC) #13
dominickn
On 2016/05/12 04:52:55, clamy wrote: > You could check NavigationHandle::HasUserGesture, which should be true on ...
4 years, 7 months ago (2016-05-12 05:13:57 UTC) #14
Charlie Reis
Thanks! content/ LGTM, with one question you or asanka@ might be able to answer. https://codereview.chromium.org/1964863002/diff/80001/chrome/browser/download/download_request_limiter.cc ...
4 years, 7 months ago (2016-05-13 00:36:12 UTC) #15
dominickn
Thanks! @asanka: ping https://codereview.chromium.org/1964863002/diff/80001/chrome/browser/download/download_request_limiter.cc File chrome/browser/download/download_request_limiter.cc (right): https://codereview.chromium.org/1964863002/diff/80001/chrome/browser/download/download_request_limiter.cc#newcode78 chrome/browser/download/download_request_limiter.cc:78: // refresh can't be abused to ...
4 years, 7 months ago (2016-05-17 01:31:48 UTC) #16
clamy
Thanks! Lgtm with one nit. https://codereview.chromium.org/1964863002/diff/100001/content/browser/frame_host/navigator_impl.cc File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1964863002/diff/100001/content/browser/frame_host/navigator_impl.cc#newcode179 content/browser/frame_host/navigator_impl.cc:179: // otherwise. Browser navigations ...
4 years, 7 months ago (2016-05-18 13:53:36 UTC) #17
asanka
lgtm % nits and test suggestion. Sorry about the delay. https://codereview.chromium.org/1964863002/diff/80001/chrome/browser/download/download_request_limiter.cc File chrome/browser/download/download_request_limiter.cc (right): https://codereview.chromium.org/1964863002/diff/80001/chrome/browser/download/download_request_limiter.cc#newcode90 ...
4 years, 7 months ago (2016-05-19 02:18:30 UTC) #19
dominickn
Thanks! https://codereview.chromium.org/1964863002/diff/100001/chrome/browser/download/download_request_limiter.cc File chrome/browser/download/download_request_limiter.cc (right): https://codereview.chromium.org/1964863002/diff/100001/chrome/browser/download/download_request_limiter.cc#newcode88 chrome/browser/download/download_request_limiter.cc:88: // User has either allowed all downloads or ...
4 years, 7 months ago (2016-05-19 07:25:45 UTC) #20
commit-bot: I haz the power
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
4 years, 7 months ago (2016-05-19 07:26:31 UTC) #23
commit-bot: I haz the power
Committed patchset #8 (id:140001)
4 years, 7 months ago (2016-05-19 08:53:44 UTC) #25
commit-bot: I haz the power
Patchset 8 (id:??) landed as https://crrev.com/b38c9f6740d73bd4e40bc197c3983739bbeec0ba Cr-Commit-Position: refs/heads/master@{#394721}
4 years, 7 months ago (2016-05-19 08:55:30 UTC) #27
asanka
One thing I forgot to ask about this CL: Does it correctly deal with workflows ...
4 years, 7 months ago (2016-05-19 14:05:33 UTC) #28
dominickn
On 2016/05/19 14:05:33, asanka wrote: > One thing I forgot to ask about this CL: ...
4 years, 7 months ago (2016-05-19 14:42:42 UTC) #29
asanka
On 2016/05/19 at 14:42:42, dominickn wrote: > On 2016/05/19 14:05:33, asanka wrote: > > One ...
4 years, 7 months ago (2016-05-19 15:19:28 UTC) #30
dominickn
4 years, 7 months ago (2016-05-19 23:30:46 UTC) #31
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. :)

Powered by Google App Engine
This is Rietveld 408576698