v5: reject out-of-range array indices instead of padding with nulls

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))
 

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))
+				}
+			})
+		}
+	})
+}