diff --git a/internal/appctx/context.go b/internal/appctx/context.go index 3107418ce..53bacef99 100644 --- a/internal/appctx/context.go +++ b/internal/appctx/context.go @@ -81,10 +81,33 @@ 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} + // Create HTTP client for auth manager (OAuth discovery, token refresh). + httpClient := &http.Client{ + Timeout: 30 * time.Second, + CheckRedirect: checkAuthClientRedirect, + } authMgr := auth.NewManager(cfg, httpClient) // Create observability components diff --git a/internal/appctx/context_test.go b/internal/appctx/context_test.go index c72c5c601..87d730e68 100644 --- a/internal/appctx/context_test.go +++ b/internal/appctx/context_test.go @@ -7,7 +7,9 @@ import ( "net/http" "net/http/httptest" "os" + "sync/atomic" "testing" + "time" "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/stretchr/testify/assert" @@ -48,6 +50,39 @@ 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 433573e71..e9f2b4b3f 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -5,6 +5,7 @@ import ( "bufio" "context" "encoding/json" + "errors" "fmt" "io" "net" @@ -194,6 +195,19 @@ func (m *Manager) refreshLocked(ctx context.Context, origin string, creds *Crede tokenEndpoint := creds.TokenEndpoint + // The token endpoint here is a persisted (possibly migrated) value from + // the credential store and receives the refresh token plus client + // credentials. The SDK's RequireSecureEndpoint only checks scheme==https, + // so a poisoned store could still carry userinfo (https://user@evil/), + // empty-host, or opaque/malformed https forms that it would let through. + // Apply the same strict check used for the other OAuth endpoints before + // any POST. + if u, err := url.Parse(tokenEndpoint); err != nil { + return output.ErrAuth(fmt.Sprintf("invalid token endpoint %q: %v", tokenEndpoint, err)) + } else if !isSecureEndpointURL(u) { + return output.ErrAuth(fmt.Sprintf("invalid token endpoint %q: must be an absolute https URL (or http on loopback)", tokenEndpoint)) + } + // Resolve client credentials for the refresh request var clientID, clientSecret string switch creds.OAuthType { @@ -621,7 +635,43 @@ func (m *Manager) loadBC3Client() (*ClientCredentials, error) { return &creds, nil } +// isSecureEndpointURL reports whether u uses a scheme safe for OAuth endpoints +// derived from the server-controlled discovery document: https, or http only on +// loopback for local development. The URL must also be absolute with a hostname — +// url.Parse accepts opaque forms like "https:foo" and port-only authorities like +// "https://:3000/" that carry the right scheme but no hostname, which would +// otherwise slip through to the transport or browser launcher. URLs carrying +// userinfo (user:pass@host) are rejected outright: they enable phishing +// displays in browsers and net/http synthesizes a Basic Authorization header +// from them. Centralizing the rule keeps the registration, authorization, and +// redirect-following checks consistent. +func isSecureEndpointURL(u *url.URL) bool { + if u.Hostname() == "" { + return false + } + // Userinfo enables phishing display in browsers ("evil.example@real.host") + // and Basic-auth synthesis in net/http requests. + if u.User != nil { + return false + } + // IsLocalhost takes the host:port form and strips the port itself. + return u.Scheme == "https" || (u.Scheme == "http" && hostutil.IsLocalhost(u.Host)) +} + func (m *Manager) registerBC3Client(ctx context.Context, registrationEndpoint string, opts *LoginOptions) (*ClientCredentials, error) { + // The registration endpoint comes from the server-controlled discovery + // document. Restrict it to https (or http on loopback for local + // development) so a hostile discovery doc can't hand the DCR POST a + // file:// (or other) scheme that RequireSecureURL would let through. + // Mirrors buildAuthURL's scheme whitelist. + u, err := url.Parse(registrationEndpoint) + if err != nil { + return nil, output.ErrAuth(fmt.Sprintf("invalid registration endpoint %q: %v", registrationEndpoint, err)) + } + if !isSecureEndpointURL(u) { + return nil, output.ErrAuth(fmt.Sprintf("invalid registration endpoint %q: must be an absolute https URL (or http on loopback)", registrationEndpoint)) + } + customRedirect := opts.RedirectURI != defaultRedirectURI regReq := map[string]any{ "client_name": "basecamp-cli", @@ -643,8 +693,51 @@ func (m *Manager) registerBC3Client(ctx context.Context, registrationEndpoint st } req.Header.Set("Content-Type", "application/json") - resp, err := m.httpClient.Do(req) + // Use a dedicated client with its own CheckRedirect guard. The DCR POST body + // carries only client metadata (no auth code or refresh token), so following a + // proxy-canonicalized 3xx redirect is safe — and necessary, since the manager's + // guarded client would silently fail first-time login on such redirects. But Go + // replays the POST body on 307/308, so re-validate EACH hop's target with the + // same scheme rule applied to the registration endpoint; otherwise a hostile + // server could 307 the body to a file:// or non-loopback http:// URL, escaping + // the whitelist that only covered the original endpoint. + checkRedirect := func(req *http.Request, via []*http.Request) error { + if !isSecureEndpointURL(req.URL) { + return output.ErrAuth(fmt.Sprintf("refusing DCR redirect to %q: must be an absolute https URL (or http on loopback)", req.URL.String())) + } + // Only 307/308 preserve the POST method and body. On 301/302/303 Go + // downgrades the upcoming request (req here) to a body-less GET, so the + // registration would silently arrive empty and first-time login would + // fail confusingly. Refuse instead of resending as GET. + if req.Method != http.MethodPost { + return output.ErrAuth(fmt.Sprintf("refusing DCR redirect to %q: redirect downgraded the registration POST to %s, dropping the request body; the endpoint must redirect with 307/308", req.URL.String(), req.Method)) + } + // A redirect loop is a deterministic endpoint misconfiguration, not a + // transient network failure: return an auth-class error so the + // errors.As unwrap below surfaces it directly instead of masking it + // as a retryable output.ErrNetwork. + if len(via) >= 10 { + return output.ErrAuth(fmt.Sprintf("registration endpoint redirect loop: stopped after 10 redirects at %q", req.URL.String())) + } + return nil + } + var dcrClient *http.Client + if m.httpClient != nil { + c := *m.httpClient // http.Client has no locks; value copy is safe + c.CheckRedirect = checkRedirect + dcrClient = &c + } else { + dcrClient = &http.Client{Timeout: 30 * time.Second, CheckRedirect: checkRedirect} + } + resp, err := dcrClient.Do(req) if err != nil { + // A CheckRedirect rejection surfaces as a *url.Error wrapping our + // output.ErrAuth. Surface it directly rather than masking the security + // failure as a retryable network error. + var outErr *output.Error + if errors.As(err, &outErr) { + return nil, outErr + } return nil, output.ErrNetwork(err) } defer resp.Body.Close() @@ -701,7 +794,15 @@ func (m *Manager) saveBC3Client(creds *ClientCredentials) error { func (m *Manager) buildAuthURL(cfg *oauth.Config, oauthType, scope, state, codeChallenge, clientID string, opts *LoginOptions) (string, error) { u, err := url.Parse(cfg.AuthorizationEndpoint) if err != nil { - return "", err + return "", output.ErrAuth(fmt.Sprintf("invalid authorization endpoint %q: %v", cfg.AuthorizationEndpoint, err)) + } + + // The authorization endpoint comes from the server-controlled discovery + // document and is later dispatched to the OS browser handler (xdg-open / + // open). Restrict it to https (or http on loopback for local development) + // so a hostile discovery doc can't hand the OS a file:// (or other) URL. + if !isSecureEndpointURL(u) { + return "", output.ErrAuth(fmt.Sprintf("invalid authorization endpoint %q: must be an absolute https URL (or http on loopback)", cfg.AuthorizationEndpoint)) } q := u.Query() @@ -725,6 +826,19 @@ func (m *Manager) buildAuthURL(cfg *oauth.Config, oauthType, scope, state, codeC } func (m *Manager) exchangeCode(ctx context.Context, cfg *oauth.Config, oauthType, code, codeVerifier string, clientCreds *ClientCredentials, opts *LoginOptions) (*Credentials, error) { + // The token endpoint comes from the server-controlled discovery document + // and receives the authorization code plus client credentials. The SDK's + // RequireSecureEndpoint only checks scheme==https, which lets userinfo + // (https://legit@evil.com/token) and empty-host forms through. Apply the + // same strict check used for the registration and authorization endpoints. + u, err := url.Parse(cfg.TokenEndpoint) + if err != nil { + return nil, output.ErrAuth(fmt.Sprintf("invalid token endpoint %q: %v", cfg.TokenEndpoint, err)) + } + if !isSecureEndpointURL(u) { + return nil, output.ErrAuth(fmt.Sprintf("invalid token endpoint %q: must be an absolute https URL (or http on loopback)", cfg.TokenEndpoint)) + } + exchanger := oauth.NewExchanger(m.httpClient) req := oauth.ExchangeRequest{ diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 5af0e0b1a..336a79d1e 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -21,6 +21,7 @@ import ( "github.com/stretchr/testify/require" "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/output" ) // syncLogger is a thread-safe log collector for remote-mode login tests. @@ -592,6 +593,74 @@ func TestBuildAuthURL_UsesResolvedRedirectURI(t *testing.T) { assert.Contains(t, authURL, "redirect_uri=http%3A%2F%2Flocalhost%3A9999%2Fmy-callback") } +// TestBuildAuthURL_RejectsUnsafeScheme guards against a hostile discovery +// document handing the OS browser launcher a non-https authorization endpoint +// (e.g. file://). https and http-on-loopback are accepted; everything else +// must error before reaching OpenBrowser. +func TestBuildAuthURL_RejectsUnsafeScheme(t *testing.T) { + m := &Manager{cfg: config.Default(), httpClient: http.DefaultClient} + opts := &LoginOptions{RedirectURI: "http://localhost:9999/callback"} + + accepted := []string{ + "https://auth.example.com/authorize", + "http://localhost:3000/authorize", + "http://127.0.0.1:3000/authorize", + } + for _, endpoint := range accepted { + t.Run("accepts "+endpoint, func(t *testing.T) { + oauthCfg := &oauth.Config{AuthorizationEndpoint: endpoint} + _, err := m.buildAuthURL(oauthCfg, "bc3", "read", "state", "challenge", "cid", opts) + require.NoError(t, err) + }) + } + + rejected := []string{ + "file:///etc/passwd", + "http://evil.example.com/authorize", + "javascript:alert(1)", + "-flag", + "https:foo", // opaque form: right scheme, no host + "https://", // absolute form with empty host + "https://:3000/auth", // non-empty Host (":3000") but empty hostname + "http://:8080/auth", // port-only authority is not loopback + "https://evil.example@auth.example.com/authorize", // userinfo enables phishing display and Basic-auth synthesis + } + for _, endpoint := range rejected { + t.Run("rejects "+endpoint, func(t *testing.T) { + oauthCfg := &oauth.Config{AuthorizationEndpoint: endpoint} + _, err := m.buildAuthURL(oauthCfg, "bc3", "read", "state", "challenge", "cid", opts) + require.Error(t, err) + }) + } +} + +// TestBuildAuthURL_RejectsMalformedEndpoint guards the url.Parse failure path: +// a discovery-controlled AuthorizationEndpoint that url.Parse itself rejects +// must surface as an auth-class error (with its exit code and re-auth hint), +// matching the token/registration endpoint parse-error wraps — not leak the +// raw parse error. +func TestBuildAuthURL_RejectsMalformedEndpoint(t *testing.T) { + m := &Manager{cfg: config.Default(), httpClient: http.DefaultClient} + opts := &LoginOptions{RedirectURI: "http://localhost:9999/callback"} + + malformed := []string{ + "://bad", // missing protocol scheme + "https://\x7f", // control character url.Parse rejects + } + for _, endpoint := range malformed { + t.Run("rejects "+endpoint, func(t *testing.T) { + oauthCfg := &oauth.Config{AuthorizationEndpoint: endpoint} + _, err := m.buildAuthURL(oauthCfg, "bc3", "read", "state", "challenge", "cid", opts) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid authorization endpoint") + + var outErr *output.Error + require.ErrorAs(t, err, &outErr) + assert.Equal(t, output.CodeAuth, outErr.Code) + }) + } +} + func TestExchangeCode_UsesResolvedRedirectURI(t *testing.T) { // Capture the request body sent to the token endpoint var receivedBody string @@ -614,6 +683,68 @@ func TestExchangeCode_UsesResolvedRedirectURI(t *testing.T) { assert.Contains(t, receivedBody, "redirect_uri=http%3A%2F%2Flocalhost%3A7777%2Fcb") } +// recordingTransport fails every request and records that one was attempted, +// proving an endpoint rejection happened before any network POST. +type recordingTransport struct{ attempted atomic.Bool } + +func (t *recordingTransport) RoundTrip(*http.Request) (*http.Response, error) { + t.attempted.Store(true) + return nil, fmt.Errorf("unexpected network request") +} + +// unsafeTokenEndpoints are forms the SDK's scheme-only RequireSecureEndpoint +// would let through (or that only fail later in the transport), which the CLI +// must reject up front: userinfo smuggling, opaque no-host forms, and plain +// http off loopback. +var unsafeTokenEndpoints = []string{ + "https://legit@evil.com/token", // userinfo enables phishing display and Basic-auth synthesis + "https:foo", // opaque form: right scheme, no host + "http://evil.com/token", // http off loopback +} + +func TestExchangeCode_RejectsUnsafeTokenEndpoint(t *testing.T) { + clientCreds := &ClientCredentials{ClientID: "cid", ClientSecret: "csecret"} + opts := &LoginOptions{RedirectURI: "http://localhost:7777/cb"} + + for _, endpoint := range unsafeTokenEndpoints { + t.Run("rejects "+endpoint, func(t *testing.T) { + transport := &recordingTransport{} + m := &Manager{cfg: config.Default(), httpClient: &http.Client{Transport: transport}} + oauthCfg := &oauth.Config{TokenEndpoint: endpoint} + _, err := m.exchangeCode(context.Background(), oauthCfg, "launchpad", "code123", "", clientCreds, opts) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid token endpoint") + assert.False(t, transport.attempted.Load(), "must reject before any network POST") + }) + } +} + +func TestRefreshLocked_RejectsUnsafeTokenEndpoint(t *testing.T) { + t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "") + t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "") + + for _, endpoint := range unsafeTokenEndpoints { + t.Run("rejects "+endpoint, func(t *testing.T) { + transport := &recordingTransport{} + m := &Manager{ + cfg: config.Default(), + httpClient: &http.Client{Transport: transport}, + store: newTestStore(t, t.TempDir()), + } + creds := &Credentials{ + AccessToken: "old-token", + RefreshToken: "old-refresh", + OAuthType: "launchpad", + TokenEndpoint: endpoint, + } + err := m.refreshLocked(context.Background(), "test", creds) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid token endpoint") + assert.False(t, transport.attempted.Load(), "must reject before any network POST") + }) + } +} + func TestRegisterBC3Client_UsesResolvedRedirectURI(t *testing.T) { var receivedBody map[string]any srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -697,6 +828,231 @@ func TestRegisterBC3Client_DefaultRedirectPersisted(t *testing.T) { assert.NoError(t, statErr, "client.json should be written for default redirect URI") } +// TestRegisterBC3Client_RejectsUnsafeScheme guards against a hostile discovery +// document handing the DCR POST a non-https registration endpoint (e.g. +// file://). https and http-on-loopback are accepted; everything else must error +// before any request is made. Mirrors buildAuthURL's scheme whitelist. +func TestRegisterBC3Client_RejectsUnsafeScheme(t *testing.T) { + m := &Manager{cfg: config.Default(), httpClient: http.DefaultClient} + opts := &LoginOptions{RedirectURI: defaultRedirectURI} + + rejected := []string{ + "file:///etc/passwd", + "http://evil.example.com/register", + "ftp://evil.example.com/register", + "javascript:alert(1)", + "data:text/html,foo", + "https:foo", // opaque form: right scheme, no host + "https://", // absolute form with empty host + "https://:3000/register", // non-empty Host (":3000") but empty hostname + "http://:8080/register", // port-only authority is not loopback + "https://evil.example@auth.example.com/register", // userinfo enables phishing display and Basic-auth synthesis + } + for _, endpoint := range rejected { + t.Run("rejects "+endpoint, func(t *testing.T) { + _, err := m.registerBC3Client(context.Background(), endpoint, opts) + require.Error(t, err) + assert.Contains(t, err.Error(), "registration endpoint") + }) + } +} + +// TestRegisterBC3Client_FollowsRedirect verifies the DCR POST follows a +// proxy-canonicalized 3xx on the registration endpoint (rather than silently +// failing under the manager's GET-only guard) when the redirect target stays +// within the secure-endpoint whitelist — here a loopback http:// hop. The DCR +// body carries only client metadata, so following such a redirect is safe. +func TestRegisterBC3Client_FollowsRedirect(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/register", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "/register-canonical", http.StatusTemporaryRedirect) + }) + mux.HandleFunc("/register-canonical", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"client_id":"dcr-id","client_secret":"dcr-secret"}`) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + tmpDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmpDir) + + // Manager carries a guarded client (as appctx wires it) to prove the DCR + // path uses its own unguarded client rather than m.httpClient. + guarded := srv.Client() + guarded.CheckRedirect = func(_ *http.Request, via []*http.Request) error { + if len(via) > 0 && via[0].Method != http.MethodGet && via[0].Method != http.MethodHead { + return http.ErrUseLastResponse + } + return nil + } + m := &Manager{ + cfg: config.Default(), + httpClient: guarded, + store: newTestStore(t, tmpDir), + } + opts := &LoginOptions{RedirectURI: "http://localhost:7777/cb"} + + creds, err := m.registerBC3Client(context.Background(), srv.URL+"/register", opts) + require.NoError(t, err) + assert.Equal(t, "dcr-id", creds.ClientID) +} + +// TestRegisterBC3Client_FollowsHTTPSRedirect verifies an https redirect hop is +// followed: the scheme stays within the secure-endpoint whitelist, so the +// re-validation in CheckRedirect must not reject it. +func TestRegisterBC3Client_FollowsHTTPSRedirect(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/register", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "/register-canonical", http.StatusTemporaryRedirect) + }) + mux.HandleFunc("/register-canonical", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"client_id":"dcr-id","client_secret":"dcr-secret"}`) + }) + srv := httptest.NewTLSServer(mux) + defer srv.Close() + + tmpDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmpDir) + + m := &Manager{ + cfg: config.Default(), + httpClient: srv.Client(), // trusts the test server's TLS cert + store: newTestStore(t, tmpDir), + } + opts := &LoginOptions{RedirectURI: "http://localhost:7777/cb"} + + creds, err := m.registerBC3Client(context.Background(), srv.URL+"/register", opts) + require.NoError(t, err) + assert.Equal(t, "dcr-id", creds.ClientID) +} + +// TestRegisterBC3Client_RejectsUnsafeRedirect guards against a hostile server +// 307/308-redirecting the DCR POST (body and all) to a scheme/host outside the +// secure-endpoint whitelist that was only enforced on the original endpoint. +// Each redirect hop must be re-validated, so file:// and non-loopback http:// +// targets are rejected before the body is replayed. +func TestRegisterBC3Client_RejectsUnsafeRedirect(t *testing.T) { + targets := map[string]string{ + "file scheme": "file:///etc/passwd", + "non-loopback http": "http://evil.example.com/register", + "other scheme": "ftp://evil.example.com/register", + } + for name, target := range targets { + t.Run(name, func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/register", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, target, http.StatusTemporaryRedirect) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + tmpDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmpDir) + + m := &Manager{ + cfg: config.Default(), + httpClient: srv.Client(), + store: newTestStore(t, tmpDir), + } + opts := &LoginOptions{RedirectURI: "http://localhost:7777/cb"} + + _, err := m.registerBC3Client(context.Background(), srv.URL+"/register", opts) + require.Error(t, err) + assert.Contains(t, err.Error(), "redirect") + + // The rejection is a security failure, not a transient network + // error: it must surface as an auth-class error (with its exit + // code and re-auth hint), not be masked as a retryable ErrNetwork. + var outErr *output.Error + require.ErrorAs(t, err, &outErr) + assert.Equal(t, output.CodeAuth, outErr.Code) + }) + } +} + +// TestRegisterBC3Client_RejectsMethodDowngradeRedirect guards against a +// 301/302/303 on the registration endpoint: Go follows those by downgrading +// the POST to a body-less GET, so the DCR registration would silently arrive +// empty at the target. The guard must reject the downgraded hop with an +// auth-class error, and the redirect target must never see the GET. +func TestRegisterBC3Client_RejectsMethodDowngradeRedirect(t *testing.T) { + for _, status := range []int{http.StatusMovedPermanently, http.StatusFound, http.StatusSeeOther} { + t.Run(http.StatusText(status), func(t *testing.T) { + var targetHits atomic.Int32 + mux := http.NewServeMux() + mux.HandleFunc("/register", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "/register-canonical", status) + }) + mux.HandleFunc("/register-canonical", func(w http.ResponseWriter, r *http.Request) { + targetHits.Add(1) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + tmpDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmpDir) + + m := &Manager{ + cfg: config.Default(), + httpClient: srv.Client(), + store: newTestStore(t, tmpDir), + } + opts := &LoginOptions{RedirectURI: "http://localhost:7777/cb"} + + _, err := m.registerBC3Client(context.Background(), srv.URL+"/register", opts) + require.Error(t, err) + assert.Contains(t, err.Error(), "downgraded the registration POST") + + // Security failure, not a transient network error: must surface as + // an auth-class error, same as the unsafe-redirect rejections. + var outErr *output.Error + require.ErrorAs(t, err, &outErr) + assert.Equal(t, output.CodeAuth, outErr.Code) + + assert.Zero(t, targetHits.Load(), "redirect target must never receive the body-less GET") + }) + } +} + +// TestRegisterBC3Client_StopsRedirectLoop guards against a looping endpoint that +// keeps issuing same-scheme (loopback http) redirects: each hop passes the +// secure-endpoint re-validation, so without a hop cap the DCR client would spin +// until the 30s timeout. The cap must fail fast after 10 redirects. +func TestRegisterBC3Client_StopsRedirectLoop(t *testing.T) { + var hops atomic.Int32 + mux := http.NewServeMux() + mux.HandleFunc("/register", func(w http.ResponseWriter, r *http.Request) { + hops.Add(1) + http.Redirect(w, r, "/register", http.StatusTemporaryRedirect) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + tmpDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmpDir) + + m := &Manager{ + cfg: config.Default(), + httpClient: srv.Client(), + store: newTestStore(t, tmpDir), + } + opts := &LoginOptions{RedirectURI: "http://localhost:7777/cb"} + + _, err := m.registerBC3Client(context.Background(), srv.URL+"/register", opts) + 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") + + // A redirect loop is a deterministic endpoint misconfiguration, not a + // transient network failure: it must surface as an auth-class error, not + // be masked as a retryable ErrNetwork. + var outErr *output.Error + require.ErrorAs(t, err, &outErr) + assert.Equal(t, output.CodeAuth, outErr.Code) +} + func TestLoadClientCredentials_BC3_CustomRedirect_SkipsStoredClient(t *testing.T) { // DCR server srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -1463,6 +1819,78 @@ 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. +func guardedClient() *http.Client { + return &http.Client{ + Timeout: 30 * time.Second, + CheckRedirect: func(_ *http.Request, via []*http.Request) error { + if len(via) > 0 && via[0].Method != http.MethodGet && via[0].Method != http.MethodHead { + return http.ErrUseLastResponse + } + return nil + }, + } +} + +// TestRefreshLocked_GuardBlocksCredentialPOSTRedirect proves the guarded client +// refuses to follow a 307/308 redirect on the token-refresh POST. The token +// endpoint sets GetBody (the body is a *strings.Reader), so without the guard +// Go would replay the refresh_token to the redirect target — which is only +// origin-validated on the initial endpoint, not on redirect hops. The guard +// must turn the redirect into the last response so refreshLocked errors out and +// the second handler never sees the credential body. +func TestRefreshLocked_GuardBlocksCredentialPOSTRedirect(t *testing.T) { + t.Setenv("BASECAMP_OAUTH_CLIENT_ID", "") + t.Setenv("BASECAMP_OAUTH_CLIENT_SECRET", "") + + for name, status := range map[string]int{ + "307 temporary": http.StatusTemporaryRedirect, + "308 permanent": http.StatusPermanentRedirect, + } { + t.Run(name, func(t *testing.T) { + var replayed atomic.Bool + mux := http.NewServeMux() + mux.HandleFunc("/authorization/token", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "/stolen", status) + }) + mux.HandleFunc("/stolen", func(w http.ResponseWriter, r *http.Request) { + // If the guard ever fails, the credential POST lands here. + replayed.Store(true) + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `{"access_token":"leaked","refresh_token":"leaked"}`) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + tmpDir := t.TempDir() + cfg := config.Default() + cfg.BaseURL = srv.URL + + m := &Manager{ + cfg: cfg, + httpClient: guardedClient(), + store: newTestStore(t, tmpDir), + } + + creds := &Credentials{ + AccessToken: "old-token", + RefreshToken: "secret-refresh", + OAuthType: "launchpad", + TokenEndpoint: srv.URL + "/authorization/token", + ExpiresAt: time.Now().Add(-1 * time.Hour).Unix(), + } + require.NoError(t, m.store.Save(srv.URL, creds)) + + err := m.refreshLocked(context.Background(), srv.URL, creds) + require.Error(t, err, "refresh must fail rather than follow the credential POST redirect") + assert.False(t, replayed.Load(), + "guard must block the redirect: the refresh_token POST must not reach the redirect target") + }) + } +} + func TestRefreshLocked_BC3SendsClientID(t *testing.T) { var mu sync.Mutex var capturedBody string diff --git a/internal/cli/harden_ancestorflag_linux.go b/internal/cli/harden_ancestorflag_linux.go new file mode 100644 index 000000000..4eb5f8dc2 --- /dev/null +++ b/internal/cli/harden_ancestorflag_linux.go @@ -0,0 +1,19 @@ +//go:build linux + +package cli + +import "golang.org/x/sys/unix" + +// ancestorAccessFlag is the access mode used to open the path components during +// the harden descent — every ancestor AND the final target. On Linux we use +// O_PATH so a traverse-only ancestor (execute/search but not read, e.g. /home at +// 0711 on hardened distros) can still be opened: O_PATH needs only search +// permission, not read, where O_RDONLY on a directory would fail with EACCES and +// abort the whole harden. It also lets the FINAL target be opened even when its +// owner-read bit is cleared (e.g. an euid-owned 0333/0377 dir), so an +// owner-unreadable but still group/world-writable config dir can be verified and +// tightened rather than failing the open. An O_PATH fd is still usable as the +// dirfd for the next Openat and can be Fstat'd. It cannot be Fchmod'd, so the +// target is tightened via unix.Fchmodat by name on the vetted parent fd, not by +// Fchmod on its own fd. +const ancestorAccessFlag = unix.O_PATH diff --git a/internal/cli/harden_ancestorflag_other.go b/internal/cli/harden_ancestorflag_other.go new file mode 100644 index 000000000..88ecaac31 --- /dev/null +++ b/internal/cli/harden_ancestorflag_other.go @@ -0,0 +1,15 @@ +//go:build unix && !linux + +package cli + +import "syscall" + +// ancestorAccessFlag is the access mode used to open the path components during +// the harden descent — every ancestor and the final target. unix.O_PATH is a +// Linux-only flag and is not defined for every non-Linux unix GOOS by +// golang.org/x/sys/unix, so on those platforms we fall back to O_RDONLY. This +// means a traverse-only ancestor (execute but not read) will fail to open and +// abort the harden on non-Linux, and an owner-unreadable target (owner-read bit +// cleared) likewise fails its open — acceptable limitations, since Linux is the +// primary/CI platform and the one where 0711 ancestors (e.g. /home) are common. +const ancestorAccessFlag = syscall.O_RDONLY diff --git a/internal/cli/harden_ctime_bsd_test.go b/internal/cli/harden_ctime_bsd_test.go new file mode 100644 index 000000000..bdb12da89 --- /dev/null +++ b/internal/cli/harden_ctime_bsd_test.go @@ -0,0 +1,11 @@ +//go:build darwin || ios || freebsd || netbsd + +package cli + +import "syscall" + +// ctimespec returns the inode change time from a Stat_t. The BSD-derived unix +// GOOSes (darwin/ios/freebsd/netbsd) name the field Ctimespec rather than +// Linux's Ctim, so this helper is build-tagged to keep the idempotence test +// portable. +func ctimespec(st *syscall.Stat_t) syscall.Timespec { return st.Ctimespec } diff --git a/internal/cli/harden_ctime_linux_test.go b/internal/cli/harden_ctime_linux_test.go new file mode 100644 index 000000000..ffb7241ff --- /dev/null +++ b/internal/cli/harden_ctime_linux_test.go @@ -0,0 +1,10 @@ +//go:build linux + +package cli + +import "syscall" + +// ctimespec returns the inode change time from a Stat_t. The field is named +// Ctim on Linux and Ctimespec on the BSD-derived unix GOOSes, so this helper is +// build-tagged to keep the idempotence test portable. +func ctimespec(st *syscall.Stat_t) syscall.Timespec { return st.Ctim } diff --git a/internal/cli/harden_ctime_unix_test.go b/internal/cli/harden_ctime_unix_test.go new file mode 100644 index 000000000..05ce6e5eb --- /dev/null +++ b/internal/cli/harden_ctime_unix_test.go @@ -0,0 +1,13 @@ +//go:build unix && !linux && !darwin && !ios && !freebsd && !netbsd && !aix && !hurd + +package cli + +import "syscall" + +// ctimespec returns the inode change time from a Stat_t. This catch-all covers +// the remaining unix GOOSes that name the field Ctim with a syscall.Timespec +// type (android, dragonfly, openbsd, solaris, illumos), so we need not +// enumerate each one. aix is excluded because its Ctim is an StTimespec_t +// rather than a syscall.Timespec, and hurd is excluded because it has no +// syscall.Stat_t; the cli package's tests do not build on those two targets. +func ctimespec(st *syscall.Stat_t) syscall.Timespec { return st.Ctim } diff --git a/internal/cli/harden_other.go b/internal/cli/harden_other.go new file mode 100644 index 000000000..331b56650 --- /dev/null +++ b/internal/cli/harden_other.go @@ -0,0 +1,7 @@ +//go:build !unix + +package cli + +// hardenConfigDir is a no-op on non-Unix platforms (Windows, plan9, js/wasm), +// where the Unix permission and ownership model does not apply. +func hardenConfigDir(string) {} diff --git a/internal/cli/harden_unix.go b/internal/cli/harden_unix.go new file mode 100644 index 000000000..0f851fb4a --- /dev/null +++ b/internal/cli/harden_unix.go @@ -0,0 +1,254 @@ +//go:build unix + +package cli + +import ( + "io/fs" + "os" + "path/filepath" + "strings" + "syscall" + + "golang.org/x/sys/unix" +) + +// hardenConfigDir tightens dir to 0700 using a step-by-step openat descent that +// refuses a symlink at EVERY path component. Starting from an fd on "/", it +// walks the path one component at a time with +// Openat(parentFd, comp, ancestorAccessFlag|O_DIRECTORY|O_NOFOLLOW|O_NONBLOCK|O_CLOEXEC): +// +// - EVERY component — ancestors (root "/", every intermediate dir, the +// immediate parent) AND the final target — is opened traverse-only: +// ancestorAccessFlag is unix.O_PATH on Linux and O_RDONLY on other unix +// GOOSes. O_PATH needs only execute/search permission, not read, so an +// execute-only ancestor (e.g. /home at 0711 on hardened distros) can still +// be descended — with O_RDONLY it would fail EACCES and abort the whole +// harden, leaving the config dir world-listable for exactly those users. It +// also makes the final-target verify MODE-INDEPENDENT: an owner-unreadable +// but euid-owned config dir (owner-read bit cleared but still group/world- +// writable, e.g. 0333/0377) can still be Fstat'd and tightened, whereas an +// O_RDONLY target open would fail EACCES and leave that credential-bearing +// dir writable by others. An O_PATH fd is still valid as the next Openat +// dirfd and can be Fstat'd for the ancestorIsSafe vetting. O_PATH|O_NOFOLLOW +// still refuses a symlinked component. An O_PATH fd cannot be Fchmod'd, so +// the target's tighten is done via Fchmodat by NAME on the still-open, +// ancestor-vetted parent fd (see below). +// - O_NOFOLLOW on each hop makes a symlinked component fail with ELOOP. A +// single os.OpenFile(dir, O_NOFOLLOW) only rejects a symlinked FINAL +// component; a symlinked ANCESTOR in an attacker-writable location (e.g. +// XDG_CONFIG_HOME=/tmp/attacker/basecamp where the attacker owns +// /tmp/attacker) is still followed at open time, letting the attacker aim +// an ancestor at a different euid-owned dir and have the fchmod tighten +// THAT dir. Refusing a symlink at every hop closes that vector: no ancestor +// can redirect the final fd. +// - O_DIRECTORY makes every component fail unless it is a real directory +// (also rejects a FIFO or regular file planted anywhere on the path). +// - O_NONBLOCK guarantees the open never blocks — without it, a pre-planted +// named pipe on the path would hang until a writer appeared, stalling every +// command. O_CLOEXEC is hygiene so a transient fd can't leak across an exec. +// +// ancestorIsSafe applies to the ANCESTORS only — the root "/", every +// intermediate directory, AND the config dir's immediate PARENT — but NEVER to +// the final target itself. As each ancestor fd opens it is fstat'd and run +// through ancestorIsSafe BEFORE its fd is closed. This closes a residual +// rename-swap vector that O_NOFOLLOW alone does not: under a group/world-writable +// NON-sticky ancestor (e.g. XDG_CONFIG_HOME=/shared at 0777), a foreign user with +// write access to that parent can rename() a real euid-owned directory into the +// config path between our openat hops. O_NOFOLLOW does not reject a renamed REAL +// directory, so without this check the ownership test would pass and the fchmod +// would tighten the substituted victim-owned dir. The check aborts (no chmod) on +// any ancestor fd whose directory is either group/world-writable without the +// sticky bit, or owned by a foreign non-root uid. A sticky world-writable parent +// like /tmp (1777) stays SAFE — the sticky bit prevents renaming or deleting +// others' entries — so a config dir under /tmp is still hardened. Root-owned +// (uid 0) and self-owned (euid) ancestors are trusted system directories. Because +// every check runs on an already-open fd reached without following any symlink, +// there is no path re-resolution and thus no TOCTOU window. Vetting the immediate +// PARENT is what defeats the rename-swap: a safe parent cannot have a foreign +// entry renamed into the target slot. +// +// The final target is NOT run through ancestorIsSafe: its own permissiveness is +// precisely the thing being tightened (a config dir left at 0775/0777 is the +// case this feature exists to clamp to 0700), so rejecting it for being writable +// would defeat the purpose. It is opened traverse-only (O_PATH on Linux) with the +// same O_NOFOLLOW|O_DIRECTORY flags (still no symlink, still a real dir) and gated +// ONLY on: Fstat succeeds, it is a directory, and it is owned by the current +// effective user (ancestors may be root-owned; the target must be ours). Opening +// traverse-only rather than O_RDONLY means an owner-unreadable target can still be +// Fstat'd and verified. The tighten is skipped entirely when the dir is already +// 0700 (the steady state) so we don't churn the inode ctime or spend a syscall on +// every command run; otherwise it is done via Fchmodat by name on the vetted +// parent fd — an O_PATH fd cannot be Fchmod'd directly. Fchmodat-by-name is safe +// because the parent is ancestor-vetted (no attacker can rename within it) and the +// target was opened O_NOFOLLOW (not a symlink), so the name resolves to exactly the +// inode we Fstat'd. Fchmodat uses flags 0, NOT AT_SYMLINK_NOFOLLOW: Linux's +// fchmodat(2) rejects AT_SYMLINK_NOFOLLOW for a chmod with ENOTSUP, and the swap +// safety above does not rely on it. Because every check and the chmod resolve +// through fds reached without following any symlink, no path component can be +// swapped in between (no Lstat->Chmod TOCTOU race). Non-Linux caveat: where +// ancestorAccessFlag falls back to O_RDONLY, the target open still needs owner +// read — a documented pre-existing limitation, not addressed here. +// +// Best-effort: ANY failure at ANY step (symlink, FIFO/non-dir, missing dir, +// permission, foreign owner, unsafe ancestor, fchmod error) silently returns +// without chmod, never blocking startup on a perms cleanup. The caller (root.go) +// guarantees dir is absolute; dir == "/" (no components) is skipped, since +// hardening the root is neither needed nor safe. +func hardenConfigDir(dir string) { + components := splitPathComponents(dir) + if len(components) == 0 { + return // dir == "/" or empty: nothing to harden + } + + // EVERY component — ANCESTORS and the FINAL target alike — is opened + // traverse-only (ancestorAccessFlag is unix.O_PATH on Linux, O_RDONLY + // elsewhere) so an execute-only ancestor (e.g. /home at 0711, no read) does + // not abort the harden, and so an owner-UNREADABLE target (owner-read bit + // cleared but still group/world-writable, e.g. 0333/0377) can still be + // Fstat'd and tightened rather than failing the open with EACCES. O_NOFOLLOW + // still refuses a symlinked component and O_DIRECTORY a non-dir; an O_PATH fd + // is still usable as the next Openat dirfd and can be Fstat'd for the + // ancestorIsSafe vetting. The target's tighten cannot use Fchmod on its own + // O_PATH fd (O_PATH fds cannot Fchmod), so it is done via Fchmodat by name on + // the still-open, vetted parent fd (see below). + const ancestorFlags = ancestorAccessFlag | syscall.O_DIRECTORY | syscall.O_NOFOLLOW | syscall.O_NONBLOCK | syscall.O_CLOEXEC + + euid := os.Geteuid() + + fd, err := syscall.Open("/", ancestorFlags, 0) + if err != nil { + return + } + // The root "/" fd counts as an ancestor: verify it before descending. + var st syscall.Stat_t + if err := syscall.Fstat(fd, &st); err != nil || !ancestorIsSafe(&st, euid) { + _ = syscall.Close(fd) //nolint:errcheck // read-only fd; nothing to flush + return + } + + // Descend one component at a time through the ANCESTORS ONLY + // (components[:len(components)-1]): every element except the final target, + // which includes the config dir's immediate parent. Each hop opens the child + // relative to the current dir fd, then closes the parent. A symlinked (ELOOP) + // or non-dir (ENOTDIR) component fails the Openat and aborts with no chmod. + // Every opened ancestor fd is fstat'd and vetted by ancestorIsSafe before it + // is closed, so a directory renamed into the path under a mutable parent is + // rejected before the fchmod. The final target is opened after the loop and + // is deliberately NOT run through ancestorIsSafe (see the doc comment). + for _, comp := range components[:len(components)-1] { + // unix.Openat (not syscall.Openat, which the stdlib exports only on + // linux) keeps this portable across every unix GOOS. golang.org/x/sys + // is already a direct dependency, so this adds no new module. + next, err := unix.Openat(fd, comp, ancestorFlags, 0) + _ = syscall.Close(fd) //nolint:errcheck // read-only fd; nothing to flush + if err != nil { + return + } + fd = next + if err := syscall.Fstat(fd, &st); err != nil || !ancestorIsSafe(&st, euid) { + _ = syscall.Close(fd) //nolint:errcheck // read-only fd; nothing to flush + return + } + } + + // After the ancestor loop, fd is the config dir's immediate PARENT — the last + // vetted ancestor. Keep it OPEN until after the chmod: the tighten is done via + // Fchmodat by name on this fd, not Fchmod on the target's own fd. The + // single-component edge case (e.g. dir == "/x") lands here directly: the loop + // above ran zero times and fd is still the vetted root, which IS this target's + // parent — the same code path works. + parentFd := fd + defer syscall.Close(parentFd) //nolint:errcheck // read-only fd; nothing to flush + + // Open the FINAL component (the target) relative to its vetted parent with + // the SAME traverse-only flags as the ancestors. Opening traverse-only makes + // the verify+tighten MODE-INDEPENDENT: an owner-unreadable but euid-owned + // config dir (owner-read bit cleared, still group/world-writable) can still be + // Fstat'd and tightened, whereas an O_RDONLY open would fail EACCES and leave + // that credential-bearing dir writable by others. O_NOFOLLOW keeps a symlinked + // target out; O_DIRECTORY keeps a FIFO/non-dir out. + last := components[len(components)-1] + targetFd, err := unix.Openat(parentFd, last, ancestorFlags, 0) + if err != nil { + return + } + defer syscall.Close(targetFd) //nolint:errcheck // read-only fd; nothing to flush + + // Fstat works on an O_PATH fd. The target is deliberately NOT run through + // ancestorIsSafe — its own permissiveness is precisely what we tighten — so + // gate ONLY on: it is a directory and owned by the current euid (ancestors may + // be root-owned; the target must be ours). + if err := syscall.Fstat(targetFd, &st); err != nil { + return + } + if st.Mode&syscall.S_IFMT != syscall.S_IFDIR || st.Uid != uint32(euid) { //nolint:gosec // G115: Geteuid() returns a valid non-negative uid that fits in uint32 + return + } + // Idempotent: only chmod when the perm actually differs from 0700. When the + // dir is already 0700 (the steady state after the first run), skip the syscall + // so we don't churn the inode ctime on every command invocation. + if os.FileMode(st.Mode).Perm() == 0o700 { + return + } + // Tighten by NAME on the vetted parent fd, since an O_PATH target fd cannot be + // Fchmod'd. This is safe against a symlink/rename swap: the parent passed + // ancestorIsSafe (not writable-by-others without sticky, not foreign-owned) so + // no attacker can rename within it, and the target was just opened O_NOFOLLOW + // confirming it is not a symlink — so Fchmodat-by-name hits exactly the inode + // we Fstat'd. Flags are 0, NOT AT_SYMLINK_NOFOLLOW: Linux's fchmodat(2) rejects + // AT_SYMLINK_NOFOLLOW for a chmod with ENOTSUP, and the swap safety above does + // not depend on it. + // + // On non-Linux unix, ancestorAccessFlag is O_RDONLY, so the target open above + // still needs owner read there — a documented pre-existing limitation of the + // O_RDONLY fallback, not addressed here. + _ = unix.Fchmodat(parentFd, last, 0o700, 0) //nolint:gosec // G302: 0700 is correct for a directory (needs the execute bit) that can hold credentials.json +} + +// ancestorIsSafe reports whether a directory (identified by st) on the config +// path is safe to descend through toward an fchmod. A directory is UNSAFE and +// must abort the harden if it is: +// +// - group- or world-writable without the sticky bit (st.Mode&0o022 != 0 && +// st.Mode&unix.S_ISVTX == 0): a foreign user could rename a real euid-owned +// directory into the path here, and O_NOFOLLOW would not reject the swap. A +// sticky world-writable dir (e.g. /tmp at 1777) is SAFE because the sticky +// bit forbids renaming or deleting entries owned by others. +// - owned by a foreign non-root uid (st.Uid != euid && st.Uid != 0): its owner +// could perform the same swap. Root-owned (uid 0) and self-owned (euid) +// directories are trusted. +func ancestorIsSafe(st *syscall.Stat_t, euid int) bool { + if st.Mode&0o022 != 0 && st.Mode&unix.S_ISVTX == 0 { + return false + } + if st.Uid != uint32(euid) && st.Uid != 0 { //nolint:gosec // G115: Geteuid() returns a valid non-negative uid that fits in uint32 + return false + } + return true +} + +// splitPathComponents splits an absolute path into its non-empty components +// after cleaning it, so the descent can open each one in turn. The leading +// empty element from the root "/" (and any redundant separators) is dropped; +// "/" yields no components. +func splitPathComponents(dir string) []string { + parts := strings.Split(filepath.Clean(dir), "/") + components := parts[:0] + for _, p := range parts { + if p != "" { + components = append(components, p) + } + } + return components +} + +// ownedByUID reports whether fi is owned by uid. A FileInfo whose Sys() is +// not a *syscall.Stat_t cannot prove ownership, so it is treated as not +// owned (fail closed). +func ownedByUID(fi fs.FileInfo, uid int) bool { + st, ok := fi.Sys().(*syscall.Stat_t) + if !ok { + return false + } + return st.Uid == uint32(uid) //nolint:gosec // G115: Geteuid() returns a valid non-negative uid that fits in uint32 +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 3c8b89986..47e20dfd8 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -44,6 +44,24 @@ func NewRootCmd() *cobra.Command { return nil } + // Tighten global config dir perms: an older CLI version (or the + // skill/update bootstrap) may have created it world-listable at + // 0755, yet it can hold credentials.json. Best-effort, runs before + // anything writes into the dir. On Unix, hardenConfigDir walks the + // path component-by-component with openat(O_NOFOLLOW), fstat-vetting + // each ancestor (rejecting symlink/rename swaps and writable or + // foreign-owned ancestors), then tightens the target via fchmodat on + // the vetted parent fd — so no swap can redirect the chmod. On + // non-Unix platforms (harden_other.go) it's a no-op, since the Unix + // permission model doesn't apply. + // Only harden an absolute path: a relative XDG_CONFIG_HOME would + // resolve against the cwd, so chmod'ing it would surprise-tighten + // an arbitrary cwd-relative dir. Treat non-absolute as unsafe and + // skip it (matching the pre-rewrite hasWritableAncestor guard). + if cfgDir := config.GlobalConfigDir(); cfgDir != "" && filepath.IsAbs(cfgDir) { + hardenConfigDir(cfgDir) + } + // Start background update check early so it runs during command execution updateCheck = commands.StartUpdateCheck() @@ -98,7 +116,13 @@ func NewRootCmd() *cobra.Command { return err } // Re-apply env and flag overrides (they take precedence over profile values) - config.LoadFromEnv(cfg) + if err := config.LoadFromEnv(cfg); err != nil { + if bareRoot { + initBareRootApp(cfg) + return nil + } + return err + } config.ApplyOverrides(cfg, config.FlagOverrides{ Account: flags.Account, Project: flags.Project, @@ -128,6 +152,12 @@ func NewRootCmd() *cobra.Command { } } + // Note: llm_endpoint is deliberately NOT validated here. It is + // consumed only by the dev-gated TUI's summarize path, which + // fail-closes via summarize.ValidateEndpoint in + // workspace.NewSession — validating it at startup would brick + // unrelated commands over a value they never use. + // Resolve behavior preferences: explicit flag > config > version.IsDev() resolvePreferences(cmd, cfg, &flags) diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index ad8a02035..636d03c78 100644 --- a/internal/cli/root_test.go +++ b/internal/cli/root_test.go @@ -15,6 +15,31 @@ import ( "github.com/basecamp/basecamp-cli/internal/version" ) +// TestBadLLMEndpointDoesNotBlockUnrelatedCommands is a regression test for +// the startup-validation lockout: llm_endpoint is consumed only by the +// dev-gated TUI's summarize path (which fail-closes via +// summarize.ValidateEndpoint in workspace.NewSession — see TestValidateEndpoint +// there), so PersistentPreRunE must not reject an endpoint that would fail +// that validation. Here the openai provider + API key + plain-http remote +// endpoint is the worst case, and an ordinary command still runs. +func TestBadLLMEndpointDoesNotBlockUnrelatedCommands(t *testing.T) { + isolateRootTest(t) + t.Setenv("BASECAMP_LLM_PROVIDER", "openai") + t.Setenv("BASECAMP_LLM_API_KEY", "secret") + t.Setenv("BASECAMP_LLM_ENDPOINT", "http://remote-host:1234") + + root := NewRootCmd() + root.AddCommand(&cobra.Command{ + Use: "noop", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + }) + root.SetOut(&bytes.Buffer{}) + root.SetErr(&bytes.Buffer{}) + root.SetArgs([]string{"noop"}) + + require.NoError(t, root.Execute()) +} + func TestResolvePreferences(t *testing.T) { boolPtr := func(b bool) *bool { return &b } intPtr := func(i int) *int { return &i } diff --git a/internal/cli/root_unix_test.go b/internal/cli/root_unix_test.go new file mode 100644 index 000000000..c25cf0aa8 --- /dev/null +++ b/internal/cli/root_unix_test.go @@ -0,0 +1,394 @@ +//go:build unix + +package cli + +import ( + "io/fs" + "os" + "path/filepath" + "runtime" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" + + "github.com/basecamp/basecamp-cli/internal/config" +) + +// TestHardenConfigDir exercises the step-by-step openat descent that hardens +// the global config dir (open "/", Openat each component with O_NOFOLLOW, +// then fstat -> fchmod the final fd). Refusing a symlink at every component +// means neither a symlinked final component nor a symlinked ANCESTOR can +// redirect the fchmod, and a dir under a sticky world-writable parent like +// /tmp is still hardened like any other. +func TestHardenConfigDir(t *testing.T) { + mode := func(t *testing.T, path string) os.FileMode { + t.Helper() + fi, err := os.Stat(path) + require.NoError(t, err) + return fi.Mode().Perm() + } + + t.Run("tightens a normal config dir to 0700", func(t *testing.T) { + dir := filepath.Join(t.TempDir(), "basecamp") + require.NoError(t, os.Mkdir(dir, 0o755)) + require.NoError(t, os.Chmod(dir, 0o755)) + + hardenConfigDir(dir) + + assert.Equal(t, os.FileMode(0o700), mode(t, dir)) + }) + + t.Run("group/world-writable target under a safe parent is tightened to 0700", func(t *testing.T) { + // Regression: the target dir's OWN permissiveness (0775/0777) is exactly + // the case this feature exists to clamp to 0700. ancestorIsSafe must gate + // only the ANCESTORS, never the final target — otherwise a group- or + // world-writable config dir (the most permissive, most-in-need case) would + // be rejected as an "unsafe ancestor" and left untightened. The parent here + // is a normal 0755 euid-owned temp dir (safe), so only the target's own mode + // is in play. + for _, targetMode := range []os.FileMode{0o775, 0o777} { + parent := t.TempDir() // default 0700-ish, euid-owned, non-writable by others + require.NoError(t, os.Chmod(parent, 0o755)) + dir := filepath.Join(parent, "basecamp") + require.NoError(t, os.Mkdir(dir, targetMode)) + require.NoError(t, os.Chmod(dir, targetMode)) + + hardenConfigDir(dir) + + assert.Equal(t, os.FileMode(0o700), mode(t, dir), + "target with mode %#o under a safe parent must be tightened", targetMode) + } + }) + + t.Run("owner-unreadable target under a safe parent is tightened to 0700", func(t *testing.T) { + // Gap this fix closes: an euid-owned config dir with the owner-READ bit + // cleared but still group/world-writable (e.g. 0333 or 0377) is exactly a + // credential-bearing dir left writable by others. The predecessor opened the + // FINAL target O_RDONLY, so this dir failed the open with EACCES and the + // ownership check + tighten never ran — it stayed writable. Opening the + // target traverse-only (O_PATH on Linux) and tightening via Fchmodat on the + // vetted parent fd makes the verify+tighten mode-independent, so the dir is + // clamped to 0700. + // + // Linux-only: the fix relies on O_PATH, which lets an owner-unreadable dir + // be opened for Fstat. On non-Linux unix, ancestorAccessFlag falls back to + // O_RDONLY and the target open still needs owner read, so this scenario + // remains a documented limitation there. + if runtime.GOOS != "linux" { + t.Skip("owner-unreadable target open relies on O_PATH, Linux-only; O_RDONLY fallback elsewhere still needs read") + } + for _, targetMode := range []os.FileMode{0o333, 0o377} { + parent := t.TempDir() + require.NoError(t, os.Chmod(parent, 0o755)) + dir := filepath.Join(parent, "basecamp") + require.NoError(t, os.Mkdir(dir, 0o700)) + // Clear the owner-read bit (leaving owner write+execute) while keeping the + // dir group/world-writable — the untightened, at-risk state. + require.NoError(t, os.Chmod(dir, targetMode)) + + hardenConfigDir(dir) + + assert.Equal(t, os.FileMode(0o700), mode(t, dir), + "owner-unreadable target with mode %#o under a safe parent must be tightened", targetMode) + } + }) + + t.Run("symlinked final component is not followed", func(t *testing.T) { + base := t.TempDir() + target := filepath.Join(base, "real-cfg") + require.NoError(t, os.Mkdir(target, 0o755)) + require.NoError(t, os.Chmod(target, 0o755)) + link := filepath.Join(base, "link") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unavailable in this environment: %v", err) + } + + hardenConfigDir(link) + + // O_NOFOLLOW makes the open fail; the symlink target's mode is untouched. + assert.Equal(t, os.FileMode(0o755), mode(t, target)) + }) + + t.Run("symlinked ancestor is not followed", func(t *testing.T) { + // The single-open predecessor (os.OpenFile(dir, O_NOFOLLOW)) only + // rejected a symlinked FINAL component. A symlinked ANCESTOR was still + // followed at open time, so an attacker who controlled an ancestor could + // aim it at a different euid-owned dir and have the fchmod tighten THAT + // dir. Here `link` is an ANCESTOR of the requested path, not the final + // component: under the old code hardenConfigDir(base/link/child) would + // follow link -> real and chmod base/real/child to 0700. The + // per-component O_NOFOLLOW descent refuses `link` (ELOOP) and chmods + // nothing. + base := t.TempDir() + realDir := filepath.Join(base, "realDir") + require.NoError(t, os.Mkdir(realDir, 0o755)) + victim := filepath.Join(realDir, "child") + require.NoError(t, os.Mkdir(victim, 0o755)) + require.NoError(t, os.Chmod(victim, 0o755)) + link := filepath.Join(base, "link") + if err := os.Symlink(realDir, link); err != nil { + t.Skipf("symlinks unavailable in this environment: %v", err) + } + + // Sanity: the requested path resolves through the ancestor symlink to + // the victim dir, which is exactly what the old single-open code would + // have opened and tightened. + dir := filepath.Join(link, "child") + resolved, err := filepath.EvalSymlinks(dir) + require.NoError(t, err) + require.Equal(t, victim, resolved) + + hardenConfigDir(dir) + + // The ancestor symlink is refused, so the victim dir keeps its mode. + assert.Equal(t, os.FileMode(0o755), mode(t, victim)) + assert.Equal(t, os.FileMode(0o755), mode(t, realDir)) + }) + + t.Run("dir under a sticky 1777 parent is hardened", func(t *testing.T) { + // Previously the ancestor walk skipped any dir under a world-writable + // parent (e.g. a config dir under /tmp), leaving it loose. The fd-based + // sequence is race-free regardless of ancestry, so it hardens it. + parent := filepath.Join(t.TempDir(), "tmp-style") + require.NoError(t, os.Mkdir(parent, 0o777)) + require.NoError(t, os.Chmod(parent, 0o777|os.ModeSticky)) + dir := filepath.Join(parent, "basecamp") + require.NoError(t, os.Mkdir(dir, 0o755)) + require.NoError(t, os.Chmod(dir, 0o755)) + + hardenConfigDir(dir) + + assert.Equal(t, os.FileMode(0o700), mode(t, dir)) + }) + + t.Run("dir under a group/world-writable non-sticky parent is not hardened", func(t *testing.T) { + // Under a NON-sticky world-writable ancestor (e.g. XDG_CONFIG_HOME=/shared + // at 0777), a foreign user with write access to that parent can rename a + // real euid-owned directory into the config path between our openat hops. + // O_NOFOLLOW does not reject a renamed REAL dir, so the ownership check + // would pass and the fchmod would tighten a substituted victim dir. The + // fd-based ancestor-safety check aborts on the writable non-sticky parent + // before any chmod. The target itself is euid-owned; only the parent is + // unsafe. + parent := filepath.Join(t.TempDir(), "shared") + require.NoError(t, os.Mkdir(parent, 0o777)) + require.NoError(t, os.Chmod(parent, 0o777)) // world-writable, NO sticky bit + dir := filepath.Join(parent, "basecamp") + require.NoError(t, os.Mkdir(dir, 0o755)) + require.NoError(t, os.Chmod(dir, 0o755)) + + hardenConfigDir(dir) + + // Ancestor-safety abort: the loose parent means the dir keeps its mode. + assert.Equal(t, os.FileMode(0o755), mode(t, dir)) + }) + + t.Run("execute-only (0711) ancestor still hardens via the O_PATH descent", func(t *testing.T) { + // Regression: the ancestor descent used to open each component O_RDONLY, + // which requires READ permission. A traverse-only ancestor (execute but + // not read, e.g. /home at 0711 on hardened distros) granted search but not + // read, so the Openat failed EACCES and the WHOLE harden aborted — leaving + // the config dir world-listable for exactly those users. On Linux the + // ancestor components are now opened with O_PATH, which needs only + // execute/search, so the descent succeeds and the target is tightened. + // + // Note: the current euid OWNS this ancestor, so it retains read via the + // owner bits even at 0711 — a same-owner test can't fully starve read the + // way a foreign 0711 /home would. This still exercises the O_PATH descent + // path end-to-end (the parent carries no group/world read or write) and + // asserts hardening completes; a genuinely read-denied ancestor would + // require a second uid. + grandparent := filepath.Join(t.TempDir(), "traverse-only") + require.NoError(t, os.Mkdir(grandparent, 0o711)) + require.NoError(t, os.Chmod(grandparent, 0o711)) // execute-only for group/other + dir := filepath.Join(grandparent, "basecamp") + require.NoError(t, os.Mkdir(dir, 0o755)) + require.NoError(t, os.Chmod(dir, 0o755)) + + hardenConfigDir(dir) + + assert.Equal(t, os.FileMode(0o700), mode(t, dir), + "a config dir under an execute-only ancestor must still be hardened") + }) + + t.Run("already-0700 dir stays 0700 and skips the fchmod", func(t *testing.T) { + // Idempotence: on the steady-state run the dir is already 0700, so the + // fchmod is skipped to avoid churning the inode ctime every command. We + // assert the mode is unchanged and, best-effort, that the ctime did not + // advance (skipping the fchmod means no ctime bump). Timestamp granularity + // on some runners can make the ctime check flaky, so it only fails when the + // ctime clearly moved forward after a short spin. + dir := filepath.Join(t.TempDir(), "basecamp") + require.NoError(t, os.Mkdir(dir, 0o700)) + require.NoError(t, os.Chmod(dir, 0o700)) + + ctimeOf := func() syscall.Timespec { + var st syscall.Stat_t + require.NoError(t, syscall.Stat(dir, &st)) + return ctimespec(&st) + } + before := ctimeOf() + + hardenConfigDir(dir) + + assert.Equal(t, os.FileMode(0o700), mode(t, dir), "an already-0700 dir must remain 0700") + + after := ctimeOf() + // If the fchmod ran it would advance ctime; skipping it leaves ctime put. + // Compare seconds+nanos as a single moment; tolerate equal (skipped) but + // flag a forward jump. + beforeNs := before.Sec*1e9 + before.Nsec + afterNs := after.Sec*1e9 + after.Nsec + assert.LessOrEqual(t, afterNs, beforeNs, + "ctime advanced, suggesting the fchmod was not skipped on an already-0700 dir") + }) + + t.Run("regular file is not chmod'd", func(t *testing.T) { + file := filepath.Join(t.TempDir(), "not-a-dir") + require.NoError(t, os.WriteFile(file, []byte("x"), 0o644)) + + hardenConfigDir(file) + + assert.Equal(t, os.FileMode(0o644), mode(t, file)) + }) + + t.Run("missing dir is a no-op", func(t *testing.T) { + assert.NotPanics(t, func() { + hardenConfigDir(filepath.Join(t.TempDir(), "does-not-exist")) + }) + }) + + t.Run("FIFO at config path does not hang and is not chmod'd", func(t *testing.T) { + // A pre-planted named pipe at the config path used to hang open() until + // a writer appeared (blocking every command). O_NONBLOCK|O_DIRECTORY + // make the open return immediately and fail on the non-directory, so + // nothing is chmod'd. + fifo := filepath.Join(t.TempDir(), "basecamp") + if err := syscall.Mkfifo(fifo, 0o644); err != nil { + t.Skipf("mkfifo unavailable in this environment: %v", err) + } + before, err := os.Lstat(fifo) + require.NoError(t, err) + + done := make(chan struct{}) + go func() { + hardenConfigDir(fifo) + close(done) + }() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("hardenConfigDir hung on a FIFO config path") + } + + after, err := os.Lstat(fifo) + require.NoError(t, err) + assert.NotZero(t, after.Mode()&os.ModeNamedPipe, "path should still be a FIFO") + assert.Equal(t, before.Mode(), after.Mode(), "FIFO mode must be untouched") + }) + + t.Run("relative cfgDir is skipped by the call-site IsAbs gate", func(t *testing.T) { + // GlobalConfigDir only Cleans a relative XDG_CONFIG_HOME, so it can + // return a non-absolute path. The IsAbs gate lives at the call site in + // root.go (`cfgDir != "" && filepath.IsAbs(cfgDir)`); testing it there + // is the cleanly reachable seam since hardenConfigDir itself takes a + // resolved path. Verify the gate skips a relative cfgDir so no + // cwd-relative dir is surprise-chmod'd. + t.Setenv("XDG_CONFIG_HOME", "relative-cfg-home") + cfgDir := config.GlobalConfigDir() + require.False(t, filepath.IsAbs(cfgDir), "expected a relative cfgDir for this case") + assert.False(t, cfgDir != "" && filepath.IsAbs(cfgDir), "gate must skip a relative cfgDir") + }) + + t.Run("foreign-owned dir is not chmod'd", func(t *testing.T) { + // Producing a genuinely foreign-owned dir requires root (chown to + // another uid). Run the end-to-end case when we have root; the + // ownership predicate itself is covered for all users by + // TestOwnedByUID below. + if os.Geteuid() != 0 { + t.Skip("need root to chown a dir to a foreign uid; ownership logic covered by TestOwnedByUID") + } + dir := filepath.Join(t.TempDir(), "foreign") + require.NoError(t, os.Mkdir(dir, 0o755)) + require.NoError(t, os.Chmod(dir, 0o755)) + const foreignUID = 65534 // conventionally "nobody" + if err := os.Chown(dir, foreignUID, foreignUID); err != nil { + t.Skipf("cannot chown to foreign uid %d: %v", foreignUID, err) + } + + hardenConfigDir(dir) + + assert.Equal(t, os.FileMode(0o755), mode(t, dir)) + }) +} + +// TestOwnedByUID exercises the ownership predicate that gates the fchmod +// directly, since constructing a real foreign-owned dir requires root. +func TestOwnedByUID(t *testing.T) { + fi, err := os.Stat(t.TempDir()) + require.NoError(t, err) + + t.Run("self-owned dir matches current euid", func(t *testing.T) { + assert.True(t, ownedByUID(fi, os.Geteuid())) + }) + + t.Run("mismatched uid is foreign", func(t *testing.T) { + assert.False(t, ownedByUID(fi, os.Geteuid()+1)) + }) + + t.Run("FileInfo without Stat_t fails closed", func(t *testing.T) { + assert.False(t, ownedByUID(fakeFileInfo{}, os.Geteuid())) + }) +} + +// TestAncestorIsSafe exercises the fd-based ancestor-safety predicate that +// guards the descent: a dir is unsafe if it is group/world-writable without the +// sticky bit, or owned by a foreign non-root uid. +func TestAncestorIsSafe(t *testing.T) { + euid := os.Geteuid() + + t.Run("self-owned 0755 is safe", func(t *testing.T) { + st := &syscall.Stat_t{Mode: 0o755, Uid: uint32(euid)} + assert.True(t, ancestorIsSafe(st, euid)) + }) + + t.Run("root-owned 0755 is safe", func(t *testing.T) { + st := &syscall.Stat_t{Mode: 0o755, Uid: 0} + assert.True(t, ancestorIsSafe(st, euid)) + }) + + t.Run("world-writable with sticky bit is safe", func(t *testing.T) { + st := &syscall.Stat_t{Mode: 0o777 | unix.S_ISVTX, Uid: uint32(euid)} + assert.True(t, ancestorIsSafe(st, euid)) + }) + + t.Run("world-writable without sticky bit is unsafe", func(t *testing.T) { + st := &syscall.Stat_t{Mode: 0o777, Uid: uint32(euid)} + assert.False(t, ancestorIsSafe(st, euid)) + }) + + t.Run("group-writable without sticky bit is unsafe", func(t *testing.T) { + st := &syscall.Stat_t{Mode: 0o775, Uid: uint32(euid)} + assert.False(t, ancestorIsSafe(st, euid)) + }) + + t.Run("foreign non-root owner is unsafe", func(t *testing.T) { + st := &syscall.Stat_t{Mode: 0o755, Uid: uint32(euid) + 1} + assert.False(t, ancestorIsSafe(st, euid)) + }) +} + +// fakeFileInfo is a FileInfo whose Sys() is not a *syscall.Stat_t, modeling a +// filesystem that can't prove ownership. +type fakeFileInfo struct{} + +func (fakeFileInfo) Name() string { return "fake" } +func (fakeFileInfo) Size() int64 { return 0 } +func (fakeFileInfo) Mode() fs.FileMode { return fs.ModeDir | 0o755 } +func (fakeFileInfo) ModTime() time.Time { return time.Time{} } +func (fakeFileInfo) IsDir() bool { return true } +func (fakeFileInfo) Sys() any { return nil } diff --git a/internal/commands/api.go b/internal/commands/api.go index a2223e6ac..959df7628 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -3,6 +3,7 @@ package commands import ( "encoding/json" "fmt" + "net/url" "regexp" "strings" @@ -47,7 +48,10 @@ func newAPIGetCmd() *cobra.Command { return err } - path := parsePath(args[0]) + path, err := parsePath(args[0], app.Config.BaseURL, app.Config.AccountID) + if err != nil { + return err + } resp, err := app.Account().Get(cmd.Context(), path) if err != nil { return convertSDKError(err) @@ -85,7 +89,10 @@ func newAPIPostCmd() *cobra.Command { return err } - path := parsePath(args[0]) + path, err := parsePath(args[0], app.Config.BaseURL, app.Config.AccountID) + if err != nil { + return err + } // Parse JSON data var body any @@ -134,7 +141,10 @@ func newAPIPutCmd() *cobra.Command { return err } - path := parsePath(args[0]) + path, err := parsePath(args[0], app.Config.BaseURL, app.Config.AccountID) + if err != nil { + return err + } // Parse JSON data var body any @@ -176,7 +186,10 @@ func newAPIDeleteCmd() *cobra.Command { return err } - path := parsePath(args[0]) + path, err := parsePath(args[0], app.Config.BaseURL, app.Config.AccountID) + if err != nil { + return err + } resp, err := app.Account().Delete(cmd.Context(), path) if err != nil { return convertSDKError(err) @@ -201,20 +214,92 @@ func apiPathArgs(cmd *cobra.Command, args []string) error { return nil } -// parsePath extracts and normalizes the API path. -// Handles full URLs and relative paths. The leading slash is stripped because -// the SDK's accountPath and buildURL both add one — keeping it here would -// double-slash and, on Windows, MSYS/Git Bash converts /path to C:\...\path. -func parsePath(input string) string { - urlPattern := regexp.MustCompile(`^https?://[^/]+/[0-9]+(/.*)`) - if matches := urlPattern.FindStringSubmatch(input); len(matches) > 1 { - return matches[1] +// accountSegmentPattern matches a leading / segment in an API +// path so it can be dropped — the SDK re-prefixes the configured account. The +// trailing path is optional so a bare "/" (no further segments) +// also matches, leaving m[2] empty. +var accountSegmentPattern = regexp.MustCompile(`^/([0-9]+)(/.*)?$`) + +// parsePath normalizes the user-supplied API path against the configured base +// URL. It accepts relative paths ("projects.json", "/projects.json") and +// absolute Basecamp URLs whose host matches baseURL — from which the path is +// extracted and a leading / segment dropped (the SDK re-prefixes +// the configured account). Absolute URLs on ANY other host are rejected so the +// bearer token is never attached to a request bound for a foreign host. +// +// A leading account segment must match the configured accountID: the SDK +// re-prefixes the configured account, so silently stripping a foreign account +// id would retarget the request (including DELETEs) at the wrong account. +// When accountID is empty (no configured account to compare against), the +// segment is stripped as before. +// +// All leading slashes and a mixed-case scheme are normalized first so neither +// "//https://evil/…" nor "HTTPS://evil/…" can smuggle an absolute URL past the +// host check (URL schemes are case-insensitive per RFC 3986 §3.1). +func parsePath(input, baseURL, accountID string) (string, error) { + candidate := strings.TrimLeft(input, "/") + lower := strings.ToLower(candidate) + if strings.HasPrefix(lower, "http://") || strings.HasPrefix(lower, "https://") { + u, err := url.Parse(candidate) + if err != nil || u.Host == "" { + return "", output.ErrUsage("invalid API URL: " + input) + } + base, baseErr := url.Parse(baseURL) + if baseErr != nil || base.Host == "" || !sameHostPort(u, base) { + return "", output.ErrUsage("API path must be relative or a Basecamp URL on the configured host; refusing to send credentials to " + input) + } + // Same host: use the path (+ query), dropping a leading / + // — but only when it matches the configured account. + path := u.EscapedPath() + if m := accountSegmentPattern.FindStringSubmatch(path); m != nil { + if accountID != "" && m[1] != accountID { + return "", output.ErrUsage(fmt.Sprintf("URL is for account %s; refusing to retarget to configured account %s", m[1], accountID)) + } + // A bare "/" leaves m[2] empty; the request targets the + // account root, so normalize to "/" rather than an empty path. + if path = m[2]; path == "" { + path = "/" + } + } + // A same-host URL with no path at all (e.g. "https://host" or a + // query-only "https://host?foo=bar") leaves path empty since the + // account-segment pattern can't match ""; normalize to the root. + if path == "" { + path = "/" + } + if u.RawQuery != "" { + path += "?" + u.RawQuery + } + return path, nil } - // Strip leading slash — the SDK prefixes the account path. - input = strings.TrimPrefix(input, "/") + // Relative path — return without leading slashes; the SDK prefixes the + // account path (keeping one here would double-slash and, on Windows, + // MSYS/Git Bash converts /path to C:\...\path). + return candidate, nil +} + +// sameHostPort reports whether u and base address the same host: hostnames +// compared case-insensitively, ports compared after normalizing an omitted +// port to the scheme default. A matching hostname on a different port is +// still a different origin and must not receive credentials. +func sameHostPort(u, base *url.URL) bool { + if !strings.EqualFold(u.Hostname(), base.Hostname()) { + return false + } + return effectivePort(u) == effectivePort(base) +} - return input +// effectivePort returns the URL's explicit port, or the scheme's default +// (80 for http, 443 for https) when omitted. +func effectivePort(u *url.URL) string { + if port := u.Port(); port != "" { + return port + } + if strings.EqualFold(u.Scheme, "http") { + return "80" + } + return "443" } // apiSummary generates a summary from the API response. diff --git a/internal/commands/api_test.go b/internal/commands/api_test.go index 103ec1f00..901bc3635 100644 --- a/internal/commands/api_test.go +++ b/internal/commands/api_test.go @@ -12,6 +12,11 @@ import ( "github.com/basecamp/basecamp-cli/internal/output" ) +const ( + testBaseURL = "https://3.basecampapi.com" + testAccountID = "12345" +) + func TestParsePath(t *testing.T) { tests := []struct { input string @@ -21,13 +26,96 @@ func TestParsePath(t *testing.T) { {"/projects.json", "projects.json"}, {"buckets/123/todos/456.json", "buckets/123/todos/456.json"}, {"/buckets/123/todos/456.json", "buckets/123/todos/456.json"}, - {"https://3.basecampapi.com/999/projects.json", "/projects.json"}, + // Same-host absolute URLs: extract the path, dropping the account + // segment when it matches the configured account. + {"https://3.basecampapi.com/12345/projects.json", "/projects.json"}, {"https://3.basecampapi.com/12345/buckets/1/todos/2.json", "/buckets/1/todos/2.json"}, + // Same host but no account segment — accepted, path used as-is. + {"https://3.basecampapi.com/projects.json", "/projects.json"}, + // Same-host URL with no path at all normalizes to the account root. + {"https://3.basecampapi.com", "/"}, + // Same-host query-only URL normalizes to root and keeps the query. + {"https://3.basecampapi.com?foo=bar", "/?foo=bar"}, + // A bare "/" with no trailing segment still matches and + // strips to the account root path rather than leaving it re-prefixed. + {"https://3.basecampapi.com/12345", "/"}, + // Bare "/" plus a query strips to root and keeps the query. + {"https://3.basecampapi.com/12345?foo=bar", "/?foo=bar"}, + // Query strings are preserved. + {"https://3.basecampapi.com/12345/projects.json?page=2", "/projects.json?page=2"}, + // Schemes are case-insensitive (RFC 3986): a same-host uppercase scheme + // still extracts the path. + {"HTTPS://3.basecampapi.com/12345/projects.json", "/projects.json"}, + // An explicit default port is still the configured host. + {"https://3.basecampapi.com:443/12345/projects.json", "/projects.json"}, } for _, tt := range tests { t.Run(tt.input, func(t *testing.T) { - assert.Equal(t, tt.want, parsePath(tt.input)) + got, err := parsePath(tt.input, testBaseURL, testAccountID) + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +// TestParsePathRejectsForeignAccount guards against silent account +// retargeting: the SDK re-prefixes the configured account, so stripping a +// different account's id from a pasted URL would aim the request (including +// DELETEs) at the configured account instead of the one the URL names. +func TestParsePathRejectsForeignAccount(t *testing.T) { + for _, input := range []string{ + "https://3.basecampapi.com/999/projects.json", + "https://3.basecampapi.com/999/buckets/1/todos/2.json", + "https://3.basecampapi.com/999/projects.json?page=2", + // A bare foreign "/" (no trailing path) must still be + // rejected — the broadened pattern must not open a retargeting hole. + "https://3.basecampapi.com/999", + } { + t.Run(input, func(t *testing.T) { + got, err := parsePath(input, testBaseURL, testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), "refusing to retarget") + assert.Empty(t, got) + }) + } + + t.Run("empty configured account strips the segment as before", func(t *testing.T) { + got, err := parsePath("https://3.basecampapi.com/999/projects.json", testBaseURL, "") + require.NoError(t, err) + assert.Equal(t, "/projects.json", got) + }) +} + +// TestParsePathRejectsForeignHosts guards against bearer-token exfiltration: an +// absolute URL whose host differs from the configured base URL must be rejected +// before it reaches the SDK, which would otherwise attach the Authorization +// header and send credentials to the foreign host. Covers mixed-case schemes, +// leading-slash smuggling attempts, and a same-hostname non-default port. +func TestParsePathRejectsForeignHosts(t *testing.T) { + bad := []string{ + "https://evil.com/projects.json", + "http://evil.com/projects.json", + "https://evil.com/", + "https://evil.com", + "https://evil.example/999/projects.json", // foreign host, numeric form + "http://127.0.0.1:9999/projects.json", + "HTTPS://evil.example/projects.json", // uppercase scheme + "HtTpS://evil.example/projects.json", // mixed-case scheme + "/https://evil.example/projects.json", // leading-slash smuggling + "/HTTPS://evil.example/projects.json", // leading slash + uppercase + "//https://evil.example/projects.json", // double-slash smuggling + "///https://evil.example/x", // triple-slash smuggling + "https://3.basecampapi.com:8443/projects.json", // same hostname, non-default port + "https://3.basecampapi.com:443.evil.com/projects.json", // port-lookalike host smuggling + "https://3.basecampapi.com:abc/x", // non-numeric port + } + + for _, input := range bad { + t.Run(input, func(t *testing.T) { + got, err := parsePath(input, testBaseURL, testAccountID) + require.Error(t, err) + assert.Empty(t, got) }) } } diff --git a/internal/commands/config.go b/internal/commands/config.go index d588f2432..ea5c39324 100644 --- a/internal/commands/config.go +++ b/internal/commands/config.go @@ -14,6 +14,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/hostutil" "github.com/basecamp/basecamp-cli/internal/output" "github.com/basecamp/basecamp-cli/internal/tui/resolve" ) @@ -291,10 +292,21 @@ Valid keys: account_id, project_id (or project), todolist_id, base_url, cache_di case "llm_provider": validProviders := map[string]bool{ "anthropic": true, "openai": true, "ollama": true, - "apple": true, "none": true, "auto": true, + "apple": true, "none": true, "disabled": true, "auto": true, } if !validProviders[value] { - return output.ErrUsage(fmt.Sprintf("llm_provider must be one of: anthropic, openai, ollama, apple, none, auto (got %q)", value)) + return output.ErrUsage(fmt.Sprintf("llm_provider must be one of: anthropic, openai, ollama, apple, none, disabled, auto (got %q)", value)) + } + configData[key] = value + case "llm_endpoint": + if !config.IsHTTPURL(value) { + return output.ErrUsage("llm_endpoint must be an http:// or https:// URL with a host") + } + // Warn (don't reject) on plain-http non-localhost endpoints: they're + // fine for ollama, but the LLM path fails closed for credentialed + // providers (openai) when an API key is set. + if err := hostutil.RequireSecureURL(value); err != nil { + fmt.Fprintln(os.Stderr, "warning: llm_endpoint is plain http on a non-localhost host; the LLM feature will refuse it if an API key is set with the openai provider") } configData[key] = value case "llm_max_concurrent": @@ -343,12 +355,13 @@ Valid keys: account_id, project_id (or project), todolist_id, base_url, cache_di return fmt.Errorf("failed to write config: %w", err) } - // Warn when writing authority keys to local config without trust - if !global && isAuthorityKey(key) { + // Warn when writing trust-gated keys to local config without trust — + // otherwise the value is silently ignored on the next load. + if !global && isTrustGatedKey(key) { absPath, _ := filepath.Abs(configPath) ts := config.LoadTrustStore(config.GlobalConfigDir()) if ts == nil || !ts.IsTrusted(configPath) { - fmt.Fprintf(os.Stderr, "warning: authority key %q in local config requires trust to take effect; run:\n basecamp config trust %s\n", key, config.ShellQuote(absPath)) + fmt.Fprintf(os.Stderr, "warning: %q in local config requires trust to take effect; run:\n basecamp config trust %s\n", key, config.ShellQuote(absPath)) } } @@ -406,6 +419,21 @@ func isAuthorityKey(key string) bool { return false } +// isTrustGatedKey reports whether key is ignored when loaded from an untrusted +// local/repo config. It's the authority keys plus the cache/LLM keys gated for +// the same reason (cache redirection, paid-model substitution, cost +// amplification), so `config set` warns before a local write silently no-ops. +func isTrustGatedKey(key string) bool { + if isAuthorityKey(key) { + return true + } + switch key { + case "cache_dir", "cache_enabled", "llm_model", "llm_max_concurrent", "llm_token_budget": + return true + } + return false +} + // redactSecret returns "(set)" if the value is non-empty, empty string otherwise. func redactSecret(value string) string { if value != "" { diff --git a/internal/commands/config_test.go b/internal/commands/config_test.go index 8bcbdc7d7..932bb4365 100644 --- a/internal/commands/config_test.go +++ b/internal/commands/config_test.go @@ -337,12 +337,82 @@ func TestConfigSet_AuthorityKeyWarnsWithPath(t *testing.T) { stderr := string(buf[:n]) absPath := filepath.Join(tmpDir, ".basecamp", "config.json") - assert.Contains(t, stderr, "authority key") + assert.Contains(t, stderr, `"base_url"`) assert.Contains(t, stderr, "requires trust") assert.Contains(t, stderr, absPath, "warning must include the exact config path") assert.Contains(t, stderr, "'"+absPath+"'", "path must be single-quoted for shell safety") } +// TestConfigSet_GatedNonAuthorityKeyWarns verifies the trust warning also fires +// for the cache/LLM keys gated on load (cache_dir, llm_model, …), so a local +// `config set` doesn't silently produce a value that's ignored next run. +func TestConfigSet_GatedNonAuthorityKeyWarns(t *testing.T) { + app, _ := setupConfigTestApp(t) + + tmpDir, _ := filepath.EvalSymlinks(t.TempDir()) + origDir, _ := os.Getwd() + require.NoError(t, os.Chdir(tmpDir)) + defer os.Chdir(origDir) + require.NoError(t, os.MkdirAll(".basecamp", 0755)) + + origStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + err := executeConfigCommand(app, "set", "cache_dir", "/tmp/somewhere") + require.NoError(t, err) + + w.Close() + var buf [4096]byte + n, _ := r.Read(buf[:]) + os.Stderr = origStderr + + stderr := string(buf[:n]) + assert.Contains(t, stderr, `"cache_dir"`) + assert.Contains(t, stderr, "requires trust") +} + +// --- config set llm_endpoint insecure-endpoint warning tests --- + +// setLLMEndpointCapturingStderr runs `config set --global llm_endpoint ` +// and returns what was written to stderr. Global scope keeps the trust-gated +// local-config warning (llm_endpoint is an authority key) out of the capture. +func setLLMEndpointCapturingStderr(t *testing.T, value string) string { + t.Helper() + app, _ := setupConfigTestApp(t) + + origStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + err := executeConfigCommand(app, "set", "--global", "llm_endpoint", value) + + w.Close() + var buf [4096]byte + n, _ := r.Read(buf[:]) + os.Stderr = origStderr + + require.NoError(t, err) + return string(buf[:n]) +} + +// TestConfigSet_LLMEndpointInsecureWarns verifies that a plain-http +// non-localhost llm_endpoint is accepted (ollama legitimately uses http) but +// warns that the LLM path will refuse it for credentialed providers. +func TestConfigSet_LLMEndpointInsecureWarns(t *testing.T) { + stderr := setLLMEndpointCapturingStderr(t, "http://remote:8080") + assert.Contains(t, stderr, "warning: llm_endpoint is plain http on a non-localhost host") + assert.Contains(t, stderr, "the LLM feature will refuse it") +} + +func TestConfigSet_LLMEndpointSecureNoWarning(t *testing.T) { + assert.Empty(t, setLLMEndpointCapturingStderr(t, "https://remote:8080")) +} + +func TestConfigSet_LLMEndpointLocalhostHTTPNoWarning(t *testing.T) { + assert.Empty(t, setLLMEndpointCapturingStderr(t, "http://localhost:11434")) +} + // --- Config project tests --- // setupConfigProjectTestApp creates a test app wired to an httptest server @@ -423,6 +493,32 @@ func TestConfigSet_ProjectAlias(t *testing.T) { assert.Equal(t, "12345", saved["project_id"]) } +func TestConfigSet_LLMProviderValidation(t *testing.T) { + app, _ := setupConfigTestApp(t) + + tmpDir, _ := filepath.EvalSymlinks(t.TempDir()) + origDir, _ := os.Getwd() + require.NoError(t, os.Chdir(tmpDir)) + defer os.Chdir(origDir) + + require.NoError(t, os.MkdirAll(".basecamp", 0755)) + + // "disabled" is valid: DetectProvider treats it like "none". + err := executeConfigCommand(app, "set", "llm_provider", "disabled") + require.NoError(t, err) + + data, err := os.ReadFile(filepath.Join(tmpDir, ".basecamp", "config.json")) + require.NoError(t, err) + var saved map[string]any + require.NoError(t, json.Unmarshal(data, &saved)) + assert.Equal(t, "disabled", saved["llm_provider"]) + + // Unknown providers are rejected, and the error lists "disabled". + err = executeConfigCommand(app, "set", "llm_provider", "bogus") + require.Error(t, err) + assert.Contains(t, err.Error(), "disabled") +} + func TestConfigUnset_ProjectAlias(t *testing.T) { app, _ := setupConfigTestApp(t) diff --git a/internal/commands/skill.go b/internal/commands/skill.go index 8ce645b82..aa727ef0c 100644 --- a/internal/commands/skill.go +++ b/internal/commands/skill.go @@ -344,7 +344,8 @@ func RefreshSkillsIfVersionChanged() bool { // On transient failure, leave the sentinel stale so the next run retries. needsRefresh := baselineSkillInstalled() if !needsRefresh || refreshed { - _ = os.MkdirAll(filepath.Dir(sentinelPath), 0o755) //nolint:gosec // G301: config dir + // 0o700: GlobalConfigDir can hold credentials.json; keep it owner-only. + _ = os.MkdirAll(filepath.Dir(sentinelPath), 0o700) _ = os.WriteFile(sentinelPath, []byte(version.Version), 0o644) //nolint:gosec // G306: not a secret } diff --git a/internal/commands/timeline.go b/internal/commands/timeline.go index 99d2a903e..f71c205bd 100644 --- a/internal/commands/timeline.go +++ b/internal/commands/timeline.go @@ -18,6 +18,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/richtext" ) // NewTimelineCmd creates the timeline command for viewing activity feeds. @@ -395,19 +396,26 @@ func formatEvent(e basecamp.TimelineEvent) string { timeStr = "--:--" } + // API-controlled fields are sanitized before rendering: rune truncation + // preserves escape bytes, and the alt-screen watch TUI would otherwise + // execute embedded OSC/ANSI sequences (terminal injection). + // Strip before the empty check so a name that's only escape sequences + // still falls back to the placeholder rather than rendering blank. creatorName := "Someone" - if e.Creator != nil && e.Creator.Name != "" { - creatorName = e.Creator.Name + if e.Creator != nil { + if name := richtext.SanitizeSingleLine(e.Creator.Name); name != "" { + creatorName = name + } } - action := e.Action + action := richtext.SanitizeSingleLine(e.Action) if action == "" { action = "updated" } - title := e.Title + title := richtext.SanitizeSingleLine(e.Title) if title == "" { - title = e.SummaryExcerpt + title = richtext.SanitizeSingleLine(e.SummaryExcerpt) } // Truncate at rune boundary for proper Unicode handling if len([]rune(title)) > 40 { @@ -422,6 +430,19 @@ func formatEvent(e basecamp.TimelineEvent) string { ) } +// watchLabel builds a watch-status label from an API-controlled name, +// sanitizing it for the single-line terminal sink. An all-escape name collapses +// to "" after sanitization, which would render a blank trailing value (e.g. +// "activity for "); in that case fall back to the already-resolved ID so the +// label always names something concrete. +func watchLabel(format, name, fallbackID string) string { + safeName := richtext.SanitizeSingleLine(name) + if safeName == "" { + safeName = fallbackID + } + return fmt.Sprintf(format, safeName) +} + func runTimelineWatch(cmd *cobra.Command, args []string, project, person string, interval time.Duration, limit, page int, all bool) error { if all { return output.ErrUsage("--all cannot be used with --watch") @@ -507,7 +528,7 @@ func runTimelineWatch(cmd *cobra.Command, args []string, project, person string, if err != nil { return output.ErrUsage("Invalid person ID") } - description = fmt.Sprintf("activity for %s", personName) + description = watchLabel("activity for %s", personName, resolvedPersonID) fetchFn = func(ctx context.Context) ([]basecamp.TimelineEvent, error) { result, err := app.Account().Timeline().PersonProgress(ctx, personID, opts) if err != nil { @@ -525,7 +546,7 @@ func runTimelineWatch(cmd *cobra.Command, args []string, project, person string, if err != nil { return output.ErrUsage("Invalid project ID") } - description = fmt.Sprintf("activity in %s", projectName) + description = watchLabel("activity in %s", projectName, resolvedProjectID) fetchFn = func(ctx context.Context) ([]basecamp.TimelineEvent, error) { r, err := app.Account().Timeline().ProjectTimeline(ctx, projectIDInt, opts) if err != nil { diff --git a/internal/commands/timeline_test.go b/internal/commands/timeline_test.go new file mode 100644 index 000000000..0bd24a99b --- /dev/null +++ b/internal/commands/timeline_test.go @@ -0,0 +1,80 @@ +package commands + +import ( + "strings" + "testing" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/charmbracelet/x/ansi" + "github.com/stretchr/testify/assert" +) + +// TestFormatEventSanitizesSingleLine verifies that API-controlled fields +// rendered into the single-line alt-screen watch event string are collapsed to +// one line: bare CR between words becomes a separator (words are not glued), +// and embedded newlines/tabs/escape sequences cannot break the layout or inject +// into the terminal. +func TestFormatEventSanitizesSingleLine(t *testing.T) { + e := basecamp.TimelineEvent{ + Creator: &basecamp.Person{Name: "Ann\rBeth"}, + Action: "com\nmented", + Title: "hello\tworld\x1b[31m\r\nagain\u009b0m", + } + + // Strip lipgloss's own styling so we assert on the API-controlled content. + got := ansi.Strip(formatEvent(e)) + + assert.NotContains(t, got, "\n") + assert.NotContains(t, got, "\r") + assert.NotContains(t, got, "\t") + assert.NotContains(t, got, "\x1b") + assert.NotContains(t, got, "\u009b") + + // Words remain space-separated rather than glued together. + assert.Contains(t, got, "Ann Beth") + assert.Contains(t, got, "com mented") + assert.Contains(t, got, "hello world") + assert.NotContains(t, got, "AnnBeth") + assert.NotContains(t, got, "commented") +} + +// TestFormatEventEmptyFieldsFallBack verifies the empty-check fallbacks still +// fire when a field sanitizes down to the empty string (all whitespace/control). +func TestFormatEventEmptyFieldsFallBack(t *testing.T) { + e := basecamp.TimelineEvent{ + Creator: &basecamp.Person{Name: "\x1b[0m\r\n"}, + Action: "\t \r ", + Title: "\x1b[31m", + SummaryExcerpt: "fell\rback", + } + + got := ansi.Strip(formatEvent(e)) + + assert.Contains(t, got, "Someone") + assert.Contains(t, got, "updated") + // Title was empty, so SummaryExcerpt is used and also single-lined. + assert.Contains(t, got, "fell back") + assert.False(t, strings.Contains(got, "fellback")) +} + +// TestWatchLabelSanitizesName verifies a normal name is sanitized for the +// single-line watch-status sink and kept when non-empty. +func TestWatchLabelSanitizesName(t *testing.T) { + got := watchLabel("activity for %s", "Ann\x1b[31m Beth\r", "12345") + assert.Equal(t, "activity for Ann Beth", got) + assert.NotContains(t, got, "\x1b") +} + +// TestWatchLabelFallsBackToID verifies that an all-escape name — which collapses +// to "" after sanitization — falls back to the already-resolved ID so the label +// never renders with a blank trailing value ("activity for "). +func TestWatchLabelFallsBackToID(t *testing.T) { + allEscape := "\x1b[2J\x1b]0;x\x07\x08\x7f" + + person := watchLabel("activity for %s", allEscape, "12345") + assert.Equal(t, "activity for 12345", person) + assert.NotContains(t, person, "\x1b") + + project := watchLabel("activity in %s", allEscape, "67890") + assert.Equal(t, "activity in 67890", project) +} diff --git a/internal/commands/tui.go b/internal/commands/tui.go index b59e27b19..b09d8a23f 100644 --- a/internal/commands/tui.go +++ b/internal/commands/tui.go @@ -90,7 +90,10 @@ func NewTUICmd() *cobra.Command { } } - session := workspace.NewSession(app) + session, err := workspace.NewSession(app) + if err != nil { + return err + } defer session.Shutdown() // Deep-link: parse URL argument and set initial navigation target. @@ -111,7 +114,7 @@ func NewTUICmd() *cobra.Command { p := tea.NewProgram(model) - _, err := p.Run() + _, err = p.Run() model.CloseWatcher() return err }, diff --git a/internal/commands/update_notice.go b/internal/commands/update_notice.go index 71557bfc6..a2cc8d90c 100644 --- a/internal/commands/update_notice.go +++ b/internal/commands/update_notice.go @@ -130,6 +130,7 @@ func writeUpdateCache(latestVersion string) { return } dir := filepath.Dir(updateCachePath()) - _ = os.MkdirAll(dir, 0o755) //nolint:gosec // G301: config dir + // 0o700: GlobalConfigDir can hold credentials.json; keep it owner-only. + _ = os.MkdirAll(dir, 0o700) _ = os.WriteFile(updateCachePath(), data, 0o644) //nolint:gosec // G306: not a secret } diff --git a/internal/commands/wizard_agents.go b/internal/commands/wizard_agents.go index afa915145..c69d8af84 100644 --- a/internal/commands/wizard_agents.go +++ b/internal/commands/wizard_agents.go @@ -321,7 +321,22 @@ func removeStaleClaudePlugins(ctx context.Context, claudePath string, plugins [] for _, p := range plugins { if len(p.Scopes) > 0 { anyRemoved := false + // someScopeInvalid tracks whether any scope failed validPluginScope + // (i.e. no scoped uninstall could be attempted for it). Those entries + // are unreachable by scoped uninstalls, so the unscoped fallback must + // run even when another, valid scope was removed successfully. We must + // NOT fall back just because a VALID scope's uninstall failed at + // runtime — that would wrongly strip every-scope install when the + // targeted ones merely errored. + someScopeInvalid := false for _, scope := range p.Scopes { + // scope comes from installed_plugins.json (not first-party). + // Whitelist it so a "-"-leading value can't inject a flag into + // the uninstall argv. + if !validPluginScope(scope) { + someScopeInvalid = true + continue + } c := exec.CommandContext(ctx, claudePath, "plugin", "uninstall", p.Key, "--scope", scope) //nolint:gosec // G204: claudePath from FindClaudeBinary if err := c.Run(); err == nil { anyRemoved = true @@ -331,26 +346,68 @@ func removeStaleClaudePlugins(ctx context.Context, claudePath string, plugins [] } } } - if anyRemoved { - removed = append(removed, p.Key) - } - } else { - n := 0 - for i := 0; i < 10; i++ { - c := exec.CommandContext(ctx, claudePath, "plugin", "uninstall", p.Key) //nolint:gosec // G204: claudePath from FindClaudeBinary - if err := c.Run(); err != nil { - break + if someScopeInvalid { + // At least one scope was invalid, so its entry was never targeted + // by a scoped uninstall. Fall back to the unscoped retry removal + // so the plugin isn't silently left installed under that scope. + if uninstallUnscoped(ctx, claudePath, p.Key) { + anyRemoved = true + // The unscoped removal strips EVERY install of the key, + // including valid-scoped ones whose scoped uninstall failed + // at runtime (and thus were never recorded above). Record + // those valid scopes so runClaudeSetup reinstalls them + // instead of silently dropping the user's install. + for _, scope := range p.Scopes { + if validPluginScope(scope) && !scopeSeen[scope] { + scopeSeen[scope] = true + scopes = append(scopes, scope) + } + } } - n++ } - if n > 0 { + if anyRemoved { removed = append(removed, p.Key) } + } else if uninstallUnscoped(ctx, claudePath, p.Key) { + removed = append(removed, p.Key) } } return removed, scopes } +// uninstallUnscoped removes every installation of key by retrying an unscoped +// `claude plugin uninstall` until it fails (entry gone) or a safety cap of 10 +// iterations is reached. Reports whether at least one uninstall succeeded. +func uninstallUnscoped(ctx context.Context, claudePath, key string) bool { + n := 0 + for i := 0; i < 10; i++ { + c := exec.CommandContext(ctx, claudePath, "plugin", "uninstall", key) //nolint:gosec // G204: claudePath from FindClaudeBinary + if err := c.Run(); err != nil { + break + } + n++ + } + return n > 0 +} + +// validPluginScope reports whether scope is one of Claude's accepted plugin +// scopes. Used to gate untrusted scope values from installed_plugins.json +// before they reach `claude plugin uninstall --scope `. +// +// `claude plugin uninstall --scope` only accepts user/project/local, so a +// "global"-scoped entry is invalid here: leaving it valid would make a scoped +// uninstall fail silently while suppressing the unscoped fallback, stranding +// the plugin. Treating "global" as invalid sets someScopeInvalid so the +// unscoped fallback removes it. +func validPluginScope(scope string) bool { + switch scope { + case "user", "project", "local": + return true + default: + return false + } +} + // claudeManualInstallHint returns the two-line manual install instructions. func claudeManualInstallHint(styles *tui.Styles) (string, string) { return styles.Bold.Render(fmt.Sprintf(" claude plugin marketplace add %s", harness.ClaudeMarketplaceSource)), diff --git a/internal/commands/wizard_agents_test.go b/internal/commands/wizard_agents_test.go new file mode 100644 index 000000000..c0e28ff41 --- /dev/null +++ b/internal/commands/wizard_agents_test.go @@ -0,0 +1,173 @@ +package commands + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/harness" +) + +// TestValidPluginScope guards the argv-injection whitelist: scope values come +// from installed_plugins.json (not first-party), so a "-"-leading value must +// not reach `claude plugin uninstall --scope `. +func TestValidPluginScope(t *testing.T) { + valid := []string{"user", "project", "local"} + for _, s := range valid { + if !validPluginScope(s) { + t.Errorf("validPluginScope(%q) = false, want true", s) + } + } + + // "global" is not a scope `claude plugin uninstall --scope` accepts, so it + // must be treated as invalid: keeping it valid would make a scoped uninstall + // fail silently while suppressing the unscoped fallback, stranding the plugin. + invalid := []string{"", "global", "-rf", "--force", "User", "system", "/etc", "user ", " user"} + for _, s := range invalid { + if validPluginScope(s) { + t.Errorf("validPluginScope(%q) = true, want false", s) + } + } +} + +// stubClaudeUninstall writes a stub `claude` binary that logs every invocation +// to logFile and exits with a non-zero code for scoped uninstall calls when +// failScoped is true (otherwise they succeed). Unscoped uninstalls succeed once +// per key then fail (hard-coded), so the retry loop runs once before ending, +// mirroring real "entry gone" behavior. Returns its absolute path. +func stubClaudeUninstall(t *testing.T, failScoped bool) string { + t.Helper() + dir := t.TempDir() + logFile := filepath.Join(dir, "calls.log") + markerDir := filepath.Join(dir, "markers") + require.NoError(t, os.MkdirAll(markerDir, 0o755)) + + scopedExit := "0" + if failScoped { + scopedExit = "1" + } + script := "#!/bin/sh\n" + + "echo \"$*\" >> \"" + logFile + "\"\n" + + "case \"$1 $2\" in\n" + + " \"plugin uninstall\")\n" + + " if [ \"$4\" = \"--scope\" ]; then exit " + scopedExit + "; fi\n" + + // unscoped: succeed once per key, then fail so the retry loop ends. + " MARKER=\"" + markerDir + "/$3.removed\"\n" + + " if [ ! -f \"$MARKER\" ]; then > \"$MARKER\"; exit 0; fi\n" + + " exit 1\n" + + " ;;\n" + + " *) exit 0 ;;\n" + + "esac\n" + path := filepath.Join(dir, "claude") + require.NoError(t, os.WriteFile(path, []byte(script), 0o755)) //nolint:gosec // G306: test helper + return path +} + +func readClaudeCalls(t *testing.T, claudePath string) string { + t.Helper() + data, err := os.ReadFile(filepath.Join(filepath.Dir(claudePath), "calls.log")) + if os.IsNotExist(err) { + return "" + } + require.NoError(t, err) + return string(data) +} + +// TestRemoveStaleClaudePluginsAllScopesInvalid verifies the YL7 fix: when every +// recorded scope fails validPluginScope (no scoped uninstall is attempted), we +// fall back to an unscoped removal so the plugin isn't silently left installed. +func TestRemoveStaleClaudePluginsAllScopesInvalid(t *testing.T) { + claude := stubClaudeUninstall(t, false) + plugins := []harness.StalePlugin{{Key: "basecamp@37signals", Scopes: []string{"-rf", "--force"}}} + + removed, scopes := removeStaleClaudePlugins(context.Background(), claude, plugins) + + calls := readClaudeCalls(t, claude) + assert.NotContains(t, calls, "--scope", "no scoped uninstall should be attempted for invalid scopes") + assert.Contains(t, calls, "plugin uninstall basecamp@37signals", "unscoped fallback should run") + assert.Equal(t, []string{"basecamp@37signals"}, removed) + assert.Empty(t, scopes) +} + +// TestRemoveStaleClaudePluginsGlobalScopeFallsBack verifies that a stale entry +// whose only recorded scope is "global" (which `claude plugin uninstall --scope` +// rejects) is treated as all-invalid, so the unscoped fallback removes it rather +// than leaving it silently installed behind a failing scoped uninstall. +func TestRemoveStaleClaudePluginsGlobalScopeFallsBack(t *testing.T) { + claude := stubClaudeUninstall(t, false) + plugins := []harness.StalePlugin{{Key: "basecamp@37signals", Scopes: []string{"global"}}} + + removed, scopes := removeStaleClaudePlugins(context.Background(), claude, plugins) + + calls := readClaudeCalls(t, claude) + assert.NotContains(t, calls, "--scope", "no scoped uninstall should be attempted for a global scope") + assert.Contains(t, calls, "plugin uninstall basecamp@37signals", "unscoped fallback should run") + assert.Equal(t, []string{"basecamp@37signals"}, removed) + assert.Empty(t, scopes) +} + +// TestRemoveStaleClaudePluginsMixedScopes verifies that when a plugin key has +// BOTH a valid scope and an invalid legacy scope (e.g. "global"), the valid +// scope's successful uninstall does not suppress the unscoped fallback: the +// entry installed under the invalid scope must still be removed, and the key +// must appear in removed exactly once. +func TestRemoveStaleClaudePluginsMixedScopes(t *testing.T) { + claude := stubClaudeUninstall(t, false) + plugins := []harness.StalePlugin{{Key: "basecamp@37signals", Scopes: []string{"user", "global"}}} + + removed, scopes := removeStaleClaudePlugins(context.Background(), claude, plugins) + + calls := readClaudeCalls(t, claude) + assert.Contains(t, calls, "plugin uninstall basecamp@37signals --scope user", + "valid scope should be uninstalled explicitly") + assert.NotContains(t, calls, "--scope global", "invalid scope must never reach the argv") + assert.Contains(t, calls, "plugin uninstall basecamp@37signals\n", + "unscoped fallback should run for the invalid-scope entry") + assert.Equal(t, []string{"basecamp@37signals"}, removed, "key reported once despite two removal paths") + assert.Equal(t, []string{"user"}, scopes, + "valid scope recorded exactly once: the scoped-uninstall success and the fallback must not double-add it") +} + +// TestRemoveStaleClaudePluginsMixedScopesValidUninstallFails verifies that when +// a plugin mixes a VALID scope whose scoped uninstall fails at runtime with an +// INVALID scope, the unscoped fallback (which strips every install of the key, +// including the valid-scoped one) still records the valid scope in the returned +// scopes list so runClaudeSetup reinstalls it instead of silently dropping it. +func TestRemoveStaleClaudePluginsMixedScopesValidUninstallFails(t *testing.T) { + claude := stubClaudeUninstall(t, true) + plugins := []harness.StalePlugin{{Key: "basecamp@37signals", Scopes: []string{"project", "global"}}} + + removed, scopes := removeStaleClaudePlugins(context.Background(), claude, plugins) + + calls := readClaudeCalls(t, claude) + assert.Contains(t, calls, "plugin uninstall basecamp@37signals --scope project", + "valid scope should be attempted explicitly even though it fails") + assert.NotContains(t, calls, "--scope global", "invalid scope must never reach the argv") + assert.Contains(t, calls, "plugin uninstall basecamp@37signals\n", + "unscoped fallback should run for the invalid-scope entry") + assert.Equal(t, []string{"basecamp@37signals"}, removed) + assert.Equal(t, []string{"project"}, scopes, + "valid scope stripped by the unscoped fallback must be recorded exactly once for reinstall") +} + +// TestRemoveStaleClaudePluginsValidScopeUninstallFails verifies the regression +// fix: when scopes are VALID but the scoped uninstall fails at runtime, we must +// NOT fall back to an unscoped removal (which would wrongly strip every scope). +func TestRemoveStaleClaudePluginsValidScopeUninstallFails(t *testing.T) { + claude := stubClaudeUninstall(t, true) + plugins := []harness.StalePlugin{{Key: "basecamp@37signals", Scopes: []string{"user", "project"}}} + + removed, scopes := removeStaleClaudePlugins(context.Background(), claude, plugins) + + calls := readClaudeCalls(t, claude) + assert.Contains(t, calls, "plugin uninstall basecamp@37signals --scope user") + assert.Contains(t, calls, "plugin uninstall basecamp@37signals --scope project") + assert.NotContains(t, calls, "plugin uninstall basecamp@37signals\n", + "no unscoped fallback when valid scopes were attempted but failed") + assert.Empty(t, removed) + assert.Empty(t, scopes) +} diff --git a/internal/completion/cache.go b/internal/completion/cache.go index 7c3588b86..e882be5ca 100644 --- a/internal/completion/cache.go +++ b/internal/completion/cache.go @@ -10,6 +10,8 @@ import ( "path/filepath" "sync" "time" + + "github.com/basecamp/basecamp-cli/internal/config" ) // CachedProject holds project data for tab completion. @@ -366,13 +368,23 @@ func loadConfigForCompletion() *configForCompletion { loadProfilesFromFile(cfg, globalPath) } - // Repo config (walk up to find .git, then .basecamp/config.json) + // profiles is an authority key (it can redirect authenticated traffic), so + // repo/local configs must be explicitly trusted before their profiles feed + // completion — mirroring config.loadFromFile's trust gating. System/global + // configs above are trusted by location. + trust := config.LoadTrustStore(config.GlobalConfigDir()) + + // Repo config (walk up to find .git, then .basecamp/config.json). + // .git may be a file (worktree/submodule marker) rather than a directory, + // so check existence only — mirroring config.RepoConfigPath. if dir, err := os.Getwd(); err == nil { for { gitPath := filepath.Join(dir, ".git") - if fi, err := os.Stat(gitPath); err == nil && fi.IsDir() { + if _, err := os.Stat(gitPath); err == nil { repoConfig := filepath.Join(dir, ".basecamp", "config.json") - loadProfilesFromFile(cfg, repoConfig) + if trust != nil && trust.IsTrusted(repoConfig) { + loadProfilesFromFile(cfg, repoConfig) + } break } parent := filepath.Dir(dir) @@ -385,7 +397,9 @@ func loadConfigForCompletion() *configForCompletion { // Local config localPath := filepath.Join(".basecamp", "config.json") - loadProfilesFromFile(cfg, localPath) + if trust != nil && trust.IsTrusted(localPath) { + loadProfilesFromFile(cfg, localPath) + } return cfg } diff --git a/internal/completion/cache_test.go b/internal/completion/cache_test.go index 6e687a166..d92f3ac58 100644 --- a/internal/completion/cache_test.go +++ b/internal/completion/cache_test.go @@ -9,6 +9,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/config" ) func TestStore_SaveAndLoad(t *testing.T) { @@ -434,3 +436,80 @@ func TestStore_AccountsPreservedOnOtherUpdates(t *testing.T) { require.NoError(t, err) assert.Equal(t, 1, len(loaded.Accounts)) } + +// setupCompletionRepo builds a hermetic repo layout for loadConfigForCompletion +// tests: an isolated XDG_CONFIG_HOME (global config + trust store), a repo root +// containing .git (directory or worktree-style file) and .basecamp/config.json +// with a "repoprofile" profile, and CWD set to a subdirectory so the repo config +// is only reachable via the .git walk-up (not as a local config). Returns the +// repo config path. +func setupCompletionRepo(t *testing.T, gitAsFile bool) string { + t.Helper() + + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + + repoDir := t.TempDir() + gitPath := filepath.Join(repoDir, ".git") + if gitAsFile { + require.NoError(t, os.WriteFile(gitPath, []byte("gitdir: /elsewhere/worktrees/x\n"), 0o600)) + } else { + require.NoError(t, os.Mkdir(gitPath, 0o755)) + } + + require.NoError(t, os.Mkdir(filepath.Join(repoDir, ".basecamp"), 0o755)) + cfgPath := filepath.Join(repoDir, ".basecamp", "config.json") + cfgJSON := `{"profiles":{"repoprofile":{"base_url":"https://repo.example.com"}}}` + require.NoError(t, os.WriteFile(cfgPath, []byte(cfgJSON), 0o600)) + + subDir := filepath.Join(repoDir, "sub") + require.NoError(t, os.Mkdir(subDir, 0o755)) + t.Chdir(subDir) + + return cfgPath +} + +// trustConfigPath marks path as trusted in the same trust store that +// loadConfigForCompletion consults (trusted-configs.json under the global +// config dir, here redirected via XDG_CONFIG_HOME). +func trustConfigPath(t *testing.T, path string) { + t.Helper() + require.NoError(t, config.NewTrustStore(config.GlobalConfigDir()).Trust(path)) +} + +func TestLoadConfigForCompletion_TrustedRepoConfig(t *testing.T) { + cfgPath := setupCompletionRepo(t, false) + trustConfigPath(t, cfgPath) + + cfg := loadConfigForCompletion() + + require.Contains(t, cfg.Profiles, "repoprofile") + assert.Equal(t, "https://repo.example.com", cfg.Profiles["repoprofile"].BaseURL) +} + +func TestLoadConfigForCompletion_UntrustedRepoConfig(t *testing.T) { + setupCompletionRepo(t, false) + + cfg := loadConfigForCompletion() + + assert.NotContains(t, cfg.Profiles, "repoprofile") +} + +func TestLoadConfigForCompletion_GitFileWorktreeMarker(t *testing.T) { + // .git as a file (worktree/submodule marker) must anchor the repo root + // the same as a .git directory. + cfgPath := setupCompletionRepo(t, true) + trustConfigPath(t, cfgPath) + + cfg := loadConfigForCompletion() + + require.Contains(t, cfg.Profiles, "repoprofile") + assert.Equal(t, "https://repo.example.com", cfg.Profiles["repoprofile"].BaseURL) +} + +func TestLoadConfigForCompletion_UntrustedGitFileWorktreeMarker(t *testing.T) { + setupCompletionRepo(t, true) + + cfg := loadConfigForCompletion() + + assert.NotContains(t, cfg.Profiles, "repoprofile") +} diff --git a/internal/completion/completer.go b/internal/completion/completer.go index 74468eb28..7ff2f7a2d 100644 --- a/internal/completion/completer.go +++ b/internal/completion/completer.go @@ -9,6 +9,7 @@ import ( "github.com/spf13/cobra" "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/richtext" ) // CacheDirFunc returns the cache directory to use for completion. @@ -91,7 +92,7 @@ func (c *Completer) ProjectCompletion() cobra.CompletionFunc { // Use ID as completion value with name as description completion := cobra.CompletionWithDesc( fmt.Sprintf("%d", p.ID), - p.Name, + sanitizeCompletionDesc(p.Name), ) completions = append(completions, completion) } @@ -114,6 +115,12 @@ func (c *Completer) ProjectNameCompletion() cobra.CompletionFunc { toCompleteLower := strings.ToLower(toComplete) var completions []cobra.Completion for _, p := range ranked { + // Skip names carrying control characters: the value must round-trip + // verbatim for name resolution, so it can't be sanitized. The + // project stays reachable by ID. + if hasControlChars(p.Name) { + continue + } nameLower := strings.ToLower(p.Name) if strings.HasPrefix(nameLower, toCompleteLower) || strings.Contains(nameLower, toCompleteLower) { @@ -164,7 +171,7 @@ func (c *Completer) PeopleCompletion() cobra.CompletionFunc { } completion := cobra.CompletionWithDesc( fmt.Sprintf("%d", p.ID), - desc, + sanitizeCompletionDesc(desc), ) completions = append(completions, completion) } @@ -199,6 +206,12 @@ func (c *Completer) PeopleNameCompletion() cobra.CompletionFunc { }) for _, p := range sorted { + // Skip names carrying control characters: the value must round-trip + // verbatim for name resolution, so it can't be sanitized. The + // person stays reachable by ID. + if hasControlChars(p.Name) { + continue + } nameLower := strings.ToLower(p.Name) if strings.HasPrefix(nameLower, toCompleteLower) || strings.Contains(nameLower, toCompleteLower) { @@ -237,7 +250,7 @@ func (c *Completer) AccountCompletion() cobra.CompletionFunc { strings.HasPrefix(nameLower, toCompleteLower) || strings.Contains(nameLower, toCompleteLower) { // Use ID as completion value with name as description - completions = append(completions, cobra.CompletionWithDesc(idStr, a.Name)) + completions = append(completions, cobra.CompletionWithDesc(idStr, sanitizeCompletionDesc(a.Name))) } } @@ -266,11 +279,17 @@ func (c *Completer) ProfileCompletion() cobra.CompletionFunc { toCompleteLower := strings.ToLower(toComplete) var completions []cobra.Completion for _, p := range sorted { + // Skip names carrying control characters: the profile name is the + // completion value and must round-trip verbatim, so it can't be + // sanitized. + if hasControlChars(p.Name) { + continue + } nameLower := strings.ToLower(p.Name) if strings.HasPrefix(nameLower, toCompleteLower) || strings.Contains(nameLower, toCompleteLower) { // Use name as completion value with base URL as description - completions = append(completions, cobra.CompletionWithDesc(p.Name, p.BaseURL)) + completions = append(completions, cobra.CompletionWithDesc(p.Name, sanitizeCompletionDesc(p.BaseURL))) } } @@ -278,6 +297,38 @@ func (c *Completer) ProfileCompletion() cobra.CompletionFunc { } } +// sanitizeCompletionDesc strips terminal escape sequences and control +// characters from a completion description. Descriptions can carry API- or +// config-controlled strings (project/person/account names, profile base_url) +// which the shell renders to the terminal; stripping them prevents terminal +// injection. +// +// It delegates to richtext.SanitizeTerminal for the ANSI-aware pass so a full +// escape sequence like ESC[31m is removed whole (rather than leaving "[31m" +// litter after only the ESC byte is dropped), then drops any newlines, carriage +// returns, and tabs that SanitizeTerminal preserves so descriptions stay on a +// single line. Ordinary spaces and other printable text are left untouched, so +// hasControlChars only flags genuine control/escape content. +func sanitizeCompletionDesc(s string) string { + s = richtext.SanitizeTerminal(s) + return strings.Map(func(r rune) rune { + if r == '\n' || r == '\r' || r == '\t' { + return -1 + } + return r + }, s) +} + +// hasControlChars reports whether s would be altered by sanitizeCompletionDesc: +// it contains control characters (C0 including ESC, DEL, or C1) or invalid +// UTF-8 (strings.Map rewrites invalid bytes to U+FFFD, so a raw C1 byte like +// \x9b also differs). Used to SKIP name-valued completion candidates outright — +// sanitizing a completion VALUE would break resolution, which matches by the +// exact name string. +func hasControlChars(s string) bool { + return s != sanitizeCompletionDesc(s) +} + // rankProjects returns projects sorted by priority: // 1. HQ (purpose="hq") // 2. Bookmarked diff --git a/internal/completion/completer_test.go b/internal/completion/completer_test.go index e21cd30ea..4ab86c3f1 100644 --- a/internal/completion/completer_test.go +++ b/internal/completion/completer_test.go @@ -24,6 +24,32 @@ func newTestCmd() *cobra.Command { return cmd } +func TestSanitizeCompletionDesc(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"C0 control SOH stripped", "a\x01b", "ab"}, + {"C0 control US stripped", "a\x1fb", "ab"}, + {"DEL stripped", "a\x7fb", "ab"}, + {"C1 control 0x80 stripped", "a\u0080b", "ab"}, + {"C1 control 0x9f stripped", "a\u009fb", "ab"}, + {"valid accented rune passes through", "café", "café"}, + {"valid emoji rune passes through", "party🎉", "party🎉"}, + {"full ANSI escape sequence stripped whole", "\x1b[31mred\x1b[0m", "red"}, + {"internal double space preserved", "a b", "a b"}, + {"newline dropped", "a\nb", "ab"}, + {"tab dropped", "a\tb", "ab"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, sanitizeCompletionDesc(tt.in)) + }) + } +} + func TestRankProjects(t *testing.T) { now := time.Now() projects := []CachedProject{ @@ -336,3 +362,76 @@ func TestCompleterAccountEmptyCache(t *testing.T) { assert.Len(t, completions, 0, "expected no completions with empty cache") assert.Equal(t, cobra.ShellCompDirectiveNoFileComp, directive, "expected NoFileComp directive") } + +func TestHasControlChars(t *testing.T) { + assert.False(t, hasControlChars("Normal Name")) + assert.False(t, hasControlChars("café party🎉")) + assert.False(t, hasControlChars("Double Space"), "internal double space is benign, not a control char") + assert.True(t, hasControlChars("Evil\x1b[31m"), "ESC (C0)") + assert.True(t, hasControlChars("\x1b[31mred\x1b[0m"), "full ANSI SGR escape sequence") + assert.True(t, hasControlChars("Evil\u009b31m"), "CSI (C1)") + assert.True(t, hasControlChars("Evil\x9bRaw"), "raw C1 byte (invalid UTF-8)") + assert.True(t, hasControlChars("Evil\x7f"), "DEL") +} + +// Name-valued completions can't sanitize the value (resolution matches the +// exact name string), so candidates whose names carry control characters — +// ESC (C0) or CSI (C1) sequences that could inject into the user's terminal +// via the completion machinery — must be skipped entirely. +func TestProjectNameCompletionSkipsControlCharNames(t *testing.T) { + tmpDir := t.TempDir() + store := NewStore(tmpDir) + + require.NoError(t, store.UpdateProjects([]CachedProject{ + {ID: 1, Name: "Good Project"}, + {ID: 2, Name: "Evil\x1b]0;pwned\x07Project"}, + {ID: 3, Name: "Evil\u009b31mProject"}, + })) + + fn := newTestCompleter(tmpDir).ProjectNameCompletion() + completions, _ := fn(newTestCmd(), nil, "") + + require.Len(t, completions, 1) + assert.Equal(t, "Good Project", completions[0]) +} + +func TestPeopleNameCompletionSkipsControlCharNames(t *testing.T) { + tmpDir := t.TempDir() + store := NewStore(tmpDir) + + require.NoError(t, store.UpdatePeople([]CachedPerson{ + {ID: 1, Name: "Alice Smith"}, + {ID: 2, Name: "Bob\x1b[2JJones"}, + {ID: 3, Name: "Carol\u009b31mWilliams"}, + })) + + fn := newTestCompleter(tmpDir).PeopleNameCompletion() + completions, _ := fn(newTestCmd(), nil, "a") + + // "a" matches Alice and the malicious C-a-rol name; only Alice survives + // the control-char skip ("me" doesn't match "a", Bob doesn't contain "a"). + require.Len(t, completions, 1) + assert.Equal(t, "Alice Smith", completions[0]) +} + +func TestProfileCompletionSkipsControlCharNames(t *testing.T) { + // Profiles load from config files; isolate global config and make sure + // no repo/local config is reachable from CWD. + configHome := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", configHome) + t.Chdir(t.TempDir()) + + require.NoError(t, os.MkdirAll(filepath.Join(configHome, "basecamp"), 0o700)) + cfgJSON := `{"profiles":{` + + `"good":{"base_url":"https://good.example.com"},` + + `"evil\u001b[31m":{"base_url":"https://evil.example.com"},` + + `"evil\u009bcsi":{"base_url":"https://evil2.example.com"}}}` + require.NoError(t, os.WriteFile( + filepath.Join(configHome, "basecamp", "config.json"), []byte(cfgJSON), 0o600)) + + fn := newTestCompleter(t.TempDir()).ProfileCompletion() + completions, _ := fn(newTestCmd(), nil, "") + + require.Len(t, completions, 1) + assert.Equal(t, "good\thttps://good.example.com", completions[0]) +} diff --git a/internal/config/config.go b/internal/config/config.go index e9662ac8c..5efb2a150 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -4,6 +4,7 @@ package config import ( "encoding/json" "fmt" + "net/url" "os" "path/filepath" "strings" @@ -144,7 +145,9 @@ func Load(overrides FlagOverrides) (*Config, error) { } // Load from environment - LoadFromEnv(cfg) + if err := LoadFromEnv(cfg); err != nil { + return nil, err + } // Apply flag overrides ApplyOverrides(cfg, overrides) @@ -196,12 +199,24 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { cfg.Sources["scope"] = string(source) } if v, ok := fileCfg["cache_dir"].(string); ok && v != "" { - cfg.CacheDir = v - cfg.Sources["cache_dir"] = string(source) + // cache_dir redirects every cache write (completion, resilience, TUI + // workspace, recents, traces). An untrusted local/repo config could + // point it at any user-writable path, so gate it like other authority + // keys. filepath.Clean normalizes the accepted value. + if untrusted { + fmt.Fprintf(os.Stderr, "warning: ignoring cache_dir %q from %s config at %s\n (trust-gated key from local/repo config; run `basecamp config trust %s` to allow)\n", v, source, path, ShellQuote(path)) + } else { + cfg.CacheDir = filepath.Clean(v) + cfg.Sources["cache_dir"] = string(source) + } } if v, ok := fileCfg["cache_enabled"].(bool); ok { - cfg.CacheEnabled = v - cfg.Sources["cache_enabled"] = string(source) + if untrusted { + fmt.Fprintf(os.Stderr, "warning: ignoring cache_enabled from %s config at %s\n (trust-gated key from local/repo config; run `basecamp config trust %s` to allow)\n", source, path, ShellQuote(path)) + } else { + cfg.CacheEnabled = v + cfg.Sources["cache_enabled"] = string(source) + } } if v, ok := fileCfg["format"].(string); ok && v != "" { cfg.Format = v @@ -237,8 +252,14 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { } } if v, ok := fileCfg["llm_model"].(string); ok && v != "" { - cfg.LLMModel = v - cfg.Sources["llm_model"] = string(source) + // Gate like other LLM authority keys: an untrusted config could + // silently substitute a costlier paid model. + if untrusted { + fmt.Fprintf(os.Stderr, "warning: ignoring llm_model %q from %s config at %s\n (trust-gated key from local/repo config; run `basecamp config trust %s` to allow)\n", v, source, path, ShellQuote(path)) + } else { + cfg.LLMModel = v + cfg.Sources["llm_model"] = string(source) + } } if v, ok := fileCfg["llm_api_key"].(string); ok && v != "" { // Secret: only from global/system config, never local/repo @@ -253,6 +274,13 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { if untrusted { fmt.Fprintf(os.Stderr, "warning: ignoring llm_endpoint %q from %s config at %s\n (authority key from local/repo config; run `basecamp config trust %s` to allow)\n", v, source, path, ShellQuote(path)) } else { + // Keep the value even if malformed (non-http(s)/hostless): + // summarize.ValidateEndpoint rejects it at the point of + // consumption (the TUI's LLM path) when the provider sends + // llm_api_key to the endpoint, failing closed rather than + // silently dropping it and falling back to the default provider. + // Other commands never consume it, so `config unset llm_endpoint` + // can always repair. cfg.LLMEndpoint = v cfg.Sources["llm_endpoint"] = string(source) } @@ -260,7 +288,11 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { if v, ok := fileCfg["llm_max_concurrent"]; ok { if fv, ok := v.(float64); ok { iv := int(fv) - if iv >= 1 && iv <= 10 && fv == float64(iv) { + // Gate like other LLM authority keys: block a malicious repo from + // inflating paid-LLM concurrency (cost amplification). + if untrusted { + fmt.Fprintf(os.Stderr, "warning: ignoring llm_max_concurrent from %s config at %s\n (trust-gated key from local/repo config; run `basecamp config trust %s` to allow)\n", source, path, ShellQuote(path)) + } else if iv >= 1 && iv <= 10 && fv == float64(iv) { cfg.LLMMaxConcurrent = iv cfg.Sources["llm_max_concurrent"] = string(source) } @@ -269,7 +301,10 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { if v, ok := fileCfg["llm_token_budget"]; ok { if fv, ok := v.(float64); ok { iv := int(fv) - if iv >= 100 && iv <= 100000 && fv == float64(iv) { + // Gate like other LLM authority keys (cost amplification). + if untrusted { + fmt.Fprintf(os.Stderr, "warning: ignoring llm_token_budget from %s config at %s\n (trust-gated key from local/repo config; run `basecamp config trust %s` to allow)\n", source, path, ShellQuote(path)) + } else if iv >= 100 && iv <= 100000 && fv == float64(iv) { cfg.LLMTokenBudget = iv cfg.Sources["llm_token_budget"] = string(source) } @@ -335,7 +370,20 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { // LoadFromEnv loads configuration from environment variables. // Exported so root.go can re-apply after profile overlay. -func LoadFromEnv(cfg *Config) { +// +// Values are stored as-is, malformed or not — mirroring the trusted-file +// path in loadFromFile. Validation happens at use-time in +// summarize.ValidateEndpoint (the TUI's LLM path, the only consumer of +// llm_endpoint), which fails closed for providers that send llm_api_key +// to the endpoint. Other commands never consume the value, so +// `basecamp config unset llm_endpoint` (or unsetting the env var) can +// always repair a bad value. Erroring here would block every command — +// including the repair path — for all providers, even ones that never +// consume the endpoint. +// +// Always returns nil today; the error return is kept so validation can be +// reintroduced for a specific variable without churning every caller. +func LoadFromEnv(cfg *Config) error { if v := os.Getenv("BASECAMP_BASE_URL"); v != "" { cfg.BaseURL = v cfg.Sources["base_url"] = string(SourceEnv) @@ -385,9 +433,17 @@ func LoadFromEnv(cfg *Config) { cfg.Sources["llm_api_key"] = string(SourceEnv) } if v := os.Getenv("BASECAMP_LLM_ENDPOINT"); v != "" { + // Keep the value even if malformed (non-http(s)/hostless), mirroring + // the trusted-file path in loadFromFile: summarize.ValidateEndpoint + // rejects it at the point of consumption (the TUI's LLM path) for + // providers that send llm_api_key to the endpoint, failing closed + // rather than silently dropping it and falling back to the default + // provider. Other commands never consume it, so + // `config unset llm_endpoint` can always repair. cfg.LLMEndpoint = v cfg.Sources["llm_endpoint"] = string(SourceEnv) } + return nil } // NonInteractiveEnv reports whether BASECAMP_NONINTERACTIVE is set to a truthy @@ -675,6 +731,19 @@ func NormalizeBaseURL(url string) string { return strings.TrimSuffix(url, "/") } +// IsHTTPURL reports whether rawURL is an absolute http(s) URL with a host. +// Used to reject non-web schemes (file://, etc.) and hostless forms +// (e.g. "https:example.com", which url.Parse accepts with an empty Host) +// for llm_endpoint, since those would later be concatenated into a request +// URL and produce confusing/invalid requests. +func IsHTTPURL(rawURL string) bool { + u, err := url.Parse(rawURL) + if err != nil { + return false + } + return (u.Scheme == "http" || u.Scheme == "https") && u.Hostname() != "" +} + // ShellQuote returns a POSIX single-quoted string safe for copy-paste into // a shell. Single quotes inside the value are escaped as '\” (end quote, // escaped literal quote, resume quote). diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 60d9244b9..08a2cf278 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -117,7 +117,7 @@ func TestLoadFromEnv(t *testing.T) { os.Setenv("BASECAMP_CACHE_ENABLED", "false") cfg := Default() - LoadFromEnv(cfg) + require.NoError(t, LoadFromEnv(cfg)) // Verify values loaded assert.Equal(t, "http://env.example.com", cfg.BaseURL) @@ -149,7 +149,7 @@ func TestLoadFromEnvPrecedence(t *testing.T) { os.Setenv("BASECAMP_BASE_URL", "http://basecamp.example.com") cfg := Default() - LoadFromEnv(cfg) + require.NoError(t, LoadFromEnv(cfg)) assert.Equal(t, "http://basecamp.example.com", cfg.BaseURL) } @@ -286,7 +286,7 @@ func TestFullLayeringPrecedence(t *testing.T) { // Apply layers in order loadFromFile(cfg, globalConfig, SourceGlobal, nil) loadFromFile(cfg, localConfig, SourceLocal, nil) - LoadFromEnv(cfg) + require.NoError(t, LoadFromEnv(cfg)) ApplyOverrides(cfg, FlagOverrides{ // No flag overrides }) @@ -382,7 +382,7 @@ func TestCacheEnabledEnvParsing(t *testing.T) { cfg := Default() cfg.CacheEnabled = tt.startValue - LoadFromEnv(cfg) + require.NoError(t, LoadFromEnv(cfg)) assert.Equal(t, tt.expected, cfg.CacheEnabled) }) @@ -404,7 +404,7 @@ func TestCacheEnabledEnvEmpty(t *testing.T) { cfg := Default() cfg.CacheEnabled = true - LoadFromEnv(cfg) + require.NoError(t, LoadFromEnv(cfg)) // Should remain true (env var not set, so doesn't change) assert.True(t, cfg.CacheEnabled) @@ -781,6 +781,195 @@ func TestLoadFromFile_BaseURLRejectedFromLocal(t *testing.T) { assert.Equal(t, "https://3.basecampapi.com", cfg.BaseURL, "local config should not override base_url") } +// TestLoadFromFile_AuthorityKeysRejectedFromLocal verifies the sibling +// authority keys closed by the security audit (cache_dir, cache_enabled, +// llm_model, llm_max_concurrent, llm_token_budget) are ignored — with a +// warning — when they appear in an untrusted local/repo config, while their +// default values are preserved. +func TestLoadFromFile_AuthorityKeysRejectedFromLocal(t *testing.T) { + defaults := Default() + + cases := []struct { + name string + json string + warnFrag string + assertDef func(t *testing.T, cfg *Config) + }{ + { + name: "cache_dir", + json: `{"cache_dir":"/home/victim/.ssh"}`, + warnFrag: "ignoring cache_dir", + assertDef: func(t *testing.T, cfg *Config) { + assert.Equal(t, defaults.CacheDir, cfg.CacheDir) + assert.Empty(t, cfg.Sources["cache_dir"]) + }, + }, + { + name: "cache_enabled", + json: `{"cache_enabled":false}`, + warnFrag: "ignoring cache_enabled", + assertDef: func(t *testing.T, cfg *Config) { + assert.Equal(t, defaults.CacheEnabled, cfg.CacheEnabled) + assert.Empty(t, cfg.Sources["cache_enabled"]) + }, + }, + { + name: "llm_model", + json: `{"llm_model":"gpt-4-turbo-expensive"}`, + warnFrag: "ignoring llm_model", + assertDef: func(t *testing.T, cfg *Config) { + assert.Equal(t, defaults.LLMModel, cfg.LLMModel) + assert.Empty(t, cfg.Sources["llm_model"]) + }, + }, + { + name: "llm_max_concurrent", + json: `{"llm_max_concurrent":10}`, + warnFrag: "ignoring llm_max_concurrent", + assertDef: func(t *testing.T, cfg *Config) { + assert.Equal(t, defaults.LLMMaxConcurrent, cfg.LLMMaxConcurrent) + assert.Empty(t, cfg.Sources["llm_max_concurrent"]) + }, + }, + { + name: "llm_token_budget", + json: `{"llm_token_budget":100000}`, + warnFrag: "ignoring llm_token_budget", + assertDef: func(t *testing.T, cfg *Config) { + assert.Equal(t, defaults.LLMTokenBudget, cfg.LLMTokenBudget) + assert.Empty(t, cfg.Sources["llm_token_budget"]) + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.json") + require.NoError(t, os.WriteFile(configPath, []byte(tc.json), 0644)) + + origStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + cfg := Default() + loadFromFile(cfg, configPath, SourceLocal, nil) // nil trust → untrusted + + w.Close() + var buf [1024]byte + n, _ := r.Read(buf[:]) + os.Stderr = origStderr + + assert.Contains(t, string(buf[:n]), tc.warnFrag) + tc.assertDef(t, cfg) + }) + } +} + +// TestLoadFromFile_AuthorityKeysAcceptedFromGlobal confirms the same keys are +// honored from a trusted (global) source, so the gating doesn't over-reach. +func TestLoadFromFile_AuthorityKeysAcceptedFromGlobal(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{ + "cache_dir":"/var/cache/basecamp", + "cache_enabled":false, + "llm_model":"gpt-4", + "llm_max_concurrent":7, + "llm_token_budget":5000 + }`), 0644)) + + cfg := Default() + loadFromFile(cfg, configPath, SourceGlobal, nil) + + assert.Equal(t, "/var/cache/basecamp", cfg.CacheDir) + assert.False(t, cfg.CacheEnabled) + assert.Equal(t, "gpt-4", cfg.LLMModel) + assert.Equal(t, 7, cfg.LLMMaxConcurrent) + assert.Equal(t, 5000, cfg.LLMTokenBudget) +} + +// TestLoadFromFile_LLMEndpointMalformedKept verifies a malformed (non-http(s) or +// hostless) llm_endpoint from a trusted source is KEPT rather than warned+dropped. +// Dropping it would silently fall back to the default provider; keeping it lets +// the dev-gated TUI summarize path fail closed at consumption +// (summarize.ValidateEndpoint in workspace.NewSession) while config subcommands +// can still repair via `config unset llm_endpoint`. +func TestLoadFromFile_LLMEndpointMalformedKept(t *testing.T) { + for _, bad := range []string{"file:///etc/passwd", "https:example.com", "ssh://host/x"} { + t.Run(bad, func(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.json") + body, _ := json.Marshal(map[string]any{"llm_endpoint": bad}) + require.NoError(t, os.WriteFile(configPath, body, 0644)) + + cfg := Default() + loadFromFile(cfg, configPath, SourceGlobal, nil) + + assert.Equal(t, bad, cfg.LLMEndpoint) + assert.Equal(t, "global", cfg.Sources["llm_endpoint"]) + }) + } +} + +// TestLoadFromEnv_LLMEndpointMalformedKept verifies a non-http(s) or hostless +// BASECAMP_LLM_ENDPOINT is stored as-is rather than erroring at load time, +// mirroring the trusted-file path. Erroring here would block every command — +// including the `config unset llm_endpoint` repair path — for all providers. +// summarize.ValidateEndpoint (called from workspace.NewSession, the only +// consumer of llm_endpoint) rejects the value at use time for providers that +// send llm_api_key to the endpoint, so the typo still surfaces instead of +// silently falling back to the default provider. +func TestLoadFromEnv_LLMEndpointMalformedKept(t *testing.T) { + for _, bad := range []string{"file:///etc/passwd", "https:example.com", "ssh://host/x"} { + t.Run(bad, func(t *testing.T) { + t.Setenv("BASECAMP_LLM_ENDPOINT", bad) + + cfg := Default() + require.NoError(t, LoadFromEnv(cfg)) + + assert.Equal(t, bad, cfg.LLMEndpoint) + assert.Equal(t, "env", cfg.Sources["llm_endpoint"]) + }) + } +} + +// TestLoadFromEnv_LLMEndpointAccepted verifies a valid endpoint loads with env +// provenance and no error. +func TestLoadFromEnv_LLMEndpointAccepted(t *testing.T) { + t.Setenv("BASECAMP_LLM_ENDPOINT", "https://api.openai.com/v1") + + cfg := Default() + require.NoError(t, LoadFromEnv(cfg)) + + assert.Equal(t, "https://api.openai.com/v1", cfg.LLMEndpoint) + assert.Equal(t, "env", cfg.Sources["llm_endpoint"]) +} + +// TestLoadFromEnv_LLMEndpointEmpty verifies an unset BASECAMP_LLM_ENDPOINT is a +// no-op: no error, and no endpoint or provenance recorded. +func TestLoadFromEnv_LLMEndpointEmpty(t *testing.T) { + t.Setenv("BASECAMP_LLM_ENDPOINT", "") + + cfg := Default() + require.NoError(t, LoadFromEnv(cfg)) + + assert.Empty(t, cfg.LLMEndpoint) + assert.Empty(t, cfg.Sources["llm_endpoint"]) +} + +// TestIsHTTPURL covers the scheme+host validation reused at llm_endpoint's +// call sites: config-set acceptance (internal/commands/config.go) and +// consumption-time validation (summarize.ValidateEndpoint). +func TestIsHTTPURL(t *testing.T) { + for _, ok := range []string{"http://localhost:11434", "https://api.openai.com/v1", "http://127.0.0.1:8080/x", "http://host:8080", "https://host", "http://[::1]:8080"} { + assert.True(t, IsHTTPURL(ok), "%q should be accepted", ok) + } + for _, bad := range []string{"", "file:///etc/passwd", "ssh://h/x", "https:example.com", "https://", "not a url", "/relative", "http://:8080", "https://:443"} { + assert.False(t, IsHTTPURL(bad), "%q should be rejected", bad) + } +} + func TestLoadFromFile_BaseURLNoWarningForGlobal(t *testing.T) { tmpDir := t.TempDir() configPath := filepath.Join(tmpDir, "config.json") @@ -961,7 +1150,7 @@ func TestPreferencesFromEnv(t *testing.T) { os.Setenv("BASECAMP_STATS", "0") cfg := Default() - LoadFromEnv(cfg) + require.NoError(t, LoadFromEnv(cfg)) require.NotNil(t, cfg.Hints) assert.True(t, *cfg.Hints) @@ -994,7 +1183,7 @@ func TestPreferencesEnvOverridesFile(t *testing.T) { cfg := Default() loadFromFile(cfg, configPath, SourceGlobal, nil) - LoadFromEnv(cfg) + require.NoError(t, LoadFromEnv(cfg)) require.NotNil(t, cfg.Hints) assert.False(t, *cfg.Hints, "env should override file") @@ -1042,7 +1231,7 @@ func TestEnvInvalidBoolIgnored(t *testing.T) { os.Setenv("BASECAMP_HINTS", "banana") cfg := Default() - LoadFromEnv(cfg) + require.NoError(t, LoadFromEnv(cfg)) assert.Nil(t, cfg.Hints, "invalid env bool should leave pointer nil") } diff --git a/internal/output/envelope.go b/internal/output/envelope.go index 1bf01cd88..fa83ca6df 100644 --- a/internal/output/envelope.go +++ b/internal/output/envelope.go @@ -1,10 +1,13 @@ package output import ( + "bytes" "encoding/json" "fmt" "io" "os" + "sort" + "strconv" "strings" "github.com/itchyny/gojq" @@ -13,6 +16,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/observability" "github.com/basecamp/basecamp-cli/internal/presenter" + "github.com/basecamp/basecamp-cli/internal/richtext" ) // NormalizeData converts json.RawMessage and other types to standard Go types. @@ -202,7 +206,13 @@ func (w *Writer) write(v any) error { // early-return so that --agent --jq still emits the diagnostic. if w.opts.Format == FormatQuiet { if resp, ok := v.(*Response); ok && resp.noticeDiagnostic && resp.Notice != "" { - fmt.Fprintf(w.opts.ErrWriter, "notice: %s\n", resp.Notice) + // The notice may interpolate API-controlled strings and stderr is + // a terminal sink; sanitize and keep the diagnostic to one line. + // Sanitize first, then gate: an all-escape notice collapses to "" + // and must not emit a blank "notice: " line. + if notice := sanitizeText(resp.Notice, true, false); notice != "" { + fmt.Fprintf(w.opts.ErrWriter, "notice: %s\n", notice) + } } } @@ -282,6 +292,12 @@ func (w *Writer) writeJQ(v any) error { return ErrJQRuntime(fmt.Errorf("unmarshal: %w", err)) } + // Sanitization is TTY-gated: --jq to a terminal strips escape sequences + // and control characters to prevent terminal injection, while piped or + // redirected output is the machine-consumption contract (matching + // writeJSON) and passes bytes through verbatim. + tty := isTTY(w.opts.Writer) + iter := code.Run(input) for { result, ok := iter.Next() @@ -291,10 +307,24 @@ func (w *Writer) writeJQ(v any) error { if err, ok := result.(error); ok { return ErrJQRuntime(err) } - // Print strings without JSON encoding for cleaner output + // Print strings without JSON encoding for cleaner output. + // On a TTY, SanitizeTerminal guards against terminal injection: --jq + // can select API-controlled strings, and unlike JSON marshaling (which + // escapes control bytes) this path would otherwise emit raw OSC/CSI + // sequences. if s, ok := result.(string); ok { + if tty { + s = richtext.SanitizeTerminal(s) + } fmt.Fprintln(w.opts.Writer, s) } else { + // Compound results (arrays/objects) are marshaled, but Go's JSON + // encoder only escapes C0 controls — UTF-8-encoded C1 controls + // (U+0080–U+009F) pass through raw and execute on C1-honoring + // terminals. On a TTY, sanitize every string leaf (and key) first. + if tty { + result = sanitizeJSONValue(result) + } raw, err := json.Marshal(result) if err != nil { return ErrJQRuntime(fmt.Errorf("result not serializable: %w", err)) @@ -305,8 +335,64 @@ func (w *Writer) writeJQ(v any) error { return nil } -// isTTY checks if the writer is a terminal. -func isTTY(w io.Writer) bool { +// sanitizeJSONValue recursively strips terminal escape sequences and control +// characters from every string in a jq result — map keys, map values, and +// slice elements — before it is marshaled for a TTY-bound --jq sink. +// Non-string leaves (numbers, bools, nil, and gojq's numeric types) pass +// through unchanged. +func sanitizeJSONValue(v any) any { + switch val := v.(type) { + case string: + return richtext.SanitizeTerminal(val) + case []any: + for i, elem := range val { + val[i] = sanitizeJSONValue(elem) + } + return val + case map[string]any: + return sanitizeJSONMap(val) + default: + return v + } +} + +// sanitizeJSONMap rebuilds a map with terminal-safe keys without ever +// dropping an entry. Stripping keys would let a hostile "\x1b[31mtitle" +// collapse onto a legitimate "title" and silently replace or hide that +// field. Instead, keys already free of escapes and controls keep their name +// — a hostile key can never displace one — and keys changed by sanitization +// are visibly escaped (strconv.Quote), re-quoting with delimiters until the +// name is unique so even a literal key that mimics escape notation cannot +// be overwritten. Escaped keys are processed in sorted order so the +// resulting names are deterministic. +func sanitizeJSONMap(val map[string]any) map[string]any { + out := make(map[string]any, len(val)) + var escaped []string + for k, elem := range val { + if richtext.SanitizeTerminal(k) == k { + out[k] = sanitizeJSONValue(elem) + } else { + escaped = append(escaped, k) + } + } + sort.Strings(escaped) + for _, k := range escaped { + quoted := strconv.Quote(k) + name := quoted[1 : len(quoted)-1] + for { + if _, taken := out[name]; !taken { + break + } + name = strconv.Quote(name) + } + out[name] = sanitizeJSONValue(val[k]) + } + return out +} + +// isTTY checks if the writer is a terminal. It is a package variable so tests +// can simulate a TTY for in-memory writers. +var isTTY = func(w io.Writer) bool { if f, ok := w.(*os.File); ok { fi, err := f.Stat() if err != nil { @@ -317,6 +403,15 @@ func isTTY(w io.Writer) bool { return false } +// writeJSON emits the full envelope as JSON. Sanitization is TTY-gated, +// matching writeJQ: when the target is a pipe or file, JSON is the +// machine-consumption contract and every byte passes through verbatim so +// data fidelity is preserved for programmatic consumers (FormatAuto only +// selects JSON when stdout is not a TTY, so this is the common case). When +// the target is a terminal — forced --json/--agent on an interactive TTY — +// string leaves are C1/escape-sanitized (via sanitizeJSONValue) so raw C1 +// controls cannot execute on a C1-honoring terminal. Other terminal-facing +// sinks (styled, markdown, --ids, and --jq on a TTY) sanitize the same way. func (w *Writer) writeJSON(v any) error { toEncode := v if resp, ok := v.(*Response); ok { @@ -325,6 +420,29 @@ func (w *Writer) writeJSON(v any) error { respCopy.Data = NormalizeData(resp.Data) toEncode = &respCopy } + // On a TTY, sanitize string leaves to strip C1/escape controls: Go's JSON + // encoder escapes C0 controls but passes UTF-8-encoded C1 controls + // (U+0080–U+009F) through raw. Piped/redirected output stays verbatim. + if isTTY(w.opts.Writer) { + raw, err := json.Marshal(toEncode) + if err != nil { + return err + } + // Decode with UseNumber so JSON numbers stay json.Number (a named + // string type) instead of float64. Large Basecamp IDs (>2^53) would + // otherwise lose precision or render in exponent form. json.Number + // passes through sanitizeJSONValue untouched (its `case string:` does + // not match a named type), so the exact numeric text is preserved. + dec := json.NewDecoder(bytes.NewReader(raw)) + dec.UseNumber() + var decoded any + if err := dec.Decode(&decoded); err != nil { + return err + } + enc := json.NewEncoder(w.opts.Writer) + enc.SetIndent("", " ") + return enc.Encode(sanitizeJSONValue(decoded)) + } enc := json.NewEncoder(w.opts.Writer) enc.SetIndent("", " ") return enc.Encode(toEncode) @@ -350,17 +468,29 @@ func (w *Writer) writeIDs(v any) error { case []map[string]any: for _, item := range d { if id, ok := item["id"]; ok { - fmt.Fprintln(w.opts.Writer, id) + fmt.Fprintln(w.opts.Writer, stripIfString(id)) } } case map[string]any: if id, ok := d["id"]; ok { - fmt.Fprintln(w.opts.Writer, id) + fmt.Fprintln(w.opts.Writer, stripIfString(id)) } } return nil } +// stripIfString makes a string value safe for the line-oriented IDs sink: +// escape sequences and control characters are stripped, and remaining +// whitespace (including the newlines and tabs SanitizeTerminal preserves) +// collapses to single spaces so one value cannot span or inject extra +// lines. Non-string values (json.Number ids, etc.) pass through unchanged. +func stripIfString(v any) any { + if s, ok := v.(string); ok { + return sanitizeText(s, true, false) + } + return v +} + func (w *Writer) writeCount(v any) error { resp, ok := v.(*Response) if !ok { @@ -427,12 +557,16 @@ func (w *Writer) writeLiteralMarkdown(v any) error { type ResponseOption func(*Response) // WithSummary adds a summary to the response. +// The value is stored verbatim so machine (JSON) output preserves the +// original content; terminal sinks (styled/markdown renderers, quiet-mode +// stderr diagnostics) sanitize at render time. func WithSummary(s string) ResponseOption { return func(r *Response) { r.Summary = s } } // WithNotice adds an informational notice to the response. // Use this for non-error messages like truncation warnings. +// Like WithSummary, the value is stored verbatim; terminal sinks sanitize. func WithNotice(s string) ResponseOption { return func(r *Response) { r.Notice = s; r.noticeDiagnostic = false } } @@ -536,17 +670,25 @@ func (w *Writer) presentStyledEntity(resp *Response) bool { var out strings.Builder r := NewRenderer(w.opts.Writer, true) - if resp.Summary != "" { - out.WriteString(r.Summary.Render(resp.Summary)) + // sanitizeText (single-line) defends against terminal injection from + // API-controlled summary/notice content and keeps each value on one line. + // Sanitization happens only here at the render sink — machine (JSON) + // output carries these verbatim. + // Sanitize first, then gate: an all-escape summary/notice collapses to "" + // and must emit no blank styled line or trailing spacer. + summary := sanitizeText(resp.Summary, true, false) + if summary != "" { + out.WriteString(r.Summary.Render(summary)) out.WriteString("\n") } - if resp.Notice != "" { - out.WriteString(r.Hint.Render(resp.Notice)) + notice := sanitizeText(resp.Notice, true, false) + if notice != "" { + out.WriteString(r.Hint.Render(notice)) out.WriteString("\n") } - if resp.Summary != "" || resp.Notice != "" { + if summary != "" || notice != "" { out.WriteString("\n") } @@ -595,15 +737,20 @@ func (w *Writer) presentMarkdownEntity(resp *Response) bool { var out strings.Builder mr := NewMarkdownRenderer(w.opts.Writer) - if resp.Summary != "" { - out.WriteString("## " + resp.Summary + "\n") + // Sink-level ANSI stripping (see presentStyledEntity). Sanitize first, then + // gate: an all-escape summary/notice collapses to "" and must not emit an + // empty "## " heading or "*...*" line. + summary := sanitizeText(resp.Summary, true, false) + if summary != "" { + out.WriteString("## " + summary + "\n") } - if resp.Notice != "" { - out.WriteString("*" + resp.Notice + "*\n") + notice := sanitizeText(resp.Notice, true, false) + if notice != "" { + out.WriteString("*" + notice + "*\n") } - if resp.Summary != "" || resp.Notice != "" { + if summary != "" || notice != "" { out.WriteString("\n") } @@ -620,9 +767,9 @@ func (w *Writer) presentMarkdownEntity(resp *Response) bool { if len(resp.Breadcrumbs) > 0 { out.WriteString("\n### Hints\n\n") for _, bc := range resp.Breadcrumbs { - line := "- `" + bc.Cmd + "`" + line := "- `" + sanitizeText(bc.Cmd, true, false) + "`" if bc.Description != "" { - line += " — " + bc.Description + line += " — " + sanitizeText(bc.Description, true, false) } out.WriteString(line + "\n") } diff --git a/internal/output/output_test.go b/internal/output/output_test.go index 834c2b9d8..1d6c72665 100644 --- a/internal/output/output_test.go +++ b/internal/output/output_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "math" "strings" "testing" @@ -593,6 +594,47 @@ func TestWriterIDsFormat(t *testing.T) { assert.Equal(t, "123\n456\n", output) } +// TestWriterIDsFormatStripsANSI verifies that string id values (an +// API-controlled raw print sink) are sanitized of terminal escape sequences. +func TestWriterIDsFormatStripsANSI(t *testing.T) { + var buf bytes.Buffer + w := New(Options{ + Format: FormatIDs, + Writer: &buf, + }) + + data := []map[string]any{ + {"id": "\x1b]0;evil\x07abc-123"}, + {"id": "\x1b[31m456\x1b[0m"}, + } + err := w.OK(data) + require.NoError(t, err) + + assert.Equal(t, "abc-123\n456\n", buf.String()) + assert.NotContains(t, buf.String(), "\x1b") +} + +// TestWriterIDsFormatSingleLine verifies that a string id cannot span or +// inject extra lines into the line-oriented IDs sink: newlines, tabs, and +// carriage returns collapse to single spaces, keeping one id per line. +func TestWriterIDsFormatSingleLine(t *testing.T) { + var buf bytes.Buffer + w := New(Options{ + Format: FormatIDs, + Writer: &buf, + }) + + data := []map[string]any{ + {"id": "abc\n999\ndef"}, + {"id": "tab\there"}, + {"id": "cr\rjoined"}, + } + err := w.OK(data) + require.NoError(t, err) + + assert.Equal(t, "abc 999 def\ntab here\ncr joined\n", buf.String()) +} + func TestWriterCountFormat(t *testing.T) { var buf bytes.Buffer w := New(Options{ @@ -2267,12 +2309,13 @@ func TestWriterStyledErrorIncludesRequestID(t *testing.T) { Code: basecamp.CodeAPI, Message: "server error", HTTPStatus: 500, - RequestID: "req-cli-123\x1b[31m\nnext", + RequestID: "req-cli-123\x1b[31m\nnext\rword", }) require.NoError(t, writeErr, "Err() failed") output := ansi.Strip(buf.String()) - assert.Contains(t, output, "Request ID: req-cli-123 next") + assert.Contains(t, output, "Request ID: req-cli-123 next word", + "CR separates words instead of joining them") assert.NotContains(t, output, "[31m") } @@ -2341,6 +2384,21 @@ func TestFormatCellStripsANSIEscapes(t *testing.T) { input: "\x1b[1;31mred bold\x1b[0m", expected: "red bold", }, + { + name: "C1 CSI survives ansi.Strip but not SanitizeTerminal", + input: string(rune(0x9b)) + "31mred" + string(rune(0x9b)) + "0m", // U+009B + expected: "31mred0m", + }, + { + name: "C1 OSC with BEL terminator", + input: string(rune(0x9d)) + "0;evil\adone", // U+009D + expected: "0;evildone", + }, + { + name: "DEL character", + input: "De\x7fleted", + expected: "Deleted", + }, } for _, tt := range tests { @@ -2351,6 +2409,65 @@ func TestFormatCellStripsANSIEscapes(t *testing.T) { } } +// TestWriterStyledTableStripsC1Controls verifies that the styled table +// renderer strips UTF-8-encoded C1 controls and DEL from API-controlled data. +// ansi.Strip alone leaves these intact, and C1-honoring terminals execute +// them directly (e.g. `api get` rendering a hostile record title). +func TestWriterStyledTableStripsC1Controls(t *testing.T) { + c1CSI := string(rune(0x9b)) // U+009B CSI + c1OSC := string(rune(0x9d)) // U+009D OSC + + var buf bytes.Buffer + w := New(Options{Format: FormatStyled, Writer: &buf}) + + data := []map[string]any{ + {"id": 1, "title": c1CSI + "31mRed alert" + c1CSI + "0m"}, + {"id": 2, "title": c1OSC + "0;evil\aDe\x7fleted"}, + } + require.NoError(t, w.OK(data)) + + out := buf.String() + assert.Contains(t, out, "Red alert") + assert.Contains(t, out, "Deleted") + assert.NotContains(t, out, c1CSI) + assert.NotContains(t, out, c1OSC) + assert.NotContains(t, out, "\x7f") + assert.NotContains(t, out, "\a") +} + +// TestWriterStyledStringDataStripsC1Controls covers renderData's string and +// detail-object paths: C1 controls and DEL in string payloads and object +// values must not reach the terminal. +func TestWriterStyledStringDataStripsC1Controls(t *testing.T) { + c1CSI := string(rune(0x9b)) // U+009B CSI + + t.Run("string data", func(t *testing.T) { + var buf bytes.Buffer + w := New(Options{Format: FormatStyled, Writer: &buf}) + require.NoError(t, w.OK(c1CSI+"31mplain string"+c1CSI+"0m")) + + out := buf.String() + assert.Contains(t, out, "plain string") + assert.NotContains(t, out, c1CSI) + }) + + t.Run("detail object", func(t *testing.T) { + var buf bytes.Buffer + w := New(Options{Format: FormatStyled, Writer: &buf}) + require.NoError(t, w.OK(map[string]any{ + "id": 1, + "title": c1CSI + "31mdetail value" + c1CSI + "0m", + "note": "De\x7fleted", + })) + + out := buf.String() + assert.Contains(t, out, "detail value") + assert.Contains(t, out, "Deleted") + assert.NotContains(t, out, c1CSI) + assert.NotContains(t, out, "\x7f") + }) +} + // ============================================================================= // WithDisplayData Contract Tests // ============================================================================= @@ -2475,6 +2592,27 @@ func TestFormatCellStripsANSIFromArrayElements(t *testing.T) { result := formatCell(input) assert.Equal(t, "Task One", result) }) + + t.Run("map elements whose only field is an escaped string id", func(t *testing.T) { + // The id arm is the last fallback; a hostile string id must be + // sanitized like its sibling arms rather than printed raw. + input := []any{ + map[string]any{"id": "12\x1b]0;pwned\x07\u009b31m3\x7f"}, + } + result := formatCell(input) + assert.NotContains(t, result, "\x1b", "ESC leaked from string id") + assertNoC1OrDEL(t, result, "formatCell string id") + assert.Contains(t, result, "123", "sanitized id digits should survive") + }) + + t.Run("map elements with non-string id are unaffected", func(t *testing.T) { + input := []any{ + map[string]any{"id": json.Number("456")}, + map[string]any{"id": float64(789)}, + } + result := formatCell(input) + assert.Equal(t, "456, 789", result) + }) } func TestFormatCellCollapsesNewlines(t *testing.T) { @@ -3146,6 +3284,376 @@ func TestWriterJQFilterStringOutput(t *testing.T) { assert.Equal(t, "Buy milk\nShip feature", lines) } +// forceTTY makes isTTY report true for the duration of the test, simulating a +// terminal-attached writer for in-memory buffers. jq output sanitization is +// TTY-gated, so tests exercising the sanitizing path must opt in. +func forceTTY(t *testing.T) { + t.Helper() + orig := isTTY + isTTY = func(io.Writer) bool { return true } + t.Cleanup(func() { isTTY = orig }) +} + +// TestWriterJQFilterStringStripsANSIOnTTY verifies that jq string results are +// sanitized of terminal escape sequences before printing to a TTY. Unlike +// JSON output (which escapes control bytes), string results print raw, so +// --jq selecting an API-controlled string must not emit OSC/CSI sequences to +// the terminal. +func TestWriterJQFilterStringStripsANSIOnTTY(t *testing.T) { + forceTTY(t) + var buf bytes.Buffer + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + JQFilter: ".data[].title", + }) + + data := []map[string]any{ + {"id": 1, "title": "\x1b]0;evil\x07Set title"}, + {"id": 2, "title": "\x1b[31mRed alert\x1b[0m"}, + } + err := w.OK(data) + require.NoError(t, err) + + assert.Equal(t, "Set title\nRed alert", strings.TrimSpace(buf.String())) + assert.NotContains(t, buf.String(), "\x1b") +} + +// TestWriterJQFilterStringStripsC1ControlsOnTTY verifies that UTF-8-encoded +// Unicode C1 controls (which survive ansi.Strip but execute on C1-honoring +// terminals) and DEL are stripped from jq string results printed to a TTY. +func TestWriterJQFilterStringStripsC1ControlsOnTTY(t *testing.T) { + forceTTY(t) + var buf bytes.Buffer + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + JQFilter: ".data[].title", + }) + + data := []map[string]any{ + {"id": 1, "title": "\u009b31mRed alert\u009b0m"}, // U+009B CSI + {"id": 2, "title": "\u009d0;evil\aSet title"}, // U+009D OSC, BEL-terminated + {"id": 3, "title": "De\u007fleted"}, // DEL + } + err := w.OK(data) + require.NoError(t, err) + + assert.Equal(t, "31mRed alert0m\n0;evilSet title\nDeleted", strings.TrimSpace(buf.String())) + assert.NotContains(t, buf.String(), "\u009b") + assert.NotContains(t, buf.String(), "\u009d") + assert.NotContains(t, buf.String(), "\u007f") + assert.NotContains(t, buf.String(), "\a") +} + +// TestWriterJQFilterCompoundStripsC1Controls verifies that compound jq +// results (arrays/objects) printed to a TTY are sanitized before marshaling. +// Go's JSON encoder escapes C0 controls but emits UTF-8-encoded C1 controls +// (U+0080–U+009F) raw, so string leaves and map keys must be stripped before +// they reach the terminal. +func TestWriterJQFilterCompoundStripsC1Controls(t *testing.T) { + forceTTY(t) + var buf bytes.Buffer + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + JQFilter: ".data", + }) + + data := []map[string]any{ + {"id": 1, "title": "\u009b31mRed alert\u009b0m"}, // U+009B CSI in value + {"id": 2, "ti\u009dtle": "nested", "note": "\u009cclean"}, // U+009D OSC in key, U+009C ST in value + {"id": 3, "nested": map[string]any{"deep": "\u0090payload\u009c"}}, // C1 in nested object + } + err := w.OK(data) + require.NoError(t, err) + + out := buf.String() + // Raw C1 bytes must not survive (U+009B is 0xC2 0x9B in UTF-8, etc.) + assert.NotContains(t, out, "\u009b") + assert.NotContains(t, out, "\u009c") + assert.NotContains(t, out, "\u009d") + assert.NotContains(t, out, "\u0090") + + // Legitimate content survives with structure intact. Keys containing + // controls are visibly escaped rather than stripped, so they can never + // collide with a clean key. + var result []map[string]any + require.NoError(t, json.Unmarshal(buf.Bytes(), &result)) + require.Len(t, result, 3) + assert.Equal(t, "31mRed alert0m", result[0]["title"]) + assert.Equal(t, "nested", result[1]["ti\\u009dtle"], "escaped map key") + assert.Equal(t, "clean", result[1]["note"]) + nested, ok := result[2]["nested"].(map[string]any) + require.True(t, ok) + assert.Equal(t, "payload", nested["deep"]) +} + +// TestWriterJQFilterCompoundKeyCollision verifies that a hostile map key +// that sanitizes to the same string as a legitimate key cannot replace or +// hide it: clean keys keep their slots and values \u2014 including a literal key +// that mimics escape notation \u2014 and hostile keys are preserved under +// visibly-escaped names, re-quoted until unique, instead of being dropped. +func TestWriterJQFilterCompoundKeyCollision(t *testing.T) { + forceTTY(t) + var buf bytes.Buffer + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + JQFilter: ".data", + }) + + data := map[string]any{ + "title": "legit", + "\x1b[31mtitle": "impostor", + "\u009b31mtitle": "c1 impostor", + "\\x1b[31mtitle": "lookalike", // literal text matching the ESC key's escaped form + } + err := w.OK(data) + require.NoError(t, err) + + var result map[string]any + require.NoError(t, json.Unmarshal(buf.Bytes(), &result)) + require.Len(t, result, 4, "no entry silently dropped") + assert.Equal(t, "legit", result["title"], "clean key keeps its slot") + assert.Equal(t, "lookalike", result["\\x1b[31mtitle"], "clean lookalike keeps its slot") + assert.Equal(t, "impostor", result["\"\\\\x1b[31mtitle\""], "colliding hostile key re-quoted") + assert.Equal(t, "c1 impostor", result["\\u009b31mtitle"]) + assert.NotContains(t, buf.String(), "\x1b") + assert.NotContains(t, buf.String(), "\u009b") +} + +// TestWriterJQFilterStringVerbatimWhenPiped verifies that jq string results +// written to a non-TTY sink (pipes, files, the default in tests) preserve +// every byte verbatim: non-TTY output is the machine-consumption contract +// (matching writeJSON), and sanitizing would silently corrupt data fidelity. +func TestWriterJQFilterStringVerbatimWhenPiped(t *testing.T) { + var buf bytes.Buffer // bytes.Buffer is not a TTY + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + JQFilter: ".data[].title", + }) + + c1CSI := string(rune(0x9b)) // U+009B, UTF-8-encoded C1 CSI + data := []map[string]any{ + {"id": 1, "title": "\x1b[31mRed alert\x1b[0m"}, // ESC-prefixed CSI + {"id": 2, "title": c1CSI + "31mStill red" + c1CSI + "0m"}, // C1 CSI + {"id": 3, "title": "De\x7fleted"}, // DEL + } + err := w.OK(data) + require.NoError(t, err) + + assert.Equal(t, + "\x1b[31mRed alert\x1b[0m\n"+c1CSI+"31mStill red"+c1CSI+"0m\nDe\x7fleted", + strings.TrimSpace(buf.String())) +} + +// TestWriterJQFilterCompoundVerbatimWhenPiped verifies that compound jq +// results written to a non-TTY sink are marshaled without sanitization — +// string leaves and map keys keep their original bytes. +func TestWriterJQFilterCompoundVerbatimWhenPiped(t *testing.T) { + var buf bytes.Buffer // bytes.Buffer is not a TTY + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + JQFilter: ".data", + }) + + c1CSI := string(rune(0x9b)) // U+009B, UTF-8-encoded C1 CSI + data := []map[string]any{ + {"id": 1, "title": c1CSI + "31mRed alert" + c1CSI + "0m"}, + } + err := w.OK(data) + require.NoError(t, err) + + var result []map[string]any + require.NoError(t, json.Unmarshal(buf.Bytes(), &result)) + require.Len(t, result, 1) + assert.Equal(t, c1CSI+"31mRed alert"+c1CSI+"0m", result[0]["title"], + "C1 bytes must round-trip through --jq when piped") +} + +// TestWriterMarkdownEntityBreadcrumbsStripEscapes verifies that the entity +// Markdown sink (presentMarkdownEntity) sanitizes API-controlled breadcrumb +// Cmd/Description before writing them, matching the generic Markdown renderer. +// Markdown output carries no legitimate ANSI, so no escape or C1 byte may +// survive. +func TestWriterMarkdownEntityBreadcrumbsStripEscapes(t *testing.T) { + var buf bytes.Buffer + w := New(Options{ + Format: FormatMarkdown, + Writer: &buf, + }) + + data := map[string]any{ + "id": float64(12345), + "content": "Fix the login bug", + "completed": false, + } + err := w.OK(data, + WithEntity("todo"), // routes through presentMarkdownEntity + WithBreadcrumbs(Breadcrumb{ + Action: "show", + Cmd: "basecamp \x1b]0;pwncmd\x07show \u009b31m1", // ESC OSC + C1 CSI + Description: "View details and \x1b[31mmore", // ESC CSI + }), + ) + require.NoError(t, err) + + out := buf.String() + assert.Contains(t, out, "Hints", "entity sink should render breadcrumbs") + assert.NotContains(t, out, "\x1b") + assert.NotContains(t, out, "\u009b") + assert.NotContains(t, out, "\u009d") + assert.NotContains(t, out, "\x7f") + assert.NotContains(t, out, "pwncmd", "OSC payload must be stripped") +} + +// TestWriterStyledFormatHeaderStripsControlsInKey verifies that an +// API-controlled DATA map KEY (rendered as a styled header/label via +// formatHeader) cannot smuggle C1 controls, DEL, or an OSC payload to the +// terminal. Styled output legitimately contains ESC-prefixed SGR codes, so +// this asserts on the injection classes that never appear in legitimate +// styling. +func TestWriterStyledFormatHeaderStripsControlsInKey(t *testing.T) { + var buf bytes.Buffer + w := New(Options{ + Format: FormatStyled, + Writer: &buf, + }) + + data := map[string]any{ + "na\x1b]0;pwnosc\x07me": "value-osc", // ESC OSC injection in key + "ti\u009b31mtle": "value-c1", // C1 CSI in key + "de\x7fl": "value-del", // DEL in key + } + err := w.OK(data) + require.NoError(t, err) + + out := buf.String() + assert.Contains(t, out, "value-osc", "cell values still render") + assert.NotContains(t, out, "pwnosc", "OSC payload must be stripped from header") + assert.NotContains(t, out, "\u009b") + assert.NotContains(t, out, "\x7f") +} + +// TestWriterMarkdownFormatHeaderStripsEscapesInKey verifies the same +// formatHeader choke point for the Markdown sink. Markdown carries no +// legitimate ANSI, so neither ESC-prefixed sequences nor C1 controls in a +// map key may reach the rendered label. +func TestWriterMarkdownFormatHeaderStripsEscapesInKey(t *testing.T) { + var buf bytes.Buffer + w := New(Options{ + Format: FormatMarkdown, + Writer: &buf, + }) + + data := map[string]any{ + "na\x1b[31mme": "value-esc", // ESC CSI in key + "ti\u009b31mtle": "value-c1", // C1 CSI in key + "de\x7fscr\x1b]0;pwnosc\x07r": "value-osc", // DEL + ESC OSC in key + } + err := w.OK(data) + require.NoError(t, err) + + out := buf.String() + assert.Contains(t, out, "value-esc", "cell values still render") + assert.NotContains(t, out, "\x1b") + assert.NotContains(t, out, "\u009b") + assert.NotContains(t, out, "\x7f") + assert.NotContains(t, out, "pwnosc") +} + +// TestWriterJSONStripsC1OnTTY verifies that forced --json/--agent output to an +// interactive terminal has string leaves sanitized of C1/escape controls: the +// Go JSON encoder escapes C0 controls but emits UTF-8-encoded C1 controls +// (U+0080-U+009F) raw, so a hostile value would otherwise execute on a +// C1-honoring terminal. +func TestWriterJSONStripsC1OnTTY(t *testing.T) { + forceTTY(t) + var buf bytes.Buffer + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + }) + + data := map[string]any{ + "title": "\u009b31mRed alert\u009b0m", // C1 CSI + "note": "De\x7fleted", // DEL + } + err := w.OK(data) + require.NoError(t, err) + + out := buf.String() + assert.NotContains(t, out, "\u009b", "C1 CSI must be stripped on a TTY") + assert.NotContains(t, out, "\x7f", "DEL must be stripped on a TTY") + + var resp struct { + Data map[string]any `json:"data"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &resp)) + assert.Equal(t, "31mRed alert0m", resp.Data["title"]) + assert.Equal(t, "Deleted", resp.Data["note"]) +} + +// TestWriterJSONPreservesLargeIntOnTTY verifies that the TTY-sanitize path +// preserves large integer IDs exactly. The path round-trips through +// json.Marshal → json.Decoder, and decoding must use UseNumber so numbers stay +// json.Number rather than float64 — otherwise IDs above 2^53 would lose +// precision or render in exponent form (e.g. 9.007...e+15). json.Number is a +// named string type, so it does NOT match sanitizeJSONValue's `case string:` +// arm (a type switch matches only the exact string type); it falls through to +// the default case and is returned unchanged, so its exact numeric text survives. +func TestWriterJSONPreservesLargeIntOnTTY(t *testing.T) { + forceTTY(t) + var buf bytes.Buffer + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + }) + + data := map[string]any{ + "id": 9007199254740993, // 2^53 + 1: not exactly representable as float64 + "title": "\u009b31mRed\u009b0m", + } + err := w.OK(data) + require.NoError(t, err) + + out := buf.String() + assert.Contains(t, out, "9007199254740993", + "large integer ID must survive the TTY round-trip with exact digits") + assert.NotContains(t, out, "9.007", "ID must not render in exponent/float form") + assert.NotContains(t, out, "9007199254740992", "ID must not lose precision") +} + +// TestWriterJSONVerbatimWhenPiped verifies that JSON written to a non-TTY sink +// (pipes, files, the default in tests) preserves every byte verbatim: piped +// JSON is the machine-consumption contract, and sanitizing would silently +// corrupt data fidelity for programmatic consumers. +func TestWriterJSONVerbatimWhenPiped(t *testing.T) { + var buf bytes.Buffer // bytes.Buffer is not a TTY + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + }) + + data := map[string]any{ + "title": "\u009b31mRed alert\u009b0m", // C1 CSI + "note": "De\x7fleted", // DEL + } + err := w.OK(data) + require.NoError(t, err) + + var resp struct { + Data map[string]any `json:"data"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &resp)) + assert.Equal(t, "\u009b31mRed alert\u009b0m", resp.Data["title"], + "C1 bytes must round-trip when piped") + assert.Equal(t, "De\x7fleted", resp.Data["note"], "DEL must round-trip when piped") +} + func TestWriterJQFilterSelect(t *testing.T) { var buf bytes.Buffer w := New(Options{ @@ -3648,3 +4156,328 @@ func TestPluralNoun(t *testing.T) { assert.Equal(t, tt.expected, PluralNoun(tt.input), "PluralNoun(%q)", tt.input) } } + +// hostileText exercises every terminal-injection payload class the renderers +// must neutralize: an ESC-based OSC title set, a UTF-8-encoded C1 CSI color +// pair (survives ansi.Strip), a bare CR (must separate words, not glue them), +// and DEL. +const hostileText = "Evil\x1b]0;pwned\x07 \u009b31mProject\u009b0m\rna\u007fme" + +// allEscapeText is composed entirely of ESC-based escape sequences and bare +// C0/DEL control characters — no printable runes survive sanitization, so +// sanitizeText collapses it to the empty string. Sinks must gate on the +// sanitized value so this emits no blank line, heading, or marker. +const allEscapeText = "\x1b[2J\x1b[0m\x1b]0;x\x07\x08\x7f" + +// TestWithSummaryNoticeVerbatim verifies that summary/notice values are +// stored verbatim — consistent with error/hint/data, the JSON machine +// contract carries original content and sanitization happens only at +// terminal sinks. +func TestWithSummaryNoticeVerbatim(t *testing.T) { + t.Run("WithSummary", func(t *testing.T) { + r := &Response{} + WithSummary(hostileText)(r) + assert.Equal(t, hostileText, r.Summary) + }) + + t.Run("WithNotice", func(t *testing.T) { + r := &Response{} + WithNotice(hostileText)(r) + assert.Equal(t, hostileText, r.Notice) + assert.False(t, r.noticeDiagnostic) + }) + + t.Run("WithDiagnostic", func(t *testing.T) { + r := &Response{} + WithDiagnostic(hostileText)(r) + assert.Equal(t, hostileText, r.Notice) + assert.True(t, r.noticeDiagnostic) + }) +} + +// TestJSONEnvelopeSummaryNoticeVerbatim verifies the machine contract +// end-to-end: the JSON envelope carries summary and notice byte-for-byte. +func TestJSONEnvelopeSummaryNoticeVerbatim(t *testing.T) { + var buf bytes.Buffer + w := New(Options{Format: FormatJSON, Writer: &buf}) + + err := w.OK(map[string]string{"id": "1"}, WithSummary(hostileText), WithNotice(hostileText)) + require.NoError(t, err) + + var decoded map[string]any + require.NoError(t, json.Unmarshal(buf.Bytes(), &decoded)) + assert.Equal(t, hostileText, decoded["summary"]) + assert.Equal(t, hostileText, decoded["notice"]) +} + +// TestQuietDiagnosticStderrSanitized verifies the quiet-mode stderr +// diagnostic — a terminal sink outside the renderers — strips escape +// sequences and keeps the notice to a single line. +func TestQuietDiagnosticStderrSanitized(t *testing.T) { + var stdout, stderr bytes.Buffer + w := New(Options{Format: FormatQuiet, Writer: &stdout, ErrWriter: &stderr}) + + err := w.OK(map[string]string{"id": "1"}, WithDiagnostic(hostileText)) + require.NoError(t, err) + + assert.Equal(t, "notice: Evil 31mProject0m name\n", stderr.String()) +} + +// TestQuietDiagnosticStderrEmptyAfterSanitize verifies that a diagnostic notice +// that collapses to "" after sanitization (all-escape content) emits nothing on +// stderr — no blank "notice: " line. +func TestQuietDiagnosticStderrEmptyAfterSanitize(t *testing.T) { + var stdout, stderr bytes.Buffer + w := New(Options{Format: FormatQuiet, Writer: &stdout, ErrWriter: &stderr}) + + require.NoError(t, w.OK(map[string]string{"id": "1"}, WithDiagnostic(allEscapeText))) + + assert.Empty(t, stderr.String(), "all-escape diagnostic must not emit a blank notice line") +} + +// TestSummaryNoticeEmptyAfterSanitizeSinks verifies that summary/notice values +// that collapse to "" after sanitization (all-escape content) leave the four +// single-line sinks — styled and markdown, generic and entity — byte-identical +// to rendering with no summary/notice at all. This pins the sanitize-first, +// gate-on-non-empty rule: no blank styled line, no empty "## " heading or +// "*...*" marker, and no trailing spacer newline. +func TestSummaryNoticeEmptyAfterSanitizeSinks(t *testing.T) { + render := func(format Format, entity bool, withSummaryNotice bool) string { + var buf bytes.Buffer + w := New(Options{Format: format, Writer: &buf}) + var opts []ResponseOption + if withSummaryNotice { + opts = append(opts, WithSummary(allEscapeText), WithNotice(allEscapeText)) + } + var data any = map[string]string{"id": "1"} + if entity { + data = map[string]any{ + "id": float64(100), + "subject": "Weekly update", + "content": "Body", + "creator": map[string]any{"name": "Bob"}, + "created_at": "2026-03-01T09:00:00Z", + } + opts = append(opts, WithEntity("message")) + } + require.NoError(t, w.OK(data, opts...)) + return buf.String() + } + + cases := []struct { + name string + format Format + entity bool + }{ + {"styled generic", FormatStyled, false}, + {"markdown generic", FormatMarkdown, false}, + {"styled entity", FormatStyled, true}, + {"markdown entity", FormatMarkdown, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + baseline := render(tc.format, tc.entity, false) + hostile := render(tc.format, tc.entity, true) + // Byte-identical output proves no empty styled line, "## " heading, + // "*...*" marker, or trailing spacer newline was emitted for the + // collapsed summary/notice. + assert.Equal(t, baseline, hostile, + "all-escape summary/notice must render identically to no summary/notice") + assertNoC1OrDEL(t, hostile, tc.name) + }) + } +} + +// assertNoC1OrDEL fails if any C1 control (U+0080-U+009F) or DEL leaked into +// output. C1 controls survive ansi.Strip but are executed directly by +// C1-honoring terminals. +func assertNoC1OrDEL(t *testing.T, out, sink string) { + t.Helper() + for _, r := range out { + if (r >= 0x80 && r <= 0x9f) || r == 0x7f { + t.Fatalf("terminal control %U leaked into %s output", r, sink) + } + } +} + +// assertErrorSinksSanitized renders err through the styled, markdown, and +// plain RenderError sinks and asserts no injection payload survives in any +// of them, while the neutralized text (wantFragment) still renders. +func assertErrorSinksSanitized(t *testing.T, err error, wantFragment string) { + t.Helper() + + var styledBuf bytes.Buffer + w := New(Options{Format: FormatStyled, Writer: &styledBuf}) + require.NoError(t, w.Err(err)) + styled := styledBuf.String() + + var mdBuf bytes.Buffer + w = New(Options{Format: FormatMarkdown, Writer: &mdBuf}) + require.NoError(t, w.Err(err)) + markdown := mdBuf.String() + + // The plain branch is unreachable through Writer formats (FormatStyled + // forces styling), so drive the renderer directly. + e := AsError(err) + var plainBuf bytes.Buffer + renderErr := NewRenderer(&plainBuf, false).RenderError(&plainBuf, &ErrorResponse{Error: e.Message, Hint: e.Hint}) + require.NoError(t, renderErr) + plain := plainBuf.String() + + // Styled output legitimately carries the renderer's own SGR codes, so + // assert on the hostile payload classes: no OSC introducer, no CR, no + // C1/DEL controls. + assert.Contains(t, styled, "\x1b[", "styled output should be styled") + assert.NotContains(t, styled, "\x1b]", "OSC leaked into styled output") + assert.NotContains(t, styled, "\r", "CR leaked into styled output") + assertNoC1OrDEL(t, styled, "styled") + assert.Contains(t, ansi.Strip(styled), wantFragment) + + for sink, out := range map[string]string{"markdown": markdown, "plain": plain} { + assert.NotContains(t, out, "\x1b", "ESC leaked into %s output", sink) + assert.NotContains(t, out, "\r", "CR leaked into %s output", sink) + assertNoC1OrDEL(t, out, sink) + assert.Contains(t, out, wantFragment, sink) + } +} + +// TestRenderErrorSanitizesAmbiguousMatches drives C1/CSI-laden project names +// through an ambiguous-match error, which interpolates them into the hint. +func TestRenderErrorSanitizesAmbiguousMatches(t *testing.T) { + err := ErrAmbiguous("project", []string{hostileText, "Second " + hostileText}) + assertErrorSinksSanitized(t, err, "31mProject0m") +} + +// TestRenderErrorSanitizesNotFoundHint drives hostile person names through a +// not-found error, which interpolates them into both message and hint. +func TestRenderErrorSanitizesNotFoundHint(t *testing.T) { + err := ErrNotFoundHint("Person", hostileText, "Use 'basecamp people' to find "+hostileText) + assertErrorSinksSanitized(t, err, "31mProject0m") +} + +// TestRenderErrorSanitizesSDKErrorMessage drives a hostile server-supplied +// message through an SDK error. +func TestRenderErrorSanitizesSDKErrorMessage(t *testing.T) { + err := &basecamp.Error{ + Code: basecamp.CodeAPI, + Message: "server error: " + hostileText, + HTTPStatus: 500, + } + assertErrorSinksSanitized(t, err, "31mProject0m") +} + +// assertSinkNeutralized asserts a rendered terminal sink neither leaks a +// terminal-control payload class nor glues CR-separated words, while the +// neutralized hostileText still renders on a single line. Styled output +// legitimately carries the renderer's own SGR codes, so it is checked for the +// hostile payload classes (OSC, CR, C1/DEL) rather than a blanket ESC ban. +func assertSinkNeutralized(t *testing.T, out, sink string, styled bool) { + t.Helper() + if styled { + assert.NotContains(t, out, "\x1b]", "OSC leaked into %s output", sink) + } else { + assert.NotContains(t, out, "\x1b", "ESC leaked into %s output", sink) + } + assert.NotContains(t, out, "\r", "CR leaked into %s output", sink) + assertNoC1OrDEL(t, out, sink) + // The bare CR in hostileText must separate "0m" and "name" with a space + // (not glue them) and must not break the value across lines. + assert.Contains(t, ansi.Strip(out), "31mProject0m name", + "%s: neutralized hostile text must render single-line and un-glued", sink) +} + +// TestSummaryNoticeSinksSanitized drives hostile summary/notice content through +// all four single-line summary/notice sinks — styled and markdown, generic and +// entity presenters — asserting escape/C1/CR payloads are neutralized and that +// a bare CR between words keeps them space-separated on a single line. +func TestSummaryNoticeSinksSanitized(t *testing.T) { + // Notice carries a pure CR-between-words payload to pin the no-glue rule. + const notice = "before\rafter" + + render := func(format Format, entity bool) string { + var buf bytes.Buffer + w := New(Options{Format: format, Writer: &buf}) + opts := []ResponseOption{WithSummary(hostileText), WithNotice(notice)} + var data any = map[string]string{"id": "1"} + if entity { + data = map[string]any{ + "id": float64(100), + "subject": "Weekly update", + "content": "Body", + "creator": map[string]any{"name": "Bob"}, + "created_at": "2026-03-01T09:00:00Z", + } + opts = append(opts, WithEntity("message")) + } + require.NoError(t, w.OK(data, opts...)) + return buf.String() + } + + cases := []struct { + name string + format Format + entity bool + styled bool + }{ + {"styled generic", FormatStyled, false, true}, + {"markdown generic", FormatMarkdown, false, false}, + {"styled entity", FormatStyled, true, true}, + {"markdown entity", FormatMarkdown, true, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + out := render(tc.format, tc.entity) + assertSinkNeutralized(t, out, tc.name, tc.styled) + + stripped := ansi.Strip(out) + assert.Contains(t, stripped, "before after", + "CR-separated notice words must stay space-separated") + assert.NotContains(t, stripped, "beforeafter", + "CR must not glue notice words together") + assert.NotContains(t, out, "before\nafter", + "single-line notice must not break across lines") + }) + } +} + +// TestBreadcrumbSinksSanitized drives hostile breadcrumb Cmd/Description (a +// previously uncovered sink) through the styled and markdown renderers. +func TestBreadcrumbSinksSanitized(t *testing.T) { + render := func(format Format) string { + var buf bytes.Buffer + w := New(Options{Format: format, Writer: &buf}) + require.NoError(t, w.OK( + map[string]string{"id": "1"}, + WithBreadcrumbs(Breadcrumb{Action: "act", Cmd: hostileText, Description: hostileText}), + )) + return buf.String() + } + + assertSinkNeutralized(t, render(FormatStyled), "styled breadcrumb", true) + assertSinkNeutralized(t, render(FormatMarkdown), "markdown breadcrumb", false) +} + +// TestAttachmentMetaSinksSanitized drives hostile attachment metadata +// (content_type/filesize — a previously uncovered sink) through the styled and +// markdown renderers. +func TestAttachmentMetaSinksSanitized(t *testing.T) { + render := func(format Format) string { + var buf bytes.Buffer + w := New(Options{Format: format, Writer: &buf}) + data := map[string]any{ + "id": float64(1), + "content_attachments": []any{ + map[string]any{ + "filename": "file.bin", + "content_type": hostileText, + "filesize": hostileText, + }, + }, + } + require.NoError(t, w.OK(data)) + return buf.String() + } + + assertSinkNeutralized(t, render(FormatStyled), "styled attachment meta", true) + assertSinkNeutralized(t, render(FormatMarkdown), "markdown attachment meta", false) +} diff --git a/internal/output/render.go b/internal/output/render.go index 4f903a839..69ec012d0 100644 --- a/internal/output/render.go +++ b/internal/output/render.go @@ -9,7 +9,6 @@ import ( "sort" "strings" "time" - "unicode" "charm.land/lipgloss/v2" "charm.land/lipgloss/v2/table" @@ -113,19 +112,27 @@ func terminalInfo(w io.Writer) (width int, isTTY bool) { func (r *Renderer) RenderResponse(w io.Writer, resp *Response) error { var b strings.Builder - // Summary line - if resp.Summary != "" { - b.WriteString(r.Summary.Render(resp.Summary)) + // Summary line. sanitizeText (single-line) guards against terminal + // injection from API-controlled summary/notice content and keeps the + // value on one line: CR is normalized to a separator so words don't glue, + // and embedded newlines/tabs are collapsed. Sanitization happens only here + // at the terminal sink — machine (JSON) output carries these verbatim. + // Sanitize first, then gate on the sanitized value: an all-escape summary + // collapses to "" and must emit no blank styled line or trailing spacer. + summary := sanitizeText(resp.Summary, true, false) + if summary != "" { + b.WriteString(r.Summary.Render(summary)) b.WriteString("\n") } // Notice (e.g., truncation warning) - if resp.Notice != "" { - b.WriteString(r.Hint.Render(resp.Notice)) + notice := sanitizeText(resp.Notice, true, false) + if notice != "" { + b.WriteString(r.Hint.Render(notice)) b.WriteString("\n") } - if resp.Summary != "" || resp.Notice != "" { + if summary != "" || notice != "" { b.WriteString("\n") } @@ -159,6 +166,13 @@ func (r *Renderer) RenderResponse(w io.Writer, resp *Response) error { func (r *Renderer) RenderError(w io.Writer, resp *ErrorResponse) error { var b strings.Builder + // Error and hint frequently interpolate API-controlled strings + // (project/person names, server messages), so sanitize them for the + // terminal before rendering. sanitizeText normalizes CR first so word + // boundaries survive SanitizeTerminal. + errMsg := sanitizeText(resp.Error, false, true) + hint := sanitizeText(resp.Hint, false, true) + if r.styled { // Create a styled error box with border errorIcon := "✗" @@ -169,7 +183,7 @@ func (r *Renderer) RenderError(w io.Writer, resp *ErrorResponse) error { // border (2) + padding (2) r.width-4, 40) - errorMsg := wrapText(resp.Error, maxWidth) + errorMsg := wrapText(errMsg, maxWidth) // Build content lines var contentLines []string @@ -179,9 +193,9 @@ func (r *Renderer) RenderError(w io.Writer, resp *ErrorResponse) error { contentLines = append(contentLines, r.Data.Render(line)) } - if resp.Hint != "" { + if hint != "" { contentLines = append(contentLines, "") - hintMsg := wrapText(resp.Hint, maxWidth) + hintMsg := wrapText(hint, maxWidth) for i, line := range strings.Split(hintMsg, "\n") { if i == 0 { contentLines = append(contentLines, r.Hint.Render("→ "+line)) @@ -208,11 +222,11 @@ func (r *Renderer) RenderError(w io.Writer, resp *ErrorResponse) error { b.WriteString("\n") } else { // Plain text output (no styling) - b.WriteString("Error: " + resp.Error) + b.WriteString("Error: " + errMsg) b.WriteString("\n") - if resp.Hint != "" { - b.WriteString("Hint: " + resp.Hint) + if hint != "" { + b.WriteString("Hint: " + hint) b.WriteString("\n") } if requestID := errorRequestID(resp); requestID != "" { @@ -258,18 +272,28 @@ func wrappedRequestIDLines(requestID string, width int) []string { } func sanitizeRequestID(requestID string) string { - requestID = ansi.Strip(requestID) - requestID = strings.Map(func(r rune) rune { - switch { - case r == '\n' || r == '\r' || r == '\t': - return ' ' - case unicode.IsControl(r): - return -1 - default: - return r - } - }, requestID) - return strings.Join(strings.Fields(requestID), " ") + return sanitizeText(requestID, true, false) +} + +// sanitizeText prepares an API-controlled string for a terminal sink. +// CR/CRLF are normalized to newlines first — SanitizeTerminal deletes bare +// CR, which would join adjacent words instead of separating them. When +// convertHTML is set, HTML is converted to markdown before sanitization +// because entity decoding (e.g. ›) can reintroduce C1 controls that +// were not present in the raw input; SanitizeTerminal therefore always runs +// last. When singleLine is set, remaining whitespace is collapsed to single +// spaces so the value occupies exactly one output line. +func sanitizeText(s string, singleLine, convertHTML bool) string { + s = strings.ReplaceAll(s, "\r\n", "\n") + s = strings.ReplaceAll(s, "\r", "\n") + if convertHTML && richtext.IsHTML(s) { + s = richtext.HTMLToMarkdown(s) + } + s = richtext.SanitizeTerminal(s) + if singleLine { + s = strings.Join(strings.Fields(s), " ") + } + return s } func escapeMarkdownText(s string) string { @@ -390,7 +414,7 @@ func (r *Renderer) renderData(b *strings.Builder, data any) { } case string: - b.WriteString(r.Data.Render(ansi.Strip(d))) + b.WriteString(r.Data.Render(richtext.SanitizeTerminal(d))) b.WriteString("\n") case nil: @@ -399,7 +423,7 @@ func (r *Renderer) renderData(b *strings.Builder, data any) { default: // Fallback: format as string - b.WriteString(r.Data.Render(ansi.Strip(fmt.Sprintf("%v", data)))) + b.WriteString(r.Data.Render(richtext.SanitizeTerminal(fmt.Sprintf("%v", data)))) b.WriteString("\n") } } @@ -766,7 +790,7 @@ func topLevelAttachmentSections(data map[string]any) []attachmentSection { func attachmentDisplayName(att map[string]any) string { for _, key := range []string{"filename", "caption", "path", "url", "sgid"} { if value, ok := att[key].(string); ok && value != "" { - return ansi.Strip(value) + return richtext.SanitizeTerminal(value) } } return "attachment" @@ -775,19 +799,19 @@ func attachmentDisplayName(att map[string]any) string { func attachmentDisplayMeta(att map[string]any) string { var parts []string if contentType, ok := att["content_type"].(string); ok && contentType != "" { - parts = append(parts, contentType) + parts = append(parts, sanitizeText(contentType, true, false)) } if filesize, ok := att["filesize"].(string); ok && filesize != "" { - parts = append(parts, filesize+" bytes") + parts = append(parts, sanitizeText(filesize, true, false)+" bytes") } if path, ok := att["path"].(string); ok && path != "" { - parts = append(parts, "saved to "+path) + parts = append(parts, "saved to "+sanitizeText(path, true, false)) } if status, ok := att["download_status"].(string); ok && status != "" { - parts = append(parts, status) + parts = append(parts, sanitizeText(status, true, false)) } if errText, ok := att["download_error"].(string); ok && errText != "" { - parts = append(parts, "error: "+errText) + parts = append(parts, "error: "+sanitizeText(errText, true, false)) } return strings.Join(parts, " · ") } @@ -795,11 +819,11 @@ func attachmentDisplayMeta(att map[string]any) string { func commentCreatorName(comment map[string]any) string { if creator, ok := comment["creator"].(map[string]any); ok { if name, ok := creator["name"].(string); ok && name != "" { - return ansi.Strip(name) + return richtext.SanitizeTerminal(name) } } if name, ok := comment["creator_name"].(string); ok && name != "" { - return ansi.Strip(name) + return richtext.SanitizeTerminal(name) } return "Unknown" } @@ -810,13 +834,7 @@ func commentTimestamp(comment map[string]any) string { func commentBody(comment map[string]any) string { content, _ := comment["content"].(string) - content = ansi.Strip(content) - if richtext.IsHTML(content) { - content = richtext.HTMLToMarkdown(content) - } - content = strings.ReplaceAll(content, "\r\n", "\n") - content = strings.ReplaceAll(content, "\r", "\n") - return strings.TrimSpace(content) + return strings.TrimSpace(sanitizeText(content, false, true)) } func (r *Renderer) renderCommentsSection(b *strings.Builder, comments []map[string]any) { @@ -962,9 +980,9 @@ func (r *Renderer) renderBreadcrumbs(b *strings.Builder, crumbs []Breadcrumb) { b.WriteString(r.Muted.Render("Hints:")) b.WriteString("\n") for _, bc := range crumbs { - line := r.Data.Render(" " + bc.Cmd) + line := r.Data.Render(" " + sanitizeText(bc.Cmd, true, false)) if bc.Description != "" { - line += r.Subtle.Render(" # " + bc.Description) + line += r.Subtle.Render(" # " + sanitizeText(bc.Description, true, false)) } b.WriteString(line + "\n") } @@ -981,6 +999,11 @@ func (r *Renderer) renderStats(b *strings.Builder, stats map[string]any) { } func formatHeader(key string) string { + // Keys can be API-controlled response/field keys (see detectColumns and + // renderObject callers), so strip terminal escapes before they reach a + // styled or markdown header/label. sanitizeText (single-line) mirrors the + // treatment of cell values; literal, clean keys are unchanged. + key = sanitizeText(key, true, false) key = strings.ReplaceAll(key, "_", " ") key = strings.TrimSuffix(key, " on") key = strings.TrimSuffix(key, " at") @@ -999,10 +1022,7 @@ func formatCell(val any) string { case nil: return "" case string: - v = ansi.Strip(v) - if richtext.IsHTML(v) { - v = richtext.HTMLToMarkdown(v) - } + v = sanitizeText(v, false, true) if strings.ContainsAny(v, "\n\r") { v = strings.Join(strings.Fields(v), " ") } @@ -1035,7 +1055,7 @@ func formatCell(val any) string { for _, item := range v { switch elem := item.(type) { case string: - s := ansi.Strip(elem) + s := richtext.SanitizeTerminal(elem) if strings.ContainsAny(s, "\n\r") { s = strings.Join(strings.Fields(s), " ") } @@ -1056,27 +1076,34 @@ func formatCell(val any) string { // from the calendar/reports API response and use summary as their // display name (Schedule::Entry#title delegates to summary in bc3). if filename, ok := elem["filename"].(string); ok && filename != "" { - items = append(items, ansi.Strip(filename)) + items = append(items, richtext.SanitizeTerminal(filename)) } else if caption, ok := elem["caption"].(string); ok && caption != "" { - items = append(items, ansi.Strip(caption)) + items = append(items, richtext.SanitizeTerminal(caption)) } else if name, ok := elem["name"].(string); ok && name != "" { - items = append(items, ansi.Strip(name)) + items = append(items, richtext.SanitizeTerminal(name)) } else if title, ok := elem["title"].(string); ok && title != "" { - items = append(items, ansi.Strip(title)) + items = append(items, richtext.SanitizeTerminal(title)) } else if summary, ok := elem["summary"].(string); ok && summary != "" { - items = append(items, ansi.Strip(summary)) + items = append(items, richtext.SanitizeTerminal(summary)) } else if path, ok := elem["path"].(string); ok && path != "" { - items = append(items, ansi.Strip(path)) + items = append(items, richtext.SanitizeTerminal(path)) } else if id, ok := elem["id"]; ok { - items = append(items, fmt.Sprintf("%v", id)) + // String ids are API-controlled and reach a terminal sink, + // so sanitize them like the sibling arms. Non-string ids + // (e.g. json.Number) carry no escape bytes and are safe. + if s, ok := id.(string); ok { + items = append(items, richtext.SanitizeTerminal(s)) + } else { + items = append(items, fmt.Sprintf("%v", id)) + } } default: - items = append(items, ansi.Strip(fmt.Sprintf("%v", item))) + items = append(items, richtext.SanitizeTerminal(fmt.Sprintf("%v", item))) } } return strings.Join(items, ", ") default: - return ansi.Strip(fmt.Sprintf("%v", v)) + return richtext.SanitizeTerminal(fmt.Sprintf("%v", v)) } } @@ -1103,10 +1130,7 @@ func formatDetailValue(key string, val any) string { case nil: return "" case string: - v = ansi.Strip(v) - if richtext.IsHTML(v) { - v = richtext.HTMLToMarkdown(v) - } + v = sanitizeText(v, false, true) if strings.ContainsAny(v, "\n\r") { v = strings.Join(strings.Fields(v), " ") } @@ -1195,17 +1219,24 @@ func NewMarkdownRenderer(w io.Writer) *MarkdownRenderer { func (r *MarkdownRenderer) RenderResponse(w io.Writer, resp *Response) error { var b strings.Builder - // Summary as heading - if resp.Summary != "" { - b.WriteString("## " + resp.Summary + "\n") + // Summary as heading. sanitizeText (single-line) guards against terminal + // injection and keeps the heading/notice on one line. Sanitization happens + // only at terminal sinks like this one — machine (JSON) output carries + // summary/notice verbatim. + // Sanitize first, then gate: an all-escape summary/notice collapses to "" + // and must not emit an empty "## " heading or "*...*" line. + summary := sanitizeText(resp.Summary, true, false) + if summary != "" { + b.WriteString("## " + summary + "\n") } // Notice (e.g., truncation warning) - if resp.Notice != "" { - b.WriteString("*" + resp.Notice + "*\n") + notice := sanitizeText(resp.Notice, true, false) + if notice != "" { + b.WriteString("*" + notice + "*\n") } - if resp.Summary != "" || resp.Notice != "" { + if summary != "" || notice != "" { b.WriteString("\n") } @@ -1217,9 +1248,9 @@ func (r *MarkdownRenderer) RenderResponse(w io.Writer, resp *Response) error { if len(resp.Breadcrumbs) > 0 { b.WriteString("\n### Hints\n\n") for _, bc := range resp.Breadcrumbs { - line := "- `" + bc.Cmd + "`" + line := "- `" + sanitizeText(bc.Cmd, true, false) + "`" if bc.Description != "" { - line += " — " + bc.Description + line += " — " + sanitizeText(bc.Description, true, false) } b.WriteString(line + "\n") } @@ -1239,9 +1270,11 @@ func (r *MarkdownRenderer) RenderResponse(w io.Writer, resp *Response) error { func (r *MarkdownRenderer) RenderError(w io.Writer, resp *ErrorResponse) error { var b strings.Builder - b.WriteString("**Error:** " + resp.Error + "\n") - if resp.Hint != "" { - b.WriteString("\n*Hint: " + resp.Hint + "*\n") + // Error and hint interpolate API-controlled strings; sanitize for the + // terminal (see (*Renderer).RenderError). + b.WriteString("**Error:** " + sanitizeText(resp.Error, false, true) + "\n") + if hint := sanitizeText(resp.Hint, false, true); hint != "" { + b.WriteString("\n*Hint: " + hint + "*\n") } if requestID := sanitizeRequestID(errorRequestID(resp)); requestID != "" { b.WriteString("\nRequest ID: " + escapeMarkdownText(requestID) + "\n") @@ -1275,13 +1308,13 @@ func (r *MarkdownRenderer) renderData(b *strings.Builder, data any) { } case string: - b.WriteString(ansi.Strip(d) + "\n") + b.WriteString(richtext.SanitizeTerminal(d) + "\n") case nil: b.WriteString("*No data*\n") default: - fmt.Fprintf(b, "%v\n", ansi.Strip(fmt.Sprintf("%v", data))) + fmt.Fprintf(b, "%v\n", richtext.SanitizeTerminal(fmt.Sprintf("%v", data))) } } diff --git a/internal/presenter/format.go b/internal/presenter/format.go index 55dc92822..2126eae6b 100644 --- a/internal/presenter/format.go +++ b/internal/presenter/format.go @@ -52,6 +52,11 @@ func formatDate(val any, locale Locale) string { if !ok || str == "" { return "" } + // Strip terminal escape sequences up front so a timestamp with embedded + // escapes still parses, and the fallback below is already sanitized. A date + // value renders in a single-line detail field, so collapse embedded + // CR/newlines/tabs too — a malformed value can't break the row. + str = richtext.SanitizeSingleLine(str) // Try ISO8601 full timestamp if t, err := time.Parse(time.RFC3339, str); err == nil { @@ -71,6 +76,11 @@ func formatRelativeTime(val any, locale Locale) string { if !ok || str == "" { return "" } + // Strip terminal escape sequences up front so a timestamp with embedded + // escapes still parses, and the fallback below is already sanitized. A + // relative-time value renders in a single-line detail field, so collapse + // embedded CR/newlines/tabs too — a malformed value can't break the row. + str = richtext.SanitizeSingleLine(str) t, err := time.Parse(time.RFC3339, str) if err != nil { @@ -129,7 +139,12 @@ func formatPeople(val any) string { for _, item := range arr { if m, ok := item.(map[string]any); ok { if name, ok := m["name"].(string); ok { - names = append(names, name) + // Names render in a single-line detail row (e.g. todo.assignees), + // so collapse embedded CR/newlines/tabs to spaces rather than + // letting a crafted name break the row or glue words together. + if stripped := richtext.SanitizeSingleLine(name); stripped != "" { + names = append(names, stripped) + } } } } @@ -168,7 +183,11 @@ func parseDockItems(val any) []dockItem { result := make([]dockItem, len(items)) for i, m := range items { title, _ := m["title"].(string) + // A dock title/name occupies one line of the dock listing, so collapse + // embedded CR/newlines/tabs to spaces to keep each row intact. + title = richtext.SanitizeSingleLine(title) name, _ := m["name"].(string) + name = richtext.SanitizeSingleLine(name) if title == "" { title = name } @@ -263,7 +282,9 @@ func dockPosition(m map[string]any) int { func formatPerson(val any) string { if m, ok := val.(map[string]any); ok { if name, ok := m["name"].(string); ok { - return name + // A person name is a single key/value detail row value, so collapse + // embedded CR/newlines/tabs to spaces to keep the row on one line. + return richtext.SanitizeSingleLine(name) } } return "" @@ -295,8 +316,14 @@ func formatText(val any) string { case nil: return "" case string: + // Strip terminal escape sequences from API-controlled strings before + // they reach a styled/markdown sink (terminal injection defense). + v = richtext.SanitizeTerminal(v) if richtext.IsHTML(v) { - return richtext.HTMLToMarkdown(v) + // Defense-in-depth: strip the generated markdown too before it can + // reach a styled/markdown sink. The input is already stripped above + // and HTMLToMarkdown never emits ESC, so this is belt-and-suspenders. + return richtext.SanitizeTerminal(richtext.HTMLToMarkdown(v)) } return v case bool: diff --git a/internal/presenter/format_test.go b/internal/presenter/format_test.go index 416bb55ac..71a6a8ad9 100644 --- a/internal/presenter/format_test.go +++ b/internal/presenter/format_test.go @@ -2,7 +2,9 @@ package presenter import ( "encoding/json" + "strings" "testing" + "time" ) func TestFormatDock(t *testing.T) { @@ -133,3 +135,232 @@ func TestFormatDockTitleFallsBackToName(t *testing.T) { t.Errorf("formatDock(title fallback) = %q, want %q", got, want) } } + +func TestFormatDockStripsANSIFromTitleAndName(t *testing.T) { + dock := []any{ + map[string]any{ + "title": "\x1b]8;;http://evil.example\x07To-dos\x1b]8;;\x07", + "name": "\x1b[31mtodoset\x1b[0m", + "enabled": true, + "id": float64(1), + }, + } + + got := formatDock(dock) + want := "1 To-dos (todoset)" + if got != want { + t.Errorf("formatDock(ANSI in title/name) = %q, want %q", got, want) + } +} + +func TestFormatDockTitleFallbackStripsANSIFromName(t *testing.T) { + dock := []any{ + map[string]any{ + "name": "\x1b[31mtodoset\x1b[0m", + "enabled": true, + "id": float64(1), + }, + } + + got := formatDock(dock) + want := "1 todoset" + if got != want { + t.Errorf("formatDock(ANSI in name fallback) = %q, want %q", got, want) + } +} + +func TestFormatPeopleStripsANSI(t *testing.T) { + people := []any{ + map[string]any{"name": "\x1b[31mAlice\x1b[0m"}, + map[string]any{"name": "Bob"}, + } + + got := formatPeople(people) + want := "Alice, Bob" + if got != want { + t.Errorf("formatPeople(ANSI names) = %q, want %q", got, want) + } +} + +// TestFormatPeopleStripsC1Controls covers UTF-8-encoded Unicode C1 controls +// (U+0080-U+009F) and DEL, which survive ansi.Strip but are executed by +// C1-honoring terminals. +func TestFormatPeopleStripsC1Controls(t *testing.T) { + people := []any{ + map[string]any{"name": "\u009b31mAlice\u009b0m"}, // U+009B CSI + map[string]any{"name": "\u009d0;evil\aBob"}, // U+009D OSC, BEL-terminated + map[string]any{"name": "Ca\u007frol"}, // DEL + } + + got := formatPeople(people) + want := "31mAlice0m, 0;evilBob, Carol" + if got != want { + t.Errorf("formatPeople(C1 names) = %q, want %q", got, want) + } +} + +func TestFormatDateStripsANSIFromUnparseableInput(t *testing.T) { + locale := NewLocale("") + for _, in := range []string{"not-a-date\x1b[31mX", "\x1b]0;pwn\x07bad"} { + got := formatDate(in, locale) + if strings.ContainsRune(got, 0x1b) { + t.Errorf("formatDate(%q) = %q, want no escape byte", in, got) + } + } +} + +// TestFormatDateStripsC1ControlsFromUnparseableInput covers UTF-8-encoded C1 +// controls and DEL, which survive ansi.Strip but execute on C1-honoring +// terminals. +func TestFormatDateStripsC1ControlsFromUnparseableInput(t *testing.T) { + locale := NewLocale("") + for _, in := range []string{"not-a-date\u009b31mX", "\u009d0;pwn\abad", "not-a-date\u007fX"} { + got := formatDate(in, locale) + for _, r := range got { + if r < 0x20 && r != '\n' && r != '\t' || r == 0x7f || (r >= 0x80 && r <= 0x9f) { + t.Errorf("formatDate(%q) = %q, want no control rune U+%04X", in, got, r) + } + } + } +} + +func TestFormatRelativeTimeStripsANSIFromUnparseableInput(t *testing.T) { + locale := NewLocale("") + for _, in := range []string{"not-a-date\x1b[31mX", "\x1b]0;pwn\x07bad"} { + got := formatRelativeTime(in, locale) + if strings.ContainsRune(got, 0x1b) { + t.Errorf("formatRelativeTime(%q) = %q, want no escape byte", in, got) + } + } +} + +func TestFormatDateParsesTimestampWithEmbeddedANSI(t *testing.T) { + locale := NewLocale("") + want := formatDate("2026-07-01T12:00:00Z", locale) + for _, in := range []string{ + "\x1b[31m2026-07-01T12:00:00Z\x1b[0m", + "2026-07-01T\x1b[31m12:00:00Z", + } { + got := formatDate(in, locale) + if got != want { + t.Errorf("formatDate(%q) = %q, want formatted date %q", in, got, want) + } + } + + wantDateOnly := formatDate("2026-07-01", locale) + in := "\x1b[31m2026-07-01\x1b[0m" + if got := formatDate(in, locale); got != wantDateOnly { + t.Errorf("formatDate(%q) = %q, want formatted date %q", in, got, wantDateOnly) + } +} + +func TestFormatRelativeTimeParsesTimestampWithEmbeddedANSI(t *testing.T) { + locale := NewLocale("") + clean := time.Now().Add(-2 * time.Hour).Format(time.RFC3339) + want := formatRelativeTime(clean, locale) + for _, in := range []string{ + "\x1b[31m" + clean + "\x1b[0m", + clean[:10] + "\x1b[31m" + clean[10:], + } { + got := formatRelativeTime(in, locale) + if got != want { + t.Errorf("formatRelativeTime(%q) = %q, want relative time %q", in, got, want) + } + } +} + +func TestFormatPeopleDropsAllEscapeName(t *testing.T) { + people := []any{ + map[string]any{"name": "Alice"}, + map[string]any{"name": "\x1b[0m\x1b]8;;\x07"}, + } + + got := formatPeople(people) + want := "Alice" + if got != want { + t.Errorf("formatPeople(all-escape name dropped) = %q, want %q", got, want) + } +} + +// assertSingleLine fails if s contains a newline, carriage return, or tab — +// i.e. if a single-line sink leaked whitespace that could break its row. +func assertSingleLine(t *testing.T, label, s string) { + t.Helper() + if strings.ContainsAny(s, "\n\r\t") { + t.Errorf("%s = %q, want single line (no \\n/\\r/\\t)", label, s) + } +} + +// formatPeople feeds a single-line detail row (e.g. todo.assignees), so a name +// with a bare CR plus embedded newline/tab must collapse to one line with the +// CR acting as a word separator rather than gluing words together. +func TestFormatPeopleCollapsesWhitespace(t *testing.T) { + people := []any{ + map[string]any{"name": "Ann\rBob"}, + map[string]any{"name": "Carol\nDave\tErin"}, + } + + got := formatPeople(people) + want := "Ann Bob, Carol Dave Erin" + if got != want { + t.Errorf("formatPeople(embedded whitespace) = %q, want %q", got, want) + } + assertSingleLine(t, "formatPeople", got) +} + +// formatPerson is a single key/value detail row value, so embedded +// CR/newlines/tabs collapse to spaces. +func TestFormatPersonCollapsesWhitespace(t *testing.T) { + got := formatPerson(map[string]any{"name": "Ann\rBob\nCarol\tDave"}) + want := "Ann Bob Carol Dave" + if got != want { + t.Errorf("formatPerson(embedded whitespace) = %q, want %q", got, want) + } + assertSingleLine(t, "formatPerson", got) +} + +// A dock title/name occupies one line of the dock listing, so embedded +// CR/newlines/tabs must collapse rather than break the row layout. +func TestFormatDockCollapsesWhitespaceInTitleAndName(t *testing.T) { + dock := []any{ + map[string]any{ + "title": "To\rdos\nList", + "name": "todo\tset", + "enabled": true, + "id": float64(1), + }, + } + + got := formatDock(dock) + want := "1 To dos List (todo set)" + if got != want { + t.Errorf("formatDock(embedded whitespace) = %q, want %q", got, want) + } + assertSingleLine(t, "formatDock", got) +} + +// A date value renders in a single-line detail field; an unparseable value with +// embedded CR/newlines/tabs falls back to the sanitized string, which must stay +// on one line. +func TestFormatDateCollapsesWhitespaceInUnparseableInput(t *testing.T) { + locale := NewLocale("") + got := formatDate("not-a-date\rmore\nstuff\there", locale) + want := "not-a-date more stuff here" + if got != want { + t.Errorf("formatDate(embedded whitespace) = %q, want %q", got, want) + } + assertSingleLine(t, "formatDate", got) +} + +// A relative-time value renders in a single-line detail field; an unparseable +// value with embedded CR/newlines/tabs falls back to the sanitized string, +// which must stay on one line. +func TestFormatRelativeTimeCollapsesWhitespaceInUnparseableInput(t *testing.T) { + locale := NewLocale("") + got := formatRelativeTime("not-a-date\rmore\nstuff\there", locale) + want := "not-a-date more stuff here" + if got != want { + t.Errorf("formatRelativeTime(embedded whitespace) = %q, want %q", got, want) + } + assertSingleLine(t, "formatRelativeTime", got) +} diff --git a/internal/presenter/presenter_test.go b/internal/presenter/presenter_test.go index 8b256f7ef..7396297d2 100644 --- a/internal/presenter/presenter_test.go +++ b/internal/presenter/presenter_test.go @@ -324,6 +324,25 @@ func TestRenderTemplateLargeID(t *testing.T) { } } +func TestRenderTemplateSanitizesSingleLine(t *testing.T) { + // RenderTemplate feeds single-line sinks (headlines, affordance commands). + // An API value with a bare CR between words, embedded \n/\t, and a C1/ESC + // sequence must collapse to a single space-separated line with no control + // or escape bytes — a bare CR must NOT glue words together. + data := map[string]any{"content": "alpha\rbeta\ngamma\tdelta\x1b[31m\x9b0m"} + + got := RenderTemplate("{{.content}}", data) + + for _, r := range []rune{'\n', '\r', '\t', '\x1b', '\x9b'} { + if strings.ContainsRune(got, r) { + t.Errorf("RenderTemplate output %q still contains control/escape byte %q", got, r) + } + } + if got != "alpha beta gamma delta" { + t.Errorf("RenderTemplate = %q, want words space-separated (not glued): %q", got, "alpha beta gamma delta") + } +} + func TestEvalCondition(t *testing.T) { data := map[string]any{"completed": false} @@ -1617,6 +1636,217 @@ func TestRenderTaskItemCommaInAssigneeName(t *testing.T) { } } +func TestExtractPeopleNamesStripsANSI(t *testing.T) { + // Person names come from the API and must not carry ESC/OSC sequences + // into terminal output via the markdown task-list path. + val := []any{ + map[string]any{"name": "\x1b]0;pwn\x07Bob"}, + map[string]any{"name": "\x1b[31mAlice"}, + } + names := extractPeopleNames(val) + if len(names) != 2 { + t.Fatalf("Expected 2 names, got %d: %v", len(names), names) + } + if names[0] != "Bob" { + t.Errorf("names[0] = %q, want %q", names[0], "Bob") + } + if names[1] != "Alice" { + t.Errorf("names[1] = %q, want %q", names[1], "Alice") + } + for _, n := range names { + if strings.ContainsRune(n, '\x1b') { + t.Errorf("name %q still contains an escape sequence", n) + } + } +} + +func TestExtractPeopleNamesCollapsesToSingleLine(t *testing.T) { + // A person name carrying a bare CR between words plus embedded \n/\t and a + // C1/ESC sequence must collapse to one space-separated line — the bare CR + // must NOT glue words together, and no control/escape bytes may survive. + val := []any{ + map[string]any{"name": "Ada\rLovelace\nGrace\tHopper\x1b[31m\x9b"}, + } + names := extractPeopleNames(val) + if len(names) != 1 { + t.Fatalf("Expected 1 name, got %d: %v", len(names), names) + } + if names[0] != "Ada Lovelace Grace Hopper" { + t.Errorf("names[0] = %q, want words space-separated (not glued): %q", names[0], "Ada Lovelace Grace Hopper") + } + for _, r := range []rune{'\n', '\r', '\t', '\x1b', '\x9b'} { + if strings.ContainsRune(names[0], r) { + t.Errorf("name %q still contains control/escape byte %q", names[0], r) + } + } +} + +func TestRenderTaskItemStripsANSIInAssigneeName(t *testing.T) { + schema := LookupByName("todo") + if schema == nil { + t.Fatal("Expected todo schema") + } + + data := []map[string]any{ + { + "content": "Review PR", + "completed": false, + "due_on": "", + "assignees": []any{map[string]any{"name": "\x1b]0;pwn\x07Bob"}}, + }, + } + + var buf strings.Builder + if err := RenderListMarkdown(&buf, schema, data, enUS, ""); err != nil { + t.Fatalf("RenderListMarkdown failed: %v", err) + } + + out := buf.String() + if strings.ContainsRune(out, '\x1b') { + t.Errorf("Markdown task-list output should not contain escape sequences, got:\n%q", out) + } + if !strings.Contains(out, "@Bob") { + t.Errorf("Should render stripped assignee name '@Bob', got:\n%s", out) + } +} + +func TestRenderListMarkdownStripsANSIInGroupHeading(t *testing.T) { + // Group headings come from a raw API string (bucket.name) and must not + // carry ESC/OSC sequences into the markdown task-list output. + schema := LookupByName("todo") + if schema == nil { + t.Fatal("Expected todo schema") + } + + data := []map[string]any{ + { + "content": "Fix bug", "completed": false, "due_on": "", "assignees": []any{}, + "bucket": map[string]any{"name": "\x1b]0;pwn\x07Project Alpha"}, + }, + { + "content": "Write docs", "completed": true, "due_on": "", "assignees": []any{}, + "bucket": map[string]any{"name": "\x1b[31mProject Beta"}, + }, + } + + var buf strings.Builder + if err := RenderListMarkdown(&buf, schema, data, enUS, ""); err != nil { + t.Fatalf("RenderListMarkdown failed: %v", err) + } + + out := buf.String() + if strings.ContainsRune(out, '\x1b') { + t.Errorf("Markdown group heading should not contain escape sequences, got:\n%q", out) + } + if !strings.Contains(out, "## Project Alpha") { + t.Errorf("Should render stripped heading '## Project Alpha', got:\n%s", out) + } + if !strings.Contains(out, "## Project Beta") { + t.Errorf("Should render stripped heading '## Project Beta', got:\n%s", out) + } +} + +func TestRenderListMarkdownGroupHeadingAllEscapeFallsBackToOther(t *testing.T) { + // A group name that is entirely ESC/OSC sequences strips to "". The + // empty-check must run AFTER richtext.SanitizeTerminal so it falls back + // to "Other" instead of emitting a blank "## " heading. + schema := LookupByName("todo") + if schema == nil { + t.Fatal("Expected todo schema") + } + + data := []map[string]any{ + { + "content": "Fix bug", "completed": false, "due_on": "", "assignees": []any{}, + "bucket": map[string]any{"name": "\x1b]0;pwn\x07\x1b[31m\x1b[2J"}, + }, + { + "content": "Write docs", "completed": true, "due_on": "", "assignees": []any{}, + "bucket": map[string]any{"name": "Real Project"}, + }, + } + + var buf strings.Builder + if err := RenderListMarkdown(&buf, schema, data, enUS, "bucket.name"); err != nil { + t.Fatalf("RenderListMarkdown failed: %v", err) + } + + out := buf.String() + if strings.ContainsRune(out, '\x1b') { + t.Errorf("Markdown group heading should not contain escape sequences, got:\n%q", out) + } + if !strings.Contains(out, "## Other") { + t.Errorf("All-escape group name should render under '## Other', got:\n%s", out) + } + if strings.Contains(out, "## \n") { + t.Errorf("Should not emit a blank '## ' heading, got:\n%q", out) + } +} + +func TestRenderListMarkdownGroupHeadingCollapsesToSingleLine(t *testing.T) { + // A group name (bucket.name) with a bare CR between words plus embedded + // \n/\t and a C1/ESC sequence must collapse to a single "## " heading on + // one line: the bare CR must not glue words, embedded newlines must not + // spill the heading across lines, and no control/escape bytes may survive. + schema := LookupByName("todo") + if schema == nil { + t.Fatal("Expected todo schema") + } + + // Two buckets so headings are not suppressed (single-group runs hide them). + data := []map[string]any{ + { + "content": "Fix bug", "completed": false, "due_on": "", "assignees": []any{}, + "bucket": map[string]any{"name": "Project\rAlpha\nTeam\tOne\x1b[31m\x9b0m"}, + }, + { + "content": "Write docs", "completed": true, "due_on": "", "assignees": []any{}, + "bucket": map[string]any{"name": "Second Bucket"}, + }, + } + + var buf strings.Builder + if err := RenderListMarkdown(&buf, schema, data, enUS, "bucket.name"); err != nil { + t.Fatalf("RenderListMarkdown failed: %v", err) + } + + out := buf.String() + for _, r := range []rune{'\r', '\t', '\x1b', '\x9b'} { + if strings.ContainsRune(out, r) { + t.Errorf("Markdown group heading should not contain control/escape byte %q, got:\n%q", r, out) + } + } + if !strings.Contains(out, "## Project Alpha Team One\n") { + t.Errorf("Should render collapsed single-line heading '## Project Alpha Team One', got:\n%q", out) + } +} + +func TestRenderListMarkdownChokepointStripsANSI(t *testing.T) { + // Defense-in-depth: an escape injected into any API field on the literal + // markdown path must be absent from the final rendered markdown. + schema := LookupByName("todo") + if schema == nil { + t.Fatal("Expected todo schema") + } + + data := []map[string]any{ + { + "content": "Innocuous \x1b]8;;http://evil\x07title", "completed": false, "due_on": "", + "assignees": []any{map[string]any{"name": "\x1b[31mMallory"}}, + "bucket": map[string]any{"name": "\x1b[2JGroup"}, + }, + } + + var buf strings.Builder + if err := RenderListMarkdown(&buf, schema, data, enUS, "bucket.name"); err != nil { + t.Fatalf("RenderListMarkdown failed: %v", err) + } + + if strings.ContainsRune(buf.String(), '\x1b') { + t.Errorf("Final markdown must contain no escape bytes, got:\n%q", buf.String()) + } +} + // ============================================================================= // HTML → Markdown Conversion Tests // ============================================================================= @@ -1731,6 +1961,83 @@ func TestRenderHeadlineHTMLPreservesLiteralAsterisks(t *testing.T) { } } +func TestRenderHeadlineHTMLStripsEscape(t *testing.T) { + schema := &EntitySchema{ + Identity: Identity{Label: "content"}, + } + // An ESC embedded in the HTML content must not survive into the rendered + // headline: it is stripped before reaching the styled (Primary.Render) sink. + data := map[string]any{ + "content": "

