ValueError from oauthlib when validating querystrings is raised as 500 instead of 400 HTTP error

[duck-nukem]

Describe the bug
When passing in invalid hex encoding in the querystring (for example %%2A), a ValueError is raised in https://github.com/oauthlib/oauthlib/blob/b69fa53fd836dc559aa7fcd78ce075bcbe361629/oauthlib/common.py#L395 and the Django application will return 500 HTTP error code.

To Reproduce

1. Have an endpoint that accepts the token in a querystring
2. Send an invalid querystring (in our example it was an extra % - not converted to %25) just before a hex value.

Examples:
/endpoint/?evae%%7B7%2A191%7Daith=
/endpoint/?beiy%%%%%%%%3moh=

Expected behavior
I think a 400 status would be more appropriate, it's the client that's sending invalid data.

Version
1.5.0

• I have tested with the latest published release and it's still a problem.
• I have tested with the master branch and it's still a problem.

Additional context

1. First I'd like to ask some help in triaging this issue - is it best fixed in django-oauth-toolkit, or would it be better to be fixed in oauthlib?
2. More than happy to contribute to the issue with a PR

Thank you!

[rtpg]

Thanks for the report here @kreatemore!

I believe that this should be fixed at the django-oauth-toolkit level, and my gut feeling is that this should be captured at the view level.

I would want to be sure that we get the issue fixed, so would you be able to provide a patch or a code snippet with either:

• some code that you think fixes your specific usecase
• a testcase that you think covers your specific usecase, even if it doesn't fix it

If you provide either one or the other, I can provide the other "half" to get this in a mergable state. If you provide both, that would be the most helpful of course!

Thanks for the detailed report here in any case

