Patchwork url: avoid re-issuing incorrect password (issue3210)

login
register
mail settings
Submitter Kim Randell
Date July 29, 2016, 2:46 p.m.
Message ID <554314fc96124a2e9a4fe9eb20aa6727@OMGSVR120.omg.local>
Download mbox | patch
Permalink /patch/16005/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Kim Randell - July 29, 2016, 2:46 p.m.
# HG changeset patch
# User Kim Randell
# Date 1469792767 -3600
#      Fri Jul 29 12:46:07 2016 +0100
# Branch stable
# Node ID 9ae7fc90e0bac36de9b725587eb093f98b34619f
# Parent  8421cbebc783e7f3cb17cfb62b4095113f8d666b
url: avoid re-issuing incorrect password (issue3210)

Some draconian IT setups lock accounts after a small number of incorrect
password attempts. Mercurial's implementation of the urllib2 authentication was
causing 5 retry attempts with the same credentials, without prompting the user.
The code was attempting to check whether the authorization token had changed,
but unfortunately was reading the misleading 'headers' member of the request
instead of using the 'get_header' accessor.

Modelled on fix for Python issue 8797:
https://bugs.python.org/issue8797
https://hg.python.org/cpython/rev/30e8a8f22a2a
Matt Mackall - July 29, 2016, 3:32 p.m.
On Fri, 2016-07-29 at 14:46 +0000, Kim Randell wrote:
> # HG changeset patch
> # User Kim Randell
> # Date 1469792767 -3600
> #      Fri Jul 29 12:46:07 2016 +0100
> # Branch stable
> # Node ID 9ae7fc90e0bac36de9b725587eb093f98b34619f
> # Parent  8421cbebc783e7f3cb17cfb62b4095113f8d666b
> url: avoid re-issuing incorrect password (issue3210)
> 
> Some draconian IT setups lock accounts after a small number of incorrect
> password attempts. Mercurial's implementation of the urllib2 authentication
> was
> causing 5 retry attempts with the same credentials, without prompting the
> user.
> The code was attempting to check whether the authorization token had changed,
> but unfortunately was reading the misleading 'headers' member of the request
> instead of using the 'get_header' accessor.
> 
> Modelled on fix for Python issue 8797:
> https://bugs.python.org/issue8797
> https://hg.python.org/cpython/rev/30e8a8f22a2a

Wow, this looks too simple to be true!

The salient difference between headers.get and get_headers is:

    def get_header(self, header_name, default=None):
        return self.headers.get(
            header_name,
            self.unredirected_hdrs.get(header_name, default))

..this unredirected_hdrs field, which is where we're actually stashing the auth
header:

            req.add_unredirected_header(self.auth_header, auth)

Looks like our copy of this code went in in Dec 2013, while the fix went into
CPython (buried in a much larger patch, ugh) in Aug 2014.



> diff -r 8421cbebc783 -r 9ae7fc90e0ba mercurial/url.py
> --- a/mercurial/url.py	Wed Jul 27 13:57:51 2016 +0100
> +++ b/mercurial/url.py	Fri Jul 29 12:46:07 2016 +0100
> @@ -451,7 +451,7 @@
>          if pw is not None:
>              raw = "%s:%s" % (user, pw)
>              auth = 'Basic %s' % base64.b64encode(raw).strip()
> -            if req.headers.get(self.auth_header, None) == auth:
> +            if req.get_header(self.auth_header, None) == auth:
>                  return None
>              self.auth = auth
>              req.add_unredirected_header(self.auth_header, auth)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - July 30, 2016, 10:59 a.m.
On Fri, 29 Jul 2016 10:32:10 -0500, Matt Mackall wrote:
> On Fri, 2016-07-29 at 14:46 +0000, Kim Randell wrote:
> > # HG changeset patch
> > # User Kim Randell
> > # Date 1469792767 -3600
> > #      Fri Jul 29 12:46:07 2016 +0100
> > # Branch stable
> > # Node ID 9ae7fc90e0bac36de9b725587eb093f98b34619f
> > # Parent  8421cbebc783e7f3cb17cfb62b4095113f8d666b
> > url: avoid re-issuing incorrect password (issue3210)
> > 
> > Some draconian IT setups lock accounts after a small number of incorrect
> > password attempts. Mercurial's implementation of the urllib2 authentication
> > was
> > causing 5 retry attempts with the same credentials, without prompting the
> > user.
> > The code was attempting to check whether the authorization token had changed,
> > but unfortunately was reading the misleading 'headers' member of the request
> > instead of using the 'get_header' accessor.
> > 
> > Modelled on fix for Python issue 8797:
> > https://bugs.python.org/issue8797
> > https://hg.python.org/cpython/rev/30e8a8f22a2a
> 
> Wow, this looks too simple to be true!
> 
> The salient difference between headers.get and get_headers is:
> 
>     def get_header(self, header_name, default=None):
>         return self.headers.get(
>             header_name,
>             self.unredirected_hdrs.get(header_name, default))
> 
> ..this unredirected_hdrs field, which is where we're actually stashing the auth
> header:
> 
>             req.add_unredirected_header(self.auth_header, auth)
> 
> Looks like our copy of this code went in in Dec 2013, while the fix went into
> CPython (buried in a much larger patch, ugh) in Aug 2014.

Yeah, this patch looks good per comparing our initial copy, a939eeb94833, and
CPython history.

check-commit complains. Kim, can we copy your email address?

+  Revision 87b01d3c2222 does not comply with rules
+  ------------------------------------------------------
+  1: username is not an email address
+   # User Kim Randell
Kim Randell - July 30, 2016, 9:13 p.m.
Yuya Nishihara wrote:
> check-commit complains. Kim, can we copy your email address?

Yep, fine by me.

--
Kim
Matt Mackall - July 30, 2016, 11:55 p.m.
On Sat, 2016-07-30 at 21:13 +0000, Kim Randell wrote:
> Yuya Nishihara wrote:
> > 
> > check-commit complains. Kim, can we copy your email address?
> Yep, fine by me.

Queued for stable, thanks!

Patch

diff -r 8421cbebc783 -r 9ae7fc90e0ba mercurial/url.py
--- a/mercurial/url.py	Wed Jul 27 13:57:51 2016 +0100
+++ b/mercurial/url.py	Fri Jul 29 12:46:07 2016 +0100
@@ -451,7 +451,7 @@ 
         if pw is not None:
             raw = "%s:%s" % (user, pw)
             auth = 'Basic %s' % base64.b64encode(raw).strip()
-            if req.headers.get(self.auth_header, None) == auth:
+            if req.get_header(self.auth_header, None) == auth:
                 return None
             self.auth = auth
             req.add_unredirected_header(self.auth_header, auth)