fix sql injection

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.

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);
+  }
+}

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;
+}

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<UserRow, "id">): void {
+    this.#db
+      .prepare("INSERT INTO users (name, email) VALUES (?, ?)")
+      .run(user.name, user.email);
+  }
+}

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();
+  });
+});