-
-
Notifications
You must be signed in to change notification settings - Fork 694
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
Comments
Glad I wasn't the only one who made that mistake :) |
It looks like we don't currently allow This may be a non-trivial change to make since we currently have several arguments such as Give it a shot if you'd like. 👍 |
This permissive function signature (along with a typo) resulted in what appears to be quite a serious vulnerability in the social-auth-core library: 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.] |
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 |
This appears to still be an issue: Line 72 in a863a73
(I'm not sure it makes sense to have stalebot running on this one.) |
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 |
Should the PR be made against |
I recently had a situation where I was passing
algorithm
instead ofalgorithms
tojwt.decode()
. Due to the**kwargs
eating thealgorithm
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?The text was updated successfully, but these errors were encountered: