diff --git a/e2e/stdin_dash.bats b/e2e/stdin_dash.bats new file mode 100644 index 000000000..7458f5e97 --- /dev/null +++ b/e2e/stdin_dash.bats @@ -0,0 +1,149 @@ +#!/usr/bin/env bats +# stdin_dash.bats - "-" (read from stdin) support and the tier-2 dash guard. +# +# Tier 1: content inputs accept "-" to read piped stdin. Tier 2: everywhere +# else, a literal "-" combined with piped stdin is a usage error instead of +# silently becoming literal content — except cobra's generated meta commands +# (help, __complete), which are deliberately exempt and covered below. Every +# case resolves locally — usage errors before any request, or a config write — +# so no cassette or server is needed. + +load test_helper + + +# Tier 1 — "-" resolves against stdin + +@test "todos create - with empty pipe is a usage error, not an empty todo" { + create_credentials + create_global_config '{"account_id": 99999, "project_id": 123}' + + run bash -c "printf '' | basecamp todos create - --json" + assert_failure + assert_json_value '.code' 'usage' + assert_output_contains "empty" +} + +@test "comments create - on a TTY-like stdin errors immediately instead of hanging" { + create_credentials + create_global_config '{"account_id": 99999, "project_id": 123}' + + # /dev/null is a character device — the TTY stand-in. Must not block. + run bash -c "basecamp comments create 123 - --json < /dev/null" + assert_failure + assert_json_value '.code' 'usage' + assert_output_contains "nothing is piped" +} + +@test "comments create with a bare pipe and no dash teaches the dash" { + create_credentials + create_global_config '{"account_id": 99999, "project_id": 123}' + + run bash -c "printf 'hello' | basecamp comments create 123 --json" + assert_failure + assert_json_value '.code' 'usage' + assert_json_value '.hint | contains("pass \"-\"")' 'true' +} + + +# Tier 2 — stray literal "-" with a pipe is rejected + +@test "projects create - with piped stdin is rejected with the -- escape" { + create_credentials + create_global_config '{"account_id": 99999}' + + run bash -c "printf 'x' | basecamp projects create - --json" + assert_failure + assert_json_value '.code' 'usage' + assert_json_value '.error | contains("")' 'true' + assert_json_value '.hint | contains("after the -- separator")' 'true' +} + +@test "a -- - after the separator passes the guard and lands literally" { + create_credentials + create_global_config '{"account_id": 99999}' + + # config set writes locally — a deterministic success proving the escaped + # "-" passed the guard and was stored as a literal value. + run bash -c "cd '$TEST_PROJECT' && printf 'x' | basecamp config set project_id --json -- -" + assert_success + assert_json_value '.data.value' '-' + + run bash -c "cd '$TEST_PROJECT' && basecamp config show --json < /dev/null" + assert_success + assert_json_value '.data.project_id.value' '-' +} + +@test "a piped bare dash at the root is rejected, not silently quick-started" { + create_credentials + create_global_config '{"account_id": 99999}' + + run bash -c "printf 'x' | basecamp - --json" + assert_failure + assert_json_value '.code' 'usage' + assert_json_value '.error | contains("does not read stdin")' 'true' +} + +@test "basecamp unknowncmd still reports an unknown command" { + create_credentials + create_global_config '{"account_id": 99999}' + + run bash -c "basecamp unknowncmd --json < /dev/null" + assert_failure + assert_output_contains "unknown command" +} + +@test "piped help - is exempt from the dash guard" { + create_credentials + create_global_config '{"account_id": 99999}' + + run bash -c "printf 'x' | basecamp help - --json" + assert_success +} + +@test "piped completion of a flag word is exempt from the dash guard" { + create_credentials + create_global_config '{"account_id": 99999}' + + # The shell passes the word being completed, so "todos create -" runs + # this. Guarding it would break flag completion everywhere. + run bash -c "printf 'x' | basecamp __complete todos create -" + assert_success + assert_output_contains "--description" +} + + +# --jq error rendering. Not stdin-specific, but it shares this file's contract: +# exactly one document reaches stdout, so a machine consumer can parse it. + +@test "a jq filter that fails partway leaves one document on stdout" { + create_credentials + create_global_config '{"account_id": 99999}' + + # This filter emits .error, then raises. writeJQ streams results as it + # produces them, so the first line is already on stdout when it fails — + # replaying the envelope would append a second, incompatible document. + run bash -c "basecamp todos create --jq '.error, error(\"stop\")' 2>/dev/null < /dev/null" + assert_failure + [ "${#lines[@]}" -eq 1 ] + assert_output_contains "required" + + # The failure is still reported on stderr, but response-selected text in a + # jq runtime error cannot inject terminal controls or forge another line. + run bash -c "basecamp todos create --jq 'error(\"\\u001b[31mPWN\\u001b[0m\\nforged\")' 2>&1 >/dev/null < /dev/null" + assert_failure + [ "${#lines[@]}" -eq 1 ] + [[ "$output" != *$'\033'* ]] + assert_output_contains "--jq" + assert_output_contains "PWN forged" +} + +@test "an unparseable jq filter still renders an error raised before validation" { + create_credentials + create_global_config '{"account_id": 99999}' + + # The stray-dash guard fires before --jq is validated, so the envelope would + # otherwise be rendered through an unparseable filter and print nothing. + run bash -c "printf 'x' | basecamp --jq '.[invalid' - 2>/dev/null" + assert_failure + assert_output_contains "does not read stdin" +} diff --git a/internal/appctx/context.go b/internal/appctx/context.go index 53bacef99..54906af87 100644 --- a/internal/appctx/context.go +++ b/internal/appctx/context.go @@ -19,6 +19,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/observability" "github.com/basecamp/basecamp-cli/internal/output" "github.com/basecamp/basecamp-cli/internal/resilience" + "github.com/basecamp/basecamp-cli/internal/stdinarg" "github.com/basecamp/basecamp-cli/internal/tui/resolve" "github.com/basecamp/basecamp-cli/internal/version" ) @@ -374,13 +375,11 @@ func (a *App) IsInteractive() bool { return false } - // Check if stdout is a terminal - fi, err := os.Stdout.Stat() - if err != nil { - return false - } - - return (fi.Mode() & os.ModeCharDevice) != 0 + // Both stdout and stdin must be character devices: a TUI draws to stdout + // and reads keystrokes from stdin, so a pipe on either end can never + // drive one — and when the command is consuming piped content (a "-" + // stdin input), a TUI would eat that content as key events. + return stdinarg.InteractiveStdio() } // WithApp stores the app in the context. diff --git a/internal/cli/cobra_error_test.go b/internal/cli/cobra_error_test.go new file mode 100644 index 000000000..7ca7f7866 --- /dev/null +++ b/internal/cli/cobra_error_test.go @@ -0,0 +1,125 @@ +package cli + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + + "github.com/basecamp/basecamp-cli/internal/output" +) + +// Cobra's arity messages are usage failures by construction. They used to fall +// through to the default classification (api_error, exit 7), which tells an +// agent to retry a call that can never succeed. +func TestTransformCobraErrorClassifiesArityAsUsage(t *testing.T) { + for _, msg := range []string{ + "accepts at most 2 arg(s), received 3", + "accepts 1 arg(s), received 2", + "accepts between 1 and 2 arg(s), received 4", + } { + t.Run(msg, func(t *testing.T) { + err := transformCobraError(errors.New(msg)) + + var outErr *output.Error + require.True(t, errors.As(err, &outErr), "expected *output.Error, got %T", err) + assert.Equal(t, output.CodeUsage, outErr.Code) + assert.Equal(t, msg, outErr.Message, "the wording is already clear; only the code was wrong") + }) + } +} + +// The zero-arg case keeps its friendlier rewrite. +func TestTransformCobraErrorKeepsZeroArgRewrite(t *testing.T) { + err := transformCobraError(errors.New("accepts 1 arg(s), received 0")) + + var outErr *output.Error + require.True(t, errors.As(err, &outErr)) + assert.Equal(t, output.CodeUsage, outErr.Code) + assert.Equal(t, "ID required", outErr.Message) +} + +// A typed error already carries a code, an HTTP status and a retryable flag. +// Matching on its rendered text would flatten all of that into a bare usage +// string — so an API error that merely quotes an arity phrase is left alone. +func TestTransformCobraErrorPreservesTypedErrors(t *testing.T) { + t.Run("SDK error", func(t *testing.T) { + original := &basecamp.Error{ + Code: basecamp.CodeAPI, + Message: "server rejected the payload: accepts 1 arg(s), received 2", + HTTPStatus: 422, + Retryable: true, + } + + err := transformCobraError(original) + + var sdkErr *basecamp.Error + require.True(t, errors.As(err, &sdkErr), "expected the SDK error to survive, got %T", err) + assert.Equal(t, basecamp.CodeAPI, sdkErr.Code) + assert.Equal(t, 422, sdkErr.HTTPStatus) + assert.True(t, sdkErr.Retryable) + }) + + t.Run("output error", func(t *testing.T) { + original := output.ErrNotFound("todo", "123") + + err := transformCobraError(original) + + var outErr *output.Error + require.True(t, errors.As(err, &outErr)) + assert.Equal(t, output.CodeNotFound, outErr.Code, "must not be reclassified as usage") + }) +} + +// Anchored: a command's own error that merely contains the phrase is not an +// arity failure and must keep its own classification path. +func TestTransformCobraErrorIgnoresUnanchoredArityText(t *testing.T) { + msg := "the API said: accepts 1 arg(s), received 2 (and then some)" + + err := transformCobraError(errors.New(msg)) + + var outErr *output.Error + assert.False(t, errors.As(err, &outErr), "should be left untouched, got %T", err) + assert.Equal(t, msg, err.Error()) +} + +// jqUsable decides whether the fallback writer may keep a filter. It answers +// only the question that is knowable before any output exists — a filter that +// parses and compiles can still fail partway through producing results, which +// is why the jq-backed path never retries a failed write. That end-to-end +// property needs the real binary and is covered in e2e/stdin_dash.bats; this +// only pins the predicate. +func TestJQUsable(t *testing.T) { + for _, tc := range []struct { + name string + filter string + want bool + }{ + {"valid", ".error", true}, + {"parse failure", ".[invalid", false}, + {"compile failure", ".foo | undefined_function", false}, + {"valid but fails at runtime", `.error, error("stop")`, true}, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, jqUsable(tc.filter)) + }) + } +} + +func TestJQRenderErrorDiagnosticIsTerminalSafeAndSingleLine(t *testing.T) { + err := output.ErrJQRuntime(errors.New("error: \x1b[31mPWN\x1b[0m\r\nforged\tline \x1b]8;;https://evil.example\aLINK\x1b]8;;\a \u009b31mC1\u009b0m")) + + got := jqRenderErrorDiagnostic(err) + + assert.NotContains(t, got, "\x1b") + assert.NotContains(t, got, "\r") + assert.NotContains(t, got, "\n") + assert.NotContains(t, got, "\t") + assert.NotContains(t, got, "\u009b") + assert.Contains(t, got, "PWN") + assert.Contains(t, got, "forged line LINK") + assert.Contains(t, got, "C1") +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 49a6c7846..745791be5 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -2,6 +2,7 @@ package cli import ( "encoding/json" + "errors" "fmt" "os" "path/filepath" @@ -9,6 +10,7 @@ import ( "sort" "strings" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/itchyny/gojq" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -20,6 +22,8 @@ import ( "github.com/basecamp/basecamp-cli/internal/harness" "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/stdinarg" "github.com/basecamp/basecamp-cli/internal/tui" "github.com/basecamp/basecamp-cli/internal/version" ) @@ -352,6 +356,12 @@ func Execute() { cmd.AddCommand(commands.NewBonfireCmd()) cmd.AddCommand(commands.NewAgentHookCmd()) + // Tier-2 stdin guard: reject a stray literal "-" when stdin is piped, + // everywhere a command doesn't explicitly accept it — except cobra's + // generated meta commands, which are deliberately exempt (see + // commands.InstallDashGuard). + commands.InstallDashGuard(cmd) + // Use ExecuteC to get the executed command (for correct context access) executedCmd, err := cmd.ExecuteC() @@ -387,12 +397,27 @@ func Execute() { disableJQ := output.IsJQError(err) if !disableJQ { if app := appctx.FromContext(executedCmd.Context()); app != nil { - if writeErr := app.Err(err); writeErr == nil { + writeErr := app.Err(err) + if writeErr == nil { os.Exit(output.ExitCodeFor(apiErr.Code)) } - // app.Err() write failed (e.g. jq runtime error on the error - // envelope, or broken pipe). Disable jq in the fallback writer - // to avoid replaying the same failure. + + // The write failed. When a filter was in play it may already + // have emitted results — writeJQ streams each one as it is + // produced, so a filter like `.error, error("stop")` prints + // before it fails. Replaying the envelope on stdout would + // append a second, unfiltered document to the first, which is + // two incompatible outputs for the machine consumer the filter + // exists to serve. Once a jq-backed write has begun, stdout is + // final: report the failure on stderr and stop. + if jq, _ := cmd.PersistentFlags().GetString("jq"); jq != "" { + fmt.Fprintln(os.Stderr, jqRenderErrorDiagnostic(writeErr)) + os.Exit(output.ExitCodeFor(apiErr.Code)) + } + + // No filter: nothing partial can have been written through one, + // so the plain fallback below is still the right last resort + // (e.g. a broken pipe). disableJQ = true } } @@ -430,6 +455,18 @@ func Execute() { format = output.FormatJSON } + // This path runs only when nothing has been written yet: either no app + // was available (an error raised before it was built) or no filter was + // in play. An unusable filter must not swallow the error — --jq is + // validated in the pre-run, so an error raised *before* that check + // would otherwise render through an unparseable filter and exit + // non-zero having printed nothing. Decide usability up front rather + // than retrying after a failed write, which is what the jq-backed path + // above refuses to do. + if jqFilter != "" && !jqUsable(jqFilter) { + jqFilter = "" + } + writer := output.New(output.Options{ Format: format, Writer: os.Stdout, @@ -441,6 +478,33 @@ func Execute() { } } +// jqUsable reports whether a filter parses and compiles. Only these failures +// are knowable before any output is produced, which is what makes clearing the +// filter safe: a filter that fails at runtime may already have written. +func jqUsable(filter string) bool { + q, err := gojq.Parse(filter) + if err != nil { + return false + } + _, err = gojq.Compile(q, gojq.WithEnvironLoader(os.Environ)) + return err == nil +} + +// jqRenderErrorDiagnostic renders a terminal-safe, single-line explanation of +// a failed jq-backed error write. A jq runtime error can include response data +// selected by the filter, so it must not reach stderr verbatim. +func jqRenderErrorDiagnostic(err error) string { + const prefix = "error rendering error output through --jq" + if err == nil { + return prefix + } + detail := richtext.SanitizeSingleLine(err.Error()) + if detail == "" { + return prefix + } + return prefix + ": " + detail +} + // resolveProfile determines which profile to use. // Resolution order: // 1. --profile / -P flag @@ -513,8 +577,14 @@ func profileNames(cfg *config.Config) string { return strings.Join(names, ", ") } -// isInteractiveTTY returns true if stdout is a character device (e.g. a -// terminal) and no noninteractive mode is set. +// isInteractiveTTY reports whether the profile picker may run: no +// noninteractive mode set, and both ends of stdio are character devices. +// +// Stdin counts because the picker is a TUI reading key events, and this runs +// from PersistentPreRunE — before any command touches its own input. Gating on +// stdout alone let "printf body | basecamp todos create -" open the picker on +// a terminal stdout and consume the piped body as keystrokes. Same predicate +// as App.IsInteractive and resolve.Resolver.IsInteractive. func isInteractiveTTY(flags appctx.GlobalFlags) bool { if config.NonInteractiveEnv() { return false @@ -525,12 +595,7 @@ func isInteractiveTTY(flags appctx.GlobalFlags) bool { return false } - // Check if stdout is a character device (e.g. a terminal) - fi, err := os.Stdout.Stat() - if err != nil { - return false - } - return (fi.Mode() & os.ModeCharDevice) != 0 + return stdinarg.InteractiveStdio() } // promptForProfile shows an interactive picker for profile selection. @@ -614,9 +679,27 @@ func isMachineConsumer(root *cobra.Command) bool { return false } +// cobraArityError matches cobra's four arity messages exactly (ExactArgs, +// MaximumNArgs, RangeArgs; MinimumNArgs is handled by an earlier rule). Anchored +// so a command's own error that merely quotes the phrase is left alone. +var cobraArityError = regexp.MustCompile(`^accepts (\d+|at most \d+|between \d+ and \d+) arg\(s\), received \d+$`) + // transformCobraError transforms Cobra's default error messages to match the // Bash CLI format for consistency with existing tests and user expectations. +// +// Only untyped errors are rewritten. An error that already carries a code, an +// HTTP status, a hint or a retryable flag is ours or the SDK's, and matching on +// its rendered text would flatten that metadata into a bare usage string. func transformCobraError(err error) error { + var outErr *output.Error + if errors.As(err, &outErr) { + return err + } + var sdkErr *basecamp.Error + if errors.As(err, &sdkErr) { + return err + } + msg := err.Error() // Transform "flag needs an argument: --FLAG" → "--FLAG requires a value" @@ -659,6 +742,14 @@ func transformCobraError(err error) error { return output.ErrUsage("ID required") } + // Every other cobra arity message ("accepts at most 2 arg(s), received 3") + // is a usage error by construction — only the code was wrong, so agents + // branching on it saw api_error and could retry a call that will never + // succeed. The wording is already clear; keep it and fix the code. + if cobraArityError.MatchString(msg) { + return output.ErrUsage(msg) + } + // Transform "required flag(s) X not set" → more specific message if strings.HasPrefix(msg, "required flag(s) ") { re := regexp.MustCompile(`required flag\(s\) "(\w+)" not set`) @@ -762,6 +853,12 @@ func emitAgentHelp(cmd *cobra.Command) { } } + // Synthesize the stdin note from the allow_dash annotation, so every + // command that accepts "-" auto-documents it. + if note := stdinDashNote(cmd, info.Args); note != "" { + info.Notes = append(info.Notes, note) + } + // Subcommands (include aliases so the CLI surface snapshot tracks them) for _, sub := range cmd.Commands() { if sub.IsAvailableCommand() || sub.Name() == "help" { @@ -826,3 +923,34 @@ func emitAgentHelp(cmd *cobra.Command) { _ = json.NewEncoder(cmd.OutOrStdout()).Encode(info) } + +// stdinDashNote renders the "-" (stdin) inputs a command accepts, from its +// allow_dash annotation: positionals by their Use-string names, flags by +// --name. Returns "" when the command reads no stdin input — --out's "-" +// means stdout, so it never appears here. +func stdinDashNote(cmd *cobra.Command, args []ArgInfo) string { + allow := stdinarg.ParseAllow(cmd.Annotations[stdinarg.AnnotationAllowDash]) + if allow.Empty() { + return "" + } + + var parts []string + for i, a := range args { + if allow.Arg(i) { + if a.Required { + parts = append(parts, "<"+a.Name+">") + } else { + parts = append(parts, "["+a.Name+"]") + } + } + } + for _, token := range strings.Fields(cmd.Annotations[stdinarg.AnnotationAllowDash]) { + if name, ok := strings.CutPrefix(token, "flag:"); ok && name != "out" { + parts = append(parts, "--"+name) + } + } + if len(parts) == 0 { + return "" + } + return "Pass - to read from stdin: " + strings.Join(parts, ", ") +} diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index 636d03c78..a68c88781 100644 --- a/internal/cli/root_test.go +++ b/internal/cli/root_test.go @@ -12,6 +12,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/commands" "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/stdinarg" "github.com/basecamp/basecamp-cli/internal/version" ) @@ -165,16 +166,7 @@ func isolateRootTest(t *testing.T) { } func TestIsInteractiveTTYWithNonInteractiveEnv(t *testing.T) { - devNull, err := os.Open(os.DevNull) - if err != nil { - t.Skip(os.DevNull + " not available") - } - origStdout := os.Stdout - os.Stdout = devNull - t.Cleanup(func() { - os.Stdout = origStdout - devNull.Close() - }) + stubCharDeviceStdio(t) t.Setenv("BASECAMP_NONINTERACTIVE", "") require.True(t, isInteractiveTTY(appctx.GlobalFlags{})) @@ -285,3 +277,89 @@ func TestVersionWithJQReturnsUsageError(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "--jq is not supported by the version command") } + +// The profile picker runs from PersistentPreRunE, before any command reads its +// own input. Piped stdin can never drive a TUI, and when the invocation is +// feeding a "-" content input the picker would eat that body as keystrokes — +// so a terminal stdout is not on its own enough to open one. +func TestIsInteractiveTTYRequiresUnpipedStdin(t *testing.T) { + stubCharDeviceStdio(t) + t.Setenv("BASECAMP_NONINTERACTIVE", "") + + require.True(t, isInteractiveTTY(appctx.GlobalFlags{}), "char-device stdio is interactive") + + reader, writer, err := os.Pipe() + require.NoError(t, err) + origStdin := os.Stdin + os.Stdin = reader + t.Cleanup(func() { + os.Stdin = origStdin + reader.Close() + writer.Close() + }) + + assert.False(t, isInteractiveTTY(appctx.GlobalFlags{}), + "piped stdin must not open the profile picker") +} + +// The root's dash guard hangs off the front of its persistent pre-run (its Args +// must stay nil for cobra's unknown-command handling), so quick-start's own +// interactive paths sit behind it. The e2e suite always has a piped stdout, +// which takes the machine-output branch and never reaches them — this covers +// the other side: a character-device stdout, the terminal stand-in, with a +// piped stdin. The root carries a subcommand so InstallDashGuard takes the +// pre-run branch production uses, not the Args branch for a childless root. +func TestRootDashGuardWithTerminalStdout(t *testing.T) { + isolateRootTest(t) + + stubCharDeviceStdio(t) + t.Setenv("BASECAMP_NONINTERACTIVE", "") + + reader, writer, err := os.Pipe() + require.NoError(t, err) + origStdin := os.Stdin + os.Stdin = reader + t.Cleanup(func() { + os.Stdin = origStdin + reader.Close() + writer.Close() + }) + _, _ = writer.WriteString("piped body") + writer.Close() + + // Terminal stdout plus piped stdin: no TUI may open, and the stray "-" + // must be rejected rather than quietly quick-starting. + assert.False(t, stdinarg.InteractiveStdio(), + "piped stdin must close every TUI gate even with a terminal stdout") + + root := NewRootCmd() + root.AddCommand(commands.NewConfigCmd()) + commands.InstallDashGuard(root) + require.Nil(t, root.Args, "the root must keep nil Args for cobra's unknown-command lookup") + root.SetIn(reader) + root.SetOut(&bytes.Buffer{}) + root.SetErr(&bytes.Buffer{}) + root.SetArgs([]string{"-"}) + + err = root.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "does not read stdin") +} + +// stubCharDeviceStdio points both stdout and stdin at /dev/null, a character +// device, so interactivity assertions do not depend on how `go test` itself was +// invoked — a piped stdin on the test runner would otherwise fail the +// interactive baseline now that both streams are checked. +func stubCharDeviceStdio(t *testing.T) { + t.Helper() + devNull, err := os.Open(os.DevNull) + if err != nil { + t.Skip(os.DevNull + " not available") + } + origStdout, origStdin := os.Stdout, os.Stdin + os.Stdout, os.Stdin = devNull, devNull + t.Cleanup(func() { + os.Stdout, os.Stdin = origStdout, origStdin + devNull.Close() + }) +} diff --git a/internal/commands/api.go b/internal/commands/api.go index 959df7628..2fa4dc3db 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -74,7 +74,10 @@ func newAPIPostCmd() *cobra.Command { cmd := &cobra.Command{ Use: "post ", Short: "POST request to API", - Long: "Make a raw POST request to any Basecamp API endpoint.", + Long: `Make a raw POST request to any Basecamp API endpoint. + +Use --data - to read the JSON body from stdin: + printf '{"content":"Buy milk"}' | basecamp api post buckets/1/todolists/2/todos.json --data -`, Example: ` basecamp api post buckets/123/todolists/456/todos.json -d '{"content":"Buy milk"}' basecamp api post buckets/123/message_boards/789/messages.json -d '{"subject":"Hello","content":"

