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.
|
Moves or renames a secret within the current vault.
|
||||||
- Fails if the destination already exists
|
- 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
|
- Preserves all versions and metadata
|
||||||
|
|
||||||
### Version Management
|
### Version Management
|
||||||
|
|||||||
@@ -25,6 +25,13 @@ Bring the repo into policy compliance in one commit:
|
|||||||
|
|
||||||
# Completed Steps
|
# 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
|
- 2026-10-04: Lint runs only in docker: `script/lint` builds
|
||||||
`Dockerfile.lint`, where golangci-lint is a build step rebuilt on
|
`Dockerfile.lint`, where golangci-lint is a build step rebuilt on
|
||||||
every run (`--no-cache-filter`), so an unchanged tree is linted too;
|
every run (`--no-cache-filter`), so an unchanged tree is linted too;
|
||||||
|
|||||||
@@ -1,9 +1,15 @@
|
|||||||
package cli_test
|
package cli_test
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"os"
|
||||||
|
"path/filepath"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
"git.eeqj.de/sneak/secret/internal/cli"
|
"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/spf13/cobra"
|
||||||
"github.com/stretchr/testify/require"
|
"github.com/stretchr/testify/require"
|
||||||
)
|
)
|
||||||
@@ -97,3 +103,127 @@ func TestMoveWithinOtherVaultKeepsCurrentVault(t *testing.T) {
|
|||||||
require.Contains(t, after, workSecrets+"y/")
|
require.Contains(t, after, workSecrets+"y/")
|
||||||
require.NotContains(t, after, workSecrets+"x/")
|
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"
|
"fmt"
|
||||||
"io"
|
"io"
|
||||||
"log"
|
"log"
|
||||||
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
"slices"
|
"slices"
|
||||||
"strings"
|
"strings"
|
||||||
@@ -890,6 +891,18 @@ func (cli *Instance) moveSecretWithinVault(
|
|||||||
destEncoded := strings.ReplaceAll(dest, "/", "%")
|
destEncoded := strings.ReplaceAll(dest, "/", "%")
|
||||||
destDir := filepath.Join(vaultDir, "secrets.d", destEncoded)
|
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)
|
exists, err = afero.DirExists(cli.fs, destDir)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return fmt.Errorf("failed to check if destination secret exists: %w", err)
|
return fmt.Errorf("failed to check if destination secret exists: %w", err)
|
||||||
@@ -916,6 +929,31 @@ func (cli *Instance) moveSecretWithinVault(
|
|||||||
return nil
|
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
|
// moveSecretCrossVault handles moving between two different vaults. Its
|
||||||
// caller, MoveSecret, has already checked both secret names and that both
|
// caller, MoveSecret, has already checked both secret names and that both
|
||||||
// vaults exist.
|
// vaults exist.
|
||||||
@@ -940,6 +978,27 @@ func (cli *Instance) moveSecretCrossVault(
|
|||||||
srcSecretName, errSecretNotFound, srcVault.Name)
|
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)
|
// Unlock destination vault (will fail if neither mnemonic nor unlocker available)
|
||||||
_, err = destVault.GetOrDeriveLongTermKey()
|
_, err = destVault.GetOrDeriveLongTermKey()
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|||||||
Reference in New Issue
Block a user