Fix content verification code for undreadable and deleted files.

chrome/browser/extensions/extension_protocols_unittest.cc

Index: chrome/browser/extensions/extension_protocols_unittest.cc
diff --git a/chrome/browser/extensions/extension_protocols_unittest.cc b/chrome/browser/extensions/extension_protocols_unittest.cc
index e3da5f8a4a3d0642b7ef4b413e386b2063fc8c56..f24b1566e69dc058ede1846dcd74b48d9468ff84 100644
--- a/chrome/browser/extensions/extension_protocols_unittest.cc
+++ b/chrome/browser/extensions/extension_protocols_unittest.cc
@@ -7,23 +7,32 @@
 #include <memory>
 #include <string>
 
+#include "base/command_line.h"
 #include "base/files/file_util.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
+#include "base/test/test_file_util.h"
 #include "base/values.h"
+#include "chrome/browser/extensions/chrome_content_verifier_delegate.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/crx_file/id_util.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/previews_state.h"
 #include "content/public/test/mock_resource_context.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/test_utils.h"
+#include "extensions/browser/content_verifier.h"
 #include "extensions/browser/extension_protocols.h"
 #include "extensions/browser/info_map.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/extension_builder.h"
 #include "extensions/common/file_util.h"
 #include "net/base/request_priority.h"
 #include "net/url_request/url_request.h"
@@ -43,6 +52,23 @@ base::FilePath GetTestPath(const std::string& name) {
   return path.AppendASCII("extensions").AppendASCII(name);
 }
 
+// Helper function that creates a file at |relative_path| within |directory|
+// and fills it with |content|.
+bool AddFileToDirectory(const base::FilePath& directory,
+                        const base::FilePath& relative_path,
+                        const std::string& content) {
+  base::FilePath full_path = directory.Append(relative_path);
+  if (!base::CreateDirectory(full_path.DirName()))

[Devlin, 2017/03/24 02:01:44]

In practice, shouldn't this be created as part of CreateUniqueTempDir()?

[lazyboy, 2017/03/24 18:27:47]

Of course. I copy pasted this function.
Done.

+    return false;
+  int result = base::WriteFile(full_path, content.data(), content.size());
+  return (static_cast<size_t>(result) == content.size());
+}
+
+std::unique_ptr<TestingProfile> GetTestingProfile() {
+  TestingProfile::Builder profile_builder;
+  return profile_builder.Build();
+}
+
 scoped_refptr<Extension> CreateTestExtension(const std::string& name,
                                              bool incognito_split_mode) {
   base::DictionaryValue manifest;
@@ -98,24 +124,79 @@ scoped_refptr<Extension> CreateTestResponseHeaderExtension() {
   return extension;
 }
 
+// A ContentVerifyJob::TestDelegate that observes DoneReading().
+class JobDelegate : public ContentVerifyJob::TestDelegate {
+ public:
+  JobDelegate() {}
+  ~JobDelegate() override {}
+
+  ContentVerifyJob::FailureReason BytesRead(const ExtensionId& id,
+                                            int count,

[Devlin, 2017/03/24 02:01:44]

Is it worth verifying the bytes read against expectations?

[lazyboy, 2017/03/24 18:27:47]

Not in particular for this test, but doesn't hurt.
Done.

+                                            const char* data) override {
+    return ContentVerifyJob::NONE;
+  }
+
+  ContentVerifyJob::FailureReason DoneReading(const ExtensionId& id) override {
+    seen_done_reading_extension_ids_.insert(id);
+    if (waiting_for_extension_id_ && *waiting_for_extension_id_ == id &&
+        loop_runner_.get() && loop_runner_->loop_running()) {
+      loop_runner_->Quit();
+    }
+    return ContentVerifyJob::NONE;
+  }
+
+  void WaitForDoneReading(const ExtensionId& id) {
+    ASSERT_FALSE(waiting_for_extension_id_);
+    if (seen_done_reading_extension_ids_.count(id))
+      return;
+    waiting_for_extension_id_ = base::MakeUnique<ExtensionId>(id);
+    loop_runner_ = new content::MessageLoopRunner();

[Devlin, 2017/03/24 02:01:44]

I typically prefer base::RunLoop for this, since the Quit() syntax is a little
easier and more forgiving (e.g., you can call Quit() when the loop isn't
running, etc).  Is there a reason to use MessageLoopRunner here?

[lazyboy, 2017/03/24 18:27:47]

Thanks.
Done.

+    loop_runner_->Run();
+    loop_runner_ = nullptr;
+  }
+
+  void Reset() {
+    waiting_for_extension_id_.reset();
+    seen_done_reading_extension_ids_.clear();
+  }
+
+ private:
+  std::set<ExtensionId> seen_done_reading_extension_ids_;
+  std::unique_ptr<ExtensionId> waiting_for_extension_id_;

[Devlin, 2017/03/24 02:01:44]

nit: I think base::Optional is a better fit here.

[lazyboy, 2017/03/24 18:27:47]

Done.

+  scoped_refptr<content::MessageLoopRunner> loop_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(JobDelegate);
+};
+
 }  // namespace
 
 // This test lives in src/chrome instead of src/extensions because it tests
 // functionality delegated back to Chrome via ChromeExtensionsBrowserClient.
 // See chrome/browser/extensions/chrome_url_request_util.cc.
