Skip to content

Commit c71171e

Browse files
fredbiCopilotclaude
committed
fix: panics in Pointer.Get, Pointer.Set and json name resolution
Get and Set panicked on several ordinary document shapes. All are reachable from the public API with no privileges, so a consumer resolving pointers against attacker-influenced JSON could be crashed. Set with a nil value -- what encoding/json decodes a JSON null into -- called reflect.Value.Type on the zero Value in the struct and slice branches: reflect: call of reflect.Value.Type on zero Value A nil value now resolves to the zero value of the target when the target can hold nil, and reports an error wrapping ErrPointer when it cannot: a string field has no representation of null, and substituting "" would erase the difference. The map branch of setSingleImpl had no guard at all. reflect.Value.SetMapIndex panicked whenever the value was not assignable to the map value type, and deleted the member outright when handed the zero Value, so setting a map[string]any member to null silently removed it. The member is now kept with a nil value, and a mismatched type reports an error instead of panicking. jsonname.buildnameIndex recursed into anonymous fields without checking their kind, so embedding *Base panicked while building the name index under the default provider: reflect: NumField of non-struct type *main.Base The same panic hit the exported GetJSONNames for any non-struct subject. Anonymous fields are now reduced with structTypeOf: a pointer is dereferenced, and a field that is not a struct promotes nothing. newNameIndex and the three entry points tolerate a nil or non-struct subject, and GoNameProvider gets the same nil-subject guard. Fixing that alone only moved the crash: a field promoted through a nil embedded pointer then panicked in reflect.Value.FieldByName. The four call sites go through fieldByName, which uses reflect.Value.FieldByIndexErr and reports "cannot reach field". Maps keyed by a non-string type panicked in MapIndex and SetMapIndex, on Get as well as Set. Tokens are now converted against the map key type by mapKeyValue, which also makes map[NamedStringType]V resolve instead of panicking. A sweep of 4224 Get, Set, GetForToken and SetForToken calls over these shapes panics 0 times. The same sweep panics 483 times on master. The three copies of the nil-handling block are folded into resolveSetValue, which also clears the gocognit (44 > 30) and nestif reports that the first version of this branch introduced. Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Frédéric BIDON <fredbi@yahoo.com>
1 parent 2109391 commit c71171e

7 files changed

Lines changed: 623 additions & 47 deletions

File tree

‎.claude/CLAUDE.md‎

