Closed Bug 1409202 Opened 2 years ago Closed 2 years ago

Web Authentication - Restrict to active documents

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jcj, Assigned: ttaubert)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, Whiteboard: [webauthn][webauthn-wd07])

Attachments

(1 file, 1 obsolete file)

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
Priority: -- → P2
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.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1407789
> 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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
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
Flags: needinfo?(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.)
Flags: needinfo?(bzbarsky)
Blocks: 1430150
Blocks: 1417679
Assignee: nobody → ttaubert
Status: REOPENED → ASSIGNED
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.
Attachment #8956508 - Flags: review?(jjones)
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?
Attachment #8956508 - Flags: review?(bugs)
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.
Attachment #8956508 - Flags: review?(jjones) → review-
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.
Attachment #8956508 - Attachment is obsolete: true
Attachment #8956508 - Flags: review?(bugs)
Comment on attachment 8956559 [details]
Bug 1409202 - Web Authentication - Restrict to selected tabs in the active window

(r? here too for visibility.)
Attachment #8956559 - Flags: review?(bugs)
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 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
Attachment #8956559 - Flags: review+
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.
Attachment #8956559 - Flags: review?(bugs) → review-
(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 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
Attachment #8956559 - Flags: review+
Attachment #8956559 - Attachment description: Bug 1409202 - Web Authentication - Restrict to active documents → Bug 1409202 - Web Authentication - Restrict to selected tabs in the active window
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
Attachment #8956559 - Flags: review+
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
Attachment #8956559 - Flags: review+
Attachment #8956559 - Flags: review- → review?(bugs)
Comment on attachment 8956559 [details]
Bug 1409202 - Web Authentication - Restrict to selected tabs in the active window

Phabricator and review flags are weird...
Attachment #8956559 - Flags: review?(jjones)
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
Attachment #8956559 - Flags: review+
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! :)
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Attachment #8956559 - Flags: review?(jjones)
Attachment #8956559 - Flags: review?(bugs)
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd8202671cf1
Web Authentication - Restrict to selected tabs in the active window r=jcj
https://hg.mozilla.org/mozilla-central/rev/dd8202671cf1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla60
Hmm, where did I resigned as reviewer for that latter version?
Depends on: 1444605
(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.
Depends on: 1445242
Blocks: 1448408
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?
(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.
You need to log in before you can comment on or make changes to this bug.