Skip to content

BC5 OAuth: device flow (RFC 8628, 5 SDKs)#370

Open
jeremy wants to merge 61 commits into
mainfrom
oauth-device-flow
Open

BC5 OAuth: device flow (RFC 8628, 5 SDKs)#370
jeremy wants to merge 61 commits into
mainfrom
oauth-device-flow

Conversation

@jeremy

@jeremy jeremy commented Jul 15, 2026

Copy link
Copy Markdown
Member

Device flow (RFC 8628)

Part 2 of 2. #369 (resource-first discovery) is now merged; this PR is restacked directly on main and reviewed on its own.

Why this exists

Not every client can open a browser to sign in. Command-line tools, CI tasks, and input-constrained or headless devices need a login flow that works with nothing but a code and a URL the user visits elsewhere. This PR adds the OAuth 2.0 Device Authorization Grant (RFC 8628) to every SDK, giving those clients a first-class, browserless path to a Basecamp access token.

It is the second half of Basecamp's OAuth modernization: #369 taught the SDKs to discover the right authorization server; this PR teaches them to authenticate against it without a browser.

What you get — one uniform API across TypeScript, Python, Go, Ruby, and Kotlin

  • requestDeviceAuthorization(...) — begin the grant; returns the user code, verification URL, and polling parameters.
  • pollDeviceToken(...) — poll to completion, honoring the server's pacing (authorization_pending, sustained slow_down), a monotonic expiry deadline, backoff on transient transport failure, and caller cancellation.
  • performDeviceLogin(...) — the end-to-end convenience path: takes an already-discovered config, guards that the server actually supports the device grant, surfaces the user code/URL through a display hook, and polls until the user approves.

Terminal outcomes are a single typed DeviceFlowError whose category is derived from the reason (denied/expired → auth, transport → retryable network, unsupported → validation, cancelled → native cancellation), uniform across all five languages.

Compatibility & rollout

  • Additive. New surface only; nothing existing changes.
  • Activates with the server. The device grant becomes usable once a Basecamp authorization server advertises a device-authorization endpoint and the device_code grant. Today's Launchpad metadata advertises neither, so performDeviceLogin reports unavailable against it by design — the capability lights up when the first-party server does, with no client change required.
  • Targeted at the pre-registered public basecamp-cli client (no client secret, grants device_code + refresh_token).

Design intent

  • Correct pacing and lifetimes. The poll loop uses a monotonic, injectable clock; sustains slow_down increments; anchors expiry to a real deadline rather than a running counter; and never overflows timer arithmetic. Durations are validated uniformly across languages (positive whole seconds within a safe bound; integer-valued floats accepted, fractional rejected).
  • Malformed responses fail cleanly. A tokenless or non-object success body, a non-string code/URL, or an out-of-range lifetime is a typed api_error — never a raw crash or a misclassified retryable transport error.
  • Same contract, five languages. Shared specification and per-SDK regression tests keep polling, backoff, cancellation, the capability guard, and error mapping behaviorally identical everywhere.

Verification

make check — all five SDKs (plus Swift) and the conformance/lint gates — passes at the branch tip.


Summary by cubic

Adds the OAuth 2.0 device authorization flow (RFC 8628) to TypeScript, Python, Go, Ruby, and Kotlin so basecamp-cli can log in without a browser. Codifies and hardens the device-flow contract in SPEC.md §16 and updates SDK READMEs.

  • New Features

    • Uniform APIs in 5 SDKs: requestDeviceAuthorization, pollDeviceToken, performDeviceLogin; plus DeviceAuthorization and DEVICE_CODE_GRANT_TYPE.
    • Capability guard via discovery; requires device_authorization_endpoint and grant_types_supporteddevice_code (Launchpad-only configs return unavailable).
    • Polling: handles authorization_pending and sustained slow_down (+5s), uses a monotonic expiry deadline, is cancellation-aware, enforces HTTPS with no redirects, and separates server interval from timeout backoff.
  • Bug Fixes

    • Token responses: 3xx classified by status before reading the body; error must be a non-empty string (else http_<status>); expires_in (when present) is a positive whole number ≤ 2_147_483_647; token_type defaults to Bearer when absent/null; explicit empty token_type is api_error; all malformed-field errors carry httpStatus.
    • Device-authorization: check status before parse; carry httpStatus on all validation errors; accept verification_uri_complete null as absent; Go models it as optional (*string).
    • Timeouts & caps: per-operation timeout normalization (device default 30s, guarded maximums) and body-cap normalization applied at public entry points; enforce wall-clock read deadlines and skip draining bodies whose status is unused; TypeScript normalizes invalid timeoutMs; Kotlin marks completed 5xx as non-retryable.
    • Cancellation: propagate caller cancellation during device-auth POST and body read (Go); interruptible poll waits (Python/Ruby); fix abort race and use a runtime-agnostic abort error (TypeScript).
    • Guards: poll entry points validate caller durations (non-finite/≤0/oversized rejected as usage); capability guard requires real array membership (no substring matches).

Written for commit e9f1414. Summary will update on new commits.

Review in cubic

Copilot AI review requested due to automatic review settings July 15, 2026 14:46
@github-actions github-actions Bot added documentation Improvements or additions to documentation typescript Pull requests that update TypeScript code ruby Pull requests that update the Ruby SDK go kotlin enhancement New feature or request and removed documentation Improvements or additions to documentation labels Jul 15, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2890db6e63

ℹ️ 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".

Comment thread typescript/src/oauth/device.ts
Comment thread typescript/src/oauth/device.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements the OAuth 2.0 Device Authorization Grant (RFC 8628) contract in SPEC.md and rolls it out across the TypeScript, Python, Ruby, Go, and Kotlin SDKs, including deterministic tests and end-user documentation.

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.

Changes:

  • Adds RFC 8628 device-flow primitives (requestDeviceAuthorization, pollDeviceToken) plus an orchestrator (performDeviceLogin) with capability guarding and typed terminal errors.
  • Implements RFC 8628 polling behavior (sustained slow_down, timeout backoff, monotonic deadline / injectable clock, cancellation awareness) with per-SDK test coverage.
  • Updates SDK READMEs and exports to document and expose the new device-flow APIs.

Reviewed changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
typescript/tests/oauth/device.test.ts Adds deterministic Vitest coverage for device auth, polling, cancellation, and error mapping.
typescript/src/oauth/types.ts Introduces DeviceAuthorization type used by device-flow APIs.
typescript/src/oauth/index.ts Exports device-flow APIs, types, and error types from the oauth entrypoint.
typescript/src/oauth/device.ts Implements device authorization request + token polling loop for RFC 8628.
typescript/src/oauth/device-login.ts Adds performDeviceLogin orchestrator and capability guard.
typescript/src/oauth/device-errors.ts Defines DeviceFlowError + reason→category mapping for terminal device-flow outcomes.
typescript/README.md Documents how to use device flow and the derived error taxonomy.
SPEC.md Adds the cross-SDK device-flow contract (RFC 8628) and terminal-outcome mapping.
ruby/test/basecamp/oauth_device_test.rb Adds deterministic Minitest coverage for request/poll/orchestrate behavior.
ruby/README.md Documents device authorization usage and error taxonomy.
ruby/lib/basecamp/oauth/device_flow.rb Implements request/poll/orchestrate device flow for Ruby.
ruby/lib/basecamp/oauth/device_flow_error.rb Adds Ruby DeviceFlowError with derived parent error type.
ruby/lib/basecamp/oauth/device_authorization.rb Defines Ruby DeviceAuthorization value object for the device/user code pair.
ruby/lib/basecamp/oauth.rb Exposes device-flow functions as public OAuth entrypoints.
python/tests/oauth/test_device.py Adds pytest coverage for request/poll/orchestrate behavior and error mapping.
python/src/basecamp/oauth/errors.py Adds Python DeviceFlowError and reason→parent-category mapping (incl. usage).
python/src/basecamp/oauth/device.py Implements synchronous device-flow request/poll/orchestrate functions.
python/src/basecamp/oauth/device_authorization.py Defines Python DeviceAuthorization dataclass.
python/src/basecamp/oauth/init.py Re-exports device-flow APIs and types from the oauth package.
python/README.md Documents device flow usage and terminal error mapping.
kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/OAuthDeviceTest.kt Adds deterministic coroutine/virtual-time tests for device flow.
kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/ErrorTest.kt Extends error classification tests to include the new DeviceFlow exception type.
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/oauth/Exchange.kt Makes refreshToken client secret optional for public clients.
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/oauth/Device.kt Implements device flow in Kotlin (request/poll/orchestrate + TimeSource-based deadline).
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/BasecampException.kt Introduces BasecampException.DeviceFlow with reason-derived taxonomy and defaults.
kotlin/README.md Documents Kotlin device authorization flow usage and behavior.
go/README.md Documents Go device flow usage, capability guard, and error taxonomy behavior.
go/pkg/basecamp/oauth/device.go Implements device flow for Go with bounded reads, backoff, deadline, and cancellation semantics.
go/pkg/basecamp/oauth/device_test.go Adds deterministic Go tests for request/poll/orchestrate behavior including malformed 2xx token responses.
go/pkg/basecamp/oauth/device_errors.go Adds DeviceFlowError type with reason-derived taxonomy and retryability.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread typescript/src/oauth/device.ts Outdated
Comment thread typescript/src/oauth/device.ts Outdated
Comment thread kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/oauth/Device.kt Outdated
Comment thread kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/oauth/Device.kt Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 30 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="typescript/src/oauth/types.ts">

<violation number="1" location="typescript/src/oauth/types.ts:82">
P2: Consumers using the documented `@37signals/basecamp` entrypoint cannot name the new `DeviceAuthorization` type because it is only exported from `@37signals/basecamp/oauth`. Re-exporting it from the root entrypoint would keep the public device-flow API usable without a subpath import.</violation>

<violation number="2" location="typescript/src/oauth/types.ts:92">
P2: Malformed device responses with fractional `expires_in` or `interval` values are accepted and used to calculate polling deadlines and delays. The response validation should require finite integer values before constructing this type.

(Based on your team's feedback about rejecting non-integral semantic integer fields.) [FEEDBACK_USED].</violation>
</file>

<file name="typescript/README.md">

<violation number="1" location="typescript/README.md:238">
P2: This example does not compile because the package root does not export `performDeviceLogin` or `DeviceFlowError`. Import them from `@37signals/basecamp/oauth` (or add root re-exports) so the documented example is usable.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread typescript/src/oauth/device.ts
Comment thread go/README.md Outdated
Comment thread kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/oauth/Device.kt Outdated
Comment thread go/pkg/basecamp/oauth/device.go
Comment thread typescript/src/oauth/device-login.ts
Comment thread python/src/basecamp/oauth/device.py
Comment thread go/pkg/basecamp/oauth/device.go Outdated
Comment thread go/pkg/basecamp/oauth/device.go Outdated
Comment thread go/pkg/basecamp/oauth/device_errors.go
@jeremy
jeremy force-pushed the oauth-discovery-transition branch from b012173 to 336df9d Compare July 15, 2026 19:34
@jeremy
jeremy requested a review from Copilot July 15, 2026 19:34
@jeremy
jeremy force-pushed the oauth-device-flow branch from 2890db6 to b8d4cbb Compare July 15, 2026 19:34
@github-actions github-actions Bot added documentation Improvements or additions to documentation and removed documentation Improvements or additions to documentation labels Jul 15, 2026
@jeremy

jeremy commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

Review round 2 — pushed (history folded, 6 commits/branch preserved)

Addressed the genuine blockers from the automated + manual review. Fixes are folded into their owning per-SDK commits (discovery-layer → PR #369, device-layer → PR #370); both branch tips pass full make check.

Discovery (PR #369):

  • Kotlin: coroutine CancellationException now propagates (no silent Launchpad fallback after a cancelled discovery); requireOriginRoot rejects empty userinfo.
  • Ruby: get_absolute(allow_origin:) is no longer a general credential escape hatch — allow_origin is re-validated through require_origin_root! (fails closed on non-https/path/query/junk), so the bearer token can only reach Launchpad, the configured base, or a discovery-selected-and-validated issuer.
  • Go & Ruby: origin parsing rejects out-of-range ports (1–65535).
  • TS & Python: issuer-mismatch is classified via a structured marker (module-private to the discovery module), not error-message text.
  • Python & Ruby: response field type validation (strings/ints/arrays-of-strings), not truthiness.

Device flow (PR #370):

  • Bounded/streaming reads on device-auth + token responses (TS, Python, Ruby, Kotlin; Go already bounded).
  • Redirect suppression on all device POSTs (Go ErrUseLastResponse, Kotlin followRedirects=false, TS redirect:"manual", Python follow_redirects=False, Ruby injected-client guard).
  • TS: malformed 2xx token response → api_error (not retryable transport). Kotlin: empty access_token rejected.
  • Polling deadline now bounds everything: per-request timeout always set, backoff wait clamped so it can't overshoot the monotonic expires_in deadline, deadline re-checked after the display hook.
  • READMEs: device examples now derive the config from resource-first discovery (not Launchpad, which advertises no device endpoint).

Dismissed — false positive: the Ruby IPv6 claim is incorrect under the project Ruby — URI.parse("http://[::1]:3000").host preserves [::1], and require_origin_root!/localhost? already handle it. No change.

Trivy (Go SDK) check: the failure is a scanner-version/DB discrepancy on an unchanged go.sum (neither PR touches go.mod/go.sum); local trivy — 0.69.1 and a fresh :2 DB — reports zero HIGH/CRITICAL. It reproduces on main, so it's pre-existing and needs a CI-side Trivy pin/.trivyignore or a dependency bump on main, not a change in these feature PRs.

Still under the agreed merge hold pending bc3 #9471.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.

Comment thread typescript/src/oauth/device-login.ts
Comment thread go/pkg/basecamp/oauth/device.go
Comment thread go/pkg/basecamp/oauth/device.go

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b8d4cbb813

ℹ️ 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".

Comment thread typescript/src/oauth/device-login.ts Outdated
Comment thread go/pkg/basecamp/oauth/device.go Outdated
@jeremy
jeremy marked this pull request as draft July 15, 2026 23:54
@jeremy
jeremy force-pushed the oauth-discovery-transition branch from 336df9d to 64e2ef3 Compare July 16, 2026 02:10
@jeremy
jeremy requested a review from Copilot July 16, 2026 02:10
@jeremy
jeremy force-pushed the oauth-device-flow branch from b8d4cbb to 7207f74 Compare July 16, 2026 02:10
@github-actions github-actions Bot added documentation Improvements or additions to documentation and removed documentation Improvements or additions to documentation labels Jul 16, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.

Comment thread go/pkg/basecamp/oauth/device.go Outdated
Comment thread kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/oauth/Device.kt Outdated
Comment thread kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/oauth/Device.kt Outdated
jeremy added 29 commits July 21, 2026 22:36
httpx's timeout is per-read (it resets on every received chunk) and httpx has no
total-request timeout, so a peer slow-dripping a VALID response byte-by-byte —
each read under the timeout — could hold a device request open far past it
(verified: a 0.5s timeout took ~20s under a 0.3s drip). The prior monotonic-
deadline checks only ran between chunks and could not bound the blocking header
read; closing the client from a watchdog does not interrupt a blocked read
either. Run the request on a daemon worker and bound the CALLER with
join(timeout): a stalled/drip request now returns within the timeout as a
retryable transport failure, and the abandoned daemon worker never blocks exit.
Adds a real-socket drip test asserting the bound.

Also harden _normalize_timeout to validate its operation default (extract
_finite_positive_timeout): a caller passing an invalid default can no longer
disable both bounds — it falls back to the discovery constant.
- parse_device_authorization parsed JSON before checking status, so a non-2xx
  with a non-JSON body (common for 500/502) surfaced as a parse error instead of
  'failed with status …'. Check status first (matching discovery and the Python
  request path); the token poll still parses non-2xx bodies for authorization_pending.
- request_device_authorization / poll_device_token now normalize the timeout ONCE
  at operation entry (outside the poll loop) and pass the SAME value to build_client
  and every post_form, so an invalid input can't leave the default-built client's
  socket timeout unbounded. post_form no longer re-normalizes.
…fication_uri_complete (TS)

- requestDeviceAuthorization and pollDeviceToken hard-coded 30000 in their
  parameter defaults while DEFAULT_DEVICE_TIMEOUT_MS existed — reference the
  constant so the omitted-default and normalized-invalid paths can't diverge.
- RawDeviceAuthorization.verification_uri_complete is typed string | null to
  reflect that a JSON null on the wire is accepted as absent (normalized to
  undefined), matching the Go/Kotlin decoders.
… wait

The daemon-worker bound (previous commit) bounded the CALLER but could not
terminate the leaked worker/socket under a sustained drip, and a huge finite
timeout overflowed thread join. Two fixes:
- Run the request as a coroutine under asyncio.wait_for, which CANCELS it and
  closes the socket at the deadline (verified: caller bounded AND no leaked
  worker — thread count unchanged, unlike a sync thread that can't be killed).
  httpx has no total-request timeout and its per-read timeout resets on drip.
- Cap the request timeout at _MAX_DEVICE_REQUEST_TIMEOUT (3600s) via a maximum=
  arg on _normalize_timeout so a huge value falls back to the default instead of
  overflowing the wall-clock wait primitive.
Also make the poll wait interruptible: time.sleep is not cancellable, so a cancel
set mid-wait was not seen until the whole (possibly grown slow_down) interval
elapsed. _wait_cancellable polls should_cancel every _CANCEL_POLL_INTERVAL when a
probe is set (single sleep otherwise, preserving the schedule) — matching the
ctx/AbortSignal/coroutine cancellation Go/TS/Kotlin already have. Adds tests.
A plain sleep is not cancellable, so a cancellation set during the wait was not
observed until the whole (possibly grown slow_down) interval elapsed. Add
wait_cancellable: with the default no-op probe a single sleep preserves the exact
wait schedule; with a real cancelled probe it polls every
CANCEL_POLL_INTERVAL_SECONDS and raises promptly — matching Go/TS/Kotlin's
cancellable waits. Adds a prompt-cancellation test.
asyncio.run() raises RuntimeError when the sync device helpers are invoked from
code that already has a running loop (Jupyter/FastAPI/async CLI), leaking a raw
error before any request is made. Run the asyncio.wait_for-bounded coroutine in a
DEDICATED thread with its own event loop instead: it works from any context, and
because wait_for cancels the request at the deadline the thread always completes
within ~timeout (joining it never blocks; no leaked worker). Typed
OAuthError/DeviceFlowError/ReadTimeout are re-raised on the caller thread.

Also clamp the final _normalize_timeout fallback to a caller-supplied maximum
below _DISCOVERY_TIMEOUT, so an impossible maximum can't be bypassed. Adds the
inclusive-boundary and sub-default-maximum tests.
Inserting wait_cancellable left the POST doc-comment stranded above it; move the
POST contract back directly above post_form and keep wait_cancellable's own doc
with wait_cancellable. Comment-only.
… (Python)

- The bounded coroutine now runs on a DAEMON worker joined with a small grace
  (timeout + _WORKER_JOIN_GRACE): asyncio.wait_for cancels the request at the
  deadline so the worker normally finishes well within it, but if the async
  cancellation/cleanup itself stalls, the caller returns a timeout rather than
  block on an unbounded join — the daemon never blocks interpreter exit.
- Re-sync backoff to the grown interval after slow_down: the reset used the
  pre-increment value, so a following timeout doubled the stale interval (5→10)
  instead of the grown one (10→20), polling too aggressively under combined
  throttling + timeouts. Adds a slow_down-then-timeout schedule test ([5,10,20]).
After slow_down raised the interval, a following poll timeout doubled the stale
pre-increment backoff (5→10) instead of the grown interval (10→20). Re-sync the
backoff to the grown interval. Adds a timeout-on-attempt transport + a
slow_down-then-timeout schedule test asserting [5, 10, 20].
Re-sync backoff to the grown interval after slow_down so a following timeout
doubles from the new interval, not the stale pre-increment one.
Re-sync backoffSeconds to the grown interval after slow_down so a following
timeout doubles from the new interval, not the stale pre-increment one.
The signal could abort between the initial aborted-check and attaching the
listener; with { once } that abort event is already spent, so the listener never
fires and the promise settles only at the full timeout. Re-check signal.aborted
after attaching and handle it manually, dropping the now-dead listener.
…(TypeScript)

- Device-auth and token-poll paths read the response body BEFORE checking status,
  so a non-2xx / suppressed-3xx response that slowly streamed (or never finished)
  its body could time out mid-read and be misclassified: a hard api_error surfaced
  as a retryable transport failure (device-auth), or a redirecting token endpoint
  kept the poll loop backing off and retrying until expiry instead of failing.
  Check status first — a non-2xx (device-auth) or 3xx (token) whose body is unused
  fails by status immediately; a 4xx token body is still read for
  authorization_pending/slow_down.
- Validate a token  as a non-empty string: a non-string (e.g. {"error":123})
  no longer becomes the OAuth error code — fall back to http_<status>, matching the
  other SDKs. Adds tests.
data.get("error") or f"http_{status}" accepted any truthy non-string (dict/number)
as the OAuth error code. Treat a non-string/empty error as absent and fall back to
http_<status>, matching the other SDKs. Adds a test.
RequestDeviceAuthorization drained the body BEFORE checking status, and the token
poll read the body before classifying a 3xx. A non-2xx/3xx that slowly streamed
its body could hit the request timeout mid-read and be misclassified — a hard
api_error as a retryable transport failure, or a redirecting token endpoint kept
the poll backing off and retrying until expiry. Check status first: a non-2xx
device-auth and a 3xx token response (whose bodies are unused) fail by status
immediately; a 4xx token body is still read for authorization_pending/slow_down.
Adds an oversized-3xx test asserting it fails by status, not the size cap.
…cript)

The status-first throw branches (non-2xx device-auth, 3xx token) left response.body
undrained, so under repeated failures the response stream could retain sockets /
connection-pool resources. Cancel it non-blockingly before throwing; the status
error still surfaces immediately.
The token poll read the whole body before the status branches, so a redirect that
slowly streamed its body could time out mid-read and be retried by the poll loop
until expiry. Return the redirect api_error by status BEFORE readBoundedText; a 4xx
body is still read for the OAuth error. Device-auth already checked status first.
_post_form_bounded read the whole body before the caller checked status, so a slow
non-2xx (device-auth) or 3xx (token) body could time out mid-read and be
misclassified as transport / retried until expiry. Add a read_body(status)
predicate — evaluated once headers arrive — so a caller returns without draining a
body it does not use (device-auth reads only 2xx; the poll skips 3xx, still reads
4xx). Adds an oversized-3xx test asserting it fails by status, not the size cap.
Faraday streams the body during the synchronous request, so a slow non-2xx/3xx
body could time out (→ transport / poll retry-until-expiry) before post_form
checked status. Faraday passes env.status to on_data once headers arrive: a new
skip_status predicate raises Fetcher::SkipBody from on_data for a status whose body
the caller does not use (device-auth non-2xx, token 3xx), and post_form returns it
empty for the caller to classify. Adds a streaming-adapter test asserting an
oversized 302 body fails by status, not the size cap.
PollDeviceToken is an exported entry point: a non-positive expiresIn built a
deadline in the past, and an oversized one overflowed the
time.Duration(seconds) * time.Second math into a garbage deadline. Reject both
up front as a usage error, mirroring the TS pollDeviceToken guard. The
PerformDeviceLogin path is unaffected: it polls via pollDeviceTokenUntil with
already-validated, clamped values.
poll_device_token is a public entry point: a non-finite expires_in (inf) built
a deadline that never passes — an unbounded poll loop — and zero/negative/
oversized/bool durations are not schedulable waits. Reject them up front as a
usage error, mirroring the TS/Go caller guards. perform_device_login is
unaffected: it passes the server-validated interval and a positive fractional
remaining lifetime, both within the shared ceiling.

Also align the worker-join comment with the is_alive backstop it sits above,
note the 3.11+ TimeoutError aliasing the isinstance check relies on, and
complete the docstring raise list (usage, api_error).
…adapter (Ruby)

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 reached post_form with the
skip-status body un-skipped, so an oversized 3xx tripped the size cap instead of
the redirect api_error. Re-apply skip_status to the completed response.status as
a backstop, and state the one genuinely unreachable case honestly in place:
Faraday has no headers-time callback, so a body that stalls PAST the read
timeout surfaces as a bounded transport timeout (never followed, never
unbounded); exact headers-first semantics for that case belong to the transport
primitive shared with discovery, tracked as a pre-go-live hardening follow-up.

Also reject out-of-range caller durations at the public poll entry as usage
(nil interval raised NoMethodError; an infinite expires_in built a deadline
that never passes), mirroring the Go/TS/Python guards; coerce integer-valued
Float expires_in/interval to Integer, matching build_token and the declared
types; and fold build_client into Fetcher.build_client, which it duplicated.
DeviceLoginOptions.signal is deliberately scoped to the polling loop — the
device-code request is bounded by its own timeout and the display hook is caller
code; say so on the option. defaultClock: name the Date.now() branch for what it
is, a wall-clock last resort for runtimes without performance, so the
MonotonicClock label cannot be read as a guarantee the fallback does not keep.
…tlin)

A 4xx token body of {"error":""} decodes cleanly (error is a required non-null
String, so no SerializationException fires) and surfaced a dangling message;
normalize it to http_<status> via ifEmpty, matching the coercion Go, TS, Python,
and Ruby already apply.

Reject out-of-range interval/expiresIn at the exported pollDeviceToken entry as
a usage error: an oversized Long saturates Duration to infinite, building a
deadline that never passes — an unbounded poll loop — and non-positive values
are not schedulable. performDeviceLogin is unaffected (it passes
server-validated values and its own issuance-anchored deadline).

The token 3xx early return needs no explicit channel cancel: HttpStatement
.execute runs cleanup in its finally, which cancels the raw content channel
(verified against ktor-client-core 3.5.1); state that guarantee in place.
Complete the BasecampException KDoc when-example with the Ambiguous branch.
…Python)

Two guard escapes on the public entries:

The caller-duration guard ran math.isfinite before the range checks, and
isfinite converts its argument to float — so an astronomically large int
(10**400) raised OverflowError out of the guard instead of the usage error.
Range checks now run first (int comparisons never convert; NaN compares False
on both and falls through to the isfinite reject).

max_body_bytes was forwarded unnormalized, so an invalid runtime cap
(float("inf"), None, a negative) disabled the streaming memory bound —
total > inf never trips — despite the documented 1 MiB guard. Normalize at
both public entries via _normalize_body_cap, now parameterized with an
operation-specific default (validated too), mirroring _normalize_timeout.
max_body_bytes was forwarded unnormalized into bounded_reader and the buffered
fallback, so an invalid runtime cap (Float::INFINITY, nil, a negative) on a
direct DeviceFlow call disabled the size bound the methods advertise. Add
Fetcher.normalize_body_cap (non-negative Integer, validated default — the same
discipline the discovery initializer applies) and normalize once at the
request/poll entries alongside the existing timeout normalization.
…ice flow (Ruby)

Discovery#initialize and Resource#initialize carried inline copies of the cap
normalization that Fetcher.normalize_body_cap now implements; route both
through the shared helper so the three consumers cannot drift.
BasecampException.Api defaults retryable to httpStatus in 500..599, so a
completed 5xx from the device-auth or token endpoint read as retryable — but in
the device flow only the transport reason is retryable, and the other four SDKs
surface these as a non-retryable api_error (Python/Ruby default false, Go zero
value, TS unset). Pass retryable = false at the three 5xx-capable raise sites:
the device-auth non-2xx classification, the token PollResult.Other fault, and
the shared readBoundedText size cap (device polls read 5xx bodies; an oversized
body is a completed fault in discovery too).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.

Comment on lines +537 to +558
var raw struct {
AccessToken string `json:"access_token"`
RefreshToken string `json:"refresh_token"`
TokenType *string `json:"token_type"`
ExpiresIn *float64 `json:"expires_in"`
Scope string `json:"scope"`
}
if err := json.Unmarshal(body, &raw); err != nil {
return pollResult{kind: pollInvalidResponse, status: resp.StatusCode, err: fmt.Errorf("parsing device token response: %w", err)}
}
if raw.AccessToken == "" {
return pollResult{kind: pollInvalidResponse, status: resp.StatusCode, err: errors.New("device token response missing access_token")}
}
if raw.TokenType != nil && *raw.TokenType == "" {
return pollResult{kind: pollInvalidResponse, status: resp.StatusCode, err: errors.New("device token response token_type must be a non-empty string")}
}
token := Token{
AccessToken: raw.AccessToken,
RefreshToken: raw.RefreshToken,
TokenType: "Bearer",
Scope: raw.Scope,
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request go kotlin python Pull requests that update the Python SDK ruby Pull requests that update the Ruby SDK typescript Pull requests that update TypeScript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants