Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(66)

Issue 1165893004: [Downloads] Prevent dangerous files from being opened automatically. (Closed)

Created:
4 years, 2 months ago by asanka
Modified:
4 years, 2 months ago
CC:
benjhayden+dwatch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Downloads] Prevent executable files from being opened automatically. Users can accidentally mark executable or other dangerous file types for opening automatically upon download. This change prevents such file types from being considered for auto open. If the user already has any dangerous file types in the auto-open list, those entries will be ignored on new browser sessions. Any action that causes the persisted auto-open list to be updated will, as a side-effect, premanently remove any dangerous file types that were there. Currently the list of file types that are excluded from auto open is maintained by chrome/browser/download/download_extension.cc. BUG=461858 TEST=(1) Download an .exe (on Windows) or .swf file. (2) Verify that the "Always open files of this type" option is disabled. Committed: https://crrev.com/39841e54180e2583dffa16fbbb9b99fd293821d0 Cr-Commit-Position: refs/heads/master@{#334677}

Patch Set 1 #

Total comments: 5

Patch Set 2 : Fix comment #

Patch Set 3 : std::string::front() is a C++11 feature. Don't use it yet. #

Patch Set 4 : Use a blacklist instead of including all dangerous file types. #

Total comments: 24

Patch Set 5 : Fix comment #

Total comments: 5

Patch Set 6 : Add more details about file types and fiddle with no auto open flag. #

Patch Set 7 : Update comment. #

Total comments: 8

Patch Set 8 : Clarify comments. #

Patch Set 9 : #

Patch Set 10 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+456 lines, -138 lines) Patch
M chrome/browser/download/download_commands.cc View 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/download/download_extensions.h View 1 2 3 4 5 6 7 1 chunk +28 lines, -0 lines 0 comments Download
M chrome/browser/download/download_extensions.cc View 1 2 3 4 5 6 7 3 chunks +277 lines, -129 lines 0 comments Download
M chrome/browser/download/download_prefs.h View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -2 lines 0 comments Download
M chrome/browser/download/download_prefs.cc View 1 2 3 4 2 chunks +24 lines, -7 lines 0 comments Download
A chrome/browser/download/download_prefs_unittest.cc View 1 2 3 4 5 1 chunk +117 lines, -0 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 23 (5 generated)
asanka
4 years, 2 months ago (2015-06-03 22:23:53 UTC) #2
Randy Smith (Not in Mondays)
On 2015/06/03 22:23:53, asanka wrote: *chuckle* CC me on the bug so I can look ...
4 years, 2 months ago (2015-06-03 22:29:42 UTC) #3
Randy Smith (Not in Mondays)
First round; high level behavior and nits. https://codereview.chromium.org/1165893004/diff/1/chrome/browser/download/download_extensions.cc File chrome/browser/download/download_extensions.cc (right): https://codereview.chromium.org/1165893004/diff/1/chrome/browser/download/download_extensions.cc#newcode235 chrome/browser/download/download_extensions.cc:235: return !extension.empty() ...
4 years, 2 months ago (2015-06-03 23:44:08 UTC) #4
asanka
https://codereview.chromium.org/1165893004/diff/1/chrome/browser/download/download_extensions.cc File chrome/browser/download/download_extensions.cc (right): https://codereview.chromium.org/1165893004/diff/1/chrome/browser/download/download_extensions.cc#newcode235 chrome/browser/download/download_extensions.cc:235: return !extension.empty() && GetFileDangerLevel(path) == NOT_DANGEROUS; On 2015/06/03 at ...
4 years, 2 months ago (2015-06-04 15:30:48 UTC) #5
Randy Smith (Not in Mondays)
Quick note: Asanka and I spoke offline, and he's agreed to explore an explicit blacklist ...
4 years, 2 months ago (2015-06-04 19:06:23 UTC) #6
asanka
4 years, 2 months ago (2015-06-05 16:17:34 UTC) #8
asanka
On 2015/06/05 at 16:17:34, asanka wrote: > Right. Forgot to add a comment. The latest ...
4 years, 2 months ago (2015-06-05 19:01:48 UTC) #9
palmer
LGTM modulo nits and a potentially off-base question about alternative data streams. https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download/download_extensions.cc File chrome/browser/download/download_extensions.cc ...
4 years, 2 months ago (2015-06-08 19:05:08 UTC) #10
asanka
https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download/download_extensions.cc File chrome/browser/download/download_extensions.cc (right): https://codereview.chromium.org/1165893004/diff/60001/chrome/browser/download/download_extensions.cc#newcode230 chrome/browser/download/download_extensions.cc:230: {"dex", ALLOW_ON_USER_GESTURE | EXCLUDE_FROM_AUTO_OPEN}, On 2015/06/08 at 19:05:07, palmer ...
4 years, 2 months ago (2015-06-09 00:38:10 UTC) #11
palmer
> Hm. We are making the following assumptions: > > 1. Chrome can only auto-open ...
4 years, 2 months ago (2015-06-09 01:06:20 UTC) #12
Randy Smith (Not in Mondays)
I apologize for my slowness of response on this review. This is a partial review ...
4 years, 2 months ago (2015-06-09 19:40:04 UTC) #13
Randy Smith (Not in Mondays)
Remaining comments. https://codereview.chromium.org/1165893004/diff/80001/chrome/browser/download/download_prefs_unittest.cc File chrome/browser/download/download_prefs_unittest.cc (right): https://codereview.chromium.org/1165893004/diff/80001/chrome/browser/download/download_prefs_unittest.cc#newcode25 chrome/browser/download/download_prefs_unittest.cc:25: const base::FilePath kFileWithNoExtension(FILE_PATH_LITERAL("abcd")); This doesn't look like ...
4 years, 2 months ago (2015-06-09 21:29:25 UTC) #14
asanka
Have another look? Rather than explain in the CR, I've attempted to summarize the criteria ...
4 years, 2 months ago (2015-06-12 20:12:53 UTC) #16
Randy Smith (Not in Mondays)
LGTM; all comments below are suggestions that you can do with as you wish. https://codereview.chromium.org/1165893004/diff/80001/chrome/browser/download/download_prefs_unittest.cc ...
4 years, 2 months ago (2015-06-15 19:19:17 UTC) #17
asanka
https://codereview.chromium.org/1165893004/diff/140001/chrome/browser/download/download_extensions.cc File chrome/browser/download/download_extensions.cc (right): https://codereview.chromium.org/1165893004/diff/140001/chrome/browser/download/download_extensions.cc#newcode78 chrome/browser/download/download_extensions.cc:78: // ALLOW_ON_USER_GESTURE. Even if the user explicitly consents to ...
4 years, 2 months ago (2015-06-16 19:23:42 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1165893004/200001
4 years, 2 months ago (2015-06-16 19:25:30 UTC) #21
commit-bot: I haz the power
Committed patchset #10 (id:200001)
4 years, 2 months ago (2015-06-16 20:23:27 UTC) #22
commit-bot: I haz the power
4 years, 2 months ago (2015-06-16 20:24:25 UTC) #23
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/39841e54180e2583dffa16fbbb9b99fd293821d0
Cr-Commit-Position: refs/heads/master@{#334677}

Powered by Google App Engine
This is Rietveld 408576698