Merge pull request #253 from vernu/dev

Dev

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
+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=

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')
+    })
   })
 })

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(
       {

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 };
   }
 }

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()
+      })
+    })
+  })
 })

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<any> {
-
-    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<any> {
+    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<any> {
-
-    const smsBatch = await this.smsBatchModel.findById(smsBatchId);
+  async getSmsBatchById(deviceId: string, smsBatchId: string): Promise<any> {
+    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(

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)
   }

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