-
Notifications
You must be signed in to change notification settings - Fork 376
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
Fix NoMethodError when a 2 segment token is missing 'alg' header #502
Conversation
lib/jwt/decode.rb
Outdated
@@ -113,6 +113,8 @@ def segment_length | |||
end | |||
|
|||
def none_algorithm? | |||
return false unless algorithm.respond_to? :casecmp |
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.
I wonder if it would just be simpler to check if it's a string or just is truthy.
return false unless algorithm
return false unless algithm.is_a?(String)
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.
Maybe even the original approach would be the simplest.
algorithm == 'none'
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.
Checking for a specific type is not very Ruby-way approach. Because Ruby promotes duck typing, and therefore it shouldn't matter if a class is directly based on a String
or it implements exactly the same interface independently - the code should work anyways. So checking for the presence of a certain method is quite common in Ruby to ensure the interface of the object is supported, I think it is intentionally preferred over checking the actual class.
Checking algorithm == 'none'
is fine, the only thing is that it stops supporting case-insensitive matches, but I have quickly checked the RFC and it never explicitly requires to support anything other than none
.
I have also found that algorithm name is supposed to be case sensitive.
And none
is listed as an algorithm name:
So I guess supporting something like None
or nOnE
could be seen as a spec violation.
Great catch and nice with additional tests to ensure this behaviour in the future. Added a comment about the added condition. Would suggest making the check more generic to avoid checking for a certain method. |
0eb6e1f
to
ed6cbb4
Compare
@anakinj updated |
Looks great. Would you be so kind and add a changelog entry to the |
ed6cbb4
to
9dae74d
Compare
@anakinj done |
Super! Thank you for the effort fixing this!❤️ |
This PR fixes an issue that was introduced at #425
Currently if one passes a token of 2 segments without
alg
header thedecode
method raisesNoMethorError
becausealgorithm
isnil
but it tries to callcasecmp
on it here:https://github.com/jwt/ruby-jwt/blob/master/lib/jwt/decode.rb#L116
The expected result would be to raise an exception that is inherited from
JWT::DecodeError
instead of a generic exception likeNoMethodError
.