From 15244a54be5a26a6655476703e07759bba1a01ff Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Tue, 24 Mar 2020 09:12:12 +0100 Subject: [PATCH 1/4] [misc] WebsocketController: limit the update size to 7mb bail out early on -- especially do not push the update into redis for doc-updater to discard it. Confirm the update silently, otherwise the frontend will send it again. Broadcast a 'otUpdateError' message and disconnect the client, like doc-updater would do. --- .../app/coffee/WebsocketController.coffee | 18 +++++++ .../real-time/config/settings.defaults.coffee | 5 ++ .../acceptance/coffee/ApplyUpdateTests.coffee | 54 +++++++++++++++++++ .../coffee/WebsocketControllerTests.coffee | 32 +++++++++++ 4 files changed, 109 insertions(+) diff --git a/services/real-time/app/coffee/WebsocketController.coffee b/services/real-time/app/coffee/WebsocketController.coffee index d091c5e4ed..1e4ca5160c 100644 --- a/services/real-time/app/coffee/WebsocketController.coffee +++ b/services/real-time/app/coffee/WebsocketController.coffee @@ -1,5 +1,6 @@ logger = require "logger-sharelatex" metrics = require "metrics-sharelatex" +settings = require "settings-sharelatex" WebApiManager = require "./WebApiManager" AuthorizationManager = require "./AuthorizationManager" DocumentUpdaterManager = require "./DocumentUpdaterManager" @@ -202,6 +203,23 @@ module.exports = WebsocketController = Utils.getClientAttributes client, ["user_id", "project_id"], (error, {user_id, project_id}) -> return callback(error) if error? return callback(new Error("no project_id found on client")) if !project_id? + + updateSize = JSON.stringify(update).length + if updateSize > settings.maxUpdateSize + metrics.inc "update_too_large" + logger.warn({user_id, project_id, doc_id, updateSize}, "update is too large") + + # mark the update as received -- the client should not send it again! + callback() + + # trigger an out-of-sync error + message = {project_id, doc_id, error: "update is too large"} + setTimeout () -> + client.emit "otUpdateError", message.error, message + client.disconnect() + , 100 + return + WebsocketController._assertClientCanApplyUpdate client, doc_id, update, (error) -> if error? logger.warn {err: error, doc_id, client_id: client.id, version: update.v}, "client is not authorized to make update" diff --git a/services/real-time/config/settings.defaults.coffee b/services/real-time/config/settings.defaults.coffee index 5814530552..9ca2348b0b 100644 --- a/services/real-time/config/settings.defaults.coffee +++ b/services/real-time/config/settings.defaults.coffee @@ -52,6 +52,11 @@ settings = max_doc_length: 2 * 1024 * 1024 # 2mb + # combine + # max_doc_length (2mb see above) * 2 (delete + insert) + # max_ranges_size (3mb see MAX_RANGES_SIZE in doc-updater) + maxUpdateSize: parseInt(process.env['MAX_UPDATE_SIZE']) or 7 * 1024 * 1024 + shutdownDrainTimeWindow: process.env['SHUTDOWN_DRAIN_TIME_WINDOW'] or 9 continualPubsubTraffic: process.env['CONTINUAL_PUBSUB_TRAFFIC'] or false diff --git a/services/real-time/test/acceptance/coffee/ApplyUpdateTests.coffee b/services/real-time/test/acceptance/coffee/ApplyUpdateTests.coffee index 842e4fcc4c..2d52975d71 100644 --- a/services/real-time/test/acceptance/coffee/ApplyUpdateTests.coffee +++ b/services/real-time/test/acceptance/coffee/ApplyUpdateTests.coffee @@ -68,6 +68,60 @@ describe "applyOtUpdate", -> (cb) => rclient.del redisSettings.documentupdater.key_schema.pendingUpdates(@doc_id), cb ], done + describe "when authorized with a huge edit update", -> + before (done) -> + @update = { + op: {p: 12, t: "foo"}, + junk: 'this update is too large'.repeat(1024 * 300) # >7MB + } + async.series [ + (cb) => + FixturesManager.setUpProject { + privilegeLevel: "readAndWrite" + }, (e, {@project_id, @user_id}) => + cb(e) + + (cb) => + FixturesManager.setUpDoc @project_id, {@lines, @version, @ops}, (e, {@doc_id}) => + cb(e) + + (cb) => + @client = RealTimeClient.connect() + @client.on "connectionAccepted", cb + @client.on "otUpdateError", (@otUpdateError) => + + (cb) => + @client.emit "joinProject", project_id: @project_id, cb + + (cb) => + @client.emit "joinDoc", @doc_id, cb + + (cb) => + @client.emit "applyOtUpdate", @doc_id, @update, (@error) => + cb() + ], done + + it "should not return an error", -> + expect(@error).to.not.exist + + it "should send an otUpdateError to the client", (done) -> + setTimeout () => + expect(@otUpdateError).to.exist + done() + , 300 + + it "should disconnect the client", (done) -> + setTimeout () => + @client.socket.connected.should.equal false + done() + , 300 + + it "should not put the update in redis", (done) -> + rclient.llen redisSettings.documentupdater.key_schema.pendingUpdates({@doc_id}), (error, len) => + len.should.equal 0 + done() + return null + describe "when authorized to read-only with an edit update", -> before (done) -> async.series [ diff --git a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee index f2f1531834..b6b4520ced 100644 --- a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee +++ b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee @@ -32,6 +32,7 @@ describe 'WebsocketController', -> "./DocumentUpdaterManager": @DocumentUpdaterManager = {} "./ConnectedUsersManager": @ConnectedUsersManager = {} "./WebsocketLoadBalancer": @WebsocketLoadBalancer = {} + "settings-sharelatex": {maxUpdateSize: 7 * 1024 * 1024} "logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub(), warn: sinon.stub() } "metrics-sharelatex": @metrics = inc: sinon.stub() @@ -668,6 +669,37 @@ describe 'WebsocketController', -> it "should call the callback with the error", -> @callback.calledWith(@error).should.equal true + describe "update_too_large", -> + beforeEach (done) -> + @client.disconnect = sinon.stub() + @client.emit = sinon.stub() + @update = { + op: {p: 12, t: "foo"}, + junk: 'this update is too large'.repeat(1024 * 300) # >7MB + } + @client.params.user_id = @user_id + @client.params.project_id = @project_id + @WebsocketController.applyOtUpdate @client, @doc_id, @update, @callback + setTimeout -> + done() + , 201 + + it "should call the callback with no error", -> + @callback.called.should.equal true + @callback.args[0].should.deep.equal [] + + it "should log a warning with the size and context", -> + @logger.warn.called.should.equal true + @logger.warn.args[0].should.deep.equal [{ + @user_id, @project_id, @doc_id, updateSize: 7372835 + }, 'update is too large'] + + it "should send an otUpdateError the client", -> + @client.emit.calledWith('otUpdateError').should.equal true + + it "should disconnect the client", -> + @client.disconnect.called.should.equal true + describe "_assertClientCanApplyUpdate", -> beforeEach -> @edit_update = { op: [{i: "foo", p: 42}, {c: "bar", p: 132}] } # comments may still be in an edit op From cb675d38c2232a23f605683cc32adcbbbfe7bf52 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Tue, 24 Mar 2020 09:14:15 +0100 Subject: [PATCH 2/4] [misc] SafeJsonParse: align the size limit with the frontend->rt limit frontend -> real-time and doc-updater -> real-time should be in sync. Otherwise we can send a payload to doc-updater, but can not receive the confirmation of it -- and the client will send it again in a loop. Also log the size of the payload. --- services/real-time/app/coffee/SafeJsonParse.coffee | 4 ++-- services/real-time/test/unit/coffee/SafeJsonParseTest.coffee | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/services/real-time/app/coffee/SafeJsonParse.coffee b/services/real-time/app/coffee/SafeJsonParse.coffee index edfa921d74..afeb72f96e 100644 --- a/services/real-time/app/coffee/SafeJsonParse.coffee +++ b/services/real-time/app/coffee/SafeJsonParse.coffee @@ -3,8 +3,8 @@ logger = require "logger-sharelatex" module.exports = parse: (data, callback = (error, parsed) ->) -> - if data.length > (Settings.max_doc_length or 2 * 1024 * 1024) - logger.error {head: data.slice(0,1024)}, "data too large to parse" + if data.length > Settings.maxUpdateSize + logger.error {head: data.slice(0,1024), length: data.length}, "data too large to parse" return callback new Error("data too large to parse") try parsed = JSON.parse(data) diff --git a/services/real-time/test/unit/coffee/SafeJsonParseTest.coffee b/services/real-time/test/unit/coffee/SafeJsonParseTest.coffee index 6a6b5a951c..b652a2faae 100644 --- a/services/real-time/test/unit/coffee/SafeJsonParseTest.coffee +++ b/services/real-time/test/unit/coffee/SafeJsonParseTest.coffee @@ -8,7 +8,7 @@ describe 'SafeJsonParse', -> beforeEach -> @SafeJsonParse = SandboxedModule.require modulePath, requires: "settings-sharelatex": @Settings = { - max_doc_length: 16 * 1024 + maxUpdateSize: 16 * 1024 } "logger-sharelatex": @logger = {error: sinon.stub()} @@ -27,7 +27,7 @@ describe 'SafeJsonParse', -> # we have a 2k overhead on top of max size big_blob = Array(16*1024).join("A") data = "{\"foo\": \"#{big_blob}\"}" - @Settings.max_doc_length = 2 * 1024 + @Settings.maxUpdateSize = 2 * 1024 @SafeJsonParse.parse data, (error, parsed) => @logger.error.called.should.equal true expect(error).to.exist From af53d3b6034bf3dd55a79ecd535fd33e9db93a30 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Tue, 24 Mar 2020 11:22:28 +0100 Subject: [PATCH 3/4] [misc] skip duplicate JSON serialization for size check --- .../app/coffee/DocumentUpdaterManager.coffee | 8 +++++ .../app/coffee/WebsocketController.coffee | 32 +++++++++---------- .../acceptance/coffee/ApplyUpdateTests.coffee | 6 ++-- .../coffee/DocumentUpdaterManagerTests.coffee | 15 +++++++++ .../coffee/WebsocketControllerTests.coffee | 8 ++--- 5 files changed, 46 insertions(+), 23 deletions(-) diff --git a/services/real-time/app/coffee/DocumentUpdaterManager.coffee b/services/real-time/app/coffee/DocumentUpdaterManager.coffee index 9968b78753..bec80fae34 100644 --- a/services/real-time/app/coffee/DocumentUpdaterManager.coffee +++ b/services/real-time/app/coffee/DocumentUpdaterManager.coffee @@ -61,9 +61,17 @@ module.exports = DocumentUpdaterManager = change = _.pick change, allowedKeys jsonChange = JSON.stringify change if jsonChange.indexOf("\u0000") != -1 + # memory corruption check error = new Error("null bytes found in op") logger.error err: error, project_id: project_id, doc_id: doc_id, jsonChange: jsonChange, error.message return callback(error) + + updateSize = jsonChange.length + if updateSize > settings.maxUpdateSize + error = new Error("update is too large") + error.updateSize = updateSize + return callback(error) + doc_key = "#{project_id}:#{doc_id}" # Push onto pendingUpdates for doc_id first, because once the doc updater # gets an entry on pending-updates-list, it starts processing. diff --git a/services/real-time/app/coffee/WebsocketController.coffee b/services/real-time/app/coffee/WebsocketController.coffee index 1e4ca5160c..786a77d114 100644 --- a/services/real-time/app/coffee/WebsocketController.coffee +++ b/services/real-time/app/coffee/WebsocketController.coffee @@ -204,22 +204,6 @@ module.exports = WebsocketController = return callback(error) if error? return callback(new Error("no project_id found on client")) if !project_id? - updateSize = JSON.stringify(update).length - if updateSize > settings.maxUpdateSize - metrics.inc "update_too_large" - logger.warn({user_id, project_id, doc_id, updateSize}, "update is too large") - - # mark the update as received -- the client should not send it again! - callback() - - # trigger an out-of-sync error - message = {project_id, doc_id, error: "update is too large"} - setTimeout () -> - client.emit "otUpdateError", message.error, message - client.disconnect() - , 100 - return - WebsocketController._assertClientCanApplyUpdate client, doc_id, update, (error) -> if error? logger.warn {err: error, doc_id, client_id: client.id, version: update.v}, "client is not authorized to make update" @@ -236,6 +220,22 @@ module.exports = WebsocketController = logger.log {user_id, doc_id, project_id, client_id: client.id, version: update.v}, "sending update to doc updater" DocumentUpdaterManager.queueChange project_id, doc_id, update, (error) -> + if error?.message == "update is too large" + metrics.inc "update_too_large" + updateSize = error.updateSize + logger.warn({user_id, project_id, doc_id, updateSize}, "update is too large") + + # mark the update as received -- the client should not send it again! + callback() + + # trigger an out-of-sync error + message = {project_id, doc_id, error: "update is too large"} + setTimeout () -> + client.emit "otUpdateError", message.error, message + client.disconnect() + , 100 + return + if error? logger.error {err: error, project_id, doc_id, client_id: client.id, version: update.v}, "document was not available for update" client.disconnect() diff --git a/services/real-time/test/acceptance/coffee/ApplyUpdateTests.coffee b/services/real-time/test/acceptance/coffee/ApplyUpdateTests.coffee index 2d52975d71..7820a090d8 100644 --- a/services/real-time/test/acceptance/coffee/ApplyUpdateTests.coffee +++ b/services/real-time/test/acceptance/coffee/ApplyUpdateTests.coffee @@ -71,8 +71,10 @@ describe "applyOtUpdate", -> describe "when authorized with a huge edit update", -> before (done) -> @update = { - op: {p: 12, t: "foo"}, - junk: 'this update is too large'.repeat(1024 * 300) # >7MB + op: { + p: 12, + t: "update is too large".repeat(1024 * 400) # >7MB + } } async.series [ (cb) => diff --git a/services/real-time/test/unit/coffee/DocumentUpdaterManagerTests.coffee b/services/real-time/test/unit/coffee/DocumentUpdaterManagerTests.coffee index 482d82b0f7..a6bf0a5e47 100644 --- a/services/real-time/test/unit/coffee/DocumentUpdaterManagerTests.coffee +++ b/services/real-time/test/unit/coffee/DocumentUpdaterManagerTests.coffee @@ -15,6 +15,7 @@ describe 'DocumentUpdaterManager', -> redis: documentupdater: key_schema: pendingUpdates: ({doc_id}) -> "PendingUpdates:#{doc_id}" + maxUpdateSize: 7 * 1024 * 1024 @rclient = {auth:->} @DocumentUpdaterManager = SandboxedModule.require modulePath, @@ -163,6 +164,20 @@ describe 'DocumentUpdaterManager', -> it "should not push the change onto the pending-updates-list queue", -> @rclient.rpush.called.should.equal false + describe "when the update is too large", -> + beforeEach -> + @change = {op: {p: 12,t: "update is too large".repeat(1024 * 400)}} + @DocumentUpdaterManager.queueChange(@project_id, @doc_id, @change, @callback) + + it "should return an error", -> + @callback.calledWithExactly(sinon.match(Error)).should.equal true + + it "should add the size to the error", -> + @callback.args[0][0].updateSize.should.equal 7782422 + + it "should not push the change onto the pending-updates-list queue", -> + @rclient.rpush.called.should.equal false + describe "with invalid keys", -> beforeEach -> @change = { diff --git a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee index b6b4520ced..62243f797c 100644 --- a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee +++ b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee @@ -32,7 +32,6 @@ describe 'WebsocketController', -> "./DocumentUpdaterManager": @DocumentUpdaterManager = {} "./ConnectedUsersManager": @ConnectedUsersManager = {} "./WebsocketLoadBalancer": @WebsocketLoadBalancer = {} - "settings-sharelatex": {maxUpdateSize: 7 * 1024 * 1024} "logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub(), warn: sinon.stub() } "metrics-sharelatex": @metrics = inc: sinon.stub() @@ -673,12 +672,11 @@ describe 'WebsocketController', -> beforeEach (done) -> @client.disconnect = sinon.stub() @client.emit = sinon.stub() - @update = { - op: {p: 12, t: "foo"}, - junk: 'this update is too large'.repeat(1024 * 300) # >7MB - } @client.params.user_id = @user_id @client.params.project_id = @project_id + error = new Error("update is too large") + error.updateSize = 7372835 + @DocumentUpdaterManager.queueChange = sinon.stub().callsArgWith(3, error) @WebsocketController.applyOtUpdate @client, @doc_id, @update, @callback setTimeout -> done() From 69569e3571d1e6878959a9afd00de7e8a3cdbb66 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Tue, 24 Mar 2020 16:21:29 +0100 Subject: [PATCH 4/4] [misc] config: add headroom for JSON serialization in maxUpdateSize --- services/real-time/config/settings.defaults.coffee | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/services/real-time/config/settings.defaults.coffee b/services/real-time/config/settings.defaults.coffee index 9ca2348b0b..ee4bd74316 100644 --- a/services/real-time/config/settings.defaults.coffee +++ b/services/real-time/config/settings.defaults.coffee @@ -54,8 +54,9 @@ settings = # combine # max_doc_length (2mb see above) * 2 (delete + insert) - # max_ranges_size (3mb see MAX_RANGES_SIZE in doc-updater) - maxUpdateSize: parseInt(process.env['MAX_UPDATE_SIZE']) or 7 * 1024 * 1024 + # max_ranges_size (3mb see MAX_RANGES_SIZE in document-updater) + # overhead for JSON serialization + maxUpdateSize: parseInt(process.env['MAX_UPDATE_SIZE']) or 7 * 1024 * 1024 + 64 * 1024 shutdownDrainTimeWindow: process.env['SHUTDOWN_DRAIN_TIME_WINDOW'] or 9