From 03b8a1901db6438e331ac330ecd3f0dec621e4f7 Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Fri, 2 Aug 2019 14:09:48 +0100 Subject: [PATCH] Account for ranges in large json payloads and add line-count limit --- services/docstore/app.coffee | 4 +- .../docstore/app/coffee/HttpController.coffee | 10 ++++ .../docstore/config/settings.defaults.coffee | 2 +- .../coffee/UpdatingDocsTests.coffee | 47 +++++++++++++++++++ .../unit/coffee/HttpControllerTests.coffee | 15 ++++++ 5 files changed, 75 insertions(+), 3 deletions(-) diff --git a/services/docstore/app.coffee b/services/docstore/app.coffee index 7038c53ed7..c32e3a6b19 100644 --- a/services/docstore/app.coffee +++ b/services/docstore/app.coffee @@ -36,8 +36,8 @@ app.get '/project/:project_id/doc', HttpController.getAllDocs app.get '/project/:project_id/ranges', HttpController.getAllRanges app.get '/project/:project_id/doc/:doc_id', HttpController.getDoc app.get '/project/:project_id/doc/:doc_id/raw', HttpController.getRawDoc -# Add 64kb overhead for the JSON encoding -app.post '/project/:project_id/doc/:doc_id', bodyParser.json(limit: Settings.max_doc_length + 64 * 1024), HttpController.updateDoc +# Add 64kb overhead for the JSON encoding, and double the size to allow for ranges in the json payload +app.post '/project/:project_id/doc/:doc_id', bodyParser.json(limit: (Settings.max_doc_length + 64 * 1024) * 2), HttpController.updateDoc app.del '/project/:project_id/doc/:doc_id', HttpController.deleteDoc app.post '/project/:project_id/archive', HttpController.archiveAllDocs diff --git a/services/docstore/app/coffee/HttpController.coffee b/services/docstore/app/coffee/HttpController.coffee index ae2ce1670f..cec535e1cd 100644 --- a/services/docstore/app/coffee/HttpController.coffee +++ b/services/docstore/app/coffee/HttpController.coffee @@ -2,6 +2,7 @@ DocManager = require "./DocManager" logger = require "logger-sharelatex" DocArchive = require "./DocArchiveManager" HealthChecker = require "./HealthChecker" +Settings = require "settings-sharelatex" module.exports = HttpController = @@ -68,6 +69,15 @@ module.exports = HttpController = res.send 400 # Bad Request return + bodyLength = lines.reduce( + (len, line) => line.length + len + 0 + ) + if bodyLength > Settings.max_doc_length + logger.error project_id: project_id, doc_id: doc_id, bodyLength: bodyLength, "document body too large" + res.status(413).send("document body too large") + return + logger.log project_id: project_id, doc_id: doc_id, "got http request to update doc" DocManager.updateDoc project_id, doc_id, lines, version, ranges, (error, modified, rev) -> return next(error) if error? diff --git a/services/docstore/config/settings.defaults.coffee b/services/docstore/config/settings.defaults.coffee index 8d8b8c74a2..e45570d3fe 100644 --- a/services/docstore/config/settings.defaults.coffee +++ b/services/docstore/config/settings.defaults.coffee @@ -28,4 +28,4 @@ if process.env['AWS_ACCESS_KEY_ID']? and process.env['AWS_SECRET_ACCESS_KEY']? a secret: process.env['AWS_SECRET_ACCESS_KEY'] bucket: process.env['AWS_BUCKET'] -module.exports = Settings \ No newline at end of file +module.exports = Settings diff --git a/services/docstore/test/acceptance/coffee/UpdatingDocsTests.coffee b/services/docstore/test/acceptance/coffee/UpdatingDocsTests.coffee index 4d567e8f4c..2b916a3a83 100644 --- a/services/docstore/test/acceptance/coffee/UpdatingDocsTests.coffee +++ b/services/docstore/test/acceptance/coffee/UpdatingDocsTests.coffee @@ -176,3 +176,50 @@ describe "Applying updates to a doc", -> DocstoreClient.getDoc @project_id, @doc_id, {}, (error, res, doc) => doc.lines.should.deep.equal @largeLines done() + + describe "when there is a large json payload", -> + beforeEach (done) -> + line = new Array(1025).join("x") # 1kb + @largeLines = Array.apply(null, Array(1024)).map(() -> line) # 1kb + @originalRanges.padding = Array.apply(null, Array(2049)).map(() -> line) # 2mb + 1kb + DocstoreClient.updateDoc @project_id, @doc_id, @largeLines, @version, @originalRanges, (error, @res, @body) => + done() + + it "should return modified = true", -> + @body.modified.should.equal true + + it "should update the doc in the API", (done) -> + DocstoreClient.getDoc @project_id, @doc_id, {}, (error, res, doc) => + doc.lines.should.deep.equal @largeLines + done() + + describe "when the document body is too large", -> + beforeEach (done) -> + line = new Array(1025).join("x") # 1kb + @largeLines = Array.apply(null, Array(2049)).map(() -> line) # 2mb + 1kb + DocstoreClient.updateDoc @project_id, @doc_id, @largeLines, @version, @originalRanges, (error, @res, @body) => + done() + + it "should return 413", -> + @res.statusCode.should.equal 413 + + it "should report body too large", -> + @res.body.should.equal 'document body too large' + + it "should not update the doc in the API", (done) -> + DocstoreClient.getDoc @project_id, @doc_id, {}, (error, res, doc) => + doc.lines.should.deep.equal @originalLines + done() + + describe "when the json payload is too large", -> + beforeEach (done) -> + line = new Array(1025).join("x") # 1kb + @largeLines = Array.apply(null, Array(1024)).map(() -> line) # 1kb + @originalRanges.padding = Array.apply(null, Array(4096)).map(() -> line) # 4mb + DocstoreClient.updateDoc @project_id, @doc_id, @largeLines, @version, @originalRanges, (error, @res, @body) => + done() + + it "should not update the doc in the API", (done) -> + DocstoreClient.getDoc @project_id, @doc_id, {}, (error, res, doc) => + doc.lines.should.deep.equal @originalLines + done() diff --git a/services/docstore/test/unit/coffee/HttpControllerTests.coffee b/services/docstore/test/unit/coffee/HttpControllerTests.coffee index 362325845d..3b7ffbbe8d 100644 --- a/services/docstore/test/unit/coffee/HttpControllerTests.coffee +++ b/services/docstore/test/unit/coffee/HttpControllerTests.coffee @@ -15,6 +15,7 @@ describe "HttpController", -> "logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub() } "./HealthChecker": {} @res = { send: sinon.stub(), json: sinon.stub(), setHeader:sinon.stub() } + @res.status = sinon.stub().returns(@res) @req = { query:{}} @next = sinon.stub() @project_id = "mock-project-id" @@ -292,6 +293,20 @@ describe "HttpController", -> .calledWith(400) .should.equal true + describe "when the doc body is too large", -> + beforeEach -> + @req.body = + lines: @lines = Array(2049).fill('a'.repeat(1024)) + version: @version = 42 + ranges: @ranges = { changes: "mock" } + @HttpController.updateDoc @req, @res, @next + + it "should return a 413 (too large) response", -> + sinon.assert.calledWith(@res.status, 413) + + it "should report that the document body is too large", -> + sinon.assert.calledWith(@res.send, "document body too large") + describe "deleteDoc", -> beforeEach -> @req.params =