auth-providers: OIDC callback hardening (3 commits)

pods/authProviders/src/openid.ts

@@ -74,17 +74,62 @@ export function registerOpenid (
     })(ctx, next)
   })
 
-  router.get(
-    redirectURL,
-    async (ctx, next) => {
-      const state = safeParseAuthState(ctx.query?.state)
-      const branding = getBranding(brandings, state?.branding)
-
-      await passport.authenticate('oidc', {
-        failureRedirect: concatLink(branding?.front ?? frontUrl, '/login')
-      })(ctx, next)
-    },
-    async (ctx, next) => {
+  router.get(redirectURL, async (ctx, next) => {
+    const state = safeParseAuthState(ctx.query?.state)
+    const branding = getBranding(brandings, state?.branding)
+    const loginUrl = concatLink(branding?.front ?? frontUrl, '/login')
+
+    try {
+      // INSTRUMENTATION (Codex-approved): explicit-callback variant captures
+      // err/info/status that would otherwise be swallowed by the strategy.
+      // PRIVACY: never log raw code, raw state, tokens, or full ctx.state.user.
+      await new Promise<void>((resolve) => {
+        passport.authenticate('oidc', { failureRedirect: loginUrl }, (err: any, user: any, info: any, status: any) => {
+          // L-AUTH-2: keep only a terse error line in normal operation; the
+          // verbose diagnostics (errStack/sessionKeys/host/…) are enabled by
+          // OIDC_DEBUG so anonymous repeated invalid callbacks cannot flood logs.
+          const baseDiag = {
+            stage: 'oidc_callback',
+            hasErr: err != null,
+            errName: err?.name,
+            errMessage: err?.message,
+            statusCode: status,
+            hasUser: user != null
+          }
+          const diag =
+            process.env.OIDC_DEBUG === 'true'
+              ? {
+                  ...baseDiag,
+                  errStack: err?.stack,
+                  infoSummary: info?.message ?? String(info ?? ''),
+                  hasSession: ctx.session != null,
+                  sessionKeys: ctx.session != null ? Object.keys(ctx.session) : [],
+                  hasCookieHeader: ctx.request.headers.cookie != null,
+                  host: ctx.request.headers.host,
+                  forwardedProto: ctx.request.headers['x-forwarded-proto'],
+                  statePresent: typeof ctx.query?.state === 'string',
+                  stateLength: typeof ctx.query?.state === 'string' ? (ctx.query.state as string).length : 0,
+                  codePresent: typeof ctx.query?.code === 'string'
+                }
+              : baseDiag
+          if (err != null || user == null) {
+            measureCtx.error('OIDC callback failed', diag)
+          } else {
+            measureCtx.info('OIDC callback succeeded — entering handleProviderAuth', {
+              hasSession: ctx.session != null
+            })
+            ctx.state.user = user
+          }
+          resolve()
+        })(ctx, async () => {})
+      })
+
+      if (ctx.state.user == null) {
+        // Strategy failed; redirect explicitly so we never bubble a 500.
+        ctx.redirect(loginUrl + '?error=oidc_callback_failed')
+        return
+      }
+
       const email = ctx.state.user.email
       const verifiedEmail = (ctx.state.user.email_verified as boolean) ? email : ''
       const nameParts = (ctx.state.user.name ?? ctx.state.user.username ?? '').split(' ')
@@ -109,11 +154,46 @@ export function registerOpenid (
 
       if (redirectUrl !== '') {
         ctx.redirect(redirectUrl)
+      } else {
+        // L-AUTH-4: handleProviderAuth signals an unresolvable login with '' (e.g.
+        // no account + signup disabled). Terminate fail-closed with an explicit
+        // 302 to /login instead of leaving a hanging/empty response.
+        ctx.redirect(loginUrl + '?error=oidc_no_account')
       }
 
       await next()
+    } catch (err: any) {
+      // Permanent invalid-callback guard: ANY failure → 302 to /login, never 500.
+      // L-AUTH-2: verbose fields behind OIDC_DEBUG (see the callback diag above).
+      const baseDiag = {
+        stage: 'oidc_callback',
+        hasErr: true,
+        errName: err?.name,
+        errMessage: err?.message,
+        statusCode: undefined,
+        hasUser: ctx.state?.user != null
+      }
+      measureCtx.error(
+        'OIDC callback failed',
+        process.env.OIDC_DEBUG === 'true'
+          ? {
+              ...baseDiag,
+              errStack: err?.stack,
+              infoSummary: '',
+              hasSession: ctx.session != null,
+              sessionKeys: ctx.session != null ? Object.keys(ctx.session) : [],
+              hasCookieHeader: ctx.request.headers.cookie != null,
+              host: ctx.request.headers.host,
+              forwardedProto: ctx.request.headers['x-forwarded-proto'],
+              statePresent: typeof ctx.query?.state === 'string',
+              stateLength: typeof ctx.query?.state === 'string' ? (ctx.query.state as string).length : 0,
+              codePresent: typeof ctx.query?.code === 'string'
+            }
+          : baseDiag
+      )
+      ctx.redirect(loginUrl + '?error=oidc_callback_failed')
     }
-  )
+  })
 
   return { name, displayName }
 }

server/account/src/__tests__/adminEmails.test.ts

@@ -0,0 +1,155 @@
+//
+// Copyright © 2026 Hardcore Engineering Inc.
+//
+// Licensed under the Eclipse Public License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License. You may
+// obtain a copy of the License at https://www.eclipse.org/legal/epl-2.0
+//
+// SPDX-License-Identifier: EPL-2.0
+//
+// Coverage for admin.ts hardening: both env-set (parseAdminEmails) AND
+// lookup (isAdminEmail) must trim+lowercase. Today ADMIN_EMAILS is clean,
+// but a future env value like " Admin@Example.com " would silently break
+// admin detection without normalization on the env-side. Mongo backend
+// already has this hardening (collections/mongo.ts:1141-1147); this test
+// covers the equivalent for the shared helper.
+//
+
+import type * as AdminModuleType from '../admin'
+
+type AdminModule = typeof AdminModuleType
+
+/**
+ * Re-import admin.ts with a controlled ADMIN_EMAILS env value.
+ * admin.ts evaluates ADMIN_EMAILS at module-load, so each test scenario
+ * needs an isolated module load.
+ */
+function loadAdmin (envValue: string | undefined): AdminModule {
+  let mod: AdminModule | undefined
+  jest.isolateModules(() => {
+    if (envValue === undefined) {
+      delete process.env.ADMIN_EMAILS
+    } else {
+      process.env.ADMIN_EMAILS = envValue
+    }
+    mod = jest.requireActual<AdminModule>('../admin')
+  })
+  if (mod === undefined) throw new Error('admin module did not load')
+  return mod
+}
+
+describe('admin.ts — env parsing + lookup normalization', () => {
+  const originalEnv = process.env.ADMIN_EMAILS
+
+  afterAll(() => {
+    if (originalEnv === undefined) {
+      delete process.env.ADMIN_EMAILS
+    } else {
+      process.env.ADMIN_EMAILS = originalEnv
+    }
+  })
+
+  test('1. whitespace tolerance — env entry with leading/trailing spaces', () => {
+    const { isAdminEmail } = loadAdmin(' Admin@Example.com ')
+    expect(isAdminEmail('admin@example.com')).toBe(true)
+  })
+
+  test('2. case-insensitive env — uppercase env entry matches lowercase lookup', () => {
+    const { isAdminEmail } = loadAdmin('ADMIN@EXAMPLE.COM')
+    expect(isAdminEmail('admin@example.com')).toBe(true)
+  })
+
+  test('3. case-insensitive lookup — lowercase env matches uppercase lookup input', () => {
+    const { isAdminEmail } = loadAdmin('admin@example.com')
+    expect(isAdminEmail('ADMIN@EXAMPLE.COM')).toBe(true)
+  })
+
+  test('4. L-AUTH-3: no-@ entries are DROPPED by default (fail-closed) + warn', () => {
+    const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {})
+    const prevFlag = process.env.ADMIN_EMAILS_ALLOW_LOGIN_ID
+    delete process.env.ADMIN_EMAILS_ALLOW_LOGIN_ID
+    try {
+      const { isAdminEmail } = loadAdmin('foo,bar@@,baz')
+      // No-@ entries dropped; only the '@'-bearing entry survives.
+      expect(isAdminEmail('foo')).toBe(false)
+      expect(isAdminEmail('baz')).toBe(false)
+      expect(isAdminEmail('bar@@')).toBe(true)
+      expect(warnSpy).toHaveBeenCalledWith('ADMIN_EMAILS contains entries without "@" (DROPPED)', {
+        entries: ['foo', 'baz']
+      })
+    } finally {
+      warnSpy.mockRestore()
+      if (prevFlag === undefined) delete process.env.ADMIN_EMAILS_ALLOW_LOGIN_ID
+      else process.env.ADMIN_EMAILS_ALLOW_LOGIN_ID = prevFlag
+    }
+  })
+
+  test('4b. L-AUTH-3: injected logger sees DROPPED no-@ entries by default', () => {
+    // Direct parseAdminEmails call with custom logger — proves the
+    // injection point works (admin.ts has no MeasureContext at load time).
+    const prevFlag = process.env.ADMIN_EMAILS_ALLOW_LOGIN_ID
+    delete process.env.ADMIN_EMAILS_ALLOW_LOGIN_ID
+    try {
+      const { parseAdminEmails } = loadAdmin('')
+      const warn = jest.fn()
+      const set = parseAdminEmails('foo,alice@example.com,baz', { warn })
+      expect(set.has('foo')).toBe(false)
+      expect(set.has('alice@example.com')).toBe(true)
+      expect(set.has('baz')).toBe(false)
+      expect(set.size).toBe(1)
+      expect(warn).toHaveBeenCalledWith('ADMIN_EMAILS contains entries without "@" (DROPPED)', {
+        entries: ['foo', 'baz']
+      })
+    } finally {
+      if (prevFlag === undefined) delete process.env.ADMIN_EMAILS_ALLOW_LOGIN_ID
+      else process.env.ADMIN_EMAILS_ALLOW_LOGIN_ID = prevFlag
+    }
+  })
+
+  test('4c. L-AUTH-3: no-@ entries kept when ADMIN_EMAILS_ALLOW_LOGIN_ID=true (opt-in)', () => {
+    const prevFlag = process.env.ADMIN_EMAILS_ALLOW_LOGIN_ID
+    process.env.ADMIN_EMAILS_ALLOW_LOGIN_ID = 'true'
+    try {
+      const { parseAdminEmails } = loadAdmin('')
+      const warn = jest.fn()
+      const set = parseAdminEmails('foo,alice@example.com,baz', { warn })
+      expect(set.has('foo')).toBe(true)
+      expect(set.has('alice@example.com')).toBe(true)
+      expect(set.has('baz')).toBe(true)
+      expect(set.size).toBe(3)
+      expect(warn).toHaveBeenCalledWith(
+        'ADMIN_EMAILS contains entries without "@" (kept (ADMIN_EMAILS_ALLOW_LOGIN_ID=true))',
+        { entries: ['foo', 'baz'] }
+      )
+    } finally {
+      if (prevFlag === undefined) delete process.env.ADMIN_EMAILS_ALLOW_LOGIN_ID
+      else process.env.ADMIN_EMAILS_ALLOW_LOGIN_ID = prevFlag
+    }
+  })
+
+  test('5. empty env string — empty set, all lookups return false', () => {
+    const { isAdminEmail } = loadAdmin('')
+    expect(isAdminEmail('admin@example.com')).toBe(false)
+    expect(isAdminEmail('')).toBe(false)
+  })
+
+  test('6. unset env — empty set, all lookups return false', () => {
+    const { isAdminEmail } = loadAdmin(undefined)
+    expect(isAdminEmail('admin@example.com')).toBe(false)
+  })
+
+  test('7. multiple entries with mixed whitespace + case all normalized', () => {
+    const { isAdminEmail } = loadAdmin('alice@example.com, bob@example.org , Carol@example.io')
+    expect(isAdminEmail('alice@example.com')).toBe(true)
+    expect(isAdminEmail('bob@example.org')).toBe(true)
+    expect(isAdminEmail('carol@example.io')).toBe(true)
+    // Lookup also normalizes
+    expect(isAdminEmail('  Bob@Example.ORG  ')).toBe(true)
+  })
+
+  test('8. null/undefined lookup input — return false (no throw)', () => {
+    const { isAdminEmail } = loadAdmin('admin@example.com')
+    expect(isAdminEmail(null)).toBe(false)
+    expect(isAdminEmail(undefined)).toBe(false)
+  })
+})

