[docstore] adopt strict zod types and extend test coverage (#36076)

GitOrigin-RevId: b9f1b965dbdb763ce541dabbe89300ce6d044217

services/docstore/app.js

@@ -28,22 +28,6 @@ app.use(Metrics.http.monitor(logger))
 
 Metrics.injectMetricsRoute(app)
 
-app.param('project_id', function (req, res, next, projectId) {
-  if (projectId?.match(/^[0-9a-f]{24}$/)) {
-    next()
-  } else {
-    next(new Error('invalid project id'))
-  }
-})
-
-app.param('doc_id', function (req, res, next, docId) {
-  if (docId?.match(/^[0-9a-f]{24}$/)) {
-    next()
-  } else {
-    next(new Error('invalid doc id'))
-  }
-})
-
 app.get('/project/:project_id/doc-deleted', HttpController.getAllDeletedDocs)
 app.get('/project/:project_id/doc', HttpController.getAllDocs)
 app.get(

services/docstore/app/js/HttpController.js

@@ -5,21 +5,114 @@ import HealthChecker from './HealthChecker.js'
 import Errors from './Errors.js'
 import Settings from '@overleaf/settings'
 import { expressify } from '@overleaf/promise-utils'
-import { parseReq, z } from '@overleaf/validation-tools'
+import { parseReq, z, zz } from '@overleaf/validation-tools'
+import * as schemas from './schemas.js'
+
+const projectParamsSchema = z.object({
+  params: z.strictObject({
+    project_id: zz.objectId(),
+  }),
+})
+
+const docParamsSchema = z.object({
+  params: z.strictObject({
+    project_id: zz.objectId(),
+    doc_id: zz.objectId(),
+  }),
+})
+
+const getDocSchema = z.object({
+  params: z.strictObject({
+    project_id: zz.objectId(),
+    doc_id: zz.objectId(),
+  }),
+  query: z.strictObject({
+    include_deleted: z.stringbool().default(false),
+  }),
+})
+
+// Rollout-temporary fallback (loosened primary schema; no zod validation
+// existed for this route on main); delete when this route's
+// REQ_VALIDATION_MODE instrumentation is removed.
+const getDocFallbackSchema = z.object({
+  params: z.object({
+    project_id: z.string(),
+    doc_id: z.string(),
+  }),
+  query: z.object({
+    include_deleted: z.stringbool().default(false),
+  }),
+})
+
+const projectHasRangesSchema = z.object({
+  params: z.strictObject({
+    project_id: zz.objectId(),
+  }),
+  query: z.strictObject({
+    useSecondary: z.stringbool().default(false),
+  }),
+})
+
+// Rollout-temporary fallback (loosened primary schema; no zod validation
+// existed for this route on main); delete when this route's
+// REQ_VALIDATION_MODE instrumentation is removed.
+const projectHasRangesFallbackSchema = z.object({
+  params: z.object({
+    project_id: z.string(),
+  }),
+  query: z.object({
+    useSecondary: z.stringbool().default(false),
+  }),
+})
+
+const updateDocSchema = z.object({
+  params: z.strictObject({
+    project_id: zz.objectId(),
+    doc_id: zz.objectId(),
+  }),
+  body: z.strictObject({
+    lines: z.array(z.string()),
+    version: z.number(),
+    ranges: schemas.ranges,
+  }),
+})
 
 const patchDocSchema = z.object({
-  body: z
-    .object({
+  params: z.strictObject({
+    project_id: zz.objectId(),
+    doc_id: zz.objectId(),
+  }),
+  body: z.strictObject({
     deleted: z.literal(true),
     deletedAt: z.coerce.date(),
     name: z.string(),
+  }),
 })
-    .strict(),
+
+// Rollout-temporary fallback (pre-refinement schema from main); delete
+// when this route's REQ_VALIDATION_MODE instrumentation is removed. Only
+// `body` was validated by parseReq() on main -- `params` were read directly
+// off `req.params` unvalidated, so this keeps them as bare strings rather
+// than reconstructing a param check that never existed.
+const patchDocFallbackSchema = z.object({
+  params: z.object({
+    project_id: z.string(),
+    doc_id: z.string(),
+  }),
+  body: z.strictObject({
+    deleted: z.literal(true),
+    deletedAt: z.coerce.date(),
+    name: z.string(),
+  }),
 })
 
 async function getDoc(req, res) {
-  const { doc_id: docId, project_id: projectId } = req.params
-  const includeDeleted = req.query.include_deleted === 'true'
+  const { params, query } = parseReq(req, getDocSchema, {
+    logOnly: true,
+    fallbackSchema: getDocFallbackSchema,
+  })
+  const { doc_id: docId, project_id: projectId } = params
+  const includeDeleted = query.include_deleted
   logger.debug({ projectId, docId }, 'getting doc')
   const doc = await DocManager.getFullDoc(projectId, docId)
   logger.debug({ docId, projectId }, 'got doc')
@@ -31,7 +124,8 @@ async function getDoc(req, res) {
 }
 
 async function peekDoc(req, res) {
-  const { doc_id: docId, project_id: projectId } = req.params
+  const { params } = parseReq(req, docParamsSchema, { logOnly: true })
+  const { doc_id: docId, project_id: projectId } = params
   logger.debug({ projectId, docId }, 'peeking doc')
   const doc = await DocManager.peekDoc(projectId, docId, {
     deleted: true,
@@ -46,13 +140,15 @@ async function peekDoc(req, res) {
 }
 
 async function isDocDeleted(req, res) {
-  const { doc_id: docId, project_id: projectId } = req.params
+  const { params } = parseReq(req, docParamsSchema, { logOnly: true })
+  const { doc_id: docId, project_id: projectId } = params
   const deleted = await DocManager.isDocDeleted(projectId, docId)
   res.json({ deleted })
 }
 
 async function getRawDoc(req, res) {
-  const { doc_id: docId, project_id: projectId } = req.params
+  const { params } = parseReq(req, docParamsSchema, { logOnly: true })
+  const { doc_id: docId, project_id: projectId } = params
   logger.debug({ projectId, docId }, 'getting raw doc')
   const content = await DocManager.getDocLines(projectId, docId)
   res.setHeader('content-type', 'text/plain')
@@ -60,7 +156,8 @@ async function getRawDoc(req, res) {
 }
 
 async function getAllDocs(req, res) {
-  const { project_id: projectId } = req.params
+  const { params } = parseReq(req, projectParamsSchema, { logOnly: true })
+  const { project_id: projectId } = params
   logger.debug({ projectId }, 'getting all docs')
   const docs = await DocManager.getAllNonDeletedDocs(projectId, {
     lines: true,
@@ -77,7 +174,8 @@ async function getAllDocs(req, res) {
 }
 
 async function getAllDocsWithRanges(req, res) {
-  const { project_id: projectId } = req.params
+  const { params } = parseReq(req, projectParamsSchema, { logOnly: true })
+  const { project_id: projectId } = params
   logger.debug({ projectId }, 'getting all docs with ranges')
   const docs = await DocManager.getAllNonDeletedDocs(projectId, {
     lines: true,
@@ -95,13 +193,15 @@ async function getAllDocsWithRanges(req, res) {
 }
 
 async function getAllDocVersions(req, res) {
-  const { project_id: projectId } = req.params
+  const { params } = parseReq(req, projectParamsSchema, { logOnly: true })
+  const { project_id: projectId } = params
   const docs = await DocManager.getAllDocVersions(projectId)
   res.json(docs)
 }
 
 async function getAllDeletedDocs(req, res) {
-  const { project_id: projectId } = req.params
+  const { params } = parseReq(req, projectParamsSchema, { logOnly: true })
+  const { project_id: projectId } = params
   logger.debug({ projectId }, 'getting all deleted docs')
   const docs = await DocManager.getAllDeletedDocs(projectId, {
     name: true,
@@ -117,7 +217,8 @@ async function getAllDeletedDocs(req, res) {
 }
 
 async function getAllRanges(req, res) {
-  const { project_id: projectId } = req.params
+  const { params } = parseReq(req, projectParamsSchema, { logOnly: true })
+  const { project_id: projectId } = params
   logger.debug({ projectId }, 'getting all ranges')
   const docs = await DocManager.getAllNonDeletedDocs(projectId, {
     ranges: true,
@@ -126,20 +227,26 @@ async function getAllRanges(req, res) {
 }
 
 async function getCommentThreadIds(req, res) {
-  const { project_id: projectId } = req.params
+  const { params } = parseReq(req, projectParamsSchema, { logOnly: true })
+  const { project_id: projectId } = params
   const threadIds = await DocManager.getCommentThreadIds(projectId)
   res.json(threadIds)
 }
 
 async function getTrackedChangesUserIds(req, res) {
-  const { project_id: projectId } = req.params
+  const { params } = parseReq(req, projectParamsSchema, { logOnly: true })
+  const { project_id: projectId } = params
   const userIds = await DocManager.getTrackedChangesUserIds(projectId)
   res.json(userIds)
 }
 
 async function projectHasRanges(req, res) {
-  const { project_id: projectId } = req.params
-  const useSecondary = req.query.useSecondary === 'true'
+  const { params, query } = parseReq(req, projectHasRangesSchema, {
+    logOnly: true,
+    fallbackSchema: projectHasRangesFallbackSchema,
+  })
+  const { project_id: projectId } = params
+  const { useSecondary } = query
   const projectHasRanges = await DocManager.projectHasRanges(
     projectId,
     useSecondary
@@ -148,11 +255,12 @@ async function projectHasRanges(req, res) {
 }
 
 async function updateDoc(req, res) {
-  const { doc_id: docId, project_id: projectId } = req.params
-  const lines = req.body?.lines
-  const version = req.body?.version
-  const ranges = req.body?.ranges
+  const { params, body } = parseReq(req, updateDocSchema, { logOnly: true })
+  const { doc_id: docId, project_id: projectId } = params
+  const { lines, version, ranges } = body
 
+  // Rollout-temporary fallback (validation on the fallback schema); delete
+  // when this route's REQ_VALIDATION_MODE instrumentation is removed.
   if (lines == null || !(lines instanceof Array)) {
     logger.error({ projectId, docId }, 'no doc lines provided')
     res.sendStatus(400) // Bad Request
@@ -193,8 +301,10 @@ async function updateDoc(req, res) {
 }
 
 async function patchDoc(req, res) {
-  const { body: meta } = parseReq(req, patchDocSchema)
-  const { doc_id: docId, project_id: projectId } = req.params
+  const { params, body: meta } = parseReq(req, patchDocSchema, {
+    fallbackSchema: patchDocFallbackSchema,
+  })
+  const { doc_id: docId, project_id: projectId } = params
   logger.debug({ projectId, docId }, 'patching doc')
   await DocManager.patchDoc(projectId, docId, meta)
   res.sendStatus(204)
@@ -227,21 +337,24 @@ function _buildDocsArrayView(projectId, docs) {
 }
 
 async function archiveAllDocs(req, res) {
-  const { project_id: projectId } = req.params
+  const { params } = parseReq(req, projectParamsSchema, { logOnly: true })
+  const { project_id: projectId } = params
   logger.debug({ projectId }, 'archiving all docs')
   await DocArchive.archiveAllDocs(projectId)
   res.sendStatus(204)
 }
 
 async function archiveDoc(req, res) {
-  const { doc_id: docId, project_id: projectId } = req.params
+  const { params } = parseReq(req, docParamsSchema, { logOnly: true })
+  const { doc_id: docId, project_id: projectId } = params
   logger.debug({ projectId, docId }, 'archiving a doc')
   await DocArchive.archiveDoc(projectId, docId)
   res.sendStatus(204)
 }
 
 async function unArchiveAllDocs(req, res) {
-  const { project_id: projectId } = req.params
+  const { params } = parseReq(req, projectParamsSchema, { logOnly: true })
+  const { project_id: projectId } = params
   logger.debug({ projectId }, 'unarchiving all docs')
   try {
     await DocArchive.unArchiveAllDocs(projectId)
@@ -256,7 +369,8 @@ async function unArchiveAllDocs(req, res) {
 }
 
 async function destroyProject(req, res) {
-  const { project_id: projectId } = req.params
+  const { params } = parseReq(req, projectParamsSchema, { logOnly: true })
+  const { project_id: projectId } = params
   logger.debug({ projectId }, 'destroying all docs')
   await DocArchive.destroyProject(projectId)
   res.sendStatus(204)

services/docstore/app/js/schemas.js

@@ -0,0 +1,61 @@
+// @ts-check
+import { z } from '@overleaf/validation-tools'
+
+// Zod schemas for the ranges data (RangesTracker format) accepted by
+// updateDoc.
+//
+// Ranges reach docstore via web from several writers and are stored (and
+// later round-tripped, e.g. by project clone) without normalization, so the
+// schemas model wire reality rather than the ideal shape:
+// - document-updater flushes RangesTracker data: change/comment ids are
+//   RangesTracker ids (seed + increment) or thread ids, i.e. strings that are
+//   not always ObjectIds (and old fixtures/documents carry arbitrary string
+//   thread ids), see libraries/ranges-tracker/index.cjs
+// - history restores send id-less changes and comment ops with a `resolved`
+//   flag (stripped again by RangeManager), and detached comments without an
+//   id, see overleaf-editor-core/lib/doc_updater_compatible_ranges.js
+
+const insertOp = z.strictObject({
+  i: z.string(),
+  p: z.number().int().min(0),
+  u: z.boolean().optional(),
+})
+
+const deleteOp = z.strictObject({
+  d: z.string(),
+  p: z.number().int().min(0),
+  u: z.boolean().optional(),
+})
+
+const commentOp = z.strictObject({
+  c: z.string().optional(),
+  p: z.number().int().min(0).optional(),
+  // comment thread ids are usually ObjectIds, but legacy documents carry
+  // arbitrary strings
+  t: z.string().optional(),
+  u: z.boolean().optional(),
+  // sent by history restores; removed again by RangeManager
+  resolved: z.boolean().optional(),
+})
+
+const rangeMetadata = z.strictObject({
+  user_id: z.string().optional(),
+  ts: z.string().optional(),
+})
+
+const comment = z.strictObject({
+  id: z.string().optional(),
+  op: commentOp,
+  metadata: rangeMetadata.optional(),
+})
+
+const trackedChange = z.strictObject({
+  id: z.string().optional(),
+  op: insertOp.or(deleteOp).optional(),
+  metadata: rangeMetadata.optional(),
+})
+
+export const ranges = z.strictObject({
+  comments: z.array(comment).optional(),
+  changes: z.array(trackedChange).optional(),
+})

services/docstore/test/acceptance/js/DeletingDocsTests.js

@@ -32,7 +32,7 @@ function deleteTestSuite(deleteDoc) {
     this.doc_id = new ObjectId()
     this.lines = ['original', 'lines']
     this.version = 42
-    this.ranges = []
+    this.ranges = {}
     await DocstoreApp.ensureRunning()
     await DocstoreClient.createDoc(
       this.project_id,
@@ -350,7 +350,7 @@ describe("Destroying a project's documents", function () {
     this.doc_id = new ObjectId()
     this.lines = ['original', 'lines']
     this.version = 42
-    this.ranges = []
+    this.ranges = {}
     await DocstoreApp.ensureRunning()
     await DocstoreClient.createDoc(
       this.project_id,

services/docstore/test/acceptance/js/GettingDocsTests.js

@@ -1,5 +1,6 @@
 import mongodb from 'mongodb-legacy'
 import { expect } from 'chai'
+import { expectValidationError } from '@overleaf/validation-tools/testUtils.js'
 import DocstoreApp from './helpers/DocstoreApp.js'
 import DocstoreClient from './helpers/DocstoreClient.js'
 
@@ -16,7 +17,7 @@ describe('Getting a doc', function () {
         {
           id: new ObjectId().toString(),
           op: { i: 'foo', p: 3 },
-          meta: {
+          metadata: {
             user_id: new ObjectId().toString(),
             ts: new Date().toJSON(),
           },
@@ -67,6 +68,47 @@ describe('Getting a doc', function () {
     })
   })
 
+  describe('when the doc id is not a valid ObjectId', function () {
+    it('should return a 404', async function () {
+      let err
+      try {
+        await DocstoreClient.getDoc(this.project_id, 'not-an-object-id')
+        expect.fail('should have thrown')
+      } catch (error) {
+        err = error
+      }
+      expectValidationError(err, 404, 'doc_id')
+    })
+  })
+
+  describe('when the project id is not a valid ObjectId', function () {
+    it('should return a 404', async function () {
+      let err
+      try {
+        await DocstoreClient.getDoc('not-an-object-id', this.doc_id)
+        expect.fail('should have thrown')
+      } catch (error) {
+        err = error
+      }
+      expectValidationError(err, 404, 'project_id')
+    })
+  })
+
+  describe('when the include_deleted query value is invalid', function () {
+    it('should return a 400', async function () {
+      let err
+      try {
+        await DocstoreClient.getDoc(this.project_id, this.doc_id, {
+          include_deleted: 'banana',
+        })
+        expect.fail('should have thrown')
+      } catch (error) {
+        err = error
+      }
+      expectValidationError(err, 400, 'include_deleted')
+    })
+  })
+
   describe('when the doc is a deleted doc', function () {
     beforeEach(async function () {
       this.deleted_doc_id = new ObjectId()

services/docstore/test/acceptance/js/UpdatingDocsTests.js

@@ -1,4 +1,5 @@
 import mongodb from 'mongodb-legacy'
+import { expectValidationError } from '@overleaf/validation-tools/testUtils.js'
 import DocstoreApp from './helpers/DocstoreApp.js'
 import DocstoreClient from './helpers/DocstoreClient.js'
 
@@ -15,9 +16,9 @@ describe('Applying updates to a doc', function () {
         {
           id: new ObjectId().toString(),
           op: { i: '$foo', p: 3 },
-          meta: {
+          metadata: {
             user_id: new ObjectId().toString(),
-            ts: new Date().toString(),
+            ts: new Date().toISOString(),
           },
         },
       ],
@@ -27,9 +28,9 @@ describe('Applying updates to a doc', function () {
         {
           id: new ObjectId().toString(),
           op: { i: '$bar', p: 6 },
-          meta: {
+          metadata: {
             user_id: new ObjectId().toString(),
-            ts: new Date().toString(),
+            ts: new Date().toISOString(),
           },
         },
       ],
@@ -259,6 +260,43 @@ describe('Applying updates to a doc', function () {
     })
   })
 
+  describe('when an unknown ranges field is provided', function () {
+    let err
+    beforeEach(async function () {
+      try {
+        this.body = await DocstoreClient.updateDoc(
+          this.project_id,
+          this.doc_id,
+          this.newLines,
+          this.version + 1,
+          { ...this.originalRanges, unknownField: 'x' }
+        )
+      } catch (error) {
+        err = error
+      }
+    })
+
+    it('should return 400', function () {
+      err.info.status.should.equal(400)
+    })
+
+    // handleValidationError() (@overleaf/validation-tools) composes the body
+    // as { error: fromError(zodError).toString(), statusCode }; fromError()
+    // renders the literal unrecognized-key name for a strictObject
+    // rejection. Asserting on the body (not just the status code) confirms
+    // the *intended* field is what rejected the request, not some unrelated
+    // bug that happens to also return the same status.
+    it('should return a body naming the unknown field', function () {
+      expectValidationError(err, 400, 'unknownField')
+    })
+
+    it('should not update the doc in the API', async function () {
+      const doc = await DocstoreClient.getDoc(this.project_id, this.doc_id)
+      doc.lines.should.deep.equal(this.originalLines)
+      doc.version.should.equal(this.version)
+    })
+  })
+
   describe('when no version is provided', function () {
     let statusCode
     beforeEach(async function () {
@@ -313,9 +351,13 @@ describe('Applying updates to a doc', function () {
     beforeEach(async function () {
       const line = new Array(1025).join('x') // 1kb
       this.largeLines = Array.apply(null, Array(1024)).map(() => line) // 1kb
-      this.originalRanges.padding = Array.apply(null, Array(2049)).map(
-        () => line
-      ) // 2mb + 1kb
+      // bloat the ranges payload beyond max_doc_length (2mb + 1kb)
+      this.originalRanges.comments = [
+        {
+          id: new ObjectId().toString(),
+          op: { c: line.repeat(2049), p: 0, t: new ObjectId().toString() },
+        },
+      ]
       this.body = await DocstoreClient.updateDoc(
         this.project_id,
         this.doc_id,
@@ -372,7 +414,13 @@ describe('Applying updates to a doc', function () {
     beforeEach(async function () {
       const line = new Array(1024).join('x') // 1 KB
       this.largeLines = new Array(8192).fill(line) // 8 MB
-      this.originalRanges.padding = new Array(6144).fill(line) // 6 MB
+      // bloat the ranges payload by 6 MB
+      this.originalRanges.comments = [
+        {
+          id: new ObjectId().toString(),
+          op: { c: line.repeat(6144), p: 0, t: new ObjectId().toString() },
+        },
+      ]
 
       try {
         this.body = await DocstoreClient.updateDoc(

services/docstore/test/unit/js/HttpController.test.js

@@ -49,8 +49,8 @@ describe('HttpController', () => {
     ctx.res.status = sinon.stub().returns(ctx.res)
     ctx.req = { query: {} }
     ctx.next = sinon.stub()
-    ctx.projectId = 'mock-project-id'
-    ctx.docId = 'mock-doc-id'
+    ctx.projectId = new ObjectId().toString()
+    ctx.docId = new ObjectId().toString()
     ctx.doc = {
       _id: ctx.docId,
       lines: ['mock', 'lines', ' here', '', '', ' spaces '],
@@ -350,7 +350,18 @@ describe('HttpController', () => {
         ctx.req.body = {
           lines: (ctx.lines = ['hello', 'world']),
           version: (ctx.version = 42),
-          ranges: (ctx.ranges = { changes: 'mock' }),
+          ranges: (ctx.ranges = {
+            changes: [
+              {
+                id: new ObjectId().toString(),
+                op: { i: 'foo', p: 3 },
+                metadata: {
+                  user_id: new ObjectId().toString(),
+                  ts: new Date().toJSON(),
+                },
+              },
+            ],
+          }),
         }
         ctx.rev = 5
         ctx.DocManager.updateDoc = sinon
@@ -412,14 +423,15 @@ describe('HttpController', () => {
         ctx.DocManager.updateDoc.called.should.equal(false)
       })
 
-      it('should return a 400 (bad request) response', ctx => {
-        ctx.res.sendStatus.calledWith(400).should.equal(true)
+      it('should pass a validation error to next', ctx => {
+        expect(ctx.next).to.have.been.calledOnce
+        expect(ctx.next.firstCall.args[0].name).to.equal('InvalidRequestError')
       })
     })
 
     describe('when the doc version are not provided', () => {
       beforeEach(async ctx => {
-        ctx.req.body = { version: 42, lines: ['hello world'] }
+        ctx.req.body = { lines: ['hello world'], ranges: {} }
         ctx.DocManager.updateDoc = sinon
           .stub()
           .resolves({ modified: false, rev: 0 })
@@ -430,8 +442,9 @@ describe('HttpController', () => {
         ctx.DocManager.updateDoc.called.should.equal(false)
       })
 
-      it('should return a 400 (bad request) response', ctx => {
-        ctx.res.sendStatus.calledWith(400).should.equal(true)
+      it('should pass a validation error to next', ctx => {
+        expect(ctx.next).to.have.been.calledOnce
+        expect(ctx.next.firstCall.args[0].name).to.equal('InvalidRequestError')
       })
     })
 
@@ -448,8 +461,9 @@ describe('HttpController', () => {
         ctx.DocManager.updateDoc.called.should.equal(false)
       })
 
-      it('should return a 400 (bad request) response', ctx => {
-        ctx.res.sendStatus.calledWith(400).should.equal(true)
+      it('should pass a validation error to next', ctx => {
+        expect(ctx.next).to.have.been.calledOnce
+        expect(ctx.next.firstCall.args[0].name).to.equal('InvalidRequestError')
       })
     })
 
@@ -458,7 +472,7 @@ describe('HttpController', () => {
         ctx.req.body = {
           lines: (ctx.lines = Array(2049).fill('a'.repeat(1024))),
           version: (ctx.version = 42),
-          ranges: (ctx.ranges = { changes: 'mock' }),
+          ranges: (ctx.ranges = {}),
         }
         ctx.DocManager.updateDoc = sinon
           .stub()