-
-
Notifications
You must be signed in to change notification settings - Fork 793
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
ValueError from oauthlib when validating querystrings is raised as 500 instead of 400 HTTP error #954
Comments
Thanks for the report here @kreatemore! I believe that this should be fixed at the 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:
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 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=[])
```) |
Thanks for the pointers, I'll have a go at it this week! |
@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
Thanks 😊 |
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 Here we have stuff like
in So... there's two options here (or something in the middle):
To be honest at $JOB the way we would handle this is to capture any exception, log the exception, then do an |
Just an aside, I saw #958 and it is a similiar flavor of "non- |
I started testing some of the errors from our logs, and found the following:
After handling 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)
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/
Instead of handling it in either 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
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? |
Oh, I totally forgot about I think taht, for example, catching The question I have a bit is whether even 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. |
…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]>
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
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
Additional context
Thank you!
The text was updated successfully, but these errors were encountered: