diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..8522b6e --- /dev/null +++ b/PR.md @@ -0,0 +1,58 @@ +# fix(read): prevent panic on negative slice index in a key path + +## Problem + +Looking up a key whose path contains a negative array index panics instead of +reporting "not found": + +```go +c.LoadData(map[string]any{"arr": []any{"a", "b", "c"}}) + +c.GetValue("arr.-1") // panic: runtime error: index out of range [-1] +c.Exists("arr.-1") // panic: runtime error: index out of range [-1] +``` + +Key paths frequently come from user input or configuration, so a stray +negative index crashes the caller. + +## Root cause + +`read.go`, in both `GetValue` and `Exists`, the slice cases parse the path +segment with `strconv.Atoi` and validate only the upper bound: + +```go +case []any: // is array(load from file) + i, err := strconv.Atoi(k) + if err != nil || len(typeData) <= i { // no lower-bound check + ... + return + } + item = typeData[i] // typeData[-1] panics +``` + +`strconv.Atoi("-1")` succeeds with `i == -1`, which passes `len(typeData) <= i` +(`3 <= -1` is false), so `typeData[-1]` is evaluated and panics. This affects +the `[]int`, `[]string` and `[]any` cases in both functions (six sites). + +## Fix + +Add a lower-bound check (`i < 0`) to every slice-index guard so a negative +index is treated as not found: + +```go +if err != nil || i < 0 || len(typeData) <= i { // GetValue +if err != nil || i < 0 || i >= len(typeData) { // Exists +``` + +## Test + +Added `TestConfig_GetValue_negativeIndex` in `read_test.go`, asserting that both +`GetValue("arr.-1")` and `Exists("arr.-1")` do not panic and report not found. + +- Gate: `go test ./...` -> ok (PASS, exit 0, all packages) +- `go vet ./...` -> exit 0 +- `go build ./...` -> exit 0 +- `gofmt -l read.go read_test.go` -> clean + +Counterfactual: reverting `read.go` makes the test fail with +`Panic value: runtime.boundsError{x:-1, y:3, ...}`. diff --git a/read.go b/read.go index 3bea68c..9c3ce52 100644 --- a/read.go +++ b/read.go @@ -63,19 +63,19 @@ func (c *Config) Exists(key string, findByPath ...bool) (ok bool) { } case []int: // is array(is from Set) i, err := strconv.Atoi(k) - if err != nil || i >= len(typeData) { + if err != nil || i < 0 || i >= len(typeData) { return false } item = typeData[i] case []string: // is array(is from Set) i, err := strconv.Atoi(k) - if err != nil || i >= len(typeData) { + if err != nil || i < 0 || i >= len(typeData) { return false } item = typeData[i] case []any: // is array(load from file) i, err := strconv.Atoi(k) - if err != nil || i >= len(typeData) { + if err != nil || i < 0 || i >= len(typeData) { return false } item = typeData[i] @@ -214,7 +214,7 @@ func (c *Config) GetValue(key string, findByPath ...bool) (value any, ok bool) { i, err := strconv.Atoi(k) // check slice index - if err != nil || len(typeData) <= i { + if err != nil || i < 0 || len(typeData) <= i { ok = false c.addError(err) return @@ -223,7 +223,7 @@ func (c *Config) GetValue(key string, findByPath ...bool) (value any, ok bool) { item = typeData[i] case []string: // is array(is from Set) i, err := strconv.Atoi(k) - if err != nil || len(typeData) <= i { + if err != nil || i < 0 || len(typeData) <= i { ok = false c.addError(err) return @@ -232,7 +232,7 @@ func (c *Config) GetValue(key string, findByPath ...bool) (value any, ok bool) { item = typeData[i] case []any: // is array(load from file) i, err := strconv.Atoi(k) - if err != nil || len(typeData) <= i { + if err != nil || i < 0 || len(typeData) <= i { ok = false c.addError(err) return diff --git a/read_test.go b/read_test.go index deaf395..12222ef 100644 --- a/read_test.go +++ b/read_test.go @@ -499,3 +499,25 @@ func TestConfig_Duration_getter(t *testing.T) { is.Eq(7*time.Second, c.Duration("missing", 7*time.Second)) is.Eq(time.Duration(0), c.Duration("missing")) } + +func TestConfig_GetValue_negativeIndex(t *testing.T) { + is := assert.New(t) + + c := New("test-neg") + err := c.LoadData(map[string]any{ + "arr": []any{"a", "b", "c"}, + }) + is.NoErr(err) + + // A negative slice index in a key path must not panic; it is simply + // treated as not found. + is.NotPanics(func() { + v, ok := c.GetValue("arr.-1") + is.False(ok) + is.Nil(v) + }) + + is.NotPanics(func() { + is.False(c.Exists("arr.-1")) + }) +}