From 830d676f4f443a3b09d1df33ff8879a8e1eab593 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 1 Dec 2015 11:05:49 +0000 Subject: [PATCH] Add in limit on all JSON parsing --- .../coffee/DocumentUpdaterController.coffee | 16 +++++---- .../real-time/app/coffee/SafeJsonParse.coffee | 13 +++++++ .../app/coffee/WebsocketLoadBalancer.coffee | 14 +++++--- .../real-time/config/settings.defaults.coffee | 4 ++- .../DocumentUpdaterControllerTests.coffee | 10 ++++++ .../test/unit/coffee/SafeJsonParseTest.coffee | 34 +++++++++++++++++++ .../coffee/WebsocketLoadBalancerTests.coffee | 12 ++++++- 7 files changed, 89 insertions(+), 14 deletions(-) create mode 100644 services/real-time/app/coffee/SafeJsonParse.coffee create mode 100644 services/real-time/test/unit/coffee/SafeJsonParseTest.coffee diff --git a/services/real-time/app/coffee/DocumentUpdaterController.coffee b/services/real-time/app/coffee/DocumentUpdaterController.coffee index 3123aee942..9ff6b8ee62 100644 --- a/services/real-time/app/coffee/DocumentUpdaterController.coffee +++ b/services/real-time/app/coffee/DocumentUpdaterController.coffee @@ -2,6 +2,7 @@ logger = require "logger-sharelatex" settings = require 'settings-sharelatex' redis = require("redis-sharelatex") rclient = redis.createClient(settings.redis.web) +SafeJsonParse = require "./SafeJsonParse" MESSAGE_SIZE_LOG_LIMIT = 1024 * 1024 # 1Mb @@ -15,13 +16,14 @@ module.exports = DocumentUpdaterController = DocumentUpdaterController._processMessageFromDocumentUpdater(io, channel, message) _processMessageFromDocumentUpdater: (io, channel, message) -> - if message.length > MESSAGE_SIZE_LOG_LIMIT - logger.log {length: message.length, head: message.slice(0,200)}, "large message from doc updater" - message = JSON.parse message - if message.op? - DocumentUpdaterController._applyUpdateFromDocumentUpdater(io, message.doc_id, message.op) - else if message.error? - DocumentUpdaterController._processErrorFromDocumentUpdater(io, message.doc_id, message.error, message) + SafeJsonParse.parse message, (error, message) -> + if error? + logger.error {err: error, channel}, "error parsing JSON" + return + if message.op? + DocumentUpdaterController._applyUpdateFromDocumentUpdater(io, message.doc_id, message.op) + else if message.error? + DocumentUpdaterController._processErrorFromDocumentUpdater(io, message.doc_id, message.error, message) _applyUpdateFromDocumentUpdater: (io, doc_id, update) -> for client in io.sockets.clients(doc_id) diff --git a/services/real-time/app/coffee/SafeJsonParse.coffee b/services/real-time/app/coffee/SafeJsonParse.coffee new file mode 100644 index 0000000000..edfa921d74 --- /dev/null +++ b/services/real-time/app/coffee/SafeJsonParse.coffee @@ -0,0 +1,13 @@ +Settings = require "settings-sharelatex" +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" + return callback new Error("data too large to parse") + try + parsed = JSON.parse(data) + catch e + return callback e + callback null, parsed \ No newline at end of file diff --git a/services/real-time/app/coffee/WebsocketLoadBalancer.coffee b/services/real-time/app/coffee/WebsocketLoadBalancer.coffee index 4635a42256..d346c41107 100644 --- a/services/real-time/app/coffee/WebsocketLoadBalancer.coffee +++ b/services/real-time/app/coffee/WebsocketLoadBalancer.coffee @@ -1,6 +1,7 @@ Settings = require 'settings-sharelatex' logger = require 'logger-sharelatex' redis = require("redis-sharelatex") +SafeJsonParse = require "./SafeJsonParse" rclientPub = redis.createClient(Settings.redis.web) rclientSub = redis.createClient(Settings.redis.web) @@ -28,9 +29,12 @@ module.exports = WebsocketLoadBalancer = WebsocketLoadBalancer._processEditorEvent io, channel, message _processEditorEvent: (io, channel, message) -> - message = JSON.parse(message) - if message.room_id == "all" - io.sockets.emit(message.message, message.payload...) - else if message.room_id? - io.sockets.in(message.room_id).emit(message.message, message.payload...) + SafeJsonParse.parse message, (error, message) -> + if error? + logger.error {err: error, channel}, "error parsing JSON" + return + if message.room_id == "all" + io.sockets.emit(message.message, message.payload...) + else if message.room_id? + io.sockets.in(message.room_id).emit(message.message, message.payload...) diff --git a/services/real-time/config/settings.defaults.coffee b/services/real-time/config/settings.defaults.coffee index 50abd714c4..eaf5e7d61e 100644 --- a/services/real-time/config/settings.defaults.coffee +++ b/services/real-time/config/settings.defaults.coffee @@ -25,4 +25,6 @@ module.exports = security: sessionSecret: "secret-please-change" - cookieName:"sharelatex.sid" \ No newline at end of file + cookieName:"sharelatex.sid" + + max_doc_length: 2 * 1024 * 1024 # 2mb \ No newline at end of file diff --git a/services/real-time/test/unit/coffee/DocumentUpdaterControllerTests.coffee b/services/real-time/test/unit/coffee/DocumentUpdaterControllerTests.coffee index 186b493669..8673b12b41 100644 --- a/services/real-time/test/unit/coffee/DocumentUpdaterControllerTests.coffee +++ b/services/real-time/test/unit/coffee/DocumentUpdaterControllerTests.coffee @@ -17,6 +17,8 @@ describe "DocumentUpdaterController", -> "redis-sharelatex" : createClient: ()=> @rclient = {auth:->} + "./SafeJsonParse": @SafeJsonParse = + parse: (data, cb) => cb null, JSON.parse(data) describe "listenForUpdatesFromDocumentUpdater", -> beforeEach -> @@ -31,6 +33,14 @@ describe "DocumentUpdaterController", -> @rclient.on.calledWith("message").should.equal true describe "_processMessageFromDocumentUpdater", -> + describe "with bad JSON", -> + beforeEach -> + @SafeJsonParse.parse = sinon.stub().callsArgWith 1, new Error("oops") + @EditorUpdatesController._processMessageFromDocumentUpdater @io, "applied-ops", "blah" + + it "should log an error", -> + @logger.error.called.should.equal true + describe "with update", -> beforeEach -> @message = diff --git a/services/real-time/test/unit/coffee/SafeJsonParseTest.coffee b/services/real-time/test/unit/coffee/SafeJsonParseTest.coffee new file mode 100644 index 0000000000..6a6b5a951c --- /dev/null +++ b/services/real-time/test/unit/coffee/SafeJsonParseTest.coffee @@ -0,0 +1,34 @@ +require('chai').should() +expect = require("chai").expect +SandboxedModule = require('sandboxed-module') +modulePath = '../../../app/js/SafeJsonParse' +sinon = require("sinon") + +describe 'SafeJsonParse', -> + beforeEach -> + @SafeJsonParse = SandboxedModule.require modulePath, requires: + "settings-sharelatex": @Settings = { + max_doc_length: 16 * 1024 + } + "logger-sharelatex": @logger = {error: sinon.stub()} + + describe "parse", -> + it "should parse documents correctly", (done) -> + @SafeJsonParse.parse '{"foo": "bar"}', (error, parsed) -> + expect(parsed).to.deep.equal {foo: "bar"} + done() + + it "should return an error on bad data", (done) -> + @SafeJsonParse.parse 'blah', (error, parsed) -> + expect(error).to.exist + done() + + it "should return an error on oversized data", (done) -> + # 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 + @SafeJsonParse.parse data, (error, parsed) => + @logger.error.called.should.equal true + expect(error).to.exist + done() \ No newline at end of file diff --git a/services/real-time/test/unit/coffee/WebsocketLoadBalancerTests.coffee b/services/real-time/test/unit/coffee/WebsocketLoadBalancerTests.coffee index d6add7b3ba..07afe988ab 100644 --- a/services/real-time/test/unit/coffee/WebsocketLoadBalancerTests.coffee +++ b/services/real-time/test/unit/coffee/WebsocketLoadBalancerTests.coffee @@ -9,7 +9,9 @@ describe "WebsocketLoadBalancer", -> "redis-sharelatex": createClient: () -> auth:-> - "logger-sharelatex": { log: sinon.stub(), err: sinon.stub() } + "logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub() } + "./SafeJsonParse": @SafeJsonParse = + parse: (data, cb) => cb null, JSON.parse(data) @io = {} @WebsocketLoadBalancer.rclientPub = publish: sinon.stub() @WebsocketLoadBalancer.rclientSub = @@ -59,6 +61,14 @@ describe "WebsocketLoadBalancer", -> .should.equal true describe "_processEditorEvent", -> + describe "with bad JSON", -> + beforeEach -> + @SafeJsonParse.parse = sinon.stub().callsArgWith 1, new Error("oops") + @WebsocketLoadBalancer._processEditorEvent(@io, "editor-events", "blah") + + it "should log an error", -> + @logger.error.called.should.equal true + describe "with a designated room", -> beforeEach -> @io.sockets =