fix(auth): restrict signup write-surface to client-settable fields (mass assignment)

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

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.

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';

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;

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,
 };