[filestore] adopt strict zod types and extend test coverage (#36112)

GitOrigin-RevId: a761477f2b412133da2d71748233c712c5728fae

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

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
 

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}`
 

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(),
+})

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"
   },

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 = [

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,
   }
 }
 

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)

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)
+    })
+  })
+})