diff --git a/addon_service/common/aiohttp_session.py b/addon_service/common/aiohttp_session.py index 6f83274f..9cf356d1 100644 --- a/addon_service/common/aiohttp_session.py +++ b/addon_service/common/aiohttp_session.py @@ -14,6 +14,10 @@ "close_singleton_client_session__blocking", ) +# an identifying user-agent: some providers' CDNs (e.g. Mendeley, behind Cloudflare) +# challenge or block requests sent with aiohttp's default `Python/x.y aiohttp/z` user-agent +USER_AGENT = "GravyValet/1.0" + __SINGLETON_CLIENT_SESSION_STORE: MutableMapping[threading.Thread, ClientSession] = ( weakref.WeakKeyDictionary() ) @@ -30,6 +34,7 @@ async def get_singleton_client_session() -> aiohttp.ClientSession: if thread_id not in __SINGLETON_CLIENT_SESSION_STORE: __SINGLETON_CLIENT_SESSION_STORE[thread_id] = aiohttp.ClientSession( cookie_jar=aiohttp.DummyCookieJar(), # ignore all cookies + headers={"User-Agent": USER_AGENT}, ) return __SINGLETON_CLIENT_SESSION_STORE[thread_id] diff --git a/addon_service/oauth2/views.py b/addon_service/oauth2/views.py index 4c6d30ff..9650c0e6 100644 --- a/addon_service/oauth2/views.py +++ b/addon_service/oauth2/views.py @@ -1,4 +1,5 @@ import asyncio +import logging from http import HTTPStatus from asgiref.sync import sync_to_async @@ -13,6 +14,22 @@ from addon_service.oauth2.utils import get_initial_access_token +_logger = logging.getLogger(__name__) + +# https://www.rfc-editor.org/rfc/rfc6749.html#section-4.1.2.1 +_KNOWN_ERROR_CODES = frozenset( + ( + "invalid_request", + "unauthorized_client", + "access_denied", + "unsupported_response_type", + "invalid_scope", + "server_error", + "temporarily_unavailable", + ) +) + + # Exclude oAuth views from openapi schema as they are from internal use only @extend_schema(exclude=True) @transaction.non_atomic_requests # async views and ATOMIC_REQUESTS do not mix @@ -23,7 +40,20 @@ async def oauth2_callback_view(request): see https://www.rfc-editor.org/rfc/rfc6749.html#section-4.1.2 """ - # TODO: handle error: https://www.rfc-editor.org/rfc/rfc6749.html#section-4.1.2.1 + if "error" in request.GET: + # query params are attacker-controlled: never reflect them, and never as html + _error = request.GET["error"] + _logger.warning( + "oauth2 callback error=%r error_description=%r", + _error[:100], + request.GET.get("error_description", "")[:200], + ) + return HttpResponse( + "oauth2 authorization failed" + + (f": {_error}" if _error in _KNOWN_ERROR_CODES else ""), + status=HTTPStatus.BAD_REQUEST, + content_type="text/plain; charset=utf-8", + ) _state_token = request.GET["state"] _authorization_code = request.GET["code"] _token_metadata, _oauth_client_config = await _resolve_state_token(_state_token) diff --git a/addon_service/tests/test_oauth2_callback_view.py b/addon_service/tests/test_oauth2_callback_view.py new file mode 100644 index 00000000..e3d17396 --- /dev/null +++ b/addon_service/tests/test_oauth2_callback_view.py @@ -0,0 +1,42 @@ +from http import HTTPStatus +from unittest import IsolatedAsyncioTestCase + +from django.test import SimpleTestCase +from django.urls import reverse + +from addon_service.common.aiohttp_session import ( + USER_AGENT, + close_singleton_client_session, + get_singleton_client_session, +) + + +class TestOAuth2CallbackViewErrors(SimpleTestCase): + def _get_error(self, **params): + return self.client.get(reverse("oauth2-callback"), {"state": "1.n", **params}) + + def test_known_error_code(self): + _resp = self._get_error(error="access_denied", error_description="no thanks") + self.assertEqual(_resp.status_code, HTTPStatus.BAD_REQUEST) + self.assertIn("access_denied", _resp.content.decode()) + self.assertNotIn("no thanks", _resp.content.decode()) + + def test_error_params_are_not_reflected(self): + with self.assertLogs("addon_service.oauth2.views", "WARNING"): + _resp = self._get_error( + error="", + error_description="", + ) + self.assertEqual(_resp.status_code, HTTPStatus.BAD_REQUEST) + self.assertTrue(_resp["Content-Type"].startswith("text/plain")) + self.assertNotIn("