diff --git a/backend/src/intake/draftingWorker.ts b/backend/src/intake/draftingWorker.ts index 670dcec..cee2b6e 100644 --- a/backend/src/intake/draftingWorker.ts +++ b/backend/src/intake/draftingWorker.ts @@ -72,8 +72,14 @@ async function readPhotos(itemId: number): Promise { } async function namesOf(table: 'categories' | 'tags'): Promise { - // 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); } diff --git a/backend/src/itemSelect.ts b/backend/src/itemSelect.ts index bea2280..28afc76 100644 --- a/backend/src/itemSelect.ts +++ b/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; diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index b7b37dc..e522607 100755 --- a/backend/src/routes/admin.ts +++ b/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(`${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(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(`${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(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 diff --git a/backend/src/routes/adminItemDrafts.ts b/backend/src/routes/adminItemDrafts.ts index 04554c8..3fb15c3 100644 --- a/backend/src/routes/adminItemDrafts.ts +++ b/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. diff --git a/backend/src/routes/adminUploadLinks.ts b/backend/src/routes/adminUploadLinks.ts index 72ef918..bf129b3 100644 --- a/backend/src/routes/adminUploadLinks.ts +++ b/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(`${LINK_SELECT} ORDER BY created_at DESC`); + const { rows } = await pool.query(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'; } diff --git a/backend/src/routes/items.ts b/backend/src/routes/items.ts index ee54a29..c0e99a8 100755 --- a/backend/src/routes/items.ts +++ b/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( - `${PUBLIC_ITEM_SELECT} WHERE i.id = $1 AND ${EXCLUDE_PENDING}`, - [req.params.id] - ); + const { rows } = await pool.query(PUBLIC_ITEM_BY_ID, [req.params.id]); if (!rows.length) return res.status(404).json({ error: 'not found' }); res.json(rows[0]); }));