Skip to content

🔒️ Stop json0 ops bypassing the prototype pollution guard #722

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
alecgibson wants to merge 1 commit into from
+309 −22
Closed

🔒️ Stop json0 ops bypassing the prototype pollution guard #722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 55 additions & 12 deletions lib/ot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,18 +108,9 @@ function applyOpEdit(snapshot, edit) {
var type = types.map[snapshot.type];
if (!type) return new ShareDBError(ERROR_CODE.ERR_DOC_TYPE_NOT_RECOGNIZED, 'Unknown type');

if (type.name === 'json0' && Array.isArray(edit)) {
for (var i = 0; i < edit.length; i++) {
var opComponent = edit[i];
if (Array.isArray(opComponent.p)) {
for (var j = 0; j < opComponent.p.length; j++) {
var pathSegment = opComponent.p[j];
if (util.isDangerousProperty(pathSegment)) {
return new ShareDBError(ERROR_CODE.ERR_OT_OP_NOT_APPLIED, 'Invalid path segment');
}
}
}
}
if (type.name === 'json0') {
var pathError = checkJson0OpPaths(edit);
if (pathError) return pathError;
}

try {
Expand All @@ -129,6 +120,51 @@ function applyOpEdit(snapshot, edit) {
}
}

// ot-json0 walks ops with .length and numeric indexing, so it treats array-like
// objects as ops too, and it coerces .length. This traversal has to match it
// exactly: anything ot-json0 will apply has to be checked here.
// See GHSA-9rqw-j2q5-gg2g
function checkJson0OpPaths(op) {
if (op == null) return;
for (var i = 0; i < op.length; i++) {
var component = op[i];
// ot-json0's checkValidOp() rejects the whole op before applying any of it,
// so there is nothing beyond this component left to check
if (!component || typeof component !== 'object' || !Array.isArray(component.p)) return;
for (var j = 0; j < component.p.length; j++) {
if (util.isDangerousProperty(component.p[j])) {
return new ShareDBError(ERROR_CODE.ERR_OT_OP_NOT_APPLIED, 'Invalid path segment');
}
}
}
}

// Submitted ops are held to the shape ot-json0 documents, so that nothing
// array-like can be committed in the first place. Ops that are already
// committed only get their paths checked, since ops that ot-json0 quietly
// treats as no-ops were committable by older versions of ShareDB
function checkJson0Op(op) {
if (!Array.isArray(op)) {
return new ShareDBError(ERROR_CODE.ERR_OT_OP_BADLY_FORMED, 'json0 op must be an array');
}

for (var i = 0; i < op.length; i++) {
var component = op[i];
if (!component || typeof component !== 'object' || !Array.isArray(component.p)) {
return new ShareDBError(ERROR_CODE.ERR_OT_OP_NOT_APPLIED, 'Missing path');
}
}

return checkJson0OpPaths(op);
}

exports.checkOpForType = function(typeName, op) {
if (!('op' in op)) return;
var type = types.map[typeName];
if (!type || type.name !== 'json0') return;
return checkJson0Op(op.op);
};

exports.transform = function(type, op, appliedOp) {
// There are 16 cases this function needs to deal with - which are all the
// combinations of create/delete/op/noop from both op and appliedOp
Expand Down Expand Up @@ -178,6 +214,13 @@ exports.applyOps = function(snapshot, ops, options) {
options = options || {};
for (var index = 0; index < ops.length; index++) {
var op = ops[index];
// 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;
}
if (options._normalizeLegacyJson0Ops) {
try {
normalizeLegacyJson0Ops(snapshot, op);
Expand Down
6 changes: 3 additions & 3 deletions lib/projections.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ function projectEdit(fields, op) {
}
} else {
// The path has a first element. Just check it against the fields.
if (fields[path[0]]) {
if (util.hasOwn(fields, path[0])) {
result.push(c);
}
}
Expand Down Expand Up @@ -91,7 +91,7 @@ function isSnapshotAllowed(fields, snapshot) {
return false;
}
for (var k in snapshot.data) {
if (!fields[k]) return false;
if (!util.hasOwn(fields, k)) return false;
}
return true;
}
Expand All @@ -101,7 +101,7 @@ function isEditAllowed(fields, op) {
var c = op[i];
if (c.p.length === 0) {
return false;
} else if (!fields[c.p[0]]) {
} else if (!util.hasOwn(fields, c.p[0])) {
return false;
}
}
Expand Down
6 changes: 6 additions & 0 deletions lib/submit-request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,12 @@ SubmitRequest.prototype.submit = function(callback) {
request.snapshot = snapshot;
request._addSnapshotMeta();

// The type is only known once we have the snapshot, so this is the earliest
// we can validate the op against it. It has to happen before the op reaches
// any type function, including through $fixup() in the apply middleware
var opError = ot.checkOpForType(snapshot.type, op);
if (opError) return callback(opError);

if (op.v == null) {
if (op.create && snapshot.type && op.src) {
// If the document was already created by another op, we will return a
Expand Down
7 changes: 3 additions & 4 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,13 @@ exports.clone = function(obj) {
return (obj === undefined) ? undefined : JSON.parse(JSON.stringify(obj));
};

// A null prototype makes '__proto__' an ordinary own key, so it can be in the map
var objectProtoPropNames = Object.create(null);
Object.getOwnPropertyNames(Object.prototype).forEach(function(prop) {
if (prop !== '__proto__') {
objectProtoPropNames[prop] = true;
}
objectProtoPropNames[prop] = true;
});
exports.isDangerousProperty = function(propName) {
return propName === '__proto__' || objectProtoPropNames[propName];
return objectProtoPropNames[propName] === true;
};

try {
Expand Down
6 changes: 3 additions & 3 deletions test/backend.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ describe('Backend', function() {
title: '1984',
author: 'George Orwell'
});
var op = {op: {p: ['publication'], oi: 1949}};
var op = {op: [{p: ['publication'], oi: 1949}]};

@alecgibson alecgibson Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alecgibson alecgibson Sep 4, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

stream.on('data', function(data) {
expect(data.op).to.eql(op.op);
done();
Expand Down Expand Up @@ -245,7 +245,7 @@ describe('Backend', function() {
done();
});

var op = {op: {p: ['publicationYear'], oi: 1949}};
var op = {op: [{p: ['publicationYear'], oi: 1949}]};
backend.submit(agent, 'books', '1984', op, null, function(error) {
if (error) done(error);
});
Expand All @@ -262,7 +262,7 @@ describe('Backend', function() {
done();
});

var op = {op: {p: ['publicationYear'], oi: 1949}};
var op = {op: [{p: ['publicationYear'], oi: 1949}]};
backend.submit(agent, 'books', '1984', op, null, function() {
// Swallow the error
});
Expand Down
74 changes: 74 additions & 0 deletions test/client/doc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,10 @@ describe('Doc', function() {
});
}

afterEach(function() {
delete Object.prototype.polluted;
});

['__proto__', 'constructor'].forEach(function(badProp) {
it('Rejects ops with collection ' + badProp, function(done) {
var collectionName = badProp;
Expand Down Expand Up @@ -755,6 +759,76 @@ describe('Doc', function() {
});
});
});

[
{
name: 'an array-like op',
op: {0: {p: ['__proto__', 'polluted'], oi: 'oops'}, length: 1},
error: 'json0 op must be an array'
},
{
name: 'ops with a path segment that is not a string',
op: [{p: [['__proto__'], 'polluted'], oi: 'oops'}],
error: 'Invalid path segment'
},
{
name: 'ops with a component that is not an object',
op: [null],
error: 'Missing path'
}
].forEach(function(test) {
it('Rejects ' + test.name, function(done) {
var connection = this.connection;
var collectionName = 'test-collection';
var docId = 'test-doc';
connection.get(collectionName, docId).create({id: docId}, function(err) {
if (err) {
return done(err);
}
expectReceiveError(connection, collectionName, docId, test.error, function(error) {
if (error) {
return done(error);
}
expect({}.polluted).to.equal(undefined);
done();
});
connection.send({
a: 'op',
c: collectionName,
d: docId,
v: 1,
seq: connection.seq++,
x: {},
op: test.op
});
});
});
});

it('Rejects an array-like op before the apply middleware can fix it up', function(done) {
var connection = this.connection;
var collectionName = 'test-collection';
var docId = 'test-doc';
this.backend.use('apply', function(request, next) {
if ('op' in request.op) request.$fixup([{p: ['fixed'], oi: true}]);
next();
});
connection.get(collectionName, docId).create({id: docId}, function(err) {
if (err) {
return done(err);
}
expectReceiveError(connection, collectionName, docId, 'json0 op must be an array', done);
connection.send({
a: 'op',
c: collectionName,
d: docId,
v: 1,
seq: connection.seq++,
x: {},
op: {0: {p: ['colour'], oi: 'red'}, length: 1}
});
});
});
});

describe('toSnapshot', function() {
Expand Down
Loading
Loading