Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,11 @@ afterAll(async () => {
describe('PasskeyChallengeManager (integration)', () => {
describe('generateRegistrationChallenge', () => {
it('stores challenge with uid and type=registration', async () => {
const challenge =
await manager.generateRegistrationChallenge('deadbeef');
const challenge = await manager.generateRegistrationChallenge('deadbeef');

const stored = await manager.consumeRegistrationChallenge(
'deadbeef',
challenge
challenge,
'deadbeef'
);

expect(stored?.challenge).toBe(challenge);
Expand All @@ -87,29 +86,24 @@ describe('PasskeyChallengeManager (integration)', () => {
});

describe('generateAuthenticationChallenge', () => {
it('stores challenge with uid and type=authentication', async () => {
const challenge =
await manager.generateAuthenticationChallenge('deadbeef');
it('stores challenge with no uid and type=authentication', async () => {
const challenge = await manager.generateAuthenticationChallenge();

const stored = await manager.consumeAuthenticationChallenge(
'deadbeef',
challenge
);
const stored = await manager.consumeAuthenticationChallenge(challenge);

expect(stored?.challenge).toBe(challenge);
expect(stored?.type).toBe('authentication');
expect(stored?.uid).toBe('deadbeef');
expect(stored?.uid).toBeUndefined();
});
});

describe('generateUpgradeChallenge', () => {
it('stores challenge with uid and type=upgrade', async () => {
const challenge =
await manager.generateUpgradeChallenge('cafebabe');
const challenge = await manager.generateUpgradeChallenge('cafebabe');

const stored = await manager.consumeUpgradeChallenge(
'cafebabe',
challenge
challenge,
'cafebabe'
);

expect(stored?.challenge).toBe(challenge);
Expand All @@ -120,22 +114,21 @@ describe('PasskeyChallengeManager (integration)', () => {

describe('consumeChallenge', () => {
it('returns null on second consume (single-use enforcement)', async () => {
const challenge =
await manager.generateRegistrationChallenge('deadbeef');
const challenge = await manager.generateRegistrationChallenge('deadbeef');

await manager.consumeRegistrationChallenge('deadbeef', challenge);
await manager.consumeRegistrationChallenge(challenge, 'deadbeef');

const secondAttempt = await manager.consumeRegistrationChallenge(
'deadbeef',
challenge
challenge,
'deadbeef'
);
expect(secondAttempt).toBeNull();
});

it('returns null for an unknown challenge', async () => {
const result = await manager.consumeRegistrationChallenge(
'deadbeef',
'nonexistent-challenge'
'nonexistent-challenge',
'deadbeef'
);
expect(result).toBeNull();
});
Expand All @@ -158,32 +151,31 @@ describe('PasskeyChallengeManager (integration)', () => {
await new Promise((resolve) => setTimeout(resolve, 1100));

const result = await shortManager.consumeRegistrationChallenge(
'deadbeef',
challenge
challenge,
'deadbeef'
);
expect(result).toBeNull();
}, 5_000);
});

describe('deleteChallenge', () => {
it('removes the key from Redis', async () => {
const challenge =
await manager.generateRegistrationChallenge('deadbeef');
const challenge = await manager.generateRegistrationChallenge('deadbeef');

await manager.deleteChallenge(challenge, 'registration', 'deadbeef');
await manager.deleteChallenge('registration', challenge, 'deadbeef');

const result = await manager.consumeRegistrationChallenge(
'deadbeef',
challenge
challenge,
'deadbeef'
);

expect(result).toBeNull();
});

it('does not throw when the key does not exist', async () => {
const result = await manager.deleteChallenge(
'nonexistent-challenge',
'registration',
'nonexistent-challenge',
'deadbeef'
);
expect(result).toBeUndefined();
Expand Down
42 changes: 17 additions & 25 deletions libs/accounts/passkey/src/lib/passkey.challenge.manager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ describe('PasskeyChallengeManager', () => {
await manager.generateRegistrationChallenge('deadbeef');

expect(mockRedis.set).toHaveBeenCalledWith(
`passkey:challenge:deadbeef:registration:${MOCK_CHALLENGE}`,
`passkey:challenge:registration:${MOCK_CHALLENGE}:deadbeef`,
expect.any(String),
'EX',
CHALLENGE_TIMEOUT_MS / 1000
Expand Down Expand Up @@ -109,18 +109,16 @@ describe('PasskeyChallengeManager', () => {
});

describe('generateAuthenticationChallenge', () => {
it('stores the challenge with type=authentication and uid', async () => {
it('stores the challenge with type=authentication and no uid', async () => {
mockRedis.set.mockResolvedValue('OK');
await manager.generateAuthenticationChallenge('deadbeef');
await manager.generateAuthenticationChallenge();

const [key, rawJson] = mockRedis.set.mock.calls[0];
const stored: StoredChallenge = JSON.parse(rawJson);

expect(key).toBe(
`passkey:challenge:deadbeef:authentication:${MOCK_CHALLENGE}`
);
expect(key).toBe(`passkey:challenge:authentication:${MOCK_CHALLENGE}`);
expect(stored.type).toBe('authentication');
expect(stored.uid).toBe('deadbeef');
expect(stored.uid).toBeUndefined();
});
});

Expand All @@ -132,9 +130,7 @@ describe('PasskeyChallengeManager', () => {
const [key, rawJson] = mockRedis.set.mock.calls[0];
const stored: StoredChallenge = JSON.parse(rawJson);

expect(key).toBe(
`passkey:challenge:cafebabe:upgrade:${MOCK_CHALLENGE}`
);
expect(key).toBe(`passkey:challenge:upgrade:${MOCK_CHALLENGE}:cafebabe`);
expect(stored.type).toBe('upgrade');
expect(stored.uid).toBe('cafebabe');
});
Expand Down Expand Up @@ -170,8 +166,8 @@ describe('PasskeyChallengeManager', () => {
mockRedis.getdel.mockResolvedValue(JSON.stringify(stored));

const result = await manager.consumeRegistrationChallenge(
'deadbeef',
MOCK_CHALLENGE
MOCK_CHALLENGE,
'deadbeef'
);

expect(result).toEqual(stored);
Expand All @@ -185,8 +181,8 @@ describe('PasskeyChallengeManager', () => {
mockRedis.getdel.mockResolvedValue(null);

const result = await manager.consumeRegistrationChallenge(
'deadbeef',
MOCK_CHALLENGE
MOCK_CHALLENGE,
'deadbeef'
);

expect(result).toBeNull();
Expand All @@ -200,8 +196,8 @@ describe('PasskeyChallengeManager', () => {
mockRedis.getdel.mockResolvedValue('not a valid json string');

const result = await manager.consumeRegistrationChallenge(
'deadbeef',
MOCK_CHALLENGE
MOCK_CHALLENGE,
'deadbeef'
);

expect(result).toBeNull();
Expand All @@ -215,32 +211,28 @@ describe('PasskeyChallengeManager', () => {
const stored = makeStored({ type: 'authentication' });
mockRedis.getdel.mockResolvedValue(JSON.stringify(stored));

await manager.consumeAuthenticationChallenge('deadbeef', MOCK_CHALLENGE);
await manager.consumeAuthenticationChallenge(MOCK_CHALLENGE);

expect(mockRedis.getdel).toHaveBeenCalledWith(
`passkey:challenge:deadbeef:authentication:${MOCK_CHALLENGE}`
`passkey:challenge:authentication:${MOCK_CHALLENGE}`
);
});
});

describe('deleteChallenge', () => {
it('calls redis.del with the correct key', async () => {
mockRedis.del.mockResolvedValue(1);
await manager.deleteChallenge(
MOCK_CHALLENGE,
'registration',
'deadbeef'
);
await manager.deleteChallenge('registration', MOCK_CHALLENGE, 'deadbeef');

expect(mockRedis.del).toHaveBeenCalledWith(
`passkey:challenge:deadbeef:registration:${MOCK_CHALLENGE}`
`passkey:challenge:registration:${MOCK_CHALLENGE}:deadbeef`
);
});

it('does not throw when the key does not exist (del returns 0)', async () => {
mockRedis.del.mockResolvedValue(0);
await expect(
manager.deleteChallenge(MOCK_CHALLENGE, 'authentication', 'deadbeef')
manager.deleteChallenge('authentication', MOCK_CHALLENGE, 'deadbeef')
).resolves.toBeUndefined();
});
});
Expand Down
53 changes: 28 additions & 25 deletions libs/accounts/passkey/src/lib/passkey.challenge.manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export class PasskeyChallengeManager {
/**
* Generates a registration challenge for the WebAuthn attestation ceremony.
*
* @param input - Must include the hex-encoded uid of the user.
* @param uid - Hex-encoded uid of the user.
* @returns Base64url-encoded 32-byte challenge string.
*/
async generateRegistrationChallenge(uid: string): Promise<string> {
Expand All @@ -126,14 +126,14 @@ export class PasskeyChallengeManager {
*
* @returns Base64url-encoded 32-byte challenge string.
*/
async generateAuthenticationChallenge(uid: string): Promise<string> {
return this.generateChallenge('authentication', uid);
async generateAuthenticationChallenge(): Promise<string> {
return this.generateChallenge('authentication');
}

/**
* Generates an upgrade challenge for the WebAuthn PRF key-wrapping ceremony.
*
* @param input - Must include the hex-encoded uid of the user.
* @param uid - Hex-encoded uid of the user.
* @returns Base64url-encoded 32-byte challenge string.
*/
async generateUpgradeChallenge(uid: string): Promise<string> {
Expand All @@ -142,42 +142,42 @@ export class PasskeyChallengeManager {

/**
* Fetches and deletes a registration challenge from Redis.
* @param uid The hex-encoded uid of the user.
* @param challenge The base64url-encoded challenge string.
*
* @param challenge - The base64url-encoded challenge string.
* @param uid - Hex-encoded uid of the user.
* @returns The stored challenge metadata or null if not found or expired.
*/
async consumeRegistrationChallenge(
uid: string,
challenge: string
challenge: string,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any particular reason to swap this back?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, I can see why. Since the consumeAuthenticationChallenge() only takes the challenge string, now all functions have the same first parameter

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is to ensure that all methods have consistent parameter orders. I put uid at last because it can be optional in other methods.

uid: string
): Promise<StoredChallenge | null> {
return this.consumeChallenge(uid, 'registration', challenge);
return this.consumeChallenge('registration', challenge, uid);
}

/**
* Fetches and deletes an authentication challenge from Redis.
*
* @param uid The hex-encoded uid of the user.
* @param challenge The base64url-encoded challenge string.
* @returns The stored challenge metadata or null if not found or expired.
*/
async consumeAuthenticationChallenge(
uid: string,
challenge: string
): Promise<StoredChallenge | null> {
return this.consumeChallenge(uid, 'authentication', challenge);
return this.consumeChallenge('authentication', challenge);
}

/**
* Fetches and deletes an upgrade challenge from Redis.
* @param uid The hex-encoded uid of the user.
* @param challenge The base64url-encoded challenge string.
*
* @param challenge - The base64url-encoded challenge string.
* @param uid - Hex-encoded uid of the user.
* @returns The stored challenge metadata or null if not found or expired.
*/
async consumeUpgradeChallenge(
uid: string,
challenge: string
challenge: string,
uid: string
): Promise<StoredChallenge | null> {
return this.consumeChallenge(uid, 'upgrade', challenge);
return this.consumeChallenge('upgrade', challenge, uid);
}

/**
Expand All @@ -187,13 +187,13 @@ export class PasskeyChallengeManager {
* has already been consumed). GETDEL in validateChallenge handles the normal case.
* This method succeeds silently even if the key does not exist.
*
* @param challenge - The base64url-encoded challenge to delete.
* @param type - The challenge type (used to reconstruct the Redis key).
* @param challenge - The base64url-encoded challenge to delete.
* @param uid - The hex-encoded uid of the user.
*/
async deleteChallenge(
challenge: string,
type: ChallengeType,
challenge: string,
uid: string
): Promise<void> {
const key = this.buildKey(type, challenge, uid);
Expand All @@ -209,14 +209,15 @@ export class PasskeyChallengeManager {
* operation, enforcing single-use semantics. Returns null if not found, expired,
* or if the stored value is invalid JSON.
*
* @param challenge - The base64url-encoded challenge from the WebAuthn ceremony.
* @param type - The expected challenge type (prevents cross-ceremony attacks).
* @param challenge - The base64url-encoded challenge from the WebAuthn ceremony.
* @param uid - Optional hex-encoded uid
* @returns The stored challenge metadata (uid, type, timestamps) or null if not found expired.
*/
private async consumeChallenge(
uid: string,
type: ChallengeType,
challenge: string
challenge: string,
uid?: string
): Promise<StoredChallenge | null> {
const key = this.buildKey(type, challenge, uid);
const raw = await this.redis.getdel(key);
Expand All @@ -242,7 +243,7 @@ export class PasskeyChallengeManager {

private async generateChallenge(
type: ChallengeType,
uid: string
uid?: string
): Promise<string> {
const challenge = randomBytes(32).toString('base64url');
const now = Date.now();
Expand All @@ -269,8 +270,10 @@ export class PasskeyChallengeManager {
private buildKey(
type: ChallengeType,
challenge: string,
uid: string
uid?: string
): string {
return `passkey:challenge:${uid}:${type}:${challenge}`;
return uid
? `passkey:challenge:${type}:${challenge}:${uid}`
: `passkey:challenge:${type}:${challenge}`;
}
}
Loading
Loading