diff --git a/backend/src/imageUpload.ts b/backend/src/imageUpload.ts new file mode 100644 index 0000000..d75dd0d --- /dev/null +++ b/backend/src/imageUpload.ts @@ -0,0 +1,286 @@ +/** + * The one validated path from a multipart request to files on the uploads + * volume. + * + * Extracted from routes/admin.ts when a second caller appeared (#222's public + * intake endpoint). It is deliberately one module rather than two similar + * ones: every property that makes an upload safe here — the type allowlist, + * the magic-byte check after the write, names from a CSPRNG rather than from + * `originalname`, the re-encode that strips EXIF, and the cleanup of whatever + * a refused request left behind — is a property a second implementation would + * have to reproduce exactly. A near-copy that drifted would be precisely the + * gap #95, #103, #180 and #226 exist to close. + * + * Nothing below changed in the move. The comments came with it, because they + * record why the code is shaped as it is and are the most valuable part of it. + */ + +import { Request, Response, NextFunction } from 'express'; +import multer from 'multer'; +import { promises as fs } from 'fs'; +import { randomUUID } from 'crypto'; +import { PoolClient } from 'pg'; +import { + ALLOWED_IMAGE_TYPES, + SIGNATURE_BYTES, + extensionFor, + isAllowedImageType, + signatureMatches +} from './uploadTypes'; +import { reencodeInPlace } from './imageProcessing'; + +const UPLOADS_DIR = process.env.UPLOADS_DIR || '/app/uploads'; + +// Multer writes to disk with no size cap unless one is given, so a single +// request could fill the uploads volume. Bound every dimension of the +// multipart body: image count, bytes per image, and the small text fields +// (name/description/price) that accompany them. +const MAX_IMAGES_PER_REQUEST = 6; +// 8 MB, not 8 MiB — this is the ceiling S5693 treats as safe, and 8 * 1024 * +// 1024 sits just over it. Plenty for a product photo either way. +const MAX_IMAGE_BYTES = 8_000_000; +const MAX_TEXT_FIELDS = 8; +const MAX_TEXT_FIELD_BYTES = 64 * 1024; + +// Refused before a byte is written. This catches the honest mistake — picking a +// PDF by accident — and nothing more, because file.mimetype is whatever the +// caller wrote in the multipart headers. The bytes are checked after the write; +// see verifyUploadedImages. +class UnsupportedImageTypeError extends Error {} + +const storage = multer.diskStorage({ + destination: UPLOADS_DIR, + // Stored names come from a CSPRNG rather than a timestamp plus Math.random, + // which is predictable enough that a caller could guess (or collide with) + // another upload's path. + // + // The extension comes from the validated content type rather than from + // path.extname(file.originalname), so the name on disk cannot disagree with + // what the file claims to be — a caller cannot get `.html` onto the uploads + // volume by naming their file that way. + filename: (_req, file, cb) => { + const ext = extensionFor(file.mimetype); + if (!ext) { + // Unreachable while fileFilter runs first, and here so that it stays + // unreachable rather than silently writing a file with no extension. + cb(new UnsupportedImageTypeError(`unsupported image type ${file.mimetype}`), ''); + return; + } + cb(null, `${randomUUID()}${ext}`); + } +}); + +// Reviewed for #180. Bounding one request is only half the problem — see +// discardUnlessAccepted below for the other half, which is bounding what the +// volume accumulates across requests that were refused. +const upload = multer({ + storage, + limits: { + fileSize: MAX_IMAGE_BYTES, + files: MAX_IMAGES_PER_REQUEST, + fields: MAX_TEXT_FIELDS, + fieldSize: MAX_TEXT_FIELD_BYTES + }, + fileFilter: (_req, file, cb) => { + if (!isAllowedImageType(file.mimetype)) { + cb(new UnsupportedImageTypeError( + `${file.mimetype} is not an accepted image type — allowed: ${ALLOWED_IMAGE_TYPES.join(', ')}` + )); + return; + } + cb(null, true); + } +}); + +// Reads only the leading bytes — enough to identify a format, not enough to +// care how large the file is. The handle is closed before anything is unlinked, +// because an open handle makes the unlink fail on Windows. +// +// Reviewed for #180. The path is not caller-controlled despite arriving from a +// request: multer composes it from `destination`, which is a server constant, +// and `filename`, which the storage above sets to `randomUUID()` plus an +// extension looked up from the validated content type. The caller's +// `originalname` is never consulted, so no part of the path traverses anywhere. +async function readHead(filePath: string): Promise { + const handle = await fs.open(filePath, 'r'); + try { + const buffer = Buffer.alloc(SIGNATURE_BYTES); + const { bytesRead } = await handle.read(buffer, 0, SIGNATURE_BYTES, 0); + return buffer.subarray(0, bytesRead); + } finally { + await handle.close(); + } +} + +// Best effort: a file that cannot be removed should not turn a 400 into a 500, +// but it must not be left behind quietly either. +async function discardUploads(files: Express.Multer.File[]): Promise { + await Promise.all( + files.map((file) => + fs.unlink(file.path).catch((err: unknown) => { + console.error(`[upload] could not remove rejected file ${file.path}:`, err); + }) + ) + ); +} + +/** + * Removes a request's uploaded files unless the request actually succeeded. + * + * multer writes to disk before any route logic runs, and its own cleanup only + * covers errors it raised itself. Everything after that — a failed signature + * check, a malformed `category_id`, a database error, a dropped connection — + * previously left the bytes on the volume with nothing referencing them: no row + * to find them by, and no bound on how many could accumulate. Bounding the size + * of one upload does not help if every refused upload is kept forever (#180). + * + * Registered as soon as multer succeeds rather than at each `return`, so a + * route added later inherits it instead of having to remember it. That is the + * whole reason it is a hook and not a call: the failure it prevents is someone + * adding a fourth early return. + * + * `close` rather than `finish`, so an aborted connection is covered too, and + * `writableEnded` distinguishes a response that completed from one that never + * did — the latter is not a success however its status code reads. + */ +function discardUnlessAccepted(req: Request, res: Response): void { + res.on('close', () => { + if (res.writableEnded && res.statusCode < 400) return; + void discardUploads((req.files as Express.Multer.File[]) || []); + }); +} + +/** + * Confirms each stored file actually is what it was declared to be. + * + * This cannot happen in multer's fileFilter, which runs before the stream has + * been read — there are no bytes to look at yet. So the check runs after the + * write. + * + * Checking only: removing the files is discardUnlessAccepted's job, and doing + * it here as well would unlink twice and log an ENOENT for every refused + * upload. That also covers the case this function used to miss — `readHead` + * itself throwing, which returned no message and so cleaned up nothing. + * + * Returns the message to refuse with, or null when everything checks out. + */ +async function verifyUploadedImages(req: Request): Promise { + const files = (req.files as Express.Multer.File[]) || []; + + for (const file of files) { + const head = await readHead(file.path); + if (!signatureMatches(file.mimetype, head)) { + return `${file.originalname} does not contain ${file.mimetype} data`; + } + } + + return null; +} + +/** + * Rebuilds every accepted file so it carries no metadata (#226). + * + * After verification, deliberately: re-encoding a file whose bytes do not match + * its declared type would be doing work on something already refused, and + * sharp's own error would replace the clearer message that check produces. + * + * A failure here refuses the upload rather than storing the original. Storing + * it would mean the one case where a photo keeps the coordinates it was taken + * at is the case nobody was told about. + * + * Returns the message to refuse with, or null when every file was rebuilt. + */ +async function stripUploadedImages(req: Request): Promise { + const files = (req.files as Express.Multer.File[]) || []; + + for (const file of files) { + try { + await reencodeInPlace(file.path, file.mimetype); + } catch (err) { + console.error(`[upload] could not re-encode ${file.path}:`, err); + return `${file.originalname} could not be processed`; + } + } + + return null; +} + +// No error-handling middleware is mounted on the app, so translate multer's +// limit errors here instead of letting them surface as a generic 500. +const uploadImages = (req: Request, res: Response, next: NextFunction) => { + upload.array('images', MAX_IMAGES_PER_REQUEST)(req, res, (err: unknown) => { + if (err instanceof UnsupportedImageTypeError) { + return res.status(400).json({ error: err.message }); + } + if (err instanceof multer.MulterError) { + const status = err.code === 'LIMIT_FILE_SIZE' ? 413 : 400; + return res.status(status).json({ error: err.message }); + } + if (err) { + return next(err); + } + + // Every file is on disk by this point and multer will not clean up after + // itself again, so the bytes become this request's responsibility before + // anything else is allowed to fail. + discardUnlessAccepted(req, res); + + verifyUploadedImages(req) + .then((problem) => { + if (problem) { + res.status(400).json({ error: problem }); + return null; + } + return stripUploadedImages(req); + }) + .then((problem) => { + // The first stage returns null both when it answered and when it found + // nothing wrong, so the response itself is what distinguishes them. + if (res.headersSent) return; + if (problem) { + res.status(400).json({ error: problem }); + return; + } + next(); + }) + .catch(next); + }); +}; + +/** + * Records uploaded files as an item's images. + * + * Create and update wrote this loop separately, differing only in where the id + * came from and where the sort order started — zero for a new item, one past + * the current maximum for an existing one. Both are parameters now. + * + * It also means the `/uploads/` prefix is written once. That matters more than + * it looks: #103 made the stored value the path `uploadUrl` joins an origin + * onto, so it is a contract rather than a string, and two places to change it + * is one place to forget. + */ +async function insertItemImages( + client: PoolClient, + itemId: number, + files: Express.Multer.File[], + firstSortOrder: number +): Promise { + // Iterated by entry rather than by index, so there is no possibly-undefined + // element to guard — the create path used to fall back to an empty filename, + // which would have stored a path pointing at the uploads directory itself. + for (const [offset, file] of files.entries()) { + await client.query( + `INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, $3)`, + [itemId, `/uploads/${file.filename}`, firstSortOrder + offset] + ); + } +} + +export { + uploadImages, + verifyUploadedImages, + stripUploadedImages, + insertItemImages, + MAX_IMAGES_PER_REQUEST, + MAX_IMAGE_BYTES +}; diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index 8f333a8..0ad0f70 100755 --- a/backend/src/routes/admin.ts +++ b/backend/src/routes/admin.ts @@ -1,7 +1,4 @@ -import { Router, Request, Response, NextFunction } from 'express'; -import multer from 'multer'; -import { promises as fs } from 'fs'; -import { randomUUID } from 'crypto'; +import { Router, Request, Response } from 'express'; import { PoolClient } from 'pg'; import { pool, requireRow } from '../db'; import { ADMIN_ITEM_SELECT, AdminItemRow, ItemRecord } from '../itemSelect'; @@ -9,15 +6,13 @@ import { ItemStatus } from '../types'; import { asyncRoute } from '../asyncRoute'; import { parseItemFilters, buildItemFilterSql, FilterError } from '../itemFilters'; import { tagColorFor } from '../utils'; -import { - ALLOWED_IMAGE_TYPES, - SIGNATURE_BYTES, - extensionFor, - isAllowedImageType, - signatureMatches -} from '../uploadTypes'; import { notifyFavoritersOfSale, notifyFavoritersOfRemoval, collectFavoriteRecipients } from '../favoriteAlerts'; -import { reencodeInPlace } from '../imageProcessing'; +// The upload pipeline moved to src/imageUpload.ts when #222's public intake +// endpoint became a second caller. Mounting uploadImages gets the type +// allowlist, the magic-byte check, and the EXIF-stripping re-encode together — +// which is the point of it being one module rather than something each route +// assembles for itself. +import { uploadImages, insertItemImages } from '../imageUpload'; const router = Router(); @@ -31,223 +26,6 @@ interface ItemStatusRow { status: ItemStatus; } -const UPLOADS_DIR = process.env.UPLOADS_DIR || '/app/uploads'; - -// Multer writes to disk with no size cap unless one is given, so a single -// request could fill the uploads volume. Bound every dimension of the -// multipart body: image count, bytes per image, and the small text fields -// (name/description/price) that accompany them. -const MAX_IMAGES_PER_REQUEST = 6; -// 8 MB, not 8 MiB — this is the ceiling S5693 treats as safe, and 8 * 1024 * -// 1024 sits just over it. Plenty for a product photo either way. -const MAX_IMAGE_BYTES = 8_000_000; -const MAX_TEXT_FIELDS = 8; -const MAX_TEXT_FIELD_BYTES = 64 * 1024; - -// Refused before a byte is written. This catches the honest mistake — picking a -// PDF by accident — and nothing more, because file.mimetype is whatever the -// caller wrote in the multipart headers. The bytes are checked after the write; -// see verifyUploadedImages. -class UnsupportedImageTypeError extends Error {} - -const storage = multer.diskStorage({ - destination: UPLOADS_DIR, - // Stored names come from a CSPRNG rather than a timestamp plus Math.random, - // which is predictable enough that a caller could guess (or collide with) - // another upload's path. - // - // The extension comes from the validated content type rather than from - // path.extname(file.originalname), so the name on disk cannot disagree with - // what the file claims to be — a caller cannot get `.html` onto the uploads - // volume by naming their file that way. - filename: (_req, file, cb) => { - const ext = extensionFor(file.mimetype); - if (!ext) { - // Unreachable while fileFilter runs first, and here so that it stays - // unreachable rather than silently writing a file with no extension. - cb(new UnsupportedImageTypeError(`unsupported image type ${file.mimetype}`), ''); - return; - } - cb(null, `${randomUUID()}${ext}`); - } -}); - -// Reviewed for #180. Bounding one request is only half the problem — see -// discardUnlessAccepted below for the other half, which is bounding what the -// volume accumulates across requests that were refused. -const upload = multer({ - storage, - limits: { - fileSize: MAX_IMAGE_BYTES, - files: MAX_IMAGES_PER_REQUEST, - fields: MAX_TEXT_FIELDS, - fieldSize: MAX_TEXT_FIELD_BYTES - }, - fileFilter: (_req, file, cb) => { - if (!isAllowedImageType(file.mimetype)) { - cb(new UnsupportedImageTypeError( - `${file.mimetype} is not an accepted image type — allowed: ${ALLOWED_IMAGE_TYPES.join(', ')}` - )); - return; - } - cb(null, true); - } -}); - -// Reads only the leading bytes — enough to identify a format, not enough to -// care how large the file is. The handle is closed before anything is unlinked, -// because an open handle makes the unlink fail on Windows. -// -// Reviewed for #180. The path is not caller-controlled despite arriving from a -// request: multer composes it from `destination`, which is a server constant, -// and `filename`, which the storage above sets to `randomUUID()` plus an -// extension looked up from the validated content type. The caller's -// `originalname` is never consulted, so no part of the path traverses anywhere. -async function readHead(filePath: string): Promise { - const handle = await fs.open(filePath, 'r'); - try { - const buffer = Buffer.alloc(SIGNATURE_BYTES); - const { bytesRead } = await handle.read(buffer, 0, SIGNATURE_BYTES, 0); - return buffer.subarray(0, bytesRead); - } finally { - await handle.close(); - } -} - -// Best effort: a file that cannot be removed should not turn a 400 into a 500, -// but it must not be left behind quietly either. -async function discardUploads(files: Express.Multer.File[]): Promise { - await Promise.all( - files.map((file) => - fs.unlink(file.path).catch((err: unknown) => { - console.error(`[upload] could not remove rejected file ${file.path}:`, err); - }) - ) - ); -} - -/** - * Removes a request's uploaded files unless the request actually succeeded. - * - * multer writes to disk before any route logic runs, and its own cleanup only - * covers errors it raised itself. Everything after that — a failed signature - * check, a malformed `category_id`, a database error, a dropped connection — - * previously left the bytes on the volume with nothing referencing them: no row - * to find them by, and no bound on how many could accumulate. Bounding the size - * of one upload does not help if every refused upload is kept forever (#180). - * - * Registered as soon as multer succeeds rather than at each `return`, so a - * route added later inherits it instead of having to remember it. That is the - * whole reason it is a hook and not a call: the failure it prevents is someone - * adding a fourth early return. - * - * `close` rather than `finish`, so an aborted connection is covered too, and - * `writableEnded` distinguishes a response that completed from one that never - * did — the latter is not a success however its status code reads. - */ -function discardUnlessAccepted(req: Request, res: Response): void { - res.on('close', () => { - if (res.writableEnded && res.statusCode < 400) return; - void discardUploads((req.files as Express.Multer.File[]) || []); - }); -} - -/** - * Confirms each stored file actually is what it was declared to be. - * - * This cannot happen in multer's fileFilter, which runs before the stream has - * been read — there are no bytes to look at yet. So the check runs after the - * write. - * - * Checking only: removing the files is discardUnlessAccepted's job, and doing - * it here as well would unlink twice and log an ENOENT for every refused - * upload. That also covers the case this function used to miss — `readHead` - * itself throwing, which returned no message and so cleaned up nothing. - * - * Returns the message to refuse with, or null when everything checks out. - */ -async function verifyUploadedImages(req: Request): Promise { - const files = (req.files as Express.Multer.File[]) || []; - - for (const file of files) { - const head = await readHead(file.path); - if (!signatureMatches(file.mimetype, head)) { - return `${file.originalname} does not contain ${file.mimetype} data`; - } - } - - return null; -} - -/** - * Rebuilds every accepted file so it carries no metadata (#226). - * - * After verification, deliberately: re-encoding a file whose bytes do not match - * its declared type would be doing work on something already refused, and - * sharp's own error would replace the clearer message that check produces. - * - * A failure here refuses the upload rather than storing the original. Storing - * it would mean the one case where a photo keeps the coordinates it was taken - * at is the case nobody was told about. - * - * Returns the message to refuse with, or null when every file was rebuilt. - */ -async function stripUploadedImages(req: Request): Promise { - const files = (req.files as Express.Multer.File[]) || []; - - for (const file of files) { - try { - await reencodeInPlace(file.path, file.mimetype); - } catch (err) { - console.error(`[upload] could not re-encode ${file.path}:`, err); - return `${file.originalname} could not be processed`; - } - } - - return null; -} - -// No error-handling middleware is mounted on the app, so translate multer's -// limit errors here instead of letting them surface as a generic 500. -const uploadImages = (req: Request, res: Response, next: NextFunction) => { - upload.array('images', MAX_IMAGES_PER_REQUEST)(req, res, (err: unknown) => { - if (err instanceof UnsupportedImageTypeError) { - return res.status(400).json({ error: err.message }); - } - if (err instanceof multer.MulterError) { - const status = err.code === 'LIMIT_FILE_SIZE' ? 413 : 400; - return res.status(status).json({ error: err.message }); - } - if (err) { - return next(err); - } - - // Every file is on disk by this point and multer will not clean up after - // itself again, so the bytes become this request's responsibility before - // anything else is allowed to fail. - discardUnlessAccepted(req, res); - - verifyUploadedImages(req) - .then((problem) => { - if (problem) { - res.status(400).json({ error: problem }); - return null; - } - return stripUploadedImages(req); - }) - .then((problem) => { - // The first stage returns null both when it answered and when it found - // nothing wrong, so the response itself is what distinguishes them. - if (res.headersSent) return; - if (problem) { - res.status(400).json({ error: problem }); - return; - } - next(); - }) - .catch(next); - }); -}; // The multipart body carries category_id and tags as text fields. An absent // field means "leave as-is" on update, which is why these return undefined @@ -305,34 +83,6 @@ async function setItemTags(client: PoolClient, itemId: number, tagIds: number[]) } } -/** - * Records uploaded files as an item's images. - * - * Create and update wrote this loop separately, differing only in where the id - * came from and where the sort order started — zero for a new item, one past - * the current maximum for an existing one. Both are parameters now. - * - * It also means the `/uploads/` prefix is written once. That matters more than - * it looks: #103 made the stored value the path `uploadUrl` joins an origin - * onto, so it is a contract rather than a string, and two places to change it - * is one place to forget. - */ -async function insertItemImages( - client: PoolClient, - itemId: number, - files: Express.Multer.File[], - firstSortOrder: number -): Promise { - // Iterated by entry rather than by index, so there is no possibly-undefined - // element to guard — the create path used to fall back to an empty filename, - // which would have stored a path pointing at the uploads directory itself. - for (const [offset, file] of files.entries()) { - await client.query( - `INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, $3)`, - [itemId, `/uploads/${file.filename}`, firstSortOrder + offset] - ); - } -} /** * The two optional fields the item form submits as multipart text.