diff --git a/services/filestore/app.js b/services/filestore/app.js index 9d138c8111..db5b24a53f 100644 --- a/services/filestore/app.js +++ b/services/filestore/app.js @@ -9,6 +9,7 @@ import express from 'express' import fileController from './app/js/FileController.js' import keyBuilder from './app/js/KeyBuilder.js' import RequestLogger from './app/js/RequestLogger.js' +import { handleValidationError } from '@overleaf/validation-tools' logger.initialize(process.env.METRICS_APP_NAME || 'filestore') @@ -68,6 +69,11 @@ if (settings.filestore.stores.template_files) { ) } +// Not a named `:key(.*)` param: Express 4.22.1's bundled path-to-regexp has a +// bug where combining a named param with a custom (.*) regex both duplicates +// the match under a numeric key AND corrupts the captured value (off-by-one +// substring). The anonymous `*` wildcard doesn't have this bug, so keep it +// and validate the numeric params[0] key directly (see schemas.js). app.get( '/bucket/:bucket/key/*', keyBuilder.bucketFileKeyMiddleware, @@ -97,6 +103,7 @@ app.get('/health_check', (req, res) => { res.sendStatus(200) }) +app.use(handleValidationError) app.use(RequestLogger.errorHandler) const port = settings.internal.filestore.port || 3009 diff --git a/services/filestore/app/js/FileController.js b/services/filestore/app/js/FileController.js index c2dffe76cb..5e00ca93dc 100644 --- a/services/filestore/app/js/FileController.js +++ b/services/filestore/app/js/FileController.js @@ -3,6 +3,12 @@ import metrics from '@overleaf/metrics' import parseRange from 'range-parser' import Errors from './Errors.js' import { pipeline } from 'node:stream' +import { parseReq } from '@overleaf/validation-tools' +import { + getFileQuerySchema, + getFileQueryFallbackSchema, + insertFileSchema, +} from './schemas.js' const maxSizeInBytes = 1024 * 1024 * 1024 // 1GB @@ -14,7 +20,11 @@ export default { function getFile(req, res, next) { const { key, bucket } = req - const { format, style } = req.query + const { query, headers } = parseReq(req, getFileQuerySchema, { + logOnly: true, + fallbackSchema: getFileQueryFallbackSchema, + }) + const { format, style, cacheWarm } = query const options = { key, bucket, @@ -30,11 +40,11 @@ function getFile(req, res, next) { bucket, format, style, - cacheWarm: req.query.cacheWarm, + cacheWarm, }) - if (req.headers.range) { - const range = _getRange(req.headers.range) + if (headers.range) { + const range = _getRange(headers.range) if (range) { options.start = range.start options.end = range.end @@ -67,7 +77,7 @@ function getFile(req, res, next) { return } - if (req.query.cacheWarm) { + if (cacheWarm) { fileStream.destroy() return res.sendStatus(200).end() } @@ -115,6 +125,7 @@ function getFileHead(req, res, next) { } function insertFile(req, res, next) { + parseReq(req, insertFileSchema, { logOnly: true }) metrics.inc('insertFile') const { key, bucket } = req diff --git a/services/filestore/app/js/KeyBuilder.js b/services/filestore/app/js/KeyBuilder.js index c548508280..bdc23ae3e6 100644 --- a/services/filestore/app/js/KeyBuilder.js +++ b/services/filestore/app/js/KeyBuilder.js @@ -1,5 +1,14 @@ import settings from '@overleaf/settings' import projectKey from '@overleaf/object-persistor/src/ProjectKey.js' +import { parseReq } from '@overleaf/validation-tools' +import { + bucketFileParamsSchema, + globalBlobFileParamsSchema, + projectBlobFileParamsSchema, + projectBlobFileParamsFallbackSchema, + templateFileParamsSchema, + templateFileParamsFallbackSchema, +} from './schemas.js' export default { getConvertedFolderKey, @@ -31,34 +40,43 @@ function addCachingToKey(key, opts) { } function bucketFileKeyMiddleware(req, res, next) { - req.bucket = req.params.bucket - req.key = req.params[0] + const { params } = parseReq(req, bucketFileParamsSchema, { + logOnly: true, + }) + req.bucket = params.bucket + req.key = params[0] next() } function globalBlobFileKeyMiddleware(req, res, next) { + const { params } = parseReq(req, globalBlobFileParamsSchema, { + logOnly: true, + }) req.bucket = settings.filestore.stores.global_blobs - const { hash } = req.params + const { hash } = params req.key = `${hash.slice(0, 2)}/${hash.slice(2, 4)}/${hash.slice(4)}` req.useSubdirectories = true next() } function projectBlobFileKeyMiddleware(req, res, next) { + const { params } = parseReq(req, projectBlobFileParamsSchema, { + logOnly: true, + fallbackSchema: projectBlobFileParamsFallbackSchema, + }) req.bucket = settings.filestore.stores.project_blobs - const { historyId, hash } = req.params + const { historyId, hash } = params req.key = `${projectKey.format(historyId)}/${hash.slice(0, 2)}/${hash.slice(2)}` req.useSubdirectories = true next() } function templateFileKeyMiddleware(req, res, next) { - const { - template_id: templateId, - format, - version, - sub_type: subType, - } = req.params + const { params } = parseReq(req, templateFileParamsSchema, { + logOnly: true, + fallbackSchema: templateFileParamsFallbackSchema, + }) + const { template_id: templateId, format, version, sub_type: subType } = params req.key = `${templateId}/v/${version}/${format}` diff --git a/services/filestore/app/js/schemas.js b/services/filestore/app/js/schemas.js new file mode 100644 index 0000000000..2a20978f96 --- /dev/null +++ b/services/filestore/app/js/schemas.js @@ -0,0 +1,123 @@ +import { z, zz } from '@overleaf/validation-tools' +import { rawBlobHash } from 'overleaf-editor-core/lib/schemas.js' + +// Mongo-linked or legacy v1 numeric project/history id (see +// project-history's app/js/HttpController.js for the same historyIdSchema +// pattern -- most history routes accept either a Mongo-linked or legacy v1 +// numeric project id). +const historyIdSchema = zz.objectId().or(z.coerce.number()) + +// KeyBuilder's *FileKeyMiddleware functions are the sole consumers of +// req.params for their routes (FileController only ever reads the derived +// req.key/req.bucket set by these middlewares), so each gets a strict, +// route-owning schema rather than the permissive wrapper used by +// cross-cutting middleware. + +// Note: this ":format" path param is unrelated to the "format"/"style" +// *query* params validated below (getFileQuerySchema) -- it's really a +// per-file identifier (an arbitrary id or filename) that becomes a storage +// key segment, matching how templates' own filestore Proxy.js validates the +// same value (forwarded verbatim) as "file_type". +export const templateFileParamsSchema = z.object({ + params: z.strictObject({ + template_id: zz.objectId(), + version: z.coerce.number().int().nonnegative(), + format: zz.filepath(), + // present only on the .../v/:version/:format/:sub_type mount; this same + // middleware is used for both routes + sub_type: zz.filepath().optional(), + }), +}) + +// Rollout-temporary fallback (loosened variant of templateFileParamsSchema: +// z.strictObject -> z.object, zz.objectId()/zz.filepath() -> z.string(), +// int()/nonnegative() refinements dropped, z.coerce.number() kept) for the +// REQ_VALIDATION_MODE log-only rollout; delete when this route's +// instrumentation is removed. +export const templateFileParamsFallbackSchema = z.object({ + params: z.object({ + template_id: z.string(), + version: z.coerce.number(), + format: z.string(), + sub_type: z.string().optional(), + }), +}) + +export const bucketFileParamsSchema = z.object({ + params: z.strictObject({ + bucket: z.string().regex(/^[a-z0-9-]+$/), + // the anonymous `*` wildcard in app.js (kept anonymous rather than a + // named `:key(.*)` -- see the comment there for why); Express exposes + // its capture under the numeric key '0'. May contain nested segments. + 0: zz.filepath(), + }), +}) + +export const globalBlobFileParamsSchema = z.object({ + params: z.strictObject({ + hash: rawBlobHash, + }), +}) + +export const projectBlobFileParamsSchema = z.object({ + params: z.strictObject({ + historyId: historyIdSchema, + hash: rawBlobHash, + }), +}) + +// Rollout-temporary fallback (loosened variant of projectBlobFileParamsSchema: +// z.strictObject -> z.object, zz.objectId()/regex refinements dropped from +// historyId/hash, z.coerce.number() kept in the historyId union) for the +// REQ_VALIDATION_MODE log-only rollout; delete when this route's +// instrumentation is removed. +export const projectBlobFileParamsFallbackSchema = z.object({ + params: z.object({ + historyId: z.string().or(z.coerce.number()), + hash: z.string(), + }), +}) + +// Shared across every getFile/getFileHead route regardless of which +// *FileKeyMiddleware ran first. +export const getFileQuerySchema = z.object({ + query: z.strictObject({ + // desired output conversion format/style (currently only 'png' / + // 'thumbnail' / 'preview' are ever accepted downstream in + // FileConverter.js and FileHandler.js -- left as a plain optional + // string here since an unsupported value already yields a well-defined + // ConversionError/500 today, and no existing test pins a 400 for this). + format: z.string().optional(), + style: z.string().optional(), + cacheWarm: z.stringbool().optional(), + }), + headers: z.object({ + range: z.string().optional(), + }), +}) + +// Rollout-temporary fallback (loosened variant of getFileQuerySchema: +// z.strictObject -> z.object, z.stringbool() kept since cacheWarm is used as +// a real boolean downstream -- a raw string like "false" would be truthy) +// for the REQ_VALIDATION_MODE log-only rollout; delete when this route's +// instrumentation is removed. +export const getFileQueryFallbackSchema = z.object({ + query: z.object({ + format: z.string().optional(), + style: z.string().optional(), + cacheWarm: z.stringbool().optional(), + }), + headers: z.object({ + range: z.string().optional(), + }), +}) + +// insertFile reads the raw request stream itself (no body parser is +// mounted), so the body is validated as a streamed body. No fallbackSchema: +// insertFile never reads parseReq()'s return value (FileHandler.insertFile +// reads the stream off `req` directly), so a raw passthrough on failure +// can't hand the handler a wrong type -- a fallback here would just +// duplicate the primary schema. +export const insertFileSchema = z.object({ + body: zz.streamedBody(), +}) diff --git a/services/filestore/package.json b/services/filestore/package.json index c39c5b67c1..b01be6da30 100644 --- a/services/filestore/package.json +++ b/services/filestore/package.json @@ -25,11 +25,13 @@ "@overleaf/object-persistor": "workspace:*", "@overleaf/settings": "workspace:*", "@overleaf/stream-utils": "^0.1.0", + "@overleaf/validation-tools": "workspace:*", "bunyan": "^1.8.15", "express": "4.22.1", "glob": "^12.0.0", "lodash.once": "^4.1.1", "node-fetch": "^2.7.0", + "overleaf-editor-core": "workspace:*", "range-parser": "^1.2.1", "tiny-async-pool": "^1.1.0" }, diff --git a/services/filestore/test/acceptance/js/FilestoreTests.js b/services/filestore/test/acceptance/js/FilestoreTests.js index 9f0311e2e4..3a154a9ccf 100644 --- a/services/filestore/test/acceptance/js/FilestoreTests.js +++ b/services/filestore/test/acceptance/js/FilestoreTests.js @@ -32,6 +32,8 @@ import { S3Persistor } from '@overleaf/object-persistor/src/S3Persistor.js' import crypto from 'node:crypto' import { WritableBuffer } from '@overleaf/stream-utils' import { gzipSync } from 'node:zlib' +import projectKey from '@overleaf/object-persistor/src/ProjectKey.js' +import { expectValidationError } from '@overleaf/validation-tools/testUtils.js' const { expect } = chai @@ -371,6 +373,182 @@ describe('Filestore', function () { } }) + describe('with a sub_type file', function () { + // there is no POST route for the /:sub_type variant, so seed + // directly via the persistor (same technique used elsewhere in this + // file for keys that the HTTP API can't produce). + const subType = 'thumbnail' + let subTypeContent + + beforeEach('upload file with sub_type', async function () { + subTypeContent = `sub_type content ${Math.random()}` + await app.persistor.sendStream( + Settings.filestore.stores.template_files, + `${fileKey}/${subType}`, + Stream.Readable.from([subTypeContent]) + ) + }) + + it('should be able to get the file back', async function () { + const response = await fetch(`${fileUrl}/${subType}`) + const body = await response.text() + expect(body).to.equal(subTypeContent) + }) + }) + + describe('history blobs', function () { + // a fresh random hash per test, not a shared constant: several + // backend variants reuse the same physical bucket (see + // TestConfig.js), and mocha shards run in parallel processes, so a + // fixed key would race across them. + function randomHash() { + return crypto.randomBytes(20).toString('hex') + } + + // The blob routes always request useSubdirectories: true (blob + // keys are namespaced by nested prefixes, not flat ids), but + // FSPersistor#sendStream ignores per-call useSubdirectories and + // only consults its own construction-time default (false here) -- + // so seeding through the persistor would flatten '/' to '_' on + // write while the read path expects real subdirectories. Write + // directly to match what the read path (and real callers, which + // never go through filestore's own insertFile for these blobs + // anyway) actually expects. + async function seedBlob(bucket, key, content) { + if (backendSettings.backend === 'fs') { + const fsPath = Path.join(bucket, key) + await fs.promises.mkdir(Path.dirname(fsPath), { + recursive: true, + }) + await fs.promises.writeFile(fsPath, content) + } else { + await app.persistor.sendStream( + bucket, + key, + Stream.Readable.from([content]) + ) + } + } + + // Global blobs are keyed purely by hash prefix (no project-folder + // concept at all), which is fundamentally incompatible with + // PerProjectEncryptedS3Persistor's per-project encryption scheme -- + // in production, a bucket that isn't per-project (deks/chunks/ + // project-blobs) is rejected outright by pathToProjectFolder (see + // history-v1/storage/lib/backupPersistor.mjs). Project blobs *are* + // project-shaped (via projectKey.format()) and are expected to work + // under s3SSEC, so only global blobs skip this backend. + if (backendSettings.backend !== 's3SSEC') { + describe('global blob', function () { + let hash, globalBlobContent + + beforeEach('upload global blob', async function () { + hash = randomHash() + globalBlobContent = `global blob content ${Math.random()}` + const globalBlobKey = `${hash.slice(0, 2)}/${hash.slice(2, 4)}/${hash.slice(4)}` + await seedBlob( + Settings.filestore.stores.global_blobs, + globalBlobKey, + globalBlobContent + ) + }) + + it('should be able to get the blob back', async function () { + const response = await fetch( + `${filestoreUrl}/history/global/hash/${hash}` + ) + const body = await response.text() + expect(body).to.equal(globalBlobContent) + }) + + it('should return 404 for an unknown hash', async function () { + const response = await fetch( + `${filestoreUrl}/history/global/hash/${randomHash()}` + ) + expect(response.status).to.equal(404) + }) + }) + } + + describe('project blob', function () { + let hash, historyId, projectBlobContent + + beforeEach('upload project blob', async function () { + hash = randomHash() + historyId = new ObjectId().toString() + projectBlobContent = `project blob content ${Math.random()}` + const projectBlobKey = `${projectKey.format(historyId)}/${hash.slice(0, 2)}/${hash.slice(2)}` + await seedBlob( + Settings.filestore.stores.project_blobs, + projectBlobKey, + projectBlobContent + ) + }) + + it('should be able to get the blob back', async function () { + const response = await fetch( + `${filestoreUrl}/history/project/${historyId}/hash/${hash}` + ) + const body = await response.text() + expect(body).to.equal(projectBlobContent) + }) + + it('should return 404 for an unknown hash', async function () { + const response = await fetch( + `${filestoreUrl}/history/project/${historyId}/hash/${randomHash()}` + ) + expect(response.status).to.equal(404) + }) + }) + }) + + // handleValidationError() (@overleaf/validation-tools) composes the + // body as { error: fromError(zodError).toString(), statusCode }; + // fromError() renders each failing issue with its dotted schema path + // (e.g. `at "params.template_id"`). Asserting on the body (not just + // the status code) confirms the *intended* field is what rejected the + // request, not some unrelated bug that happens to also return the + // same status. This only needs to run once (per backend, for shard + // coverage) since validation happens before any persistor call. + describe('validation', function () { + it('should return 404 for a malformed template id', async function () { + const response = await fetch( + `${filestoreUrl}/template/not-an-object-id/v/0/${fileId}` + ) + const body = await response.text() + expectValidationError({ response, body }, 404, 'template_id') + }) + + it('should return 404 for a path traversal payload in the format segment', async function () { + // encodeURIComponent so the traversal payload arrives as a + // literal path segment (the value of params.format) instead of + // being collapsed by fetch()/URL's dot-segment normalization + // before the request reaches the wire, or splitting into extra + // path segments that wouldn't match this route at all. + const response = await fetch( + `${filestoreUrl}/template/${templateId}/v/0/${encodeURIComponent('../../etc/passwd')}` + ) + const body = await response.text() + expectValidationError({ response, body }, 404, 'format') + }) + + it('should return 404 for a malformed hash on a global blob', async function () { + const response = await fetch( + `${filestoreUrl}/history/global/hash/not-a-valid-hash` + ) + const body = await response.text() + expectValidationError({ response, body }, 404, 'hash') + }) + + it('should return 404 for a malformed historyId on a project blob', async function () { + const response = await fetch( + `${filestoreUrl}/history/project/not-an-object-id/hash/${'a'.repeat(40)}` + ) + const body = await response.text() + expectValidationError({ response, body }, 404, 'historyId') + }) + }) + describe('with multiple files', function () { let fileIds, fileUrls, otherFileUrls, otherProjectUrl const localFileReadPaths = [ diff --git a/services/filestore/test/acceptance/js/TestConfig.js b/services/filestore/test/acceptance/js/TestConfig.js index 1807c22a5c..1f441796b1 100644 --- a/services/filestore/test/acceptance/js/TestConfig.js +++ b/services/filestore/test/acceptance/js/TestConfig.js @@ -38,10 +38,22 @@ function s3SSECConfig() { automaticallyRotateDEKEncryption: true, dataEncryptionKeyBucketName: process.env.AWS_S3_USER_FILES_DEK_BUCKET_NAME, pathToProjectFolder(_bucketName, path) { - const match = path.match(/^[a-f0-9]{24}\//) - if (!match) throw new Error('not a project-folder') - const [projectFolder] = match - return projectFolder + // template_files keys are `templateId/v/version/fileId` (literal + // 24-hex prefix). project_blobs keys are + // `projectKey.format(historyId)/hash-prefix/hash-rest`, i.e. a + // reversed, zero-padded historyId split into 3/3/rest digits (numeric + // legacy ids) or 3/3/18 hex chars (reversed ObjectId hex) -- both + // stores share this test bucket, so accept either shape. See + // @overleaf/object-persistor/src/ProjectKey.js and + // history-v1/storage/lib/backupPersistor.mjs's PROJECT_FOLDER_REGEX + // for the same distinction in production. + const rawId = path.match(/^[a-f0-9]{24}\//) + if (rawId) return rawId[0] + const reversedId = path.match( + /^(?:\d{3}\/\d{3}\/\d{3,}|[a-f0-9]{3}\/[a-f0-9]{3}\/[a-f0-9]{18})\// + ) + if (reversedId) return reversedId[0] + throw new Error('not a project-folder') }, async getRootKeyEncryptionKeys() { return S3SSECKeys @@ -60,8 +72,15 @@ function s3ConfigDefaultProviderCredentials() { } function s3Stores() { + // project_blobs/global_blobs reuse the template_files bucket in tests -- + // buckets are just opaque containers keyed by an unrelated namespace, so + // this is enough to exercise the routing/key-building/validation for the + // history blob routes without provisioning dedicated test buckets. + const templateFiles = process.env.AWS_S3_TEMPLATE_FILES_BUCKET_NAME return { - template_files: process.env.AWS_S3_TEMPLATE_FILES_BUCKET_NAME, + template_files: templateFiles, + project_blobs: templateFiles, + global_blobs: templateFiles, } } @@ -78,17 +97,26 @@ function gcsConfig() { } function gcsStores() { + // see s3Stores() above: reuse the same bucket for the history blob stores. + const templateFiles = process.env.GCS_TEMPLATE_FILES_BUCKET_NAME return { - template_files: process.env.GCS_TEMPLATE_FILES_BUCKET_NAME, + template_files: templateFiles, + project_blobs: templateFiles, + global_blobs: templateFiles, } } function fsStores() { + // see s3Stores() above: reuse the same directory for the history blob + // stores. + const templateFiles = Path.resolve( + import.meta.dirname, + '../../../template_files' + ) return { - template_files: Path.resolve( - import.meta.dirname, - '../../../template_files' - ), + template_files: templateFiles, + project_blobs: templateFiles, + global_blobs: templateFiles, } } diff --git a/services/filestore/test/unit/js/FileController.test.js b/services/filestore/test/unit/js/FileController.test.js index 0de2685707..07d3285bb3 100644 --- a/services/filestore/test/unit/js/FileController.test.js +++ b/services/filestore/test/unit/js/FileController.test.js @@ -99,7 +99,8 @@ describe('FileController', () => { }) it('should send a 200 if the cacheWarm param is true', async () => { - req.query.cacheWarm = true + // real query values always arrive as strings + req.query = { cacheWarm: 'true' } await new Promise(resolve => { res.sendStatus = statusCode => { expect(statusCode).to.equal(200) diff --git a/services/filestore/test/unit/js/schemas.test.js b/services/filestore/test/unit/js/schemas.test.js new file mode 100644 index 0000000000..e915592540 --- /dev/null +++ b/services/filestore/test/unit/js/schemas.test.js @@ -0,0 +1,27 @@ +import { describe, it, expect } from 'vitest' +import { bucketFileParamsSchema } from '../../../app/js/schemas.js' + +describe('schemas', function () { + describe('bucketFileParamsSchema', function () { + it('accepts a well-formed bucket name', function () { + const result = bucketFileParamsSchema.safeParse({ + params: { bucket: 'my-bucket-123', 0: 'some/key' }, + }) + expect(result.success).to.equal(true) + }) + + it('rejects a bucket name with disallowed characters', function () { + const result = bucketFileParamsSchema.safeParse({ + params: { bucket: 'My_Bucket!', 0: 'some/key' }, + }) + expect(result.success).to.equal(false) + }) + + it('rejects an empty bucket name', function () { + const result = bucketFileParamsSchema.safeParse({ + params: { bucket: '', 0: 'some/key' }, + }) + expect(result.success).to.equal(false) + }) + }) +}) diff --git a/yarn.lock b/yarn.lock index 697532aafd..07c44036f3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7249,6 +7249,7 @@ __metadata: "@overleaf/object-persistor": "workspace:*" "@overleaf/settings": "workspace:*" "@overleaf/stream-utils": "npm:^0.1.0" + "@overleaf/validation-tools": "workspace:*" "@types/node": "npm:^24.5.2" bunyan: "npm:^1.8.15" chai: "npm:^4.3.6" @@ -7262,6 +7263,7 @@ __metadata: mocha-multi-reporters: "npm:^1.5.1" mongodb: "npm:6.12.0" node-fetch: "npm:^2.7.0" + overleaf-editor-core: "workspace:*" range-parser: "npm:^1.2.1" sinon: "npm:9.0.2" sinon-chai: "npm:^3.7.0"