diff --git a/services/docstore/app.js b/services/docstore/app.js index 4ccdd0c1e9..6a60be3c8b 100644 --- a/services/docstore/app.js +++ b/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( diff --git a/services/docstore/app/js/HttpController.js b/services/docstore/app/js/HttpController.js index e378870d81..85a3c8f206 100644 --- a/services/docstore/app/js/HttpController.js +++ b/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({ - deleted: z.literal(true), - deletedAt: z.coerce.date(), - name: z.string(), - }) - .strict(), + 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(), + }), +}) + +// 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) diff --git a/services/docstore/app/js/schemas.js b/services/docstore/app/js/schemas.js new file mode 100644 index 0000000000..e57dfdd660 --- /dev/null +++ b/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(), +}) diff --git a/services/docstore/test/acceptance/js/DeletingDocsTests.js b/services/docstore/test/acceptance/js/DeletingDocsTests.js index 4c3c6599c9..4f7d5fe7db 100644 --- a/services/docstore/test/acceptance/js/DeletingDocsTests.js +++ b/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, diff --git a/services/docstore/test/acceptance/js/GettingDocsTests.js b/services/docstore/test/acceptance/js/GettingDocsTests.js index 3c7080cc0f..7c09fc6912 100644 --- a/services/docstore/test/acceptance/js/GettingDocsTests.js +++ b/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() diff --git a/services/docstore/test/acceptance/js/UpdatingDocsTests.js b/services/docstore/test/acceptance/js/UpdatingDocsTests.js index 1fb190f2ea..f7f3e995b7 100644 --- a/services/docstore/test/acceptance/js/UpdatingDocsTests.js +++ b/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( diff --git a/services/docstore/test/unit/js/HttpController.test.js b/services/docstore/test/unit/js/HttpController.test.js index d8c2a95ce0..2150ef8ec7 100644 --- a/services/docstore/test/unit/js/HttpController.test.js +++ b/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()