fix(intake): stop a throttled sender being told their link is dead (#222)

Adding e2e specs for the submission page found a defect in the page they were written for, which is what they were for.

One limiter counted page loads and submissions against the same twenty-per-quarter-hour allowance, so a sender working through a box of stock ran out after ten items — the exact person the feature exists for, and the exact case the limiter's own comment said must not be refused. The comment said refusing them costs a consignment while the number quietly did it.

Worse, the page could not tell a 429 from a 404. `fetchIntakeLink` treated any non-OK response as "no link", so a throttled sender was told "This link is not active" and sent to ask for a replacement — which could not have helped, because the problem was their address and a minute of patience. Two conditions needing opposite reactions were sharing a message.

Now two limiters, because the two requests cost different things. Reading a link hits one indexed row and writes nothing, so that allowance is generous at 120: someone re-reading the form or losing their signal should never be told to wait. Submitting writes up to six files, so that is the one worth bounding, at 30 — more than anyone photographing items can manage and far less than a script would want.

The page gains a third state. Unknown, revoked and used-up still collapse into one "not active" card, because whether a link exists is not something a stranger needs to learn. Throttled is deliberately kept apart from them, since "wait a moment" and "go and ask for another link" are opposite instructions.

Measured rather than assumed, on a freshly started process both times: before, 25 page loads produced 14 rejections; after, 40 produce none. The first attempt at that measurement was wrong and worth recording — the restart had failed with EADDRINUSE, so it read 30 of 30 against the old process's already-exhausted store.

The two specs now pass in a full parallel run alongside everything else. They are scoped the way #241 asks: unique run ids, assertions naming only this run's rows, nothing asserted about the table as a whole.

Backend: 284 integration, 309 unit. Frontend: build clean, lint unchanged at 2 pre-existing warnings.

Ref #222, #241

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
+ * Two limiters rather than one, because the two requests cost different things.
-// 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.
+ * Reading a link is a page load: it hits one indexed row and writes nothing.
-export const intakeLimiter = rateLimit({
+ * 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,

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) => {

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<string | null>(null);
+  const [state, setState] = useState<LinkState>({ kind: 'unusable' });
   const [checking, setChecking] = useState(true);
   const [files, setFiles] = useState<UploadFile[]>([]);
   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
+  // Kept apart from the state below on purpose. The two need opposite
-  // of revoked, unknown or used-up it was would tell a stranger whether a link
+  // reactions — wait a moment, versus go and ask for a different link — so
-  // they guessed at exists.
+  // telling a throttled sender their link was dead would send them to fetch a
-  if (label === null) {
+  // replacement that could not have helped.
+  if (state.kind === 'throttled') {
+    return (
+      <div style={{ maxWidth: 640, margin: '48px auto', padding: '0 16px' }}>
+        <Card>
+          <Title level={3}>Too many requests just now</Title>
+          <Paragraph>
+            Your link is fine — this page has just been asked for too many times from your
+            connection. Wait a minute and reload.
+          </Paragraph>
+        </Card>
+      </div>
+    );
+  }
+
+  // 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 (
       <div style={{ maxWidth: 640, margin: '48px auto', padding: '0 16px' }}>
         <Card>

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
+ * The server answers 404 for unknown, revoked and exhausted links alike, and
- * exhausted links are indistinguishable, because whether a link exists is not
+ * that is deliberate — whether a link exists is not something a stranger needs
- * something a stranger needs to learn. So there is one "this link does not
+ * to learn. So all three collapse into one `unusable` state here rather than
- * work" state here rather than several the page would have to explain, and
+ * several the page would have to explain.
- * null is the whole of it.
+ *
+ * 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<IntakeLink | null> {
+export async function fetchIntakeLink(token: string): Promise<LinkState> {
   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 };

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);
+  });
+});

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();
+  });
+});