chromeos: Check both original and absolute paths for file: scheme

net/url_request/url_request_unittest.cc

Index: net/url_request/url_request_unittest.cc
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index 2d158a0492764e8196d54bd59c0bd6f0be81e459..0e2ada367106440cb3a6211a628326e6943fa315 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -835,6 +835,46 @@ class URLRequestTest : public PlatformTest {
   TestURLRequestContext default_context_;
 };
 
+// This NetworkDelegate is picky about what files are accessible. Only
+// whitelisted files are allowed.
+class CookieBlockingNetworkDelegate : public TestNetworkDelegate {
+ public:
+  CookieBlockingNetworkDelegate(){};
+
+  // Adds |directory| to the access white list.
+  void AddToWhitelist(const base::FilePath& directory) {
+    // TODO(satorux): Remove this. This is for debugging test failures on
+    // Android bots.
+    LOG(ERROR) << "@@ AddToWhitelist: " << directory.value();

[mmenke, 2017/05/24 19:25:51]

nit:  Remove logging.

[satorux1, 2017/05/25 01:51:07]

Done.

+    whitelist_.insert(directory);
+  }
+
+  // Returns true if |path| matches the white list.
+  bool OnCanAccessFileInternal(const base::FilePath& path) const {

[mmenke, 2017/05/24 19:25:50]

nit:  Private?  Doesn't really matter, except that a public *Internal method
seems a bit weird.

[satorux1, 2017/05/25 01:51:07]

Done.

+    for (const auto& directory : whitelist_) {
+      if (directory == path || directory.IsParent(path))
+        return true;
+    }
+    return false;
+  }
+
+  // Returns true only if both |original_path| and |absolute_path| match the
+  // white list.
+  bool OnCanAccessFile(const URLRequest& request,
+                       const base::FilePath& original_path,
+                       const base::FilePath& absolute_path) const override {
+    LOG(ERROR) << "@@ original_path: " << original_path.value();
+    LOG(ERROR) << "@@ absolute_path: " << absolute_path.value();

[mmenke, 2017/05/24 19:25:50]

nit:  Remove logging.

[satorux1, 2017/05/25 01:51:07]

Done.

+    return (OnCanAccessFileInternal(original_path) &&
+            OnCanAccessFileInternal(absolute_path));
+  }
+
+ private:
+  std::set<base::FilePath> whitelist_;
+
+  DISALLOW_COPY_AND_ASSIGN(CookieBlockingNetworkDelegate);
+};
+
 TEST_F(URLRequestTest, AboutBlankTest) {
   TestDelegate d;
   {
@@ -1083,39 +1123,183 @@ TEST_F(URLRequestTest, FileTestMultipleRanges) {
 TEST_F(URLRequestTest, AllowFileURLs) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  LOG(ERROR) << "@@ temp_dir: " << temp_dir.GetPath().value();

[mmenke, 2017/05/24 19:25:50]

nit:  Remove logging (Not going to call out the others)

[satorux1, 2017/05/25 01:51:07]

Done.

+  // Get an absolute path since temp_dir can contain a symbolic link. As of
+  // now, Mac and Android bots return a path with a symbolic link.
+  base::FilePath absolute_temp_dir =
+      base::MakeAbsoluteFilePath(temp_dir.GetPath());
+
   base::FilePath test_file;
-  ASSERT_TRUE(base::CreateTemporaryFileInDir(temp_dir.GetPath(), &test_file));
+  ASSERT_TRUE(base::CreateTemporaryFileInDir(absolute_temp_dir, &test_file));
+  LOG(ERROR) << "@@ test_file: " << test_file.value();
+  // The directory part of the path returned from CreateTemporaryFileInDir()
+  // can be slightly different from temp_dir on Windows.

[mmenke, 2017/05/24 19:25:50]

nit:  |absolute_temp_dir|?  (|| around local names is the convention in net/,
and match variable name)

[satorux1, 2017/05/25 01:51:07]

Done.

+  // Example: C:\\Users\\CHROME~2 -> C:\\Users\\chrome-bot
+  // Hence the test should use the directory name of test_file, rather than
+  // temp_dir, for whitelisting.
+  base::FilePath real_temp_dir = test_file.DirName();
+  LOG(ERROR) << "@@ real_temp_dir: " << real_temp_dir.value();
   std::string test_data("monkey");
   base::WriteFile(test_file, test_data.data(), test_data.size());
   GURL test_file_url = FilePathToFileURL(test_file);
-
   {
     TestDelegate d;
-    TestNetworkDelegate network_delegate;
-    network_delegate.set_can_access_files(true);
+    CookieBlockingNetworkDelegate network_delegate;
+    network_delegate.AddToWhitelist(real_temp_dir);
     default_context_.set_network_delegate(&network_delegate);
     std::unique_ptr<URLRequest> r(default_context_.CreateRequest(
         test_file_url, DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS));
     r->Start();
     base::RunLoop().Run();
+    // This should be allowed as the file path is white listed.

[mmenke, 2017/05/24 19:25:51]

nit:  You're using whitelisting above, white listed here.  I believe it's
actually one word.

[satorux1, 2017/05/25 01:51:07]

Done.

     EXPECT_FALSE(d.request_failed());
     EXPECT_EQ(test_data, d.data_received());
   }
 
   {
     TestDelegate d;
-    TestNetworkDelegate network_delegate;
-    network_delegate.set_can_access_files(false);
+    CookieBlockingNetworkDelegate network_delegate;
     default_context_.set_network_delegate(&network_delegate);
     std::unique_ptr<URLRequest> r(default_context_.CreateRequest(
         test_file_url, DEFAULT_PRIORITY, &d, TRAFFIC_ANNOTATION_FOR_TESTS));
     r->Start();
     base::RunLoop().Run();
+    // This should be rejected as the file path is not white listed.

[mmenke, 2017/05/24 19:25:50]

whitelisted

[satorux1, 2017/05/25 01:51:07]

Done.

     EXPECT_TRUE(d.request_failed());
     EXPECT_EQ("", d.data_received());
+    EXPECT_EQ(ERR_ACCESS_DENIED, d.request_status());
   }
 }
 
+#if defined(OS_POSIX)  // Bacause of symbolic links.
+
+TEST_F(URLRequestTest, SymlinksToFiles) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  // Get an absolute path since temp_dir can contain a symbolic link.
+  base::FilePath absolute_temp_dir =
+      base::MakeAbsoluteFilePath(temp_dir.GetPath());
+
+  // Create a good directory (will be white listed) and a good file.

[mmenke, 2017/05/24 19:25:51]

whitelisted

[satorux1, 2017/05/25 01:51:07]

Done.

+  base::FilePath good_dir = absolute_temp_dir.AppendASCII("good");
+  ASSERT_TRUE(base::CreateDirectory(good_dir));
+  base::FilePath good_file;
+  ASSERT_TRUE(base::CreateTemporaryFileInDir(good_dir, &good_file));
+  std::string good_data("good");
+  base::WriteFile(good_file, good_data.data(), good_data.size());
+  // See the comment in AllowFileURLs() for why doing this.

[mmenke, 2017/05/24 19:25:50]

why doing this -> why this is done (Or why this is needed)

[satorux1, 2017/05/25 01:51:07]

Done.

+  base::FilePath real_good_dir = good_file.DirName();
+
+  // Create a bad directory (will not be white listed) and a bad file.

[mmenke, 2017/05/24 19:25:50]

whitelisted

[satorux1, 2017/05/25 01:51:06]

Done.

+  base::FilePath bad_dir = absolute_temp_dir.AppendASCII("bad");
+  ASSERT_TRUE(base::CreateDirectory(bad_dir));
+  base::FilePath bad_file;
+  ASSERT_TRUE(base::CreateTemporaryFileInDir(bad_dir, &bad_file));
+  std::string bad_data("bad");
+  base::WriteFile(bad_file, bad_data.data(), bad_data.size());
+
+  // This symlink will point to the good file. Access to the symlink will be
+  // allowed as both the symlink and the destination file are in the same
+  // good directory.
+  base::FilePath good_symlink = good_dir.AppendASCII("good_symlink");
+  ASSERT_TRUE(base::CreateSymbolicLink(good_file, good_symlink));
+  GURL good_file_url = FilePathToFileURL(good_symlink);
+  // This symlink will point to the bad file. Even though the symlink is in
+  // the good directory, access to the symlink will be rejected since it
+  // points to the bad file.
+  base::FilePath bad_symlink = good_dir.AppendASCII("bad_symlink");
+  ASSERT_TRUE(base::CreateSymbolicLink(bad_file, bad_symlink));
+  GURL bad_file_url = FilePathToFileURL(bad_symlink);
+
+  {
+    TestDelegate d;
+    CookieBlockingNetworkDelegate network_delegate;
+    network_delegate.AddToWhitelist(real_good_dir);
+    default_context_.set_network_delegate(&network_delegate);

[mmenke, 2017/05/24 19:25:50]

Optional:  Can move this network delegate outside of these two blocks, to make
it clearer it's shared.

[satorux1, 2017/05/25 01:51:06]

Good idea. Done!

+    std::unique_ptr<URLRequest> r(
+        default_context_.CreateRequest(good_file_url, DEFAULT_PRIORITY, &d));
+    r->Start();
+    base::RunLoop().Run();
+    // good_file_url should be allowed.
+    EXPECT_FALSE(d.request_failed());
+    EXPECT_EQ(good_data, d.data_received());
+  }
+
+  {
+    TestDelegate d;
+    CookieBlockingNetworkDelegate network_delegate;
+    network_delegate.AddToWhitelist(real_good_dir);
+    default_context_.set_network_delegate(&network_delegate);
+    std::unique_ptr<URLRequest> r(
+        default_context_.CreateRequest(bad_file_url, DEFAULT_PRIORITY, &d));
+    r->Start();
+    base::RunLoop().Run();
+    // bad_file_url should be rejected.
+    EXPECT_TRUE(d.request_failed());
+    EXPECT_EQ("", d.data_received());
+    EXPECT_EQ(ERR_ACCESS_DENIED, d.request_status());
+  }
+}
+
+TEST_F(URLRequestTest, SymlinksToDirs) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  // Get an absolute path since temp_dir can contain a symbolic link.
+  base::FilePath absolute_temp_dir =
+      base::MakeAbsoluteFilePath(temp_dir.GetPath());
+
+  // Create a good directory (will be white listed).

[mmenke, 2017/05/24 19:25:50]

whitelisted

[satorux1, 2017/05/25 01:51:06]

Done.

+  base::FilePath good_dir = absolute_temp_dir.AppendASCII("good");
+  ASSERT_TRUE(base::CreateDirectory(good_dir));
+
+  // Create a bad directory (will not be white listed).

[mmenke, 2017/05/24 19:25:50]

whitelisted

[satorux1, 2017/05/25 01:51:07]

Done.

+  base::FilePath bad_dir = absolute_temp_dir.AppendASCII("bad");
+  ASSERT_TRUE(base::CreateDirectory(bad_dir));
+
+  // This symlink will point to the good directory. Access to the symlink
+  // will be allowed as the symlink is in the good dir that'll be white
+  // listed.
+  base::FilePath good_symlink = good_dir.AppendASCII("good_symlink");
+  ASSERT_TRUE(base::CreateSymbolicLink(good_dir, good_symlink));
+  GURL good_file_url = FilePathToFileURL(good_symlink);
+  // This symlink will point to the bad directory. Even though the symlink is
+  // in the good directory, access to the symlink will be rejected since it
+  // points to the bad directory.
+  base::FilePath bad_symlink = good_dir.AppendASCII("bad_symlink");
+  ASSERT_TRUE(base::CreateSymbolicLink(bad_dir, bad_symlink));
+  GURL bad_file_url = FilePathToFileURL(bad_symlink);
+
+  {
+    TestDelegate d;
+    CookieBlockingNetworkDelegate network_delegate;
+    network_delegate.AddToWhitelist(good_dir);
+    default_context_.set_network_delegate(&network_delegate);

[mmenke, 2017/05/24 19:25:50]

Again, can share these

[satorux1, 2017/05/25 01:51:07]

Done.

+    std::unique_ptr<URLRequest> r(
+        default_context_.CreateRequest(good_file_url, DEFAULT_PRIORITY, &d));
+    r->Start();
+    base::RunLoop().Run();
+    // good_file_url should be allowed.
+    EXPECT_FALSE(d.request_failed());
+    ASSERT_NE(d.data_received().find("good_symlink"), std::string::npos);
+  }
+
+  {
+    TestDelegate d;
+    CookieBlockingNetworkDelegate network_delegate;
+    network_delegate.AddToWhitelist(good_dir);
+    default_context_.set_network_delegate(&network_delegate);
+    std::unique_ptr<URLRequest> r(
+        default_context_.CreateRequest(bad_file_url, DEFAULT_PRIORITY, &d));
+    r->Start();
+    base::RunLoop().Run();
+    // bad_file_url should be rejected.
+    EXPECT_TRUE(d.request_failed());
+    EXPECT_EQ("", d.data_received());
+    EXPECT_EQ(ERR_ACCESS_DENIED, d.request_status());
+  }
+}
+
+#endif  // defined(OS_POSIX)
 
 TEST_F(URLRequestTest, FileDirCancelTest) {
   // Put in mock resource provider.
@@ -1167,9 +1351,8 @@ TEST_F(URLRequestTest, FileDirOutputSanity) {
   EXPECT_GT(info.size, 0);
   std::string sentinel_output = GetDirectoryListingEntry(
       base::string16(sentinel_name, sentinel_name + strlen(sentinel_name)),
-      std::string(sentinel_name),
-      false /* is_dir */,
-      info.size,
+      std::string(sentinel_name), false /* is_dir */, info.size,
+
       info.last_modified);
 
   ASSERT_LT(0, d.bytes_received());