Refuse redirects and bound timeouts on the token exchange path in every SDK - #813
Conversation
…ry SDK The token-exchange/refresh POST carries the authorization code, the client secret, or the refresh token, and it was the last response-steerable hop that still followed redirects (Go: ten hops, a 307/308 re-POSTing the form; TS: twenty) and the last credential POST unbounded in Go. Every SDK with an exchange path now refuses 301/302/303/307/308 with the typed api_error carrying the real status and the SPEC §14 "not followed" message, classified before any body read, on injected clients too; timeouts converge on the shared 30 s default / 3600 s ceiling / normalize-to-default rule (new Go WithExchangerTimeout; AuthManager refresh gains suppression and classification but keeps the operator-configured client's timeout). Ruby's default lane moves to the headers-first Fetcher.stream_http transport and its legacy OauthTokenProvider gets the full contract; the injected-Faraday lane keeps the injected-client fidelity tier, stated in the new SPEC §16 "Token-Endpoint Transport Policy" subsection. Appendix F retracts the wrong claim that Kotlin's exchange followed redirects (Ktor never follows POSTs) and records Kotlin's real defects — a 3xx thrown as Auth with the status lost, no HttpTimeout, injected clients used verbatim — all closed here.
github-actions
Bot
added
typescript
9e04730 to
8b5cfb9
Compare
There was a problem hiding this comment.
Pull request overview
Standardizes token-endpoint redirect refusal and timeout handling across Go, Kotlin, TypeScript, Python, and Ruby.
Changes:
- Refuses credential-bearing redirects with typed errors.
- Adds or normalizes 30-second request bounds.
- Documents and tests the cross-SDK transport policy.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
SPEC.md |
Defines the token-endpoint transport policy. |
MIGRATING.md |
Documents behavior and migration impacts. |
go/pkg/basecamp/oauth/exchange.go |
Adds redirect refusal and request deadlines. |
go/pkg/basecamp/oauth/oauth_test.go |
Tests exchange transport policy. |
go/pkg/basecamp/auth.go |
Refuses redirects during managed refresh. |
go/pkg/basecamp/auth_test.go |
Tests managed-refresh refusal. |
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/oauth/Exchange.kt |
Hardens token clients and error mapping. |
kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/OAuthTest.kt |
Tests redirects and timeout classification. |
typescript/src/oauth/limits.ts |
Centralizes timeout and abort helpers. |
typescript/src/oauth/exchange.ts |
Adds manual redirects and bounded awaiting. |
typescript/src/oauth/device.ts |
Reuses shared OAuth limits. |
typescript/tests/oauth/oauth.test.ts |
Tests redirects and timeout normalization. |
python/src/basecamp/oauth/exchange.py |
Adds headers-first redirect classification. |
python/tests/oauth/test_exchange.py |
Tests Python redirect refusal. |
ruby/lib/basecamp/oauth/fetcher.rb |
Generalizes injected-client validation text. |
ruby/lib/basecamp/oauth/exchange.rb |
Introduces hardened exchange transports. |
ruby/lib/basecamp/oauth_token_provider.rb |
Hardens legacy refresh requests. |
ruby/test/basecamp/security_test.rb |
Updates body-limit error expectations. |
ruby/test/basecamp/oauth_transport_test.rb |
Tests headers-first classification. |
ruby/test/basecamp/oauth_test.rb |
Tests timeout normalization. |
ruby/test/basecamp/oauth_ssrf_test.rb |
Tests redirect and injected-client handling. |
ruby/test/basecamp/auth_test.rb |
Tests legacy refresh transport policy. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e04730bdc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Consumer-side counterpart is up: basecamp/basecamp-cli#656 makes the #804/#810 address policy live in the CLI (pinned to v0.15.0; its skipped redirect-status-survival test un-skips at the re-pin past this PR). Cross-SDK enforcement tracking: #818. |
…aqueredirects Review round on #813: - Python: the exchange posted through a bare httpx.stream, whose timeout is per I/O phase and whose read() buffers the whole body before the 1 MiB check. Route it through the shared request_bounded transport instead — total wall-clock deadline, streaming size cap, redirect statuses skipped via read_body so they still classify from the headers with the body unread. _parse_token_response now takes (status, body). - TypeScript: a browser fetch answers redirect: "manual" with an opaqueredirect (type "opaqueredirect", status 0), which the status table could not see — refuse it by type, without a status to carry. - MIGRATING: the signed-hop rule shipped in #809, not issue #805. - Kotlin/SPEC/MIGRATING: state that the engine re-wrap governs Ktor's client-level redirect plugin only; every Ktor engine defaults engine-level following off, so an injected engine must not opt in (the Kotlin counterpart of Ruby's adapter-only rule).
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
go/pkg/basecamp/oauth/exchange.go:28
- This public type comment says every 3xx is a typed
api_error, but the implementation and SPEC intentionally keep 304 and other non-redirect 3xx on generic handling. Narrow the wording to the five refused redirect statuses.
// [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).
go/pkg/basecamp/oauth/exchange.go:82
- The sentence immediately above says
http.DefaultClientrestores pre-policy behavior “outright,” which now contradicts this new contract: only address enforcement is disabled; redirect suppression and the exchanger timeout remain active. Update the constructor documentation so callers do not expect the old redirect/timeout behavior.
// 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.
MIGRATING.md:63
- The accompanying test records another Ruby migration: oversized exchange responses now raise
Basecamp::Oauth::OauthErrorinstead ofBasecamp::ApiError. Because existingrescue Basecamp::ApiErrorhandlers no longer catch this failure, document that exception-taxonomy break in the Ruby migration bullet.
- **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,
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…s overclaiming request_bounded's oversized-body OAuthError dropped the http_status the buffered pre-transport path used to carry, even though the cap can only trip on a status read_body already admitted — restore it (discovery and the device flow inherit the enrichment) and pin it in the exchange cap test. Two doc corrections from review: the Exchanger type comment claimed every 3xx is a typed api_error where only the five refused statuses are (304 stays generic), and NewExchanger's http.DefaultClient sentence promised the pre-policy behavior "outright" when redirect refusal and the timeout survive every client choice. MIGRATING's Ruby bullet now records the taxonomy break the streaming cap introduced: an oversized Exchange response raises Oauth::OauthError where it raised ApiError.
|
Fixed the three suppressed review comments in 0f2d31e: the Exchanger type comment now names the five refused statuses instead of claiming every 3xx (304 and the rest stay on the generic non-200 path, matching SPEC and the implementation); NewExchanger's http.DefaultClient sentence no longer promises the pre-policy behavior "outright" — it now says only the address policy is switched off while redirect refusal and the request timeout survive every client choice; and MIGRATING's Ruby bullet records the exception-taxonomy break for oversized Exchange responses (streaming cap raises Basecamp::Oauth::OauthError where the post-hoc check raised Basecamp::ApiError). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f2d31eac9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ntract SPEC's Token-Endpoint Transport Policy promised every refused redirect a typed error carrying its status, but a browser Fetch answers redirect: "manual" with an opaqueredirect whose status the browser hides — TypeScript's browser branch deliberately throws without httpStatus. Name that carrier exception in the contract and in MIGRATING's TypeScript bullet: the "not followed" substring is the portable contract, the status field is not, so cross-runtime consumers must not key redirect handling on it.
The token-exchange/refresh POST carries the highest-value credentials the SDK ever sends — the authorization code, the client secret, or the refresh token — and after #809 established "no response-steered hop follows redirects" (SPEC §14) and #810 policed the endpoint addresses, it remained the one response-steerable hop that still followed redirects (Go: 10 hops, 307/308 re-POSTing the form; TS: 20) and ran unbounded in Go. This PR makes the exchange path uniform with the device flow and states the contract as SPEC §16 Token-Endpoint Transport Policy
[static].The contract, in every SDK with an exchange path
redirect {status} on the token endpoint is not followed(substring contract:not followed, same as §14), and theLocationis never dialled. Any other 3xx (304 above all) stays the generic non-2xx failure.Per SDK
doTokenRequestcarries the POST on anoRedirectClientcopy (injected clients included), classifies redirects before the body read, and runs under a child-context deadline — 30 s default, newWithExchangerTimeout(ceiling = the shared device ceiling).AuthManager.refreshLockedgains suppression + classification but no timeout (it runs on the operator-configured API client).postTokenRequestnow wraps every client (injected engines re-wrapped, thehardenedDeviceClientpattern) withfollowRedirects = false+ 30 sHttpTimeout; a 3xx isBasecampException.Apiwith the real status where it previously fell into the genericAuthbranch with the status lost;HttpRequestTimeoutExceptionmaps to retryableNetwork.redirect: "manual"+ status-first refusal (a browser'sopaqueredirectis refused by type, status unavailable);timeoutMsnow clamps through the shared resolver (NaN/Infinity no longer instant-abort or unbound); the whole round trip races the abort signal, so a custom fetch that ignoresAbortSignalcannot hold the exchange past its deadline.raceAbort/the timeout resolver moved tooauth/limits.ts; the device flow delegates.httpx.postto the sharedrequest_boundedtransport — total wall-clock deadline (httpx's own timeout is per I/O phase), streaming 1 MiB cap, redirect statuses skipped viaread_bodyso they classify from the headers with the body unread.Fetcher.stream_httptransport; an injected Faraday client is vetted byensure_redirects_suppressed!and bounded by the device flow's full wall-clock discipline; the constructor timeout normalizes (3600 s ceiling). The legacyOauthTokenProvider.perform_refresh, previously a bare unboundedFaraday.post, gets the full contract (stream_http, 30 s, redirect refusal asApiErrorwith the status).Corrections recorded
Appendix F previously claimed Kotlin's exchange followed redirects, reasoned from source absence of
followRedirects = false. It never did (Ktor'sHttpRedirectdefaultscheckHttpMethod = true; CIO doesn't follow at engine level) — the appendix now retracts that explicitly and names Kotlin's real defects, all closed here. The non-uniformity paragraph collapses to the uniform state.Conformance scope
Per-SDK unit tests only; the subsection is
[static]. The oauth-token corpus schema is resource-semantics-scoped (a response is status + body); one redirect case would need headers/redirect/never-dialled vocabulary — instrument stretching, stated in one SPEC sentence. (The corpus has no 3xx fixtures, so no expected classification changed.)Tests
Five-status table over BOTH exchange and refresh in every SDK, plus: 304-stays-generic, attacker-
Location-never-dialled counters (the attacker serves a usable token, proving refusal stopped the chain), stalled-body-302 headers-first classification (Go, Python, Ruby live-socket), timeout normalization tables, live-timeout bounds, TS's never-settling signal-ignoring fetch, Ruby injected-follower refusal at construction + narrowed-lane pins, and the legacy provider's redirect/no-mutation-on-refusal tests. Every refusal was mutation-verified (suppression/classification reverted → tests fail; restored → green).Two honest gaps, on the record: Kotlin's
followRedirects = falseis defense-in-depth no in-repo engine can falsify (Ktor never follows POSTs regardless — the status-first classification is what the tests kill); and Kotlin's 30 sHttpTimeoutinstallation is asserted by code with the exception-mapping tested (runTest's virtual clock cannot advanceHttpTimeout's real-dispatcher timer).Migration
MIGRATING.md "Unreleased" entry: no knob to re-enable following; Go's ctx-less callers now get a 30 s default; Kotlin catch-sites keyed to the old
Authclassification of 3xx must move toApi.Summary by cubic
Refuses token-endpoint redirects and bounds exchange/refresh timeouts across all SDKs. Previously Go/TS followed redirects (Go up to 10 hops, TS up to 20) and Go's exchange could run unbounded; now 301/302/303/307/308 return a typed API error containing "not followed," classification happens before any body read, defaults are 30 s with a 3600 s ceiling (invalid values normalize), and injected clients also refuse redirects (the
Locationis never dialed). 304 stays on the generic non-2xx path.Exchangersuppresses redirects via a shallow no-redirect client and adds a child-context timeout (30 s default;WithExchangerTimeout, ceiling 3600 s). Redirects classify before reads.AuthManager.Refreshnow refuses redirects but keeps the operator client's timeout.followRedirects = falseand a 30 sHttpTimeout. 3xx now surfaces asBasecampException.Api(httpStatus);HttpRequestTimeoutExceptionmaps to retryableNetwork.redirect: "manual", classifies redirects status-first, and refuses a browser's status-hiddenopaqueredirectby type; "not followed" — not the status field — is the portable contract across runtimes.timeoutMsclamps via shared helpers; the request races anAbortSignalso custom fetches cannot hold past the deadline. Shared helpers live inoauth/limits.ts; device flow imports them.request_boundedtransport — total wall-clock deadline, 1 MiB streaming cap — withfollow_redirects=False, classifying redirects from headers before any body read.Fetcher.stream_http; injected Faraday clients are vetted for redirect suppression and run under a wall-clock deadline; constructor timeout normalizes. LegacyOauthTokenProviderrefresh now uses the same transport, 30 s bound, and redirect refusal.surfguardFix: Use correct bucket-scoped endpoints for timesheet reports #24/Add CardColumns Watch/Unwatch and Checkins UpdateAnswer methods #25).Migration
WithExchangerTimeoutto adjust.Auth; they now surface asApiwith the real status.timeoutMsno longer instant-abort or run unbounded; custom fetches that ignoreAbortSignalno longer extend the exchange beyond its timeout.OauthErrorinstead ofApiError.Written for commit b3e398e. Summary will update on new commits.