diff --git a/TODO.md b/TODO.md index 8e76667..b919e96 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,13 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 2026-10-03: `version rm`, `version promote` and `get --version` + accept a version only if it is one of the versions `version list` + lists for that secret, compared as typed before any path is built + (`secret.VersionExists`), and touch nothing otherwise. An empty + `--version` is rejected instead of meaning the current version. + Before, `secret version rm x ../../..` deleted the whole vault, + `secret version rm x ..` the secret, and `.` or `""` every version. - 2026-10-03: Key material is wiped on every exit: `Entry()` returns the exit code after its deferred `memguard.Purge()` has run, and only `main` calls `os.Exit`. SIGINT and SIGTERM go through memguard's diff --git a/internal/cli/integration_test.go b/internal/cli/integration_test.go index adf4a6c..8e410d8 100644 --- a/internal/cli/integration_test.go +++ b/internal/cli/integration_test.go @@ -829,6 +829,14 @@ func test09GetSpecificVersion(t *testing.T, tempDir, testMnemonic string, runSec require.NoError(t, err, "get current version should succeed") assert.Equal(t, "newpassword456", strings.TrimSpace(output), "should return new secret value without --version") + + // An empty --version is not a version; it does not mean the current one + output, err = runSecretWithEnv(map[string]string{ + secret.EnvMnemonic: testMnemonic, + }, "get", "--version", "", "database/password") + + require.Error(t, err, "get with an empty version should fail") + assert.Contains(t, output, "version '' not found", "should reject the empty version") } func test10PromoteVersion(t *testing.T, tempDir, testMnemonic string, runSecret func(...string) (string, error), runSecretWithEnv func(map[string]string, ...string) (string, error)) { diff --git a/internal/cli/path_traversal_test.go b/internal/cli/path_traversal_test.go index 35889d8..9e242ce 100644 --- a/internal/cli/path_traversal_test.go +++ b/internal/cli/path_traversal_test.go @@ -1,6 +1,7 @@ package cli_test import ( + "fmt" "maps" "os" "slices" @@ -119,13 +120,13 @@ func newFsFromSnapshot(t *testing.T, tree map[string]string) afero.Fs { } // requireRejectedAndUnchanged runs a command on a copy of the state -// directory recorded in before. It requires exactly the error -// vault.ValidateSecretName gives for the rejected name, so that a later -// check rejecting the name does not count, and everything under the state -// directory as it was: the error alone proves nothing, since it could come -// after the vault had already been deleted. +// directory recorded in before. It requires an error with exactly the +// message of want, so that a later check rejecting the argument does not +// count, and everything under the state directory as it was: the error +// alone proves nothing, since it could come after the vault had already +// been deleted. func requireRejectedAndUnchanged( - t *testing.T, before map[string]string, rejected string, + t *testing.T, before map[string]string, want error, run func(c *cli.Instance) error, ) { t.Helper() @@ -135,8 +136,7 @@ func requireRejectedAndUnchanged( err := run(cli.NewCLIInstanceWithStateDir(fs, testStateDir)) require.Equal(t, before, snapshotStateDir(t, fs)) - require.ErrorIs(t, err, vault.ErrInvalidSecretName) - require.EqualError(t, err, vault.ValidateSecretName(rejected).Error()) + require.EqualError(t, err, want.Error()) } // TestInvalidSecretNameLeavesVaultsUnchanged is a regression test for @@ -229,11 +229,92 @@ func TestInvalidSecretNameLeavesVaultsUnchanged(t *testing.T) { for _, tt := range tests { t.Run(tt.command, func(t *testing.T) { - requireRejectedAndUnchanged(t, before, tt.rejected, tt.run) + requireRejectedAndUnchanged(t, before, vault.ValidateSecretName(tt.rejected), tt.run) }) } } +// TestInvalidVersionLeavesVaultsUnchanged is a regression test for +// https://git.eeqj.de/sneak/secret/issues/67, where +// `secret version rm x ../../..` deleted the whole vault, +// `secret version rm x ..` the secret x, and `secret version rm x .` or +// `secret version rm x ""` every version of x. A version argument is +// accepted only if it is one of the versions `secret version list` lists. +// +//nolint:paralleltest // newTwoVaultFs uses t.Setenv +func TestInvalidVersionLeavesVaultsUnchanged(t *testing.T) { + before := snapshotStateDir(t, newTwoVaultFs(t)) + + cmd := &cobra.Command{} + + commands := []struct { + command string + run func(c *cli.Instance, version string) error + }{ + {"version rm x", func(c *cli.Instance, version string) error { + return c.RemoveVersion(cmd, "x", version) + }}, + {"version promote x", func(c *cli.Instance, version string) error { + return c.PromoteVersion(cmd, "x", version) + }}, + {"get x --version", func(c *cli.Instance, version string) error { + return c.GetSecretWithVersion(cmd, "x", version) + }}, + } + + for _, tt := range commands { + for _, version := range []string{"", ".", "..", "../../..", "a/b"} { + t.Run(fmt.Sprintf("%s %q", tt.command, version), func(t *testing.T) { + want := fmt.Errorf("version '%s' %w '%s'", + version, vault.ErrVersionNotFound, "x") + requireRejectedAndUnchanged(t, before, want, + func(c *cli.Instance) error { return tt.run(c, version) }) + }) + } + } +} + +// TestRemoveVersionRemovesOnlyThatVersion checks that `secret version rm` +// with a version that is not the current one removes that version and +// changes nothing else. +// +//nolint:paralleltest // newTwoVaultFs uses t.Setenv +func TestRemoveVersionRemovesOnlyThatVersion(t *testing.T) { + fs := newTwoVaultFs(t) + + vlt, err := vault.GetCurrentVault(fs, testStateDir) + require.NoError(t, err) + + // A second version of "x" becomes the current one. + err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("new")), true) + require.NoError(t, err) + + secretDir := testStateDir + "/vaults.d/default/secrets.d/x" + versions, err := secret.ListVersions(fs, secretDir) + require.NoError(t, err) + require.Len(t, versions, 2) + + // ListVersions lists the newest version first. + oldDir := secretDir + "/versions/" + versions[1] + "/" + before := snapshotStateDir(t, fs) + require.Contains(t, before, oldDir) + + c := cli.NewCLIInstanceWithStateDir(fs, testStateDir) + err = c.RemoveVersion(&cobra.Command{}, "x", versions[1]) + require.NoError(t, err) + + // Expected: the state as before without everything under oldDir. + want := map[string]string{} + + for path, content := range before { + if !strings.HasPrefix(path, oldDir) { + want[path] = content + } + } + + require.Equal(t, want, snapshotStateDir(t, fs)) +} + // TestMoveToVaultNameRenamesInCurrentVault checks that `secret mv x work`, // where "work" is also the name of a vault, renames the secret "x" to "work" // in the current vault and changes nothing else. diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index 7fbf6f0..9f394e2 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -109,6 +109,12 @@ func newGetCmd() *cobra.Command { return fmt.Errorf("failed to initialize CLI: %w", err) } + // Without --version, get the current version. A given + // --version is checked as typed, so an empty one is rejected. + if !cmd.Flags().Changed("version") { + return cli.GetSecret(cmd, args[0]) + } + return cli.GetSecretWithVersion(cmd, args[0], version) }, } @@ -393,12 +399,32 @@ func (cli *Instance) AddSecret(secretName string, force bool) error { return nil } -// GetSecret retrieves and prints a secret from the current vault +// GetSecret retrieves and prints the current version of a secret func (cli *Instance) GetSecret(cmd *cobra.Command, secretName string) error { - return cli.GetSecretWithVersion(cmd, secretName, "") + secret.Debug("GetSecret called", "secretName", secretName) + + // Store the command for output + cli.cmd = cmd + + // Get current vault + vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) + if err != nil { + return err + } + + value, err := vlt.GetSecret(secretName) + if err != nil { + return err + } + + // Print the secret value to stdout + _, _ = cli.Print(string(value)) + + return nil } -// GetSecretWithVersion retrieves and prints a specific version of a secret +// GetSecretWithVersion retrieves and prints a specific version of a secret. +// The version must be one of the secret's versions. func (cli *Instance) GetSecretWithVersion( cmd *cobra.Command, secretName string, version string, ) error { @@ -417,13 +443,7 @@ func (cli *Instance) GetSecretWithVersion( } // Get the secret value - var value []byte - if version == "" { - value, err = vlt.GetSecret(secretName) - } else { - value, err = vlt.GetSecretVersion(secretName, version) - } - + value, err := vlt.GetSecretVersion(secretName, version) if err != nil { secret.Debug("Failed to get secret", "error", err) diff --git a/internal/cli/version.go b/internal/cli/version.go index feb04d9..3fada21 100644 --- a/internal/cli/version.go +++ b/internal/cli/version.go @@ -265,9 +265,7 @@ func (cli *Instance) PromoteVersion( secretDir := filepath.Join(vaultDir, "secrets.d", encodedName) // Check if version exists - versionDir := filepath.Join(secretDir, "versions", version) - - exists, err := afero.DirExists(cli.fs, versionDir) + exists, err := secret.VersionExists(cli.fs, secretDir, version) if err != nil { return fmt.Errorf("failed to check if version exists: %w", err) } @@ -323,9 +321,7 @@ func (cli *Instance) RemoveVersion( } // Check if version exists - versionDir := filepath.Join(secretDir, "versions", version) - - exists, err = afero.DirExists(cli.fs, versionDir) + exists, err = secret.VersionExists(cli.fs, secretDir, version) if err != nil { return fmt.Errorf("failed to check if version exists: %w", err) } @@ -348,6 +344,8 @@ func (cli *Instance) RemoveVersion( } // Remove the version directory + versionDir := filepath.Join(secretDir, "versions", version) + err = cli.fs.RemoveAll(versionDir) if err != nil { return fmt.Errorf("failed to remove version: %w", err) diff --git a/internal/cli/version_test.go b/internal/cli/version_test.go index 79b9e44..d2296d8 100644 --- a/internal/cli/version_test.go +++ b/internal/cli/version_test.go @@ -276,8 +276,8 @@ func TestGetSecretWithVersion(t *testing.T) { var buf bytes.Buffer cmd.SetOut(&buf) - // Test getting current version (empty version string) - err = cli.GetSecretWithVersion(cmd, "test/secret", "") + // Test getting the current version + err = cli.GetSecret(cmd, "test/secret") require.NoError(t, err) assert.Equal(t, "version-2", buf.String()) diff --git a/internal/secret/version.go b/internal/secret/version.go index 39efed2..46f72fb 100644 --- a/internal/secret/version.go +++ b/internal/secret/version.go @@ -6,6 +6,7 @@ import ( "fmt" "log/slog" "path/filepath" + "slices" "sort" "strings" "time" @@ -524,6 +525,18 @@ func ListVersions(fs afero.Fs, secretDir string) ([]string, error) { return versions, nil } +// VersionExists reports whether version is one of the versions ListVersions +// lists for the secret in secretDir. It only compares names, so a version +// the user typed can be checked with it before any path is built from it. +func VersionExists(fs afero.Fs, secretDir string, version string) (bool, error) { + versions, err := ListVersions(fs, secretDir) + if err != nil { + return false, err + } + + return slices.Contains(versions, version), nil +} + // GetCurrentVersion returns the version that the "current" file points to // The file contains just the version name (e.g., "20231215.001") func GetCurrentVersion(fs afero.Fs, secretDir string) (string, error) { diff --git a/internal/vault/errors.go b/internal/vault/errors.go index 2fb57ee..706fb65 100644 --- a/internal/vault/errors.go +++ b/internal/vault/errors.go @@ -49,7 +49,7 @@ var ( // ErrVersionNotFound indicates the requested secret version does not // exist. Composed as - // "version not found for secret ". + // "version '' not found for secret ''". ErrVersionNotFound = errors.New("not found for secret") // ErrNoVersions indicates the source secret has no versions. Composed diff --git a/internal/vault/integration_version_test.go b/internal/vault/integration_version_test.go index 806c89c..b20a569 100644 --- a/internal/vault/integration_version_test.go +++ b/internal/vault/integration_version_test.go @@ -235,10 +235,10 @@ func testRetrieveSpecificVersions( require.NoError(t, err) assert.Equal(t, []byte("version-3-data"), value3) - // Empty version should return current - valueCurrent, err := vault.GetSecretVersion(secretName, "") - require.NoError(t, err) - assert.Equal(t, []byte("version-3-data"), valueCurrent) + // An empty version is not one of the versions; GetSecret gets the + // current one + _, err = vault.GetSecretVersion(secretName, "") + require.ErrorIs(t, err, ErrVersionNotFound) } func testPromoteOldVersion( diff --git a/internal/vault/secrets.go b/internal/vault/secrets.go index 1ae7769..17c6cdc 100644 --- a/internal/vault/secrets.go +++ b/internal/vault/secrets.go @@ -259,18 +259,31 @@ func updateVersionMetadata( return nil } -// GetSecret retrieves a secret from this vault +// GetSecret retrieves the current version of a secret from this vault func (v *Vault) GetSecret(name string) ([]byte, error) { secret.DebugWith("Getting secret from vault", slog.String("vault_name", v.Name), slog.String("secret_name", name), ) - return v.GetSecretVersion(name, "") + // GetSecretObject validates the name and checks that the secret exists + secretObj, err := v.GetSecretObject(name) + if err != nil { + return nil, err + } + + currentVersion, err := secret.GetCurrentVersion(v.fs, secretObj.Directory) + if err != nil { + secret.Debug("Failed to get current version", "error", err, "secret_name", name) + + return nil, fmt.Errorf("failed to get current version: %w", err) + } + + return v.GetSecretVersion(name, currentVersion) } -// GetSecretVersion retrieves a specific version of a secret (empty version -// means current) +// GetSecretVersion retrieves a specific version of a secret. The version +// must be one of the secret's versions; GetSecret gets the current one. func (v *Vault) GetSecretVersion(name string, version string) ([]byte, error) { secret.DebugWith("Getting secret version from vault", slog.String("vault_name", v.Name), @@ -278,8 +291,8 @@ func (v *Vault) GetSecretVersion(name string, version string) ([]byte, error) { slog.String("version", version), ) - // Validate the name and resolve the version to fetch - version, err := v.resolveSecretVersion(name, version) + // Validate the name and check that the version exists + err := v.checkSecretVersion(name, version) if err != nil { return nil, err } @@ -640,15 +653,15 @@ func (v *Vault) updatePreviousVersion( return nil } -// resolveSecretVersion validates the secret name, verifies the secret and -// version exist, and resolves an empty version to the current one. -func (v *Vault) resolveSecretVersion(name, version string) (string, error) { +// checkSecretVersion validates the secret name and verifies that the secret +// exists and that version is one of its versions. +func (v *Vault) checkSecretVersion(name, version string) error { // Validate secret name to prevent path traversal err := ValidateSecretName(name) if err != nil { secret.Debug("Invalid secret name provided", "secret_name", name) - return "", err + return err } // Get vault directory @@ -656,7 +669,7 @@ func (v *Vault) resolveSecretVersion(name, version string) (string, error) { if err != nil { secret.Debug("Failed to get vault directory", "error", err, "vault_name", v.Name) - return "", err + return err } // Convert slashes to percent signs for storage @@ -668,50 +681,30 @@ func (v *Vault) resolveSecretVersion(name, version string) (string, error) { if err != nil { secret.Debug("Failed to check if secret exists", "error", err, "secret_name", name) - return "", fmt.Errorf("failed to check if secret exists: %w", err) + return fmt.Errorf("failed to check if secret exists: %w", err) } if !exists { secret.Debug("Secret not found in vault", "secret_name", name, "vault_name", v.Name) - return "", fmt.Errorf("secret %s %w", name, ErrSecretNotFound) - } - - // Determine which version to get - if version == "" { - // Get current version - currentVersion, err := secret.GetCurrentVersion(v.fs, secretDir) - if err != nil { - secret.Debug("Failed to get current version", "error", err, "secret_name", name) - - return "", fmt.Errorf("failed to get current version: %w", err) - } - - version = currentVersion - - secret.Debug("Using current version", "version", version, "secret_name", name) + return fmt.Errorf("secret %s %w", name, ErrSecretNotFound) } // Check if version exists - versionPath := filepath.Join(secretDir, "versions", version) - - exists, err = afero.DirExists(v.fs, versionPath) + exists, err = secret.VersionExists(v.fs, secretDir, version) if err != nil { secret.Debug("Failed to check if version exists", "error", err, "version", version) - return "", fmt.Errorf("failed to check if version exists: %w", err) + return fmt.Errorf("failed to check if version exists: %w", err) } if !exists { secret.Debug("Version not found", "version", version, "secret_name", name) - return "", fmt.Errorf( - "version %s %w %s", - version, ErrVersionNotFound, name, - ) + return fmt.Errorf("version '%s' %w '%s'", version, ErrVersionNotFound, name) } - return version, nil + return nil } // createAndSaveVersion generates a new version name, sets the version diff --git a/internal/vault/secrets_version_test.go b/internal/vault/secrets_version_test.go index c9ac261..6a0d9d8 100644 --- a/internal/vault/secrets_version_test.go +++ b/internal/vault/secrets_version_test.go @@ -202,10 +202,10 @@ func TestVaultGetSecretVersion(t *testing.T) { require.NoError(t, err) assert.Equal(t, []byte("version-2"), value) - // Get current (empty version) - value, err = vault.GetSecretVersion(testSecretPath, "") - require.NoError(t, err) - assert.Equal(t, []byte("version-2"), value) + // An empty version is not one of the versions; GetSecret gets the + // current one + _, err = vault.GetSecretVersion(testSecretPath, "") + require.ErrorIs(t, err, ErrVersionNotFound) } //nolint:paralleltest // createTestVaultWithKey uses t.Setenv