Refuse credentials in a storage URL's endpoint or rclone remote name (closes #322)

storage_url refused only the outer URL's userinfo and unknown query
parameters. Userinfo inside the s3 endpoint= value, and rclone's
remote,param=value parameters in the remote name, were accepted and
then printed by info, remote info and snapshot create. Both are now
refused when the storage URL is parsed, without echoing the value: a
host name never contains "@", and an rclone remote name never contains
a comma. The rclone case has its own error pointing at rclone config,
since the existing credentials error names the s3.* keys.

Model: opus-5-5

README.md

@@ -286,7 +286,9 @@ needed.
   remove orphaned blobs from remote storage. By default keeps only the latest
   snapshot per name; use `--keep-newer-than` for a rolling window.
 * `--keep-newer-than <duration>`: With `--prune`, keep snapshots newer than
-  this duration instead of only the latest (e.g. `4w`, `30d`, `6mo`, `1y`)
+  this duration instead of only the latest (e.g. `4w`, `30d`, `6mo`, `1y`).
+  Without `--prune`, or with a value that is not a valid duration, the
+  command fails before the backup starts.
 
 **`snapshot list`**: Show every snapshot known to this host — the union
 of the local index and the backup destination store — with timestamps
@@ -431,7 +433,10 @@ object store. Credentials are read from `s3.access_key_id` and
 `s3.secret_access_key` in the config file.
 
 **Local filesystem** (`file:///path/to/backup`): Stores blobs and metadata on
-a local or mounted filesystem. Useful for testing or backing up to a NAS.
+a local or mounted filesystem. Useful for testing or backing up to a NAS. The
+path must be absolute; a relative one is refused when the config loads.
+Percent-escapes in the path are decoded, so `my%20stick` and `my stick` name
+the same directory.
 
 **Rclone** (`rclone://remote/path`): Uses rclone's 70+ supported cloud
 providers. Requires rclone to be configured separately (`rclone config`).

TODO.md

@@ -22,17 +22,6 @@ the tag exists and is exercised; what is left is merging `next` to
 
 # Completed Steps
 
