fix: prevent unauthorized asset uploads to consume memory

CLAUDE.md

@@ -407,7 +407,7 @@ settings table, so they have no static shape. `index.ts` and `worker.ts` build t
 literal and assert it to `WikiGlobal`, since each populates the object progressively.
 
 `backend/types/fastify.d.ts` augments Fastify: session fields (`authenticated`, `user`,
-`permissions`) and the per-route `config.permissions` used by the `preHandler` permission hook.
+`permissions`) and the per-route `config.permissions` used by the `preParsing` permission hook.
 
 **Four dynamic paths are extension-sensitive** and invisible to the type checker — they must be
 updated by hand if the files they point at are ever renamed:
@@ -643,11 +643,18 @@ Consequences worth knowing:
   configs), `WIKI.ROOTPATH`, `WIKI.SERVERPATH`, `WIKI.INSTANCE_ID`.
 - **Routes** are Fastify plugins: `async function routes(app) { ... }` with a default export.
 - **Permissions** are declared per-route in `config.permissions`, and enforced by a single
-  `preHandler` hook in `index.ts`. The array is OR-ed; a nested array is AND-ed
+  `preParsing` hook in `index.ts`. The array is OR-ed; a nested array is AND-ed
   (`permissions: ['read:sites', ['manage:users', 'manage:groups']]`). `manage:system` bypasses every
   check. `@fastify/swagger`'s `transform` folds these into the OpenAPI description automatically —
   so declaring them is also how they get documented. Only **global** permissions belong here; see
   [Permissions](#permissions) for the other kind and how they are checked.
+- **A route that takes an upload refuses before reading it.** Fastify buffers a body in full before
+  `preValidation`, let alone the handler, so a check written in the handler lets anybody make the
+  server hold `bodyLimit` bytes per request just to be told no. `config.permissions` is already
+  checked in time. A route checking in its handler instead (a page rule, a profile setting) does the
+  check in a route-level `preParsing` hook, as the asset upload and avatar routes do. That hook runs
+  on the query string BEFORE schema validation, so it has to cope with a missing or malformed value
+  rather than trust the schema.
 - **Every route needs a `schema`** with `summary`, `tags`, and response schemas. `hideUntagged` is on,
   so an untagged route is invisible in the API docs. Reuse `$ref` schemas from `api/schemas/`.
 - **Errors** via `@fastify/sensible` helpers (`reply.notFound()`, `reply.badRequest()`,

backend/api/assets.ts

@@ -1,4 +1,5 @@
 import type { FastifyInstance, FastifyRequest } from 'fastify'
+import { validate as uuidValidate } from 'uuid'
 
 import { audit } from '../helpers/audit.ts'
 import { attachmentDisposition, decodeTreePath, normalizeFolderPath } from '../helpers/common.ts'
@@ -51,6 +52,18 @@ function mayOnAsset(
   })
 }
 
