diff --git a/.github/workflows/build-and-test.yaml b/.github/workflows/build-and-test.yaml index 579e1633..d6f1344a 100644 --- a/.github/workflows/build-and-test.yaml +++ b/.github/workflows/build-and-test.yaml @@ -66,11 +66,51 @@ jobs: pnpm run build pnpm test - - name: Build web + # Web previously ran `build` only, so its unit and e2e suites never + # gated a merge. Lint, typecheck and unit tests are fast and + # deterministic, so they block. + - name: Install web dependencies run: | cd web pnpm install - pnpm run build + + - name: Lint web + run: | + cd web + pnpm lint + + - name: Typecheck web + run: | + cd web + pnpm typecheck + + - name: Unit test web + run: | + cd web + pnpm test + + - name: Build web + run: | + cd web + pnpm run build + + # E2e is fully mocked (e2e/mock-api.ts intercepts every backend call), + # but it drives a real browser, so it is reported without blocking until + # it has proven stable across a few merges. + # + # TODO: remove continue-on-error once that is established. It is here to + # avoid wedging the merge queue on a browser flake, not because e2e + # failures are acceptable. + - name: Install Playwright browser + run: | + cd web + pnpm exec playwright install --with-deps chromium + + - name: E2e test web (non-blocking for now) + continue-on-error: true + run: | + cd web + pnpm test:e2e build-and-test-android: name: Build and Test Android diff --git a/api/src/auth/auth.service.spec.ts b/api/src/auth/auth.service.spec.ts index d8eb49d1..653ffa96 100644 --- a/api/src/auth/auth.service.spec.ts +++ b/api/src/auth/auth.service.spec.ts @@ -1,141 +1,390 @@ -import { Test, TestingModule } from '@nestjs/testing' -import { getModelToken } from '@nestjs/mongoose' import { HttpException } from '@nestjs/common' import * as bcrypt from 'bcryptjs' import { AuthService } from './auth.service' -import { UsersService } from '../users/users.service' -import { JwtService } from '@nestjs/jwt' -import { MailService } from '../mail/mail.service' -import { TurnstileService } from '../common/turnstile.service' -import { ApiKey } from './schemas/api-key.schema' -import { PasswordReset } from './schemas/password-reset.schema' -import { AccessLog } from './schemas/access-log.schema' -import { EmailVerification } from './schemas/email-verification.schema' - -// Regression tests for CWE-640 password reset OTP brute-force protection. -describe('AuthService.resetPassword — OTP brute-force protection', () => { - let service: AuthService - - const buildUser = () => - ({ - _id: 'user-1', - email: 'victim@example.com', - password: 'existing-hash', - save: jest.fn().mockResolvedValue(undefined), - }) as any - - const buildResetDoc = async (rawOtp: string, overrides: any = {}) => { - const doc: any = { - user: 'user-1', - otp: await bcrypt.hash(rawOtp, 4), - expiresAt: new Date(Date.now() + 10 * 60 * 1000), - attempts: 0, - ...overrides, - } - doc.save = jest.fn().mockImplementation(() => Promise.resolve(doc)) - return doc + +// AuthService takes eight constructor deps. Only the ones a given flow +// touches are given real behaviour; the rest are inert stubs. +const build = () => { + let lastApiKeyDoc: any + + const apiKeyModel: any = jest.fn().mockImplementation((doc: any) => { + lastApiKeyDoc = { ...doc, save: jest.fn().mockResolvedValue(undefined) } + return lastApiKeyDoc + }) + apiKeyModel.findOne = jest.fn() + apiKeyModel.findById = jest.fn() + + const usersService = { + findOne: jest.fn(), + findOneWithPassword: jest.fn(), + create: jest.fn(), + } + const passwordResetModel = { findOne: jest.fn(), findOneAndUpdate: jest.fn() } + const mailService = { sendEmailFromTemplate: jest.fn().mockResolvedValue(undefined) } + const jwtService = { sign: jest.fn().mockReturnValue('signed-jwt') } + const turnstileService = { verify: jest.fn().mockResolvedValue(undefined) } + + const service = new AuthService( + usersService as any, + jwtService as any, + apiKeyModel, + passwordResetModel as any, + {} as any, // accessLogModel + {} as any, // emailVerificationModel + mailService as any, + turnstileService as any, + ) + + return { + service, + apiKeyModel, + usersService, + passwordResetModel, + mailService, + jwtService, + turnstileService, + getLastApiKeyDoc: () => lastApiKeyDoc, } +} + +describe('AuthService', () => { + describe('validateEmail', () => { + it('accepts a well-formed address', async () => { + const { service } = build() + await expect(service.validateEmail('a@b.com')).resolves.toBeUndefined() + }) + + it.each(['plainaddress', 'no-at-sign.com', 'missing@dot', '@no-local.com'])( + 'rejects %s', + async (bad) => { + const { service } = build() + await expect(service.validateEmail(bad)).rejects.toThrow(HttpException) + }, + ) + }) + + describe('validatePassword', () => { + it('rejects a password shorter than 6 characters', async () => { + const { service } = build() + await expect(service.validatePassword('12345')).rejects.toThrow(HttpException) + }) + + it('accepts the 6 and 128 character boundaries', async () => { + const { service } = build() + await expect(service.validatePassword('123456')).resolves.toBeUndefined() + await expect(service.validatePassword('a'.repeat(128))).resolves.toBeUndefined() + }) + + it('rejects a password longer than 128 characters', async () => { + const { service } = build() + await expect(service.validatePassword('a'.repeat(129))).rejects.toThrow( + HttpException, + ) + }) + }) + + describe('generateApiKey', () => { + it('returns a raw key and persists only a masked value plus a bcrypt hash', async () => { + const { service, getLastApiKeyDoc } = build() + + const result = await service.generateApiKey({ _id: 'user_1' } as any) - const mockUsersService = { findOne: jest.fn() } - const mockJwtService = { sign: jest.fn() } - const mockMailService = { sendEmailFromTemplate: jest.fn() } - const mockTurnstileService = { verify: jest.fn() } - const mockPasswordResetModel = { findOne: jest.fn() } - const emptyModel = {} - - beforeEach(async () => { - const module: TestingModule = await Test.createTestingModule({ - providers: [ - AuthService, - { provide: UsersService, useValue: mockUsersService }, - { provide: JwtService, useValue: mockJwtService }, - { provide: MailService, useValue: mockMailService }, - { provide: TurnstileService, useValue: mockTurnstileService }, - { provide: getModelToken(ApiKey.name), useValue: emptyModel }, - { - provide: getModelToken(PasswordReset.name), - useValue: mockPasswordResetModel, - }, - { provide: getModelToken(AccessLog.name), useValue: emptyModel }, - { - provide: getModelToken(EmailVerification.name), - useValue: emptyModel, - }, - ], - }).compile() - - service = module.get(AuthService) - jest.clearAllMocks() + expect(typeof result.apiKey).toBe('string') + const doc = getLastApiKeyDoc() + // The stored apiKey is masked: it must not be the raw key. + expect(doc.apiKey).not.toBe(result.apiKey) + expect(doc.apiKey.endsWith('*'.repeat(18))).toBe(true) + expect(doc.apiKey.startsWith(result.apiKey.substr(0, 17))).toBe(true) + // The stored hash is a bcrypt hash of the raw key, never the raw key. + expect(doc.hashedApiKey).not.toBe(result.apiKey) + expect(bcrypt.compareSync(result.apiKey, doc.hashedApiKey)).toBe(true) + expect(doc.user).toBe('user_1') + expect(doc.save).toHaveBeenCalledTimes(1) + }) }) - it('locks out the reset record after 5 wrong OTP attempts (brute-force prevention)', async () => { - const user = buildUser() - const reset = await buildResetDoc('123456') - mockUsersService.findOne.mockResolvedValue(user) - mockPasswordResetModel.findOne.mockResolvedValue(reset) + describe('findActiveApiKeyByClientKey', () => { + const revokedClause = { + $or: [{ revokedAt: null }, { revokedAt: { $exists: false } }], + } + + it('resolves via the exact masked match when one exists', async () => { + const { service, apiKeyModel } = build() + const hit = { apiKey: 'masked', user: 'user_1' } + apiKeyModel.findOne.mockResolvedValueOnce(hit) + + const raw = 'abcdefghijklmnopqrstuvwxyz' + const found = await service.findActiveApiKeyByClientKey(raw) + + expect(found).toBe(hit) + expect(apiKeyModel.findOne).toHaveBeenCalledTimes(1) + expect(apiKeyModel.findOne).toHaveBeenCalledWith({ + apiKey: `${raw.substring(0, 17)}${'*'.repeat(18)}`, + ...revokedClause, + }) + }) + + it('falls back to a prefix regex when there is no masked match', async () => { + const { service, apiKeyModel } = build() + const hit = { apiKey: 'legacy', user: 'user_1' } + apiKeyModel.findOne.mockResolvedValueOnce(null).mockResolvedValueOnce(hit) + + const raw = 'abcdefghijklmnopqrstuvwxyz' + const found = await service.findActiveApiKeyByClientKey(raw) + + expect(found).toBe(hit) + expect(apiKeyModel.findOne).toHaveBeenCalledTimes(2) + const fallbackArg = apiKeyModel.findOne.mock.calls[1][0] + expect(fallbackArg.apiKey.$regex).toBeInstanceOf(RegExp) + // A legitimate prefix matches its own masked value. + expect(fallbackArg.apiKey.$regex.test(raw.substring(0, 17))).toBe(true) + // The revoked-key exclusion is applied on both lookups. + expect(fallbackArg.$or).toEqual(revokedClause.$or) + }) + + it('does not throw when the key contains regex metacharacters', async () => { + const { service, apiKeyModel } = build() + apiKeyModel.findOne.mockResolvedValue(null) // no masked hit -> regex fallback + + const raw = '((((' + 'x'.repeat(20) + await expect(service.findActiveApiKeyByClientKey(raw)).resolves.toBeNull() + + // The prefix is escaped before it reaches the RegExp, so the parens are + // literal and the pattern compiles instead of throwing SyntaxError. + const fallbackArg = apiKeyModel.findOne.mock.calls[1][0] + expect(fallbackArg.apiKey.$regex.source).toContain('\\(') + }) + }) + + describe('changePassword', () => { + const withOldPassword = async (old: string) => { + const ctx = build() + const stored = { + _id: 'user_1', + password: bcrypt.hashSync(old, 10), + save: jest.fn().mockResolvedValue(undefined), + } + ctx.usersService.findOneWithPassword.mockResolvedValue(stored) + return { ...ctx, stored } + } + + it('rejects a wrong old password without saving', async () => { + const { service, stored } = await withOldPassword('correct-old') - // 5 wrong attempts — each should be rejected. The 5th one must consume - // the reset record so no further attempts (right OR wrong) can succeed. - for (let i = 0; i < 5; i++) { await expect( - service.resetPassword({ - email: user.email, - otp: '000000', - newPassword: 'a-new-password', - }), - ).rejects.toBeInstanceOf(HttpException) + service.changePassword( + { oldPassword: 'wrong', newPassword: 'a-brand-new-password' }, + { _id: 'user_1' } as any, + ), + ).rejects.toThrow(HttpException) + expect(stored.save).not.toHaveBeenCalled() + }) + + it('updates the hash on success', async () => { + const { service, stored } = await withOldPassword('correct-old') + const before = stored.password + + await service.changePassword( + { oldPassword: 'correct-old', newPassword: 'a-brand-new-password' }, + { _id: 'user_1' } as any, + ) + + expect(stored.save).toHaveBeenCalledTimes(1) + expect(stored.password).not.toBe(before) + expect(bcrypt.compareSync('a-brand-new-password', stored.password)).toBe(true) + }) + }) + + describe('register input validation', () => { + const registerSetup = () => { + const ctx = build() + ctx.usersService.findOne.mockResolvedValue(null) // no existing user + // If validation is (incorrectly) skipped, register would reach create; + // return a usable doc so the pre-fix path resolves rather than erroring. + ctx.usersService.create.mockResolvedValue({ + _id: 'user_1', + email: 'x', + lastLoginAt: null, + save: jest.fn().mockResolvedValue(undefined), + toObject: () => ({ _id: 'user_1' }), + }) + return ctx } - // After the lockout, submitting the CORRECT otp must still be rejected — - // the record has been invalidated by the failed-attempts counter. - mockPasswordResetModel.findOne.mockResolvedValue(reset) - await expect( - service.resetPassword({ - email: user.email, - otp: '123456', - newPassword: 'a-new-password', - }), - ).rejects.toBeInstanceOf(HttpException) - - // Password must not have been rewritten by any of the above. - expect(user.save).not.toHaveBeenCalled() - // The reset record must have been persisted with attempts incremented - // (i.e., the failed-attempt tracking is durable, not in-memory only). - expect(reset.save).toHaveBeenCalled() - expect(reset.attempts).toBeGreaterThanOrEqual(5) + it('rejects a malformed email and does not create the user', async () => { + const { service, usersService } = registerSetup() + + await expect( + service.register({ + name: 'Ada', + email: 'not-an-email', + password: 'a-valid-password', + turnstileToken: 'token', + }), + ).rejects.toThrow(HttpException) + expect(usersService.create).not.toHaveBeenCalled() + }) + + it('rejects a too-short password and does not create the user', async () => { + const { service, usersService } = registerSetup() + + await expect( + service.register({ + name: 'Ada', + email: 'a@b.com', + password: '123', + turnstileToken: 'token', + }), + ).rejects.toThrow(HttpException) + expect(usersService.create).not.toHaveBeenCalled() + }) + + it('creates the user when email and password are valid', async () => { + const { service, usersService } = registerSetup() + + await service.register({ + name: 'Ada', + email: 'a@b.com', + password: 'a-valid-password', + turnstileToken: 'token', + }) + expect(usersService.create).toHaveBeenCalledTimes(1) + }) }) - it('accepts the correct OTP when attempts are below the limit', async () => { - const user = buildUser() - const reset = await buildResetDoc('654321', { attempts: 2 }) - mockUsersService.findOne.mockResolvedValue(user) - mockPasswordResetModel.findOne.mockResolvedValue(reset) - - await expect( - service.resetPassword({ - email: user.email, - otp: '654321', - newPassword: 'a-new-password', - }), - ).resolves.toEqual(expect.objectContaining({ message: expect.any(String) })) - - expect(user.save).toHaveBeenCalled() + describe('changePassword input validation', () => { + it('rejects a too-short new password without saving', async () => { + const ctx = build() + const stored = { + _id: 'user_1', + password: bcrypt.hashSync('correct-old', 10), + save: jest.fn().mockResolvedValue(undefined), + } + ctx.usersService.findOneWithPassword.mockResolvedValue(stored) + + await expect( + ctx.service.changePassword( + { oldPassword: 'correct-old', newPassword: '123' }, + { _id: 'user_1' } as any, + ), + ).rejects.toThrow(HttpException) + expect(stored.save).not.toHaveBeenCalled() + }) }) - it('rejects immediately if the record is already locked out (attempts >= limit)', async () => { - const user = buildUser() - const reset = await buildResetDoc('654321', { attempts: 5 }) - mockUsersService.findOne.mockResolvedValue(user) - mockPasswordResetModel.findOne.mockResolvedValue(reset) - - await expect( - service.resetPassword({ - email: user.email, - otp: '654321', // correct OTP - newPassword: 'a-new-password', - }), - ).rejects.toBeInstanceOf(HttpException) - - expect(user.save).not.toHaveBeenCalled() + describe('resetPassword', () => { + const setup = () => { + const ctx = build() + const user = { + _id: 'user_1', + email: 'a@b.com', + name: 'Ada', + password: 'old-hash', + save: jest.fn().mockResolvedValue(undefined), + } + ctx.usersService.findOne.mockResolvedValue(user) + return { ...ctx, user } + } + + const MAX_ATTEMPTS = 5 + + const buildReset = (rawOtp: string, attempts = 0) => ({ + _id: 'reset_1', + otp: bcrypt.hashSync(rawOtp, 10), + expiresAt: new Date(Date.now() + 60_000), + attempts, + save: jest.fn().mockResolvedValue(undefined), + }) + + // The service reads the newest record, then claims an attempt against it + // with an atomic findOneAndUpdate. This models that server-side guard: + // no match once the record is at the cap, otherwise increment and return. + const stageReset = (ctx: ReturnType, reset: any) => { + ctx.passwordResetModel.findOne.mockResolvedValue(reset) + ctx.passwordResetModel.findOneAndUpdate.mockImplementation(async () => { + if (reset.attempts >= MAX_ATTEMPTS) return null + reset.attempts += 1 + return reset + }) + } + + const submit = (ctx: ReturnType, otp: string) => + ctx.service.resetPassword({ + email: 'a@b.com', + otp, + newPassword: 'new-password', + }) + + it('rejects when there is no valid reset record', async () => { + const ctx = setup() + ctx.passwordResetModel.findOne.mockResolvedValue(null) + + await expect(submit(ctx, '1234')).rejects.toThrow(HttpException) + expect(ctx.user.save).not.toHaveBeenCalled() + }) + + it('rejects when the OTP does not match', async () => { + const ctx = setup() + stageReset(ctx, buildReset('9999')) + + await expect(submit(ctx, '1234')).rejects.toThrow(HttpException) + expect(ctx.user.save).not.toHaveBeenCalled() + }) + + it('updates the password and expires the reset on success', async () => { + const ctx = setup() + const reset = buildReset('1234') + stageReset(ctx, reset) + + const res = await submit(ctx, '1234') + + expect(ctx.user.save).toHaveBeenCalledTimes(1) + expect(bcrypt.compareSync('new-password', ctx.user.password)).toBe(true) + expect(reset.save).toHaveBeenCalledTimes(1) + // The reset window is closed (expiry moved to now or earlier). + expect(reset.expiresAt.getTime()).toBeLessThanOrEqual(Date.now()) + expect(res.message).toMatch(/reset/i) + }) + + it('locks the record out after 5 wrong OTPs, including against the correct one', async () => { + const ctx = setup() + const reset = buildReset('1234') + stageReset(ctx, reset) + + for (let i = 0; i < MAX_ATTEMPTS; i++) { + await expect(submit(ctx, '0000')).rejects.toThrow(HttpException) + } + // The correct OTP must not rescue an exhausted record. + await expect(submit(ctx, '1234')).rejects.toThrow(HttpException) + + expect(ctx.user.save).not.toHaveBeenCalled() + expect(reset.attempts).toBe(MAX_ATTEMPTS) + }) + + it('rejects the correct OTP when the record is already at the cap', async () => { + const ctx = setup() + stageReset(ctx, buildReset('1234', MAX_ATTEMPTS)) + + await expect(submit(ctx, '1234')).rejects.toThrow(HttpException) + expect(ctx.user.save).not.toHaveBeenCalled() + }) + + it('claims each attempt atomically so parallel guesses cannot bypass the cap', async () => { + const ctx = setup() + stageReset(ctx, buildReset('1234')) + + await expect(submit(ctx, '0000')).rejects.toThrow(HttpException) + + // A read-modify-write counter would let concurrent requests all observe + // the same count, so the increment has to happen in the query itself. + const [filter, update] = + ctx.passwordResetModel.findOneAndUpdate.mock.calls[0] + expect(update).toEqual({ $inc: { attempts: 1 } }) + expect(filter._id).toBe('reset_1') + expect(filter.$or).toEqual([ + { attempts: { $lt: MAX_ATTEMPTS } }, + { attempts: { $exists: false } }, + ]) + }) }) }) diff --git a/api/src/auth/auth.service.ts b/api/src/auth/auth.service.ts index 3e48df46..27ae323f 100644 --- a/api/src/auth/auth.service.ts +++ b/api/src/auth/auth.service.ts @@ -14,6 +14,7 @@ import { } from './schemas/password-reset.schema' import { MailService } from '../mail/mail.service' import { TurnstileService } from '../common/turnstile.service' +import { escapeRegExp } from '../common/escape-regexp' import { RequestResetPasswordInputDTO, ResetPasswordInputDTO } from './auth.dto' import { AccessLog } from './schemas/access-log.schema' import { @@ -21,6 +22,15 @@ import { EmailVerificationDocument, } from './schemas/email-verification.schema' +// Failed OTP submissions allowed against a single password reset record. +const MAX_PASSWORD_RESET_ATTEMPTS = 5 + +// For register and login, which hold the hash in memory before responding. +export const withoutPassword = (user: UserDocument) => { + const { password, ...safe } = user.toObject() + return safe +} + @Injectable() export class AuthService { constructor( @@ -39,7 +49,9 @@ export class AuthService { async login(userData: any) { await this.turnstileService.verify(userData.turnstileToken) - const user = await this.usersService.findOne({ email: userData.email }) + const user = await this.usersService.findOneWithPassword({ + email: userData.email, + }) if (!user) { throw new HttpException( { error: 'Invalid credentials' }, @@ -60,7 +72,7 @@ export class AuthService { const payload = { email: user.email, sub: user._id } return { accessToken: this.jwtService.sign(payload), - user, + user: withoutPassword(user), } } @@ -101,7 +113,7 @@ export class AuthService { const payload = { email: user.email, sub: user._id } return { accessToken: this.jwtService.sign(payload), - user, + user: withoutPassword(user), } } @@ -118,8 +130,8 @@ export class AuthService { ) } - this.validateEmail(userData.email) - this.validatePassword(userData.password) + await this.validateEmail(userData.email) + await this.validatePassword(userData.password) const hashedPassword = await bcrypt.hash(userData.password, 10) const { turnstileToken, ...sanitizedUserData } = userData @@ -140,7 +152,7 @@ export class AuthService { return { accessToken: this.jwtService.sign(payload), - user, + user: withoutPassword(user), } } @@ -194,17 +206,12 @@ export class AuthService { return { message: 'Password reset email sent' } } - // Reject after this many failed OTP submissions against the same record. - // A 6-digit OTP has ~1e6 possibilities and lives for 20 minutes, so we - // cap online guessing well below what an attacker would need to succeed. - private static readonly MAX_PASSWORD_RESET_ATTEMPTS = 5 - async resetPassword({ email, otp, newPassword }: ResetPasswordInputDTO) { const user = await this.usersService.findOne({ email }) if (!user) { throw new HttpException({ error: 'User not found' }, HttpStatus.NOT_FOUND) } - const passwordReset = await this.passwordResetModel.findOne( + const latestReset = await this.passwordResetModel.findOne( { user: user._id, expiresAt: { $gt: new Date() }, @@ -213,34 +220,27 @@ export class AuthService { { sort: { createdAt: -1 } }, ) - if (!passwordReset) { + if (!latestReset) { throw new HttpException({ error: 'Invalid OTP' }, HttpStatus.BAD_REQUEST) } - // Consume the record if the attempt limit has already been hit. This - // prevents an attacker from brute-forcing a 6-digit OTP within the - // 20-minute window: after MAX_PASSWORD_RESET_ATTEMPTS wrong guesses the - // record is invalidated and a fresh reset request is required. - if ( - (passwordReset.attempts ?? 0) >= AuthService.MAX_PASSWORD_RESET_ATTEMPTS - ) { - passwordReset.expiresAt = new Date(Date.now()) - await passwordReset.save() - throw new HttpException({ error: 'Invalid OTP' }, HttpStatus.BAD_REQUEST) - } + // Claim an attempt atomically so concurrent guesses cannot all read the + // same count and slip past the cap. A null result means the record is + // already locked out and a fresh reset request is required. + const passwordReset = await this.passwordResetModel.findOneAndUpdate( + { + _id: latestReset._id, + // Records predating this counter have no attempts field at all. + $or: [ + { attempts: { $lt: MAX_PASSWORD_RESET_ATTEMPTS } }, + { attempts: { $exists: false } }, + ], + }, + { $inc: { attempts: 1 } }, + { new: true }, + ) - if (!(await bcrypt.compare(otp, passwordReset.otp))) { - // Track the failed attempt durably so retries across requests count - // against the same lockout budget. - passwordReset.attempts = (passwordReset.attempts ?? 0) + 1 - if ( - passwordReset.attempts >= AuthService.MAX_PASSWORD_RESET_ATTEMPTS - ) { - // Invalidate the record on the last failed attempt so a subsequent - // correct-guess submission cannot succeed. - passwordReset.expiresAt = new Date(Date.now()) - } - await passwordReset.save() + if (!passwordReset || !(await bcrypt.compare(otp, passwordReset.otp))) { throw new HttpException({ error: 'Invalid OTP' }, HttpStatus.BAD_REQUEST) } @@ -272,7 +272,9 @@ export class AuthService { input: { oldPassword: string; newPassword: string }, user: UserDocument, ) { - const userToUpdate = await this.usersService.findOne({ _id: user._id }) + const userToUpdate = await this.usersService.findOneWithPassword({ + _id: user._id, + }) if (!userToUpdate) { throw new HttpException({ error: 'User not found' }, HttpStatus.NOT_FOUND) } @@ -283,7 +285,7 @@ export class AuthService { ) } - this.validatePassword(input.newPassword) + await this.validatePassword(input.newPassword) const hashedPassword = await bcrypt.hash(input.newPassword, 10) userToUpdate.password = hashedPassword @@ -432,7 +434,7 @@ export class AuthService { if (byMasked) { return byMasked } - const regex = new RegExp(`^${prefix}`, 'g') + const regex = new RegExp(`^${escapeRegExp(prefix)}`, 'g') return this.apiKeyModel.findOne({ apiKey: { $regex: regex }, ...revokedClause, diff --git a/api/src/auth/guards/auth.guard.spec.ts b/api/src/auth/guards/auth.guard.spec.ts new file mode 100644 index 00000000..a5e937cf --- /dev/null +++ b/api/src/auth/guards/auth.guard.spec.ts @@ -0,0 +1,117 @@ +import { HttpException } from '@nestjs/common' +import { ExecutionContext } from '@nestjs/common' +import { JwtService } from '@nestjs/jwt' +import { AuthGuard } from './auth.guard' +import { AuthService } from '../auth.service' +import { UsersService } from '../../users/users.service' +import * as bcrypt from 'bcryptjs' + +// Build a minimal ExecutionContext whose HTTP request is `request`. +const contextFor = (request: any): ExecutionContext => + ({ + switchToHttp: () => ({ getRequest: () => request }), + }) as unknown as ExecutionContext + +describe('AuthGuard', () => { + let guard: AuthGuard + let jwtService: { verify: jest.Mock } + let usersService: { findOne: jest.Mock } + let authService: { + findActiveApiKeyByClientKey: jest.Mock + trackAccessLog: jest.Mock + } + + const user = { _id: 'user_1', id: 'user_1' } + + beforeEach(() => { + jwtService = { verify: jest.fn() } + usersService = { findOne: jest.fn() } + authService = { + findActiveApiKeyByClientKey: jest.fn(), + trackAccessLog: jest.fn(), + } + guard = new AuthGuard( + jwtService as unknown as JwtService, + usersService as unknown as UsersService, + authService as unknown as AuthService, + ) + }) + + describe('bearer token', () => { + it('resolves the user for a valid bearer token', async () => { + jwtService.verify.mockReturnValue({ sub: 'user_1' }) + usersService.findOne.mockResolvedValue(user) + const request: any = { headers: { authorization: 'Bearer good' }, query: {} } + + await expect(guard.canActivate(contextFor(request))).resolves.toBe(true) + expect(usersService.findOne).toHaveBeenCalledWith({ _id: 'user_1' }) + expect(request.user).toBe(user) + }) + + it('throws 401 when the bearer token is invalid or expired', async () => { + jwtService.verify.mockImplementation(() => { + throw new Error('jwt expired') + }) + const request: any = { headers: { authorization: 'Bearer bad' }, query: {} } + + await expect(guard.canActivate(contextFor(request))).rejects.toThrow( + HttpException, + ) + expect(usersService.findOne).not.toHaveBeenCalled() + }) + }) + + describe('api key', () => { + it('resolves via x-api-key and attaches request.apiKey when the hash matches', async () => { + const apiKey = { user: 'user_1', hashedApiKey: 'hashed' } + authService.findActiveApiKeyByClientKey.mockResolvedValue(apiKey) + jest.spyOn(bcrypt, 'compareSync').mockReturnValue(true) + usersService.findOne.mockResolvedValue(user) + const request: any = { headers: { 'x-api-key': 'raw-key' }, query: {} } + + await expect(guard.canActivate(contextFor(request))).resolves.toBe(true) + expect(request.apiKey).toBe(apiKey) + expect(request.user).toBe(user) + }) + + it('rejects a key whose hash does not match', async () => { + const apiKey = { user: 'user_1', hashedApiKey: 'hashed' } + authService.findActiveApiKeyByClientKey.mockResolvedValue(apiKey) + jest.spyOn(bcrypt, 'compareSync').mockReturnValue(false) + const request: any = { headers: { 'x-api-key': 'raw-key' }, query: {} } + + await expect(guard.canActivate(contextFor(request))).rejects.toThrow( + HttpException, + ) + expect(usersService.findOne).not.toHaveBeenCalled() + }) + + it('rejects when no active api key is found', async () => { + authService.findActiveApiKeyByClientKey.mockResolvedValue(null) + const request: any = { headers: { 'x-api-key': 'raw-key' }, query: {} } + + await expect(guard.canActivate(contextFor(request))).rejects.toThrow( + HttpException, + ) + }) + }) + + it('throws 401 when no credentials are present', async () => { + const request: any = { headers: {}, query: {} } + await expect(guard.canActivate(contextFor(request))).rejects.toThrow( + HttpException, + ) + }) + + it('throws 401 when the token resolves a user id but the user no longer exists', async () => { + jwtService.verify.mockReturnValue({ sub: 'ghost' }) + usersService.findOne.mockResolvedValue(null) + const request: any = { headers: { authorization: 'Bearer good' }, query: {} } + + await expect(guard.canActivate(contextFor(request))).rejects.toThrow( + HttpException, + ) + }) + + afterEach(() => jest.restoreAllMocks()) +}) diff --git a/api/src/auth/guards/can-modify-api-key.guard.spec.ts b/api/src/auth/guards/can-modify-api-key.guard.spec.ts new file mode 100644 index 00000000..70b053de --- /dev/null +++ b/api/src/auth/guards/can-modify-api-key.guard.spec.ts @@ -0,0 +1,66 @@ +import { HttpException } from '@nestjs/common' +import { ExecutionContext } from '@nestjs/common' +import { CanModifyApiKey } from './can-modify-api-key.guard' +import { AuthService } from '../auth.service' +import { UserRole } from '../../users/user-roles.enum' + +const VALID_ID = '507f1f77bcf86cd799439011' + +const contextFor = (request: any): ExecutionContext => + ({ + switchToHttp: () => ({ getRequest: () => request }), + }) as unknown as ExecutionContext + +describe('CanModifyApiKey', () => { + let guard: CanModifyApiKey + let authService: { findApiKeyById: jest.Mock } + + beforeEach(() => { + authService = { findApiKeyById: jest.fn() } + guard = new CanModifyApiKey(authService as unknown as AuthService) + }) + + it('allows the owner of the api key', async () => { + authService.findApiKeyById.mockResolvedValue({ user: 'user_1' }) + const request = { params: { id: VALID_ID }, user: { id: 'user_1' } } + + await expect(guard.canActivate(contextFor(request))).resolves.toBe(true) + }) + + it('rejects a non-owner (cross-tenant access)', async () => { + authService.findApiKeyById.mockResolvedValue({ user: 'owner' }) + const request = { params: { id: VALID_ID }, user: { id: 'attacker' } } + + await expect(guard.canActivate(contextFor(request))).rejects.toThrow( + HttpException, + ) + }) + + it('allows an admin regardless of ownership', async () => { + authService.findApiKeyById.mockResolvedValue({ user: 'owner' }) + const request = { + params: { id: VALID_ID }, + user: { id: 'someone-else', role: UserRole.ADMIN }, + } + + await expect(guard.canActivate(contextFor(request))).resolves.toBe(true) + }) + + it('throws 400 for an invalid id', async () => { + const request = { params: { id: 'not-an-objectid' }, user: { id: 'user_1' } } + + await expect(guard.canActivate(contextFor(request))).rejects.toThrow( + HttpException, + ) + expect(authService.findApiKeyById).not.toHaveBeenCalled() + }) + + it('rejects when the api key does not exist', async () => { + authService.findApiKeyById.mockResolvedValue(null) + const request = { params: { id: VALID_ID }, user: { id: 'user_1' } } + + await expect(guard.canActivate(contextFor(request))).rejects.toThrow( + HttpException, + ) + }) +}) diff --git a/api/src/auth/schemas/password-reset.schema.ts b/api/src/auth/schemas/password-reset.schema.ts index 412a65b5..2058797a 100644 --- a/api/src/auth/schemas/password-reset.schema.ts +++ b/api/src/auth/schemas/password-reset.schema.ts @@ -17,8 +17,7 @@ export class PasswordReset { @Prop({ type: Date }) expiresAt: Date - // Number of failed OTP verification attempts against this record. - // Used to lock out brute-force attempts (see auth.service.resetPassword). + // OTP verification attempts, used to lock the record out of brute-forcing. @Prop({ type: Number, default: 0 }) attempts: number } diff --git a/api/src/auth/without-password.spec.ts b/api/src/auth/without-password.spec.ts new file mode 100644 index 00000000..bc24b3ab --- /dev/null +++ b/api/src/auth/without-password.spec.ts @@ -0,0 +1,41 @@ +import { withoutPassword } from './auth.service' +import type { UserDocument } from '../users/schemas/user.schema' + +// Fake document: only toObject() matters to the helper. +const doc = (fields: Record) => + ({ toObject: () => ({ ...fields }) }) as unknown as UserDocument + +describe('withoutPassword', () => { + it('removes the password hash', () => { + const safe = withoutPassword( + doc({ email: 'a@b.com', password: '$2a$10$hashedvalue' }) + ) + + expect(safe).not.toHaveProperty('password') + }) + + it('keeps everything else the client needs', () => { + const safe = withoutPassword( + doc({ + _id: 'u1', + email: 'a@b.com', + name: 'Ada', + role: 'regular', + password: '$2a$10$hashedvalue', + }) + ) + + expect(safe).toEqual({ + _id: 'u1', + email: 'a@b.com', + name: 'Ada', + role: 'regular', + }) + }) + + it('is harmless when the hash was never loaded', () => { + const safe = withoutPassword(doc({ email: 'a@b.com' })) + + expect(safe).toEqual({ email: 'a@b.com' }) + }) +}) diff --git a/api/src/common/escape-regexp.spec.ts b/api/src/common/escape-regexp.spec.ts new file mode 100644 index 00000000..56679bc2 --- /dev/null +++ b/api/src/common/escape-regexp.spec.ts @@ -0,0 +1,35 @@ +import { escapeRegExp } from './escape-regexp' + +describe('escapeRegExp', () => { + it('leaves plain text untouched', () => { + expect(escapeRegExp('hello world')).toBe('hello world') + expect(escapeRegExp('+14155550101')).toBe('\\+14155550101') + }) + + it('makes an unbalanced paren safe to compile', () => { + // Unescaped, `new RegExp('(')` throws and would fail the whole request. + expect(() => new RegExp(escapeRegExp('('))).not.toThrow() + expect(() => new RegExp(escapeRegExp('a)b['))).not.toThrow() + }) + + it('treats wildcards as literal characters', () => { + const pattern = new RegExp(escapeRegExp('.*'), 'i') + // A literal ".*" must not match arbitrary text. + expect(pattern.test('anything at all')).toBe(false) + expect(pattern.test('literally .* here')).toBe(true) + }) + + it('treats a dot as a dot, not "any character"', () => { + const pattern = new RegExp(escapeRegExp('a.c'), 'i') + expect(pattern.test('abc')).toBe(false) + expect(pattern.test('a.c')).toBe(true) + }) + + it('defuses a nested quantifier (ReDoS shape)', () => { + const pattern = new RegExp(escapeRegExp('(a+)+$'), 'i') + const start = Date.now() + pattern.test('a'.repeat(2000) + 'b') + // As a literal it is a plain substring scan, not catastrophic backtracking. + expect(Date.now() - start).toBeLessThan(100) + }) +}) diff --git a/api/src/common/escape-regexp.ts b/api/src/common/escape-regexp.ts new file mode 100644 index 00000000..a45d74ce --- /dev/null +++ b/api/src/common/escape-regexp.ts @@ -0,0 +1,11 @@ +/** + * Escape every regular-expression metacharacter in a string so it can be + * embedded in a RegExp as a literal. + * + * Search terms come straight from user input. Without this, "(" alone throws + * a SyntaxError and fails the request, "." matches any character instead of a + * dot, and a nested quantifier is a ReDoS vector. + */ +export function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') +} diff --git a/api/src/gateway/gateway.controller.ts b/api/src/gateway/gateway.controller.ts index 0eab5a28..450c972d 100644 --- a/api/src/gateway/gateway.controller.ts +++ b/api/src/gateway/gateway.controller.ts @@ -160,8 +160,9 @@ export class GatewayController { const page = req.query.page ? parseInt(req.query.page, 10) : 1; const limit = req.query.limit ? Math.min(parseInt(req.query.limit, 10), 100) : 50; const type = req.query.type || ''; - - const result = await this.gatewayService.getMessages(deviceId, type, page, limit); + const search = req.query.search || ''; + + const result = await this.gatewayService.getMessages(deviceId, type, page, limit, search); return result; } diff --git a/api/src/gateway/gateway.service.spec.ts b/api/src/gateway/gateway.service.spec.ts index 7f43cd83..b77e6695 100644 --- a/api/src/gateway/gateway.service.spec.ts +++ b/api/src/gateway/gateway.service.spec.ts @@ -279,12 +279,17 @@ describe('GatewayService', () => { { _id: 'device2', model: 'iPhone 13' }, ] - it('should return all devices for a user', async () => { + it('should return a user\'s devices without the push token or serial', async () => { mockDeviceModel.find.mockResolvedValue(mockDevices) const result = await service.getDevicesForUser(mockUser) - expect(mockDeviceModel.find).toHaveBeenCalledWith({ user: mockUser._id }) + const [filter, projection] = mockDeviceModel.find.mock.calls[0] + expect(filter).toEqual({ user: mockUser._id }) + // fcmToken is a push credential and serial is a hardware id; neither + // should be shipped to the browser in the device list. + expect(projection).toContain('-fcmToken') + expect(projection).toContain('-serial') expect(result).toEqual(mockDevices) }) }) @@ -802,6 +807,65 @@ describe('GatewayService', () => { HttpException, ) }) + + it('should search across message body, recipient and sender', async () => { + await service.getMessages(mockDeviceId, '', 1, 10, 'alice') + + const expectedQuery = { + device: mockDevice._id, + $or: [ + { message: /alice/i }, + { recipient: /alice/i }, + { sender: /alice/i }, + ], + } + + expect(mockSmsModel.countDocuments).toHaveBeenCalledWith(expectedQuery) + expect(mockSmsModel.find).toHaveBeenCalledWith( + expectedQuery, + null, + expect.any(Object), + ) + }) + + it('should combine search with the type filter', async () => { + await service.getMessages(mockDeviceId, 'sent', 1, 10, 'alice') + + expect(mockSmsModel.find).toHaveBeenCalledWith( + expect.objectContaining({ + device: mockDevice._id, + type: SMSType.SENT, + $or: expect.any(Array), + }), + null, + expect.any(Object), + ) + }) + + it('should ignore an empty or whitespace-only search', async () => { + await service.getMessages(mockDeviceId, '', 1, 10, ' ') + + expect(mockSmsModel.find).toHaveBeenCalledWith( + { device: mockDevice._id }, + null, + expect.any(Object), + ) + }) + + it('should escape regex metacharacters in the search term', async () => { + // Unescaped, this throws a SyntaxError and fails the request. + await expect( + service.getMessages(mockDeviceId, '', 1, 10, '('), + ).resolves.toBeDefined() + + // A wildcard must be matched literally, not treated as "any character". + await service.getMessages(mockDeviceId, '', 1, 10, '.*') + + const call = mockSmsModel.find.mock.calls.at(-1) + const messagePattern = call[0].$or[0].message as RegExp + expect(messagePattern.test('anything at all')).toBe(false) + expect(messagePattern.test('contains .* literally')).toBe(true) + }) }) describe('getStatsForUser', () => { diff --git a/api/src/gateway/gateway.service.ts b/api/src/gateway/gateway.service.ts index fe396c90..b0e2c38d 100644 --- a/api/src/gateway/gateway.service.ts +++ b/api/src/gateway/gateway.service.ts @@ -24,6 +24,7 @@ import { WebhookEvent } from '../webhook/webhook-event.enum' import { WebhookService } from '../webhook/webhook.service' import { BillingService } from '../billing/billing.service' import { SmsQueueService } from './queue/sms-queue.service' +import { escapeRegExp } from '../common/escape-regexp' @Injectable() export class GatewayService { @@ -139,7 +140,11 @@ export class GatewayService { } async getDevicesForUser(user: User): Promise { - return await this.deviceModel.find({ user: user._id }) + // Exclude the push credential and hardware serial from the client response. + return await this.deviceModel.find( + { user: user._id }, + '-fcmToken -serial', + ) } async getDeviceById(deviceId: string): Promise { @@ -947,6 +952,7 @@ export class GatewayService { type = '', page = 1, limit = 50, + search = '', ): Promise<{ data: any[]; meta: any }> { const device = await this.deviceModel.findById(deviceId) @@ -972,6 +978,19 @@ export class GatewayService { query.type = SMSType.RECEIVED } + // Free-text search across the message body and either party's number. + // The input is escaped before it reaches RegExp: an unescaped "(" throws + // and fails the request, and a crafted pattern is a ReDoS vector. + const trimmedSearch = search?.trim() + if (trimmedSearch) { + const pattern = new RegExp(escapeRegExp(trimmedSearch), 'i') + query.$or = [ + { message: pattern }, + { recipient: pattern }, + { sender: pattern }, + ] + } + // Get total count for pagination metadata const total = await this.smsModel.countDocuments(query) diff --git a/api/src/gateway/guards/can-modify-device.guard.spec.ts b/api/src/gateway/guards/can-modify-device.guard.spec.ts new file mode 100644 index 00000000..f3a3cbaa --- /dev/null +++ b/api/src/gateway/guards/can-modify-device.guard.spec.ts @@ -0,0 +1,66 @@ +import { HttpException } from '@nestjs/common' +import { ExecutionContext } from '@nestjs/common' +import { CanModifyDevice } from './can-modify-device.guard' +import { GatewayService } from '../gateway.service' +import { UserRole } from '../../users/user-roles.enum' + +const VALID_ID = '507f1f77bcf86cd799439011' + +const contextFor = (request: any): ExecutionContext => + ({ + switchToHttp: () => ({ getRequest: () => request }), + }) as unknown as ExecutionContext + +describe('CanModifyDevice', () => { + let guard: CanModifyDevice + let gatewayService: { getDeviceById: jest.Mock } + + beforeEach(() => { + gatewayService = { getDeviceById: jest.fn() } + guard = new CanModifyDevice(gatewayService as unknown as GatewayService) + }) + + it('allows the owner of the device', async () => { + gatewayService.getDeviceById.mockResolvedValue({ user: 'user_1' }) + const request = { params: { id: VALID_ID }, user: { id: 'user_1' } } + + await expect(guard.canActivate(contextFor(request))).resolves.toBe(true) + }) + + it('rejects a non-owner (cross-tenant access)', async () => { + gatewayService.getDeviceById.mockResolvedValue({ user: 'owner' }) + const request = { params: { id: VALID_ID }, user: { id: 'attacker' } } + + await expect(guard.canActivate(contextFor(request))).rejects.toThrow( + HttpException, + ) + }) + + it('allows an admin regardless of ownership', async () => { + gatewayService.getDeviceById.mockResolvedValue({ user: 'owner' }) + const request = { + params: { id: VALID_ID }, + user: { id: 'someone-else', role: UserRole.ADMIN }, + } + + await expect(guard.canActivate(contextFor(request))).resolves.toBe(true) + }) + + it('throws 400 for an invalid device id', async () => { + const request = { params: { id: 'not-an-objectid' }, user: { id: 'user_1' } } + + await expect(guard.canActivate(contextFor(request))).rejects.toThrow( + HttpException, + ) + expect(gatewayService.getDeviceById).not.toHaveBeenCalled() + }) + + it('rejects when the device does not exist', async () => { + gatewayService.getDeviceById.mockResolvedValue(null) + const request = { params: { id: VALID_ID }, user: { id: 'user_1' } } + + await expect(guard.canActivate(contextFor(request))).rejects.toThrow( + HttpException, + ) + }) +}) diff --git a/api/src/support/support.service.spec.ts b/api/src/support/support.service.spec.ts new file mode 100644 index 00000000..c0ff0862 --- /dev/null +++ b/api/src/support/support.service.spec.ts @@ -0,0 +1,122 @@ +import { ConflictException, HttpException, NotFoundException } from '@nestjs/common' +import { SupportService } from './support.service' +import { SupportCategory } from './dto/create-support-message.dto' + +const VALID_USER_ID = '507f1f77bcf86cd799439011' + +const build = () => { + let lastMessageDoc: any + + const supportMessageModel: any = jest.fn().mockImplementation((doc: any) => { + lastMessageDoc = { + ...doc, + _id: 'msg_1', + save: jest.fn().mockResolvedValue({ ...doc, _id: 'msg_1' }), + } + return lastMessageDoc + }) + supportMessageModel.countDocuments = jest.fn() + supportMessageModel.findOne = jest.fn() + + const userModel = { + findById: jest.fn(), + updateOne: jest.fn().mockResolvedValue(undefined), + } + const mailService = { sendEmailFromTemplate: jest.fn().mockResolvedValue(undefined) } + + const service = new SupportService( + supportMessageModel, + userModel as any, + mailService as any, + ) + + return { + service, + supportMessageModel, + userModel, + mailService, + getLastMessageDoc: () => lastMessageDoc, + } +} + +const dto = (overrides: Record = {}) => ({ + name: 'Ada', + email: 'a@b.com', + category: SupportCategory.GENERAL, + message: 'help please', + turnstileToken: 'token', + ...overrides, +}) + +describe('SupportService', () => { + describe('createSupportMessage', () => { + it('rejects once the 24h rate limit is reached, without persisting or emailing', async () => { + const { service, supportMessageModel, mailService } = build() + supportMessageModel.countDocuments.mockResolvedValue(3) + + await expect(service.createSupportMessage(dto() as any)).rejects.toThrow( + HttpException, + ) + expect(mailService.sendEmailFromTemplate).not.toHaveBeenCalled() + }) + + it('persists the message and sends a confirmation on success', async () => { + const { service, supportMessageModel, mailService, getLastMessageDoc } = build() + supportMessageModel.countDocuments.mockResolvedValue(0) + + const res = await service.createSupportMessage(dto() as any) + + expect(getLastMessageDoc().save).toHaveBeenCalledTimes(1) + // The turnstile token is stripped before persistence. + expect(getLastMessageDoc()).not.toHaveProperty('turnstileToken') + expect(mailService.sendEmailFromTemplate).toHaveBeenCalledTimes(1) + expect(mailService.sendEmailFromTemplate.mock.calls[0][0].to).toBe('a@b.com') + expect(res.message).toMatch(/submitted/i) + }) + }) + + describe('requestAccountDeletion', () => { + it('rejects when the user id is missing or invalid', async () => { + const { service } = build() + await expect( + service.requestAccountDeletion(dto({ user: 'not-an-id' }) as any), + ).rejects.toThrow(NotFoundException) + }) + + it('rejects when the user does not exist', async () => { + const { service, userModel } = build() + userModel.findById.mockResolvedValue(null) + + await expect( + service.requestAccountDeletion(dto({ user: VALID_USER_ID }) as any), + ).rejects.toThrow(NotFoundException) + }) + + it('rejects a duplicate deletion request', async () => { + const { service, userModel, supportMessageModel } = build() + userModel.findById.mockResolvedValue({ _id: VALID_USER_ID, email: 'a@b.com', name: 'Ada' }) + supportMessageModel.findOne.mockResolvedValue({ _id: 'existing' }) + + await expect( + service.requestAccountDeletion(dto({ user: VALID_USER_ID }) as any), + ).rejects.toThrow(ConflictException) + }) + + it('records the deletion request with reason and emails the user on success', async () => { + const { service, userModel, supportMessageModel, mailService } = build() + userModel.findById.mockResolvedValue({ _id: VALID_USER_ID, email: 'a@b.com', name: 'Ada' }) + supportMessageModel.findOne.mockResolvedValue(null) + + const res = await service.requestAccountDeletion( + dto({ user: VALID_USER_ID, message: 'no longer needed' }) as any, + ) + + expect(userModel.updateOne).toHaveBeenCalledTimes(1) + const [, update] = userModel.updateOne.mock.calls[0] + expect(update.accountDeletionRequestedAt).toBeInstanceOf(Date) + expect(update.accountDeletionReason).toBe('no longer needed') + expect(mailService.sendEmailFromTemplate).toHaveBeenCalledTimes(1) + expect(res.message).toMatch(/submitted/i) + }) + }) +}) diff --git a/api/src/users/schemas/user.schema.spec.ts b/api/src/users/schemas/user.schema.spec.ts new file mode 100644 index 00000000..dc07c12f --- /dev/null +++ b/api/src/users/schemas/user.schema.spec.ts @@ -0,0 +1,14 @@ +import { UserSchema } from './user.schema' + +describe('User schema', () => { + it('never selects the password hash by default', () => { + expect(UserSchema.path('password').options.select).toBe(false) + }) + + it.each(['email', 'name', 'role', 'emailVerifiedAt'])( + 'still returns %s by default', + (field) => { + expect(UserSchema.path(field).options.select).toBeUndefined() + } + ) +}) diff --git a/api/src/users/schemas/user.schema.ts b/api/src/users/schemas/user.schema.ts index 66050f06..14dae2dd 100644 --- a/api/src/users/schemas/user.schema.ts +++ b/api/src/users/schemas/user.schema.ts @@ -23,7 +23,8 @@ export class User { @Prop({ type: String, trim: true }) phone?: string - @Prop({ type: String }) + // Never loaded unless a caller asks with .select('+password'). + @Prop({ type: String, select: false }) password: string @Prop({ type: String, default: UserRole.REGULAR }) diff --git a/api/src/users/users.service.ts b/api/src/users/users.service.ts index 8678d69d..8550f449 100644 --- a/api/src/users/users.service.ts +++ b/api/src/users/users.service.ts @@ -18,6 +18,11 @@ export class UsersService { return await this.userModel.findOne(params) } + // Only for flows that verify a password. Never return this to a client. + async findOneWithPassword(params) { + return await this.userModel.findOne(params).select('+password') + } + async findAll() { return await this.userModel.find() } diff --git a/api/src/webhook/webhook.service.spec.ts b/api/src/webhook/webhook.service.spec.ts new file mode 100644 index 00000000..535c9eb3 --- /dev/null +++ b/api/src/webhook/webhook.service.spec.ts @@ -0,0 +1,173 @@ +import { HttpException } from '@nestjs/common' +import * as crypto from 'crypto' +import axios from 'axios' +import { WebhookService } from './webhook.service' + +jest.mock('axios') +const mockedAxios = axios as jest.Mocked + +const build = () => { + const webhookSubscriptionModel: any = { + findById: jest.fn(), + findOne: jest.fn(), + countDocuments: jest.fn(), + create: jest.fn(), + updateOne: jest.fn().mockResolvedValue(undefined), + } + const webhookNotificationModel: any = { findById: jest.fn() } + + const service = new WebhookService( + webhookSubscriptionModel, + webhookNotificationModel, + {} as any, // webhookQueueService + {} as any, // mailService + {} as any, // usersService + ) + + return { service, webhookSubscriptionModel, webhookNotificationModel } +} + +describe('WebhookService', () => { + beforeEach(() => jest.clearAllMocks()) + + describe('validateDeliveryUrl (SSRF guard)', () => { + const validate = (service: WebhookService, url: string) => + (service as any).validateDeliveryUrl(url) + + it('accepts a normal https URL', () => { + const { service } = build() + expect(() => validate(service, 'https://example.com/hook')).not.toThrow() + }) + + it.each([ + 'ftp://example.com/hook', + 'file:///etc/passwd', + 'not-a-url', + ])('rejects a non-http(s) or malformed URL: %s', (url) => { + const { service } = build() + expect(() => validate(service, url)).toThrow(HttpException) + }) + + it.each([ + 'http://localhost/hook', + 'http://127.0.0.1/hook', + 'http://10.0.0.5/hook', + 'http://192.168.1.10/hook', + 'http://169.254.169.254/latest/meta-data', // cloud metadata IP + 'http://172.16.0.1/hook', + ])('rejects a private or loopback host: %s', (url) => { + const { service } = build() + expect(() => validate(service, url)).toThrow(HttpException) + }) + }) + + describe('signing secret validation', () => { + it('rejects a create with a secret shorter than 20 characters', async () => { + const { service } = build() + await expect( + service.create({ + user: { _id: 'user_1' }, + createWebhookDto: { + name: 'w', + events: ['message.received'], + deliveryUrl: 'https://example.com/hook', + signingSecret: 'too-short', + }, + }), + ).rejects.toThrow(HttpException) + }) + + it('rejects an update that sets a secret shorter than 20 characters', async () => { + const { service, webhookSubscriptionModel } = build() + webhookSubscriptionModel.findOne.mockResolvedValue({ + signingSecret: 'a'.repeat(20), + save: jest.fn(), + }) + + await expect( + service.update({ + user: { _id: 'user_1' }, + webhookId: 'wh_1', + updateWebhookDto: { signingSecret: 'short' }, + }), + ).rejects.toThrow(HttpException) + }) + }) + + describe('attemptWebhookDelivery', () => { + const activeSubscription = (overrides: Record = {}) => ({ + _id: 'ws_1', + isActive: true, + deletedAt: null, + deliveryUrl: 'https://example.com/hook', + signingSecret: 'a-signing-secret-of-enough-length', + ...overrides, + }) + + const notification = (): any => ({ + _id: 'wn_1', + webhookSubscription: 'ws_1', + payload: { hello: 'world' }, + deliveryAttemptCount: 0, + save: jest.fn().mockResolvedValue(undefined), + }) + + it('signs the payload with HMAC-SHA256 of the signing secret', async () => { + const { service, webhookSubscriptionModel, webhookNotificationModel } = build() + const sub = activeSubscription() + const notif = notification() + webhookNotificationModel.findById.mockResolvedValue(notif) + webhookSubscriptionModel.findById.mockResolvedValue(sub) + mockedAxios.post.mockResolvedValue({ status: 200, data: 'ok' }) + + await service.attemptWebhookDelivery('wn_1') + + const expected = crypto + .createHmac('sha256', sub.signingSecret) + .update(JSON.stringify(notif.payload)) + .digest('hex') + + expect(mockedAxios.post).toHaveBeenCalledTimes(1) + const [, , config] = mockedAxios.post.mock.calls[0] + expect((config as any).headers['X-Signature']).toBe(expected) + }) + + it('produces a different signature when the secret differs', async () => { + const payload = { hello: 'world' } + const sig = (secret: string) => + crypto.createHmac('sha256', secret).update(JSON.stringify(payload)).digest('hex') + expect(sig('a-signing-secret-of-enough-length')).not.toBe( + sig('a-different-secret-of-enough-length'), + ) + }) + + it('aborts delivery without calling axios when the subscription is inactive', async () => { + const { service, webhookSubscriptionModel, webhookNotificationModel } = build() + const notif = notification() + webhookNotificationModel.findById.mockResolvedValue(notif) + webhookSubscriptionModel.findById.mockResolvedValue( + activeSubscription({ isActive: false }), + ) + + await service.attemptWebhookDelivery('wn_1') + + expect(notif.deliveryAttemptAbortedAt).toBeInstanceOf(Date) + expect(notif.save).toHaveBeenCalledTimes(1) + expect(mockedAxios.post).not.toHaveBeenCalled() + }) + + it('aborts delivery when the subscription has been soft-deleted', async () => { + const { service, webhookSubscriptionModel, webhookNotificationModel } = build() + const notif = notification() + webhookNotificationModel.findById.mockResolvedValue(notif) + webhookSubscriptionModel.findById.mockResolvedValue( + activeSubscription({ deletedAt: new Date() }), + ) + + await service.attemptWebhookDelivery('wn_1') + + expect(notif.deliveryAttemptAbortedAt).toBeInstanceOf(Date) + expect(mockedAxios.post).not.toHaveBeenCalled() + }) + }) +}) diff --git a/web/.env.example b/web/.env.example index 23023d1c..ac555934 100644 --- a/web/.env.example +++ b/web/.env.example @@ -4,9 +4,10 @@ NEXT_PUBLIC_LATEST_APP_VERSION_CODE=17 NEXT_PUBLIC_GOOGLE_CLIENT_ID= NEXT_PUBLIC_TAWKTO_EMBED_URL= -AUTH_SECRET= # https://generate-secret.vercel.app/32 - -DATABASE_URL=mongodb://adminUser:adminPassword@textbee-db:27017/textbee?authSource=admin +# Used by NextAuth (next-auth v4) to sign/verify the session JWT. Must match +# the value read by proxy.ts (process.env.NEXTAUTH_SECRET). +NEXTAUTH_SECRET= # https://generate-secret.vercel.app/32 +NEXTAUTH_URL=http://localhost:3000 MAIL_HOST= MAIL_PORT= diff --git a/web/.eslintrc.json b/web/.eslintrc.json deleted file mode 100644 index d2a18faf..00000000 --- a/web/.eslintrc.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "extends": "next/core-web-vitals", - "rules": { - "react/no-unescaped-entities": "off" - } -} diff --git a/web/.gitignore b/web/.gitignore index c574c9f1..bd43cb79 100644 --- a/web/.gitignore +++ b/web/.gitignore @@ -7,6 +7,9 @@ # testing /coverage +/test-results +/playwright-report +/playwright/.cache # next.js /.next/ diff --git a/web/app/(app)/(auth)/(components)/login-form.test.tsx b/web/app/(app)/(auth)/(components)/login-form.test.tsx new file mode 100644 index 00000000..900ca56f --- /dev/null +++ b/web/app/(app)/(auth)/(components)/login-form.test.tsx @@ -0,0 +1,92 @@ +import { describe, expect, it, vi, beforeEach } from 'vitest' +import { render, screen, waitFor } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import LoginForm from './login-form' + +const push = vi.fn() +const refresh = vi.fn() +vi.mock('next/navigation', () => ({ + useRouter: () => ({ push, refresh }), +})) + +const signIn = vi.fn() +vi.mock('next-auth/react', () => ({ signIn: (...a: unknown[]) => signIn(...a) })) + +// signIn reflects next-auth's real redirect semantics: with redirect:true it +// navigates and never resolves a usable result; with redirect:false it resolves +// { error, ok }. A component that reads the result while passing redirect:true +// can never see the error, which is the bug under test. +const redirectAwareSignIn = (_provider: string, opts: { redirect?: boolean }) => + opts?.redirect + ? undefined + : { error: 'CredentialsSignin', ok: false, status: 401, url: null } + +// Turnstile needs a live Cloudflare widget it cannot get in jsdom, so the hook +// is mocked to hand the form a token immediately, making the form submittable. +vi.mock('@/lib/turnstile', () => ({ + useTurnstile: (opts: { onToken?: (t: string) => void }) => { + opts.onToken?.('test-turnstile-token') + return { + containerRef: { current: null }, + token: 'test-turnstile-token', + error: null, + isReady: true, + } + }, +})) + +async function fillAndSubmit() { + await userEvent.type(screen.getByLabelText('Email'), 'user@example.com') + await userEvent.type(screen.getByLabelText('Password'), 'secret123') + await userEvent.click(screen.getByRole('button', { name: /sign in/i })) +} + +describe('LoginForm', () => { + beforeEach(() => { + vi.clearAllMocks() + signIn.mockImplementation(redirectAwareSignIn) + }) + + // The reported bug: wrong credentials showed no message and reloaded, because + // redirect:true made the error branch dead code. + it('shows an error and does not navigate on wrong credentials', async () => { + // Uses the redirect-aware default: only redirect:false yields a readable + // error. Against the redirect:true code this returns undefined, so the + // message never renders and this test fails, which is the point. + render() + await fillAndSubmit() + + expect( + await screen.findByText(/invalid email or password/i) + ).toBeInTheDocument() + expect(push).not.toHaveBeenCalled() + }) + + it('calls signIn without a redirect so the result can be read', async () => { + render() + await fillAndSubmit() + + await waitFor(() => expect(signIn).toHaveBeenCalled()) + expect(signIn).toHaveBeenCalledWith( + 'email-password-login', + expect.objectContaining({ redirect: false }) + ) + }) + + it('navigates to the dashboard on success', async () => { + signIn.mockResolvedValueOnce({ + error: null, + ok: true, + status: 200, + url: null, + }) + + render() + await fillAndSubmit() + + await waitFor(() => expect(push).toHaveBeenCalledWith('/dashboard')) + expect( + screen.queryByText(/invalid email or password/i) + ).not.toBeInTheDocument() + }) +}) diff --git a/web/app/(app)/(auth)/(components)/login-form.tsx b/web/app/(app)/(auth)/(components)/login-form.tsx index 2b13832d..1d0d5131 100644 --- a/web/app/(app)/(auth)/(components)/login-form.tsx +++ b/web/app/(app)/(auth)/(components)/login-form.tsx @@ -80,9 +80,11 @@ export default function LoginForm() { } try { + // redirect:false so the result is returned here. With redirect:true + // next-auth navigates on both success and failure, which reloaded the + // page and made the error branch below dead code. const result = await signIn('email-password-login', { - redirect: true, - callbackUrl: Routes.dashboard, + redirect: false, email: data.email, password: data.password, turnstileToken: data.turnstileToken, @@ -93,7 +95,11 @@ export default function LoginForm() { type: 'manual', message: 'Invalid email or password', }) + return } + + router.push(Routes.dashboard) + router.refresh() } catch (error) { console.error('login error:', error) form.setError('root', { diff --git a/web/app/(app)/(auth)/(components)/register-form.test.tsx b/web/app/(app)/(auth)/(components)/register-form.test.tsx new file mode 100644 index 00000000..c45730bc --- /dev/null +++ b/web/app/(app)/(auth)/(components)/register-form.test.tsx @@ -0,0 +1,62 @@ +import { describe, expect, it, vi, beforeEach } from 'vitest' +import { render, screen, waitFor } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import RegisterForm from './register-form' + +const push = vi.fn() +vi.mock('next/navigation', () => ({ useRouter: () => ({ push, refresh: vi.fn() }) })) + +const signIn = vi.fn() +vi.mock('next-auth/react', () => ({ signIn: (...a: unknown[]) => signIn(...a) })) + +vi.mock('@/lib/turnstile', () => ({ + useTurnstile: (opts: { onToken?: (t: string) => void }) => { + opts.onToken?.('test-turnstile-token') + return { containerRef: { current: null }, token: 'test-turnstile-token', error: null, isReady: true } + }, +})) + +async function fillAndSubmit() { + await userEvent.type(screen.getByLabelText('Full Name'), 'Ada Lovelace') + await userEvent.type(screen.getByLabelText('Email'), 'ada@example.com') + await userEvent.type(screen.getByLabelText('Password'), 'password123') + await userEvent.click(screen.getByRole('button', { name: /create account|sign up|register/i })) +} + +// register-form already handles errors correctly (redirect:false). These lock +// that so the login bug (dead error branch under redirect:true) cannot be +// reintroduced here unnoticed. +describe('RegisterForm', () => { + beforeEach(() => vi.clearAllMocks()) + + it('shows an error and does not navigate when registration fails', async () => { + signIn.mockResolvedValueOnce({ error: 'CredentialsSignin', ok: false }) + render() + await fillAndSubmit() + + expect(await screen.findByText(/failed to create account/i)).toBeInTheDocument() + expect(push).not.toHaveBeenCalled() + }) + + it('submits with redirect:false so the result is readable', async () => { + signIn.mockResolvedValueOnce({ ok: true, error: null }) + render() + await fillAndSubmit() + + await waitFor(() => expect(signIn).toHaveBeenCalled()) + expect(signIn).toHaveBeenCalledWith( + 'email-password-register', + expect.objectContaining({ redirect: false }) + ) + }) + + it('navigates to verify-email on success', async () => { + signIn.mockResolvedValueOnce({ ok: true, error: null }) + render() + await fillAndSubmit() + + await waitFor(() => + expect(push).toHaveBeenCalledWith('/verify-email?verificationEmailSent=1') + ) + }) +}) diff --git a/web/app/(app)/(auth)/(components)/register-form.tsx b/web/app/(app)/(auth)/(components)/register-form.tsx index 30f7928a..7c199d4e 100644 --- a/web/app/(app)/(auth)/(components)/register-form.tsx +++ b/web/app/(app)/(auth)/(components)/register-form.tsx @@ -36,12 +36,18 @@ const registerSchema = z.object({ .min(1, { message: 'Please complete the bot verification' }), }) -type RegisterFormValues = z.infer +// marketingOptIn is `.optional().default(true)`, so zod's input and output +// types differ: optional going in, guaranteed coming out. The form holds the +// input shape and the submit handler receives the output shape, which is what +// useForm's third generic is for. Using z.infer (the output) for both made the +// resolver unassignable. +type RegisterFormInput = z.input +type RegisterFormValues = z.output export default function RegisterForm() { const router = useRouter() - const form = useForm({ + const form = useForm({ resolver: zodResolver(registerSchema), defaultValues: { name: '', diff --git a/web/app/(app)/(auth)/(components)/request-password-reset-form.tsx b/web/app/(app)/(auth)/(components)/request-password-reset-form.tsx index 24c80af2..371da5d4 100644 --- a/web/app/(app)/(auth)/(components)/request-password-reset-form.tsx +++ b/web/app/(app)/(auth)/(components)/request-password-reset-form.tsx @@ -100,7 +100,7 @@ export default function RequestPasswordResetForm() { return (
- + Reset your password diff --git a/web/app/(app)/(auth)/(components)/reset-password-form.test.tsx b/web/app/(app)/(auth)/(components)/reset-password-form.test.tsx new file mode 100644 index 00000000..9f5d60d4 --- /dev/null +++ b/web/app/(app)/(auth)/(components)/reset-password-form.test.tsx @@ -0,0 +1,58 @@ +import { describe, expect, it, vi, beforeEach } from 'vitest' +import { render, screen, waitFor } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import ResetPasswordForm from './reset-password-form' + +const post = vi.fn() +vi.mock('@/lib/httpBrowserClient', () => ({ default: { post: (...a: unknown[]) => post(...a) } })) + +async function fillAndSubmit() { + await userEvent.type(screen.getByLabelText('New Password'), 'newpassword123') + await userEvent.type(screen.getByLabelText('Confirm Password'), 'newpassword123') + await userEvent.click(screen.getByRole('button', { name: /reset password/i })) +} + +describe('ResetPasswordForm', () => { + beforeEach(() => vi.clearAllMocks()) + + const renderForm = () => + render() + + // The bug: the catch set root.serverError but the JSX read errors.root.message, + // so a failed reset showed an empty paragraph and no message. + it('shows an error message when the reset fails', async () => { + post.mockRejectedValueOnce(new Error('boom')) + renderForm() + await fillAndSubmit() + + expect( + await screen.findByText(/failed to reset password/i) + ).toBeInTheDocument() + }) + + // The second bug: the handler swallows the error, so isSubmitSuccessful stays + // true and the success alert rendered even on failure. + it('does not claim success when the reset fails', async () => { + post.mockRejectedValueOnce(new Error('boom')) + renderForm() + await fillAndSubmit() + + await screen.findByText(/failed to reset password/i) + expect( + screen.queryByText(/password reset successful/i) + ).not.toBeInTheDocument() + }) + + it('shows the success alert when the reset succeeds', async () => { + post.mockResolvedValueOnce({ data: {} }) + renderForm() + await fillAndSubmit() + + expect( + await screen.findByText(/password reset successful/i) + ).toBeInTheDocument() + expect( + screen.queryByText(/failed to reset password/i) + ).not.toBeInTheDocument() + }) +}) diff --git a/web/app/(app)/(auth)/(components)/reset-password-form.tsx b/web/app/(app)/(auth)/(components)/reset-password-form.tsx index 54ef108b..9636293e 100644 --- a/web/app/(app)/(auth)/(components)/reset-password-form.tsx +++ b/web/app/(app)/(auth)/(components)/reset-password-form.tsx @@ -70,11 +70,14 @@ export default function ResetPasswordForm({ }) const onResetPassword = async (data: ResetPasswordFormValues) => { + form.clearErrors() try { await httpBrowserClient.post(ApiEndpoints.auth.resetPassword(), data) } catch (error) { console.error(error) - form.setError('root.serverError', { + // 'root', not 'root.serverError': the JSX renders errors.root.message, + // and a nested key leaves that undefined so no message showed. + form.setError('root', { message: 'Failed to reset password', }) } @@ -82,7 +85,7 @@ export default function ResetPasswordForm({ return (
- + Reset your password @@ -174,7 +177,8 @@ export default function ResetPasswordForm({ - {form.formState.isSubmitted && form.formState.isSubmitSuccessful && ( + {form.formState.isSubmitSuccessful && + !form.formState.errors.root && ( {/* */} Password reset successful diff --git a/web/app/(app)/(auth)/layout.tsx b/web/app/(app)/(auth)/layout.tsx index 48eacd27..b5311859 100644 --- a/web/app/(app)/(auth)/layout.tsx +++ b/web/app/(app)/(auth)/layout.tsx @@ -1,7 +1,16 @@ +import Footer from '@/components/shared/footer' + +// The footer is rendered per-section rather than in (app)/layout.tsx, because +// the dashboard's fixed sidebar would otherwise overlap a full-width footer. export default function AuthLayout({ children, }: { children: React.ReactNode }) { - return <>{children} + return ( + <> + {children} +