From 1fc632598a0c89f45be86ff973c2d4c636918d96 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Mon, 31 Aug 2026 15:10:16 -0500 Subject: [PATCH] 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); + }); +});