diff --git a/MIGRATIONS.md b/MIGRATIONS.md index 750915cd4..b5e44dbe2 100644 --- a/MIGRATIONS.md +++ b/MIGRATIONS.md @@ -4,6 +4,52 @@ Breaking changes and upgrade notes for downstream projects. --- +## OAuth account linking + Express 5 callback fix (2026-04-23) + +Two related auth fixes that ship together. + +### 1. Express 5 GET callback no longer crashes + +Enabling Google OAuth on a downstream project used to crash on first signin with `Cannot read properties of undefined (reading 'strategy')`. Root cause: Express 5 leaves `req.body` as `undefined` on GET requests (Express 4 initialized it to `{}`). + +- `modules/auth/controllers/auth.controller.js` — `oauthCallback` optional-chains `req.body` access +- Apple OAuth (POST `form_post`) was never affected — no change to behavior + +### 2. Account linking by verified email + +Before: a local signup at `user@x.com` followed by a Google signin with the same email crashed on Mongo's unique-email index (E11000) — user locked out. + +Now: `checkOAuthUserProfile` follows a 4-step lookup: + +1. `(provider, providerData[key])` — primary identity (OAuth-first users) +2. `additionalProvidersData[provider][key]` — linked users on subsequent signins +3. `email` match **with provider-verified email** → atomic link (`UserService.linkProviderByEmail`) +4. No match → create new user with `emailVerified` reflecting provider verification + +Linking attaches the OAuth `providerData` under `user.additionalProvidersData[provider]` and **does not overwrite `user.provider`** — so password reset (gated on `provider === 'local'`) and local login keep working for linked users. + +### Security gates + +- Provider + key allowlists (`ALLOWED_PROVIDERS = {google, apple}`, `ALLOWED_PROVIDER_KEYS = {id, sub, email}`) validate the dynamic query path before Mongo. +- `emailVerifiedByProvider: true` required before linking — prevents takeover via a future OIDC provider that returns `email_verified: false` for someone else's address. +- `/token` response sanitizes `accessToken` / `refreshToken` out of `additionalProvidersData` before serialization. + +### Action for downstream + +1. Run `/update-stack` to pull both fixes in one go. +2. Env vars to set in prod K8s for Google (per project that wants OAuth enabled): + - `DEVKIT_NODE_oAuth_google_clientID` + - `DEVKIT_NODE_oAuth_google_clientSecret` + - `DEVKIT_NODE_oAuth_google_callbackURL` — e.g. `https://api.{project}.{tld}/api/auth/google/callback` +3. Register the callback URL in Google Cloud Console (OAuth 2.0 client, Web type). For Apple: same pattern on `decodedIdToken.email_verified`. +4. `/api/auth/config` returns `oAuth.google: true` once the clientID is set — the Vue signin/signup buttons activate automatically via `serverConfig.oAuth.google`. + +### Schema note + +`additionalProvidersData` already existed in the Mongoose user schema and is now exposed in the Zod user schema too. No Mongo migration needed — existing users have an empty field. + +--- + ## Analytics: request-aware feature-flag helpers (2026-04-23) `analytics` service gains two sugar helpers that extract the PostHog `distinctId` from an Express request, so routes no longer need to repeat `req.user?.id ?? req.sessionID ?? 'anonymous'` (and can never forget the anonymous fallback): diff --git a/modules/auth/controllers/auth.controller.js b/modules/auth/controllers/auth.controller.js index 62f78306d..2361f4989 100644 --- a/modules/auth/controllers/auth.controller.js +++ b/modules/auth/controllers/auth.controller.js @@ -234,6 +234,25 @@ const signin = async (req, res) => { * @param {Object} res - Express response object * TODO: escape deprecated */ +/** + * @desc Strip OAuth tokens from an additionalProvidersData map before serializing to client. + * Only the provider identity fields are safe to expose; access/refresh tokens must stay server-side. + * @param {Object|undefined} apd - raw additionalProvidersData + * @returns {Object|undefined} sanitized map with accessToken/refreshToken removed per provider + */ +const sanitizeAdditionalProvidersData = (apd) => { + if (!apd || typeof apd !== 'object') return undefined; + const sanitized = {}; + for (const [prov, data] of Object.entries(apd)) { + if (data && typeof data === 'object') { + // eslint-disable-next-line no-unused-vars + const { accessToken, refreshToken, ...safe } = data; + sanitized[prov] = safe; + } + } + return Object.keys(sanitized).length > 0 ? sanitized : undefined; +}; + const token = async (req, res) => { let user = null; if (req.user) { @@ -248,7 +267,7 @@ const token = async (req, res) => { email: req.user.email, lastName: req.user.lastName, firstName: req.user.firstName, - additionalProvidersData: req.user.additionalProvidersData, + additionalProvidersData: sanitizeAdditionalProvidersData(req.user.additionalProvidersData), emailVerified: req.user.emailVerified, currentOrganization: req.user.currentOrganization, lastLoginAt: req.user.lastLoginAt, @@ -295,13 +314,32 @@ const oauthCall = (req, res, next) => { }; /** - * @desc Endpoint to save oAuthProfile + * Known OAuth providers — used to validate the `provider` argument and `key` argument + * before constructing dynamic query paths, preventing prototype-pollution-style injections. + */ +const ALLOWED_PROVIDERS = new Set(['google', 'apple']); +const ALLOWED_PROVIDER_KEYS = new Set(['sub', 'id', 'email']); + +/** + * @desc Resolve or create a user from an OAuth profile. Lookup order: + * 1. Primary identity (provider + providerData[key]) + * 2. Linked identity (additionalProvidersData[provider][key]) + * 3. Link-on-verified-email (provider-verified email matches an existing local user) + * 4. Create new user * @param {Object} profil - OAuth user profile object - * @param {string} key - Provider key to lookup providerData - * @param {string} provider - OAuth provider name + * @param {string} key - Provider key to lookup providerData (must be in ALLOWED_PROVIDER_KEYS) + * @param {string} provider - OAuth provider name (must be in ALLOWED_PROVIDERS) + * @returns {Promise} sanitized user document (existing, linked, or newly created) */ const checkOAuthUserProfile = async (profil, key, provider) => { - // check if user exist + // Guard: validate provider and key against allowlists before using as dynamic object keys + if (!ALLOWED_PROVIDERS.has(provider)) { + throw new AppError('oAuth, unsupported provider', { code: 'VALIDATION_ERROR', details: { provider } }); + } + if (!ALLOWED_PROVIDER_KEYS.has(key)) { + throw new AppError('oAuth, unsupported provider key', { code: 'VALIDATION_ERROR', details: { key } }); + } + // 1. Primary identity: match on (provider, providerData[key]) — OAuth-first users try { const query = {}; query[`providerData.${key}`] = profil.providerData[key]; @@ -311,7 +349,28 @@ const checkOAuthUserProfile = async (profil, key, provider) => { } catch (err) { throw new AppError('oAuth, find user failed', { code: 'SERVICE_ERROR', details: err }); } - // if no, generate + // 2. Linked identity: match on additionalProvidersData[provider][key] — locals already linked + try { + const query = {}; + query[`additionalProvidersData.${provider}.${key}`] = profil.providerData[key]; + const search = await UserService.search(query); + if (search.length === 1) return search[0]; + } catch (err) { + throw new AppError('oAuth, find linked user failed', { code: 'SERVICE_ERROR', details: err }); + } + // 3. Link on verified email: if a local user exists with the same email and the OAuth + // provider vouches for it, attach providerData under additionalProvidersData.{provider} + // without overwriting user.provider (keeps password reset + local login intact). + // Atomic findOneAndUpdate avoids TOCTOU races between concurrent OAuth callbacks. + if (profil.email && profil.emailVerifiedByProvider) { + try { + const linked = await UserService.linkProviderByEmail(profil.email, provider, profil.providerData); + if (linked) return linked; + } catch (err) { + throw new AppError('oAuth, link to existing user failed', { code: 'SERVICE_ERROR', details: err }); + } + } + // 4. No match → create new user try { const user = { firstName: profil.firstName, @@ -320,6 +379,7 @@ const checkOAuthUserProfile = async (profil, key, provider) => { avatar: profil.avatar || '', provider, providerData: profil.providerData || null, + emailVerified: !!profil.emailVerifiedByProvider, }; const result = model.getResultFromZod(user, UsersSchema.User); // check error diff --git a/modules/auth/strategies/local/apple.js b/modules/auth/strategies/local/apple.js index 98e11e439..bff5ba68e 100644 --- a/modules/auth/strategies/local/apple.js +++ b/modules/auth/strategies/local/apple.js @@ -12,12 +12,14 @@ const callbackURL = `${config.api.protocol}://${config.api.host}${config.api.por }/auth/apple/callback`; /** - * @desc function to prepare map callback to user profile - * @param {req} - * @param {accessToken} - * @param {refreshToken} - * @param {profile} - * @param {cb} callback + * @desc Map Apple OAuth callback to user profile and delegate to checkOAuthUserProfile + * @param {Object} req - Express request (passReqToCallback) + * @param {string} accessToken - Apple access token + * @param {string} refreshToken - Apple refresh token + * @param {Object} decodedIdToken - Decoded Apple ID token + * @param {Object} profile - Apple profile (may be empty on repeat sign-ins) + * @param {Function} cb - Passport callback (err, user) + * @returns {Promise} */ const prepare = async (req, accessToken, refreshToken, decodedIdToken, profile, cb) => { // Set the provider data and include tokens @@ -35,6 +37,7 @@ const prepare = async (req, accessToken, refreshToken, decodedIdToken, profile, avatar: null, provider: 'apple', providerData, + emailVerifiedByProvider: decodedIdToken.email_verified === true || decodedIdToken.email_verified === 'true', }; // Save the user OAuth profile try { diff --git a/modules/auth/strategies/local/google.js b/modules/auth/strategies/local/google.js index a01f4dece..7ce3b24ca 100644 --- a/modules/auth/strategies/local/google.js +++ b/modules/auth/strategies/local/google.js @@ -12,11 +12,12 @@ const callbackURL = `${config.api.protocol}://${config.api.host}${config.api.por }/auth/google/callback`; /** - * @desc function to prepare map callback to user profile - * @param {accessToken} - * @param {refreshToken} - * @param {profile} - * @param {cb} callback + * @desc Map Google OAuth callback to user profile and delegate to checkOAuthUserProfile + * @param {string} accessToken - Google access token + * @param {string} refreshToken - Google refresh token + * @param {Object} profile - Google profile object + * @param {Function} cb - Passport callback (err, user) + * @returns {Promise} */ const prepare = async (accessToken, refreshToken, profile, cb) => { // Set the provider data and include tokens @@ -31,6 +32,7 @@ const prepare = async (accessToken, refreshToken, profile, cb) => { avatar: providerData.picture ? providerData.picture : undefined, provider: 'google', providerData, + emailVerifiedByProvider: providerData.email_verified === true, }; // Save the user OAuth profile try { diff --git a/modules/auth/tests/auth.integration.tests.js b/modules/auth/tests/auth.integration.tests.js index 0cc6cca58..8344941e8 100644 --- a/modules/auth/tests/auth.integration.tests.js +++ b/modules/auth/tests/auth.integration.tests.js @@ -673,6 +673,183 @@ describe('Auth integration tests:', () => { } catch (_) { /* cleanup – ignore errors */ } }); + test('should link OAuth signin to existing local user when provider verifies email', async () => { + // Seed a local user with a password (provider=local), email not yet linked via OAuth + const localEmail = 'oauthlink-local@test.com'; + const localUser = await UserService.create({ + firstName: 'Local', + lastName: 'Link', + email: localEmail, + password: credentials[0].password, + provider: 'local', + roles: ['user'], + }); + + // OAuth signin arrives with matching email + provider-verified flag + const profil = { + firstName: 'Local', + lastName: 'Link', + email: localEmail, + avatar: '', + providerData: { sub: 'google-link-sub-12345', email_verified: true }, + emailVerifiedByProvider: true, + }; + const linked = await AuthController.checkOAuthUserProfile(profil, 'sub', 'google'); + + expect(linked).toBeDefined(); + expect(linked.id).toBe(localUser.id); + expect(linked.provider).toBe('local'); // provider kept so password reset still works + expect(linked.emailVerified).toBe(true); + // Verify additionalProvidersData was persisted (getBrut bypasses the sanitize whitelist) + const brutLinked = await UserService.getBrut({ id: linked.id }); + expect(brutLinked.additionalProvidersData?.google?.sub).toBe('google-link-sub-12345'); + + // Subsequent signin with the same Google sub should find the linked user via step 2 + const second = await AuthController.checkOAuthUserProfile(profil, 'sub', 'google'); + expect(second.id).toBe(localUser.id); + + try { await UserService.remove(linked); } catch (_) { /* cleanup */ } + }); + + test('should NOT link when OAuth provider did not verify the email', async () => { + const sharedEmail = 'oauthlink-unverified@test.com'; + const localUser = await UserService.create({ + firstName: 'Unverified', + lastName: 'Link', + email: sharedEmail, + password: credentials[0].password, + provider: 'local', + roles: ['user'], + }); + + // OAuth arrives for the SAME email but email_verified=false — must not link + const profil = { + firstName: 'Unverified', + lastName: 'Link', + email: sharedEmail, + avatar: '', + providerData: { sub: 'google-unverified-sub-999', email_verified: false }, + emailVerifiedByProvider: false, + }; + // Falls through to create branch → duplicate email → unique-index error + await expect( + AuthController.checkOAuthUserProfile(profil, 'sub', 'google'), + ).rejects.toThrow(); + // Local account must remain untouched — no OAuth data attached + const untouched = await UserService.getBrut({ email: sharedEmail }); + expect(untouched).toBeDefined(); + expect(untouched.additionalProvidersData?.google).toBeUndefined(); + + try { await UserService.remove(localUser); } catch (_) { /* cleanup */ } + }); + + test('should reject link when local email matches but OAuth provider did not verify (no takeover)', async () => { + const sharedEmail = 'oauthlink-takeover@test.com'; + const localUser = await UserService.create({ + firstName: 'Victim', + lastName: 'User', + email: sharedEmail, + password: credentials[0].password, + provider: 'local', + roles: ['user'], + }); + + // Attacker tries OAuth with same email but provider says email_verified=false + const profil = { + firstName: 'Attacker', + lastName: 'User', + email: sharedEmail, + avatar: '', + providerData: { sub: 'google-attacker-sub-42', email_verified: false }, + emailVerifiedByProvider: false, + }; + // Must error — unverified email falls to create branch → duplicate email → AppError + await expect( + AuthController.checkOAuthUserProfile(profil, 'sub', 'google'), + ).rejects.toMatchObject({ code: 'CONTROLLER_ERROR' }); + // Verify the local account was NOT modified — exactly one user with this email, no OAuth data + const users = await UserService.search({ email: sharedEmail }); + expect(users.length).toBe(1); + expect(users[0].additionalProvidersData).toBeUndefined(); + + try { await UserService.remove(localUser); } catch (_) { /* cleanup */ } + }); + + test('should set emailVerified=true when creating a fresh OAuth user with verified email', async () => { + const profil = { + firstName: 'Fresh', + lastName: 'OAuth', + email: 'oauth-fresh@test.com', + avatar: '', + providerData: { sub: 'google-fresh-sub-55', email_verified: true }, + emailVerifiedByProvider: true, + }; + const created = await AuthController.checkOAuthUserProfile(profil, 'sub', 'google'); + expect(created.emailVerified).toBe(true); + expect(created.provider).toBe('google'); + + try { await UserService.remove(created); } catch (_) { /* cleanup */ } + }); + + test('should throw VALIDATION_ERROR for unsupported OAuth provider', async () => { + const profil = { + firstName: 'Test', + lastName: 'User', + email: 'test@test.com', + avatar: '', + providerData: { sub: 'bad-sub' }, + emailVerifiedByProvider: false, + }; + await expect( + AuthController.checkOAuthUserProfile(profil, 'sub', 'badprovider'), + ).rejects.toMatchObject({ code: 'VALIDATION_ERROR' }); + }); + + test('should throw VALIDATION_ERROR for unsupported provider key', async () => { + const profil = { + firstName: 'Test', + lastName: 'User', + email: 'test@test.com', + avatar: '', + providerData: { badkey: 'value' }, + emailVerifiedByProvider: false, + }; + await expect( + AuthController.checkOAuthUserProfile(profil, 'badkey', 'google'), + ).rejects.toMatchObject({ code: 'VALIDATION_ERROR' }); + }); + + test('token endpoint should strip accessToken/refreshToken from additionalProvidersData', async () => { + // Create a local user then sign in to get an auth cookie + const localEmail = 'token-sanitize@test.com'; + const localPassword = 'W@os.jsI$Aw3$0m3'; + const localUser = await UserService.create({ + firstName: 'Token', + lastName: 'Sanitize', + email: localEmail, + password: localPassword, + provider: 'local', + roles: ['user'], + }); + // Manually inject additionalProvidersData with tokens (simulating a linked OAuth account) + const brutUser = await UserService.getBrut({ email: localEmail }); + await UserService.update(brutUser, { + additionalProvidersData: { + google: { sub: 'google-sub-sanitize', accessToken: 'secret-token', refreshToken: 'secret-refresh', email_verified: true }, + }, + }, 'recover'); + + // Sign in as this user and call the token endpoint (agent persists the session cookie) + await agent.post('/api/auth/signin').send({ email: localEmail, password: localPassword }).expect(200); + const tokenResult = await agent.get('/api/auth/token').expect(200); + + expect(tokenResult.body.user.additionalProvidersData?.google?.sub).toBe('google-sub-sanitize'); + expect(tokenResult.body.user.additionalProvidersData?.google?.accessToken).toBeUndefined(); + expect(tokenResult.body.user.additionalProvidersData?.google?.refreshToken).toBeUndefined(); + + try { await UserService.remove(localUser); } catch (_) { /* cleanup */ } + }); + afterAll(async () => { for (const u of oauthUsers) { try { diff --git a/modules/users/config/users.development.config.js b/modules/users/config/users.development.config.js index d3d3d0558..2f9411617 100644 --- a/modules/users/config/users.development.config.js +++ b/modules/users/config/users.development.config.js @@ -29,7 +29,7 @@ const config = { ], update: ['firstName', 'lastName', 'bio', 'position', 'email', 'avatar', 'complementary'], updateAdmin: ['firstName', 'lastName', 'bio', 'position', 'email', 'avatar', 'roles', 'complementary'], - recover: ['password', 'resetPasswordToken', 'resetPasswordExpires', 'emailVerified', 'emailVerificationToken', 'emailVerificationExpires'], + recover: ['password', 'resetPasswordToken', 'resetPasswordExpires', 'emailVerified', 'emailVerificationToken', 'emailVerificationExpires', 'additionalProvidersData'], roles: ['user', 'admin'], }, }, diff --git a/modules/users/models/users.schema.js b/modules/users/models/users.schema.js index eb19bef26..ba0f58437 100644 --- a/modules/users/models/users.schema.js +++ b/modules/users/models/users.schema.js @@ -24,6 +24,7 @@ const User = z.object({ /* Provider */ provider: z.string().optional(), providerData: z.record(z.string(), z.unknown()).optional(), + additionalProvidersData: z.record(z.string(), z.record(z.string(), z.unknown())).optional(), /* Password */ password: z.string() .max(config.zxcvbn.maxSize) diff --git a/modules/users/repositories/users.repository.js b/modules/users/repositories/users.repository.js index c5415582b..68ca668cc 100644 --- a/modules/users/repositories/users.repository.js +++ b/modules/users/repositories/users.repository.js @@ -191,6 +191,21 @@ const findWithFilter = (filter, select) => User.find(filter).select(select || '' */ const updateMany = (filter, data) => User.updateMany(filter, data, { runValidators: true }).exec(); +/** + * @desc Atomically attach an OAuth provider to an existing user matched by email. + * Uses findOneAndUpdate to avoid TOCTOU races between concurrent OAuth callbacks. + * @param {string} email - The email to match + * @param {string} provider - The OAuth provider key (e.g. 'google', 'apple') + * @param {Object} providerData - The provider's identity data to store + * @returns {Promise} Updated user document or null if no match + */ +const linkProviderByEmail = (email, provider, providerData) => + User.findOneAndUpdate( + { email }, + { $set: { [`additionalProvidersData.${provider}`]: providerData, emailVerified: true } }, + { returnDocument: 'after', runValidators: true }, + ).exec(); + export default { list, create, @@ -206,4 +221,5 @@ export default { findByIdAndUpdatePopulated, findWithFilter, updateMany, + linkProviderByEmail, }; diff --git a/modules/users/services/users.service.js b/modules/users/services/users.service.js index c5904c4b0..5287e216d 100644 --- a/modules/users/services/users.service.js +++ b/modules/users/services/users.service.js @@ -183,6 +183,19 @@ const searchByNameOrEmail = (search) => UserRepository.searchByNameOrEmail(searc */ const findByEmail = (email) => UserRepository.findByEmail(email); +/** + * @desc Atomically attach an OAuth provider to an existing user matched by email. + * Uses a single findOneAndUpdate to avoid TOCTOU races between concurrent OAuth callbacks. + * @param {string} email - The email to match + * @param {string} provider - The OAuth provider key (e.g. 'google', 'apple') + * @param {Object} providerData - The provider's identity data to store + * @returns {Promise} sanitized updated user or null if no match + */ +const linkProviderByEmail = async (email, provider, providerData) => { + const result = await UserRepository.linkProviderByEmail(email, provider, providerData); + return result ? removeSensitive(result) : null; +}; + export default { list, create, @@ -198,5 +211,6 @@ export default { findByIdAndUpdatePopulated, searchByNameOrEmail, findByEmail, + linkProviderByEmail, removeSensitive, };