fix oauth exchange bug

Author: Tsche

src/feedback/api/routes.py

@@ -2,6 +2,7 @@
 
 import hashlib
 import json
+import logging
 from typing import cast
 
 from starlette.requests import Request
@@ -23,6 +24,9 @@
 from feedback.service.oauth_state import GrantError, StateError
 from feedback.service.runtime import FeedbackRuntime
 
+oauth_logger = logging.getLogger("feedback.oauth")
+github_logger = logging.getLogger("feedback.github")
+
 
 async def homepage(request: Request) -> Response:
     callback = services(request).config.service.oauth_callback
@@ -114,6 +118,16 @@ async def oauth_exchange(request: Request) -> Response:
     except StateError:
         raise ApiError("invalid_oauth_state", "Authorization must be restarted.", 400) from None
     except OAuthError as exc:
+        oauth_logger.warning(
+            "GitHub OAuth exchange failed: reason=%s status=%s upstream_code=%s "
+            "github_request_id=%s site=%s origin=%s",
+            exc.reason,
+            exc.status,
+            exc.upstream_code,
+            exc.request_id,
+            site.id,
+            origin,
+        )
         status = 502 if exc.code == "oauth_exchange_ambiguous" else 400
         raise ApiError(exc.code, "Authorization must be restarted.", status) from exc
     if container.grants is None:
@@ -151,6 +165,15 @@ async def ensure_discussion(request: Request) -> Response:
     except DiscussionError as exc:
         raise ApiError("discussion_invalid", str(exc), 400) from exc
     except GitHubError as exc:
