refactor(pds-core): extract OAuth client-id resolution from CSS middleware

packages/pds-core/src/__tests__/client-css-injection.test.ts

@@ -9,7 +9,7 @@ import {
 } from '../lib/client-css-injection.js'
 
 function mockLogger() {
-  return { info: vi.fn(), warn: vi.fn(), debug: vi.fn() }
+  return { info: vi.fn(), error: vi.fn(), warn: vi.fn(), debug: vi.fn() }
 }
 
 describe('shouldInjectClientCss', () => {
@@ -263,6 +263,37 @@ describe('createClientCssInjectionMiddleware', () => {
     expect(next).toHaveBeenCalledOnce()
   })
 
+  // request_uri is a short-lived bearer reference to the PAR entry, so a
+  // log line carrying its value is replayable by anyone reading the logs.
+  it('logs request_uri presence but never its value when PAR resolution fails', async () => {
+    const requestUri = 'urn:ietf:params:oauth:request_uri:secret-par-handle'
+    const logger = mockLogger()
+    const middleware = createClientCssInjectionMiddleware({
+      trustedClients: [trustedClient],
+      resolveClientMetadata: vi.fn(),
+      getClientCss: vi.fn(),
+      resolveClientIdFromRequestUri: vi
+        .fn()
+        .mockRejectedValue(new Error('boom')),
+      logger,
+    })
+    const req = {
+      method: 'GET',
+      path: '/oauth/authorize',
+      query: { request_uri: requestUri },
+    }
+    const { res } = createResponseDouble()
+    const next = vi.fn()
+
+    await middleware(req, res, next)
+
+    expect(logger.error).toHaveBeenCalledOnce()
+    const [context] = logger.error.mock.calls[0]
+    expect(context).toMatchObject({ hasRequestUri: true })
+    expect(JSON.stringify(context)).not.toContain(requestUri)
+    expect(next).toHaveBeenCalledOnce()
+  })
+
   // ─── Regression: ERR_HTTP_HEADERS_SENT crash ─────────────────────────
   //
   // @atproto/oauth-provider flushes response headers before calling

packages/pds-core/src/__tests__/oauth-request-context.test.ts

@@ -0,0 +1,83 @@
+import { describe, expect, it, vi } from 'vitest'
+import { resolveOAuthClientIdFromQuery } from '../lib/oauth-request-context.js'
+
+const CLIENT_ID = 'https://app.example/client.json'
+const REQUEST_URI = 'urn:ietf:params:oauth:request_uri:req-123'
+
+describe('resolveOAuthClientIdFromQuery', () => {
+  it('prefers an explicit client_id query parameter', async () => {
+    const resolver = vi.fn()
+
+    await expect(
+      resolveOAuthClientIdFromQuery({ client_id: CLIENT_ID }, resolver),
+    ).resolves.toBe(CLIENT_ID)
+    // The PAR round-trip is skipped entirely when the id is already present.
+    expect(resolver).not.toHaveBeenCalled()
+  })
+
+  it('prefers client_id even when a request_uri is also present', async () => {
+    const resolver = vi.fn().mockResolvedValue('https://other.example/c.json')
+
+    await expect(
+      resolveOAuthClientIdFromQuery(
+        { client_id: CLIENT_ID, request_uri: REQUEST_URI },
+        resolver,
+      ),
+    ).resolves.toBe(CLIENT_ID)
+    expect(resolver).not.toHaveBeenCalled()
+  })
+
+  it('falls back to resolving the PAR request_uri', async () => {
+    const resolver = vi.fn().mockResolvedValue(CLIENT_ID)
+
+    await expect(
+      resolveOAuthClientIdFromQuery({ request_uri: REQUEST_URI }, resolver),
+    ).resolves.toBe(CLIENT_ID)
+    expect(resolver).toHaveBeenCalledWith(REQUEST_URI)
+  })
+
+  it('returns undefined when the resolver finds no client for the request_uri', async () => {
+    const resolver = vi.fn().mockResolvedValue(undefined)
+
+    await expect(
+      resolveOAuthClientIdFromQuery({ request_uri: REQUEST_URI }, resolver),
+    ).resolves.toBeUndefined()
+  })
+
+  it('returns undefined when no resolver is supplied', async () => {
+    await expect(
+      resolveOAuthClientIdFromQuery({ request_uri: REQUEST_URI }),
+    ).resolves.toBeUndefined()
+  })
+
+  it('returns undefined for an empty query', async () => {
+    const resolver = vi.fn()
+
+    await expect(
+      resolveOAuthClientIdFromQuery({}, resolver),
+    ).resolves.toBeUndefined()
+    expect(resolver).not.toHaveBeenCalled()
+  })
+
+  it.each([
+    { name: 'non-string client_id', query: { client_id: 42 } },
+    { name: 'non-string request_uri', query: { request_uri: 42 } },
+  ])('ignores a $name', async ({ query }) => {
+    const resolver = vi.fn().mockResolvedValue(CLIENT_ID)
+
+    await expect(
+      resolveOAuthClientIdFromQuery(query, resolver),
+    ).resolves.toBeUndefined()
+    expect(resolver).not.toHaveBeenCalled()
+  })
+
+  it('propagates resolver rejections to the caller', async () => {
+    // Deliberate: the module leaves error handling to each middleware so
+    // they can choose their own logging and fallback behaviour.
+    const resolver = vi.fn().mockRejectedValue(new Error('PAR lookup failed'))
+
+    await expect(
+      resolveOAuthClientIdFromQuery({ request_uri: REQUEST_URI }, resolver),
+    ).rejects.toThrow('PAR lookup failed')
+  })
+})

packages/pds-core/src/lib/client-css-injection.ts

@@ -2,9 +2,14 @@ import { createHash } from 'node:crypto'
 
 import type { ClientMetadata } from '@certified-app/shared'
 import { DEFAULT_BRANDING_CSS } from './default-branding.js'
+import {
+  resolveOAuthClientIdFromQuery,
+  type ResolveClientIdFromRequestUri,
+} from './oauth-request-context.js'
 
 type LoggerLike = {
   info: (obj: object, msg: string) => void
+  error: (obj: object, msg: string) => void
   warn: (obj: object, msg: string) => void
   debug: (obj: object, msg: string) => void
 }
@@ -18,9 +23,7 @@ type ClientCssInjectionDeps = {
     trustedClients: string[],
   ) => string | null
   /** Resolve client_id from a PAR request_uri (optional). */
-  resolveClientIdFromRequestUri?: (
-    requestUri: string,
-  ) => Promise<string | undefined>
+  resolveClientIdFromRequestUri?: ResolveClientIdFromRequestUri
   logger: LoggerLike
 }
 
@@ -158,25 +161,19 @@ export function createClientCssInjectionMiddleware({
       return
     }
 
-    // Resolve client_id: it may be on the query string directly, or
-    // inside a PAR request_uri that needs to be looked up via the
-    // oauth-provider's request manager. PAR-based flows (the common
-    // case in ePDS) only carry request_uri on the query string.
-    let clientId =
-      typeof query.client_id === 'string' ? query.client_id : undefined
-    if (!clientId && resolveClientIdFromRequestUri) {
-      const requestUri =
-        typeof query.request_uri === 'string' ? query.request_uri : undefined
-      if (requestUri) {
-        try {
-          clientId = await resolveClientIdFromRequestUri(requestUri)
-        } catch (err) {
-          logger.warn(
-            { err, hasRequestUri: true },
-            'CSS middleware: failed to resolve client_id from request_uri',
-          )
-        }
-      }
+    let clientId: string | undefined
+    try {
+      clientId = await resolveOAuthClientIdFromQuery(
+        query,
+        resolveClientIdFromRequestUri,
+      )
+    } catch (err) {
+      // Log presence, not the value: request_uri is a short-lived bearer
+      // reference to the PAR entry, so a log line carrying it is replayable.
+      logger.error(
+        { err, hasRequestUri: typeof query.request_uri === 'string' },
+        'CSS middleware: failed to resolve client_id from request_uri',
+      )
     }
 
     // Resolve trusted-client css if applicable. Untrusted/unknown

packages/pds-core/src/lib/oauth-request-context.ts

@@ -0,0 +1,37 @@
+/**
+ * OAuth request-context helpers for pds-core response enrichment.
+ *
+ * OAuth authorize pages may receive either a direct `client_id` query
+ * parameter or only a PAR `request_uri`. This module owns the narrow
+ * resolution step from the current request query to the OAuth client id,
+ * while callers keep their feature-specific policy decisions local.
+ */
+
+export type OAuthRequestQuery = Record<string, unknown>
+
+export type ResolveClientIdFromRequestUri = (
+  requestUri: string,
+) => Promise<string | undefined>
+
+/**
+ * Resolve the OAuth client id visible from the current request query.
+ *
+ * Prefer an explicit `client_id` query parameter. When it is absent,
+ * optionally resolve the PAR `request_uri` through the provider request
+ * manager supplied by the caller. Resolver errors are intentionally left
+ * to the caller so each middleware can choose its own logging and fallback
+ * behaviour.
+ */
+export async function resolveOAuthClientIdFromQuery(
+  query: OAuthRequestQuery,
+  resolveClientIdFromRequestUri?: ResolveClientIdFromRequestUri,
+): Promise<string | undefined> {
+  if (typeof query.client_id === 'string') return query.client_id
+  if (!resolveClientIdFromRequestUri) return undefined
+
+  const requestUri =
+    typeof query.request_uri === 'string' ? query.request_uri : undefined
+  if (!requestUri) return undefined
+
+  return resolveClientIdFromRequestUri(requestUri)
+}