fix(keyring): keep pre-v0.3.0 vaults readable after the entry rename

config.go

@@ -78,11 +78,11 @@ func SaveConfigJSON(config Config, path string) error {
 		return fmt.Errorf("failed to marshal config: %w", err)
 	}
 
-	if err := os.MkdirAll(filepath.Dir(path), 0750); err != nil {
+	if err := os.MkdirAll(filepath.Dir(path), vaultDirMode); err != nil {
 		return fmt.Errorf("failed to create config directory: %w", err)
 	}
 
-	if err := os.WriteFile(filepath.Clean(path), data, 0600); err != nil {
+	if err := os.WriteFile(filepath.Clean(path), data, vaultFileMode); err != nil {
 		return fmt.Errorf("failed to write config file: %w", err)
 	}
 

config_test.go

@@ -0,0 +1,44 @@
+package vault_test
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/flowexec/vault"
+)
+
+// A vault config carries the provider's command templates and environment
+// values, and its presence alone discloses which secret backends a user has
+// configured. The file was already 0600, but the directory SaveConfigJSON
+// created for it was group-readable.
+func TestSaveConfigJSONWritesOwnerOnly(t *testing.T) {
+	dir := t.TempDir()
+	nested := filepath.Join(dir, "configs")
+	path := filepath.Join(nested, "myvault.json")
+
+	cfg := vault.Config{
+		ID:          "myvault",
+		Type:        vault.ProviderTypeUnencrypted,
+		Unencrypted: &vault.UnencryptedConfig{StoragePath: dir},
+	}
+	if err := vault.SaveConfigJSON(cfg, path); err != nil {
+		t.Fatalf("SaveConfigJSON() error = %v", err)
+	}
+
+	fileInfo, err := os.Stat(path)
+	if err != nil {
+		t.Fatalf("Stat(file) error = %v", err)
+	}
+	if perm := fileInfo.Mode().Perm(); perm != 0600 {
+		t.Errorf("config file mode = %o, want 0600", perm)
+	}
+
+	dirInfo, err := os.Stat(nested)
+	if err != nil {
+		t.Fatalf("Stat(dir) error = %v", err)
+	}
+	if perm := dirInfo.Mode().Perm(); perm != 0700 {
+		t.Errorf("config directory mode = %o, want 0700", perm)
+	}
+}

external.go

@@ -115,7 +115,7 @@ func (v *ExternalVaultProvider) getSecretLocked(key string) (Secret, error) {
 			return nil, fmt.Errorf("failed to parse output: %w", err)
 		}
 	} else {
-		secretValue = strings.TrimSpace(output)
+		secretValue = trimCommandNewline(output)
 	}
 
 	return NewSecretValue([]byte(secretValue)), nil
@@ -308,6 +308,17 @@ func (v *ExternalVaultProvider) hasSecretViaExistsCmd(key string) (bool, error)
 	// A non-zero exit conventionally means "absent", but it is also how an expired
 	// session, a network failure, or a permissions problem surfaces. NotFoundPattern
 	// lets a config say which failures actually mean absence.
+	//
+	// It can only say so about failures that produce a message, though. An exists
+	// command may answer purely by exit status -- `test -f`, `jq -e` -- and then
+	// there is no text for the pattern to match. Treating that as "the pattern did
+	// not match, so this is a real error" turns every ordinary miss into a failure,
+	// so a silent non-zero exit is taken at its word: absent.
+	var cmdErr *commandError
+	if errors.As(err, &cmdErr) && cmdErr.diagnostics() == "" {
+		return false, nil
+	}
+
 	if v.cfg.NotFoundPattern != "" && !strings.Contains(err.Error(), v.cfg.NotFoundPattern) {
 		return false, err
 	}
@@ -383,6 +394,27 @@ func (v *ExternalVaultProvider) Metadata() (Metadata, error) {
 	return Metadata{RawData: metadataOutput}, nil
 }
 
