Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handles ValueErrors with invalid hex values in query strings (#954) #963

Merged
merged 9 commits into from
Oct 19, 2021
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Abhishek Patel
Alan Crosswell
Aleksander Vaskevich
Alessandro De Angelis
Alex Szabó
Allisson Azevedo
Anvesh Agarwal
Aristóbulo Meneses
Expand Down
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Added
* #712, #636, #808. Calls to `django.contrib.auth.authenticate()` now pass a `request`
to provide compatibility with backends that need one.

### Fixed
* #524 Restrict usage of timezone aware expire dates to Django projects with USE_TZ set to True.
* #954 Query strings with invalid hex values now raise a SuspiciousOperation exception

## [1.5.0] 2021-03-18

Expand Down
12 changes: 9 additions & 3 deletions oauth2_provider/backends.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from django.contrib.auth import get_user_model
from django.core.exceptions import SuspiciousOperation

from .oauth2_backends import get_oauthlib_core

Expand All @@ -14,9 +15,14 @@ class OAuth2Backend:

def authenticate(self, request=None, **credentials):
if request is not None:
valid, r = OAuthLibCore.verify_request(request, scopes=[])
if valid:
return r.user
try:
valid, request = OAuthLibCore.verify_request(request, scopes=[])
except ValueError as error:
raise SuspiciousOperation(error)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As raised in #954 (comment) is catching all ValueErrors too broad? I think it should be fine, but it might end up exposing error messages users were never meant to see, possibly resulting in security flaws?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also a bit worried about this catching "real issues" and causing them to end up not being logged (for example in one Django setup I know SuspiciousOperation won't get logged into Sentry but a raw ValueError would). So I could imagine some time lost as this absorbs a real error.

The minimalist approach here would be to check if error.message == "invalid hex string" (or whatever) and only transform the error then (otherwise just reraising the ValueError). But to be honest this might not be a major thing.

Copy link
Contributor Author

@duck-nukem duck-nukem Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated it now to check the message of the error, otherwise re-raises the exception as-is.

else:
if valid:
return request.user

return None

def get_user(self, user_id):
Expand Down
8 changes: 6 additions & 2 deletions oauth2_provider/views/mixins.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import logging

from django.conf import settings
from django.core.exceptions import ImproperlyConfigured
from django.core.exceptions import ImproperlyConfigured, SuspiciousOperation
from django.http import HttpResponseForbidden, HttpResponseNotFound

from ..exceptions import FatalClientError
Expand Down Expand Up @@ -150,7 +150,11 @@ def verify_request(self, request):
:param request: The current django.http.HttpRequest object
"""
core = self.get_oauthlib_core()
return core.verify_request(request, scopes=self.get_scopes())

try:
return core.verify_request(request, scopes=self.get_scopes())
except ValueError as error:
raise SuspiciousOperation(error)

def get_scopes(self):
"""
Expand Down
13 changes: 12 additions & 1 deletion tests/test_auth_backends.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import pytest
from django.contrib.auth import get_user_model
from django.contrib.auth.models import AnonymousUser
from django.core.exceptions import SuspiciousOperation
from django.http import HttpResponse
from django.test import RequestFactory, TestCase
from django.test.utils import modify_settings, override_settings
Expand All @@ -9,7 +11,6 @@
from oauth2_provider.middleware import OAuth2TokenMiddleware
from oauth2_provider.models import get_access_token_model, get_application_model


UserModel = get_user_model()
ApplicationModel = get_application_model()
AccessTokenModel = get_access_token_model()
Expand Down Expand Up @@ -51,6 +52,16 @@ def test_authenticate(self):
u = backend.authenticate(**credentials)
self.assertEqual(u, self.user)

def test_authenticate_raises_error_with_invalid_hex_in_query_params(self):
auth_headers = {
"HTTP_AUTHORIZATION": "Bearer " + "tokstr",
}
request = self.factory.get("/a-resource?auth_token=%%7A", **auth_headers)
credentials = {"request": request}

with pytest.raises(SuspiciousOperation):
OAuth2Backend().authenticate(**credentials)

def test_authenticate_fail(self):
auth_headers = {
"HTTP_AUTHORIZATION": "Bearer " + "badstring",
Expand Down
24 changes: 16 additions & 8 deletions tests/test_client_credential.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import pytest
from django.contrib.auth import get_user_model
from django.core.exceptions import SuspiciousOperation
from django.test import RequestFactory, TestCase
from django.urls import reverse
from django.views.generic import View
Expand Down Expand Up @@ -101,21 +102,22 @@ def test_client_credential_user_is_none_on_access_token(self):
self.assertIsNone(access_token.user)


class TestView(OAuthLibMixin, View):
server_class = BackendApplicationServer
validator_class = OAuth2Validator
oauthlib_backend_class = OAuthLibCore

def get_scopes(self):
return ["read", "write"]


class TestExtendedRequest(BaseTest):
@classmethod
def setUpClass(cls):
cls.request_factory = RequestFactory()
super().setUpClass()

def test_extended_request(self):
class TestView(OAuthLibMixin, View):
server_class = BackendApplicationServer
validator_class = OAuth2Validator
oauthlib_backend_class = OAuthLibCore

def get_scopes(self):
return ["read", "write"]

token_request_data = {
"grant_type": "client_credentials",
}
Expand Down Expand Up @@ -143,6 +145,12 @@ def get_scopes(self):
self.assertEqual(r.client, self.application)
self.assertEqual(r.scopes, ["read", "write"])

def test_raises_error_with_invalid_hex_in_query_params(self):
request = self.request_factory.get("/fake-req?auth_token=%%7A")

with pytest.raises(SuspiciousOperation):
TestView().verify_request(request)


class TestClientResourcePasswordBased(BaseTest):
def test_client_resource_password_based(self):
Expand Down