diff options
| author | benjhayden@chromium.org <benjhayden@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-05-05 03:25:28 +0000 |
|---|---|---|
| committer | benjhayden@chromium.org <benjhayden@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-05-05 03:25:28 +0000 |
| commit | 023fa56d95c8554a931f29772311be45ece0ce7d (patch) | |
| tree | 42b2d6ab72f08709af8583d89c84ad1ec2e1e919 | |
| parent | 88c180f52c569642e5be7edfd227689e65b7caa1 (diff) | |
| download | chromium_src-023fa56d95c8554a931f29772311be45ece0ce7d.zip chromium_src-023fa56d95c8554a931f29772311be45ece0ce7d.tar.gz chromium_src-023fa56d95c8554a931f29772311be45ece0ce7d.tar.bz2 | |
Sanitize directory names from chrome.downloads.onDeterminingFilename.
Also fixes subdirectories in DownloadPathReservationTracker and a bug in downloads_api_unittest.cc where Events were incorrectly matching expectations (false positives).
BUG=181332
Review URL: https://chromiumcodereview.appspot.com/13866027
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@198343 0039d316-1c4b-4281-b951-d872f2087c98
15 files changed, 1033 insertions, 467 deletions
diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc index 1e314dc..3a5b037 100644 --- a/chrome/browser/download/chrome_download_manager_delegate.cc +++ b/chrome/browser/download/chrome_download_manager_delegate.cc @@ -420,6 +420,7 @@ void ChromeDownloadManagerDelegate::NotifyExtensions( void ChromeDownloadManagerDelegate::ReserveVirtualPath( content::DownloadItem* download, const base::FilePath& virtual_path, + bool create_directory, DownloadPathReservationTracker::FilenameConflictAction conflict_action, const DownloadTargetDeterminerDelegate::ReservedPathCallback& callback) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); @@ -434,7 +435,7 @@ void ChromeDownloadManagerDelegate::ReserveVirtualPath( #endif DownloadPathReservationTracker::GetReservedPath( *download, virtual_path, download_prefs_->DownloadPath(), - conflict_action, callback); + create_directory, conflict_action, callback); } void ChromeDownloadManagerDelegate::PromptUserForDownloadPath( diff --git a/chrome/browser/download/chrome_download_manager_delegate.h b/chrome/browser/download/chrome_download_manager_delegate.h index d6cedd4..7f8a257 100644 --- a/chrome/browser/download/chrome_download_manager_delegate.h +++ b/chrome/browser/download/chrome_download_manager_delegate.h @@ -114,6 +114,7 @@ class ChromeDownloadManagerDelegate virtual void ReserveVirtualPath( content::DownloadItem* download, const base::FilePath& virtual_path, + bool create_directory, DownloadPathReservationTracker::FilenameConflictAction conflict_action, const ReservedPathCallback& callback) OVERRIDE; virtual void PromptUserForDownloadPath( diff --git a/chrome/browser/download/chrome_download_manager_delegate_unittest.cc b/chrome/browser/download/chrome_download_manager_delegate_unittest.cc index f44e050..e72b837 100644 --- a/chrome/browser/download/chrome_download_manager_delegate_unittest.cc +++ b/chrome/browser/download/chrome_download_manager_delegate_unittest.cc @@ -94,14 +94,15 @@ class TestChromeDownloadManagerDelegate : public ChromeDownloadManagerDelegate { virtual void ReserveVirtualPath( content::DownloadItem* download, - const base::FilePath& target_path, + const base::FilePath& virtual_path, + bool create_directory, DownloadPathReservationTracker::FilenameConflictAction conflict_action, const DownloadPathReservationTracker::ReservedPathCallback& callback) OVERRIDE { // Pretend the path reservation succeeded without any change to // |target_path|. MessageLoop::current()->PostTask(FROM_HERE, - base::Bind(callback, target_path, true)); + base::Bind(callback, virtual_path, true)); } virtual void PromptUserForDownloadPath( diff --git a/chrome/browser/download/download_path_reservation_tracker.cc b/chrome/browser/download/download_path_reservation_tracker.cc index 0fc0e28..7c7c1a5 100644 --- a/chrome/browser/download/download_path_reservation_tracker.cc +++ b/chrome/browser/download/download_path_reservation_tracker.cc @@ -155,6 +155,7 @@ void CreateReservation( DownloadId download_id, const base::FilePath& suggested_path, const base::FilePath& default_download_path, + bool create_directory, DownloadPathReservationTracker::FilenameConflictAction conflict_action, const DownloadPathReservationTracker::ReservedPathCallback& callback) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); @@ -170,34 +171,37 @@ void CreateReservation( DCHECK(!ContainsKey(reservations, download_id)); base::FilePath target_path(suggested_path.NormalizePathSeparators()); + base::FilePath target_dir = target_path.DirName(); + base::FilePath filename = target_path.BaseName(); bool is_path_writeable = true; bool has_conflicts = false; bool name_too_long = false; - // Create the default download path if it doesn't already exist and is where - // we are going to create the downloaded file. |target_path| might point - // elsewhere if this was a programmatic download. - if (!default_download_path.empty() && - default_download_path == target_path.DirName() && - !file_util::DirectoryExists(default_download_path)) { - file_util::CreateDirectory(default_download_path); + // Create target_dir if necessary and appropriate. target_dir may be the last + // directory that the user selected in a FilePicker; if that directory has + // since been removed, do NOT automatically re-create it. Only automatically + // create the directory if it is the default Downloads directory or if the + // caller explicitly requested automatic directory creation. + if (!file_util::DirectoryExists(target_dir) && + (create_directory || + (!default_download_path.empty() && + (default_download_path == target_dir)))) { + file_util::CreateDirectory(target_dir); } // Check writability of the suggested path. If we can't write to it, default // to the user's "My Documents" directory. We'll prompt them in this case. - base::FilePath dir = target_path.DirName(); - base::FilePath filename = target_path.BaseName(); - if (!file_util::PathIsWritable(dir)) { - DVLOG(1) << "Unable to write to directory \"" << dir.value() << "\""; + if (!file_util::PathIsWritable(target_dir)) { + DVLOG(1) << "Unable to write to directory \"" << target_dir.value() << "\""; is_path_writeable = false; - PathService::Get(chrome::DIR_USER_DOCUMENTS, &dir); - target_path = dir.Append(filename); + PathService::Get(chrome::DIR_USER_DOCUMENTS, &target_dir); + target_path = target_dir.Append(filename); } if (is_path_writeable) { // Check the limit of file name length if it could be obtained. When the // suggested name exceeds the limit, truncate or prompt the user. - int max_length = file_util::GetMaximumPathComponentLength(dir); + int max_length = file_util::GetMaximumPathComponentLength(target_dir); if (max_length != -1) { int limit = max_length - kIntermediateNameSuffixLength; if (limit <= 0 || !TruncateFileName(&target_path, limit)) @@ -342,6 +346,7 @@ void DownloadPathReservationTracker::GetReservedPath( DownloadItem& download_item, const base::FilePath& target_path, const base::FilePath& default_path, + bool create_directory, FilenameConflictAction conflict_action, const ReservedPathCallback& callback) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); @@ -350,10 +355,14 @@ void DownloadPathReservationTracker::GetReservedPath( new DownloadItemObserver(download_item); // DownloadItemObserver deletes itself. - BrowserThread::PostTask( - BrowserThread::FILE, FROM_HERE, - base::Bind(&CreateReservation, download_item.GetGlobalId(), - target_path, default_path, conflict_action, callback)); + BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, base::Bind( + &CreateReservation, + download_item.GetGlobalId(), + target_path, + default_path, + create_directory, + conflict_action, + callback)); } // static diff --git a/chrome/browser/download/download_path_reservation_tracker.h b/chrome/browser/download/download_path_reservation_tracker.h index cf55479..9c2da56 100644 --- a/chrome/browser/download/download_path_reservation_tracker.h +++ b/chrome/browser/download/download_path_reservation_tracker.h @@ -7,62 +7,6 @@ #include "base/callback_forward.h" -// DownloadPathReservationTracker: Track download paths that are in use by -// active downloads. -// -// Chrome attempts to uniquify filenames that are assigned to downloads in order -// to avoid overwriting files that already exist on the file system. Downloads -// that are considered potentially dangerous use random intermediate filenames. -// Therefore only considering files that exist on the filesystem is -// insufficient. This class tracks files that are assigned to active downloads -// so that uniquification can take those into account as well. -// -// When a path needs to be assigned to a download, the |GetReservedPath()| -// static method is called on the UI thread along with a reference to the -// download item that will eventually receive the reserved path: -// -// DownloadItem& download_item = <...> -// DownloadPathReservationTracker::GetReservedPath( -// download_item, -// requested_target_path, -// default_download_path, -// should_uniquify_path, -// completion_callback); -// -// This call creates a path reservation that will live until |download_item| is -// interrupted, cancelled, completes or is removed. -// -// The process of issuing a reservation happens on the FILE thread, and -// involves: -// -// - Creating |default_download_path| if it doesn't already exist. -// -// - Verifying that |requested_target_path| is writeable. If not, the user's -// documents folder is used instead. -// -// - Uniquifying |requested_target_path| by suffixing the filename with a -// uniquifier (e.g. "foo.txt" -> "foo (1).txt") in order to avoid conflicts -// with files that already exist on the file system or other download path -// reservations. Uniquifying is only done if |should_uniquify_path| is true. -// -// - Posting a task back to the UI thread to invoke |completion_callback| with -// the reserved path and a bool indicating whether the returned path was -// verified as being writeable and unique. -// -// In addition, if the target path of |download_item| is changed to a path other -// than the reserved path, then the reservation will be updated to match. Such -// changes can happen if a "Save As" dialog was displayed and the user chose a -// different path. The new target path is not checked against active paths to -// enforce uniqueness. It is only used for uniquifying new reservations. -// -// Once |completion_callback| is invoked, it is the caller's responsibility to -// handle cases where the target path could not be verified and set the target -// path of the |download_item| appropriately. -// -// Note: The current implementation doesn't look at symlinks/mount points. E.g.: -// It considers 'foo/bar/x.pdf' and 'foo/baz/x.pdf' to be two different paths, -// even though 'bar' might be a symlink to 'baz'. - namespace base { class FilePath; } @@ -71,10 +15,12 @@ namespace content { class DownloadItem; } -// Issues and tracks download paths that are in use by the download system. When -// a target path is set for a download, this object tracks the path and the -// associated download item so that subsequent downloads can avoid using the -// same path. +// Chrome attempts to uniquify filenames that are assigned to downloads in order +// to avoid overwriting files that already exist on the file system. Downloads +// that are considered potentially dangerous use random intermediate filenames. +// Therefore only considering files that exist on the filesystem is +// insufficient. This class tracks files that are assigned to active downloads +// so that uniquification can take those into account as well. class DownloadPathReservationTracker { public: // Callback used with |GetReservedPath|. |target_path| specifies the target @@ -99,17 +45,50 @@ class DownloadPathReservationTracker { PROMPT, }; - // Called on the UI thread to request a download path reservation. Begins - // observing |download_item| and initiates creating a reservation on the FILE - // thread. Will not modify any state of |download_item|. + // When a path needs to be assigned to a download, this method is called on + // the UI thread along with a reference to the download item that will + // eventually receive the reserved path. This method creates a path + // reservation that will live until |download_item| is interrupted, cancelled, + // completes or is removed. This method will not modify |download_item|. + // + // The process of issuing a reservation happens on the FILE thread, and + // involves: + // + // - Creating |requested_target_path.DirName()| if it doesn't already exist + // and either |create_directory| or |requested_target_path.DirName() == + // default_download_path|. + // + // - Verifying that |requested_target_path| is writeable. If not, the user's + // documents folder is used instead. + // + // - Uniquifying |requested_target_path| by suffixing the filename with a + // uniquifier (e.g. "foo.txt" -> "foo (1).txt") in order to avoid conflicts + // with files that already exist on the file system or other download path + // reservations. Uniquifying is only done if |conflict_action| is UNIQUIFY. + // + // - Posting a task back to the UI thread to invoke |completion_callback| with + // the reserved path and a bool indicating whether the returned path was + // verified as being writeable and unique. + // + // In addition, if the target path of |download_item| is changed to a path + // other than the reserved path, then the reservation will be updated to + // match. Such changes can happen if a "Save As" dialog was displayed and the + // user chose a different path. The new target path is not checked against + // active paths to enforce uniqueness. It is only used for uniquifying new + // reservations. + // + // Once |completion_callback| is invoked, it is the caller's responsibility to + // handle cases where the target path could not be verified and set the target + // path of the |download_item| appropriately. // - // |default_download_path| is the user's default download path. If this - // directory does not exist and is the parent directory of - // |requested_target_path|, the directory will be created. + // The current implementation doesn't look at symlinks/mount points. E.g.: It + // considers 'foo/bar/x.pdf' and 'foo/baz/x.pdf' to be two different paths, + // even though 'bar' might be a symlink to 'baz'. static void GetReservedPath( content::DownloadItem& download_item, const base::FilePath& requested_target_path, const base::FilePath& default_download_path, + bool create_directory, FilenameConflictAction conflict_action, const ReservedPathCallback& callback); diff --git a/chrome/browser/download/download_path_reservation_tracker_unittest.cc b/chrome/browser/download/download_path_reservation_tracker_unittest.cc index b082228..6c38236 100644 --- a/chrome/browser/download/download_path_reservation_tracker_unittest.cc +++ b/chrome/browser/download/download_path_reservation_tracker_unittest.cc @@ -76,6 +76,7 @@ class DownloadPathReservationTrackerTest : public testing::Test { void CallGetReservedPath( DownloadItem& download_item, const base::FilePath& target_path, + bool create_directory, DownloadPathReservationTracker::FilenameConflictAction conflict_action, base::FilePath* return_path, bool* return_verified); @@ -141,6 +142,7 @@ bool DownloadPathReservationTrackerTest::IsPathInUse( void DownloadPathReservationTrackerTest::CallGetReservedPath( DownloadItem& download_item, const base::FilePath& target_path, + bool create_directory, DownloadPathReservationTracker::FilenameConflictAction conflict_action, base::FilePath* return_path, bool* return_verified) { @@ -150,7 +152,11 @@ void DownloadPathReservationTrackerTest::CallGetReservedPath( this); bool did_run_callback = false; DownloadPathReservationTracker::GetReservedPath( - download_item, target_path, default_download_path(), conflict_action, + download_item, + target_path, + default_download_path(), + create_directory, + conflict_action, base::Bind(&DownloadPathReservationTrackerTest::TestReservedPathCallback, weak_ptr_factory.GetWeakPtr(), return_path, return_verified, &did_run_callback)); @@ -187,7 +193,14 @@ TEST_F(DownloadPathReservationTrackerTest, BasicReservation) { bool verified = false; DownloadPathReservationTracker::FilenameConflictAction conflict_action = DownloadPathReservationTracker::OVERWRITE; - CallGetReservedPath(*item, path, conflict_action, &reserved_path, &verified); + bool create_directory = false; + CallGetReservedPath( + *item, + path, + create_directory, + conflict_action, + &reserved_path, + &verified); EXPECT_TRUE(IsPathInUse(path)); EXPECT_TRUE(verified); EXPECT_EQ(path.value(), reserved_path.value()); @@ -209,7 +222,14 @@ TEST_F(DownloadPathReservationTrackerTest, InterruptedDownload) { bool verified = false; DownloadPathReservationTracker::FilenameConflictAction conflict_action = DownloadPathReservationTracker::OVERWRITE; - CallGetReservedPath(*item, path, conflict_action, &reserved_path, &verified); + bool create_directory = false; + CallGetReservedPath( + *item, + path, + create_directory, + conflict_action, + &reserved_path, + &verified); EXPECT_TRUE(IsPathInUse(path)); EXPECT_TRUE(verified); EXPECT_EQ(path.value(), reserved_path.value()); @@ -231,7 +251,14 @@ TEST_F(DownloadPathReservationTrackerTest, CompleteDownload) { bool verified = false; DownloadPathReservationTracker::FilenameConflictAction conflict_action = DownloadPathReservationTracker::OVERWRITE; - CallGetReservedPath(*item, path, conflict_action, &reserved_path, &verified); + bool create_directory = false; + CallGetReservedPath( + *item, + path, + create_directory, + conflict_action, + &reserved_path, + &verified); EXPECT_TRUE(IsPathInUse(path)); EXPECT_TRUE(verified); EXPECT_EQ(path.value(), reserved_path.value()); @@ -261,9 +288,16 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingFiles) { base::FilePath reserved_path; bool verified = false; + bool create_directory = false; DownloadPathReservationTracker::FilenameConflictAction conflict_action = DownloadPathReservationTracker::UNIQUIFY; - CallGetReservedPath(*item, path, conflict_action, &reserved_path, &verified); + CallGetReservedPath( + *item, + path, + create_directory, + conflict_action, + &reserved_path, + &verified); EXPECT_TRUE(IsPathInUse(path)); EXPECT_TRUE(IsPathInUse(reserved_path)); EXPECT_TRUE(verified); @@ -291,11 +325,17 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingReservations) { base::FilePath reserved_path1; bool verified = false; + bool create_directory = false; DownloadPathReservationTracker::FilenameConflictAction conflict_action = DownloadPathReservationTracker::UNIQUIFY; CallGetReservedPath( - *item1, path, conflict_action, &reserved_path1, &verified); + *item1, + path, + create_directory, + conflict_action, + &reserved_path1, + &verified); EXPECT_TRUE(IsPathInUse(path)); EXPECT_TRUE(verified); @@ -305,7 +345,12 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingReservations) { scoped_ptr<FakeDownloadItem> item2(CreateDownloadItem(2)); base::FilePath reserved_path2; CallGetReservedPath( - *item2, path, conflict_action, &reserved_path2, &verified); + *item2, + path, + create_directory, + conflict_action, + &reserved_path2, + &verified); EXPECT_TRUE(IsPathInUse(path)); EXPECT_TRUE(IsPathInUse(uniquified_path)); EXPECT_EQ(uniquified_path.value(), reserved_path2.value()); @@ -320,7 +365,12 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingReservations) { scoped_ptr<FakeDownloadItem> item2(CreateDownloadItem(2)); base::FilePath reserved_path2; CallGetReservedPath( - *item2, path, conflict_action, &reserved_path2, &verified); + *item2, + path, + create_directory, + conflict_action, + &reserved_path2, + &verified); EXPECT_TRUE(IsPathInUse(path)); EXPECT_TRUE(IsPathInUse(uniquified_path)); EXPECT_EQ(uniquified_path.value(), reserved_path2.value()); @@ -333,7 +383,12 @@ TEST_F(DownloadPathReservationTrackerTest, ConflictingReservations) { base::FilePath reserved_path3; conflict_action = DownloadPathReservationTracker::OVERWRITE; CallGetReservedPath( - *item3, path, conflict_action, &reserved_path3, &verified); + *item3, + path, + create_directory, + conflict_action, + &reserved_path3, + &verified); EXPECT_TRUE(IsPathInUse(path)); EXPECT_FALSE(IsPathInUse(uniquified_path)); @@ -351,6 +406,7 @@ TEST_F(DownloadPathReservationTrackerTest, UnresolvedConflicts) { DownloadPathReservationTracker::kMaxUniqueFiles + 1]; DownloadPathReservationTracker::FilenameConflictAction conflict_action = DownloadPathReservationTracker::UNIQUIFY; + bool create_directory = false; // Create |kMaxUniqueFiles + 1| reservations for |path|. The first reservation // will have no uniquifier. The |kMaxUniqueFiles| remaining reservations do. for (int i = 0; i <= DownloadPathReservationTracker::kMaxUniqueFiles; i++) { @@ -366,7 +422,12 @@ TEST_F(DownloadPathReservationTrackerTest, UnresolvedConflicts) { items[i].reset(CreateDownloadItem(i)); EXPECT_FALSE(IsPathInUse(expected_path)); CallGetReservedPath( - *items[i], path, conflict_action, &reserved_path, &verified); + *items[i], + path, + create_directory, + conflict_action, + &reserved_path, + &verified); EXPECT_TRUE(IsPathInUse(expected_path)); EXPECT_EQ(expected_path.value(), reserved_path.value()); EXPECT_TRUE(verified); @@ -376,7 +437,13 @@ TEST_F(DownloadPathReservationTrackerTest, UnresolvedConflicts) { CreateDownloadItem(DownloadPathReservationTracker::kMaxUniqueFiles + 1)); base::FilePath reserved_path; bool verified = true; - CallGetReservedPath(*item, path, conflict_action, &reserved_path, &verified); + CallGetReservedPath( + *item, + path, + create_directory, + conflict_action, + &reserved_path, + &verified); EXPECT_FALSE(verified); EXPECT_EQ(path.value(), reserved_path.value()); } @@ -398,8 +465,14 @@ TEST_F(DownloadPathReservationTrackerTest, UnwriteableDirectory) { bool verified = true; DownloadPathReservationTracker::FilenameConflictAction conflict_action = DownloadPathReservationTracker::OVERWRITE; + bool create_directory = false; CallGetReservedPath( - *item, path, conflict_action, &reserved_path, &verified); + *item, + path, + create_directory, + conflict_action, + &reserved_path, + &verified); // Verification fails. EXPECT_FALSE(verified); EXPECT_EQ(path.BaseName().value(), reserved_path.BaseName().value()); @@ -415,13 +488,19 @@ TEST_F(DownloadPathReservationTrackerTest, CreateDefaultDownloadPath) { ASSERT_FALSE(file_util::DirectoryExists(dir)); DownloadPathReservationTracker::FilenameConflictAction conflict_action = DownloadPathReservationTracker::OVERWRITE; + bool create_directory = false; { scoped_ptr<FakeDownloadItem> item(CreateDownloadItem(1)); base::FilePath reserved_path; bool verified = true; CallGetReservedPath( - *item, path, conflict_action, &reserved_path, &verified); + *item, + path, + create_directory, + conflict_action, + &reserved_path, + &verified); // Verification fails because the directory doesn't exist. EXPECT_FALSE(verified); } @@ -432,7 +511,12 @@ TEST_F(DownloadPathReservationTrackerTest, CreateDefaultDownloadPath) { bool verified = true; set_default_download_path(dir); CallGetReservedPath( - *item, path, conflict_action, &reserved_path, &verified); + *item, + path, + create_directory, + conflict_action, + &reserved_path, + &verified); // Verification succeeds because the directory is created. EXPECT_TRUE(verified); EXPECT_TRUE(file_util::DirectoryExists(dir)); @@ -451,7 +535,14 @@ TEST_F(DownloadPathReservationTrackerTest, UpdatesToTargetPath) { bool verified = false; DownloadPathReservationTracker::FilenameConflictAction conflict_action = DownloadPathReservationTracker::OVERWRITE; - CallGetReservedPath(*item, path, conflict_action, &reserved_path, &verified); + bool create_directory = false; + CallGetReservedPath( + *item, + path, + create_directory, + conflict_action, + &reserved_path, + &verified); EXPECT_TRUE(IsPathInUse(path)); EXPECT_TRUE(verified); EXPECT_EQ(path.value(), reserved_path.value()); @@ -502,7 +593,14 @@ TEST_F(DownloadPathReservationTrackerTest, BasicTruncation) { bool verified = false; DownloadPathReservationTracker::FilenameConflictAction conflict_action = DownloadPathReservationTracker::OVERWRITE; - CallGetReservedPath(*item, path, conflict_action, &reserved_path, &verified); + bool create_directory = false; + CallGetReservedPath( + *item, + path, + create_directory, + conflict_action, + &reserved_path, + &verified); EXPECT_TRUE(IsPathInUse(reserved_path)); EXPECT_TRUE(verified); // The file name length is truncated to max_length. @@ -537,7 +635,14 @@ TEST_F(DownloadPathReservationTrackerTest, TruncationConflict) { bool verified = false; DownloadPathReservationTracker::FilenameConflictAction conflict_action = DownloadPathReservationTracker::UNIQUIFY; - CallGetReservedPath(*item, path, conflict_action, &reserved_path, &verified); + bool create_directory = false; + CallGetReservedPath( + *item, + path, + create_directory, + conflict_action, + &reserved_path, + &verified); EXPECT_TRUE(IsPathInUse(reserved_path)); EXPECT_TRUE(verified); EXPECT_EQ(path2, reserved_path); @@ -559,7 +664,14 @@ TEST_F(DownloadPathReservationTrackerTest, TruncationFail) { bool verified = false; DownloadPathReservationTracker::FilenameConflictAction conflict_action = DownloadPathReservationTracker::OVERWRITE; - CallGetReservedPath(*item, path, conflict_action, &reserved_path, &verified); + bool create_directory = false; + CallGetReservedPath( + *item, + path, + create_directory, + conflict_action, + &reserved_path, + &verified); // We cannot truncate a path with very long extension. EXPECT_FALSE(verified); } diff --git a/chrome/browser/download/download_target_determiner.cc b/chrome/browser/download/download_target_determiner.cc index f11bc98..f077cae 100644 --- a/chrome/browser/download/download_target_determiner.cc +++ b/chrome/browser/download/download_target_determiner.cc @@ -64,6 +64,7 @@ DownloadTargetDeterminer::DownloadTargetDeterminer( const content::DownloadTargetCallback& callback) : next_state_(STATE_GENERATE_TARGET_PATH), should_prompt_(false), + create_directory_(false), conflict_action_(download->GetForcedFilePath().empty() ? DownloadPathReservationTracker::UNIQUIFY : DownloadPathReservationTracker::OVERWRITE), @@ -225,6 +226,7 @@ void DownloadTargetDeterminer::NotifyExtensionsDone( // suggest it. net::GenerateSafeFileName(std::string(), false, &new_path); virtual_path_ = new_path; + create_directory_ = true; conflict_action_ = conflict_action; } @@ -239,7 +241,7 @@ DownloadTargetDeterminer::Result next_state_ = STATE_PROMPT_USER_FOR_DOWNLOAD_PATH; delegate_->ReserveVirtualPath( - download_, virtual_path_, conflict_action_, + download_, virtual_path_, create_directory_, conflict_action_, base::Bind(&DownloadTargetDeterminer::ReserveVirtualPathDone, weak_ptr_factory_.GetWeakPtr())); return QUIT_DOLOOP; diff --git a/chrome/browser/download/download_target_determiner.h b/chrome/browser/download/download_target_determiner.h index 6ec4e45..6032e42 100644 --- a/chrome/browser/download/download_target_determiner.h +++ b/chrome/browser/download/download_target_determiner.h @@ -231,6 +231,7 @@ class DownloadTargetDeterminer // state State next_state_; bool should_prompt_; + bool create_directory_; DownloadPathReservationTracker::FilenameConflictAction conflict_action_; content::DownloadDangerType danger_type_; base::FilePath virtual_path_; diff --git a/chrome/browser/download/download_target_determiner_delegate.h b/chrome/browser/download/download_target_determiner_delegate.h index bba99ae..b303afe 100644 --- a/chrome/browser/download/download_target_determiner_delegate.h +++ b/chrome/browser/download/download_target_determiner_delegate.h @@ -79,6 +79,7 @@ class DownloadTargetDeterminerDelegate { virtual void ReserveVirtualPath( content::DownloadItem* download, const base::FilePath& virtual_path, + bool create_directory, DownloadPathReservationTracker::FilenameConflictAction conflict_action, const ReservedPathCallback& callback) = 0; diff --git a/chrome/browser/download/download_target_determiner_unittest.cc b/chrome/browser/download/download_target_determiner_unittest.cc index ecffb69..7dabfe4 100644 --- a/chrome/browser/download/download_target_determiner_unittest.cc +++ b/chrome/browser/download/download_target_determiner_unittest.cc @@ -140,8 +140,8 @@ class MockDownloadTargetDeterminerDelegate : MOCK_METHOD3(DetermineLocalPath, void(DownloadItem*, const base::FilePath&, const LocalPathCallback&)); - MOCK_METHOD4(ReserveVirtualPath, - void(DownloadItem*, const base::FilePath&, + MOCK_METHOD5(ReserveVirtualPath, + void(DownloadItem*, const base::FilePath&, bool, DownloadPathReservationTracker::FilenameConflictAction, const ReservedPathCallback&)); @@ -153,7 +153,7 @@ class MockDownloadTargetDeterminerDelegate : .WillByDefault(WithArg<2>( ScheduleCallback2(base::FilePath(), DownloadPathReservationTracker::UNIQUIFY))); - ON_CALL(*this, ReserveVirtualPath(_, _, _, _)) + ON_CALL(*this, ReserveVirtualPath(_, _, _, _, _)) .WillByDefault(Invoke( &MockDownloadTargetDeterminerDelegate::NullReserveVirtualPath)); ON_CALL(*this, PromptUserForDownloadPath(_, _, _)) @@ -167,6 +167,7 @@ class MockDownloadTargetDeterminerDelegate : static void NullReserveVirtualPath( DownloadItem* download, const base::FilePath& virtual_path, + bool create_directory, DownloadPathReservationTracker::FilenameConflictAction conflict_action, const DownloadTargetDeterminerDelegate::ReservedPathCallback& callback); static void NullPromptUser( @@ -442,6 +443,7 @@ void DownloadTargetDeterminerTest::DownloadTargetVerifier( void MockDownloadTargetDeterminerDelegate::NullReserveVirtualPath( DownloadItem* download, const base::FilePath& virtual_path, + bool create_directory, DownloadPathReservationTracker::FilenameConflictAction conflict_action, const DownloadTargetDeterminerDelegate::ReservedPathCallback& callback) { callback.Run(virtual_path, true); @@ -999,8 +1001,8 @@ TEST_F(DownloadTargetDeterminerTest, TargetDeterminer_ReservationFailed) { }; // Setup ReserveVirtualPath() to fail. - ON_CALL(*delegate(), ReserveVirtualPath(_, _, _, _)) - .WillByDefault(WithArg<3>(ScheduleCallback2( + ON_CALL(*delegate(), ReserveVirtualPath(_, _, _, _, _)) + .WillByDefault(WithArg<4>(ScheduleCallback2( GetPathInDownloadDir(FILE_PATH_LITERAL("bar.txt")), false))); RunTestCasesWithActiveItem(kReservationFailedCases, arraysize(kReservationFailedCases)); @@ -1419,8 +1421,8 @@ TEST_F(DownloadTargetDeterminerTest, ScheduleCallback2(overridden_path, DownloadPathReservationTracker::OVERWRITE))); EXPECT_CALL(*delegate(), ReserveVirtualPath( - _, full_overridden_path, DownloadPathReservationTracker::OVERWRITE, _)) - .WillOnce(WithArg<3>( + _, full_overridden_path, true, DownloadPathReservationTracker::OVERWRITE, + _)).WillOnce(WithArg<4>( ScheduleCallback2(full_overridden_path, true))); RunTestCase(test_case, item.get()); @@ -1431,8 +1433,8 @@ TEST_F(DownloadTargetDeterminerTest, ScheduleCallback2(overridden_path, DownloadPathReservationTracker::PROMPT))); EXPECT_CALL(*delegate(), ReserveVirtualPath( - _, full_overridden_path, DownloadPathReservationTracker::PROMPT, _)) - .WillOnce(WithArg<3>( + _, full_overridden_path, true, DownloadPathReservationTracker::PROMPT, _)) + .WillOnce(WithArg<4>( ScheduleCallback2(full_overridden_path, true))); RunTestCase(test_case, item.get()); } diff --git a/chrome/browser/extensions/api/downloads/downloads_api.cc b/chrome/browser/extensions/api/downloads/downloads_api.cc index c7c73be..1479297 100644 --- a/chrome/browser/extensions/api/downloads/downloads_api.cc +++ b/chrome/browser/extensions/api/downloads/downloads_api.cc @@ -196,14 +196,6 @@ DownloadItem::DownloadState StateEnumFromString(const std::string& state) { return DownloadItem::MAX_DOWNLOAD_STATE; } -bool ValidateFilename(const base::FilePath& filename) { - return !filename.empty() && - (filename == filename.StripTrailingSeparators()) && - (filename.BaseName().value() != base::FilePath::kCurrentDirectory) && - !filename.ReferencesParent() && - !filename.IsAbsolute(); -} - std::string TimeToISO8601(const base::Time& t) { base::Time::Exploded exploded; t.UTCExplode(&exploded); @@ -607,8 +599,9 @@ class ExtensionDownloadsEventRouterData : public base::SupportsUserData::Data { return false; } - // Returns false if this extension_id was not expected or if this extension_id - // has already reported, regardless of whether the filename is valid. + // Returns false if this |extension_id| was not expected or if this + // |extension_id| has already reported. The caller is responsible for + // validating |filename|. bool DeterminerCallback( const std::string& extension_id, const base::FilePath& filename, @@ -623,7 +616,7 @@ class ExtensionDownloadsEventRouterData : public base::SupportsUserData::Data { // Do not use filename if another determiner has already overridden the // filename and they take precedence. Extensions that were installed // later take precedence over previous extensions. - if (ValidateFilename(filename) && + if (!filename.empty() && (determiner_.extension_id.empty() || (determiners_[index].install_time > determiner_.install_time))) { determined_filename_ = filename; @@ -843,7 +836,7 @@ bool DownloadsDownloadFunction::RunImpl() { #elif defined(OS_POSIX) base::FilePath file_path(*options.filename.get()); #endif - if (!ValidateFilename(file_path) || + if (!net::IsSafePortableBasename(file_path) || (file_path.DirName().value() != base::FilePath::kCurrentDirectory)) { error_ = download_extension_errors::kInvalidFilenameError; return false; @@ -1278,7 +1271,7 @@ bool ExtensionDownloadsEventRouter::DetermineFilename( bool include_incognito, const std::string& ext_id, int download_id, - const base::FilePath& filename, + const base::FilePath& const_filename, extensions::api::downloads::FilenameConflictAction conflict_action, std::string* error) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); @@ -1305,12 +1298,20 @@ bool ExtensionDownloadsEventRouter::DetermineFilename( *error = download_extension_errors::kInvalidOperationError; return false; } + base::FilePath::StringType filename_str(const_filename.value()); + // Allow windows-style directory separators on all platforms. + std::replace(filename_str.begin(), filename_str.end(), + FILE_PATH_LITERAL('\\'), FILE_PATH_LITERAL('/')); + base::FilePath filename(filename_str); + bool valid_filename = net::IsSafePortableRelativePath(filename); + filename = (valid_filename ? filename.NormalizePathSeparators() : + base::FilePath()); if (!data->DeterminerCallback(ext_id, filename, conflict_action)) { // Nobody expects this ext_id! *error = download_extension_errors::kInvalidOperationError; return false; } - if (!filename.empty() && !ValidateFilename(filename)) { + if (!const_filename.empty() && !valid_filename) { // If this is moved to before DeterminerCallback(), then it will block // forever waiting for this ext_id to report. *error = download_extension_errors::kInvalidFilenameError; diff --git a/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc b/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc index 1b2b57e..2154ddf 100644 --- a/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc +++ b/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc @@ -106,7 +106,11 @@ class DownloadsEventsListener : public content::NotificationObserver { const base::Time& caught() { return caught_; } - bool Equals(const Event& other) { + bool Satisfies(const Event& other) const { + return other.SatisfiedBy(*this); + } + + bool SatisfiedBy(const Event& other) const { if ((profile_ != other.profile_) || (event_name_ != other.event_name_)) return false; @@ -127,9 +131,9 @@ class DownloadsEventsListener : public content::NotificationObserver { for (base::DictionaryValue::Iterator iter(*left_dict); !iter.IsAtEnd(); iter.Advance()) { base::Value* right_value = NULL; - if (right_dict->HasKey(iter.key()) && - right_dict->Get(iter.key(), &right_value) && - !iter.value().Equals(right_value)) { + if (!right_dict->HasKey(iter.key()) || + (right_dict->Get(iter.key(), &right_value) && + !iter.value().Equals(right_value))) { return false; } } @@ -181,7 +185,7 @@ class DownloadsEventsListener : public content::NotificationObserver { events_.push_back(new_event); if (waiting_ && waiting_for_.get() && - waiting_for_->Equals(*new_event)) { + new_event->Satisfies(*waiting_for_)) { waiting_ = false; MessageLoopForUI::current()->Quit(); } @@ -198,8 +202,9 @@ class DownloadsEventsListener : public content::NotificationObserver { waiting_for_.reset(new Event(profile, event_name, json_args, base::Time())); for (std::deque<Event*>::const_iterator iter = events_.begin(); iter != events_.end(); ++iter) { - if ((*iter)->Equals(*waiting_for_.get())) + if ((*iter)->Satisfies(*waiting_for_.get())) { return true; + } } waiting_ = true; content::RunMessageLoop(); @@ -930,12 +935,6 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, EXPECT_TRUE(RunFunction(new DownloadsOpenFunction(), DownloadItemIdAsArgList(download_item))); EXPECT_TRUE(download_item->GetOpened()); - ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, - base::StringPrintf("[{\"id\": %d," - " \"opened\": {" - " \"previous\": false," - " \"current\": true}}]", - download_item->GetId()))); } IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, @@ -1559,22 +1558,25 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, ASSERT_TRUE(WaitFor(events::kOnDownloadCreated, base::StringPrintf("[{\"danger\": \"safe\"," - " \"incognito\": false," - " \"mime\": \"text/plain\"," - " \"paused\": false," - " \"url\": \"%s\"}]", - download_url.c_str()))); + " \"incognito\": false," + " \"mime\": \"text/plain\"," + " \"paused\": false," + " \"url\": \"%s\"}]", + download_url.c_str()))); ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," - " \"filename\": {" - " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", - result_id, - GetFilename("slow.txt.crdownload").c_str(), - GetFilename("slow.txt").c_str()))); + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("slow.txt.crdownload").c_str(), + GetFilename("slow.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, + base::StringPrintf("[{\"id\": %d," + " \"state\": {" + " \"previous\": \"in_progress\"," + " \"current\": \"complete\"}}]", + result_id))); } // Test that we can start a download from an incognito context, and that the @@ -1600,22 +1602,25 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, ASSERT_TRUE(WaitFor(events::kOnDownloadCreated, base::StringPrintf("[{\"danger\": \"safe\"," - " \"incognito\": true," - " \"mime\": \"text/plain\"," - " \"paused\": false," - " \"url\": \"%s\"}]", - download_url.c_str()))); + " \"incognito\": true," + " \"mime\": \"text/plain\"," + " \"paused\": false," + " \"url\": \"%s\"}]", + download_url.c_str()))); ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\":%d," - " \"filename\": {" - " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"current\": \"complete\"," - " \"previous\": \"in_progress\"}}]", - result_id, - GetFilename("slow.txt.crdownload").c_str(), - GetFilename("slow.txt").c_str()))); + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("slow.txt.crdownload").c_str(), + GetFilename("slow.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, + base::StringPrintf("[{\"id\":%d," + " \"state\": {" + " \"current\": \"complete\"," + " \"previous\": \"in_progress\"}}]", + result_id))); } // Test that we disallow certain headers case-insensitively. @@ -1753,22 +1758,25 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, ASSERT_TRUE(WaitFor(events::kOnDownloadCreated, base::StringPrintf("[{\"danger\": \"safe\"," - " \"incognito\": false," - " \"mime\": \"text/plain\"," - " \"paused\": false," - " \"url\": \"%s\"}]", - download_url.c_str()))); + " \"incognito\": false," + " \"mime\": \"text/plain\"," + " \"paused\": false," + " \"url\": \"%s\"}]", + download_url.c_str()))); ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," - " \"filename\": {" - " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", - result_id, - GetFilename("slow.txt.crdownload").c_str(), - GetFilename("slow.txt").c_str()))); + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("slow.txt.crdownload").c_str(), + GetFilename("slow.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, + base::StringPrintf("[{\"id\": %d," + " \"state\": {" + " \"previous\": \"in_progress\"," + " \"current\": \"complete\"}}]", + result_id))); } // Valid data URLs are valid URLs. @@ -1792,22 +1800,25 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, ASSERT_TRUE(WaitFor(events::kOnDownloadCreated, base::StringPrintf("[{\"danger\": \"safe\"," - " \"incognito\": false," - " \"mime\": \"text/plain\"," - " \"paused\": false," - " \"url\": \"%s\"}]", - download_url.c_str()))); + " \"incognito\": false," + " \"mime\": \"text/plain\"," + " \"paused\": false," + " \"url\": \"%s\"}]", + download_url.c_str()))); ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," - " \"filename\": {" - " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", - result_id, - GetFilename("data.txt.crdownload").c_str(), - GetFilename("data.txt").c_str()))); + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("data.txt.crdownload").c_str(), + GetFilename("data.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, + base::StringPrintf("[{\"id\": %d," + " \"state\": {" + " \"previous\": \"in_progress\"," + " \"current\": \"complete\"}}]", + result_id))); } // Valid file URLs are valid URLs. @@ -1851,13 +1862,16 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, base::StringPrintf("[{\"id\": %d," " \"filename\": {" " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", + " \"current\": \"%s\"}}]", result_id, GetFilename("file.txt.crdownload").c_str(), GetFilename("file.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, + base::StringPrintf("[{\"id\": %d," + " \"state\": {" + " \"previous\": \"in_progress\"," + " \"current\": \"complete\"}}]", + result_id))); } // Test that auth-basic-succeed would fail if the resource requires the @@ -1920,24 +1934,27 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, ASSERT_TRUE(WaitFor(events::kOnDownloadCreated, base::StringPrintf("[{\"danger\": \"safe\"," - " \"incognito\": false," - " \"mime\": \"application/octet-stream\"," - " \"paused\": false," - " \"url\": \"%s\"}]", - download_url.c_str()))); + " \"incognito\": false," + " \"mime\": \"application/octet-stream\"," + " \"paused\": false," + " \"url\": \"%s\"}]", + download_url.c_str()))); std::string incomplete_filename = GetFilename( "headers-succeed.txt.crdownload"); ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," - " \"filename\": {" - " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", - result_id, - incomplete_filename.c_str(), - GetFilename("headers-succeed.txt").c_str()))); + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + incomplete_filename.c_str(), + GetFilename("headers-succeed.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, + base::StringPrintf("[{\"id\": %d," + " \"state\": {" + " \"previous\": \"in_progress\"," + " \"current\": \"complete\"}}]", + result_id))); } // Test that headers-succeed would fail if the resource requires the headers and @@ -2048,15 +2065,18 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, " \"url\": \"%s\"}]", download_url.c_str()))); ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}," " \"filename\": {" " \"previous\": \"%s\"," " \"current\": \"%s\"}}]", result_id, GetFilename("post-succeed.txt.crdownload").c_str(), GetFilename("post-succeed.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, + base::StringPrintf("[{\"id\": %d," + " \"state\": {" + " \"previous\": \"in_progress\"," + " \"current\": \"complete\"}}]", + result_id))); } // Test that downloadPostSuccess would fail if the resource requires the POST @@ -2217,13 +2237,16 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, base::StringPrintf("[{\"id\": %d," " \"filename\": {" " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", + " \"current\": \"%s\"}}]", result_id, GetFilename("on_record.txt.crdownload").c_str(), GetFilename("on_record.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, + base::StringPrintf("[{\"id\": %d," + " \"state\": {" + " \"previous\": \"in_progress\"," + " \"current\": \"complete\"}}]", + result_id))); std::string disk_data; EXPECT_TRUE(file_util::ReadFileToString(item->GetFullPath(), &disk_data)); EXPECT_STREQ(kPayloadData, disk_data.c_str()); @@ -2282,15 +2305,12 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, // The download should complete successfully. ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," - " \"filename\": {" - " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", - result_id, - GetFilename("slow.txt.crdownload").c_str(), - GetFilename("slow.txt").c_str()))); + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("slow.txt.crdownload").c_str(), + GetFilename("slow.txt").c_str()))); ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," " \"state\": {" @@ -2367,15 +2387,12 @@ IN_PROC_BROWSER_TEST_F( result_id))); ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," - " \"filename\": {" - " \"previous\": \"%s\"," - " \"current\": \"%s\"}," " \"state\": {" " \"previous\": \"in_progress\"," " \"current\": \"complete\"}}]", - result_id, - GetFilename("overridden.swf.crdownload").c_str(), - GetFilename("overridden.swf").c_str()))); + result_id))); + EXPECT_EQ(downloads_directory().AppendASCII("overridden.swf"), + item->GetFullPath()); } IN_PROC_BROWSER_TEST_F( @@ -2431,13 +2448,212 @@ IN_PROC_BROWSER_TEST_F( base::StringPrintf("[{\"id\": %d," " \"filename\": {" " \"previous\": \"%s\"," - " \"current\": \"%s\"}," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("slow.txt.crdownload").c_str(), + GetFilename("slow.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, + base::StringPrintf("[{\"id\": %d," " \"state\": {" " \"previous\": \"in_progress\"," " \"current\": \"complete\"}}]", + result_id))); +} + +IN_PROC_BROWSER_TEST_F( + DownloadExtensionTest, + DownloadExtensionTest_OnDeterminingFilename_IllegalFilename) { + GoOnTheRecord(); + LoadExtension("downloads_split"); + AddFilenameDeterminer(); + CHECK(StartTestServer()); + std::string download_url = test_server()->GetURL("slow?0").spec(); + + // Start downloading a file. + scoped_ptr<base::Value> result(RunFunctionAndReturnResult( + new DownloadsDownloadFunction(), base::StringPrintf( + "[{\"url\": \"%s\"}]", download_url.c_str()))); + ASSERT_TRUE(result.get()); + int result_id = -1; + ASSERT_TRUE(result->GetAsInteger(&result_id)); + DownloadItem* item = GetCurrentManager()->GetDownload(result_id); + ASSERT_TRUE(item); + ScopedCancellingItem canceller(item); + ASSERT_EQ(download_url, item->GetOriginalUrl().spec()); + + ASSERT_TRUE(WaitFor(events::kOnDownloadCreated, + base::StringPrintf("[{\"danger\": \"safe\"," + " \"incognito\": false," + " \"id\": %d," + " \"mime\": \"text/plain\"," + " \"paused\": false," + " \"url\": \"%s\"}]", result_id, - GetFilename("slow.txt.crdownload").c_str(), - GetFilename("slow.txt").c_str()))); + download_url.c_str()))); + ASSERT_TRUE(WaitFor( + events::kOnDownloadDeterminingFilename, + base::StringPrintf("[{\"id\": %d," + " \"filename\":\"slow.txt\"}]", + result_id))); + ASSERT_TRUE(item->GetTargetFilePath().empty()); + ASSERT_TRUE(item->IsInProgress()); + + // Respond to the onDeterminingFilename. + std::string error; + ASSERT_FALSE(ExtensionDownloadsEventRouter::DetermineFilename( + browser()->profile(), + false, + GetExtensionId(), + result_id, + base::FilePath(FILE_PATH_LITERAL("<")), + extensions::api::downloads::FILENAME_CONFLICT_ACTION_UNIQUIFY, + &error)); + EXPECT_STREQ(download_extension_errors::kInvalidFilenameError, error.c_str()); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf( + "[{\"id\": %d," + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("slow.txt.crdownload").c_str(), + GetFilename("slow.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf( + "[{\"id\": %d," + " \"state\": {" + " \"previous\": \"in_progress\"," + " \"current\": \"complete\"}}]", + result_id))); +} + +IN_PROC_BROWSER_TEST_F( + DownloadExtensionTest, + DownloadExtensionTest_OnDeterminingFilename_IllegalFilenameExtension) { + GoOnTheRecord(); + LoadExtension("downloads_split"); + AddFilenameDeterminer(); + CHECK(StartTestServer()); + std::string download_url = test_server()->GetURL("slow?0").spec(); + + // Start downloading a file. + scoped_ptr<base::Value> result(RunFunctionAndReturnResult( + new DownloadsDownloadFunction(), base::StringPrintf( + "[{\"url\": \"%s\"}]", download_url.c_str()))); + ASSERT_TRUE(result.get()); + int result_id = -1; + ASSERT_TRUE(result->GetAsInteger(&result_id)); + DownloadItem* item = GetCurrentManager()->GetDownload(result_id); + ASSERT_TRUE(item); + ScopedCancellingItem canceller(item); + ASSERT_EQ(download_url, item->GetOriginalUrl().spec()); + + ASSERT_TRUE(WaitFor(events::kOnDownloadCreated, + base::StringPrintf("[{\"danger\": \"safe\"," + " \"incognito\": false," + " \"id\": %d," + " \"mime\": \"text/plain\"," + " \"paused\": false," + " \"url\": \"%s\"}]", + result_id, + download_url.c_str()))); + ASSERT_TRUE(WaitFor( + events::kOnDownloadDeterminingFilename, + base::StringPrintf("[{\"id\": %d," + " \"filename\":\"slow.txt\"}]", + result_id))); + ASSERT_TRUE(item->GetTargetFilePath().empty()); + ASSERT_TRUE(item->IsInProgress()); + + // Respond to the onDeterminingFilename. + std::string error; + ASSERT_FALSE(ExtensionDownloadsEventRouter::DetermineFilename( + browser()->profile(), + false, + GetExtensionId(), + result_id, + base::FilePath(FILE_PATH_LITERAL( + "My Computer.{20D04FE0-3AEA-1069-A2D8-08002B30309D}/foo")), + extensions::api::downloads::FILENAME_CONFLICT_ACTION_UNIQUIFY, + &error)); + EXPECT_STREQ(download_extension_errors::kInvalidFilenameError, error.c_str()); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf( + "[{\"id\": %d," + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("slow.txt.crdownload").c_str(), + GetFilename("slow.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf( + "[{\"id\": %d," + " \"state\": {" + " \"previous\": \"in_progress\"," + " \"current\": \"complete\"}}]", + result_id))); +} + +IN_PROC_BROWSER_TEST_F( + DownloadExtensionTest, + DownloadExtensionTest_OnDeterminingFilename_ReservedFilename) { + GoOnTheRecord(); + LoadExtension("downloads_split"); + AddFilenameDeterminer(); + CHECK(StartTestServer()); + std::string download_url = test_server()->GetURL("slow?0").spec(); + + // Start downloading a file. + scoped_ptr<base::Value> result(RunFunctionAndReturnResult( + new DownloadsDownloadFunction(), base::StringPrintf( + "[{\"url\": \"%s\"}]", download_url.c_str()))); + ASSERT_TRUE(result.get()); + int result_id = -1; + ASSERT_TRUE(result->GetAsInteger(&result_id)); + DownloadItem* item = GetCurrentManager()->GetDownload(result_id); + ASSERT_TRUE(item); + ScopedCancellingItem canceller(item); + ASSERT_EQ(download_url, item->GetOriginalUrl().spec()); + + ASSERT_TRUE(WaitFor(events::kOnDownloadCreated, + base::StringPrintf("[{\"danger\": \"safe\"," + " \"incognito\": false," + " \"id\": %d," + " \"mime\": \"text/plain\"," + " \"paused\": false," + " \"url\": \"%s\"}]", + result_id, + download_url.c_str()))); + ASSERT_TRUE(WaitFor( + events::kOnDownloadDeterminingFilename, + base::StringPrintf("[{\"id\": %d," + " \"filename\":\"slow.txt\"}]", + result_id))); + ASSERT_TRUE(item->GetTargetFilePath().empty()); + ASSERT_TRUE(item->IsInProgress()); + + // Respond to the onDeterminingFilename. + std::string error; + ASSERT_FALSE(ExtensionDownloadsEventRouter::DetermineFilename( + browser()->profile(), + false, + GetExtensionId(), + result_id, + base::FilePath(FILE_PATH_LITERAL("con.foo")), + extensions::api::downloads::FILENAME_CONFLICT_ACTION_UNIQUIFY, + &error)); + EXPECT_STREQ(download_extension_errors::kInvalidFilenameError, error.c_str()); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf( + "[{\"id\": %d," + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("slow.txt.crdownload").c_str(), + GetFilename("slow.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf( + "[{\"id\": %d," + " \"state\": {" + " \"previous\": \"in_progress\"," + " \"current\": \"complete\"}}]", + result_id))); } IN_PROC_BROWSER_TEST_F( @@ -2491,15 +2707,18 @@ IN_PROC_BROWSER_TEST_F( EXPECT_STREQ(download_extension_errors::kInvalidFilenameError, error.c_str()); ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," - " \"filename\": {" - " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", - result_id, - GetFilename("slow.txt.crdownload").c_str(), - GetFilename("slow.txt").c_str()))); + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("slow.txt.crdownload").c_str(), + GetFilename("slow.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, + base::StringPrintf("[{\"id\": %d," + " \"state\": {" + " \"previous\": \"in_progress\"," + " \"current\": \"complete\"}}]", + result_id))); } IN_PROC_BROWSER_TEST_F( @@ -2553,15 +2772,18 @@ IN_PROC_BROWSER_TEST_F( EXPECT_STREQ(download_extension_errors::kInvalidFilenameError, error.c_str()); ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," - " \"filename\": {" - " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", - result_id, - GetFilename("slow.txt.crdownload").c_str(), - GetFilename("slow.txt").c_str()))); + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("slow.txt.crdownload").c_str(), + GetFilename("slow.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, + base::StringPrintf("[{\"id\": %d," + " \"state\": {" + " \"previous\": \"in_progress\"," + " \"current\": \"complete\"}}]", + result_id))); } IN_PROC_BROWSER_TEST_F( @@ -2618,13 +2840,16 @@ IN_PROC_BROWSER_TEST_F( base::StringPrintf("[{\"id\": %d," " \"filename\": {" " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", + " \"current\": \"%s\"}}]", result_id, GetFilename("slow.txt.crdownload").c_str(), GetFilename("slow.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, + base::StringPrintf("[{\"id\": %d," + " \"state\": {" + " \"previous\": \"in_progress\"," + " \"current\": \"complete\"}}]", + result_id))); } IN_PROC_BROWSER_TEST_F( @@ -2679,15 +2904,18 @@ IN_PROC_BROWSER_TEST_F( ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," - " \"filename\": {" - " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", - result_id, - GetFilename("slow.txt.crdownload").c_str(), - GetFilename("slow.txt").c_str()))); + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("slow.txt.crdownload").c_str(), + GetFilename("slow.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, + base::StringPrintf("[{\"id\": %d," + " \"state\": {" + " \"previous\": \"in_progress\"," + " \"current\": \"complete\"}}]", + result_id))); } IN_PROC_BROWSER_TEST_F( @@ -2712,13 +2940,13 @@ IN_PROC_BROWSER_TEST_F( ASSERT_EQ(download_url, item->GetOriginalUrl().spec()); ASSERT_TRUE(WaitFor(events::kOnDownloadCreated, base::StringPrintf("[{\"danger\": \"safe\"," - " \"incognito\": false," - " \"id\": %d," - " \"mime\": \"text/plain\"," - " \"paused\": false," - " \"url\": \"%s\"}]", - result_id, - download_url.c_str()))); + " \"incognito\": false," + " \"id\": %d," + " \"mime\": \"text/plain\"," + " \"paused\": false," + " \"url\": \"%s\"}]", + result_id, + download_url.c_str()))); ASSERT_TRUE(WaitFor( events::kOnDownloadDeterminingFilename, base::StringPrintf("[{\"id\": %d," @@ -2741,15 +2969,12 @@ IN_PROC_BROWSER_TEST_F( ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," - " \"filename\": {" - " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", - result_id, - GetFilename("slow.txt.crdownload").c_str(), - GetFilename("slow.txt").c_str()))); + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("slow.txt.crdownload").c_str(), + GetFilename("slow.txt").c_str()))); ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," " \"state\": {" @@ -2771,13 +2996,13 @@ IN_PROC_BROWSER_TEST_F( ASSERT_TRUE(WaitFor(events::kOnDownloadCreated, base::StringPrintf("[{\"danger\": \"safe\"," - " \"incognito\": false," - " \"id\": %d," - " \"mime\": \"text/plain\"," - " \"paused\": false," - " \"url\": \"%s\"}]", - result_id, - download_url.c_str()))); + " \"incognito\": false," + " \"id\": %d," + " \"mime\": \"text/plain\"," + " \"paused\": false," + " \"url\": \"%s\"}]", + result_id, + download_url.c_str()))); ASSERT_TRUE(WaitFor( events::kOnDownloadDeterminingFilename, base::StringPrintf("[{\"id\": %d," @@ -2803,15 +3028,12 @@ IN_PROC_BROWSER_TEST_F( ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," - " \"filename\": {" - " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", - result_id, - GetFilename("foo.crdownload").c_str(), - GetFilename("foo").c_str()))); + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("foo.crdownload").c_str(), + GetFilename("foo").c_str()))); ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," " \"state\": {" @@ -2931,15 +3153,12 @@ IN_PROC_BROWSER_TEST_F( // The download should complete successfully. ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," - " \"filename\": {" - " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", - result_id, - GetFilename("42.txt.crdownload").c_str(), - GetFilename("42.txt").c_str()))); + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("42.txt.crdownload").c_str(), + GetFilename("42.txt").c_str()))); ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," " \"state\": {" @@ -2994,15 +3213,12 @@ IN_PROC_BROWSER_TEST_F( // The download should complete successfully. ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," - " \"filename\": {" - " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", - result_id, - GetFilename("5.txt.crdownload").c_str(), - GetFilename("5.txt").c_str()))); + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("5.txt.crdownload").c_str(), + GetFilename("5.txt").c_str()))); ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," " \"state\": {" @@ -3072,15 +3288,12 @@ IN_PROC_BROWSER_TEST_F( // The download should complete successfully. ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," - " \"filename\": {" - " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", - result_id, - GetFilename("42.txt.crdownload").c_str(), - GetFilename("42.txt").c_str()))); + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("42.txt.crdownload").c_str(), + GetFilename("42.txt").c_str()))); ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," " \"state\": {" @@ -3134,15 +3347,12 @@ IN_PROC_BROWSER_TEST_F( // The download should complete successfully. ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," - " \"filename\": {" - " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", - result_id, - GetFilename("42 (1).txt.crdownload").c_str(), - GetFilename("42 (1).txt").c_str()))); + " \"filename\": {" + " \"previous\": \"%s\"," + " \"current\": \"%s\"}}]", + result_id, + GetFilename("42 (1).txt.crdownload").c_str(), + GetFilename("42 (1).txt").c_str()))); ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, base::StringPrintf("[{\"id\": %d," " \"state\": {" @@ -3266,13 +3476,16 @@ IN_PROC_BROWSER_TEST_F( base::StringPrintf("[{\"id\": %d," " \"filename\": {" " \"previous\": \"%s\"," - " \"current\": \"%s\"}," - " \"state\": {" - " \"previous\": \"in_progress\"," - " \"current\": \"complete\"}}]", + " \"current\": \"%s\"}}]", item->GetId(), GetFilename("42.txt.crdownload").c_str(), GetFilename("42.txt").c_str()))); + ASSERT_TRUE(WaitFor(events::kOnDownloadChanged, + base::StringPrintf("[{\"id\": %d," + " \"state\": {" + " \"previous\": \"in_progress\"," + " \"current\": \"complete\"}}]", + item->GetId()))); } // TODO(benjhayden) Figure out why DisableExtension() does not fire diff --git a/net/base/net_util.cc b/net/base/net_util.cc index a9e4d90..a279293 100644 --- a/net/base/net_util.cc +++ b/net/base/net_util.cc @@ -163,16 +163,6 @@ static const int kAllowedFtpPorts[] = { 22, // ssh }; -#if defined(OS_WIN) -std::string::size_type CountTrailingChars( - const std::string& input, - const std::string::value_type trailing_chars[]) { - const size_t last_good_char = input.find_last_not_of(trailing_chars); - return (last_good_char == std::string::npos) ? - input.length() : (input.length() - last_good_char - 1); -} -#endif - // Does some simple normalization of scripts so we can allow certain scripts // to exist together. // TODO(brettw) bug 880223: we should allow some other languages to be @@ -710,25 +700,30 @@ void AppendFormattedComponent(const std::string& spec, } } -void SanitizeGeneratedFileName(std::string& filename) { - if (!filename.empty()) { - // Remove "." from the beginning and end of the file name to avoid tricks - // with hidden files, "..", and "." - TrimString(filename, ".", &filename); -#if defined(OS_WIN) +void SanitizeGeneratedFileName(base::FilePath::StringType* filename, + bool replace_trailing) { + const base::FilePath::CharType kReplace[] = FILE_PATH_LITERAL("-"); + if (filename->empty()) + return; + if (replace_trailing) { // Handle CreateFile() stripping trailing dots and spaces on filenames // http://support.microsoft.com/kb/115827 - std::string::size_type pos = filename.find_last_not_of(" ."); - if (pos == std::string::npos) - filename.resize(0); - else - filename.resize(++pos); -#endif - // Replace any path information by changing path separators with - // underscores. - ReplaceSubstringsAfterOffset(&filename, 0, "/", "_"); - ReplaceSubstringsAfterOffset(&filename, 0, "\\", "_"); + size_t length = filename->size(); + size_t pos = filename->find_last_not_of(FILE_PATH_LITERAL(" .")); + filename->resize((pos == std::string::npos) ? 0 : (pos + 1)); + TrimWhitespace(*filename, TRIM_TRAILING, filename); + if (filename->empty()) + return; + size_t trimmed = length - filename->size(); + if (trimmed) + filename->insert(filename->end(), trimmed, kReplace[0]); } + TrimString(*filename, FILE_PATH_LITERAL("."), filename); + if (filename->empty()) + return; + // Replace any path information by changing path separators. + ReplaceSubstringsAfterOffset(filename, 0, FILE_PATH_LITERAL("/"), kReplace); + ReplaceSubstringsAfterOffset(filename, 0, FILE_PATH_LITERAL("\\"), kReplace); } // Returns the filename determined from the last component of the path portion @@ -773,72 +768,67 @@ std::string GetFileNameFromURL(const GURL& url, return decoded_filename; } -#if defined(OS_WIN) // Returns whether the specified extension is automatically integrated into the // windows shell. -bool IsShellIntegratedExtension(const base::string16& extension) { - base::string16 extension_lower = StringToLowerASCII(extension); - - static const wchar_t* const integrated_extensions[] = { - // See <http://msdn.microsoft.com/en-us/library/ms811694.aspx>. - L"local", - // Right-clicking on shortcuts can be magical. - L"lnk", - }; +bool IsShellIntegratedExtension(const base::FilePath::StringType& extension) { + base::FilePath::StringType extension_lower = StringToLowerASCII(extension); - for (int i = 0; i < arraysize(integrated_extensions); ++i) { - if (extension_lower == integrated_extensions[i]) - return true; - } - - // See <http://www.juniper.net/security/auto/vulnerabilities/vuln2612.html>. - // That vulnerability report is not exactly on point, but files become magical - // if their end in a CLSID. Here we block extensions that look like CLSIDs. - if (!extension_lower.empty() && extension_lower[0] == L'{' && - extension_lower[extension_lower.length() - 1] == L'}') + // http://msdn.microsoft.com/en-us/library/ms811694.aspx + // Right-clicking on shortcuts can be magical. + if ((extension_lower == FILE_PATH_LITERAL("local")) || + (extension_lower == FILE_PATH_LITERAL("lnk"))) return true; + // http://www.juniper.net/security/auto/vulnerabilities/vuln2612.html + // Files become magical if they end in a CLSID, so block such extensions. + if (!extension_lower.empty() && + (extension_lower[0] == FILE_PATH_LITERAL('{')) && + (extension_lower[extension_lower.length() - 1] == FILE_PATH_LITERAL('}'))) + return true; return false; } // Returns whether the specified file name is a reserved name on windows. // This includes names like "com2.zip" (which correspond to devices) and // desktop.ini and thumbs.db which have special meaning to the windows shell. -bool IsReservedName(const base::string16& filename) { +bool IsReservedName(const base::FilePath::StringType& filename) { // This list is taken from the MSDN article "Naming a file" // http://msdn2.microsoft.com/en-us/library/aa365247(VS.85).aspx // I also added clock$ because GetSaveFileName seems to consider it as a // reserved name too. - static const wchar_t* const known_devices[] = { - L"con", L"prn", L"aux", L"nul", L"com1", L"com2", L"com3", L"com4", L"com5", - L"com6", L"com7", L"com8", L"com9", L"lpt1", L"lpt2", L"lpt3", L"lpt4", - L"lpt5", L"lpt6", L"lpt7", L"lpt8", L"lpt9", L"clock$" + static const char* const known_devices[] = { + "con", "prn", "aux", "nul", "com1", "com2", "com3", "com4", "com5", + "com6", "com7", "com8", "com9", "lpt1", "lpt2", "lpt3", "lpt4", + "lpt5", "lpt6", "lpt7", "lpt8", "lpt9", "clock$" }; - base::string16 filename_lower = StringToLowerASCII(filename); +#if defined(OS_WIN) + std::string filename_lower = StringToLowerASCII(WideToUTF8(filename)); +#elif defined(OS_POSIX) + std::string filename_lower = StringToLowerASCII(filename); +#endif - for (int i = 0; i < arraysize(known_devices); ++i) { + for (size_t i = 0; i < arraysize(known_devices); ++i) { // Exact match. if (filename_lower == known_devices[i]) return true; // Starts with "DEVICE.". - if (filename_lower.find(base::string16(known_devices[i]) + L".") == 0) + if (filename_lower.find(std::string(known_devices[i]) + ".") == 0) return true; } - static const wchar_t* const magic_names[] = { + static const char* const magic_names[] = { // These file names are used by the "Customize folder" feature of the shell. - L"desktop.ini", - L"thumbs.db", + "desktop.ini", + "thumbs.db", }; - for (int i = 0; i < arraysize(magic_names); ++i) { + for (size_t i = 0; i < arraysize(magic_names); ++i) { if (filename_lower == magic_names[i]) return true; } return false; } -#endif // OS_WIN // Examines the current extension in |file_name| and modifies it if necessary in // order to ensure the filename is safe. If |file_name| doesn't contain an @@ -898,6 +888,16 @@ void EnsureSafeExtension(const std::string& mime_type, *file_name = file_name->ReplaceExtension(extension); } +bool FilePathToString16(const base::FilePath& path, base::string16* converted) { +#if defined(OS_WIN) + return WideToUTF16(path.value().c_str(), path.value().size(), converted); +#elif defined(OS_POSIX) + std::string component8 = path.AsUTF8Unsafe(); + return !component8.empty() && + UTF8ToUTF16(component8.c_str(), component8.size(), converted); +#endif +} + } // namespace const FormatUrlType kFormatUrlOmitNothing = 0; @@ -1121,6 +1121,41 @@ base::string16 StripWWWFromHost(const GURL& url) { return StripWWW(ASCIIToUTF16(url.host())); } +bool IsSafePortablePathComponent(const base::FilePath& component) { + base::string16 component16; + base::FilePath::StringType sanitized = component.value(); + SanitizeGeneratedFileName(&sanitized, true); + base::FilePath::StringType extension = component.Extension(); + if (!extension.empty()) + extension.erase(extension.begin()); // Erase preceding '.'. + return !component.empty() && + (component == component.BaseName()) && + (component == component.StripTrailingSeparators()) && + FilePathToString16(component, &component16) && + file_util::IsFilenameLegal(component16) && + !IsShellIntegratedExtension(extension) && + (sanitized == component.value()); +} + +bool IsSafePortableBasename(const base::FilePath& filename) { + return IsSafePortablePathComponent(filename) && + !IsReservedName(filename.value()); +} + +bool IsSafePortableRelativePath(const base::FilePath& path) { + if (path.empty() || path.IsAbsolute() || path.EndsWithSeparator()) + return false; + std::vector<base::FilePath::StringType> components; + path.GetComponents(&components); + if (components.empty()) + return false; + for (size_t i = 0; i < components.size() - 1; ++i) { + if (!IsSafePortablePathComponent(base::FilePath(components[i]))) + return false; + } + return IsSafePortableBasename(path.BaseName()); +} + void GenerateSafeFileName(const std::string& mime_type, bool ignore_extension, base::FilePath* file_path) { @@ -1154,7 +1189,8 @@ base::string16 GetSuggestedFilename(const GURL& url, // We don't translate this fallback string, "download". If localization is // needed, the caller should provide localized fallback in |default_name|. - static const char* kFinalFallbackName = "download"; + static const base::FilePath::CharType kFinalFallbackName[] = + FILE_PATH_LITERAL("download"); std::string filename; // In UTF-8 bool overwrite_extension = false; @@ -1177,49 +1213,44 @@ base::string16 GetSuggestedFilename(const GURL& url, // Finally try the URL hostname, but only if there's no default specified in // |default_name|. Some schemes (e.g.: file:, about:, data:) do not have a // host name. - if (filename.empty() && default_name.empty() && - url.is_valid() && !url.host().empty()) { + if (filename.empty() && + default_name.empty() && + url.is_valid() && + !url.host().empty()) { // TODO(jungshik) : Decode a 'punycoded' IDN hostname. (bug 1264451) filename = url.host(); } + bool replace_trailing = false; + base::FilePath::StringType result_str, default_name_str; #if defined(OS_WIN) - std::string::size_type trimmed_trailing_character_count = - CountTrailingChars(filename, " ."); -#endif - SanitizeGeneratedFileName(filename); - // Sanitization can cause the filename to disappear (e.g.: if the filename - // consisted entirely of spaces and '.'s), in which case we use the default. - if (filename.empty()) { -#if defined(OS_WIN) - trimmed_trailing_character_count = 0; + replace_trailing = true; + result_str = UTF8ToUTF16(filename); + default_name_str = UTF8ToUTF16(default_name); +#else + result_str = filename; + default_name_str = default_name; #endif + SanitizeGeneratedFileName(&result_str, replace_trailing); + if (result_str.find_last_not_of(FILE_PATH_LITERAL("-_")) == + base::FilePath::StringType::npos) { + result_str = !default_name_str.empty() ? default_name_str : + base::FilePath::StringType(kFinalFallbackName); overwrite_extension = false; - if (default_name.empty()) - filename = kFinalFallbackName; } - -#if defined(OS_WIN) - base::string16 path = UTF8ToUTF16(filename.empty() ? default_name : filename); - // On Windows we want to preserve or replace all characters including - // whitespace to prevent file extension obfuscation on trusted websites - // e.g. Gmail might think evil.exe. is safe, so we don't want it to become - // evil.exe when we download it - base::string16::size_type path_length_before_trim = path.length(); - TrimWhitespace(path, TRIM_TRAILING, &path); - trimmed_trailing_character_count += path_length_before_trim - path.length(); - file_util::ReplaceIllegalCharactersInPath(&path, '-'); - path.append(trimmed_trailing_character_count, '-'); - base::FilePath result(path); + file_util::ReplaceIllegalCharactersInPath(&result_str, '-'); + base::FilePath result(result_str); GenerateSafeFileName(mime_type, overwrite_extension, &result); - return result.value(); -#else - std::string path = filename.empty() ? default_name : filename; - file_util::ReplaceIllegalCharactersInPath(&path, '-'); - base::FilePath result(path); - GenerateSafeFileName(mime_type, overwrite_extension, &result); - return UTF8ToUTF16(result.value()); -#endif + + base::string16 result16; + if (!FilePathToString16(result, &result16)) { + result = base::FilePath(default_name_str); + if (!FilePathToString16(result, &result16)) { + result = base::FilePath(kFinalFallbackName); + FilePathToString16(result, &result16); + } + } + return result16; } base::FilePath GenerateFileName(const GURL& url, diff --git a/net/base/net_util.h b/net/base/net_util.h index 30584a7..5cfd1ea 100644 --- a/net/base/net_util.h +++ b/net/base/net_util.h @@ -287,6 +287,27 @@ NET_EXPORT base::FilePath GenerateFileName( const std::string& mime_type, const std::string& default_name); +// Valid basenames: +// * are not empty +// * are not Windows reserved names (CON, NUL.zip, etc.) +// * are just basenames +// * do not have trailing separators +// * do not equal kCurrentDirectory +// * do not reference the parent directory +// * are valid path components, which: +// - * are not the empty string +// - * do not contain illegal characters +// - * do not end with Windows shell-integrated extensions (even on posix) +// - * do not begin with '.' (which would hide them in most file managers) +// - * do not end with ' ' or '.' +NET_EXPORT bool IsSafePortableBasename(const base::FilePath& path); + +// Basenames of valid relative paths are IsSafePortableBasename(), and internal +// path components of valid relative paths are valid path components as +// described above IsSafePortableBasename(). Valid relative paths are not +// absolute paths. +NET_EXPORT bool IsSafePortableRelativePath(const base::FilePath& path); + // Ensures that the filename and extension is safe to use in the filesystem. // // Assumes that |file_path| already contains a valid path or file name. On diff --git a/net/base/net_util_unittest.cc b/net/base/net_util_unittest.cc index ab8a236..4ddafe6 100644 --- a/net/base/net_util_unittest.cc +++ b/net/base/net_util_unittest.cc @@ -385,6 +385,7 @@ struct CompliantHostCase { }; struct GenerateFilenameCase { + int lineno; const char* url; const char* content_disp_header; const char* referrer_charset; @@ -484,9 +485,7 @@ std::string DumpIPNumber(const IPAddressNumber& v) { return out; } -void RunGenerateFileNameTestCase(const GenerateFilenameCase* test_case, - size_t iteration, - const char* suite) { +void RunGenerateFileNameTestCase(const GenerateFilenameCase* test_case) { std::string default_filename(WideToUTF8(test_case->default_filename)); base::FilePath file_path = GenerateFileName( GURL(test_case->url), test_case->content_disp_header, @@ -494,7 +493,7 @@ void RunGenerateFileNameTestCase(const GenerateFilenameCase* test_case, test_case->mime_type, default_filename); EXPECT_EQ(test_case->expected_filename, file_util::FilePathAsWString(file_path)) - << "Iteration " << iteration << " of " << suite << ": " << test_case->url; + << "test case at line number: " << test_case->lineno; } } // anonymous namespace @@ -801,19 +800,15 @@ TEST(NetUtilTest, StripWWW) { #if defined(OS_WIN) #define JPEG_EXT L".jpg" #define HTML_EXT L".htm" -#define TXT_EXT L".txt" -#define TAR_EXT L".tar" #elif defined(OS_MACOSX) #define JPEG_EXT L".jpeg" #define HTML_EXT L".html" -#define TXT_EXT L".txt" -#define TAR_EXT L".tar" #else #define JPEG_EXT L".jpg" #define HTML_EXT L".html" +#endif #define TXT_EXT L".txt" #define TAR_EXT L".tar" -#endif TEST(NetUtilTest, GenerateSafeFileName) { const struct { @@ -989,6 +984,7 @@ TEST(NetUtilTest, GenerateFileName) { // handled including failovers when the header is malformed. const GenerateFilenameCase selection_tests[] = { { + __LINE__, "http://www.google.com/", "attachment; filename=test.html", "", @@ -998,6 +994,7 @@ TEST(NetUtilTest, GenerateFileName) { L"test.html" }, { + __LINE__, "http://www.google.com/", "attachment; filename=\"test.html\"", "", @@ -1007,6 +1004,7 @@ TEST(NetUtilTest, GenerateFileName) { L"test.html" }, { + __LINE__, "http://www.google.com/", "attachment; filename= \"test.html\"", "", @@ -1016,6 +1014,7 @@ TEST(NetUtilTest, GenerateFileName) { L"test.html" }, { + __LINE__, "http://www.google.com/", "attachment; filename = \"test.html\"", "", @@ -1025,6 +1024,7 @@ TEST(NetUtilTest, GenerateFileName) { L"test.html" }, { // filename is whitespace. Should failover to URL host + __LINE__, "http://www.google.com/", "attachment; filename= ", "", @@ -1034,6 +1034,7 @@ TEST(NetUtilTest, GenerateFileName) { L"www.google.com" }, { // No filename. + __LINE__, "http://www.google.com/path/test.html", "attachment", "", @@ -1043,6 +1044,7 @@ TEST(NetUtilTest, GenerateFileName) { L"test.html" }, { // Ditto + __LINE__, "http://www.google.com/path/test.html", "attachment;", "", @@ -1052,6 +1054,7 @@ TEST(NetUtilTest, GenerateFileName) { L"test.html" }, { // No C-D + __LINE__, "http://www.google.com/", "", "", @@ -1061,6 +1064,7 @@ TEST(NetUtilTest, GenerateFileName) { L"www.google.com" }, { + __LINE__, "http://www.google.com/test.html", "", "", @@ -1072,6 +1076,7 @@ TEST(NetUtilTest, GenerateFileName) { { // Now that we use googleurl's ExtractFileName, this case falls back to // the hostname. If this behavior is not desirable, we'd better change // ExtractFileName (in url_parse). + __LINE__, "http://www.google.com/path/", "", "", @@ -1081,6 +1086,7 @@ TEST(NetUtilTest, GenerateFileName) { L"www.google.com" }, { + __LINE__, "http://www.google.com/path", "", "", @@ -1090,6 +1096,7 @@ TEST(NetUtilTest, GenerateFileName) { L"path" }, { + __LINE__, "file:///", "", "", @@ -1099,6 +1106,7 @@ TEST(NetUtilTest, GenerateFileName) { L"download" }, { + __LINE__, "file:///path/testfile", "", "", @@ -1108,6 +1116,7 @@ TEST(NetUtilTest, GenerateFileName) { L"testfile" }, { + __LINE__, "non-standard-scheme:", "", "", @@ -1117,6 +1126,7 @@ TEST(NetUtilTest, GenerateFileName) { L"download" }, { // C-D should override default + __LINE__, "http://www.google.com/", "attachment; filename =\"test.html\"", "", @@ -1126,6 +1136,7 @@ TEST(NetUtilTest, GenerateFileName) { L"test.html" }, { // But the URL shouldn't + __LINE__, "http://www.google.com/", "", "", @@ -1135,15 +1146,17 @@ TEST(NetUtilTest, GenerateFileName) { L"download" }, { + __LINE__, "http://www.google.com/", "attachment; filename=\"../test.html\"", "", "", "", L"", - L"_test.html" + L"-test.html" }, { + __LINE__, "http://www.google.com/", "attachment; filename=\"..\\test.html\"", "", @@ -1153,15 +1166,17 @@ TEST(NetUtilTest, GenerateFileName) { L"test.html" }, { + __LINE__, "http://www.google.com/", "attachment; filename=\"..\\\\test.html\"", "", "", "", L"", - L"_test.html" + L"-test.html" }, { // Filename disappears after leading and trailing periods are removed. + __LINE__, "http://www.google.com/", "attachment; filename=\"..\"", "", @@ -1171,6 +1186,7 @@ TEST(NetUtilTest, GenerateFileName) { L"default" }, { // C-D specified filename disappears. Failover to final filename. + __LINE__, "http://www.google.com/test.html", "attachment; filename=\"..\"", "", @@ -1181,6 +1197,7 @@ TEST(NetUtilTest, GenerateFileName) { }, // Below is a small subset of cases taken from HttpContentDisposition tests. { + __LINE__, "http://www.google.com/", "attachment; filename=\"%EC%98%88%EC%88%A0%20" "%EC%98%88%EC%88%A0.jpg\"", @@ -1191,6 +1208,7 @@ TEST(NetUtilTest, GenerateFileName) { L"\uc608\uc220 \uc608\uc220.jpg" }, { + __LINE__, "http://www.google.com/%EC%98%88%EC%88%A0%20%EC%98%88%EC%88%A0.jpg", "", "", @@ -1200,6 +1218,7 @@ TEST(NetUtilTest, GenerateFileName) { L"\uc608\uc220 \uc608\uc220.jpg" }, { + __LINE__, "http://www.google.com/", "attachment;", "", @@ -1209,6 +1228,7 @@ TEST(NetUtilTest, GenerateFileName) { L"\uB2E4\uC6B4\uB85C\uB4DC" }, { + __LINE__, "http://www.google.com/", "attachment; filename=\"=?EUC-JP?Q?=B7=DD=BD=" "D13=2Epng?=\"", @@ -1219,6 +1239,7 @@ TEST(NetUtilTest, GenerateFileName) { L"\u82b8\u88533.png" }, { + __LINE__, "http://www.example.com/images?id=3", "attachment; filename=caf\xc3\xa9.png", "iso-8859-1", @@ -1228,6 +1249,7 @@ TEST(NetUtilTest, GenerateFileName) { L"caf\u00e9.png" }, { + __LINE__, "http://www.example.com/images?id=3", "attachment; filename=caf\xe5.png", "windows-1253", @@ -1237,6 +1259,7 @@ TEST(NetUtilTest, GenerateFileName) { L"caf\u03b5.png" }, { + __LINE__, "http://www.example.com/file?id=3", "attachment; name=\xcf\xc2\xd4\xd8.zip", "GBK", @@ -1246,6 +1269,7 @@ TEST(NetUtilTest, GenerateFileName) { L"\u4e0b\u8f7d.zip" }, { // Invalid C-D header. Extracts filename from url. + __LINE__, "http://www.google.com/test.html", "attachment; filename==?iiso88591?Q?caf=EG?=", "", @@ -1256,6 +1280,7 @@ TEST(NetUtilTest, GenerateFileName) { }, // about: and data: URLs { + __LINE__, "about:chrome", "", "", @@ -1265,6 +1290,7 @@ TEST(NetUtilTest, GenerateFileName) { L"download" }, { + __LINE__, "data:,looks/like/a.path", "", "", @@ -1274,6 +1300,7 @@ TEST(NetUtilTest, GenerateFileName) { L"download" }, { + __LINE__, "data:text/plain;base64,VG8gYmUgb3Igbm90IHRvIGJlLg=", "", "", @@ -1283,6 +1310,7 @@ TEST(NetUtilTest, GenerateFileName) { L"download" }, { + __LINE__, "data:,looks/like/a.path", "", "", @@ -1292,6 +1320,7 @@ TEST(NetUtilTest, GenerateFileName) { L"default_filename_is_given" }, { + __LINE__, "data:,looks/like/a.path", "", "", @@ -1301,6 +1330,7 @@ TEST(NetUtilTest, GenerateFileName) { L"\u65e5\u672c\u8a9e" }, { // The filename encoding is specified by the referrer charset. + __LINE__, "http://example.com/V%FDvojov%E1%20psychologie.doc", "", "iso-8859-1", @@ -1310,6 +1340,7 @@ TEST(NetUtilTest, GenerateFileName) { L"V\u00fdvojov\u00e1 psychologie.doc" }, { // Suggested filename takes precedence over URL + __LINE__, "http://www.google.com/test", "", "", @@ -1319,6 +1350,7 @@ TEST(NetUtilTest, GenerateFileName) { L"suggested" }, { // The content-disposition has higher precedence over the suggested name. + __LINE__, "http://www.google.com/test", "attachment; filename=test.html", "", @@ -1331,6 +1363,7 @@ TEST(NetUtilTest, GenerateFileName) { { // The filename encoding doesn't match the referrer charset, the system // charset, or UTF-8. // TODO(jshin): we need to handle this case. + __LINE__, "http://example.com/V%FDvojov%E1%20psychologie.doc", "", "utf-8", @@ -1342,6 +1375,7 @@ TEST(NetUtilTest, GenerateFileName) { #endif // Raw 8bit characters in C-D { + __LINE__, "http://www.example.com/images?id=3", "attachment; filename=caf\xc3\xa9.png", "iso-8859-1", @@ -1351,6 +1385,7 @@ TEST(NetUtilTest, GenerateFileName) { L"caf\u00e9.png" }, { + __LINE__, "http://www.example.com/images?id=3", "attachment; filename=caf\xe5.png", "windows-1253", @@ -1360,6 +1395,7 @@ TEST(NetUtilTest, GenerateFileName) { L"caf\u03b5.png" }, { // No 'filename' keyword in the disposition, use the URL + __LINE__, "http://www.evil.com/my_download.txt", "a_file_name.txt", "", @@ -1369,6 +1405,7 @@ TEST(NetUtilTest, GenerateFileName) { L"my_download.txt" }, { // Spaces in the disposition file name + __LINE__, "http://www.frontpagehacker.com/a_download.exe", "filename=My Downloaded File.exe", "", @@ -1378,6 +1415,7 @@ TEST(NetUtilTest, GenerateFileName) { L"My Downloaded File.exe" }, { // % encoded + __LINE__, "http://www.examples.com/", "attachment; " "filename=\"%EC%98%88%EC%88%A0%20%EC%98%88%EC%88%A0.jpg\"", @@ -1388,6 +1426,7 @@ TEST(NetUtilTest, GenerateFileName) { L"\uc608\uc220 \uc608\uc220.jpg" }, { // name= parameter + __LINE__, "http://www.examples.com/q.cgi?id=abc", "attachment; name=abc de.pdf", "", @@ -1397,6 +1436,7 @@ TEST(NetUtilTest, GenerateFileName) { L"abc de.pdf" }, { + __LINE__, "http://www.example.com/path", "filename=\"=?EUC-JP?Q?=B7=DD=BD=D13=2Epng?=\"", "", @@ -1407,6 +1447,7 @@ TEST(NetUtilTest, GenerateFileName) { }, { // The following two have invalid CD headers and filenames come from the // URL. + __LINE__, "http://www.example.com/test%20123", "attachment; filename==?iiso88591?Q?caf=EG?=", "", @@ -1416,6 +1457,7 @@ TEST(NetUtilTest, GenerateFileName) { L"test 123" JPEG_EXT }, { + __LINE__, "http://www.google.com/%EC%98%88%EC%88%A0%20%EC%98%88%EC%88%A0.jpg", "malformed_disposition", "", @@ -1425,6 +1467,7 @@ TEST(NetUtilTest, GenerateFileName) { L"\uc608\uc220 \uc608\uc220.jpg" }, { // Invalid C-D. No filename from URL. Falls back to 'download'. + __LINE__, "http://www.google.com/path1/path2/", "attachment; filename==?iso88591?Q?caf=E3?", "", @@ -1441,6 +1484,7 @@ TEST(NetUtilTest, GenerateFileName) { const GenerateFilenameCase generation_tests[] = { // Dotfiles. Ensures preceeding period(s) stripped. { + __LINE__, "http://www.google.com/.test.html", "", "", @@ -1450,6 +1494,7 @@ TEST(NetUtilTest, GenerateFileName) { L"test.html" }, { + __LINE__, "http://www.google.com/.test", "", "", @@ -1459,6 +1504,7 @@ TEST(NetUtilTest, GenerateFileName) { L"test" }, { + __LINE__, "http://www.google.com/..test", "", "", @@ -1468,42 +1514,47 @@ TEST(NetUtilTest, GenerateFileName) { L"test" }, { // Disposition has relative paths, remove directory separators + __LINE__, "http://www.evil.com/my_download.txt", "filename=../../../../././../a_file_name.txt", "", "", "text/plain", L"download", - L"_.._.._.._._._.._a_file_name.txt" + L"-..-..-..-.-.-..-a_file_name.txt" }, { // Disposition has parent directories, remove directory separators + __LINE__, "http://www.evil.com/my_download.txt", "filename=dir1/dir2/a_file_name.txt", "", "", "text/plain", L"download", - L"dir1_dir2_a_file_name.txt" + L"dir1-dir2-a_file_name.txt" }, { // Disposition has relative paths, remove directory separators + __LINE__, "http://www.evil.com/my_download.txt", "filename=..\\..\\..\\..\\.\\.\\..\\a_file_name.txt", "", "", "text/plain", L"download", - L"_.._.._.._._._.._a_file_name.txt" + L"-..-..-..-.-.-..-a_file_name.txt" }, { // Disposition has parent directories, remove directory separators + __LINE__, "http://www.evil.com/my_download.txt", "filename=dir1\\dir2\\a_file_name.txt", "", "", "text/plain", L"download", - L"dir1_dir2_a_file_name.txt" + L"dir1-dir2-a_file_name.txt" }, { // No useful information in disposition or URL, use default + __LINE__, "http://www.truncated.com/path/", "", "", @@ -1513,15 +1564,17 @@ TEST(NetUtilTest, GenerateFileName) { L"download" TXT_EXT }, { // Filename looks like HTML? + __LINE__, "http://www.evil.com/get/malware/here", "filename=\"<blink>Hello kitty</blink>\"", "", "", "text/plain", L"default", - L"-blink-Hello kitty-_blink-" TXT_EXT + L"-blink-Hello kitty--blink-" TXT_EXT }, { // A normal avi should get .avi and not .avi.avi + __LINE__, "https://blah.google.com/misc/2.avi", "", "", @@ -1531,6 +1584,7 @@ TEST(NetUtilTest, GenerateFileName) { L"2.avi" }, { // Extension generation + __LINE__, "http://www.example.com/my-cat", "filename=my-cat", "", @@ -1540,6 +1594,7 @@ TEST(NetUtilTest, GenerateFileName) { L"my-cat" JPEG_EXT }, { + __LINE__, "http://www.example.com/my-cat", "filename=my-cat", "", @@ -1549,6 +1604,7 @@ TEST(NetUtilTest, GenerateFileName) { L"my-cat.txt" }, { + __LINE__, "http://www.example.com/my-cat", "filename=my-cat", "", @@ -1558,6 +1614,7 @@ TEST(NetUtilTest, GenerateFileName) { L"my-cat" HTML_EXT }, { // Unknown MIME type + __LINE__, "http://www.example.com/my-cat", "filename=my-cat", "", @@ -1567,6 +1624,7 @@ TEST(NetUtilTest, GenerateFileName) { L"my-cat" }, { + __LINE__, "http://www.example.com/my-cat.jpg", "filename=my-cat.jpg", "", @@ -1578,6 +1636,7 @@ TEST(NetUtilTest, GenerateFileName) { // Windows specific tests #if defined(OS_WIN) { + __LINE__, "http://www.goodguy.com/evil.exe", "filename=evil.exe", "", @@ -1587,6 +1646,7 @@ TEST(NetUtilTest, GenerateFileName) { L"evil.exe" }, { + __LINE__, "http://www.goodguy.com/ok.exe", "filename=ok.exe", "", @@ -1596,6 +1656,7 @@ TEST(NetUtilTest, GenerateFileName) { L"ok.exe" }, { + __LINE__, "http://www.goodguy.com/evil.dll", "filename=evil.dll", "", @@ -1605,6 +1666,7 @@ TEST(NetUtilTest, GenerateFileName) { L"evil.dll" }, { + __LINE__, "http://www.goodguy.com/evil.exe", "filename=evil", "", @@ -1615,6 +1677,7 @@ TEST(NetUtilTest, GenerateFileName) { }, // Test truncation of trailing dots and spaces { + __LINE__, "http://www.goodguy.com/evil.exe ", "filename=evil.exe ", "", @@ -1624,6 +1687,7 @@ TEST(NetUtilTest, GenerateFileName) { L"evil.exe" }, { + __LINE__, "http://www.goodguy.com/evil.exe.", "filename=evil.exe.", "", @@ -1633,6 +1697,7 @@ TEST(NetUtilTest, GenerateFileName) { L"evil.exe-" }, { + __LINE__, "http://www.goodguy.com/evil.exe. . .", "filename=evil.exe. . .", "", @@ -1642,6 +1707,7 @@ TEST(NetUtilTest, GenerateFileName) { L"evil.exe-------" }, { + __LINE__, "http://www.goodguy.com/evil.", "filename=evil.", "", @@ -1651,6 +1717,7 @@ TEST(NetUtilTest, GenerateFileName) { L"evil-" }, { + __LINE__, "http://www.goodguy.com/. . . . .", "filename=. . . . .", "", @@ -1660,6 +1727,7 @@ TEST(NetUtilTest, GenerateFileName) { L"download" }, { + __LINE__, "http://www.badguy.com/attachment?name=meh.exe%C2%A0", "attachment; filename=\"meh.exe\xC2\xA0\"", "", @@ -1670,6 +1738,7 @@ TEST(NetUtilTest, GenerateFileName) { }, #endif // OS_WIN { + __LINE__, "http://www.goodguy.com/utils.js", "filename=utils.js", "", @@ -1679,6 +1748,7 @@ TEST(NetUtilTest, GenerateFileName) { L"utils.js" }, { + __LINE__, "http://www.goodguy.com/contacts.js", "filename=contacts.js", "", @@ -1688,6 +1758,7 @@ TEST(NetUtilTest, GenerateFileName) { L"contacts.js" }, { + __LINE__, "http://www.goodguy.com/utils.js", "filename=utils.js", "", @@ -1697,6 +1768,7 @@ TEST(NetUtilTest, GenerateFileName) { L"utils.js" }, { + __LINE__, "http://www.goodguy.com/utils.js", "filename=utils.js", "", @@ -1706,6 +1778,7 @@ TEST(NetUtilTest, GenerateFileName) { L"utils.js" }, { + __LINE__, "http://www.goodguy.com/utils.js", "filename=utils.js", "", @@ -1715,15 +1788,17 @@ TEST(NetUtilTest, GenerateFileName) { L"utils.js" }, { + __LINE__, "http://www.goodguy.com/utils.js", - "filename=utils.js", - "", - "", - "application/ecmascript;version=4", - L"download", - L"utils.js" + "filename=utils.js", + "", + "", + "application/ecmascript;version=4", + L"download", + L"utils.js" }, { + __LINE__, "http://www.goodguy.com/program.exe", "filename=program.exe", "", @@ -1733,24 +1808,27 @@ TEST(NetUtilTest, GenerateFileName) { L"program.exe" }, { + __LINE__, "http://www.evil.com/../foo.txt", "filename=../foo.txt", "", "", "text/plain", L"download", - L"_foo.txt" + L"-foo.txt" }, { + __LINE__, "http://www.evil.com/..\\foo.txt", "filename=..\\foo.txt", "", "", "text/plain", L"download", - L"_foo.txt" + L"-foo.txt" }, { + __LINE__, "http://www.evil.com/.hidden", "filename=.hidden", "", @@ -1760,6 +1838,7 @@ TEST(NetUtilTest, GenerateFileName) { L"hidden" TXT_EXT }, { + __LINE__, "http://www.evil.com/trailing.", "filename=trailing.", "", @@ -1770,9 +1849,10 @@ TEST(NetUtilTest, GenerateFileName) { L"trailing-" #else L"trailing" -#endif //OS_WIN +#endif }, { + __LINE__, "http://www.evil.com/trailing.", "filename=trailing.", "", @@ -1783,9 +1863,10 @@ TEST(NetUtilTest, GenerateFileName) { L"trailing-" TXT_EXT #else L"trailing" TXT_EXT -#endif //OS_WIN +#endif }, { + __LINE__, "http://www.evil.com/.", "filename=.", "", @@ -1795,6 +1876,7 @@ TEST(NetUtilTest, GenerateFileName) { L"download" }, { + __LINE__, "http://www.evil.com/..", "filename=..", "", @@ -1804,6 +1886,7 @@ TEST(NetUtilTest, GenerateFileName) { L"download" }, { + __LINE__, "http://www.evil.com/...", "filename=...", "", @@ -1813,6 +1896,7 @@ TEST(NetUtilTest, GenerateFileName) { L"download" }, { // Note that this one doesn't have "filename=" on it. + __LINE__, "http://www.evil.com/", "a_file_name.txt", "", @@ -1822,6 +1906,7 @@ TEST(NetUtilTest, GenerateFileName) { L"download" JPEG_EXT }, { + __LINE__, "http://www.evil.com/", "filename=", "", @@ -1831,6 +1916,7 @@ TEST(NetUtilTest, GenerateFileName) { L"download" JPEG_EXT }, { + __LINE__, "http://www.example.com/simple", "filename=simple", "", @@ -1841,6 +1927,7 @@ TEST(NetUtilTest, GenerateFileName) { }, // Reserved words on Windows { + __LINE__, "http://www.goodguy.com/COM1", "filename=COM1", "", @@ -1854,6 +1941,7 @@ TEST(NetUtilTest, GenerateFileName) { #endif }, { + __LINE__, "http://www.goodguy.com/COM4.txt", "filename=COM4.txt", "", @@ -1867,6 +1955,7 @@ TEST(NetUtilTest, GenerateFileName) { #endif }, { + __LINE__, "http://www.goodguy.com/lpt1.TXT", "filename=lpt1.TXT", "", @@ -1880,6 +1969,7 @@ TEST(NetUtilTest, GenerateFileName) { #endif }, { + __LINE__, "http://www.goodguy.com/clock$.txt", "filename=clock$.txt", "", @@ -1893,6 +1983,7 @@ TEST(NetUtilTest, GenerateFileName) { #endif }, { // Validation should also apply to sugested name + __LINE__, "http://www.goodguy.com/blah$.txt", "filename=clock$.txt", "", @@ -1906,6 +1997,7 @@ TEST(NetUtilTest, GenerateFileName) { #endif }, { + __LINE__, "http://www.goodguy.com/mycom1.foo", "filename=mycom1.foo", "", @@ -1915,6 +2007,7 @@ TEST(NetUtilTest, GenerateFileName) { L"mycom1.foo" }, { + __LINE__, "http://www.badguy.com/Setup.exe.local", "filename=Setup.exe.local", "", @@ -1928,6 +2021,7 @@ TEST(NetUtilTest, GenerateFileName) { #endif }, { + __LINE__, "http://www.badguy.com/Setup.exe.local", "filename=Setup.exe.local.local", "", @@ -1941,6 +2035,7 @@ TEST(NetUtilTest, GenerateFileName) { #endif }, { + __LINE__, "http://www.badguy.com/Setup.exe.lnk", "filename=Setup.exe.lnk", "", @@ -1954,6 +2049,7 @@ TEST(NetUtilTest, GenerateFileName) { #endif }, { + __LINE__, "http://www.badguy.com/Desktop.ini", "filename=Desktop.ini", "", @@ -1967,6 +2063,7 @@ TEST(NetUtilTest, GenerateFileName) { #endif }, { + __LINE__, "http://www.badguy.com/Thumbs.db", "filename=Thumbs.db", "", @@ -1980,6 +2077,7 @@ TEST(NetUtilTest, GenerateFileName) { #endif }, { + __LINE__, "http://www.hotmail.com", "filename=source.jpg", "", @@ -1989,6 +2087,7 @@ TEST(NetUtilTest, GenerateFileName) { L"source.jpg" }, { // http://crbug.com/5772. + __LINE__, "http://www.example.com/foo.tar.gz", "", "", @@ -1998,6 +2097,7 @@ TEST(NetUtilTest, GenerateFileName) { L"foo.tar.gz" }, { // http://crbug.com/52250. + __LINE__, "http://www.example.com/foo.tgz", "", "", @@ -2007,6 +2107,7 @@ TEST(NetUtilTest, GenerateFileName) { L"foo.tgz" }, { // http://crbug.com/7337. + __LINE__, "http://maged.lordaeron.org/blank.reg", "", "", @@ -2016,6 +2117,7 @@ TEST(NetUtilTest, GenerateFileName) { L"blank.reg" }, { + __LINE__, "http://www.example.com/bar.tar", "", "", @@ -2025,6 +2127,7 @@ TEST(NetUtilTest, GenerateFileName) { L"bar.tar" }, { + __LINE__, "http://www.example.com/bar.bogus", "", "", @@ -2034,15 +2137,17 @@ TEST(NetUtilTest, GenerateFileName) { L"bar.bogus" }, { // http://crbug.com/20337 + __LINE__, "http://www.example.com/.download.txt", "filename=.download.txt", "", "", "text/plain", - L"download", + L"-download", L"download.txt" }, { // http://crbug.com/56855. + __LINE__, "http://www.example.com/bar.sh", "", "", @@ -2052,6 +2157,7 @@ TEST(NetUtilTest, GenerateFileName) { L"bar.sh" }, { // http://crbug.com/61571 + __LINE__, "http://www.example.com/npdf.php?fn=foobar.pdf", "", "", @@ -2061,6 +2167,7 @@ TEST(NetUtilTest, GenerateFileName) { L"npdf" TXT_EXT }, { // Shouldn't overwrite C-D specified extension. + __LINE__, "http://www.example.com/npdf.php?fn=foobar.pdf", "filename=foobar.jpg", "", @@ -2070,6 +2177,7 @@ TEST(NetUtilTest, GenerateFileName) { L"foobar.jpg" }, { // http://crbug.com/87719 + __LINE__, "http://www.example.com/image.aspx?id=blargh", "", "", @@ -2080,6 +2188,7 @@ TEST(NetUtilTest, GenerateFileName) { }, #if defined(OS_CHROMEOS) { // http://crosbug.com/26028 + __LINE__, "http://www.example.com/fooa%cc%88.txt", "", "", @@ -2092,15 +2201,15 @@ TEST(NetUtilTest, GenerateFileName) { }; for (size_t i = 0; i < ARRAYSIZE_UNSAFE(selection_tests); ++i) - RunGenerateFileNameTestCase(&selection_tests[i], i, "selection"); + RunGenerateFileNameTestCase(&selection_tests[i]); for (size_t i = 0; i < ARRAYSIZE_UNSAFE(generation_tests); ++i) - RunGenerateFileNameTestCase(&generation_tests[i], i, "generation"); + RunGenerateFileNameTestCase(&generation_tests[i]); for (size_t i = 0; i < ARRAYSIZE_UNSAFE(generation_tests); ++i) { GenerateFilenameCase test_case = generation_tests[i]; test_case.referrer_charset = "GBK"; - RunGenerateFileNameTestCase(&test_case, i, "generation (referrer=GBK)"); + RunGenerateFileNameTestCase(&test_case); } } @@ -3302,4 +3411,86 @@ TEST(NetUtilTest, GetNetworkList) { } } +static const base::FilePath::CharType* kSafePortableBasenames[] = { + FILE_PATH_LITERAL("a"), + FILE_PATH_LITERAL("a.txt"), + FILE_PATH_LITERAL("a b.txt"), + FILE_PATH_LITERAL("a-b.txt"), + FILE_PATH_LITERAL("My Computer"), + FILE_PATH_LITERAL(" Computer"), +}; + +static const base::FilePath::CharType* kUnsafePortableBasenames[] = { + FILE_PATH_LITERAL(""), + FILE_PATH_LITERAL("."), + FILE_PATH_LITERAL(".."), + FILE_PATH_LITERAL("..."), + FILE_PATH_LITERAL("con"), + FILE_PATH_LITERAL("con.zip"), + FILE_PATH_LITERAL("NUL"), + FILE_PATH_LITERAL("NUL.zip"), + FILE_PATH_LITERAL(".a"), + FILE_PATH_LITERAL("a."), + FILE_PATH_LITERAL("a\"a"), + FILE_PATH_LITERAL("a<a"), + FILE_PATH_LITERAL("a>a"), + FILE_PATH_LITERAL("a?a"), + FILE_PATH_LITERAL("a/"), + FILE_PATH_LITERAL("a\\"), + FILE_PATH_LITERAL("a "), + FILE_PATH_LITERAL("a . ."), + FILE_PATH_LITERAL("My Computer.{a}"), + FILE_PATH_LITERAL("My Computer.{20D04FE0-3AEA-1069-A2D8-08002B30309D}"), +#if !defined(OS_WIN) + FILE_PATH_LITERAL("a\\a"), +#endif +}; + +static const base::FilePath::CharType* kSafePortableRelativePaths[] = { + FILE_PATH_LITERAL("a/a"), +#if defined(OS_WIN) + FILE_PATH_LITERAL("a\\a"), +#endif +}; + +TEST(NetUtilTest, IsSafePortableBasename) { + for (size_t i = 0 ; i < arraysize(kSafePortableBasenames); ++i) { + EXPECT_TRUE(IsSafePortableBasename(base::FilePath( + kSafePortableBasenames[i]))) << kSafePortableBasenames[i]; + } + for (size_t i = 0 ; i < arraysize(kUnsafePortableBasenames); ++i) { + EXPECT_FALSE(IsSafePortableBasename(base::FilePath( + kUnsafePortableBasenames[i]))) << kUnsafePortableBasenames[i]; + } + for (size_t i = 0 ; i < arraysize(kSafePortableRelativePaths); ++i) { + EXPECT_FALSE(IsSafePortableBasename(base::FilePath( + kSafePortableRelativePaths[i]))) << kSafePortableRelativePaths[i]; + } +} + +TEST(NetUtilTest, IsSafePortableRelativePath) { + base::FilePath safe_dirname(FILE_PATH_LITERAL("a")); + for (size_t i = 0 ; i < arraysize(kSafePortableBasenames); ++i) { + EXPECT_TRUE(IsSafePortableRelativePath(base::FilePath( + kSafePortableBasenames[i]))) << kSafePortableBasenames[i]; + EXPECT_TRUE(IsSafePortableRelativePath(safe_dirname.Append(base::FilePath( + kSafePortableBasenames[i])))) << kSafePortableBasenames[i]; + } + for (size_t i = 0 ; i < arraysize(kSafePortableRelativePaths); ++i) { + EXPECT_TRUE(IsSafePortableRelativePath(base::FilePath( + kSafePortableRelativePaths[i]))) << kSafePortableRelativePaths[i]; + EXPECT_TRUE(IsSafePortableRelativePath(safe_dirname.Append(base::FilePath( + kSafePortableRelativePaths[i])))) << kSafePortableRelativePaths[i]; + } + for (size_t i = 0 ; i < arraysize(kUnsafePortableBasenames); ++i) { + EXPECT_FALSE(IsSafePortableRelativePath(base::FilePath( + kUnsafePortableBasenames[i]))) << kUnsafePortableBasenames[i]; + if (!base::FilePath::StringType(kUnsafePortableBasenames[i]).empty()) { + EXPECT_FALSE(IsSafePortableRelativePath(safe_dirname.Append( + base::FilePath(kUnsafePortableBasenames[i])))) + << kUnsafePortableBasenames[i]; + } + } +} + } // namespace net |