Refuse a relative file:// storage_url and decode its escapes (closes #299)
check / check (push) Waiting to run
check / check (push) Waiting to run
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
This commit is contained in:
@@ -433,7 +433,10 @@ object store. Credentials are read from `s3.access_key_id` and
|
|||||||
`s3.secret_access_key` in the config file.
|
`s3.secret_access_key` in the config file.
|
||||||
|
|
||||||
**Local filesystem** (`file:///path/to/backup`): Stores blobs and metadata on
|
**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
|
**Rclone** (`rclone://remote/path`): Uses rclone's 70+ supported cloud
|
||||||
providers. Requires rclone to be configured separately (`rclone config`).
|
providers. Requires rclone to be configured separately (`rclone config`).
|
||||||
|
|||||||
@@ -61,6 +61,9 @@ var (
|
|||||||
errBadS3PartSize = errors.New("s3.part_size must be between 5MiB and 5GiB")
|
errBadS3PartSize = errors.New("s3.part_size must be between 5MiB and 5GiB")
|
||||||
errBadStorageScheme = errors.New(
|
errBadStorageScheme = errors.New(
|
||||||
"storage_url must start with s3://, file://, or rclone://")
|
"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(
|
errStorageNotConfigured = errors.New(
|
||||||
"storage not configured; set storage_url or provide s3.endpoint + " +
|
"storage not configured; set storage_url or provide s3.endpoint + " +
|
||||||
"s3.bucket + credentials")
|
"s3.bucket + credentials")
|
||||||
@@ -461,11 +464,18 @@ func (c *Config) validateStorage() error {
|
|||||||
|
|
||||||
// validateStorageURL validates URL-based storage configuration. File and
|
// validateStorageURL validates URL-based storage configuration. File and
|
||||||
// rclone URLs need no credentials; S3 URLs require the legacy s3.*
|
// 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 {
|
func (c *Config) validateStorageURL() error {
|
||||||
switch {
|
switch {
|
||||||
case strings.HasPrefix(c.StorageURL, "file://"):
|
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
|
return nil
|
||||||
case strings.HasPrefix(c.StorageURL, "rclone://"):
|
case strings.HasPrefix(c.StorageURL, "rclone://"):
|
||||||
// Rclone storage uses rclone's own config
|
// Rclone storage uses rclone's own config
|
||||||
|
|||||||
@@ -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
|
// TestValidateAgeRecipients checks that recipients are parsed at config load
|
||||||
// (a bad entry fails immediately, not mid-backup) and that no invalid entry —
|
// (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
|
// least of all a pasted secret key — is echoed in the error. An empty list
|
||||||
|
|||||||
+24
-10
@@ -19,6 +19,8 @@ const (
|
|||||||
var (
|
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")
|
ErrMissingBucket = errors.New("s3 URL missing bucket name")
|
||||||
ErrMissingRemote = errors.New("rclone URL missing remote name")
|
ErrMissingRemote = errors.New("rclone URL missing remote name")
|
||||||
ErrUnsupportedScheme = errors.New(
|
ErrUnsupportedScheme = errors.New(
|
||||||
@@ -51,17 +53,8 @@ func ParseStorageURL(rawURL string) (*URL, error) {
|
|||||||
return nil, ErrEmptyStorageURL
|
return nil, ErrEmptyStorageURL
|
||||||
}
|
}
|
||||||
|
|
||||||
// Handle file:// URLs
|
|
||||||
if after, ok := strings.CutPrefix(rawURL, "file://"); ok {
|
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://") {
|
if strings.HasPrefix(rawURL, "s3://") {
|
||||||
@@ -75,6 +68,27 @@ func ParseStorageURL(rawURL string) (*URL, error) {
|
|||||||
return nil, ErrUnsupportedScheme
|
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
|
// parseS3URL parses an s3://bucket/prefix URL. It rejects credentials in
|
||||||
// the userinfo and any query parameter other than endpoint, region and
|
// the userinfo and any query parameter other than endpoint, region and
|
||||||
// ssl, so a credential-bearing URL is never stored or echoed.
|
// ssl, so a credential-bearing URL is never stored or echoed.
|
||||||
|
|||||||
@@ -14,7 +14,10 @@ import (
|
|||||||
func TestParseStorageURLValid(t *testing.T) {
|
func TestParseStorageURLValid(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
const bucket = "mybucket"
|
const (
|
||||||
|
bucket = "mybucket"
|
||||||
|
fileScheme = "file"
|
||||||
|
)
|
||||||
|
|
||||||
cases := []struct {
|
cases := []struct {
|
||||||
name string
|
name string
|
||||||
@@ -24,7 +27,17 @@ func TestParseStorageURLValid(t *testing.T) {
|
|||||||
{
|
{
|
||||||
name: "file absolute path",
|
name: "file absolute path",
|
||||||
raw: "file:///var/backups/vaultik",
|
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",
|
name: "s3 bucket and prefix, ssl defaults on",
|
||||||
@@ -90,6 +103,7 @@ func TestParseStorageURLErrors(t *testing.T) {
|
|||||||
}{
|
}{
|
||||||
{"empty url", "", storage.ErrEmptyStorageURL},
|
{"empty url", "", storage.ErrEmptyStorageURL},
|
||||||
{"file empty path", "file://", storage.ErrEmptyFilePath},
|
{"file empty path", "file://", storage.ErrEmptyFilePath},
|
||||||
|
{"file relative path", "file://dest", storage.ErrRelativeFilePath},
|
||||||
{"s3 missing bucket", "s3://", storage.ErrMissingBucket},
|
{"s3 missing bucket", "s3://", storage.ErrMissingBucket},
|
||||||
{"s3 missing bucket with path", "s3:///justprefix", storage.ErrMissingBucket},
|
{"s3 missing bucket with path", "s3:///justprefix", storage.ErrMissingBucket},
|
||||||
{"rclone missing remote", "rclone://", storage.ErrMissingRemote},
|
{"rclone missing remote", "rclone://", storage.ErrMissingRemote},
|
||||||
|
|||||||
Reference in New Issue
Block a user