From d1be81b6b33abc1162ee6b9952d4bd1ca761b071 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tao=20Bojl=C3=A9n?= Date: Tue, 16 Jun 2026 13:02:17 +0100 Subject: [PATCH] auth: fix SAML cross-org authentication bypass (#783) * Fix SAML cross-org authentication bypass The SAML verify callbacks looked up the user by email alone (kyselyUserFindByEmail), ignoring the org named in the callback path (/saml/login/:orgId). Because the same email can exist across tenants, an assertion signed by one org's IdP could resolve a user belonging to a different org and create a session as that cross-tenant user. Bind the lookup to the path org via a new org-scoped kyselyUserFindByEmailAndOrg, used by a shared resolveSamlUser helper for both the signon and logout verify callbacks. A user from another org is no longer found, so login fails. Addresses GHSA-2v93-383c-9fw2. Co-Authored-By: Claude Opus 4.8 (1M context) * Reject missing/invalid SAML email claim before lookup resolveSamlUser coerced the email claim with String(profile?.email), turning a missing claim into the literal "undefined" (and an array claim into a comma-joined string) and using it as a lookup key. Validate that the claim is a non-empty string and reject the authentication attempt before querying otherwise. Co-Authored-By: Claude Opus 4.8 (1M context) * Refactor resolveSamlUser to receive KyselyPg directly Match the codebase DI convention: consumers receive the Bottle-provided KyselyPg and call the free persistence functions directly (as UserApi, RoleApi, etc. do), rather than injecting a bespoke findUser closure. resolveSamlUser now takes db: UsersDb and calls kyselyUserFindByEmailAndOrg itself; api.ts passes KyselyPg. Its tests move to the real-DB testWithFixture pattern used for the persistence layer, exercising the org-scoped lookup against Postgres. Co-Authored-By: Claude Opus 4.8 (1M context) * Extract getOrgIdFromPath helper and log SAML login DB failures - Add a shared getOrgIdFromPath(req) helper and use it in both getSamlOptions and resolveSamlUser to DRY the orgId path-param check. - Thread the Tracer into resolveSamlUser and narrow the catch to the DB lookup so a genuine outage during login is logged via logActiveSpanFailedIfAny (observable) and surfaced as an internal error, instead of swallowing the done() dispatch in a broad catch. - Share one verify callback for the SAML signon and logout slots. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- server/api.ts | 76 ++----- .../datasources/userKyselyPersistence.ts | 17 +- ...KyselyPersistenceFindByEmailAndOrg.test.ts | 94 ++++++++ server/graphql/utils/orgIdFromPath.ts | 13 ++ server/graphql/utils/resolveSamlUser.test.ts | 205 ++++++++++++++++++ server/graphql/utils/resolveSamlUser.ts | 62 ++++++ 6 files changed, 411 insertions(+), 56 deletions(-) create mode 100644 server/graphql/datasources/userKyselyPersistenceFindByEmailAndOrg.test.ts create mode 100644 server/graphql/utils/orgIdFromPath.ts create mode 100644 server/graphql/utils/resolveSamlUser.test.ts create mode 100644 server/graphql/utils/resolveSamlUser.ts diff --git a/server/api.ts b/server/api.ts index fba4980..b425ccd 100644 --- a/server/api.ts +++ b/server/api.ts @@ -6,7 +6,11 @@ import { ApolloServerPluginLandingPageDisabled } from '@apollo/server/plugin/dis import { expressMiddleware } from '@as-integrations/express5'; import { makeExecutableSchema } from '@graphql-tools/schema'; import { MapperKind, mapSchema } from '@graphql-tools/utils'; -import { MultiSamlStrategy } from '@node-saml/passport-saml'; +import { + MultiSamlStrategy, + type Profile, + type VerifiedCallback, +} from '@node-saml/passport-saml'; import { SpanStatusCode } from '@opentelemetry/api'; import { ATTR_EXCEPTION_MESSAGE, @@ -15,21 +19,19 @@ import { } from '@opentelemetry/semantic-conventions'; import connectPgSimple from 'connect-pg-simple'; import cors from 'cors'; -import express, { type ErrorRequestHandler } from 'express'; +import express, { type ErrorRequestHandler, type Request } from 'express'; import session from 'express-session'; import { GraphQLError, type GraphQLFormattedError } from 'graphql'; import helmet from 'helmet'; import passport from 'passport'; -import { makeLoginUserDoesNotExistError } from './graphql/datasources/userApiErrors.js'; -import { - kyselyUserFindByEmail, - kyselyUserFindById, -} from './graphql/datasources/userKyselyPersistence.js'; +import { kyselyUserFindById } from './graphql/datasources/userKyselyPersistence.js'; import resolvers, { type Context } from './graphql/resolvers.js'; import typeDefs from './graphql/schema.js'; import { authSchemaWrapper } from './graphql/utils/authorization.js'; +import { getOrgIdFromPath } from './graphql/utils/orgIdFromPath.js'; import { buildPassportContext } from './graphql/utils/passportContext.js'; +import { resolveSamlUser } from './graphql/utils/resolveSamlUser.js'; import { safeDepthLimit } from './graphql/utils/safeDepthLimit.js'; import { type Dependencies } from './iocContainer/index.js'; import { safeGetEnvInt } from './iocContainer/utils.js'; @@ -145,14 +147,22 @@ export default async function makeApiServer(deps: Dependencies) { app.use(passport.initialize()); app.use(passport.session()); + // Shared signon/logout verify: bind the user lookup to the org named in the + // callback path so an assertion authenticating one org can never resolve a + // user from another (GHSA-2v93-383c-9fw2). + const verify = async ( + req: Request, + profile: Profile | null, + done: VerifiedCallback, + ) => resolveSamlUser(KyselyPg, deps.Tracer, req, profile, done); + passport.use( new MultiSamlStrategy( { passReqToCallback: true, async getSamlOptions(req, done) { // orgId path param should be set in the /saml/* route handlers. - const rawOrgId = req.params['orgId']; - const orgId = typeof rawOrgId === 'string' ? rawOrgId : undefined; + const orgId = getOrgIdFromPath(req); if (!orgId) { return done( @@ -190,52 +200,8 @@ export default async function makeApiServer(deps: Dependencies) { }); }, }, - async (_req, profile, done) => { - try { - const user = await kyselyUserFindByEmail( - KyselyPg, - String(profile?.email), - ); - // we should have already checked for this, but couldn't hurt to check - // again - if (user == null) { - return done( - makeLoginUserDoesNotExistError({ shouldErrorSpan: true }), - ); - } - - return done(null, user); - } catch (e) { - return done( - makeInternalServerError('Unknown error during login attempt', { - shouldErrorSpan: true, - }), - ); - } - }, - async (_req, profile, done) => { - try { - const user = await kyselyUserFindByEmail( - KyselyPg, - String(profile?.email), - ); - // we should have already checked for this, but couldn't hurt to check - // again - if (user == null) { - return done( - makeLoginUserDoesNotExistError({ shouldErrorSpan: true }), - ); - } - - return done(null, user); - } catch (e) { - return done( - makeInternalServerError('Unknown error during login attempt', { - shouldErrorSpan: true, - }), - ); - } - }, + verify, + verify, ), ); diff --git a/server/graphql/datasources/userKyselyPersistence.ts b/server/graphql/datasources/userKyselyPersistence.ts index 89377c6..e7b0999 100644 --- a/server/graphql/datasources/userKyselyPersistence.ts +++ b/server/graphql/datasources/userKyselyPersistence.ts @@ -58,7 +58,7 @@ export type GraphQLUserParent = { // Aligns with `ruleKyselyPersistence.ts`: persistence helpers operate on the // full app schema. Lets fixtures and `kyselyCreateRule` callers share a single // `Kysely` handle without running into Kysely's invariant generic. -type UsersDb = Kysely; +export type UsersDb = Kysely; type UserRow = { id: string; @@ -189,6 +189,21 @@ export async function kyselyUserFindByEmail( return row === undefined ? undefined : rowToGraphQLUserParent(row); } +export async function kyselyUserFindByEmailAndOrg( + db: UsersDb, + opts: { email: string; orgId: string }, +): Promise { + const row = await db + .selectFrom('public.users') + .select(USER_COLUMNS) + .select(loginMethodsAsTextArray) + .select(permissionsArray) + .where('email', '=', opts.email) + .where('org_id', '=', opts.orgId) + .executeTakeFirst(); + return row === undefined ? undefined : rowToGraphQLUserParent(row); +} + export async function kyselyUserFindByIds( db: UsersDb, ids: readonly string[], diff --git a/server/graphql/datasources/userKyselyPersistenceFindByEmailAndOrg.test.ts b/server/graphql/datasources/userKyselyPersistenceFindByEmailAndOrg.test.ts new file mode 100644 index 0000000..7fdcc69 --- /dev/null +++ b/server/graphql/datasources/userKyselyPersistenceFindByEmailAndOrg.test.ts @@ -0,0 +1,94 @@ +import { faker } from '@faker-js/faker'; +import { uid } from 'uid'; + +import { UserRole } from '../../services/userManagementService/index.js'; +import createOrg from '../../test/fixtureHelpers/createOrg.js'; +import { makeMockedServer } from '../../test/setupMockedServer.js'; +import { makeTestWithFixture } from '../../test/utils.js'; +import { + kyselyUserDeleteById, + kyselyUserFindByEmailAndOrg, + kyselyUserInsert, +} from './userKyselyPersistence.js'; + +function samlUserInput(orgId: string) { + return { + id: uid(), + orgId, + email: faker.internet.email(), + firstName: faker.name.firstName(), + lastName: faker.name.lastName(), + role: UserRole.ADMIN, + loginMethods: ['saml'] as const, + password: null, + }; +} + +describe('kyselyUserFindByEmailAndOrg', () => { + const testWithFixture = makeTestWithFixture(async () => { + const { deps, shutdown } = await makeMockedServer(); + const { org, cleanup: orgCleanup } = await createOrg( + { + KyselyPg: deps.KyselyPg, + ModerationConfigService: deps.ModerationConfigService, + ApiKeyService: deps.ApiKeyService, + }, + uid(), + ); + return { + deps, + org, + async cleanup() { + await orgCleanup(); + await shutdown(); + }, + }; + }); + + testWithFixture( + 'returns the row when email and org match', + async ({ deps, org }) => { + const input = samlUserInput(org.id); + await kyselyUserInsert({ db: deps.KyselyPg, ...input }); + try { + const result = await kyselyUserFindByEmailAndOrg(deps.KyselyPg, { + email: input.email, + orgId: org.id, + }); + expect(result).toMatchObject({ id: input.id, orgId: org.id }); + } finally { + await kyselyUserDeleteById(deps.KyselyPg, input.id); + } + }, + ); + + // Security regression (GHSA-2v93-383c-9fw2): a SAML assertion that + // authenticates `orgId` must never resolve a user who lives in another org. + testWithFixture( + 'returns undefined when the email exists in a different org', + async ({ deps, org }) => { + const input = samlUserInput(org.id); + await kyselyUserInsert({ db: deps.KyselyPg, ...input }); + try { + const result = await kyselyUserFindByEmailAndOrg(deps.KyselyPg, { + email: input.email, + orgId: `different-org-${uid()}`, + }); + expect(result).toBeUndefined(); + } finally { + await kyselyUserDeleteById(deps.KyselyPg, input.id); + } + }, + ); + + testWithFixture( + 'returns undefined when the email does not exist', + async ({ deps, org }) => { + const result = await kyselyUserFindByEmailAndOrg(deps.KyselyPg, { + email: `missing-${uid()}@example.com`, + orgId: org.id, + }); + expect(result).toBeUndefined(); + }, + ); +}); diff --git a/server/graphql/utils/orgIdFromPath.ts b/server/graphql/utils/orgIdFromPath.ts new file mode 100644 index 0000000..bd4581f --- /dev/null +++ b/server/graphql/utils/orgIdFromPath.ts @@ -0,0 +1,13 @@ +import { type Request } from 'express'; + +/** + * Read the `orgId` path param (e.g. `/saml/login/:orgId/callback`), returning + * `undefined` when it is missing or not a string. Callers decide how to handle + * its absence (typically a not-found error). + */ +export function getOrgIdFromPath( + req: Pick, +): string | undefined { + const rawOrgId = req.params['orgId']; + return typeof rawOrgId === 'string' ? rawOrgId : undefined; +} diff --git a/server/graphql/utils/resolveSamlUser.test.ts b/server/graphql/utils/resolveSamlUser.test.ts new file mode 100644 index 0000000..c28154c --- /dev/null +++ b/server/graphql/utils/resolveSamlUser.test.ts @@ -0,0 +1,205 @@ +import { faker } from '@faker-js/faker'; +import { type Profile } from '@node-saml/passport-saml'; +import { type Request } from 'express'; +import { uid } from 'uid'; + +import { UserRole } from '../../services/userManagementService/index.js'; +import createOrg from '../../test/fixtureHelpers/createOrg.js'; +import { makeMockedServer } from '../../test/setupMockedServer.js'; +import { makeTestWithFixture } from '../../test/utils.js'; +import { type default as SafeTracer } from '../../utils/SafeTracer.js'; +import { + kyselyUserDeleteById, + kyselyUserInsert, + type UsersDb, +} from '../datasources/userKyselyPersistence.js'; +import { resolveSamlUser } from './resolveSamlUser.js'; + +function makeReq(orgId?: string): Pick { + return { params: orgId === undefined ? {} : { orgId } }; +} + +function samlUserInput(orgId: string) { + return { + id: uid(), + orgId, + email: faker.internet.email(), + firstName: faker.name.firstName(), + lastName: faker.name.lastName(), + role: UserRole.ADMIN, + loginMethods: ['saml'] as const, + password: null, + }; +} + +describe('resolveSamlUser', () => { + const testWithFixture = makeTestWithFixture(async () => { + const { deps, shutdown } = await makeMockedServer(); + const { org, cleanup: orgCleanup } = await createOrg( + { + KyselyPg: deps.KyselyPg, + ModerationConfigService: deps.ModerationConfigService, + ApiKeyService: deps.ApiKeyService, + }, + uid(), + ); + return { + deps, + org, + async cleanup() { + await orgCleanup(); + await shutdown(); + }, + }; + }); + + testWithFixture( + 'passes the user to done when the email belongs to the path org', + async ({ deps, org }) => { + const input = samlUserInput(org.id); + await kyselyUserInsert({ db: deps.KyselyPg, ...input }); + const done = jest.fn(); + try { + await resolveSamlUser( + deps.KyselyPg, + deps.Tracer, + makeReq(org.id), + { email: input.email }, + done, + ); + expect(done).toHaveBeenCalledTimes(1); + const [err, user] = done.mock.calls[0]; + expect(err).toBeNull(); + expect(user).toMatchObject({ id: input.id, orgId: org.id }); + } finally { + await kyselyUserDeleteById(deps.KyselyPg, input.id); + } + }, + ); + + // Security regression (GHSA-2v93-383c-9fw2): an assertion authenticating one + // org must never resolve a user who lives in another org. + testWithFixture( + 'rejects when the email belongs to a different org', + async ({ deps, org }) => { + const input = samlUserInput(org.id); + await kyselyUserInsert({ db: deps.KyselyPg, ...input }); + const done = jest.fn(); + try { + await resolveSamlUser( + deps.KyselyPg, + deps.Tracer, + makeReq(`different-org-${uid()}`), + { email: input.email }, + done, + ); + const [err, user] = done.mock.calls[0]; + expect(err).toBeInstanceOf(Error); + expect(user).toBeUndefined(); + } finally { + await kyselyUserDeleteById(deps.KyselyPg, input.id); + } + }, + ); + + testWithFixture( + 'rejects when no user exists for the email in that org', + async ({ deps, org }) => { + const done = jest.fn(); + await resolveSamlUser( + deps.KyselyPg, + deps.Tracer, + makeReq(org.id), + { email: `missing-${uid()}@example.com` }, + done, + ); + expect(done.mock.calls[0][0]).toBeInstanceOf(Error); + expect(done.mock.calls[0][1]).toBeUndefined(); + }, + ); + + testWithFixture( + 'rejects when orgId is missing from the path', + async ({ deps }) => { + const done = jest.fn(); + await resolveSamlUser( + deps.KyselyPg, + deps.Tracer, + makeReq(undefined), + { email: 'a@example.com' }, + done, + ); + expect(done.mock.calls[0][0]).toBeInstanceOf(Error); + }, + ); + + // A missing/blank email claim must be rejected outright, never coerced + // (e.g. String(undefined) === "undefined") and used as a lookup key. + for (const [label, profile] of [ + ['missing', {}], + ['undefined', { email: undefined }], + ['empty', { email: '' }], + ] as const) { + testWithFixture( + `rejects without a match when the email claim is ${label}`, + async ({ deps, org }) => { + const done = jest.fn(); + await resolveSamlUser( + deps.KyselyPg, + deps.Tracer, + makeReq(org.id), + profile, + done, + ); + expect(done.mock.calls[0][0]).toBeInstanceOf(Error); + expect(done.mock.calls[0][1]).toBeUndefined(); + }, + ); + } + + // Defensive: node-saml types email as a string, but a multi-valued attribute + // could arrive as an array at runtime — reject, don't coerce. + testWithFixture( + 'rejects when the email claim is not a string', + async ({ deps, org }) => { + const arrayProfile = { email: ['a@example.com'] }; + const done = jest.fn(); + await resolveSamlUser( + deps.KyselyPg, + deps.Tracer, + makeReq(org.id), + arrayProfile as unknown as Pick, + done, + ); + expect(done.mock.calls[0][0]).toBeInstanceOf(Error); + expect(done.mock.calls[0][1]).toBeUndefined(); + }, + ); + + // A genuine DB failure during the lookup must be logged to the tracer (so + // outages are observable) and surfaced as an internal error, not swallowed. + test('logs to the tracer and returns an internal error on a DB failure', async () => { + const dbError = new Error('connection refused'); + const db = { + selectFrom() { + throw dbError; + }, + } as unknown as UsersDb; + const tracer = { + logActiveSpanFailedIfAny: jest.fn(), + } as unknown as SafeTracer; + const done = jest.fn(); + + await resolveSamlUser( + db, + tracer, + makeReq('some-org'), + { email: 'a@example.com' }, + done, + ); + + expect(tracer.logActiveSpanFailedIfAny).toHaveBeenCalledWith(dbError); + expect(done.mock.calls[0][0]).toBeInstanceOf(Error); + expect(done.mock.calls[0][1]).toBeUndefined(); + }); +}); diff --git a/server/graphql/utils/resolveSamlUser.ts b/server/graphql/utils/resolveSamlUser.ts new file mode 100644 index 0000000..dd3d021 --- /dev/null +++ b/server/graphql/utils/resolveSamlUser.ts @@ -0,0 +1,62 @@ +import { type Profile, type VerifiedCallback } from '@node-saml/passport-saml'; +import { type Request } from 'express'; + +import { + makeInternalServerError, + makeNotFoundError, +} from '../../utils/errors.js'; +import { type default as SafeTracer } from '../../utils/SafeTracer.js'; +import { makeLoginUserDoesNotExistError } from '../datasources/userApiErrors.js'; +import { + kyselyUserFindByEmailAndOrg, + type GraphQLUserParent, + type UsersDb, +} from '../datasources/userKyselyPersistence.js'; +import { getOrgIdFromPath } from './orgIdFromPath.js'; + +/** + * Resolves the authenticated user for a SAML assertion, binding the lookup to + * the org named in the callback path (`/saml/login/:orgId/callback`). + */ +export async function resolveSamlUser( + db: UsersDb, + tracer: SafeTracer, + req: Pick, + profile: Pick | null, + done: VerifiedCallback, +): Promise { + const orgId = getOrgIdFromPath(req); + if (!orgId) { + return done( + makeNotFoundError('orgId not found in path.', { shouldErrorSpan: true }), + ); + } + + // Reject a missing/blank/non-string email claim instead of coercing it + // (e.g. String(undefined) === "undefined") into a lookup key. + const email = profile?.email; + if (typeof email !== 'string' || email.length === 0) { + return done(makeLoginUserDoesNotExistError({ shouldErrorSpan: true })); + } + + // Scope the catch to the DB lookup so a genuine outage is logged and surfaced + // as an internal error, while the `done` dispatch below isn't swallowed. + let user: GraphQLUserParent | undefined; + try { + user = await kyselyUserFindByEmailAndOrg(db, { email, orgId }); + } catch (e) { + tracer.logActiveSpanFailedIfAny(e); + return done( + makeInternalServerError('Unknown error during login attempt', { + shouldErrorSpan: true, + }), + ); + } + + // we should have already checked for this, but couldn't hurt to check again + if (user == null) { + return done(makeLoginUserDoesNotExistError({ shouldErrorSpan: true })); + } + + return done(null, user); +} -- 2.51.2