+// commandError carries a failing command's diagnostic output alongside the
+// error, so callers can tell "the command answered by exit status alone" from
+// "the command complained about something" without parsing an error string.
+type commandError struct {
+	stderr string
+	err    error
+}
+
+func (e *commandError) Error() string {
+	if e.stderr == "" {
+		return fmt.Sprintf("command failed: %v", e.err)
+	}
+	return fmt.Sprintf("command failed: %v, stderr: %s", e.err, e.stderr)
+}
+
+func (e *commandError) Unwrap() error { return e.err }
+
+// diagnostics returns the command's output, trimmed. Empty means the command
+// said nothing and reported only through its exit status.
+func (e *commandError) diagnostics() string { return strings.TrimSpace(e.stderr) }
+
 func (v *ExternalVaultProvider) executeCommand(cmd, input string) (string, error) {
 	ctx := v.ctx
 	if ctx == nil {
@@ -396,7 +428,7 @@ func (v *ExternalVaultProvider) executeCommand(cmd, input string) (string, error
 
 	output, runErr := v.execute(ctx, cmd, input, v.cfg.WorkingDir, v.environmentToSlice())
 	if runErr != nil {
-		return "", fmt.Errorf("command failed: %w, stderr: %s", runErr, output)
+		return "", &commandError{stderr: output, err: runErr}
 	}
 
 	return output, nil
@@ -515,7 +547,25 @@ func execute(ctx context.Context, cmd, input, dir string, envList []string) (str
 	// Only stdout is the result. Merging stderr in on success concatenates any
 	// warning the backend emits (e.g. "gpg: WARNING: unsafe permissions") onto
 	// the secret value itself. stderr is still returned on the error path above.
-	return strings.TrimSpace(stdOutBuffer.String()), nil
+	//
+	// Returned verbatim: trimming here would silently corrupt any secret with
+	// deliberate leading or trailing whitespace. Callers that want a tidy string
+	// (list, metadata) trim for themselves; GetSecret strips only the single
+	// trailing newline a command adds.
+	return stdOutBuffer.String(), nil
+}
+
+// trimCommandNewline removes the one trailing line ending a command conventionally
+// adds to its output, and nothing else.
+//
+// TrimSpace would take real data with it: a passphrase may legitimately begin or
+// end with a space, and a PEM block ends in a newline that some parsers require.
+// A secret whose true value ends in a newline is still indistinguishable from one
+// that does not -- that is inherent to reading a value off a command's stdout,
+// and no amount of trimming policy can recover it.
+func trimCommandNewline(s string) string {
+	s = strings.TrimSuffix(s, "\n")
+	return strings.TrimSuffix(s, "\r")
 }
 
 // expandEnv returns a new map with environment references expanded. It must not

external_test.go

@@ -419,9 +419,14 @@ func TestHasSecret_NotFoundPatternDistinguishesRealFailures(t *testing.T) {
 	cfg.Exists.CommandTemplate = "check {{key}}"
 	cfg.NotFoundPattern = "ParameterNotFound"
 
+	// execute() reports a generic "exited with non-zero status" error and returns
+	// the backend's own message as the command output, so these mocks put the
+	// diagnostic where the real executor puts it. A mock that instead encodes it
+	// in the error would be testing a shape that cannot occur.
 	t.Run("absent", func(t *testing.T) {
 		provider := newTestProvider(t, cfg)
-		provider.SetExecutionFunc(capturingExec(&execCapture{}, "", fmt.Errorf("ParameterNotFound: nope")))
+		provider.SetExecutionFunc(capturingExec(&execCapture{},
+			"ParameterNotFound: nope", fmt.Errorf("exit status 1")))
 
 		exists, err := provider.HasSecret("k")
 		if err != nil {
@@ -434,7 +439,8 @@ func TestHasSecret_NotFoundPatternDistinguishesRealFailures(t *testing.T) {
 
 	t.Run("real failure surfaces", func(t *testing.T) {
 		provider := newTestProvider(t, cfg)
-		provider.SetExecutionFunc(capturingExec(&execCapture{}, "", fmt.Errorf("ExpiredToken: session expired")))
+		provider.SetExecutionFunc(capturingExec(&execCapture{},
+			"ExpiredToken: session expired", fmt.Errorf("exit status 254")))
 
 		if _, err := provider.HasSecret("k"); err == nil {
 			t.Error("HasSecret() error = nil, want the expired-session error to surface")
@@ -635,3 +641,118 @@ func TestConcurrentGetSecretDoesNotRaceOnEnvironment(t *testing.T) {
 		t.Errorf("config Environment was mutated: LITERAL = %q, want %q", got, "$(tty)")
 	}
 }
+
+// An exists command may answer purely by exit status -- `test -f`, `jq -e` --
+// leaving no message for NotFoundPattern to match. Treating that as "the
+// pattern did not match, so this is a real error" turned every ordinary miss
+// into a failure. Found by driving the pass preset against a real store.
+func TestHasSecret_SilentNonZeroExitMeansAbsent(t *testing.T) {
+	cfg := validExternalConfig()
+	cfg.Exists.CommandTemplate = "test -f /nonexistent/{{key}}"
+	cfg.NotFoundPattern = "is not in the password store"
+
+	provider := newTestProvider(t, cfg)
+
+	exists, err := provider.HasSecret("missing")
+	if err != nil {
+		t.Fatalf("HasSecret() on a silent non-zero exit = %v, want a plain false", err)
+	}
+	if exists {
+		t.Error("HasSecret() = true, want false")
+	}
+}
+
+// A command that *does* complain still gets its message checked, so an expired
+// session is not silently reported as "the secret does not exist".
+func TestHasSecret_DiagnosticNotMatchingPatternSurfaces(t *testing.T) {
+	cfg := validExternalConfig()
+	cfg.Exists.CommandTemplate = "echo 'ExpiredToken: session expired' 1>&2; exit 1"
+	cfg.NotFoundPattern = "ParameterNotFound"
+
+	provider := newTestProvider(t, cfg)
+
+	if _, err := provider.HasSecret("k"); err == nil {
+		t.Error("HasSecret() error = nil, want the expired-session failure to surface")
+	}
+}
+
+func TestHasSecret_DiagnosticMatchingPatternMeansAbsent(t *testing.T) {
+	cfg := validExternalConfig()
+	cfg.Exists.CommandTemplate = "echo 'ParameterNotFound: nope' 1>&2; exit 1"
+	cfg.NotFoundPattern = "ParameterNotFound"
+
+	provider := newTestProvider(t, cfg)
+
+	exists, err := provider.HasSecret("k")
+	if err != nil {
+		t.Fatalf("HasSecret() = %v, want a plain false", err)
+	}
+	if exists {
+		t.Error("HasSecret() = true, want false")
+	}
+}
+
+// GetSecret used to TrimSpace the command's output, so a secret with deliberate
+// leading or trailing whitespace was stored correctly by the backend and came
+// back mangled. Only the single trailing newline a command adds is removed.
+func TestGetSecret_PreservesDeliberateWhitespace(t *testing.T) {
+	for _, tc := range []struct {
+		name, stdout, want string
+	}{
+		{"leading space", " value\n", " value"},
+		{"trailing space", "value \n", "value "},
+		{"only spaces", "   \n", "   "},
+		{"tabs", "\tvalue\t\n", "\tvalue\t"},
+		{"internal newlines", "line1\nline2\n", "line1\nline2"},
+		{"no trailing newline", "value", "value"},
+		{"crlf", "value\r\n", "value"},
+		{"empty", "", ""},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			provider := newTestProvider(t, validExternalConfig())
+			provider.SetExecutionFunc(func(
+				_ context.Context, _, _, _ string, _ []string,
+			) (string, error) {
+				return tc.stdout, nil
+			})
+
+			secret, err := provider.GetSecret("k")
+			if err != nil {
+				t.Fatalf("GetSecret() error = %v", err)
+			}
+			if got := secret.PlainTextString(); got != tc.want {
+				t.Errorf("GetSecret() = %q, want %q", got, tc.want)
+			}
+		})
+	}
+}
+
+// List and metadata still tidy their output; only the secret value is verbatim.
+func TestListAndMetadataStillTrim(t *testing.T) {
+	cfg := validExternalConfig()
+	cfg.List.CommandTemplate = "ls"
+	cfg.Metadata.CommandTemplate = "status"
+
+	provider := newTestProvider(t, cfg)
+	provider.SetExecutionFunc(func(
+		_ context.Context, _, _, _ string, _ []string,
+	) (string, error) {
+		return "  alpha  \n  beta  \n", nil
+	})
+
+	keys, err := provider.ListSecrets()
+	if err != nil {
+		t.Fatalf("ListSecrets() error = %v", err)
+	}
+	if len(keys) != 2 || keys[0] != "alpha" || keys[1] != "beta" {
+		t.Errorf("ListSecrets() = %q, want [alpha beta]", keys)
+	}
+
+	md, err := provider.Metadata()
+	if err != nil {
+		t.Fatalf("Metadata() error = %v", err)
+	}
+	if strings.HasPrefix(md.RawData, " ") || strings.HasSuffix(md.RawData, "\n") {
+		t.Errorf("Metadata().RawData = %q, want it trimmed", md.RawData)
+	}
+}