diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..2319c73 --- /dev/null +++ b/PR.md @@ -0,0 +1,63 @@ +# fix(flag): honor full separator length when splitting map key=value pairs + +## Problem + +Map flags support a configurable key/value separator via the `sep` tag. When the +separator is more than one byte long, the tail of the separator leaks into the +parsed value: + +```go +type T struct { + Map map[string]string `cli:"m" sep:"=>"` +} +// parsing -mkey=>val yields Map["key"] == ">val", want "val" +``` + +Internally: + +```go +splitKeyVal("key=>val", "=>") // ("key", ">val"), want ("key", "val") +splitKeyVal("a::b", "::") // ("a", ":b"), want ("a", "b") +``` + +The default separator is `=` (single byte), so the common case is unaffected; +the defect only appears for multi-character separators. + +## Root cause + +`flag.go`, `splitKeyVal`: + +```go +index := strings.Index(s, sep) +if index == -1 { + return s, "", nil +} +return s[:index], s[index+1:], nil +``` + +`strings.Index` returns the byte offset of the start of `sep`, but the value is +sliced from `index+1`, which only skips a single byte of the separator instead +of its whole length. + +## Fix + +Skip the full separator length: + +```go +return s[:index], s[index+len(sep):], nil +``` + +## Test + +Added `TestMapMultiCharSep` in `cli_test.go`, covering both `splitKeyVal` +directly and a map flag parsed through `parseArgv` with `sep:"=>"`. + +- Gate: `go test ./...` -> ok (PASS, exit 0, all packages) +- `go build ./...` -> exit 0 +- Counterfactual: reverting `flag.go` makes the test fail with + `splitKeyVal("key=>val", "=>") = ("key", ">val"), want ("key", "val")`. + +Note: `go vet ./...` exits 1 both before and after this change, due to +pre-existing example issues in `example_test.go` +(`ExampleNumOptionFunc_* refers to unknown identifier: NumOptionFunc`), which +are unrelated to this fix. diff --git a/cli_test.go b/cli_test.go index ab79c43..c19b90c 100644 --- a/cli_test.go +++ b/cli_test.go @@ -727,3 +727,25 @@ func TestDecoder(t *testing.T) { assert.Nil(t, flagSet.err) assert.Equal(t, v.D, customT{K1: "string", K2: 2}) } + +func TestMapMultiCharSep(t *testing.T) { + // White-box: split on a multi-character separator must not leak the + // separator tail into the value. + if k, v, _ := splitKeyVal("key=>val", "=>"); k != "key" || v != "val" { + t.Errorf(`splitKeyVal("key=>val", "=>") = (%q, %q), want ("key", "val")`, k, v) + } + + // Public API: a map flag configured with a multi-char sep. + type T struct { + Map map[string]string `cli:"m" sep:"=>"` + } + clr := color.Color{} + v := new(T) + flagSet := parseArgv([]string{"-mkey=>val"}, v, clr) + if flagSet.err != nil { + t.Fatalf("unexpected error: %v", flagSet.err) + } + if got := v.Map["key"]; got != "val" { + t.Errorf(`map value = %q, want "val"`, got) + } +} diff --git a/flag.go b/flag.go index 55b71cf..b57c0e4 100644 --- a/flag.go +++ b/flag.go @@ -436,7 +436,7 @@ func splitKeyVal(s, sep string) (key, val string, err error) { if index == -1 { return s, "", nil } - return s[:index], s[index+1:], nil + return s[:index], s[index+len(sep):], nil } func minmaxIntCheck(kind reflect.Kind, v int64) bool {