🔒️ Stop json0 ops bypassing the prototype pollution guard

[alecgibson]

Refs GHSA-9rqw-j2q5-gg2g

At the moment, applyOpEdit() only scans an op's paths for dangerous segments if Array.isArray(edit). ot-json0 isn't so fussy: both checkValidOp() and apply() walk the op with op.length and numeric indexing, so an array-like object is an op as far as the type is concerned, and isn't an op as far as the guard is concerned.

Any client that can submit an op can therefore write to Object.prototype in the server process, for the lifetime of that process, with a single message:

{"a":"op","c":"docs","d":"doc","v":1,"src":"c1","seq":1,
 "op":{"0":{"p":["__proto__","polluted"],"oi":"x"},"length":1}}

The server acks that like any other op, and the document data is unchanged, so nothing looks wrong from either end.

The root cause is that our validation and ot-json0's traversal disagree about what counts as an op, and the guard has three more holes of its own:

• isDangerousProperty() builds its lookup map from Object.getOwnPropertyNames(Object.prototype) but skips the __proto__ key. That map has a null prototype, so __proto__ is an ordinary own key there and the exclusion buys us nothing. It just means the lookup, which coerces its key, misses anything that stringifies to __proto__, so [{p: [['__proto__'], 'x'], oi: 1}] was accepted with no array-like trickery at all.
• op: [null] threw a TypeError out of the guard itself, from inside a backend.trigger() callback, which takes the process down.
• normalizeLegacyJson0Ops() applies op components itself, before apply() guards anything, so applyOps() polluted and then returned Invalid path segment.

This change makes the path scan traverse an op exactly the way ot-json0 does — .length and numeric indexing, including its coercion of a string length — and uses it everywhere an op is applied, including the legacy normalisation. Anything ot-json0 will apply is now checked.

Submitted ops are additionally held to the shape ot-json0 documents, and that happens in submit-request rather than in ot.apply(). It has to be before the op reaches any type function, because $fixup() in apply middleware composes the op, and ot-json0's compose() throws on an array-like, which is another way to take the process down. It can't go in checkOp(), which runs before we have the snapshot and so doesn't know the document's type.

Note the shape check deliberately doesn't apply to ops we've already committed. ot-json0 quietly treats a non-array as a no-op, and backend.submit() accepted those until now, so they exist in real op histories — three of our own tests submitted them. Rejecting them on replay would make those documents permanently unreadable through fetchSnapshot(), which is exactly what normalizeLegacyJson0Ops() exists to avoid. The path check, not the shape check, is what closes the pollution; the shape check is ingress hardening.

Finally, projections tested its field allow-list with plain-object truthiness, so fields['__proto__'] and fields['toString'] were truthy, and a projection reported those segments as allowed fields. Those lookups now use util.hasOwn().

This does change backend.submit(): a json0 op that isn't an array used to commit as a silent no-op and bump the version, and is now rejected with ERR_OT_OP_BADLY_FORMED. Clients are unaffected, since Doc._submit() runs type.normalize() first, which wraps a bare component into an array.

Note also that none of this defends a client against ops arriving from the server, which has no equivalent guard. That's tracked in #721

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

[coveralls]

coverage: 97.487% (+0.01%) from 97.474% — fix-json0-op-path-guard-bypass into master

[alecgibson]

Why did this test change? If this broke without the change, this is technically an API breakage which we should avoid if possible.

[alecgibson]

Good question, and you're right that it's an API breakage — so I've split it out and this PR is superseded. The pollution fix is #724, with no API change and this test untouched; the strictness that broke it is #725, stacked on top.

To answer it directly: the test broke because backend.submit() started rejecting a json0 op that isn't an array. It passed before because ot-json0's apply() walks an op with op.length and numeric indexing, so a bare component iterates zero times and the snapshot comes back untouched — the op committed, bumped the version and got published, having changed nothing. Which is to say the assertion here was only ever checking that an op with no effect was broadcast.

What makes it more than a tidy-up is that compose() and invert() don't share apply()'s tolerance:

compose({p: ['x'], oi: 1}, fixup)     -> dest.push is not a function
invert({p: ['x'], oi: 1})             -> op.slice is not a function

$fixup() composes, from apply middleware, where the throw is uncaught. So this exact op shape takes the process down on any server using fixups — I reproduced it on master with a three-line fixup middleware and one raw socket frame. No ShareDB client can send it (Doc._submit() normalises first), but nothing stops a hand-written one.

So it isn't avoidable while still accepting the op: the shape is either rejected, or it stays a remote crash for fixup users. #725 argues that case on its own merits and carries the three test/backend.js changes with it, so the advisory fix in #724 doesn't depend on the decision.

[alecgibson]

Superseded by #724 (the advisory fix, no API change) and #725 (the op-shape rejection, stacked on it) — see the review thread for why they're separable.