Permalink
Browse files

Return 401 on incorrect authorization supplied. Fixes #1719.

  • Loading branch information...
jaraco committed Jun 17, 2018
1 parent 773f780 commit 1d41828764d83e52c3012c37c0b9c721f4c260d1
Showing with 13 additions and 4 deletions.
  1. +13 −4 cherrypy/lib/auth_digest.py
@@ -163,20 +163,28 @@ class HttpDigestAuthorization(object):
re-calculation of the digest.
"""
scheme = 'digest'
def errmsg(self, s):
return 'Digest Authorization header: %s' % s
@classmethod
def matches(cls, header):
scheme, _, _ = header.partition(' ')
return scheme.lower() == cls.scheme

This comment has been minimized.

@webknjaz

webknjaz Jun 17, 2018

Member

how about header[:len(cls.scheme)].lower() == cls.scheme and header[len(cls.scheme)] == ' '?

This comment has been minimized.

@webknjaz

webknjaz Jun 17, 2018

Member

or

return header[:len(cls.scheme) + 1].lower() == '{} '.format(cls.scheme)
def __init__(
self, auth_header, http_method,
debug=False, accept_charset=DEFAULT_CHARSET[:],
):
self.http_method = http_method
self.debug = debug
scheme, params = auth_header.split(' ', 1)
self.scheme = scheme.lower()
if self.scheme != 'digest':
if not self.matches(auth_header):
raise ValueError('Authorization scheme is not "Digest"')
scheme, params = auth_header.split(' ', 1)

This comment has been minimized.

@webknjaz

webknjaz Jun 17, 2018

Member

@jaraco you split the header twice, this could be unified

This comment has been minimized.

@jaraco

jaraco Jun 17, 2018

Member

Yeah, I was a little unhappy about that, but those two splits are subtly different. The first just ensures that 'digest' was indicated. The second actually enforces that the scheme is followed by a space. I didn't necessarily want to enforce that second aspect during the "matches" pre-check. Moreover, I don't want to burden a caller of .matches with an interface that handles params. All of these concerns left me to decide to have somewhat duplicative functionality (it actually gets called three times, once prior to the construction of this object, then twice in the construction of this object).

If you can propose something better, I'm open to ideas.

This comment has been minimized.

@webknjaz

webknjaz Jun 17, 2018

Member

We could also have this method returning actual parts and raising an exception if it's not a match, so it would be a single time check:

    @classmethod
    def split_authorization_header(cls, header):
        scheme, _, params = header.partition(' ')
        if scheme.lower() != cls.scheme:
            raise ValueError(
                'Authorization scheme {present_scheme} is not "{expected_scheme}"'.
                format(present_scheme=scheme, expected_scheme=cls.scheme.title())
            )
        return scheme, params
self.auth_header = auth_header
# make a dict of the params
@@ -407,8 +415,9 @@ def digest_auth(realm, get_ha1, key, debug=False, accept_charset='utf-8'):
respond_401 = functools.partial(
_respond_401, realm, key, accept_charset, debug)
if auth_header is None:
if not HttpDigestAuthorization.matches(auth_header or ''):

This comment has been minimized.

@webknjaz

webknjaz Jun 17, 2018

Member
try:
    HttpDigestAuthorization.split_authorization_header(auth_header)
except ValueError:
    respond_401()
respond_401()
msg = 'The Authorization header could not be parsed.'
with cherrypy.HTTPError.handle(ValueError, 400, msg):
auth = HttpDigestAuthorization(

0 comments on commit 1d41828

Please sign in to comment.