U2F RegisterResponse should have "version" field

RESOLVED FIXED in Firefox 57

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jcj, Assigned: jcj)

Tracking

(Blocks: 1 bug)

57 Branch
mozilla58
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox57 fixed, firefox58 fixed)

Details

(Whiteboard: [u2f], URL)

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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;
};
```
(Assignee)

Comment 1

2 years ago
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.
status-firefox57: --- → affected
status-firefox58: --- → affected
Comment hidden (mozreview-request)
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?
Attachment #8913464 - Flags: review?(dkeeler) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
mozreview-review-reply
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 :)
(Assignee)

Comment 6

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

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

Comment 7

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f5a42ccee0a4
Set U2F version field on RegisterResponse r=keeler
Keywords: checkin-needed

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f5a42ccee0a4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

Comment 9

2 years ago
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?
(Assignee)

Comment 10

2 years ago
(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. :)
(Assignee)

Comment 11

2 years ago
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
Attachment #8913464 - Flags: approval-mozilla-beta?
(Assignee)

Comment 12

2 years ago
(Oh, and this is a pref'd off experimental feature, also.)
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
Attachment #8913464 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 14

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/cfde20f68c1f
status-firefox57: affected → fixed
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.