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;