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

Updating PyJWT to 2.10.0 broke login #831

Open
jeremyqa opened this issue Nov 19, 2024 · 8 comments · May be fixed by #843
Open

Updating PyJWT to 2.10.0 broke login #831

jeremyqa opened this issue Nov 19, 2024 · 8 comments · May be fixed by #843
Labels

Comments

@jeremyqa
Copy link

jeremyqa commented Nov 19, 2024

I don't usually write issues, so bear with me. We noticed our dev environment suddenly wasn't letting anyone log in. It turns out it's because PyJWT updated to 2.10.0, and that began enforcing that subject be a string.

djangorestframework-simplejwt==5.3.1
├── Django [required: >=3.2, installed: 4.2.16]
│   ├── asgiref [required: >=3.6.0,<4, installed: 3.8.1]
│   └── sqlparse [required: >=0.3.1, installed: 0.5.2]
├── djangorestframework [required: >=3.12, installed: 3.14.0]
│   ├── Django [required: >=3.0, installed: 4.2.16]
│   │   ├── asgiref [required: >=3.6.0,<4, installed: 3.8.1]
│   │   └── sqlparse [required: >=0.3.1, installed: 0.5.2]
│   └── pytz [required: Any, installed: 2023.3]
└── PyJWT [required: >=1.7.1,<3, installed: 2.10.0]

In this section, the code only sometimes converts the id to a string

> /usr/local/lib/python3.11/site-packages/rest_framework_simplejwt/tokens.py(208)

 197         @classmethod
 198         def for_user(cls, user: AuthUser) -> "Token":
 199             """
 200             Returns an authorization token for the given user that will be provided
 201             after authenticating the user's credentials.
 202             """
 204             user_id = getattr(user, api_settings.USER_ID_FIELD)
 205             if not isinstance(user_id, int):
 206                 user_id = str(user_id)

That returns a seemingly fine token. However, when trying to use it, downstream from here

> /usr/local/lib/python3.11/site-packages/rest_framework_simplejwt/authentication.py(50)

  40         def authenticate(self, request: Request) -> Optional[Tuple[AuthUser, Token]]:
  42             header = self.get_header(request)
  43             if header is None:
  44                 return None
  45
  46             raw_token = self.get_raw_token(header)
  47             if raw_token is None:
  48                 return None
  49
  50  ->         validated_token = self.get_validated_token(raw_token)
  51
  52             return self.get_user(validated_token), validated_token

vv

> /usr/local/lib/python3.11/site-packages/rest_framework_simplejwt/tokens.py(56)

  37         def __init__(self, token: Optional["Token"] = None, verify: bool = True) -> None:
  38             """
  39             !!!! IMPORTANT !!!! MUST raise a TokenError with a user-facing error
  40             message if the given token is invalid, expired, or otherwise not safe
  41             to use.
  42             """
  43             if self.token_type is None or self.lifetime is None:
  44                 raise TokenError(_("Cannot create token with no type or lifetime"))
  45
  46             self.token = token
  47             self.current_time = aware_utcnow()
  48
  49             # Set up token
  50             if token is not None:
  51                 # An encoded token was provided
  52                 token_backend = self.get_token_backend()
  53
  54                 # Decode token
  55                 try:
  56  ->                 self.payload = token_backend.decode(token, verify=verify)
  57                 except TokenBackendError:
  58                     raise TokenError(_("Token is invalid or expired"))
  59

All the way down in

