Merge pull request #552 from MasterKale/feat/530-remove-user-id-footgun

feat/530-remove-user-id-footgun

6 files changed, 39 insertions, 57 deletions

diff --git a/packages/browser/src/helpers/bufferToUTF8String.ts b/packages/browser/src/helpers/bufferToUTF8String.ts
deleted file mode 100644
index 0da3246..0000000
--- a/packages/browser/src/helpers/bufferToUTF8String.ts
+++ /dev/null
@@ -1,7 +0,0 @@
-/**
- * A helper method to convert an arbitrary ArrayBuffer, returned from an authenticator, to a UTF-8
- * string.
- */
-export function bufferToUTF8String(value: ArrayBuffer): string {
-  return new TextDecoder('utf-8').decode(value);
-}
diff --git a/packages/browser/src/helpers/utf8StringToBuffer.ts b/packages/browser/src/helpers/utf8StringToBuffer.ts
deleted file mode 100644
index b15e5fe..0000000
--- a/packages/browser/src/helpers/utf8StringToBuffer.ts
+++ /dev/null
@@ -1,7 +0,0 @@
-/**
- * A helper method to convert an arbitrary string sent from the server to an ArrayBuffer the
- * authenticator will expect.
- */
-export function utf8StringToBuffer(value: string): ArrayBuffer {
-  return new TextEncoder().encode(value);
-}
diff --git a/packages/browser/src/methods/startAuthentication.test.ts b/packages/browser/src/methods/startAuthentication.test.ts
index 5450e16..d2dd4b7 100644
--- a/packages/browser/src/methods/startAuthentication.test.ts
+++ b/packages/browser/src/methods/startAuthentication.test.ts
@@ -7,7 +7,7 @@ import {
 
 import { browserSupportsWebAuthn } from '../helpers/browserSupportsWebAuthn';
 import { browserSupportsWebAuthnAutofill } from '../helpers/browserSupportsWebAuthnAutofill';
-import { utf8StringToBuffer } from '../helpers/utf8StringToBuffer';
+import { base64URLStringToBuffer } from '../helpers/base64URLStringToBuffer';
 import { bufferToBase64URLString } from '../helpers/bufferToBase64URLString';
 import { WebAuthnError } from '../helpers/webAuthnError';
 import { generateCustomError } from '../helpers/__jest__/generateCustomError';
@@ -25,11 +25,11 @@ const mockSupportsAutofill = browserSupportsWebAuthnAutofill as jest.Mock;
 const mockAuthenticatorData = 'mockAuthenticatorData';
 const mockClientDataJSON = 'mockClientDataJSON';
 const mockSignature = 'mockSignature';
-const mockUserHandle = 'mockUserHandle';
+const mockUserHandle = 'f4pdy3fpA34';
 
 // With ASCII challenge
 const goodOpts1: PublicKeyCredentialRequestOptionsJSON = {
-  challenge: bufferToBase64URLString(utf8StringToBuffer('fizz')),
+  challenge: '1T6uHri4OAQ',
   allowCredentials: [
     {
       id: 'C0VGlvYFratUdAV1iCw-ULpUW8E-exHPXQChBfyVeJZCMfjMFcwDmOFgoMUz39LoMtCJUBW8WPlLkGT6q8qTCg',
@@ -42,7 +42,7 @@ const goodOpts1: PublicKeyCredentialRequestOptionsJSON = {
 
 // With UTF-8 challenge
 const goodOpts2UTF8: PublicKeyCredentialRequestOptionsJSON = {
-  challenge: bufferToBase64URLString(utf8StringToBuffer('やれやれだぜ')),
+  challenge: bufferToBase64URLString(new TextEncoder().encode('やれやれだぜ')),
   allowCredentials: [],
   timeout: 1,
 };
@@ -78,7 +78,7 @@ test('should convert options before passing to navigator.credentials.get(...)',
   const credId = argsPublicKey.allowCredentials[0].id;
 
   expect(new Uint8Array(argsPublicKey.challenge)).toEqual(
-    new Uint8Array([102, 105, 122, 122]),
+    new Uint8Array([213, 62, 174, 30, 184, 184, 56, 4]),
   );
   // Make sure the credential ID is an ArrayBuffer with a length of 64
   expect(credId instanceof ArrayBuffer).toEqual(true);
@@ -87,7 +87,7 @@ test('should convert options before passing to navigator.credentials.get(...)',
 
 test('should support optional allowCredential', async () => {
   await startAuthentication({
-    challenge: bufferToBase64URLString(utf8StringToBuffer('fizz')),
+    challenge: '1T6uHri4OAQ',
     timeout: 1,
   });
 
@@ -96,7 +96,7 @@ test('should support optional allowCredential', async () => {
 
 test('should convert allow allowCredential to undefined when empty', async () => {
   await startAuthentication({
-    challenge: bufferToBase64URLString(utf8StringToBuffer('fizz')),
+    challenge: '1T6uHri4OAQ',
     timeout: 1,
     allowCredentials: [],
   });
@@ -113,7 +113,7 @@ test('should return base64url-encoded response values', async () => {
           authenticatorData: Buffer.from(mockAuthenticatorData, 'ascii'),
           clientDataJSON: Buffer.from(mockClientDataJSON, 'ascii'),
           signature: Buffer.from(mockSignature, 'ascii'),
-          userHandle: Buffer.from(mockUserHandle, 'ascii'),
+          userHandle: base64URLStringToBuffer(mockUserHandle),
         },
         getClientExtensionResults: () => ({}),
         type: 'public-key',
@@ -130,10 +130,10 @@ test('should return base64url-encoded response values', async () => {
   );
   expect(response.response.clientDataJSON).toEqual('bW9ja0NsaWVudERhdGFKU09O');
   expect(response.response.signature).toEqual('bW9ja1NpZ25hdHVyZQ');
-  expect(response.response.userHandle).toEqual('mockUserHandle');
+  expect(response.response.userHandle).toEqual('f4pdy3fpA34');
 });
 
-test('should throw error if WebAuthn isn\'t supported', async () => {
+test("should throw error if WebAuthn isn't supported", async () => {
   mockSupportsWebAuthn.mockReturnValue(false);
 
   await expect(startAuthentication(goodOpts1)).rejects.toThrow(
diff --git a/packages/browser/src/methods/startAuthentication.ts b/packages/browser/src/methods/startAuthentication.ts
index 390b6ef..8b2e02d 100644
--- a/packages/browser/src/methods/startAuthentication.ts
+++ b/packages/browser/src/methods/startAuthentication.ts
@@ -6,7 +6,6 @@ import {
 
 import { bufferToBase64URLString } from '../helpers/bufferToBase64URLString';
 import { base64URLStringToBuffer } from '../helpers/base64URLStringToBuffer';
-import { bufferToUTF8String } from '../helpers/bufferToUTF8String';
 import { browserSupportsWebAuthn } from '../helpers/browserSupportsWebAuthn';
 import { browserSupportsWebAuthnAutofill } from '../helpers/browserSupportsWebAuthnAutofill';
 import { toPublicKeyCredentialDescriptor } from '../helpers/toPublicKeyCredentialDescriptor';
@@ -17,12 +16,11 @@ import { toAuthenticatorAttachment } from '../helpers/toAuthenticatorAttachment'
 /**
  * Begin authenticator "login" via WebAuthn assertion
  *
- * @param requestOptionsJSON Output from **@simplewebauthn/server**'s `generateAuthenticationOptions()`
- * @param useBrowserAutofill Initialize conditional UI to enable logging in via browser
- * autofill prompts
+ * @param optionsJSON Output from **@simplewebauthn/server**'s `generateAuthenticationOptions()`
+ * @param useBrowserAutofill (Optional) Initialize conditional UI to enable logging in via browser autofill prompts. Defaults to `false`.
  */
 export async function startAuthentication(
-  requestOptionsJSON: PublicKeyCredentialRequestOptionsJSON,
+  optionsJSON: PublicKeyCredentialRequestOptionsJSON,
   useBrowserAutofill = false,
 ): Promise<AuthenticationResponseJSON> {
   if (!browserSupportsWebAuthn()) {
@@ -32,16 +30,16 @@ export async function startAuthentication(
   // We need to avoid passing empty array to avoid blocking retrieval
   // of public key
   let allowCredentials;
-  if (requestOptionsJSON.allowCredentials?.length !== 0) {
-    allowCredentials = requestOptionsJSON.allowCredentials?.map(
+  if (optionsJSON.allowCredentials?.length !== 0) {
+    allowCredentials = optionsJSON.allowCredentials?.map(
       toPublicKeyCredentialDescriptor,
     );
   }
 
   // We need to convert some values to Uint8Arrays before passing the credentials to the navigator
   const publicKey: PublicKeyCredentialRequestOptions = {
-    ...requestOptionsJSON,
-    challenge: base64URLStringToBuffer(requestOptionsJSON.challenge),
+    ...optionsJSON,
+    challenge: base64URLStringToBuffer(optionsJSON.challenge),
     allowCredentials,
   };
 
@@ -59,7 +57,7 @@ export async function startAuthentication(
 
     // Check for an <input> with "webauthn" in its `autocomplete` attribute
     const eligibleInputs = document.querySelectorAll(
-      'input[autocomplete$=\'webauthn\']',
+      "input[autocomplete$='webauthn']",
     );
 
     // WebAuthn autofill requires at least one valid input
@@ -97,7 +95,7 @@ export async function startAuthentication(
 
   let userHandle = undefined;
   if (response.userHandle) {
-    userHandle = bufferToUTF8String(response.userHandle);
+    userHandle = bufferToBase64URLString(response.userHandle);
   }
 
   // Convert values to base64 to make it easier to send back to the server
diff --git a/packages/browser/src/methods/startRegistration.test.ts b/packages/browser/src/methods/startRegistration.test.ts
index e6bdcb7..6b62a79 100644
--- a/packages/browser/src/methods/startRegistration.test.ts
+++ b/packages/browser/src/methods/startRegistration.test.ts
@@ -6,12 +6,10 @@ import {
 } from '@simplewebauthn/types';
 import { generateCustomError } from '../helpers/__jest__/generateCustomError';
 import { browserSupportsWebAuthn } from '../helpers/browserSupportsWebAuthn';
-import { bufferToBase64URLString } from '../helpers/bufferToBase64URLString';
+import { base64URLStringToBuffer } from '../helpers/base64URLStringToBuffer';
 import { WebAuthnError } from '../helpers/webAuthnError';
 import { WebAuthnAbortService } from '../helpers/webAuthnAbortService';
 
-import { utf8StringToBuffer } from '../helpers/utf8StringToBuffer';
-
 import { startRegistration } from './startRegistration';
 
 jest.mock('../helpers/browserSupportsWebAuthn');
@@ -23,7 +21,7 @@ const mockAttestationObject = 'mockAtte';
 const mockClientDataJSON = 'mockClie';
 
 const goodOpts1: PublicKeyCredentialCreationOptionsJSON = {
-  challenge: bufferToBase64URLString(utf8StringToBuffer('fizz')),
+  challenge: '1T6uHri4OAQ',
   attestation: 'direct',
   pubKeyCredParams: [
     {
@@ -36,7 +34,7 @@ const goodOpts1: PublicKeyCredentialCreationOptionsJSON = {
     name: 'SimpleWebAuthn',
   },
   user: {
-    id: '5678',
+    id: 'f4pdy3fpA34',
     displayName: 'username',
     name: 'username',
   },
@@ -77,10 +75,10 @@ test('should convert options before passing to navigator.credentials.create(...)
 
   // Make sure challenge and user.id are converted to Buffers
   expect(new Uint8Array(argsPublicKey.challenge)).toEqual(
-    new Uint8Array([102, 105, 122, 122]),
+    new Uint8Array([213, 62, 174, 30, 184, 184, 56, 4]),
   );
   expect(new Uint8Array(argsPublicKey.user.id)).toEqual(
-    new Uint8Array([53, 54, 55, 56]),
+    new Uint8Array([127, 138, 93, 203, 119, 233, 3, 126]),
   );
 
   // Confirm construction of excludeCredentials array
@@ -95,8 +93,8 @@ test('should return base64url-encoded response values', async () => {
     (): Promise<RegistrationCredential> => {
       return new Promise((resolve) => {
         resolve({
-          id: 'foobar',
-          rawId: utf8StringToBuffer('foobar'),
+          id: '6mUg8GzxDxs',
+          rawId: base64URLStringToBuffer('6mUg8GzxDxs'),
           response: {
             attestationObject: Buffer.from(mockAttestationObject, 'ascii'),
             clientDataJSON: Buffer.from(mockClientDataJSON, 'ascii'),
@@ -115,12 +113,12 @@ test('should return base64url-encoded response values', async () => {
 
   const response = await startRegistration(goodOpts1);
 
-  expect(response.rawId).toEqual('Zm9vYmFy');
+  expect(response.rawId).toEqual('6mUg8GzxDxs');
   expect(response.response.attestationObject).toEqual('bW9ja0F0dGU');
   expect(response.response.clientDataJSON).toEqual('bW9ja0NsaWU');
 });
 
-test('should throw error if WebAuthn isn\'t supported', async () => {
+test("should throw error if WebAuthn isn't supported", async () => {
   mockSupportsWebauthn.mockReturnValue(false);
 
   await expect(startRegistration(goodOpts1)).rejects.toThrow(
@@ -216,8 +214,8 @@ test('should support "cable" transport in excludeCredentials', async () => {
 
 test('should return "cable" transport from response', async () => {
   mockNavigatorCreate.mockResolvedValue({
-    id: 'foobar',
-    rawId: utf8StringToBuffer('foobar'),
+    id: '6mUg8GzxDxs',
+    rawId: base64URLStringToBuffer('6mUg8GzxDxs'),
     response: {
       attestationObject: Buffer.from(mockAttestationObject, 'ascii'),
       clientDataJSON: Buffer.from(mockClientDataJSON, 'ascii'),
@@ -589,7 +587,8 @@ describe('WebAuthnError', () => {
         ...goodOpts1,
         user: {
           ...goodOpts1.user,
-          id: Array(65).fill('a').join(''),
+          // A base64url string 100 characters long should decode to ~70 bytes
+          id: Array(100).fill('a').join(''),
         },
       };
 
diff --git a/packages/browser/src/methods/startRegistration.ts b/packages/browser/src/methods/startRegistration.ts
index 09117c3..b661de7 100644
--- a/packages/browser/src/methods/startRegistration.ts
+++ b/packages/browser/src/methods/startRegistration.ts
@@ -5,7 +5,6 @@ import {
   RegistrationResponseJSON,
 } from '@simplewebauthn/types';
 
-import { utf8StringToBuffer } from '../helpers/utf8StringToBuffer';
 import { bufferToBase64URLString } from '../helpers/bufferToBase64URLString';
 import { base64URLStringToBuffer } from '../helpers/base64URLStringToBuffer';
 import { browserSupportsWebAuthn } from '../helpers/browserSupportsWebAuthn';
@@ -17,10 +16,10 @@ import { toAuthenticatorAttachment } from '../helpers/toAuthenticatorAttachment'
 /**
  * Begin authenticator "registration" via WebAuthn attestation
  *
- * @param creationOptionsJSON Output from **@simplewebauthn/server**'s `generateRegistrationOptions()`
+ * @param optionsJSON Output from **@simplewebauthn/server**'s `generateRegistrationOptions()`
  */
 export async function startRegistration(
-  creationOptionsJSON: PublicKeyCredentialCreationOptionsJSON,
+  optionsJSON: PublicKeyCredentialCreationOptionsJSON,
 ): Promise<RegistrationResponseJSON> {
   if (!browserSupportsWebAuthn()) {
     throw new Error('WebAuthn is not supported in this browser');
@@ -28,13 +27,13 @@ export async function startRegistration(
 
   // We need to convert some values to Uint8Arrays before passing the credentials to the navigator
   const publicKey: PublicKeyCredentialCreationOptions = {
-    ...creationOptionsJSON,
-    challenge: base64URLStringToBuffer(creationOptionsJSON.challenge),
+    ...optionsJSON,
+    challenge: base64URLStringToBuffer(optionsJSON.challenge),
     user: {
-      ...creationOptionsJSON.user,
-      id: utf8StringToBuffer(creationOptionsJSON.user.id),
+      ...optionsJSON.user,
+      id: base64URLStringToBuffer(optionsJSON.user.id),
     },
-    excludeCredentials: creationOptionsJSON.excludeCredentials?.map(
+    excludeCredentials: optionsJSON.excludeCredentials?.map(
       toPublicKeyCredentialDescriptor,
     ),
   };