+        github_logger.warning(
+            "GitHub discussion request failed: code=%s status=%s github_request_id=%s "
+            "site=%s origin=%s",
+            exc.code,
+            exc.status,
+            exc.request_id,
+            site.id,
+            origin,
+        )
         raise ApiError("github_unavailable", "GitHub is temporarily unavailable.", 502) from exc
     return cors(
         JSONResponse(

src/feedback/app.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import logging
 import os
 import stat
 import time
@@ -32,6 +33,8 @@
 from feedback.service.reaction_cache import ReactionRefresher
 from feedback.service.runtime import FeedbackRuntime
 
+logger = logging.getLogger("feedback.runtime")
+
 
 def create_app(
     config: Config | None = None,
@@ -88,6 +91,11 @@ async def lifespan(application: Starlette) -> AsyncIterator[None]:
             discussions=resolved_discussions,
         )
         application.state.services = services
+        logger.info(
+            "Feedback service started: public_origin=%s sites=%s",
+            loaded.service.public_origin,
+            ",".join(sorted(loaded.sites)),
+        )
         try:
             yield
         finally:

src/feedback/protocol/github/oauth.py

@@ -11,9 +11,21 @@
 
 
 class OAuthError(RuntimeError):
-    def __init__(self, code: str) -> None:
+    def __init__(
+        self,
+        code: str,
+        *,
+        reason: str,
+        status: int | None = None,
+        upstream_code: str | None = None,
+        request_id: str | None = None,
+    ) -> None:
         super().__init__(code)
         self.code = code
+        self.reason = reason
+        self.status = status
+        self.upstream_code = upstream_code
+        self.request_id = request_id
 
 
 @dataclass(frozen=True, slots=True)
@@ -80,23 +92,51 @@ async def exchange(
                 },
             )
         except httpx.TimeoutException as exc:
-            raise OAuthError("oauth_exchange_ambiguous") from exc
-        if response.status_code != 200:
-            raise OAuthError("oauth_exchange_failed")
+            raise OAuthError("oauth_exchange_ambiguous", reason="timeout") from exc
+        except httpx.RequestError as exc:
+            raise OAuthError("oauth_exchange_failed", reason="transport_error") from exc
         try:
             body = response.json()
         except ValueError as exc:
-            raise OAuthError("oauth_exchange_failed") from exc
-        if not isinstance(body, dict) or body.get("error"):
-            raise OAuthError("oauth_exchange_failed")
+            raise OAuthError(
+                "oauth_exchange_failed",
+                reason="invalid_json",
+                status=response.status_code,
+                request_id=response.headers.get("x-github-request-id"),
+            ) from exc
+        if not isinstance(body, dict):
+            raise OAuthError(
+                "oauth_exchange_failed",
+                reason="invalid_payload",
+                status=response.status_code,
+                request_id=response.headers.get("x-github-request-id"),
+            )
+        upstream_code = body.get("error")
+        if response.status_code != 200 or isinstance(upstream_code, str):
+            raise OAuthError(
+                "oauth_exchange_failed",
+                reason="upstream_rejected",
+                status=response.status_code,
+                upstream_code=upstream_code if isinstance(upstream_code, str) else None,
+                request_id=response.headers.get("x-github-request-id"),
+            )
         token = body.get("access_token")
         expires_in = body.get("expires_in")
+        # GitHub Apps may disable expiring user access tokens. Such responses do
+        # not include expires_in; keep our browser session bounded to eight hours.
+        if expires_in is None:
+            expires_in = 8 * 60 * 60
         if (
             not isinstance(token, str)
             or not token.startswith("ghu_")
             or isinstance(expires_in, bool)
             or not isinstance(expires_in, int)
             or not 1 <= expires_in <= 8 * 60 * 60
         ):
-            raise OAuthError("oauth_exchange_failed")
+            raise OAuthError(
+                "oauth_exchange_failed",
+                reason="invalid_token_payload",
+                status=response.status_code,
+                request_id=response.headers.get("x-github-request-id"),
+            )
         return AccessToken(token, int(self._clock()) + expires_in), verified_state

src/feedback/service/runtime.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import asyncio
+import logging
 from collections.abc import Callable
 from dataclasses import dataclass, field
 
@@ -11,6 +12,8 @@
 from feedback.service.oauth_state import CreationGrantSigner
 from feedback.service.reaction_cache import ReactionRefresher
 
+logger = logging.getLogger("feedback.runtime")
+
 
 @dataclass(slots=True)
 class FeedbackRuntime:
@@ -56,4 +59,10 @@ async def close(self) -> None:
 
 def _consume_task(task: asyncio.Task[int]) -> None:
     if not task.cancelled():
-        task.exception()
+        error = task.exception()
+        if error is not None:
+            logger.warning(
+                "Background reaction refresh failed: error_type=%s error=%s",
+                type(error).__name__,
+                error,
+            )

tests/test_oauth.py

@@ -1,6 +1,7 @@
 from urllib.parse import parse_qs, urlsplit
 
 import httpx
+import pytest
 from starlette.testclient import TestClient
 
 from feedback.app import create_app
@@ -58,6 +59,84 @@ def handler(request: httpx.Request) -> httpx.Response:
     assert exchanged["repository_id"] == ["R_repo"]
 
 
+def test_exchange_bounds_nonexpiring_github_token_to_eight_hours(config: Config) -> None:
+    http = httpx.AsyncClient(
+        transport=httpx.MockTransport(
+            lambda _: httpx.Response(200, json={"access_token": "ghu_user"})
+        )
+    )
+    oauth = OAuthClient(
+        client_id="Iv1.client",
+        client_secret="client-secret",
+        callback_url=config.service.oauth_callback,
+        signer=StateSigner(b"k" * 32, clock=lambda: 1_000),
+        http=http,
+        clock=lambda: 1_000,
+    )
+    grants = CreationGrantSigner(b"k" * 32, clock=lambda: 1_000)
+    with TestClient(create_app(config, clock=lambda: 1_000, oauth=oauth, grants=grants)) as client:
+        authorize = client.post(
+            "/v1/sites/cpp-social/oauth/authorize",
+            headers={"Origin": "https://cpp.social"},
+            json={"challenge": pkce_challenge(VERIFIER), "nonce": NONCE},
+        )
+        exchange = client.post(
+            "/v1/sites/cpp-social/oauth/exchange",
+            headers={"Origin": "https://cpp.social"},
+            json={
+                "code": "temporary-code",
+                "state": authorize.json()["state"],
+                "verifier": VERIFIER,
+            },
+        )
+
+    assert exchange.status_code == 200
+    assert exchange.json()["expires_at"] == 29_800
+
+
+def test_exchange_logs_safe_upstream_failure_details(
+    config: Config, caplog: pytest.LogCaptureFixture
+) -> None:
+    http = httpx.AsyncClient(
+        transport=httpx.MockTransport(
+            lambda _: httpx.Response(
+                200,
+                headers={"x-github-request-id": "request-123"},
+                json={"error": "bad_verification_code", "error_description": "secret detail"},
+            )
+        )
+    )
+    oauth = OAuthClient(
+        client_id="Iv1.client",
+        client_secret="client-secret",
+        callback_url=config.service.oauth_callback,
+        signer=StateSigner(b"k" * 32, clock=lambda: 1_000),
+        http=http,
+        clock=lambda: 1_000,
+    )
+    grants = CreationGrantSigner(b"k" * 32, clock=lambda: 1_000)
+    with TestClient(create_app(config, clock=lambda: 1_000, oauth=oauth, grants=grants)) as client:
+        authorize = client.post(
+            "/v1/sites/cpp-social/oauth/authorize",
+            headers={"Origin": "https://cpp.social"},
+            json={"challenge": pkce_challenge(VERIFIER), "nonce": NONCE},
+        )
+        response = client.post(
+            "/v1/sites/cpp-social/oauth/exchange",
+            headers={"Origin": "https://cpp.social"},
+            json={
+                "code": "temporary-code",
+                "state": authorize.json()["state"],
+                "verifier": VERIFIER,
+            },
+        )
+
+    assert response.status_code == 400
+    assert "upstream_code=bad_verification_code" in caplog.text
+    assert "github_request_id=request-123" in caplog.text
+    assert "secret detail" not in caplog.text
+
+
 def test_oauth_rejects_origin_content_type_and_duplicate_fields(config: Config) -> None:
     oauth = OAuthClient(
         client_id="Iv1.client",