diff --git a/backend/src/rateLimit.ts b/backend/src/rateLimit.ts index 6b4cc29..4498cc9 100644 --- a/backend/src/rateLimit.ts +++ b/backend/src/rateLimit.ts @@ -144,13 +144,41 @@ 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({ +/** + * Two limiters rather than one, because the two requests cost different things. + * + * Reading a link is a page load: it hits one indexed row and writes nothing. + * Submitting writes up to six files to the uploads volume. Counting them + * against a single allowance meant reloading the page consumed the budget for + * sending items, and at twenty apiece that allowance ran out after ten items — + * for exactly the person this feature is for, somebody working through a box + * of stock. The comment here used to say refusing them costs a consignment, + * while the number quietly did it. + * + * Both still key on the caller alone, since a submission carries no email. The + * `keyByCallerAndEmail` comment warns that a bare `ip:` bucket is a shared + * allowance rather than a per-caller one, and that trade is accepted here: the + * link is the per-caller identity and its `max_submissions` is the per-caller + * cap, while these bound what one address can throw at an unauthenticated + * endpoint. + */ +export const intakeViewLimiter = rateLimit({ windowMs: 15 * 60 * 1000, - limit: 20, + // Generous, because it is a page load. Someone re-reading the form, losing + // their signal, or coming back to it should never be told to wait. + limit: 120, + keyGenerator: keyByCaller, + standardHeaders: 'draft-7', + legacyHeaders: false, + message: { error: 'too many requests — please try again shortly' } +}); + +export const intakeSubmitLimiter = rateLimit({ + windowMs: 15 * 60 * 1000, + // Each of these writes files, so this is the one worth bounding. Thirty in a + // quarter of an hour is more than anyone photographing items can manage and + // far less than a script would want. + limit: 30, keyGenerator: keyByCaller, standardHeaders: 'draft-7', legacyHeaders: false, diff --git a/backend/src/routes/intake.ts b/backend/src/routes/intake.ts index ec4df6d..62db46c 100644 --- a/backend/src/routes/intake.ts +++ b/backend/src/routes/intake.ts @@ -3,7 +3,7 @@ import { pool, requireRow } from '../db'; import { asyncRoute } from '../asyncRoute'; import { hashToken } from '../uploadLinks'; import { uploadImages, verifyUploadedImages, insertItemImages } from '../imageUpload'; -import { intakeLimiter } from '../rateLimit'; +import { intakeViewLimiter, intakeSubmitLimiter } from '../rateLimit'; const router = Router(); @@ -72,7 +72,7 @@ const requireUsableLink = asyncRoute( } ); -router.get('/:token', intakeLimiter, asyncRoute(async (req: Request, res: Response) => { +router.get('/:token', intakeViewLimiter, asyncRoute(async (req: Request, res: Response) => { const link = await usableLink(req.params.token as string); if (!link) { return res.status(404).json({ error: 'not found' }); @@ -83,7 +83,7 @@ router.get('/:token', intakeLimiter, asyncRoute(async (req: Request, res: Respon router.post( '/:token', - intakeLimiter, + intakeSubmitLimiter, requireUsableLink, uploadImages, asyncRoute(async (req: Request, res: Response) => { diff --git a/frontend/src/intake/Submit.tsx b/frontend/src/intake/Submit.tsx index d1db0ec..2151898 100644 --- a/frontend/src/intake/Submit.tsx +++ b/frontend/src/intake/Submit.tsx @@ -11,6 +11,7 @@ import Space from 'antd/es/space'; import { UploadOutlined } from '@ant-design/icons'; import type { UploadFile } from 'antd/es/upload/interface'; import { fetchIntakeLink, submitItem } from './intakeApi'; +import type { LinkState } from './intakeApi'; const { Title, Paragraph } = Typography; const { TextArea } = Input; @@ -28,7 +29,7 @@ const MAX_IMAGES = 6; export default function Submit() { const { token = '' } = useParams(); - const [label, setLabel] = useState(null); + const [state, setState] = useState({ kind: 'unusable' }); const [checking, setChecking] = useState(true); const [files, setFiles] = useState([]); const [note, setNote] = useState(''); @@ -38,11 +39,11 @@ export default function Submit() { useEffect(() => { let cancelled = false; - void fetchIntakeLink(token).then((link) => { + void fetchIntakeLink(token).then((result) => { // The token can change if the URL does, and a late response from the // previous one would otherwise overwrite the current answer. if (cancelled) return; - setLabel(link?.label ?? null); + setState(result); setChecking(false); }); return () => { @@ -80,10 +81,28 @@ export default function Submit() { ); } - // 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) { + // Kept apart from the state below on purpose. The two need opposite + // reactions — wait a moment, versus go and ask for a different link — so + // telling a throttled sender their link was dead would send them to fetch a + // replacement that could not have helped. + if (state.kind === 'throttled') { + return ( +
+ + Too many requests just now + + Your link is fine — this page has just been asked for too many times from your + connection. Wait a minute and reload. + + +
+ ); + } + + // One state for unknown, revoked and used-up alike, matching the server's + // single 404. Saying which it was would tell a stranger whether a link they + // guessed at exists. + if (state.kind === 'unusable') { return (
diff --git a/frontend/src/intake/intakeApi.ts b/frontend/src/intake/intakeApi.ts index 4b4d495..a743183 100644 --- a/frontend/src/intake/intakeApi.ts +++ b/frontend/src/intake/intakeApi.ts @@ -2,19 +2,33 @@ export interface IntakeLink { label: string; } +export type LinkState = + | { kind: 'usable'; link: IntakeLink } + | { kind: 'unusable' } + | { kind: 'throttled' }; + /** * Whether a link works, and what it is called. * - * Every refusal from the server is a 404 by design — unknown, revoked and - * exhausted links are indistinguishable, because whether a link exists is not - * something a stranger needs to learn. So there is one "this link does not - * work" state here rather than several the page would have to explain, and - * null is the whole of it. + * The server answers 404 for unknown, revoked and exhausted links alike, and + * that is deliberate — whether a link exists is not something a stranger needs + * to learn. So all three collapse into one `unusable` state here rather than + * several the page would have to explain. + * + * A 429 is deliberately *not* one of them. This used to treat any non-OK + * response as "no link", which meant a rate-limited sender was told their link + * was dead — sending them to ask for a replacement that would not have helped, + * because the problem was the address they were coming from and a minute of + * patience. Two conditions that need opposite reactions must not share a + * message. */ -export async function fetchIntakeLink(token: string): Promise { +export async function fetchIntakeLink(token: string): Promise { const res = await fetch(`/api/intake/${encodeURIComponent(token)}`); - if (!res.ok) return null; - return res.json(); + + if (res.status === 429) return { kind: 'throttled' }; + if (!res.ok) return { kind: 'unusable' }; + + return { kind: 'usable', link: await res.json() }; } export type SubmitResult = { ok: true } | { ok: false; error: string }; diff --git a/frontend/tests/e2e/admin-upload-links.spec.ts b/frontend/tests/e2e/admin-upload-links.spec.ts new file mode 100644 index 0000000..3317821 --- /dev/null +++ b/frontend/tests/e2e/admin-upload-links.spec.ts @@ -0,0 +1,72 @@ +import { test, expect, uniqueSuffix } from './fixtures'; + +/** + * Issuing and revoking upload links from the admin (#222). + * + * Every link is labelled with a fresh run id and every assertion is scoped to + * that row. The suite is fullyParallel against one shared database, so an + * assertion on the table as a whole would be an assertion on whatever other + * specs happen to be doing (#241). + */ + +test.describe('Managing upload links', () => { + test('issues a link, shows its token once, and lists it bounded', async ({ page, admin }) => { + const label = `Sarah ${uniqueSuffix()}`; + + await admin.goto(); + await page.getByRole('tab', { name: 'Upload links' }).click(); + + await page.getByLabel('Link label').fill(label); + await page.getByRole('button', { name: 'Create link' }).click(); + + // Shown exactly once. The server keeps only a digest, so there is no + // reveal to come back to — the copy has to say so. + await expect(page.getByText('Copy this link now')).toBeVisible(); + await expect(page.getByText(/\/submit\//)).toBeVisible(); + await expect(page.getByText(/cannot be shown again/)).toBeVisible(); + + const row = page.getByRole('row', { name: new RegExp(label) }); + await expect(row).toBeVisible(); + // The default cap, not unlimited. An unbounded link should be asked for. + await expect(row.getByText('0 of 25')).toBeVisible(); + await expect(row.getByText('Active')).toBeVisible(); + }); + + test('revokes a link, naming the action on the confirm', async ({ page, admin }) => { + const label = `Temporary ${uniqueSuffix()}`; + + await admin.goto(); + await page.getByRole('tab', { name: 'Upload links' }).click(); + await page.getByLabel('Link label').fill(label); + await page.getByRole('button', { name: 'Create link' }).click(); + + const row = page.getByRole('row', { name: new RegExp(label) }); + await row.getByRole('button', { name: 'Revoke' }).click(); + + // The confirm names what it does rather than saying OK, which is what the + // rest of this admin's destructive actions do. + await page.getByRole('tooltip').getByRole('button', { name: 'Revoke' }).click(); + + await expect(row.getByText('Revoked')).toBeVisible(); + // The row stays: what arrived through the link is kept, and the record of + // where it came from with it. + await expect(row).toBeVisible(); + }); + + // Blank-means-unlimited would make the least deliberate action produce the + // least bounded link, so unlimited is a checkbox rather than an empty field. + test('makes unlimited a deliberate choice', async ({ page, admin }) => { + const label = `Always on ${uniqueSuffix()}`; + + await admin.goto(); + await page.getByRole('tab', { name: 'Upload links' }).click(); + await page.getByLabel('Link label').fill(label); + await page.getByText('No limit').click(); + await page.getByRole('button', { name: 'Create link' }).click(); + + const row = page.getByRole('row', { name: new RegExp(label) }); + await expect(row).toBeVisible(); + // A bare count rather than "0 of N". + await expect(row.getByText('0 of', { exact: false })).toHaveCount(0); + }); +}); diff --git a/frontend/tests/e2e/intake-submit.spec.ts b/frontend/tests/e2e/intake-submit.spec.ts new file mode 100644 index 0000000..d5af958 --- /dev/null +++ b/frontend/tests/e2e/intake-submit.spec.ts @@ -0,0 +1,76 @@ +import { test, expect, createAdminContext, uniqueSuffix } from './fixtures'; + +/** + * The public submission page (#222). + * + * Every fixture here carries a run id and every assertion names only what this + * run created. The suite is fullyParallel against one shared database, so a + * spec that asserts on anything catalogue-wide is asserting on other specs too + * (#241). + */ +const RUN = `i${uniqueSuffix()}`; + +let token = ''; + +test.beforeAll(async ({ playwright }) => { + const api = await createAdminContext(playwright); + + const res = await api.post('/api/admin/upload-links', { + data: { label: `Intake spec ${RUN}` } + }); + expect(res.status(), 'creating the upload link').toBe(201); + token = (await res.json()).token; + + await api.dispose(); +}); + +test.describe('Sending in an item through a link', () => { + test('shows the form for a link that works', async ({ page }) => { + await page.goto(`/submit/${token}`); + + await expect(page.getByRole('heading', { name: 'Send in an item' })).toBeVisible(); + await expect(page.getByRole('button', { name: 'Choose photos' })).toBeVisible(); + }); + + // Nothing to send is not a submission, and the server would refuse it — but + // the sender should not have to find that out by pressing the button. + test('will not send until a photo is chosen', async ({ page }) => { + await page.goto(`/submit/${token}`); + + await expect(page.getByRole('button', { name: 'Send' })).toBeDisabled(); + }); + + // One state for every refusal, matching the server's single 404. Saying which + // kind of dead a link is would tell a stranger whether one they guessed at + // exists. + test('explains an unusable link without saying which kind', async ({ page }) => { + await page.goto('/submit/not-a-real-token'); + + await expect(page.getByRole('heading', { name: 'This link is not active' })).toBeVisible(); + await expect(page.getByRole('button', { name: 'Choose photos' })).toHaveCount(0); + }); + + test('accepts a photo and says it arrived', async ({ page }) => { + await page.goto(`/submit/${token}`); + + // A real 1x1 PNG, so the server's magic-byte check sees what it expects + // rather than a buffer that merely starts correctly. + const png = Buffer.from( + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==', + 'base64' + ); + await page.setInputFiles('input[type="file"]', { + name: `${RUN}.png`, + mimeType: 'image/png', + buffer: png + }); + await page.getByLabel('Anything you know about this item').fill(`Stoneware ${RUN}`); + + await page.getByRole('button', { name: 'Send' }).click(); + + await expect(page.getByRole('heading', { name: 'Thank you — it arrived' })).toBeVisible(); + // The wording matters: it is what stops a sender wondering why their item + // is not on the site. + await expect(page.getByText(/Nothing is listed for sale until/)).toBeVisible(); + }); +});