Merge pull request #35734 from overleaf/bg-issue35720

add zod validation to ExportsController and TemplatesController

GitOrigin-RevId: 8f283999665d730d491160aabe75b2643b8c2178

services/web/app/src/Features/Exports/ExportsController.mjs

@@ -74,9 +74,20 @@ async function exportProject(req, res, next) {
   }
 }
 
+const exportStatusSchema = z.object({
+  params: z.object({
+    export_id: zz.submissionId(),
+  }),
+  query: z.object({
+    token: z.string().optional(),
+  }),
+})
+
 async function exportStatus(req, res) {
-  const { export_id: exportId } = req.params
-  const { token } = req.query
+  const {
+    params: { export_id: exportId },
+    query: { token },
+  } = parseReq(req, exportStatusSchema)
   if (!token && settings.exports?.requireToken) {
     return res.status(403).json({
       export_json: {

services/web/app/src/Features/Templates/TemplatesController.mjs

@@ -2,20 +2,30 @@ import path from 'node:path'
 import SessionManager from '../Authentication/SessionManager.mjs'
 import TemplatesManager from './TemplatesManager.mjs'
 import ProjectHelper from '../Project/ProjectHelper.mjs'
-import logger from '@overleaf/logger'
 import { expressify } from '@overleaf/promise-utils'
+import { parseReq, z } from '../../infrastructure/Validation.mjs'
+
+// numeric v1 ids (template id / template version id)
+const numericId = z.string().regex(/^[0-9]+$/)
+
+const getV1TemplateSchema = z.object({
+  params: z.object({ Template_version_id: numericId }),
+  query: z.object({ id: numericId }),
+})
+
+const createProjectFromV1TemplateSchema = z.object({
+  body: z.object({
+    templateId: numericId,
+    templateVersionId: numericId,
+  }),
+})
 
 const TemplatesController = {
   async getV1Template(req, res) {
-    const templateVersionId = req.params.Template_version_id
-    const templateId = req.query.id
-    if (!/^[0-9]+$/.test(templateVersionId) || !/^[0-9]+$/.test(templateId)) {
-      logger.err(
-        { templateVersionId, templateId },
-        'invalid template id or version'
-      )
-      return res.sendStatus(400)
-    }
+    const {
+      params: { Template_version_id: templateVersionId },
+      query: { id: templateId },
+    } = parseReq(req, getV1TemplateSchema)
     const data = {
       templateVersionId,
       templateId,
@@ -35,14 +45,17 @@ const TemplatesController = {
   },
 
   async createProjectFromV1Template(req, res) {
+    const {
+      body: { templateId, templateVersionId },
+    } = parseReq(req, createProjectFromV1TemplateSchema)
     const userId = SessionManager.getLoggedInUserId(req.session)
     const project = await TemplatesManager.promises.createProjectFromV1Template(
       req.body.brandVariationId,
       req.body.compiler,
       req.body.mainFile,
-      req.body.templateId,
+      templateId,
       req.body.templateName,
-      req.body.templateVersionId,
+      templateVersionId,
       userId,
       req.body.imageName
     )

services/web/test/unit/src/Exports/ExportsController.test.mjs

@@ -203,10 +203,10 @@ describe('ExportsController', function () {
       json: sinon.stub(),
     }
 
-    ctx.req.params = { project_id: projectId, export_id: 897 }
+    ctx.req.params = { project_id: projectId, export_id: '897' }
     ctx.req.query = { token: 'mock-token' }
     await ctx.controller.exportStatus(ctx.req, res)
-    expect(ctx.handler.fetchExport).to.have.been.calledWith(897, 'mock-token')
+    expect(ctx.handler.fetchExport).to.have.been.calledWith('897', 'mock-token')
     expect(res.json.args[0][0]).to.deep.equal({
       export_json: {
         status_summary: 'completed',
@@ -223,7 +223,7 @@ describe('ExportsController', function () {
 
   describe('exportStatus token validation', function () {
     beforeEach(function (ctx) {
-      ctx.req.params = { project_id: projectId, export_id: 897 }
+      ctx.req.params = { project_id: projectId, export_id: '897' }
       ctx.handler.fetchExport = sinon.stub().resolves(
         `{
   "id":897,
@@ -264,7 +264,7 @@ describe('ExportsController', function () {
         }
         await ctx.controller.exportStatus(ctx.req, res)
         expect(ctx.handler.fetchExport).to.have.been.calledWith(
-          897,
+          '897',
           'mock-token'
         )
       })
@@ -277,7 +277,10 @@ describe('ExportsController', function () {
           json: sinon.stub(),
         }
         await ctx.controller.exportStatus(ctx.req, res)
-        expect(ctx.handler.fetchExport).to.have.been.calledWith(897, undefined)
+        expect(ctx.handler.fetchExport).to.have.been.calledWith(
+          '897',
+          undefined
+        )
       })
     })
 
@@ -289,7 +292,7 @@ describe('ExportsController', function () {
           .rejects(new Error('Request failed: 404'))
         await ctx.controller.exportStatus(ctx.req, ctx.res)
         expect(ctx.handler.fetchExport).to.have.been.calledWith(
-          897,
+          '897',
           'wrong-token'
         )
         expect(ctx.res.json.args[0][0]).to.deep.equal({
@@ -302,6 +305,46 @@ describe('ExportsController', function () {
     })
   })
 
+  describe('exportStatus export_id validation', function () {
+    beforeEach(function (ctx) {
+      ctx.handler.fetchExport = sinon.stub().resolves('{}')
+      ctx.req.query = { token: 'mock-token' }
+    })
+
+    it('should reject an invalid encoded export_id without calling v1', async function (ctx) {
+      ctx.req.params = {
+        project_id: projectId,
+        export_id: '..%2f..%2f..%2fv2%2fusers',
+      }
+      await ctx.controller.exportStatus(ctx.req, ctx.res, ctx.next)
+      expect(ctx.next).to.have.been.calledWith(sinon.match.instanceOf(Error))
+      expect(ctx.handler.fetchExport).not.to.have.been.called
+    })
+
+    it('should reject an export_id with invalid characters without calling v1', async function (ctx) {
+      ctx.req.params = {
+        project_id: projectId,
+        export_id: '../../../etc/passwd',
+      }
+      await ctx.controller.exportStatus(ctx.req, ctx.res, ctx.next)
+      expect(ctx.next).to.have.been.calledWith(sinon.match.instanceOf(Error))
+      expect(ctx.handler.fetchExport).not.to.have.been.called
+    })
+
+    it('should accept a well-formed export_id', async function (ctx) {
+      ctx.req.params = {
+        project_id: projectId,
+        export_id: 'abc-123_DEF',
+      }
+      await ctx.controller.exportStatus(ctx.req, ctx.res, ctx.next)
+      expect(ctx.next).not.to.have.been.called
+      expect(ctx.handler.fetchExport).to.have.been.calledWith(
+        'abc-123_DEF',
+        'mock-token'
+      )
+    })
+  })
+
   describe('exportDownload token validation', function () {
     beforeEach(function (ctx) {
       ctx.req.params = {

services/web/test/unit/src/Templates/TemplatesController.test.mjs

@@ -52,9 +52,9 @@ describe('TemplatesController', function () {
         brandVariationId: 'brand-variation-id',
         compiler: 'compiler',
         mainFile: 'main-file',
-        templateId: 'template-id',
+        templateId: '123',
         templateName: 'template-name',
-        templateVersionId: 'template-version-id',
+        templateVersionId: '456',
       },
       session: {
         templateData: 'template-data',
@@ -63,7 +63,11 @@ describe('TemplatesController', function () {
         },
       },
     }
-    return (ctx.res = { redirect: sinon.stub() })
+    return (ctx.res = {
+      redirect: sinon.stub(),
+      sendStatus: sinon.stub(),
+      render: sinon.stub(),
+    })
   })
 
   describe('createProjectFromV1Template', function () {
@@ -85,9 +89,9 @@ describe('TemplatesController', function () {
           'brand-variation-id',
           'compiler',
           'main-file',
-          'template-id',
+          '123',
           'template-name',
-          'template-version-id',
+          '456',
           'user-id'
         )
       })
@@ -103,6 +107,46 @@ describe('TemplatesController', function () {
       })
     })
 
+    describe('input validation', function () {
+      it('should reject an invalid templateVersionId', async function (ctx) {
+        ctx.req.body.templateVersionId = '../../../../../../123'
+        await ctx.TemplatesController.createProjectFromV1Template(
+          ctx.req,
+          ctx.res,
+          ctx.next
+        )
+        ctx.next.should.have.been.calledWithMatch(sinon.match.instanceOf(Error))
+        ctx.TemplatesManager.promises.createProjectFromV1Template.should.not
+          .have.been.called
+        ctx.res.redirect.called.should.equal(false)
+      })
+
+      it('should reject a non-numeric templateId', async function (ctx) {
+        ctx.req.body.templateId = 'not-a-number'
+        await ctx.TemplatesController.createProjectFromV1Template(
+          ctx.req,
+          ctx.res,
+          ctx.next
+        )
+        ctx.next.should.have.been.calledWithMatch(sinon.match.instanceOf(Error))
+        ctx.TemplatesManager.promises.createProjectFromV1Template.should.not
+          .have.been.called
+        ctx.res.redirect.called.should.equal(false)
+      })
+
+      it('should reject a missing templateVersionId', async function (ctx) {
+        delete ctx.req.body.templateVersionId
+        await ctx.TemplatesController.createProjectFromV1Template(
+          ctx.req,
+          ctx.res,
+          ctx.next
+        )
+        ctx.next.should.have.been.calledWithMatch(sinon.match.instanceOf(Error))
+        ctx.TemplatesManager.promises.createProjectFromV1Template.should.not
+          .have.been.called
+      })
+    })
+
     describe('on error', function () {
       beforeEach(function (ctx) {
         ctx.TemplatesManager.promises.createProjectFromV1Template.rejects(
@@ -126,4 +170,53 @@ describe('TemplatesController', function () {
       })
     })
   })
+
+  describe('getV1Template', function () {
+    beforeEach(function (ctx) {
+      ctx.req.params = { Template_version_id: '456' }
+      ctx.req.query = { id: '123' }
+      ctx.ProjectHelper.compilerFromV1Engine.returns('pdflatex')
+    })
+
+    it('should render the template page for valid ids', async function (ctx) {
+      ctx.req.query.templateName = 'template-name'
+      ctx.req.query.latexEngine = 'latex_dvipdf'
+      await ctx.TemplatesController.getV1Template(ctx.req, ctx.res, ctx.next)
+      ctx.next.called.should.equal(false)
+      ctx.res.render.should.have.been.calledWithMatch(sinon.match.string, {
+        templateVersionId: '456',
+        templateId: '123',
+        name: 'template-name',
+        compiler: 'pdflatex',
+      })
+    })
+
+    it('should reject an invalid Template_version_id', async function (ctx) {
+      ctx.req.params.Template_version_id = '../../../../123'
+      await ctx.TemplatesController.getV1Template(ctx.req, ctx.res, ctx.next)
+      ctx.next.should.have.been.calledWithMatch(sinon.match.instanceOf(Error))
+      ctx.res.render.called.should.equal(false)
+    })
+
+    it('should reject a missing Template_version_id param', async function (ctx) {
+      delete ctx.req.params.Template_version_id
+      await ctx.TemplatesController.getV1Template(ctx.req, ctx.res, ctx.next)
+      ctx.next.should.have.been.calledWithMatch(sinon.match.instanceOf(Error))
+      ctx.res.render.called.should.equal(false)
+    })
+
+    it('should reject a non-numeric id query param', async function (ctx) {
+      ctx.req.query.id = 'not-a-number'
+      await ctx.TemplatesController.getV1Template(ctx.req, ctx.res, ctx.next)
+      ctx.next.should.have.been.calledWithMatch(sinon.match.instanceOf(Error))
+      ctx.res.render.called.should.equal(false)
+    })
+
+    it('should reject a missing id query param', async function (ctx) {
+      delete ctx.req.query.id
+      await ctx.TemplatesController.getV1Template(ctx.req, ctx.res, ctx.next)
+      ctx.next.should.have.been.calledWithMatch(sinon.match.instanceOf(Error))
+      ctx.res.render.called.should.equal(false)
+    })
+  })
 })