multiple calling of startAuthentication in conditional autofill causes reset to clear webauthnAbortService

[sameh-amwal]

when startAuthentication is called multiple times in succession, the createNewAbortSignal is called 3 times options.signal = webauthnAbortService.createNewAbortSignal();

However the corresponding reset is called later also 3 times causing WebAuthnAbortService.controller to be undefined. i.e. the abort controller of the last call is lost webauthnAbortService.reset();

Here is my proposal to fix this.

• change the reset method to accept an abortController

  reset(abortSignal) {
  if (this.controller?.signal === abortSignal) {
    this.controller = undefined;
  }
  }

• call the reset method with the associated controller, so this way the controller is not overriden

  webauthnAbortService.reset(options.signal);

Here is an attached video of the symptoms:

https://user-images.githubusercontent.com/100665288/192169058-3585728f-eee8-4c2c-a04c-4e89f31f4089.mov

[MasterKale]

when startAuthentication is called multiple times in succession...

Can you provide some example code of how you're calling startAuthentication() multiple times in a row? It's unclear from your demo video what exact sequence of events can recreate the issue.

There should only ever be one active WebAuthn request active at a time. Are you demoing a SPA that might trigger multiple startAuthentication(..., true) to set up Conditional UI any time this component is mounted? I tried recreating a few times on a basic HTML page and no client side routing and couldn't reproduce your exact issue; I did see the aborting of the conditional UI startAuthentication() calling reset after the new abort signal is created (because it seems the abort event is not synchronous):

        Starting conditional UI WebAuthn call
        Creating new abort signal for upcoming authentication attempt
        createNewAbortSignal() called
        creating new controller
        returning new signal
        Starting modal WebAuthn call
        Creating new abort signal for upcoming authentication attempt
        createNewAbortSignal() called
        found existing controller, calling abort() on it
------> creating new controller
        returning new signal
        resetting abort service after conditional UI
------> reset() called
        (Conditional UI) Aborting existing WebAuthn call to allow for another call
        resetting abort service after modal UI
        reset() called
        Error: NotAllowedError: User clicked cancel, or the authentication ceremony timed out
            at HTMLButtonElement.<anonymous> ((index):242:19)

This is some console output from my starting a conditional UI request on load (which creates the first abort controller), and then clicking a button to launch the WebAuthn modal UI and creates a second controller.

The problem seems to come from the fact that, when I click the "Authenticate" button which aborts the conditional UI request, the abort signal is asynchronous and gives us no means of waiting for the signal to be processed. The finally {} block in startAuthentication(..., true) is thus calling reset() after a new abort controller is created for the modal UI's startAuthentication(...), and so the WebAuthnAbortService loses track of that second controller (because the conditional UI's reset() sets its reference to it to undefined.)

Above are more notes for myself as I try to figure out a fix. I'm not sure your suggestion to refactor to pass in options.signal into reset() is the way to go, as it would mean that a pending Conditional UI request could never be aborted the moment the modal UI appears, because this.controller?.signal would only ever point to the existing options.signal for the modal UI request.

[sameh-amwal]

Are you demoing a SPA that might trigger multiple startAuthentication(..., true) to set up Conditional UI any time this component is mounted?

Yes that's exactly true. I am using a react front end that is managing state. My code is communicating with the backend every time the phone number changes to get a new webauth challenge. this causes the code to fire multiple times at the beginning.

Here are the relevant parts of my code

  const [keyMatch, setKeyMatch] = useState(false);
  const [useBiometrics, setUseBiometrics] = useState(false);
  const [phoneNumber, setPhoneNumber] = React.useState(parsePhoneNumber(savedPhoneNumber ?? ""));
  const [phoneValid, setPhoneValid] = React.useState(false);
  const [supportsWebauthAutofill, setSupportsWebauthAutofill] = React.useState(true);

  React.useEffect(() => {
    browserSupportsWebAuthnAutofill().then((support: boolean) => setSupportsWebauthAutofill(support));
  }, []);

  const handlePhoneNumberChange = async () => {
    setPhoneValid(phoneNumber?.isValid() ?? false);
    if (phoneNumber?.isValid()) {
      const phone_number = phoneNumber.number;
      if (phone_number !== props.order?.client_phone_number) {
        setUseBiometrics(false);
        setKeyMatch(false);
        setPhoneValid(false);
        try {
          const result = await fetch(`${serverURL}/transactions/${props.order?.id}/phone`, {
            method: 'POST',
            headers: {
              'Content-Type': 'application/json',
            },
            body: JSON.stringify({
              phone_number,
            }),
            credentials: 'include',
          })
          if (result.ok) {
            const payload = await result.json();
            props.order_actions.setOrder(payload);
            setPhoneValid(true);
          }
        } catch (err) {
          console.log(err);
        }
      }
    }
  }

  React.useEffect(() => {
    if (props.order?.webauth?.allowCredentials) {
      for (const cred of props.order.webauth.allowCredentials) {
        if (agent.os.name !== "Mac OS") {
          setUseBiometrics(true);
        }
        if (window.localStorage?.getItem(cred.id)) {
          setUseBiometrics(true);
          setKeyMatch(true);
        }
      }
    } else {
      setUseBiometrics(false);
      setKeyMatch(false);
    }
  }, [props.order]);

  React.useEffect(() => {
    if (props.order) {
      handlePhoneNumberChange();
    }
  }, [phoneNumber, props.order, supportsWebauthAutofill]);

  React.useEffect(() => {
    if (props.order) {
      const { webauth } = props.order;
      if (webauth) {
        browserSupportsWebAuthnAutofill().then((useBrowserAutofill: boolean) => {
          if (useBrowserAutofill || useBiometrics && (isAndroid || isChrome)) {
            props.user_actions.webauthLogin(webauth, useBrowserAutofill);
          }
        })
      }
    }
  }, [props.order, useBiometrics]);

