1380421 - Regression: Tolerate origin RP IDs in WebAuthn

Status: RESOLVED FIXED

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

Description

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

A recent fixup commit [1] changed "RP ID" fields in WebAuthn to be domain strings rather than origins, which matches the current editor's draft of Web Authentication. Unfortunately, this is contrary to the interop WD-05, which requires they be origins.

We should be tolerant of origins for now, and file a follow-on to remove when we get past initial interop.

[1] https://hg.mozilla.org/mozilla-central/rev/e173fd86d931

Comment 1

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

Attachment: Bug 1380421 - Handle URIs for RP IDs (1/2)

A recent fixup commit [1] changed "RP ID" fields in WebAuthn to be domain
strings rather than origins, which matches the current editor's draft of
Web Authentication. Unfortunately, this is contrary to the interop WD-05,
which requires they be origins.

We should be tolerant of origins for now, and file a follow-on to remove when
we get past initial interop.

[1] https://hg.mozilla.org/mozilla-central/rev/e173fd86d931

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

Comment 2

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

Attachment: Bug 1380421 - Update test to handle origin RP IDs, too (2/2)

This adjusts tests to also check origin-based RP IDs, for interop
purposes. When we officially move up to WD-06, we'll want to remove these.

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

Comment 3

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8886658 [details]
Bug 1380421 - Handle URIs for RP IDs (1/2)

https://reviewboard.mozilla.org/r/157436/#review162608

LGTM - just a couple of comments.

::: commit-message-09a42:8
(Diff revision 1)
> +A recent fixup commit [1] changed "RP ID" fields in WebAuthn to be domain
> +strings rather than origins, which matches the current editor's draft of
> +Web Authentication. Unfortunately, this is contrary to the interop WD-05,
> +which requires they be origins.
> +
> +We should be tolerant of origins for now, and file a follow-on to remove when

Let's file it and reference it here.

::: dom/webauthn/WebAuthnManager.cpp:180
(Diff revision 1)
>      return NS_ERROR_FAILURE;
>    }
>  
> -  if (!html->IsRegistrableDomainSuffixOfOrEqualTo(aInputRpId, originHost)) {
> +  // WD-05 origin compatibility support - aInputRpId might be a URI/origin,
> +  // so catch that (Bug 1380421)
> +  nsAutoString inputRpId = nsAutoString(aInputRpId);

Seems like this would be simpler as `nsAutoString inputRpId(aInputRpId);` ?

::: dom/webauthn/WebAuthnManager.cpp:182
(Diff revision 1)
>  
> -  if (!html->IsRegistrableDomainSuffixOfOrEqualTo(aInputRpId, originHost)) {
> +  // WD-05 origin compatibility support - aInputRpId might be a URI/origin,
> +  // so catch that (Bug 1380421)
> +  nsAutoString inputRpId = nsAutoString(aInputRpId);
> +  nsCOMPtr<nsIURI> inputUri;
> +  if (!NS_FAILED(NS_NewURI(getter_AddRefs(inputUri), aInputRpId, nullptr, nullptr))) {

NS_SUCCEEDED?
Also, the `nullptr` arguments have defaults (of `nullptr`), so we probably don't have to even specify them.

Comment 4

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8886659 [details]
Bug 1380421 - Update test to handle origin RP IDs, too (2/2)

https://reviewboard.mozilla.org/r/157438/#review162612

Looks good - just a few questions.

::: dom/webauthn/tests/test_webauthn_sameorigin.html:148
(Diff revision 1)
>            return credm.create({publicKey: makeCredentialOptions})
>                        .then(arrivingHereIsBad)
>                        .catch(expectSecurityError);
>          },
> -        function () {
> -          // Test basic good call
> +        function() {
> +          // Test basic good call but using an origin (Bug 1380421)

Seems like this would make more sense to go right before the new testcase at the end.

::: dom/webauthn/tests/test_webauthn_sameorigin.html:153
(Diff revision 1)
> -          let publicKeyCredentialRequestOptions = {
> -            challenge: chall,
> -            rpId: document.domain,
> +          let rp = {id: window.origin};
> +          let makeCredentialOptions = {
> +            rp: rp, user: user, challenge: chall, parameters: [param]
> -            allowList: [gTrackedCredential]
>            };
> -          return credm.get({publicKey: publicKeyCredentialRequestOptions})
> +          return credm.create({publicKey: makeCredentialOptions})

Do we not still want this `credentials.get` testcase?

::: dom/webauthn/tests/test_webauthn_sameorigin.html:235
(Diff revision 1)
>            return credm.get({publicKey: publicKeyCredentialRequestOptions})
>                        .then(arrivingHereIsBad)
>                        .catch(expectSecurityError);
>          },
>          function () {
> -          SimpleTest.finish();
> +          // Test basic good call but using an origin (Bug 1380421)

Is the `SimpleTest.finish()` call not still needed?

Comment 5

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

Comment on attachment 8886659 [details]
Bug 1380421 - Update test to handle origin RP IDs, too (2/2)

https://reviewboard.mozilla.org/r/157438/#review162612

> Seems like this would make more sense to go right before the new testcase at the end.

Agreed, and I put comments around it that they should go away with Bug 1381126.

> Do we not still want this `credentials.get` testcase?

Oh, geez. Good catch, not sure how I whacked that. I put it back.

> Is the `SimpleTest.finish()` call not still needed?

It's not, it's actually a dupe call and it prompts a console warning.

Comment 6

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

Comment on attachment 8886658 [details]
Bug 1380421 - Handle URIs for RP IDs (1/2)

https://reviewboard.mozilla.org/r/157436/#review162608

Thanks for the reviews!

> Let's file it and reference it here.

Got it.

> Seems like this would be simpler as `nsAutoString inputRpId(aInputRpId);` ?

Heh, heh... *sheepish* Of course. Thanks :)

> NS_SUCCEEDED?
> Also, the `nullptr` arguments have defaults (of `nullptr`), so we probably don't have to even specify them.

When all you've got is `NS_FAILED`, everything looks like an `nsresult`. No, wait, that doesn't make sense. Anyway, yeah, hehe, I forgot about `NS_SUCCEEDED`. Fixed.

And good catch about the default args. That's nice and cleaner. :thumbsup:

Comment 7

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

Comment on attachment 8886658 [details]
Bug 1380421 - Handle URIs for RP IDs (1/2)

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

Comment 8

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

Comment on attachment 8886659 [details]
Bug 1380421 - Update test to handle origin RP IDs, too (2/2)

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

Comment 9

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

Try run looks good, marking checkin-needed. Thanks for the reviews!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15436a89237a
Handle URIs for RP IDs (1/2) r=keeler
https://hg.mozilla.org/integration/autoland/rev/03db2b65e5a6
Update test to handle origin RP IDs, too (2/2) r=keeler

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/15436a89237a
https://hg.mozilla.org/mozilla-central/rev/03db2b65e5a6