From d967f894eb9e738a5fdd3472a8333d0cbcc5cbfe Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 14 Oct 2026 19:21:23 +0000 Subject: [PATCH] secret get fails when it cannot write the value (closes #155) GetSecret and GetSecretWithVersion discarded the result of writing the value to stdout, so a full disk or a failing stdout left the caller with an empty or cut-short file and exit status 0. The write error is now returned, so the command prints it and exits 1. Deviation: a stdout the shell closed before `secret` started still exits 0, because the Go runtime opens /dev/null in its place and the write there succeeds. Model: opus-5-5 --- README.md | 8 ++++++- TODO.md | 7 ++++++ internal/cli/secrets.go | 10 +++++++-- internal/cli/version_test.go | 41 ++++++++++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 1e2b834..9e9fd61 100644 --- a/README.md +++ b/README.md @@ -142,7 +142,13 @@ contain `//`, or have `..` as a path segment. #### `secret get [--version ]` -Retrieves and outputs a secret value to stdout. +Retrieves and outputs a secret value to stdout. A failed write of the value, +such as to a file on a full disk, prints the error and exits 1, with two +exceptions. If the program reading from a pipe has already exited, as in +`secret get s | true`, the broken-pipe signal (SIGPIPE) ends `secret` with no +message and the shell reports exit status 141. If the shell closed stdout before +`secret` started (`secret get s >&-`), the value is written to `/dev/null` and +`secret get` exits 0. - `--version, -v`: Get a specific version (default: current) diff --git a/TODO.md b/TODO.md index d659129..e315f66 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,13 @@ https://git.eeqj.de/sneak/secret/milestone/12 # Completed Steps +- 2026-10-14: `secret get` fails when it cannot write the value to stdout + (https://git.eeqj.de/sneak/secret/issues/155). The write's error was ignored, + so a script that sent the value to a file on a full disk saw `secret get` + succeed and was left with an empty or cut-short file. That error is now the + command's error, and `secret get` exits 1. A stdout the shell closed (`>&-`) + still gives exit 0, because the Go runtime opens `/dev/null` in its place + before `secret` starts and the write there succeeds. - 2026-10-14: The passphrase and mnemonic prompts ask on the controlling terminal (`/dev/tty`) instead of stdin, so `echo v | secret add s1 --force`, `cat file | secret encrypt key` and `cat file.age | secret decrypt key` ask diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index 1728e2b..6598ff9 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -443,7 +443,10 @@ func (cli *Instance) GetSecret(cmd *cobra.Command, secretName string) error { defer value.Destroy() // Write the value straight from locked memory, with no trailing newline - _, _ = cmd.OutOrStdout().Write(value.Bytes()) + _, err = cmd.OutOrStdout().Write(value.Bytes()) + if err != nil { + return fmt.Errorf("failed to write secret value: %w", err) + } return nil } @@ -478,7 +481,10 @@ func (cli *Instance) GetSecretWithVersion( secret.Debug("Got secret value", "valueLength", value.Size()) // Write the value straight from locked memory, with no trailing newline - _, _ = cmd.OutOrStdout().Write(value.Bytes()) + _, err = cmd.OutOrStdout().Write(value.Bytes()) + if err != nil { + return fmt.Errorf("failed to write secret value: %w", err) + } secret.Debug("Printed value to stdout") diff --git a/internal/cli/version_test.go b/internal/cli/version_test.go index 018b5d3..8904da6 100644 --- a/internal/cli/version_test.go +++ b/internal/cli/version_test.go @@ -8,6 +8,7 @@ // - TestPromoteNonExistentVersion: Tests error handling for invalid promotion // - TestGetSecretWithVersion: Tests `secret get --version` flag functionality // - TestGetSecretWritesBinaryValue: Tests `secret get` output of binary values +// - TestGetSecretReturnsWriteError: Tests `secret get` when stdout fails // - TestVersionCommandStructure: Tests command structure and help text // - TestListVersionsEmptyOutput: Tests edge case with no versions // @@ -20,6 +21,7 @@ package cli import ( "bytes" + "os" "path/filepath" "strings" "testing" @@ -360,6 +362,45 @@ func TestGetSecretWritesBinaryValue(t *testing.T) { assert.Equal(t, value, buf.Bytes()) } +// TestGetSecretReturnsWriteError checks that `secret get`, with and without +// a version, returns the error of a failed write of the value. +func TestGetSecretReturnsWriteError(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + cli := NewCLIInstanceWithStateDir(fs, testStateDir) + cli.Mnemonic = testMnemonicBuffer(t) + + setupTestVault(t, fs) + + vlt, err := vault.GetCurrentVault(fs, testStateDir) + require.NoError(t, err) + + addTestSecret(t, vlt, []byte("value"), false) + + vaultDir, err := vlt.GetDirectory() + require.NoError(t, err) + + versions, err := secret.ListVersions(fs, + filepath.Join(vaultDir, "secrets.d", "test%secret")) + require.NoError(t, err) + require.Len(t, versions, 1) + + // Stdout is a file closed before the value is written to it + out, err := os.Create(filepath.Join(t.TempDir(), "stdout")) + require.NoError(t, err) + require.NoError(t, out.Close()) + + cmd := newRootCmd() + cmd.SetOut(out) + + err = cli.GetSecret(cmd, "test/secret") + require.ErrorIs(t, err, os.ErrClosed) + + err = cli.GetSecretWithVersion(cmd, "test/secret", versions[0]) + require.ErrorIs(t, err, os.ErrClosed) +} + //nolint:paralleltest // reads process environment to determine the state dir func TestVersionCommandStructure(t *testing.T) { // Test that version commands are properly structured