(for my own triaging, I applied the following change and was able to see the ValueError discussed here by applying the following change and running tox -e py38-dj31 -- -k test_validate_authorization_request_unsafe_query

modified   tests/test_oauth2_backends.py
@@ -106,7 +106,7 @@ class TestOAuthLibCore(TestCase):
         auth_headers = {
             "HTTP_AUTHORIZATION": "Bearer " + "a_casual_token",
         }
-        request = self.factory.get("/fake-resource?next=/fake", **auth_headers)
+        request = self.factory.get("/fake-resource?evae%%7B7%2A191%7Daith=", **auth_headers)
 
         oauthlib_core = get_oauthlib_core()
         oauthlib_core.verify_request(request, scopes=[])
```)

[duck-nukem]

Thanks for the pointers, I'll have a go at it this week!

[duck-nukem]

@rtpg I'm making progress on this, however I'd like to ask a couple of questions before submitting a PR:

The issue reported in our logs was originating from oauth2_provider/contrib/rest_framework/authentication.py, however I saw that oauth2_provider.backends.OAuth2Backend.authenticate is prone to the same error (maybe more, haven't done a thorough search yet).

• Should I look at both, and if so, is there a way where a function used by both could live?
• For oauth2_provider.contrib.rest_framework.authentication.OAuth2Authentication.authenticate I think raise ParseError(detail='Invalid hex encoding in query string') (error message is not final) is appropriate, but not sure what exception should be raised from oauth2_provider.backends.OAuth2Backend.authenticate

Thanks 😊

[rtpg]

Sorry, had to take a minute to look through the code and what existing stuff was in place already.

I think the overall objective here is to make it so that this kind of failure returns a 400 instead of a 500. So we're looking at a view-level change.

In oauth2_provider.views.base there's AuthorizationView, which is used by a lot of the endpoints. I think it makes sense for the error handling to happen in that area in general, and that should end up being used by many of the configurations people have in place.

Here we have stuff like

    def get(self, request, *args, **kwargs):
        try:
            scopes, credentials = self.validate_authorization_request(request)
        except OAuthToolkitError as error:
            # Application is not available at this time.
            return self.error_response(error, application=None)

in AuthorizationView. There are a couple try/except there.

So... there's two options here (or something in the middle):

• modify the except clause to capture that ValueError and do an error_response from there
• capture the ValueError further down the stack and then transform that into an OAuthToolkitError.

To be honest at $JOB the way we would handle this is to capture any exception, log the exception, then do an error_response (since in any case you need to reply!), but we don't really have the logging or anything set up here. So here transforming the exception class into OAuthToolkitError might be the smoothest way forward here.

[rtpg]

Just an aside, I saw #958 and it is a similiar flavor of "non-OAuthToolkitError bubbling up to the user" error... we probably need to get a bit better about handling exceptions that happen within a request.

[duck-nukem]

I started testing some of the errors from our logs, and found the following:

$ curl -o /dev/null -sw '%{http_code}\n' -X POST "http://localhost/endpoint/?evae%%7B7%2A191%7Daith="
status=500 - ValueError: Invalid hex encoding in query string.

After handling ValueError it's still a 500 from rest framework, I think this is fine as-is. Here's the patch that I used to handle it (just in case):

Index: oauth2_provider/contrib/rest_framework/authentication.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/oauth2_provider/contrib/rest_framework/authentication.py b/oauth2_provider/contrib/rest_framework/authentication.py
--- a/oauth2_provider/contrib/rest_framework/authentication.py	(revision 27bd0af6d86864268c0fcb6a0e3dbb2a08ace74d)
+++ b/oauth2_provider/contrib/rest_framework/authentication.py	(date 1618324966678)
@@ -1,5 +1,6 @@
 from collections import OrderedDict
 
+from django.core.exceptions import SuspiciousOperation
 from rest_framework.authentication import BaseAuthentication
 
 from ...oauth2_backends import get_oauthlib_core
@@ -24,9 +25,15 @@
         or None otherwise.
         """
         oauthlib_core = get_oauthlib_core()
-        valid, r = oauthlib_core.verify_request(request, scopes=[])
-        if valid:
-            return r.user, r.access_token
+
+        try:
+            valid, r = oauthlib_core.verify_request(request, scopes=[])
+        except ValueError as err:
+            raise SuspiciousOperation(err)
+        else:
+            if valid:
+                return r.user, r.access_token
+
         request.oauth2_error = getattr(r, "oauth2_error", {})
         return None
 
Index: tests/test_rest_framework.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tests/test_rest_framework.py b/tests/test_rest_framework.py
--- a/tests/test_rest_framework.py	(revision 27bd0af6d86864268c0fcb6a0e3dbb2a08ace74d)
+++ b/tests/test_rest_framework.py	(date 1618325479381)
@@ -158,6 +158,12 @@
         response = self.client.get("/oauth2-test/", HTTP_AUTHORIZATION=auth)
         self.assertEqual(response.status_code, 200)
 
+    def test_authentication_with_invalid_hex(self):
+        auth = self._create_authorization_header(self.access_token.token)
+
+        response = self.client.get("/oauth2-test/?%%7A", HTTP_AUTHORIZATION=auth)
+        self.assertEqual(response.status_code, 400)
+
     def test_authentication_denied(self):
         response = self.client.get("/oauth2-test/")
         self.assertEqual(response.status_code, 401)

---

$ curl -o /dev/null -sw '%{http_code}\n' -X POST "http://localhost/oauth/token/?grant_type=%%7A"
status=500 - ValueError: Invalid hex encoding in query string.

Index: tests/test_client_credential.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tests/test_client_credential.py b/tests/test_client_credential.py
--- a/tests/test_client_credential.py	(revision 27bd0af6d86864268c0fcb6a0e3dbb2a08ace74d)
+++ b/tests/test_client_credential.py	(date 1618325938706)
@@ -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
@@ -101,21 +102,22 @@
         self.assertIsNone(access_token.user)
 
 
-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
+class TestView(OAuthLibMixin, View):
+    server_class = BackendApplicationServer
+    validator_class = OAuth2Validator
+    oauthlib_backend_class = OAuthLibCore
 
-            def get_scopes(self):
-                return ["read", "write"]
+    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):
         token_request_data = {
             "grant_type": "client_credentials",
         }
@@ -143,6 +145,12 @@
         self.assertEqual(r.client, self.application)
         self.assertEqual(r.scopes, ["read", "write"])
 
+    def test_invalid_hex(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):
Index: oauth2_provider/views/mixins.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/oauth2_provider/views/mixins.py b/oauth2_provider/views/mixins.py
--- a/oauth2_provider/views/mixins.py	(revision 27bd0af6d86864268c0fcb6a0e3dbb2a08ace74d)
+++ b/oauth2_provider/views/mixins.py	(date 1618325938703)
@@ -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,13 @@
         :param request: The current django.http.HttpRequest object
         """
         core = self.get_oauthlib_core()
-        return core.verify_request(request, scopes=self.get_scopes())
+
+        try:
+            verified_request = core.verify_request(request, scopes=self.get_scopes())
+        except ValueError as error:
+            raise SuspiciousOperation(error)
+        else:
+            return verified_request
 
     def get_scopes(self):
         """

retest with diff: 400 - Bad request /oauth/token/

---

$ curl -o /dev/null -sw '%{http_code}\n' -X POST "http://localhost/endpoint/?auth_token=evae%%7B7%2A191%7Daith="
status=500 - ValueError: Invalid hex encoding in query string.

Instead of handling it in either oauth2_provider.backends.OAuth2Backend.authenticate or oauth2_provider.oauth2_backends.OAuthLibCore.verify_request I tried changing the middleware in hopes of being able to catch more errors without the need of handling them individually:

Index: oauth2_provider/middleware.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/oauth2_provider/middleware.py b/oauth2_provider/middleware.py
--- a/oauth2_provider/middleware.py	(revision 27bd0af6d86864268c0fcb6a0e3dbb2a08ace74d)
+++ b/oauth2_provider/middleware.py	(date 1618322848954)
@@ -1,4 +1,5 @@
 from django.contrib.auth import authenticate
+from django.core.exceptions import SuspiciousOperation
 from django.utils.cache import patch_vary_headers
 
 
@@ -29,9 +30,13 @@
         # do something only if request contains a Bearer token
         if request.META.get("HTTP_AUTHORIZATION", "").startswith("Bearer"):
             if not hasattr(request, "user") or request.user.is_anonymous:
-                user = authenticate(request=request)
-                if user:
-                    request.user = request._cached_user = user
+                try:
+                    user = authenticate(request=request)
+                except Exception as e:
+                    raise SuspiciousOperation(e)
+                else:
+                    if user:
+                        request.user = request._cached_user = user
 
         response = self.get_response(request)
         patch_vary_headers(response, ("Authorization",))

retest with diff: retest with diff: 400 - Bad request /endpoint/

I've used django.core.exceptions.SuspiciousOperation as it:

• uses the 400 HTTP status code
• is explicitly handled by django.core.handlers.exception.response_for_exception
• seemed appropriate for unhandled exceptions regarding authentication

What do you think of this? Should I create a PR with the changes above, or is it not quite what you had in mind?

[rtpg]

Oh, I totally forgot about SuspiciousOperation, that is a nice and easy solution here IMO.

I think taht, for example, catching ValueErrors that way would be a good solution. I don't think the overbroad capturing of all exceptions in your last version of the patch (because it would mean that certain things like configuration errors might leak through.

The question I have a bit is whether even ValueError is too broad. Like is there a security implication? 500s in production configurations don't expose anythign to the user, but here we would expose the error message (which I think makes sense here for this case).

I think that opening a PR with that second patch would be a good idea! Then we could get input from other people and see what they think.