From 3a748961148a25ca6ebd3cd08660e5c683f0b946 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 11 Nov 2026 00:26:47 +0000 Subject: [PATCH] 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. config set refuses such a key before writing. The key-to-field lookup moves from internal/cli to config.SettingType so both use it. Model: opus-5-5 --- README.md | 10 +- internal/cli/config.go | 51 +++------- internal/cli/config_test.go | 10 +- internal/config/config.go | 26 +++++- internal/config/keys.go | 176 +++++++++++++++++++++++++++++++++++ internal/config/keys_test.go | 115 +++++++++++++++++++++++ 6 files changed, 341 insertions(+), 47 deletions(-) create mode 100644 internal/config/keys.go create mode 100644 internal/config/keys_test.go diff --git a/README.md b/README.md index be62a24..002180c 100644 --- a/README.md +++ b/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 -`config init` writes before `storage_url` is set or one with a `${ENV:...}` -reference to a variable not set in this shell, is checked one setting at a time: -only a change after which the changed setting cannot be read, such as one giving -it the wrong type (a single value where a list belongs or a word where a number -belongs), is refused. +the file is left unchanged. A config that does not load is checked one setting +at a time. Examples are the file `config init` writes before `storage_url` is +set, and one with a `${ENV:...}` reference to a variable not set in this shell. +In such a config, an unknown key or a value of the wrong type is refused. **`snapshot create`**: Perform incremental backup of configured snapshots. diff --git a/internal/cli/config.go b/internal/cli/config.go index 96976aa..b61c8dc 100644 --- a/internal/cli/config.go +++ b/internal/cli/config.go @@ -46,6 +46,7 @@ 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") + errNoSuchSetting = errors.New("no setting has this key") errWouldNotLoad = errors.New( "vaultik would not load the config after this change, " + "so the file was not changed") @@ -443,11 +444,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 -storage_url is set or one with a ${ENV:...} reference to a variable -not set here, is checked one setting at a time: only a change after -which the changed setting cannot be read, such as one giving it the -wrong type, is refused. +config that does not load is checked one setting at a time. Examples +are the file config init writes before storage_url is set, and one +with a ${ENV:...} reference to a variable not set in this shell. In +such a config, an unknown key or a value of the wrong type is refused. Examples: vaultik config set storage_url "file:///mnt/backups" @@ -484,6 +484,11 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error { keys := strings.Split(key, ".") + _, ok := config.SettingType(keys) + if !ok { + return fmt.Errorf("%w: %s", errNoSuchSetting, key) + } + err = yamlPathSet(root, keys, value) if err != nil { return err @@ -739,41 +744,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 -// snapshots.home.exclude.0 do. +// config.Config, as s3.access_key_id and snapshots.home.exclude.0 do. func configKeyIsString(keys []string) bool { - typ := reflect.TypeFor[config.Config]() + typ, ok := config.SettingType(keys) - for _, key := range keys { - 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 + return ok && typ.Kind() == reflect.String } // yamlSetInMapping resolves (creating if needed) the value node for the diff --git a/internal/cli/config_test.go b/internal/cli/config_test.go index 7f7d390..6265cd0 100644 --- a/internal/cli/config_test.go +++ b/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", + "no setting has this key: nosuchkey"}, + {configWithoutRecipients, "snapshots.home.exlude", "/tmp", + "no setting has this key: snapshots.home.exlude"}, + {defaultConfigText(macOS), "compression_lvl", "9", + "no setting has this key: compression_lvl"}, // Index 1 of the empty list the missing key gives. {configWithoutRecipients, "age_recipients.1", testRecipient, "index out of range"}, diff --git a/internal/config/config.go b/internal/config/config.go index 12015f7..dec4eca 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -9,6 +9,7 @@ import ( "fmt" "os" "path/filepath" + "reflect" "sort" "strings" "time" @@ -286,15 +287,36 @@ 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 -// using smartconfig, and expands ~ in paths. +// refuses a key that no setting has, applies default values for optional +// 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](), "") + 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, diff --git a/internal/config/keys.go b/internal/config/keys.go new file mode 100644 index 0000000..354e43d --- /dev/null +++ b/internal/config/keys.go @@ -0,0 +1,176 @@ +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") + +// 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 false when no setting has that key. +func SettingType(keys []string) (reflect.Type, bool) { + 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, false + } + + typ = field.Type + default: + return nil, false + } + } + + return typ, true +} + +// 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. +func checkKeys(node *yaml.Node, typ reflect.Type, path string) error { + switch node.Kind { + case yaml.DocumentNode: + for _, child := range node.Content { + err := checkKeys(child, typ, path) + if err != nil { + return err + } + } + case yaml.AliasNode: + return checkKeys(node.Alias, typ, path) + 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))) + if err != nil { + return err + } + } + case yaml.MappingNode: + return checkMappingKeys(node, typ, path) + 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](), "") +} + +// checkMappingKeys is checkKeys for a map. +func checkMappingKeys(node *yaml.Node, typ reflect.Type, path string) 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) + 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) + 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 +} diff --git a/internal/config/keys_test.go b/internal/config/keys_test.go new file mode 100644 index 0000000..e74b3eb --- /dev/null +++ b/internal/config/keys_test.go @@ -0,0 +1,115 @@ +package config //nolint:testpackage // checks the unexported errUnknownKey + +import ( + "errors" + "os" + "path/filepath" + "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) + } + }) + } +} + +// 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) + } +}