Refuse a relative file:// storage_url and decode its escapes (closes #299)

The path after file:// was used as written. file://dest resolved
against the working directory, so a backup run from another directory
wrote elsewhere, exited 0 and left a snapshot that could not be
restored; my%20stick created a directory with that literal name.

Config load now refuses a file:// storage_url whose path is not
absolute. ParseStorageURL refuses it too and decodes percent-escapes;
a literal space still works. The rule is written in both packages
because config cannot import storage, which imports config.

A % not followed by two hex digits now fails when the storage is
opened, not at config load.

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`).

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/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 {

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/url.go

@@ -17,8 +17,10 @@ const (
 
 // Sentinel errors for storage URL parsing.
 var (
-	ErrEmptyStorageURL   = errors.New("storage URL is empty")
+	ErrEmptyStorageURL  = errors.New("storage URL is empty")
-	ErrEmptyFilePath     = errors.New("file URL path 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(
@@ -51,17 +53,8 @@ func ParseStorageURL(rawURL string) (*URL, error) {
 		return nil, ErrEmptyStorageURL
 	}
 
-	// Handle file:// URLs
 	if after, ok := strings.CutPrefix(rawURL, "file://"); ok {
-		path := after
+		return parseFileURL(after)
-		if path == "" {
-			return nil, ErrEmptyFilePath
-		}
-
-		return &URL{
-			Scheme: schemeFile,
-			Prefix: path,
-		}, nil
 	}
 
 	if strings.HasPrefix(rawURL, "s3://") {
@@ -75,6 +68,27 @@ 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.

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},

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)
 		}
 	}