From bd70aaa76c780a5cddab3d8c00965e782f0051b0 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 14 Feb 2017 16:11:43 +0000 Subject: [PATCH 1/3] add sha1 hash support on writes --- .../app/coffee/RedisKeyBuilder.coffee | 2 ++ .../app/coffee/RedisManager.coffee | 28 +++++++++++++++---- .../config/settings.defaults.coffee | 1 + .../RedisBackend/RedisBackendTests.coffee | 2 ++ .../RedisManager/RedisManagerTests.coffee | 5 +++- 5 files changed, 32 insertions(+), 6 deletions(-) diff --git a/services/document-updater/app/coffee/RedisKeyBuilder.coffee b/services/document-updater/app/coffee/RedisKeyBuilder.coffee index 1b5e548809..adde3ee1c9 100644 --- a/services/document-updater/app/coffee/RedisKeyBuilder.coffee +++ b/services/document-updater/app/coffee/RedisKeyBuilder.coffee @@ -28,6 +28,8 @@ module.exports = RedisKeyBuilder = return (key_schema) -> key_schema.docOps({doc_id}) docVersion: ({doc_id}) -> return (key_schema) -> key_schema.docVersion({doc_id}) + docHash: ({doc_id}) -> + return (key_schema) -> key_schema.docHash({doc_id}) projectKey: ({doc_id}) -> return (key_schema) -> key_schema.projectKey({doc_id}) uncompressedHistoryOp: ({doc_id}) -> diff --git a/services/document-updater/app/coffee/RedisManager.coffee b/services/document-updater/app/coffee/RedisManager.coffee index be5166b94c..d9f210f1d6 100644 --- a/services/document-updater/app/coffee/RedisManager.coffee +++ b/services/document-updater/app/coffee/RedisManager.coffee @@ -6,6 +6,7 @@ keys = require('./RedisKeyBuilder') logger = require('logger-sharelatex') metrics = require('./Metrics') Errors = require "./Errors" +crypto = require "crypto" # Make times easy to read minutes = 60 # seconds for Redis expire @@ -18,12 +19,15 @@ module.exports = RedisManager = callback = (error) -> timer.done() _callback(error) - logger.log project_id:project_id, doc_id:doc_id, version: version, "putting doc in redis" + docLines = JSON.stringify(docLines) + docHash = RedisManager._computeHash(docLines) + logger.log project_id:project_id, doc_id:doc_id, version: version, hash:docHash, "putting doc in redis" ranges = RedisManager._serializeRanges(ranges) multi = rclient.multi() - multi.set keys.docLines(doc_id:doc_id), JSON.stringify(docLines) + multi.set keys.docLines(doc_id:doc_id), docLines multi.set keys.projectKey({doc_id:doc_id}), project_id multi.set keys.docVersion(doc_id:doc_id), version + multi.set keys.docHash(doc_id:doc_id), docHash if ranges? multi.set keys.ranges(doc_id:doc_id), ranges else @@ -46,6 +50,7 @@ module.exports = RedisManager = multi.del keys.docLines(doc_id:doc_id) multi.del keys.projectKey(doc_id:doc_id) multi.del keys.docVersion(doc_id:doc_id) + multi.del keys.docHash(doc_id:doc_id) multi.del keys.ranges(doc_id:doc_id) multi.exec (error) -> return callback(error) if error? @@ -56,11 +61,17 @@ module.exports = RedisManager = multi = rclient.multi() multi.get keys.docLines(doc_id:doc_id) multi.get keys.docVersion(doc_id:doc_id) + multi.get keys.docHash(doc_id:doc_id) multi.get keys.projectKey(doc_id:doc_id) multi.get keys.ranges(doc_id:doc_id) - multi.exec (error, [docLines, version, doc_project_id, ranges])-> + multi.exec (error, [docLines, version, storedHash, doc_project_id, ranges])-> timer.done() return callback(error) if error? + if docLines? + computedHash = RedisManager._computeHash(docLines) + if computedHash isnt storedHash + logger.error project_id: project_id, doc_id: doc_id, doc_project_id: doc_project_id, computedHash: computedHash, storedHash: storedHash, "hash mismatch on retrieved document" + try docLines = JSON.parse docLines ranges = RedisManager._deserializeRanges(ranges) @@ -121,8 +132,11 @@ module.exports = RedisManager = return callback(error) jsonOps = appliedOps.map (op) -> JSON.stringify op multi = rclient.multi() - multi.set keys.docLines(doc_id:doc_id), JSON.stringify(docLines) + newDocLines = JSON.stringify(docLines) + newHash = RedisManager._computeHash(newDocLines) + multi.set keys.docLines(doc_id:doc_id), newDocLines multi.set keys.docVersion(doc_id:doc_id), newVersion + multi.set keys.docHash(doc_id:doc_id), newHash if jsonOps.length > 0 multi.rpush keys.docOps(doc_id: doc_id), jsonOps... multi.expire keys.docOps(doc_id: doc_id), RedisManager.DOC_OPS_TTL @@ -150,4 +164,8 @@ module.exports = RedisManager = if !ranges? or ranges == "" return {} else - return JSON.parse(ranges) \ No newline at end of file + return JSON.parse(ranges) + + _computeHash: (docLines) -> + # use sha1 checksum of doclines to detect data corruption + return crypto.createHash('sha1').update(docLines).digest('hex') diff --git a/services/document-updater/config/settings.defaults.coffee b/services/document-updater/config/settings.defaults.coffee index 42daf505d9..ae0f9fe681 100755 --- a/services/document-updater/config/settings.defaults.coffee +++ b/services/document-updater/config/settings.defaults.coffee @@ -30,6 +30,7 @@ module.exports = docLines: ({doc_id}) -> "doclines:#{doc_id}" docOps: ({doc_id}) -> "DocOps:#{doc_id}" docVersion: ({doc_id}) -> "DocVersion:#{doc_id}" + docHash: ({doc_id}) -> "DocHash:#{doc_id}" projectKey: ({doc_id}) -> "ProjectId:#{doc_id}" docsInProject: ({project_id}) -> "DocsIn:#{project_id}" ranges: ({doc_id}) -> "Ranges:#{doc_id}" diff --git a/services/document-updater/test/unit/coffee/RedisBackend/RedisBackendTests.coffee b/services/document-updater/test/unit/coffee/RedisBackend/RedisBackendTests.coffee index 814a0b932d..52bb69bebb 100644 --- a/services/document-updater/test/unit/coffee/RedisBackend/RedisBackendTests.coffee +++ b/services/document-updater/test/unit/coffee/RedisBackend/RedisBackendTests.coffee @@ -19,6 +19,7 @@ describe "RedisBackend", -> docLines: ({doc_id}) -> "doclines:#{doc_id}" docOps: ({doc_id}) -> "DocOps:#{doc_id}" docVersion: ({doc_id}) -> "DocVersion:#{doc_id}" + docHash: ({doc_id}) -> "DocHash:#{doc_id}" projectKey: ({doc_id}) -> "ProjectId:#{doc_id}" pendingUpdates: ({doc_id}) -> "PendingUpdates:#{doc_id}" docsInProject: ({project_id}) -> "DocsIn:#{project_id}" @@ -33,6 +34,7 @@ describe "RedisBackend", -> docLines: ({doc_id}) -> "doclines:{#{doc_id}}" docOps: ({doc_id}) -> "DocOps:{#{doc_id}}" docVersion: ({doc_id}) -> "DocVersion:{#{doc_id}}" + docHash: ({doc_id}) -> "DocHash:{#{doc_id}}" projectKey: ({doc_id}) -> "ProjectId:{#{doc_id}}" pendingUpdates: ({doc_id}) -> "PendingUpdates:{#{doc_id}}" docsInProject: ({project_id}) -> "DocsIn:{#{project_id}}" diff --git a/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee b/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee index 420d2039b4..6e235b4a9d 100644 --- a/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee @@ -4,6 +4,7 @@ should = chai.should() modulePath = "../../../../app/js/RedisManager.js" SandboxedModule = require('sandboxed-module') Errors = require "../../../../app/js/Errors" +crypto = require "crypto" describe "RedisManager", -> beforeEach -> @@ -19,6 +20,7 @@ describe "RedisManager", -> docLines: ({doc_id}) -> "doclines:#{doc_id}" docOps: ({doc_id}) -> "DocOps:#{doc_id}" docVersion: ({doc_id}) -> "DocVersion:#{doc_id}" + docHash: ({doc_id}) -> "DocHash:#{doc_id}" projectKey: ({doc_id}) -> "ProjectId:#{doc_id}" pendingUpdates: ({doc_id}) -> "PendingUpdates:#{doc_id}" docsInProject: ({project_id}) -> "DocsIn:#{project_id}" @@ -38,10 +40,11 @@ describe "RedisManager", -> @lines = ["one", "two", "three"] @jsonlines = JSON.stringify @lines @version = 42 + @hash = crypto.createHash('sha1').update(@jsonlines).digest('hex') @ranges = { comments: "mock", entries: "mock" } @json_ranges = JSON.stringify @ranges @rclient.get = sinon.stub() - @rclient.exec = sinon.stub().callsArgWith(0, null, [@jsonlines, @version, @project_id, @json_ranges]) + @rclient.exec = sinon.stub().callsArgWith(0, null, [@jsonlines, @version, @hash, @project_id, @json_ranges]) describe "successfully", -> beforeEach -> From b5a4458b68cdda5f24c2721beec2fe7a0a05c477 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 15 Feb 2017 14:12:36 +0000 Subject: [PATCH 2/3] check sha1 hash value only if present --- services/document-updater/app/coffee/RedisManager.coffee | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/services/document-updater/app/coffee/RedisManager.coffee b/services/document-updater/app/coffee/RedisManager.coffee index d9f210f1d6..ab95d1129f 100644 --- a/services/document-updater/app/coffee/RedisManager.coffee +++ b/services/document-updater/app/coffee/RedisManager.coffee @@ -67,7 +67,9 @@ module.exports = RedisManager = multi.exec (error, [docLines, version, storedHash, doc_project_id, ranges])-> timer.done() return callback(error) if error? - if docLines? + + # check sha1 hash value if present + if docLines? and storedHash? computedHash = RedisManager._computeHash(docLines) if computedHash isnt storedHash logger.error project_id: project_id, doc_id: doc_id, doc_project_id: doc_project_id, computedHash: computedHash, storedHash: storedHash, "hash mismatch on retrieved document" From a3c127e46936b63cd04309da6cf8cde586063fa9 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 20 Feb 2017 13:53:25 +0000 Subject: [PATCH 3/3] added unit tests --- .../RedisManager/RedisManagerTests.coffee | 44 ++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee b/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee index 6e235b4a9d..daad278174 100644 --- a/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee +++ b/services/document-updater/test/unit/coffee/RedisManager/RedisManagerTests.coffee @@ -59,7 +59,12 @@ describe "RedisManager", -> @rclient.get .calledWith("DocVersion:#{@doc_id}") .should.equal true - + + it 'should get the hash', -> + @rclient.get + .calledWith("DocHash:#{@doc_id}") + .should.equal true + it "should get the ranges", -> @rclient.get .calledWith("Ranges:#{@doc_id}") @@ -70,6 +75,26 @@ describe "RedisManager", -> .calledWith(null, @lines, @version, @ranges) .should.equal true + it 'should not log any errors', -> + @logger.error.calledWith() + .should.equal false + + describe "with a corrupted document", -> + beforeEach -> + @badHash = "INVALID-HASH-VALUE" + @rclient.exec = sinon.stub().callsArgWith(0, null, [@jsonlines, @version, @badHash, @project_id, @json_ranges]) + @RedisManager.getDoc @project_id, @doc_id, @callback + + it 'should log a hash error', -> + @logger.error.calledWith() + .should.equal true + + it 'should return the document', -> + @callback + .calledWith(null, @lines, @version, @ranges) + .should.equal true + + describe "getDoc with an invalid project id", -> beforeEach -> @another_project_id = "project-id-456" @@ -177,6 +202,7 @@ describe "RedisManager", -> @lines = ["one", "two", "three"] @ops = [{ op: [{ i: "foo", p: 4 }] },{ op: [{ i: "bar", p: 8 }] }] @version = 42 + @hash = crypto.createHash('sha1').update(JSON.stringify(@lines)).digest('hex') @ranges = { comments: "mock", entries: "mock" } @rclient.exec = sinon.stub().callsArg(0) @@ -200,6 +226,11 @@ describe "RedisManager", -> @rclient.set .calledWith("DocVersion:#{@doc_id}", @version) .should.equal true + + it "should set the hash", -> + @rclient.set + .calledWith("DocHash:#{@doc_id}", @hash) + .should.equal true it "should set the ranges", -> @rclient.set @@ -275,6 +306,7 @@ describe "RedisManager", -> @rclient.exec.yields() @lines = ["one", "two", "three"] @version = 42 + @hash = crypto.createHash('sha1').update(JSON.stringify(@lines)).digest('hex') @ranges = { comments: "mock", entries: "mock" } describe "with non-empty ranges", -> @@ -290,6 +322,11 @@ describe "RedisManager", -> @rclient.set .calledWith("DocVersion:#{@doc_id}", @version) .should.equal true + + it "should set the hash", -> + @rclient.set + .calledWith("DocHash:#{@doc_id}", @hash) + .should.equal true it "should set the ranges", -> @rclient.set @@ -336,6 +373,11 @@ describe "RedisManager", -> @rclient.del .calledWith("DocVersion:#{@doc_id}") .should.equal true + + it "should delete the hash", -> + @rclient.del + .calledWith("DocHash:#{@doc_id}") + .should.equal true it "should delete the project_id for the doc", -> @rclient.del