Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is not entirely correct. By default no token is rejected based on an algorithm value alone. The specified algorithms is matched against a list of available algorithms
GetSigningMethod
:jwt/signing_method.go
Line 30 in 62e504c
After this step, the key returned in
keyFunc
is forwarded to the signing method implementation, which checks whether the type of key is suitable for that particular method or not.In order to avoid simple mistakes, the signing method of
none
only allows keys of a very special global variablejwt.UnsafeAllowNoneSignatureType
.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 think I captured this, but just not that intricate detail. The main reason why I made the change here is that from the original comment it is not quite clear if the Parse function will be secure by default or not. It just mentions a rather unclear recommendation "caller is strongly encouraged to set the WithValidMethods option".
But hey, if you have a better idea for wording I'm all for it. I'm just looking to make the documentation clearer for users.
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.
Understood. I will try to come up with a very condensed version of what I wrote for the doc. The issue that I have with the wording "When a keyFunc is set and a tokenString is provided, the library will not accept the 'none' type algorithm, ensuring security by default" is that strictly speaking is not true. If you have a keyFunc that returns
jwt.UnsafeAllowNoneSignatureType
it will accept the 'none' type. The 'none' type does not have any special handling.