Reject invalid secret names before any command builds a path (closes #33)
check / check (push) Successful in 49s
check / check (push) Successful in 49s
`secret rm ..` deleted the whole vault; `secret rm .` and `secret rm ""` deleted every secret. rm, mv, the version commands, encrypt and decrypt built paths from the name unchecked; import checked it only after reading the source file. Each now calls vault.ValidateSecretName, which wraps the existing rule, on the name as given, before building any path; MoveSecret checks both names before switching the current vault. Its error and README.md state the rule. The test-only copy of the rule in internal/secret is removed. The regression test runs each rejected command on a copy of two in-memory vaults and requires the exact error and an unchanged state directory. Model: opus-5-5
This commit is contained in:
@@ -113,7 +113,9 @@ automatically switch to another vault if removing the current one.
|
||||
Adds a secret to the current vault. Reads the secret value from stdin.
|
||||
- `--force, -f`: Overwrite existing secret
|
||||
|
||||
**Secret Name Format:** `[a-z0-9\.\-\_\/]+`
|
||||
**Secret Name Format:** only ASCII letters, digits, `.`, `-`, `_` and `/`
|
||||
are allowed, and a name must not be empty, start with `.` or `/`, end with
|
||||
`/`, contain `//`, or have `..` as a path segment.
|
||||
- Forward slashes (`/`) are converted to percent signs (`%`) for storage
|
||||
- Examples: `database/password`, `api.key`, `ssh_private_key`
|
||||
|
||||
|
||||
@@ -25,6 +25,13 @@ Bring the repo into policy compliance in one commit:
|
||||
|
||||
# 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, before switching the current vault), `import`,
|
||||
`version list`/`promote`/`rm`, `encrypt` and `decrypt`. The error
|
||||
and `README.md` state the naming rule. Before, `secret rm ..`
|
||||
deleted the whole vault and `secret rm .` every secret in it.
|
||||
- 2026-10-03: The keychain unlocker's age key passphrase stays in
|
||||
locked memory: it is generated into a locked buffer, and the
|
||||
keychain JSON is written and read by `KeychainData` code in
|
||||
@@ -100,8 +107,7 @@ Bring the repo into policy compliance in one commit:
|
||||
to GPGEncryptFunc and EncryptWithPassphrase.
|
||||
- Race conditions: no file locking in vault/secrets.go:142-176;
|
||||
non-atomic writes can leave the vault inconsistent.
|
||||
- Input validation: dots in secret names risk path traversal
|
||||
(vault/secrets.go:75-99); no maximum secret size (DoS).
|
||||
- Input validation: no maximum secret size (DoS).
|
||||
- Timing attacks: bytes.Equal passphrase compare (cli/init.go:
|
||||
209-216); non-constant-time public key compare (vault.go:95-100).
|
||||
- High priority:
|
||||
|
||||
@@ -122,6 +122,11 @@ func (cli *Instance) resolveEncryptionKey(
|
||||
|
||||
// Encrypt encrypts data using an age secret key stored in a secret
|
||||
func (cli *Instance) Encrypt(secretName, inputFile, outputFile string) error {
|
||||
err := vault.ValidateSecretName(secretName)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Get current vault
|
||||
vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
|
||||
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
|
||||
func (cli *Instance) Decrypt(secretName, inputFile, outputFile string) error {
|
||||
err := vault.ValidateSecretName(secretName)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Get current vault
|
||||
vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
|
||||
if err != nil {
|
||||
|
||||
@@ -0,0 +1,267 @@
|
||||
package cli_test
|
||||
|
||||
import (
|
||||
"maps"
|
||||
"os"
|
||||
"slices"
|
||||
"strings"
|
||||
"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
|
||||
}
|
||||
|
||||
// newFsFromSnapshot returns a new in-memory filesystem holding exactly the
|
||||
// directories and files recorded by snapshotStateDir.
|
||||
//
|
||||
//nolint:ireturn // afero.Fs is the filesystem abstraction used throughout
|
||||
func newFsFromSnapshot(t *testing.T, tree map[string]string) afero.Fs {
|
||||
t.Helper()
|
||||
|
||||
fs := afero.NewMemMapFs()
|
||||
|
||||
// In sorted order every directory comes before its contents.
|
||||
for _, path := range slices.Sorted(maps.Keys(tree)) {
|
||||
dir, isDir := strings.CutSuffix(path, "/")
|
||||
if isDir {
|
||||
require.NoError(t, fs.MkdirAll(dir, secret.DirPerms))
|
||||
|
||||
continue
|
||||
}
|
||||
|
||||
err := afero.WriteFile(fs, path, []byte(tree[path]), secret.FilePerms)
|
||||
require.NoError(t, err)
|
||||
}
|
||||
|
||||
return 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.
|
||||
func requireRejectedAndUnchanged(
|
||||
t *testing.T, before map[string]string, rejected string,
|
||||
run func(c *cli.Instance) error,
|
||||
) {
|
||||
t.Helper()
|
||||
|
||||
fs := newFsFromSnapshot(t, before)
|
||||
|
||||
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())
|
||||
}
|
||||
|
||||
// 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 // newTwoVaultFs uses t.Setenv
|
||||
func TestInvalidSecretNameLeavesVaultsUnchanged(t *testing.T) {
|
||||
// Creating a passphrase unlocker is slow by design, so the vaults are
|
||||
// created once and each case runs on its own copy of them.
|
||||
before := snapshotStateDir(t, newTwoVaultFs(t))
|
||||
|
||||
vaultDir := testStateDir + "/vaults.d/default"
|
||||
require.Contains(t, before, vaultDir+"/secrets.d/x/")
|
||||
require.Contains(t, before, vaultDir+"/unlockers.d/passphrase/")
|
||||
require.Equal(t, "default", before[testStateDir+"/currentvault"])
|
||||
|
||||
cmd := &cobra.Command{}
|
||||
|
||||
tests := []struct {
|
||||
command string
|
||||
rejected string // the secret name the command must reject
|
||||
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", "../../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 x ""`, "", func(c *cli.Instance) error {
|
||||
return c.MoveSecret(cmd, "x", "", true)
|
||||
}},
|
||||
// "work" is not the current vault: a move within it must not
|
||||
// select it when a name is rejected.
|
||||
{"mv --force work:.. work:x", "..", func(c *cli.Instance) error {
|
||||
return c.MoveSecret(cmd, "work:..", "work:x", true)
|
||||
}},
|
||||
{"mv --force work:x work:..", "..", func(c *cli.Instance) error {
|
||||
return c.MoveSecret(cmd, "work:x", "work:..", true)
|
||||
}},
|
||||
{"mv --force default:.. work", "..", func(c *cli.Instance) error {
|
||||
return c.MoveSecret(cmd, "default:..", "work", true)
|
||||
}},
|
||||
{"mv --force default:.. work:y", "..", func(c *cli.Instance) error {
|
||||
return c.MoveSecret(cmd, "default:..", "work:y", 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", "../../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, before, tt.rejected, tt.run)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// 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.
|
||||
//
|
||||
//nolint:paralleltest // newTwoVaultFs uses t.Setenv
|
||||
func TestMoveToVaultNameRenamesInCurrentVault(t *testing.T) {
|
||||
before := snapshotStateDir(t, newTwoVaultFs(t))
|
||||
fs := newFsFromSnapshot(t, before)
|
||||
|
||||
c := cli.NewCLIInstanceWithStateDir(fs, testStateDir)
|
||||
err := c.MoveSecret(&cobra.Command{}, "x", "work", false)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Expected: the state as before, with everything under the current
|
||||
// vault's secrets.d/x/ now under secrets.d/work/.
|
||||
oldDir := testStateDir + "/vaults.d/default/secrets.d/x/"
|
||||
newDir := testStateDir + "/vaults.d/default/secrets.d/work/"
|
||||
want := map[string]string{}
|
||||
|
||||
for path, content := range before {
|
||||
rest, found := strings.CutPrefix(path, oldDir)
|
||||
if found {
|
||||
path = newDir + rest
|
||||
}
|
||||
|
||||
want[path] = content
|
||||
}
|
||||
|
||||
require.Contains(t, want, newDir)
|
||||
require.Equal(t, want, snapshotStateDir(t, fs))
|
||||
}
|
||||
+39
-17
@@ -603,6 +603,11 @@ func printSecretsTable(
|
||||
func (cli *Instance) ImportSecret(
|
||||
cmd *cobra.Command, secretName, sourceFile string, force bool,
|
||||
) error {
|
||||
err := vault.ValidateSecretName(secretName)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Get current vault
|
||||
vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
|
||||
if err != nil {
|
||||
@@ -649,6 +654,11 @@ func (cli *Instance) ImportSecret(
|
||||
|
||||
// RemoveSecret removes a secret from the vault
|
||||
func (cli *Instance) RemoveSecret(cmd *cobra.Command, secretName string, _ bool) error {
|
||||
err := vault.ValidateSecretName(secretName)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Get current vault
|
||||
currentVlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
|
||||
if err != nil {
|
||||
@@ -702,13 +712,8 @@ func (cli *Instance) MoveSecret(
|
||||
srcVaultName, srcSecretName, srcQualified := ParseVaultSecretRef(source)
|
||||
destVaultName, destSecretName, destQualified := ParseVaultSecretRef(dest)
|
||||
|
||||
// If neither is qualified, this is a simple within-vault rename
|
||||
if !srcQualified && !destQualified {
|
||||
return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force)
|
||||
}
|
||||
|
||||
// Cross-vault move requires source to be qualified
|
||||
if !srcQualified {
|
||||
if !srcQualified && destQualified {
|
||||
return errCrossVaultSourceUnqualified
|
||||
}
|
||||
|
||||
@@ -716,31 +721,46 @@ func (cli *Instance) MoveSecret(
|
||||
// Format: "work:secret default" means move to vault "default"
|
||||
// Format: "work:secret default:newname" means move to vault "default"
|
||||
// with a new name
|
||||
if !destQualified {
|
||||
if srcQualified && !destQualified {
|
||||
// Check if dest is actually a vault name
|
||||
vaults, err := vault.ListVaults(cli.fs, cli.stateDir)
|
||||
if err == nil && slices.Contains(vaults, dest) {
|
||||
// dest is a vault name, use source secret name
|
||||
destVaultName = dest
|
||||
destSecretName = srcSecretName
|
||||
}
|
||||
|
||||
// If destVaultName is still empty, dest is a secret name in source vault
|
||||
if destVaultName == "" {
|
||||
} else {
|
||||
// dest is a secret name in source vault
|
||||
destVaultName = srcVaultName
|
||||
destSecretName = dest
|
||||
}
|
||||
}
|
||||
|
||||
// If destination secret name is empty, use source secret name
|
||||
if destSecretName == "" {
|
||||
// If destination secret name is empty, use source secret name. A plain
|
||||
// rename keeps it empty, so that the check below rejects it.
|
||||
if srcQualified && destSecretName == "" {
|
||||
destSecretName = srcSecretName
|
||||
}
|
||||
|
||||
// Check both names, for every form of the move, before selecting a vault
|
||||
// below, so that a rejected move leaves the current vault as it was.
|
||||
err := vault.ValidateSecretName(srcSecretName)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
err = vault.ValidateSecretName(destSecretName)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// If neither is qualified, this is a simple within-vault rename
|
||||
if !srcQualified && !destQualified {
|
||||
return cli.moveSecretWithinVault(cmd, srcSecretName, destSecretName, force)
|
||||
}
|
||||
|
||||
// Same vault? Use simple rename if possible (optimization)
|
||||
if srcVaultName == destVaultName {
|
||||
// Select the vault and do a simple move
|
||||
err := vault.SelectVault(cli.fs, cli.stateDir, srcVaultName)
|
||||
err = vault.SelectVault(cli.fs, cli.stateDir, srcVaultName)
|
||||
if err != nil {
|
||||
return fmt.Errorf("failed to select vault '%s': %w", srcVaultName, err)
|
||||
}
|
||||
@@ -753,7 +773,8 @@ func (cli *Instance) MoveSecret(
|
||||
cmd, srcVaultName, srcSecretName, destVaultName, destSecretName, force)
|
||||
}
|
||||
|
||||
// moveSecretWithinVault handles rename within the current vault
|
||||
// moveSecretWithinVault handles rename within the current vault. Its caller,
|
||||
// MoveSecret, has already checked both secret names.
|
||||
func (cli *Instance) moveSecretWithinVault(
|
||||
cmd *cobra.Command, source, dest string, force bool,
|
||||
) error {
|
||||
@@ -808,7 +829,8 @@ func (cli *Instance) moveSecretWithinVault(
|
||||
return nil
|
||||
}
|
||||
|
||||
// moveSecretCrossVault handles moving between different vaults
|
||||
// moveSecretCrossVault handles moving between different vaults. Its caller,
|
||||
// MoveSecret, has already checked both secret names.
|
||||
func (cli *Instance) moveSecretCrossVault(
|
||||
cmd *cobra.Command,
|
||||
srcVaultName, srcSecretName,
|
||||
|
||||
@@ -112,6 +112,11 @@ func VersionCommands(cli *Instance) *cobra.Command {
|
||||
func (cli *Instance) ListVersions(cmd *cobra.Command, secretName string) error {
|
||||
secret.Debug("ListVersions called", "secret_name", secretName)
|
||||
|
||||
err := vault.ValidateSecretName(secretName)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Get current vault
|
||||
vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
|
||||
if err != nil {
|
||||
@@ -239,6 +244,11 @@ func formatVersionTime(t *time.Time) string {
|
||||
func (cli *Instance) PromoteVersion(
|
||||
cmd *cobra.Command, secretName string, version string,
|
||||
) error {
|
||||
err := vault.ValidateSecretName(secretName)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Get current vault
|
||||
vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
|
||||
if err != nil {
|
||||
@@ -282,6 +292,11 @@ func (cli *Instance) PromoteVersion(
|
||||
func (cli *Instance) RemoveVersion(
|
||||
cmd *cobra.Command, secretName string, version string,
|
||||
) error {
|
||||
err := vault.ValidateSecretName(secretName)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Get current vault
|
||||
vlt, err := vault.GetCurrentVault(cli.fs, cli.stateDir)
|
||||
if err != nil {
|
||||
|
||||
@@ -310,64 +310,6 @@ func TestPerSecretKeyFunctionality(t *testing.T) {
|
||||
})
|
||||
}
|
||||
|
||||
// For testing purposes only
|
||||
func isValidSecretName(name string) bool {
|
||||
if name == "" {
|
||||
return false
|
||||
}
|
||||
// Valid characters for secret names: letters, numbers, dash, dot, underscore, slash
|
||||
for _, char := range name {
|
||||
if (char < 'a' || char > 'z') && // lowercase letters
|
||||
(char < 'A' || char > 'Z') && // uppercase letters
|
||||
(char < '0' || char > '9') && // numbers
|
||||
char != '-' && // dash
|
||||
char != '.' && // dot
|
||||
char != '_' && // underscore
|
||||
char != '/' { // slash
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
return true
|
||||
}
|
||||
|
||||
func TestSecretNameValidation(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
valid bool
|
||||
}{
|
||||
{"valid-name", true},
|
||||
{"valid.name", true},
|
||||
{"valid_name", true},
|
||||
{"valid/path/name", true},
|
||||
{"123valid", true},
|
||||
{"", false},
|
||||
{"Valid-Upper-Name", true}, // uppercase allowed
|
||||
{"2025-11-21-ber1app1-vaultik-test-bucket-AKI", true}, // real-world uppercase key ID
|
||||
{"MixedCase/Path/Name", true}, // mixed case with path
|
||||
{"invalid name", false}, // space not allowed
|
||||
{"invalid@name", false}, // @ not allowed
|
||||
}
|
||||
|
||||
for _, test := range tests {
|
||||
t.Run(test.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
result := isValidSecretName(test.name)
|
||||
if result != test.valid {
|
||||
t.Errorf(
|
||||
"isValidSecretName(%q) = %v, want %v",
|
||||
test.name,
|
||||
result,
|
||||
test.valid,
|
||||
)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestSecretGetValueWithEnvMnemonicUsesVaultDerivationIndex(t *testing.T) {
|
||||
// This test demonstrates the bug where GetValue uses hardcoded index 0
|
||||
// instead of the vault's actual derivation index when using environment mnemonic
|
||||
|
||||
@@ -29,10 +29,11 @@ var (
|
||||
// ErrNilValueBuffer indicates a nil value buffer was supplied.
|
||||
ErrNilValueBuffer = errors.New("value buffer is nil")
|
||||
|
||||
// ErrInvalidSecretName indicates a secret name that does not match
|
||||
// the allowed pattern [a-z0-9.\-_/]+. Composed as
|
||||
// "invalid secret name '<name>': must match pattern [a-z0-9.\-_/]+",
|
||||
// or as "invalid secret name: <name>" by GetSecretObject.
|
||||
// ErrInvalidSecretName indicates a secret name that breaks the naming
|
||||
// rule: only ASCII letters, digits, '.', '-', '_' and '/'; not empty;
|
||||
// no leading '.' or '/', no trailing '/', no '//', no '..' path segment.
|
||||
// Composed by ValidateSecretName as
|
||||
// "invalid secret name '<name>': <the rule>".
|
||||
ErrInvalidSecretName = errors.New("invalid secret name")
|
||||
|
||||
// ErrSecretExists indicates the secret already exists and --force
|
||||
|
||||
+26
-12
@@ -79,6 +79,7 @@ func (v *Vault) ListSecrets() ([]string, error) {
|
||||
// - No leading or trailing slashes
|
||||
// - No double slashes
|
||||
// - No names starting with dots
|
||||
// - No ".." path segments
|
||||
func isValidSecretName(name string) bool {
|
||||
if name == "" {
|
||||
return false
|
||||
@@ -110,6 +111,22 @@ func isValidSecretName(name string) bool {
|
||||
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': only ASCII letters, digits, '.', '-', '_' and '/' are allowed, "+
|
||||
"and a name must not be empty, start with '.' or '/', end with '/', "+
|
||||
"contain '//', or have '..' as a path segment",
|
||||
ErrInvalidSecretName, name,
|
||||
)
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// AddSecret adds a secret to this vault
|
||||
func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool) error {
|
||||
if value == nil {
|
||||
@@ -124,13 +141,11 @@ func (v *Vault) AddSecret(name string, value *memguard.LockedBuffer, force bool)
|
||||
)
|
||||
|
||||
// Validate secret name
|
||||
if !isValidSecretName(name) {
|
||||
err := ValidateSecretName(name)
|
||||
if err != nil {
|
||||
secret.Debug("Invalid secret name provided", "secret_name", name)
|
||||
|
||||
return fmt.Errorf(
|
||||
"%w '%s': must match pattern [a-z0-9.\\-_/]+",
|
||||
ErrInvalidSecretName, name,
|
||||
)
|
||||
return err
|
||||
}
|
||||
|
||||
secret.Debug("Secret name validation passed", "secret_name", name)
|
||||
@@ -358,8 +373,9 @@ func (v *Vault) UnlockVault() (*age.X25519Identity, error) {
|
||||
|
||||
// GetSecretObject retrieves a Secret object with metadata loaded from this vault
|
||||
func (v *Vault) GetSecretObject(name string) (*secret.Secret, error) {
|
||||
if !isValidSecretName(name) {
|
||||
return nil, fmt.Errorf("%w: %s", ErrInvalidSecretName, name)
|
||||
err := ValidateSecretName(name)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
// First check if the secret exists by checking for the metadata file
|
||||
@@ -628,13 +644,11 @@ func (v *Vault) updatePreviousVersion(
|
||||
// version exist, and resolves an empty version to the current one.
|
||||
func (v *Vault) resolveSecretVersion(name, version string) (string, error) {
|
||||
// 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)
|
||||
|
||||
return "", fmt.Errorf(
|
||||
"%w '%s': must match pattern [a-z0-9.\\-_/]+",
|
||||
ErrInvalidSecretName, name,
|
||||
)
|
||||
return "", err
|
||||
}
|
||||
|
||||
// Get vault directory
|
||||
|
||||
Reference in New Issue
Block a user