From 65a846a43685392eafbb9dd777cfbe0134422783 Mon Sep 17 00:00:00 2001 From: Antonella Sgarlatta Date: Sat, 18 Apr 2026 00:35:26 -0300 Subject: [PATCH 01/17] fix: Fixes content type authorization check for shared vaults --- .../Item/SaveRule/SharedVaultFilter.spec.ts | 45 +++++++++++++++++++ .../Domain/Item/SaveRule/SharedVaultFilter.ts | 16 +++++-- 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/packages/syncing-server/src/Domain/Item/SaveRule/SharedVaultFilter.spec.ts b/packages/syncing-server/src/Domain/Item/SaveRule/SharedVaultFilter.spec.ts index 33c0a665b..b2a5b2fff 100644 --- a/packages/syncing-server/src/Domain/Item/SaveRule/SharedVaultFilter.spec.ts +++ b/packages/syncing-server/src/Domain/Item/SaveRule/SharedVaultFilter.spec.ts @@ -754,6 +754,51 @@ describe('SharedVaultFilter', () => { expect(result.passed).toBe(false) }) + + it('should return as not passed if existing item is key system items key and incoming content type is null', async () => { + sharedVaultUser = SharedVaultUser.create({ + permission: SharedVaultUserPermission.create(SharedVaultUserPermission.PERMISSIONS.Write).getValue(), + sharedVaultUuid: Uuid.create('00000000-0000-0000-0000-000000000000').getValue(), + userUuid: Uuid.create('00000000-0000-0000-0000-000000000000').getValue(), + timestamps: Timestamps.create(123, 123).getValue(), + isDesignatedSurvivor: false, + }).getValue() + + existingItem = Item.create({ + ...existingItem.props, + contentType: ContentType.create(ContentType.TYPES.KeySystemItemsKey).getValue(), + }).getValue() + + itemHash = ItemHash.create({ + ...itemHash.props, + content_type: null, + }).getValue() + + determineSharedVaultOperationOnItem.execute = jest.fn().mockReturnValue( + Result.ok( + SharedVaultOperationOnItem.create({ + userUuid: Uuid.create('00000000-0000-0000-0000-000000000000').getValue(), + sharedVaultUuid: Uuid.create('00000000-0000-0000-0000-000000000000').getValue(), + type: SharedVaultOperationOnItem.TYPES.SaveToSharedVault, + incomingItemHash: itemHash, + existingItem, + }).getValue(), + ), + ) + + sharedVaultUserRepository.findByUserUuidAndSharedVaultUuid = jest.fn().mockResolvedValue(sharedVaultUser) + + const filter = createFilter() + const result = await filter.check({ + apiVersion: '001', + existingItem: existingItem, + itemHash: itemHash, + userUuid: '00000000-0000-0000-0000-000000000000', + snjsVersion: '2.200.0', + }) + + expect(result.passed).toBe(false) + }) }) describe('when the shared vault operation on item is: create to shared vault', () => { diff --git a/packages/syncing-server/src/Domain/Item/SaveRule/SharedVaultFilter.ts b/packages/syncing-server/src/Domain/Item/SaveRule/SharedVaultFilter.ts index 7fcefea28..cccd757bf 100644 --- a/packages/syncing-server/src/Domain/Item/SaveRule/SharedVaultFilter.ts +++ b/packages/syncing-server/src/Domain/Item/SaveRule/SharedVaultFilter.ts @@ -101,8 +101,12 @@ export class SharedVaultFilter implements ItemSaveRuleInterface { } } - private isAuthorizedToSaveContentType(contentType: string | null, permission: SharedVaultUserPermission): boolean { - if (contentType === ContentType.TYPES.KeySystemItemsKey) { + private isAuthorizedToSaveContentType( + incomingContentType: string | null, + existingContentType: string | null, + permission: SharedVaultUserPermission, + ): boolean { + if ([incomingContentType, existingContentType].includes(ContentType.TYPES.KeySystemItemsKey)) { return permission.value === SharedVaultUserPermission.PERMISSIONS.Admin } @@ -177,8 +181,14 @@ export class SharedVaultFilter implements ItemSaveRuleInterface { operation: SharedVaultOperationOnItem, sharedVaultPermission: SharedVaultUserPermission, ): boolean { + const existingContentType = operation.props.existingItem?.props.contentType.value ?? null + if ( - !this.isAuthorizedToSaveContentType(operation.props.incomingItemHash.props.content_type, sharedVaultPermission) + !this.isAuthorizedToSaveContentType( + operation.props.incomingItemHash.props.content_type, + existingContentType, + sharedVaultPermission, + ) ) { return false } From 46bbf323f6b3ad245b02c3f8d2502185863ec6d3 Mon Sep 17 00:00:00 2001 From: Antonella Sgarlatta Date: Tue, 21 Apr 2026 14:25:57 -0300 Subject: [PATCH 02/17] fix: Adds authentication for bt-token endpoint --- packages/api-gateway/src/Controller/v1/PaymentsController.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/api-gateway/src/Controller/v1/PaymentsController.ts b/packages/api-gateway/src/Controller/v1/PaymentsController.ts index 368deda0a..7fff57e34 100644 --- a/packages/api-gateway/src/Controller/v1/PaymentsController.ts +++ b/packages/api-gateway/src/Controller/v1/PaymentsController.ts @@ -145,6 +145,11 @@ export class PaymentsController extends BaseHttpController { await this.httpService.callPaymentsServer(request, response, 'api/pro_users/stripe-setup-intent', request.body) } + @httpGet('/pro_users/get-bt-token', TYPES.ApiGateway_SubscriptionTokenAuthMiddleware) + async getBraintreeToken(request: Request, response: Response): Promise { + await this.httpService.callPaymentsServer(request, response, 'api/pro_users/get-bt-token', request.body) + } + @all('/pro_users(/*)?') async proUsers(request: Request, response: Response): Promise { await this.httpService.callPaymentsServer(request, response, request.path.replace('v1', 'api'), request.body) From 9eb2b22c6eb58e8cafc7b3fd9d7dff9e2475098d Mon Sep 17 00:00:00 2001 From: Antonella Sgarlatta Date: Mon, 27 Apr 2026 15:47:10 +0000 Subject: [PATCH 03/17] fix: Validates shared vault owner context on token creation --- .../src/Controller/AuthMiddleware.ts | 10 +- .../CreateCrossServiceToken.spec.ts | 103 +++++++++++++++--- .../CreateCrossServiceToken.ts | 28 ++++- 3 files changed, 121 insertions(+), 20 deletions(-) diff --git a/packages/api-gateway/src/Controller/AuthMiddleware.ts b/packages/api-gateway/src/Controller/AuthMiddleware.ts index 8e3af806f..5cb230376 100644 --- a/packages/api-gateway/src/Controller/AuthMiddleware.ts +++ b/packages/api-gateway/src/Controller/AuthMiddleware.ts @@ -30,14 +30,14 @@ export abstract class AuthMiddleware extends BaseMiddleware { const authHeaderValue = request.headers.authorization as string const sharedVaultOwnerContextHeaderValue = request.headers['x-shared-vault-owner-context'] as string | undefined - const cacheKey = `${authHeaderValue}${ - sharedVaultOwnerContextHeaderValue ? `:${sharedVaultOwnerContextHeaderValue}` : '' - }` + const shouldUseCrossServiceTokenCache = + !!this.crossServiceTokenCacheTTL && sharedVaultOwnerContextHeaderValue === undefined + const cacheKey = authHeaderValue try { let crossServiceTokenFetchedFromCache = true let crossServiceToken = null - if (this.crossServiceTokenCacheTTL) { + if (shouldUseCrossServiceTokenCache) { crossServiceToken = await this.crossServiceTokenCache.get(cacheKey) } @@ -83,7 +83,7 @@ export abstract class AuthMiddleware extends BaseMiddleware { const decodedToken = verify(crossServiceToken, this.jwtSecret, { algorithms: ['HS256'] }) - if (this.crossServiceTokenCacheTTL && !crossServiceTokenFetchedFromCache) { + if (shouldUseCrossServiceTokenCache && !crossServiceTokenFetchedFromCache) { await this.crossServiceTokenCache.set({ key: cacheKey, encodedCrossServiceToken: crossServiceToken, diff --git a/packages/auth/src/Domain/UseCase/CreateCrossServiceToken/CreateCrossServiceToken.spec.ts b/packages/auth/src/Domain/UseCase/CreateCrossServiceToken/CreateCrossServiceToken.spec.ts index 1716a42cb..351d65a7e 100644 --- a/packages/auth/src/Domain/UseCase/CreateCrossServiceToken/CreateCrossServiceToken.spec.ts +++ b/packages/auth/src/Domain/UseCase/CreateCrossServiceToken/CreateCrossServiceToken.spec.ts @@ -27,6 +27,10 @@ import { GetActiveSessionsForUser } from '../GetActiveSessionsForUser' import { Permission } from '../../Permission/Permission' describe('CreateCrossServiceToken', () => { + const authenticatedUserUuid = '00000000-0000-0000-0000-000000000000' + const sharedVaultOwnerContextUuid = '10000000-0000-0000-0000-000000000000' + const sharedVaultUuid = '00000000-0000-0000-0000-000000000000' + let userProjector: ProjectorInterface let sessionProjector: ProjectorInterface let roleProjector: ProjectorInterface @@ -78,15 +82,13 @@ describe('CreateCrossServiceToken', () => { role.permissions = Promise.resolve([]) user = { - uuid: '00000000-0000-0000-0000-000000000000', + uuid: authenticatedUserUuid, email: 'test@test.te', } as jest.Mocked user.roles = Promise.resolve([role]) userProjector = {} as jest.Mocked> - userProjector.projectSimple = jest - .fn() - .mockReturnValue({ uuid: '00000000-0000-0000-0000-000000000000', email: 'test@test.te' }) + userProjector.projectSimple = jest.fn().mockReturnValue({ uuid: authenticatedUserUuid, email: 'test@test.te' }) roleProjector = {} as jest.Mocked> roleProjector.projectSimple = jest.fn().mockReturnValue({ name: 'role1', uuid: '1-3-4' }) @@ -110,7 +112,7 @@ describe('CreateCrossServiceToken', () => { value: '100', timestamps: Timestamps.create(123456789, 123456789).getValue(), serverEncryptionVersion: EncryptionVersion.Unencrypted, - userSubscriptionUuid: Uuid.create('00000000-0000-0000-0000-000000000000').getValue(), + userSubscriptionUuid: Uuid.create(authenticatedUserUuid).getValue(), }).getValue(), }), ) @@ -119,15 +121,21 @@ describe('CreateCrossServiceToken', () => { getRegularSubscription.execute = jest.fn().mockReturnValue(Result.fail('not found')) sharedVaultUserRepository = {} as jest.Mocked - sharedVaultUserRepository.findByUserUuid = jest.fn().mockReturnValue([ - SharedVaultUser.create({ + sharedVaultUserRepository.findByUserUuid = jest.fn().mockImplementation((userUuid: Uuid) => { + const commonAssociation = SharedVaultUser.create({ permission: SharedVaultUserPermission.create('read').getValue(), - sharedVaultUuid: Uuid.create('00000000-0000-0000-0000-000000000000').getValue(), + sharedVaultUuid: Uuid.create(sharedVaultUuid).getValue(), timestamps: Timestamps.create(123456789, 123456789).getValue(), - userUuid: Uuid.create('00000000-0000-0000-0000-000000000000').getValue(), + userUuid: Uuid.create(userUuid.value).getValue(), isDesignatedSurvivor: false, - }).getValue(), - ]) + }).getValue() + + if ([authenticatedUserUuid, sharedVaultOwnerContextUuid].includes(userUuid.value)) { + return [commonAssociation] + } + + return [] + }) }) it('should create a cross service token for user', async () => { @@ -358,7 +366,7 @@ describe('CreateCrossServiceToken', () => { await createUseCase().execute({ user, session, - sharedVaultOwnerContext: '00000000-0000-0000-0000-000000000000', + sharedVaultOwnerContext: sharedVaultOwnerContextUuid, }) expect(tokenEncoder.encodeExpirableToken).toHaveBeenCalledWith( @@ -396,7 +404,7 @@ describe('CreateCrossServiceToken', () => { const result = await createUseCase().execute({ user, session, - sharedVaultOwnerContext: '00000000-0000-0000-0000-000000000000', + sharedVaultOwnerContext: sharedVaultOwnerContextUuid, }) expect(result.isFailed()).toBeTruthy() @@ -411,11 +419,78 @@ describe('CreateCrossServiceToken', () => { const result = await createUseCase().execute({ user, session, - sharedVaultOwnerContext: '00000000-0000-0000-0000-000000000000', + sharedVaultOwnerContext: sharedVaultOwnerContextUuid, }) expect(result.isFailed()).toBeTruthy() }) + + it('should return an error if user does not belong to any shared vault with owner context user', async () => { + const regularSubscription = {} as jest.Mocked + getRegularSubscription.execute = jest.fn().mockReturnValue(Result.ok(regularSubscription)) + + sharedVaultUserRepository.findByUserUuid = jest.fn().mockImplementation((userUuid: Uuid) => { + const userSpecificSharedVaultUuid = + userUuid.value === authenticatedUserUuid + ? Uuid.create('00000000-0000-0000-0000-000000000000').getValue() + : Uuid.create('20000000-0000-0000-0000-000000000000').getValue() + + return [ + SharedVaultUser.create({ + permission: SharedVaultUserPermission.create('read').getValue(), + sharedVaultUuid: userSpecificSharedVaultUuid, + timestamps: Timestamps.create(123456789, 123456789).getValue(), + userUuid: Uuid.create(userUuid.value).getValue(), + isDesignatedSurvivor: false, + }).getValue(), + ] + }) + + const result = await createUseCase().execute({ + user, + session, + sharedVaultOwnerContext: sharedVaultOwnerContextUuid, + }) + + expect(result.isFailed()).toBeTruthy() + expect(getRegularSubscription.execute).not.toHaveBeenCalled() + }) + + it('should return an error if shared vault owner context is not a valid uuid', async () => { + const result = await createUseCase().execute({ + user, + session, + sharedVaultOwnerContext: 'invalid-uuid', + }) + + expect(result.isFailed()).toBeTruthy() + expect(result.getError()).toContain('Could not create cross service token with shared vault owner context') + expect(getRegularSubscription.execute).not.toHaveBeenCalled() + }) + + it('should skip shared vault membership validation if shared vault owner context matches the authenticated user', async () => { + const regularSubscription = {} as jest.Mocked + getRegularSubscription.execute = jest.fn().mockReturnValue(Result.ok(regularSubscription)) + + await createUseCase().execute({ + user, + session, + sharedVaultOwnerContext: authenticatedUserUuid, + }) + + expect(sharedVaultUserRepository.findByUserUuid).toHaveBeenCalledTimes(1) + expect(getRegularSubscription.execute).toHaveBeenCalledWith({ + userUuid: authenticatedUserUuid, + }) + expect(tokenEncoder.encodeExpirableToken).toHaveBeenCalledWith( + expect.objectContaining({ + shared_vault_owner_context: { + upload_bytes_limit: 100, + }, + }), + 60, + ) + }) }) describe('version determination', () => { diff --git a/packages/auth/src/Domain/UseCase/CreateCrossServiceToken/CreateCrossServiceToken.ts b/packages/auth/src/Domain/UseCase/CreateCrossServiceToken/CreateCrossServiceToken.ts index 4167d01ec..6bb930533 100644 --- a/packages/auth/src/Domain/UseCase/CreateCrossServiceToken/CreateCrossServiceToken.ts +++ b/packages/auth/src/Domain/UseCase/CreateCrossServiceToken/CreateCrossServiceToken.ts @@ -77,8 +77,34 @@ export class CreateCrossServiceToken implements UseCaseInterface { } if (dto.sharedVaultOwnerContext !== undefined) { + const sharedVaultOwnerContextUuidOrError = Uuid.create(dto.sharedVaultOwnerContext) + if (sharedVaultOwnerContextUuidOrError.isFailed()) { + return Result.fail( + `Could not create cross service token with shared vault owner context: ${sharedVaultOwnerContextUuidOrError.getError()}`, + ) + } + const sharedVaultOwnerContextUuid = sharedVaultOwnerContextUuidOrError.getValue() + + if (sharedVaultOwnerContextUuid.value !== user.uuid) { + const sharedVaultOwnerAssociations = await this.sharedVaultUserRepository.findByUserUuid( + sharedVaultOwnerContextUuid, + ) + const authenticatedUserSharedVaultUuids = new Set( + sharedVaultAssociations.map((association) => association.props.sharedVaultUuid.value), + ) + const authenticatedUserSharesVaultWithContextOwner = sharedVaultOwnerAssociations.some((association) => + authenticatedUserSharedVaultUuids.has(association.props.sharedVaultUuid.value), + ) + + if (!authenticatedUserSharesVaultWithContextOwner) { + return Result.fail( + `Could not create cross service token with shared vault owner context for user ${user.uuid}`, + ) + } + } + const regularSubscriptionOrError = await this.getRegularSubscription.execute({ - userUuid: dto.sharedVaultOwnerContext, + userUuid: sharedVaultOwnerContextUuid.value, }) if (regularSubscriptionOrError.isFailed()) { return Result.fail(regularSubscriptionOrError.getError()) From fd5ca2e10131fbe73f1fb588e130f63274a554e5 Mon Sep 17 00:00:00 2001 From: Antonella Sgarlatta Date: Wed, 6 May 2026 00:15:51 -0300 Subject: [PATCH 04/17] fix: add index to authenticators table --- ...8037105000-add-authenticators-user-uuid-index.ts | 13 +++++++++++++ ...8037105000-add-authenticators-user-uuid-index.ts | 13 +++++++++++++ .../auth/src/Infra/TypeORM/TypeORMAuthenticator.ts | 3 ++- 3 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 packages/auth/migrations/mysql/1778037105000-add-authenticators-user-uuid-index.ts create mode 100644 packages/auth/migrations/sqlite/1778037105000-add-authenticators-user-uuid-index.ts diff --git a/packages/auth/migrations/mysql/1778037105000-add-authenticators-user-uuid-index.ts b/packages/auth/migrations/mysql/1778037105000-add-authenticators-user-uuid-index.ts new file mode 100644 index 000000000..91e644572 --- /dev/null +++ b/packages/auth/migrations/mysql/1778037105000-add-authenticators-user-uuid-index.ts @@ -0,0 +1,13 @@ +import { MigrationInterface, QueryRunner } from 'typeorm' + +export class AddAuthenticatorsUserUuidIndex1778037105000 implements MigrationInterface { + name = 'AddAuthenticatorsUserUuidIndex1778037105000' + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query('CREATE INDEX `index_authenticators_on_user_uuid` ON `authenticators` (`user_uuid`)') + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query('DROP INDEX `index_authenticators_on_user_uuid` ON `authenticators`') + } +} diff --git a/packages/auth/migrations/sqlite/1778037105000-add-authenticators-user-uuid-index.ts b/packages/auth/migrations/sqlite/1778037105000-add-authenticators-user-uuid-index.ts new file mode 100644 index 000000000..0ef84f79b --- /dev/null +++ b/packages/auth/migrations/sqlite/1778037105000-add-authenticators-user-uuid-index.ts @@ -0,0 +1,13 @@ +import { MigrationInterface, QueryRunner } from 'typeorm' + +export class AddAuthenticatorsUserUuidIndex1778037105000 implements MigrationInterface { + name = 'AddAuthenticatorsUserUuidIndex1778037105000' + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query('CREATE INDEX "index_authenticators_on_user_uuid" ON "authenticators" ("user_uuid")') + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query('DROP INDEX "index_authenticators_on_user_uuid"') + } +} diff --git a/packages/auth/src/Infra/TypeORM/TypeORMAuthenticator.ts b/packages/auth/src/Infra/TypeORM/TypeORMAuthenticator.ts index ba8a03e3c..32a2f5ab6 100644 --- a/packages/auth/src/Infra/TypeORM/TypeORMAuthenticator.ts +++ b/packages/auth/src/Infra/TypeORM/TypeORMAuthenticator.ts @@ -1,4 +1,4 @@ -import { Column, Entity, PrimaryGeneratedColumn } from 'typeorm' +import { Column, Entity, Index, PrimaryGeneratedColumn } from 'typeorm' @Entity({ name: 'authenticators' }) export class TypeORMAuthenticator { @@ -9,6 +9,7 @@ export class TypeORMAuthenticator { name: 'user_uuid', length: 36, }) + @Index('index_authenticators_on_user_uuid') declare userUuid: string @Column({ From 075b9413d30cc9a77849719538bc5b80a66c0de9 Mon Sep 17 00:00:00 2001 From: Antonella Sgarlatta Date: Thu, 7 May 2026 16:33:09 -0300 Subject: [PATCH 05/17] fix: fix authenticator options response --- ...AuthenticatorAuthenticationOptions.spec.ts | 14 +++++++ ...erateAuthenticatorAuthenticationOptions.ts | 40 +++++++++++-------- 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/packages/auth/src/Domain/UseCase/GenerateAuthenticatorAuthenticationOptions/GenerateAuthenticatorAuthenticationOptions.spec.ts b/packages/auth/src/Domain/UseCase/GenerateAuthenticatorAuthenticationOptions/GenerateAuthenticatorAuthenticationOptions.spec.ts index e735f627b..243a42612 100644 --- a/packages/auth/src/Domain/UseCase/GenerateAuthenticatorAuthenticationOptions/GenerateAuthenticatorAuthenticationOptions.spec.ts +++ b/packages/auth/src/Domain/UseCase/GenerateAuthenticatorAuthenticationOptions/GenerateAuthenticatorAuthenticationOptions.spec.ts @@ -83,6 +83,20 @@ describe('GenerateAuthenticatorAuthenticationOptions', () => { }) expect(result.isFailed()).toBe(false) + expect(result.getValue().userVerification).toBe('preferred') + expect(authenticatorChallengeRepository.save).not.toHaveBeenCalled() + }) + + it('should return pseudo options if user does not have authenticators', async () => { + authenticatorRepository.findByUserUuid = jest.fn().mockReturnValue([]) + + const result = await createUseCase().execute({ + username: 'test@test.te', + }) + + expect(result.isFailed()).toBe(false) + expect(result.getValue().userVerification).toBe('preferred') + expect(authenticatorChallengeRepository.save).not.toHaveBeenCalled() }) it('should return error if authenticator challenge is invalid', async () => { diff --git a/packages/auth/src/Domain/UseCase/GenerateAuthenticatorAuthenticationOptions/GenerateAuthenticatorAuthenticationOptions.ts b/packages/auth/src/Domain/UseCase/GenerateAuthenticatorAuthenticationOptions/GenerateAuthenticatorAuthenticationOptions.ts index 0667dbcd3..c583f9b89 100644 --- a/packages/auth/src/Domain/UseCase/GenerateAuthenticatorAuthenticationOptions/GenerateAuthenticatorAuthenticationOptions.ts +++ b/packages/auth/src/Domain/UseCase/GenerateAuthenticatorAuthenticationOptions/GenerateAuthenticatorAuthenticationOptions.ts @@ -30,22 +30,7 @@ export class GenerateAuthenticatorAuthenticationOptions const user = await this.userRepository.findOneByUsernameOrEmail(username) if (user === null) { - const credentialIdHash = crypto - .createHash('sha256') - .update(`u2f-selector-${dto.username}${this.pseudoKeyParamsKey}`) - .digest('base64url') - - const options = await generateAuthenticationOptions({ - allowCredentials: [ - { - id: Buffer.from(credentialIdHash), - type: 'public-key', - transports: [], - }, - ], - userVerification: 'discouraged', - }) - + const options = await this.generatePseudoOptions(dto.username) return Result.ok(options) } @@ -56,6 +41,11 @@ export class GenerateAuthenticatorAuthenticationOptions const userUuid = userUuidOrError.getValue() const authenticators = await this.authenticatorRepository.findByUserUuid(userUuid) + if (authenticators.length === 0) { + const options = await this.generatePseudoOptions(dto.username) + return Result.ok(options) + } + const options = await generateAuthenticationOptions({ allowCredentials: authenticators.map((authenticator) => ({ id: authenticator.props.credentialId, @@ -81,4 +71,22 @@ export class GenerateAuthenticatorAuthenticationOptions return Result.ok(options) } + + private async generatePseudoOptions(username: string): Promise { + const credentialIdHash = crypto + .createHash('sha256') + .update(`u2f-selector-${username}${this.pseudoKeyParamsKey}`) + .digest('base64url') + + return generateAuthenticationOptions({ + allowCredentials: [ + { + id: Buffer.from(credentialIdHash), + type: 'public-key', + transports: [], + }, + ], + userVerification: 'preferred', + }) + } } From 08692cb3e550821fddc8dee1a87f6b6cee249fef Mon Sep 17 00:00:00 2001 From: Antonella Sgarlatta Date: Fri, 5 Jun 2026 13:15:39 +0000 Subject: [PATCH 06/17] fix: Prevents failed captchas from causing login lock --- packages/auth/src/Bootstrap/Container.ts | 39 ++-- .../auth/src/Domain/UseCase/SignIn.spec.ts | 155 ++++++++++++- packages/auth/src/Domain/UseCase/SignIn.ts | 62 ++--- .../auth/src/Domain/UseCase/SignInResponse.ts | 1 + .../SignInWithRecoveryCodes.spec.ts | 211 +++++++++++++++--- .../SignInWithRecoveryCodes.ts | 100 +++++---- .../SignInWithRecoveryCodesResponse.ts | 12 + .../AnnotatedAuthController.ts | 3 - .../Base/BaseAuthController.ts | 58 ++--- 9 files changed, 482 insertions(+), 159 deletions(-) create mode 100644 packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodesResponse.ts diff --git a/packages/auth/src/Bootstrap/Container.ts b/packages/auth/src/Bootstrap/Container.ts index a89008224..254325b48 100644 --- a/packages/auth/src/Bootstrap/Container.ts +++ b/packages/auth/src/Bootstrap/Container.ts @@ -1171,6 +1171,24 @@ export class ContainerConfigLoader { container.get(TYPES.Auth_CaptchaServer), ), ) + container + .bind(TYPES.Auth_ClearLoginAttempts) + .toConstantValue( + new ClearLoginAttempts( + container.get(TYPES.Auth_UserRepository), + container.get(TYPES.Auth_LockRepository), + container.get(TYPES.Auth_Logger), + ), + ) + container + .bind(TYPES.Auth_IncreaseLoginAttempts) + .toConstantValue( + new IncreaseLoginAttempts( + container.get(TYPES.Auth_UserRepository), + container.get(TYPES.Auth_LockRepository), + container.get(TYPES.Auth_MAX_LOGIN_ATTEMPTS), + ), + ) container .bind(TYPES.Auth_SignIn) .toConstantValue( @@ -1186,6 +1204,8 @@ export class ContainerConfigLoader { container.get(TYPES.Auth_MAX_LOGIN_ATTEMPTS), container.get(TYPES.Auth_LockRepository), container.get(TYPES.Auth_VerifyHumanInteraction), + container.get(TYPES.Auth_IncreaseLoginAttempts), + container.get(TYPES.Auth_ClearLoginAttempts), ), ) container @@ -1204,24 +1224,6 @@ export class ContainerConfigLoader { container.get(TYPES.Auth_Logger), ), ) - container - .bind(TYPES.Auth_ClearLoginAttempts) - .toConstantValue( - new ClearLoginAttempts( - container.get(TYPES.Auth_UserRepository), - container.get(TYPES.Auth_LockRepository), - container.get(TYPES.Auth_Logger), - ), - ) - container - .bind(TYPES.Auth_IncreaseLoginAttempts) - .toConstantValue( - new IncreaseLoginAttempts( - container.get(TYPES.Auth_UserRepository), - container.get(TYPES.Auth_LockRepository), - container.get(TYPES.Auth_MAX_LOGIN_ATTEMPTS), - ), - ) container .bind(TYPES.Auth_GetUserKeyParamsRecovery) .toConstantValue( @@ -1871,7 +1873,6 @@ export class ContainerConfigLoader { container.get(TYPES.Auth_SignIn), container.get(TYPES.Auth_GetUserKeyParams), container.get(TYPES.Auth_ClearLoginAttempts), - container.get(TYPES.Auth_IncreaseLoginAttempts), container.get(TYPES.Auth_Logger), container.get(TYPES.Auth_AuthController), container.get(TYPES.Auth_Register), diff --git a/packages/auth/src/Domain/UseCase/SignIn.spec.ts b/packages/auth/src/Domain/UseCase/SignIn.spec.ts index b7519dac4..43b8e23b4 100644 --- a/packages/auth/src/Domain/UseCase/SignIn.spec.ts +++ b/packages/auth/src/Domain/UseCase/SignIn.spec.ts @@ -17,6 +17,8 @@ import { Session } from '../Session/Session' import { LockRepositoryInterface } from '../User/LockRepositoryInterface' import { VerifyHumanInteraction } from './VerifyHumanInteraction/VerifyHumanInteraction' import { Result } from '@standardnotes/domain-core' +import { IncreaseLoginAttempts } from './IncreaseLoginAttempts' +import { ClearLoginAttempts } from './ClearLoginAttempts' describe('SignIn', () => { let user: User @@ -33,7 +35,8 @@ describe('SignIn', () => { let maxNonCaptchaAttempts: number let lockRepository: LockRepositoryInterface let verifyHumanInteractionUseCase: VerifyHumanInteraction - + let increaseLoginAttempts: IncreaseLoginAttempts + let clearLoginAttempts: ClearLoginAttempts const createUseCase = () => new SignIn( userRepository, @@ -47,6 +50,8 @@ describe('SignIn', () => { maxNonCaptchaAttempts, lockRepository, verifyHumanInteractionUseCase, + increaseLoginAttempts, + clearLoginAttempts, ) beforeEach(() => { @@ -92,8 +97,22 @@ describe('SignIn', () => { lockRepository.getLockCounter = jest.fn().mockReturnValue(0) maxNonCaptchaAttempts = 6 + + increaseLoginAttempts = {} as jest.Mocked + increaseLoginAttempts.execute = jest.fn().mockReturnValue(Result.ok({ isNonCaptchaLimitReached: false })) + + clearLoginAttempts = {} as jest.Mocked + clearLoginAttempts.execute = jest.fn() + + verifyHumanInteractionUseCase = {} as jest.Mocked + verifyHumanInteractionUseCase.execute = jest.fn().mockReturnValue(Result.ok()) }) + const requireHumanVerification = () => { + lockRepository.getLockCounter = jest.fn().mockReturnValueOnce(maxNonCaptchaAttempts).mockReturnValueOnce(0) + verifyHumanInteractionUseCase.execute = jest.fn() + } + it('should fail sign in a legacy user without code verifier', async () => { pkceRepository.removeCodeChallenge = jest.fn().mockReturnValue(false) @@ -113,6 +132,7 @@ describe('SignIn', () => { success: false, errorCode: 410, errorMessage: 'Please update your client application.', + isNonCaptchaLimitReached: false, }) }) @@ -132,6 +152,7 @@ describe('SignIn', () => { success: false, errorCode: 410, errorMessage: 'Please update your client application.', + isNonCaptchaLimitReached: false, }) }) @@ -157,6 +178,7 @@ describe('SignIn', () => { success: false, errorCode: 410, errorMessage: 'Please update your client application.', + isNonCaptchaLimitReached: false, }) }) @@ -173,6 +195,7 @@ describe('SignIn', () => { ).toEqual({ success: false, errorMessage: 'Username cannot be empty', + isNonCaptchaLimitReached: false, }) expect(domainEventFactory.createEmailRequestedEvent).not.toHaveBeenCalled() @@ -192,6 +215,7 @@ describe('SignIn', () => { ).toEqual({ success: false, errorMessage: 'Invalid api version: invalid', + isNonCaptchaLimitReached: false, }) expect(domainEventFactory.createEmailRequestedEvent).not.toHaveBeenCalled() @@ -218,6 +242,7 @@ describe('SignIn', () => { expect(domainEventFactory.createEmailRequestedEvent).toHaveBeenCalled() expect(domainEventPublisher.publish).toHaveBeenCalled() + expect(clearLoginAttempts.execute).toHaveBeenCalledWith({ email: 'test@test.te' }) }) it('should sign in a user even if publishing a sign in event fails', async () => { @@ -256,6 +281,7 @@ describe('SignIn', () => { ).toEqual({ success: false, errorMessage: 'Invalid email or password', + isNonCaptchaLimitReached: false, }) }) @@ -274,6 +300,7 @@ describe('SignIn', () => { ).toEqual({ success: false, errorMessage: 'Invalid email or password', + isNonCaptchaLimitReached: false, }) }) @@ -292,6 +319,7 @@ describe('SignIn', () => { ).toEqual({ success: false, errorMessage: 'Invalid email or password', + isNonCaptchaLimitReached: false, }) }) @@ -363,6 +391,131 @@ describe('SignIn', () => { ).toEqual({ success: false, errorMessage: 'Human verification step failed.', + isNonCaptchaLimitReached: true, }) }) + + it('should return isNonCaptchaLimitReached when incrementing login attempts reaches the limit', async () => { + increaseLoginAttempts.execute = jest.fn().mockReturnValue(Result.ok({ isNonCaptchaLimitReached: true })) + + expect( + await createUseCase().execute({ + email: 'test@test.te', + password: 'asdasd123123', + userAgent: 'Google Chrome', + apiVersion: '20190520', + ephemeralSession: false, + codeVerifier: 'test', + }), + ).toEqual({ + success: false, + errorMessage: 'Invalid email or password', + isNonCaptchaLimitReached: true, + }) + }) + + it('should increment login attempts on invalid password', async () => { + await createUseCase().execute({ + email: 'test@test.te', + password: 'asdasd123123', + userAgent: 'Google Chrome', + apiVersion: '20190520', + ephemeralSession: false, + codeVerifier: 'test', + }) + + expect(increaseLoginAttempts.execute).toHaveBeenCalledWith({ + email: 'test@test.te', + skipUsernameValidation: true, + }) + }) + + it('should not increment login attempts when human verification fails', async () => { + requireHumanVerification() + verifyHumanInteractionUseCase.execute = jest + .fn() + .mockReturnValueOnce(Result.fail('Human verification step failed.')) + + await createUseCase().execute({ + email: 'test@test.te', + password: 'qweqwe123123', + userAgent: 'Google Chrome', + apiVersion: '20190520', + ephemeralSession: false, + codeVerifier: 'test', + hvmToken: 'bad-token', + }) + + expect(increaseLoginAttempts.execute).not.toHaveBeenCalled() + }) + + it('should not increment login attempts when human verification token is missing', async () => { + requireHumanVerification() + verifyHumanInteractionUseCase.execute = jest.fn().mockReturnValueOnce(Result.fail('No HVM token available.')) + + await createUseCase().execute({ + email: 'test@test.te', + password: 'qweqwe123123', + userAgent: 'Google Chrome', + apiVersion: '20190520', + ephemeralSession: false, + codeVerifier: 'test', + }) + + expect(increaseLoginAttempts.execute).not.toHaveBeenCalled() + }) + + it('should not set isNonCaptchaLimitReached when increasing login attempts fails', async () => { + increaseLoginAttempts.execute = jest.fn().mockReturnValue(Result.fail('invalid email')) + + expect( + await createUseCase().execute({ + email: 'test@test.te', + password: 'asdasd123123', + userAgent: 'Google Chrome', + apiVersion: '20190520', + ephemeralSession: false, + codeVerifier: 'test', + }), + ).toEqual({ + success: false, + errorMessage: 'Invalid email or password', + }) + }) + + it('should require human verification in captcha mode and not increment on failure', async () => { + lockRepository.getLockCounter = jest.fn().mockReturnValueOnce(0).mockReturnValueOnce(1) + verifyHumanInteractionUseCase.execute = jest + .fn() + .mockReturnValueOnce(Result.fail('Human verification step failed.')) + + await createUseCase().execute({ + email: 'test@test.te', + password: 'qweqwe123123', + userAgent: 'Google Chrome', + apiVersion: '20190520', + ephemeralSession: false, + codeVerifier: 'test', + hvmToken: 'bad-token', + }) + + expect(increaseLoginAttempts.execute).not.toHaveBeenCalled() + }) + + it('should not increment login attempts on human verification failure for unknown user', async () => { + userRepository.findOneByUsernameOrEmail = jest.fn().mockReturnValue(null) + lockRepository.getLockCounter = jest.fn().mockReturnValueOnce(maxNonCaptchaAttempts).mockReturnValueOnce(0) + verifyHumanInteractionUseCase.execute = jest.fn().mockReturnValueOnce(Result.fail('No HVM token available.')) + + await createUseCase().execute({ + email: 'test@test.te', + password: 'asdasd123123', + userAgent: 'Google Chrome', + apiVersion: '20190520', + ephemeralSession: false, + codeVerifier: 'test', + }) + + expect(increaseLoginAttempts.execute).not.toHaveBeenCalled() + }) }) diff --git a/packages/auth/src/Domain/UseCase/SignIn.ts b/packages/auth/src/Domain/UseCase/SignIn.ts index 1d7ab1fa5..31a8e7ac8 100644 --- a/packages/auth/src/Domain/UseCase/SignIn.ts +++ b/packages/auth/src/Domain/UseCase/SignIn.ts @@ -18,6 +18,8 @@ import { ApiVersion } from '../Api/ApiVersion' import { HttpStatusCode } from '@standardnotes/responses' import { VerifyHumanInteraction } from './VerifyHumanInteraction/VerifyHumanInteraction' import { LockRepositoryInterface } from '../User/LockRepositoryInterface' +import { IncreaseLoginAttempts } from './IncreaseLoginAttempts' +import { ClearLoginAttempts } from './ClearLoginAttempts' export class SignIn implements UseCaseInterface { constructor( @@ -32,43 +34,36 @@ export class SignIn implements UseCaseInterface { private maxNonCaptchaAttempts: number, private lockRepository: LockRepositoryInterface, private verifyHumanInteractionUseCase: VerifyHumanInteraction, + private increaseLoginAttempts: IncreaseLoginAttempts, + private clearLoginAttempts: ClearLoginAttempts, ) {} async execute(dto: SignInDTO): Promise { if (!dto.codeVerifier) { - return { - success: false, - errorMessage: 'Please update your client application.', - errorCode: HttpStatusCode.Gone, - } + return this.failAfterIncrementingLoginAttempts( + dto.email, + 'Please update your client application.', + HttpStatusCode.Gone, + ) } const validCodeVerifier = await this.validateCodeVerifier(dto.codeVerifier) if (!validCodeVerifier) { this.logger.debug('Code verifier does not match') - return { - success: false, - errorMessage: 'Invalid email or password', - } + return this.failAfterIncrementingLoginAttempts(dto.email, 'Invalid email or password') } const apiVersionOrError = ApiVersion.create(dto.apiVersion) if (apiVersionOrError.isFailed()) { - return { - success: false, - errorMessage: apiVersionOrError.getError(), - } + return this.failAfterIncrementingLoginAttempts(dto.email, apiVersionOrError.getError()) } const apiVersion = apiVersionOrError.getValue() /** Skip validation which was newly added in 2025, to allow existing users to continue to sign in */ const usernameOrError = Username.create(dto.email, { skipValidation: true }) if (usernameOrError.isFailed()) { - return { - success: false, - errorMessage: usernameOrError.getError(), - } + return this.failAfterIncrementingLoginAttempts(dto.email, usernameOrError.getError()) } const username = usernameOrError.getValue() @@ -83,26 +78,21 @@ export class SignIn implements UseCaseInterface { return { success: false, errorMessage: humanVerificationBeforeCheckingUsernameAndPasswordResult.getError(), + isNonCaptchaLimitReached: true, } } if (!user) { this.logger.debug(`User with email ${dto.email} was not found`) - return { - success: false, - errorMessage: 'Invalid email or password', - } + 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') - return { - success: false, - errorMessage: 'Invalid email or password', - } + return this.failAfterIncrementingLoginAttempts(dto.email, 'Invalid email or password') } const authResponseFactory = this.authResponseFactoryResolver.resolveAuthResponseFactoryVersion(apiVersion) @@ -119,12 +109,34 @@ export class SignIn implements UseCaseInterface { application: dto.application, }) + await this.clearLoginAttempts.execute({ email: dto.email }) + return { success: true, result, } } + private async failAfterIncrementingLoginAttempts( + email: string, + errorMessage: string, + errorCode?: HttpStatusCode, + ): Promise { + const increaseResultOrError = await this.increaseLoginAttempts.execute({ + email, + skipUsernameValidation: true, + }) + + return { + success: false, + errorMessage, + errorCode, + isNonCaptchaLimitReached: increaseResultOrError.isFailed() + ? undefined + : increaseResultOrError.getValue().isNonCaptchaLimitReached, + } + } + private async validateCodeVerifier(codeVerifier: string): Promise { const codeChallenge = this.crypter.base64URLEncode(this.crypter.sha256Hash(codeVerifier)) diff --git a/packages/auth/src/Domain/UseCase/SignInResponse.ts b/packages/auth/src/Domain/UseCase/SignInResponse.ts index 43576493f..8cd5434d5 100644 --- a/packages/auth/src/Domain/UseCase/SignInResponse.ts +++ b/packages/auth/src/Domain/UseCase/SignInResponse.ts @@ -7,6 +7,7 @@ export type SignInResponse = success: false errorMessage: string errorCode?: HttpStatusCode + isNonCaptchaLimitReached?: boolean } | { success: true diff --git a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.spec.ts b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.spec.ts index dd3f3b702..7fc7e64bf 100644 --- a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.spec.ts +++ b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.spec.ts @@ -1,6 +1,5 @@ import { Result } from '@standardnotes/domain-core' -import { AuthResponse20200115 } from '../../Auth/AuthResponse20200115' import { AuthResponseFactory20200115 } from '../../Auth/AuthResponseFactory20200115' import { AuthenticatorRepositoryInterface } from '../../Authenticator/AuthenticatorRepositoryInterface' import { CrypterInterface } from '../../Encryption/CrypterInterface' @@ -58,7 +57,7 @@ describe('SignInWithRecoveryCodes', () => { } as jest.Mocked) authResponseFactory = {} as jest.Mocked - authResponseFactory.createResponse = jest.fn().mockReturnValue({} as jest.Mocked) + authResponseFactory.createResponse = jest.fn().mockReturnValue({ response: { foo: 'bar' }, session: {} }) pkceRepository = {} as jest.Mocked pkceRepository.removeCodeChallenge = jest.fn().mockReturnValue(true) @@ -76,7 +75,7 @@ describe('SignInWithRecoveryCodes', () => { generateRecoveryCodes.execute = jest.fn().mockReturnValue(Result.ok('1234 5678')) increaseLoginAttempts = {} as jest.Mocked - increaseLoginAttempts.execute = jest.fn() + increaseLoginAttempts.execute = jest.fn().mockReturnValue(Result.ok({ isNonCaptchaLimitReached: false })) clearLoginAttempts = {} as jest.Mocked clearLoginAttempts.execute = jest.fn() @@ -91,8 +90,16 @@ describe('SignInWithRecoveryCodes', () => { lockRepository.getLockCounter = jest.fn().mockReturnValue(0) maxNonCaptchaAttempts = 6 + + verifyHumanInteractionUseCase = {} as jest.Mocked + verifyHumanInteractionUseCase.execute = jest.fn().mockReturnValue(Result.ok()) }) + const requireHumanVerification = () => { + lockRepository.getLockCounter = jest.fn().mockReturnValueOnce(maxNonCaptchaAttempts).mockReturnValueOnce(0) + verifyHumanInteractionUseCase.execute = jest.fn() + } + it('should return error if password is not provided', async () => { const result = await createUseCase().execute({ apiVersion: ApiVersion.VERSIONS.v20200115, @@ -103,8 +110,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Empty password') + expect(result).toEqual({ + success: false, + errorMessage: 'Empty password', + isNonCaptchaLimitReached: false, + }) }) it('should return error if username is not provided', async () => { @@ -117,8 +127,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Could not sign in with recovery codes: Username cannot be empty') + expect(result).toEqual({ + success: false, + errorMessage: 'Could not sign in with recovery codes: Username cannot be empty', + isNonCaptchaLimitReached: false, + }) }) it('should return error if code verifier is not provided', async () => { @@ -131,8 +144,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Invalid code verifier') + expect(result).toEqual({ + success: false, + errorMessage: 'Invalid code verifier', + isNonCaptchaLimitReached: false, + }) }) it('should return error if recovery codes are not provided', async () => { @@ -145,8 +161,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Empty recovery codes') + expect(result).toEqual({ + success: false, + errorMessage: 'Empty recovery codes', + isNonCaptchaLimitReached: false, + }) }) it('should return error if code verifier is invalid', async () => { @@ -161,8 +180,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Invalid code verifier') + expect(result).toEqual({ + success: false, + errorMessage: 'Invalid code verifier', + isNonCaptchaLimitReached: false, + }) }) it('should return error if user is not found', async () => { @@ -177,8 +199,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Could not find user') + expect(result).toEqual({ + success: false, + errorMessage: 'Could not find user', + isNonCaptchaLimitReached: false, + }) }) it('should return error if recovery codes are invalid', async () => { @@ -191,8 +216,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Invalid recovery codes') + expect(result).toEqual({ + success: false, + errorMessage: 'Invalid recovery codes', + isNonCaptchaLimitReached: false, + }) }) it('should return error if api version is invalid', async () => { @@ -205,7 +233,15 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) + expect(result).toEqual({ + success: false, + errorMessage: 'Invalid api version: invalid', + isNonCaptchaLimitReached: false, + }) + expect(increaseLoginAttempts.execute).toHaveBeenCalledWith({ + email: 'test@test.te', + skipUsernameValidation: true, + }) }) it('should return error if api version does not support recovery sign in', async () => { @@ -218,7 +254,15 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) + expect(result).toEqual({ + success: false, + errorMessage: 'Unsupported api version', + isNonCaptchaLimitReached: false, + }) + expect(increaseLoginAttempts.execute).toHaveBeenCalledWith({ + email: 'test@test.te', + skipUsernameValidation: true, + }) }) it('should return error if password does not match', async () => { @@ -231,8 +275,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Invalid password') + expect(result).toEqual({ + success: false, + errorMessage: 'Invalid password', + isNonCaptchaLimitReached: false, + }) }) it('should return error if recovery codes are not generated for user', async () => { @@ -247,8 +294,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: '1234 5678', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('User does not have recovery codes generated') + expect(result).toEqual({ + success: false, + errorMessage: 'User does not have recovery codes generated', + isNonCaptchaLimitReached: false, + }) }) it('should return error if generating new recovery codes fails', async () => { @@ -263,8 +313,12 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: 'foo', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Could not sign in with recovery codes: Oops') + expect(result).toEqual({ + success: false, + errorMessage: 'Could not sign in with recovery codes: Oops', + isNonCaptchaLimitReached: false, + }) + expect(authResponseFactory.createResponse).not.toHaveBeenCalled() }) it('should return error if user has an invalid uuid', async () => { @@ -282,8 +336,11 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: 'foo', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Invalid user uuid') + expect(result).toEqual({ + success: false, + errorMessage: 'Invalid user uuid', + isNonCaptchaLimitReached: false, + }) }) it('should return error if user requires human verification but no hvmtoken provided', async () => { @@ -302,8 +359,100 @@ describe('SignInWithRecoveryCodes', () => { recoveryCodes: 'foo', }) - expect(result.isFailed()).toBe(true) - expect(result.getError()).toBe('Human verification step failed.') + expect(result).toEqual({ + success: false, + errorMessage: 'Human verification step failed.', + isNonCaptchaLimitReached: true, + }) + }) + + it('should return isNonCaptchaLimitReached when incrementing login attempts reaches the limit', async () => { + increaseLoginAttempts.execute = jest.fn().mockReturnValue(Result.ok({ isNonCaptchaLimitReached: true })) + + const result = await createUseCase().execute({ + apiVersion: ApiVersion.VERSIONS.v20200115, + userAgent: 'user-agent', + username: 'test@test.te', + password: 'asdasd123123', + codeVerifier: 'code-verifier', + recoveryCodes: '1234 5678', + }) + + expect(result).toEqual({ + success: false, + errorMessage: 'Invalid password', + isNonCaptchaLimitReached: true, + }) + }) + + it('should increment login attempts once on invalid password', async () => { + await createUseCase().execute({ + apiVersion: ApiVersion.VERSIONS.v20200115, + userAgent: 'user-agent', + username: 'test@test.te', + password: 'asdasd123123', + codeVerifier: 'code-verifier', + recoveryCodes: '1234 5678', + }) + + expect(increaseLoginAttempts.execute).toHaveBeenCalledTimes(1) + expect(increaseLoginAttempts.execute).toHaveBeenCalledWith({ + email: 'test@test.te', + skipUsernameValidation: true, + }) + }) + + it('should not increment login attempts when human verification fails', async () => { + requireHumanVerification() + verifyHumanInteractionUseCase.execute = jest + .fn() + .mockReturnValueOnce(Result.fail('Human verification step failed.')) + + await createUseCase().execute({ + apiVersion: ApiVersion.VERSIONS.v20200115, + userAgent: 'user-agent', + username: 'test@test.te', + password: 'qweqwe123123', + codeVerifier: 'code-verifier', + recoveryCodes: 'foo', + hvmToken: 'bad-token', + }) + + expect(increaseLoginAttempts.execute).not.toHaveBeenCalled() + }) + + it('should not increment login attempts when human verification token is missing', async () => { + requireHumanVerification() + verifyHumanInteractionUseCase.execute = jest.fn().mockReturnValueOnce(Result.fail('No HVM token available.')) + + await createUseCase().execute({ + apiVersion: ApiVersion.VERSIONS.v20200115, + userAgent: 'user-agent', + username: 'test@test.te', + password: 'qweqwe123123', + codeVerifier: 'code-verifier', + recoveryCodes: 'foo', + }) + + expect(increaseLoginAttempts.execute).not.toHaveBeenCalled() + }) + + it('should not set isNonCaptchaLimitReached when increasing login attempts fails', async () => { + increaseLoginAttempts.execute = jest.fn().mockReturnValue(Result.fail('invalid email')) + + const result = await createUseCase().execute({ + apiVersion: ApiVersion.VERSIONS.v20200115, + userAgent: 'user-agent', + username: 'test@test.te', + password: 'asdasd123123', + codeVerifier: 'code-verifier', + recoveryCodes: '1234 5678', + }) + + expect(result).toEqual({ + success: false, + errorMessage: 'Invalid password', + }) }) it('should return auth response with human verification required and passing', async () => { @@ -326,7 +475,7 @@ describe('SignInWithRecoveryCodes', () => { userUuid: '00000000-0000-0000-0000-000000000000', }) expect(authenticatorRepository.removeByUserUuid).toHaveBeenCalled() - expect(result.isFailed()).toBe(false) + expect(result.success).toBe(true) }) it('should return auth response', async () => { @@ -345,6 +494,6 @@ describe('SignInWithRecoveryCodes', () => { userUuid: '00000000-0000-0000-0000-000000000000', }) expect(authenticatorRepository.removeByUserUuid).toHaveBeenCalled() - expect(result.isFailed()).toBe(false) + expect(result.success).toBe(true) }) }) diff --git a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.ts b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.ts index bf7e829fe..5e62c2931 100644 --- a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.ts +++ b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.ts @@ -1,13 +1,13 @@ import * as bcrypt from 'bcryptjs' -import { Result, SettingName, UseCaseInterface, Username, Uuid, Validator } from '@standardnotes/domain-core' +import { Result, SettingName, Username, Uuid, Validator } from '@standardnotes/domain-core' -import { AuthResponse20200115 } from '../../Auth/AuthResponse20200115' import { CrypterInterface } from '../../Encryption/CrypterInterface' import { PKCERepositoryInterface } from '../../User/PKCERepositoryInterface' import { UserRepositoryInterface } from '../../User/UserRepositoryInterface' import { GenerateRecoveryCodes } from '../GenerateRecoveryCodes/GenerateRecoveryCodes' import { SignInWithRecoveryCodesDTO } from './SignInWithRecoveryCodesDTO' +import { SignInWithRecoveryCodesResponse } from './SignInWithRecoveryCodesResponse' import { AuthResponseFactory20200115 } from '../../Auth/AuthResponseFactory20200115' import { IncreaseLoginAttempts } from '../IncreaseLoginAttempts' import { ClearLoginAttempts } from '../ClearLoginAttempts' @@ -17,8 +17,9 @@ import { ApiVersion } from '../../Api/ApiVersion' import { GetSetting } from '../GetSetting/GetSetting' import { LockRepositoryInterface } from '../../User/LockRepositoryInterface' import { VerifyHumanInteraction } from '../VerifyHumanInteraction/VerifyHumanInteraction' +import { UseCaseInterface } from '../UseCaseInterface' -export class SignInWithRecoveryCodes implements UseCaseInterface { +export class SignInWithRecoveryCodes implements UseCaseInterface { constructor( private userRepository: UserRepositoryInterface, private authResponseFactory: AuthResponseFactory20200115, @@ -35,20 +36,23 @@ export class SignInWithRecoveryCodes implements UseCaseInterface> { + async execute(dto: SignInWithRecoveryCodesDTO): Promise { const apiVersionOrError = ApiVersion.create(dto.apiVersion) if (apiVersionOrError.isFailed()) { - return Result.fail(apiVersionOrError.getError()) + return this.failAfterIncrementingLoginAttempts(dto.username, apiVersionOrError.getError()) } const apiVersion = apiVersionOrError.getValue() if (!apiVersion.isSupportedForRecoverySignIn()) { - return Result.fail('Unsupported api version') + return this.failAfterIncrementingLoginAttempts(dto.username, 'Unsupported api version') } const usernameOrError = Username.create(dto.username) if (usernameOrError.isFailed()) { - return Result.fail(`Could not sign in with recovery codes: ${usernameOrError.getError()}`) + return this.failAfterIncrementingLoginAttempts( + dto.username, + `Could not sign in with recovery codes: ${usernameOrError.getError()}`, + ) } const username = usernameOrError.getValue() @@ -60,49 +64,41 @@ export class SignInWithRecoveryCodes implements UseCaseInterface { + const increaseResultOrError = await this.increaseLoginAttempts.execute({ + email, + skipUsernameValidation: true, + }) + + return { + success: false, + errorMessage, + isNonCaptchaLimitReached: increaseResultOrError.isFailed() + ? undefined + : increaseResultOrError.getValue().isNonCaptchaLimitReached, + } } private async validateCodeVerifier(codeVerifier: string): Promise { diff --git a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodesResponse.ts b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodesResponse.ts new file mode 100644 index 000000000..d573330c1 --- /dev/null +++ b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodesResponse.ts @@ -0,0 +1,12 @@ +import { AuthResponseCreationResult } from '../../Auth/AuthResponseCreationResult' + +export type SignInWithRecoveryCodesResponse = + | { + success: false + errorMessage: string + isNonCaptchaLimitReached?: boolean + } + | { + success: true + result: AuthResponseCreationResult + } diff --git a/packages/auth/src/Infra/InversifyExpressUtils/AnnotatedAuthController.ts b/packages/auth/src/Infra/InversifyExpressUtils/AnnotatedAuthController.ts index fa6eaadc6..c474851af 100644 --- a/packages/auth/src/Infra/InversifyExpressUtils/AnnotatedAuthController.ts +++ b/packages/auth/src/Infra/InversifyExpressUtils/AnnotatedAuthController.ts @@ -10,7 +10,6 @@ import TYPES from '../../Bootstrap/Types' import { SignIn } from '../../Domain/UseCase/SignIn' import { ClearLoginAttempts } from '../../Domain/UseCase/ClearLoginAttempts' import { VerifyMFA } from '../../Domain/UseCase/VerifyMFA' -import { IncreaseLoginAttempts } from '../../Domain/UseCase/IncreaseLoginAttempts' import { Logger } from 'winston' import { GetUserKeyParams } from '../../Domain/UseCase/GetUserKeyParams/GetUserKeyParams' import { AuthController } from '../../Controller/AuthController' @@ -32,7 +31,6 @@ export class AnnotatedAuthController extends BaseAuthController { @inject(TYPES.Auth_SignIn) override signInUseCase: SignIn, @inject(TYPES.Auth_GetUserKeyParams) override getUserKeyParams: GetUserKeyParams, @inject(TYPES.Auth_ClearLoginAttempts) override clearLoginAttempts: ClearLoginAttempts, - @inject(TYPES.Auth_IncreaseLoginAttempts) override increaseLoginAttempts: IncreaseLoginAttempts, @inject(TYPES.Auth_Logger) override logger: Logger, @inject(TYPES.Auth_AuthController) override authController: AuthController, @inject(TYPES.Auth_Register) override registerUser: Register, @@ -50,7 +48,6 @@ export class AnnotatedAuthController extends BaseAuthController { signInUseCase, getUserKeyParams, clearLoginAttempts, - increaseLoginAttempts, logger, authController, registerUser, diff --git a/packages/auth/src/Infra/InversifyExpressUtils/Base/BaseAuthController.ts b/packages/auth/src/Infra/InversifyExpressUtils/Base/BaseAuthController.ts index 76727e9bf..5cb1c7e85 100644 --- a/packages/auth/src/Infra/InversifyExpressUtils/Base/BaseAuthController.ts +++ b/packages/auth/src/Infra/InversifyExpressUtils/Base/BaseAuthController.ts @@ -4,7 +4,6 @@ import { Logger } from 'winston' import { ClearLoginAttempts } from '../../../Domain/UseCase/ClearLoginAttempts' import { GetUserKeyParams } from '../../../Domain/UseCase/GetUserKeyParams/GetUserKeyParams' -import { IncreaseLoginAttempts } from '../../../Domain/UseCase/IncreaseLoginAttempts' import { SignIn } from '../../../Domain/UseCase/SignIn' import { VerifyMFA } from '../../../Domain/UseCase/VerifyMFA' import { AuthController } from '../../../Controller/AuthController' @@ -29,7 +28,6 @@ export class BaseAuthController extends BaseHttpController { protected signInUseCase: SignIn, protected getUserKeyParams: GetUserKeyParams, protected clearLoginAttempts: ClearLoginAttempts, - protected increaseLoginAttempts: IncreaseLoginAttempts, protected logger: Logger, protected authController: AuthController, protected registerUser: Register, @@ -147,16 +145,8 @@ export class BaseAuthController extends BaseHttpController { }) if (!signInResult.success) { - const resultOrError = await this.increaseLoginAttempts.execute({ email: request.body.email, skipUsernameValidation: true }) - if (resultOrError.isFailed()) { - this.logger.error(`Failed to increase login attempts: ${resultOrError.getError()}`, { - application: request.headers['x-application-version'] as string, - }) - } else { - const result = resultOrError.getValue() - if (result.isNonCaptchaLimitReached) { - response.setHeader('x-captcha-required', this.captchaUIUrl) - } + if (signInResult.isNonCaptchaLimitReached) { + response.setHeader('x-captcha-required', this.captchaUIUrl) } return this.json( @@ -169,8 +159,6 @@ export class BaseAuthController extends BaseHttpController { ) } - await this.clearLoginAttempts.execute({ email: request.body.email }) - if (signInResult.result.response !== undefined) { const session = signInResult.result.session as Session const user = signInResult.result.response.user @@ -208,7 +196,7 @@ export class BaseAuthController extends BaseHttpController { } async recoveryLogin(request: Request, response: Response): Promise { - const result = await this.signInWithRecoveryCodes.execute({ + const signInResponse = await this.signInWithRecoveryCodes.execute({ apiVersion: request.body.api_version, userAgent: request.headers['user-agent'], codeVerifier: request.body.code_verifier, @@ -220,24 +208,11 @@ export class BaseAuthController extends BaseHttpController { application: request.headers['x-application-version'] as string, }) - if (result.isFailed()) { - this.logger.debug(`Failed to sign in with recovery codes: ${result.getError()}`) + if (!signInResponse.success) { + this.logger.debug(`Failed to sign in with recovery codes: ${signInResponse.errorMessage}`) - const increasLoginAttemtpsResultOrError = await this.increaseLoginAttempts.execute({ - email: request.body.username, - }) - if (increasLoginAttemtpsResultOrError.isFailed()) { - this.logger.error( - `Failed to increase login attempts on recovery login: ${increasLoginAttemtpsResultOrError.getError()}`, - { - application: request.headers['x-application-version'] as string, - }, - ) - } else { - const increasLoginAttemtpsResult = increasLoginAttemtpsResultOrError.getValue() - if (increasLoginAttemtpsResult.isNonCaptchaLimitReached) { - response.setHeader('x-captcha-required', this.captchaUIUrl) - } + if (signInResponse.isNonCaptchaLimitReached) { + response.setHeader('x-captcha-required', this.captchaUIUrl) } return this.json( @@ -250,14 +225,23 @@ export class BaseAuthController extends BaseHttpController { ) } - await this.clearLoginAttempts.execute({ email: request.body.username }) + const authResponse = signInResponse.result.response - const signInWithRecoveryCodesResult = result.getValue() + if (authResponse === undefined) { + return this.json( + { + error: { + message: 'Invalid login credentials.', + }, + }, + HttpStatusCode.Unauthorized, + ) + } return this.json({ - session: signInWithRecoveryCodesResult.sessionBody, - key_params: signInWithRecoveryCodesResult.keyParams, - user: signInWithRecoveryCodesResult.user, + session: authResponse.sessionBody, + key_params: authResponse.keyParams, + user: authResponse.user, }) } From f80b17baff65bb0eb85247890ef5e993c712a103 Mon Sep 17 00:00:00 2001 From: Antonella Sgarlatta Date: Fri, 5 Jun 2026 12:57:45 -0300 Subject: [PATCH 07/17] chore: add clear login attempts admin endpoint --- packages/auth/src/Bootstrap/Container.ts | 1 + .../AnnotatedAdminController.ts | 18 +++++++++- .../Base/BaseAdminController.ts | 33 +++++++++++++++++++ 3 files changed, 51 insertions(+), 1 deletion(-) diff --git a/packages/auth/src/Bootstrap/Container.ts b/packages/auth/src/Bootstrap/Container.ts index 254325b48..738222724 100644 --- a/packages/auth/src/Bootstrap/Container.ts +++ b/packages/auth/src/Bootstrap/Container.ts @@ -1964,6 +1964,7 @@ export class ContainerConfigLoader { container.get(TYPES.Auth_UserRepository), container.get(TYPES.Auth_CreateSubscriptionToken), container.get(TYPES.Auth_CreateOfflineSubscriptionToken), + container.get(TYPES.Auth_ClearLoginAttempts), container.get(TYPES.Auth_ControllerContainer), ), ) diff --git a/packages/auth/src/Infra/InversifyExpressUtils/AnnotatedAdminController.ts b/packages/auth/src/Infra/InversifyExpressUtils/AnnotatedAdminController.ts index 7e4bf6591..d1faf7f9d 100644 --- a/packages/auth/src/Infra/InversifyExpressUtils/AnnotatedAdminController.ts +++ b/packages/auth/src/Infra/InversifyExpressUtils/AnnotatedAdminController.ts @@ -12,6 +12,7 @@ import TYPES from '../../Bootstrap/Types' import { BaseAdminController } from './Base/BaseAdminController' import { CreateOfflineSubscriptionToken } from '../../Domain/UseCase/CreateOfflineSubscriptionToken/CreateOfflineSubscriptionToken' import { CreateSubscriptionToken } from '../../Domain/UseCase/CreateSubscriptionToken/CreateSubscriptionToken' +import { ClearLoginAttempts } from '../../Domain/UseCase/ClearLoginAttempts' import { DeleteSetting } from '../../Domain/UseCase/DeleteSetting/DeleteSetting' import { GetSetting } from './../../Domain/UseCase/GetSetting/GetSetting' import { UserRepositoryInterface } from '../../Domain/User/UserRepositoryInterface' @@ -25,8 +26,16 @@ export class AnnotatedAdminController extends BaseAdminController { @inject(TYPES.Auth_CreateSubscriptionToken) override createSubscriptionToken: CreateSubscriptionToken, @inject(TYPES.Auth_CreateOfflineSubscriptionToken) override createOfflineSubscriptionToken: CreateOfflineSubscriptionToken, + @inject(TYPES.Auth_ClearLoginAttempts) override clearLoginAttempts: ClearLoginAttempts, ) { - super(doDeleteSetting, doGetSetting, userRepository, createSubscriptionToken, createOfflineSubscriptionToken) + super( + doDeleteSetting, + doGetSetting, + userRepository, + createSubscriptionToken, + createOfflineSubscriptionToken, + clearLoginAttempts, + ) } @httpGet('/user/:email') @@ -60,4 +69,11 @@ export class AnnotatedAdminController extends BaseAdminController { ): Promise { return super.disableEmailBackups(request) } + + @httpDelete('/users/:email/login-attempts') + override async deleteLoginAttempts( + request: Request, + ): Promise { + return super.deleteLoginAttempts(request) + } } diff --git a/packages/auth/src/Infra/InversifyExpressUtils/Base/BaseAdminController.ts b/packages/auth/src/Infra/InversifyExpressUtils/Base/BaseAdminController.ts index ee9340789..54837ef1f 100644 --- a/packages/auth/src/Infra/InversifyExpressUtils/Base/BaseAdminController.ts +++ b/packages/auth/src/Infra/InversifyExpressUtils/Base/BaseAdminController.ts @@ -4,6 +4,7 @@ import { Request } from 'express' import { CreateOfflineSubscriptionToken } from '../../../Domain/UseCase/CreateOfflineSubscriptionToken/CreateOfflineSubscriptionToken' import { CreateSubscriptionToken } from '../../../Domain/UseCase/CreateSubscriptionToken/CreateSubscriptionToken' +import { ClearLoginAttempts } from '../../../Domain/UseCase/ClearLoginAttempts' import { GetSetting } from './../../../Domain/UseCase/GetSetting/GetSetting' import { DeleteSetting } from '../../../Domain/UseCase/DeleteSetting/DeleteSetting' import { UserRepositoryInterface } from '../../../Domain/User/UserRepositoryInterface' @@ -16,6 +17,7 @@ export class BaseAdminController extends BaseHttpController { protected userRepository: UserRepositoryInterface, protected createSubscriptionToken: CreateSubscriptionToken, protected createOfflineSubscriptionToken: CreateOfflineSubscriptionToken, + protected clearLoginAttempts: ClearLoginAttempts, private controllerContainer?: ControllerContainerInterface, ) { super() @@ -26,6 +28,7 @@ export class BaseAdminController extends BaseHttpController { this.controllerContainer.register('admin.createToken', this.createToken.bind(this)) this.controllerContainer.register('admin.createOfflineToken', this.createOfflineToken.bind(this)) this.controllerContainer.register('admin.disableEmailBackups', this.disableEmailBackups.bind(this)) + this.controllerContainer.register('admin.deleteLoginAttempts', this.deleteLoginAttempts.bind(this)) } } @@ -145,4 +148,34 @@ export class BaseAdminController extends BaseHttpController { return this.badRequest('No email backups found') } + + async deleteLoginAttempts(request: Request): Promise { + const { email } = request.params + + if (!email) { + return this.json( + { + error: { + message: 'Missing email parameter.', + }, + }, + 400, + ) + } + + const result = await this.clearLoginAttempts.execute({ email }) + + if (result.isFailed()) { + return this.json( + { + error: { + message: result.getError(), + }, + }, + 400, + ) + } + + return this.ok() + } } From e4944b1600d0bdd7a7f42620bfbbac4703b41bf6 Mon Sep 17 00:00:00 2001 From: Antonella Sgarlatta Date: Fri, 29 May 2026 21:21:00 -0300 Subject: [PATCH 08/17] fix: Disallows overwriting mfa secret setting --- .../SetSettingValue/SetSettingValue.spec.ts | 51 ++++++++++++++++++- .../SetSettingValue/SetSettingValue.ts | 8 +++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/packages/auth/src/Domain/UseCase/SetSettingValue/SetSettingValue.spec.ts b/packages/auth/src/Domain/UseCase/SetSettingValue/SetSettingValue.spec.ts index 2afaee018..b35245060 100644 --- a/packages/auth/src/Domain/UseCase/SetSettingValue/SetSettingValue.spec.ts +++ b/packages/auth/src/Domain/UseCase/SetSettingValue/SetSettingValue.spec.ts @@ -118,7 +118,7 @@ describe('SetSettingValue', () => { ) }) - it('should update an existing setting', async () => { + it('should return error when trying to overwrite an active MFA secret', async () => { const setting = Setting.create({ name: SettingName.NAMES.MfaSecret, value: '1243359u42395834', @@ -138,6 +138,55 @@ describe('SetSettingValue', () => { value: 'value', }) + expect(result.isFailed()).toBe(true) + expect(result.getError()).toBe('Failed to set MFA secret.') + expect(settingRepository.update).not.toHaveBeenCalled() + }) + + it('should allow setting MFA secret when existing row has a null value', async () => { + const setting = Setting.create({ + name: SettingName.NAMES.MfaSecret, + value: null, + serverEncryptionVersion: EncryptionVersion.Default, + userUuid: Uuid.create('00000000-0000-0000-0000-000000000000').getValue(), + sensitive: true, + timestamps: Timestamps.create(123, 123).getValue(), + }).getValue() + + getSetting.execute = jest.fn().mockReturnValue(Result.ok({ setting })) + + const useCase = createUseCase() + + const result = await useCase.execute({ + userUuid: '00000000-0000-0000-0000-000000000000', + settingName: SettingName.NAMES.MfaSecret, + value: 'value', + }) + + expect(result.isFailed()).toBe(false) + expect(settingRepository.update).toHaveBeenCalled() + }) + + it('should update an existing setting', async () => { + const setting = Setting.create({ + name: SettingName.NAMES.EmailBackupFrequency, + value: 'daily', + serverEncryptionVersion: EncryptionVersion.Unencrypted, + userUuid: Uuid.create('00000000-0000-0000-0000-000000000000').getValue(), + sensitive: false, + timestamps: Timestamps.create(123, 123).getValue(), + }).getValue() + + getSetting.execute = jest.fn().mockReturnValue(Result.ok({ setting })) + + const useCase = createUseCase() + + const result = await useCase.execute({ + userUuid: '00000000-0000-0000-0000-000000000000', + settingName: SettingName.NAMES.EmailBackupFrequency, + value: 'weekly', + }) + expect(result.isFailed()).toBe(false) expect(settingRepository.update).toHaveBeenCalled() }) diff --git a/packages/auth/src/Domain/UseCase/SetSettingValue/SetSettingValue.ts b/packages/auth/src/Domain/UseCase/SetSettingValue/SetSettingValue.ts index 33cb56e64..615b377fc 100644 --- a/packages/auth/src/Domain/UseCase/SetSettingValue/SetSettingValue.ts +++ b/packages/auth/src/Domain/UseCase/SetSettingValue/SetSettingValue.ts @@ -48,6 +48,14 @@ export class SetSettingValue implements UseCaseInterface { decrypted: false, }) + if ( + settingName.value === SettingName.NAMES.MfaSecret && + !settingExists.isFailed() && + settingExists.getValue().setting.props.value !== null + ) { + return Result.fail('Failed to set MFA secret.') + } + const sensitive = this.settingsAssociationService.getSensitivityForSetting(settingName) const encryptionVersion = this.settingsAssociationService.getEncryptionVersionForSetting(settingName) From e0968d3806a8035a816e26b30dd6246b09579657 Mon Sep 17 00:00:00 2001 From: Antonella Sgarlatta Date: Tue, 23 Jun 2026 18:56:09 -0300 Subject: [PATCH 09/17] fix: Fixes admin and internal endpoints reachable through route params --- packages/api-gateway/src/Service/Resolver/EndpointResolver.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/api-gateway/src/Service/Resolver/EndpointResolver.ts b/packages/api-gateway/src/Service/Resolver/EndpointResolver.ts index 4d30f500c..ecf9dc9b3 100644 --- a/packages/api-gateway/src/Service/Resolver/EndpointResolver.ts +++ b/packages/api-gateway/src/Service/Resolver/EndpointResolver.ts @@ -96,7 +96,7 @@ export class EndpointResolver implements EndpointResolverInterface { resolveEndpointOrMethodIdentifier(method: string, endpoint: string, ...params: string[]): string { if (!this.isConfiguredForHomeServer) { if (params.length > 0) { - return params.reduce((acc, param) => acc.replace(/:[a-zA-Z0-9]+/, param), endpoint) + return params.reduce((acc, param) => acc.replace(/:[a-zA-Z0-9]+/, encodeURIComponent(param)), endpoint) } return endpoint From fe95f5d7b5b7b920a2606cf4d984c7098899fe7d Mon Sep 17 00:00:00 2001 From: Giedrius Koksta Date: Thu, 2 Jul 2026 07:39:46 +0000 Subject: [PATCH 10/17] SNOTES-726 implement transaction for quota updates --- ...3924-unique-index-subscription-settings.ts | 29 +++ ...3924-unique-index-subscription-settings.ts | 29 +++ packages/auth/src/Bootstrap/Container.ts | 4 +- .../SubscriptionSettingRepositoryInterface.ts | 13 ++ .../UpdateStorageQuotaUsedForUser.spec.ts | 203 ++++++++---------- .../UpdateStorageQuotaUsedForUser.ts | 48 ++--- .../TypeORM/TypeORMSubscriptionSetting.ts | 2 +- .../TypeORMSubscriptionSettingRepository.ts | 39 +++- 8 files changed, 223 insertions(+), 144 deletions(-) create mode 100644 packages/auth/migrations/mysql/1782907913924-unique-index-subscription-settings.ts create mode 100644 packages/auth/migrations/sqlite/1782907913924-unique-index-subscription-settings.ts diff --git a/packages/auth/migrations/mysql/1782907913924-unique-index-subscription-settings.ts b/packages/auth/migrations/mysql/1782907913924-unique-index-subscription-settings.ts new file mode 100644 index 000000000..e4a8bcce6 --- /dev/null +++ b/packages/auth/migrations/mysql/1782907913924-unique-index-subscription-settings.ts @@ -0,0 +1,29 @@ +import { MigrationInterface, QueryRunner } from 'typeorm' + +export class UniqueIndexSubscriptionSettings1782907913924 implements MigrationInterface { + name = 'UniqueIndexSubscriptionSettings1782907913924' + + public async up(queryRunner: QueryRunner): Promise { + // Remove duplicate (name, user_subscription_uuid) rows, keeping only the most recently updated + // one per pair (tie-broken deterministically by uuid) so the unique index below can be created. + await queryRunner.query( + 'DELETE s1 FROM `subscription_settings` s1 ' + + 'INNER JOIN `subscription_settings` s2 ' + + 'ON s1.name = s2.name ' + + 'AND s1.user_subscription_uuid = s2.user_subscription_uuid ' + + 'AND (s2.updated_at > s1.updated_at OR (s2.updated_at = s1.updated_at AND s2.uuid > s1.uuid))', + ) + + await queryRunner.query('DROP INDEX `index_settings_on_name_and_user_subscription_uuid` ON `subscription_settings`') + await queryRunner.query( + 'CREATE UNIQUE INDEX `index_settings_on_name_and_user_subscription_uuid` ON `subscription_settings` (`name`, `user_subscription_uuid`)', + ) + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query('DROP INDEX `index_settings_on_name_and_user_subscription_uuid` ON `subscription_settings`') + await queryRunner.query( + 'CREATE INDEX `index_settings_on_name_and_user_subscription_uuid` ON `subscription_settings` (`name`, `user_subscription_uuid`)', + ) + } +} diff --git a/packages/auth/migrations/sqlite/1782907913924-unique-index-subscription-settings.ts b/packages/auth/migrations/sqlite/1782907913924-unique-index-subscription-settings.ts new file mode 100644 index 000000000..c86211ae4 --- /dev/null +++ b/packages/auth/migrations/sqlite/1782907913924-unique-index-subscription-settings.ts @@ -0,0 +1,29 @@ +import { MigrationInterface, QueryRunner } from 'typeorm' + +export class UniqueIndexSubscriptionSettings1782907913924 implements MigrationInterface { + name = 'UniqueIndexSubscriptionSettings1782907913924' + + public async up(queryRunner: QueryRunner): Promise { + // Remove duplicate (name, user_subscription_uuid) rows, keeping only the most recently updated + // one per pair (tie-broken deterministically by uuid) so the unique index below can be created. + await queryRunner.query( + 'DELETE FROM "subscription_settings" WHERE uuid NOT IN (' + + 'SELECT uuid FROM (' + + 'SELECT uuid, ROW_NUMBER() OVER (' + + 'PARTITION BY name, user_subscription_uuid ORDER BY updated_at DESC, uuid DESC) rn ' + + 'FROM "subscription_settings") WHERE rn = 1)', + ) + + await queryRunner.query('DROP INDEX "index_settings_on_name_and_user_subscription_uuid"') + await queryRunner.query( + 'CREATE UNIQUE INDEX "index_settings_on_name_and_user_subscription_uuid" ON "subscription_settings" ("name", "user_subscription_uuid")', + ) + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query('DROP INDEX "index_settings_on_name_and_user_subscription_uuid"') + await queryRunner.query( + 'CREATE INDEX "index_settings_on_name_and_user_subscription_uuid" ON "subscription_settings" ("name", "user_subscription_uuid")', + ) + } +} diff --git a/packages/auth/src/Bootstrap/Container.ts b/packages/auth/src/Bootstrap/Container.ts index 738222724..9fe7768a5 100644 --- a/packages/auth/src/Bootstrap/Container.ts +++ b/packages/auth/src/Bootstrap/Container.ts @@ -1434,8 +1434,8 @@ export class ContainerConfigLoader { container.get(TYPES.Auth_UserRepository), container.get(TYPES.Auth_GetRegularSubscriptionForUser), container.get(TYPES.Auth_GetSharedSubscriptionForUser), - container.get(TYPES.Auth_GetSubscriptionSetting), - container.get(TYPES.Auth_SetSubscriptionSettingValue), + container.get(TYPES.Auth_SubscriptionSettingRepository), + container.get(TYPES.Auth_Timer), container.get(TYPES.Auth_Logger), ), ) diff --git a/packages/auth/src/Domain/Setting/SubscriptionSettingRepositoryInterface.ts b/packages/auth/src/Domain/Setting/SubscriptionSettingRepositoryInterface.ts index 99fbca0ef..667818197 100644 --- a/packages/auth/src/Domain/Setting/SubscriptionSettingRepositoryInterface.ts +++ b/packages/auth/src/Domain/Setting/SubscriptionSettingRepositoryInterface.ts @@ -8,4 +8,17 @@ export interface SubscriptionSettingRepositoryInterface { findAllBySubscriptionUuid(userSubscriptionUuid: Uuid): Promise insert(subscriptionSetting: SubscriptionSetting): Promise update(subscriptionSetting: SubscriptionSetting): Promise + /** + * Atomically adds `delta` to the numeric value of the `(name, userSubscriptionUuid)` counter + * setting at the database level, clamped to a minimum of 0, creating the row if it does not yet + * exist. Relies on the unique index on `(name, user_subscription_uuid)` to avoid the + * read-modify-write and create-if-missing races that occur when this is computed in application + * code. + */ + incrementCounterValueForNameAndUserSubscriptionUuid( + name: string, + userSubscriptionUuid: Uuid, + delta: number, + updatedAt: number, + ): Promise } diff --git a/packages/auth/src/Domain/UseCase/UpdateStorageQuotaUsedForUser/UpdateStorageQuotaUsedForUser.spec.ts b/packages/auth/src/Domain/UseCase/UpdateStorageQuotaUsedForUser/UpdateStorageQuotaUsedForUser.spec.ts index 260bdca67..680faaf07 100644 --- a/packages/auth/src/Domain/UseCase/UpdateStorageQuotaUsedForUser/UpdateStorageQuotaUsedForUser.spec.ts +++ b/packages/auth/src/Domain/UseCase/UpdateStorageQuotaUsedForUser/UpdateStorageQuotaUsedForUser.spec.ts @@ -6,12 +6,10 @@ import { User } from '../../User/User' import { UserRepositoryInterface } from '../../User/UserRepositoryInterface' import { GetSharedSubscriptionForUser } from '../GetSharedSubscriptionForUser/GetSharedSubscriptionForUser' import { GetRegularSubscriptionForUser } from '../GetRegularSubscriptionForUser/GetRegularSubscriptionForUser' -import { GetSubscriptionSetting } from '../GetSubscriptionSetting/GetSubscriptionSetting' -import { SetSubscriptionSettingValue } from '../SetSubscriptionSettingValue/SetSubscriptionSettingValue' import { Logger } from 'winston' -import { Result, SettingName, Timestamps, Uuid } from '@standardnotes/domain-core' -import { SubscriptionSetting } from '../../Setting/SubscriptionSetting' -import { EncryptionVersion } from '../../Encryption/EncryptionVersion' +import { Result, SettingName } from '@standardnotes/domain-core' +import { TimerInterface } from '@standardnotes/time' +import { SubscriptionSettingRepositoryInterface } from '../../Setting/SubscriptionSettingRepositoryInterface' describe('UpdateStorageQuotaUsedForUser', () => { let userRepository: UserRepositoryInterface @@ -20,17 +18,20 @@ describe('UpdateStorageQuotaUsedForUser', () => { let sharedSubscription: UserSubscription let getSharedSubscription: GetSharedSubscriptionForUser let getRegularSubscription: GetRegularSubscriptionForUser - let getSubscriptionSetting: GetSubscriptionSetting - let setSubscriptonSettingValue: SetSubscriptionSettingValue + let subscriptionSettingRepository: SubscriptionSettingRepositoryInterface + let timer: TimerInterface let logger: Logger + const regularSubscriptionUuid = '00000000-0000-0000-0000-000000000000' + const sharedSubscriptionUuid = '11111111-1111-1111-1111-111111111111' + const createUseCase = () => new UpdateStorageQuotaUsedForUser( userRepository, getRegularSubscription, getSharedSubscription, - getSubscriptionSetting, - setSubscriptonSettingValue, + subscriptionSettingRepository, + timer, logger, ) @@ -43,13 +44,13 @@ describe('UpdateStorageQuotaUsedForUser', () => { userRepository.findOneByUuid = jest.fn().mockReturnValue(user) regularSubscription = { - uuid: '00000000-0000-0000-0000-000000000000', + uuid: regularSubscriptionUuid, subscriptionType: UserSubscriptionType.Regular, userUuid: '123', } as jest.Mocked sharedSubscription = { - uuid: '2-3-4', + uuid: sharedSubscriptionUuid, subscriptionType: UserSubscriptionType.Shared, userUuid: '123', } as jest.Mocked @@ -60,28 +61,71 @@ describe('UpdateStorageQuotaUsedForUser', () => { getRegularSubscription = {} as jest.Mocked getRegularSubscription.execute = jest.fn().mockReturnValue(Result.ok(regularSubscription)) - getSubscriptionSetting = {} as jest.Mocked - getSubscriptionSetting.execute = jest.fn().mockReturnValue(Result.fail('not found')) + subscriptionSettingRepository = {} as jest.Mocked + subscriptionSettingRepository.incrementCounterValueForNameAndUserSubscriptionUuid = jest + .fn() + .mockResolvedValue(undefined) - setSubscriptonSettingValue = {} as jest.Mocked - setSubscriptonSettingValue.execute = jest.fn().mockReturnValue(Result.ok()) + timer = {} as jest.Mocked + timer.getTimestampInMicroseconds = jest.fn().mockReturnValue(123) logger = {} as jest.Mocked logger.error = jest.fn() }) - it('should create a bytes used setting if one does not exist', async () => { + it('should atomically add the bytes used delta for the subscription', async () => { const result = await createUseCase().execute({ - userUuid: '00000000-0000-0000-0000-000000000000', + userUuid: regularSubscriptionUuid, bytesUsed: 123, }) expect(result.isFailed()).toBeFalsy() - expect(setSubscriptonSettingValue.execute).toHaveBeenCalledWith({ - settingName: 'FILE_UPLOAD_BYTES_USED', - value: '123', - userSubscriptionUuid: '00000000-0000-0000-0000-000000000000', + + expect( + subscriptionSettingRepository.incrementCounterValueForNameAndUserSubscriptionUuid, + ).toHaveBeenCalledWith(SettingName.NAMES.FileUploadBytesUsed, expect.objectContaining({ value: regularSubscriptionUuid }), 123, 123) + }) + + it('should atomically subtract the bytes used delta for the subscription', async () => { + const result = await createUseCase().execute({ + userUuid: regularSubscriptionUuid, + bytesUsed: -123, }) + + expect(result.isFailed()).toBeFalsy() + + expect( + subscriptionSettingRepository.incrementCounterValueForNameAndUserSubscriptionUuid, + ).toHaveBeenCalledWith(SettingName.NAMES.FileUploadBytesUsed, expect.objectContaining({ value: regularSubscriptionUuid }), -123, 123) + }) + + it('should update the bytes used on both the regular and shared subscription', async () => { + const result = await createUseCase().execute({ + userUuid: regularSubscriptionUuid, + bytesUsed: 123, + }) + + expect(result.isFailed()).toBeFalsy() + + expect( + subscriptionSettingRepository.incrementCounterValueForNameAndUserSubscriptionUuid, + ).toHaveBeenCalledWith(SettingName.NAMES.FileUploadBytesUsed, expect.objectContaining({ value: sharedSubscriptionUuid }), 123, 123) + expect( + subscriptionSettingRepository.incrementCounterValueForNameAndUserSubscriptionUuid, + ).toHaveBeenCalledWith(SettingName.NAMES.FileUploadBytesUsed, expect.objectContaining({ value: regularSubscriptionUuid }), 123, 123) + expect(subscriptionSettingRepository.incrementCounterValueForNameAndUserSubscriptionUuid).toHaveBeenCalledTimes(2) + }) + + it('should update only the regular subscription when there is no shared subscription', async () => { + getSharedSubscription.execute = jest.fn().mockReturnValue(Result.fail('no shared subscription')) + + const result = await createUseCase().execute({ + userUuid: regularSubscriptionUuid, + bytesUsed: 123, + }) + + expect(result.isFailed()).toBeFalsy() + expect(subscriptionSettingRepository.incrementCounterValueForNameAndUserSubscriptionUuid).toHaveBeenCalledTimes(1) }) it('should not do anything if a user uuid is invalid', async () => { @@ -91,110 +135,45 @@ describe('UpdateStorageQuotaUsedForUser', () => { }) expect(result.isFailed()).toBeTruthy() - expect(setSubscriptonSettingValue.execute).not.toHaveBeenCalled() + expect(subscriptionSettingRepository.incrementCounterValueForNameAndUserSubscriptionUuid).not.toHaveBeenCalled() }) it('should not do anything if a user is not found', async () => { userRepository.findOneByUuid = jest.fn().mockReturnValue(null) const result = await createUseCase().execute({ - userUuid: '00000000-0000-0000-0000-000000000000', + userUuid: regularSubscriptionUuid, bytesUsed: 123, }) expect(result.isFailed()).toBeTruthy() - expect(setSubscriptonSettingValue.execute).not.toHaveBeenCalled() + expect(subscriptionSettingRepository.incrementCounterValueForNameAndUserSubscriptionUuid).not.toHaveBeenCalled() }) - describe('updating existing quota', () => { - beforeEach(() => { - getSubscriptionSetting.execute = jest.fn().mockReturnValue( - Result.ok({ - setting: SubscriptionSetting.create({ - name: SettingName.NAMES.FileUploadBytesUsed, - sensitive: false, - serverEncryptionVersion: EncryptionVersion.Unencrypted, - timestamps: Timestamps.create(123, 123).getValue(), - userSubscriptionUuid: Uuid.create('00000000-0000-0000-0000-000000000000').getValue(), - value: '345', - }).getValue(), - }), - ) + it('should not do anything if a user subscription is not found', async () => { + getRegularSubscription.execute = jest.fn().mockReturnValue(Result.fail('error')) + getSharedSubscription.execute = jest.fn().mockReturnValue(Result.fail('error')) + + const result = await createUseCase().execute({ + userUuid: regularSubscriptionUuid, + bytesUsed: 123, + }) + expect(result.isFailed()).toBeTruthy() + + expect(subscriptionSettingRepository.incrementCounterValueForNameAndUserSubscriptionUuid).not.toHaveBeenCalled() + }) + + it('should log an error and skip the update if a subscription has an invalid uuid', async () => { + getSharedSubscription.execute = jest.fn().mockReturnValue(Result.fail('no shared subscription')) + regularSubscription.uuid = 'invalid-subscription-uuid' + + const result = await createUseCase().execute({ + userUuid: regularSubscriptionUuid, + bytesUsed: 123, }) - it('should not do anything if a user subscription is not found', async () => { - getRegularSubscription.execute = jest.fn().mockReturnValue(Result.fail('error')) - getSharedSubscription.execute = jest.fn().mockReturnValue(Result.fail('error')) - - const result = await createUseCase().execute({ - userUuid: '00000000-0000-0000-0000-000000000000', - bytesUsed: 123, - }) - expect(result.isFailed()).toBeTruthy() - - expect(setSubscriptonSettingValue.execute).not.toHaveBeenCalled() - }) - - it('should add bytes used setting if one does exist', async () => { - const result = await createUseCase().execute({ - userUuid: '00000000-0000-0000-0000-000000000000', - bytesUsed: 123, - }) - expect(result.isFailed()).toBeFalsy() - - expect(setSubscriptonSettingValue.execute).toHaveBeenCalledWith({ - settingName: 'FILE_UPLOAD_BYTES_USED', - value: '468', - userSubscriptionUuid: '00000000-0000-0000-0000-000000000000', - }) - }) - - it('should subtract bytes used setting if one does exist', async () => { - const result = await createUseCase().execute({ - userUuid: '00000000-0000-0000-0000-000000000000', - bytesUsed: -123, - }) - expect(result.isFailed()).toBeFalsy() - - expect(setSubscriptonSettingValue.execute).toHaveBeenCalledWith({ - settingName: 'FILE_UPLOAD_BYTES_USED', - value: '222', - userSubscriptionUuid: '00000000-0000-0000-0000-000000000000', - }) - }) - - it('should not subtract below 0', async () => { - const result = await createUseCase().execute({ - userUuid: '00000000-0000-0000-0000-000000000000', - bytesUsed: -1234, - }) - expect(result.isFailed()).toBeFalsy() - - expect(setSubscriptonSettingValue.execute).toHaveBeenCalledWith({ - settingName: 'FILE_UPLOAD_BYTES_USED', - value: '0', - userSubscriptionUuid: '00000000-0000-0000-0000-000000000000', - }) - }) - - it('should update a bytes used setting on both regular and shared subscription', async () => { - const result = await createUseCase().execute({ - userUuid: '00000000-0000-0000-0000-000000000000', - bytesUsed: 123, - }) - expect(result.isFailed()).toBeFalsy() - - expect(setSubscriptonSettingValue.execute).toHaveBeenCalledWith({ - settingName: 'FILE_UPLOAD_BYTES_USED', - value: '468', - userSubscriptionUuid: '00000000-0000-0000-0000-000000000000', - }) - - expect(setSubscriptonSettingValue.execute).toHaveBeenCalledWith({ - settingName: 'FILE_UPLOAD_BYTES_USED', - value: '468', - userSubscriptionUuid: '2-3-4', - }) - }) + expect(result.isFailed()).toBeFalsy() + expect(logger.error).toHaveBeenCalled() + expect(subscriptionSettingRepository.incrementCounterValueForNameAndUserSubscriptionUuid).not.toHaveBeenCalled() }) }) diff --git a/packages/auth/src/Domain/UseCase/UpdateStorageQuotaUsedForUser/UpdateStorageQuotaUsedForUser.ts b/packages/auth/src/Domain/UseCase/UpdateStorageQuotaUsedForUser/UpdateStorageQuotaUsedForUser.ts index 473b2f8c5..c47a3a4eb 100644 --- a/packages/auth/src/Domain/UseCase/UpdateStorageQuotaUsedForUser/UpdateStorageQuotaUsedForUser.ts +++ b/packages/auth/src/Domain/UseCase/UpdateStorageQuotaUsedForUser/UpdateStorageQuotaUsedForUser.ts @@ -1,21 +1,21 @@ import { Result, SettingName, UseCaseInterface, Uuid } from '@standardnotes/domain-core' +import { TimerInterface } from '@standardnotes/time' import { UserSubscription } from '../../Subscription/UserSubscription' import { UserRepositoryInterface } from '../../User/UserRepositoryInterface' import { UpdateStorageQuotaUsedForUserDTO } from './UpdateStorageQuotaUsedForUserDTO' import { GetRegularSubscriptionForUser } from '../GetRegularSubscriptionForUser/GetRegularSubscriptionForUser' -import { GetSubscriptionSetting } from '../GetSubscriptionSetting/GetSubscriptionSetting' -import { SetSubscriptionSettingValue } from '../SetSubscriptionSettingValue/SetSubscriptionSettingValue' import { Logger } from 'winston' import { GetSharedSubscriptionForUser } from '../GetSharedSubscriptionForUser/GetSharedSubscriptionForUser' +import { SubscriptionSettingRepositoryInterface } from '../../Setting/SubscriptionSettingRepositoryInterface' export class UpdateStorageQuotaUsedForUser implements UseCaseInterface { constructor( private userRepository: UserRepositoryInterface, private getRegularSubscription: GetRegularSubscriptionForUser, private getSharedSubscription: GetSharedSubscriptionForUser, - private getSubscriptionSetting: GetSubscriptionSetting, - private setSubscriptonSettingValue: SetSubscriptionSettingValue, + private subscriptionSettingRepository: SubscriptionSettingRepositoryInterface, + private timer: TimerInterface, private logger: Logger, ) {} @@ -55,31 +55,23 @@ export class UpdateStorageQuotaUsedForUser implements UseCaseInterface { } private async updateUploadBytesUsedSetting(subscription: UserSubscription, bytesUsed: number): Promise { - let bytesAlreadyUsed = '0' - - const bytesUsedSettingExists = await this.getSubscriptionSetting.execute({ - userSubscriptionUuid: subscription.uuid, - settingName: SettingName.NAMES.FileUploadBytesUsed, - allowSensitiveRetrieval: false, - }) - - if (!bytesUsedSettingExists.isFailed()) { - const bytesUsedSetting = bytesUsedSettingExists.getValue() - bytesAlreadyUsed = bytesUsedSetting.setting.props.value as string + const userSubscriptionUuidOrError = Uuid.create(subscription.uuid) + if (userSubscriptionUuidOrError.isFailed()) { + this.logger.error( + `Could not update file upload bytes used for subscription ${subscription.uuid}: ${userSubscriptionUuidOrError.getError()}`, + ) + return } + const userSubscriptionUuid = userSubscriptionUuidOrError.getValue() - const bytesUsedNewTotal = +bytesAlreadyUsed + bytesUsed - const bytesUsedValue = bytesUsedNewTotal < 0 ? 0 : bytesUsedNewTotal - - const result = await this.setSubscriptonSettingValue.execute({ - userSubscriptionUuid: subscription.uuid, - settingName: SettingName.NAMES.FileUploadBytesUsed, - value: bytesUsedValue.toString(), - }) - - /* istanbul ignore next */ - if (result.isFailed()) { - this.logger.error(`Could not set file upload bytes used for subscription ${subscription.uuid}`) - } + // Apply the change as an atomic database-level delta that creates the row if it does not exist. + // The unique index on (name, user_subscription_uuid) makes this race-safe for both concurrent + // increments and concurrent first-time creations, so no read/lock/transaction is needed here. + await this.subscriptionSettingRepository.incrementCounterValueForNameAndUserSubscriptionUuid( + SettingName.NAMES.FileUploadBytesUsed, + userSubscriptionUuid, + bytesUsed, + this.timer.getTimestampInMicroseconds(), + ) } } diff --git a/packages/auth/src/Infra/TypeORM/TypeORMSubscriptionSetting.ts b/packages/auth/src/Infra/TypeORM/TypeORMSubscriptionSetting.ts index 8ff2f324f..c85d92cef 100644 --- a/packages/auth/src/Infra/TypeORM/TypeORMSubscriptionSetting.ts +++ b/packages/auth/src/Infra/TypeORM/TypeORMSubscriptionSetting.ts @@ -1,7 +1,7 @@ import { Column, Entity, Index, PrimaryGeneratedColumn } from 'typeorm' @Entity({ name: 'subscription_settings' }) -@Index('index_settings_on_name_and_user_subscription_uuid', ['name', 'userSubscriptionUuid']) +@Index('index_settings_on_name_and_user_subscription_uuid', ['name', 'userSubscriptionUuid'], { unique: true }) export class TypeORMSubscriptionSetting { @PrimaryGeneratedColumn('uuid') declare uuid: string diff --git a/packages/auth/src/Infra/TypeORM/TypeORMSubscriptionSettingRepository.ts b/packages/auth/src/Infra/TypeORM/TypeORMSubscriptionSettingRepository.ts index d3f0c7a15..ccd6ba753 100644 --- a/packages/auth/src/Infra/TypeORM/TypeORMSubscriptionSettingRepository.ts +++ b/packages/auth/src/Infra/TypeORM/TypeORMSubscriptionSettingRepository.ts @@ -1,5 +1,5 @@ import { Repository } from 'typeorm' -import { MapperInterface, Uuid } from '@standardnotes/domain-core' +import { MapperInterface, UniqueEntityId, Uuid } from '@standardnotes/domain-core' import { SubscriptionSettingRepositoryInterface } from '../../Domain/Setting/SubscriptionSettingRepositoryInterface' import { SubscriptionSetting } from '../../Domain/Setting/SubscriptionSetting' @@ -69,4 +69,41 @@ export class TypeORMSubscriptionSettingRepository implements SubscriptionSetting return this.mapper.toDomain(persistence) } + + async incrementCounterValueForNameAndUserSubscriptionUuid( + name: string, + userSubscriptionUuid: Uuid, + delta: number, + updatedAt: number, + ): Promise { + // Atomic upsert: relies on the unique index on (name, user_subscription_uuid) so that + // concurrent updates for the same subscription cannot create duplicate rows or lose an + // increment. The value is stored as text but treated as a signed integer counter, clamped + // to a minimum of 0. `created_at` is only set when a new row is inserted. + const uuid = new UniqueEntityId().toString() + const isSQLite = this.ormRepository.manager.connection.options.type === 'sqlite' + + const query = isSQLite + ? 'INSERT INTO "subscription_settings" ' + + '("uuid", "name", "value", "server_encryption_version", "created_at", "updated_at", "sensitive", "user_subscription_uuid") ' + + 'VALUES (?, ?, MAX(?, 0), 0, ?, ?, 0, ?) ' + + 'ON CONFLICT ("name", "user_subscription_uuid") DO UPDATE SET ' + + "value = MAX(CAST(COALESCE(value, '0') AS INTEGER) + ?, 0), updated_at = ?" + : 'INSERT INTO `subscription_settings` ' + + '(`uuid`, `name`, `value`, `server_encryption_version`, `created_at`, `updated_at`, `sensitive`, `user_subscription_uuid`) ' + + 'VALUES (?, ?, GREATEST(?, 0), 0, ?, ?, 0, ?) ' + + 'ON DUPLICATE KEY UPDATE ' + + "value = GREATEST(CAST(COALESCE(value, '0') AS SIGNED) + ?, 0), updated_at = ?" + + await this.ormRepository.manager.query(query, [ + uuid, + name, + delta, + updatedAt, + updatedAt, + userSubscriptionUuid.value, + delta, + updatedAt, + ]) + } } From 34c2523627c000952b2f950cb16a648b1f3a3f82 Mon Sep 17 00:00:00 2001 From: Antonella Sgarlatta Date: Mon, 6 Jul 2026 15:53:45 +0000 Subject: [PATCH 11/17] fix: Disallows deleting recovery codes through API --- .../SettingsAssociationService.spec.ts | 3 + .../Setting/SettingsAssociationService.ts | 1 + .../DeleteSetting/DeleteSetting.spec.ts | 112 +++++++++++++++++- .../UseCase/DeleteSetting/DeleteSetting.ts | 24 ++++ .../UseCase/DeleteSetting/DeleteSettingDto.ts | 1 + .../Base/BaseSettingsController.ts | 1 + 6 files changed, 141 insertions(+), 1 deletion(-) diff --git a/packages/auth/src/Domain/Setting/SettingsAssociationService.spec.ts b/packages/auth/src/Domain/Setting/SettingsAssociationService.spec.ts index 3bf7decc5..d00162ea0 100644 --- a/packages/auth/src/Domain/Setting/SettingsAssociationService.spec.ts +++ b/packages/auth/src/Domain/Setting/SettingsAssociationService.spec.ts @@ -20,6 +20,9 @@ describe('SettingsAssociationService', () => { expect( createService().isSettingMutableByClient(SettingName.create(SettingName.NAMES.ListedAuthorSecrets).getValue()), ).toBeFalsy() + expect( + createService().isSettingMutableByClient(SettingName.create(SettingName.NAMES.RecoveryCodes).getValue()), + ).toBeFalsy() }) it('should return default encryption version for a setting which enecryption version is not strictly defined', () => { diff --git a/packages/auth/src/Domain/Setting/SettingsAssociationService.ts b/packages/auth/src/Domain/Setting/SettingsAssociationService.ts index 8262120b8..99d3bbe48 100644 --- a/packages/auth/src/Domain/Setting/SettingsAssociationService.ts +++ b/packages/auth/src/Domain/Setting/SettingsAssociationService.ts @@ -34,6 +34,7 @@ export class SettingsAssociationService implements SettingsAssociationServiceInt private readonly CLIENT_IMMUTABLE_SETTINGS = [ SettingName.NAMES.ListedAuthorSecrets, + SettingName.NAMES.RecoveryCodes, SettingName.NAMES.FileUploadBytesLimit, SettingName.NAMES.FileUploadBytesUsed, ] diff --git a/packages/auth/src/Domain/UseCase/DeleteSetting/DeleteSetting.spec.ts b/packages/auth/src/Domain/UseCase/DeleteSetting/DeleteSetting.spec.ts index 8cce19077..7d0569ddd 100644 --- a/packages/auth/src/Domain/UseCase/DeleteSetting/DeleteSetting.spec.ts +++ b/packages/auth/src/Domain/UseCase/DeleteSetting/DeleteSetting.spec.ts @@ -8,15 +8,20 @@ import { SettingRepositoryInterface } from '../../Setting/SettingRepositoryInter import { DeleteSetting } from './DeleteSetting' import { SettingName, Timestamps, Uuid, Result } from '@standardnotes/domain-core' import { VerifyUserServerPassword } from '../VerifyUserServerPassword/VerifyUserServerPassword' +import { SettingsAssociationService } from '../../Setting/SettingsAssociationService' describe('DeleteSetting', () => { let setting: Setting let sensitiveSetting: Setting + let recoveryCodesSetting: Setting + let listedAuthorSecretsSetting: Setting let settingRepository: SettingRepositoryInterface let verifyUserServerPassword: VerifyUserServerPassword + let settingsAssociationService: SettingsAssociationService let timer: TimerInterface - const createUseCase = () => new DeleteSetting(settingRepository, verifyUserServerPassword, timer) + const createUseCase = () => + new DeleteSetting(settingRepository, verifyUserServerPassword, timer, settingsAssociationService) beforeEach(() => { setting = Setting.create({ @@ -37,6 +42,24 @@ describe('DeleteSetting', () => { timestamps: Timestamps.create(123, 123).getValue(), }).getValue() + recoveryCodesSetting = Setting.create({ + name: SettingName.NAMES.RecoveryCodes, + value: 'ABCD EFGH IJKL MNOP', + serverEncryptionVersion: 0, + userUuid: Uuid.create('00000000-0000-0000-0000-000000000000').getValue(), + sensitive: true, + timestamps: Timestamps.create(123, 123).getValue(), + }).getValue() + + listedAuthorSecretsSetting = Setting.create({ + name: SettingName.NAMES.ListedAuthorSecrets, + value: '[]', + serverEncryptionVersion: 0, + userUuid: Uuid.create('00000000-0000-0000-0000-000000000000').getValue(), + sensitive: false, + timestamps: Timestamps.create(123, 123).getValue(), + }).getValue() + settingRepository = {} as jest.Mocked settingRepository.findLastByNameAndUserUuid = jest.fn().mockReturnValue(setting) settingRepository.findOneByUuid = jest.fn().mockReturnValue(setting) @@ -46,6 +69,8 @@ describe('DeleteSetting', () => { verifyUserServerPassword = {} as jest.Mocked verifyUserServerPassword.execute = jest.fn() + settingsAssociationService = new SettingsAssociationService() + timer = {} as jest.Mocked timer.getTimestampInMicroseconds = jest.fn().mockReturnValue(1) }) @@ -218,6 +243,90 @@ describe('DeleteSetting', () => { }) }) + describe('client permission validation for immutable settings', () => { + it('should not allow client to delete recovery codes', async () => { + settingRepository.findLastByNameAndUserUuid = jest.fn().mockReturnValue(recoveryCodesSetting) + + const result = await createUseCase().execute({ + settingName: SettingName.NAMES.RecoveryCodes, + userUuid: '00000000-0000-0000-0000-000000000000', + checkUserPermissions: true, + }) + + expect(result.success).toBe(false) + if (!result.success) { + expect(result.error.message).toBe( + 'User 00000000-0000-0000-0000-000000000000 does not have permission to delete setting RECOVERY_CODES.', + ) + } + expect(settingRepository.deleteByUserUuid).not.toHaveBeenCalled() + }) + + it('should not allow client to delete listed author secrets', async () => { + settingRepository.findLastByNameAndUserUuid = jest.fn().mockReturnValue(listedAuthorSecretsSetting) + + const result = await createUseCase().execute({ + settingName: SettingName.NAMES.ListedAuthorSecrets, + userUuid: '00000000-0000-0000-0000-000000000000', + checkUserPermissions: true, + }) + + expect(result.success).toBe(false) + if (!result.success) { + expect(result.error.message).toBe( + 'User 00000000-0000-0000-0000-000000000000 does not have permission to delete setting LISTED_AUTHOR_SECRETS.', + ) + } + expect(settingRepository.deleteByUserUuid).not.toHaveBeenCalled() + }) + + it('should allow delete of immutable settings when checkUserPermissions is false', async () => { + settingRepository.findLastByNameAndUserUuid = jest.fn().mockReturnValue(recoveryCodesSetting) + + const result = await createUseCase().execute({ + settingName: SettingName.NAMES.RecoveryCodes, + userUuid: '00000000-0000-0000-0000-000000000000', + checkUserPermissions: false, + }) + + expect(result.success).toBe(true) + expect(settingRepository.deleteByUserUuid).toHaveBeenCalledWith({ + userUuid: '00000000-0000-0000-0000-000000000000', + settingName: SettingName.NAMES.RecoveryCodes, + }) + }) + + it('should allow client to delete mutable settings when checkUserPermissions is true', async () => { + const result = await createUseCase().execute({ + settingName: SettingName.NAMES.LogSessionUserAgent, + userUuid: '00000000-0000-0000-0000-000000000000', + checkUserPermissions: true, + }) + + expect(result.success).toBe(true) + expect(settingRepository.deleteByUserUuid).toHaveBeenCalledWith({ + userUuid: '00000000-0000-0000-0000-000000000000', + settingName: SettingName.NAMES.LogSessionUserAgent, + }) + }) + + it('should return error for invalid setting name when checkUserPermissions is true', async () => { + settingRepository.findLastByNameAndUserUuid = jest.fn().mockReturnValue(setting) + + const result = await createUseCase().execute({ + settingName: 'INVALID_SETTING', + userUuid: '00000000-0000-0000-0000-000000000000', + checkUserPermissions: true, + }) + + expect(result.success).toBe(false) + if (!result.success) { + expect(result.error.message).toBe('Invalid setting name: INVALID_SETTING') + } + expect(settingRepository.deleteByUserUuid).not.toHaveBeenCalled() + }) + }) + it('should delete a setting by name and user uuid', async () => { const result = await createUseCase().execute({ settingName: SettingName.NAMES.LogSessionUserAgent, @@ -240,6 +349,7 @@ describe('DeleteSetting', () => { userUuid: '00000000-0000-0000-0000-000000000000', serverPassword: 'correct-password', shouldVerifyUserServerPassword: true, + checkUserPermissions: true, }) expect(result.success).toBe(true) diff --git a/packages/auth/src/Domain/UseCase/DeleteSetting/DeleteSetting.ts b/packages/auth/src/Domain/UseCase/DeleteSetting/DeleteSetting.ts index 4a8aacb2a..27fb8f60e 100644 --- a/packages/auth/src/Domain/UseCase/DeleteSetting/DeleteSetting.ts +++ b/packages/auth/src/Domain/UseCase/DeleteSetting/DeleteSetting.ts @@ -8,6 +8,7 @@ import { UseCaseInterface } from '../UseCaseInterface' import TYPES from '../../../Bootstrap/Types' import { SettingRepositoryInterface } from '../../Setting/SettingRepositoryInterface' import { Setting } from '../../Setting/Setting' +import { SettingsAssociationServiceInterface } from '../../Setting/SettingsAssociationServiceInterface' import { VerifyUserServerPassword } from '../VerifyUserServerPassword/VerifyUserServerPassword' @injectable() @@ -16,6 +17,8 @@ export class DeleteSetting implements UseCaseInterface { @inject(TYPES.Auth_SettingRepository) private settingRepository: SettingRepositoryInterface, @inject(TYPES.Auth_VerifyUserServerPassword) private verifyUserServerPassword: VerifyUserServerPassword, @inject(TYPES.Auth_Timer) private timer: TimerInterface, + @inject(TYPES.Auth_SettingsAssociationService) + private settingsAssociationService: SettingsAssociationServiceInterface, ) {} async execute(dto: DeleteSettingDto): Promise { @@ -32,6 +35,27 @@ export class DeleteSetting implements UseCaseInterface { } } + if (dto.checkUserPermissions) { + const settingNameOrError = SettingName.create(settingName) + if (settingNameOrError.isFailed()) { + return { + success: false, + error: { + message: settingNameOrError.getError(), + }, + } + } + + if (!this.settingsAssociationService.isSettingMutableByClient(settingNameOrError.getValue())) { + return { + success: false, + error: { + message: `User ${userUuid} does not have permission to delete setting ${settingName}.`, + }, + } + } + } + if (shouldVerifyUserServerPassword && [SettingName.NAMES.MfaSecret].includes(setting.props.name)) { const verifyUserServerPasswordResult = await this.verifyUserServerPassword.execute({ userUuid, diff --git a/packages/auth/src/Domain/UseCase/DeleteSetting/DeleteSettingDto.ts b/packages/auth/src/Domain/UseCase/DeleteSetting/DeleteSettingDto.ts index 04a4d51b4..5a7f62949 100644 --- a/packages/auth/src/Domain/UseCase/DeleteSetting/DeleteSettingDto.ts +++ b/packages/auth/src/Domain/UseCase/DeleteSetting/DeleteSettingDto.ts @@ -7,4 +7,5 @@ export type DeleteSettingDto = { serverPassword?: string authTokenVersion?: number shouldVerifyUserServerPassword?: boolean + checkUserPermissions?: boolean } diff --git a/packages/auth/src/Infra/InversifyExpressUtils/Base/BaseSettingsController.ts b/packages/auth/src/Infra/InversifyExpressUtils/Base/BaseSettingsController.ts index 7eed9288c..c58733f9d 100644 --- a/packages/auth/src/Infra/InversifyExpressUtils/Base/BaseSettingsController.ts +++ b/packages/auth/src/Infra/InversifyExpressUtils/Base/BaseSettingsController.ts @@ -265,6 +265,7 @@ export class BaseSettingsController extends BaseHttpController { serverPassword, authTokenVersion: locals.authTokenVersion, shouldVerifyUserServerPassword: true, + checkUserPermissions: true, }) if (result.success) { From 218893338b8cd497324a30384456c72100c07875 Mon Sep 17 00:00:00 2001 From: Antonella Sgarlatta Date: Thu, 9 Jul 2026 18:59:00 -0300 Subject: [PATCH 12/17] fix: Adds token authentication for offline Braintree token --- .../api-gateway/src/Controller/v1/OfflineController.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/api-gateway/src/Controller/v1/OfflineController.ts b/packages/api-gateway/src/Controller/v1/OfflineController.ts index 374de91b2..178906d01 100644 --- a/packages/api-gateway/src/Controller/v1/OfflineController.ts +++ b/packages/api-gateway/src/Controller/v1/OfflineController.ts @@ -44,4 +44,14 @@ export class OfflineController extends BaseHttpController { request.body, ) } + + @httpPost('/payments/checkout-session') + async createOfflineCheckoutSession(request: Request, response: Response): Promise { + await this.httpService.callPaymentsServer(request, response, 'api/pro_users/checkout-session/offline', request.body) + } + + @httpGet('/payments/get-bt-token') + async getOfflineBraintreeToken(request: Request, response: Response): Promise { + await this.httpService.callPaymentsServer(request, response, 'api/pro_users/get-bt-token/offline', request.body) + } } From 6abafb50c51407fed19fc919f32a6db77bfa26e6 Mon Sep 17 00:00:00 2001 From: Antonella Sgarlatta Date: Wed, 15 Jul 2026 19:07:25 -0300 Subject: [PATCH 13/17] fix: Removes orphaned offline features token setting records --- .../OfflineSettingRepositoryInterface.ts | 1 + .../Setting/OfflineSettingService.spec.ts | 35 +++++++++++++++++++ .../Domain/Setting/OfflineSettingService.ts | 8 +++++ .../TypeORMOfflineSettingRepository.ts | 12 +++++++ 4 files changed, 56 insertions(+) diff --git a/packages/auth/src/Domain/Setting/OfflineSettingRepositoryInterface.ts b/packages/auth/src/Domain/Setting/OfflineSettingRepositoryInterface.ts index 5db08d75e..ec3c1a09e 100644 --- a/packages/auth/src/Domain/Setting/OfflineSettingRepositoryInterface.ts +++ b/packages/auth/src/Domain/Setting/OfflineSettingRepositoryInterface.ts @@ -5,4 +5,5 @@ export interface OfflineSettingRepositoryInterface { findOneByNameAndEmail(name: OfflineSettingName, email: string): Promise findOneByNameAndValue(name: OfflineSettingName, value: string): Promise save(offlineSetting: OfflineSetting): Promise + deleteByNameAndValueExcludingEmail(name: OfflineSettingName, value: string, email: string): Promise } diff --git a/packages/auth/src/Domain/Setting/OfflineSettingService.spec.ts b/packages/auth/src/Domain/Setting/OfflineSettingService.spec.ts index 824d7065d..9788186b4 100644 --- a/packages/auth/src/Domain/Setting/OfflineSettingService.spec.ts +++ b/packages/auth/src/Domain/Setting/OfflineSettingService.spec.ts @@ -20,6 +20,7 @@ describe('OfflineSettingService', () => { offlineSettingRepository = {} as jest.Mocked offlineSettingRepository.findOneByNameAndEmail = jest.fn().mockReturnValue(null) offlineSettingRepository.save = jest.fn() + offlineSettingRepository.deleteByNameAndValueExcludingEmail = jest.fn() timer = {} as jest.Mocked timer.getTimestampInMicroseconds = jest.fn().mockReturnValue(123) @@ -40,6 +41,11 @@ describe('OfflineSettingService', () => { updatedAt: 123, serverEncryptionVersion: 0, }) + expect(offlineSettingRepository.deleteByNameAndValueExcludingEmail).toHaveBeenCalledWith( + OfflineSettingName.FeaturesToken, + 'test', + 'test@test.com', + ) }) it('should update an existing offline setting', async () => { @@ -55,5 +61,34 @@ describe('OfflineSettingService', () => { value: 'test', updatedAt: 123, }) + expect(offlineSettingRepository.deleteByNameAndValueExcludingEmail).toHaveBeenCalledWith( + OfflineSettingName.FeaturesToken, + 'test', + 'test@test.com', + ) + }) + + it('should delete stale offline settings mapped to the same token under a different email', async () => { + await createService().createOrUpdate({ + email: 'new@test.com', + name: OfflineSettingName.FeaturesToken, + value: 'shared-token', + }) + + expect(offlineSettingRepository.deleteByNameAndValueExcludingEmail).toHaveBeenCalledWith( + OfflineSettingName.FeaturesToken, + 'shared-token', + 'new@test.com', + ) + }) + + it('should not delete stale settings for non-extension-key offline settings', async () => { + await createService().createOrUpdate({ + email: 'test@test.com', + name: 'OTHER_SETTING' as OfflineSettingName, + value: 'test', + }) + + expect(offlineSettingRepository.deleteByNameAndValueExcludingEmail).not.toHaveBeenCalled() }) }) diff --git a/packages/auth/src/Domain/Setting/OfflineSettingService.ts b/packages/auth/src/Domain/Setting/OfflineSettingService.ts index 6987f73dc..1be838e93 100644 --- a/packages/auth/src/Domain/Setting/OfflineSettingService.ts +++ b/packages/auth/src/Domain/Setting/OfflineSettingService.ts @@ -35,6 +35,14 @@ export class OfflineSettingService implements OfflineSettingServiceInterface { offlineSetting = await this.offlineSettingRepository.save(offlineSetting) + if (dto.name === OfflineSettingName.FeaturesToken) { + await this.offlineSettingRepository.deleteByNameAndValueExcludingEmail( + dto.name, + dto.value, + dto.email, + ) + } + return { success: true, offlineSetting, diff --git a/packages/auth/src/Infra/TypeORM/TypeORMOfflineSettingRepository.ts b/packages/auth/src/Infra/TypeORM/TypeORMOfflineSettingRepository.ts index b8b262900..8fad1d9c9 100644 --- a/packages/auth/src/Infra/TypeORM/TypeORMOfflineSettingRepository.ts +++ b/packages/auth/src/Infra/TypeORM/TypeORMOfflineSettingRepository.ts @@ -33,4 +33,16 @@ export class TypeORMOfflineSettingRepository implements OfflineSettingRepository }) .getOne() } + + async deleteByNameAndValueExcludingEmail(name: OfflineSettingName, value: string, email: string): Promise { + await this.ormRepository + .createQueryBuilder() + .delete() + .where('name = :name AND value = :value AND email != :email', { + name, + value, + email, + }) + .execute() + } } From d07c91594a1bab4d2f3eead12d92be1566172ba1 Mon Sep 17 00:00:00 2001 From: Antonella Sgarlatta Date: Tue, 25 Aug 2026 16:50:49 +0000 Subject: [PATCH 14/17] fix: Prevents Webauthn challenge replay --- .env.sample | 7 ++ docker/docker-entrypoint.sh | 3 + packages/auth/.env.sample | 1 + packages/auth/src/Bootstrap/Container.ts | 8 ++ packages/auth/src/Bootstrap/Types.ts | 1 + .../AuthenticatorChallenge.spec.ts | 11 +++ .../Authenticator/AuthenticatorChallenge.ts | 4 + ...thenticatorChallengeRepositoryInterface.ts | 1 + ...uthenticatorAuthenticationResponse.spec.ts | 74 +++++++++++++- ...rifyAuthenticatorAuthenticationResponse.ts | 15 ++- ...yAuthenticatorRegistrationResponse.spec.ts | 99 ++++++++++++------- ...VerifyAuthenticatorRegistrationResponse.ts | 15 ++- ...TypeORMAuthenticatorChallengeRepository.ts | 11 +++ 13 files changed, 208 insertions(+), 42 deletions(-) diff --git a/.env.sample b/.env.sample index 7ec91d7d8..40eef32d9 100644 --- a/.env.sample +++ b/.env.sample @@ -24,3 +24,10 @@ CACHE_TYPE=redis AUTH_JWT_SECRET= AUTH_SERVER_ENCRYPTION_SERVER_KEY= VALET_TOKEN_SECRET= + +# (Optional) U2F Setup +AUTH_SERVER_U2F_RELYING_PARTY_ID= +AUTH_SERVER_U2F_RELYING_PARTY_NAME= +AUTH_SERVER_U2F_EXPECTED_ORIGIN= +AUTH_SERVER_U2F_REQUIRE_USER_VERIFICATION= +AUTH_SERVER_U2F_CHALLENGE_MAX_AGE_SECONDS= diff --git a/docker/docker-entrypoint.sh b/docker/docker-entrypoint.sh index 2b5dc06a4..0e2b2875e 100755 --- a/docker/docker-entrypoint.sh +++ b/docker/docker-entrypoint.sh @@ -231,6 +231,9 @@ fi if [ -z "$AUTH_SERVER_U2F_REQUIRE_USER_VERIFICATION" ]; then export AUTH_SERVER_U2F_REQUIRE_USER_VERIFICATION=false fi +if [ -z "$AUTH_SERVER_U2F_CHALLENGE_MAX_AGE_SECONDS" ]; then + export AUTH_SERVER_U2F_CHALLENGE_MAX_AGE_SECONDS=300 +fi printenv | grep AUTH_SERVER_ | sed 's/AUTH_SERVER_//g' > /opt/server/packages/auth/.env diff --git a/packages/auth/.env.sample b/packages/auth/.env.sample index 3fbca68da..58745fd34 100644 --- a/packages/auth/.env.sample +++ b/packages/auth/.env.sample @@ -61,6 +61,7 @@ CAPTCHA_UI_URL= U2F_RELYING_PARTY_ID= U2F_RELYING_PARTY_NAME= U2F_EXPECTED_ORIGIN= +U2F_CHALLENGE_MAX_AGE_SECONDS= # Application versiom greater than this var will result in cross service token version 2 APPLICATION_VERSION_THRESHOLD_FOR_TOKEN_VERSION_2= diff --git a/packages/auth/src/Bootstrap/Container.ts b/packages/auth/src/Bootstrap/Container.ts index 9fe7768a5..cd676b230 100644 --- a/packages/auth/src/Bootstrap/Container.ts +++ b/packages/auth/src/Bootstrap/Container.ts @@ -675,6 +675,12 @@ export class ContainerConfigLoader { container .bind(TYPES.Auth_U2F_REQUIRE_USER_VERIFICATION) .toConstantValue(env.get('U2F_REQUIRE_USER_VERIFICATION', true) === 'true') + const challengeMaxAgeSeconds = Number(env.get('U2F_CHALLENGE_MAX_AGE_SECONDS', true)) + container + .bind(TYPES.Auth_AUTHENTICATOR_CHALLENGE_MAX_AGE_SECONDS) + .toConstantValue( + Number.isFinite(challengeMaxAgeSeconds) && challengeMaxAgeSeconds > 0 ? challengeMaxAgeSeconds : 300, + ) container .bind(TYPES.Auth_READONLY_USERS) .toConstantValue(env.get('READONLY_USERS', true) ? env.get('READONLY_USERS', true).split(',') : []) @@ -1036,6 +1042,7 @@ export class ContainerConfigLoader { container.get(TYPES.Auth_U2F_REQUIRE_USER_VERIFICATION), container.get(TYPES.Auth_UserRepository), container.get(TYPES.Auth_FeatureService), + container.get(TYPES.Auth_AUTHENTICATOR_CHALLENGE_MAX_AGE_SECONDS), ), ) container @@ -1057,6 +1064,7 @@ export class ContainerConfigLoader { container.get(TYPES.Auth_U2F_RELYING_PARTY_ID), container.get(TYPES.Auth_U2F_EXPECTED_ORIGIN), container.get(TYPES.Auth_U2F_REQUIRE_USER_VERIFICATION), + container.get(TYPES.Auth_AUTHENTICATOR_CHALLENGE_MAX_AGE_SECONDS), ), ) container diff --git a/packages/auth/src/Bootstrap/Types.ts b/packages/auth/src/Bootstrap/Types.ts index 0a1886cb8..bf8aeb0dd 100644 --- a/packages/auth/src/Bootstrap/Types.ts +++ b/packages/auth/src/Bootstrap/Types.ts @@ -101,6 +101,7 @@ const TYPES = { Auth_U2F_RELYING_PARTY_NAME: Symbol.for('Auth_U2F_RELYING_PARTY_NAME'), Auth_U2F_EXPECTED_ORIGIN: Symbol.for('Auth_U2F_EXPECTED_ORIGIN'), Auth_U2F_REQUIRE_USER_VERIFICATION: Symbol.for('Auth_U2F_REQUIRE_USER_VERIFICATION'), + Auth_AUTHENTICATOR_CHALLENGE_MAX_AGE_SECONDS: Symbol.for('Auth_AUTHENTICATOR_CHALLENGE_MAX_AGE_SECONDS'), Auth_READONLY_USERS: Symbol.for('Auth_READONLY_USERS'), Auth_IS_CONFIGURED_FOR_HOME_SERVER_OR_SELF_HOSTING: Symbol.for('Auth_IS_CONFIGURED_FOR_HOME_SERVER_OR_SELF_HOSTING'), Auth_CAPTCHA_SERVER_URL: Symbol.for('Auth_CAPTCHA_SERVER_URL'), diff --git a/packages/auth/src/Domain/Authenticator/AuthenticatorChallenge.spec.ts b/packages/auth/src/Domain/Authenticator/AuthenticatorChallenge.spec.ts index 37e2948fd..fba6b38aa 100644 --- a/packages/auth/src/Domain/Authenticator/AuthenticatorChallenge.spec.ts +++ b/packages/auth/src/Domain/Authenticator/AuthenticatorChallenge.spec.ts @@ -13,4 +13,15 @@ describe('AuthenticatorChallenge', () => { expect(entityOrError.isFailed()).toBeFalsy() expect(entityOrError.getValue().id).not.toBeNull() }) + + it('should detect expired challenges', () => { + const challenge = AuthenticatorChallenge.create({ + userUuid: Uuid.create('00000000-0000-0000-0000-000000000000').getValue(), + createdAt: new Date(Date.now() - 301_000), + challenge: 'challenge', + }).getValue() + + expect(challenge.isExpired(300)).toBeTruthy() + expect(challenge.isExpired(600)).toBeFalsy() + }) }) diff --git a/packages/auth/src/Domain/Authenticator/AuthenticatorChallenge.ts b/packages/auth/src/Domain/Authenticator/AuthenticatorChallenge.ts index 3630b6aae..c4f7052bb 100644 --- a/packages/auth/src/Domain/Authenticator/AuthenticatorChallenge.ts +++ b/packages/auth/src/Domain/Authenticator/AuthenticatorChallenge.ts @@ -10,4 +10,8 @@ export class AuthenticatorChallenge extends Entity static create(props: AuthenticatorChallengeProps, id?: UniqueEntityId): Result { return Result.ok(new AuthenticatorChallenge(props, id)) } + + isExpired(maxAgeSeconds: number, now: Date = new Date()): boolean { + return now.getTime() - this.props.createdAt.getTime() > maxAgeSeconds * 1000 + } } diff --git a/packages/auth/src/Domain/Authenticator/AuthenticatorChallengeRepositoryInterface.ts b/packages/auth/src/Domain/Authenticator/AuthenticatorChallengeRepositoryInterface.ts index ef13607e2..9201d15b3 100644 --- a/packages/auth/src/Domain/Authenticator/AuthenticatorChallengeRepositoryInterface.ts +++ b/packages/auth/src/Domain/Authenticator/AuthenticatorChallengeRepositoryInterface.ts @@ -5,4 +5,5 @@ import { AuthenticatorChallenge } from './AuthenticatorChallenge' export interface AuthenticatorChallengeRepositoryInterface { findByUserUuid(userUuid: Uuid): Promise save(authenticatorChallenge: AuthenticatorChallenge): Promise + deleteByUserUuid(userUuid: Uuid): Promise } diff --git a/packages/auth/src/Domain/UseCase/VerifyAuthenticatorAuthenticationResponse/VerifyAuthenticatorAuthenticationResponse.spec.ts b/packages/auth/src/Domain/UseCase/VerifyAuthenticatorAuthenticationResponse/VerifyAuthenticatorAuthenticationResponse.spec.ts index 1ec4e9beb..94fe3346a 100644 --- a/packages/auth/src/Domain/UseCase/VerifyAuthenticatorAuthenticationResponse/VerifyAuthenticatorAuthenticationResponse.spec.ts +++ b/packages/auth/src/Domain/UseCase/VerifyAuthenticatorAuthenticationResponse/VerifyAuthenticatorAuthenticationResponse.spec.ts @@ -19,6 +19,7 @@ describe('VerifyAuthenticatorAuthenticationResponse', () => { 'standardnotes.com', ['localhost', 'https://app.standardnotes.com'], true, + 300, ) beforeEach(() => { @@ -38,11 +39,14 @@ describe('VerifyAuthenticatorAuthenticationResponse', () => { authenticatorRepository.updateCounter = jest.fn() authenticatorChallengeRepository = {} as jest.Mocked - authenticatorChallengeRepository.findByUserUuid = jest.fn().mockReturnValue({ - props: { + authenticatorChallengeRepository.findByUserUuid = jest.fn().mockReturnValue( + AuthenticatorChallenge.create({ + userUuid: Uuid.create('00000000-0000-0000-0000-000000000000').getValue(), challenge: 'challenge', - }, - } as jest.Mocked) + createdAt: new Date(), + }).getValue(), + ) + authenticatorChallengeRepository.deleteByUserUuid = jest.fn().mockResolvedValue(1) }) it('should return error if user uuid is invalid', async () => { @@ -221,5 +225,67 @@ describe('VerifyAuthenticatorAuthenticationResponse', () => { expect(result.isFailed()).toBeFalsy() expect(authenticatorRepository.updateCounter).toHaveBeenCalled() + expect(authenticatorChallengeRepository.deleteByUserUuid).toHaveBeenCalled() + }) + + it('should return error if authenticator challenge is already consumed', async () => { + authenticatorChallengeRepository.deleteByUserUuid = jest.fn().mockResolvedValue(0) + + const useCase = createUseCase() + + const result = await useCase.execute({ + userUuid: '00000000-0000-0000-0000-000000000000', + authenticatorResponse: { + authenticatorAttachment: 'platform', + clientExtensionResults: {}, + id: 'id', + rawId: 'rawId', + response: { + authenticatorData: 'authenticatorData', + clientDataJSON: 'clientDataJSON', + signature: 'signature', + userHandle: 'userHandle', + }, + type: 'public-key', + }, + }) + + expect(result.isFailed()).toBeTruthy() + expect(result.getError()).toEqual( + 'Could not verify authenticator authentication response: challenge already consumed', + ) + }) + + it('should return error if authenticator challenge is expired', async () => { + authenticatorChallengeRepository.findByUserUuid = jest.fn().mockReturnValue( + AuthenticatorChallenge.create({ + userUuid: Uuid.create('00000000-0000-0000-0000-000000000000').getValue(), + challenge: 'challenge', + createdAt: new Date(Date.now() - 301_000), + }).getValue(), + ) + + const useCase = createUseCase() + + const result = await useCase.execute({ + userUuid: '00000000-0000-0000-0000-000000000000', + authenticatorResponse: { + authenticatorAttachment: 'platform', + clientExtensionResults: {}, + id: 'id', + rawId: 'rawId', + response: { + authenticatorData: 'authenticatorData', + clientDataJSON: 'clientDataJSON', + signature: 'signature', + userHandle: 'userHandle', + }, + type: 'public-key', + }, + }) + + expect(result.isFailed()).toBeTruthy() + expect(result.getError()).toEqual('Could not verify authenticator authentication response: challenge expired') + expect(authenticatorChallengeRepository.deleteByUserUuid).toHaveBeenCalled() }) }) diff --git a/packages/auth/src/Domain/UseCase/VerifyAuthenticatorAuthenticationResponse/VerifyAuthenticatorAuthenticationResponse.ts b/packages/auth/src/Domain/UseCase/VerifyAuthenticatorAuthenticationResponse/VerifyAuthenticatorAuthenticationResponse.ts index 901bbbdf7..e98c6d360 100644 --- a/packages/auth/src/Domain/UseCase/VerifyAuthenticatorAuthenticationResponse/VerifyAuthenticatorAuthenticationResponse.ts +++ b/packages/auth/src/Domain/UseCase/VerifyAuthenticatorAuthenticationResponse/VerifyAuthenticatorAuthenticationResponse.ts @@ -13,6 +13,7 @@ export class VerifyAuthenticatorAuthenticationResponse implements UseCaseInterfa private relyingPartyId: string, private expectedOrigin: string[], private requireUserVerification: boolean, + private authenticatorChallengeMaxAgeSeconds: number, ) {} async execute(dto: VerifyAuthenticatorAuthenticationResponseDTO): Promise> { @@ -27,6 +28,18 @@ export class VerifyAuthenticatorAuthenticationResponse implements UseCaseInterfa return Result.fail('Could not verify authenticator authentication response: challenge not found') } + if (authenticatorChallenge.isExpired(this.authenticatorChallengeMaxAgeSeconds)) { + await this.authenticatorChallengeRepository.deleteByUserUuid(userUuid) + + return Result.fail('Could not verify authenticator authentication response: challenge expired') + } + + const expectedChallenge = authenticatorChallenge.props.challenge.toString() + const deletedRows = await this.authenticatorChallengeRepository.deleteByUserUuid(userUuid) + if (deletedRows === 0) { + return Result.fail('Could not verify authenticator authentication response: challenge already consumed') + } + const authenticator = await this.authenticatorRepository.findByUserUuidAndCredentialId( userUuid, dto.authenticatorResponse.id as string, @@ -41,7 +54,7 @@ export class VerifyAuthenticatorAuthenticationResponse implements UseCaseInterfa try { verification = await verifyAuthenticationResponse({ response: dto.authenticatorResponse, - expectedChallenge: authenticatorChallenge.props.challenge.toString(), + expectedChallenge, expectedOrigin: this.expectedOrigin, expectedRPID: this.relyingPartyId, requireUserVerification: this.requireUserVerification, diff --git a/packages/auth/src/Domain/UseCase/VerifyAuthenticatorRegistrationResponse/VerifyAuthenticatorRegistrationResponse.spec.ts b/packages/auth/src/Domain/UseCase/VerifyAuthenticatorRegistrationResponse/VerifyAuthenticatorRegistrationResponse.spec.ts index 5623151ab..0d1a51e41 100644 --- a/packages/auth/src/Domain/UseCase/VerifyAuthenticatorRegistrationResponse/VerifyAuthenticatorRegistrationResponse.spec.ts +++ b/packages/auth/src/Domain/UseCase/VerifyAuthenticatorRegistrationResponse/VerifyAuthenticatorRegistrationResponse.spec.ts @@ -1,7 +1,7 @@ import * as simeplWebAuthnServer from '@simplewebauthn/server' import { VerifiedRegistrationResponse } from '@simplewebauthn/server' import { RegistrationResponseJSON } from '@simplewebauthn/typescript-types' -import { Result } from '@standardnotes/domain-core' +import { Result, Uuid } from '@standardnotes/domain-core' import { Authenticator } from '../../Authenticator/Authenticator' import { AuthenticatorChallenge } from '../../Authenticator/AuthenticatorChallenge' @@ -18,6 +18,13 @@ describe('VerifyAuthenticatorRegistrationResponse', () => { let userRepository: UserRepositoryInterface let featureService: FeatureServiceInterface + const createChallenge = (createdAt = new Date()) => + AuthenticatorChallenge.create({ + userUuid: Uuid.create('00000000-0000-0000-0000-000000000000').getValue(), + challenge: 'challenge', + createdAt, + }).getValue() + const createUseCase = () => new VerifyAuthenticatorRegistrationResponse( authenticatorRepository, @@ -27,6 +34,7 @@ describe('VerifyAuthenticatorRegistrationResponse', () => { true, userRepository, featureService, + 300, ) beforeEach(() => { @@ -34,11 +42,8 @@ describe('VerifyAuthenticatorRegistrationResponse', () => { authenticatorRepository.save = jest.fn() authenticatorChallengeRepository = {} as jest.Mocked - authenticatorChallengeRepository.findByUserUuid = jest.fn().mockReturnValue({ - props: { - challenge: 'challenge', - }, - } as jest.Mocked) + authenticatorChallengeRepository.findByUserUuid = jest.fn().mockReturnValue(createChallenge()) + authenticatorChallengeRepository.deleteByUserUuid = jest.fn().mockResolvedValue(1) userRepository = {} as jest.Mocked userRepository.findOneByUuid = jest.fn().mockReturnValue({} as jest.Mocked) @@ -142,12 +147,6 @@ describe('VerifyAuthenticatorRegistrationResponse', () => { }) it('should return error if verification could not verify', async () => { - authenticatorChallengeRepository.findByUserUuid = jest.fn().mockReturnValue({ - props: { - challenge: 'challenge', - }, - } as jest.Mocked) - const useCase = createUseCase() const mock = jest.spyOn(simeplWebAuthnServer, 'verifyRegistrationResponse') @@ -185,12 +184,6 @@ describe('VerifyAuthenticatorRegistrationResponse', () => { }) it('should return error if verification throws error', async () => { - authenticatorChallengeRepository.findByUserUuid = jest.fn().mockReturnValue({ - props: { - challenge: 'challenge', - }, - } as jest.Mocked) - const useCase = createUseCase() const mock = jest.spyOn(simeplWebAuthnServer, 'verifyRegistrationResponse') @@ -219,12 +212,6 @@ describe('VerifyAuthenticatorRegistrationResponse', () => { }) it('should return error if verification is missing registration info', async () => { - authenticatorChallengeRepository.findByUserUuid = jest.fn().mockReturnValue({ - props: { - challenge: 'challenge', - }, - } as jest.Mocked) - const useCase = createUseCase() const mock = jest.spyOn(simeplWebAuthnServer, 'verifyRegistrationResponse') @@ -257,12 +244,6 @@ describe('VerifyAuthenticatorRegistrationResponse', () => { }) it('should return error if authenticator could not be created', async () => { - authenticatorChallengeRepository.findByUserUuid = jest.fn().mockReturnValue({ - props: { - challenge: 'challenge', - }, - } as jest.Mocked) - const useCase = createUseCase() const mock = jest.spyOn(simeplWebAuthnServer, 'verifyRegistrationResponse') @@ -306,12 +287,6 @@ describe('VerifyAuthenticatorRegistrationResponse', () => { }) it('should verify authenticator registration response', async () => { - authenticatorChallengeRepository.findByUserUuid = jest.fn().mockReturnValue({ - props: { - challenge: 'challenge', - }, - } as jest.Mocked) - const useCase = createUseCase() const mock = jest.spyOn(simeplWebAuthnServer, 'verifyRegistrationResponse') @@ -343,7 +318,59 @@ describe('VerifyAuthenticatorRegistrationResponse', () => { }) expect(result.isFailed()).toBeFalsy() + expect(authenticatorChallengeRepository.deleteByUserUuid).toHaveBeenCalled() mock.mockRestore() }) + + it('should return error if authenticator challenge is already consumed', async () => { + authenticatorChallengeRepository.deleteByUserUuid = jest.fn().mockResolvedValue(0) + + const useCase = createUseCase() + + const result = await useCase.execute({ + userUuid: '00000000-0000-0000-0000-000000000000', + attestationResponse: { + id: 'id', + rawId: 'rawId', + response: { + attestationObject: 'attestationObject', + clientDataJSON: 'clientDataJSON', + }, + type: 'public-key', + clientExtensionResults: {}, + } as jest.Mocked, + }) + + expect(result.isFailed()).toBeTruthy() + expect(result.getError()).toEqual( + 'Could not verify authenticator registration response: challenge already consumed', + ) + }) + + it('should return error if authenticator challenge is expired', async () => { + authenticatorChallengeRepository.findByUserUuid = jest + .fn() + .mockReturnValue(createChallenge(new Date(Date.now() - 301_000))) + + const useCase = createUseCase() + + const result = await useCase.execute({ + userUuid: '00000000-0000-0000-0000-000000000000', + attestationResponse: { + id: 'id', + rawId: 'rawId', + response: { + attestationObject: 'attestationObject', + clientDataJSON: 'clientDataJSON', + }, + type: 'public-key', + clientExtensionResults: {}, + } as jest.Mocked, + }) + + expect(result.isFailed()).toBeTruthy() + expect(result.getError()).toEqual('Could not verify authenticator registration response: challenge expired') + expect(authenticatorChallengeRepository.deleteByUserUuid).toHaveBeenCalled() + }) }) diff --git a/packages/auth/src/Domain/UseCase/VerifyAuthenticatorRegistrationResponse/VerifyAuthenticatorRegistrationResponse.ts b/packages/auth/src/Domain/UseCase/VerifyAuthenticatorRegistrationResponse/VerifyAuthenticatorRegistrationResponse.ts index 8857da93e..e44c43cf1 100644 --- a/packages/auth/src/Domain/UseCase/VerifyAuthenticatorRegistrationResponse/VerifyAuthenticatorRegistrationResponse.ts +++ b/packages/auth/src/Domain/UseCase/VerifyAuthenticatorRegistrationResponse/VerifyAuthenticatorRegistrationResponse.ts @@ -18,6 +18,7 @@ export class VerifyAuthenticatorRegistrationResponse implements UseCaseInterface private requireUserVerification: boolean, private userRepository: UserRepositoryInterface, private featureService: FeatureServiceInterface, + private authenticatorChallengeMaxAgeSeconds: number, ) {} async execute(dto: VerifyAuthenticatorRegistrationResponseDTO): Promise> { @@ -46,11 +47,23 @@ export class VerifyAuthenticatorRegistrationResponse implements UseCaseInterface return Result.fail('Could not verify authenticator registration response: challenge not found') } + if (authenticatorChallenge.isExpired(this.authenticatorChallengeMaxAgeSeconds)) { + await this.authenticatorChallengeRepository.deleteByUserUuid(userUuid) + + return Result.fail('Could not verify authenticator registration response: challenge expired') + } + + const expectedChallenge = authenticatorChallenge.props.challenge.toString() + const deletedRows = await this.authenticatorChallengeRepository.deleteByUserUuid(userUuid) + if (deletedRows === 0) { + return Result.fail('Could not verify authenticator registration response: challenge already consumed') + } + let verification: VerifiedRegistrationResponse try { verification = await verifyRegistrationResponse({ response: dto.attestationResponse, - expectedChallenge: authenticatorChallenge.props.challenge.toString(), + expectedChallenge, expectedOrigin: this.expectedOrigin, expectedRPID: this.relyingPartyId, requireUserVerification: this.requireUserVerification, diff --git a/packages/auth/src/Infra/TypeORM/TypeORMAuthenticatorChallengeRepository.ts b/packages/auth/src/Infra/TypeORM/TypeORMAuthenticatorChallengeRepository.ts index 9c522edf2..bd356826c 100644 --- a/packages/auth/src/Infra/TypeORM/TypeORMAuthenticatorChallengeRepository.ts +++ b/packages/auth/src/Infra/TypeORM/TypeORMAuthenticatorChallengeRepository.ts @@ -40,4 +40,15 @@ export class TypeORMAuthenticatorChallengeRepository implements AuthenticatorCha return this.mapper.toDomain(persistence) } + + async deleteByUserUuid(userUuid: Uuid): Promise { + const result = await this.ormRepository + .createQueryBuilder() + .delete() + .from(TypeORMAuthenticatorChallenge) + .where('user_uuid = :userUuid', { userUuid: userUuid.value }) + .execute() + + return result.affected ?? 0 + } } From f1ea94e571e0f6aa8a7ecc1987981b6befd02516 Mon Sep 17 00:00:00 2001 From: Antonella Sgarlatta Date: Fri, 14 Aug 2026 15:54:27 -0300 Subject: [PATCH 15/17] fix: Fixes ephemeral session revocation --- .../RedisEphemeralSessionRepository.spec.ts | 60 +++++++++++++++++++ .../Redis/RedisEphemeralSessionRepository.ts | 6 ++ .../TypeORMEphemeralSessionRepository.spec.ts | 57 ++++++++++++++++++ .../TypeORMEphemeralSessionRepository.ts | 6 ++ 4 files changed, 129 insertions(+) create mode 100644 packages/auth/src/Infra/Redis/RedisEphemeralSessionRepository.spec.ts create mode 100644 packages/auth/src/Infra/TypeORM/TypeORMEphemeralSessionRepository.spec.ts diff --git a/packages/auth/src/Infra/Redis/RedisEphemeralSessionRepository.spec.ts b/packages/auth/src/Infra/Redis/RedisEphemeralSessionRepository.spec.ts new file mode 100644 index 000000000..b65234c76 --- /dev/null +++ b/packages/auth/src/Infra/Redis/RedisEphemeralSessionRepository.spec.ts @@ -0,0 +1,60 @@ +import * as IORedis from 'ioredis' + +import { EphemeralSession } from '../../Domain/Session/EphemeralSession' +import { RedisEphemeralSessionRepository } from './RedisEphemeralSessionRepository' + +describe('RedisEphemeralSessionRepository', () => { + let redisClient: jest.Mocked + let pipeline: { + del: jest.Mock + srem: jest.Mock + exec: jest.Mock + } + let repository: RedisEphemeralSessionRepository + + beforeEach(() => { + pipeline = { + del: jest.fn().mockReturnThis(), + srem: jest.fn().mockReturnThis(), + exec: jest.fn().mockResolvedValue([]), + } + + redisClient = { + get: jest.fn(), + pipeline: jest.fn().mockReturnValue(pipeline), + } as unknown as jest.Mocked + + repository = new RedisEphemeralSessionRepository(redisClient, 3600) + }) + + describe('deleteOne', () => { + it('should delete private identifier mapping when revoking a session', async () => { + const session = { + uuid: 'session-uuid', + userUuid: 'user-uuid', + privateIdentifier: 'private-id', + } as EphemeralSession + + redisClient.get.mockResolvedValue(JSON.stringify(session)) + + await repository.deleteOne('session-uuid', 'user-uuid') + + expect(redisClient.get).toHaveBeenCalledWith('session:session-uuid:user-uuid') + expect(pipeline.del).toHaveBeenCalledWith('session:session-uuid') + expect(pipeline.del).toHaveBeenCalledWith('session:session-uuid:user-uuid') + expect(pipeline.del).toHaveBeenCalledWith('session-private-id:private-id') + expect(pipeline.srem).toHaveBeenCalledWith('user-sessions:user-uuid', 'session-uuid') + expect(pipeline.exec).toHaveBeenCalled() + }) + + it('should skip private identifier deletion when session is not found', async () => { + redisClient.get.mockResolvedValue(null) + + await repository.deleteOne('session-uuid', 'user-uuid') + + expect(pipeline.del).toHaveBeenCalledWith('session:session-uuid') + expect(pipeline.del).toHaveBeenCalledWith('session:session-uuid:user-uuid') + expect(pipeline.del).not.toHaveBeenCalledWith(expect.stringContaining('session-private-id')) + }) + }) +}) diff --git a/packages/auth/src/Infra/Redis/RedisEphemeralSessionRepository.ts b/packages/auth/src/Infra/Redis/RedisEphemeralSessionRepository.ts index aaf6d3f7b..7e2c6bcb5 100644 --- a/packages/auth/src/Infra/Redis/RedisEphemeralSessionRepository.ts +++ b/packages/auth/src/Infra/Redis/RedisEphemeralSessionRepository.ts @@ -26,12 +26,18 @@ export class RedisEphemeralSessionRepository implements EphemeralSessionReposito } async deleteOne(uuid: string, userUuid: string): Promise { + const session = await this.findOneByUuidAndUserUuid(uuid, userUuid) + const pipeline = this.redisClient.pipeline() pipeline.del(`${this.PREFIX}:${uuid}`) pipeline.del(`${this.PREFIX}:${uuid}:${userUuid}`) pipeline.srem(`${this.USER_SESSIONS_PREFIX}:${userUuid}`, uuid) + if (session?.privateIdentifier) { + pipeline.del(`${this.PREFIX_PRIVATE_ID}:${session.privateIdentifier}`) + } + await pipeline.exec() } diff --git a/packages/auth/src/Infra/TypeORM/TypeORMEphemeralSessionRepository.spec.ts b/packages/auth/src/Infra/TypeORM/TypeORMEphemeralSessionRepository.spec.ts new file mode 100644 index 000000000..20fa62329 --- /dev/null +++ b/packages/auth/src/Infra/TypeORM/TypeORMEphemeralSessionRepository.spec.ts @@ -0,0 +1,57 @@ +import { CacheEntry, CacheEntryRepositoryInterface } from '@standardnotes/domain-core' +import { TimerInterface } from '@standardnotes/time' + +import { EphemeralSession } from '../../Domain/Session/EphemeralSession' +import { TypeORMEphemeralSessionRepository } from './TypeORMEphemeralSessionRepository' + +describe('TypeORMEphemeralSessionRepository', () => { + let cacheEntryRepository: jest.Mocked + let timer: jest.Mocked + let repository: TypeORMEphemeralSessionRepository + + beforeEach(() => { + cacheEntryRepository = { + findUnexpiredOneByKey: jest.fn(), + removeByKey: jest.fn(), + save: jest.fn(), + } as unknown as jest.Mocked + + timer = {} as jest.Mocked + + repository = new TypeORMEphemeralSessionRepository(cacheEntryRepository, 3600, timer) + }) + + describe('deleteOne', () => { + it('should delete private identifier mapping when revoking a session', async () => { + const session = { + uuid: 'session-uuid', + userUuid: 'user-uuid', + privateIdentifier: 'private-id', + } as EphemeralSession + + cacheEntryRepository.findUnexpiredOneByKey.mockResolvedValueOnce( + CacheEntry.create({ + key: 'session:session-uuid:user-uuid', + value: JSON.stringify(session), + expiresAt: new Date(), + }).getValue(), + ) + + await repository.deleteOne('session-uuid', 'user-uuid') + + expect(cacheEntryRepository.removeByKey).toHaveBeenCalledWith('session:session-uuid') + expect(cacheEntryRepository.removeByKey).toHaveBeenCalledWith('session:session-uuid:user-uuid') + expect(cacheEntryRepository.removeByKey).toHaveBeenCalledWith('session-private-id:private-id') + }) + + it('should skip private identifier deletion when session is not found', async () => { + cacheEntryRepository.findUnexpiredOneByKey.mockResolvedValueOnce(null) + + await repository.deleteOne('session-uuid', 'user-uuid') + + expect(cacheEntryRepository.removeByKey).toHaveBeenCalledWith('session:session-uuid') + expect(cacheEntryRepository.removeByKey).toHaveBeenCalledWith('session:session-uuid:user-uuid') + expect(cacheEntryRepository.removeByKey).not.toHaveBeenCalledWith(expect.stringContaining('session-private-id')) + }) + }) +}) diff --git a/packages/auth/src/Infra/TypeORM/TypeORMEphemeralSessionRepository.ts b/packages/auth/src/Infra/TypeORM/TypeORMEphemeralSessionRepository.ts index 76d30392a..f73e0d8b0 100644 --- a/packages/auth/src/Infra/TypeORM/TypeORMEphemeralSessionRepository.ts +++ b/packages/auth/src/Infra/TypeORM/TypeORMEphemeralSessionRepository.ts @@ -27,9 +27,15 @@ export class TypeORMEphemeralSessionRepository implements EphemeralSessionReposi } async deleteOne(uuid: string, userUuid: string): Promise { + const session = await this.findOneByUuidAndUserUuid(uuid, userUuid) + await this.cacheEntryRepository.removeByKey(`${this.PREFIX}:${uuid}`) await this.cacheEntryRepository.removeByKey(`${this.PREFIX}:${uuid}:${userUuid}`) + if (session?.privateIdentifier) { + await this.cacheEntryRepository.removeByKey(`${this.PREFIX_PRIVATE_ID}:${session.privateIdentifier}`) + } + const userSessionsJSON = await this.cacheEntryRepository.findUnexpiredOneByKey( `${this.USER_SESSIONS_PREFIX}:${userUuid}`, ) From 3ca680ce4e60bda4d81d51e7c304f3327f0df420 Mon Sep 17 00:00:00 2001 From: Antonella Sgarlatta Date: Tue, 8 Sep 2026 18:15:54 +0000 Subject: [PATCH 16/17] fix: Binds PKCE challenge to user uuid --- packages/auth/src/Bootstrap/Container.ts | 1 + .../GetUserKeyParams/GetUserKeyParams.spec.ts | 4 ++-- .../GetUserKeyParams/GetUserKeyParams.ts | 2 +- .../GetUserKeyParams.spec.ts | 3 ++- .../GetUserKeyParamsRecovery.ts | 2 +- .../auth/src/Domain/UseCase/SignIn.spec.ts | 22 ++++++++++++++++++ packages/auth/src/Domain/UseCase/SignIn.ts | 21 +++++++++-------- .../SignInWithRecoveryCodes.spec.ts | 8 ++++++- .../SignInWithRecoveryCodes.ts | 23 +++++++++++++------ .../Domain/User/PKCERepositoryInterface.ts | 4 ++-- .../src/Infra/Redis/RedisPKCERepository.ts | 16 +++++++++---- .../Infra/TypeORM/TypeORMPKCERepository.ts | 17 ++++++++++---- 12 files changed, 91 insertions(+), 32 deletions(-) diff --git a/packages/auth/src/Bootstrap/Container.ts b/packages/auth/src/Bootstrap/Container.ts index cd676b230..3c897ef52 100644 --- a/packages/auth/src/Bootstrap/Container.ts +++ b/packages/auth/src/Bootstrap/Container.ts @@ -1347,6 +1347,7 @@ export class ContainerConfigLoader { container.get(TYPES.Auth_MAX_LOGIN_ATTEMPTS), container.get(TYPES.Auth_LockRepository), container.get(TYPES.Auth_VerifyHumanInteraction), + container.get(TYPES.Auth_Logger), ), ) container diff --git a/packages/auth/src/Domain/UseCase/GetUserKeyParams/GetUserKeyParams.spec.ts b/packages/auth/src/Domain/UseCase/GetUserKeyParams/GetUserKeyParams.spec.ts index e4a208c70..5b8f4304f 100644 --- a/packages/auth/src/Domain/UseCase/GetUserKeyParams/GetUserKeyParams.spec.ts +++ b/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 = { uuid: '1-2-3' } as jest.Mocked userRepository = {} as jest.Mocked 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 () => { diff --git a/packages/auth/src/Domain/UseCase/GetUserKeyParams/GetUserKeyParams.ts b/packages/auth/src/Domain/UseCase/GetUserKeyParams/GetUserKeyParams.ts index 81f708857..e7e850c10 100644 --- a/packages/auth/src/Domain/UseCase/GetUserKeyParams/GetUserKeyParams.ts +++ b/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 { if (this.isCodeChallengedVersion(dto)) { - await this.pkceRepository.storeCodeChallenge(dto.codeChallenge) + await this.pkceRepository.storeCodeChallenge(dto.codeChallenge, user.uuid) } return this.keyParamsFactory.create(user, authenticated) diff --git a/packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParams.spec.ts b/packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParams.spec.ts index ba887f5dc..251254030 100644 --- a/packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParams.spec.ts +++ b/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 = { uuid: 'user-uuid' } as jest.Mocked userRepository = {} as jest.Mocked 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) }) diff --git a/packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParamsRecovery.ts b/packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParamsRecovery.ts index 8df307292..84aa14af7 100644 --- a/packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParamsRecovery.ts +++ b/packages/auth/src/Domain/UseCase/GetUserKeyParamsRecovery/GetUserKeyParamsRecovery.ts @@ -70,7 +70,7 @@ export class GetUserKeyParamsRecovery implements UseCaseInterface } private async createKeyParams(codeChallenge: string, user: User): Promise { - await this.pkceRepository.storeCodeChallenge(codeChallenge) + await this.pkceRepository.storeCodeChallenge(codeChallenge, user.uuid) return this.keyParamsFactory.create(user, false) } diff --git a/packages/auth/src/Domain/UseCase/SignIn.spec.ts b/packages/auth/src/Domain/UseCase/SignIn.spec.ts index 43b8e23b4..6cfb276b1 100644 --- a/packages/auth/src/Domain/UseCase/SignIn.spec.ts +++ b/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: 'test@test.te' }) }) + 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: 'test@test.te', + 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') diff --git a/packages/auth/src/Domain/UseCase/SignIn.ts b/packages/auth/src/Domain/UseCase/SignIn.ts index 31a8e7ac8..d9cd6a040 100644 --- a/packages/auth/src/Domain/UseCase/SignIn.ts +++ b/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 { + private async validateCodeVerifier(codeVerifier: string, userUuid: string): Promise { 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 } diff --git a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.spec.ts b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.spec.ts index 7fc7e64bf..854426371 100644 --- a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.spec.ts +++ b/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 verifyHumanInteractionUseCase.execute = jest.fn().mockReturnValue(Result.ok()) + + logger = {} as jest.Mocked + 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, }) }) diff --git a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.ts b/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.ts index 5e62c2931..a17bcadb4 100644 --- a/packages/auth/src/Domain/UseCase/SignInWithRecoveryCodes/SignInWithRecoveryCodes.ts +++ b/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 { @@ -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 { + private async validateCodeVerifier(codeVerifier: string, userUuid: string): Promise { 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 } diff --git a/packages/auth/src/Domain/User/PKCERepositoryInterface.ts b/packages/auth/src/Domain/User/PKCERepositoryInterface.ts index 3665cdc5e..cf52ca54e 100644 --- a/packages/auth/src/Domain/User/PKCERepositoryInterface.ts +++ b/packages/auth/src/Domain/User/PKCERepositoryInterface.ts @@ -1,4 +1,4 @@ export interface PKCERepositoryInterface { - storeCodeChallenge(codeChallenge: string): Promise - removeCodeChallenge(codeChallenge: string): Promise + storeCodeChallenge(codeChallenge: string, userUuid: string): Promise + removeCodeChallenge(codeChallenge: string, userUuid: string): Promise } diff --git a/packages/auth/src/Infra/Redis/RedisPKCERepository.ts b/packages/auth/src/Infra/Redis/RedisPKCERepository.ts index 5666c434c..a76880e09 100644 --- a/packages/auth/src/Infra/Redis/RedisPKCERepository.ts +++ b/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 { + async storeCodeChallenge(codeChallenge: string, userUuid: string): Promise { 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 { - const entriesRemoved = await this.redisClient.del(`${this.PREFIX}:${codeChallenge}`) + async removeCodeChallenge(codeChallenge: string, userUuid: string): Promise { + 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}`) diff --git a/packages/auth/src/Infra/TypeORM/TypeORMPKCERepository.ts b/packages/auth/src/Infra/TypeORM/TypeORMPKCERepository.ts index 4f2131726..21497a69b 100644 --- a/packages/auth/src/Infra/TypeORM/TypeORMPKCERepository.ts +++ b/packages/auth/src/Infra/TypeORM/TypeORMPKCERepository.ts @@ -13,20 +13,29 @@ export class TypeORMPKCERepository implements PKCERepositoryInterface { private timer: TimerInterface, ) {} - async storeCodeChallenge(codeChallenge: string): Promise { + async storeCodeChallenge(codeChallenge: string, userUuid: string): Promise { 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 { - await this.cacheEntryRepository.removeByKey(`${this.PREFIX}:${codeChallenge}`) + async removeCodeChallenge(codeChallenge: string, userUuid: string): Promise { + 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 } From c879a0ebb0c4ec4a34e255ba81c5ee93c769dca0 Mon Sep 17 00:00:00 2001 From: Antonella Sgarlatta Date: Tue, 8 Sep 2026 18:22:36 +0000 Subject: [PATCH 17/17] fix: Prevents self invites to subscriptions --- ...AcceptSharedSubscriptionInvitation.spec.ts | 29 ++++++++++++++--- .../AcceptSharedSubscriptionInvitation.ts | 7 ++++ .../InviteToSharedSubscription.spec.ts | 32 +++++++++++++++++++ .../InviteToSharedSubscription.ts | 28 +++++++++++++--- 4 files changed, 86 insertions(+), 10 deletions(-) diff --git a/packages/auth/src/Domain/UseCase/AcceptSharedSubscriptionInvitation/AcceptSharedSubscriptionInvitation.spec.ts b/packages/auth/src/Domain/UseCase/AcceptSharedSubscriptionInvitation/AcceptSharedSubscriptionInvitation.spec.ts index a8df4f600..ae5ee3302 100644 --- a/packages/auth/src/Domain/UseCase/AcceptSharedSubscriptionInvitation/AcceptSharedSubscriptionInvitation.spec.ts +++ b/packages/auth/src/Domain/UseCase/AcceptSharedSubscriptionInvitation/AcceptSharedSubscriptionInvitation.spec.ts @@ -65,7 +65,7 @@ describe('AcceptSharedSubscriptionInvitation', () => { inviteeSubscription = { endsAt: 3, planName: SubscriptionName.PlusPlan } as jest.Mocked - inviterSubscription = { endsAt: 3, planName: SubscriptionName.PlusPlan } as jest.Mocked + inviterSubscription = { endsAt: 3, planName: SubscriptionName.PlusPlan, userUuid: '456' } as jest.Mocked userSubscriptionRepository = {} as jest.Mocked userSubscriptionRepository.findBySubscriptionIdAndType = jest.fn().mockReturnValue([inviterSubscription]) @@ -114,8 +114,8 @@ describe('AcceptSharedSubscriptionInvitation', () => { }) it('should create a shared subscription upon accepting the invitation if inviter has a second subscription', async () => { - const inviterSubscription1 = { endsAt: 1, planName: SubscriptionName.PlusPlan } as jest.Mocked - const inviterSubscription2 = { endsAt: 5, planName: SubscriptionName.PlusPlan } as jest.Mocked + const inviterSubscription1 = { endsAt: 1, planName: SubscriptionName.PlusPlan, userUuid: '456' } as jest.Mocked + const inviterSubscription2 = { endsAt: 5, planName: SubscriptionName.PlusPlan, userUuid: '456' } as jest.Mocked timer.getTimestampInMicroseconds = jest.fn().mockReturnValue(3) @@ -208,6 +208,25 @@ describe('AcceptSharedSubscriptionInvitation', () => { expect(applyDefaultSubscriptionSettings.execute).not.toHaveBeenCalled() }) + it('should not create a shared subscription if invitee is the inviter', async () => { + inviterSubscription = { endsAt: 3, planName: SubscriptionName.PlusPlan, userUuid: '123' } as jest.Mocked + userSubscriptionRepository.findBySubscriptionIdAndType = jest.fn().mockReturnValue([inviterSubscription]) + + expect( + await createUseCase().execute({ + sharedSubscriptionInvitationUuid: '1-2-3', + }), + ).toEqual({ + success: false, + message: 'You cannot accept a subscription invitation sent to yourself.', + }) + + expect(sharedSubscriptionInvitationRepository.save).not.toHaveBeenCalled() + expect(userSubscriptionRepository.save).not.toHaveBeenCalled() + expect(roleService.addUserRoleBasedOnSubscription).not.toHaveBeenCalled() + expect(applyDefaultSubscriptionSettings.execute).not.toHaveBeenCalled() + }) + it('should not create a shared subscription if inviter subscription is not found', async () => { userSubscriptionRepository.findBySubscriptionIdAndType = jest.fn().mockReturnValue([]) expect( @@ -226,8 +245,8 @@ describe('AcceptSharedSubscriptionInvitation', () => { }) it('should not create a shared subscription if inviter subscriptions are not active', async () => { - const inviterSubscription1 = { endsAt: 1, planName: SubscriptionName.PlusPlan } as jest.Mocked - const inviterSubscription2 = { endsAt: 2, planName: SubscriptionName.PlusPlan } as jest.Mocked + const inviterSubscription1 = { endsAt: 1, planName: SubscriptionName.PlusPlan, userUuid: '456' } as jest.Mocked + const inviterSubscription2 = { endsAt: 2, planName: SubscriptionName.PlusPlan, userUuid: '456' } as jest.Mocked timer.getTimestampInMicroseconds = jest.fn().mockReturnValue(3) diff --git a/packages/auth/src/Domain/UseCase/AcceptSharedSubscriptionInvitation/AcceptSharedSubscriptionInvitation.ts b/packages/auth/src/Domain/UseCase/AcceptSharedSubscriptionInvitation/AcceptSharedSubscriptionInvitation.ts index 70af840e5..a4ac05a0d 100644 --- a/packages/auth/src/Domain/UseCase/AcceptSharedSubscriptionInvitation/AcceptSharedSubscriptionInvitation.ts +++ b/packages/auth/src/Domain/UseCase/AcceptSharedSubscriptionInvitation/AcceptSharedSubscriptionInvitation.ts @@ -74,6 +74,13 @@ export class AcceptSharedSubscriptionInvitation implements UseCaseInterface { } const inviterUserSubscription = activeUserSubscriptions[0] + if (invitee.uuid === inviterUserSubscription.userUuid) { + return { + success: false, + message: 'You cannot accept a subscription invitation sent to yourself.', + } + } + sharedSubscriptionInvitation.status = InvitationStatus.Accepted sharedSubscriptionInvitation.updatedAt = this.timer.getTimestampInMicroseconds() diff --git a/packages/auth/src/Domain/UseCase/InviteToSharedSubscription/InviteToSharedSubscription.spec.ts b/packages/auth/src/Domain/UseCase/InviteToSharedSubscription/InviteToSharedSubscription.spec.ts index e97871cf5..4ba150f48 100644 --- a/packages/auth/src/Domain/UseCase/InviteToSharedSubscription/InviteToSharedSubscription.spec.ts +++ b/packages/auth/src/Domain/UseCase/InviteToSharedSubscription/InviteToSharedSubscription.spec.ts @@ -58,6 +58,38 @@ describe('InviteToSharedSubscription', () => { domainEventFactory.createEmailRequestedEvent = jest.fn().mockReturnValue({} as jest.Mocked) }) + it('should not create an invitation if user invites themselves', async () => { + expect( + await createUseCase().execute({ + inviteeIdentifier: 'inviter@test.te', + inviterUuid: '1-2-3', + inviterEmail: 'inviter@test.te', + inviterRoles: [RoleName.NAMES.ProUser], + }), + ).toEqual({ + success: false, + }) + + expect(sharedSubscriptionInvitationRepository.save).not.toHaveBeenCalled() + expect(domainEventFactory.createSharedSubscriptionInvitationCreatedEvent).not.toHaveBeenCalled() + expect(domainEventPublisher.publish).not.toHaveBeenCalled() + }) + + it('should not create an inivitation if user invites themselves with different email casing', async () => { + expect( + await createUseCase().execute({ + inviteeIdentifier: 'Inviter@Test.TE', + inviterUuid: '1-2-3', + inviterEmail: 'inviter@test.te', + inviterRoles: [RoleName.NAMES.ProUser], + }), + ).toEqual({ + success: false, + }) + + expect(sharedSubscriptionInvitationRepository.save).not.toHaveBeenCalled() + }) + it('should not create an inivitation for sharing the subscription if inviter has no subscription', async () => { userSubscriptionRepository.findOneByUserUuid = jest.fn().mockReturnValue(null) diff --git a/packages/auth/src/Domain/UseCase/InviteToSharedSubscription/InviteToSharedSubscription.ts b/packages/auth/src/Domain/UseCase/InviteToSharedSubscription/InviteToSharedSubscription.ts index 77c8edbc6..2952e3632 100644 --- a/packages/auth/src/Domain/UseCase/InviteToSharedSubscription/InviteToSharedSubscription.ts +++ b/packages/auth/src/Domain/UseCase/InviteToSharedSubscription/InviteToSharedSubscription.ts @@ -45,6 +45,16 @@ export class InviteToSharedSubscription implements UseCaseInterface { } } + const inviteeIdentifierType = this.isInviteeIdentifierPotentiallyAPrivateUsernameAccount(dto.inviteeIdentifier) + ? InviteeIdentifierType.Hash + : InviteeIdentifierType.Email + + if (this.isSelfInvite(dto.inviteeIdentifier, dto.inviterEmail, inviteeIdentifierType)) { + return { + success: false, + } + } + const numberOfUsedInvites = await this.sharedSubscriptionInvitationRepository.countByInviterEmailAndStatus( dto.inviterEmail, [InvitationStatus.Sent, InvitationStatus.Accepted], @@ -69,11 +79,7 @@ export class InviteToSharedSubscription implements UseCaseInterface { sharedSubscriptionInvition.inviterIdentifier = dto.inviterEmail sharedSubscriptionInvition.inviterIdentifierType = InviterIdentifierType.Email sharedSubscriptionInvition.inviteeIdentifier = dto.inviteeIdentifier - sharedSubscriptionInvition.inviteeIdentifierType = this.isInviteeIdentifierPotentiallyAPrivateUsernameAccount( - dto.inviteeIdentifier, - ) - ? InviteeIdentifierType.Hash - : InviteeIdentifierType.Email + sharedSubscriptionInvition.inviteeIdentifierType = inviteeIdentifierType sharedSubscriptionInvition.status = InvitationStatus.Sent sharedSubscriptionInvition.subscriptionId = inviterUserSubscription.subscriptionId as number sharedSubscriptionInvition.createdAt = this.timer.getTimestampInMicroseconds() @@ -110,4 +116,16 @@ export class InviteToSharedSubscription implements UseCaseInterface { private isInviteeIdentifierPotentiallyAPrivateUsernameAccount(identifier: string): boolean { return identifier.length === 64 && !identifier.includes('@') } + + private isSelfInvite( + inviteeIdentifier: string, + inviterEmail: string, + inviteeIdentifierType: InviteeIdentifierType, + ): boolean { + if (inviteeIdentifierType !== InviteeIdentifierType.Email) { + return false + } + + return inviteeIdentifier.trim().toLowerCase() === inviterEmail.trim().toLowerCase() + } }