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

reference "UTF-8 decode" directly rather than "parse JSON from bytes" #792

Closed
equalsJeffH opened this issue Feb 9, 2018 · 2 comments · Fixed by #793
Closed

reference "UTF-8 decode" directly rather than "parse JSON from bytes" #792

equalsJeffH opened this issue Feb 9, 2018 · 2 comments · Fixed by #793

Comments

@equalsJeffH
Copy link
Contributor

equalsJeffH commented Feb 9, 2018

we recently insterted:

Let |C|, the [=client data=] claimed as collected during the credential creation, be the result of [=parse JSON from bytes|parsing JSON from the bytes=] in |response|.{{AuthenticatorResponse/clientDataJSON}}.

However, this is most likely invoked RP-server-side, and is JS-specific (as we discussed). I'm thinking we can make it implementation-non-specific by referencing https://encoding.spec.whatwg.org/#utf-8-decode, which is what parse JSON from bytes does in any case, and indicating that any impl is fine as long as the end results are the same as UTF-8 decode provides.

See PR #793

also fixes #712

@equalsJeffH equalsJeffH added this to the CR milestone Feb 9, 2018
@equalsJeffH equalsJeffH self-assigned this Feb 9, 2018
@emlun
Copy link
Member

emlun commented Feb 9, 2018

I agree with making it not JS specific. Just note that this is not only UTF decoding, but also JSON parsing after that.

@equalsJeffH
Copy link
Contributor Author

note that this is not only UTF decoding, but also JSON parsing after that.

ah, yes, will update PR accordingly

selfissued pushed a commit that referenced this issue Feb 10, 2018
* use UTF-8 decode alg directly

* grammatical addition

* ditto

* add JSON explicit parsing step

* fixup inter-step references, thx emlun!

* apply same changes to #verifying-assertion

* ident |C| as being client data
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.

2 participants