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( { 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( 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 }, })