Reject invalid secret names before any command builds a path (closes #33)
check / check (push) Successful in 1m13s

`secret rm ..` resolved to the vault directory and deleted the whole
vault; `secret rm .` and `secret rm ""` deleted every secret. rm, mv and
import built paths from the name without checking it, and so did the
version commands, encrypt and decrypt.

vault.ValidateSecretName wraps the existing name rule and returns
ErrInvalidSecretName. Each of those commands calls it on the name as
given, both names for a move, before building any path. AddSecret and
GetSecretVersion use it too, so the rule has one implementation.

The regression test snapshots every file under the state directory of
two in-memory vaults and requires it unchanged after each rejected
command.

Model: opus-5-5
This commit is contained in:
2026-10-03 12:31:35 +00:00
parent d52b4f1240
commit 526a6be17b
6 changed files with 264 additions and 12 deletions
+7 -2
View File
@@ -25,6 +25,12 @@ Bring the repo into policy compliance in one commit:
# Completed Steps # Completed Steps
- 2026-10-03: Every command that builds a path from a secret name
checks the name first with `vault.ValidateSecretName` and touches
nothing when it is invalid: `rm`, `mv` (both names, within a vault
and between vaults), `import`, `version list`/`promote`/`rm`,
`encrypt` and `decrypt`. Before, `secret rm ..` deleted the whole
vault and `secret rm .` every secret in it.
- 2026-10-02: A plain `docker build .` builds again: the size tests - 2026-10-02: A plain `docker build .` builds again: the size tests
skip a case that needs more locked memory than the process can skip a case that needs more locked memory than the process can
lock, and run every case under `script/cibuild`. The image stamps the lock, and run every case under `script/cibuild`. The image stamps the
@@ -96,8 +102,7 @@ Bring the repo into policy compliance in one commit:
buffer.Bytes() to GPGEncryptFunc and EncryptWithPassphrase. buffer.Bytes() to GPGEncryptFunc and EncryptWithPassphrase.
- Race conditions: no file locking in vault/secrets.go:142-176; - Race conditions: no file locking in vault/secrets.go:142-176;
non-atomic writes can leave the vault inconsistent. non-atomic writes can leave the vault inconsistent.
- Input validation: dots in secret names risk path traversal - Input validation: no maximum secret size (DoS).
(vault/secrets.go:75-99); no maximum secret size (DoS).
- Timing attacks: bytes.Equal passphrase compare (cli/init.go: - Timing attacks: bytes.Equal passphrase compare (cli/init.go:
209-216); non-constant-time public key compare (vault.go:95-100). 209-216); non-constant-time public key compare (vault.go:95-100).
- High priority: - High priority:
+10
View File
@@ -122,6 +122,11 @@ func (cli *Instance) resolveEncryptionKey(
// Encrypt encrypts data using an age secret key stored in a secret // Encrypt encrypts data using an age secret key stored in a secret
func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error { func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
err := vault.ValidateSecretName(secretName)
if err != nil {
return err
}
// Get current vault // Get current vault
vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
if err != nil { if err != nil {
@@ -191,6 +196,11 @@ func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
// Decrypt decrypts data using an age secret key stored in a secret // Decrypt decrypts data using an age secret key stored in a secret
func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error { func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
err := vault.ValidateSecretName(secretName)
if err != nil {
return err
}
// Get current vault // Get current vault
vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
if err != nil { if err != nil {
+182
View File
@@ -0,0 +1,182 @@
package cli_test
import (
"os"
"testing"
"git.eeqj.de/sneak/secret/internal/cli"
"git.eeqj.de/sneak/secret/internal/secret"
"git.eeqj.de/sneak/secret/internal/vault"
"github.com/awnumar/memguard"
"github.com/spf13/afero"
"github.com/spf13/cobra"
"github.com/stretchr/testify/require"
)
const (
// testStateDir is the in-memory state directory of the test vaults.
testStateDir = "/test/state"
// testPassphrase protects the passphrase unlocker of each test vault.
testPassphrase = "test-passphrase"
// testVersion is a version name in the format the vault uses.
testVersion = "20260101.001"
// missingFile is an import source that does not exist, so an import
// that opened it before checking the name would fail with another error.
missingFile = "/no/such/file"
)
// newTwoVaultFs returns an in-memory filesystem holding the vaults "work"
// and "default", the current one. Each holds the secret "x" and a
// passphrase unlocker, so both secrets.d and unlockers.d have contents.
//
//nolint:ireturn // afero.Fs is the filesystem abstraction used throughout
func newTwoVaultFs(t *testing.T) afero.Fs {
t.Helper()
t.Setenv(secret.EnvMnemonic, testMnemonic)
fs := afero.NewMemMapFs()
for _, name := range []string{"work", "default"} {
vlt, err := vault.CreateVault(fs, testStateDir, name)
require.NoError(t, err)
err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("value")), false)
require.NoError(t, err)
_, err = vlt.CreatePassphraseUnlocker(
memguard.NewBufferFromBytes([]byte(testPassphrase)))
require.NoError(t, err)
}
return fs
}
// snapshotStateDir maps every file under the state directory to its
// contents, and every directory, written with a trailing "/", to "". Two
// snapshots are equal only if nothing in it was added, removed or changed.
func snapshotStateDir(t *testing.T, fs afero.Fs) map[string]string {
t.Helper()
tree := map[string]string{}
err := afero.Walk(fs, testStateDir, func(
path string, info os.FileInfo, err error,
) error {
if err != nil {
return err
}
if info.IsDir() {
tree[path+"/"] = ""
return nil
}
content, err := afero.ReadFile(fs, path)
if err != nil {
return err
}
tree[path] = string(content)
return nil
})
require.NoError(t, err)
return tree
}
// requireRejectedAndUnchanged runs a command against the two test vaults
// and requires that it fails with vault.ErrInvalidSecretName and leaves
// everything under the state directory as it was. The error alone proves
// nothing: it could be returned after the vault had already been deleted.
func requireRejectedAndUnchanged(t *testing.T, run func(c *cli.Instance) error) {
t.Helper()
fs := newTwoVaultFs(t)
before := snapshotStateDir(t, fs)
vaultDir := testStateDir + "/vaults.d/default"
require.Contains(t, before, vaultDir+"/secrets.d/x/")
require.Contains(t, before, vaultDir+"/unlockers.d/passphrase/")
err := run(cli.NewCLIInstanceWithStateDir(fs, testStateDir))
require.Equal(t, before, snapshotStateDir(t, fs))
require.ErrorIs(t, err, vault.ErrInvalidSecretName)
}
// TestInvalidSecretNameLeavesVaultsUnchanged is a regression test for
// https://git.eeqj.de/sneak/secret/issues/33, where `secret rm ..` deleted
// the whole vault, and `secret rm .` or `secret rm ""` every secret in it.
// Moves and imports use --force, so that only the name check stands in
// the way.
//
//nolint:paralleltest // subtests use t.Setenv via newTwoVaultFs
func TestInvalidSecretNameLeavesVaultsUnchanged(t *testing.T) {
cmd := &cobra.Command{}
tests := []struct {
command string
run func(c *cli.Instance) error
}{
{"rm ..", func(c *cli.Instance) error {
return c.RemoveSecret(cmd, "..", false)
}},
{"rm .", func(c *cli.Instance) error {
return c.RemoveSecret(cmd, ".", false)
}},
{`rm ""`, func(c *cli.Instance) error {
return c.RemoveSecret(cmd, "", false)
}},
{"rm ../../etc", func(c *cli.Instance) error {
return c.RemoveSecret(cmd, "../../etc", false)
}},
{"mv --force .. x", func(c *cli.Instance) error {
return c.MoveSecret(cmd, "..", "x", true)
}},
{"mv --force x ..", func(c *cli.Instance) error {
return c.MoveSecret(cmd, "x", "..", true)
}},
{"mv --force default:.. work", func(c *cli.Instance) error {
return c.MoveSecret(cmd, "default:..", "work", true)
}},
{"mv --force default:x work:..", func(c *cli.Instance) error {
return c.MoveSecret(cmd, "default:x", "work:..", true)
}},
{"import --force ..", func(c *cli.Instance) error {
return c.ImportSecret(cmd, "..", missingFile, true)
}},
{"import --force .", func(c *cli.Instance) error {
return c.ImportSecret(cmd, ".", missingFile, true)
}},
{"import --force ../../etc", func(c *cli.Instance) error {
return c.ImportSecret(cmd, "../../etc", missingFile, true)
}},
{"version list ..", func(c *cli.Instance) error {
return c.ListVersions(cmd, "..")
}},
{"version promote ..", func(c *cli.Instance) error {
return c.PromoteVersion(cmd, "..", testVersion)
}},
{"version rm ..", func(c *cli.Instance) error {
return c.RemoveVersion(cmd, "..", testVersion)
}},
{"encrypt ..", func(c *cli.Instance) error {
return c.Encrypt("..", "", "")
}},
{"decrypt ..", func(c *cli.Instance) error {
return c.Decrypt("..", "", "")
}},
}
for _, tt := range tests {
t.Run(tt.command, func(t *testing.T) {
requireRejectedAndUnchanged(t, tt.run)
})
}
}
+30
View File
@@ -603,6 +603,11 @@ func printSecretsTable(
func (cli *Instance) ImportSecret( func (cli *Instance) ImportSecret(
cmd *cobra.Command, secretName, sourceFile string, force bool, cmd *cobra.Command, secretName, sourceFile string, force bool,
) error { ) error {
err := vault.ValidateSecretName(secretName)
if err != nil {
return err
}
// Get current vault // Get current vault
vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
if err != nil { if err != nil {
@@ -649,6 +654,11 @@ func (cli *Instance) ImportSecret(
// RemoveSecret removes a secret from the vault // RemoveSecret removes a secret from the vault
func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool) error { func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool) error {
err := vault.ValidateSecretName(secretName)
if err != nil {
return err
}
// Get current vault // Get current vault
currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
if err != nil { if err != nil {
@@ -757,6 +767,16 @@ func (cli *Instance) MoveSecret(
func (cli *Instance) moveSecretWithinVault( func (cli *Instance) moveSecretWithinVault(
cmd *cobra.Command, source, dest string, force bool, cmd *cobra.Command, source, dest string, force bool,
) error { ) error {
err := vault.ValidateSecretName(source)
if err != nil {
return err
}
err = vault.ValidateSecretName(dest)
if err != nil {
return err
}
currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
if err != nil { if err != nil {
return err return err
@@ -815,6 +835,16 @@ func (cli *Instance) moveSecretCrossVault(
destVaultName, destSecretName string, destVaultName, destSecretName string,
force bool, force bool,
) error { ) error {
err := vault.ValidateSecretName(srcSecretName)
if err != nil {
return err
}
err = vault.ValidateSecretName(destSecretName)
if err != nil {
return err
}
// Get source vault // Get source vault
srcVault := vault.NewVault(cli.fs, cli.stateDir, srcVaultName) srcVault := vault.NewVault(cli.fs, cli.stateDir, srcVaultName)
+15
View File
@@ -112,6 +112,11 @@ func VersionCommands(cli *Instance) *cobra.Command {
func (cli *Instance) ListVersions(cmd *cobra.Command, secretName string) error { func (cli *Instance) ListVersions(cmd *cobra.Command, secretName string) error {
secret.Debug("ListVersions called", "secret_name", secretName) secret.Debug("ListVersions called", "secret_name", secretName)
err := vault.ValidateSecretName(secretName)
if err != nil {
return err
}
// Get current vault // Get current vault
vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
if err != nil { if err != nil {
@@ -239,6 +244,11 @@ func formatVersionTime(t *time.Time) string {
func (cli *Instance) PromoteVersion( func (cli *Instance) PromoteVersion(
cmd *cobra.Command, secretName string, version string, cmd *cobra.Command, secretName string, version string,
) error { ) error {
err := vault.ValidateSecretName(secretName)
if err != nil {
return err
}
// Get current vault // Get current vault
vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
if err != nil { if err != nil {
@@ -282,6 +292,11 @@ func (cli *Instance) PromoteVersion(
func (cli *Instance) RemoveVersion( func (cli *Instance) RemoveVersion(
cmd *cobra.Command, secretName string, version string, cmd *cobra.Command, secretName string, version string,
) error { ) error {
err := vault.ValidateSecretName(secretName)
if err != nil {
return err
}
// Get current vault // Get current vault
vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir) vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
if err != nil { if err != nil {
+20 -10
View File
@@ -110,6 +110,20 @@ func isValidSecretName(name string) bool {
return matched return matched
} }
// ValidateSecretName returns an error wrapping ErrInvalidSecretName when
// name is not a valid secret name. Call it on the name exactly as the user
// gave it, before building any path from it.
func ValidateSecretName(name string) error {
if !isValidSecretName(name) {
return fmt.Errorf(
"%w '%s': must match pattern [a-z0-9.\\-_/]+",
ErrInvalidSecretName, name,
)
}
return nil
}
// AddSecret adds a secret to this vault // AddSecret adds a secret to this vault
func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool) error { func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool) error {
if value == nil { if value == nil {
@@ -124,13 +138,11 @@ func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool)
) )
// Validate secret name // Validate secret name
if !isValidSecretName(name) { err := ValidateSecretName(name)
if err != nil {
secret.Debug("Invalid secret name provided", "secret_name", name) secret.Debug("Invalid secret name provided", "secret_name", name)
return fmt.Errorf( return err
"%w '%s': must match pattern [a-z0-9.\\-_/]+",
ErrInvalidSecretName, name,
)
} }
secret.Debug("Secret name validation passed", "secret_name", name) secret.Debug("Secret name validation passed", "secret_name", name)
@@ -628,13 +640,11 @@ func (v *Vault) updatePreviousVersion(
// version exist, and resolves an empty version to the current one. // version exist, and resolves an empty version to the current one.
func (v *Vault) resolveSecretVersion(name, version string) (string, error) { func (v *Vault) resolveSecretVersion(name, version string) (string, error) {
// Validate secret name to prevent path traversal // Validate secret name to prevent path traversal
if !isValidSecretName(name) { err := ValidateSecretName(name)
if err != nil {
secret.Debug("Invalid secret name provided", "secret_name", name) secret.Debug("Invalid secret name provided", "secret_name", name)
return "", fmt.Errorf( return "", err
"%w '%s': must match pattern [a-z0-9.\\-_/]+",
ErrInvalidSecretName, name,
)
} }
// Get vault directory // Get vault directory