Refuse redirects and bound timeouts on the token exchange path in every SDK

Merged
JE
1mo ago
+1591-193

The token-exchange/refresh POST carries the highest-value credentials the SDK ever sends — the authorization code, the client secret, or the refresh token — and after #809 established "no response-steered hop follows redirects" (SPEC §14) and #810 policed the endpoint addresses, it remained the one response-steerable hop that still followed redirects (Go: 10 hops, 307/308 re-POSTing the form; TS: 20) and ran unbounded in Go. This PR makes the exchange path uniform with the device flow and states the contract as SPEC §16 Token-Endpoint Transport Policy [static].

The contract, in every SDK with an exchange path

  • A 301/302/303/307/308 from the token endpoint surfaces as the SDK's typed API error carrying that status, message redirect {status} on the token endpoint is not followed (substring contract: not followed, same as §14), and the Location is never dialled. Any other 3xx (304 above all) stays the generic non-2xx failure.
  • Classification precedes the body read on default transports: a 302 whose body stalls forever is the typed refusal, never a timeout. The one narrowed lane is Ruby's injected-Faraday client (buffered by nature) — it keeps the injected-client fidelity tier's wall-clock-bounded, completed-response classification, stated in the SPEC subsection.
  • Timeouts converge on 30 s default / 3600 s ceiling / invalid-normalizes-to-default, the numbers every other credential POST already uses.
  • Suppression rides injected clients too (device-flow precedent); the §16 address policy deliberately does not — the distinction is stated in SPEC.

