diff --git a/ERRORS.md b/ERRORS.md index 3e2de1447..6b8583d01 100644 --- a/ERRORS.md +++ b/ERRORS.md @@ -30,3 +30,4 @@ Use this file as a compact memory of recurring AI mistakes. - [2026-05-31] billing/stripe: reading `price.metadata.planId` in `customer.subscription.updated` webhook handler -> field is EMPTY in real Stripe webhook payloads (planId lives on the Product, not the Price); use a `priceId → plan` map built at boot from `config.stripe.prices` instead; see pierreb-devkit/Node#3742 - [2026-06-04] repository: top-level `const Foo = mongoose.model('Foo')` in a repository file -> this is evaluated at import time; safe in an HTTP server (loadModels() runs first) but silently crashes standalone scripts (crons, migrations) with `MissingSchemaError` when import order differs; tests miss it because jest mocks intercept the module entirely; fix = lazy getter `const Foo = () => mongoose.model('Foo')` (call sites: `Foo().find(...)`) or dynamic import after `loadModels()` in the entrypoint; see pierreb-devkit/Node#3789 - [2026-06-15] deps/audit: leaving `npm audit` advisories unaddressed on the assumption they need a major bump -> run `npm audit fix` (never `--force`) first; the runtime-tree DoS/ReDoS items (`qs`, `path-to-regexp`, `brace-expansion`) all fixed via in-range bumps, no residual. These are DoS-class but NOT attacker-reachable in this stack: Express route patterns are static (no user-controlled `path-to-regexp` input) and `qs`/`brace-expansion` only parse server-side query strings under fixed code paths — still bump them to keep the tree clean and avoid scanner noise. +- [2026-07-16] security: `users.repository.js findByIdAndUpdatePopulated()` does `.populate()` with no `.select()`, so `organizations.controller.js switchOrganization` serialized the raw doc (password hash + OAuth tokens + reset/verification tokens) straight to the client; `users.account.controller.js me()` separately forwarded `providerData` (OAuth tokens) verbatim -> any endpoint returning a Mongoose user doc must go through `UserService.removeSensitive()` (whitelist, `modules/users/utils/sanitizeUser.js`) at the response boundary, never serialize `req.user`/a populated doc directly; a local-signup fixture's `providerData` defaults to `{}` so a naive falsy check won't catch this — seed a fake OAuth token in tests to prove the leak is actually closed; see pierreb-devkit/Node#3963 diff --git a/modules/organizations/controllers/organizations.controller.js b/modules/organizations/controllers/organizations.controller.js index 3e6b70158..4a4919af5 100644 --- a/modules/organizations/controllers/organizations.controller.js +++ b/modules/organizations/controllers/organizations.controller.js @@ -12,6 +12,7 @@ import serializeAbilities from '../../../lib/helpers/abilities.js'; import OrganizationsService from '../services/organizations.crud.service.js'; import MembershipService from '../services/organizations.membership.service.js'; import AnalyticsService from '../../../lib/services/analytics.js'; +import UserService from '../../users/services/users.service.js'; const tokenCookieOptions = { httpOnly: true, @@ -257,7 +258,8 @@ const switchOrganization = async (req, res) => { expiresIn: config.jwt.expiresIn, }); - // Build abilities for the new org context + // Build abilities for the new org context — uses the raw doc (needs + // user.roles/_id only), sanitization happens below, at the response boundary. const ability = await policy.defineAbilityFor(updatedUser, membership); const abilities = serializeAbilities(ability); @@ -268,7 +270,10 @@ const switchOrganization = async (req, res) => { type: 'success', message: 'organization switched', data: { - user: updatedUser, + // updatedUser comes straight off an unselected findByIdAndUpdate().populate() + // (users.repository.js) and carries password/providerData/reset+verification + // tokens — sanitize before serializing (#3963). + user: UserService.removeSensitive(updatedUser), abilities, tokenExpiresIn: Date.now() + config.jwt.expiresIn * 1000, }, diff --git a/modules/organizations/tests/organizations.controller.unit.tests.js b/modules/organizations/tests/organizations.controller.unit.tests.js index 59c7925e8..071e9434e 100644 --- a/modules/organizations/tests/organizations.controller.unit.tests.js +++ b/modules/organizations/tests/organizations.controller.unit.tests.js @@ -4,12 +4,14 @@ import { jest, describe, test, expect, beforeEach } from '@jest/globals'; const mockCrudRemove = jest.fn(); +const mockCrudSwitchOrganization = jest.fn(); const mockListByUser = jest.fn(); const mockLeave = jest.fn(); jest.unstable_mockModule('../services/organizations.crud.service.js', () => ({ default: { remove: mockCrudRemove, + switchOrganization: mockCrudSwitchOrganization, }, })); @@ -26,6 +28,19 @@ jest.unstable_mockModule('../../../lib/services/analytics.js', () => ({ }, })); +// Mock the users service boundary with the REAL sanitizeUser.removeSensitive +// (pure — lodash + config only, no mongoose) rather than a stub, so this test +// exercises actual whitelist-based stripping (#3963) instead of merely +// asserting a mock got called. Mocking the module at all is required because +// users.service.js -> users.repository.js does `mongoose.model('User')` at +// import time, which throws outside a bootstrapped app (see ERRORS.md 2026-06-04). +const { removeSensitive } = await import('../../users/utils/sanitizeUser.js'); +jest.unstable_mockModule('../../users/services/users.service.js', () => ({ + default: { + removeSensitive, + }, +})); + const { default: organizationsController } = await import('../controllers/organizations.controller.js'); /** @@ -54,6 +69,7 @@ describe('Organizations controller unit tests:', () => { const res = {}; res.status = jest.fn().mockReturnValue(res); res.json = jest.fn().mockReturnValue(res); + res.cookie = jest.fn().mockReturnValue(res); return res; } @@ -120,6 +136,92 @@ describe('Organizations controller unit tests:', () => { }); }); + describe('switchOrganization', () => { + /** + * @desc Build a fake Mongoose-like updated-user document, the shape + * `UserService.findByIdAndUpdatePopulated` returns with no `.select()` — + * every sensitive field a real doc could carry, plus the fields the + * response legitimately needs. + * @returns {Object} fake mongoose document with a `toJSON` method + */ + function fakeUpdatedUserDoc() { + const plain = { + _id: 'u1', + id: 'u1', + email: 'switcher@test.com', + roles: ['user'], + firstName: 'Switch', + lastName: 'User', + currentOrganization: 'org2', + password: '$2b$10$leakedHashShouldNeverReachClient', + providerData: { accessToken: 'leaked-access-token', refreshToken: 'leaked-refresh-token' }, + additionalProvidersData: { google: { accessToken: 'leaked-additional-token' } }, + resetPasswordToken: 'leaked-reset-token', + resetPasswordExpires: new Date(), + emailVerificationToken: 'leaked-verification-token', + emailVerificationExpires: new Date(), + failedLoginAttempts: 3, + lockUntil: null, + }; + return { ...plain, toJSON: () => plain }; + } + + test('sanitizes the response user — strips password/providerData/reset tokens, keeps legit fields', async () => { + const updatedUser = fakeUpdatedUserDoc(); + mockCrudSwitchOrganization.mockResolvedValue({ + user: updatedUser, + membership: { role: 'owner', organizationId: 'org2' }, + }); + + const req = mockReq({ + params: { organizationId: 'org2' }, + organization: { _id: 'org2', id: 'org2' }, + }); + const res = mockRes(); + + await organizationsController.switchOrganization(req, res); + + expect(mockCrudSwitchOrganization).toHaveBeenCalledWith(req.user, 'org2'); + expect(res.status).toHaveBeenCalledWith(200); + const [payload] = res.json.mock.calls[0]; + const responseUser = payload.data.user; + + // Sensitive fields — must be ABSENT + expect(responseUser.password).toBeUndefined(); + expect(responseUser.providerData).toBeUndefined(); + expect(responseUser.additionalProvidersData).toBeUndefined(); + expect(responseUser.resetPasswordToken).toBeUndefined(); + expect(responseUser.resetPasswordExpires).toBeUndefined(); + expect(responseUser.emailVerificationToken).toBeUndefined(); + expect(responseUser.emailVerificationExpires).toBeUndefined(); + expect(responseUser.failedLoginAttempts).toBeUndefined(); + expect(responseUser.lockUntil).toBeUndefined(); + + // Legit fields — must be PRESENT + expect(responseUser.id).toBe('u1'); + expect(responseUser.email).toBe('switcher@test.com'); + expect(responseUser.roles).toEqual(['user']); + expect(responseUser.currentOrganization).toBe('org2'); + expect(responseUser.firstName).toBe('Switch'); + }); + + test('returns 403 when the user is not a member of the target organization', async () => { + const err = new Error('User is not a member of this organization'); + err.code = 'FORBIDDEN'; + mockCrudSwitchOrganization.mockRejectedValue(err); + + const req = mockReq({ + params: { organizationId: 'org2' }, + organization: { _id: 'org2', id: 'org2' }, + }); + const res = mockRes(); + + await organizationsController.switchOrganization(req, res); + + expect(res.status).toHaveBeenCalledWith(403); + }); + }); + describe('leave', () => { test('should call MembershipService.leave with user and org ids and return success', async () => { mockLeave.mockResolvedValue({ success: true }); diff --git a/modules/organizations/tests/organizations.integration.tests.js b/modules/organizations/tests/organizations.integration.tests.js index d530be89d..43940d6d5 100644 --- a/modules/organizations/tests/organizations.integration.tests.js +++ b/modules/organizations/tests/organizations.integration.tests.js @@ -206,6 +206,79 @@ describe('Organizations integration tests:', () => { }); }); + describe('POST /api/organizations/:organizationId/switch (#3963 — sanitized response)', () => { + let switchAgent; + let MembershipService; + let user; + let org2; + + beforeAll(async () => { + config.organizations = { enabled: true, autoCreate: true, domainMatching: false }; + switchAgent = request.agent((await bootstrap()).app); + MembershipService = (await import(path.resolve('./modules/organizations/services/organizations.membership.service.js'))).default; + + const signupRes = await switchAgent + .post('/api/auth/signup') + .send({ + firstName: 'Switch', + lastName: 'User', + email: 'switch-3963@test.com', + password: 'W@os.jsI$Aw3$0m3', + provider: 'local', + }) + .expect(200); + user = signupRes.body.user; + + // A second organization the user OWNS (owner role needed — `switch` maps to + // the CASL `create` action on Organization, which only the owner's `manage` + // wildcard grants), so the switch has somewhere valid to go. + org2 = await OrganizationsRepository.create({ name: 'Switch Target Org', slug: `switch-target-3963-${Date.now()}` }); + await MembershipService.create({ userId: user._id || user.id, organizationId: org2._id, role: 'owner' }); + + // Simulate what a real production user document carries — a linked OAuth + // account and a stray reset token — so the assertions below prove the + // response strips them rather than merely "happening" not to have them. + await UserService.updateById(user.id, { + providerData: { accessToken: 'leaked-switch-access-token', refreshToken: 'leaked-switch-refresh-token' }, + resetPasswordToken: 'leaked-switch-reset-token', + resetPasswordExpires: new Date(Date.now() + 3600000), + }); + }); + + test('response user must not leak password/providerData/reset tokens, and must keep legit fields', async () => { + const result = await switchAgent.post(`/api/organizations/${org2._id}/switch`).expect(200); + + expect(result.body.type).toBe('success'); + expect(result.body.message).toBe('organization switched'); + const responseUser = result.body.data.user; + expect(responseUser).toBeInstanceOf(Object); + + // Sensitive fields — must be ABSENT + expect(responseUser.password).toBeUndefined(); + expect(responseUser.providerData).toBeUndefined(); + expect(responseUser.additionalProvidersData).toBeUndefined(); + expect(responseUser.resetPasswordToken).toBeUndefined(); + expect(responseUser.resetPasswordExpires).toBeUndefined(); + expect(responseUser.emailVerificationToken).toBeUndefined(); + expect(responseUser.emailVerificationExpires).toBeUndefined(); + expect(responseUser.failedLoginAttempts).toBeUndefined(); + expect(responseUser.lockUntil).toBeUndefined(); + expect(JSON.stringify(result.body)).not.toContain('leaked-switch-access-token'); + expect(JSON.stringify(result.body)).not.toContain('leaked-switch-refresh-token'); + expect(JSON.stringify(result.body)).not.toContain('leaked-switch-reset-token'); + + // Legit fields — must be PRESENT and correct + expect(responseUser.id).toBe(String(user.id)); + expect(responseUser.email).toBe(user.email); + expect(responseUser.roles).toBeInstanceOf(Array); + expect(String(responseUser.currentOrganization._id || responseUser.currentOrganization)).toBe(String(org2._id)); + }); + + afterAll(async () => { + await cleanupUser(user); + }); + }); + // Mongoose disconnect afterAll(async () => { config.organizations = { ...originalOrganizations }; diff --git a/modules/users/controllers/users.account.controller.js b/modules/users/controllers/users.account.controller.js index fd191552b..1f43ff487 100644 --- a/modules/users/controllers/users.account.controller.js +++ b/modules/users/controllers/users.account.controller.js @@ -59,6 +59,8 @@ const remove = async (req, res) => { const me = (req, res) => { // Sanitize the user - short term solution. Copied from core.controller.js // TODO create proper passport mock: See https://gist.github.com/mweibel/5219403 + // providerData (OAuth access+refresh tokens, see auth/strategies/local/{google,apple}.js) + // is intentionally NOT whitelisted here — never forward it to the client (#3963). let user = null; if (req.user) { user = { @@ -69,7 +71,6 @@ const me = (req, res) => { email: req.user.email, lastName: req.user.lastName, firstName: req.user.firstName, - providerData: req.user.providerData, // others complementary: req.user.complementary, }; diff --git a/modules/users/tests/user.account.integration.tests.js b/modules/users/tests/user.account.integration.tests.js index 686432924..055fe9fa7 100644 --- a/modules/users/tests/user.account.integration.tests.js +++ b/modules/users/tests/user.account.integration.tests.js @@ -198,6 +198,32 @@ describe('User integration tests:', () => { } }); + // #3963: providerData holds OAuth access+refresh tokens (see + // auth/strategies/local/{google,apple}.js). A local-signup user's providerData + // is an empty object, which would trivially pass a naive "is it falsy" check — + // simulate a linked OAuth account (real tokens) to prove the leak is actually + // closed, not just absent because this fixture never had anything to leak. + test('should NOT include providerData (OAuth tokens) in /me response', async () => { + try { + await UserService.updateById(user.id, { + providerData: { accessToken: 'leaked-me-access-token', refreshToken: 'leaked-me-refresh-token' }, + }); + + const result = await agent.get('/api/users/me').expect(200); + expect(result.body.data.providerData).toBeUndefined(); + expect(JSON.stringify(result.body.data)).not.toContain('leaked-me-access-token'); + expect(JSON.stringify(result.body.data)).not.toContain('leaked-me-refresh-token'); + + // legit fields still present + expect(result.body.data.email).toBe(user.email); + expect(result.body.data.id).toBe(String(user.id)); + expect(result.body.data.roles).toBeInstanceOf(Array); + } catch (err) { + console.log(err); + expect(err).toBeFalsy(); + } + }); + test('should include terms in user details after signing them', async () => { // Sign terms first try {