Refuse redirects on the signed download hop in every SDK - #809
Merged
Conversation
DownloadURL's second hop — the unauthenticated GET of the presigned URL hop 1 named — followed redirects in four SDKs, each by a different stack default: net/http's ten hops in Go, fetch's twenty in TypeScript, an explicit follow_redirects=True in Python, and the redirect-following Transport entry point in Swift. Kotlin reused hop 1's followRedirects = false client and Ruby's Net::HTTP#request never follows, so two SDKs had refused since #178 introduced the path. Every other hop a response could steer already refuses, and this one was the unargued exception (#805). The policy is now stated in SPEC §14 "Hop-2 Redirect Policy" and held by a conformance case: a 3xx from the signed host surfaces as the API error carrying that status, with a message saying the redirect is not followed, and its Location is never dialled. Refusal rather than a hop cap or per-hop validation because a cap bounds loops, not destination, and there is no origin to validate a legitimately cross-origin signed URL against — the only policy the SDK can state is "the host the API named, and nothing that host names in turn". Safe to refuse on the evidence: upstream, hop 2 is a presigned GET against a single-endpoint S3-compatible store (bc3 config/storage.yml, minted by Downloading#respond_with_download_redirect, no direct_download_endpoint), which answers it directly — the region redirects that make "S3 redirects" real are an AWS multi-region artefact this store does not have — and local environments send the body on hop 1. Kotlin and Ruby shipping the refusal with no broken download is the strongest evidence of all. Go sets CheckRedirect: ErrUseLastResponse on the bare client; TypeScript passes redirect: "manual"; Python sets follow_redirects=False and checks "not 2xx" rather than ">= 400", which would have returned a 3xx or 304 as a success with an empty body; Swift routes the hop through the Transport's dataNoRedirect entry point, and the test MockTransport now records which entry point carried each request so the seam is pinned. Kotlin and Ruby gain the explicit refusal and its message; Ruby matches the five redirect statuses rather than Net::HTTPRedirection, which also covers 304. Each SDK pins the refusal with a unit test whose third server must stay undialled, and MIGRATING records the behaviour change.
github-actions
Bot
added
typescript
jeremy
added
breaking
There was a problem hiding this comment.
Pull request overview
Standardizes signed-download redirect refusal across all six SDKs and adds conformance coverage.
Changes:
- Disables hop-2 redirects and returns explicit API errors.
- Adds cross-SDK tests and conformance assertions.
- Documents the policy and migration impact.
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 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
SPEC.md |
Defines the hop-2 redirect policy. |
MIGRATING.md |
Documents the behavioral change. |
conformance/tests/downloads.json |
Adds redirect-refusal conformance coverage. |
go/pkg/basecamp/download.go |
Refuses signed-hop redirects. |
go/pkg/basecamp/download_test.go |
Tests Go redirect refusal. |
typescript/src/download.ts |
Uses manual redirect handling. |
typescript/tests/download.test.ts |
Tests TypeScript refusal. |
python/src/basecamp/download.py |
Disables redirects and rejects non-2xx responses. |
python/tests/test_download.py |
Tests sync and async behavior. |
ruby/lib/basecamp/client.rb |
Explicitly rejects redirect statuses. |
ruby/test/basecamp/download_test.rb |
Tests redirects and 304 handling. |
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt |
Explicitly rejects hop-2 redirects. |
kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/DownloadTest.kt |
Tests Kotlin refusal. |
swift/Sources/Basecamp/Download.swift |
Surfaces signed-hop redirects as errors. |
swift/Sources/Basecamp/HTTP/HTTPClient.swift |
Uses the no-redirect transport path. |
swift/Tests/BasecampTests/DownloadTests.swift |
Tests Swift transport selection. |
swift/Tests/BasecampTests/Support/MockTransport.swift |
Records redirect behavior in tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The prose in SPEC §14, MIGRATING and the conformance case promised the "not followed" error for any 3xx. Every implementation recognises only 301, 302, 303, 307 and 308 — the set hop 1 dispatches on — and a 304 or any other 3xx is the generic non-2xx failure, which the Python and Ruby tests already pin. The algorithm stated the right set; the sentences around it now do too, and the code comments say "a redirect" where the adjacent code names the set.
…t copy URLSessionTransport.dataNoRedirect built a fresh URLSession from the caller's configuration with RedirectBlockingDelegate as its only delegate, so a caller's session delegate — auth challenges, certificate pinning, mTLS — did not apply to hop 1, and once hop 2 moved onto the same entry point it would have silently lost that policy for the storage host too. Block the redirect with a task-level delegate on the caller's own session instead, exactly as data(for:) already does to sanitize credentials: a task delegate shadows only the callbacks it implements, so the session delegate keeps handling everything else.
jeremy
added a commit
that referenced
this pull request
Aug 22, 2026
* origin/main: Police the endpoints a discovered issuer names, not only the issuer (#810) Refuse redirects on the signed download hop in every SDK (#809) Quiet known-noise CodeQL alerts without losing coverage (#807) Judge the advertised OAuth issuer's address, not just its spelling (#804) Let Go's raw GET retry loop see the Retry-After it already parses (#796) Deflake three tests that raced a wall clock, and gate the class that produced two of them (#794) SPEC §6: decide which statuses honour Retry-After, and how each loop composes it (#793) Pin the conformance runners' fixture reads, and give CI a leg that can see them break (#791) Report an anonymous embed the timestamp walk cannot resolve, instead of skipping it (#790) # Conflicts: # go/go.mod
jeremy
added a commit
that referenced
this pull request
Aug 22, 2026
Main moved by four while this PR was in review: #804, #807, #809, #810. Merged in; what each needed here, verified against the tree: - #809 and #810 wrote their own Unreleased entries when they merged (#805, #806) and carry `breaking` -- nothing to add. - #807 is CI-internal -- nothing to add. - #804 carries `breaking` but had no entry: resource-first discovery's second hop now rides the address-policed shared client, refusing special-use-space issuers non-retryably and dropping the caller's transport for that hop. Entry added beside #806's, with the variadic NewDiscoverer compile note and the remedies in policy order. - The trailer's verification commit moves to fa15fc1 and its in-flight list shrinks to what is actually in flight. Codex round 5's guard finding rides along: the pyproject check now parses the [project] table (awk section-scoped exact line) instead of matching a version assignment anywhere in the file. Proven with the literal bypass -- a stale [project].version plus an exact assignment in another table passed the old whole-file grep and is refused now; restore by copy, diff -q clean.
jeremy
added a commit
that referenced
this pull request
Aug 22, 2026
…nt-feed-go-connector * origin/event-feed-foundations: Make the fake transport honour the oversize sentinel it mirrors Police the endpoints a discovered issuer names, not only the issuer (#810) Refuse redirects on the signed download hop in every SDK (#809) Quiet known-noise CodeQL alerts without losing coverage (#807) Judge the advertised OAuth issuer's address, not just its spelling (#804) Let Go's raw GET retry loop see the Retry-After it already parses (#796) Deflake three tests that raced a wall clock, and gate the class that produced two of them (#794) SPEC §6: decide which statuses honour Retry-After, and how each loop composes it (#793) Pin the conformance runners' fixture reads, and give CI a leg that can see them break (#791) Report an anonymous embed the timestamp walk cannot resolve, instead of skipping it (#790)
jeremy
added a commit
that referenced
this pull request
Aug 22, 2026
…ent-feed-conformance-driver * origin/event-feed-go-connector: Event feed: verdicts that lost coin flips, and a driver blind to eras Fix the filters-clone comment that stated the opposite of the code Event feed: a policy reason never echoes what the server wrote Event feed: wakes are not verdicts, and three more places the order was the bug Make the fake transport honour the oversize sentinel it mirrors Police the endpoints a discovered issuer names, not only the issuer (#810) Refuse redirects on the signed download hop in every SDK (#809) Event feed: the oversize verdict survived the seam but not the sanitizer Event feed: close the rendering-cap class, type the oversize rejection Event feed: cancellation could not reach the one exit, and the origin was checked after the mangle Event feed: own the ledger's bytes, cap the dial rendering, test the store fake Quiet known-noise CodeQL alerts without losing coverage (#807) Judge the advertised OAuth issuer's address, not just its spelling (#804) Let Go's raw GET retry loop see the Retry-After it already parses (#796) Deflake three tests that raced a wall clock, and gate the class that produced two of them (#794) SPEC §6: decide which statuses honour Retry-After, and how each loop composes it (#793) Pin the conformance runners' fixture reads, and give CI a leg that can see them break (#791) Report an anonymous embed the timestamp walk cannot resolve, instead of skipping it (#790)
jeremy
added a commit
that referenced
this pull request
Aug 22, 2026
* Make the release guide and guards tell the truth before v0.15.0 MIGRATING.md and the release tooling carried nine catalogued defects; this repairs the seven that live in the tree. The retro-labels on merged PRs and the Unreleased -> v0.15.0 promotion happen at tag time. - Re-file the two post-tag entries out of "# v0.14.0" into "# Unreleased": the #662 absent-expiry changes and the TimelineEventData pointer retype both landed in #703, after the tag. Proof: `git show go/v0.14.0:MIGRATING.md` contains neither heading. - Write the four entries the section was missing: #773 (the merge-safe Go reads return a transport failure verbatim -- errors.As and Retryable results move), #737 (four TS paginated methods now declare the ListResult they always returned), #735 (every generated Swift model has a public init -- recorded as NOT a break: no existing initializer changed shape, the 35 affected models were previously unconstructible so no consumer code exists against them), and the maxPages runtime cap (`1919e77f7`, a bare commit label-generated notes cannot list). - Fix the #650 miscount: `position` is conditional on the wire but was modeled before #723, so it is not one of "the seven" -- two of the seven new keys are conditional, plus `position`. Derived from the v0.14.0 and current Tool schemas and the bc3 partial's own `if`s. - Refresh the #604 table's Kotlin row to agree with the #750 entry and the KDoc it cites: the SerializationException lands in `decodeFailure`, the discriminator; `cause` mirrors it and is explicitly not one. - Rewrite the "# Not in this release" trailer: "Nothing is in flight ... merged at 9a819e4" was 53 commits stale. It now names the verification commit and the actual in-flight set, and dates the historical record below it. - Close the `make release` guard gap: it grepped seven of the ten files scripts/bump-version.sh writes, so a truncated bump could tag with the root package.json, typescript/src/client.ts or python/pyproject.toml constant stale. All three join the guard. Proven by mutating each file and watching `make release` refuse with the new message; restored by copy, verified with diff -q. - Release bodies now say that a change merged without a pull request appears only in MIGRATING.md, since generate_release_notes builds from merged PRs and structurally cannot list bare commits. * Absorb the bot round: exact guards, honest scope, the missing Ruby entry Seven fixes from the Copilot and Codex reviews, all taken: - The pyproject guard is an exact whole-line match (grep -qxF). The old regex left dots unescaped and the end unanchored, so a valid-PEP-440 "0.15+0" passed it and failed only in the Python release workflow, after other SDKs had published -- the exact post-tag failure class this PR exists to close. - typescript/package-lock.json (both SDK-version fields, via jq) and ruby/Gemfile.lock join the lockfile guards; bump-version.sh rewrites both, and neither was checked. All three new/changed guards proven by mutation: each refused with its message and exit 2, restored by copy, diff -q clean. - The trailer no longer claims every count in the guide was measured at 8fcb39a -- v0.13.0's totals state their own 9a819e4 baseline. The claim is scoped to the Unreleased section and the in-flight survey. - The #735 entry tells the two Swift shapes apart: updateGaugeNeedle was callable only as the nil-payload {} that bc3 400s; updateMyPreferences was not callable at all (outer requires the unconstructible payload). - The maxPages entry described sloppy-mode assignment wrong: [[Set]] on an inherited getter-only accessor creates no own property -- the assignment is silently ignored, not shadowed. - Three Ruby bare commits (2f21c9d, 3281530, 4785146) were consumer-visible -- crashes on mailto:/hostless server-supplied URLs became ApiError refusals -- and had entries nowhere. One combined entry records the class and the rescue that stops matching. - The release-body sentence no longer promises MIGRATING.md is a complete record of PR-less commits; it states the mechanism (the generated notes cannot see them) and points at the guide for consumer-visible changes. * Scope the Ruby entry's no-request claim to the rejected target Codex round 2: the request that returned the malformed Link or Location header was necessarily already sent — only the follow-up to the rejected target is prevented. Saying "before anything is sent" misled anyone reasoning about hooks or request counts. * Absorb Codex round 3: getter scope, PATH-spec anchor, test-import claim - The #773 entry scoped the classification change to the four composites; Documents.Get installs markBodyReadFailures itself and Schedules.GetEntry delegates to getEntryWithBody, so direct getter callers see it too. The entry now names the getters and the composites built on them. - Both ruby Gemfile.lock guards anchor to the 4-space PATH-spec line with grep -qxF; the loose match could be satisfied by the version-bearing 2-space entry while the PATH spec stayed stale. Proven: mutating only the PATH-spec line now refuses with exit 2, and a clean tree passes the guard block. - The #735 entry claimed every Swift test file uses @testable import; the generator-only test files import BasecampGenerator plain. Narrowed to every test file that imports the SDK module. * Read the JSON versions as fields, and check both Gemfile.lock records Codex round 4 and Copilot round 2 converged on the same hole: grep -qF over package.json matches a "version" string anywhere in the document, so a stale top-level version passed while any nested metadata field carried the requested one. Proven literally: a crafted package.json with top-level 0.13.9 and a nested 0.14.0 satisfied the old grep and is refused by the new jq field read. typescript/package.json gets the same treatment -- same shape, same class. Copilot also wanted both version-bearing Gemfile.lock records checked, not just the PATH spec: a lockfile whose CHECKSUMS entry lags the PATH spec would pass the anchored guard and fail only post-tag. Both files now check both exact lines; a CHECKSUMS-only staleness is refused with its own message, proven by mutation with restore-by-copy. * Absorb the four main merges and Codex round 5 Main moved by four while this PR was in review: #804, #807, #809, #810. Merged in; what each needed here, verified against the tree: - #809 and #810 wrote their own Unreleased entries when they merged (#805, #806) and carry `breaking` -- nothing to add. - #807 is CI-internal -- nothing to add. - #804 carries `breaking` but had no entry: resource-first discovery's second hop now rides the address-policed shared client, refusing special-use-space issuers non-retryably and dropping the caller's transport for that hop. Entry added beside #806's, with the variadic NewDiscoverer compile note and the remedies in policy order. - The trailer's verification commit moves to fa15fc1 and its in-flight list shrinks to what is actually in flight. Codex round 5's guard finding rides along: the pyproject check now parses the [project] table (awk section-scoped exact line) instead of matching a version assignment anywhere in the file. Proven with the literal bypass -- a stale [project].version plus an exact assignment in another table passed the old whole-file grep and is refused now; restore by copy, diff -q clean. * Guard the heading promotion, and fix two entry misstatements Copilot round 3 caught the release procedure's last unguarded step: the "# Unreleased" -> "# v(VERSION)" promotion was a hand edit nothing enforced, so a tag could ship with its notes still filed as unreleased. scripts/promote-migrating.sh now does the rewrite (exact-line, idempotent, refusing the both-headings and neither-heading states), bump-version.sh calls it as step 11, and make release guards both directions: the promoted heading must exist and "# Unreleased" must not. Proven: release refuses on today's tree; the script promotes a scratch copy correctly, is idempotent, and errors on both degenerate states. Codex round 6's two rides along: - The TS client guard is an exact whole-line match including the semicolon, so a comment carrying the assignment text cannot satisfy it while the real constant lags. - The #804 entry's remedy list dropped the address-class split in transcription: AllowLoopback re-admits loopback and nothing else, and Allow does not pierce the IANA tables -- for RFC 1918 the policy must be built without them, which is the implementation's own documented spelling. The entry now says so, and the #735 entry stops claiming UpdateGaugeNeedleRequest has a required member (its member is optional; the outer init exists because request models always got one). * Watch promote-migrating.sh with the gate that watches its caller Copilot: delegating the promotion to a new script moved release-bump behavior out of the sensitive-change gate's sight -- bump-version.sh is listed in extra-patterns and the new script was not. It is now.
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.
Closes #805.
The finding, corrected
DownloadURL's second hop — the unauthenticated GET of the presigned URL hop 1 named — followed redirects in four SDKs, each by a different stack default:CheckRedirecton the bare client → net/http defaultfetch(resolvedLocation)→redirect: "follow"follow_redirects=True, written outtransport.data(for:), the following entry pointThe issue's table said Ruby had no download path. It does (
Client#download_url), and it has refused hop-2 redirects since #178 introduced the feature, becauseNet::HTTP#requestnever follows. Kotlin has refused since #178 too — it runs hop 2 on hop 1'sfollowRedirects = falseclient. So the fleet was 4 following / 2 refusing, and every other hop a response could steer (hop 1 here, §16 discovery, §23 polls) already refuses.The product call, answered from bc3
The issue deferred the remedy pending "what the fleet's storage backends actually do". From bc3:
S3WithDirectEndpointsServiceagainst a single Pure FlashBlade endpoint (config/storage.yml:https://storage.basecamp.com, path-style, nodirect_download_endpoint, nothing redirecting in front of it).Downloading#respond_with_download_redirectfrom the blob's presigned service URL. A presigned GET on a path-style single endpoint is answered by that endpoint — the region/virtual-host redirects behind "S3 redirects" are an AWS multi-region artefact this store does not have.send_fileon hop 1 (respond_with_download_on_disk); hop 2 never occurs.So: suppress, uniformly (the issue's option 1). Not a cap — a cap bounds loops and resource use, not destination; one redirect to an internal address is under every cap. Not per-hop validation — a signed URL is legitimately cross-origin to the API and the SDK holds no roster of storage hosts, so the only policy it can state is "the host the API named, and nothing that host names in turn". Refusal states exactly that. If the storage tier ever starts redirecting, every SDK fails loudly with the 3xx and a "not followed" message, and the remedy is a spec change argued from the new evidence — not a default that happened to work.
What changed
redirect: manual; new### Hop-2 Redirect Policy [conformance]records the rule, the rejected alternatives, the bc3 evidence, and the failure mode. Appendix D row added.downloads.json"DownloadURL refuses a redirect on the signed second hop" — three scripted responses,requestCount: 2,statusCode: 302,errorMessage: "not followed",requestPath /signed/logo.pngat index -1. It discriminates: Go, TS and Python consumed the third response before this change.CheckRedirect: ErrUseLastResponseon the bare client;isRedirectStatusshared with hop 1's dispatch.fetch(resolvedLocation, { redirect: "manual" }); a browser's opaqueredirect (status 0) falls into the existing!okrefusal.follow_redirects=False, plus a real bug fix — hop 2 checked>= 400, so a 3xx (or a 304) would have returned as a success with an empty body. Now "not 2xx". Pinned by a 304 test.fetchSignedDownloadusestransport.dataNoRedirect(for:). The testMockTransportnow records which entry point carried each request, so the test pins the seam the mock cannot otherwise observe.URLSessionTransport.dataNoRedirectnow blocks the redirect with a task-level delegate on the caller's own session (asdata(for:)already does to sanitize credentials) instead of a one-shot session copy, so a caller's pinning/mTLS/auth-challenge delegate applies on both hops — it was silently dropped on hop 1 before.Net::HTTPRedirection, which also covers 304 (caught in self-review, tested).Verified
Go (
go test ./..., golangci-lint, auth-routable guard), TS (full vitest, oxlint, tsc), Python (ruff, mypy, pytest), Ruby (full suite, rubocop), Kotlin (jvmTest),doc-constants-check,conformance-fixtures-check, and the Go/TS/Ruby/Python/Kotlin conformance runners all passing the new case by name.Not verified locally: Swift. The package does not build on Linux for pre-existing reasons (
CFAbsoluteTimeGetCurrentinBaseService.swift); the Swift source and test changes are reviewed by eye only. CI is the check — please look at that leg first.Filed separately from #804 by design; no dependency either way.
Summary by cubic
Refuses redirects on the signed download hop across all SDKs. Previously Go, TypeScript, Python, and Swift followed hop‑2 redirects; now hop 2 never follows, returns 301/302/303/307/308 as an API error with a “not followed” message, treats any other non‑2xx as failure, and never dials the Location.
net/httpCheckRedirect to refuse; TypeScript usesfetchwithredirect: "manual"; Python setshttpxfollow_redirects=Falseand checks “not 2xx”; Swift routes hop 2 throughURLSession’s no‑redirect path and now blocks redirects via a task‑level delegate on the caller’s session (preserving the session delegate); Kotlin and Ruby surface the explicit refusal message. Each adds a test pinning that the third host is never contacted.Written for commit cc33400. Summary will update on new commits.