Skip to content

[PB-4737] feat(user): add public keys validation for account recovery #851

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
douglas-xt wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[PB-4737] feat(user): add public keys validation for account recovery #851

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c635ea3
feat(user): add public keys validation for account recovery
douglas-xt Jan 7, 2026
4880def
fix(user): address PR review comments for public keys validation
douglas-xt Jan 8, 2026
b3b4406
fix(user): simplify public keys validation
douglas-xt Jan 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/modules/user/dto/recover-account.dto.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ import {
IsEmail,
IsNotEmpty,
IsOptional,
IsString,
ValidateNested,
} from 'class-validator';
import { Type } from 'class-transformer';

class PrivateKeysDto {
@ApiProperty()
Expand All @@ -14,6 +16,19 @@ class PrivateKeysDto {
@ApiProperty()
kyber: string;
}

class PublicKeysDto {
@ApiProperty()
@IsString()
@IsNotEmpty()
ecc: string;

@ApiProperty()
@IsString()
@IsNotEmpty()
kyber: string;
}
Copy link
Member

@sg-gs sg-gs Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a line between this class and the next one

Copy link
Contributor

@TamaraFinogina TamaraFinogina Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why in this class ecc and kyber are optional? Isn't it enough just to have optional public keys field in RecoverAccountDto? Because now we can have {ecc: undefined, kyber: underfined}, {ecc: , kyber: underfined}, {ecc: undefined, kyber: } and I don't think we should accept those as valid input


export class RecoverAccountDto {
@ApiProperty({
example: 'some_hashed_pass',
Expand Down Expand Up @@ -53,6 +68,12 @@ export class RecoverAccountDto {
@IsOptional()
@ValidateNested()
privateKeys?: PrivateKeysDto;

@ApiProperty()
@IsOptional()
@ValidateNested()
@Type(() => PublicKeysDto)
publicKeys?: PublicKeysDto;
}

export class DeprecatedRecoverAccountDto {
Expand Down
4 changes: 4 additions & 0 deletions src/modules/user/user.controller.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1348,6 +1348,7 @@ describe('User Controller', () => {
mnemonic: mockRecoverAccountNoKeys.mnemonic,
password: mockRecoverAccountNoKeys.password,
salt: mockRecoverAccountNoKeys.salt,
publicKeys: undefined,
},
true,
);
Expand All @@ -1369,6 +1370,7 @@ describe('User Controller', () => {
password: mockRecoverAccountDto.password,
salt: mockRecoverAccountDto.salt,
privateKeys: mockRecoverAccountDto.privateKeys,
publicKeys: undefined,
},
false,
);
Expand Down Expand Up @@ -1404,6 +1406,7 @@ describe('User Controller', () => {
password: mockRecoverAccountDto.password,
salt: mockRecoverAccountDto.salt,
privateKeys: mockRecoverAccountDto.privateKeys,
publicKeys: undefined,
},
false,
);
Expand Down Expand Up @@ -1437,6 +1440,7 @@ describe('User Controller', () => {
password: mockRecoverAccountDto.password,
salt: mockRecoverAccountDto.salt,
privateKeys: mockRecoverAccountDto.privateKeys,
publicKeys: undefined,
},
false,
);
Expand Down
1 change: 1 addition & 0 deletions src/modules/user/user.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -884,6 +884,7 @@ export class UserController {
...(shouldResetAccount
? undefined
: { privateKeys: body.privateKeys }),
publicKeys: body.publicKeys,
},
shouldResetAccount,
);
Expand Down
123 changes: 123 additions & 0 deletions src/modules/user/user.usecase.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2658,6 +2658,10 @@ describe('User use cases', () => {

describe('updateCredentials', () => {
const mockUser = newUser();
const mockPublicKeys = {
ecc: 'existing_ecc_public_key',
kyber: 'existing_kyber_public_key',
};
const mockCredentials = {
mnemonic: 'encrypted_mnemonic',
password: 'encrypted_password',
Expand All @@ -2666,13 +2670,17 @@ describe('User use cases', () => {
ecc: 'encrypted_ecc_key',
kyber: 'encrypted_kyber_key',
},
publicKeys: mockPublicKeys,
};

const decryptedPassword = 'decrypted_password';
const decryptedSalt = 'decrypted_salt';

beforeEach(() => {
jest.clearAllMocks();
jest
.spyOn(keyServerUseCases, 'getPublicKeys')
.mockResolvedValue(mockPublicKeys);
jest.spyOn(cryptoService, 'decryptText').mockImplementation((text) => {
if (text === mockCredentials.password) return decryptedPassword;
if (text === mockCredentials.salt) return decryptedSalt;
Expand Down Expand Up @@ -2743,6 +2751,113 @@ describe('User use cases', () => {
),
).rejects.toThrow(BadRequestException);
});

it('When publicKeys are provided and match existing keys, then it should succeed', async () => {
const existingPublicKeys = {
ecc: 'existing_ecc_public_key',
kyber: 'existing_kyber_public_key',
};

jest
.spyOn(keyServerUseCases, 'getPublicKeys')
.mockResolvedValue(existingPublicKeys);

const credentialsWithPublicKeys = {
...mockCredentials,
publicKeys: existingPublicKeys,
};

await userUseCases.updateCredentials(
mockUser.uuid,
credentialsWithPublicKeys,
);

expect(keyServerUseCases.getPublicKeys).toHaveBeenCalledWith(mockUser.id);
expect(userRepository.updateByUuid).toHaveBeenCalled();
});

it('When publicKeys.ecc does not match existing key, then it should throw', async () => {
const existingPublicKeys = {
ecc: 'existing_ecc_public_key',
kyber: 'existing_kyber_public_key',
};

jest
.spyOn(keyServerUseCases, 'getPublicKeys')
.mockResolvedValue(existingPublicKeys);

const credentialsWithWrongEcc = {
...mockCredentials,
publicKeys: {
ecc: 'wrong_ecc_public_key',
kyber: 'existing_kyber_public_key',
},
};

await expect(
userUseCases.updateCredentials(mockUser.uuid, credentialsWithWrongEcc),
).rejects.toThrow(BadRequestException);
});

it('When publicKeys.kyber does not match existing key, then it should throw', async () => {
const existingPublicKeys = {
ecc: 'existing_ecc_public_key',
kyber: 'existing_kyber_public_key',
};

jest
.spyOn(keyServerUseCases, 'getPublicKeys')
.mockResolvedValue(existingPublicKeys);

const credentialsWithWrongKyber = {
...mockCredentials,
publicKeys: {
ecc: 'existing_ecc_public_key',
kyber: 'wrong_kyber_public_key',
},
};

await expect(
userUseCases.updateCredentials(
mockUser.uuid,
credentialsWithWrongKyber,
),
).rejects.toThrow(BadRequestException);
});

it('When publicKeys are not provided and not resetting, then it should throw', async () => {
const credentialsWithoutPublicKeys = {
mnemonic: mockCredentials.mnemonic,
password: mockCredentials.password,
salt: mockCredentials.salt,
privateKeys: mockCredentials.privateKeys,
};

await expect(
userUseCases.updateCredentials(
mockUser.uuid,
credentialsWithoutPublicKeys,
),
).rejects.toThrow(BadRequestException);
});

it('When publicKeys are not provided but resetting account, then it should succeed', async () => {
jest.spyOn(keyServerUseCases, 'getPublicKeys');

const credentialsWithoutPublicKeys = {
mnemonic: mockCredentials.mnemonic,
password: mockCredentials.password,
salt: mockCredentials.salt,
};

await userUseCases.updateCredentials(
mockUser.uuid,
credentialsWithoutPublicKeys,
true,
);

expect(keyServerUseCases.getPublicKeys).not.toHaveBeenCalled();
});
});

describe('updateCredentialsOld', () => {
Expand Down Expand Up @@ -3461,6 +3576,10 @@ describe('User use cases', () => {

describe('updateCredentials', () => {
const mockUser = newUser();
const mockPublicKeys = {
ecc: 'existing_ecc_public_key',
kyber: 'existing_kyber_public_key',
};
const mockCredentials = {
mnemonic: 'encrypted_mnemonic',
password: 'encrypted_password',
Expand All @@ -3469,13 +3588,17 @@ describe('User use cases', () => {
ecc: 'encrypted_ecc_key',
kyber: 'encrypted_kyber_key',
},
publicKeys: mockPublicKeys,
};

const decryptedPassword = 'decrypted_password';
const decryptedSalt = 'decrypted_salt';

beforeEach(() => {
jest.clearAllMocks();
jest
.spyOn(keyServerUseCases, 'getPublicKeys')
.mockResolvedValue(mockPublicKeys);
jest.spyOn(cryptoService, 'decryptText').mockImplementation((text) => {
if (text === mockCredentials.password) return decryptedPassword;
if (text === mockCredentials.salt) return decryptedSalt;
Expand Down
25 changes: 24 additions & 1 deletion src/modules/user/user.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1075,10 +1075,15 @@ export class UserUseCases {
ecc: string;
kyber: string;
};
publicKeys?: {
ecc?: string;
kyber?: string;
};
},
withReset = false,
): Promise<void> {
const { mnemonic, password, salt, privateKeys } = newCredentials;
const { mnemonic, password, salt, privateKeys, publicKeys } =
newCredentials;

const shouldUpdateKeys = privateKeys && Object.keys(privateKeys).length > 0;

Expand All @@ -1088,8 +1093,26 @@ export class UserUseCases {
);
}

if (!withReset && !publicKeys) {
throw new BadRequestException('Invalid keys');
}

const user = await this.userRepository.findByUuid(userUuid);

if (publicKeys) {
Copy link
Contributor

@TamaraFinogina TamaraFinogina Jan 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if withReset = true and mismatching public keys, then backup won't go through?

Copy link
Contributor

@TamaraFinogina TamaraFinogina Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sg-gs Do we require/can send publicKeys for account reset? Becuase now we check public keys and if they fail we abort even if it's withReset=true

const existingKeys = await this.keyServerUseCases.getPublicKeys(user.id);
const eccMismatch = publicKeys.ecc && existingKeys.ecc !== publicKeys.ecc;
Copy link
Contributor

@TamaraFinogina TamaraFinogina Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we send publicKeys = {ecc: underfined, kyber: underfined}, then publicKeys.ecc would be false, right? so, the eccMismatch will be false without even checking existingKeys.ecc !== publicKeys.ecc; part, no?

I think we shouldn't accept those keys. Or at least use something like
! publicKeys.ecc || existingKeys.ecc !== publicKeys.ecc

then undefined, null or similar keeys will give a mismatch

const kyberMismatch =
publicKeys.kyber && existingKeys.kyber !== publicKeys.kyber;
Copy link
Contributor

@TamaraFinogina TamaraFinogina Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@douglas-xt This check would pass for publicKeys = {ecc: underfined, kyber: underfined} without even checking what existingKeys are. I know that now they are not empty, but if we change that in the future this line becomes an issue. Can we remove publicKeys.kyber && and just do the comparison?


if (eccMismatch) {
throw new BadRequestException('Invalid ECC public key');
}
if (kyberMismatch) {
throw new BadRequestException('Invalid Kyber public key');
}
}

if (shouldUpdateKeys) {
for (const [version, privateKey] of Object.entries(privateKeys)) {
await this.keyServerUseCases.updateByUserAndEncryptVersion(
Expand Down
Loading