From 7347f2b730147f00468fb86afb3b31d9bcb2e8c6 Mon Sep 17 00:00:00 2001 From: isra el Date: Sun, 2 Aug 2026 14:29:22 +0300 Subject: [PATCH 1/3] fix(gateway): scope SMS and batch lookups to the requesting device getSMSById and getSmsBatchById resolved records by id alone, so the device in the route was not part of the query. Both now filter on the device, and the batch id supplied in the sms-status body is matched the same way. A record that does not belong to the device reads as not found, so a mismatch is indistinguishable from a missing id. Co-Authored-By: Claude Opus 5 (1M context) --- api/src/gateway/gateway.controller.ts | 7 +- api/src/gateway/gateway.service.spec.ts | 92 +++++++++++++++++++++++++ api/src/gateway/gateway.service.ts | 31 ++++++--- 3 files changed, 119 insertions(+), 11 deletions(-) diff --git a/api/src/gateway/gateway.controller.ts b/api/src/gateway/gateway.controller.ts index 450c972..bee43f4 100644 --- a/api/src/gateway/gateway.controller.ts +++ b/api/src/gateway/gateway.controller.ts @@ -185,7 +185,7 @@ export class GatewayController { @Param('id') deviceId: string, @Param('smsId') smsId: string, ) { - const data = await this.gatewayService.getSMSById(smsId); + const data = await this.gatewayService.getSMSById(deviceId, smsId); return { data }; } @@ -196,7 +196,10 @@ export class GatewayController { @Param('id') deviceId: string, @Param('smsBatchId') smsBatchId: string, ) { - const data = await this.gatewayService.getSmsBatchById(smsBatchId); + const data = await this.gatewayService.getSmsBatchById( + deviceId, + smsBatchId, + ); return { data }; } } diff --git a/api/src/gateway/gateway.service.spec.ts b/api/src/gateway/gateway.service.spec.ts index b77e669..d0c39d5 100644 --- a/api/src/gateway/gateway.service.spec.ts +++ b/api/src/gateway/gateway.service.spec.ts @@ -53,12 +53,15 @@ describe('GatewayService', () => { create: jest.fn(), find: jest.fn(), findOne: jest.fn(), + findById: jest.fn(), + findByIdAndUpdate: jest.fn(), updateMany: jest.fn(), countDocuments: jest.fn(), } const mockSmsBatchModel = { create: jest.fn(), + findOne: jest.fn(), findByIdAndUpdate: jest.fn(), } @@ -914,4 +917,93 @@ describe('GatewayService', () => { }) }) }) + + // The device guard only sees the :id param, so the second identifier on + // these routes has to be bound to the device by the query itself. + describe('SMS lookups are scoped to the requesting device', () => { + const OWN_DEVICE = '507f1f77bcf86cd799439011' + const OTHER_SMS = '507f1f77bcf86cd799439022' + + describe('getSMSById', () => { + it('filters on both the sms id and the device', async () => { + mockSmsModel.findOne.mockResolvedValue({ _id: OTHER_SMS }) + + await service.getSMSById(OWN_DEVICE, OTHER_SMS) + + expect(mockSmsModel.findOne).toHaveBeenCalledWith({ + _id: OTHER_SMS, + device: OWN_DEVICE, + }) + }) + + it('reports another device\'s message as not found', async () => { + mockSmsModel.findOne.mockResolvedValue(null) + + await expect( + service.getSMSById(OWN_DEVICE, OTHER_SMS), + ).rejects.toThrow(HttpException) + }) + + it('reports a malformed id as not found rather than throwing a cast error', async () => { + await expect( + service.getSMSById(OWN_DEVICE, 'not-an-objectid'), + ).rejects.toThrow(HttpException) + expect(mockSmsModel.findOne).not.toHaveBeenCalled() + }) + }) + + describe('getSmsBatchById', () => { + it('filters the batch on the device', async () => { + mockSmsBatchModel.findOne.mockResolvedValue(null) + + await expect( + service.getSmsBatchById(OWN_DEVICE, OTHER_SMS), + ).rejects.toThrow(HttpException) + + const filter = mockSmsBatchModel.findOne.mock.calls[0][0] + expect(filter._id).toBe(OTHER_SMS) + expect(filter.device.toString()).toBe(OWN_DEVICE) + }) + + it('does not read the batch messages when the batch is not this device\'s', async () => { + mockSmsBatchModel.findOne.mockResolvedValue(null) + + await expect( + service.getSmsBatchById(OWN_DEVICE, OTHER_SMS), + ).rejects.toThrow(HttpException) + expect(mockSmsModel.find).not.toHaveBeenCalled() + }) + }) + + describe('updateSMSStatus', () => { + const OTHER_BATCH = '507f1f77bcf86cd799439033' + + it('leaves a batch belonging to another device untouched', async () => { + mockDeviceModel.findById.mockResolvedValue({ + _id: OWN_DEVICE, + user: 'user_1', + }) + mockSmsModel.findById.mockResolvedValue({ + _id: 'own_sms', + device: OWN_DEVICE, + status: 'pending', + }) + mockSmsModel.findByIdAndUpdate.mockResolvedValue({ + _id: 'own_sms', + status: 'sent', + }) + mockSmsBatchModel.findOne.mockResolvedValue(null) + + await service.updateSMSStatus(OWN_DEVICE, { + smsId: 'own_sms', + smsBatchId: OTHER_BATCH, + status: 'sent', + } as any) + + const filter = mockSmsBatchModel.findOne.mock.calls[0][0] + expect(filter.device.toString()).toBe(OWN_DEVICE) + expect(mockSmsBatchModel.findByIdAndUpdate).not.toHaveBeenCalled() + }) + }) + }) }) diff --git a/api/src/gateway/gateway.service.ts b/api/src/gateway/gateway.service.ts index b0e2c38..63b343f 100644 --- a/api/src/gateway/gateway.service.ts +++ b/api/src/gateway/gateway.service.ts @@ -1084,9 +1084,15 @@ const updatedSms = await this.smsModel.findByIdAndUpdate( { new: true } ); - // Check if all SMS in batch have the same status, then update batch status - if (dto.smsBatchId) { - const smsBatch = await this.smsBatchModel.findById(dto.smsBatchId); + // Check if all SMS in batch have the same status, then update batch status. + // The batch id comes from the body, so it is matched against this device. + if (dto.smsBatchId && Types.ObjectId.isValid(dto.smsBatchId)) { + // SMSBatch types `device` as the populated Device, so the filter is cast + // to match the ObjectId actually stored. Runtime behavior is unchanged. + const smsBatch = await this.smsBatchModel.findOne({ + _id: dto.smsBatchId, + device: new Types.ObjectId(deviceId), + } as any); if (smsBatch) { const allSmsInBatch = await this.smsModel.find({ smsBatch: dto.smsBatchId }); @@ -1175,9 +1181,12 @@ const updatedSms = await this.smsModel.findByIdAndUpdate( } } - async getSMSById(smsId: string): Promise { - - const sms = await this.smsModel.findById(smsId); + // Scoped to the device from the route so a message is only reachable through + // the device that owns it. A mismatch is reported as not found. + async getSMSById(deviceId: string, smsId: string): Promise { + const sms = Types.ObjectId.isValid(smsId) + ? await this.smsModel.findOne({ _id: smsId, device: deviceId }) + : null; if (!sms) { throw new HttpException( @@ -1192,9 +1201,13 @@ const updatedSms = await this.smsModel.findByIdAndUpdate( return sms; } - async getSmsBatchById(smsBatchId: string): Promise { - - const smsBatch = await this.smsBatchModel.findById(smsBatchId); + async getSmsBatchById(deviceId: string, smsBatchId: string): Promise { + const smsBatch = Types.ObjectId.isValid(smsBatchId) + ? await this.smsBatchModel.findOne({ + _id: smsBatchId, + device: new Types.ObjectId(deviceId), + } as any) + : null; if (!smsBatch) { throw new HttpException( From fdc1609946b2ae13dfcd85baf31a36bb60f29171 Mon Sep 17 00:00:00 2001 From: isra el Date: Sun, 2 Aug 2026 14:29:31 +0300 Subject: [PATCH 2/3] fix(auth): check Google token audience and align reset responses The tokeninfo response was trusted without checking who the token was issued for, so a token minted for any other Google OAuth client was accepted. The audience is now matched against GOOGLE_CLIENT_ID and the verified-email flag is required. The audience check is skipped with a logged warning when the variable is unset, so an unconfigured environment does not lose Google sign-in. Password reset requests now return the same response whether or not the address is registered, an unknown address on reset reports the same error as a bad code, a non-string email no longer reaches the query, and the code is drawn from crypto.randomInt. Co-Authored-By: Claude Opus 5 (1M context) --- api/.env.example | 7 ++- api/src/auth/auth.service.spec.ts | 101 ++++++++++++++++++++++++++++++ api/src/auth/auth.service.ts | 70 ++++++++++++++++++--- 3 files changed, 170 insertions(+), 8 deletions(-) diff --git a/api/.env.example b/api/.env.example index 46ea35d..825335e 100644 --- a/api/.env.example +++ b/api/.env.example @@ -39,4 +39,9 @@ WEBHOOK_AUTO_DISABLE_MIN_FAILURE_RATE=0.50 USE_SMS_QUEUE=false REDIS_URL=redis://localhost:6379 # if queue is enabled, redis url is required -CLOUDFLARE_TURNSTILE_SECRET_KEY=1x0000000000000000000000000000000AA \ No newline at end of file +CLOUDFLARE_TURNSTILE_SECRET_KEY=1x0000000000000000000000000000000AA + +# OAuth client id(s) that Google sign-in tokens must be issued for. Must match +# the web app's NEXT_PUBLIC_GOOGLE_CLIENT_ID. Comma-separate for several clients. +# When unset, the audience is not checked and a warning is logged. +GOOGLE_CLIENT_ID= \ No newline at end of file diff --git a/api/src/auth/auth.service.spec.ts b/api/src/auth/auth.service.spec.ts index 6698800..cfd9c68 100644 --- a/api/src/auth/auth.service.spec.ts +++ b/api/src/auth/auth.service.spec.ts @@ -1,6 +1,7 @@ import { HttpException } from '@nestjs/common' import * as bcrypt from 'bcryptjs' import { createHash } from 'crypto' +import axios from 'axios' import { AuthService } from './auth.service' const sha256 = (value: string) => @@ -575,5 +576,105 @@ describe('AuthService', () => { { attempts: { $exists: false } }, ]) }) + + it('reports an unknown address the same way as a bad code', async () => { + const ctx = build() + ctx.usersService.findOne.mockResolvedValue(null) + + await expect( + ctx.service.resetPassword({ + email: 'nobody@b.com', + otp: '123456', + newPassword: 'Str0ng!pass', + } as any), + ).rejects.toMatchObject({ + response: { error: 'Invalid OTP' }, + status: 400, + }) + }) + + it('does not pass a non-string email to the user lookup', async () => { + const ctx = build() + + await expect( + ctx.service.resetPassword({ + email: { $ne: null } as any, + otp: '123456', + newPassword: 'Str0ng!pass', + } as any), + ).rejects.toThrow(HttpException) + expect(ctx.usersService.findOne).not.toHaveBeenCalled() + }) + }) + + // tokeninfo only proves Google signed the token. Without these checks a token + // minted for any other Google OAuth client would be accepted. + describe('loginWithGoogle', () => { + const ORIGINAL_ENV = process.env.GOOGLE_CLIENT_ID + const OURS = 'our-client-id.apps.googleusercontent.com' + + const stageTokenInfo = (data: any) => { + jest.spyOn(axios, 'get').mockResolvedValue({ data } as any) + } + + beforeEach(() => { + process.env.GOOGLE_CLIENT_ID = OURS + }) + + afterEach(() => { + process.env.GOOGLE_CLIENT_ID = ORIGINAL_ENV + jest.restoreAllMocks() + }) + + it('rejects a token minted for a different OAuth client', async () => { + const ctx = build() + stageTokenInfo({ + aud: 'someone-elses-client.apps.googleusercontent.com', + email: 'victim@example.com', + email_verified: 'true', + sub: 'g1', + }) + + await expect(ctx.service.loginWithGoogle('tok')).rejects.toThrow( + HttpException, + ) + expect(ctx.usersService.findOne).not.toHaveBeenCalled() + }) + + it('rejects a token whose email is not verified', async () => { + const ctx = build() + stageTokenInfo({ + aud: OURS, + email: 'victim@example.com', + email_verified: 'false', + sub: 'g1', + }) + + await expect(ctx.service.loginWithGoogle('tok')).rejects.toThrow( + HttpException, + ) + expect(ctx.usersService.findOne).not.toHaveBeenCalled() + }) + + it('accepts our own audience with a verified email', async () => { + const ctx = build() + stageTokenInfo({ + aud: OURS, + email: 'ada@example.com', + email_verified: 'true', + sub: 'g1', + name: 'Ada', + }) + ctx.usersService.findOne.mockResolvedValue({ + _id: 'user_1', + email: 'ada@example.com', + save: jest.fn().mockResolvedValue(undefined), + toObject: () => ({ _id: 'user_1', email: 'ada@example.com' }), + }) + + const result = await ctx.service.loginWithGoogle('tok') + + expect(result.accessToken).toBe('signed-jwt') + }) }) }) diff --git a/api/src/auth/auth.service.ts b/api/src/auth/auth.service.ts index 2f0e4dc..210ab16 100644 --- a/api/src/auth/auth.service.ts +++ b/api/src/auth/auth.service.ts @@ -82,11 +82,47 @@ export class AuthService { } } + // tokeninfo only proves Google signed the token, not that it was issued for + // this app, so the audience and the verified-email flag are checked here. + private assertGoogleTokenIsForThisApp(tokenInfo: { + aud?: string + email_verified?: boolean | string + }) { + const allowedAudiences = (process.env.GOOGLE_CLIENT_ID ?? '') + .split(',') + .map((value) => value.trim()) + .filter(Boolean) + + if (!allowedAudiences.length) { + // Unset in this environment: log rather than reject, so a missing config + // value cannot take Google sign-in down. Set GOOGLE_CLIENT_ID to enable. + console.error( + 'loginWithGoogle: GOOGLE_CLIENT_ID is not set, skipping audience check', + ) + } else if (!allowedAudiences.includes(tokenInfo.aud)) { + throw new HttpException({ error: 'Unauthorized' }, HttpStatus.UNAUTHORIZED) + } + + if ( + tokenInfo.email_verified !== true && + tokenInfo.email_verified !== 'true' + ) { + throw new HttpException( + { error: 'Google account email is not verified' }, + HttpStatus.UNAUTHORIZED, + ) + } + } + async loginWithGoogle(idToken: string) { const response = await axios.get( - `https://oauth2.googleapis.com/tokeninfo?id_token=${idToken}`, + `https://oauth2.googleapis.com/tokeninfo?id_token=${encodeURIComponent( + idToken, + )}`, ) + this.assertGoogleTokenIsForThisApp(response.data) + const { sub: googleId, name, email, picture } = response.data let user = await this.usersService.findOne({ email }) @@ -168,11 +204,20 @@ export class AuthService { }: RequestResetPasswordInputDTO) { await this.turnstileService.verify(turnstileToken) + // Both branches below return this, so the response says nothing about + // whether the address is registered. + const acceptedResponse = { + message: 'If email is found you will receive a password reset email', + } + + // Guards against a non-string reaching the query as a Mongo operator. + if (typeof email !== 'string') { + return acceptedResponse + } + const user = await this.usersService.findOne({ email }) if (!user) { - return { - message: 'If email is found you will receive a password reset email', - } + return acceptedResponse } // Check if user has requested password reset more than 5 times in the last 24 hours @@ -189,7 +234,7 @@ export class AuthService { ) } - const otp = Math.floor(100000 + Math.random() * 900000).toString() + const otp = randomInt(100000, 1000000).toString() const expiresAt = new Date(Date.now() + 20 * 60 * 1000) const hashedOtp = await bcrypt.hash(otp, 10) @@ -209,13 +254,24 @@ export class AuthService { context: { name: user.name, resetLink, otp }, }) - return { message: 'Password reset email sent' } + return acceptedResponse } async resetPassword({ email, otp, newPassword }: ResetPasswordInputDTO) { + // Matches the other failure paths below so an unknown address is not + // distinguishable from a bad code. + const invalidOtp = new HttpException( + { error: 'Invalid OTP' }, + HttpStatus.BAD_REQUEST, + ) + + if (typeof email !== 'string') { + throw invalidOtp + } + const user = await this.usersService.findOne({ email }) if (!user) { - throw new HttpException({ error: 'User not found' }, HttpStatus.NOT_FOUND) + throw invalidOtp } const latestReset = await this.passwordResetModel.findOne( { From 083f2c8cd55aadb9f5093bd779a91d563721c133 Mon Sep 17 00:00:00 2001 From: isra el Date: Sun, 2 Aug 2026 14:29:31 +0300 Subject: [PATCH 3/3] fix(support): require authentication for support requests The customer-support route accepted anonymous callers and only overwrote the user field when one was authenticated, so the body could set it. The route now requires authentication and always takes the user from the token. The daily cap counts per authenticated user rather than per self-reported email address. Co-Authored-By: Claude Opus 5 (1M context) --- api/src/support/support.controller.ts | 10 ++++------ api/src/support/support.service.ts | 5 +++-- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/api/src/support/support.controller.ts b/api/src/support/support.controller.ts index 08b585f..80a1096 100644 --- a/api/src/support/support.controller.ts +++ b/api/src/support/support.controller.ts @@ -6,7 +6,7 @@ import { import { SupportService } from './support.service' import { JwtAuthGuard } from '../auth/jwt-auth.guard' import { Request } from 'express' -import { OptionalAuthGuard } from '../auth/guards/optional-auth.guard' +import { AuthGuard } from '../auth/guards/auth.guard' import { TurnstileService } from '../common/turnstile.service' @Controller('support') @@ -16,7 +16,7 @@ export class SupportController { private readonly turnstileService: TurnstileService, ) {} - @UseGuards(OptionalAuthGuard) + @UseGuards(AuthGuard) @Post('customer-support') async createSupportMessage( @Body() createSupportMessageDto: CreateSupportMessageDto, @@ -31,10 +31,8 @@ export class SupportController { createSupportMessageDto.ip = ip createSupportMessageDto.userAgent = userAgent - // If user is authenticated, associate the support request with the user - if (req.user) { - createSupportMessageDto.user = req.user['_id'] - } + // Always taken from the token so the body cannot set it + createSupportMessageDto.user = req.user['_id'] return this.supportService.createSupportMessage(createSupportMessageDto) } diff --git a/api/src/support/support.service.ts b/api/src/support/support.service.ts index 2dfeb69..dcc90d8 100644 --- a/api/src/support/support.service.ts +++ b/api/src/support/support.service.ts @@ -32,10 +32,11 @@ export class SupportService { ): Promise<{ message: string }> { const { turnstileToken, ...sanitizedDto } = createSupportMessageDto try { - // Check rate limit: max 3 requests per 24 hours + // Check rate limit: max 3 requests per 24 hours. + // Keyed on the authenticated user, not the self-reported email. const twentyFourHoursAgo = new Date(Date.now() - 24 * 60 * 60 * 1000) const recentRequestsCount = await this.supportMessageModel.countDocuments({ - email: sanitizedDto.email, + user: sanitizedDto.user, createdAt: { $gte: twentyFourHoursAgo }, })