diff --git a/CLAUDE.md b/CLAUDE.md index bc301c96..69808b5a 100644 --- a/CLAUDE.md +++ b/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()`, diff --git a/backend/api/assets.ts b/backend/api/assets.ts index 103f9d78..25dd4ab2 100644 --- a/backend/api/assets.ts +++ b/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() + 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, diff --git a/backend/api/users.ts b/backend/api/users.ts index 54848cc2..73e1c325 100644 --- a/backend/api/users.ts +++ b/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) { diff --git a/backend/index.ts b/backend/index.ts index 04d1f31a..6da6f1fa 100644 --- a/backend/index.ts +++ b/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 }) // ---------------------------------------- diff --git a/backend/types/fastify.d.ts b/backend/types/fastify.d.ts index 1a4a076b..bda007de 100644 --- a/backend/types/fastify.d.ts +++ b/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. */