pyjwt icon indicating copy to clipboard operation
pyjwt copied to clipboard

Remove **kwargs from encode() and decode()

Open vimalloc opened this issue 8 years ago • 8 comments

I recently had a situation where I was passing algorithm instead of algorithms to jwt.decode(). Due to the **kwargs eating the algorithm parameter and the decode method looking at the encoded JWT to figure out what type of algorithm to use, everything appeared to be working the way you would expect, even though it wasn't actually doing what I wanted it to.

If there wasn't the **kwargs there, my code would have raised a TypeError instead and immediately let me know I wasn't using the API correctly. Personally, I think that is very desirable.

Would you be interested in a patch to remove **kwargs and replace them all with named arguments instead?

vimalloc avatar Apr 27 '17 18:04 vimalloc

Glad I wasn't the only one who made that mistake :)

vimalloc avatar Jul 06 '17 05:07 vimalloc

It looks like we don't currently allow **kwargs on encode() but I do think we should definitely remove the **kwargs from decode().

This may be a non-trivial change to make since we currently have several arguments such as audience= and issuer= which are not currently explicitly enumerated in the function definitions for decode().

Give it a shot if you'd like. 👍

mark-adams avatar Nov 27 '17 04:11 mark-adams

This permissive function signature (along with a typo) resulted in what appears to be quite a serious vulnerability in the social-auth-core library:

https://github.com/python-social-auth/social-core/commit/eed3007c4ccdbe959b1a3ac83102fe869d261948#diff-c00186529f1572bf1eba22a82687e9d9bb4a5424a84f82a9e6f3bf41ce800e02L128

I believe this would allow someone to forge Apple ID auth tokens by using HS256 and Apple's public key, although I have not tested this.

The JWT spec's acceptance of token-specified algorithms has been the cause of a number of vulnerabilities, so there should be a good deal of caution around anything that could fail to pin the algorithms list. (I'd argue that the algorithms arg should be mandatory, in fact, but that's a ticket for another day... :-P)

[EDIT: Clarified that I meant that the JWT spec, not the pyjwt library, in last paragraph.]

timmc-edx avatar Jan 13 '21 17:01 timmc-edx

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

github-actions[bot] avatar Jun 24 '22 02:06 github-actions[bot]

This appears to still be an issue: https://github.com/jpadilla/pyjwt/blob/a863a7329027fe1e993c866374b195785978b2a9/jwt/api_jwt.py#L72

(I'm not sure it makes sense to have stalebot running on this one.)

timmc-edx avatar Jun 25 '22 13:06 timmc-edx

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

github-actions[bot] avatar Aug 25 '22 02:08 github-actions[bot]