From 6453e5f5dbd5da4eedc239bb5e5be67dd2677b37 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 21 Aug 2026 23:19:44 -0700 Subject: [PATCH 1/3] Refuse redirects on the signed download hop in every SDK MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- MIGRATING.md | 26 ++++++++ SPEC.md | 50 ++++++++++++++-- conformance/tests/downloads.json | 20 +++++++ go/pkg/basecamp/download.go | 34 +++++++++-- go/pkg/basecamp/download_test.go | 58 ++++++++++++++++++ .../kotlin/com/basecamp/sdk/Download.kt | 23 +++++++- .../kotlin/com/basecamp/sdk/DownloadTest.kt | 39 ++++++++++++ python/src/basecamp/download.py | 39 ++++++++---- python/tests/test_download.py | 59 +++++++++++++++++++ ruby/lib/basecamp/client.rb | 15 ++++- ruby/test/basecamp/download_test.rb | 41 +++++++++++++ swift/Sources/Basecamp/Download.swift | 14 ++++- swift/Sources/Basecamp/HTTP/HTTPClient.swift | 12 +++- swift/Tests/BasecampTests/DownloadTests.swift | 40 +++++++++++++ .../BasecampTests/Support/MockTransport.swift | 19 ++++-- typescript/src/download.ts | 27 +++++++-- typescript/tests/download.test.ts | 33 +++++++++++ 17 files changed, 512 insertions(+), 37 deletions(-) diff --git a/MIGRATING.md b/MIGRATING.md index 1b24efeaf..06df7f52c 100644 --- a/MIGRATING.md +++ b/MIGRATING.md @@ -13,6 +13,32 @@ what wrong behaviour you get if you ignore one. This file is that half. # Unreleased +### All SDKs: the signed download hop no longer follows redirects (#805) + +`DownloadURL` (`downloadURL`, `download_url`, `UploadsService.Download` and its +siblings) is two hops: an authenticated GET to the API host, which answers with +a 302 to a presigned storage URL, then an unauthenticated GET of that URL. The +first hop never followed redirects — the SDK reads `Location` itself. The +second hop did, in Go (net/http's default, ten hops), TypeScript (`fetch`'s +default, twenty), Python (`follow_redirects=True`, written out) and Swift (the +redirect-following `Transport` entry point). Kotlin and Ruby never did. + +All six now refuse. A 3xx from the storage host surfaces as the SDK's API +error carrying that status — `*basecamp.Error` with `HTTPStatus: 302`, +`BasecampError` with `code: "api_error"` and `httpStatus: 302`, `ApiError` +with `http_status=302`, and so on — with a message saying the redirect is +**not followed**, and the `Location` it named is never dialled. SPEC §14 +"Hop-2 Redirect Policy" states the rule and the evidence behind it: Basecamp's +storage tier answers presigned GETs from a single endpoint, and two SDKs have +shipped a non-following second hop since the download path existed. + +**Wrong behaviour you get if you ignore it:** none against Basecamp. Against +another API host whose storage does redirect — a CDN in front of an object +store, a multi-region bucket answering with a region redirect — downloads that +used to succeed now fail with the 3xx. There is no knob to re-enable +following; the fix is for that host to return the storage URL it actually +serves from. + ### `BasecampError.api` gained a fifth associated value (#750) **Swift only, and it is a compile error** — the one shape of break you cannot diff --git a/SPEC.md b/SPEC.md index 49f4605c7..a923f151a 100644 --- a/SPEC.md +++ b/SPEC.md @@ -1705,7 +1705,10 @@ Where: ### Redirect Handling -`follow_redirects = false` for download flow (§14). Redirect responses are handled explicitly. +`follow_redirects = false` on **both** hops of the download flow (§14). Hop 1's redirect is the +flow's own dispatch — the SDK reads `Location` itself and decides what to do with it — and a +redirect on hop 2 is refused outright (§14 "Hop-2 Redirect Policy"). Redirect responses are +handled explicitly, never by the HTTP stack's default policy. For cross-origin redirects, strip the `Authorization` header to prevent credential leakage. @@ -1735,9 +1738,10 @@ FUNCTION downloadURL(raw_url: String) → DownloadResult f. If response is any other error → ⊥ BasecampError from response, without retry. 4. Hop 2 — Unauthenticated fetch (signed URL): - a. Fetch Location URL with NO auth headers. Hop 2 is NEVER retried and NEVER authenticated — the signed URL is single-purpose and credentials must not leak to the storage host. - b. If not 2xx → ⊥ BasecampError. - c. → DownloadResult from response body. + a. Fetch Location URL with NO auth headers and redirect: manual. Hop 2 is NEVER retried, NEVER authenticated and NEVER redirected — the signed URL is single-purpose, credentials must not leak to the storage host, and the storage host does not get to choose a further destination (Hop-2 Redirect Policy below). + b. If response is a redirect (301, 302, 303, 307, 308) → ⊥ BasecampError api_error carrying that status; its Location is never dialled. + c. If not 2xx → ⊥ BasecampError. + d. → DownloadResult from response body. END ``` @@ -1764,6 +1768,43 @@ Attempt budget per SDK — disabling retry (each SDK's spelling of `enable_retry Python and Ruby carve downloads out of their ungoverned GET taxonomy (which retries 500): the download hop uses the declared `{429, 502, 503, 504}` set, in both directions — the taxonomy neither widens nor vetoes it. `DownloadURL` is deliberately absent from `behavior-model.json`; SDKs pass this policy to their retry primitive directly rather than looking it up by operation. +### Hop-2 Redirect Policy `[conformance]` + +The signed hop follows no redirect. A 3xx from the storage host surfaces as `api_error` carrying +that status, with a message saying the redirect is **not followed** — the substring the conformance +case asserts — and the `Location` it carries is never dialled. The refusal is a property of hop 2's +own HTTP client, not of the dispatch around it: `CheckRedirect: ErrUseLastResponse` (Go), +`redirect: "manual"` (TS), `follow_redirects=False` (httpx), `dataNoRedirect` (Swift's +`Transport`), `followRedirects = false` (Ktor), `Net::HTTP#request` (Ruby, which never follows). +Every other hop in the SDK that a response could steer already refuses redirects or validates +each target — hop 1 here, §16's discovery fetches, §23's polls — and until #805 this hop was the +exception in four SDKs, by four different stack defaults, none of them argued. + +**Why refuse rather than cap or validate.** Hop 2's target is the one URL the API host named, and +that host is operator-configured; what a followed redirect adds is a destination chosen by whoever +answers *that* URL. A hop cap bounds loops and resource use, not destination — one redirect to an +internal address is under every cap. Per-hop validation has nothing to validate against: 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. + +**Why it is safe to refuse.** Upstream, hop 2 is a presigned GET against a single-endpoint +S3-compatible object store (bc3 `config/storage.yml`; the redirect is minted by +`Downloading#respond_with_download_redirect` from the blob's service URL, with no +`direct_download_endpoint` configured). A presigned GET on a path-style single endpoint is answered +by that endpoint — the region and virtual-host redirects that make "S3 redirects" a real phenomenon +are artefacts of AWS's multi-region addressing, which this store does not have — and nothing +redirecting sits in front of it. Local and test environments never reach hop 2 at all: +`respond_with_download_on_disk` sends the body on hop 1. The strongest evidence is empirical, +though: Kotlin (by design, reusing hop 1's `followRedirects = false` client) and Ruby (by +`Net::HTTP`'s default) have refused hop-2 redirects since #178 introduced the download path, with +no download reported broken. + +**What happens if that changes.** Should the storage tier ever start redirecting — a CDN in front of +it, a region move — every SDK fails loudly with the 3xx and the "not followed" message rather than +quietly following somewhere. That is deliberate: the remedy is then a spec change argued from the new +evidence, with a destination policy attached, not a default that happened to work. + ### DownloadResult RECORD ``` @@ -3887,6 +3928,7 @@ what `make doc-constants-check` asserts — not a case-by-case index. | `downloads.json` | DownloadURL does not retry hop 1 on 500 | §14, §7 | | `downloads.json` | DownloadURL honors Retry-After on 429 at the auth'd first hop | §14, §7 | | `downloads.json` | DownloadURL surfaces redirect with no Location | §14 | +| `downloads.json` | DownloadURL refuses a redirect on the signed second hop | §14 | | `network-retry.json` | Network error on a non-idempotent POST is not retried | §7 (Gate 2) | | `network-retry.json` | Network error on an idempotent POST is retried then succeeds | §7 (Gate 2) | | `uploads_download.json` | UploadsDownload delegates through DownloadURL primitive | §14, §18 | diff --git a/conformance/tests/downloads.json b/conformance/tests/downloads.json index c5bb622ce..e709bcbac 100644 --- a/conformance/tests/downloads.json +++ b/conformance/tests/downloads.json @@ -132,5 +132,25 @@ {"type": "errorMessage", "expected": "no Location"} ], "tags": ["download", "redirect", "error"] + }, + { + "name": "DownloadURL refuses a redirect on the signed second hop", + "description": "Hop 2 is a presigned GET against the one host hop 1 named, and follows no redirect (SPEC 14 Hop-2 Redirect Policy): a 3xx from the signed host surfaces as an API error carrying that status, and its Location is never dialled. Exactly two requests — the third scripted response must stay unconsumed, which is what distinguishes refusing the redirect from following it.", + "operation": "DownloadURL", + "method": "GET", + "path": "/999999999/blobs/abcd1234/download/logo.png", + "mockResponses": [ + {"status": 302, "headers": {"Location": "/signed/logo.png"}}, + {"status": 302, "headers": {"Location": "/elsewhere/logo.png"}}, + {"status": 200, "headers": {"Content-Type": "image/png"}, "body": "pixels"} + ], + "assertions": [ + {"type": "requestCount", "expected": 2}, + {"type": "statusCode", "expected": 302}, + {"type": "errorMessage", "expected": "not followed"}, + {"type": "headerAbsent", "path": "Authorization", "index": -1}, + {"type": "requestPath", "expected": "/signed/logo.png", "index": -1} + ], + "tags": ["download", "redirect", "hop-2", "error"] } ] diff --git a/go/pkg/basecamp/download.go b/go/pkg/basecamp/download.go index 294625be5..aa6e2f46c 100644 --- a/go/pkg/basecamp/download.go +++ b/go/pkg/basecamp/download.go @@ -12,7 +12,9 @@ import ( // fetchSignedDownload fetches content from a signed download URL (e.g., S3). // Uses the bare transport (no loggingTransport, no auth headers) and no -// client-level timeout so the caller owns the streaming lifecycle. +// client-level timeout so the caller owns the streaming lifecycle. It follows +// no redirect: the signed URL is the one destination the API host named, and +// a 3xx from it is surfaced, not dialled (SPEC §14 "Hop-2 Redirect Policy"). func (c *Client) fetchSignedDownload(ctx context.Context, downloadURL string) (*http.Response, error) { req, err := http.NewRequestWithContext(ctx, "GET", downloadURL, nil) if err != nil { @@ -26,6 +28,13 @@ func (c *Client) fetchSignedDownload(ctx context.Context, downloadURL string) (* httpClient := &http.Client{ Transport: transport, Timeout: 0, // no client-level timeout — streaming owned by caller + // Same policy as the authenticated hop and every other response- + // steerable hop in the SDK. Without this, net/http's default follows + // up to ten redirects wherever the storage host points, and the caller + // receives the final body as if it were the requested file (#805). + CheckRedirect: func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse + }, } resp, err := httpClient.Do(req) // #nosec G704 -- SDK HTTP client: URL is caller-configured @@ -33,6 +42,11 @@ func (c *Client) fetchSignedDownload(ctx context.Context, downloadURL string) (* return nil, fmt.Errorf("failed to download file: %w", err) } + if isRedirectStatus(resp.StatusCode) { + _ = resp.Body.Close() + return nil, ErrAPI(resp.StatusCode, fmt.Sprintf("redirect %d on the signed download hop is not followed", resp.StatusCode)) + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { _ = resp.Body.Close() return nil, ErrAPI(resp.StatusCode, fmt.Sprintf("download failed with status %d", resp.StatusCode)) @@ -47,7 +61,9 @@ func (c *Client) fetchSignedDownload(ctx context.Context, downloadURL string) (* // authenticated first hop (which typically 302s to a signed download URL), // and unauthenticated second hop to fetch the actual file content. Common // inputs include storage blob URLs from elements and any -// other signed-download URL that routes through the API. +// other signed-download URL that routes through the API. Neither hop follows +// a redirect on its own: hop 1's is the dispatch to hop 2, and a redirect on +// hop 2 is an error (SPEC §14 "Hop-2 Redirect Policy"). // // The caller is responsible for closing the returned Body. func (ac *AccountClient) DownloadURL(ctx context.Context, rawURL string) (result *DownloadResult, err error) { @@ -229,8 +245,7 @@ func (c *Client) fetchAPIDownload(ctx context.Context, rawURL string) (*Download } switch { - case resp.StatusCode == 301 || resp.StatusCode == 302 || resp.StatusCode == 303 || - resp.StatusCode == 307 || resp.StatusCode == 308: + case isRedirectStatus(resp.StatusCode): location := resp.Header.Get("Location") // Drain the redirect body up to MaxErrorBodyBytes before close so the // underlying connection can return to the keep-alive pool for hop 2. @@ -277,6 +292,17 @@ func (c *Client) fetchAPIDownload(ctx context.Context, rawURL string) (*Download } } +// isRedirectStatus reports whether status is one of the redirects the +// download flow dispatches on (SPEC §14 step 3d) — and, on hop 2, refuses. +func isRedirectStatus(status int) bool { + switch status { + case http.StatusMovedPermanently, http.StatusFound, http.StatusSeeOther, + http.StatusTemporaryRedirect, http.StatusPermanentRedirect: + return true + } + return false +} + // filenameFromURL extracts a filename from the last path segment of a URL. // Falls back to "download" if the URL is unparseable or has no path segments. func filenameFromURL(rawURL string) string { diff --git a/go/pkg/basecamp/download_test.go b/go/pkg/basecamp/download_test.go index b27be5b90..05570485f 100644 --- a/go/pkg/basecamp/download_test.go +++ b/go/pkg/basecamp/download_test.go @@ -689,6 +689,64 @@ func TestDownload_SecondLegNoTimeout(t *testing.T) { } } +// The signed hop follows no redirect (SPEC §14 "Hop-2 Redirect Policy"). A +// storage host that answers the presigned GET with a 3xx is surfaced with that +// status, and the Location it names is never dialled — the body a third server +// would have returned must not reach the caller as if it were the file (#805). +// Before CheckRedirect was set on the bare client, net/http followed this +// chain and the caller received "SECRET". +func TestDownload_SecondLegRefusesRedirect(t *testing.T) { + var thirdHits atomic.Int32 + thirdServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + thirdHits.Add(1) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("SECRET")) + })) + defer thirdServer.Close() + + var signedHits atomic.Int32 + signedServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + signedHits.Add(1) + w.Header().Set("Location", thirdServer.URL+"/elsewhere/file.png") + w.WriteHeader(http.StatusFound) + })) + defer signedServer.Close() + + apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Location", signedServer.URL+"/bucket/signed-file.png") + w.WriteHeader(http.StatusFound) + })) + defer apiServer.Close() + + cfg := DefaultConfig() + cfg.BaseURL = apiServer.URL + client := NewClient(cfg, &StaticTokenProvider{Token: "test-token"}, WithTransport(http.DefaultTransport)) + ac := client.ForAccount("12345") + + result, err := ac.DownloadURL(context.Background(), + "https://storage.3.basecamp.com/999/blobs/abc/download/photo.png") + if err == nil { + _ = result.Body.Close() + t.Fatal("expected the signed hop's redirect to be refused, got a result") + } + var apiErr *Error + if !errors.As(err, &apiErr) { + t.Fatalf("expected *Error, got %T: %v", err, err) + } + if apiErr.HTTPStatus != http.StatusFound { + t.Errorf("expected HTTPStatus 302, got %d", apiErr.HTTPStatus) + } + if !strings.Contains(apiErr.Message, "not followed") { + t.Errorf("expected the message to name the refusal, got %q", apiErr.Message) + } + if got := signedHits.Load(); got != 1 { + t.Errorf("expected exactly one request to the signed host, got %d", got) + } + if got := thirdHits.Load(); got != 0 { + t.Errorf("the redirect target was dialled %d time(s); hop 2 must not follow", got) + } +} + // --- Auth-hop retry behavior --- func TestDownloadURL_AuthHopRetriesOn503(t *testing.T) { diff --git a/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt b/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt index 893c0fe2b..2c7750065 100644 --- a/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt +++ b/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt @@ -23,6 +23,9 @@ import kotlinx.coroutines.delay */ private val DOWNLOAD_RETRY_ON = setOf(429, 502, 503, 504) +/** The redirects hop 1 dispatches on (SPEC §14 step 3d) and hop 2 refuses. */ +private val REDIRECT_STATUSES = setOf(301, 302, 303, 307, 308) + /** * Result of downloading file content from a URL. * @@ -83,7 +86,9 @@ fun filenameFromURL(rawURL: String): String { * * Handles the full download flow: URL rewriting to the configured API host, * authenticated first hop (which typically 302s to a signed download URL), - * and unauthenticated second hop to fetch the actual file content. + * and unauthenticated second hop to fetch the actual file content. Neither + * hop follows a redirect on its own: hop 1's is the dispatch to hop 2, and a + * redirect on hop 2 is an error (SPEC §14 "Hop-2 Redirect Policy"). * * The first hop retries under the SPEC §14 policy — network errors plus * {429, 502, 503, 504}, never 500 — with exponential backoff (Retry-After @@ -127,7 +132,9 @@ suspend fun AccountClient.downloadURL(rawURL: String): DownloadResult { val rewrittenURL = rewriteOrigin(rawURL, parent.config.baseUrl) // Create one-shot client with no redirect following, sharing the engine - // and applying the SDK's timeout settings + // and applying the SDK's timeout settings. Both hops run on it: hop 1 + // so the SDK reads the redirect itself, hop 2 so the signed host cannot + // choose a further destination (SPEC §14 "Hop-2 Redirect Policy"). val timeoutMs = parent.config.timeout.inWholeMilliseconds val noRedirectClient = HttpClient(httpClient.httpClient.engine) { followRedirects = false @@ -152,7 +159,7 @@ suspend fun AccountClient.downloadURL(rawURL: String): DownloadResult { val status = response.status.value when { - status in setOf(301, 302, 303, 307, 308) -> { + status in REDIRECT_STATUSES -> { // Redirect — extract Location, proceed to hop 2 val location = response.headers[HttpHeaders.Location] if (location.isNullOrEmpty()) { @@ -180,6 +187,16 @@ suspend fun AccountClient.downloadURL(rawURL: String): DownloadResult { ) } + // The client above does not follow, so a 3xx lands here: the + // signed URL is the one destination the API host named, and + // its Location is never dialled (#805). + if (signedResponse.status.value in REDIRECT_STATUSES) { + throw BasecampException.Api( + "redirect ${signedResponse.status.value} on the signed download hop is not followed", + signedResponse.status.value, + ) + } + if (signedResponse.status.value !in 200..299) { throw BasecampException.Api( "download failed with status ${signedResponse.status.value}", diff --git a/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/DownloadTest.kt b/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/DownloadTest.kt index f16709260..2d3299ff7 100644 --- a/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/DownloadTest.kt +++ b/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/DownloadTest.kt @@ -313,6 +313,45 @@ class DownloadTest { client.close() } + // SPEC §14 "Hop-2 Redirect Policy": the signed URL is the one destination + // the API host named. A 3xx from it surfaces with its status, and the + // Location it names is never dialled (#805). Kotlin has always run hop 2 on + // hop 1's followRedirects = false client; this pins the explicit refusal. + @Test + fun downloadURL_hop2RedirectIsRefusedNotFollowed() = runTest { + var requestCount = 0 + var thirdHits = 0 + val client = mockClient({ request -> + requestCount++ + when (request.url.encodedPath) { + "/elsewhere/file" -> { + thirdHits++ + respond(content = ByteReadChannel("SECRET"), status = HttpStatusCode.OK) + } + "/signed/file" -> respond( + content = ByteReadChannel(""), + status = HttpStatusCode.Found, + headers = headersOf(HttpHeaders.Location to listOf("http://localhost:3000/elsewhere/file")) + ) + else -> respond( + content = ByteReadChannel(""), + status = HttpStatusCode.Found, + headers = headersOf(HttpHeaders.Location to listOf("http://localhost:3000/signed/file")) + ) + } + }) + val account = client.forAccount("12345") + + val e = assertFailsWith { + account.downloadURL("http://localhost:3000/12345/attachments/abc/download/file.txt") + } + assertEquals(302, e.httpStatus) + assertTrue(e.message!!.contains("not followed"), "expected the message to name the refusal, got ${e.message}") + assertEquals(2, requestCount, "the redirect target must never be dialled") + assertEquals(0, thirdHits) + client.close() + } + // -- Auth header tests -- @Test diff --git a/python/src/basecamp/download.py b/python/src/basecamp/download.py index bd1d9df56..847b0f7ce 100644 --- a/python/src/basecamp/download.py +++ b/python/src/basecamp/download.py @@ -8,6 +8,9 @@ from basecamp import _security from basecamp.errors import ApiError, NetworkError, UsageError +# The redirects hop 1 dispatches on (SPEC §14 step 3d) and hop 2 refuses. +_REDIRECT_STATUSES = frozenset({301, 302, 303, 307, 308}) + @dataclass(frozen=True) class DownloadResult: @@ -51,7 +54,7 @@ def download_sync(raw_url: str, *, http_client, config) -> DownloadResult: response = http_client.get_download(rewritten_url) - if response.status_code in {301, 302, 303, 307, 308}: + if response.status_code in _REDIRECT_STATUSES: location = response.headers.get("Location") or response.headers.get("location") if not location: raise ApiError(f"redirect {response.status_code} with no Location header") @@ -82,7 +85,7 @@ async def download_async(raw_url: str, *, http_client, config) -> DownloadResult response = await http_client.get_download(rewritten_url) - if response.status_code in {301, 302, 303, 307, 308}: + if response.status_code in _REDIRECT_STATUSES: location = response.headers.get("Location") or response.headers.get("location") if not location: raise ApiError(f"redirect {response.status_code} with no Location header") @@ -116,25 +119,37 @@ def _validate_url(raw_url: str) -> None: raise UsageError("download URL scheme must be http or https") +def _check_signed(response: httpx.Response) -> httpx.Response: + """Hop-2 dispatch: a redirect is refused, any other non-2xx is the download failing. + + The signed URL is the one destination the API host named; a 3xx from it is + surfaced with its status, never dialled (SPEC §14 "Hop-2 Redirect Policy"). + Checked as "not 2xx" rather than ">= 400" so the 3xx the client no longer + follows cannot pass as a success with an empty body. + """ + status = response.status_code + if status in _REDIRECT_STATUSES: + raise ApiError(f"redirect {status} on the signed download hop is not followed", http_status=status) + if not 200 <= status < 300: + raise ApiError(f"download failed with status {status}", http_status=status) + return response + + def _fetch_signed(url: str, *, timeout: float) -> httpx.Response: - """Unauthenticated GET for signed download URL.""" + """Unauthenticated GET for signed download URL. Follows no redirect (#805).""" try: - with httpx.Client(timeout=timeout, follow_redirects=True) as client: + with httpx.Client(timeout=timeout, follow_redirects=False) as client: response = client.get(url) - if response.status_code >= 400: - raise ApiError(f"download failed with status {response.status_code}", http_status=response.status_code) - return response except httpx.HTTPError as e: raise NetworkError(f"Download failed: {e}") from e + return _check_signed(response) async def _fetch_signed_async(url: str, *, timeout: float) -> httpx.Response: - """Async unauthenticated GET for signed download URL.""" + """Async unauthenticated GET for signed download URL. Follows no redirect (#805).""" try: - async with httpx.AsyncClient(timeout=timeout, follow_redirects=True) as client: + async with httpx.AsyncClient(timeout=timeout, follow_redirects=False) as client: response = await client.get(url) - if response.status_code >= 400: - raise ApiError(f"download failed with status {response.status_code}", http_status=response.status_code) - return response except httpx.HTTPError as e: raise NetworkError(f"Download failed: {e}") from e + return _check_signed(response) diff --git a/python/tests/test_download.py b/python/tests/test_download.py index 55763b102..2091c2ebc 100644 --- a/python/tests/test_download.py +++ b/python/tests/test_download.py @@ -110,6 +110,65 @@ def test_302_follows_to_signed_url(self): assert result.content_type == "application/pdf" assert result.filename == "doc.pdf" + # SPEC §14 "Hop-2 Redirect Policy": the signed URL is the one destination the + # API host named. A 3xx from it surfaces with its status, and the Location it + # names is never dialled (#805). Before hop 2 passed follow_redirects=False, + # httpx followed this chain and the caller received b"SECRET" as the file. + @respx.mock + def test_hop2_redirect_is_refused_not_followed(self): + respx.get("https://3.basecampapi.com/files/doc.pdf").mock( + return_value=httpx.Response(302, headers={"Location": "https://signed.storage.com/doc.pdf?sig=xyz"}) + ) + respx.get("https://signed.storage.com/doc.pdf?sig=xyz").mock( + return_value=httpx.Response(302, headers={"Location": "https://elsewhere.example.com/final/doc.pdf"}) + ) + third = respx.get("https://elsewhere.example.com/final/doc.pdf").mock( + return_value=httpx.Response(200, content=b"SECRET") + ) + + with pytest.raises(ApiError, match="not followed") as excinfo: + download_sync("https://original.com/files/doc.pdf", http_client=make_http(), config=make_config()) + + assert excinfo.value.http_status == 302 + assert not third.called + + @respx.mock + @pytest.mark.asyncio + async def test_hop2_redirect_is_refused_not_followed_async(self): + respx.get("https://3.basecampapi.com/files/doc.pdf").mock( + return_value=httpx.Response(302, headers={"Location": "https://signed.storage.com/doc.pdf?sig=xyz"}) + ) + respx.get("https://signed.storage.com/doc.pdf?sig=xyz").mock( + return_value=httpx.Response(302, headers={"Location": "https://elsewhere.example.com/final/doc.pdf"}) + ) + third = respx.get("https://elsewhere.example.com/final/doc.pdf").mock( + return_value=httpx.Response(200, content=b"SECRET") + ) + config = make_config() + http = AsyncHttpClient(config, AsyncBearerAuth(AsyncStaticTokenProvider("test-token"))) + + with pytest.raises(ApiError, match="not followed") as excinfo: + await download_async("https://original.com/files/doc.pdf", http_client=http, config=config) + + assert excinfo.value.http_status == 302 + assert not third.called + + # Hop 2 used to accept anything below 400, so a 3xx the client no longer + # follows — or a 304, which is outside the redirect set — would have + # returned as a success with an empty body. Any non-2xx is the download + # failing. + @respx.mock + def test_hop2_non_2xx_below_400_is_a_failure(self): + respx.get("https://3.basecampapi.com/files/doc.pdf").mock( + return_value=httpx.Response(302, headers={"Location": "https://signed.storage.com/doc.pdf?sig=xyz"}) + ) + respx.get("https://signed.storage.com/doc.pdf?sig=xyz").mock(return_value=httpx.Response(304)) + + with pytest.raises(ApiError, match="download failed with status 304") as excinfo: + download_sync("https://original.com/files/doc.pdf", http_client=make_http(), config=make_config()) + + assert excinfo.value.http_status == 304 + class TestDirectDownload: @respx.mock diff --git a/ruby/lib/basecamp/client.rb b/ruby/lib/basecamp/client.rb index af90e8b23..beb81ff40 100644 --- a/ruby/lib/basecamp/client.rb +++ b/ruby/lib/basecamp/client.rb @@ -240,7 +240,9 @@ def paginate_wrapped(path, key:, params: {}, operation: nil, max_items: nil) # # Handles the full download flow: URL rewriting to the configured API host, # authenticated first hop (which typically 302s to a signed download URL), - # and unauthenticated second hop to fetch the actual file content. + # and unauthenticated second hop to fetch the actual file content. Neither + # hop follows a redirect on its own: hop 1's is the dispatch to hop 2, and + # a redirect on hop 2 is an error (SPEC §14 "Hop-2 Redirect Policy"). # # @param raw_url [String] absolute download URL (e.g., from bc-attachment elements) # @return [DownloadResult] the download result with body, content_type, content_length, filename @@ -644,11 +646,22 @@ def fetch_signed_download(url) request = Net::HTTP::Get.new(uri) begin + # Net::HTTP#request never follows a redirect, which is the policy: + # the signed URL is the one destination the API host named, and a + # 3xx from it is refused below, not dialled (SPEC §14 "Hop-2 + # Redirect Policy"). Stated here so a move to a following client + # (Faraday, Net::HTTP.get_response's callers) has to argue with it. response = http_client.request(request) rescue StandardError => e raise NetworkError.new("Download failed: #{e.message}", cause: e) end + # The exact set hop 1 dispatches on, not Net::HTTPRedirection — that + # class also covers 304, which is a cache answer, not a redirect. + if [ 301, 302, 303, 307, 308 ].include?(response.code.to_i) + raise ApiError.new("redirect #{response.code} on the signed download hop is not followed", http_status: response.code.to_i) + end + unless response.is_a?(Net::HTTPSuccess) raise ApiError.new("download failed with status #{response.code}", http_status: response.code.to_i) end diff --git a/ruby/test/basecamp/download_test.rb b/ruby/test/basecamp/download_test.rb index 1da4fb086..0b4ce2833 100644 --- a/ruby/test/basecamp/download_test.rb +++ b/ruby/test/basecamp/download_test.rb @@ -251,6 +251,47 @@ def test_download_url_s3_error end end + # SPEC §14 "Hop-2 Redirect Policy": the signed URL is the one destination the + # API host named. A 3xx from it surfaces with its status, and the Location it + # names is never dialled (#805). Net::HTTP#request never follows, so this pins + # the explicit refusal rather than an accident of the client. + def test_download_url_hop2_redirect_is_refused_not_followed + stub_request(:get, "#{base_url}/12345/attachments/abc/download/file.txt") + .with(headers: { "Authorization" => "Bearer #{access_token}" }) + .to_return(status: 302, headers: { "Location" => "https://s3.amazonaws.com/bucket/file" }) + + stub_request(:get, "https://s3.amazonaws.com/bucket/file") + .to_return(status: 302, headers: { "Location" => "https://elsewhere.example.com/final/file" }) + + stub_request(:get, "https://elsewhere.example.com/final/file") + .to_return(status: 200, body: "SECRET") + + error = assert_raises(Basecamp::ApiError) do + @account.download_url("https://3.basecampapi.com/12345/attachments/abc/download/file.txt") + end + + assert_equal 302, error.http_status + assert_match(/not followed/, error.message) + assert_not_requested(:get, "https://elsewhere.example.com/final/file") + end + + # 304 is Net::HTTPRedirection in net/http's class tree but not a redirect the + # flow dispatches on: it is the download failing, not a refused redirect. + def test_download_url_hop2_304_is_a_failure_not_a_refused_redirect + stub_request(:get, "#{base_url}/12345/attachments/abc/download/file.txt") + .with(headers: { "Authorization" => "Bearer #{access_token}" }) + .to_return(status: 302, headers: { "Location" => "https://s3.amazonaws.com/bucket/file" }) + + stub_request(:get, "https://s3.amazonaws.com/bucket/file").to_return(status: 304) + + error = assert_raises(Basecamp::ApiError) do + @account.download_url("https://3.basecampapi.com/12345/attachments/abc/download/file.txt") + end + + assert_equal 304, error.http_status + assert_match(/download failed with status 304/, error.message) + end + # -- Auth header tests -- def test_download_url_auth_on_api_not_on_s3 diff --git a/swift/Sources/Basecamp/Download.swift b/swift/Sources/Basecamp/Download.swift index 960b25671..afea8ba9c 100644 --- a/swift/Sources/Basecamp/Download.swift +++ b/swift/Sources/Basecamp/Download.swift @@ -43,7 +43,9 @@ extension AccountClient { /// /// Handles the full download flow: URL rewriting to the configured API host, /// authenticated first hop (which typically 302s to a signed download URL), - /// and unauthenticated second hop to fetch the actual file content. + /// and unauthenticated second hop to fetch the actual file content. Neither + /// hop follows a redirect on its own: hop 1's is the dispatch to hop 2, and + /// a redirect on hop 2 is an error (SPEC §14 "Hop-2 Redirect Policy"). /// /// The first hop retries under the SPEC §14 policy — network errors plus /// {429, 502, 503, 504}, never 500 — with exponential backoff, honoring @@ -111,6 +113,16 @@ extension AccountClient { // Hop 2: fetch from signed URL (no auth, no hooks) let (signedData, signedResponse) = try await httpClient.fetchSignedDownload(url: resolvedLocation) + // The transport's no-redirect entry point hands a 3xx back: the + // signed URL is the one destination the API host named, and its + // Location is never dialled (#805). + if [301, 302, 303, 307, 308].contains(signedResponse.statusCode) { + throw BasecampError.api( + message: "redirect \(signedResponse.statusCode) on the signed download hop is not followed", + httpStatus: signedResponse.statusCode, hint: nil, requestId: nil, decodeFailure: nil + ) + } + guard signedResponse.statusCode >= 200 && signedResponse.statusCode < 300 else { throw BasecampError.api( message: "download failed with status \(signedResponse.statusCode)", diff --git a/swift/Sources/Basecamp/HTTP/HTTPClient.swift b/swift/Sources/Basecamp/HTTP/HTTPClient.swift index cab8154a1..8603231d8 100644 --- a/swift/Sources/Basecamp/HTTP/HTTPClient.swift +++ b/swift/Sources/Basecamp/HTTP/HTTPClient.swift @@ -495,8 +495,10 @@ package final class HTTPClient: Sendable { throw BasecampError.network(message: "Download failed after \(maxAttempts) attempts", cause: nil) } - /// Unauthenticated GET via bare transport. No hooks. - /// Used by downloadURL for the signed-URL hop. + /// Unauthenticated GET via bare transport. No hooks, and no redirect + /// following: the signed URL is the one destination the API host named, + /// and a 3xx from it is handed back for `downloadURL` to refuse (SPEC §14 + /// "Hop-2 Redirect Policy"). Used by downloadURL for the signed-URL hop. package func fetchSignedDownload(url: String) async throws -> (Data, HTTPURLResponse) { guard let requestURL = URL(string: url) else { throw BasecampError.usage(message: "Invalid URL: \(url)", hint: nil) @@ -507,7 +509,11 @@ package final class HTTPClient: Sendable { request.timeoutInterval = config.timeoutInterval do { - let (data, response) = try await transport.data(for: request) + // The no-redirect entry point, as on hop 1. `data(for:)` would follow + // wherever the storage host pointed — stripping credentials on a + // cross-origin hop, but still delivering the final body as if it + // were the requested file (#805). + let (data, response) = try await transport.dataNoRedirect(for: request) guard let httpResponse = response as? HTTPURLResponse else { // Neither of these two helpers retries, so the guard can surface diff --git a/swift/Tests/BasecampTests/DownloadTests.swift b/swift/Tests/BasecampTests/DownloadTests.swift index 0ef0c164b..422b52428 100644 --- a/swift/Tests/BasecampTests/DownloadTests.swift +++ b/swift/Tests/BasecampTests/DownloadTests.swift @@ -300,6 +300,46 @@ final class DownloadTests: XCTestCase { } } + // SPEC §14 "Hop-2 Redirect Policy": the signed URL is the one destination + // the API host named. A 3xx from it surfaces with its status, and the + // Location it names is never dialled (#805). The mock cannot follow a + // redirect itself, so the last assertion pins the seam that guarantees + // that in production: both hops go through the transport's no-redirect + // entry point, where `data(for:)` would have followed the chain. + func testDownloadURL_hop2RedirectIsRefusedNotFollowed() async throws { + let counter = Counter() + let transport = MockTransport { request in + let count = counter.increment() + let location = count == 1 + ? "https://s3.amazonaws.com/bucket/file" + : "https://elsewhere.example.com/final/file" + return ( + Data(), + makeHTTPResponse( + url: request.url!.absoluteString, + statusCode: 302, + headers: ["Location": location] + ) + ) + } + let account = makeTestAccountClient(transport: transport) + + do { + _ = try await account.downloadURL("https://3.basecampapi.com/999999999/attachments/abc/download/file.txt") + XCTFail("Expected the signed hop's redirect to be refused") + } catch let error as BasecampError { + guard case .api(let message, let httpStatus, _, _, _) = error else { + XCTFail("Expected api error, got \(error)") + return + } + XCTAssertEqual(httpStatus, 302) + XCTAssertTrue(message.contains("not followed"), "expected the message to name the refusal, got \(message)") + } + + XCTAssertEqual(transport.requests.count, 2, "the redirect target must never be dialled") + XCTAssertEqual(transport.requests.map(\.followsRedirects), [false, false]) + } + // MARK: - Auth Header Tests func testDownloadURL_authOnApiNotOnS3() async throws { diff --git a/swift/Tests/BasecampTests/Support/MockTransport.swift b/swift/Tests/BasecampTests/Support/MockTransport.swift index 186716ae3..44a67d138 100644 --- a/swift/Tests/BasecampTests/Support/MockTransport.swift +++ b/swift/Tests/BasecampTests/Support/MockTransport.swift @@ -18,6 +18,11 @@ import Foundation final class MockTransport: Transport, @unchecked Sendable { struct RecordedRequest: Sendable { let request: URLRequest + /// Which `Transport` entry point carried it: false for + /// `dataNoRedirect(for:)`, the one both download hops must use. The + /// mock cannot follow a redirect itself, so this is how a test pins + /// that production will not either. + let followsRedirects: Bool } private let handler: @Sendable (URLRequest) async throws -> (Data, URLResponse) @@ -56,14 +61,18 @@ final class MockTransport: Transport, @unchecked Sendable { } func data(for request: URLRequest) async throws -> (Data, URLResponse) { - lock.withLock { - _requests.append(RecordedRequest(request: request)) - } - return try await handler(request) + try await serve(request, followsRedirects: true) } func dataNoRedirect(for request: URLRequest) async throws -> (Data, URLResponse) { - try await data(for: request) + try await serve(request, followsRedirects: false) + } + + private func serve(_ request: URLRequest, followsRedirects: Bool) async throws -> (Data, URLResponse) { + lock.withLock { + _requests.append(RecordedRequest(request: request, followsRedirects: followsRedirects)) + } + return try await handler(request) } /// Resets the recorded requests. diff --git a/typescript/src/download.ts b/typescript/src/download.ts index a29436bbc..7124e4962 100644 --- a/typescript/src/download.ts +++ b/typescript/src/download.ts @@ -21,6 +21,9 @@ const DOWNLOAD_MAX_ATTEMPTS = 3; const DOWNLOAD_RETRY_ON = [429, 502, 503, 504]; const DOWNLOAD_RETRY_BASE_DELAY_MS = 1000; +/** The redirects hop 1 dispatches on (SPEC §14 step 3d) and hop 2 refuses. */ +const REDIRECT_STATUSES = [301, 302, 303, 307, 308]; + /** * Result of downloading file content from a URL. */ @@ -87,7 +90,9 @@ interface DownloadDeps { * authenticated first hop (which typically 302s to a signed download URL), * and unauthenticated second hop to fetch the actual file content. Common * inputs include storage blob URLs from elements and any - * other signed-download URL that routes through the API. + * other signed-download URL that routes through the API. Neither hop follows + * a redirect on its own: hop 1's is the dispatch to hop 2, and a redirect on + * hop 2 is an error (SPEC §14 "Hop-2 Redirect Policy"). */ export function createDownloadURL(deps: DownloadDeps): (rawURL: string) => Promise { const { authStrategy, userAgent, baseUrl, hooks, requestTimeoutMs, enableRetry, retryBaseDelayMs } = deps; @@ -222,7 +227,7 @@ export function createDownloadURL(deps: DownloadDeps): (rawURL: string) => Promi emit.finalize({ statusCode: response.status }); // Dispatch on response status - const isRedirect = [301, 302, 303, 307, 308].includes(response.status); + const isRedirect = REDIRECT_STATUSES.includes(response.status); if (isRedirect) { // Redirect — extract Location, cancel body, proceed to hop 2 const location = response.headers.get("Location"); @@ -236,15 +241,29 @@ export function createDownloadURL(deps: DownloadDeps): (rawURL: string) => Promi // Resolve relative Location against the rewritten API URL const resolvedLocation = new URL(location, rewrittenURL).href; - // Hop 2: fetch from signed URL (no auth, no timeout, no request hooks) + // Hop 2: fetch from signed URL (no auth, no timeout, no request hooks). + // `redirect: "manual"`, as on hop 1: the signed URL is the one + // destination the API host named, and a 3xx from it is refused below + // rather than followed wherever the storage host points (#805). Node's + // fetch hands the 3xx back with its status; a browser's yields an + // opaqueredirect (status 0), which the !ok branch refuses the same way. let signedResponse: Response; try { - signedResponse = await fetch(resolvedLocation); + signedResponse = await fetch(resolvedLocation, { redirect: "manual" }); } catch (err) { const error = err instanceof Error ? err : new Error(String(err)); throw Errors.network(error.message, error); } + if (REDIRECT_STATUSES.includes(signedResponse.status)) { + signedResponse.body?.cancel(); + throw new BasecampError( + "api_error", + `redirect ${signedResponse.status} on the signed download hop is not followed`, + { httpStatus: signedResponse.status }, + ); + } + if (!signedResponse.ok) { signedResponse.body?.cancel(); throw new BasecampError( diff --git a/typescript/tests/download.test.ts b/typescript/tests/download.test.ts index 56f65b04f..695eb9a0b 100644 --- a/typescript/tests/download.test.ts +++ b/typescript/tests/download.test.ts @@ -9,6 +9,8 @@ import { BasecampError } from "../src/errors.js"; const BASE_URL = "https://3.basecampapi.com/12345"; const API_ORIGIN = "https://3.basecampapi.com"; const S3_URL = "https://s3.amazonaws.com/bucket/signed-file.png"; +/** Where a redirecting signed host would send hop 2 — and where hop 2 must never go. */ +const THIRD_URL = "https://elsewhere.example.com/final/file.png"; function makeClient(hooks?: BasecampHooks, enableRetry?: boolean) { return createBasecampClient({ @@ -314,6 +316,37 @@ describe("downloadURL", () => { ).rejects.toThrow(BasecampError); }); + it("refuses a redirect on the signed second hop", async () => { + // SPEC §14 "Hop-2 Redirect Policy": the signed URL is the one destination + // the API host named. A 3xx from it surfaces with its status, and the + // Location it names is never dialled (#805). Before hop 2 passed + // `redirect: "manual"`, fetch followed this chain and the caller + // received "SECRET" as the file. + let thirdHits = 0; + server.use( + http.get(`${API_ORIGIN}/*`, () => { + return new HttpResponse(null, { status: 302, headers: { Location: S3_URL } }); + }), + http.get(S3_URL, () => { + return new HttpResponse(null, { status: 302, headers: { Location: THIRD_URL } }); + }), + http.get(THIRD_URL, () => { + thirdHits += 1; + return HttpResponse.text("SECRET"); + }), + ); + + const client = makeClient(); + await expect( + client.downloadURL("https://storage.3.basecamp.com/999/blobs/abc/download/file.png"), + ).rejects.toMatchObject({ + code: "api_error", + httpStatus: 302, + message: expect.stringContaining("not followed"), + }); + expect(thirdHits).toBe(0); + }); + it("handles signed-download network failure after successful redirect", async () => { server.use( http.get(`${API_ORIGIN}/*`, () => { From a17bf5ef098eef155c52ad52f7730994ba1730d0 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 21 Aug 2026 23:50:37 -0700 Subject: [PATCH 2/3] Say which statuses the signed hop refuses, not "a 3xx" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- MIGRATING.md | 5 +++-- SPEC.md | 4 ++-- conformance/tests/downloads.json | 2 +- go/pkg/basecamp/download.go | 2 +- go/pkg/basecamp/download_test.go | 2 +- .../sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt | 2 +- .../src/commonTest/kotlin/com/basecamp/sdk/DownloadTest.kt | 2 +- python/src/basecamp/download.py | 2 +- python/tests/test_download.py | 2 +- ruby/lib/basecamp/client.rb | 2 +- ruby/test/basecamp/download_test.rb | 2 +- swift/Sources/Basecamp/Download.swift | 2 +- swift/Sources/Basecamp/HTTP/HTTPClient.swift | 2 +- swift/Tests/BasecampTests/DownloadTests.swift | 2 +- typescript/src/download.ts | 4 ++-- typescript/tests/download.test.ts | 2 +- 16 files changed, 20 insertions(+), 19 deletions(-) diff --git a/MIGRATING.md b/MIGRATING.md index 06df7f52c..010586e98 100644 --- a/MIGRATING.md +++ b/MIGRATING.md @@ -23,7 +23,8 @@ second hop did, in Go (net/http's default, ten hops), TypeScript (`fetch`'s default, twenty), Python (`follow_redirects=True`, written out) and Swift (the redirect-following `Transport` entry point). Kotlin and Ruby never did. -All six now refuse. A 3xx from the storage host surfaces as the SDK's API +All six now refuse. A redirect — 301, 302, 303, 307 or 308; any other 3xx +is the generic non-2xx failure — from the storage host surfaces as the SDK's API error carrying that status — `*basecamp.Error` with `HTTPStatus: 302`, `BasecampError` with `code: "api_error"` and `httpStatus: 302`, `ApiError` with `http_status=302`, and so on — with a message saying the redirect is @@ -35,7 +36,7 @@ shipped a non-following second hop since the download path existed. **Wrong behaviour you get if you ignore it:** none against Basecamp. Against another API host whose storage does redirect — a CDN in front of an object store, a multi-region bucket answering with a region redirect — downloads that -used to succeed now fail with the 3xx. There is no knob to re-enable +used to succeed now fail with the redirect's status. There is no knob to re-enable following; the fix is for that host to return the storage URL it actually serves from. diff --git a/SPEC.md b/SPEC.md index a923f151a..ef72671c2 100644 --- a/SPEC.md +++ b/SPEC.md @@ -1770,7 +1770,7 @@ Python and Ruby carve downloads out of their ungoverned GET taxonomy (which retr ### Hop-2 Redirect Policy `[conformance]` -The signed hop follows no redirect. A 3xx from the storage host surfaces as `api_error` carrying +The signed hop follows no redirect. A redirect (301, 302, 303, 307 or 308) from the storage host surfaces as `api_error` carrying that status, with a message saying the redirect is **not followed** — the substring the conformance case asserts — and the `Location` it carries is never dialled. The refusal is a property of hop 2's own HTTP client, not of the dispatch around it: `CheckRedirect: ErrUseLastResponse` (Go), @@ -1801,7 +1801,7 @@ though: Kotlin (by design, reusing hop 1's `followRedirects = false` client) and no download reported broken. **What happens if that changes.** Should the storage tier ever start redirecting — a CDN in front of -it, a region move — every SDK fails loudly with the 3xx and the "not followed" message rather than +it, a region move — every SDK fails loudly with the redirect's status and the "not followed" message rather than quietly following somewhere. That is deliberate: the remedy is then a spec change argued from the new evidence, with a destination policy attached, not a default that happened to work. diff --git a/conformance/tests/downloads.json b/conformance/tests/downloads.json index e709bcbac..e33707b1b 100644 --- a/conformance/tests/downloads.json +++ b/conformance/tests/downloads.json @@ -135,7 +135,7 @@ }, { "name": "DownloadURL refuses a redirect on the signed second hop", - "description": "Hop 2 is a presigned GET against the one host hop 1 named, and follows no redirect (SPEC 14 Hop-2 Redirect Policy): a 3xx from the signed host surfaces as an API error carrying that status, and its Location is never dialled. Exactly two requests — the third scripted response must stay unconsumed, which is what distinguishes refusing the redirect from following it.", + "description": "Hop 2 is a presigned GET against the one host hop 1 named, and follows no redirect (SPEC 14 Hop-2 Redirect Policy): a redirect (301, 302, 303, 307 or 308) from the signed host surfaces as an API error carrying that status, and its Location is never dialled. Exactly two requests — the third scripted response must stay unconsumed, which is what distinguishes refusing the redirect from following it.", "operation": "DownloadURL", "method": "GET", "path": "/999999999/blobs/abcd1234/download/logo.png", diff --git a/go/pkg/basecamp/download.go b/go/pkg/basecamp/download.go index aa6e2f46c..d6c4c9cac 100644 --- a/go/pkg/basecamp/download.go +++ b/go/pkg/basecamp/download.go @@ -14,7 +14,7 @@ import ( // Uses the bare transport (no loggingTransport, no auth headers) and no // client-level timeout so the caller owns the streaming lifecycle. It follows // no redirect: the signed URL is the one destination the API host named, and -// a 3xx from it is surfaced, not dialled (SPEC §14 "Hop-2 Redirect Policy"). +// a redirect from it is surfaced, not dialled (SPEC §14 "Hop-2 Redirect Policy"). func (c *Client) fetchSignedDownload(ctx context.Context, downloadURL string) (*http.Response, error) { req, err := http.NewRequestWithContext(ctx, "GET", downloadURL, nil) if err != nil { diff --git a/go/pkg/basecamp/download_test.go b/go/pkg/basecamp/download_test.go index 05570485f..7117a2955 100644 --- a/go/pkg/basecamp/download_test.go +++ b/go/pkg/basecamp/download_test.go @@ -690,7 +690,7 @@ func TestDownload_SecondLegNoTimeout(t *testing.T) { } // The signed hop follows no redirect (SPEC §14 "Hop-2 Redirect Policy"). A -// storage host that answers the presigned GET with a 3xx is surfaced with that +// storage host that answers the presigned GET with a redirect is surfaced with that // status, and the Location it names is never dialled — the body a third server // would have returned must not reach the caller as if it were the file (#805). // Before CheckRedirect was set on the bare client, net/http followed this diff --git a/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt b/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt index 2c7750065..1b0c59280 100644 --- a/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt +++ b/kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt @@ -187,7 +187,7 @@ suspend fun AccountClient.downloadURL(rawURL: String): DownloadResult { ) } - // The client above does not follow, so a 3xx lands here: the + // The client above does not follow, so a redirect lands here: the // signed URL is the one destination the API host named, and // its Location is never dialled (#805). if (signedResponse.status.value in REDIRECT_STATUSES) { diff --git a/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/DownloadTest.kt b/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/DownloadTest.kt index 2d3299ff7..89d786891 100644 --- a/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/DownloadTest.kt +++ b/kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/DownloadTest.kt @@ -314,7 +314,7 @@ class DownloadTest { } // SPEC §14 "Hop-2 Redirect Policy": the signed URL is the one destination - // the API host named. A 3xx from it surfaces with its status, and the + // the API host named. A redirect from it surfaces with its status, and the // Location it names is never dialled (#805). Kotlin has always run hop 2 on // hop 1's followRedirects = false client; this pins the explicit refusal. @Test diff --git a/python/src/basecamp/download.py b/python/src/basecamp/download.py index 847b0f7ce..836ff9d57 100644 --- a/python/src/basecamp/download.py +++ b/python/src/basecamp/download.py @@ -122,7 +122,7 @@ def _validate_url(raw_url: str) -> None: def _check_signed(response: httpx.Response) -> httpx.Response: """Hop-2 dispatch: a redirect is refused, any other non-2xx is the download failing. - The signed URL is the one destination the API host named; a 3xx from it is + The signed URL is the one destination the API host named; a redirect from it is surfaced with its status, never dialled (SPEC §14 "Hop-2 Redirect Policy"). Checked as "not 2xx" rather than ">= 400" so the 3xx the client no longer follows cannot pass as a success with an empty body. diff --git a/python/tests/test_download.py b/python/tests/test_download.py index 2091c2ebc..903c64986 100644 --- a/python/tests/test_download.py +++ b/python/tests/test_download.py @@ -111,7 +111,7 @@ def test_302_follows_to_signed_url(self): assert result.filename == "doc.pdf" # SPEC §14 "Hop-2 Redirect Policy": the signed URL is the one destination the - # API host named. A 3xx from it surfaces with its status, and the Location it + # API host named. A redirect from it surfaces with its status, and the Location it # names is never dialled (#805). Before hop 2 passed follow_redirects=False, # httpx followed this chain and the caller received b"SECRET" as the file. @respx.mock diff --git a/ruby/lib/basecamp/client.rb b/ruby/lib/basecamp/client.rb index beb81ff40..64c1dcace 100644 --- a/ruby/lib/basecamp/client.rb +++ b/ruby/lib/basecamp/client.rb @@ -648,7 +648,7 @@ def fetch_signed_download(url) begin # Net::HTTP#request never follows a redirect, which is the policy: # the signed URL is the one destination the API host named, and a - # 3xx from it is refused below, not dialled (SPEC §14 "Hop-2 + # redirect from it is refused below, not dialled (SPEC §14 "Hop-2 # Redirect Policy"). Stated here so a move to a following client # (Faraday, Net::HTTP.get_response's callers) has to argue with it. response = http_client.request(request) diff --git a/ruby/test/basecamp/download_test.rb b/ruby/test/basecamp/download_test.rb index 0b4ce2833..d4f79c521 100644 --- a/ruby/test/basecamp/download_test.rb +++ b/ruby/test/basecamp/download_test.rb @@ -252,7 +252,7 @@ def test_download_url_s3_error end # SPEC §14 "Hop-2 Redirect Policy": the signed URL is the one destination the - # API host named. A 3xx from it surfaces with its status, and the Location it + # API host named. A redirect from it surfaces with its status, and the Location it # names is never dialled (#805). Net::HTTP#request never follows, so this pins # the explicit refusal rather than an accident of the client. def test_download_url_hop2_redirect_is_refused_not_followed diff --git a/swift/Sources/Basecamp/Download.swift b/swift/Sources/Basecamp/Download.swift index afea8ba9c..1a216579d 100644 --- a/swift/Sources/Basecamp/Download.swift +++ b/swift/Sources/Basecamp/Download.swift @@ -113,7 +113,7 @@ extension AccountClient { // Hop 2: fetch from signed URL (no auth, no hooks) let (signedData, signedResponse) = try await httpClient.fetchSignedDownload(url: resolvedLocation) - // The transport's no-redirect entry point hands a 3xx back: the + // The transport's no-redirect entry point hands a redirect back: the // signed URL is the one destination the API host named, and its // Location is never dialled (#805). if [301, 302, 303, 307, 308].contains(signedResponse.statusCode) { diff --git a/swift/Sources/Basecamp/HTTP/HTTPClient.swift b/swift/Sources/Basecamp/HTTP/HTTPClient.swift index 8603231d8..d1e3f2d39 100644 --- a/swift/Sources/Basecamp/HTTP/HTTPClient.swift +++ b/swift/Sources/Basecamp/HTTP/HTTPClient.swift @@ -497,7 +497,7 @@ package final class HTTPClient: Sendable { /// Unauthenticated GET via bare transport. No hooks, and no redirect /// following: the signed URL is the one destination the API host named, - /// and a 3xx from it is handed back for `downloadURL` to refuse (SPEC §14 + /// and a redirect from it is handed back for `downloadURL` to refuse (SPEC §14 /// "Hop-2 Redirect Policy"). Used by downloadURL for the signed-URL hop. package func fetchSignedDownload(url: String) async throws -> (Data, HTTPURLResponse) { guard let requestURL = URL(string: url) else { diff --git a/swift/Tests/BasecampTests/DownloadTests.swift b/swift/Tests/BasecampTests/DownloadTests.swift index 422b52428..d08513209 100644 --- a/swift/Tests/BasecampTests/DownloadTests.swift +++ b/swift/Tests/BasecampTests/DownloadTests.swift @@ -301,7 +301,7 @@ final class DownloadTests: XCTestCase { } // SPEC §14 "Hop-2 Redirect Policy": the signed URL is the one destination - // the API host named. A 3xx from it surfaces with its status, and the + // the API host named. A redirect from it surfaces with its status, and the // Location it names is never dialled (#805). The mock cannot follow a // redirect itself, so the last assertion pins the seam that guarantees // that in production: both hops go through the transport's no-redirect diff --git a/typescript/src/download.ts b/typescript/src/download.ts index 7124e4962..7e01b137d 100644 --- a/typescript/src/download.ts +++ b/typescript/src/download.ts @@ -243,9 +243,9 @@ export function createDownloadURL(deps: DownloadDeps): (rawURL: string) => Promi // Hop 2: fetch from signed URL (no auth, no timeout, no request hooks). // `redirect: "manual"`, as on hop 1: the signed URL is the one - // destination the API host named, and a 3xx from it is refused below + // destination the API host named, and a redirect from it is refused below // rather than followed wherever the storage host points (#805). Node's - // fetch hands the 3xx back with its status; a browser's yields an + // fetch hands the redirect back with its status; a browser's yields an // opaqueredirect (status 0), which the !ok branch refuses the same way. let signedResponse: Response; try { diff --git a/typescript/tests/download.test.ts b/typescript/tests/download.test.ts index 695eb9a0b..15d04d34f 100644 --- a/typescript/tests/download.test.ts +++ b/typescript/tests/download.test.ts @@ -318,7 +318,7 @@ describe("downloadURL", () => { it("refuses a redirect on the signed second hop", async () => { // SPEC §14 "Hop-2 Redirect Policy": the signed URL is the one destination - // the API host named. A 3xx from it surfaces with its status, and the + // the API host named. A redirect from it surfaces with its status, and the // Location it names is never dialled (#805). Before hop 2 passed // `redirect: "manual"`, fetch followed this chain and the caller // received "SECRET" as the file. From cc3340056353470611bfb3b273af81cf64edeec1 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 21 Aug 2026 23:51:58 -0700 Subject: [PATCH 3/3] Block Swift download redirects on the caller's session, not a one-shot copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- swift/Sources/Basecamp/HTTP/Transport.swift | 24 ++++++++++----------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/swift/Sources/Basecamp/HTTP/Transport.swift b/swift/Sources/Basecamp/HTTP/Transport.swift index c09d5900c..56bc1c10a 100644 --- a/swift/Sources/Basecamp/HTTP/Transport.swift +++ b/swift/Sources/Basecamp/HTTP/Transport.swift @@ -13,7 +13,9 @@ public protocol Transport: Sendable { /// Loads data for the given request without following redirects. /// - /// Used by `downloadURL` for the first hop where redirect capture is needed. + /// Used by `downloadURL` for both hops: the first, where the SDK reads the + /// redirect itself, and the signed second, where a redirect is refused + /// (SPEC §14 "Hop-2 Redirect Policy"). func dataNoRedirect(for request: URLRequest) async throws -> (Data, URLResponse) } @@ -36,18 +38,14 @@ public struct URLSessionTransport: Transport, Sendable { } public func dataNoRedirect(for request: URLRequest) async throws -> (Data, URLResponse) { - // Create a one-shot session that inherits the caller's configuration (timeouts, - // TLS, proxy) but blocks redirects via a delegate. Custom delegate behavior - // (auth challenges, metrics) from the caller's session is not preserved — - // only configuration is carried over. - let delegate = RedirectBlockingDelegate() - let noRedirectSession = URLSession( - configuration: session.configuration, - delegate: delegate, - delegateQueue: nil - ) - defer { noRedirectSession.finishTasksAndInvalidate() } - return try await noRedirectSession.data(for: request) + // A task-level delegate on the caller's own session, exactly as + // `data(for:)` sanitizes credentials above: it shadows only the redirect + // callback, so the session delegate's auth challenges, certificate + // pinning and metrics still apply. This used to build a one-shot session + // that inherited the caller's configuration but not its delegate, which + // silently dropped a pinning or mTLS policy on hop 1 — and would have on + // hop 2 once it moved onto this entry point (#809). + try await session.data(for: request, delegate: RedirectBlockingDelegate()) } }