Skip to content

fix(auth): return 4xx for bad OIDC authorization codes#1790

Merged
rohilsurana merged 4 commits into
mainfrom
fix/auth-callback-invalid-grant-4xx
Jul 22, 2026
Merged

fix(auth): return 4xx for bad OIDC authorization codes#1790
rohilsurana merged 4 commits into
mainfrom
fix/auth-callback-invalid-grant-4xx

Conversation

@whoAbhishekSah

@whoAbhishekSah whoAbhishekSah commented Jul 22, 2026

Copy link
Copy Markdown
Member

Problem

AuthCallback returns a 500 (internal) whenever the OIDC token exchange fails with an oauth2 error like invalid_grant:

"code":"internal","error":"internal: AuthCallback: ... oauth2: \"invalid_grant\" \"Bad Request\"","method":"/raystack.frontier.v1beta1.FrontierService/AuthCallback"

invalid_grant means the authorization code was bad. That is a client-side problem, not a server fault. It happens during normal operation:

  • the code was already used (page refresh, double submit, retry — codes are single-use)
  • the code expired (user sat on the consent screen too long)
  • redirect URI mismatch
  • a bot, crawler, or browser prefetch opened the callback link first, so the code was already used by the time the real request arrived

🤖 Generated with Claude Code

AuthCallback returned a 500 (internal) whenever the OIDC token
exchange failed with an oauth2 error like invalid_grant. That error
means the authorization code was bad (expired, already used, or the
redirect did not match) which is a client problem, not a server fault.
It fired a critical alert for normal background noise such as retries,
double submits, and link prefetching.

Detect the oauth2.RetrieveError from the token exchange and wrap it in
a new ErrOIDCTokenExchange sentinel. The handler maps that sentinel to
CodeInvalidArgument so the caller gets a 4xx instead of a 500.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented Jul 22, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
frontier Ready Ready Preview, Comment Jul 22, 2026 7:58am

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@whoAbhishekSah, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 31 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: aaed823f-19be-465d-be00-1cfba65da9ef

📥 Commits

Reviewing files that changed from the base of the PR and between f68ae97 and 5a3f2c0.

📒 Files selected for processing (2)
  • core/authenticate/service.go
  • internal/api/v1beta1connect/authenticate.go
📝 Walkthrough

Walkthrough

OIDC token retrieval errors are classified using OAuth2 invalid-grant detection, wrapped with a public sentinel, tested, and returned by AuthCallback as invalid-argument responses.

Changes

OIDC error handling

Layer / File(s) Summary
Classify OIDC token exchange failures
core/authenticate/service.go, core/authenticate/oidc_grant_test.go
Detects invalid-grant OAuth2 retrieval errors, wraps them with ErrOIDCTokenExchange, preserves other token errors, and tests wrapped and non-matching cases.
Map exchange errors at the API boundary
internal/api/v1beta1connect/authenticate.go
Logs the classified error and returns connect.CodeInvalidArgument from AuthCallback.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: rohilsurana

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/api/v1beta1connect/authenticate.go (1)

110-114: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Return a stable client-facing error here. connect.NewError sends err.Error() to clients, and FinishFlow wraps ErrOIDCTokenExchange with the raw oauth2.RetrieveError. Log the original error, but return authenticate.ErrOIDCTokenExchange (or another fixed public error) instead.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 79e07395-0971-4c8e-8e75-cd8c7597dec2

📥 Commits

Reviewing files that changed from the base of the PR and between 0728b99 and 11adde0.

📒 Files selected for processing (2)
  • core/authenticate/service.go
  • internal/api/v1beta1connect/authenticate.go

Comment thread core/authenticate/service.go Outdated
@coveralls

coveralls commented Jul 22, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 29902145949

Coverage decreased (-0.004%) to 46.22%

Details

  • Coverage decreased (-0.004%) from the base build.
  • Patch coverage: 12 uncovered changes across 2 files (6 of 18 lines covered, 33.33%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
core/authenticate/service.go 14 6 42.86%
internal/api/v1beta1connect/authenticate.go 4 0 0.0%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 38479
Covered Lines: 17785
Line Coverage: 46.22%
Coverage Strength: 13.36 hits per line

💛 - Coveralls

oauth2 returns a RetrieveError for any non-2xx token endpoint response,
so the earlier check also turned provider 429 and 5xx failures into a
4xx. That would hide a real IdP outage and stop it from paging.

Narrow the check to RFC 6749 "invalid_grant" via a small isInvalidGrantErr
helper, and add unit tests covering invalid_grant, 429, 5xx, misconfig,
and non-oauth2 errors.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A malformed (non-base64) state made ExtractFlowFromOIDCState fail, which
FinishFlow turned into ErrUnsupportedMethod and the handler mapped to a
500. An empty or invalid state already returned a 400, so the same class
of bad client input gave two different codes.

Map ErrUnsupportedMethod to CodeInvalidArgument in AuthCallback so a bad
state is a 4xx, consistent with the empty-state case.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@whoAbhishekSah

whoAbhishekSah commented Jul 22, 2026

Copy link
Copy Markdown
Member Author

Live test results

Tested against a local build of this branch (commit 5a3f2c04) with a real Google OIDC provider. OIDC flows were started via Authenticate, then AuthCallback was called with crafted state/code values.

# Scenario Response HTTP
1 OIDC bad / expired / reused code (Google returns invalid_grant) invalid_argumentfailed to exchange oidc authorization code: oauth2: "invalid_grant" 400
2 OIDC missing code invalid_argumentOIDC code is missing 400
3 OIDC empty state invalid_argumentinvalid auth state 400
4 OIDC malformed (non-base64) state invalid_argumentunsupported authentication method 400
5 OIDC valid base64 state, non-UUID flow id invalid_argumentunsupported authentication method 400
6 mailotp wrong OTP invalid_argumentinvalid mail otp 400
7 mailotp valid OTP (happy path) session cookie issued 200

Before this PR, cases 1 and 4 returned 500 internal.

  • Case 1 covers the token-exchange fix (11adde0b), narrowed to invalid_grant only (f68ae970).
  • Cases 4 and 5 cover the malformed-state fix (5a3f2c04).
  • Cases 2, 3, 6, 7 are regression checks — unchanged.

Not exercised live

A token-endpoint error that is a RetrieveError but not invalid_grant (provider 429/5xx, invalid_client, invalid_request) should fall through to 500. Real Google returns invalid_grant for any bad code, so this branch was checked by reading isInvalidGrantErr.

@rohilsurana
rohilsurana merged commit 2a92639 into main Jul 22, 2026
8 checks passed
@rohilsurana
rohilsurana deleted the fix/auth-callback-invalid-grant-4xx branch July 22, 2026 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants