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 cbb7d74..77e8b0c 100644 --- a/docs/superpowers/plans/2026-08-29-intake-upload-links.md +++ b/docs/superpowers/plans/2026-08-29-intake-upload-links.md @@ -37,6 +37,7 @@ - `backend/src/routes/adminUploadLinks.ts` — admin CRUD and revoke - `backend/src/routes/intake.ts` — the public submission endpoint - `backend/tests/unit/uploadLinks.test.ts` +- `backend/tests/unit/imageUpload.test.ts` - `backend/tests/integration/uploadLinks.integration.test.ts` - `backend/tests/integration/intake.integration.test.ts` - `frontend/src/intake/Submit.tsx` — the public submission page @@ -1501,14 +1502,303 @@ git commit -m "feat(intake): manage upload links from the admin (#NNN)" --- +### Task 8: Refuse uploads when the volume is nearly full + +Both of the remaining tasks must come after Task 2 (the extraction). Task 8 is independent of Tasks 3–7; Task 9 edits the router Task 4 writes, so it comes after that. + +The gap this closes: the ordering fix in Task 5 stops a caller with a *bad* token writing anything, but a caller with a working one can still send 6 × 8 MB per request against a limiter that allows 20 requests per window. Nothing anywhere checks whether the volume can take it, and the volume is shared with the admin upload path — so intake filling it breaks the whole shop, not just intake. + +**Files:** +- Modify: `backend/src/imageUpload.ts`, `backend/src/routes/intake.ts`, `backend/src/routes/admin.ts` +- Test: `backend/tests/unit/imageUpload.test.ts` (create), `backend/tests/integration/intake.integration.test.ts` (extend) + +**Interfaces:** +- Produces: + - `hasRoomFor(availableBytes: number): boolean` — pure, exported for its test + - `requireVolumeRoom` — middleware, `503` when the volume is low + +- [ ] **Step 1: Write the failing unit test** + +Create `backend/tests/unit/imageUpload.test.ts`: + +```ts +import { hasRoomFor, MIN_FREE_BYTES } from '../../src/imageUpload'; + +// The threshold decision is pure and is the whole of the policy, so it is +// tested directly rather than through a request — the same reasoning that has +// keyByCallerAndEmail and isAllowedRecipient exported for their tests. +describe('hasRoomFor', () => { + it('accepts a volume with room to spare', () => { + expect(hasRoomFor(MIN_FREE_BYTES * 10)).toBe(true); + }); + + it('refuses a volume below the floor', () => { + expect(hasRoomFor(MIN_FREE_BYTES - 1)).toBe(false); + }); + + // Exactly at the floor is still enough: the floor is what must remain, and + // an off-by-one here is the difference between a shop that works and one + // that refuses every upload. + it('accepts a volume exactly at the floor', () => { + expect(hasRoomFor(MIN_FREE_BYTES)).toBe(true); + }); + + it('refuses an empty volume', () => { + expect(hasRoomFor(0)).toBe(false); + }); +}); +``` + +- [ ] **Step 2: Run it to verify it fails** + +```bash +cd backend && npx jest -c jest.unit.config.js imageUpload +``` + +Expected: FAIL — `hasRoomFor` is not exported. + +- [ ] **Step 3: Implement the guard** + +Append to `backend/src/imageUpload.ts`: + +```ts +import { statfs } from 'fs/promises'; + +/** + * How much of the uploads volume must remain free for an upload to be accepted. + * + * One gigabyte is not a tuned number; it is a floor chosen so that the volume + * filling degrades into refused uploads rather than into a database that + * cannot write, logs that cannot rotate, and a shop that is down. There is + * deliberately no environment variable: an operator who wants a different floor + * is making a decision that should be visible in a diff. + */ +export const MIN_FREE_BYTES = 1_000_000_000; + +/** Pure, so the policy can be tested without a filesystem. */ +export function hasRoomFor(availableBytes: number): boolean { + return availableBytes >= MIN_FREE_BYTES; +} + +/** + * Refuses an upload when the uploads volume is nearly full. + * + * Fails closed. A volume that cannot be measured is not one to assume is + * empty — and in practice `statfs` failing means the directory is missing or + * unreadable, in which case the upload was going to fail anyway, just later + * and less clearly. + * + * 503 rather than 507: the condition is temporary and about the server rather + * than the request, and 507 is obscure enough that intermediaries and clients + * handle it inconsistently. + */ +export const requireVolumeRoom = asyncRoute( + async (_req: Request, res: Response, next: NextFunction) => { + try { + const stats = await statfs(UPLOADS_DIR); + if (!hasRoomFor(stats.bavail * stats.bsize)) { + console.error(`[upload] refusing: less than ${MIN_FREE_BYTES} bytes free on ${UPLOADS_DIR}`); + res.status(503).json({ error: 'storage is full — please try again later' }); + return; + } + } catch (err) { + console.error(`[upload] could not measure ${UPLOADS_DIR}:`, err); + res.status(503).json({ error: 'storage is unavailable — please try again later' }); + return; + } + next(); + } +); +``` + +Add to that file's imports: + +```ts +import { Request, Response, NextFunction } from 'express'; +import { asyncRoute } from './asyncRoute'; +``` + +- [ ] **Step 4: Apply it to every route that writes files** + +In `backend/src/routes/intake.ts`, before `uploadImages`: + +```ts +router.post('/:token', intakeLimiter, requireUsableLink, requireVolumeRoom, uploadImages, asyncRoute(async (req: Request, res: Response) => { +``` + +In `backend/src/routes/admin.ts`, on both routes that take files — the guard belongs on the shared pipeline, not only on the public half, since both write to the same volume: + +```ts +router.post('/items', requireVolumeRoom, uploadImages, asyncRoute(async (req: Request, res: Response) => { +``` + +```ts +router.put('/items/:id', requireVolumeRoom, uploadImages, asyncRoute(async (req: Request, res: Response) => { +``` + +Import it in both files from `../imageUpload`. + +- [ ] **Step 5: Run the unit test and the whole suite** + +```bash +cd backend +npx jest -c jest.unit.config.js imageUpload +npm run test:unit +npm run test:integration +npm run lint && npm run build +``` + +Expected: PASS throughout. The integration suites exercise the admin upload routes, so a mistake in the middleware ordering surfaces there as a 503 on every upload. + +- [ ] **Step 6: Commit** + +```bash +git add backend/src/imageUpload.ts backend/src/routes/intake.ts backend/src/routes/admin.ts backend/tests/unit/imageUpload.test.ts +git commit -m "feat(uploads): refuse uploads when the volume is nearly full (#NNN)" +``` + +--- + +### Task 9: Bound a link by default + +The gap this closes: `max_submissions` is nullable and the router as written in Task 4 sends `null` whenever the field is absent, so the ordinary act of creating a link produces an unbounded one. A cap that has to be remembered is not a control. + +**Files:** +- Modify: `backend/src/routes/adminUploadLinks.ts`, `frontend/src/admin/UploadLinks.tsx` +- Test: `backend/tests/integration/uploadLinks.integration.test.ts` (extend) + +- [ ] **Step 1: Write the failing test** + +Add to `backend/tests/integration/uploadLinks.integration.test.ts`, inside `describe('issuing an upload link')`: + +```ts + // Omitting the field is the common case, so it is the case that must 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(); + }); +``` + +- [ ] **Step 2: Run it to verify it fails** + +```bash +cd backend && npx jest -c jest.integration.config.js --runInBand uploadLinks.integration +``` + +Expected: FAIL — `max_submissions` is `null` where `25` was expected. + +- [ ] **Step 3: Change the default** + +In `backend/src/routes/adminUploadLinks.ts`, replace the cap-parsing block from Task 4 with: + +```ts +/** + * The cap a link gets when nobody chose one. + * + * Not a tuned number — it is large enough that an ordinary contributor never + * meets it and small enough that a leaked link cannot be used indefinitely + * before anyone notices. The point is that the default is finite at all. + */ +const DEFAULT_MAX_SUBMISSIONS = 25; + +// Three cases, and they are deliberately different. Absent means "nobody +// decided", which gets the bounded default. An explicit null means "unlimited", +// which is a decision someone made and can be seen in the request. A number is +// itself. Reading absent as unlimited is what made every link unbounded. +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; +} +``` + +- [ ] **Step 4: Make the frontend say so** + +In `frontend/src/admin/UploadLinks.tsx`, the cap input starts at the default and unlimited becomes a deliberate choice rather than an empty field. Replace the `cap` state and its input: + +```tsx + const [cap, setCap] = useState('25'); + const [unlimited, setUnlimited] = useState(false); +``` + +```tsx + setCap(e.target.value)} + style={{ width: 140 }} + /> + setUnlimited(e.target.checked)}> + No limit + +``` + +Import `Checkbox from 'antd/es/checkbox'`. In `create`, send the three cases explicitly: + +```tsx + body: JSON.stringify({ + label, + // null is how unlimited is asked for; a number otherwise. The field is + // never omitted from this client, so the server's default is a + // safeguard for other callers rather than this screen's behaviour. + maxSubmissions: unlimited ? null : Number(cap) + }) +``` + +And reset it after a successful create — `setCap('25'); setUnlimited(false);` in place of `setCap('')`. + +- [ ] **Step 5: Run everything** + +```bash +cd backend && npx jest -c jest.integration.config.js --runInBand uploadLinks.integration && npm run lint && npm run build +cd ../frontend && npm run lint && npm run build +``` + +Expected: PASS. + +- [ ] **Step 6: Commit** + +```bash +git add backend/src/routes/adminUploadLinks.ts backend/tests/integration/uploadLinks.integration.test.ts frontend/src/admin/UploadLinks.tsx +git commit -m "feat(intake): bound an upload link by default (#NNN)" +``` + +--- + ## Done when - A link created in `/admin` accepts a submission at `/submit/`, and the item appears in the admin inventory as pending with its photos and note. - Revoking that link makes the page show the inactive card and the endpoint return 404. - A capped link stops at its cap. - A file whose bytes disagree with its declared type is refused and leaves nothing on disk or in the database. +- A token that does not work causes no bytes to be written at all, and the test asserting that passes. +- A link created without a stated cap comes back bounded at 25; unlimited requires asking for it. - `npm run test:unit`, `npm run test:integration`, `npm run lint` and `npm run build` all pass in `backend/`; `npm run lint` and `npm run build` pass in `frontend/`. ## Not in this slice The drafting worker, the notification email and its signed action links, and the review queue. `item_drafts` is created here with its AI columns unpopulated so slice 2 adds behaviour rather than schema. The `price_source` column is written as `'default'` and not yet read. + +Two hardening items are deliberately separate issues rather than tasks here. **Re-encoding uploads to strip EXIF and cut stored bytes** touches the shared pipeline the admin path also uses, adds a native dependency, and has its own testing surface — it deserves its own review rather than riding along. **A global daily submission ceiling with an alert** needs state of its own and an outbound email, which is more than the two guards above. Both are filed against #220. + +Until the re-encode lands, uploaded photos retain their EXIF — including GPS coordinates — and are served publicly. That is true of the existing admin upload path today and is not introduced by this slice, but this slice widens who can put such a file there.