Merge pull request #89 from hypercerts-org/fix/preview-cache-and-debug-endpoints

Author: aspiers

.changeset/preview-cache-and-debug-endpoints.md

@@ -0,0 +1,17 @@
+---
+'ePDS': patch
+---
+
+Fix two preview-route cache bugs and remove long-stale debug endpoints.
+
+**Affects:** Client app developers, Operators
+
+**Client app developers:**
+
+- Preview-route fetch failures no longer poison the shared client-metadata cache. Previously, a failed preview fetch for a `client_id` with a valid 10-minute entry would overwrite that entry with a 60-second branding-less fallback, silently dropping `branding.css` on real OAuth flows for up to a minute. The in-memory cache is now only written by real-flow resolution.
+- The auth-service HTML preview pages (`/preview/login`, `/preview/login-otp`, `/preview/choose-handle`, `/preview/choose-handle-picker`, `/preview/recovery`, `/preview/recovery-otp`, and the `/preview` index) now send `Cache-Control: no-store`. Without it, a browser refresh could serve a cached page and never ask the server for fresh `branding.css`, breaking the advertised "edit `branding.css`, refresh the preview page" workflow.
+- `/preview/validate` now flags `branding.css` whose escaped size exceeds the 32 KB injection limit as an error, instead of reporting `ok` and letting the developer discover later that their CSS was silently dropped on real OAuth flows. Byte counts now match `getClientCss()`'s measurement (escaped UTF-8).
+
+**Operators:**
+
+- Removed `/_internal/debug-grants` and `/_internal/debug-recent-accounts`. These were added as temporary HYPER-270 debugging endpoints with a code comment marking them for removal before PR #21 shipped (v0.2.2); they survived through v0.2.2, v0.3.0, v0.4.0, and the pending v0.5.0. The matching env var `EPDS_DEBUG_GRANTS` is no longer read.

packages/auth-service/src/routes/preview.ts

@@ -95,6 +95,13 @@ function queryString(req: Request, name: string): string | undefined {
 
 function sendHtml(res: Response, html: string): void {
   res.setHeader('Content-Type', 'text/html; charset=utf-8')
+  // Preview flows server-side bypass the client-metadata cache, but that
+  // only helps if the browser actually asks for the page. Without
+  // no-store, heuristic caching (RFC 9111 §4.2.2) lets the browser serve
+  // the previous HTML on refresh, so fresh branding.css never reaches
+  // the client. The JSON endpoints already set this header; HTML was
+  // the missing piece.
+  res.setHeader('Cache-Control', 'no-store')
   res.send(html)
 }
 

packages/pds-core/src/index.ts

@@ -759,93 +759,6 @@ async function main() {
     }
   })
 
-  // TEMPORARY debug endpoint for HYPER-270 investigation.
-  //
-  // Dumps all authorized_client and account_device rows for a given DID so
-  // we can observe which grants were (or weren't) persisted when a user
-  // clicks Authorize on the upstream consent UI for an untrusted client.
-  //
-  // Gated on EPDS_DEBUG_GRANTS=1 in addition to the usual
-  // EPDS_INTERNAL_SECRET check so it only comes online in environments
-  // where we've explicitly enabled it. Remove this endpoint and its env
-  // var before merging PR #21.
-  if (process.env.EPDS_DEBUG_GRANTS === '1') {
-    pds.app.get('/_internal/debug-grants', async (req, res) => {
-      if (!verifyInternalSecret(req.headers['x-internal-secret'])) {
-        res.status(401).json({ error: 'Unauthorized' })
-        return
-      }
-      const sub = ((req.query.sub as string) || '').trim()
-      if (!sub) {
-        res.status(400).json({ error: 'Missing sub' })
-        return
-      }
-      try {
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any -- @atproto/pds accountManager internals not exported
-        const am = pds.ctx.accountManager as any
-        const db = am.db
-
-        const authorizedClients = await db.db
-          .selectFrom('authorized_client')
-          .select(['did', 'clientId', 'createdAt', 'updatedAt', 'data'])
-          .where('did', '=', sub)
-          .execute()
-
-        const accountDevices = await db.db
-          .selectFrom('account_device')
-          .select(['did', 'deviceId', 'createdAt', 'updatedAt'])
-          .where('did', '=', sub)
-          .execute()
-
-        res.json({ sub, authorizedClients, accountDevices })
-      } catch (err) {
-        logger.error({ err, sub }, 'debug-grants query failed')
-        res.status(500).json({ error: 'query_failed', message: String(err) })
-      }
-    })
-
-    // Also expose a "recent accounts" lookup so we can find the sub of the
-    // e2e test user without knowing its DID up front.
-    pds.app.get('/_internal/debug-recent-accounts', async (req, res) => {
-      if (!verifyInternalSecret(req.headers['x-internal-secret'])) {
-        res.status(401).json({ error: 'Unauthorized' })
-        return
-      }
-      const limit = Math.min(100, Math.max(1, Number(req.query.limit) || 5))
-      try {
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-        const am = pds.ctx.accountManager as any
-        const db = am.db
-
-        // Join actor with account to pull the email too, so we can
-        // cross-reference e2e scenario test emails (e.g. the
-        // "approved-untrusted-<ts>@example.com" pattern) against the
-        // DID in the debug-grants lookup.
-        const rows = await db.db
-          .selectFrom('actor')
-          .leftJoin('account', 'account.did', 'actor.did')
-          .select([
-            'actor.did as did',
-            'actor.handle as handle',
-            'actor.createdAt as createdAt',
-            'account.email as email',
-          ])
-          .orderBy('actor.createdAt', 'desc')
-          .limit(limit)
-          .execute()
-
-        res.json({ rows })
-      } catch (err) {
-        logger.error({ err }, 'debug-recent-accounts query failed')
-        res.status(500).json({ error: 'query_failed', message: String(err) })
-      }
-    })
-
-    logger.warn(
-      'EPDS_DEBUG_GRANTS=1: /_internal/debug-grants and /_internal/debug-recent-accounts endpoints are enabled (HYPER-270 debugging)',
-    )
-  }
-
   // Protected internal endpoint for auth service to reset the inactivity timer
   // on a pending PAR request_uri. Called when the user loads the handle selection
   // page so the request doesn't expire while they are choosing a handle.

