FTP: make the code handling server messages in directory listings more general.

net/ftp/ftp_directory_listing_parser_ls.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/ftp/ftp_directory_listing_parser_ls.h"
 
 #include <vector>
 
 #include "base/string_number_conversions.h"
 #include "base/string_split.h"
@@ skipping 33 matching lines @@
   // separate this column from the next column (number of links), resulting
   // in additional characters at the end. Also, sometimes there is a "+"
   // sign at the end indicating the file has ACLs set.
 
   // In fact, we don't even expect three "rwx" triplets of permission
   // listing, as some FTP servers like Hylafax only send two.
   return (LooksLikeUnixPermission(text.substr(1, 3)) &&
           LooksLikeUnixPermission(text.substr(4, 3)));
 }
 
-bool LooksLikePermissionDeniedError(const string16& text) {
-  // Try to recognize a three-part colon-separated error message:
-  //
-  //   1. ftpd server name
-  //   2. directory name (often just ".")
-  //   3. message text (usually "Permission denied")
-  std::vector<string16> parts;
-  base::SplitString(CollapseWhitespace(text, false), ':', &parts);
-
-  if (parts.size() != 3)
-    return false;
-
-  return parts[2].find(ASCIIToUTF16("Permission denied")) != string16::npos;
-}
-
 // Returns the column index of the end of the date listing and detected
 // last modification time.
 bool DetectColumnOffsetAndModificationTime(const std::vector<string16>& columns,
                                            const base::Time& current_time,
                                            size_t* offset,
                                            base::Time* modification_time) {
   // The column offset can be arbitrarily large if some fields
   // like owner or group name contain spaces. Try offsets from left to right
   // and use the first one that matches a date listing.
   //
@@ skipping 73 matching lines @@
       continue;
     }
 
     FtpDirectoryListingEntry entry;
 
     size_t column_offset;
     if (!DetectColumnOffsetAndModificationTime(columns,
                                                current_time,
                                                &column_offset,
                                                &entry.last_modified)) {
-      // If we can't recognize a normal listing line, maybe it's an error?
-      // In that case, just ignore the error, but still recognize the data
-      // as valid listing.
-      if (LooksLikePermissionDeniedError(lines[i]))
+      // Some servers send a message in on of the first few lines.

[eroman, 2012/04/13 18:37:26]

nit: "in on of the" --> "in one of the".

[Paweł Hajdan Jr., 2012/04/14 10:12:14]

Done.

+      // All those messages have in common is the string ".:",
+      // where "." means the current directory, and ":" separates it
+      // from the rest of the message, which may be empty.
+      if (lines[i].find(ASCIIToUTF16(".:")) != string16::npos)

[gavinp, 2012/04/12 18:03:14]

LGTM.  Initially I thought this was going to not allow files with :. in their
name, but it does.

My question though is, why do we not want to show a directory listing at all if
there's a single unparseable line in the response, but only if that line doesn't
contain ".:" ?

[Paweł Hajdan Jr., 2012/04/13 16:02:34]

This is parser of "ls -l" format. We have other formats too. In this case,
returning false is more like "this is not ls -l format, try parsing this as some
other format". See also ftp_directory_listing_parser.cc
If the line contains ".:", it is valid. To be more precise, there should be at
most one such line in the listing, and it should appear before any "parseable"
and meaningful lines containing actual listing entries. However, the past
experience with the parser shows that such strictness is not really worth it,
and would needlessly increase complexity of the parser. We're not doing
validation of the listing here, there is no spec anyway.

And the tests with whole listings (ftp_directory_listing_parser_unittest.cc)
should help detect cases in which the parser would confuse formats.

         continue;
 
       return false;
     }
 
     if (!LooksLikeUnixPermissionsListing(columns[0]))
       return false;
     if (columns[0][0] == 'l') {
       entry.type = FtpDirectoryListingEntry::SYMLINK;
     } else if (columns[0][0] == 'd') {
@@ skipping 42 matching lines @@
         entry.name = entry.name.substr(0, pos);
     }
 
     entries->push_back(entry);
   }
 
   return true;
 }
 
 }  // namespace net