docs(intake): design and implementation plans for the intake pipeline (#220) #221
@@ -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
|
||||
<Input
|
||||
placeholder="Max uses"
|
||||
value={cap}
|
||||
disabled={unlimited}
|
||||
onChange={(e) => setCap(e.target.value)}
|
||||
style={{ width: 140 }}
|
||||
/>
|
||||
<Checkbox checked={unlimited} onChange={(e) => setUnlimited(e.target.checked)}>
|
||||
No limit
|
||||
</Checkbox>
|
||||
```
|
||||
|
||||
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/<token>`, 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.
|
||||
|
||||
Reference in New Issue
Block a user