From d35f030960617cb4d0dbe9a3e89b797df2e7cf0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20Szab=C3=B3=E2=80=AE?= Date: Tue, 19 Oct 2021 08:16:56 +0200 Subject: [PATCH] Handles ValueErrors with invalid hex values in query strings (#954) (#963) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Handles ValueErrors with invalid hex values in query strings and reraises them as SuspiciousOperations (#954) * Unified erorr naming (err and error) when handling ValueErrors * Added Alex Szabó to AUTHORS * Adds fix message to CHANGELOG.md * Narrows handling of ValueErrors to a specific error (invalid hex in query string) * Fixes formatting Co-authored-by: Asif Saif Uddin --- AUTHORS | 1 + CHANGELOG.md | 5 +++-- oauth2_provider/backends.py | 15 ++++++++++++--- oauth2_provider/views/mixins.py | 11 +++++++++-- tests/test_auth_backends.py | 21 ++++++++++++++++++++ tests/test_client_credential.py | 34 +++++++++++++++++++++++++-------- 6 files changed, 72 insertions(+), 15 deletions(-) diff --git a/AUTHORS b/AUTHORS index dc5572ce7..d5c30ebbf 100644 --- a/AUTHORS +++ b/AUTHORS @@ -11,6 +11,7 @@ Abhishek Patel Alan Crosswell Aleksander Vaskevich Alessandro De Angelis +Alex Szabó Allisson Azevedo Andrew Chen Wang Anvesh Agarwal diff --git a/CHANGELOG.md b/CHANGELOG.md index 16e98073d..9194c8781 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,10 +26,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed * #524 Restrict usage of timezone aware expire dates to Django projects with USE_TZ set to True. -* #955 Avoid doubling of `oauth2_provider` urls mountpath in json response for OIDC view `ConnectDiscoveryInfoView`. - Breaks existing OIDC discovery output * #953 Allow loopback redirect URIs with random ports using http scheme, localhost address and no explicit port configuration in the allowed redirect_uris for Oauth2 Applications (RFC8252) +* #954 Query strings with invalid hex values now raise a SuspiciousOperation exception +* #955 Avoid doubling of `oauth2_provider` urls mountpath in json response for OIDC view `ConnectDiscoveryInfoView`. + Breaks existing OIDC discovery output ## [1.5.0] 2021-03-18 diff --git a/oauth2_provider/backends.py b/oauth2_provider/backends.py index 3f6fab9af..2570cd62b 100644 --- a/oauth2_provider/backends.py +++ b/oauth2_provider/backends.py @@ -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 @@ -14,9 +15,17 @@ 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: + if str(error) == "Invalid hex encoding in query string.": + raise SuspiciousOperation(error) + else: + raise + else: + if valid: + return request.user + return None def get_user(self, user_id): diff --git a/oauth2_provider/views/mixins.py b/oauth2_provider/views/mixins.py index 477d24e24..ebb654216 100644 --- a/oauth2_provider/views/mixins.py +++ b/oauth2_provider/views/mixins.py @@ -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 @@ -150,7 +150,14 @@ 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: + if str(error) == "Invalid hex encoding in query string.": + raise SuspiciousOperation(error) + else: + raise def get_scopes(self): """ diff --git a/tests/test_auth_backends.py b/tests/test_auth_backends.py index 151fc30d2..8eeb8ef12 100644 --- a/tests/test_auth_backends.py +++ b/tests/test_auth_backends.py @@ -1,5 +1,9 @@ +from unittest.mock import patch + +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 @@ -51,6 +55,23 @@ 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) + + @patch("oauth2_provider.backends.OAuthLibCore.verify_request") + def test_value_errors_are_reraised(self, patched_verify_request): + patched_verify_request.side_effect = ValueError("Generic error") + + with pytest.raises(ValueError): + OAuth2Backend().authenticate(request={}) + def test_authenticate_fail(self): auth_headers = { "HTTP_AUTHORIZATION": "Bearer " + "badstring", diff --git a/tests/test_client_credential.py b/tests/test_client_credential.py index 8b9aa3bc2..8159d55db 100644 --- a/tests/test_client_credential.py +++ b/tests/test_client_credential.py @@ -1,8 +1,10 @@ import json +from unittest.mock import patch from urllib.parse import quote_plus 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 @@ -101,6 +103,15 @@ 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): @@ -108,14 +119,6 @@ def setUpClass(cls): 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", } @@ -143,6 +146,21 @@ 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) + + @patch("oauth2_provider.views.mixins.OAuthLibMixin.get_oauthlib_core") + def test_reraises_value_errors_as_is(self, patched_core): + patched_core.return_value.verify_request.side_effect = ValueError("Generic error") + + request = self.request_factory.get("/fake-req") + + with pytest.raises(ValueError): + TestView().verify_request(request) + class TestClientResourcePasswordBased(BaseTest): def test_client_resource_password_based(self):