Lines changed: 46 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,39 +8,73 @@ Go implementation of [JSON Pointer (RFC 6901)](https://datatracker.ietf.org/doc/
88
and mutating JSON documents represented as Go values. Unlike most implementations, it works not only with
99
`map[string]any` and slices, but also with Go structs (resolved via `json` struct tags and reflection).
1010

11-
See [docs/MAINTAINERS.md](../docs/MAINTAINERS.md) for CI/CD, release process, and repo structure details.
11+
### Package layout
1212

13-
### Package layout (single package)
13+
Two packages in one module (`github.com/go-openapi/jsonpointer`):
1414

1515
| File | Contents |
1616
|------|----------|
17-
| `pointer.go` | Core types (`Pointer`, `JSONPointable`, `JSONSetable`), `New`, `Get`, `Set`, `Offset`, `Escape`/`Unescape` |
18-
| `errors.go` | Sentinel errors: `ErrPointer`, `ErrInvalidStart`, `ErrUnsupportedValueType` |
17+
| `pointer.go` | `Pointer`, `New`, `Get`, `Set`, `Offset`, `GetForToken`, `SetForToken`, `Escape`/`Unescape`, and the reflection helpers |
18+
| `ifaces.go` | `JSONPointable`, `JSONSetable`, `NameProvider` interfaces |
19+
| `options.go` | `Option`, `WithNameProvider`, `SetDefaultNameProvider`, `UseGoNameProvider`, `DefaultNameProvider` |
20+
| `errors.go` | Sentinel errors and their constructors |
21+
| `jsonname/name_provider.go` | `NameProvider` — the default, tag-only resolver (`DefaultJSONNameProvider`) |
22+
| `jsonname/go_name_provider.go` | `GoNameProvider` — the alternative resolver, follows `encoding/json` rules |
1923

2024
### Key API
2125

2226
- `New(string) (Pointer, error)` — parse a JSON pointer string (e.g. `"/foo/0/bar"`)
23-
- `Pointer.Get(document any) (any, reflect.Kind, error)` — retrieve a value
24-
- `Pointer.Set(document, value any) (any, error)` — set a value (document must be pointer/map/slice)
27+
- `Pointer.Get(document any, opts ...Option) (any, reflect.Kind, error)` — retrieve a value
28+
- `Pointer.Set(document, value any, opts ...Option) (any, error)` — set a value (document must be a pointer, map or slice)
2529
- `Pointer.Offset(jsonString string) (int64, error)` — byte offset of token in raw JSON
2630
- `GetForToken` / `SetForToken` — single-level convenience helpers
2731
- `Escape` / `Unescape` — RFC 6901 token escaping (`~0` ↔ `~`, `~1` ↔ `/`)
2832

2933
Custom types can implement `JSONPointable` (for Get) or `JSONSetable` (for Set) to bypass reflection.
3034

35+
Sentinel errors: `ErrPointer`, `ErrInvalidStart`, `ErrUnsupportedValueType`, `ErrDashToken`.
36+
Errors raised by the package wrap `ErrPointer`. Errors returned by a caller's `JSONLookup` or
37+
`JSONSet` are passed through unchanged, so `errors.Is(err, ErrPointer)` is false for those.
38+
39+
### Name providers
40+
41+
Two implementations of the `NameProvider` interface resolve go field names to json names. They
42+
differ, and the difference is load-bearing for most "why is my field unreachable" questions:
43+
44+
- `jsonname.NameProvider` (the default, `jsonname.DefaultJSONNameProvider`): only fields carrying a
45+
non-empty `json` tag are reachable.
46+
- `jsonname.GoNameProvider` (opt in with `UseGoNameProvider()` or `WithNameProvider`): follows
47+
`encoding/json`, so untagged exported fields are reachable under their go name.
48+
3149
### Dependencies
3250

33-
- `github.com/go-openapi/swag/jsonname` — struct tag → JSON field name resolution
3451
- `github.com/go-openapi/testify/v2` — test-only assertions
3552

36-
### Notable historical design decisions
53+
The module has no non-test dependency. `jsonname` is a subpackage of this repository, not the
54+
similarly named `github.com/go-openapi/swag/jsonname`.
3755

38-
See also .claude/plans/ROADMAP.md.
56+
### Notable design decisions
3957

40-
- Struct fields **must** have a `json` tag to be reachable; untagged fields are ignored
41-
(differs from `encoding/json` which defaults to the Go field name).
42-
- Anonymous embedded struct fields are traversed only if tagged.
58+
- Under the default provider, struct fields **must** have a `json` tag to be reachable; untagged
59+
fields are ignored (differs from `encoding/json`, which defaults to the go field name). Use
60+
`UseGoNameProvider()` for `encoding/json` semantics.
61+
- An anonymous (embedded) field is walked for the fields it promotes, and its own `json` tag is
62+
ignored — tagged or not, the tag never becomes a reachable key.
63+
- Embedding is traversed through a pointer (`*Base`). Resolving a promoted field on a value whose
64+
embedded pointer is nil reports an error; it does not allocate the intermediate struct.
65+
- An anonymous field that is not a struct (e.g. an embedded named slice) promotes nothing and is
66+
unreachable.
67+
- Maps must be keyed by a string type. A named string type is converted; `map[int]V` and friends
68+
cannot be addressed by a pointer and report an error.
69+
- `Set` with a nil value (what `encoding/json` decodes a JSON null into) writes the zero value when
70+
the target can hold nil — interface, pointer, map, slice, chan, func. On a target that cannot
71+
represent null (a `string` field, an element of `[]int`) it reports an error rather than
72+
substituting a zero value. Setting a map member to nil keeps the member; it does not delete it.
4373
- The RFC 6901 `"-"` array suffix is supported on `Pointer.Set` as an append
4474
operation (RFC 6902 convention). On `Pointer.Get` / `Pointer.Offset` it is
4575
always an error per RFC 6901 §4.
4676

77+
### Conventions
78+
79+
See `.claude/rules/` for contribution, linting, go and technical-writing conventions.
80+
Format with `golangci-lint fmt`, and check a change with `golangci-lint run --new-from-rev master`.

‎errors.go‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,10 @@
33

44
package jsonpointer
55

6-
import "fmt"
6+
import (
7+
"fmt"
8+
"reflect"
9+
)
710

811
type pointerError string
912

@@ -47,6 +50,24 @@ func errInvalidReference(token string) error {
4750
return fmt.Errorf("invalid token reference %q: %w", token, ErrPointer)
4851
}
4952

53+
func errNoField(token string) error {
54+
return fmt.Errorf("object has no field %q: %w", token, ErrPointer)
55+
}
56+
57+
// errUnreachableField reports a field that exists on the type but cannot be reached on this value,
58+
// because it is promoted through a nil embedded pointer.
59+
func errUnreachableField(name string, cause error) error {
60+
return fmt.Errorf("cannot reach field %q: %w: %w", name, cause, ErrPointer)
61+
}
62+
63+
// errMapKey reports a reference token that cannot serve as a key of the target map type.
64+
//
65+
// JSON object member names are strings, so only maps keyed by a string type (or by an interface
66+
// type a string satisfies) can be addressed by a JSON pointer.
67+
func errMapKey(token string, mapType reflect.Type) error {
68+
return fmt.Errorf("can't use token %q as a key of map type %v: %w", token, mapType, ErrPointer)
69+
}
70+
5071
func errDashOnGet() error {
5172
return fmt.Errorf("cannot resolve %q token on get: %w: %w", dashToken, ErrDashToken, ErrPointer)
5273
}

‎jsonname/go_name_provider.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ func (n *GoNameProvider) GetJSONNames(subject any) []string {
4646
n.lock.Lock()
4747
defer n.lock.Unlock()
4848

49-
tpe := reflect.Indirect(reflect.ValueOf(subject)).Type()
49+
tpe := typeOfSubject(subject)
5050
names := n.nameIndexFor(tpe)
5151

5252
res := make([]string, 0, len(names.jsonNames))
@@ -59,7 +59,7 @@ func (n *GoNameProvider) GetJSONNames(subject any) []string {
5959

6060
// GetJSONName gets the json name for a go property name.
6161
func (n *GoNameProvider) GetJSONName(subject any, name string) (string, bool) {
62-
tpe := reflect.Indirect(reflect.ValueOf(subject)).Type()
62+
tpe := typeOfSubject(subject)
6363

6464
return n.GetJSONNameForType(tpe, name)
6565
}
@@ -77,7 +77,7 @@ func (n *GoNameProvider) GetJSONNameForType(tpe reflect.Type, name string) (stri
7777

7878
// GetGoName gets the go name for a json property name.
7979
func (n *GoNameProvider) GetGoName(subject any, name string) (string, bool) {
80-
tpe := reflect.Indirect(reflect.ValueOf(subject)).Type()
80+
tpe := typeOfSubject(subject)
8181

8282
return n.GetGoNameForType(tpe, name)
8383
}
@@ -132,7 +132,7 @@ func buildGoNameIndex(tpe reflect.Type) nameIndex {
132132
//
133133
//nolint:gocognit // everything is inlined to help the compiler determine what escapes and what doesn't
134134
func collectGoFields(tpe reflect.Type) []fieldEntry {
135-
if tpe.Kind() != reflect.Struct {
135+
if tpe == nil || tpe.Kind() != reflect.Struct {
136136
return nil
137137
}
138138

‎jsonname/name_provider.go‎

Lines changed: 49 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,13 @@ func buildnameIndex(tpe reflect.Type, idx, reverseIdx map[string]string) {
4747
}
4848

4949
if targetDes.Anonymous { // walk embedded structures tree down first
50-
buildnameIndex(targetDes.Type, idx, reverseIdx)
50+
// An embedded field is not necessarily a struct: it may be a pointer to a struct, or a named
51+
// non-struct type (e.g. an embedded named slice or map). Only struct shapes carry promoted
52+
// fields worth indexing; anything else contributes no json name and must not be walked.
53+
if embedded := structTypeOf(targetDes.Type); embedded != nil {
54+
buildnameIndex(embedded, idx, reverseIdx)
55+
}
56+
5157
continue
5258
}
5359

@@ -73,18 +79,57 @@ func buildnameIndex(tpe reflect.Type, idx, reverseIdx map[string]string) {
7379
}
7480

7581
func newNameIndex(tpe reflect.Type) nameIndex {
82+
tpe = structTypeOf(tpe)
83+
if tpe == nil {
84+
// only struct shapes carry json names: anything else indexes to nothing.
85+
return nameIndex{jsonNames: map[string]string{}, goNames: map[string]string{}}
86+
}
87+
7688
idx := make(map[string]string, tpe.NumField())
7789
reverseIdx := make(map[string]string, tpe.NumField())
7890

7991
buildnameIndex(tpe, idx, reverseIdx)
8092
return nameIndex{jsonNames: idx, goNames: reverseIdx}
8193
}
8294

95+
// structTypeOf reduces tpe to the struct type it designates, dereferencing a pointer type.
96+
//
97+
// It returns nil when tpe is nil or does not designate a struct, so that callers may treat
98+
// "nothing to index here" uniformly instead of panicking in [reflect.Type.NumField].
99+
func structTypeOf(tpe reflect.Type) reflect.Type {
100+
if tpe == nil {
101+
return nil
102+
}
103+
104+
if tpe.Kind() == reflect.Pointer {
105+
tpe = tpe.Elem()
106+
}
107+
108+
if tpe.Kind() != reflect.Struct {
109+
return nil
110+
}
111+
112+
return tpe
113+
}
114+
115+
// typeOfSubject returns the type of a document subject, dereferencing pointers.
116+
//
117+
// It returns nil for an untyped nil subject, which resolves to an empty name index rather than
118+
// panicking in [reflect.Value.Type].
119+
func typeOfSubject(subject any) reflect.Type {
120+
rValue := reflect.Indirect(reflect.ValueOf(subject))
121+
if !rValue.IsValid() {
122+
return nil
123+
}
124+
125+
return rValue.Type()
126+
}
127+
83128
// GetJSONNames gets all the json property names for a type.
84129
func (n *NameProvider) GetJSONNames(subject any) []string {
85130
n.lock.Lock()
86131
defer n.lock.Unlock()
87-
tpe := reflect.Indirect(reflect.ValueOf(subject)).Type()
132+
tpe := typeOfSubject(subject)
88133
names, ok := n.index[tpe]
89134
if !ok {
90135
names = n.makeNameIndex(tpe)
@@ -99,7 +144,7 @@ func (n *NameProvider) GetJSONNames(subject any) []string {
99144

100145
// GetJSONName gets the json name for a go property name.
101146
func (n *NameProvider) GetJSONName(subject any, name string) (string, bool) {
102-
tpe := reflect.Indirect(reflect.ValueOf(subject)).Type()
147+
tpe := typeOfSubject(subject)
103148
return n.GetJSONNameForType(tpe, name)
104149
}
105150

@@ -117,7 +162,7 @@ func (n *NameProvider) GetJSONNameForType(tpe reflect.Type, name string) (string
117162

118163
// GetGoName gets the go name for a json property name.
119164
func (n *NameProvider) GetGoName(subject any, name string) (string, bool) {
120-
tpe := reflect.Indirect(reflect.ValueOf(subject)).Type()
165+
tpe := typeOfSubject(subject)
121166
return n.GetGoNameForType(tpe, name)
122167
}
123168

‎jsonname/name_provider_test.go‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package jsonname
55

66
import (
7+
"fmt"
78
"reflect"
89
"testing"
910

@@ -124,3 +125,100 @@ func TestNameProvider(t *testing.T) {
124125

125126
assert.Len(t, provider.index, 1)
126127
}
128+
129+
type EmbeddedBase struct {
130+
ID string `json:"id"`
131+
}
132+
133+
// WithEmbeddedPointer embeds a pointer to a struct, the shape that used to panic in
134+
// buildnameIndex with "reflect: NumField of non-struct type *jsonname.EmbeddedBase".
135+
type WithEmbeddedPointer struct {
136+
*EmbeddedBase
137+
138+
Name string `json:"name"`
139+
}
140+
141+
type NamedSlice []string
142+
143+
// WithEmbeddedNonStruct embeds a named slice type, which carries no field to promote.
144+
type WithEmbeddedNonStruct struct {
145+
NamedSlice
146+
147+
Name string `json:"name"`
148+
}
149+
150+
type WithEmbeddedTaggedNonStruct struct {
151+
NamedSlice `json:"list"`
152+
153+
Name string `json:"name"`
154+
}
155+
156+
func TestNameProviderAnonymousFields(t *testing.T) {
157+
t.Run("should promote fields of an embedded pointer to struct", func(t *testing.T) {
158+
provider := NewNameProvider()
159+
160+
nm, ok := provider.GetGoName(WithEmbeddedPointer{EmbeddedBase: nil, Name: ""}, "id")
161+
assert.TrueT(t, ok)
162+
assert.EqualT(t, "ID", nm)
163+
164+
nm, ok = provider.GetGoName(WithEmbeddedPointer{EmbeddedBase: nil, Name: ""}, "name")
165+
assert.TrueT(t, ok)
166+
assert.EqualT(t, "Name", nm)
167+
})
168+
169+
t.Run("should index a struct embedding a non-struct type", func(t *testing.T) {
170+
provider := NewNameProvider()
171+
172+
nm, ok := provider.GetGoName(WithEmbeddedNonStruct{NamedSlice: nil, Name: ""}, "name")
173+
assert.TrueT(t, ok)
174+
assert.EqualT(t, "Name", nm)
175+
})
176+
177+
t.Run("should index a struct embedding a tagged non-struct type", func(t *testing.T) {
178+
provider := NewNameProvider()
179+
180+
nm, ok := provider.GetGoName(WithEmbeddedTaggedNonStruct{NamedSlice: nil, Name: ""}, "name")
181+
assert.TrueT(t, ok)
182+
assert.EqualT(t, "Name", nm)
183+
})
184+
}
185+
186+
func TestNameProviderNonStructSubjects(t *testing.T) {
187+
// a subject that is not a struct carries no json name: it indexes to nothing rather than
188+
// panicking in reflect.Type.NumField or reflect.Value.Type.
189+
for _, subject := range []any{
190+
map[string]any{"a": 1},
191+
42,
192+
"a string",
193+
[]int{1, 2},
194+
nil,
195+
} {
196+
t.Run(fmt.Sprintf("subject %T", subject), func(t *testing.T) {
197+
t.Run("NameProvider", func(t *testing.T) {
198+
provider := NewNameProvider()
199+
assert.Empty(t, provider.GetJSONNames(subject))
200+
201+
nm, ok := provider.GetGoName(subject, "a")
202+
assert.FalseT(t, ok)
203+
assert.Empty(t, nm)
204+
205+
nm, ok = provider.GetJSONName(subject, "A")
206+
assert.FalseT(t, ok)
207+
assert.Empty(t, nm)
208+
})
209+
210+
t.Run("GoNameProvider", func(t *testing.T) {
211+
provider := NewGoNameProvider()
212+
assert.Empty(t, provider.GetJSONNames(subject))
213+
214+
nm, ok := provider.GetGoName(subject, "a")
215+
assert.FalseT(t, ok)
216+
assert.Empty(t, nm)
217+
218+
nm, ok = provider.GetJSONName(subject, "A")
219+
assert.FalseT(t, ok)
220+
assert.Empty(t, nm)
221+
})
222+
})
223+
}
224+
}

0 commit comments

Comments
 (0)