djangorestframework-simplejwt icon indicating copy to clipboard operation
djangorestframework-simplejwt copied to clipboard

Prevent incorrect usage of Token.for_user

Open pchiquet opened this issue 9 months ago • 8 comments

Attempt to address #779

  • for_validated_user() replaces current for_user() method (no functionality change, it is just a rename)
  • for_user() now checks user.is_active flag thanks the api_settings.USER_AUTHENTICATION_RULE callable

pchiquet avatar May 02 '24 14:05 pchiquet

It is still possible to call the .for_validated_user method on every user (also disabled ones).

sevdog avatar May 15 '24 13:05 sevdog

It is still possible to call the .for_validated_user method on every user (also disabled ones).

yes, because for_validated_user is just a rename of the old for_user method.

What has changed:

  • for_validated_user is not documented (whereas for_user is mentioned in the documentation)
  • for_validated_user naming suggests that the user must be validated before using the method

pchiquet avatar May 15 '24 13:05 pchiquet

We have the security issue alerting for almost 4 months now: https://github.com/advisories/GHSA-5vcc-86wm-547q

Is there anything to be done to accelerate the merge and shipping of the new version?

hakan-77 avatar Jul 03 '24 17:07 hakan-77

We have the security issue alerting for almost 4 months now: GHSA-5vcc-86wm-547q

Is there anything to be done to accelerate the merge and shipping of the new version?

+1 can we get this shipped ??

lokeshwarobuli avatar Jul 16 '24 22:07 lokeshwarobuli

We have the security issue alerting for almost 4 months now: GHSA-5vcc-86wm-547q

Is there anything to be done to accelerate the merge and shipping of the new version?

+1

nahue avatar Jul 18 '24 12:07 nahue

Hi @nils-van-zuijlen is there anything remaining for this to be merged?

hakan-77 avatar Aug 06 '24 19:08 hakan-77

Hi @nils-van-zuijlen is there anything remaining for this to be merged?

I do not know, I'm not a member / I don't have merge rights.

nils-van-zuijlen avatar Aug 07 '24 06:08 nils-van-zuijlen

This PR probabily is not going to be merged.

Please read these comments on the original issue: https://github.com/jazzband/djangorestframework-simplejwt/issues/779#issuecomment-2088709374 and https://github.com/jazzband/djangorestframework-simplejwt/issues/779#issuecomment-2208889198

TL; DR: if you do not use JWTStatelessUserAuthentication you are not vulnerable, the advisory was incomplete.

sevdog avatar Aug 07 '24 10:08 sevdog