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..0544afe618aac57ad13eb465b2e2abf683a2aff8 100644
--- a/chrome/browser/extensions/extension_protocols_unittest.cc
+++ b/chrome/browser/extensions/extension_protocols_unittest.cc
@@ -7,23 +7,31 @@
 #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 +51,16 @@ 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);
+  int result = base::WriteFile(full_path, content.data(), content.size());
+  return (static_cast<size_t>(result) == content.size());

[Devlin, 2017/03/24 18:50:09]

nit: wrapping parens unnecessary

[lazyboy, 2017/03/24 19:22:54]

Done.

+}
+
 scoped_refptr<Extension> CreateTestExtension(const std::string& name,
                                              bool incognito_split_mode) {
   base::DictionaryValue manifest;
@@ -98,24 +116,104 @@ scoped_refptr<Extension> CreateTestResponseHeaderExtension() {
   return extension;
 }
 
+// A ContentVerifyJob::TestDelegate that observes DoneReading().
+class JobDelegate : public ContentVerifyJob::TestDelegate {
+ public:
+  explicit JobDelegate(const std::string& expected_contents)
+      : expected_contents_(expected_contents) {}
+  ~JobDelegate() override {}
+
+  ContentVerifyJob::FailureReason BytesRead(const ExtensionId& id,
+                                            int count,
+                                            const char* data) override {
+    read_contents_.append(data, count);
+    return ContentVerifyJob::NONE;
+  }
+
+  ContentVerifyJob::FailureReason DoneReading(const ExtensionId& id) override {
+    seen_done_reading_extension_ids_.insert(id);
+    if (waiting_for_extension_id_ == id)
+      run_loop_.Quit();
+
+    if (!base::StartsWith(expected_contents_, read_contents_,
+                          base::CompareCase::SENSITIVE)) {
+      ADD_FAILURE() << "Unexpected read, expected: " << expected_contents_
+                    << ", but found: " << read_contents_;
+    }
+    return ContentVerifyJob::NONE;
+  }
+
+  void WaitForDoneReading(const ExtensionId& id) {
+    ASSERT_FALSE(waiting_for_extension_id_);
+    if (seen_done_reading_extension_ids_.count(id))

[Devlin, 2017/03/24 18:50:09]

This is actually unnecessary, since RunLoop::Run() will quit immediately if
Quit() has already been called.  But it doesn't hurt, so up to you.

[lazyboy, 2017/03/24 19:22:53]

Assigning waiting_for_extension_id_ in that case seems a bit confusing to me.
Going to keep this one.

+      return;
+    waiting_for_extension_id_ = id;
+    run_loop_.Run();
+  }
+
+  void Reset() {

[Devlin, 2017/03/24 18:50:09]

And since Quit() can have preemptive effects, we need to reset the run loop
here, too (sadly, there's no RunLoop::Reset, so I think we just need to create a
new one here.)

Alternatively, you could design this to be a single-use class, which is also
okay.

[lazyboy, 2017/03/24 19:22:52]

Added new RunLoop here. Done.

+    read_contents_.clear();
+    waiting_for_extension_id_.reset();
+    seen_done_reading_extension_ids_.clear();
+  }
+
+ private:
+  std::string expected_contents_;
+  std::string read_contents_;
+  std::set<ExtensionId> seen_done_reading_extension_ids_;
+  base::Optional<ExtensionId> waiting_for_extension_id_;
+  base::RunLoop run_loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(JobDelegate);
+};
+
+class ScopedVerifyJobDelegateOverride {

[Devlin, 2017/03/24 18:50:09]

Optional: is there a reason to not just have JobDelegate handle scoping itself?

[lazyboy, 2017/03/24 19:22:54]

Ah thanks, fixed.

+ public:
+  explicit ScopedVerifyJobDelegateOverride(const std::string& expected_content)
+      : delegate_(expected_content) {
+    ContentVerifyJob::SetDelegateForTests(&delegate_);
+  }
+  ~ScopedVerifyJobDelegateOverride() {
+    ContentVerifyJob::SetDelegateForTests(nullptr);
+  }
+
+  JobDelegate* delegate() { return &delegate_; }
+
+ private:
+  JobDelegate delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedVerifyJobDelegateOverride);
+};
+
 }  // 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_ = TestingProfile::Builder().Build();
     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 +273,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 +364,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 +401,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 +439,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 +486,7 @@ TEST_F(ExtensionProtocolTest, AllowFrameRequests) {
   }
 }
 
-
-TEST_F(ExtensionProtocolTest, MetadataFolder) {
+TEST_F(ExtensionProtocolsTest, MetadataFolder) {
   SetProtocolHandler(false);
 
   base::FilePath extension_dir = GetTestPath("metadata_folder");
@@ -414,4 +513,59 @@ TEST_F(ExtensionProtocolTest, MetadataFolder) {
             DoRequest(*extension, relative_path.AsUTF8Unsafe()));
 }
 
+// Tests that unreadable files and deleted files correctly go through
+// ContentVerifyJob.
+TEST_F(ExtensionProtocolsTest, VerificationSeenForFileAccessErrors) {
+  const char kFooJsContents[] = "hello world.";
+  ScopedVerifyJobDelegateOverride scoped_override(kFooJsContents);
+  JobDelegate* test_job_delegate = scoped_override.delegate();
+  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, kFooJsContents))
+      << "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());

[Devlin, 2017/03/24 18:50:09]

nit: may as well ASSERT here; we'll crash below anyways.

[lazyboy, 2017/03/24 19:22:52]

Done.

+  content_verifier_->OnExtensionLoaded(testing_profile_.get(), extension.get());
+  // Wait for PostTask to ContentVerifierIOData::AddData() to finish.
+  content::RunAllPendingInMessageLoop();
+
+  // Valid and readable foo.js.
+  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