diff --git a/internal/commands/notes.go b/internal/commands/notes.go index 0405e276..7a51a7dd 100644 --- a/internal/commands/notes.go +++ b/internal/commands/notes.go @@ -2,6 +2,7 @@ package commands import ( "fmt" + "io" "os" "strings" @@ -196,11 +197,11 @@ func notesContent(cmd *cobra.Command, args []string, file string) (string, error } return notesRequireContent(content) case file != "": - data, err := os.ReadFile(file) + content, err := notesFileContent(file) if err != nil { - return "", output.ErrUsage(fmt.Sprintf("failed to read %s: %v", file, err)) + return "", err } - return notesRequireContent(string(data)) + return notesRequireContent(content) case positional != "": content, err := resolveContentValue(cmd, positional, 0, "[content]") if err != nil { @@ -215,6 +216,25 @@ func notesContent(cmd *cobra.Command, args []string, file string) (string, error ) } +// notesFileContent reads --file under the same cap as --file -, so which side +// of the "-" the bytes arrive on never changes whether they are accepted. +func notesFileContent(file string) (string, error) { + f, err := os.Open(file) + if err != nil { + return "", output.ErrUsage(fmt.Sprintf("failed to read %s: %v", file, err)) + } + defer f.Close() + + data, err := io.ReadAll(io.LimitReader(f, maxStdinContent+1)) + if err != nil { + return "", output.ErrUsage(fmt.Sprintf("failed to read %s: %v", file, err)) + } + if len(data) > maxStdinContent { + return "", output.ErrUsage(fmt.Sprintf("--file %s exceeds %d bytes", file, maxStdinContent)) + } + return string(data), nil +} + // notesRequireContent refuses to blank the note by accident. // // set replaces everything, so an empty file or an empty pipe would silently diff --git a/internal/commands/notes_test.go b/internal/commands/notes_test.go index 4b960158..48058441 100644 --- a/internal/commands/notes_test.go +++ b/internal/commands/notes_test.go @@ -124,6 +124,26 @@ func TestNotesSetReadsFromAFile(t *testing.T) { assert.NotContains(t, body.Note.Content, "# Heading", "raw Markdown must not reach the wire") } +// --file and --file - are the same read, so they agree on the boundary: a file +// of exactly maxStdinContent is content, one byte past it is a usage error. +func TestNotesSetBoundsFileAtTheStdinCap(t *testing.T) { + dir := t.TempDir() + atCap := filepath.Join(dir, "at-cap.md") + require.NoError(t, os.WriteFile(atCap, []byte(strings.Repeat("a", maxStdinContent)), 0o600)) + overCap := filepath.Join(dir, "over-cap.md") + require.NoError(t, os.WriteFile(overCap, []byte(strings.Repeat("a", maxStdinContent+1)), 0o600)) + + app, transport, _ := setupPersonalFeedApp(t, notesUpdateRoute()) + require.NoError(t, executeRecordingCommand(NewNotesCmd(), app, "set", "--file", atCap)) + assert.NotEmpty(t, transport.recorded()) + + app, transport, _ = setupPersonalFeedApp(t, notesUpdateRoute()) + err := executeRecordingCommand(NewNotesCmd(), app, "set", "--file", overCap) + outErr := requireBookmarksUsageError(t, err) + assert.Contains(t, outErr.Message, "exceeds 1048576 bytes") + assert.Empty(t, transport.recorded(), "a rejected write must not reach the server") +} + func TestNotesSetReadsDashFromStdin(t *testing.T) { app, transport, _ := setupPersonalFeedApp(t, notesUpdateRoute()) diff --git a/internal/commands/stdin.go b/internal/commands/stdin.go index 8b1e0c14..d895609a 100644 --- a/internal/commands/stdin.go +++ b/internal/commands/stdin.go @@ -37,6 +37,13 @@ func allowDash(cmd *cobra.Command, tokens ...string) { cmd.Annotations[stdinarg.AnnotationAllowDash] = merged } +// maxStdinContent caps how much a "-" placeholder — and the --file twin that +// shares its refusals — will read. It matches maxAgentHookInput by coincidence +// of scale, not by kinship: that one bounds a JSON envelope an agent harness +// writes, this one bounds prose a person pipes, and the two should stay free +// to move apart. +const maxStdinContent = 1 << 20 + // readStdinContent reads content for a "-" placeholder from piped stdin. // // Nothing piped (a TTY) is a usage error rather than a silent read: waiting on @@ -45,6 +52,15 @@ func allowDash(cmd *cobra.Command, tokens ...string) { // never an intentional write, and for update-style commands it would be an // implicit clear. // +// A stream over maxStdinContent is refused as well, and refused rather than +// truncated: `yes | basecamp api post /valid --data -` is a perfectly valid +// invocation that would otherwise read until the process dies, and silently +// posting the first megabyte would write partial content to Basecamp and +// report success — unrecoverable once saved. The cap counts bytes read, so it +// lands before the trim below rather than on the trimmed result: telling an +// overflow that is only a trailing newline from any other would mean reading +// past the cap, which is the thing being prevented. +// // 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. @@ -55,10 +71,13 @@ func readStdinContent(cmd *cobra.Command, what string) (string, error) { stdinEscapeHint(cmd, what), ) } - data, err := io.ReadAll(cmd.InOrStdin()) + data, err := io.ReadAll(io.LimitReader(cmd.InOrStdin(), maxStdinContent+1)) if err != nil { return "", output.ErrUsage(fmt.Sprintf("failed to read %s from stdin: %v", what, err)) } + if len(data) > maxStdinContent { + return "", output.ErrUsage(fmt.Sprintf("stdin for %s exceeds %d bytes", what, maxStdinContent)) + } content := strings.TrimRight(string(data), "\r\n") if strings.TrimSpace(content) == "" { return "", output.ErrUsage(fmt.Sprintf("stdin for %s is empty", what)) diff --git a/internal/commands/stdin_test.go b/internal/commands/stdin_test.go index 229d81e5..19a26e8a 100644 --- a/internal/commands/stdin_test.go +++ b/internal/commands/stdin_test.go @@ -95,6 +95,39 @@ func TestReadStdinContentBlankPipeIsUsageError(t *testing.T) { assert.Contains(t, outErr.Message, "empty") } +// The cap is a read bound, so it is checked against the bytes read rather than +// the trimmed content: exactly maxStdinContent is content, one byte past it is +// a refusal — including when that byte is the trailing newline every pipe ends +// with, which is the only way to stop reading at the cap at all. +func TestReadStdinContentAtTheCapIsAccepted(t *testing.T) { + body := strings.Repeat("a", maxStdinContent) + cmd := &cobra.Command{Use: "x"} + cmd.SetIn(strings.NewReader(body)) + + content, err := readStdinContent(cmd, "") + require.NoError(t, err) + assert.Equal(t, body, content) +} + +func TestReadStdinContentOverTheCapIsUsageError(t *testing.T) { + for _, tc := range []struct { + name string + body string + }{ + {"one byte over", strings.Repeat("a", maxStdinContent+1)}, + {"the cap plus a trailing newline", strings.Repeat("a", maxStdinContent) + "\n"}, + } { + t.Run(tc.name, func(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + cmd.SetIn(strings.NewReader(tc.body)) + + _, err := readStdinContent(cmd, "") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "exceeds 1048576 bytes") + }) + } +} + func TestResolveContentArgJoinsLiteralArgs(t *testing.T) { cmd := &cobra.Command{Use: "x"} content, err := resolveContentArg(cmd, []string{"hello", "world"}, 1)