+/**
+ * Where an upload is going and who is sending it, settled by the upload route's `preParsing` hook and
+ * read back by its handler, so that the write goes exactly where the permission check looked.
+ */
+interface UploadTarget {
+  authorId: string
+  folderPath: string | null
+  destination: string
+  locale: string
+}
+const uploadTargets = new WeakMap<FastifyRequest, UploadTarget>()
+
 async function routes(app: FastifyInstance) {
   // -> An upload is the raw file rather than a multipart form: one file per request, with the name and
   //    the destination in the query string. The catch-all only claims content types nothing else
@@ -78,8 +91,56 @@ async function routes(app: FastifyInstance) {
     {
       /*
         No route-level `permissions`: that hook reads the group-wide list, and asset permissions come
-        from a group's RULES, which address the folder the file is in. Checked below.
+        from a group's RULES, which address the folder the file is in.
+
+        Checked here, before the body is parsed rather than in the handler: parsing buffers the whole
+        file into memory, so a check that waited for it would let anybody — signed in or not — make
+        the server hold `uploadMaxFileSize` per request only to be refused. Which also means the query
+        string has not been through the schema yet, so whatever the check reads is checked here too.
       */
+      preParsing: async (req, reply, payload) => {
+        // -> An asset records who uploaded it, and an API key is not a who
+        const authorId = req.session?.authenticated ? req.session.user?.id : null
+        if (!authorId) {
+          throw app.httpErrors.unauthorized('Uploading an asset requires a logged in user.')
+        }
+        const { fileName, folderId, folderPath: requestedPath, locale: requestedLocale } = req.query
+        if (
+          typeof fileName !== 'string' ||
+          !fileName ||
+          (folderId !== undefined && (typeof folderId !== 'string' || !uuidValidate(folderId))) ||
+          (requestedPath !== undefined && typeof requestedPath !== 'string') ||
+          (requestedLocale !== undefined && typeof requestedLocale !== 'string')
+        ) {
+          throw app.httpErrors.badRequest('Invalid upload destination.')
+        }
+
+        /*
+          Where this is going, as a path, which is what a rule addresses. An ID has to be looked up to
+          get one; a path is already one, and is normalized here rather than trusted -- the model
+          would happily create a folder called `..`.
+        */
+        const folder = folderId ? await WIKI.models.tree.getFolderById(folderId) : null
+        const folderPath = folderId ? null : normalizeFolderPath(requestedPath)
+        const parentPath = folder ? decodeTreePath(folder.folderPath) : ''
+        const destination = folder
+          ? [parentPath, folder.fileName].filter(Boolean).join('/')
+          : (folderPath ?? '')
+        // -> Settled once, since the rule check and the write have to be asking about the same locale
+        const locale =
+          requestedLocale ?? WIKI.sites[req.params.siteId]?.config?.locales?.primary ?? 'en'
+        if (
+          !mayOnAsset(req, 'write:assets', {
+            folderPath: destination,
+            fileName,
+            locale
+          })
+        ) {
+          throw app.httpErrors.forbidden('You are not allowed to upload a file here.')
+        }
+        uploadTargets.set(req, { authorId, folderPath, destination, locale })
+        return payload
+      },
       schema: {
         summary: 'Upload an asset',
         description: `The body is the file itself, not a multipart form — send the bytes with their \`Content-Type\`. At most ${Math.round((WIKI.config.security?.uploadMaxFileSize ?? 10485760) / 1024 / 1024)} MB. The file name is sanitized, so the stored name in the response may differ from the one sent; the type served back later comes from that name's extension rather than from the request. Images get a thumbnail when the Sharp extension is installed.\n\nA file already at that name in that folder is settled by the site's upload conflict behavior: \`overwrite\` (the default) replaces it in place and answers with its existing ID, \`reject\` answers 409, and \`new\` stores the arrival as the next free \`name-1.ext\`. So the name and ID in the response are what to link to — never the ones that were sent. A page or a folder holding the name is answered 409 whichever behavior is set.`,
@@ -140,41 +201,11 @@ async function routes(app: FastifyInstance) {
       }
     },
     async (req, reply) => {
-      // -> An asset records who uploaded it, and an API key is not a who
-      const authorId = req.session?.authenticated ? req.session.user?.id : null
-      if (!authorId) {
-        return reply.unauthorized('Uploading an asset requires a logged in user.')
-      }
       const data = req.body
       if (!Buffer.isBuffer(data) || data.length < 1) {
         return reply.badRequest('No file was sent.')
       }
-
-      /*
-        Where this is going, as a path, which is what a rule addresses. An ID has to be looked up to
-        get one; a path is already one, and is normalized here rather than trusted -- the model would
-        happily create a folder called `..`.
-      */
-      const folder = req.query.folderId
-        ? await WIKI.models.tree.getFolderById(req.query.folderId)
-        : null
-      const folderPath = req.query.folderId ? null : normalizeFolderPath(req.query.folderPath)
-      const parentPath = folder ? decodeTreePath(folder.folderPath) : ''
-      const destination = folder
-        ? [parentPath, folder.fileName].filter(Boolean).join('/')
-        : (folderPath ?? '')
-      // -> Settled once, since the rule check and the write have to be asking about the same locale
-      const locale =
-        req.query.locale ?? WIKI.sites[req.params.siteId]?.config?.locales?.primary ?? 'en'
-      if (
-        !mayOnAsset(req, 'write:assets', {
-          folderPath: destination,
-          fileName: req.query.fileName,
-          locale
-        })
-      ) {
-        return reply.forbidden('You are not allowed to upload a file here.')
-      }
+      const { authorId, folderPath, destination, locale } = uploadTargets.get(req)!
       const asset = await WIKI.models.assets.upload({
         siteId: req.params.siteId,
         locale,

backend/api/users.ts

@@ -421,6 +421,17 @@ async function routes(app: FastifyInstance) {
   app.put(
     '/profile/avatar',
     {
+      // -> Before the body is parsed, which buffers the whole image: refusing afterwards would let
+      //    anybody make the server hold `avatarUploadLimit` per request only to be told no
+      preParsing: async (req, reply, payload) => {
+        if (!sessionUserId(req)) {
+          throw app.httpErrors.unauthorized()
+        }
+        if (!WIKI.models.authentication.isProfileEditingAllowed()) {
+          throw app.httpErrors.forbidden('Profile editing is disabled on this wiki.')
+        }
+        return payload
+      },
       schema: {
         summary: "Replace the logged in user's own avatar",
         description: `The body is the raw image, not a multipart form — send the file itself with its \`Content-Type\`. At most ${avatarUploadLimit / 1024 / 1024} MB, and it must really be one of the accepted formats: the bytes are checked, not the declared type. Resized to a 180x180 JPEG when the Sharp extension is installed, otherwise stored as uploaded. Requires profile editing to be enabled on this wiki (Administration → Authentication).`,
@@ -442,14 +453,8 @@ async function routes(app: FastifyInstance) {
         }
       }
     },
-    async (req, reply) => {
-      const userId = sessionUserId(req)
-      if (!userId) {
-        return reply.unauthorized()
-      }
-      if (!WIKI.models.authentication.isProfileEditingAllowed()) {
-        return reply.forbidden('Profile editing is disabled on this wiki.')
-      }
+    async (req) => {
+      const userId = sessionUserId(req)!
 
       const data = req.body
       if (!Buffer.isBuffer(data) || data.length < 1) {

backend/index.ts

@@ -608,7 +608,16 @@ async function initHTTPServer() {
   // Permissions
   // ----------------------------------------
 
-  app.addHook('preHandler', (req, reply, done) => {
+  /*
+    Before the body is parsed, not after: a parser buffers the whole body into memory, and the upload
+    routes accept megabytes (the import blob route, gigabytes) — so a check that waited for it would
+    let anybody make the server hold that much per request just to be told no. Everything this reads
+    is settled by the time `preParsing` runs, the session and the API key both being `onRequest`.
+
+    Thrown rather than sent, which is how an async `preParsing` hook ends a request — whatever it
+    resolves with is taken as the payload stream. The error reaches the same error handler either way.
+  */
+  app.addHook('preParsing', async (req, reply, payload) => {
     const routePermissions = req.routeOptions.config?.permissions
     if (routePermissions && routePermissions.length > 0) {
       // -> A verified API key stands in for a session, carrying the permissions of the groups it was
@@ -620,7 +629,7 @@ async function initHTTPServer() {
           : null
       // Unauthenticated / No Permissions
       if (!permissions || permissions.length < 1) {
-        return reply.unauthorized()
+        throw app.httpErrors.unauthorized()
       }
       // Is Root Admin?
       if (!permissions.includes('manage:system')) {
@@ -635,11 +644,11 @@ async function initHTTPServer() {
         })
         // Forbidden
         if (!isAllowed) {
-          return reply.forbidden()
+          throw app.httpErrors.forbidden()
         }
       }
     }
-    done()
+    return payload
   })
 
   // ----------------------------------------

backend/types/fastify.d.ts

@@ -89,7 +89,7 @@ declare module 'fastify' {
 
   interface FastifyContextConfig {
     /**
-     * Permissions required to reach the route, enforced by the `preHandler` hook in `index.ts`.
+     * Permissions required to reach the route, enforced by the `preParsing` hook in `index.ts`.
      *
      * The outer array is OR-ed; a nested array is AND-ed. `manage:system` bypasses the check.
      */