From 42bd39fa234f6a5dff88501428e3439ce5f11362 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 12 Oct 2026 23:35:31 +0000 Subject: [PATCH] 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 | 5 +++- internal/config/config.go | 14 +++++++++-- internal/config/config_test.go | 29 +++++++++++++++++++++++ internal/storage/url.go | 38 ++++++++++++++++++++---------- internal/storage/url_parse_test.go | 18 ++++++++++++-- 5 files changed, 87 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 306b27d..355322d 100644 --- a/README.md +++ b/README.md @@ -433,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`). diff --git a/internal/config/config.go b/internal/config/config.go index e8527d7..eb51263 100644 --- a/internal/config/config.go +++ b/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 diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 18d0b20..bd7abe3 100644 --- a/internal/config/config_test.go +++ b/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 diff --git a/internal/storage/url.go b/internal/storage/url.go index 8da6eca..3e41198 100644 --- a/internal/storage/url.go +++ b/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( @@ -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 - if path == "" { - return nil, ErrEmptyFilePath - } - - return &URL{ - Scheme: schemeFile, - Prefix: path, - }, nil + return parseFileURL(after) } 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. diff --git a/internal/storage/url_parse_test.go b/internal/storage/url_parse_test.go index ad0cce7..2151249 100644 --- a/internal/storage/url_parse_test.go +++ b/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},