improve UI, fix bearer token expiry

Author: Tsche

README.md

@@ -99,18 +99,20 @@ replaces them with GitHub's absolute counts. Counter responses use `no-cache`, s
 browsers revalidate with this service; that does not imply a GitHub request while
 the relevant snapshot remains fresh.
 
-The browser runtime keeps the last counter snapshot in local storage for up to
-seven days. Consumers can render it synchronously while the API request is in
-flight, avoiding a flash of zero counters. Snapshots contain only the site/resource
-key, discussion node ID, counts, and save time; authentication tokens are not part
-of this cache. Storage is optional and failures fall back to the network normally.
+The browser runtime keeps the last counter snapshot and the last confirmed viewer
+vote state in local storage for up to seven days. Consumers can render them
+synchronously while the API request is in flight, avoiding a flash of zero counters
+or unselected vote buttons. Snapshots contain only the site/resource key, discussion
+node ID, counts, viewer state, and save time; authentication tokens are not part of
+this cache. Storage is optional and failures fall back to the network normally.
 
 Operational logs are emitted at GitHub boundaries rather than for every HTTP
 request. Reaction-refresh lines include the site, trigger (`requested` or
 `sweep`), batch size, updated-row count, and duration. Failures include safe
 GitHub status/request IDs where available. Discussion discovery/creation, OAuth
-failures, vote failures, startup, and sweep summaries are also logged. Client
-IPs, origins, resource URLs, authorization codes, and tokens are not logged.
+failures, rejected creation grants, one-off GitHub App bearer retries, vote
+failures, startup, and sweep summaries are also logged. Client IPs, origins,
+resource URLs, authorization codes, and tokens are not logged.
 
 ## Local checks
 

runtime/src/api/client.ts

@@ -1,19 +1,23 @@
 import { validateResourceId } from "../feedback/resources.js";
 import type { Resource } from "../feedback/resources.js";
-import type { Vote, VoteResult } from "../protocol/github.js";
+import type { ViewerVote, Vote, VoteResult } from "../protocol/github.js";
 
