Merge pull request #1493 from sharelatex/sk-read-write-token-match-on-prefix

Constant-time comparison for read-write tokens

GitOrigin-RevId: ddd83de551c540544fde426d7d5aca9f4c83fcc7

services/web/app/coffee/Features/Project/ProjectDetailsHandler.coffee

@@ -141,7 +141,9 @@ module.exports = ProjectDetailsHandler =
 		project.tokens ||= {}
 		tokens = project.tokens
 		if !tokens.readAndWrite?
-			tokens.readAndWrite = ProjectTokenGenerator.readAndWriteToken()
+			{ token, numericPrefix } = ProjectTokenGenerator.readAndWriteToken()
+			tokens.readAndWrite = token
+			tokens.readAndWritePrefix = numericPrefix
 		if !tokens.readOnly?
 			ProjectTokenGenerator.generateUniqueReadOnlyToken (err, token) ->
 				return callback(err) if err?

services/web/app/coffee/Features/Project/ProjectTokenGenerator.coffee

@@ -44,7 +44,7 @@ module.exports = ProjectTokenGenerator =
 			ProjectTokenGenerator.TOKEN_ALPHA
 		)
 		fullToken = "#{numerics}#{token}"
-		return fullToken
+		return { token: fullToken, numericPrefix: numerics }
 
 	generateUniqueReadOnlyToken: (callback=(err, token)->) ->
 		Async.retry 10

services/web/app/coffee/Features/TokenAccess/TokenAccessHandler.coffee

@@ -6,16 +6,48 @@ UserGetter = require '../User/UserGetter'
 ObjectId = require("mongojs").ObjectId
 Settings = require('settings-sharelatex')
 V1Api = require "../V1/V1Api"
+crypto = require 'crypto'
 
 module.exports = TokenAccessHandler =
 
 	ANONYMOUS_READ_AND_WRITE_ENABLED:
 		Settings.allowAnonymousReadAndWriteSharing == true
 
-	findProjectWithReadOnlyToken: (token, callback=(err, project, projectExists)->) ->
+	_extractNumericPrefix: (token) ->
+		token.match(/^(\d+)\w+/)
+
+	_getProjectByReadOnlyToken: (token, callback=(err, project)->) ->
 		Project.findOne {
 			'tokens.readOnly': token
-		}, {_id: 1, publicAccesLevel: 1, owner_ref: 1}, (err, project) ->
+		}, {_id: 1, tokens: 1, publicAccesLevel: 1, owner_ref: 1}, callback
+
+	_getProjectByEitherToken: (token, callback=(err, project)->) ->
+		TokenAccessHandler._getProjectByReadOnlyToken token, (err, project) ->
+			return callback(err) if err?
+			if project?
+				return callback(null, project)
+			TokenAccessHandler._getProjectByReadAndWriteToken token, (err, project) ->
+				return callback(err) if err?
+				callback(null, project)
+
+	_getProjectByReadAndWriteToken: (token, callback=(err, project)->) ->
+		numericPrefixMatch = TokenAccessHandler._extractNumericPrefix(token)
+		if !numericPrefixMatch
+			return callback(null, null)
+		numerics = numericPrefixMatch[1]
+		Project.findOne {
+			'tokens.readAndWritePrefix': numerics
+		}, {_id: 1, tokens: 1, publicAccesLevel: 1, owner_ref: 1}, (err, project) ->
+			return callback(err) if err?
+			if !project?
+				return callback(null, null)
+			if !crypto.timingSafeEqual(new Buffer(token), new Buffer(project.tokens.readAndWrite))
+				logger.err {token}, "read-and-write token match on numeric section, but not on full token"
+				return callback(null, null)
+			callback(null, project)
+
+	findProjectWithReadOnlyToken: (token, callback=(err, project, projectExists)->) ->
+		TokenAccessHandler._getProjectByReadOnlyToken token, (err, project) ->
 			if err?
 				return callback(err)
 			if !project?
@@ -25,32 +57,26 @@ module.exports = TokenAccessHandler =
 			return callback(null, project, true)
 
 	findProjectWithReadAndWriteToken: (token, callback=(err, project, projectExists)->) ->
-		Project.findOne {
-			'tokens.readAndWrite': token
-		}, {_id: 1, publicAccesLevel: 1, owner_ref: 1}, (err, project) ->
+		TokenAccessHandler._getProjectByReadAndWriteToken token, (err, project) ->
 			if err?
 				return callback(err)
 			if !project?
 				return callback(null, null, false) # Project doesn't exist, so we handle differently
 			if project.publicAccesLevel != PublicAccessLevels.TOKEN_BASED
-				return callback(null, null, true) # Project does exist, but it isn't token based
+					return callback(null, null, true) # Project does exist, but it isn't token based
 			return callback(null, project, true)
 
+	_userIsMember: (userId, projectId, callback=(err, isMember)->) ->
+		CollaboratorsHandler.isUserInvitedMemberOfProject userId, projectId, callback
+
 	findProjectWithHigherAccess: (token, userId, callback=(err, project)->) ->
-		Project.findOne {
-			$or: [
-				{'tokens.readAndWrite': token},
-				{'tokens.readOnly': token}
-			]
-		}, {_id: 1}, (err, project) ->
-			if err?
-				return callback(err)
+		TokenAccessHandler._getProjectByEitherToken token, (err, project) ->
+			return callback(err) if err?
 			if !project?
 				return callback(null, null)
 			projectId = project._id
