diff --git a/lib/ot.js b/lib/ot.js index 5cf349f6..c142904f 100644 --- a/lib/ot.js +++ b/lib/ot.js @@ -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 { @@ -129,6 +120,25 @@ 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'); + } + } + } +} + 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 @@ -178,6 +188,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); diff --git a/lib/projections.js b/lib/projections.js index b9a43c20..f91a3341 100644 --- a/lib/projections.js +++ b/lib/projections.js @@ -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); } } @@ -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; } @@ -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; } } diff --git a/lib/util.js b/lib/util.js index 92235049..345fa50d 100644 --- a/lib/util.js +++ b/lib/util.js @@ -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 { diff --git a/test/client/doc.js b/test/client/doc.js index 3caa5ddf..f30258c0 100644 --- a/test/client/doc.js +++ b/test/client/doc.js @@ -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; @@ -755,6 +759,46 @@ describe('Doc', function() { }); }); }); + + // ot-json0 walks ops with .length and numeric indexing, so it applies an + // array-like object as if it were an op + [ + { + name: 'an array-like op', + op: {0: {p: ['__proto__', 'polluted'], oi: 'oops'}, length: 1} + }, + { + name: 'ops with a path segment that is not a string', + op: [{p: [['__proto__'], 'polluted'], oi: 'oops'}] + } + ].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, 'Invalid path segment', 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 + }); + }); + }); + }); }); describe('toSnapshot', function() { diff --git a/test/ot.js b/test/ot.js index 0b524613..6d093001 100644 --- a/test/ot.js +++ b/test/ot.js @@ -125,6 +125,68 @@ describe('ot', function() { }); }); + describe('json0 op validation', function() { + var snapshot; + + beforeEach(function() { + snapshot = {v: 6, type: type.uri, data: {colour: 'blue'}}; + }); + + afterEach(function() { + delete Object.prototype.polluted; + }); + + it('does not pollute the prototype through an array-like op', function() { + var op = {0: {p: ['__proto__', 'polluted'], oi: 'yes'}, length: 1}; + var error = ot.apply(snapshot, {v: 6, op: op}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + expect(error.message).to.equal('Invalid path segment'); + expect({}.polluted).to.equal(undefined); + }); + + it('does not pollute the prototype through an array-like op with a string length', function() { + var op = {0: {p: ['__proto__', 'polluted'], oi: 'yes'}, length: '1'}; + var error = ot.apply(snapshot, {v: 6, op: op}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + expect(error.message).to.equal('Invalid path segment'); + expect({}.polluted).to.equal(undefined); + }); + + it('does not pollute the prototype through a path segment that is not a string', function() { + var error = ot.apply(snapshot, {v: 6, op: [{p: [['__proto__'], 'polluted'], oi: 'yes'}]}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + expect(error.message).to.equal('Invalid path segment'); + expect({}.polluted).to.equal(undefined); + }); + + ['__proto__', 'constructor'].forEach(function(badProp) { + it('rejects ' + badProp + ' as a path segment', function() { + var error = ot.apply(snapshot, {v: 6, op: [{p: [badProp, 'polluted'], oi: 'yes'}]}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + expect(error.message).to.equal('Invalid path segment'); + expect({}.polluted).to.equal(undefined); + }); + }); + + it('returns an error for an op component that is not an object', function() { + var error; + expect(function() { + error = ot.apply(snapshot, {v: 6, op: [null]}); + }).to.not.throw(); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + }); + + it('returns an error for an array-like op whose components are missing', function() { + var error = ot.apply(snapshot, {v: 6, op: {length: 1e9}}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + }); + + it('accepts an op that ot-json0 treats as a no-op', function() { + expect(ot.apply(snapshot, {v: 6, op: {p: ['colour'], oi: 'red'}})).equal(); + expect(snapshot).to.eql({v: 7, type: type.uri, data: {colour: 'blue'}}); + }); + }); + describe('no-op', function() { it('works on existing docs', function() { var doc = {v: 6, type: type.uri, data: 'Hi'}; @@ -374,6 +436,55 @@ describe('ot', function() { describe('applyOps', function() { describe('with normalization turned on', function() { + afterEach(function() { + delete Object.prototype.polluted; + }); + + it('does not pollute the prototype while normalizing a legacy op', function() { + var snapshot = { + type: 'http://sharejs.org/types/JSONv0', + data: {title: 'Wee Free Men'} + }; + + var ops = [ + { + v: 1, + op: [ + {p: ['__proto__', 'polluted'], oi: 'yes'}, + {p: ['title'], od: 'Wee Free Men', oi: 'Nation'} + ] + } + ]; + + var error = ot.applyOps(snapshot, ops, { + _normalizeLegacyJson0Ops: true + }); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + expect(error.message).to.equal('Invalid path segment'); + expect({}.polluted).to.equal(undefined); + }); + + it('replays a legacy op that ot-json0 treats as a no-op', function() { + var snapshot = { + type: 'http://sharejs.org/types/JSONv0', + data: {title: 'Wee Free Men'} + }; + + var ops = [ + { + v: 1, + op: {p: ['title'], od: 'Wee Free Men', oi: 'Nation'} + } + ]; + + var error = ot.applyOps(snapshot, ops, { + _normalizeLegacyJson0Ops: true + }); + expect(error).to.be.undefined; + expect(snapshot.data).to.eql({title: 'Wee Free Men'}); + expect(snapshot.v).to.equal(2); + }); + it('applies an op to a snapshot', function() { var snapshot = { type: 'http://sharejs.org/types/JSONv0', diff --git a/test/projections.js b/test/projections.js index 57c85195..f4aaf881 100644 --- a/test/projections.js +++ b/test/projections.js @@ -385,4 +385,20 @@ describe('projection utility methods', function() { ); }); }); + + describe('field names inherited from Object.prototype', function() { + ['__proto__', 'constructor', 'toString'].forEach(function(badProp) { + it('does not treat ' + badProp + ' as a projected field', function() { + var op = {op: [{p: [badProp, 'x'], oi: 'oops'}]}; + expect(projections.isOpAllowed(null, {x: true}, op)).equal(false); + projections.projectOp({x: true}, op); + expect(op).eql({op: []}); + }); + + it('does not treat ' + badProp + ' as an allowed snapshot field', function() { + var data = JSON.parse('{"' + badProp + '": "oops"}'); + expect(projections.isSnapshotAllowed({x: true}, {type: type, data: data})).equal(false); + }); + }); + }); });