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

MIGRATING.md

@@ -11,6 +11,60 @@ what wrong behaviour you get if you ignore one. This file is that half.
 
 ---
 
+# Unreleased
+
+### All SDKs: token exchange and refresh no longer follow redirects, and every default lane is timeout-bounded (#813)
+
+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 it was the last response-steerable hop that still followed redirects
+anywhere (SPEC §16 "Token-Endpoint Transport Policy"; §14 established the
+rule on the download's signed hop in #805). Go's `Exchanger` followed up to
+ten hops (a 307/308 re-POSTs the form), TypeScript's `exchangeCode`/
+`refreshToken` up to twenty, and Go's `AuthManager.Refresh` followed on
+whatever client it was handed. Kotlin never actually followed — Ktor only
+follows GET/HEAD by default — but threw a 3xx as `BasecampException.Auth`
+with the real status lost. Python and Ruby's default lanes never followed.
+
+All five SDKs with an exchange path now refuse uniformly. A 301, 302, 303,
+307 or 308 — any other 3xx stays the generic non-2xx failure — surfaces as
+the SDK's typed API error carrying that status (`*basecamp.Error` with
+`HTTPStatus: 302`, `BasecampException.Api(httpStatus = 302)`, `BasecampError`
+with `httpStatus: 302`, `OAuthError`/`OauthError` with `http_status=302`)
+with a message saying the redirect is **not followed**, and the `Location` is
+never dialled. This applies on injected clients too: your client keeps its
+transport and address policy, but never re-enables following. There is no
+knob.
+
+Timeouts converged on the same numbers as every other credential POST — 30 s
+default, 3600 s ceiling, invalid values normalize to the default:
+
+- **Go**: `doTokenRequest` was unbounded unless the caller's context carried a
+  deadline. `Exchange`/`Refresh` now run under a 30 s per-request timeout by
+  default; `WithExchangerTimeout` adjusts it. `AuthManager.Refresh` gains no
+  timeout (it runs on the operator-configured API client) but now refuses
+  redirects like every other credential POST.
+- **Kotlin**: the exchange's default client carried no `HttpTimeout`; it now
+  bounds each token request at 30 s, on injected clients too (engine re-wrap,
+  the device-flow pattern). A 3xx is `Api` with the real status where it was
+  `Auth` — update any `catch` that relied on the old taxonomy.
+- **TypeScript**: `timeoutMs` was passed to `setTimeout` unclamped, so `NaN`
+  or `Infinity` disabled or instant-fired the abort; it now normalizes to
+  30 s, and a custom `fetch` that ignores its `AbortSignal` can no longer
+  hold the exchange past the deadline.
+- **Ruby**: the `Exchange` constructor's `timeout:` is normalized (ceiling
+  3600 s), the default lane moved to the headers-first `Fetcher.stream_http`
+  transport, and an injected Faraday client is now vetted for redirect
+  middleware and bounded by a wall-clock deadline — a slow-drip body can no
+  longer hold the request open. The legacy `OauthTokenProvider` refresh,
+  previously unbounded, gets the full 30 s contract and redirect refusal.
+
+**Wrong behaviour you get if you ignore it:** a token endpoint behind a
+redirecting front — a CDN, an http→https rewrite, a host consolidation — now
+fails with the redirect's status instead of silently re-POSTing credentials
+to wherever it pointed. The fix is to configure the endpoint URL that
+actually answers.
+
 # v0.15.0
 
 ### Go: device-flow and token-exchange requests are address-policed by default (#806)

SPEC.md

@@ -2173,6 +2173,51 @@ status still outranks a deadline race; a response completing past the
 deadline without one is refused as a transport timeout. Callers needing exact
 headers-time classification use the default transport.
 
+#### Token-Endpoint Transport Policy `[static]`
+
+Every token-exchange and refresh POST — `exchangeCode`/`refreshToken` and
+their per-SDK spellings, Ruby's legacy `OauthTokenProvider`, and Go's
+`AuthManager` refresh — carries the transport contract the device flow and
+the signed download hop (§14 "Hop-2 Redirect Policy") already hold. The
+device flow's own POSTs already refuse and classify redirects status-first
+under their own messages; the message contract below binds the
+exchange/refresh paths it names:
+
+- **No redirect is followed.** A 301, 302, 303, 307 or 308 from the token
+  endpoint surfaces as the SDK's typed API error (`api_error`) carrying that
+  status, with a message saying the redirect is **not followed** — the same
+  substring contract as §14 — and the `Location` it names is never dialled.
+  A followed 307/308 would re-POST the form — the authorization code, the
+  client secret, or the refresh token — to a destination the response chose.
+  Any other 3xx (304 above all) is the generic non-2xx failure, not this
+  refusal.
+- **Classification precedes the body read on the default transport.** The
+  refusal is read off the status line, so a redirect whose body stalls
+  forever is the typed refusal above, never a timeout. The one permitted
+  exception is an injected buffered client (Ruby's `http_client:` Faraday
+  lane), which has no headers-time seam: it keeps the injected-client
+  fidelity tier's coarser contract stated above — status-first
+  classification on the completed response, wall-clock deadline otherwise.
+- **The request is timeout-bounded**: 30 s default, the shared 3600 s
+  ceiling, and an invalid caller value normalizes to the default — the
+  numbers every other credential POST already converged on. One exclusion:
+  Go's `AuthManager` refresh runs on the operator-configured API client and
+  adds no SDK timeout of its own; it still refuses redirects, because a
+  response-steered hop is response-steered whichever client carries it.
+- **Suppression applies to injected clients too** (the device-flow
+  precedent). Handing these functions your own client keeps your transport,
+  dialer, and address policy — requirement 6's enforcement is deliberately
+  NOT layered on top — but never re-enables redirect following. The
+  distinction: redirect refusal is a property of the token-request
+  functions themselves; the address policy is a property of the default
+  client they post on.
+
+Marked `[static]` (per-SDK unit tests), not `[conformance]`: the oauth-token
+corpus schema is scoped to resource semantics — a response is a status and a
+body — and expressing even one redirect case would mean adding headers,
+redirect, and never-dialled vocabulary to that schema, stretching the
+instrument past what it observes.
+
 ### Launchpad Legacy Format
 
 The Basecamp Launchpad OAuth endpoints use a mix of standard and legacy parameters:
