Log JWT token validation details#22232
Conversation
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
prashantkumar1982
force-pushed
the
codex/jwt-token-validation-debug-logs
branch
from
5473e05 to
c65876a
Compare
product-security-plaid-production
Bot
requested review from
DylanTinianov and
silaslenihan
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c65876abd4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if refreshErr := v.refreshJWKS(ctx); refreshErr != nil { | ||
| v.lggr.Warnw("JWKS refresh failed", "error", refreshErr, "kid", kid) | ||
| return nil, fmt.Errorf("%w: kid=%s", ErrJWKSKeyNotFound, kid) |
There was a problem hiding this comment.
When refreshJWKS fails (for example, Auth0 JWKS is temporarily unavailable), this branch now returns only ErrJWKSKeyNotFound and no longer logs refreshErr, so token validation failures become indistinguishable from a real missing kid. That regression removes the actionable root cause operators previously had, which directly undermines the new failure diagnostics for production incidents. Consider wrapping or logging refreshErr here so outages surface as JWKS fetch failures rather than generic key-not-found errors.
Useful? React with 👍 / 👎.
prashantkumar1982
force-pushed
the
codex/jwt-token-validation-debug-logs
branch
from
c65876a to
830b777
Compare
timothyF95
left a comment
timothyF95
left a comment
There was a problem hiding this comment.
Approved only to be deployed to STAGING for testing
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 830b777f23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if len(tokenString) <= maxLen { | ||
| return tokenString | ||
| } |
There was a problem hiding this comment.
safeTokenPrefix currently returns the entire token when its length is <= tokenLogPrefixLen, so any short Auth value (for example, a misrouted API key or short bearer token) is written verbatim in tokenValidationError logs. This code path runs for invalid tokens before verification, so with debug logging enabled an attacker/client can cause raw credential leakage to logs; the helper should always redact so complete token material is never emitted.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 830b777f23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "claims": token.Claims, | ||
| "valid": token.Valid, |
There was a problem hiding this comment.
When debug logging is enabled, this path now emits token.Claims verbatim for every validation failure, including tokens that have not passed signature validation. Because req.Auth is client-controlled, an attacker can send oversized or sensitive claim payloads and force large claim blobs into logs, which creates a log-amplification/PII exposure risk that did not exist before this change. Consider redacting to a small allowlist (e.g., iss, aud, kid) or truncating claim values before logging.
Useful? React with 👍 / 👎.
|
2 participants
Summary