🔒️ Stop json0 ops bypassing the prototype pollution guard - #724
Conversation
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 two 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 bought us nothing. It just
means the lookup, which coerces its key, missed anything that
stringifies to `__proto__`, so `[{p: [['__proto__'], 'x'], oi: 1}]`
was accepted with no array-like trickery at all.
- `normalizeLegacyJson0Ops()` applies op components itself, before
`apply()` guards anything, so `applyOps()` polluted and *then*
returned `Invalid path segment`. That one needs no array-like op
either, and is reachable through `fetchSnapshot()`.
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`, since `{"length": "1"}` applies too — and
runs it everywhere an op is applied, including the legacy
normalisation. Anything `ot-json0` will apply is now checked.
It also stops the guard crashing the process on its own: reading
`opComponent.p` threw an uncaught `TypeError` for `op: [null]`, from
inside a `backend.trigger()` callback. The scan now stops at the first
component `ot-json0` would reject, and leaves it to complain, which it
does inside the existing `try`/`catch`. Stopping rather than skipping
past it also matters because `{"length": 1e9}` would otherwise spin.
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 go through `isFieldAllowed()`, which checks the field
is an own property as well as truthy, so a falsey field value keeps
behaving as it did.
Note this deliberately doesn't police the *shape* of an op, only its
paths, so no op that used to apply stops applying. `ot-json0` quietly
treats a non-array as a no-op, and `backend.submit()` accepts those, so
they exist in real op histories — rejecting them here would make those
documents permanently unreadable through `fetchSnapshot()`, which is
exactly what `normalizeLegacyJson0Ops()` exists to avoid. Those shapes
are a problem for a different reason, handled separately.
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](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
78d308e to
9cfe991
Compare
|
@dawidreedsy I'm going to go ahead and release this since it's a security issue, but could you please sense-check it when you're back from leave? |
There was a problem hiding this comment.
🟡 Changes recommended
New code in applyOps() uses 'op' in op, which can be influenced by prototype inheritance/pollution and should use an own-property check in this security-sensitive path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens ShareDB’s json0 operational transform path-segment guard to prevent prototype pollution via array-like ops and legacy op normalization, aligning ShareDB’s validation with ot-json0’s traversal behavior.
Changes:
- Scan json0 ops for dangerous path segments using
op.length+ numeric indexing (matchingot-json0), including string-coercedlength. - Ensure the path guard runs in all op-application paths, including legacy json0 normalization in
applyOps(). - Fix projection allow-list checks to use own-properties (
util.hasOwn) and add tests covering falsy and prototype-inherited field names.
File summaries
| File | Description |
|---|---|
| lib/ot.js | Adds json0 path traversal guard matching ot-json0 and applies it during legacy normalization. |
| lib/util.js | Adjusts dangerous-property detection to include __proto__ in the prototype property-name map. |
| lib/projections.js | Uses util.hasOwn-based checks for projection field allow-listing to avoid prototype-inherited keys. |
| test/ot.js | Adds regression tests covering array-like ops, non-string path segments, and legacy normalization pollution. |
| test/client/doc.js | Adds client/server integration coverage ensuring server rejects array-like / invalid-segment ops without polluting prototypes. |
| test/projections.js | Adds tests for falsy field values and Object.prototype-inherited field names in projection logic. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // normalizeLegacyJson0Ops() applies op components itself, so paths have to | ||
| // be checked before it runs, not just in exports.apply() | ||
| var type = types.map[snapshot.type]; | ||
| if (type && type.name === 'json0' && 'op' in op) { | ||
| var pathError = checkJson0OpPaths(op.op); | ||
| if (pathError) return pathError; | ||
| } |
Refs GHSA-9rqw-j2q5-gg2g
At the moment,
applyOpEdit()only scans an op's paths for dangerous segments ifArray.isArray(edit).ot-json0isn't so fussy: bothcheckValidOp()andapply()walk the op withop.lengthand 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.prototypein the server process, for the lifetime of that process, with a single message: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 two more holes of its own:isDangerousProperty()builds its lookup map fromObject.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 bought us nothing. It just means the lookup, which coerces its key, missed anything that stringifies to__proto__, so[{p: [['__proto__'], 'x'], oi: 1}]was accepted with no array-like trickery at all.normalizeLegacyJson0Ops()applies op components itself, beforeapply()guards anything, soapplyOps()polluted and then returnedInvalid path segment. That one needs no array-like op either, and is reachable throughfetchSnapshot().This change makes the path scan traverse an op exactly the way
ot-json0does —.lengthand numeric indexing, including its coercion of a stringlength, since{"length": "1"}applies too — and runs it everywhere an op is applied, including the legacy normalisation. Anythingot-json0will apply is now checked.It also stops the guard crashing the process on its own: reading
opComponent.pthrew an uncaughtTypeErrorforop: [null], from inside abackend.trigger()callback. The scan now stops at the first componentot-json0would reject, and leaves it to complain, which it does inside the existingtry/catch. Stopping rather than skipping past it also matters because{"length": 1e9}would otherwise spin.Finally,
projectionstested its field allow-list with plain-object truthiness, sofields['__proto__']andfields['toString']were truthy, and a projection reported those segments as allowed fields. Those lookups now useutil.hasOwn().Note this deliberately doesn't police the shape of an op, only its paths, so no op that used to apply stops applying.
ot-json0quietly treats a non-array as a no-op, andbackend.submit()accepts those, so they exist in real op histories — rejecting them here would make those documents permanently unreadable throughfetchSnapshot(), which is exactly whatnormalizeLegacyJson0Ops()exists to avoid. Those shapes are a problem for a different reason, handled separately.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