Files
redefined-designs/backend/src/routes/items.ts
T
bermudalambandClaude Opus 5 c704c07b89 docs(security): put the SQL injection invariant where it is enforced (#202)
#180 cleared the three `typescript:S2077` hotspots and marked them Reviewed/Safe on the dashboard, but the repository half never reached main — the branch carrying it was deleted before merge, so the markers are cleared, the issue is closed, and nothing in the code said why. That is the exact state #180 set out to avoid: "the justification has to live in the repository, not only in SonarQube's UI".

The three call-site comments are restored, with two corrections a review of the original found.

They were in the wrong file. `buildItemFilterSql` is where the rule actually lives: both callers splice its clauses straight into query text, so only a placeholder index may ever be interpolated into one and every value must go onto `params`. That function's header said nothing about it, and it is where a seventh clause would be added.

This matters more than ordinary comment placement because of how a cleared hotspot behaves. Reviewed/Safe stays marked and does not re-raise when a *different* file changes, so the one edit that would break this — interpolating a filter value in `itemFilters.ts` — was the one edit that would have got neither a warning nor a fresh marker.

`items.ts` gets the same note. It builds `${PUBLIC_ITEM_SELECT} WHERE ${where}` from the identical construct and is reachable without signing in, but SonarQube never flagged it, so the higher-exposure copy was the undocumented one. It also records why joining with AND cannot weaken `EXCLUDE_PENDING`: no fragment carries a top-level OR for the join to re-associate against.

The wording was slightly false. "The single interpolation is `$${next}`" — the tags clause also interpolates `$${next + 1}`. Same category, so the argument is untouched, but a reader checking it literally finds a counter-example immediately, and a comment asserting safety cannot afford that.

Two tests make the invariant fail a build rather than depend on being read. One feeds values built by hand rather than parsed — `"1); DROP TABLE items; --"` in every field — and asserts none of it reaches the clause text, which states directly that these literals are safe with no parser at all. The other asserts two disjoint filter sets produce byte-identical SQL, which catches a value that happens not to look hostile.

Both were mutation-tested rather than assumed: interpolating `filters.minPriceCents` into the price clause — the precise edit the comment forbids — fails both, and one pre-existing test besides. Reverted, and the diff against main for `itemFilters.ts` is comment-only.

Verified: backend build clean, 280 unit tests pass.

Closes #202
Refs #180

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-28 14:04:21 -05:00

105 lines
4.6 KiB
TypeScript
Executable File

import { Router, Request, Response } from 'express';
import { pool } from '../db';
import { asyncRoute } from '../asyncRoute';
import { PUBLIC_ITEM_SELECT, PublicItemRow } from '../itemSelect';
import {
parseItemFilters,
buildItemFilterSql,
FilterError,
NON_PUBLIC_STATUSES,
STOREFRONT_DEFAULT_STATUSES,
STOREFRONT_ALL_STATUSES
} from '../itemFilters';
// Applied to every public read, unconditionally. This route has never had a
// status filter of its own — sold items are listed and rendered with a Sold
// badge on purpose — so hiding pending items cannot be expressed as one more
// optional filter. It has to be a clause the caller cannot opt out of.
const EXCLUDE_PENDING = `i.status <> 'pending'`;
const router = Router();
router.get('/', asyncRoute(async (req: Request, res: Response) => {
let filters;
try {
filters = parseItemFilters(req.query as Record<string, unknown>);
} catch (err) {
// A malformed filter is returned as an error rather than ignored, so a
// broken link shows itself instead of quietly listing the whole catalogue.
if (err instanceof FilterError) {
return res.status(400).json({ error: err.message });
}
throw err;
}
// 401 rather than an empty list: a signed-out visitor asking for "my
// favorites" has no favorites to be empty of, and answering with [] would
// render as "no items match these filters" — a plausible-looking lie. The
// storefront prompts for sign-in instead of sending this, so reaching here
// means a bookmarked link outlived its session.
if (filters.favoritesOnly && !req.customerId) {
return res.status(401).json({ error: 'sign in to filter by favorites' });
}
// Refused rather than quietly answered. The filter parser is shared with the
// admin routes, where 'pending' is valid, so it parses here too — and with
// the exclusion below it would return an empty list, which reads as "no items
// match" rather than "you may not ask that".
//
// Checked across every requested status, not just a single one: `?status=
// available,pending` must be refused for naming pending at all, rather than
// quietly answered because the first name in the list happened to be allowed.
if (filters.status?.some((status) => NON_PUBLIC_STATUSES.includes(status))) {
return res.status(400).json({ error: 'invalid status' });
}
// No preference means Not Sold rather than everything. Applied here rather
// than in the parser, which is shared with the admin, where the same absence
// has to go on meaning "every status including pending".
//
// Except when the customer asked for their own favorites, where the default
// stays everything. A favorite that has just sold is often exactly what the
// customer came to look at — they were emailed to say so — and hiding it
// would make an item they curated vanish without explanation. That was a
// deliberate decision before this filter existed, and defaulting favorites to
// Not Sold would have quietly reversed it. An explicit ?status= still wins,
// so the choice remains theirs.
const defaultStatuses = filters.favoritesOnly
? STOREFRONT_ALL_STATUSES
: STOREFRONT_DEFAULT_STATUSES;
const effectiveFilters = {
...filters,
status: filters.status ?? [...defaultStatuses]
};
const { clauses, params } = buildItemFilterSql(effectiveFilters, 1, req.customerId ?? null);
// The same construct SonarQube flagged as S2077 in admin.ts and which is
// marked Reviewed/Safe there (#180) — and this is the copy reachable without
// signing in, so it is worth saying here too rather than relying on the
// reader having seen the other one. It holds for the same reason: the clauses
// are literals from buildItemFilterSql carrying only placeholder indices, and
// EXCLUDE_PENDING is a module constant. Joining with AND cannot weaken
// EXCLUDE_PENDING either, because no fragment contains a top-level OR for the
// join to re-associate against.
const where = [EXCLUDE_PENDING, ...clauses].join(' AND ');
const { rows } = await pool.query<PublicItemRow>(
`${PUBLIC_ITEM_SELECT} WHERE ${where} ORDER BY i.created_at DESC`,
params
);
res.json(rows);
}));
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]
);
if (!rows.length) return res.status(404).json({ error: 'not found' });
res.json(rows[0]);
}));
export default router;