Skip to content
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

fix: Remove ArrayBuffer checks from WebAuthnIdentity #857

Merged
merged 3 commits into from
Mar 18, 2024
Merged

fix: Remove ArrayBuffer checks from WebAuthnIdentity #857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
843c833
fix: Remove ArrayBuffer checks from WebAuthnIdentity
Mar 15, 2024
52fbb90
bufFromBufLike
krpeacock Mar 15, 2024
b3c30de
checking presence of attestationObject instead of checking for ArrayB…
krpeacock Mar 15, 2024
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
1 change: 1 addition & 0 deletions docs/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

* feat: adds `fromPem` method for `identity-secp256k1`
* feat: HttpAgent tracks a watermark from the latest readState call. Queries with signatures made before the watermark will be automatically retried, and rejected if they are still behind.
* fix: remove `ArrrayBuffer` checks from `WebAuthnIdentity` to resolve issues with the Bitwarden password manager

## [1.0.1] - 2024-02-20

Expand Down
45 changes: 21 additions & 24 deletions packages/identity/src/identity/webauthn.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
} from '@dfinity/agent';
import borc from 'borc';
import { randomBytes } from '@noble/hashes/utils';
import { bufFromBufLike } from '@dfinity/candid';

function _coseToDerEncodedBlob(cose: ArrayBuffer): DerEncodedPublicKey {
return wrapDER(cose, DER_COSE_OID).buffer as DerEncodedPublicKey;
Expand Down Expand Up @@ -104,15 +105,17 @@ async function _createCredential(
},
},
},
)) as PublicKeyCredentialWithAttachment;
)) as PublicKeyCredentialWithAttachment | null;

// Validate that it's the correct type at runtime, since WebAuthn does not HAVE to
// reply with a PublicKeyCredential.
if (creds.response === undefined || !(creds.rawId instanceof ArrayBuffer)) {
if (creds === null) {
return null;
} else {
return creds;
}

return {
...creds,
// Some password managers will return a Uint8Array, so we ensure we return an ArrayBuffer.
rawId: bufFromBufLike(creds.rawId),
};
}

// See https://www.iana.org/assignments/cose/cose.xhtml#algorithms for a complete
Expand Down Expand Up @@ -154,7 +157,7 @@ export class WebAuthnIdentity extends SignIdentity {
}

const response = creds.response as AuthenticatorAttestationResponse;
Copy link
Member

Choose a reason for hiding this comment

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

Aren't you now potentially casting an undefined to an AuthenticatorAttestationResponse, given that the snippet you removed also checked for creds.response === undefined? In other words, aren't you potentially facing uncaught undefined issues in the following lines?

Copy link
Contributor

Choose a reason for hiding this comment

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

None of the typing suggests that creds.response can be undefined, given the other checks we've done to make sure that _createCredential yields PublicKeyCredentialWithAttachment | null

Copy link
Member

Choose a reason for hiding this comment

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

So this undefined test was useless?

if (creds.response === undefined || !(creds.rawId instanceof ArrayBuffer)) {

Good then, thanks for the feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

It may have been. We have also updated the version of Typescript and other browser typings since this was written, so something I'm unaware of may have changed in the meantime

if (!(response.attestationObject instanceof ArrayBuffer)) {
if (response.attestationObject === undefined) {
throw new Error('Was expecting an attestation response.');
}

Expand Down Expand Up @@ -214,24 +217,18 @@ export class WebAuthnIdentity extends SignIdentity {
}

const response = result.response as AuthenticatorAssertionResponse;
if (
response.signature instanceof ArrayBuffer &&
response.authenticatorData instanceof ArrayBuffer
) {
const cbor = borc.encode(
new borc.Tagged(55799, {
authenticator_data: new Uint8Array(response.authenticatorData),
client_data_json: new TextDecoder().decode(response.clientDataJSON),
signature: new Uint8Array(response.signature),
}),
);
if (!cbor) {
throw new Error('failed to encode cbor');
}
return cbor.buffer as Signature;
} else {
throw new Error('Invalid response from WebAuthn.');

const cbor = borc.encode(
new borc.Tagged(55799, {
authenticator_data: new Uint8Array(response.authenticatorData),
client_data_json: new TextDecoder().decode(response.clientDataJSON),
signature: new Uint8Array(response.signature),
}),
);
if (!cbor) {
throw new Error('failed to encode cbor');
}
return cbor.buffer as Signature;
}

/**
Expand Down
Loading