Repository navigation
chore(sentryapps): Add exp validation and requriement for jwts - #121673
Conversation
| :param audience: Set this to the audience you expect to be present in the claims. Set | ||
| this to ``False`` to disable verifying the audience. |
There was a problem hiding this comment.
Bug: The new options parameter in jwt.decode can be used to pass {"verify_aud": False}, which bypasses audience verification even when an audience is specified.
Severity: MEDIUM
Suggested Fix
The jwt.decode function should be modified to ensure its internal audience verification logic cannot be overridden by the options parameter. When an audience is provided, explicitly set decode_options["verify_aud"] = True. Alternatively, disallow the verify_aud key from being passed in the options dictionary.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/sentry/utils/jwt.py#L64-L65
Potential issue: The introduction of the `options` parameter to the
`sentry.utils.jwt.decode` function allows a caller to bypass audience verification. If a
caller passes `options={"verify_aud": False}` along with a specific `audience`
parameter, the `verify_aud` setting in `options` takes precedence within the underlying
`pyjwt.decode` call. This disables the audience check, creating a latent security
vulnerability. While no current production code exploits this pattern, it enables a
bypass that was not possible before this change and could be inadvertently used in the
future.
Did we get this right? 👍 / 👎 to inform future reviews.
| encoded_jwt, | ||
| client_secret, | ||
| algorithms=["HS256"], | ||
| options={"require": ["exp"], "verify_exp": True}, |
There was a problem hiding this comment.
Thoughts on making this option a default configuration in decode? The clanker tells me that there is only one place where exp is potentially missing (Jira Server Webhoks). Making this a default config would harden existing uses of decode and make it easier to do the right thing for any future uses of this method.
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
JWT decoding now requires an exp claim by default, so test tokens must include one. Co-Authored-By: Claude <noreply@anthropic.com>
Reject client-secret JWTs that expire more than an hour in the future, and keep each jti in the replay cache until its JWT expires instead of for a fixed 60 seconds. Co-Authored-By: Claude <noreply@anthropic.com>
e4e2d69 to
b8e4004
Compare
…tomically Lower the maximum client-secret JWT lifetime to 5 minutes, and use cache.add so checking and recording a jti is a single atomic operation. Co-Authored-By: Claude <noreply@anthropic.com>
Log a warning when a client-secret JWT is rejected for a missing required claim, an expiration too far in the future, or a reused jti, so we can see how many integrations the new checks affect. Co-Authored-By: Claude <noreply@anthropic.com>
Per VULN-2518 --------- Co-authored-by: Michelle Tran <michelle.tran@sentry.io> Co-authored-by: Claude <noreply@anthropic.com>
Per VULN-2518