1383799 - WebAuthn operations in-flight must be cancelled on tab-switch

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, enhancement, P1)

Description

[J.C. Jones [:jcj] (he/him)]

WebAuthn operations that are in-flight with authenticators must be cancelled when switching tabs.

There's an Issue [1] opened with the WebAuthn spec for this already, but the language is _not_ in spec. Still, it's necessary for security, spec or not.

This also matches how Chromium handles U2F operations during a tab switch.

The spec language I've proposed is in the issue, but I did that w/o real knowledge of how this would be implemented. I think we should just do what's most appropriate for Firefox here and I will work to make the spec permit Firefox's take.

[1] https://github.com/w3c/webauthn/issues/316

Comment 1

[J.C. Jones [:jcj] (he/him)]

This was bugging me, so I stole it, Tim. Here's a patch that accomplishes the job... but I don't know how to test it with mochitests as-is. It's not really testable with the soft-token as-is, so this might be one that needs manual testing.

(Adding Matt to the QA field for context before we proceed with this)

Comment 2

[J.C. Jones [:jcj] (he/him)]

Attachment: Bug 1383799 - Cancel WebAuthn operations on tab-switch

WebAuthn operations that are in-flight with authenticators must be cancelled
when switching tabs.

There's an Issue [1] opened with the WebAuthn spec for this already, but the
language is _not_ in spec. Still, it's necessary for security, spec or not.

This also matches how Chromium handles U2F operations during a tab switch.

[1] https://github.com/w3c/webauthn/issues/316

Review commit: https://reviewboard.mozilla.org/r/165066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165066/

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8893996 [details]
Bug 1383799 - Cancel WebAuthn operations on tab-switch

https://reviewboard.mozilla.org/r/165066/#review171612

It shouldn't be hard to write tests but we should get this landed, to simplify QA and have test builds earlier. File a follow-up?

::: dom/webauthn/WebAuthnManager.cpp:230
(Diff revision 1)
> +  MOZ_ASSERT(aListener);
> +
> +  nsCOMPtr<nsIDocument> doc = aParent->GetExtantDoc();
> +  if (doc) {
> +    return doc->AddSystemEventListener(kVisibilityChange,
> +                                       /* listener */ aListener,

That comment is rather obvious ;)

::: dom/webauthn/WebAuthnManager.cpp:247
(Diff revision 1)
> +  MOZ_ASSERT(aListener);
> +
> +  nsCOMPtr<nsIDocument> doc = aParent->GetExtantDoc();
> +  if (doc) {
> +    return doc->RemoveSystemEventListener(kVisibilityChange,
> +                                          /* listener */ aListener,

(same)

::: dom/webauthn/WebAuthnManager.cpp:268
(Diff revision 1)
> +  if (mCurrentParent) {
> +    nsresult rv = StopListeningForVisibilityEvents(mCurrentParent, this);
> +    Unused << NS_WARN_IF(NS_FAILED(rv));
> +  }

We should probably also null `mCurrentParent`?

::: dom/webauthn/WebAuthnManager.cpp:269
(Diff revision 1)
> +    nsresult rv = StopListeningForVisibilityEvents(mCurrentParent, this);
> +    Unused << NS_WARN_IF(NS_FAILED(rv));

I think we should just make `ListenForVisibilityEvents()` and `StopListeningForVisibilityEvents()` void. Then we could in those functions `WARN_IF` doc=null and the call sites would look better.

::: dom/webauthn/WebAuthnManager.cpp:940
(Diff revision 1)
> +
>    if (mTransactionPromise) {
>      mTransactionPromise->MaybeReject(aError);
>    }
> +
> +  StartCancel();

If we have this here we will call `SendRequestCancel()` every time we abort, even from e.g. `FinishMakeCredential()`.

I think the `StartCancel()` call should go into `HandleEvent()` that then also calls `Cancel(NS_ERROR_ABORT)`.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to J.C. Jones [:jcj] from comment #1)
> This was bugging me, so I stole it, Tim. Here's a patch that accomplishes

Thanks btw :)

> the job... but I don't know how to test it with mochitests as-is. It's not
> really testable with the soft-token as-is, so this might be one that needs
> manual testing.

Right... so it actually is hard to write tests. I spoke too fast. A follow-up is probably good to investigate anyway.

Comment 5

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8893996 [details]
Bug 1383799 - Cancel WebAuthn operations on tab-switch

https://reviewboard.mozilla.org/r/165066/#review171612

I've filed the follow-up in Bug 1388854.

Thanks for the review!

> We should probably also null `mCurrentParent`?

good call!

> I think we should just make `ListenForVisibilityEvents()` and `StopListeningForVisibilityEvents()` void. Then we could in those functions `WARN_IF` doc=null and the call sites would look better.

This makes sense. I restructured it a bit; see what you think.

> If we have this here we will call `SendRequestCancel()` every time we abort, even from e.g. `FinishMakeCredential()`.
> 
> I think the `StartCancel()` call should go into `HandleEvent()` that then also calls `Cancel(NS_ERROR_ABORT)`.

I was thinking of this as a feature, not a bug, but I don't recall what case I was worried about. I went ahead and changed it as you said, and if we see lingering-blinks or something, we can re-assess.

Comment 6

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8893996 [details]
Bug 1383799 - Cancel WebAuthn operations on tab-switch

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165066/diff/1-2/

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8893996 [details]
Bug 1383799 - Cancel WebAuthn operations on tab-switch

https://reviewboard.mozilla.org/r/165066/#review172148

Good patch. 11/10

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7a53ff2f8cb
Cancel WebAuthn operations on tab-switch r=ttaubert

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f7a53ff2f8cb

Comment 10

[Matt Wobensmith [:mwobensmith][:matt:]]

As far as tests, this is hard to automate but not as hard to test manually. I believe we will need a whole suite of similar tests that involve typical user scenarios like this.

Comment 11

[Matt Wobensmith [:mwobensmith][:matt:]]

I am adding this to a list of potential tests, and these will (hopefully) become part of the certification process for future Fx builds.

Comment 12

[Matt Wobensmith [:mwobensmith][:matt:]]

I've added a test case in Test Rail (C65531) to explicitly cover this case, under the Web Authentication > Boundary Tests section.