fix(auth): stop reading an unreachable Keycloak as "no registered redirect URIs"

The redirect_uri check is fail-closed on an empty allowlist, and the cache handed
it an empty allowlist for two very different reasons: Keycloak said the client has
no URIs, or Keycloak could not be asked at all. fetchClientConfig caught every
error and returned `{ redirectUris: [] }`, so an admin-API failure came back out
of /auth/authorize as

  400 redirect_uri does not match a registered redirect URI for this client

blaming the app's configuration for an outage on ours. Both call sites already
had the right branch for this — authorize and the callback handler answer a
THROWN lookup with `Unable to validate redirect_uri` and an error-level log — but
the cache swallowed the throw, so neither branch could ever fire. This is the
same shape as the WAF incident recorded in keycloak-stack.ts.

Worse, the empty result was cached for the full 5-minute TTL, so a single hiccup
rejected every launch for five minutes.

The lookup now reports found / absent / unavailable. Only found and absent are
cached, absent on a short 30s TTL because absence usually means "not created
yet" (the compliance workflow deletes and recreates its client mid-run).
getRegisteredRedirectUris throws on unavailable; getSmartClientConfig stays
lenient because token-time enrichment only reads patientFacing and must not fail
a token issue over an unreachable admin API.

Also drops a duplicated KcAdminClient construction in favour of the existing
getAdminClient factory, which exists precisely to be a test seam, and moves the
cache behind createClientConfigCache so the new test drives it by injection —
a sibling test mock.modules this whole module, and bun's module mocks are
process-global.

Backend 1389 pass / 31 pre-existing fail (same 31 without this diff), typecheck,
lint.

Author: quotentiroler

.github/workflows/smart-compliance-tests.yml

@@ -1,3 +1,6 @@
+# SPDX-FileCopyrightText: Max Health Inc.
+# SPDX-License-Identifier: AGPL-3.0-or-later OR LicenseRef-Commercial
+
 name: SMART Compliance Tests (Inferno)
 
 on:
@@ -473,20 +476,30 @@ jobs:
         if: steps.config.outputs.target != 'deployed'
         run: |
           cd backend
-          bun run start &
+          # Log to a file: a backgrounded process's stdout stops being captured
+          # when this step ends, which left every later failure undiagnosable.
+          bun run start > /tmp/backend.log 2>&1 &
           APP_PID=$!
           echo "app_pid=$APP_PID" >> $GITHUB_ENV
           
           # Wait for app to be ready
           echo "Waiting for application..."
+          READY=false
           for i in {1..30}; do
             if curl -sf http://localhost:8445/health; then
               echo "Application is ready!"
+              READY=true
               break
             fi
             echo "Attempt $i/30..."
             sleep 2
           done
+
+          if [ "$READY" != "true" ]; then
+            echo "ERROR: application never became healthy — backend log follows"
+            cat /tmp/backend.log || true
+            exit 1
+          fi
         env:
           NODE_ENV: production
           KEYCLOAK_BASE_URL: http://localhost:8080
@@ -1371,12 +1384,12 @@ jobs:
             echo "(Deployed mode — container logs not available from CI runner.)"
             echo "Check VPS logs: docker logs proxy-smart-backend-beta --tail 200 2>&1 | grep -iE 'token|auth|error|warn'"
           else
-            # Local CI: dump from Docker containers
+            # Local CI: the backend runs as a bare process, not a container.
             echo "--- Backend logs (auth-related) ---"
-            docker logs backend 2>&1 | grep -iE 'token endpoint|keycloak token|forwarding to keycloak|error response|warn|400|401|422|500' | tail -100 || echo "(no backend container)"
+            grep -iE 'redirect_uri|registered redirect|token endpoint|keycloak|error|warn|400|401|422|500' /tmp/backend.log | tail -100 || echo "(nothing matched)"
             echo ""
-            echo "--- Backend logs (last 50 lines) ---"
-            docker logs backend 2>&1 | tail -50 || echo "(no backend container)"
+            echo "--- Backend logs (last 80 lines) ---"
+            tail -80 /tmp/backend.log || echo "(no /tmp/backend.log)"
             echo ""
             echo "--- Keycloak logs (auth-related, last 50 lines) ---"
             docker logs keycloak 2>&1 | grep -iE 'token|grant|client|error|warn' | tail -50 || echo "(no keycloak container)"
