fix: surface DPoP proof rejections instead of "unknown error" (CXP-1002) - #202
fix: surface DPoP proof rejections instead of "unknown error" (CXP-1002)#202agustin-conductor wants to merge 1 commit into
Conversation
f5ed03a to
388b2cc
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // "the API returned an unknown error". Restating the header as the JSON the SDK | ||
| // expects turns that back into the reason Okta actually gave. | ||
| func annotateEmptyDPoPError(resp *http.Response, body []byte) { | ||
| if len(bytes.TrimSpace(body)) > 0 { |
There was a problem hiding this comment.
🟡 Suggestion: annotateEmptyDPoPError infers "body is empty" from the body argument, but sniffClientErrorBody only populates it for 4xx — it returns nil for 2xx/3xx/5xx. Since finish is the single exit for every response, a 200 or 5xx whose WWW-Authenticate carries error=/error_description= params (RFC 9110 permits the header on any status) would have its real body closed and replaced by the synthesized JSON. Worth guarding with the same range the sniffer uses, e.g. if resp.StatusCode < 400 || resp.StatusCode >= 500 { return }. (medium confidence — not observed on the probed tenants, but the invariant is only held by the caller today)
There was a problem hiding this comment.
Addressed in 25a1ddb — you were right that the invariant was only held by the caller. Added if resp.StatusCode < 400 || resp.StatusCode >= 500 { return } inside the function, so it no longer infers anything from an empty body argument outside the range where sniffClientErrorBody populates it. TestAnnotateEmptyDPoPError_OnlyTouchesClientErrors covers 200/302/500 with a real body plus a challenge header present.
| if !strings.Contains(strings.ToLower(challenge), "dpop") && !strings.Contains(errCode, "dpop") { | ||
| return | ||
| } | ||
| ctxzap.Extract(ctx).Warn("oktaauth: dpop failure on resource request", |
There was a problem hiding this comment.
🟡 Suggestion: this Warn is unsampled and fires once per failing request. Since the PR deliberately does not retry a generic invalid_dpop_proof (clock skew), a skewed clock means every request in a sync emits this line with the full challenge string — thousands per sync, which is exactly the alert-noise case the repo criteria call out (L7). Consider logarithmic sampling (1, 10, 100, every 1000) with a total_occurrences field.
There was a problem hiding this comment.
Addressed in 25a1ddb with logarithmic sampling per L7 — 1, 10, 100, then every 1000, with a total_occurrences field. The counter lives on the dpopRoundTripper rather than in a package var so two connectors in one process don't share it.
(It briefly went to Debug instead; your follow-up on that thread was the better call and it's back at Warn.)
|
|
||
| // authParam returns the quoted value of one auth-param, matching only at a | ||
| // parameter boundary so "error" does not match inside "error_description". | ||
| func authParam(challenge, name string) string { |
There was a problem hiding this comment.
🟡 Suggestion: authParam scans the entire WWW-Authenticate value without scoping to the DPoP scheme. RFC 9110 §11.6.1 allows several challenges in one header (Bearer error="invalid_token", DPoP error="use_dpop_nonce"), and the first boundary match wins — so a Bearer challenge's error/error_description would be reported as the DPoP reason and synthesized into the response body. Scoping the scan to the substring starting at the DPoP scheme token would make it match the function's stated intent.
There was a problem hiding this comment.
Addressed in 25a1ddb, though not the way you suggested — the parser is gone entirely.
Scoping the scan fixed one ordering and left the reverse broken (you caught that in the follow-up), and correct scheme attribution needs a real RFC 9110 parser. The synthesis doesn't actually need attribution: it only has to hand the SDK the challenge the SDK is about to discard. So the whole header is now restated verbatim as errorSummary, and this class of bug dissolves rather than being fixed — nothing is attributed, so nothing can be misattributed. Net −121 lines.
Bonus: resource and scope now survive into the error too, which the parsed version dropped.
| func bodyExcerpt(body []byte) string { | ||
| excerpt := strings.TrimSpace(string(body)) | ||
| if len(excerpt) > errorBodyExcerptLimit { | ||
| return excerpt[:errorBodyExcerptLimit] + "..." |
There was a problem hiding this comment.
🟡 Suggestion: slicing at a fixed byte offset can cut a multi-byte UTF-8 rune in half, so the excerpt embedded in the returned error (and any structured log field built from it) can carry invalid UTF-8. Backing up to a rune boundary — e.g. for !utf8.ValidString(excerpt[:n]) { n-- }, or strings.ToValidUTF8 on the result — keeps the message clean.
There was a problem hiding this comment.
Addressed in 25a1ddb. Went with the rune-boundary backup rather than strings.ToValidUTF8 — utf8.RuneStart walks back at most 3 bytes with no allocation, where ToValidUTF8 rescans and rebuilds the string. Extracted as truncateAtRuneBoundary and used by both truncation paths (see your follow-up on the challenge one).
Connector PR Review: fix: surface DPoP proof rejections instead of "unknown error" (CXP-1002)Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryScanned the full PR diff ( Security IssuesNone found. The DPoP failure log emits only the Correctness IssuesNone found. Suggestions
Prompt for AI agents |
388b2cc to
136af7d
Compare
| // Three-byte runes tile the limit so it lands mid-rune. | ||
| body := strings.Repeat("é", errorBodyExcerptLimit) |
There was a problem hiding this comment.
🟡 Suggestion: this test doesn't exercise the bug it names. é (U+00E9) is two bytes, not three, so strings.Repeat("é", 200) is 400 bytes and excerpt[:200] lands exactly on a rune boundary (100 whole runes). utf8.ValidString would pass even with a plain excerpt[:errorBodyExcerptLimit] and no strings.ToValidUTF8, so the guard added in bodyExcerpt is untested. Use a genuine three-byte rune so 200 doesn't divide evenly — e.g. strings.Repeat("€", errorBodyExcerptLimit) (or "あ"), which puts the cut mid-rune at byte 200.
There was a problem hiding this comment.
Correct, and it was my error — é is two bytes, so 200 divided evenly and the test proved nothing. Fixed in 25a1ddb by switching to € (three bytes, so the cut lands mid-rune at byte 200).
Verified by mutation this time: with the guard removed the test fails with excerpt is not valid UTF-8: "€€€…\xe2\x82...", and passes with it restored.
| if !strings.Contains(strings.ToLower(challenge), "dpop") && !strings.Contains(errCode, "dpop") { | ||
| return | ||
| } | ||
| ctxzap.Extract(ctx).Debug("oktaauth: dpop failure on resource request", |
There was a problem hiding this comment.
🟡 Suggestion: this is the diagnostic the PR describes as "Warn on any DPoP-related 4xx … what will identify replay vs. clock skew on the next occurrence", but at Debug it won't be emitted in a production sync, so the next occurrence still gives you nothing. Repo criteria L1 puts upstream 4xx at Warn; L7's answer to a per-request warning is logarithmic sampling (1, 10, 100, every 1000) with a total_occurrences field, not silencing. Consider Warn + sampling here, or at minimum update the PR description so it matches the code.
There was a problem hiding this comment.
Agreed, and reverted in 25a1ddb — back to Warn, with the logarithmic sampling from L7 (1, 10, 100, every 1000) and a total_occurrences field, so the noise concern is handled by rate-limiting rather than by silencing.
You were also right about the description mismatch; the PR body now states the sampling and cites L1/L7.
| func dpopChallengeScope(challenge string) string { | ||
| for i := 0; i+len(dpopHdr) <= len(challenge); i++ { | ||
| if !strings.EqualFold(challenge[i:i+len(dpopHdr)], dpopHdr) { | ||
| continue | ||
| } | ||
| after := i + len(dpopHdr) | ||
| startsToken := i == 0 || challenge[i-1] == ' ' || challenge[i-1] == ',' | ||
| // A scheme token is followed by whitespace or ends the value; anything else | ||
| // means this is a parameter name that merely begins with the same letters. | ||
| endsToken := after >= len(challenge) || challenge[after] == ' ' | ||
| if startsToken && endsToken { | ||
| return challenge[i:] | ||
| } | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the scope is bounded at the start but not at the end — it returns challenge[i:] all the way to the end of the header. With the challenges in the other order (DPoP algs="RS256", Bearer error="invalid_token", error_description="wrong scheme"), authParam finds the Bearer params and they get reported as the DPoP reason and synthesized into the error body by annotateEmptyDPoPError. TestDPoPChallengeParams_ScopedToDPoPScheme only covers "bearer challenge first", so this order isn't caught. Terminate the slice at the next scheme token (a bare token followed by whitespace, not name="), and add a DPoP-first test case. Related: isResourceNonceChallenge (line 217) still does an unscoped strings.Contains on the whole header, so it doesn't get this scoping at all.
There was a problem hiding this comment.
Right on both counts — bounding only the start left the reverse ordering broken, and isResourceNonceChallenge wasn't scoped at all.
Rather than add end-bounding, 25a1ddb removes the parser: the challenge is restated verbatim, so no attribution happens and neither ordering can misattribute. isResourceNonceChallenge needs no scoping either, since use_dpop_nonce is a DPoP-specific code. Tests cover a Bearer-only header and a mixed header, asserting the neighbouring challenge is preserved rather than swapped in.
136af7d to
c635520
Compare
| zap.Int("send", send), | ||
| zap.Bool("proof_replay", reason == dpopRetryProofReplay), | ||
| ) | ||
| _, _ = io.Copy(io.Discard, resp.Body) |
There was a problem hiding this comment.
🟡 Suggestion: io.Copy(io.Discard, resp.Body) dereferences resp.Body unguarded, while both sniffClientErrorBody (line 328) and annotateEmptyDPoPError (line 266) explicitly nil-check it. A transport that returns a nil-bodied 4xx carrying a use_dpop_nonce challenge header reaches this line via dpopRetryNonce and panics. net/http's own transport always sets a body so this is unlikely in production, but the guard is inconsistent with the rest of the file — a if resp.Body != nil wrapper around the drain/close pair would settle it.
There was a problem hiding this comment.
Confirmed a real panic and fixed in 25a1ddb. io.Discard implements ReaderFrom, so io.Copy calls Discard.ReadFrom(nil) which calls Read on a nil interface:
PANIC: runtime error: invalid memory address or nil pointer dereference
Your path analysis holds too — sniffClientErrorBody returns early on a nil body without replacing it, so nothing intervenes before the drain. Guarded the drain/close pair, and TestRoundTripper_NilResponseBodyDoesNotPanic uses a stub transport that returns a nil-bodied 4xx with a nonce challenge. Verified by mutation: without the guard the test panics.
| return | ||
| } | ||
| if len(challenge) > maxChallengeSummaryLen { | ||
| challenge = challenge[:maxChallengeSummaryLen] + "..." |
There was a problem hiding this comment.
🟡 Suggestion: challenge[:maxChallengeSummaryLen] cuts at a byte offset and can split a multi-byte rune — the same hazard the sibling bodyExcerpt in pkg/connector/helpers.go:141 guards with strings.ToValidUTF8, and which TestBodyExcerpt_TruncatesOnRuneBoundary pins. Impact here is cosmetic (json.Marshal substitutes U+FFFD rather than erroring, and WWW-Authenticate is ASCII in practice), but applying the same strings.ToValidUTF8 keeps the two truncation paths consistent.
There was a problem hiding this comment.
Addressed in 25a1ddb — both paths now share truncateAtRuneBoundary, which backs up over continuation bytes with utf8.RuneStart.
Agreed the impact here is cosmetic: RFC 9110 §5.5 makes header field values US-ASCII, so a multi-byte rune in WWW-Authenticate is already out of spec. Consistency was the right reason to do it.
| // Both markers are DPoP-specific, so finding them anywhere in the header is | ||
| // unambiguous; no need to work out which challenge they belong to. | ||
| if strings.Contains(challenge, invalidDPoPProofErrorCode) && strings.Contains(challenge, proofReplayDesc) { | ||
| return dpopRetryProofReplay |
There was a problem hiding this comment.
🟡 Suggestion: the comment says "both markers are DPoP-specific", but proofReplayDesc = "already been used" is plain English, not a DPoP token — the two strings.Contains checks match independently anywhere in the header. A multi-challenge WWW-Authenticate carrying a DPoP clock-skew rejection plus a Bearer error_description="...already been used" would be classified as a replay and resent. Bounded at maxDPoPRetrySends, so the cost is a couple of wasted requests, and TestRoundTripper_BearerChallengeDoesNotTriggerProofRetry covers only the Bearer-only header, not the mixed one. Worth either matching proofReplayDesc only within the DPoP challenge or softening the comment's claim.
There was a problem hiding this comment.
You're right — the comment overclaimed. invalid_dpop_proof is DPoP-specific; "already been used" is plain English.
Fixed the code rather than softening the comment: 25a1ddb widens proofReplayDesc to "DPoP proof JWT has already been used", which is DPoP-specific, so the claim now holds and the cross-challenge false positive is gone. Fail-safe direction too — a reworded description means no retry, i.e. the pre-PR behavior.
Added TestRoundTripper_MixedChallengeDoesNotTriggerProofRetry for the case you named: DPoP rejecting for clock skew alongside a Bearer challenge mentioning reuse → 1 call, no retry.
c635520 to
25a1ddb
Compare
| prefix, err := io.ReadAll(io.LimitReader(resp.Body, errorBodySniffLimit)) | ||
| resp.Body = restoredBody{ | ||
| Reader: io.MultiReader(bytes.NewReader(prefix), resp.Body), | ||
| Closer: resp.Body, | ||
| } | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| return prefix |
There was a problem hiding this comment.
🟡 Suggestion: on a partial-read error prefix has already been restored onto resp.Body, but returning nil tells the callers there was no body at all. finish then hands that nil to annotateEmptyDPoPError, which sees an empty body and replaces the restored partial payload with the challenge restatement (and retryableDPoPFailure loses a body-carried use_dpop_nonce). Returning prefix unconditionally keeps whatever did arrive; the restore path is already correct either way.
| prefix, err := io.ReadAll(io.LimitReader(resp.Body, errorBodySniffLimit)) | |
| resp.Body = restoredBody{ | |
| Reader: io.MultiReader(bytes.NewReader(prefix), resp.Body), | |
| Closer: resp.Body, | |
| } | |
| if err != nil { | |
| return nil | |
| } | |
| return prefix | |
| prefix, err := io.ReadAll(io.LimitReader(resp.Body, errorBodySniffLimit)) | |
| resp.Body = restoredBody{ | |
| Reader: io.MultiReader(bytes.NewReader(prefix), resp.Body), | |
| Closer: resp.Body, | |
| } | |
| // Even on a read error, prefix holds the bytes that did arrive and has been | |
| // restored onto the body -- report them rather than claiming an empty body, | |
| // which would let annotateEmptyDPoPError overwrite real content. | |
| return prefix | |
| } |
There was a problem hiding this comment.
Addressed in fffa61c — accepted, prefix is now returned unconditionally. Your trace was right about the consequence; the mutation test shows it directly, with the partial payload replaced by the challenge restatement when the old return nil is restored.
One note on the snippet: it keeps prefix, err := and then never uses err, which does not compile. Used prefix, _ := with a comment explaining the read error is dropped because the caller meets it again on its own read.
Worth adding that this path is not hypothetical for this PR — a body dying mid-read is the same stale-connection scenario the proof-replay hypothesis rests on.
| bytes, err := io.ReadAll(response.Body) | ||
| if err != nil { | ||
| return okta.Error{}, err | ||
| return okta.Error{}, fmt.Errorf("okta-connectorv2: %s: read error body: %w", response.Status, err) | ||
| } | ||
|
|
||
| // An empty or non-JSON body used to surface as a bare "unexpected end of JSON | ||
| // input" with no status code, which made an empty 400 indistinguishable from | ||
| // an empty 403 in logs. | ||
| err = json.Unmarshal(bytes, &errOkta) | ||
| if err != nil { | ||
| return okta.Error{}, err | ||
| return okta.Error{}, fmt.Errorf("okta-connectorv2: %s: unparseable error body %q: %w", response.Status, bodyExcerpt(bytes), err) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the new messages add the status text and the prefix, but still carry no gRPC code. All five call sites (group.go:211, role.go:456/498, app.go:407/479) return this verbatim, so an unparseable 403 body surfaces as codes.Unknown, while a parseable one reaches handleOktaResponseError → WrapErrorsWithRateLimitInfo(GrpcCodeFromHTTPStatus(...)) and gets PermissionDenied. Same upstream failure, two different codes depending only on whether Okta sent a body. Consider wrapping with uhttp.WrapErrors(uhttp.GrpcCodeFromHTTPStatus(response.StatusCode), ...) here so the retry/surface decision is driven by the status either way.
There was a problem hiding this comment.
Addressed in fffa61c — the asymmetry was real: an unparseable 403 reached the sync as codes.Unknown while a parseable one became PermissionDenied for the same upstream failure, and a 429 was retryable only if Okta happened to send a body.
getError now routes both failure paths through uhttp.WrapErrors(uhttp.GrpcCodeFromHTTPStatus(response.StatusCode), ...) in a small bodyReadError helper, keeping the connector prefix, the status text, and the body excerpt in the message. TestGetError_ClassifiesByHTTPStatus covers 403/429/404/400, asserting both the code and that the message still carries prefix and status.
Okta answers a bad DPoP proof with a 400, a zero-length body, and the reason in a DPoP-scheme WWW-Authenticate header. The vendored SDK's CheckResponseForError reads that header only for 401/403 responses whose scheme is Bearer, then discards the decode failure on the empty body, so every such rejection reached the caller as "the API returned an unknown error" and failed the whole sync with no way to tell what happened. Confirmed live against a DPoP-enabled tenant. A replayed proof jti, an iat more than five minutes old, and an iat in the future all produce that exact shape; an unsolicited nonce is accepted, and no resource response ever offers a DPoP-Nonce, so the resource server issues no nonce challenges at all. oktaauth: - restate the WWW-Authenticate challenge as the error body the SDK expects, so it renders what Okta actually said. The challenge is passed through whole rather than parsed into auth-params: RFC 9110 allows several challenges in one header, attributing a param to the right scheme needs a real parser, and quoting the header drops nothing and cannot misattribute one scheme's error to another. Bounded to client errors with a genuinely empty body, so a real body is never replaced -- including a body that failed partway through being read, whose bytes are reported rather than mistaken for an empty body. - retry once with a fresh proof when Okta reports a replayed proof jti; uhttp's transport retries a failed request with its headers untouched, so a stale-connection retry resends the same proof. Matched on the full DPoP-specific phrase, not on "already been used" alone, which another scheme's challenge in the same header could carry. A generic invalid_dpop_proof is not retried, since a skewed clock would only triple the traffic. - log any DPoP-related 4xx at Warn with the full challenge, before the SDK discards it, logarithmically sampled with a total_occurrences field: a skewed clock is not retried, so an unsampled line would fire once per request for a whole sync - guard the retry drain against a nil response body. net/http always sets one, but io.Copy on a nil ReadCloser panics and the two neighbouring helpers already check it. - match a nonce challenge on 400 as well as 401, and in the JSON-body shape as well as the header, with a bounded retry loop. Defensive: unreachable on the tenants observed, but correct per RFC 9449 and consistent with the token endpoint's existing handling connector: - getError now names the HTTP status, excerpts an undecodable body, and carries a grpc code derived from that status. Its five call sites return its error verbatim, so an empty body surfaced as a bare "unexpected end of JSON input" with no status and no prefix, and an unparseable body reached the sync as codes.Unknown while a parseable one became PermissionDenied for the same upstream failure. #200 rewrote handleOktaResponseError but left getError untouched. - the event feed no longer returns the SDK error bare, so it picks up a grpc code and the status line like every other call site. #200 did not touch event_log.go. Both truncation paths back up to a rune boundary rather than cutting at a byte offset, so an excerpt reaching an error message or a log field stays valid UTF-8. Status codes and grpc codes for everything routed through handleOktaResponseError, including 429, come from #200's GrpcCodeFromHTTPStatus fallback; no duplicate handling is added here. Validated with four clean full syncs against a live tenant (119 actions, no errors) plus unit coverage of each path using the captured challenge strings. The panic guard, the partial-read fix, and both rune-boundary cuts are verified by mutation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
25a1ddb to
fffa61c
Compare
mateoHernandez123
left a comment
mateoHernandez123
left a comment
There was a problem hiding this comment.
Looks good — the empty-body DPoP 400 now surfaces Okta's reason and the replay retry is gated on the live challenge string.
|
|
||
| // truncateAtRuneBoundary cuts s to at most maxBytes without splitting a rune, so | ||
| // the result stays valid UTF-8. A raw byte slice can leave a partial rune behind. | ||
| func truncateAtRuneBoundary(s string, maxBytes int) string { |
There was a problem hiding this comment.
This is a verbatim duplicate of truncateAtRuneBoundary added in pkg/connector/helpers.go:160 by this same PR. Also, pkg/oktaauth/token_source.go:309's pre-existing formatTokenError has the exact bug this function guards against (excerpt[:errorBodyExcerptLimit] can split a UTF-8 rune) and isn't updated to use it. Suggest consolidating to one rune-safe truncation helper in this package and pointing formatTokenError at it too, rather than shipping the fix twice while leaving a live instance of the bug unfixed next to it.
4 participants
Fixes the
the API returned an unknown errorfailures that were breaking full syncs onbaton-oktaconnectors using OAuth 2.0 / private-key auth. CXP-1002Rebased onto #201. That PR rewrote
handleOktaResponseError, which overlapped with an earlier version of this branch — see Relationship to #200 below for what was dropped as redundant.The problem
Okta answers a bad DPoP proof with a 400, a zero-length body, and the reason in a
DPoP-schemeWWW-Authenticateheader.The vendored SDK is structurally blind to that.
CheckResponseForError(requestExecutor.go:634-651) readsWWW-Authenticateonly when the status is 401/403 and the header containsBearer— it misses on both counts here. It then runs_ = json.NewDecoder(...).Decode(&e)on the empty body, discarding the decode error, and returns a zero-valuedokta.ErrorwhoseError()isthe API returned an unknown error. One failed call cancels the sync, so a ~0.16% call-level failure rate stopped full syncs from ever completing.Root cause, confirmed live
Probed against a DPoP-enabled tenant with hand-crafted proofs:
WWW-Authenticatejti)error="invalid_dpop_proof","The DPoP proof JWT has already been used."iat10 min past"…issued more than five minutes in the past."iat10 min future"…issued in the future."So the trigger is
invalid_dpop_proof, notuse_dpop_nonceas the ticket originally concluded. A likely replay path exists in our own stack:uhttp's transport retries on stale connections (transport.go:292) andretryableRequestreturns the request with headers untouched — same proof, samejti— and it sits below this round-tripper, so no fresh proof is minted. Clock skew produces the identical signature.The nonce theory is ruled out: 59/59 instrumented resource calls sent a proof with no
nonceclaim and got 200, Okta never returned aDPoP-Nonceon a resource response, and an unsolicited stale nonce is simply ignored. Okta's nonce requirement is authorization-server only, andtoken_source.goalready handles that correctly.Changes
pkg/oktaauth/round_tripper.go— the substance of this PR.invalid_dpop_proofis deliberately not retried — a skewed clock would only triple the traffic.The challenge is passed through whole rather than parsed into its auth-params. RFC 9110 allows several challenges in one header (
Bearer error="invalid_token", DPoP error="use_dpop_nonce"), and attributing a param to the right scheme needs a real parser — quoting the header drops nothing (resourceandscopesurvive too) and cannot misattribute one scheme's error to another. Bounded to client errors with an empty body, so a real body is never replaced.total_occurrencesfield, perci-review.mdL1 and L7, since a skewed clock is not retried and would otherwise emit one line per request for a whole sync. This is what will identify replay vs. clock skew on the next occurrence.pkg/connector/helpers.go—getErrornow names the HTTP status and excerpts an undecodable body. Its five call sites (group.go:211,role.go:456/498,app.go:407/479) all return its error verbatim, so an empty body surfaced as a bareunexpected end of JSON inputwith no status and no prefix. #200 rewrotehandleOktaResponseErrorbut leftgetErroruntouched.pkg/connector/event_log.go— the event feed no longer returns the SDK error bare, so it picks up a grpc code and the status line like every other call site. #200 did not touch this file.Relationship to #200
#200's fallback is
WrapErrorsWithRateLimitInfo(GrpcCodeFromHTTPStatus(status), ...), and that helper opens withstatus.New(preferredCode, resp.Status)— so the status line already becomes the error message for every non-2xx, with a meaningful grpc code (400→InvalidArgument, 403→PermissionDenied, 429→Unavailable) and rate-limit details attached.Two changes from the pre-rebase version of this branch were therefore dropped as redundant:
429 → codes.Unavailablemapping —GrpcCodeFromHTTPStatusalready does this, and also carries rate-limit infoisOpaqueOktaErrorbranch that named the status for zero-valued errors — Return GRPC statuses for more Okta errors. #200 does it for all non-2xx, more generallyTheir
helpers_test.gocovers those paths (14 cases, including 429), so the corresponding tests here were dropped too. Nothing in this PR duplicates #200's handling.Testing
pkg/oktaauth/nonce_challenge_test.go— body-sniff restoration (including a body twice the sniff limit, and that 200s are never buffered), replay-retry vs. clock-skew-no-retry, the auth-param boundary parser, bounded retries, and an end-to-end check that the realokta.CheckResponseForErrornow renders Okta's reason. Fixtures are the verbatim challenge strings captured from the live probe.pkg/connector/get_error_test.go— status naming and body excerpting on an undecodable body, that the excerpt trims to a rune boundary (verified by mutation: removing the guard fails the test), and that a decodable body still parses.go build,go vet,golangci-lintclean (only pre-existinggoconsthits inactions.go).Notes for review
nonce-retry, which undersells it — the nonce widening is the defensive part; the proof-rejection handling is the actual fix.okta.logs.readis never requested — is split out as CXP-1021 and deliberately not in this PR, since it changes the scope set for every existing OAuth customer.🤖 Generated with Claude Code