-export interface ReactionState {
+interface CounterState {
   id: string | null;
   up: number;
   down: number;
   age: number;
   stale: boolean;
 }
 
+export interface ReactionState extends CounterState {
+  viewer: ViewerVote;
+}
+
 interface ReactionEnvelope {
   v: 1;
   site: string;
-  items: Record<string, ReactionState>;
+  items: Record<string, CounterState>;
 }
 
 export interface ClientOptions {
@@ -86,9 +90,10 @@ export class FeedbackClient {
     const result = new Map<string, ReactionState>();
     for (const key of normalized) {
       const state = payload.items[key] ?? emptyState();
-      if (!isReactionState(state)) throw new TypeError("Invalid feedback service response");
-      result.set(key, state);
-      this.#storeReaction(key, state);
+      if (!isCounterState(state)) throw new TypeError("Invalid feedback service response");
+      const combined = { ...state, viewer: this.#cachedReaction(key)?.viewer ?? "none" };
+      result.set(key, combined);
+      this.#storeReaction(key, combined);
     }
     return result;
   }
@@ -168,6 +173,7 @@ export class FeedbackClient {
       down: result.down,
       age: 0,
       stale: true,
+      viewer: result.viewer,
     });
     return result;
   }
@@ -239,7 +245,7 @@ export class FeedbackError extends Error {
 }
 
 function emptyState(): ReactionState {
-  return { id: null, up: 0, down: 0, age: 0, stale: false };
+  return { id: null, up: 0, down: 0, age: 0, stale: false, viewer: "none" };
 }
 
 function isEnvelope(value: unknown, site: string): value is ReactionEnvelope {
@@ -248,7 +254,7 @@ function isEnvelope(value: unknown, site: string): value is ReactionEnvelope {
   return candidate.v === 1 && candidate.site === site && !!candidate.items && typeof candidate.items === "object";
 }
 
-function isReactionState(value: unknown): value is ReactionState {
+function isCounterState(value: unknown): value is CounterState {
   if (!value || typeof value !== "object") return false;
   const state = value as Partial<ReactionState>;
   return (
@@ -260,6 +266,14 @@ function isReactionState(value: unknown): value is ReactionState {
   );
 }
 
+function isReactionState(value: unknown): value is ReactionState {
+  return isCounterState(value) && isViewerVote((value as Partial<ReactionState>).viewer);
+}
+
+function isViewerVote(value: unknown): value is ViewerVote {
+  return value === "up" || value === "down" || value === "both" || value === "none";
+}
+
 function counterKey(site: string, resource: string): string {
   return `cppsocial.feedback.v1.${site}.reaction.${resource}`;
 }

runtime/src/example/page.ts

@@ -41,7 +41,7 @@ function renderCached(): void {
     const card = cards.get(key);
     if (!card) continue;
     card.discussionId = state.id;
-    render(card, state.up, state.down);
+    render(card, state.up, state.down, state.viewer);
   }
   status.textContent = "Showing saved counts while updating…";
 }
@@ -54,7 +54,7 @@ async function refresh(): Promise<void> {
       const state = states.get(key);
       if (!state) throw new Error(`Missing reaction state for ${key}`);
       card.discussionId = state.id;
-      render(card, state.up, state.down);
+      render(card, state.up, state.down, state.viewer);
       stale ||= state.stale;
     }
     status.textContent = stale ? "Some cached counts could not be refreshed." : "Ready.";

runtime/test/client.test.ts

@@ -121,9 +121,18 @@ void test("last counter values are available before the network responds", async
     apiOrigin: "https://feedback-api.cpp.social",
     site: "cpp-social",
     counterStorage: storage,
-    fetch: () => Promise.resolve(Response.json(response)),
+    fetch: (input) => Promise.resolve(Response.json(
+      (input instanceof Request ? input.url : input.toString()).endsWith("/votes")
+        ? { v: 1, up: 15, down: 2, viewer: "up" }
+        : response,
+    )),
   });
   await first.reactions(["article"]);
+  await first.vote(
+    "article",
+    "up",
+    { value: "ghu_user", expiresAt: 2_000_000_000, creationGrant: "grant" },
+  );
   const reloaded = new FeedbackClient({
     apiOrigin: "https://feedback-api.cpp.social",
     site: "cpp-social",
@@ -135,7 +144,8 @@ void test("last counter values are available before the network responds", async
 
   assert.ok(cached);
   assert.equal(cached.id, "D_article");
-  assert.equal(cached.up, 14);
+  assert.equal(cached.up, 15);
   assert.equal(cached.down, 2);
   assert.equal(cached.stale, true);
+  assert.equal(cached.viewer, "up");
 });

src/feedback/api/routes.py

@@ -166,7 +166,12 @@ async def ensure_discussion(request: Request) -> Response:
         discussion = await container.discussions.ensure(
             site, container.databases[site.id], body.resource
         )
-    except GrantError:
+    except GrantError as exc:
+        oauth_logger.warning(
+            "Discussion creation grant rejected: reason=%s site=%s",
+            str(exc),
+            site.id,
+        )
         raise ApiError("invalid_creation_grant", "Authentication must be restarted.", 401) from None
     except DiscussionError as exc:
         raise ApiError("discussion_invalid", str(exc), 400) from exc

src/feedback/protocol/github/client.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import asyncio
+import logging
 import time
 from collections.abc import Callable, Mapping
 from dataclasses import dataclass
@@ -10,6 +11,8 @@
 import httpx
 import jwt
 
+logger = logging.getLogger("feedback.github")
+
 
 class GitHubError(RuntimeError):
     def __init__(
@@ -97,15 +100,15 @@ async def graphql(
 
     async def _request_installation_token(self, installation_id: int) -> InstallationToken:
         self._raise_if_rate_limited()
-        async with self._requests:
-            response = await self._http.post(
-                f"https://api.github.com/app/installations/{installation_id}/access_tokens",
-                headers={
-                    "Accept": "application/vnd.github+json",
-                    "Authorization": f"Bearer {self.app_jwt()}",
-                    "X-GitHub-Api-Version": "2022-11-28",
-                },
+        response = await self._installation_token_request(installation_id)
+        if response.status_code == 401:
+            logger.warning(
+                "GitHub App bearer token rejected; retrying once: status=401 "
+                "github_request_id=%s installation_id=%s",
+                response.headers.get("x-github-request-id"),
+                installation_id,
             )
+            response = await self._installation_token_request(installation_id)
         if response.status_code != 201:
             raise self._error(response, "installation_token_failed")
         self._rate_limit_failures = 0
@@ -120,6 +123,22 @@ async def _request_installation_token(self, installation_id: int) -> Installatio
             raise _response_error(response, "github_malformed_response") from exc
         return InstallationToken(value, expires_at)
 
+    async def _installation_token_request(self, installation_id: int) -> httpx.Response:
+        async with self._requests:
+            try:
+                return await self._http.post(
+                    f"https://api.github.com/app/installations/{installation_id}/access_tokens",
+                    headers={
+                        "Accept": "application/vnd.github+json",
+                        "Authorization": f"Bearer {self.app_jwt()}",
+                        "X-GitHub-Api-Version": "2022-11-28",
+                    },
+                )
+            except httpx.TimeoutException as exc:
+                raise GitHubError("github_timeout") from exc
+            except httpx.RequestError as exc:
+                raise GitHubError("github_transport_error") from exc
+
     async def _graphql_request(
         self, token: str, query: str, variables: Mapping[str, object]
     ) -> httpx.Response:

src/feedback/service/oauth_state.py

@@ -111,13 +111,13 @@ def __init__(
         self,
         key: bytes,
         *,
-        lifetime_seconds: int = 300,
+        lifetime_seconds: int = 8 * 60 * 60,
         clock: Callable[[], float] = time.time,
     ) -> None:
         if len(key) < 32:
             raise ValueError("grant signing key must contain at least 32 bytes")
-        if not 60 <= lifetime_seconds <= 600:
-            raise ValueError("grant lifetime must be from 60 through 600 seconds")
+        if not 60 <= lifetime_seconds <= 8 * 60 * 60:
+            raise ValueError("grant lifetime must be from 60 through 28800 seconds")
         self._key = hmac.digest(key, b"discussion-creation-grant", "sha256")
         self._lifetime = lifetime_seconds
         self._clock = clock
@@ -168,7 +168,7 @@ def verify(self, token: str, *, site: str, origin: str) -> None:
             or not isinstance(value["exp"], int)
             or value["iat"] > now + 30
             or value["exp"] < now
-            or value["exp"] - value["iat"] > 600
+            or value["exp"] - value["iat"] > 8 * 60 * 60
         ):
             raise GrantError("invalid creation grant")
 

tests/test_discussions.py

@@ -82,7 +82,9 @@ async def test_ensure_rejects_unconfigured_canonical_origin(config: Config, tmp_
         )
 
 
-def test_ensure_endpoint_requires_an_origin_bound_grant(config: Config) -> None:
+def test_ensure_endpoint_requires_an_origin_bound_grant(
+    config: Config, caplog: pytest.LogCaptureFixture
+) -> None:
     grants = CreationGrantSigner(b"k" * 32, clock=lambda: 1_000)
     app = create_app(
         config,
@@ -106,19 +108,23 @@ def test_ensure_endpoint_requires_an_origin_bound_grant(config: Config) -> None:
                 "grant": grant,
             },
         )
-        rejected = client.post(
-            "/v1/sites/cpp-social/discussions/ensure",
-            headers={"Origin": "https://cpp.social"},
-            json={
-                "key": "feedback/other",
-                "url": "https://cpp.social/other/",
-                "grant": grant[:-1] + ("A" if grant[-1] != "A" else "B"),
-            },
-        )
+        with caplog.at_level("WARNING", logger="feedback.oauth"):
+            rejected = client.post(
+                "/v1/sites/cpp-social/discussions/ensure",
+                headers={"Origin": "https://cpp.social"},
+                json={
+                    "key": "feedback/other",
+                    "url": "https://cpp.social/other/",
+                    "grant": grant[:-1] + ("A" if grant[-1] != "A" else "B"),
+                },
+            )
 
     assert response.status_code == 200
     assert response.json() == {"v": 1, "id": "D_example", "number": 7}
     assert rejected.status_code == 401
+    assert "Discussion creation grant rejected" in caplog.text
+    assert "site=cpp-social" in caplog.text
+    assert grant not in caplog.text
 
 
 @pytest.mark.asyncio

tests/test_github.py

@@ -56,6 +56,40 @@ def handler(request: httpx.Request) -> httpx.Response:
     assert requests == 1
 
 
+@pytest.mark.asyncio
+async def test_installation_token_retries_one_rejected_app_bearer(
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    requests = 0
+
+    def handler(request: httpx.Request) -> httpx.Response:
+        nonlocal requests
+        requests += 1
+        if requests == 1:
+            return httpx.Response(
+                401,
+                headers={"X-GitHub-Request-Id": "request-123"},
+                json={"message": "A JSON web token could not be decoded"},
+            )
+        return httpx.Response(
+            201,
+            json={"token": "ghs_secret", "expires_at": "2030-01-01T00:00:00Z"},
+        )
+
+    async with httpx.AsyncClient(transport=httpx.MockTransport(handler)) as http:
+        client = GitHubClient(
+            app_id=123, private_key=pem_private_key(), http=http, clock=lambda: 1_000
+        )
+        with caplog.at_level("WARNING", logger="feedback.github"):
+            token = await client.installation_token(7)
+
+    assert token == "ghs_secret"
+    assert requests == 2
+    assert "GitHub App bearer token rejected; retrying once" in caplog.text
+    assert "github_request_id=request-123" in caplog.text
+    assert "JSON web token" not in caplog.text
+
+
 @pytest.mark.asyncio
 async def test_graphql_refreshes_once_after_unauthorized() -> None:
     token_requests = 0

tests/test_security.py

@@ -68,7 +68,7 @@ def test_signed_state_expires() -> None:
         signer.verify(token, site="cpp-social", origin="https://cpp.social", verifier=VERIFIER)
 
 
-def test_creation_grant_is_short_lived_and_origin_bound() -> None:
+def test_creation_grant_matches_login_lifetime_and_is_origin_bound() -> None:
     now = 1_000
     signer = CreationGrantSigner(b"k" * 32, clock=lambda: now)
     grant = signer.issue(site="cpp-social", origin="https://cpp.social", nonce=NONCE)
@@ -77,5 +77,7 @@ def test_creation_grant_is_short_lived_and_origin_bound() -> None:
     with pytest.raises(GrantError):
         signer.verify(grant, site="cpp-social", origin="https://other.example")
     now = 1_301
+    signer.verify(grant, site="cpp-social", origin="https://cpp.social")
+    now = 1_000 + 8 * 60 * 60 + 1
     with pytest.raises(GrantError):
         signer.verify(grant, site="cpp-social", origin="https://cpp.social")