@@ -1422,6 +1435,7 @@ jobs:
           
           # Also save service logs (only exists for local targets)
           docker logs keycloak > test-results/keycloak.log 2>&1 || true
+          cp /tmp/backend.log test-results/backend.log 2>/dev/null || true
 
       - name: Upload Artifacts
         if: always()

backend/src/lib/smart-client-config-cache.ts

@@ -11,9 +11,8 @@
  * Avoids hitting Keycloak admin API on every token exchange.
  */
 
-import KcAdminClient from '@keycloak/keycloak-admin-client'
 import { isCimdClientId, resolveCimdRedirectUris, type SmartProxyLogger } from '@proxy-smart/auth'
-import { config } from '@/config'
+import { getAdminClient } from '@/lib/kc-admin-factory'
 import { logger } from '@/lib/logger'
 
 /** The lib takes a flat logger; adapt our structured one once, here. */
@@ -36,31 +35,101 @@ export interface SmartClientConfig {
   redirectUris: string[]
 }
 
+/**
+ * The three distinguishable outcomes of asking Keycloak about a client.
+ *
+ * `unavailable` exists because collapsing it into `absent` is what turns a
+ * Keycloak hiccup into "this client has no registered redirect URIs", and the
+ * redirect_uri check is fail-closed on an empty allowlist — so an infrastructure
+ * failure came out as a client-configuration error and rejected every launch.
+ */
+export type ClientLookup =
+  | { status: 'found'; config: SmartClientConfig }
+  | { status: 'absent' }
+  | { status: 'unavailable'; reason: string }
+
+/** How the cache reaches the client directory. Injectable so tests need no module mocking. */
+export type ClientLookupSource = (clientId: string) => Promise<ClientLookup>
+
 interface CacheEntry {
   config: SmartClientConfig
   expiresAt: number
 }
 
 const DEFAULT_TTL_MS = 5 * 60 * 1000 // 5 minutes
+/** Absence is usually "not created yet", so re-ask soon (admin create/recreate races). */
+const ABSENT_TTL_MS = 30 * 1000
 
-const cache = new Map<string, CacheEntry>()
+const EMPTY_CONFIG: SmartClientConfig = { redirectUris: [] }
 
 /**
- * Get the SMART client config for a given clientId.
- * Returns cached value if available; otherwise fetches from Keycloak.
+ * Build a caching client-config reader over a lookup source.
+ *
+ * Exported so tests can drive the caching and failure semantics through a fake
+ * source. `mock.module` is process-global in bun, so a sibling test that mocks
+ * this whole module would otherwise make these paths untestable.
  */
