From 2aff40d6dea967af92658b836b0613e8412180ee Mon Sep 17 00:00:00 2001 From: Henning Perl Date: Fri, 31 Jul 2026 20:35:14 +0200 Subject: [PATCH] Reject out-of-range array indices in ensurePathExists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EnsurePathExistsOnAdd padded an array with nulls, one element at a time, to reach any index a patch named. Each append copies the array, so a ~60 byte operation such as [{"op":"add","path":"/a/2000000000","value":1}] costs O(index) allocations and O(index²) copies. There is no option to bound it, the loop checks no context, and a caller cannot see the index in advance without walking the patch itself. In a server that accepts patches from its clients this is a denial of service: 100 KB of such operations allocated tens of gigabytes and ran for minutes in our testing. RFC 6902, section 4.1 already requires that an array index be at most the number of elements in the array, so the padding was outside the specification to begin with. Both padding loops now return ErrInvalidIndex instead. Every index the array actually reaches still works: an existing element, the array's length (an append), "-", negative indices where they are enabled, and creating a fresh array for index 0. Two cases in Cases asserted the padding and now assert the rejection in a test of their own. --- v5/patch.go | 24 ++++++++------- v5/patch_test.go | 79 +++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 78 insertions(+), 25 deletions(-) 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)) + } + }) + } + }) +}