diff --git a/backend/src/itemFilters.ts b/backend/src/itemFilters.ts index ab43eee..2a51f4f 100644 --- a/backend/src/itemFilters.ts +++ b/backend/src/itemFilters.ts @@ -2,7 +2,9 @@ // filters. Kept apart from the route so the rules can be unit-tested without a // database, and so items.ts stays a thin handler. +import { Expression, SqlBool, sql } from 'kysely'; import { ItemStatus } from './types'; +import { ItemContext } from './itemSelect'; export type { ItemStatus }; export class FilterError extends Error {} @@ -59,11 +61,6 @@ export const STOREFRONT_DEFAULT_STATUSES: readonly ItemStatus[] = ['available', // shape this codebase keeps designing against. export const STOREFRONT_ALL_STATUSES: readonly ItemStatus[] = ['available', 'reserved', 'sold']; -export interface BuiltFilter { - clauses: string[]; - params: unknown[]; -} - // Deliberately excludes a leading sign and any decimal point: every filter // value is a non-negative integer (an id, or a price in cents), so '-1' and // '10.5' are caller mistakes worth surfacing rather than silently coercing. @@ -158,7 +155,7 @@ function parseTagIds(value: unknown): number[] { continue; } const id = parseId(trimmed, 'tags'); - // Duplicates would inflate the required-match count in buildItemFilterSql + // Duplicates would inflate the required-match count in itemFilterExpressions // and make the filter match nothing at all. if (!tagIds.includes(id)) { tagIds.push(id); @@ -236,24 +233,21 @@ export function parseItemFilters(query: Record): ItemFilters { return { categoryIds, tagIds, minPriceCents, maxPriceCents, status, favoritesOnly }; } -// Returns WHERE fragments plus their parameters, with placeholders numbered -// from `startIndex` so the caller can splice these in after its own params. +// Composes the filter clauses as Kysely expressions. // -// SECURITY INVARIANT, and it is load-bearing. Both callers splice these clauses -// straight into query text — admin.ts as `${ADMIN_ITEM_SELECT} ${where}`, and -// items.ts as `${PUBLIC_ITEM_SELECT} WHERE ${where}`, which is reachable -// without signing in. So the only thing that may ever be interpolated into a -// string pushed onto `clauses` is a placeholder index: `$${next}`, or -// `$${next + 1}` in the tags clause. Every value goes onto `params` and is -// bound by the driver. Interpolating a filter value here would be SQL injection -// at both call sites, and `parseItemFilters` refusing malformed input is not -// what prevents it — these literals would be safe with no parser at all. +// This returned `{ clauses: string[]; params: unknown[] }` until #308, and both +// callers spliced the clauses straight into query text. The invariant that made +// that safe — only a placeholder index may ever be interpolated into a clause, +// never a value — was a sixteen-line comment and two tests standing between an +// edit and a live injection on a route reachable without signing in. // -// Stated here rather than only at the call sites because this is where the rule -// is enforced and where a seventh clause would be added. SonarQube raised S2077 -// on the call sites and they are marked Reviewed/Safe (#180); that marking does -// not re-raise when this file changes, so this comment and the two tests over -// it are what stand between that edit and a live injection. See #202. +// It is now a property of the type system. `${value}` inside a Kysely `sql` +// template emits a bind parameter, never text, and the builder expressions +// cannot express interpolation at all. The two tests at the bottom of +// itemFilters.test.ts still exist and now assert against the SQL Kysely +// actually emits, which is a stronger claim than the one they used to make. +// +// `startIndex` is gone with the splicing it existed for. // // `favoritesCustomerId` is required rather than optional so a caller has to say // whose favorites it means, even when it means nobody's. Both routes already @@ -261,17 +255,14 @@ export function parseItemFilters(query: Record): ItemFilters { // a programming error — but it is here so that a future caller which forgets // the guard fails loudly instead of quietly ignoring the filter and listing the // whole catalogue. -export function buildItemFilterSql( +export function itemFilterExpressions( + eb: ItemContext, filters: ItemFilters, - startIndex: number, favoritesCustomerId: number | null -): BuiltFilter { - const clauses: string[] = []; - const params: unknown[] = []; - let next = startIndex; +): Expression[] { + const clauses: Expression[] = []; if (filters.categoryIds.length) { - params.push(filters.categoryIds); // Selecting a category means "and everything filed beneath it", so walk the // tree down from each chosen node. A recursive CTE keeps the tree // un-denormalized: reparenting stays a single UPDATE with no stored paths @@ -281,61 +272,61 @@ export function buildItemFilterSql( // walked in the same recursion. That also gives the OR for free: the union // of the subtrees is exactly "filed under any of these", and an item filed // under two selected branches appears once because IN is a set test. - clauses.push(`i.category_id IN ( + // + // Still a `sql` template, because the builder expresses a recursive CTE no + // better than this does. `${filters.categoryIds}` is one bind parameter + // holding the whole array — not a placeholder list — which is why no + // sql.param() ceremony appears here. See src/db-kysely/CONVENTIONS.md. + clauses.push(sql`i.category_id IN ( WITH RECURSIVE subtree AS ( - SELECT id FROM categories WHERE id = ANY($${next}::int[]) + SELECT id FROM categories WHERE id = ANY(${filters.categoryIds}::int[]) UNION ALL SELECT c.id FROM categories c JOIN subtree s ON c.parent_id = s.id ) SELECT id FROM subtree )`); - next++; } if (filters.tagIds.length) { - params.push(filters.tagIds, filters.tagIds.length); // AND, not OR: the item must carry every selected tag. Matching with // `tag_id = ANY(...)` alone would return items holding just one of them, so // the count of matched rows has to equal the number requested. - clauses.push( - `(SELECT COUNT(*) FROM item_tags it - WHERE it.item_id = i.id AND it.tag_id = ANY($${next}::int[])) = $${next + 1}` - ); - next += 2; + clauses.push(sql`(SELECT COUNT(*) FROM item_tags it + WHERE it.item_id = i.id AND it.tag_id = ANY(${filters.tagIds}::int[])) = ${filters.tagIds.length}`); } if (filters.minPriceCents !== null) { - params.push(filters.minPriceCents); - clauses.push(`i.price_cents >= $${next}`); - next++; + clauses.push(eb('i.price_cents', '>=', filters.minPriceCents)); } if (filters.maxPriceCents !== null) { - params.push(filters.maxPriceCents); - clauses.push(`i.price_cents <= $${next}`); - next++; + clauses.push(eb('i.price_cents', '<=', filters.maxPriceCents)); } if (filters.status !== null) { - params.push(filters.status); - // ANY rather than equality, so one status and several use the same clause. - // The ::text[] cast is explicit because `status` is a text column and the - // driver would otherwise have to infer the array's element type. - clauses.push(`i.status = ANY($${next}::text[])`); - next++; + // `in` replaces the `= ANY($n::text[])` this used to build. Kysely emits + // the placeholder list itself, so one status and several use the same + // expression and the explicit ::text[] cast is no longer needed. + clauses.push(eb('i.status', 'in', filters.status)); } if (filters.favoritesOnly) { if (favoritesCustomerId === null) { throw new Error('favorites filter requires a customer id'); } - params.push(favoritesCustomerId); // EXISTS rather than a join: an item is favorited by a customer at most // once, but joining would still risk multiplying rows if that ever changed, // and this reads as the membership test it is. - clauses.push(`EXISTS (SELECT 1 FROM favorites f WHERE f.item_id = i.id AND f.customer_id = $${next})`); - next++; + clauses.push( + eb.exists( + eb + .selectFrom('favorites as f') + .select('f.item_id') + .whereRef('f.item_id', '=', 'i.id') + .where('f.customer_id', '=', favoritesCustomerId) + ) + ); } - return { clauses, params }; + return clauses; } diff --git a/backend/src/itemSelect.ts b/backend/src/itemSelect.ts index 4ef8a7a..ab5d487 100644 --- a/backend/src/itemSelect.ts +++ b/backend/src/itemSelect.ts @@ -1,95 +1,124 @@ -// Shared item SELECT shapes for the public and admin routes, and the row types +// Shared item query shapes for the public and admin routes, and the row types // they return. // // The types live here rather than in types.ts because they describe a -// projection, not a table. ADMIN_ITEM_SELECT takes `i.*` and PUBLIC_ITEM_SELECT -// names its columns so the storefront never sees paypal_order_id or -// reserved_until — typing both as "an items row" would quietly re-admit exactly -// the columns that select was written to exclude. +// projection, not a table. adminItemQuery takes every column of items and +// publicItemQuery names its columns, so the storefront never sees +// paypal_order_id or reserved_until — typing both as "an items row" would +// quietly re-admit exactly the columns that projection was written to exclude. // -// KEPT IN STEP BY HAND. `pool.query` asserts a shape; it does not check the -// SQL, which TypeScript never reads. Dropping a column from a select below -// without dropping it from its type compiles cleanly and every read of it goes -// on type-checking while being undefined at runtime. The integration suite is -// the only thing that catches that, because it runs these queries against a -// real schema. Change a select and its type together. +// These were SQL string constants until #308. They had to be kept in step with +// their row types by hand, because `pool.query` asserts a shape and never +// checks it against the SQL, so dropping a column from a select without +// dropping it from its type compiled cleanly and went undefined at run time — +// and only the integration suite ever caught it. Built through Kysely, that is +// a compile error, because the row type now follows from the projection. // -// Images and tags are pulled as scalar subqueries rather than LEFT JOIN + +// Images and tags are pulled as aggregate subqueries rather than LEFT JOIN + // GROUP BY. Joining two one-to-many relations in the same query multiplies // their rows together — an item with 2 images and 3 tags would aggregate 6 // rows, silently repeating every image three times. Subqueries keep each -// aggregate independent and drop the GROUP BY entirely. +// aggregate independent and drop the GROUP BY entirely. `jsonArrayFrom` emits +// `coalesce(json_agg(agg), '[]')`, which is what these hand-wrote before. +import { ExpressionBuilder } from 'kysely'; +import { jsonArrayFrom } from 'kysely/helpers/postgres'; +import { db } from './db'; +import { DB } from './db-kysely/schema'; import { ItemStatus, ItemImage, ItemTag } from './types'; -const IMAGES_SUBQUERY = ` - COALESCE(( - SELECT json_agg(json_build_object('id', img.id, 'image_path', img.image_path, 'sort_order', img.sort_order) - ORDER BY img.sort_order) - FROM item_images img - WHERE img.item_id = i.id - ), '[]') AS images`; +/** + * The aliases every item query and every filter clause is written against. + * + * `i` and `c` are kept from the SQL these replaced. Not because short names are + * better, but because the filter clauses, the subquery correlations and the + * ORDER BY all reference them, and renaming them in the same change that moved + * the builder would have made the diff unreadable against the SQL it replaces. + */ +export type ItemContext = ExpressionBuilder< + DB & { i: DB['items']; c: DB['categories'] }, + 'i' | 'c' +>; + +/** The public image fields. Correlated to the outer item by `whereRef`. */ +function imagesFor(eb: ItemContext) { + return jsonArrayFrom( + eb + .selectFrom('item_images as img') + .select(['img.id', 'img.image_path', 'img.sort_order']) + .whereRef('img.item_id', '=', 'i.id') + .orderBy('img.sort_order') + ).as('images'); +} /** * Admin-only images, carrying `original_image_path` alongside the public * fields — the field the inventory screen needs to know whether a photo has a * cut-out to restore (#293). * - * A separate subquery rather than adding the column to `IMAGES_SUBQUERY` - * itself, for the same reason `PUBLIC_ITEM_SELECT` names its columns instead - * of using `i.*`: an original filename is internal — nobody's business on the - * storefront — and folding it into the one subquery both selects share would - * put it in every public item response too. + * A separate function rather than a flag on `imagesFor`, for the same reason + * `publicItemQuery` names its columns instead of taking them all: an original + * filename is internal, nobody's business on the storefront, and a boolean in + * the middle of the thing that keeps it off the public API is one edit away + * from being passed wrongly. */ -const ADMIN_IMAGES_SUBQUERY = ` - COALESCE(( - SELECT json_agg(json_build_object('id', img.id, 'image_path', img.image_path, 'sort_order', img.sort_order, - 'original_image_path', img.original_image_path) - ORDER BY img.sort_order) - FROM item_images img - WHERE img.item_id = i.id - ), '[]') AS images`; +function adminImagesFor(eb: ItemContext) { + return jsonArrayFrom( + eb + .selectFrom('item_images as img') + .select(['img.id', 'img.image_path', 'img.sort_order', 'img.original_image_path']) + .whereRef('img.item_id', '=', 'i.id') + .orderBy('img.sort_order') + ).as('images'); +} -const TAGS_SUBQUERY = ` - COALESCE(( - SELECT json_agg(json_build_object('id', t.id, 'name', t.name, 'color', t.color) ORDER BY t.name) - FROM item_tags it - JOIN tags t ON t.id = it.tag_id - WHERE it.item_id = i.id - ), '[]') AS tags`; - -const FROM_CLAUSE = ` - FROM items i - LEFT JOIN categories c ON c.id = i.category_id`; - -// The storefront gets an explicit column list — it has no business seeing -// paypal_order_id or reserved_until. -export const PUBLIC_ITEM_SELECT = ` - SELECT i.id, i.name, i.description, i.price_cents, i.status, i.created_at, i.category_id, - c.name AS category_name, - ${IMAGES_SUBQUERY}, - ${TAGS_SUBQUERY} - ${FROM_CLAUSE}`; - -export const ADMIN_ITEM_SELECT = ` - SELECT i.*, - c.name AS category_name, - ${ADMIN_IMAGES_SUBQUERY}, - ${TAGS_SUBQUERY} - ${FROM_CLAUSE}`; +function tagsFor(eb: ItemContext) { + return jsonArrayFrom( + eb + .selectFrom('item_tags as it') + .innerJoin('tags as t', 't.id', 'it.tag_id') + .select(['t.id', 't.name', 't.color']) + .whereRef('it.item_id', '=', 'i.id') + .orderBy('t.name') + ).as('tags'); +} /** - * One admin item, by id — the whole query, not a fragment. + * The storefront's projection — an explicit column list, because it has no + * business seeing paypal_order_id or reserved_until. * - * 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. + * A function rather than a constant so each caller gets a fresh builder. Kysely + * builders are immutable, so sharing one would be safe, but a function makes it + * obvious that adding a `where` does not affect anyone else. */ -export const ADMIN_ITEM_BY_ID = `${ADMIN_ITEM_SELECT} WHERE i.id = $1`; +export function publicItemQuery() { + return db + .selectFrom('items as i') + .leftJoin('categories as c', 'c.id', 'i.category_id') + .select([ + 'i.id', + 'i.name', + 'i.description', + 'i.price_cents', + 'i.status', + 'i.created_at', + 'i.category_id', + 'c.name as category_name' + ]) + .select(imagesFor) + .select(tagsFor); +} + +/** The admin projection — every item column, plus the admin image fields. */ +export function adminItemQuery() { + return db + .selectFrom('items as i') + .leftJoin('categories as c', 'c.id', 'i.category_id') + .selectAll('i') + .select('c.name as category_name') + .select(adminImagesFor) + .select(tagsFor); +} /** The columns every item select returns, whichever of the two it is. */ interface ItemRowBase { diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index 06da459..7f416c0 100755 --- a/backend/src/routes/admin.ts +++ b/backend/src/routes/admin.ts @@ -1,10 +1,10 @@ import { Router, Request, Response } from 'express'; import { PoolClient } from 'pg'; import { pool, requireRow } from '../db'; -import { ADMIN_ITEM_SELECT, ADMIN_ITEM_BY_ID, AdminItemRow, ItemRecord } from '../itemSelect'; +import { adminItemQuery, AdminItemRow, ItemRecord } from '../itemSelect'; import { ItemStatus } from '../types'; import { asyncRoute } from '../asyncRoute'; -import { parseItemFilters, buildItemFilterSql, FilterError } from '../itemFilters'; +import { parseItemFilters, itemFilterExpressions, FilterError } from '../itemFilters'; import { readId, tagColorFor } from '../utils'; import { notifyFavoritersOfSale, notifyFavoritersOfRemoval, collectFavoriteRecipients } from '../favoriteAlerts'; import { removeBackgroundsForItem, restoreOriginalsForItem } from '../intake/backgroundRemoval'; @@ -132,21 +132,22 @@ router.get('/items', asyncRoute(async (req: Request, res: Response) => { 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 only interpolations - // inside any of them are placeholder indices — `$${next}`, and `$${next + 1}` - // in the tags clause — numbers, seeded from the startIndex argument and - // incremented locally. Neither is ever 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 where = clauses.length ? `WHERE ${clauses.join(' AND ')}` : ''; - const { rows } = await pool.query(`${ADMIN_ITEM_SELECT} ${where} ORDER BY i.created_at DESC`, params); + // No interpolation, and nothing to argue about. Until #308 this assembled + // `${ADMIN_ITEM_SELECT} ${where}` from clauses composed at run time, and + // sixteen lines in itemFilters.ts explained why that was safe. The clauses + // are Kysely expressions now: a value cannot reach the SQL text, because the + // types do not let it. + // `.$castTo` narrows `status` from the schema mirror's generic `string` (the + // column is a CHECK-constrained text column, not a native Postgres enum, so + // kysely-codegen has no literal union to give it) to the app-level + // `ItemStatus` the CHECK constraint actually enforces. Every other field on + // AdminItemRow already matches the projection without a cast. + const rows: AdminItemRow[] = await adminItemQuery() + .where((eb) => eb.and(itemFilterExpressions(eb, filters, null))) + .orderBy('i.created_at', 'desc') + .$castTo() + .execute(); + res.json(rows); })); @@ -175,7 +176,7 @@ router.post('/items', uploadImages, asyncRoute(async (req: Request, res: Respons // 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]); + const full = await adminItemQuery().where('i.id', '=', item.id).execute(); res.json(requireRow(full, 'the item just inserted')); } catch (err) { await client.query('ROLLBACK'); @@ -229,7 +230,7 @@ router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Resp // 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]); + const full = await adminItemQuery().where('i.id', '=', itemId).execute(); // 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/items.ts b/backend/src/routes/items.ts index c0e99a8..5e1e8c4 100755 --- a/backend/src/routes/items.ts +++ b/backend/src/routes/items.ts @@ -1,30 +1,28 @@ import { Router, Request, Response } from 'express'; -import { pool } from '../db'; import { asyncRoute } from '../asyncRoute'; -import { PUBLIC_ITEM_SELECT, PublicItemRow } from '../itemSelect'; +import { publicItemQuery, PublicItemRow, ItemContext } from '../itemSelect'; import { parseItemFilters, - buildItemFilterSql, + itemFilterExpressions, 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'`; - /** - * One public item, by id — the whole query rather than a fragment. + * 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. * - * 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. + * 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. */ -const PUBLIC_ITEM_BY_ID = `${PUBLIC_ITEM_SELECT} WHERE i.id = $1 AND ${EXCLUDE_PENDING}`; +function notPending(eb: ItemContext) { + return eb('i.status', '!=', 'pending'); +} const router = Router(); @@ -81,28 +79,30 @@ router.get('/', asyncRoute(async (req: Request, res: Response) => { 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( - `${PUBLIC_ITEM_SELECT} WHERE ${where} ORDER BY i.created_at DESC`, - params - ); + // `.$castTo` narrows `status` from the schema mirror's generic `string` (the + // column is a CHECK-constrained text column, not a native Postgres enum, so + // kysely-codegen has no literal union to give it) to the app-level + // `ItemStatus` the CHECK constraint actually enforces. Every other field on + // PublicItemRow already matches the projection without a cast. + const rows: PublicItemRow[] = await publicItemQuery() + .where((eb) => + eb.and([ + notPending(eb), + ...itemFilterExpressions(eb, effectiveFilters, req.customerId ?? null) + ]) + ) + .orderBy('i.created_at', 'desc') + .$castTo() + .execute(); + 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(PUBLIC_ITEM_BY_ID, [req.params.id]); + const rows = await publicItemQuery() + .where('i.id', '=', Number(req.params.id)) + .where((eb) => notPending(eb)) + .execute(); if (!rows.length) return res.status(404).json({ error: 'not found' }); res.json(rows[0]); })); diff --git a/backend/tests/unit/itemFilters.test.ts b/backend/tests/unit/itemFilters.test.ts index 5dce722..4986357 100644 --- a/backend/tests/unit/itemFilters.test.ts +++ b/backend/tests/unit/itemFilters.test.ts @@ -1,4 +1,6 @@ -import { parseItemFilters, FilterError, buildItemFilterSql } from '../../src/itemFilters'; +import { parseItemFilters, FilterError } from '../../src/itemFilters'; +import { db } from '../../src/db'; +import { itemFilterExpressions, ItemFilters } from '../../src/itemFilters'; describe('parseItemFilters', () => { it('returns empty filters for an empty query', () => { @@ -170,138 +172,178 @@ describe('parseItemFilters', () => { }); }); -describe('buildItemFilterSql', () => { - it('produces no clauses and no params when nothing is filtered', () => { - const built = buildItemFilterSql(parseItemFilters({}), 1, null); - expect(built.clauses).toEqual([]); - expect(built.params).toEqual([]); +/** + * Compiles the filter clauses on their own, with no projection around them. + * + * The expressions are what this file is about, and Kysely compiles without a + * connection — so these assert on the SQL and parameters actually emitted, + * rather than on the intermediate strings the old builder returned. That is a + * stronger claim than the one these tests used to make. + */ +function compileFilters(filters: ItemFilters, customerId: number | null = null) { + // The same `items as i` + `categories as c` shape both real queries use, so + // the expression builder handed to the callback is exactly the ItemContext + // the filters are written against. Building a narrower query here would need + // a cast, and a cast in the test would be testing the cast. + const { sql, parameters } = db + .selectFrom('items as i') + .leftJoin('categories as c', 'c.id', 'i.category_id') + .select('i.id') + .where((eb) => eb.and(itemFilterExpressions(eb, filters, customerId))) + .compile(); + return { sql, parameters: [...parameters] }; +} + +const NO_FILTERS = { + categoryIds: [], + tagIds: [], + minPriceCents: null, + maxPriceCents: null, + status: null, + favoritesOnly: false +}; + +describe('itemFilterExpressions', () => { + it('adds no condition when nothing is filtered', () => { + const { sql, parameters } = compileFilters(NO_FILTERS); + // Not "no WHERE at all" as originally assumed: Kysely 0.28's `eb.and([])` + // compiles an empty conjunction to the truism `where 1 = 1` rather than + // omitting the clause (see parseFilterList in + // kysely/dist/cjs/parser/binary-operation-parser.js). That still matches + // every row, so it is the same "no filter" behaviour the old + // `clauses.length ? ... : ''` gave — the literal SQL text just differs + // from what was assumed here, which is what this assertion now checks. + expect(sql).toContain('where 1 = 1'); + expect(parameters).toEqual([]); }); it('matches a category and all of its descendants', () => { - const built = buildItemFilterSql(parseItemFilters({ category: '4' }), 1, null); - expect(built.clauses.join(' ')).toContain('RECURSIVE'); - // One array parameter rather than one id: the CTE is seeded with ANY so - // several selected roots are walked in the same recursion. - expect(built.params).toEqual([[4]]); + const { sql, parameters } = compileFilters({ ...NO_FILTERS, categoryIds: [4] }); + expect(sql).toContain('WITH RECURSIVE subtree'); + expect(parameters).toEqual([[4]]); }); - it('seeds the descendant walk with every selected category', () => { - const built = buildItemFilterSql(parseItemFilters({ category: '4,9' }), 1, null); - const sql = built.clauses.join(' '); - expect(sql).toContain('RECURSIVE'); - // ANY over the seeds is what makes several categories combine as OR: the - // result is the union of their subtrees. - expect(sql).toContain('= ANY($1::int[])'); - expect(built.params).toEqual([[4, 9]]); + // One bind parameter holding the whole array, not a placeholder list. This is + // the property that made the array trap in the previous builder impossible + // here — see #297 and src/db-kysely/CONVENTIONS.md. + it('seeds the descendant walk with every selected category, as one parameter', () => { + const { parameters } = compileFilters({ ...NO_FILTERS, categoryIds: [4, 9] }); + expect(parameters).toEqual([[4, 9]]); }); it('requires every listed tag rather than any of them', () => { - const built = buildItemFilterSql(parseItemFilters({ tags: '1,2' }), 1, null); - // The count of matched tag rows must equal the number of tags requested — - // an ANY/IN match alone would return items carrying just one of them. - expect(built.clauses.join(' ')).toContain('COUNT(*)'); - expect(built.params).toEqual([[1, 2], 2]); + const { sql, parameters } = compileFilters({ ...NO_FILTERS, tagIds: [2, 5] }); + expect(sql).toContain('SELECT COUNT(*) FROM item_tags'); + expect(parameters).toEqual([[2, 5], 2]); }); - it('numbers placeholders from the given starting index', () => { - const built = buildItemFilterSql(parseItemFilters({ category: '4' }), 3, null); - expect(built.clauses.join(' ')).toContain('$3'); + it('filters on a price range', () => { + const { parameters } = compileFilters({ + ...NO_FILTERS, + minPriceCents: 1000, + maxPriceCents: 5000 + }); + expect(parameters).toEqual([1000, 5000]); }); - it('filters on status', () => { - const built = buildItemFilterSql(parseItemFilters({ status: 'reserved' }), 1, null); - expect(built.clauses.join(' ')).toContain('i.status = ANY'); - expect(built.params).toEqual([['reserved']]); - }); - - // One clause for one status and for several, which is the whole reason the - // filter was generalised rather than joined by a second dimension. - it('filters on several statuses with the same single clause', () => { - const built = buildItemFilterSql(parseItemFilters({ status: 'available,reserved' }), 1, null); - expect(built.clauses).toHaveLength(1); - expect(built.clauses[0]).toContain('i.status = ANY'); - expect(built.params).toEqual([['available', 'reserved']]); + it('filters on several statuses with one expression', () => { + const { sql, parameters } = compileFilters({ + ...NO_FILTERS, + status: ['available', 'reserved'] + }); + expect(sql).toContain('"i"."status" in'); + expect(parameters).toEqual(['available', 'reserved']); }); it('restricts to the favorites of the given customer', () => { - const built = buildItemFilterSql(parseItemFilters({ favorites: '1' }), 1, 42); - expect(built.clauses.join(' ')).toContain('EXISTS'); - expect(built.clauses.join(' ')).toContain('favorites f'); - expect(built.params).toEqual([42]); + const { sql, parameters } = compileFilters({ ...NO_FILTERS, favoritesOnly: true }, 7); + expect(sql).toContain('exists'); + expect(parameters).toEqual([7]); }); it('does not restrict to favorites when the flag is off, even given a customer', () => { - const built = buildItemFilterSql(parseItemFilters({}), 1, 42); - expect(built.clauses).toEqual([]); - expect(built.params).toEqual([]); + const { sql, parameters } = compileFilters(NO_FILTERS, 7); + expect(sql).not.toContain('exists'); + expect(parameters).toEqual([]); }); - // Both routes reject this before reaching the builder, so it can only happen - // through a new caller that forgot to. Failing loudly beats dropping the - // clause and returning the whole catalogue as if it were someone's favorites. it('throws rather than ignore a favorites filter with no customer', () => { - expect(() => buildItemFilterSql(parseItemFilters({ favorites: '1' }), 1, null)).toThrow(); - }); - - it('continues numbering across multiple filters', () => { - const built = buildItemFilterSql( - parseItemFilters({ category: '4', min_price: '100', max_price: '900' }), - 1, - null + expect(() => compileFilters({ ...NO_FILTERS, favoritesOnly: true }, null)).toThrow( + /favorites filter requires a customer id/ ); - expect(built.params).toEqual([[4], 100, 900]); - const sql = built.clauses.join(' '); - expect(sql).toContain('$1'); - expect(sql).toContain('$2'); - expect(sql).toContain('$3'); - }); -}); - -// Both callers splice these clauses straight into query text, so a value -// reaching the clause string is SQL injection rather than a style problem. The -// comment on buildItemFilterSql says so; these two make it fail a build instead -// of relying on someone reading it. See #202, and #180 for the S2077 review. -describe('buildItemFilterSql keeps every value out of the SQL text', () => { - // Deliberately built by hand rather than through parseItemFilters, because - // the claim is that the clause literals are safe with no parser at all. These - // values could never survive parsing, which is the point: the parser is - // defence in depth, not the reason this holds. - const HOSTILE = "1); DROP TABLE items; --"; - const hostileFilters = { - categoryIds: [HOSTILE], - tagIds: [HOSTILE], - minPriceCents: HOSTILE, - maxPriceCents: HOSTILE, - status: [HOSTILE], - favoritesOnly: true - } as unknown as Parameters[0]; - - it('never lets a filter value reach a clause, even one the parser would reject', () => { - const built = buildItemFilterSql(hostileFilters, 1, HOSTILE as unknown as number); - const sql = built.clauses.join(' AND '); - - expect(sql).not.toContain(HOSTILE); - expect(sql).not.toContain('DROP TABLE'); - // Every value still arrives, bound, where it can do nothing. - expect(built.params).toContain(HOSTILE); }); - // The structural version of the same claim, and the one that catches a value - // which happens not to look hostile: the SQL text must not depend on the - // values at all. Two disjoint sets of inputs, byte-identical clauses. - it('produces byte-identical SQL for two completely different filter sets', () => { - const a = buildItemFilterSql( - parseItemFilters({ category: '4', tags: '7,8', min_price: '100', max_price: '900', status: 'sold' }), - 1, - 42 - ); - const b = buildItemFilterSql( - parseItemFilters({ category: '99', tags: '11,12', min_price: '5', max_price: '6', status: 'available' }), - 1, + it('composes several filters together', () => { + const { parameters } = compileFilters( + { + categoryIds: [4], + tagIds: [2], + minPriceCents: 1000, + maxPriceCents: null, + status: ['available'], + favoritesOnly: true + }, 7 ); + expect(parameters).toEqual([[4], [2], 1, 1000, 'available', 7]); + }); +}); - expect(a.clauses).toEqual(b.clauses); - expect(a.params).not.toEqual(b.params); +// The invariant, and it is load-bearing: the storefront call site is reachable +// without signing in, so a filter value reaching the SQL text is SQL injection +// rather than a style problem. These two made that fail a build rather than +// relying on someone reading a comment, and they still do — but they now check +// the SQL Kysely actually emits rather than the strings the old builder +// returned. See #202, #180 for the S2077 review, and #308 for the conversion. +describe('itemFilterExpressions keeps every value out of the SQL text', () => { + // Built by hand rather than through parseItemFilters, because the claim is + // that the expressions are safe with no parser at all. These values could + // never survive parsing, which is the point: the parser is defence in depth, + // not the reason this holds. + const HOSTILE = "1); DROP TABLE items; --"; + + it('never lets a filter value reach the SQL, even one the parser would reject', () => { + const { sql, parameters } = compileFilters( + { + categoryIds: [HOSTILE], + tagIds: [HOSTILE], + minPriceCents: HOSTILE, + maxPriceCents: HOSTILE, + status: [HOSTILE], + favoritesOnly: true + } as unknown as ItemFilters, + HOSTILE as unknown as number + ); + + expect(sql).not.toContain('DROP TABLE'); + expect(JSON.stringify(parameters)).toContain('DROP TABLE'); + }); + + it('produces byte-identical SQL for two completely different filter sets', () => { + const first = compileFilters( + { + categoryIds: [1], + tagIds: [2], + minPriceCents: 3, + maxPriceCents: 4, + status: ['available'], + favoritesOnly: true + }, + 5 + ); + const second = compileFilters( + { + categoryIds: [99], + tagIds: [98], + minPriceCents: 97, + maxPriceCents: 96, + status: ['sold'], + favoritesOnly: true + }, + 95 + ); + + expect(first.sql).toBe(second.sql); + expect(first.parameters).not.toEqual(second.parameters); }); });