urllib2 basicauth broken in 2.6.5: RuntimeError: maximum recursion depth exceeded in cmp #53043
Comments
|
The URL I'm trying to open is using basicauth and the username/password is incorrect. This works flawlessly in 2.6.4. File "/home/jono/work/sites/test123/urllib2.py", line 397, in open |
jonozzz
mannequin
added
type-crash
|
Is the behavior you posted observable on trunk? |
|
Okay, I see you have mentioned 2.6.5 in the Title.
|
|
The same problem for me, it tries to authenticate infinite times if the user/password are incorrect |
|
This is fixed in r81636, r81637, r81638, r81637. I had a thought that Basic Authentication need not be retried (no where in the RFC it mentions about it), but I found some references that clients do present the dialog a couple of times for retry on wrong authentication, so we are going for the same. This is tested with actual resource setup. Closing this issue as this bug is fixed. |
|
Why would you waste the time and resources to test 5 times for a known to be wrong credential ?! This is not like in a browser where the user is presented with a dialog box 5 times and he/she can try different credentials. I don't see the point... |
|
The point of retry is for auth failure, which can happen due to any reason. It is not just for a wrong password. I was thinking just doing a single verification and failing, but did find some resources (not just browsers) which adopt this approach of retry. BTW, Basic auth is gone now and Digest auth has some recommendations so I found its fine to do the same way as Digest auth does. |
|
I would like to point out that this is not going to work if someone visits more than 5 sites with the same authentication manager. This would have to be documentated, at least. |
|
+1 for putting retry control on the password manager. Probably the default should be set to 1. If I understand correctly, it is the password manager that would be prompting the user for a new password, if someone chose to implement such a password manager. |
|
Agree to single retry for Basic Auth. We are just dealing with BasicAuthentication here, so that's why we did not have in the HTTPPasswdManager. But, let me consider that viewpoint too. |
|
FYI: This also causes problems with Mercurial pushing to google code - see http://mercurial.selenic.com/bts/issue2179 . IIRC google code redirects to an url which then in turn requests authentication. I'm not sure how that will work with a retry count of 1. The funny and inconsistent thing is that it apparently would work if they replied 403 instead of 401. "durin42" from google/mercurial knows the details. (It seems suspicious to me that there are code paths where the retry counter is reset. Doesn't that mean that a remote server will be able to cause endless recursion? Shouldn't there be some kind of hard upper limit on the number of retries?) |
|
I work in sidux and my Mercurial currently doesn't work. The python version already contains the fix for this issue (revision 81637) and it crashes Mercurial ("authorization failed") whenever a command involves more than 5 requests to the repository. I fixed it by resetting the retry counter upon successful authorization (see patch). Maybe this helps someone in a similar situation. The patch was made against trunk using "diff -u". |
|
zenyatta: Which Mercurial version? We thought we had implemented a sufficiently ugly workaround in Mercurial. Please file an issue in http://mercurial.selenic.com/bts/ . |
|
I've also run into this problem after upgrading to Python 2.6.6. My code, which uses the same HTTPBasicAuthHandler instance for many requests to the same server, worked correctly with Python 2.6.2 and broke with 2.6.6. It would be great if zenyatta's patch to fix the regression was included in 2.6.7. |
|
I checked in a modified version of reset the retry count for respnse code !=401 in the following checkins: Unfortunately, we wont be able to patch the 2.6.x release. You might want to use patch which is attached or fix it at mercurial end. Another issue, bpo-9639 was fixed to reset the retry on successful response. I am going to close this issue, there was a minor discussion in the middle if 5 retries in worth it, but it looks like it is (msg107261). |
That should be release27-maint. :) |
|
Senthil, can you tell us why this fix is correct - and convince us that it is the Final Fix for this issue? Not because I don't trust you, but because this issue has a bad track record. Some comments/questions to this patch: Why do 401 require such special handling? Why not handle it like the other errors? How do this work together with http://code.google.com/p/support/issues/detail?id=3985 ? Detail: I'm surprised you don't use reset_retry_count() - that makes it a bit harder to grok the code. And the patch doesn't reduce the complexity of the code. But ... I really don't understand ... .retried is a kind of error counter. Why do we reset it on errors? I would expect it to be reset on success ... or perhaps on anything but 401, 403 and 407. Or perhaps it should be reset whenever a new URL is requested. |
|
On Thu, Aug 26, 2010 at 3:24 PM, Mads Kiilerich <report@bugs.python.org> wrote:
Hello Mads, this may not be be final fix. I just adopted what was I agree with you respect to the other error codes, there is already Thanks, |
|
On 08/27/2010 03:47 AM, Senthil Kumaran wrote:
FWIW: From Mercurial it is our experience that the reset counter only |
|
See also new bpo-9698. |
|
I think there's a much simpler solution to this ticket than the retry logic that's currently in place. The code originally avoided the infinite recursion by checking to see if the previous request had already submitted the auth credentials that would be used in the retry. If it had, it would return None. If it hadn't, it would add the auth credentials to the request header and the request again: if req.headers.get(self.auth_header, None) == auth:
return None
req.add_header(self.auth_header, auth)Then, to fix bpo-3819, it was changed. Instead of calling add_header, it called add_unredirected_header: if req.headers.get(self.auth_header, None) == auth:
return None
req.add_unredirected_header(self.auth_header, auth)This caused the loop because the auth creds were going into unredirected_hdrs instead of the headers dict. But I think the original logic is sound. The code just wasn't checking in all the headers. Luckily there's a get_header method that checks both for you. This one-line change should fix the issue: if req.get_header(self.auth_header, None) == auth:
return None
req.add_unredirected_header(self.auth_header, auth)I think this fix is cleaner and makes more sense, but I'm worried I might be missing something. I don't fully understand the distinction between headers and unredirected headers. Maybe there's a reason why the code isn't checking in unredirected headers for the auth header. I'm attaching a patch. I'm new to contributing to python so I apologize if the format is wrong. |
pitrou
removed
the
type-crash
|
Thanks Sam! I can confirm that your patch is working well with basic auth. I have not tried other authentication methods, though. I'm not sure about the unredirected headers, but your fix looks like the cleanest. |
|
This bug has been open for a while and I had lost sight of it. Upon prompted recently, I dug bit A brief history.
However, this fix is not satisfactory for multiple reasons like fix is not satisfactory and not Also, it is important to note that, before a regression was introduced in r59118, the prior So, going back to that behavior was the right thing to do. Sam Bull's suggestion and patch looks right to me. It is correct to verify the un-redirected hdrs I have improved upon the patch for 3.5 and added tests which were not present too. I think, this |
|
New changeset e0510a3bdf8f by Senthil Kumaran in branch '2.7': New changeset 3435c5865cfc by Senthil Kumaran in branch '3.4': New changeset 7cb90d70ce19 by Senthil Kumaran in branch 'default': |
|
test_urllib2_localnet now fails since the changeset 3435c5865cfc. Example: test_basic_auth_success (test.test_urllib2_localnet.BasicAuthTests) ... ---------------------------------------- Exception happened during processing of request from ('127.0.0.1', 51031)
Traceback (most recent call last):
File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/socketserver.py", line 321, in _handle_request_noblock
self.process_request(request, client_address)
File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/socketserver.py", line 347, in process_request
self.finish_request(request, client_address)
File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/socketserver.py", line 360, in finish_request
self.RequestHandlerClass(request, client_address, self)
File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/test/test_urllib2_localnet.py", line 289, in http_server_with_basic_auth_handler
return BasicAuthHandler(*args, **kwargs)
File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/test/test_urllib2_localnet.py", line 212, in __init__
http.server.SimpleHTTPRequestHandler.__init__(self, *args, **kwargs)
File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/socketserver.py", line 685, in __init__
self.handle()
File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/http/server.py", line 398, in handle
self.handle_one_request()
File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/http/server.py", line 386, in handle_one_request
method()
File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/test/test_urllib2_localnet.py", line 235, in do_GET
http.server.SimpleHTTPRequestHandler.do_GET(self)
File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/http/server.py", line 677, in do_GET
f = self.send_head()
File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/http/server.py", line 716, in send_head
return self.list_directory(path)
File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/http/server.py", line 772, in list_directory
% (urllib.parse.quote(linkname), html.escape(displayname)))
File "/home/buildbot/buildarea/3.x.coghlan-redhat/build/Lib/urllib/parse.py", line 688, in quote
string = string.encode(encoding, errors)
UnicodeEncodeError: 'utf-8' codec can't encode character '\udcff' in position 16: surrogates not allowedERROR |
|
New changeset 5336a01542e2 by Senthil Kumaran in branch '2.7': New changeset d51e739004bc by Senthil Kumaran in branch '3.4': New changeset 3d45155b7b9b by Senthil Kumaran in branch 'default': |
|
I have backed out my changes. The buildbot failures were new as I could not reproduce them in my local test on Mac. I will run the buildbot suite on branch and fix it before committing. |
|
May be bpo-22165 patch fixes this test failure. |
|
New changeset c1edc4e43eb1 by Senthil Kumaran in branch '2.7': New changeset 30e8a8f22a2a by Senthil Kumaran in branch '3.4': New changeset 10d0a692b1b6 by Senthil Kumaran in branch 'default': |
|
This is fixed in all active versions (2.7.8+, 3.4.2? and 3.5). Thanks all! |
|
I don't have an example case, but from reading the code it looks as though the AbstractDigestAuthHandler has exactly the same faults as the old version of AbstractBasicAuthHandler (i.e. using req.headers.get instead of req.get_header, and corresponding retry workarounds). Should it be updated to match? |
|
Kim: if you can prove it is broken, please open a new issue, this one is closed (you can reference it by just typing # followed by the issue number, and the tracker will automatically make a link). |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: