applyPatch accepts invalid array index tokens and writes to the wrong position

[MildlyMeticulous]

applyPatch accepts array index tokens that RFC 6901 forbids, and writes them to the wrong position instead of returning an error. Negative, leading-zero, fractional and out-of-range indices all apply silently.

import {applyPatch} from 'rfc6902'   // 5.3.0

const doc = {bar: [1, 2]}
applyPatch(doc, [{op: 'add', path: '/bar/-1', value: 'X'}])
// returns [null]  ->  {bar: [1, 'X', 2]}

-1 is not a valid array index. RFC 6901 section 4 defines the token as "0" / ( %x31-39 *DIGIT ), and RFC 6902 adds - for append. -1 is neither, so this should be an error. Instead the value lands at index 1, which is a position the patch never named.

What gets through

patch	document	result	expected
add /bar/-1	{bar:[1,2]}	{bar:[1,"X",2]}	error
add /bar/-2	{bar:[1,2,3]}	{bar:[1,"X",2,3]}	error
add /bar/00	{bar:[1,2]}	{bar:["X",1,2]}	error
add /bar/1.5	{bar:[1,2]}	{bar:[1,"X",2]}	error
add /bar/1e0	{bar:[1,2]}	{bar:[1,"X",2]}	error
add /bar/8	{bar:[1,2]}	{bar:[1,2,"X"]}	error, index > length
add /bar	["foo","sil"]	[42,"foo","sil"]	error, object op on array
add /- (no value)	[1]	[1,null]	error, value is required
replace /0 (no value)	[1]	[null]	error, value is required

replace /bar/-1 is a quieter version of the same thing: it returns [null] and changes nothing, so a caller checking only the error list is told the replace succeeded when the document was untouched.

Where it comes from

Every row above falls out of converting the token with Number() and handing it to Array.prototype.splice with no validation and no bounds check:

const splice = (tok, arr) => { const a = [...arr]; a.splice(Number(tok), 0, 'X'); return a }

splice('-1',  [1,2])          // [1,'X',2]        splice counts from the end
splice('-2',  [1,2,3])        // [1,'X',2,3]
splice('00',  [1,2])          // ['X',1,2]        Number('00') === 0
splice('1.5', [1,2])          // [1,'X',2]        truncated
splice('1e0', [1,2])          // [1,'X',2]
splice('8',   [1,2])          // [1,2,'X']        clamped to length
splice('bar', ['foo','sil'])  // ['X','foo','sil'] NaN becomes 0

The missing-value rows look like the same shape: value is read as undefined and stored, then serialises as null. RFC 6902 sections 4.1 and 4.3 make value a required member of add and replace.

Why it matters

JSON Patch is usually applied to documents from a client, so the patch is untrusted input. A peer that validates the pointer rejects /bar/-1; this applies it at a different index. Two services using different implementations end up with different documents from the same request, and neither reports a problem.

Suggestion

The community suite at json-patch/json-patch-tests covers all of these. test/spec.yaml here is a separate set, so those cases are not currently exercised. Running the two together would catch this class. For reference, on the 108 cases in that suite (tests.json plus spec_tests.json) I get 97 passing.

I left root-pointer behaviour out of the table. applyPatch mutates in place so it cannot replace the root, and #107 shows returning MissingError there is deliberate, even though the suite counts those as failures.

Checked against 5.3.0 on Node 24, comparing object results by deep equality rather than key order.

[chbrown]

I added tests for json-patch/json-patch-tests, and fixes for most of what was broken.

Can you verify that HEAD (≈ 6299d30) resolves this? Thanks.

[MildlyMeticulous]

Verified on 6299d30: 13 of the 19 cases from the report now error correctly. add is fixed by fd1c264, and remove is covered too, since its endpoint.value === undefined check rejects these before _remove runs.

Six are still accepted, all in replace, copy, and move:

applyPatch({bar: [1, 2]}, [{op: 'replace', path: '/bar/-1', value: 'X'}])
// [null], and doc.bar['-1'] === 'X' -- a non-index property, invisible to JSON.stringify

applyPatch({bar: [1, 2], src: 'S'}, [{op: 'copy', from: '/src', path: '/bar/-1'}])
// doc.bar === [1, 'S', 2]

replace uses parseInt(key) >= length, which is false for both NaN and -1, so it falls through to parent[key] = value. copy and move call _add with no check, so they still reach the original splice(Number(key), 0, value).

#114 extracts the check add already does into two helpers and applies it at all four sites, with 32 tests.