MasterKale / SimpleWebAuthn

WebAuthn, Simplified. A collection of TypeScript-first libraries for simpler WebAuthn integration. Supports modern browsers, Node, Deno, and more.
https://simplewebauthn.dev
MIT License
1.62k stars 137 forks source link

Aborting authentication throws a string instead of an error #359

Closed mhamann closed 1 year ago

mhamann commented 1 year ago

Describe the issue

Calling startAuthentication(opts, false) in the browser after a previous call to startAuthentication(opts, true) aborts the previous authentication flow. This causes the browser runtime to throw, which is good, but in this case, what's thrown is a string.

When navigator.credentials.get(...) is aborted by an AbortController, the browser runtime throws whatever you pass into .abort(reason) (at least on Chromium-based browsers. Safari seems to ignore the abort signal entirely.).

It seems like the reason passed to abort should be an instance of Error instead.

Reproduction Steps

var aborter = new AbortController();
navigator.credentials.get({
    publicKey: {
      challenge: new Uint8Array([12, 34, 56, 78, 90])
    },
    signal: aborter.signal
}).catch(err => {
  console.log(typeof err);
  console.log(err);
});
aborter.abort("Stop authenticating!");

// Prints:
// string
// Stop authenticating!

Replacing the previous aborter.abort(...) with the following causes an Error to be thrown:

aborter.abort(new Error('Stop authenticating!'));

// Prints:
// object
// Error: Stop authenticating!
//       at <stack>

Expected behavior

An Error should be thrown instead of a string.

Dependencies

SimpleWebAuthn Libraries

$ npm list --depth=0 | grep @simplewebauthn
├── @simplewebauthn/browser@7.0.1
├── @simplewebauthn/typescript-types@7.0.0
MasterKale commented 1 year ago

There's not a lot of guidance on what a "proper" reason should be. MDN simply says the following about AbortSignal.reason:

A JavaScript value that indicates the abort reason, or undefined, if not aborted.

And in the MDN docs for AbortController.abort() it says reason can be any JS value:

The reason why the operation was aborted, which can be any JavaScript value. If not specified, the reason is set to "AbortError" DOMException.

It seems like the reason passed to abort should be an instance of Error instead.

I suppose the idea is that err in a .catch() will always be some kind of Error, except in this one case where's a string? I can see that being a point of developer friction 🤔

MasterKale commented 1 year ago

Note to self: this seems like a reasonable request so I'll go ahead with it.

mhamann commented 1 year ago

Thanks! I started looking into contributing this back via PR. The change itself seemed pretty straightforward, but testing it was causing some headaches. I'm interested to see your solution 😁

MasterKale commented 1 year ago

Note to self: I haven't forgotten about this, I'm just waiting till I land #367 and then this'll be a fast follow. It'll all get released as v7.2.0 (unless I discover a reason it all needs to go out as v8.0.0) when I finally merge everything pending in.

MasterKale commented 1 year ago

@mhamann I just published @simplewebauthn/browser@7.2.0 that should resolve this issue 🚀