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

* 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 <[email protected]>

Author: duck-nukem

AUTHORS

@@ -11,6 +11,7 @@ Abhishek Patel
 Alan Crosswell
 Aleksander Vaskevich
 Alessandro De Angelis
+Alex Szabó
 Allisson Azevedo
 Andrew Chen Wang
 Anvesh Agarwal

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
 

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):

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):
         """

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",

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,21 +103,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",
         }
@@ -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):