fix(flag): honor full separator length when splitting map key=value pairs
splitKeyVal sliced the value from index+1, skipping only one byte of the key/value separator. With a multi-character sep tag (e.g. sep:"=>") the remaining separator bytes leaked into the value. Slice from index+len(sep) so the whole separator is consumed.
This commit is contained in:
1 parent
78a016e843
commit
d03cd5d09c
3 files changed
+86
-1
No files matched your search
@@ -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.
|
||||||
+22
@@ -727,3 +727,25 @@ func TestDecoder(t *testing.T) {
|
|||||||
assert.Nil(t, flagSet.err)
|
assert.Nil(t, flagSet.err)
|
||||||
assert.Equal(t, v.D, customT{K1: "string", K2: 2})
|
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)
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -436,7 +436,7 @@ func splitKeyVal(s, sep string) (key, val string, err error) {
|
|||||||
if index == -1 {
|
if index == -1 {
|
||||||
return s, "", nil
|
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 {
|
func minmaxIntCheck(kind reflect.Kind, v int64) bool {
|
||||||
|
|||||||
Reference in new issue
Block a user