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.

Sign up for GitHub

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

Jump to bottom

fix #792: use utf 8 decode directly #793

Merged
merged 8 commits into from
Feb 10, 2018
Merged

fix #792: use utf 8 decode directly #793

merged 8 commits into from
Feb 10, 2018

Conversation

equalsJeffH
Copy link
Contributor

@equalsJeffH equalsJeffH commented Feb 9, 2018 •
edited by pr-preview bot
Loading

fixes #792

Preview | Diff

@equalsJeffH equalsJeffH added this to the CR milestone Feb 9, 2018
@equalsJeffH equalsJeffH self-assigned this Feb 9, 2018
@equalsJeffH equalsJeffH mentioned this pull request Feb 9, 2018
Closed
emlun
emlun requested changes Feb 9, 2018
View reviewed changes
Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the intent, but I think this makes it seem like |C| is to be processed as text rather than a JSON data structure. I think we need some mention of parsing JSON in there still.

@equalsJeffH
Copy link
Contributor Author

agreed, added explicit JSON parsing step

emlun
emlun approved these changes Feb 9, 2018
View reviewed changes
Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although there are probably a few hard-coded step references after this that need to be updated.

@emlun
Copy link
Member

emlun commented Feb 9, 2018

The assertion verification algorithm might need this change, too.

@nadalin
Copy link
Contributor

nadalin commented Feb 9, 2018

@selfissued @akshayku Please review

@equalsJeffH
Copy link
Contributor Author

@emlun

there are probably a few hard-coded step references after this that need to be updated.

doh! thx!

selfissued
selfissued approved these changes Feb 9, 2018
View reviewed changes
Copy link
Contributor

@selfissued selfissued left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@equalsJeffH
Copy link
Contributor Author

equalsJeffH commented Feb 9, 2018 •
edited
Loading

@emlun

The assertion verification algorithm might need this change, too.

indeed, it did, done. double-checked inter-step references. fixed up denotation of |C| as client data (rather than the intermediate |JSONtext|).

@equalsJeffH
Copy link
Contributor Author

would be good to get a careful double-double check on this to see if I messed anything else up thx. :)

selfissued
selfissued approved these changes Feb 9, 2018
View reviewed changes
Copy link
Contributor

@selfissued selfissued left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@nadalin
Copy link
Contributor

nadalin commented Feb 10, 2018

@equalsJeffH Please merge

@nadalin
Copy link
Contributor

nadalin commented Feb 10, 2018

@selfissued can you merge as we want to generate a document

@selfissued selfissued merged commit 57ab456 into master Feb 10, 2018
WebAuthnBot pushed a commit that referenced this pull request Feb 10, 2018
Built by Travis-CI: 57ab456 fix #792: use utf 8 decode directly (#793)
5c0aa5e
@equalsJeffH equalsJeffH mentioned this pull request Feb 12, 2018
Closed
@emlun emlun deleted the jeffh-fix-792-use-utf-8-decode-directly branch June 12, 2019 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@selfissued selfissued selfissued approved these changes

@emlun emlun emlun approved these changes

@akshayku akshayku Awaiting requested review from akshayku

Assignees

@equalsJeffH equalsJeffH

Labels
type:technical
Projects
None yet
Milestone
Last Working Draft
Development

Successfully merging this pull request may close these issues.

reference "UTF-8 decode" directly rather than "parse JSON from bytes"
4 participants
@equalsJeffH @emlun @nadalin @selfissued