Merge pull request #4870 from overleaf/jk-bg-validate-ids

RealTime: Validate IDs
GitOrigin-RevId: 884600125d362c5632faa75dc22d957cdddc101b
This commit is contained in:
June Kelly 2021-09-01 10:10:10 +01:00 committed by Copybot
parent 224502ff40
commit 7b1044b8a8
8 changed files with 625 additions and 18 deletions

View file

@ -10,6 +10,7 @@ const HttpApiController = require('./HttpApiController')
const bodyParser = require('body-parser')
const base64id = require('base64id')
const { UnexpectedArgumentsError } = require('./Errors')
const Joi = require('@hapi/joi')
const basicAuth = require('basic-auth-connect')
const httpAuth = basicAuth(function (user, pass) {
@ -24,6 +25,11 @@ const httpAuth = basicAuth(function (user, pass) {
const HOSTNAME = require('os').hostname()
const JOI_OBJECT_ID = Joi.string()
.required()
.regex(/^[0-9a-f]{24}$/)
.message('invalid id')
let Router
module.exports = Router = {
_handleError(callback, error, client, method, attrs) {
@ -34,7 +40,21 @@ module.exports = Router = {
attrs.client_id = client.id
attrs.err = error
attrs.method = method
if (error.name === 'CodedError') {
if (Joi.isError(error)) {
logger.warn(attrs, 'validation error')
var message = 'invalid'
try {
message = error.details[0].message
} catch (e) {
// ignore unexpected errors
logger.warn({ error, e }, 'unexpected validation error')
}
const serializedError = { message }
metrics.inc('validation-error', 1, {
status: method,
})
callback(serializedError)
} else if (error.name === 'CodedError') {
logger.warn(attrs, error.message)
const serializedError = { message: error.message, code: error.info.code }
callback(serializedError)
@ -53,6 +73,8 @@ module.exports = Router = {
'not authorized',
'joinLeaveEpoch mismatch',
'doc updater could not load requested ops',
'no project_id found on client',
'cannot join multiple projects',
].includes(error.message)
) {
logger.warn(attrs, error.message)
@ -66,6 +88,11 @@ module.exports = Router = {
}
callback(serializedError)
}
if (attrs.disconnect) {
setTimeout(function () {
client.disconnect()
}, 100)
}
},
_handleInvalidArguments(client, method, args) {
@ -189,18 +216,45 @@ module.exports = Router = {
arguments
)
}
if (data.anonymousAccessToken) {
user.anonymousAccessToken = data.anonymousAccessToken
try {
Joi.assert(
data,
Joi.object({
project_id: JOI_OBJECT_ID,
anonymousAccessToken: Joi.string(),
})
)
} catch (error) {
return Router._handleError(callback, error, client, 'joinProject', {
disconnect: 1,
})
}
const { project_id, anonymousAccessToken } = data
// only allow connection to a single project
if (
client.ol_current_project_id &&
project_id !== client.ol_current_project_id
) {
return Router._handleError(
callback,
new Error('cannot join multiple projects'),
client,
'joinProject',
{ disconnect: 1 }
)
}
client.ol_current_project_id = project_id
if (anonymousAccessToken) {
user.anonymousAccessToken = anonymousAccessToken
}
WebsocketController.joinProject(
client,
user,
data.project_id,
project_id,
function (err, ...args) {
if (err) {
Router._handleError(callback, err, client, 'joinProject', {
project_id: data.project_id,
project_id: project_id,
user_id: user._id,
})
} else {
@ -253,7 +307,20 @@ module.exports = Router = {
} else {
return Router._handleInvalidArguments(client, 'joinDoc', arguments)
}
try {
Joi.assert(
{ doc_id, fromVersion, options },
Joi.object({
doc_id: JOI_OBJECT_ID,
fromVersion: Joi.number().integer(),
options: Joi.object().required(),
})
)
} catch (error) {
return Router._handleError(callback, error, client, 'joinDoc', {
disconnect: 1,
})
}
WebsocketController.joinDoc(
client,
doc_id,
@ -276,7 +343,13 @@ module.exports = Router = {
if (typeof callback !== 'function') {
return Router._handleInvalidArguments(client, 'leaveDoc', arguments)
}
try {
Joi.assert(doc_id, JOI_OBJECT_ID)
} catch (error) {
return Router._handleError(callback, error, client, 'joinDoc', {
disconnect: 1,
})
}
WebsocketController.leaveDoc(client, doc_id, function (err, ...args) {
if (err) {
Router._handleError(callback, err, client, 'leaveDoc', {
@ -352,7 +425,19 @@ module.exports = Router = {
arguments
)
}
try {
Joi.assert(
{ doc_id, update },
Joi.object({
doc_id: JOI_OBJECT_ID,
update: Joi.object().required(),
})
)
} catch (error) {
return Router._handleError(callback, error, client, 'applyOtUpdate', {
disconnect: 1,
})
}
WebsocketController.applyOtUpdate(
client,
doc_id,

View file

@ -561,6 +561,49 @@
"protobufjs": "^6.8.6"
}
},
"@hapi/address": {
"version": "4.1.0",
"resolved": "https://registry.npmjs.org/@hapi/address/-/address-4.1.0.tgz",
"integrity": "sha512-SkszZf13HVgGmChdHo/PxchnSaCJ6cetVqLzyciudzZRT0jcOouIF/Q93mgjw8cce+D+4F4C1Z/WrfFN+O3VHQ==",
"requires": {
"@hapi/hoek": "^9.0.0"
}
},
"@hapi/formula": {
"version": "2.0.0",
"resolved": "https://registry.npmjs.org/@hapi/formula/-/formula-2.0.0.tgz",
"integrity": "sha512-V87P8fv7PI0LH7LiVi8Lkf3x+KCO7pQozXRssAHNXXL9L1K+uyu4XypLXwxqVDKgyQai6qj3/KteNlrqDx4W5A=="
},
"@hapi/hoek": {
"version": "9.2.0",
"resolved": "https://registry.npmjs.org/@hapi/hoek/-/hoek-9.2.0.tgz",
"integrity": "sha512-sqKVVVOe5ivCaXDWivIJYVSaEgdQK9ul7a4Kity5Iw7u9+wBAPbX1RMSnLLmp7O4Vzj0WOWwMAJsTL00xwaNug=="
},
"@hapi/joi": {
"version": "17.1.1",
"resolved": "https://registry.npmjs.org/@hapi/joi/-/joi-17.1.1.tgz",
"integrity": "sha512-p4DKeZAoeZW4g3u7ZeRo+vCDuSDgSvtsB/NpfjXEHTUjSeINAi/RrVOWiVQ1isaoLzMvFEhe8n5065mQq1AdQg==",
"requires": {
"@hapi/address": "^4.0.1",
"@hapi/formula": "^2.0.0",
"@hapi/hoek": "^9.0.0",
"@hapi/pinpoint": "^2.0.0",
"@hapi/topo": "^5.0.0"
}
},
"@hapi/pinpoint": {
"version": "2.0.0",
"resolved": "https://registry.npmjs.org/@hapi/pinpoint/-/pinpoint-2.0.0.tgz",
"integrity": "sha512-vzXR5MY7n4XeIvLpfl3HtE3coZYO4raKXW766R6DZw/6aLqR26iuZ109K7a0NtF2Db0jxqh7xz2AxkUwpUFybw=="
},
"@hapi/topo": {
"version": "5.1.0",
"resolved": "https://registry.npmjs.org/@hapi/topo/-/topo-5.1.0.tgz",
"integrity": "sha512-foQZKJig7Ob0BMAYBfcJk8d77QtOe7Wo4ox7ff1lQYoNNAb6jwcY1ncdoy2e9wQZzvNy7ODZCYJkK8kzmcAnAg==",
"requires": {
"@hapi/hoek": "^9.0.0"
}
},
"@humanwhocodes/config-array": {
"version": "0.5.0",
"resolved": "https://registry.npmjs.org/@humanwhocodes/config-array/-/config-array-0.5.0.tgz",
@ -1241,6 +1284,7 @@
"requires": {
"anymatch": "~3.1.1",
"braces": "~3.0.2",
"fsevents": "~2.3.1",
"glob-parent": "~5.1.0",
"is-binary-path": "~2.1.0",
"is-glob": "~4.0.1",
@ -2130,7 +2174,18 @@
"version": "1.4.0",
"resolved": "https://registry.npmjs.org/esquery/-/esquery-1.4.0.tgz",
"integrity": "sha512-cCDispWt5vHHtwMY2YrAQ4ibFkAL8RbH5YGBnZBc90MolvvfkkQcJro/aZiAQUlQ3qgrYS6D6v8Gc5G5CQsc9w==",
"dev": true,
"requires": {
"estraverse": "^5.1.0"
},
"dependencies": {
"estraverse": {
"version": "5.2.0",
"resolved": "https://registry.npmjs.org/estraverse/-/estraverse-5.2.0.tgz",
"integrity": "sha512-BxbNGGNm0RyRYvUdHpIwv9IWzeM9XClbOxwoATuFdOE7ZE6wHL+HQ5T8hoPM+zHvmKzzsEqhgy0GrQ5X13afiQ==",
"dev": true
}
}
},
"esrecurse": {
"version": "4.3.0",
@ -2378,7 +2433,33 @@
"integrity": "sha512-dm9s5Pw7Jc0GvMYbshN6zchCA9RgQlzzEZX3vylR9IqFfS8XciblUXOKfW6SiuJ0e13eDYZoZV5wdrev7P3Nwg==",
"dev": true,
"requires": {
"flatted": "^3.1.0"
"flatted": "^3.1.0",
"rimraf": "^3.0.2"
},
"dependencies": {
"glob": {
"version": "7.1.7",
"resolved": "https://registry.npmjs.org/glob/-/glob-7.1.7.tgz",
"integrity": "sha512-OvD9ENzPLbegENnYP5UUfJIirTg4+XwMWGaQfQTY0JenxNvvIKP3U3/tAQSPIu/lHxXYSZmpXlUHeqAIdKzBLQ==",
"dev": true,
"requires": {
"fs.realpath": "^1.0.0",
"inflight": "^1.0.4",
"inherits": "2",
"minimatch": "^3.0.4",
"once": "^1.3.0",
"path-is-absolute": "^1.0.0"
}
},
"rimraf": {
"version": "3.0.2",
"resolved": "https://registry.npmjs.org/rimraf/-/rimraf-3.0.2.tgz",
"integrity": "sha512-JZkJMZkAGFFPP2YqXZXPbMlMBgsxzE8ILs4lMIX/2o0L9UBw9O/Y3o6wFw/i9YLapcUJWwqbi3kdxIPdC62TIA==",
"dev": true,
"requires": {
"glob": "^7.1.3"
}
}
}
},
"flatted": {
@ -2425,6 +2506,13 @@
"resolved": "https://registry.npmjs.org/fs.realpath/-/fs.realpath-1.0.0.tgz",
"integrity": "sha512-OO0pH2lK6a0hZnAdau5ItzHPI6pUlvI7jMVnxUQRtw4owF2wk8lOSabtGDCTP4Ggrg2MbGnWO9X8K1t4+fGMDw=="
},
"fsevents": {
"version": "2.3.2",
"resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.2.tgz",
"integrity": "sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==",
"dev": true,
"optional": true
},
"function-bind": {
"version": "1.1.1",
"resolved": "https://registry.npmjs.org/function-bind/-/function-bind-1.1.1.tgz",
@ -4871,6 +4959,24 @@
"version": "4.3.0",
"resolved": "https://registry.npmjs.org/ansi-styles/-/ansi-styles-4.3.0.tgz",
"integrity": "sha512-zbB9rCJAT1rbjiVDb2hqKFHNYLxgtk8NURxZ3IZwD3F6NtxbXZQCnnSi1Lkx+IDohdPlFp222wVALIheZJQSEg==",
"dev": true,
"requires": {
"color-convert": "^2.0.1"
}
},
"color-convert": {
"version": "2.0.1",
"resolved": "https://registry.npmjs.org/color-convert/-/color-convert-2.0.1.tgz",
"integrity": "sha512-RRECPsj7iu/xb5oKYcsFHSppFNnsj/52OVTRKb4zP5onXwVF3zVmmToNcOfGC+CRDpfK/U584fMg38ZHCaElKQ==",
"dev": true,
"requires": {
"color-name": "~1.1.4"
}
},
"color-name": {
"version": "1.1.4",
"resolved": "https://registry.npmjs.org/color-name/-/color-name-1.1.4.tgz",
"integrity": "sha512-dOy+3AuW3a2wNbZHIuMZpTcgjGuLU/uBL/ubcZF9OXbDo8ff4O8yVp5Bf0efS8uEoYo5q4Fx7dY9OgQGXgAsQA==",
"dev": true
}
}
@ -5111,9 +5217,16 @@
"dev": true,
"requires": {
"fast-deep-equal": "^3.1.1",
"json-schema-traverse": "^1.0.0",
"require-from-string": "^2.0.2",
"uri-js": "^4.2.2"
}
},
"json-schema-traverse": {
"version": "1.0.0",
"resolved": "https://registry.npmjs.org/json-schema-traverse/-/json-schema-traverse-1.0.0.tgz",
"integrity": "sha512-NM8/P9n3XjXhIZn1lLhkFaACTOURQXjWhV4BA/RnOv8xvgqtqpAX9IO4mRQxSx1Rlo4tqzeqb0sOlruaOy3dug==",
"dev": true
}
}
},
@ -5492,8 +5605,35 @@
"integrity": "sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q==",
"dev": true,
"requires": {
"ansi-styles": "^4.0.0",
"string-width": "^4.1.0",
"strip-ansi": "^6.0.0"
},
"dependencies": {
"ansi-styles": {
"version": "4.3.0",
"resolved": "https://registry.npmjs.org/ansi-styles/-/ansi-styles-4.3.0.tgz",
"integrity": "sha512-zbB9rCJAT1rbjiVDb2hqKFHNYLxgtk8NURxZ3IZwD3F6NtxbXZQCnnSi1Lkx+IDohdPlFp222wVALIheZJQSEg==",
"dev": true,
"requires": {
"color-convert": "^2.0.1"
}
},
"color-convert": {
"version": "2.0.1",
"resolved": "https://registry.npmjs.org/color-convert/-/color-convert-2.0.1.tgz",
"integrity": "sha512-RRECPsj7iu/xb5oKYcsFHSppFNnsj/52OVTRKb4zP5onXwVF3zVmmToNcOfGC+CRDpfK/U584fMg38ZHCaElKQ==",
"dev": true,
"requires": {
"color-name": "~1.1.4"
}
},
"color-name": {
"version": "1.1.4",
"resolved": "https://registry.npmjs.org/color-name/-/color-name-1.1.4.tgz",
"integrity": "sha512-dOy+3AuW3a2wNbZHIuMZpTcgjGuLU/uBL/ubcZF9OXbDo8ff4O8yVp5Bf0efS8uEoYo5q4Fx7dY9OgQGXgAsQA==",
"dev": true
}
}
},
"wrappy": {

View file

@ -22,6 +22,7 @@
"lint:fix": "eslint --fix ."
},
"dependencies": {
"@hapi/joi": "^17.1.1",
"@overleaf/metrics": "^3.5.1",
"@overleaf/o-error": "^3.1.0",
"@overleaf/redis-wrapper": "^2.0.0",

View file

@ -359,7 +359,7 @@ describe('applyOtUpdate', function () {
})
})
return describe('when authorized to read-only with a comment update', function () {
describe('when authorized to read-only with a comment update', function () {
before(function (done) {
this.comment_update = {
op: [{ c: 'foo', p: 42 }],
@ -472,4 +472,176 @@ describe('applyOtUpdate', function () {
)
})
})
describe('when authorized with an edit update to an invalid doc', function () {
before(function (done) {
return async.series(
[
cb => {
return FixturesManager.setUpProject(
{
privilegeLevel: 'readOnly',
},
(e, { project_id, user_id }) => {
this.project_id = project_id
this.user_id = user_id
return cb(e)
}
)
},
cb => {
return FixturesManager.setUpDoc(
this.project_id,
{ lines: this.lines, version: this.version, ops: this.ops },
(e, { doc_id }) => {
this.doc_id = doc_id
return cb(e)
}
)
},
cb => {
this.client = RealTimeClient.connect()
return this.client.on('connectionAccepted', cb)
},
cb => {
return this.client.emit(
'joinProject',
{ project_id: this.project_id },
cb
)
},
cb => {
return this.client.emit('joinDoc', this.doc_id, cb)
},
cb => {
return this.client.emit(
'applyOtUpdate',
'invalid-doc-id',
this.update,
error => {
this.error = error
return cb()
}
)
},
],
done
)
})
it('should return an error', function () {
return expect(this.error).to.exist
})
it('should disconnect the client', function (done) {
return setTimeout(() => {
this.client.socket.connected.should.equal(false)
return done()
}, 300)
})
return it('should not put the update in redis', function (done) {
rclient.llen(
redisSettings.documentupdater.key_schema.pendingUpdates({
doc_id: this.doc_id,
}),
(error, len) => {
len.should.equal(0)
return done()
}
)
return null
})
})
describe('when authorized with an invalid edit update', function () {
before(function (done) {
return async.series(
[
cb => {
return FixturesManager.setUpProject(
{
privilegeLevel: 'readAndWrite',
},
(e, { project_id, user_id }) => {
this.project_id = project_id
this.user_id = user_id
return cb(e)
}
)
},
cb => {
return FixturesManager.setUpDoc(
this.project_id,
{ lines: this.lines, version: this.version, ops: this.ops },
(e, { doc_id }) => {
this.doc_id = doc_id
return cb(e)
}
)
},
cb => {
this.client = RealTimeClient.connect()
return this.client.on('connectionAccepted', cb)
},
cb => {
return this.client.emit(
'joinProject',
{ project_id: this.project_id },
cb
)
},
cb => {
return this.client.emit('joinDoc', this.doc_id, cb)
},
cb => {
return this.client.emit(
'applyOtUpdate',
this.doc_id,
'invalid-update',
error => {
this.error = error
return cb()
}
)
},
],
done
)
})
it('should return an error', function () {
return expect(this.error).to.exist
})
it('should disconnect the client', function (done) {
return setTimeout(() => {
this.client.socket.connected.should.equal(false)
return done()
}, 300)
})
return it('should not put the update in redis', function (done) {
rclient.llen(
redisSettings.documentupdater.key_schema.pendingUpdates({
doc_id: this.doc_id,
}),
(error, len) => {
len.should.equal(0)
return done()
}
)
return null
})
})
})

View file

@ -292,6 +292,90 @@ describe('joinDoc', function () {
// project, since joinProject already catches that. If you can join a project,
// then you can join a doc in that project.
describe('for an invalid doc', function () {
before(function (done) {
return async.series(
[
cb => {
return FixturesManager.setUpProject(
{
privilegeLevel: 'owner',
},
(e, { project_id, user_id }) => {
this.project_id = project_id
this.user_id = user_id
return cb(e)
}
)
},
cb => {
return FixturesManager.setUpDoc(
this.project_id,
{
lines: this.lines,
version: this.version,
ops: this.ops,
ranges: this.ranges,
},
(e, { doc_id }) => {
this.doc_id = doc_id
return cb(e)
}
)
},
cb => {
this.client = RealTimeClient.connect()
return this.client.on('connectionAccepted', cb)
},
cb => {
return this.client.emit(
'joinProject',
{ project_id: this.project_id },
cb
)
},
cb => {
return this.client.emit(
'joinDoc',
'invalid-doc-id',
(error, ...rest) => {
this.error = error
return cb()
}
)
},
],
done
)
})
it('should not get the doc from the doc updater', function () {
return MockDocUpdaterServer.getDocument
.calledWith(this.project_id, 'invalid-doc-id')
.should.equal(false)
})
it('should return an invalid id error', function () {
this.error.message.should.equal('invalid id')
})
return it('should not have joined the doc room', function (done) {
return RealTimeClient.getConnectedClient(
this.client.socket.sessionid,
(error, client) => {
expect(Array.from(client.rooms).includes('invalid-doc-id')).to.equal(
false
)
return done()
}
)
})
})
describe('with a fromVersion', function () {
before(function (done) {
this.fromVersion = 36

View file

@ -181,7 +181,7 @@ describe('joinProject', function () {
cb => {
return FixturesManager.setUpProject(
{
project_id: 'forbidden',
project_id: '403403403403403403403403', // forbidden
privilegeLevel: 'owner',
project: {
name: 'Test Project',
@ -242,7 +242,7 @@ describe('joinProject', function () {
cb => {
return FixturesManager.setUpProject(
{
project_id: 'not-found',
project_id: '404404404404404404404404', // not-found
privilegeLevel: 'owner',
project: {
name: 'Test Project',
@ -296,6 +296,115 @@ describe('joinProject', function () {
})
})
describe('when invalid', function () {
before(function (done) {
MockWebServer.joinProject.resetHistory()
return async.series(
[
cb => {
this.client = RealTimeClient.connect()
return this.client.on('connectionAccepted', cb)
},
cb => {
return this.client.emit(
'joinProject',
{ project_id: 'invalid-id' },
error => {
this.error = error
return cb()
}
)
},
],
done
)
})
it('should return an invalid id error', function () {
this.error.message.should.equal('invalid id')
})
it('should not call to web', function () {
MockWebServer.joinProject.called.should.equal(false)
})
})
describe('when joining more than one project', function () {
before(function (done) {
return async.series(
[
cb => {
return FixturesManager.setUpProject(
{
privilegeLevel: 'owner',
project: {
name: 'Other Project',
},
},
(e, { project_id, user_id }) => {
this.other_project_id = project_id
this.other_user_id = user_id
return cb(e)
}
)
},
cb => {
return FixturesManager.setUpProject(
{
user_id: this.other_user_id,
privilegeLevel: 'owner',
project: {
name: 'Test Project',
},
},
(e, { project_id, user_id }) => {
this.project_id = project_id
this.user_id = user_id
return cb(e)
}
)
},
cb => {
this.client = RealTimeClient.connect()
return this.client.on('connectionAccepted', cb)
},
cb => {
return this.client.emit(
'joinProject',
{ project_id: this.project_id },
(error, project, privilegeLevel, protocolVersion) => {
this.project = project
this.privilegeLevel = privilegeLevel
this.protocolVersion = protocolVersion
return cb(error)
}
)
},
cb => {
return this.client.emit(
'joinProject',
{ project_id: this.other_project_id },
error => {
this.error = error
return cb()
}
)
},
],
done
)
})
return it('should return an error', function () {
this.error.message.should.equal('cannot join multiple projects')
})
})
return describe('when over rate limit', function () {
before(function (done) {
return async.series(
@ -308,7 +417,7 @@ describe('joinProject', function () {
cb => {
return this.client.emit(
'joinProject',
{ project_id: 'rate-limited' },
{ project_id: '429429429429429429429429' }, // rate-limited
error => {
this.error = error
return cb()

View file

@ -119,6 +119,19 @@ describe('leaveDoc', function () {
})
})
describe('then leaving an invalid doc', function () {
beforeEach(function (done) {
return this.client.emit('leaveDoc', 'bad-id', error => {
this.error = error
return done()
})
})
return it('should return an error', function () {
return expect(this.error).to.exist
})
})
describe('when sending a leaveDoc request before the previous joinDoc request has completed', function () {
beforeEach(function (done) {
this.client.emit('leaveDoc', this.doc_id, () => {})

View file

@ -39,13 +39,16 @@ module.exports = MockWebServer = {
joinProjectRequest(req, res, next) {
const { project_id } = req.params
const { user_id } = req.query
if (project_id === 'not-found') {
if (project_id === '404404404404404404404404') {
// not-found
return res.status(404).send()
}
if (project_id === 'forbidden') {
if (project_id === '403403403403403403403403') {
// forbidden
return res.status(403).send()
}
if (project_id === 'rate-limited') {
if (project_id === '429429429429429429429429') {
// rate-limited
return res.status(429).send()
} else {
return MockWebServer.joinProject(