@@ -4129,6 +4174,8 @@ Every operation has a `retry` block, including non-idempotent POSTs. For non-ide
 in Go only. This is a deliberate Go-first move, not an oversight in the other
 five: the enforcement seam it needs (a dial-time `Control` hook, plus a shared
 classification table) exists cheaply in Go and does not in the others.
+Extending enforcement to the remaining SDKs is tracked in #818 (umbrella;
+per-SDK #814/#815/#816/#817, upstream `surfguard` #24/#25).
 
 | SDK | Advertised-issuer hop |
 |-----|----------------------|
@@ -4151,7 +4198,8 @@ only derivation that pierces them — so an on-premises policy is built as
 time, on every credential-bearing POST — is likewise implemented in Go only,
 with the same policy and the same override shape as the issuer hop. The
 per-SDK state, and the seam each SDK would need, so the follow-ups are
-specified rather than rediscovered:
+specified rather than rediscovered (tracked in #818; per-SDK
+#814/#815/#816/#817):
 
 | SDK | Device-authorization and token endpoint POSTs |
 |-----|-----------------------------------------------|
@@ -4162,23 +4210,27 @@ specified rather than rediscovered:
 | Kotlin | Scheme gate, bounded timeout, `followRedirects = false`, bounded body. No classification tables; the JVM seam is OkHttp's `Dns` interface (OkHttp connects to exactly the addresses it returns, so filtering there is connect-time judgement), with no multiplatform equivalent |
 | Swift | Not applicable — ships no OAuth device flow or discovery |
 
-Two things hold in every SDK, policy or not, and are not what this divergence
-is about: the scheme gate and the bounded body. Two more are NOT uniform on
-the exchange path, and listing them as universal is how a reader infers a
-guarantee nobody implemented. The bounded timeout holds on the device flow in
-all four, but Go's `doTokenRequest` bounds nothing itself — the caller's
-context is the only deadline, and the shared policy client deliberately
-carries no client timeout — and Kotlin's `postTokenRequest` builds its default
-client without `HttpTimeout`; TS (30 s), Python (`_TOKEN_TIMEOUT`), and Ruby
-(Faraday timeouts) do bound theirs. Redirect suppression: Go's `Exchanger` and
-Kotlin's `exchangeCode`/`refreshToken` follow redirects (TS's exchange passes
-no `redirect:` option, so it follows too), where the device flow suppresses
-them in all four — a 307 re-POSTs the credentials to the `Location`. Under
-Go's policy client each redirect hop's dial is judged, so the address policy
-holds across a redirect; the public-host re-POST does not need the policy to
-be exploitable. This is the same cross-SDK shape #805 had on the download's
-signed hop before #809 closed it there — the exchange path is now the
-remaining redirect-following exception.
+Four things now hold in every SDK with an exchange path, policy or not, and
+are not what this divergence is about: the scheme gate, the bounded body, the
+bounded timeout, and redirect suppression. The last two were the exchange
+path's own divergence — the same cross-SDK shape #805 had on the download's
+signed hop before #809 closed it there — until §16 "Token-Endpoint Transport
+Policy" closed it here: every token-exchange/refresh POST refuses the five
+redirect statuses with a typed `api_error` carrying the real status, and
+every default lane runs under the shared 30 s default / 3600 s ceiling
+(Go's `AuthManager` refresh keeps its operator-configured client's timeout
+and refuses redirects like every other credential POST).
+
+One earlier claim in this section deserves a retraction rather than a silent
+edit: previous revisions said Kotlin's `exchangeCode`/`refreshToken` followed
+redirects, reasoned from the absence of `followRedirects = false` in the
+source. They never followed — Ktor's `HttpRedirect` plugin defaults
+`checkHttpMethod = true` (only GET/HEAD follow) and the CIO engine does not
+follow at engine level. Kotlin's real defects were adjacent: a 3xx was thrown
+through the generic error branch as `BasecampException.Auth` with the real
+status lost, the default exchange client carried no `HttpTimeout`, and an
+injected client was used verbatim, leaving redirect behavior to the engine
+the caller happened to pick. All three are closed by the same policy above.
 
 The behavioral tightening is the same as the issuer hop's, and it reaches one
 more consumer shape: a Go caller that hand-configures a loopback or RFC 1918