[42] > /usr/local/lib/python3.11/site-packages/jwt/api_jwt.py(167)decode_complete()
-> self._validate_claims(
 167  ->         self._validate_claims(
 168                 payload,
 169                 merged_options,
 170                 audience=audience,
 171                 issuer=issuer,
 172                 leeway=leeway,
 173                 subject=subject,
 174             )

it blows up

[43] > /usr/local/lib/python3.11/site-packages/jwt/api_jwt.py(273)_validate_claims()
-> self._validate_sub(payload, subject)
> /usr/local/lib/python3.11/site-packages/jwt/api_jwt.py(273)

 272             if options["verify_sub"]:
 273  ->             self._validate_sub(payload, subject)
 274
 275             if options["verify_jti"]:
 276                 self._validate_jti(payload)
InvalidSubjectError: Subject must be a string
 287         def _validate_sub(self, payload: dict[str, Any], subject=None) -> None:
 288             """
 289             Checks whether "sub" if in the payload is valid ot not.
 290             This is an Optional claim
 291
 292             :param payload(dict): The payload which needs to be validated
 293             :param subject(str): The subject of the token
 294             """
 295
 296             if "sub" not in payload:
 297                 return
 298
 299             if not isinstance(payload["sub"], str):
 300  ->             raise InvalidSubjectError("Subject must be a string")

(Pdb++) payload
{'token_type': 'access', 'exp': 1732058200, 'iat': 1732054600, 'jti': '537bae19596045f09177d29195d2ed71', 'sub': 3}

because sub is 3 and not "3", i guess.

Maybe if simplejwt passes verify_sub: False when it calls jwt.decode in /usr/local/lib/python3.11/site-packages/rest_framework_simplejwt/backends.py(139) it would work? PyJWT seems to merge options

 166             merged_options = {**self.options, **options}
 167  ->         self._validate_claims(
 168                 payload,
 169                 merged_options,
 170                 audience=audience,
 171                 issuer=issuer,
 172                 leeway=leeway,
 173                 subject=subject,
 174             )
 175
 176             decoded["payload"] = payload
 177             return decoded
(Pdb++) self.options
{'verify_signature': True, 'verify_exp': True, 'verify_nbf': True, 'verify_iat': True, 'verify_aud': True, 'verify_iss': True, 'verify_sub': True, 'verify_jti': True, 'require': []}
(Pdb++) options
{'verify_aud': False, 'verify_signature': True}
(Pdb++) merged_options
{'verify_signature': True, 'verify_exp': True, 'verify_nbf': True, 'verify_iat': True, 'verify_aud': False, 'verify_iss': True, 'verify_sub': True, 'verify_jti': True, 'require': []}

This seems to stem from the changes added in jpadilla/pyjwt#1005

@nagasiNR
Copy link

I had the same issue today.
In my Flask project I use a package flask_jwt_extended which has PyJWT dependency.
I set PyJWT version to 2.8.0 in a requirements.txt to prevent uploading the latest version (2.10.0) of PyJWT inside CI/CD builds.

@Nixxs
Copy link

Nixxs commented Dec 11, 2024

ahh ok yeah I've just run into this issue as well, also using flask_jwt_extended and found this issue today.

good to see I'm not the only one, I guess we enforce the previous version for now until this is fixed?

btw @nagasiNR I found that 2.9.0 works still so maybe you can try that one. but as soon as you hit 2.10.0 or 2.10.1 it breaks.

@Andrew-Chen-Wang
Copy link
Member

Andrew-Chen-Wang commented Jan 2, 2025

There was a PR #837 that tried to ignore the sub claim by @grayver . I believe a proper fix would be to stringify sub claim. A proper migration path forward would be:

  1. Upgrade SimpleJWT to a new version with the stringifying of the sub claim fix. I assume PyJWT <2.10 was stringifying the integer sub claim.
  2. Deploy your code to slowly migrate users.
  3. Once all users' JWTs are migrated to stringified sub claimed tokens, update PyJWT to 2.10.

The fix involves: upgrading SimpleJWT by a minor version, capping PyJWT to <2.10 due to this issue, then bumping SimpleJWT to a major version and removing the PyJWT version cap.

What are everyone's thoughts here on this migration path forward?

@grayver
Copy link

grayver commented Jan 4, 2025

I believe a proper fix would be to stringify sub claim.

Since RFC defines "sub" claim as a string, it should be string. But current version of SimpleJWT makes it integer out of the box, so stringifying it breaks back compatibility.

I assume PyJWT <2.10 was stringifying the integer sub claim.

No

I would consider the following patching way:

  1. In the new minor version of SimpleJWT either cap PyJWT to <2.10 or ignore sub claim check by PyJWT as I proposed in my MR.
  2. In the new major version of SimpleJWT stringify "sub" claim, add breaking changes warning and revert the change from 1.

@Andrew-Chen-Wang
Copy link
Member

Why should we ignore sub claim if PyJWT < 2.10? It's always been working on <2.10; even if it was silently failing, people could be relying on it. Removing stuff unnecessarily could be a breaking change too. We should simply cap the version. Lmk if I'm misunderstanding.

@AlexanderNeilson
Copy link

AlexanderNeilson commented Jan 5, 2025 via email

@grayver
Copy link

grayver commented Jan 5, 2025

Why should we ignore sub claim if PyJWT < 2.10?

We shouldn't. I proposed either pin PyJWT version to <2.10 or ignore sub claim check by PyJWT.

I agree that pinning PyJWT looks cleaner solution, so we can go this way in the next minor release.

But remove this pin in the next major release, where sub claim becomes a string.

@Andrew-Chen-Wang
Copy link
Member

It seems to me if the project is creating invalid JWT’s then if libraries are catching these are being tougher on this the user experience will get

Yes exactly. This is what I'm assuming happened as well for the sub field. A similar change was made during the PyJWT 1.7.1 to PyJWT 2.0 transition as well where they required a string rather than any serializable type for many of their parameters.

I agree that pinning PyJWT looks cleaner solution, so we can go this way in the next minor release.
But remove this pin in the next major release, where sub claim becomes a string.

Perfect thanks for the review @grayver Let's go with this path.

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