Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Length restriction on user handles has no processing model associated with it #1302

Closed
bzbarsky opened this issue Sep 18, 2019 · 13 comments · Fixed by #1539
Closed

Length restriction on user handles has no processing model associated with it #1302

bzbarsky opened this issue Sep 18, 2019 · 13 comments · Fixed by #1539

Comments

@bzbarsky
Copy link

https://w3c.github.io/webauthn/#dom-publickeycredentialuserentity-id says:

A user handle is an opaque byte sequence with a maximum size of 64 bytes

but nothing says where or whether that maximum size is enforced. Based on code inspection, Firefox checks for this in code that gets called from CredentialsContainer.create and throws an exception (a DOMException with name "TypeError") if the user handle is longer than 64 bytes. Also based on code inspection it looks like Chrome throws a JS TypeError in this situation (with a comment about https://www.w3.org/TR/webauthn/#user-handle).

Note that the lack of clear processing model leads to lack of interop here: the two browsers are throwing totally different kinds of exceptions...

@jcjones

@bzbarsky
Copy link
Author

Also, disabling this check in Firefox does not obviously lead to any web platform test failures, unless I am missing something....

@nadalin nadalin added this to the L2-WD-02 milestone Sep 20, 2019
@equalsJeffH
Copy link
Contributor

@jcjones notes at webauthn f2f 20-Sep-2019 that this underscores the desirability for landing PR #1256 and being able construct viable Web Platform Tests.

@nadalin nadalin modified the milestones: L2-WD-02, L2-CR Oct 16, 2019
@agl agl assigned agl and unassigned jcjones Nov 18, 2020
@nadalin nadalin assigned jcjones and unassigned jcjones Nov 18, 2020
@nsatragno
Copy link
Member

We already have a WPT that checks for this. I think this is good enough - closing the issue.

@bzbarsky
Copy link
Author

Just because there are tests doesn't mean the spec actually defines the behavior the tests are testing for. And in particular, the test is testing that a TypeError is thrown, but the spec doesn't actually say that!

That is, the test is not actually based on the spec, so it's not a valid test. Please do fix the spec. @nsatragno I can't reopen this issue, unfortunately; can you please do that?

@agl
Copy link
Contributor

agl commented Nov 20, 2020

On the 2020-11-18 call the WG decided that if a WPT existed for this then the issue would be closed. There are lots of requirements in the spec that are not explicitly tested for in the processing steps and the value in writing them all out does not seem worthwhile.

@bzbarsky
Copy link
Author

That is so completely backwards. The point of a spec is that it needs to specify what an implementation is supposed to do. A spec that does not do that is not actually usable for implementation. The tests can be used to validate implementations, but the tests are not the spec; they have no normative value and can be wrong, not match the spec, etc.

@annevk I don't know whether Mozilla's W3C rep cares about this, but they should at least know about it.

@bzbarsky
Copy link
Author

To be clear, the issue here is not that the requirement is not explicitly "tested for in the processing steps"; the issue is that requirement does not actually define what the behavior should be!

@nsatragno
Copy link
Member

Friendly reminder to follow the code of ethics and professional conduct for all w3c communications.

@othermaciej
Copy link
Member

I agree with @bzbarsky. Tests are supposed to test normative requirements defined by the spec. While tests are a great help to interoperability, it’s actively harmful to have the tests checks for things that are not normative requirements of the spec. Normally, when it’s noticed that a test checks for something not in the spec, either the spec is fixed to specify what’s intended, or the test is removed or modified. That is how other WGs do it. WebAuthentication should not be an exception to that normal flow.

@jcjones
Copy link
Contributor

jcjones commented Nov 30, 2020

Per all the above comments. re-opening to re-triage

@jcjones jcjones reopened this Nov 30, 2020
@dveditz
Copy link
Member

dveditz commented Dec 3, 2020

NB that this issue is not about the difference between Chrome and Firefox behavior in this specific case: Firefox was changed to match the Chrome behavior in bug 1610732 last January. The point is that the behavior is not in the spec and can only be determined experimentally.

@agl
Copy link
Contributor

agl commented Dec 9, 2020

Neither of the working group member organisations who requested action here have actually proffered any pull requests thus I've attempted #1539 to move things forward.

@annevk
Copy link
Member

annevk commented Dec 10, 2020

I have to say I rather disagree with the idea that the onus is on those raising a problem to fix it. I'm reasonably certain that if you'll ask others in the Chrome team they'll agree that a specification that does not define its processing models fully has a problem. And that it's not necessarily up to those pointing that out to fix it.

equalsJeffH added a commit that referenced this issue Dec 10, 2020
* Add algorithm step to enforce specified limits on user.id.

Fixes #1302.

* fix broken step# reference due to added step above

Co-authored-by: JeffH <jdhodges@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants