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.

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

Merged
merged 1 commit into from Jul 17, 2022

Conversation

cmrd-senya
Copy link
Contributor

This PR fixes an issue that was introduced at #425

Currently if one passes a token of 2 segments without alg header the decode method raises NoMethorError because algorithm is nil but it tries to call casecmp 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 like NoMethodError.

@@ -113,6 +113,8 @@ def segment_length
end

def none_algorithm?
return false unless algorithm.respond_to? :casecmp
Copy link
Member

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)

Copy link
Member

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'

Copy link
Contributor Author

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.

image

And none is listed as an algorithm name:

image

So I guess supporting something like None or nOnE could be seen as a spec violation.

https://datatracker.ietf.org/doc/html/rfc7518#section-7.1.1

@anakinj
Copy link
Member

anakinj commented Jul 15, 2022

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.

@cmrd-senya
Copy link
Contributor Author

@anakinj updated

@anakinj
Copy link
Member

anakinj commented Jul 17, 2022

Looks great. Would you be so kind and add a changelog entry to the CHANGELOG.md under the "Fixes and enhancements"?

@cmrd-senya
Copy link
Contributor Author

@anakinj done

@anakinj anakinj merged commit 7c29ccd into jwt:master Jul 17, 2022
@cmrd-senya cmrd-senya deleted the missing-algorithm-exception branch July 17, 2022 18:13
@anakinj
Copy link
Member

anakinj commented Jul 17, 2022

Super! Thank you for the effort fixing this!❤️

@excpt excpt added this to the 2.5.0 milestone Jul 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants