fix(oidc): map ErrSubjectInvalid to invalid_grant instead of server_error - #950
fix(oidc): map ErrSubjectInvalid to invalid_grant instead of server_error#950youdie006 wants to merge 1 commit into
Conversation
…rror A JWT-profile (RFC 7523) grant where iss != sub is rejected with ErrSubjectInvalid. DefaultToServerError did not list ErrSubjectInvalid in its invalid_grant case, so it fell through to default and was returned as server_error (HTTP 500) instead of invalid_grant (HTTP 400) - a spurious 500 for a merely-invalid request, logged at error level. Its sibling ErrSubjectMissing was already mapped to invalid_grant; ErrSubjectInvalid being absent was an oversight. The fix only corrects the error code of an already-rejected request and weakens no security check. Closes zitadel#945
![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: 7cc22b6b6a
ℹ️ 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".
| oidcErr = ErrInvalidRequest().WithParent(err).WithDescription("%s", description) | ||
| case errors.Is(err, ErrIssuerInvalid), | ||
| errors.Is(err, ErrSubjectMissing), | ||
| errors.Is(err, ErrSubjectInvalid), |
There was a problem hiding this comment.
When a private_key_jwt used to authenticate an authorization-code or refresh-token request has iss != sub, VerifyJWTAssertion returns this same sentinel and AuthorizePrivateJWTKey propagates it unchanged, so RequestError now reports invalid_grant. RFC 7523 §3.2 requires invalid_client for an invalid JWT used as client authentication; invalid_grant applies to the JWT authorization-grant flow in §3.1. Because this verifier is shared by both modes, translate the error in the mode-specific callers rather than mapping it globally here.
Useful? React with 👍 / 👎.
1 participant
Which Problems Are Solved
iss != subis rejected withErrSubjectInvalid("delegation not allowed, issuer and sub must be identical").DefaultToServerErrordid not listErrSubjectInvalidin itsinvalid_grantcase, so it fell through todefault:and was returned asserver_error(HTTP 500) instead ofinvalid_grant(HTTP 400).server_erroris logged at error level) generates misleading error-level noise for ordinary bad client input.How the Problems Are Solved
errors.Is(err, ErrSubjectInvalid)to theinvalid_grantcase inDefaultToServerError, immediately after the siblingErrSubjectMissingentry.ErrSubjectMissing(the other subject-validation failure) was already mapped toinvalid_grant;ErrSubjectInvalidbeing absent was a clear oversight.invalid_grant. The fix only corrects the HTTP status/error code of an already-rejected request; it weakens no security check -- an assertion withiss != subis still rejected exactly as before.Additional Changes
TestDefaultToServerError_SubjectInvalidassertingErrSubjectInvalidmaps toInvalidGrant, matching the existing test style inpkg/oidc/error_test.go.Additional Context
server_error; after it, it returnsinvalid_grant.go test ./pkg/oidc/...and./pkg/op/...pass,gofmtandgo vetare clean.