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

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

Open
vimalloc opened this issue Apr 27, 2017 · 7 comments
Open

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

vimalloc opened this issue Apr 27, 2017 · 7 comments

Comments

@vimalloc
Copy link
Contributor

vimalloc commented Apr 27, 2017

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
Copy link
Contributor Author

vimalloc commented Jul 6, 2017

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

@mark-adams mark-adams changed the title (Question) Removal of **kwargs? Remove **kwargs from encode() and decode() Aug 26, 2017
@mark-adams
Copy link
Contributor

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. 👍

@timmc-edx
Copy link

timmc-edx commented Jan 13, 2021

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

python-social-auth/social-core@eed3007#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.]

@github-actions
Copy link

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 github-actions bot added the stale Issues without activity for more than 60 days label Jun 24, 2022
@timmc-edx
Copy link

This appears to still be an issue:

**kwargs,

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

@jpadilla jpadilla removed the stale Issues without activity for more than 60 days label Jun 25, 2022
@github-actions
Copy link

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 github-actions bot added the stale Issues without activity for more than 60 days label Aug 25, 2022
@jpadilla jpadilla added keep and removed stale Issues without activity for more than 60 days labels Aug 25, 2022
@cleder
Copy link
Contributor

cleder commented Oct 1, 2024

Should the PR be made against master ? It seems the 2.0.0 branch is stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants