Skip to content

Commit aa57f10

Browse files
authored
fix(auth): close nOAuth account takeover via email-based OAuth linking (#5156)
* fix(auth): close nOAuth account takeover via email-based OAuth linking Restrict the unauthenticated sign-in endpoints to first-party login providers, trim trustedProviders to providers that verify email ownership, and stop hardcoding emailVerified for multi-tenant Microsoft and Salesforce connectors. * test(auth): cover Microsoft id-token emailVerified derivation Extract the Microsoft ID-token email-verification logic into a pure deriveMicrosoftEmailVerified helper and add unit coverage for explicit, verified-claim, partial, absent, and malformed Azure AD claim combinations. * fix(auth): check the provider field the sign-in handler actually uses The allowlist guard resolved the provider with `provider ?? providerId`, but Better Auth reads `provider` on /sign-in/social and `providerId` on /sign-in/oauth2. A request to /sign-in/oauth2 with an allowed `provider` and a blocked `providerId` could pass the guard while the handler started OAuth for the blocked connector. Resolve the field per path via getRequestedSignInProviderId so the guard checks the same field the handler acts on.
1 parent 82cb324 commit aa57f10

5 files changed

Lines changed: 265 additions & 53 deletions

File tree

apps/sim/lib/auth/auth.ts

Lines changed: 40 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -88,10 +88,15 @@ import { syncAllWebhooksForCredentialSet } from '@/lib/webhooks/utils.server'
8888
import { disableUserResources } from '@/lib/workflows/lifecycle'
8989
import { SSO_TRUSTED_PROVIDERS } from '@/ee/sso/constants'
9090
import { createAnonymousSession, ensureAnonymousUserExists } from './anonymous'
91+
import { getRequestedSignInProviderId, isSignInProviderAllowed } from './constants'
9192

9293
const logger = createLogger('Auth')
9394

94-
import { getMicrosoftRefreshTokenExpiry, isMicrosoftProvider } from '@/lib/oauth/microsoft'
95+
import {
96+
deriveMicrosoftEmailVerified,
97+
getMicrosoftRefreshTokenExpiry,
98+
isMicrosoftProvider,
99+
} from '@/lib/oauth/microsoft'
95100
import { getCanonicalScopesForProvider } from '@/lib/oauth/utils'
96101

97102
/**
@@ -128,12 +133,14 @@ function getMicrosoftUserInfoFromIdToken(tokens: { accessToken?: string }, provi
128133
)
129134
}
130135

136+
const emailVerified = deriveMicrosoftEmailVerified(payload, email)
137+
131138
const now = new Date()
132139
return {
133140
id: `${payload.oid || payload.sub}-${generateId()}`,
134141
name: (payload.name as string) || 'Microsoft User',
135142
email,
136-
emailVerified: true,
143+
emailVerified,
137144
createdAt: now,
138145
updatedAt: now,
139146
}
@@ -661,60 +668,21 @@ export const auth = betterAuth({
661668
enabled: true,
662669
allowDifferentEmails: true,
663670
requireLocalEmailVerified: false,
671+
/**
672+
* Only providers that verify email ownership may auto-link to an existing
673+
* account during sign-in. Integration connectors are deliberately absent:
674+
* they connect through the authenticated `/oauth2/link` flow, which binds
675+
* to the current session user and never consults this list. `microsoft` is
676+
* also excluded because it authenticates against the multi-tenant
677+
* `/common/` endpoint where the email claim is attacker-controllable;
678+
* leaving it trusted would bypass the email-verified check and allow
679+
* nOAuth account takeover. Microsoft sign-in still works — it just links
680+
* to an existing account only when the IdP asserts a verified email.
681+
*/
664682
trustedProviders: [
665683
'google',
666684
'github',
667685
'email-password',
668-
'confluence',
669-
'x',
670-
'notion',
671-
'microsoft',
672-
'slack',
673-
'reddit',
674-
'webflow',
675-
'asana',
676-
'pipedrive',
677-
'hubspot',
678-
'linkedin',
679-
'spotify',
680-
'google-email',
681-
'google-calendar',
682-
'google-contacts',
683-
'google-drive',
684-
'google-docs',
685-
'google-sheets',
686-
'google-forms',
687-
'google-ads',
688-
'google-bigquery',
689-
'google-vault',
690-
'google-groups',
691-
'google-meet',
692-
'google-tasks',
693-
'vertex-ai',
694-
695-
'microsoft-ad',
696-
'microsoft-dataverse',
697-
'microsoft-teams',
698-
'microsoft-excel',
699-
'microsoft-planner',
700-
'outlook',
701-
'onedrive',
702-
'sharepoint',
703-
'jira',
704-
'airtable',
705-
'box',
706-
'dropbox',
707-
'salesforce',
708-
'wealthbox',
709-
'zoom',
710-
'wordpress',
711-
'linear',
712-
'monday',
713-
'attio',
714-
'shopify',
715-
'trello',
716-
'calcom',
717-
'docusign',
718686
...SSO_TRUSTED_PROVIDERS,
719687
...additionalTrustedSsoProviders,
720688
],
@@ -883,6 +851,25 @@ export const auth = betterAuth({
883851
},
884852
hooks: {
885853
before: createAuthMiddleware(async (ctx) => {
854+
/**
855+
* Restrict the unauthenticated sign-in endpoints to first-party login
856+
* providers. Better Auth registers every generic-OAuth integration
857+
* connector as a social provider, so without this guard `microsoft-ad`,
858+
* `salesforce`, `jira`, and the rest are reachable through
859+
* `/sign-in/social` and `/sign-in/oauth2` and can mint a session for any
860+
* user by email (nOAuth account takeover). Connectors are connected only
861+
* through the authenticated `/oauth2/link` flow, which is unaffected.
862+
*/
863+
if (ctx.path === '/sign-in/social' || ctx.path === '/sign-in/oauth2') {
864+
const requestedProviderId = getRequestedSignInProviderId(ctx.path, ctx.body)
865+
if (!isSignInProviderAllowed(requestedProviderId)) {
866+
throw new APIError('FORBIDDEN', {
867+
message:
868+
'This provider can only be connected from a signed-in account and cannot be used to sign in.',
869+
})
870+
}
871+
}
872+
886873
if (ctx.path.startsWith('/sign-up') && isRegistrationDisabled)
887874
throw new APIError('FORBIDDEN', {
888875
message: 'Registration is disabled, please contact your admin.',
@@ -1921,7 +1908,7 @@ export const auth = betterAuth({
19211908
id: `${(data.user_id || data.sub).toString()}-${generateId()}`,
19221909
name: data.name || 'Salesforce User',
19231910
email: data.email || `salesforce-${data.user_id}@salesforce.com`,
1924-
emailVerified: data.email_verified || true,
1911+
emailVerified: data.email_verified === true,
19251912
image: data.picture || undefined,
19261913
createdAt: new Date(),
19271914
updatedAt: new Date(),
Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it } from 'vitest'
5+
import {
6+
getRequestedSignInProviderId,
7+
isSignInProviderAllowed,
8+
SIGN_IN_PROVIDER_IDS,
9+
} from '@/lib/auth/constants'
10+
11+
describe('sign-in provider allowlist', () => {
12+
it('permits only the first-party login providers', () => {
13+
expect([...SIGN_IN_PROVIDER_IDS]).toEqual(['google', 'github', 'microsoft'])
14+
})
15+
16+
it('allows first-party login providers to sign in', () => {
17+
for (const providerId of SIGN_IN_PROVIDER_IDS) {
18+
expect(isSignInProviderAllowed(providerId)).toBe(true)
19+
}
20+
})
21+
22+
it('rejects integration connectors from the sign-in endpoints', () => {
23+
const connectors = [
24+
'microsoft-ad',
25+
'microsoft-teams',
26+
'microsoft-excel',
27+
'outlook',
28+
'onedrive',
29+
'sharepoint',
30+
'salesforce',
31+
'jira',
32+
'confluence',
33+
'hubspot',
34+
'box',
35+
'wordpress',
36+
'google-drive',
37+
'google-sheets',
38+
'vertex-ai',
39+
]
40+
for (const providerId of connectors) {
41+
expect(isSignInProviderAllowed(providerId)).toBe(false)
42+
}
43+
})
44+
45+
it('rejects missing or malformed provider identifiers', () => {
46+
expect(isSignInProviderAllowed(undefined)).toBe(false)
47+
expect(isSignInProviderAllowed(null)).toBe(false)
48+
expect(isSignInProviderAllowed('')).toBe(false)
49+
expect(isSignInProviderAllowed(123)).toBe(false)
50+
expect(isSignInProviderAllowed({ provider: 'google' })).toBe(false)
51+
})
52+
})
53+
54+
describe('getRequestedSignInProviderId', () => {
55+
it('reads the `provider` field on /sign-in/social', () => {
56+
expect(getRequestedSignInProviderId('/sign-in/social', { provider: 'microsoft' })).toBe(
57+
'microsoft'
58+
)
59+
})
60+
61+
it('reads the `providerId` field on /sign-in/oauth2', () => {
62+
expect(getRequestedSignInProviderId('/sign-in/oauth2', { providerId: 'salesforce' })).toBe(
63+
'salesforce'
64+
)
65+
})
66+
67+
it('checks the field the /sign-in/oauth2 handler uses, ignoring a decoy `provider`', () => {
68+
const body = { provider: 'google', providerId: 'salesforce' }
69+
const resolved = getRequestedSignInProviderId('/sign-in/oauth2', body)
70+
expect(resolved).toBe('salesforce')
71+
expect(isSignInProviderAllowed(resolved)).toBe(false)
72+
})
73+
74+
it('checks the field the /sign-in/social handler uses, ignoring a decoy `providerId`', () => {
75+
const body = { provider: 'salesforce', providerId: 'google' }
76+
const resolved = getRequestedSignInProviderId('/sign-in/social', body)
77+
expect(resolved).toBe('salesforce')
78+
expect(isSignInProviderAllowed(resolved)).toBe(false)
79+
})
80+
81+
it('returns undefined for unrelated paths and missing bodies', () => {
82+
expect(getRequestedSignInProviderId('/sign-in/email', { provider: 'google' })).toBeUndefined()
83+
expect(getRequestedSignInProviderId('/sign-in/social', undefined)).toBeUndefined()
84+
expect(getRequestedSignInProviderId('/sign-in/oauth2', null)).toBeUndefined()
85+
})
86+
})

apps/sim/lib/auth/constants.ts

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,3 +8,47 @@ export const ANONYMOUS_USER = {
88
emailVerified: true,
99
image: null,
1010
} as const
11+
12+
/**
13+
* Provider IDs permitted to authenticate (create or link a session) through the
14+
* unauthenticated sign-in endpoints `/sign-in/social` and `/sign-in/oauth2`.
15+
*
16+
* Only first-party login providers that verify email ownership belong here.
17+
* Integration connectors (Salesforce, Jira, the Microsoft/Google work
18+
* connectors, etc.) are deliberately excluded: they are connected exclusively
19+
* through the authenticated `/oauth2/link` flow, which binds the new account to
20+
* the current session user and never mints a session. Allowing a connector to
21+
* reach the sign-in endpoints enables nOAuth-style account takeover, where a
22+
* multi-tenant IdP asserting an attacker-controlled, unverified email mints a
23+
* session for the matching existing user. SSO uses a separate `/sign-in/sso`
24+
* endpoint and is unaffected by this list.
25+
*/
26+
export const SIGN_IN_PROVIDER_IDS = ['google', 'github', 'microsoft'] as const
27+
28+
const signInProviderIdSet: ReadonlySet<string> = new Set(SIGN_IN_PROVIDER_IDS)
29+
30+
/**
31+
* Returns true when `providerId` is a first-party login provider allowed to sign
32+
* in. Used to reject integration connectors at the sign-in endpoints so they can
33+
* only ever be connected through the authenticated link flow.
34+
*/
35+
export function isSignInProviderAllowed(providerId: unknown): boolean {
36+
return typeof providerId === 'string' && signInProviderIdSet.has(providerId)
37+
}
38+
39+
/**
40+
* Resolves the provider identifier the sign-in handler will actually act on for a
41+
* given auth path. Better Auth reads `provider` on `/sign-in/social` and
42+
* `providerId` on `/sign-in/oauth2`, so the allowlist guard must read the same
43+
* field the handler uses. Reading the wrong field would let a request pass the
44+
* guard with an allowed value in one field while the handler starts OAuth for a
45+
* blocked value in the other, reopening connector sign-in.
46+
*/
47+
export function getRequestedSignInProviderId(
48+
path: string,
49+
body: { provider?: unknown; providerId?: unknown } | null | undefined
50+
): unknown {
51+
if (path === '/sign-in/social') return body?.provider
52+
if (path === '/sign-in/oauth2') return body?.providerId
53+
return undefined
54+
}
Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it } from 'vitest'
5+
import { deriveMicrosoftEmailVerified, isMicrosoftProvider } from '@/lib/oauth/microsoft'
6+
7+
const EMAIL = 'user@contoso.com'
8+
9+
describe('deriveMicrosoftEmailVerified', () => {
10+
it('honors an explicit email_verified=true claim', () => {
11+
expect(deriveMicrosoftEmailVerified({ email_verified: true }, EMAIL)).toBe(true)
12+
})
13+
14+
it('honors an explicit email_verified=false claim over verified-email claims', () => {
15+
expect(
16+
deriveMicrosoftEmailVerified(
17+
{ email_verified: false, verified_primary_email: [EMAIL] },
18+
EMAIL
19+
)
20+
).toBe(false)
21+
})
22+
23+
it('treats a verified primary email matching the email as verified', () => {
24+
expect(deriveMicrosoftEmailVerified({ verified_primary_email: [EMAIL] }, EMAIL)).toBe(true)
25+
})
26+
27+
it('treats a verified secondary email matching the email as verified', () => {
28+
expect(
29+
deriveMicrosoftEmailVerified({ verified_secondary_email: ['x@y.com', EMAIL] }, EMAIL)
30+
).toBe(true)
31+
})
32+
33+
it('does not verify when the verified-email claims do not include the email', () => {
34+
expect(
35+
deriveMicrosoftEmailVerified(
36+
{
37+
verified_primary_email: ['other@contoso.com'],
38+
verified_secondary_email: ['another@contoso.com'],
39+
},
40+
EMAIL
41+
)
42+
).toBe(false)
43+
})
44+
45+
it('defaults to false when no verification claim is present (typical Azure AD token)', () => {
46+
expect(deriveMicrosoftEmailVerified({ name: 'User', oid: 'abc' }, EMAIL)).toBe(false)
47+
})
48+
49+
it('defaults to false for an empty claim set', () => {
50+
expect(deriveMicrosoftEmailVerified({}, EMAIL)).toBe(false)
51+
})
52+
53+
it('coerces a truthy non-boolean email_verified claim', () => {
54+
expect(deriveMicrosoftEmailVerified({ email_verified: 'true' }, EMAIL)).toBe(true)
55+
})
56+
57+
it('treats a malformed verified-email claim as unverified', () => {
58+
expect(deriveMicrosoftEmailVerified({ verified_primary_email: 'not-an-array' }, EMAIL)).toBe(
59+
false
60+
)
61+
})
62+
})
63+
64+
describe('isMicrosoftProvider', () => {
65+
it('recognizes Microsoft connector provider IDs', () => {
66+
expect(isMicrosoftProvider('microsoft-ad')).toBe(true)
67+
expect(isMicrosoftProvider('outlook')).toBe(true)
68+
})
69+
70+
it('rejects non-Microsoft provider IDs', () => {
71+
expect(isMicrosoftProvider('google')).toBe(false)
72+
expect(isMicrosoftProvider('microsoft')).toBe(false)
73+
})
74+
})

apps/sim/lib/oauth/microsoft.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,3 +19,24 @@ export function isMicrosoftProvider(providerId: string): boolean {
1919
export function getMicrosoftRefreshTokenExpiry(): Date {
2020
return new Date(Date.now() + MICROSOFT_REFRESH_TOKEN_LIFETIME_DAYS * 24 * 60 * 60 * 1000)
2121
}
22+
23+
/**
24+
* Derives whether a Microsoft ID token proves ownership of `email`. Azure AD's
25+
* `email`/`upn` claims are unverified and mutable on multi-tenant (`/common/`)
26+
* endpoints, so the email is trusted only when the token explicitly proves it via
27+
* the `email_verified` claim or the verified-email claims, mirroring Better
28+
* Auth's built-in Microsoft provider. Defaults to `false` when no claim asserts
29+
* verification, so an attacker-controlled tenant can never assert a verified
30+
* email it does not own.
31+
*/
32+
export function deriveMicrosoftEmailVerified(
33+
claims: Record<string, unknown>,
34+
email: string
35+
): boolean {
36+
if (claims.email_verified !== undefined) {
37+
return Boolean(claims.email_verified)
38+
}
39+
const verifiedPrimaryEmail = claims.verified_primary_email as string[] | undefined
40+
const verifiedSecondaryEmail = claims.verified_secondary_email as string[] | undefined
41+
return Boolean(verifiedPrimaryEmail?.includes(email) || verifiedSecondaryEmail?.includes(email))
42+
}

0 commit comments

Comments
 (0)