Files
redefined-designs/backend/src/routes/items.ts
T
synAdminandClaude Opus 5 8956b9f122
Linting / lint (pull_request) Successful in 3m18s
SonarQube Analysis / sonarqube (pull_request) Failing after 24m32s
fix(items): read the id strictly, and stop the error test needing a route that does not (#307)
Item 5. GET /api/items/:id was the last route reading its id with a bare Number(), so an unreadable id reached Postgres and came back to the caller as a 500 for an item that cannot exist. It answers 404 now, like every other id-taking route since #207.

It could not be fixed on its own, which is why it stayed. errorHandling.integration.test.ts used this route's looseness as its way of making a handler reject: tightening the parse would have left that test green while removing the thing it tests. So the test now fails a database call directly, with a spy on pool.query against a route that makes one. That is the failure the error middleware actually exists for, and it does not depend on any route declining to validate — the previous comment's own conclusion, that moving the trigger to cart.ts would only move the wart.

Two things fixed along the way that the issue asked about but that switching to readId would not have delivered on its own.

readId was not as strict as its name suggests. Number reads 5.0, 1e2, 0x10 and +5 as 5, 100, 16 and 5 — every one a positive integer, so every check readId made passed and the route fetched a real row for a URL nobody wrote. /items/5.0 answered with item 5. This never raised an error and so never announced itself; the issue noticed it only because #308 converted the comparison to a real integer. An id is a string of digits, so it is matched against digits before being parsed.

readId is also now bounded at the top of a 32-bit serial. Above that Postgres raises 22003 rather than returning nothing, which is the same wrong answer to the caller as the 22P02 the function was written to prevent — a 500 for an id that identifies nothing.

The leak assertion was passing for the wrong reason. It checks the response does not contain "syntax" or "items", and the error it was checking against happened to contain both only by accident of which route was used. The injected failure now contains both words deliberately, and the whole message is asserted against as well, so a future error format cannot slip through by wording itself differently.

Verified: tsc clean, lint 0 errors with no new warnings, 485 unit tests passing across 33 suites — seven of them new, covering the inputs above. The integration suite cannot run on this machine, so whether the rewritten error test passes is for CI to say.

Item 4, coverage, is not closed by this and cannot be closed yet: the SonarQube scan step has been skipped on every recent run because it is gated on the earlier steps succeeding, and those steps were failing. The dashboard is therefore stale. Reported on the issue rather than guessed at.

Refs #307

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-08 15:59:12 -05:00

118 lines
4.8 KiB
TypeScript
Executable File

import { Router, Request, Response } from 'express';
import { asyncRoute } from '../asyncRoute';
import { publicItemQuery, PublicItemRow, ItemContext } from '../itemSelect';
import { readId } from '../utils';
import {
parseItemFilters,
itemFilterExpressions,
FilterError,
NON_PUBLIC_STATUSES,
STOREFRONT_DEFAULT_STATUSES,
STOREFRONT_ALL_STATUSES
} from '../itemFilters';
/**
* Pending items are excluded everywhere, 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.
*
* An expression rather than the SQL literal this was until #308, so it composes
* with the filter clauses through `eb.and` instead of being joined into a
* string. That join used to need its own argument about why AND could not
* weaken it; `and` cannot re-associate anything.
*/
function notPending(eb: ItemContext) {
return eb('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 rows: PublicItemRow[] = await publicItemQuery()
.where((eb) =>
eb.and([
notPending(eb),
...itemFilterExpressions(eb, effectiveFilters, req.customerId ?? null)
])
)
.orderBy('i.created_at', 'desc')
.execute();
res.json(rows);
}));
router.get('/:id', asyncRoute(async (req: Request, res: Response) => {
// readId, like every other id-taking route since #207. This was the last one
// reading its id with a bare Number(), which meant an unreadable id reached
// Postgres and came back to the caller as a 500 for an item that cannot
// exist. 404 is what "/items/abc" actually means.
//
// It was left on Number() because errorHandling.integration.test.ts used this
// route's looseness as its way of making a handler reject. That test now
// fails a database call directly instead, so it no longer depends on a route
// declining to validate — which is what allowed this to be fixed (#307).
const id = readId(req.params.id);
if (id === null) return res.status(404).json({ error: 'not found' });
const rows = await publicItemQuery()
.where('i.id', '=', id)
.where((eb) => notPending(eb))
.execute();
if (!rows.length) return res.status(404).json({ error: 'not found' });
res.json(rows[0]);
}));
export default router;