From a4ae0ea12f720ca2477292f4677075b50a5a4fb1 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 6 May 2021 09:39:52 +0100 Subject: [PATCH 1/4] [ShareJsUpdateManager] double check doc size before flushing --- .../app/js/ShareJsUpdateManager.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/services/document-updater/app/js/ShareJsUpdateManager.js b/services/document-updater/app/js/ShareJsUpdateManager.js index 607ae2d9fa..8ae91df32c 100644 --- a/services/document-updater/app/js/ShareJsUpdateManager.js +++ b/services/document-updater/app/js/ShareJsUpdateManager.js @@ -87,6 +87,20 @@ module.exports = ShareJsUpdateManager = { if (error != null) { return callback(error) } + const docSizeAfter = data.snapshot.length + if (docSizeAfter > Settings.max_doc_length) { + const docSizeBefore = lines.join('\n').length + const err = new Error( + 'blocking persistence of ShareJs update: doc size exceeds limits' + ) + logger.error( + { project_id, doc_id, err, docSizeBefore, docSizeAfter }, + err.message + ) + metrics.inc('sharejs.other-error') + const publicError = 'Update takes doc over max doc size' + return callback(publicError) + } // only check hash when present and no other updates have been applied if (update.hash != null && incomingUpdateVersion === version) { const ourHash = ShareJsUpdateManager._computeHash(data.snapshot) From c707d0b345da315e0b537c3d95ac4fd5b6ac4daa Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 6 May 2021 17:19:23 +0100 Subject: [PATCH 2/4] [RedisManager] block inserting of too large docs into redis --- services/document-updater/app/js/RedisManager.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/services/document-updater/app/js/RedisManager.js b/services/document-updater/app/js/RedisManager.js index 73d85f60d5..d81c7151b3 100644 --- a/services/document-updater/app/js/RedisManager.js +++ b/services/document-updater/app/js/RedisManager.js @@ -72,6 +72,13 @@ module.exports = RedisManager = { logger.error({ err: error, doc_id, docLines }, error.message) return callback(error) } + // Do a cheap size check on the serialized blob. + if (docLines.length > Settings.max_doc_length) { + const docSize = docLines.length + const err = new Error('blocking doc insert into redis: doc is too large') + logger.error({ project_id, doc_id, err, docSize }, err.message) + return callback(err) + } const docHash = RedisManager._computeHash(docLines) // record bytes sent to redis metrics.summary('redis.docLines', docLines.length, { status: 'set' }) From 757e1e98c519961ccd036af7d7ce7f1dd2478118 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 6 May 2021 17:30:50 +0100 Subject: [PATCH 3/4] [RedisManager] block updating of too large docs in redis --- services/document-updater/app/js/RedisManager.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/services/document-updater/app/js/RedisManager.js b/services/document-updater/app/js/RedisManager.js index d81c7151b3..418e3ec6d4 100644 --- a/services/document-updater/app/js/RedisManager.js +++ b/services/document-updater/app/js/RedisManager.js @@ -468,6 +468,13 @@ module.exports = RedisManager = { logger.error({ err: error, doc_id, newDocLines }, error.message) return callback(error) } + // Do a cheap size check on the serialized blob. + if (newDocLines.length > Settings.max_doc_length) { + const err = new Error('blocking doc update: doc is too large') + const docSize = newDocLines.length + logger.error({ project_id, doc_id, err, docSize }, err.message) + return callback(err) + } const newHash = RedisManager._computeHash(newDocLines) const opVersions = appliedOps.map((op) => (op != null ? op.v : undefined)) From 309ad818f69ffc71f46c8299d21a0db95d4a5538 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 6 May 2021 18:11:52 +0100 Subject: [PATCH 4/4] [SizeCheckTests] add acceptance tests for size limit checks --- .../test/acceptance/js/SizeCheckTests.js | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 services/document-updater/test/acceptance/js/SizeCheckTests.js diff --git a/services/document-updater/test/acceptance/js/SizeCheckTests.js b/services/document-updater/test/acceptance/js/SizeCheckTests.js new file mode 100644 index 0000000000..288cc485e1 --- /dev/null +++ b/services/document-updater/test/acceptance/js/SizeCheckTests.js @@ -0,0 +1,129 @@ +const { expect } = require('chai') +const Settings = require('settings-sharelatex') + +const MockWebApi = require('./helpers/MockWebApi') +const DocUpdaterClient = require('./helpers/DocUpdaterClient') +const DocUpdaterApp = require('./helpers/DocUpdaterApp') + +describe('SizeChecks', function () { + before(function (done) { + DocUpdaterApp.ensureRunning(done) + }) + beforeEach(function () { + this.version = 0 + this.update = { + doc: this.doc_id, + op: [ + { + i: 'insert some more lines that will bring it above the limit\n', + p: 42 + } + ], + v: this.version + } + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() + }) + + describe('when a doc is above the doc size limit already', function () { + beforeEach(function () { + this.lines = ['0123456789'.repeat(Settings.max_doc_length / 10 + 1)] + MockWebApi.insertDoc(this.project_id, this.doc_id, { + lines: this.lines, + v: this.version + }) + }) + + it('should error when fetching the doc', function (done) { + DocUpdaterClient.getDoc(this.project_id, this.doc_id, (error, res) => { + if (error) return done(error) + expect(res.statusCode).to.equal(500) + done() + }) + }) + + describe('when trying to update', function () { + beforeEach(function (done) { + const update = { + doc: this.doc_id, + op: this.update.op, + v: this.version + } + DocUpdaterClient.sendUpdate( + this.project_id, + this.doc_id, + update, + (error) => { + if (error != null) { + throw error + } + setTimeout(done, 200) + } + ) + }) + + it('should still error when fetching the doc', function (done) { + DocUpdaterClient.getDoc(this.project_id, this.doc_id, (error, res) => { + if (error) return done(error) + expect(res.statusCode).to.equal(500) + done() + }) + }) + }) + }) + + describe('when a doc is just below the doc size limit', function () { + beforeEach(function () { + this.lines = ['0123456789'.repeat(Settings.max_doc_length / 10 - 1)] + MockWebApi.insertDoc(this.project_id, this.doc_id, { + lines: this.lines, + v: this.version + }) + }) + + it('should be able to fetch the doc', function (done) { + DocUpdaterClient.getDoc( + this.project_id, + this.doc_id, + (error, res, doc) => { + if (error) return done(error) + expect(doc.lines).to.deep.equal(this.lines) + done() + } + ) + }) + + describe('when trying to update', function () { + beforeEach(function (done) { + const update = { + doc: this.doc_id, + op: this.update.op, + v: this.version + } + DocUpdaterClient.sendUpdate( + this.project_id, + this.doc_id, + update, + (error) => { + if (error != null) { + throw error + } + setTimeout(done, 200) + } + ) + }) + + it('should not update the doc', function (done) { + DocUpdaterClient.getDoc( + this.project_id, + this.doc_id, + (error, res, doc) => { + if (error) return done(error) + expect(doc.lines).to.deep.equal(this.lines) + done() + } + ) + }) + }) + }) +})