diff --git a/modules/auth/controllers/auth.controller.js b/modules/auth/controllers/auth.controller.js index 893429d68..89fc9121f 100644 --- a/modules/auth/controllers/auth.controller.js +++ b/modules/auth/controllers/auth.controller.js @@ -115,8 +115,31 @@ const signup = async (req, res) => { } return responses.error(res, 404, 'Signup error', 'Registration is currently deactivated')(); } - // Force default role on public signup — clients must not self-assign admin - const safeBody = { ...req.body, roles: ['user'] }; + // Force default role on public signup — clients must not self-assign admin. + // Defense-in-depth against mass assignment: the SignupUser route schema already + // rejects server-owned keys (.strict()), but UserService.create does NO whitelist + // filtering, so we ALSO scrub the body here. Force roles + emailVerified, and delete + // every server-owned field a client could otherwise seed (provider identity, reset / + // verification tokens, lockout counters). emailVerified:true would self-verify the + // account and defeat the OAuth-annexation guard (linkProviderByEmail matches on + // emailVerified:true); a pre-seeded providerData enables identity hijack. + // `referredBy` is accepted by SignupUser (preventing a .strict() 422 on invite paths + // that may send it) but ALWAYS deleted here — the server sets it via the invite + // finalize seam so a client can never self-assign a referrer. + const safeBody = { ...req.body, roles: ['user'], emailVerified: false }; + for (const serverOwned of [ + 'providerData', + 'additionalProvidersData', + 'resetPasswordToken', + 'resetPasswordExpires', + 'emailVerificationToken', + 'emailVerificationExpires', + 'failedLoginAttempts', + 'lockUntil', + 'lastLoginAt', + 'currentOrganization', + 'referredBy', + ]) delete safeBody[serverOwned]; // Invite-gated signup: canonicalize the account email to the invite's pinned // (lowercased) email. Enforces the pin exactly AND makes the case-insensitive // unique-email index (email_ci_unique, collation strength-2) a reliable single-use backstop — concurrent case-variant diff --git a/modules/auth/routes/auth.routes.js b/modules/auth/routes/auth.routes.js index ca5267748..204ea41d3 100644 --- a/modules/auth/routes/auth.routes.js +++ b/modules/auth/routes/auth.routes.js @@ -74,7 +74,11 @@ export default (app) => { app.route('/api/auth/reset').post(authLimiter, authPassword.reset); // Setting up the users authentication api - app.route('/api/auth/signup').post(authLimiter, model.isValid(UsersSchema.User), auth.signup); + // Validate signup against the restricted, `.strict()` SignupUser schema (NOT the full + // User schema): for a POST, model.isValid replaces req.body with the FULL parsed + // output, so any field on the schema is client-writable — using SignupUser limits that + // write surface to the safe client-settable fields and rejects server-owned keys. + app.route('/api/auth/signup').post(authLimiter, model.isValid(UsersSchema.SignupUser), auth.signup); app.route('/api/auth/signin').post(authLimiter, auth.signinAuthenticate, auth.signin); // Signout: intentionally no JWT middleware — must clear the cookie even if the // token is expired/invalid/missing so the client cannot get silently re-logged in. diff --git a/modules/auth/tests/auth.integration.tests.js b/modules/auth/tests/auth.integration.tests.js index 37352c9ce..952025a29 100644 --- a/modules/auth/tests/auth.integration.tests.js +++ b/modules/auth/tests/auth.integration.tests.js @@ -189,6 +189,55 @@ describe('Auth integration tests:', () => { } }); + test('should reject signup that carries server-owned fields (mass assignment)', async () => { + // A public signup may only set client-settable fields. Posting server-owned + // fields (emailVerified, providerData, reset/verification tokens, roles) must be + // REJECTED by the restricted `.strict()` SignupUser schema — never silently + // persisted. Self-verifying defeats the OAuth-annexation guard; a pre-seeded + // providerData enables account hijack. The request is refused (422) AND no + // account is created. + const attackerEmail = 'massassign_attacker_@test.com'; + try { + const existing = await UserService.getBrut({ email: attackerEmail }); + if (existing) await UserService.remove(existing); + } catch (_) { /* cleanup */ } + + try { + await agent + .post('/api/auth/signup') + .send({ + firstName: 'Mallory', + lastName: 'Attacker', + email: attackerEmail, + password: credentials[0].password, + roles: ['admin'], + emailVerified: true, + providerData: { email: attackerEmail, sub: 'attacker-injected-sub' }, + additionalProvidersData: { google: { sub: 'attacker-injected-sub' } }, + resetPasswordToken: 'attacker-reset-token', + resetPasswordExpires: new Date().toISOString(), + emailVerificationToken: 'attacker-verify-token', + emailVerificationExpires: new Date().toISOString(), + failedLoginAttempts: 99, + lockUntil: new Date().toISOString(), + lastLoginAt: new Date().toISOString(), + currentOrganization: 'attacker-org-id', + }) + .expect(422); + + // The restricted schema rejected the request — no account was created. + const persisted = await UserService.getBrut({ email: attackerEmail }); + expect(persisted == null).toBe(true); + } catch (err) { + expect(err).toBeFalsy(); + } finally { + try { + const leftover = await UserService.getBrut({ email: attackerEmail }); + if (leftover) await UserService.remove(leftover); + } catch (_) { /* cleanup */ } + } + }); + test('should reject registration when email is already in use', async () => { // Init user edited _userEdited.email = 'register_new_user_@test.com'; diff --git a/modules/auth/tests/auth.silent.catch.unit.tests.js b/modules/auth/tests/auth.silent.catch.unit.tests.js index 4f8aed206..e0e8055eb 100644 --- a/modules/auth/tests/auth.silent.catch.unit.tests.js +++ b/modules/auth/tests/auth.silent.catch.unit.tests.js @@ -152,6 +152,183 @@ describe('auth.controller silent-catch error logging:', () => { }); }); +describe('auth.controller signup mass-assignment strip:', () => { + beforeEach(() => { + jest.resetModules(); + + jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ + default: { warn: jest.fn(), error: jest.fn(), info: jest.fn() }, + })); + }); + + test('should scrub server-owned fields and force roles/emailVerified before create', async () => { + // Defense-in-depth: even if a body reaches the controller carrying server-owned + // fields (route schema bypassed / future-relaxed), UserService.create — which does + // NO whitelisting — must receive a scrubbed body. Force roles + emailVerified:false + // and drop providerData / reset+verification tokens / lockout counters. + const mockCreate = jest.fn().mockResolvedValue({ + id: 'u1', email: 'x@y.com', firstName: 'A', lastName: 'B', provider: 'local', + }); + + jest.unstable_mockModule('../../../modules/users/services/users.service.js', () => ({ + default: { + create: mockCreate, + getBrut: jest.fn().mockResolvedValue({ id: 'u1' }), + update: jest.fn().mockResolvedValue({}), + remove: jest.fn(), + count: jest.fn().mockResolvedValue(0), + }, + })); + + jest.unstable_mockModule('../../../modules/auth/services/auth.eligibility.js', () => ({ + default: { + registerSignupEligibility: jest.fn(), + assertSignupEligible: jest.fn().mockResolvedValue(undefined), + _reset: jest.fn(), + }, + })); + + jest.unstable_mockModule('../../../modules/organizations/services/organizations.service.js', () => ({ + default: { + handleSignupOrganization: jest.fn().mockResolvedValue({ + organization: null, joined: false, pendingJoin: false, + abilities: [], organizationSetupRequired: false, + emailVerificationRequired: false, suggestedOrganization: null, + }), + }, + })); + + jest.unstable_mockModule('../../../modules/organizations/services/organizations.crud.service.js', () => ({ + default: { autoSetCurrentOrganization: jest.fn() }, + })); + + jest.unstable_mockModule('../../../modules/organizations/services/organizations.membership.service.js', () => ({ + default: { findByUserAndOrganization: jest.fn(), listPendingByUser: jest.fn().mockResolvedValue([]) }, + })); + + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { + sign: { up: true, in: true }, + jwt: { secret: 'test-secret', expiresIn: 3600 }, + cookie: { secure: false, sameSite: 'lax' }, + organizations: { enabled: false }, + app: { title: 'Test', contact: 'test@test.com' }, + }, + })); + + jest.unstable_mockModule('../../../lib/middlewares/model.js', () => ({ + default: { getResultFromZod: jest.fn(), checkError: jest.fn() }, + })); + + // Mailer off — signup auto-verifies AFTER create via a separate update; that path + // does not affect the body handed to create, which is what we assert here. + jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({ + default: { + isConfigured: jest.fn().mockReturnValue(false), + sendMail: jest.fn().mockResolvedValue({ accepted: ['x@y.com'] }), + }, + })); + + jest.unstable_mockModule('../../../lib/helpers/responses.js', () => ({ + default: { + success: jest.fn().mockReturnValue(jest.fn()), + error: jest.fn().mockReturnValue(jest.fn()), + }, + })); + + jest.unstable_mockModule('../../../lib/helpers/errors.js', () => ({ + default: { getMessage: jest.fn().mockReturnValue('error') }, + })); + + jest.unstable_mockModule('../../../lib/helpers/AppError.js', () => ({ + default: class AppError extends Error { + constructor(msg, opts) { + super(msg); + this.code = opts?.code; + this.details = opts?.details; + } + }, + })); + + jest.unstable_mockModule('../../../modules/users/models/users.schema.js', () => ({ + default: { User: {}, SignupUser: {} }, + })); + + jest.unstable_mockModule('../../../lib/middlewares/policy.js', () => ({ + default: { defineAbilityFor: jest.fn().mockResolvedValue({}) }, + })); + + jest.unstable_mockModule('../../../lib/helpers/abilities.js', () => ({ + default: jest.fn().mockReturnValue([]), + })); + + jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({ + default: jest.fn().mockReturnValue('http://localhost:3000'), + })); + + jest.unstable_mockModule('../../../lib/services/analytics.js', () => ({ + default: { identify: jest.fn(), groupIdentify: jest.fn(), capture: jest.fn() }, + })); + + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + + const req = { + body: { + email: 'x@y.com', + firstName: 'A', + lastName: 'B', + password: 'P@ss1234!', + // Server-owned fields an attacker might inject if validation were bypassed: + roles: ['admin'], + emailVerified: true, + providerData: { sub: 'attacker-sub' }, + additionalProvidersData: { google: { sub: 'attacker-sub' } }, + resetPasswordToken: 'attacker-reset', + resetPasswordExpires: new Date(), + emailVerificationToken: 'attacker-verify', + emailVerificationExpires: new Date(), + failedLoginAttempts: 99, + lockUntil: new Date(), + lastLoginAt: new Date(), + // referredBy is accepted by SignupUser schema (to avoid .strict() 422 on invite + // paths) but must be stripped by the controller — server owns it via invite finalize. + referredBy: '64b2f0000000000000000999', + }, + query: {}, + }; + const res = { + status: jest.fn().mockReturnThis(), + cookie: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), + }; + + await AuthController.signup(req, res); + + expect(mockCreate).toHaveBeenCalledTimes(1); + const createdWith = mockCreate.mock.calls[0][0]; + expect(createdWith.roles).toEqual(['user']); + expect(createdWith.emailVerified).toBe(false); + for (const field of [ + 'providerData', + 'additionalProvidersData', + 'resetPasswordToken', + 'resetPasswordExpires', + 'emailVerificationToken', + 'emailVerificationExpires', + 'failedLoginAttempts', + 'lockUntil', + 'lastLoginAt', + 'currentOrganization', + 'referredBy', + ]) { + expect(Object.prototype.hasOwnProperty.call(createdWith, field)).toBe(false); + } + // Legitimate client fields survive + expect(createdWith.email).toBe('x@y.com'); + expect(createdWith.firstName).toBe('A'); + }); +}); + describe('auth.password.controller silent-catch error logging:', () => { let mockWarn; let mockError; diff --git a/modules/users/models/users.schema.js b/modules/users/models/users.schema.js index 32f441cc6..78054ead6 100644 --- a/modules/users/models/users.schema.js +++ b/modules/users/models/users.schema.js @@ -53,23 +53,72 @@ const User = z.object({ // Referral substrate (#5) — `referredBy` is DELIBERATELY NOT declared here. // This is a server-only field set on invite acceptance (the invitations finalize // seam, via UserService.updateById, which bypasses Zod + the update whitelist). - // The signup route validates the body with `model.isValid(User)`, and for POST it - // replaces req.body with the FULL Zod output — so any field present in this schema - // becomes client-writable on signup. Omitting `referredBy` makes Zod strip it from - // every client-facing parse (signup POST + PUT /api/users), guaranteeing a client - // can never self-assign a referrer. Do NOT add it here. (The Mongoose model + the - // updateById raw path are all the server needs to persist it.) - // When a future feature (e.g. the P8b referrals view) needs to READ/EXPOSE - // `referredBy`, do it via a response projection / the read whitelist — NEVER by - // adding it to this Zod schema (which is also the signup-POST write surface, so - // adding it here reintroduces the client-writable hole this omission closes). + // + // Architecture note: the signup route validates the body with `model.isValid(SignupUser)` + // (not `User`), and for POST replaces req.body with the full Zod output — so only fields + // declared on `SignupUser` reach the controller. `SignupUser` deliberately accepts + // `referredBy` (to avoid a .strict() 422 on invite-path clients) but the controller + // DELETES it before UserService.create so a client can never self-assign a referrer. + // See SignupUser JSDoc below and the controller's defense-in-depth strip for details. + // + // `User` still omits `referredBy` here to protect the `UserUpdate`/PUT surface and the + // OAuth-bootstrap path (both validated with `User`). Do NOT add `referredBy` to `User`. + // The Mongoose model + the updateById raw path are all the server needs to persist it. + // When a future feature needs to READ/EXPOSE `referredBy`, do it via a response + // projection / the read whitelist — NEVER by adding it to this schema. // others complementary: z.record(z.string(), z.unknown()).nullable().optional(), }); const UserUpdate = User.partial(); +/** + * Public signup write surface. + * + * The signup route validates the body with `model.isValid(...)`, and for a POST that + * REPLACES req.body with the FULL Zod output — so EVERY field declared on the parsed + * schema becomes client-writable on signup. Validating signup with the full `User` + * schema therefore lets a public client self-assign server-owned fields + * (emailVerified, providerData/additionalProvidersData, reset/verification tokens, + * lockout counters, roles) — a mass-assignment hole: emailVerified:true self-verifies + * and defeats the OAuth-annexation guard, and a pre-seeded providerData enables hijack. + * + * `SignupUser` exposes ONLY the fields a public signup may legitimately set — the same + * safe surface encoded by `config.whitelists.users.default` / `.update` — and is + * `.strict()`, so any unknown / server-owned key is REJECTED (422) instead of silently + * persisted. Field DEFAULTS mirror `User` exactly (empty-string defaults, password + * `.default('')` + the shared strength refinement read from config) so the + * password-less / OAuth-bootstrap signup paths keep working. The controller adds a + * defense-in-depth strip on top, because the service layer does no whitelisting. + * + * `provider` is accepted (a local signup legitimately carries `provider:'local'`); it + * is NOT a privilege field on its own — without a client-writable providerData (omitted + * here) and with emailVerified forced false in the controller, a chosen provider value + * cannot annex an OAuth identity. `roles` is intentionally OMITTED: the controller + * forces `['user']`. + * + * `referredBy` is accepted (the invitation P8a test — and real clients on an invite + * flow — may send it to prove the server ignores it) but DELETED by the controller + * before UserService.create; the server always sets referredBy via the invite finalize + * seam (invitations module, not the signup body). Accepting it here prevents a .strict() + * 422 on invite-path signups while still guaranteeing a client can never self-assign. + */ +const SignupUser = z.object({ + firstName: User.shape.firstName, + lastName: User.shape.lastName, + bio: User.shape.bio, + position: User.shape.position, + email: User.shape.email, + avatar: User.shape.avatar, + provider: User.shape.provider, + password: User.shape.password, + terms: User.shape.terms, + complementary: User.shape.complementary, + referredBy: z.string().optional(), +}).strict(); + export default { User, UserUpdate, + SignupUser, };