Unescape FTP URL paths, Firefox-compatible.

Unescape FTP URL paths, Firefox-compatible.

TEST=See bug.
BUG=20304
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=26686

[eroman, 2009-09-16 21:11:37]

Please add unit-tests.

I suggest making a static method, and doing unit-tests via FRIEND_TEST.

http://codereview.chromium.org/200145/diff/1/2
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/200145/diff/1/2#newcode328
Line 328: path = UnescapeURLComponent(path, UnescapeRule::SPACES);
So unfortunately the FTP spec does not define what characters are valid in a
pathname (it leaves that up to particular server implementations, based on what
characters are considered valid for its filesystem).

As such, I don't believe the call to UnescapeURLComponent() above is liberal
enough in its unescaping.

In particular, I don't believe it will unescape the following characters:
    #?&=+ 

This seems like it will be another compatibility bug, since in particular "+"
and "=" are perfectly good characters in FAT/NTFS/EXT2 ...

Can you get some empirical data on what other browsers will send to the FTP
server?

http://codereview.chromium.org/200145/diff/1/2#newcode329
Line 329: DCHECK(IsValidFTPCommandString(path));
This doesn't seem right.

Well, the call to UnescapeURLComponent() above may yield non-ASCII characters,
which will cause IsValidFTPCommandString() to fail.

In particular, note that UnescapeURLComponent() unescapes all high-bit
characters (c >= 0x80)

[Paweł Hajdan Jr., 2009-09-18 22:34:33]

Code updated to match Firefox's behavior. Test added.

[eroman, 2009-09-18 22:51:23]

lgtm, but please document somewhere the experiments you ran with firefox and how
this differs from the FTP spec.

http://codereview.chromium.org/200145/diff/3001/3002
File net/ftp/ftp_network_transaction.cc (left):

http://codereview.chromium.org/200145/diff/3001/3002#oldcode31
Line 31: // RFC 959 only allows ASCII strings.
Please add a comment in IsValidFTPComment() that we allow non-ASCII, and the
empirical data that led to that decision.

http://codereview.chromium.org/200145/diff/3001/3002
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/200145/diff/3001/3002#newcode326
Line 326: path = UnescapeURLComponent(path, unescape_rules);
 Please add a comment that escaping to non-ASCII is OK.

[wtc, 2009-09-21 18:04:25]

LGTM.