-
Notifications
You must be signed in to change notification settings - Fork 5
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
Passkey tests #231
Merged
Merged
Passkey tests #231
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of having the browser in the loop, we can just sign this ourselves. Just need to figure out the formats ;)
Something must be getting mixed up, because these recorded signatures already passed the simplewebauth verification checks so they should verify with the web crpyto libs. Will need to dig through that lib again to see what's going on compared to how I recorded these.
So there was issue where the signature was too long and it was hard to see, but now all the data formats are using raw bytes so should be directly compable. No clue as to what's incompatible between the two ES256 implementions
The problem before was double hash, the problem now is making bad s values during key generation
JSON parsing fails on duplicate keys, so it's impossible to get coverage on the duplicate key checks. Also explicitly not testing cross-origin, as we don't care anymore about it's value (unless we want to save that with the key, because it should otherwise be managed by the signer with the rp hosts)
Don't allow any invalid keys instead of just checking the most recent key in case there are duplicate keys in the JSON
Rename the variables and have the same range check as low S
MexicanAce
reviewed
Jan 6, 2025
Co-authored-by: Nicolas Villanueva <[email protected]>
Helps with consistency between the SDK and contract tests, also helps clean up the test code with a quick refactor
ly0va
approved these changes
Jan 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Instead of having the browser in the loop, we can just sign this ourselves to generate test coverage around the web auth n validation steps.
Additional context
We have e2e tests in another repo that will break if we fail to sign correctly, but we can exercise more of the validation without the browser in the loop to generate too much of the signatures.