Download FTP resources, do not render them.

With the exception of generated directory listing pages, resources
requested via FTP will be downloaded rather than rendered.

Intent thread: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/eopgOoY1QLs
Spec: https://github.com/whatwg/fetch/pull/839

Bug: 744499
Change-Id: Ic04f17986cbceb511a19d541f4dd4c67685df81a
Reviewed-on: https://chromium-review.googlesource.com/c/1337338
Commit-Queue: Mike West <mkwst@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611125}

chrome/browser/chrome_navigation_browsertest.cc

@@ -845,9 +845,8 @@ class WillProcessResponseObserver : public content::WebContentsObserver {
 // submitted to load an iframe.
 // See https://crbug.com/757809.
 // Note: This test couldn't be a content_browsertests, since there would be
-// not handler defined for the "ftp" protocol in
+// no handler defined for the "ftp" protocol in
 // URLRequestJobFactoryImpl::protocol_handler_map_.
-// Flaky on Mac only.  http://crbug.com/816646
 IN_PROC_BROWSER_TEST_F(ChromeNavigationBrowserTest, BlockLegacySubresources) {
   net::SpawnedTestServer ftp_server(
       net::SpawnedTestServer::TYPE_FTP,
@@ -855,7 +854,6 @@ IN_PROC_BROWSER_TEST_F(ChromeNavigationBrowserTest, BlockLegacySubresources) {
   ASSERT_TRUE(ftp_server.Start());
 
   GURL main_url_http(embedded_test_server()->GetURL("/iframe.html"));
-  GURL main_url_ftp(ftp_server.GetURL("iframe.html"));
   GURL iframe_url_http(embedded_test_server()->GetURL("/simple.html"));
   GURL iframe_url_ftp(ftp_server.GetURL("simple.html"));
   GURL redirect_url(embedded_test_server()->GetURL("/server-redirect?"));
@@ -867,8 +865,6 @@ IN_PROC_BROWSER_TEST_F(ChromeNavigationBrowserTest, BlockLegacySubresources) {
   } kTestCases[] = {
       {main_url_http, iframe_url_http, true},
       {main_url_http, iframe_url_ftp, false},
-      {main_url_ftp, iframe_url_http, true},
-      {main_url_ftp, iframe_url_ftp, true},
   };
   for (const auto test_case : kTestCases) {
     // Blocking the request should work, even after a redirect.

chrome/browser/net/ftp_browsertest.cc

@@ -6,12 +6,15 @@
 #include "base/files/file_path.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
+#include "content/public/test/download_test_observer.h"
 #include "net/test/spawned_test_server/spawned_test_server.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
@@ -74,6 +77,11 @@ IN_PROC_BROWSER_TEST_F(FtpBrowserTest, DirectoryListingNavigation) {
   WaitForTitle(browser()->tab_strip_model()->GetActiveWebContents(),
                "Index of /dir1/");
 
+  // Navigate to file `test.html`, verify that it's downloaded.
+  content::DownloadTestObserverTerminal download_test_observer_terminal(
+      content::BrowserContext::GetDownloadManager(browser()->profile()), 1,
+      content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_IGNORE);
+
   EXPECT_TRUE(content::ExecuteScript(
       browser()->tab_strip_model()->GetActiveWebContents(),
       "(function() {"
@@ -91,6 +99,8 @@ IN_PROC_BROWSER_TEST_F(FtpBrowserTest, DirectoryListingNavigation) {
       "  }"
       "})()"));
 
-  WaitForTitle(browser()->tab_strip_model()->GetActiveWebContents(),
-               "PASS");
+  download_test_observer_terminal.WaitForFinished();
+  EXPECT_EQ(download_test_observer_terminal.NumDownloadsSeenInState(
+                download::DownloadItem::COMPLETE),
+            1u);
 }

chrome/browser/pdf/pdf_extension_test.cc

@@ -1104,17 +1104,6 @@ IN_PROC_BROWSER_TEST_F(PDFExtensionTest, NavigationOnCorrectTab) {
   EXPECT_FALSE(active_web_contents->GetController().GetPendingEntry());
 }
 
-IN_PROC_BROWSER_TEST_F(PDFExtensionTest, OpenFromFTP) {
-  net::SpawnedTestServer ftp_server(
-      net::SpawnedTestServer::TYPE_FTP,
-      base::FilePath(FILE_PATH_LITERAL("chrome/test/data/pdf")));
-  ASSERT_TRUE(ftp_server.Start());
-
-  GURL url(ftp_server.GetURL("/test.pdf"));
-  ASSERT_TRUE(LoadPdf(url));
-  EXPECT_EQ(base::ASCIIToUTF16("test.pdf"), GetActiveWebContents()->GetTitle());
-}
-
 // For both PDFExtensionTest and PDFIsolatedExtensionTest, MultipleDomains case
 // is flaky.
 // https://crbug.com/825038

net/base/mime_sniffer.cc

@@ -698,7 +698,6 @@ static bool SniffCRX(const char* content,
 bool ShouldSniffMimeType(const GURL& url, const std::string& mime_type) {
   bool sniffable_scheme = url.is_empty() ||
                           url.SchemeIsHTTPOrHTTPS() ||
-                          url.SchemeIs("ftp") ||
 #if defined(OS_ANDROID)
                           url.SchemeIs("content") ||
 #endif

net/base/mime_sniffer_unittest.cc

@@ -4,8 +4,10 @@
 
 #include "net/base/mime_sniffer.h"
 
+#include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
+#include "url/url_constants.h"
 
 namespace net {
 namespace {
@@ -31,6 +33,38 @@ static std::string SniffMimeType(const std::string& content,
   return mime_type;
 }
 
+TEST(MimeSnifferTest, SniffableSchemes) {
+  struct {
+    const char* scheme;
+    bool sniffable;
+  } kTestCases[] = {
+    {url::kAboutScheme, false},
+    {url::kBlobScheme, false},
+#if defined(OS_ANDROID)
+    {url::kContentScheme, true},
+#else
+    {url::kContentScheme, false},
+#endif
+    {url::kContentIDScheme, false},
+    {url::kDataScheme, false},
+    {url::kFileScheme, true},
+    {url::kFileSystemScheme, true},
+    {url::kFtpScheme, false},
+    {url::kGopherScheme, false},
+    {url::kHttpScheme, true},
+    {url::kHttpsScheme, true},
+    {url::kJavaScriptScheme, false},
+    {url::kMailToScheme, false},
+    {url::kWsScheme, false},
+    {url::kWssScheme, false}
+  };
+
+  for (const auto test_case : kTestCases) {
+    GURL url(std::string(test_case.scheme) + "://host/path/whatever");
+    EXPECT_EQ(test_case.sniffable, ShouldSniffMimeType(url, ""));
+  }
+}
+
 TEST(MimeSnifferTest, BoundaryConditionsTest) {
   std::string mime_type;
   std::string type_hint;

net/url_request/url_request_ftp_job.cc

@@ -72,12 +72,18 @@ bool URLRequestFtpJob::GetMimeType(std::string* mime_type) const {
       return true;
     }
   } else {
-    // No special handling of MIME type is needed. As opposed to direct FTP
-    // transaction, we do not get a raw directory listing to parse.
-    return http_transaction_->GetResponseInfo()->
-        headers->GetMimeType(mime_type);
+    std::string proxy_mime;
+    http_transaction_->GetResponseInfo()->headers->GetMimeType(&proxy_mime);
+    if (proxy_mime == "text/vnd.chromium.ftp-dir") {
+      *mime_type = "text/vnd.chromium.ftp-dir";
+      return true;
+    }
   }
-  return false;
+
+  // FTP resources other than directory listings ought to be handled as raw
+  // binary data, not sniffed into HTML or etc.
+  *mime_type = "application/octet-stream";
+  return true;
 }
 
 void URLRequestFtpJob::GetResponseInfo(HttpResponseInfo* info) {

net/url_request/url_request_ftp_job_unittest.cc

@@ -9,6 +9,7 @@
 
 #include "base/memory/ref_counted.h"
 #include "base/run_loop.h"
+#include "base/strings/strcat.h"
 #include "net/base/completion_once_callback.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/load_states.h"
@@ -279,6 +280,9 @@ class URLRequestFtpJobTest : public TestWithScopedTaskEnvironment {
 
   FtpTestURLRequestContext* request_context() { return &request_context_; }
   TestNetworkDelegate* network_delegate() { return &network_delegate_; }
+  std::vector<std::unique_ptr<SequencedSocketData>>* socket_data() {
+    return &socket_data_;
+  }
 
  private:
   std::vector<std::unique_ptr<SequencedSocketData>> socket_data_;
@@ -289,37 +293,69 @@ class URLRequestFtpJobTest : public TestWithScopedTaskEnvironment {
 };
 
 TEST_F(URLRequestFtpJobTest, FtpProxyRequest) {
-  MockWrite writes[] = {
-    MockWrite(ASYNC, 0, "GET ftp://ftp.example.com/ HTTP/1.1\r\n"
-              "Host: ftp.example.com\r\n"
-              "Proxy-Connection: keep-alive\r\n\r\n"),
-  };
-  MockRead reads[] = {
-    MockRead(ASYNC, 1, "HTTP/1.1 200 OK\r\n"),
-    MockRead(ASYNC, 2, "Content-Length: 9\r\n\r\n"),
-    MockRead(ASYNC, 3, "test.html"),
+  const struct {
+    const char* proxy_mime;
+    const char* expected_mime;
+  } kTestCases[] = {
+      {"text/vnd.chromium.ftp-dir", "text/vnd.chromium.ftp-dir"},
+      {"text/html", "application/octet-stream"},
+      {"text/plain", "application/octet-stream"},
+      {"image/png", "application/octet-stream"},
+      {"application/json", "application/octet-stream"},
+      {"", "application/octet-stream"},
   };
 
-  AddSocket(reads, writes);
-
-  TestDelegate request_delegate;
-  std::unique_ptr<URLRequest> url_request(request_context()->CreateRequest(
-      GURL("ftp://ftp.example.com/"), DEFAULT_PRIORITY, &request_delegate,
-      TRAFFIC_ANNOTATION_FOR_TESTS));
-  url_request->Start();
-  ASSERT_TRUE(url_request->is_pending());
-
-  // The TestDelegate will by default quit the message loop on completion.
-  base::RunLoop().Run();
-
-  EXPECT_THAT(request_delegate.request_status(), IsOk());
-  EXPECT_EQ(ProxyServer(ProxyServer::SCHEME_HTTP,
-                        HostPortPair::FromString("localhost:80")),
-            url_request->proxy_server());
-  EXPECT_EQ(1, network_delegate()->completed_requests());
-  EXPECT_EQ(0, network_delegate()->error_count());
-  EXPECT_FALSE(request_delegate.auth_required_called());
-  EXPECT_EQ("test.html", request_delegate.data_received());
+  int completed_requests = 0;
+  for (const auto test : kTestCases) {
+    SCOPED_TRACE(testing::Message()
+                 << std::endl
+                 << "Proxied MIME: " << test.proxy_mime << std::endl
+                 << "Expected MIME: " << test.expected_mime << std::endl);
+
+    std::string test_mime =
+        strlen(test.proxy_mime) == 0
+            ? "X-Placeholding: Header\r\n"
+            : base::StrCat({"Content-Type: ", test.proxy_mime, "\r\n"});
+
+    MockWrite writes[] = {
+        MockWrite(ASYNC, 0,
+                  "GET ftp://ftp.example.com/ HTTP/1.1\r\n"
+                  "Host: ftp.example.com\r\n"
+                  "Proxy-Connection: keep-alive\r\n\r\n"),
+    };
+    MockRead reads[] = {
+        MockRead(ASYNC, 1, "HTTP/1.1 200 OK\r\n"),
+        MockRead(ASYNC, 2, test_mime.c_str()),
+        MockRead(ASYNC, 3, "Content-Length: 9\r\n\r\n"),
+        MockRead(ASYNC, 4, "test.html"),
+    };
+
+    socket_data()->clear();
+    AddSocket(reads, writes);
+
+    TestDelegate request_delegate;
+    std::unique_ptr<URLRequest> url_request(request_context()->CreateRequest(
+        GURL("ftp://ftp.example.com/"), DEFAULT_PRIORITY, &request_delegate,
+        TRAFFIC_ANNOTATION_FOR_TESTS));
+    url_request->Start();
+    ASSERT_TRUE(url_request->is_pending());
+
+    // The TestDelegate will by default quit the message loop on completion.
+    base::RunLoop().Run();
+
+    EXPECT_THAT(request_delegate.request_status(), IsOk());
+    EXPECT_EQ(ProxyServer(ProxyServer::SCHEME_HTTP,
+                          HostPortPair::FromString("localhost:80")),
+              url_request->proxy_server());
+    EXPECT_EQ(++completed_requests, network_delegate()->completed_requests());
+    EXPECT_EQ(0, network_delegate()->error_count());
+    EXPECT_FALSE(request_delegate.auth_required_called());
+    EXPECT_EQ("test.html", request_delegate.data_received());
+
+    std::string mime;
+    url_request->GetMimeType(&mime);
+    EXPECT_EQ(test.expected_mime, mime);
+  }
 }
 
 // Regression test for http://crbug.com/237526.

net/url_request/url_request_unittest.cc

@@ -11222,6 +11222,34 @@ TEST_F(URLRequestTestFTP, FTPGetTestAnonymous) {
   }
 }
 
+TEST_F(URLRequestTestFTP, FTPMimeType) {
+  ASSERT_TRUE(ftp_test_server_.Start());
+
+  struct {
+    const char* path;
+    const char* mime;
+  } test_cases[] = {
+      {"/", "text/vnd.chromium.ftp-dir"},
+      {kFtpTestFile, "application/octet-stream"},
+  };
+
+  for (const auto test : test_cases) {
+    TestDelegate d;
+
+    std::unique_ptr<URLRequest> r(default_context().CreateRequest(
+        ftp_test_server_.GetURL(test.path), DEFAULT_PRIORITY, &d,
+        TRAFFIC_ANNOTATION_FOR_TESTS));
+    r->Start();
+    EXPECT_TRUE(r->is_pending());
+
+    d.RunUntilComplete();
+
+    std::string mime;
+    r->GetMimeType(&mime);
+    EXPECT_EQ(test.mime, mime);
+  }
+}
+
 TEST_F(URLRequestTestFTP, FTPGetTest) {
   ASSERT_TRUE(ftp_test_server_.Start());