From 4bd293c5494d04ddc5e3269c36d1063c17c2405b Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Sun, 26 Jul 2026 16:44:39 -0400 Subject: [PATCH 1/6] fix(keyring): keep pre-v0.3.0 vaults readable after the entry rename v0.3.0 length-prefixed keyring entry names to fix a real collision, but shipped no fallback -- so every secret in an existing keyring vault became unreachable. The data was never lost: it is still in the OS keyring, under the old name, and the vault was simply looking somewhere else and reporting "secret not found". A user upgrading would reasonably conclude their secrets had been deleted. Reads now fall back to the pre-v0.3.0 name, writes migrate the entry to the current name and delete the old one, and deletes clear both so a legacy copy cannot survive and reappear on the next read. Nothing has to be re-entered. The three new tests were confirmed to fail without the fallback before being committed: GetSecret returns "secret not found" on a legacy entry, the write leaves the old entry in place, and DeleteSecret cannot find the secret at all. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HtuDuGkSqTfAXSepf8b51p --- keyring.go | 74 +++++++++++++++++++++++---- keyring_test.go | 131 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 196 insertions(+), 9 deletions(-) diff --git a/keyring.go b/keyring.go index a33fecd..21c059a 100644 --- a/keyring.go +++ b/keyring.go @@ -58,6 +58,20 @@ func (v *KeyringVault) namespaced(kind, key string) string { return fmt.Sprintf("%d:%s:%s:%s", len(v.id), v.id, kind, key) } +// legacyNamespaced reproduces the pre-v0.3.0 entry name. +// +// Renaming the entries fixed a real collision but made every secret in an +// existing keyring vault unreachable: the data is still in the OS keyring, just +// under the old name. Reads fall back to this, and the next write migrates the +// entry, so an existing vault keeps working without the user re-entering +// anything. +func (v *KeyringVault) legacyNamespaced(kind, key string) string { + if key == "" { + return fmt.Sprintf("%s-%s", v.id, kind) + } + return fmt.Sprintf("%s-%s-%s", v.id, kind, key) +} + func (v *KeyringVault) metadataKey() string { return v.namespaced("metadata", "") } @@ -70,6 +84,48 @@ func (v *KeyringVault) secretsListKey() string { return v.namespaced("secrets-list", "") } +// get reads an entry, falling back to the pre-v0.3.0 name. The returned bool +// reports whether the value came from a legacy entry and so needs migrating. +func (v *KeyringVault) get(kind, key string) (string, bool, error) { + data, err := keyring.Get(v.service, v.namespaced(kind, key)) + if err == nil { + return data, false, nil + } + if !errors.Is(err, keyring.ErrNotFound) { + return "", false, err + } + + data, legacyErr := keyring.Get(v.service, v.legacyNamespaced(kind, key)) + if legacyErr != nil { + // Report the miss against the current name; the legacy lookup is an + // implementation detail and its error would only confuse. + return "", false, err + } + return data, true, nil +} + +// set writes an entry under the current name and removes any legacy entry it +// supersedes, so the rename completes on first write rather than leaving two +// copies of a secret in the keyring. +func (v *KeyringVault) set(kind, key, value string) error { + if err := keyring.Set(v.service, v.namespaced(kind, key), value); err != nil { + return err + } + _ = keyring.Delete(v.service, v.legacyNamespaced(kind, key)) + return nil +} + +// remove deletes both the current and legacy entries so a delete cannot leave +// the old copy behind to reappear on the next read. +func (v *KeyringVault) remove(kind, key string) error { + err := keyring.Delete(v.service, v.namespaced(kind, key)) + legacyErr := keyring.Delete(v.service, v.legacyNamespaced(kind, key)) + if err != nil && errors.Is(err, keyring.ErrNotFound) && legacyErr == nil { + return nil + } + return err +} + func (v *KeyringVault) initMetadata() error { now := time.Now() v.metadata = Metadata{ @@ -81,7 +137,7 @@ func (v *KeyringVault) initMetadata() error { } func (v *KeyringVault) loadMetadata() error { - data, err := keyring.Get(v.service, v.metadataKey()) + data, _, err := v.get("metadata", "") if err != nil { return err } @@ -103,11 +159,11 @@ func (v *KeyringVault) saveMetadata() error { return fmt.Errorf("failed to marshal metadata: %w", err) } - return keyring.Set(v.service, v.metadataKey(), string(data)) + return v.set("metadata", "", string(data)) } func (v *KeyringVault) loadSecretsList() ([]string, error) { - data, err := keyring.Get(v.service, v.secretsListKey()) + data, _, err := v.get("secrets-list", "") if err != nil { if errors.Is(err, keyring.ErrNotFound) { return []string{}, nil @@ -129,7 +185,7 @@ func (v *KeyringVault) saveSecretsList(secrets []string) error { return fmt.Errorf("failed to marshal secrets list: %w", err) } - return keyring.Set(v.service, v.secretsListKey(), string(data)) + return v.set("secrets-list", "", string(data)) } func (v *KeyringVault) addSecretToList(key string) error { @@ -187,7 +243,7 @@ func (v *KeyringVault) GetSecret(key string) (Secret, error) { return nil, err } - data, err := keyring.Get(v.service, v.secretKey(key)) + data, _, err := v.get("secret", key) if err != nil { if errors.Is(err, keyring.ErrNotFound) { return nil, ErrSecretNotFound @@ -206,7 +262,7 @@ func (v *KeyringVault) SetSecret(key string, secret Secret) error { return err } - if err := keyring.Set(v.service, v.secretKey(key), secret.PlainTextString()); err != nil { + if err := v.set("secret", key, secret.PlainTextString()); err != nil { return fmt.Errorf("failed to set secret in keyring: %w", err) } @@ -226,7 +282,7 @@ func (v *KeyringVault) DeleteSecret(key string) error { } // Check if secret exists first - _, err := keyring.Get(v.service, v.secretKey(key)) + _, _, err := v.get("secret", key) if err != nil { if errors.Is(err, keyring.ErrNotFound) { return ErrSecretNotFound @@ -234,7 +290,7 @@ func (v *KeyringVault) DeleteSecret(key string) error { return fmt.Errorf("failed to check secret existence: %w", err) } - if err := keyring.Delete(v.service, v.secretKey(key)); err != nil { + if err := v.remove("secret", key); err != nil { return fmt.Errorf("failed to delete secret from keyring: %w", err) } @@ -269,7 +325,7 @@ func (v *KeyringVault) HasSecret(key string) (bool, error) { return false, err } - _, err := keyring.Get(v.service, v.secretKey(key)) + _, _, err := v.get("secret", key) if err != nil { if errors.Is(err, keyring.ErrNotFound) { return false, nil diff --git a/keyring_test.go b/keyring_test.go index 3cafc8c..c4685ac 100644 --- a/keyring_test.go +++ b/keyring_test.go @@ -375,3 +375,134 @@ func TestKeyringVault_SortedOutput(t *testing.T) { } } } + +// Renaming keyring entries in v0.3.0 fixed a real collision, but the data for +// every existing keyring vault is still stored under the old names. Without a +// fallback, upgrading silently makes every secret look deleted -- the keyring +// still holds it, the vault just looks in the wrong place. +func TestKeyringVault_ReadsPreV030EntryNames(t *testing.T) { + keyring.MockInit() + + const ( + vaultID = "legacy-vault" + key = "api-key" + value = "legacy-secret-value" + ) + + // Seed the keyring exactly as a pre-v0.3.0 vault would have left it. + if err := keyring.Set(testKeyringService, vaultID+"-secret-"+key, value); err != nil { + t.Fatalf("failed to seed legacy secret: %v", err) + } + if err := keyring.Set(testKeyringService, vaultID+"-secrets-list", `["`+key+`"]`); err != nil { + t.Fatalf("failed to seed legacy secrets list: %v", err) + } + if err := keyring.Set(testKeyringService, vaultID+"-metadata", `{"created":"2024-01-01T00:00:00Z"}`); err != nil { + t.Fatalf("failed to seed legacy metadata: %v", err) + } + + vlt, _, err := vault.New(vaultID, + vault.WithProvider(vault.ProviderTypeKeyring), + vault.WithKeyringService(testKeyringService), + ) + if err != nil { + t.Fatalf("Failed to open a pre-v0.3.0 keyring vault: %v", err) + } + defer vlt.Close() + + secret, err := vlt.GetSecret(key) + if err != nil { + t.Fatalf("GetSecret() on a legacy entry = %v, want the stored value", err) + } + if got := secret.PlainTextString(); got != value { + t.Errorf("GetSecret() = %q, want %q", got, value) + } + + exists, err := vlt.HasSecret(key) + if err != nil || !exists { + t.Errorf("HasSecret() = (%v, %v), want (true, nil)", exists, err) + } + + keys, err := vlt.ListSecrets() + if err != nil { + t.Fatalf("ListSecrets() error = %v", err) + } + if len(keys) != 1 || keys[0] != key { + t.Errorf("ListSecrets() = %v, want [%s]", keys, key) + } +} + +// Writing migrates the entry to the current name and drops the old one, so the +// keyring does not end up holding two copies of the same secret. +func TestKeyringVault_WriteMigratesLegacyEntry(t *testing.T) { + keyring.MockInit() + + const ( + vaultID = "legacy-vault" + key = "api-key" + ) + + if err := keyring.Set(testKeyringService, vaultID+"-secret-"+key, "old-value"); err != nil { + t.Fatalf("failed to seed legacy secret: %v", err) + } + + vlt, _, err := vault.New(vaultID, + vault.WithProvider(vault.ProviderTypeKeyring), + vault.WithKeyringService(testKeyringService), + ) + if err != nil { + t.Fatalf("Failed to create keyring vault: %v", err) + } + defer vlt.Close() + + if err := vlt.SetSecret(key, vault.NewSecretValue([]byte("new-value"))); err != nil { + t.Fatalf("SetSecret() error = %v", err) + } + + // The legacy entry must be gone, not left behind holding a stale secret. + if _, err := keyring.Get(testKeyringService, vaultID+"-secret-"+key); !errors.Is(err, keyring.ErrNotFound) { + t.Errorf("legacy entry still present after write: err = %v", err) + } + + secret, err := vlt.GetSecret(key) + if err != nil { + t.Fatalf("GetSecret() error = %v", err) + } + if got := secret.PlainTextString(); got != "new-value" { + t.Errorf("GetSecret() = %q, want %q", got, "new-value") + } +} + +// Deleting must clear both names, or the legacy copy reappears on the next read. +func TestKeyringVault_DeleteRemovesLegacyEntry(t *testing.T) { + keyring.MockInit() + + const ( + vaultID = "legacy-vault" + key = "api-key" + ) + + if err := keyring.Set(testKeyringService, vaultID+"-secret-"+key, "old-value"); err != nil { + t.Fatalf("failed to seed legacy secret: %v", err) + } + + vlt, _, err := vault.New(vaultID, + vault.WithProvider(vault.ProviderTypeKeyring), + vault.WithKeyringService(testKeyringService), + ) + if err != nil { + t.Fatalf("Failed to create keyring vault: %v", err) + } + defer vlt.Close() + + if err := vlt.DeleteSecret(key); err != nil { + t.Fatalf("DeleteSecret() error = %v", err) + } + + exists, err := vlt.HasSecret(key) + if err != nil { + t.Fatalf("HasSecret() error = %v", err) + } + if exists { + t.Error("secret still readable after delete; the legacy entry survived") + } +} From 8a38f57114064cb71ea5ffcda283b34e3a5e47af Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Sun, 26 Jul 2026 16:54:51 -0400 Subject: [PATCH 2/6] fix(config): create the vault config directory owner-only SaveConfigJSON wrote the config file 0600 but created its parent directory 0750, so the directory holding every vault's configuration was group-readable. A vault config carries the provider's command templates and environment values, and its presence alone discloses which secret backends a user has configured. Now uses the same vaultDirMode/vaultFileMode constants as the vault storage directory, which was already tightened to 0700, so the two cannot drift. Found while building Mochi's vault presets: a test asserting the mode of the directory Mochi hands to `vault create --config` failed against the library. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HtuDuGkSqTfAXSepf8b51p --- config.go | 8 ++++++-- config_test.go | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 config_test.go diff --git a/config.go b/config.go index 469e45d..7500376 100644 --- a/config.go +++ b/config.go @@ -78,11 +78,15 @@ 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 { + // Owner-only, matching the vault storage directory. A vault config carries + // the provider's command templates and environment values, and its presence + // alone discloses which secret backends a user has configured; there is no + // reason for it to be group-readable when the file itself is 0600. + 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) } diff --git a/config_test.go b/config_test.go new file mode 100644 index 0000000..3380766 --- /dev/null +++ b/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) + } +} From 577891bd1d5ed4b1d3a0f8bee2640909983178ce Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Sun, 26 Jul 2026 17:04:09 -0400 Subject: [PATCH 3/6] fix(external): a silent non-zero exit from an exists command means absent NotFoundPattern, added in v0.3.0, is matched against a failing command's message. But an exists command may answer purely by exit status -- `test -f`, `jq -e`, and most file or key probes -- and then there is no text to match. The pattern "did not match", so every ordinary miss was reported as a backend failure instead of a plain false. Concretely: with the pass preset, HasSecret("nosuchkey") returned `command failed: command exited with non-zero status exit status 1, stderr: ` rather than (false, nil). Found by driving a rendered preset against a real pass store, not by reading the code -- the mock-based tests could not see it, because they encoded the diagnostic in the error rather than in the command output where execute() actually puts it. A failing command's output is now carried on a typed commandError, so the three cases are distinguished without parsing an error string: - exit non-zero, no output -> absent (the command's only answer) - exit non-zero, output matches -> absent - exit non-zero, output does not -> a real error, surfaced That keeps the feature doing its job -- an expired AWS session still surfaces instead of masquerading as "the secret does not exist" -- while letting an exit-code-only probe work as written. Also corrects TestHasSecret_NotFoundPatternDistinguishesRealFailures, which put the backend's message in the error and left the command output empty. execute() never does that: it returns a generic "exited with non-zero status" error and the backend's message as output. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HtuDuGkSqTfAXSepf8b51p --- external.go | 34 ++++++++++++++++++++++++++- external_test.go | 60 ++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 91 insertions(+), 3 deletions(-) diff --git a/external.go b/external.go index d38f0a7..7042d8e 100644 --- a/external.go +++ b/external.go @@ -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 diff --git a/external_test.go b/external_test.go index 3319273..f3520d0 100644 --- a/external_test.go +++ b/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,53 @@ 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") + } +} From 996d2a67d298bcf7b6ddfccab644c5b036b0b6d6 Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Sun, 26 Jul 2026 17:17:13 -0400 Subject: [PATCH 4/6] fix(external): stop trimming deliberate whitespace off secret values GetSecret ran the command's output through TrimSpace, so a secret with a meaningful leading space, trailing space or tab was stored correctly by the backend and came back mangled. Verified against a real backend before and after: stored " value" -> returned "value" stored "value " -> returned "value" stored " " -> returned "" stored "\tvalue\t" -> returned "value" A passphrase may legitimately begin or end with a space, and a PEM block ends in a newline some parsers require, so this is data loss rather than tidying. execute() now returns stdout verbatim and GetSecret strips only the single trailing line ending a command conventionally adds. List and metadata still trim, since that output is descriptive rather than the secret itself. One limit remains and is documented in the code: a secret whose true value ends in a newline is indistinguishable from one that does not. That is inherent to reading a value off a command's stdout, and no trimming policy can recover it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HtuDuGkSqTfAXSepf8b51p --- external.go | 22 ++++++++++++++-- external_test.go | 65 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 2 deletions(-) diff --git a/external.go b/external.go index 7042d8e..00faa62 100644 --- a/external.go +++ b/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 @@ -547,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 diff --git a/external_test.go b/external_test.go index f3520d0..5c7cac4 100644 --- a/external_test.go +++ b/external_test.go @@ -691,3 +691,68 @@ func TestHasSecret_DiagnosticMatchingPatternMeansAbsent(t *testing.T) { 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) + } +} From a97ec32a2591002a18df06ad413954e8a6a3b57e Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Sun, 26 Jul 2026 20:02:53 -0400 Subject: [PATCH 5/6] fix(keyring): drop the dead helpers and unused flag the rename left behind Introducing get/set/remove moved every call site off metadataKey, secretKey and secretsListKey without removing them, and golangci-lint's unused and unparam checks caught both that and get's second return value. The bool reported whether a value came from a legacy entry so a caller could migrate it. Nothing ever read it, and nothing should: set deletes the legacy entry unconditionally, so the migration happens on the next write either way. Verified with golangci-lint v2.12.2, the version CI pins -- the local v2.7.2 rejects this repo's config outright, which is why this was not caught before pushing. Co-Authored-By: Claude Opus 5 (1M context) --- keyring.go | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/keyring.go b/keyring.go index 21c059a..88dedcc 100644 --- a/keyring.go +++ b/keyring.go @@ -72,36 +72,28 @@ func (v *KeyringVault) legacyNamespaced(kind, key string) string { return fmt.Sprintf("%s-%s-%s", v.id, kind, key) } -func (v *KeyringVault) metadataKey() string { - return v.namespaced("metadata", "") -} - -func (v *KeyringVault) secretKey(key string) string { - return v.namespaced("secret", key) -} - -func (v *KeyringVault) secretsListKey() string { - return v.namespaced("secrets-list", "") -} - -// get reads an entry, falling back to the pre-v0.3.0 name. The returned bool -// reports whether the value came from a legacy entry and so needs migrating. -func (v *KeyringVault) get(kind, key string) (string, bool, error) { +// get reads an entry, falling back to the pre-v0.3.0 name. +// +// Whether the value came from a legacy entry is deliberately not reported. It +// would only be useful for deciding to migrate, and set already deletes the +// legacy entry unconditionally, so the migration happens on the next write +// either way. +func (v *KeyringVault) get(kind, key string) (string, error) { data, err := keyring.Get(v.service, v.namespaced(kind, key)) if err == nil { - return data, false, nil + return data, nil } if !errors.Is(err, keyring.ErrNotFound) { - return "", false, err + return "", err } data, legacyErr := keyring.Get(v.service, v.legacyNamespaced(kind, key)) if legacyErr != nil { // Report the miss against the current name; the legacy lookup is an // implementation detail and its error would only confuse. - return "", false, err + return "", err } - return data, true, nil + return data, nil } // set writes an entry under the current name and removes any legacy entry it @@ -137,7 +129,7 @@ func (v *KeyringVault) initMetadata() error { } func (v *KeyringVault) loadMetadata() error { - data, _, err := v.get("metadata", "") + data, err := v.get("metadata", "") if err != nil { return err } @@ -163,7 +155,7 @@ func (v *KeyringVault) saveMetadata() error { } func (v *KeyringVault) loadSecretsList() ([]string, error) { - data, _, err := v.get("secrets-list", "") + data, err := v.get("secrets-list", "") if err != nil { if errors.Is(err, keyring.ErrNotFound) { return []string{}, nil @@ -243,7 +235,7 @@ func (v *KeyringVault) GetSecret(key string) (Secret, error) { return nil, err } - data, _, err := v.get("secret", key) + data, err := v.get("secret", key) if err != nil { if errors.Is(err, keyring.ErrNotFound) { return nil, ErrSecretNotFound @@ -282,7 +274,7 @@ func (v *KeyringVault) DeleteSecret(key string) error { } // Check if secret exists first - _, _, err := v.get("secret", key) + _, err := v.get("secret", key) if err != nil { if errors.Is(err, keyring.ErrNotFound) { return ErrSecretNotFound @@ -325,7 +317,7 @@ func (v *KeyringVault) HasSecret(key string) (bool, error) { return false, err } - _, _, err := v.get("secret", key) + _, err := v.get("secret", key) if err != nil { if errors.Is(err, keyring.ErrNotFound) { return false, nil From e692ac8e12ec01490d68e9e8c50dc0e73decc4c8 Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Sun, 26 Jul 2026 20:11:12 -0400 Subject: [PATCH 6/6] Remove unnecessary comments from config.go Removed comments about vault config security and readability. --- config.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/config.go b/config.go index 7500376..f14682c 100644 --- a/config.go +++ b/config.go @@ -78,10 +78,6 @@ func SaveConfigJSON(config Config, path string) error { return fmt.Errorf("failed to marshal config: %w", err) } - // Owner-only, matching the vault storage directory. A vault config carries - // the provider's command templates and environment values, and its presence - // alone discloses which secret backends a user has configured; there is no - // reason for it to be group-readable when the file itself is 0600. if err := os.MkdirAll(filepath.Dir(path), vaultDirMode); err != nil { return fmt.Errorf("failed to create config directory: %w", err) }