From 49969f6e0d6d1fe99d1ab0dff1244f9244098d55 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 6 Aug 2026 10:56:36 +0200 Subject: [PATCH] [history-v1] adopt strict zod types and extend test coverage (#36089) GitOrigin-RevId: 7bdffe2c05c7c17be6eb3e4296d8df05d650c53e --- .../document-updater/app/js/HttpController.js | 3 +- .../api/controllers/project_import.js | 40 +-- .../history-v1/api/controllers/projects.js | 80 ++++-- .../history-v1/api/middleware/security.js | 19 +- services/history-v1/api/schema.js | 270 ++++++++++++++++-- services/history-v1/app.js | 10 +- services/history-v1/backup-deletion-app.mjs | 23 +- services/history-v1/backup-verifier-app.mjs | 23 +- .../acceptance/js/api/backupVerifier.test.mjs | 17 ++ .../acceptance/js/api/project_blobs.test.js | 35 +++ .../acceptance/js/api/project_import.test.js | 79 +++++ .../test/acceptance/js/api/projects.test.js | 2 +- .../js/storage/support/MockFilestore.mjs | 16 +- .../test/acceptance/js/ValidationTests.js | 4 - 14 files changed, 534 insertions(+), 87 deletions(-) diff --git a/services/document-updater/app/js/HttpController.js b/services/document-updater/app/js/HttpController.js index 36f06502bb..824b74a4b3 100644 --- a/services/document-updater/app/js/HttpController.js +++ b/services/document-updater/app/js/HttpController.js @@ -597,8 +597,7 @@ const renameUpdateSchema = z.strictObject({ id: zz.objectId(), // forwarded (via project-history) into history-v1's archive/zip builder // (project_archive.js `archive.append(content, { name: pathname })`), so a - // traversal payload here is a zip-slip risk, not just a display string -- - // see zod-string-field-hardening.md, category C. + // traversal payload here is a zip-slip risk, not just a display string. pathname: zz.safePath(), // deletes are renames to an empty newPathname (see the file comment // above), so the empty string is a legitimate sentinel here, not a diff --git a/services/history-v1/api/controllers/project_import.js b/services/history-v1/api/controllers/project_import.js index 8d585220b7..39b6eab9fc 100644 --- a/services/history-v1/api/controllers/project_import.js +++ b/services/history-v1/api/controllers/project_import.js @@ -38,32 +38,15 @@ const redisBackend = require('../../storage/lib/chunk_store/redis') const rollout = new Rollout(config) rollout.report(logger) // display the rollout configuration in the logs -function getParam(req, name, location = 'path') { - switch (location) { - case 'path': - return req.params?.[name] - case 'query': - return req.query?.[name] - case 'body': - if (name === 'body') { - return req.body - } - if (req.body?.[name] !== undefined) { - return req.body[name] - } - return undefined - default: - return undefined - } -} async function importSnapshot(req, res) { - const { params, body } = parseReq(req, schemas.importSnapshot) + const { params, body } = parseReq(req, schemas.importSnapshot, { + fallbackSchema: schemas.importSnapshotFallbackSchema, + }) const projectId = params.project_id - const rawSnapshot = getParam({ body }, 'snapshot', 'body') ?? body let snapshot try { - snapshot = Snapshot.fromRaw(rawSnapshot) + snapshot = Snapshot.fromRaw(body) } catch (err) { logger.warn({ err, projectId }, 'failed to import snapshot') return render.unprocessableEntity(res) @@ -96,16 +79,17 @@ function getPersistLimits() { } async function importChanges(req, res, next) { - const { params, query, body } = parseReq(req, schemas.importChanges) + const { params, query, body } = parseReq(req, schemas.importChanges, { + fallbackSchema: schemas.importChangesFallbackSchema, + }) const projectId = params.project_id - const rawChanges = getParam({ body }, 'changes', 'body') ?? body const endVersion = query.end_version const returnSnapshot = query.return_snapshot ?? 'none' let changes try { - changes = rawChanges.map(Change.fromRaw) + changes = body.map(rawChange => Change.mustFromRaw(rawChange)) } catch (err) { logger.warn({ err, projectId }, 'failed to parse changes') return render.unprocessableEntity(res) @@ -236,7 +220,9 @@ async function setContent(req, res) { } async function flushChanges(req, res, next) { - const { params } = parseReq(req, schemas.flushChanges) + const { params } = parseReq(req, schemas.flushChanges, { + fallbackSchema: schemas.flushChangesFallbackSchema, + }) const projectId = params.project_id // Use the same limits importChanges, since these are passed to persistChanges const limits = { @@ -256,7 +242,9 @@ async function flushChanges(req, res, next) { } async function expireProject(req, res, next) { - const { params } = parseReq(req, schemas.expireProject) + const { params } = parseReq(req, schemas.expireProject, { + fallbackSchema: schemas.expireProjectFallbackSchema, + }) const projectId = params.project_id await redisBackend.expireProject(projectId) res.status(HTTPStatus.OK).end() diff --git a/services/history-v1/api/controllers/projects.js b/services/history-v1/api/controllers/projects.js index cc22326a56..428800a654 100644 --- a/services/history-v1/api/controllers/projects.js +++ b/services/history-v1/api/controllers/projects.js @@ -37,7 +37,9 @@ const { IncrementalResponse } = require('@overleaf/stream-utils') const pipeline = promisify(Stream.pipeline) async function initializeProject(req, res, next) { - const { body } = parseReq(req, schemas.initializeProject) + const { body } = parseReq(req, schemas.initializeProject, { + fallbackSchema: schemas.initializeProjectFallbackSchema, + }) let projectId = body?.projectId try { projectId = await chunkStore.initializeProject(projectId) @@ -56,7 +58,9 @@ async function cloneProject(req, res) { const { body: { targetProjectId }, params: { project_id: sourceProjectId }, - } = parseReq(req, schemas.cloneProject) + } = parseReq(req, schemas.cloneProject, { + fallbackSchema: schemas.cloneProjectFallbackSchema, + }) const incrResp = new IncrementalResponse({ res, @@ -109,7 +113,9 @@ async function cloneProject(req, res) { } async function getLatestContent(req, res, next) { - const { params } = parseReq(req, schemas.getLatestContent) + const { params } = parseReq(req, schemas.getLatestContent, { + fallbackSchema: schemas.getLatestContentFallbackSchema, + }) const projectId = params.project_id const blobStore = new BlobStore(projectId) const chunk = await chunkStore.loadLatest(projectId) @@ -120,7 +126,9 @@ async function getLatestContent(req, res, next) { } async function getContentAtVersion(req, res, next) { - const { params } = parseReq(req, schemas.getContentAtVersion) + const { params } = parseReq(req, schemas.getContentAtVersion, { + fallbackSchema: schemas.getContentAtVersionFallbackSchema, + }) const projectId = params.project_id const version = params.version const blobStore = new BlobStore(projectId) @@ -130,7 +138,9 @@ async function getContentAtVersion(req, res, next) { } async function getLatestHashedContent(req, res, next) { - const { params } = parseReq(req, schemas.getLatestHashedContent) + const { params } = parseReq(req, schemas.getLatestHashedContent, { + fallbackSchema: schemas.getLatestHashedContentFallbackSchema, + }) const projectId = params.project_id const blobStore = new HashCheckBlobStore(new BlobStore(projectId)) const chunk = await chunkStore.loadLatest(projectId) @@ -142,7 +152,9 @@ async function getLatestHashedContent(req, res, next) { } async function getLatestHistory(req, res, next) { - const { params } = parseReq(req, schemas.getLatestHistory) + const { params } = parseReq(req, schemas.getLatestHistory, { + fallbackSchema: schemas.getLatestHistoryFallbackSchema, + }) const projectId = params.project_id try { const chunk = await chunkStore.loadLatest(projectId) @@ -158,7 +170,9 @@ async function getLatestHistory(req, res, next) { } async function getLatestHistoryRaw(req, res, next) { - const { params, query } = parseReq(req, schemas.getLatestHistoryRaw) + const { params, query } = parseReq(req, schemas.getLatestHistoryRaw, { + fallbackSchema: schemas.getLatestHistoryRawFallbackSchema, + }) const projectId = params.project_id const readOnly = query.readOnly try { @@ -179,7 +193,9 @@ async function getLatestHistoryRaw(req, res, next) { } async function getHistory(req, res, next) { - const { params } = parseReq(req, schemas.getHistory) + const { params } = parseReq(req, schemas.getHistory, { + fallbackSchema: schemas.getHistoryFallbackSchema, + }) const projectId = params.project_id const version = params.version try { @@ -196,7 +212,9 @@ async function getHistory(req, res, next) { } async function getHistoryBefore(req, res, next) { - const { params } = parseReq(req, schemas.getHistoryBefore) + const { params } = parseReq(req, schemas.getHistoryBefore, { + fallbackSchema: schemas.getHistoryBeforeFallbackSchema, + }) const projectId = params.project_id const timestamp = params.timestamp try { @@ -216,7 +234,9 @@ async function getHistoryBefore(req, res, next) { * Get all changes since the beginning of history or since a given version */ async function getChanges(req, res, next) { - const { params, query } = parseReq(req, schemas.getChanges) + const { params, query } = parseReq(req, schemas.getChanges, { + fallbackSchema: schemas.getChangesFallbackSchema, + }) const projectId = params.project_id const sinceParam = query.since const since = sinceParam == null ? 0 : sinceParam @@ -245,7 +265,9 @@ async function getChanges(req, res, next) { } async function getLatestZip(req, res, next) { - const { params } = parseReq(req, schemas.getLatestZip) + const { params } = parseReq(req, schemas.getLatestZip, { + fallbackSchema: schemas.getLatestZipFallbackSchema, + }) const projectId = params.project_id const blobStore = new BlobStore(projectId) @@ -268,7 +290,9 @@ async function getLatestZip(req, res, next) { } async function getZip(req, res, next) { - const { params } = parseReq(req, schemas.getZip) + const { params } = parseReq(req, schemas.getZip, { + fallbackSchema: schemas.getZipFallbackSchema, + }) const projectId = params.project_id const version = params.version const blobStore = new BlobStore(projectId) @@ -301,7 +325,9 @@ async function streamZip(snapshot, blobStore, res) { } async function createZip(req, res, next) { - const { params } = parseReq(req, schemas.createZip) + const { params } = parseReq(req, schemas.createZip, { + fallbackSchema: schemas.createZipFallbackSchema, + }) const projectId = params.project_id const version = params.version try { @@ -322,7 +348,9 @@ async function createZip(req, res, next) { } async function deleteProject(req, res, next) { - const { params } = parseReq(req, schemas.deleteProject) + const { params } = parseReq(req, schemas.deleteProject, { + fallbackSchema: schemas.deleteProjectFallbackSchema, + }) const projectId = params.project_id const blobStore = new BlobStore(projectId) @@ -335,7 +363,9 @@ async function deleteProject(req, res, next) { } async function createProjectBlob(req, res, next) { - const { params } = parseReq(req, schemas.createProjectBlob) + const { params } = parseReq(req, schemas.createProjectBlob, { + fallbackSchema: schemas.createProjectBlobFallbackSchema, + }) const projectId = params.project_id const expectedHash = params.hash const maxUploadSize = parseInt(config.get('maxFileUploadSize'), 10) @@ -373,7 +403,9 @@ async function createProjectBlob(req, res, next) { } async function headProjectBlob(req, res) { - const { params } = parseReq(req, schemas.headProjectBlob) + const { params } = parseReq(req, schemas.headProjectBlob, { + fallbackSchema: schemas.headProjectBlobFallbackSchema, + }) const projectId = params.project_id const hash = params.hash @@ -406,7 +438,9 @@ function _getRangeOpts(header) { } async function getProjectBlob(req, res, next) { - const { params, headers } = parseReq(req, schemas.getProjectBlob) + const { params, headers } = parseReq(req, schemas.getProjectBlob, { + fallbackSchema: schemas.getProjectBlobFallbackSchema, + }) const projectId = params.project_id const hash = params.hash const rangeHeader = headers.range || '' @@ -474,7 +508,9 @@ async function getProjectBlob(req, res, next) { } async function copyProjectBlob(req, res, next) { - const { params, query } = parseReq(req, schemas.copyProjectBlob) + const { params, query } = parseReq(req, schemas.copyProjectBlob, { + fallbackSchema: schemas.copyProjectBlobFallbackSchema, + }) const sourceProjectId = query.copyFrom const targetProjectId = params.project_id const blobHash = params.hash @@ -544,7 +580,9 @@ function sumUpByteLength(blobs) { } async function getBlobStats(req, res) { - const { params, body } = parseReq(req, schemas.getBlobStats) + const { params, body } = parseReq(req, schemas.getBlobStats, { + fallbackSchema: schemas.getBlobStatsFallbackSchema, + }) const projectId = params.project_id const blobHashes = body.blobHashes || [] for (const hash of blobHashes) { @@ -569,7 +607,9 @@ async function getBlobStats(req, res) { } async function getProjectBlobsStats(req, res) { - const { body } = parseReq(req, schemas.getProjectBlobsStats) + const { body } = parseReq(req, schemas.getProjectBlobsStats, { + fallbackSchema: schemas.getProjectBlobsStatsFallbackSchema, + }) const projectIds = body.projectIds const { blobs } = await getProjectBlobsBatch( projectIds.map(id => { diff --git a/services/history-v1/api/middleware/security.js b/services/history-v1/api/middleware/security.js index 4835e455a0..0fd39df4b1 100644 --- a/services/history-v1/api/middleware/security.js +++ b/services/history-v1/api/middleware/security.js @@ -5,9 +5,15 @@ const config = require('config') const HTTPStatus = require('http-status') const jwt = require('jsonwebtoken') const tsscmp = require('tsscmp') -const { parseReq } = require('@overleaf/validation-tools') +const { parseReq, z } = require('@overleaf/validation-tools') const schemas = require('../schema') +// middleware schema is non-strict: it validates only the field this +// middleware consumes; the route schema stays responsible for strictness +const tokenQuerySchema = z.object({ + query: z.object({ token: z.string().optional() }), +}) + function hasValidBasicAuthCredentials(req) { const credentials = basicAuth(req) if (!credentials) return false @@ -68,8 +74,11 @@ function configureJWTAuth(mode = 'jwt') { } let token - if ((mode === 'either' || mode === 'token') && req.query.token) { - token = req.query.token + // Rollout-temporary: reading req.query.token directly on main; log-only + // while REQ_VALIDATION_MODE=log rolls this new call site out. + const { query } = parseReq(req, tokenQuerySchema, { logOnly: true }) + if ((mode === 'either' || mode === 'token') && query.token) { + token = query.token } else if ( (mode === 'either' || mode === 'jwt') && req.headers.authorization && @@ -103,7 +112,9 @@ function configureJWTAuth(mode = 'jwt') { throw error } - const { params } = parseReq(req, schemas.projectId) + const { params } = parseReq(req, schemas.projectId, { + fallbackSchema: schemas.projectIdFallbackSchema, + }) if (decoded.project_id.toString() !== params.project_id.toString()) { const err = new Error('Wrong project_id') err.statusCode = HTTPStatus.FORBIDDEN diff --git a/services/history-v1/api/schema.js b/services/history-v1/api/schema.js index b05fe8e0d3..497051793f 100644 --- a/services/history-v1/api/schema.js +++ b/services/history-v1/api/schema.js @@ -2,10 +2,21 @@ const { z, zz } = require('@overleaf/validation-tools') const Blob = require('overleaf-editor-core').Blob +const { + rawSnapshot, + rawChange, + rawBlobHash, + rawFileMetadata, +} = require('overleaf-editor-core/lib/schemas') const hexHashPattern = new RegExp(Blob.HEX_HASH_RX_STRING) -const fileSchema = z +// Rollout-temporary fallback building blocks (main's pre-refinement shapes +// for imported snapshots/changes, before they were validated against +// overleaf-editor-core's raw* schemas); delete alongside the +// importSnapshotFallbackSchema/importChangesFallbackSchema opts below when +// this route's REQ_VALIDATION_MODE instrumentation is removed. +const fallbackFileSchema = z .object({ hash: z.string().optional(), rangesHash: z.string().optional(), @@ -15,22 +26,22 @@ const fileSchema = z }) .passthrough() -const v2DocVersionsSchema = z.object({ +const fallbackV2DocVersionsSchema = z.object({ pathname: z.string().optional(), v: z.number().int().optional(), }) -const snapshotSchema = z.object({ - files: z.record(z.string(), fileSchema), +const fallbackSnapshotSchema = z.object({ + files: z.record(z.string(), fallbackFileSchema), projectVersion: z.string().optional(), v2DocVersions: z - .record(z.string(), v2DocVersionsSchema) + .record(z.string(), fallbackV2DocVersionsSchema) .nullable() .optional(), timestamp: z.string().optional(), }) -const operationSchema = z +const fallbackOperationSchema = z .object({ pathname: z.string().optional(), newPathname: z.string().optional(), @@ -39,29 +50,27 @@ const operationSchema = z hash: z.string(), }) .optional(), - // TODO: Use rawEditOperation from overleaf-editor-core/lib/schemas.js - // instead textOperation: z.array(z.any()).optional(), - file: fileSchema.optional(), + file: fallbackFileSchema.optional(), contentHash: z.string().optional(), }) .passthrough() -const originSchema = z +const fallbackOriginSchema = z .object({ kind: z.string().optional(), }) .passthrough() -const changeSchema = z +const fallbackChangeSchema = z .object({ timestamp: z.string(), - operations: z.array(operationSchema), + operations: z.array(fallbackOperationSchema), authors: z.array(z.number().int().nullable()).optional(), v2Authors: z.array(z.string().nullable()).optional(), - origin: originSchema.optional(), + origin: fallbackOriginSchema.optional(), projectVersion: z.string().optional(), - v2DocVersions: z.record(z.string(), v2DocVersionsSchema).optional(), + v2DocVersions: z.record(z.string(), fallbackV2DocVersionsSchema).optional(), }) .passthrough() @@ -70,19 +79,38 @@ const setContentBodyBase = z.object({ source: z.string(), userId: zz.objectId().optional(), timestamp: zz.datetime(), - metadata: z.object({}).passthrough().optional(), + metadata: rawFileMetadata.optional(), trackChanges: z.boolean().optional(), }) const schemas = { projectId: z.object({ + params: z + .object({ + project_id: zz.projectHistoryId().optional(), + }) + .optional(), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + projectIdFallbackSchema: z.object({ params: z .object({ project_id: z.string().optional(), }) .optional(), }), + initializeProject: z.object({ + body: z + .strictObject({ + projectId: zz.projectHistoryId().optional(), + }) + .optional(), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + initializeProjectFallbackSchema: z.object({ body: z .object({ projectId: z.string().optional(), @@ -91,22 +119,48 @@ const schemas = { }), cloneProject: z.object({ + body: z.strictObject({ + targetProjectId: zz.projectHistoryId(), + }), + params: z.object({ + project_id: zz.projectHistoryId(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + cloneProjectFallbackSchema: z.object({ body: z.object({ targetProjectId: z.string(), }), - params: z.object({ project_id: z.string(), }), }), getProjectBlobsStats: z.object({ + body: z.strictObject({ + projectIds: z.array(zz.projectHistoryId()), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + getProjectBlobsStatsFallbackSchema: z.object({ body: z.object({ projectIds: z.array(z.string()), }), }), getBlobStats: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + }), + body: z.strictObject({ + blobHashes: z.array(rawBlobHash), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + getBlobStatsFallbackSchema: z.object({ params: z.object({ project_id: z.string(), }), @@ -116,13 +170,30 @@ const schemas = { }), deleteProject: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + deleteProjectFallbackSchema: z.object({ params: z.object({ project_id: z.string(), }), - body: z.any().optional(), }), getProjectBlob: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + hash: z.string().regex(hexHashPattern), + }), + headers: z.object({ + range: z.string().optional(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + getProjectBlobFallbackSchema: z.object({ params: z.object({ project_id: z.string(), hash: z.string().regex(hexHashPattern), @@ -133,6 +204,14 @@ const schemas = { }), headProjectBlob: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + hash: z.string().regex(hexHashPattern), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + headProjectBlobFallbackSchema: z.object({ params: z.object({ project_id: z.string(), hash: z.string().regex(hexHashPattern), @@ -140,14 +219,33 @@ const schemas = { }), createProjectBlob: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + hash: z.string().regex(hexHashPattern), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + createProjectBlobFallbackSchema: z.object({ params: z.object({ project_id: z.string(), hash: z.string().regex(hexHashPattern), }), - body: z.any().optional(), }), copyProjectBlob: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + hash: z.string().regex(hexHashPattern), + }), + query: z.object({ + copyFrom: zz.projectHistoryId(), + sizeLimit: z.coerce.number().optional(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + copyProjectBlobFallbackSchema: z.object({ params: z.object({ project_id: z.string(), hash: z.string().regex(hexHashPattern), @@ -156,28 +254,58 @@ const schemas = { copyFrom: z.string(), sizeLimit: z.coerce.number().optional(), }), - body: z.any().optional(), }), getLatestContent: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + getLatestContentFallbackSchema: z.object({ params: z.object({ project_id: z.string(), }), }), getLatestHashedContent: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + getLatestHashedContentFallbackSchema: z.object({ params: z.object({ project_id: z.string(), }), }), getLatestHistory: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + getLatestHistoryFallbackSchema: z.object({ params: z.object({ project_id: z.string(), }), }), getLatestHistoryRaw: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + }), + query: z.object({ + readOnly: z.coerce.boolean().optional(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + getLatestHistoryRawFallbackSchema: z.object({ params: z.object({ project_id: z.string(), }), @@ -188,11 +316,19 @@ const schemas = { getLatestPersistedHistory: z.object({ params: z.object({ - project_id: z.string(), + project_id: zz.projectHistoryId(), }), }), getHistory: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + version: z.coerce.number(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + getHistoryFallbackSchema: z.object({ params: z.object({ project_id: z.string(), version: z.coerce.number(), @@ -200,6 +336,14 @@ const schemas = { }), getContentAtVersion: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + version: z.coerce.number(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + getContentAtVersionFallbackSchema: z.object({ params: z.object({ project_id: z.string(), version: z.coerce.number(), @@ -207,6 +351,14 @@ const schemas = { }), getHistoryBefore: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + timestamp: zz.datetime(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + getHistoryBeforeFallbackSchema: z.object({ params: z.object({ project_id: z.string(), timestamp: zz.datetime(), @@ -214,12 +366,27 @@ const schemas = { }), getLatestZip: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + getLatestZipFallbackSchema: z.object({ params: z.object({ project_id: z.string(), }), }), getZip: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + version: z.coerce.number(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + getZipFallbackSchema: z.object({ params: z.object({ project_id: z.string(), version: z.coerce.number(), @@ -227,14 +394,31 @@ const schemas = { }), createZip: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + version: z.coerce.number(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + createZipFallbackSchema: z.object({ params: z.object({ project_id: z.string(), version: z.coerce.number(), }), - body: z.any().optional(), }), getChanges: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + }), + query: z.object({ + since: z.coerce.number().optional(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + getChangesFallbackSchema: z.object({ params: z.object({ project_id: z.string(), }), @@ -244,13 +428,37 @@ const schemas = { }), importSnapshot: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + }), + body: rawSnapshot, + }), + // Rollout-temporary fallback (pre-refinement schema from main, before + // imported snapshots were validated against overleaf-editor-core's + // rawSnapshot); delete when this route's REQ_VALIDATION_MODE + // instrumentation is removed. + importSnapshotFallbackSchema: z.object({ params: z.object({ project_id: z.string(), }), - body: snapshotSchema, + body: fallbackSnapshotSchema, }), importChanges: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + }), + query: z.object({ + end_version: z.coerce.number(), + return_snapshot: z.enum(['hashed', 'none']).optional(), + }), + body: z.array(rawChange), + }), + // Rollout-temporary fallback (pre-refinement schema from main, before + // imported changes were validated against overleaf-editor-core's + // rawChange); delete when this route's REQ_VALIDATION_MODE + // instrumentation is removed. + importChangesFallbackSchema: z.object({ params: z.object({ project_id: z.string(), }), @@ -258,7 +466,7 @@ const schemas = { end_version: z.coerce.number(), return_snapshot: z.enum(['hashed', 'none']).optional(), }), - body: z.array(changeSchema), + body: z.array(fallbackChangeSchema), }), setContent: z.object({ @@ -287,17 +495,29 @@ const schemas = { }), flushChanges: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + flushChangesFallbackSchema: z.object({ params: z.object({ project_id: z.string(), }), - body: z.any().optional(), }), expireProject: z.object({ + params: z.object({ + project_id: zz.projectHistoryId(), + }), + }), + // Rollout-temporary fallback (pre-refinement schema from main); delete + // when this route's REQ_VALIDATION_MODE instrumentation is removed. + expireProjectFallbackSchema: z.object({ params: z.object({ project_id: z.string(), }), - body: z.any().optional(), }), } diff --git a/services/history-v1/app.js b/services/history-v1/app.js index c9042de8fb..7a61c55cdd 100644 --- a/services/history-v1/app.js +++ b/services/history-v1/app.js @@ -17,7 +17,10 @@ const healthChecks = require('./api/controllers/health_checks') const { mongodb, loadGlobalBlobs } = require('./storage') const projectsRoutes = require('./api/routes/projects') const projectImportRoutes = require('./api/routes/project_import') -const { createHandleValidationError } = require('@overleaf/validation-tools') +const { + createHandleValidationError, + getRawReqInput, +} = require('@overleaf/validation-tools') Events.setMaxListeners(20) const app = express() @@ -73,7 +76,10 @@ function setupErrorHandling() { app.use(handleValidationError) app.use(function (err, req, res, next) { - const projectId = req.params?.project_id || req.body?.projectId + // terminal error handler must not throw while extracting log context + // (case 2: final error-handler logging) + const { params, body } = getRawReqInput(req) + const projectId = params?.project_id || body?.projectId logger.error({ err, projectId }, err.message) if (res.headersSent) { diff --git a/services/history-v1/backup-deletion-app.mjs b/services/history-v1/backup-deletion-app.mjs index 53aa298aee..500dfca5b3 100644 --- a/services/history-v1/backup-deletion-app.mjs +++ b/services/history-v1/backup-deletion-app.mjs @@ -8,6 +8,12 @@ import express from 'express' import logger from '@overleaf/logger' import Metrics from '@overleaf/metrics' import { hasValidBasicAuthCredentials } from './api/middleware/security.js' +import { + handleValidationError, + parseReq, + z, + zz, +} from '@overleaf/validation-tools' import { deleteProjectBackupCb, healthCheck, @@ -32,8 +38,21 @@ function basicAuth(req, res, next) { res.sendStatus(401) } +const deleteProjectBackupSchema = z.object({ + params: z.strictObject({ + projectId: zz.objectId(), + }), +}) + app.delete('/project/:projectId/backup', basicAuth, (req, res, next) => { - deleteProjectBackupCb(req.params.projectId, err => { + // Rollout-temporary: main read req.params directly here; log-only while + // REQ_VALIDATION_MODE=log rolls this new call site out. No transforms in + // deleteProjectBackupSchema, so a raw passthrough on failure needs no + // fallback. + const { params } = parseReq(req, deleteProjectBackupSchema, { + logOnly: true, + }) + deleteProjectBackupCb(params.projectId, err => { if (err) { return next(err) } @@ -52,6 +71,8 @@ app.get('/health_check', (req, res, next) => { }) }) +app.use(handleValidationError) + app.use((err, req, res, next) => { req.logger.addFields({ err }) if (err instanceof NotReadyToDelete) { diff --git a/services/history-v1/backup-verifier-app.mjs b/services/history-v1/backup-verifier-app.mjs index 856a15dd53..2e441ed7cc 100644 --- a/services/history-v1/backup-verifier-app.mjs +++ b/services/history-v1/backup-verifier-app.mjs @@ -15,7 +15,14 @@ import { } from './storage/lib/backupVerifier.mjs' import { mongodb } from './storage/index.js' import { expressify } from '@overleaf/promise-utils' +import { + handleValidationError, + parseReq, + z, + zz, +} from '@overleaf/validation-tools' import { Blob } from 'overleaf-editor-core' +import editorCoreSchemas from 'overleaf-editor-core/lib/schemas.js' import { loadGlobalBlobs } from './storage/lib/blob_store/index.js' import { EventEmitter } from 'node:events' import { @@ -33,10 +40,22 @@ Metrics.leaked_sockets.monitor(logger) Metrics.event_loop.monitor(logger) Metrics.memory.monitor(logger) +const verifyBlobSchema = z.object({ + params: z.strictObject({ + historyId: zz.projectHistoryId(), + hash: editorCoreSchemas.rawBlobHash, + }), +}) + app.get( '/history/:historyId/blob/:hash/verify', expressify(async (req, res) => { - const { historyId, hash } = req.params + // Rollout-temporary: main read req.params directly here; log-only while + // REQ_VALIDATION_MODE=log rolls this new call site out. No transforms in + // verifyBlobSchema, so a raw passthrough on failure needs no fallback. + const { historyId, hash } = parseReq(req, verifyBlobSchema, { + logOnly: true, + }).params try { await verifyBlob(historyId, hash) res.sendStatus(200) @@ -65,6 +84,8 @@ app.get( }) ) +app.use(handleValidationError) + app.use((err, req, res, next) => { req.logger.addFields({ err }) req.logger.setLevel('error') diff --git a/services/history-v1/test/acceptance/js/api/backupVerifier.test.mjs b/services/history-v1/test/acceptance/js/api/backupVerifier.test.mjs index e898620c5b..817ff4ecb2 100644 --- a/services/history-v1/test/acceptance/js/api/backupVerifier.test.mjs +++ b/services/history-v1/test/acceptance/js/api/backupVerifier.test.mjs @@ -470,5 +470,22 @@ describe('backupVerifier', function () { const response = await verifyBlobHTTP(historyIdMongo, blobHashMongo) expect(response.status).to.equal(200) }) + + it('rejects a historyId that is not a Mongo ObjectId or Postgres id', async function () { + const response = await verifyBlobHTTP( + 'a%2F..%2F..%2F..%2F..%2F..%2Fetc', + 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' + ) + expect(response.status).to.equal(404) + const body = await response.json() + expect(body.error).to.include('historyId') + }) + + it('rejects a hash that is not a 40-character hex blob hash', async function () { + const response = await verifyBlobHTTP(historyIdPostgres, 'not-a-hash') + expect(response.status).to.equal(404) + const body = await response.json() + expect(body.error).to.include('hash') + }) }) }) diff --git a/services/history-v1/test/acceptance/js/api/project_blobs.test.js b/services/history-v1/test/acceptance/js/api/project_blobs.test.js index b9de19a7f7..44d091d13a 100644 --- a/services/history-v1/test/acceptance/js/api/project_blobs.test.js +++ b/services/history-v1/test/acceptance/js/api/project_blobs.test.js @@ -432,4 +432,39 @@ describe('Project blobs API', function () { expect(response.status).to.equal(HTTPStatus.NO_CONTENT) }) }) + + describe('validation', function () { + it('rejects a project_id that is not a Mongo ObjectId or Postgres id', async function () { + const badProjectId = 'a/../../../../../etc' + const badToken = testServer.createTokenForProject(badProjectId) + const response = await fetch( + testServer.url( + `/api/projects/${encodeURIComponent(badProjectId)}/blobs/${testFiles.HELLO_TXT_HASH}` + ), + { headers: { Authorization: `Bearer ${badToken}` } } + ) + expect(response.status).to.equal(HTTPStatus.NOT_FOUND) + const body = await response.json() + expect(body.error).to.include('project_id') + }) + + it('rejects a copyFrom that is not a Mongo ObjectId or Postgres id', async function () { + const targetProjectId = '456' + const targetToken = testServer.createTokenForProject(targetProjectId) + const url = new URL( + testServer.url( + `/api/projects/${targetProjectId}/blobs/${testFiles.HELLO_TXT_HASH}` + ) + ) + url.searchParams.append('copyFrom', 'a/../../../../../etc') + + const response = await fetch(url, { + method: 'POST', + headers: { Authorization: `Bearer ${targetToken}` }, + }) + expect(response.status).to.equal(HTTPStatus.UNPROCESSABLE_ENTITY) + const body = await response.json() + expect(body.error).to.include('copyFrom') + }) + }) }) diff --git a/services/history-v1/test/acceptance/js/api/project_import.test.js b/services/history-v1/test/acceptance/js/api/project_import.test.js index fb173238f8..7c6d9c812f 100644 --- a/services/history-v1/test/acceptance/js/api/project_import.test.js +++ b/services/history-v1/test/acceptance/js/api/project_import.test.js @@ -54,4 +54,83 @@ describe('project import', function () { expect(importResponse.status).to.equal(HTTPStatus.CREATED) expect(importResponse.obj).to.deep.equal({ resyncNeeded: false }) }) + + it('accepts an add-file operation with importedAt-only file metadata', async function () { + const basicAuthClient = testServer.basicAuthClient + const projectId = await testProjects.createEmptyProject() + + // upload an empty file + const response = await fetch( + testServer.url( + `/api/projects/${projectId}/blobs/${File.EMPTY_FILE_HASH}`, + { qs: { pathname: 'main.tex' } } + ), + { + method: 'PUT', + body: fs.createReadStream(testFiles.path('empty.tex')), + headers: { + Authorization: testServer.basicAuthHeader, + }, + } + ) + expect(response.ok).to.be.true + + const testFile = File.fromHash(File.EMPTY_FILE_HASH, undefined, { + importedAt: new Date().toISOString(), + }) + const testChange = new Change( + [Operation.addFile('main.tex', testFile)], + new Date() + ) + + const importResponse = + await basicAuthClient.apis.ProjectImport.importChanges1({ + project_id: projectId, + end_version: 0, + changes: [testChange.toRaw()], + }) + + expect(importResponse.status).to.equal(HTTPStatus.CREATED) + expect(importResponse.obj).to.deep.equal({ resyncNeeded: false }) + }) + + it('accepts an add-file operation with main-flag and importedAt file metadata', async function () { + const basicAuthClient = testServer.basicAuthClient + const projectId = await testProjects.createEmptyProject() + + // upload an empty file + const response = await fetch( + testServer.url( + `/api/projects/${projectId}/blobs/${File.EMPTY_FILE_HASH}`, + { qs: { pathname: 'main.tex' } } + ), + { + method: 'PUT', + body: fs.createReadStream(testFiles.path('empty.tex')), + headers: { + Authorization: testServer.basicAuthHeader, + }, + } + ) + expect(response.ok).to.be.true + + const testFile = File.fromHash(File.EMPTY_FILE_HASH, undefined, { + main: true, + importedAt: new Date().toISOString(), + }) + const testChange = new Change( + [Operation.addFile('main.tex', testFile)], + new Date() + ) + + const importResponse = + await basicAuthClient.apis.ProjectImport.importChanges1({ + project_id: projectId, + end_version: 0, + changes: [testChange.toRaw()], + }) + + expect(importResponse.status).to.equal(HTTPStatus.CREATED) + expect(importResponse.obj).to.deep.equal({ resyncNeeded: false }) + }) }) diff --git a/services/history-v1/test/acceptance/js/api/projects.test.js b/services/history-v1/test/acceptance/js/api/projects.test.js index 20e19a3c39..226366083b 100644 --- a/services/history-v1/test/acceptance/js/api/projects.test.js +++ b/services/history-v1/test/acceptance/js/api/projects.test.js @@ -439,7 +439,7 @@ describe('project controller', function () { blobHashes: ['non-existent-hash'], }, }), - HTTPStatus.INTERNAL_SERVER_ERROR + HTTPStatus.UNPROCESSABLE_ENTITY ) }) diff --git a/services/history-v1/test/acceptance/js/storage/support/MockFilestore.mjs b/services/history-v1/test/acceptance/js/storage/support/MockFilestore.mjs index 55d0923c34..d81bc171bb 100644 --- a/services/history-v1/test/acceptance/js/storage/support/MockFilestore.mjs +++ b/services/history-v1/test/acceptance/js/storage/support/MockFilestore.mjs @@ -1,4 +1,17 @@ import express from 'express' +import { + handleValidationError, + parseReq, + z, + zz, +} from '@overleaf/validation-tools' + +const getFileSchema = z.object({ + params: z.strictObject({ + projectId: zz.objectId(), + fileId: zz.objectId(), + }), +}) class MockFilestore { constructor() { @@ -10,11 +23,12 @@ class MockFilestore { this.app = express() this.app.get('/project/:projectId/file/:fileId', (req, res) => { - const { projectId, fileId } = req.params + const { projectId, fileId } = parseReq(req, getFileSchema).params const content = this.files[projectId]?.[fileId] if (!content) return res.status(404).end() res.status(200).end(content) }) + this.app.use(handleValidationError) } start() { diff --git a/services/project-history/test/acceptance/js/ValidationTests.js b/services/project-history/test/acceptance/js/ValidationTests.js index 558e0ecff4..9d97dd39e3 100644 --- a/services/project-history/test/acceptance/js/ValidationTests.js +++ b/services/project-history/test/acceptance/js/ValidationTests.js @@ -71,10 +71,6 @@ describe('Validation', function () { }) it('should return 400 for a non-datetime created_at in the label body', async function () { - // created_at previously accepted a bare string (e.g. an epoch-ms - // string), so an invalid value would silently become an Invalid Date - // downstream instead of being rejected at the schema boundary -- see - // zod-string-field-hardening.md's housekeeping note. const projectId = new ObjectId().toString() let err try {