-
Notifications
You must be signed in to change notification settings - Fork 374
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 signature has expired error if payload is a string #555
Conversation
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.
Im wondering if this is too big of a change for a minor version. This will require the @payload
to be a Hash or an object responding to has_key?
. I would also argue that it's totally fine to pass a string as the payload. The RFC states JWTs encode claims to be transmitted as a JSON [RFC7159] object that is used as the payload of a JSON Web Signature
@anakinj |
lib/jwt/verify.rb
Outdated
@@ -38,12 +38,12 @@ def verify_aud | |||
end | |||
|
|||
def verify_expiration | |||
return unless @payload.include?('exp') | |||
return unless @payload.key?('exp') |
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.
What if there would be a method on this class, something like
def contains_key?(payload, key)
payload.respond_to?(:key?) && payload.key?(key)
end
Then this row would be
return unless contains_key?(@payload, 'exp')
I think the proposed changes are great but we need to support the case when the passed payload is not responding to the To my understanding passing a string as the payload is not prohibited. What comes to the expiry, the ´exp´ claim (header) is optional for a JWT token. Added a little suggestion on how we could handle such cases. |
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.
Totally forgot to ask if there by any chance would be possible to have some kind of test to validate this change?
Great work and thanks for you patience |
currently it's possible to encode a string payload and decode it but decoding a string containing "exp" throws signature has expired payload which could be misleading as the payload has to be a hash inorder for this to work properly.
Possible before this PR:
Reason for making this PR is has_key will atleast throw undefined method error so that the error won't go unnoticed in production if the payload is a string by mistake.