-class ExtensionProtocolTest : public testing::Test {
+class ExtensionProtocolsTest : public testing::Test {
  public:
-  ExtensionProtocolTest()
+  ExtensionProtocolsTest()
       : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
         old_factory_(NULL),
         resource_context_(&test_url_request_context_) {}
 
   void SetUp() override {
     testing::Test::SetUp();
+    testing_profile_ = GetTestingProfile();

[Devlin, 2017/03/24 02:01:44]

nitty nit: maybe just inline:
testing_profile_ = TestingProfile::Builder().Build();

[lazyboy, 2017/03/24 18:27:47]

Done.

     extension_info_map_ = new InfoMap();
     net::URLRequestContext* request_context =
         resource_context_.GetRequestContext();
     old_factory_ = request_context->job_factory();
+
+    // Set up content verification.
+    base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+    command_line->AppendSwitchASCII(
+        switches::kExtensionContentVerification,
+        switches::kExtensionContentVerificationEnforce);
+    content_verifier_ = new ContentVerifier(
+        testing_profile_.get(),
+        new ChromeContentVerifierDelegate(testing_profile_.get()));
+    extension_info_map_->SetContentVerifier(content_verifier_.get());
   }
 
   void TearDown() override {
@@ -175,13 +256,15 @@ class ExtensionProtocolTest : public testing::Test {
   net::TestDelegate test_delegate_;
   net::TestURLRequestContext test_url_request_context_;
   content::MockResourceContext resource_context_;
+  scoped_refptr<ContentVerifier> content_verifier_;
+  std::unique_ptr<TestingProfile> testing_profile_;
 };
 
 // Tests that making a chrome-extension request in an incognito context is
 // only allowed under the right circumstances (if the extension is allowed
 // in incognito, and it's either a non-main-frame request or a split-mode
 // extension).
-TEST_F(ExtensionProtocolTest, IncognitoRequest) {
+TEST_F(ExtensionProtocolsTest, IncognitoRequest) {
   // Register an incognito extension protocol handler.
   SetProtocolHandler(true);
 
@@ -264,7 +347,7 @@ void CheckForContentLengthHeader(net::URLRequest* request) {
 
 // Tests getting a resource for a component extension works correctly, both when
 // the extension is enabled and when it is disabled.
-TEST_F(ExtensionProtocolTest, ComponentResourceRequest) {
+TEST_F(ExtensionProtocolsTest, ComponentResourceRequest) {
   // Register a non-incognito extension protocol handler.
   SetProtocolHandler(false);
 
@@ -301,7 +384,7 @@ TEST_F(ExtensionProtocolTest, ComponentResourceRequest) {
 
 // Tests that a URL request for resource from an extension returns a few
 // expected response headers.
-TEST_F(ExtensionProtocolTest, ResourceRequestResponseHeaders) {
+TEST_F(ExtensionProtocolsTest, ResourceRequestResponseHeaders) {
   // Register a non-incognito extension protocol handler.
   SetProtocolHandler(false);
 
@@ -339,7 +422,7 @@ TEST_F(ExtensionProtocolTest, ResourceRequestResponseHeaders) {
 
 // Tests that a URL request for main frame or subframe from an extension
 // succeeds, but subresources fail. See http://crbug.com/312269.
-TEST_F(ExtensionProtocolTest, AllowFrameRequests) {
+TEST_F(ExtensionProtocolsTest, AllowFrameRequests) {
   // Register a non-incognito extension protocol handler.
   SetProtocolHandler(false);
 
@@ -386,8 +469,7 @@ TEST_F(ExtensionProtocolTest, AllowFrameRequests) {
   }
 }
 
-
-TEST_F(ExtensionProtocolTest, MetadataFolder) {
+TEST_F(ExtensionProtocolsTest, MetadataFolder) {
   SetProtocolHandler(false);
 
   base::FilePath extension_dir = GetTestPath("metadata_folder");
@@ -414,4 +496,58 @@ TEST_F(ExtensionProtocolTest, MetadataFolder) {
             DoRequest(*extension, relative_path.AsUTF8Unsafe()));
 }
 
+// Tests that unreadable files and deleted files correctly go through
+// ContentVerifyJob.
+TEST_F(ExtensionProtocolsTest, VerificationSeenForFileAccessErrors) {
+  JobDelegate test_job_delegate;
+  ContentVerifyJob::SetDelegateForTests(&test_job_delegate);

[Devlin, 2017/03/24 02:01:44]

nit: clean up state; unset this at the end.

[lazyboy, 2017/03/24 18:27:47]

Done.

+  SetProtocolHandler(false);
+
+  const std::string kFooJs("foo.js");
+  // Create a temporary directory that a fake extension will live in and fill
+  // it with some test files.
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  base::FilePath foo_js(FILE_PATH_LITERAL(kFooJs));
+  ASSERT_TRUE(AddFileToDirectory(temp_dir.GetPath(), foo_js, "hello world."))
+      << "Failed to write " << temp_dir.GetPath().value() << "/"
+      << foo_js.value();
+
+  ExtensionBuilder builder;
+  builder
+      .SetManifest(DictionaryBuilder()
+                       .Set("name", "Foo")
+                       .Set("version", "1.0")
+                       .Set("manifest_version", 2)
+                       .Set("update_url",
+                            "https://clients2.google.com/service/update2/crx")
+                       .Build())
+      .SetID(crx_file::id_util::GenerateId("whatever"))
+      .SetPath(temp_dir.GetPath())
+      .SetLocation(Manifest::INTERNAL);
+  scoped_refptr<Extension> extension(builder.Build());
+
+  EXPECT_TRUE(extension.get());
+  content_verifier_->OnExtensionLoaded(testing_profile_.get(), extension.get());
+  // Wait for PostTask to ContentVerifierIOData::AddData() to finish.
+  content::RunAllPendingInMessageLoop();
+
+  // Valid and redable foo.js.

[Devlin, 2017/03/24 02:01:44]

*readable

[lazyboy, 2017/03/24 18:27:47]

Done.

+  EXPECT_EQ(net::OK, DoRequest(*extension, kFooJs));
+  test_job_delegate.WaitForDoneReading(extension->id());
+
+  // chmod -r foo.js.
+  base::FilePath foo_path = temp_dir.GetPath().AppendASCII(kFooJs);
+  ASSERT_TRUE(base::MakeFileUnreadable(foo_path));
+  test_job_delegate.Reset();
+  EXPECT_EQ(net::ERR_ACCESS_DENIED, DoRequest(*extension, kFooJs));
+  test_job_delegate.WaitForDoneReading(extension->id());
+
+  // Delete foo.js.
+  ASSERT_TRUE(base::DieFileDie(foo_path, false));
+  test_job_delegate.Reset();
+  EXPECT_EQ(net::ERR_FILE_NOT_FOUND, DoRequest(*extension, kFooJs));
+  test_job_delegate.WaitForDoneReading(extension->id());
+}
+
 }  // namespace extensions