fix(pds-core): friendly OAuth redirect when PAR has expired

The /oauth/epds-callback handler caught all failures with a single
res.json({error: 'Authentication failed'}). The catch did try to
redirect back to the client first, but it called requestManager.get
(requestUri) to fetch the redirect_uri — and that's the very call
whose failure brought us into the catch in the first place. By the
time we got there, @atproto/oauth-provider had already deleted the
expired PAR row inside RequestManager.get() (line 244 of upstream's
request-manager.js: expired rows throw AccessDeniedError AND are
deleted in the same call), so the recovery re-fetch threw too and
we fell through to raw JSON. A real user just hit this: they took
their time on the OTP step, the PAR died, and they ended up staring
at {"error":"Authentication failed"} on the PDS host with no way
to retry from there.

Two changes:

  1. Stash redirect_uri and state on the closure as soon as Step 2's
     requestManager.get() returns successfully. The catch block now
     reads from these captured values instead of attempting a doomed
     re-fetch. When the captures are empty (Step 2 itself threw —
     the PAR was already dead on entry), we fall through to the
     HTML page below.

  2. Render a styled HTML page via the existing renderError() helper
     when no redirect_uri is recoverable, instead of leaking JSON.
     Status: 400 for the expected expiry case (matching upstream's
     access_denied semantics), 500 for an unexpected one (matching
     server_error).

For PAR expiry specifically (the bug the user hit), the redirect now
carries error=access_denied and an error_description that reads
"Your sign-in took too long to complete and timed out. Please start
sign-in again." — readable, descriptive copy that an OAuth client
can show directly to the user. access_denied matches the error code
@atproto/oauth-provider already uses for PAR expiry on its own
paths (e.g. when a user is bounced through /oauth/authorize with an
expired request_uri), so clients see one error code regardless of
which path inside the AS surfaced the timeout. The user-friendly
text lives in error_description, which is where clients are
supposed to source human-readable copy from.

Turns @par-callback-error from RED to GREEN. Changeset describes
the user- and developer-visible behaviour change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: aspiers

.changeset/par-expiry-bridge-and-callback-leak.md

@@ -0,0 +1,11 @@
+---
+'ePDS': patch
+---
+
+Sign-in no longer fails with a raw JSON error page when a user takes too long on the OTP step.
+
+**Affects:** End users, OAuth client developers
+
+**End users:** Previously, if you took more than five minutes between requesting your one-time code and submitting it (a slow inbox, switching tabs, fishing the code out of spam, multiple Resend cycles), sign-in could fail with a blank page showing only `{"error": "Authentication failed"}` on the PDS host — even though your OTP code itself was still valid. You now get redirected back to the app you were signing into with a standard OAuth error and a human-readable message explaining that sign-in timed out, so the app can offer to start the flow again. If a redirect back to the app isn't possible, you see a styled error page on the PDS instead of the raw JSON.
+
+**OAuth client developers:** when a sign-in expires, `/oauth/epds-callback` now redirects to your registered `redirect_uri` with `error=access_denied`, `error_description` (a sentence explaining the timeout), `iss`, and `state` query parameters per RFC 6749 §4.1.2.1, mirroring the error code that `@atproto/oauth-provider` already uses for PAR expiry on its own paths. Apps that were checking for the previous JSON shape on this endpoint should switch to handling the standard OAuth error redirect, and source human-readable copy from `error_description` rather than from `error`.

packages/pds-core/src/index.ts

@@ -253,6 +253,18 @@ async function main() {
       return
     }
 
