Refuse redirects on the signed download hop in every SDK

MIGRATING.md

@@ -13,6 +13,33 @@ 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 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
+**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 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.
+
 ### `BasecampError.api` gained a fifth associated value (#750)
 
 **Swift only, and it is a compile error** — the one shape of break you cannot

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 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),
+`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 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.
+
 ### 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 |

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 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",
+    "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"]
   }
 ]

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 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 {
@@ -26,13 +28,25 @@ 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
 	if err != nil {
 		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 <bc-attachment> 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 {

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 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
+// 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) {

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 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) {
+                        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}",