danger\x1b[31mred\x1b[0m

", + } + got := RenderHeadline(schema, data) + if strings.Contains(got, "\x1b") { + t.Errorf("RenderHeadline must strip ESC from HTML content, got: %q", got) + } +} + +// The identity-label fallback in renderHeadlineRaw formats a raw data value +// without going through RenderTemplate, so a plain (non-HTML) value with a bare +// CR plus embedded newline/tab must collapse to a single-line headline rather +// than break across lines or glue words together. +func TestRenderHeadlineCollapsesWhitespace(t *testing.T) { + schema := &EntitySchema{ + Identity: Identity{Label: "content"}, + } + data := map[string]any{ + "content": "Line one\rLine two\ttabbed\nlast", + } + got := RenderHeadline(schema, data) + want := "Line one Line two tabbed last" + if got != want { + t.Errorf("RenderHeadline(embedded whitespace) = %q, want %q", got, want) + } + if strings.ContainsAny(got, "\n\r\t") { + t.Errorf("RenderHeadline = %q, want single line (no \\n/\\r/\\t)", got) + } +} + +// Guard: renderTaskListMarkdown emits a genuinely multi-line document (one line +// per task item). It is intentionally left on SanitizeTerminal — a blanket +// conversion to SanitizeSingleLine would collapse the whole list to one line. +// This test fails loudly if that ever happens. +func TestRenderTaskListMarkdownPreservesNewlines(t *testing.T) { + schema := LookupByName("todo") + if schema == nil { + t.Fatal("Expected todo schema") + } + + data := []map[string]any{ + {"content": "Fix bug", "completed": false, "due_on": "", "assignees": []any{}}, + {"content": "Write tests", "completed": true, "due_on": "", "assignees": []any{}}, + } + + var buf strings.Builder + if err := RenderListMarkdown(&buf, schema, data, enUS, ""); err != nil { + t.Fatalf("RenderListMarkdown failed: %v", err) + } + out := buf.String() + + if !strings.Contains(out, "- [ ] Fix bug\n") { + t.Errorf("task list should keep the first item on its own line, got:\n%q", out) + } + if !strings.Contains(out, "- [x] Write tests\n") { + t.Errorf("task list should keep the second item on its own line, got:\n%q", out) + } + if strings.Count(out, "\n") < 2 { + t.Errorf("multi-line task list collapsed to a single line, got:\n%q", out) + } +} + +func TestFormatTextHTMLStripsEscape(t *testing.T) { + // An ESC embedded in HTML content must not survive HTMLToMarkdown into the + // returned string, which can reach a styled/markdown sink. + got := formatText("

danger\x1b[31mred\x1b[0m

") + if strings.Contains(got, "\x1b") { + t.Errorf("formatText must strip ESC from HTML content, got: %q", got) + } +} + func TestRenderTaskItemHTMLContent(t *testing.T) { schema := LookupByName("todo") if schema == nil { diff --git a/internal/presenter/render.go b/internal/presenter/render.go index 377ab7cf4..e0bab283d 100644 --- a/internal/presenter/render.go +++ b/internal/presenter/render.go @@ -335,6 +335,8 @@ func renderAffordances(b *strings.Builder, schema *EntitySchema, data map[string b.WriteString("\n") // Find max command width for alignment + // RenderTemplate strips terminal escapes from the interpolated, API-controlled + // command (centralized in template.go). maxCmd := 0 renderedCmds := make([]string, len(visible)) for i, a := range visible { @@ -548,7 +550,12 @@ func renderTaskListMarkdown(w io.Writer, schema *EntitySchema, data []map[string if i > 0 { b.WriteString("\n") } - heading := g.name + // Group headings come from a raw API string (e.g. bucket.name via + // extractDotPath) that never passes through formatText. Sanitize for + // a single-line sink before the empty-check so an all-escape-sequence + // name falls back to "Other" instead of emitting a blank "## ", and + // so an embedded CR/newline/tab can't break the heading across lines. + heading := richtext.SanitizeSingleLine(g.name) if heading == "" { heading = "Other" } @@ -559,7 +566,11 @@ func renderTaskListMarkdown(w io.Writer, schema *EntitySchema, data []map[string } } - _, err := io.WriteString(w, b.String()) + // Defense-in-depth: this is the literal-markdown chokepoint. Output is plain + // generated markdown that never legitimately carries ANSI escape bytes + // (styling lives on the separate lipgloss path), so a final strip closes any + // future API-controlled sink without harming legitimate content. + _, err := io.WriteString(w, richtext.SanitizeTerminal(b.String())) return err } @@ -645,7 +656,9 @@ func extractPeopleNames(val any) []string { for _, item := range arr { if m, ok := item.(map[string]any); ok { if name, ok := m["name"].(string); ok && name != "" { - names = append(names, name) + if s := richtext.SanitizeSingleLine(name); s != "" { + names = append(names, s) + } } } } diff --git a/internal/presenter/template.go b/internal/presenter/template.go index b98095749..474ce3d0f 100644 --- a/internal/presenter/template.go +++ b/internal/presenter/template.go @@ -34,7 +34,13 @@ func RenderTemplate(tmpl string, data map[string]any) string { if err := t.Execute(&buf, sanitizeNumericValues(data)); err != nil { return "