CE-1315 Map failed OAuth2 token exchange to a grpc status in uhttp - #1106
CE-1315 Map failed OAuth2 token exchange to a grpc status in uhttp#1106luisina-santos wants to merge 9 commits into
Conversation
A rejected client-credentials token exchange surfaces as *oauth2.RetrieveError wrapping the token endpoint's real HTTP response, but wrapTransientNetworkError had no case for it, so it fell through unclassified and callers (including exit.LogExit) saw codes.Unknown instead of Unauthenticated/PermissionDenied. This affects any connector using clientcredentials.Config-based auth via BaseHttpClient, whether hand-rolled or through OAuth2ClientCredentials. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
General PR Review: Map failed OAuth2 token exchange to a grpc status in uhttpBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commit adds an 8-line block to Risk triage (per Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
Mapping GrpcCodeFromHTTPStatus(Response.StatusCode) alone missed two real shapes: some token endpoints report the error param on a 2xx response, and RFC 6749 5.2 makes 400 (not 401) the default status for invalid_client/invalid_grant, which the status-only mapping turned into InvalidArgument instead of Unauthenticated. Consult RetrieveError.ErrorCode first and fall back to the HTTP status only when the server didn't send a recognized error code. Also make the new test cases wrap *oauth2.RetrieveError in *url.Error, the shape http.Client.Do actually produces, so the errors.As unwrap this fix depends on is exercised against its real production shape. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
All three suggestions from the review addressed in b876f7e:
|
| return codes.Unauthenticated, true | ||
| case "access_denied": | ||
| return codes.PermissionDenied, true | ||
| case "invalid_grant", "invalid_scope", "invalid_request", "unsupported_grant_type", "unsupported_response_type": |
There was a problem hiding this comment.
🟡 Suggestion: invalid_grant → InvalidArgument misclassifies the credential-failure case for grants other than client_credentials. Per RFC 6749 §5.2, invalid_grant covers an expired/revoked refresh token and a wrong username/password under the password grant — for hand-rolled oauth2.Config connectors that is exactly "the saved credential is dead, reauthorize." InvalidArgument is not in IsSyncPreservable's allow-list (pkg/sync/syncer.go:109-119), so the artifact is discarded and exit.LogExit reports code 3 instead of 16, which is the misreporting this PR exists to fix. Consider Unauthenticated for invalid_grant, leaving invalid_request/invalid_scope/unsupported_* as InvalidArgument.
There was a problem hiding this comment.
Fixed in b2c238d: invalid_grant now maps to Unauthenticated alongside invalid_client. Verified against the RFC text directly (not just this review's paraphrase) — §5.2 does name "resource owner credentials" as one of the things invalid_grant covers, which is a genuine credential failure for the password grant, not a malformed-request case. Added a test case asserting this.
| if retrieveErr.Response != nil { | ||
| return WrapErrors(GrpcCodeFromHTTPStatus(retrieveErr.Response.StatusCode), oauthTokenErrorMessage(retrieveErr), err) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: this fallback also flips retry behavior for transient token-endpoint failures, which the PR description doesn't mention. A 429/502/503 from the token endpoint previously fell through unclassified (codes.Unknown), and retry.Retryer.ShouldWaitAndRetry (pkg/retry/retry.go:61) only retries Unavailable/DeadlineExceeded — so it failed fast. Now GrpcCodeFromHTTPStatus returns Unavailable, and the syncer's retryer is configured with MaxAttempts: 0 (pkg/sync/parallel_syncer.go:151), so a persistently 5xx-ing token endpoint retries until the run-duration limit instead of terminating. That's arguably the right behavior (it matches a 503 on a normal API call), but it's a default-behavior change worth stating in the PR body and covering in the table — no case in errors_test.go exercises the 429/5xx or unrecognized-error-param rows, which is precisely where the classification flips.
There was a problem hiding this comment.
Confirmed both claims against the actual code before acting: retry.Retryer.ShouldWaitAndRetry (retry.go:61) only retries Unavailable/DeadlineExceeded, and the syncer's retryer is built with MaxAttempts: 0 (parallel_syncer.go:151), which the RetryConfig doc confirms means unlimited. Added a 429 test case documenting the new Unavailable/retryable classification, plus a case for an unrecognized error param falling back to the HTTP status, and added a "Downstream behavior changes" section to the PR description (in b2c238d and the description edit) covering both this and the IsSyncPreservable side effect.
| // the spec defines, signaling the caller to fall back to the HTTP status. | ||
| func oauthTokenErrorCode(errCode string) (code codes.Code, ok bool) { | ||
| switch errCode { | ||
| case "invalid_client", "unauthorized_client": |
There was a problem hiding this comment.
🟡 Suggestion: unauthorized_client reads more like PermissionDenied than Unauthenticated. RFC 6749 §5.2 defines it as "the authenticated client is not authorized to use this authorization grant type" — the credential was accepted, the grant type wasn't. Reporting Unauthenticated points an operator at rotating the client secret, which won't fix a grant-type/config mismatch. Both codes are preservable in IsSyncPreservable, so the only observable difference is the exit code and message, but the message is the point of this change.
There was a problem hiding this comment.
Fixed in b2c238d: unauthorized_client now maps to PermissionDenied. Confirmed against RFC 6749 §5.2 directly — "the authenticated client is not authorized to use this authorization grant type" does describe an accepted identity denied a specific grant type, so PermissionDenied is the right fit over Unauthenticated. Added a test case asserting this.
Every branch returns explicit values, so the names were unused by the function body — nonamedreturns flags them regardless. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
invalid_grant explicitly covers "resource owner credentials" (a wrong username/password under the password grant), not just an authorization code or refresh token — a genuine credential failure, so it belongs with invalid_client's Unauthenticated rather than InvalidArgument. unauthorized_client per RFC 6749 5.2 is "the authenticated client is not authorized to use this authorization grant type" — the identity was accepted, so PermissionDenied fits better than Unauthenticated. Also add coverage for the two classification-flip cases raised in review: a 429/5xx token-endpoint failure now falls back to codes.Unavailable (and becomes retryable, documented in the PR body), and an unrecognized error param falls back to the HTTP status while keeping the error param in the message. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| // - invalid_client and unauthorized_client both name the client's | ||
| // credential/identity as the problem — RFC 6749 defines invalid_client as | ||
| // "Client authentication failed", and unauthorized_client as "The | ||
| // authenticated client is not authorized to use this authorization grant | ||
| // type" (authenticated, but not entitled) — PermissionDenied, not | ||
| // Unauthenticated, since re-presenting a different secret won't fix a | ||
| // grant-type mismatch. | ||
| // - invalid_grant explicitly covers "resource owner credentials" (a wrong | ||
| // username/password under the password grant) alongside an expired or | ||
| // revoked authorization code/refresh token — a genuine credential | ||
| // failure, so it maps with invalid_client rather than the InvalidArgument | ||
| // bucket. | ||
| // - access_denied, invalid_scope, invalid_request, unsupported_grant_type, | ||
| // and unsupported_response_type describe a malformed or disallowed | ||
| // request rather than a rejected identity. |
There was a problem hiding this comment.
🟡 Suggestion: This doc comment no longer matches the code after b2c238d. Bullet 1 groups invalid_client with unauthorized_client and concludes "PermissionDenied, not Unauthenticated", but the switch maps invalid_client → Unauthenticated. Bullet 3 lists access_denied among the "malformed or disallowed request" codes, yet it maps to PermissionDenied. Regroup the bullets by the code actually returned (invalid_client/invalid_grant → Unauthenticated; unauthorized_client/access_denied → PermissionDenied; the rest → InvalidArgument) so the rationale doesn't argue against the implementation. (high confidence)
There was a problem hiding this comment.
Resolved as a side effect of the comment-trim commit (536e0be) — the whole bulleted doc block this pointed at is gone; oauthTokenErrorCode now has a one-line comment and the mapping table lives in the PR description instead.
| if code, ok := oauthTokenErrorCode(retrieveErr.ErrorCode); ok { | ||
| return WrapErrors(code, oauthTokenErrorMessage(retrieveErr), err) | ||
| } | ||
| if retrieveErr.Response != nil { | ||
| return WrapErrors(GrpcCodeFromHTTPStatus(retrieveErr.Response.StatusCode), oauthTokenErrorMessage(retrieveErr), err) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Error-param-first is unconditional, so a transient token-endpoint status that also carries a recognized RFC error param loses retryability. E.g. HTTP 429 or 503 with {"error":"invalid_request"} (some providers report throttling this way) now returns InvalidArgument, which retry.Retryer.ShouldWaitAndRetry will not retry — while the same 429 with no error param maps to Unavailable and is retried, as it would be on a normal API call. Consider checking the status first when Response.StatusCode is 429 or ≥500 and only consulting the error param otherwise. The test table has no cell where a recognized error param coexists with a transient status; adding that permutation would pin whichever behavior you intend. (medium confidence)
There was a problem hiding this comment.
Fixed in ee3cec0: a transient status (429 or any 5xx) is now checked first, before the error param, via a new isTransientHTTPStatus helper mirroring GrpcCodeFromHTTPStatus's own Unavailable set. Added a test case for exactly the permutation you named (429 + invalid_client) plus a retry-liveness test asserting against a real retry.Retryer (rate-limited retries, rejected credentials doesn't).
Also fixes a stale doc comment left over from the previous commit that still described unauthorized_client as mapping the same as invalid_client. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| if code, ok := oauthTokenErrorCode(retrieveErr.ErrorCode); ok { | ||
| return WrapErrors(code, oauthTokenErrorMessage(retrieveErr), err) | ||
| } | ||
| if retrieveErr.Response != nil { |
There was a problem hiding this comment.
🟡 Suggestion: this nil-guard implies Response can be nil, but when it is nil and ErrorCode is empty/unrecognized the error falls through into the chain below, where isHTTP2ClientConnectionLost calls err.Error() — and RetrieveError.Error() dereferences r.Response.Status on exactly that branch (vendor/golang.org/x/oauth2/token.go:213), so it would panic. Consider making the errors.As branch total: always return a wrapped error, which would also make oauthTokenErrorMessage's default arm reachable (today it is dead, since the function is only called when ErrorCode is non-empty or Response is non-nil).
Medium confidence — the vendored oauth2 always populates Response, so this is a defensive-consistency point rather than a live crash.
There was a problem hiding this comment.
Confirmed the mechanics against the vendored source before acting: doTokenRoundTrip always sets Response from the real HTTP response, so this isn't reachable today — but agreed it's worth closing since RetrieveError.Response is a plain nullable field with no such guarantee in its public contract. Fixed in ee3cec0: the errors.As branch is now total, so it always returns before reaching isHTTP2ClientConnectionLost's err.Error() call. Added a case with ErrorCode empty and Response nil asserting it maps cleanly (codes.Unknown) instead of falling through.
| return WrapErrors(code, oauthTokenErrorMessage(retrieveErr), err) | ||
| } | ||
| if retrieveErr.Response != nil { | ||
| return WrapErrors(GrpcCodeFromHTTPStatus(retrieveErr.Response.StatusCode), oauthTokenErrorMessage(retrieveErr), err) |
There was a problem hiding this comment.
🟡 Suggestion: this fallback flips token-endpoint 429/5xx from Unknown (fail fast) to Unavailable (retryable), and the syncer's retryer runs with MaxAttempts: 0, so a persistently failing token endpoint now backs off until the run-duration limit instead of exiting. The new 429 Too Many Requests table case pins the code but not the retry consequence; TestWrapTransientNetworkError_NXDOMAINIsTerminal (pkg/uhttp/errors_test.go:291) is the in-repo precedent for pinning that via Retryer.ShouldWaitAndRetry, and this new retryable class deserves the same guard.
This is the follow-on half of a previously raised point — the PR body now documents the flip, so only the liveness assertion is missing.
There was a problem hiding this comment.
Added in ee3cec0: TestWrapTransientNetworkError_OAuthTokenEndpointTransientIsRetried, mirroring TestWrapTransientNetworkError_NXDOMAINIsTerminal exactly as suggested — asserts against a real retry.Retryer that a rate-limited token endpoint is retried and rejected credentials are not.
RetrieveError branch total A 429/5xx token-endpoint response with a recognized RFC 6749 error param (some providers report throttling as invalid_request) was mapping to InvalidArgument and losing retry eligibility. Check the transient status first, regardless of the error param. Also make the errors.As branch total: previously an unmatched error code with a nil Response fell through to the rest of the function, where isHTTP2ClientConnectionLost calls err.Error(), and RetrieveError.Error() dereferences Response.Status unconditionally when ErrorCode is empty -- a nil Response there would panic. The vendored x/oauth2 always populates Response today, so this wasn't a live crash, but nothing in the type's contract guarantees that. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| // codes.Unavailable, so a transient token-endpoint failure stays retryable | ||
| // regardless of what error param the body also carries. | ||
| func isTransientHTTPStatus(statusCode int) bool { | ||
| return statusCode == http.StatusTooManyRequests || statusCode >= 500 |
There was a problem hiding this comment.
🟡 Suggestion: this doesn't quite mirror the Unavailable set it claims to. GrpcCodeFromHTTPStatus maps 501 to codes.Unimplemented (explicit case, before the 5xx range) and any >= 600 to codes.Unknown — neither is retryable, and neither is preservable under IsSyncPreservable. So a 501 token response carrying invalid_client now short-circuits the error-param branch and lands on Unimplemented instead of Unauthenticated, discarding the sync artifact. Gating on the mapped code makes the mirror true by construction:
| return statusCode == http.StatusTooManyRequests || statusCode >= 500 | |
| func isTransientHTTPStatus(statusCode int) bool { | |
| return GrpcCodeFromHTTPStatus(statusCode) == codes.Unavailable | |
| } |
There was a problem hiding this comment.
Fixed in 85a3c28 exactly as suggested — isTransientHTTPStatus now checks GrpcCodeFromHTTPStatus(statusCode) == codes.Unavailable directly instead of approximating it. Added a test case pinning 501 as non-transient (Unimplemented, not Unavailable).
| var retrieveErr *oauth2.RetrieveError | ||
| if errors.As(err, &retrieveErr) { | ||
| if retrieveErr.Response != nil && isTransientHTTPStatus(retrieveErr.Response.StatusCode) { | ||
| return WrapErrors(GrpcCodeFromHTTPStatus(retrieveErr.Response.StatusCode), retrieveErr.Response.Status, err) |
There was a problem hiding this comment.
🟡 Suggestion: the transient branch uses the bare Response.Status as the message, so an error_description the endpoint sent alongside a 429/5xx is dropped — and that field usually carries the most actionable detail (e.g. "rate limit exceeded, retry after 3600 seconds"). Only the code selection needs the status to win here; the message could still be oauthTokenErrorMessage(retrieveErr), which falls back to Response.Status anyway when no error param is present. (Low confidence — deliberate per the PR description and pinned by a test, so this is a judgment call, not a defect.)
There was a problem hiding this comment.
Applied in 85a3c28, with a small adjustment: keeping the status text rather than switching fully to oauthTokenErrorMessage, since that prefers ErrorCode first and would reintroduce the original problem (a 429 with an error param would show just "invalid_request", hiding why it's Unavailable). New transientOAuthTokenMessage leads with Response.Status and appends ErrorDescription when present, so both the classification reason and the actionable detail survive. Added a test case with both fields set.
… msg isTransientHTTPStatus approximated GrpcCodeFromHTTPStatus's Unavailable set with a numeric range, which wrongly included 501 (explicitly mapped to Unimplemented, not Unavailable). Consult the mapping directly so the two can't drift. The transient-status message also dropped ErrorDescription, which usually carries the most actionable detail (e.g. a rate-limit retry hint). Keep the status text (it explains the Unavailable classification) but append the description when the server sent one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| // isTransientHTTPStatus reports whether GrpcCodeFromHTTPStatus maps | ||
| // statusCode to codes.Unavailable, so a transient token-endpoint failure | ||
| // stays retryable regardless of what error param the body also carries. | ||
| func isTransientHTTPStatus(statusCode int) bool { | ||
| return GrpcCodeFromHTTPStatus(statusCode) == codes.Unavailable | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the transient set is now exactly codes.Unavailable, which excludes 408 — GrpcCodeFromHTTPStatus(408) returns DeadlineExceeded. That reintroduces the asymmetry this branch exists to prevent: a 408 token response without an error param falls through to the status and gets DeadlineExceeded (retried by retry.Retryer.ShouldWaitAndRetry, preservable), but the same 408 with a recognized param like invalid_request short-circuits to InvalidArgument — neither retried nor preservable. Consider matching the retryer's full retryable set instead: code := GrpcCodeFromHTTPStatus(statusCode); return code == codes.Unavailable || code == codes.DeadlineExceeded, with a 408 + invalid_request table case. (Medium confidence; narrow — 408 from a token endpoint carrying an RFC 6749 param is rare.)
There was a problem hiding this comment.
Fixed in d7ff8bc — isTransientHTTPStatus now also treats codes.DeadlineExceeded as transient (408 included), not just Unavailable, matching retry.Retryer's actual retryable set. Added a test case for 408 + a recognized error param.
| var retrieveErr *oauth2.RetrieveError | ||
| if errors.As(err, &retrieveErr) { | ||
| if retrieveErr.Response != nil && isTransientHTTPStatus(retrieveErr.Response.StatusCode) { | ||
| return WrapErrors(GrpcCodeFromHTTPStatus(retrieveErr.Response.StatusCode), transientOAuthTokenMessage(retrieveErr), err) |
There was a problem hiding this comment.
🟡 Suggestion: this is the one path in uhttp that produces a 429 Unavailable without attaching rate-limit details. wrapper.go:592 uses WrapErrorsWithRateLimitInfo so that retry.Retryer.ShouldWaitAndRetry can read the *v2.RateLimitDescription detail (and mark the sleep as a rate-limit wait rather than a plain retry, pkg/retry/retry.go:88-133). retrieveErr.Response carries the real headers, so the token endpoint's Retry-After/X-RateLimit-* is available here but discarded. Attaching the detail while keeping this message would make token-endpoint throttling account the same way as an API-call 429. (Medium confidence; impact is bounded since the retryer caps waits at MaxDelay.)
There was a problem hiding this comment.
Applied in d7ff8bc — new wrapTransientOAuthTokenError extracts rate-limit data from retrieveErr.Response's real headers via ratelimit.ExtractRateLimitData and attaches it as a status detail, mirroring WrapErrorsWithRateLimitInfo. Added TestWrapTransientNetworkError_OAuthTransientAttachesRateLimitDetails asserting a Retry-After header produces a *v2.RateLimitDescription detail.
| code := codes.Unknown | ||
| if retrieveErr.Response != nil { | ||
| code = GrpcCodeFromHTTPStatus(retrieveErr.Response.StatusCode) | ||
| } | ||
| return WrapErrors(code, oauthTokenErrorMessage(retrieveErr), err) |
There was a problem hiding this comment.
🟡 Suggestion: the nil-Response defense is narrower than the PR describes. Making the branch total stops this function from calling err.Error(), but WrapErrors joins the original err into the result, so the returned error still embeds the *oauth2.RetrieveError — and RetrieveError.Error() unconditionally dereferences r.Response.Status when ErrorCode is empty (vendor/golang.org/x/oauth2/token.go:212). Any caller that formats the returned error (zap.Error, %v, exit.LogExit) panics in exactly the nil-Response/no-error-param case the new test pins. If the goal is real safety rather than deferral, don't join the raw retrieveErr when Response == nil — wrap a sanitized error instead. (Low confidence on reachability: the vendored x/oauth2 always populates Response, so this only matters for the hypothetical the branch was written to defend against.)
There was a problem hiding this comment.
Investigated this properly rather than taking the reachability caveat at face value, and the premise doesn't hold: I empirically confirmed the panic never propagates through errors.Join.Error() either. url.Error.Error() invokes RetrieveError.Error() via its own internal fmt.Sprintf("%s %q: %s", ..., e.Err) call, and Go's fmt package recovers a panicking Error()/String() method during its own formatting, substituting "%!s(PANIC=...)" rather than propagating — verified directly with a probe test (panic() traced to token.go:212 when RetrieveError.Error() is called bare, but calling .Error() on the errors.Join result that embeds it, exactly what exit.LogExit does, produces a clean string with the PANIC=... text embedded, no crash). Reverted the "don't join err" change in d7ff8bc, since it wasn't preventing anything and was discarding the errors.Is/As chain to err for no benefit. Kept a dedicated TestWrapTransientNetworkError_OAuthNilResponseDoesNotPanicOnFormat test pinning the no-panic behavior directly, and documented the mechanism in the PR description's Nil-Response section rather than relying on either of our reasoning.
correct an overstated panic claim isTransientHTTPStatus only checked for Unavailable, but 408 maps to DeadlineExceeded, which retry.Retryer also treats as retryable -- the same asymmetry the transient-status-first ordering exists to prevent. Now checks both codes. The transient branch was building its own status manually, discarding any rate-limit detail retrieveErr.Response's real headers carry. wrapTransientOAuthTokenError now extracts and attaches it the same way WrapErrorsWithRateLimitInfo does for normal API responses, so retry.Retryer can compute rate-limit-aware backoff for a throttled token endpoint too. Reverted the "don't join err" change from the previous commit: verified (not guessed) that it wasn't preventing a real panic. RetrieveError's panic on a nil Response only ever occurs while it's being formatted by a %s/%v verb -- inside url.Error.Error()'s own fmt.Sprintf call -- and fmt recovers panics from Stringer/error methods it formats internally, substituting "%!s(PANIC=...)" instead of propagating. Confirmed this holds through errors.Join too. Restored the simpler WrapErrors call and the errors.Is/As chain to err that not joining it had discarded. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| rateLimited := wrapTransientNetworkError(wrapAsTokenRequestError(&oauth2.RetrieveError{ | ||
| Response: &http.Response{StatusCode: http.StatusTooManyRequests, Status: "429 Too Many Requests"}, | ||
| })) | ||
| require.Equal(t, codes.Unavailable, status.Code(rateLimited)) | ||
| require.True(t, newRetryer().ShouldWaitAndRetry(t.Context(), rateLimited), | ||
| "a rate-limited token endpoint must still be retried") | ||
|
|
||
| rejected := wrapTransientNetworkError(wrapAsTokenRequestError(&oauth2.RetrieveError{ | ||
| ErrorCode: "invalid_client", | ||
| Response: &http.Response{StatusCode: http.StatusUnauthorized, Status: "401 Unauthorized"}, | ||
| })) | ||
| require.Equal(t, codes.Unauthenticated, status.Code(rejected)) | ||
| require.False(t, newRetryer().ShouldWaitAndRetry(t.Context(), rejected), | ||
| "rejected credentials must not be retried") |
There was a problem hiding this comment.
🟡 Suggestion: this retry-liveness test covers the Unavailable arm (429) and the terminal arm (401), but not the DeadlineExceeded arm this commit just added to isTransientHTTPStatus. The 408 case is only asserted at the grpc-code level in the table test, so the actual invariant the new code depends on — that retry.Retryer.ShouldWaitAndRetry retries DeadlineExceeded too — isn't pinned here. A third block asserting newRetryer().ShouldWaitAndRetry(...) is true for a 408 RetrieveError would close that gap. (Medium confidence, non-blocking.)
There was a problem hiding this comment.
Fixed in 75094c6 — added a third block asserting ShouldWaitAndRetry is true for a 408 RetrieveError, closing the DeadlineExceeded gap.
TestWrapTransientNetworkError_OAuthTokenEndpointTransientIsRetried covered Unavailable (429) and terminal (401) but not the DeadlineExceeded arm isTransientHTTPStatus gained. Assert retry.Retryer.ShouldWaitAndRetry against a 408 too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
luisina-santos
changed the title
1 participant
Summary
*oauth2.RetrieveErrorwrapping the token endpoint's real HTTP response, butwrapTransientNetworkErrorhad no case for it — the error fell through unclassified, so callers (includingexit.LogExit) sawcodes.Unknowninstead ofUnauthenticated/PermissionDenied.wrapTransientNetworkError, checked in this order onceerrors.Asmatches a*oauth2.RetrieveError:GrpcCodeFromHTTPStatus(Response.StatusCode)isUnavailableorDeadlineExceeded(the two codesretry.Retryer.ShouldWaitAndRetrytreats as retryable), checked by consulting the mapping directly rather than approximating it with a numeric range (501 isUnimplemented, not part of this set, even though it's ≥ 500). The message keepsResponse.Status(it explains the classification) and appendsErrorDescriptionwhen the server sent one, so an actionable detail like a rate-limit retry hint isn't dropped. Rate-limit detail from the response headers (Retry-After, etc.) is attached to the status the same wayWrapErrorsWithRateLimitInfodoes for a normal API response, so the retryer can compute rate-limit-aware backoff for a throttled token endpoint too.errorparameter is consulted:invalid_client/invalid_grant→Unauthenticated(both name a rejected credential — the latter explicitly covers "resource owner credentials" under the password grant, not just an authorization code/refresh token);unauthorized_client/access_denied→PermissionDenied(unauthorized_clientper RFC 6749 is "the authenticated client is not authorized to use this authorization grant type" — the identity was accepted, so this isn't an auth failure);invalid_scope/invalid_request/unsupported_grant_type/unsupported_response_type→InvalidArgument(malformed/disallowed request, not a rejected identity).GrpcCodeFromHTTPStatus(Response.StatusCode), orcodes.Unknownif there's noResponseat all.The branch is total once
errors.Asmatches — it always returns a wrapped error, so a*oauth2.RetrieveErroris never run through the network-error checks below it (see "Nil-Response" below for why this is a code-quality choice, not a panic-safety one).clientcredentials.Config-based auth throughBaseHttpClient, whether hand-rolled (asbaton-kyribadoes) or via this package's ownOAuth2ClientCredentialshelper — verified both produce the same unmappedcodes.Unknowntoday.CE-1315
Why the error param takes priority over the HTTP status (for non-transient statuses)
Two real-world shapes break a status-code-only mapping:
errorparam on an HTTP 200 response. Mapping by status alone would map that tocodes.Unknownwith a misleading"200 OK"message.invalid_client/invalid_grant(401 is only a MAY forinvalid_client).GrpcCodeFromHTTPStatus(400)returnsInvalidArgument, which would misclassify exactly the credentials-rejected case this PR exists to fix.Why a transient status overrides the error param
A 429/5xx token-endpoint response has no RFC 6749 error param that reliably indicates a request-level rejection — some providers report throttling using an
errorvalue likeinvalid_requestalongside a 429. Letting the error param win there would turn a transient failure intoInvalidArgument, which isn't retried, discarding a request that would have succeeded on retry.Nil-Response
*oauth2.RetrieveError.Responseis a plain nullable field; the vendoredx/oauth2always populates it when it constructs aRetrieveErrorfrom a real HTTP round trip, but nothing in the type's contract guarantees that for every caller or future version.RetrieveError.Error()unconditionally dereferencesResponse.StatuswhenErrorCodeis empty, which looks like a panic risk on a nilResponse— an earlier revision of this PR made the branch total and stopped joining the raw error into the result specifically to avoid that. Verified (not assumed) that neither was actually necessary for safety: the panic only ever occurs whileRetrieveError.Error()is being formatted via a%s/%vverb — which is exactly howurl.Error.Error()invokes it internally (fmt.Sprintf("%s %q: %s", ..., e.Err)) — and Go'sfmtpackage recovers a panickingError()/String()method during its own formatting, substituting%!s(PANIC=...)instead of propagating. Confirmed this holds througherrors.Jointoo (itsError()method calls each joined error'sError()directly, but that call isurl.Error.Error(), which is itselffmt.Sprintf-based and therefore self-protecting). The branch stays total for cleaner control flow (an oauth2-specific error shouldn't be run through unrelated network-error heuristics), and the result still joins the rawerr— aTestWrapTransientNetworkError_OAuthNilResponseDoesNotPanicOnFormattest pins the no-panic behavior directly rather than relying on this reasoning alone.Downstream behavior changes
pkg/sync/syncer.go'sIsSyncPreservable(frozen per RFC 0009) treatsUnauthenticatedandPermissionDeniedas preservable. A token-exchange failure that previously producedcodes.Unknownhad its sync artifact discarded; after this change it's preserved, matching how a401/403on a normal API call already behaves.codes.Unavailable.retry.Retryer.ShouldWaitAndRetryonly retriesUnavailable/DeadlineExceeded, and the syncer's retryer is configured withMaxAttempts: 0(pkg/sync/parallel_syncer.go:151, meaning unlimited attempts), so a persistently 5xx-ing token endpoint now retries with backoff instead of failing fast on an unclassified error.Both look like the correct, intended consequence of the fix (matching how a normal API response already behaves) rather than regressions, but are called out here since they're real behavior changes for any runner relying on artifact retention or fail-fast behavior around token-endpoint failures.
Verification
TestWrapTransientNetworkErrorcovering:invalid_clienton 401,invalid_clienton the RFC-default 400, anerrorparam on a 200 response,access_denied,invalid_grant(credential failure, notInvalidArgument),unauthorized_client(authenticated-but-not-entitled,PermissionDenied),invalid_scope(InvalidArgument), a status-only fallback with no error param (403), a 429 rate-limit falling back toUnavailable/retryable, an unrecognized vendor-specific error param falling back to the HTTP status while keeping the error param in the message, aRetrieveErrorwith no error param and noResponsestill mapping cleanly instead of falling through, a transient status (429) overriding a recognized error param, a transient status keepingErrorDescriptionalongsideResponse.Statusin the message, 501 correctly excluded from the transient set (it'sUnimplemented, notUnavailable), and 408 correctly included (it'sDeadlineExceeded, also retryable). Each case wraps the*oauth2.RetrieveErrorin*url.Error, the shapehttp.Client.Doactually produces in production, so theerrors.Asunwrap this fix depends on is exercised against real-world shape rather than a bare struct.TestWrapTransientNetworkError_OAuthTransientAttachesRateLimitDetailsasserts aRetry-Afterheader on a 429 produces a*v2.RateLimitDescriptiondetail on the returned status.TestWrapTransientNetworkError_OAuthNilResponseDoesNotPanicOnFormatasserts calling.Error()on the result of a nil-Response/empty-ErrorCodeRetrieveErrordoesn't panic (see "Nil-Response" above).TestWrapTransientNetworkError_OAuthTokenEndpointTransientIsRetried, mirroring the existingTestWrapTransientNetworkError_NXDOMAINIsTerminalpattern) assert against a realretry.Retryer, not just the grpc code: a rate-limited token endpoint is retried, rejected credentials are not.pkg/uhttptest suite passes.golangci-lint run ./pkg/uhttp/... --new-from-rev=<base>reports 0 issues on the diff.baton-kyribabinary (patched via a localreplacedirective), re-verified after each revision: bad credentials went from exit code2(Unknown) to16(Unauthenticated); valid credentials still completed a normal sync with exit0; a synthetic 400-statusinvalid_clientresponse (matching a real-world provider's RFC-default behavior) also correctly mapped toUnauthenticatedrather thanInvalidArgument.Test plan
go build ./...go test ./pkg/uhttp/...golangci-lint run ./pkg/uhttp/...baton-kyriba) via local module replace, including a 400-statusinvalid_clientscenario