Add unit tests for the interpretation of quotes in SandboxOpenReadOnlyFile and SandboxDeleteFile

R=proberge

Bug: 1023545
Change-Id: Icc7f7ed30ba4ac51fbf5725e61cfafeb2aebe150
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1906475Reviewed-by: proberge <proberge@chromium.org>
Commit-Queue: Joe Mason <joenotcharles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714481}

chrome/chrome_cleaner/engines/broker/cleaner_sandbox_interface_unittest.cc

@@ -327,6 +327,49 @@ TEST_F(CleanerSandboxInterfaceDeleteFileTest, AllowTrailingWhitespace) {
   EXPECT_FALSE(base::PathExists(file_path));
 }
 
+TEST_F(CleanerSandboxInterfaceDeleteFileTest, QuotedPath) {
+  base::ScopedTempDir temp;
+  ASSERT_TRUE(temp.CreateUniqueTempDir());
+  base::FilePath file_path = temp.GetPath().Append(L"temp_file.exe");
+
+  ASSERT_TRUE(chrome_cleaner::CreateFileInFolder(
+      file_path.DirName(), file_path.BaseName().value().c_str()));
+
+  const base::FilePath quoted_path(L"\"" + file_path.value() + L"\"");
+
+  // RemoveNow should reject the file name because it starts with an invalid
+  // character. This needs to match the behaviour of SandboxOpenFileReadOnly,
+  // which is tested in ScannerSandboxInterface_OpenReadOnlyFile.BasicFile,
+  // since the same path could be passed to both.
+  chrome_cleaner::VerifyRemoveNowFailure(quoted_path, file_remover_.get());
+  EXPECT_TRUE(base::PathExists(file_path));
+}
+
+TEST_F(CleanerSandboxInterfaceDeleteFileTest, QuotedFilename) {
+  base::ScopedTempDir temp;
+  ASSERT_TRUE(temp.CreateUniqueTempDir());
+  base::FilePath file_path = temp.GetPath().Append(L"temp_file.exe");
+
+  ASSERT_TRUE(chrome_cleaner::CreateFileInFolder(
+      file_path.DirName(), file_path.BaseName().value().c_str()));
+
+  const base::FilePath quoted_path =
+      temp.GetPath().Append(L"\"temp_file.exe\"");
+
+  // RemoveNow should return true because the file name is valid, but refers to
+  // a file that already doesn't exist. The important thing is that the quotes
+  // aren't interpreted, which would cause '"temp_file.exe"' and
+  // 'temp_file.exe' to refer to the same thing.
+  //
+  // This needs to match the behaviour of SandboxOpenFileReadOnly, which is
+  // tested in ScannerSandboxInterface_OpenReadOnlyFile.BasicFile, since the
+  // same path could be passed to both. It would also be ok if both RemoveNow
+  // and OpenFileReadOnly interpreted the quotes, as long as their behaviour
+  // matches.
+  chrome_cleaner::VerifyRemoveNowSuccess(quoted_path, file_remover_.get());
+  EXPECT_TRUE(base::PathExists(file_path));
+}
+
 TEST_F(CleanerSandboxInterfaceDeleteFileTest, DeleteAlternativeStream) {
   base::ScopedTempDir temp;
   ASSERT_TRUE(temp.CreateUniqueTempDir());

chrome/chrome_cleaner/engines/broker/scanner_sandbox_interface_unittest.cc

@@ -607,6 +607,24 @@ TEST_F(ScannerSandboxInterface_OpenReadOnlyFile, BasicFile) {
   handle = SandboxOpenReadOnlyFile(base::FilePath(path_with_space),
                                    FILE_ATTRIBUTE_NORMAL);
   EXPECT_TRUE(handle.IsValid());
+
+  // Make sure quotes aren't interpreted. The same path might be passed to
+  // SandboxDeleteFile, which doesn't interpret quotes.
+  const base::FilePath quoted_path(L"\"" + file_path.value() + L"\"");
+  handle = SandboxOpenReadOnlyFile(quoted_path, FILE_ATTRIBUTE_NORMAL);
+  EXPECT_FALSE(handle.IsValid())
+      << "SandboxOpenReadOnlyFile is interpreting quotes around path names; "
+         "this will cause problems if the same path is passed to DeleteFile "
+         "(see CleanerSandboxInterfaceDeleteFileTest.QuotedPath)";
+
+  const base::FilePath partly_quoted_path =
+      temp.GetPath().Append(L"\"temp_file.exe\"");
+  handle = SandboxOpenReadOnlyFile(partly_quoted_path, FILE_ATTRIBUTE_NORMAL);
+  EXPECT_FALSE(handle.IsValid())
+      << "SandboxOpenReadOnlyFile is interpreting quotes around path "
+         "components; this will cause problems if the same path is passed to "
+         "DeleteFile (see "
+         "CleanerSandboxInterfaceDeleteFileTest.QuotedFilename)";
 }
 
 TEST_F(ScannerSandboxInterface_OpenReadOnlyFile, NoFile) {