For downloads requiring a user gesture, also require...

For downloads requiring a user gesture, also require the user to have visited the site before today, to hamper attackers.

BUG=81741
TEST=.exe downloads on Windows triggered by an explicit link click should prompt you to confirm iff they are hosted on a site you have not visited before today.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86518

[Randy Smith (Not in Mondays), 2011-05-24 18:28:25]

Initial round of review.  My primary comment is around the fork/join semantics,
but a couple of other things may follow from that.

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
File chrome/browser/download/download_history.cc (left):

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
chrome/browser/download/download_history.cc:55: // TODO(paulg) see bug 958058.
EXPLICIT_ACCESS below is wrong.
What's the context for this elimination?  I was going to look at the bug, but it
looks large to be in our issue tracker.

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
chrome/browser/download/download_history.h:35: // Checks whether |info|'s
referrer has been visited before today.
|info| looks like a typo/left over from previous CL.

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
chrome/browser/download/download_item.h:280: void SetFileDangerous(bool
dangerous);
This accessor doesn't look like it's used outside of tests.  Could we get rid of
it?

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
chrome/browser/download/download_item.h:281: void SetUrlDangerous(bool
dangerous);
This may be an area where we disagree, but I'm trying to limit the "bag of bits"
feeling about the DownloadItem, and hence I'd rather have exposed as accessor
only the state transition that the item actually undergoes.  So (assuming you
can get rid of the TriState enum as per my other comment) would you be willing
to make this accessor only do the transition from safe to dangerous?

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
chrome/browser/download/download_item.h:282: void SetVisitedBefore(bool
visited_referrer_before);
Similarly, could you drop the argument and just call this if you need to mark
the DownloadItem as visited before?  (Or not visited before; I don't care what
the default state is.)

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
chrome/browser/download/download_manager.cc:288: if (download) {
It's a personal style preference (which probably means you can quote me chapter
and versus in the style guide against it :-}) but I like to have "the main path"
outside of conditionals, all other things being equal.  So my preference would
be to retain the "if (!download) return;" pattern, though if you feel strongly I
won't stop the change.

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
chrome/browser/download/download_manager.cc:294: void
DownloadManager::HistoryAndSBChecksMaybeDone(DownloadItem* download) {
One of the biggest things we've done in the past couple of months is to de-race
the download system down to the absolute minimum of races--that was a big source
of the flakiness that had the tests disabled for so long.  For that reason, I
want to keep to an absolute minium the number of places where we do things in
parallel and "join back up" later (as this function does).  My evaluation was
that the only really really important things to do in parallel were the data
download and the user interaction, since either of those could take an arbitrary
time.  So we've made just about everything linear along the user interaction
path and made the data download go in parallel with that.

So unless you have some reason to think that there's a major advantage to doing
the fork/join around the history and safe browsing checks, could we keep the
flow linear?  

(I'm hopeful that that also means you can ditch the TriState enum, since you'll
know by what code you're in whether or not you've done the check.)

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
chrome/browser/download/download_manager.h:240: void
CheckVisitedBeforeDone(int32 download_id, bool visited_before);
I don't understand the meaning of |visited_before| (either here or looking at
the code) which suggests that it needs a better comment.

[Peter Kasting, 2011-05-24 20:55:00]

New snap up.  All comments addressed except as noted below.

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
File chrome/browser/download/download_history.cc (left):

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
chrome/browser/download/download_history.cc:55: // TODO(paulg) see bug 958058.
EXPLICIT_ACCESS below is wrong.
On 2011/05/24 18:28:25, rdsmith wrote:
> What's the context for this elimination?  I was going to look at the bug, but
it
> looks large to be in our issue tracker.

The bug is from the internal Google tracker.  This dates from before when we had
proper incognito mode handling for downloads.  Now that we have such handling
these comments are no longer accurate or necessary.

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
chrome/browser/download/download_item.h:280: void SetFileDangerous(bool
dangerous);
On 2011/05/24 18:28:25, rdsmith wrote:
> This accessor doesn't look like it's used outside of tests.  Could we get rid
of
> it?

You're right that it's only used in tests, but how can I get rid of it?  Do you
want me to add friend declarations to this class?

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
chrome/browser/download/download_manager.h:240: void
CheckVisitedBeforeDone(int32 download_id, bool visited_before);
On 2011/05/24 18:28:25, rdsmith wrote:
> I don't understand the meaning of |visited_before| (either here or looking at
> the code) which suggests that it needs a better comment.

I added the word "referrer" to the function and argument names.  See if that
helps.

[Randy Smith (Not in Mondays), 2011-05-24 21:48:56]

I think I'm good with this.  LGTM.

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/7065015/diff/2004/chrome/browser/download/down...
chrome/browser/download/download_item.h:280: void SetFileDangerous(bool
dangerous);
On 2011/05/24 20:55:00, Peter Kasting wrote:
> On 2011/05/24 18:28:25, rdsmith wrote:
> > This accessor doesn't look like it's used outside of tests.  Could we get
rid
> of
> > it?
> 
> You're right that it's only used in tests, but how can I get rid of it?  Do
you
> want me to add friend declarations to this class?

You could use SetFileCheckResults based on a DownloadStateInfo structure; that's
the way that it gets set in normal processing.  But that would probably take
some re-working of the test, and I think the reason for this accessor is that
you're getting rid of superfluous members in DownloadCreateInfo, which is gravy
from my point of view even with the extra accessor in DownloadItem, so don't
worry about it unless you feel an urge to do the extra cleanup.