feat(intake): issue and revoke named upload links (#222)

Three routes behind the admin gate: list, create, revoke. A link is named because provenance matters more than convenience — when one is shared further than intended the question is which one, and every submission will record the link it arrived through, so revoking kills that link rather than the feature.

The token is returned by exactly one response and is unrecoverable afterwards, which is why the admin screen has to present it as a one-time reveal. The listing selects its columns explicitly rather than `SELECT *`, so `token_hash` cannot reach a response the moment somebody adds a convenience — and a test asserts the listing carries neither the token nor the digest.

An absent `maxSubmissions` gets a bounded default of 25 rather than null. Absent means nobody decided; an explicit null means unlimited, which is a decision visible in the request. Reading absent as unlimited is what would quietly make every link unbounded, and the common case is the one that has to be safe.

Revoking is idempotent through COALESCE, and a test asserts the second call returns the *same* timestamp rather than merely succeeding. The useful fact is when access ended, and a button that errors on a double-click teaches people to distrust it — which is the last thing wanted on the control that contains a leak.

Mounted above the `/api/admin` catch-all, which would otherwise swallow the path, and behind requireAdminGate on the router itself per the reasoning in middleware/adminGate.ts.

Lint caught me reintroducing something this codebase had already solved: I wrote `.replace(/\/+$/, '')` to trim PUBLIC_URL, and app.ts carried a hand-written loop with a comment explaining that exact regex backtracks. Rather than duplicate the loop, trimTrailingSlashes moved to utils.ts and both callers now share it.

Backend: 272 integration (9 new), 308 unit, lint back to its 6 pre-existing warnings, build clean.

Ref #222

backend/src/app.ts

@@ -9,6 +9,7 @@ import adminSettingsRouter from './routes/adminSettings';
 import adminEmailTemplatesRouter from './routes/adminEmailTemplates';
 import adminCategoriesRouter from './routes/adminCategories';
 import adminTagsRouter from './routes/adminTags';
+import adminUploadLinksRouter from './routes/adminUploadLinks';
 import adminVersionRouter from './routes/adminVersion';
 import filtersRouter from './routes/filters';
 import customersRouter from './routes/customers';
@@ -20,6 +21,7 @@ import { attachCustomer } from './middleware/customerAuth';
 import { requireAdminGate } from './middleware/adminGate';
 import { asyncRoute } from './asyncRoute';
 import { uploadsRouter } from './uploads';
+import { trimTrailingSlashes } from './utils';
 
 const app = express();
 // Express advertises itself in X-Powered-By by default, which hands an
@@ -36,13 +38,6 @@ app.use(cookieParser());
 app.use(asyncRoute(attachCustomer));
 app.use('/uploads', uploadsRouter(process.env.UPLOADS_DIR || '/app/uploads'));
 
-// Trimmed with a loop rather than a `/+$/` regex, which backtracks.
-function trimTrailingSlashes(value: string): string {
-  let trimmed = value;
-  while (trimmed.endsWith('/')) trimmed = trimmed.slice(0, -1);
-  return trimmed;
-}
-
 app.get('/api/config', (_req, res) => {
   const clientId = process.env.PAYPAL_CLIENT_ID;
   const isPlaceholder = !clientId || clientId.length < 10 || clientId === 'REPLACE_WITH_PAYPAL_CLIENT_ID';
@@ -79,6 +74,7 @@ app.use('/api/admin/settings', requireAdminGate, adminSettingsRouter);
 app.use('/api/admin/email-templates', requireAdminGate, adminEmailTemplatesRouter);
 app.use('/api/admin/categories', requireAdminGate, adminCategoriesRouter);
 app.use('/api/admin/tags', requireAdminGate, adminTagsRouter);
+app.use('/api/admin/upload-links', requireAdminGate, adminUploadLinksRouter);
 app.use('/api/admin/version', requireAdminGate, adminVersionRouter);
 app.use('/api/admin', requireAdminGate, adminRouter);
 app.use('/api/customers/me/addresses', shippingAddressesRouter);

backend/src/routes/adminUploadLinks.ts

@@ -0,0 +1,116 @@
+import { Router, Request, Response } from 'express';
+import { pool, requireRow } from '../db';
+import { asyncRoute } from '../asyncRoute';
+import { generateToken, hashToken } from '../uploadLinks';
+import { trimTrailingSlashes } from '../utils';
+
+const router = Router();
+
+/**
+ * Issuing and retiring the links that open the public intake endpoint (#222).
+ *
+ * A link is named because provenance matters more than convenience here. When
+ * one is shared further than intended the question is *which* one, and the
+ * answer has to come from somewhere — so every submission records the link it
+ * arrived through, and revoking kills that link rather than the feature.
+ *
+ * The token is returned by exactly one response in this file and is
+ * unrecoverable afterwards. That is why the admin screen has to present it as
+ * a one-time reveal rather than a field to come back to, and why losing it
+ * means issuing a new link rather than looking the old one up.
+ */
+
+/**
+ * Shaped so a `SELECT *` can never leak the digest into a response.
+ *
+ * Spelling the columns out is the point: `SELECT *` here would put
+ * `token_hash` into every listing the moment somebody added a convenience.
+ */
+const LINK_SELECT = `
+  SELECT id, label, revoked_at, submission_count, max_submissions, last_used_at, created_at
+  FROM upload_links
+`;
+
+/**
+ * The cap a link gets when nobody chose one.
+ *
+ * Not a tuned number — large enough that an ordinary contributor never meets
+ * it, small enough that a link shared further than intended cannot be used
+ * indefinitely before anyone notices. The point is that the default is finite
+ * at all.
+ */
+const DEFAULT_MAX_SUBMISSIONS = 25;
+
+interface UploadLinkRow {
+  id: number;
+  label: string;
+  revoked_at: string | null;
+  submission_count: number;
+  max_submissions: number | null;
+  last_used_at: string | null;
+  created_at: string;
+}
+
+router.get('/', asyncRoute(async (_req: Request, res: Response) => {
+  const { rows } = await pool.query<UploadLinkRow>(`${LINK_SELECT} ORDER BY created_at DESC`);
+  res.json(rows);
+}));
+
+router.post('/', asyncRoute(async (req: Request, res: Response) => {
+  const label = typeof req.body?.label === 'string' ? req.body.label.trim() : '';
+  if (label === '') {
+    return res.status(400).json({ error: 'a label is required' });
+  }
+
+  // Three cases, deliberately distinct. Absent means nobody decided, which
+  // gets the bounded default. An explicit null means unlimited — a decision
+  // someone made, visible in the request. A number is itself. Reading absent
+  // as unlimited is what would make every link unbounded by default.
+  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;
+  }
+
+  const token = generateToken();
+  const { rows } = await pool.query<UploadLinkRow>(
+    `INSERT INTO upload_links (label, token_hash, max_submissions)
+     VALUES ($1, $2, $3)
+     RETURNING id, label, revoked_at, submission_count, max_submissions, last_used_at, created_at`,
+    [label, hashToken(token), maxSubmissions]
+  );
+  const link = requireRow(rows, 'the upload_links INSERT');
+
+  // PUBLIC_URL is already required alongside SMTP and is what every other
+  // outbound link is built from. Absent in local development, which yields a
+  // relative URL the admin screen can still show and copy usefully.
+  const base = trimTrailingSlashes(process.env.PUBLIC_URL ?? '');
+  res.status(201).json({ ...link, token, url: `${base}/submit/${token}` });
+}));
+
+router.post('/:id/revoke', asyncRoute(async (req: Request, res: Response) => {
+  // COALESCE so revoking twice keeps the original timestamp. The useful fact
+  // is when access ended, and a second click should neither rewrite that nor
+  // fail — a button that errors on a double-click teaches people to distrust
+  // it, which is the last thing wanted on the control that contains a leak.
+  const { rows } = await pool.query<UploadLinkRow>(
+    `UPDATE upload_links SET revoked_at = COALESCE(revoked_at, now())
+     WHERE id = $1
+     RETURNING id, label, revoked_at, submission_count, max_submissions, last_used_at, created_at`,
+    [req.params.id]
+  );
+
+  const link = rows[0];
+  if (!link) {
+    return res.status(404).json({ error: 'not found' });
+  }
+  res.json(link);
+}));
+
+export default router;

backend/src/utils.ts

@@ -73,3 +73,21 @@ export function tagColorFor(name: string): string {
 
 export const MARKETING_CONSENT_TEXT =
   'I want to receive occasional emails about new one-of-a-kind items from Redefined Designs. I can unsubscribe at any time.';
+
+/**
+ * Strips trailing slashes so a base URL can be joined with a stored path.
+ *
+ * A loop rather than `/\/+$/`, which backtracks: sonarjs flags that pattern as
+ * super-linear, and the input here is an environment variable rather than
+ * anything hostile, but the cheap version is no harder to read.
+ *
+ * Shared because two callers now need it — `/api/config` sends
+ * `uploadsBaseUrl` this way, and the upload-link routes build a submission URL
+ * from PUBLIC_URL. Stored paths always begin with a slash, so trimming the
+ * base is what stops the join producing a double.
+ */
+export function trimTrailingSlashes(value: string): string {
+  let trimmed = value;
+  while (trimmed.endsWith('/')) trimmed = trimmed.slice(0, -1);
+  return trimmed;
+}

backend/tests/integration/uploadLinks.integration.test.ts

@@ -0,0 +1,110 @@
+import request from 'supertest';
+import app from '../../src/app';
+import { pool } from '../../src/db';
+import { resetDb, closeDb } from './setup/testDb';
+
+beforeEach(async () => {
+  await resetDb();
+});
+
+afterAll(async () => {
+  await pool.end();
+  await closeDb();
+});
+
+describe('issuing an upload link', () => {
+  it('returns the token exactly once, at creation', async () => {
+    const created = await request(app)
+      .post('/api/admin/upload-links')
+      .send({ label: 'Sarah' });
+
+    expect(created.status).toBe(201);
+    expect(created.body.label).toBe('Sarah');
+    expect(created.body.token).toMatch(/^[A-Za-z0-9_-]{43}$/);
+    expect(created.body.url).toContain(`/submit/${created.body.token}`);
+
+    const listed = await request(app).get('/api/admin/upload-links');
+    expect(listed.status).toBe(200);
+    expect(listed.body).toHaveLength(1);
+    // The whole point of storing a digest: the listing cannot hand it back.
+    expect(listed.body[0].token).toBeUndefined();
+    expect(listed.body[0].token_hash).toBeUndefined();
+  });
+
+  it('stores the digest rather than the token', async () => {
+    const created = await request(app)
+      .post('/api/admin/upload-links')
+      .send({ label: 'Estate sale box 3' });
+
+    const { rows } = await pool.query<{ token_hash: string }>(
+      `SELECT token_hash FROM upload_links`
+    );
+    expect(rows[0]?.token_hash).not.toBe(created.body.token);
+    expect(rows[0]?.token_hash).toMatch(/^[a-f0-9]{64}$/);
+  });
+
+  it('refuses a link with no label', async () => {
+    const res = await request(app).post('/api/admin/upload-links').send({ label: '  ' });
+    expect(res.status).toBe(400);
+  });
+
+  it('refuses a non-positive submission cap', async () => {
+    const res = await request(app)
+      .post('/api/admin/upload-links')
+      .send({ label: 'Bad cap', maxSubmissions: 0 });
+    expect(res.status).toBe(400);
+  });
+
+  // Omitting the field is the common case, so it is the case that has to 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();
+  });
+});
+
+describe('revoking an upload link', () => {
+  it('stamps revoked_at and reports it in the listing', async () => {
+    const created = await request(app)
+      .post('/api/admin/upload-links')
+      .send({ label: 'Temporary' });
+
+    const revoked = await request(app)
+      .post(`/api/admin/upload-links/${created.body.id}/revoke`);
+
+    expect(revoked.status).toBe(200);
+    expect(revoked.body.revoked_at).not.toBeNull();
+  });
+
+  // The useful fact is when access ended, so a second click must not rewrite
+  // it — and it must not be an error either, because a button that fails on a
+  // double-click teaches people to distrust it.
+  it('is idempotent, keeping the original timestamp', async () => {
+    const created = await request(app)
+      .post('/api/admin/upload-links')
+      .send({ label: 'Temporary' });
+
+    const first = await request(app).post(`/api/admin/upload-links/${created.body.id}/revoke`);
+    const second = await request(app).post(`/api/admin/upload-links/${created.body.id}/revoke`);
+
+    expect(second.status).toBe(200);
+    expect(second.body.revoked_at).toBe(first.body.revoked_at);
+  });
+
+  it('404s for a link that does not exist', async () => {
+    const res = await request(app).post('/api/admin/upload-links/9999/revoke');
+    expect(res.status).toBe(404);
+  });
+});