diff --git a/MIGRATING.md b/MIGRATING.md index f4f70212b..5b719d6a3 100644 --- a/MIGRATING.md +++ b/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) diff --git a/SPEC.md b/SPEC.md index a126aac2a..acefddd77 100644 --- a/SPEC.md +++ b/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 diff --git a/go/pkg/basecamp/auth.go b/go/pkg/basecamp/auth.go index 4736f0567..169a176e2 100644 --- a/go/pkg/basecamp/auth.go +++ b/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))) diff --git a/go/pkg/basecamp/auth_test.go b/go/pkg/basecamp/auth_test.go index 7547818a7..af15de714 100644 --- a/go/pkg/basecamp/auth_test.go +++ b/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) + } + }) + } +} diff --git a/go/pkg/basecamp/oauth/exchange.go b/go/pkg/basecamp/oauth/exchange.go index 063489940..6948ab2fa 100644 --- a/go/pkg/basecamp/oauth/exchange.go +++ b/go/pkg/basecamp/oauth/exchange.go @@ -23,9 +23,12 @@ import ( // may be one that DiscoverFromResource's metadata chose. By default the // Exchanger therefore carries those POSTs on a client that judges the // endpoint's literal address at dial time against [DefaultIssuerPolicy]; see -// [NewExchanger] for the overrides. +// [NewExchanger] for the overrides. It never follows a redirect from the +// token endpoint — a 3xx surfaces as a typed api_error — and bounds each +// request at [WithExchangerTimeout]'s deadline (30 s by default). type Exchanger struct { httpClient *http.Client + timeout time.Duration } // ExchangerOption configures an Exchanger at construction. @@ -34,6 +37,7 @@ type ExchangerOption func(*exchangerConfig) type exchangerConfig struct { policy surfguard.Policy policySet bool + timeout time.Duration } // WithExchangerPolicy replaces [DefaultIssuerPolicy] for the token endpoint @@ -49,6 +53,17 @@ func WithExchangerPolicy(p surfguard.Policy) ExchangerOption { return func(c *exchangerConfig) { c.policy, c.policySet = p, true } } +// WithExchangerTimeout bounds each token-endpoint request at d instead of the +// 30-second default — the same per-request budget as the device flow's +// WithDeviceTimeout, with the same shared 3600 s ceiling and the same +// normalize-at-entry rule: a non-positive or beyond-ceiling value falls back +// to the default. The bound is a child context deadline, not a client +// mutation, so it holds on an injected client's requests too; a caller +// context with a sooner deadline still wins. +func WithExchangerTimeout(d time.Duration) ExchangerOption { + return func(c *exchangerConfig) { c.timeout = d } +} + // NewExchanger creates an Exchanger. // // A nil httpClient selects the policy-enforced default: the shared @@ -61,13 +76,23 @@ func WithExchangerPolicy(p surfguard.Policy) ExchangerOption { // DefaultIssuerPolicy().RoundTripper() where that is possible. Passing // http.DefaultClient restores the pre-policy behavior outright. // +// Redirect suppression is not the address policy and rides every lane: the +// token endpoint's redirects are refused on an injected client too, via a +// per-request shallow copy that never mutates the caller's client — the same +// contract as the device flow's POSTs. +// // An Exchanger given WithExchangerPolicy owns a transport, and has no Close, so // build it once and reuse it rather than constructing one per exchange. func NewExchanger(httpClient *http.Client, opts ...ExchangerOption) *Exchanger { - cfg := exchangerConfig{} + cfg := exchangerConfig{timeout: defaultTokenRequestTimeout} for _, o := range opts { o(&cfg) } + // Non-positive AND oversized values both fall back to the default: the + // same normalize-at-entry discipline as newDeviceConfig. + if cfg.timeout <= 0 || cfg.timeout > maxDeviceRequestTimeout { + cfg.timeout = defaultTokenRequestTimeout + } switch { case httpClient != nil: case cfg.policySet: @@ -75,7 +100,7 @@ func NewExchanger(httpClient *http.Client, opts ...ExchangerOption) *Exchanger { default: httpClient = sharedPolicyClient() } - return &Exchanger{httpClient: httpClient} + return &Exchanger{httpClient: httpClient, timeout: cfg.timeout} } // Exchange exchanges an authorization code for access and refresh tokens. @@ -151,6 +176,26 @@ const maxTokenResponseBytes int64 = 1 * 1024 * 1024 // maxErrorMessageLen is the maximum length for error messages included in errors. const maxErrorMessageLen = 500 +// defaultTokenRequestTimeout bounds each token exchange/refresh round-trip — +// the 30 s every other credential POST already converged on (the device flow +// here, TS/Python/Ruby's exchange). The ceiling is the device flow's shared +// maxDeviceRequestTimeout. +const defaultTokenRequestTimeout = 30 * time.Second + +// isRedirectStatus reports whether status is one of the redirects the token +// endpoint refuses to follow (SPEC §16 "Token-Endpoint Transport Policy" — +// the same set as SPEC §14's download hop). 304 is not among them and stays +// on the generic non-200 path. The basecamp package keeps its own unexported +// copy for the download flow. +func isRedirectStatus(status int) bool { + switch status { + case http.StatusMovedPermanently, http.StatusFound, http.StatusSeeOther, + http.StatusTemporaryRedirect, http.StatusPermanentRedirect: + return true + } + return false +} + func (e *Exchanger) doTokenRequest(ctx context.Context, tokenEndpoint string, data url.Values) (*Token, error) { // Validate HTTPS to prevent sending tokens/credentials over plaintext // Allow localhost for testing against local mock OAuth servers @@ -158,7 +203,12 @@ func (e *Exchanger) doTokenRequest(ctx context.Context, tokenEndpoint string, da return nil, fmt.Errorf("token endpoint validation failed for %q: %w", tokenEndpoint, err) } - httpReq, err := http.NewRequestWithContext(ctx, "POST", tokenEndpoint, strings.NewReader(data.Encode())) + // A child deadline, not a client mutation, so it bounds the request on the + // injected-client lane too; a caller context with a sooner deadline wins. + reqCtx, cancel := context.WithTimeout(ctx, e.timeout) + defer cancel() + + httpReq, err := http.NewRequestWithContext(reqCtx, "POST", tokenEndpoint, strings.NewReader(data.Encode())) if err != nil { return nil, fmt.Errorf("creating token request: %w", err) } @@ -172,7 +222,13 @@ func (e *Exchanger) doTokenRequest(ctx context.Context, tokenEndpoint string, da // time (DefaultIssuerPolicy, #806). This request carries the authorization // code, the client secret, or a refresh token, so it is the highest-value // of the three such call sites. - resp, err := e.httpClient.Do(httpReq) // #nosec G704 -- see the note above: address-policed by default + // + // noRedirectClient rides every lane, injected clients included: redirect + // suppression is a transport invariant (SPEC §16 "Token-Endpoint Transport + // Policy"), not part of the address policy a caller's client opts out of. + // The shallow copy keeps the caller's (and the shared policy) client + // unmutated. + resp, err := noRedirectClient(e.httpClient).Do(httpReq) // #nosec G704 -- see the note above: address-policed by default if err != nil { // A policy refusal is a typed, permanent verdict on the endpoint; every // other failure keeps the untyped wrap callers already match on. @@ -183,6 +239,14 @@ func (e *Exchanger) doTokenRequest(ctx context.Context, tokenEndpoint string, da } defer func() { _ = resp.Body.Close() }() + // A refused redirect is a typed api fault classified by status BEFORE the + // body read: a 3xx that streams its body slowly (or never) must surface as + // this error now, not as a mid-read timeout. Every other non-200 keeps the + // body-informed handling below. + if isRedirectStatus(resp.StatusCode) { + return nil, basecamp.ErrAPI(resp.StatusCode, fmt.Sprintf("redirect %d on the token endpoint is not followed", resp.StatusCode)) + } + // Bounded read to prevent OOM from malicious/corrupted responses lr := io.LimitReader(resp.Body, maxTokenResponseBytes+1) body, err := io.ReadAll(lr) diff --git a/go/pkg/basecamp/oauth/oauth_test.go b/go/pkg/basecamp/oauth/oauth_test.go index 2d5df76c5..0d7c2f035 100644 --- a/go/pkg/basecamp/oauth/oauth_test.go +++ b/go/pkg/basecamp/oauth/oauth_test.go @@ -5,9 +5,11 @@ import ( "encoding/json" "errors" "fmt" + "math" "net/http" "net/http/httptest" "strings" + "sync/atomic" "testing" "time" @@ -678,3 +680,189 @@ func TestExchanger_TokenResponseResource(t *testing.T) { }) } } + +// tokenRedirectServers starts a token endpoint answering every POST with the +// given redirect status and a Location naming a second server, and returns the +// endpoint URL plus the Location target's hit counter. As in +// endpoint_policy_test.go, the counter is the load-bearing assertion — and the +// target serves a USABLE token, so suppression that silently broke would +// surface as a successful exchange against the wrong host, not a +// differently-worded failure. +func tokenRedirectServers(t *testing.T, status int) (string, *atomic.Int64) { + t.Helper() + 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":"tok","token_type":"Bearer","expires_in":3600}`)) + })) + t.Cleanup(target.Close) + endpoint := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Location", target.URL) + w.WriteHeader(status) + })) + t.Cleanup(endpoint.Close) + return endpoint.URL, &hits +} + +// assertTokenRedirectRefused checks the refusal's shape: typed api_error +// carrying the real status, the "not followed" message contract (SPEC §16 +// "Token-Endpoint Transport Policy"), and a Location host that was never +// dialed. +func assertTokenRedirectRefused(t *testing.T, err error, status int, hits *atomic.Int64) { + t.Helper() + var be *basecamp.Error + if !errors.As(err, &be) { + t.Fatalf("error = %v, want *basecamp.Error", err) + } + if be.Code != basecamp.CodeAPI { + t.Errorf("Code = %q, want %q", be.Code, basecamp.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) + } +} + +// TestExchanger_RefusesTokenEndpointRedirects pins the full refused set on +// both operations and both client lanes: the SDK-built policy client and an +// injected client (a plain *http.Client would otherwise follow — 301/302/303 +// as a GET, 307/308 re-POSTing the credentials). +func TestExchanger_RefusesTokenEndpointRedirects(t *testing.T) { + for _, status := range []int{301, 302, 303, 307, 308} { + t.Run(fmt.Sprintf("policy client %d", status), func(t *testing.T) { + endpoint, hits := tokenRedirectServers(t, status) + e := NewExchanger(nil, WithExchangerPolicy(DefaultIssuerPolicy().AllowLoopback())) + + _, err := e.Exchange(context.Background(), ExchangeRequest{ + TokenEndpoint: endpoint, Code: "code", RedirectURI: "http://localhost/cb", ClientID: "id", + }) + assertTokenRedirectRefused(t, err, status, hits) + + _, err = e.Refresh(context.Background(), RefreshRequest{TokenEndpoint: endpoint, RefreshToken: "refresh"}) + assertTokenRedirectRefused(t, err, status, hits) + }) + t.Run(fmt.Sprintf("injected client %d", status), func(t *testing.T) { + endpoint, hits := tokenRedirectServers(t, status) + e := NewExchanger(&http.Client{}) + + _, err := e.Exchange(context.Background(), ExchangeRequest{ + TokenEndpoint: endpoint, Code: "code", RedirectURI: "http://localhost/cb", ClientID: "id", + }) + assertTokenRedirectRefused(t, err, status, hits) + + _, err = e.Refresh(context.Background(), RefreshRequest{TokenEndpoint: endpoint, RefreshToken: "refresh"}) + assertTokenRedirectRefused(t, err, status, hits) + }) + } +} + +// TestExchanger_304StaysGenericNon200 pins the boundary of the refused set: a +// 304 is a cache validator, not a followable redirect, and keeps the untyped +// non-200 wrap. +func TestExchanger_304StaysGenericNon200(t *testing.T) { + endpoint := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotModified) + })) + t.Cleanup(endpoint.Close) + + _, err := NewExchanger(&http.Client{}).Refresh(context.Background(), + RefreshRequest{TokenEndpoint: endpoint.URL, RefreshToken: "refresh"}) + if err == nil { + t.Fatal("Refresh() error = nil, want the generic non-200 failure") + } + if strings.Contains(err.Error(), "not followed") { + t.Errorf("error = %v; 304 must not classify as a refused redirect", err) + } + if !strings.Contains(err.Error(), "status 304") { + t.Errorf("error = %v, want the generic status-304 wrap", err) + } +} + +// TestExchanger_StalledRedirectBodyClassifiedBeforeRead proves the refusal is +// status-first: a 302 whose body never completes classifies immediately +// instead of timing out mid-read. +func TestExchanger_StalledRedirectBodyClassifiedBeforeRead(t *testing.T) { + var hits atomic.Int64 + target := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + hits.Add(1) + })) + defer target.Close() + + release := make(chan struct{}) + endpoint := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Location", target.URL) + w.WriteHeader(http.StatusFound) + w.(http.Flusher).Flush() + <-release // the body never completes + })) + defer endpoint.Close() + defer close(release) // unblock the handler before Close waits on it + + start := time.Now() + _, err := NewExchanger(&http.Client{}).Refresh(context.Background(), + RefreshRequest{TokenEndpoint: endpoint.URL, RefreshToken: "refresh"}) + assertTokenRedirectRefused(t, err, http.StatusFound, &hits) + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Errorf("classification took %v, want status-first (before any body read)", elapsed) + } +} + +// TestNewExchanger_TimeoutClamp pins the normalize-at-entry rule shared with +// the device flow: non-positive and beyond-ceiling values fall back to the +// 30 s default; the ceiling itself and ordinary values are honored. +func TestNewExchanger_TimeoutClamp(t *testing.T) { + tests := []struct { + name string + opts []ExchangerOption + want time.Duration + }{ + {"default when unset", nil, defaultTokenRequestTimeout}, + {"zero clamps to default", []ExchangerOption{WithExchangerTimeout(0)}, defaultTokenRequestTimeout}, + {"negative clamps to default", []ExchangerOption{WithExchangerTimeout(-time.Second)}, defaultTokenRequestTimeout}, + {"beyond ceiling clamps to default", []ExchangerOption{WithExchangerTimeout(maxDeviceRequestTimeout + time.Second)}, defaultTokenRequestTimeout}, + {"MaxInt64 clamps to default", []ExchangerOption{WithExchangerTimeout(time.Duration(math.MaxInt64))}, defaultTokenRequestTimeout}, + {"ceiling accepted", []ExchangerOption{WithExchangerTimeout(maxDeviceRequestTimeout)}, maxDeviceRequestTimeout}, + {"ordinary value accepted", []ExchangerOption{WithExchangerTimeout(5 * time.Second)}, 5 * time.Second}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := NewExchanger(&http.Client{}, tt.opts...).timeout; got != tt.want { + t.Errorf("timeout = %v, want %v", got, tt.want) + } + }) + } +} + +// TestExchanger_RequestTimeout proves the bound is live on an injected client +// with a context carrying no deadline of its own — the lane that was +// previously unbounded. +func TestExchanger_RequestTimeout(t *testing.T) { + release := make(chan struct{}) + endpoint := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + select { + case <-r.Context().Done(): + case <-release: // the response never arrives + } + })) + defer endpoint.Close() + defer close(release) // unblock the handler before Close waits on it + + e := NewExchanger(&http.Client{}, WithExchangerTimeout(100*time.Millisecond)) + start := time.Now() + _, err := e.Refresh(context.Background(), RefreshRequest{TokenEndpoint: endpoint.URL, RefreshToken: "refresh"}) + if err == nil { + t.Fatal("Refresh() error = nil, want the timeout failure") + } + if !strings.Contains(err.Error(), "token request failed") { + t.Errorf("error = %v, want the untyped transport wrap", err) + } + if elapsed := time.Since(start); elapsed > 3*time.Second { + t.Errorf("request ran %v, want it bounded near the 100ms deadline", elapsed) + } +} diff --git a/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/oauth/Exchange.kt b/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/oauth/Exchange.kt index ce0413e23..7f64bea1f 100644 --- a/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/oauth/Exchange.kt +++ b/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/oauth/Exchange.kt @@ -1,6 +1,7 @@ package com.basecamp.sdk.oauth import io.ktor.client.* +import io.ktor.client.plugins.* import io.ktor.client.request.* import io.ktor.client.request.forms.* import io.ktor.client.statement.* @@ -51,6 +52,47 @@ internal data class OAuthErrorResponse( private val tokenJson = Json { ignoreUnknownKeys = true } private const val MAX_RESPONSE_SIZE = 1_048_576L // 1 MB +/** Bounded per-request timeout for every token-endpoint POST — the 30 s credential-POST default shared across the SDKs (SPEC §16). */ +private const val TOKEN_REQUEST_TIMEOUT_MS = 30_000L + +/** + * The redirects the token endpoint refuses outright (SPEC §16 "Token-Endpoint + * Transport Policy") — the same set the signed download hop refuses (#809). + * 304 is deliberately absent: it is a cache validator with no `Location`, and + * falls through to the generic non-success branch below. + */ +private val REDIRECT_STATUSES = setOf(301, 302, 303, 307, 308) + +/** + * Builds a hardened HTTP client for token-endpoint POSTs: redirects suppressed + * ([HttpClient.followRedirects] = false, so a 3xx is classified below rather + * than any engine chasing an attacker-influenced `Location` with the + * credentials re-POSTed) and a bounded per-request timeout ([HttpTimeout]) so + * a stalled token request cannot hang an exchange or refresh unbounded. + * + * When [baseClient] is supplied its engine is reused but wrapped so the + * hardening applies regardless — redirect suppression is a security + * invariant, not a default (the device flow does the same). The returned + * wrapper is always closed by the caller and, because Ktor only closes + * engines it created, the borrowed engine survives. + */ +private fun hardenedTokenClient(baseClient: HttpClient?): HttpClient { + val engine = baseClient?.engine + return if (engine != null) { + HttpClient(engine) { + followRedirects = false + expectSuccess = false + install(HttpTimeout) { requestTimeoutMillis = TOKEN_REQUEST_TIMEOUT_MS } + } + } else { + HttpClient { + followRedirects = false + expectSuccess = false + install(HttpTimeout) { requestTimeoutMillis = TOKEN_REQUEST_TIMEOUT_MS } + } + } +} + /** * Exchanges an authorization code for tokens. * @@ -162,20 +204,33 @@ private suspend fun postTokenRequest( // Never POST credentials over cleartext (localhost exempt for dev/test). requireSecureEndpoint(endpoint, "token endpoint") - val httpClient = client ?: HttpClient() - val shouldClose = client == null + val httpClient = hardenedTokenClient(client) try { val response = httpClient.submitForm(endpoint, params) { accept(ContentType.Application.Json) } + val status = response.status.value + + // Status-first: a refused redirect is classified BEFORE any body read, + // so a 3xx that drip-feeds its body cannot degrade into a timeout. The + // hardened client never follows, and the `Location` is never dialled — + // the endpoint the caller (or discovered metadata) named is the one + // destination these credentials go to (SPEC §16). + if (status in REDIRECT_STATUSES) { + throw BasecampException.Api( + "redirect $status on the token endpoint is not followed", + httpStatus = status, + ) + } + val body = response.bodyAsText() if (body.length > MAX_RESPONSE_SIZE) { throw BasecampException.Api( "OAuth token response exceeds size limit", - httpStatus = response.status.value, + httpStatus = status, ) } @@ -183,7 +238,7 @@ private suspend fun postTokenRequest( val errorResp = runCatching { tokenJson.decodeFromString(body) }.getOrNull() val message = errorResp?.errorDescription ?: errorResp?.error - ?: "Token request failed: HTTP ${response.status.value}" + ?: "Token request failed: HTTP $status" throw BasecampException.Auth( message = BasecampException.truncateMessage(message), ) @@ -194,7 +249,7 @@ private suspend fun postTokenRequest( // messages — map to a status-only fault (no cause: cause messages // surface in stack traces) instead of propagating it. val raw = runCatching { tokenJson.decodeFromString(body) }.getOrElse { - throw BasecampException.Api("Failed to parse token response", httpStatus = response.status.value) + throw BasecampException.Api("Failed to parse token response", httpStatus = status) } // resource: absent and JSON null decode to null (unset); when present // it must be non-empty (SPEC §16) — an empty binding is not a binding. @@ -202,7 +257,7 @@ private suspend fun postTokenRequest( if (raw.resource != null && raw.resource.isEmpty()) { throw BasecampException.Api( "Token response resource must be a non-empty string when present", - httpStatus = response.status.value, + httpStatus = status, ) } // A 2xx with an EMPTY access_token is malformed, not a success — @@ -210,7 +265,7 @@ private suspend fun postTokenRequest( if (raw.accessToken.isEmpty()) { throw BasecampException.Api( "Token response missing access_token", - httpStatus = response.status.value, + httpStatus = status, ) } @@ -223,7 +278,7 @@ private suspend fun postTokenRequest( if (it.isEmpty()) { throw BasecampException.Api( "Token response token_type must be a non-empty string when present", - httpStatus = response.status.value, + httpStatus = status, ) } } ?: "Bearer" @@ -237,7 +292,15 @@ private suspend fun postTokenRequest( scope = raw.scope, resource = raw.resource, ) + } catch (e: HttpRequestTimeoutException) { + // The wrapper's HttpTimeout fired. Mapped explicitly — it subclasses + // CancellationException, so left alone it would masquerade as a + // cooperative cancellation — to the retryable network fault the other + // SDKs raise here ("Token request timed out", TS/Python/Ruby). + throw BasecampException.Network("Token request timed out", cause = e) } finally { - if (shouldClose) httpClient.close() + // Always ours: hardenedTokenClient built it, an injected client only + // lent its engine (which close() leaves running). + httpClient.close() } } diff --git a/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/OAuthTest.kt b/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/OAuthTest.kt index 3f982e006..579d46172 100644 --- a/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/OAuthTest.kt +++ b/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/OAuthTest.kt @@ -3,6 +3,7 @@ package com.basecamp.sdk import com.basecamp.sdk.oauth.* import io.ktor.client.* import io.ktor.client.engine.mock.* +import io.ktor.client.plugins.HttpRequestTimeoutException import io.ktor.http.* import kotlinx.coroutines.test.runTest import java.security.MessageDigest @@ -522,4 +523,134 @@ class OAuthTest { httpClient.close() } + + // ========================================================================= + // Token-endpoint transport policy (SPEC §16): redirects refused, timeouts + // mapped. Every test here injects its client, so the engine re-wrap path + // (hardening applies to caller-supplied clients too) is what is exercised. + // ========================================================================= + + private val redirectStatuses = listOf( + HttpStatusCode.MovedPermanently, // 301 + HttpStatusCode.Found, // 302 + HttpStatusCode.SeeOther, // 303 + HttpStatusCode.TemporaryRedirect, // 307 + HttpStatusCode.PermanentRedirect, // 308 + ) + + @Test + fun exchangeRefusesEveryRedirectStatus() = runTest { + for (redirect in redirectStatuses) { + val engine = MockEngine { + respond( + content = """{"access_token": "planted-by-attacker"}""", + status = redirect, + headers = headersOf(HttpHeaders.Location, "https://attacker.example/token"), + ) + } + val httpClient = HttpClient(engine) + try { + val e = assertFailsWith { + exchangeCode( + tokenEndpoint = "https://launchpad.37signals.com/authorization/token", + code = "c", + redirectUri = "https://myapp.com/callback", + clientId = "id", + clientSecret = "s", + client = httpClient, + ) + } + assertEquals(redirect.value, e.httpStatus, "the real status must survive classification") + assertTrue(e.message!!.contains("not followed"), "message contract, got: ${e.message}") + // Exactly one request: the Location target is never dialled. + assertEquals(1, engine.requestHistory.size) + } finally { + httpClient.close() + } + } + } + + @Test + fun refreshRefusesEveryRedirectStatus() = runTest { + for (redirect in redirectStatuses) { + val engine = MockEngine { + respond( + content = """{"access_token": "planted-by-attacker"}""", + status = redirect, + headers = headersOf(HttpHeaders.Location, "https://attacker.example/token"), + ) + } + val httpClient = HttpClient(engine) + try { + val e = assertFailsWith { + refreshToken( + tokenEndpoint = "https://launchpad.37signals.com/authorization/token", + refreshToken = "refresh-456", + clientId = "basecamp-cli", + client = httpClient, + ) + } + assertEquals(redirect.value, e.httpStatus, "the real status must survive classification") + assertTrue(e.message!!.contains("not followed"), "message contract, got: ${e.message}") + assertEquals(1, engine.requestHistory.size) + } finally { + httpClient.close() + } + } + } + + @Test + fun exchange304StaysOnTheGenericBranch() = runTest { + // 304 is a cache validator, not a Location redirect: it takes the + // generic non-success classification, never the refused-redirect one. + val engine = MockEngine { + respond(content = "", status = HttpStatusCode.NotModified) + } + val httpClient = HttpClient(engine) + try { + val e = assertFailsWith { + exchangeCode( + tokenEndpoint = "https://launchpad.37signals.com/authorization/token", + code = "c", + redirectUri = "https://myapp.com/callback", + clientId = "id", + clientSecret = "s", + client = httpClient, + ) + } + assertTrue(e.message!!.contains("HTTP 304")) + assertFalse(e.message!!.contains("not followed")) + } finally { + httpClient.close() + } + } + + @Test + fun exchangeMapsRequestTimeoutToRetryableNetwork() = runTest { + // HttpRequestTimeoutException subclasses CancellationException, so an + // unmapped one would masquerade as a cooperative cancellation. Raised + // from the handler because the wrapper's HttpTimeout timer runs on a + // real dispatcher runTest's virtual clock cannot advance. + val engine = MockEngine { + throw HttpRequestTimeoutException( + "https://launchpad.37signals.com/authorization/token", + 30_000L, + ) + } + val httpClient = HttpClient(engine) + try { + val e = assertFailsWith { + refreshToken( + tokenEndpoint = "https://launchpad.37signals.com/authorization/token", + refreshToken = "refresh-456", + clientId = "basecamp-cli", + client = httpClient, + ) + } + assertTrue(e.message!!.contains("timed out")) + assertTrue(e.retryable) + } finally { + httpClient.close() + } + } } diff --git a/python/src/basecamp/oauth/exchange.py b/python/src/basecamp/oauth/exchange.py index bc0ba3477..c65d24363 100644 --- a/python/src/basecamp/oauth/exchange.py +++ b/python/src/basecamp/oauth/exchange.py @@ -9,6 +9,12 @@ _TOKEN_TIMEOUT = 30.0 +# The redirects a token endpoint is refused (SPEC §16 "Token-Endpoint +# Transport Policy") — same set as the signed download hop (SPEC §14). 304 is +# not in the set: it is a cache validator, not a redirect-with-Location, and +# falls through to the generic non-2xx handling. +_REDIRECT_STATUSES = frozenset({301, 302, 303, 307, 308}) + def exchange_code( token_endpoint: str, @@ -107,7 +113,16 @@ def _token_request(token_endpoint: str, params: dict[str, str]) -> OAuthToken: require_https(token_endpoint, "token endpoint") try: - response = httpx.post( + # httpx.stream, not httpx.post: the status is classified from the + # headers BEFORE the body is consumed, so a refused redirect whose + # body stalls forever fails as the typed api_error below instead of + # degrading into a timeout. follow_redirects=False is httpx's + # default, but it is a load-bearing SSRF control here (SPEC §16 + # "Token-Endpoint Transport Policy"), not library happenstance — + # state it. The body read stays inside the try so a mid-read + # timeout or transport fault maps through the same handlers. + with httpx.stream( + "POST", token_endpoint, data=params, headers={ @@ -115,7 +130,17 @@ def _token_request(token_endpoint: str, params: dict[str, str]) -> OAuthToken: "Accept": "application/json", }, timeout=_TOKEN_TIMEOUT, - ) + follow_redirects=False, + ) as response: + # A redirect is never a valid token-endpoint outcome and its + # Location is never dialled — refuse it with the body unread. + if response.status_code in _REDIRECT_STATUSES: + raise OAuthError( + "api_error", + f"redirect {response.status_code} on the token endpoint is not followed", + http_status=response.status_code, + ) + response.read() except httpx.TimeoutException as exc: raise OAuthError("network", "Token request timed out", retryable=True) from exc except httpx.HTTPError as exc: diff --git a/python/tests/oauth/test_exchange.py b/python/tests/oauth/test_exchange.py index 125b1d56a..161392099 100644 --- a/python/tests/oauth/test_exchange.py +++ b/python/tests/oauth/test_exchange.py @@ -232,3 +232,81 @@ def test_token_response_malformed_resource_rejected(self, resource): assert exc_info.value.code == "api_error" assert "resource" in str(exc_info.value) + + +REDIRECT_STATUSES = (301, 302, 303, 307, 308) + +ATTACKER_LOCATION = "https://attacker.example/steal" + + +def _exchange(): + return exchange_code( + TOKEN_ENDPOINT, + code="auth-code-123", + redirect_uri="https://myapp.com/callback", + client_id="client-id", + ) + + +def _refresh(): + return refresh_token(TOKEN_ENDPOINT, refresh_tok="refresh-tok-123") + + +class TestRedirectRefusal: + """SPEC §16 "Token-Endpoint Transport Policy": a redirect is refused by + status with its body unread, and its Location is never dialled.""" + + @respx.mock + @pytest.mark.parametrize("call", [_exchange, _refresh], ids=["exchange", "refresh"]) + @pytest.mark.parametrize("status", REDIRECT_STATUSES) + def test_redirects_are_refused_and_never_followed(self, status, call): + route = respx.post(TOKEN_ENDPOINT).mock( + return_value=httpx.Response(status, headers={"Location": ATTACKER_LOCATION}) + ) + # A usable token waits at the Location — following it would "succeed", + # which is exactly the mutation this test exists to catch. + attacker = respx.route(host="attacker.example").mock(return_value=httpx.Response(200, json=TOKEN_RESPONSE)) + + with pytest.raises(OAuthError) as exc_info: + call() + + assert exc_info.value.oauth_type == "api_error" + assert exc_info.value.http_status == status + assert "not followed" in str(exc_info.value) + assert route.call_count == 1 + assert attacker.call_count == 0 + + @respx.mock + def test_304_is_generic_not_a_refused_redirect(self): + # 304 is a cache validator, not a redirect-with-Location — it keeps + # the generic malformed-response classification. + respx.post(TOKEN_ENDPOINT).mock(return_value=httpx.Response(304)) + + with pytest.raises(OAuthError) as exc_info: + _exchange() + + assert exc_info.value.oauth_type == "api_error" + assert exc_info.value.http_status == 304 + assert "not followed" not in str(exc_info.value) + + @respx.mock + def test_redirect_classified_from_headers_before_any_body_read(self): + # A refused redirect whose body never completes must classify from + # the headers, not time out mid-read: the stream raises if iterated. + class ExplodingStream(httpx.SyncByteStream): + def __iter__(self): + raise AssertionError("a refused redirect's body must never be read") + + respx.post(TOKEN_ENDPOINT).mock( + return_value=httpx.Response( + 302, + headers={"Location": ATTACKER_LOCATION}, + stream=ExplodingStream(), + ) + ) + + with pytest.raises(OAuthError) as exc_info: + _exchange() + + assert exc_info.value.http_status == 302 + assert "not followed" in str(exc_info.value) diff --git a/ruby/lib/basecamp/oauth/exchange.rb b/ruby/lib/basecamp/oauth/exchange.rb index b04ecce4f..0f93fd45f 100644 --- a/ruby/lib/basecamp/oauth/exchange.rb +++ b/ruby/lib/basecamp/oauth/exchange.rb @@ -2,16 +2,53 @@ require "faraday" require "json" +require "timeout" require "uri" module Basecamp module Oauth # Handles OAuth 2 token exchange and refresh operations. + # + # Both operations POST credentials — the authorization code and client + # secret, or the refresh token — to a token endpoint the caller names, + # which may be one that discovery's metadata chose. The POST therefore + # rides the same hardened transport discipline as the device flow + # (SPEC §16 "Token-Endpoint Transport Policy"): redirects are refused + # rather than followed, the whole request is wall-clock bounded, and the + # body reads under the shared streaming cap. class Exchange - # @param http_client [Faraday::Connection, nil] HTTP client (uses default if nil) - # @param timeout [Integer] Request timeout in seconds (default: 30) - def initialize(http_client: nil, timeout: 30) - @http_client = http_client || build_default_client(timeout) + # The redirect statuses a token endpoint response is refused for + # (SPEC §16 "Token-Endpoint Transport Policy") — the same set the signed + # download hop refuses (SPEC §14). 304 is deliberately absent: it is a + # cache validator, not a redirect-with-Location, and stays on the + # generic non-success path. + REDIRECT_STATUSES = [ 301, 302, 303, 307, 308 ].freeze + + # Default per-request timeout in seconds — the shared credential-POST + # default every SDK's token and device POSTs converge on (SPEC §16). + DEFAULT_TIMEOUT = 30 + + # Cap on a token response body (1 MiB), matching the device flow's and + # the other SDKs' token-response bound. + MAX_BODY_BYTES = 1 * 1024 * 1024 + + # @param http_client [Faraday::Connection, nil] HTTP client. Nil selects + # the headers-first default transport ({Fetcher.stream_http}); an + # injected connection is refused unless its stack is verifiably + # redirect-free (adapter-only), and keeps the injected-client fidelity + # tier: status classification only after the (bounded) read completes, + # deadline enforced wall-clock around the call. + # @param timeout [Numeric] Request timeout in seconds (default: 30). + # Invalid values and values beyond the shared 3600 s ceiling fall back + # to the default rather than disabling the bound. + def initialize(http_client: nil, timeout: DEFAULT_TIMEOUT) + # An injected connection is the caller's transport, but redirect + # suppression is not negotiable on a credential POST: refuse a stack + # that could follow (or rewrite) before any request is issued — the + # same guard discovery, resource, and the device flow apply. + Fetcher.ensure_redirects_suppressed!(http_client) if http_client + @http_client = http_client + @timeout = Fetcher.normalize_timeout(timeout, default: DEFAULT_TIMEOUT) end # Exchanges an authorization code for access and refresh tokens. @@ -80,14 +117,6 @@ def refresh(request) private - def build_default_client(timeout) - Faraday.new do |conn| - conn.options.timeout = timeout - conn.options.open_timeout = timeout - conn.adapter Faraday.default_adapter - end - end - def validate_exchange_request!(request) raise OauthError.new("validation", "Token endpoint is required") if request.token_endpoint.to_s.empty? raise OauthError.new("validation", "Authorization code is required") if request.code.to_s.empty? @@ -145,30 +174,128 @@ def build_refresh_params(request) def do_token_request(token_endpoint, params) Basecamp::Security.require_https_unless_localhost!(token_endpoint, "token endpoint") - response = @http_client.post(token_endpoint) do |req| - req.headers["Content-Type"] = "application/x-www-form-urlencoded" - req.headers["Accept"] = "application/json" - req.body = URI.encode_www_form(params) + status, body = post_form( + token_endpoint, params, + skip_status: ->(s) { REDIRECT_STATUSES.include?(s) } + ) + + # A refused redirect is a typed verdict classified by status alone — + # its body (skipped above) is never a token, and the credential POST + # is never re-issued toward Location (SPEC §16). + if REDIRECT_STATUSES.include?(status) + raise OauthError.new( + "api_error", + "redirect #{status} on the token endpoint is not followed", + http_status: status + ) end - parse_token_response(response) + parse_token_response(status, body) rescue Faraday::TimeoutError raise OauthError.new("network", "Token request timed out", retryable: true) rescue Faraday::Error => e raise OauthError.new("network", "Token request failed: #{e.message}", retryable: true) end - def parse_token_response(response) - Basecamp::Security.check_body_size!(response.body, Basecamp::Security::MAX_ERROR_BODY_BYTES, "Token") + # POSTs the token form and returns +[status, body]+, reading under the + # same bounded/streaming cap as discovery and the device flow. + # + # With no injected client the POST runs on the headers-first + # {Fetcher.stream_http} primitive: +skip_status+ classifies a redirect + # by status at HEADER time (its body is never read, even one that + # stalls forever), redirects are structurally never followed, and a + # watchdog bounds the whole request — a stalled or byte-dripped header + # phase included — at the timeout. An INJECTED Faraday connection keeps + # the Faraday path below. + def post_form(url, params, skip_status:) + if @http_client.nil? + Fetcher.stream_http( + :post, url, + headers: { "Content-Type" => "application/x-www-form-urlencoded", "Accept" => "application/json" }, + form: params, timeout: @timeout, max_body_bytes: MAX_BODY_BYTES, skip_status: skip_status + ) + else + post_form_injected(url, params, skip_status) + end + rescue Fetcher::SkipBody => e + # The body was intentionally not drained (a redirect's body is never a + # token) — classify by status upstream. + [ e.status, "" ] + rescue Fetcher::BodyTooLarge + raise OauthError.new("api_error", "Token response exceeds size cap") + rescue Fetcher::ReadDeadlineExceeded + # A slow-drip read is a transport timeout, not an api_error — surface + # as the Faraday timeout the caller's rescue classifies. + raise Faraday::TimeoutError, "Token request read exceeded the timeout deadline" + end + + # Injected-client (Faraday) lane — the injected-client fidelity tier + # (SPEC §16): the same invariants as the default transport (suppressed + # redirects, bounded body, whole-request wall clock), with buffered + # classification. +req.options.timeout+ below bounds only each socket + # read and resets on every +on_data+ chunk, so a slow-drip peer could + # otherwise hold the credential POST open past the timeout while + # staying under the cap — the monotonic deadline bounds the WHOLE + # request, and +Timeout.timeout+ enforces it through a stalled or + # dripped HEADER phase, where +on_data+ (a body callback) never runs. + def post_form_injected(url, params, skip_status) + deadline = Fetcher.monotonic_now + @timeout + chunks, on_data = Fetcher.bounded_reader(MAX_BODY_BYTES, deadline: deadline, skip_status: skip_status) + # The window is the REMAINING budget, not a fresh timeout: time spent + # before dispatch already counts against the deadline, so the request + # can never run past it. + remaining = deadline - Fetcher.monotonic_now + raise Faraday::TimeoutError, "request budget exhausted before dispatch" if remaining <= 0 + + response = Timeout.timeout(remaining, Faraday::TimeoutError) do + @http_client.post(url) do |req| + req.headers["Content-Type"] = "application/x-www-form-urlencoded" + req.headers["Accept"] = "application/json" + req.body = URI.encode_www_form(params) + req.options.timeout = @timeout + req.options.open_timeout = @timeout + req.options.on_data = on_data + end + end + + # Status-first backstop on the completed response: the +on_data+ + # SkipBody fast-path only fires when the adapter streams AND passes + # +env+ (Faraday >= 2.5). A buffered adapter that ignores +on_data+, + # an older Faraday (2.0–2.4) that omits +env+, or a header-only + # response reaches here with the redirect body un-skipped — re-apply + # +skip_status+ to the final status so a redirect is classified by + # status for every client shape, never buffered into a size-cap error. + # A definitive completed status outranks the deadline re-check below; + # everything else completing past the deadline is refused as the same + # transport-shaped timeout (Timeout.timeout's interrupt can land late). + if skip_status.call(response.status) + [ response.status, "" ] + elsif Fetcher.monotonic_now > deadline + raise Faraday::TimeoutError, "response completed after the deadline" + else + body = + if chunks.empty? + raw = response.body.to_s + raise Fetcher::BodyTooLarge if raw.bytesize > MAX_BODY_BYTES + + raw + else + chunks.join + end + + [ response.status, body.dup.force_encoding(Encoding::UTF_8) ] + end + end - data = JSON.parse(response.body) + def parse_token_response(status, body) + data = JSON.parse(body) - handle_error_response(response.status, data) unless response.success? + handle_error_response(status, data) unless (200..299).cover?(status) unless data["access_token"].is_a?(String) && !data["access_token"].empty? raise OauthError.new( "api_error", "Token response missing or non-string access_token", - http_status: response.status + http_status: status ) end @@ -179,7 +306,7 @@ def parse_token_response(response) raise OauthError.new( "api_error", "Token response resource must be a non-empty string when present", - http_status: response.status + http_status: status ) end @@ -191,7 +318,7 @@ def parse_token_response(response) raise OauthError.new( "api_error", "Token response token_type must be a non-empty string when present", - http_status: response.status + http_status: status ) end @@ -213,7 +340,7 @@ def parse_token_response(response) raise OauthError.new( "api_error", "Failed to parse token response", - http_status: response.status + http_status: status ), cause: nil end diff --git a/ruby/lib/basecamp/oauth/fetcher.rb b/ruby/lib/basecamp/oauth/fetcher.rb index c484dcb59..530e976cd 100644 --- a/ruby/lib/basecamp/oauth/fetcher.rb +++ b/ruby/lib/basecamp/oauth/fetcher.rb @@ -239,7 +239,7 @@ def self.ensure_redirects_suppressed!(client) raise OauthError.new( "validation", - "Injected OAuth discovery client must carry only an adapter (no middleware); " \ + "Injected OAuth client must carry only an adapter (no middleware); " \ "found #{offending.klass.name}. Redirects are suppressed for SSRF safety, so a " \ "connection whose middleware stack cannot be verified redirect-free is refused" ) diff --git a/ruby/lib/basecamp/oauth_token_provider.rb b/ruby/lib/basecamp/oauth_token_provider.rb index 31e369b83..39d8a5017 100644 --- a/ruby/lib/basecamp/oauth_token_provider.rb +++ b/ruby/lib/basecamp/oauth_token_provider.rb @@ -16,6 +16,22 @@ class OauthTokenProvider # Token endpoint for Basecamp OAuth TOKEN_URL = "https://launchpad.37signals.com/authorization/token" + # The redirect statuses a token endpoint response is refused for + # (SPEC §16 "Token-Endpoint Transport Policy") — the refresh POST carries + # the refresh token and client secret, and a redirect must surface as a + # typed fault rather than re-issue those credentials toward Location. + # 304 stays on the generic non-success path (a cache validator, not a + # redirect-with-Location). + REDIRECT_STATUSES = [ 301, 302, 303, 307, 308 ].freeze + + # Whole-request bound in seconds for the refresh POST — the shared + # credential-POST default (SPEC §16). Enforced as socket timeouts AND a + # monotonic wall-clock deadline by the transport below. + REFRESH_TIMEOUT = 30 + + # Cap on a refresh response body (1 MiB), matching the exchange path. + MAX_RESPONSE_BYTES = 1 * 1024 * 1024 + # @return [String, nil] the current refresh token attr_reader :refresh_token @@ -77,30 +93,51 @@ def refresh_if_needed perform_refresh if expired? && refreshable? end + # The refresh POST runs on the headers-first {Oauth::Fetcher.stream_http} + # primitive — the same transport as the exchange and device paths — so it + # gets the full SPEC §16 discipline rather than a bare Faraday.post: + # redirects structurally never followed and classified at header time, + # socket timeouts plus a monotonic whole-request watchdog (a slow-drip + # peer cannot hold the refresh open past REFRESH_TIMEOUT), and a bounded + # streaming body read. def perform_refresh require "faraday" require "json" - require "uri" - - response = Faraday.post(TOKEN_URL) do |req| - req.headers["Content-Type"] = "application/x-www-form-urlencoded" - req.body = URI.encode_www_form( - type: "refresh", - refresh_token: @refresh_token, - client_id: @client_id, - client_secret: @client_secret - ) - end - raise AuthError.new("Token refresh failed: #{response.status}") unless response.success? + status, body = Oauth::Fetcher.stream_http( + :post, TOKEN_URL, + headers: { "Content-Type" => "application/x-www-form-urlencoded" }, + form: { + "type" => "refresh", + "refresh_token" => @refresh_token, + "client_id" => @client_id, + "client_secret" => @client_secret + }, + timeout: REFRESH_TIMEOUT, + max_body_bytes: MAX_RESPONSE_BYTES, + skip_status: ->(s) { REDIRECT_STATUSES.include?(s) } + ) + + # A refused redirect is a typed api fault carrying the real status — + # not the generic AuthError below, which would imply the credentials + # were judged and rejected when no such judgement happened. + if REDIRECT_STATUSES.include?(status) + raise ApiError.new("redirect #{status} on the token endpoint is not followed", http_status: status) + end + raise AuthError.new("Token refresh failed: #{status}") unless (200..299).cover?(status) - data = JSON.parse(response.body) + data = JSON.parse(body) @access_token = data["access_token"] @expires_at = Time.now + data["expires_in"].to_i if data["expires_in"] @on_refresh&.call(@access_token, @refresh_token, @expires_at) true + rescue Oauth::Fetcher::BodyTooLarge + raise ApiError.new("Token refresh response exceeds size cap") + rescue Oauth::Fetcher::ReadDeadlineExceeded => e + # A slow-drip read past the deadline is a transport timeout. + raise NetworkError.new("Token refresh network error", cause: e) rescue Faraday::Error => e raise NetworkError.new("Token refresh network error", cause: e) end diff --git a/ruby/test/basecamp/auth_test.rb b/ruby/test/basecamp/auth_test.rb index c30130ee1..ea2166e33 100644 --- a/ruby/test/basecamp/auth_test.rb +++ b/ruby/test/basecamp/auth_test.rb @@ -137,4 +137,77 @@ def test_refresh_failure provider.refresh end end + + def test_refresh_refuses_every_redirect_status_and_never_follows + # SPEC §16 "Token-Endpoint Transport Policy": the refresh POST carries the + # refresh token and client secret, so a redirect surfaces as a typed + # api fault with the real status — never a re-POST toward Location. + attacker_stub = stub_request(:post, "https://attacker.example.com/token") + .to_return(status: 200, body: { access_token: "stolen" }.to_json, + headers: { "Content-Type" => "application/json" }) + + Basecamp::OauthTokenProvider::REDIRECT_STATUSES.each do |status| + stub_request(:post, "https://launchpad.37signals.com/authorization/token") + .to_return(status: status, headers: { "Location" => "https://attacker.example.com/token" }) + + provider = Basecamp::OauthTokenProvider.new( + access_token: "old-token", + refresh_token: "refresh-token", + client_id: "client-id", + client_secret: "client-secret" + ) + + error = assert_raises(Basecamp::ApiError, status.to_s) { provider.refresh } + assert_equal status, error.http_status, status.to_s + assert_match(/not followed/, error.message, status.to_s) + assert_equal "old-token", provider.instance_variable_get(:@access_token), + "a refused redirect must not mutate the stored token" + end + assert_not_requested(attacker_stub) + end + + def test_refresh_runs_on_the_bounded_headers_first_transport + # perform_refresh must ride Fetcher.stream_http with the shared 30 s + # budget, the body cap, and the redirect skip set. The transport's own + # guarantees under that budget — the monotonic whole-request watchdog, the + # header-time classification, the streaming cap — are proven against live + # sockets in oauth_transport_test.rb; this pins that the provider actually + # dispatches through it (TOKEN_URL is a fixed constant, so a live + # stalled-server deadline test would need a seam the provider does not + # otherwise want). + captured = nil + + provider = Basecamp::OauthTokenProvider.new( + access_token: "old-token", + refresh_token: "refresh-token", + client_id: "client-id", + client_secret: "client-secret" + ) + + # Swap the module function and restore by re-delegating — the same idiom + # the transport tests use for IPSocket.getaddress (minitest/mock is not + # loadable under bundled_gems here). + original = Basecamp::Oauth::Fetcher.method(:stream_http) + Basecamp::Oauth::Fetcher.define_singleton_method(:stream_http) do |method, url, **kwargs| + captured = [ method, url, kwargs ] + [ 200, { access_token: "new-token", expires_in: 3600 }.to_json ] + end + begin + assert provider.refresh + ensure + Basecamp::Oauth::Fetcher.define_singleton_method(:stream_http) do |*args, **kwargs, &blk| + original.call(*args, **kwargs, &blk) + end + end + + method, url, kwargs = captured + assert_equal :post, method + assert_equal Basecamp::OauthTokenProvider::TOKEN_URL, url + assert_equal Basecamp::OauthTokenProvider::REFRESH_TIMEOUT, kwargs[:timeout] + assert_equal Basecamp::OauthTokenProvider::MAX_RESPONSE_BYTES, kwargs[:max_body_bytes] + assert Basecamp::OauthTokenProvider::REDIRECT_STATUSES.all? { |s| kwargs[:skip_status].call(s) }, + "every refused redirect status must be in the skip set" + assert_not kwargs[:skip_status].call(200), "success must not be skipped" + assert_equal "new-token", provider.instance_variable_get(:@access_token) + end end diff --git a/ruby/test/basecamp/oauth_ssrf_test.rb b/ruby/test/basecamp/oauth_ssrf_test.rb index 35781240c..74bdeb64b 100644 --- a/ruby/test/basecamp/oauth_ssrf_test.rb +++ b/ruby/test/basecamp/oauth_ssrf_test.rb @@ -324,6 +324,127 @@ def test_normalize_timeout_honors_operation_default Basecamp::Oauth::Fetcher.normalize_timeout(Basecamp::Oauth::Fetcher::MAX_REQUEST_TIMEOUT) end + def test_exchange_and_refresh_refuse_every_redirect_status_and_never_follow + endpoint = "https://issuer.redirect-test.example/oauth/token" + attacker = "https://attacker.example.com" + + entry_points = { + exchange: -> do + Basecamp::Oauth.exchange_code( + token_endpoint: endpoint, code: "c", + redirect_uri: "https://myapp.com/callback", client_id: "id" + ) + end, + refresh: -> do + Basecamp::Oauth.refresh_token( + token_endpoint: endpoint, refresh_token: "r", client_id: "id" + ) + end + } + + # The full refused set (SPEC §16 "Token-Endpoint Transport Policy"), on + # BOTH credential-POST entry points: typed api_error carrying the real + # status, the contractual "not followed" message, and the Location host + # never dialed — a usable token behind the redirect must stay unreachable. + Basecamp::Oauth::Exchange::REDIRECT_STATUSES.each do |status| + entry_points.each do |name, run| + stub_request(:post, endpoint) + .to_return(status: status, headers: { "Location" => "#{attacker}/token" }) + attacker_stub = stub_request(:post, "#{attacker}/token") + .to_return(status: 200, body: { access_token: "stolen" }.to_json, + headers: { "Content-Type" => "application/json" }) + + error = assert_raises(Basecamp::Oauth::OauthError, "#{name} #{status}") { run.call } + assert_equal "api_error", error.type, "#{name} #{status}" + assert_equal status, error.http_status, "#{name} #{status}" + assert_match(/not followed/, error.message, "#{name} #{status}") + assert_not_requested(attacker_stub) + WebMock.reset! + end + end + end + + def test_exchange_304_stays_on_the_generic_non_success_path + # 304 is a cache validator, not a redirect-with-Location — it must classify + # through the generic non-success handling, not the redirect refusal. + endpoint = "https://issuer.redirect-test.example/oauth/token" + stub_request(:post, endpoint).to_return(status: 304, body: "") + + error = assert_raises(Basecamp::Oauth::OauthError) do + Basecamp::Oauth.exchange_code( + token_endpoint: endpoint, code: "c", + redirect_uri: "https://myapp.com/callback", client_id: "id" + ) + end + assert_equal "api_error", error.type + assert_equal 304, error.http_status + assert_no_match(/not followed/, error.message) + end + + def test_exchange_injected_client_carrying_redirect_middleware_is_rejected + connection = Faraday.new do |conn| + conn.use RedirectFollowingMiddleware + conn.adapter Faraday.default_adapter + end + + # The exchange applies the same redirect-suppression guard as discovery and + # the device flow: an injected client whose stack cannot be verified + # redirect-free is refused at construction, before any credential POST. + error = assert_raises(Basecamp::Oauth::OauthError) do + Basecamp::Oauth::Exchange.new(http_client: connection) + end + assert_equal "validation", error.type + end + + def test_exchange_injected_adapter_only_client_redirect_refused_by_backstop + # An adapter-only injected client passes the construction guard, and its + # (buffered) response classifies through the status-first backstop: the + # redirect surfaces as the typed refusal and Location is never dialed — + # the injected-client fidelity tier's coarser timing, same invariants. + endpoint = "https://issuer.redirect-test.example/oauth/token" + attacker = "https://attacker.example.com" + stub_request(:post, endpoint) + .to_return(status: 302, headers: { "Location" => "#{attacker}/token" }, body: "ignored") + attacker_stub = stub_request(:post, "#{attacker}/token") + .to_return(status: 200, body: { access_token: "stolen" }.to_json, + headers: { "Content-Type" => "application/json" }) + + connection = Faraday.new { |conn| conn.adapter Faraday.default_adapter } + exchange = Basecamp::Oauth::Exchange.new(http_client: connection) + error = assert_raises(Basecamp::Oauth::OauthError) do + exchange.exchange(Basecamp::Oauth::ExchangeRequest.new( + token_endpoint: endpoint, code: "c", + redirect_uri: "https://myapp.com/callback", client_id: "id" + )) + end + assert_equal "api_error", error.type + assert_equal 302, error.http_status + assert_match(/not followed/, error.message) + assert_not_requested(attacker_stub) + end + + def test_exchange_injected_slow_drip_aborts_on_wall_clock_deadline + # The injected lane's narrowed contract: buffered classification, but the + # SAME whole-request wall clock as the default transport — a slow-drip + # token body cannot hold the credential POST open past the timeout. + body = { "access_token" => "a", "pad" => "x" * 200 }.to_json + meter = { delivered: 0 } + connection = Faraday.new do |conn| + conn.adapter SlowDripAdapter, body: body, chunk_size: 1, pause: 0.02, meter: meter + end + + exchange = Basecamp::Oauth::Exchange.new(http_client: connection, timeout: 0.1) + error = assert_raises(Basecamp::Oauth::OauthError) do + exchange.exchange(Basecamp::Oauth::ExchangeRequest.new( + token_endpoint: "https://issuer.redirect-test.example/oauth/token", code: "c", + redirect_uri: "https://myapp.com/callback", client_id: "id" + )) + end + assert_equal "network", error.type + assert error.retryable, "wall-clock timeout must be retryable" + assert_operator meter[:delivered], :<, body.bytesize + end + def test_redirect_is_not_followed issuer = "https://issuer.redirect-test.example" attacker = "https://attacker.example.com" diff --git a/ruby/test/basecamp/oauth_test.rb b/ruby/test/basecamp/oauth_test.rb index 477ec8adf..abb0847e7 100644 --- a/ruby/test/basecamp/oauth_test.rb +++ b/ruby/test/basecamp/oauth_test.rb @@ -398,6 +398,23 @@ def test_token_response_malformed_resource_rejected end end + def test_exchange_timeout_normalization + # An invalid or beyond-ceiling timeout must fall back to the default + # rather than disable the socket timeouts and the wall-clock deadline — + # the same normalize-at-entry discipline as discovery and the device flow. + default = Basecamp::Oauth::Exchange::DEFAULT_TIMEOUT + [ nil, 0, -1, Float::INFINITY, Float::NAN, "30", 1e100 ].each do |bad| + exchange = Basecamp::Oauth::Exchange.new(timeout: bad) + assert_equal default, exchange.instance_variable_get(:@timeout), + "expected #{bad.inspect} to normalize to the default" + end + # Valid values are preserved, up to and including the shared ceiling. + assert_equal 5, Basecamp::Oauth::Exchange.new(timeout: 5).instance_variable_get(:@timeout) + assert_equal Basecamp::Oauth::Fetcher::MAX_REQUEST_TIMEOUT, + Basecamp::Oauth::Exchange.new(timeout: Basecamp::Oauth::Fetcher::MAX_REQUEST_TIMEOUT) \ + .instance_variable_get(:@timeout) + end + def test_token_expired token = Basecamp::Oauth::Token.new( access_token: "test", diff --git a/ruby/test/basecamp/oauth_transport_test.rb b/ruby/test/basecamp/oauth_transport_test.rb index bb0c715da..31b8df7e1 100644 --- a/ruby/test/basecamp/oauth_transport_test.rb +++ b/ruby/test/basecamp/oauth_transport_test.rb @@ -168,6 +168,34 @@ def test_token_poll_302_with_stalled_body_is_immediate_api_error_with_zero_retri assert_operator seconds, :<, TIMEOUT end + def test_exchange_302_with_stalled_body_is_immediate_api_error + # The token exchange rides the same headers-first transport: a redirect + # whose body never arrives classifies at HEADER time as the typed refusal + # (SPEC §16 "Token-Endpoint Transport Policy"), never as a body timeout, + # and the credential POST is issued exactly once. + endpoint, accepts = start_server do |conn| + conn.write("HTTP/1.1 302 Found\r\nLocation: https://attacker.example/\r\nContent-Length: 1000\r\n\r\n") + sleep 30 + end + + error = nil + seconds = elapsed do + error = assert_raises(Basecamp::Oauth::OauthError) do + Basecamp::Oauth.exchange_code( + token_endpoint: "#{endpoint}/token", code: "c", + redirect_uri: "https://myapp.com/callback", client_id: "id", + timeout: TIMEOUT + ) + end + end + + assert_equal "api_error", error.type + assert_equal 302, error.http_status + assert_match(/not followed/, error.message) + assert_equal 1, accepts.length, "a header-classified redirect must never be retried or followed" + assert_operator seconds, :<, TIMEOUT, "status must classify at header time, not after a body timeout" + end + def test_skipped_response_closes_the_connection_undrained # Releasing the connection matters as much as classifying it: the server # must observe the socket close (EPIPE/RST) instead of feeding a body to a diff --git a/ruby/test/basecamp/security_test.rb b/ruby/test/basecamp/security_test.rb index 128eb534c..f45ed9062 100644 --- a/ruby/test/basecamp/security_test.rb +++ b/ruby/test/basecamp/security_test.rb @@ -598,7 +598,10 @@ def test_oauth_response_body_size_limit stub_request(:post, "https://launchpad.37signals.com/authorization/token") .to_return(status: 200, body: huge_body, headers: { "Content-Type" => "application/json" }) - assert_raises(Basecamp::ApiError) do + # The exchange path now reads under the shared streaming cap and maps the + # violation to the OAuth taxonomy (api_error), matching discovery and the + # device flow — the old path leaked a raw Basecamp::ApiError instead. + error = assert_raises(Basecamp::Oauth::OauthError) do Basecamp::Oauth.exchange_code( token_endpoint: "https://launchpad.37signals.com/authorization/token", code: "auth-code", @@ -606,6 +609,7 @@ def test_oauth_response_body_size_limit client_id: "client-id" ) end + assert_equal "api_error", error.type end end diff --git a/typescript/src/oauth/device.ts b/typescript/src/oauth/device.ts index 87016fdb2..52abbcc02 100644 --- a/typescript/src/oauth/device.ts +++ b/typescript/src/oauth/device.ts @@ -10,7 +10,7 @@ import { BasecampError, truncateErrorMessage } from "../errors.js"; import { requireSecureEndpoint } from "../security.js"; import { readBodyBounded } from "./discovery.js"; import { DeviceFlowError } from "./device-errors.js"; -import { MAX_TOKEN_LIFETIME_SECONDS } from "./limits.js"; +import { MAX_TOKEN_LIFETIME_SECONDS, abortError, raceAbort, resolveRequestTimeoutMs } from "./limits.js"; // Re-exported for existing importers; the declaration lives in limits.ts so // non-device consumers need not pull this module. @@ -37,31 +37,14 @@ const MAX_DEVICE_SECONDS = 2_147_483; const DEFAULT_DEVICE_TIMEOUT_MS = 30_000; /** - * Ceiling (ms) for a caller-supplied per-request timeout: the shared 3600 s - * bound (Go's maxDeviceRequestTimeout, Python's _MAX_DEVICE_REQUEST_TIMEOUT, - * Ruby's Fetcher::MAX_REQUEST_TIMEOUT). A large finite value — up to the - * ~24.8-day MAX_DEVICE_SECONDS timer bound this previously allowed — would - * hold a stalled request open for weeks, defeating the bounded-request - * guarantee. - */ -const MAX_DEVICE_REQUEST_TIMEOUT_MS = 3600 * 1000; - -/** - * Coerce a caller-supplied request timeout (ms) to a finite, positive, timer-safe - * value no greater than the shared ceiling. `setTimeout` silently coerces a - * non-finite delay (NaN/Infinity) or one beyond its 32-bit range to ~1 ms — an - * immediate abort that would masquerade as a `DeviceFlowError("transport")` - * (and, in the poll loop, as repeated timeout backoffs). Fall back to the - * default instead, mirroring how the other SDKs normalize an invalid device - * timeout. + * Coerce a caller-supplied request timeout to the shared clamp + * (`resolveRequestTimeoutMs`, limits.ts), falling back to the device flow's + * own 30 s default — an invalid value would otherwise become an immediate + * abort masquerading as a `DeviceFlowError("transport")` (and, in the poll + * loop, as repeated timeout backoffs). */ function resolveDeviceTimeoutMs(timeoutMs: number): number { - if (!Number.isFinite(timeoutMs) || timeoutMs <= 0 || timeoutMs > MAX_DEVICE_REQUEST_TIMEOUT_MS) { - return DEFAULT_DEVICE_TIMEOUT_MS; - } - // Whole milliseconds, at least 1: timers truncate fractional delays toward - // 0, so 0.5 would become an immediate abort. - return Math.max(1, Math.floor(timeoutMs)); + return resolveRequestTimeoutMs(timeoutMs, DEFAULT_DEVICE_TIMEOUT_MS); } @@ -810,57 +793,8 @@ async function postDeviceToken( } } -// A plain Error tagged "AbortError" rather than `new DOMException(...)`: -// DOMException is not guaranteed in every JS runtime that can run this SDK -// (referencing it there throws ReferenceError). isAbort() matches on -// `name === "AbortError"`, so cancellation stays runtime-agnostic. -function abortError(): Error { - const err = new Error("Aborted"); - err.name = "AbortError"; - return err; -} - -/** - * Races `run` against `signal`: rejects with AbortError the moment the signal - * fires, even if the underlying promise NEVER settles. A cooperative fetch - * already rejects on abort — this enforces the same contract on a custom fetch - * that ignores its AbortSignal, so a late 200 cannot hand back a result and a - * never-settling fetch cannot hold the public call past its timeout. An - * already-aborted signal rejects without invoking `run`. A late settlement is - * discarded (settling an already-settled promise is a no-op), and its - * rejection path stays handled — no unhandled rejection escapes. - */ -function raceAbort(signal: AbortSignal, run: () => Promise): Promise { - return new Promise((resolve, reject) => { - const onAbort = () => reject(abortError()); - if (signal.aborted) { - onAbort(); - return; - } - signal.addEventListener("abort", onAbort, { once: true }); - // Microtask wrapper: a user-provided seam (custom fetch/sleepFn) can - // throw SYNCHRONOUSLY despite the TS type — without this, that throw - // would escape before the handlers attach and strand the listener. - Promise.resolve() - .then(() => { - // The abort can win between entry and this microtask (the outer - // promise has already rejected) — never invoke the seam post-abort; - // an AbortSignal-ignoring fetch would still send the POST. - if (signal.aborted) throw abortError(); - return run(); - }) - .then( - (value) => { - signal.removeEventListener("abort", onAbort); - resolve(value); - }, - (err) => { - signal.removeEventListener("abort", onAbort); - reject(err); - } - ); - }); -} +// abortError and raceAbort live in limits.ts (shared with the token-exchange +// path); the poll loop and sleep below use them unchanged. function sleep(ms: number, signal?: AbortSignal): Promise { return new Promise((resolve, reject) => { diff --git a/typescript/src/oauth/exchange.ts b/typescript/src/oauth/exchange.ts index 9d96b9417..7c51da7ba 100644 --- a/typescript/src/oauth/exchange.ts +++ b/typescript/src/oauth/exchange.ts @@ -5,7 +5,7 @@ * Supports both standard OAuth 2.0 and Basecamp's Launchpad legacy format. */ -import { MAX_TOKEN_LIFETIME_SECONDS } from "./limits.js"; +import { MAX_TOKEN_LIFETIME_SECONDS, raceAbort, resolveRequestTimeoutMs } from "./limits.js"; import { BasecampError } from "../errors.js"; import { isLocalhost } from "../security.js"; import type { @@ -16,6 +16,17 @@ import type { OAuthErrorResponse, } from "./types.js"; +/** Default per-request timeout (ms) for a token exchange/refresh round trip. */ +const DEFAULT_TOKEN_TIMEOUT_MS = 30_000; + +/** + * The redirects the token endpoint is refused (SPEC §16 "Token-Endpoint + * Transport Policy") — the same set the signed download hop refuses (§14). + * 304 is not in the set: it is a cache validator, not a redirect-with- + * Location, and falls through to the generic non-ok handling below. + */ +const REDIRECT_STATUSES = [301, 302, 303, 307, 308]; + /** * Options for token exchange/refresh operations. */ @@ -282,32 +293,62 @@ async function doTokenRequest( ): Promise { requireHTTPS(tokenEndpoint, "token endpoint"); - const { fetch: customFetch = globalThis.fetch, timeoutMs = 30000 } = options; + const { fetch: customFetch = globalThis.fetch, timeoutMs = DEFAULT_TOKEN_TIMEOUT_MS } = options; - // Create abort controller for timeout + // Create abort controller for timeout. The clamp closes the unbounded + // hole an unvalidated timeoutMs left open: NaN/Infinity made setTimeout + // fire at ~1 ms (an instant abort masquerading as a network failure), and + // an oversized finite value could hold a stalled request open for weeks. const controller = new AbortController(); - const timeoutId = setTimeout(() => controller.abort(), timeoutMs); + const timeoutId = setTimeout( + () => controller.abort(), + resolveRequestTimeoutMs(timeoutMs, DEFAULT_TOKEN_TIMEOUT_MS) + ); try { - const response = await customFetch(tokenEndpoint, { - method: "POST", - headers: { - "Content-Type": "application/x-www-form-urlencoded", - Accept: "application/json", - }, - body: body.toString(), - signal: controller.signal, - }); + // The whole round trip runs INSIDE a race against the controller's + // signal, like the device flow's POSTs: a cooperative fetch rejects on + // abort anyway, but a custom fetch that ignores its AbortSignal must not + // hold the exchange past its timeout — the race rejects the moment the + // timeout fires, and a late settlement is discarded. + const { response, responseText } = await raceAbort(controller.signal, async () => { + const raced = await customFetch(tokenEndpoint, { + method: "POST", + headers: { + "Content-Type": "application/x-www-form-urlencoded", + Accept: "application/json", + }, + body: body.toString(), + signal: controller.signal, + // Never chase an attacker-influenced Location: the token endpoint may + // come from discovered metadata, and a followed 307/308 would re-POST + // the credentials wherever it points (SPEC §16). + redirect: "manual", + }); - const MAX_TOKEN_RESPONSE_BYTES = 1 * 1024 * 1024; // 1 MB + // A suppressed redirect is refused by status BEFORE any body read, so a + // 3xx whose body stalls forever cannot degrade into a timeout. Release + // the unread stream (non-blocking) so refusals don't retain sockets. + if (REDIRECT_STATUSES.includes(raced.status)) { + void raced.body?.cancel().catch(() => {}); + throw new BasecampError( + "api_error", + `redirect ${raced.status} on the token endpoint is not followed`, + { httpStatus: raced.status } + ); + } - // Use streaming reader with true byte-level limit enforcement. - // This handles cases where Content-Length is absent or inaccurate. - const responseText = await readResponseWithByteLimit( - response, - MAX_TOKEN_RESPONSE_BYTES, - response.status - ); + const MAX_TOKEN_RESPONSE_BYTES = 1 * 1024 * 1024; // 1 MB + + // Use streaming reader with true byte-level limit enforcement. + // This handles cases where Content-Length is absent or inaccurate. + const racedText = await readResponseWithByteLimit( + raced, + MAX_TOKEN_RESPONSE_BYTES, + raced.status + ); + return { response: raced, responseText: racedText }; + }); let data: RawTokenResponse | OAuthErrorResponse; diff --git a/typescript/src/oauth/limits.ts b/typescript/src/oauth/limits.ts index 11eacec5f..052dee2d9 100644 --- a/typescript/src/oauth/limits.ts +++ b/typescript/src/oauth/limits.ts @@ -1,6 +1,9 @@ /** - * Shared OAuth limits (SPEC §16), split out so the token-exchange path can - * import them without pulling the whole device-flow module. + * Shared OAuth request primitives (SPEC §16), split out so the token-exchange + * path can import them without pulling the whole device-flow module: the + * token-lifetime ceiling, the per-request timeout resolver, and the abort + * race that bounds a round trip even when a custom fetch ignores its + * AbortSignal. */ /** @@ -13,3 +16,80 @@ * across all five SDKs. */ export const MAX_TOKEN_LIFETIME_SECONDS = 2_147_483_647; + +/** + * Ceiling (ms) for a caller-supplied per-request timeout: the shared 3600 s + * bound (Go's maxDeviceRequestTimeout, Python's _MAX_DEVICE_REQUEST_TIMEOUT, + * Ruby's Fetcher::MAX_REQUEST_TIMEOUT). A large finite value would hold a + * stalled request open for weeks, defeating the bounded-request guarantee. + */ +export const MAX_REQUEST_TIMEOUT_MS = 3600 * 1000; + +/** + * Coerce a caller-supplied request timeout (ms) to a finite, positive, + * timer-safe value no greater than the shared ceiling. `setTimeout` silently + * coerces a non-finite delay (NaN/Infinity) or one beyond its 32-bit range to + * ~1 ms — an immediate abort that would masquerade as a transport failure. + * Fall back to the operation's own default instead, mirroring how the other + * SDKs normalize an invalid OAuth request timeout. + */ +export function resolveRequestTimeoutMs(timeoutMs: number, defaultMs: number): number { + if (!Number.isFinite(timeoutMs) || timeoutMs <= 0 || timeoutMs > MAX_REQUEST_TIMEOUT_MS) { + return defaultMs; + } + // Whole milliseconds, at least 1: timers truncate fractional delays toward + // 0, so 0.5 would become an immediate abort. + return Math.max(1, Math.floor(timeoutMs)); +} + +// A plain Error tagged "AbortError" rather than `new DOMException(...)`: +// DOMException is not guaranteed in every JS runtime that can run this SDK +// (referencing it there throws ReferenceError). isAbort() matches on +// `name === "AbortError"`, so cancellation stays runtime-agnostic. +export function abortError(): Error { + const err = new Error("Aborted"); + err.name = "AbortError"; + return err; +} + +/** + * Races `run` against `signal`: rejects with AbortError the moment the signal + * fires, even if the underlying promise NEVER settles. A cooperative fetch + * already rejects on abort — this enforces the same contract on a custom fetch + * that ignores its AbortSignal, so a late 200 cannot hand back a result and a + * never-settling fetch cannot hold the public call past its timeout. An + * already-aborted signal rejects without invoking `run`. A late settlement is + * discarded (settling an already-settled promise is a no-op), and its + * rejection path stays handled — no unhandled rejection escapes. + */ +export function raceAbort(signal: AbortSignal, run: () => Promise): Promise { + return new Promise((resolve, reject) => { + const onAbort = () => reject(abortError()); + if (signal.aborted) { + onAbort(); + return; + } + signal.addEventListener("abort", onAbort, { once: true }); + // Microtask wrapper: a user-provided seam (custom fetch/sleepFn) can + // throw SYNCHRONOUSLY despite the TS type — without this, that throw + // would escape before the handlers attach and strand the listener. + Promise.resolve() + .then(() => { + // The abort can win between entry and this microtask (the outer + // promise has already rejected) — never invoke the seam post-abort; + // an AbortSignal-ignoring fetch would still send the POST. + if (signal.aborted) throw abortError(); + return run(); + }) + .then( + (value) => { + signal.removeEventListener("abort", onAbort); + resolve(value); + }, + (err) => { + signal.removeEventListener("abort", onAbort); + reject(err); + } + ); + }); +} diff --git a/typescript/tests/oauth/oauth.test.ts b/typescript/tests/oauth/oauth.test.ts index aefca32b6..5dca299be 100644 --- a/typescript/tests/oauth/oauth.test.ts +++ b/typescript/tests/oauth/oauth.test.ts @@ -5,7 +5,7 @@ */ import { describe, it, expect } from "vitest"; -import { http, HttpResponse } from "msw"; +import { http, HttpResponse, delay } from "msw"; import { server } from "../setup.js"; import { discover, @@ -620,6 +620,104 @@ describe("Token Exchange", () => { }); }); +describe("Token-Endpoint Transport Policy", () => { + const tokenEndpoint = "https://launchpad.37signals.com/authorization/token"; + const attackerUrl = "https://attacker.example.com/steal"; + + const callExchange = () => + exchangeCode({ + tokenEndpoint, + code: "auth_code_123", + redirectUri: "https://myapp.com/callback", + clientId: "my_client_id", + }); + const callRefresh = () => + refreshToken({ + tokenEndpoint, + refreshToken: "my_refresh_token", + }); + + describe.each([ + ["exchangeCode", callExchange], + ["refreshToken", callRefresh], + ] as const)("%s", (_name, call) => { + it.each([301, 302, 303, 307, 308])( + "refuses a %d as api_error and never dials its Location", + async (status) => { + // SPEC §16 "Token-Endpoint Transport Policy": a redirect from the + // token endpoint surfaces with its status, and the Location it names + // is never dialled — a followed 307/308 would re-POST the credentials + // wherever it points. A usable token behind the Location proves the + // refusal is what stopped the chain, not a broken attacker handler. + let attackerHits = 0; + server.use( + http.post(tokenEndpoint, () => + new HttpResponse(null, { status, headers: { Location: attackerUrl } }) + ), + http.all(attackerUrl, () => { + attackerHits += 1; + return HttpResponse.json({ access_token: "stolen_token" }); + }) + ); + + await expect(call()).rejects.toMatchObject({ + code: "api_error", + httpStatus: status, + message: expect.stringContaining("not followed"), + }); + expect(attackerHits).toBe(0); + } + ); + }); + + it("keeps 304 on the generic non-ok path, not the redirect refusal", async () => { + // 304 is a cache validator, not a redirect-with-Location. + server.use(http.post(tokenEndpoint, () => new HttpResponse(null, { status: 304 }))); + + const err = await callRefresh().catch((e) => e); + expect(err).toBeInstanceOf(BasecampError); + expect(err.code).toBe("api_error"); + expect(err.httpStatus).toBe(304); + expect(err.message).not.toContain("not followed"); + }); + + it.each([Number.NaN, Infinity, -5, 0])( + "normalizes invalid timeoutMs %p to the default instead of instant-aborting", + async (badTimeout) => { + // An unclamped NaN/Infinity became setTimeout's ~1 ms delay — an + // immediate abort masquerading as "Token request timed out". + server.use( + http.post(tokenEndpoint, async () => { + await delay(50); + return HttpResponse.json({ access_token: "tok", token_type: "Bearer" }); + }) + ); + + const token = await refreshToken( + { tokenEndpoint, refreshToken: "my_refresh_token" }, + { timeoutMs: badTimeout } + ); + expect(token.accessToken).toBe("tok"); + } + ); + + it("bounds the exchange even when a custom fetch ignores its AbortSignal", async () => { + // A never-settling fetch that ignores its signal must not hold the call + // open past the timeout — the raceAbort wrapper rejects on the timer. + const neverSettles: typeof globalThis.fetch = () => new Promise(() => {}); + + await expect( + refreshToken( + { tokenEndpoint, refreshToken: "my_refresh_token" }, + { fetch: neverSettles, timeoutMs: 20 } + ) + ).rejects.toMatchObject({ + code: "network", + message: expect.stringContaining("timed out"), + }); + }); +}); + describe("Response Size Limits", () => { const tokenEndpoint = "https://launchpad.37signals.com/authorization/token";