diff --git a/services/web/app/src/Features/LinkedFiles/LinkedFilesController.mjs b/services/web/app/src/Features/LinkedFiles/LinkedFilesController.mjs index d83806136f..ee9d5c0dbf 100644 --- a/services/web/app/src/Features/LinkedFiles/LinkedFilesController.mjs +++ b/services/web/app/src/Features/LinkedFiles/LinkedFilesController.mjs @@ -60,6 +60,17 @@ const createLinkedFileSchema = z.object({ }), }) +const refreshLinkedFileSchema = z.object({ + params: z.object({ + project_id: zz.objectId(), + file_id: zz.objectId(), + }), + body: z.object({ + clientId: z.string().optional(), + shouldReindexReferences: z.boolean().optional(), + }), +}) + async function createLinkedFile(req, res, next) { const { params, body } = parseReq(req, createLinkedFileSchema) const { project_id: projectId } = params @@ -94,8 +105,9 @@ async function createLinkedFile(req, res, next) { } async function refreshLinkedFile(req, res, next) { - const { project_id: projectId, file_id: fileId } = req.params - const { clientId } = req.body + const { params, body } = parseReq(req, refreshLinkedFileSchema) + const { project_id: projectId, file_id: fileId } = params + const { clientId, shouldReindexReferences } = body const userId = SessionManager.getLoggedInUserId(req.session) const { file, parentFolder } = await LinkedFilesHandler.promises.getFileById( @@ -136,7 +148,7 @@ async function refreshLinkedFile(req, res, next) { return LinkedFilesController.handleError(err, req, res, next) } - if (req.body.shouldReindexReferences) { + if (shouldReindexReferences) { // Signal to clients that they should re-index references EditorRealTimeController.emitToRoom( projectId, diff --git a/services/web/app/src/Features/LinkedFiles/LinkedFilesHandler.mjs b/services/web/app/src/Features/LinkedFiles/LinkedFilesHandler.mjs index be0836edab..3d0c4a6caf 100644 --- a/services/web/app/src/Features/LinkedFiles/LinkedFilesHandler.mjs +++ b/services/web/app/src/Features/LinkedFiles/LinkedFilesHandler.mjs @@ -1,4 +1,5 @@ import fs from 'node:fs' +import mongodb from 'mongodb-legacy' import FileWriter from '../../infrastructure/FileWriter.mjs' import EditorController from '../Editor/EditorController.mjs' import ProjectLocator from '../Project/ProjectLocator.mjs' @@ -7,6 +8,8 @@ import ProjectGetter from '../Project/ProjectGetter.mjs' import LinkedFilesErrors from './LinkedFilesErrors.mjs' import { callbackifyAll } from '@overleaf/promise-utils' +const { ObjectId } = mongodb + const { ProjectNotFoundError, V1ProjectNotFoundError, BadDataError } = LinkedFilesErrors @@ -25,6 +28,13 @@ const LinkedFilesHandler = { async getSourceProject(data) { const projection = { _id: 1, name: 1, overleaf: 1 } // include the historyId for future use if (data.v1_source_doc_id != null) { + // v1_source_doc_id must be a string or a number. + if ( + typeof data.v1_source_doc_id !== 'string' && + typeof data.v1_source_doc_id !== 'number' + ) { + throw new BadDataError('invalid v1_source_doc_id') + } const project = await Project.findOne( { 'overleaf.id': data.v1_source_doc_id }, projection @@ -36,6 +46,13 @@ const LinkedFilesHandler = { return project } else if (data.source_project_id != null) { + // source_project_id must be a plain ObjectId string. + if ( + typeof data.source_project_id !== 'string' || + !ObjectId.isValid(data.source_project_id) + ) { + throw new BadDataError('invalid source_project_id') + } const project = await ProjectGetter.promises.getProject( data.source_project_id, projection diff --git a/services/web/test/acceptance/src/LinkedFilesTests.mjs b/services/web/test/acceptance/src/LinkedFilesTests.mjs index cc25097d3e..5afe3b5d6a 100644 --- a/services/web/test/acceptance/src/LinkedFilesTests.mjs +++ b/services/web/test/acceptance/src/LinkedFilesTests.mjs @@ -222,7 +222,7 @@ describe('LinkedFiles', function () { v1_source_doc_id: 9999999, // We won't find this id in the database source_entity_path: 'example.jpeg', }, - _id: 'abcd', + _id: '000000000000000000000abc', rev: 0, created: new Date(), name: 'example.jpeg', @@ -232,7 +232,7 @@ describe('LinkedFiles', function () { it('should refuse to refresh', async function () { const { response, body } = await owner.doRequest('post', { - url: `/project/${projectOneId}/linked_file/abcd/refresh`, + url: `/project/${projectOneId}/linked_file/000000000000000000000abc/refresh`, json: true, }) expect(response.statusCode).to.equal(409) @@ -505,7 +505,7 @@ describe('LinkedFiles', function () { v1_source_doc_id: 9999999, // We won't find this id in the database source_output_file_path: 'output.pdf', }, - _id: 'abcdef', + _id: '000000000000000000abcdef', rev: 0, created: new Date(), name: 'whatever.pdf', @@ -515,7 +515,7 @@ describe('LinkedFiles', function () { it('should refuse to refresh', async function () { const { response, body } = await owner.doRequest('post', { - url: `/project/${projectOneId}/linked_file/abcdef/refresh`, + url: `/project/${projectOneId}/linked_file/000000000000000000abcdef/refresh`, json: true, }) expect(response.statusCode).to.equal(409) diff --git a/services/web/test/unit/src/LinkedFiles/LinkedFilesController.test.mjs b/services/web/test/unit/src/LinkedFiles/LinkedFilesController.test.mjs index 1c4adddea0..b2ae3749cc 100644 --- a/services/web/test/unit/src/LinkedFiles/LinkedFilesController.test.mjs +++ b/services/web/test/unit/src/LinkedFiles/LinkedFilesController.test.mjs @@ -21,7 +21,10 @@ describe('LinkedFilesController', function () { refreshLinkedFile: sinon.stub().resolves(), }, } - ctx.projectId = 'projectId' + // project_id, parent_folder_id and file_id are validated as Mongo + // ObjectIds, so use well-formed values throughout. + ctx.projectId = '507f1f77bcf86cd799439011' + ctx.parentFolderId = '507f191e810c19729de860eb' ctx.provider = 'provider' ctx.fileName = 'linked-file-name' ctx.data = { customAgentData: 'foo' } @@ -119,21 +122,30 @@ describe('LinkedFilesController', function () { name: ctx.fileName, provider: ctx.provider, data: ctx.data, + parent_folder_id: ctx.parentFolderId, }, } ctx.next = sinon.stub() }) it('sets importedAt timestamp on linkedFileData', async function (ctx) { - await new Promise(resolve => { - ctx.next = sinon.stub().callsFake(() => resolve('unexpected error')) + await new Promise((resolve, reject) => { + ctx.next = sinon + .stub() + .callsFake(err => + reject(err || new Error('next called unexpectedly')) + ) ctx.res = { json: () => { expect(ctx.Agent.promises.createLinkedFile).to.have.been.calledWith( ctx.projectId, - { ...ctx.data, importedAt: ctx.fakeTime.toISOString() }, + { + customAgentData: 'foo', + provider: ctx.provider, + importedAt: ctx.fakeTime.toISOString(), + }, ctx.fileName, - undefined, + ctx.parentFolderId, ctx.userId ) resolve() @@ -145,6 +157,7 @@ describe('LinkedFilesController', function () { }) describe('refreshLinkedFiles', function () { beforeEach(function (ctx) { + ctx.fileId = '507f191e810c19729de860ea' ctx.data.provider = ctx.provider ctx.file = { name: ctx.fileName, @@ -154,7 +167,7 @@ describe('LinkedFilesController', function () { }, } ctx.LinkedFilesHandler.promises.getFileById - .withArgs(ctx.projectId, 'file-id') + .withArgs(ctx.projectId, ctx.fileId) .resolves({ file: ctx.file, path: 'fake-path', @@ -163,15 +176,19 @@ describe('LinkedFilesController', function () { }, }) ctx.req = { - params: { project_id: ctx.projectId, file_id: 'file-id' }, + params: { project_id: ctx.projectId, file_id: ctx.fileId }, body: {}, } ctx.next = sinon.stub() }) it('resets importedAt timestamp on linkedFileData', async function (ctx) { - await new Promise(resolve => { - ctx.next = sinon.stub().callsFake(() => resolve('unexpected error')) + await new Promise((resolve, reject) => { + ctx.next = sinon + .stub() + .callsFake(err => + reject(err || new Error('next called unexpectedly')) + ) ctx.res = { json: () => { expect( @@ -182,7 +199,7 @@ describe('LinkedFilesController', function () { ...ctx.data, importedAt: ctx.fakeTime.toISOString(), }, - ctx.name, + ctx.fileName, 'parent-folder-id', ctx.userId ) @@ -193,6 +210,21 @@ describe('LinkedFilesController', function () { }) }) + it('rejects invalid params without calling the agent', async function (ctx) { + ctx.req.params.file_id = 'not-an-object-id' + await new Promise(resolve => { + ctx.next = sinon.stub().callsFake(() => resolve()) + ctx.res = { + json: () => resolve(), + sendStatus: () => resolve(), + } + ctx.LinkedFilesController.refreshLinkedFile(ctx.req, ctx.res, ctx.next) + }) + expect(ctx.next).to.have.been.calledOnce + expect(ctx.next.firstCall.args[0]?.name).to.equal('InvalidParamsError') + expect(ctx.Agent.promises.refreshLinkedFile).to.not.have.been.called + }) + describe('when bib file re-indexing is required', function () { const clientId = 'client-id' beforeEach(function (ctx) { @@ -201,8 +233,12 @@ describe('LinkedFilesController', function () { }) it('informs clients to re-index bib references', async function (ctx) { - await new Promise(resolve => { - ctx.next = sinon.stub().callsFake(() => resolve('unexpected error')) + await new Promise((resolve, reject) => { + ctx.next = sinon + .stub() + .callsFake(err => + reject(err || new Error('next called unexpectedly')) + ) ctx.res = { json: () => { expect( diff --git a/services/web/test/unit/src/LinkedFiles/LinkedFilesHandler.test.mjs b/services/web/test/unit/src/LinkedFiles/LinkedFilesHandler.test.mjs new file mode 100644 index 0000000000..bb34a0d7ab --- /dev/null +++ b/services/web/test/unit/src/LinkedFiles/LinkedFilesHandler.test.mjs @@ -0,0 +1,127 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import sinon from 'sinon' + +const modulePath = + '../../../../app/src/Features/LinkedFiles/LinkedFilesHandler.mjs' + +// The module under test is imported dynamically in each test, so it resolves +// its own copy of the error classes. Assert on the stable `name` property +// rather than `instanceof`, which would compare against a different class +// instance. +async function getRejection(promise) { + try { + await promise + return null + } catch (error) { + return error + } +} + +describe('LinkedFilesHandler', function () { + beforeEach(async function (ctx) { + ctx.projectId = '507f1f77bcf86cd799439011' + ctx.project = { _id: ctx.projectId, name: 'source project' } + + ctx.Project = { + findOne: sinon.stub().returns({ exec: sinon.stub().resolves(null) }), + } + ctx.ProjectGetter = { + promises: { + getProject: sinon.stub().resolves(ctx.project), + }, + } + + vi.doMock('../../../../app/src/models/Project', () => ({ + Project: ctx.Project, + })) + vi.doMock('../../../../app/src/Features/Project/ProjectGetter', () => ({ + default: ctx.ProjectGetter, + })) + vi.doMock('../../../../app/src/infrastructure/FileWriter', () => ({ + default: {}, + })) + vi.doMock('../../../../app/src/Features/Editor/EditorController', () => ({ + default: {}, + })) + vi.doMock('../../../../app/src/Features/Project/ProjectLocator', () => ({ + default: {}, + })) + + ctx.LinkedFilesHandler = (await import(modulePath)).default + }) + + describe('getSourceProject', function () { + describe('with a source_project_id', function () { + it('rejects a source_project_id that is not a string', async function (ctx) { + const error = await getRejection( + ctx.LinkedFilesHandler.promises.getSourceProject({ + source_project_id: { _id: ctx.projectId, other: { $regex: '^6' } }, + source_entity_path: 'main.tex', + }) + ) + expect(error?.name).to.equal('BadDataError') + expect(ctx.ProjectGetter.promises.getProject).to.not.have.been.called + }) + + it('rejects a non-ObjectId string', async function (ctx) { + const error = await getRejection( + ctx.LinkedFilesHandler.promises.getSourceProject({ + source_project_id: 'not-an-object-id', + source_entity_path: 'main.tex', + }) + ) + expect(error?.name).to.equal('BadDataError') + expect(ctx.ProjectGetter.promises.getProject).to.not.have.been.called + }) + + it('passes a valid ObjectId string through to getProject', async function (ctx) { + const project = await ctx.LinkedFilesHandler.promises.getSourceProject({ + source_project_id: ctx.projectId, + source_entity_path: 'main.tex', + }) + expect(project).to.equal(ctx.project) + expect(ctx.ProjectGetter.promises.getProject).to.have.been.calledWith( + ctx.projectId + ) + }) + + it('rejects with ProjectNotFoundError when no project matches', async function (ctx) { + ctx.ProjectGetter.promises.getProject.resolves(null) + const error = await getRejection( + ctx.LinkedFilesHandler.promises.getSourceProject({ + source_project_id: ctx.projectId, + source_entity_path: 'main.tex', + }) + ) + expect(error?.name).to.equal('ProjectNotFoundError') + }) + }) + + describe('with a v1_source_doc_id', function () { + it('rejects a v1_source_doc_id that is not a string or number', async function (ctx) { + const error = await getRejection( + ctx.LinkedFilesHandler.promises.getSourceProject({ + v1_source_doc_id: { other: '1' }, + source_entity_path: 'main.tex', + }) + ) + expect(error?.name).to.equal('BadDataError') + expect(ctx.Project.findOne).to.not.have.been.called + }) + + it('passes a numeric id through to Project.findOne', async function (ctx) { + ctx.Project.findOne.returns({ + exec: sinon.stub().resolves(ctx.project), + }) + const project = await ctx.LinkedFilesHandler.promises.getSourceProject({ + v1_source_doc_id: 1234, + source_entity_path: 'main.tex', + }) + expect(project).to.equal(ctx.project) + expect(ctx.Project.findOne).to.have.been.calledWith({ + 'overleaf.id': 1234, + }) + }) + }) + }) +})