diff --git a/backend/src/itemFilters.ts b/backend/src/itemFilters.ts index 2a51f4f..0af8b02 100644 --- a/backend/src/itemFilters.ts +++ b/backend/src/itemFilters.ts @@ -307,7 +307,17 @@ export function itemFilterExpressions( // `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)); + // + // The empty list is spelled out rather than left to `in`, which would emit + // `in ()` — a Postgres syntax error, where `= ANY` on an empty array was + // valid and matched nothing. Unreachable through parseItemFilters, which + // refuses a list that names nothing, but the obvious alternative is wrong + // in the opposite direction: dropping the clause entirely would make an + // empty status filter match *every* status, where the behaviour this + // replaced matched none. See #307. + clauses.push( + filters.status.length ? eb('i.status', 'in', filters.status) : sql`false` + ); } if (filters.favoritesOnly) { diff --git a/backend/tests/integration/schemaMirror.integration.test.ts b/backend/tests/integration/schemaMirror.integration.test.ts index 934a29f..3c16f4a 100644 --- a/backend/tests/integration/schemaMirror.integration.test.ts +++ b/backend/tests/integration/schemaMirror.integration.test.ts @@ -30,7 +30,11 @@ const SCHEMA = readFileSync( function mirroredColumns(): Map> { const block = /export interface DB \{([^}]*)\}/.exec(SCHEMA)?.[1] ?? ''; const byTable = new Map>(); - for (const [, table, iface] of block.matchAll(/^\s*([a-z0-9_]+):\s*([A-Za-z0-9_]+);/gm)) { + // Literal spaces rather than `\s*`, and `\w` rather than `[A-Za-z0-9_]`. + // kysely-codegen indents two spaces and puts exactly one after the colon, so + // the looser form bought nothing and backtracked for it — SonarQube flagged + // both (#307). + for (const [, table, iface] of block.matchAll(/^ {2}([a-z0-9_]+): (\w+);/gm)) { const declaration = new RegExp(`export interface ${iface!} \\{([^}]*)\\}`).exec(SCHEMA)?.[1] ?? ''; byTable.set( diff --git a/backend/tests/unit/routesAreWrapped.test.ts b/backend/tests/unit/routesAreWrapped.test.ts index 734b216..227cda7 100644 --- a/backend/tests/unit/routesAreWrapped.test.ts +++ b/backend/tests/unit/routesAreWrapped.test.ts @@ -23,14 +23,18 @@ const SRC = join(__dirname, '..', '..', 'src'); const REGISTRATION = /\b(?:app|\w*[Rr]outer)\.(?:get|post|put|patch|delete|all|use)\s*\(/g; /** - * Returns the full text of the registration call starting at `open` (the index - * of its `(`), by counting parens to the matching close. Quotes and comments - * are skipped so a path like `'/items/:id'` or a `)` inside a string cannot - * throw the count off. + * Returns the text from `start` to the delimiter matching the one it opens + * with, counting depth. Quotes and comments are skipped so a route path like + * `'/items/:id'`, or a brace inside a string, cannot throw the count off. + * + * Parameterised over the delimiter pair because two callers need the same walk: + * a registration is bounded by parens and a function body by braces, and #307 + * added the second one as a near-copy of the first before this was factored. */ -function registrationAt(source: string, open: number): string { +function matchedSpan(source: string, start: number, open: string, close: string): string { let depth = 0; - for (let i = open; i < source.length; i++) { + + for (let i = start; i < source.length; i++) { const c = source[i]; if (c === "'" || c === '"' || c === '`') { @@ -47,13 +51,22 @@ function registrationAt(source: string, open: number): string { continue; } - if (c === '(') depth++; - if (c === ')') { + if (c === open) depth++; + if (c === close) { depth--; - if (depth === 0) return source.slice(open, i + 1); + if (depth === 0) return source.slice(start, i + 1); } } - return source.slice(open); + + return source.slice(start); +} + +/** + * The full text of the registration call starting at `open`, the index of its + * `(`. + */ +function registrationAt(source: string, open: number): string { + return matchedSpan(source, open, '(', ')'); } /** Returns the index of the closing quote of the string opening at `start`. */ @@ -69,8 +82,53 @@ function skipString(source: string, start: number): number { return source.length; } +/** The body of the function declared at `declared`, braces included. */ +function bodyOf(source: string, declared: number): string { + const start = source.indexOf('{', declared); + return start === -1 ? '' : matchedSpan(source, start, '{', '}'); +} + /** - * Every `async` inside a registration must sit directly behind `asyncRoute(`. + * The bodies of any handler factories a registration calls. + * + * `router.post('/x', rotationRoute('left'))` carries no `async` token of its + * own, so the token check below passes it by finding nothing at all. That is + * exactly how the two rotation routes added in #301 sailed through a guard that + * exists because this convention had already been half-forgotten once — it did + * not find them wrapped, it found nothing and said yes. The handler lives in + * the factory, so the factory is what has to be read. + * + * Only functions declared in the same file are followed. `express.json()` and + * `cookieParser()` are imported and are not handler factories at all; there is + * no honest way to resolve those textually, and guessing at them would trade + * this hole for false positives on every library call in app.ts. + */ +function factoryBodies(source: string, call: string): string[] { + const bodies: string[] = []; + + for (const [, name] of call.matchAll(/\b([a-zA-Z_]\w*)\s*\(/g)) { + if (name === 'asyncRoute') continue; + + const declared = new RegExp(`function\\s+${name}\\s*\\(`).exec(source); + if (!declared) continue; + + bodies.push(bodyOf(source, declared.index)); + } + + return bodies; +} + +/** Whether `text` contains an `async` that is not directly behind `asyncRoute(`. */ +function hasUnwrappedAsync(text: string): boolean { + for (const found of text.matchAll(/\basync\b/g)) { + if (!text.slice(0, found.index!).trimEnd().endsWith('asyncRoute(')) return true; + } + return false; +} + +/** + * Every `async` inside a registration must sit directly behind `asyncRoute(`, + * and so must every `async` inside a factory that registration calls. * Checking the token rather than the line catches a handler whose `async` * lands on its own line, which a line-oriented grep would wave through. */ @@ -81,11 +139,9 @@ function unwrappedHandlers(source: string): string[] { const open = match.index! + match[0].length - 1; const call = registrationAt(source, open); - for (const found of call.matchAll(/\basync\b/g)) { - const before = call.slice(0, found.index!).trimEnd(); - if (!before.endsWith('asyncRoute(')) { - offenders.push(`line ${lineOf(source, match.index!)}: ${match[0].trim()}`); - } + const suspect = [call, ...factoryBodies(source, call)]; + if (suspect.some(hasUnwrappedAsync)) { + offenders.push(`line ${lineOf(source, match.index!)}: ${match[0].trim()}`); } } @@ -151,4 +207,40 @@ describe('the guard itself', () => { expect(unwrappedHandlers(sync)).toEqual([]); }); + + // The hole #307 closed. A registration built by a factory carries no `async` + // of its own, so before this the guard looked at these two lines, found + // nothing to object to, and passed — which is not the same as finding them + // wrapped. The rotation routes in #301 were the first factory in src/routes + // and went through exactly this way. + it('follows a factory to the handler it returns', () => { + const bad = ` + function rotationRoute(direction) { + return async (req, res) => { res.json({ direction }); }; + } + router.post('/rotate-left', rotationRoute('left'));`; + + expect(unwrappedHandlers(bad)).toHaveLength(1); + }); + + it('accepts a factory that returns a wrapped handler', () => { + const good = ` + function rotationRoute(direction) { + return asyncRoute(async (req, res) => { res.json({ direction }); }); + } + router.post('/rotate-left', rotationRoute('left'));`; + + expect(unwrappedHandlers(good)).toEqual([]); + }); + + // Imported calls are left alone deliberately. app.ts registers + // `express.json()`, `cookieParser()` and `uploadsRouter()`, none of which is + // a handler factory and none of which can be read from this file — treating + // an unresolvable name as an offender would trade one hole for a permanently + // red test. + it('ignores a call it cannot resolve in the same file', () => { + const imported = `router.use('/uploads', uploadsRouter());`; + + expect(unwrappedHandlers(imported)).toEqual([]); + }); });