1403279 - U2F RegisterResponse should have "version" field

Status: RESOLVED FIXED

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

Description

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

https://www.fidoalliance.org/specs/fido-u2f-v1.1-id-20160915/fido-u2f-javascript-api-v1.1-id-20160915.html#dictionary-u2fregisterrequest-members does show that the RegisterResponse WebIDL should have `version`, so we should emit it as well.

```
dictionary RegisterResponse {
    DOMString version;
    DOMString registrationData;
    DOMString clientData;
};
```

Comment 1

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

Originally noted from https://github.com/gavinwahl/django-u2f/issues/24#issuecomment-332276043

This does affect Firefox 57 and 58, and while this is a pref'd-off, experimental feature, it'd be good to fix it in 57 since the u2f4moz addon stopped working when we moved to Web Extensions.

Comment 2

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

Attachment: Bug 1403279 - Set U2F version field on RegisterResponse

The U2F specification defines the RegisterResponse.Version field as being set to
"U2F_V2" [1] on successful registrations, which we appear to have overlooked.

This sets the field and adds a few checks to the register test.


[1] https://www.fidoalliance.org/specs/fido-u2f-v1.1-id-20160915/fido-u2f-javascript-api-v1.1-id-20160915.html#dictionary-u2fregisterrequest-members

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

Comment 3

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

Comment on attachment 8913464 [details]
Bug 1403279 - Set U2F version field on RegisterResponse

https://reviewboard.mozilla.org/r/184796/#review189968

Looks good.

::: commit-message-6dea0:9
(Diff revision 1)
> +"U2F_V2" [1] on successful registrations, which we appear to have overlooked.
> +
> +This sets the field and adds a few checks to the register test.
> +
> +
> +[1] https://www.fidoalliance.org/specs/fido-u2f-v1.1-id-20160915/fido-u2f-javascript-api-v1.1-id-20160915.html#dictionary-u2fregisterrequest-members

Wouldn't this link: https://www.fidoalliance.org/specs/fido-u2f-v1.1-id-20160915/fido-u2f-javascript-api-v1.1-id-20160915.html#idl-def-RegisterResponse make more sense here?

Comment 4

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

Comment on attachment 8913464 [details]
Bug 1403279 - Set U2F version field on RegisterResponse

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

Comment 5

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

Comment on attachment 8913464 [details]
Bug 1403279 - Set U2F version field on RegisterResponse

https://reviewboard.mozilla.org/r/184796/#review189968

Thanks for the review!

> Wouldn't this link: https://www.fidoalliance.org/specs/fido-u2f-v1.1-id-20160915/fido-u2f-javascript-api-v1.1-id-20160915.html#idl-def-RegisterResponse make more sense here?

Yeah, good idea :)

Comment 6

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

Try run looks good [1] -- marking checkin-needed!

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ea0f9ebb3460f98d7729f7ed86e9c93f9cb52f8

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5a42ccee0a4
Set U2F version field on RegisterResponse r=keeler

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 9

[Justin]

As the person who notified J.C. about this issue, I want to thank everyone involved for fixing, reviewing, and merging this so quickly. Bravo!

On a related note, J.C. said:

> This does affect Firefox 57 and 58, and while this is a pref'd-off, experimental feature, it'd be good to fix it in 57 since the u2f4moz addon stopped working when we moved to Web Extensions.

I concur with J.C.'s assessment. It really would be ideal if this fix were included in Firefox 57.

To Wes and/or anyone else responsible for making such decisions, do you think it's possible to include this fix in Firefox 57?

Comment 10

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

(In reply to Justin from comment #9)
> To Wes and/or anyone else responsible for making such decisions, do you
> think it's possible to include this fix in Firefox 57?

I'll request uplift to 57 for it on Monday once it makes it through the weekend. It's always good to make sure any given change has some bake-time in Nightly before requesting. :)

Comment 11

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

Comment on attachment 8913464 [details]
Bug 1403279 - Set U2F version field on RegisterResponse

Approval Request Comment
[Feature/Bug causing the regression]: None, this was an oversight
[User impact if declined]: Experimental U2F support continues to miss this static-constant field. Sites depending on this behavior would have no U2F support until 58, as the u2f4moz add-on cannot be made into a web extension.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's a one-liner (with tests) to set a static constant which was overlooked in the spec.
[String changes made/needed]: None

Comment 12

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

(Oh, and this is a pref'd off experimental feature, also.)

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8913464 [details]
Bug 1403279 - Set U2F version field on RegisterResponse

Usually, we don't take uplifts for feature which are behind a pref but this is a trivial change with a test, so, taking it.
Should be in 57b5

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cfde20f68c1f