Unescape FTP URL paths, Firefox-compatible.

TEST=See bug.
BUG=20304

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

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

3 files changed, 55 insertions, 30 deletions

diff --git a/net/ftp/ftp_network_transaction.cc b/net/ftp/ftp_network_transaction.cc
index e1a4114caf..5a90885 100644
--- a/net/ftp/ftp_network_transaction.cc
+++ b/net/ftp/ftp_network_transaction.cc
@@ -7,6 +7,7 @@
 #include "base/compiler_specific.h"
 #include "base/string_util.h"
 #include "net/base/connection_type_histograms.h"
+#include "net/base/escape.h"
 #include "net/base/load_log.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_util.h"
@@ -28,9 +29,9 @@ namespace {
 
 // Returns true if |input| can be safely used as a part of FTP command.
 bool IsValidFTPCommandString(const std::string& input) {
-  // RFC 959 only allows ASCII strings.
-  if (!IsStringASCII(input))
-    return false;
+  // RFC 959 only allows ASCII strings, but at least Firefox can send non-ASCII
+  // characters in the command if the request path contains them. To be
+  // compatible, we do the same and allow non-ASCII characters in a command.
 
   // Protect agains newline injection attack.
   if (input.find_first_of("\r\n") != std::string::npos)
@@ -322,6 +323,17 @@ void FtpNetworkTransaction::OnIOComplete(int result) {
     DoCallback(rv);
 }
 
+std::string FtpNetworkTransaction::GetRequestPathForFtpCommand() const {
+  std::string path = (request_->url.has_path() ? request_->url.path() : "/");
+  UnescapeRule::Type unescape_rules = UnescapeRule::SPACES |
+                                      UnescapeRule::URL_SPECIAL_CHARS;
+  // This may unescape to non-ASCII characters, but we allow that. See the
+  // comment for IsValidFTPCommandString.
+  path = UnescapeURLComponent(path, unescape_rules);
+  DCHECK(IsValidFTPCommandString(path));
+  return path;
+}
+
 int FtpNetworkTransaction::DoLoop(int result) {
   DCHECK(next_state_ != STATE_NONE);
 
@@ -788,11 +800,7 @@ int FtpNetworkTransaction::ProcessResponsePASV(
 
 // SIZE command
 int FtpNetworkTransaction::DoCtrlWriteSIZE() {
-  std::string command = "SIZE";
-  if (request_->url.has_path()) {
-    command.append(" ");
-    command.append(request_->url.path());
-  }
+  std::string command = "SIZE " + GetRequestPathForFtpCommand();
   next_state_ = STATE_CTRL_READ;
   return SendFtpCommand(command, COMMAND_SIZE);
 }
@@ -826,13 +834,7 @@ int FtpNetworkTransaction::ProcessResponseSIZE(
 
 // RETR command
 int FtpNetworkTransaction::DoCtrlWriteRETR() {
-  std::string command = "RETR";
-  if (request_->url.has_path()) {
-    command.append(" ");
-    command.append(request_->url.path());
-  } else {
-    command.append(" /");
-  }
+  std::string command = "RETR " + GetRequestPathForFtpCommand();
   next_state_ = STATE_CTRL_READ;
   return SendFtpCommand(command, COMMAND_RETR);
 }
@@ -879,13 +881,7 @@ int FtpNetworkTransaction::ProcessResponseRETR(
 
 // MDMT command
 int FtpNetworkTransaction::DoCtrlWriteMDTM() {
-  std::string command = "MDTM";
-  if (request_->url.has_path()) {
-    command.append(" ");
-    command.append(request_->url.path());
-  } else {
-    command.append(" /");
-  }
+  std::string command = "MDTM " + GetRequestPathForFtpCommand();
   next_state_ = STATE_CTRL_READ;
   return SendFtpCommand(command, COMMAND_MDTM);
 }
@@ -915,13 +911,7 @@ int FtpNetworkTransaction::ProcessResponseMDTM(
 
 // CWD command
 int FtpNetworkTransaction::DoCtrlWriteCWD() {
-  std::string command = "CWD";
-  if (request_->url.has_path()) {
-    command.append(" ");
-    command.append(request_->url.path());
-  } else {
-    command.append(" /");
-  }
+  std::string command = "CWD " + GetRequestPathForFtpCommand();
   next_state_ = STATE_CTRL_READ;
   return SendFtpCommand(command, COMMAND_CWD);
 }
diff --git a/net/ftp/ftp_network_transaction.h b/net/ftp/ftp_network_transaction.h
index ee9c141..3366c71 100644
--- a/net/ftp/ftp_network_transaction.h
+++ b/net/ftp/ftp_network_transaction.h
@@ -101,6 +101,9 @@ class FtpNetworkTransaction : public FtpTransaction {
   // code to be in range 100-599.
   static ErrorClass GetErrorClass(int response_code);
 
+  // Returns request path suitable to be included in an FTP command.
+  std::string GetRequestPathForFtpCommand() const;
+
   // Runs the state transition loop.
   int DoLoop(int result);
 
diff --git a/net/ftp/ftp_network_transaction_unittest.cc b/net/ftp/ftp_network_transaction_unittest.cc
index 140a37d..54b0501 100644
--- a/net/ftp/ftp_network_transaction_unittest.cc
+++ b/net/ftp/ftp_network_transaction_unittest.cc
@@ -202,7 +202,6 @@ class FtpMockControlSocketFileDownload : public FtpMockControlSocket {
         return Verify("MDTM /file\r\n", data, PRE_RETR,
                       "213 20070221112533\r\n");
       case PRE_RETR:
-        // TODO(phajdan.jr): Also test with "150 Accepted Data Connection".
         return Verify("RETR /file\r\n", data, PRE_QUIT, "200 OK\r\n");
       default:
         return FtpMockControlSocket::OnWrite(data);
@@ -213,6 +212,33 @@ class FtpMockControlSocketFileDownload : public FtpMockControlSocket {
   DISALLOW_COPY_AND_ASSIGN(FtpMockControlSocketFileDownload);
 };
 
+class FtpMockControlSocketEscaping : public FtpMockControlSocket {
+ public:
+  FtpMockControlSocketEscaping() {
+  }
+
+  virtual MockWriteResult OnWrite(const std::string& data) {
+    if (InjectFault())
+      return MockWriteResult(true, data.length());
+    switch (state()) {
+      case PRE_SIZE:
+        return Verify("SIZE / !\"#$%y\200\201\r\n", data, PRE_MDTM,
+                      "213 18\r\n");
+      case PRE_MDTM:
+        return Verify("MDTM / !\"#$%y\200\201\r\n", data, PRE_RETR,
+                      "213 20070221112533\r\n");
+      case PRE_RETR:
+        return Verify("RETR / !\"#$%y\200\201\r\n", data, PRE_QUIT,
+                      "200 OK\r\n");
+      default:
+        return FtpMockControlSocket::OnWrite(data);
+    }
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FtpMockControlSocketEscaping);
+};
+
 class FtpMockControlSocketFileDownloadAcceptedDataConnection
     : public FtpMockControlSocketFileDownload {
  public:
@@ -683,6 +709,12 @@ TEST_F(FtpNetworkTransactionTest, EvilRestartPassword) {
   EXPECT_EQ(ERR_MALFORMED_IDENTITY, callback_.WaitForResult());
 }
 
+TEST_F(FtpNetworkTransactionTest, Escaping) {
+  FtpMockControlSocketEscaping ctrl_socket;
+  ExecuteTransaction(&ctrl_socket, "ftp://host/%20%21%22%23%24%25%79%80%81",
+                     OK);
+}
+
 TEST_F(FtpNetworkTransactionTest, DirectoryTransactionFailUser) {
   FtpMockControlSocketDirectoryListing ctrl_socket;
   TransactionFailHelper(&ctrl_socket,