Merge pull request 'chore(sonarqube): keep interpolation out of query call sites (#294)' (#295) from chore/294-no-interpolation-at-query-sites into main

Reviewed-on: #295

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
-  // here to interpolate from a request.
+  // The one query in this codebase that cannot be parameterized, rather than one
+  // 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:
-    // ADMIN_ITEM_SELECT interpolates nothing of its own, and the id is bound as
-    // $1 rather than formatted in. Same shape as the update route below, where
-    // the bound value is caller-supplied — which is precisely why it is a
-    // parameter.
-    const { rows: full } = await pool.query<AdminItemRow>(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [item.id]);
+    // No interpolation here at all now: the whole query is a constant and the id
+    // is bound as $1. It always was bound — what changed is that a reader no
+    // longer has to check that the interpolated half carries no caller data,
+    // because there is no interpolated half. See #294.
+    const { rows: full } = await pool.query<AdminItemRow>(ADMIN_ITEM_BY_ID, [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.
-    // req.params.id is caller-controlled and goes through the driver as a bound
-    // parameter; it never reaches the query text.
-    const { rows: full } = await pool.query<AdminItemRow>(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [itemId]);
+    // The same constant as the create route above. itemId is caller-controlled
+    // and goes through the driver as a bound parameter; it never reaches the
+    // query text.
+    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(`${DRAFT_SELECT} WHERE d.state <> 'discarded' ORDER BY d.created_at DESC`);
+      ? await pool.query(DRAFTS_BY_STATE, [state])
+      : 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
-    // unconfigured environment, which is not strictly accurate — a fourth
-    // outcome would be a real distinction, but nothing consumes it and the
-    // admin's next action is identical either way: copy the link and send it
-    // by hand.
+    // Assigned only here, not also at the declaration. The duplicate initialiser
+    // was flagged as S1854 (#294), and it was worse than redundant: it made the
+    // two ways of reaching this line 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" 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>(
-    `${PUBLIC_ITEM_SELECT} WHERE i.id = $1 AND ${EXCLUDE_PENDING}`,
-    [req.params.id]
-  );
+  const { rows } = await pool.query<PublicItemRow>(PUBLIC_ITEM_BY_ID, [req.params.id]);
   if (!rows.length) return res.status(404).json({ error: 'not found' });
   res.json(rows[0]);
 }));