diff --git a/internal/auth/agent.go b/internal/auth/agent.go index a01b0c301..60f14e171 100644 --- a/internal/auth/agent.go +++ b/internal/auth/agent.go @@ -15,6 +15,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/config" "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/richtext" ) // Agent self-tokens: the RFC 6749 §4.4 client_credentials grant. @@ -465,7 +466,7 @@ func (m *Manager) accountToBind() string { source := config.Source(m.cfg.Sources["account_id"]) supplied := source == config.SourceFlag || source == config.SourceEnv if supplied && m.cfg.AccountID != "" { - return shellQuote(m.cfg.AccountID) + return richtext.ShellQuote(m.cfg.AccountID) } return "" } @@ -511,11 +512,11 @@ func (m *Manager) agentRemedy(err error, clientID, scope string) error { func (m *Manager) agentLoginCommand(clientID, scope string) string { id := "" if clientID != "" { - id = shellQuote(clientID) + id = richtext.ShellQuote(clientID) } profile := "" if m.cfg.ActiveProfile != "" { - profile = shellQuote(m.cfg.ActiveProfile) + profile = richtext.ShellQuote(m.cfg.ActiveProfile) } command := "... | basecamp auth login --with-client-credentials --client-id " + id + " -P " + profile // A profile with no entry yet is one the login creates, and creating @@ -538,7 +539,7 @@ func (m *Manager) agentLoginCommand(clientID, scope string) string { // back with full access, or be refused for asking for more than its // client is allowed. full is the default and adding it says nothing. if scope != "" && scope != scopeFull { - command += " --scope " + shellQuote(scope) + command += " --scope " + richtext.ShellQuote(scope) } return command } diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 29c5cc58c..c5a7b56aa 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -192,35 +192,11 @@ func (m *Manager) loginRemedy() (command, lead string) { return m.agentLoginCommand(kind.clientID, kind.scope), "Pipe the agent's client secret in:" } if m.cfg.ActiveProfile != "" { - return "basecamp auth login -P " + shellQuote(m.cfg.ActiveProfile), "Run:" + return "basecamp auth login -P " + richtext.ShellQuote(m.cfg.ActiveProfile), "Run:" } return "basecamp auth login", "Run:" } -// shellQuote renders s safe to embed in an emitted shell command: a clearly -// inert name passes through bare, anything else is single-quoted — the one -// POSIX form in which nothing substitutes — with embedded single quotes -// spelled: -// -// '\'' -// -// indented so gofmt leaves it as written. Profile names come from -// configuration files, which do not -// apply the create-time name check. -func shellQuote(s string) string { - if s != "" && strings.IndexFunc(s, shellActive) < 0 { - return s - } - return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" -} - -// shellActive reports whether r can mean anything to a POSIX shell outside -// quotes; letters, digits and a few inert punctuation marks cannot. -func shellActive(r rune) bool { - inert := r >= 'a' && r <= 'z' || r >= 'A' && r <= 'Z' || r >= '0' && r <= '9' || strings.ContainsRune("_./:@%+=-", r) - return !inert -} - // remember records what the credential in play is, for the remedy an // error will carry. Every Manager path that gets its hands on a // credential — serving a token, renewing one, storing one, answering diff --git a/internal/commands/config.go b/internal/commands/config.go index ea5c39324..0f7ede307 100644 --- a/internal/commands/config.go +++ b/internal/commands/config.go @@ -16,6 +16,7 @@ import ( "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/richtext" "github.com/basecamp/basecamp-cli/internal/tui/resolve" ) @@ -361,7 +362,7 @@ Valid keys: account_id, project_id (or project), todolist_id, base_url, cache_di absPath, _ := filepath.Abs(configPath) ts := config.LoadTrustStore(config.GlobalConfigDir()) if ts == nil || !ts.IsTrusted(configPath) { - 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)) + fmt.Fprintf(os.Stderr, "warning: %q in local config requires trust to take effect; run:\n basecamp config trust %s\n", key, richtext.ShellQuote(absPath)) } } diff --git a/internal/commands/config_test.go b/internal/commands/config_test.go index 932bb4365..dd800efdb 100644 --- a/internal/commands/config_test.go +++ b/internal/commands/config_test.go @@ -18,6 +18,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/config" "github.com/basecamp/basecamp-cli/internal/names" "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/richtext" ) func TestAtomicWriteFile_OverwriteExisting(t *testing.T) { @@ -310,37 +311,73 @@ func TestIsAuthorityKey(t *testing.T) { // --- config set authority-key warning test --- +// The warning names a `basecamp config trust ` a person pastes, so the +// path in it is shell-quoted. +// +// This used to assert single quotes around the path unconditionally, because +// the copy of the quoting this package called wrapped every value whether or +// not it needed it. The shared richtext.ShellQuote leaves a value that can +// mean nothing to a shell alone, so on a POSIX filesystem an ordinary path +// now appears bare — the same word, spelled shorter. What "for shell safety" +// was actually claiming is the second case: a path that does carry shell +// syntax is encoded, and an apostrophe in it is spliced rather than merely +// wrapped, which is the one spelling a wrapper gets wrong. +// +// The expectation is asked of richtext.ShellQuote rather than spelled out, +// because this path is absolute and so is rooted in whatever the host calls +// its temporary directory — a spelling that may hold apostrophes or spaces +// of its own, and that is different again on Windows. Comparing against the +// encoder means the same thing on every machine. Whether an ordinary path +// comes out bare cannot be asked of an absolute path for that reason; it is +// pinned against a fixed relative fixture in +// internal/config/trust_unix_test.go. func TestConfigSet_AuthorityKeyWarnsWithPath(t *testing.T) { - app, _ := setupConfigTestApp(t) - - // Work in a temp dir so config set writes .basecamp/config.json there - 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)) - - // Capture stderr for the warning - origStderr := os.Stderr - r, w, _ := os.Pipe() - os.Stderr = w - - err := executeConfigCommand(app, "set", "base_url", "https://custom.example.com") - require.NoError(t, err) - - w.Close() - var buf [4096]byte - n, _ := r.Read(buf[:]) - os.Stderr = origStderr - - stderr := string(buf[:n]) - absPath := filepath.Join(tmpDir, ".basecamp", "config.json") - - 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") + for name, dir := range map[string]string{ + "an ordinary path": "plain", + "a path with an apostrophe in": "o'brien", + } { + t.Run(name, func(t *testing.T) { + app, _ := setupConfigTestApp(t) + + // Work in a temp dir so config set writes .basecamp/config.json there + tmpDir, _ := filepath.EvalSymlinks(t.TempDir()) + tmpDir = filepath.Join(tmpDir, dir) + require.NoError(t, os.MkdirAll(filepath.Join(tmpDir, ".basecamp"), 0o755)) + origDir, _ := os.Getwd() + require.NoError(t, os.Chdir(tmpDir)) + defer os.Chdir(origDir) + + // Capture stderr for the warning + origStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + err := executeConfigCommand(app, "set", "base_url", "https://custom.example.com") + require.NoError(t, err) + + w.Close() + var buf [4096]byte + n, _ := r.Read(buf[:]) + os.Stderr = origStderr + + stderr := string(buf[:n]) + absPath := filepath.Join(tmpDir, ".basecamp", "config.json") + + assert.Contains(t, stderr, `"base_url"`) + assert.Contains(t, stderr, "requires trust") + assert.Contains(t, stderr, "basecamp config trust "+richtext.ShellQuote(absPath), + "the warning must name the command with the path encoded for a shell") + // Only the apostrophe fixture gets extra assertions. The + // ordinary one is covered by the exact ShellQuote comparison + // above; asking it for a raw substring would be asking about + // TMPDIR, which may hold an apostrophe of its own and would + // then correctly be spliced (Copilot on #769). + if dir != "plain" { + assert.NotContains(t, stderr, absPath, "a path with an apostrophe cannot appear raw") + assert.Contains(t, stderr, `'\''`, "the apostrophe must be spliced out and back in") + } + }) + } } // TestConfigSet_GatedNonAuthorityKeyWarns verifies the trust warning also fires diff --git a/internal/commands/connect.go b/internal/commands/connect.go index 8ccf841a3..be759dcf1 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -130,7 +130,7 @@ func runConnectShow(app *appctx.App) error { switch { case errors.Is(err, os.ErrNotExist): return output.ErrNotFoundHint("connect.json for profile", name, - "The profile has not been set up. Set it up: basecamp connect setup -P "+shellQuote(name)+" --operator-profile '' --serve ") + "The profile has not been set up. Set it up: basecamp connect setup -P "+richtext.ShellQuote(name)+" --operator-profile '' --serve ") case err != nil && runtime.GOOS == "windows": return output.ErrUsageHint("connect.json cannot be used: "+setup.ErrorText(err), "The connector's setup is not supported on Windows: this CLI cannot verify who can change connect.json there.") @@ -618,7 +618,7 @@ func runConnectSetup(cmd *cobra.Command, app *appctx.App, f *connectSetupFlags) // connect.json nobody else may change is usage. func classifyWriteError(name string, err error) error { var apiErr *output.Error - profile := shellQuote(name) + profile := richtext.ShellQuote(name) switch { case errors.Is(err, context.Canceled), errors.Is(err, context.DeadlineExceeded): // A person who stopped the command, or a deadline: the run ended, @@ -853,7 +853,7 @@ func parsePositiveID(flag, raw string) (int64, error) { // against an Agent's credential or connect.json, and a person's login with // nothing pinning it to the bot. func refuseCredentialConflicts(name, path, held string, expect int64, exists bool, existing setup.File) error { - profile := shellQuote(name) + profile := richtext.ShellQuote(name) switch { // What connect.json already says comes first: a remediation that the // existing file would refuse anyway is no remediation. @@ -891,7 +891,7 @@ func connectAccount(app *appctx.App, name string) (string, error) { p := app.Config.Profiles[name] if p == nil { return "", output.ErrUsageHint(fmt.Sprintf("Profile %q does not exist", name), - "Connect the agent first: basecamp auth agent connect -P "+shellQuote(name)) + "Connect the agent first: basecamp auth agent connect -P "+richtext.ShellQuote(name)) } bound, err := canonicalAccount(p.AccountID) if err != nil { @@ -937,7 +937,7 @@ func unboundProfileError(cfg *config.Config, name string) error { } global := filepath.Join(config.GlobalConfigDir(), "config.json") return output.ErrUsageHint(message, - "For an Agent, connecting it binds its own account: basecamp auth agent connect -P "+shellQuote(name)+ + "For an Agent, connecting it binds its own account: basecamp auth agent connect -P "+richtext.ShellQuote(name)+ ". A bot user's browser login binds none, so for a bot add account_id to the profile's entry in "+richtext.SanitizeSingleLine(global)+".") } @@ -1036,7 +1036,7 @@ func operatorProfileManager(ctx context.Context, app *appctx.App, profile string case err != nil: return nil, err case kind == "": - return nil, output.ErrUsageHint(fmt.Sprintf("Operator profile %q holds no credential", profile), "Log in: basecamp auth login -P "+shellQuote(profile)) + return nil, output.ErrUsageHint(fmt.Sprintf("Operator profile %q holds no credential", profile), "Log in: basecamp auth login -P "+richtext.ShellQuote(profile)) case kind == setup.KindAgent: return nil, output.ErrUsage(fmt.Sprintf("Operator profile %q holds an Agent's credential; an operator is a person", profile)) } diff --git a/internal/commands/connect_doctor.go b/internal/commands/connect_doctor.go index 5fdcda3b5..d5dded65f 100644 --- a/internal/commands/connect_doctor.go +++ b/internal/commands/connect_doctor.go @@ -170,7 +170,7 @@ func ledgerChecks(ctx context.Context, p connectProfile) []setup.Check { if s.Hold != nil { checks = append(checks, setup.Check{Name: "Hold", Status: setup.StatusWarn, Message: fmt.Sprintf("Held since %s by %s: nothing dispatches or posts", s.Hold.HeldAt.UTC().Format(time.RFC3339), richtext.SanitizeSingleLine(s.Hold.HeldBy)), - Hint: "Review held records in basecamp connect status, then basecamp connect release -P " + shellQuote(p.name)}) + Hint: "Review held records in basecamp connect status, then basecamp connect release -P " + richtext.ShellQuote(p.name)}) } if len(s.Indeterminate) > 0 { checks = append(checks, setup.Check{Name: "Lifecycle messages", Status: setup.StatusWarn, @@ -373,7 +373,7 @@ func driverChecks(p connectProfile) []setup.Check { if p.file.Driver != setup.DriverSpawn && p.file.Driver != setup.DriverACP { checks = append(checks, setup.Check{Name: "Driver", Status: setup.StatusFail, Message: fmt.Sprintf("Driver %q is not %q or %q, and the connector refuses to start on it", p.file.Driver, setup.DriverSpawn, setup.DriverACP), - Hint: "basecamp connect setup -P " + shellQuote(p.name) + " --driver spawn"}) + Hint: "basecamp connect setup -P " + richtext.ShellQuote(p.name) + " --driver spawn"}) } return checks } diff --git a/internal/commands/connect_doctor_mcp_unix.go b/internal/commands/connect_doctor_mcp_unix.go index 6659fbc93..b01ecfcaa 100644 --- a/internal/commands/connect_doctor_mcp_unix.go +++ b/internal/commands/connect_doctor_mcp_unix.go @@ -14,6 +14,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/connector" "github.com/basecamp/basecamp-cli/internal/connector/driver" "github.com/basecamp/basecamp-cli/internal/connector/setup" + "github.com/basecamp/basecamp-cli/internal/richtext" "github.com/basecamp/basecamp-cli/internal/version" ) @@ -83,7 +84,7 @@ func mcpHandshakeCheck(ctx context.Context, profile string) setup.Check { if err != nil { session = nil c.Status, c.Message = setup.StatusFail, "The agent's MCP server did not complete the handshake: "+setup.ErrorText(err) - c.Hint = "Run basecamp mcp -P " + shellQuote(profile) + " and read its stderr." + c.Hint = "Run basecamp mcp -P " + richtext.ShellQuote(profile) + " and read its stderr." return c } tools := 0 @@ -98,6 +99,6 @@ func mcpHandshakeCheck(ctx context.Context, profile string) setup.Check { c.Status, c.Message = setup.StatusFail, "The agent's MCP server lists no tools" return c } - c.Status, c.Message = setup.StatusPass, fmt.Sprintf("The agent's MCP server (basecamp mcp -P %s) answered with %d tools; the basecamp_connect domain is served only to a dispatched worker", shellQuote(profile), tools) + c.Status, c.Message = setup.StatusPass, fmt.Sprintf("The agent's MCP server (basecamp mcp -P %s) answered with %d tools; the basecamp_connect domain is served only to a dispatched worker", richtext.ShellQuote(profile), tools) return c } diff --git a/internal/commands/connect_operator.go b/internal/commands/connect_operator.go index f906f3065..3709e2754 100644 --- a/internal/commands/connect_operator.go +++ b/internal/commands/connect_operator.go @@ -71,7 +71,7 @@ func loadConnectProfile(cmd *cobra.Command) (connectProfile, error) { file, err := setup.Load(path) switch { case errors.Is(err, os.ErrNotExist): - return connectProfile{}, output.ErrUsageHint(fmt.Sprintf("Profile %q is not set up as a connector", name), "Run: basecamp connect setup -P "+shellQuote(name)) + return connectProfile{}, output.ErrUsageHint(fmt.Sprintf("Profile %q is not set up as a connector", name), "Run: basecamp connect setup -P "+richtext.ShellQuote(name)) case err != nil: return connectProfile{}, output.ErrUsage("connect.json cannot be used: " + err.Error()) } @@ -119,7 +119,7 @@ func openConnectLedger(ctx context.Context, p connectProfile, requireStopped boo } path := filepath.Join(dir, connector.LedgerFile) if _, err := os.Lstat(path); errors.Is(err, os.ErrNotExist) { - return nil, done, output.ErrUsageHint(fmt.Sprintf("Profile %q's connector has no ledger yet", p.name), "Run the connector first: basecamp connect -P "+shellQuote(p.name)) + return nil, done, output.ErrUsageHint(fmt.Sprintf("Profile %q's connector has no ledger yet", p.name), "Run the connector first: basecamp connect -P "+richtext.ShellQuote(p.name)) } // Opening for a decision migrates the ledger, and a connector running on // an older binary's schema must not have its triggers replaced under it. @@ -232,9 +232,9 @@ func runConnectStatus(cmd *cobra.Command, shadow bool) error { ledger, err := connector.OpenLedgerReadOnly(cmd.Context(), filepath.Join(dir, connector.LedgerFile)) if errors.Is(err, os.ErrNotExist) { if shadow { - return output.ErrUsageHint(fmt.Sprintf("Profile %q has no shadow ledger", p.name), "Run the shadow connector first: basecamp connect -P "+shellQuote(p.name)+" --shadow") + return output.ErrUsageHint(fmt.Sprintf("Profile %q has no shadow ledger", p.name), "Run the shadow connector first: basecamp connect -P "+richtext.ShellQuote(p.name)+" --shadow") } - return output.ErrUsageHint(fmt.Sprintf("Profile %q's connector has no ledger yet", p.name), "Run the connector first: basecamp connect -P "+shellQuote(p.name)) + return output.ErrUsageHint(fmt.Sprintf("Profile %q's connector has no ledger yet", p.name), "Run the connector first: basecamp connect -P "+richtext.ShellQuote(p.name)) } if err != nil { return err @@ -764,7 +764,7 @@ The file is JSON: return err } if _, err := os.Lstat(filepath.Join(dir, connector.LedgerFile)); errors.Is(err, os.ErrNotExist) { - return output.ErrUsageHint(fmt.Sprintf("Profile %q's connector has no ledger yet", p.name), "Promote the shadow first: basecamp connect shadow promote -P "+shellQuote(p.name)) + return output.ErrUsageHint(fmt.Sprintf("Profile %q's connector has no ledger yet", p.name), "Promote the shadow first: basecamp connect shadow promote -P "+richtext.ShellQuote(p.name)) } // The connector must be stopped: the open takes the instance lock // and holds it for the import. diff --git a/internal/commands/feed.go b/internal/commands/feed.go index d03240926..a7c024615 100644 --- a/internal/commands/feed.go +++ b/internal/commands/feed.go @@ -15,6 +15,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/richtext" ) // The account event feed: the catch-up poll lane (`events poll`), the agent @@ -549,7 +550,7 @@ func (r feedRequest) resumeNotice(position string, capped bool, maxPages int) st // into a command somebody pastes, and nothing about it promises to be a bare // word. func (r feedRequest) positionCommand(position string) string { - return fmt.Sprintf("%s --position %s%s", r.lane.pollCmd, shellQuote(position), r.filters) + return fmt.Sprintf("%s --position %s%s", r.lane.pollCmd, richtext.ShellQuote(position), r.filters) } // newEventsPollCmd builds `basecamp events poll`. diff --git a/internal/commands/files.go b/internal/commands/files.go index decd18e54..ed272e4af 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -7,7 +7,6 @@ import ( "net/url" "os" "path/filepath" - "regexp" "slices" "strconv" "strings" @@ -1675,7 +1674,7 @@ You can pass either an upload ID or a Basecamp URL: // its project scope, and an explicit --project carries over — // unlike versions, both follow-up commands resolve a project // before fetching, so a bare ID could prompt or fail headless. - ref := shellQuote(args[0]) + ref := richtext.ShellQuote(args[0]) scope := breadcrumbScope(scopeProject(app, *project)) respOpts := []output.ResponseOption{ @@ -1871,7 +1870,7 @@ You can pass either an upload ID or a Basecamp URL: // Breadcrumbs reuse the caller's own reference and carry an // explicit --project: download resolves a project before fetching, // so a bare ID without the scope could prompt or fail headless. - ref := shellQuote(args[0]) + ref := richtext.ShellQuote(args[0]) scope := breadcrumbScope(scopeProject(app, *project)) return app.OK(upload, @@ -1900,28 +1899,10 @@ You can pass either an upload ID or a Basecamp URL: return cmd } -// shellSafeRe matches strings that need no quoting in an emitted shell -// command: IDs, plain Basecamp URLs, and simple names. Everything else gets -// single-quoted. -var shellSafeRe = regexp.MustCompile(`^[A-Za-z0-9_./:@%+=-]+$`) - -// shellQuote renders s safe to embed in an emitted shell command. Clearly -// inert strings pass through bare; anything else is single-quoted — the one -// POSIX form in which nothing substitutes — with embedded single quotes -// spliced out and back in as: -// -// '\'' -// -// indented so gofmt leaves it as written. This is an -// encoding applied to every embedded value, not a -// metacharacter list: breadcrumbs interpolate user- and API-controlled text, -// and escaping cases one at a time is how quoting bugs recur. -func shellQuote(s string) string { - if s != "" && shellSafeRe.MatchString(s) { - return s - } - return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" -} +// shellQuote is richtext.ShellQuote under the name this package gave its own +// copy, which this change removed. One call site is left — connect_run.go, +// which another change owns — and this goes when that one moves. +func shellQuote(s string) string { return richtext.ShellQuote(s) } // scopeProject resolves the project value a breadcrumb should carry: the // group-level flag, else the root-level --project (app.Flags.Project) — the @@ -1939,7 +1920,7 @@ func breadcrumbScope(project string) string { if project == "" { return "" } - return " --project " + shellQuote(project) + return " --project " + richtext.ShellQuote(project) } func newFilesUpdateCmd(project *string) *cobra.Command { diff --git a/internal/commands/files_test.go b/internal/commands/files_test.go index c8db0e84c..5227d9b6d 100644 --- a/internal/commands/files_test.go +++ b/internal/commands/files_test.go @@ -20,6 +20,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/richtext" ) func TestIsStorageURL(t *testing.T) { @@ -1493,21 +1494,36 @@ func TestScopeProject(t *testing.T) { assert.Equal(t, "", scopeProject(app, "")) } -// TestShellQuote pins the breadcrumb encoding: inert strings (IDs, plain -// Basecamp URLs) pass bare, and everything else is single-quoted so no shell -// syntax survives — quoting is an encoding, not a metacharacter list. -func TestShellQuote(t *testing.T) { +// TestBreadcrumbsQuoteTheProjectTheyCarry pins the breadcrumb encoding at +// the caller. This package used to hold its own copy of the quoting and +// this test called it directly; the copy is richtext.ShellQuote now, so the +// test asks the breadcrumb itself instead — inert names pass bare, and +// everything else is single-quoted so no shell syntax survives. Quoting is +// an encoding, not a metacharacter list. +func TestBreadcrumbsQuoteTheProjectTheyCarry(t *testing.T) { + for input, want := range map[string]string{ + "789": " --project 789", + "release;id": ` --project 'release;id'`, + "$(command) project": ` --project '$(command) project'`, + "My Project": ` --project 'My Project'`, + "O'Brien's": ` --project 'O'\''Brien'\''s'`, + "": "", + } { + assert.Equal(t, want, breadcrumbScope(input), "breadcrumbScope(%q)", input) + } +} + +// And the upload reference a breadcrumb echoes back gets the same treatment: +// a plain ID or URL stays readable, a URL carrying shell syntax does not +// survive as shell syntax. +func TestABreadcrumbReferenceIsQuotedLikeTheProject(t *testing.T) { for input, want := range map[string]string{ "789": "789", "https://3.basecamp.com/99999/buckets/456/uploads/789": "https://3.basecamp.com/99999/buckets/456/uploads/789", "https://3.basecamp.com/99999/buckets/456/uploads/789?x=$(touch pwned)": `'https://3.basecamp.com/99999/buckets/456/uploads/789?x=$(touch pwned)'`, - "release;id": `'release;id'`, - "$(command) project": `'$(command) project'`, - "My Project": `'My Project'`, - "O'Brien's": `'O'\''Brien'\''s'`, - "": `''`, + "": `''`, } { - assert.Equal(t, want, shellQuote(input), "shellQuote(%q)", input) + assert.Equal(t, want, richtext.ShellQuote(input), "richtext.ShellQuote(%q)", input) } } diff --git a/internal/commands/templates.go b/internal/commands/templates.go index a8482b3dc..34da8a7fc 100644 --- a/internal/commands/templates.go +++ b/internal/commands/templates.go @@ -14,6 +14,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/dateparse" "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/richtext" ) // NewTemplatesCmd creates the templates command for managing project and to-do list templates. @@ -295,7 +296,7 @@ func validateTemplateCopyTodoset(cmd *cobra.Command, app *appctx.App, todosetID, func templateCommandContextArgs(profile string, persistentAccount bool, accountID string) string { args := "" if profile != "" { - args += " --profile " + shellQuote(profile) + args += " --profile " + richtext.ShellQuote(profile) } return args + replyAccountArg(persistentAccount, accountID) } diff --git a/internal/config/config.go b/internal/config/config.go index f62b5dc24..1c64a3ad5 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -8,6 +8,8 @@ import ( "os" "path/filepath" "strings" + + "github.com/basecamp/basecamp-cli/internal/richtext" ) // Config holds the resolved configuration. @@ -246,7 +248,7 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { if v, ok := fileCfg["base_url"].(string); ok && v != "" { if untrusted { - fmt.Fprintf(os.Stderr, "warning: ignoring base_url %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)) + fmt.Fprintf(os.Stderr, "warning: ignoring base_url %q from %s config at %s\n (authority key from local/repo config; run `basecamp config trust %s` to allow)\n", v, source, path, richtext.ShellQuote(path)) } else { cfg.BaseURL = v cfg.Sources["base_url"] = string(source) @@ -274,7 +276,7 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { // 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)) + 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, richtext.ShellQuote(path)) } else { cfg.CacheDir = filepath.Clean(v) cfg.Sources["cache_dir"] = string(source) @@ -282,7 +284,7 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { } if v, ok := fileCfg["cache_enabled"].(bool); ok { 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)) + 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, richtext.ShellQuote(path)) } else { cfg.CacheEnabled = v cfg.Sources["cache_enabled"] = string(source) @@ -315,7 +317,7 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { } if v, ok := fileCfg["llm_provider"].(string); ok && v != "" { if untrusted { - fmt.Fprintf(os.Stderr, "warning: ignoring llm_provider %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)) + fmt.Fprintf(os.Stderr, "warning: ignoring llm_provider %q from %s config at %s\n (authority key from local/repo config; run `basecamp config trust %s` to allow)\n", v, source, path, richtext.ShellQuote(path)) } else { cfg.LLMProvider = v cfg.Sources["llm_provider"] = string(source) @@ -325,7 +327,7 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { // 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)) + 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, richtext.ShellQuote(path)) } else { cfg.LLMModel = v cfg.Sources["llm_model"] = string(source) @@ -342,7 +344,7 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { } if v, ok := fileCfg["llm_endpoint"].(string); ok && v != "" { 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)) + 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, richtext.ShellQuote(path)) } else { // Keep the value even if malformed (non-http(s)/hostless): // summarize.ValidateEndpoint rejects it at the point of @@ -361,7 +363,7 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { // 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)) + 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, richtext.ShellQuote(path)) } else if iv >= 1 && iv <= 10 && fv == float64(iv) { cfg.LLMMaxConcurrent = iv cfg.Sources["llm_max_concurrent"] = string(source) @@ -373,7 +375,7 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { iv := int(fv) // 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)) + 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, richtext.ShellQuote(path)) } else if iv >= 100 && iv <= 100000 && fv == float64(iv) { cfg.LLMTokenBudget = iv cfg.Sources["llm_token_budget"] = string(source) @@ -393,7 +395,7 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { } if v, ok := fileCfg["default_profile"].(string); ok && v != "" { if untrusted { - fmt.Fprintf(os.Stderr, "warning: ignoring default_profile %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)) + fmt.Fprintf(os.Stderr, "warning: ignoring default_profile %q from %s config at %s\n (authority key from local/repo config; run `basecamp config trust %s` to allow)\n", v, source, path, richtext.ShellQuote(path)) } else { cfg.DefaultProfile = v cfg.Sources["default_profile"] = string(source) @@ -401,7 +403,7 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { } if v, ok := fileCfg["profiles"].(map[string]any); ok { if untrusted { - fmt.Fprintf(os.Stderr, "warning: ignoring profiles from %s config at %s\n (authority key from local/repo config; run `basecamp config trust %s` to allow)\n", source, path, ShellQuote(path)) + fmt.Fprintf(os.Stderr, "warning: ignoring profiles from %s config at %s\n (authority key from local/repo config; run `basecamp config trust %s` to allow)\n", source, path, richtext.ShellQuote(path)) } else { for name, profileData := range v { if profileMap, ok := profileData.(map[string]any); ok { @@ -894,20 +896,3 @@ func IsHTTPURL(rawURL string) bool { } return (u.Scheme == "http" || u.Scheme == "https") && u.Hostname() != "" } - -// ShellQuote returns a POSIX single-quoted string safe for copy-paste into -// a shell. A single quote cannot be escaped inside single quotes, so each -// one in the value is spliced out and back in as: -// -// '\'' -// -// that is: quote, backslash, quote, quote. It is written as an indented -// block on purpose — gofmt reformats doc-comment prose and rewrites that -// sequence into a curly closing quote, so a caller copying it out of prose -// would get a form no shell reads (Copilot on #765). -// -// internal/richtext.ShellQuote is the shared one new code should use; this -// copy predates it, as do the ones in internal/commands and internal/auth. -func ShellQuote(s string) string { - return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" -} diff --git a/internal/config/trust_test.go b/internal/config/trust_test.go index ed302ba42..41a74076e 100644 --- a/internal/config/trust_test.go +++ b/internal/config/trust_test.go @@ -2,12 +2,16 @@ package config import ( "encoding/json" + "io" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/richtext" ) func TestTrustStore_EmptyByDefault(t *testing.T) { @@ -234,3 +238,55 @@ func TestLoadTrustStore_EmptyDir(t *testing.T) { ts := LoadTrustStore("") assert.Nil(t, ts) } + +// The untrusted-config warning names a `basecamp config trust ` a +// person pastes, so the path in it is shell-quoted. An apostrophe in the +// path is the case that tells quoting apart from wrapping — it has to be +// spliced out of the quotes and back in, since no escape reaches inside +// single quotes — and that holds wherever this builds. +// +// The fixture is a relative path under a directory this test changes into, +// so the expectation is an exact string rather than a substring of whatever +// the host spells its temporary directory as. TMPDIR may itself hold +// apostrophes or spaces, and a quoting test rooted in it would be asserting +// something about the machine (Copilot on #769). +// +// The other half of the change lives in trust_unix_test.go: this package's +// deleted copy wrapped every value whether or not it needed it, and the +// shared richtext.ShellQuote leaves an inert one alone. That is a statement +// about POSIX paths only. A Windows path carries backslashes, which mean +// something to a shell, so there is no bare path to assert there — and the +// Go suite runs on Linux, so an ungated assertion about it would have gone +// on passing while being wrong for everyone who runs the tests on Windows. +func TestTheTrustWarningSplicesAnApostropheInThePath(t *testing.T) { + t.Chdir(t.TempDir()) + const configPath = "o'brien/config.json" + require.NoError(t, os.MkdirAll("o'brien", 0o755)) + require.NoError(t, os.WriteFile(configPath, []byte(`{"base_url": "https://evil.example.com"}`), 0o644)) + + // quote, backslash, quote, quote — the splice, not a wrapper. + require.Equal(t, `'o'\''brien/config.json'`, richtext.ShellQuote(configPath)) + + assert.Contains(t, captureStderr(t, func() { + loadFromFile(Default(), configPath, SourceLocal, nil) + }), "basecamp config trust "+`'o'\''brien/config.json'`+"`") +} + +// captureStderr runs fn with os.Stderr redirected and returns what it wrote. +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + orig := os.Stderr + r, w, err := os.Pipe() + require.NoError(t, err) + os.Stderr = w + done := make(chan string, 1) + go func() { + var b strings.Builder + _, _ = io.Copy(&b, r) + done <- b.String() + }() + fn() + require.NoError(t, w.Close()) + os.Stderr = orig + return <-done +} diff --git a/internal/config/trust_unix_test.go b/internal/config/trust_unix_test.go new file mode 100644 index 000000000..81d1b4138 --- /dev/null +++ b/internal/config/trust_unix_test.go @@ -0,0 +1,46 @@ +//go:build unix + +package config + +import ( + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/richtext" +) + +// A path built from characters that can mean nothing to a shell is named +// bare in the trust warning. This package used to quote it anyway — its own +// copy of the quoting wrapped every value unconditionally — and the move to +// richtext.ShellQuote is what changed the message. Pinned here so the change +// is a decision and not a drift. +// +// The fixture is a relative path under a directory this test changes into, +// not a t.TempDir() path, because TMPDIR is the machine talking: a valid +// POSIX one may hold spaces, apostrophes or anything else, and on macOS /var +// resolves through a symlink. A quoting test rooted in it asserts something +// about the host rather than about the quoting, and would fail on a host +// that spells its temporary directory differently (Copilot on #769). The +// expectation below is therefore an exact string, the same on every machine +// this builds for. +// +// Unix-gated because the property is about POSIX paths: a Windows path +// carries backslashes, which are shell-active, so richtext.ShellQuote +// rightly quotes it there and there is nothing bare to assert. The +// apostrophe half of the same behavior is portable and lives next door in +// trust_test.go. +func TestTheTrustWarningLeavesAPathThatNeedsNoQuotingBare(t *testing.T) { + t.Chdir(t.TempDir()) + const configPath = "repo/.basecamp/config.json" + require.NoError(t, os.MkdirAll("repo/.basecamp", 0o755)) + require.NoError(t, os.WriteFile(configPath, []byte(`{"base_url": "https://evil.example.com"}`), 0o644)) + + require.Equal(t, configPath, richtext.ShellQuote(configPath), "an inert path must come through unchanged") + + assert.Contains(t, captureStderr(t, func() { + loadFromFile(Default(), configPath, SourceLocal, nil) + }), "basecamp config trust repo/.basecamp/config.json`") +} diff --git a/internal/connector/recovery_harness_test.go b/internal/connector/recovery_harness_test.go index 11141d1a3..381cc2ce8 100644 --- a/internal/connector/recovery_harness_test.go +++ b/internal/connector/recovery_harness_test.go @@ -28,6 +28,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/connector/driver/drivertest" "github.com/basecamp/basecamp-cli/internal/connector/driver/spawn" "github.com/basecamp/basecamp-cli/internal/connector/setup" + "github.com/basecamp/basecamp-cli/internal/richtext" ) // The integrated recovery harness (plan step 22). @@ -361,7 +362,7 @@ func newHarness(t *testing.T, d harnessDriver, sc harnessScenario) *harness { require.NoError(t, exeErr) h.agent = filepath.Join(dir, "agent") wrapper := "#!/bin/sh\n" + - harnessAgentEnv + "=" + shellQuote(d.Name) + " " + harnessDirEnv + "=" + shellQuote(dir) + " exec " + shellQuote(exe) + ` "$@"` + "\n" + harnessAgentEnv + "=" + richtext.ShellQuote(d.Name) + " " + harnessDirEnv + "=" + richtext.ShellQuote(dir) + " exec " + richtext.ShellQuote(exe) + ` "$@"` + "\n" require.NoError(t, os.WriteFile(h.agent, []byte(wrapper), 0o700)) //nolint:gosec // the fake agent's wrapper must be executable for _, name := range []string{feedFile, storeFile, linesFile, pollsFile, agentLogFile, liveFile} { require.NoError(t, os.WriteFile(filepath.Join(dir, name), nil, 0o600)) @@ -389,8 +390,6 @@ func harnessStateDir(t *testing.T, dir string) string { return state } -func shellQuote(s string) string { return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } - func (h *harness) writeScenario() { data, err := json.Marshal(h.sc) require.NoError(h.t, err) diff --git a/internal/harness/codex_unix_test.go b/internal/harness/codex_unix_test.go index db4fdbb32..21e6f8cd1 100644 --- a/internal/harness/codex_unix_test.go +++ b/internal/harness/codex_unix_test.go @@ -15,6 +15,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/richtext" ) // codexWrapper runs script through sh as the probe's command, with a @@ -27,9 +29,13 @@ func codexWrapper(t *testing.T, script string, deadline time.Duration) (int, err t.Skip("sh not available") } pidFile := filepath.Join(t.TempDir(), "descendant.pid") - // TempDir follows TMPDIR, which may hold a space or a shell metacharacter, - // so the path goes into the script single-quoted. - script = strings.ReplaceAll(script, "PIDFILE", "'"+strings.ReplaceAll(pidFile, "'", `'\''`)+"'") + // TempDir follows TMPDIR, which may hold a space or a shell + // metacharacter, so the path is encoded before it goes into the script. + // richtext.ShellQuote is the one implementation of that; this used to + // splice the quotes inline here, which is a copy with no name on it and + // so the kind a search for duplicate quoting never finds (Copilot on + // #769). + script = strings.ReplaceAll(script, "PIDFILE", richtext.ShellQuote(pidFile)) ctx, cancel := context.WithTimeout(context.Background(), deadline) defer cancel() diff --git a/internal/richtext/shellquote.go b/internal/richtext/shellquote.go index 8002b08d7..51f92c7f8 100644 --- a/internal/richtext/shellquote.go +++ b/internal/richtext/shellquote.go @@ -21,9 +21,12 @@ import "strings" // which is held to whatever check applied when the value was first created. // Escaping cases one at a time is how quoting bugs recur. // -// internal/commands and internal/auth each carry a copy of this, written -// before there was a shared home for it. This is the one new code should -// use; converging those two is not this change's to do. +// internal/auth, internal/config, internal/commands and the connector's +// recovery harness each carried a copy of this, written before there was a +// shared home for it. They are gone: this is the only implementation, and +// the only one new code should use. Two of those copies quoted +// unconditionally, so a value that needs no quoting now appears bare where +// they were used — the same shell word, spelled shorter. func ShellQuote(s string) string { if s != "" && strings.IndexFunc(s, shellActive) < 0 { return s diff --git a/internal/richtext/shellquote_unix_test.go b/internal/richtext/shellquote_unix_test.go index 467e2affe..aad1c5656 100644 --- a/internal/richtext/shellquote_unix_test.go +++ b/internal/richtext/shellquote_unix_test.go @@ -22,7 +22,12 @@ import ( func TestShellQuoteSurvivesARealShell(t *testing.T) { for _, in := range []string{ "agent", "", "two words", "a;rm -rf /", "$(echo pwned)", "`echo pwned`", - "it's", "'", "'; echo pwned; '", "a\nb", `back\slash`, "*", "~root", + "it's", "'", "'; echo pwned; '", "a\nb", "a\tb", `back\slash`, "*", "~root", + // The shapes the three deleted copies carried: a profile name + // (auth), a config path (config), and an opaque feed position + // (commands). An apostrophe in a home directory is the case that + // tells quoting apart from wrapping, and it is not hypothetical. + "work profile", "/home/o'brien/.basecamp/config.json", "pos 1; rm -rf /", } { out, err := exec.CommandContext(t.Context(), "/bin/sh", "-c", "printf %s "+ShellQuote(in)).Output() require.NoError(t, err, "input %q quoted as %s", in, ShellQuote(in))