diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 99b77987..8b86a80b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -70,6 +70,9 @@ jobs: - name: Check service layer drift run: ../scripts/check-service-drift.sh + - name: Check auth-routable consumer invariants + run: ../scripts/check-auth-routable-consumers.sh + - name: Test with coverage run: | # Measure coverage for hand-written code only (generated code is excluded) diff --git a/Makefile b/Makefile index c9ddeb10..99081117 100644 --- a/Makefile +++ b/Makefile @@ -227,6 +227,14 @@ go-check-drift: @echo "==> Checking service layer drift..." @./scripts/check-service-drift.sh +.PHONY: auth-routable-check + +# Check that hop-2-only primitives are not called outside the authenticated +# download path. Guards against regressing the @basecampAuthRoutableUrl contract. +auth-routable-check: + @echo "==> Checking auth-routable consumer invariants..." + @./scripts/check-auth-routable-consumers.sh + #------------------------------------------------------------------------------ # TypeScript SDK targets #------------------------------------------------------------------------------ @@ -575,7 +583,7 @@ tools: #------------------------------------------------------------------------------ # Run all checks (Smithy + Go + TypeScript + Ruby + Kotlin + Swift + Python + Behavior Model + Conformance + Provenance + Actions lint) -check: lint-actions sync-spec-version-check smithy-check behavior-model-check provenance-check sync-api-version-check go-check-drift kt-check-drift go-check ts-check rb-check kt-check swift-check py-check conformance +check: lint-actions sync-spec-version-check smithy-check behavior-model-check provenance-check sync-api-version-check go-check-drift auth-routable-check kt-check-drift go-check ts-check rb-check kt-check swift-check py-check conformance @echo "==> All checks passed" # Clean all build artifacts diff --git a/openapi.json b/openapi.json index 1444563b..7e0383d9 100644 --- a/openapi.json +++ b/openapi.json @@ -21089,7 +21089,8 @@ "format": "int64" }, "download_url": { - "type": "string" + "type": "string", + "x-basecamp-auth-routable-url": {} } } }, @@ -27568,7 +27569,8 @@ "x-go-type-skip-optional-pointer": true }, "download_url": { - "type": "string" + "type": "string", + "x-basecamp-auth-routable-url": {} }, "filename": { "type": "string" diff --git a/scripts/check-auth-routable-consumers.sh b/scripts/check-auth-routable-consumers.sh new file mode 100755 index 00000000..51efdb7b --- /dev/null +++ b/scripts/check-auth-routable-consumers.sh @@ -0,0 +1,66 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Guard: fetchSignedDownload is the hop-2-only primitive for fields tagged +# @basecampAuthRoutableUrl. It must be called only from download.go, and only +# from the single call site inside fetchAPIDownload that performs the +# authenticated hop 1 first. Any other caller is either re-inventing the +# two-hop flow or skipping hop 1. + +# Resolve paths relative to the script location, not the caller's cwd. The +# CI test-go job invokes this from `working-directory: go` as +# `../scripts/check-auth-routable-consumers.sh`; without this, `git grep` +# pathspecs would resolve under `go/go/...` (silent no-op) and the literal +# path to download.go would not exist. Same pattern as check-service-drift.sh. +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_DIR="$(dirname "$SCRIPT_DIR")" + +# Rule 1: any reference to fetchSignedDownload in non-test Go code outside +# download.go is a violation. Uses `git grep` (only tracked files, clean +# pathspec for excluding tests) and a POSIX word-boundary — `(^|[^[:alnum:]_])` +# rather than `\b` — so the pattern works on any POSIX ERE, not just the +# GNU/BSD extension. The identifier is matched whether it is followed by `(` +# (a direct call) or any other non-word boundary — catches method-value +# captures like `fn := c.fetchSignedDownload` that would bypass a +# `\(`-anchored pattern, plus doc-comment mentions (stale examples warrant +# review). The pathspec scopes the scan to *.go files via globstar so non-Go +# files (url-routes.json, api-provenance.json, etc.) aren't subject to the +# soft-signal heuristic. +EXTERNAL=$(git -C "$REPO_DIR" grep -nE '(^|[^[:alnum:]_])fetchSignedDownload([^[:alnum:]_]|$)' \ + -- ':(glob)go/pkg/basecamp/**/*.go' ':!*_test.go' \ + | grep -v '^go/pkg/basecamp/download\.go:' || true) + +if [ -n "${EXTERNAL}" ]; then + echo "ERROR: fetchSignedDownload reference outside go/pkg/basecamp/download.go" + echo "" + echo "${EXTERNAL}" + echo "" + echo "Consumers of @basecampAuthRoutableUrl fields (e.g., Upload.download_url," + echo "CampfireLineAttachment.download_url) MUST route through the two-hop helper" + echo "Client.fetchAPIDownload (or the public AccountClient.DownloadURL), which" + echo "performs the authenticated first hop before the signed fetch." + echo "" + echo "See spec/basecamp-traits.smithy: basecampAuthRoutableUrl contract." + exit 1 +fi + +# Rule 2: download.go must contain exactly one method-call site of +# fetchSignedDownload — the one inside fetchAPIDownload. The function +# declaration `func (c *Client) fetchSignedDownload(` lacks the leading `.` +# and is excluded by this pattern. A second call site in download.go would +# silently bypass Rule 1's file-level exemption, so this rule surfaces it. +DOWNLOAD_GO="$REPO_DIR/go/pkg/basecamp/download.go" +CALL_SITES=$(grep -cE '\.fetchSignedDownload[[:space:]]*\(' "$DOWNLOAD_GO" || true) + +if [ "${CALL_SITES}" != "1" ]; then + echo "ERROR: download.go has ${CALL_SITES} call site(s) of fetchSignedDownload, expected exactly 1" + echo "" + grep -nE '\.fetchSignedDownload[[:space:]]*\(' "$DOWNLOAD_GO" || true + echo "" + echo "The only legitimate call is inside Client.fetchAPIDownload, after the" + echo "authenticated first hop. If you are adding a genuinely new caller," + echo "update this guard to reflect the new invariant." + exit 1 +fi + +echo "auth-routable consumer check: passed" diff --git a/spec/basecamp-traits.smithy b/spec/basecamp-traits.smithy index 8e3ea8fd..2fecf8dd 100644 --- a/spec/basecamp-traits.smithy +++ b/spec/basecamp-traits.smithy @@ -94,6 +94,68 @@ structure basecampSensitive { redact: Boolean } +/// Marks a URL member whose value must be fetched through the configured +/// API host with the SDK's Bearer credential; the API host 302s to a +/// short-lived signed URL that the consumer follows without auth. +/// +/// Contract for consumers (SDKs and hand-rolled helpers): +/// 1. Rewrite the URL's scheme + host to the configured API base URL, +/// preserving path, query, and fragment. +/// 2. Issue an authenticated GET (Bearer credential + SDK User-Agent) +/// with auto-redirect disabled so the 3xx is captured. +/// 3. On 301/302/303/307/308, read Location, close the first body, +/// and GET the resolved URL with a bare transport — no auth +/// headers, no logging middleware. The signature is the credential. +/// 4. On direct 2xx, stream the first response body as-is. +/// 5. Tests that exercise the download flow MUST assert that BOTH +/// hops fired with the correct auth: capture each request's +/// Authorization header and check Bearer on the metadata fetch +/// and authenticated download hop, none on the signed hop. +/// "No assertion fired" and "assertion fired and passed" are +/// indistinguishable otherwise — both masked the bug behind PR +/// #278. The host of the URL stubbed in metadata may either +/// match the configured API base (mirrors what real fixtures +/// return — e.g. `/{accountId}/blobs/{blob}/download/{filename}` +/// for Upload, `/{accountId}/buckets/{bucketId}/uploads/{id}/download/{filename}` +/// for CampfireLineAttachment) or use a different host like +/// `storage.3.basecamp.com/...` to make the SDK's host-rewrite +/// step visible (the convention in primitive tests like Go's +/// `download_test.go`). Either is acceptable; the auth-hop +/// assertions are what enforce the contract. Schema-shape-only +/// tests (unmarshaling assertions that set `download_url` +/// without exercising transport) are exempt but should still +/// use an API-host-shaped URL for clarity. +/// +/// The two-hop flow is not automatically retried end-to-end: streaming +/// body ownership passes to the caller after hop 2, so a retry would +/// double-consume. The authenticated first hop may be retried internally +/// per the SDK's standard retry policy (see PR #278 in the Go SDK). +/// +/// This trait does NOT apply to pagination `Link: rel=next` URLs, +/// which are fetched as same-origin authed GETs without a redirect +/// step (see Go `Client.followPagination` in `client.go`). +/// +/// Every SDK implements hops 1–4 in a language-native primitive; +/// external (cross-package/application-level) consumers MUST call the +/// public client method, not the raw HTTP client. References: Go +/// `AccountClient.DownloadURL`, TypeScript `BasecampClient.downloadURL` +/// (backed by `createDownloadURL`), Python `AccountClient.download_url` +/// / `AsyncAccountClient.download_url` (backed by `download_sync` / +/// `download_async`), Ruby `AccountClient#download_url`, Swift +/// `AccountClient.downloadURL`, Kotlin `AccountClient.downloadURL`. +/// +/// SDK-internal service code (e.g., Go service methods that already +/// own an `OperationInfo` lifecycle, like `UploadsService.Download`) +/// MUST call the internal two-hop helper directly (Go: +/// `Client.fetchAPIDownload`) rather than the public primitive — +/// otherwise the public primitive's own `OnOperationStart`/`OnOperationEnd` +/// fires nested inside the caller's, creating a double-logged request. +/// +/// Emits x-basecamp-auth-routable-url extension to OpenAPI for SDK code generators. +@trait(selector: "structure > member") +@specificationExtension(as: "x-basecamp-auth-routable-url") +structure basecampAuthRoutableUrl {} + // ============================================================================ // Legacy Traits - Keep for backward compatibility (not emitted to OpenAPI) // ============================================================================ diff --git a/spec/basecamp.smithy b/spec/basecamp.smithy index dd892f11..67f0b341 100644 --- a/spec/basecamp.smithy +++ b/spec/basecamp.smithy @@ -45,6 +45,7 @@ use basecamp.traits#basecampPagination use basecamp.traits#basecampIdempotent use basecamp.traits#basecampMultipart use basecamp.traits#basecampSensitive +use basecamp.traits#basecampAuthRoutableUrl /// Basecamp API @restJson1 @@ -3038,6 +3039,7 @@ structure Upload { byte_size: Long width: Integer height: Integer + @basecampAuthRoutableUrl download_url: String filename: String boosts_count: Integer @@ -3840,6 +3842,7 @@ structure CampfireLineAttachment { filename: String content_type: String byte_size: Long + @basecampAuthRoutableUrl download_url: String } diff --git a/typescript/src/generated/openapi-stripped.json b/typescript/src/generated/openapi-stripped.json index 7cfc526c..409ac073 100644 --- a/typescript/src/generated/openapi-stripped.json +++ b/typescript/src/generated/openapi-stripped.json @@ -18765,7 +18765,8 @@ "format": "int64" }, "download_url": { - "type": "string" + "type": "string", + "x-basecamp-auth-routable-url": {} } } }, @@ -25244,7 +25245,8 @@ "x-go-type-skip-optional-pointer": true }, "download_url": { - "type": "string" + "type": "string", + "x-basecamp-auth-routable-url": {} }, "filename": { "type": "string" diff --git a/typescript/tests/services/uploads.test.ts b/typescript/tests/services/uploads.test.ts index 735f4636..a908b045 100644 --- a/typescript/tests/services/uploads.test.ts +++ b/typescript/tests/services/uploads.test.ts @@ -37,7 +37,7 @@ describe("UploadsService", () => { filename: "report.pdf", content_type: "application/pdf", byte_size: 1024000, - download_url: "https://example.com/download/report.pdf", + download_url: "https://3.basecampapi.com/12345/blobs/abcd/download/report.pdf", status: "active", };