diff --git a/v5/patch.go b/v5/patch.go index 83102e5..2cdaa18 100644 --- a/v5/patch.go +++ b/v5/patch.go @@ -848,16 +848,16 @@ func ensurePathExists(pd *container, path string, options *ApplyOptions) error { if target == nil || ok != nil { - // If the current container is an array which has fewer elements than our target index, - // pad the current container with nulls. + // If the current container is an array, the index must be one the + // array already reaches. RFC 6902, section 4.1 allows an index at + // most equal to the number of elements, and padding the array with + // nulls to reach a larger one lets a patch of constant size choose + // how much memory and CPU this costs. if arrIndex, err = strconv.Atoi(part); err == nil { pa, ok := doc.(*partialArray) if ok && arrIndex >= len(pa.nodes)+1 { - // Pad the array with null values up to the required index. - for i := len(pa.nodes); i <= arrIndex-1; i++ { - doc.add(strconv.Itoa(i), newLazyNode(newRawMessage(rawJSONNull)), options) - } + return fmt.Errorf("Unable to ensure path for out of range index: %d: %w", arrIndex, ErrInvalidIndex) } } @@ -877,14 +877,16 @@ func ensurePathExists(pd *container, path string, options *ApplyOptions) error { arrIndex = 0 } + // The array is created empty, so only its first element is + // reachable. Padding it with nulls to reach a larger index has + // the same unbounded cost as above. + if arrIndex > 0 { + return fmt.Errorf("Unable to ensure path for out of range index: %d: %w", arrIndex, ErrInvalidIndex) + } + newNode := newLazyNode(newRawMessage(rawJSONArray)) doc.add(part, newNode, options) doc, _ = newNode.intoAry() - - // Pad the new array with null values up to the required index. - for i := 0; i < arrIndex; i++ { - doc.add(strconv.Itoa(i), newLazyNode(newRawMessage(rawJSONNull)), options) - } } else { newNode := newLazyNode(newRawMessage(rawJSONObject)) diff --git a/v5/patch_test.go b/v5/patch_test.go index 1f807f5..e4834ff 100644 --- a/v5/patch_test.go +++ b/v5/patch_test.go @@ -3,6 +3,7 @@ package jsonpatch import ( "bytes" "encoding/json" + "errors" "fmt" "reflect" "testing" @@ -498,13 +499,6 @@ var Cases = []Case{ false, true, }, - { - `{}`, - `[ { "op": "add", "path": "/a/b/3", "value": "hello" } ]`, - `{"a": {"b": [null, null, null, "hello"] } }`, - false, - true, - }, { `{"a": []}`, `[ { "op": "add", "path": "/a/-1", "value": "hello" } ]`, @@ -526,13 +520,6 @@ var Cases = []Case{ false, true, }, - { - `{"a": [{"b": "whatever"}]}`, - `[ { "op": "add", "path": "/a/2/b/c", "value": "hello" } ]`, - `{"a": [{"b": "whatever"}, null, {"b": {"c": "hello"}}]}`, - false, - true, - }, { `{"a": [{"b": "whatever"}]}`, `[ { "op": "add", "path": "/a/1/b/c", "value": "hello" } ]`, @@ -1245,3 +1232,67 @@ func init() { "foo": &msg, } } + +// TestEnsurePathExistsOnAddRejectsOutOfRangeIndex covers the array indices +// EnsurePathExistsOnAdd creates a path for. RFC 6902, section 4.1 allows an +// index at most equal to the number of elements in the array, and reaching a +// larger one used to pad the array with nulls one element at a time. The cost +// of that is chosen by the patch rather than bounded by its size: the last case +// here is a 46 byte patch that allocated tens of gigabytes and ran for minutes +// before it was rejected. +func TestEnsurePathExistsOnAddRejectsOutOfRangeIndex(t *testing.T) { + options := NewApplyOptions() + options.EnsurePathExistsOnAdd = true + + t.Run("rejected", func(t *testing.T) { + for _, c := range []struct{ name, doc, patch string }{ + {"beyond a new array", `{}`, + `[ { "op": "add", "path": "/a/b/3", "value": "hello" } ]`}, + {"beyond an existing array", `{"a": [{"b": "whatever"}]}`, + `[ { "op": "add", "path": "/a/2/b/c", "value": "hello" } ]`}, + {"beyond an empty array", `{"a": []}`, + `[ { "op": "add", "path": "/a/1/b", "value": "hello" } ]`}, + {"large index", `{}`, + `[ { "op": "add", "path": "/a/2000000000", "value": 1 } ]`}, + } { + t.Run(c.name, func(t *testing.T) { + _, err := applyPatchWithOptions(c.doc, c.patch, options) + if !errors.Is(err, ErrInvalidIndex) { + t.Errorf("expected ErrInvalidIndex, got %v", err) + } + }) + } + }) + + // Every index the array does reach still works. + t.Run("accepted", func(t *testing.T) { + for _, c := range []struct{ name, doc, patch, result string }{ + {"first element of a new array", `{}`, + `[ { "op": "add", "path": "/a/0/b", "value": "hello" } ]`, + `{"a": [{"b": "hello"}]}`}, + {"append to a new array", `{}`, + `[ { "op": "add", "path": "/a/-", "value": "hello" } ]`, + `{"a": ["hello"]}`}, + {"at the length of an existing array", `{"a": [{"b": "whatever"}]}`, + `[ { "op": "add", "path": "/a/1/b/c", "value": "hello" } ]`, + `{"a": [{"b": "whatever"}, {"b": {"c": "hello"}}]}`}, + {"inside an existing element", `{"a": [{}]}`, + `[ { "op": "add", "path": "/a/0/b/c", "value": "hello" } ]`, + `{"a": [{"b": {"c": "hello"}}]}`}, + {"negative index", `{"a": []}`, + `[ { "op": "add", "path": "/a/-1", "value": "hello" } ]`, + `{"a": ["hello"]}`}, + } { + t.Run(c.name, func(t *testing.T) { + out, err := applyPatchWithOptions(c.doc, c.patch, options) + if err != nil { + t.Fatalf("unable to apply patch: %s", err) + } + if !compareJSON(out, c.result) { + t.Errorf("patch did not apply. Expected:\n%s\n\nActual:\n%s", + reformatJSON(c.result), reformatJSON(out)) + } + }) + } + }) +}