World

"}'`, Args: apiPathArgs, @@ -85,6 +88,18 @@ func newAPIPostCmd() *cobra.Command { } app := appctx.FromContext(cmd.Context()) + + // Refuse a foreign host before the pipe is drained; the rest of the + // path parse needs the resolved account, so it follows. + if err := rejectForeignAPIPath(args[0], app.Config.BaseURL); err != nil { + return err + } + + data, err := resolveContentValue(cmd, data, -1, "--data") + if err != nil { + return err + } + if err := ensureAccount(cmd, app); err != nil { return err } @@ -116,7 +131,9 @@ func newAPIPostCmd() *cobra.Command { }, } - cmd.Flags().StringVarP(&data, "data", "d", "", "JSON request body (required)") + cmd.Flags().StringVarP(&data, "data", "d", "", "JSON request body (required); use - to read from stdin") + + allowDash(cmd, "flag:data") return cmd } @@ -125,9 +142,12 @@ func newAPIPutCmd() *cobra.Command { var data string cmd := &cobra.Command{ - Use: "put ", - Short: "PUT request to API", - Long: "Make a raw PUT request to any Basecamp API endpoint.", + Use: "put ", + Short: "PUT request to API", + Long: `Make a raw PUT request to any Basecamp API endpoint. + +Use --data - to read the JSON body from stdin: + printf '{"content":"Updated"}' | basecamp api put buckets/1/todos/2.json --data -`, Example: ` basecamp api put buckets/123/todos/456.json -d '{"content":"Updated todo"}'`, Args: apiPathArgs, RunE: func(cmd *cobra.Command, args []string) error { @@ -137,6 +157,18 @@ func newAPIPutCmd() *cobra.Command { } app := appctx.FromContext(cmd.Context()) + + // Refuse a foreign host before the pipe is drained; the rest of the + // path parse needs the resolved account, so it follows. + if err := rejectForeignAPIPath(args[0], app.Config.BaseURL); err != nil { + return err + } + + data, err := resolveContentValue(cmd, data, -1, "--data") + if err != nil { + return err + } + if err := ensureAccount(cmd, app); err != nil { return err } @@ -168,7 +200,9 @@ func newAPIPutCmd() *cobra.Command { }, } - cmd.Flags().StringVarP(&data, "data", "d", "", "JSON request body (required)") + cmd.Flags().StringVarP(&data, "data", "d", "", "JSON request body (required); use - to read from stdin") + + allowDash(cmd, "flag:data") return cmd } @@ -236,6 +270,28 @@ var accountSegmentPattern = regexp.MustCompile(`^/([0-9]+)(/.*)?$`) // 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). +// rejectForeignAPIPath answers the half of parsePath that needs only the +// configured base URL: an absolute URL on another host is refused outright, so +// credentials never leave the configured host. The account-segment half needs a +// resolved account, so it stays in parsePath — this exists so the host check can +// run before a "-" drains stdin for an invocation that cannot be sent. +func rejectForeignAPIPath(input, baseURL string) error { + candidate := strings.TrimLeft(input, "/") + lower := strings.ToLower(candidate) + if !strings.HasPrefix(lower, "http://") && !strings.HasPrefix(lower, "https://") { + return nil + } + 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) + } + return nil +} + func parsePath(input, baseURL, accountID string) (string, error) { candidate := strings.TrimLeft(input, "/") lower := strings.ToLower(candidate) diff --git a/internal/commands/attach.go b/internal/commands/attach.go index e3bbd413a..ac62cdb04 100644 --- a/internal/commands/attach.go +++ b/internal/commands/attach.go @@ -80,18 +80,34 @@ No project is needed — attachment upload is account-scoped.`, return cmd } +// validateAttachPaths checks every --attach path is readable. It needs no +// account and no network, so commands that also read a "-" content input run it +// first: an unreadable attachment dooms the invocation, and draining the pipe +// for it makes the caller wait on a producer whose output is discarded — or +// lets a blank pipe answer "stdin is empty" instead of naming the file. +func validateAttachPaths(paths []string) error { + for _, path := range paths { + if err := richtext.ValidateFile(richtext.NormalizeDragPath(path)); err != nil { + return fmt.Errorf("%s: %w", path, err) + } + } + return nil +} + // uploadAttachments uploads each path and returns attachment references. // Sequential, fails on first error. func uploadAttachments(cmd *cobra.Command, app *appctx.App, paths []string) ([]richtext.AttachmentRef, error) { + // Callers that read stdin run this first; repeating it here is idempotent + // and keeps this function correct on its own. + if err := validateAttachPaths(paths); err != nil { + return nil, err + } + refs := make([]richtext.AttachmentRef, 0, len(paths)) for _, path := range paths { normalized := richtext.NormalizeDragPath(path) - if err := richtext.ValidateFile(normalized); err != nil { - return nil, fmt.Errorf("%s: %w", path, err) - } - contentType := richtext.DetectMIME(normalized) filename := filepath.Base(normalized) diff --git a/internal/commands/attachments.go b/internal/commands/attachments.go index e1b383294..44bf3baa3 100644 --- a/internal/commands/attachments.go +++ b/internal/commands/attachments.go @@ -395,6 +395,9 @@ Options: cmd.Flags().IntVar(&index, "index", 0, "Select attachment by 1-based index") cmd.Flags().StringVarP(&recordType, "type", "t", "", "Recording type hint (todo, todolist, message, comment, card, card-table, document, schedule-entry, checkin, answer, forward, upload)") + // --out - means stream to stdout — exempt from the stdin dash guard. + allowDash(cmd, "flag:out") + return cmd } diff --git a/internal/commands/boost.go b/internal/commands/boost.go index 66d05e879..df1698f77 100644 --- a/internal/commands/boost.go +++ b/internal/commands/boost.go @@ -271,15 +271,35 @@ Use --event to boost a specific event within the item.`, Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) + + // Both identifiers are decidable from the arguments alone, so they + // are checked before the pipe is drained; runBoostCreate parses + // them again once the project is known. + recordingID, _ := extractWithProject(args[0]) + if _, err := strconv.ParseInt(recordingID, 10, 64); err != nil { + return output.ErrUsage("Invalid ID") + } + if eventID != "" { + if _, err := strconv.ParseInt(eventID, 10, 64); err != nil { + return output.ErrUsage("Invalid event ID") + } + } + + content, err := resolveContentValue(cmd, args[1], 1, "") + if err != nil { + return err + } if err := ensureAccount(cmd, app); err != nil { return err } - return runBoostCreate(cmd, app, args[0], *project, args[1], eventID) + return runBoostCreate(cmd, app, args[0], *project, content, eventID) }, } cmd.Flags().StringVar(&eventID, "event", "", "Event ID (for event-specific boosts)") + allowDash(cmd, "arg:1") + return cmd } diff --git a/internal/commands/cards.go b/internal/commands/cards.go index bfb38631e..c15a58cf1 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -851,9 +851,15 @@ func newCardsCreateCmd(project, cardTable *string) *cobra.Command { cmd := &cobra.Command{ Use: "create [body]", Short: "Create a new card", - Long: "Create a new card in a project's card table.", + Long: `Create a new card in a project's card table. + +Use - as the body argument to read the body from stdin: + printf 'Card body' | basecamp cards create "My card" - --in myproject`, Example: ` basecamp cards create "My card" --in myproject basecamp cards create --in myproject -- "--title with dashes"`, + // Bounded so a stray third token is a usage error rather than being + // silently dropped after "-" has already drained stdin. + Args: cobra.MaximumNArgs(2), RunE: func(cmd *cobra.Command, args []string) error { // Show help when invoked with no title if len(args) == 0 { @@ -864,9 +870,27 @@ func newCardsCreateCmd(project, cardTable *string) *cobra.Command { if strings.TrimSpace(title) == "" { return cmd.Help() } + // A named column needs --card-table to resolve against, and that is + // knowable from the flags alone. Attachment paths are readable or + // not regardless of the body. Both precede the read, so a doomed + // invocation never costs the caller a drained pipe. + if column != "" && !isNumericID(column) && *cardTable == "" { + return output.ErrUsage("--card-table is required when using --column with a name") + } + if err := requireNumericID(*cardTable, "card table ID"); err != nil { + return err + } + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + var content string if len(args) > 1 { - content = args[1] + var err error + content, err = resolveContentValue(cmd, args[1], 1, "[body]") + if err != nil { + return err + } } app := appctx.FromContext(cmd.Context()) @@ -875,12 +899,6 @@ func newCardsCreateCmd(project, cardTable *string) *cobra.Command { return err } - // Column name (non-numeric) requires --card-table for resolution - // Numeric column IDs can be used directly without card table discovery - if column != "" && !isNumericID(column) && *cardTable == "" { - return output.ErrUsage("--card-table is required when using --column with a name") - } - // Resolve project, with interactive fallback projectID := *project if projectID == "" { @@ -1051,6 +1069,8 @@ func newCardsCreateCmd(project, cardTable *string) *cobra.Command { cmd.Flags().StringVar(&assignee, "to", "", "Assignee (alias for --assignee)") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") + allowDash(cmd, "arg:1") + completer := completion.NewCompleter(nil) _ = cmd.RegisterFlagCompletionFunc("assignee", completer.PeopleNameCompletion()) _ = cmd.RegisterFlagCompletionFunc("to", completer.PeopleNameCompletion()) @@ -1079,12 +1099,6 @@ You can pass either a card ID or a Basecamp URL: return noChanges(cmd) } - app := appctx.FromContext(cmd.Context()) - - if err := ensureAccount(cmd, app); err != nil { - return err - } - // Extract ID from URL if provided cardIDStr := extractID(args[0]) @@ -1093,10 +1107,31 @@ You can pass either a card ID or a Basecamp URL: return output.ErrUsage("Invalid card ID") } + // Attachment paths are readable or not regardless of the body, so + // check them before the pipe is drained. + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + + // Syntactic checks first, then "-", then account and network: a + // malformed ID is answered without waiting on the producer, and a + // blank pipe cannot mask it. + content, err := resolveContentValue(cmd, content, -1, "--body") + if err != nil { + return err + } + + app := appctx.FromContext(cmd.Context()) + + if err := ensureAccount(cmd, app); err != nil { + return err + } + req := &basecamp.UpdateCardRequest{} if title != "" { req.Title = &title } + var mentionNotice string var html string if content != "" { @@ -1160,7 +1195,7 @@ You can pass either a card ID or a Basecamp URL: } cmd.Flags().StringVarP(&title, "title", "t", "", "New title") - cmd.Flags().StringVarP(&content, "body", "b", "", "New body content") + cmd.Flags().StringVarP(&content, "body", "b", "", "New body content; use - to read from stdin") cmd.Flags().StringVarP(&due, "due", "d", "", "Due date (natural language or YYYY-MM-DD)") cmd.Flags().StringVar(&assignee, "assignee", "", "Assignee ID or name") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") @@ -1169,6 +1204,8 @@ You can pass either a card ID or a Basecamp URL: completer := completion.NewCompleter(nil) _ = cmd.RegisterFlagCompletionFunc("assignee", completer.PeopleNameCompletion()) + allowDash(cmd, "flag:body") + return cmd } @@ -2022,6 +2059,15 @@ func newCardsColumnCreateCmd(project, cardTable *string) *cobra.Command { app := appctx.FromContext(cmd.Context()) + if err := requireNumericID(*cardTable, "card table ID"); err != nil { + return err + } + + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + if err := ensureAccount(cmd, app); err != nil { return err } @@ -2085,7 +2131,9 @@ func newCardsColumnCreateCmd(project, cardTable *string) *cobra.Command { }, } - cmd.Flags().StringVarP(&description, "description", "d", "", "Column description") + cmd.Flags().StringVarP(&description, "description", "d", "", "Column description; use - to read from stdin") + + allowDash(cmd, "flag:description") return cmd } @@ -2108,12 +2156,6 @@ You can pass either a column ID or a Basecamp URL: return noChanges(cmd) } - app := appctx.FromContext(cmd.Context()) - - if err := ensureAccount(cmd, app); err != nil { - return err - } - // Extract ID from URL if provided columnIDStr := extractID(args[0]) columnID, err := strconv.ParseInt(columnIDStr, 10, 64) @@ -2121,6 +2163,18 @@ You can pass either a column ID or a Basecamp URL: return output.ErrUsage("Invalid column ID") } + // Syntactic checks first, then "-", then account and network. + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + + app := appctx.FromContext(cmd.Context()) + + if err := ensureAccount(cmd, app); err != nil { + return err + } + req := &basecamp.UpdateColumnRequest{ Title: title, Description: description, @@ -2138,7 +2192,9 @@ You can pass either a column ID or a Basecamp URL: } cmd.Flags().StringVarP(&title, "title", "t", "", "New title") - cmd.Flags().StringVarP(&description, "description", "d", "", "New description") + cmd.Flags().StringVarP(&description, "description", "d", "", "New description; use - to read from stdin") + + allowDash(cmd, "flag:description") return cmd } diff --git a/internal/commands/chat.go b/internal/commands/chat.go index 47f765353..65e404657 100644 --- a/internal/commands/chat.go +++ b/internal/commands/chat.go @@ -308,15 +308,54 @@ By default, messages are sent as plain text. Use --content-type text/html for rich text (HTML) messages. @mentions (@Name or @First.Last) are resolved automatically and the -content type is promoted to text/html when mentions are present.`, +content type is promoted to text/html when mentions are present. + +Use - as the message argument to read the message from stdin: + printf 'Build is green' | basecamp chat post - --in my-project`, Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) - // Validate user input first, before checking account + // Validate user input first, before checking account. A + // positional and --content are the same input, so supplying both + // is rejected rather than one silently winning — with "-" in + // play, the losing source would discard piped content unread. messageContent := content + argIndex, what := -1, "--content" if len(args) > 0 { + if cmd.Flags().Changed("content") { + return output.ErrUsage("cannot combine a <message> argument with --content") + } messageContent = args[0] + argIndex, what = 0, "<message>" + } + + // An explicitly supplied --room must be numeric; when it is absent + // the room is resolved from the project, which needs the network. + // Check the supplied case here so a malformed room does not cost + // the caller a drained pipe. Same for the content mode, which chat + // update already checks before its own read. + if *chatID != "" { + if _, err := strconv.ParseInt(*chatID, 10, 64); err != nil { + return output.ErrUsage("Invalid chat room ID") + } + } + switch *contentType { + case "", "text/html", "text/plain": + default: + return output.ErrUsage(fmt.Sprintf("unsupported --content-type %q (expected text/html or text/plain)", *contentType)) + } + + // Attachment paths are readable or not regardless of the body, so + // check them before the pipe is drained. + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + + var err error + messageContent, err = resolveContentValue(cmd, messageContent, argIndex, what) + if err != nil { + return err } // Show help when invoked with no message content @@ -332,10 +371,12 @@ content type is promoted to text/html when mentions are present.`, }, } - cmd.Flags().StringVar(&content, "content", "", "Message content") + cmd.Flags().StringVar(&content, "content", "", "Message content; use - to read from stdin") cmd.Flags().StringVar(contentType, "content-type", "", "Content type (text/html for rich text)") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") + allowDash(cmd, "arg:0", "flag:content") + return cmd } @@ -766,18 +807,24 @@ edit to rich text.`, return missingArg(cmd, "<id|url>") } + // A positional and --content are the same input, so supplying + // both is rejected rather than one silently winning — with "-" + // in play, the losing source would discard piped content unread. messageContent := content + argIndex, what := -1, "--content" if len(args) > 1 { + if cmd.Flags().Changed("content") { + return output.ErrUsage("cannot combine a [content] argument with --content") + } messageContent = args[1] + argIndex, what = 1, "[content]" } - if strings.TrimSpace(messageContent) == "" { - return missingArg(cmd, "<content>") - } - - // Validate the content mode before any request or account setup so an - // unknown --content-type fails fast rather than silently sending raw - // bytes (the SDK no longer validates content type for us). + // Validate the content mode before reading stdin, not just before + // the request: an unknown --content-type dooms the invocation, and + // draining the pipe first makes the caller wait on a producer whose + // output is already discarded — or lets a blank pipe answer "stdin + // is empty" instead of naming the bad flag. ct := *contentType switch ct { case "", "text/html", "text/plain": @@ -785,17 +832,14 @@ edit to rich text.`, return output.ErrUsage(fmt.Sprintf("unsupported --content-type %q (expected text/html or text/plain)", ct)) } - if err := ensureAccount(cmd, app); err != nil { - return err - } - - // Resolve the line reference. A bare numeric ID falls through to - // --room/dock resolution; a URL must be a chat-line URL on a trusted - // host so a pasted card/todo/message URL — or a look-alike on an - // attacker-controlled host — can't be misinterpreted into an edit. + // Resolve the line reference before the read: the URL host and + // shape, an explicitly supplied --room, and a bare numeric line ID + // are all decidable from the arguments and the configured base URL, + // so a bad reference must not cost the caller a drained pipe. lineID := args[0] urlChatID := "" urlProjectID := "" + var parsedURL *urlarg.Parsed if urlarg.IsURL(args[0]) { if !hostutil.IsTrustedBasecampHost(args[0], app.Config.BaseURL) { return output.ErrUsage("refusing untrusted host in URL — expected a Basecamp URL") @@ -808,15 +852,41 @@ edit to rich text.`, if parsed == nil || parsed.Type != "lines" || parsed.IsCollection { return output.ErrUsage("expected a chat-line ID or URL of the form /chats/{c}/lines/{l} or /chats/{c}@{l}") } - // Guard against editing in the wrong account: the URL names an - // account, and if it disagrees with the configured one the safe - // move is to stop rather than silently target a different account. - if parsed.AccountID != "" && app.Config.AccountID != "" && parsed.AccountID != app.Config.AccountID { - return output.ErrUsage(fmt.Sprintf("URL account %s does not match the configured account %s", parsed.AccountID, app.Config.AccountID)) - } lineID = parsed.RecordingID urlChatID = parsed.CampfireID urlProjectID = parsed.ProjectID + parsedURL = parsed + } + if _, err := strconv.ParseInt(lineID, 10, 64); err != nil { + return output.ErrUsage("Invalid chat line ID") + } + if *chatID != "" { + if _, err := strconv.ParseInt(*chatID, 10, 64); err != nil { + return output.ErrUsage("Invalid chat room ID") + } + } + + var contentErr error + messageContent, contentErr = resolveContentValue(cmd, messageContent, argIndex, what) + if contentErr != nil { + return contentErr + } + + if strings.TrimSpace(messageContent) == "" { + return missingArg(cmd, "<content>") + } + + if err := ensureAccount(cmd, app); err != nil { + return err + } + + // The URL's host and shape were settled before the read; only the + // account comparison had to wait for a resolved account. Editing in + // the wrong account is worth stopping for rather than silently + // targeting a different one. + if parsedURL != nil && parsedURL.AccountID != "" && app.Config.AccountID != "" && + parsedURL.AccountID != app.Config.AccountID { + return output.ErrUsage(fmt.Sprintf("URL account %s does not match the configured account %s", parsedURL.AccountID, app.Config.AccountID)) } // Resolve the chat (campfire) ID, and a project only when needed. The @@ -958,9 +1028,11 @@ edit to rich text.`, }, } - cmd.Flags().StringVar(&content, "content", "", "New message content") + cmd.Flags().StringVar(&content, "content", "", "New message content; use - to read from stdin") cmd.Flags().StringVar(contentType, "content-type", "", "Input handling: text/html (supply HTML) or text/plain (verbatim); applied locally, edits always render as rich text") + allowDash(cmd, "arg:1", "flag:content") + return cmd } diff --git a/internal/commands/chat_test.go b/internal/commands/chat_test.go index f58cafd4f..8d126f7d5 100644 --- a/internal/commands/chat_test.go +++ b/internal/commands/chat_test.go @@ -1796,3 +1796,19 @@ func TestChatRoomShorthandFlag(t *testing.T) { require.Len(t, envelope.Data, 1) assert.Equal(t, "Engineering", envelope.Data[0]["title"]) } + +// TestChatPostRejectsPositionalWithContentFlag verifies that a positional +// message and --content are rejected together instead of one silently +// winning — with "-" in play, the losing source would discard piped +// content unread. +func TestChatPostRejectsPositionalWithContentFlag(t *testing.T) { + app := &appctx.App{Config: &config.Config{}} + + err := executeChatCommand(NewChatCmd(), app, "post", "literal", "--content", "other") + require.Error(t, err) + require.Contains(t, err.Error(), "cannot combine") + + err = executeChatCommand(NewChatCmd(), app, "update", "123", "literal", "--content", "other") + require.Error(t, err) + require.Contains(t, err.Error(), "cannot combine") +} diff --git a/internal/commands/checkins.go b/internal/commands/checkins.go index ee4a327fa..fff393ae8 100644 --- a/internal/commands/checkins.go +++ b/internal/commands/checkins.go @@ -1269,7 +1269,23 @@ func newCheckinsAnswerCreateCmd(project *string) *cobra.Command { } questionID := args[0] - content := strings.Join(args[1:], " ") + + // Decidable from the arguments alone, so it precedes the read; the + // project-resolution path parses it again once the project is known. + if _, err := strconv.ParseInt(questionID, 10, 64); err != nil { + return output.ErrUsage("Invalid question ID") + } + + // Attachment paths are readable or not regardless of the body, so + // check them before the pipe is drained. + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + + content, err := resolveContentArg(cmd, args[1:], 1) + if err != nil { + return err + } app := appctx.FromContext(cmd.Context()) @@ -1359,6 +1375,8 @@ func newCheckinsAnswerCreateCmd(project *string) *cobra.Command { cmd.Flags().StringVar(&groupOn, "date", "", "Date to group answer (ISO 8601, e.g., 2024-01-22; defaults to today)") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") + allowDash(cmd, "arg:1+") + return cmd } @@ -1381,7 +1399,16 @@ You can pass either an answer ID or a Basecamp URL: // Extract ID and project from URL if provided answerIDStr, urlProjectID := extractWithProject(args[0]) - content := strings.Join(args[1:], " ") + // Decidable from the arguments alone, so it precedes the read; the + // project-resolution path parses it again once the project is known. + if _, err := strconv.ParseInt(answerIDStr, 10, 64); err != nil { + return output.ErrUsage("Invalid answer ID") + } + + content, err := resolveContentArg(cmd, args[1:], 1) + if err != nil { + return err + } app := appctx.FromContext(cmd.Context()) @@ -1461,6 +1488,8 @@ You can pass either an answer ID or a Basecamp URL: }, } + allowDash(cmd, "arg:1+") + return cmd } diff --git a/internal/commands/commands_test.go b/internal/commands/commands_test.go index f9a8dc975..85336c486 100644 --- a/internal/commands/commands_test.go +++ b/internal/commands/commands_test.go @@ -119,6 +119,7 @@ func buildRootWithAllCommands() *cobra.Command { root.AddCommand(commands.NewTUICmd()) root.AddCommand(commands.NewProfileCmd()) root.AddCommand(commands.NewBonfireCmd()) + commands.InstallDashGuard(root) root.InitDefaultHelpCmd() return root } diff --git a/internal/commands/comment.go b/internal/commands/comment.go index 05ec36564..16348cb5d 100644 --- a/internal/commands/comment.go +++ b/internal/commands/comment.go @@ -5,7 +5,6 @@ import ( "encoding/json" "errors" "fmt" - "io" "net/http" "os" "sort" @@ -23,6 +22,7 @@ import ( "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/stdinarg" "github.com/basecamp/basecamp-cli/internal/urlarg" ) @@ -1049,7 +1049,19 @@ as backslash-n.`, return missingArg(cmd, "<content>") } - content, err := contentArgOrStdin(cmd, args[1:]) + // Extract comment ID from URL if provided + // Uses extractCommentWithProject to prefer CommentID from URL fragments + commentIDStr, _ := extractCommentWithProject(args[0]) + + commentID, err := strconv.ParseInt(commentIDStr, 10, 64) + if err != nil { + return output.ErrUsage("Invalid comment ID") + } + + // Syntactic checks first, then "-", then account and network: a + // malformed ID is answered without waiting on the producer, and a + // blank pipe cannot mask it. + content, err := resolveContentArg(cmd, args[1:], 1) if err != nil { return err } @@ -1062,15 +1074,6 @@ as backslash-n.`, return err } - // Extract comment ID from URL if provided - // Uses extractCommentWithProject to prefer CommentID from URL fragments - commentIDStr, _ := extractCommentWithProject(args[0]) - - commentID, err := strconv.ParseInt(commentIDStr, 10, 64) - if err != nil { - return output.ErrUsage("Invalid comment ID") - } - // Convert Markdown content to HTML for Basecamp's rich text fields html := richtext.MarkdownToHTML(content) @@ -1114,6 +1117,8 @@ as backslash-n.`, }, } + allowDash(cmd, "arg:1+") + return cmd } @@ -1132,8 +1137,8 @@ Comma-separated IDs add the same comment to multiple items: basecamp comments create 789,012,345 "Looks good!" basecamp comments create https://3.basecamp.com/123/buckets/456/todos/789 "Looks good!" -Content can also be piped from stdin: - printf 'Looks good!' | basecamp comments create 789 +Content can be piped from stdin by passing - as the content argument: + printf 'Looks good!' | basecamp comments create 789 - Content supports Markdown and @mentions (@Name or @First.Last): basecamp comments create 789 "Hey @Jane.Smith, **please review**" @@ -1161,10 +1166,16 @@ busybox-ash) it posts a literal leading $ and keeps \n as backslash-n: return output.ErrUsage("cannot combine --edit and positional content") } + // Attachment paths are readable or not regardless of the body, so + // check them before the pipe is drained. + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + var content string if len(args) > 1 { var err error - content, err = contentArgOrStdin(cmd, args[1:]) + content, err = resolveContentArg(cmd, args[1:], 1) if err != nil { return err } @@ -1180,21 +1191,19 @@ busybox-ash) it posts a literal leading $ and keeps \n as backslash-n: } } - if !edit && strings.TrimSpace(content) == "" { - stdinContent, hasPipedStdin, err := readPipedStdin(cmd) - if err != nil { - return err - } - if hasPipedStdin { - content = stdinContent - } - } - - // Show help when invoked with no content; keep error if editor was opened + // Show help when invoked with no content; keep error if editor was opened. + // A pipe without "-" is deliberately not consumed: teach the explicit + // placeholder instead of silently reading stdin. if strings.TrimSpace(content) == "" { if edit { return output.ErrUsage("Comment content required") } + if stdinarg.IsPiped(cmd.InOrStdin()) { + return output.ErrUsageHint( + "<content> required", + fmt.Sprintf(`To read the piped stdin, pass "-" as the content: %s %s -`, cmd.CommandPath(), recordingArg), + ) + } return missingArg(cmd, "<content>") } @@ -1336,16 +1345,7 @@ busybox-ash) it posts a literal leading $ and keeps \n as backslash-n: cmd.Flags().BoolVar(&edit, "edit", false, "Open $EDITOR to compose content") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") - return cmd -} + allowDash(cmd, "arg:1+") -func contentArgOrStdin(cmd *cobra.Command, args []string) (string, error) { - if len(args) == 1 && args[0] == "-" { - b, err := io.ReadAll(cmd.InOrStdin()) - if err != nil { - return "", output.ErrUsage(fmt.Sprintf("failed to read content from stdin: %v", err)) - } - return string(b), nil - } - return strings.Join(args, " "), nil + return cmd } diff --git a/internal/commands/comment_test.go b/internal/commands/comment_test.go index a44f8c4f5..9c40d2203 100644 --- a/internal/commands/comment_test.go +++ b/internal/commands/comment_test.go @@ -84,23 +84,26 @@ func TestCommentsUpdateRejectsEmptyDashContent(t *testing.T) { var outErr *output.Error require.True(t, errors.As(err, &outErr), "expected *output.Error, got %T: %v", err, err) assert.Equal(t, output.CodeUsage, outErr.Code) - assert.Equal(t, "<content> required", outErr.Message) + assert.Equal(t, "stdin for <content> is empty", outErr.Message) assert.Empty(t, transport.capturedBodies) } -func TestCommentsCreateReadsContentFromStdin(t *testing.T) { +// A pipe without "-" is no longer consumed implicitly: the error teaches the +// explicit placeholder instead, and nothing reaches the server. +func TestCommentsCreateBarePipeErrorsWithDashHint(t *testing.T) { transport := &mockCommentWriteTransport{} app, _ := setupCommentsWriteTestApp(t, transport) + app.Flags.JSON = true cmd := NewCommentsCmd() cmd.SetIn(strings.NewReader("hello from stdin")) err := executeCommand(cmd, app, "create", "123") - require.NoError(t, err) - require.Len(t, transport.capturedBodies, 1) - - var body map[string]any - require.NoError(t, json.Unmarshal(transport.capturedBodies[0], &body)) - assert.Equal(t, "<p>hello from stdin</p>", body["content"]) + require.Error(t, err) + var outErr *output.Error + require.True(t, errors.As(err, &outErr)) + assert.Equal(t, output.CodeUsage, outErr.Code) + assert.Contains(t, outErr.Hint, `"-"`) + assert.Empty(t, transport.capturedBodies) } func TestCommentsCreatePrefersPositionalContentOverStdin(t *testing.T) { @@ -140,7 +143,7 @@ func TestCommentsCreateMissingContentReturnsUsageBeforeAccountResolution(t *test assert.NotContains(t, err.Error(), "account") } -func TestReadPipedStdinIgnoresUnreadableStdin(t *testing.T) { +func TestReadStdinContentUnreadableStdinIsUsageError(t *testing.T) { r, w, err := os.Pipe() require.NoError(t, err) require.NoError(t, r.Close()) @@ -148,10 +151,12 @@ func TestReadPipedStdinIgnoresUnreadableStdin(t *testing.T) { cmd := newCommentsCreateCmd() cmd.SetIn(r) - content, hasPipedStdin, err := readPipedStdin(cmd) - require.NoError(t, err) + content, err := readStdinContent(cmd, "<content>") + require.Error(t, err) + var outErr *output.Error + require.True(t, errors.As(err, &outErr)) + assert.Equal(t, output.CodeUsage, outErr.Code) assert.Empty(t, content) - assert.False(t, hasPipedStdin) } func setupCommentsWriteTestApp(t *testing.T, transport http.RoundTripper) (*appctx.App, *bytes.Buffer) { diff --git a/internal/commands/dash_guard_test.go b/internal/commands/dash_guard_test.go new file mode 100644 index 000000000..26c39dbf4 --- /dev/null +++ b/internal/commands/dash_guard_test.go @@ -0,0 +1,444 @@ +package commands + +import ( + "errors" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/stdinarg" +) + +// dashProbe records whether a guarded command's original RunE ran, and with +// which args — the passthrough half of the guard's contract. +type dashProbe struct { + ran bool + args []string +} + +func newDashProbeCmd(probe *dashProbe, tokens ...string) *cobra.Command { + cmd := &cobra.Command{ + Use: "probe <name>", + RunE: func(cmd *cobra.Command, args []string) error { + probe.ran = true + probe.args = args + return nil + }, + } + cmd.Flags().String("title", "", "") + cmd.Flags().String("out", "", "") + cmd.Flags().StringArray("attach", nil, "") + if len(tokens) > 0 { + allowDash(cmd, tokens...) + } + InstallDashGuard(cmd) + cmd.SetOut(&strings.Builder{}) + cmd.SetErr(&strings.Builder{}) + return cmd +} + +func TestDashGuardRejectsUnlistedPositionalWhenPiped(t *testing.T) { + app, _ := setupTestApp(t) + app.Flags.JSON = true + + cmd := NewProjectsCmd() + InstallDashGuard(cmd) + cmd.SetIn(strings.NewReader("piped")) + + err := executeCommand(cmd, app, "create", "-") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "<name>") + assert.Contains(t, outErr.Hint, "--") + assert.Contains(t, outErr.Hint, "--description", "hint should point at where stdin is accepted") +} + +func TestDashGuardRejectsUnlistedFlagWhenPiped(t *testing.T) { + app, _ := setupTestApp(t) + app.Flags.JSON = true + + cmd := NewTodosCmd() + InstallDashGuard(cmd) + cmd.SetIn(strings.NewReader("piped")) + + err := executeCommand(cmd, app, "update", "1", "--title", "-") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "--title") + // -- doesn't escape flag values, so the hint must not claim it does. + assert.NotContains(t, outErr.Hint, "-- separator") + assert.Contains(t, outErr.Hint, "without piped stdin") +} + +// The guard runs at Args-validation time: before the command's own Args +// check, PreRunE, and required-flag validation, so the stray-dash error is +// what the caller sees instead of a competing usage error — and no pre-run +// side effect happens first. +func TestDashGuardFiresBeforeArgsPreRunAndRequiredFlags(t *testing.T) { + preRunRan := false + cmd := &cobra.Command{ + Use: "probe <name> <other>", + Args: cobra.ExactArgs(2), + PreRunE: func(cmd *cobra.Command, args []string) error { + preRunRan = true + return nil + }, + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + cmd.Flags().String("title", "", "") + require.NoError(t, cmd.MarkFlagRequired("title")) + InstallDashGuard(cmd) + cmd.SetIn(strings.NewReader("piped")) + cmd.SetOut(&strings.Builder{}) + cmd.SetErr(&strings.Builder{}) + cmd.SetArgs([]string{"-"}) // one arg: ExactArgs(2) and the missing --title would both error later + + err := cmd.Execute() + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "<name>") + assert.False(t, preRunRan, "guard must fire before PreRunE") +} + +// Alias flags share one backing value; a value set through both spellings is +// one logical input, not two. +func TestDashGuardAliasFlagsCountOnce(t *testing.T) { + newAliasCmd := func(probe *dashProbe) *cobra.Command { + var description string + cmd := &cobra.Command{ + Use: "probe", + RunE: func(cmd *cobra.Command, args []string) error { + probe.ran = true + probe.args = []string{description} + return nil + }, + } + cmd.Flags().StringVar(&description, "description", "", "") + cmd.Flags().StringVar(&description, "desc", "", "") + allowDash(cmd, "flag:description", "flag:desc") + InstallDashGuard(cmd) + cmd.SetOut(&strings.Builder{}) + cmd.SetErr(&strings.Builder{}) + return cmd + } + + // Dash last: the merged value is "-", one allowed stdin input. + probe := &dashProbe{} + cmd := newAliasCmd(probe) + cmd.SetIn(strings.NewReader("piped")) + cmd.SetArgs([]string{"--description", "old", "--desc", "-"}) + require.NoError(t, cmd.Execute()) + assert.Equal(t, []string{"-"}, probe.args) + + // Dash first: the literal value wins, no dash in play at all. + probe = &dashProbe{} + cmd = newAliasCmd(probe) + cmd.SetIn(strings.NewReader("piped")) + cmd.SetArgs([]string{"--desc", "-", "--description", "old"}) + require.NoError(t, cmd.Execute()) + assert.Equal(t, []string{"old"}, probe.args) +} + +func TestDashGuardPassesLiteralDashOnTTY(t *testing.T) { + probe := &dashProbe{} + cmd := newDashProbeCmd(probe) + devNullStdin(t, cmd) + cmd.SetArgs([]string{"-", "--title", "-"}) + + require.NoError(t, cmd.Execute()) + assert.True(t, probe.ran) + assert.Equal(t, []string{"-"}, probe.args) +} + +func TestDashGuardExemptsOutFlag(t *testing.T) { + probe := &dashProbe{} + cmd := newDashProbeCmd(probe, "flag:out") + cmd.SetIn(strings.NewReader("piped")) + cmd.SetArgs([]string{"name", "--out", "-"}) + + require.NoError(t, cmd.Execute()) + assert.True(t, probe.ran) +} + +func TestDashGuardRejectsTwoAllowedDashes(t *testing.T) { + app, _ := setupTestApp(t) + app.Flags.JSON = true + + cmd := NewChatCmd() + InstallDashGuard(cmd) + cmd.SetIn(strings.NewReader("piped")) + + err := executeCommand(cmd, app, "post", "-", "--content", "-") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "only one input") +} + +// Two allowed dashes can never both be satisfied, even on a TTY. +func TestDashGuardRejectsTwoAllowedDashesOnTTY(t *testing.T) { + app, _ := setupTestApp(t) + app.Flags.JSON = true + + cmd := NewChatCmd() + InstallDashGuard(cmd) + devNullStdin(t, cmd) + + err := executeCommand(cmd, app, "post", "-", "--content", "-") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "only one input") +} + +func TestDashGuardRejectsUnlistedAttachAlongsideAllowedBody(t *testing.T) { + app, _ := setupTestApp(t) + app.Flags.JSON = true + + cmd := NewMessagesCmd() + InstallDashGuard(cmd) + cmd.SetIn(strings.NewReader("piped")) + + err := executeCommand(cmd, app, "create", "title", "-", "--attach", "-") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "--attach") +} + +func TestDashGuardSeparatorEscapesLiteralDash(t *testing.T) { + probe := &dashProbe{} + cmd := newDashProbeCmd(probe) + cmd.SetIn(strings.NewReader("piped")) + cmd.SetArgs([]string{"--", "-"}) + + require.NoError(t, cmd.Execute()) + assert.True(t, probe.ran) + assert.Equal(t, []string{"-"}, probe.args) +} + +// The download commands carry the --out exemption so "-" (stdout) never trips +// the stdin guard. +func TestDownloadCommandsExemptOutFlag(t *testing.T) { + for _, cmd := range []*cobra.Command{NewAttachmentsCmd(), NewFilesCmd()} { + download := findSubcommand(cmd, "download") + require.NotNil(t, download, "%s download not found", cmd.Name()) + allow := stdinarg.ParseAllow(download.Annotations[stdinarg.AnnotationAllowDash]) + assert.True(t, allow.Flag("out"), "%s download should exempt --out", cmd.Name()) + } +} + +// Parsed state keeps only the merged value, never which spelling the caller +// typed — so naming one alias is a coin flip that reports "--in" for a caller +// who wrote "--project -". Name the whole group instead. +func TestDashGuardNamesTheWholeAliasGroup(t *testing.T) { + app, _ := setupTestApp(t) + app.Flags.JSON = true + + // --project/--in are two spellings of one persistent value on the group. + cmd := NewCardsCmd() + InstallDashGuard(cmd) + cmd.SetIn(strings.NewReader("piped")) + + err := executeCommand(cmd, app, "create", "Title", "--in", "old", "--project", "-") + outErr := requireUsageErr(t, err) + // pflag visits flags in sorted order, so the group label is stable. + assert.Contains(t, outErr.Message, "--in/--project") +} + +// A flag with no alias still reads as a single spelling. +func TestDashGuardNamesASoloFlagPlainly(t *testing.T) { + app, _ := setupTestApp(t) + app.Flags.JSON = true + + cmd := NewTodosCmd() + InstallDashGuard(cmd) + cmd.SetIn(strings.NewReader("piped")) + + err := executeCommand(cmd, app, "update", "1", "--title", "-") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "--title") + assert.NotContains(t, outErr.Message, "/--") +} + +// The root keeps nil Args so cobra's legacyArgs still rejects unknown +// subcommands, so its guard hangs off the front of its persistent pre-run +// instead. All four root behaviors have to survive together. +func TestDashGuardOnRootPreservesUnknownCommandHandling(t *testing.T) { + newRoot := func(probe *dashProbe) *cobra.Command { + root := &cobra.Command{ + Use: "basecamp", + RunE: func(cmd *cobra.Command, args []string) error { + probe.ran = true + probe.args = args + return nil + }, + } + root.AddCommand(&cobra.Command{ + Use: "todos", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + }) + InstallDashGuard(root) + require.Nil(t, root.Args, "root Args must stay nil for legacyArgs") + root.SetOut(&strings.Builder{}) + root.SetErr(&strings.Builder{}) + return root + } + + t.Run("piped dash is a usage error", func(t *testing.T) { + probe := &dashProbe{} + root := newRoot(probe) + root.SetIn(strings.NewReader("piped")) + root.SetArgs([]string{"-"}) + + outErr := requireUsageErr(t, root.Execute()) + assert.Contains(t, outErr.Message, "does not read stdin") + assert.False(t, probe.ran, "quick-start must not run on a stray dash") + }) + + t.Run("separator keeps the dash literal", func(t *testing.T) { + probe := &dashProbe{} + root := newRoot(probe) + root.SetIn(strings.NewReader("piped")) + root.SetArgs([]string{"--", "-"}) + + require.NoError(t, root.Execute()) + assert.True(t, probe.ran) + assert.Equal(t, []string{"-"}, probe.args) + }) + + t.Run("unknown subcommand still errors", func(t *testing.T) { + probe := &dashProbe{} + root := newRoot(probe) + root.SetIn(strings.NewReader("piped")) + root.SetArgs([]string{"unknowncmd"}) + + err := root.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown command") + assert.False(t, probe.ran) + }) + + t.Run("bare invocation still runs", func(t *testing.T) { + probe := &dashProbe{} + root := newRoot(probe) + root.SetIn(strings.NewReader("piped")) + root.SetArgs(nil) + + require.NoError(t, root.Execute()) + assert.True(t, probe.ran) + }) +} + +// The root guard runs at the front of the persistent pre-run, so nothing the +// caller did not ask about — config loading, profile resolution, --jq +// validation — can answer ahead of the stray dash. +func TestDashGuardOnRootPrecedesPersistentPreRun(t *testing.T) { + preRunRan := false + root := &cobra.Command{ + Use: "basecamp", + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + preRunRan = true + return errors.New("pre-run would have answered first") + }, + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + root.AddCommand(&cobra.Command{ + Use: "todos", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + }) + InstallDashGuard(root) + root.SetIn(strings.NewReader("piped")) + root.SetOut(&strings.Builder{}) + root.SetErr(&strings.Builder{}) + root.SetArgs([]string{"-"}) + + outErr := requireUsageErr(t, root.Execute()) + assert.Contains(t, outErr.Message, "does not read stdin") + assert.False(t, preRunRan, "the guard must precede the root's pre-run work") +} + +// Subcommands inherit the root's persistent pre-run; the guard there must not +// double-fire for them, since they are already guarded at Args-validation time. +func TestDashGuardOnRootDoesNotAffectSubcommands(t *testing.T) { + preRunRan := false + var got []string + root := &cobra.Command{ + Use: "basecamp", + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + preRunRan = true + return nil + }, + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + sub := &cobra.Command{ + Use: "notes", + RunE: func(cmd *cobra.Command, args []string) error { + got = args + return nil + }, + } + allowDash(sub, "arg:0") + root.AddCommand(sub) + InstallDashGuard(root) + root.SetIn(strings.NewReader("piped")) + root.SetOut(&strings.Builder{}) + root.SetErr(&strings.Builder{}) + root.SetArgs([]string{"notes", "-"}) + + require.NoError(t, root.Execute()) + assert.True(t, preRunRan, "the inherited pre-run still runs for subcommands") + assert.Equal(t, []string{"-"}, got, "an allowed dash reaches the subcommand") +} + +// Tier 2 stops a stray "-" landing as content, so cobra's generated commands +// are exempt: they perform no Basecamp content write. For the completion +// commands the exemption is load-bearing, not merely harmless — the shell +// passes the word being completed as an argument, so "todos create -<TAB>" +// runs "__complete todos create -". Guarding that would break completion for +// every flag in the CLI. +func TestDashGuardExemptsGeneratedMetaCommands(t *testing.T) { + root := &cobra.Command{ + Use: "basecamp", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + root.AddCommand(&cobra.Command{ + Use: "todos", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + }) + InstallDashGuard(root) + + help := findSubcommand(root, "help") + require.NotNil(t, help, "InstallDashGuard must materialize the help command so the exemption is explicit") + assert.True(t, isMetaCommand(help)) + + for _, name := range []string{cobra.ShellCompRequestCmd, cobra.ShellCompNoDescRequestCmd} { + assert.True(t, isMetaCommand(&cobra.Command{Use: name}), "%s must stay unguarded", name) + } + + // A piped "help -" resolves like any unknown topic rather than erroring. + root.SetIn(strings.NewReader("piped")) + root.SetOut(&strings.Builder{}) + root.SetErr(&strings.Builder{}) + root.SetArgs([]string{"help", "-"}) + assert.NoError(t, root.Execute()) +} + +// Setting PersistentPreRunE shadows a non-E hook cobra would otherwise run in +// its place. Production uses the E form, so this only guards the refactor. +func TestDashGuardOnRootKeepsNonErrorPersistentPreRun(t *testing.T) { + preRunRan := false + root := &cobra.Command{ + Use: "basecamp", + PersistentPreRun: func(cmd *cobra.Command, args []string) { + preRunRan = true + }, + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + root.AddCommand(&cobra.Command{ + Use: "todos", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + }) + InstallDashGuard(root) + root.SetIn(strings.NewReader("piped")) + root.SetOut(&strings.Builder{}) + root.SetErr(&strings.Builder{}) + root.SetArgs(nil) + + require.NoError(t, root.Execute()) + assert.True(t, preRunRan, "the non-error pre-run must still run") +} diff --git a/internal/commands/files.go b/internal/commands/files.go index 3f543348b..b00a9f417 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -887,13 +887,26 @@ as an upload in the target folder (vault).`, basecamp uploads create ./photo.png --folder 123 --description "Site photo"`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - return runUploadFile(cmd, *project, *vaultID, args[0], description, visibleToClients) + filePath, err := validateUploadPath(args[0]) + if err != nil { + return err + } + if err := requireNumericID(*vaultID, "folder ID"); err != nil { + return err + } + description, err = resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + return runUploadFile(cmd, *project, *vaultID, filePath, description, visibleToClients) }, } - cmd.Flags().StringVar(&description, "description", "", "Upload description (Markdown)") + cmd.Flags().StringVar(&description, "description", "", "Upload description (Markdown); use - to read from stdin") cmd.Flags().BoolVar(&visibleToClients, "visible-to-clients", false, "Make the upload visible to clients (root Docs & Files folder only; a nested folder inherits its folder's visibility). Omit for the server default.") + allowDash(cmd, "flag:description") + return cmd } @@ -915,7 +928,18 @@ attachment and then created as an upload in the target folder.`, basecamp upload ./photo.png --folder 123 --description "Site photo"`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - return runUploadFile(cmd, project, vaultID, args[0], description, visibleToClients) + filePath, err := validateUploadPath(args[0]) + if err != nil { + return err + } + if err := requireNumericID(vaultID, "folder ID"); err != nil { + return err + } + description, err = resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + return runUploadFile(cmd, project, vaultID, filePath, description, visibleToClients) }, } @@ -923,9 +947,11 @@ attachment and then created as an upload in the target folder.`, cmd.Flags().StringVar(&project, "in", "", "Project ID (alias for --project)") cmd.Flags().StringVar(&vaultID, "vault", "", "Folder ID (default: root)") cmd.Flags().StringVar(&vaultID, "folder", "", "Folder ID (alias for --vault)") - cmd.Flags().StringVar(&description, "description", "", "Upload description (Markdown)") + cmd.Flags().StringVar(&description, "description", "", "Upload description (Markdown); use - to read from stdin") cmd.Flags().BoolVar(&visibleToClients, "visible-to-clients", false, "Make the upload visible to clients (root Docs & Files folder only; a nested folder inherits its folder's visibility). Omit for the server default.") + allowDash(cmd, "flag:description") + return cmd } @@ -956,6 +982,19 @@ func resolveVaultClientVisibility(cmd *cobra.Command, app *appctx.App, vaultFlag return &value, nil } +// validateUploadPath normalizes a drag/paste path and checks the file is +// readable. It needs no account and no network, so the upload commands run it +// before resolving a "-" description: a missing file should not cost the caller +// a drained pipe, and a blank pipe must not answer "stdin is empty" instead of +// naming the unreadable file. +func validateUploadPath(filePath string) (string, error) { + filePath = richtext.NormalizeDragPath(filePath) + if err := richtext.ValidateFile(filePath); err != nil { + return "", fmt.Errorf("%s: %w", filePath, err) + } + return filePath, nil +} + func runUploadFile(cmd *cobra.Command, project, vaultID, filePath, description string, visibleToClients bool) error { app := appctx.FromContext(cmd.Context()) @@ -963,10 +1002,12 @@ func runUploadFile(cmd *cobra.Command, project, vaultID, filePath, description s return err } - // Normalize drag/paste paths and validate - filePath = richtext.NormalizeDragPath(filePath) - if err := richtext.ValidateFile(filePath); err != nil { - return fmt.Errorf("%s: %w", filePath, err) + // Normalize drag/paste paths and validate. Callers that read stdin run + // this first (see validateUploadPath); repeating it is idempotent and + // keeps this function correct on its own. + filePath, err := validateUploadPath(filePath) + if err != nil { + return err } // Resolve project, with interactive fallback @@ -1214,6 +1255,13 @@ func newDocsCreateCmd(project, vaultID *string) *cobra.Command { cmd := &cobra.Command{ Use: "create <title> [content]", Short: "Create a new document", + Long: `Create a new document in a project's Docs & Files area. + +Use - as the content argument to read the document body from stdin: + basecamp docs documents create "Title" - --in my-project < body.md`, + // Bounded so a stray third token is a usage error rather than being + // silently dropped after "-" has already drained stdin. + Args: cobra.MaximumNArgs(2), RunE: func(cmd *cobra.Command, args []string) error { // Show help when invoked with no arguments if len(args) == 0 { @@ -1222,14 +1270,34 @@ func newDocsCreateCmd(project, vaultID *string) *cobra.Command { title := args[0] - app := appctx.FromContext(cmd.Context()) + // Resolve "-" before any account or network work, so a bad stdin + // gets the stdin error rather than "--account is required". + if err := rejectSubscribeConflict(cmd.Flags().Changed("subscribe"), noSubscribe); err != nil { + return err + } + if err := requireNumericID(*vaultID, "folder ID"); err != nil { + return err + } - if err := ensureAccount(cmd, app); err != nil { + // Attachment paths are readable or not regardless of the body, so + // check them before the pipe is drained. + if err := validateAttachPaths(attachFiles); err != nil { return err } + content := "" if len(args) > 1 { - content = args[1] + var contentErr error + content, contentErr = resolveContentValue(cmd, args[1], 1, "[content]") + if contentErr != nil { + return contentErr + } + } + + app := appctx.FromContext(cmd.Context()) + + if err := ensureAccount(cmd, app); err != nil { + return err } // Resolve subscription flags before project (fail fast on bad input) @@ -1339,6 +1407,8 @@ func newDocsCreateCmd(project, vaultID *string) *cobra.Command { cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") cmd.Flags().BoolVar(&visibleToClients, "visible-to-clients", false, "Make the document visible to clients (root Docs & Files folder only; a nested folder inherits its folder's visibility). Omit for the server default.") + allowDash(cmd, "arg:1") + return cmd } @@ -1693,48 +1763,66 @@ You can pass either an upload ID or a Basecamp URL: Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) - if err := ensureAccount(cmd, app); err != nil { - return err + + uploadIDStr := extractID(args[0]) + uploadID, err := strconv.ParseInt(uploadIDStr, 10, 64) + if err != nil || uploadID <= 0 { + return output.ErrUsage("Invalid upload ID") } - // A pasted URL names its own account; refuse one that isn't the - // session's before anything is staged — extractID keeps only the - // numeric ID, which would silently retarget the configured - // account's same-numbered upload on a mutating request. - // A URL-shaped argument must live on a trusted Basecamp host: - // the URL router is host-agnostic, so a look-alike on an - // attacker-controlled host would otherwise pass the identity - // checks below and retarget the configured account's upload — - // the confused-deputy case hostutil exists to prevent. - if urlarg.IsURL(args[0]) && !hostutil.IsTrustedBasecampHost(args[0], app.Config.BaseURL) { - return output.ErrUsage("refusing untrusted host in URL — expected a Basecamp URL") - } - - if parsed := urlarg.Parse(args[0]); parsed != nil { - if parsed.AccountID != "" && parsed.AccountID != app.Config.AccountID { - return output.ErrUsage(fmt.Sprintf("URL is for account %s, but this session uses account %s", parsed.AccountID, app.Config.AccountID)) + filePath := richtext.NormalizeDragPath(args[1]) + if err := richtext.ValidateFile(filePath); err != nil { + return fmt.Errorf("%s: %w", filePath, err) + } + + // A URL-shaped argument must live on a trusted Basecamp host: the + // URL router is host-agnostic, so a look-alike on an + // attacker-controlled host would otherwise pass the identity checks + // and retarget the configured account's upload — the confused-deputy + // case hostutil exists to prevent. That, and whether the URL names a + // single upload at all, need only the configured base URL, so they + // run before the read; only the account comparison waits. + var parsedURL *urlarg.Parsed + if urlarg.IsURL(args[0]) { + if !hostutil.IsTrustedBasecampHost(args[0], app.Config.BaseURL) { + return output.ErrUsage("refusing untrusted host in URL — expected a Basecamp URL") } - if parsed.Type != "uploads" { - return output.ErrUsage(fmt.Sprintf("URL identifies a %s recording, not an upload", parsed.Type)) + parsedURL = urlarg.Parse(args[0]) + } + if parsedURL != nil { + if parsedURL.Type != "uploads" { + return output.ErrUsage(fmt.Sprintf("URL identifies a %s recording, not an upload", parsedURL.Type)) } // A collection URL (/vaults/456/uploads, /buckets/456/uploads) // also parses as type "uploads", but its extracted ID is the // PARENT's — the identity predicate needs the recording half // too, or extractID retargets a same-numbered upload. - if parsed.IsCollection || parsed.RecordingID == "" { + if parsedURL.IsCollection || parsedURL.RecordingID == "" { return output.ErrUsage("URL identifies an uploads listing, not a single upload") } } - uploadIDStr := extractID(args[0]) - uploadID, err := strconv.ParseInt(uploadIDStr, 10, 64) - if err != nil || uploadID <= 0 { - return output.ErrUsage("Invalid upload ID") + // Syntactic checks first, then "-", then account and network: a + // malformed ID, missing file or foreign host is answered without + // waiting on the producer. The account-identity checks below need + // the session account, so they necessarily follow. Only an exact + // "-" reads stdin; --description "" stays the clear idiom. + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err } - filePath := richtext.NormalizeDragPath(args[1]) - if err := richtext.ValidateFile(filePath); err != nil { - return fmt.Errorf("%s: %w", filePath, err) + if err := ensureAccount(cmd, app); err != nil { + return err + } + + // A pasted URL names its own account; refuse one that isn't the + // session's before anything is staged — extractID keeps only the + // numeric ID, which would silently retarget the configured + // account's same-numbered upload on a mutating request. Host and + // shape were settled before the read; only this needed the account. + if parsedURL != nil && parsedURL.AccountID != "" && parsedURL.AccountID != app.Config.AccountID { + return output.ErrUsage(fmt.Sprintf("URL is for account %s, but this session uses account %s", parsedURL.AccountID, app.Config.AccountID)) } // Resolve the description first: its local-image references can @@ -1805,9 +1893,11 @@ You can pass either an upload ID or a Basecamp URL: }, } - cmd.Flags().StringVar(&description, "description", "", "New description (Markdown); omit to carry the current one forward") + cmd.Flags().StringVar(&description, "description", "", "New description (Markdown); omit to carry the current one forward; use - to read from stdin") cmd.Flags().StringVar(&baseName, "base-name", "", "Rename the file (without extension); omit to keep the uploaded file's name") + allowDash(cmd, "flag:description") + return cmd } @@ -1866,6 +1956,47 @@ You can pass either an item ID or a Basecamp URL: Annotations: map[string]string{"agent_notes": "Document updates preserve untouched title/content by fetching current state first because BC3 rebuilds documents from permitted params on PUT; explicit clears via --title \"\"/--content \"\" work because the SDK strips empty strings to absent fields, which the controller then nulls. Upload/vault updates do not clear by omission, so empty-valued flags are rejected CLI-side."}, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + // Extract ID and project from URL if provided + itemIDStr, urlProjectID := extractWithProject(args[0]) + + itemID, err := strconv.ParseInt(itemIDStr, 10, 64) + if err != nil { + return output.ErrUsage("Invalid item ID") + } + + // Syntactic checks first, then "-", then account and network: a + // malformed ID is answered without waiting on the producer, and a + // blank pipe cannot mask it. The --type vocabulary is checked here + // too — the switch below cannot move, since its no-op branches read + // the resolved content, but an unknown type dooms the invocation on + // its own and must not cost the caller a drained pipe. + itemType = strings.ToLower(strings.TrimSpace(itemType)) + switch itemType { + case "", "document", "doc", "vault", "folder", "upload", "file": + default: + return output.ErrUsageHint( + fmt.Sprintf("Invalid type: %s", itemType), + "Use: vault, document, or upload", + ) + } + + // --content is meaningless for a folder, and whether it was given is + // knowable before its value is: the switch below needs the resolved + // content for its no-op branches, this does not. + if cmd.Flags().Changed("content") { + switch itemType { + case "vault", "folder": + return output.ErrUsage("--content can only be used with --type document or upload") + } + } + + // Only an exact "-" reads stdin; --content "" stays the clear idiom. + var contentErr error + content, contentErr = resolveContentValue(cmd, content, -1, "--content") + if contentErr != nil { + return contentErr + } + titleChanged := cmd.Flags().Changed("title") contentChanged := cmd.Flags().Changed("content") titleTrimmed := strings.TrimSpace(title) @@ -1877,7 +2008,6 @@ You can pass either an item ID or a Basecamp URL: docContentSet := contentChanged && (content == "" || contentTrimmed != "") nonDocTitleSet := titleChanged && titleTrimmed != "" nonDocContentSet := contentChanged && contentTrimmed != "" - itemType = strings.ToLower(strings.TrimSpace(itemType)) switch itemType { case "", "document", "doc": if !docTitleSet && !docContentSet { @@ -1907,14 +2037,6 @@ You can pass either an item ID or a Basecamp URL: return err } - // Extract ID and project from URL if provided - itemIDStr, urlProjectID := extractWithProject(args[0]) - - itemID, err := strconv.ParseInt(itemIDStr, 10, 64) - if err != nil { - return output.ErrUsage("Invalid item ID") - } - // Resolve project - use URL > flag > config, with interactive fallback projectID := *project if projectID == "" && urlProjectID != "" { @@ -2061,9 +2183,11 @@ You can pass either an item ID or a Basecamp URL: } cmd.Flags().StringVarP(&title, "title", "t", "", "New title") - cmd.Flags().StringVarP(&content, "content", "c", "", "New content") + cmd.Flags().StringVarP(&content, "content", "c", "", "New content; use - to read from stdin") cmd.Flags().StringVar(&itemType, "type", "", "Item type (vault, document, upload)") + allowDash(cmd, "flag:content") + return cmd } @@ -2280,6 +2404,9 @@ Use --out - to stream the file to stdout (for piping to other commands).`, cmd.Flags().StringVarP(&outDir, "out", "o", "", "Output directory (default: current directory)") + // --out - means stream to stdout — exempt from the stdin dash guard. + allowDash(cmd, "flag:out") + return cmd } diff --git a/internal/commands/gauges.go b/internal/commands/gauges.go index f90e0aae5..ff5a2958d 100644 --- a/internal/commands/gauges.go +++ b/internal/commands/gauges.go @@ -178,6 +178,27 @@ func newGaugesCreateCmd(project *string) *cobra.Command { basecamp gauges create --position 75 --color green --in MyProject basecamp gauges create --position 50 --color yellow --description "Halfway there" --in MyProject`, RunE: func(cmd *cobra.Command, args []string) error { + if !cmd.Flags().Changed("position") { + return output.ErrUsage("--position is required") + } + if position < 0 || position > 100 { + return output.ErrUsage("--position must be between 0 and 100") + } + + // --notify custom without --subscriptions cannot succeed, and that + // is knowable from the flags alone, so it is decided before the pipe + // is drained rather than at request-building time. + if notify == "custom" && len(subscriptions) == 0 { + return output.ErrUsage("--subscriptions required when using --notify custom") + } + + // Local validation, then "-", then account: a bad stdin gets the + // stdin error rather than "--account is required". + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + app := appctx.FromContext(cmd.Context()) if err := ensureAccount(cmd, app); err != nil { @@ -194,13 +215,6 @@ func newGaugesCreateCmd(project *string) *cobra.Command { return output.ErrUsage("Invalid project ID") } - if !cmd.Flags().Changed("position") { - return output.ErrUsage("--position is required") - } - if position < 0 || position > 100 { - return output.ErrUsage("--position must be between 0 and 100") - } - req := &basecamp.CreateGaugeNeedleRequest{ Position: position, } @@ -213,9 +227,6 @@ func newGaugesCreateCmd(project *string) *cobra.Command { if notify != "" { req.Notify = notify if notify == "custom" { - if len(subscriptions) == 0 { - return output.ErrUsage("--subscriptions required when using --notify custom") - } req.Subscriptions = subscriptions } } @@ -240,10 +251,12 @@ func newGaugesCreateCmd(project *string) *cobra.Command { cmd.Flags().Int32Var(&position, "position", 0, "Position on gauge (0-100, required)") cmd.Flags().StringVar(&color, "color", "", "Needle color: green, yellow, or red") - cmd.Flags().StringVar(&description, "description", "", "Description (rich text HTML)") + cmd.Flags().StringVar(&description, "description", "", "Description (rich text HTML); use - to read from stdin") cmd.Flags().StringVar(¬ify, "notify", "", "Notification mode: everyone, working_on, or custom") cmd.Flags().Int64SliceVar(&subscriptions, "subscriptions", nil, "Person IDs to notify (used with --notify custom)") + allowDash(cmd, "flag:description") + return cmd } @@ -258,10 +271,8 @@ func newGaugesUpdateCmd() *cobra.Command { basecamp gauges update 12345 --description "Updated status"`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - app := appctx.FromContext(cmd.Context()) - - if err := ensureAccount(cmd, app); err != nil { - return err + if !cmd.Flags().Changed("description") { + return output.ErrUsage("No changes specified (use --description)") } needleID, err := strconv.ParseInt(args[0], 10, 64) @@ -269,8 +280,16 @@ func newGaugesUpdateCmd() *cobra.Command { return output.ErrUsage("Invalid needle ID") } - if !cmd.Flags().Changed("description") { - return output.ErrUsage("No changes specified (use --description)") + // Syntactic checks first, then "-", then account and network. + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + + app := appctx.FromContext(cmd.Context()) + + if err := ensureAccount(cmd, app); err != nil { + return err } req := &basecamp.UpdateGaugeNeedleRequest{ @@ -295,7 +314,9 @@ func newGaugesUpdateCmd() *cobra.Command { }, } - cmd.Flags().StringVar(&description, "description", "", "New description (rich text HTML)") + cmd.Flags().StringVar(&description, "description", "", "New description (rich text HTML); use - to read from stdin") + + allowDash(cmd, "flag:description") return cmd } diff --git a/internal/commands/help_paths_test.go b/internal/commands/help_paths_test.go new file mode 100644 index 000000000..194ef9e83 --- /dev/null +++ b/internal/commands/help_paths_test.go @@ -0,0 +1,101 @@ +package commands_test + +import ( + "regexp" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A help example that names a path cobra cannot resolve exits 0 showing group +// help, so nothing fails and the wrong path keeps getting taught. That is how +// "basecamp docs create" survived: Find() landed on the `docs` group with +// "create" left over. +// +// Coverage is bounded and deliberately so: only the leading run of bare +// lowercase words after "basecamp" is resolved (stopping at the first flag, +// placeholder, quote, or shell metacharacter), and a leftover token is only +// reported when it is the name or alias of some command in the tree — which is +// what distinguishes a mistyped subcommand from a literal argument like a +// project name. +func TestHelpExampleCommandPathsResolveExactly(t *testing.T) { + root := buildRootWithAllCommands() + + commandWords := map[string]bool{} + var collect func(*cobra.Command) + collect = func(cmd *cobra.Command) { + for _, sub := range cmd.Commands() { + commandWords[sub.Name()] = true + for _, alias := range sub.Aliases { + commandWords[alias] = true + } + collect(sub) + } + } + collect(root) + + word := regexp.MustCompile(`^[a-z][a-z0-9-]*$`) + + var checked int + var walk func(*cobra.Command) + walk = func(cmd *cobra.Command) { + for _, text := range []string{cmd.Long, cmd.Example} { + for _, line := range strings.Split(text, "\n") { + _, after, found := strings.Cut(strings.TrimSpace(line), "basecamp ") + if !found { + continue + } + var path []string + for _, token := range strings.Fields(after) { + if !word.MatchString(token) { + break + } + path = append(path, token) + } + // Prose ("basecamp is a CLI tool ...") is not an invocation: + // require the first word to be a real top-level command. + if len(path) == 0 || root.Commands() == nil || !isTopLevel(root, path[0]) { + continue + } + + target, remaining, err := root.Find(path) + require.NoError(t, err, "line %q", line) + checked++ + + // Only a *group* can swallow a mistyped subcommand: it shows + // its help and exits 0. A leaf's leftovers are its arguments + // ("assignments due overdue"), even when the word happens to + // name a command elsewhere in the tree. + if len(remaining) > 0 && target.HasSubCommands() && commandWords[remaining[0]] { + assert.Fail(t, + "example names a path that does not resolve", + "%s: %q resolves to the %q group with %q left over — it exits 0 showing group help", + cmd.CommandPath(), strings.TrimSpace(line), target.CommandPath(), remaining[0]) + } + } + } + for _, sub := range cmd.Commands() { + walk(sub) + } + } + walk(root) + + require.Greater(t, checked, 100, "expected the help corpus to yield many command paths") +} + +func isTopLevel(root *cobra.Command, name string) bool { + for _, sub := range root.Commands() { + if sub.Name() == name { + return true + } + for _, alias := range sub.Aliases { + if alias == name { + return true + } + } + } + return false +} diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index 0e81dd215..d4164d4e4 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -5,7 +5,6 @@ import ( "encoding/json" "errors" "fmt" - "io" "os" "strconv" "strings" @@ -87,25 +86,6 @@ func isMachineOutput(cmd *cobra.Command) bool { return false } -func readPipedStdin(cmd *cobra.Command) (string, bool, error) { - stdin := cmd.InOrStdin() - if f, ok := stdin.(*os.File); ok { - fi, _ := f.Stat() - if fi == nil { - return "", false, nil - } - if (fi.Mode() & os.ModeCharDevice) != 0 { - return "", false, nil - } - } - - data, err := io.ReadAll(stdin) - if err != nil { - return "", false, fmt.Errorf("failed to read stdin: %w", err) - } - return string(data), true, nil -} - // DockTool represents a tool in a project's dock. type DockTool struct { Name string `json:"name"` @@ -159,6 +139,21 @@ func dockToolNotFoundError(all []DockTool, dockName, projectID, friendlyName str return output.ErrNotFoundHint(friendlyName, projectID, fmt.Sprintf("Project has no %s", friendlyName)) } +// requireNumericID rejects an explicitly supplied dock or container ID that is +// not numeric. getDockToolID returns an explicit value verbatim — there is no +// name resolution for these — so the check needs neither an account nor the +// network, and commands that also read a "-" input run it before the read +// rather than discovering it after a request is already being built. +func requireNumericID(value, label string) error { + if value == "" { + return nil + } + if _, err := strconv.ParseInt(value, 10, 64); err != nil { + return output.ErrUsage("Invalid " + label) + } + return nil +} + // getDockToolID retrieves a dock tool ID from a project, handling the multi-dock case. // // When multiple tools of the same type exist in the project: @@ -480,9 +475,21 @@ func resolvePersonIDs(ctx context.Context, resolver *names.Resolver, input strin // // subscribeChanged should be true when the --subscribe flag was explicitly // provided on the command line (i.e. cmd.Flags().Changed("subscribe")). -func applySubscribeFlags(ctx context.Context, resolver *names.Resolver, subscribe string, subscribeChanged, noSubscribe bool) (*[]int64, error) { +// rejectSubscribeConflict answers the one part of applySubscribeFlags that +// needs neither the network nor an account, so callers that read stdin can +// settle it first: draining a pipe for an invocation this rejects makes the +// caller wait on a producer whose output is discarded, and lets a blank pipe +// answer "stdin is empty" instead of naming the conflict. +func rejectSubscribeConflict(subscribeChanged, noSubscribe bool) error { if subscribeChanged && noSubscribe { - return nil, output.ErrUsage("--subscribe and --no-subscribe are mutually exclusive") + return output.ErrUsage("--subscribe and --no-subscribe are mutually exclusive") + } + return nil +} + +func applySubscribeFlags(ctx context.Context, resolver *names.Resolver, subscribe string, subscribeChanged, noSubscribe bool) (*[]int64, error) { + if err := rejectSubscribeConflict(subscribeChanged, noSubscribe); err != nil { + return nil, err } if noSubscribe { empty := []int64{} diff --git a/internal/commands/messages.go b/internal/commands/messages.go index 82014f678..3e6c1b94c 100644 --- a/internal/commands/messages.go +++ b/internal/commands/messages.go @@ -431,7 +431,13 @@ func newMessagesCreateCmd(project *string, messageBoard *string) *cobra.Command cmd := &cobra.Command{ Use: "create <title> [body]", Short: "Create a new message", - Long: "Post a new message to a project's message board.", + Long: `Post a new message to a project's message board. + +Use - as the body argument to read the body from stdin: + printf 'Long **Markdown** body' | basecamp messages create "Title" -`, + // Bounded so a stray third token is a usage error rather than being + // silently dropped after "-" has already drained stdin. + Args: cobra.MaximumNArgs(2), RunE: func(cmd *cobra.Command, args []string) error { // Show help when invoked with no title if len(args) == 0 { @@ -449,10 +455,29 @@ func newMessagesCreateCmd(project *string, messageBoard *string) *cobra.Command return cmd.Help() } - // Validate user input first, before checking account + // Validate user input first, before checking account. The --edit + // exclusion runs before "-" resolution so --edit … - errors + // without consuming stdin. if edit && body != "" { return output.ErrUsage("cannot combine --edit and body argument") } + if err := rejectSubscribeConflict(cmd.Flags().Changed("subscribe"), noSubscribe); err != nil { + return err + } + if err := requireNumericID(*messageBoard, "message board ID"); err != nil { + return err + } + // Attachment paths are readable or not regardless of the body, so + // check them before the pipe is drained. + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + + var err error + body, err = resolveContentValue(cmd, body, 1, "[body]") + if err != nil { + return err + } if edit { fi, err := os.Stdin.Stat() if err != nil || (fi.Mode()&os.ModeCharDevice) == 0 { @@ -591,6 +616,8 @@ func newMessagesCreateCmd(project *string, messageBoard *string) *cobra.Command cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") cmd.Flags().BoolVar(&visibleToClients, "visible-to-clients", false, "Make the message visible to clients on the project (omit for the server default; client-authenticated callers always post client-visible)") + allowDash(cmd, "arg:1") + return cmd } @@ -612,12 +639,6 @@ You can pass either a message ID or a Basecamp URL: return noChanges(cmd) } - app := appctx.FromContext(cmd.Context()) - - if err := ensureAccount(cmd, app); err != nil { - return err - } - // Extract ID from URL if provided messageIDStr := extractID(args[0]) @@ -626,6 +647,20 @@ You can pass either a message ID or a Basecamp URL: return output.ErrUsage("Invalid message ID") } + // Syntactic checks first, then "-", then account and network: a + // malformed ID is answered without waiting on the producer, and a + // blank pipe cannot mask it. + body, err = resolveContentValue(cmd, body, -1, "--body") + if err != nil { + return err + } + + app := appctx.FromContext(cmd.Context()) + + if err := ensureAccount(cmd, app); err != nil { + return err + } + // Build SDK request // Convert Markdown content to HTML for Basecamp's rich text fields html := richtext.MarkdownToHTML(body) @@ -672,7 +707,9 @@ You can pass either a message ID or a Basecamp URL: } cmd.Flags().StringVarP(&title, "title", "t", "", "New title") - cmd.Flags().StringVarP(&body, "body", "b", "", "New body content") + cmd.Flags().StringVarP(&body, "body", "b", "", "New body content; use - to read from stdin") + + allowDash(cmd, "flag:body") return cmd } diff --git a/internal/commands/notes.go b/internal/commands/notes.go index f43d41f35..0405e2769 100644 --- a/internal/commands/notes.go +++ b/internal/commands/notes.go @@ -31,7 +31,7 @@ person, so there is nothing to list and no id to pass. basecamp notes show basecamp notes set "Remember to follow up on the Q3 rollout" basecamp notes set --file notes.md - cat notes.md | basecamp notes set`, + cat notes.md | basecamp notes set -`, Annotations: map[string]string{ "agent_notes": "Account-wide and personal — no --in <project> needed.\n" + "Singleton: no id. 'set' replaces the whole note; it does not append.", @@ -108,16 +108,17 @@ func newNotesSetCmd() *cobra.Command { Short: "Replace your personal note", Long: `Replace your personal note with new content. -Content comes from a positional argument, --file, or piped stdin. Markdown is -converted to HTML, since the note is a rich text field — passing raw text -through would store escaped markup rather than formatting. +Content comes from a positional argument or --file; either accepts - to read +from stdin. Markdown is converted to HTML, since the note is a rich text +field — passing raw text through would store escaped markup rather than +formatting. This replaces the whole note; it does not append. The first write creates the note, so there is no separate "create" step. basecamp notes set "Follow up with Ann on the rollout" basecamp notes set --file notes.md - cat notes.md | basecamp notes set + cat notes.md | basecamp notes set - Attachments are out of scope: this writes the note body only. @@ -163,44 +164,37 @@ a destructive verb deserves its own review, not a rider on a bump.`, }, } - cmd.Flags().StringVarP(&file, "file", "f", "", "Read note content from a file") + cmd.Flags().StringVarP(&file, "file", "f", "", "Read note content from a file; use - to read from stdin") + + allowDash(cmd, "arg:0", "flag:file") return cmd } -// notesContent resolves the note body from exactly one of the three inputs. -// -// Naming two sources is a usage error rather than a silent precedence rule: a +// notesContent resolves the note body from exactly one of two inputs: the +// positional argument (where "-" reads stdin) or --file (where "-" also reads +// stdin). Naming both is a usage error rather than a silent precedence rule: a // caller who passes both an argument and --file has a wrong expectation about // which one wins, and this command overwrites the whole note. // -// All three sources are detected before any of them is chosen. Checking stdin -// only after an argument and --file had been ruled out made -// `generate | basecamp notes set --file fallback.md` overwrite the note from -// the file and discard the generated body without a word — the precise failure -// this function exists to prevent, in the one command that replaces everything. +// A pipe without "-" is deliberately not consumed as an implicit third source. +// Reading it silently made `generate | basecamp notes set --file fallback.md` +// a coin-flip over which body survives; requiring the explicit "-" makes the +// caller name the source in the one command that replaces everything. func notesContent(cmd *cobra.Command, args []string, file string) (string, error) { positional := strings.Join(args, " ") - piped, ok, err := readPipedStdin(cmd) - if err != nil { - return "", err - } - // An empty pipe is not a source. A redirected-but-empty stdin carries no - // body to lose, so it must not turn a valid `--file` call into an error. - hasPipe := ok && strings.TrimSpace(piped) != "" - - named := 0 - for _, present := range []bool{file != "", positional != "", hasPipe} { - if present { - named++ - } - } - if named > 1 { - return "", output.ErrUsage("pass note content as an argument, with --file, or on stdin — not more than one") + if file != "" && positional != "" { + return "", output.ErrUsage("pass note content as an argument or with --file — not both") } switch { + case file == "-": + content, err := readStdinContent(cmd, "--file") + if err != nil { + return "", err + } + return notesRequireContent(content) case file != "": data, err := os.ReadFile(file) if err != nil { @@ -208,14 +202,16 @@ func notesContent(cmd *cobra.Command, args []string, file string) (string, error } return notesRequireContent(string(data)) case positional != "": - return notesRequireContent(positional) - case hasPipe: - return notesRequireContent(piped) + content, err := resolveContentValue(cmd, positional, 0, "[content]") + if err != nil { + return "", err + } + return notesRequireContent(content) } return "", output.ErrUsageHint( "note content is required", - `Pass it as an argument, with --file, or on stdin: basecamp notes set "..."`, + `Pass it as an argument, with --file, or pipe it and pass "-": basecamp notes set -`, ) } diff --git a/internal/commands/notes_test.go b/internal/commands/notes_test.go index e89b12932..4b960158d 100644 --- a/internal/commands/notes_test.go +++ b/internal/commands/notes_test.go @@ -124,13 +124,13 @@ func TestNotesSetReadsFromAFile(t *testing.T) { assert.NotContains(t, body.Note.Content, "# Heading", "raw Markdown must not reach the wire") } -func TestNotesSetReadsPipedStdin(t *testing.T) { +func TestNotesSetReadsDashFromStdin(t *testing.T) { app, transport, _ := setupPersonalFeedApp(t, notesUpdateRoute()) cmd := NewNotesCmd() cmd.SetIn(strings.NewReader("piped note body")) - require.NoError(t, executeRecordingCommand(cmd, app, "set")) + require.NoError(t, executeRecordingCommand(cmd, app, "set", "-")) var body struct { Note struct { @@ -141,6 +141,38 @@ func TestNotesSetReadsPipedStdin(t *testing.T) { assert.Contains(t, body.Note.Content, "piped note body") } +func TestNotesSetReadsDashFileFromStdin(t *testing.T) { + app, transport, _ := setupPersonalFeedApp(t, notesUpdateRoute()) + + cmd := NewNotesCmd() + cmd.SetIn(strings.NewReader("piped note body")) + + require.NoError(t, executeRecordingCommand(cmd, app, "set", "--file", "-")) + + var body struct { + Note struct { + Content string `json:"content"` + } `json:"note"` + } + require.NoError(t, json.Unmarshal([]byte(transport.last(t).Body), &body)) + assert.Contains(t, body.Note.Content, "piped note body") +} + +// A pipe without "-" is not consumed. With no other source named, the error +// teaches the explicit placeholder instead of silently reading the pipe. +func TestNotesSetBarePipeErrorsWithDashHint(t *testing.T) { + app, transport, _ := setupPersonalFeedApp(t, notesUpdateRoute()) + + cmd := NewNotesCmd() + cmd.SetIn(strings.NewReader("piped note body")) + + err := executeRecordingCommand(cmd, app, "set") + + outErr := requireBookmarksUsageError(t, err) + assert.Contains(t, outErr.Hint, "notes set -") + assert.Empty(t, transport.recorded(), "a rejected write must not reach the server") +} + // set replaces the whole note, so the failure modes that would silently erase // it are rejected before the request rather than written through. func TestNotesSetRejectsAmbiguousOrEmptyInput(t *testing.T) { @@ -169,19 +201,20 @@ func TestNotesSetRejectsAmbiguousOrEmptyInput(t *testing.T) { } } -// A piped body must never lose to a flag. `generate | basecamp notes set --file -// fallback.md` used to overwrite the note from the file and throw the generated -// body away, because stdin was only consulted after --file had been ruled out. -func TestNotesSetRejectsPipedContentAlongsideAnotherSource(t *testing.T) { +// A pipe is only ever a source through an explicit "-". When another source is +// named, the unclaimed pipe is ignored — the CLI-wide rule since bare-pipe +// reads were removed — rather than triggering the old ambiguity error. +func TestNotesSetIgnoresUnclaimedPipeWhenSourceIsNamed(t *testing.T) { populated := filepath.Join(t.TempDir(), "note.md") require.NoError(t, os.WriteFile(populated, []byte("from the file"), 0o600)) for _, tc := range []struct { name string args []string + want string }{ - {"pipe and --file together", []string{"set", "--file", populated}}, - {"pipe and an argument together", []string{"set", "inline"}}, + {"pipe and --file together", []string{"set", "--file", populated}, "from the file"}, + {"pipe and an argument together", []string{"set", "inline"}, "inline"}, } { t.Run(tc.name, func(t *testing.T) { app, transport, _ := setupPersonalFeedApp(t, notesUpdateRoute()) @@ -189,14 +222,33 @@ func TestNotesSetRejectsPipedContentAlongsideAnotherSource(t *testing.T) { cmd := NewNotesCmd() cmd.SetIn(strings.NewReader("piped note body")) - err := executeRecordingCommand(cmd, app, tc.args...) + require.NoError(t, executeRecordingCommand(cmd, app, tc.args...)) - requireBookmarksUsageError(t, err) - assert.Empty(t, transport.recorded(), "a rejected write must not reach the server") + var body struct { + Note struct { + Content string `json:"content"` + } `json:"note"` + } + require.NoError(t, json.Unmarshal([]byte(transport.last(t).Body), &body)) + assert.Contains(t, body.Note.Content, tc.want) + assert.NotContains(t, body.Note.Content, "piped note body") }) } } +// Naming both explicit sources is still an ambiguity error. +func TestNotesSetRejectsArgumentAndFileTogether(t *testing.T) { + populated := filepath.Join(t.TempDir(), "note.md") + require.NoError(t, os.WriteFile(populated, []byte("from the file"), 0o600)) + + app, transport, _ := setupPersonalFeedApp(t, notesUpdateRoute()) + + err := executeRecordingCommand(NewNotesCmd(), app, "set", "inline", "--file", populated) + + requireBookmarksUsageError(t, err) + assert.Empty(t, transport.recorded(), "a rejected write must not reach the server") +} + // An empty pipe carries no body to lose, so it must not turn an otherwise valid // --file call into an ambiguity error. func TestNotesSetIgnoresAnEmptyPipeAlongsideAFile(t *testing.T) { diff --git a/internal/commands/projects.go b/internal/commands/projects.go index 0ec4164e6..e696eaeef 100644 --- a/internal/commands/projects.go +++ b/internal/commands/projects.go @@ -274,6 +274,11 @@ func newProjectsCreateCmd() *cobra.Command { return fmt.Errorf("app not initialized") } + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + // Resolve account if not configured (enables interactive prompt) if err := ensureAccount(cmd, app); err != nil { return err @@ -296,7 +301,9 @@ func newProjectsCreateCmd() *cobra.Command { }, } - cmd.Flags().StringVarP(&description, "description", "d", "", "Project description") + cmd.Flags().StringVarP(&description, "description", "d", "", "Project description; use - to read from stdin") + + allowDash(cmd, "flag:description") return cmd } @@ -324,16 +331,22 @@ Examples: return fmt.Errorf("app not initialized") } - // Resolve account if not configured (enables interactive prompt) - if err := ensureAccount(cmd, app); err != nil { - return err - } - projectID, err := strconv.ParseInt(args[0], 10, 64) if err != nil { return output.ErrUsage("Invalid project ID") } + // Syntactic checks first, then "-", then account and network. + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + + // Resolve account if not configured (enables interactive prompt) + if err := ensureAccount(cmd, app); err != nil { + return err + } + // For update, we need to provide name (required by SDK) // If only description is provided, we need to fetch current name first updateName := name @@ -372,7 +385,9 @@ Examples: } cmd.Flags().StringVarP(&name, "name", "n", "", "New name") - cmd.Flags().StringVarP(&description, "description", "d", "", "New description") + cmd.Flags().StringVarP(&description, "description", "d", "", "New description; use - to read from stdin") + + allowDash(cmd, "flag:description") return cmd } diff --git a/internal/commands/schedule.go b/internal/commands/schedule.go index 743b495ef..31f7a5e95 100644 --- a/internal/commands/schedule.go +++ b/internal/commands/schedule.go @@ -428,11 +428,6 @@ func newScheduleCreateCmd(project, scheduleID *string) *cobra.Command { return missingArg(cmd, "<summary>") } - app := appctx.FromContext(cmd.Context()) - if err := ensureAccount(cmd, app); err != nil { - return err - } - if startsAt == "" { return output.ErrUsage("--starts-at required (ISO 8601 datetime)") } @@ -446,6 +441,31 @@ func newScheduleCreateCmd(project, scheduleID *string) *cobra.Command { return err } + if err := rejectSubscribeConflict(cmd.Flags().Changed("subscribe"), noSubscribe); err != nil { + return err + } + if err := requireNumericID(*scheduleID, "schedule ID"); err != nil { + return err + } + + // Attachment paths are readable or not regardless of the body, so + // check them before the pipe is drained. + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + + // Local validation, then "-", then account: a bad stdin gets the + // stdin error rather than "--account is required". + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + + app := appctx.FromContext(cmd.Context()) + if err := ensureAccount(cmd, app); err != nil { + return err + } + return runScheduleCreate(cmd, app, *project, *scheduleID, entrySummary, startsAt, endsAt, description, allDay, notify, visibleToClients, participants, subscribe, noSubscribe, attachFiles) }, } @@ -456,13 +476,15 @@ func newScheduleCreateCmd(project, scheduleID *string) *cobra.Command { cmd.Flags().StringVar(&startsAt, "start", "", "Start time (alias)") cmd.Flags().StringVar(&endsAt, "ends-at", "", "End time (ISO 8601)") cmd.Flags().StringVar(&endsAt, "end", "", "End time (alias)") - cmd.Flags().StringVar(&description, "description", "", "Detailed description") + cmd.Flags().StringVar(&description, "description", "", "Detailed description; use - to read from stdin") cmd.Flags().StringVar(&description, "desc", "", "Description (alias)") cmd.Flags().BoolVar(&allDay, "all-day", false, "Mark as all-day event") cmd.Flags().BoolVar(¬ify, "notify", false, "Notify participants") cmd.Flags().StringVar(&participants, "participants", "", "Comma-separated person IDs") cmd.Flags().StringVar(&participants, "people", "", "Person IDs (alias)") cmd.Flags().StringVar(&subscribe, "subscribe", "", "Subscribe specific people (comma-separated names, emails, IDs, or \"me\")") + + allowDash(cmd, "flag:description", "flag:desc") cmd.Flags().BoolVar(&noSubscribe, "no-subscribe", false, "Don't subscribe anyone else (silent, no notifications)") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") cmd.Flags().BoolVar(&visibleToClients, "visible-to-clients", false, "Make the schedule entry visible to clients on the project (omit for the server default; client-authenticated callers always post client-visible)") @@ -613,12 +635,47 @@ You can pass either an entry ID or a Basecamp URL: Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) - if err := ensureAccount(cmd, app); err != nil { + + // Extract ID and project from URL if provided. Purely syntactic, + // so it precedes the stdin read like every other target-ID check. + entryID, urlProjectID := extractWithProject(args[0]) + + // This command used to send a malformed ID to the server as 0 — + // the ParseInt further down discarded its error. Reject it here + // instead, like every sibling update command, and before the pipe + // is drained. + entryIDInt, err := strconv.ParseInt(entryID, 10, 64) + if err != nil { + return output.ErrUsage("Invalid schedule entry ID") + } + + // The timestamp formats and the attachment paths are decidable from + // the flags alone, as they already are on the create path — so they + // precede the read rather than following account and project + // resolution. + if startsAt != "" { + if err := validateScheduleTimestamp("starts-at", startsAt); err != nil { + return err + } + } + if endsAt != "" { + if err := validateScheduleTimestamp("ends-at", endsAt); err != nil { + return err + } + } + if err := validateAttachPaths(attachFiles); err != nil { return err } - // Extract ID and project from URL if provided - entryID, urlProjectID := extractWithProject(args[0]) + // Syntactic checks first, then "-", then account and network. + description, err = resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + + if err := ensureAccount(cmd, app); err != nil { + return err + } // Resolve project - use URL > flag > config, with interactive fallback projectID := *project @@ -643,8 +700,6 @@ You can pass either an entry ID or a Basecamp URL: return err } - entryIDInt, _ := strconv.ParseInt(entryID, 10, 64) - // Build request with provided fields only req := &basecamp.UpdateScheduleEntryRequest{} hasChanges := false @@ -654,16 +709,10 @@ You can pass either an entry ID or a Basecamp URL: hasChanges = true } if startsAt != "" { - if err := validateScheduleTimestamp("starts-at", startsAt); err != nil { - return err - } req.StartsAt = basecamp.Ptr(startsAt) hasChanges = true } if endsAt != "" { - if err := validateScheduleTimestamp("ends-at", endsAt); err != nil { - return err - } req.EndsAt = basecamp.Ptr(endsAt) hasChanges = true } @@ -764,7 +813,7 @@ You can pass either an entry ID or a Basecamp URL: cmd.Flags().StringVar(&startsAt, "start", "", "Start time (alias)") cmd.Flags().StringVar(&endsAt, "ends-at", "", "End time (ISO 8601)") cmd.Flags().StringVar(&endsAt, "end", "", "End time (alias)") - cmd.Flags().StringVar(&description, "description", "", "Detailed description") + cmd.Flags().StringVar(&description, "description", "", "Detailed description; use - to read from stdin") cmd.Flags().StringVar(&description, "desc", "", "Description (alias)") cmd.Flags().BoolVar(&allDay, "all-day", false, "Mark as all-day event") cmd.Flags().BoolVar(¬ify, "notify", false, "Notify participants") @@ -772,6 +821,8 @@ You can pass either an entry ID or a Basecamp URL: cmd.Flags().StringVar(&participants, "people", "", "Person IDs (alias)") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") + allowDash(cmd, "flag:description", "flag:desc") + return cmd } diff --git a/internal/commands/stdin.go b/internal/commands/stdin.go new file mode 100644 index 000000000..8b1e0c143 --- /dev/null +++ b/internal/commands/stdin.go @@ -0,0 +1,393 @@ +package commands + +import ( + "fmt" + "io" + "strings" + + "github.com/spf13/cobra" + "github.com/spf13/pflag" + + "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/stdinarg" +) + +// This file is the single home for "-" (read from stdin) handling. +// +// Tier 1: commands that accept content register where "-" is honored via +// allowDash and resolve it with resolveContentArg / resolveContentValue. +// Tier 2: every other exact "-" — positional or flag value — is caught by the +// dash guard installed over the whole command tree: when stdin is piped, a +// stray "-" is ambiguous (the caller almost certainly meant "read the pipe"), +// so it fails as a usage error instead of landing as literal content. On a +// TTY, a literal "-" stays legal everywhere. Cobra's generated meta commands +// (help, __complete) are the one deliberate exemption — see isMetaCommand. + +// allowDash marks where cmd accepts "-" as "read from stdin", merging with any +// tokens already registered. Tokens: "arg:0" (exact positional index), +// "arg:1+" (that index and beyond), "flag:data" (a flag value). +func allowDash(cmd *cobra.Command, tokens ...string) { + if cmd.Annotations == nil { + cmd.Annotations = map[string]string{} + } + merged := strings.Join(tokens, " ") + if existing := cmd.Annotations[stdinarg.AnnotationAllowDash]; existing != "" { + merged = existing + " " + merged + } + cmd.Annotations[stdinarg.AnnotationAllowDash] = merged +} + +// readStdinContent reads content for a "-" placeholder from piped stdin. +// +// Nothing piped (a TTY) is a usage error rather than a silent read: waiting on +// an interactive terminal looks like a hang, so the error teaches the escape +// hatches instead. A piped-but-blank stdin is also refused — blank content is +// never an intentional write, and for update-style commands it would be an +// implicit clear. +// +// Trailing newlines — LF and CRLF alike — are trimmed: Markdown bodies don't +// care, but titles and boosts (16-rune limit) do, and virtually every pipe +// ends with one. Interior line breaks are untouched. +func readStdinContent(cmd *cobra.Command, what string) (string, error) { + if !stdinarg.IsPiped(cmd.InOrStdin()) { + return "", output.ErrUsageHint( + fmt.Sprintf(`%s is "-" (read from stdin) but nothing is piped`, what), + stdinEscapeHint(cmd, what), + ) + } + data, err := io.ReadAll(cmd.InOrStdin()) + if err != nil { + return "", output.ErrUsage(fmt.Sprintf("failed to read %s from stdin: %v", what, err)) + } + content := strings.TrimRight(string(data), "\r\n") + if strings.TrimSpace(content) == "" { + return "", output.ErrUsage(fmt.Sprintf("stdin for %s is empty", what)) + } + return content, nil +} + +// stdinEscapeHint lists the ways to satisfy a "-" from an interactive +// terminal, mentioning --edit only where the command has it. +// +// The examples repeat the input that actually carried the "-": suggesting a +// bare trailing "-" for a flag would exceed the command's positional arity. +func stdinEscapeHint(cmd *cobra.Command, what string) string { + path := cmd.CommandPath() + source := "-" + if strings.HasPrefix(what, "--") { + source = what + " -" + } + hint := fmt.Sprintf( + "Pipe the content (printf '...' | %[1]s ... %[2]s), use a heredoc (%[1]s ... %[2]s <<'EOF'), or run cat | %[1]s ... %[2]s and type the content, ending with Ctrl-D", + path, source) + if cmd.Flags().Lookup("edit") != nil { + hint += "; or compose with --edit" + } + return hint +} + +// resolveContentArg resolves the join-all positional content pattern: exactly +// ["-"] reads stdin, any other args join with spaces. A "-" mixed in with +// other tokens is a usage error — it can't be both stdin and part of the +// joined text, and pre-guard versions silently posted the literal join. +// Every join-all site names its positional <content>, so errors do too. +// +// argsOffset is the index of args[0] in the command's full positional list, +// so a "-" placed at or after the "--" separator stays literal. +func resolveContentArg(cmd *cobra.Command, args []string, argsOffset int) (string, error) { + dashes := 0 + for i, a := range args { + if a == "-" && !afterDashSeparator(cmd, argsOffset+i) { + dashes++ + } + } + switch { + case dashes == 0: + return strings.Join(args, " "), nil + case len(args) == 1: + return readStdinContent(cmd, "<content>") + default: + return "", output.ErrUsage(`"-" (stdin) must be the only <content> argument`) + } +} + +// resolveContentValue resolves a single content value — an exact positional +// (pass its index) or a flag value (pass argIndex -1). Exactly "-" reads +// stdin; a positional "-" at or after the "--" separator stays literal. +func resolveContentValue(cmd *cobra.Command, value string, argIndex int, what string) (string, error) { + if value != "-" || (argIndex >= 0 && afterDashSeparator(cmd, argIndex)) { + return value, nil + } + return readStdinContent(cmd, what) +} + +// afterDashSeparator reports whether the positional at index came after the +// "--" separator, making it literal by definition. +func afterDashSeparator(cmd *cobra.Command, index int) bool { + lenAtDash := cmd.ArgsLenAtDash() + return lenAtDash >= 0 && index >= lenAtDash +} + +// InstallDashGuard wraps every non-meta runnable command in the tree with the +// tier-2 dash guard. It wraps the Args validator: cobra runs ValidateArgs +// after flag parsing (so Changed and ArgsLenAtDash are available) but before the +// persistent pre-run chain, PreRunE, and required-flag validation — so a +// stray "-" is rejected before any lifecycle side effect (config hardening, +// the update check) and before a competing usage error can shadow it. It is +// not a PersistentPreRunE hook because cobra runs only the innermost one — +// the agent hook already shadows the root's, and any future subtree would +// silently lose the guard. A nil Args means ArbitraryArgs (always nil), so +// wrapping it is behavior-preserving. +func InstallDashGuard(root *cobra.Command) { + // Materialize cobra's generated help command before the walk so the + // exemption below is a decision the tree records, not an accident of + // running before ExecuteC adds it. Idempotent; ExecuteC calls it again. + if !root.HasParent() { + root.InitDefaultHelpCmd() + } + + // The root's nil Args is load-bearing: cobra's Find() rejects unknown + // subcommands (legacyArgs) only while Args == nil, so wrapping it would + // turn "basecamp unknowncmd" into a quickstart run. Guard the front of its + // persistent pre-run instead — the earliest hook that still leaves Args + // nil, and earlier than RunE, so the stray-dash error is not shadowed by + // config loading, profile resolution or --jq validation, none of which the + // caller asked about. + // + // Subcommands inherit this hook when they define none of their own, so it + // acts only when the root itself is executing; every subcommand is already + // guarded at Args-validation time, which is earlier still. + skipRootArgs := root.Args == nil && !root.HasParent() && root.HasSubCommands() + switch { + case !root.Runnable(), isMetaCommand(root): + case skipRootArgs: + existingE := root.PersistentPreRunE + existing := root.PersistentPreRun + root.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { + if cmd == root { + if err := guardDashArgs(cmd, args); err != nil { + return err + } + } + // Setting the E form shadows any non-E hook cobra would have run + // in its place, so call it here rather than dropping it. + switch { + case existingE != nil: + return existingE(cmd, args) + case existing != nil: + existing(cmd, args) + } + return nil + } + default: + existing := root.Args + root.Args = func(cmd *cobra.Command, args []string) error { + if err := guardDashArgs(cmd, args); err != nil { + return err + } + if existing != nil { + return existing(cmd, args) + } + return nil + } + } + for _, sub := range root.Commands() { + InstallDashGuard(sub) + } +} + +// isMetaCommand reports whether cmd is one of cobra's generated commands, which +// the tier-2 guard deliberately skips. +// +// Tier 2 exists to stop a stray "-" landing as content. These perform no +// Basecamp content write — they print to stdout and stop there — so a literal +// "-" has nothing to corrupt: "help -" resolves like any unknown topic and +// prints help. +// For the completion commands the exemption is load-bearing rather than merely +// harmless — the shell passes the word being completed as an argument, so +// "basecamp todos create -<TAB>" runs "basecamp __complete todos create -". +// Guarding that would break completion for every flag in the CLI. +func isMetaCommand(cmd *cobra.Command) bool { + switch cmd.Name() { + case "help", cobra.ShellCompRequestCmd, cobra.ShellCompNoDescRequestCmd: + return true + } + return false +} + +// guardDashArgs enforces the tier-2 policy for one invocation: +// +// 1. Collect every exact "-" — positionals before the "--" separator, plus +// changed string-ish flags whose value (or element) is exactly "-". +// 2. More than one allowed "-" can never be satisfied by one stdin, so that +// fails regardless of pipe state. +// 3. A disallowed "-" combined with piped stdin is ambiguous — the caller +// meant the pipe — so it fails with a hint naming the offender. +// 4. Otherwise pass through: a TTY literal "-" stays legal everywhere. +func guardDashArgs(cmd *cobra.Command, args []string) error { + allow := stdinarg.ParseAllow(cmd.Annotations[stdinarg.AnnotationAllowDash]) + + allowed := 0 + var disallowedArgs, disallowedFlags []string + + for i, a := range args { + if a != "-" || afterDashSeparator(cmd, i) { + continue + } + if allow.Arg(i) { + allowed++ + } else { + disallowedArgs = append(disallowedArgs, positionalName(cmd, i)) + } + } + + for _, group := range changedFlagGroups(cmd) { + dashes := 0 + switch group.value.Type() { + case "string": + if group.value.String() == "-" { + dashes = 1 + } + case "stringArray", "stringSlice": + if sv, ok := group.value.(pflag.SliceValue); ok { + for _, v := range sv.GetSlice() { + if v == "-" { + dashes++ + } + } + } + default: + continue + } + if dashes == 0 { + continue + } + if group.allowed(allow) { + allowed += dashes + } else { + disallowedFlags = append(disallowedFlags, group.label()) + } + } + + if allowed > 1 { + return output.ErrUsage(`only one input can read from stdin ("-") at a time`) + } + disallowed := append(append([]string{}, disallowedArgs...), disallowedFlags...) + if len(disallowed) > 0 && stdinarg.IsPiped(cmd.InOrStdin()) { + msg := fmt.Sprintf(`%s does not read stdin via "-" for %s`, + cmd.CommandPath(), strings.Join(disallowed, ", ")) + // -- only escapes positionals; a flag value has no in-line escape, so + // the honest remedy there is an unpiped stdin. Naming a concrete + // redirect would be wrong on Windows and on headless runners with no + // controlling terminal, so the hint stays at the shape of the fix. + var hints []string + if len(disallowedArgs) > 0 { + hints = append(hints, `For a literal "-" argument, pass it after the -- separator`) + } + if len(disallowedFlags) > 0 { + hints = append(hints, `For a literal "-" flag value, run the command without piped stdin`) + } + if accepts := describeAllowed(cmd, allow); accepts != "" { + hints = append(hints, "this command reads stdin when \"-\" is given as "+accepts) + } + return output.ErrUsageHint(msg, strings.Join(hints, "; ")) + } + return nil +} + +// flagGroup is one logical input: every spelling pflag has bound to the same +// backing value. Aliases (--description/--desc, --in/--project) share a single +// pflag.Value instance, so grouping on it keeps a value set through two +// spellings from counting as two stdin inputs. +type flagGroup struct { + names []string + value pflag.Value + changed bool +} + +// label names the group for a guard error. Parsed state does not record which +// spelling the caller typed — only the merged value survives — so an alias +// group names every spelling rather than guessing one and being wrong. +func (g *flagGroup) label() string { + return "--" + strings.Join(g.names, "/--") +} + +// allowed reports whether any spelling of this input is registered for stdin. +// One value, one policy: registering a single alias covers the group. +func (g *flagGroup) allowed(allow stdinarg.Allow) bool { + for _, name := range g.names { + if allow.Flag(name) { + return true + } + } + return false +} + +// changedFlagGroups returns the groups the caller actually set, in flag- +// declaration order within each group. +func changedFlagGroups(cmd *cobra.Command) []*flagGroup { + var groups []*flagGroup + index := map[pflag.Value]*flagGroup{} + cmd.Flags().VisitAll(func(f *pflag.Flag) { + group, ok := index[f.Value] + if !ok { + group = &flagGroup{value: f.Value} + index[f.Value] = group + groups = append(groups, group) + } + group.names = append(group.names, f.Name) + group.changed = group.changed || f.Changed + }) + changed := groups[:0] + for _, group := range groups { + if group.changed { + changed = append(changed, group) + } + } + return changed +} + +// positionalName names a positional for guard errors, preferring the +// placeholder from the Use string ("<name>") over a bare ordinal. +func positionalName(cmd *cobra.Command, index int) string { + if placeholders := usePlaceholders(cmd); index < len(placeholders) { + return placeholders[index] + } + return fmt.Sprintf("argument %d", index+1) +} + +// describeAllowed renders the allow set for hints: placeholder names for +// positionals, --name for flags. +func describeAllowed(cmd *cobra.Command, allow stdinarg.Allow) string { + var parts []string + placeholders := usePlaceholders(cmd) + for i, p := range placeholders { + if allow.Arg(i) { + parts = append(parts, p) + } + } + for _, token := range strings.Fields(cmd.Annotations[stdinarg.AnnotationAllowDash]) { + // --out is exempted for "-" meaning stdout, not stdin — listing it + // under "reads stdin" would teach the wrong thing. + if name, ok := strings.CutPrefix(token, "flag:"); ok && name != "out" { + parts = append(parts, "--"+name) + } + } + return strings.Join(parts, ", ") +} + +// usePlaceholders extracts the positional placeholders ("<id|url>", +// "[content]") from the command's Use string, in order. +func usePlaceholders(cmd *cobra.Command) []string { + fields := strings.Fields(cmd.Use) + if len(fields) == 0 { + return nil + } + var placeholders []string + for _, f := range fields[1:] { + if strings.HasPrefix(f, "<") || strings.HasPrefix(f, "[") { + placeholders = append(placeholders, f) + } + } + return placeholders +} diff --git a/internal/commands/stdin_integration_test.go b/internal/commands/stdin_integration_test.go new file mode 100644 index 000000000..2332e422a --- /dev/null +++ b/internal/commands/stdin_integration_test.go @@ -0,0 +1,489 @@ +package commands + +// Integration coverage for the "-" (stdin) tier-1 patterns: one command per +// resolver shape, driven through a real Execute with a mock transport. + +import ( + "bytes" + "encoding/json" + "errors" + "net/http" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/auth" + "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/names" + "github.com/basecamp/basecamp-cli/internal/output" +) + +// Pattern 2: exact positional — messages create <title> [body]. +func TestMessagesCreateBodyDashReadsStdin(t *testing.T) { + transport := &mockMessageCreateTransport{} + app, _ := setupMessagesMockApp(t, transport) + + cmd := NewMessagesCmd() + cmd.SetIn(strings.NewReader("Body **from stdin**\n")) + + err := executeMessagesCommand(cmd, app, "create", "Title", "-") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBody) + + var body map[string]any + require.NoError(t, json.Unmarshal(transport.capturedBody, &body)) + assert.Equal(t, "Title", body["subject"]) + content, _ := body["content"].(string) + assert.Contains(t, content, "<strong>from stdin</strong>") +} + +// Pattern 3: content flag — api post --data -. +func TestAPIPostDataDashReadsStdin(t *testing.T) { + transport := &mockCommentWriteTransport{} + app, _ := setupCommentsWriteTestApp(t, transport) + + cmd := NewAPICmd() + cmd.SetIn(strings.NewReader(`{"content":"from stdin"}` + "\n")) + + err := executeCommand(cmd, app, "post", "/buckets/1/todos.json", "--data", "-") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBodies) + + var body map[string]any + require.NoError(t, json.Unmarshal(transport.capturedBodies[0], &body)) + assert.Equal(t, "from stdin", body["content"]) +} + +// Pattern 1: join-all positionals — todos create <content>. +func TestTodosCreateDashReadsStdin(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + + transport := &mockTodoCreateTransport{} + cfg := &config.Config{AccountID: "99999", ProjectID: "123", TodolistID: "456"} + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &todosTestTokenProvider{}, + basecamp.WithTransport(transport), + basecamp.WithMaxRetries(1), + ) + authMgr := auth.NewManager(cfg, nil) + app := &appctx.App{ + Config: cfg, + Auth: authMgr, + SDK: sdkClient, + Names: names.NewResolver(sdkClient, authMgr, cfg.AccountID), + Output: output.New(output.Options{Format: output.FormatJSON, Writer: &bytes.Buffer{}}), + } + + cmd := NewTodosCmd() + cmd.SetIn(strings.NewReader("Call the vendor back\n")) + + err := executeTodosCommand(cmd, app, "create", "-") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBody) + + var body map[string]any + require.NoError(t, json.Unmarshal(transport.capturedBody, &body)) + assert.Equal(t, "Call the vendor back", body["content"], + "the trailing newline must be trimmed from a piped title") +} + +// Boost content from stdin: the trailing newline is trimmed before the 16-rune +// limit is applied, so a printf'd emoji doesn't burn a rune. +func TestBoostCreateDashReadsStdin(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + + transport := &mockBoostTransport{} + app, _ := newBoostTestApp(transport) + + cmd := NewBoostsCmd() + cmd.SetIn(strings.NewReader("🎉\n")) + + err := executeBoostCommand(cmd, app, "create", "456", "-") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBody) + + var body map[string]any + require.NoError(t, json.Unmarshal(transport.capturedBody, &body)) + assert.Equal(t, "🎉", body["content"]) +} + +func TestBoostCreateDashOverLimitStillRejected(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + + transport := &mockBoostTransport{} + app, _ := newBoostTestApp(transport) + + cmd := NewBoostsCmd() + cmd.SetIn(strings.NewReader("seventeen chars!!\n")) + + err := executeBoostCommand(cmd, app, "create", "456", "-") + require.Error(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.Contains(t, e.Message, "Boost content too long") +} + +// Pattern 3 on an update: todos update --description -. +func TestTodosUpdateDescriptionDashReadsStdin(t *testing.T) { + transport := &mockCommentWriteTransport{} + app, _ := setupCommentsWriteTestApp(t, transport) + + cmd := NewTodosCmd() + cmd.SetIn(strings.NewReader("New **details**\n")) + + err := executeCommand(cmd, app, "update", "789", "--description", "-") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBodies) + + found := false + for _, captured := range transport.capturedBodies { + var body map[string]any + if json.Unmarshal(captured, &body) == nil { + if desc, _ := body["description"].(string); strings.Contains(desc, "<strong>details</strong>") { + found = true + } + } + } + assert.True(t, found, "the piped description should reach the wire as HTML") +} + +// countingTransport records whether any request escaped the command. +type countingTransport struct{ calls int } + +func (t *countingTransport) RoundTrip(*http.Request) (*http.Response, error) { + t.calls++ + return nil, errors.New("network disabled in tests") +} + +// trackingReader records whether stdin was ever read. +type trackingReader struct { + r *strings.Reader + read bool +} + +func (t *trackingReader) Read(p []byte) (int, error) { + t.read = true + return t.r.Read(p) +} + +func setupTransportTestApp(t *testing.T, transport http.RoundTripper) *appctx.App { + t.Helper() + t.Setenv("BASECAMP_NO_KEYRING", "1") + + cfg := &config.Config{AccountID: "99999", ProjectID: "123"} + authMgr := auth.NewManager(cfg, nil) + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &testTokenProvider{}, + basecamp.WithTransport(transport), + basecamp.WithMaxRetries(1), + ) + return &appctx.App{ + Config: cfg, + Auth: authMgr, + SDK: sdkClient, + Names: names.NewResolver(sdkClient, authMgr, cfg.AccountID), + Output: output.New(output.Options{Format: output.FormatJSON, Writer: &bytes.Buffer{}}), + } +} + +// The <title> [body] creates bound their arity, so a stray trailing token is a +// usage error at Args-validation time — before "-" drains stdin and before any +// request is built. Without the bound the extra token was silently dropped +// *after* stdin had already been consumed. +func TestExactPositionalCreatesRejectExtraArgsBeforeConsumingStdin(t *testing.T) { + for _, tc := range []struct { + name string + cmd func() *cobra.Command + path []string + }{ + {"messages", NewMessagesCmd, []string{"create"}}, + {"cards", NewCardsCmd, []string{"create"}}, + {"docs", NewDocsCmd, []string{"documents", "create"}}, + } { + t.Run(tc.name, func(t *testing.T) { + transport := &countingTransport{} + app := setupTransportTestApp(t, transport) + + stdin := &trackingReader{r: strings.NewReader("body from stdin")} + cmd := tc.cmd() + InstallDashGuard(cmd) + cmd.SetIn(stdin) + + args := append(append([]string{}, tc.path...), "Title", "-", "unexpected") + err := executeCommand(cmd, app, args...) + require.Error(t, err) + assert.Contains(t, err.Error(), "accepts at most 2 arg") + assert.False(t, stdin.read, "stdin must not be consumed before arity validation") + assert.Zero(t, transport.calls, "no request may be issued") + }) + } +} + +// A flag-borne "-" with nothing piped must suggest an escape that parses: +// "api post ... --data -", never a bare positional "-" (api post takes one). +func TestAPIPostDataDashOnTTYHintPreservesTheFlag(t *testing.T) { + transport := &countingTransport{} + app := setupTransportTestApp(t, transport) + + cmd := NewAPICmd() + InstallDashGuard(cmd) + devNullStdin(t, cmd) + + err := executeCommand(cmd, app, "post", "/buckets/1/todos.json", "--data", "-") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Hint, "api post ... --data -") + assert.Zero(t, transport.calls) +} + +// setupNoAccountApp builds an app with no account configured, so any command +// that reaches account resolution fails with "--account is required". +func setupNoAccountApp(t *testing.T, transport http.RoundTripper) *appctx.App { + t.Helper() + t.Setenv("BASECAMP_NO_KEYRING", "1") + + cfg := &config.Config{} + authMgr := auth.NewManager(cfg, nil) + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &testTokenProvider{}, + basecamp.WithTransport(transport), + basecamp.WithMaxRetries(1), + ) + return &appctx.App{ + Config: cfg, + Auth: authMgr, + SDK: sdkClient, + Names: names.NewResolver(sdkClient, authMgr, cfg.AccountID), + Output: output.New(output.Options{Format: output.FormatJSON, Writer: &bytes.Buffer{}}), + } +} + +// Every "-" must be diagnosed before account, project or network work, so the +// caller gets the stdin error the feature promises instead of "--account is +// required" — or, worse, a resolution round-trip for an invocation that was +// never going to run. Driven on a TTY stdin because that error is produced by +// the resolver itself, which pins where in the sequence it ran. +func TestStdinResolvesBeforeAccountAndProject(t *testing.T) { + for _, tc := range []struct { + name string + cmd func() *cobra.Command + args []string + }{ + {"cards update --body", NewCardsCmd, []string{"update", "1", "--body", "-"}}, + {"docs create [content]", NewDocsCmd, []string{"documents", "create", "Title", "-"}}, + {"gauges create --description", NewGaugesCmd, []string{"create", "--position", "50", "--description", "-"}}, + {"gauges update --description", NewGaugesCmd, []string{"update", "1", "--description", "-"}}, + {"schedule create --description", NewScheduleCmd, []string{ + "create", "Title", "--starts-at", "2026-01-01T10:00:00Z", "--ends-at", "2026-01-01T11:00:00Z", "--description", "-", + }}, + {"templates update --description", NewTemplatesCmd, []string{"update", "1", "--description", "-"}}, + {"templates construct --description", NewTemplatesCmd, []string{"construct", "1", "--name", "P", "--description", "-"}}, + } { + t.Run(tc.name, func(t *testing.T) { + transport := &countingTransport{} + app := setupNoAccountApp(t, transport) + + cmd := tc.cmd() + InstallDashGuard(cmd) + devNullStdin(t, cmd) + + err := executeCommand(cmd, app, tc.args...) + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "nothing is piped", + "expected the stdin error, got %q", outErr.Message) + assert.NotContains(t, outErr.Message, "account") + assert.Zero(t, transport.calls, "no request may be issued") + }) + } +} + +// Two explicit content sources used to resolve by silent precedence: the +// positional won and --content was dropped, so "--content -" left the pipe +// unread and posted the positional instead. +func TestChatRejectsPositionalAlongsideContentFlag(t *testing.T) { + for _, tc := range []struct { + name string + args []string + }{ + {"post", []string{"post", "hello", "--content", "-"}}, + {"update", []string{"update", "123", "hello", "--content", "-"}}, + } { + t.Run(tc.name, func(t *testing.T) { + transport := &countingTransport{} + app := setupTransportTestApp(t, transport) + + stdin := &trackingReader{r: strings.NewReader("from stdin")} + cmd := NewChatCmd() + InstallDashGuard(cmd) + cmd.SetIn(stdin) + + err := executeCommand(cmd, app, tc.args...) + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "--content") + assert.False(t, stdin.read, "the discarded source must not consume stdin") + assert.Zero(t, transport.calls) + }) + } +} + +// A malformed target ID is knowable from the arguments alone, so it must be +// reported without first draining the pipe: reading blocks on the producer, and +// a blank pipe would answer "stdin is empty" instead of naming the bad ID. +func TestMalformedIDRejectedBeforeReadingStdin(t *testing.T) { + for _, tc := range []struct { + name string + cmd func() *cobra.Command + args []string + }{ + {"cards update", NewCardsCmd, []string{"update", "nope", "--body", "-"}}, + {"cards column update", NewCardsCmd, []string{"column", "update", "nope", "--description", "-"}}, + {"gauges update", NewGaugesCmd, []string{"update", "nope", "--description", "-"}}, + {"templates update", NewTemplatesCmd, []string{"update", "nope", "--description", "-"}}, + {"templates construct", NewTemplatesCmd, []string{"construct", "nope", "--name", "P", "--description", "-"}}, + {"messages update", NewMessagesCmd, []string{"update", "nope", "--body", "-"}}, + {"todos update", NewTodosCmd, []string{"update", "nope", "--description", "-"}}, + {"todolists update", NewTodolistsCmd, []string{"update", "nope", "--description", "-"}}, + {"projects update", NewProjectsCmd, []string{"update", "nope", "--description", "-"}}, + {"comments update", NewCommentsCmd, []string{"update", "nope", "-"}}, + {"files update", NewFilesCmd, []string{"update", "nope", "--content", "-"}}, + } { + t.Run(tc.name, func(t *testing.T) { + transport := &countingTransport{} + app := setupTransportTestApp(t, transport) + + stdin := &trackingReader{r: strings.NewReader("body from stdin")} + cmd := tc.cmd() + InstallDashGuard(cmd) + cmd.SetIn(stdin) + + err := executeCommand(cmd, app, tc.args...) + outErr := requireUsageErr(t, err) + assert.Contains(t, strings.ToLower(outErr.Message), "invalid", + "expected the malformed-ID error, got %q", outErr.Message) + assert.False(t, stdin.read, "stdin must not be drained before the ID is validated") + assert.Zero(t, transport.calls, "no request may be issued") + }) + } +} + +// An invocation that is already doomed by its flags or arguments must be +// rejected before the pipe is drained. Otherwise the caller waits on a producer +// whose output is discarded, an unbounded one buffers into memory, and a blank +// one answers "stdin is empty" instead of naming the real problem. +func TestDeterministicFailuresRejectedBeforeReadingStdin(t *testing.T) { + for _, tc := range []struct { + name string + cmd func() *cobra.Command + args []string + want string + }{ + {"api foreign host", NewAPICmd, + []string{"post", "https://evil.example/x", "--data", "-"}, "configured host"}, + {"chat post bad room", NewChatCmd, + []string{"post", "-", "--room", "nope"}, "Invalid chat room ID"}, + {"chat post bad content-type", NewChatCmd, + []string{"post", "-", "--content-type", "bogus"}, "unsupported --content-type"}, + {"todos update bad due date", NewTodosCmd, + []string{"update", "1", "--due", "not-a-date", "--description", "-"}, "Invalid due date"}, + {"chat update bad content-type", NewChatCmd, + []string{"update", "1", "-", "--content-type", "bogus"}, "unsupported --content-type"}, + {"boost bad id", NewBoostsCmd, + []string{"create", "nope", "-"}, "Invalid ID"}, + {"boost bad event", NewBoostsCmd, + []string{"create", "1", "-", "--event", "nope"}, "Invalid event ID"}, + {"checkins bad question", NewCheckinsCmd, + []string{"answer", "create", "nope", "-"}, "Invalid question ID"}, + {"checkins bad answer", NewCheckinsCmd, + []string{"answer", "update", "nope", "-"}, "Invalid answer ID"}, + {"files bad type", NewFilesCmd, + []string{"update", "1", "--type", "nonsense", "--content", "-"}, "Invalid type"}, + {"todos sweep without a filter", NewTodosCmd, + []string{"sweep", "--comment", "-"}, "requires a filter"}, + {"todos loose with a list", NewTodosCmd, + []string{"create", "-", "--loose", "--list", "123"}, "cannot be combined with --list"}, + {"gauges custom notify", NewGaugesCmd, + []string{"create", "--position", "50", "--notify", "custom", "--description", "-"}, "--subscriptions required"}, + {"docs subscribe conflict", NewDocsCmd, + []string{"documents", "create", "Title", "-", "--subscribe", "me", "--no-subscribe"}, "mutually exclusive"}, + {"messages subscribe conflict", NewMessagesCmd, + []string{"create", "Title", "-", "--subscribe", "me", "--no-subscribe"}, "mutually exclusive"}, + {"messages bad attachment", NewMessagesCmd, + []string{"create", "Title", "-", "--attach", "/nope/missing.png"}, "missing.png"}, + {"cards named column without a table", NewCardsCmd, + []string{"create", "Title", "-", "--column", "Backlog"}, "--card-table is required"}, + {"files replace untrusted host", NewFilesCmd, + []string{"replace", "https://evil.example/123/buckets/456/uploads/789", "stdin.go", "--description", "-"}, "untrusted host"}, + {"files vault with content", NewFilesCmd, + []string{"update", "123", "--type", "vault", "--content", "-"}, "--content can only be used"}, + {"schedule bad timestamp", NewScheduleCmd, + []string{"update", "123", "--starts-at", "invalid", "--description", "-"}, "starts-at"}, + {"cards update bad attachment", NewCardsCmd, + []string{"update", "1", "--body", "-", "--attach", "/nope/missing.png"}, "missing.png"}, + {"chat update untrusted host", NewChatCmd, + []string{"update", "https://evil.example/1/buckets/2/chats/3/lines/4", "-"}, "untrusted host"}, + {"chat update wrong recording type", NewChatCmd, + []string{"update", "https://3.basecamp.com/1/buckets/2/todos/4", "-"}, "expected a chat-line"}, + {"chat update bad line id", NewChatCmd, + []string{"update", "nope", "-"}, "Invalid chat line ID"}, + {"files replace wrong recording type", NewFilesCmd, + []string{"replace", "https://3.basecamp.com/1/buckets/2/todos/4", "stdin.go", "--description", "-"}, "not an upload"}, + {"schedule create bad schedule id", NewScheduleCmd, + []string{"create", "Title", "--starts-at", "2026-01-01T10:00:00Z", "--ends-at", "2026-01-01T11:00:00Z", "--schedule", "nope", "--description", "-"}, "Invalid schedule ID"}, + {"messages create bad board id", NewMessagesCmd, + []string{"create", "Title", "-", "--message-board", "nope"}, "Invalid message board ID"}, + {"docs create bad folder id", NewDocsCmd, + []string{"documents", "create", "Title", "-", "--folder", "nope"}, "Invalid folder ID"}, + {"todolists create bad todoset id", NewTodolistsCmd, + []string{"create", "Title", "--description", "-", "--todoset", "nope"}, "Invalid todoset ID"}, + {"cards column create bad table id", NewCardsCmd, + []string{"column", "create", "Title", "--description", "-", "--card-table", "nope"}, "Invalid card table ID"}, + {"todos create bad todoset id", NewTodosCmd, + []string{"create", "-", "--todoset", "nope"}, "Invalid todoset ID"}, + {"schedule bad entry id", NewScheduleCmd, + []string{"update", "nope", "--description", "-"}, "Invalid schedule entry ID"}, + {"uploads create bad folder id", NewUploadsCmd, + []string{"create", "stdin.go", "--folder", "nope", "--description", "-"}, "Invalid folder ID"}, + {"upload shortcut bad folder id", NewUploadCmd, + []string{"stdin.go", "--folder", "nope", "--description", "-"}, "Invalid folder ID"}, + {"uploads unreadable file", NewUploadsCmd, + []string{"create", "/nope/missing.txt", "--description", "-"}, "missing.txt"}, + } { + t.Run(tc.name, func(t *testing.T) { + transport := &countingTransport{} + app := setupTransportTestApp(t, transport) + + stdin := &trackingReader{r: strings.NewReader("body from stdin")} + cmd := tc.cmd() + InstallDashGuard(cmd) + cmd.SetIn(stdin) + + err := executeCommand(cmd, app, tc.args...) + require.Error(t, err) + assert.Contains(t, err.Error(), tc.want) + assert.False(t, stdin.read, "stdin must not be drained for an invocation that cannot succeed") + assert.Zero(t, transport.calls, "no request may be issued") + }) + } +} + +// The --loose gate has to see a configured todolist too, not just the local +// --list flag: a global --todolist arrives via app.Flags and used to be caught +// only after the pipe was drained and the project resolved. +func TestTodosLooseRejectsAConfiguredTodolistBeforeReadingStdin(t *testing.T) { + transport := &countingTransport{} + app := setupTransportTestApp(t, transport) + app.Flags.Todolist = "123" + + stdin := &trackingReader{r: strings.NewReader("body from stdin")} + cmd := NewTodosCmd() + InstallDashGuard(cmd) + cmd.SetIn(stdin) + + err := executeCommand(cmd, app, "create", "-", "--loose") + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot be combined with --list") + assert.False(t, stdin.read, "stdin must not be drained for an invocation that cannot succeed") + assert.Zero(t, transport.calls) +} diff --git a/internal/commands/stdin_ordering_test.go b/internal/commands/stdin_ordering_test.go new file mode 100644 index 000000000..8ec6ef00a --- /dev/null +++ b/internal/commands/stdin_ordering_test.go @@ -0,0 +1,154 @@ +package commands_test + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Reading stdin is the one step in a RunE that cannot be undone and cannot be +// hurried: it blocks until the producer is done. Everything that can reject the +// invocation from the arguments alone — parsing the target ID out of args[0], +// validating a file path — has to happen first, or a typo'd ID waits on a slow +// pipe and a blank one reports "stdin is empty" instead of "Invalid card ID". +// +// This was fixed at thirteen call sites by hand; the ordering is a property of +// every future one too, which is what this test holds. It reads the AST rather +// than running the commands, so it covers call sites no test exercises — but +// only syntactic forms it can recognize, listed in syntacticArgUse below. +func TestSyntacticArgChecksPrecedeStdinReads(t *testing.T) { + dir := "." + entries, err := os.ReadDir(dir) + require.NoError(t, err) + + fset := token.NewFileSet() + var checked int + + for _, entry := range entries { + name := entry.Name() + if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + file, err := parser.ParseFile(fset, filepath.Join(dir, name), nil, 0) + require.NoError(t, err) + + ast.Inspect(file, func(n ast.Node) bool { + kv, ok := n.(*ast.KeyValueExpr) + if !ok { + return true + } + if key, ok := kv.Key.(*ast.Ident); !ok || (key.Name != "RunE" && key.Name != "PreRunE") { + return true + } + + firstStdinRead := token.NoPos + var lateChecks []token.Pos + ast.Inspect(kv.Value, func(inner ast.Node) bool { + call, ok := inner.(*ast.CallExpr) + if !ok { + return true + } + switch { + case stdinResolver(call): + if !firstStdinRead.IsValid() { + firstStdinRead = call.Pos() + } + case deterministicCheck(call): + // Every recognized check must precede the read, not just + // the earliest one: comparing first-to-first lets a single + // early check hide every later one. + if firstStdinRead.IsValid() && call.Pos() > firstStdinRead { + lateChecks = append(lateChecks, call.Pos()) + } + } + return true + }) + + if !firstStdinRead.IsValid() { + return true + } + checked++ + for _, late := range lateChecks { + assert.Fail(t, "stdin is read before a check that decides the invocation", + "%s: reads stdin at %s but checks its arguments at %s — hoist the check above the resolver", + name, fset.Position(firstStdinRead), fset.Position(late)) + } + return true + }) + } + + require.Greater(t, checked, 10, "expected many commands to both read stdin and check args") +} + +// stdinResolver matches the two functions that can read from stdin. +func stdinResolver(call *ast.CallExpr) bool { + name, ok := call.Fun.(*ast.Ident) + return ok && (name.Name == "resolveContentValue" || name.Name == "resolveContentArg") +} + +// deterministicCheck matches a call that can decide the invocation without the +// network: a syntactic use of args, or one of the local validation helpers. +// Recognizing a form by name is exactly as wide as the names listed; a new +// helper needs adding here, which is why the coverage claim above is bounded. +// +// Normalizers and branch selectors are deliberately absent. dateparse.Parse, +// isNumericID and urlarg.IsURL cannot reject an invocation, so listing them +// would report branch selection as if it were validation, and the noise would +// be answered by suppressions instead of fixes. +func deterministicCheck(call *ast.CallExpr) bool { + if id, ok := call.Fun.(*ast.Ident); ok { + switch id.Name { + case "validateAttachPaths", "validateUploadPath", "validateScheduleTimestamp", + "rejectSubscribeConflict", "rejectForeignAPIPath", "requireNumericID": + return true + } + } + if sel, ok := call.Fun.(*ast.SelectorExpr); ok { + if pkg, ok := sel.X.(*ast.Ident); ok { + switch pkg.Name + "." + sel.Sel.Name { + case "hostutil.IsTrustedBasecampHost", "urlarg.Parse": + return true + } + } + } + return syntacticArgUse(call) +} + +// syntacticArgUse matches a call that derives something from args without any +// account, config, or network dependency. +func syntacticArgUse(call *ast.CallExpr) bool { + name, ok := call.Fun.(*ast.Ident) + if !ok { + if sel, isSel := call.Fun.(*ast.SelectorExpr); isSel { + pkg, isPkg := sel.X.(*ast.Ident) + if !isPkg || pkg.Name != "strconv" || sel.Sel.Name != "ParseInt" { + return false + } + } else { + return false + } + } else { + switch name.Name { + case "extractID", "extractWithProject", "extractCommentWithProject": + default: + return false + } + } + + // Only when it reads the positional arguments directly. + for _, arg := range call.Args { + if index, ok := arg.(*ast.IndexExpr); ok { + if ident, ok := index.X.(*ast.Ident); ok && ident.Name == "args" { + return true + } + } + } + return false +} diff --git a/internal/commands/stdin_test.go b/internal/commands/stdin_test.go new file mode 100644 index 000000000..229d81e54 --- /dev/null +++ b/internal/commands/stdin_test.go @@ -0,0 +1,184 @@ +package commands + +import ( + "errors" + "os" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/output" +) + +// devNullStdin wires a command's stdin to /dev/null, a character device — the +// established TTY stand-in (see edit_test.go). +func devNullStdin(t *testing.T, cmd *cobra.Command) { + t.Helper() + devNull, err := os.Open(os.DevNull) + require.NoError(t, err) + t.Cleanup(func() { devNull.Close() }) + cmd.SetIn(devNull) +} + +func requireUsageErr(t *testing.T, err error) *output.Error { + t.Helper() + require.Error(t, err) + var outErr *output.Error + require.True(t, errors.As(err, &outErr), "expected *output.Error, got %T: %v", err, err) + assert.Equal(t, output.CodeUsage, outErr.Code) + return outErr +} + +func TestReadStdinContentTrimsTrailingNewlines(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + cmd.SetIn(strings.NewReader("🎉\n")) + + content, err := readStdinContent(cmd, "<content>") + require.NoError(t, err) + assert.Equal(t, "🎉", content) +} + +// CRLF pipes (Windows tools, curl -w) must not leave a stray \r behind — it +// would count against boost's 16-rune limit and corrupt titles. +func TestReadStdinContentTrimsTrailingCRLF(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + cmd.SetIn(strings.NewReader("exactly16chars!!\r\n")) + + content, err := readStdinContent(cmd, "<content>") + require.NoError(t, err) + assert.Equal(t, "exactly16chars!!", content) +} + +func TestReadStdinContentTTYIsUsageErrorWithEscapeHints(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + devNullStdin(t, cmd) + + _, err := readStdinContent(cmd, "<content>") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "nothing is piped") + assert.Contains(t, outErr.Hint, "heredoc") + assert.NotContains(t, outErr.Hint, "--edit", "no --edit flag on this command") +} + +// The hint must name the input that actually carried the "-". Suggesting a +// bare trailing "-" for a flag-borne dash would exceed the command's +// positional arity — an escape the caller cannot use. +func TestReadStdinContentTTYHintNamesTheFlag(t *testing.T) { + cmd := &cobra.Command{Use: "post <path>"} + devNullStdin(t, cmd) + + _, err := readStdinContent(cmd, "--data") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Hint, "--data -") + assert.NotContains(t, outErr.Hint, "... -)", "a bare positional dash would break arity") +} + +func TestReadStdinContentTTYHintMentionsEditWhenAvailable(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + cmd.Flags().Bool("edit", false, "") + devNullStdin(t, cmd) + + _, err := readStdinContent(cmd, "<content>") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Hint, "--edit") +} + +func TestReadStdinContentBlankPipeIsUsageError(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + cmd.SetIn(strings.NewReader(" \n\n")) + + _, err := readStdinContent(cmd, "<content>") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "empty") +} + +func TestResolveContentArgJoinsLiteralArgs(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + content, err := resolveContentArg(cmd, []string{"hello", "world"}, 1) + require.NoError(t, err) + assert.Equal(t, "hello world", content) +} + +func TestResolveContentArgLoneDashReadsStdin(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + cmd.SetIn(strings.NewReader("from stdin\n")) + + content, err := resolveContentArg(cmd, []string{"-"}, 1) + require.NoError(t, err) + assert.Equal(t, "from stdin", content) +} + +func TestResolveContentArgDashAmongOthersIsUsageError(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + cmd.SetIn(strings.NewReader("from stdin")) + + _, err := resolveContentArg(cmd, []string{"-", "extra"}, 1) + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "only") +} + +// A "-" placed after the -- separator is literal: parsed through a real +// Execute so ArgsLenAtDash is set. +func TestResolveContentArgDashAfterSeparatorIsLiteral(t *testing.T) { + var content string + cmd := &cobra.Command{ + Use: "x <id> <content>", + RunE: func(cmd *cobra.Command, args []string) error { + var err error + content, err = resolveContentArg(cmd, args[1:], 1) + return err + }, + } + cmd.SetIn(strings.NewReader("must not be read")) + cmd.SetArgs([]string{"123", "--", "-"}) + cmd.SetOut(&strings.Builder{}) + cmd.SetErr(&strings.Builder{}) + + require.NoError(t, cmd.Execute()) + assert.Equal(t, "-", content) +} + +func TestResolveContentValueFlagDashReadsStdin(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + cmd.SetIn(strings.NewReader("flag body\n")) + + content, err := resolveContentValue(cmd, "-", -1, "--data") + require.NoError(t, err) + assert.Equal(t, "flag body", content) +} + +func TestResolveContentValueLiteralPassesThrough(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + content, err := resolveContentValue(cmd, "plain", -1, "--data") + require.NoError(t, err) + assert.Equal(t, "plain", content) +} + +func TestResolveContentValuePositionalDashAfterSeparatorIsLiteral(t *testing.T) { + var body string + cmd := &cobra.Command{ + Use: "x <title> [body]", + RunE: func(cmd *cobra.Command, args []string) error { + var err error + body, err = resolveContentValue(cmd, args[1], 1, "[body]") + return err + }, + } + cmd.SetIn(strings.NewReader("must not be read")) + cmd.SetArgs([]string{"--", "title", "-"}) + cmd.SetOut(&strings.Builder{}) + cmd.SetErr(&strings.Builder{}) + + require.NoError(t, cmd.Execute()) + assert.Equal(t, "-", body) +} + +func TestAllowDashMergesTokens(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + allowDash(cmd, "arg:0") + allowDash(cmd, "flag:description") + assert.Equal(t, "arg:0 flag:description", cmd.Annotations["allow_dash"]) +} diff --git a/internal/commands/templates.go b/internal/commands/templates.go index 654ba376b..80ddcce58 100644 --- a/internal/commands/templates.go +++ b/internal/commands/templates.go @@ -201,6 +201,11 @@ func newTemplatesCreateCmd() *cobra.Command { app := appctx.FromContext(cmd.Context()) + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + if err := ensureAccount(cmd, app); err != nil { return err } @@ -234,9 +239,11 @@ func newTemplatesCreateCmd() *cobra.Command { } cmd.Flags().StringVar(&name, "name", "", "Template name") - cmd.Flags().StringVar(&description, "description", "", "Template description") + cmd.Flags().StringVar(&description, "description", "", "Template description; use - to read from stdin") cmd.Flags().StringVar(&description, "desc", "", "Template description (alias)") + allowDash(cmd, "flag:description", "flag:desc") + return cmd } @@ -250,10 +257,8 @@ func newTemplatesUpdateCmd() *cobra.Command { Long: "Update an existing template's name or description.", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - app := appctx.FromContext(cmd.Context()) - - if err := ensureAccount(cmd, app); err != nil { - return err + if name == "" && description == "" { + return noChanges(cmd) } templateID, err := strconv.ParseInt(args[0], 10, 64) @@ -261,8 +266,16 @@ func newTemplatesUpdateCmd() *cobra.Command { return output.ErrUsage("Invalid template ID") } - if name == "" && description == "" { - return noChanges(cmd) + // Syntactic checks first, then "-", then account and network. + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + + app := appctx.FromContext(cmd.Context()) + + if err := ensureAccount(cmd, app); err != nil { + return err } // SDK requires name for update, fetch current if not provided @@ -299,9 +312,11 @@ func newTemplatesUpdateCmd() *cobra.Command { } cmd.Flags().StringVar(&name, "name", "", "New name") - cmd.Flags().StringVar(&description, "description", "", "New description") + cmd.Flags().StringVar(&description, "description", "", "New description; use - to read from stdin") cmd.Flags().StringVar(&description, "desc", "", "New description (alias)") + allowDash(cmd, "flag:description", "flag:desc") + return cmd } @@ -360,10 +375,8 @@ This is an asynchronous operation. The command returns a construction ID which can be polled via 'templates construction' until the status is "completed".`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - app := appctx.FromContext(cmd.Context()) - - if err := ensureAccount(cmd, app); err != nil { - return err + if projectName == "" { + return output.ErrUsage("--name is required (project name)") } templateID, err := strconv.ParseInt(args[0], 10, 64) @@ -371,8 +384,16 @@ which can be polled via 'templates construction' until the status is "completed" return output.ErrUsage("Invalid template ID") } - if projectName == "" { - return output.ErrUsage("--name is required (project name)") + // Syntactic checks first, then "-", then account and network. + projectDesc, err := resolveContentValue(cmd, projectDesc, -1, "--description") + if err != nil { + return err + } + + app := appctx.FromContext(cmd.Context()) + + if err := ensureAccount(cmd, app); err != nil { + return err } construction, err := app.Account().Templates().CreateProject(cmd.Context(), templateID, projectName, projectDesc) @@ -394,10 +415,12 @@ which can be polled via 'templates construction' until the status is "completed" } cmd.Flags().StringVar(&projectName, "name", "", "Project name (required)") - cmd.Flags().StringVar(&projectDesc, "description", "", "Project description") + cmd.Flags().StringVar(&projectDesc, "description", "", "Project description; use - to read from stdin") cmd.Flags().StringVar(&projectDesc, "desc", "", "Project description (alias)") _ = cmd.MarkFlagRequired("name") + allowDash(cmd, "flag:description", "flag:desc") + return cmd } diff --git a/internal/commands/todolists.go b/internal/commands/todolists.go index 9405e69a7..dbd20f635 100644 --- a/internal/commands/todolists.go +++ b/internal/commands/todolists.go @@ -288,6 +288,15 @@ func newTodolistsCreateCmd(project, todosetID *string) *cobra.Command { return fmt.Errorf("app not initialized") } + if err := requireNumericID(*todosetID, "todoset ID"); err != nil { + return err + } + + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + if err := ensureAccount(cmd, app); err != nil { return err } @@ -366,9 +375,11 @@ func newTodolistsCreateCmd(project, todosetID *string) *cobra.Command { } cmd.Flags().StringVarP(todosetID, "todoset", "t", "", "Todoset ID (for projects with multiple todosets)") - cmd.Flags().StringVarP(&description, "description", "d", "", "Todolist description") + cmd.Flags().StringVarP(&description, "description", "d", "", "Todolist description; use - to read from stdin") cmd.Flags().BoolVar(&visibleToClients, "visible-to-clients", false, "Make the todolist visible to clients on the project (omit for the server default; client-authenticated callers always post client-visible)") + allowDash(cmd, "flag:description") + return cmd } @@ -395,12 +406,26 @@ You can pass either a todolist ID or a Basecamp URL: return fmt.Errorf("app not initialized") } - if err := ensureAccount(cmd, app); err != nil { + // Extract ID and project from URL if provided + todolistIDStr, urlProjectID := extractWithProject(args[0]) + + // Parse todolist ID as int64 + todolistID, err := strconv.ParseInt(todolistIDStr, 10, 64) + if err != nil { + return output.ErrUsage("Invalid todolist ID") + } + + // Syntactic checks first, then "-", then account and network: a + // malformed ID is answered without waiting on the producer, and a + // blank pipe cannot mask it. + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { return err } - // Extract ID and project from URL if provided - todolistIDStr, urlProjectID := extractWithProject(args[0]) + if err := ensureAccount(cmd, app); err != nil { + return err + } // Resolve project - use URL > flag > config, with interactive fallback projectID := *project @@ -419,12 +444,6 @@ You can pass either a todolist ID or a Basecamp URL: } } - // Parse todolist ID as int64 - todolistID, err := strconv.ParseInt(todolistIDStr, 10, 64) - if err != nil { - return output.ErrUsage("Invalid todolist ID") - } - // Build SDK request req := &basecamp.UpdateTodolistRequest{ Name: name, @@ -457,7 +476,9 @@ You can pass either a todolist ID or a Basecamp URL: } cmd.Flags().StringVarP(&name, "name", "n", "", "New name") - cmd.Flags().StringVarP(&description, "description", "d", "", "New description") + cmd.Flags().StringVarP(&description, "description", "d", "", "New description; use - to read from stdin") + + allowDash(cmd, "flag:description") return cmd } diff --git a/internal/commands/todos.go b/internal/commands/todos.go index 6850ed403..560930639 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -1243,7 +1243,10 @@ project's to-do set instead, outside any list: basecamp todos create "Call the vendor back" --loose --in <project> ---loose needs no list, so it neither prompts for one nor accepts --list.`, +--loose needs no list, so it neither prompts for one nor accepts --list. + +Use - as the content argument to read the todo title from stdin: + printf 'Call the vendor back' | basecamp todos create - --in <project>`, RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) if app == nil { @@ -1254,11 +1257,42 @@ project's to-do set instead, outside any list: if len(args) == 0 { return missingArg(cmd, "<content>") } - content := strings.Join(args, " ") + + // --loose and a named list are mutually exclusive, and that is + // knowable from the flags alone. Decide it before the pipe is + // drained: a doomed invocation should not make the caller wait on a + // producer, and a blank pipe must not answer "stdin is empty" + // instead of naming the conflict. The destination resolution below + // still repeats the check, since a configured todolist is only one + // of its inputs. + if loose && (cmd.Flags().Changed("list") || todolist != "" || app.Flags.Todolist != "") { + return output.ErrUsageHint( + "--loose creates a todo outside any list, so it cannot be combined with --list", + "Drop --list to create on the to-do set, or drop --loose to create in that list") + } + + // Attachment paths are readable or not regardless of the body, so + // check them before the pipe is drained. + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + if err := requireNumericID(todoset, "todoset ID"); err != nil { + return err + } + + content, err := resolveContentArg(cmd, args, 0) + if err != nil { + return err + } if strings.TrimSpace(content) == "" { return cmd.Help() } + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + if err := ensureAccount(cmd, app); err != nil { return err } @@ -1437,13 +1471,15 @@ project's to-do set instead, outside any list: cmd.Flags().StringVar(&assignee, "assignee", "", "Assignee ID") cmd.Flags().StringVar(&assignee, "to", "", "Assignee ID (alias for --assignee)") cmd.Flags().StringVarP(&due, "due", "d", "", "Due date (YYYY-MM-DD)") - cmd.Flags().StringVar(&description, "description", "", "Extended description (Markdown)") + cmd.Flags().StringVar(&description, "description", "", "Extended description (Markdown); use - to read from stdin") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") cmd.Flags().StringVar(¬ifyOnCompletion, "notify-on-completion", "", "People to notify when done (names or IDs, comma-separated)") // Not --todoset: that flag already means "which to-do set", and this one // means "no list at all". cmd.Flags().BoolVar(&loose, "loose", false, "Create on the to-do set, outside any list") + allowDash(cmd, "arg:0+", "flag:description") + // Register tab completion for flags completer := completion.NewCompleter(nil) _ = cmd.RegisterFlagCompletionFunc("project", completer.ProjectNameCompletion()) @@ -1537,15 +1573,6 @@ Set or clear the people notified when the todo is completed: return noChanges(cmd) } - app := appctx.FromContext(cmd.Context()) - if app == nil { - return fmt.Errorf("app not initialized") - } - - if err := ensureAccount(cmd, app); err != nil { - return err - } - // Extract ID from URL if provided todoIDStr := extractID(args[0]) todoID, err := strconv.ParseInt(todoIDStr, 10, 64) @@ -1553,14 +1580,9 @@ Set or clear the people notified when the todo is completed: return output.ErrUsage("Invalid todo ID") } - // Pre-Edit validation and resolution — no todo HTTP happens here. - // Image uploads are deferred into the Edit closure so a missing - // todo can't orphan uploaded attachments. - var descHTML string - if !clearDescription && description != "" { - descHTML = richtext.MarkdownToHTML(description) - } - + // Date formats are decidable from the flags alone, so they join the + // ID check ahead of the read. The parsed values are carried forward + // rather than re-derived below. var parsedDue string if !clearDue && strings.TrimSpace(due) != "" { parsedDue = dateparse.Parse(due) @@ -1576,6 +1598,32 @@ Set or clear the people notified when the todo is completed: } } + // Syntactic checks first, then "-", then account and network: a + // malformed ID or date is answered without waiting on the producer, + // and a blank pipe cannot mask it. Only an exact "-" reads stdin; + // --description "" stays the clear idiom. + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + + app := appctx.FromContext(cmd.Context()) + if app == nil { + return fmt.Errorf("app not initialized") + } + + if err := ensureAccount(cmd, app); err != nil { + return err + } + + // Pre-Edit validation and resolution — no todo HTTP happens here. + // Image uploads are deferred into the Edit closure so a missing + // todo can't orphan uploaded attachments. + var descHTML string + if !clearDescription && description != "" { + descHTML = richtext.MarkdownToHTML(description) + } + var assigneeIDs []int64 if assigneeChanged { if assigneeIDs, err = resolveAssigneeIDs(cmd.Context(), app, assignee); err != nil { @@ -1664,7 +1712,7 @@ Set or clear the people notified when the todo is completed: } cmd.Flags().StringVarP(&title, "title", "t", "", "Todo title (plain text)") - cmd.Flags().StringVar(&description, "description", "", "Extended description (Markdown)") + cmd.Flags().StringVar(&description, "description", "", "Extended description (Markdown); use - to read from stdin") cmd.Flags().StringVar(&assignee, "assignee", "", "Assignees (names or IDs, comma-separated)") cmd.Flags().StringVar(&assignee, "to", "", "Assignees (alias for --assignee)") cmd.Flags().StringVarP(&due, "due", "d", "", "Due date (natural language or YYYY-MM-DD)") @@ -1682,6 +1730,8 @@ Set or clear the people notified when the todo is completed: _ = cmd.RegisterFlagCompletionFunc("to", completer.PeopleNameCompletion()) _ = cmd.RegisterFlagCompletionFunc("notify-on-completion", completer.PeopleNameCompletion()) + allowDash(cmd, "flag:description") + return cmd } @@ -1873,15 +1923,25 @@ Examples: basecamp todos sweep --in <project> --assignee me --comment "Following up"`, RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) - if err := ensureAccount(cmd, app); err != nil { - return err - } - // Require at least one filter + // Require at least one filter. This gate decides the invocation on + // its own, so it runs before the pipe is drained: otherwise the + // caller waits on a producer whose output is already discarded, and + // a blank pipe answers "stdin is empty" instead of naming the + // missing filter. if !overdueOnly && assignee == "" { return output.ErrUsageHint("Sweep requires a filter", "Use --overdue or --assignee to select todos") } + comment, err := resolveContentValue(cmd, comment, -1, "--comment") + if err != nil { + return err + } + + if err := ensureAccount(cmd, app); err != nil { + return err + } + // Require at least one action if comment == "" && !complete { return output.ErrUsageHint("Sweep requires an action", "Use --comment and/or --complete") @@ -2020,11 +2080,13 @@ Examples: cmd.Flags().StringVarP(&todoset, "todoset", "t", "", "Todoset ID (for projects with multiple todosets)") cmd.Flags().StringVar(&assignee, "assignee", "", "Filter by assignee") cmd.Flags().BoolVar(&overdueOnly, "overdue", false, "Filter overdue todos") - cmd.Flags().StringVarP(&comment, "comment", "c", "", "Comment to add to matching todos") + cmd.Flags().StringVarP(&comment, "comment", "c", "", "Comment to add to matching todos; use - to read from stdin") cmd.Flags().BoolVar(&complete, "complete", false, "Mark matching todos as complete") cmd.Flags().BoolVar(&complete, "done", false, "Mark matching todos as complete (alias)") cmd.Flags().BoolVarP(&dryRun, "dry-run", "n", false, "Preview without making changes") + allowDash(cmd, "flag:comment") + // Register tab completion for flags completer := completion.NewCompleter(nil) _ = cmd.RegisterFlagCompletionFunc("project", completer.ProjectNameCompletion()) diff --git a/internal/stdinarg/stdinarg.go b/internal/stdinarg/stdinarg.go new file mode 100644 index 000000000..193643bc8 --- /dev/null +++ b/internal/stdinarg/stdinarg.go @@ -0,0 +1,109 @@ +// Package stdinarg carries the shared vocabulary for "-" (read from stdin) +// argument handling: the cobra annotation that marks where a command accepts +// "-", and pipe detection for deciding whether a stray "-" is ambiguous. +// +// It is a leaf package because both internal/commands (which resolves "-" and +// installs the guard) and internal/cli (which surfaces the annotation in agent +// help) need the same annotation key, and cli already depends on commands. +package stdinarg + +import ( + "io" + "os" + "strconv" + "strings" +) + +// AnnotationAllowDash is the cmd.Annotations key marking where a command +// accepts "-" as "read from stdin". The value is a space-separated list of +// tokens: "arg:0" (exact positional index), "arg:1+" (that index and beyond), +// "flag:data" (the --data flag). Everything not listed is guarded: a literal +// "-" there combined with piped stdin is rejected as ambiguous. +const AnnotationAllowDash = "allow_dash" + +// Allow is the parsed form of an AnnotationAllowDash value. +type Allow struct { + args map[int]bool + argsFrom int // "arg:N+" allows every index >= argsFrom; -1 when absent + flags map[string]bool +} + +// ParseAllow parses a space-separated token list ("arg:0 arg:1+ flag:data") +// into an Allow. Unrecognized tokens are ignored rather than failing: the +// annotation is authored in-repo and covered by tests, so a typo shows up as +// a guarded (rejected) input, not a silent bypass. +func ParseAllow(s string) Allow { + allow := Allow{argsFrom: -1} + for _, token := range strings.Fields(s) { + switch { + case strings.HasPrefix(token, "arg:"): + spec := strings.TrimPrefix(token, "arg:") + open := strings.HasSuffix(spec, "+") + if n, err := strconv.Atoi(strings.TrimSuffix(spec, "+")); err == nil { + if open { + if allow.argsFrom == -1 || n < allow.argsFrom { + allow.argsFrom = n + } + } else { + if allow.args == nil { + allow.args = map[int]bool{} + } + allow.args[n] = true + } + } + case strings.HasPrefix(token, "flag:"): + if allow.flags == nil { + allow.flags = map[string]bool{} + } + allow.flags[strings.TrimPrefix(token, "flag:")] = true + } + } + return allow +} + +// Arg reports whether "-" is allowed at positional index i. +func (a Allow) Arg(i int) bool { + return a.args[i] || (a.argsFrom != -1 && i >= a.argsFrom) +} + +// Flag reports whether "-" is allowed as the named flag's value. +func (a Allow) Flag(name string) bool { + return a.flags[name] +} + +// Empty reports whether the Allow permits "-" nowhere. +func (a Allow) Empty() bool { + return len(a.args) == 0 && a.argsFrom == -1 && len(a.flags) == 0 +} + +// IsPiped reports whether the reader carries piped (redirected) input rather +// than an interactive terminal. A non-*os.File reader — the cmd.SetIn test +// seam — always counts as piped. For a real file, a character device means a +// terminal; anything else (pipe, regular file redirect) is piped input. +func IsPiped(r io.Reader) bool { + f, ok := r.(*os.File) + if !ok { + return true + } + fi, err := f.Stat() + if err != nil { + return false + } + return fi.Mode()&os.ModeCharDevice == 0 +} + +// InteractiveStdio reports whether both stdout and stdin are character +// devices — the floor for launching anything that draws to the terminal and +// reads keystrokes. A TUI (picker, wizard) reads key events from stdin, so a +// pipe or redirected file can never drive one — and when the command is +// consuming piped content (a "-" stdin input), a TUI would eat that content +// as key events. +func InteractiveStdio() bool { + for _, f := range []*os.File{os.Stdout, os.Stdin} { + fi, err := f.Stat() + if err != nil || fi.Mode()&os.ModeCharDevice == 0 { + return false + } + } + return true +} diff --git a/internal/stdinarg/stdinarg_test.go b/internal/stdinarg/stdinarg_test.go new file mode 100644 index 000000000..d80c06579 --- /dev/null +++ b/internal/stdinarg/stdinarg_test.go @@ -0,0 +1,105 @@ +package stdinarg + +import ( + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseAllowExactArg(t *testing.T) { + allow := ParseAllow("arg:0") + assert.True(t, allow.Arg(0)) + assert.False(t, allow.Arg(1)) + assert.False(t, allow.Flag("data")) + assert.False(t, allow.Empty()) +} + +func TestParseAllowOpenEndedArg(t *testing.T) { + allow := ParseAllow("arg:1+") + assert.False(t, allow.Arg(0)) + assert.True(t, allow.Arg(1)) + assert.True(t, allow.Arg(5)) +} + +func TestParseAllowFlags(t *testing.T) { + allow := ParseAllow("flag:data flag:out") + assert.True(t, allow.Flag("data")) + assert.True(t, allow.Flag("out")) + assert.False(t, allow.Flag("body")) + assert.False(t, allow.Arg(0)) +} + +func TestParseAllowMixed(t *testing.T) { + allow := ParseAllow("arg:0 arg:2+ flag:description") + assert.True(t, allow.Arg(0)) + assert.False(t, allow.Arg(1)) + assert.True(t, allow.Arg(2)) + assert.True(t, allow.Arg(3)) + assert.True(t, allow.Flag("description")) +} + +func TestParseAllowEmptyAndGarbage(t *testing.T) { + assert.True(t, ParseAllow("").Empty()) + assert.True(t, ParseAllow("arg:x bogus flag").Empty()) +} + +func TestIsPipedNonFileReader(t *testing.T) { + assert.True(t, IsPiped(strings.NewReader("piped"))) +} + +func TestIsPipedCharDevice(t *testing.T) { + devNull, err := os.Open(os.DevNull) + require.NoError(t, err) + defer devNull.Close() + + assert.False(t, IsPiped(devNull)) +} + +func TestIsPipedRegularFile(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "stdin") + require.NoError(t, err) + defer f.Close() + + assert.True(t, IsPiped(f)) +} + +// TestInteractiveStdio proves TUIs are gated off when stdin is piped: a +// wizard or picker reads keystrokes from stdin, so piped stdin would be +// consumed as key events — including piped content meant for a "-" input. +func TestInteractiveStdio(t *testing.T) { + devnull, err := os.OpenFile(os.DevNull, os.O_RDWR, 0) + if err != nil { + t.Fatalf("open %s: %v", os.DevNull, err) + } + defer devnull.Close() + + pipeR, pipeW, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + defer pipeR.Close() + defer pipeW.Close() + + origOut, origIn := os.Stdout, os.Stdin + t.Cleanup(func() { os.Stdout, os.Stdin = origOut, origIn }) + + // /dev/null is a character device, standing in for a terminal on both + // ends without needing a PTY. + os.Stdout, os.Stdin = devnull, devnull + if !InteractiveStdio() { + t.Fatal("expected interactive with char-device stdout and stdin") + } + + os.Stdin = pipeR + if InteractiveStdio() { + t.Fatal("expected non-interactive with piped stdin") + } + + os.Stdout, os.Stdin = pipeW, devnull + if InteractiveStdio() { + t.Fatal("expected non-interactive with piped stdout") + } +} diff --git a/internal/tui/resolve/resolve.go b/internal/tui/resolve/resolve.go index 3018737aa..65a9496dd 100644 --- a/internal/tui/resolve/resolve.go +++ b/internal/tui/resolve/resolve.go @@ -5,12 +5,12 @@ package resolve import ( "context" - "os" "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/basecamp/basecamp-cli/internal/auth" "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/stdinarg" "github.com/basecamp/basecamp-cli/internal/tui" ) @@ -101,11 +101,11 @@ func (r *Resolver) Flags() *Flags { } // IsInteractive returns true if interactive prompts can be shown. -// This checks both stdout and machine-output flags. +// This checks stdout, stdin, and machine-output flags. // Returns false if BASECAMP_NONINTERACTIVE is set, if any machine-output flag is -// set (--agent, --json, --quiet, --ids-only, --count), or if stdout is not a -// character device (the guard treats any char device — a terminal, /dev/null, -// etc. — as interactive-capable). +// set (--agent, --json, --quiet, --ids-only, --count), or if stdout or stdin is +// not a character device (the guard treats any char device — a terminal, +// /dev/null, etc. — as interactive-capable). func (r *Resolver) IsInteractive() bool { // Explicit escape hatch: BASECAMP_NONINTERACTIVE forces non-interactive mode // even under a PTY, without changing the output format. @@ -120,12 +120,11 @@ func (r *Resolver) IsInteractive() bool { } } - // Check if stdout is a character device (e.g. a terminal) - fi, err := os.Stdout.Stat() - if err != nil { - return false - } - return (fi.Mode() & os.ModeCharDevice) != 0 + // Both stdout and stdin must be character devices: pickers draw to + // stdout and read keystrokes from stdin, so a pipe on either end can + // never drive one — and when the command is consuming piped content + // (a "-" stdin input), a picker would eat that content as key events. + return stdinarg.InteractiveStdio() } // ResolvedValue represents a value that was resolved, along with metadata diff --git a/internal/tui/resolve/resolve_test.go b/internal/tui/resolve/resolve_test.go new file mode 100644 index 000000000..bdd1a0235 --- /dev/null +++ b/internal/tui/resolve/resolve_test.go @@ -0,0 +1,46 @@ +package resolve + +import ( + "os" + "testing" +) + +// TestIsInteractiveRequiresStdinCharDevice proves pickers are gated off when +// stdin is piped: a Bubble Tea picker reads keystrokes from stdin, so piped +// stdin can never drive one — and when a command is consuming piped content +// (a "-" stdin input), a picker would eat that content as key events. +func TestIsInteractiveRequiresStdinCharDevice(t *testing.T) { + t.Setenv("BASECAMP_NONINTERACTIVE", "") + + devnull, err := os.OpenFile(os.DevNull, os.O_RDWR, 0) + if err != nil { + t.Fatalf("open %s: %v", os.DevNull, err) + } + defer devnull.Close() + + pipeR, pipeW, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + defer pipeR.Close() + defer pipeW.Close() + + origOut, origIn := os.Stdout, os.Stdin + t.Cleanup(func() { os.Stdout, os.Stdin = origOut, origIn }) + + // /dev/null is a character device, so it stands in for a terminal on + // both ends without needing a PTY. + os.Stdout = devnull + + r := New(nil, nil, nil) + + os.Stdin = devnull + if !r.IsInteractive() { + t.Fatal("expected interactive with char-device stdout and stdin") + } + + os.Stdin = pipeR + if r.IsInteractive() { + t.Fatal("expected non-interactive with piped stdin: a picker would consume the pipe as key events") + } +} diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 35138fed9..10d665451 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -108,6 +108,27 @@ Full CLI coverage: 155 endpoints across todos, cards, messages, files, schedule, ```bash printf '%s\n' '海报 mockup 方向稿：' '' '<bc-attachment ...>' | basecamp comments create <recording_id> - --in <project> --json ``` + `-` means "read from stdin" on every content input: content-kind positionals + (`comments create/update`, `messages create [body]`, `cards create [body]`, + `todos create`, `docs documents create [content]`, `chat post/update`, `boost create`, + `checkins answer create/update`, `notes set`) and content flags (`--data` on + `api post/put`, `--body`, `--content`, `--description`, `--comment` on + `todos sweep`, `--file` on `notes set`). Each command's `--agent` help lists + its stdin inputs. Rules: + - A pipe is **never consumed implicitly** — without `-` it is ignored (or, where + content is required and missing, the error teaches `-`). + - Only one input can read stdin per invocation. + - A literal `-` anywhere else (a title, a name, a path) **errors when stdin is + piped**. Escape a positional after the `--` separator + (`basecamp projects create -- -`); a flag value has no in-line escape — run + the command without piped stdin. `basecamp help` and shell completion are + exempt: they write nothing to Basecamp, and completion legitimately + receives `-` as the word being completed. + - `-` with nothing piped (interactive TTY) errors immediately instead of + hanging; use a pipe, a heredoc (`basecamp comments create <id> - <<'EOF'`), + or `--edit` where offered. + - Trailing newlines are trimmed from stdin content, so `printf 'x\n' | ... -` + posts `x` (this keeps `boost create -` inside its 16-rune limit). 6. **Project scope is mandatory for most commands** — via `--in <project>` or `.basecamp/config.json`. Cross-project exceptions: `basecamp reports assigned` for assigned work, `basecamp assignments` for structured assignment views, `basecamp reports overdue` for overdue todos, `basecamp reports schedule` for upcoming schedule across all projects, `basecamp recordings <type>` for browsing by type, `basecamp notifications` for notifications, `basecamp gauges list` for account-wide gauges, and the seven list commands covered in item 7. 7. **Account-wide listing.** `basecamp todos list --all-projects --json` lists across every project; the same flag does the same on `cards list`, `messages list`, `comments list`, `files list`, `forwards list`, and `checkins answers`. It overrides a configured project, and with no project in scope those commands already list account-wide rather than prompting. Flags that name something inside a single project are rejected there rather than silently ignored. Account-wide listings return **the first 100 items by default** — account-wide "all" is the whole account, not one project's worth. Use `--limit N` to raise the cap (it walks pages until N are collected) or `--all` for everything. `--page N` fetches exactly one page, but only on the paginated listings. @@ -1086,8 +1107,9 @@ at 250 server-side. `notes` is a single private scratchpad — one per person, no id, nothing to list. Before your first write it renders empty rather than 404ing. `set` **replaces** -the whole note (it does not append) and takes content from an argument, -`--file`, or piped stdin; Markdown is converted to HTML. +the whole note (it does not append) and takes content from an argument or +`--file` — either accepts `-` to read stdin (`cat notes.md | basecamp notes set -`); +a pipe without `-` is not consumed. Markdown is converted to HTML. ### Calendars