fix/webauthn-abort-controller-race-condition #275
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR attempts to remove a race condition from browser's WebAuthnAbortService. It was discovered that multiple invocations of
startAuthentication()
in a row (especially in a SPA using clientside routing), particularly when conditional UI is started, could lead toWebAuthnAbortService.reset()
being called early which would fail a subsequent call tostartAuthentication()
with aDOMException
and "a request is already pending."I determined that this was due to the call to
this.controller.abort()
not actually aborting thenavigator.credentials.get()
synchronously. We can'tawait
anAbortController.abort()
to wait for thenavigator.credentials.get()
to be aborted; we can only abort and cross our fingers that we don't callstartAuthentication()
until the WebAuthn API call actually terminates in response to the abort signal.This can lead to
.reset()
being called by the firststartAuthentication()
'stry / catch / finally
after a subsequent call towebauthnAbortService.createNewAbortSignal()
, which expectsthis.controller
to be populated with the new controller but ends up becomingundefined
when the.reset()
gets called after the first controller calls.abort()
. The second call tostartAuthentication()
can no longer be aborted, and a third call will error out because the second WebAuthn API call is indeed still pending.See #273 (comment), I included some logging output that might make it clearer if the above description is insufficient.
The fix is to remove the
reset()
method fromWebAuthnAbortService
. In testing I observed no issues with possibly repeatedly calling.abort()
on an already-aborted controller. Thus we can leave the service'sthis.controller
populated between calls tocreateNewAbortSignal()
and forget about trying to tame a race condition.Fixes #273.
Demonstration
Screen.Recording.2022-09-27.at.9.08.30.PM.mov