kumar303 / hawkrest

Hawk HTTP Authorization for Django Rest Framework
BSD 3-Clause "New" or "Revised" License
19 stars 8 forks source link

Authentication backend and middleware use different strings to check if Hawk request #37

Closed edmorley closed 6 years ago

edmorley commented 6 years ago

This evening Treeherder experienced thousands of HTTP 500s of form:

Traceback (most recent call last): 
  File "/app/.heroku/python/lib/python2.7/site-packages/django/core/handlers/base.py", line 131, in get_response 
    response = middleware_method(request, response) 
  File "/app/.heroku/python/lib/python2.7/site-packages/newrelic/hooks/framework_django.py", line 333, in wrapper 
    return wrapped(*args, **kwargs) 
  File "/app/.heroku/python/lib/python2.7/site-packages/hawkrest/middleware.py", line 22, in process_response 
    raise RuntimeError('Django did not handle an incoming ' 
RuntimeError: Django did not handle an incoming Hawk request properly 

(Using hawkrest 1.0.0, mohawk 0.3.4, djangorestframework 3.6.4, Django 1.11.6, Python 2.7.14)

This exception only occurs if the authentication backend didn't process the request, but middleware.py's process_response() did.

From code inspection, one way this might occur, if is the HTTP_AUTHORIZATION header contained the string Hawk with no space before subsequent characters, since the conditional for both pieces aren't consistent in their use of whitespace: https://github.com/kumar303/hawkrest/blob/1.0.0/hawkrest/__init__.py#L74 https://github.com/kumar303/hawkrest/blob/1.0.0/hawkrest/middleware.py#L11

Whilst looking at the middleware it also seems like it could be simplified somewhat, with either an early return, or at least a consolidation of conditionals - worth doing given middleware is on the hot path.

kumar303 commented 6 years ago

Yeah, it looks like checking for the auth header is out of sync.

It checks with a space here: https://github.com/kumar303/hawkrest/blob/ebb9690f8930a936fb2cc6c5ba19b70061c9bc94/hawkrest/__init__.py#L74

And it checks without a space here: https://github.com/kumar303/hawkrest/blob/ebb9690f8930a936fb2cc6c5ba19b70061c9bc94/hawkrest/middleware.py#L11

They should both use a shared helper function to know if the request was Hawk authenticated or not.