Skip to content

Commit 03ac1ad

Browse files
committed
fix(plugins): handle nil intermediate values in JSON path traversal
Uses go-openapi/jsonpointer@v0.21.2 fix to improve handling of intermediate nil values. Signed-off-by: Matthew F Leader <mleader@redhat.com>
1 parent a7a9644 commit 03ac1ad

File tree

4 files changed

+103
-41
lines changed

4 files changed

+103
-41
lines changed

go.mod

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,10 @@ go 1.23.0
44

55
require (
66
github.com/go-logr/logr v1.4.1
7+
github.com/go-openapi/jsonpointer v0.21.2
78
github.com/google/go-cmp v0.7.0
8-
github.com/onsi/ginkgo/v2 v2.17.1
9-
github.com/onsi/gomega v1.32.0
109
github.com/stretchr/testify v1.10.0
10+
go.uber.org/zap v1.26.0
1111
gopkg.in/yaml.v2 v2.4.0
1212
gopkg.in/yaml.v3 v3.0.1
1313
k8s.io/api v0.29.2
@@ -31,10 +31,8 @@ require (
3131
github.com/fsnotify/fsnotify v1.7.0 // indirect
3232
github.com/go-errors/errors v1.4.2 // indirect
3333
github.com/go-logr/zapr v1.3.0 // indirect
34-
github.com/go-openapi/jsonpointer v0.21.1 // indirect
3534
github.com/go-openapi/jsonreference v0.21.0 // indirect
3635
github.com/go-openapi/swag v0.23.1 // indirect
37-
github.com/go-task/slim-sprig v0.0.0-20230315185526-52ccab3ef572 // indirect
3836
github.com/gogo/protobuf v1.3.2 // indirect
3937
github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8 // indirect
4038
github.com/golang/protobuf v1.5.4 // indirect
@@ -52,6 +50,8 @@ require (
5250
github.com/modern-go/reflect2 v1.0.2 // indirect
5351
github.com/monochromegane/go-gitignore v0.0.0-20200626010858-205db1a8cc00 // indirect
5452
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
53+
github.com/onsi/ginkgo/v2 v2.17.1 // indirect
54+
github.com/onsi/gomega v1.32.0 // indirect
5555
github.com/pkg/errors v0.9.1 // indirect
5656
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
5757
github.com/prometheus/client_golang v1.18.0 // indirect
@@ -61,15 +61,13 @@ require (
6161
github.com/spf13/pflag v1.0.5 // indirect
6262
github.com/xlab/treeprint v1.2.0 // indirect
6363
go.uber.org/multierr v1.11.0 // indirect
64-
go.uber.org/zap v1.26.0 // indirect
6564
golang.org/x/exp v0.0.0-20250506013437-ce4c2cf36ca6 // indirect
6665
golang.org/x/net v0.40.0 // indirect
6766
golang.org/x/oauth2 v0.30.0 // indirect
6867
golang.org/x/sys v0.33.0 // indirect
6968
golang.org/x/term v0.32.0 // indirect
7069
golang.org/x/text v0.25.0 // indirect
7170
golang.org/x/time v0.11.0 // indirect
72-
golang.org/x/tools v0.33.0 // indirect
7371
gomodules.xyz/jsonpatch/v2 v2.5.0 // indirect
7472
google.golang.org/protobuf v1.36.6 // indirect
7573
gopkg.in/evanphx/json-patch.v4 v4.12.0 // indirect

go.sum

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,8 @@ github.com/go-logr/logr v1.4.1 h1:pKouT5E8xu9zeFC39JXRDukb6JFQPXM5p5I91188VAQ=
2323
github.com/go-logr/logr v1.4.1/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
2424
github.com/go-logr/zapr v1.3.0 h1:XGdV8XW8zdwFiwOA2Dryh1gj2KRQyOOoNmBy4EplIcQ=
2525
github.com/go-logr/zapr v1.3.0/go.mod h1:YKepepNBd1u/oyhd/yQmtjVXmm9uML4IXUgMOwR8/Gg=
26-
github.com/go-openapi/jsonpointer v0.21.1 h1:whnzv/pNXtK2FbX/W9yJfRmE2gsmkfahjMKB0fZvcic=
27-
github.com/go-openapi/jsonpointer v0.21.1/go.mod h1:50I1STOfbY1ycR8jGz8DaMeLCdXiI6aDteEdRNNzpdk=
26+
github.com/go-openapi/jsonpointer v0.21.2 h1:AqQaNADVwq/VnkCmQg6ogE+M3FOsKTytwges0JdwVuA=
27+
github.com/go-openapi/jsonpointer v0.21.2/go.mod h1:50I1STOfbY1ycR8jGz8DaMeLCdXiI6aDteEdRNNzpdk=
2828
github.com/go-openapi/jsonreference v0.21.0 h1:Rs+Y7hSXT83Jacb7kFyjn4ijOuVGSvOdF2+tg1TRrwQ=
2929
github.com/go-openapi/jsonreference v0.21.0/go.mod h1:LmZmgsrTkVg9LG4EaHeY8cBDslNPMo06cago5JNLkm4=
3030
github.com/go-openapi/swag v0.23.1 h1:lpsStH0n2ittzTnbaSloVZLuB5+fvSY/+hnagBjSNZU=
@@ -103,7 +103,6 @@ github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+
103103
github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY=
104104
github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA=
105105
github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
106-
github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
107106
github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
108107
github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA=
109108
github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=

pkg/deploy/plugins/field_mutator.go

Lines changed: 40 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package plugins
22

33
import (
4+
"encoding/json"
45
"fmt"
56
"strings"
67

@@ -126,53 +127,60 @@ func setTargetField(res *resource.Resource, value any, mapping FieldMapping) err
126127
}
127128

128129
func setWithPathCreation(data any, ptr jsonpointer.Pointer, value any) (any, error) {
129-
// try setting value if the path already exists
130-
if updatedData, err := ptr.Set(data, value); err == nil {
131-
return updatedData, nil
130+
// try direct set first
131+
if result, err := ptr.Set(data, value); err == nil {
132+
return result, nil
132133
}
133134

134-
// otherwise, we need to create the path first
135+
// create missing path structure
135136
tokens := ptr.DecodedTokens()
136137
if len(tokens) == 0 {
137138
return value, nil
138139
}
139-
result := data
140140

141-
// JSON Pointer Set() fails if intermediate paths don't exist, so we must
142-
// build the path incrementally from root to target, creating missing containers
143-
for i := 1; i <= len(tokens)-1; i++ {
144-
partialPath := "/" + strings.Join(tokens[:i], "/")
145-
partialPtr, err := jsonpointer.New(partialPath)
146-
if err != nil {
147-
return nil, fmt.Errorf("failed to create partial pointer: %w", err)
141+
result := deepCopyData(data)
142+
143+
// create each missing parent container
144+
for i := 0; i < len(tokens)-1; i++ {
145+
parentPath := "/" + strings.Join(tokens[:i+1], "/")
146+
parentPtr, _ := jsonpointer.New(parentPath)
147+
148+
// skip if parent already exists and is not nil
149+
if parent, _, err := parentPtr.Get(result); err == nil && parent != nil {
150+
continue
148151
}
149-
// Get() is used as existence test - error means path doesn't exist and needs creation
150-
_, _, err = partialPtr.Get(result)
151-
if err != nil {
152-
nextToken := tokens[i]
153-
var newContainer any
154-
// create array if next token is numeric (e.g., "/ports/0")
155-
if isNumericString(nextToken) {
156-
newContainer = make([]any, 0)
157-
} else {
158-
// create map otherwise (e.g., "/spec/strategy")
159-
newContainer = make(map[string]any)
160-
}
161152

162-
// create the missing path segment
163-
result, err = partialPtr.Set(result, newContainer)
164-
if err != nil {
165-
return nil, fmt.Errorf("failed to create intermediate path %q: %w", partialPath, err)
166-
}
153+
// create container based on next token type
154+
var container any
155+
if isNumericString(tokens[i+1]) {
156+
container = make([]any, 0)
157+
} else {
158+
container = make(map[string]any)
159+
}
160+
161+
// set the container (jsonpointer handles the rest)
162+
var err error
163+
if result, err = parentPtr.Set(result, container); err != nil {
164+
return nil, fmt.Errorf("failed to create path at %q: %w", parentPath, err)
167165
}
168166
}
169167

170-
result, err := ptr.Set(result, value)
168+
// final set should now succeed
169+
return ptr.Set(result, value)
170+
}
171+
172+
// deepCopyData creates a deep copy of the data structure using JSON marshal/unmarshal.
173+
func deepCopyData(data any) any {
174+
jsonBytes, err := json.Marshal(data)
171175
if err != nil {
172-
return nil, fmt.Errorf("failed to set final value: %w", err)
176+
return data // fallback to original if copy fails
173177
}
174178

175-
return result, nil
179+
var result any
180+
if err := json.Unmarshal(jsonBytes, &result); err != nil {
181+
return data // fallback to original if copy fails
182+
}
183+
return result
176184
}
177185

178186
func updateResource(res *resource.Resource, updatedData any) error {

pkg/deploy/plugins/field_mutator_test.go

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,3 +293,60 @@ func TestTransform(t *testing.T) {
293293
})
294294
}
295295
}
296+
297+
// TestJsonPointerNilTraversalPanic reproduces the panic when jsonpointer encounters nil intermediate values.
298+
func TestJsonPointerNilTraversalPanic(t *testing.T) {
299+
t.Run("panic when traversing through nil intermediate value", func(t *testing.T) {
300+
// create a data structure with nil intermediate value that causes jsonpointer to panic
301+
testData := map[string]any{
302+
"apiVersion": "v1",
303+
"kind": "TestResource",
304+
"metadata": map[string]any{
305+
"name": "test-resource",
306+
},
307+
"spec": map[string]any{
308+
"container": map[string]any{
309+
"properties": nil, // this nil causes the panic when traversed
310+
},
311+
},
312+
}
313+
314+
rf := resource.NewFactory(nil)
315+
res, err := rf.FromMap(testData)
316+
require.NoError(t, err)
317+
318+
resMap := resmap.New()
319+
require.NoError(t, resMap.Append(res))
320+
321+
// field mapping that attempts to traverse through the nil intermediate value
322+
transformer := CreateFieldMutator(FieldMutatorConfig{
323+
Mappings: []FieldMapping{
324+
{
325+
SourceValue: nil,
326+
DefaultValue: "test-value",
327+
TargetField: "/spec/container/properties/name", // this path goes through nil
328+
TargetKind: "TestResource",
329+
CreateIfNotExists: true,
330+
},
331+
},
332+
})
333+
334+
err = transformer.Transform(resMap)
335+
require.NoError(t, err, "Transform should succeed after the fix")
336+
337+
// nil properties should be replaced with a proper map
338+
updatedRes := resMap.GetByIndex(0)
339+
resMap2, err := updatedRes.Map()
340+
require.NoError(t, err)
341+
342+
spec, ok := resMap2["spec"].(map[string]any)
343+
require.True(t, ok, "spec should be a map")
344+
container, ok := spec["container"].(map[string]any)
345+
require.True(t, ok, "container should be a map")
346+
properties, ok := container["properties"].(map[string]any)
347+
require.True(t, ok, "properties should be a map")
348+
349+
require.Equal(t, "test-value", properties["name"], "name should be set correctly")
350+
require.NotNil(t, properties, "properties should no longer be nil")
351+
})
352+
}

0 commit comments

Comments
 (0)