diff --git a/services/document-updater/app.js b/services/document-updater/app.js index 7053406e74..fb9fcf6559 100644 --- a/services/document-updater/app.js +++ b/services/document-updater/app.js @@ -13,6 +13,7 @@ const RedisManager = require('./app/js/RedisManager') const DispatchManager = require('./app/js/DispatchManager') const DeleteQueueManager = require('./app/js/DeleteQueueManager') const Errors = require('./app/js/Errors') +const { handleValidationError } = require('@overleaf/validation-tools') const HttpController = require('./app/js/HttpController') const mongodb = require('./app/js/mongodb') const async = require('async') @@ -103,22 +104,6 @@ app.get('/health_check', (req, res, next) => { // record http metrics for the routes below this point app.use(Metrics.http.monitor(logger)) -app.param('project_id', (req, res, next, projectId) => { - if (projectId != null && projectId.match(/^[0-9a-f]{24}$/)) { - return next() - } else { - return next(new Error('invalid project id')) - } -}) - -app.param('doc_id', (req, res, next, docId) => { - if (docId != null && docId.match(/^[0-9a-f]{24}$/)) { - return next() - } else { - return next(new Error('invalid doc id')) - } -}) - // Record requests that come in after we've started shutting down - for investigation. app.use((req, res, next) => { if (Settings.shuttingDown) { @@ -205,6 +190,8 @@ app.get('/total', (req, res, next) => { }) }) +app.use(handleValidationError) + app.use((error, req, res, next) => { if (error instanceof Errors.NotFoundError) { return res.sendStatus(404) diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index d128bb9e89..f2191793ad 100644 --- a/services/document-updater/app/js/HttpController.js +++ b/services/document-updater/app/js/HttpController.js @@ -1,4 +1,7 @@ const { expressify } = require('@overleaf/promise-utils') +const { parseReq, z, zz } = require('@overleaf/validation-tools') +const editorCoreSchemas = require('overleaf-editor-core/lib/schemas') +const schemas = require('./schemas') const DocumentManager = require('./DocumentManager') const HistoryManager = require('./HistoryManager') const ProjectManager = require('./ProjectManager') @@ -12,20 +15,54 @@ const { getTotalSizeOfLines } = require('./Limits') const { StringFileData } = require('overleaf-editor-core') const HistoryConversions = require('./HistoryConversions') +const projectParamsSchema = z.strictObject({ + project_id: zz.objectId(), +}) + +const docParamsSchema = z.strictObject({ + project_id: zz.objectId(), + doc_id: zz.objectId(), +}) + +const commentParamsSchema = z.strictObject({ + project_id: zz.objectId(), + doc_id: zz.objectId(), + comment_id: zz.objectId(), +}) + +const getDocSchema = z.object({ + params: docParamsSchema, + query: z.strictObject({ + historyOTSupport: z.stringbool().default(false), + fromVersion: z.coerce.number().int().default(-1), + }), +}) + +// 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({ + historyOTSupport: z.stringbool().default(false), + fromVersion: z.coerce.number().int().default(-1), + }), +}) + async function getDoc(req, res) { - let fromVersion - const docId = req.params.doc_id - const projectId = req.params.project_id + const { params, query } = parseReq(req, getDocSchema, { + logOnly: true, + fallbackSchema: getDocFallbackSchema, + }) + const { project_id: projectId, doc_id: docId } = params + const { fromVersion } = query logger.debug({ projectId, docId }, 'getting doc via http') const timer = new Metrics.Timer('http.getDoc') - if (req.query.fromVersion != null) { - fromVersion = parseInt(req.query.fromVersion, 10) - } else { - fromVersion = -1 - } - let { lines, version, ops, ranges, pathname, type } = await DocumentManager.promises.getDocAndRecentOpsWithLock( projectId, @@ -39,7 +76,7 @@ async function getDoc(req, res) { throw new Errors.NotFoundError('document not found') } - if (!Array.isArray(lines) && req.query.historyOTSupport !== 'true') { + if (!Array.isArray(lines) && !query.historyOTSupport) { ;({ lines, ranges } = HistoryConversions.fromHistoryOT(lines)) } @@ -55,10 +92,13 @@ async function getDoc(req, res) { }) } +const getCommentSchema = z.object({ + params: commentParamsSchema, +}) + async function getComment(req, res) { - const docId = req.params.doc_id - const projectId = req.params.project_id - const commentId = req.params.comment_id + const { params } = parseReq(req, getCommentSchema, { logOnly: true }) + const { project_id: projectId, doc_id: docId, comment_id: commentId } = params logger.debug({ projectId, docId, commentId }, 'getting comment via http') @@ -75,10 +115,33 @@ async function getComment(req, res) { res.json(comment) } +const peekDocSchema = z.object({ + params: docParamsSchema, + query: z.strictObject({ + historyOTSupport: 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 peekDocFallbackSchema = z.object({ + params: z.object({ + project_id: z.string(), + doc_id: z.string(), + }), + query: z.object({ + historyOTSupport: z.stringbool().default(false), + }), +}) + // return the doc from redis if present, but don't load it from mongo async function peekDoc(req, res) { - const docId = req.params.doc_id - const projectId = req.params.project_id + const { params, query } = parseReq(req, peekDocSchema, { + logOnly: true, + fallbackSchema: peekDocFallbackSchema, + }) + const { project_id: projectId, doc_id: docId } = params logger.debug({ projectId, docId }, 'peeking at doc via http') let { lines, version } = await RedisManager.promises.getDoc(projectId, docId) @@ -87,7 +150,7 @@ async function peekDoc(req, res) { throw new Errors.NotFoundError('document not found') } - if (!Array.isArray(lines) && req.query.historyOTSupport !== 'true') { + if (!Array.isArray(lines) && !query.historyOTSupport) { const file = StringFileData.fromRaw(lines) // TODO(24596): tc support for history-ot lines = file.getLines() @@ -96,32 +159,27 @@ async function peekDoc(req, res) { res.json({ id: docId, lines, version }) } +const getProjectDocsAndFlushIfOldSchema = z.object({ + params: projectParamsSchema, + query: z.strictObject({ + state: z.string().optional(), + }), +}) + async function getProjectDocsAndFlushIfOld(req, res) { - const projectId = req.params.project_id - const projectStateHash = req.query.state - // exclude is string of existing docs "id:version,id:version,..." - const excludeItems = - req.query.exclude != null ? req.query.exclude.split(',') : [] - logger.debug({ projectId, exclude: excludeItems }, 'getting docs via http') + const { params, query } = parseReq(req, getProjectDocsAndFlushIfOldSchema, { + logOnly: true, + }) + const projectId = params.project_id + const projectStateHash = query.state + logger.debug({ projectId }, 'getting docs via http') const timer = new Metrics.Timer('http.getAllDocs') - const excludeVersions = {} - - for (const item of excludeItems) { - const [id, version] = item.split(':') - excludeVersions[id] = version - } - - logger.debug( - { projectId, projectStateHash, excludeVersions }, - 'excluding versions' - ) let result try { result = await ProjectManager.promises.getProjectDocsAndFlushIfOld( projectId, - projectStateHash, - excludeVersions + projectStateHash ) } catch (error) { if (error instanceof Errors.ProjectStateChangedError) { @@ -142,8 +200,13 @@ async function getProjectDocsAndFlushIfOld(req, res) { res.send(result) } +const projectOnlySchema = z.object({ + params: projectParamsSchema, +}) + async function getProjectLastUpdatedAt(req, res) { - const projectId = req.params.project_id + const projectId = parseReq(req, projectOnlySchema, { logOnly: true }).params + .project_id let timestamps = await ProjectManager.promises.getProjectDocsTimestamps(projectId) @@ -158,13 +221,15 @@ async function getProjectLastUpdatedAt(req, res) { } async function getProjectRanges(req, res) { - const projectId = req.params.project_id + const projectId = parseReq(req, projectOnlySchema, { logOnly: true }).params + .project_id const docs = await ProjectManager.promises.getProjectRanges(projectId) res.json({ docs }) } async function clearProjectState(req, res) { - const projectId = req.params.project_id + const projectId = parseReq(req, projectOnlySchema, { logOnly: true }).params + .project_id const timer = new Metrics.Timer('http.clearProjectState') logger.debug({ projectId }, 'clearing project state via http') await ProjectManager.promises.clearProjectState(projectId) @@ -172,10 +237,21 @@ async function clearProjectState(req, res) { res.sendStatus(200) } +const setDocSchema = z.object({ + params: docParamsSchema, + body: z.strictObject({ + lines: z.array(z.string()), + source: z.string(), + user_id: zz.objectId().nullish(), + undoing: z.boolean().optional(), + trackChanges: z.boolean().optional(), + }), +}) + async function setDoc(req, res) { - const docId = req.params.doc_id - const projectId = req.params.project_id - const { lines, source, user_id: userId, undoing, trackChanges } = req.body + const { params, body } = parseReq(req, setDocSchema, { logOnly: true }) + const { project_id: projectId, doc_id: docId } = params + const { lines, source, user_id: userId, undoing, trackChanges } = body if (trackChanges && !userId) { return res.status(400).send('track changes requires a user id') } @@ -213,10 +289,22 @@ async function setDoc(req, res) { res.json(result || {}) } +const appendToDocSchema = z.object({ + params: docParamsSchema, + body: z.strictObject({ + lines: z.array(z.string()), + source: z.string(), + user_id: zz.objectId().nullish(), + trackChanges: z.boolean().optional(), + }), +}) + async function appendToDoc(req, res) { - const docId = req.params.doc_id - const projectId = req.params.project_id - const { lines, source, user_id: userId, trackChanges } = req.body + const { params, body } = parseReq(req, appendToDocSchema, { + logOnly: true, + }) + const { project_id: projectId, doc_id: docId } = params + const { lines, source, user_id: userId, trackChanges } = body if (trackChanges && !userId) { return res.status(400).send('track changes requires a user id') } @@ -249,9 +337,13 @@ async function appendToDoc(req, res) { res.json(result) } +const docOnlySchema = z.object({ + params: docParamsSchema, +}) + async function flushDocIfLoaded(req, res) { - const docId = req.params.doc_id - const projectId = req.params.project_id + const { params } = parseReq(req, docOnlySchema, { logOnly: true }) + const { project_id: projectId, doc_id: docId } = params logger.debug({ projectId, docId }, 'flushing doc via http') const timer = new Metrics.Timer('http.flushDoc') await DocumentManager.promises.flushDocIfLoadedWithLock(projectId, docId) @@ -260,10 +352,33 @@ async function flushDocIfLoaded(req, res) { res.sendStatus(204) // No Content } +const deleteDocSchema = z.object({ + params: docParamsSchema, + query: z.strictObject({ + ignore_flush_errors: 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 deleteDocFallbackSchema = z.object({ + params: z.object({ + project_id: z.string(), + doc_id: z.string(), + }), + query: z.object({ + ignore_flush_errors: z.stringbool().default(false), + }), +}) + async function deleteDoc(req, res) { - const docId = req.params.doc_id - const projectId = req.params.project_id - const ignoreFlushErrors = req.query.ignore_flush_errors === 'true' + const { params, query } = parseReq(req, deleteDocSchema, { + logOnly: true, + fallbackSchema: deleteDocFallbackSchema, + }) + const { project_id: projectId, doc_id: docId } = params + const ignoreFlushErrors = query.ignore_flush_errors const timer = new Metrics.Timer('http.deleteDoc') logger.debug({ projectId, docId }, 'deleting doc via http') @@ -283,7 +398,8 @@ async function deleteDoc(req, res) { } async function flushProject(req, res) { - const projectId = req.params.project_id + const projectId = parseReq(req, projectOnlySchema, { logOnly: true }).params + .project_id logger.debug({ projectId }, 'flushing project via http') const timer = new Metrics.Timer('http.flushProject') await ProjectManager.promises.flushProjectWithLocks(projectId) @@ -292,17 +408,42 @@ async function flushProject(req, res) { res.sendStatus(204) // No Content } +const deleteProjectSchema = z.object({ + params: projectParamsSchema, + query: z.strictObject({ + background: z.stringbool().default(false), + shutdown: 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 deleteProjectFallbackSchema = z.object({ + params: z.object({ + project_id: z.string(), + }), + query: z.object({ + background: z.stringbool().default(false), + shutdown: z.stringbool().default(false), + }), +}) + async function deleteProject(req, res) { - const projectId = req.params.project_id + const { params, query } = parseReq(req, deleteProjectSchema, { + logOnly: true, + fallbackSchema: deleteProjectFallbackSchema, + }) + const projectId = params.project_id logger.debug({ projectId }, 'deleting project via http') const options = {} - if (req.query.background) { + if (query.background) { options.background = true } // allow non-urgent flushes to be queued - if (req.query.shutdown) { + if (query.shutdown) { options.skip_history_flush = true } // don't flush history when realtime shuts down - if (req.query.background) { + if (query.background) { await ProjectManager.promises.queueFlushAndDeleteProject(projectId) logger.debug({ projectId }, 'queue delete of project via http') } else { @@ -318,8 +459,26 @@ async function deleteProject(req, res) { res.sendStatus(204) } +const deleteMultipleProjectsSchema = z.object({ + body: z.strictObject({ + project_ids: z.array(zz.objectId()).default([]), + }), +}) + +// 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 deleteMultipleProjectsFallbackSchema = z.object({ + body: z.object({ + project_ids: z.array(z.string()).default([]), + }), +}) + async function deleteMultipleProjects(req, res) { - const projectIds = req.body.project_ids || [] + const projectIds = parseReq(req, deleteMultipleProjectsSchema, { + logOnly: true, + fallbackSchema: deleteMultipleProjectsFallbackSchema, + }).body.project_ids logger.debug({ projectIds }, 'deleting multiple projects via http') for (const projectId of projectIds) { logger.debug({ projectId }, 'queue delete of project via http') @@ -328,11 +487,26 @@ async function deleteMultipleProjects(req, res) { res.sendStatus(204) // No Content } +// tracked-change ids are RangesTracker ids (seed + increment), not ObjectIds +const acceptChangesSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + doc_id: zz.objectId(), + change_id: z.string().min(1).optional(), + }), + body: z.strictObject({ + change_ids: z.array(z.string().min(1)).optional(), + }), +}) + async function acceptChanges(req, res) { - const { project_id: projectId, doc_id: docId } = req.params - let changeIds = req.body.change_ids + const { params, body } = parseReq(req, acceptChangesSchema, { + logOnly: true, + }) + const { project_id: projectId, doc_id: docId } = params + let changeIds = body.change_ids if (changeIds == null) { - changeIds = [req.params.change_id] + changeIds = [params.change_id] } logger.debug( { projectId, docId }, @@ -354,13 +528,19 @@ async function acceptChanges(req, res) { res.status(200).json({ changeContributors }) } +const commentWithUserSchema = z.object({ + params: commentParamsSchema, + body: z.strictObject({ + user_id: zz.objectId().optional(), + }), +}) + async function resolveComment(req, res) { - const { - project_id: projectId, - doc_id: docId, - comment_id: commentId, - } = req.params - const userId = req.body.user_id + const { params, body } = parseReq(req, commentWithUserSchema, { + logOnly: true, + }) + const { project_id: projectId, doc_id: docId, comment_id: commentId } = params + const userId = body.user_id logger.debug({ projectId, docId, commentId }, 'resolving comment via http') await DocumentManager.promises.updateCommentStateWithLock( projectId, @@ -374,12 +554,11 @@ async function resolveComment(req, res) { } async function reopenComment(req, res) { - const { - project_id: projectId, - doc_id: docId, - comment_id: commentId, - } = req.params - const userId = req.body.user_id + const { params, body } = parseReq(req, commentWithUserSchema, { + logOnly: true, + }) + const { project_id: projectId, doc_id: docId, comment_id: commentId } = params + const userId = body.user_id logger.debug({ projectId, docId, commentId }, 'reopening comment via http') await DocumentManager.promises.updateCommentStateWithLock( projectId, @@ -393,12 +572,11 @@ async function reopenComment(req, res) { } async function deleteComment(req, res) { - const { - project_id: projectId, - doc_id: docId, - comment_id: commentId, - } = req.params - const userId = req.body.user_id + const { params, body } = parseReq(req, commentWithUserSchema, { + logOnly: true, + }) + const { project_id: projectId, doc_id: docId, comment_id: commentId } = params + const userId = body.user_id logger.debug({ projectId, docId, commentId }, 'deleting comment via http') const timer = new Metrics.Timer('http.deleteComment') await DocumentManager.promises.deleteCommentWithLock( @@ -412,10 +590,114 @@ async function deleteComment(req, res) { res.sendStatus(204) // No Content } +// project-structure updates as built by web's DocumentUpdaterHandler +// (_getUpdates): deletes are renames to an empty newPathname +const renameUpdateSchema = z.strictObject({ + type: z.enum(['rename-doc', 'rename-file']), + id: zz.objectId(), + // forwarded (via project-history) into history-v1's archive/zip builder + // (project_archive.js `archive.append(content, { name: pathname })`), so a + // traversal payload here is a zip-slip risk, not just a display string -- + // see zod-string-field-hardening.md, category C. + pathname: zz.safePath(), + // deletes are renames to an empty newPathname (see the file comment + // above), so the empty string is a legitimate sentinel here, not a + // validation gap. + newPathname: zz.safePath().or(z.literal('')), +}) + +const addUpdateSchema = z.strictObject({ + type: z.enum(['add-doc', 'add-file']), + id: zz.objectId(), + pathname: zz.safePath(), + docLines: z.string().optional(), + ranges: schemas.ranges.optional(), + historyRangesSupport: z.boolean().optional(), + // legacy filestore url for files without a created blob + url: z.string().nullish(), + hash: editorCoreSchemas.rawBlobHash.optional(), + metadata: editorCoreSchemas.rawFileMetadata.optional(), + createdBlob: z.boolean().optional(), +}) + +// 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 renameUpdateFallbackSchema = z.object({ + type: z.enum(['rename-doc', 'rename-file']), + id: z.string(), + pathname: z.string(), + newPathname: z.string(), +}) + +// 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 addUpdateFallbackSchema = z.object({ + type: z.enum(['add-doc', 'add-file']), + id: z.string(), + pathname: z.string(), + docLines: z.string().optional(), + ranges: z.object({}).passthrough().optional(), + historyRangesSupport: z.boolean().optional(), + url: z.string().nullish(), + hash: z.string().optional(), + metadata: z.object({}).passthrough().optional(), + createdBlob: z.boolean().optional(), +}) + +const updateProjectSchema = z.object({ + params: projectParamsSchema, + body: z.strictObject({ + projectHistoryId: z.union([z.number(), zz.projectHistoryId()]).optional(), + userId: zz.objectId().nullish(), + updates: z.array(renameUpdateSchema.or(addUpdateSchema)).default([]), + version: z.union([z.number(), z.string()]), + // `source` is polymorphic (see Utils.extractOriginOrSource): either a + // plain descriptive string (e.g. 'editor' for live user edits) or the + // richer Origin/RestoreOrigin/RestoreFileOrigin/RestoreProjectOrigin + // raw shape (see overleaf-editor-core/lib/origin/) that + // RestoreManager.mjs sends when reverting a file/project from history -- + // it carries the version/timestamp of the restored version, which is + // needed to build a valid history Change origin downstream. web also + // sends an explicit `null` (not an omitted field) for system-initiated + // project-structure changes that aren't attributable to a specific + // editor action -- project creation (root/bib doc, template image), + // project duplication, and zip upload all call + // DocumentUpdaterHandler.updateProjectStructure with source=null. + source: editorCoreSchemas.rawOrigin.or(z.string()).nullish(), + }), +}) + +// 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 updateProjectFallbackSchema = z.object({ + params: z.object({ + project_id: z.string(), + }), + body: z.object({ + projectHistoryId: z.union([z.number(), z.string()]).optional(), + userId: z.string().nullish(), + updates: z + .array(renameUpdateFallbackSchema.or(addUpdateFallbackSchema)) + .default([]), + version: z.union([z.number(), z.string()]), + // loosened equivalent of editorCoreSchemas.rawOrigin -- see the same + // simplification for `origin` in project-history's resyncProjectHistory + // fallback. + source: z.object({ kind: z.string() }).or(z.string()).nullish(), + }), +}) + async function updateProject(req, res) { const timer = new Metrics.Timer('http.updateProject') - const projectId = req.params.project_id - const { projectHistoryId, userId, updates = [], version, source } = req.body + const { params, body } = parseReq(req, updateProjectSchema, { + logOnly: true, + fallbackSchema: updateProjectFallbackSchema, + }) + const projectId = params.project_id + const { projectHistoryId, userId, updates, version, source } = body logger.debug({ projectId, updates, version }, 'updating project via http') await ProjectManager.promises.updateProjectWithLocks( projectId, @@ -430,15 +712,44 @@ async function updateProject(req, res) { res.sendStatus(204) // No Content } +const resyncProjectHistorySchema = z.object({ + params: projectParamsSchema, + body: z.strictObject({ + projectHistoryId: z.union([z.number(), zz.projectHistoryId()]), + docs: z.array( + z.strictObject({ + doc: zz.objectId(), + path: zz.safePath(), + }) + ), + files: z.array( + z.strictObject({ + file: zz.objectId(), + path: zz.safePath(), + // legacy filestore url for files without a created blob + url: z.string().nullish(), + _hash: editorCoreSchemas.rawBlobHash.optional(), + createdBlob: z.boolean().optional(), + metadata: editorCoreSchemas.rawFileMetadata.optional(), + }) + ), + historyRangesMigration: z.string().optional(), + resyncProjectStructureOnly: z.boolean().optional(), + }), +}) + async function resyncProjectHistory(req, res) { - const projectId = req.params.project_id + const { params, body } = parseReq(req, resyncProjectHistorySchema, { + logOnly: true, + }) + const projectId = params.project_id const { projectHistoryId, docs, files, historyRangesMigration, resyncProjectStructureOnly, - } = req.body + } = body logger.debug( { projectId, docs, files }, @@ -464,12 +775,39 @@ async function resyncProjectHistory(req, res) { res.sendStatus(204) } +const flushQueuedProjectsSchema = z.object({ + query: z.strictObject({ + limit: z.coerce.number().int().default(1000), + min_delete_age: z.coerce + .number() + .int() + .default(5 * 60 * 1000), + }), +}) + +// 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 flushQueuedProjectsFallbackSchema = z.object({ + query: z.object({ + limit: z.coerce.number().int().default(1000), + min_delete_age: z.coerce + .number() + .int() + .default(5 * 60 * 1000), + }), +}) + async function flushQueuedProjects(req, res) { res.setTimeout(10 * 60 * 1000) + const { query } = parseReq(req, flushQueuedProjectsSchema, { + logOnly: true, + fallbackSchema: flushQueuedProjectsFallbackSchema, + }) const options = { - limit: req.query.limit || 1000, + limit: query.limit, timeout: 5 * 60 * 1000, - min_delete_age: req.query.min_delete_age || 5 * 60 * 1000, + min_delete_age: query.min_delete_age, } await DeleteQueueManager.promises.flushAndDeleteOldProjects( options, @@ -492,7 +830,8 @@ async function flushQueuedProjects(req, res) { * response indicates whether the project has been blocked or not. */ async function blockProject(req, res) { - const projectId = req.params.project_id + const projectId = parseReq(req, projectOnlySchema, { logOnly: true }).params + .project_id const blocked = await RedisManager.promises.blockProject(projectId) res.json({ blocked }) } @@ -501,7 +840,8 @@ async function blockProject(req, res) { * Unblock a project */ async function unblockProject(req, res) { - const projectId = req.params.project_id + const projectId = parseReq(req, projectOnlySchema, { logOnly: true }).params + .project_id const wasBlocked = await RedisManager.promises.unblockProject(projectId) res.json({ wasBlocked }) } diff --git a/services/document-updater/app/js/ProjectManager.js b/services/document-updater/app/js/ProjectManager.js index d310df44c2..b11ab8c513 100644 --- a/services/document-updater/app/js/ProjectManager.js +++ b/services/document-updater/app/js/ProjectManager.js @@ -83,11 +83,7 @@ async function getProjectDocsTimestamps(projectId, callback) { return timestamps } -async function getProjectDocsAndFlushIfOld( - projectId, - projectStateHash, - excludeVersions -) { +async function getProjectDocsAndFlushIfOld(projectId, projectStateHash) { const timer = new Metrics.Timer('projectManager.getProjectDocsAndFlushIfOld') const projectStateChanged = diff --git a/services/document-updater/app/js/RangesManager.js b/services/document-updater/app/js/RangesManager.js index 162f161829..0131cd5051 100644 --- a/services/document-updater/app/js/RangesManager.js +++ b/services/document-updater/app/js/RangesManager.js @@ -167,7 +167,8 @@ const RangesManager = { lines, }) { /** @type {(change: TrackedChange) => boolean} */ - const isAccepted = change => acceptedChangeIds.includes(change.id) + const isAccepted = change => + change.id != null && acceptedChangeIds.includes(change.id) const historyOps = [] diff --git a/services/document-updater/app/js/schemas.js b/services/document-updater/app/js/schemas.js index e69de29bb2..bb3dd03299 100644 --- a/services/document-updater/app/js/schemas.js +++ b/services/document-updater/app/js/schemas.js @@ -0,0 +1,66 @@ +// @ts-check +'use strict' + +const { z, zz } = require('@overleaf/validation-tools') + +// Zod schemas for the sharejs-text-ot ranges data (RangesTracker format). +// The types in ./types.ts are inferred from these schemas. +// +// Ranges data is persisted round-trip (docstore <-> web <-> document-updater) +// without normalization, so legacy shapes survive on old documents and the +// schemas model them explicitly: comments/changes preloaded from web may lack `id`. +// Tightening these is tracked as part of moving the shared editor payload +// schemas into overleaf-editor-core. + +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(), + p: z.number().int().min(0), + t: zz.objectId(), + u: z.boolean().optional(), + // Used by project-history when restoring CommentSnapshots + resolved: z.boolean().optional(), +}) + +const rangeMetadata = z.strictObject({ + user_id: z.string(), + ts: z.string(), +}) + +const comment = z.strictObject({ + id: zz.objectId().optional(), + op: commentOp, + metadata: rangeMetadata.optional(), +}) + +// tracked-change ids are RangesTracker ids (seed + increment), not ObjectIds +const trackedChange = z.strictObject({ + id: z.string().optional(), + op: insertOp.or(deleteOp), + metadata: rangeMetadata, +}) + +const ranges = z.strictObject({ + comments: z.array(comment).optional(), + changes: z.array(trackedChange).optional(), +}) + +module.exports = { + insertOp, + deleteOp, + commentOp, + comment, + trackedChange, + ranges, +} diff --git a/services/document-updater/app/js/types.ts b/services/document-updater/app/js/types.ts index 851e62d8c8..83ff492eb5 100644 --- a/services/document-updater/app/js/types.ts +++ b/services/document-updater/app/js/types.ts @@ -3,6 +3,8 @@ import { ClearTrackingPropsRawData, RawEditOperation, } from 'overleaf-editor-core/lib/types' +import type { z } from '@overleaf/validation-tools' +import type * as schemas from './schemas' export type OTType = 'sharejs-text-ot' | 'history-ot' @@ -30,57 +32,25 @@ export type HistoryOTEditOperationUpdate = Omit & { export type Op = InsertOp | DeleteOp | CommentOp | RetainOp -export type InsertOp = { - i: string - p: number - u?: boolean -} +export type InsertOp = z.infer export type RetainOp = { r: string p: number } -export type DeleteOp = { - d: string - p: number - u?: boolean -} +export type DeleteOp = z.infer -export type CommentOp = { - c: string - p: number - t: string - u?: boolean - // Used by project-history when restoring CommentSnapshots - resolved?: boolean -} +export type CommentOp = z.infer /** * Ranges record on a document */ -export type Ranges = { - comments?: Comment[] - changes?: TrackedChange[] -} +export type Ranges = z.infer -export type Comment = { - id: string - op: CommentOp - metadata?: { - user_id: string - ts: string - } -} +export type Comment = z.infer -export type TrackedChange = { - id: string - op: InsertOp | DeleteOp - metadata: { - user_id: string - ts: string - } -} +export type TrackedChange = z.infer /** * Updates sent to project-history diff --git a/services/document-updater/package.json b/services/document-updater/package.json index 5b5a0f8cdf..0fe037724b 100644 --- a/services/document-updater/package.json +++ b/services/document-updater/package.json @@ -25,6 +25,7 @@ "@overleaf/ranges-tracker": "workspace:*", "@overleaf/redis-wrapper": "workspace:*", "@overleaf/settings": "workspace:*", + "@overleaf/validation-tools": "workspace:*", "@types/chai-as-promised": "^7.1.8", "async": "^3.2.5", "bull": "^3.18.0", diff --git a/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js b/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js index 2ffd050fdd..41b2b6a4b1 100644 --- a/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js +++ b/services/document-updater/test/acceptance/js/ApplyingUpdatesToProjectStructureTests.js @@ -1,5 +1,7 @@ const sinon = require('sinon') +const { expect } = require('chai') const { setTimeout } = require('node:timers/promises') +const { RequestFailedError } = require('@overleaf/fetch-utils') const Settings = require('@overleaf/settings') const rclientProjectHistory = require('@overleaf/redis-wrapper').createClient( Settings.redis.project_history @@ -11,8 +13,20 @@ const MockWebApi = require('./helpers/MockWebApi') const DocUpdaterClient = require('./helpers/DocUpdaterClient') const DocUpdaterApp = require('./helpers/DocUpdaterApp') -async function sendProjectUpdateAndWait(projectId, docId, update, version) { - await DocUpdaterClient.sendProjectUpdate(projectId, docId, update, version) +async function sendProjectUpdateAndWait( + projectId, + docId, + update, + version, + source +) { + await DocUpdaterClient.sendProjectUpdate( + projectId, + docId, + update, + version, + source + ) // It seems that we need to wait for a little while await setTimeout(200) @@ -20,7 +34,7 @@ async function sendProjectUpdateAndWait(projectId, docId, update, version) { describe("Applying updates to a project's structure", function () { before(async function () { - this.user_id = 'user-id-123' + this.user_id = DocUpdaterClient.randomId() this.version = 1234 await DocUpdaterApp.ensureRunning() @@ -438,6 +452,136 @@ describe("Applying updates to a project's structure", function () { } ) }) + + describe('with importedAt metadata', function () { + before(async function () { + this.project_id = DocUpdaterClient.randomId() + this.metadata = { importedAt: new Date().toISOString() } + this.fileUpdate = { + type: 'add-file', + id: DocUpdaterClient.randomId(), + pathname: '/file-path', + url: 'filestore.example.com', + metadata: this.metadata, + } + this.updates = [this.fileUpdate] + await sendProjectUpdateAndWait( + this.project_id, + this.user_id, + this.updates, + this.version + ) + }) + + it('should push the file addition to the project history api with the metadata', function (done) { + rclientProjectHistory.lrange( + ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), + 0, + -1, + (error, updates) => { + if (error) { + return done(error) + } + + const update = JSON.parse(updates[0]) + update.file.should.equal(this.fileUpdate.id) + expect(update.metadata).to.deep.equal(this.metadata) + + done() + } + ) + }) + }) + + describe('with main and importedAt metadata', function () { + before(async function () { + this.project_id = DocUpdaterClient.randomId() + this.metadata = { main: true, importedAt: new Date().toISOString() } + this.fileUpdate = { + type: 'add-file', + id: DocUpdaterClient.randomId(), + pathname: '/file-path', + url: 'filestore.example.com', + metadata: this.metadata, + } + this.updates = [this.fileUpdate] + await sendProjectUpdateAndWait( + this.project_id, + this.user_id, + this.updates, + this.version + ) + }) + + it('should push the file addition to the project history api with the metadata', function (done) { + rclientProjectHistory.lrange( + ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), + 0, + -1, + (error, updates) => { + if (error) { + return done(error) + } + + const update = JSON.parse(updates[0]) + update.file.should.equal(this.fileUpdate.id) + expect(update.metadata).to.deep.equal(this.metadata) + + done() + } + ) + }) + }) + + describe('with invalid metadata', function () { + before(async function () { + this.project_id = DocUpdaterClient.randomId() + this.fileUpdate = { + type: 'add-file', + id: DocUpdaterClient.randomId(), + pathname: '/file-path', + url: 'filestore.example.com', + metadata: { provider: 'not-a-real-provider' }, + } + this.updates = [this.fileUpdate] + try { + await sendProjectUpdateAndWait( + this.project_id, + this.user_id, + this.updates, + this.version + ) + this.statusCode = 200 + } catch (err) { + if (err instanceof RequestFailedError) { + this.statusCode = err.response.status + } else { + throw err + } + } + }) + + it('should return a 400 status code', function () { + this.statusCode.should.equal(400) + }) + + it('should not push anything to the project history api', function (done) { + rclientProjectHistory.lrange( + ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), + 0, + -1, + (error, updates) => { + if (error) { + return done(error) + } + + updates.should.deep.equal([]) + + done() + } + ) + }) + }) }) describe('adding a doc', function () { @@ -482,6 +626,195 @@ describe("Applying updates to a project's structure", function () { }) }) + describe('adding a doc with a null source (e.g. project creation)', function () { + // web sends an explicit `null` source (not an omitted field) when + // creating the root/bib doc for a new project, duplicating a project, or + // importing a zip upload -- see + // web/app/src/Features/Project/ProjectCreationHandler.mjs _createRootDoc. + before(async function () { + this.project_id = DocUpdaterClient.randomId() + this.docUpdate = { + type: 'add-doc', + id: DocUpdaterClient.randomId(), + pathname: '/file-path', + docLines: 'a\nb', + } + this.updates = [this.docUpdate] + await sendProjectUpdateAndWait( + this.project_id, + this.user_id, + this.updates, + this.version, + null + ) + }) + + it('should push the doc addition to the project history api without a source', function (done) { + rclientProjectHistory.lrange( + ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), + 0, + -1, + (error, updates) => { + if (error) { + return done(error) + } + + const update = JSON.parse(updates[0]) + update.doc.should.equal(this.docUpdate.id) + update.meta.user_id.should.equal(this.user_id) + expect(update.meta.source).to.not.exist + + done() + } + ) + }) + }) + + describe('adding a doc with an origin object source (e.g. restoring a project from history)', function () { + // web's RestoreManager.mjs sends the richer Origin/RestoreProjectOrigin + // raw shape (not a plain string) as the "source" of a project-structure + // change when restoring a file or project from history -- see + // overleaf-editor-core/lib/origin/restore_project_origin.js. It carries + // the version/timestamp of the restored version, needed downstream to + // build a valid history Change origin. + before(async function () { + this.project_id = DocUpdaterClient.randomId() + this.docUpdate = { + type: 'add-doc', + id: DocUpdaterClient.randomId(), + pathname: '/file-path', + docLines: 'a\nb', + } + this.updates = [this.docUpdate] + this.origin = { + kind: 'project-restore', + version: 3, + timestamp: new Date().toISOString(), + } + await sendProjectUpdateAndWait( + this.project_id, + this.user_id, + this.updates, + this.version, + this.origin + ) + }) + + it('should push the doc addition to the project history api with the origin', function (done) { + rclientProjectHistory.lrange( + ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), + 0, + -1, + (error, updates) => { + if (error) { + return done(error) + } + + const update = JSON.parse(updates[0]) + update.doc.should.equal(this.docUpdate.id) + update.meta.user_id.should.equal(this.user_id) + expect(update.meta.source).to.not.exist + expect(update.meta.origin).to.deep.equal(this.origin) + expect(update.meta.type).to.equal('external') + + done() + } + ) + }) + }) + + describe('adding a doc with a null userId (e.g. a TPDS/GitHub-sync update)', function () { + // web sends an explicit `null` userId (not an omitted field) for + // system-initiated project-structure changes that aren't attributable to + // a specific user -- e.g. TpdsController.mjs's updateProjectContents + // calls UpdateMerger.promises.mergeUpdate(null, ...) for GitHub-sync + // updates. + before(async function () { + this.project_id = DocUpdaterClient.randomId() + this.docUpdate = { + type: 'add-doc', + id: DocUpdaterClient.randomId(), + pathname: '/file-path', + docLines: 'a\nb', + } + this.updates = [this.docUpdate] + await sendProjectUpdateAndWait( + this.project_id, + null, + this.updates, + this.version + ) + }) + + it('should push the doc addition to the project history api without a user_id', function (done) { + rclientProjectHistory.lrange( + ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), + 0, + -1, + (error, updates) => { + if (error) { + return done(error) + } + + const update = JSON.parse(updates[0]) + update.doc.should.equal(this.docUpdate.id) + expect(update.meta.user_id).to.be.null + + done() + } + ) + }) + }) + + describe('adding a doc with an invalid userId', function () { + before(async function () { + this.project_id = DocUpdaterClient.randomId() + this.docUpdate = { + type: 'add-doc', + id: DocUpdaterClient.randomId(), + pathname: '/file-path', + docLines: 'a\nb', + } + this.updates = [this.docUpdate] + try { + await sendProjectUpdateAndWait( + this.project_id, + 'not-an-object-id', + this.updates, + this.version + ) + this.statusCode = 200 + } catch (err) { + if (err instanceof RequestFailedError) { + this.statusCode = err.response.status + } else { + throw err + } + } + }) + + it('should return a 400 status code', function () { + this.statusCode.should.equal(400) + }) + + it('should not push anything to the project history api', function (done) { + rclientProjectHistory.lrange( + ProjectHistoryKeys.projectHistoryOps({ project_id: this.project_id }), + 0, + -1, + (error, updates) => { + if (error) { + return done(error) + } + + updates.should.deep.equal([]) + + done() + } + ) + }) + }) + describe('with enough updates to flush to the history service', function () { before(async function () { this.project_id = DocUpdaterClient.randomId() diff --git a/services/document-updater/test/acceptance/js/GettingADocumentTests.js b/services/document-updater/test/acceptance/js/GettingADocumentTests.js index fe54288472..002aad44c4 100644 --- a/services/document-updater/test/acceptance/js/GettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/GettingADocumentTests.js @@ -5,6 +5,10 @@ const MockWebApi = require('./helpers/MockWebApi') const DocUpdaterClient = require('./helpers/DocUpdaterClient') const DocUpdaterApp = require('./helpers/DocUpdaterApp') const { RequestFailedError } = require('@overleaf/fetch-utils') +const { getRawReqInput } = require('@overleaf/validation-tools') +const { + expectValidationError, +} = require('@overleaf/validation-tools/testUtils') const PersistenceManager = require('../../../app/js/PersistenceManager') describe('Getting a document', function () { @@ -57,7 +61,7 @@ describe('Getting a document', function () { sinon .stub(MockWebApi, 'getDocumentController') .callsFake((req, res, next) => { - expect(req.query.peek).to.equal('true') + expect(getRawReqInput(req).query.peek).to.equal('true') return origGetDocumentController(req, res, next) }) this.project_id = DocUpdaterClient.randomId() @@ -257,6 +261,41 @@ describe('Getting a document', function () { }) }) + describe('when the doc id is malformed', function () { + it('should return 404', async function () { + const projectId = DocUpdaterClient.randomId() + let err + try { + await DocUpdaterClient.getDoc(projectId, 'not-an-object-id') + expect.fail('should have thrown') + } catch (error) { + err = error + } + expectValidationError(err, 404, 'doc_id') + }) + }) + + describe('when the project id is malformed', function () { + it('should return 404', async function () { + const docId = DocUpdaterClient.randomId() + let err + try { + // encodeURIComponent so the traversal payload arrives as a literal + // path segment (the value of params.project_id) instead of being + // collapsed by fetch()/URL's dot-segment normalization before the + // request reaches the wire. + await DocUpdaterClient.getDoc( + encodeURIComponent('../../etc/passwd'), + docId + ) + expect.fail('should have thrown') + } catch (error) { + err = error + } + expectValidationError(err, 404, 'project_id') + }) + }) + describe('when the web api returns an error', function () { beforeEach(function () { sinon.stub(MockWebApi, 'getDocument').rejects(new Error('oops')) diff --git a/services/document-updater/test/acceptance/js/SettingADocumentTests.js b/services/document-updater/test/acceptance/js/SettingADocumentTests.js index 8fff000219..1154ad738e 100644 --- a/services/document-updater/test/acceptance/js/SettingADocumentTests.js +++ b/services/document-updater/test/acceptance/js/SettingADocumentTests.js @@ -34,7 +34,7 @@ describe('Setting a document', function () { this.result = ['one', 'one and a half', 'two', 'three'] this.newLines = ['these', 'are', 'the', 'new', 'lines'] this.source = 'dropbox' - this.user_id = 'user-id-123' + this.user_id = DocUpdaterClient.randomId() sinon.spy(MockProjectHistoryApi, 'flushProject') sinon.spy(MockWebApi, 'setDocument') @@ -83,7 +83,15 @@ describe('Setting a document', function () { it('should send the updated doc lines and version to the web api', function () { MockWebApi.setDocument - .calledWith(this.project_id, this.doc_id, this.newLines) + .calledWith( + this.project_id, + this.doc_id, + this.newLines, + this.version + 2, + {}, + sinon.match.string, + this.user_id + ) .should.equal(true) }) @@ -302,6 +310,83 @@ describe('Setting a document', function () { }) }) + describe('with a null user id', function () { + before(async function () { + numberOfReceivedUpdates = 0 + this.project_id = DocUpdaterClient.randomId() + this.doc_id = DocUpdaterClient.randomId() + MockWebApi.insertDoc(this.project_id, this.doc_id, { + lines: this.lines, + version: this.version, + }) + await DocUpdaterClient.preloadDoc(this.project_id, this.doc_id) + await DocUpdaterClient.sendUpdate( + this.project_id, + this.doc_id, + this.update + ) + await setTimeout(200) + this.body = await DocUpdaterClient.setDocLines( + this.project_id, + this.doc_id, + this.newLines, + this.source, + null, + false + ) + }) + + after(function () { + MockProjectHistoryApi.flushProject.resetHistory() + MockWebApi.setDocument.resetHistory() + }) + + it('should emit two updates (from sendUpdate and setDocLines)', function () { + expect(numberOfReceivedUpdates).to.equal(2) + }) + + it('should send the updated doc lines and version to the web api', function () { + MockWebApi.setDocument + .calledWith( + this.project_id, + this.doc_id, + this.newLines, + this.version + 2, + {}, + sinon.match.string, + null + ) + .should.equal(true) + }) + + it('should update the lines in the doc updater', async function () { + const doc = await DocUpdaterClient.getDoc(this.project_id, this.doc_id) + doc.lines.should.deep.equal(this.newLines) + }) + + it('should bump the version in the doc updater', async function () { + const doc = await DocUpdaterClient.getDoc(this.project_id, this.doc_id) + doc.version.should.equal(this.version + 2) + }) + + it('should leave the document in redis', function (done) { + docUpdaterRedis.get( + Keys.docLines({ doc_id: this.doc_id }), + (error, lines) => { + if (error) { + throw error + } + expect(JSON.parse(lines)).to.deep.equal(this.newLines) + done() + } + ) + }) + + it('should return the mongo rev in the json response', function () { + this.body.should.deep.equal({ rev: '123' }) + }) + }) + const DOC_TOO_LARGE_TEST_CASES = [ { desc: 'when the updated doc is too large for the body parser', diff --git a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js index ec9d0d1239..c9992588e2 100644 --- a/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js +++ b/services/document-updater/test/acceptance/js/helpers/DocUpdaterClient.js @@ -237,10 +237,10 @@ module.exports = DocUpdaterClient = { ) }, - async sendProjectUpdate(projectId, userId, updates, version) { + async sendProjectUpdate(projectId, userId, updates, version, source) { await fetchNothing(`http://127.0.0.1:3003/project/${projectId}`, { method: 'POST', - json: { userId, updates, version }, + json: { userId, updates, version, source }, }) }, } diff --git a/services/document-updater/test/acceptance/js/helpers/MockDocstoreApi.js b/services/document-updater/test/acceptance/js/helpers/MockDocstoreApi.js index 334f8bc4fa..1cc6574862 100644 --- a/services/document-updater/test/acceptance/js/helpers/MockDocstoreApi.js +++ b/services/document-updater/test/acceptance/js/helpers/MockDocstoreApi.js @@ -1,8 +1,41 @@ const express = require('express') const { expressify } = require('@overleaf/promise-utils') +const { + handleValidationError, + parseReq, + z, + zz, +} = require('@overleaf/validation-tools') const app = express() const MAX_REQUEST_SIZE = 2 * (2 * 1024 * 1024 + 64 * 1024) +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(), + }), +}) + +// the partially-deleted-doc fixup payload sent by +// scripts/check_redis_mongo_sync_state.js +const patchDocumentSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + doc_id: zz.objectId(), + }), + body: z.strictObject({ + name: z.string(), + deleted: z.boolean(), + deletedAt: z.iso.datetime(), + }), +}) + const MockDocstoreApi = { docs: {}, @@ -48,8 +81,9 @@ const MockDocstoreApi = { app.get( '/project/:project_id/doc-deleted', expressify(async (req, res) => { + const { params } = parseReq(req, projectParamsSchema) try { - const docs = await this.getAllDeletedDocs(req.params.project_id) + const docs = await this.getAllDeletedDocs(params.project_id) return res.json(docs) } catch (error) { return res.sendStatus(500) @@ -60,11 +94,9 @@ const MockDocstoreApi = { app.get( '/project/:project_id/doc/:doc_id/peek', expressify(async (req, res) => { + const { params } = parseReq(req, docParamsSchema) try { - const doc = await this.peekDocument( - req.params.project_id, - req.params.doc_id - ) + const doc = await this.peekDocument(params.project_id, params.doc_id) if (doc) { return res.json(doc) } else { @@ -80,11 +112,12 @@ const MockDocstoreApi = { '/project/:project_id/doc/:doc_id', express.json({ limit: MAX_REQUEST_SIZE }), expressify(async (req, res) => { + const { params, body } = parseReq(req, patchDocumentSchema) try { await MockDocstoreApi.patchDocument( - req.params.project_id, - req.params.doc_id, - req.body + params.project_id, + params.doc_id, + body ) return res.sendStatus(204) } catch (error) { @@ -93,6 +126,8 @@ const MockDocstoreApi = { }) ) + app.use(handleValidationError) + app .listen(3016, error => { if (error) { diff --git a/services/document-updater/test/acceptance/js/helpers/MockProjectHistoryApi.js b/services/document-updater/test/acceptance/js/helpers/MockProjectHistoryApi.js index 21237fdf94..dcccf6452b 100644 --- a/services/document-updater/test/acceptance/js/helpers/MockProjectHistoryApi.js +++ b/services/document-updater/test/acceptance/js/helpers/MockProjectHistoryApi.js @@ -1,8 +1,20 @@ let MockProjectHistoryApi const { expressify } = require('@overleaf/promise-utils') const express = require('express') +const { + handleValidationError, + parseReq, + z, + zz, +} = require('@overleaf/validation-tools') const app = express() +const flushProjectSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + }), +}) + module.exports = MockProjectHistoryApi = { async flushProject(docId) {}, @@ -10,8 +22,9 @@ module.exports = MockProjectHistoryApi = { app.post( '/project/:project_id/flush', expressify(async (req, res, next) => { + const { params } = parseReq(req, flushProjectSchema) try { - await this.flushProject(req.params.project_id) + await this.flushProject(params.project_id) return res.sendStatus(204) } catch (error) { return res.sendStatus(500) @@ -19,6 +32,8 @@ module.exports = MockProjectHistoryApi = { }) ) + app.use(handleValidationError) + return app.listen(3054, error => { if (error != null) { throw error diff --git a/services/document-updater/test/acceptance/js/helpers/MockWebApi.js b/services/document-updater/test/acceptance/js/helpers/MockWebApi.js index dde8e89968..07447bd38e 100644 --- a/services/document-updater/test/acceptance/js/helpers/MockWebApi.js +++ b/services/document-updater/test/acceptance/js/helpers/MockWebApi.js @@ -4,9 +4,41 @@ const tsscmp = require('tsscmp') const express = require('express') const { expressify } = require('@overleaf/promise-utils') const Settings = require('@overleaf/settings') +const { + handleValidationError, + parseReq, + z, + zz, +} = require('@overleaf/validation-tools') +const { rawStringFileData } = require('overleaf-editor-core/lib/schemas') +const schemas = require('../../../../app/js/schemas') const app = express() const MAX_REQUEST_SIZE = 2 * (2 * 1024 * 1024 + 64 * 1024) +const getDocumentSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + doc_id: zz.objectId(), + }), +}) + +// the flush payload built by PersistenceManager.setDoc: lines are either +// sharejs lines or history-ot raw file data; lastUpdatedAt is a redis string +// (ms since epoch) unless freshly set in-process +const setDocumentSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + doc_id: zz.objectId(), + }), + body: z.strictObject({ + lines: z.array(z.string()).or(rawStringFileData), + version: z.number().int(), + ranges: schemas.ranges.nullish(), + lastUpdatedAt: z.union([z.string(), z.number()]).nullish(), + lastUpdatedBy: z.string().nullish(), + }), +}) + module.exports = MockWebApi = { docs: {}, @@ -52,11 +84,9 @@ module.exports = MockWebApi = { }, async getDocumentController(req, res, next) { + const { params } = parseReq(req, getDocumentSchema) try { - const doc = await this.getDocument( - req.params.project_id, - req.params.doc_id - ) + const doc = await this.getDocument(params.project_id, params.doc_id) if (doc != null) { return res.send(JSON.stringify(doc)) } else { @@ -68,15 +98,16 @@ module.exports = MockWebApi = { }, async setDocumentController(req, res, next) { + const { params, body } = parseReq(req, setDocumentSchema) try { const ok = await this.setDocument( - req.params.project_id, - req.params.doc_id, - req.body.lines, - req.body.version, - req.body.ranges, - req.body.lastUpdatedAt, - req.body.lastUpdatedBy + params.project_id, + params.doc_id, + body.lines, + body.version, + body.ranges, + body.lastUpdatedAt, + body.lastUpdatedBy ) if (!ok) { return res.sendStatus(404) @@ -118,6 +149,8 @@ module.exports = MockWebApi = { }) ) + app.use(handleValidationError) + return app .listen(3000, error => { if (error != null) { diff --git a/services/document-updater/test/setup.js b/services/document-updater/test/setup.js index 13fb106745..26e211bd52 100644 --- a/services/document-updater/test/setup.js +++ b/services/document-updater/test/setup.js @@ -32,6 +32,9 @@ SandboxedModule.configure({ '@overleaf/logger': stubs.logger, 'mongodb-legacy': require('mongodb-legacy'), // for ObjectId comparisons 'overleaf-editor-core': require('overleaf-editor-core'), // does not play nice with sandbox + // pulls in mongodb (via zod ObjectId helpers), which does not compile in + // the sandbox (missing TextEncoder) + '@overleaf/validation-tools': require('@overleaf/validation-tools'), }, globals: { Buffer, JSON, Math, console, process, URL }, sourceTransformers: { diff --git a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js index 69ea98ffc4..16969bb22c 100644 --- a/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js +++ b/services/document-updater/test/unit/js/HttpController/HttpControllerTests.js @@ -5,9 +5,10 @@ const Errors = require('../../../../app/js/Errors.js') describe('HttpController', function () { beforeEach(function () { - this.project_id = 'project-id-123' + this.project_id = 'aaaaaaaaaaaaaaaaaaaaaaaa' this.projectHistoryId = '123' - this.doc_id = 'doc-id-123' + this.doc_id = 'bbbbbbbbbbbbbbbbbbbbbbbb' + this.comment_id = 'cccccccccccccccccccccccc' this.source = 'editor' this.next = sinon.stub() this.res = { @@ -308,7 +309,7 @@ describe('HttpController', function () { beforeEach(function () { this.lines = ['one', 'two', 'three'] this.source = 'dropbox' - this.user_id = 'user-id-123' + this.user_id = 'dddddddddddddddddddddddd' this.req = { headers: {}, params: { @@ -650,7 +651,7 @@ describe('HttpController', function () { describe('with the background=true option from realtime', function () { beforeEach(async function () { - this.req.query = { background: true, shutdown: true } + this.req.query = { background: 'true', shutdown: 'true' } await this.HttpController.deleteProject(this.req, this.res, this.next) }) @@ -681,7 +682,7 @@ describe('HttpController', function () { params: { project_id: this.project_id, doc_id: this.doc_id, - change_id: (this.change_id = 'mock-change-od-1'), + change_id: (this.change_id = 'ddddddddddddddddddddddd1'), }, query: {}, body: {}, @@ -729,10 +730,10 @@ describe('HttpController', function () { describe('succesfully with with multiple changes', function () { beforeEach(async function () { this.change_ids = [ - 'mock-change-od-1', - 'mock-change-od-2', - 'mock-change-od-3', - 'mock-change-od-4', + 'ddddddddddddddddddddddd1', + 'ddddddddddddddddddddddd2', + 'ddddddddddddddddddddddd3', + 'ddddddddddddddddddddddd4', ] this.req.body = { change_ids: this.change_ids } await this.HttpController.acceptChanges(this.req, this.res, this.next) @@ -772,12 +773,12 @@ describe('HttpController', function () { describe('resolveComment', function () { beforeEach(function () { - this.user_id = 'user-id-123' + this.user_id = 'dddddddddddddddddddddddd' this.req = { params: { project_id: this.project_id, doc_id: this.doc_id, - comment_id: (this.comment_id = 'mock-comment-id'), + comment_id: (this.comment_id = 'cccccccccccccccccccccccc'), }, query: {}, body: { @@ -836,12 +837,12 @@ describe('HttpController', function () { describe('reopenComment', function () { beforeEach(function () { - this.user_id = 'user-id-123' + this.user_id = 'dddddddddddddddddddddddd' this.req = { params: { project_id: this.project_id, doc_id: this.doc_id, - comment_id: (this.comment_id = 'mock-comment-id'), + comment_id: (this.comment_id = 'cccccccccccccccccccccccc'), }, query: {}, body: { @@ -900,12 +901,12 @@ describe('HttpController', function () { describe('deleteComment', function () { beforeEach(function () { - this.user_id = 'user-id-123' + this.user_id = 'dddddddddddddddddddddddd' this.req = { params: { project_id: this.project_id, doc_id: this.doc_id, - comment_id: (this.comment_id = 'mock-comment-id'), + comment_id: (this.comment_id = 'cccccccccccccccccccccccc'), }, query: {}, body: { @@ -997,8 +998,7 @@ describe('HttpController', function () { it('should get docs from the project manager', function () { this.ProjectManager.promises.getProjectDocsAndFlushIfOld.should.have.been.calledWith( this.project_id, - this.state, - {} + this.state ) }) @@ -1008,10 +1008,7 @@ describe('HttpController', function () { it('should log the request', function () { this.logger.debug - .calledWith( - { projectId: this.project_id, exclude: [] }, - 'getting docs via http' - ) + .calledWith({ projectId: this.project_id }, 'getting docs via http') .should.equal(true) }) @@ -1066,23 +1063,33 @@ describe('HttpController', function () { describe('updateProject', function () { beforeEach(function () { - this.projectHistoryId = 'history-id-123' - this.userId = 'user-id-123' + this.projectHistoryId = 'ffffffffffffffffffffffff' + this.userId = 'dddddddddddddddddddddddd' this.updates = [ { type: 'rename-doc', - id: 1, + id: 'aaaaaaaaaaaaaaaaaaaaaaa1', pathname: 'thesis.tex', newPathname: 'book.tex', }, - { type: 'add-doc', id: 2, pathname: 'article.tex', docLines: 'hello' }, + { + type: 'add-doc', + id: 'aaaaaaaaaaaaaaaaaaaaaaa2', + pathname: 'article.tex', + docLines: 'hello', + }, { type: 'rename-file', - id: 3, + id: 'aaaaaaaaaaaaaaaaaaaaaaa3', pathname: 'apple.png', newPathname: 'banana.png', }, - { type: 'add-file', id: 4, url: 'filestore.example.com/4' }, + { + type: 'add-file', + id: 'aaaaaaaaaaaaaaaaaaaaaaa4', + pathname: 'banana.png', + url: 'filestore.example.com/4', + }, ] this.version = 1234567 this.req = { @@ -1137,14 +1144,54 @@ describe('HttpController', function () { this.next.calledWith(sinon.match.instanceOf(Error)).should.equal(true) }) }) + + describe('when projectHistoryId is malformed', function () { + beforeEach(async function () { + this.req.body.projectHistoryId = '../../../../etc/passwd' + await this.HttpController.updateProject(this.req, this.res, this.next) + }) + + it('should call next with the error', function () { + this.next.calledWith(sinon.match.instanceOf(Error)).should.equal(true) + }) + + it('should not update the project', function () { + this.ProjectManager.promises.updateProjectWithLocks.called.should.equal( + false + ) + }) + }) + + describe('when an add-file update has a malformed hash', function () { + beforeEach(async function () { + this.req.body.updates[3].hash = 'not-a-valid-hash' + await this.HttpController.updateProject(this.req, this.res, this.next) + }) + + it('should call next with the error', function () { + this.next.calledWith(sinon.match.instanceOf(Error)).should.equal(true) + }) + + it('should not update the project', function () { + this.ProjectManager.promises.updateProjectWithLocks.called.should.equal( + false + ) + }) + }) }) describe('resyncProjectHistory', function () { beforeEach(function () { - this.projectHistoryId = 'history-id-123' - this.docs = sinon.stub() - this.files = sinon.stub() - this.fileUpdates = sinon.stub() + this.projectHistoryId = 'ffffffffffffffffffffffff' + this.docs = [{ doc: 'bbbbbbbbbbbbbbbbbbbbbbbb', path: 'main.tex' }] + this.files = [ + { + file: 'eeeeeeeeeeeeeeeeeeeeeeee', + path: 'universe.png', + _hash: 'eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee', + createdBlob: true, + }, + ] this.req = { query: {}, body: { @@ -1198,13 +1245,55 @@ describe('HttpController', function () { this.next.calledWith(sinon.match.instanceOf(Error)).should.equal(true) }) }) + + describe('when projectHistoryId is malformed', function () { + beforeEach(async function () { + this.req.body.projectHistoryId = '../../../../etc/passwd' + await this.HttpController.resyncProjectHistory( + this.req, + this.res, + this.next + ) + }) + + it('should call next with the error', function () { + this.next.calledWith(sinon.match.instanceOf(Error)).should.equal(true) + }) + + it('should not resync the project history', function () { + this.HistoryManager.promises.resyncProjectHistory.called.should.equal( + false + ) + }) + }) + + describe('when a file _hash is malformed', function () { + beforeEach(async function () { + this.req.body.files[0]._hash = 'not-a-valid-hash' + await this.HttpController.resyncProjectHistory( + this.req, + this.res, + this.next + ) + }) + + it('should call next with the error', function () { + this.next.calledWith(sinon.match.instanceOf(Error)).should.equal(true) + }) + + it('should not resync the project history', function () { + this.HistoryManager.promises.resyncProjectHistory.called.should.equal( + false + ) + }) + }) }) describe('appendToDoc', function () { beforeEach(function () { this.lines = ['one', 'two', 'three'] this.source = 'dropbox' - this.user_id = 'user-id-123' + this.user_id = 'dddddddddddddddddddddddd' this.req = { headers: {}, params: { @@ -1216,7 +1305,6 @@ describe('HttpController', function () { lines: this.lines, source: this.source, user_id: this.user_id, - undoing: (this.undoing = true), }, } }) diff --git a/services/document-updater/test/unit/js/ProjectManager/getProjectDocsTests.js b/services/document-updater/test/unit/js/ProjectManager/getProjectDocsTests.js index db95ec3ce2..66c8182148 100644 --- a/services/document-updater/test/unit/js/ProjectManager/getProjectDocsTests.js +++ b/services/document-updater/test/unit/js/ProjectManager/getProjectDocsTests.js @@ -78,8 +78,7 @@ describe('ProjectManager - getProjectDocsAndFlushIfOld', function () { this.result = await this.ProjectManager.promises.getProjectDocsAndFlushIfOld( this.project_id, - this.projectStateHash, - this.excludeVersions + this.projectStateHash ) }) @@ -112,8 +111,7 @@ describe('ProjectManager - getProjectDocsAndFlushIfOld', function () { await expect( this.ProjectManager.promises.getProjectDocsAndFlushIfOld( this.project_id, - this.projectStateHash, - this.excludeVersions + this.projectStateHash ) ).to.be.rejectedWith(Errors.ProjectStateChangedError) }) @@ -147,8 +145,7 @@ describe('ProjectManager - getProjectDocsAndFlushIfOld', function () { await expect( this.ProjectManager.promises.getProjectDocsAndFlushIfOld( this.project_id, - this.projectStateHash, - this.excludeVersions + this.projectStateHash ) ).to.be.rejected }) diff --git a/yarn.lock b/yarn.lock index 7f1bcabe06..af92f8024c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7172,6 +7172,7 @@ __metadata: "@overleaf/ranges-tracker": "workspace:*" "@overleaf/redis-wrapper": "workspace:*" "@overleaf/settings": "workspace:*" + "@overleaf/validation-tools": "workspace:*" "@types/chai-as-promised": "npm:^7.1.8" "@types/ioredis": "npm:^4.28.10" "@types/mocha": "npm:^10.0.6"