From 12c1e9f8ecd731afc4e092967508fa226b80a461 Mon Sep 17 00:00:00 2001 From: Philippe Scorsolini Date: Wed, 22 Feb 2023 18:16:32 +0100 Subject: [PATCH 1/2] fix: enforce max index value for paths Signed-off-by: Philippe Scorsolini (cherry picked from commit 7560fbc0415bf69c219b96949f3b2d2839538d56) --- pkg/fieldpath/paved.go | 37 ++++++++++++++++++++++++++++++++----- pkg/fieldpath/paved_test.go | 36 +++++++++++++++++++++++++++++++++++- 2 files changed, 67 insertions(+), 6 deletions(-) diff --git a/pkg/fieldpath/paved.go b/pkg/fieldpath/paved.go index bcfc90bad..1ef49c275 100644 --- a/pkg/fieldpath/paved.go +++ b/pkg/fieldpath/paved.go @@ -25,6 +25,9 @@ import ( "github.com/crossplane/crossplane-runtime/pkg/errors" ) +// DefaultMaxFieldPathIndex is the max allowed index in a field path. +const DefaultMaxFieldPathIndex = 1024 + type errNotFound struct { error } @@ -46,19 +49,39 @@ func IsNotFound(err error) bool { // A Paved JSON object supports getting and setting values by their field path. type Paved struct { - object map[string]any + object map[string]any + maxFieldPathIndex uint } +type PavedOption func(paved *Paved) + // PaveObject paves a runtime.Object, making it possible to get and set values // by field path. o must be a non-nil pointer to an object. -func PaveObject(o runtime.Object) (*Paved, error) { +func PaveObject(o runtime.Object, opts ...PavedOption) (*Paved, error) { u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(o) - return Pave(u), errors.Wrap(err, "cannot convert object to unstructured data") + return Pave(u, opts...), errors.Wrap(err, "cannot convert object to unstructured data") } // Pave a JSON object, making it possible to get and set values by field path. -func Pave(object map[string]any) *Paved { - return &Paved{object: object} +func Pave(object map[string]any, opts ...PavedOption) *Paved { + p := &Paved{object: object, maxFieldPathIndex: DefaultMaxFieldPathIndex} + + for _, opt := range opts { + opt(p) + } + + return p +} + +// WithMaxFieldPathIndex returns a PavedOption that sets the max allowed index for field paths, 0 means no limit. +func WithMaxFieldPathIndex(max uint) PavedOption { + return func(paved *Paved) { + paved.maxFieldPathIndex = max + } +} + +func (p *Paved) maxFieldPathIndexEnabled() bool { + return p.maxFieldPathIndex > 0 } // MarshalJSON to the underlying object. @@ -358,6 +381,10 @@ func (p *Paved) setValue(s Segments, value any) error { return errors.Errorf("%s is not an array", s[:i]) } + if p.maxFieldPathIndexEnabled() && current.Index > p.maxFieldPathIndex { + return errors.Errorf("index %d is greater than max allowed index %d", current.Index, p.maxFieldPathIndex) + } + if final { array[current.Index] = v return nil diff --git a/pkg/fieldpath/paved_test.go b/pkg/fieldpath/paved_test.go index 1828b949c..ab0dbd9ee 100644 --- a/pkg/fieldpath/paved_test.go +++ b/pkg/fieldpath/paved_test.go @@ -17,6 +17,7 @@ limitations under the License. package fieldpath import ( + "fmt" "testing" "github.com/google/go-cmp/cmp" @@ -593,6 +594,7 @@ func TestSetValue(t *testing.T) { type args struct { path string value any + opts []PavedOption } type want struct { object map[string]any @@ -737,6 +739,38 @@ func TestSetValue(t *testing.T) { }, }, }, + "RejectsHighIndexes": { + reason: "Paths having indexes above the maximum default value are rejected", + data: []byte(`{"data":["a"]}`), + args: args{ + path: fmt.Sprintf("data[%v]", MaxFieldPathIndex+1), + value: "c", + }, + want: want{ + object: map[string]any{ + "data": []any{"a"}}, + err: errors.Wrap(errors.Errorf("found index above max (%[1]v > %[2]v): data[%[1]v]", + MaxFieldPathIndex+1, MaxFieldPathIndex), "invalid segments"), + }, + }, + "NotRejectsHighIndexesIfNoDefaultOptions": { + reason: "Paths having indexes above the maximum default value are not rejected if default disabled", + data: []byte(`{"data":["a"]}`), + args: args{ + path: fmt.Sprintf("data[%v]", MaxFieldPathIndex+1), + value: "c", + opts: []PavedOption{}, + }, + want: want{ + object: map[string]any{ + "data": func() []any { + res := make([]any, MaxFieldPathIndex+2) + res[0] = "a" + res[MaxFieldPathIndex+1] = "c" + return res + }()}, + }, + }, "MapStringString": { reason: "A map of string to string should be converted to a map of string to any", data: []byte(`{"metadata":{}}`), @@ -817,7 +851,7 @@ func TestSetValue(t *testing.T) { t.Run(name, func(t *testing.T) { in := make(map[string]any) _ = json.Unmarshal(tc.data, &in) - p := Pave(in) + p := Pave(in, tc.args.opts...) err := p.SetValue(tc.args.path, tc.args.value) if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" { From abf25d727eaacf27bfbbdb9e9c9ea9f0705611de Mon Sep 17 00:00:00 2001 From: Philippe Scorsolini Date: Wed, 8 Mar 2023 15:10:39 +0100 Subject: [PATCH 2/2] fix: properly validate max index Signed-off-by: Philippe Scorsolini (cherry picked from commit 0aac4ba54682327e6c4154a2ab4bcce1347cd7c3) --- pkg/fieldpath/paved.go | 39 ++++++++++++++++++++++++++++--------- pkg/fieldpath/paved_test.go | 14 ++++++------- 2 files changed, 37 insertions(+), 16 deletions(-) diff --git a/pkg/fieldpath/paved.go b/pkg/fieldpath/paved.go index 1ef49c275..bbc9482e8 100644 --- a/pkg/fieldpath/paved.go +++ b/pkg/fieldpath/paved.go @@ -53,6 +53,7 @@ type Paved struct { maxFieldPathIndex uint } +// PavedOption can be used to configure a Paved behavior. type PavedOption func(paved *Paved) // PaveObject paves a runtime.Object, making it possible to get and set values @@ -361,13 +362,13 @@ func (p *Paved) setValue(s Segments, value any) error { // any per https://golang.org/pkg/encoding/json/#Unmarshal. We // marshal our value to JSON and unmarshal it into an any to ensure // it meets these criteria before setting it within p.object. - var v any - j, err := json.Marshal(value) + v, err := toValidJSON(value) if err != nil { - return errors.Wrap(err, "cannot marshal value to JSON") + return err } - if err := json.Unmarshal(j, &v); err != nil { - return errors.Wrap(err, "cannot unmarshal value from JSON") + + if err := p.validateSegments(s); err != nil { + return err } var in any = p.object @@ -381,10 +382,6 @@ func (p *Paved) setValue(s Segments, value any) error { return errors.Errorf("%s is not an array", s[:i]) } - if p.maxFieldPathIndexEnabled() && current.Index > p.maxFieldPathIndex { - return errors.Errorf("index %d is greater than max allowed index %d", current.Index, p.maxFieldPathIndex) - } - if final { array[current.Index] = v return nil @@ -412,6 +409,18 @@ func (p *Paved) setValue(s Segments, value any) error { return nil } +func toValidJSON(value any) (any, error) { + var v any + j, err := json.Marshal(value) + if err != nil { + return nil, errors.Wrap(err, "cannot marshal value to JSON") + } + if err := json.Unmarshal(j, &v); err != nil { + return nil, errors.Wrap(err, "cannot unmarshal value from JSON") + } + return v, nil +} + func prepareElement(array []any, current, next Segment) { // If this segment is not the final one and doesn't exist we need to // create it for our next segment. @@ -483,6 +492,18 @@ func (p *Paved) SetValue(path string, value any) error { return p.setValue(segments, value) } +func (p *Paved) validateSegments(s Segments) error { + if !p.maxFieldPathIndexEnabled() { + return nil + } + for _, segment := range s { + if segment.Type == SegmentIndex && segment.Index > p.maxFieldPathIndex { + return errors.Errorf("index %v is greater than max allowed index %d", segment.Index, p.maxFieldPathIndex) + } + } + return nil +} + // SetString value at the supplied field path. func (p *Paved) SetString(path, value string) error { return p.SetValue(path, value) diff --git a/pkg/fieldpath/paved_test.go b/pkg/fieldpath/paved_test.go index ab0dbd9ee..37ceaf0ce 100644 --- a/pkg/fieldpath/paved_test.go +++ b/pkg/fieldpath/paved_test.go @@ -743,30 +743,30 @@ func TestSetValue(t *testing.T) { reason: "Paths having indexes above the maximum default value are rejected", data: []byte(`{"data":["a"]}`), args: args{ - path: fmt.Sprintf("data[%v]", MaxFieldPathIndex+1), + path: fmt.Sprintf("data[%v]", DefaultMaxFieldPathIndex+1), value: "c", }, want: want{ object: map[string]any{ "data": []any{"a"}}, - err: errors.Wrap(errors.Errorf("found index above max (%[1]v > %[2]v): data[%[1]v]", - MaxFieldPathIndex+1, MaxFieldPathIndex), "invalid segments"), + err: errors.Errorf("index %v is greater than max allowed index %v", + DefaultMaxFieldPathIndex+1, DefaultMaxFieldPathIndex), }, }, "NotRejectsHighIndexesIfNoDefaultOptions": { reason: "Paths having indexes above the maximum default value are not rejected if default disabled", data: []byte(`{"data":["a"]}`), args: args{ - path: fmt.Sprintf("data[%v]", MaxFieldPathIndex+1), + path: fmt.Sprintf("data[%v]", DefaultMaxFieldPathIndex+1), value: "c", - opts: []PavedOption{}, + opts: []PavedOption{WithMaxFieldPathIndex(0)}, }, want: want{ object: map[string]any{ "data": func() []any { - res := make([]any, MaxFieldPathIndex+2) + res := make([]any, DefaultMaxFieldPathIndex+2) res[0] = "a" - res[MaxFieldPathIndex+1] = "c" + res[DefaultMaxFieldPathIndex+1] = "c" return res }()}, },