-- 2026-10-13: Made a `file://` destination list through a symlinked
-  directory ([issue #296](https://git.eeqj.de/sneak/vaultik/issues/296)).
-  A `metadata/` or `blobs/` directory that was a symlink to a directory
-  elsewhere was listed as one file, so the destination store listed as
-  empty while backups and restores still worked through the link.
-  `prune` then removed the local snapshot records and deleted every blob,
-  and `remote info` and `remote nuke` found no blobs. The listings now
-  read through the link as `Get` and `Stat` do. A link whose target is
-  missing, such as one to a volume that is not mounted, or a link
-  pointing back up the tree makes them fail.
-
 - 2026-10-13: Stopped `remote info` counting a file under `blobs/` that
   is not a blob as an orphaned blob
   ([issue #295](https://git.eeqj.de/sneak/vaultik/issues/295)). A stray

internal/blob/packer.go

@@ -245,6 +245,17 @@ func (p *Packer) Flush(ctx context.Context) error {
 	return nil
 }
 
+// DiscardCurrentBlob closes and removes the temp file of the blob being
+// packed, if any, and drops that blob. A caller that stops before Flush
+// must call it, or the temp file is left behind. Thread-safe.
+func (p *Packer) DiscardCurrentBlob() {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+
+	p.cleanupTempFile()
+	p.currentBlob = nil
+}
+
 // FinalizeBlob finalizes the current blob being assembled.
 // This compresses the accumulated chunks, encrypts the result, and computes
 // the content-addressed hash. The finalized blob is either passed to the

internal/cli/entry_snapshot_create_prune_test.go

@@ -0,0 +1,108 @@
+package cli //nolint:testpackage // shares runEntry and the hermetic config helpers
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Flags of `snapshot create` used to build argument vectors below.
+const (
+	flagPrune         = "--prune"
+	flagKeepNewerThan = "--keep-newer-than"
+)
+
+// TestEntrySnapshotCreateRejectsKeepNewerThanBeforeBackup checks that a
+// --keep-newer-than the post-backup prune could not use stops `snapshot
+// create` before the backup: the command fails and the destination
+// store stays empty. It used to be read only by the prune, after the
+// snapshot was uploaded, and without --prune it was ignored.
+//
+//nolint:paralleltest // replaces os.Args, os.Stdout, os.Stderr and the xdg globals
+func TestEntrySnapshotCreateRejectsKeepNewerThanBeforeBackup(t *testing.T) {
+	for _, testCase := range []struct {
+		name string
+		args []string
+	}{
+		{
+			name: "decimal in a day count",
+			args: []string{flagPrune, flagKeepNewerThan, "1.5d"},
+		},
+		{
+			name: "unknown unit",
+			args: []string{flagPrune, flagKeepNewerThan, "4x"},
+		},
+		{
+			name: "without --prune",
+			args: []string{flagKeepNewerThan, "4w"},
+		},
+	} {
+		t.Run(testCase.name, func(t *testing.T) {
+			configPath, storeDir := writeBackupConfig(t)
+
+			code, _, _ := runEntry(t, append(
+				[]string{flagConfig, configPath, cmdSnapshot, cmdCreate},
+				testCase.args...)...)
+
+			assert.Equal(t, 1, code)
+
+			stored, err := os.ReadDir(storeDir)
+			require.NoError(t, err)
+			assert.Empty(t, stored, "nothing may be uploaded")
+		})
+	}
+}
+
+// TestEntrySnapshotCreateReportsPostBackupPruneFailure makes the
+// post-backup prune fail after a good backup, by planting a manifest
+// that cannot be decoded, and checks that the failure is reported as the
+// prune's and not as a failed snapshot creation.
+//
+//nolint:paralleltest // replaces os.Args, os.Stdout, os.Stderr and the xdg globals
+func TestEntrySnapshotCreateReportsPostBackupPruneFailure(t *testing.T) {
+	configPath, storeDir := writeBackupConfig(t)
+
+	// A remote key is 64 lowercase hex characters. The prune reads every
+	// manifest on the store and fails on one it cannot decode.
+	plantedDir := filepath.Join(storeDir, "metadata", strings.Repeat("a", 64))
+	require.NoError(t, os.MkdirAll(plantedDir, 0o750))
+	require.NoError(t, os.WriteFile(
+		filepath.Join(plantedDir, "manifest.json.zst"),
+		[]byte("not a valid manifest"), 0o600))
+
+	code, _, stderr := runEntry(t,
+		flagConfig, configPath, cmdSnapshot, cmdCreate, flagPrune)
+
+	assert.Equal(t, 1, code)
+	assert.Contains(t, stderr, "Backup completed, but post-backup prune failed")
+	assert.NotContains(t, stderr, "Snapshot creation failed")
+
+	snapshots, err := os.ReadDir(filepath.Join(storeDir, "metadata"))
+	require.NoError(t, err)
+	assert.Len(t, snapshots, 2,
+		"the new snapshot is on the store beside the planted one")
+}
+
+// writeBackupConfig builds a hermetic config with one small file in its
+// source directory and an empty destination store directory. Returns the
+// config path and the store directory.
+func writeBackupConfig(t *testing.T) (string, string) {
+	t.Helper()
+
+	configPath := writeHermeticPruneConfig(t, false)
+
+	// writeHermeticPruneConfig puts the source and store directories
+	// beside the config file.
+	dir := filepath.Dir(configPath)
+	sourceDir := filepath.Join(dir, "source")
+
+	require.NoError(t, os.Mkdir(sourceDir, 0o750))
+	require.NoError(t, os.WriteFile(
+		filepath.Join(sourceDir, "file.txt"), []byte("hello"), 0o600))
+
+	return configPath, filepath.Join(dir, "store")
+}

internal/cli/snapshot.go

@@ -71,6 +71,11 @@ otherwise from the platform config directory (~/.config/vaultik/config.yml
 on Linux), then /etc/vaultik/config.yml.`,
 		Args: cobra.ArbitraryArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {
+			err := opts.Validate()
+			if err != nil {
+				return err
+			}
+
 			// Pass snapshot names from args
 			opts.Snapshots = args
 			// --skip-errors is a global flag on the root command.
@@ -97,6 +102,13 @@ on Linux), then /etc/vaultik/config.yml.`,
 			}, func(v *vaultik.Vaultik) error {
 				return v.CreateSnapshot(opts)
 			}, func(err error) {
+				if errors.Is(err, vaultik.ErrPostBackupPruneFailed) {
+					log.Error("Post-backup prune failed", "error", err)
+					ReportErrorf("Backup completed, but %v", err)
+
+					return
+				}
+
 				log.Error("Snapshot creation failed", "error", err)
 				ReportErrorf("Snapshot creation failed: %v", err)
 			})

internal/config/config.go

@@ -61,6 +61,9 @@ var (
 	errBadS3PartSize    = errors.New("s3.part_size must be between 5MiB and 5GiB")
 	errBadStorageScheme = errors.New(
 		"storage_url must start with s3://, file://, or rclone://")
+	errFileURLNotAbsolute = errors.New(
+		"a file:// storage_url needs an absolute path, " +
+			"as in file:///path/to/backup")
 	errStorageNotConfigured = errors.New(
 		"storage not configured; set storage_url or provide s3.endpoint + " +
 			"s3.bucket + credentials")
@@ -461,11 +464,18 @@ func (c *Config) validateStorage() error {
 
 // validateStorageURL validates URL-based storage configuration. File and
 // rclone URLs need no credentials; S3 URLs require the legacy s3.*
-// credential fields.
+// credential fields. A file URL must name an absolute path: a relative one
+// would resolve against the working directory, so the destination would
+// move with it.
 func (c *Config) validateStorageURL() error {
 	switch {
 	case strings.HasPrefix(c.StorageURL, "file://"):
-		// File storage doesn't need S3 credentials
+		// storage.ParseStorageURL applies the same rule, but config
+		// cannot import storage, which imports config.
+		if !strings.HasPrefix(c.StorageURL, "file:///") {
+			return errFileURLNotAbsolute
+		}
+
 		return nil
 	case strings.HasPrefix(c.StorageURL, "rclone://"):
 		// Rclone storage uses rclone's own config

internal/config/config_test.go

@@ -337,6 +337,35 @@ func TestLoadS3PartSize(t *testing.T) {
 	})
 }
 
+// TestLoadRejectsRelativeFileURL checks that a file:// storage_url with a
+// relative path fails at load. Accepted, it would resolve against the
+// working directory, so a backup run from another directory would write
+// to a different destination and still succeed.
+func TestLoadRejectsRelativeFileURL(t *testing.T) {
+	t.Parallel()
+
+	const relativeConfig = "snapshots:\n" +
+		"  test:\n" +
+		"    paths: [/tmp/vaultik-test-source]\n" +
+		"storage_url: file://dest\n"
+
+	path := filepath.Join(t.TempDir(), "config.yml")
+
+	err := os.WriteFile(path, []byte(relativeConfig), 0o600)
+	if err != nil {
+		t.Fatalf("write config: %v", err)
+	}
+
+	_, err = Load(path)
+	if !errors.Is(err, errFileURLNotAbsolute) {
+		t.Fatalf("Load() error = %v, want errFileURLNotAbsolute", err)
+	}
+
+	if !strings.Contains(err.Error(), "absolute path") {
+		t.Errorf("error %q does not say to use an absolute path", err.Error())
+	}
+}
+
 // TestValidateAgeRecipients checks that recipients are parsed at config load
 // (a bad entry fails immediately, not mid-backup) and that no invalid entry —
 // least of all a pasted secret key — is echoed in the error. An empty list

internal/snapshot/exclude_test.go

@@ -531,3 +531,50 @@ func TestExcludePatterns_UnanchoredPatternFile(t *testing.T) {
 	require.Equal(t, 1, result.FilesScanned,
 		"Unanchored pattern for file should match anywhere")
 }
+
+func TestExcludePatterns_DoubleStarDirectoryAtEveryDepth(t *testing.T) {
+	t.Parallel()
+
+	// As in gitignore, a leading **/ also matches directly under the source dir
+	fs := setupAnchoredTestFS(t)
+
+	scanner, repos, cleanup := createTestScanner(t, fs, []string{"**/projectname"})
+	defer cleanup()
+
+	ctx := context.Background()
+	createSnapshotRecord(ctx, t, repos, "test-snapshot")
+
+	result, err := scanner.Scan(ctx, "/backup", "test-snapshot")
+	require.NoError(t, err)
+
+	// **/projectname should exclude BOTH:
+	// - /backup/projectname/file.txt
+	// - /backup/otherproject/projectname/file.txt
+	// Total files: 4, excluded: 2
+	require.Equal(t, 2, result.FilesScanned,
+		"**/ directory pattern should match at the top level and nested")
+}
+
+func TestExcludePatterns_DoubleStarFileAtEveryDepth(t *testing.T) {
+	t.Parallel()
+
+	// As in gitignore, a leading **/ also matches directly under the source dir
+	fs := setupAnchoredTestFS(t)
+
+	scanner, repos, cleanup := createTestScanner(t, fs, []string{"**/*.txt"})
+	defer cleanup()
+
+	ctx := context.Background()
+	createSnapshotRecord(ctx, t, repos, "test-snapshot")
+
+	result, err := scanner.Scan(ctx, "/backup", "test-snapshot")
+	require.NoError(t, err)
+
+	// **/*.txt should exclude ALL .txt files:
+	// - /backup/file.txt
+	// - /backup/projectname/file.txt
+	// - /backup/otherproject/projectname/file.txt
+	// Total files: 4, excluded: 3
+	require.Equal(t, 1, result.FilesScanned,
+		"**/ file pattern should match at the top level and nested")
+}

internal/snapshot/scanner.go

@@ -1295,6 +1295,11 @@ func (s *Scanner) batchAddFilesToSnapshot(
 func (s *Scanner) processPhase(
 	ctx context.Context, filesToProcess []*FileToProcess, result *ScanResult,
 ) error {
+	// On an error or cancellation the blob being packed is never
+	// finalized, so its temp file must be removed here. After a
+	// successful flush there is no such blob and this does nothing.
+	defer s.packer.DiscardCurrentBlob()
+
 	// Calculate total bytes to process
 	var totalBytes int64
 	for _, f := range filesToProcess {
@@ -2013,6 +2018,14 @@ func compileExcludePatterns(patterns []string) []compiledPattern {
 		// Remove trailing slash if present (directory indicator)
 		pattern = strings.TrimSuffix(pattern, "/")
 
+		// A leading "**/" demands a "/" before the name and so misses an
+		// entry directly under the source path. An unanchored pattern
+		// already matches at every depth, so drop the prefix: as in
+		// gitignore, "**/x" then also matches a top-level "x".
+		if !anchored {
+			pattern = strings.TrimPrefix(pattern, "**/")
+		}
+
 		// Compile the glob pattern
 		// For patterns without path separators, we need to match them as components
 		// e.g., ".git" should match ".git" anywhere in the path

internal/snapshot/skip_errors_test.go

@@ -381,3 +381,60 @@ func TestScannerCancelStopsSkipErrorsRun(t *testing.T) {
 		})
 	}
 }
+
+// TestScannerRemovesBlobTempFileWhenRunStops checks that a run stopped
+// part-way, by a file it cannot read or by a cancellation, removes the
+// temp file of the blob it was packing.
+func TestScannerRemovesBlobTempFileWhenRunStops(t *testing.T) {
+	t.Parallel()
+
+	// a.txt is backed up first and starts a blob; the run stops at b.txt.
+	const target = "/source/b.txt"
+
+	tests := []struct {
+		name  string
+		newFs func(cancel context.CancelFunc) afero.Fs
+	}{
+		{
+			name: "file cannot be read",
+			newFs: func(context.CancelFunc) afero.Fs {
+				return &readFailFs{Fs: afero.NewMemMapFs(), target: target}
+			},
+		},
+		{
+			name: "cancelled",
+			newFs: func(cancel context.CancelFunc) afero.Fs {
+				return &cancelOnOpenFs{
+					Fs: afero.NewMemMapFs(), target: target, cancel: cancel,
+				}
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+
+			fs := tt.newFs(cancel)
+			writeSkipErrorTestFile(t, fs, "/source/a.txt", "first file content")
+			writeSkipErrorTestFile(t, fs, target, "second file content")
+
+			_, err := runSkipErrorScan(ctx, t, fs, "/source", false, nil)
+			if err == nil {
+				t.Fatal("expected the scan to stop with an error, got nil")
+			}
+
+			left, err := afero.Glob(fs, filepath.Join(os.TempDir(), "vaultik-blob-*"))
+			if err != nil {
+				t.Fatalf("listing blob temp files: %v", err)
+			}
+
+			if len(left) != 0 {
+				t.Fatalf("expected no blob temp file left, got %v", left)
+			}
+		})
+	}
+}

internal/storage/file.go

@@ -120,10 +120,9 @@ func (f *FileStorer) Delete(_ context.Context, key string) error {
 	return nil
 }
 
-// List returns all keys with the given prefix, reading through symlinked
-// directories. It fails when the destination directory is missing or the
-// prefix is a symlink whose target is missing; a missing prefix under the
-// destination directory is an empty listing.
+// List returns all keys with the given prefix. It fails when the
+// destination directory is missing; a missing prefix under it is an
+// empty listing.
 func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error) {
 	var keys []string
 
@@ -134,7 +133,8 @@ func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error)
 
 	basePath := f.fullPath(prefix)
 
-	exists, err := f.prefixExists(basePath)
+	// Check if the prefix exists
+	exists, err := afero.Exists(f.fs, basePath)
 	if err != nil {
 		return nil, fmt.Errorf("checking path: %w", err)
 	}
@@ -143,7 +143,7 @@ func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error)
 		return keys, nil // Empty list for non-existent prefix
 	}
 
-	err = f.walk(basePath, func(path string, info os.FileInfo, err error) error {
+	err = afero.Walk(f.fs, basePath, func(path string, info os.FileInfo, err error) error {
 		if err != nil {
 			return err
 		}
@@ -176,9 +176,7 @@ func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error)
 }
 
 // ListStream returns a channel of ObjectInfo for large result sets. Like
-// List, it reads through symlinked directories and sends an error when
-// the destination directory is missing or the prefix is a symlink whose
-// target is missing.
+// List, it sends an error when the destination directory is missing.
 func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan ObjectInfo {
 	ch := make(chan ObjectInfo)
 
@@ -194,7 +192,8 @@ func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan Objec
 
 		basePath := f.fullPath(prefix)
 
-		exists, err := f.prefixExists(basePath)
+		// Check if the prefix exists
+		exists, err := afero.Exists(f.fs, basePath)
 		if err != nil {
 			ch <- ObjectInfo{Err: fmt.Errorf("checking path: %w", err)}
 
@@ -205,7 +204,7 @@ func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan Objec
 			return // Empty channel for non-existent prefix
 		}
 
-		_ = f.walk(basePath, func(path string, info os.FileInfo, err error) error {
+		_ = afero.Walk(f.fs, basePath, func(path string, info os.FileInfo, err error) error {
 			// Check context cancellation
 			select {
 			case <-ctx.Done():
@@ -353,40 +352,6 @@ func (f *FileStorer) writeAtomic(
 	return nil
 }
 
-// walk is afero.Walk, except that it reads through a symlinked directory
-// the way Get and Stat do, where afero.Walk reports the link as one file.
-// afero.Walk uses Lstat, which does not follow a link, whenever the
-// filesystem has one; the struct below carries only the afero.Fs methods,
-// so the walk uses Stat instead. A link back up the tree is followed until
-// the operating system refuses the path for too many levels of links, and
-// the walk then fails.
-func (f *FileStorer) walk(root string, fn filepath.WalkFunc) error {
-	return afero.Walk(struct{ afero.Fs }{f.fs}, root, fn)
-}
-
-// prefixExists is afero.Exists, except that it does not follow a symlink
-// at path. A symlinked prefix whose target is missing, such as a link to
-// a volume that is not mounted, therefore exists, and walk then fails on
-// it; with afero.Exists it would list as empty, and prune would delete
-// every blob.
-func (f *FileStorer) prefixExists(path string) (bool, error) {
-	lstater, ok := f.fs.(afero.Lstater)
-	if !ok {
-		return afero.Exists(f.fs, path)
-	}
-
-	_, _, err := lstater.LstatIfPossible(path)
-	if err == nil {
-		return true, nil
-	}
-
-	if os.IsNotExist(err) {
-		return false, nil
-	}
-
-	return false, err
-}
-
 // fullPath returns the full filesystem path for a key.
 func (f *FileStorer) fullPath(key string) string {
 	return filepath.Join(f.basePath, key)

internal/storage/file_backend_test.go

@@ -4,10 +4,7 @@ import (
 	"context"
 	"errors"
 	"io/fs"
-	"os"
 	"path/filepath"
-	"reflect"
-	"strings"
 	"testing"
 
 	"sneak.berlin/go/vaultik/internal/storage"
@@ -63,151 +60,3 @@ func TestFileStorerListMissingDestination(t *testing.T) {
 		t.Errorf("ListStream error = %v, want a not-exist error", streamErr)
 	}
 }
-
-// TestFileStorerListsThroughSymlinkedDirectory checks that List and
-// ListStream read through a symlinked metadata/ or blobs/ directory, as Get
-// and Stat do, instead of listing the link as one key. Listing the whole
-// destination meets the link part-way through the walk; listing its prefix
-// starts the walk at the link.
-func TestFileStorerListsThroughSymlinkedDirectory(t *testing.T) {
-	t.Parallel()
-
-	for _, dir := range []string{"metadata", "blobs"} {
-		t.Run(dir, func(t *testing.T) {
-			t.Parallel()
-
-			base := t.TempDir()
-			key := dir + "/aa/bb/object"
-
-			s, err := storage.NewFileStorer(base)
-			if err != nil {
-				t.Fatalf("NewFileStorer: %v", err)
-			}
-
-			err = s.Put(context.Background(), key, strings.NewReader("data"))
-			if err != nil {
-				t.Fatalf("Put: %v", err)
-			}
-
-			linkPath := filepath.Join(base, dir)
-			movedPath := filepath.Join(t.TempDir(), dir)
-
-			err = os.Rename(linkPath, movedPath)
-			if err != nil {
-				t.Fatalf("moving %s away: %v", dir, err)
-			}
-
-			err = os.Symlink(movedPath, linkPath)
-			if err != nil {
-				t.Fatalf("linking %s: %v", dir, err)
-			}
-
-			want := []string{key}
-
-			for _, prefix := range []string{"", dir + "/"} {
-				got := listSorted(t, s, prefix)
-				if !reflect.DeepEqual(got, want) {
-					t.Errorf("List(%q) = %v, want %v", prefix, got, want)
-				}
-
-				got = listStreamKeys(t, s, prefix)
-				if !reflect.DeepEqual(got, want) {
-					t.Errorf("ListStream(%q) = %v, want %v", prefix, got, want)
-				}
-			}
-		})
-	}
-}
-
-// TestFileStorerListFailsOnSymlinkWithMissingTarget checks that List and
-// ListStream fail on a symlinked metadata/ or blobs/ directory whose target
-// is missing, such as a link to a volume that is not mounted, as they do on
-// a missing destination directory, instead of listing it as empty.
-func TestFileStorerListFailsOnSymlinkWithMissingTarget(t *testing.T) {
-	t.Parallel()
-
-	ctx := context.Background()
-
-	for _, dir := range []string{"metadata", "blobs"} {
-		t.Run(dir, func(t *testing.T) {
-			t.Parallel()
-
-			base := t.TempDir()
-
-			s, err := storage.NewFileStorer(base)
-			if err != nil {
-				t.Fatalf("NewFileStorer: %v", err)
-			}
-
-			unmounted := filepath.Join(t.TempDir(), "unmounted")
-
-			err = os.Symlink(unmounted, filepath.Join(base, dir))
-			if err != nil {
-				t.Fatalf("Symlink: %v", err)
-			}
-
-			for _, prefix := range []string{"", dir + "/"} {
-				keys, err := s.List(ctx, prefix)
-				if !errors.Is(err, fs.ErrNotExist) {
-					t.Errorf("List(%q) = %v, %v; want a not-exist error",
-						prefix, keys, err)
-				}
-
-				var streamErr error
-
-				for object := range s.ListStream(ctx, prefix) {
-					if object.Err != nil {
-						streamErr = object.Err
-					}
-				}
-
-				if !errors.Is(streamErr, fs.ErrNotExist) {
-					t.Errorf("ListStream(%q) error = %v, want a not-exist error",
-						prefix, streamErr)
-				}
-			}
-		})
-	}
-}
-
-// TestFileStorerListFailsOnSymlinkLoop checks that a symlink pointing back
-// at the destination makes List and ListStream fail instead of walking it
-// forever.
-func TestFileStorerListFailsOnSymlinkLoop(t *testing.T) {
-	t.Parallel()
-
-	ctx := context.Background()
-	base := t.TempDir()
-
-	s, err := storage.NewFileStorer(base)
-	if err != nil {
-		t.Fatalf("NewFileStorer: %v", err)
-	}
-
-	err = s.Put(ctx, testBlobKey, strings.NewReader("data"))
-	if err != nil {
-		t.Fatalf("Put: %v", err)
-	}
-
-	err = os.Symlink(base, filepath.Join(base, "blobs", "loop"))
-	if err != nil {
-		t.Fatalf("Symlink: %v", err)
-	}
-
-	keys, err := s.List(ctx, "blobs/")
-	if err == nil {
-		t.Errorf("List = %d keys and no error, want an error", len(keys))
-	}
-
-	var streamErr error
-
-	for object := range s.ListStream(ctx, "blobs/") {
-		if object.Err != nil {
-			streamErr = object.Err
-		}
-	}
-
-	if streamErr == nil {
-		t.Error("ListStream sent no error, want one")
-	}
-}

internal/storage/url.go

@@ -17,8 +17,10 @@ const (
 
 // Sentinel errors for storage URL parsing.
 var (
-	ErrEmptyStorageURL   = errors.New("storage URL is empty")
-	ErrEmptyFilePath     = errors.New("file URL path is empty")
+	ErrEmptyStorageURL  = errors.New("storage URL is empty")
+	ErrEmptyFilePath    = errors.New("file URL path is empty")
+	ErrRelativeFilePath = errors.New(
+		"file URL needs an absolute path, as in file:///path/to/backup")
 	ErrMissingBucket     = errors.New("s3 URL missing bucket name")
 	ErrMissingRemote     = errors.New("rclone URL missing remote name")
 	ErrUnsupportedScheme = errors.New(
@@ -27,7 +29,10 @@ var (
 	ErrURLCredentials     = errors.New(
 		"storage URL must not carry credentials; " +
 			"set s3.access_key_id and s3.secret_access_key in the config instead")
-	ErrURLUnknownParam = errors.New("unknown query parameter in storage URL")
+	ErrURLUnknownParam    = errors.New("unknown query parameter in storage URL")
+	ErrRcloneRemoteParams = errors.New(
+		"rclone URL must not carry parameters in the remote name; " +
+			"set them on the remote with rclone config instead")
 )
 
 // URL represents a parsed storage URL.
@@ -51,17 +56,8 @@ func ParseStorageURL(rawURL string) (*URL, error) {
 		return nil, ErrEmptyStorageURL
 	}
 
-	// Handle file:// URLs
 	if after, ok := strings.CutPrefix(rawURL, "file://"); ok {
-		path := after
-		if path == "" {
-			return nil, ErrEmptyFilePath
-		}
-
-		return &URL{
-			Scheme: schemeFile,
-			Prefix: path,
-		}, nil
+		return parseFileURL(after)
 	}
 
 	if strings.HasPrefix(rawURL, "s3://") {
@@ -75,9 +71,31 @@ func ParseStorageURL(rawURL string) (*URL, error) {
 	return nil, ErrUnsupportedScheme
 }
 
+// parseFileURL parses the path that follows file://. The path must be
+// absolute: a relative one would resolve against the working directory,
+// so the destination would move with it. Percent-escapes are decoded, so
+// my%20stick names the directory "my stick"; a literal space is kept.
+func parseFileURL(rawPath string) (*URL, error) {
+	if rawPath == "" {
+		return nil, ErrEmptyFilePath
+	}
+
+	if !strings.HasPrefix(rawPath, "/") {
+		return nil, ErrRelativeFilePath
+	}
+
+	path, err := url.PathUnescape(rawPath)
+	if err != nil {
+		return nil, fmt.Errorf("invalid file URL path: %w", err)
+	}
+
+	return &URL{Scheme: schemeFile, Prefix: path}, nil
+}
+
 // parseS3URL parses an s3://bucket/prefix URL. It rejects credentials in
-// the userinfo and any query parameter other than endpoint, region and
-// ssl, so a credential-bearing URL is never stored or echoed.
+// the userinfo or in the endpoint parameter's userinfo, and any query
+// parameter other than endpoint, region and ssl, so a credential-bearing
+// URL is never stored or echoed.
 func parseS3URL(rawURL string) (*URL, error) {
 	u, err := url.Parse(rawURL)
 	if err != nil {
@@ -100,6 +118,14 @@ func parseS3URL(rawURL string) (*URL, error) {
 		return nil, err
 	}
 
+	// A host name never contains "@", so one in an endpoint, with or
+	// without a scheme, can only introduce userinfo.
+	for _, endpoint := range query["endpoint"] {
+		if strings.Contains(endpoint, "@") {
+			return nil, ErrURLCredentials
+		}
+	}
+
 	return &URL{
 		Scheme:   schemeS3,
 		Bucket:   bucket,
@@ -111,8 +137,9 @@ func parseS3URL(rawURL string) (*URL, error) {
 }
 
 // parseRcloneURL parses an rclone://remote/path URL. rclone:// takes no
-// query parameters, so credentials in the userinfo and any parameter at
-// all are rejected rather than silently ignored.
+// query parameters, so credentials in the userinfo, parameters in the
+// remote name and any query parameter at all are rejected rather than
+// silently ignored.
 func parseRcloneURL(rawURL string) (*URL, error) {
 	u, err := url.Parse(rawURL)
 	if err != nil {
@@ -128,6 +155,12 @@ func parseRcloneURL(rawURL string) (*URL, error) {
 		return nil, ErrMissingRemote
 	}
 
+	// An rclone remote name never contains a comma; one starts rclone's
+	// remote,param=value form, whose values can be credentials.
+	if strings.Contains(remote, ",") {
+		return nil, ErrRcloneRemoteParams
+	}
+
 	err = rejectUnknownParams(u.Query())
 	if err != nil {
 		return nil, err

internal/storage/url_parse_test.go

@@ -14,7 +14,10 @@ import (
 func TestParseStorageURLValid(t *testing.T) {
 	t.Parallel()
 
-	const bucket = "mybucket"
+	const (
+		bucket     = "mybucket"
+		fileScheme = "file"
+	)
 
 	cases := []struct {
 		name string
@@ -24,7 +27,17 @@ func TestParseStorageURLValid(t *testing.T) {
 		{
 			name: "file absolute path",
 			raw:  "file:///var/backups/vaultik",
-			want: &storage.URL{Scheme: "file", Prefix: "/var/backups/vaultik"},
+			want: &storage.URL{Scheme: fileScheme, Prefix: "/var/backups/vaultik"},
+		},
+		{
+			name: "file percent-escaped path is decoded",
+			raw:  "file:///Volumes/my%20stick/backup",
+			want: &storage.URL{Scheme: fileScheme, Prefix: "/Volumes/my stick/backup"},
+		},
+		{
+			name: "file path with a literal space",
+			raw:  "file:///Volumes/my stick/backup",
+			want: &storage.URL{Scheme: fileScheme, Prefix: "/Volumes/my stick/backup"},
 		},
 		{
 			name: "s3 bucket and prefix, ssl defaults on",
@@ -90,6 +103,7 @@ func TestParseStorageURLErrors(t *testing.T) {
 	}{
 		{"empty url", "", storage.ErrEmptyStorageURL},
 		{"file empty path", "file://", storage.ErrEmptyFilePath},
+		{"file relative path", "file://dest", storage.ErrRelativeFilePath},
 		{"s3 missing bucket", "s3://", storage.ErrMissingBucket},
 		{"s3 missing bucket with path", "s3:///justprefix", storage.ErrMissingBucket},
 		{"rclone missing remote", "rclone://", storage.ErrMissingRemote},
@@ -136,6 +150,19 @@ func TestParseStorageURLRejectsCredentials(t *testing.T) {
 			wantErr: storage.ErrURLCredentials,
 			secrets: []string{key, secret, "mybucket"},
 		},
+		{
+			name: "s3 userinfo in endpoint",
+			raw: "s3://mybucket?endpoint=https://" +
+				key + ":" + secret + "@minio.example.com",
+			wantErr: storage.ErrURLCredentials,
+			secrets: []string{key, secret},
+		},
+		{
+			name:    "s3 userinfo in endpoint without scheme",
+			raw:     "s3://mybucket?endpoint=" + key + ":" + secret + "@minio.example.com",
+			wantErr: storage.ErrURLCredentials,
+			secrets: []string{key, secret},
+		},
 		{
 			name:    "s3 unknown query param",
 			raw:     "s3://mybucket?access_key=" + key + "&secret=" + secret,
@@ -189,6 +216,27 @@ func TestParseStorageURLRejectsCredentials(t *testing.T) {
 	}
 }
 
+// TestParseStorageURLRejectsRcloneRemoteParams checks that an rclone
+// remote name carrying parameters (rclone's "remote,param=value" form,
+// which can hold a credential) is rejected without echoing the value.
+func TestParseStorageURLRejectsRcloneRemoteParams(t *testing.T) {
+	t.Parallel()
+
+	const secret = "topsecret"
+
+	raw := "rclone://gdrive,client_secret=" + secret + "/backups"
+
+	_, err := storage.ParseStorageURL(raw)
+	if !errors.Is(err, storage.ErrRcloneRemoteParams) {
+		t.Fatalf("ParseStorageURL(%q) error = %v, want %v",
+			raw, err, storage.ErrRcloneRemoteParams)
+	}
+
+	if strings.Contains(err.Error(), secret) {
+		t.Errorf("error message leaked %q: %v", secret, err.Error())
+	}
+}
+
 // TestParseStorageURLParseFailureHidesURL checks that when url.Parse
 // itself fails, the wrapped error carries only the inner cause, not the
 // *url.Error whose text embeds the raw (possibly credential-bearing) URL.

internal/vaultik/snapshot.go

@@ -28,8 +28,14 @@ var (
 	errNoAgeRecipients      = errors.New(
 		"creating a snapshot needs at least one public key in " +
 			"age_recipients (generate a keypair with: age-keygen)")
+	errKeepNewerThanNeedsPrune = errors.New("--keep-newer-than requires --prune")
 )
 
+// ErrPostBackupPruneFailed wraps a failure of the post-backup prune,
+// which runs only after every snapshot was created and uploaded, so the
+// caller can report it apart from a failed backup.
+var ErrPostBackupPruneFailed = errors.New("post-backup prune failed")
+
 // listRecentLimit caps how many snapshot rows are fetched from the
 // local index for listing and purge operations.
 const listRecentLimit = 10000
@@ -45,6 +51,26 @@ type SnapshotCreateOptions struct {
 	Snapshots     []string // Optional list of snapshot names to process (empty = all)
 }
 
+// Validate fails when KeepNewerThan is set without Prune or is not a
+// valid duration. Only the post-backup prune reads KeepNewerThan, after
+// the backup, so the caller checks it before starting one.
+func (opts *SnapshotCreateOptions) Validate() error {
+	if opts.KeepNewerThan == "" {
+		return nil
+	}
+
+	if !opts.Prune {
+		return errKeepNewerThanNeedsPrune
+	}
+
+	_, err := parseDuration(opts.KeepNewerThan)
+	if err != nil {
+		return fmt.Errorf("--keep-newer-than: %w", err)
+	}
+
+	return nil
+}
+
 // CreateSnapshot executes the snapshot creation operation
 func (v *Vaultik) CreateSnapshot(opts *SnapshotCreateOptions) error {
 	// config.Load accepts an empty list, since listing, verifying and
@@ -123,7 +149,7 @@ func (v *Vaultik) CreateSnapshot(opts *SnapshotCreateOptions) error {
 	if opts.Prune {
 		err = v.runPostBackupPrune(snapshotNames, opts.KeepNewerThan)
 		if err != nil {
-			return fmt.Errorf("post-backup prune: %w", err)
+			return fmt.Errorf("%w: %w", ErrPostBackupPruneFailed, err)
 		}
 	}
 

internal/vaultik/symlinked_destination_test.go

@@ -1,121 +0,0 @@
-package vaultik_test
-
-import (
-	"context"
-	"io/fs"
-	"os"
-	"path/filepath"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/vaultik/internal/log"
-	"sneak.berlin/go/vaultik/internal/vaultik"
-)
-
-// TestPruneDeletesNothingThroughSymlinkedDirectory checks that prune keeps
-// every blob and the local snapshot record when the metadata/ or blobs/
-// directory of a file:// destination is a symlink to a directory elsewhere
-// (https://git.eeqj.de/sneak/vaultik/issues/296). A backup writes through
-// such a link, so prune has to list through it too.
-func TestPruneDeletesNothingThroughSymlinkedDirectory(t *testing.T) {
-	log.Initialize(log.Config{})
-	t.Parallel()
-
-	for _, dir := range []string{"metadata", "blobs"} {
-		t.Run(dir, func(t *testing.T) {
-			t.Parallel()
-
-			ctx := context.Background()
-			storeDir := filepath.Join(t.TempDir(), "store")
-			v, repos, _ := backUpToFileDestination(ctx, t, storeDir)
-			blobFiles := blobFilesIn(t, storeDir)
-			moveBehindSymlink(t, storeDir, dir)
-
-			require.NoError(t, v.Prune(&vaultik.PruneOptions{Force: true}))
-
-			// When blobs/ is the link, these paths run through it.
-			for _, blobFile := range blobFiles {
-				assert.FileExists(t, blobFile, "prune must delete no blob")
-			}
-
-			snapshots, err := repos.Snapshots.ListRecent(ctx, listRecentTestLimit)
-			require.NoError(t, err)
-			assert.Len(t, snapshots, 1, "prune must delete no local snapshot record")
-		})
-	}
-}
-
-// TestPruneFailsOnSymlinkedDirectoryWithMissingTarget checks that prune
-// fails, deleting no blob and keeping the local snapshot record, when the
-// metadata/ or blobs/ directory of a file:// destination is a symlink
-// whose target is missing, as a link to a volume that is not mounted is.
-func TestPruneFailsOnSymlinkedDirectoryWithMissingTarget(t *testing.T) {
-	log.Initialize(log.Config{})
-	t.Parallel()
-
-	for _, dir := range []string{"metadata", "blobs"} {
-		t.Run(dir, func(t *testing.T) {
-			t.Parallel()
-
-			ctx := context.Background()
-			storeDir := filepath.Join(t.TempDir(), "store")
-			v, repos, _ := backUpToFileDestination(ctx, t, storeDir)
-			blobFiles := blobFilesIn(t, storeDir)
-			targetPath := moveBehindSymlink(t, storeDir, dir)
-
-			// Unmount the volume the link points to, and mount it again
-			// once prune has run.
-			unmountedPath := targetPath + "-unmounted"
-			require.NoError(t, os.Rename(targetPath, unmountedPath))
-
-			err := v.Prune(&vaultik.PruneOptions{Force: true})
-			require.ErrorIs(t, err, fs.ErrNotExist)
-
-			require.NoError(t, os.Rename(unmountedPath, targetPath))
-
-			// When blobs/ is the link, these paths run through it.
-			for _, blobFile := range blobFiles {
-				assert.FileExists(t, blobFile, "prune must delete no blob")
-			}
-
-			snapshots, err := repos.Snapshots.ListRecent(ctx, listRecentTestLimit)
-			require.NoError(t, err)
-			assert.Len(t, snapshots, 1, "prune must delete no local snapshot record")
-		})
-	}
-}
-
-// blobFilesIn returns the paths of the blob files under storeDir/blobs, and
-// fails the test when there are none.
-func blobFilesIn(t *testing.T, storeDir string) []string {
-	t.Helper()
-
-	var blobFiles []string
-
-	err := filepath.WalkDir(filepath.Join(storeDir, "blobs"),
-		func(path string, entry fs.DirEntry, err error) error {
-			if err == nil && !entry.IsDir() {
-				blobFiles = append(blobFiles, path)
-			}
-
-			return err
-		})
-	require.NoError(t, err)
-	require.NotEmpty(t, blobFiles)
-
-	return blobFiles
-}
-
-// moveBehindSymlink moves storeDir/dir to a directory elsewhere, puts a
-// symlink to it in its place, and returns where it went.
-func moveBehindSymlink(t *testing.T, storeDir, dir string) string {
-	t.Helper()
-
-	linkPath := filepath.Join(storeDir, dir)
-	targetPath := filepath.Join(t.TempDir(), dir)
-	require.NoError(t, os.Rename(linkPath, targetPath))
-	require.NoError(t, os.Symlink(targetPath, linkPath))
-
-	return targetPath
-}