server/account/src/admin.ts

@@ -12,8 +12,59 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 //
-const ADMIN_EMAILS = new Set(process.env.ADMIN_EMAILS?.split(',') ?? [])
 
-export function isAdminEmail (email: string): boolean {
-  return ADMIN_EMAILS.has(email.trim())
+interface AdminEmailsLogger {
+  warn?: (msg: string, attrs?: Record<string, unknown>) => void
+}
+
+/**
+ * Parse the ADMIN_EMAILS env value into a normalized Set.
+ *
+ * - split on ','
+ * - trim() each entry
+ * - toLowerCase() each entry
+ * - drop empty entries
+ * - L-AUTH-3: entries without '@' are DROPPED by default (a typo like
+ *   ADMIN_EMAILS=admin,michel@… would otherwise mint an unintended admin
+ *   identifier). Deployments that intentionally use login-id style values must
+ *   opt in with ADMIN_EMAILS_ALLOW_LOGIN_ID=true; a warning is always emitted
+ *   listing the affected entries.
+ */
+export function parseAdminEmails (envValue: string | undefined, logger?: AdminEmailsLogger): Set<string> {
+  if (envValue == null || envValue === '') return new Set()
+  const allowLoginId = process.env.ADMIN_EMAILS_ALLOW_LOGIN_ID === 'true'
+  const entries = envValue
+    .split(',')
+    .map((s) => s.trim().toLowerCase())
+    .filter((s) => s.length > 0)
+  const invalid: string[] = []
+  const kept: string[] = []
+  for (const entry of entries) {
+    if (entry.includes('@')) {
+      kept.push(entry)
+    } else if (allowLoginId) {
+      kept.push(entry)
+      invalid.push(entry) // kept, but still warn for visibility
+    } else {
+      invalid.push(entry) // fail-closed: dropped
+    }
+  }
+  if (invalid.length > 0) {
+    const warn =
+      logger?.warn ??
+      ((msg: string, attrs?: Record<string, unknown>) => {
+        // eslint-disable-next-line no-console
+        console.warn(msg, attrs)
+      })
+    const action = allowLoginId ? 'kept (ADMIN_EMAILS_ALLOW_LOGIN_ID=true)' : 'DROPPED'
+    warn(`ADMIN_EMAILS contains entries without "@" (${action})`, { entries: invalid })
+  }
+  return new Set(kept)
+}
+
+const ADMIN_EMAILS = parseAdminEmails(process.env.ADMIN_EMAILS)
+
+export function isAdminEmail (email: string | null | undefined): boolean {
+  if (email == null || email === '') return false
+  return ADMIN_EMAILS.has(email.trim().toLowerCase())
 }