Skip to content

fix: for issues with _allowCredentials and device registration #582

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

Merged
ryanbas21 merged 1 commit into from
Mar 10, 2026
Merged

fix: for issues with _allowCredentials and device registration #582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 34 additions & 0 deletions packages/javascript-sdk/src/fr-webauthn/fr-webauthn.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,4 +210,38 @@ describe('Test FRWebAuthn class with Conditional UI', () => {
}),
);
});

it('should throw NotSupportedError if WebAuthn is not supported', async () => {
// Mock WebAuthn not supported
const spy = vi.spyOn(FRWebAuthn, 'isWebAuthnSupported').mockReturnValue(false);

await expect(FRWebAuthn.getRegistrationCredential({} as any)).rejects.toThrow(
'PublicKeyCredential not supported by this browser',
);

spy.mockRestore();
});

it('should correctly convert _allowCredentials id to Int8Array', () => {
const metadata: any = {
_action: 'webauthn_authentication',
challenge: 'JEisuqkVMhI490jM0/iEgrRz+j94OoGc7gdY4gYicSk=',
relyingPartyId: '',
_allowCredentials: [
{
type: 'public-key',
id: [1, 2, 3, 4],
transports: ['usb'],
},
],
timeout: 60000,
};

const publicKey = FRWebAuthn.createAuthenticationPublicKey(metadata);

expect(publicKey.allowCredentials).toBeDefined();
expect(publicKey.allowCredentials![0].id).toBeInstanceOf(Int8Array);
const idArray = publicKey.allowCredentials![0].id as Int8Array;
expect(Array.from(idArray)).toEqual([1, 2, 3, 4]);
});
});
11 changes: 9 additions & 2 deletions packages/javascript-sdk/src/fr-webauthn/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ abstract class FRWebAuthn {
options: PublicKeyCredentialCreationOptions,
): Promise<PublicKeyCredential | null> {
// Feature check before we attempt registering a device
if (this.isWebAuthnSupported()) {
if (!this.isWebAuthnSupported()) {
const e = new Error('PublicKeyCredential not supported by this browser');
e.name = WebAuthnOutcomeType.NotSupportedError;
throw e;
Expand Down Expand Up @@ -534,7 +534,14 @@ abstract class FRWebAuthn {
// Use the structured _allowCredentials if available, otherwise parse the string format
let allowCredentialsValue: PublicKeyCredentialDescriptor[] | undefined;
if (_allowCredentials && Array.isArray(_allowCredentials)) {
allowCredentialsValue = _allowCredentials;
// The incoming _allowCredentials entries have an `id` property of type `Array`, which is rejected by `navigator.credentials.get()`.
// Converting it to a TypedArray here to meet the spec (https://developer.mozilla.org/en-US/docs/Web/API/PublicKeyCredentialRequestOptions#id).
Comment on lines +537 to +538
Copy link
Contributor

@ryanbas21 ryanbas21 Mar 10, 2026

Choose a reason for hiding this comment

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

I don't understand this comment. The type PublicKeyCredentialDescriptor shows this

interface PublicKeyCredentialDescriptor {
    id: BufferSource;
    transports?: AuthenticatorTransport[];
    type: PublicKeyCredentialType;
}

This type comes from lib.dom.ts https://www.dec112.at/docs/ng112-js/1.9.4/interfaces/_internal_.PublicKeyCredentialDescriptor.html

Are we saying this is wrong?

navigator.credentials.get accepts CredentialRequestOptions

interface CredentialRequestOptions {
    mediation?: CredentialMediationRequirement;
    publicKey?: PublicKeyCredentialRequestOptions;
    signal?: AbortSignal;
}

Pulling your branch down, and just looking at cred it seems like this is already the correct type?

Image

I'm a bit confused by this change.

Copy link
Contributor Author

@thomas-schofield-fr thomas-schofield-fr Mar 10, 2026

Choose a reason for hiding this comment

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

I think TypeScript just doesn't correctly verify the incoming metadataCallback.getOutputValue('data') is actually the correct type all the way through - specifically, the id property is Array which doesn't conform to BufferSource.
Screenshot 2026-03-10 at 16 19 18

If we don't make the change, the error from navigator.credentials.get is fairly clear:
Screenshot 2026-03-10 at 16 24 00

Copy link
Contributor

@ryanbas21 ryanbas21 Mar 10, 2026

Choose a reason for hiding this comment

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

Does this happen on the latest version of the SDK before these changes? I feel like this would be a pretty breaking bug?

Copy link
Contributor Author

@thomas-schofield-fr thomas-schofield-fr Mar 10, 2026

Choose a reason for hiding this comment

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

It does not, it's a new issue since 03135cf#diff-112486585734aee9407beea04e071c53e7eddce56a465b7462455fcb0e3bb346R514

allowCredentialsValue = _allowCredentials.map((cred) => {
return {
...cred,
id: new Int8Array(cred.id as unknown as number[]),
};
});
} else {
allowCredentialsValue = parseCredentials(allowCredentials || acceptableCredentials || '');
}
Expand Down
Loading