-export async function getSmartClientConfig(clientId: string): Promise<SmartClientConfig> {
-  const now = Date.now()
-  const cached = cache.get(clientId)
+export function createClientConfigCache(source: ClientLookupSource) {
+  const cache = new Map<string, CacheEntry>()
+
+  /** A failed lookup is NEVER cached — that would stretch one hiccup across the whole TTL. */
+  async function lookup(clientId: string): Promise<ClientLookup> {
+    const now = Date.now()
+    const cached = cache.get(clientId)
+
+    if (cached && now < cached.expiresAt) {
+      return { status: 'found', config: cached.config }
+    }
+
+    const result = await source(clientId)
+
+    if (result.status === 'found') {
+      cache.set(clientId, { config: result.config, expiresAt: now + DEFAULT_TTL_MS })
+    } else if (result.status === 'absent') {
+      cache.set(clientId, { config: EMPTY_CONFIG, expiresAt: now + ABSENT_TTL_MS })
+    }
+
+    return result
+  }
+
+  /** Lenient reader: see getSmartClientConfig. */
+  async function getSmartClientConfig(clientId: string): Promise<SmartClientConfig> {
+    const result = await lookup(clientId)
+    return result.status === 'found' ? result.config : EMPTY_CONFIG
+  }
 
-  if (cached && now < cached.expiresAt) {
-    return cached.config
+  /** Strict reader: see getRegisteredRedirectUris. */
+  async function getRegisteredRedirectUris(clientId: string): Promise<string[]> {
+    if (!clientId) return []
+    if (isCimdClientId(clientId)) {
+      return resolveCimdRedirectUris(clientId, { logger: smartLogger })
+    }
+    const result = await lookup(clientId)
+    if (result.status === 'unavailable') {
+      throw new Error(`Cannot read registered redirect URIs for "${clientId}": ${result.reason}`)
+    }
+    return result.status === 'found' ? result.config.redirectUris : []
   }
 
-  // Fetch from Keycloak
-  const fetched = await fetchClientConfig(clientId)
-  cache.set(clientId, { config: fetched, expiresAt: now + DEFAULT_TTL_MS })
-  return fetched
+  return {
+    getSmartClientConfig,
+    getRegisteredRedirectUris,
+    invalidate: (clientId: string) => cache.delete(clientId),
+    clear: () => cache.clear(),
+  }
+}
+
+const defaultCache = createClientConfigCache(fetchClientConfig)
+
+/**
+ * Get the SMART client config for a given clientId.
+ *
+ * Lenient by design: token-time enrichment only reads `patientFacing`, and an
+ * unreachable Keycloak must not stop a token being issued. Callers that need a
+ * trustworthy allowlist use `getRegisteredRedirectUris`, which fails loudly.
+ */
+export async function getSmartClientConfig(clientId: string): Promise<SmartClientConfig> {
+  return defaultCache.getSmartClientConfig(clientId)
 }
 
 /**
@@ -81,54 +150,47 @@ export async function getSmartClientConfig(clientId: string): Promise<SmartClien
  * client registered. An earlier attempt special-cased CIMD at the interception
  * site instead, which silently delegated an authorization-server MUST to Keycloak.
  *
- * Returns an empty array for unknown clients, an unverifiable metadata document,
- * or an unavailable Keycloak — the caller treats an empty allowlist as "reject
- * every redirect_uri" (fail-closed).
+ * Returns an empty array for an unknown client or an unverifiable metadata
+ * document — the caller treats an empty allowlist as "reject every redirect_uri"
+ * (fail-closed).
+ *
+ * THROWS when Keycloak could not be asked at all. That is not the same as "this
+ * client has no registered URIs", and the callers act on the difference: both
+ * the authorize interceptor and the callback handler already answer a thrown
+ * lookup with `Unable to validate redirect_uri` and an error-level log, instead
+ * of blaming the client's configuration for an outage on our side.
  *
  * Wired into `@proxy-smart/auth`'s `getRegisteredRedirectUris` dependency.
  */
 export async function getRegisteredRedirectUris(clientId: string): Promise<string[]> {
-  if (!clientId) return []
-  if (isCimdClientId(clientId)) {
-    return resolveCimdRedirectUris(clientId, { logger: smartLogger })
-  }
-  const { redirectUris } = await getSmartClientConfig(clientId)
-  return redirectUris
+  return defaultCache.getRegisteredRedirectUris(clientId)
 }
 
 /**
  * Invalidate cache for a specific client (call after admin updates).
  */
 export function invalidateClientConfig(clientId: string): void {
-  cache.delete(clientId)
+  defaultCache.invalidate(clientId)
 }
 
 /**
  * Clear the entire client config cache.
  */
 export function clearClientConfigCache(): void {
-  cache.clear()
+  defaultCache.clear()
 }
 
-async function fetchClientConfig(clientId: string): Promise<SmartClientConfig> {
-  if (!config.keycloak.isConfigured || !config.keycloak.adminClientId || !config.keycloak.adminClientSecret) {
-    return { redirectUris: [] }
-  }
-
+async function fetchClientConfig(clientId: string): Promise<ClientLookup> {
   try {
-    const admin = new KcAdminClient({
-      baseUrl: config.keycloak.baseUrl!,
-      realmName: config.keycloak.realm!,
-    })
-    await admin.auth({
-      grantType: 'client_credentials',
-      clientId: config.keycloak.adminClientId,
-      clientSecret: config.keycloak.adminClientSecret,
-    })
+    const admin = await getAdminClient()
+    if (!admin) {
+      return { status: 'unavailable', reason: 'Keycloak admin credentials are not configured' }
+    }
 
     const clients = await admin.clients.find({ clientId, max: 1 })
     if (!clients || clients.length === 0) {
-      return { redirectUris: [] }
+      logger.auth.warn('Keycloak has no such client', { clientId })
+      return { status: 'absent' }
     }
 
     const attrs = clients[0].attributes || {}
@@ -139,12 +201,10 @@ async function fetchClientConfig(clientId: string): Promise<SmartClientConfig> {
 
     const redirectUris = Array.isArray(clients[0].redirectUris) ? clients[0].redirectUris : []
 
-    return { patientFacing, redirectUris }
+    return { status: 'found', config: { patientFacing, redirectUris } }
   } catch (error) {
-    logger.auth.warn('Failed to fetch client config from Keycloak', {
-      clientId,
-      error: error instanceof Error ? error.message : 'Unknown error',
-    })
-    return { redirectUris: [] }
+    const reason = error instanceof Error ? error.message : 'Unknown error'
+    logger.auth.error('Cannot reach Keycloak to read client config', { clientId, error: reason })
+    return { status: 'unavailable', reason }
   }
 }

backend/test/registered-redirect-uris-lookup.test.ts

@@ -0,0 +1,90 @@
+// SPDX-FileCopyrightText: Max Health Inc.
+// SPDX-License-Identifier: AGPL-3.0-or-later OR LicenseRef-Commercial
+
+/**
+ * The client-config cache must distinguish "this client has no registered
+ * redirect URIs" from "Keycloak could not be asked".
+ *
+ * The redirect_uri check is fail-closed on an empty allowlist, so flattening an
+ * unreachable Keycloak into `[]` made every SMART launch fail with
+ * "redirect_uri does not match a registered redirect URI for this client" —
+ * blaming the app's configuration for an outage on our side. Worse, the empty
+ * result was cached, so one hiccup rejected every launch for the whole TTL.
+ *
+ * Driven through createClientConfigCache rather than mock.module: bun's module
+ * mocks are process-global, and a sibling test mocks this whole module.
+ */
+import { describe, it, expect, beforeEach } from 'bun:test'
+import { createClientConfigCache, type ClientLookup } from '../src/lib/smart-client-config-cache'
+
+const CLIENT = 'inferno-test-client'
+const REDIRECT = 'http://localhost:4567/custom/smart_stu2_2/redirect'
+
+let mode: 'found' | 'absent' | 'unavailable' = 'found'
+let calls = 0
+
+function source(_clientId: string): Promise<ClientLookup> {
+  calls++
+  if (mode === 'unavailable') {
+    return Promise.resolve({ status: 'unavailable', reason: 'connect ECONNREFUSED 127.0.0.1:8080' })
+  }
+  if (mode === 'absent') return Promise.resolve({ status: 'absent' })
+  return Promise.resolve({
+    status: 'found',
+    config: { redirectUris: [REDIRECT], patientFacing: true },
+  })
+}
+
+let cache = createClientConfigCache(source)
+
+describe('client config cache — allowlist vs unavailable', () => {
+  beforeEach(() => {
+    cache = createClientConfigCache(source)
+    calls = 0
+    mode = 'found'
+  })
+
+  it('returns the URIs registered for a known client', async () => {
+    expect(await cache.getRegisteredRedirectUris(CLIENT)).toEqual([REDIRECT])
+  })
+
+  it('returns an empty allowlist for a client Keycloak does not have', async () => {
+    mode = 'absent'
+    expect(await cache.getRegisteredRedirectUris('no-such-client')).toEqual([])
+  })
+
+  it('throws instead of returning an empty allowlist when Keycloak is unreachable', async () => {
+    mode = 'unavailable'
+    await expect(cache.getRegisteredRedirectUris(CLIENT)).rejects.toThrow(
+      /Cannot read registered redirect URIs/,
+    )
+  })
+
+  it('does not cache an unreachable lookup, so recovery is immediate', async () => {
+    mode = 'unavailable'
+    await expect(cache.getRegisteredRedirectUris(CLIENT)).rejects.toThrow()
+
+    mode = 'found'
+    expect(await cache.getRegisteredRedirectUris(CLIENT)).toEqual([REDIRECT])
+  })
+
+  it('caches a successful lookup', async () => {
+    await cache.getRegisteredRedirectUris(CLIENT)
+    await cache.getRegisteredRedirectUris(CLIENT)
+    expect(calls).toBe(1)
+  })
+
+  it('re-asks after an absent client is invalidated (admin recreate)', async () => {
+    mode = 'absent'
+    expect(await cache.getRegisteredRedirectUris(CLIENT)).toEqual([])
+
+    cache.invalidate(CLIENT)
+    mode = 'found'
+    expect(await cache.getRegisteredRedirectUris(CLIENT)).toEqual([REDIRECT])
+  })
+
+  it('keeps token-time config lenient — no throw when Keycloak is unreachable', async () => {
+    mode = 'unavailable'
+    expect(await cache.getSmartClientConfig(CLIENT)).toEqual({ redirectUris: [] })
+  })
+})