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

feat/530-remove-user-id-footgun

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(

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

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(''),
         },
       };
 

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,24 +16,24 @@ 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');
   }
 
   // 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,
     ),
   };

packages/server/src/authentication/verifyAuthenticationResponse.test.ts

@@ -390,7 +390,7 @@ Deno.test('should pass verification if custom challenge verifier returns true',
         actualChallenge: string;
         arbitraryData: string;
       } = JSON.parse(
-        isoBase64URL.toString(challenge),
+        isoBase64URL.toUTF8String(challenge),
       );
       return parsedChallenge.actualChallenge ===
         'K3QxOjnVJLiGlnVEp5va5QJeMVWNf_7PYgutgbAtAUA';
@@ -448,7 +448,7 @@ Deno.test('should pass verification if custom challenge verifier returns a Promi
         actualChallenge: string;
         arbitraryData: string;
       } = JSON.parse(
-        isoBase64URL.toString(challenge),
+        isoBase64URL.toUTF8String(challenge),
       );
       return Promise.resolve(
         parsedChallenge.actualChallenge ===
@@ -597,7 +597,7 @@ const assertionResponse: AuthenticationResponseJSON = {
   clientExtensionResults: {},
   type: 'public-key',
 };
-const assertionChallenge = isoBase64URL.fromString(
+const assertionChallenge = isoBase64URL.fromUTF8String(
   'totallyUniqueValueEveryTime',
 );
 const assertionOrigin = 'https://dev.dontneeda.pw';
@@ -627,7 +627,7 @@ const assertionFirstTimeUsedResponse: AuthenticationResponseJSON = {
   type: 'public-key',
   clientExtensionResults: {},
 };
-const assertionFirstTimeUsedChallenge = isoBase64URL.fromString(
+const assertionFirstTimeUsedChallenge = isoBase64URL.fromUTF8String(
   'totallyUniqueValueEveryAssertion',
 );
 const assertionFirstTimeUsedOrigin = 'https://dev.dontneeda.pw';

packages/server/src/helpers/decodeClientDataJSON.ts

@@ -5,7 +5,7 @@ import type { Base64URLString } from '../deps.ts';
  * Decode an authenticator's base64url-encoded clientDataJSON to JSON
  */
 export function decodeClientDataJSON(data: Base64URLString): ClientDataJSON {
-  const toString = isoBase64URL.toString(data);
+  const toString = isoBase64URL.toUTF8String(data);
   const clientData: ClientDataJSON = JSON.parse(toString);
 
   return _decodeClientDataJSONInternals.stubThis(clientData);

packages/server/src/helpers/generateUserID.test.ts

@@ -0,0 +1,16 @@
+import { assert, assertNotEquals } from 'https://deno.land/[email protected]/assert/mod.ts';
+
+import { generateUserID } from './generateUserID.ts';
+
+Deno.test('should return a buffer of 32 bytes', async () => {
+  const userID = await generateUserID();
+
+  assert(userID.byteLength === 32);
+});
+
+Deno.test('should return random bytes on each execution', async () => {
+  const userID1 = await generateUserID();
+  const userID2 = await generateUserID();
+
+  assertNotEquals(userID1, userID2);
+});

packages/server/src/helpers/generateUserID.ts

@@ -0,0 +1,21 @@
+import { isoCrypto } from './iso/index.ts';
+
+/**
+ * Generate a suitably random value to be used as user ID
+ */
+export async function generateUserID(): Promise<Uint8Array> {
+  /**
+   * WebAuthn spec says user.id has a max length of 64 bytes. I prefer how 32 random bytes look
+   * after they're base64url-encoded so I'm choosing to go with that here.
+   */
+  const newUserID = new Uint8Array(32);
+
+  await isoCrypto.getRandomValues(newUserID);
+
+  return _generateUserIDInternals.stubThis(newUserID);
+}
+
+// Make it possible to stub the return value during testing
+export const _generateUserIDInternals = {
+  stubThis: (value: Uint8Array) => value,
+};