packages/shared/src/__tests__/client-metadata.test.ts

@@ -83,6 +83,30 @@ describe('resolveClientMetadata', () => {
     expect(first).toEqual(second)
     expect(first.client_name).toBe('Cached App')
   })
+
+  it('noCache:true + fetch failure does not poison a valid cache entry', async () => {
+    // Regression test for a bug where preview flows that failed to
+    // fetch client metadata would overwrite a real flow's 10-minute
+    // cache entry with a 60-second branding-less fallback, silently
+    // dropping CSS on real OAuth flows for up to a minute.
+    const url = 'https://127.0.0.1/client-metadata.json' // NOSONAR — testing SSRF guard
+    _seedClientMetadataCacheForTest(url, {
+      client_name: 'Seeded App',
+      branding: { css: 'body { background: red; }' },
+    })
+    // safeFetch rejects 127.0.0.1 as a non-unicast host, so this fetch
+    // fails deterministically. The preview-style call returns the
+    // fallback metadata to its caller (so the preview page still
+    // renders), but must NOT clobber the seeded cache entry.
+    const previewResult = await resolveClientMetadata(url, { noCache: true })
+    expect(previewResult.client_name).toBe('127.0.0.1')
+    expect(previewResult.branding).toBeUndefined()
+    // A subsequent real-flow call (noCache defaulted to false) must
+    // still see the original seeded entry — not a poisoned fallback.
+    const realFlowResult = await resolveClientMetadata(url)
+    expect(realFlowResult.client_name).toBe('Seeded App')
+    expect(realFlowResult.branding?.css).toBe('body { background: red; }')
+  })
 })
 
 describe('resolveClientName', () => {

packages/shared/src/__tests__/preview-validation.test.ts

@@ -83,6 +83,32 @@ describe('validateClientMetadataForPreview', () => {
     expect(byId['trusted-client'].severity).toBe('ok')
   })
 
+  it('flags branding.css as error when escaped size exceeds the 32 KB limit', async () => {
+    // getClientCss measures *escaped* bytes against MAX_CSS_BYTES and
+    // silently returns null above it. The validator must mirror that
+    // check, not a raw-byte check — otherwise a developer whose CSS is
+    // just under the raw limit but over it after escapeCss() still sees
+    // "ok" here and gets their CSS silently dropped on real flows.
+    //
+    // Fixture construction: exactly MAX_CSS_BYTES (32 768) raw bytes,
+    // composed entirely of `</style>` occurrences that escapeCss()
+    // rewrites to `\u003c/style>` (+5 bytes each). Raw = 4096 × 8 =
+    // 32 768 (at the limit, passes a naive raw check); escaped =
+    // 4096 × 13 = 53 248 (over).
+    const url = 'https://heavy.example/client-metadata.json'
+    const bigCss = '</style>'.repeat(4096)
+    expect(bigCss.length).toBe(32_768)
+    mockFetchOnce({
+      client_id: url,
+      redirect_uris: ['https://heavy.example/cb'],
+      branding: { css: bigCss },
+    })
+    const result = await validateClientMetadataForPreview(url, [url])
+    const byId = Object.fromEntries(result.checks.map((c) => [c.id, c]))
+    expect(byId['branding-css'].severity).toBe('error')
+    expect(byId['branding-css'].detail).toContain('exceeds')
+  })
+
   it('warns (not errors) when optional branding fields are missing', async () => {
     const url = 'https://plain.example/client-metadata.json'
     mockFetchOnce({

packages/shared/src/client-metadata.ts

@@ -146,7 +146,7 @@ export async function resolveClientMetadata(
         { clientId, status: res.status },
         'Client metadata fetch returned non-OK status; using fallback',
       )
-      return fallback(clientId)
+      return fallback(clientId, options.noCache === true)
     }
 
     const metadata = (await res.json()) as ClientMetadata
@@ -167,18 +167,26 @@ export async function resolveClientMetadata(
       { err, clientId },
       'Client metadata fetch failed; using fallback',
     )
-    return fallback(clientId)
+    return fallback(clientId, options.noCache === true)
   }
 }
 
-function fallback(clientId: string): ClientMetadata {
+function fallback(clientId: string, skipCache: boolean): ClientMetadata {
   const name = extractDomain(clientId)
   const metadata = { client_name: name || undefined }
-  // Cache failures briefly (1 minute) to avoid hammering
-  cache.set(clientId, {
-    metadata,
-    expiresAt: Date.now() + 60_000,
-  })
+  // Cache failures briefly (1 minute) to avoid hammering — but only
+  // when called from a real flow. `noCache:true` callers (preview
+  // flows) skip the cache read and skip writing this degraded
+  // fallback entry; a successful fetch further up still writes to the
+  // cache, per the `noCache` JSDoc. Without this guard, a failing
+  // preview fetch would overwrite a valid 10-minute entry with a
+  // branding-less 60-second one, silently dropping CSS on real flows.
+  if (!skipCache) {
+    cache.set(clientId, {
+      metadata,
+      expiresAt: Date.now() + 60_000,
+    })
+  }
   return metadata
 }
 
@@ -202,7 +210,7 @@ export function escapeCss(css: string): string {
 }
 
 /** Maximum allowed size for injected CSS. Values above this are dropped. */
-const MAX_CSS_BYTES = 32_768 // 32 KB
+export const MAX_CSS_BYTES = 32_768 // 32 KB
 
 /**
  * Returns escaped CSS for injection if the client is trusted, or null.

packages/shared/src/index.ts

@@ -40,6 +40,7 @@ export {
   resolveClientMetadata,
   resolveClientName,
   escapeCss,
+  MAX_CSS_BYTES,
   getClientCss,
   clearClientMetadataCache,
   getClientMetadataCacheStatus,

packages/shared/src/preview-validation.ts

@@ -11,7 +11,11 @@
  * fetch, then a flat list of content checks.
  */
 
-import type { ClientMetadata } from './client-metadata.js'
+import {
+  escapeCss,
+  MAX_CSS_BYTES,
+  type ClientMetadata,
+} from './client-metadata.js'
 import { escapeHtml } from './html.js'
 import { makeSafeFetch } from './safe-fetch.js'
 
@@ -332,14 +336,30 @@ function checkBrandingCss(metadata: ClientMetadata): PreviewCheck {
   const labelHtml = `${code('branding.css')} present`
 
   if (typeof cssString === 'string' && cssString.trim().length > 0) {
-    const bytes = new TextEncoder().encode(cssString).byteLength
+    // Mirror getClientCss's size check: it measures the escaped form
+    // (each `</style>` → `\u003c/style>`, +5 bytes) against
+    // MAX_CSS_BYTES and silently returns null when it's over.
+    // Reporting the raw byte count here would tell devs their CSS is
+    // fine up to 32 KB raw when in fact it gets dropped on real flows.
+    const escaped = escapeCss(cssString)
+    const bytes = Buffer.byteLength(escaped, 'utf8')
+    if (bytes > MAX_CSS_BYTES) {
+      return {
+        id: 'branding-css',
+        label,
+        severity: 'error',
+        detail: `${bytes.toLocaleString()} bytes (escaped) exceeds the ${MAX_CSS_BYTES.toLocaleString()}-byte limit. getClientCss() will silently drop it on real OAuth flows.`,
+        labelHtml,
+        detailHtml: `${bytes.toLocaleString()} bytes (escaped) exceeds the ${MAX_CSS_BYTES.toLocaleString()}-byte limit. ${code('getClientCss()')} will silently drop it on real OAuth flows.`,
+      }
+    }
     return {
       id: 'branding-css',
       label,
       severity: 'ok',
-      detail: `${bytes.toLocaleString()} bytes. Injected into /preview/consent (pds-core) and the auth-service pages when the client is trusted.`,
+      detail: `${bytes.toLocaleString()} bytes (escaped). Injected into /preview/consent (pds-core) and the auth-service pages when the client is trusted.`,
       labelHtml,
-      detailHtml: `${bytes.toLocaleString()} bytes. Injected into ${code('/preview/consent')} (pds-core) and the auth-service pages when the client is trusted.`,
+      detailHtml: `${bytes.toLocaleString()} bytes (escaped). Injected into ${code('/preview/consent')} (pds-core) and the auth-service pages when the client is trusted.`,
     }
   }
   if (cssString !== undefined) {