1409202 - Web Authentication - Restrict to active documents

Status: RESOLVED FIXED

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

Description

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

Credential Management defines that if the responsible document is not the active document in a top-level browsing context, then operations should return NotSupportedError [1][2].

The top-level browsing context is being handled in Bug 1407789.

[1] https://w3c.github.io/webappsec-credential-management/#algorithm-request
[2] https://w3c.github.io/webappsec-credential-management/#algorithm-create

Comment 1

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

I believe the algorithm from Bug 1407789 handles this after all, as the definition of "active document" is apparently more narrow than I had thought when making this bug.

I'm going to wontfix this for now, as the stated ambition is met. That said, we might need to repurpose this bug in the future to also abort Web Authentication if the browser itself isn't at the focus of the window manager. That concept currently isn't in HTML, so that is also not how the spec is written as of now, but that might change.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> I believe the algorithm from Bug 1407789 handles this after all

I don't see how it can.  For example, it would not handle it for a toplevel non-active thing, afaict.

Comment 3

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

Boris: 

Yeah, intuitively I feel like this is supposed to be about window managers, but that doesn't appear to be what the text says, unless I just don't understand browsing contexts... but to be honest, I don't think I have the nuance there.

Can you give me a brief rundown of what object I should check in CredentialsContainer's IsSameOriginWithAncestors[1] to properly check the Active Document[2] per the algorithm in 2.1 of Credential Management[3]? I'm sure I can figure it out if you point me in the right direction.

[1] https://searchfox.org/mozilla-central/source/dom/credentialmanagement/CredentialsContainer.cpp#44
[2] https://html.spec.whatwg.org/multipage/browsers.html#active-document
[3] https://w3c.github.io/webappsec-credential-management/#core-infrastructure

Comment 4

[Boris Zbarsky [:bzbarsky]]

> intuitively I feel like this is supposed to be about window managers

No, it's about whether a page has been navigated away from or not.

> what object I should check in CredentialsContainer's IsSameOriginWithAncestors[1]
> to properly check the Active Document

Hmm, this is never checking anything about whether the responsible document is the active document, unlike comment 0.

To implement the spec algorithm at https://w3c.github.io/webappsec-credential-management/#same-origin-with-its-ancestors you would want to walk up either the outer window or docshell parent chain (using nsGlobalWindowOuter::GetScriptableParent in the former case, which is probably simpler to do than the latter case) and look at the origins of the documents in those (nsGlobalWindowOuter::GetDocument).  The algorithm we have in IsSameOriginWithAncestors does something quite different, and gives very different answers in some cases.

Of course I can't tell whether the spec algorithm is actually the desired one...

(We should probably try to add some web platform tests here too...  Not sure how easy this stuff is to test.)

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1409202-Web-Authentication-Restrict-to-active-do.patch

This patch restricts call to navigator.credentials.* to active documents and aborts WebAuthn requests whenever the parent chrome window loses focus, or the tab/browser is backgrounded.

The latter part isn't defined by the spec, although it probably should be at some point.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8956508 [details] [diff] [review]
0001-Bug-1409202-Web-Authentication-Restrict-to-active-do.patch

As Boris is out... Olli, can you please check that this is sound? Especially the change to nsGlobalWindowOuter.cpp?

Comment 7

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

Comment on attachment 8956508 [details] [diff] [review]
0001-Bug-1409202-Web-Authentication-Restrict-to-active-do.patch

Review of attachment 8956508 [details] [diff] [review]:
-----------------------------------------------------------------

Everything looks good, but I've enough concerns about nsGlobalWindowOuter to mark r- for right now. Olli can probably set me straight that my concerns are crazy and based on me not knowing how this stuff works. :)

::: dom/base/nsGlobalWindowOuter.cpp
@@ +6764,5 @@
> +    if (res.Failed()) {
> +      return;
> +    }
> +
> +    event->InitEvent(NS_LITERAL_STRING("MozWindowBackgrounded"), true, true);

I don't understand the events system well, but it seems like this should not be cancelable.

@@ +6777,1 @@
>    mIsBackground = aIsBackground;

