diff --git a/MIGRATING.md b/MIGRATING.md index 1b24efeaf..010586e98 100644 --- a/MIGRATING.md +++ b/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 diff --git a/SPEC.md b/SPEC.md index 49f4605c7..ef72671c2 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 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 | diff --git a/conformance/tests/downloads.json b/conformance/tests/downloads.json index c5bb622ce..e33707b1b 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 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"] } ] diff --git a/go/pkg/basecamp/download.go b/go/pkg/basecamp/download.go index 294625be5..d6c4c9cac 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 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,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..7117a2955 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 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) { 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..1b0c59280 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 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}", 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..89d786891 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 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 + 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..836ff9d57 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 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. + """ + 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..903c64986 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 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 + 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..64c1dcace 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 + # 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) 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..d4f79c521 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 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 + 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..1a216579d 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 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) { + 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..d1e3f2d39 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 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 { 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/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()) } } diff --git a/swift/Tests/BasecampTests/DownloadTests.swift b/swift/Tests/BasecampTests/DownloadTests.swift index 0ef0c164b..d08513209 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 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 + // 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..7e01b137d 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 redirect from it is refused below + // rather than followed wherever the storage host points (#805). Node's + // 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 { - 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..15d04d34f 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 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. + 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}/*`, () => {