From 40666e1cc5921baebaec0d49535a028ce99cfd1f Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Wed, 17 Jun 2026 20:24:01 -0400 Subject: [PATCH] fix(19): BL-04 keep context oidcIss/oidcSub null for local users (no fabricated identity sentinels) --- apps/api/src/auth/devBypass.ts | 25 +++++++++++++--- apps/api/src/auth/localAuthMiddleware.ts | 19 +++++++----- .../tests/auth/localAuthMiddleware.test.ts | 30 +++++++++++++++++++ 3 files changed, 62 insertions(+), 12 deletions(-) diff --git a/apps/api/src/auth/devBypass.ts b/apps/api/src/auth/devBypass.ts index ee71159..2f2671b 100644 --- a/apps/api/src/auth/devBypass.ts +++ b/apps/api/src/auth/devBypass.ts @@ -44,15 +44,32 @@ export const DEV_USER = { color: COLOR_PALETTE[0], // '#4A90D9' — first palette slot } as const; +/** + * The shape stored on c.get('user') across the bypass, local-session, and OIDC paths. + * + * BL-04: oidcIss/oidcSub are NULLABLE. Local users have null OIDC fields, and + * localAuthMiddleware must NOT fabricate sentinel ('local'/String(id)) values — those + * share the uniqueness domain (uniq_oidc_identity) with real OIDC identities and could + * collide with a genuine (iss,sub) pair if ever persisted. DEV_USER carries non-null + * 'dev'/'dev-user' values and remains assignable to this widened shape. + */ +export interface ContextUser { + id: number; + oidcIss: string | null; + oidcSub: string | null; + displayName: string | null; + color: string; +} + /** * Extend Hono's ContextVariableMap so that c.get('user') / c.set('user', ...) - * are statically typed throughout the app. The value type is the DEV_USER shape, - * which is compatible with both the bypass path and any future app-level user object - * stored on context (they share the same id/displayName/color subset). + * are statically typed throughout the app. The value type is ContextUser — the shape + * shared by the dev-bypass path, the local-session path (nullable oidc fields), and any + * future app-level user object stored on context. */ declare module 'hono' { interface ContextVariableMap { - user: typeof DEV_USER; + user: ContextUser; } } diff --git a/apps/api/src/auth/localAuthMiddleware.ts b/apps/api/src/auth/localAuthMiddleware.ts index b952137..65befe9 100644 --- a/apps/api/src/auth/localAuthMiddleware.ts +++ b/apps/api/src/auth/localAuthMiddleware.ts @@ -33,7 +33,7 @@ import { eq } from 'drizzle-orm'; import { db } from '../db/client.js'; import { users } from '../db/schema.js'; import { verifyLocalSessionCookie } from './localSession.js'; -import type { DEV_USER } from './devBypass.js'; +import type { ContextUser } from './devBypass.js'; /** * Returns a Hono MiddlewareHandler that: @@ -81,17 +81,20 @@ export function localAuthMiddleware(): MiddlewareHandler { return; } - // Populate c.get('user') with the same shape as DEV_USER (devBypass.ts ContextVariableMap). - // oidcIss/oidcSub: local users have nullable oidcIss/oidcSub — use fallback strings so the - // shape is compatible with typeof DEV_USER at runtime. Cast required because ContextVariableMap - // is narrowed to the const DEV_USER literal type. + // Populate c.get('user') with the ContextUser shape (devBypass.ts ContextVariableMap). + // BL-04: keep oidcIss/oidcSub as NULL for local users — do NOT fabricate + // 'local'/String(id) sentinels. Those values share the uniq_oidc_identity uniqueness + // domain with real OIDC identities, so persisting them (e.g. a future upsertUser call + // using these context values) would let two local users collide or a local user shadow + // a genuine OIDC identity. ContextUser widens oidcIss/oidcSub to string | null so no + // cast is needed. c.set('user', { id: row.id, - oidcIss: row.oidcIss ?? 'local', - oidcSub: row.oidcSub ?? String(row.id), + oidcIss: row.oidcIss ?? null, + oidcSub: row.oidcSub ?? null, displayName: row.displayName ?? null, color: row.color ?? '#4A90D9', - } as typeof DEV_USER); + } satisfies ContextUser); await next(); }; diff --git a/apps/api/tests/auth/localAuthMiddleware.test.ts b/apps/api/tests/auth/localAuthMiddleware.test.ts index 4dc9590..6bba8ab 100644 --- a/apps/api/tests/auth/localAuthMiddleware.test.ts +++ b/apps/api/tests/auth/localAuthMiddleware.test.ts @@ -155,6 +155,36 @@ describe('localAuthMiddleware', () => { expect(u.color).toBe('#FF5733'); }); + it('Test 1c (BL-04): local user with null DB oidc fields → context oidcIss/oidcSub are NULL (no fabricated sentinels)', async () => { + mockVerifyResult = 9; + mockDbSelectResult.push({ + id: 9, + oidcIss: null, // local user — no OIDC identity bound + oidcSub: null, + displayName: 'Local Member', + color: '#22AA88', + }); + + const localAuthMiddleware = await getMiddleware(); + const app = new Hono(); + let capturedUser: unknown; + + app.use('/api/*', localAuthMiddleware()); + app.get('/api/test', (c) => { + capturedUser = c.get('user'); + return c.json({ ok: true }); + }); + + const res = await app.request('/api/test'); + expect(res.status).toBe(200); + const u = capturedUser as { id: number; oidcIss: string | null; oidcSub: string | null }; + expect(u.id).toBe(9); + // BL-04: must be null — NOT 'local' / String(id) sentinels that share the + // uniq_oidc_identity domain with real OIDC identities. + expect(u.oidcIss).toBeNull(); + expect(u.oidcSub).toBeNull(); + }); + it('Test 2: no cookie → pure passthrough; c.get("user") remains unset (Pitfall-1 guard)', async () => { mockVerifyResult = null; // No cookie / invalid