fix: Binds PKCE challenge to user uuid

packages/auth/src/Bootstrap/Container.ts

@@ -1347,6 +1347,7 @@ export class ContainerConfigLoader {
           container.get<number>(TYPES.Auth_MAX_LOGIN_ATTEMPTS),
           container.get<LockRepositoryInterface>(TYPES.Auth_LockRepository),
           container.get<VerifyHumanInteraction>(TYPES.Auth_VerifyHumanInteraction),
+          container.get<winston.Logger>(TYPES.Auth_Logger),
         ),
       )
     container

packages/auth/src/Domain/UseCase/GetUserKeyParams/GetUserKeyParams.spec.ts

@@ -21,7 +21,7 @@ describe('GetUserKeyParams', () => {
     keyParamsFactory.create = jest.fn().mockReturnValue({ foo: 'bar' })
     keyParamsFactory.createPseudoParams = jest.fn().mockReturnValue({ bar: 'baz' })
 
-    user = {} as jest.Mocked<User>
+    user = { uuid: '1-2-3' } as jest.Mocked<User>
 
     userRepository = {} as jest.Mocked<UserRepositoryInterface>
     userRepository.findOneByUsernameOrEmail = jest.fn().mockReturnValue(user)
@@ -97,7 +97,7 @@ describe('GetUserKeyParams', () => {
       },
     })
 
-    expect(pkceRepository.storeCodeChallenge).toHaveBeenCalledWith('test')
+    expect(pkceRepository.storeCodeChallenge).toHaveBeenCalledWith('test', '1-2-3')
   })
 
   it('should get pseudo key params for a non existing user - when searching by email', async () => {

packages/auth/src/Domain/UseCase/GetUserKeyParams/GetUserKeyParams.ts

@@ -63,7 +63,7 @@ export class GetUserKeyParams implements UseCaseInterface {
 
   private async createKeyParams(dto: GetUserKeyParamsDTO, user: User, authenticated: boolean): Promise<KeyParamsData> {
     if (this.isCodeChallengedVersion(dto)) {
-      await this.pkceRepository.storeCodeChallenge(dto.codeChallenge)
+      await this.pkceRepository.storeCodeChallenge(dto.codeChallenge, user.uuid)
     }
 
     return this.keyParamsFactory.create(user, authenticated)

packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParams.spec.ts

@@ -22,7 +22,7 @@ describe('GetUserKeyParamsRecovery', () => {
     keyParamsFactory.create = jest.fn().mockReturnValue({ foo: 'bar' })
     keyParamsFactory.createPseudoParams = jest.fn().mockReturnValue({ bar: 'baz' })
 
-    user = {} as jest.Mocked<User>
+    user = { uuid: 'user-uuid' } as jest.Mocked<User>
 
     userRepository = {} as jest.Mocked<UserRepositoryInterface>
     userRepository.findOneByUsernameOrEmail = jest.fn().mockReturnValue(user)
@@ -121,6 +121,7 @@ describe('GetUserKeyParamsRecovery', () => {
     })
 
     expect(keyParamsFactory.create).toHaveBeenCalled()
+    expect(pkceRepository.storeCodeChallenge).toHaveBeenCalledWith('codeChallenge', 'user-uuid')
     expect(result.isFailed()).toBe(false)
   })
 

packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParamsRecovery.ts

@@ -70,7 +70,7 @@ export class GetUserKeyParamsRecovery implements UseCaseInterface<KeyParamsData>
   }
 
   private async createKeyParams(codeChallenge: string, user: User): Promise<KeyParamsData> {
-    await this.pkceRepository.storeCodeChallenge(codeChallenge)
+    await this.pkceRepository.storeCodeChallenge(codeChallenge, user.uuid)
 
     return this.keyParamsFactory.create(user, false)
   }

packages/auth/src/Domain/UseCase/SignIn.spec.ts

@@ -242,9 +242,31 @@ describe('SignIn', () => {
 
     expect(domainEventFactory.createEmailRequestedEvent).toHaveBeenCalled()
     expect(domainEventPublisher.publish).toHaveBeenCalled()
+    expect(pkceRepository.removeCodeChallenge).toHaveBeenCalledWith('base64-url-encoded', '1-2-3')
     expect(clearLoginAttempts.execute).toHaveBeenCalledWith({ email: '[email protected]' })
   })
 
+  it('should not sign in when pkce challenge was registered for a different user', async () => {
+    pkceRepository.removeCodeChallenge = jest.fn().mockReturnValue(false)
+
+    expect(
+      await createUseCase().execute({
+        email: '[email protected]',
+        password: 'qweqwe123123',
+        userAgent: 'Google Chrome',
+        apiVersion: '20190520',
+        ephemeralSession: false,
+        codeVerifier: 'test',
+      }),
+    ).toEqual({
+      success: false,
+      errorMessage: 'Invalid email or password',
+      isNonCaptchaLimitReached: false,
+    })
+
+    expect(pkceRepository.removeCodeChallenge).toHaveBeenCalledWith('base64-url-encoded', '1-2-3')
+  })
+
   it('should sign in a user even if publishing a sign in event fails', async () => {
     domainEventPublisher.publish = jest.fn().mockImplementation(() => {
       throw new Error('Oops')

packages/auth/src/Domain/UseCase/SignIn.ts

@@ -47,13 +47,6 @@ export class SignIn implements UseCaseInterface {
       )
     }
 
-    const validCodeVerifier = await this.validateCodeVerifier(dto.codeVerifier)
-    if (!validCodeVerifier) {
-      this.logger.debug('Code verifier does not match')
-
-      return this.failAfterIncrementingLoginAttempts(dto.email, 'Invalid email or password')
-    }
-
     const apiVersionOrError = ApiVersion.create(dto.apiVersion)
     if (apiVersionOrError.isFailed()) {
       return this.failAfterIncrementingLoginAttempts(dto.email, apiVersionOrError.getError())
@@ -88,6 +81,13 @@ export class SignIn implements UseCaseInterface {
       return this.failAfterIncrementingLoginAttempts(dto.email, 'Invalid email or password')
     }
 
+    const validCodeVerifier = await this.validateCodeVerifier(dto.codeVerifier, user.uuid)
+    if (!validCodeVerifier) {
+      this.logger.debug('Code verifier does not match')
+
+      return this.failAfterIncrementingLoginAttempts(dto.email, 'Invalid email or password')
+    }
+
     const passwordMatches = await bcrypt.compare(dto.password, user.encryptedPassword)
     if (!passwordMatches) {
       this.logger.debug('Password does not match')
@@ -137,10 +137,13 @@ export class SignIn implements UseCaseInterface {
     }
   }
 
-  private async validateCodeVerifier(codeVerifier: string): Promise<boolean> {
+  private async validateCodeVerifier(codeVerifier: string, userUuid: string): Promise<boolean> {
     const codeChallenge = this.crypter.base64URLEncode(this.crypter.sha256Hash(codeVerifier))
 
-    const matchingCodeChallengeWasPresentAndRemoved = await this.pkceRepository.removeCodeChallenge(codeChallenge)
+    const matchingCodeChallengeWasPresentAndRemoved = await this.pkceRepository.removeCodeChallenge(
+      codeChallenge,
+      userUuid,
+    )
 
     return matchingCodeChallengeWasPresentAndRemoved
   }

packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.spec.ts

@@ -16,6 +16,7 @@ import { GetSetting } from '../GetSetting/GetSetting'
 import { ApiVersion } from '../../Api/ApiVersion'
 import { LockRepositoryInterface } from '../../User/LockRepositoryInterface'
 import { VerifyHumanInteraction } from '../VerifyHumanInteraction/VerifyHumanInteraction'
+import { Logger } from 'winston'
 
 describe('SignInWithRecoveryCodes', () => {
   let userRepository: UserRepositoryInterface
@@ -31,6 +32,7 @@ describe('SignInWithRecoveryCodes', () => {
   let maxNonCaptchaAttempts: number
   let lockRepository: LockRepositoryInterface
   let verifyHumanInteractionUseCase: VerifyHumanInteraction
+  let logger: Logger
 
   const createUseCase = () =>
     new SignInWithRecoveryCodes(
@@ -47,6 +49,7 @@ describe('SignInWithRecoveryCodes', () => {
       maxNonCaptchaAttempts,
       lockRepository,
       verifyHumanInteractionUseCase,
+      logger,
     )
 
   beforeEach(() => {
@@ -93,6 +96,9 @@ describe('SignInWithRecoveryCodes', () => {
 
     verifyHumanInteractionUseCase = {} as jest.Mocked<VerifyHumanInteraction>
     verifyHumanInteractionUseCase.execute = jest.fn().mockReturnValue(Result.ok())
+
+    logger = {} as jest.Mocked<Logger>
+    logger.debug = jest.fn()
   })
 
   const requireHumanVerification = () => {
@@ -201,7 +207,7 @@ describe('SignInWithRecoveryCodes', () => {
 
     expect(result).toEqual({
       success: false,
-      errorMessage: 'Could not find user',
+      errorMessage: 'Invalid code verifier',
       isNonCaptchaLimitReached: false,
     })
   })

packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.ts

@@ -1,5 +1,6 @@
 import * as bcrypt from 'bcryptjs'
 import { Result, SettingName, Username, Uuid, Validator } from '@standardnotes/domain-core'
+import { Logger } from 'winston'
 
 import { CrypterInterface } from '../../Encryption/CrypterInterface'
 import { PKCERepositoryInterface } from '../../User/PKCERepositoryInterface'
@@ -34,6 +35,7 @@ export class SignInWithRecoveryCodes implements UseCaseInterface {
     private maxNonCaptchaAttempts: number,
     private lockRepository: LockRepositoryInterface,
     private verifyHumanInteractionUseCase: VerifyHumanInteraction,
+    private logger: Logger,
   ) {}
 
   async execute(dto: SignInWithRecoveryCodesDTO): Promise<SignInWithRecoveryCodesResponse> {
@@ -71,8 +73,16 @@ export class SignInWithRecoveryCodes implements UseCaseInterface {
       }
     }
 
-    const validCodeVerifier = await this.validateCodeVerifier(dto.codeVerifier)
+    if (!user) {
+      this.logger.debug(`User with username ${username.value} was not found`)
+
+      return this.failAfterIncrementingLoginAttempts(username.value, 'Invalid code verifier')
+    }
+
+    const validCodeVerifier = await this.validateCodeVerifier(dto.codeVerifier, user.uuid)
     if (!validCodeVerifier) {
+      this.logger.debug('Code verifier does not match')
+
       return this.failAfterIncrementingLoginAttempts(username.value, 'Invalid code verifier')
     }
 
@@ -86,10 +96,6 @@ export class SignInWithRecoveryCodes implements UseCaseInterface {
       return this.failAfterIncrementingLoginAttempts(username.value, 'Empty recovery codes')
     }
 
-    if (!user) {
-      return this.failAfterIncrementingLoginAttempts(username.value, 'Could not find user')
-    }
-
     const userUuidOrError = Uuid.create(user.uuid)
     if (userUuidOrError.isFailed()) {
       return this.failAfterIncrementingLoginAttempts(username.value, 'Invalid user uuid')
@@ -169,7 +175,7 @@ export class SignInWithRecoveryCodes implements UseCaseInterface {
     }
   }
 
-  private async validateCodeVerifier(codeVerifier: string): Promise<boolean> {
+  private async validateCodeVerifier(codeVerifier: string, userUuid: string): Promise<boolean> {
     const codeEmptinessVerificationResult = Validator.isNotEmpty(codeVerifier)
     if (codeEmptinessVerificationResult.isFailed()) {
       return false
@@ -177,7 +183,10 @@ export class SignInWithRecoveryCodes implements UseCaseInterface {
 
     const codeChallenge = this.crypter.base64URLEncode(this.crypter.sha256Hash(codeVerifier))
 
-    const matchingCodeChallengeWasPresentAndRemoved = await this.pkceRepository.removeCodeChallenge(codeChallenge)
+    const matchingCodeChallengeWasPresentAndRemoved = await this.pkceRepository.removeCodeChallenge(
+      codeChallenge,
+      userUuid,
+    )
 
     return matchingCodeChallengeWasPresentAndRemoved
   }

packages/auth/src/Domain/User/PKCERepositoryInterface.ts

@@ -1,4 +1,4 @@
 export interface PKCERepositoryInterface {
-  storeCodeChallenge(codeChallenge: string): Promise<void>
-  removeCodeChallenge(codeChallenge: string): Promise<boolean>
+  storeCodeChallenge(codeChallenge: string, userUuid: string): Promise<void>
+  removeCodeChallenge(codeChallenge: string, userUuid: string): Promise<boolean>
 }

packages/auth/src/Infra/Redis/RedisPKCERepository.ts

@@ -14,14 +14,22 @@ export class RedisPKCERepository implements PKCERepositoryInterface {
     @inject(TYPES.Auth_Logger) private logger: Logger,
   ) {}
 
-  async storeCodeChallenge(codeChallenge: string): Promise<void> {
+  async storeCodeChallenge(codeChallenge: string, userUuid: string): Promise<void> {
     this.logger.debug(`Storing code challenge: ${codeChallenge}`)
 
-    await this.redisClient.setex(`${this.PREFIX}:${codeChallenge}`, 3600, codeChallenge)
+    await this.redisClient.setex(`${this.PREFIX}:${codeChallenge}`, 3600, userUuid)
   }
 
-  async removeCodeChallenge(codeChallenge: string): Promise<boolean> {
-    const entriesRemoved = await this.redisClient.del(`${this.PREFIX}:${codeChallenge}`)
+  async removeCodeChallenge(codeChallenge: string, userUuid: string): Promise<boolean> {
+    const key = `${this.PREFIX}:${codeChallenge}`
+    const storedUserUuid = await this.redisClient.get(key)
+
+    // Legacy entries (pre user-uuid binding) stored value = codeChallenge; remove after 3600s TTL window.
+    if (!storedUserUuid || (storedUserUuid !== userUuid && storedUserUuid !== codeChallenge)) {
+      return false
+    }
+
+    const entriesRemoved = await this.redisClient.del(key)
 
     this.logger.debug(`Removed ${entriesRemoved} entries for code challenge: ${codeChallenge}`)
 

packages/auth/src/Infra/TypeORM/TypeORMPKCERepository.ts

@@ -13,20 +13,29 @@ export class TypeORMPKCERepository implements PKCERepositoryInterface {
     private timer: TimerInterface,
   ) {}
 
-  async storeCodeChallenge(codeChallenge: string): Promise<void> {
+  async storeCodeChallenge(codeChallenge: string, userUuid: string): Promise<void> {
     this.logger.debug(`Storing code challenge: ${codeChallenge}`)
 
     await this.cacheEntryRepository.save(
       CacheEntry.create({
         key: `${this.PREFIX}:${codeChallenge}`,
-        value: codeChallenge,
+        value: userUuid,
         expiresAt: this.timer.getUTCDateNSecondsAhead(3600),
       }).getValue(),
     )
   }
 
-  async removeCodeChallenge(codeChallenge: string): Promise<boolean> {
-    await this.cacheEntryRepository.removeByKey(`${this.PREFIX}:${codeChallenge}`)
+  async removeCodeChallenge(codeChallenge: string, userUuid: string): Promise<boolean> {
+    const key = `${this.PREFIX}:${codeChallenge}`
+    const cacheEntry = await this.cacheEntryRepository.findUnexpiredOneByKey(key)
+
+    // Legacy entries (pre user-uuid binding) stored value = codeChallenge; remove after 3600s TTL window.
+    const storedValue = cacheEntry?.props.value
+    if (!storedValue || (storedValue !== userUuid && storedValue !== codeChallenge)) {
+      return false
+    }
+
+    await this.cacheEntryRepository.removeByKey(key)
 
     return true
   }