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

Prevent incorrect usage of Token.for_user #804

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

pchiquet
Copy link

@pchiquet pchiquet commented May 2, 2024

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

@sevdog
Copy link

sevdog commented May 15, 2024

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

@pchiquet
Copy link
Author

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

@hakan-77
Copy link

hakan-77 commented Jul 3, 2024

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?

@lokeshwarobuli
Copy link

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 ??

@nahue
Copy link

nahue commented Jul 18, 2024

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

@hakan-77
Copy link

hakan-77 commented Aug 6, 2024

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

@nils-van-zuijlen
Copy link

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.

@sevdog
Copy link

sevdog commented Aug 7, 2024

This PR probabily is not going to be merged.

Please read these comments on the original issue: #779 (comment) and #779 (comment)

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

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

Successfully merging this pull request may close these issues.