[ENG-12178] Fix Mendeley OAuth: handle callback errors, retry Cloudflare-blocked requests

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]
 

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)

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="<script>alert(1)</script>",
+                error_description="<img src=x onerror=alert(2)>",
+            )
+        self.assertEqual(_resp.status_code, HTTPStatus.BAD_REQUEST)
+        self.assertTrue(_resp["Content-Type"].startswith("text/plain"))
+        self.assertNotIn("<script", _resp.content.decode())
+        self.assertNotIn("<img", _resp.content.decode())
+
+
+class TestSingletonClientSession(IsolatedAsyncioTestCase):
+    async def test_identifying_user_agent(self):
+        _session = await get_singleton_client_session()
+        try:
+            self.assertEqual(_session.headers["User-Agent"], USER_AGENT)
+        finally:
+            await close_singleton_client_session()