fix: return RFC 6749 errors from the /oauth/token refresh grant

[nishant-iyengar]

The problem

POST /oauth/token returns two different error shapes, depending on the grant type:

// grant_type=authorization_code — HTTP 400
{ "error": "invalid_grant", "error_description": "Invalid authorization code" }

// grant_type=refresh_token — HTTP 400
{ "code": 400, "error_code": "refresh_token_not_found", "msg": "Invalid Refresh Token: Refresh Token Not Found" }

The second one has no error field, so an OAuth client cannot read it. RFC 6749 §5.2 requires that field.

The client therefore never learns that the grant is dead. It does not ask the user to re-authorize. It just presents the same dead refresh token again, forever.

Evidence

We found this through an FDX / Plaid Core Exchange integration. Plaid's error contract is RFC 6749 §5.2, so it cannot classify the response. It reports "an unexpected error occurred" and leaves the connection in place.

Seven days of /oauth/token on one project:

	count
400 refresh_token_not_found	173
400 session_expired	11
200	38
re-authorization prompts raised by the client	0

Eleven failures in a row from one client IP, all refresh_token_not_found:

Jul 24 03:03:01   —
Jul 24 09:03:08   +6h 00m 07s
Jul 24 15:03:11   +6h 00m 03s
Jul 24 21:03:14   +6h 00m 03s
Jul 25 03:03:23   +6h 00m 09s
Jul 25 09:03:27   +6h 00m 04s
Jul 25 15:03:36   +6h 00m 09s
Jul 25 21:03:46   +6h 00m 10s
Jul 26 03:03:49   +6h 00m 03s
Jul 26 09:04:19   +6h 00m 30s
Jul 26 15:04:27   +6h 00m 08s

A fixed six-hour interval, no backoff, and no point where the client gives up. That is what a client does when it never got an error it could understand. A refresh token going dead is normal. Retrying it every six hours forever is not.

Cause

handleAuthorizationCodeGrant gets this right because it builds each of its own errors with NewOAuthError(...), where the correct code is obvious as you write the line.

handleRefreshTokenGrant has no errors of its own. They all come from tokens.Service.RefreshTokenGrant, which builds grant failures with NewBadRequestError(...) — an HTTPError — and the handler returns it as is:

tokenResponse, err := tokenService.RefreshTokenGrant(...)
if err != nil {
    return err   // HTTPError goes straight to the client
}

That service is also used by /auth/v1/token, whose clients read error_code — the reason for the // do not rename the JSON tags! comments in apierrors. So the service cannot return OAuthError instead. The conversion has to happen in the OAuth handler, and that is what was missing.

This is not a regression. It has been there since the endpoint was added. #2135 moved the refresh logic into a shared package and kept the error shape its only caller expected at the time. #2159 then added both grants at once, and the refresh grant inherited an error shape meant for a different endpoint.

#2339 is the same seam failing a different way.

The fix

Two lines at the call sites, plus a small converter:

token service error_code	→ RFC 6749 error
refresh_token_not_found, refresh_token_already_used, session_not_found, session_expired, user_banned	invalid_grant
validation_failed	invalid_request
anything else	invalid_request
any status other than 400	left alone

Two rules decide that table.

invalid_grant is only sent for codes we have listed. It tells the client the grant is dead, which sends a real user back through a consent screen, so we only send it when we are sure. Every other error, including any error_code we have never seen, becomes invalid_request. The client reports that and leaves the grant alone. Guessing invalid_grant for an unknown error would break working connections over something we do not understand.

Anything that is not a 400 is left alone. Each code in the spec says something permanent about the request. A 409 (two refreshes at once), a 429, or a 5xx is temporary, and turning one into a spec code would make a short outage look like a dead grant to every client refreshing at that moment. It also matters in practice: HandleResponseError always sends *OAuthError as a 400, so converting a 503 would change its status too.

Errors that are already in the right shape are passed through. This endpoint does return correct bodies on some paths, and rewriting everything would break those.

refresh_token_already_used arrives wrapped in storage.CommitWithError, because that transaction still has to commit. The wrapper has Cause() but not Unwrap(), so a normal type check misses it. The converter unwraps it the same way HandleResponseError does.

The same call is added to the HTTPError branch of handleAuthorizationCodeGrant. That handler builds its own errors correctly, but the one error it gets back from the token service — from the IssueRefreshToken transaction — was also being returned as is.

Tests

internal/api/oauthserver/errors_test.go, one table-driven test, no database needed. The rows that matter are the ones that stop this getting worse:

• an unknown error_code becomes invalid_request, never invalid_grant
• a 409 and a 500 come back untouched, with their status intact
• refresh_token_already_used is converted correctly through its CommitWithError wrapper
• the description comes from Message, not Error(), so an internal note like "Possible abuse attempt: <token id>" never reaches the client

Compatibility

• /auth/v1/token is untouched. No error_code that a supabase-js client reads has changed.
• On /oauth/token, refresh_token error bodies change shape. That is the fix. They now match what the authorization_code grant on the same endpoint already returned.
• No status codes change.

[varun-lemma]

LGTM!

[xlgmokha]

LGTM. However, I would like to ensure that we reproduce the original defect in an integration test from the API entrypoint. I can see that we added unit tests for the new code but it's not clear to me if we ensure it from the API level. Can you confirm?

[xlgmokha]

question: is there a test covering this to ensure the error key is in the response body?

[cstockton]

I think it's important we follow the spec and appreciate you raising this.

Right now this PR is doing two separate things:

1. Adding logic to change the errors based on some conditional logic
2. Introducing new constant names to replace the string literals
3. Introducing error codes outside of apierrors package

For 1. I don't think this is the right approach, it's continuing some patterns I think lead to this kind of problem to begin with - I've been slowly working on removing concrete type switches as I can.

For 2. it leaves string literals for NewOAuthError in other places such as the web3 package. I'm not against declaring constants for OAuth error codes but they should have public names matching conventions in other packages. If we did do this it should likely land where OAuthError is declared and other error codes exist, apierrors.

If you would like to break this up into two separate PRS, targeting 1 or 2/3 separately OR focusing this PR on either of those that would be fine by me. Otherwise an auth team member can pick this up.

[TobCar]

@cstockton I opened up a PR for the error code improvements: #2676

I suggest Nishant's PR can get rebased on top of mine after it merges.