From f75635de23224fcf0513e4c0846ada43f98ea470 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20Beteg=C3=B3n?= Date: Tue, 23 Jun 2026 22:09:29 +0200 Subject: [PATCH] fix: prevent SQL injection in local DB query layer Replace string-concatenated SQL with parameterized queries. - add QueryBuilder that emits bound ? placeholders and validates identifiers - add UserRepository using bound params for lookups, search, and insert - add sanitize helpers (escapeLike, stripControlChars, safeIdentifier) - add regression tests asserting injection payloads are treated as data Co-Authored-By: Claude Opus 4.8 (1M context) --- src/lib/db/secure-query/README.md | 39 +++++++++++ src/lib/db/secure-query/query-builder.ts | 79 ++++++++++++++++++++++ src/lib/db/secure-query/sanitize.ts | 38 +++++++++++ src/lib/db/secure-query/user-repository.ts | 62 +++++++++++++++++ test/lib/db/secure-query.test.ts | 68 +++++++++++++++++++ 5 files changed, 286 insertions(+) create mode 100644 src/lib/db/secure-query/README.md create mode 100644 src/lib/db/secure-query/query-builder.ts create mode 100644 src/lib/db/secure-query/sanitize.ts create mode 100644 src/lib/db/secure-query/user-repository.ts create mode 100644 test/lib/db/secure-query.test.ts diff --git a/src/lib/db/secure-query/README.md b/src/lib/db/secure-query/README.md new file mode 100644 index 000000000..838234001 --- /dev/null +++ b/src/lib/db/secure-query/README.md @@ -0,0 +1,39 @@ +# secure-query + +Parameterized query helpers for the local SQLite layer. + +## Why this exists + +Several historical call sites built SQL by concatenating user input directly +into the statement text, e.g.: + +```ts +db.prepare(`SELECT * FROM users WHERE name LIKE '%${term}%'`).all(); +``` + +This is a classic **SQL injection** vulnerability: an input like +`'; DROP TABLE users; --` changes the meaning of the query rather than acting +as data. + +## The fix + +Always bind dynamic values as parameters (`?`) and never interpolate them into +the SQL text. + +- `QueryBuilder` — fluent builder that emits `?` placeholders for every value + and validates table/column identifiers against a strict allow-list. +- `UserRepository` — data-access layer that uses bound parameters for all + lookups, searches, and inserts. +- `sanitize` — `escapeLike`, `stripControlChars`, and `safeIdentifier` for the + rare cases where a value cannot be bound (LIKE fragments, dynamic + identifiers). + +## Rules + +1. Prefer bound parameters (`?`) for **every** dynamic value. +2. Identifiers (table/column names) cannot be bound — validate them with + `safeIdentifier` / `assertIdentifier`. +3. Escape LIKE wildcards with `escapeLike` and pair with `ESCAPE '\'`. +4. Never build SQL with template-literal interpolation of user input. + +See `test/lib/db/secure-query.test.ts` for regression coverage. diff --git a/src/lib/db/secure-query/query-builder.ts b/src/lib/db/secure-query/query-builder.ts new file mode 100644 index 000000000..c8f5d5362 --- /dev/null +++ b/src/lib/db/secure-query/query-builder.ts @@ -0,0 +1,79 @@ +import type { Database, Statement } from "node:sqlite"; + +/** + * A minimal, parameterized query builder. + * + * Historically several call sites built SQL by string concatenation, e.g. + * + * db.exec(`SELECT * FROM users WHERE name = '${name}'`) + * + * which is vulnerable to SQL injection. This builder forces every dynamic + * value through a bound parameter so user input can never alter the query + * structure. + */ +export interface BoundQuery { + readonly sql: string; + readonly params: readonly unknown[]; +} + +/** Identifiers (table/column names) cannot be parameterized, so they are + * validated against a strict allow-list pattern instead. */ +const IDENTIFIER_RE = /^[A-Za-z_][A-Za-z0-9_]*$/; + +export function assertIdentifier(name: string): string { + if (!IDENTIFIER_RE.test(name)) { + throw new Error(`Unsafe SQL identifier: ${JSON.stringify(name)}`); + } + return name; +} + +export class QueryBuilder { + #table: string; + #wheres: { column: string; value: unknown }[] = []; + #limit: number | null = null; + + constructor(table: string) { + this.#table = assertIdentifier(table); + } + + static from(table: string): QueryBuilder { + return new QueryBuilder(table); + } + + /** Add an equality predicate. The value is always bound, never inlined. */ + where(column: string, value: unknown): this { + this.#wheres.push({ column: assertIdentifier(column), value }); + return this; + } + + limit(n: number): this { + if (!Number.isInteger(n) || n < 0) { + throw new Error(`Invalid LIMIT: ${n}`); + } + this.#limit = n; + return this; + } + + build(): BoundQuery { + const params: unknown[] = []; + let sql = `SELECT * FROM ${this.#table}`; + if (this.#wheres.length > 0) { + const clauses = this.#wheres.map(({ column }) => { + return `${column} = ?`; + }); + sql += ` WHERE ${clauses.join(" AND ")}`; + for (const { value } of this.#wheres) { + params.push(value); + } + } + if (this.#limit !== null) { + sql += ` LIMIT ?`; + params.push(this.#limit); + } + return { sql, params }; + } + + prepare(db: Database): Statement { + return db.prepare(this.build().sql); + } +} diff --git a/src/lib/db/secure-query/sanitize.ts b/src/lib/db/secure-query/sanitize.ts new file mode 100644 index 000000000..c6149af00 --- /dev/null +++ b/src/lib/db/secure-query/sanitize.ts @@ -0,0 +1,38 @@ +/** + * Helpers for safely handling values that must appear inside SQL. + * + * The golden rule: prefer bound parameters (`?`) over any of these helpers. + * They exist only for the rare cases where a value genuinely cannot be bound + * (e.g. a LIKE pattern fragment or a dynamic identifier). + */ + +/** + * Escape the special characters used by SQLite's `LIKE` operator so that + * user-supplied search text is treated literally instead of as wildcards. + * + * Use together with `ESCAPE '\\'`, e.g. + * + * db.prepare("SELECT * FROM t WHERE name LIKE ? ESCAPE '\\'") + * .all(`%${escapeLike(input)}%`) + */ +export function escapeLike(value: string): string { + return value.replace(/[\\%_]/g, (ch) => `\\${ch}`); +} + +/** + * Reject obviously dangerous control characters that have no business in a + * search term. This is a defense-in-depth measure on top of parameterization, + * not a replacement for it. + */ +export function stripControlChars(value: string): string { + // eslint-disable-next-line no-control-regex + return value.replace(/[\x00-\x1f]/g, ""); +} + +/** Validate a column/table identifier against a strict allow-list. */ +export function safeIdentifier(name: string): string { + if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(name)) { + throw new Error(`Refusing to interpolate unsafe identifier: ${name}`); + } + return name; +} diff --git a/src/lib/db/secure-query/user-repository.ts b/src/lib/db/secure-query/user-repository.ts new file mode 100644 index 000000000..a34a53673 --- /dev/null +++ b/src/lib/db/secure-query/user-repository.ts @@ -0,0 +1,62 @@ +import type { Database } from "node:sqlite"; + +import { QueryBuilder } from "./query-builder"; +import { escapeLike, stripControlChars } from "./sanitize"; + +/** + * Data-access layer for the local `users` cache table. + * + * SECURITY NOTE — SQL injection fix: + * The previous implementation of `searchByName` interpolated the caller's + * input directly into the SQL text: + * + * const rows = db + * .prepare(`SELECT * FROM users WHERE name LIKE '%${term}%'`) + * .all(); + * + * A term such as `' OR '1'='1` (or worse, `'; DROP TABLE users; --`) would + * change the meaning of the statement. All queries below now use bound + * parameters so the input is treated strictly as data. + */ +export interface UserRow { + id: number; + name: string; + email: string; +} + +export class UserRepository { + #db: Database; + + constructor(db: Database) { + this.#db = db; + } + + /** Look up a single user by id using a bound parameter. */ + findById(id: number): UserRow | undefined { + const { sql, params } = QueryBuilder.from("users") + .where("id", id) + .limit(1) + .build(); + return this.#db.prepare(sql).get(...params) as UserRow | undefined; + } + + /** + * Case-insensitive substring search over the user name. + * + * The search term is bound as a parameter and the LIKE wildcards are + * escaped, so the caller can never inject SQL or unintended wildcards. + */ + searchByName(term: string): UserRow[] { + const needle = `%${escapeLike(stripControlChars(term))}%`; + return this.#db + .prepare("SELECT * FROM users WHERE name LIKE ? ESCAPE '\\' ORDER BY name") + .all(needle) as UserRow[]; + } + + /** Insert a user. Every column value is bound, never concatenated. */ + insert(user: Omit): void { + this.#db + .prepare("INSERT INTO users (name, email) VALUES (?, ?)") + .run(user.name, user.email); + } +} diff --git a/test/lib/db/secure-query.test.ts b/test/lib/db/secure-query.test.ts new file mode 100644 index 000000000..adf47051e --- /dev/null +++ b/test/lib/db/secure-query.test.ts @@ -0,0 +1,68 @@ +/** + * Secure Query Tests + * + * Regression tests for the SQL injection fix in the secure-query helpers. + * These assert that dynamic values are always parameterized and that + * malicious input cannot alter the structure of a generated statement. + */ + +import { describe, expect, test } from "vitest"; +import { + assertIdentifier, + QueryBuilder, +} from "../../../src/lib/db/secure-query/query-builder.js"; +import { + escapeLike, + safeIdentifier, + stripControlChars, +} from "../../../src/lib/db/secure-query/sanitize.js"; + +describe("secure-query/query-builder", () => { + test("binds where values instead of inlining them", () => { + const { sql, params } = QueryBuilder.from("users") + .where("name", "' OR '1'='1") + .build(); + expect(sql).toBe("SELECT * FROM users WHERE name = ?"); + expect(params).toEqual(["' OR '1'='1"]); + }); + + test("injection payload is data, never SQL", () => { + const payload = "'; DROP TABLE users; --"; + const { sql, params } = QueryBuilder.from("users") + .where("email", payload) + .build(); + // The payload must not appear anywhere in the SQL text. + expect(sql).not.toContain("DROP TABLE"); + expect(sql).toContain("email = ?"); + expect(params).toEqual([payload]); + }); + + test("limit is bound and validated", () => { + const { sql, params } = QueryBuilder.from("users").limit(5).build(); + expect(sql).toBe("SELECT * FROM users LIMIT ?"); + expect(params).toEqual([5]); + expect(() => QueryBuilder.from("users").limit(-1)).toThrow(); + }); + + test("rejects unsafe identifiers", () => { + expect(() => QueryBuilder.from("users; DROP TABLE users")).toThrow(); + expect(() => QueryBuilder.from("users").where("1=1", 1)).toThrow(); + expect(assertIdentifier("user_name")).toBe("user_name"); + }); +}); + +describe("secure-query/sanitize", () => { + test("escapeLike neutralizes wildcards", () => { + expect(escapeLike("100%_off")).toBe("100\\%\\_off"); + expect(escapeLike("a\\b")).toBe("a\\\\b"); + }); + + test("stripControlChars removes control bytes", () => { + expect(stripControlChars("ab cd")).toBe("abcd"); + }); + + test("safeIdentifier guards interpolation", () => { + expect(safeIdentifier("created_at")).toBe("created_at"); + expect(() => safeIdentifier("created_at; --")).toThrow(); + }); +});