From a86aa4e8192a1d123c14920768a3fe612c85fd10 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 11:59:47 +0200 Subject: [PATCH] ci(server): refuse SQL text in repositories and seeders, move the gallery oracle to tests The repository rule that keeps dialect SQL in sql-functions.ts only matched raw() and the sql tag, so a SQL string handed straight to the connection, em.getConnection().execute('INSERT OR IGNORE ...'), passed unseen, and the seeders sat outside every repository rule. A new block refuses a string or template whose text starts like a statement as the first argument of execute, run, all or get in src/db/repositories and src/db/seeders; a result mode such as qb.execute('run') stays legal. The files that already hold such SQL are named one by one so the list can only shrink: MaintenanceRepository (PRAGMA, VACUUM INTO), DemoRepository and six of the seven seeders. The selector groups are named once in eslint.config.mjs, and the selector test now also checks the new block per path, its exceptions, and that raw() stays forbidden in repositories. journey-gallery-order.ts exported raw SQLite ORDER BY text from src/nest that no production code read any more since the gallery reads moved onto JourneyPhotosRepository. Two parity tests still compare the Kysely rebuild against it, so it moves to tests/helpers as their oracle. --- server/CLAUDE.md | 2 +- server/eslint.config.mjs | 49 +++++++++++++++++ server/src/db/dialect/sql-functions.ts | 2 +- .../nest/journey/journey-domain.service.ts | 8 +-- .../helpers/legacy-gallery-order.ts} | 6 ++- .../unit/db/dialect/sql-functions.test.ts | 4 +- .../restricted-syntax-selectors.test.ts | 52 ++++++++++++++++++- .../unit/nest/journey-domain.service.test.ts | 2 +- .../unit/nest/journey-share.service.test.ts | 2 +- 9 files changed, 115 insertions(+), 12 deletions(-) rename server/{src/nest/journey/journey-gallery-order.ts => tests/helpers/legacy-gallery-order.ts} (84%) diff --git a/server/CLAUDE.md b/server/CLAUDE.md index d312b19d8..c0d1ef56a 100644 --- a/server/CLAUDE.md +++ b/server/CLAUDE.md @@ -45,7 +45,7 @@ Nest owns everything. Every domain is a DI module under `src/nest//` (`c - **Thin controller → injectable provider.** Never resurrect a plain function-module service; don't grow god services — split a domain into several modules when it outgrows one class (trips and auth already are). - **Inject, don't reach for module globals.** No imports of the global `db` proxy from `nest/` layers at all any more — the last production importer converted onto a repository, and the export now survives only so ~190 test files' `vi.mock('…/db/database', …)` factories keep typechecking; no new code depending on the `ws` singleton or event-sink globals. There is no `DatabaseService` to inject either — take the repository(ies) you need plus `UnitOfWork` for transactions. Modules must be importable without side effects. -- **Transactions are not optional.** Multi-statement writes go in `await this.uow.transactional(async () => …)` (`UnitOfWork` from `nest/database`, provided by the global `OrmModule`); never hand-roll `BEGIN`/`COMMIT` and never call `db.transaction()` (the Phase 1 async sweep removes it). Every DB-touching method is `async`; `@typescript-eslint/no-floating-promises` and `no-misused-promises` are errors, so an unawaited call or a `Promise` in a condition fails lint. Bind values with `?`; identifiers only via a literal allow-list. +- **Transactions are not optional.** Multi-statement writes go in `await this.uow.transactional(async () => …)` (`UnitOfWork` from `nest/database`, provided by the global `OrmModule`); never hand-roll `BEGIN`/`COMMIT` and never call `db.transaction()` (the Phase 1 async sweep removes it). Every DB-touching method is `async`; `@typescript-eslint/no-floating-promises` and `no-misused-promises` are errors, so an unawaited call or a `Promise` in a condition fails lint. Bind values with `?`; identifiers only via a literal allow-list. In `src/db/repositories/` and `src/db/seeders/`, ESLint refuses raw SQL in any spelling: `raw()`, the `sql` tag, and SQL text handed to `execute`/`run`/`all`/`get`. Build the statement with the query builder; `MaintenanceRepository`, `DemoRepository` and the six existing seeders are the named exceptions in `eslint.config.mjs`. - **Migrations are append-only.** A migration is identified by its class name in `mikro_orm_migrations`, and files sort by the timestamp in that name — never rename or delete one that has shipped, and never edit its body; add a new one. No env interpolation in SQL. (The old positional `schema_version` array and the hand-kept `db/migrations.ts` test builder are both gone; the numbered migrations are the only schema history, for tests too.) A new foreign key comes with an index that leads with its column(s), or deleting the parent scans the child table; `tests/unit/db/foreign-key-indexes.test.ts` fails on one without. - **Every write endpoint validates** through the Zod pipe with a schema from `@trek/shared` — no `Record` + ad-hoc checks. Also verify every referenced id exists and belongs to the same trip. - **Every outbound `fetch`** gets a timeout, a response-size cap and boundary validation — no `as`-casting provider responses, no silent `catch → null`. User-influenced URLs go through the SSRF guard. Never fall back to another user's API key. ESLint requires an init with a `signal` on `fetch`, `safeFetch`, `safeFetchFollow` and `safeFetchAdminConfigured` alike (`safeFetchLlm` takes `LLM_TIMEOUT_MS` inside). Behind that, the guarded wrappers (`utils/ssrfGuard.ts`) cap every body at `DEFAULT_MAX_RESPONSE_BYTES` (64 MB) and, for a call without a signal, wait `DEFAULT_RESPONSE_TIMEOUT_MS` (30 s) for the headers. Pass a tighter `maxBytes` when you buffer a small answer, `timeoutMs` for a deadline over the whole exchange, and `maxBytes: null` only where the body is streamed on or counted by the caller (`pipeAsset`, the doc-sync transfers, the Ollama pull). diff --git a/server/eslint.config.mjs b/server/eslint.config.mjs index 6e57b911e..6bfcd7bad 100644 --- a/server/eslint.config.mjs +++ b/server/eslint.config.mjs @@ -69,6 +69,23 @@ const FETCH_SELECTORS = [ }, ]; +// The statements a SQL string starts with. Matching on the text rather than on +// the method name keeps `qb.execute('run')` (a result mode, not SQL) legal. +const SQL_TEXT = String.raw`/^\s*(SELECT|INSERT|UPDATE|DELETE|REPLACE|WITH|PRAGMA|VACUUM|CREATE|DROP|ALTER|ATTACH|BEGIN|COMMIT)\b/i`; +const SQL_STRING_MESSAGE = + 'A SQL string handed to the connection is raw SQL too. Build it with the query builder (this.kysely(), em.createQueryBuilder) and route dialect functions through src/db/dialect/sql-functions.ts.'; + +const STRING_SQL_SELECTORS = [ + { + selector: `CallExpression[callee.property.name=/^(execute|run|all|get)$/] > Literal.arguments:first-child[value=${SQL_TEXT}]`, + message: SQL_STRING_MESSAGE, + }, + { + selector: `CallExpression[callee.property.name=/^(execute|run|all|get)$/] > TemplateLiteral.arguments:first-child[quasis.0.value.raw=${SQL_TEXT}]`, + message: SQL_STRING_MESSAGE, + }, +]; + export default tseslint.config( gitignore({ strict: false }), { @@ -299,6 +316,38 @@ export default tseslint.config( 'no-restricted-syntax': ['error', ...ENV_SELECTORS, ...RAW_SQL_SELECTORS, ...FETCH_SELECTORS], }, }, + { + // RAW_SQL_SELECTORS only see `raw()` and the `sql` tag. A SQL string + // handed straight to the connection + // (`em.getConnection().execute('INSERT OR IGNORE ...')`) is raw SQL all the + // same and passed unseen, in the seeders too, which no repository rule + // reached. The files below already hold such strings and are named one by + // one, so the list can only shrink: MaintenanceRepository is the SQLite + // maintenance adapter (PRAGMA, VACUUM INTO), DemoRepository is the + // documented escape hatch for the demo seed data, and every seeder but + // DatabaseSeeder writes its rows as SQL text (four of them with the + // SQLite-only INSERT OR IGNORE). A new seeder starts on the query builder. + files: ['src/db/repositories/**/*.ts', 'src/db/seeders/**/*.ts'], + ignores: [ + 'src/db/repositories/MaintenanceRepository.ts', + 'src/db/repositories/DemoRepository.ts', + 'src/db/seeders/AddonSeeder.ts', + 'src/db/seeders/AdminSeeder.ts', + 'src/db/seeders/CategorySeeder.ts', + 'src/db/seeders/DocumentProviderSeeder.ts', + 'src/db/seeders/PhotoProviderSeeder.ts', + 'src/db/seeders/SchemaVersionSeeder.ts', + ], + rules: { + 'no-restricted-syntax': [ + 'error', + ...ENV_SELECTORS, + ...RAW_SQL_SELECTORS, + ...FETCH_SELECTORS, + ...STRING_SQL_SELECTORS, + ], + }, + }, { // The plugin RPC decorator kit is written to stay extractable into its own // package, so its dependencies are pinned to Nest plus the two host modules it diff --git a/server/src/db/dialect/sql-functions.ts b/server/src/db/dialect/sql-functions.ts index b20e4d807..bf6a6498c 100644 --- a/server/src/db/dialect/sql-functions.ts +++ b/server/src/db/dialect/sql-functions.ts @@ -736,7 +736,7 @@ export function lowerTrimParam(platform: Platform, value: string): RawQueryFragm // --------------------------------------------------------------------------- // Plan 3g Task 0 (R1) — no consumer yet: Task 1 (`getJourneyFull`'s gallery // read) and Task 3 (`getPublicJourney`'s gallery read) each fold this into -// their own rebuild of `journey-gallery-order.ts`'s `GALLERY_CHRONOLOGICAL_ORDER` +// their own rebuild of the legacy `GALLERY_CHRONOLOGICAL_ORDER` (tests/helpers/) // text (a `COALESCE(NULLIF(tp.taken_at, ''), , // )` ORDER BY, worked out in full in Task 0's report for both to paste // in verbatim, neither re-deriving it). Added here, Kysely-only (no MikroORM diff --git a/server/src/nest/journey/journey-domain.service.ts b/server/src/nest/journey/journey-domain.service.ts index 446a75086..c708ddfec 100644 --- a/server/src/nest/journey/journey-domain.service.ts +++ b/server/src/nest/journey/journey-domain.service.ts @@ -86,10 +86,10 @@ function countryNamesFor(points: { country: string | null }[]): Record { }); // Plan 3g Task 0 (R1) — unixEpochToIsoKysely, the third fallback tier of - // `GALLERY_CHRONOLOGICAL_ORDER` (journey-gallery-order.ts). No consumer + // `GALLERY_CHRONOLOGICAL_ORDER` (tests/helpers/legacy-gallery-order.ts). No consumer // yet: Task 1 (getJourneyFull) and Task 3 (getPublicJourney) each fold this // into their own rebuild of the constant. The full composed ORDER BY proof // (coalesce + nullif + correlated subquery + this helper, on a seeded @@ -1337,7 +1337,7 @@ describe('sql-functions (sqlite)', () => { /** * Plan 3g Task 0 (R1) — the full Kysely rebuild of - * `journey-gallery-order.ts`'s `GALLERY_CHRONOLOGICAL_ORDER`, as a worked + * `GALLERY_CHRONOLOGICAL_ORDER` (tests/helpers/legacy-gallery-order.ts), as a worked * example: `COALESCE(NULLIF(tp.taken_at, ''), , ) ASC, gp.sort_order ASC, gp.id ASC`. NOT exported from diff --git a/server/tests/unit/eslint-rules/restricted-syntax-selectors.test.ts b/server/tests/unit/eslint-rules/restricted-syntax-selectors.test.ts index e7f0047c5..29737e788 100644 --- a/server/tests/unit/eslint-rules/restricted-syntax-selectors.test.ts +++ b/server/tests/unit/eslint-rules/restricted-syntax-selectors.test.ts @@ -4,7 +4,7 @@ import { ESLint, Linter } from 'eslint'; import path from 'path'; import { beforeAll, describe, expect, it } from 'vitest'; -// The outbound-timeout guard is no-restricted-syntax selectors in +// The outbound-timeout and raw-SQL guards are no-restricted-syntax selectors in // eslint.config.mjs, and a selector that stops matching fails silently: the // lint stays green and nothing is checked. This asks ESLint for the options it // really applies to a given path (so the flat-config blocks and their ignores @@ -90,3 +90,53 @@ describe('outbound fetch timeout selectors', () => { expect(messages(repo, `${PRELUDE}\nvoid safeFetch(url, undefined, {});`)).toEqual([TIMEOUT]); }); }); + +describe('raw SQL string selectors', () => { + const SQL = ` +declare const connection: { execute(sql: string, params?: unknown[], mode?: string): Promise }; +declare const qb: { execute(mode: string): Promise }; +declare const repo: { run(sql: string, params?: unknown[]): Promise; get(key: string): unknown }; +`; + const hits = (options: unknown[], code: string) => + messages(options, `${SQL}\n${code}`).filter((m) => m.startsWith('A SQL string handed to the connection')); + + it('flags a SQL string handed to the connection in a repository and in a seeder', async () => { + for (const file of ['src/db/repositories/Probe.repository.ts', 'src/db/seeders/ProbeSeeder.ts']) { + const options = await selectorsFor(file); + expect(hits(options, "void connection.execute('INSERT OR IGNORE INTO t (a) VALUES (?)', [1]);")).toHaveLength(1); + expect(hits(options, 'void connection.execute(` select * from t where id = ?`, [1]);')).toHaveLength(1); + expect(hits(options, "void repo.run('PRAGMA wal_checkpoint(TRUNCATE)');")).toHaveLength(1); + // A result mode and a map lookup are not SQL. + expect(hits(options, "void qb.execute('run'); void repo.get('key');")).toEqual([]); + } + }); + + it('still forbids raw() and the sql tag in a repository', async () => { + const options = await selectorsFor('src/db/repositories/Probe.repository.ts'); + const found = messages(options, "declare function raw(s: string): unknown;\nvoid raw('now()');"); + expect(found.some((m) => m.startsWith('Dialect SQL goes through'))).toBe(true); + }); + + it('leaves the named adapters alone, and only them', async () => { + for (const file of [ + 'src/db/repositories/MaintenanceRepository.ts', + 'src/db/repositories/DemoRepository.ts', + 'src/db/seeders/AddonSeeder.ts', + 'src/db/seeders/AdminSeeder.ts', + 'src/db/seeders/CategorySeeder.ts', + 'src/db/seeders/DocumentProviderSeeder.ts', + 'src/db/seeders/PhotoProviderSeeder.ts', + 'src/db/seeders/SchemaVersionSeeder.ts', + ]) { + const options = await selectorsFor(file); + expect(hits(options, "void connection.execute('PRAGMA wal_checkpoint(TRUNCATE)');"), file).toEqual([]); + // The rest of the guard still applies to them. + expect(messages(options, `${PRELUDE}\nvoid safeFetch(url);`), file).toContain(TIMEOUT); + } + }); + + it('does not reach outside src/db', async () => { + const options = await selectorsFor('src/nest/memories/probe.service.ts'); + expect(hits(options, "void connection.execute('SELECT 1');")).toEqual([]); + }); +}); diff --git a/server/tests/unit/nest/journey-domain.service.test.ts b/server/tests/unit/nest/journey-domain.service.test.ts index d3f4a6ffc..fa66f8213 100644 --- a/server/tests/unit/nest/journey-domain.service.test.ts +++ b/server/tests/unit/nest/journey-domain.service.test.ts @@ -59,7 +59,7 @@ import { // M5d (task-5-review.md) — full-key parity tests for Task 2's read models, // ported from the reviewer's probe (scratchpad r3g/probe/zz-r3g-parity-probe.test.ts). import { todayUtc } from '@trek/shared'; -import { GALLERY_CHRONOLOGICAL_ORDER } from '../../../src/nest/journey/journey-gallery-order'; +import { GALLERY_CHRONOLOGICAL_ORDER } from '../../helpers/legacy-gallery-order'; let svc: JourneyDomainService; // Plan 3g Task 1's own additions (below, "repositories (R9's parity + mutation diff --git a/server/tests/unit/nest/journey-share.service.test.ts b/server/tests/unit/nest/journey-share.service.test.ts index 85083acf0..b4061d5cd 100644 --- a/server/tests/unit/nest/journey-share.service.test.ts +++ b/server/tests/unit/nest/journey-share.service.test.ts @@ -49,7 +49,7 @@ import { createTestJourneyShareTokensRepo } from '../../helpers/journey-share-re import type { JourneyPublicGalleryRow, JourneyShareTokensRepository } from '../../../src/db/repositories/JourneyShareTokens.repository'; import type { JourneyEntriesRepository } from '../../../src/db/repositories/JourneyEntries.repository'; import type { JourneyEntryPhotosRepository } from '../../../src/db/repositories/JourneyEntryPhotos.repository'; -import { GALLERY_CHRONOLOGICAL_ORDER } from '../../../src/nest/journey/journey-gallery-order'; +import { GALLERY_CHRONOLOGICAL_ORDER } from '../../helpers/legacy-gallery-order'; let svc: JourneyShareService; let t: TestOrm;