and that's the code of props.user_actions.webauthLogin

export function webauthLogin(requestOptionsJSON: PublicKeyCredentialRequestOptionsJSON, useBrowserAutofill?: boolean) {
  return (async (dispatch: UserDispatchFunction,
    getState: () => AmwalState) => {
    try {
      const state = getState();
      const attResp = await startAuthentication(requestOptionsJSON, useBrowserAutofill);
      const verificationResp = await fetch(`${serverURL}/client/signin/`, {
        method: 'POST',
        headers: {
          'Content-Type': 'application/json',
        },
        body: JSON.stringify(attResp),
        credentials: 'include',
      });
      if (!verificationResp.ok) {
        throw verificationResp.statusText || await verificationResp.text();
      }
      const result = await verificationResp.json();
      // Wait for the results of verification
      window.localStorage?.setItem(attResp.id, "1")
      window.localStorage?.setItem("lastCredId", attResp.id)
      dispatch({
        type: UserActions.LOGIN,
        updateUser: result.user,
        authToken: result.token,
      });
    } catch (err: any) {
      console.log(`webauth(${useBrowserAutofill})`)
      console.log(err);
      dispatch({
        type: UserActions.SET_ERROR,
        error: err || "error",
      })
    }
  })
}

[sameh-amwal]

Here is another screen recording with breakpoints showing the issue

https://user-images.githubusercontent.com/100665288/192274209-abe5bdce-53e3-4495-8f57-270a8139e938.mov

[MasterKale]

I played around a bit more with this last night, I think the solution will be to just get rid of reset(). There's no observable penalty for calling .abort() on a controller more than once, so there's no need to defensively clear an instance of AbortController. I appear to be able to leave the controller intact until the next time createNewAbortSignal() is called, even if that somehow means this.controller.abort() gets called again 🤔

I'll prep a PR and maybe you can take it out for a spin.

[sameh-amwal]

I'll prep a PR and maybe you can take it out for a spin.

would love to. Thanks a lot for the great library and being on top of issues.

[MasterKale]

@sameh-amwal I have a PR ready with a fix. If you're comfortable with cloning and npm install'ing file paths you should be able to test it locally with something like this:

$> git clone https://github.com/MasterKale/SimpleWebAuthn.git
$> cd SimpleWebAuthn
$> git checkout fix/webauthn-abort-controller-race-condition
$> npm install
$> npm run build:browser
$> cd ../my-project
$> npm install ../SimpleWebAuthn/packages/browser
$> npm start

NPM does support npm install'ing branches on a repo, but unfortunately because SimpleWebAuthn is a monorepo you have to jump through a few extra steps instead.

If the steps above prove too tricky then let me know and I can cut a new alpha release for you to simply npm install and confirm the fix.

[sameh-amwal]

It worked! A minor feedback for getting the npm run build:browser to work. I had to update the package.json as such to get it to work.

diff --git a/package.json b/package.json
index 5b0b404..d4e5788 100644
--- a/package.json
+++ b/package.json
@@ -14,6 +14,8 @@
     "dev:browser": "lerna exec npm run test:watch --scope=@simplewebauthn/browser"
   },
   "devDependencies": {
+    "@rollup/plugin-node-resolve": "^14.1.0",
+    "@rollup/plugin-typescript": "^8.5.0",
     "@types/express": "^4.17.9",
     "@types/jest": "^27.0.1",
     "@typescript-eslint/eslint-plugin": "^5.31.0",
@@ -27,6 +29,8 @@
     "nx": "^14.4.3",
     "prettier": "^2.2.1",
     "rimraf": "^3.0.2",
+    "rollup-plugin-terser": "^7.0.2",
+    "rollup-plugin-version-injector": "^1.3.3",
     "semver": "^7.3.2",
     "ts-jest": "^27.0.5",
     "ts-morph": "^11.0.3",

https://user-images.githubusercontent.com/100665288/192731589-7d15c7d9-377c-4af5-9134-185079e6b610.mov

[MasterKale]

A minor feedback for getting the npm run build:browser to work. I had to update the package.json as such to get it to work.

Thanks for the feedback, I managed to figure out how to prevent this from happening again as you're not the first person interacting with this project lately to give similar feedback. I addressed it in #276.

It worked!

Alright, so I can take this comment to mean that the fix in #275 successfully addresses this issue?

[sameh-amwal]

Alright, so I can take this comment to mean that the fix in #275 successfully addresses this issue?

Absolutely. Thanks for the fix 🙏

[MasterKale]

The fix for this is now available in @simplewebauthn/browser@6.2.1 🥳