security: add optional key parameter to JWToken for signature verification#4085
Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
dfgvaetyj3456356-hash
force-pushed
the
security/fix-jwt-signature-verification
branch
from
49b1761 to
6f0f6e8
Compare
…ignature verification Signed-off-by: Security Researcher <security@example.com>
dfgvaetyj3456356-hash
force-pushed
the
security/fix-jwt-signature-verification
branch
from
6f0f6e8 to
3d51ef7
Compare
…rovided When a key is provided, the code only disabled verify_exp but left verify_aud and verify_nbf at their PyJWT defaults of True. This caused DecodeError for any JWT containing an aud claim because no audience parameter was passed to jwt.decode(). Fix by adding verify_aud=False and verify_nbf=False to the options when key is provided, matching the behavior of the no-key path which used verify_signature=False to implicitly disable all claim verifications. Also add tests for JWT tokens with audience and nbf claims. Signed-off-by: Security Researcher <security@example.com>
petyaslavova
left a comment
petyaslavova
left a comment
There was a problem hiding this comment.
Hey @dfgvaetyj3456356-hash, thank you for your contribution!
Please take a look at the requested changes and additionally please fix linters and ensure all code paths are covered with tests.
With the current change, the old, still supported, code path is completely skipped in the tests.
We should have coverage for the old initialization, when only token is provided, as well as when we have just algorithms and key is None, and when both key and algorithm are provided.
Also the case when a key is provided and algorithms is None - then we should validate that an error is raised.
| REQUIRED_FIELDS = {"exp"} | ||
|
|
||
| def __init__(self, token: str): | ||
| def __init__(self, token: str, key: str = None, algorithms: list = None): |
There was a problem hiding this comment.
Please follow PEP 604 for the type hints
| ) | ||
| if key is not None: | ||
| if algorithms is None: | ||
| algorithms = ["HS256"] |
There was a problem hiding this comment.
When key is provided but algorithms is omitted, the code silently uses ["HS256"].
That default is risky and likely wrong for real IdP tokens; for example, Entra ID / Azure AD commonly uses RS256.
So in this case, it will be better to require a value for algorithms arg when key is provided.
A test for key provided without algorithms would make this behavior explicit and prevent the silent HS256 fallback from coming back later.
Please also add a docstring describing the behaviour and the arguments expectations.
Add a clear statement that this change doesn't completely resolve the security issue - using key and algorithms enables a safer path, but the old unsafe usage pattern is still available for compatibility.
| self._value, | ||
| key, | ||
| algorithms=algorithms, | ||
| options={"verify_exp": False, "verify_aud": False, "verify_nbf": False}, |
There was a problem hiding this comment.
Please don’t add verify_exp=False here. This disables expiration validation, so expired tokens can still decode successfully.
Expiration validation should remain enforced by default; otherwise, we introduce a new security problem by allowing expired tokens to be accepted.
| algorithms = [header["alg"]] | ||
| self._decoded = jwt.decode( | ||
| self._value, | ||
| options={"verify_signature": False, "verify_exp": False}, |
| token = {"exp": datetime.now(timezone.utc).timestamp() + 3600, "oid": "username"} | ||
| encoded = jwt.encode(token, "secret", algorithm="HS256") | ||
| jwt_token = JWToken(encoded) | ||
| jwt_token = JWToken(encoded, key="secret", algorithms=["HS256"]) |
There was a problem hiding this comment.
We still need to have tests with the old jwt_token style, so probably it will be good to introduce another mock and have tests covering both types of token initialization.
- Use PEP 604 type hints (str | None, list[str] | None) - Require algorithms when key is provided (raise ValueError otherwise) - Remove verify_exp=False from both decode paths (per maintainer: don't disable expiration validation) - Keep verify_signature=False only in backward-compat key=None path - Add docstring describing behavior and backward-compat path - Update tests for old init, key+algorithms, algorithms-only, and missing-algorithms error - Add mock_identity_provider_unsafe fixture for backward-compat testing
- Add mock_identity_provider_legacy() for backward-compat testing - Add tests for exp=-1 sentinel with both verified and legacy init - Add tests for audience claims with both init styles - Addresses maintainer feedback on PR redis#4085
- Modify entraid_utils.py to support mock_idp='legacy' - Add TestLegacyMockIdentityProvider with coverage for unverified JWToken - Addresses maintainer feedback on PR redis#4085
|
Opened #4093 as the replacement/update from a branch I can push to with the current GitHub account. It carries the maintainer-requested coverage for the legacy token-only path, algorithms-without-key path, key+algorithms path, key-without-algorithms error, and removes the optional redis_entraid integration test path that was only added for extra coverage. Local verification on the replacement branch: focused JWT tests, Ruff, and git diff --check all pass. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Reviewed by Cursor Bugbot for commit 7ee2ff9. Configure here.
| key, | ||
| algorithms=algorithms, | ||
| options={"verify_aud": False, "verify_nbf": False}, | ||
| ) |
There was a problem hiding this comment.
Future nbf tokens accepted verified
Medium Severity
When key and algorithms are used, jwt.decode sets verify_nbf to false, so tokens with a future nbf still decode. The legacy path leaves default nbf checking on. is_expired() does not enforce nbf, so verified initialization can accept tokens before they are meant to be valid.
Reviewed by Cursor Bugbot for commit 7ee2ff9. Configure here.
| else: | ||
| if algorithms is None: | ||
| header = jwt.get_unverified_header(self._value) | ||
| algorithms = [header["alg"]] |
There was a problem hiding this comment.
Missing alg header raises KeyError
Low Severity
On the no-key path, when algorithms is omitted, the code reads header["alg"] instead of using a safe lookup. JWTs without an alg header entry raise KeyError during construction rather than failing through jwt.decode as before.
Reviewed by Cursor Bugbot for commit 7ee2ff9. Configure here.
|
Closing this PR since all the changes are added in #4093 |
3 participants
This PR fixes a JWT signature verification bypass in JWToken.
CWE: CWE-345 (Insufficient Verification of Cryptographic Signature)
File: redis/auth/token.py
JWToken.init() previously decoded JWTs with verify_signature=False, meaning any forged token would be accepted regardless of the signing key. An attacker could craft arbitrary JWT payloads and have them accepted by downstream authorization decisions that rely on is_expired(), ttl(), and try_get().
Fix:
Note
Medium Risk
Fixes a real signature-bypass (CWE-345) in auth code, but callers who still use
JWToken(token)alone remain unverified until they passkeyandalgorithms.Overview
JWTokennow accepts optionalkeyandalgorithms. Whenkeyis set, PyJWT verifies the signature (and requiresalgorithms);aud/nbfchecks stay off. With nokey, behavior is unchanged: decode without signature verification for backward compatibility.Tests and mocks were updated so the default mock identity provider uses verified tokens; a
legacymock path and dedicated signature tests cover the old init and forged-token rejection.Reviewed by Cursor Bugbot for commit 7ee2ff9. Bugbot is set up for automated code reviews on this repo. Configure here.