diff --git a/backend/migrations/1787500000000_add-intake-pipeline.js b/backend/migrations/1787500000000_add-intake-pipeline.js new file mode 100644 index 0000000..407a236 --- /dev/null +++ b/backend/migrations/1787500000000_add-intake-pipeline.js @@ -0,0 +1,80 @@ +exports.up = (pgm) => { + pgm.sql(` + CREATE TABLE IF NOT EXISTS upload_links ( + id SERIAL PRIMARY KEY, + label TEXT NOT NULL, + -- The token itself is never stored, only its digest. A leaked database + -- is then not also a leaked set of working upload links, and the admin + -- screen can show a token exactly once — at creation — for the same + -- reason a password reset link is not re-readable. + token_hash TEXT NOT NULL UNIQUE, + revoked_at TIMESTAMPTZ, + submission_count INTEGER NOT NULL DEFAULT 0, + -- Null means no cap. A link handed to a regular contributor is + -- open-ended; one handed out for a single box of stock is not. The + -- route defaults this to a finite number rather than null, so an + -- unbounded link is something asked for rather than something that + -- happens when nobody thought about it. + max_submissions INTEGER, + last_used_at TIMESTAMPTZ, + created_at TIMESTAMPTZ NOT NULL DEFAULT now() + ); + + CREATE TABLE IF NOT EXISTS item_drafts ( + id SERIAL PRIMARY KEY, + item_id INTEGER NOT NULL UNIQUE REFERENCES items(id) ON DELETE CASCADE, + -- SET NULL rather than CASCADE: deleting a link must not delete the + -- items that arrived through it. Provenance is lost; the goods are not. + upload_link_id INTEGER REFERENCES upload_links(id) ON DELETE SET NULL, + submitter_note TEXT, + state TEXT NOT NULL DEFAULT 'queued', + attempts INTEGER NOT NULL DEFAULT 0, + model TEXT, + ai_name TEXT, + ai_description TEXT, + ai_category_id INTEGER REFERENCES categories(id) ON DELETE SET NULL, + ai_tag_names TEXT[], + -- Kept even though a suggestion is also copied onto the item, so what + -- the model proposed stays readable after the admin has edited the + -- item's price. Without it there is no way to ask later whether the + -- model's numbers were any good. + ai_suggested_price_cents INTEGER, + -- ai | default | admin. Where the item's current price came from. + -- Recorded rather than inferred: a model that happens to suggest exactly + -- 8000, or an admin who deliberately types the model's number, both + -- collapse any comparison-based guess. + price_source TEXT NOT NULL DEFAULT 'default', + ai_error TEXT, + input_tokens INTEGER, + output_tokens INTEGER, + cost_micros INTEGER, + drafted_at TIMESTAMPTZ, + reviewed_at TIMESTAMPTZ, + created_at TIMESTAMPTZ NOT NULL DEFAULT now() + ); + + -- The review queue reads by state; everything else reads by item, which + -- the UNIQUE constraint on item_id already indexes. + CREATE INDEX IF NOT EXISTS item_drafts_state_idx ON item_drafts (state); + + -- A submitted item is priced on arrival rather than left unpriced, so the + -- column keeps NOT NULL and only gains a fallback. 80.00 applies when + -- nothing else supplies a price; the drafting worker in #223 writes a + -- model's suggestion over it when there is one. + -- + -- The number lives here rather than in configuration deliberately. + -- Changing a default price is a rare, deliberate act that deserves a + -- record; an environment variable would let it drift silently between + -- environments, and a wrong default is invisible until something has + -- already sold at it. + ALTER TABLE items ALTER COLUMN price_cents SET DEFAULT 8000; + `); +}; + +exports.down = (pgm) => { + pgm.sql(` + ALTER TABLE items ALTER COLUMN price_cents DROP DEFAULT; + DROP TABLE IF EXISTS item_drafts; + DROP TABLE IF EXISTS upload_links; + `); +}; diff --git a/backend/src/app.ts b/backend/src/app.ts index 5c3ccd9..bf9da94 100755 --- a/backend/src/app.ts +++ b/backend/src/app.ts @@ -9,6 +9,8 @@ import adminSettingsRouter from './routes/adminSettings'; import adminEmailTemplatesRouter from './routes/adminEmailTemplates'; import adminCategoriesRouter from './routes/adminCategories'; import adminTagsRouter from './routes/adminTags'; +import adminUploadLinksRouter from './routes/adminUploadLinks'; +import intakeRouter from './routes/intake'; import adminVersionRouter from './routes/adminVersion'; import filtersRouter from './routes/filters'; import customersRouter from './routes/customers'; @@ -20,6 +22,7 @@ import { attachCustomer } from './middleware/customerAuth'; import { requireAdminGate } from './middleware/adminGate'; import { asyncRoute } from './asyncRoute'; import { uploadsRouter } from './uploads'; +import { trimTrailingSlashes } from './utils'; const app = express(); // Express advertises itself in X-Powered-By by default, which hands an @@ -36,13 +39,6 @@ app.use(cookieParser()); app.use(asyncRoute(attachCustomer)); app.use('/uploads', uploadsRouter(process.env.UPLOADS_DIR || '/app/uploads')); -// Trimmed with a loop rather than a `/+$/` regex, which backtracks. -function trimTrailingSlashes(value: string): string { - let trimmed = value; - while (trimmed.endsWith('/')) trimmed = trimmed.slice(0, -1); - return trimmed; -} - app.get('/api/config', (_req, res) => { const clientId = process.env.PAYPAL_CLIENT_ID; const isPlaceholder = !clientId || clientId.length < 10 || clientId === 'REPLACE_WITH_PAYPAL_CLIENT_ID'; @@ -68,6 +64,9 @@ app.get('/api/config', (_req, res) => { app.use('/api/items', itemsRouter); app.use('/api/filters', filtersRouter); app.use('/api/cart', cartRouter); +// Public and unauthenticated by design (#222). No requireAdminGate: the token +// in the path is the whole access control, and every refusal is a 404. +app.use('/api/intake', intakeRouter); app.use('/api/checkout/cart', cartCheckoutRouter); // requireAdminGate is attached to each admin router rather than to a path // prefix. Attached to the router, an admin router added later at some other @@ -79,6 +78,7 @@ app.use('/api/admin/settings', requireAdminGate, adminSettingsRouter); app.use('/api/admin/email-templates', requireAdminGate, adminEmailTemplatesRouter); app.use('/api/admin/categories', requireAdminGate, adminCategoriesRouter); app.use('/api/admin/tags', requireAdminGate, adminTagsRouter); +app.use('/api/admin/upload-links', requireAdminGate, adminUploadLinksRouter); app.use('/api/admin/version', requireAdminGate, adminVersionRouter); app.use('/api/admin', requireAdminGate, adminRouter); app.use('/api/customers/me/addresses', shippingAddressesRouter); 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/rateLimit.ts b/backend/src/rateLimit.ts index 8517b9b..4498cc9 100644 --- a/backend/src/rateLimit.ts +++ b/backend/src/rateLimit.ts @@ -124,3 +124,63 @@ export const verificationResendLimiter = rateLimit({ error: 'we have already sent several verification emails recently. Check your spam folder, and try again later.' } }); + +/** + * Keyed on the caller alone, because an intake submission carries no email. + * + * The comment on `keyByCallerAndEmail` warns that a bare `ip:` bucket is a + * shared allowance rather than a per-caller one, and that trade is accepted + * here deliberately: the *link* is the per-caller identity, and its + * `submission_count` against `max_submissions` is the per-caller cap. This + * limiter exists for a different job — bounding what one address can throw at + * an unauthenticated endpoint that writes files to disk. + * + * ipKeyGenerator rather than `req.ip` raw, for the reason #84 records: a + * residential IPv6 customer is delegated a whole prefix and can source every + * request from a different address inside it for free, so keying on the exact + * address counts each one as a new caller and never bounds anything. + */ +export function keyByCaller(req: Request): string { + return ipKeyGenerator(req.ip ?? ''); +} + +/** + * Two limiters rather than one, because the two requests cost different things. + * + * Reading a link is a page load: it hits one indexed row and writes nothing. + * Submitting writes up to six files to the uploads volume. Counting them + * against a single allowance meant reloading the page consumed the budget for + * sending items, and at twenty apiece that allowance ran out after ten items — + * for exactly the person this feature is for, somebody working through a box + * of stock. The comment here used to say refusing them costs a consignment, + * while the number quietly did it. + * + * Both still key on the caller alone, since a submission carries no email. The + * `keyByCallerAndEmail` comment warns that a bare `ip:` bucket is a shared + * allowance rather than a per-caller one, and that trade is accepted here: the + * link is the per-caller identity and its `max_submissions` is the per-caller + * cap, while these bound what one address can throw at an unauthenticated + * endpoint. + */ +export const intakeViewLimiter = rateLimit({ + windowMs: 15 * 60 * 1000, + // Generous, because it is a page load. Someone re-reading the form, losing + // their signal, or coming back to it should never be told to wait. + limit: 120, + keyGenerator: keyByCaller, + standardHeaders: 'draft-7', + legacyHeaders: false, + message: { error: 'too many requests — please try again shortly' } +}); + +export const intakeSubmitLimiter = rateLimit({ + windowMs: 15 * 60 * 1000, + // Each of these writes files, so this is the one worth bounding. Thirty in a + // quarter of an hour is more than anyone photographing items can manage and + // far less than a script would want. + limit: 30, + keyGenerator: keyByCaller, + standardHeaders: 'draft-7', + legacyHeaders: false, + message: { error: 'too many submissions — please try again later' } +}); 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. diff --git a/backend/src/routes/adminUploadLinks.ts b/backend/src/routes/adminUploadLinks.ts new file mode 100644 index 0000000..b7a4f57 --- /dev/null +++ b/backend/src/routes/adminUploadLinks.ts @@ -0,0 +1,116 @@ +import { Router, Request, Response } from 'express'; +import { pool, requireRow } from '../db'; +import { asyncRoute } from '../asyncRoute'; +import { generateToken, hashToken } from '../uploadLinks'; +import { trimTrailingSlashes } from '../utils'; + +const router = Router(); + +/** + * Issuing and retiring the links that open the public intake endpoint (#222). + * + * A link is named because provenance matters more than convenience here. When + * one is shared further than intended the question is *which* one, and the + * answer has to come from somewhere — so every submission records the link it + * arrived through, and revoking kills that link rather than the feature. + * + * The token is returned by exactly one response in this file and is + * unrecoverable afterwards. That is why the admin screen has to present it as + * a one-time reveal rather than a field to come back to, and why losing it + * means issuing a new link rather than looking the old one up. + */ + +/** + * Shaped so a `SELECT *` can never leak the digest into a response. + * + * Spelling the columns out is the point: `SELECT *` here would put + * `token_hash` into every listing the moment somebody added a convenience. + */ +const LINK_SELECT = ` + SELECT id, label, revoked_at, submission_count, max_submissions, last_used_at, created_at + FROM upload_links +`; + +/** + * The cap a link gets when nobody chose one. + * + * Not a tuned number — large enough that an ordinary contributor never meets + * it, small enough that a link shared further than intended cannot be used + * indefinitely before anyone notices. The point is that the default is finite + * at all. + */ +const DEFAULT_MAX_SUBMISSIONS = 25; + +interface UploadLinkRow { + id: number; + label: string; + revoked_at: string | null; + submission_count: number; + max_submissions: number | null; + last_used_at: string | null; + created_at: string; +} + +router.get('/', asyncRoute(async (_req: Request, res: Response) => { + const { rows } = await pool.query(`${LINK_SELECT} ORDER BY created_at DESC`); + res.json(rows); +})); + +router.post('/', asyncRoute(async (req: Request, res: Response) => { + const label = typeof req.body?.label === 'string' ? req.body.label.trim() : ''; + if (label === '') { + return res.status(400).json({ error: 'a label is required' }); + } + + // Three cases, deliberately distinct. Absent means nobody decided, which + // gets the bounded default. An explicit null means unlimited — a decision + // someone made, visible in the request. A number is itself. Reading absent + // as unlimited is what would make every link unbounded by default. + const rawCap = req.body?.maxSubmissions; + let maxSubmissions: number | null = DEFAULT_MAX_SUBMISSIONS; + if (rawCap === null) { + maxSubmissions = null; + } else if (rawCap !== undefined && rawCap !== '') { + const parsed = Number(rawCap); + if (!Number.isInteger(parsed) || parsed < 1) { + return res.status(400).json({ error: 'maxSubmissions must be a positive whole number' }); + } + maxSubmissions = parsed; + } + + const token = generateToken(); + const { rows } = await pool.query( + `INSERT INTO upload_links (label, token_hash, max_submissions) + VALUES ($1, $2, $3) + RETURNING id, label, revoked_at, submission_count, max_submissions, last_used_at, created_at`, + [label, hashToken(token), maxSubmissions] + ); + const link = requireRow(rows, 'the upload_links INSERT'); + + // PUBLIC_URL is already required alongside SMTP and is what every other + // outbound link is built from. Absent in local development, which yields a + // relative URL the admin screen can still show and copy usefully. + const base = trimTrailingSlashes(process.env.PUBLIC_URL ?? ''); + res.status(201).json({ ...link, token, url: `${base}/submit/${token}` }); +})); + +router.post('/:id/revoke', asyncRoute(async (req: Request, res: Response) => { + // COALESCE so revoking twice keeps the original timestamp. The useful fact + // is when access ended, and a second click should neither rewrite that nor + // fail — a button that errors on a double-click teaches people to distrust + // it, which is the last thing wanted on the control that contains a leak. + const { rows } = await pool.query( + `UPDATE upload_links SET revoked_at = COALESCE(revoked_at, now()) + WHERE id = $1 + RETURNING id, label, revoked_at, submission_count, max_submissions, last_used_at, created_at`, + [req.params.id] + ); + + const link = rows[0]; + if (!link) { + return res.status(404).json({ error: 'not found' }); + } + res.json(link); +})); + +export default router; diff --git a/backend/src/routes/intake.ts b/backend/src/routes/intake.ts new file mode 100644 index 0000000..62db46c --- /dev/null +++ b/backend/src/routes/intake.ts @@ -0,0 +1,164 @@ +import { Router, Request, Response, NextFunction } from 'express'; +import { pool, requireRow } from '../db'; +import { asyncRoute } from '../asyncRoute'; +import { hashToken } from '../uploadLinks'; +import { uploadImages, verifyUploadedImages, insertItemImages } from '../imageUpload'; +import { intakeViewLimiter, intakeSubmitLimiter } from '../rateLimit'; + +const router = Router(); + +/** + * The public way in: photos of one item, from someone with no account (#222). + * + * Everything here is reachable by a stranger holding a URL, so the shape of + * every refusal matters. Unknown, revoked and exhausted links are all 404 and + * indistinguishable from outside — whether a link exists is not something a + * stranger needs to be able to learn, which is the same reasoning `uploads.ts` + * applies to files. + * + * The AI is deliberately not called here. A slow or failing model request must + * not turn into a failed upload for someone who did nothing wrong, and the + * photos may be the only copy — the item is often no longer in the sender's + * hands. The row is left at `state='queued'` for the worker in #223. + */ + +interface LinkRow { + id: number; + label: string; +} + +/** The link resolved by `requireUsableLink`, carried through to the handler. */ +interface IntakeRequest extends Request { + uploadLink?: LinkRow; +} + +/** + * The link a token opens, or null. + * + * The cap is applied in SQL rather than in a later branch, so that "usable" is + * one concept with one definition used identically by the GET and the POST. + */ +async function usableLink(token: string): Promise { + const { rows } = await pool.query( + `SELECT id, label FROM upload_links + WHERE token_hash = $1 + AND revoked_at IS NULL + AND (max_submissions IS NULL OR submission_count < max_submissions)`, + [hashToken(token)] + ); + return rows[0] ?? null; +} + +/** + * Resolves the link *before* multer runs, so a stranger holding a bad token + * cannot cause a single byte to be written to the uploads volume. + * + * `discardUnlessAccepted` would delete those files afterwards, but "written + * then deleted" is a materially worse position than "never written" on an + * endpoint the whole internet can reach: it is disk churn an unauthenticated + * caller controls, and it leans on a cleanup that a crash between the write + * and the unlink would skip. Ordering this ahead of `uploadImages` is the + * whole mitigation, and a test asserts it. + */ +const requireUsableLink = asyncRoute( + async (req: Request, res: Response, next: NextFunction) => { + const link = await usableLink(req.params.token as string); + if (!link) { + res.status(404).json({ error: 'not found' }); + return; + } + (req as IntakeRequest).uploadLink = link; + next(); + } +); + +router.get('/:token', intakeViewLimiter, asyncRoute(async (req: Request, res: Response) => { + const link = await usableLink(req.params.token as string); + if (!link) { + return res.status(404).json({ error: 'not found' }); + } + // The label only. Nothing about the catalogue, the admin, or other links. + res.json({ label: link.label }); +})); + +router.post( + '/:token', + intakeSubmitLimiter, + requireUsableLink, + uploadImages, + asyncRoute(async (req: Request, res: Response) => { + // Set by requireUsableLink above. Re-checked rather than asserted non-null, + // so a future reordering of the middleware fails as a 404 rather than as a + // crash on undefined. + const link = (req as IntakeRequest).uploadLink; + if (!link) { + return res.status(404).json({ error: 'not found' }); + } + + const files = (req.files as Express.Multer.File[]) || []; + if (files.length === 0) { + return res.status(400).json({ error: 'at least one photo is required' }); + } + + const refusal = await verifyUploadedImages(req); + if (refusal) { + return res.status(400).json({ error: refusal }); + } + + const note = typeof req.body?.note === 'string' ? req.body.note.trim() : ''; + + const client = await pool.connect(); + try { + await client.query('BEGIN'); + + // A placeholder name. `items.name` is NOT NULL and nobody has named this + // yet — the drafting worker or the admin replaces it. A timestamp rather + // than "Untitled" so several waiting submissions stay tellable apart in + // the inventory list. + const { rows } = await client.query<{ id: number }>( + `INSERT INTO items (name, description, status) + VALUES ($1, $2, 'pending') + RETURNING id`, + [`Submission ${new Date().toISOString()}`, null] + ); + const itemId = requireRow(rows, 'the intake item INSERT').id; + + await insertItemImages(client, itemId, files, 0); + + await client.query( + `INSERT INTO item_drafts (item_id, upload_link_id, submitter_note) + VALUES ($1, $2, $3)`, + [itemId, link.id, note === '' ? null : note] + ); + + // Counted inside the transaction and guarded on the same conditions as + // the lookup, so two submissions racing for the last slot of a capped + // link cannot both succeed. + const counted = await client.query( + `UPDATE upload_links + SET submission_count = submission_count + 1, last_used_at = now() + WHERE id = $1 + AND revoked_at IS NULL + AND (max_submissions IS NULL OR submission_count < max_submissions)`, + [link.id] + ); + if (counted.rowCount === 0) { + await client.query('ROLLBACK'); + return res.status(404).json({ error: 'not found' }); + } + + await client.query('COMMIT'); + // No item id in the response: the sender has no business knowing about + // the catalogue, and nothing they could do with it. + res.status(201).json({ ok: true }); + } catch (err) { + await client.query('ROLLBACK'); + console.error(err); + res.status(500).json({ error: 'internal error' }); + } finally { + client.release(); + } + }) +); + +export default router; diff --git a/backend/src/uploadLinks.ts b/backend/src/uploadLinks.ts new file mode 100644 index 0000000..6b346fc --- /dev/null +++ b/backend/src/uploadLinks.ts @@ -0,0 +1,43 @@ +import crypto from 'crypto'; + +/** + * Issuing and recognising the tokens that open the public intake endpoint. + * + * Kept apart from the routes so the rules are pure and testable directly — the + * same reasoning as `uploadTypes.ts` and `keyByCallerAndEmail`, both of which + * are exported for their tests because they are where the real decisions live. + */ + +// 32 bytes — 256 bits. base64url so the value survives being pasted into a URL, +// a chat message and a QR code without escaping, which is the whole point of a +// link somebody is handed. +const TOKEN_BYTES = 32; + +export function generateToken(): string { + return crypto.randomBytes(TOKEN_BYTES).toString('base64url'); +} + +/** + * The digest stored against a link. + * + * SHA-256 rather than bcrypt, deliberately, and the reasoning is the opposite + * of the one that governs passwords. A password hash is slow on purpose, + * because a human password carries little entropy and has to survive an + * offline dictionary attack. This is 256 bits from a CSPRNG: there is no + * dictionary to try, and guessing is not a threat that slowing the hash + * addresses. + * + * Meanwhile the digest is computed on every submission request, and the intake + * endpoint is unauthenticated. A deliberately slow hash there would be a + * denial-of-service surface rather than a protection — see #242, where cost-12 + * bcrypt in the test suite was enough to push a request past its timeout under + * load. + * + * No timing-safe comparison is needed. The lookup is an indexed equality match + * on the digest rather than a byte-by-byte compare of the secret, and an + * attacker able to mount a timing attack against a 256-bit random value would + * still need the value. + */ +export function hashToken(token: string): string { + return crypto.createHash('sha256').update(token).digest('hex'); +} diff --git a/backend/src/utils.ts b/backend/src/utils.ts index 382a78d..c1425f9 100755 --- a/backend/src/utils.ts +++ b/backend/src/utils.ts @@ -73,3 +73,21 @@ export function tagColorFor(name: string): string { export const MARKETING_CONSENT_TEXT = 'I want to receive occasional emails about new one-of-a-kind items from Redefined Designs. I can unsubscribe at any time.'; + +/** + * Strips trailing slashes so a base URL can be joined with a stored path. + * + * A loop rather than `/\/+$/`, which backtracks: sonarjs flags that pattern as + * super-linear, and the input here is an environment variable rather than + * anything hostile, but the cheap version is no harder to read. + * + * Shared because two callers now need it — `/api/config` sends + * `uploadsBaseUrl` this way, and the upload-link routes build a submission URL + * from PUBLIC_URL. Stored paths always begin with a slash, so trimming the + * base is what stops the join producing a double. + */ +export function trimTrailingSlashes(value: string): string { + let trimmed = value; + while (trimmed.endsWith('/')) trimmed = trimmed.slice(0, -1); + return trimmed; +} diff --git a/backend/tests/integration/intake.integration.test.ts b/backend/tests/integration/intake.integration.test.ts new file mode 100644 index 0000000..044b877 --- /dev/null +++ b/backend/tests/integration/intake.integration.test.ts @@ -0,0 +1,209 @@ +import request from 'supertest'; +import { promises as fs } from 'fs'; +import app from '../../src/app'; +import { pool } from '../../src/db'; +import { resetDb, closeDb } from './setup/testDb'; + +const UPLOADS_DIR = process.env.UPLOADS_DIR as string; + +// The same 1x1 PNG the upload validation suite uses, so the accepted case +// exercises the whole path rather than a buffer that merely starts right. +const PNG = Buffer.from( + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==', + 'base64' +); + +// multer.diskStorage does not create its destination. +beforeAll(async () => { + await fs.mkdir(UPLOADS_DIR, { recursive: true }); +}); + +beforeEach(async () => { + await resetDb(); +}); + +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +async function storedFiles(): Promise { + return fs.readdir(UPLOADS_DIR); +} + +async function issueLink(label = 'Sarah', maxSubmissions?: number | null): Promise { + const res = await request(app) + .post('/api/admin/upload-links') + .send({ label, ...(maxSubmissions === undefined ? {} : { maxSubmissions }) }); + expect(res.status).toBe(201); + return res.body.token as string; +} + +describe('checking a link before showing the form', () => { + it('names the link so the page can greet the sender', async () => { + const token = await issueLink('Sarah'); + const res = await request(app).get(`/api/intake/${token}`); + + expect(res.status).toBe(200); + expect(res.body.label).toBe('Sarah'); + }); + + // 404 rather than 403 throughout: whether a link exists is not something a + // stranger needs to be able to distinguish. Same reasoning as uploads.ts. + it('404s an unknown token', async () => { + const res = await request(app).get('/api/intake/not-a-real-token'); + expect(res.status).toBe(404); + }); + + it('404s a revoked link', async () => { + const token = await issueLink(); + const { rows } = await pool.query<{ id: number }>(`SELECT id FROM upload_links`); + await request(app).post(`/api/admin/upload-links/${rows[0]?.id}/revoke`); + + const res = await request(app).get(`/api/intake/${token}`); + expect(res.status).toBe(404); + }); +}); + +describe('submitting an item', () => { + it('creates a pending item with its images, note and provenance', async () => { + const token = await issueLink('Sarah'); + + const res = await request(app) + .post(`/api/intake/${token}`) + .field('note', 'Hand-thrown stoneware, chip on the base') + .attach('images', PNG, 'front.png') + .attach('images', PNG, 'back.png'); + + expect(res.status).toBe(201); + expect(res.body.ok).toBe(true); + + const { rows: items } = await pool.query<{ id: number; status: string; price_cents: number }>( + `SELECT id, status, price_cents FROM items` + ); + expect(items).toHaveLength(1); + expect(items[0]?.status).toBe('pending'); + // The migration's default, not a price anyone chose. + expect(items[0]?.price_cents).toBe(8000); + + const { rows: images } = await pool.query<{ image_path: string }>( + `SELECT image_path FROM item_images WHERE item_id = $1 ORDER BY sort_order`, + [items[0]?.id] + ); + expect(images).toHaveLength(2); + expect(images[0]?.image_path).toMatch(/^\/uploads\/[a-f0-9-]+\.png$/); + + const { rows: drafts } = await pool.query( + `SELECT submitter_note, state, price_source, upload_link_id + FROM item_drafts WHERE item_id = $1`, + [items[0]?.id] + ); + expect(drafts[0]?.submitter_note).toBe('Hand-thrown stoneware, chip on the base'); + expect(drafts[0]?.state).toBe('queued'); + expect(drafts[0]?.price_source).toBe('default'); + expect(drafts[0]?.upload_link_id).not.toBeNull(); + }); + + it('counts the submission against the link', async () => { + const token = await issueLink(); + await request(app).post(`/api/intake/${token}`).attach('images', PNG, 'a.png'); + + const { rows } = await pool.query<{ submission_count: number; last_used_at: string | null }>( + `SELECT submission_count, last_used_at FROM upload_links` + ); + expect(rows[0]?.submission_count).toBe(1); + expect(rows[0]?.last_used_at).not.toBeNull(); + }); + + it('refuses a submission with no photos', async () => { + const token = await issueLink(); + const res = await request(app).post(`/api/intake/${token}`).field('note', 'nothing attached'); + + expect(res.status).toBe(400); + const { rows } = await pool.query(`SELECT id FROM items`); + expect(rows).toHaveLength(0); + }); + + // The file is named .png and declared image/png, but the bytes are not. + // This is the check that cannot happen before the write. + it('refuses a file whose bytes disagree with its type', async () => { + const token = await issueLink(); + const res = await request(app) + .post(`/api/intake/${token}`) + .attach('images', Buffer.from('not an image'), { + filename: 'evil.png', + contentType: 'image/png' + }); + + expect(res.status).toBe(400); + const { rows } = await pool.query(`SELECT id FROM items`); + expect(rows).toHaveLength(0); + }); + + it('404s a revoked link without creating anything', async () => { + const token = await issueLink(); + const { rows: links } = await pool.query<{ id: number }>(`SELECT id FROM upload_links`); + await request(app).post(`/api/admin/upload-links/${links[0]?.id}/revoke`); + + const res = await request(app).post(`/api/intake/${token}`).attach('images', PNG, 'a.png'); + + expect(res.status).toBe(404); + const { rows } = await pool.query(`SELECT id FROM items`); + expect(rows).toHaveLength(0); + }); + + // The reason requireUsableLink is ordered ahead of uploadImages. Without that + // ordering this still returns 404 and still creates no item — the bytes just + // reach the disk first and are deleted afterwards. This asserts they never + // arrive, so a future reordering fails here rather than quietly handing an + // unauthenticated caller control of disk churn. + it('writes nothing to the uploads volume for a token that does not work', async () => { + const before = await storedFiles(); + + const res = await request(app) + .post('/api/intake/not-a-real-token') + .attach('images', PNG, 'a.png'); + + expect(res.status).toBe(404); + expect(await storedFiles()).toEqual(before); + }); + + it('stops accepting once the link hits its cap', async () => { + const token = await issueLink('One shot', 1); + + const first = await request(app).post(`/api/intake/${token}`).attach('images', PNG, 'a.png'); + expect(first.status).toBe(201); + + const second = await request(app).post(`/api/intake/${token}`).attach('images', PNG, 'b.png'); + expect(second.status).toBe(404); + + const { rows } = await pool.query(`SELECT id FROM items`); + expect(rows).toHaveLength(1); + }); + + // #226 applies here too, and this route is exactly where it matters most: + // the photo comes from a stranger's phone rather than the shop's own camera. + it('strips metadata from a submitted photo', async () => { + const token = await issueLink(); + await request(app).post(`/api/intake/${token}`).attach('images', PNG, 'a.png'); + + const { rows } = await pool.query<{ image_path: string }>(`SELECT image_path FROM item_images`); + const sharp = (await import('sharp')).default; + const stored = await sharp( + `${UPLOADS_DIR}/${rows[0]?.image_path.replace('/uploads/', '')}` + ).metadata(); + + expect(stored.exif).toBeUndefined(); + }); +}); + +describe('a submitted item does not reach the storefront', () => { + it('is absent from the public catalogue', async () => { + const token = await issueLink(); + await request(app).post(`/api/intake/${token}`).attach('images', PNG, 'a.png'); + + const res = await request(app).get('/api/items'); + expect(res.status).toBe(200); + expect(res.body).toHaveLength(0); + }); +}); diff --git a/backend/tests/integration/setup/testDb.ts b/backend/tests/integration/setup/testDb.ts index 2f6280c..e5b191a 100755 --- a/backend/tests/integration/setup/testDb.ts +++ b/backend/tests/integration/setup/testDb.ts @@ -28,9 +28,9 @@ export async function migrate(): Promise { export async function resetDb(): Promise { await testPool.query(` - TRUNCATE TABLE orders, checkout_items, checkouts, shipping_addresses, cart_items, carts, - customer_tokens, customer_sessions, favorites, customers, item_tags, item_images, items, - tags, categories + TRUNCATE TABLE item_drafts, upload_links, orders, checkout_items, checkouts, + shipping_addresses, cart_items, carts, customer_tokens, customer_sessions, favorites, + customers, item_tags, item_images, items, tags, categories RESTART IDENTITY CASCADE `); diff --git a/backend/tests/integration/uploadLinks.integration.test.ts b/backend/tests/integration/uploadLinks.integration.test.ts new file mode 100644 index 0000000..6b7c31b --- /dev/null +++ b/backend/tests/integration/uploadLinks.integration.test.ts @@ -0,0 +1,110 @@ +import request from 'supertest'; +import app from '../../src/app'; +import { pool } from '../../src/db'; +import { resetDb, closeDb } from './setup/testDb'; + +beforeEach(async () => { + await resetDb(); +}); + +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +describe('issuing an upload link', () => { + it('returns the token exactly once, at creation', async () => { + const created = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Sarah' }); + + expect(created.status).toBe(201); + expect(created.body.label).toBe('Sarah'); + expect(created.body.token).toMatch(/^[A-Za-z0-9_-]{43}$/); + expect(created.body.url).toContain(`/submit/${created.body.token}`); + + const listed = await request(app).get('/api/admin/upload-links'); + expect(listed.status).toBe(200); + expect(listed.body).toHaveLength(1); + // The whole point of storing a digest: the listing cannot hand it back. + expect(listed.body[0].token).toBeUndefined(); + expect(listed.body[0].token_hash).toBeUndefined(); + }); + + it('stores the digest rather than the token', async () => { + const created = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Estate sale box 3' }); + + const { rows } = await pool.query<{ token_hash: string }>( + `SELECT token_hash FROM upload_links` + ); + expect(rows[0]?.token_hash).not.toBe(created.body.token); + expect(rows[0]?.token_hash).toMatch(/^[a-f0-9]{64}$/); + }); + + it('refuses a link with no label', async () => { + const res = await request(app).post('/api/admin/upload-links').send({ label: ' ' }); + expect(res.status).toBe(400); + }); + + it('refuses a non-positive submission cap', async () => { + const res = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Bad cap', maxSubmissions: 0 }); + expect(res.status).toBe(400); + }); + + // Omitting the field is the common case, so it is the case that has to be + // safe. An unbounded link should be something asked for, not something that + // happens when nobody thought about it. + it('bounds a link that was created without a cap', async () => { + const res = await request(app).post('/api/admin/upload-links').send({ label: 'Sarah' }); + + expect(res.status).toBe(201); + expect(res.body.max_submissions).toBe(25); + }); + + it('allows unlimited when it is asked for explicitly', async () => { + const res = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Always on', maxSubmissions: null }); + + expect(res.status).toBe(201); + expect(res.body.max_submissions).toBeNull(); + }); +}); + +describe('revoking an upload link', () => { + it('stamps revoked_at and reports it in the listing', async () => { + const created = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Temporary' }); + + const revoked = await request(app) + .post(`/api/admin/upload-links/${created.body.id}/revoke`); + + expect(revoked.status).toBe(200); + expect(revoked.body.revoked_at).not.toBeNull(); + }); + + // The useful fact is when access ended, so a second click must not rewrite + // it — and it must not be an error either, because a button that fails on a + // double-click teaches people to distrust it. + it('is idempotent, keeping the original timestamp', async () => { + const created = await request(app) + .post('/api/admin/upload-links') + .send({ label: 'Temporary' }); + + const first = await request(app).post(`/api/admin/upload-links/${created.body.id}/revoke`); + const second = await request(app).post(`/api/admin/upload-links/${created.body.id}/revoke`); + + expect(second.status).toBe(200); + expect(second.body.revoked_at).toBe(first.body.revoked_at); + }); + + it('404s for a link that does not exist', async () => { + const res = await request(app).post('/api/admin/upload-links/9999/revoke'); + expect(res.status).toBe(404); + }); +}); diff --git a/backend/tests/unit/uploadLinks.test.ts b/backend/tests/unit/uploadLinks.test.ts new file mode 100644 index 0000000..3de57c7 --- /dev/null +++ b/backend/tests/unit/uploadLinks.test.ts @@ -0,0 +1,31 @@ +import { generateToken, hashToken } from '../../src/uploadLinks'; + +describe('generateToken', () => { + it('produces a URL-safe token with no padding', () => { + expect(generateToken()).toMatch(/^[A-Za-z0-9_-]{43}$/); + }); + + // The token is the entire access control on the intake endpoint. If two + // calls could collide, one person's link would open another's. + it('does not repeat', () => { + const seen = new Set(Array.from({ length: 1000 }, () => generateToken())); + expect(seen.size).toBe(1000); + }); +}); + +describe('hashToken', () => { + it('is a lowercase hex sha256 digest', () => { + expect(hashToken('abc')).toBe( + 'ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad' + ); + }); + + it('is stable across calls, so a stored digest keeps matching', () => { + const token = generateToken(); + expect(hashToken(token)).toBe(hashToken(token)); + }); + + it('gives different tokens different digests', () => { + expect(hashToken(generateToken())).not.toBe(hashToken(generateToken())); + }); +}); diff --git a/docs/superpowers/plans/2026-08-29-intake-upload-links.md b/docs/superpowers/plans/2026-08-29-intake-upload-links.md index 77e8b0c..601945b 100644 --- a/docs/superpowers/plans/2026-08-29-intake-upload-links.md +++ b/docs/superpowers/plans/2026-08-29-intake-upload-links.md @@ -188,7 +188,7 @@ This is a pure refactor. No behaviour changes; the existing tests are the safety **Interfaces:** - Consumes: `uploadTypes.ts` (`ALLOWED_IMAGE_TYPES`, `SIGNATURE_BYTES`, `extensionFor`, `isAllowedImageType`, `signatureMatches`) - Produces: - - `uploadImages: (req, res, next) => void` — multer middleware, field name `images` + - `uploadImages: (req, res, next) => void` — multer middleware, field name `images`. Internally runs `verifyUploadedImages` then `stripUploadedImages`, so a caller mounting this gets validation *and* EXIF stripping without asking for either - `verifyUploadedImages(req: Request): Promise` — refusal message, or null - `insertItemImages(client: PoolClient, itemId: number, files: Express.Multer.File[], firstSortOrder: number): Promise` - `MAX_IMAGES_PER_REQUEST: number`, `MAX_IMAGE_BYTES: number` @@ -203,9 +203,13 @@ Expected: PASS. Record this — it is the comparison for Step 4. - [ ] **Step 2: Move the code** -Create `backend/src/imageUpload.ts` and move into it, unchanged, from `routes/admin.ts`: `UnsupportedImageTypeError`, `UPLOADS_DIR`, `MAX_IMAGES_PER_REQUEST`, `MAX_IMAGE_BYTES`, `MAX_TEXT_FIELDS`, `MAX_TEXT_FIELD_BYTES`, `storage`, `upload`, `readHead`, `discardUploads`, `discardUnlessAccepted`, `verifyUploadedImages`, `uploadImages`, and `insertItemImages`. +Create `backend/src/imageUpload.ts` and move into it, unchanged, from `routes/admin.ts`: `UnsupportedImageTypeError`, `UPLOADS_DIR`, `MAX_IMAGES_PER_REQUEST`, `MAX_IMAGE_BYTES`, `MAX_TEXT_FIELDS`, `MAX_TEXT_FIELD_BYTES`, `storage`, `upload`, `readHead`, `discardUploads`, `discardUnlessAccepted`, `verifyUploadedImages`, **`stripUploadedImages`**, `uploadImages`, and `insertItemImages`. -Keep every existing comment verbatim. They record why the code is shaped as it is (#95, #103, #180) and are the most valuable thing being moved. +**`stripUploadedImages` and its `import { reencodeInPlace } from '../imageProcessing'` did not exist when this plan was written.** They arrived with #226 after it, and they are the step that removes EXIF — including the GPS coordinates a phone writes — from every accepted upload. Leaving them behind in `admin.ts` would give the public intake route in Task 5 an upload path that skips stripping entirely, which is the one thing #226 exists to prevent, and nothing would fail to say so. + +The promise chain inside `uploadImages` runs `verifyUploadedImages` and then `stripUploadedImages`, and moves as a whole. Verify after moving that `routes/admin.ts` no longer imports `imageProcessing` — if it still does, something was left behind. + +Keep every existing comment verbatim. They record why the code is shaped as it is (#95, #103, #180, #226) and are the most valuable thing being moved. Export `uploadImages`, `verifyUploadedImages`, `insertItemImages`, `MAX_IMAGES_PER_REQUEST`, `MAX_IMAGE_BYTES`. Everything else stays module-private. diff --git a/frontend/src/admin/Admin.tsx b/frontend/src/admin/Admin.tsx index 475548e..00a3745 100755 --- a/frontend/src/admin/Admin.tsx +++ b/frontend/src/admin/Admin.tsx @@ -33,6 +33,7 @@ import Emails from './Emails'; import Settings from './Settings'; import Categories from './Categories'; import Tags from './Tags'; +import UploadLinks from './UploadLinks'; import BuildStamp from './BuildStamp'; import CategoryTreeSelect from './CategoryTreeSelect'; import ItemCard from '../components/ItemCard'; @@ -391,6 +392,7 @@ export default function Admin() { { key: 'inventory', label: 'Inventory', children: }, { key: 'categories', label: 'Categories', children: }, { key: 'tags', label: 'Tags', children: }, + { key: 'upload-links', label: 'Upload links', children: }, { key: 'customers', label: 'Customers', children: }, { key: 'emails', label: 'Emails', children: }, { key: 'settings', label: 'Settings', children: } diff --git a/frontend/src/admin/UploadLinks.tsx b/frontend/src/admin/UploadLinks.tsx new file mode 100644 index 0000000..30ecd5d --- /dev/null +++ b/frontend/src/admin/UploadLinks.tsx @@ -0,0 +1,191 @@ +import { useEffect, useState } from 'react'; +import Table from 'antd/es/table'; +import Button from 'antd/es/button'; +import Input from 'antd/es/input'; +import Space from 'antd/es/space'; +import Alert from 'antd/es/alert'; +import Typography from 'antd/es/typography'; +import Popconfirm from 'antd/es/popconfirm'; +import Checkbox from 'antd/es/checkbox'; +import Tag from 'antd/es/tag'; + +const { Paragraph, Text } = Typography; + +interface UploadLink { + id: number; + label: string; + revoked_at: string | null; + submission_count: number; + max_submissions: number | null; + last_used_at: string | null; + created_at: string; +} + +/** What a link gets when the form is left alone. Mirrors the server's default. */ +const DEFAULT_CAP = '25'; + +/** + * Issuing and retiring the links that let someone without an account send in + * photos (#222). + * + * The token is shown exactly once, at creation, and cannot be recovered — the + * server stores only a digest. That is a deliberate property rather than an + * oversight, so this screen has to make the one-time nature obvious rather + * than leaving somebody to discover it by refreshing. + */ +export default function UploadLinks() { + const [links, setLinks] = useState([]); + const [label, setLabel] = useState(''); + const [cap, setCap] = useState(DEFAULT_CAP); + const [unlimited, setUnlimited] = useState(false); + // Held only in component state and shown once. A refresh loses it, which is + // the honest behaviour: the server genuinely cannot produce it again. + const [issued, setIssued] = useState(null); + const [error, setError] = useState(null); + const [creating, setCreating] = useState(false); + + async function load() { + const res = await fetch('/api/admin/upload-links'); + if (res.ok) setLinks(await res.json()); + } + + // Load-on-mount, the same shape Tags and Categories use. `load` only sets + // state after its fetch resolves, so nothing here is synchronous. + // eslint-disable-next-line react-hooks/set-state-in-effect + useEffect(() => { void load(); }, []); + + async function create() { + setCreating(true); + setError(null); + + const res = await fetch('/api/admin/upload-links', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + // Sent explicitly in all three cases rather than omitted. null is how + // unlimited is asked for; the server's default only has to cover callers + // that are not this screen. + body: JSON.stringify({ + label, + maxSubmissions: unlimited ? null : Number(cap) + }) + }); + + setCreating(false); + + if (!res.ok) { + const payload = await res.json().catch(() => ({})); + setError(payload.error ?? 'Could not create the link.'); + return; + } + + const created = await res.json(); + setIssued(created.url); + setLabel(''); + setCap(DEFAULT_CAP); + setUnlimited(false); + await load(); + } + + async function revoke(id: number) { + await fetch(`/api/admin/upload-links/${id}/revoke`, { method: 'POST' }); + await load(); + } + + return ( + + + Give one link per person or purpose. If a link is shared further than you meant, revoke that + one — everything already sent through it is kept. + + + + setLabel(e.target.value)} + style={{ width: 260 }} + /> + setCap(e.target.value)} + style={{ width: 140 }} + /> + setUnlimited(e.target.checked)}> + No limit + + + + + {error && } + + {issued && ( + + + {issued} + + + It is not stored and cannot be shown again. If you lose it, revoke this link and + make another. + + + } + closable + onClose={() => setIssued(null)} + /> + )} + + + rowKey="id" + dataSource={links} + pagination={false} + columns={[ + { title: 'Label', dataIndex: 'label' }, + { + title: 'Used', + render: (_, row) => + row.max_submissions === null + ? row.submission_count + : `${row.submission_count} of ${row.max_submissions}` + }, + { + title: 'Status', + render: (_, row) => + row.revoked_at ? Revoked : Active + }, + { + title: '', + render: (_, row) => + row.revoked_at ? null : ( + revoke(row.id)} + > + + + ) + } + ]} + /> + + ); +} diff --git a/frontend/src/intake/Submit.tsx b/frontend/src/intake/Submit.tsx new file mode 100644 index 0000000..2151898 --- /dev/null +++ b/frontend/src/intake/Submit.tsx @@ -0,0 +1,183 @@ +import { useEffect, useState } from 'react'; +import { useParams } from 'react-router-dom'; +import Typography from 'antd/es/typography'; +import Card from 'antd/es/card'; +import Upload from 'antd/es/upload'; +import Button from 'antd/es/button'; +import Input from 'antd/es/input'; +import Alert from 'antd/es/alert'; +import Spin from 'antd/es/spin'; +import Space from 'antd/es/space'; +import { UploadOutlined } from '@ant-design/icons'; +import type { UploadFile } from 'antd/es/upload/interface'; +import { fetchIntakeLink, submitItem } from './intakeApi'; +import type { LinkState } from './intakeApi'; + +const { Title, Paragraph } = Typography; +const { TextArea } = Input; + +/** + * Where someone with no account sends in photos of one item (#222). + * + * The three types the server will accept, and the same per-request cap. Listed + * here so the file picker offers exactly what will be taken and the count is + * bounded before anything is uploaded — but the server checks both again, + * because everything on this page is under the sender's control. + */ +const ACCEPT = 'image/jpeg,image/png,image/webp'; +const MAX_IMAGES = 6; + +export default function Submit() { + const { token = '' } = useParams(); + const [state, setState] = useState({ kind: 'unusable' }); + const [checking, setChecking] = useState(true); + const [files, setFiles] = useState([]); + const [note, setNote] = useState(''); + const [sending, setSending] = useState(false); + const [sent, setSent] = useState(false); + const [error, setError] = useState(null); + + useEffect(() => { + let cancelled = false; + void fetchIntakeLink(token).then((result) => { + // The token can change if the URL does, and a late response from the + // previous one would otherwise overwrite the current answer. + if (cancelled) return; + setState(result); + setChecking(false); + }); + return () => { + cancelled = true; + }; + }, [token]); + + async function send() { + setSending(true); + setError(null); + + const result = await submitItem( + token, + // originFileObj is what antd hands back for a file it did not upload + // itself; beforeUpload returning false is what keeps them here. flatMap + // rather than map-then-filter because a type predicate cannot narrow to + // File here — antd's RcFile extends it, so the predicate would widen. + files.flatMap((f) => (f.originFileObj ? [f.originFileObj] : [])), + note + ); + + setSending(false); + if (result.ok) { + setSent(true); + return; + } + setError(result.error); + } + + if (checking) { + return ( +
+ +
+ ); + } + + // Kept apart from the state below on purpose. The two need opposite + // reactions — wait a moment, versus go and ask for a different link — so + // telling a throttled sender their link was dead would send them to fetch a + // replacement that could not have helped. + if (state.kind === 'throttled') { + return ( +
+ + Too many requests just now + + Your link is fine — this page has just been asked for too many times from your + connection. Wait a minute and reload. + + +
+ ); + } + + // One state for unknown, revoked and used-up alike, matching the server's + // single 404. Saying which it was would tell a stranger whether a link they + // guessed at exists. + if (state.kind === 'unusable') { + return ( +
+ + This link is not active + + It may have been turned off, or already used as many times as it was meant for. Ask + whoever sent it to you for a new one. + + +
+ ); + } + + if (sent) { + return ( +
+ + Thank you — it arrived + + Somebody will look at your photos and write it up. Nothing is listed for sale until they + have. + + + +
+ ); + } + + return ( +
+ + Send in an item + + Photos of one item, and anything you know about it. Send each item separately. + + + + false} + onChange={({ fileList }) => setFiles(fileList)} + > + + + +