Validate the JSON data in load#36
Conversation
Check to make sure that the loaded JSON actually contains data in the keys we are going to use before we use them, to avoid runtime exceptions on Nil.
wallin
left a comment
wallin
left a comment
There was a problem hiding this comment.
Hey @ashleym1972, thank for you contribution. Would you mind adding a test for this.
lib/u2f/register_response.rb
Outdated
| end | ||
|
|
||
| if data['clientData'].blank? || data['registrationData'].blank? | ||
| raise RegistrationError, code: 2 |
There was a problem hiding this comment.
Put the code in a constant, for clarity
When trying to sign response make sue the data is correct and raise a known error rather than a method missing error on bad data.
1 similar comment
|
Looks good to me. @mastahyeti any thoughts? |
btoews
left a comment
btoews
left a comment
There was a problem hiding this comment.
This seems okay. Validating a JSON schema would be more robust, but would probably be overkill. Also, can you add a similar check to ClientData.load_from_json while you're at it?
lib/u2f/sign_response.rb
Outdated
|
|
||
| def self.load_from_json(json) | ||
| data = ::JSON.parse(json) | ||
| if data['clientData'].nil? || data['keyHandle'].nil? || data['signatureData'].nil? |
There was a problem hiding this comment.
Why are you doing a #nil? check in one place and just checking that the key exists in the other?
There was a problem hiding this comment.
I'm not sure which one people here prefer.
There was a problem hiding this comment.
Whichever, but it would be nice to be consistent if the intent is the same.
lib/u2f/register_response.rb
Outdated
| end | ||
|
|
||
| if !data.key?('clientData') || !data.key?('registrationData') | ||
| raise RegistrationError, code: BAD_REQUEST |
There was a problem hiding this comment.
These error codes normally come from the browser. It might make more sense to pass a message: argument instead of a code:.
There was a problem hiding this comment.
Surprisingly that is not my use case. Additionally, the Rails / browser mind set in this library is a very bad idea.
There was a problem hiding this comment.
The error codes are specific to the JavaScript API (here). It would make more sense to specify an error message here than to reuse the codes from the JS API.
1 similar comment
|
What's still missing here? |
|
Sorry dropped the ball on this one. Merging |
|
Thanks @wallin - this library is great. |
5 participants
Check to make sure that the loaded JSON actually contains data in the keys we are going to use before we use them, to avoid runtime exceptions on Nil.