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.

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

Aborting authentication throws a string instead of an error #359

Closed
mhamann opened this issue Feb 15, 2023 · 5 comments · Fixed by #371
Closed

Aborting authentication throws a string instead of an error #359

mhamann opened this issue Feb 15, 2023 · 5 comments · Fixed by #371

Comments

@mhamann
Copy link

mhamann commented Feb 15, 2023

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

  • OS: macOS 13.2
  • Browser: Chrome 110
  • Authenticator: MacBook Pro Touch ID

SimpleWebAuthn Libraries

$ npm list --depth=0 | grep @simplewebauthn
├── @simplewebauthn/browser@7.0.1
├── @simplewebauthn/typescript-types@7.0.0
@MasterKale
Copy link
Owner

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
Copy link
Owner

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

@mhamann
Copy link
Author

mhamann commented Feb 16, 2023

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
Copy link
Owner

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants