OAuth transport hardening: headers-first, totally-bounded fetches (Ruby + Python)#376
Open
jeremy wants to merge 13 commits into
Open
OAuth transport hardening: headers-first, totally-bounded fetches (Ruby + Python)#376jeremy wants to merge 13 commits into
jeremy wants to merge 13 commits into
Conversation
Faraday exposes no headers-time callback — on_data is a body callback — so two response shapes could not be classified or bounded correctly on the default path: a non-2xx/3xx whose body stalls (misclassified as a transport timeout and retried, where SPEC.md §16 promises an immediate api_error) and a stalled or byte-dripped HEADER phase (a per-read timeout resets on every byte, so a drip holds the request open past any deadline). Fetcher.stream_http replaces Faraday for SDK-built requests, on Net::HTTP: - The response block yields at HEADER time, so skip_status classifies before any body read; raising out of the block closes the socket undrained. - A watchdog thread closes the connection at a monotonic wall-clock deadline, interrupting even a blocked or dripped header read (close from another thread raises IOError in the blocked reader — verified on live sockets). max_retries = 0 is load-bearing: the idempotent-retry default would silently reopen the connection the watchdog just closed. - The body streams under the existing cap + deadline; redirects are structurally never followed; transport failures surface as the same Faraday error classes so both paths share caller rescues. Discovery, resource, and device-flow default paths all route through it. An INJECTED Faraday connection keeps the previous path (redirect-verified, per-request timeout, wall-clock deadline, status backstop) with its documented stall residual, now scoped to injection only. Acceptance tests run against real sockets (WebMock fully disabled — its patched Net::HTTP buffers even allowed requests, destroying the header-time semantics under test): header stall, header drip, body drip, oversized body, undrained-close on skip, zero retries on a stalled 302 token response, and watchdog thread cleanup.
Discovery still fetched over sync httpx with a between-chunks wall-clock deadline — which never bounds a header-phase stall or drip, because sync httpx has no total-request timeout (its per-read timeout resets on every chunk) and closing the client from a watchdog does not interrupt a blocked read. The device flow already solved this with an async client cancelled by asyncio.wait_for on a dedicated worker thread; discovery, which handles the SSRF-exposed URLs, was the more exposed consumer still missing it. Extract that core into _transport.request_bounded and route both consumers through it: device _post_form_bounded delegates (same name, signature, and messages), and discovery _fetch_discovery_document drops its deadline loop for the same total-request bound. Error mapping is unchanged on both. Real-socket tests (respx cannot stall or drip): discovery of headers-then- stall-forever and of a byte-dripped body are both bounded to ~timeout, classified as retryable network timeouts, and leak no worker threads.
There was a problem hiding this comment.
Pull request overview
This PR hardens the OAuth HTTP transport in the Ruby and Python SDKs by enforcing headers-first status classification and a true total-request timeout for discovery and device-flow fetches, closing stalled-body and header-drip/stall edge cases that per-read timeouts can’t bound.
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:
- Ruby: Introduces
Basecamp::Oauth::Fetcher.stream_http(Net::HTTP) as the default headers-first, watchdog-bounded transport; keeps injected Faraday connections on the Faraday path. - Python: Adds
basecamp.oauth._transport.request_bounded(async httpx +asyncio.wait_foron a worker thread) and reuses it for both discovery and device flow. - Adds real-socket acceptance tests in Ruby and Python for header/body stall and drip scenarios and thread cleanup.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ruby/test/basecamp/oauth_transport_test.rb | Real-socket acceptance tests for headers-first classification and total timeout bounding. |
| ruby/lib/basecamp/oauth/resource.rb | Switches default transport selection to nil (Net::HTTP path) unless an HTTP client is injected. |
| ruby/lib/basecamp/oauth/fetcher.rb | Adds stream_http (Net::HTTP) transport, routes discovery fetches via Net::HTTP by default, and isolates injected-Faraday fetch logic. |
| ruby/lib/basecamp/oauth/discovery.rb | Switches default transport selection to nil (Net::HTTP path) unless an HTTP client is injected. |
| ruby/lib/basecamp/oauth/device_flow.rb | Routes device-flow POSTs through Net::HTTP headers-first transport when no client is injected; retains Faraday path for injected clients. |
| python/tests/oauth/test_transport.py | Real-socket tests validating total-timeout bounding and worker thread cleanup. |
| python/src/basecamp/oauth/discovery.py | Replaces per-chunk wall-clock loop with shared request_bounded transport core. |
| python/src/basecamp/oauth/device.py | Delegates bounded POST behavior to shared request_bounded transport core. |
| python/src/basecamp/oauth/_transport.py | New shared bounded transport core implementing total-request timeout and streaming size cap. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: a9efbd8bea
ℹ️ 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".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…der (Ruby) Net::HTTPBadResponse / Net::HTTPHeaderSyntaxError / Net::ProtocolError are bare StandardError subclasses, so a malformed status line from a non-HTTP peer leaked raw from the public discovery/device APIs; map them to Faraday::ConnectionFailed with the other transport failures (real-socket regression test). Fetcher.build_client lost its last caller when the default paths moved to stream_http — remove it. Correct the stream_http timeout doc (the deadline is anchored before connect and open_timeout carries the same value, so the total wall time is ~timeout, not ~2x) and state the marker-exception contract explicitly: BodyTooLarge/ReadDeadlineExceeded deliberately stay non-Faraday so each caller maps them to its operation-specific message, as on the Faraday path.
A 5s cancellation-cleanup grace meant a stalled cleanup could hold the caller to timeout + 5s — materially past the documented total-request bound for small timeouts. Cleanup after a wait_for cancellation is just closing a socket, so 1s is ample; joining with no grace at all would race a request completing right at the deadline. The daemon worker still never blocks interpreter exit.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29be98be02
ℹ️ 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".
…(Ruby) SPEC.md requires non-2xx on either discovery hop to surface as api_error, never network — but the default path drained the error body before the status check, so a stalled/dripped 500 body became a retryable network timeout. Skip non-2xx bodies on the default transport (the diagnostic body was best-effort at most across the SDKs — TS swallows read failures, Go ignores the read error, Kotlin never reads it; category, retryability, and http_status are the observable contract). The injected Faraday path still carries the body, so the message appends it only when present. This reverses an earlier review-thread decline: SPEC.md and the actual sibling-SDK behavior contradict the diagnostic-body contract that decline claimed. Map OpenSSL::SSL::SSLError to Faraday::SSLError exactly as faraday-net_http does, with a real-TLS regression proving a self-signed certificate aborts the handshake and classifies as the same network error as before the transport swap. Test teardown now kills+joins server threads and closes accepted sockets, so the stall handlers cannot outlive their tests.
Same SPEC contract as the Ruby commit: a non-2xx discovery response with a stalled body must classify as api_error at header time, not drain into a network timeout. Pass a 2xx-only read_body to the bounded core and drop the now-unreachable body text from the non-2xx message. Real-socket regression: a 500 with a forever-stalled body classifies immediately.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f06315451
ℹ️ 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".
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…us-only errors (Ruby) Errno::ETIMEDOUT is a SystemCallError but it is a TIMEOUT: the catch-all ConnectionFailed mapping terminated the device poll where faraday-net_http (Timeout::Error, Errno::ETIMEDOUT → Faraday::TimeoutError) fed the transient backoff — special-case it through the timeout mapping. stream_http now defaults Content-Type to application/x-www-form-urlencoded when a form body is given (explicit headers still win), so the helper cannot be called into a subtle server-side parse failure. fetch_json errors go status-only on BOTH paths: embedding the truncated body kept attacker-influenced content in exception messages on the injected path and diverged from the body-less default transport and Python. Category, retryability, and http_status are the observable contract.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e34c74b0e
ℹ️ 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".
A URL like https:foo passes the scheme-only HTTPS guard but parses with a nil hostname, which surfaced as a raw ArgumentError from inside Net::HTTP — outside the transport error contract. Validate the parsed host (and an unparsable URL) before constructing the client, raising a validation OauthError.
stream_http uses Net::HTTP, URI, and OpenSSL constants that were loaded only transitively through Faraday; require them explicitly so Fetcher stands on its own under Zeitwerk-isolated loading.
stream_http rejected nothing for an unknown verb (a :put typo silently became a GET); request_bounded sent params as a body on any method (a GET-with-body is commonly rejected server-side and hard to debug). Both internal helpers now fail fast on the misuse — ArgumentError / ValueError — with unit tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a534e5e698
ℹ️ 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".
A peer that accepts the connection but stops reading trips Net::WriteTimeout, which is a Timeout::Error — not an IOError or SystemCallError — so it escaped the rescue set raw. Rescue Timeout::Error (covering open/read/write alike, the exact class faraday-net_http rescues) and set write_timeout to the normalized value so the write phase is bounded by the same budget. Also require openssl explicitly in the transport test, which uses it directly.
…s (Ruby) The client aborting a connection surfaces on the server side as EPIPE on macOS but ECONNRESET on Linux, so the CI Ruby 4.0 job hit an unrescued Errno::ECONNRESET in the oversized-body handler (the one-off local error was the same race). Every handler and accept-loop rescue now takes IOError + SystemCallError uniformly, covering EPIPE/ECONNRESET/EBADF alike.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The BC5 OAuth work (#369 discovery, #370 device flow) hardened every fetch with redirect suppression, streaming body caps, and per-request timeouts. Review of #370 surfaced two response shapes that per-read timeouts and body-callback streaming cannot classify or bound, in the two SDKs whose HTTP stacks lack a total-request bound:
on_datafires per body chunk), so a 3xx/non-2xx whose body never arrives was classified as a transport timeout and retried — where SPEC.md §16 requires an immediateapi_error. Bounded and redirect-safe, but a contract deviation, documented in-code during BC5 OAuth: device flow (RFC 8628, 5 SDKs) #370 review.asyncio.wait_for-cancelled worker; Python's discovery — which handles the SSRF-exposed URLs — did not.This PR closes both, before BC5 go-live, so every SDK-built OAuth fetch has exact status-first classification and a true total-request bound. Go, TypeScript, and Kotlin already have both properties natively (response-at-headers APIs + context/abort/plugin deadlines) and are untouched.
What
Ruby —
Fetcher.stream_http, one headers-first primitive for discovery + device flow. Net::HTTP's block form yields at header time:skip_statusclassifies before any body read, and raising out of the block closes the socket undrained. A watchdog thread closes the connection at a monotonic deadline, interrupting even a blocked or byte-dripped header read (max_retries = 0is load-bearing — the idempotent-retry default silently reopens the connection the watchdog just closed). Redirects are structurally never followed. Injected Faraday connections keep the previous verified path; the stall residual is now scoped to injection only.Python —
_transport.request_bounded, the device flow's bounded worker shared with discovery. Theasyncio.wait_for-cancelled, worker-thread-hosted core moves to_transport.py;_post_form_boundeddelegates unchanged and discovery's between-chunks deadline loop (which never bounded the header phase) is replaced by the same total bound.Verification
Real-socket acceptance tests in both languages (mock layers cannot stall or drip — WebMock's patched Net::HTTP buffers even allowed requests, and respx serves instantly):
api_error, exactly one request, zero retriesapi_errorby statusFull suites green: Ruby 750 runs / rubocop clean; Python 177 tests / mypy / ruff.
Sequencing
Stacked on #370 (device flow, converged and merge-held). Intended to land immediately after #370, with no SDK release between them, so the transport contract is uniform from the first device-flow release.
Summary by cubic
Hardened OAuth transport in Ruby and Python with headers-first status handling and true total-request timeouts. Defaults now use the bounded transport; discovery treats status as final and transport/TLS errors map consistently.
New Features
Basecamp::Oauth::Fetcher.stream_http(Net::HTTP) with headers-first classification, redirect suppression, watchdog deadline,skip_status, and a streaming cap; discovery/resource/device flow now default to this transport, keeping injected Faraday clients redirect-verified.basecamp.oauth._transport.request_boundedand used it in discovery and device flow to enforce total timeouts, suppress redirects, and cap streaming viaread_body.Bug Fixes
api_error(Ruby and Python); errors are now status-only on both Ruby paths (default and injected).write_timeoutand mapNet::WriteTimeout/Timeout::Error(andErrno::ETIMEDOUT) toFaraday::TimeoutError; fail-closed validation for invalid/hostless endpoint URLs; TLS handshake failures map toFaraday::SSLError; malformed HTTP/protocol-parse failures map toFaraday::ConnectionFailed; form POSTs default toapplication/x-www-form-urlencoded; explicitly requirenet/http,openssl, anduri;Fetcher.stream_httpfails fast on unsupported methods.request_boundednow fails fast whenparamsis provided on non-POST.Written for commit b3d8f00. Summary will update on new commits.