From 007254a1f0eb23b22cc849b71438d7de84a7f8db Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 4 Oct 2026 13:58:46 +0200 Subject: [PATCH] Check vault names in every command that takes one (closes #68) A vault name may use only lowercase ASCII letters, digits, `.`, `-` and `_`, and must not be empty, `.` or `..`; the error now states that rule. `vault create`, `vault import`, `vault select`, `vault remove`, both vault names of `mv` and shell completion of a `vault:secret` argument check the name as typed before building any path from it. Before, `vault import ..` wrote a long-term key and an unlocker into the state directory itself, and `vault select ..` made that the current vault. Model: opus-5-5 --- README.md | 3 ++ TODO.md | 9 +++++ internal/cli/completions.go | 8 ++++- internal/cli/completions_test.go | 41 +++++++++++++++++++++++ internal/cli/move_test.go | 19 +++++------ internal/cli/path_traversal_test.go | 52 +++++++++++++++++++++++++++++ internal/cli/secrets.go | 16 ++++++--- internal/cli/vault.go | 10 ++++++ internal/vault/errors.go | 7 ++-- internal/vault/management.go | 41 ++++++++++++++--------- 10 files changed, 172 insertions(+), 34 deletions(-) create mode 100644 internal/cli/completions_test.go diff --git a/README.md b/README.md index f89b5a6..a50be3c 100644 --- a/README.md +++ b/README.md @@ -91,6 +91,9 @@ Lists all available vaults. The current vault is marked. Creates a new vault with the specified name. +**Vault Name Format:** only lowercase ASCII letters, digits, `.`, `-` and `_` +are allowed, and a name must not be empty, `.` or `..`. + #### `secret vault select ` Switches to the specified vault for subsequent operations. diff --git a/TODO.md b/TODO.md index d89b291..46c18f7 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,15 @@ Bring the repo into policy compliance in one commit: # Completed Steps +- 2026-10-04: A vault name may use only lowercase ASCII letters, digits, + `.`, `-` and `_`, and must not be empty, `.` or `..` + (https://git.eeqj.de/sneak/secret/issues/68); the error and `README.md` + state the rule. `vault create`, `vault import`, `vault select`, + `vault remove`, both vault names of `mv` and shell completion of a + `vault:secret` argument check the name as typed with + `vault.ValidateVaultName` before building any path from it. Before, + `vault import ..` wrote a long-term key and an unlocker into the state + directory itself, and `vault select ..` made that the current vault. - 2026-10-04: `script/cibuild` runs the checks again on an unchanged tree (https://git.eeqj.de/sneak/secret/issues/54). It passes the current time as the `CHECK_EPOCH` build argument, which both the lint diff --git a/internal/cli/completions.go b/internal/cli/completions.go index 576fa2f..e1ecb8c 100644 --- a/internal/cli/completions.go +++ b/internal/cli/completions.go @@ -123,7 +123,9 @@ func getVaultNamesCompletionFunc(fs afero.Fs, stateDir string) func( } // completeVaultQualifiedSecrets completes "vault:secret" references once a -// colon is present in the input +// colon is present in the input. It completes nothing when the vault part +// is not a valid vault name, so that a name such as ".." cannot list a +// directory outside vaults.d. func completeVaultQualifiedSecrets( fs afero.Fs, stateDir, toComplete string, ) []string { @@ -134,6 +136,10 @@ func completeVaultQualifiedSecrets( vaultName := parts[0] secretPrefix := parts[1] + if vault.ValidateVaultName(vaultName) != nil { + return nil + } + vlt := vault.NewVault(fs, stateDir, vaultName) secrets, err := vlt.ListSecrets() diff --git a/internal/cli/completions_test.go b/internal/cli/completions_test.go new file mode 100644 index 0000000..c53e92a --- /dev/null +++ b/internal/cli/completions_test.go @@ -0,0 +1,41 @@ +//nolint:testpackage // white-box test of unexported internals +package cli + +import ( + "path/filepath" + "testing" + + "github.com/spf13/afero" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestVaultSecretCompletionRejectsInvalidVaultName is a regression test for +// https://git.eeqj.de/sneak/secret/issues/68: completing a `vault:secret` +// argument lists nothing when the vault part is not a valid vault name, even +// where that name, joined onto vaults.d, leads to a secrets.d directory. +func TestVaultSecretCompletionRejectsInvalidVaultName(t *testing.T) { + t.Parallel() + + const ( + stateDir = "/state" + dirPerm = 0o700 + ) + + fs := afero.NewMemMapFs() + + // The vault "work" holds the secret "x". So does every directory an + // invalid name below would lead to from vaults.d. + for _, vaultName := range []string{"work", ".", "..", "a/b"} { + secretDir := filepath.Join(stateDir, "vaults.d", vaultName, "secrets.d", "x") + require.NoError(t, fs.MkdirAll(secretDir, dirPerm)) + } + + assert.Equal(t, []string{"work:x"}, + completeVaultQualifiedSecrets(fs, stateDir, "work:")) + + for _, toComplete := range []string{".:", "..:", "a/b:"} { + assert.Empty(t, completeVaultQualifiedSecrets(fs, stateDir, toComplete), + "completing %q", toComplete) + } +} diff --git a/internal/cli/move_test.go b/internal/cli/move_test.go index a5f216e..9564ae3 100644 --- a/internal/cli/move_test.go +++ b/internal/cli/move_test.go @@ -48,26 +48,25 @@ func TestRejectedMoveWithinVaultLeavesStateUnchanged(t *testing.T) { "mv work:nosuch work:y", "work:nosuch", "work:y", false, "secret 'nosuch' not found", }, - // Only an existing vault is used, so ".." cannot reach the state - // directory itself. + // Only an existing vault is used. { - "mv --force ..:x ..:y", "..:x", "..:y", true, - "vault '..' does not exist", + "mv --force nosuch:x nosuch:y", "nosuch:x", "nosuch:y", true, + "vault 'nosuch' does not exist", }, - // Each of these spells "work" a second way. The spelling is not an - // existing vault name, so the move is not taken for a move between - // two vaults, which would delete the destination, here the source. + // Each of these spells "work" a second way. The spelling is not a + // valid vault name, so the move is not taken for a move between two + // vaults, which would delete the destination, here the source. { "mv --force work:x work/:x", workX, "work/:x", true, - "vault 'work/' does not exist", + vault.ValidateVaultName("work/").Error(), }, { "mv --force work/:x work:", "work/:x", "work:", true, - "vault 'work/' does not exist", + vault.ValidateVaultName("work/").Error(), }, { "mv --force work:x ./work:x", workX, "./work:x", true, - "vault './work' does not exist", + vault.ValidateVaultName("./work").Error(), }, } diff --git a/internal/cli/path_traversal_test.go b/internal/cli/path_traversal_test.go index 75dbc76..81c3a86 100644 --- a/internal/cli/path_traversal_test.go +++ b/internal/cli/path_traversal_test.go @@ -292,6 +292,58 @@ func TestInvalidVersionLeavesVaultsUnchanged(t *testing.T) { } } +// TestInvalidVaultNameLeavesStateUnchanged is a regression test for +// https://git.eeqj.de/sneak/secret/issues/68, where +// `secret vault import ..` wrote a long-term key and an unlocker into the +// state directory itself, and `secret vault select ..` made it the current +// vault. Each command that takes a vault name must reject an invalid one +// before building a path from it. The mnemonic and the passphrase are set, +// and moves and removals use --force, so that only the name check stands +// in the way. +// +//nolint:paralleltest // newTwoVaultFs uses t.Setenv +func TestInvalidVaultNameLeavesStateUnchanged(t *testing.T) { + before := snapshotStateDir(t, newTwoVaultFs(t)) + + t.Setenv(secret.EnvUnlockPassphrase, testPassphrase) + + cmd := &cobra.Command{} + + // Each command is a format with %q where the vault name goes. + commands := []struct { + command string + run func(c *cli.Instance, name string) error + }{ + {"vault create %q", func(c *cli.Instance, name string) error { + return c.CreateVault(cmd, name) + }}, + {"vault import %q", func(c *cli.Instance, name string) error { + return c.VaultImport(cmd, name) + }}, + {"vault select %q", func(c *cli.Instance, name string) error { + return c.SelectVault(cmd, name) + }}, + {"vault remove --force %q", func(c *cli.Instance, name string) error { + return c.RemoveVault(cmd, name, true) + }}, + {"mv --force %q:x work:x", func(c *cli.Instance, name string) error { + return c.MoveSecret(cmd, name+":x", "work:x", true) + }}, + {"mv --force default:x %q:x", func(c *cli.Instance, name string) error { + return c.MoveSecret(cmd, "default:x", name+":x", true) + }}, + } + + for _, tt := range commands { + for _, name := range []string{"", ".", "..", "a/b"} { + t.Run(fmt.Sprintf(tt.command, name), func(t *testing.T) { + requireRejectedAndUnchanged(t, before, vault.ValidateVaultName(name), + func(c *cli.Instance) error { return tt.run(c, name) }) + }) + } + } +} + // TestRemoveVersionRemovesOnlyThatVersion checks that `secret version rm` // with a version that is not the current one removes that version and // changes nothing else. diff --git a/internal/cli/secrets.go b/internal/cli/secrets.go index 9cef727..052a7a8 100644 --- a/internal/cli/secrets.go +++ b/internal/cli/secrets.go @@ -811,9 +811,9 @@ func (cli *Instance) moveSecret( cmd, vlt, srcSecretName, destSecretName, force) } - // Both vaults must be existing vaults by exact name, so that two - // spellings of one vault, such as "work" and "work/", are never taken for - // two vaults. A named vault does not become the current vault. + // Both vault names must be valid and name existing vaults exactly, so + // that two spellings of one vault, such as "work" and "work/", are never + // taken for two vaults. A named vault does not become the current vault. srcVault, err := cli.existingVault(srcVaultName) if err != nil { return err @@ -833,9 +833,15 @@ func (cli *Instance) moveSecret( cmd, srcVault, srcSecretName, destVault, destSecretName, force) } -// existingVault returns the vault with the given name, or an error if there -// is none. Unlike vault.SelectVault, it leaves the current vault as it is. +// existingVault returns the vault with the given name, or an error if the +// name is not a valid vault name or there is no such vault. Unlike +// vault.SelectVault, it leaves the current vault as it is. func (cli *Instance) existingVault(name string) (*vault.Vault, error) { + err := vault.ValidateVaultName(name) + if err != nil { + return nil, err + } + vaults, err := vault.ListVaults(cli.fs, cli.stateDir) if err != nil { return nil, fmt.Errorf("failed to list vaults: %w", err) diff --git a/internal/cli/vault.go b/internal/cli/vault.go index e2271a2..a7c93c0 100644 --- a/internal/cli/vault.go +++ b/internal/cli/vault.go @@ -462,6 +462,11 @@ func updateVaultImportMetadata( // VaultImport imports a mnemonic into a specific vault, holding the state // directory lock while importMnemonic runs func (cli *Instance) VaultImport(cmd *cobra.Command, vaultName string) error { + err := vault.ValidateVaultName(vaultName) + if err != nil { + return err + } + release, err := vault.LockStateDir(cli.fs, cli.stateDir) if err != nil { return err @@ -617,6 +622,11 @@ func (cli *Instance) switchAwayFromVault( // RemoveVault removes a vault, holding the state directory lock while // removeVault runs func (cli *Instance) RemoveVault(cmd *cobra.Command, name string, force bool) error { + err := vault.ValidateVaultName(name) + if err != nil { + return err + } + release, err := vault.LockStateDir(cli.fs, cli.stateDir) if err != nil { return err diff --git a/internal/vault/errors.go b/internal/vault/errors.go index 37af7c7..a4f0e27 100644 --- a/internal/vault/errors.go +++ b/internal/vault/errors.go @@ -17,9 +17,10 @@ var ( "derived public key does not match vault: mnemonic may be incorrect", ) - // ErrInvalidVaultName indicates a vault name that does not match the - // allowed pattern [a-z0-9.\-_]+. Composed as - // "invalid vault name '': must match pattern [a-z0-9.\-_]+". + // ErrInvalidVaultName indicates a vault name that breaks the naming + // rule: only lowercase ASCII letters, digits, '.', '-' and '_'; not + // empty, "." or "..". Composed by ValidateVaultName as + // "invalid vault name '': ". ErrInvalidVaultName = errors.New("invalid vault name") // ErrVaultNotFound indicates the named vault does not exist. Composed diff --git a/internal/vault/management.go b/internal/vault/management.go index af545d6..14f06e3 100644 --- a/internal/vault/management.go +++ b/internal/vault/management.go @@ -24,10 +24,12 @@ func init() { }) } -// isValidVaultName validates vault names according to the format [a-z0-9\.\-\_]+ -// Note: We don't allow slashes in vault names unlike secret names +// isValidVaultName reports whether name is a valid vault name: only +// lowercase ASCII letters, digits, '.', '-' and '_', and not empty, "." or +// "..". With no path separator allowed, a vault is always one directory +// directly under vaults.d. func isValidVaultName(name string) bool { - if name == "" { + if name == "" || name == "." || name == ".." { return false } @@ -36,6 +38,21 @@ func isValidVaultName(name string) bool { return matched } +// ValidateVaultName returns an error wrapping ErrInvalidVaultName when name +// is not a valid vault name. Call it on the name exactly as the user gave it, +// before building any path from it. +func ValidateVaultName(name string) error { + if !isValidVaultName(name) { + return fmt.Errorf( + "%w '%s': only lowercase ASCII letters, digits, '.', '-' and '_' "+ + "are allowed, and a name must not be empty, '.' or '..'", + ErrInvalidVaultName, name, + ) + } + + return nil +} + // ResolveVaultSymlink reads the currentvault file to get the path to the current vault // The file contains just the vault name (e.g., "default") func ResolveVaultSymlink(fs afero.Fs, currentVaultPath string) (string, error) { @@ -199,14 +216,11 @@ func processMnemonicForVault( func CreateVault(fs afero.Fs, stateDir string, name string) (*Vault, error) { secret.Debug("Creating new vault", "name", name, "state_dir", stateDir) - // Validate vault name - if !isValidVaultName(name) { + err := ValidateVaultName(name) + if err != nil { secret.Debug("Invalid vault name provided", "vault_name", name) - return nil, fmt.Errorf( - "%w '%s': must match pattern [a-z0-9.\\-_]+", - ErrInvalidVaultName, name, - ) + return nil, err } secret.Debug("Vault name validation passed", "vault_name", name) @@ -285,14 +299,11 @@ func CreateVault(fs afero.Fs, stateDir string, name string) (*Vault, error) { func SelectVault(fs afero.Fs, stateDir string, name string) error { secret.Debug("Selecting vault", "vault_name", name, "state_dir", stateDir) - // Validate vault name - if !isValidVaultName(name) { + err := ValidateVaultName(name) + if err != nil { secret.Debug("Invalid vault name provided", "vault_name", name) - return fmt.Errorf( - "%w '%s': must match pattern [a-z0-9.\\-_]+", - ErrInvalidVaultName, name, - ) + return err } secret.Debug("Vault name validation passed", "vault_name", name)