Fix an issue that download filename from content disposition is not sanitized

Fix an issue that download filename from content disposition is not sanitized

The filename from content disposition may not be sanitized.
This change uses net::GetSuggestedFileName() to sanitize the filename
before passing it to java.
Also, there is no need to call URLUtil.guessFileName() since
GetSuggestedFileName() will always return a non-null value.

BUG=570750
Committed: https://crrev.com/168f723d0f0ce60d92a6307c754181f6d8644583
Cr-Commit-Position: refs/heads/master@{#377447}

[qinmin, 2016-02-19 20:27:58]

qinmin@chromium.org changed reviewers:
+ asanka@chromium.org, tedchoc@chromium.org

[qinmin, 2016-02-19 20:27:58]

PTAL

[asanka, 2016-02-22 22:56:08]

https://codereview.chromium.org/1717783002/diff/1/content/browser/android/dow...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1717783002/diff/1/content/browser/android/dow...
content/browser/android/download_controller_android_impl.cc:394:
net::HttpContentDisposition header(info.content_disposition, "");
header is unused.

https://codereview.chromium.org/1717783002/diff/1/content/browser/android/dow...
content/browser/android/download_controller_android_impl.cc:403:
std::string()));  // default name
Provide a default name in case one cannot be generated based on the URL and
Content-Disposition.

[qinmin, 2016-02-22 23:03:36]

https://codereview.chromium.org/1717783002/diff/1/content/browser/android/dow...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1717783002/diff/1/content/browser/android/dow...
content/browser/android/download_controller_android_impl.cc:394:
net::HttpContentDisposition header(info.content_disposition, "");
On 2016/02/22 22:56:08, asanka wrote:
> header is unused.

Done.

https://codereview.chromium.org/1717783002/diff/1/content/browser/android/dow...
content/browser/android/download_controller_android_impl.cc:403:
std::string()));  // default name
On 2016/02/22 22:56:08, asanka wrote:
> Provide a default name in case one cannot be generated based on the URL and
> Content-Disposition.

net::GetSuggestedFilename() will fallback to "download" as filename, so i think
this is not needed.

[asanka, 2016-02-23 00:05:56]

https://codereview.chromium.org/1717783002/diff/1/content/browser/android/dow...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1717783002/diff/1/content/browser/android/dow...
content/browser/android/download_controller_android_impl.cc:403:
std::string()));  // default name
On 2016/02/22 at 23:03:36, qinmin wrote:
> On 2016/02/22 22:56:08, asanka wrote:
> > Provide a default name in case one cannot be generated based on the URL and
> > Content-Disposition.
> 
> net::GetSuggestedFilename() will fallback to "download" as filename, so i
think this is not needed.

That's not a localized string. See IDS_DEFAULT_DOWNLOAD_FILENAME for the correct
fallback filename.

[qinmin, 2016-02-23 07:23:10]

https://codereview.chromium.org/1717783002/diff/1/content/browser/android/dow...
File content/browser/android/download_controller_android_impl.cc (right):

https://codereview.chromium.org/1717783002/diff/1/content/browser/android/dow...
content/browser/android/download_controller_android_impl.cc:403:
std::string()));  // default name
On 2016/02/23 00:05:56, asanka wrote:
> On 2016/02/22 at 23:03:36, qinmin wrote:
> > On 2016/02/22 22:56:08, asanka wrote:
> > > Provide a default name in case one cannot be generated based on the URL
and
> > > Content-Disposition.
> > 
> > net::GetSuggestedFilename() will fallback to "download" as filename, so i
> think this is not needed.
> 
> That's not a localized string. See IDS_DEFAULT_DOWNLOAD_FILENAME for the
correct
> fallback filename.

Done. IDS_DEFAULT_DOWNLOAD_FILENAME is defined in chrome/, use
DownloadManagerService to pass the localized string to this class

[qinmin, 2016-02-23 07:23:24]

The CQ bit was checked by qinmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-23 07:24:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1717783002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1717783002/40001

[commit-bot: I haz the power, 2016-02-23 08:28:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-23 08:28:36]

Dry run: This issue passed the CQ dry run.

[asanka, 2016-02-24 20:04:44]

https://codereview.chromium.org/1717783002/diff/40001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/1717783002/diff/40001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:51:
l10n_util::GetStringUTF8(IDS_DEFAULT_DOWNLOAD_FILENAME));
Why can't DownloadControllerAndroid do this on its own?

[qinmin, 2016-02-24 21:56:06]

https://codereview.chromium.org/1717783002/diff/40001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/1717783002/diff/40001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:51:
l10n_util::GetStringUTF8(IDS_DEFAULT_DOWNLOAD_FILENAME));
On 2016/02/24 20:04:44, asanka wrote:
> Why can't DownloadControllerAndroid do this on its own?

2 reasons:
1. IDS_DEFAULT_DOWNLOAD_FILENAME is in chrome/, instead of content/
2. l10n_util is in ui/base, and content/ doesn't depend on that

DownloadManagerService are initialized at browser start up, so it can pass the
localized string from chrome/ to content/

[asanka, 2016-02-24 22:08:03]

lgtm

https://codereview.chromium.org/1717783002/diff/40001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/1717783002/diff/40001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:51:
l10n_util::GetStringUTF8(IDS_DEFAULT_DOWNLOAD_FILENAME));
On 2016/02/24 at 21:56:06, qinmin wrote:
> On 2016/02/24 20:04:44, asanka wrote:
> > Why can't DownloadControllerAndroid do this on its own?
> 
> 2 reasons:
> 1. IDS_DEFAULT_DOWNLOAD_FILENAME is in chrome/, instead of content/
> 2. l10n_util is in ui/base, and content/ doesn't depend on that
> 
> DownloadManagerService are initialized at browser start up, so it can pass the
localized string from chrome/ to content/

Ah. I overlooked the fact that DownloadControllerAndroid is in //content.

[Ted C, 2016-02-24 23:27:49]

lgtm

[qinmin, 2016-02-24 23:41:36]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2016-02-24 23:45:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1717783002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1717783002/40001

[commit-bot: I haz the power, 2016-02-25 01:10:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-25 01:12:41]

Description was changed from

==========
Fix an issue that download filename from content disposition is not sanitized

The filename from content disposition may not be sanitized.
This change uses net::GetSuggestedFileName() to sanitize the filename
before passing it to java.
Also, there is no need to call URLUtil.guessFileName() since
GetSuggestedFileName() will always return a non-null value.

BUG=570750
==========

to

==========
Fix an issue that download filename from content disposition is not sanitized

The filename from content disposition may not be sanitized.
This change uses net::GetSuggestedFileName() to sanitize the filename
before passing it to java.
Also, there is no need to call URLUtil.guessFileName() since
GetSuggestedFileName() will always return a non-null value.

BUG=570750

Committed: https://crrev.com/168f723d0f0ce60d92a6307c754181f6d8644583
Cr-Commit-Position: refs/heads/master@{#377447}
==========

[commit-bot: I haz the power, 2016-02-25 01:12:42]

Patchset 3 (id:??) landed as
https://crrev.com/168f723d0f0ce60d92a6307c754181f6d8644583
Cr-Commit-Position: refs/heads/master@{#377447}