From 68e72d8bdc72d44d1da98f1cd415d44c056e3a88 Mon Sep 17 00:00:00 2001 From: isra el Date: Sun, 19 Jul 2026 18:21:38 +0300 Subject: [PATCH 1/7] test(api): lock the three authorization guards The guards that prevent cross-tenant access had zero tests, which is the highest-risk gap in the backend: a silent regression here would let one account reach another account's devices or api keys. These specs pin the current authz contract without changing any code: - AuthGuard: valid bearer resolves the user; invalid/expired bearer is 401; a valid x-api-key resolves via findActiveApiKeyByClientKey plus a matching bcrypt hash and attaches request.apiKey; a non-matching hash, an unknown key, and no credentials are all rejected; a token that resolves an id for a user that no longer exists is 401. - CanModifyDevice / CanModifyApiKey: owner passes; non-owner is rejected (the cross-tenant case); admin passes regardless; an invalid ObjectId is 400 before any lookup; a missing record is rejected. Guards are plain constructor-injected classes, so they are instantiated directly with mocked services and a minimal ExecutionContext, which is lighter than a testing module and equally faithful. 17 tests, all passing against the current unchanged guards. Co-Authored-By: Claude Fable 5 --- api/src/auth/guards/auth.guard.spec.ts | 117 ++++++++++++++++++ .../guards/can-modify-api-key.guard.spec.ts | 66 ++++++++++ .../guards/can-modify-device.guard.spec.ts | 66 ++++++++++ 3 files changed, 249 insertions(+) create mode 100644 api/src/auth/guards/auth.guard.spec.ts create mode 100644 api/src/auth/guards/can-modify-api-key.guard.spec.ts create mode 100644 api/src/gateway/guards/can-modify-device.guard.spec.ts 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/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, + ) + }) +}) From 9b7a8bcc5500aa9eeb141df4547f9556e9105779 Mon Sep 17 00:00:00 2001 From: isra el Date: Sun, 19 Jul 2026 18:24:31 +0300 Subject: [PATCH 2/7] test(api): lock auth.service core flows auth.service was 521 lines with only the withoutPassword helper covered. These specs pin the flows a regression would hurt most, all against the current unchanged code: - validateEmail / validatePassword: boundary and malformed-input cases, including the 6 and 128 character password edges. - findActiveApiKeyByClientKey: the exact-masked hit path and the legacy prefix-regex fallback, asserting the revoked-key exclusion is applied on both lookups. This is the behaviour lock for the upcoming regex-escape fix: it captures how a legitimate key resolves today. - generateApiKey: the raw key is returned once, and only a masked value plus a bcrypt hash of the key are persisted (never the raw key). - changePassword: a wrong old password is rejected without saving; the success path replaces the stored hash. - resetPassword: a missing or non-matching OTP is rejected without saving; the success path updates the password and closes the reset window. Note (to be fixed under the refactor part, not here): changePassword calls validatePassword without await, so the length rule does not actually block a weak password on change. Left as-is in this behaviour -lock commit and tracked for a guarded fix. 16 tests, all passing against current code. Co-Authored-By: Claude Fable 5 --- api/src/auth/auth.service.spec.ts | 240 ++++++++++++++++++++++++++++++ 1 file changed, 240 insertions(+) create mode 100644 api/src/auth/auth.service.spec.ts diff --git a/api/src/auth/auth.service.spec.ts b/api/src/auth/auth.service.spec.ts new file mode 100644 index 00000000..da8c0a44 --- /dev/null +++ b/api/src/auth/auth.service.spec.ts @@ -0,0 +1,240 @@ +import { HttpException } from '@nestjs/common' +import * as bcrypt from 'bcryptjs' +import { AuthService } from './auth.service' + +// 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(), + } + const passwordResetModel = { findOne: jest.fn() } + const mailService = { sendEmailFromTemplate: jest.fn().mockResolvedValue(undefined) } + + const service = new AuthService( + usersService as any, + {} as any, // jwtService + apiKeyModel, + passwordResetModel as any, + {} as any, // accessLogModel + {} as any, // emailVerificationModel + mailService as any, + {} as any, // turnstileService + ) + + return { + service, + apiKeyModel, + usersService, + passwordResetModel, + mailService, + 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) + + 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) + }) + }) + + 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) + }) + }) + + 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') + + await expect( + 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('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 } + } + + it('rejects when there is no valid reset record', async () => { + const { service, passwordResetModel, user } = setup() + passwordResetModel.findOne.mockResolvedValue(null) + + await expect( + service.resetPassword({ email: 'a@b.com', otp: '1234', newPassword: 'new-password' }), + ).rejects.toThrow(HttpException) + expect(user.save).not.toHaveBeenCalled() + }) + + it('rejects when the OTP does not match', async () => { + const { service, passwordResetModel, user } = setup() + passwordResetModel.findOne.mockResolvedValue({ + otp: bcrypt.hashSync('9999', 10), + save: jest.fn(), + }) + + await expect( + service.resetPassword({ email: 'a@b.com', otp: '1234', newPassword: 'new-password' }), + ).rejects.toThrow(HttpException) + expect(user.save).not.toHaveBeenCalled() + }) + + it('updates the password and expires the reset on success', async () => { + const { service, passwordResetModel, user } = setup() + const reset = { + otp: bcrypt.hashSync('1234', 10), + expiresAt: new Date(Date.now() + 60_000), + save: jest.fn().mockResolvedValue(undefined), + } + passwordResetModel.findOne.mockResolvedValue(reset) + + const res = await service.resetPassword({ + email: 'a@b.com', + otp: '1234', + newPassword: 'new-password', + }) + + expect(user.save).toHaveBeenCalledTimes(1) + expect(bcrypt.compareSync('new-password', 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) + }) + }) +}) From 78fb040c213b09cf0e3464838d78451fc07a41d9 Mon Sep 17 00:00:00 2001 From: isra el Date: Sun, 19 Jul 2026 18:26:24 +0300 Subject: [PATCH 3/7] test(api): lock webhook.service security-critical units webhook.service was 1039 lines with zero tests. These specs pin the parts a regression would hurt most, without changing any code: - validateDeliveryUrl: accepts normal https; rejects non-http(s) and malformed URLs; rejects loopback and private hosts including the cloud metadata IP. This locks the SSRF guard so it cannot silently regress. - Signing secret validation: a secret under 20 characters is rejected on both create and update. - attemptWebhookDelivery signing: the X-Signature header equals an independently computed HMAC-SHA256 of the JSON payload under the subscription secret, and the signature changes with the secret. - Delivery abort: when the subscription is inactive or soft-deleted, the attempt is marked aborted and saved and no HTTP request is made. axios is mocked so no request leaves the process; private methods are exercised through the instance. 16 tests, all passing against current code. Co-Authored-By: Claude Fable 5 --- api/src/webhook/webhook.service.spec.ts | 173 ++++++++++++++++++++++++ 1 file changed, 173 insertions(+) create mode 100644 api/src/webhook/webhook.service.spec.ts 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() + }) + }) +}) From b514891cad364d2787c24abfd92dcc4eb25a90dd Mon Sep 17 00:00:00 2001 From: isra el Date: Sun, 19 Jul 2026 18:27:32 +0300 Subject: [PATCH 4/7] test(api): lock support.service flows support.service had no tests. These lock both flows against current code: - createSupportMessage: the 24h rate limit rejects a fourth request without persisting or emailing; the success path saves the message, strips the turnstile token before persistence, emails the requester, and returns success. - requestAccountDeletion: a missing or invalid user id and an unknown user are both NotFound; a second request is a Conflict; the success path records accountDeletionRequestedAt with the reason and emails the user. 6 tests, all passing against current code. Co-Authored-By: Claude Fable 5 --- api/src/support/support.service.spec.ts | 122 ++++++++++++++++++++++++ 1 file changed, 122 insertions(+) create mode 100644 api/src/support/support.service.spec.ts 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) + }) + }) +}) From 6b66160f7f3fae9dc5525c935f1e4b413edf6045 Mon Sep 17 00:00:00 2001 From: isra el Date: Sun, 19 Jul 2026 18:33:57 +0300 Subject: [PATCH 5/7] fix(api): actually enforce email and password validation validateEmail and validatePassword are async and signal failure by throwing (a rejected promise). Three callers invoked them without await: register (both) and changePassword. An unawaited rejected promise is dropped, so execution continued past the check: registration accepted a malformed email or an out-of-range password, and a password change accepted a too-short new password. The validators were effectively dead. Adding await makes the existing rules take effect at their intended point. Legitimate clients are unaffected: the web signup and change-password forms already enforce a valid email and the length bounds, so this only rejects input the service was always meant to reject, with a clean 400 instead of persisting bad data. Guards were written first and seen to fail against the pre-fix code (register with a bad email or short password still created the user; changePassword with a short password still saved), then pass after the await is added. The valid-input paths are asserted to still succeed. This is the dependency-free half of what a global ValidationPipe would have covered. The pipe itself is deferred: class-transformer is not installed, so enabling it now would add a dependency and risk crashing once any DTO gains a decorator. Recommended as a separate follow-up. Co-Authored-By: Claude Fable 5 --- api/src/auth/auth.service.spec.ts | 86 ++++++++++++++++++++++++++++++- api/src/auth/auth.service.ts | 6 +-- 2 files changed, 87 insertions(+), 5 deletions(-) diff --git a/api/src/auth/auth.service.spec.ts b/api/src/auth/auth.service.spec.ts index da8c0a44..fd93292b 100644 --- a/api/src/auth/auth.service.spec.ts +++ b/api/src/auth/auth.service.spec.ts @@ -17,19 +17,22 @@ const build = () => { const usersService = { findOne: jest.fn(), findOneWithPassword: jest.fn(), + create: jest.fn(), } const passwordResetModel = { findOne: 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, - {} as any, // jwtService + jwtService as any, apiKeyModel, passwordResetModel as any, {} as any, // accessLogModel {} as any, // emailVerificationModel mailService as any, - {} as any, // turnstileService + turnstileService as any, ) return { @@ -38,6 +41,8 @@ const build = () => { usersService, passwordResetModel, mailService, + jwtService, + turnstileService, getLastApiKeyDoc: () => lastApiKeyDoc, } } @@ -177,6 +182,83 @@ describe('AuthService', () => { }) }) + 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 + } + + 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) + }) + }) + + 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() + }) + }) + describe('resetPassword', () => { const setup = () => { const ctx = build() diff --git a/api/src/auth/auth.service.ts b/api/src/auth/auth.service.ts index cb788f09..193d51a2 100644 --- a/api/src/auth/auth.service.ts +++ b/api/src/auth/auth.service.ts @@ -126,8 +126,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 @@ -261,7 +261,7 @@ export class AuthService { ) } - this.validatePassword(input.newPassword) + await this.validatePassword(input.newPassword) const hashedPassword = await bcrypt.hash(input.newPassword, 10) userToUpdate.password = hashedPassword From e58412fa14ee0b8b17d5ccc460965c21b9de3989 Mon Sep 17 00:00:00 2001 From: isra el Date: Sun, 19 Jul 2026 18:35:36 +0300 Subject: [PATCH 6/7] fix(api): escape the api-key lookup regex findActiveApiKeyByClientKey builds a fallback lookup as new RegExp(`^${prefix}`) from the client-supplied key prefix and runs it as a Mongo $regex. A prefix containing regex metacharacters compiles to the wrong pattern or, for something like "((((", throws SyntaxError and fails the request. On the authentication hot path that is a denial and a correctness hole. The prefix is now escaped with escapeRegExp before it reaches the RegExp. escapeRegExp already existed under gateway with its own spec; it is moved to src/common so auth and gateway share one copy, and the gateway import is updated. No behaviour change for the gateway caller. Guarded by the existing lock (a legitimate key still resolves via the masked hit and the prefix fallback) plus a new case, written first and seen to fail against the pre-fix code (it threw "Unterminated group"): a key with metacharacters now compiles to a literal pattern and resolves to no match instead of throwing. Co-Authored-By: Claude Fable 5 --- api/src/auth/auth.service.spec.ts | 13 +++++++++++++ api/src/auth/auth.service.ts | 3 ++- api/src/{gateway => common}/escape-regexp.spec.ts | 0 api/src/{gateway => common}/escape-regexp.ts | 0 api/src/gateway/gateway.service.ts | 2 +- 5 files changed, 16 insertions(+), 2 deletions(-) rename api/src/{gateway => common}/escape-regexp.spec.ts (100%) rename api/src/{gateway => common}/escape-regexp.ts (100%) diff --git a/api/src/auth/auth.service.spec.ts b/api/src/auth/auth.service.spec.ts index fd93292b..a55adf67 100644 --- a/api/src/auth/auth.service.spec.ts +++ b/api/src/auth/auth.service.spec.ts @@ -141,6 +141,19 @@ describe('AuthService', () => { // 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', () => { diff --git a/api/src/auth/auth.service.ts b/api/src/auth/auth.service.ts index 193d51a2..63818c67 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 { @@ -410,7 +411,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/gateway/escape-regexp.spec.ts b/api/src/common/escape-regexp.spec.ts similarity index 100% rename from api/src/gateway/escape-regexp.spec.ts rename to api/src/common/escape-regexp.spec.ts diff --git a/api/src/gateway/escape-regexp.ts b/api/src/common/escape-regexp.ts similarity index 100% rename from api/src/gateway/escape-regexp.ts rename to api/src/common/escape-regexp.ts diff --git a/api/src/gateway/gateway.service.ts b/api/src/gateway/gateway.service.ts index 440ea60c..139ef1f3 100644 --- a/api/src/gateway/gateway.service.ts +++ b/api/src/gateway/gateway.service.ts @@ -24,7 +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 './escape-regexp' +import { escapeRegExp } from '../common/escape-regexp' @Injectable() export class GatewayService { From 8b635a2344238e7da5b856485f5e4af159f5ece2 Mon Sep 17 00:00:00 2001 From: isra el Date: Sun, 19 Jul 2026 18:37:40 +0300 Subject: [PATCH 7/7] fix(api): keep push token and serial out of the device list getDevicesForUser did find({ user }) with no projection, so the device list endpoint returned the full document, including the fcmToken push credential and the hardware serial, straight to the browser. Neither is used by the dashboard or the Android client. The query now projects both fields out with '-fcmToken -serial'. Only the user-facing list is narrowed; getDeviceById, which feeds the guards and internal logic, is untouched. Guarded by the existing getDevicesForUser test, rewritten to assert the projection and seen to fail against the pre-change single-argument call. Co-Authored-By: Claude Fable 5 --- api/src/gateway/gateway.service.spec.ts | 9 +++++++-- api/src/gateway/gateway.service.ts | 6 +++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/api/src/gateway/gateway.service.spec.ts b/api/src/gateway/gateway.service.spec.ts index 45eeb8e0..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) }) }) diff --git a/api/src/gateway/gateway.service.ts b/api/src/gateway/gateway.service.ts index 139ef1f3..b0e2c38d 100644 --- a/api/src/gateway/gateway.service.ts +++ b/api/src/gateway/gateway.service.ts @@ -140,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 {