diff --git a/pods/authProviders/src/openid.ts b/pods/authProviders/src/openid.ts index 0a19d99b288..da927c940d0 100644 --- a/pods/authProviders/src/openid.ts +++ b/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((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 } } diff --git a/server/account/src/__tests__/adminEmails.test.ts b/server/account/src/__tests__/adminEmails.test.ts new file mode 100644 index 00000000000..305536e2ad4 --- /dev/null +++ b/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('../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) + }) +}) diff --git a/server/account/src/admin.ts b/server/account/src/admin.ts index c5199e4b30d..a4c4185d67d 100644 --- a/server/account/src/admin.ts +++ b/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) => 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 { + 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) => { + // 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()) }