From 7d8ac15ef88559b51004193e392196184d85ac2b Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Mon, 31 Aug 2026 14:48:30 -0500 Subject: [PATCH 1/9] docs(intake): refresh the extraction task for the changes #226 made (#222) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This plan was written on 2026-08-29, before #226 landed. Its Task 2 lists what to move out of routes/admin.ts into the shared image pipeline, and that list is now missing `stripUploadedImages` and the `reencodeInPlace` import it depends on, because neither existed when the list was written. Executing it as written would have left the re-encode behind in admin.ts, and the public intake route added in Task 5 would then have had an upload path that skips EXIF stripping entirely. That is precisely what #226 exists to prevent — a stranger photographing an item at home publishing the coordinates it was taken at — and nothing in the suite would have failed to say so, because the intake tests are written against a route that does not exist yet. The task now names the function, says why it matters, and adds a check with a definite answer: after the move, routes/admin.ts must no longer import imageProcessing. If it still does, something was left behind. Ref #222, #226 --- .../plans/2026-08-29-intake-upload-links.md | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) 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. -- 2.54.0 From 6df32af784d285e6b1569638d038e6e6bd40acc7 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Mon, 31 Aug 2026 14:51:25 -0500 Subject: [PATCH 2/9] feat(intake): add upload_links and item_drafts, and default an item's price (#222) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The schema for the intake pipeline. A submission becomes an `items` row at status 'pending' — already invisible to every public and storefront query since #90 — with an `item_drafts` row beside it holding the submitter's note, which link it arrived through, and the fields the drafting worker will fill in later. `upload_links` stores a digest rather than a token, so a leaked database is not also a leaked set of working links, and the admin screen can show a token exactly once. `max_submissions` is nullable for "no cap", but the route will default it to a finite number: an unbounded link should be something asked for, not something that happens when nobody thought about it. `item_drafts.upload_link_id` is ON DELETE SET NULL rather than CASCADE. Deleting a link must not delete the items that arrived through it — provenance is lost, the goods are not. `items.price_cents` keeps NOT NULL and gains a default of 80.00, so an arriving item is always priced. That is the decision taken in the design review over making the column nullable: it costs the schema-level guarantee that nothing can publish at a price nobody chose, and buys not having to teach the cart, the checkout and thirteen other files about an item without a price. The protection moves into the review queue, and `price_source` exists so that queue can say whether a number came from a model, the default, or a person. The number lives in the migration rather than in configuration. 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. Verified up, down and up again rather than only forwards — an irreversible migration is one that cannot be tested. Then verified by inspection rather than assumption: the default reads 8000, both tables and the state index exist, and an item inserted with no price comes back at 8000. Backend: 263 integration, 302 unit, all passing against the new schema. Ref #222 --- .../1787500000000_add-intake-pipeline.js | 80 +++++++++++++++++++ backend/tests/integration/setup/testDb.ts | 6 +- 2 files changed, 83 insertions(+), 3 deletions(-) create mode 100644 backend/migrations/1787500000000_add-intake-pipeline.js 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/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 `); -- 2.54.0 From 1a5a8b837b145d00bb5bf981aac2e5fb53f74b94 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Mon, 31 Aug 2026 14:56:46 -0500 Subject: [PATCH 3/9] refactor(uploads): extract the validated image pipeline for a second caller (#222) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pure move, no behaviour change. #222's public intake endpoint needs the same path from a multipart request to files on the uploads volume that the admin routes use, and the alternative to sharing it is a near-copy that has to reproduce every safety property exactly: 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. A copy that drifted on any of those is the gap #95, #103, #180 and #226 exist to close. `stripUploadedImages` moved with the rest, which the plan originally did not say — it was written before #226 added it. Leaving it behind would have given the intake route an upload path that skips EXIF stripping, and no test would have failed to say so, because the intake tests are written against a route that does not exist yet. The refreshed plan added a check with a definite answer, and it now holds: routes/admin.ts no longer imports imageProcessing at all. Every comment came across verbatim. They record why the code is shaped as it is and are the most valuable part of what moved. Lint caught something the compiler did not: MAX_IMAGES_PER_REQUEST was left imported into admin.ts, where its only use — `upload.array('images', MAX_IMAGES_PER_REQUEST)` — had moved away with the middleware. It stays exported from imageUpload for the intake route's caps, but admin.ts does not need it. Verified as a refactor rather than as a change: the four suites that exercise this path hardest were run before the move and after it, 44 tests both times, same suites, same count. Full backend: 263 integration, 302 unit, lint back to its 6 pre-existing warnings, build clean. Ref #222 --- backend/src/imageUpload.ts | 286 ++++++++++++++++++++++++++++++++++++ backend/src/routes/admin.ts | 264 +-------------------------------- 2 files changed, 293 insertions(+), 257 deletions(-) create mode 100644 backend/src/imageUpload.ts diff --git a/backend/src/imageUpload.ts b/backend/src/imageUpload.ts new file mode 100644 index 0000000..d75dd0d --- /dev/null +++ b/backend/src/imageUpload.ts @@ -0,0 +1,286 @@ +/** + * The one validated path from a multipart request to files on the uploads + * volume. + * + * Extracted from routes/admin.ts when a second caller appeared (#222's public + * intake endpoint). It is deliberately one module rather than two similar + * ones: every property that makes an upload safe here — the type allowlist, + * the magic-byte check after the write, names from a CSPRNG rather than from + * `originalname`, the re-encode that strips EXIF, and the cleanup of whatever + * a refused request left behind — is a property a second implementation would + * have to reproduce exactly. A near-copy that drifted would be precisely the + * gap #95, #103, #180 and #226 exist to close. + * + * Nothing below changed in the move. The comments came with it, because they + * record why the code is shaped as it is and are the most valuable part of it. + */ + +import { Request, Response, NextFunction } from 'express'; +import multer from 'multer'; +import { promises as fs } from 'fs'; +import { randomUUID } from 'crypto'; +import { PoolClient } from 'pg'; +import { + ALLOWED_IMAGE_TYPES, + SIGNATURE_BYTES, + extensionFor, + isAllowedImageType, + signatureMatches +} from './uploadTypes'; +import { reencodeInPlace } from './imageProcessing'; + +const UPLOADS_DIR = process.env.UPLOADS_DIR || '/app/uploads'; + +// Multer writes to disk with no size cap unless one is given, so a single +// request could fill the uploads volume. Bound every dimension of the +// multipart body: image count, bytes per image, and the small text fields +// (name/description/price) that accompany them. +const MAX_IMAGES_PER_REQUEST = 6; +// 8 MB, not 8 MiB — this is the ceiling S5693 treats as safe, and 8 * 1024 * +// 1024 sits just over it. Plenty for a product photo either way. +const MAX_IMAGE_BYTES = 8_000_000; +const MAX_TEXT_FIELDS = 8; +const MAX_TEXT_FIELD_BYTES = 64 * 1024; + +// Refused before a byte is written. This catches the honest mistake — picking a +// PDF by accident — and nothing more, because file.mimetype is whatever the +// caller wrote in the multipart headers. The bytes are checked after the write; +// see verifyUploadedImages. +class UnsupportedImageTypeError extends Error {} + +const storage = multer.diskStorage({ + destination: UPLOADS_DIR, + // Stored names come from a CSPRNG rather than a timestamp plus Math.random, + // which is predictable enough that a caller could guess (or collide with) + // another upload's path. + // + // The extension comes from the validated content type rather than from + // path.extname(file.originalname), so the name on disk cannot disagree with + // what the file claims to be — a caller cannot get `.html` onto the uploads + // volume by naming their file that way. + filename: (_req, file, cb) => { + const ext = extensionFor(file.mimetype); + if (!ext) { + // Unreachable while fileFilter runs first, and here so that it stays + // unreachable rather than silently writing a file with no extension. + cb(new UnsupportedImageTypeError(`unsupported image type ${file.mimetype}`), ''); + return; + } + cb(null, `${randomUUID()}${ext}`); + } +}); + +// Reviewed for #180. Bounding one request is only half the problem — see +// discardUnlessAccepted below for the other half, which is bounding what the +// volume accumulates across requests that were refused. +const upload = multer({ + storage, + limits: { + fileSize: MAX_IMAGE_BYTES, + files: MAX_IMAGES_PER_REQUEST, + fields: MAX_TEXT_FIELDS, + fieldSize: MAX_TEXT_FIELD_BYTES + }, + fileFilter: (_req, file, cb) => { + if (!isAllowedImageType(file.mimetype)) { + cb(new UnsupportedImageTypeError( + `${file.mimetype} is not an accepted image type — allowed: ${ALLOWED_IMAGE_TYPES.join(', ')}` + )); + return; + } + cb(null, true); + } +}); + +// Reads only the leading bytes — enough to identify a format, not enough to +// care how large the file is. The handle is closed before anything is unlinked, +// because an open handle makes the unlink fail on Windows. +// +// Reviewed for #180. The path is not caller-controlled despite arriving from a +// request: multer composes it from `destination`, which is a server constant, +// and `filename`, which the storage above sets to `randomUUID()` plus an +// extension looked up from the validated content type. The caller's +// `originalname` is never consulted, so no part of the path traverses anywhere. +async function readHead(filePath: string): Promise { + const handle = await fs.open(filePath, 'r'); + try { + const buffer = Buffer.alloc(SIGNATURE_BYTES); + const { bytesRead } = await handle.read(buffer, 0, SIGNATURE_BYTES, 0); + return buffer.subarray(0, bytesRead); + } finally { + await handle.close(); + } +} + +// Best effort: a file that cannot be removed should not turn a 400 into a 500, +// but it must not be left behind quietly either. +async function discardUploads(files: Express.Multer.File[]): Promise { + await Promise.all( + files.map((file) => + fs.unlink(file.path).catch((err: unknown) => { + console.error(`[upload] could not remove rejected file ${file.path}:`, err); + }) + ) + ); +} + +/** + * Removes a request's uploaded files unless the request actually succeeded. + * + * multer writes to disk before any route logic runs, and its own cleanup only + * covers errors it raised itself. Everything after that — a failed signature + * check, a malformed `category_id`, a database error, a dropped connection — + * previously left the bytes on the volume with nothing referencing them: no row + * to find them by, and no bound on how many could accumulate. Bounding the size + * of one upload does not help if every refused upload is kept forever (#180). + * + * Registered as soon as multer succeeds rather than at each `return`, so a + * route added later inherits it instead of having to remember it. That is the + * whole reason it is a hook and not a call: the failure it prevents is someone + * adding a fourth early return. + * + * `close` rather than `finish`, so an aborted connection is covered too, and + * `writableEnded` distinguishes a response that completed from one that never + * did — the latter is not a success however its status code reads. + */ +function discardUnlessAccepted(req: Request, res: Response): void { + res.on('close', () => { + if (res.writableEnded && res.statusCode < 400) return; + void discardUploads((req.files as Express.Multer.File[]) || []); + }); +} + +/** + * Confirms each stored file actually is what it was declared to be. + * + * This cannot happen in multer's fileFilter, which runs before the stream has + * been read — there are no bytes to look at yet. So the check runs after the + * write. + * + * Checking only: removing the files is discardUnlessAccepted's job, and doing + * it here as well would unlink twice and log an ENOENT for every refused + * upload. That also covers the case this function used to miss — `readHead` + * itself throwing, which returned no message and so cleaned up nothing. + * + * Returns the message to refuse with, or null when everything checks out. + */ +async function verifyUploadedImages(req: Request): Promise { + const files = (req.files as Express.Multer.File[]) || []; + + for (const file of files) { + const head = await readHead(file.path); + if (!signatureMatches(file.mimetype, head)) { + return `${file.originalname} does not contain ${file.mimetype} data`; + } + } + + return null; +} + +/** + * Rebuilds every accepted file so it carries no metadata (#226). + * + * After verification, deliberately: re-encoding a file whose bytes do not match + * its declared type would be doing work on something already refused, and + * sharp's own error would replace the clearer message that check produces. + * + * A failure here refuses the upload rather than storing the original. Storing + * it would mean the one case where a photo keeps the coordinates it was taken + * at is the case nobody was told about. + * + * Returns the message to refuse with, or null when every file was rebuilt. + */ +async function stripUploadedImages(req: Request): Promise { + const files = (req.files as Express.Multer.File[]) || []; + + for (const file of files) { + try { + await reencodeInPlace(file.path, file.mimetype); + } catch (err) { + console.error(`[upload] could not re-encode ${file.path}:`, err); + return `${file.originalname} could not be processed`; + } + } + + return null; +} + +// No error-handling middleware is mounted on the app, so translate multer's +// limit errors here instead of letting them surface as a generic 500. +const uploadImages = (req: Request, res: Response, next: NextFunction) => { + upload.array('images', MAX_IMAGES_PER_REQUEST)(req, res, (err: unknown) => { + if (err instanceof UnsupportedImageTypeError) { + return res.status(400).json({ error: err.message }); + } + if (err instanceof multer.MulterError) { + const status = err.code === 'LIMIT_FILE_SIZE' ? 413 : 400; + return res.status(status).json({ error: err.message }); + } + if (err) { + return next(err); + } + + // Every file is on disk by this point and multer will not clean up after + // itself again, so the bytes become this request's responsibility before + // anything else is allowed to fail. + discardUnlessAccepted(req, res); + + verifyUploadedImages(req) + .then((problem) => { + if (problem) { + res.status(400).json({ error: problem }); + return null; + } + return stripUploadedImages(req); + }) + .then((problem) => { + // The first stage returns null both when it answered and when it found + // nothing wrong, so the response itself is what distinguishes them. + if (res.headersSent) return; + if (problem) { + res.status(400).json({ error: problem }); + return; + } + next(); + }) + .catch(next); + }); +}; + +/** + * Records uploaded files as an item's images. + * + * Create and update wrote this loop separately, differing only in where the id + * came from and where the sort order started — zero for a new item, one past + * the current maximum for an existing one. Both are parameters now. + * + * It also means the `/uploads/` prefix is written once. That matters more than + * it looks: #103 made the stored value the path `uploadUrl` joins an origin + * onto, so it is a contract rather than a string, and two places to change it + * is one place to forget. + */ +async function insertItemImages( + client: PoolClient, + itemId: number, + files: Express.Multer.File[], + firstSortOrder: number +): Promise { + // Iterated by entry rather than by index, so there is no possibly-undefined + // element to guard — the create path used to fall back to an empty filename, + // which would have stored a path pointing at the uploads directory itself. + for (const [offset, file] of files.entries()) { + await client.query( + `INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, $3)`, + [itemId, `/uploads/${file.filename}`, firstSortOrder + offset] + ); + } +} + +export { + uploadImages, + verifyUploadedImages, + stripUploadedImages, + insertItemImages, + MAX_IMAGES_PER_REQUEST, + MAX_IMAGE_BYTES +}; diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index 8f333a8..0ad0f70 100755 --- a/backend/src/routes/admin.ts +++ b/backend/src/routes/admin.ts @@ -1,7 +1,4 @@ -import { Router, Request, Response, NextFunction } from 'express'; -import multer from 'multer'; -import { promises as fs } from 'fs'; -import { randomUUID } from 'crypto'; +import { Router, Request, Response } from 'express'; import { PoolClient } from 'pg'; import { pool, requireRow } from '../db'; import { ADMIN_ITEM_SELECT, AdminItemRow, ItemRecord } from '../itemSelect'; @@ -9,15 +6,13 @@ import { ItemStatus } from '../types'; import { asyncRoute } from '../asyncRoute'; import { parseItemFilters, buildItemFilterSql, FilterError } from '../itemFilters'; import { tagColorFor } from '../utils'; -import { - ALLOWED_IMAGE_TYPES, - SIGNATURE_BYTES, - extensionFor, - isAllowedImageType, - signatureMatches -} from '../uploadTypes'; import { notifyFavoritersOfSale, notifyFavoritersOfRemoval, collectFavoriteRecipients } from '../favoriteAlerts'; -import { reencodeInPlace } from '../imageProcessing'; +// The upload pipeline moved to src/imageUpload.ts when #222's public intake +// endpoint became a second caller. Mounting uploadImages gets the type +// allowlist, the magic-byte check, and the EXIF-stripping re-encode together — +// which is the point of it being one module rather than something each route +// assembles for itself. +import { uploadImages, insertItemImages } from '../imageUpload'; const router = Router(); @@ -31,223 +26,6 @@ interface ItemStatusRow { status: ItemStatus; } -const UPLOADS_DIR = process.env.UPLOADS_DIR || '/app/uploads'; - -// Multer writes to disk with no size cap unless one is given, so a single -// request could fill the uploads volume. Bound every dimension of the -// multipart body: image count, bytes per image, and the small text fields -// (name/description/price) that accompany them. -const MAX_IMAGES_PER_REQUEST = 6; -// 8 MB, not 8 MiB — this is the ceiling S5693 treats as safe, and 8 * 1024 * -// 1024 sits just over it. Plenty for a product photo either way. -const MAX_IMAGE_BYTES = 8_000_000; -const MAX_TEXT_FIELDS = 8; -const MAX_TEXT_FIELD_BYTES = 64 * 1024; - -// Refused before a byte is written. This catches the honest mistake — picking a -// PDF by accident — and nothing more, because file.mimetype is whatever the -// caller wrote in the multipart headers. The bytes are checked after the write; -// see verifyUploadedImages. -class UnsupportedImageTypeError extends Error {} - -const storage = multer.diskStorage({ - destination: UPLOADS_DIR, - // Stored names come from a CSPRNG rather than a timestamp plus Math.random, - // which is predictable enough that a caller could guess (or collide with) - // another upload's path. - // - // The extension comes from the validated content type rather than from - // path.extname(file.originalname), so the name on disk cannot disagree with - // what the file claims to be — a caller cannot get `.html` onto the uploads - // volume by naming their file that way. - filename: (_req, file, cb) => { - const ext = extensionFor(file.mimetype); - if (!ext) { - // Unreachable while fileFilter runs first, and here so that it stays - // unreachable rather than silently writing a file with no extension. - cb(new UnsupportedImageTypeError(`unsupported image type ${file.mimetype}`), ''); - return; - } - cb(null, `${randomUUID()}${ext}`); - } -}); - -// Reviewed for #180. Bounding one request is only half the problem — see -// discardUnlessAccepted below for the other half, which is bounding what the -// volume accumulates across requests that were refused. -const upload = multer({ - storage, - limits: { - fileSize: MAX_IMAGE_BYTES, - files: MAX_IMAGES_PER_REQUEST, - fields: MAX_TEXT_FIELDS, - fieldSize: MAX_TEXT_FIELD_BYTES - }, - fileFilter: (_req, file, cb) => { - if (!isAllowedImageType(file.mimetype)) { - cb(new UnsupportedImageTypeError( - `${file.mimetype} is not an accepted image type — allowed: ${ALLOWED_IMAGE_TYPES.join(', ')}` - )); - return; - } - cb(null, true); - } -}); - -// Reads only the leading bytes — enough to identify a format, not enough to -// care how large the file is. The handle is closed before anything is unlinked, -// because an open handle makes the unlink fail on Windows. -// -// Reviewed for #180. The path is not caller-controlled despite arriving from a -// request: multer composes it from `destination`, which is a server constant, -// and `filename`, which the storage above sets to `randomUUID()` plus an -// extension looked up from the validated content type. The caller's -// `originalname` is never consulted, so no part of the path traverses anywhere. -async function readHead(filePath: string): Promise { - const handle = await fs.open(filePath, 'r'); - try { - const buffer = Buffer.alloc(SIGNATURE_BYTES); - const { bytesRead } = await handle.read(buffer, 0, SIGNATURE_BYTES, 0); - return buffer.subarray(0, bytesRead); - } finally { - await handle.close(); - } -} - -// Best effort: a file that cannot be removed should not turn a 400 into a 500, -// but it must not be left behind quietly either. -async function discardUploads(files: Express.Multer.File[]): Promise { - await Promise.all( - files.map((file) => - fs.unlink(file.path).catch((err: unknown) => { - console.error(`[upload] could not remove rejected file ${file.path}:`, err); - }) - ) - ); -} - -/** - * Removes a request's uploaded files unless the request actually succeeded. - * - * multer writes to disk before any route logic runs, and its own cleanup only - * covers errors it raised itself. Everything after that — a failed signature - * check, a malformed `category_id`, a database error, a dropped connection — - * previously left the bytes on the volume with nothing referencing them: no row - * to find them by, and no bound on how many could accumulate. Bounding the size - * of one upload does not help if every refused upload is kept forever (#180). - * - * Registered as soon as multer succeeds rather than at each `return`, so a - * route added later inherits it instead of having to remember it. That is the - * whole reason it is a hook and not a call: the failure it prevents is someone - * adding a fourth early return. - * - * `close` rather than `finish`, so an aborted connection is covered too, and - * `writableEnded` distinguishes a response that completed from one that never - * did — the latter is not a success however its status code reads. - */ -function discardUnlessAccepted(req: Request, res: Response): void { - res.on('close', () => { - if (res.writableEnded && res.statusCode < 400) return; - void discardUploads((req.files as Express.Multer.File[]) || []); - }); -} - -/** - * Confirms each stored file actually is what it was declared to be. - * - * This cannot happen in multer's fileFilter, which runs before the stream has - * been read — there are no bytes to look at yet. So the check runs after the - * write. - * - * Checking only: removing the files is discardUnlessAccepted's job, and doing - * it here as well would unlink twice and log an ENOENT for every refused - * upload. That also covers the case this function used to miss — `readHead` - * itself throwing, which returned no message and so cleaned up nothing. - * - * Returns the message to refuse with, or null when everything checks out. - */ -async function verifyUploadedImages(req: Request): Promise { - const files = (req.files as Express.Multer.File[]) || []; - - for (const file of files) { - const head = await readHead(file.path); - if (!signatureMatches(file.mimetype, head)) { - return `${file.originalname} does not contain ${file.mimetype} data`; - } - } - - return null; -} - -/** - * Rebuilds every accepted file so it carries no metadata (#226). - * - * After verification, deliberately: re-encoding a file whose bytes do not match - * its declared type would be doing work on something already refused, and - * sharp's own error would replace the clearer message that check produces. - * - * A failure here refuses the upload rather than storing the original. Storing - * it would mean the one case where a photo keeps the coordinates it was taken - * at is the case nobody was told about. - * - * Returns the message to refuse with, or null when every file was rebuilt. - */ -async function stripUploadedImages(req: Request): Promise { - const files = (req.files as Express.Multer.File[]) || []; - - for (const file of files) { - try { - await reencodeInPlace(file.path, file.mimetype); - } catch (err) { - console.error(`[upload] could not re-encode ${file.path}:`, err); - return `${file.originalname} could not be processed`; - } - } - - return null; -} - -// No error-handling middleware is mounted on the app, so translate multer's -// limit errors here instead of letting them surface as a generic 500. -const uploadImages = (req: Request, res: Response, next: NextFunction) => { - upload.array('images', MAX_IMAGES_PER_REQUEST)(req, res, (err: unknown) => { - if (err instanceof UnsupportedImageTypeError) { - return res.status(400).json({ error: err.message }); - } - if (err instanceof multer.MulterError) { - const status = err.code === 'LIMIT_FILE_SIZE' ? 413 : 400; - return res.status(status).json({ error: err.message }); - } - if (err) { - return next(err); - } - - // Every file is on disk by this point and multer will not clean up after - // itself again, so the bytes become this request's responsibility before - // anything else is allowed to fail. - discardUnlessAccepted(req, res); - - verifyUploadedImages(req) - .then((problem) => { - if (problem) { - res.status(400).json({ error: problem }); - return null; - } - return stripUploadedImages(req); - }) - .then((problem) => { - // The first stage returns null both when it answered and when it found - // nothing wrong, so the response itself is what distinguishes them. - if (res.headersSent) return; - if (problem) { - res.status(400).json({ error: problem }); - return; - } - next(); - }) - .catch(next); - }); -}; // The multipart body carries category_id and tags as text fields. An absent // field means "leave as-is" on update, which is why these return undefined @@ -305,34 +83,6 @@ async function setItemTags(client: PoolClient, itemId: number, tagIds: number[]) } } -/** - * Records uploaded files as an item's images. - * - * Create and update wrote this loop separately, differing only in where the id - * came from and where the sort order started — zero for a new item, one past - * the current maximum for an existing one. Both are parameters now. - * - * It also means the `/uploads/` prefix is written once. That matters more than - * it looks: #103 made the stored value the path `uploadUrl` joins an origin - * onto, so it is a contract rather than a string, and two places to change it - * is one place to forget. - */ -async function insertItemImages( - client: PoolClient, - itemId: number, - files: Express.Multer.File[], - firstSortOrder: number -): Promise { - // Iterated by entry rather than by index, so there is no possibly-undefined - // element to guard — the create path used to fall back to an empty filename, - // which would have stored a path pointing at the uploads directory itself. - for (const [offset, file] of files.entries()) { - await client.query( - `INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, $3)`, - [itemId, `/uploads/${file.filename}`, firstSortOrder + offset] - ); - } -} /** * The two optional fields the item form submits as multipart text. -- 2.54.0 From 2b2cbe119e53dd5104c8a915b7061959e70a6b5c Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Mon, 31 Aug 2026 15:00:36 -0500 Subject: [PATCH 4/9] feat(intake): generate and hash upload link tokens (#222) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The token is the entire access control on an endpoint the whole internet can reach, so both halves are pure and tested directly rather than through a request — the same reasoning that has uploadTypes.ts and keyByCallerAndEmail exported for their tests. 32 bytes of CSPRNG output, base64url so the value survives being pasted into a URL, a chat message or a QR code without escaping. That matters for something a person is handed rather than something a machine reads. The collision test runs a thousand generations rather than asserting the obvious, because a repeat would mean one person's link opening another's. SHA-256 rather than bcrypt, and the reasoning inverts the one that governs passwords. A password hash is slow on purpose because a human password carries little entropy and must survive an offline dictionary attack. This is 256 bits from a CSPRNG: there is no dictionary, so slowing the hash buys nothing. Meanwhile the digest is computed on every submission to an unauthenticated endpoint, where a deliberately slow hash would be a denial-of-service surface — #242 is the local proof that cost-12 hashing on a request path is enough to push it past a timeout under load. No timing-safe comparison, deliberately: the lookup is an indexed equality match on the digest rather than a byte-by-byte compare of the secret, and an attacker who could mount a timing attack against a 256-bit random value would still need the value. Backend: 307 unit tests, lint unchanged at 6 pre-existing warnings, build clean. Ref #222 --- backend/src/uploadLinks.ts | 43 ++++++++++++++++++++++++++ backend/tests/unit/uploadLinks.test.ts | 31 +++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 backend/src/uploadLinks.ts create mode 100644 backend/tests/unit/uploadLinks.test.ts 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/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())); + }); +}); -- 2.54.0 From 3392f6f10dc7e507c9975ee1adc34dcc5b4012b9 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Mon, 31 Aug 2026 15:05:47 -0500 Subject: [PATCH 5/9] feat(intake): issue and revoke named upload links (#222) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three routes behind the admin gate: list, create, revoke. A link is named because provenance matters more than convenience — when one is shared further than intended the question is which one, and every submission will record the link it arrived through, so revoking kills that link rather than the feature. The token is returned by exactly one response and is unrecoverable afterwards, which is why the admin screen has to present it as a one-time reveal. The listing selects its columns explicitly rather than `SELECT *`, so `token_hash` cannot reach a response the moment somebody adds a convenience — and a test asserts the listing carries neither the token nor the digest. An absent `maxSubmissions` gets a bounded default of 25 rather than null. Absent means nobody decided; an explicit null means unlimited, which is a decision visible in the request. Reading absent as unlimited is what would quietly make every link unbounded, and the common case is the one that has to be safe. Revoking is idempotent through COALESCE, and a test asserts the second call returns the *same* timestamp rather than merely succeeding. The useful fact is when access ended, and 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. Mounted above the `/api/admin` catch-all, which would otherwise swallow the path, and behind requireAdminGate on the router itself per the reasoning in middleware/adminGate.ts. Lint caught me reintroducing something this codebase had already solved: I wrote `.replace(/\/+$/, '')` to trim PUBLIC_URL, and app.ts carried a hand-written loop with a comment explaining that exact regex backtracks. Rather than duplicate the loop, trimTrailingSlashes moved to utils.ts and both callers now share it. Backend: 272 integration (9 new), 308 unit, lint back to its 6 pre-existing warnings, build clean. Ref #222 --- backend/src/app.ts | 10 +- backend/src/routes/adminUploadLinks.ts | 116 ++++++++++++++++++ backend/src/utils.ts | 18 +++ .../uploadLinks.integration.test.ts | 110 +++++++++++++++++ 4 files changed, 247 insertions(+), 7 deletions(-) create mode 100644 backend/src/routes/adminUploadLinks.ts create mode 100644 backend/tests/integration/uploadLinks.integration.test.ts diff --git a/backend/src/app.ts b/backend/src/app.ts index 5c3ccd9..87dea74 100755 --- a/backend/src/app.ts +++ b/backend/src/app.ts @@ -9,6 +9,7 @@ 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 adminVersionRouter from './routes/adminVersion'; import filtersRouter from './routes/filters'; import customersRouter from './routes/customers'; @@ -20,6 +21,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 +38,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'; @@ -79,6 +74,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/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/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/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); + }); +}); -- 2.54.0 From 1fc632598a0c89f45be86ff973c2d4c636918d96 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Mon, 31 Aug 2026 15:10:16 -0500 Subject: [PATCH 6/9] feat(intake): accept photo submissions through a shared link (#222) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The public way in. Photos of one item plus a free-text note, from someone with no account, landing as an `items` row at status 'pending' — already invisible to every public and storefront query since #90, so nothing is live by accident. Every refusal is a 404. Unknown, revoked and exhausted links are indistinguishable from outside, because whether a link exists is not something a stranger needs to be able to learn — the same reasoning uploads.ts applies to files. The link is resolved *before* multer runs, and that ordering is the point rather than an implementation detail. discardUnlessAccepted would delete the files afterwards, but "written then deleted" is materially worse than "never written" on an endpoint the whole internet can reach: it is disk churn an unauthenticated caller controls, and it leans on an unlink that a crash between write and delete would skip. A test asserts the volume is untouched for a bad token, so a future reordering fails loudly instead of quietly handing that control away. The link counter is incremented 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. The response carries no item id: the sender has no business knowing about the catalogue and nothing they could do with it. 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 waits at state 'queued' for #223. The new limiter keys on the caller alone, since a submission carries no email. keyByCallerAndEmail's comment warns that a bare ip bucket is a shared allowance, and that trade is taken knowingly: the link is the per-caller identity and its cap is the per-caller bound, while this limiter does the different job of bounding what one address can throw at an endpoint that writes files. Twenty per fifteen minutes is deliberately looser than the password-reset allowance — somebody photographing a box of stock legitimately submits several in a row, and refusing them costs a consignment. Because the route mounts the shared uploadImages, it inherits the type allowlist, the magic-byte check and #226's EXIF stripping without asking for any of them. A test asserts the stripping specifically, since this is the route where it matters most: the photo comes from a stranger's phone rather than the shop's own camera. Backend: 284 integration (12 new), 309 unit, lint unchanged at 6 pre-existing warnings, build clean. Ref #222 --- backend/src/app.ts | 4 + backend/src/rateLimit.ts | 32 +++ backend/src/routes/intake.ts | 164 ++++++++++++++ .../integration/intake.integration.test.ts | 209 ++++++++++++++++++ 4 files changed, 409 insertions(+) create mode 100644 backend/src/routes/intake.ts create mode 100644 backend/tests/integration/intake.integration.test.ts diff --git a/backend/src/app.ts b/backend/src/app.ts index 87dea74..bf9da94 100755 --- a/backend/src/app.ts +++ b/backend/src/app.ts @@ -10,6 +10,7 @@ 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'; @@ -63,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 diff --git a/backend/src/rateLimit.ts b/backend/src/rateLimit.ts index 8517b9b..6b4cc29 100644 --- a/backend/src/rateLimit.ts +++ b/backend/src/rateLimit.ts @@ -124,3 +124,35 @@ 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 ?? ''); +} + +// Deliberately looser than the password-reset allowance. Somebody photographing +// a box of stock legitimately submits several items in a row, and the cost of +// refusing them is a lost consignment — whereas the cost of allowing a few too +// many is some disk the volume guard and the per-link cap already bound. +export const intakeLimiter = rateLimit({ + windowMs: 15 * 60 * 1000, + limit: 20, + keyGenerator: keyByCaller, + standardHeaders: 'draft-7', + legacyHeaders: false, + message: { error: 'too many submissions — please try again later' } +}); diff --git a/backend/src/routes/intake.ts b/backend/src/routes/intake.ts new file mode 100644 index 0000000..ec4df6d --- /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 { intakeLimiter } 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', intakeLimiter, 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', + intakeLimiter, + 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/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); + }); +}); -- 2.54.0 From e7b01fdb360ccc221a0d2cc0f7c59b94d52c7598 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Mon, 31 Aug 2026 15:17:40 -0500 Subject: [PATCH 7/9] feat(intake): add the public submission page (#222) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Where someone with no account sends in photos of one item. Route /submit/:token, outside the authentik gate by design: the token in the URL is the whole access control, which is what #222 chose deliberately over accounts. One state for every refusal, matching the server's single 404. Unknown, revoked and used-up links all render the same "this link is not active" card, because saying which kind of dead it was would tell a stranger whether a link they guessed at exists — the server is careful about that and the page must not undo it. `beforeUpload` returns false so antd keeps the files rather than uploading each one as it is picked. The submission is then a single request the server can accept or refuse as a unit, which is what makes the transaction on the other side meaningful. The accepted types and the six-file cap are stated here so the picker offers exactly what will be taken, but both are checked again server-side, because everything on this page is under the sender's control. The fetch effect guards against a late response from a previous token overwriting the current answer, which is reachable simply by editing the URL. TypeScript caught a real mistake rather than a stylistic one: `.filter((f): f is File => ...)` on antd's originFileObj does not narrow, because RcFile extends File and the predicate would widen rather than narrow. flatMap avoids the predicate entirely. Verified in a browser rather than by inspection: a throwaway Playwright run against the live stack confirmed the form renders for a good token, the inactive card renders for a bad one, and a photo can actually be sent and acknowledged. The database then showed the item at status pending with the default price, the draft carrying the note and its originating link, the image row written, the link's counter at one — and zero storefront-visible items, which is the property that matters most. Ref #222 --- frontend/src/intake/Submit.tsx | 164 +++++++++++++++++++++++++++++++ frontend/src/intake/intakeApi.ts | 45 +++++++++ frontend/src/main.tsx | 3 + 3 files changed, 212 insertions(+) create mode 100644 frontend/src/intake/Submit.tsx create mode 100644 frontend/src/intake/intakeApi.ts diff --git a/frontend/src/intake/Submit.tsx b/frontend/src/intake/Submit.tsx new file mode 100644 index 0000000..d1db0ec --- /dev/null +++ b/frontend/src/intake/Submit.tsx @@ -0,0 +1,164 @@ +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'; + +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 [label, setLabel] = useState(null); + 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((link) => { + // 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; + setLabel(link?.label ?? null); + 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 ( +
+ +
+ ); + } + + // One state for every refusal, matching the server's single 404. Saying which + // of revoked, unknown or used-up it was would tell a stranger whether a link + // they guessed at exists. + if (label === null) { + 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)} + > + + + +