Merge pull request #353 from MasterKale/fix/352-not-allowed-error

fix/352-not-allowed-error
MasterKale · Feb 9, 2023 · c26df43 · c26df43
2 parents 94aaa5f + dc40f40
commit c26df43
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 42 deletions.
diff --git a/packages/browser/src/helpers/__jest__/generateCustomError.ts b/packages/browser/src/helpers/__jest__/generateCustomError.ts
@@ -10,8 +10,9 @@ type WebAuthnErrorName =
   | 'SecurityError'
   | 'UnknownError';
 
-export function generateCustomError(name: WebAuthnErrorName): Error {
+export function generateCustomError(name: WebAuthnErrorName, message = ''): Error {
   const customError = new Error();
   customError.name = name;
+  customError.message = message;
   return customError;
 }
diff --git a/packages/browser/src/helpers/identifyAuthenticationError.ts b/packages/browser/src/helpers/identifyAuthenticationError.ts
@@ -23,21 +23,10 @@ export function identifyAuthenticationError({
       return new WebAuthnError('Authentication ceremony was sent an abort signal', 'AbortError');
     }
   } else if (error.name === 'NotAllowedError') {
-    if (publicKey.allowCredentials?.length) {
-      // https://www.w3.org/TR/webauthn-2/#sctn-discover-from-external-source (Step 17)
-      // https://www.w3.org/TR/webauthn-2/#sctn-op-get-assertion (Step 6)
-      return new WebAuthnError(
-        'No available authenticator recognized any of the allowed credentials',
-        'NotAllowedError',
-      );
-    }
-
-    // https://www.w3.org/TR/webauthn-2/#sctn-discover-from-external-source (Step 18)
-    // https://www.w3.org/TR/webauthn-2/#sctn-op-get-assertion (Step 7)
-    return new WebAuthnError(
-      'User clicked cancel, or the authentication ceremony timed out',
-      'NotAllowedError',
-    );
+    /**
+     * Pass the error directly through. Platforms are overloading this error beyond what the spec
+     * defines and we don't want to overwrite potentially useful error messages.
+     */
   } else if (error.name === 'SecurityError') {
     const effectiveDomain = window.location.hostname;
     if (!isValidDomain(effectiveDomain)) {

diff --git a/packages/browser/src/helpers/identifyRegistrationError.ts b/packages/browser/src/helpers/identifyRegistrationError.ts
@@ -41,12 +41,10 @@ export function identifyRegistrationError({
     // https://www.w3.org/TR/webauthn-2/#sctn-op-make-cred (Step 3)
     return new WebAuthnError('The authenticator was previously registered', 'InvalidStateError');
   } else if (error.name === 'NotAllowedError') {
-    // https://www.w3.org/TR/webauthn-2/#sctn-createCredential (Step 20)
-    // https://www.w3.org/TR/webauthn-2/#sctn-createCredential (Step 21)
-    return new WebAuthnError(
-      'User clicked cancel, or the registration ceremony timed out',
-      'NotAllowedError',
-    );
+    /**
+     * Pass the error directly through. Platforms are overloading this error beyond what the spec
+     * defines and we don't want to overwrite potentially useful error messages.
+     */
   } else if (error.name === 'NotSupportedError') {
     const validPubKeyCredParams = publicKey.pubKeyCredParams.filter(
       param => param.type === 'public-key',

diff --git a/packages/browser/src/methods/startAuthentication.test.ts b/packages/browser/src/methods/startAuthentication.test.ts
@@ -332,29 +332,38 @@ describe('WebAuthnError', () => {
   });
 
   describe('NotAllowedError', () => {
-    const NotAllowedError = generateCustomError('NotAllowedError');
-
-    test('should identify unrecognized allowed credentials', async () => {
+    test('should pass through error message (iOS Safari - Operation failed)', async () => {
+      /**
+       * Thrown when biometric is not enrolled, or a Safari bug prevents conditional UI from being
+       * aborted properly between page reloads.
+       *
+       * See https://github.com/MasterKale/SimpleWebAuthn/discussions/350#discussioncomment-4896572
+       */
+      const NotAllowedError = generateCustomError('NotAllowedError', 'Operation failed.');
       mockNavigatorGet.mockRejectedValueOnce(NotAllowedError);
 
       const rejected = await expect(startAuthentication(goodOpts1)).rejects;
-      rejected.toThrow(WebAuthnError);
-      rejected.toThrow(/allowed credentials/i);
+      rejected.toThrow(Error);
+      rejected.toThrow(/operation failed/i);
       rejected.toHaveProperty('name', 'NotAllowedError');
     });
 
-    test('should identify cancellation or timeout', async () => {
+    test('should pass through error message (Chrome M110 - Bad TLS Cert)', async () => {
+      /**
+       * Starting from Chrome M110, WebAuthn is blocked if the site is being displayed on a URL with
+       * TLS certificate issues. This includes during development.
+       *
+       * See https://github.com/MasterKale/SimpleWebAuthn/discussions/351#discussioncomment-4910458
+       */
+      const NotAllowedError = generateCustomError(
+        'NotAllowedError',
+        'WebAuthn is not supported on sites with TLS certificate errors.'
+      );
       mockNavigatorGet.mockRejectedValueOnce(NotAllowedError);
 
-      const opts = {
-        ...goodOpts1,
-        allowCredentials: [],
-      };
-
-      const rejected = await expect(startAuthentication(opts)).rejects;
-      rejected.toThrow(WebAuthnError);
-      rejected.toThrow(/cancel/i);
-      rejected.toThrow(/timed out/i);
+      const rejected = await expect(startAuthentication(goodOpts1)).rejects;
+      rejected.toThrow(Error);
+      rejected.toThrow(/sites with TLS certificate errors/i);
       rejected.toHaveProperty('name', 'NotAllowedError');
     });
   });

diff --git a/packages/browser/src/methods/startRegistration.test.ts b/packages/browser/src/methods/startRegistration.test.ts
@@ -324,15 +324,38 @@ describe('WebAuthnError', () => {
   });
 
   describe('NotAllowedError', () => {
-    const NotAllowedError = generateCustomError('NotAllowedError');
+    test('should pass through error message (iOS Safari - Operation failed)', async () => {
+      /**
+       * Thrown when biometric is not enrolled, or a Safari bug prevents conditional UI from being
+       * aborted properly between page reloads.
+       *
+       * See https://github.com/MasterKale/SimpleWebAuthn/discussions/350#discussioncomment-4896572
+       */
+      const NotAllowedError = generateCustomError('NotAllowedError', 'Operation failed.');
+      mockNavigatorCreate.mockRejectedValueOnce(NotAllowedError);
+
+      const rejected = await expect(startRegistration(goodOpts1)).rejects;
+      rejected.toThrow(Error);
+      rejected.toThrow(/operation failed/i);
+      rejected.toHaveProperty('name', 'NotAllowedError');
+    });
 
-    test('should identify cancellation or timeout', async () => {
+    test('should pass through error message (Chrome M110 - Bad TLS Cert)', async () => {
+      /**
+       * Starting from Chrome M110, WebAuthn is blocked if the site is being displayed on a URL with
+       * TLS certificate issues. This includes during development.
+       *
+       * See https://github.com/MasterKale/SimpleWebAuthn/discussions/351#discussioncomment-4910458
+       */
+      const NotAllowedError = generateCustomError(
+        'NotAllowedError',
+        'WebAuthn is not supported on sites with TLS certificate errors.'
+      );
       mockNavigatorCreate.mockRejectedValueOnce(NotAllowedError);
 
       const rejected = await expect(startRegistration(goodOpts1)).rejects;
-      rejected.toThrow(WebAuthnError);
-      rejected.toThrow(/cancel/i);
-      rejected.toThrow(/timed out/i);
+      rejected.toThrow(Error);
+      rejected.toThrow(/sites with TLS certificate errors/i);
       rejected.toHaveProperty('name', 'NotAllowedError');
     });
   });