Make config set create a list for an index under a missing key (closes #308)

`config set age_recipients.0 age1...` on a file without `age_recipients`
created a map keyed `0` and wrote it, after which every command failed
to parse the config. A missing key is now created as a list when the
next path element is a number, and an index that list cannot take is an
error.

`config set` now decodes its result into the config type before
writing, and on failure refuses and leaves the file unchanged. It does
not validate: settings are set one at a time, and validation would
refuse a config still being filled in.

Judgement call: a `${...}` reference set on a setting that is not text
is now refused, since it decodes as text; `config edit` can still write
one.

Model: opus-5-5

README.md

@@ -271,8 +271,14 @@ existing file. Created with mode `0600` since it will contain credentials.
 **`config set`**: Set a scalar config value by dotted YAML path
 (e.g. `vaultik config set compression_level 9`,
 `vaultik config set storage_url "file:///mnt/backups"`). Comments and
-formatting in the file are preserved; intermediate maps are created as
-needed.
+formatting in the file are preserved. Missing keys along the path are
+created: as a list when the next path element is a number (so
+`vaultik config set age_recipients.0 age1...` works on a file without
+`age_recipients`), otherwise as a map. A change that gives a setting the
+wrong type, such as a single value where a list belongs or a word where
+a number belongs, is refused and the file is left unchanged. Whether the
+config is complete and its values are in range is checked when a command
+loads it, not by `config set`, so settings can be set one at a time.
 
 **`snapshot create`**: Perform incremental backup of configured snapshots.
 * Optional snapshot names argument to create specific snapshots (default: all)
@@ -433,7 +439,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/cli/config.go

@@ -44,6 +44,8 @@ var (
 	errNeedNumericIndex = errors.New("key is a list; use a numeric index")
 	errIndexOutOfRange  = errors.New("index out of range")
 	errNotMapOrList     = errors.New("key is not a map or list")
+	errWouldNotLoad     = errors.New(
+		"the config would no longer load, so the file was not changed")
 )
 
 const defaultConfigTemplate = `# vaultik configuration
@@ -385,8 +387,11 @@ func newConfigSetCommand() *cobra.Command {
 		Use:   "set <key> <value>",
 		Short: "Set a config value by dotted path (e.g. compression_level 5)",
 		Long: `Sets a scalar config value addressed by dotted YAML path and writes
-the file back, preserving comments and formatting. Intermediate maps
-are created as needed.
+the file back, preserving comments and formatting. Missing keys along
+the path are created: as a list when the next path element is a
+number, otherwise as a map. A change that gives a setting the wrong
+type, such as a single value where a list belongs, is refused and the
+file is left unchanged.
 
 Examples:
   vaultik config set storage_url "file:///mnt/backups"
@@ -426,6 +431,14 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error {
 		return fmt.Errorf("marshaling config: %w", err)
 	}
 
+	// Refuse a result that config.Load could not decode. Load also
+	// validates the config; that is left out here because settings are set
+	// one at a time, so a config being filled in is incomplete in between.
+	err = yaml.Unmarshal(data, &config.Config{})
+	if err != nil {
+		return fmt.Errorf("%w: %w", errWouldNotLoad, err)
+	}
+
 	err = os.WriteFile(path, data, configFileMode)
 	if err != nil {
 		return fmt.Errorf("writing config file: %w", err)
@@ -553,8 +566,8 @@ func yamlPathGet(root *yaml.Node, keys []string) (*yaml.Node, error) {
 	return node, nil
 }
 
-// yamlPathSet navigates a dotted key path, creating intermediate maps as
-// needed, and sets the final key to the given scalar value. Numeric path
+// yamlPathSet navigates a dotted key path, creating missing keys as needed,
+// and sets the final key to the given scalar value. Numeric path
 // components index into sequences; an index equal to the sequence length
 // appends a new element (e.g. "age_recipients.1" on a 1-element list).
 func yamlPathSet(root *yaml.Node, keys []string, value string) error {
@@ -567,12 +580,12 @@ func yamlPathSet(root *yaml.Node, keys []string, value string) error {
 		node = node.Content[0]
 	}
 
-	for i, key := range keys {
+	for i := range keys {
 		last := i == len(keys)-1
 
 		switch node.Kind {
 		case yaml.MappingNode:
-			node = yamlSetInMapping(node, key, value, last)
+			node = yamlSetInMapping(node, keys, i, value, last)
 		case yaml.SequenceNode:
 			next, err := yamlSetInSequence(node, keys, i, value, last)
 			if err != nil {
@@ -641,14 +654,16 @@ func yamlField(typ reflect.Type, key string) (reflect.StructField, bool) {
 	return reflect.StructField{}, false
 }
 
-// yamlSetInMapping resolves (creating if needed) the value node for key
-// within a mapping node, setting it to value when it is the final path
-// element, and returns the node to descend into.
-func yamlSetInMapping(node *yaml.Node, key, value string, last bool) *yaml.Node {
+// yamlSetInMapping resolves (creating if needed) the value node for the
+// path element keys[i] within a mapping node, setting it to value when it
+// is the final path element, and returns the node to descend into.
+func yamlSetInMapping(
+	node *yaml.Node, keys []string, i int, value string, last bool,
+) *yaml.Node {
 	var valueNode *yaml.Node
 
 	for j := 0; j+1 < len(node.Content); j += 2 {
-		if node.Content[j].Value == key {
+		if node.Content[j].Value == keys[i] {
 			valueNode = node.Content[j+1]
 
 			break
@@ -656,13 +671,8 @@ func yamlSetInMapping(node *yaml.Node, key, value string, last bool) *yaml.Node
 	}
 
 	if valueNode == nil {
-		keyNode := &yaml.Node{Kind: yaml.ScalarNode, Value: key}
-
-		valueNode = &yaml.Node{Kind: yaml.MappingNode}
-		if last {
-			valueNode = &yaml.Node{Kind: yaml.ScalarNode, Value: value}
-		}
-
+		keyNode := &yaml.Node{Kind: yaml.ScalarNode, Value: keys[i]}
+		valueNode = newPathNode(keys, i, value)
 		node.Content = append(node.Content, keyNode, valueNode)
 	} else if last {
 		setScalar(valueNode, value)
@@ -690,12 +700,7 @@ func yamlSetInSequence(
 	}
 
 	if idx == len(node.Content) {
-		newNode := &yaml.Node{Kind: yaml.MappingNode}
-		if last {
-			newNode = &yaml.Node{Kind: yaml.ScalarNode, Value: value}
-		}
-
-		node.Content = append(node.Content, newNode)
+		node.Content = append(node.Content, newPathNode(keys, i, value))
 	} else if last {
 		setScalar(node.Content[idx], value)
 	}
@@ -703,6 +708,22 @@ func yamlSetInSequence(
 	return node.Content[idx], nil
 }
 
+// newPathNode returns the node to create for the missing path element
+// keys[i]: the value when it is the final path element, otherwise an empty
+// list when the next path element is a number and an empty map when not.
+func newPathNode(keys []string, i int, value string) *yaml.Node {
+	if i == len(keys)-1 {
+		return &yaml.Node{Kind: yaml.ScalarNode, Value: value}
+	}
+
+	_, err := strconv.Atoi(keys[i+1])
+	if err == nil {
+		return &yaml.Node{Kind: yaml.SequenceNode}
+	}
+
+	return &yaml.Node{Kind: yaml.MappingNode}
+}
+
 // setScalar overwrites a node in place with a plain scalar value.
 func setScalar(n *yaml.Node, value string) {
 	n.Kind = yaml.ScalarNode

internal/cli/config_test.go

@@ -58,14 +58,15 @@ func TestDefaultConfigTemplateParses(t *testing.T) {
 	}
 }
 
+// testRecipient is a valid age public key.
+const testRecipient = "age1278m9q7dp3chsh2dcy82qk27v047zywyvtxwnj4cvt0z65jw6a7q5dqhfj"
+
 // TestConfigSetRecipientOnFreshConfig follows the README quickstart: on the
 // file `config init` writes, `config set age_recipients.0` and
 // `config set storage_url` give a config that loads with that recipient.
 func TestConfigSetRecipientOnFreshConfig(t *testing.T) {
 	t.Parallel()
 
-	const recipient = "age1278m9q7dp3chsh2dcy82qk27v047zywyvtxwnj4cvt0z65jw6a7q5dqhfj"
-
 	path := filepath.Join(t.TempDir(), "config.yml")
 
 	err := os.WriteFile(path, []byte(defaultConfigTemplate), configFileMode)
@@ -75,7 +76,7 @@ func TestConfigSetRecipientOnFreshConfig(t *testing.T) {
 
 	out := ui.NewWithColor(&bytes.Buffer{}, false)
 
-	err = writeConfigSet(out, path, "age_recipients.0", recipient)
+	err = writeConfigSet(out, path, "age_recipients.0", testRecipient)
 	if err != nil {
 		t.Fatalf("config set age_recipients.0: %v", err)
 	}
@@ -90,8 +91,111 @@ func TestConfigSetRecipientOnFreshConfig(t *testing.T) {
 		t.Fatalf("config.Load: %v", err)
 	}
 
-	if len(cfg.AgeRecipients) != 1 || cfg.AgeRecipients[0] != recipient {
-		t.Errorf("age_recipients = %v, want [%s]", cfg.AgeRecipients, recipient)
+	if len(cfg.AgeRecipients) != 1 || cfg.AgeRecipients[0] != testRecipient {
+		t.Errorf("age_recipients = %v, want [%s]", cfg.AgeRecipients, testRecipient)
+	}
+}
+
+// configWithoutRecipients loads, but has no age_recipients key and no
+// snapshot named work.
+const configWithoutRecipients = `storage_url: file:///mnt/backups
+snapshots:
+  home:
+    paths:
+      - "~"
+`
+
+// TestConfigSetIndexUnderMissingKey checks that a list index under a key
+// the file does not have creates a list, and that the result loads.
+func TestConfigSetIndexUnderMissingKey(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		key   string
+		value string
+		field func(cfg *config.Config) []string
+	}{
+		{"age_recipients.0", testRecipient,
+			func(cfg *config.Config) []string { return cfg.AgeRecipients }},
+		{"snapshots.work.paths.0", "/srv/work",
+			func(cfg *config.Config) []string { return cfg.Snapshots["work"].Paths }},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.key, func(t *testing.T) {
+			t.Parallel()
+
+			path := filepath.Join(t.TempDir(), "config.yml")
+
+			err := os.WriteFile(path, []byte(configWithoutRecipients), configFileMode)
+			if err != nil {
+				t.Fatalf("write config: %v", err)
+			}
+
+			err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false),
+				path, tt.key, tt.value)
+			if err != nil {
+				t.Fatalf("config set %s: %v", tt.key, err)
+			}
+
+			cfg, err := config.Load(path)
+			if err != nil {
+				t.Fatalf("config.Load: %v", err)
+			}
+
+			got := tt.field(cfg)
+			if len(got) != 1 || got[0] != tt.value {
+				t.Errorf("%s gives %v, want [%s]", tt.key, got, tt.value)
+			}
+		})
+	}
+}
+
+// TestConfigSetKeepsFileThatWouldNotLoad checks that config set refuses a
+// change after which the config would not load, and leaves the file as it
+// was.
+func TestConfigSetKeepsFileThatWouldNotLoad(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		key   string
+		value string
+	}{
+		// Index 1 of the empty list the missing key gives.
+		{"age_recipients.1", testRecipient},
+		// A list set to a single value.
+		{"snapshots.home.paths", "/srv"},
+		{"compression_level", "fast"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.key, func(t *testing.T) {
+			t.Parallel()
+
+			path := filepath.Join(t.TempDir(), "config.yml")
+
+			err := os.WriteFile(path, []byte(configWithoutRecipients), configFileMode)
+			if err != nil {
+				t.Fatalf("write config: %v", err)
+			}
+
+			err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false),
+				path, tt.key, tt.value)
+			if err == nil {
+				t.Errorf("config set %s %s succeeded, want an error",
+					tt.key, tt.value)
+			}
+
+			data, err := os.ReadFile(path)
+			if err != nil {
+				t.Fatalf("read config: %v", err)
+			}
+
+			if string(data) != configWithoutRecipients {
+				t.Errorf("config set %s changed the file to:\n%s",
+					tt.key, data)
+			}
+		})
 	}
 }
 

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

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