diff --git a/packages/twenty-server/src/engine/core-modules/auth/strategies/saml.auth.strategy.spec.ts b/packages/twenty-server/src/engine/core-modules/auth/strategies/saml.auth.strategy.spec.ts new file mode 100644 index 0000000000..c0665ac4b2 --- /dev/null +++ b/packages/twenty-server/src/engine/core-modules/auth/strategies/saml.auth.strategy.spec.ts @@ -0,0 +1,205 @@ +/* @license Enterprise */ + +import { type Request } from 'express'; + +import { type SSOService } from 'src/engine/core-modules/sso/services/sso.service'; + +import { SamlAuthStrategy } from './saml.auth.strategy'; + +const IDP_A = 'idp-a-uuid'; +const IDP_B = 'idp-b-uuid'; +const VALID_EMAIL = 'alice@example.com'; + +type RelayStateInput = + | { kind: 'absent' } + | { kind: 'raw'; value: string } + | { kind: 'json'; value: Record }; + +const buildRequest = ({ + paramsIdpId, + relayState = { kind: 'absent' }, +}: { + paramsIdpId: string; + relayState?: RelayStateInput; +}) => { + let body: Record = {}; + + if (relayState.kind === 'raw') { + body = { RelayState: relayState.value }; + } else if (relayState.kind === 'json') { + body = { RelayState: JSON.stringify(relayState.value) }; + } + + return { + params: { identityProviderId: paramsIdpId }, + body, + } as unknown as Request; +}; + +const buildProfile = (email: string = VALID_EMAIL) => + ({ + email, + nameID: email, + issuer: 'irrelevant', + }) as unknown as Parameters[1]; + +describe('SamlAuthStrategy.validate', () => { + let strategy: SamlAuthStrategy; + + beforeEach(() => { + const ssoService = { + findSSOIdentityProviderById: jest.fn(), + isSAMLIdentityProvider: jest.fn(), + buildIssuerURL: jest.fn(), + buildCallbackUrl: jest.fn(), + } as unknown as SSOService; + + strategy = new SamlAuthStrategy(ssoService); + }); + + // Regression test for the workspace-confusion finding. An attacker-controlled + // RelayState that claims a different identity-provider id than the one whose + // cert verified the assertion must have zero influence on the resolved IdP. + it('ignores RelayState.identityProviderId and sources it exclusively from the URL path', async () => { + const request = buildRequest({ + paramsIdpId: IDP_A, + relayState: { kind: 'json', value: { identityProviderId: IDP_B } }, + }); + const done = jest.fn(); + + await strategy.validate(request, buildProfile(), done); + + expect(done).toHaveBeenCalledTimes(1); + expect(done.mock.calls[0][0]).toBeNull(); + expect(done.mock.calls[0][1]).toEqual({ + identityProviderId: IDP_A, + workspaceInviteHash: undefined, + email: VALID_EMAIL, + }); + }); + + it('accepts a request with no RelayState at all (no workspaceInviteHash threaded through)', async () => { + const request = buildRequest({ paramsIdpId: IDP_A }); + const done = jest.fn(); + + await strategy.validate(request, buildProfile(), done); + + expect(done).toHaveBeenCalledTimes(1); + expect(done.mock.calls[0][0]).toBeNull(); + expect(done.mock.calls[0][1]).toEqual({ + identityProviderId: IDP_A, + workspaceInviteHash: undefined, + email: VALID_EMAIL, + }); + }); + + it('threads workspaceInviteHash from RelayState through to req.user', async () => { + const request = buildRequest({ + paramsIdpId: IDP_A, + relayState: { + kind: 'json', + value: { workspaceInviteHash: 'invite-hash-123' }, + }, + }); + const done = jest.fn(); + + await strategy.validate(request, buildProfile(), done); + + expect(done).toHaveBeenCalledTimes(1); + expect(done.mock.calls[0][0]).toBeNull(); + expect(done.mock.calls[0][1]).toEqual({ + identityProviderId: IDP_A, + workspaceInviteHash: 'invite-hash-123', + email: VALID_EMAIL, + }); + }); + + it('still resolves identityProviderId from the URL path when RelayState mixes a malicious id with a real invite hash', async () => { + const request = buildRequest({ + paramsIdpId: IDP_A, + relayState: { + kind: 'json', + value: { + identityProviderId: IDP_B, + workspaceInviteHash: 'invite-hash-123', + }, + }, + }); + const done = jest.fn(); + + await strategy.validate(request, buildProfile(), done); + + expect(done).toHaveBeenCalledTimes(1); + expect(done.mock.calls[0][0]).toBeNull(); + expect(done.mock.calls[0][1]).toEqual({ + identityProviderId: IDP_A, + workspaceInviteHash: 'invite-hash-123', + email: VALID_EMAIL, + }); + }); + + it('tolerates malformed RelayState JSON without throwing (no invite hash extracted)', async () => { + const request = buildRequest({ + paramsIdpId: IDP_A, + relayState: { kind: 'raw', value: 'not-json{' }, + }); + const done = jest.fn(); + + await strategy.validate(request, buildProfile(), done); + + expect(done).toHaveBeenCalledTimes(1); + expect(done.mock.calls[0][0]).toBeNull(); + expect(done.mock.calls[0][1]).toEqual({ + identityProviderId: IDP_A, + workspaceInviteHash: undefined, + email: VALID_EMAIL, + }); + }); + + it('ignores non-string workspaceInviteHash payloads in RelayState', async () => { + const request = buildRequest({ + paramsIdpId: IDP_A, + relayState: { + kind: 'json', + value: { workspaceInviteHash: { nested: 'object' } }, + }, + }); + const done = jest.fn(); + + await strategy.validate(request, buildProfile(), done); + + expect(done).toHaveBeenCalledTimes(1); + expect(done.mock.calls[0][0]).toBeNull(); + expect(done.mock.calls[0][1]).toEqual({ + identityProviderId: IDP_A, + workspaceInviteHash: undefined, + email: VALID_EMAIL, + }); + }); + + it('rejects auth when the email claim is invalid', async () => { + const request = buildRequest({ paramsIdpId: IDP_A }); + const done = jest.fn(); + + await strategy.validate(request, buildProfile('not-an-email'), done); + + expect(done).toHaveBeenCalledTimes(1); + expect(done.mock.calls[0][0]).toBeInstanceOf(Error); + expect(done.mock.calls[0][0].message).toBe('Invalid email'); + }); + + it('rejects auth when the profile is missing', async () => { + const request = buildRequest({ paramsIdpId: IDP_A }); + const done = jest.fn(); + + await strategy.validate( + request, + undefined as unknown as Parameters[1], + done, + ); + + expect(done).toHaveBeenCalledTimes(1); + expect(done.mock.calls[0][0]).toBeInstanceOf(Error); + expect(done.mock.calls[0][0].message).toBe('Profile must be provided'); + }); +}); diff --git a/packages/twenty-server/src/engine/core-modules/auth/strategies/saml.auth.strategy.ts b/packages/twenty-server/src/engine/core-modules/auth/strategies/saml.auth.strategy.ts index 689e10f415..ed035a08c5 100644 --- a/packages/twenty-server/src/engine/core-modules/auth/strategies/saml.auth.strategy.ts +++ b/packages/twenty-server/src/engine/core-modules/auth/strategies/saml.auth.strategy.ts @@ -13,13 +13,32 @@ import { import { type AuthenticateOptions } from '@node-saml/passport-saml/lib/types'; import { isEmail } from 'class-validator'; import { type Request } from 'express'; +import { z } from 'zod'; -import { - AuthException, - AuthExceptionCode, -} from 'src/engine/core-modules/auth/auth.exception'; import { SSOService } from 'src/engine/core-modules/sso/services/sso.service'; +const WORKSPACE_INVITE_HASH_PAYLOAD_SCHEMA = z.object({ + workspaceInviteHash: z.string().optional(), +}); + +const RELAY_STATE_BODY_SCHEMA = z.object({ + RelayState: z + .string() + .transform((raw, ctx) => { + try { + return JSON.parse(raw) as unknown; + } catch { + ctx.addIssue({ + code: 'custom', + message: 'RelayState is not valid JSON', + }); + + return z.NEVER; + } + }) + .pipe(WORKSPACE_INVITE_HASH_PAYLOAD_SCHEMA), +}); + export type SAMLRequest = Omit< Request, 'user' | 'workspace' | 'workspaceMetadataVersion' @@ -83,43 +102,37 @@ export class SamlAuthStrategy extends PassportStrategy( } authenticate(req: Request, options: AuthenticateOptions) { + const queryParseResult = WORKSPACE_INVITE_HASH_PAYLOAD_SCHEMA.safeParse( + req.query, + ); + const workspaceInviteHash = queryParseResult.success + ? queryParseResult.data.workspaceInviteHash + : undefined; + super.authenticate(req, { ...options, - additionalParams: { - RelayState: JSON.stringify({ - identityProviderId: req.params.identityProviderId, - ...(req.query.workspaceInviteHash - ? { workspaceInviteHash: req.query.workspaceInviteHash } - : {}), - }), - }, + ...(workspaceInviteHash !== undefined + ? { + additionalParams: { + RelayState: JSON.stringify({ workspaceInviteHash }), + }, + } + : {}), }); } - private extractState(req: Request): { - identityProviderId: string; - workspaceInviteHash?: string; - } { - try { - if ('RelayState' in req.body && typeof req.body.RelayState === 'string') { - const RelayState = JSON.parse(req.body.RelayState); + private extractWorkspaceInviteHash(req: Request): string | undefined { + const result = RELAY_STATE_BODY_SCHEMA.safeParse(req.body); - return { - identityProviderId: RelayState.identityProviderId, - workspaceInviteHash: RelayState.workspaceInviteHash, - }; - } - - throw new Error(); - } catch { - throw new AuthException('Invalid state', AuthExceptionCode.INVALID_INPUT); - } + return result.success + ? result.data.RelayState.workspaceInviteHash + : undefined; } validate: VerifyWithRequest = async (request, profile, done) => { try { if (!profile) { - return done(new Error('Profile is must be provided')); + return done(new Error('Profile must be provided')); } const email = profile.email ?? profile.mail ?? profile.nameID; @@ -127,9 +140,12 @@ export class SamlAuthStrategy extends PassportStrategy( if (!isEmail(email)) { return done(new Error('Invalid email')); } - const state = this.extractState(request); - done(null, { ...state, email }); + done(null, { + identityProviderId: request.params.identityProviderId, + workspaceInviteHash: this.extractWorkspaceInviteHash(request), + email, + }); } catch (err) { done(err); }