Issue 2244: urllib and urllib2 decode userinfo multiple times

Both urllib and urllib2 call urllib.unquote() multiple times on data in
the userinfo section of an FTP URL.  One call occurs at the end of the
urllib.splituser() function.  In urllib, the other call appears in
URLOpener.open_ftp().  In urllib2, the other two occur in
FTPHandler.ftp_open() and Request.get_host().

The effect of this is that if the userinfo section of an FTP url should
need to contain a literal % sign followed by two digits, the % sign must
be double-encoded as %2525 (for urllib) or triple-encoded as %252525
(for urllib2) in order for the URL to be accessed.

The proper behavior would be to only ever unquote a given data segment
once.  The W3's URI: Generic Syntax RFC
(http://gbiv.com/protocols/uri/rfc/rfc3986.html) addresses this very
issue in section 2.4 (When to Encode or Decode): "Implementations must
not percent-encode or decode the same string more than once, as decoding
an already decoded string might lead to misinterpreting a percent data
octet as the beginning of a percent-encoding, or vice versa in the case
of percent-encoding an already percent-encoded string."

The solution would be to standardize where in urllib and urllib2 the
unquoting happens, and then make sure it happens nowhere else.  I'm not
familiar enough with the libraries to know where it should be removed
without possibly breaking other behavior.  It seems that just removing
the map/unquote call in urllib.splituser() would fix the problem in
urllib.  I would guess the call in urllib2 Request.get_host() should
also be removed, as the RFC referenced above says clearly that only
individual data segments of the URL should be decoded, not larger
portions that might contain delimiters (: and @).

I've attached a patchset for these suggested changes.  Very superficial
testing suggests that the patch doesn't break anything obvious, but I
make no guarantees.

Fred: You most recently touched the code impacted by this test, does
this sound reasonable?

Re-assigned as per maintainers list.

By confirming the "%2525" case, I guess this is really a bug. But the patch cause test_urllib2.py failed. I modified the patch to fix it.

This bug exists on py3k also, so I worked out a patch for py3k, too.

Thanks Ray Allen. Usually the patch against the py3k branch is enough, it will be ported to other branches.

I am attaching a patch against py3k which adds some tests to demonstrate the original bug when issuing a Request() to a ftp:// URL.

To do this, the tests add checks for user and passwd.  The previous version of checks asserted that the .user and .passwd of the returned request should be "".  Checking .user is necessary to verify the original bug.  

I was confused by a comment in the fixture, "ftp authentication not yet implemented by FTPHandler", which appeared to justify the assumption that .user and .passwd must be "".  This may be true, but .user and .passwd are being set by the Request.  The test includes the minimal augmentation to extend on the original behavior of the test.  Someone with greater knowledge than I might look at that comment to see if it is out-of-date, or simply too vague.

I can confirm that the combination of urllib_issue_updated.diff and urllib_ftptests_doubleencode.patch apply cleanly against py3k, that the added tests exercise the described bug, and that the full test suite passes after applying the patches.

The patches look clean and conforming to PEP 8.

Fixed in r86520 (py3k) and r86522 (release31-maint). 
unquote happens at the first level inside Request class, _parse method.

Shall port to py2.7.

back-ported to release27-maint in r86554.

messages: + msg121611

resolution: fixed
stage: patch review -> resolved

messages: + msg110875

messages: + msg110858

assignee: fdrake -> orsenthil