Fix content verification code for undreadable and deleted files.

extensions/browser/content_verify_job_unittest.cc

Index: extensions/browser/content_verify_job_unittest.cc
diff --git a/extensions/browser/content_verify_job_unittest.cc b/extensions/browser/content_verify_job_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d2a098eff6f5b8f283560dd4833374ceb5c5c571
--- /dev/null
+++ b/extensions/browser/content_verify_job_unittest.cc
@@ -0,0 +1,215 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/memory/weak_ptr.h"

[Devlin, 2017/03/29 15:51:57]

needed?

[lazyboy, 2017/03/29 17:12:50]

No, removed.

+#include "base/path_service.h"
+#include "base/run_loop.h"
+#include "base/version.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "extensions/browser/content_hash_reader.h"
+#include "extensions/browser/extensions_test.h"
+#include "extensions/common/constants.h"
+#include "extensions/common/extension_paths.h"
+#include "extensions/common/file_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/zlib/google/zip.h"
+
+namespace extensions {
+
+namespace {
+
+extensions::ContentHashReader* CreateContentHashReader(

[Devlin, 2017/03/29 15:51:57]

nit: return a refptr

[lazyboy, 2017/03/29 17:12:50]

Done.

+    const Extension& extension,
+    base::FilePath& extension_resource_path) {
+  return new ContentHashReader(
+      extension.id(), *extension.version(), extension.path(),
+      extension_resource_path,
+      ContentVerifierKey(kWebstoreSignaturesPublicKey,
+                         kWebstoreSignaturesPublicKeySize));
+}
+
+void DoNothingWithReasonParam(
+    extensions::ContentVerifyJob::FailureReason reason) {}

[Devlin, 2017/03/29 15:51:57]

nit: no extensions:: prefix (also the rest of this file)

[lazyboy, 2017/03/29 17:12:50]

Done.

+
+class JobTestObserver : public extensions::ContentVerifyJob::TestObserver {
+ public:
+  JobTestObserver(const std::string& extension_id,
+                  const base::FilePath& relative_path)
+      : extension_id_(extension_id), relative_path_(relative_path) {
+    extensions::ContentVerifyJob::SetObserverForTests(this);
+  }
+  ~JobTestObserver() {
+    extensions::ContentVerifyJob::SetObserverForTests(nullptr);
+  }
+
+  void JobStarted(const std::string& extension_id,
+                  const base::FilePath& relative_path) override {}
+
+  void JobFinished(const std::string& extension_id,
+                   const base::FilePath& relative_path,
+                   ContentVerifyJob::FailureReason reason) override {
+    if (extension_id != extension_id_ || relative_path != relative_path_)
+      return;
+    failure_reason_ = reason;
+    run_loop_.Quit();
+  }
+
+  extensions::ContentVerifyJob::FailureReason WaitAndGetFailureReason() {
+    // Run() returns immediately if Quit() has already been called.
+    run_loop_.Run();
+    return failure_reason_;

[Devlin, 2017/03/29 15:51:56]

maybe make failure_reason_ a base::Optional<>, and then DCHECK/EXPECT_TRUE it
here?

[lazyboy, 2017/03/29 17:12:50]

Done.

+  }
+
+ private:
+  base::RunLoop run_loop_;
+  std::string extension_id_;
+  base::FilePath relative_path_;
+  extensions::ContentVerifyJob::FailureReason failure_reason_;
+
+  DISALLOW_COPY_AND_ASSIGN(JobTestObserver);
+};
+
+}  // namespace
+
+class ContentVerifyJobUnittest : public ExtensionsTest {
+ public:
+  ContentVerifyJobUnittest() {}
+  ~ContentVerifyJobUnittest() override {}
+
+  void SetUp() override {
+    ExtensionsTest::SetUp();
+
+    // Needed for ContentVerifyJob::Start().
+    browser_threads_.reset(new content::TestBrowserThreadBundle(

[Devlin, 2017/03/29 15:51:57]

prefer base::MakeUnique

[lazyboy, 2017/03/29 17:12:50]

Done.

+        content::TestBrowserThreadBundle::REAL_IO_THREAD));
+  }
+
+  // Helper to get files from our subdirectory in the general extensions test
+  // data dir.
+  base::FilePath GetTestPath(const base::FilePath& relative_path) {
+    base::FilePath base_path;
+    EXPECT_TRUE(PathService::Get(extensions::DIR_TEST_DATA, &base_path));
+    base_path = base_path.AppendASCII("content_hash_fetcher");
+    return base_path.Append(relative_path);
+  }
+
+  // Unzips the extension source from |extension_zip| into a temporary
+  // directory and loads it. Returns the resuling Extension object.
+  // |destination| points to the path where the extension was extracted.
+  scoped_refptr<Extension> UnzipToTempDirAndLoad(
+      const base::FilePath& extension_zip,
+      base::FilePath* destination) {
+    EXPECT_TRUE(temp_dir_.CreateUniqueTempDir());
+    *destination = temp_dir_.GetPath();
+    EXPECT_TRUE(zip::Unzip(extension_zip, *destination));
+
+    std::string error;
+    scoped_refptr<Extension> extension = file_util::LoadExtension(
+        *destination, Manifest::INTERNAL, 0 /* flags */, &error);
+    EXPECT_NE(nullptr, extension.get()) << " error:'" << error << "'";
+    return extension;
+  }
+
+ private:
+  base::ScopedTempDir temp_dir_;
+  std::unique_ptr<content::TestBrowserThreadBundle> browser_threads_;
+
+  DISALLOW_COPY_AND_ASSIGN(ContentVerifyJobUnittest);
+};
+
+// Tests that deleted legitimate files trigger content verification failure.
+// Also tests that non-existent file request does not trigger content
+// verification failure.
+TEST_F(ContentVerifyJobUnittest, DeletedAndMissingFiles) {
+  base::FilePath unzipped_path;
+  base::FilePath test_dir_base =
+      GetTestPath(base::FilePath(FILE_PATH_LITERAL("with_verified_contents")));

[Devlin, 2017/03/29 15:51:57]

I don't see this - did you forget to add it to the patch?

[lazyboy, 2017/03/29 17:12:50]

Yes, added now.

+  scoped_refptr<Extension> extension = UnzipToTempDirAndLoad(
+      test_dir_base.AppendASCII("source_all.zip"), &unzipped_path);
+  ASSERT_TRUE(extension.get());
+  // Make sure there is a verified_contents.json file there as this test cannot
+  // fetch it.
+  EXPECT_TRUE(
+      base::PathExists(file_util::GetVerifiedContentsPath(extension->path())));
+
+  const base::FilePath::CharType kExistentResource[] =
+      FILE_PATH_LITERAL("background.js");
+  base::FilePath existent_resource_path(kExistentResource);
+  {
+    // Make sure background.js passes verification correctly.
+    JobTestObserver observer(extension->id(), existent_resource_path);
+
+    scoped_refptr<ContentHashReader> content_hash_reader =
+        CreateContentHashReader(*extension.get(), existent_resource_path);
+    scoped_refptr<ContentVerifyJob> verify_job = new ContentVerifyJob(
+        content_hash_reader.get(), base::Bind(&DoNothingWithReasonParam));
+    verify_job->Start();
+    {
+      // Simulate serving background.js.
+      std::string background_contents;
+      base::ReadFileToString(
+          unzipped_path.Append(base::FilePath(kExistentResource)),
+          &background_contents);
+      verify_job->BytesRead(background_contents.size(),
+                            base::string_as_array(&background_contents));
+      verify_job->DoneReading();
+    }
+    ContentVerifyJob::FailureReason reason = observer.WaitAndGetFailureReason();
+    // Expect no content-verification failure.
+    EXPECT_EQ(ContentVerifyJob::NONE, reason);
+  }
+
+  {
+    // Once background.js is deleted, verification will result in HASH_MISMATCH.
+    JobTestObserver observer(extension->id(), existent_resource_path);
+    // Now delete the existent file.
+    EXPECT_TRUE(base::DeleteFile(
+        unzipped_path.Append(base::FilePath(kExistentResource)), false));
+
+    scoped_refptr<ContentHashReader> content_hash_reader =
+        CreateContentHashReader(*extension.get(), existent_resource_path);
+    scoped_refptr<ContentVerifyJob> verify_job = new ContentVerifyJob(
+        content_hash_reader.get(), base::Bind(&DoNothingWithReasonParam));
+    verify_job->Start();
+    {
+      // Simulate serving deleted background.js.
+      std::string tmp;
+      verify_job->BytesRead(0, base::string_as_array(&tmp));
+      verify_job->DoneReading();
+    }
+    ContentVerifyJob::FailureReason reason = observer.WaitAndGetFailureReason();
+    // Expect content-verification failure.

[Devlin, 2017/03/29 15:51:56]

nitty nit: comment probably unnecessary with the comment on line 168 + the
expect_eq

[lazyboy, 2017/03/29 17:12:50]

Done.

+    EXPECT_EQ(ContentVerifyJob::HASH_MISMATCH, reason);
+  }
+
+  {
+    // Now ask for a non-existent resource non-existent.js. Verification should
+    // skip this file as it is not listed in our verified_contents.json file.
+    const base::FilePath::CharType kNonExistentResource[] =
+        FILE_PATH_LITERAL("non-existent.js");
+    base::FilePath non_existent_resource_path(kNonExistentResource);
+    JobTestObserver observer(extension->id(), non_existent_resource_path);
+
+    scoped_refptr<ContentHashReader> content_hash_reader =
+        CreateContentHashReader(*extension.get(), non_existent_resource_path);
+    scoped_refptr<ContentVerifyJob> verify_job = new ContentVerifyJob(
+        content_hash_reader.get(), base::Bind(&DoNothingWithReasonParam));
+    verify_job->Start();
+    {
+      // Simulate non existent file read.
+      std::string tmp;
+      verify_job->BytesRead(0, base::string_as_array(&tmp));
+      verify_job->DoneReading();
+    }
+    ContentVerifyJob::FailureReason reason = observer.WaitAndGetFailureReason();
+    // Expect no content-verification failure.
+    EXPECT_EQ(ContentVerifyJob::NONE, reason);
+  }
+}

[Devlin, 2017/03/29 15:51:57]

Since we're adding unit tests for this (yay! :)), can we add a quick one for a
hash mismatch on a non-empty file?  e.g. just rewriting background.js with
"foo"?

[lazyboy, 2017/03/29 17:12:50]

Good idea, added.

+
+}  // namespace extensions