diff --git a/lib/ot.js b/lib/ot.js index 5cf349f6..4618598b 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,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 @@ -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); 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/submit-request.js b/lib/submit-request.js index d2cac8d9..afcff7ac 100644 --- a/lib/submit-request.js +++ b/lib/submit-request.js @@ -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 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/backend.js b/test/backend.js index e0af1256..8ab84c66 100644 --- a/test/backend.js +++ b/test/backend.js @@ -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}]}; stream.on('data', function(data) { expect(data.op).to.eql(op.op); done(); @@ -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); }); @@ -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 }); diff --git a/test/client/doc.js b/test/client/doc.js index 3caa5ddf..55833ca2 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,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() { diff --git a/test/ot.js b/test/ot.js index 0b524613..e4e6774a 100644 --- a/test/ot.js +++ b/test/ot.js @@ -125,6 +125,106 @@ 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('checkOpForType', function() { + it('rejects an array-like json0 op', function() { + var op = {op: {0: {p: ['colour'], oi: 'red'}, length: 1}}; + var error = ot.checkOpForType(type.uri, op); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_BADLY_FORMED); + }); + + it('rejects a json0 op that is not an array', function() { + var error = ot.checkOpForType(type.uri, {op: {p: ['colour'], oi: 'red'}}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_BADLY_FORMED); + }); + + it('rejects a json0 op component that is not an object', function() { + var error = ot.checkOpForType(type.uri, {op: [null]}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + expect(error.message).to.equal('Missing path'); + }); + + it('rejects a dangerous path segment', function() { + var error = ot.checkOpForType(type.uri, {op: [{p: ['__proto__', 'x'], oi: 'yes'}]}); + expect(error.code).to.equal(ERROR_CODE.ERR_OT_OP_NOT_APPLIED); + expect(error.message).to.equal('Invalid path segment'); + }); + + it('accepts a valid json0 op', function() { + expect(ot.checkOpForType(type.uri, {op: [{p: ['colour'], oi: 'red'}]})).equal(); + }); + + it('leaves ops for other types alone', function() { + expect(ot.checkOpForType(presenceType.uri, {op: {index: 0, value: 'hi'}})).equal(); + }); + + it('leaves creates and deletes alone', function() { + expect(ot.checkOpForType(type.uri, {create: {type: type.uri}})).equal(); + expect(ot.checkOpForType(type.uri, {del: true})).equal(); + }); + }); + describe('no-op', function() { it('works on existing docs', function() { var doc = {v: 6, type: type.uri, data: 'Hi'}; @@ -374,6 +474,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); + }); + }); + }); });