It seems like if we return early from the new code above, we still want to set this so that the window state doesn't become weird.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8956508 [details] [diff] [review]
0001-Bug-1409202-Web-Authentication-Restrict-to-active-do.patch

Review of attachment 8956508 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindowOuter.cpp
@@ +6764,5 @@
> +    if (res.Failed()) {
> +      return;
> +    }
> +
> +    event->InitEvent(NS_LITERAL_STRING("MozWindowBackgrounded"), true, true);

Good point.

@@ +6777,1 @@
>    mIsBackground = aIsBackground;

Good point as well.

Comment 9

[Phabricator Automation]

Attachment: Bug 1409202 - Web Authentication - Restrict to selected tabs in the active window

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8956559 [details]
Bug 1409202 - Web Authentication - Restrict to selected tabs in the active window

(r? here too for visibility.)

Comment 11

[Boris Zbarsky [:bzbarsky]]

So just to be clear, this is _not_ using the spec concept of "active document", right?  It's probably best to not use "active document" in the commit message....

Comment 12

[Phabricator Automation]

Comment on attachment 8956559 [details]
Bug 1409202 - Web Authentication - Restrict to selected tabs in the active window

J.C. Jones [:jcj] has approved the revision.

https://phabricator.services.mozilla.com/D688

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8956559 [details]
Bug 1409202 - Web Authentication - Restrict to selected tabs in the active window

I couldn't figure out how to r- in Phabricator, so doing it here.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11)
> So just to be clear, this is _not_ using the spec concept of "active
> document", right?  It's probably best to not use "active document" in the
> commit message....

Yeah... I'll reword the message.

Comment 15

[Phabricator Automation]

Comment on attachment 8956559 [details]
Bug 1409202 - Web Authentication - Restrict to selected tabs in the active window

J.C. Jones [:jcj] has been removed from the revision.

https://phabricator.services.mozilla.com/D688

Comment 16

[Phabricator Automation]

Comment on attachment 8956559 [details]
Bug 1409202 - Web Authentication - Restrict to selected tabs in the active window

J.C. Jones [:jcj] has approved the revision.

https://phabricator.services.mozilla.com/D688

Comment 17

[Phabricator Automation]

Comment on attachment 8956559 [details]
Bug 1409202 - Web Authentication - Restrict to selected tabs in the active window

J.C. Jones [:jcj] has been removed from the revision.

https://phabricator.services.mozilla.com/D688

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8956559 [details]
Bug 1409202 - Web Authentication - Restrict to selected tabs in the active window

Phabricator and review flags are weird...

Comment 19

[Phabricator Automation]

Comment on attachment 8956559 [details]
Bug 1409202 - Web Authentication - Restrict to selected tabs in the active window

J.C. Jones [:jcj] has approved the revision.

https://phabricator.services.mozilla.com/D688

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Olli, I'm not sure if you intentionally resigned as a reviewer or whether you wanted to do a second pass? I incorporated your feedback and there are no changes outside dom/{credentialmanagement,webauthn}/ anymore and it's all working as intended. Let me know, I'll do whatever is required to get this into Fx 60. Thanks! :)

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8956559 [details]
Bug 1409202 - Web Authentication - Restrict to selected tabs in the active window

Let's land this as is so that it makes Fx 60. I'm happy to work on and uplift any feedback afterwards.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd8202671cf1
Web Authentication - Restrict to selected tabs in the active window r=jcj

Comment 23

[Noemi Erli[:noemi_erli]]

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

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Hmm, where did I resigned as reviewer for that latter version?

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #24)
> Hmm, where did I resigned as reviewer for that latter version?

D688 still lists you as resigned, sorry if I interpreted that wrong.

Comment 26

[Chris Mills [:cmills]]

I've added a note to cover this here:

https://developer.mozilla.org/en-US/docs/Web/API/Web_Authentication_API#Browser_compatibility

I've not mentioned it in the release notes, as I believe this is still only supported in Nightly, correct?

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #26)
> I've not mentioned it in the release notes, as I believe this is still only
> supported in Nightly, correct?

It's going to ship with Fx 60.