chore(sonarqube): keep interpolation out of query call sites (#294)

The quality gate was red on one condition only — new_security_hotspots_reviewed at 75 against a threshold of 100 — and the outstanding hotspot was admin.ts's `${ADMIN_ITEM_SELECT} WHERE i.id = $1`.

Worth being exact about what was wrong with it, because it was not what it looked like. The value was already parameterized: itemId was bound as $1, travelled through the driver's separate parameter channel, and never entered the query text. What was interpolated was a module constant containing no caller data. S2077 fires on the template literal rather than on the value, because the rule cannot tell a constant from a request field — and neither, at a glance, can a person reading it.

So the fix is not to parameterize something already parameterized. It is to stop interpolating at query call sites at all, which turns a property somebody has to verify into one they can see. Every query whose shape is fixed is now a named constant and every such call passes an identifier: ADMIN_ITEM_BY_ID for the two admin routes, PUBLIC_ITEM_BY_ID, LINK_LIST, and the two draft-queue shapes. Seven interpolating call sites become three.

The three that remain cannot become constants and now say so rather than looking like ones nobody got to. admin.ts and items.ts build their WHERE at run time from buildItemFilterSql, whose fragments are string literals whose only interpolations are placeholder indices; that reasoning was already written down and is unchanged. draftingWorker interpolates a table name, and this is the one query here that genuinely cannot be parameterized in any form — a bound parameter is a value, and Postgres will not accept an identifier as one, so the choice is interpolation or nothing. What makes it safe is the closed 'categories' | 'tags' union, and the comment now says that instead of merely asserting there is nothing to worry about.

Also clears the project's only open Sonar issue, S1854 on adminUploadLinks, which #260 introduced and which I had deferred as a tidiness point. It was more than that: outcome was initialised at its declaration and assigned the same value again in the catch, which made two different failures look like one. A template that will not render, or a stored template that cannot be loaded, is not an SMTP problem, and reporting it as "not configured" sent the admin looking in the wrong place. The SMTP-rejection conflation that was actually agreed stays, and is now the only thing that catch conflates.

Closes #294

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

backend/src/intake/draftingWorker.ts

@@ -72,8 +72,14 @@ async function readPhotos(itemId: number): Promise<Photo[]> {
 }
 
 async function namesOf(table: 'categories' | 'tags'): Promise<string[]> {
-  // The table name is a closed union, never caller input — there is nothing
+  // The one query in this codebase that cannot be parameterized, rather than one
-  // here to interpolate from a request.
+  // that merely has not been. A bound parameter is a *value*: Postgres will not
+  // accept `SELECT name FROM $1`, because an identifier has to be part of the
+  // parsed statement. So the choice is interpolation or nothing.
+  //
+  // What makes it safe is the type. `table` is the closed union
+  // 'categories' | 'tags', so the only two strings that can reach this line are
+  // both written above it, and neither is derived from a request. See #294.
   const { rows } = await pool.query<{ name: string }>(`SELECT name FROM ${table} ORDER BY name`);
   return rows.map((row) => row.name);
 }

backend/src/itemSelect.ts

@@ -58,6 +58,19 @@ export const ADMIN_ITEM_SELECT = `
     ${TAGS_SUBQUERY}
   ${FROM_CLAUSE}`;
 
+/**
+ * One admin item, by id — the whole query, not a fragment.
+ *
+ * A named constant rather than `${ADMIN_ITEM_SELECT} WHERE i.id = $1` written
+ * at each call, so no query call site interpolates anything at all. The id was
+ * always bound as $1 and never reached the query text, but S2077 fires on the
+ * template literal rather than on the value, because the rule cannot tell a
+ * module constant from a request field — and neither, at a glance, can a
+ * reader. Hoisting it makes the property structural instead of an assertion
+ * somebody has to re-make every time the line moves. See #294.
+ */
+export const ADMIN_ITEM_BY_ID = `${ADMIN_ITEM_SELECT} WHERE i.id = $1`;
+
 /** The columns every item select returns, whichever of the two it is. */
 interface ItemRowBase {
   id: number;

backend/src/routes/admin.ts

@@ -1,7 +1,7 @@
 import { Router, Request, Response } from 'express';
 import { PoolClient } from 'pg';
 import { pool, requireRow } from '../db';
-import { ADMIN_ITEM_SELECT, AdminItemRow, ItemRecord } from '../itemSelect';
+import { ADMIN_ITEM_SELECT, ADMIN_ITEM_BY_ID, AdminItemRow, ItemRecord } from '../itemSelect';
 import { ItemStatus } from '../types';
 import { asyncRoute } from '../asyncRoute';
 import { parseItemFilters, buildItemFilterSql, FilterError } from '../itemFilters';
@@ -168,12 +168,11 @@ router.post('/items', uploadImages, asyncRoute(async (req: Request, res: Respons
       await setItemTags(client, item.id, await resolveTagIds(client, tagNames));
     }
     await client.query('COMMIT');
-    // S2077 again, and here the template is a module constant plus a literal:
+    // No interpolation here at all now: the whole query is a constant and the id
-    // ADMIN_ITEM_SELECT interpolates nothing of its own, and the id is bound as
+    // is bound as $1. It always was bound — what changed is that a reader no
-    // $1 rather than formatted in. Same shape as the update route below, where
+    // longer has to check that the interpolated half carries no caller data,
-    // the bound value is caller-supplied — which is precisely why it is a
+    // because there is no interpolated half. See #294.
-    // parameter.
+    const { rows: full } = await pool.query<AdminItemRow>(ADMIN_ITEM_BY_ID, [item.id]);
-    const { rows: full } = await pool.query<AdminItemRow>(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [item.id]);
     res.json(requireRow(full, 'the item just inserted'));
   } catch (err) {
     await client.query('ROLLBACK');
@@ -224,10 +223,10 @@ router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Resp
       await insertItemImages(client, itemId, files, nextSort);
     }
     await client.query('COMMIT');
-    // S2077, the same constant-plus-$1 shape as the create route above.
+    // The same constant as the create route above. itemId is caller-controlled
-    // req.params.id is caller-controlled and goes through the driver as a bound
+    // and goes through the driver as a bound parameter; it never reaches the
-    // parameter; it never reaches the query text.
+    // query text.
-    const { rows: full } = await pool.query<AdminItemRow>(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [itemId]);
+    const { rows: full } = await pool.query<AdminItemRow>(ADMIN_ITEM_BY_ID, [itemId]);
     // The create route beside this one has always used requireRow here. This
     // one did not, so an UPDATE matching nothing committed happily, the SELECT
     // returned nothing, and the caller got 200 with an empty body — a success

backend/src/routes/adminItemDrafts.ts

@@ -46,6 +46,16 @@ const DRAFT_SELECT = `
   LEFT JOIN upload_links l ON l.id = d.upload_link_id
 `;
 
+/**
+ * The two shapes the queue is ever asked for, as whole queries.
+ *
+ * Named rather than assembled at the call, so neither branch of the ternary
+ * interpolates anything — the state is bound as $1 in the first and the second
+ * carries no caller data at all. See #294.
+ */
+const DRAFTS_BY_STATE = `${DRAFT_SELECT} WHERE d.state = $1 ORDER BY d.created_at DESC`;
+const DRAFTS_NOT_DISCARDED = `${DRAFT_SELECT} WHERE d.state <> 'discarded' ORDER BY d.created_at DESC`;
+
 /**
  * Discarded rows are excluded by default rather than deleted.
  *
@@ -59,8 +69,8 @@ router.get(
     const state = typeof req.query.state === 'string' ? req.query.state : null;
 
     const { rows } = state
-      ? await pool.query(`${DRAFT_SELECT} WHERE d.state = $1 ORDER BY d.created_at DESC`, [state])
+      ? await pool.query(DRAFTS_BY_STATE, [state])
-      : await pool.query(`${DRAFT_SELECT} WHERE d.state <> 'discarded' ORDER BY d.created_at DESC`);
+      : await pool.query(DRAFTS_NOT_DISCARDED);
 
     // Whether the control has anything behind it, alongside the rows. A second
     // endpoint for one boolean would be a round trip the queue already makes.

backend/src/routes/adminUploadLinks.ts

@@ -34,6 +34,9 @@ const LINK_SELECT = `
   FROM upload_links
 `;
 
+/** Every link, newest first. A whole query, so the call interpolates nothing (#294). */
+const LINK_LIST = `${LINK_SELECT} ORDER BY created_at DESC`;
+
 /**
  * The cap a link gets when nobody chose one.
  *
@@ -69,7 +72,7 @@ function submissionsAllowed(maxSubmissions: number | null): string {
 }
 
 router.get('/', asyncRoute(async (_req: Request, res: Response) => {
-  const { rows } = await pool.query<UploadLinkRow>(`${LINK_SELECT} ORDER BY created_at DESC`);
+  const { rows } = await pool.query<UploadLinkRow>(LINK_LIST);
   res.json(rows);
 }));
 
@@ -123,7 +126,7 @@ router.post('/', asyncRoute(async (req: Request, res: Response) => {
   // A failure does not roll the link back. The token is shown exactly once, so
   // a rollback would leave the admin retrying and holding a different link,
   // discarding work that succeeded for the sake of tidiness.
-  let outcome: MailOutcome = 'skipped-unconfigured';
+  let outcome: MailOutcome;
   try {
     const template = renderTemplate('uploadLink', await loadStoredTemplate('uploadLink'), {
       submitUrl: url,
@@ -135,11 +138,17 @@ router.post('/', asyncRoute(async (req: Request, res: Response) => {
     // Reported, not thrown. The link exists and is usable; the admin needs to
     // be told the mail did not go, not handed a 500 for a link that was made.
     //
-    // An SMTP rejection also lands here and is reported the same way as an
+    // Assigned only here, not also at the declaration. The duplicate initialiser
-    // unconfigured environment, which is not strictly accurate — a fourth
+    // was flagged as S1854 (#294), and it was worse than redundant: it made the
-    // outcome would be a real distinction, but nothing consumes it and the
+    // two ways of reaching this line look like one. A template that will not
-    // admin's next action is identical either way: copy the link and send it
+    // render, or a stored template that cannot be loaded, is not an SMTP
-    // by hand.
+    // problem, and reporting it as "not configured" pointed the admin at the
+    // wrong thing entirely.
+    //
+    // An SMTP rejection landing here and being reported as unconfigured is the
+    // conflation that was actually agreed: a fourth outcome would be a real
+    // distinction, nothing consumes it, and the admin's next action is identical
+    // either way — copy the link and send it by hand.
     console.error(`[upload-links] could not email ${email}:`, err);
     outcome = 'skipped-unconfigured';
   }

backend/src/routes/items.ts

@@ -17,6 +17,15 @@ import {
 // optional filter. It has to be a clause the caller cannot opt out of.
 const EXCLUDE_PENDING = `i.status <> 'pending'`;
 
+/**
+ * One public item, by id — the whole query rather than a fragment.
+ *
+ * Built once here so the call site interpolates nothing. Both halves were
+ * always constants and the id was always bound as $1, but a template literal at
+ * a query call is a thing a reader has to verify rather than see. See #294.
+ */
+const PUBLIC_ITEM_BY_ID = `${PUBLIC_ITEM_SELECT} WHERE i.id = $1 AND ${EXCLUDE_PENDING}`;
+
 const router = Router();
 
 router.get('/', asyncRoute(async (req: Request, res: Response) => {
@@ -93,10 +102,7 @@ router.get('/:id', asyncRoute(async (req: Request, res: Response) => {
   // Excluded here too, not only from the list. A pending item that stayed
   // fetchable by id would be hidden from the catalogue and still reachable by
   // anyone who guessed or kept a link.
-  const { rows } = await pool.query<PublicItemRow>(
+  const { rows } = await pool.query<PublicItemRow>(PUBLIC_ITEM_BY_ID, [req.params.id]);
-    `${PUBLIC_ITEM_SELECT} WHERE i.id = $1 AND ${EXCLUDE_PENDING}`,
-    [req.params.id]
-  );
   if (!rows.length) return res.status(404).json({ error: 'not found' });
   res.json(rows[0]);
 }));