Add Security Warning while using `for_user` method

[dmdhrumilmistry]

Add security warning in for_user documentation to avoid introducing vulnerability in other applications.

Security Warning: validate whether user is active or not before generating token for the user. This will only create token for the user, it doesn't perform user validation whether user is active or not.

[Andrew-Chen-Wang]

Thanks for filing. cc @robd003

This is more of a documentation thing as using .for_user directly is non-standard, but because of my very own telling to everyone to just use SimpleJWT classes directly, this mistake is attributed to me. Unfortunately, it works by design and is following DRY principles and separation of duties where the validation takes place on the serializer part, which is Django standard, but I'll add some documentation to warn users.

[sevdog]

This is an issue only if used with JWTStatelessUserAuthentication/JWTTokenUserAuthentication, while JWTAuthentication class is not affected since it queries the database to check if ther user is active.

The "bad configuration" is:

whenever the Token.for_user method is used without prior user validation and JWTStatelessUserAuthentication or JWTTokenUserAuthentication (which is an alias of ther previous one) authentication class is used.

[dmdhrumilmistry]

Thanks for filing. cc @robd003

This is more of a documentation thing as using .for_user directly is non-standard, but because of my very own telling to everyone to just use SimpleJWT classes directly, this mistake is attributed to me. Unfortunately, it works by design and is following DRY principles and separation of duties where the validation takes place on the serializer part, which is Django standard, but I'll add some documentation to warn users.

I understand but there are several projects using it directly without any kind of user's active status validation which makes those projects vulnerable.

[Andrew-Chen-Wang]

I'm all for adding a logger.error and logger.critical, but outside of that is changing functionality and design principles. I'm not used to CVE reports and actionables in an open source context, but this is my understanding of general good design and consistent behavior for developers. Let me know if that's not how I should be approaching this and why.

[dmdhrumilmistry]

I'm all for adding a logger.error and logger.critical, but outside of that is changing functionality and design principles. I'm not used to CVE reports and actionables in an open source context, but this is my understanding of general good design and consistent behavior for developers. Let me know if that's not how I should be approaching this and why.

Adding logs can help users to avoid such issues. If possible let's also add more secure examples in docs so it doesn't introduce vulns in their projects.

[igorer88]

@Andrew-Chen-Wang Why don't add an exception raised if using .for_user directly?

As @dmdhrumilmistry says

there are several projects using it directly without any kind of user's active status validation which makes those projects vulnerable.

and as the vuln doc says:
CVE-2024-22513

so people doesn't have to check this manually:
if user and user.is_active:

Because this line in the vuln makes it important I think:

This feature introduces the ability to use different subclasses of the Token class, such as RefreshToken, SlidingToken, AccessToken, and UntypedToken with for_user method. However, this enhancement also expands the potential attack surface.

And it won't be shown on vuln scanners.

[doctornkz-intelas]

Hello folks. How can we go forward with this issue? Our DataDog is giving MEDIUM level for this CVE and it's hard to ignore such cases. We are safe with that, but the issue is increasing noise from our monitoring. Thank you for your effort.

[alimony]

Adding that it might be a legal requirement for some users not to have this active CVE, for compliance reasons.

[Andrew-Chen-Wang]

Why don't add an exception raised if using .for_user directly?

There are valid use cases for utilizing .for_user directly.

so people doesn't have to check this manually:
if user and user.is_active:

Some project don't use is_active flag such as for GDPR reasons where a full deletion is required.

We have a utility user validation check function, but, as noted before, the problem is utilizing the experimental JWTStatelessUserAuthentication. This means implementing the validation check would incur a database query which completely defeats the purpose of the class.

Again, this is a documentation error, that I or any willing users can make a PR for during free time.

[Andrew-Chen-Wang]

Adding that it might be a legal requirement for some users not to have this active CVE, for compliance reasons.

Definitely a valid reason that this should be ack-ed. If it's not really noticeable yet, I've been fairly disengaged due to constrained time for a solid 2 years around SimpleJWT. If anyone would like to assist with a PR (I think our RTDocs are also broken possibly?), I'm happy to take a look.