From 60855204ba9e59a9e57b03420b50cfe3dac66447 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 22 Aug 2026 19:40:14 -0700 Subject: [PATCH 1/6] Police OAuth egress with per-provenance address enforcement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Manager passed its one general-purpose client to every SDK OAuth entry point, and a caller-supplied client is the caller's, enforcement included — so the SDK's dial-time SSRF address policy (basecamp-sdk#804/#810) was never live in the CLI. OAuth traffic now rides two lazily built lanes whose policies derive from operator configuration: BC5 (cfg.BaseURL; discovery both hops via WithIssuerHTTPClient, device authorization and polling, bc5-typed refreshes) and Launchpad (launchpadURL(); web-flow exchange, launchpad-typed refreshes), each admitting loopback exactly when its own anchor is local, so a localhost Launchpad override cannot grant loopback to a production BC5 flow. Each lane wraps a proxy-aware, fail-closed transport: resolver errors refuse the request outright, an operator opt-out (BASECAMP_OAUTH_USE_PROXY=1) routes only positively-proxied requests through a cloned DefaultTransport pinned to the one construction-time httpproxy snapshot (downgrades logged), and everything else stays on the surfguard-policed direct transport — NO_PROXY exclusions included, so there is no path to unguarded direct egress. Protected mode warns, deduplicated and by effective routing, when a configured proxy is ignored. checkAuthClientRedirect moves to internal/auth with its tests (appctx now passes nil and owns none of this), refreshLocked/exchangeCode stop flattening SDK errors to ErrAPI(0) so surfguard.ErrBlocked and typed statuses survive the CLI boundary, and the SDK pins to v0.15.0 via scripts/bump-sdk.sh (Tool.Name *string fallout included). --- go.mod | 5 +- go.sum | 6 +- internal/appctx/context.go | 32 +- internal/appctx/context_test.go | 35 -- internal/auth/auth.go | 82 +++- internal/auth/auth_test.go | 9 +- internal/auth/client.go | 294 +++++++++++++ internal/auth/client_test.go | 600 +++++++++++++++++++++++++++ internal/commands/tools.go | 6 +- internal/version/sdk-provenance.json | 10 +- 10 files changed, 996 insertions(+), 83 deletions(-) create mode 100644 internal/auth/client.go create mode 100644 internal/auth/client_test.go diff --git a/go.mod b/go.mod index 8b671847..61836337 100644 --- a/go.mod +++ b/go.mod @@ -6,9 +6,10 @@ require ( charm.land/bubbles/v2 v2.2.0 charm.land/bubbletea/v2 v2.0.9 charm.land/lipgloss/v2 v2.0.6 - github.com/basecamp/basecamp-sdk/go v0.14.0 + github.com/basecamp/basecamp-sdk/go v0.15.0 github.com/basecamp/cli v0.2.2-0.20260828230226-767413fc712d github.com/basecamp/mcp v0.0.0-20260828100356-2d6f44b51e9d + github.com/basecamp/surfguard/go v0.1.0 github.com/charmbracelet/bubbles v1.0.0 github.com/charmbracelet/glamour v1.0.0 github.com/charmbracelet/huh v1.0.0 @@ -25,6 +26,7 @@ require ( github.com/yuin/goldmark v1.8.5 github.com/zalando/go-keyring v0.2.8 golang.org/x/mod v0.40.0 + golang.org/x/net v0.57.0 golang.org/x/sys v0.47.0 golang.org/x/text v0.41.0 gopkg.in/yaml.v3 v3.0.1 @@ -133,7 +135,6 @@ require ( go.opentelemetry.io/otel/trace v1.45.0 // indirect go.yaml.in/yaml/v3 v3.0.5 // indirect golang.org/x/crypto v0.55.0 // indirect - golang.org/x/net v0.57.0 // indirect golang.org/x/oauth2 v0.36.0 // indirect golang.org/x/sync v0.22.0 // indirect golang.org/x/term v0.45.0 // indirect diff --git a/go.sum b/go.sum index ab820645..9e1e3e5e 100644 --- a/go.sum +++ b/go.sum @@ -87,12 +87,14 @@ github.com/aymanbagabas/go-udiff v0.4.1 h1:OEIrQ8maEeDBXQDoGCbbTTXYJMYRCRO1fnodZ github.com/aymanbagabas/go-udiff v0.4.1/go.mod h1:0L9PGwj20lrtmEMeyw4WKJ/TMyDtvAoK9bf2u/mNo3w= github.com/aymerick/douceur v0.2.0 h1:Mv+mAeH1Q+n9Fr+oyamOlAkUNPWPlA8PPGR0QAaYuPk= github.com/aymerick/douceur v0.2.0/go.mod h1:wlT5vV2O3h55X9m7iVYN0TBM0NH/MmbLnd30/FjWUq4= -github.com/basecamp/basecamp-sdk/go v0.14.0 h1:Jyzmu3ucqwVe+YNC1zWJBOdPjBaTapyZWX7y0cAvaYo= -github.com/basecamp/basecamp-sdk/go v0.14.0/go.mod h1:BlEtZTW78rOY5geVtKaCiccYT9Jz+QGgPuXLo2pROWk= +github.com/basecamp/basecamp-sdk/go v0.15.0 h1:Yxp3WM7rZ7PDcXrOTc4dI9EFOqwHojpqVJ/zbdXnoVg= +github.com/basecamp/basecamp-sdk/go v0.15.0/go.mod h1:00mgcmi89PlnHnLJNwcJjwryo4W5bYJtA2FoqVgHP54= github.com/basecamp/cli v0.2.2-0.20260828230226-767413fc712d h1:jAzDrCCzDpIwhbFT1xVVs0z2xpXoDEkomHfKB2bUUp8= github.com/basecamp/cli v0.2.2-0.20260828230226-767413fc712d/go.mod h1:iTBTaWvsPEFIcZfkxQHEfISyJ6sZ7036K6bNx0RY3EE= github.com/basecamp/mcp v0.0.0-20260828100356-2d6f44b51e9d h1:zEQVGq1x1nhKMZ2TudFAcSJ32CHT8richI1vQakIKz4= github.com/basecamp/mcp v0.0.0-20260828100356-2d6f44b51e9d/go.mod h1:Ee2c/q1/pg+5T5741PIuA3s6VJMQC7I0XBNXIHIujzA= +github.com/basecamp/surfguard/go v0.1.0 h1:JMo+MZQEOBRqnUylzD0/jS9S42Fp+XKWMG+4LWfu/XE= +github.com/basecamp/surfguard/go v0.1.0/go.mod h1:y5MWhE5S/CZAPzUOS0LoOYGUVxTeWFByFDssE3/l4b4= github.com/blang/semver v3.5.1+incompatible h1:cQNTCjp13qL8KC3Nbxr/y2Bqb63oX6wdnnjpJbkM4JQ= github.com/blang/semver v3.5.1+incompatible/go.mod h1:kRBLl5iJ+tD4TcOOxsy/0fnwebNt5EWlYSAyrTnjyyk= github.com/bmatcuk/doublestar v1.1.1/go.mod h1:UD6OnuiIn0yFxxA2le/rnRU1G4RaI4UvFv1sNto9p6w= diff --git a/internal/appctx/context.go b/internal/appctx/context.go index 7f65992c..bdcbada8 100644 --- a/internal/appctx/context.go +++ b/internal/appctx/context.go @@ -9,7 +9,6 @@ import ( "path/filepath" "strconv" "strings" - "time" "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" @@ -82,34 +81,13 @@ func (a *authAdapter) AccessToken(ctx context.Context) (string, error) { return a.mgr.AccessToken(ctx) } -// checkAuthClientRedirect is the CheckRedirect guard for the auth manager's HTTP -// client (OAuth discovery, token refresh). Refuse to follow redirects for -// non-idempotent requests: RFC 6749 token endpoints don't legitimately -// 3xx-redirect POSTs, and because the exchange/refresh requests set GetBody, Go -// would replay the auth code / refresh_token to the redirect target (only the -// initial endpoint is origin-validated). Idempotent GET/HEAD requests (e.g. -// OAuth discovery) carry no credential body, so they may follow redirects -// normally — blocking those would needlessly fail discovery and force the -// Launchpad fallback. Still cap the hop count so a looping endpoint fails fast -// instead of spinning until the client timeout. -func checkAuthClientRedirect(_ *http.Request, via []*http.Request) error { - if len(via) > 0 && via[0].Method != http.MethodGet && via[0].Method != http.MethodHead { - return http.ErrUseLastResponse - } - if len(via) >= 10 { - return fmt.Errorf("stopped after 10 redirects") - } - return nil -} - // NewApp creates a new App with the given configuration. func NewApp(cfg *config.Config) *App { - // Create HTTP client for auth manager (OAuth discovery, token refresh). - httpClient := &http.Client{ - Timeout: 30 * time.Second, - CheckRedirect: checkAuthClientRedirect, - } - authMgr := auth.NewManager(cfg, httpClient) + // nil client: the auth Manager builds its own per-provenance OAuth + // clients (address-policed, redirect-guarded, 30s timeout) — see + // internal/auth/client.go. Passing a client here would replace them + // wholesale, enforcement included. + authMgr := auth.NewManager(cfg, nil) // Create observability components // Collector always runs to gather stats; hooks control output verbosity diff --git a/internal/appctx/context_test.go b/internal/appctx/context_test.go index 249e5e42..3bbeb372 100644 --- a/internal/appctx/context_test.go +++ b/internal/appctx/context_test.go @@ -8,9 +8,7 @@ import ( "net/http/httptest" "os" "runtime" - "sync/atomic" "testing" - "time" "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/stretchr/testify/assert" @@ -51,39 +49,6 @@ func TestNewAppSetsCombinedUserAgent(t *testing.T) { require.NoError(t, err) } -// TestCheckAuthClientRedirect_StopsLoop verifies the auth client's redirect -// guard caps idempotent (GET) follows at Go's default 10-hop limit. A looping -// endpoint would otherwise spin until the 30s client timeout instead of failing -// fast, since the guard only blocks non-GET/HEAD redirects. -func TestCheckAuthClientRedirect_StopsLoop(t *testing.T) { - var hops atomic.Int32 - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - hops.Add(1) - http.Redirect(w, r, "/", http.StatusFound) - })) - defer srv.Close() - - client := &http.Client{CheckRedirect: checkAuthClientRedirect, Timeout: 5 * time.Second} - req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, srv.URL, nil) - require.NoError(t, err) - resp, err := client.Do(req) - if resp != nil { - _ = resp.Body.Close() - } - require.Error(t, err, "redirect loop must fail rather than hang") - assert.Contains(t, err.Error(), "stopped after 10 redirects") - assert.LessOrEqual(t, hops.Load(), int32(11), "client must give up around the 10-redirect cap") -} - -// TestCheckAuthClientRedirect_BlocksCredentialPOST verifies a non-GET/HEAD -// initial request never follows a redirect: the guard returns ErrUseLastResponse -// so a credential-bearing POST body is not replayed to the redirect target. -func TestCheckAuthClientRedirect_BlocksCredentialPOST(t *testing.T) { - post := &http.Request{Method: http.MethodPost} - err := checkAuthClientRedirect(nil, []*http.Request{post}) - assert.ErrorIs(t, err, http.ErrUseLastResponse) -} - func TestWithAppAndFromContext(t *testing.T) { cfg := &config.Config{} app := NewApp(cfg) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 5bd836ed..6422b9e2 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -63,14 +63,43 @@ const ( // Manager handles OAuth authentication. type Manager struct { - cfg *config.Config - store *Store + cfg *config.Config + store *Store + + // httpClient is the caller-owned injection seam, used by tests to stub + // OAuth traffic. When non-nil it carries EVERY OAuth request: it + // collapses the per-provenance lanes below and bypasses the SDK's + // address enforcement by design (the SDK's "yours, enforcement + // included" contract). Production construction (appctx) passes nil and + // the Manager builds the per-lane policed clients itself. httpClient *http.Client + // Per-provenance OAuth egress lanes (see client.go): BC5 traffic rides a + // client whose address policy derives from cfg.BaseURL, Launchpad + // traffic one derived from launchpadURL(). Built lazily so a malformed + // anchor fails the OAuth operation that needed it, cached for the + // Manager's lifetime. + bc5Lane oauthLane + lpLane oauthLane + + // proxyEnv is the one construction-time proxy-environment snapshot both + // lanes share, built under proxyOnce on first lane use. + proxyOnce sync.Once + proxyEnv *proxyEnvState + + // Warnf receives transport-policy warnings (a proxy ignored for OAuth + // traffic, a malformed opt-out value). Test seam; nil means stderr. + Warnf func(format string, args ...any) + mu sync.Mutex } // NewManager creates a new auth manager. +// +// A nil httpClient is the production configuration: OAuth requests ride +// per-provenance clients that enforce the SDK's address policy at dial time. +// A non-nil httpClient is caller-owned (test-only in this codebase) and +// carries every OAuth request as-is — no address policy is applied on top. func NewManager(cfg *config.Config, httpClient *http.Client) *Manager { return &Manager{ cfg: cfg, @@ -245,7 +274,18 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede } } - exchanger := oauth.NewExchanger(m.httpClient) + // The refresh lane follows the stored credential's provenance: bc5-typed + // credentials refresh against a BC5-discovered endpoint, everything else + // is Launchpad. OAuthType and TokenEndpoint are persisted independently — + // this selects a policy anchor, it does not validate their binding. + laneClient, laneErr := m.launchpadClient() + if creds.OAuthType == oauthTypeBC5 { + laneClient, laneErr = m.bc5Client() + } + if laneErr != nil { + return laneErr + } + exchanger := oauth.NewExchanger(laneClient) req := oauth.RefreshRequest{ TokenEndpoint: tokenEndpoint, @@ -260,7 +300,7 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede token, err := exchanger.Refresh(ctx, req) if err != nil { - return output.ErrAPI(0, fmt.Sprintf("token refresh failed: %v", err)) + return wrapOAuthError("token refresh failed", err) } creds.AccessToken = token.AccessToken @@ -568,9 +608,17 @@ func (m *Manager) loginDevice(ctx context.Context, credKey string, oauthCfg *oau requestedScope = scopeFull } + // The device-authorization POST and the token polling both ride the BC5 + // lane client (the SDK carries them on the one WithDeviceHTTPClient + // client), so both endpoints the discovery document named are judged by + // the policy cfg.BaseURL earned. + deviceClient, err := m.bc5Client() + if err != nil { + return nil, err + } devOpts := make([]oauth.DeviceOption, 0, 2+len(opts.deviceOptions)) devOpts = append(devOpts, - oauth.WithDeviceHTTPClient(m.httpClient), + oauth.WithDeviceHTTPClient(deviceClient), oauth.WithDeviceScope(requestedScope), ) devOpts = append(devOpts, opts.deviceOptions...) @@ -706,7 +754,19 @@ func (m *Manager) discoverOAuth(ctx context.Context, log func(string)) (*discove return nil, err } - discoverer := oauth.NewDiscoverer(m.httpClient) + // Both discovery hops ride the BC5 lane client. Hop 2 (the advertised + // issuer's metadata) would otherwise ride the SDK's internal default + // client, whose policy blocks loopback and knows nothing of the CLI's + // local configuration — a local resource's local advertised issuer would + // be refused right after hop 1 succeeded. The lane client carries the + // per-provenance policy (AllowLoopback iff cfg.BaseURL is local), so + // enforcement is preserved in both modes, and the SDK still adds its own + // redirect suppression around it. + discoveryClient, err := m.bc5Client() + if err != nil { + return nil, err + } + discoverer := oauth.NewDiscoverer(discoveryClient, oauth.WithIssuerHTTPClient(discoveryClient)) res, err := discoverer.DiscoverFromResource(ctx, origin) if err != nil { // Hard selection failure: propagate unchanged. output.AsError at the @@ -933,7 +993,13 @@ func (m *Manager) exchangeCode(ctx context.Context, cfg *oauth.Config, code stri return nil, err } - exchanger := oauth.NewExchanger(m.httpClient) + // The web-flow code exchange is Launchpad-provenance traffic (BC5 logins + // go through the device flow), so it rides the Launchpad lane client. + laneClient, laneErr := m.launchpadClient() + if laneErr != nil { + return nil, laneErr + } + exchanger := oauth.NewExchanger(laneClient) req := oauth.ExchangeRequest{ TokenEndpoint: cfg.TokenEndpoint, @@ -946,7 +1012,7 @@ func (m *Manager) exchangeCode(ctx context.Context, cfg *oauth.Config, code stri token, err := exchanger.Exchange(ctx, req) if err != nil { - return nil, output.ErrAPI(0, fmt.Sprintf("token exchange failed: %v", err)) + return nil, wrapOAuthError("token exchange failed", err) } creds := &Credentials{ diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 4019313e..fb894c8f 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -1362,9 +1362,12 @@ func TestRefreshLocked_LaunchpadSendsClientID(t *testing.T) { assert.Contains(t, body, "client_secret="+launchpadClientSecret) } -// guardedClient mirrors the CheckRedirect guard appctx.NewApp installs on the -// auth manager's HTTP client: non-GET/HEAD redirects are refused so the client -// never replays a credential-bearing POST body to a redirect target. +// guardedClient mirrors the CheckRedirect guard the Manager installs on its +// per-provenance lane clients (checkAuthClientRedirect in client.go): +// non-GET/HEAD redirects are refused so the client never replays a +// credential-bearing POST body to a redirect target. Injected here so these +// tests exercise the guard against live httptest redirects without the lane +// policy refusing the loopback servers. func guardedClient() *http.Client { return &http.Client{ Timeout: 30 * time.Second, diff --git a/internal/auth/client.go b/internal/auth/client.go new file mode 100644 index 00000000..44f5a287 --- /dev/null +++ b/internal/auth/client.go @@ -0,0 +1,294 @@ +package auth + +import ( + "fmt" + "net/http" + "net/url" + "os" + "strings" + "sync" + "time" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp/oauth" + surfguard "github.com/basecamp/surfguard/go" + "golang.org/x/net/http/httpproxy" + + "github.com/basecamp/basecamp-cli/internal/hostutil" + "github.com/basecamp/basecamp-cli/internal/output" +) + +// oauthUseProxyEnv opts OAuth traffic out of the SSRF address policy for +// requests the environment's proxy configuration actually routes to a proxy. +// Only the exact value "1" enables it; anything else is refused with a +// warning rather than guessed at. +const oauthUseProxyEnv = "BASECAMP_OAUTH_USE_PROXY" + +// oauthClientTimeout bounds each OAuth HTTP request on the lane clients — +// the same 30s the pre-lane appctx client carried. +const oauthClientTimeout = 30 * time.Second + +// checkAuthClientRedirect is the CheckRedirect guard for the Manager's OAuth +// lane clients (discovery, device flow, token exchange and refresh). Refuse to +// follow redirects for non-idempotent requests: RFC 6749 token endpoints don't +// legitimately 3xx-redirect POSTs, and because the exchange/refresh requests +// set GetBody, Go would replay the auth code / refresh_token to the redirect +// target (only the initial endpoint is origin-validated). Idempotent GET/HEAD +// requests (e.g. OAuth discovery) carry no credential body, so they may follow +// redirects normally — blocking those would needlessly fail discovery and +// force the Launchpad fallback. Still cap the hop count so a looping endpoint +// fails fast instead of spinning until the client timeout. +func checkAuthClientRedirect(_ *http.Request, via []*http.Request) error { + if len(via) > 0 && via[0].Method != http.MethodGet && via[0].Method != http.MethodHead { + return http.ErrUseLastResponse + } + if len(via) >= 10 { + return fmt.Errorf("stopped after 10 redirects") + } + return nil +} + +// oauthEndpointPolicy derives the address policy for one OAuth egress lane +// from that lane's anchor URL — the operator-configured base of the flows the +// lane carries, not any URL a server response named. Loopback is admitted +// only when the anchor itself is local: a developer pointing the CLI at +// http://3.basecamp.localhost:3001 has trusted that space explicitly, and the +// admission must not leak to the other lane (a localhost Launchpad override +// must not let production BC5 metadata name a loopback token endpoint). +// +// The label names the anchor in errors without echoing its value: anchor URLs +// can arrive via environment overrides, and parse failures can reproduce +// their input. +func oauthEndpointPolicy(anchorURL, label string) (surfguard.Policy, error) { + u, err := url.Parse(anchorURL) + if err != nil || u.Opaque != "" || !u.IsAbs() || u.Hostname() == "" { + return surfguard.Policy{}, output.ErrAuth(fmt.Sprintf("invalid %s: must be an absolute URL with a hostname", label)) + } + policy := oauth.DefaultIssuerPolicy() + // IsLocalhost matches a host[:port] string case-sensitively; DNS names + // are case-insensitive, so lowercase first — the same normalization + // resourceOrigin and isSecureEndpointURL apply. + if hostutil.IsLocalhost(strings.ToLower(u.Host)) { + policy = policy.AllowLoopback() + } + return policy, nil +} + +// proxyEnvState is the Manager's one construction-time snapshot of the proxy +// environment: the resolver both lanes route and warn against, and whether +// the operator opted OAuth traffic out of address enforcement when a proxy +// applies. Snapshotting once keeps routing and warnings from ever +// disagreeing — http.ProxyFromEnvironment caches process-globally on its own +// schedule and could diverge from a second FromEnvironment read. +type proxyEnvState struct { + resolve func(*url.URL) (*url.URL, error) + optOut bool +} + +func (m *Manager) proxyState() *proxyEnvState { + m.proxyOnce.Do(func() { + st := &proxyEnvState{resolve: httpproxy.FromEnvironment().ProxyFunc()} + // Set-but-empty means unset, matching how httpproxy reads its own + // variables: "" is what env-scrubbing tooling writes, not a request. + switch v := os.Getenv(oauthUseProxyEnv); v { + case "": + case "1": + st.optOut = true + default: + // A malformed opt-out is OFF, loudly: silently honoring "yes" + // would disable enforcement on a value nobody defined, and + // silently ignoring it would leave the operator believing they + // opted out. + m.warnf("warning: %s=%q is not understood (only \"1\" enables it); OAuth requests keep the SSRF address policy", oauthUseProxyEnv, v) + } + m.proxyEnv = st + }) + return m.proxyEnv +} + +// warnf routes transport-policy warnings to the Manager's Warnf seam, or +// stderr by default — these fire inside RoundTrip, where no command logger +// is in scope. +func (m *Manager) warnf(format string, args ...any) { + if m.Warnf != nil { + m.Warnf(format, args...) + return + } + fmt.Fprintf(os.Stderr, format+"\n", args...) +} + +// oauthTransport is the per-lane egress RoundTripper for OAuth traffic. It +// owns two sub-transports: the lane's address-policed direct transport +// (surfguard dial-time enforcement; Proxy nil by construction), and — only +// when the operator set BASECAMP_OAUTH_USE_PROXY=1 — a proxying clone of +// http.DefaultTransport. +// +// Routing is per-request and fail-closed: +// +// - the snapshot proxy resolver errors → the request is refused before +// either sub-transport runs. A malformed HTTP_PROXY must not degrade +// into egress the operator asked to route elsewhere; +// - opt-out set and the resolver names a proxy for this URL → the proxied +// sub-transport carries it, with address enforcement off for exactly +// that request (the proxy owns the connection); the downgrade is logged; +// - everything else → the guarded direct transport. A NO_PROXY exclusion +// or absent proxy config therefore stays enforced even in opt-out mode — +// there is no path to unguarded DIRECT egress. +// +// In the default (protected) mode the guarded transport applies +// unconditionally; the resolver is consulted only to warn — deduplicated per +// endpoint — that a configured proxy is being ignored for OAuth traffic. +// Consulting it per actual request URL means the discovered issuer, device, +// polling, and persisted refresh endpoints are each evaluated, not just the +// lane's anchor. +// +// The transports live for the Manager's (process) lifetime and are never +// rebuilt per call; idle connections are bounded by each sub-transport's +// IdleConnTimeout, so a process-lifetime CLI Manager needs no explicit Close. +type oauthTransport struct { + guarded http.RoundTripper + proxied http.RoundTripper + resolve func(*url.URL) (*url.URL, error) + optOut bool + warnf func(format string, args ...any) + + mu sync.Mutex + warned map[string]bool +} + +func (t *oauthTransport) RoundTrip(req *http.Request) (*http.Response, error) { + proxyURL, err := t.resolve(req.URL) + if err != nil { + return nil, fmt.Errorf("refusing OAuth request: proxy configuration error: %w", err) + } + switch { + case proxyURL == nil: + return t.guarded.RoundTrip(req) + case t.optOut: + // Deduplicated per endpoint: the device poll re-POSTs the same URL + // every few seconds, and one downgrade notice per endpoint records + // the decision without drowning the terminal. + t.warnOnce(req.URL, "warning: OAuth request to %s routed through proxy %s WITHOUT the SSRF address policy (%s=1)", + redactedEndpoint(req.URL), proxyURL.Redacted(), oauthUseProxyEnv) + return t.proxied.RoundTrip(req) + default: + t.warnOnce(req.URL, "warning: ignoring proxy %s for OAuth request to %s: the SSRF address policy requires direct egress; set %s=1 to route OAuth through the proxy without address enforcement", + proxyURL.Redacted(), redactedEndpoint(req.URL), oauthUseProxyEnv) + return t.guarded.RoundTrip(req) + } +} + +func (t *oauthTransport) warnOnce(u *url.URL, format string, args ...any) { + key := redactedEndpoint(u) + t.mu.Lock() + seen := t.warned[key] + t.warned[key] = true + t.mu.Unlock() + if !seen { + t.warnf(format, args...) + } +} + +// redactedEndpoint renders a request URL for warnings and dedupe keys without +// its query or userinfo — endpoint paths are diagnostic, query strings can +// carry parameters that don't belong in a terminal. +func redactedEndpoint(u *url.URL) string { + return u.Scheme + "://" + u.Host + u.Path +} + +// proxiedTransport clones http.DefaultTransport (never mutating the global) +// and pins its routing to the SAME construction-time snapshot resolver the +// wrapper consults — http.ProxyFromEnvironment's process-global cache could +// diverge from the snapshot and recreate exactly the unguarded direct +// fallback the wrapper exists to prevent. +func proxiedTransport(resolve func(*url.URL) (*url.URL, error)) *http.Transport { + base, ok := http.DefaultTransport.(*http.Transport) + if !ok { + // http.DefaultTransport is *http.Transport on every supported + // runtime; a replaced global is a programming error, not a request + // failure to limp past. + panic("http.DefaultTransport is not a *http.Transport") + } + t := base.Clone() + t.Proxy = func(r *http.Request) (*url.URL, error) { return resolve(r.URL) } + return t +} + +// oauthLane caches one per-provenance egress client. Lazy and +// error-returning: a malformed anchor (a bad BASECAMP_LAUNCHPAD_URL above +// all) surfaces at the OAuth operation that needed the lane, not at Manager +// construction, so it cannot break unrelated commands. +type oauthLane struct { + once sync.Once + client *http.Client + err error +} + +// bc5Client returns the egress client for BC5-provenance OAuth traffic: +// resource-first discovery (both hops), the device flow's authorization and +// polling POSTs, and refreshes of bc5-typed credentials. Its policy derives +// from cfg.BaseURL, so loopback is admitted exactly when the operator +// configured a local Basecamp. +func (m *Manager) bc5Client() (*http.Client, error) { + if m.httpClient != nil { + return m.httpClient, nil + } + m.bc5Lane.once.Do(func() { + m.bc5Lane.client, m.bc5Lane.err = m.buildLaneClient(m.cfg.BaseURL, "base URL") + }) + return m.bc5Lane.client, m.bc5Lane.err +} + +// launchpadClient returns the egress client for Launchpad-provenance OAuth +// traffic: the web-flow code exchange and refreshes of launchpad-typed +// credentials. Its policy derives from launchpadURL() (environment override +// included), independently of the BC5 lane. +func (m *Manager) launchpadClient() (*http.Client, error) { + if m.httpClient != nil { + return m.httpClient, nil + } + m.lpLane.once.Do(func() { + lpURL, err := m.launchpadURL() + if err != nil { + m.lpLane.err = err + return + } + m.lpLane.client, m.lpLane.err = m.buildLaneClient(lpURL, "Launchpad URL") + }) + return m.lpLane.client, m.lpLane.err +} + +func (m *Manager) buildLaneClient(anchorURL, label string) (*http.Client, error) { + policy, err := oauthEndpointPolicy(anchorURL, label) + if err != nil { + return nil, err + } + st := m.proxyState() + tr := &oauthTransport{ + guarded: policy.RoundTripper(), + resolve: st.resolve, + optOut: st.optOut, + warnf: m.warnf, + warned: map[string]bool{}, + } + if st.optOut { + tr.proxied = proxiedTransport(st.resolve) + } + return &http.Client{ + Timeout: oauthClientTimeout, + CheckRedirect: checkAuthClientRedirect, + Transport: tr, + }, nil +} + +// wrapOAuthError maps an SDK token-request failure into the CLI taxonomy +// without flattening it to a string: the SDK's code, HTTP status, and +// retryability survive (a refused redirect keeps its 3xx; a policy refusal +// stays matchable via errors.Is(err, surfguard.ErrBlocked)), and the original +// error stays on the cause chain for errors.As/errors.Is across the CLI +// boundary. +func wrapOAuthError(op string, err error) error { + e := *output.AsError(err) + e.Message = op + ": " + e.Message + e.Cause = err + return &e +} diff --git a/internal/auth/client_test.go b/internal/auth/client_test.go new file mode 100644 index 00000000..447d6b22 --- /dev/null +++ b/internal/auth/client_test.go @@ -0,0 +1,600 @@ +package auth + +import ( + "context" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "net/netip" + "net/url" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp/oauth" + surfguard "github.com/basecamp/surfguard/go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/output" +) + +// clearProxyEnv empties every proxy variable httpproxy reads so ambient +// developer/CI configuration cannot leak into lane construction. Tests that +// need a proxy set their own values on top. +func clearProxyEnv(t *testing.T) { + t.Helper() + for _, v := range []string{"HTTP_PROXY", "http_proxy", "HTTPS_PROXY", "https_proxy", "NO_PROXY", "no_proxy", "REQUEST_METHOD", oauthUseProxyEnv} { + t.Setenv(v, "") + } +} + +// laneManager builds a Manager with no injected client (the production +// configuration) so the per-provenance lanes are exercised for real. +func laneManager(t *testing.T, baseURL string) *Manager { + t.Helper() + cfg := config.Default() + cfg.BaseURL = baseURL + return &Manager{ + cfg: cfg, + store: newTestStore(t, t.TempDir()), + } +} + +// doGet issues a context-carrying GET on the given client — the lane tests' +// one HTTP verb — keeping the request path identical across cases. +func doGet(t *testing.T, client *http.Client, rawURL string) (*http.Response, error) { + t.Helper() + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, rawURL, nil) + require.NoError(t, err) + return client.Do(req) +} + +// TestCheckAuthClientRedirect_StopsLoop verifies the lane clients' redirect +// guard caps idempotent (GET) follows at Go's default 10-hop limit. A looping +// endpoint would otherwise spin until the 30s client timeout instead of +// failing fast, since the guard only blocks non-GET/HEAD redirects. +func TestCheckAuthClientRedirect_StopsLoop(t *testing.T) { + var hops atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hops.Add(1) + http.Redirect(w, r, "/", http.StatusFound) + })) + defer srv.Close() + + client := &http.Client{CheckRedirect: checkAuthClientRedirect, Timeout: 5 * time.Second} + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, srv.URL, nil) + require.NoError(t, err) + resp, err := client.Do(req) + if resp != nil { + _ = resp.Body.Close() + } + require.Error(t, err, "redirect loop must fail rather than hang") + assert.Contains(t, err.Error(), "stopped after 10 redirects") + assert.LessOrEqual(t, hops.Load(), int32(11), "client must give up around the 10-redirect cap") +} + +// TestCheckAuthClientRedirect_BlocksCredentialPOST verifies a non-GET/HEAD +// initial request never follows a redirect: the guard returns +// ErrUseLastResponse so a credential-bearing POST body is not replayed to the +// redirect target. +func TestCheckAuthClientRedirect_BlocksCredentialPOST(t *testing.T) { + post := &http.Request{Method: http.MethodPost} + err := checkAuthClientRedirect(nil, []*http.Request{post}) + assert.ErrorIs(t, err, http.ErrUseLastResponse) +} + +// TestOAuthEndpointPolicy_LoopbackDerivation drives the policy derivation +// through hostutil's host handling: loopback admission must key off the +// lowercased host of a parsed URL, for every spelling of "local". +func TestOAuthEndpointPolicy_LoopbackDerivation(t *testing.T) { + loopback := netip.MustParseAddr("127.0.0.1") + + cases := []struct { + name string + anchor string + wantErr bool + allowLoopback bool + }{ + {"plain localhost", "http://localhost:3001", false, true}, + {"mixed-case localhost", "http://LocalHost:3001", false, true}, + {"dot-localhost subdomain", "http://3.basecamp.localhost:3001", false, true}, + {"uppercase dot-localhost", "http://3.Basecamp.LOCALHOST:3001", false, true}, + {"IPv4 loopback", "http://127.0.0.1:3000", false, true}, + {"bracketed IPv6 loopback", "http://[::1]:3000", false, true}, + {"userinfo does not confuse host extraction", "http://user:pass@localhost:3001", false, true}, + {"production host", "https://3.basecampapi.com", false, false}, + {"production launchpad", "https://launchpad.37signals.com", false, false}, + {"userinfo on production host", "https://user@3.basecampapi.com", false, false}, + {"malformed URL", "http://[::1", true, false}, + {"relative URL", "/not/absolute", true, false}, + {"empty", "", true, false}, + {"opaque form", "https:foo", true, false}, + {"hostless", "https://", true, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + policy, err := oauthEndpointPolicy(tc.anchor, "base URL") + if tc.wantErr { + require.Error(t, err) + if tc.anchor != "" { + assert.NotContains(t, err.Error(), tc.anchor, "errors must not echo the anchor value") + } + return + } + require.NoError(t, err) + assert.Equal(t, !tc.allowLoopback, policy.Blocked(loopback), + "loopback admission for %q", tc.anchor) + }) + } +} + +// TestBC5Lane_LoopbackFollowsBaseURL proves the derived policy at the CLIENT +// level, not just the helper: a local base URL admits loopback OAuth traffic +// on the BC5 lane, a production base URL refuses it before any connection. +func TestBC5Lane_LoopbackFollowsBaseURL(t *testing.T) { + clearProxyEnv(t) + + var hits atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + hits.Add(1) + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{}`) + })) + defer srv.Close() + + t.Run("local base URL admits loopback", func(t *testing.T) { + m := laneManager(t, srv.URL) + client, err := m.bc5Client() + require.NoError(t, err) + resp, err := doGet(t, client, srv.URL) + require.NoError(t, err) + _ = resp.Body.Close() + assert.Equal(t, int32(1), hits.Load()) + }) + + t.Run("production base URL refuses loopback", func(t *testing.T) { + hits.Store(0) + m := laneManager(t, "https://3.basecampapi.com") + client, err := m.bc5Client() + require.NoError(t, err) + resp, err := doGet(t, client, srv.URL) + if resp != nil { + _ = resp.Body.Close() + } + require.Error(t, err) + assert.ErrorIs(t, err, surfguard.ErrBlocked) + assert.Zero(t, hits.Load(), "the refused target must never be dialed") + }) +} + +// TestMixedProvenance_LaunchpadAllowanceDoesNotLeakToBC5 is the negative test +// for the per-lane split: a localhost BASECAMP_LAUNCHPAD_URL grants loopback +// to the LAUNCHPAD lane only. Production BC5 metadata naming a loopback +// endpoint must still be refused — one shared loopback-enabled client would +// have inherited the unrelated Launchpad allowance. +func TestMixedProvenance_LaunchpadAllowanceDoesNotLeakToBC5(t *testing.T) { + clearProxyEnv(t) + + var hits atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + hits.Add(1) + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{}`) + })) + defer srv.Close() + + t.Setenv("BASECAMP_LAUNCHPAD_URL", srv.URL) + m := laneManager(t, "https://3.basecampapi.com") + + bc5, err := m.bc5Client() + require.NoError(t, err) + resp, err := doGet(t, bc5, srv.URL) + if resp != nil { + _ = resp.Body.Close() + } + require.Error(t, err, "the BC5 lane must not inherit the Launchpad loopback allowance") + assert.ErrorIs(t, err, surfguard.ErrBlocked) + assert.Zero(t, hits.Load()) + + lp, err := m.launchpadClient() + require.NoError(t, err) + resp, err = doGet(t, lp, srv.URL) + require.NoError(t, err, "the Launchpad lane earned loopback from its own anchor") + _ = resp.Body.Close() + assert.Equal(t, int32(1), hits.Load()) +} + +// TestLaunchpadLane_LazyValidation: a malformed Launchpad override surfaces at +// the operation that needs the lane — Manager construction and BC5-lane use +// stay unaffected. +func TestLaunchpadLane_LazyValidation(t *testing.T) { + clearProxyEnv(t) + // Survives launchpadURL()'s scheme gate (not an http:// URL) but is no + // lane anchor: relative, so the policy derivation refuses it. + t.Setenv("BASECAMP_LAUNCHPAD_URL", "not-a-url") + + m := laneManager(t, "https://3.basecampapi.com") + _, err := m.bc5Client() + require.NoError(t, err, "the BC5 lane must not depend on the Launchpad anchor") + + _, err = m.launchpadClient() + require.Error(t, err) + assert.Contains(t, err.Error(), "Launchpad URL") +} + +// countingTransport records round trips so tests can prove a sub-transport +// was, or was never, invoked. +type countingTransport struct { + calls atomic.Int32 + resp func() (*http.Response, error) +} + +func (c *countingTransport) RoundTrip(*http.Request) (*http.Response, error) { + c.calls.Add(1) + if c.resp != nil { + return c.resp() + } + return nil, errors.New("counting transport: no response configured") +} + +// TestOAuthTransport_ResolverErrorFailsClosed: a proxy-resolver error refuses +// the request BEFORE either sub-transport runs. A malformed proxy +// configuration must not degrade into egress — guarded or proxied — that the +// operator asked to route elsewhere. +func TestOAuthTransport_ResolverErrorFailsClosed(t *testing.T) { + guarded := &countingTransport{} + proxied := &countingTransport{} + tr := &oauthTransport{ + guarded: guarded, + proxied: proxied, + resolve: func(*url.URL) (*url.URL, error) { return nil, errors.New("bad proxy config") }, + optOut: true, + warnf: func(string, ...any) {}, + warned: map[string]bool{}, + } + + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, "http://public.example/token", nil) + require.NoError(t, err) + resp, err := tr.RoundTrip(req) + require.Nil(t, resp) + require.Error(t, err) + assert.Contains(t, err.Error(), "proxy configuration error") + assert.Zero(t, guarded.calls.Load(), "guarded sub-transport must not run on a resolver error") + assert.Zero(t, proxied.calls.Load(), "proxied sub-transport must not run on a resolver error") +} + +// TestOAuthTransport_CGIRefusesHTTPProxy: in a CGI environment +// (REQUEST_METHOD set) httpproxy refuses to honor HTTP_PROXY, as an ERROR — +// which our fail-closed branch turns into a refused request rather than +// silent egress. This is the one resolver-error path the real environment +// can produce. +func TestOAuthTransport_CGIRefusesHTTPProxy(t *testing.T) { + clearProxyEnv(t) + t.Setenv("HTTP_PROXY", "http://proxy.corp.example:3128") + t.Setenv("REQUEST_METHOD", "GET") + + var warnings []string + m := laneManager(t, "https://3.basecampapi.com") + m.Warnf = func(format string, args ...any) { warnings = append(warnings, fmt.Sprintf(format, args...)) } + + client, err := m.bc5Client() + require.NoError(t, err) + resp, err := doGet(t, client, "http://203.0.113.5/token") + if resp != nil { + _ = resp.Body.Close() + } + require.Error(t, err) + assert.Contains(t, err.Error(), "proxy configuration error") + assert.Empty(t, warnings, "a refused request is not a proxy-ignored warning") +} + +// TestOAuthTransport_MalformedProxyValueStaysEnforced documents httpproxy's +// handling of an unparsable proxy URL: config.init drops it, so the resolver +// reports no proxy and the request stays on the GUARDED direct transport. +// Enforcement is never lost to a broken proxy value — there is no unguarded +// fallback to reach. +func TestOAuthTransport_MalformedProxyValueStaysEnforced(t *testing.T) { + clearProxyEnv(t) + t.Setenv("HTTP_PROXY", "http://[::1%25en0:8080") // unparsable both raw and http://-prefixed + + m := laneManager(t, "https://3.basecampapi.com") + client, err := m.bc5Client() + require.NoError(t, err) + // 203.0.113.5 is TEST-NET-3 documentation space: IANA special-purpose, + // refused by the default policy before any socket opens. + resp, err := doGet(t, client, "http://203.0.113.5/token") + if resp != nil { + _ = resp.Body.Close() + } + require.Error(t, err) + assert.ErrorIs(t, err, surfguard.ErrBlocked, "the guarded transport must still enforce") +} + +// TestProtectedMode_WarnsWhenProxyIgnored: in the default mode the policy +// applies unconditionally; a proxy the environment would have used for an +// OAuth request is ignored WITH a warning, once per endpoint, and behavior +// stays enforced. +func TestProtectedMode_WarnsWhenProxyIgnored(t *testing.T) { + for _, envVar := range []string{"HTTP_PROXY", "http_proxy"} { + t.Run(envVar, func(t *testing.T) { + clearProxyEnv(t) + t.Setenv(envVar, "http://proxy.corp.example:3128") + + var warnings []string + m := laneManager(t, "https://3.basecampapi.com") + m.Warnf = func(format string, args ...any) { warnings = append(warnings, fmt.Sprintf(format, args...)) } + + client, err := m.bc5Client() + require.NoError(t, err) + + // Two requests to the same endpoint: enforcement holds on both + // (special-purpose space, zero dials), the warning fires once. + for range 2 { + resp, reqErr := doGet(t, client, "http://203.0.113.5/token") + if resp != nil { + _ = resp.Body.Close() + } + require.Error(t, reqErr) + assert.ErrorIs(t, reqErr, surfguard.ErrBlocked, "protected mode must stay enforced") + } + require.Len(t, warnings, 1, "the proxy-ignored warning is deduplicated per endpoint") + assert.Contains(t, warnings[0], "ignoring proxy") + assert.Contains(t, warnings[0], oauthUseProxyEnv+"=1") + }) + } +} + +// TestProtectedMode_NoProxyExclusionProducesNoWarning: a host NO_PROXY +// excludes was never going through the proxy, so there is nothing to warn +// about. +func TestProtectedMode_NoProxyExclusionProducesNoWarning(t *testing.T) { + clearProxyEnv(t) + t.Setenv("HTTP_PROXY", "http://proxy.corp.example:3128") + t.Setenv("NO_PROXY", "203.0.113.5") + + var warnings []string + m := laneManager(t, "https://3.basecampapi.com") + m.Warnf = func(format string, args ...any) { warnings = append(warnings, fmt.Sprintf(format, args...)) } + + client, err := m.bc5Client() + require.NoError(t, err) + resp, err := doGet(t, client, "http://203.0.113.5/token") + if resp != nil { + _ = resp.Body.Close() + } + require.Error(t, err) + assert.ErrorIs(t, err, surfguard.ErrBlocked) + assert.Empty(t, warnings) +} + +// TestOptOutMode_RoutesThroughProxyWithoutEnforcement: BASECAMP_OAUTH_USE_PROXY=1 +// routes a request the resolver assigns to a proxy through that proxy, with +// address enforcement off for exactly that request and the downgrade logged. +// The proxy owns the connection, so the target host is never resolved or +// dialed directly. +func TestOptOutMode_RoutesThroughProxyWithoutEnforcement(t *testing.T) { + var proxied atomic.Int32 + var sawAbsoluteURI atomic.Bool + proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + proxied.Add(1) + // A plain-HTTP proxy request carries the absolute target URI. + if r.URL.IsAbs() && r.URL.Host == "public.example" { + sawAbsoluteURI.Store(true) + } + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"ok":true}`) + })) + defer proxy.Close() + + clearProxyEnv(t) + t.Setenv("HTTP_PROXY", proxy.URL) + t.Setenv(oauthUseProxyEnv, "1") + + var warnings []string + m := laneManager(t, "https://3.basecampapi.com") + m.Warnf = func(format string, args ...any) { warnings = append(warnings, fmt.Sprintf(format, args...)) } + + client, err := m.bc5Client() + require.NoError(t, err) + resp, err := doGet(t, client, "http://public.example/token") + require.NoError(t, err, "the proxy answers; no direct dial happens") + _ = resp.Body.Close() + + assert.Equal(t, int32(1), proxied.Load()) + assert.True(t, sawAbsoluteURI.Load(), "the request must traverse the proxy, not dial direct") + require.Len(t, warnings, 1) + assert.Contains(t, warnings[0], "WITHOUT the SSRF address policy") +} + +// TestOptOutMode_NoProxyTargetStaysGuarded is the decisive fail-closed +// regression: opt-out mode with a NO_PROXY exclusion covering a private OAuth +// target must NOT fall back to unguarded direct egress — the request rides +// the guarded transport and the address policy refuses it with zero dials. +func TestOptOutMode_NoProxyTargetStaysGuarded(t *testing.T) { + var proxied atomic.Int32 + proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + proxied.Add(1) + w.WriteHeader(http.StatusOK) + })) + defer proxy.Close() + + clearProxyEnv(t) + t.Setenv("HTTP_PROXY", proxy.URL) + t.Setenv("NO_PROXY", "203.0.113.5") + t.Setenv(oauthUseProxyEnv, "1") + + m := laneManager(t, "https://3.basecampapi.com") + client, err := m.bc5Client() + require.NoError(t, err) + resp, err := doGet(t, client, "http://203.0.113.5/token") + if resp != nil { + _ = resp.Body.Close() + } + require.Error(t, err) + assert.ErrorIs(t, err, surfguard.ErrBlocked, "resolver-nil requests must ride the guarded transport even in opt-out mode") + assert.Zero(t, proxied.Load()) +} + +// TestOptOutMode_MalformedValuesAreOffWithWarning: only the exact value "1" +// opts out. Anything else — including set-but-empty — is treated as off, with +// a warning, and enforcement stays on. +func TestOptOutMode_MalformedValuesAreOffWithWarning(t *testing.T) { + for _, v := range []string{"yes", "2"} { + t.Run(fmt.Sprintf("value %q", v), func(t *testing.T) { + clearProxyEnv(t) + t.Setenv("HTTP_PROXY", "http://proxy.corp.example:3128") + t.Setenv(oauthUseProxyEnv, v) + + var warnings []string + m := laneManager(t, "https://3.basecampapi.com") + m.Warnf = func(format string, args ...any) { warnings = append(warnings, fmt.Sprintf(format, args...)) } + + client, err := m.bc5Client() + require.NoError(t, err) + resp, reqErr := doGet(t, client, "http://203.0.113.5/token") + if resp != nil { + _ = resp.Body.Close() + } + require.Error(t, reqErr) + assert.ErrorIs(t, reqErr, surfguard.ErrBlocked, "a malformed opt-out must not disable enforcement") + + require.NotEmpty(t, warnings) + assert.Contains(t, warnings[0], "is not understood") + }) + } + + // t.Setenv cannot distinguish set-but-empty from unset for the reader, + // but os.LookupEnv can — and clearProxyEnv sets "" for every variable, + // so every test above already runs the set-but-empty case for the + // OTHER variables. Pin the semantics explicitly: empty means unset here + // (getEnvAny-style), silently off, because "" is what clearing tooling + // writes and warning on it would fire for every user of direnv-style + // scrubbing. + t.Run(`value ""`, func(t *testing.T) { + clearProxyEnv(t) + + var warnings []string + m := laneManager(t, "https://3.basecampapi.com") + m.Warnf = func(format string, args ...any) { warnings = append(warnings, fmt.Sprintf(format, args...)) } + _, err := m.bc5Client() + require.NoError(t, err) + assert.Empty(t, warnings, "set-but-empty is indistinguishable from cleared; stay silent and off") + }) +} + +// TestDiscoverOAuth_LocalIssuerChainFollowsBaseURL is the end-to-end +// provenance test for discovery: a local resource advertising a local issuer +// succeeds under a local base URL (the lane's AllowLoopback covers hop 2 via +// WithIssuerHTTPClient), and the same chain is refused under a production +// base URL. +func TestDiscoverOAuth_LocalIssuerChainFollowsBaseURL(t *testing.T) { + clearProxyEnv(t) + + var hits atomic.Int32 + mux := http.NewServeMux() + srv := httptest.NewServer(mux) + defer srv.Close() + + mux.HandleFunc("/.well-known/oauth-protected-resource", func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `{"resource":%q,"authorization_servers":[%q]}`, srv.URL, srv.URL) + }) + mux.HandleFunc("/.well-known/oauth-authorization-server", func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `{"issuer":%q,"authorization_endpoint":%q,"token_endpoint":%q,"device_authorization_endpoint":%q,"grant_types_supported":["urn:ietf:params:oauth:grant-type:device_code"]}`, + srv.URL, srv.URL+"/authorize", srv.URL+"/token", srv.URL+"/device") + }) + + t.Run("local base URL: local advertised issuer is admitted", func(t *testing.T) { + m := laneManager(t, srv.URL) + d, err := m.discoverOAuth(context.Background(), func(string) {}) + require.NoError(t, err) + assert.Equal(t, oauthTypeBC5, d.oauthType) + assert.Equal(t, srv.URL+"/token", d.config.TokenEndpoint) + assert.Equal(t, int32(2), hits.Load(), "both hops ride the loopback-admitting lane") + }) + + t.Run("production base URL: the same loopback chain is never dialed", func(t *testing.T) { + // Build the lane under production provenance, THEN point the flow at + // the loopback chain: the policy travels with the lane, so hop 1 is + // refused before any connection. The SDK classifies a failed + // resource fetch as the soft Launchpad fallback — the observable + // here is that the loopback chain gets ZERO requests and no BC5 + // device flow is selected from it. + hits.Store(0) + m := laneManager(t, "https://3.basecampapi.com") + _, err := m.bc5Client() + require.NoError(t, err) + m.cfg.BaseURL = srv.URL + d, err := m.discoverOAuth(context.Background(), func(string) {}) + require.NoError(t, err) + assert.Equal(t, oauthTypeLaunchpad, d.oauthType, "a refused resource fetch falls back to Launchpad, never a BC5 flow from the refused chain") + assert.Zero(t, hits.Load(), "the loopback chain must never be dialed under production provenance") + }) +} + +// TestRefreshLocked_PreservesSDKErrorTaxonomy: the CLI boundary used to +// stringify SDK failures into ErrAPI(0). A policy refusal must survive as +// errors.Is(err, surfguard.ErrBlocked) and errors.As(*basecamp.Error), with +// the SDK's code intact, so callers and exit-code mapping see the real +// verdict. +func TestRefreshLocked_PreservesSDKErrorTaxonomy(t *testing.T) { + clearProxyEnv(t) + t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "") + t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "") + + m := laneManager(t, "https://3.basecampapi.com") + creds := &Credentials{ + AccessToken: "stale", + RefreshToken: "refresh-token", + OAuthType: oauthTypeLaunchpad, + // TEST-NET-3: refused by the Launchpad lane's policy at dial time. + TokenEndpoint: "https://203.0.113.5/authorization/token", + } + require.NoError(t, m.store.Save("https://3.basecampapi.com", creds)) + + err := m.refreshLocked(context.Background(), "https://3.basecampapi.com", creds) + require.Error(t, err) + + assert.ErrorIs(t, err, surfguard.ErrBlocked, "surfguard verdict must survive the CLI boundary") + var sdkErr *basecamp.Error + require.ErrorAs(t, err, &sdkErr, "the SDK's typed error must survive the CLI boundary") + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, sdkErr.Code, cliErr.Code, "the SDK's code must not be flattened") + assert.True(t, strings.HasPrefix(cliErr.Message, "token refresh failed: "), "message keeps the operation prefix: %q", cliErr.Message) +} + +// TestExchangeCode_PreservesSDKErrorTaxonomy: same contract on the web-flow +// exchange wrap. +func TestExchangeCode_PreservesSDKErrorTaxonomy(t *testing.T) { + clearProxyEnv(t) + + m := laneManager(t, "https://3.basecampapi.com") + cfg := &oauth.Config{TokenEndpoint: "https://203.0.113.5/authorization/token"} + _, err := m.exchangeCode(context.Background(), cfg, "auth-code", + &ClientCredentials{ClientID: "id", ClientSecret: "secret"}, + &LoginOptions{RedirectURI: defaultRedirectURI}) + require.Error(t, err) + + assert.ErrorIs(t, err, surfguard.ErrBlocked) + var sdkErr *basecamp.Error + require.ErrorAs(t, err, &sdkErr) + var cliErr *output.Error + require.ErrorAs(t, err, &cliErr) + assert.Equal(t, sdkErr.Code, cliErr.Code) + assert.True(t, strings.HasPrefix(cliErr.Message, "token exchange failed: ")) +} + +// TestRefreshLocked_RedirectStatusSurvives asserts a refused token-endpoint +// redirect reaches the caller as a typed error carrying the 3xx status. +func TestRefreshLocked_RedirectStatusSurvives(t *testing.T) { + t.Skip("SDK pin predates token-endpoint redirect classification (basecamp-sdk branch harden-token-exchange); un-skip at the next re-pin") +} diff --git a/internal/commands/tools.go b/internal/commands/tools.go index 85d546a0..853c3527 100644 --- a/internal/commands/tools.go +++ b/internal/commands/tools.go @@ -125,7 +125,11 @@ func newToolsShowCmd(project *string) *cobra.Command { if tool.Position != nil { posStr = fmt.Sprintf("%d", *tool.Position) } - summary := fmt.Sprintf("%s (%s) at position %s", tool.Title, tool.Name, posStr) + name := "" + if tool.Name != nil { + name = *tool.Name + } + summary := fmt.Sprintf("%s (%s) at position %s", tool.Title, name, posStr) crumbs := []output.Breadcrumb{ { diff --git a/internal/version/sdk-provenance.json b/internal/version/sdk-provenance.json index 2d868c04..3ccb5bcf 100644 --- a/internal/version/sdk-provenance.json +++ b/internal/version/sdk-provenance.json @@ -1,13 +1,13 @@ { "sdk": { "module": "github.com/basecamp/basecamp-sdk/go", - "version": "v0.14.0", - "revision": "e47f90a60376", - "updated_at": "2026-08-12T00:03:22Z" + "version": "v0.15.0", + "revision": "1dd547b3fd85", + "updated_at": "2026-08-22T09:57:22Z" }, "api": { "repo": "basecamp/bc3", - "revision": "b5d8c9df8dd957e78bf2618807623d14b4704dc2", - "synced_at": "2026-08-05" + "revision": "71b43f3d9fa90a6c26e5af21b02697e33e7f0bae", + "synced_at": "2026-08-11" } } From bb7af86d56ef823c8932a09f6022791c162c25c7 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 28 Aug 2026 22:28:40 -0700 Subject: [PATCH 2/6] Address review: escape-safe warnings, host-only proxy redaction, nil tool name, nix hash - Render OAuth endpoint paths percent-encoded in proxy warnings and dedupe keys: url.Parse decodes escapes into Path, so a hostile discovery document could put terminal control sequences or a newline into stderr. - Log proxies as scheme://host only. url.URL.Redacted masks passwords but preserves bare usernames and query values, both of which HTTP(S)_PROXY can carry as credentials. - Omit the "(name)" parenthetical in `tools show` when Tool.Name is nil, which SDK v0.15.0 documents as always for the Get projection. - Fix two doc comments that claimed set-but-empty BASECAMP_OAUTH_USE_PROXY warns; it is silently treated as unset by design. - Refresh the Nix vendorHash for the surfguard dependency (the flake job was red). --- internal/auth/client.go | 22 +++++++++++++++++----- internal/auth/client_test.go | 25 +++++++++++++++++++++++-- internal/commands/tools.go | 7 ++++--- 3 files changed, 44 insertions(+), 10 deletions(-) diff --git a/internal/auth/client.go b/internal/auth/client.go index 44f5a287..20b5bd88 100644 --- a/internal/auth/client.go +++ b/internal/auth/client.go @@ -19,7 +19,9 @@ import ( // oauthUseProxyEnv opts OAuth traffic out of the SSRF address policy for // requests the environment's proxy configuration actually routes to a proxy. -// Only the exact value "1" enables it; anything else is refused with a +// Only the exact value "1" enables it. Set-but-empty is treated as unset, +// silently: env-scrubbing tooling (direnv, t.Setenv) writes "" and cannot +// be told apart from intent. Any other non-empty value is refused with a // warning rather than guessed at. const oauthUseProxyEnv = "BASECAMP_OAUTH_USE_PROXY" @@ -168,11 +170,11 @@ func (t *oauthTransport) RoundTrip(req *http.Request) (*http.Response, error) { // every few seconds, and one downgrade notice per endpoint records // the decision without drowning the terminal. t.warnOnce(req.URL, "warning: OAuth request to %s routed through proxy %s WITHOUT the SSRF address policy (%s=1)", - redactedEndpoint(req.URL), proxyURL.Redacted(), oauthUseProxyEnv) + redactedEndpoint(req.URL), redactedProxy(proxyURL), oauthUseProxyEnv) return t.proxied.RoundTrip(req) default: t.warnOnce(req.URL, "warning: ignoring proxy %s for OAuth request to %s: the SSRF address policy requires direct egress; set %s=1 to route OAuth through the proxy without address enforcement", - proxyURL.Redacted(), redactedEndpoint(req.URL), oauthUseProxyEnv) + redactedProxy(proxyURL), redactedEndpoint(req.URL), oauthUseProxyEnv) return t.guarded.RoundTrip(req) } } @@ -190,9 +192,19 @@ func (t *oauthTransport) warnOnce(u *url.URL, format string, args ...any) { // redactedEndpoint renders a request URL for warnings and dedupe keys without // its query or userinfo — endpoint paths are diagnostic, query strings can -// carry parameters that don't belong in a terminal. +// carry parameters that don't belong in a terminal. The path is rendered in +// its percent-encoded form: endpoints come from OAuth discovery documents, +// and url.Parse decodes escapes into Path, so a hostile document could +// otherwise put terminal control sequences or a newline into the warning. func redactedEndpoint(u *url.URL) string { - return u.Scheme + "://" + u.Host + u.Path + return u.Scheme + "://" + u.Host + u.EscapedPath() +} + +// redactedProxy renders a proxy URL as scheme and host only. Proxy URLs come +// from HTTP(S)_PROXY, where credentials may be encoded as a bare username or +// as query parameters — neither of which url.URL.Redacted masks. +func redactedProxy(u *url.URL) string { + return u.Scheme + "://" + u.Host } // proxiedTransport clones http.DefaultTransport (never mutating the global) diff --git a/internal/auth/client_test.go b/internal/auth/client_test.go index 447d6b22..79758359 100644 --- a/internal/auth/client_test.go +++ b/internal/auth/client_test.go @@ -441,8 +441,9 @@ func TestOptOutMode_NoProxyTargetStaysGuarded(t *testing.T) { } // TestOptOutMode_MalformedValuesAreOffWithWarning: only the exact value "1" -// opts out. Anything else — including set-but-empty — is treated as off, with -// a warning, and enforcement stays on. +// opts out. Any other non-empty value is treated as off, with a warning, and +// enforcement stays on. Set-but-empty is off too, but silently — see the +// final subtest. func TestOptOutMode_MalformedValuesAreOffWithWarning(t *testing.T) { for _, v := range []string{"yes", "2"} { t.Run(fmt.Sprintf("value %q", v), func(t *testing.T) { @@ -598,3 +599,23 @@ func TestExchangeCode_PreservesSDKErrorTaxonomy(t *testing.T) { func TestRefreshLocked_RedirectStatusSurvives(t *testing.T) { t.Skip("SDK pin predates token-endpoint redirect classification (basecamp-sdk branch harden-token-exchange); un-skip at the next re-pin") } + +// TestRedactedEndpoint_KeepsEscapesAndDropsSecrets: the endpoint comes from +// a discovery document, so decoded control sequences must stay +// percent-encoded, and query/userinfo must not reach the terminal. +func TestRedactedEndpoint_KeepsEscapesAndDropsSecrets(t *testing.T) { + u, err := url.Parse("https://user:pw@issuer.example/token%1b%5b31m%0ainjected?code=secret#frag") + require.NoError(t, err) + got := redactedEndpoint(u) + assert.Equal(t, "https://issuer.example/token%1b%5b31m%0ainjected", got) + assert.NotContains(t, got, "\x1b") + assert.NotContains(t, got, "\n") +} + +// TestRedactedProxy_SchemeAndHostOnly: HTTP(S)_PROXY may carry a token as a +// bare username or a query parameter, which url.URL.Redacted preserves. +func TestRedactedProxy_SchemeAndHostOnly(t *testing.T) { + u, err := url.Parse("http://token123@proxy.corp.example:3128/?key=abc") + require.NoError(t, err) + assert.Equal(t, "http://proxy.corp.example:3128", redactedProxy(u)) +} diff --git a/internal/commands/tools.go b/internal/commands/tools.go index 853c3527..5839b0ef 100644 --- a/internal/commands/tools.go +++ b/internal/commands/tools.go @@ -125,11 +125,12 @@ func newToolsShowCmd(project *string) *cobra.Command { if tool.Position != nil { posStr = fmt.Sprintf("%d", *tool.Position) } - name := "" + // Name is nil for the Get projection in SDK v0.15.0; only render + // the parenthetical when there is something to put in it. + summary := fmt.Sprintf("%s at position %s", tool.Title, posStr) if tool.Name != nil { - name = *tool.Name + summary = fmt.Sprintf("%s (%s) at position %s", tool.Title, *tool.Name, posStr) } - summary := fmt.Sprintf("%s (%s) at position %s", tool.Title, name, posStr) crumbs := []output.Breadcrumb{ { From 415a2f29c58f85c611a1363d71492cc86ed84acb Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 28 Aug 2026 22:46:05 -0700 Subject: [PATCH 3/6] Sanitize OAuth transport warnings at the sink EscapedPath only covered the path: url.Parse admits UTF-8 C1 controls (U+009B CSI) in a host verbatim, so a discovery-controlled endpoint host could still inject terminal controls through the proxy warnings. Scrub the whole rendered message once in Manager.warnf, which covers the endpoint host and path, the proxy host, and any field a later warning interpolates. --- internal/auth/client.go | 12 ++++++++++-- internal/auth/client_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/internal/auth/client.go b/internal/auth/client.go index 20b5bd88..0d7fc81f 100644 --- a/internal/auth/client.go +++ b/internal/auth/client.go @@ -15,6 +15,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/hostutil" "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/richtext" ) // oauthUseProxyEnv opts OAuth traffic out of the SSRF address policy for @@ -110,12 +111,19 @@ func (m *Manager) proxyState() *proxyEnvState { // warnf routes transport-policy warnings to the Manager's Warnf seam, or // stderr by default — these fire inside RoundTrip, where no command logger // is in scope. +// +// The rendered message is sanitized as a whole before it reaches either +// sink: the endpoint host and path come from OAuth discovery documents, the +// proxy host from the environment, and url.Parse admits UTF-8 C1 controls +// (U+009B CSI and friends) in a host verbatim. Scrubbing once here covers +// every field the warnings interpolate, including ones added later. func (m *Manager) warnf(format string, args ...any) { + msg := richtext.SanitizeSingleLine(fmt.Sprintf(format, args...)) if m.Warnf != nil { - m.Warnf(format, args...) + m.Warnf("%s", msg) return } - fmt.Fprintf(os.Stderr, format+"\n", args...) + fmt.Fprintln(os.Stderr, msg) } // oauthTransport is the per-lane egress RoundTripper for OAuth traffic. It diff --git a/internal/auth/client_test.go b/internal/auth/client_test.go index 79758359..275f010d 100644 --- a/internal/auth/client_test.go +++ b/internal/auth/client_test.go @@ -619,3 +619,31 @@ func TestRedactedProxy_SchemeAndHostOnly(t *testing.T) { require.NoError(t, err) assert.Equal(t, "http://proxy.corp.example:3128", redactedProxy(u)) } + +// TestWarnf_SanitizesRenderedMessage: url.Parse admits UTF-8 C1 controls in +// a host, and EscapedPath only covers the path — so the sink scrubs the +// whole rendered warning, whichever field carried the control. +func TestWarnf_SanitizesRenderedMessage(t *testing.T) { + clearProxyEnv(t) + t.Setenv("HTTP_PROXY", "http://proxy.corp.example:3128") + + var warnings []string + m := laneManager(t, "https://3.basecampapi.com") + m.Warnf = func(format string, args ...any) { warnings = append(warnings, fmt.Sprintf(format, args...)) } + + client, err := m.bc5Client() + require.NoError(t, err) + resp, reqErr := doGet(t, client, "http://evil\u009b31m.example/token%1b%5b31m") + if resp != nil { + _ = resp.Body.Close() + } + require.Error(t, reqErr) + + require.NotEmpty(t, warnings, "the ignored-proxy warning fires before the request is refused") + for _, w := range warnings { + assert.NotContains(t, w, "\u009b") + assert.NotContains(t, w, "\x1b") + assert.NotContains(t, w, "\n") + assert.Contains(t, w, "evil31m.example", "C1 control stripped, host otherwise intact") + } +} From 53c210a79eec7a8c4ba3e4abd932b12622d447eb Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 28 Aug 2026 23:26:56 -0700 Subject: [PATCH 4/6] Sync the vendored MCP model to basecamp-sdk go/v0.15.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestCatalogModelProvenance requires the MCP model snapshot to match the SDK version go.mod pins. Synced via scripts/sync-mcp-model.sh from go/v0.15.0 (1dd547b3): API revision 2026-08-05 → 2026-08-11, description and required-field tightening only, no operation changes. --- internal/mcpserver/model/PROVENANCE.json | 4 +- internal/mcpserver/model/openapi.json | 308 ++++++++++++++++++++--- 2 files changed, 281 insertions(+), 31 deletions(-) diff --git a/internal/mcpserver/model/PROVENANCE.json b/internal/mcpserver/model/PROVENANCE.json index 64be29e2..26a3cad0 100644 --- a/internal/mcpserver/model/PROVENANCE.json +++ b/internal/mcpserver/model/PROVENANCE.json @@ -1,7 +1,7 @@ { "source": "github.com/basecamp/basecamp-sdk", - "commit": "e47f90a60376743eb8f4bbc372f295a065adccbe", - "ref": "go/v0.14.0", + "commit": "1dd547b3fd85bcdbd491ac85a66566c87f295549", + "ref": "go/v0.15.0", "files": ["behavior-model.json", "openapi.json"], "synced_by": "scripts/sync-mcp-model.sh", "patches": "tags assigned to operations the export leaves untagged (PATCHED_TAGS); binary-upload operations dropped (EXCLUDED_OPERATIONS) — see the sync script" diff --git a/internal/mcpserver/model/openapi.json b/internal/mcpserver/model/openapi.json index e05f59e4..db362eef 100644 --- a/internal/mcpserver/model/openapi.json +++ b/internal/mcpserver/model/openapi.json @@ -2,7 +2,7 @@ "openapi": "3.1.0", "info": { "title": "Basecamp", - "version": "2026-08-05", + "version": "2026-08-11", "description": "Basecamp API", "contact": { "name": "Basecamp", @@ -27432,7 +27432,8 @@ "type": "string" }, "command_url": { - "type": "string" + "type": "string", + "description": "Only present when the requester is an account administrator; possession of this URL is enough to command the bot." }, "url": { "type": "string" @@ -27441,7 +27442,8 @@ "type": "string" }, "lines_url": { - "type": "string" + "type": "string", + "description": "Only present when the requester is an account administrator; possession of this URL is enough to post lines as the bot." } }, "required": [ @@ -30414,10 +30416,13 @@ "type": "array", "items": { "$ref": "#/components/schemas/TimelineEvent" - }, - "x-go-type-skip-optional-pointer": true + } } - } + }, + "required": [ + "events", + "person" + ] }, "GetPersonResponseContent": { "$ref": "#/components/schemas/Person" @@ -31439,6 +31444,7 @@ }, "MyAssignmentAssignee": { "type": "object", + "description": "A person as bc3's `people/_person_minimal.json.jbuilder` renders them \u2014\nthe same three-key partial behind UpcomingSchedulePerson and\nOutOfOfficePerson. All three keys are emitted unconditionally, so all\nthree are required.", "properties": { "id": { "type": "integer", @@ -31454,7 +31460,9 @@ } }, "required": [ - "id" + "avatar_url", + "id", + "name" ] }, "MyAssignmentBucket": { @@ -31747,6 +31755,7 @@ }, "OutOfOfficePerson": { "type": "object", + "description": "A person as bc3's `people/_person_minimal.json.jbuilder` renders them \u2014\nthe same three-key partial behind UpcomingSchedulePerson and\nMyAssignmentAssignee. All three keys are emitted unconditionally, so all\nthree are required.", "properties": { "id": { "type": "integer", @@ -31762,7 +31771,9 @@ } }, "required": [ - "id" + "avatar_url", + "id", + "name" ] }, "PauseQuestionResponseContent": { @@ -33323,9 +33334,11 @@ }, "SearchResult": { "type": "object", + "description": "One hit from account-wide search \u2014 a polymorphic projection over every\nsearchable recording type.\n\nMost result types render the common recording envelope\n(`recordings/_recording.json.jbuilder`) plus their own recordable partial,\nbut `api_search_result_template_path` special-cases four branches \u2014 chat\nlines, kanban (card table) lists, file attachments and gauge needles \u2014 and\nthe file-attachment branch writes its own projection from scratch instead\nof the envelope. Members are therefore optional unless every branch emits\nthem; the doc comments name the branches that carry each optional member.", "properties": { "id": { "type": "integer", + "description": "The recording id. Optional only because the file-attachment branch\n(`searches/_attachment.json.jbuilder`) skips the recording envelope and\nemits none of the top-level id/title/type/url/app_url keys; every other\nbranch emits all five.", "format": "int64" }, "status": { @@ -33349,23 +33362,31 @@ } }, "title": { - "type": "string" + "type": "string", + "description": "See `id` \u2014 omitted by the file-attachment branch, emitted by every other\nbranch." }, "inherits_status": { "type": "boolean" }, "type": { - "type": "string" + "type": "string", + "description": "See `id` \u2014 omitted by the file-attachment branch, emitted by every other\nbranch. A file-attachment hit is therefore recognizable by the absence\nof this key (its file keys, `filename` through `app_download_url`, are\nthe positive signal)." }, "url": { - "type": "string" + "type": "string", + "description": "See `id` \u2014 omitted by the file-attachment branch, emitted by every other\nbranch." }, "app_url": { - "type": "string" + "type": "string", + "description": "See `id` \u2014 omitted by the file-attachment branch, emitted by every other\nbranch." }, "bookmark_url": { "type": "string" }, + "subscription_url": { + "type": "string", + "description": "Subscription URL, emitted by the common recording envelope for any\nsubscribable result \u2014 kanban lists and gauge needles among the special\nbranches, plus subscribable generic-branch types (messages, todos, \u2026)." + }, "bubble_up_url": { "type": "string", "description": "URL of the Bubble Up record for this recording (BC5 addition). Optional\nhere because this is a polymorphic projection:\n`recordings/_recording.json.jbuilder` emits the key only when the caller\npasses `local_assigns[:bubbleupable]`, and `todolists/_todolist` is the\nonly partial that does. So a Todolist-shaped instance carries it and the\nother recording types do not." @@ -33408,7 +33429,7 @@ "items": { "$ref": "#/components/schemas/RichTextAttachment" }, - "description": "Rich-text companion arrays carried through the polymorphic search\nprojection. A given result is one recording type, so it carries only\nthe array matching its rich-text attribute (`content_attachments` for a\nComment/Message, `description_attachments` for a Todo); a webhook-sourced\nresult carries neither. Optional (no `@required`), non-nullable.\n\nSearch results additionally repeat this same array under a generic\n`attachments` key. It is a redundant projection, not a distinct\naggregate: `searches/show.json.jbuilder` emits\n`recording.downloadable_attachments`, which delegates to the recordable's\nsole `rich_text_content`, through the same `attachments/_attachment`\npartial that `recordings/_rich_text.json.jbuilder` uses to build the\ncompanion array. `RichText.rich_text_attribute` permits exactly one\nrich-text attribute per model, so the two keys always carry identical\nelements. Modeling `attachments` would duplicate the field, so it is\ndeliberately not modeled.", + "description": "Rich-text companion arrays carried through the polymorphic search\nprojection. A given result is one recording type, so it carries only\nthe array matching its rich-text attribute (`content_attachments` for a\nComment/Message, `description_attachments` for a Todo); a webhook-sourced\nresult carries neither. Optional (no `@required`), non-nullable.", "x-go-type-skip-optional-pointer": true }, "description_attachments": { @@ -33419,18 +33440,219 @@ "description": "See `content_attachments` \u2014 the description-attribute companion array.", "x-go-type-skip-optional-pointer": true }, + "attachments": { + "type": "array", + "items": { + "$ref": "#/components/schemas/SearchResultAttachment" + }, + "description": "File attachments on the result, in either of two wire shapes \u2014 see\n`SearchResultAttachment`.\n\nFor a result whose recordable carries downloadable rich-text\nattachments, `searches/show.json.jbuilder` emits this key through the\nsame `attachments/_attachment` partial that builds the rich-text\ncompanion array above, so it repeats that array's elements. For a chat\n*upload* line, `chats/lines/_upload.json.jbuilder` instead builds a\nbespoke six-key aggregate inline \u2014 and because upload lines have no\nrich-text attribute, the show template never overwrites it, so that\ndistinct shape survives to the wire.", + "x-go-type-skip-optional-pointer": true + }, "subject": { "type": "string" + }, + "boosts_count": { + "type": "integer", + "description": "Boost count, emitted by the recording envelope when the branch passes\n`boostable` \u2014 chat lines and gauge needles.", + "format": "int32" + }, + "boosts_url": { + "type": "string", + "description": "See `boosts_count` \u2014 the companion URL." + }, + "language": { + "type": "string", + "description": "Language of a code chat line (`chats/lines/_code.json.jbuilder`);\nvalidated present on the model, so never null when the key is emitted." + }, + "image_url": { + "type": "string", + "description": "Image URL of a soundtracked (play-kind) chat line whose sound carries an\nimage; such a line emits `image_url` in place of `content`." + }, + "sound_url": { + "type": "string", + "description": "Sound URL of a play-kind chat line; always emitted for that kind." + }, + "subscribers": { + "type": "array", + "items": { + "$ref": "#/components/schemas/Person" + }, + "description": "Everyone subscribed to the kanban list, as full Person projections.", + "x-go-type-skip-optional-pointer": true + }, + "color": { + "type": "string", + "description": "Color of a kanban list or gauge needle. Emitted unconditionally by both\nbranches with a null value when unset, so it is nullable (the enhance\npass layers `nullable: true` onto the OpenAPI).", + "nullable": true + }, + "cards_count": { + "type": "integer", + "description": "Number of cards in the kanban list.", + "format": "int32" + }, + "comment_count": { + "type": "integer", + "description": "Comment count of a kanban list or gauge needle (branch-partial key,\nsingular `comment_count` \u2014 distinct from the envelope's\n`comments_count`, which a needle also carries).", + "format": "int32" + }, + "cards_url": { + "type": "string", + "description": "API URL of the kanban list's cards." + }, + "on_hold": { + "$ref": "#/components/schemas/CardColumnOnHold" + }, + "comments_count": { + "type": "integer", + "description": "Comment count, emitted by the recording envelope when the branch passes\n`commentable` \u2014 gauge needles among the special branches, plus\ncommentable generic-branch types.", + "format": "int32" + }, + "comments_url": { + "type": "string", + "description": "See `comments_count` \u2014 the companion URL." + }, + "position": { + "type": "integer", + "description": "Position of the result. Two emitters share the key: the recording\nenvelope emits list position for positioned recordings (kanban lists\namong the special branches), and the gauge-needle branch overwrites it\nwith the needle's own 0\u2013100 gauge position.", + "format": "int32" + }, + "filename": { + "type": "string", + "description": "Filename of a file-attachment hit. This and the following file keys are\nemitted only by the file-attachment branch \u2014 the one branch that omits\nthe id/title/type/url/app_url envelope keys." + }, + "content_type": { + "type": "string", + "description": "MIME type of a file-attachment hit." + }, + "byte_size": { + "type": "integer", + "description": "Size in bytes of a file-attachment hit.", + "format": "int64" + }, + "previewable": { + "type": "boolean", + "description": "Whether the file can be previewed." + }, + "width": { + "type": "integer", + "description": "Pixel width, emitted only when the file is previewable. May be\nfloat-spelled (`1024.0`) and nullable, like every other blob dimension \u2014\nsee `RichTextAttachment.width` for the cross-SDK typing note.", + "format": "int32", + "nullable": true, + "x-go-type": "types.FlexInt", + "x-go-type-import": { + "path": "github.com/basecamp/basecamp-sdk/go/pkg/types" + } + }, + "height": { + "type": "integer", + "description": "See `width` \u2014 same conditional emission and nullable/float-spelled\nbehavior.", + "format": "int32", + "nullable": true, + "x-go-type": "types.FlexInt", + "x-go-type-import": { + "path": "github.com/basecamp/basecamp-sdk/go/pkg/types" + } + }, + "preview_url": { + "type": "string", + "description": "Full-size preview URL of a file-attachment hit." + }, + "thumbnail_url": { + "type": "string", + "description": "Thumbnail URL of a file-attachment hit." + }, + "download_url": { + "type": "string", + "description": "Authenticated download URL of a file-attachment hit.", + "x-basecamp-auth-routable-url": {} + }, + "app_download_url": { + "type": "string", + "description": "Web (app-host) download URL of a file-attachment hit." } }, "required": [ - "app_url", "content", - "description", - "id", - "title", - "type", - "url" + "description" + ] + }, + "SearchResultAttachment": { + "type": "object", + "description": "A file attached to a search result, in either of two wire shapes.\n\nThis is an optional-field superset over the two variants the search\nprojection emits (the TimelineAttachment approach): the rich-text\nattachment/blob shape rendered through `attachments/_attachment` +\n`blobs/_blob` \u2014 the same emitters `RichTextAttachment` models \u2014 and the\nbespoke six-key aggregate a chat upload line builds inline in\n`chats/lines/_upload.json.jbuilder`. Only the four keys both variants\nalways emit are `@required`; the rest identify their variant.", + "properties": { + "filename": { + "type": "string", + "description": "Original filename (both variants)." + }, + "content_type": { + "type": "string", + "description": "MIME type of the file (both variants)." + }, + "byte_size": { + "type": "integer", + "description": "Size of the file in bytes (both variants).", + "format": "int64" + }, + "download_url": { + "type": "string", + "description": "Authenticated download URL for the file (both variants).", + "x-basecamp-auth-routable-url": {} + }, + "id": { + "type": "integer", + "description": "Attachment id (rich-text variant).", + "format": "int64" + }, + "sgid": { + "type": "string", + "description": "Signed global id of the attachment (rich-text variant)." + }, + "previewable": { + "type": "boolean", + "description": "Whether the blob can be previewed (rich-text variant)." + }, + "preview_url": { + "type": "string", + "description": "Full-size preview URL (rich-text variant)." + }, + "thumbnail_url": { + "type": "string", + "description": "Thumbnail URL (rich-text variant)." + }, + "width": { + "type": "integer", + "description": "Pixel width (rich-text variant) \u2014 null for non-image blobs and may be\nfloat-spelled (`1024.0`); see `RichTextAttachment.width`.", + "format": "int32", + "nullable": true, + "x-go-type": "types.FlexInt", + "x-go-type-import": { + "path": "github.com/basecamp/basecamp-sdk/go/pkg/types" + } + }, + "height": { + "type": "integer", + "description": "See `width` (rich-text variant).", + "format": "int32", + "nullable": true, + "x-go-type": "types.FlexInt", + "x-go-type-import": { + "path": "github.com/basecamp/basecamp-sdk/go/pkg/types" + } + }, + "title": { + "type": "string", + "description": "Title of the attachment recording (chat upload-line variant)." + }, + "url": { + "type": "string", + "description": "Browser preview URL of the blob (chat upload-line variant)." + } + }, + "required": [ + "byte_size", + "content_type", + "download_url", + "filename" ] }, "SearchType": { @@ -34440,6 +34662,9 @@ "status": { "type": "string" }, + "visible_to_clients": { + "type": "boolean" + }, "created_at": { "type": "string", "x-go-type": "time.Time", @@ -34457,15 +34682,12 @@ "title": { "type": "string" }, - "name": { - "type": "string" - }, - "enabled": { + "inherits_status": { "type": "boolean" }, - "position": { - "type": "integer", - "format": "int32" + "type": { + "type": "string", + "description": "The tool's recordable type, e.g. `Chat::Transcript`, `Todoset`, `Vault`." }, "url": { "type": "string" @@ -34473,17 +34695,45 @@ "app_url": { "type": "string" }, + "bookmark_url": { + "type": "string" + }, + "subscription_url": { + "type": "string", + "description": "Absent for tool types that are not subscribable." + }, + "position": { + "type": "integer", + "description": "Emitted only for a positioned recording. For a docked tool that makes an\nabsent position the disabled signal \u2014 disabling removes the tool from the\ndock without deleting it. Positioning is independent of dockedness, so\nthis does not generalize: a nested vault is not docked and is positioned.", + "format": "int32" + }, + "parent": { + "$ref": "#/components/schemas/RecordingParent" + }, "bucket": { "$ref": "#/components/schemas/RecordingBucket" + }, + "creator": { + "$ref": "#/components/schemas/Person" + }, + "name": { + "type": "string", + "description": "Not emitted by this projection. The dock array on a project\n(`DockItem$name`) carries the tool's slug; this key is absent from every\nGetTool/CreateTool/UpdateTool response." + }, + "enabled": { + "type": "boolean", + "description": "Not emitted by this projection. The dock array on a project\n(`DockItem$enabled`) is the authoritative enabled flag; on a docked tool's\nown response, an absent `position` is the equivalent signal." } }, "required": [ "created_at", - "enabled", + "creator", "id", - "name", + "inherits_status", "title", - "updated_at" + "type", + "updated_at", + "visible_to_clients" ] }, "UnauthorizedErrorResponseContent": { From c03a188841339fa7e48e68496fcc234a0a9f0374 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 28 Aug 2026 23:29:06 -0700 Subject: [PATCH 5/6] Refresh the Nix vendorHash after rebasing onto main The rebase merged main's dependency bumps (basecamp/cli, x/crypto, mcp) with this branch's SDK v0.15.0 + surfguard pins, which invalidates the fixed-output derivation hash. Value from the flake job's own computation. --- nix/package.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nix/package.nix b/nix/package.nix index 509a9235..3af53d42 100644 --- a/nix/package.nix +++ b/nix/package.nix @@ -8,7 +8,7 @@ buildGoModule.override { go = go_1_26; } (finalAttrs: { src = lib.cleanSource ./..; # To update: set to lib.fakeHash, run `nix build`, use the hash from the error. - vendorHash = "sha256-/ZlKQETlhqPgzMd9/e7R4Pochvo0dnjA39i7mkcgjk4="; + vendorHash = "sha256-aiOnf1ZoeFKVCDrc47QIP0aNWPf63bqdeDrpqDbWt5Y="; subPackages = [ "cmd/basecamp" ]; From d71a01576443b411da4c044338a9773e3dd010f5 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 28 Aug 2026 23:35:05 -0700 Subject: [PATCH 6/6] Test the tools show summary with and without a tool name SDK v0.15.0's Get projection omits name; pin that the summary reads 'Title at position N' with no empty parenthetical, and 'Title (name) at position N' when a name is present. --- internal/commands/tools_test.go | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/internal/commands/tools_test.go b/internal/commands/tools_test.go index 07609fda..713cca98 100644 --- a/internal/commands/tools_test.go +++ b/internal/commands/tools_test.go @@ -524,6 +524,11 @@ func (t *mockToolTransport) RoundTrip(req *http.Request) (*http.Response, error) switch { case strings.Contains(req.URL.Path, "/projects.json"): body = `[{"id": 123, "name": "Test Project"}]` + case strings.HasSuffix(req.URL.Path, "/tools/556"): + // The SDK v0.15.0 Get projection: no "name" key at all. + body = `{"id": 556, "title": "Chat", "enabled": true, "position": 2,` + + `"status": "active", "url": "https://example.com", "app_url": "https://example.com",` + + `"created_at": "2024-01-01T00:00:00Z", "updated_at": "2024-01-01T00:00:00Z"}` case strings.HasSuffix(req.URL.Path, "/tools/555"): body = `{"id": 555, "title": "Chat", "name": "chat", "enabled": true, "position": 2,` + `"status": "active", "url": "https://example.com", "app_url": "https://example.com",` + @@ -647,3 +652,29 @@ func TestToolsCreateVisibleToClientsExplicitFalse(t *testing.T) { require.True(t, transport.createCalled) assert.Equal(t, false, transport.capturedBody["visible_to_clients"]) } + +// TestToolsShowSummaryOmitsMissingName pins the summary shape on both sides +// of SDK v0.15.0's Tool.Name becoming *string: a name renders in parentheses, +// and a response without one renders no empty "()" parenthetical. +func TestToolsShowSummaryOmitsMissingName(t *testing.T) { + for _, tc := range []struct{ id, want string }{ + {"555", "Chat (chat) at position 2"}, + {"556", "Chat at position 2"}, + } { + t.Run("tool "+tc.id, func(t *testing.T) { + app, buf := newTestAppWithTransport(t, &mockToolTransport{}) + app.Config.ProjectID = "" + + project := "" + cmd := newToolsShowCmd(&project) + require.NoError(t, executeCommand(cmd, app, tc.id)) + + var envelope struct { + Summary string `json:"summary"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope)) + assert.Equal(t, tc.want, envelope.Summary) + assert.NotContains(t, envelope.Summary, "()") + }) + } +}