From 9b4d98672d7d24e1d5608824ae9aea9a34e0b9b8a339bdded0e334a8ce2c7f54 Mon Sep 17 00:00:00 2001 From: Lixen Wraith Date: Sat, 3 Oct 2026 22:08:43 -0400 Subject: [PATCH] v0.2.2 fixes: split only text sources, report a mode change once, don't block on open --- README.md | 13 +++-- builder.go | 4 +- config.go | 3 ++ decode.go | 52 +++++++++++++++----- doc/access.md | 5 +- doc/cli.md | 18 ++++--- doc/env.md | 6 ++- doc/file.md | 12 +++-- doc/reconfiguration.md | 8 +-- file.go | 9 +++- file_unix_test.go | 30 +++++++++++ fuzz_test.go | 61 +++++++++++++++++++++++ hardening_test.go | 101 +++++++++++++++++++++++++++++++++++++- loader.go | 48 +++++++++++++----- loader_test.go | 13 ++--- ownership_other.go | 2 + ownership_unix.go | 2 + watch.go | 4 ++ watcher_hardening_test.go | 33 +++++++++++++ 19 files changed, 365 insertions(+), 59 deletions(-) create mode 100644 file_unix_test.go create mode 100644 fuzz_test.go diff --git a/README.md b/README.md index 3ac5e6c..f846242 100644 --- a/README.md +++ b/README.md @@ -62,9 +62,12 @@ mutate that caller-owned object. Float64-to-float32 rounding within range remains supported. Full uint64 is supported in memory; saving a value above MaxInt64 fails without changing the destination file. Use an application-level string representation if needed. -- Strings can represent decimal numbers, booleans, comma-separated slices/arrays, - durations, RFC3339 timestamps, IP addresses, CIDR networks and URLs. Boolean to - string conversion uses `true`/`false`. There are no user decode hooks or automatic +- Strings can represent decimal numbers, booleans, durations, RFC3339 + timestamps, IP addresses, CIDR networks and URLs. For a slice or array path, + environment and command-line text splits at commas; a string from a file, + `Set` or `ScanMap` is one element, since those have native arrays and a pattern + may hold a comma. Boolean to string conversion uses `true`/`false`. + Conversion errors name the path, never the value. There are no user decode hooks or automatic `encoding.TextUnmarshaler` calls. - Setters and source loads reject values incompatible with registered defaults immediately. Check their errors. Untyped nil defaults leave the value type open; @@ -94,8 +97,8 @@ for event := range changes { ``` The watcher fingerprints file contents, waits for a quiet debounce interval, then -validates and publishes. Invalid edits, timeouts, deletion and blocked permission -changes retain the last valid state. Cancelled, expired or superseded reloads +validates and publishes. Invalid edits, timeouts, deletion and a permission +change, reported once before the next edit applies, retain the last valid state. Cancelled, expired or superseded reloads cannot publish. Stop closes subscriptions synchronously; a new Watch call can restart watching. Notifications are buffered and best effort. diff --git a/builder.go b/builder.go index 7e14441..5e10d7d 100644 --- a/builder.go +++ b/builder.go @@ -96,8 +96,8 @@ func (b *Builder) Build() (*Config, error) { // 2. Load configuration loadErr := b.cfg.loadWithOptions(b.file, b.args, b.opts) if loadErr != nil && !errors.Is(loadErr, ErrConfigNotFound) { - // Return on fatal load errors. ErrConfigNotFound is not fatal. - return nil, wrapError(ErrFileAccess, loadErr) + // Return on fatal load errors, labelled by source. ErrConfigNotFound is not fatal. + return nil, loadErr } // 3. Run non-typed validators diff --git a/config.go b/config.go index fcdb888..261c19c 100644 --- a/config.go +++ b/config.go @@ -259,6 +259,9 @@ func (c *Config) setSourceLocked(source Source, path string, value any) error { if !validSource(source) { return wrapError(ErrTypeMismatch, fmt.Errorf("invalid source %q", source)) } + if source == SourceEnv || source == SourceCLI { + value = textList(item.defaultValue, value) + } if err := validateValue(item, value); err != nil { return err } diff --git a/decode.go b/decode.go index b898e8b..46d32da 100644 --- a/decode.go +++ b/decode.go @@ -1,6 +1,7 @@ package config import ( + "errors" "fmt" "net" "net/url" @@ -120,14 +121,14 @@ func decodeInto(data any, dst reflect.Value, depth int) error { case durationType: v, err := time.ParseDuration(s) if err != nil { - return err + return errors.New("invalid duration: want a number and a unit, as 30s") } dst.SetInt(int64(v)) return nil case timeType: v, err := time.Parse(time.RFC3339Nano, s) if err != nil { - return err + return errors.New("invalid time: want RFC 3339") } dst.Set(reflect.ValueOf(v)) return nil @@ -137,7 +138,7 @@ func decodeInto(data any, dst reflect.Value, depth int) error { } v := net.ParseIP(s) if v == nil { - return fmt.Errorf("invalid IP address %q", s) + return errors.New("invalid IP address") } dst.Set(reflect.ValueOf(v)) return nil @@ -147,7 +148,7 @@ func decodeInto(data any, dst reflect.Value, depth int) error { } _, v, err := net.ParseCIDR(s) if err != nil { - return fmt.Errorf("invalid CIDR: %w", err) + return errors.New("invalid CIDR") } dst.Set(reflect.ValueOf(*v)) return nil @@ -157,7 +158,7 @@ func decodeInto(data any, dst reflect.Value, depth int) error { } v, err := url.Parse(s) if err != nil { - return fmt.Errorf("invalid URL: %w", err) + return errors.New("invalid URL") } dst.Set(reflect.ValueOf(*v)) return nil @@ -166,35 +167,34 @@ func decodeInto(data any, dst reflect.Value, depth int) error { case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: v, err := strconv.ParseInt(s, 10, dst.Type().Bits()) if err != nil { - return err + return valueless("integer", err) } dst.SetInt(v) return nil case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: v, err := strconv.ParseUint(s, 10, dst.Type().Bits()) if err != nil { - return err + return valueless("unsigned integer", err) } dst.SetUint(v) return nil case reflect.Float32, reflect.Float64: v, err := strconv.ParseFloat(s, dst.Type().Bits()) if err != nil { - return err + return valueless("float", err) } return toml.Decode(v, dst.Addr().Interface()) case reflect.Bool: v, err := strconv.ParseBool(s) if err != nil { - return err + return valueless("bool", err) } dst.SetBool(v) return nil case reflect.Slice, reflect.Array: - parts := []string{} - if s != "" { - parts = strings.Split(s, ",") - } + // One entry: files and maps have native arrays, and a pattern or + // a DN may hold commas. Text sources split earlier (textList). + parts := []string{s} data, src = parts, reflect.ValueOf(parts) } } @@ -301,6 +301,32 @@ func decodeInto(data any, dst reflect.Value, depth int) error { return nil } +// valueless reports a failed conversion without the input, which may be a +// secret misplaced into a numeric field and would reach logs and events +func valueless(kind string, err error) error { + if ne, ok := errors.AsType[*strconv.NumError](err); ok { + return fmt.Errorf("invalid %s: %w", kind, ne.Err) + } + return fmt.Errorf("invalid %s", kind) +} + +// textList splits a string for a list path at commas: environment and +// command-line values are text, where a comma is the only list syntax +func textList(defaultValue, value any) any { + s, ok := value.(string) + t := reflect.TypeOf(defaultValue) + if !ok || t == nil || t == ipType || t.Kind() != reflect.Slice && t.Kind() != reflect.Array || t.Elem().Kind() == reflect.Uint8 { + return value + } + parts := []any{} + if s != "" { + for p := range strings.SplitSeq(s, ",") { + parts = append(parts, p) + } + } + return parts +} + func fieldKey(f reflect.StructField) string { key, _, _ := strings.Cut(f.Tag.Get(FormatTOML), ",") if key == "" { diff --git a/doc/access.md b/doc/access.md index 8e3ffef..505af04 100644 --- a/doc/access.md +++ b/doc/access.md @@ -24,8 +24,9 @@ provided by the caller. A present nil value clears its destination. Conversions support signed/unsigned numbers, floats, booleans, strings, slices, arrays, string-keyed maps, structs and pointers. Durations, time.Time, net.IP, net.IPNet and url.URL are atomic configuration values, including pointer forms. -They accept their native types or documented text representations. Comma-separated -strings can populate typed numeric slices as well as string slices. +They accept their native types or documented text representations. Environment and +command-line text splits at commas into typed numeric slices as well as string +slices; any other string is one element. No truncating integer conversions or integer-to-float precision loss is allowed. Float narrowing may round within range. NaN/Inf and unsupported object graphs are diff --git a/doc/cli.md b/doc/cli.md index e24ce22..c150664 100644 --- a/doc/cli.md +++ b/doc/cli.md @@ -1,14 +1,18 @@ # Command-line arguments `LoadCLI` accepts `--server.port=9000`, `--server.port 9000`, and bare boolean flags -such as `--debug`. Arguments after `--` are positional and are not parsed. Other -positional arguments are ignored. There is no short-flag parser; use Go's flag package -when that interface is needed. Paths must obey registration's dot-separated key syntax. +such as `--debug`. A bare flag is the boolean true, so on a path that is not +boolean (`--dir --debug`) it fails with "--dir needs a value". Arguments after `--` +are positional and are not parsed. There is no short-flag parser; use Go's flag +package when that interface is needed. Paths must obey registration's dot-separated +key syntax. -Values remain strings and are validated against registered types before publication. -Each successful call replaces the complete CLI source, including an empty argument -list. Omitted flags lose their previous overrides. Unknown paths are recorded in -`UnknownCLIKeys` in sorted order and are not registered. The returned list is a copy. +Values remain strings and are validated against registered types before publication; +a slice or array path splits them at commas. Each successful call replaces the +complete CLI source, including an empty argument list. Omitted flags lose their +previous overrides. Unknown paths are recorded in `UnknownCLIKeys` in sorted order, +followed by the arguments before `--` that are no `--` flag (a positional word, a +`-q`), and are not registered. The returned list is a copy. `GenerateFlags` creates a flag.FlagSet from defaults. `BindFlags` atomically replaces the CLI source with explicitly visited flags, rejects unregistered flags, and uses diff --git a/doc/env.md b/doc/env.md index 80cac28..f124a57 100644 --- a/doc/env.md +++ b/doc/env.md @@ -8,8 +8,10 @@ Options are copied. Custom transforms run outside Config locks and may inspect C Environment values are stored as strings and validated against registered types before any part of the replacement is published. Numbers use decimal text; durations use Go duration syntax; timestamps use RFC3339Nano-compatible text. Slices and arrays -use comma-separated elements without an escaping convention. Empty text gives an -empty slice. Values larger than MaxValueSize are rejected. +use comma-separated elements without an escaping convention. An empty variable is +unset, so a template may leave one blank. Two paths whose names map to one set +variable (`a.b_c` and `a_b.c` both to `APP_A_B_C`) fail the load. Values larger than +MaxValueSize are rejected. `RegisterWithEnv` and a struct field's `env` tag load an explicitly named variable during registration. Later `LoadEnv` calls use the configured transform/whitelist, diff --git a/doc/file.md b/doc/file.md index 728c5b6..7d397bd 100644 --- a/doc/file.md +++ b/doc/file.md @@ -9,11 +9,13 @@ removes that source override, exposing the next source or registered default. Loads parse and validate before publication. Syntax errors, incompatible values, overflow and read failures leave prior values, comments and the loaded path intact. -Readers must be regular files. Security options check ownership of the opened file -on Unix and enforce size limits both before and during reading. Ownership checks -fail explicitly on unsupported platforms. PreventPathTraversal rejects relative -paths escaping through `..`; it is not a symlink sandbox. Absolute paths remain -allowed. A zero MaxFileSize means no configured byte limit. +Readers must be regular files, checked on the opened descriptor. The open does not +block, so a FIFO swapped in after the path's check fails at once, and a file whose +reads would block fails its read. Security options check ownership of the opened +file on Unix and enforce size limits both before and during reading. Ownership +checks fail explicitly on unsupported platforms. PreventPathTraversal checks the +path text only: it rejects relative paths escaping through `..`, not symlinks or +absolute paths. A zero MaxFileSize means no configured byte limit. `Save(path)` writes effective values; `SaveSource(path, source)` writes one source. Default selects registered defaults. Both use a temporary file in the destination diff --git a/doc/reconfiguration.md b/doc/reconfiguration.md index 760fe9f..362d49f 100644 --- a/doc/reconfiguration.md +++ b/doc/reconfiguration.md @@ -21,9 +21,11 @@ timeouts for the same observation emit only one notification until recovery. Events are changed registered paths, or `file_deleted`, `permissions_changed`, `reload_timeout`, `reload_error:`, and `precedence:`. A path event means -the effective value changed; a file value masked by CLI need not notify. Group/world -permission changes are blocked when VerifyPermissions is enabled; correcting the -permissions allows loading again. Default options enable that check. +the effective value changed; a file value masked by CLI need not notify. With +VerifyPermissions (on by default), a change of the group/world permission bits is +reported once as `permissions_changed` and that version is not applied; the next +change applies under the new mode. While a watcher runs, a further Watch or +WatchWithOptions call subscribes to it and its options are ignored. Notifications are buffered (10 entries) and best effort; a slow subscriber may miss events and should read the latest full state. MaxWatchers bounds retained channels. diff --git a/file.go b/file.go index e157a7c..385223b 100644 --- a/file.go +++ b/file.go @@ -68,7 +68,7 @@ func readConfigFile(ctx context.Context, path string, opts fileSettings) (*fileS if !info.Mode().IsRegular() { return nil, wrapError(ErrFileAccess, fmt.Errorf("config must be a regular file")) } - file, err := os.Open(path) + file, err := openConfig(path) if err != nil { return nil, wrapError(ErrFileAccess, err) } @@ -113,6 +113,13 @@ func readConfigFile(ctx context.Context, path string, opts fileSettings) (*fileS return &fileSnapshot{data, after, sha256.Sum256(data)}, nil } +// openConfig opens without blocking: a FIFO swapped in after the Stat cannot +// hang open(2), and a regular-looking file whose reads block (/proc/kmsg) +// fails its read instead +func openConfig(path string) (*os.File, error) { + return os.OpenFile(path, os.O_RDONLY|openNonBlock, 0) +} + func parseFile(snapshot *fileSnapshot) (map[string]any, []string, error) { root, err := toml.NewParser(snapshot.data).Parse() if err != nil { diff --git a/file_unix_test.go b/file_unix_test.go new file mode 100644 index 0000000..49d0bc5 --- /dev/null +++ b/file_unix_test.go @@ -0,0 +1,30 @@ +//go:build unix + +package config + +import ( + "path/filepath" + "syscall" + "testing" + "time" +) + +// The open cannot block: a FIFO swapped in after the Stat fails at once +func TestOpenConfigNeverBlocks(t *testing.T) { + fifo := filepath.Join(t.TempDir(), "config.toml") + if err := syscall.Mkfifo(fifo, 0600); err != nil { + t.Skip(err) + } + done := make(chan struct{}) + go func() { + if f, err := openConfig(fifo); err == nil { + f.Close() + } + close(done) + }() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("open blocked on a FIFO without a writer") + } +} diff --git a/fuzz_test.go b/fuzz_test.go new file mode 100644 index 0000000..9cf521c --- /dev/null +++ b/fuzz_test.go @@ -0,0 +1,61 @@ +package config + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +type fuzzTarget struct { + Name string `toml:"name"` + Port int64 `toml:"port"` + Ratio float64 `toml:"ratio"` + On bool `toml:"on"` + Tags []string `toml:"tags"` + IDs []uint16 `toml:"ids"` + Meta map[string]string `toml:"meta"` + Sub struct { + Level string `toml:"level"` + } `toml:"sub"` +} + +// Arguments never panic the parser, and what a load accepts decodes +func FuzzLoadCLI(f *testing.F) { + for _, s := range []string{"--name=x", "--port\x009000", "--on\x00--tags=a,b", "-q\x00x\x00--\x00--name", "--sub.level\x00--on", "--ids=1,65536"} { + f.Add(s) + } + f.Fuzz(func(t *testing.T, s string) { + cfg, err := NewBuilder().WithTarget(&fuzzTarget{}).WithArgs(strings.Split(s, "\x00")).Build() + if err != nil { + return + } + if _, err := cfg.AsStruct(); err != nil { + t.Fatalf("accepted %q, then: %v", s, err) + } + }) +} + +// File contents never panic the loader, and what LoadFile accepts decodes +func FuzzLoadFile(f *testing.F) { + for _, s := range []string{"name = \"x\"\nport = 1", "tags = \"a,b\"\n[sub]\nlevel = \"debug\"", "ids = [1, 2]\n[meta]\nk = \"v\"", "on = true # c\r\n"} { + f.Add([]byte(s)) + } + // One file and one Config per worker: each LoadFile replaces the file source + path := filepath.Join(f.TempDir(), "c.toml") + cfg, err := NewBuilder().WithTarget(&fuzzTarget{}).Build() + if err != nil { + f.Fatal(err) + } + f.Fuzz(func(t *testing.T, data []byte) { + if err := os.WriteFile(path, data, 0600); err != nil { + t.Fatal(err) + } + if cfg.LoadFile(path) != nil { + return + } + if _, err := cfg.AsStruct(); err != nil { + t.Fatalf("accepted %q, then: %v", data, err) + } + }) +} diff --git a/hardening_test.go b/hardening_test.go index 2a74cf4..5d7fc41 100644 --- a/hardening_test.go +++ b/hardening_test.go @@ -39,7 +39,7 @@ func TestCheckedConversionsAndRollback(t *testing.T) { var array struct { Values [2]uint16 `toml:"values"` } - if err := ScanMap(map[string]any{"values": "1,65535"}, &array); err != nil || array.Values != [2]uint16{1, 65535} { + if err := ScanMap(map[string]any{"values": []any{"1", "65535"}}, &array); err != nil || array.Values != [2]uint16{1, 65535} { t.Fatalf("array: %+v %v", array, err) } } @@ -301,3 +301,102 @@ func TestRegistrationAndRequiredContracts(t *testing.T) { } } } + +// A string from a file or a map is one list entry, as a pattern or a DN may +// hold commas; environment and command-line text splits at commas +func TestOnlyTextSourcesSplitLists(t *testing.T) { + type C struct { + Patterns []string `toml:"patterns"` + Hosts []string `toml:"hosts"` + Tags []string `toml:"tags"` + } + path := filepath.Join(t.TempDir(), "c.toml") + writeTestFile(t, path, []byte(`patterns = "password=\\S{8,64}"`+"\n"), 0600) + t.Setenv("ZZ_HOSTS", "a,b") + cfg, err := NewBuilder().WithTarget(&C{}).WithEnvPrefix("ZZ_").WithArgs([]string{"--tags=x,y"}).WithFile(path).Build() + if err != nil { + t.Fatal(err) + } + v, err := cfg.AsStruct() + got := v.(*C) + if err != nil || !reflect.DeepEqual(got.Patterns, []string{`password=\S{8,64}`}) || + !reflect.DeepEqual(got.Hosts, []string{"a", "b"}) || !reflect.DeepEqual(got.Tags, []string{"x", "y"}) { + t.Fatalf("%+v %v", got, err) + } + var m C + if err := ScanMap(map[string]any{"patterns": "m,n"}, &m); err != nil || !reflect.DeepEqual(m.Patterns, []string{"m,n"}) { + t.Fatalf("ScanMap: %+v %v", m, err) + } +} + +// A failed conversion names the field, never its value: a secret misplaced +// into a numeric field would reach logs and watch events +func TestConversionErrorsOmitValues(t *testing.T) { + type C struct { + Port int64 `toml:"port"` + Ratio float64 `toml:"ratio"` + On bool `toml:"on"` + Timeout time.Duration `toml:"timeout"` + } + for _, name := range []string{"ZZ_PORT", "ZZ_RATIO", "ZZ_ON", "ZZ_TIMEOUT"} { + t.Run(name, func(t *testing.T) { + t.Setenv(name, "hunter2-secret") + _, err := NewBuilder().WithTarget(&C{}).WithEnvPrefix("ZZ_").Build() + if err == nil || strings.Contains(err.Error(), "hunter2") { + t.Fatalf("err = %v", err) + } + }) + } +} + +// Arguments that are no --flag are reported, not dropped, and a bare flag +// sets only a bool: "--dir --quiet" must not make dir "true" +func TestCommandLineReportsStrayArgsAndBareFlags(t *testing.T) { + type C struct { + Dir string `toml:"dir"` + Quiet bool `toml:"quiet"` + } + cfg, err := NewBuilder().WithTarget(&C{}).WithArgs([]string{"input.log", "-q", "--quiet", "--", "after"}).Build() + if err != nil || !reflect.DeepEqual(cfg.UnknownCLIKeys(), []string{"input.log", "-q"}) { + t.Fatalf("stray: %v %v", cfg.UnknownCLIKeys(), err) + } + if _, err := NewBuilder().WithTarget(&C{}).WithArgs([]string{"--dir", "--quiet"}).Build(); err == nil || !strings.Contains(err.Error(), "--dir needs a value") { + t.Fatalf("bare flag on a string path: %v", err) + } +} + +// An empty variable is unset, and two paths cannot share one variable name +func TestEnvEmptyIsUnsetAndNamesAreUnique(t *testing.T) { + type C struct { + Port int64 `toml:"port"` + Tags []string `toml:"tags"` + } + t.Setenv("ZZ_PORT", "") + t.Setenv("ZZ_TAGS", "") + cfg, err := NewBuilder().WithTarget(&C{Port: 80, Tags: []string{"a"}}).WithEnvPrefix("ZZ_").Build() + if err != nil { + t.Fatal(err) + } + if v, _ := cfg.AsStruct(); v.(*C).Port != 80 || !reflect.DeepEqual(v.(*C).Tags, []string{"a"}) { + t.Fatalf("empty variables changed values: %+v", v) + } + c := New() + c.Register("a.b_c", "") + c.Register("a_b.c", "") + t.Setenv("ZZ_A_B_C", "x") + if err := c.LoadEnv("ZZ_"); err == nil || !strings.Contains(err.Error(), "would set both") { + t.Fatalf("colliding names: %v", err) + } +} + +// Build reports a load error under its source, not as file access +func TestBuildLabelsErrorsBySource(t *testing.T) { + type C struct { + Port int64 `toml:"port"` + } + t.Setenv("ZZ_PORT", "x") + _, err := NewBuilder().WithTarget(&C{}).WithEnvPrefix("ZZ_").Build() + if !errors.Is(err, ErrEnvParse) || errors.Is(err, ErrFileAccess) { + t.Fatalf("err = %v", err) + } +} diff --git a/loader.go b/loader.go index d831472..f680168 100644 --- a/loader.go +++ b/loader.go @@ -201,11 +201,18 @@ func (c *Config) loadEnv(opts LoadOptions) error { } c.mutex.RUnlock() values := make(map[string]any) + byVar := make(map[string]string, len(paths)) for _, path := range paths { if opts.EnvWhitelist != nil && !opts.EnvWhitelist[path] { continue } - if value, ok := os.LookupEnv(transform(path)); ok { + name := transform(path) + // Empty counts as unset, so a template may leave a variable blank + if value, ok := os.LookupEnv(name); ok && value != "" { + if other, dup := byVar[name]; dup { + return fmt.Errorf("%s would set both %s and %s", name, other, path) + } + byVar[name] = path values[path] = value } } @@ -215,13 +222,17 @@ func (c *Config) loadEnv(opts LoadOptions) error { } func (c *Config) loadCLI(args []string) error { - parsed, err := parseArgs(args) + parsed, stray, err := parseArgs(args) if err != nil { return err } c.mutex.Lock() defer c.mutex.Unlock() - return c.replaceSourceLocked(SourceCLI, flattenMap(parsed, "")) + if err := c.replaceSourceLocked(SourceCLI, flattenMap(parsed, "")); err != nil { + return err + } + c.unknownCLIKeys = append(c.unknownCLIKeys, stray...) + return nil } // Validate the entire replacement before any mutation. Caller holds c.mutex. @@ -236,6 +247,12 @@ func (c *Config) replaceSourceLocked(source Source, values map[string]any, guard } continue } + if source == SourceEnv || source == SourceCLI { + value = textList(item.defaultValue, value) + } + if t := reflect.TypeOf(item.defaultValue); source == SourceCLI && value == true && t != nil && t.Kind() != reflect.Bool { + return fmt.Errorf("--%s needs a value", path) + } if err := validateValue(item, value); err != nil { return fmt.Errorf("%s: %w", path, err) } @@ -304,14 +321,16 @@ func parseValue(s string) any { return s } -// parseArgs processes command-line arguments into a nested map structure. -func parseArgs(args []string) (map[string]any, error) { - result := make(map[string]any) +// parseArgs processes command-line arguments into a nested map structure. A +// bare flag is the bool true, which only a bool path accepts; arguments that +// are no --flag before "--" come back as stray, for UnknownCLIKeys. +func parseArgs(args []string) (result map[string]any, stray []string, err error) { + result = make(map[string]any) i := 0 for i < len(args) { arg := args[i] if !strings.HasPrefix(arg, "--") { - // Skip non-flag arguments + stray = append(stray, arg) i++ continue } @@ -324,6 +343,7 @@ func parseArgs(args []string) (map[string]any, error) { var keyPath string var valueStr string + bare := false // Check for "--key=value" format if strings.Contains(argContent, "=") { @@ -336,7 +356,7 @@ func parseArgs(args []string) (map[string]any, error) { keyPath = argContent // Check if it's a boolean flag (next arg is another flag or end of args) if i+1 >= len(args) || strings.HasPrefix(args[i+1], "--") { - valueStr = "true" + bare = true i++ // Consume only the flag argument } else { // It's a key-value pair with a space @@ -354,16 +374,20 @@ func parseArgs(args []string) (map[string]any, error) { segments := strings.Split(keyPath, ".") for _, segment := range segments { if !isValidKeySegment(segment) { - return nil, wrapError(ErrInvalidPath, fmt.Errorf("invalid command-line key segment %q in path %q", segment, keyPath)) + return nil, nil, wrapError(ErrInvalidPath, fmt.Errorf("invalid command-line key segment %q in path %q", segment, keyPath)) } } // Always store as a string. Let Scan handle final type conversion. if len(valueStr) > MaxValueSize { - return nil, ErrValueSize + return nil, nil, ErrValueSize + } + if bare { + setNestedValue(result, keyPath, true) + } else { + setNestedValue(result, keyPath, valueStr) } - setNestedValue(result, keyPath, valueStr) } - return result, nil + return result, stray, nil } diff --git a/loader_test.go b/loader_test.go index 662a775..cd57ce0 100644 --- a/loader_test.go +++ b/loader_test.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "reflect" "strings" "testing" ) @@ -229,7 +230,7 @@ func TestCLIParsing(t *testing.T) { name: "BooleanFlags", args: []string{"--enable.debug", "--disable.cache", "false"}, expected: map[string]any{ - "enable.debug": "true", + "enable.debug": true, "disable.cache": "false", }, }, @@ -244,7 +245,7 @@ func TestCLIParsing(t *testing.T) { expected: map[string]any{ "server.host": "localhost", "server.port": "8080", - "enable.tls": "true", + "enable.tls": true, "database.pool.size": "10", }, }, @@ -261,10 +262,10 @@ func TestCLIParsing(t *testing.T) { t.Run(tt.name, func(t *testing.T) { cfg := New() - // Register expected paths - for path := range tt.expected { + // Register expected paths with their type: a bare flag needs a bool path + for path, want := range tt.expected { if path != "" { // Skip empty path - cfg.Register(path, "") + cfg.Register(path, reflect.Zero(reflect.TypeOf(want)).Interface()) } } @@ -285,7 +286,7 @@ func TestCLIParsing(t *testing.T) { } t.Run("InvalidKeySegment", func(t *testing.T) { - result, err := parseArgs([]string{"--invalid!key=value"}) + result, _, err := parseArgs([]string{"--invalid!key=value"}) checkErrorContains(t, err, "invalid command-line key segment") if got := result; got != nil { t.Errorf("got %v, want nil", got) diff --git a/ownership_other.go b/ownership_other.go index 51ba210..008e345 100644 --- a/ownership_other.go +++ b/ownership_other.go @@ -7,6 +7,8 @@ import ( "os" ) +const openNonBlock = 0 + func checkFileOwner(os.FileInfo) error { return wrapError(ErrFileAccess, fmt.Errorf("file ownership checks are unsupported on this platform")) } diff --git a/ownership_unix.go b/ownership_unix.go index 13efb50..8bba98b 100644 --- a/ownership_unix.go +++ b/ownership_unix.go @@ -8,6 +8,8 @@ import ( "syscall" ) +const openNonBlock = syscall.O_NONBLOCK + func checkFileOwner(info os.FileInfo) error { stat, ok := info.Sys().(*syscall.Stat_t) if !ok || stat.Uid != uint32(os.Geteuid()) { diff --git a/watch.go b/watch.go index eca2e75..8fed3ca 100644 --- a/watch.go +++ b/watch.go @@ -197,6 +197,10 @@ func (w *watcher) checkAndReload(c *Config) { } else { w.notifyWatchers(event) w.handled = key + if event == EventPermissionsChanged { + // Reported once; the next change applies under the new mode + w.lastMode = snapshot.info.Mode() + } } return } diff --git a/watcher_hardening_test.go b/watcher_hardening_test.go index 684445a..d83f334 100644 --- a/watcher_hardening_test.go +++ b/watcher_hardening_test.go @@ -256,3 +256,36 @@ func TestBuilderDoesNotHideInvalidSourceBehindMissingFile(t *testing.T) { t.Fatalf("missing file hid invalid CLI: %v", err) } } + +// A mode change is reported once; the next edit applies under the new mode +func TestWatcherReportsPermissionChangeOnce(t *testing.T) { + c, path := watcherFixture(t) + w := manualWatcher(t, c, path) + w.opts.VerifyPermissions = true + ch := w.subscribe() + for i, step := range []struct { + edit func() error + event string + value int64 + }{ + {func() error { return os.WriteFile(path, []byte("value=2\n"), 0600) }, "value", 2}, + {func() error { return os.Chmod(path, 0640) }, EventPermissionsChanged, 2}, + {func() error { return os.WriteFile(path, []byte("value=3\n"), 0640) }, "value", 3}, + } { + if err := step.edit(); err != nil { + t.Fatal(err) + } + w.checkAndReload(c) + select { + case event := <-ch: + if event != step.event { + t.Fatalf("step %d: event %s, want %s", i, event, step.event) + } + default: + t.Fatalf("step %d: no event", i) + } + if value, _ := c.Get("value"); value != step.value { + t.Fatalf("step %d: value %v, want %d", i, value, step.value) + } + } +}