From e2564f356c991bae85b298c7becfb1bb51ecaafb Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Fri, 14 Aug 2026 08:27:41 +0200 Subject: [PATCH] Merge pull request #36222 from overleaf/jpa-zod-web-core-1 [web] core-1: adopt strict zod schema and extend test coverage GitOrigin-RevId: 6dc74a100989e6eca1565480a9ecb5f24404ef96 --- .../app/src/Features/Chat/ChatController.mjs | 74 ++++- .../DocumentUpdaterController.mjs | 13 +- .../Features/Documents/DocumentController.mjs | 135 ++++++++- .../Downloads/ProjectDownloadsController.mjs | 46 ++- .../Features/Editor/EditorHttpController.mjs | 135 +++++++-- .../app/src/Features/Editor/EditorRouter.mjs | 10 +- .../Features/History/HistoryController.mjs | 138 ++++++++- .../src/Features/Metadata/MetaController.mjs | 28 +- .../Notifications/NotificationsController.mjs | 17 +- .../Features/Project/ProjectApiController.mjs | 24 +- .../Features/Project/ProjectController.mjs | 171 +++++++++-- .../Project/ProjectListController.mjs | 41 ++- .../web/app/src/Features/Project/types.d.ts | 7 +- .../Features/Spelling/SpellingController.mjs | 22 +- .../app/src/Features/Tags/TagsController.mjs | 120 +++++++- .../Uploads/ProjectUploadController.mjs | 76 ++++- .../src/Features/Uploads/UploadsRouter.mjs | 3 +- .../acceptance/src/ProjectStructureTests.mjs | 2 +- .../web/test/acceptance/src/ChatTests.mjs | 177 +++++++++++ .../test/acceptance/src/DocUpdateTests.mjs | 2 +- .../src/EditorHttpControllerTests.mjs | 154 ++++++++++ .../src/MalformedFiletreesTests.mjs | 16 +- .../test/acceptance/src/ProjectCRUDTests.mjs | 135 +++++++++ .../src/ProjectDuplicateNameTests.mjs | 2 - .../acceptance/src/ProjectInviteTests.mjs | 255 ++++++++++++++++ .../acceptance/src/ProjectStructureTests.mjs | 23 ++ .../web/test/acceptance/src/SharingTests.mjs | 189 ++++++++++++ .../web/test/acceptance/src/TagsTests.mjs | 275 +++++++++++++++++- .../acceptance/src/mocks/MockClsiCache.mjs | 23 +- .../acceptance/src/mocks/MockClsiNginxApi.mjs | 7 +- .../unit/src/Chat/ChatController.test.mjs | 68 ++++- .../CollaboratorsController.test.mjs | 253 +++++++++++++++- .../DocumentUpdaterController.test.mjs | 4 +- .../src/Documents/DocumentController.test.mjs | 20 +- .../ProjectDownloadsController.test.mjs | 131 ++++++++- .../src/Editor/EditorHttpController.test.mjs | 95 +++--- .../src/History/HistoryController.test.mjs | 164 +++++++++++ .../unit/src/Metadata/MetaController.test.mjs | 89 +++++- .../NotificationsController.test.mjs | 31 +- .../src/Project/ProjectApiController.test.mjs | 50 +++- .../src/Project/ProjectController.test.mjs | 91 ++++-- .../Project/ProjectListController.test.mjs | 116 +++++++- .../unit/src/Tags/TagsController.test.mjs | 107 +++++-- .../Uploads/ProjectUploadController.test.mjs | 146 +++++++++- 44 files changed, 3350 insertions(+), 335 deletions(-) create mode 100644 services/web/test/acceptance/src/ChatTests.mjs diff --git a/services/web/app/src/Features/Chat/ChatController.mjs b/services/web/app/src/Features/Chat/ChatController.mjs index 3cbd7e0132..13d25eee0b 100644 --- a/services/web/app/src/Features/Chat/ChatController.mjs +++ b/services/web/app/src/Features/Chat/ChatController.mjs @@ -6,10 +6,24 @@ import SessionManager from '../Authentication/SessionManager.mjs' import UserInfoManager from '../User/UserInfoManager.mjs' import UserInfoController from '../User/UserInfoController.mjs' import ChatManager from './ChatManager.mjs' +import { z, zz, parseReq } from '../../infrastructure/Validation.mjs' + +const sendMessageSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + }), + body: z.strictObject({ + content: z.string(), + client_id: z.string().optional(), + }), +}) async function sendMessage(req, res) { - const { project_id: projectId } = req.params - const { content, client_id: clientId } = req.body + const { params, body } = parseReq(req, sendMessageSchema, { + logOnly: true, + }) + const { project_id: projectId } = params + const { content, client_id: clientId } = body const userId = SessionManager.getLoggedInUserId(req.session) if (userId == null) { throw new Error('no logged-in user') @@ -35,9 +49,34 @@ async function sendMessage(req, res) { res.sendStatus(204) } +const getMessagesSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + }), + query: z.object({ + limit: z.coerce.number().int().optional(), + before: z.coerce.number().int().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 getMessagesFallbackSchema = z.object({ + params: z.object({ + project_id: z.string(), + }), + query: z.object({ + limit: z.coerce.number().optional(), + before: z.coerce.number().optional(), + }), +}) + async function getMessages(req, res) { - const { project_id: projectId } = req.params - const { query } = req + const { params, query } = parseReq(req, getMessagesSchema, { + logOnly: true, + fallbackSchema: getMessagesFallbackSchema, + }) + const { project_id: projectId } = params const messages = await ChatApiHandler.promises.getGlobalMessages( projectId, query.limit, @@ -48,8 +87,16 @@ async function getMessages(req, res) { res.json(messages) } +const deleteMessageSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + message_id: zz.objectId(), + }), +}) + async function deleteMessage(req, res) { - const { project_id: projectId, message_id: messageId } = req.params + const { params } = parseReq(req, deleteMessageSchema, { logOnly: true }) + const { project_id: projectId, message_id: messageId } = params const userId = SessionManager.getLoggedInUserId(req.session) if (userId == null) { throw new Error('no logged-in user') @@ -64,9 +111,22 @@ async function deleteMessage(req, res) { res.sendStatus(204) } +const editMessageSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + message_id: zz.objectId(), + }), + body: z.strictObject({ + content: z.string(), + }), +}) + async function editMessage(req, res, next) { - const { project_id: projectId, message_id: messageId } = req.params - const { content } = req.body + const { params, body } = parseReq(req, editMessageSchema, { + logOnly: true, + }) + const { project_id: projectId, message_id: messageId } = params + const { content } = body const userId = SessionManager.getLoggedInUserId(req.session) if (userId == null) { throw new Error('no logged-in user') diff --git a/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterController.mjs b/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterController.mjs index 4de5446a31..fcbddb2844 100644 --- a/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterController.mjs +++ b/services/web/app/src/Features/DocumentUpdater/DocumentUpdaterController.mjs @@ -3,10 +3,19 @@ import DocumentUpdaterHandler from './DocumentUpdaterHandler.mjs' import ProjectLocator from '../Project/ProjectLocator.mjs' import { plainTextResponse } from '../../infrastructure/Response.mjs' import { expressify } from '@overleaf/promise-utils' +import { z, zz, parseReq } from '../../infrastructure/Validation.mjs' + +const getDocSchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + Doc_id: zz.objectId(), + }), +}) async function getDoc(req, res) { - const projectId = req.params.Project_id - const docId = req.params.Doc_id + const { params } = parseReq(req, getDocSchema, { logOnly: true }) + const projectId = params.Project_id + const docId = params.Doc_id try { const { element: doc } = await ProjectLocator.promises.findElement({ diff --git a/services/web/app/src/Features/Documents/DocumentController.mjs b/services/web/app/src/Features/Documents/DocumentController.mjs index cdb968cd2f..1c325e5ad2 100644 --- a/services/web/app/src/Features/Documents/DocumentController.mjs +++ b/services/web/app/src/Features/Documents/DocumentController.mjs @@ -8,11 +8,97 @@ import _ from 'lodash' import { plainTextResponse } from '../../infrastructure/Response.mjs' import { expressify } from '@overleaf/promise-utils' import Modules from '../../infrastructure/Modules.mjs' +import { z, zz, parseReq } from '../../infrastructure/Validation.mjs' + +// Zod schemas for the sharejs-text-ot ranges data (RangesTracker format) +// that document-updater flushes to this endpoint -- NOT the unrelated +// overleaf-editor-core StringFileData rawComment/rawTrackedChange shape +// (that's the canonical history-ot representation, a different shape used +// elsewhere for linked-file/history payloads). +// +// This mirrors document-updater's own app/js/schemas.js (the sender) and +// docstore's app/js/schemas.js (the next hop, which this data is forwarded +// to untouched via DocstoreManager) -- see those files' comments for why the +// fields are this permissive: ids aren't always ObjectIds (RangesTracker ids +// are seed+increment strings; legacy documents carry arbitrary string thread +// ids), and history restores send id-less changes/detached comments plus a +// `resolved` flag. +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(), + t: z.string().optional(), + u: z.boolean().optional(), + // sent by history restores; removed again by RangesManager + 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(), +}) + +const rangesSchema = z.strictObject({ + comments: z.array(comment).optional(), + changes: z.array(trackedChange).optional(), +}) + +const getDocumentSchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + doc_id: zz.objectId(), + }), + query: z.object({ + plain: z.stringbool().optional(), + peek: z.stringbool().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 getDocumentFallbackSchema = z.object({ + params: z.object({ + Project_id: z.string(), + doc_id: z.string(), + }), + query: z.object({ + plain: z.stringbool().optional(), + peek: z.stringbool().optional(), + }), +}) async function getDocument(req, res) { - const { Project_id: projectId, doc_id: docId } = req.params - const plain = req.query.plain === 'true' - const peek = req.query.peek === 'true' + const { params, query } = parseReq(req, getDocumentSchema, { + logOnly: true, + fallbackSchema: getDocumentFallbackSchema, + }) + const { Project_id: projectId, doc_id: docId } = params + const plain = query.plain === true + const peek = query.peek === true const project = await ProjectGetter.promises.getProject(projectId, { rootFolder: true, overleaf: true, @@ -77,9 +163,30 @@ async function getDocument(req, res) { } } +const setDocumentSchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + doc_id: zz.objectId(), + }), + body: z.strictObject({ + // by the time document-updater flushes a doc to web, `lines` has always + // been normalised to a plain string array (see + // DocumentManager.flushDocIfLoaded's `file.getLines()` call for + // history-ot docs) -- never the raw StringFileData shape. + lines: z.array(z.string()), + version: z.number().int(), + ranges: rangesSchema, + lastUpdatedAt: z.coerce.number().int().positive().nullish(), + lastUpdatedBy: zz.objectId().nullish(), + }), +}) + async function setDocument(req, res) { - const { Project_id: projectId, doc_id: docId } = req.params - const { lines, version, ranges, lastUpdatedAt, lastUpdatedBy } = req.body + const { params, body } = parseReq(req, setDocumentSchema, { + logOnly: true, + }) + const { Project_id: projectId, doc_id: docId } = params + const { lines, version, ranges, lastUpdatedAt, lastUpdatedBy } = body const result = await ProjectEntityUpdateHandler.promises.updateDocLines( projectId, docId, @@ -105,9 +212,23 @@ async function setDocument(req, res) { res.json(result) } +const trackChangesRejectedSchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + doc_id: zz.objectId(), + }), + body: z.strictObject({ + rejectedChangeAuthorIds: z.array(zz.objectId()), + userId: zz.objectId().optional(), + }), +}) + async function trackChangesRejected(req, res) { - const { Project_id: projectId, doc_id: docId } = req.params - const { rejectedChangeAuthorIds, userId } = req.body + const { params, body } = parseReq(req, trackChangesRejectedSchema, { + logOnly: true, + }) + const { Project_id: projectId, doc_id: docId } = params + const { rejectedChangeAuthorIds, userId } = body await Modules.promises.hooks.fire( 'trackChangesRejected', projectId, diff --git a/services/web/app/src/Features/Downloads/ProjectDownloadsController.mjs b/services/web/app/src/Features/Downloads/ProjectDownloadsController.mjs index cdedaf9ba5..7643613f12 100644 --- a/services/web/app/src/Features/Downloads/ProjectDownloadsController.mjs +++ b/services/web/app/src/Features/Downloads/ProjectDownloadsController.mjs @@ -23,7 +23,7 @@ const SUPPORTED_CONVERSION_TYPES = new Map([ ]) const exportProjectConversionSchema = z.object({ - params: z.object({ + params: z.strictObject({ Project_id: zz.objectId(), type: z.enum([...SUPPORTED_CONVERSION_TYPES.keys()]), }), @@ -34,7 +34,7 @@ const exportProjectConversionSchema = z.object({ }) const downloadPreparedProjectExportSchema = z.object({ - params: z.object({ + params: z.strictObject({ Project_id: zz.objectId(), buildId: zz.buildId(), conversionId: z.uuid(), @@ -46,6 +46,37 @@ const downloadPreparedProjectExportSchema = z.object({ }), }) +const downloadProjectParamsSchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + }), +}) + +// Guarded by AuthorizationMiddleware.ensureUserCanReadMultipleProjects, which +// already rejects a request unless every comma-separated id can be read by +// the current user (an empty/missing project_ids fails that check too), so +// by the time this handler runs the field is a real, non-empty id list. +const downloadMultipleProjectsSchema = z.object({ + query: z.object({ + project_ids: z + .string() + .transform(s => s.split(',')) + .pipe(z.array(zz.objectId())), + }), +}) + +// 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 downloadMultipleProjectsFallbackSchema = z.object({ + query: z.object({ + project_ids: z + .string() + .transform(s => s.split(',')) + .pipe(z.array(z.string())), + }), +}) + // Keep in sync with the logic for PDF files in CompileController function getSafeProjectName(project) { return project.name.replace(/[^\p{L}\p{Nd}]/gu, '_') @@ -170,7 +201,10 @@ export default { downloadProject(req, res, next) { const userId = SessionManager.getLoggedInUserId(req.session) - const projectId = req.params.Project_id + const { params } = parseReq(req, downloadProjectParamsSchema, { + logOnly: true, + }) + const projectId = params.Project_id Metrics.inc('zip-downloads') DocumentUpdaterHandler.flushProjectToMongo(projectId, function (error) { if (error) { @@ -225,7 +259,11 @@ export default { downloadMultipleProjects(req, res, next) { const userId = SessionManager.getLoggedInUserId(req.session) - const projectIds = req.query.project_ids.split(',') + const { query } = parseReq(req, downloadMultipleProjectsSchema, { + logOnly: true, + fallbackSchema: downloadMultipleProjectsFallbackSchema, + }) + const projectIds = query.project_ids Metrics.inc('zip-downloads-multiple') DocumentUpdaterHandler.flushMultipleProjectsToMongo( projectIds, diff --git a/services/web/app/src/Features/Editor/EditorHttpController.mjs b/services/web/app/src/Features/Editor/EditorHttpController.mjs index 0da6a4b8e7..06f5554538 100644 --- a/services/web/app/src/Features/Editor/EditorHttpController.mjs +++ b/services/web/app/src/Features/Editor/EditorHttpController.mjs @@ -24,11 +24,25 @@ export default { deleteDoc: expressify(deleteDoc), deleteFile: expressify(deleteFile), deleteFolder: expressify(deleteFolder), - deleteEntity: expressify(deleteEntity), _nameIsAcceptableLength, } -const joinProjectSchema = z.object({ +export const joinProjectSchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + }), + body: z.strictObject({ + // the real-time service sends the sentinel 'anonymous-user' for + // link-sharing visitors with no account (see real-time's + // Router.js/WebsocketController.js), otherwise a genuine user id + userId: zz.objectId().or(z.literal('anonymous-user')), + anonymousAccessToken: z.string().optional(), + }), +}) + +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const joinProjectFallbackSchema = z.object({ params: z.object({ Project_id: zz.objectId(), }), @@ -39,7 +53,9 @@ const joinProjectSchema = z.object({ }) async function joinProject(req, res, next) { - const { params, body } = parseReq(req, joinProjectSchema) + const { params, body } = parseReq(req, joinProjectSchema, { + fallbackSchema: joinProjectFallbackSchema, + }) const projectId = params.Project_id let userId = body.userId if (userId === 'anonymous-user') { @@ -52,7 +68,7 @@ async function joinProject(req, res, next) { isRestrictedUser, isTokenMember, isInvitedMember, - } = await _buildJoinProjectView(req, projectId, userId) + } = await _buildJoinProjectView(projectId, userId, body.anonymousAccessToken) if (!project) { return res.sendStatus(403) } @@ -76,13 +92,12 @@ async function joinProject(req, res, next) { }) } -async function _buildJoinProjectView(req, projectId, userId) { +async function _buildJoinProjectView(projectId, userId, token) { const project = await ProjectGetter.promises.getProject(projectId) if (project == null) { throw new Errors.NotFoundError('project not found') } const projectAccess = new ProjectAccess(project) - const token = req.body.anonymousAccessToken const privilegeLevel = await AuthorizationManager.promises.getPrivilegeLevelForProjectWithProjectAccess( userId, @@ -140,10 +155,37 @@ function _nameIsAcceptableLength(name) { return name != null && name.length < 150 && name.length !== 0 } +const addEntitySchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + }), + body: z.strictObject({ + // an unacceptable (missing/empty/too-long) name is handled below by + // _nameIsAcceptableLength, which replies with a bare 400 -- kept + // optional here so that path (not a validation-error response) still + // runs for a missing name + name: z.string().optional(), + // top-level creation (directly under the project root) omits this + // field entirely, or sends it as null -- see file-tree-actionable.tsx's + // getSelectedParentFolderId, which does pass the root folder's own id + // explicitly for the interactive editor's "new file/folder" UI, but + // other real callers (e.g. ProjectStructureTests.mjs's + // createExampleFolder, mirroring third-party API usage) legitimately + // omit it or send null. ProjectEntityMongoUpdateHandler's + // _confirmFolder() already treats a missing/null parentFolderId + // (`== null`) as "use the project's root folder", exactly like + // ProjectUploadController's analogous folder_id/targetFolderId fields, + // so this is modeled the same way here rather than requiring every + // caller to resolve the root folder id up front. + parent_folder_id: zz.objectId().nullish(), + }), +}) + async function addDoc(req, res, next) { - const projectId = req.params.Project_id - const { name } = req.body - const parentFolderId = req.body.parent_folder_id + const { params, body } = parseReq(req, addEntitySchema, { logOnly: true }) + const projectId = params.Project_id + const { name } = body + const parentFolderId = body.parent_folder_id const userId = SessionManager.getLoggedInUserId(req.session) if (!_nameIsAcceptableLength(name)) { @@ -173,9 +215,10 @@ async function addDoc(req, res, next) { } async function addFolder(req, res, next) { - const projectId = req.params.Project_id - const { name } = req.body - const parentFolderId = req.body.parent_folder_id + const { params, body } = parseReq(req, addEntitySchema, { logOnly: true }) + const projectId = params.Project_id + const { name } = body + const parentFolderId = body.parent_folder_id const userId = SessionManager.getLoggedInUserId(req.session) if (!_nameIsAcceptableLength(name)) { return res.sendStatus(400) @@ -204,11 +247,28 @@ async function addFolder(req, res, next) { } } +const entityActionParamsSchema = z.strictObject({ + Project_id: zz.objectId(), + entity_id: zz.objectId(), + entity_type: z.enum(['doc', 'file', 'folder']), +}) + +const renameEntitySchema = z.object({ + params: entityActionParamsSchema, + body: z.strictObject({ + name: z.string().optional(), + source: z.string().optional(), + }), +}) + async function renameEntity(req, res, next) { - const projectId = req.params.Project_id - const entityId = req.params.entity_id - const entityType = req.params.entity_type - const { name, source = 'editor' } = req.body + const { params, body } = parseReq(req, renameEntitySchema, { + logOnly: true, + }) + const projectId = params.Project_id + const entityId = params.entity_id + const entityType = params.entity_type + const { name, source = 'editor' } = body if (!_nameIsAcceptableLength(name)) { return res.sendStatus(400) } @@ -224,12 +284,21 @@ async function renameEntity(req, res, next) { res.sendStatus(204) } +const moveEntitySchema = z.object({ + params: entityActionParamsSchema, + body: z.strictObject({ + folder_id: zz.objectId(), + source: z.string().optional(), + }), +}) + async function moveEntity(req, res, next) { - const projectId = req.params.Project_id - const entityId = req.params.entity_id - const entityType = req.params.entity_type - const folderId = req.body.folder_id - const source = req.body.source ?? 'editor' + const { params, body } = parseReq(req, moveEntitySchema, { logOnly: true }) + const projectId = params.Project_id + const entityId = params.entity_id + const entityType = params.entity_type + const folderId = body.folder_id + const source = body.source ?? 'editor' const userId = SessionManager.getLoggedInUserId(req.session) await EditorController.promises.moveEntity( projectId, @@ -242,25 +311,29 @@ async function moveEntity(req, res, next) { res.sendStatus(204) } +const deleteEntitySchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + entity_id: zz.objectId(), + }), +}) + async function deleteDoc(req, res, next) { - req.params.entity_type = 'doc' - await deleteEntity(req, res, next) + await _deleteEntity(req, res, 'doc') } async function deleteFile(req, res, next) { - req.params.entity_type = 'file' - await deleteEntity(req, res, next) + await _deleteEntity(req, res, 'file') } async function deleteFolder(req, res, next) { - req.params.entity_type = 'folder' - await deleteEntity(req, res, next) + await _deleteEntity(req, res, 'folder') } -async function deleteEntity(req, res, next) { - const projectId = req.params.Project_id - const entityId = req.params.entity_id - const entityType = req.params.entity_type +async function _deleteEntity(req, res, entityType) { + const { params } = parseReq(req, deleteEntitySchema, { logOnly: true }) + const projectId = params.Project_id + const entityId = params.entity_id const userId = SessionManager.getLoggedInUserId(req.session) await EditorController.promises.deleteEntity( projectId, diff --git a/services/web/app/src/Features/Editor/EditorRouter.mjs b/services/web/app/src/Features/Editor/EditorRouter.mjs index 20b8a6cb43..00c23f7fde 100644 --- a/services/web/app/src/Features/Editor/EditorRouter.mjs +++ b/services/web/app/src/Features/Editor/EditorRouter.mjs @@ -1,9 +1,12 @@ -import EditorHttpController from './EditorHttpController.mjs' +import EditorHttpController, { + joinProjectSchema, +} from './EditorHttpController.mjs' import AuthenticationController from '../Authentication/AuthenticationController.mjs' import AuthorizationMiddleware from '../Authorization/AuthorizationMiddleware.mjs' import { RateLimiter } from '../../infrastructure/RateLimiter.mjs' import RateLimiterMiddleware from '../Security/RateLimiterMiddleware.mjs' import AsyncLocalStorage from '../../infrastructure/AsyncLocalStorage.mjs' +import { parseReq } from '../../infrastructure/Validation.mjs' const rateLimiters = { addDocToProject: new RateLimiter('add-doc-to-project', { @@ -74,8 +77,9 @@ export default { AuthenticationController.requirePrivateApiAuth(), RateLimiterMiddleware.rateLimit(rateLimiters.joinProject, { params: ['Project_id'], - // keep schema in sync with controller - getUserId: req => req.body.userId, + // reuses the controller's own schema so the two can't drift + getUserId: req => + parseReq(req, joinProjectSchema, { logOnly: true }).body.userId, }), EditorHttpController.joinProject ) diff --git a/services/web/app/src/Features/History/HistoryController.mjs b/services/web/app/src/Features/History/HistoryController.mjs index 81fd7bc352..057cfc8878 100644 --- a/services/web/app/src/Features/History/HistoryController.mjs +++ b/services/web/app/src/Features/History/HistoryController.mjs @@ -62,7 +62,7 @@ async function headBlob(req, res) { } const requestBlobSchema = z.object({ - params: z.object({ + params: z.strictObject({ project_id: zz.coercedObjectId(), hash: zz.hex().length(40), }), @@ -141,6 +141,16 @@ function setBlobCacheHeaders(res, etag) { const proxyToHistoryApiSchema = z.object({ // both project_id and Project_id are accepted for backwards compatibility + params: z.strictObject({ + Project_id: zz.objectId().optional(), + project_id: zz.objectId().optional(), + doc_id: zz.objectId().optional(), + }), +}) + +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const proxyToHistoryApiFallbackSchema = z.object({ params: z.object({ Project_id: zz.objectId().optional(), project_id: zz.objectId().optional(), @@ -154,7 +164,9 @@ const proxyToHistoryApiSchema = z.object({ * @param {any} next */ async function proxyToHistoryApi(req, res, next) { - parseReq(req, proxyToHistoryApiSchema) + parseReq(req, proxyToHistoryApiSchema, { + fallbackSchema: proxyToHistoryApiFallbackSchema, + }) const userId = SessionManager.getLoggedInUserId(req.session) const url = settings.apis.project_history.url + req.url @@ -184,6 +196,14 @@ async function proxyToHistoryApi(req, res, next) { } const proxyToHistoryApiAndInjectUserDetailsSchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + }), +}) + +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const proxyToHistoryApiAndInjectUserDetailsFallbackSchema = z.object({ params: z.object({ Project_id: zz.objectId(), }), @@ -195,7 +215,9 @@ const proxyToHistoryApiAndInjectUserDetailsSchema = z.object({ * @param {any} next */ async function proxyToHistoryApiAndInjectUserDetails(req, res, next) { - parseReq(req, proxyToHistoryApiAndInjectUserDetailsSchema) + parseReq(req, proxyToHistoryApiAndInjectUserDetailsSchema, { + fallbackSchema: proxyToHistoryApiAndInjectUserDetailsFallbackSchema, + }) const userId = SessionManager.getLoggedInUserId(req.session) const url = settings.apis.project_history.url + req.url const body = await fetchJson(url, { @@ -207,6 +229,18 @@ async function proxyToHistoryApiAndInjectUserDetails(req, res, next) { } const resyncProjectHistorySchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + }), + body: z.strictObject({ + historyRangesMigration: z.enum(['forwards', 'backwards']).optional(), + resyncProjectStructureOnly: z.boolean().default(false), + }), +}) + +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const resyncProjectHistoryFallbackSchema = z.object({ params: z.object({ Project_id: zz.objectId(), }), @@ -224,7 +258,9 @@ const resyncProjectHistorySchema = z.object({ async function resyncProjectHistory(req, res, next) { // increase timeout to 6 minutes res.setTimeout(6 * 60 * 1000) - const { params, body } = parseReq(req, resyncProjectHistorySchema) + const { params, body } = parseReq(req, resyncProjectHistorySchema, { + fallbackSchema: resyncProjectHistoryFallbackSchema, + }) const projectId = params.Project_id const opts = { historyRangesMigration: body.historyRangesMigration, @@ -248,6 +284,18 @@ async function resyncProjectHistory(req, res, next) { } const restoreFileFromV2Schema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + }), + body: z.strictObject({ + version: z.number().int().min(0), + pathname: zz.filepath(), + }), +}) + +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const restoreFileFromV2FallbackSchema = z.object({ params: z.object({ project_id: zz.objectId(), }), @@ -263,7 +311,9 @@ const restoreFileFromV2Schema = z.object({ * @param {any} next */ async function restoreFileFromV2(req, res, next) { - const { params, body } = parseReq(req, restoreFileFromV2Schema) + const { params, body } = parseReq(req, restoreFileFromV2Schema, { + fallbackSchema: restoreFileFromV2FallbackSchema, + }) const { project_id: projectId } = params const { version, pathname } = body const userId = SessionManager.getLoggedInUserId(req.session) @@ -295,6 +345,18 @@ async function restoreFileFromV2(req, res, next) { } const revertFileSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + }), + body: z.strictObject({ + version: z.number().int().min(0), + pathname: zz.filepath(), + }), +}) + +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const revertFileFallbackSchema = z.object({ params: z.object({ project_id: zz.objectId(), }), @@ -310,7 +372,9 @@ const revertFileSchema = z.object({ * @param {any} next */ async function revertFile(req, res, next) { - const { params, body } = parseReq(req, revertFileSchema) + const { params, body } = parseReq(req, revertFileSchema, { + fallbackSchema: revertFileFallbackSchema, + }) const { project_id: projectId } = params const { version, pathname } = body const userId = SessionManager.getLoggedInUserId(req.session) @@ -343,6 +407,17 @@ async function revertFile(req, res, next) { } const revertProjectSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + }), + body: z.strictObject({ + version: z.number().int().min(0), + }), +}) + +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const revertProjectFallbackSchema = z.object({ params: z.object({ project_id: zz.objectId(), }), @@ -357,7 +432,9 @@ const revertProjectSchema = z.object({ * @param {any} next */ async function revertProject(req, res, next) { - const { params, body } = parseReq(req, revertProjectSchema) + const { params, body } = parseReq(req, revertProjectSchema, { + fallbackSchema: revertProjectFallbackSchema, + }) const { project_id: projectId } = params const { version } = body const userId = SessionManager.getLoggedInUserId(req.session) @@ -384,6 +461,14 @@ async function revertProject(req, res, next) { } const getLabelsSchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + }), +}) + +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const getLabelsFallbackSchema = z.object({ params: z.object({ Project_id: zz.objectId(), }), @@ -395,7 +480,9 @@ const getLabelsSchema = z.object({ * @param {any} next */ async function getLabels(req, res, next) { - const { params } = parseReq(req, getLabelsSchema) + const { params } = parseReq(req, getLabelsSchema, { + fallbackSchema: getLabelsFallbackSchema, + }) const projectId = params.Project_id let labels = await fetchJson( @@ -407,6 +494,18 @@ async function getLabels(req, res, next) { } const createLabelSchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + }), + body: z.strictObject({ + comment: z.string(), + version: z.number().int().min(0), + }), +}) + +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const createLabelFallbackSchema = z.object({ params: z.object({ Project_id: zz.objectId(), }), @@ -422,7 +521,9 @@ const createLabelSchema = z.object({ * @param {any} next */ async function createLabel(req, res, next) { - const { params, body } = parseReq(req, createLabelSchema) + const { params, body } = parseReq(req, createLabelSchema, { + fallbackSchema: createLabelFallbackSchema, + }) const projectId = params.Project_id const { comment, version } = body const userId = SessionManager.getLoggedInUserId(req.session) @@ -519,6 +620,15 @@ function _displayNameForUser(user) { } const deleteLabelSchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + label_id: zz.objectId(), + }), +}) + +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const deleteLabelFallbackSchema = z.object({ params: z.object({ Project_id: zz.objectId(), label_id: zz.objectId(), @@ -531,7 +641,9 @@ const deleteLabelSchema = z.object({ * @param {any} next */ async function deleteLabel(req, res, next) { - const { params } = parseReq(req, deleteLabelSchema) + const { params } = parseReq(req, deleteLabelSchema, { + fallbackSchema: deleteLabelFallbackSchema, + }) const { Project_id: projectId, label_id: labelId } = params const userId = SessionManager.getLoggedInUserId(req.session) @@ -553,7 +665,7 @@ async function deleteLabel(req, res, next) { } const downloadZipOfVersionSchema = z.object({ - params: z.object({ + params: z.strictObject({ project_id: zz.objectId(), version: z.coerce.number().int().min(0), }), @@ -719,7 +831,7 @@ async function _pipeHistoryZipToResponse(v1ProjectId, version, name, req, res) { } const getLatestHistorySchema = z.object({ - params: z.object({ + params: z.strictObject({ project_id: zz.objectId(), }), }) @@ -737,7 +849,7 @@ async function getLatestHistory(req, res, next) { } const getChangesSchema = z.object({ - params: z.object({ + params: z.strictObject({ project_id: zz.objectId(), }), query: z.object({ diff --git a/services/web/app/src/Features/Metadata/MetaController.mjs b/services/web/app/src/Features/Metadata/MetaController.mjs index 8bb1646107..4fe4de2b77 100644 --- a/services/web/app/src/Features/Metadata/MetaController.mjs +++ b/services/web/app/src/Features/Metadata/MetaController.mjs @@ -5,9 +5,17 @@ import logger from '@overleaf/logger' import { expressify } from '@overleaf/promise-utils' import Analytics from '../Analytics/AnalyticsManager.mjs' import SplitTestHandler from '../SplitTests/SplitTestHandler.mjs' +import { z, zz, parseReq } from '../../infrastructure/Validation.mjs' + +const getMetadataSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + }), +}) async function getMetadata(req, res) { - const { project_id: projectId } = req.params + const { params } = parseReq(req, getMetadataSchema, { logOnly: true }) + const { project_id: projectId } = params logger.debug({ projectId }, 'getting all labels for project') @@ -55,10 +63,22 @@ function sendAnalyticsEventForPackageUsage(projectId, projectMeta) { Analytics.emitPackageUsage(projectId, { documentClasses, packages }) } +const broadcastMetadataForDocSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + doc_id: zz.objectId(), + }), + body: z.strictObject({ + broadcast: z.boolean().optional(), + }), +}) + async function broadcastMetadataForDoc(req, res) { - const { project_id: projectId } = req.params - const { doc_id: docId } = req.params - const { broadcast } = req.body + const { params, body } = parseReq(req, broadcastMetadataForDocSchema, { + logOnly: true, + }) + const { project_id: projectId, doc_id: docId } = params + const { broadcast } = body logger.debug({ projectId, docId, broadcast }, 'getting labels for doc') diff --git a/services/web/app/src/Features/Notifications/NotificationsController.mjs b/services/web/app/src/Features/Notifications/NotificationsController.mjs index 015d66580d..555d3a3e39 100644 --- a/services/web/app/src/Features/Notifications/NotificationsController.mjs +++ b/services/web/app/src/Features/Notifications/NotificationsController.mjs @@ -1,6 +1,13 @@ import NotificationsHandler from './NotificationsHandler.mjs' import SessionManager from '../Authentication/SessionManager.mjs' import _ from 'lodash' +import { z, zz, parseReq } from '../../infrastructure/Validation.mjs' + +const notificationIdParamsSchema = z.object({ + params: z.strictObject({ + notificationId: zz.objectId(), + }), +}) export default { getAllUnreadNotifications(req, res, next) { @@ -28,7 +35,10 @@ export default { markNotificationAsRead(req, res) { const userId = SessionManager.getLoggedInUserId(req.session) - const { notificationId } = req.params + const { params } = parseReq(req, notificationIdParamsSchema, { + logOnly: true, + }) + const { notificationId } = params NotificationsHandler.markAsRead(userId, notificationId, () => res.sendStatus(200) ) @@ -36,7 +46,10 @@ export default { getNotification(req, res, next) { const userId = SessionManager.getLoggedInUserId(req.session) - const { notificationId } = req.params + const { params } = parseReq(req, notificationIdParamsSchema, { + logOnly: true, + }) + const { notificationId } = params NotificationsHandler.getUserNotifications( userId, function (err, unreadNotifications) { diff --git a/services/web/app/src/Features/Project/ProjectApiController.mjs b/services/web/app/src/Features/Project/ProjectApiController.mjs index 564eaa8a2e..d2043c7e6c 100644 --- a/services/web/app/src/Features/Project/ProjectApiController.mjs +++ b/services/web/app/src/Features/Project/ProjectApiController.mjs @@ -1,22 +1,18 @@ -/* eslint-disable - max-len, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ import ProjectDetailsHandler from './ProjectDetailsHandler.mjs' +import { parseReq, z, zz } from '../../infrastructure/Validation.mjs' -import logger from '@overleaf/logger' +const getProjectDetailsSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + }), +}) export default { getProjectDetails(req, res, next) { - const { project_id: projectId } = req.params + const { params } = parseReq(req, getProjectDetailsSchema, { + logOnly: true, + }) + const { project_id: projectId } = params return ProjectDetailsHandler.getDetails( projectId, function (err, projDetails) { diff --git a/services/web/app/src/Features/Project/ProjectController.mjs b/services/web/app/src/Features/Project/ProjectController.mjs index 0112a14240..6557ecc0fd 100644 --- a/services/web/app/src/Features/Project/ProjectController.mjs +++ b/services/web/app/src/Features/Project/ProjectController.mjs @@ -67,6 +67,22 @@ const { ObjectId } = mongodb */ const updateProjectAdminSettingsSchema = z.object({ + params: z.strictObject({ + Project_id: zz.coercedObjectId(ObjectId), + }), + body: z.strictObject({ + publicAccessLevel: z + .enum( + [PublicAccessLevels.PRIVATE, PublicAccessLevels.TOKEN_BASED], + 'unexpected access level' + ) + .optional(), + }), +}) + +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const updateProjectAdminSettingsFallbackSchema = z.object({ params: z.object({ Project_id: zz.coercedObjectId(ObjectId), }), @@ -81,6 +97,24 @@ const updateProjectAdminSettingsSchema = z.object({ }) const updateProjectSettingsSchema = z.object({ + params: z.strictObject({ + Project_id: zz.coercedObjectId(), + }), + body: z.strictObject({ + compiler: z.string().optional(), + imageName: z.string().optional(), + png2pdf: z.boolean().optional(), + mainBibliographyDocId: zz.objectId().optional(), + name: z.string().optional(), + rootDocId: zz.objectId().optional(), + spellCheckLanguage: z.string().optional(), + referenceFormat: z.enum(['bibtex', 'biblatex']).optional(), + }), +}) + +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const updateProjectSettingsFallbackSchema = z.object({ params: z.object({ Project_id: zz.coercedObjectId(), }), @@ -96,6 +130,64 @@ const updateProjectSettingsSchema = z.object({ }), }) +const projectIdParamSchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + }), +}) + +const lowerCaseProjectIdParamSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + }), +}) + +const expireDeletedProjectSchema = z.object({ + params: z.strictObject({ + projectId: zz.objectId(), + }), +}) + +const cloneProjectSchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + }), + body: z.strictObject({ + projectName: z.string().optional(), + isDebugCopy: z.boolean().optional(), + cloneHistory: z.boolean().optional(), + cloneRanges: z.boolean().optional(), + tags: z.array(z.strictObject({ id: zz.objectId() })).optional(), + }), +}) + +const newProjectSchema = z.object({ + body: z.strictObject({ + projectName: z.string().optional(), + template: z.string().optional(), + }), +}) + +const renameProjectSchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + }), + body: z.strictObject({ + newProjectName: z.string(), + }), +}) + +const loadEditorSchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + detachRole: z.enum(['detacher', 'detached']).optional(), + }), + query: z.object({ + debug_pdf_detach: z.string().optional(), + ws: z.string().optional(), + }), +}) + const _ProjectController = { _isInPercentageRollout(rolloutName, objectId, percentage) { if (Settings.bypassPercentageRollouts === true) { @@ -108,7 +200,9 @@ const _ProjectController = { }, async updateProjectSettings(req, res) { - const { params, body } = parseReq(req, updateProjectSettingsSchema) + const { params, body } = parseReq(req, updateProjectSettingsSchema, { + fallbackSchema: updateProjectSettingsFallbackSchema, + }) const projectId = params.Project_id if (body.compiler != null) { @@ -156,7 +250,9 @@ const _ProjectController = { }, async updateProjectAdminSettings(req, res) { - const { params, body } = parseReq(req, updateProjectAdminSettingsSchema) + const { params, body } = parseReq(req, updateProjectAdminSettingsSchema, { + fallbackSchema: updateProjectAdminSettingsFallbackSchema, + }) const projectId = params.Project_id const user = SessionManager.getSessionUser(req.session) if (!Features.hasFeature('link-sharing')) { @@ -183,7 +279,10 @@ const _ProjectController = { }, async deleteProject(req, res) { - const projectId = req.params.Project_id + const { params } = parseReq(req, projectIdParamSchema, { + logOnly: true, + }) + const projectId = params.Project_id const user = SessionManager.getSessionUser(req.session) await ProjectDeleter.promises.deleteProject(projectId, { deleterUser: user, @@ -200,7 +299,10 @@ const _ProjectController = { }, async archiveProject(req, res) { - const projectId = req.params.Project_id + const { params } = parseReq(req, projectIdParamSchema, { + logOnly: true, + }) + const projectId = params.Project_id const userId = SessionManager.getLoggedInUserId(req.session) await ProjectDeleter.promises.archiveProject(projectId, userId) ProjectAuditLogHandler.addEntryIfManagedInBackground( @@ -213,7 +315,10 @@ const _ProjectController = { }, async unarchiveProject(req, res) { - const projectId = req.params.Project_id + const { params } = parseReq(req, projectIdParamSchema, { + logOnly: true, + }) + const projectId = params.Project_id const userId = SessionManager.getLoggedInUserId(req.session) await ProjectDeleter.promises.unarchiveProject(projectId, userId) ProjectAuditLogHandler.addEntryIfManagedInBackground( @@ -226,7 +331,10 @@ const _ProjectController = { }, async trashProject(req, res) { - const projectId = req.params.project_id + const { params } = parseReq(req, lowerCaseProjectIdParamSchema, { + logOnly: true, + }) + const projectId = params.project_id const userId = SessionManager.getLoggedInUserId(req.session) await ProjectDeleter.promises.trashProject(projectId, userId) ProjectAuditLogHandler.addEntryIfManagedInBackground( @@ -239,7 +347,10 @@ const _ProjectController = { }, async untrashProject(req, res) { - const projectId = req.params.project_id + const { params } = parseReq(req, lowerCaseProjectIdParamSchema, { + logOnly: true, + }) + const projectId = params.project_id const userId = SessionManager.getLoggedInUserId(req.session) await ProjectDeleter.promises.untrashProject(projectId, userId) ProjectAuditLogHandler.addEntryIfManagedInBackground( @@ -257,14 +368,19 @@ const _ProjectController = { }, async expireDeletedProject(req, res) { - const { projectId } = req.params + const { + params: { projectId }, + } = parseReq(req, expireDeletedProjectSchema, { logOnly: true }) await ProjectDeleter.promises.expireDeletedProject(projectId) res.sendStatus(200) }, async restoreProject(req, res) { const user = SessionManager.getLoggedInUserId(req.session) - const projectId = req.params.Project_id + const { params } = parseReq(req, projectIdParamSchema, { + logOnly: true, + }) + const projectId = params.Project_id await ProjectDeleter.promises.restoreProject(projectId) ProjectAuditLogHandler.addEntryIfManagedInBackground( projectId, @@ -278,8 +394,11 @@ const _ProjectController = { async cloneProject(req, res, next) { res.setTimeout(5 * 60 * 1000) // allow extra time for the copy to complete metrics.inc('cloned-project') - const projectId = req.params.Project_id - let { projectName, isDebugCopy, cloneHistory, cloneRanges, tags } = req.body + const { params, body } = parseReq(req, cloneProjectSchema, { + logOnly: true, + }) + const projectId = params.Project_id + let { projectName, isDebugCopy, cloneHistory, cloneRanges, tags } = body const currentUser = SessionManager.getSessionUser(req.session) if (!hasAdminAccess(currentUser)) { isDebugCopy = false @@ -345,9 +464,10 @@ const _ProjectController = { email, _id: userId, } = currentUser + const { body } = parseReq(req, newProjectSchema, { logOnly: true }) const projectName = - req.body.projectName != null ? req.body.projectName.trim() : undefined - const { template } = req.body + body.projectName != null ? body.projectName.trim() : undefined + const { template } = body const project = await (template === 'example' ? ProjectCreationHandler.promises.createExampleProject( @@ -376,8 +496,11 @@ const _ProjectController = { }, async renameProject(req, res) { - const projectId = req.params.Project_id - const newName = req.body.newProjectName + const { params, body } = parseReq(req, renameProjectSchema, { + logOnly: true, + }) + const projectId = params.Project_id + const newName = body.newProjectName await EditorController.promises.renameProject(projectId, newName) res.sendStatus(200) }, @@ -398,7 +521,10 @@ const _ProjectController = { }, async projectEntitiesJson(req, res) { - const projectId = req.params.Project_id + const { params } = parseReq(req, projectIdParamSchema, { + logOnly: true, + }) + const projectId = params.Project_id const project = await ProjectGetter.promises.getProject(projectId) const { docs, files } = @@ -416,6 +542,9 @@ const _ProjectController = { async loadEditor(req, res, next) { const timer = new metrics.Timer('load-editor') + const { params, query } = parseReq(req, loadEditorSchema, { + logOnly: true, + }) if (!Settings.editorIsOpen) { return res.render('general/closed', { title: 'updating_site' }) } @@ -461,12 +590,12 @@ const _ProjectController = { } } - const projectId = req.params.Project_id + const projectId = params.Project_id // should not be used in place of split tests query param overrides (?my-split-test-name=my-variant) function shouldDisplayFeature(name, variantFlag) { - if (req.query && req.query[name]) { - return req.query[name] === 'true' + if (query[name]) { + return query[name] === 'true' } else { return variantFlag === true } @@ -727,7 +856,7 @@ const _ProjectController = { wsUrl = Settings.wsUrlV2 metricName += '-v2' } - if (req.query && req.query.ws === 'fallback') { + if (query.ws === 'fallback') { // `?ws=fallback` will connect to the bare origin, and ignore // the custom wsUrl. Hence it must load the client side // javascript from there too. @@ -807,7 +936,7 @@ const _ProjectController = { const debugPdfDetach = shouldDisplayFeature('debug_pdf_detach') - const detachRole = req.params.detachRole + const detachRole = params.detachRole const showSymbolPalette = !Features.hasFeature('saas') || diff --git a/services/web/app/src/Features/Project/ProjectListController.mjs b/services/web/app/src/Features/Project/ProjectListController.mjs index cb9f6949da..7ba9c4dffb 100644 --- a/services/web/app/src/Features/Project/ProjectListController.mjs +++ b/services/web/app/src/Features/Project/ProjectListController.mjs @@ -33,6 +33,36 @@ import PermissionsManager from '../Authorization/PermissionsManager.mjs' import AnalyticsManager from '../Analytics/AnalyticsManager.mjs' import { OnboardingDataCollection } from '../../models/OnboardingDataCollection.mjs' import UserSettingsHelper from './UserSettingsHelper.mjs' +import { parseReq, z, zz } from '../../infrastructure/Validation.mjs' + +const getProjectsJsonSchema = z.object({ + body: z.strictObject({ + filters: z + .strictObject({ + ownedByUser: z.boolean().optional(), + sharedWithUser: z.boolean().optional(), + archived: z.boolean().optional(), + trashed: z.boolean().optional(), + // null is a distinct, meaningful value (see _hasActiveFilter) -- + // not the same as the field being absent + tag: z.string().nullish(), + search: z.string().optional(), + }) + .optional(), + sort: z + .strictObject({ + by: z.enum(['lastUpdated', 'title', 'owner']).optional(), + order: z.enum(['asc', 'desc']).optional(), + }) + .optional(), + page: z + .strictObject({ + size: z.number().int().positive().optional(), + lastId: zz.objectId().optional(), + }) + .optional(), + }), +}) /** * @import { GetProjectsRequest, GetProjectsResponse, AllUsersProjects, MongoProject, FormattedProject, MongoTag, SubscriptionRecord } from "./types" @@ -669,7 +699,8 @@ async function projectListPage(req, res, next) { * @returns {Promise} */ async function getProjectsJson(req, res) { - const { filters, page, sort } = req.body + const { body } = parseReq(req, getProjectsJsonSchema, { logOnly: true }) + const { filters, page, sort } = body const userId = SessionManager.getLoggedInUserId(req.session) const projectsPage = await _getProjects(userId, filters, sort, page) res.json(projectsPage) @@ -694,8 +725,8 @@ async function _checkForOldDebugProjects(userId) { /** * @param {string} userId * @param {Filters} filters - * @param {Sort} sort - * @param {Page} page + * @param {Partial} sort + * @param {Partial} page * @returns {Promise<{totalSize: number, projects: Project[]}>} * @private */ @@ -807,8 +838,8 @@ function _applyFilters(projects, tags, filters, userId) { /** * @param {FormattedProject[]} projects - * @param {Sort} sort - * @param {Page} page + * @param {Partial} sort + * @param {Partial} page * @returns {FormattedProject[]} * @private */ diff --git a/services/web/app/src/Features/Project/types.d.ts b/services/web/app/src/Features/Project/types.d.ts index 6c99e042d8..404b1601d6 100644 --- a/services/web/app/src/Features/Project/types.d.ts +++ b/services/web/app/src/Features/Project/types.d.ts @@ -1,4 +1,5 @@ import express from 'express' +import { ParamsDictionary, Query } from 'express-serve-static-core' import { GetProjectsRequestBody, GetProjectsResponseBody, @@ -10,10 +11,10 @@ import { ObjectId } from 'mongodb-legacy' import { Source } from '../Authorization/types' export type GetProjectsRequest = express.Request< - unknown, - unknown, + ParamsDictionary, + GetProjectsResponseBody, GetProjectsRequestBody, - unknown + Query > export type GetProjectsResponse = express.Response diff --git a/services/web/app/src/Features/Spelling/SpellingController.mjs b/services/web/app/src/Features/Spelling/SpellingController.mjs index fecb3193e6..d55fb79edd 100644 --- a/services/web/app/src/Features/Spelling/SpellingController.mjs +++ b/services/web/app/src/Features/Spelling/SpellingController.mjs @@ -5,12 +5,26 @@ import LearnedWordsManager from './LearnedWordsManager.mjs' import { z, parseReq } from '../../infrastructure/Validation.mjs' const learnSchema = z.object({ + body: z.strictObject({ + word: z.string().min(1), + }), +}) +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const learnFallbackSchema = z.object({ body: z.object({ word: z.string().min(1), }), }) const unlearnSchema = z.object({ + body: z.strictObject({ + word: z.string().min(1), + }), +}) +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const unlearnFallbackSchema = z.object({ body: z.object({ word: z.string().min(1), }), @@ -23,7 +37,9 @@ export default { * @param {any} next */ learn(req, res, next) { - const { body } = parseReq(req, learnSchema) + const { body } = parseReq(req, learnSchema, { + fallbackSchema: learnFallbackSchema, + }) const { word } = body const userId = SessionManager.getLoggedInUserId(req.session) LearnedWordsManager.learnWord( @@ -42,7 +58,9 @@ export default { * @param {any} next */ unlearn(req, res, next) { - const { body } = parseReq(req, unlearnSchema) + const { body } = parseReq(req, unlearnSchema, { + fallbackSchema: unlearnFallbackSchema, + }) const { word } = body const userId = SessionManager.getLoggedInUserId(req.session) LearnedWordsManager.unlearnWord( diff --git a/services/web/app/src/Features/Tags/TagsController.mjs b/services/web/app/src/Features/Tags/TagsController.mjs index 9c6ac6b368..d605a2c9ff 100644 --- a/services/web/app/src/Features/Tags/TagsController.mjs +++ b/services/web/app/src/Features/Tags/TagsController.mjs @@ -1,9 +1,12 @@ import TagsHandler from './TagsHandler.mjs' import SessionManager from '../Authentication/SessionManager.mjs' import Errors from '../Errors/Errors.js' -import { z, parseReq } from '../../infrastructure/Validation.mjs' +import { z, zz, parseReq } from '../../infrastructure/Validation.mjs' import { expressify } from '@overleaf/promise-utils' +// mirrors the Tag model's own color validator (models/Tag.mjs) +const COLOR_REGEX = /^#[a-fA-F0-9]{6}$/ + async function _getTags(userId, _req, res) { if (!userId) { throw new Errors.NotFoundError() @@ -12,9 +15,15 @@ async function _getTags(userId, _req, res) { res.json(allTags) } +const apiGetAllTagsSchema = z.object({ + params: z.strictObject({ + userId: zz.objectId(), + }), +}) + async function apiGetAllTags(req, res) { - const { userId } = req.params - await _getTags(userId, req, res) + const { params } = parseReq(req, apiGetAllTagsSchema, { logOnly: true }) + await _getTags(params.userId, req, res) } async function getAllTags(req, res) { @@ -23,6 +32,14 @@ async function getAllTags(req, res) { } const createTagSchema = z.object({ + body: z.strictObject({ + name: z.string().min(1), + color: z.string().regex(COLOR_REGEX).optional(), + }), +}) +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const createTagFallbackSchema = z.object({ body: z.object({ name: z.string(), color: z.string().optional(), @@ -30,21 +47,41 @@ const createTagSchema = z.object({ }) async function createTag(req, res) { - const { body } = parseReq(req, createTagSchema) + const { body } = parseReq(req, createTagSchema, { + fallbackSchema: createTagFallbackSchema, + }) const { name, color } = body const userId = SessionManager.getLoggedInUserId(req.session) const tag = await TagsHandler.promises.createTag(userId, name, color) res.json(tag) } +const addProjectToTagSchema = z.object({ + params: z.strictObject({ + tagId: zz.objectId(), + projectId: zz.objectId(), + }), +}) + async function addProjectToTag(req, res) { const userId = SessionManager.getLoggedInUserId(req.session) - const { tagId, projectId } = req.params + const { params } = parseReq(req, addProjectToTagSchema, { logOnly: true }) + const { tagId, projectId } = params await TagsHandler.promises.addProjectToTag(userId, tagId, projectId) res.status(204).end() } const addProjectsToTagSchema = z.object({ + params: z.strictObject({ + tagId: zz.objectId(), + }), + body: z.strictObject({ + projectIds: z.array(zz.objectId()), + }), +}) +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const addProjectsToTagFallbackSchema = z.object({ params: z.object({ tagId: z.string(), }), @@ -54,7 +91,9 @@ const addProjectsToTagSchema = z.object({ }) async function addProjectsToTag(req, res) { - const { params, body } = parseReq(req, addProjectsToTagSchema) + const { params, body } = parseReq(req, addProjectsToTagSchema, { + fallbackSchema: addProjectsToTagFallbackSchema, + }) const { tagId } = params const { projectIds } = body const userId = SessionManager.getLoggedInUserId(req.session) @@ -62,14 +101,34 @@ async function addProjectsToTag(req, res) { res.status(204).end() } +const removeProjectFromTagSchema = z.object({ + params: z.strictObject({ + tagId: zz.objectId(), + projectId: zz.objectId(), + }), +}) + async function removeProjectFromTag(req, res, next) { const userId = SessionManager.getLoggedInUserId(req.session) - const { tagId, projectId } = req.params + const { params } = parseReq(req, removeProjectFromTagSchema, { + logOnly: true, + }) + const { tagId, projectId } = params await TagsHandler.promises.removeProjectFromTag(userId, tagId, projectId) res.status(204).end() } const removeProjectsFromTagSchema = z.object({ + params: z.strictObject({ + tagId: zz.objectId(), + }), + body: z.strictObject({ + projectIds: z.array(zz.objectId()), + }), +}) +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const removeProjectsFromTagFallbackSchema = z.object({ params: z.object({ tagId: z.string(), }), @@ -79,7 +138,9 @@ const removeProjectsFromTagSchema = z.object({ }) async function removeProjectsFromTag(req, res, next) { - const { params, body } = parseReq(req, removeProjectsFromTagSchema) + const { params, body } = parseReq(req, removeProjectsFromTagSchema, { + fallbackSchema: removeProjectsFromTagFallbackSchema, + }) const { tagId } = params const { projectIds } = body const userId = SessionManager.getLoggedInUserId(req.session) @@ -87,14 +148,31 @@ async function removeProjectsFromTag(req, res, next) { res.status(204).end() } +const deleteTagSchema = z.object({ + params: z.strictObject({ + tagId: zz.objectId(), + }), +}) + async function deleteTag(req, res) { const userId = SessionManager.getLoggedInUserId(req.session) - const { tagId } = req.params + const { params } = parseReq(req, deleteTagSchema, { logOnly: true }) + const { tagId } = params await TagsHandler.promises.deleteTag(userId, tagId) res.status(204).end() } const renameTagSchema = z.object({ + params: z.strictObject({ + tagId: zz.objectId(), + }), + body: z.strictObject({ + name: z.string().min(1), + }), +}) +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const renameTagFallbackSchema = z.object({ params: z.object({ tagId: z.string(), }), @@ -104,10 +182,12 @@ const renameTagSchema = z.object({ }) async function renameTag(req, res) { - const { params, body } = parseReq(req, renameTagSchema) + const { params, body } = parseReq(req, renameTagSchema, { + fallbackSchema: renameTagFallbackSchema, + }) const userId = SessionManager.getLoggedInUserId(req.session) const { tagId } = params - const name = body.name + const { name } = body if (!name) { return res.status(400).end() } @@ -116,6 +196,17 @@ async function renameTag(req, res) { } const editTagSchema = z.object({ + params: z.strictObject({ + tagId: zz.objectId(), + }), + body: z.strictObject({ + name: z.string().min(1), + color: z.string().regex(COLOR_REGEX).optional(), + }), +}) +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const editTagFallbackSchema = z.object({ params: z.object({ tagId: z.string(), }), @@ -126,10 +217,11 @@ const editTagSchema = z.object({ }) async function editTag(req, res) { - const { params, body } = parseReq(req, editTagSchema) + const { params, body } = parseReq(req, editTagSchema, { + fallbackSchema: editTagFallbackSchema, + }) const { tagId } = params - const name = body.name - const color = body.color + const { name, color } = body const userId = SessionManager.getLoggedInUserId(req.session) if (!name) { return res.status(400).end() diff --git a/services/web/app/src/Features/Uploads/ProjectUploadController.mjs b/services/web/app/src/Features/Uploads/ProjectUploadController.mjs index 6f7c91a72d..2b74f12635 100644 --- a/services/web/app/src/Features/Uploads/ProjectUploadController.mjs +++ b/services/web/app/src/Features/Uploads/ProjectUploadController.mjs @@ -22,9 +22,56 @@ import { import DocumentConversionManager from './DocumentConversionManager.mjs' import ProjectOptionsHandler from '../Project/ProjectOptionsHandler.mjs' import AnalyticsManager from '../Analytics/AnalyticsManager.mjs' +import { parseReq, z, zz } from '../../infrastructure/Validation.mjs' const defaultsDeep = lodash.defaultsDeep +const uploadMetaTypeSchema = z.string().optional() + +const uploadProjectSchema = z.object({ + body: z.strictObject({ + name: z.string().nonempty(), + type: uploadMetaTypeSchema, + relativePath: zz.filepath().optional(), + }), + file: zz.uploadedFile(), +}) + +const uploadFileSchema = z.object({ + params: z.strictObject({ + Project_id: zz.objectId(), + }), + query: z.object({ + folder_id: zz.objectId().optional(), + }), + body: z.strictObject({ + // validated for presence/length by hand below (not schema-enforced), + // so an invalid name keeps returning the existing friendly + // {success:false, error:'invalid_filename'} response instead of a + // generic validation error + name: z.string().optional(), + relativePath: zz.filepath().optional(), + type: uploadMetaTypeSchema, + targetFolderId: zz.objectId().optional(), + }), + file: zz.uploadedFile(), +}) + +const importDocumentSchema = z.object({ + query: z.object({ + // validated for the ['docx','markdown'] allowlist by hand below, so an + // unsupported value keeps returning the existing friendly + // {success:false, error:'invalid_import_type'} response + type: z.string().optional(), + }), + body: z.strictObject({ + name: z.string().nonempty(), + relativePath: zz.filepath().optional(), + type: uploadMetaTypeSchema, + }), + file: zz.uploadedFile(), +}) + const upload = multer( defaultsDeep( { @@ -45,8 +92,11 @@ const upload = multer( function uploadProject(req, res, next) { const timer = new metrics.Timer('project-upload') const userId = SessionManager.getLoggedInUserId(req.session) - const { path } = req.file - const name = Path.basename(req.body.name, '.zip') + const { body, file } = parseReq(req, uploadProjectSchema, { + logOnly: true, + }) + const { path } = file + const name = Path.basename(body.name, '.zip') return ProjectUploadManager.createProjectFromZipArchive( userId, name, @@ -84,11 +134,14 @@ function uploadProject(req, res, next) { */ async function uploadFile(req, res, next) { const timer = new metrics.Timer('file-upload') - const name = req.body.name - const { path } = req.file - const projectId = req.params.Project_id + const { params, query, body, file } = parseReq(req, uploadFileSchema, { + logOnly: true, + }) + const name = body.name + const { path } = file + const projectId = params.Project_id const userId = SessionManager.getLoggedInUserId(req.session) - let { folder_id: folderId } = req.query + let folderId = query.folder_id if (name == null || name.length === 0 || name.length > 150) { await fsPromises.unlink(path).catch(unlinkErr => { logger.warn({ err: unlinkErr, path }, 'error unlinking uploaded file') @@ -101,7 +154,7 @@ async function uploadFile(req, res, next) { try { // preserve the directory structure from an uploaded folder - const { relativePath } = req.body + const { relativePath } = body // NOTE: Uppy sends a "null" string for `relativePath` when the file is not nested in a folder if (relativePath && relativePath !== 'null') { const { path } = await ProjectLocator.promises.findElement({ @@ -186,15 +239,18 @@ async function uploadFile(req, res, next) { */ async function importDocument(req, res, next) { const userId = SessionManager.getLoggedInUserId(req.session) - const { path } = req.file - const conversionType = req.query.type + const { query, body, file } = parseReq(req, importDocumentSchema, { + logOnly: true, + }) + const { path } = file + const conversionType = query.type if (!['docx', 'markdown'].includes(conversionType)) { return res.status(400).json({ success: false, error: req.i18n.translate('invalid_import_type'), }) } - const name = Path.basename(req.body.name, Path.extname(req.body.name)) + const name = Path.basename(body.name, Path.extname(body.name)) logger.debug({ path, userId, conversionType }, 'importing document file') try { const archivePath = diff --git a/services/web/app/src/Features/Uploads/UploadsRouter.mjs b/services/web/app/src/Features/Uploads/UploadsRouter.mjs index 070cc90801..ce03ea3040 100644 --- a/services/web/app/src/Features/Uploads/UploadsRouter.mjs +++ b/services/web/app/src/Features/Uploads/UploadsRouter.mjs @@ -5,6 +5,7 @@ import { RateLimiter } from '../../infrastructure/RateLimiter.mjs' import RateLimiterMiddleware from '../Security/RateLimiterMiddleware.mjs' import Settings from '@overleaf/settings' import AsyncLocalStorage from '../../infrastructure/AsyncLocalStorage.mjs' +import { getRawReqInput } from '../../infrastructure/Validation.mjs' const rateLimiters = { projectUpload: new RateLimiter('project-upload', { @@ -42,7 +43,7 @@ export default { RateLimiterMiddleware.rateLimit(rateLimiters.projectUpload), ProjectUploadController.multerMiddleware, (req, res, next) => { - req.query.type = 'docx' + getRawReqInput(req).query.type = 'docx' next() }, ProjectUploadController.importDocument diff --git a/services/web/modules/history-v1/test/acceptance/src/ProjectStructureTests.mjs b/services/web/modules/history-v1/test/acceptance/src/ProjectStructureTests.mjs index 2d54bafad4..bc5d4fcd9b 100644 --- a/services/web/modules/history-v1/test/acceptance/src/ProjectStructureTests.mjs +++ b/services/web/modules/history-v1/test/acceptance/src/ProjectStructureTests.mjs @@ -677,7 +677,7 @@ describe('ProjectStructureChanges', function () { renameItem( owner, exampleProjectId, - 'Doc', + 'doc', exampleDocId, 'wombat.tex', () => { diff --git a/services/web/test/acceptance/src/ChatTests.mjs b/services/web/test/acceptance/src/ChatTests.mjs new file mode 100644 index 0000000000..91efb04e55 --- /dev/null +++ b/services/web/test/acceptance/src/ChatTests.mjs @@ -0,0 +1,177 @@ +import { expect } from 'chai' +import UserHelper from './helpers/User.mjs' +import { expectValidationErrorRaw } from '@overleaf/validation-tools/testUtils.js' + +const User = UserHelper.promises + +describe('Chat', function () { + let owner, projectId + + beforeEach(async function () { + owner = new User() + await owner.login() + projectId = await owner.createProject('chat-test', { template: 'blank' }) + }) + + describe('GET /project/:project_id/messages', function () { + it('should return an empty list of messages initially', async function () { + const { response, body } = await owner.doRequest('get', { + url: `/project/${projectId}/messages`, + json: true, + }) + expect(response.statusCode).to.equal(200) + expect(body).to.deep.equal([]) + }) + + it('should reject a malformed project id with 404', async function () { + const { response, body } = await owner.doRequest('get', { + url: `/project/not-an-object-id/messages`, + json: true, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 404, + 'project_id' + ) + }) + }) + + describe('POST /project/:project_id/messages', function () { + it('should send a message and return it in later gets', async function () { + const sendResult = await owner.doRequest('post', { + url: `/project/${projectId}/messages`, + json: { content: 'hello world' }, + }) + expect(sendResult.response.statusCode).to.equal(204) + + const { response, body } = await owner.doRequest('get', { + url: `/project/${projectId}/messages`, + json: true, + }) + expect(response.statusCode).to.equal(200) + expect(body).to.have.length(1) + expect(body[0].content).to.equal('hello world') + expect(body[0].user.id).to.equal(owner._id.toString()) + }) + + it('should reject a missing content field', async function () { + const { response, body } = await owner.doRequest('post', { + url: `/project/${projectId}/messages`, + json: {}, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 400, + 'content' + ) + }) + + it('should accept numeric limit and before query params', async function () { + // MockChatApi does not implement limit/before filtering itself; this + // only exercises that the coerced numeric values are accepted and + // forwarded rather than rejected by validation. + const { response, body } = await owner.doRequest('get', { + url: `/project/${projectId}/messages?limit=2&before=99999999999`, + json: true, + }) + expect(response.statusCode).to.equal(200) + expect(body).to.deep.equal([]) + }) + + it('should reject a non-numeric limit', async function () { + const { response, body } = await owner.doRequest('get', { + url: `/project/${projectId}/messages?limit=not-a-number`, + json: true, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 400, + 'limit' + ) + }) + }) + + describe('POST /project/:project_id/messages/:message_id/edit', function () { + let messageId + + beforeEach(async function () { + await owner.doRequest('post', { + url: `/project/${projectId}/messages`, + json: { content: 'original content' }, + }) + const { body } = await owner.doRequest('get', { + url: `/project/${projectId}/messages`, + json: true, + }) + messageId = body[0].id + }) + + it('should edit the message content', async function () { + const editResult = await owner.doRequest('post', { + url: `/project/${projectId}/messages/${messageId}/edit`, + json: { content: 'edited content' }, + }) + expect(editResult.response.statusCode).to.equal(204) + + const { body } = await owner.doRequest('get', { + url: `/project/${projectId}/messages`, + json: true, + }) + expect(body[0].content).to.equal('edited content') + }) + + it('should reject a malformed message id with 404', async function () { + const { response, body } = await owner.doRequest('post', { + url: `/project/${projectId}/messages/not-an-object-id/edit`, + json: { content: 'edited content' }, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 404, + 'message_id' + ) + }) + }) + + describe('DELETE /project/:project_id/messages/:message_id', function () { + let messageId + + beforeEach(async function () { + await owner.doRequest('post', { + url: `/project/${projectId}/messages`, + json: { content: 'to be deleted' }, + }) + const { body } = await owner.doRequest('get', { + url: `/project/${projectId}/messages`, + json: true, + }) + messageId = body[0].id + }) + + it('should delete the message', async function () { + const deleteResult = await owner.doRequest('delete', { + url: `/project/${projectId}/messages/${messageId}`, + json: true, + }) + expect(deleteResult.response.statusCode).to.equal(204) + + const { body } = await owner.doRequest('get', { + url: `/project/${projectId}/messages`, + json: true, + }) + expect(body).to.deep.equal([]) + }) + + it('should reject a malformed message id with 404', async function () { + const { response, body } = await owner.doRequest('delete', { + url: `/project/${projectId}/messages/not-an-object-id`, + json: true, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 404, + 'message_id' + ) + }) + }) +}) diff --git a/services/web/test/acceptance/src/DocUpdateTests.mjs b/services/web/test/acceptance/src/DocUpdateTests.mjs index 7f11b855f5..1fd55cb1b5 100644 --- a/services/web/test/acceptance/src/DocUpdateTests.mjs +++ b/services/web/test/acceptance/src/DocUpdateTests.mjs @@ -86,7 +86,7 @@ describe('DocUpdate', function () { lines: ['a'], version: 1, ranges: {}, - lastUpdatedAt: new Date(), + lastUpdatedAt: String(Date.now()), lastUpdatedBy: this.user.id, } }) diff --git a/services/web/test/acceptance/src/EditorHttpControllerTests.mjs b/services/web/test/acceptance/src/EditorHttpControllerTests.mjs index 6dfdcf0858..606829de97 100644 --- a/services/web/test/acceptance/src/EditorHttpControllerTests.mjs +++ b/services/web/test/acceptance/src/EditorHttpControllerTests.mjs @@ -1,5 +1,9 @@ import User from './helpers/User.mjs' import { expect } from 'chai' +import settings from '@overleaf/settings' +import { expectValidationErrorRaw } from '@overleaf/validation-tools/testUtils.js' + +const UserPromises = User.promises describe('EditorHttpController', function () { beforeEach('login', function (done) { @@ -25,3 +29,153 @@ describe('EditorHttpController', function () { }) }) }) + +describe('EditorHttpController validation', function () { + let owner, projectId, rootFolderId + + beforeEach(async function () { + owner = new UserPromises() + await owner.login() + projectId = await owner.createProject('editor-http-controller-test', { + template: 'blank', + }) + const project = await owner.getProject(projectId) + rootFolderId = project.rootFolder[0]._id.toString() + }) + + describe('POST /project/:Project_id/doc', function () { + it('should add a doc', async function () { + const { response, body } = await owner.doRequest('post', { + url: `/project/${projectId}/doc`, + json: { name: 'another.tex', parent_folder_id: rootFolderId }, + }) + expect(response.statusCode).to.equal(200) + expect(body.name).to.equal('another.tex') + }) + + it('should reject a malformed project id with 404', async function () { + const { response, body } = await owner.doRequest('post', { + url: '/project/not-an-object-id/doc', + json: { name: 'another.tex', parent_folder_id: rootFolderId }, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 404, + 'Project_id' + ) + }) + }) + + describe('POST /project/:Project_id/folder', function () { + it('should reject a malformed project id with 404', async function () { + const { response, body } = await owner.doRequest('post', { + url: '/project/not-an-object-id/folder', + json: { name: 'a-folder', parent_folder_id: rootFolderId }, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 404, + 'Project_id' + ) + }) + }) + + describe('POST /project/:Project_id/:entity_type/:entity_id/rename', function () { + it('should reject a malformed project id with 404', async function () { + const { response, body } = await owner.doRequest('post', { + url: `/project/not-an-object-id/doc/${projectId}/rename`, + json: { name: 'renamed.tex' }, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 404, + 'Project_id' + ) + }) + + it('should reject an invalid entity_type with 404', async function () { + const { response, body } = await owner.doRequest('post', { + url: `/project/${projectId}/not-a-type/${projectId}/rename`, + json: { name: 'renamed.tex' }, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 404, + 'entity_type' + ) + }) + }) + + describe('POST /project/:Project_id/:entity_type/:entity_id/move', function () { + it('should reject a malformed project id with 404', async function () { + const { response, body } = await owner.doRequest('post', { + url: `/project/not-an-object-id/doc/${projectId}/move`, + json: { folder_id: projectId }, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 404, + 'Project_id' + ) + }) + }) + + describe('DELETE /project/:Project_id/doc/:entity_id', function () { + it('should reject a malformed project id with 404', async function () { + const { response, body } = await owner.doRequest('delete', { + url: `/project/not-an-object-id/doc/${projectId}`, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 404, + 'Project_id' + ) + }) + }) + + describe('DELETE /project/:Project_id/file/:entity_id', function () { + it('should reject a malformed project id with 404', async function () { + const { response, body } = await owner.doRequest('delete', { + url: `/project/not-an-object-id/file/${projectId}`, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 404, + 'Project_id' + ) + }) + }) + + describe('DELETE /project/:Project_id/folder/:entity_id', function () { + it('should reject a malformed project id with 404', async function () { + const { response, body } = await owner.doRequest('delete', { + url: `/project/not-an-object-id/folder/${projectId}`, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 404, + 'Project_id' + ) + }) + }) + + describe('POST /project/:Project_id/join', function () { + it('should reject a userId that is neither an ObjectId nor "anonymous-user" with 400', async function () { + const { response, body } = await owner.doRequest('post', { + url: `/project/${projectId}/join`, + auth: { + user: settings.apis.web.user, + pass: settings.apis.web.pass, + sendImmediately: true, + }, + json: { userId: 'not-a-valid-user-id' }, + jar: false, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 400, + 'userId' + ) + }) + }) +}) diff --git a/services/web/test/acceptance/src/MalformedFiletreesTests.mjs b/services/web/test/acceptance/src/MalformedFiletreesTests.mjs index 63f60ffff4..fe444e2330 100644 --- a/services/web/test/acceptance/src/MalformedFiletreesTests.mjs +++ b/services/web/test/acceptance/src/MalformedFiletreesTests.mjs @@ -9,10 +9,22 @@ const lastUpdated = new Date(42) const lastUpdatedBy = new ObjectId() const lastUpdatedChanged = new Date(1337) +// Deprecation warnings (e.g. node:punycode's DEP0040) print as an extra +// logger line on the script's stdout, which breaks the deep-equal +// assertions below on the captured output; suppress them for this child +// process only, not the main app/test process. +const runScriptEnv = { + ...process.env, + NODE_OPTIONS: [process.env.NODE_OPTIONS, '--no-deprecation'] + .filter(Boolean) + .join(' '), +} + async function runScriptFind() { try { const result = await promisify(exec)( - ['node', 'scripts/find_malformed_filetrees.mjs'].join(' ') + ['node', 'scripts/find_malformed_filetrees.mjs'].join(' '), + { env: runScriptEnv } ) return result.stdout.split('\n').filter(filterOutput) } catch (error) { @@ -30,7 +42,7 @@ async function runScriptFix(instructions) { 'scripts/fix_malformed_filetree.mjs', `--logs=<(echo '${adhocFile}')`, ].join(' '), - { shell: '/bin/bash' } + { shell: '/bin/bash', env: runScriptEnv } ) } catch (error) { logger.error({ error }, 'fix script failed unexpectedly') diff --git a/services/web/test/acceptance/src/ProjectCRUDTests.mjs b/services/web/test/acceptance/src/ProjectCRUDTests.mjs index 5b232eb86e..88234c6c45 100644 --- a/services/web/test/acceptance/src/ProjectCRUDTests.mjs +++ b/services/web/test/acceptance/src/ProjectCRUDTests.mjs @@ -172,6 +172,14 @@ describe('Project CRUD', function () { expectObjectIdArrayEqual(trashedProject.archived, []) }) }) + + it('should return 404 for a malformed project id', async function () { + const { response } = await this.user.doRequest( + 'POST', + '/project/not-an-object-id/trash' + ) + expect(response.statusCode).to.equal(404) + }) }) describe('when untrashing a project', function () { @@ -211,6 +219,108 @@ describe('Project CRUD', function () { const trashedProject = await Project.findById(this.projectId).exec() expectObjectIdArrayEqual(trashedProject.trashed, []) }) + + it('should return 404 for a malformed project id', async function () { + const { response } = await this.user.doRequest( + 'DELETE', + '/project/not-an-object-id/trash' + ) + expect(response.statusCode).to.equal(404) + }) + }) + + describe('archiving and unarchiving a project', function () { + it('should archive the project', async function () { + const { response } = await this.user.doRequest( + 'POST', + `/Project/${this.projectId}/archive` + ) + expect(response.statusCode).to.equal(200) + }) + + it('should unarchive the project', async function () { + await this.user.doRequest('POST', `/Project/${this.projectId}/archive`) + + const { response } = await this.user.doRequest( + 'DELETE', + `/Project/${this.projectId}/archive` + ) + expect(response.statusCode).to.equal(200) + }) + + it('should return 404 for a malformed project id when archiving', async function () { + const { response } = await this.user.doRequest( + 'POST', + '/Project/not-an-object-id/archive' + ) + expect(response.statusCode).to.equal(404) + }) + + it('should return 404 for a malformed project id when unarchiving', async function () { + const { response } = await this.user.doRequest( + 'DELETE', + '/Project/not-an-object-id/archive' + ) + expect(response.statusCode).to.equal(404) + }) + }) + + describe('deleting and restoring a project', function () { + it('should return 404 for a malformed project id when deleting', async function () { + const { response } = await this.user.doRequest( + 'DELETE', + '/Project/not-an-object-id' + ) + expect(response.statusCode).to.equal(404) + }) + + it('should return 404 for a malformed project id when restoring', async function () { + const { response } = await this.user.doRequest( + 'POST', + '/Project/not-an-object-id/restore' + ) + expect(response.statusCode).to.equal(404) + }) + }) + + describe('renaming a project', function () { + it('should return 404 for a malformed project id', async function () { + const { response } = await this.user.doRequest('POST', { + url: '/project/not-an-object-id/rename', + json: { newProjectName: 'a new name' }, + }) + expect(response.statusCode).to.equal(404) + }) + }) + + describe('project entities', function () { + it('should return 404 for a malformed project id', async function () { + const { response } = await this.user.doRequest( + 'GET', + '/project/not-an-object-id/entities' + ) + expect(response.statusCode).to.equal(404) + }) + }) + + describe('updateProjectSettings', function () { + it('should update the compiler', async function () { + const { response } = await this.user.doRequest('POST', { + url: `/project/${this.projectId}/settings`, + json: { compiler: 'pdflatex' }, + }) + expect(response.statusCode).to.equal(204) + const project = await Project.findById(this.projectId).exec() + expect(project.compiler).to.equal('pdflatex') + }) + + it('should return 400 for a malformed mainBibliographyDocId', async function () { + const { response } = await this.user.doRequest('POST', { + url: `/project/${this.projectId}/settings`, + json: { mainBibliographyDocId: 'not-an-object-id' }, + }) + expect(response.statusCode).to.equal(400) + }) }) describe('ProjectAdminSettings', async function () { @@ -258,6 +368,31 @@ describe('Project CRUD', function () { expect(body).to.equal('Internal Server Error') }) }) + + describe('cloning a project', function () { + it('should create a new project owned by the same user', async function () { + const { response, body } = await this.user.doRequest('POST', { + url: `/Project/${this.projectId}/clone`, + json: { projectName: 'cloned project' }, + }) + expect(response.statusCode).to.equal(200) + expect(body.project_id).to.exist + expect(body.name).to.equal('cloned project') + + const clonedProject = await Project.findById(body.project_id).exec() + expect(clonedProject.owner_ref.toString()).to.equal( + this.user._id.toString() + ) + }) + + it('should return 404 for a malformed project id', async function () { + const { response } = await this.user.doRequest('POST', { + url: '/Project/not-an-object-id/clone', + json: { projectName: 'cloned project' }, + }) + expect(response.statusCode).to.equal(404) + }) + }) }) function expectObjectIdArrayEqual(objectIdArray, stringArray) { diff --git a/services/web/test/acceptance/src/ProjectDuplicateNameTests.mjs b/services/web/test/acceptance/src/ProjectDuplicateNameTests.mjs index ba849455ea..62bcd59498 100644 --- a/services/web/test/acceptance/src/ProjectDuplicateNameTests.mjs +++ b/services/web/test/acceptance/src/ProjectDuplicateNameTests.mjs @@ -189,7 +189,6 @@ describe('ProjectDuplicateNames', function () { json: true, qs: { folder_id: this.rootFolderId, - qqfilename: 'frog.jpg', }, formData: { name: 'frog.jpg', @@ -276,7 +275,6 @@ describe('ProjectDuplicateNames', function () { json: true, qs: { folder_id: this.rootFolderId, - qqfilename: 'frog.jpg', }, formData: { qqfile: { diff --git a/services/web/test/acceptance/src/ProjectInviteTests.mjs b/services/web/test/acceptance/src/ProjectInviteTests.mjs index 935c4779c1..b4c16a5b4a 100644 --- a/services/web/test/acceptance/src/ProjectInviteTests.mjs +++ b/services/web/test/acceptance/src/ProjectInviteTests.mjs @@ -532,6 +532,112 @@ describe('ProjectInviteTests', function () { }) }) + it('should reject an unrecognized field in the invite request body', function (done) { + this.sendingUser.getCsrfToken(err => { + if (err) { + return done(err) + } + this.sendingUser.request.post( + { + uri: `/project/${this.projectId}/invite`, + json: { + email: this.email, + privileges: 'readAndWrite', + notARealField: 'nope', + }, + }, + (err, response) => { + if (err) { + return done(err) + } + expect(response.statusCode).to.equal(400) + done() + } + ) + }) + }) + + it('should reject an invalid invite id when revoking an invite', function (done) { + this.sendingUser.getCsrfToken(err => { + if (err) { + return done(err) + } + this.sendingUser.request.delete( + { + uri: `/project/${this.projectId}/invite/not-a-valid-invite-id`, + }, + (err, response) => { + if (err) { + return done(err) + } + expect(response.statusCode).to.equal(404) + done() + } + ) + }) + }) + + it('should reject an invalid invite id when resending an invite', function (done) { + this.sendingUser.getCsrfToken(err => { + if (err) { + return done(err) + } + this.sendingUser.request.post( + { + uri: `/project/${this.projectId}/invite/not-a-valid-invite-id/resend`, + }, + (err, response) => { + if (err) { + return done(err) + } + expect(response.statusCode).to.equal(404) + done() + } + ) + }) + }) + + it('should allow the project owner to resend an invite', function (done) { + Async.series( + [ + cb => { + createInvite( + this.sendingUser, + this.projectId, + this.email, + (err, invite) => { + if (err) { + return cb(err) + } + this.invite = invite + cb() + } + ) + }, + cb => { + this.sendingUser.getCsrfToken(err => { + if (err) { + return cb(err) + } + this.sendingUser.request.post( + { + uri: `/project/${this.projectId}/invite/${this.invite._id}/resend`, + }, + (err, response) => { + if (err) { + return cb(err) + } + expect(response.statusCode).to.equal(201) + cb() + } + ) + }) + }, + ], + done + ) + }) + it('should allow the project owner to create and remove invites', function (done) { Async.series( [ @@ -835,6 +941,70 @@ describe('ProjectInviteTests', function () { done ) }) + + it('should reject a malformed project id when viewing the invite', function (done) { + const link = this.link.replace( + this.projectId, + 'not-a-valid-project-id' + ) + tryFollowInviteLink(this.user, link, (err, response) => { + if (err) { + return done(err) + } + expect(response.statusCode).to.equal(404) + done() + }) + }) + + it('should accept a genuine native HTML form submission (form-encoded body, no CSRF header)', function (done) { + // views/project/invite/show-legacy.pug submits this as a plain + // native HTML form (data-ol-regular-form): no JS involved, so + // the token only ever travels as a hidden _csrf form field, + // never as a header. + this.user.getCsrfToken(err => { + if (err) { + return done(err) + } + this.user.submitNativeForm( + `/project/${this.projectId}/invite/token/${this.invite.token}/accept`, + { token: this.invite.token }, + (err, response) => { + if (err) { + return done(err) + } + expect(response.statusCode).to.equal(302) + expect(response.headers.location).to.equal( + `/project/${this.projectId}` + ) + done() + } + ) + }) + }) + + it('should still reject an unrecognized field in the body', function (done) { + this.user.getCsrfToken(err => { + if (err) { + return done(err) + } + this.user.request.post( + { + uri: `/project/${this.projectId}/invite/token/${this.invite.token}/accept`, + json: { + token: this.invite.token, + notARealField: 'nope', + }, + }, + (err, response) => { + if (err) { + return done(err) + } + expect(response.statusCode).to.equal(400) + done() + } + ) + }) + }) }) }) @@ -1041,6 +1211,91 @@ describe('ProjectInviteTests', function () { ) }) + it('should reject an unrecognized field when updating a sharing link', function (done) { + this.sendingUser.getCsrfToken(err => { + if (err) { + return done(err) + } + this.sendingUser.request.post( + { + uri: `/project/${this.projectId}/sharing-link`, + json: { + privileges: 'readOnly', + notARealField: 'nope', + }, + }, + (err, response) => { + if (err) { + return done(err) + } + expect(response.statusCode).to.equal(400) + done() + } + ) + }) + }) + + it('should reject a malformed project id when viewing the sharing-link page', function (done) { + this.sendingUser.request.get( + { + uri: '/project/not-a-valid-project-id/share', + }, + (err, response) => { + if (err) { + return done(err) + } + expect(response.statusCode).to.equal(404) + done() + } + ) + }) + + it('should reject an unrecognized field when validating a sharing link', function (done) { + Async.series( + [ + cb => this.user.login(cb), + cb => { + updateSharingLink( + this.sendingUser, + this.projectId, + 'readOnly', + (err, link) => { + if (err) { + return cb(err) + } + this.sharingLink = link + cb() + } + ) + }, + cb => { + this.user.getCsrfToken(err => { + if (err) { + return cb(err) + } + this.user.request.post( + { + uri: `/project/${this.projectId}/share/validate`, + json: { + token: this.sharingLink.token, + notARealField: 'nope', + }, + }, + (err, response) => { + if (err) { + return cb(err) + } + expect(response.statusCode).to.equal(400) + cb() + } + ) + }) + }, + ], + done + ) + }) + it('should validate and accept sharing link via /share flow', function (done) { Async.series( [ diff --git a/services/web/test/acceptance/src/ProjectStructureTests.mjs b/services/web/test/acceptance/src/ProjectStructureTests.mjs index 52520eb5d1..fe013fd81c 100644 --- a/services/web/test/acceptance/src/ProjectStructureTests.mjs +++ b/services/web/test/acceptance/src/ProjectStructureTests.mjs @@ -101,6 +101,29 @@ describe('ProjectStructureChanges', function () { return await owner.deleteItemInProject(projectId, type, itemId) } + describe('uploading a project with relativePath set to the literal "null" string', function () { + it('should create the project successfully', async function () { + const zipFilename = 'test_project_with_name.zip' + const zipFile = fs.createReadStream( + Path.resolve(Path.join(import.meta.dirname, '..', 'files', zipFilename)) + ) + + const { response, body } = await owner.doRequest('POST', { + uri: 'project/new/upload', + formData: { + name: zipFilename, + qqfile: zipFile, + relativePath: 'null', + }, + }) + + expect(response.statusCode).to.be.within(200, 299) + const parsedBody = JSON.parse(body) + expect(parsedBody.success).to.equal(true) + expect(parsedBody.project_id).to.be.a('string') + }) + }) + describe('uploading a project with a name', function () { let exampleProjectId const testProjectName = 'wombat' diff --git a/services/web/test/acceptance/src/SharingTests.mjs b/services/web/test/acceptance/src/SharingTests.mjs index 3d3306a068..44026ccd49 100644 --- a/services/web/test/acceptance/src/SharingTests.mjs +++ b/services/web/test/acceptance/src/SharingTests.mjs @@ -149,4 +149,193 @@ describe('Sharing', function () { expect(project.readOnly_refs).to.deep.equal([this.reviewer._id]) }) }) + + describe('validation', function () { + it('rejects an unrecognized field in the body when setting collaborator info', async function () { + await this.ownerSession.addUserToProject( + this.projectId, + this.collaborator, + 'readOnly' + ) + await expect( + this.ownerSession.setCollaboratorInfo( + this.projectId, + this.collaborator._id, + { privilegeLevel: 'readOnly', notARealField: 'nope' } + ) + ).to.be.rejectedWith(/failed: status=400 /) + }) + + it('rejects an unrecognized field in the body when transferring ownership', async function () { + await this.ownerSession.addUserToProject( + this.projectId, + this.collaborator, + 'readAndWrite' + ) + const { response } = await this.ownerSession.doRequest('POST', { + url: `/project/${this.projectId}/transfer-ownership`, + json: { + user_id: this.collaborator._id.toString(), + notARealField: 'nope', + }, + }) + expect(response.statusCode).to.equal(400) + }) + }) + + describe('removing a collaborator', function () { + beforeEach(async function () { + await this.ownerSession.addUserToProject( + this.projectId, + this.collaborator, + 'readAndWrite' + ) + }) + + it('lets the owner remove a collaborator', async function () { + const { response } = await this.ownerSession.doRequest('DELETE', { + url: `/project/${this.projectId}/users/${this.collaborator._id}`, + }) + expect(response.statusCode).to.equal(204) + const project = await this.ownerSession.getProject(this.projectId) + expect(project.collaberator_refs).to.deep.equal([]) + }) + + it('prevents non-owners from removing a collaborator', async function () { + const { response } = await this.collaboratorSession.doRequest('DELETE', { + url: `/project/${this.projectId}/users/${this.collaborator._id}`, + }) + expect(response.statusCode).to.equal(403) + }) + }) + + describe('leaving a project', function () { + beforeEach(async function () { + await this.ownerSession.addUserToProject( + this.projectId, + this.collaborator, + 'readAndWrite' + ) + }) + + it('lets a collaborator leave the project', async function () { + const { response } = await this.collaboratorSession.doRequest('POST', { + url: `/project/${this.projectId}/leave`, + }) + expect(response.statusCode).to.equal(204) + const project = await this.ownerSession.getProject(this.projectId) + expect(project.collaberator_refs).to.deep.equal([]) + }) + + it('rejects a malformed project id', async function () { + const { response } = await this.collaboratorSession.doRequest('POST', { + url: '/project/not-a-valid-project-id/leave', + }) + expect(response.statusCode).to.equal(404) + }) + }) + + describe('listing project members', function () { + beforeEach(async function () { + await this.ownerSession.addUserToProject( + this.projectId, + this.collaborator, + 'readAndWrite' + ) + }) + + it('lists invited members for a collaborator', async function () { + const { response, body } = await this.collaboratorSession.doRequest( + 'GET', + { url: `/project/${this.projectId}/members`, json: true } + ) + expect(response.statusCode).to.equal(200) + expect(body.members.map(member => member._id.toString())).to.have.members( + [this.collaborator._id.toString()] + ) + }) + }) + + describe('access requests', function () { + beforeEach(async function () { + await this.ownerSession.addUserToProject( + this.projectId, + this.stranger, + 'readOnly' + ) + }) + + it('lets a viewer request edit access, visible to the owner', async function () { + const requested = await this.strangerSession.doRequest('POST', { + url: `/project/${this.projectId}/request-access`, + json: { privilegeLevel: 'readAndWrite' }, + }) + expect(requested.response.statusCode).to.equal(204) + + const { response, body } = await this.ownerSession.doRequest('GET', { + url: `/project/${this.projectId}/access-requests`, + json: true, + }) + expect(response.statusCode).to.equal(200) + expect( + body.editAccessRequests.map(request => request._id.toString()) + ).to.have.members([this.stranger._id.toString()]) + }) + + it('prevents non-owners from listing access requests', async function () { + const { response } = await this.collaboratorSession.doRequest('GET', { + url: `/project/${this.projectId}/access-requests`, + json: true, + }) + expect(response.statusCode).to.equal(403) + }) + + it('lets the owner grant a pending access request', async function () { + await this.strangerSession.doRequest('POST', { + url: `/project/${this.projectId}/request-access`, + json: { privilegeLevel: 'readAndWrite' }, + }) + const { response } = await this.ownerSession.doRequest('POST', { + url: `/project/${this.projectId}/access-requests/${this.stranger._id}/grant`, + json: { privilegeLevel: 'readAndWrite' }, + }) + expect(response.statusCode).to.equal(204) + const project = await this.ownerSession.getProject(this.projectId) + expect(project.collaberator_refs.map(id => id.toString())).to.include( + this.stranger._id.toString() + ) + }) + + it('lets the owner decline a pending access request', async function () { + await this.strangerSession.doRequest('POST', { + url: `/project/${this.projectId}/request-access`, + json: { privilegeLevel: 'readAndWrite' }, + }) + const declined = await this.ownerSession.doRequest('DELETE', { + url: `/project/${this.projectId}/access-requests/${this.stranger._id}`, + }) + expect(declined.response.statusCode).to.equal(204) + + const { body } = await this.ownerSession.doRequest('GET', { + url: `/project/${this.projectId}/access-requests`, + json: true, + }) + expect(body.editAccessRequests).to.deep.equal([]) + }) + + it('rejects a malformed user id when declining an access request', async function () { + const { response } = await this.ownerSession.doRequest('DELETE', { + url: `/project/${this.projectId}/access-requests/not-a-valid-user-id`, + }) + expect(response.statusCode).to.equal(404) + }) + + it('rejects a malformed user id when granting an access request', async function () { + const { response } = await this.ownerSession.doRequest('POST', { + url: `/project/${this.projectId}/access-requests/not-a-valid-user-id/grant`, + json: { privilegeLevel: 'readAndWrite' }, + }) + expect(response.statusCode).to.equal(404) + }) + }) }) diff --git a/services/web/test/acceptance/src/TagsTests.mjs b/services/web/test/acceptance/src/TagsTests.mjs index 7c17353833..2a0327b83e 100644 --- a/services/web/test/acceptance/src/TagsTests.mjs +++ b/services/web/test/acceptance/src/TagsTests.mjs @@ -4,6 +4,7 @@ import { expect } from 'chai' import _ from 'lodash' import request from './helpers/request.js' import expectErrorResponse from './helpers/expectErrorResponse.mjs' +import { expectValidationErrorRaw } from '@overleaf/validation-tools/testUtils.js' const _initUser = (user, callback) => { async.series([cb => user.login(cb), cb => user.getCsrfToken(cb)], callback) @@ -177,6 +178,41 @@ describe('Tags', function () { ) }) }) + + it('should reject a malformed userId', function (done) { + authedRequest.get( + { url: `/user/not-an-object-id/tag`, json: true }, + (err, response) => { + expect(err).to.not.exist + expectValidationErrorRaw(response, 404, 'userId') + done() + } + ) + }) + }) + + describe('create tag validation', function () { + it('should reject an empty name', function (done) { + this.user.request.post( + { url: `/tag`, json: { name: '' } }, + (err, response) => { + expect(err).to.not.exist + expectValidationErrorRaw(response, 400, 'name') + done() + } + ) + }) + + it('should reject an invalid color', function (done) { + this.user.request.post( + { url: `/tag`, json: { name: 'a-tag', color: 'red' } }, + (err, response) => { + expect(err).to.not.exist + expectValidationErrorRaw(response, 400, 'color') + done() + } + ) + }) }) describe('rename tag', function () { @@ -185,7 +221,7 @@ describe('Tags', function () { { url: `/tag/lol/rename`, json: { name: 'five' } }, (err, response) => { expect(err).to.not.exist - expect(response.statusCode).to.equal(500) + expectValidationErrorRaw(response, 404, 'tagId') done() } ) @@ -253,7 +289,7 @@ describe('Tags', function () { { url: `/tag/lol`, json: { name: 'five' } }, (err, response) => { expect(err).to.not.exist - expect(response.statusCode).to.equal(500) + expectValidationErrorRaw(response, 404, 'tagId') done() } ) @@ -298,7 +334,7 @@ describe('Tags', function () { { url: `/tag/lol/project/bad` }, (err, response) => { expect(err).to.not.exist - expect(response.statusCode).to.equal(500) + expectValidationErrorRaw(response, 404, 'tagId') done() } ) @@ -384,5 +420,238 @@ describe('Tags', function () { }) }) }) + + it('should reject malformed tag id when removing a project', function (done) { + this.user.request.delete( + { url: `/tag/lol/project/${this.projectId}` }, + (err, response) => { + expect(err).to.not.exist + expectValidationErrorRaw(response, 404, 'tagId') + done() + } + ) + }) + }) + + describe('add projects to tag, bulk', function () { + beforeEach(function (done) { + async.series( + [ + cb => + this.user.createProject('bulk 1', (err, id) => { + this.projectId1 = id + cb(err) + }), + cb => + this.user.createProject('bulk 2', (err, id) => { + this.projectId2 = id + cb(err) + }), + ], + done + ) + }) + + it('should reject malformed tag id', function (done) { + this.user.request.post( + { url: `/tag/lol/projects`, json: { projectIds: [] } }, + (err, response) => { + expect(err).to.not.exist + expectValidationErrorRaw(response, 404, 'tagId') + done() + } + ) + }) + + it('should reject a malformed project id in the list', function (done) { + _createTags(this.user, ['bulk-tag'], (err, tags) => { + expect(err).to.not.exist + const tagId = tags[0]._id + this.user.request.post( + { + url: `/tag/${tagId}/projects`, + json: { projectIds: ['not-an-object-id'] }, + }, + (err, response) => { + expect(err).to.not.exist + expectValidationErrorRaw(response, 400, 'projectIds') + done() + } + ) + }) + }) + + it('should add multiple projects to a tag', function (done) { + _createTags(this.user, ['bulk-tag'], (err, tags) => { + expect(err).to.not.exist + const tagId = tags[0]._id + this.user.request.post( + { + url: `/tag/${tagId}/projects`, + json: { projectIds: [this.projectId1, this.projectId2] }, + }, + (err, response) => { + _expect204(err, response) + _getTags(this.user, (err, response, body) => { + _expect200(err, response) + const tag = _.find(body, t => t.name === 'bulk-tag') + expect(_.sortBy(tag.project_ids)).to.deep.equal( + _.sortBy([this.projectId1, this.projectId2]) + ) + done() + }) + } + ) + }) + }) + }) + + describe('remove projects from tag, bulk', function () { + beforeEach(function (done) { + async.series( + [ + cb => + this.user.createProject('bulk-remove 1', (err, id) => { + this.projectId1 = id + cb(err) + }), + cb => + this.user.createProject('bulk-remove 2', (err, id) => { + this.projectId2 = id + cb(err) + }), + ], + done + ) + }) + + it('should reject malformed tag id', function (done) { + this.user.request.post( + { url: `/tag/lol/projects/remove`, json: { projectIds: [] } }, + (err, response) => { + expect(err).to.not.exist + expectValidationErrorRaw(response, 404, 'tagId') + done() + } + ) + }) + + it('should reject a malformed project id in the list', function (done) { + _createTags(this.user, ['bulk-remove-tag'], (err, tags) => { + expect(err).to.not.exist + const tagId = tags[0]._id + this.user.request.post( + { + url: `/tag/${tagId}/projects/remove`, + json: { projectIds: ['not-an-object-id'] }, + }, + (err, response) => { + expect(err).to.not.exist + expectValidationErrorRaw(response, 400, 'projectIds') + done() + } + ) + }) + }) + + it('should remove multiple projects from a tag', function (done) { + _createTags(this.user, ['bulk-remove-tag'], (err, tags) => { + expect(err).to.not.exist + const tagId = tags[0]._id + this.user.request.post( + { + url: `/tag/${tagId}/projects`, + json: { projectIds: [this.projectId1, this.projectId2] }, + }, + (err, response) => { + _expect204(err, response) + this.user.request.post( + { + url: `/tag/${tagId}/projects/remove`, + json: { projectIds: [this.projectId1] }, + }, + (err, response) => { + _expect204(err, response) + _getTags(this.user, (err, response, body) => { + _expect200(err, response) + const tag = _.find(body, t => t.name === 'bulk-remove-tag') + expect(tag.project_ids).to.deep.equal([this.projectId2]) + done() + }) + } + ) + } + ) + }) + }) + }) + + describe('edit tag', function () { + it('should reject malformed tag id', function (done) { + this.user.request.post( + { url: `/tag/lol/edit`, json: { name: 'five' } }, + (err, response) => { + expect(err).to.not.exist + expectValidationErrorRaw(response, 404, 'tagId') + done() + } + ) + }) + + it('should reject an empty name', function (done) { + _createTags(this.user, ['editable'], (err, tags) => { + expect(err).to.not.exist + const tagId = tags[0]._id + this.user.request.post( + { url: `/tag/${tagId}/edit`, json: { name: '' } }, + (err, response) => { + expect(err).to.not.exist + expectValidationErrorRaw(response, 400, 'name') + done() + } + ) + }) + }) + + it('should reject an invalid color', function (done) { + _createTags(this.user, ['editable'], (err, tags) => { + expect(err).to.not.exist + const tagId = tags[0]._id + this.user.request.post( + { + url: `/tag/${tagId}/edit`, + json: { name: 'editable', color: 'blue' }, + }, + (err, response) => { + expect(err).to.not.exist + expectValidationErrorRaw(response, 400, 'color') + done() + } + ) + }) + }) + + it('should allow user to edit a tag name and color', function (done) { + _createTags(this.user, ['editable'], (err, tags) => { + expect(err).to.not.exist + const tagId = tags[0]._id + this.user.request.post( + { + url: `/tag/${tagId}/edit`, + json: { name: 'edited', color: '#abcdef' }, + }, + (err, response) => { + _expect204(err, response) + _getTags(this.user, (err, response, body) => { + _expect200(err, response) + const tag = _.find(body, t => t._id === tagId) + expect(tag.name).to.equal('edited') + expect(tag.color).to.equal('#abcdef') + done() + }) + } + ) + }) + }) }) }) diff --git a/services/web/test/acceptance/src/mocks/MockClsiCache.mjs b/services/web/test/acceptance/src/mocks/MockClsiCache.mjs index c963f7816f..a9d24ea24d 100644 --- a/services/web/test/acceptance/src/mocks/MockClsiCache.mjs +++ b/services/web/test/acceptance/src/mocks/MockClsiCache.mjs @@ -14,18 +14,8 @@ const lookupSchema = z.object({ // matches clsi-cache's own getLastOutputFileParamsSchema / // getLatestOutputFileParamsSchema (HTTPController.js) editorBuildId: zz.editorBuildId().optional(), - // this is a wildcard `:filename(.*)` param forwarded to a storage - // lookup key, so it gets the same zz.filepath() traversal hardening as - // clsi-cache's own filenameSchema + // matches clsi-cache's own filenameSchema (HTTPController.js) filename: zz.filepath(), - // this repo's patched Express additionally populates req.params[0] - // (the filename minus its first character) for a *named* regex - // wildcard like `:filename(.*)` -- unlike a bare `*` (see - // MockClsiNginxApi.mjs's outputWildcardSchema, which only gets the - // numeric key). A strictObject that doesn't account for this key - // rejects every real (non-empty filename) request with a 404 - // "Unrecognized key: 0" before it ever reaches the lookup below. - 0: z.string(), }), }) @@ -106,21 +96,20 @@ class MockClsiCache extends AbstractMockApi { applyRoutes() { // getOutputFile's two routes (with/without the per-user segment) this.app.get( - '/project/:project_id/user/:user_id/build/:editorBuildId/search/output/:filename(.*)', + '/project/:project_id/user/:user_id/build/:editorBuildId/search/output/:filename', (req, res) => this._lookup(req, res) ) this.app.get( - '/project/:project_id/build/:editorBuildId/search/output/:filename(.*)', + '/project/:project_id/build/:editorBuildId/search/output/:filename', (req, res) => this._lookup(req, res) ) // getLatestOutputFile's two routes (with/without the per-user segment) this.app.get( - '/project/:project_id/user/:user_id/latest/output/:filename(.*)', + '/project/:project_id/user/:user_id/latest/output/:filename', (req, res) => this._lookup(req, res) ) - this.app.get( - '/project/:project_id/latest/output/:filename(.*)', - (req, res) => this._lookup(req, res) + this.app.get('/project/:project_id/latest/output/:filename', (req, res) => + this._lookup(req, res) ) this.app.get('/content/:contentId', (req, res) => { diff --git a/services/web/test/acceptance/src/mocks/MockClsiNginxApi.mjs b/services/web/test/acceptance/src/mocks/MockClsiNginxApi.mjs index 5439fdf8bd..c6c696471c 100644 --- a/services/web/test/acceptance/src/mocks/MockClsiNginxApi.mjs +++ b/services/web/test/acceptance/src/mocks/MockClsiNginxApi.mjs @@ -8,18 +8,17 @@ const outputWildcardSchema = z.object({ params: z.strictObject({ project_id: zz.objectId().or(zz.submissionId()), build_id: zz.buildId(), - // bare `*` wildcard -- Express populates the match under numeric key 0 - 0: zz.filepath(), + file: zz.filepath(), }), }) class MockClsiNginxApi extends AbstractMockApi { applyRoutes() { this.app.get( - '/project/:project_id/build/:build_id/output/*', + '/project/:project_id/build/:build_id/output/:file(.+)', (req, res) => { const { params } = parseReq(req, outputWildcardSchema) - const filename = params[0] + const filename = params.file if (filename === 'output.pdf') { plainTextResponse(res, 'mock-pdf') } else if (filename === 'output.log') { diff --git a/services/web/test/unit/src/Chat/ChatController.test.mjs b/services/web/test/unit/src/Chat/ChatController.test.mjs index 55ee4d944b..edebe6cdb0 100644 --- a/services/web/test/unit/src/Chat/ChatController.test.mjs +++ b/services/web/test/unit/src/Chat/ChatController.test.mjs @@ -6,6 +6,9 @@ const MODULE_PATH = '../../../../app/src/Features/Chat/ChatController.mjs' describe('ChatController', function () { beforeEach(async function (ctx) { ctx.user_id = 'mock-user-id' + // project_id/message_id are validated as Mongo ObjectIds + ctx.project_id = '507f191e810c19729de860ea' + ctx.message_id = '507f191e810c19729de860eb' ctx.settings = {} ctx.ChatApiHandler = { promises: {} } ctx.ChatManager = { promises: {} } @@ -126,10 +129,11 @@ describe('ChatController', function () { describe('getMessages', function () { beforeEach(async function (ctx) { - ctx.req.query = { - limit: (ctx.limit = '30'), - before: (ctx.before = '12345'), - } + // sent over the wire as query-string values, but coerced to numbers + // by the schema before being passed on + ctx.req.query = { limit: '30', before: '12345' } + ctx.limit = 30 + ctx.before = 12345 ctx.ChatManager.promises.injectUserInfoIntoThreads = sinon .stub() .resolves() @@ -149,4 +153,60 @@ describe('ChatController', function () { ctx.res.json.calledWith(ctx.messages).should.equal(true) }) }) + + describe('deleteMessage', function () { + beforeEach(async function (ctx) { + ctx.req.params.message_id = ctx.message_id + ctx.ChatApiHandler.promises.deleteGlobalMessage = sinon.stub().resolves() + await ctx.ChatController.deleteMessage(ctx.req, ctx.res) + }) + + it('should tell the chat handler to delete the message', function (ctx) { + ctx.ChatApiHandler.promises.deleteGlobalMessage + .calledWith(ctx.project_id, ctx.message_id) + .should.equal(true) + }) + + it('should tell the editor real time controller about the deletion', function (ctx) { + ctx.EditorRealTimeController.emitToRoom + .calledWith(ctx.project_id, 'delete-global-message', { + messageId: ctx.message_id, + userId: ctx.user_id, + }) + .should.equal(true) + }) + + it('should return a 204 status code', function (ctx) { + ctx.res.sendStatus.calledWith(204).should.equal(true) + }) + }) + + describe('editMessage', function () { + beforeEach(async function (ctx) { + ctx.req.params.message_id = ctx.message_id + ctx.req.body = { content: (ctx.content = 'edited-content') } + ctx.ChatApiHandler.promises.editGlobalMessage = sinon.stub().resolves() + await ctx.ChatController.editMessage(ctx.req, ctx.res, sinon.stub()) + }) + + it('should tell the chat handler to edit the message', function (ctx) { + ctx.ChatApiHandler.promises.editGlobalMessage + .calledWith(ctx.project_id, ctx.message_id, ctx.user_id, ctx.content) + .should.equal(true) + }) + + it('should tell the editor real time controller about the edit', function (ctx) { + ctx.EditorRealTimeController.emitToRoom + .calledWith(ctx.project_id, 'edit-global-message', { + messageId: ctx.message_id, + userId: ctx.user_id, + content: ctx.content, + }) + .should.equal(true) + }) + + it('should return a 204 status code', function (ctx) { + ctx.res.sendStatus.calledWith(204).should.equal(true) + }) + }) }) diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsController.test.mjs b/services/web/test/unit/src/Collaborators/CollaboratorsController.test.mjs index 0a649eb1d9..d753a6fb48 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsController.test.mjs +++ b/services/web/test/unit/src/Collaborators/CollaboratorsController.test.mjs @@ -1,6 +1,10 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import sinon from 'sinon' import mongodb from 'mongodb-legacy' +import { + InvalidParamsError, + InvalidRequestError, +} from '@overleaf/validation-tools' import Errors from '../../../../app/src/Features/Errors/Errors.js' import MockRequest from '../helpers/MockRequest.mjs' import MockResponse from '../helpers/MockResponse.mjs' @@ -51,8 +55,12 @@ describe('CollaboratorsController', function () { promises: { getAllInvitedMembers: sinon.stub(), getProjectAccess: sinon.stub().resolves(ctx.projectAccess), + getPublicShareTokens: sinon.stub(), }, } + ctx.Features = { + hasFeature: sinon.stub().returns(true), + } ctx.AuthorizationManager = { promises: { getPrivilegeLevelForProject: sinon.stub().resolves('readOnly'), @@ -100,6 +108,7 @@ describe('CollaboratorsController', function () { } ctx.TokenAccessHandler = { getRequestToken: sinon.stub().returns('access-token'), + createTokenHashPrefix: sinon.stub().returns('hash-prefix'), } ctx.ProjectAuditLogHandler = { @@ -236,6 +245,10 @@ describe('CollaboratorsController', function () { default: ctx.EmailHandler, })) + vi.doMock('../../../../app/src/infrastructure/Features.mjs', () => ({ + default: ctx.Features, + })) + ctx.CollaboratorsController = (await import(MODULE_PATH)).default }) @@ -243,8 +256,8 @@ describe('CollaboratorsController', function () { beforeEach(async function (ctx) { await new Promise(resolve => { ctx.req.params = { - Project_id: ctx.projectId, - user_id: ctx.user._id, + Project_id: ctx.projectId.toString(), + user_id: ctx.user._id.toString(), } ctx.res.sendStatus = sinon.spy(() => { resolve() @@ -256,14 +269,17 @@ describe('CollaboratorsController', function () { it('should from the user from the project', function (ctx) { expect( ctx.CollaboratorsHandler.promises.removeUserFromProject - ).to.have.been.calledWith(ctx.projectId, ctx.user._id) + ).to.have.been.calledWith( + ctx.projectId.toString(), + ctx.user._id.toString() + ) }) it('should emit a userRemovedFromProject event to the proejct', function (ctx) { expect(ctx.EditorRealTimeController.emitToRoom).to.have.been.calledWith( - ctx.projectId, + ctx.projectId.toString(), 'userRemovedFromProject', - ctx.user._id + ctx.user._id.toString() ) }) @@ -273,36 +289,51 @@ describe('CollaboratorsController', function () { it('should have called emitToRoom', function (ctx) { expect(ctx.EditorRealTimeController.emitToRoom).to.have.been.calledWith( - ctx.projectId, + ctx.projectId.toString(), 'project:membership:changed' ) }) it('should look up the collaborator email', function (ctx) { expect(ctx.UserGetter.promises.getUser).to.have.been.calledWith( - { _id: ctx.user._id }, + { _id: ctx.user._id.toString() }, { email: 1 } ) }) it('should write a project audit log', function (ctx) { ctx.ProjectAuditLogHandler.addEntryInBackground.should.have.been.calledWith( - ctx.projectId, + ctx.projectId.toString(), 'remove-collaborator', ctx.user._id, ctx.req.ip, { - userId: ctx.user._id, + userId: ctx.user._id.toString(), collaboratorEmail: 'user@example.com', } ) }) }) + describe('removeUserFromProject with a malformed project id', function () { + it('rejects the request instead of removing anyone', async function (ctx) { + ctx.req.params = { + Project_id: 'not-an-object-id', + user_id: ctx.user._id.toString(), + } + await ctx.CollaboratorsController.removeUserFromProject( + ctx.req, + ctx.res + ).should.be.rejectedWith(InvalidParamsError) + expect(ctx.CollaboratorsHandler.promises.removeUserFromProject).to.not + .have.been.called + }) + }) + describe('removeSelfFromProject', function () { beforeEach(async function (ctx) { await new Promise(resolve => { - ctx.req.params = { Project_id: ctx.projectId } + ctx.req.params = { Project_id: ctx.projectId.toString() } ctx.res.sendStatus = sinon.spy(() => { resolve() }) @@ -313,12 +344,12 @@ describe('CollaboratorsController', function () { it('should remove the logged in user from the project', function (ctx) { expect( ctx.CollaboratorsHandler.promises.removeUserFromProject - ).to.have.been.calledWith(ctx.projectId, ctx.user._id) + ).to.have.been.calledWith(ctx.projectId.toString(), ctx.user._id) }) it('should emit a userRemovedFromProject event to the proejct', function (ctx) { expect(ctx.EditorRealTimeController.emitToRoom).to.have.been.calledWith( - ctx.projectId, + ctx.projectId.toString(), 'userRemovedFromProject', ctx.user._id ) @@ -327,7 +358,7 @@ describe('CollaboratorsController', function () { it('should remove the project from all tags', function (ctx) { expect( ctx.TagsHandler.promises.removeProjectFromAllTags - ).to.have.been.calledWith(ctx.user._id, ctx.projectId) + ).to.have.been.calledWith(ctx.user._id, ctx.projectId.toString()) }) it('should return a success code', function (ctx) { @@ -336,7 +367,7 @@ describe('CollaboratorsController', function () { it('should emit a project:membership:changed event to the project', function (ctx) { expect(ctx.EditorRealTimeController.emitToRoom).to.have.been.calledWith( - ctx.projectId, + ctx.projectId.toString(), 'project:membership:changed', { members: true } ) @@ -344,7 +375,7 @@ describe('CollaboratorsController', function () { it('should write a project audit log', function (ctx) { ctx.ProjectAuditLogHandler.addEntryInBackground.should.have.been.calledWith( - ctx.projectId, + ctx.projectId.toString(), 'leave-project', ctx.user._id, ctx.req.ip @@ -352,10 +383,22 @@ describe('CollaboratorsController', function () { }) }) + describe('removeSelfFromProject with a malformed project id', function () { + it('rejects the request instead of removing anyone', async function (ctx) { + ctx.req.params = { Project_id: 'not-an-object-id' } + await ctx.CollaboratorsController.removeSelfFromProject( + ctx.req, + ctx.res + ).should.be.rejectedWith(InvalidParamsError) + expect(ctx.CollaboratorsHandler.promises.removeUserFromProject).to.not + .have.been.called + }) + }) + describe('getAllMembers', function () { beforeEach(async function (ctx) { await new Promise(resolve => { - ctx.req.params = { Project_id: ctx.projectId } + ctx.req.params = { Project_id: ctx.projectId.toString() } ctx.res.json = sinon.spy(() => { resolve() }) @@ -409,6 +452,18 @@ describe('CollaboratorsController', function () { }) }) + describe('getAllMembers with a malformed project id', function () { + it('rejects the request instead of listing members', async function (ctx) { + ctx.req.params = { Project_id: 'not-an-object-id' } + await ctx.CollaboratorsController.getAllMembers( + ctx.req, + ctx.res + ).should.be.rejectedWith(InvalidParamsError) + expect(ctx.CollaboratorsGetter.promises.getAllInvitedMembers).to.not.have + .been.called + }) + }) + describe('getAccessRequests', function () { beforeEach(async function (ctx) { ctx.requesterId = new ObjectId() @@ -443,6 +498,18 @@ describe('CollaboratorsController', function () { }) }) + describe('getAccessRequests with a malformed project id', function () { + it('rejects the request instead of listing access requests', async function (ctx) { + ctx.req.params = { Project_id: 'not-an-object-id' } + await ctx.CollaboratorsController.getAccessRequests( + ctx.req, + ctx.res + ).should.be.rejectedWith(InvalidParamsError) + expect(ctx.CollaboratorsGetter.promises.getProjectAccess).to.not.have.been + .called + }) + }) + describe('setCollaboratorInfo', function () { beforeEach(function (ctx) { ctx.req.params = { @@ -502,6 +569,23 @@ describe('CollaboratorsController', function () { }) }) + it('rejects an unrecognized field in the body instead of falling back', async function (ctx) { + ctx.req.body = { privilegeLevel: 'readOnly', notARealField: 'nope' } + await new Promise(resolve => { + ctx.next = sinon.spy(err => { + expect(err).to.be.instanceOf(InvalidRequestError) + resolve() + }) + ctx.CollaboratorsController.setCollaboratorInfo( + ctx.req, + ctx.res, + ctx.next + ) + }) + expect(ctx.CollaboratorsHandler.promises.setCollaboratorPrivilegeLevel).to + .not.have.been.called + }) + describe('when setting privilege level to readAndWrite', function () { beforeEach(function (ctx) { ctx.req.body = { privilegeLevel: 'readAndWrite' } @@ -644,6 +728,19 @@ describe('CollaboratorsController', function () { ctx.CollaboratorsController.transferOwnership(ctx.req, ctx.res) }) }) + + it('rejects an unrecognized field in the body instead of falling back', async function (ctx) { + ctx.req.body = { + user_id: ctx.user._id.toString(), + notARealField: 'nope', + } + await ctx.CollaboratorsController.transferOwnership( + ctx.req, + ctx.res + ).should.be.rejectedWith(InvalidRequestError) + expect(ctx.OwnershipTransferHandler.promises.transferOwnership).to.not + .have.been.called + }) }) describe('requestAccess', function () { @@ -758,6 +855,19 @@ describe('CollaboratorsController', function () { }) }) + describe('requestAccess with a malformed project id', function () { + it('rejects the request instead of recording it', async function (ctx) { + ctx.req.params = { Project_id: 'not-an-object-id' } + ctx.req.body = { privilegeLevel: 'readAndWrite' } + await ctx.CollaboratorsController.requestAccess( + ctx.req, + ctx.res + ).should.be.rejectedWith(InvalidParamsError) + expect(ctx.CollaboratorsHandler.promises.requestAccess).to.not.have.been + .called + }) + }) + describe('declineAccessRequest', function () { beforeEach(function (ctx) { ctx.requesterId = new ObjectId() @@ -850,6 +960,21 @@ describe('CollaboratorsController', function () { }) }) + describe('declineAccessRequest with a malformed project id', function () { + it('rejects the request instead of declining it', async function (ctx) { + ctx.req.params = { + Project_id: 'not-an-object-id', + user_id: new ObjectId().toString(), + } + await ctx.CollaboratorsController.declineAccessRequest( + ctx.req, + ctx.res + ).should.be.rejectedWith(InvalidParamsError) + expect(ctx.CollaboratorsHandler.promises.declineAccessRequest).to.not.have + .been.called + }) + }) + describe('grantAccessRequest', function () { beforeEach(function (ctx) { ctx.requesterId = new ObjectId() @@ -1006,4 +1131,100 @@ describe('CollaboratorsController', function () { ) }) }) + + describe('grantAccessRequest with a malformed project id', function () { + it('rejects the request instead of granting it', async function (ctx) { + ctx.req.params = { + Project_id: 'not-an-object-id', + user_id: new ObjectId().toString(), + } + ctx.req.body = { privilegeLevel: 'readAndWrite' } + await ctx.CollaboratorsController.grantAccessRequest( + ctx.req, + ctx.res + ).should.be.rejectedWith(InvalidParamsError) + expect(ctx.CollaboratorsHandler.promises.setCollaboratorPrivilegeLevel).to + .not.have.been.called + }) + }) + + describe('getShareTokens', function () { + beforeEach(function (ctx) { + ctx.req.params = { Project_id: ctx.projectId.toString() } + }) + + it('rejects a malformed project id before checking anything else', async function (ctx) { + ctx.req.params = { Project_id: 'not-an-object-id' } + await ctx.CollaboratorsController.getShareTokens( + ctx.req, + ctx.res + ).should.be.rejectedWith(InvalidParamsError) + expect(ctx.CollaboratorsGetter.promises.getPublicShareTokens).to.not.have + .been.called + }) + + it('returns 403 when link sharing is disabled', async function (ctx) { + ctx.Features.hasFeature.withArgs('link-sharing').returns(false) + await new Promise(resolve => { + ctx.res.sendStatus = sinon.spy(() => resolve()) + ctx.CollaboratorsController.getShareTokens(ctx.req, ctx.res) + }) + expect(ctx.res.sendStatus).to.have.been.calledWith(403) + expect(ctx.CollaboratorsGetter.promises.getPublicShareTokens).to.not.have + .been.called + }) + + it('returns 403 when the logged in user has no share tokens', async function (ctx) { + ctx.CollaboratorsGetter.promises.getPublicShareTokens.resolves(null) + await new Promise(resolve => { + ctx.res.sendStatus = sinon.spy(() => resolve()) + ctx.CollaboratorsController.getShareTokens(ctx.req, ctx.res) + }) + expect(ctx.res.sendStatus).to.have.been.calledWith(403) + }) + + it('returns the hashed tokens for a logged in user', async function (ctx) { + ctx.CollaboratorsGetter.promises.getPublicShareTokens.resolves({ + readOnly: 'ro-token', + readAndWrite: 'rw-token', + }) + await new Promise(resolve => { + ctx.res.json = sinon.spy(() => resolve()) + ctx.CollaboratorsController.getShareTokens(ctx.req, ctx.res) + }) + expect( + ctx.CollaboratorsGetter.promises.getPublicShareTokens + ).to.have.been.calledWith( + new ObjectId(ctx.user._id.toString()), + new ObjectId(ctx.projectId.toString()) + ) + expect(ctx.res.json).to.have.been.calledWith({ + readOnly: 'ro-token', + readAndWrite: 'rw-token', + readOnlyHashPrefix: 'hash-prefix', + readAndWriteHashPrefix: 'hash-prefix', + }) + }) + + it('falls back to the session token for an anonymous request', async function (ctx) { + ctx.SessionManager.getLoggedInUserId = sinon.stub().returns(null) + ctx.TokenAccessHandler.getRequestToken = sinon + .stub() + .returns('anon-ro-token') + await new Promise(resolve => { + ctx.res.json = sinon.spy(() => resolve()) + ctx.CollaboratorsController.getShareTokens(ctx.req, ctx.res) + }) + expect(ctx.CollaboratorsGetter.promises.getPublicShareTokens).to.not.have + .been.called + expect(ctx.TokenAccessHandler.getRequestToken).to.have.been.calledWith( + ctx.req, + ctx.projectId.toString() + ) + expect(ctx.res.json).to.have.been.calledWith({ + readOnly: 'anon-ro-token', + readOnlyHashPrefix: 'hash-prefix', + }) + }) + }) }) diff --git a/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterController.test.mjs b/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterController.test.mjs index 1d9af0de06..9e4463339d 100644 --- a/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterController.test.mjs +++ b/services/web/test/unit/src/DocumentUpdater/DocumentUpdaterController.test.mjs @@ -37,7 +37,9 @@ describe('DocumentUpdaterController', function () { ) ctx.controller = (await import(MODULE_PATH)).default - ctx.projectId = '2k3j1lk3j21lk3j' + // Project_id/Doc_id are validated as Mongo ObjectIds + ctx.projectId = '507f191e810c19729de860ea' + ctx.docId = '507f191e810c19729de860eb' ctx.fileId = '12321kklj1lk3jk12' ctx.req = { params: { diff --git a/services/web/test/unit/src/Documents/DocumentController.test.mjs b/services/web/test/unit/src/Documents/DocumentController.test.mjs index 063930f145..0d23633985 100644 --- a/services/web/test/unit/src/Documents/DocumentController.test.mjs +++ b/services/web/test/unit/src/Documents/DocumentController.test.mjs @@ -12,7 +12,7 @@ describe('DocumentController', function () { ctx.res = new MockResponse(vi) ctx.req = new MockRequest(vi) ctx.next = sinon.stub() - ctx.doc = { _id: 'doc-id-123' } + ctx.doc = { _id: '5c9a5d5b0000000000000002' } ctx.doc_lines = ['one', 'two', 'three'] ctx.version = 42 ctx.ranges = { @@ -36,11 +36,15 @@ describe('DocumentController', function () { ], } ctx.pathname = '/a/b/c/file.tex' - ctx.lastUpdatedAt = new Date().getTime() - ctx.lastUpdatedBy = 'fake-last-updater-id' + // stored/read back as a string -- Redis (ioredis mget) always returns + // strings, and document-updater's RedisManager never parses this key + // back to a number, so this is a numeric-looking string on the wire, + // not a JS number. + ctx.lastUpdatedAt = String(new Date().getTime()) + ctx.lastUpdatedBy = '5c9a5d5b0000000000000003' ctx.rev = 5 ctx.project = { - _id: 'project-id-123', + _id: '5c9a5d5b0000000000000001', overleaf: { history: { id: 1234, @@ -212,7 +216,7 @@ describe('DocumentController', function () { ctx.doc_lines, ctx.version, ctx.ranges, - ctx.lastUpdatedAt, + Number(ctx.lastUpdatedAt), ctx.lastUpdatedBy ) }) @@ -237,7 +241,11 @@ describe('DocumentController', function () { ctx.ProjectEntityUpdateHandler.promises.updateDocLines.rejects( new Errors.NotFoundError('document does not exist') ) - ctx.req.body = { lines: ctx.doc_lines } + ctx.req.body = { + lines: ctx.doc_lines, + version: ctx.version, + ranges: ctx.ranges, + } ctx.next.callsFake(() => { resolve() }) diff --git a/services/web/test/unit/src/Downloads/ProjectDownloadsController.test.mjs b/services/web/test/unit/src/Downloads/ProjectDownloadsController.test.mjs index 25688887e4..f2cb39a809 100644 --- a/services/web/test/unit/src/Downloads/ProjectDownloadsController.test.mjs +++ b/services/web/test/unit/src/Downloads/ProjectDownloadsController.test.mjs @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' // TODO: This file was created by bulk-decaffeinate. // Fix any style issues and re-enable lint. /* @@ -10,6 +10,7 @@ import sinon from 'sinon' import MockRequest from '../helpers/MockRequest.mjs' import MockResponse from '../helpers/MockResponse.mjs' import { DocumentConversionError } from '../../../../app/src/Features/Errors/Errors.js' +import { setReqValidationModeForTests } from '@overleaf/validation-tools' vi.mock('../../../../app/src/Features/Errors/Errors.js', () => vi.importActual('../../../../app/src/Features/Errors/Errors.js') @@ -19,7 +20,7 @@ const modulePath = describe('ProjectDownloadsController', function () { beforeEach(async function (ctx) { - ctx.project_id = 'project-id-123' + ctx.project_id = '507f191e810c19729de860ea' ctx.req = new MockRequest(vi) ctx.res = new MockResponse(vi) ctx.next = sinon.stub() @@ -113,6 +114,10 @@ describe('ProjectDownloadsController', function () { ctx.ProjectDownloadsController = (await import(modulePath)).default }) + afterEach(function () { + setReqValidationModeForTests(null) + }) + describe('downloadProject', function () { beforeEach(function (ctx) { ctx.stream = { pipe: sinon.stub() } @@ -194,13 +199,72 @@ describe('ProjectDownloadsController', function () { }) }) + describe('downloadProject with an invalid Project_id', function () { + beforeEach(function (ctx) { + ctx.stream = { pipe: sinon.stub() } + ctx.ProjectZipStreamManager.createZipStreamForProject = sinon + .stub() + .yields(null, ctx.stream) + ctx.req.params = { Project_id: 'not-a-valid-object-id' } + ctx.req.ip = '192.168.1.1' + ctx.req.session = { + user: { + _id: 'user-id-123', + email: 'user@example.com', + }, + } + ctx.ProjectGetter.getProject = sinon.stub().callsArgWith(2, null, { + name: 'a project', + overleaf: { history: { id: 123 } }, + }) + ctx.DocumentUpdaterHandler.flushProjectToMongo = sinon + .stub() + .callsArgWith(1) + }) + + it('should still process the request in log mode', function (ctx) { + setReqValidationModeForTests('log') + ctx.ProjectDownloadsController.downloadProject(ctx.req, ctx.res, ctx.next) + sinon.assert.notCalled(ctx.next) + sinon.assert.calledWith( + ctx.ProjectZipStreamManager.createZipStreamForProject, + 'not-a-valid-object-id' + ) + }) + + describe('when enforced', function () { + afterEach(function () { + setReqValidationModeForTests(null) + }) + + it('should reject the request', function (ctx) { + setReqValidationModeForTests('enforce') + let error + try { + ctx.ProjectDownloadsController.downloadProject( + ctx.req, + ctx.res, + ctx.next + ) + } catch (err) { + error = err + } + expect(error).to.exist + expect(error.name).to.equal('InvalidParamsError') + sinon.assert.notCalled( + ctx.ProjectZipStreamManager.createZipStreamForProject + ) + }) + }) + }) + describe('downloadMultipleProjects', function () { beforeEach(function (ctx) { ctx.stream = { pipe: sinon.stub() } ctx.ProjectZipStreamManager.createZipStreamForMultipleProjects = sinon .stub() .yields(null, ctx.stream) - ctx.project_ids = ['project-1', 'project-2'] + ctx.project_ids = ['507f191e810c19729de860ea', '507f191e810c19729de860eb'] ctx.req.query = { project_ids: ctx.project_ids.join(',') } ctx.req.ip = '192.168.1.1' ctx.req.session = { @@ -273,6 +337,67 @@ describe('ProjectDownloadsController', function () { }) }) + describe('downloadMultipleProjects with an invalid project id', function () { + beforeEach(function (ctx) { + ctx.stream = { pipe: sinon.stub() } + ctx.ProjectZipStreamManager.createZipStreamForMultipleProjects = sinon + .stub() + .yields(null, ctx.stream) + ctx.req.query = { + project_ids: 'not-a-valid-object-id,507f191e810c19729de860eb', + } + ctx.req.ip = '192.168.1.1' + ctx.req.session = { + user: { + _id: 'user-id-123', + email: 'user@example.com', + }, + } + ctx.DocumentUpdaterHandler.flushMultipleProjectsToMongo = sinon + .stub() + .callsArgWith(1) + }) + + it('should still process the request via the fallback schema in log mode', function (ctx) { + setReqValidationModeForTests('log') + ctx.ProjectDownloadsController.downloadMultipleProjects( + ctx.req, + ctx.res, + ctx.next + ) + sinon.assert.notCalled(ctx.next) + sinon.assert.calledWith( + ctx.ProjectZipStreamManager.createZipStreamForMultipleProjects, + ['not-a-valid-object-id', '507f191e810c19729de860eb'] + ) + }) + + describe('when enforced', function () { + afterEach(function () { + setReqValidationModeForTests(null) + }) + + it('should reject the request', function (ctx) { + setReqValidationModeForTests('enforce') + let error + try { + ctx.ProjectDownloadsController.downloadMultipleProjects( + ctx.req, + ctx.res, + ctx.next + ) + } catch (err) { + error = err + } + expect(error).to.exist + expect(error.name).to.equal('InvalidRequestError') + sinon.assert.notCalled( + ctx.ProjectZipStreamManager.createZipStreamForMultipleProjects + ) + }) + }) + }) + describe('exportProjectConversion', function () { describe('with a supported type (default streaming)', function () { beforeEach(async function (ctx) { diff --git a/services/web/test/unit/src/Editor/EditorHttpController.test.mjs b/services/web/test/unit/src/Editor/EditorHttpController.test.mjs index bae4070ebc..51c41564a3 100644 --- a/services/web/test/unit/src/Editor/EditorHttpController.test.mjs +++ b/services/web/test/unit/src/Editor/EditorHttpController.test.mjs @@ -51,7 +51,8 @@ describe('EditorHttpController', function () { ctx.folder = { _id: new ObjectId() } ctx.source = 'editor' - ctx.parentFolderId = 'mock-folder-id' + // parent_folder_id/entity_id/folder_id are all validated as Mongo ObjectIds + ctx.parentFolderId = new ObjectId().toString() ctx.req = new MockRequest(vi) ctx.res = new MockResponse(vi) ctx.next = sinon.stub() @@ -526,7 +527,7 @@ describe('EditorHttpController', function () { describe('addDoc', function () { beforeEach(function (ctx) { - ctx.req.params = { Project_id: ctx.project._id } + ctx.req.params = { Project_id: ctx.project._id.toString() } ctx.req.body = { name: (ctx.docName = 'doc-name'), parent_folder_id: ctx.parentFolderId, @@ -543,7 +544,7 @@ describe('EditorHttpController', function () { it('should call EditorController.addDoc', function (ctx) { expect(ctx.EditorController.promises.addDoc).to.have.been.calledWith( - ctx.project._id, + ctx.project._id.toString(), ctx.parentFolderId, ctx.docName, [], @@ -588,7 +589,7 @@ describe('EditorHttpController', function () { describe('addFolder', function () { beforeEach(function (ctx) { ctx.folderName = 'folder-name' - ctx.req.params = { Project_id: ctx.project._id } + ctx.req.params = { Project_id: ctx.project._id.toString() } ctx.req.body = { name: ctx.folderName, parent_folder_id: ctx.parentFolderId, @@ -605,7 +606,7 @@ describe('EditorHttpController', function () { it('should call EditorController.addFolder', function (ctx) { expect(ctx.EditorController.promises.addFolder).to.have.been.calledWith( - ctx.project._id, + ctx.project._id.toString(), ctx.parentFolderId, ctx.folderName, 'editor' @@ -661,10 +662,11 @@ describe('EditorHttpController', function () { describe('renameEntity', function () { beforeEach(function (ctx) { - ctx.entityId = 'entity-id-123' - ctx.entityType = 'entity-type' + // entity_id is validated as a Mongo ObjectId; entity_type as an enum + ctx.entityId = new ObjectId().toString() + ctx.entityType = 'doc' ctx.req.params = { - Project_id: ctx.project._id, + Project_id: ctx.project._id.toString(), entity_id: ctx.entityId, entity_type: ctx.entityType, } @@ -684,7 +686,7 @@ describe('EditorHttpController', function () { expect( ctx.EditorController.promises.renameEntity ).to.have.been.calledWith( - ctx.project._id, + ctx.project._id.toString(), ctx.entityId, ctx.entityType, ctx.newName, @@ -725,11 +727,13 @@ describe('EditorHttpController', function () { describe('moveEntity', function () { beforeEach(async function (ctx) { await new Promise(resolve => { - ctx.entityId = 'entity-id-123' - ctx.entityType = 'entity-type' - ctx.folderId = 'folder-id-123' + // entity_id/folder_id are validated as Mongo ObjectIds; entity_type + // as an enum + ctx.entityId = new ObjectId().toString() + ctx.entityType = 'doc' + ctx.folderId = new ObjectId().toString() ctx.req.params = { - Project_id: ctx.project._id, + Project_id: ctx.project._id.toString(), entity_id: ctx.entityId, entity_type: ctx.entityType, } @@ -741,7 +745,7 @@ describe('EditorHttpController', function () { it('should call EditorController.moveEntity', function (ctx) { expect(ctx.EditorController.promises.moveEntity).to.have.been.calledWith( - ctx.project._id, + ctx.project._id.toString(), ctx.entityId, ctx.folderId, ctx.entityType, @@ -755,35 +759,40 @@ describe('EditorHttpController', function () { }) }) - describe('deleteEntity', function () { - beforeEach(async function (ctx) { - await new Promise(resolve => { - ctx.entityId = 'entity-id-123' - ctx.entityType = 'entity-type' - ctx.req.params = { - Project_id: ctx.project._id, - entity_id: ctx.entityId, - entity_type: ctx.entityType, - } - ctx.res.callback = resolve - ctx.EditorHttpController.deleteEntity(ctx.req, ctx.res) + for (const [name, entityType] of [ + ['deleteDoc', 'doc'], + ['deleteFile', 'file'], + ['deleteFolder', 'folder'], + ]) { + describe(name, function () { + beforeEach(async function (ctx) { + await new Promise(resolve => { + // entity_id is validated as a Mongo ObjectId + ctx.entityId = new ObjectId().toString() + ctx.req.params = { + Project_id: ctx.project._id.toString(), + entity_id: ctx.entityId, + } + ctx.res.callback = resolve + ctx.EditorHttpController[name](ctx.req, ctx.res) + }) + }) + + it('should call EditorController.deleteEntity', function (ctx) { + expect( + ctx.EditorController.promises.deleteEntity + ).to.have.been.calledWith( + ctx.project._id.toString(), + ctx.entityId, + entityType, + 'editor', + ctx.user._id + ) + }) + + it('should send back a success response', function (ctx) { + expect(ctx.res.statusCode).to.equal(204) }) }) - - it('should call EditorController.deleteEntity', function (ctx) { - expect( - ctx.EditorController.promises.deleteEntity - ).to.have.been.calledWith( - ctx.project._id, - ctx.entityId, - ctx.entityType, - 'editor', - ctx.user._id - ) - }) - - it('should send back a success response', function (ctx) { - expect(ctx.res.statusCode).to.equal(204) - }) - }) + } }) diff --git a/services/web/test/unit/src/History/HistoryController.test.mjs b/services/web/test/unit/src/History/HistoryController.test.mjs index 9e38d16402..d6ffd065c8 100644 --- a/services/web/test/unit/src/History/HistoryController.test.mjs +++ b/services/web/test/unit/src/History/HistoryController.test.mjs @@ -201,6 +201,25 @@ describe('HistoryController', function () { }) }) + describe('proxyToHistoryApi (with an invalid request)', function () { + beforeEach(async function (ctx) { + ctx.req = { + url: '/mock/url', + method: 'GET', + session: sinon.stub(), + params: { Project_id: 'not-an-object-id' }, + } + ctx.res = { set: sinon.stub() } + await ctx.HistoryController.proxyToHistoryApi(ctx.req, ctx.res, ctx.next) + }) + + it('rejects the request without calling the history api', function (ctx) { + ctx.next.should.have.been.calledOnce + ctx.next.firstCall.args[0].should.be.an.instanceof(Error) + ctx.fetchStreamWithResponse.should.not.have.been.called + }) + }) + describe('proxyToHistoryApiAndInjectUserDetails', function () { beforeEach(async function (ctx) { ctx.req = { @@ -280,6 +299,28 @@ describe('HistoryController', function () { }) }) + describe('proxyToHistoryApiAndInjectUserDetails (with an invalid request)', function () { + beforeEach(async function (ctx) { + ctx.req = { + url: '/mock/url', + method: 'GET', + params: { Project_id: 'not-an-object-id' }, + } + ctx.res = { json: sinon.stub() } + await ctx.HistoryController.proxyToHistoryApiAndInjectUserDetails( + ctx.req, + ctx.res, + ctx.next + ) + }) + + it('rejects the request without calling the history api', function (ctx) { + ctx.next.should.have.been.calledOnce + ctx.next.firstCall.args[0].should.be.an.instanceof(Error) + ctx.fetchJson.should.not.have.been.called + }) + }) + describe('resyncProjectHistory', function () { describe('for a project without project-history enabled', function () { beforeEach(async function (ctx) { @@ -329,6 +370,29 @@ describe('HistoryController', function () { ctx.res.sendStatus.should.have.been.calledWith(204) }) }) + + describe('with an invalid historyRangesMigration value', function () { + beforeEach(async function (ctx) { + ctx.req = { + params: { Project_id: ctx.project_id }, + body: { historyRangesMigration: 'sideways' }, + } + ctx.res = { setTimeout: sinon.stub(), sendStatus: sinon.stub() } + + await ctx.HistoryController.resyncProjectHistory( + ctx.req, + ctx.res, + ctx.next + ) + }) + + it('rejects the request without resyncing', function (ctx) { + ctx.next.should.have.been.calledOnce + ctx.next.firstCall.args[0].should.be.an.instanceof(Error) + ctx.ProjectEntityUpdateHandler.promises.resyncProjectHistory.should.not + .have.been.called + }) + }) }) describe('restoreFileFromV2', function () { @@ -490,6 +554,106 @@ describe('HistoryController', function () { }) }) + describe('getLabels', function () { + beforeEach(function (ctx) { + ctx.res = { json: sinon.stub() } + ctx.labels = [{ id: 'label-1', comment: 'a label' }] + ctx.fetchJson.resolves(ctx.labels) + }) + + it('fetches and returns the labels for a valid request', async function (ctx) { + ctx.req = { params: { Project_id: ctx.project_id } } + await ctx.HistoryController.getLabels(ctx.req, ctx.res, ctx.next) + ctx.fetchJson.should.have.been.calledWith( + `${ctx.settings.apis.project_history.url}/project/${ctx.project_id}/labels` + ) + ctx.res.json.should.have.been.calledWith(ctx.labels) + }) + + it('rejects a malformed Project_id without calling the history api', async function (ctx) { + ctx.req = { params: { Project_id: 'not-an-object-id' } } + await ctx.HistoryController.getLabels(ctx.req, ctx.res, ctx.next) + ctx.next.should.have.been.calledOnce + ctx.next.firstCall.args[0].should.be.an.instanceof(Error) + ctx.fetchJson.should.not.have.been.called + }) + }) + + describe('createLabel', function () { + beforeEach(function (ctx) { + ctx.res = { json: sinon.stub() } + }) + + it('creates and returns the label for a valid request', async function (ctx) { + ctx.req = { + params: { Project_id: ctx.project_id }, + body: { comment: 'a label', version: 3 }, + session: {}, + } + ctx.label = { id: 'label-1', comment: 'a label', version: 3 } + ctx.fetchJson.resolves(ctx.label) + await ctx.HistoryController.createLabel(ctx.req, ctx.res, ctx.next) + ctx.fetchJson.should.have.been.calledWith( + `${ctx.settings.apis.project_history.url}/project/${ctx.project_id}/labels`, + { + method: 'POST', + json: { comment: 'a label', version: 3, user_id: ctx.user_id }, + } + ) + ctx.res.json.should.have.been.calledWith( + sinon.match({ comment: 'a label', version: 3 }) + ) + }) + + it('rejects a negative version without calling the history api', async function (ctx) { + ctx.req = { + params: { Project_id: ctx.project_id }, + body: { comment: 'a label', version: -1 }, + session: {}, + } + await ctx.HistoryController.createLabel(ctx.req, ctx.res, ctx.next) + ctx.next.should.have.been.calledOnce + ctx.next.firstCall.args[0].should.be.an.instanceof(Error) + ctx.fetchJson.should.not.have.been.called + }) + }) + + describe('deleteLabel', function () { + beforeEach(function (ctx) { + ctx.res = { sendStatus: sinon.stub() } + ctx.labelId = '000000000000000087654321' + ctx.ProjectGetter.promises = { + getProject: sinon.stub().resolves({ + owner_ref: { equals: sinon.stub().returns(true) }, + }), + } + }) + + it('deletes the label for a valid request', async function (ctx) { + ctx.req = { + params: { Project_id: ctx.project_id, label_id: ctx.labelId }, + session: {}, + } + await ctx.HistoryController.deleteLabel(ctx.req, ctx.res, ctx.next) + ctx.fetchNothing.should.have.been.calledWith( + `${ctx.settings.apis.project_history.url}/project/${ctx.project_id}/labels/${ctx.labelId}`, + { method: 'DELETE' } + ) + ctx.res.sendStatus.should.have.been.calledWith(204) + }) + + it('rejects a malformed label_id without calling the history api', async function (ctx) { + ctx.req = { + params: { Project_id: ctx.project_id, label_id: 'not-an-object-id' }, + session: {}, + } + await ctx.HistoryController.deleteLabel(ctx.req, ctx.res, ctx.next) + ctx.next.should.have.been.calledOnce + ctx.next.firstCall.args[0].should.be.an.instanceof(Error) + ctx.fetchNothing.should.not.have.been.called + }) + }) + describe('requestBlob', function () { describe('With Range header', function () { beforeEach(async function (ctx) { diff --git a/services/web/test/unit/src/Metadata/MetaController.test.mjs b/services/web/test/unit/src/Metadata/MetaController.test.mjs index 923ce2d4cc..ee5b0852b1 100644 --- a/services/web/test/unit/src/Metadata/MetaController.test.mjs +++ b/services/web/test/unit/src/Metadata/MetaController.test.mjs @@ -1,8 +1,12 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import sinon from 'sinon' import MockResponse from '../helpers/MockResponse.mjs' +import { setReqValidationModeForTests } from '@overleaf/validation-tools' const modulePath = '../../../../app/src/Features/Metadata/MetaController.mjs' +const PROJECT_ID = '507f1f77bcf86cd799439011' +const DOC_ID = '507f191e810c19729de860ea' + describe('MetaController', function () { beforeEach(async function (ctx) { ctx.EditorRealTimeController = { @@ -46,6 +50,10 @@ describe('MetaController', function () { ctx.MetadataController = (await import(modulePath)).default }) + afterEach(function () { + setReqValidationModeForTests(null) + }) + describe('getMetadata', function () { it('should respond with json', async function (ctx) { const projectMeta = { @@ -60,18 +68,18 @@ describe('MetaController', function () { .stub() .resolves(projectMeta) - const req = { params: { project_id: 'project-id' } } + const req = { params: { project_id: PROJECT_ID } } const res = new MockResponse(vi) const next = sinon.stub() await ctx.MetadataController.getMetadata(req, res, next) ctx.MetaHandler.promises.getAllMetaForProject.should.have.been.calledWith( - 'project-id' + PROJECT_ID ) expect(res.json).toHaveBeenCalledTimes(1) expect(res.json).toHaveBeenCalledWith({ - projectId: 'project-id', + projectId: PROJECT_ID, projectMeta, }) next.should.not.have.been.called @@ -82,18 +90,33 @@ describe('MetaController', function () { .stub() .throws(new Error('woops')) - const req = { params: { project_id: 'project-id' } } + const req = { params: { project_id: PROJECT_ID } } const res = new MockResponse(vi) const next = sinon.stub() await ctx.MetadataController.getMetadata(req, res, next) ctx.MetaHandler.promises.getAllMetaForProject.should.have.been.calledWith( - 'project-id' + PROJECT_ID ) expect(res.json).not.toHaveBeenCalled() next.should.have.been.calledWithMatch(error => error instanceof Error) }) + + it('should reject a malformed project id', async function (ctx) { + const req = { params: { project_id: 'not-an-object-id' } } + const res = new MockResponse(vi) + const next = sinon.stub() + + await ctx.MetadataController.getMetadata(req, res, next) + + expect(ctx.MetaHandler.promises.getAllMetaForProject.called).to.equal( + false + ) + next.should.have.been.calledWithMatch( + error => error.name === 'InvalidParamsError' + ) + }) }) describe('broadcastMetadataForDoc', function () { @@ -107,7 +130,7 @@ describe('MetaController', function () { ctx.EditorRealTimeController.emitToRoom = sinon.stub() const req = { - params: { project_id: 'project-id', doc_id: 'doc-id' }, + params: { project_id: PROJECT_ID, doc_id: DOC_ID }, body: { broadcast: true }, } const res = new MockResponse(vi) @@ -116,7 +139,7 @@ describe('MetaController', function () { await ctx.MetadataController.broadcastMetadataForDoc(req, res, next) ctx.MetaHandler.promises.getMetaForDoc.should.have.been.calledWith( - 'project-id' + PROJECT_ID ) expect(res.json).not.toHaveBeenCalled() expect(res.sendStatus).toHaveBeenCalledTimes(1) @@ -125,7 +148,7 @@ describe('MetaController', function () { ctx.EditorRealTimeController.emitToRoom.should.have.been.calledOnce const { lastCall } = ctx.EditorRealTimeController.emitToRoom - expect(lastCall.args[0]).to.equal('project-id') + expect(lastCall.args[0]).to.equal(PROJECT_ID) expect(lastCall.args[1]).to.equal('broadcastDocMeta') expect(lastCall.args[2]).to.have.all.keys(['docId', 'meta']) }) @@ -142,7 +165,7 @@ describe('MetaController', function () { ctx.EditorRealTimeController.emitToRoom = sinon.stub() const req = { - params: { project_id: 'project-id', doc_id: 'doc-id' }, + params: { project_id: PROJECT_ID, doc_id: DOC_ID }, body: { broadcast: false }, } const res = new MockResponse(vi) @@ -151,12 +174,12 @@ describe('MetaController', function () { await ctx.MetadataController.broadcastMetadataForDoc(req, res, next) ctx.MetaHandler.promises.getMetaForDoc.should.have.been.calledWith( - 'project-id' + PROJECT_ID ) ctx.EditorRealTimeController.emitToRoom.should.not.have.been.called expect(res.json).toHaveBeenCalledTimes(1) expect(res.json).toHaveBeenCalledWith({ - docId: 'doc-id', + docId: DOC_ID, meta: docMeta, }) next.should.not.have.been.called @@ -170,7 +193,7 @@ describe('MetaController', function () { ctx.EditorRealTimeController.emitToRoom = sinon.stub() const req = { - params: { project_id: 'project-id', doc_id: 'doc-id' }, + params: { project_id: PROJECT_ID, doc_id: DOC_ID }, body: { broadcast: true }, } const res = new MockResponse(vi) @@ -179,10 +202,48 @@ describe('MetaController', function () { await ctx.MetadataController.broadcastMetadataForDoc(req, res, next) ctx.MetaHandler.promises.getMetaForDoc.should.have.been.calledWith( - 'project-id' + PROJECT_ID ) expect(res.json).not.toHaveBeenCalled() next.should.have.been.calledWithMatch(error => error instanceof Error) }) + + describe('request validation', function () { + beforeEach(function () { + setReqValidationModeForTests('enforce') + }) + + it('should reject a malformed doc id', async function (ctx) { + const req = { + params: { project_id: PROJECT_ID, doc_id: 'not-an-object-id' }, + body: { broadcast: true }, + } + const res = new MockResponse(vi) + const next = sinon.stub() + + await ctx.MetadataController.broadcastMetadataForDoc(req, res, next) + + expect(ctx.MetaHandler.promises.getMetaForDoc.called).to.equal(false) + next.should.have.been.calledWithMatch( + error => error.name === 'InvalidParamsError' + ) + }) + + it('should reject an unrecognized body field', async function (ctx) { + const req = { + params: { project_id: PROJECT_ID, doc_id: DOC_ID }, + body: { broadcast: true, evil: 'x' }, + } + const res = new MockResponse(vi) + const next = sinon.stub() + + await ctx.MetadataController.broadcastMetadataForDoc(req, res, next) + + expect(ctx.MetaHandler.promises.getMetaForDoc.called).to.equal(false) + next.should.have.been.calledWithMatch( + error => error.name === 'InvalidRequestError' + ) + }) + }) }) }) diff --git a/services/web/test/unit/src/Notifications/NotificationsController.test.mjs b/services/web/test/unit/src/Notifications/NotificationsController.test.mjs index f5e88965c3..2a43ca6988 100644 --- a/services/web/test/unit/src/Notifications/NotificationsController.test.mjs +++ b/services/web/test/unit/src/Notifications/NotificationsController.test.mjs @@ -1,5 +1,6 @@ -import { beforeEach, describe, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, it, vi } from 'vitest' import sinon from 'sinon' +import { setReqValidationModeForTests } from '@overleaf/validation-tools' const modulePath = new URL( '../../../../app/src/Features/Notifications/NotificationsController.mjs', @@ -8,7 +9,7 @@ const modulePath = new URL( describe('NotificationsController', function () { const userId = '123nd3ijdks' - const notificationId = '123njdskj9jlk' + const notificationId = '507f1f77bcf86cd799439011' beforeEach(async function (ctx) { ctx.handler = { @@ -52,6 +53,10 @@ describe('NotificationsController', function () { ctx.controller = (await import(modulePath)).default }) + afterEach(function () { + setReqValidationModeForTests(null) + }) + it('should ask the handler for all unread notifications', async function (ctx) { await new Promise(resolve => { const allNotifications = [{ _id: notificationId, user_id: userId }] @@ -98,4 +103,26 @@ describe('NotificationsController', function () { }) }) }) + + describe('request validation', function () { + beforeEach(function () { + setReqValidationModeForTests('enforce') + }) + + it('should reject a malformed notification id on markNotificationAsRead', function (ctx) { + ctx.req.params.notificationId = 'not-an-object-id' + ;(() => + ctx.controller.markNotificationAsRead(ctx.req, { + sendStatus: sinon.stub(), + })).should.throw() + }) + + it('should reject a malformed notification id on getNotification', function (ctx) { + ctx.req.params.notificationId = 'not-an-object-id' + ;(() => + ctx.controller.getNotification(ctx.req, { + json: sinon.stub(), + })).should.throw() + }) + }) }) diff --git a/services/web/test/unit/src/Project/ProjectApiController.test.mjs b/services/web/test/unit/src/Project/ProjectApiController.test.mjs index 2fb44507de..e1a522b933 100644 --- a/services/web/test/unit/src/Project/ProjectApiController.test.mjs +++ b/services/web/test/unit/src/Project/ProjectApiController.test.mjs @@ -1,5 +1,6 @@ -import { beforeEach, describe, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import sinon from 'sinon' +import { setReqValidationModeForTests } from '@overleaf/validation-tools' const modulePath = '../../../../app/src/Features/Project/ProjectApiController' @@ -15,7 +16,7 @@ describe('Project api controller', function () { ) ctx.controller = (await import(modulePath)).default - ctx.project_id = '321l3j1kjkjl' + ctx.project_id = '507f191e810c19729de860ea' ctx.req = { params: { project_id: ctx.project_id, @@ -29,6 +30,10 @@ describe('Project api controller', function () { return (ctx.projDetails = { name: 'something' }) }) + afterEach(function () { + setReqValidationModeForTests(null) + }) + describe('getProjectDetails', function () { it('should ask the project details handler for proj details', async function (ctx) { await new Promise(resolve => { @@ -53,5 +58,46 @@ describe('Project api controller', function () { ctx.controller.getProjectDetails(ctx.req, ctx.res, ctx.next) return ctx.next.calledWith('error').should.equal(true) }) + + describe('with a malformed project_id', function () { + beforeEach(function (ctx) { + ctx.req.params.project_id = 'not-a-valid-object-id' + }) + + it('should still process the request in log mode', function (ctx) { + setReqValidationModeForTests('log') + ctx.ProjectDetailsHandler.getDetails.callsArgWith( + 1, + null, + ctx.projDetails + ) + ctx.res.json = sinon.stub() + ctx.controller.getProjectDetails(ctx.req, ctx.res, ctx.next) + sinon.assert.notCalled(ctx.next) + sinon.assert.calledWith( + ctx.ProjectDetailsHandler.getDetails, + 'not-a-valid-object-id' + ) + }) + + describe('when enforced', function () { + afterEach(function () { + setReqValidationModeForTests(null) + }) + + it('should reject the request', function (ctx) { + setReqValidationModeForTests('enforce') + let error + try { + ctx.controller.getProjectDetails(ctx.req, ctx.res, ctx.next) + } catch (err) { + error = err + } + expect(error).to.exist + expect(error.name).to.equal('InvalidParamsError') + sinon.assert.notCalled(ctx.ProjectDetailsHandler.getDetails) + }) + }) + }) }) }) diff --git a/services/web/test/unit/src/Project/ProjectController.test.mjs b/services/web/test/unit/src/Project/ProjectController.test.mjs index 1e7cb8e496..f60f26bd5b 100644 --- a/services/web/test/unit/src/Project/ProjectController.test.mjs +++ b/services/web/test/unit/src/Project/ProjectController.test.mjs @@ -59,6 +59,8 @@ describe('ProjectController', function () { promises: { deleteProject: sinon.stub().resolves(), restoreProject: sinon.stub().resolves(), + archiveProject: sinon.stub().resolves(), + unarchiveProject: sinon.stub().resolves(), }, findArchivedProjects: sinon.stub(), } @@ -543,7 +545,8 @@ describe('ProjectController', function () { ctx.req = { query: {}, params: { - Project_id: ctx.project_id, + // real Express req.params are always strings + Project_id: ctx.project_id.toString(), }, headers: {}, connection: { @@ -652,6 +655,16 @@ describe('ProjectController', function () { ctx.ProjectController.updateProjectSettings(ctx.req, ctx.res) }) }) + + it('should reject a malformed mainBibliographyDocId', async function (ctx) { + await new Promise(resolve => { + ctx.req.body = { mainBibliographyDocId: 'not-an-object-id' } + ctx.ProjectController.updateProjectSettings(ctx.req, ctx.res, err => { + expect(err).to.exist + resolve() + }) + }) + }) }) describe('updateProjectAdminSettings', function () { @@ -733,7 +746,7 @@ describe('ProjectController', function () { await new Promise(resolve => { ctx.res.sendStatus = code => { ctx.ProjectDeleter.promises.deleteProject - .calledWith(ctx.project_id, { + .calledWith(ctx.project_id.toString(), { deleterUser: ctx.user, ipAddress: ctx.req.ip, deletedReason: 'user', @@ -747,12 +760,42 @@ describe('ProjectController', function () { }) }) + describe('archiveProject', function () { + it('should call the project deleter', async function (ctx) { + await new Promise(resolve => { + ctx.res.sendStatus = code => { + ctx.ProjectDeleter.promises.archiveProject + .calledWith(ctx.project_id.toString(), ctx.user._id) + .should.equal(true) + code.should.equal(200) + resolve() + } + ctx.ProjectController.archiveProject(ctx.req, ctx.res) + }) + }) + }) + + describe('unarchiveProject', function () { + it('should call the project deleter', async function (ctx) { + await new Promise(resolve => { + ctx.res.sendStatus = code => { + ctx.ProjectDeleter.promises.unarchiveProject + .calledWith(ctx.project_id.toString(), ctx.user._id) + .should.equal(true) + code.should.equal(200) + resolve() + } + ctx.ProjectController.unarchiveProject(ctx.req, ctx.res) + }) + }) + }) + describe('restoreProject', function () { it('should tell the project deleter', async function (ctx) { await new Promise(resolve => { ctx.res.sendStatus = code => { ctx.ProjectDeleter.promises.restoreProject - .calledWith(ctx.project_id) + .calledWith(ctx.project_id.toString()) .should.equal(true) code.should.equal(200) resolve() @@ -767,7 +810,7 @@ describe('ProjectController', function () { await new Promise(resolve => { ctx.res.json = json => { ctx.ProjectDuplicator.promises.duplicate - .calledWith(ctx.user, ctx.project_id, ctx.projectName) + .calledWith(ctx.user, ctx.project_id.toString(), ctx.projectName) .should.equal(true) json.project_id.should.equal(ctx.project_id) resolve() @@ -873,7 +916,7 @@ describe('ProjectController', function () { describe('renameProject', function () { beforeEach(function (ctx) { ctx.newProjectName = 'my supper great new project' - ctx.req.body.newProjectName = ctx.newProjectName + ctx.req.body = { newProjectName: ctx.newProjectName } ctx.req.params.Project_id = ctx.project_id.toString() }) @@ -1101,7 +1144,7 @@ describe('ProjectController', function () { resCode.should.equal(401) ctx.AuthorizationManager.promises.getPrivilegeLevelForProject.should.have.been.calledWith( ctx.user._id, - ctx.project_id, + ctx.project_id.toString(), 'some-token' ) resolve() @@ -1140,7 +1183,7 @@ describe('ProjectController', function () { await new Promise(resolve => { ctx.res.render = (pageName, opts) => { ctx.ProjectUpdateHandler.promises.markAsOpened - .calledWith(ctx.project_id) + .calledWith(ctx.project_id.toString()) .should.equal(true) resolve() } @@ -1357,13 +1400,13 @@ describe('ProjectController', function () { }) describe('when the projectId does not match (0)', function () { beforeEach(function (ctx) { - ctx.req.params.Project_id = ObjectId.createFromTime(0) + ctx.req.params.Project_id = ObjectId.createFromTime(0).toString() }) checkNonMatch() }) describe('when the projectId does not match (42)', function () { beforeEach(function (ctx) { - ctx.req.params.Project_id = ObjectId.createFromTime(42) + ctx.req.params.Project_id = ObjectId.createFromTime(42).toString() }) checkNonMatch() }) @@ -1375,21 +1418,21 @@ describe('ProjectController', function () { }) describe('when the projectId matches (0)', function () { beforeEach(function (ctx) { - ctx.req.params.Project_id = ObjectId.createFromTime(0) + ctx.req.params.Project_id = ObjectId.createFromTime(0).toString() }) checkMatch() checkForBetaUser() }) describe('when the projectId does not match (1)', function () { beforeEach(function (ctx) { - ctx.req.params.Project_id = ObjectId.createFromTime(1) + ctx.req.params.Project_id = ObjectId.createFromTime(1).toString() }) checkNonMatch() checkForBetaUser() }) describe('when the projectId does not match (42)', function () { beforeEach(function (ctx) { - ctx.req.params.Project_id = ObjectId.createFromTime(42) + ctx.req.params.Project_id = ObjectId.createFromTime(42).toString() }) checkNonMatch() }) @@ -1400,26 +1443,26 @@ describe('ProjectController', function () { }) describe('when the projectId matches (0)', function () { beforeEach(function (ctx) { - ctx.req.params.Project_id = ObjectId.createFromTime(0) + ctx.req.params.Project_id = ObjectId.createFromTime(0).toString() }) checkMatch() }) describe('when the projectId matches (9)', function () { beforeEach(function (ctx) { - ctx.req.params.Project_id = ObjectId.createFromTime(9) + ctx.req.params.Project_id = ObjectId.createFromTime(9).toString() }) checkMatch() checkForBetaUser() }) describe('when the projectId does not match (10)', function () { beforeEach(function (ctx) { - ctx.req.params.Project_id = ObjectId.createFromTime(10) + ctx.req.params.Project_id = ObjectId.createFromTime(10).toString() }) checkNonMatch() }) describe('when the projectId does not match (42)', function () { beforeEach(function (ctx) { - ctx.req.params.Project_id = ObjectId.createFromTime(42) + ctx.req.params.Project_id = ObjectId.createFromTime(42).toString() }) checkNonMatch() checkForBetaUser() @@ -1431,26 +1474,26 @@ describe('ProjectController', function () { }) describe('when the projectId matches (0)', function () { beforeEach(function (ctx) { - ctx.req.params.Project_id = ObjectId.createFromTime(0) + ctx.req.params.Project_id = ObjectId.createFromTime(0).toString() }) checkMatch() checkForBetaUser() }) describe('when the projectId matches (10)', function () { beforeEach(function (ctx) { - ctx.req.params.Project_id = ObjectId.createFromTime(10) + ctx.req.params.Project_id = ObjectId.createFromTime(10).toString() }) checkMatch() }) describe('when the projectId matches (42)', function () { beforeEach(function (ctx) { - ctx.req.params.Project_id = ObjectId.createFromTime(42) + ctx.req.params.Project_id = ObjectId.createFromTime(42).toString() }) checkMatch() }) describe('when the projectId matches (99)', function () { beforeEach(function (ctx) { - ctx.req.params.Project_id = ObjectId.createFromTime(99) + ctx.req.params.Project_id = ObjectId.createFromTime(99).toString() }) checkMatch() }) @@ -1570,7 +1613,7 @@ describe('ProjectController', function () { ctx.res.render = (pageName, opts) => { ctx.Modules.promises.hooks.fire.should.have.been.calledWith( 'enforceCollaboratorLimit', - ctx.project_id + ctx.project_id.toString() ) resolve() } @@ -1928,8 +1971,8 @@ describe('ProjectController', function () { describe('projectEntitiesJson', function () { beforeEach(function (ctx) { ctx.SessionManager.getLoggedInUserId = sinon.stub().returns('abc') - ctx.req.params = { Project_id: 'abcd' } - ctx.project = { _id: 'abcd' } + ctx.req.params = { Project_id: '507f191e810c19729de860ea' } + ctx.project = { _id: '507f191e810c19729de860ea' } ctx.docs = [ { path: '/things/b.txt', doc: true }, { path: '/main.tex', doc: true }, @@ -1945,7 +1988,7 @@ describe('ProjectController', function () { await new Promise(resolve => { ctx.res.json = data => { expect(data).to.deep.equal({ - project_id: 'abcd', + project_id: '507f191e810c19729de860ea', entities: [ { path: '/main.tex', type: 'doc' }, { path: '/things/a.txt', type: 'file' }, diff --git a/services/web/test/unit/src/Project/ProjectListController.test.mjs b/services/web/test/unit/src/Project/ProjectListController.test.mjs index 780c3de46a..c0dbe4e64f 100644 --- a/services/web/test/unit/src/Project/ProjectListController.test.mjs +++ b/services/web/test/unit/src/Project/ProjectListController.test.mjs @@ -1,8 +1,12 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import sinon from 'sinon' import mongodb from 'mongodb-legacy' import Errors from '../../../../app/src/Features/Errors/Errors.js' import Settings from '@overleaf/settings' +import { + InvalidRequestError, + setReqValidationModeForTests, +} from '@overleaf/validation-tools' const ObjectId = mongodb.ObjectId @@ -335,6 +339,10 @@ describe('ProjectListController', function () { ctx.res = {} }) + afterEach(function () { + setReqValidationModeForTests(null) + }) + describe('projectListPage', function () { beforeEach(function (ctx) { ctx.projects = [ @@ -1319,4 +1327,110 @@ describe('ProjectListController', function () { await ctx.ProjectListController.projectListPage(ctx.req, ctx.res) }) }) + + describe('getProjectsJson', function () { + beforeEach(function (ctx) { + ctx.projects = [ + { _id: 1, lastUpdated: new Date(1), owner_ref: 'user-1' }, + { _id: 2, lastUpdated: new Date(2), owner_ref: 'user-2' }, + ] + ctx.allProjects = { + owned: ctx.projects, + readAndWrite: [], + readOnly: [], + tokenReadAndWrite: [], + tokenReadOnly: [], + review: [], + } + ctx.ProjectGetter.promises.findAllUsersProjects.resolves(ctx.allProjects) + ctx.next = sinon.stub() + }) + + it('should respond with the paginated/filtered/sorted projects', async function (ctx) { + ctx.req.body = { + filters: { ownedByUser: true }, + sort: { by: 'lastUpdated', order: 'desc' }, + page: { size: 20 }, + } + await new Promise(resolve => { + ctx.res.json = data => { + expect(data.totalSize).to.equal(ctx.projects.length) + expect(data.projects).to.have.length(ctx.projects.length) + resolve() + } + ctx.ProjectListController.getProjectsJson(ctx.req, ctx.res, ctx.next) + }) + sinon.assert.notCalled(ctx.next) + }) + + it('should work with only a sort and no filters/page', async function (ctx) { + ctx.req.body = { sort: { by: 'title', order: 'asc' } } + await new Promise(resolve => { + ctx.res.json = data => { + expect(data.totalSize).to.equal(ctx.projects.length) + resolve() + } + ctx.ProjectListController.getProjectsJson(ctx.req, ctx.res, ctx.next) + }) + }) + + it('should not reject an unsupported sort.by value in log mode', async function (ctx) { + setReqValidationModeForTests('log') + ctx.req.body = { sort: { by: 'not-a-real-field', order: 'asc' } } + await ctx.ProjectListController.getProjectsJson( + ctx.req, + ctx.res, + ctx.next + ) + sinon.assert.calledOnce(ctx.next) + const err = ctx.next.firstCall.args[0] + expect(err).to.not.be.instanceOf(InvalidRequestError) + expect(err.message).to.equal('Invalid sorting criteria') + }) + + it('should not reject an unknown filter key in log mode', async function (ctx) { + setReqValidationModeForTests('log') + ctx.req.body = { filters: { notARealFilter: true } } + await new Promise(resolve => { + ctx.res.json = data => { + expect(data.totalSize).to.equal(ctx.projects.length) + resolve() + } + ctx.ProjectListController.getProjectsJson(ctx.req, ctx.res, ctx.next) + }) + sinon.assert.notCalled(ctx.next) + }) + + describe('request validation', function () { + beforeEach(function () { + setReqValidationModeForTests('enforce') + }) + + it('rejects an unsupported sort.by value', async function (ctx) { + ctx.req.body = { sort: { by: 'not-a-real-field', order: 'asc' } } + await ctx.ProjectListController.getProjectsJson( + ctx.req, + ctx.res, + ctx.next + ) + sinon.assert.calledWith( + ctx.next, + sinon.match.instanceOf(InvalidRequestError) + ) + }) + + it('rejects an unknown filter key', async function (ctx) { + ctx.req.body = { filters: { notARealFilter: true } } + await ctx.ProjectListController.getProjectsJson( + ctx.req, + ctx.res, + ctx.next + ) + sinon.assert.calledWith( + ctx.next, + sinon.match.instanceOf(InvalidRequestError) + ) + }) + }) + }) }) diff --git a/services/web/test/unit/src/Tags/TagsController.test.mjs b/services/web/test/unit/src/Tags/TagsController.test.mjs index b04457cd0f..4c7306c64d 100644 --- a/services/web/test/unit/src/Tags/TagsController.test.mjs +++ b/services/web/test/unit/src/Tags/TagsController.test.mjs @@ -1,12 +1,17 @@ -import { assert, beforeEach, describe, it, vi } from 'vitest' +import { afterEach, assert, beforeEach, describe, it, vi } from 'vitest' import sinon from 'sinon' -import { InvalidRequestError } from '@overleaf/validation-tools' +import { + InvalidRequestError, + setReqValidationModeForTests, +} from '@overleaf/validation-tools' const modulePath = '../../../../app/src/Features/Tags/TagsController.mjs' describe('TagsController', function () { - const userId = '123nd3ijdks' - const projectId = '123njdskj9jlk' + const userId = '507f191e810c19729de860ea' + const projectId = '507f191e810c19729de860eb' + const tagId = '507f191e810c19729de860ec' + const projectId2 = '507f191e810c19729de860ed' beforeEach(async function (ctx) { ctx.TagsHandler = { @@ -40,9 +45,7 @@ describe('TagsController', function () { ctx.TagsController = (await import(modulePath)).default ctx.req = { - params: { - projectId, - }, + params: {}, session: { user: { _id: userId, @@ -57,6 +60,10 @@ describe('TagsController', function () { ctx.res.json = sinon.stub() }) + afterEach(function () { + setReqValidationModeForTests(null) + }) + it('get all tags', async function (ctx) { await new Promise(resolve => { const allTags = [{ name: 'tag', projects: ['123423', '423423'] }] @@ -126,8 +133,8 @@ describe('TagsController', function () { it('delete a tag', async function (ctx) { await new Promise(resolve => { - ctx.req.params.tagId = ctx.tagId = 'tag-id-123' - ctx.req.session.user._id = ctx.userId = 'user-id-123' + ctx.req.params.tagId = ctx.tagId = tagId + ctx.req.session.user._id = ctx.userId = userId ctx.TagsController.deleteTag(ctx.req, { status: code => { assert.equal(code, 204) @@ -145,10 +152,48 @@ describe('TagsController', function () { }) }) + describe('rename a tag', function () { + beforeEach(function (ctx) { + ctx.req.params.tagId = ctx.tagId = tagId + ctx.req.session.user._id = ctx.userId = userId + }) + + it('with a name', async function (ctx) { + await new Promise(resolve => { + ctx.req.body = { + name: (ctx.tagName = 'new-name'), + } + ctx.TagsController.renameTag(ctx.req, { + status: code => { + assert.equal(code, 204) + sinon.assert.calledWith( + ctx.TagsHandler.promises.renameTag, + ctx.userId, + ctx.tagId, + ctx.tagName + ) + resolve() + return { + end: () => {}, + } + }, + }) + }) + }) + + it('with an empty name, even when the schema is in log-only mode', async function (ctx) { + setReqValidationModeForTests('log') + ctx.req.body = { name: '' } + await ctx.TagsController.renameTag(ctx.req, ctx.res) + sinon.assert.calledWith(ctx.res.status, 400) + sinon.assert.notCalled(ctx.TagsHandler.promises.renameTag) + }) + }) + describe('edit a tag', function () { beforeEach(function (ctx) { - ctx.req.params.tagId = ctx.tagId = 'tag-id-123' - ctx.req.session.user._id = ctx.userId = 'user-id-123' + ctx.req.params.tagId = ctx.tagId = tagId + ctx.req.session.user._id = ctx.userId = userId }) it('with a name and no color', async function (ctx) { @@ -201,17 +246,25 @@ describe('TagsController', function () { it('without a name', function (ctx) { ctx.req.body = { name: undefined } - ctx.TagsController.renameTag(ctx.req, ctx.res).should.be.rejectedWith( + ctx.TagsController.editTag(ctx.req, ctx.res).should.be.rejectedWith( InvalidRequestError ) }) + + it('with an empty name, even when the schema is in log-only mode', async function (ctx) { + setReqValidationModeForTests('log') + ctx.req.body = { name: '' } + await ctx.TagsController.editTag(ctx.req, ctx.res) + sinon.assert.calledWith(ctx.res.status, 400) + sinon.assert.notCalled(ctx.TagsHandler.promises.editTag) + }) }) it('add a project to a tag', async function (ctx) { await new Promise(resolve => { - ctx.req.params.tagId = ctx.tagId = 'tag-id-123' - ctx.req.params.projectId = ctx.projectId = 'project-id-123' - ctx.req.session.user._id = ctx.userId = 'user-id-123' + ctx.req.params.tagId = ctx.tagId = tagId + ctx.req.params.projectId = ctx.projectId = projectId + ctx.req.session.user._id = ctx.userId = userId ctx.TagsController.addProjectToTag(ctx.req, { status: code => { assert.equal(code, 204) @@ -232,12 +285,9 @@ describe('TagsController', function () { it('add projects to a tag', async function (ctx) { await new Promise(resolve => { - ctx.req.params.tagId = ctx.tagId = 'tag-id-123' - ctx.req.body.projectIds = ctx.projectIds = [ - 'project-id-123', - 'project-id-234', - ] - ctx.req.session.user._id = ctx.userId = 'user-id-123' + ctx.req.params.tagId = ctx.tagId = tagId + ctx.req.body.projectIds = ctx.projectIds = [projectId, projectId2] + ctx.req.session.user._id = ctx.userId = userId ctx.TagsController.addProjectsToTag(ctx.req, { status: code => { assert.equal(code, 204) @@ -258,9 +308,9 @@ describe('TagsController', function () { it('remove a project from a tag', async function (ctx) { await new Promise(resolve => { - ctx.req.params.tagId = ctx.tagId = 'tag-id-123' - ctx.req.params.projectId = ctx.projectId = 'project-id-123' - ctx.req.session.user._id = ctx.userId = 'user-id-123' + ctx.req.params.tagId = ctx.tagId = tagId + ctx.req.params.projectId = ctx.projectId = projectId + ctx.req.session.user._id = ctx.userId = userId ctx.TagsController.removeProjectFromTag(ctx.req, { status: code => { assert.equal(code, 204) @@ -281,12 +331,9 @@ describe('TagsController', function () { it('remove projects from a tag', async function (ctx) { await new Promise(resolve => { - ctx.req.params.tagId = ctx.tagId = 'tag-id-123' - ctx.req.body.projectIds = ctx.projectIds = [ - 'project-id-123', - 'project-id-234', - ] - ctx.req.session.user._id = ctx.userId = 'user-id-123' + ctx.req.params.tagId = ctx.tagId = tagId + ctx.req.body.projectIds = ctx.projectIds = [projectId, projectId2] + ctx.req.session.user._id = ctx.userId = userId ctx.TagsController.removeProjectsFromTag(ctx.req, { status: code => { assert.equal(code, 204) diff --git a/services/web/test/unit/src/Uploads/ProjectUploadController.test.mjs b/services/web/test/unit/src/Uploads/ProjectUploadController.test.mjs index eff82293a5..8d3fb9def3 100644 --- a/services/web/test/unit/src/Uploads/ProjectUploadController.test.mjs +++ b/services/web/test/unit/src/Uploads/ProjectUploadController.test.mjs @@ -5,11 +5,18 @@ * DS206: Consider reworking classes to avoid initClass * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md */ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import sinon from 'sinon' +import Path from 'node:path' import MockRequest from '../helpers/MockRequest.mjs' import MockResponse from '../helpers/MockResponse.mjs' import ArchiveErrors from '../../../../app/src/Features/Uploads/ArchiveErrors.mjs' +import { + InvalidParamsError, + InvalidRequestError, + setReqValidationModeForTests, +} from '@overleaf/validation-tools' +import { asZodError } from '@overleaf/validation-tools/testUtils.js' import { FileTooLargeError, DocumentConversionError, @@ -23,6 +30,21 @@ vi.mock('../../../../app/src/Features/Errors/Errors.js', () => const modulePath = '../../../../app/src/Features/Uploads/ProjectUploadController.mjs' +// A minimal but schema-valid multer file object (zz.uploadedFile()), keyed +// off the temp path used throughout these tests. +function uploadedFile(path) { + return { + fieldname: 'qqfile', + originalname: Path.basename(path), + encoding: '7bit', + mimetype: 'application/octet-stream', + size: 1234, + destination: Path.dirname(path), + filename: Path.basename(path), + path, + } +} + describe('ProjectUploadController', function () { beforeEach(async function (ctx) { let Timer @@ -142,13 +164,15 @@ describe('ProjectUploadController', function () { ctx.ProjectUploadController = (await import(modulePath)).default }) + afterEach(function () { + setReqValidationModeForTests(null) + }) + describe('uploadProject', function () { beforeEach(function (ctx) { ctx.path = '/path/to/file/on/disk.zip' ctx.fileName = 'filename.zip' - ctx.req.file = { - path: ctx.path, - } + ctx.req.file = uploadedFile(ctx.path) ctx.req.body = { name: ctx.fileName, } @@ -157,7 +181,7 @@ describe('ProjectUploadController', function () { _id: ctx.user_id, }, } - ctx.project = { _id: (ctx.project_id = 'project-id-123') } + ctx.project = { _id: (ctx.project_id = '507f191e810c19729de860ea') } ctx.fs.unlink = sinon.stub() ctx.fsPromises.unlink = sinon.stub().resolves() @@ -253,17 +277,80 @@ describe('ProjectUploadController', function () { ctx.fs.unlink.calledWith(ctx.path).should.equal(true) }) }) + + describe('request validation', function () { + beforeEach(function () { + setReqValidationModeForTests('enforce') + }) + + it('rejects an unrecognized body field', function (ctx) { + ctx.req.body.extraField = 'nope' + expect(() => + ctx.ProjectUploadController.uploadProject(ctx.req, ctx.res) + ).to.throw(InvalidRequestError) + expect(ctx.fs.unlink).not.to.have.been.called + }) + + it('accepts relativePath set to the literal "null" string', function (ctx) { + ctx.req.body.relativePath = 'null' + ctx.ProjectUploadManager.createProjectFromZipArchive = sinon + .stub() + .callsArgWith(3, null, ctx.project) + ctx.ProjectUploadController.uploadProject(ctx.req, ctx.res) + expect(ctx.res.body).to.deep.equal( + JSON.stringify({ + success: true, + project_id: ctx.project_id, + }) + ) + }) + + it('accepts a nested relativePath', function (ctx) { + ctx.req.body.relativePath = 'folder/main.tex' + ctx.ProjectUploadManager.createProjectFromZipArchive = sinon + .stub() + .callsArgWith(3, null, ctx.project) + ctx.ProjectUploadController.uploadProject(ctx.req, ctx.res) + expect(ctx.res.body).to.deep.equal( + JSON.stringify({ + success: true, + project_id: ctx.project_id, + }) + ) + }) + + it('rejects a relativePath that attempts path traversal', function (ctx) { + ctx.req.body.relativePath = '../../etc/passwd' + let error + try { + ctx.ProjectUploadController.uploadProject(ctx.req, ctx.res) + } catch (err) { + error = err + } + expect( + sinon + .match({ + name: 'InvalidRequestError', + zodError: asZodError({ + code: 'custom', + path: ['body', 'relativePath'], + message: 'path traversal detected', + }), + }) + .test(error) + ).to.be.true + expect(ctx.fs.unlink).not.to.have.been.called + }) + }) }) describe('uploadFile', function () { beforeEach(function (ctx) { - ctx.project_id = 'project-id-123' - ctx.folder_id = 'folder-id-123' + ctx.project_id = '507f191e810c19729de860ea' + ctx.folder_id = '507f191e810c19729de860eb' ctx.path = '/path/to/file/on/disk.png' ctx.fileName = 'filename.png' - ctx.req.file = { - path: ctx.path, - } + ctx.req.file = uploadedFile(ctx.path) ctx.req.body = { name: ctx.fileName, } @@ -480,13 +567,25 @@ describe('ProjectUploadController', function () { ctx.fsPromises.unlink.calledWith(ctx.path).should.equal(true) }) }) + + describe('request validation', function () { + beforeEach(function () { + setReqValidationModeForTests('enforce') + }) + + it('rejects a malformed Project_id param', async function (ctx) { + ctx.req.params.Project_id = 'not-an-object-id' + await expect( + ctx.ProjectUploadController.uploadFile(ctx.req, ctx.res) + ).to.be.rejectedWith(InvalidParamsError) + expect(ctx.fsPromises.unlink).not.to.have.been.called + }) + }) }) describe('importDocument', function () { beforeEach(async function (ctx) { - ctx.req.file = { - path: '/path/to/uploaded/file.docx', - } + ctx.req.file = uploadedFile('/path/to/uploaded/file.docx') ctx.req.body = { name: 'file.docx', } @@ -563,9 +662,7 @@ describe('ProjectUploadController', function () { describe('with conversionType=markdown', async function () { beforeEach(async function (ctx) { - ctx.req.file = { - path: '/path/to/uploaded/file.md', - } + ctx.req.file = uploadedFile('/path/to/uploaded/file.md') ctx.req.body = { name: 'file.md', } @@ -775,6 +872,23 @@ describe('ProjectUploadController', function () { expect(ctx.fsPromises.unlink).to.have.been.calledWith(ctx.req.file.path) }) }) + + describe('request validation', function () { + beforeEach(function () { + setReqValidationModeForTests('enforce') + }) + + it('rejects an unrecognized body field', async function (ctx) { + ctx.req.body.extraField = 'nope' + await expect( + ctx.ProjectUploadController.importDocument(ctx.req, ctx.res) + ).to.be.rejectedWith(InvalidRequestError) + expect( + ctx.DocumentConversionManager.promises + .convertDocumentToLaTeXZipArchive + ).not.to.have.been.called + }) + }) }) describe('multerMiddleware', function () {