go/pkg/basecamp/auth.go

@@ -359,12 +359,27 @@ func (m *AuthManager) refreshLocked(ctx context.Context, origin string, creds *C
 	}
 	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 
-	resp, err := m.httpClient.Do(req) // #nosec G704 -- SDK HTTP client: URL is caller-configured
+	// The stored endpoint's response steers where a followed redirect would
+	// re-POST the refresh token, so this hop never follows one (SPEC §16
+	// "Token-Endpoint Transport Policy") — same as the signed download hop and
+	// the oauth package's exchange. A shallow copy: the operator-configured
+	// client is never mutated, and keeps every other property it was built with.
+	client := *m.httpClient
+	client.CheckRedirect = func(*http.Request, []*http.Request) error {
+		return http.ErrUseLastResponse
+	}
+	resp, err := client.Do(req) // #nosec G704 -- SDK HTTP client: URL is caller-configured
 	if err != nil {
 		return ErrNetwork(err)
 	}
 	defer func() { _ = resp.Body.Close() }()
 
+	// Classified by status BEFORE the body read, so a redirect with a stalled
+	// body fails here rather than hanging in limitedReadAll.
+	if isRedirectStatus(resp.StatusCode) {
+		return ErrAPI(resp.StatusCode, fmt.Sprintf("redirect %d on the token endpoint is not followed", resp.StatusCode))
+	}
+
 	if resp.StatusCode != http.StatusOK {
 		body, _ := limitedReadAll(resp.Body, MaxErrorBodyBytes)
 		return ErrAPI(resp.StatusCode, fmt.Sprintf("token refresh failed: %s", truncateString(string(body), MaxErrorMessageBytes)))

go/pkg/basecamp/auth_test.go

@@ -4,9 +4,11 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 )
@@ -671,3 +673,65 @@ func TestAuthManager_Refresh_ExplicitNonPositiveExpiresInIsAPIError(t *testing.T
 		ts.Close()
 	}
 }
+
+// TestAuthManager_Refresh_RefusesTokenEndpointRedirects pins the stored-
+// endpoint refresh to the same transport policy as the exchange path (SPEC
+// §16 "Token-Endpoint Transport Policy"): every refused redirect is a typed
+// api_error carrying its status, the Location host is never dialed — the
+// operator's client would otherwise follow, 307/308 re-POSTing the refresh
+// token — and the stored credentials survive untouched.
+func TestAuthManager_Refresh_RefusesTokenEndpointRedirects(t *testing.T) {
+	for _, status := range []int{301, 302, 303, 307, 308} {
+		t.Run(fmt.Sprintf("%d", status), func(t *testing.T) {
+			t.Setenv("BASECAMP_TOKEN", "")
+			t.Setenv("BASECAMP_NO_KEYRING", "1")
+
+			var hits atomic.Int64
+			target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+				hits.Add(1)
+				w.Header().Set("Content-Type", "application/json")
+				_, _ = w.Write([]byte(`{"access_token":"stolen","expires_in":3600}`))
+			}))
+			defer target.Close()
+
+			ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+				w.Header().Set("Location", target.URL)
+				w.WriteHeader(status)
+			}))
+			defer ts.Close()
+
+			store := &CredentialStore{useKeyring: false, fallbackDir: t.TempDir()}
+			origin := NormalizeBaseURL(ts.URL)
+			_ = store.Save(origin, &Credentials{
+				AccessToken:   "old-access",
+				RefreshToken:  "old-refresh",
+				ExpiresAt:     1,
+				TokenEndpoint: ts.URL + "/token",
+			})
+
+			m := NewAuthManagerWithStore(&Config{BaseURL: ts.URL}, ts.Client(), store)
+
+			err := m.Refresh(context.Background())
+			var be *Error
+			if !errors.As(err, &be) {
+				t.Fatalf("Refresh() error = %v, want *Error", err)
+			}
+			if be.Code != CodeAPI {
+				t.Errorf("Code = %q, want %q", be.Code, CodeAPI)
+			}
+			if be.HTTPStatus != status {
+				t.Errorf("HTTPStatus = %d, want %d", be.HTTPStatus, status)
+			}
+			if !strings.Contains(be.Message, "not followed") {
+				t.Errorf("Message = %q, want it to contain %q", be.Message, "not followed")
+			}
+			if got := hits.Load(); got != 0 {
+				t.Errorf("Location target hits = %d, want 0", got)
+			}
+			creds, _ := store.Load(origin)
+			if creds.AccessToken != "old-access" {
+				t.Errorf("AccessToken = %q, want the stored credentials untouched", creds.AccessToken)
+			}
+		})
+	}
+}