From d8de261370d2e775b74141dbc1ade5f8f0955008 Mon Sep 17 00:00:00 2001 From: Domagoj Kriskovic Date: Wed, 22 Jul 2026 14:31:47 +0200 Subject: [PATCH] Add request validation to HistoryController endpoints (#35730) * Add schemas for file restoration and label management in HistoryController * Add request schemas for proxying to history API and resyncing project history * prevent transforming version:null into version:0 with coerce * move jsDoc comments above the handler * check for undefined resyncProjectStructureOnly * Add tests for restoreFileFromV2 and revertFile * Set default value for resyncProjectStructureOnly in resyncProjectHistorySchema GitOrigin-RevId: 7b8b8a4c3834b8c8a83f93052ab1f59e86c529a6 --- .../Features/History/HistoryController.mjs | 118 +++++++++-- .../src/History/HistoryController.test.mjs | 190 +++++++++++++++++- 2 files changed, 286 insertions(+), 22 deletions(-) diff --git a/services/web/app/src/Features/History/HistoryController.mjs b/services/web/app/src/Features/History/HistoryController.mjs index be9e01c006..81fd7bc352 100644 --- a/services/web/app/src/Features/History/HistoryController.mjs +++ b/services/web/app/src/Features/History/HistoryController.mjs @@ -139,12 +139,22 @@ function setBlobCacheHeaders(res, etag) { res.set('ETag', etag) } +const proxyToHistoryApiSchema = z.object({ + // both project_id and Project_id are accepted for backwards compatibility + params: z.object({ + Project_id: zz.objectId().optional(), + project_id: zz.objectId().optional(), + doc_id: zz.objectId().optional(), + }), +}) + /** * @param {any} req * @param {any} res * @param {any} next */ async function proxyToHistoryApi(req, res, next) { + parseReq(req, proxyToHistoryApiSchema) const userId = SessionManager.getLoggedInUserId(req.session) const url = settings.apis.project_history.url + req.url @@ -173,12 +183,19 @@ async function proxyToHistoryApi(req, res, next) { } } +const proxyToHistoryApiAndInjectUserDetailsSchema = z.object({ + params: z.object({ + Project_id: zz.objectId(), + }), +}) + /** * @param {any} req * @param {any} res * @param {any} next */ async function proxyToHistoryApiAndInjectUserDetails(req, res, next) { + parseReq(req, proxyToHistoryApiAndInjectUserDetailsSchema) const userId = SessionManager.getLoggedInUserId(req.session) const url = settings.apis.project_history.url + req.url const body = await fetchJson(url, { @@ -189,6 +206,16 @@ async function proxyToHistoryApiAndInjectUserDetails(req, res, next) { res.json(data) } +const resyncProjectHistorySchema = z.object({ + params: z.object({ + Project_id: zz.objectId(), + }), + body: z.object({ + historyRangesMigration: z.enum(['forwards', 'backwards']).optional(), + resyncProjectStructureOnly: z.boolean().default(false), + }), +}) + /** * @param {any} req * @param {any} res @@ -197,14 +224,11 @@ async function proxyToHistoryApiAndInjectUserDetails(req, res, next) { async function resyncProjectHistory(req, res, next) { // increase timeout to 6 minutes res.setTimeout(6 * 60 * 1000) - const projectId = req.params.Project_id - const opts = {} - const historyRangesMigration = req.body.historyRangesMigration - if (historyRangesMigration) { - opts.historyRangesMigration = historyRangesMigration - } - if (req.body.resyncProjectStructureOnly) { - opts.resyncProjectStructureOnly = req.body.resyncProjectStructureOnly + const { params, body } = parseReq(req, resyncProjectHistorySchema) + const projectId = params.Project_id + const opts = { + historyRangesMigration: body.historyRangesMigration, + resyncProjectStructureOnly: body.resyncProjectStructureOnly, } try { @@ -223,14 +247,25 @@ async function resyncProjectHistory(req, res, next) { res.sendStatus(204) } +const restoreFileFromV2Schema = z.object({ + params: z.object({ + project_id: zz.objectId(), + }), + body: z.object({ + version: z.number().int().min(0), + pathname: zz.filepath(), + }), +}) + /** * @param {any} req * @param {any} res * @param {any} next */ async function restoreFileFromV2(req, res, next) { - const { project_id: projectId } = req.params - const { version, pathname } = req.body + const { params, body } = parseReq(req, restoreFileFromV2Schema) + const { project_id: projectId } = params + const { version, pathname } = body const userId = SessionManager.getLoggedInUserId(req.session) const entity = await RestoreManager.promises.restoreFileFromV2( @@ -259,14 +294,25 @@ async function restoreFileFromV2(req, res, next) { }) } +const revertFileSchema = z.object({ + params: z.object({ + project_id: zz.objectId(), + }), + body: z.object({ + version: z.number().int().min(0), + pathname: zz.filepath(), + }), +}) + /** * @param {any} req * @param {any} res * @param {any} next */ async function revertFile(req, res, next) { - const { project_id: projectId } = req.params - const { version, pathname } = req.body + const { params, body } = parseReq(req, revertFileSchema) + const { project_id: projectId } = params + const { version, pathname } = body const userId = SessionManager.getLoggedInUserId(req.session) const entity = await RestoreManager.promises.revertFile( @@ -296,14 +342,24 @@ async function revertFile(req, res, next) { }) } +const revertProjectSchema = z.object({ + params: z.object({ + project_id: zz.objectId(), + }), + body: z.object({ + version: z.number().int().min(0), + }), +}) + /** * @param {any} req * @param {any} res * @param {any} next */ async function revertProject(req, res, next) { - const { project_id: projectId } = req.params - const { version } = req.body + const { params, body } = parseReq(req, revertProjectSchema) + const { project_id: projectId } = params + const { version } = body const userId = SessionManager.getLoggedInUserId(req.session) const reverted = await RestoreManager.promises.revertProject( @@ -327,13 +383,20 @@ async function revertProject(req, res, next) { res.json(reverted) } +const getLabelsSchema = z.object({ + params: z.object({ + Project_id: zz.objectId(), + }), +}) + /** * @param {any} req * @param {any} res * @param {any} next */ async function getLabels(req, res, next) { - const projectId = req.params.Project_id + const { params } = parseReq(req, getLabelsSchema) + const projectId = params.Project_id let labels = await fetchJson( `${settings.apis.project_history.url}/project/${projectId}/labels` @@ -343,14 +406,25 @@ async function getLabels(req, res, next) { res.json(labels) } +const createLabelSchema = z.object({ + params: z.object({ + Project_id: zz.objectId(), + }), + body: z.object({ + comment: z.string(), + version: z.number().int().min(0), + }), +}) + /** * @param {any} req * @param {any} res * @param {any} next */ async function createLabel(req, res, next) { - const projectId = req.params.Project_id - const { comment, version } = req.body + const { params, body } = parseReq(req, createLabelSchema) + const projectId = params.Project_id + const { comment, version } = body const userId = SessionManager.getLoggedInUserId(req.session) let label = await fetchJson( @@ -444,13 +518,21 @@ function _displayNameForUser(user) { return name } +const deleteLabelSchema = z.object({ + params: z.object({ + Project_id: zz.objectId(), + label_id: zz.objectId(), + }), +}) + /** * @param {any} req * @param {any} res * @param {any} next */ async function deleteLabel(req, res, next) { - const { Project_id: projectId, label_id: labelId } = req.params + const { params } = parseReq(req, deleteLabelSchema) + const { Project_id: projectId, label_id: labelId } = params const userId = SessionManager.getLoggedInUserId(req.session) const project = await ProjectGetter.promises.getProject(projectId, { diff --git a/services/web/test/unit/src/History/HistoryController.test.mjs b/services/web/test/unit/src/History/HistoryController.test.mjs index f98a2e7836..9e38d16402 100644 --- a/services/web/test/unit/src/History/HistoryController.test.mjs +++ b/services/web/test/unit/src/History/HistoryController.test.mjs @@ -119,7 +119,17 @@ describe('HistoryController', function () { vi.doMock( '../../../../app/src/Features/History/RestoreManager.mjs', () => ({ - default: (ctx.RestoreManager = {}), + default: (ctx.RestoreManager = { + promises: { + restoreFileFromV2: sinon + .stub() + .resolves({ _id: 'restored-id', type: 'doc' }), + revertFile: sinon + .stub() + .resolves({ _id: 'reverted-id', type: 'doc' }), + revertProject: sinon.stub().resolves([]), + }, + }), }) ) @@ -137,7 +147,12 @@ describe('HistoryController', function () { describe('proxyToHistoryApi', function () { beforeEach(async function (ctx) { - ctx.req = { url: '/mock/url', method: 'POST', session: sinon.stub() } + ctx.req = { + url: '/mock/url', + method: 'POST', + session: sinon.stub(), + params: {}, + } ctx.res = { set: sinon.stub(), } @@ -188,7 +203,11 @@ describe('HistoryController', function () { describe('proxyToHistoryApiAndInjectUserDetails', function () { beforeEach(async function (ctx) { - ctx.req = { url: '/mock/url', method: 'POST' } + ctx.req = { + url: '/mock/url', + method: 'POST', + params: { Project_id: ctx.project_id }, + } ctx.res = { json: sinon.stub() } ctx.data = 'mock-data' ctx.dataWithUsers = 'mock-injected-data' @@ -233,7 +252,11 @@ describe('HistoryController', function () { describe('proxyToHistoryApiAndInjectUserDetails (with the history API failing)', function () { beforeEach(async function (ctx) { ctx.url = '/mock/url' - ctx.req = { url: ctx.url, method: 'POST' } + ctx.req = { + url: ctx.url, + method: 'POST', + params: { Project_id: ctx.project_id }, + } ctx.res = { json: sinon.stub() } ctx.err = new RequestFailedError(ctx.url, {}, { status: 500 }) ctx.fetchJson.rejects(ctx.err) @@ -308,6 +331,165 @@ describe('HistoryController', function () { }) }) + describe('restoreFileFromV2', function () { + beforeEach(function (ctx) { + ctx.res = { json: sinon.stub() } + ctx.ProjectAuditLogHandler.addEntryIfManagedInBackground = sinon.stub() + }) + + it('restores the file for a valid request', async function (ctx) { + ctx.req = { + params: { project_id: ctx.project_id }, + body: { version: 42, pathname: 'foo/bar.tex' }, + session: {}, + ip: '1.2.3.4', + } + await ctx.HistoryController.restoreFileFromV2(ctx.req, ctx.res, ctx.next) + ctx.RestoreManager.promises.restoreFileFromV2.should.have.been.calledWith( + ctx.user_id, + ctx.project_id, + 42, + 'foo/bar.tex' + ) + }) + + it('rejects a non-numeric version without invoking RestoreManager', async function (ctx) { + ctx.req = { + params: { project_id: ctx.project_id }, + body: { version: 'not-a-number', pathname: 'foo/bar.tex' }, + session: {}, + ip: '1.2.3.4', + } + await ctx.HistoryController.restoreFileFromV2(ctx.req, ctx.res, ctx.next) + ctx.next.should.have.been.calledOnce + ctx.next.firstCall.args[0].should.be.an.instanceof(Error) + ctx.RestoreManager.promises.restoreFileFromV2.should.not.have.been.called + }) + + it('rejects a negative version without invoking RestoreManager', async function (ctx) { + ctx.req = { + params: { project_id: ctx.project_id }, + body: { version: -1, pathname: 'foo/bar.tex' }, + session: {}, + ip: '1.2.3.4', + } + await ctx.HistoryController.restoreFileFromV2(ctx.req, ctx.res, ctx.next) + ctx.next.should.have.been.calledOnce + ctx.next.firstCall.args[0].should.be.an.instanceof(Error) + ctx.RestoreManager.promises.restoreFileFromV2.should.not.have.been.called + }) + + it('rejects a pathname containing .. without invoking RestoreManager', async function (ctx) { + ctx.req = { + params: { project_id: ctx.project_id }, + body: { version: 42, pathname: '../../etc/passwd' }, + session: {}, + ip: '1.2.3.4', + } + await ctx.HistoryController.restoreFileFromV2(ctx.req, ctx.res, ctx.next) + ctx.next.should.have.been.calledOnce + ctx.next.firstCall.args[0].should.be.an.instanceof(Error) + ctx.RestoreManager.promises.restoreFileFromV2.should.not.have.been.called + }) + + it('rejects an absolute pathname without invoking RestoreManager', async function (ctx) { + ctx.req = { + params: { project_id: ctx.project_id }, + body: { version: 42, pathname: '/etc/passwd' }, + session: {}, + ip: '1.2.3.4', + } + await ctx.HistoryController.restoreFileFromV2(ctx.req, ctx.res, ctx.next) + ctx.next.should.have.been.calledOnce + ctx.next.firstCall.args[0].should.be.an.instanceof(Error) + ctx.RestoreManager.promises.restoreFileFromV2.should.not.have.been.called + }) + + it('rejects a path-traversal string as version without invoking RestoreManager', async function (ctx) { + ctx.req = { + params: { project_id: ctx.project_id }, + body: { version: '0/../../../ObjectId', pathname: 'foo/bar.tex' }, + session: {}, + ip: '1.2.3.4', + } + await ctx.HistoryController.restoreFileFromV2(ctx.req, ctx.res, ctx.next) + ctx.next.should.have.been.calledOnce + ctx.next.firstCall.args[0].should.be.an.instanceof(Error) + ctx.RestoreManager.promises.restoreFileFromV2.should.not.have.been.called + }) + }) + + describe('revertFile', function () { + beforeEach(function (ctx) { + ctx.res = { json: sinon.stub() } + ctx.ProjectAuditLogHandler.addEntryIfManagedInBackground = sinon.stub() + }) + + it('reverts the file for a valid request', async function (ctx) { + ctx.req = { + params: { project_id: ctx.project_id }, + body: { version: 42, pathname: 'foo/bar.tex' }, + session: {}, + ip: '1.2.3.4', + } + await ctx.HistoryController.revertFile(ctx.req, ctx.res, ctx.next) + ctx.RestoreManager.promises.revertFile.should.have.been.calledWith( + ctx.user_id, + ctx.project_id, + 42, + 'foo/bar.tex' + ) + }) + + it('rejects a malformed pathname without invoking RestoreManager', async function (ctx) { + ctx.req = { + params: { project_id: ctx.project_id }, + body: { version: 42, pathname: '../secret.tex' }, + session: {}, + ip: '1.2.3.4', + } + await ctx.HistoryController.revertFile(ctx.req, ctx.res, ctx.next) + ctx.next.should.have.been.calledOnce + ctx.next.firstCall.args[0].should.be.an.instanceof(Error) + ctx.RestoreManager.promises.revertFile.should.not.have.been.called + }) + }) + + describe('revertProject', function () { + beforeEach(function (ctx) { + ctx.res = { json: sinon.stub() } + ctx.ProjectAuditLogHandler.addEntryIfManagedInBackground = sinon.stub() + }) + + it('reverts the project for a valid request', async function (ctx) { + ctx.req = { + params: { project_id: ctx.project_id }, + body: { version: 42 }, + session: {}, + ip: '1.2.3.4', + } + await ctx.HistoryController.revertProject(ctx.req, ctx.res, ctx.next) + ctx.RestoreManager.promises.revertProject.should.have.been.calledWith( + ctx.user_id, + ctx.project_id, + 42 + ) + }) + + it('rejects a non-integer version without invoking RestoreManager', async function (ctx) { + ctx.req = { + params: { project_id: ctx.project_id }, + body: { version: 1.5 }, + session: {}, + ip: '1.2.3.4', + } + await ctx.HistoryController.revertProject(ctx.req, ctx.res, ctx.next) + ctx.next.should.have.been.calledOnce + ctx.next.firstCall.args[0].should.be.an.instanceof(Error) + ctx.RestoreManager.promises.revertProject.should.not.have.been.called + }) + }) + describe('requestBlob', function () { describe('With Range header', function () { beforeEach(async function (ctx) {