FTP: make file downloads work when directory listing is restricted

BUG=56734
TEST=see bug

Review URL: http://codereview.chromium.org/3775005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@62714 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 65 insertions, 34 deletions

diff --git a/net/ftp/ftp_network_transaction.cc b/net/ftp/ftp_network_transaction.cc
index 7197ae9b..02e4be0 100644
--- a/net/ftp/ftp_network_transaction.cc
+++ b/net/ftp/ftp_network_transaction.cc
@@ -978,6 +978,7 @@ int FtpNetworkTransaction::ProcessResponseSIZE(
       if (size < 0)
         return Stop(ERR_INVALID_RESPONSE);
       response_.expected_content_size = size;
+      resource_type_ = RESOURCE_TYPE_FILE;
       break;
     case ERROR_CLASS_INFO_NEEDED:
       break;
@@ -985,10 +986,8 @@ int FtpNetworkTransaction::ProcessResponseSIZE(
       break;
     case ERROR_CLASS_PERMANENT_ERROR:
       // It's possible that SIZE failed because the path is a directory.
-      if (response.status_code == 550 &&
-          resource_type_ == RESOURCE_TYPE_UNKNOWN) {
-        resource_type_ = RESOURCE_TYPE_DIRECTORY;
-      } else if (resource_type_ != RESOURCE_TYPE_DIRECTORY) {
+      if (resource_type_ == RESOURCE_TYPE_UNKNOWN &&
+          response.status_code != 550) {
         return Stop(GetNetErrorCodeForFtpResponseCode(response.status_code));
       }
       break;
@@ -997,10 +996,10 @@ int FtpNetworkTransaction::ProcessResponseSIZE(
       return Stop(ERR_UNEXPECTED);
   }
 
-  if (resource_type_ == RESOURCE_TYPE_DIRECTORY)
-    next_state_ = STATE_CTRL_WRITE_CWD;
-  else
+  if (resource_type_ == RESOURCE_TYPE_FILE)
     next_state_ = STATE_CTRL_WRITE_RETR;
+  else
+    next_state_ = STATE_CTRL_WRITE_CWD;
 
   return OK;
 }
@@ -1020,8 +1019,10 @@ int FtpNetworkTransaction::ProcessResponseRETR(
       // It got here either through Start or RestartWithAuth. We want that
       // method to complete. Not setting next state here will make DoLoop exit
       // and in turn make Start/RestartWithAuth complete.
+      resource_type_ = RESOURCE_TYPE_FILE;
       break;
     case ERROR_CLASS_OK:
+      resource_type_ = RESOURCE_TYPE_FILE;
       next_state_ = STATE_CTRL_WRITE_QUIT;
       break;
     case ERROR_CLASS_INFO_NEEDED:
@@ -1031,7 +1032,7 @@ int FtpNetworkTransaction::ProcessResponseRETR(
     case ERROR_CLASS_PERMANENT_ERROR:
       // Code 550 means "Failed to open file". Other codes are unrelated,
       // like "Not logged in" etc.
-      if (response.status_code != 550)
+      if (response.status_code != 550 || resource_type_ == RESOURCE_TYPE_FILE)
         return Stop(GetNetErrorCodeForFtpResponseCode(response.status_code));
 
       // It's possible that RETR failed because the path is a directory.
@@ -1046,6 +1047,11 @@ int FtpNetworkTransaction::ProcessResponseRETR(
       NOTREACHED();
       return Stop(ERR_UNEXPECTED);
   }
+
+  // We should be sure about our resource type now. Otherwise we risk
+  // an infinite loop (RETR can later send CWD, and CWD can later send RETR).
+  DCHECK_NE(RESOURCE_TYPE_UNKNOWN, resource_type_);
+
   return OK;
 }
 
@@ -1057,6 +1063,9 @@ int FtpNetworkTransaction::DoCtrlWriteCWD() {
 }
 
 int FtpNetworkTransaction::ProcessResponseCWD(const FtpCtrlResponse& response) {
+  // We should never issue CWD if we know the target resource is a file.
+  DCHECK_NE(RESOURCE_TYPE_FILE, resource_type_);
+
   switch (GetErrorClass(response.status_code)) {
     case ERROR_CLASS_INITIATED:
       return Stop(ERR_INVALID_RESPONSE);
@@ -1068,13 +1077,22 @@ int FtpNetworkTransaction::ProcessResponseCWD(const FtpCtrlResponse& response) {
     case ERROR_CLASS_TRANSIENT_ERROR:
       return Stop(GetNetErrorCodeForFtpResponseCode(response.status_code));
     case ERROR_CLASS_PERMANENT_ERROR:
-      if (resource_type_ == RESOURCE_TYPE_DIRECTORY &&
-          response.status_code == 550) {
-        // We're assuming that the resource is a directory, but the server says
-        // it's not true. The most probable interpretation is that it doesn't
-        // exist (with FTP we can't be sure).
-        return Stop(ERR_FILE_NOT_FOUND);
+      if (response.status_code == 550) {
+        if (resource_type_ == RESOURCE_TYPE_DIRECTORY) {
+          // We're assuming that the resource is a directory, but the server
+          // says it's not true. The most probable interpretation is that it
+          // doesn't exist (with FTP we can't be sure).
+          return Stop(ERR_FILE_NOT_FOUND);
+        }
+
+        // We are here because SIZE failed and we are not sure what the resource
+        // type is. It could still be file, and SIZE could fail because of
+        // an access error (http://crbug.com/56734). Try RETR just to be sure.
+        resource_type_ = RESOURCE_TYPE_FILE;
+        next_state_ = STATE_CTRL_WRITE_RETR;
+        return OK;
       }
+
       return Stop(GetNetErrorCodeForFtpResponseCode(response.status_code));
     default:
       NOTREACHED();
@@ -1182,8 +1200,9 @@ int FtpNetworkTransaction::DoDataConnect() {
 }
 
 int FtpNetworkTransaction::DoDataConnectComplete(int result) {
-  if (result == ERR_CONNECTION_TIMED_OUT && use_epsv_) {
-    // It's possible we hit a broken server, sadly. Fall back to PASV.
+  if (result != OK && use_epsv_) {
+    // It's possible we hit a broken server, sadly. They can break in different
+    // ways. Some time out, some reset a connection. Fall back to PASV.
     // TODO(phajdan.jr): remember it for future transactions with this server.
     // TODO(phajdan.jr): write a test for this code path.
     use_epsv_ = false;
diff --git a/net/ftp/ftp_network_transaction_unittest.cc b/net/ftp/ftp_network_transaction_unittest.cc
index 01eff90..b504ae8 100644
--- a/net/ftp/ftp_network_transaction_unittest.cc
+++ b/net/ftp/ftp_network_transaction_unittest.cc
@@ -333,6 +333,33 @@ class FtpSocketDataProviderFileDownload : public FtpSocketDataProvider {
   DISALLOW_COPY_AND_ASSIGN(FtpSocketDataProviderFileDownload);
 };
 
+class FtpSocketDataProviderFileNotFound : public FtpSocketDataProvider {
+ public:
+  FtpSocketDataProviderFileNotFound() {
+  }
+
+  virtual MockWriteResult OnWrite(const std::string& data) {
+    if (InjectFault())
+      return MockWriteResult(true, data.length());
+    switch (state()) {
+      case PRE_SIZE:
+        return Verify("SIZE /file\r\n", data, PRE_CWD,
+                      "550 File Not Found\r\n");
+      case PRE_CWD:
+        return Verify("CWD /file\r\n", data, PRE_RETR,
+                      "550 File Not Found\r\n");
+      case PRE_RETR:
+        return Verify("RETR /file\r\n", data, PRE_QUIT,
+                      "550 File Not Found\r\n");
+      default:
+        return FtpSocketDataProvider::OnWrite(data);
+    }
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FtpSocketDataProviderFileNotFound);
+};
+
 class FtpSocketDataProviderFileDownloadWithPasvFallback
     : public FtpSocketDataProviderFileDownload {
  public:
@@ -1212,16 +1239,6 @@ TEST_F(FtpNetworkTransactionTest, DirectoryTransactionFailCwd) {
                         ERR_FTP_FAILED);
 }
 
-TEST_F(FtpNetworkTransactionTest, DirectoryTransactionFileNotFound) {
-  FtpSocketDataProviderDirectoryListing ctrl_socket;
-  TransactionFailHelper(&ctrl_socket,
-                        "ftp://host",
-                        FtpSocketDataProvider::PRE_CWD,
-                        FtpSocketDataProvider::PRE_QUIT,
-                        "550 cannot open file\r\n",
-                        ERR_FILE_NOT_FOUND);
-}
-
 TEST_F(FtpNetworkTransactionTest, DirectoryTransactionFailMlsd) {
   FtpSocketDataProviderDirectoryListing ctrl_socket;
   TransactionFailHelper(&ctrl_socket,
@@ -1326,14 +1343,9 @@ TEST_F(FtpNetworkTransactionTest, DownloadTransactionFailRetr) {
                         ERR_FTP_FAILED);
 }
 
-TEST_F(FtpNetworkTransactionTest, DownloadTransactionFileNotFound) {
-  FtpSocketDataProviderFileDownload ctrl_socket;
-  TransactionFailHelper(&ctrl_socket,
-                        "ftp://host/file;type=i",
-                        FtpSocketDataProvider::PRE_SIZE,
-                        FtpSocketDataProvider::PRE_QUIT,
-                        "550 File Not Found\r\n",
-                        ERR_FTP_FAILED);
+TEST_F(FtpNetworkTransactionTest, FileNotFound) {
+  FtpSocketDataProviderFileNotFound ctrl_socket;
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", ERR_FTP_FAILED);
 }
 
 // Test for http://crbug.com/38845.