Skip to content

Commit

Permalink
Return 401 on incorrect authorization supplied. Fixes #1719.
Browse files Browse the repository at this point in the history
  • Loading branch information
@jaraco
jaraco committed Jun 17, 2018
1 parent 773f780 commit 1d41828
Showing 1 changed file with 13 additions and 4 deletions.
17 changes: 13 additions & 4 deletions cherrypy/lib/auth_digest.py
View file
Expand Up @@ -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.

Sign in to view

Copy link
@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.

Sign in to view

Copy link
@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.

Sign in to view

Copy link
@webknjaz

webknjaz Jun 17, 2018

Member

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

This comment has been minimized.

Sign in to view

Copy link
@jaraco

jaraco Jun 17, 2018

Author 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.

Sign in to view

Copy link
@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
Expand Down Expand Up @@ -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.

Sign in to view

Copy link
@webknjaz

webknjaz Jun 17, 2018 •
edited

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(
Expand Down

0 comments on commit 1d41828

Please sign in to comment.