Refuse config keys that no setting has (closes #419)

config.Parse walks the file as written and fails on the first key that
matches no setting, naming its dotted path and line, so a typo in
exclude or prune_grace_period no longer falls back silently. Keys that a
${...} reference brings in are checked once smartconfig has filled it
in, and named without a line. YAML merge keys and a top-level env
section, which smartconfig reads, still load; an anchor that contains
itself is an error, and each anchored map is checked once per type.
config set refuses such a key before writing, with the same error, and
sets a key under env like any other value. The key-to-field lookup
moves from internal/cli to config.SettingType so both use it.

Model: opus-5-5

README.md

@@ -313,12 +313,10 @@ 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. If the config loads, a change after which
 it would not, such as `vaultik config set compression_level 99`, is refused and
-the file is left unchanged. A config that does not load, such as the one
+the file is left unchanged. A config that does not load is checked one setting
-`config init` writes before `storage_url` is set or one with a `${ENV:...}`
+at a time. Examples are the file `config init` writes before `storage_url` is
-reference to a variable not set in this shell, is checked one setting at a time:
+set, and one with a `${ENV:...}` reference to a variable not set in this shell.
-only a change after which the changed setting cannot be read, such as one giving
+In such a config, an unknown key or a value of the wrong type is refused.
-it the wrong type (a single value where a list belongs or a word where a number
-belongs), is refused.
 
 **`snapshot create`**: Perform incremental backup of configured snapshots.
 

internal/cli/config.go

@@ -443,11 +443,10 @@ number, otherwise as a map.
 
 If the config loads, a change after which it would not, such as
 compression_level 99, is refused and the file is left unchanged. A
-config that does not load, such as the one config init writes before
+config that does not load is checked one setting at a time. Examples
-storage_url is set or one with a ${ENV:...} reference to a variable
+are the file config init writes before storage_url is set, and one
-not set here, is checked one setting at a time: only a change after
+with a ${ENV:...} reference to a variable not set in this shell. In
-which the changed setting cannot be read, such as one giving it the
+such a config, an unknown key or a value of the wrong type is refused.
-wrong type, is refused.
 
 Examples:
   vaultik config set storage_url "file:///mnt/backups"
@@ -484,6 +483,13 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error {
 
 	keys := strings.Split(key, ".")
 
+	if keys[0] != config.EnvSection {
+		_, err = config.SettingType(keys)
+		if err != nil {
+			return err
+		}
+	}
+
 	err = yamlPathSet(root, keys, value)
 	if err != nil {
 		return err
@@ -739,41 +745,11 @@ func yamlPathSet(root *yaml.Node, keys []string, value string) error {
 }
 
 // configKeyIsString reports whether the dotted key names a string in
-// config.Config, following the fields' yaml tags, as s3.access_key_id and
+// config.Config, as s3.access_key_id and snapshots.home.exclude.0 do.
-// snapshots.home.exclude.0 do.
 func configKeyIsString(keys []string) bool {
-	typ := reflect.TypeFor[config.Config]()
+	typ, err := config.SettingType(keys)
 
-	for _, key := range keys {
+	return err == nil && typ.Kind() == reflect.String
-		switch {
-		case typ.Kind() == reflect.Map || typ.Kind() == reflect.Slice:
-			// The key is a snapshot name or a list index.
-			typ = typ.Elem()
-		case typ.Kind() == reflect.Struct:
-			field, ok := yamlField(typ, key)
-			if !ok {
-				return false
-			}
-
-			typ = field.Type
-		default:
-			return false
-		}
-	}
-
-	return typ.Kind() == reflect.String
-}
-
-// yamlField returns the field of struct type typ whose yaml tag names key.
-func yamlField(typ reflect.Type, key string) (reflect.StructField, bool) {
-	for field := range typ.Fields() {
-		name, _, _ := strings.Cut(field.Tag.Get("yaml"), ",")
-		if name == key {
-			return field, true
-		}
-	}
-
-	return reflect.StructField{}, false
 }
 
 // yamlSetInMapping resolves (creating if needed) the value node for the

internal/cli/config_test.go

@@ -225,7 +225,7 @@ func TestConfigSetIndexUnderMissingKey(t *testing.T) {
 // change after which the config would not load, with an error giving the
 // reason, and leaves the file as it was. On the file `config init` writes,
 // which does not load until storage_url is set, and on one that cannot be
-// read here, only a value of the wrong type is refused.
+// read here, only an unknown key or a value of the wrong type is refused.
 func TestConfigSetKeepsFileThatWouldNotLoad(t *testing.T) {
 	t.Parallel()
 
@@ -237,6 +237,14 @@ func TestConfigSetKeepsFileThatWouldNotLoad(t *testing.T) {
 		value  string
 		reason string
 	}{
+		// Keys that no setting has, on a config that loads and on one
+		// that does not.
+		{configWithoutRecipients, "nosuchkey", "1",
+			`unknown config key "nosuchkey"`},
+		{configWithoutRecipients, "snapshots.home.exlude", "/tmp",
+			`unknown config key "snapshots.home.exlude"`},
+		{defaultConfigText(macOS), "compression_lvl", "9",
+			`unknown config key "compression_lvl"`},
 		// Index 1 of the empty list the missing key gives.
 		{configWithoutRecipients, "age_recipients.1", testRecipient,
 			"index out of range"},
@@ -335,6 +343,39 @@ func TestConfigSetRefusesOnlyItsOwnFailure(t *testing.T) {
 	}
 }
 
+// TestConfigSetWritesEnvSection checks that config set writes a key under
+// the top-level env section, whose keys are not settings, on a config that
+// loads.
+func TestConfigSetWritesEnvSection(t *testing.T) {
+	const name = "VAULTIK_TEST_CONFIG_SET_ENV"
+
+	// Loading the changed config sets the variable; t.Setenv restores it
+	// after the test.
+	t.Setenv(name, "")
+
+	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, "env."+name, "value")
+	if err != nil {
+		t.Fatalf("config set env.%s: %v", name, err)
+	}
+
+	data, err := os.ReadFile(path) //nolint:gosec // G304: test-controlled path
+	if err != nil {
+		t.Fatalf("read config: %v", err)
+	}
+
+	if !strings.Contains(string(data), "env:\n  "+name+": value\n") {
+		t.Errorf("config set env.%s wrote:\n%s", name, data)
+	}
+}
+
 // TestConfigSetStringLooksLikeNumber sets string settings to values that
 // YAML reads as numbers or booleans when they are unquoted, and checks that
 // config.Load returns each one unchanged.

internal/config/config.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"reflect"
 	"sort"
 	"strings"
 	"time"
@@ -286,15 +287,37 @@ func Load(path string) (*Config, error) {
 }
 
 // Parse reads YAML config data as Load does, without validating it. It
-// applies default values for optional fields, fills in ${...} references
+// refuses a key that no setting has, applies default values for optional
-// using smartconfig, and expands ~ in paths.
+// fields, fills in ${...} references using smartconfig, and expands ~ in
+// paths.
 func Parse(data []byte) (*Config, error) {
+	// smartconfig drops line numbers, so keys are checked in the data as
+	// written.
+	var root yaml.Node
+
+	err := yaml.Unmarshal(data, &root)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse config: %w", err)
+	}
+
+	err = checkKeys(&root, reflect.TypeFor[Config](), "",
+		map[*yaml.Node]bool{}, map[nodeAndType]bool{})
+	if err != nil {
+		return nil, err
+	}
+
 	// Load config using smartconfig for interpolation
 	sc, err := smartconfig.NewFromReader(bytes.NewReader(data))
 	if err != nil {
 		return nil, fmt.Errorf("failed to load config file: %w", err)
 	}
 
+	// A ${...} reference can bring in keys that are not in the file.
+	err = checkFilledKeys(sc.Data())
+	if err != nil {
+		return nil, err
+	}
+
 	cfg := &Config{
 		// Set defaults
 		BlobSizeLimit:    defaultBlobSizeLimit,

internal/config/keys.go

@@ -0,0 +1,214 @@
+package config
+
+import (
+	"errors"
+	"fmt"
+	"reflect"
+	"strconv"
+	"strings"
+
+	"gopkg.in/yaml.v3"
+)
+
+// yamlMergeTag is the tag YAML gives a merge key (<<), which adds the keys
+// of the map it names to the map it sits in.
+const yamlMergeTag = "!!merge"
+
+// EnvSection is the top-level key of a map of environment variables that
+// smartconfig sets while it loads the config. It is not a vaultik setting,
+// so its keys are not checked.
+const EnvSection = "env"
+
+var (
+	errUnknownKey           = errors.New("unknown config key")
+	errAnchorContainsItself = errors.New("anchor contains itself")
+)
+
+// nodeAndType is a node of a parsed config and the setting type it is
+// checked against.
+type nodeAndType struct {
+	node *yaml.Node
+	typ  reflect.Type
+}
+
+// SettingType returns the type of the setting that the dotted key names in
+// Config, following the fields' yaml tags, as for s3.bucket or
+// snapshots.home.exclude.0. A snapshot name or a list index may be any
+// word. It returns an unknown key error when no setting has that key.
+func SettingType(keys []string) (reflect.Type, error) {
+	typ := reflect.TypeFor[Config]()
+
+	for _, key := range keys {
+		switch {
+		case typ.Kind() == reflect.Map || typ.Kind() == reflect.Slice:
+			typ = typ.Elem()
+		case typ.Kind() == reflect.Struct:
+			field, ok := yamlField(typ, key)
+			if !ok {
+				return nil, fmt.Errorf("%w %q", errUnknownKey, strings.Join(keys, "."))
+			}
+
+			typ = field.Type
+		default:
+			return nil, fmt.Errorf("%w %q", errUnknownKey, strings.Join(keys, "."))
+		}
+	}
+
+	return typ, nil
+}
+
+// yamlField returns the field of struct type typ whose yaml tag names key.
+func yamlField(typ reflect.Type, key string) (reflect.StructField, bool) {
+	for field := range typ.Fields() {
+		name, _, _ := strings.Cut(field.Tag.Get("yaml"), ",")
+		if name == key && name != "-" {
+			return field, true
+		}
+	}
+
+	return reflect.StructField{}, false
+}
+
+// checkKeys returns an error naming the first key in node, a parsed config
+// or part of one at the dotted path, that no setting in typ has. The error
+// gives the key's line in the file, when it has one. A value of the wrong
+// type is left for the YAML decoder to report. following holds the aliases
+// the walk is inside. Meeting one of them again means its anchor contains
+// itself, which is an error. checked holds each anchored node the walk has
+// checked completely, with the type it was checked against. An alias to
+// that node is not followed again for that type.
+func checkKeys(node *yaml.Node, typ reflect.Type, path string,
+	following map[*yaml.Node]bool, checked map[nodeAndType]bool,
+) error {
+	switch node.Kind {
+	case yaml.DocumentNode:
+		for _, child := range node.Content {
+			err := checkKeys(child, typ, path, following, checked)
+			if err != nil {
+				return err
+			}
+		}
+	case yaml.AliasNode:
+		if following[node] {
+			return fmt.Errorf("%w: %q on line %d",
+				errAnchorContainsItself, node.Value, node.Line)
+		}
+
+		target := nodeAndType{node.Alias, typ}
+		if checked[target] {
+			return nil
+		}
+
+		following[node] = true
+		defer delete(following, node)
+
+		err := checkKeys(node.Alias, typ, path, following, checked)
+		if err != nil {
+			return err
+		}
+
+		checked[target] = true
+	case yaml.SequenceNode:
+		if typ.Kind() != reflect.Slice {
+			return nil
+		}
+
+		for i, child := range node.Content {
+			err := checkKeys(child, typ.Elem(), joinKey(path, strconv.Itoa(i)),
+				following, checked)
+			if err != nil {
+				return err
+			}
+		}
+	case yaml.MappingNode:
+		return checkMappingKeys(node, typ, path, following, checked)
+	case yaml.ScalarNode:
+		// A scalar has no keys.
+	}
+
+	return nil
+}
+
+// checkFilledKeys is checkKeys for data, the config after smartconfig has
+// filled in its ${...} references. A key that a reference brings in is not
+// in the file, and the copy that Encode makes has no line numbers.
+func checkFilledKeys(data map[string]any) error {
+	var filled yaml.Node
+
+	err := filled.Encode(data)
+	if err != nil {
+		return fmt.Errorf("failed to marshal config data: %w", err)
+	}
+
+	return checkKeys(&filled, reflect.TypeFor[Config](), "",
+		map[*yaml.Node]bool{}, map[nodeAndType]bool{})
+}
+
+// checkMappingKeys is checkKeys for a map.
+func checkMappingKeys(node *yaml.Node, typ reflect.Type, path string,
+	following map[*yaml.Node]bool, checked map[nodeAndType]bool,
+) error {
+	for i := 0; i+1 < len(node.Content); i += 2 {
+		key, value := node.Content[i], node.Content[i+1]
+
+		if key.Tag == yamlMergeTag {
+			// The value is one map or a list of maps.
+			merged := []*yaml.Node{value}
+			if value.Kind == yaml.SequenceNode {
+				merged = value.Content
+			}
+
+			for _, m := range merged {
+				err := checkKeys(m, typ, path, following, checked)
+				if err != nil {
+					return err
+				}
+			}
+
+			continue
+		}
+
+		keyPath := joinKey(path, key.Value)
+		if keyPath == EnvSection {
+			continue
+		}
+
+		var valueType reflect.Type
+
+		switch {
+		case typ.Kind() == reflect.Map:
+			valueType = typ.Elem()
+		case typ.Kind() == reflect.Struct:
+			field, ok := yamlField(typ, key.Value)
+			if !ok && key.Line == 0 {
+				// A key that a ${...} reference brought in has no line.
+				return fmt.Errorf("%w %q", errUnknownKey, keyPath)
+			}
+
+			if !ok {
+				return fmt.Errorf("%w %q on line %d",
+					errUnknownKey, keyPath, key.Line)
+			}
+
+			valueType = field.Type
+		default:
+			return nil
+		}
+
+		err := checkKeys(value, valueType, keyPath, following, checked)
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+// joinKey appends key to the dotted path.
+func joinKey(path, key string) string {
+	if path == "" {
+		return key
+	}
+
+	return path + "." + key
+}

internal/config/keys_test.go

@@ -0,0 +1,163 @@
+package config //nolint:testpackage // checks the unexported errUnknownKey
+
+import (
+	"errors"
+	"fmt"
+	"os"
+	"path/filepath"
+	"slices"
+	"strconv"
+	"strings"
+	"testing"
+)
+
+// keysTestConfig loads; each case below adds one line to it.
+const keysTestConfig = "storage_url: file:///tmp/vaultik-test-storage\n" +
+	"s3:\n" +
+	"  region: us-east-1\n" +
+	"snapshots:\n" +
+	"  test:\n" +
+	"    paths: [/tmp/vaultik-test-source]\n"
+
+// TestParseRefusesUnknownKey checks that a key no setting has, at any
+// depth, fails the parse with an error naming the key and its line. A key
+// that a ${...} reference brings in is not in the file, so its error gives
+// no line.
+func TestParseRefusesUnknownKey(t *testing.T) {
+	t.Parallel()
+
+	snapshotPath := filepath.Join(t.TempDir(), "snapshot.yml")
+
+	err := os.WriteFile(snapshotPath, []byte("other:\n"+
+		"  paths: [/tmp/vaultik-test-source]\n"+
+		"  exlude: [/tmp/cache]\n"), 0o600)
+	if err != nil {
+		t.Fatalf("write snapshot: %v", err)
+	}
+
+	tests := []struct {
+		name string
+		yaml string
+		want string
+	}{
+		{"top level", keysTestConfig + "compression_lvl: 9\n",
+			`unknown config key "compression_lvl" on line 7`},
+		{"in a snapshot", keysTestConfig + "    exlude: [/tmp/cache]\n",
+			`unknown config key "snapshots.test.exlude" on line 7`},
+		{"in s3", strings.Replace(keysTestConfig, "  region:",
+			"  bukcet: b\n  region:", 1),
+			`unknown config key "s3.bukcet" on line 3`},
+		{"from a reference",
+			keysTestConfig + "  other: ${YAML:" + snapshotPath + ":other}\n",
+			`unknown config key "snapshots.other.exlude"`},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			_, err := Parse([]byte(tt.yaml))
+			if !errors.Is(err, errUnknownKey) {
+				t.Fatalf("Parse() error = %v, want errUnknownKey", err)
+			}
+
+			if err.Error() != tt.want {
+				t.Errorf("Parse() error = %q, want %q", err.Error(), tt.want)
+			}
+		})
+	}
+}
+
+// TestParseRefusesAnchorThatContainsItself checks that a YAML merge key (<<)
+// naming the anchor of the map it sits in fails the parse with an error.
+func TestParseRefusesAnchorThatContainsItself(t *testing.T) {
+	t.Parallel()
+
+	data := strings.Replace(keysTestConfig, "  test:", "  test: &test", 1) +
+		"    <<: *test\n"
+
+	_, err := Parse([]byte(data))
+	if !errors.Is(err, errAnchorContainsItself) {
+		t.Fatalf("Parse() error = %v, want errAnchorContainsItself", err)
+	}
+
+	want := `anchor contains itself: "test" on line 7`
+	if err.Error() != want {
+		t.Errorf("Parse() error = %q, want %q", err.Error(), want)
+	}
+}
+
+// TestParseRefusesExcessiveAliasing checks that Parse returns an error for
+// a file whose merge keys (<<) fan out. Each of eight snapshots merges the
+// one before it ten times. A key check that followed every alias would walk
+// the first snapshot over 10^8 times and run past the package's test
+// timeout.
+func TestParseRefusesExcessiveAliasing(t *testing.T) {
+	t.Parallel()
+
+	const snapshots, merges = 8, 10
+
+	var data strings.Builder
+
+	data.WriteString(strings.Replace(keysTestConfig, "  test:", "  test: &s0", 1))
+
+	for i := 1; i <= snapshots; i++ {
+		aliases := slices.Repeat([]string{"*s" + strconv.Itoa(i-1)}, merges)
+		fmt.Fprintf(&data, "  s%d: &s%d\n    <<: [%s]\n",
+			i, i, strings.Join(aliases, ", "))
+	}
+
+	_, err := Parse([]byte(data.String()))
+	if err == nil {
+		t.Fatal("Parse() returned no error")
+	}
+}
+
+// TestParseAcceptsKnownKeys checks that a config with a ${...} reference
+// and a YAML merge key (<<) still parses.
+func TestParseAcceptsKnownKeys(t *testing.T) {
+	t.Parallel()
+
+	levelPath := filepath.Join(t.TempDir(), "level")
+
+	err := os.WriteFile(levelPath, []byte("9"), 0o600)
+	if err != nil {
+		t.Fatalf("write level: %v", err)
+	}
+
+	data := strings.Replace(keysTestConfig, "  test:", "  test: &test", 1) +
+		"  other:\n" +
+		"    <<: *test\n" +
+		"    exclude: [/tmp/vaultik-test-source/cache]\n" +
+		"compression_level: ${FILE:" + levelPath + "}\n"
+
+	cfg, err := Parse([]byte(data))
+	if err != nil {
+		t.Fatalf("Parse() unexpected error: %v", err)
+	}
+
+	if cfg.CompressionLevel != 9 {
+		t.Errorf("compression_level = %d, want 9", cfg.CompressionLevel)
+	}
+
+	other := cfg.Snapshots["other"]
+	if len(other.Paths) != 1 || len(other.Exclude) != 1 {
+		t.Errorf("snapshot other = %+v, want the paths of test and one exclude",
+			other)
+	}
+}
+
+// TestParseAcceptsEnvSection checks that a config with a top-level env
+// section still parses. smartconfig sets the environment variables it
+// lists, so its keys are not settings.
+func TestParseAcceptsEnvSection(t *testing.T) {
+	const name = "VAULTIK_TEST_ENV_SECTION"
+
+	// Parse sets the variable; t.Setenv restores it after the test.
+	t.Setenv(name, "")
+
+	_, err := Parse([]byte(keysTestConfig + "env:\n  " + name + ": set\n"))
+	if err != nil {
+		t.Fatalf("Parse() unexpected error: %v", err)
+	}
+}