docs(security): record why the three SQL hotspots in admin.ts are safe (#180) #213
@@ -340,6 +340,18 @@ router.get('/items', asyncRoute(async (req: Request, res: Response) => {
|
|||||||
return res.status(400).json({ error: 'favorites is not a valid inventory filter' });
|
return res.status(400).json({ error: 'favorites is not a valid inventory filter' });
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// S2077 flags every query below that assembles its SQL as a template literal,
|
||||||
|
// and this is the one where that is more than a formality: `where` really is
|
||||||
|
// built at run time. What makes it safe is that buildItemFilterSql composes
|
||||||
|
// only string literals written in itemFilters.ts. The single interpolation
|
||||||
|
// inside any of them is `$${next}` — a placeholder index, a number, seeded
|
||||||
|
// from the startIndex argument and incremented locally. It is never derived
|
||||||
|
// from a filter value.
|
||||||
|
//
|
||||||
|
// So a caller chooses which of six fixed fragments are joined, and supplies
|
||||||
|
// every value in `params`, and neither of those becomes SQL. parseItemFilters
|
||||||
|
// rejects malformed input above, but that is defence in depth rather than the
|
||||||
|
// reason this holds — the clause literals would be safe without it.
|
||||||
const { clauses, params } = buildItemFilterSql(filters, 1, null);
|
const { clauses, params } = buildItemFilterSql(filters, 1, null);
|
||||||
const where = clauses.length ? `WHERE ${clauses.join(' AND ')}` : '';
|
const where = clauses.length ? `WHERE ${clauses.join(' AND ')}` : '';
|
||||||
const { rows } = await pool.query<AdminItemRow>(`${ADMIN_ITEM_SELECT} ${where} ORDER BY i.created_at DESC`, params);
|
const { rows } = await pool.query<AdminItemRow>(`${ADMIN_ITEM_SELECT} ${where} ORDER BY i.created_at DESC`, params);
|
||||||
@@ -367,6 +379,11 @@ router.post('/items', uploadImages, asyncRoute(async (req: Request, res: Respons
|
|||||||
await setItemTags(client, item.id, await resolveTagIds(client, tagNames));
|
await setItemTags(client, item.id, await resolveTagIds(client, tagNames));
|
||||||
}
|
}
|
||||||
await client.query('COMMIT');
|
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]);
|
const { rows: full } = await pool.query<AdminItemRow>(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [item.id]);
|
||||||
res.json(requireRow(full, 'the item just inserted'));
|
res.json(requireRow(full, 'the item just inserted'));
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
@@ -415,6 +432,9 @@ router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Resp
|
|||||||
await insertItemImages(client, Number(req.params.id), files, nextSort);
|
await insertItemImages(client, Number(req.params.id), files, nextSort);
|
||||||
}
|
}
|
||||||
await client.query('COMMIT');
|
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`, [req.params.id]);
|
const { rows: full } = await pool.query<AdminItemRow>(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [req.params.id]);
|
||||||
res.json(full[0]);
|
res.json(full[0]);
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
|
|||||||
Reference in New Issue
Block a user