+    // Captured from Step 2's requestManager.get() — used by the catch
+    // block to redirect any later failure back to the client per RFC
+    // 6749 §4.1.2.1, even when the PAR row has since been deleted (in
+    // particular: RequestManager.get() deletes any expired row in the
+    // same call that throws AccessDeniedError, so by the time the
+    // catch block runs there's nothing left to re-read). Empty when
+    // Step 2 itself threw — i.e. the PAR was already dead on entry,
+    // the case the @par-callback-error scenario covers — and the
+    // catch falls through to a styled HTML page in that branch.
+    let capturedRedirectUri: string | undefined
+    let capturedState: string | undefined
+
     try {
       // Step 1: Load or create device session
       const deviceInfo = await provider.deviceManager.load(
@@ -293,6 +305,12 @@ async function main() {
       // eslint-disable-next-line @typescript-eslint/no-explicit-any -- @atproto/oauth-provider requestManager not exported
       const requestData = await (provider.requestManager as any).get(requestUri)
       const { clientId } = requestData
+      // Stash redirect_uri/state now while the PAR is alive — if a later
+      // step throws and the row has since been deleted (e.g. flushed
+      // post-success or the test-only delete-par hook), the catch block
+      // can still mount an RFC 6749 redirect to the client.
+      capturedRedirectUri = requestData?.parameters?.redirect_uri
+      capturedState = requestData?.parameters?.state
 
       // Step 3: Resolve or create the account.
       // Use the PDS accountManager directly — account.sqlite is the single source of truth.
@@ -555,33 +573,61 @@ async function main() {
     } catch (err) {
       logger.error({ err }, 'ePDS callback error')
 
-      // Try to redirect error back to client
-      try {
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any -- @atproto/oauth-provider requestManager not exported
-        const requestData = await (provider.requestManager as any).get(
-          requestUri,
+      // Distinguish a dead PAR ("This request has expired" /
+      // InvalidGrantError on a missing row) from a generic server
+      // failure: the former is a normal user-paced timeout that
+      // upstream @atproto/oauth-provider already uses
+      // error=access_denied for on its own paths, the latter is
+      // genuinely server_error per RFC 6749 §4.1.2.1. We follow the
+      // upstream precedent for the timeout case so an OAuth client
+      // sees the same `error` value regardless of which path inside
+      // the AS surfaced the expiry — the user-friendly explanation
+      // lives in error_description, which is where clients are
+      // supposed to source their human-readable copy from.
+      const errMsg = err instanceof Error ? err.message : String(err)
+      // Match the upstream-thrown messages for both flavours of dead
+      // PAR: "This request has expired" (AccessDeniedError, row was
+      // alive at request time but past its expiresAt) and "Unknown
+      // request_uri" (InvalidRequestError, row was already gone). We
+      // also keep a generic "expired" / "invalid_grant" catch-all in
+      // case upstream rewords its messages in a future patch release.
+      const isExpired =
+        /request has expired|unknown request_uri|invalid_grant|expired/i.test(
+          errMsg,
         )
-        const redirectUri = requestData?.parameters?.redirect_uri
-        if (redirectUri) {
-          const errorUrl = new URL(redirectUri)
-          errorUrl.searchParams.set('error', 'server_error')
-          errorUrl.searchParams.set(
-            'error_description',
-            'Authentication failed',
-          )
-          errorUrl.searchParams.set('iss', pdsUrl)
-          if (requestData.parameters.state) {
-            errorUrl.searchParams.set('state', requestData.parameters.state)
-          }
-          res.redirect(303, errorUrl.toString())
-          return
-        }
-      } catch {
-        // Fall through
+      const errorCode = isExpired ? 'access_denied' : 'server_error'
+      const errorDescription = isExpired
+        ? 'Your sign-in took too long to complete and timed out. Please start sign-in again.'
+        : 'Authentication failed.'
+
+      // We rely on the redirect_uri / state captured during Step 2's
+      // requestManager.get(). Don't re-fetch here: by the time we
+      // reach this branch the PAR may have just been deleted by the
+      // very call that threw above (RequestManager.get() deletes any
+      // expired row in the same call), so a second fetch would only
+      // ever throw the same error and give us no new information.
+      // When the captured values are empty (i.e. Step 2 itself threw
+      // before populating them — the PAR was already dead on entry),
+      // we fall through to the HTML fallback below.
+      if (!res.headersSent && capturedRedirectUri) {
+        const errorUrl = new URL(capturedRedirectUri)
+        errorUrl.searchParams.set('error', errorCode)
+        errorUrl.searchParams.set('error_description', errorDescription)
+        errorUrl.searchParams.set('iss', pdsUrl)
+        if (capturedState) errorUrl.searchParams.set('state', capturedState)
+        res.redirect(303, errorUrl.toString())
+        return
       }
 
+      // No redirect_uri to send the user back to. Render a styled HTML
+      // page on the PDS host instead of leaking JSON to the browser.
+      // Status: 400 for an expected expiry, 500 for an unexpected
+      // failure, matching the semantics of errorCode.
       if (!res.headersSent) {
-        res.status(500).json({ error: 'Authentication failed' })
+        res
+          .status(isExpired ? 400 : 500)
+          .type('html')
+          .send(renderError(errorDescription))
       }
     }
   })