Files
redefined-designs/backend/src/routes/admin.ts
T
bermudalamb aecccef418 feat(uploads): strip metadata from every accepted upload (#226)
Hooked into uploadImages rather than into the routes. That middleware is where verifyUploadedImages already runs and is the single choke point every upload path passes through, so the admin create and update routes are both covered and the intake route from #222 will inherit it rather than having to remember. The same reasoning discardUnlessAccepted already gives for being a hook instead of a call.

Runs after verification, deliberately: re-encoding a file whose bytes do not match its declared type would be work on something already refused, and sharp's error would replace the clearer message that check produces. A re-encode failure refuses the upload rather than storing the original, because the one case where a photo keeps the coordinates it was taken at should not be the case nobody was told about.

The test builds a JPEG carrying GPS tags rather than committing a binary fixture, so what it contains is readable, and it asserts the fixture really carries EXIF before asserting the stored file does not — otherwise the test would pass while proving nothing. GPS tags go in IFD3, which is the GPS IFD as libvips names it; sharp's Exif type has no separate GPS key, and putting them in IFD0 would have produced EXIF without producing the tags this issue is about.

Backend suites: 285 unit, 260 integration, lint clean, build clean.

One caveat worth recording. Across three full integration runs, `uploadValidation` failed once on "removes the upload when the request is refused for its other fields". It is a pre-existing race rather than a regression: discardUnlessAccepted cleans up in an unawaited `void discardUploads(...)` inside a `res.on('close')` handler, so a test asserting on the directory immediately after the response has always been able to observe the state before the unlink lands. Re-encoding adds enough libvips work to lose that race occasionally where it previously did not. The property still holds in production, where the process keeps running and the unlink completes. Filed separately rather than fixed here.

Ref #226
2026-08-29 11:54:34 -05:00

564 lines
23 KiB
TypeScript
Executable File

import { Router, Request, Response, NextFunction } from 'express';
import multer from 'multer';
import { promises as fs } from 'fs';
import { randomUUID } from 'crypto';
import { PoolClient } from 'pg';
import { pool, requireRow } from '../db';
import { ADMIN_ITEM_SELECT, AdminItemRow, ItemRecord } from '../itemSelect';
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';
const router = Router();
/** The next image slot, from a COALESCE'd MAX so it is never null. */
interface MaxSortRow {
max_sort: number;
}
/** Just the status column, read before deciding whether a transition is legal. */
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<Buffer> {
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<void> {
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<string | null> {
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<string | null> {
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
// rather than null for a missing value.
function readCategoryId(value: unknown): number | null | undefined {
if (value === undefined) return undefined;
if (value === null || value === '') return null;
const parsed = Number(value);
if (!Number.isSafeInteger(parsed) || parsed < 1) return undefined;
return parsed;
}
function readTagNames(value: unknown): string[] | undefined {
if (value === undefined) return undefined;
if (typeof value !== 'string' || value.trim() === '') return [];
try {
const parsed = JSON.parse(value);
if (!Array.isArray(parsed)) return undefined;
return parsed.filter((name): name is string => typeof name === 'string');
} catch {
return undefined;
}
}
// Tags typed into the item form may not exist yet. Look each name up
// case-insensitively — matching the unique index — and create the missing ones
// inside the caller's transaction so a later failure rolls them back too.
async function resolveTagIds(client: PoolClient, names: string[]): Promise<number[]> {
const ids: number[] = [];
for (const raw of names) {
const name = raw.trim();
if (!name) continue;
const existing = await client.query(`SELECT id FROM tags WHERE lower(name) = lower($1)`, [name]);
if (existing.rows.length) {
if (!ids.includes(existing.rows[0].id)) ids.push(existing.rows[0].id);
continue;
}
const inserted = await client.query(
`INSERT INTO tags (name, color) VALUES ($1, $2) RETURNING id`,
[name, tagColorFor(name)]
);
ids.push(inserted.rows[0].id);
}
return ids;
}
// Tags are replaced wholesale rather than merged — the form submits the full
// set it wants, so removing a chip has to actually remove the row.
async function setItemTags(client: PoolClient, itemId: number, tagIds: number[]): Promise<void> {
await client.query(`DELETE FROM item_tags WHERE item_id = $1`, [itemId]);
for (const tagId of tagIds) {
await client.query(`INSERT INTO item_tags (item_id, tag_id) VALUES ($1, $2)`, [itemId, tagId]);
}
}
/**
* 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<void> {
// 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.
*
* Both routes parsed them and refused them identically, eight lines each. The
* distinction being preserved is that `undefined` means "not submitted", which
* update reads as "leave as-is" — so an unparseable value has to be told apart
* from an absent one, which is what makes this more than a null check and worth
* having in one place.
*/
type ParsedItemFields =
| { ok: true; categoryId: number | null | undefined; tagNames: string[] | undefined }
| { ok: false; error: string };
function readOptionalItemFields(body: Record<string, unknown>): ParsedItemFields {
const categoryId = readCategoryId(body.category_id);
if (categoryId === undefined && body.category_id !== undefined) {
return { ok: false, error: 'invalid category_id' };
}
const tagNames = readTagNames(body.tags);
if (tagNames === undefined && body.tags !== undefined) {
return { ok: false, error: 'invalid tags' };
}
return { ok: true, categoryId, tagNames };
}
router.get('/items', asyncRoute(async (req: Request, res: Response) => {
// Same parser and query builder as the storefront, so admin filtering cannot
// drift from what customers see. The one addition is `status`, which is how
// the Inventory tab surfaces Reserved.
let filters;
try {
filters = parseItemFilters(req.query as Record<string, unknown>);
} catch (err) {
if (err instanceof FilterError) {
return res.status(400).json({ error: err.message });
}
throw err;
}
// Favorites belong to a customer, and the admin inventory view is not
// browsing as one. Refused rather than ignored so the mistake is visible.
if (filters.favoritesOnly) {
return res.status(400).json({ error: 'favorites is not a valid inventory filter' });
}
// S2077 flags every query below that assembles its SQL as a template literal,
// and this is the one where that is more than a formality: `where` really is
// built at run time. What makes it safe is that buildItemFilterSql composes
// only string literals written in itemFilters.ts. The only interpolations
// inside any of them are placeholder indices — `$${next}`, and `$${next + 1}`
// in the tags clause — numbers, seeded from the startIndex argument and
// incremented locally. Neither is ever derived from a filter value.
//
// So a caller chooses which of six fixed fragments are joined, and supplies
// every value in `params`, and neither of those becomes SQL. parseItemFilters
// rejects malformed input above, but that is defence in depth rather than the
// reason this holds — the clause literals would be safe without it.
const { clauses, params } = buildItemFilterSql(filters, 1, null);
const where = clauses.length ? `WHERE ${clauses.join(' AND ')}` : '';
const { rows } = await pool.query<AdminItemRow>(`${ADMIN_ITEM_SELECT} ${where} ORDER BY i.created_at DESC`, params);
res.json(rows);
}));
router.post('/items', uploadImages, asyncRoute(async (req: Request, res: Response) => {
const { name, description, price } = req.body;
const parsed = readOptionalItemFields(req.body);
if (!parsed.ok) return res.status(400).json({ error: parsed.error });
const { categoryId, tagNames } = parsed;
const files = (req.files as Express.Multer.File[]) || [];
const client = await pool.connect();
try {
await client.query('BEGIN');
const { rows } = await client.query<ItemRecord>(
`INSERT INTO items (name, description, price_cents, category_id) VALUES ($1, $2, $3, $4) RETURNING *`,
[name, description, Math.round(parseFloat(price) * 100), categoryId ?? null]
);
const item = requireRow(rows, 'the item INSERT');
await insertItemImages(client, item.id, files, 0);
if (tagNames) {
await setItemTags(client, item.id, await resolveTagIds(client, tagNames));
}
await client.query('COMMIT');
// S2077 again, and here the template is a module constant plus a literal:
// ADMIN_ITEM_SELECT interpolates nothing of its own, and the id is bound as
// $1 rather than formatted in. Same shape as the update route below, where
// the bound value is caller-supplied — which is precisely why it is a
// parameter.
const { rows: full } = await pool.query<AdminItemRow>(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [item.id]);
res.json(requireRow(full, 'the item just inserted'));
} catch (err) {
await client.query('ROLLBACK');
console.error(err);
res.status(500).json({ error: 'internal error' });
} finally {
client.release();
}
}));
router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Response) => {
const { name, description, price } = req.body;
const parsed = readOptionalItemFields(req.body);
if (!parsed.ok) return res.status(400).json({ error: parsed.error });
const { categoryId, tagNames } = parsed;
const files = (req.files as Express.Multer.File[]) || [];
const client = await pool.connect();
try {
await client.query('BEGIN');
await client.query(
`UPDATE items SET name=$1, description=$2, price_cents=$3 WHERE id=$4`,
[name, description, Math.round(parseFloat(price) * 100), req.params.id]
);
// Only touch the category when the field was actually submitted, so a
// caller that omits it doesn't silently uncategorize the item.
if (categoryId !== undefined) {
await client.query(`UPDATE items SET category_id=$1 WHERE id=$2`, [categoryId, req.params.id]);
}
if (tagNames) {
await setItemTags(client, Number(req.params.id), await resolveTagIds(client, tagNames));
}
if (files.length) {
const { rows: existing } = await client.query<MaxSortRow>(
`SELECT COALESCE(MAX(sort_order), -1) AS max_sort FROM item_images WHERE item_id = $1`,
[req.params.id]
);
// COALESCE'd MAX, so the aggregate always returns exactly one row.
const nextSort = requireRow(existing, 'the MAX(sort_order) aggregate').max_sort + 1;
// Number(), as the setItemTags call above already does: a matched route
// always has this param, but noUncheckedIndexedAccess cannot know that,
// and the helper's typed parameter surfaces what the old inline query's
// unknown[] hid.
await insertItemImages(client, Number(req.params.id), files, nextSort);
}
await client.query('COMMIT');
// S2077, the same constant-plus-$1 shape as the create route above.
// req.params.id is caller-controlled and goes through the driver as a bound
// parameter; it never reaches the query text.
const { rows: full } = await pool.query<AdminItemRow>(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [req.params.id]);
res.json(full[0]);
} catch (err) {
await client.query('ROLLBACK');
console.error(err);
res.status(500).json({ error: 'internal error' });
} finally {
client.release();
}
}));
router.delete('/items/:id', asyncRoute(async (req: Request, res: Response) => {
const itemId = Number(req.params.id);
// Collected before the delete: favorites cascade with the item, so after it
// is gone there is no record of who was watching. Restricted to unsold items
// because anyone watching a sold one has already been told it went.
const recipients = await collectFavoriteRecipients([itemId], null, true);
await pool.query(`DELETE FROM items WHERE id = $1`, [itemId]);
// Sent only once the delete has succeeded, so nobody hears about a withdrawal
// that did not happen.
await notifyFavoritersOfRemoval(recipients);
res.status(204).end();
}));
router.delete('/items/:id/images/:imageId', asyncRoute(async (req: Request, res: Response) => {
await pool.query(`DELETE FROM item_images WHERE id = $1 AND item_id = $2`, [req.params.imageId, req.params.id]);
res.status(204).end();
}));
router.post('/items/:id/mark-sold', asyncRoute(async (req: Request, res: Response) => {
const { rows } = await pool.query<ItemRecord>(
`UPDATE items SET status='sold', sold_at=now() WHERE id=$1 RETURNING *`,
[req.params.id]
);
// No buyer to exclude: an admin marking an item sold has no associated
// customer, so everyone watching it hears about it.
await notifyFavoritersOfSale([Number(req.params.id)], null);
res.json(rows[0]);
}));
// Publishing is the existing mark-available: it already sets status='available'
// and clears sold_at, reserved_until and paypal_order_id, all of which are
// no-ops on a pending item. A second endpoint running the same UPDATE would be
// duplication, so the admin UI labels that button "Publish" when the item is
// pending. This is the reverse, and it is not symmetrical — see the guard.
router.post('/items/:id/unpublish', asyncRoute(async (req: Request, res: Response) => {
const { rows } = await pool.query<ItemStatusRow>(`SELECT status FROM items WHERE id = $1`, [req.params.id]);
if (!rows.length) {
return res.status(404).json({ error: 'not found' });
}
const status = requireRow(rows, 'the item status lookup').status;
if (status === 'pending') {
return res.status(400).json({ error: 'this item is already pending' });
}
// Reserved and sold are not drafts. A reserved item is in someone's cart
// right now and hiding it would strand them mid-checkout; a sold item is a
// record of something that happened, and pulling it back would quietly
// rewrite that. Both are refused by name so the reason is on screen rather
// than left to be guessed from a generic error.
if (status === 'reserved') {
return res.status(400).json({ error: 'a customer is holding this item — it cannot be unpublished' });
}
if (status === 'sold') {
return res.status(400).json({ error: 'a sold item cannot be unpublished' });
}
const { rows: updated } = await pool.query<ItemRecord>(
`UPDATE items SET status='pending' WHERE id=$1 RETURNING *`,
[req.params.id]
);
res.json(updated[0]);
}));
router.post('/items/:id/mark-available', asyncRoute(async (req: Request, res: Response) => {
const { rows } = await pool.query<ItemRecord>(
`UPDATE items SET status='available', sold_at=NULL, reserved_until=NULL, paypal_order_id=NULL
WHERE id=$1 RETURNING *`,
[req.params.id]
);
res.json(rows[0]);
}));
export default router;