Change origin/RP ID checks to align better with the spec. #159
Conversation
| end | ||
|
|
||
| def valid_rp_id?(rp_id) | ||
| # TODO: rp_id cannot be a public suffix |
There was a problem hiding this comment.
Ideally, this line would include a check against a public suffix library. I shied away from putting that in the initial PR.
|
Also, note:
|
|
Landed :-) 👍 |
| @@ -4,6 +4,9 @@ | |||
|
|
|||
| module WebAuthn | |||
| class VerificationError < Error; end | |||
| # `UnspecifiedRpIdError` is a caller error rather than an error in the webauthn | |||
| # response. So we directly subclass `Error`. | |||
| class UnspecifiedRpIdError < Error; end | |||
There was a problem hiding this comment.
This happens if the caller doesn't specify either of expected_origin or rp_id. I figured it didn't make sense to call this a VerificationError — perhaps it should be an ArgumentError?
| client_data.origin == expected_origin | ||
| def valid_origin?(rp_id, expected_origin) | ||
| # If the caller specifies the expected origin, check that it exactly matches the signed origin. | ||
| return false unless expected_origin.nil? || client_data.origin == expected_origin |
There was a problem hiding this comment.
It would be nice to distinguish between the failure cases. Would it make sense to subclass OriginVerificationError even further?
- Rename the `original_origin` parameter to `current_origin`. - Make the `current_origin` parameter optional by defaulting to `nil` - Verify the signature origin against the RP ID. Addresses issue #150.
|
Hi @lgarron, Thank you for the PR. Isn't this change relaxing the "origin" check in a way we no longer comply with step 5 of "Registering a new credential"
and step 9 of "Verifying an Authentication Assertion"
? |
|
Hmm, I was assuming the origin would be tied to the RP ID, but I guess it's not. The RP ID should still be an ancestor domain of |
|
👍 |
original_originparameter tocurrent_origin.current_originparameter optional by defaulting tonilAddresses issue #150.
This is an attempt to preserve backwards compatibility, while: