Reject secret mv onto the same secret under another name (closes #78)
#81
@@ -139,6 +139,9 @@ matching.
|
||||
|
||||
Moves or renames a secret within the current vault.
|
||||
- Fails if the destination already exists
|
||||
- Fails if the destination is the source under another name, such as `foo`
|
||||
for `Foo` on a case-insensitive filesystem (the macOS default); there, to
|
||||
change only the case of a name, move the secret to a third name first
|
||||
- Preserves all versions and metadata
|
||||
|
||||
### Version Management
|
||||
|
||||
@@ -25,6 +25,13 @@ Bring the repo into policy compliance in one commit:
|
||||
|
||||
# Completed Steps
|
||||
|
||||
- 2026-10-04: `secret mv` rejects a move whose destination is the source
|
||||
under another name, such as `foo` for `Foo` on a case-insensitive
|
||||
filesystem (the macOS default) or a name reached through a symbolic
|
||||
link, before changing anything, with or without `--force`, within a
|
||||
vault and between vaults; before, `--force` removed the destination and
|
||||
so deleted the secret. A rename that changes only letter case works on a
|
||||
case-sensitive filesystem as before.
|
||||
- 2026-10-04: Lint runs only in docker: `script/lint` builds
|
||||
`Dockerfile.lint`, where golangci-lint is a build step rebuilt on
|
||||
every run (`--no-cache-filter`), so an unchanged tree is linted too;
|
||||
|
||||
@@ -1,9 +1,15 @@
|
||||
package cli_test
|
||||
|
||||
import (
|
||||
"os"
|
||||
"path/filepath"
|
||||
"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"
|
||||
)
|
||||
@@ -97,3 +103,127 @@ func TestMoveWithinOtherVaultKeepsCurrentVault(t *testing.T) {
|
||||
require.Contains(t, after, workSecrets+"y/")
|
||||
require.NotContains(t, after, workSecrets+"x/")
|
||||
}
|
||||
|
||||
// TestMoveOntoSameSecretUnderAnotherNameIsRejected is a regression test for
|
||||
// https://git.eeqj.de/sneak/secret/issues/78: on a case-insensitive
|
||||
// filesystem "Foo" and "foo" are one secret, and `secret mv --force Foo foo`
|
||||
// removed the destination, which was the source. Symbolic links on the real
|
||||
// filesystem give one secret two names here: in "default", "y" is a link to
|
||||
// the secret "x", and the secrets.d of "other" is a link to that of
|
||||
// "default", so other:x is default:x. Each move must be rejected and leave
|
||||
// the secret and the links as they were.
|
||||
//
|
||||
//nolint:paralleltest // t.Setenv
|
||||
func TestMoveOntoSameSecretUnderAnotherNameIsRejected(t *testing.T) {
|
||||
t.Setenv(secret.EnvMnemonic, testMnemonic)
|
||||
|
||||
const isSame = "is the same secret on this filesystem"
|
||||
|
||||
tests := []struct {
|
||||
command string
|
||||
source, dest string
|
||||
force bool
|
||||
wantErr string
|
||||
}{
|
||||
{
|
||||
"mv --force y x", "y", "x", true,
|
||||
"secret 'y' cannot be moved onto itself: 'x' " + isSame,
|
||||
},
|
||||
{
|
||||
"mv --force x y", "x", "y", true,
|
||||
"secret 'x' cannot be moved onto itself: 'y' " + isSame,
|
||||
},
|
||||
{
|
||||
"mv x y", "x", "y", false,
|
||||
"secret 'x' cannot be moved onto itself: 'y' " + isSame,
|
||||
},
|
||||
{
|
||||
"mv --force default:x other:x", "default:x", "other:x", true,
|
||||
"secret 'default:x' cannot be moved onto itself: 'other:x' " +
|
||||
isSame,
|
||||
},
|
||||
{
|
||||
"mv default:x other", "default:x", "other", false,
|
||||
"secret 'default:x' cannot be moved onto itself: 'other:x' " +
|
||||
isSame,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.command, func(t *testing.T) {
|
||||
fs := afero.NewOsFs()
|
||||
stateDir := t.TempDir()
|
||||
vaultsDir := filepath.Join(stateDir, "vaults.d")
|
||||
|
||||
// "default" is created last, so it is the current vault.
|
||||
_, err := vault.CreateVault(fs, stateDir, "other")
|
||||
require.NoError(t, err)
|
||||
|
||||
vlt, err := vault.CreateVault(fs, stateDir, "default")
|
||||
require.NoError(t, err)
|
||||
|
||||
err = vlt.AddSecret("x", memguard.NewBufferFromBytes([]byte("value")), false)
|
||||
require.NoError(t, err)
|
||||
|
||||
defaultSecrets := filepath.Join(vaultsDir, "default", "secrets.d")
|
||||
otherSecrets := filepath.Join(vaultsDir, "other", "secrets.d")
|
||||
link := filepath.Join(defaultSecrets, "y")
|
||||
|
||||
require.NoError(t, os.Symlink("x", link))
|
||||
require.NoError(t, os.Remove(otherSecrets))
|
||||
require.NoError(t, os.Symlink(defaultSecrets, otherSecrets))
|
||||
|
||||
c := cli.NewCLIInstanceWithStateDir(fs, stateDir)
|
||||
moveErr := c.MoveSecret(&cobra.Command{}, tt.source, tt.dest, tt.force)
|
||||
|
||||
value, err := vlt.GetSecret("x")
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, "value", string(value))
|
||||
|
||||
target, err := os.Readlink(link)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, "x", target)
|
||||
|
||||
target, err = os.Readlink(otherSecrets)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, defaultSecrets, target)
|
||||
|
||||
require.EqualError(t, moveErr, tt.wantErr)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestForcedCaseOnlyMoveOnCaseSensitiveFilesystem checks that where "Foo"
|
||||
// and "foo" are two secrets, `secret mv --force Foo foo` still replaces "foo"
|
||||
// with "Foo".
|
||||
func TestForcedCaseOnlyMoveOnCaseSensitiveFilesystem(t *testing.T) {
|
||||
t.Setenv(secret.EnvMnemonic, testMnemonic)
|
||||
|
||||
fs := afero.NewOsFs()
|
||||
stateDir := t.TempDir()
|
||||
|
||||
vlt, err := vault.CreateVault(fs, stateDir, "default")
|
||||
require.NoError(t, err)
|
||||
|
||||
err = vlt.AddSecret("Foo", memguard.NewBufferFromBytes([]byte("upper")), false)
|
||||
require.NoError(t, err)
|
||||
|
||||
_, err = os.Stat(filepath.Join(stateDir, "vaults.d", "default", "secrets.d", "foo"))
|
||||
if err == nil {
|
||||
t.Skip("the temporary directory is on a case-insensitive filesystem")
|
||||
}
|
||||
|
||||
err = vlt.AddSecret("foo", memguard.NewBufferFromBytes([]byte("lower")), false)
|
||||
require.NoError(t, err)
|
||||
|
||||
c := cli.NewCLIInstanceWithStateDir(fs, stateDir)
|
||||
err = c.MoveSecret(&cobra.Command{}, "Foo", "foo", true)
|
||||
require.NoError(t, err)
|
||||
|
||||
value, err := vlt.GetSecret("foo")
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, "upper", string(value))
|
||||
|
||||
_, err = vlt.GetSecret("Foo")
|
||||
require.ErrorIs(t, err, vault.ErrSecretNotFound)
|
||||
}
|
||||
|
||||
@@ -6,6 +6,7 @@ import (
|
||||
"fmt"
|
||||
"io"
|
||||
"log"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"slices"
|
||||
"strings"
|
||||
@@ -890,6 +891,18 @@ func (cli *Instance) moveSecretWithinVault(
|
||||
destEncoded := strings.ReplaceAll(dest, "/", "%")
|
||||
destDir := filepath.Join(vaultDir, "secrets.d", destEncoded)
|
||||
|
||||
// Removing a destination that is the source under another name, such as
|
||||
// "foo" for "Foo" on a case-insensitive filesystem, would delete it too.
|
||||
same, err := cli.sameDirectory(sourceDir, destDir)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
if same {
|
||||
return fmt.Errorf("secret '%s' %w: '%s' is the same secret on "+
|
||||
"this filesystem", source, errMoveOntoItself, dest)
|
||||
}
|
||||
|
||||
exists, err = afero.DirExists(cli.fs, destDir)
|
||||
if err != nil {
|
||||
return fmt.Errorf("failed to check if destination secret exists: %w", err)
|
||||
@@ -916,6 +929,31 @@ func (cli *Instance) moveSecretWithinVault(
|
||||
return nil
|
||||
}
|
||||
|
||||
// sameDirectory reports whether the existing directory dir and the path
|
||||
// other are one directory under two names, as secrets.d/Foo and
|
||||
// secrets.d/foo are on a case-insensitive filesystem, or a directory and a
|
||||
// symbolic link to it. Removing other to make room for dir would then delete
|
||||
// dir. It is false if other does not exist, and always false on the
|
||||
// in-memory filesystem, which has no such aliasing and whose files
|
||||
// os.SameFile does not compare.
|
||||
func (cli *Instance) sameDirectory(dir, other string) (bool, error) {
|
||||
dirInfo, err := cli.fs.Stat(dir)
|
||||
if err != nil {
|
||||
return false, fmt.Errorf("failed to check %s: %w", dir, err)
|
||||
}
|
||||
|
||||
otherInfo, err := cli.fs.Stat(other)
|
||||
if errors.Is(err, os.ErrNotExist) {
|
||||
return false, nil
|
||||
}
|
||||
|
||||
if err != nil {
|
||||
return false, fmt.Errorf("failed to check %s: %w", other, err)
|
||||
}
|
||||
|
||||
return os.SameFile(dirInfo, otherInfo), nil
|
||||
}
|
||||
|
||||
// moveSecretCrossVault handles moving between two different vaults. Its
|
||||
// caller, MoveSecret, has already checked both secret names and that both
|
||||
// vaults exist.
|
||||
@@ -940,6 +978,27 @@ func (cli *Instance) moveSecretCrossVault(
|
||||
srcSecretName, errSecretNotFound, srcVault.Name)
|
||||
}
|
||||
|
||||
// The source is removed after the copy, so a destination that is the
|
||||
// source under another name would be lost with it.
|
||||
destVaultDir, err := destVault.GetDirectory()
|
||||
if err != nil {
|
||||
return fmt.Errorf("failed to get destination vault directory: %w", err)
|
||||
}
|
||||
|
||||
destStorageName := strings.ReplaceAll(destSecretName, "/", "%")
|
||||
destSecretDir := filepath.Join(destVaultDir, "secrets.d", destStorageName)
|
||||
|
||||
same, err := cli.sameDirectory(srcSecretDir, destSecretDir)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
if same {
|
||||
return fmt.Errorf("secret '%s:%s' %w: '%s:%s' is the same secret on "+
|
||||
"this filesystem", srcVault.Name, srcSecretName, errMoveOntoItself,
|
||||
destVault.Name, destSecretName)
|
||||
}
|
||||
|
||||
// Unlock destination vault (will fail if neither mnemonic nor unlocker available)
|
||||
_, err = destVault.GetOrDeriveLongTermKey()
|
||||
if err != nil {
|
||||
|
||||
Reference in New Issue
Block a user