Per SDK

  • Go: doTokenRequest carries the POST on a noRedirectClient copy (injected clients included), classifies redirects before the body read, and runs under a child-context deadline — 30 s default, new WithExchangerTimeout (ceiling = the shared device ceiling). AuthManager.refreshLocked gains suppression + classification but no timeout (it runs on the operator-configured API client).
  • Kotlin: postTokenRequest now wraps every client (injected engines re-wrapped, the hardenedDeviceClient pattern) with followRedirects = false + 30 s HttpTimeout; a 3xx is BasecampException.Api with the real status where it previously fell into the generic Auth branch with the status lost; HttpRequestTimeoutException maps to retryable Network.
  • TypeScript: redirect: "manual" + status-first refusal (a browser's opaqueredirect is refused by type, status unavailable); timeoutMs now clamps through the shared resolver (NaN/Infinity no longer instant-abort or unbound); the whole round trip races the abort signal, so a custom fetch that ignores AbortSignal cannot hold the exchange past its deadline. raceAbort/the timeout resolver moved to oauth/limits.ts; the device flow delegates.
  • Python: exchange moved from httpx.post to the shared request_bounded transport — total wall-clock deadline (httpx's own timeout is per I/O phase), streaming 1 MiB cap, redirect statuses skipped via read_body so they classify from the headers with the body unread.
  • Ruby: the default lane moved to the headers-first Fetcher.stream_http transport; an injected Faraday client is vetted by ensure_redirects_suppressed! and bounded by the device flow's full wall-clock discipline; the constructor timeout normalizes (3600 s ceiling). The legacy OauthTokenProvider.perform_refresh, previously a bare unbounded Faraday.post, gets the full contract (stream_http, 30 s, redirect refusal as ApiError with the status).
  • Swift: no exchange path — no change (SPEC already records this).

Corrections recorded

Appendix F previously claimed Kotlin's exchange followed redirects, reasoned from source absence of followRedirects = false. It never did (Ktor's HttpRedirect defaults checkHttpMethod = true; CIO doesn't follow at engine level) — the appendix now retracts that explicitly and names Kotlin's real defects, all closed here. The non-uniformity paragraph collapses to the uniform state.

Conformance scope

Per-SDK unit tests only; the subsection is [static]. The oauth-token corpus schema is resource-semantics-scoped (a response is status + body); one redirect case would need headers/redirect/never-dialled vocabulary — instrument stretching, stated in one SPEC sentence. (The corpus has no 3xx fixtures, so no expected classification changed.)

Tests

Five-status table over BOTH exchange and refresh in every SDK, plus: 304-stays-generic, attacker-Location-never-dialled counters (the attacker serves a usable token, proving refusal stopped the chain), stalled-body-302 headers-first classification (Go, Python, Ruby live-socket), timeout normalization tables, live-timeout bounds, TS's never-settling signal-ignoring fetch, Ruby injected-follower refusal at construction + narrowed-lane pins, and the legacy provider's redirect/no-mutation-on-refusal tests. Every refusal was mutation-verified (suppression/classification reverted → tests fail; restored → green).

Two honest gaps, on the record: Kotlin's followRedirects = false is defense-in-depth no in-repo engine can falsify (Ktor never follows POSTs regardless — the status-first classification is what the tests kill); and Kotlin's 30 s HttpTimeout installation is asserted by code with the exception-mapping tested (runTest's virtual clock cannot advance HttpTimeout's real-dispatcher timer).

Migration

MIGRATING.md "Unreleased" entry: no knob to re-enable following; Go's ctx-less callers now get a 30 s default; Kotlin catch-sites keyed to the old Auth classification of 3xx must move to Api.

Summary by cubic

Refuses token-endpoint redirects and bounds exchange/refresh timeouts across all SDKs. Previously Go/TS followed redirects (Go up to 10 hops, TS up to 20) and Go's exchange could run unbounded; now 301/302/303/307/308 return a typed API error containing "not followed," classification happens before any body read, defaults are 30 s with a 3600 s ceiling (invalid values normalize), and injected clients also refuse redirects (the Location is never dialed). 304 stays on the generic non-2xx path.

  • Go: Exchanger suppresses redirects via a shallow no-redirect client and adds a child-context timeout (30 s default; WithExchangerTimeout, ceiling 3600 s). Redirects classify before reads. AuthManager.Refresh now refuses redirects but keeps the operator client's timeout.
  • Kotlin: Wraps all clients (injected engines included, governing Ktor's client-level redirect plugin) with followRedirects = false and a 30 s HttpTimeout. 3xx now surfaces as BasecampException.Api(httpStatus); HttpRequestTimeoutException maps to retryable Network.
  • TypeScript: Sets redirect: "manual", classifies redirects status-first, and refuses a browser's status-hidden opaqueredirect by type; "not followed" — not the status field — is the portable contract across runtimes. timeoutMs clamps via shared helpers; the request races an AbortSignal so custom fetches cannot hold past the deadline. Shared helpers live in oauth/limits.ts; device flow imports them.
  • Python: Exchange rides the shared request_bounded transport — total wall-clock deadline, 1 MiB streaming cap — with follow_redirects=False, classifying redirects from headers before any body read.
  • Ruby: Default lane uses headers-first Fetcher.stream_http; injected Faraday clients are vetted for redirect suppression and run under a wall-clock deadline; constructor timeout normalizes. Legacy OauthTokenProvider refresh now uses the same transport, 30 s bound, and redirect refusal.
  • SPEC: Adds §16 Token-Endpoint Transport Policy. Appendix F retracts the earlier claim that Kotlin followed redirects and records the closed defects. Tracks cross-SDK enforcement in #818 (per-SDK #814–#817; upstream surfguard #24/#25).

Migration

  • There is no knob to re-enable redirect following; configure token endpoints that answer directly.
  • Go: Calls without a context deadline now time out at 30 s; use WithExchangerTimeout to adjust.
  • Kotlin: Update catch-sites that treated 3xx as Auth; they now surface as Api with the real status.
  • TypeScript: Invalid timeoutMs no longer instant-abort or run unbounded; custom fetches that ignore AbortSignal no longer extend the exchange beyond its timeout.
  • Ruby: Injected Faraday connections must be adapter-only (no redirect middleware); oversized exchange responses now raise OauthError instead of ApiError.