From 63c64b0faf9a57ce55d6e92a172ef87e6c80bc3b Mon Sep 17 00:00:00 2001 From: Michael Uray Date: Tue, 23 Jun 2026 06:11:11 +0000 Subject: [PATCH 1/8] fix(authProviders): instrument OIDC callback + redirect on fail (not 500) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Production-blocking: every OIDC callback (bogus or real code) returned 500 Internal Server Error, even with valid sessions. No log was emitted between `try auth via openid` and the 500 — passport strategy threw silently before `handleProviderAuth` could log. Per Codex plan-review amendments: - Switch to explicit-callback passport.authenticate variant so strategy errors surface as err/info/status args instead of being swallowed. - Privacy-safe diagnostic shape (no raw code/state/tokens/user). Fields: stage, errMessage/Name/Stack, hasUser, infoSummary, statusCode, hasSession + sessionKeys (names only), cookie + state + code PRESENCE (lengths only), host, forwardedProto. - Permanent invalid-callback guard: ALL failures → 302 to /login with ?error=oidc_callback_failed, never 500. Same wrap covers handleProviderAuth throws. Image-tag: hardcoreeng/account:integration-wac-2026-06-21-codex-r7. Next step (P0-T2): user attempts login, we capture the actual error from the diagnostic, then ship a surgical fix in r8. Signed-off-by: Michael Uray Signed-off-by: Michael Uray --- pods/authProviders/src/openid.ts | 90 +++++++++++++++++++++++++++----- 1 file changed, 78 insertions(+), 12 deletions(-) diff --git a/pods/authProviders/src/openid.ts b/pods/authProviders/src/openid.ts index 0a19d99b288..6bfa5e56fa7 100644 --- a/pods/authProviders/src/openid.ts +++ b/pods/authProviders/src/openid.ts @@ -74,17 +74,61 @@ 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((resolve) => { + passport.authenticate( + 'oidc', + { failureRedirect: loginUrl }, + (err: any, user: any, info: any, status: any) => { + const diag = { + stage: 'oidc_callback', + hasErr: err != null, + errMessage: err?.message, + errName: err?.name, + errStack: err?.stack, + hasUser: user != null, + infoSummary: info?.message ?? String(info ?? ''), + statusCode: status, + // session + cookie diagnostics (no raw values) + hasSession: ctx.session != null, + sessionKeys: ctx.session != null ? Object.keys(ctx.session) : [], + hasCookieHeader: ctx.request.headers.cookie != null, + cookieHeaderLen: ctx.request.headers.cookie?.length ?? 0, + // request shape + host: ctx.request.headers.host, + forwardedProto: ctx.request.headers['x-forwarded-proto'], + // state presence (length only, not value) + 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' + } + if (err != null || user == null) { + measureCtx.error('OIDC callback failed', diag) + } else { + measureCtx.info('OIDC callback succeeded — entering handleProviderAuth', { + hasSession: diag.hasSession + }) + 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(' ') @@ -112,8 +156,30 @@ export function registerOpenid ( } await next() + } catch (err: any) { + // Permanent invalid-callback guard: ANY failure → 302 to /login, never 500. + measureCtx.error('OIDC callback failed', { + stage: 'oidc_callback', + hasErr: true, + errMessage: err?.message, + errName: err?.name, + errStack: err?.stack, + hasUser: ctx.state?.user != null, + infoSummary: '', + statusCode: undefined, + hasSession: ctx.session != null, + sessionKeys: ctx.session != null ? Object.keys(ctx.session) : [], + hasCookieHeader: ctx.request.headers.cookie != null, + cookieHeaderLen: ctx.request.headers.cookie?.length ?? 0, + 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' + }) + ctx.redirect(loginUrl + '?error=oidc_callback_failed') } - ) + }) return { name, displayName } } From ebdaa40a0ebdc81ea1dba5e275dfc5645a37d488 Mon Sep 17 00:00:00 2001 From: Michael Uray Date: Tue, 23 Jun 2026 06:11:17 +0000 Subject: [PATCH 2/8] fix(account): admin.ts trim+lowercase on both env-set AND lookup Codex plan-review Critical: server/account/src/admin.ts:15-18 built the ADMIN_EMAILS set without normalizing, AND isAdminEmail() didn't normalize the input. Today value is clean so isAdmin works, but a future env-value like ADMIN_EMAILS=" Michael@Uray.io " would silently break admin detection (Set lookup would miss the lowercased input). Mongo backend already has this hardening (collections/mongo.ts:1141-1147). Postgres + this helper didn't. Now BOTH sides normalize: env split -> trim -> toLowerCase -> filter non-empty + warn on no-@ entries (Codex Optional: warn not reject -- too sharp for a latent fix). Lookup also trim().toLowerCase(). Standalone upstream-PR-able per Codex Q-I3. Tests cover whitespace, case-insensitive env, case-insensitive lookup, invalid-shape warn, empty/unset env, multi-entry. Signed-off-by: Michael Uray Signed-off-by: Michael Uray --- .../account/src/__tests__/adminEmails.test.ts | 125 ++++++++++++++++++ server/account/src/admin.ts | 45 ++++++- 2 files changed, 167 insertions(+), 3 deletions(-) create mode 100644 server/account/src/__tests__/adminEmails.test.ts diff --git a/server/account/src/__tests__/adminEmails.test.ts b/server/account/src/__tests__/adminEmails.test.ts new file mode 100644 index 00000000000..74f370367d7 --- /dev/null +++ b/server/account/src/__tests__/adminEmails.test.ts @@ -0,0 +1,125 @@ +// +// 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 " Michael@Uray.io " 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('../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(' Michael@Uray.io ') + expect(isAdminEmail('michael@uray.io')).toBe(true) + }) + + test('2. case-insensitive env — uppercase env entry matches lowercase lookup', () => { + const { isAdminEmail } = loadAdmin('MICHAEL@URAY.IO') + expect(isAdminEmail('michael@uray.io')).toBe(true) + }) + + test('3. case-insensitive lookup — lowercase env matches uppercase lookup input', () => { + const { isAdminEmail } = loadAdmin('michael@uray.io') + expect(isAdminEmail('MICHAEL@URAY.IO')).toBe(true) + }) + + test('4. invalid-shape entries WARN (not reject) — no-@ entries kept for backwards compatibility', () => { + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}) + try { + const { isAdminEmail } = loadAdmin('foo,bar@@,baz') + // All entries kept; warn lists no-@ entries + expect(isAdminEmail('foo')).toBe(true) + expect(isAdminEmail('baz')).toBe(true) + expect(isAdminEmail('bar@@')).toBe(true) + expect(warnSpy).toHaveBeenCalledWith( + 'ADMIN_EMAILS contains entries without "@" (kept for backwards compatibility)', + { entries: ['foo', 'baz'] } + ) + } finally { + warnSpy.mockRestore() + } + }) + + test('4b. invalid-shape WARN via injected logger — all entries kept', () => { + // Direct parseAdminEmails call with custom logger — proves the + // injection point works (admin.ts has no MeasureContext at load time). + 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 for backwards compatibility)', + { entries: ['foo', 'baz'] } + ) + }) + + test('5. empty env string — empty set, all lookups return false', () => { + const { isAdminEmail } = loadAdmin('') + expect(isAdminEmail('michael@uray.io')).toBe(false) + expect(isAdminEmail('')).toBe(false) + }) + + test('6. unset env — empty set, all lookups return false', () => { + const { isAdminEmail } = loadAdmin(undefined) + expect(isAdminEmail('michael@uray.io')).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) + }) +}) diff --git a/server/account/src/admin.ts b/server/account/src/admin.ts index c5199e4b30d..b1a614025e8 100644 --- a/server/account/src/admin.ts +++ b/server/account/src/admin.ts @@ -12,8 +12,47 @@ // 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) => void +} + +/** + * Parse the ADMIN_EMAILS env value into a normalized Set. + * + * - split on ',' + * - trim() each entry + * - toLowerCase() each entry + * - drop empty entries + * - entries without '@' are kept (backwards compatibility with deployments + * that use login-id style ADMIN_EMAILS values, e.g. ADMIN_EMAILS=admin) + * but a warning is emitted listing them (Codex Optional: warn-don't-reject). + */ +export function parseAdminEmails (envValue: string | undefined, logger?: AdminEmailsLogger): Set { + if (envValue == null || envValue === '') return new Set() + const entries = envValue + .split(',') + .map((s) => s.trim().toLowerCase()) + .filter((s) => s.length > 0) + const invalid: string[] = [] + for (const entry of entries) { + if (!entry.includes('@')) invalid.push(entry) + } + if (invalid.length > 0) { + const warn = + logger?.warn ?? + ((msg: string, attrs?: Record) => { + // eslint-disable-next-line no-console + console.warn(msg, attrs) + }) + warn('ADMIN_EMAILS contains entries without "@" (kept for backwards compatibility)', { entries: invalid }) + } + return new Set(entries) +} + +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()) } From 7aaff27a7a216a96f7ce7dfca7c2750b1b180090 Mon Sep 17 00:00:00 2001 From: Michael Uray Date: Tue, 23 Jun 2026 06:11:20 +0000 Subject: [PATCH 3/8] chore(authProviders): drop cookieHeaderLen from OIDC diagnostics (E4 amendment) Codex E4 amendment (optional, low-priority): hasCookieHeader is sufficient to diagnose missing session-cookie issues; the cookieHeaderLen byte count adds no operational value and is one more PII-adjacent surface to keep an eye on. Drop it from both diagnostic paths in pods/authProviders/src/openid.ts. Signed-off-by: Michael Uray Signed-off-by: Michael Uray --- pods/authProviders/src/openid.ts | 68 +++++++++---------- .../account/src/__tests__/adminEmails.test.ts | 11 +-- 2 files changed, 37 insertions(+), 42 deletions(-) diff --git a/pods/authProviders/src/openid.ts b/pods/authProviders/src/openid.ts index 6bfa5e56fa7..25a5330a225 100644 --- a/pods/authProviders/src/openid.ts +++ b/pods/authProviders/src/openid.ts @@ -84,43 +84,38 @@ export function registerOpenid ( // 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((resolve) => { - passport.authenticate( - 'oidc', - { failureRedirect: loginUrl }, - (err: any, user: any, info: any, status: any) => { - const diag = { - stage: 'oidc_callback', - hasErr: err != null, - errMessage: err?.message, - errName: err?.name, - errStack: err?.stack, - hasUser: user != null, - infoSummary: info?.message ?? String(info ?? ''), - statusCode: status, - // session + cookie diagnostics (no raw values) - hasSession: ctx.session != null, - sessionKeys: ctx.session != null ? Object.keys(ctx.session) : [], - hasCookieHeader: ctx.request.headers.cookie != null, - cookieHeaderLen: ctx.request.headers.cookie?.length ?? 0, - // request shape - host: ctx.request.headers.host, - forwardedProto: ctx.request.headers['x-forwarded-proto'], - // state presence (length only, not value) - 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' - } - if (err != null || user == null) { - measureCtx.error('OIDC callback failed', diag) - } else { - measureCtx.info('OIDC callback succeeded — entering handleProviderAuth', { - hasSession: diag.hasSession - }) - ctx.state.user = user - } - resolve() + passport.authenticate('oidc', { failureRedirect: loginUrl }, (err: any, user: any, info: any, status: any) => { + const diag = { + stage: 'oidc_callback', + hasErr: err != null, + errMessage: err?.message, + errName: err?.name, + errStack: err?.stack, + hasUser: user != null, + infoSummary: info?.message ?? String(info ?? ''), + statusCode: status, + // session + cookie diagnostics (no raw values) + hasSession: ctx.session != null, + sessionKeys: ctx.session != null ? Object.keys(ctx.session) : [], + hasCookieHeader: ctx.request.headers.cookie != null, + // request shape + host: ctx.request.headers.host, + forwardedProto: ctx.request.headers['x-forwarded-proto'], + // state presence (length only, not value) + 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' } - )(ctx, async () => {}) + if (err != null || user == null) { + measureCtx.error('OIDC callback failed', diag) + } else { + measureCtx.info('OIDC callback succeeded — entering handleProviderAuth', { + hasSession: diag.hasSession + }) + ctx.state.user = user + } + resolve() + })(ctx, async () => {}) }) if (ctx.state.user == null) { @@ -170,7 +165,6 @@ export function registerOpenid ( hasSession: ctx.session != null, sessionKeys: ctx.session != null ? Object.keys(ctx.session) : [], hasCookieHeader: ctx.request.headers.cookie != null, - cookieHeaderLen: ctx.request.headers.cookie?.length ?? 0, host: ctx.request.headers.host, forwardedProto: ctx.request.headers['x-forwarded-proto'], statePresent: typeof ctx.query?.state === 'string', diff --git a/server/account/src/__tests__/adminEmails.test.ts b/server/account/src/__tests__/adminEmails.test.ts index 74f370367d7..a683bda78d6 100644 --- a/server/account/src/__tests__/adminEmails.test.ts +++ b/server/account/src/__tests__/adminEmails.test.ts @@ -74,7 +74,9 @@ describe('admin.ts — env parsing + lookup normalization', () => { expect(isAdminEmail('bar@@')).toBe(true) expect(warnSpy).toHaveBeenCalledWith( 'ADMIN_EMAILS contains entries without "@" (kept for backwards compatibility)', - { entries: ['foo', 'baz'] } + { + entries: ['foo', 'baz'] + } ) } finally { warnSpy.mockRestore() @@ -91,10 +93,9 @@ describe('admin.ts — env parsing + lookup normalization', () => { 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 for backwards compatibility)', - { entries: ['foo', 'baz'] } - ) + expect(warn).toHaveBeenCalledWith('ADMIN_EMAILS contains entries without "@" (kept for backwards compatibility)', { + entries: ['foo', 'baz'] + }) }) test('5. empty env string — empty set, all lookups return false', () => { From bda335d306f6024f3ef5a571667beb0bccdf3c36 Mon Sep 17 00:00:00 2001 From: Michael Uray Date: Wed, 8 Jul 2026 13:08:30 +0000 Subject: [PATCH 4/8] auth(oidc): gate verbose callback diagnostics behind OIDC_DEBUG (L-AUTH-2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bei jedem fehlgeschlagenen OIDC-Callback wurde ein umfangreicher Diag-Block (errStack/sessionKeys/host/…) auf error-Level geloggt -> anonymes Log- Flooding via wiederholter invalider Callbacks. Normalbetrieb loggt jetzt nur eine knappe Fehlerzeile; der volle Diag-Block nur mit OIDC_DEBUG=true. Signed-off-by: Michael Uray --- pods/authProviders/src/openid.ts | 77 +++++++++++++++++++------------- 1 file changed, 46 insertions(+), 31 deletions(-) diff --git a/pods/authProviders/src/openid.ts b/pods/authProviders/src/openid.ts index 25a5330a225..83159c0df09 100644 --- a/pods/authProviders/src/openid.ts +++ b/pods/authProviders/src/openid.ts @@ -85,32 +85,38 @@ export function registerOpenid ( // PRIVACY: never log raw code, raw state, tokens, or full ctx.state.user. await new Promise((resolve) => { passport.authenticate('oidc', { failureRedirect: loginUrl }, (err: any, user: any, info: any, status: any) => { - const diag = { + // 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, - errMessage: err?.message, errName: err?.name, - errStack: err?.stack, - hasUser: user != null, - infoSummary: info?.message ?? String(info ?? ''), + errMessage: err?.message, statusCode: status, - // session + cookie diagnostics (no raw values) - hasSession: ctx.session != null, - sessionKeys: ctx.session != null ? Object.keys(ctx.session) : [], - hasCookieHeader: ctx.request.headers.cookie != null, - // request shape - host: ctx.request.headers.host, - forwardedProto: ctx.request.headers['x-forwarded-proto'], - // state presence (length only, not value) - 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' + 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: diag.hasSession + hasSession: ctx.session != null }) ctx.state.user = user } @@ -153,24 +159,33 @@ export function registerOpenid ( await next() } catch (err: any) { // Permanent invalid-callback guard: ANY failure → 302 to /login, never 500. - measureCtx.error('OIDC callback failed', { + // L-AUTH-2: verbose fields behind OIDC_DEBUG (see the callback diag above). + const baseDiag = { stage: 'oidc_callback', hasErr: true, - errMessage: err?.message, errName: err?.name, - errStack: err?.stack, - hasUser: ctx.state?.user != null, - infoSummary: '', + errMessage: err?.message, statusCode: undefined, - 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' - }) + 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') } }) From 90bb1c7253a87e73c1d40f14638c6dbbb8e653eb Mon Sep 17 00:00:00 2001 From: Michael Uray Date: Wed, 8 Jul 2026 13:08:40 +0000 Subject: [PATCH 5/8] account(admin): drop non-email ADMIN_EMAILS entries unless explicitly allowed (L-AUTH-3) Ein Tippfehler wie ADMIN_EMAILS=admin,michel@... legte bislang einen unbeabsichtigten Admin-Identifier an (Eintrag ohne @ wurde still behalten). Jetzt werden @-lose Eintraege per Default verworfen (fail-closed) und nur bei ADMIN_EMAILS_ALLOW_LOGIN_ID=true als Login-ID akzeptiert; Warnung stets. Signed-off-by: Michael Uray --- .../account/src/__tests__/adminEmails.test.ts | 71 +++++++++++++------ server/account/src/admin.ts | 24 +++++-- 2 files changed, 68 insertions(+), 27 deletions(-) diff --git a/server/account/src/__tests__/adminEmails.test.ts b/server/account/src/__tests__/adminEmails.test.ts index a683bda78d6..15cac826384 100644 --- a/server/account/src/__tests__/adminEmails.test.ts +++ b/server/account/src/__tests__/adminEmails.test.ts @@ -64,38 +64,67 @@ describe('admin.ts — env parsing + lookup normalization', () => { expect(isAdminEmail('MICHAEL@URAY.IO')).toBe(true) }) - test('4. invalid-shape entries WARN (not reject) — no-@ entries kept for backwards compatibility', () => { + 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') - // All entries kept; warn lists no-@ entries - expect(isAdminEmail('foo')).toBe(true) - expect(isAdminEmail('baz')).toBe(true) + // 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 "@" (kept for backwards compatibility)', - { - entries: ['foo', 'baz'] - } - ) + 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. invalid-shape WARN via injected logger — all entries kept', () => { + 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 { 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 for backwards compatibility)', { - entries: ['foo', 'baz'] - }) + 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', () => { diff --git a/server/account/src/admin.ts b/server/account/src/admin.ts index b1a614025e8..a4c4185d67d 100644 --- a/server/account/src/admin.ts +++ b/server/account/src/admin.ts @@ -24,19 +24,30 @@ interface AdminEmailsLogger { * - trim() each entry * - toLowerCase() each entry * - drop empty entries - * - entries without '@' are kept (backwards compatibility with deployments - * that use login-id style ADMIN_EMAILS values, e.g. ADMIN_EMAILS=admin) - * but a warning is emitted listing them (Codex Optional: warn-don't-reject). + * - 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 { 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('@')) invalid.push(entry) + 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 = @@ -45,9 +56,10 @@ export function parseAdminEmails (envValue: string | undefined, logger?: AdminEm // eslint-disable-next-line no-console console.warn(msg, attrs) }) - warn('ADMIN_EMAILS contains entries without "@" (kept for backwards compatibility)', { entries: invalid }) + const action = allowLoginId ? 'kept (ADMIN_EMAILS_ALLOW_LOGIN_ID=true)' : 'DROPPED' + warn(`ADMIN_EMAILS contains entries without "@" (${action})`, { entries: invalid }) } - return new Set(entries) + return new Set(kept) } const ADMIN_EMAILS = parseAdminEmails(process.env.ADMIN_EMAILS) From 6f6d05e305e299934bbbebb9f55b0205cf51c74c Mon Sep 17 00:00:00 2001 From: Michael Uray Date: Wed, 8 Jul 2026 13:08:57 +0000 Subject: [PATCH 6/8] auth(oidc): redirect to /login when provider auth yields no account (L-AUTH-4) handleProviderAuth liefert '' bei einem nicht aufloesbaren Login (z.B. kein Account + Signup disabled). Bisher erfolgte dann kein Redirect und next() lief ins Leere -> haengende/leere Antwort. Jetzt fail-closed: explizites 302 auf /login?error=oidc_no_account. Signed-off-by: Michael Uray --- pods/authProviders/src/openid.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pods/authProviders/src/openid.ts b/pods/authProviders/src/openid.ts index 83159c0df09..da927c940d0 100644 --- a/pods/authProviders/src/openid.ts +++ b/pods/authProviders/src/openid.ts @@ -154,6 +154,11 @@ 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() From d05f4f986a72ca52e23b65c20aba281551ea5358 Mon Sep 17 00:00:00 2001 From: Michael Uray Date: Thu, 9 Jul 2026 07:18:09 +0000 Subject: [PATCH 7/8] test(account): use example.com fixtures in adminEmails tests Signed-off-by: Michael Uray --- .../account/src/__tests__/adminEmails.test.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/server/account/src/__tests__/adminEmails.test.ts b/server/account/src/__tests__/adminEmails.test.ts index 15cac826384..305536e2ad4 100644 --- a/server/account/src/__tests__/adminEmails.test.ts +++ b/server/account/src/__tests__/adminEmails.test.ts @@ -9,7 +9,7 @@ // // 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 " Michael@Uray.io " would silently break +// 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. @@ -50,18 +50,18 @@ describe('admin.ts — env parsing + lookup normalization', () => { }) test('1. whitespace tolerance — env entry with leading/trailing spaces', () => { - const { isAdminEmail } = loadAdmin(' Michael@Uray.io ') - expect(isAdminEmail('michael@uray.io')).toBe(true) + 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('MICHAEL@URAY.IO') - expect(isAdminEmail('michael@uray.io')).toBe(true) + 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('michael@uray.io') - expect(isAdminEmail('MICHAEL@URAY.IO')).toBe(true) + 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', () => { @@ -129,13 +129,13 @@ describe('admin.ts — env parsing + lookup normalization', () => { test('5. empty env string — empty set, all lookups return false', () => { const { isAdminEmail } = loadAdmin('') - expect(isAdminEmail('michael@uray.io')).toBe(false) + 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('michael@uray.io')).toBe(false) + expect(isAdminEmail('admin@example.com')).toBe(false) }) test('7. multiple entries with mixed whitespace + case all normalized', () => { From 514bdc56abc115a0c5c3d0216ef852ef920bc8c0 Mon Sep 17 00:00:00 2001 From: Michael Uray Date: Mon, 17 Aug 2026 11:17:29 +0000 Subject: [PATCH 8/8] auth(oidc): retry strategy registration with capped backoff (L-AUTH-5) The account service registered the OIDC passport strategy only once at startup via a single fire-and-forget Issuer.discover(). A transient issuer outage in that one boot moment left the 'oidc' strategy permanently unregistered, producing 500 "Unknown authentication strategy" on every OpenID login until a manual container restart (observed: ~10 days). Make registration self-healing: - Extract registerOidcStrategyWithRetry(), which retries the ENTIRE registration chain (discover -> new Client -> new Strategy -> passport.use) with capped exponential backoff (5s, x2, cap 60s), unbounded. The loop only returns after passport.use succeeds, so a failure at client or strategy construction (e.g. temporarily incomplete issuer metadata during an IdP upgrade) is retried too instead of permanently disabling OIDC. discover and sleep are injectable for tests; passport is a structural { use } interface. It never rejects: the success log runs failure-safe AFTER passport.use and returns immediately, so a throwing logger can never re-enter the loop and register the strategy a second time; the failure warn is likewise guarded so a throwing sink cannot break the never-rejects contract. - Gate log flooding via shouldWarnOnAttempt(): warn on the first 5 attempts and every 60th thereafter (~24 warn lines/day at the 60s cap instead of ~1440), following the branch's L-AUTH-2 rationale; error stacks only under OIDC_DEBUG. Success logs once with the attempt count. - Wire it fire-and-forget in registerOpenid() and flip an oidcReady flag on success. The .then body is a plain boolean flip that cannot throw; a trailing .catch(() => {}) is belt-and-suspenders so this fire-and-forget chain can never surface an unhandled rejection. Routes stay registered synchronously. - During the pending window, /auth/openid responds 503 with Retry-After: 5 and a short text body instead of a 500 or a redirect (the login app ignores an error query param, and L-AUTH-4 only covers the callback path). Add the package's first tests: the retry helper in isolation (schedule, 60s cap, whole-attempt retry incl. client-construction failure, warn gating, OIDC_DEBUG both ways, a throwing success log that must not re-register, and a throwing failure warn that must not reject) and the registerOpenid wiring (synchronous route registration, 503 pending window, exactly-once strategy registration). Signed-off-by: Michael Uray --- .../src/__tests__/openid.test.ts | 327 ++++++++++++++++++ pods/authProviders/src/openid.ts | 175 ++++++++-- 2 files changed, 481 insertions(+), 21 deletions(-) create mode 100644 pods/authProviders/src/__tests__/openid.test.ts diff --git a/pods/authProviders/src/__tests__/openid.test.ts b/pods/authProviders/src/__tests__/openid.test.ts new file mode 100644 index 00000000000..b50eb39a597 --- /dev/null +++ b/pods/authProviders/src/__tests__/openid.test.ts @@ -0,0 +1,327 @@ +// +// 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 the OIDC registration retry (incident: a transient issuer +// outage at boot left the account service without the 'oidc' passport strategy +// for 10 days -> 500 "Unknown authentication strategy" until a manual +// restart). Block A tests the retry helper in isolation (injected discover + +// sleep, fake passport): schedule, cap, whole-attempt retry semantics, warn +// gating, OIDC_DEBUG both ways. Block B tests the registerOpenid wiring with a +// fully mocked openid-client: non-blocking startup, 503 + Retry-After during +// the pending window, and exactly-once strategy registration. +// +/* eslint-disable @typescript-eslint/unbound-method */ + +import Router from 'koa-router' +import { Issuer, Strategy } from 'openid-client' +import { registerOidcStrategyWithRetry, registerOpenid, shouldWarnOnAttempt } from '../openid' + +jest.mock('openid-client', () => ({ + Issuer: { discover: jest.fn() }, + Strategy: jest.fn().mockImplementation(() => ({})) +})) + +function makeMeasureCtx (): any { + return { info: jest.fn(), warn: jest.fn(), error: jest.fn() } +} + +const params = { + issuerUrl: 'https://idp.example.com', + clientId: 'client-id', + clientSecret: 'client-secret', + redirectUri: 'http://accounts.example.com/auth/openid/callback' +} + +function makeSleepRecorder (): { sleep: (ms: number) => Promise, delays: number[] } { + const delays: number[] = [] + return { + delays, + sleep: async (ms: number): Promise => { + delays.push(ms) + } + } +} + +describe('A. registerOidcStrategyWithRetry — schedule, cap, whole-attempt retry, warn gating', () => { + test('1. fails K=3 times then succeeds: registers once after K+1 attempts, default schedule', async () => { + const ctx = makeMeasureCtx() + const passport = { use: jest.fn() } + const { sleep, delays } = makeSleepRecorder() + const discover = jest + .fn() + .mockRejectedValueOnce(new Error('ECONNREFUSED')) + .mockRejectedValueOnce(new Error('ECONNREFUSED')) + .mockRejectedValueOnce(new Error('ECONNREFUSED')) + .mockResolvedValue({ Client: jest.fn() }) + + const res = await registerOidcStrategyWithRetry(ctx, passport, params, { discover, sleep }) + + expect(res.attempts).toBe(4) + expect(discover).toHaveBeenCalledTimes(4) + expect(discover).toHaveBeenCalledWith('https://idp.example.com') + expect(delays).toEqual([5000, 10000, 20000]) + expect(passport.use).toHaveBeenCalledTimes(1) + expect(passport.use).toHaveBeenCalledWith('oidc', expect.anything()) + expect(Strategy).toHaveBeenCalledTimes(1) + expect(ctx.warn).toHaveBeenCalledTimes(3) + expect(ctx.info).toHaveBeenCalledWith('Registered OIDC strategy', { attempts: 4 }) + expect(ctx.error).not.toHaveBeenCalled() + }) + + test('2. backoff is capped at 60s and stays there', async () => { + const ctx = makeMeasureCtx() + const passport = { use: jest.fn() } + const { sleep, delays } = makeSleepRecorder() + const discover = jest.fn() + for (let i = 0; i < 7; i++) discover.mockRejectedValueOnce(new Error('boom')) + discover.mockResolvedValue({ Client: jest.fn() }) + + const res = await registerOidcStrategyWithRetry(ctx, passport, params, { discover, sleep }) + + expect(res.attempts).toBe(8) + expect(delays).toEqual([5000, 10000, 20000, 40000, 60000, 60000, 60000]) + }) + + test('3. immediate success: one attempt, no sleep, no warn, exactly one passport.use', async () => { + const ctx = makeMeasureCtx() + const passport = { use: jest.fn() } + const sleep = jest.fn() + const discover = jest.fn().mockResolvedValue({ Client: jest.fn() }) + + const res = await registerOidcStrategyWithRetry(ctx, passport, params, { discover, sleep }) + + expect(res.attempts).toBe(1) + expect(sleep).not.toHaveBeenCalled() + expect(ctx.warn).not.toHaveBeenCalled() + expect(passport.use).toHaveBeenCalledTimes(1) + }) + + test('4. retry covers the whole attempt: client construction failure retries too (HIGH-1)', async () => { + const ctx = makeMeasureCtx() + const passport = { use: jest.fn() } + const { sleep, delays } = makeSleepRecorder() + const throwingClient = jest.fn().mockImplementation(() => { + throw new Error('unsupported code_challenge_method') + }) + const discover = jest + .fn() + .mockResolvedValueOnce({ Client: throwingClient }) + .mockResolvedValue({ Client: jest.fn() }) + + const res = await registerOidcStrategyWithRetry(ctx, passport, params, { discover, sleep }) + + expect(res.attempts).toBe(2) + expect(discover).toHaveBeenCalledTimes(2) + expect(delays).toEqual([5000]) + expect(passport.use).toHaveBeenCalledTimes(1) + expect(ctx.warn).toHaveBeenCalledTimes(1) + expect(ctx.error).not.toHaveBeenCalled() + }) + + test('5. warn gating: attempts 1-5 and every 60th log, others are silent (MED-2)', async () => { + const ctx = makeMeasureCtx() + const passport = { use: jest.fn() } + const { sleep } = makeSleepRecorder() + const discover = jest.fn() + for (let i = 0; i < 65; i++) discover.mockRejectedValueOnce(new Error('boom')) + discover.mockResolvedValue({ Client: jest.fn() }) + + const res = await registerOidcStrategyWithRetry(ctx, passport, params, { discover, sleep }) + + expect(res.attempts).toBe(66) + expect(ctx.warn).toHaveBeenCalledTimes(6) + expect(ctx.warn.mock.calls.map((c: any[]) => c[1].attempt)).toEqual([1, 2, 3, 4, 5, 60]) + expect(ctx.info).toHaveBeenCalledWith('Registered OIDC strategy', { attempts: 66 }) + // pure gating function, spot checks + expect(shouldWarnOnAttempt(6)).toBe(false) + expect(shouldWarnOnAttempt(59)).toBe(false) + expect(shouldWarnOnAttempt(120)).toBe(true) + }) + + test('6. errStack absent without OIDC_DEBUG (L-AUTH-2 convention)', async () => { + const ctx = makeMeasureCtx() + const passport = { use: jest.fn() } + const { sleep } = makeSleepRecorder() + const discover = jest.fn().mockRejectedValueOnce(new Error('x')).mockResolvedValue({ Client: jest.fn() }) + const prev = process.env.OIDC_DEBUG + try { + delete process.env.OIDC_DEBUG + await registerOidcStrategyWithRetry(ctx, passport, params, { discover, sleep }) + expect(ctx.warn.mock.calls[0][1]).not.toHaveProperty('errStack') + } finally { + if (prev === undefined) delete process.env.OIDC_DEBUG + else process.env.OIDC_DEBUG = prev + } + }) + + test('7. errStack present with OIDC_DEBUG=true (L-AUTH-2 convention)', async () => { + const ctx = makeMeasureCtx() + const passport = { use: jest.fn() } + const { sleep } = makeSleepRecorder() + const discover = jest.fn().mockRejectedValueOnce(new Error('x')).mockResolvedValue({ Client: jest.fn() }) + const prev = process.env.OIDC_DEBUG + try { + process.env.OIDC_DEBUG = 'true' + await registerOidcStrategyWithRetry(ctx, passport, params, { discover, sleep }) + expect(ctx.warn.mock.calls[0][1]).toHaveProperty('errStack') + expect(typeof ctx.warn.mock.calls[0][1].errStack).toBe('string') + } finally { + if (prev === undefined) delete process.env.OIDC_DEBUG + else process.env.OIDC_DEBUG = prev + } + }) + + test('8. success log throwing after passport.use does not re-register (LOW-1)', async () => { + const ctx = makeMeasureCtx() + ctx.info = jest.fn((msg: string) => { + if (msg === 'Registered OIDC strategy') throw new Error('logger down') + }) + const passport = { use: jest.fn() } + const { sleep } = makeSleepRecorder() + const discover = jest.fn().mockResolvedValue({ Client: jest.fn() }) + + const res = await registerOidcStrategyWithRetry(ctx, passport, params, { discover, sleep }) + + // The throw from the success log must not re-enter the retry loop. + expect(res.attempts).toBe(1) + expect(passport.use).toHaveBeenCalledTimes(1) + expect(discover).toHaveBeenCalledTimes(1) + expect(ctx.warn).not.toHaveBeenCalled() + }) + + test('9. helper never rejects even if the failure warn throws (LOW-2)', async () => { + const ctx = makeMeasureCtx() + ctx.warn = jest.fn(() => { + throw new Error('warn sink down') + }) + const passport = { use: jest.fn() } + const { sleep } = makeSleepRecorder() + const discover = jest.fn().mockRejectedValueOnce(new Error('ECONNREFUSED')).mockResolvedValue({ Client: jest.fn() }) + + // Must resolve (contract: never rejects), not throw the warn error. + const res = await registerOidcStrategyWithRetry(ctx, passport, params, { discover, sleep }) + + expect(res.attempts).toBe(2) + expect(passport.use).toHaveBeenCalledTimes(1) + }) +}) + +describe('B. registerOpenid wiring — non-blocking startup, 503 pending window, exactly-once use', () => { + const env = process.env + + beforeEach(() => { + jest.useFakeTimers() + jest.clearAllMocks() + process.env = { + ...env, + OPENID_CLIENT_ID: 'client-id', + OPENID_CLIENT_SECRET: 'client-secret', + OPENID_ISSUER: 'https://idp.example.com', + OPENID_DISPLAY_NAME: 'Example IdP' + } + }) + + afterEach(() => { + jest.useRealTimers() + process.env = env + }) + + function callRegister (): { passport: any, router: Router, ctx: any } { + const passport: any = { use: jest.fn(), authenticate: jest.fn(() => async () => {}) } + const router = new Router() + const ctx = makeMeasureCtx() + const info = registerOpenid( + ctx, + passport, + router, + 'http://accounts.example.com', + Promise.resolve({} as any), + 'http://front.example.com', + {} + ) + expect(info).toEqual({ name: 'openid', displayName: 'Example IdP' }) + return { passport, router, ctx } + } + + async function invokeAuthRoute (router: Router): Promise { + const layer = router.stack.find((l) => l.path === '/auth/openid') + expect(layer).toBeDefined() + // encodeState dereferences ctx.request.headers (utils.ts getHost) and + // ctx.query — both must exist on the fixture. set/status/body carry the + // 503 pending-window response. + const koaCtx: any = { + redirect: jest.fn(), + set: jest.fn(), + status: undefined, + body: undefined, + query: {}, + state: {}, + request: { headers: {} } + } + await (layer as any).stack[0](koaCtx, async () => {}) + return koaCtx + } + + test('1. route registered synchronously; pending window responds 503 + Retry-After', async () => { + const discover = Issuer.discover as jest.Mock + discover.mockReturnValue(new Promise(() => {})) // discovery never settles + + const { passport, router } = callRegister() + const koaCtx = await invokeAuthRoute(router) + + expect(koaCtx.status).toBe(503) + expect(koaCtx.set).toHaveBeenCalledWith('Retry-After', '5') + expect(koaCtx.body).toEqual(expect.any(String)) + expect(koaCtx.redirect).not.toHaveBeenCalled() + expect(passport.authenticate).not.toHaveBeenCalled() + expect(passport.use).not.toHaveBeenCalled() + }) + + test('2. discover fails twice then succeeds: strategy registered exactly once, retries stop', async () => { + const discover = Issuer.discover as jest.Mock + discover + .mockRejectedValueOnce(new Error('ECONNREFUSED')) + .mockRejectedValueOnce(new Error('ECONNREFUSED')) + .mockResolvedValue({ Client: jest.fn() }) + + const { passport, ctx } = callRegister() + + // attempt 1 fails immediately; backoff 5s -> attempt 2 fails; 10s -> attempt 3 succeeds + await jest.advanceTimersByTimeAsync(5000) + await jest.advanceTimersByTimeAsync(10000) + + expect(discover).toHaveBeenCalledTimes(3) + expect(passport.use).toHaveBeenCalledTimes(1) + expect(passport.use).toHaveBeenCalledWith('oidc', expect.anything()) + expect(Strategy).toHaveBeenCalledTimes(1) + expect(ctx.info).toHaveBeenCalledWith('Registered OIDC strategy', { attempts: 3 }) + + // no further attempts, no double registration + await jest.advanceTimersByTimeAsync(600000) + expect(discover).toHaveBeenCalledTimes(3) + expect(passport.use).toHaveBeenCalledTimes(1) + }) + + test('3. after successful registration the auth route takes the passport path, no 503', async () => { + const discover = Issuer.discover as jest.Mock + discover.mockResolvedValue({ Client: jest.fn() }) + + const { passport, router } = callRegister() + await jest.advanceTimersByTimeAsync(0) // flush the resolved registration + + const koaCtx = await invokeAuthRoute(router) + expect(koaCtx.status).not.toBe(503) + expect(koaCtx.set).not.toHaveBeenCalled() + expect(koaCtx.redirect).not.toHaveBeenCalled() + expect(passport.authenticate).toHaveBeenCalledWith( + 'oidc', + expect.objectContaining({ scope: 'openid profile email' }) + ) + }) +}) diff --git a/pods/authProviders/src/openid.ts b/pods/authProviders/src/openid.ts index da927c940d0..4362a40c6d5 100644 --- a/pods/authProviders/src/openid.ts +++ b/pods/authProviders/src/openid.ts @@ -21,6 +21,130 @@ import { Issuer, Strategy } from 'openid-client' import { Passport } from '.' import { encodeState, handleProviderAuth, safeParseAuthState } from './utils' +const DISCOVERY_INITIAL_DELAY_MS = 5_000 +const DISCOVERY_MAX_DELAY_MS = 60_000 +const DISCOVERY_BACKOFF_FACTOR = 2 +const RETRY_WARN_FIRST_ATTEMPTS = 5 +const RETRY_WARN_EVERY_NTH_ATTEMPT = 60 + +export interface OidcRegistrationParams { + issuerUrl: string + clientId: string + clientSecret: string + redirectUri: string +} + +export interface OidcRetryOptions { + initialDelayMs?: number + maxDelayMs?: number + backoffFactor?: number + /** Injectable for tests; defaults to Issuer.discover. */ + discover?: (url: string) => Promise> + /** Injectable for tests; defaults to a real setTimeout-based sleep. */ + sleep?: (ms: number) => Promise +} + +/** Structural subset of passport used by the retry loop — keeps tests free of a real passport instance. */ +export interface PassportLike { + use: (name: string, strategy: any) => unknown +} + +/** + * Log-flood gating for the unbounded retry loop (L-AUTH-2 rationale): warn on + * the first 5 attempts, then on every 60th. At the 60s backoff cap that is + * ~24 warn lines/day for a permanently broken configuration instead of ~1440, + * while a real misconfiguration stays visible on an hourly cadence. + */ +export function shouldWarnOnAttempt (attempt: number): boolean { + return attempt <= RETRY_WARN_FIRST_ATTEMPTS || attempt % RETRY_WARN_EVERY_NTH_ATTEMPT === 0 +} + +/** + * Retry the ENTIRE OIDC strategy registration — discover, client construction, + * strategy construction, passport.use — with capped exponential backoff until + * it succeeds. The loop only ends after passport.use has run: openid-client can + * also throw at client/strategy construction (e.g. unsupported PKCE method in + * temporarily incomplete issuer metadata), and any of those failures must not + * permanently disable OIDC login (incident: 10 days of 500 "Unknown + * authentication strategy" until a manual restart). Deliberately unbounded and + * deliberately without transient/permanent classification: misclassifying a + * transient error would recreate the incident, the steady-state cost is + * <= 1 attempt/min, and a truly broken configuration stays visible through the + * gated warn cadence (shouldWarnOnAttempt). Never rejects. + */ +export async function registerOidcStrategyWithRetry ( + measureCtx: MeasureContext, + passport: PassportLike, + params: OidcRegistrationParams, + options: OidcRetryOptions = {} +): Promise<{ attempts: number }> { + const initialDelayMs = options.initialDelayMs ?? DISCOVERY_INITIAL_DELAY_MS + const maxDelayMs = options.maxDelayMs ?? DISCOVERY_MAX_DELAY_MS + const backoffFactor = options.backoffFactor ?? DISCOVERY_BACKOFF_FACTOR + const discover = options.discover ?? (async (url: string) => await Issuer.discover(url)) + const sleep = + options.sleep ?? + (async (ms: number) => { + await new Promise((resolve) => setTimeout(resolve, ms)) + }) + + let delayMs = initialDelayMs + for (let attempt = 1; ; attempt++) { + try { + const issuerObj = await discover(params.issuerUrl) + measureCtx.info('Discovered issuer', { issuer: issuerObj, attempts: attempt }) + + const client = new issuerObj.Client({ + client_id: params.clientId, + client_secret: params.clientSecret, + redirect_uris: [params.redirectUri], + response_types: ['code'] + }) + measureCtx.info('Created OIDC client') + + passport.use( + 'oidc', + new Strategy({ client, passReqToCallback: true }, (req: any, tokenSet: any, userinfo: any, done: any) => { + return done(null, userinfo) + }) + ) + // passport.use has run: the strategy is now registered exactly once. A throw + // from the success log below must NOT re-enter the retry loop (that would + // call passport.use a second time), so keep the log failure-safe and return + // regardless of whether it succeeds. + try { + measureCtx.info('Registered OIDC strategy', { attempts: attempt }) + } catch { + /* logging must never undo a successful registration */ + } + return { attempts: attempt } + } catch (err: any) { + if (shouldWarnOnAttempt(attempt)) { + try { + // L-AUTH-2 convention: terse warn in normal operation, stack only with OIDC_DEBUG. + measureCtx.warn('OIDC strategy registration failed — will retry', { + attempt, + nextRetryMs: delayMs, + errName: err?.name, + errMessage: err?.message, + ...(process.env.OIDC_DEBUG === 'true' ? { errStack: err?.stack } : {}) + }) + } catch { + /* the retry contract ("never rejects") outranks a warn that throws */ + } + } + await sleep(delayMs) + delayMs = Math.min(delayMs * backoffFactor, maxDelayMs) + } + } +} + +/** + * Invariant: called exactly once per process, from registerProviders at + * account-service startup (pods/authProviders/src/index.ts, the only caller). + * A second invocation would duplicate routes and the retry loop; that would + * already be a caller bug today (duplicate routes, duplicate passport.use). + */ export function registerOpenid ( measureCtx: MeasureContext, passport: Passport, @@ -40,31 +164,40 @@ export function registerOpenid ( const redirectURL = '/auth/openid/callback' if (openidClientId === undefined || openidClientSecret === undefined || issuer === undefined) return - Issuer.discover(issuer) - .then((issuerObj) => { - measureCtx.info('Discovered issuer', { issuer: issuerObj }) + let oidcReady = false - const client = new issuerObj.Client({ - client_id: openidClientId, - client_secret: openidClientSecret, - redirect_uris: [concatLink(accountsUrl, redirectURL)], - response_types: ['code'] - }) - measureCtx.info('Created OIDC client') - - passport.use( - 'oidc', - new Strategy({ client, passReqToCallback: true }, (req: any, tokenSet: any, userinfo: any, done: any) => { - return done(null, userinfo) - }) - ) - measureCtx.info('Registered OIDC strategy') - }) - .catch((err) => { - measureCtx.error('Failed to create OIDC client for IdP with the provided configuration', { err }) + // The helper never rejects (it retries forever, and both its success and its + // failure logging are failure-safe), and the .then body is a plain boolean + // flip that cannot throw. The trailing .catch(() => {}) is belt-and-suspenders: + // this chain is fire-and-forget, so even a hypothetical rejection must never + // surface as an unhandled rejection (which could crash the process). Success + // logging happens inside the helper ('Registered OIDC strategy' { attempts }). + void registerOidcStrategyWithRetry(measureCtx, passport, { + issuerUrl: issuer, + clientId: openidClientId, + clientSecret: openidClientSecret, + redirectUri: concatLink(accountsUrl, redirectURL) + }) + .then(() => { + oidcReady = true }) + .catch(() => {}) router.get('/auth/openid', async (ctx, next) => { + if (!oidcReady) { + // Pending window: the registration retry loop has not succeeded yet. + // Honest temporary-failure semantics: 503 + Retry-After matching the + // retry loop's initial delay. Deliberately not a redirect to /login — + // the login app only reads navigateUrl/token from the query + // (LoginApp.svelte), an error param would be silently ignored, and + // L-AUTH-4 only covers the callback path. Window is normally seconds + // long thanks to the registration retry above. + measureCtx.warn('OIDC login attempted before strategy registration — auth backend initializing', {}) + ctx.status = 503 + ctx.set('Retry-After', '5') + ctx.body = 'OIDC authentication is initializing, please retry shortly' + return + } measureCtx.info('try auth via', { provider: 'openid' }) const state = encodeState(ctx, brandings)