-			CollaboratorsHandler.isUserInvitedMemberOfProject userId, projectId, (err, isMember) ->
-				if err?
-					return callback(err)
+			TokenAccessHandler._userIsMember userId, projectId, (err, isMember) ->
+				return callback(err) if err?
 				callback(
 					null,
 					if isMember == true then project else null

services/web/app/coffee/models/Project.coffee

@@ -53,6 +53,13 @@ ProjectSchema = new Schema
 				partialFilterExpression: {'tokens.readAndWrite': {$exists: true}}
 			}
 		}
+		readAndWritePrefix: {
+			type: String,
+			index: {
+				unique: true,
+				partialFilterExpression: {'tokens.readAndWritePrefix': {$exists: true}}
+			}
+		}
 	tokenAccessReadOnly_refs         : [ type:ObjectId, ref:'User' ]
 	tokenAccessReadAndWrite_refs     : [ type:ObjectId, ref:'User' ]
 	fromV1TemplateId: { type: Number }

services/web/test/unit/coffee/Project/ProjectDetailsHandlerTests.coffee

@@ -287,6 +287,7 @@ describe 'ProjectDetailsHandler', ->
 					tokens:
 						readOnly: 'aaa'
 						readAndWrite: '42bbb'
+						readAndWritePrefix: '42'
 				@ProjectGetter.getProject = sinon.stub()
 					.callsArgWith(2, null, @project)
 				@ProjectModel.update = sinon.stub()
@@ -317,8 +318,12 @@ describe 'ProjectDetailsHandler', ->
 					.callsArgWith(2, null, @project)
 				@readOnlyToken = 'abc'
 				@readAndWriteToken = '42def'
+				@readAndWriteTokenPrefix = '42'
 				@ProjectTokenGenerator.generateUniqueReadOnlyToken = sinon.stub().callsArgWith(0, null, @readOnlyToken)
-				@ProjectTokenGenerator.readAndWriteToken = sinon.stub().returns(@readAndWriteToken)
+				@ProjectTokenGenerator.readAndWriteToken = sinon.stub().returns({
+					token: @readAndWriteToken
+					numericPrefix:  @readAndWriteTokenPrefix
+				})
 				@ProjectModel.update = sinon.stub()
 					.callsArgWith(2, null)
 
@@ -338,7 +343,15 @@ describe 'ProjectDetailsHandler', ->
 					expect(@ProjectModel.update.callCount).to.equal 1
 					expect(@ProjectModel.update.calledWith(
 						{_id: @project_id},
-						{$set: {tokens: {readOnly: @readOnlyToken, readAndWrite: @readAndWriteToken}}}
+						{
+							$set: {
+								tokens: {
+									readOnly: @readOnlyToken,
+									readAndWrite: @readAndWriteToken,
+									readAndWritePrefix: @readAndWriteTokenPrefix
+								}
+							}
+						}
 					)).to.equal true
 					done()
 
@@ -347,6 +360,7 @@ describe 'ProjectDetailsHandler', ->
 					expect(err).to.not.exist
 					expect(tokens).to.deep.equal {
 						readOnly: @readOnlyToken,
-						readAndWrite: @readAndWriteToken
+						readAndWrite: @readAndWriteToken,
+						readAndWritePrefix: @readAndWriteTokenPrefix
 					}
 					done()

services/web/test/unit/coffee/TokenAccess/TokenAccessHandlerTests.coffee

@@ -94,13 +94,20 @@ describe "TokenAccessHandler", ->
 
 	describe 'findProjectWithReadAndWriteToken', ->
 		beforeEach ->
+			@token = '1234bcdf'
+			@tokenPrefix = '1234'
+			@project.tokens = {
+				readOnly: 'atntntn'
+				readAndWrite: @token,
+				readAndWritePrefix: @tokenPrefix
+			}
 			@Project.findOne = sinon.stub().callsArgWith(2, null, @project)
 
 		it 'should call Project.findOne', (done) ->
 			@TokenAccessHandler.findProjectWithReadAndWriteToken @token, (err, project) =>
 				expect(@Project.findOne.callCount).to.equal 1
 				expect(@Project.findOne.calledWith({
-					'tokens.readAndWrite': @token
+					'tokens.readAndWritePrefix': @tokenPrefix
 				})).to.equal true
 				done()
 
@@ -154,10 +161,9 @@ describe "TokenAccessHandler", ->
 			it 'should call Project.findOne', (done) ->
 				@TokenAccessHandler.findProjectWithHigherAccess @token, @userId, (err, project) =>
 					expect(@Project.findOne.callCount).to.equal 1
-					expect(@Project.findOne.calledWith($or: [
-						{'tokens.readAndWrite': @token},
-						{'tokens.readOnly': @token}
-					])).to.equal true
+					expect(@Project.findOne.calledWith({
+						'tokens.readOnly': @token
+					})).to.equal true
 					done()
 
 			it 'should call isUserInvitedMemberOfProject', (done) ->
@@ -185,10 +191,9 @@ describe "TokenAccessHandler", ->
 			it 'should call Project.findOne', (done) ->
 				@TokenAccessHandler.findProjectWithHigherAccess @token, @userId, (err, project) =>
 					expect(@Project.findOne.callCount).to.equal 1
-					expect(@Project.findOne.calledWith($or: [
-						{'tokens.readAndWrite': @token},
-						{'tokens.readOnly': @token}
-					])).to.equal true
+					expect(@Project.findOne.calledWith({
+						'tokens.readOnly': @token
+					})).to.equal true
 					done()
 
 			it 'should call isUserInvitedMemberOfProject', (done) ->