mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
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.
This commit is contained in:
+1
-1
@@ -45,7 +45,7 @@ Nest owns everything. Every domain is a DI module under `src/nest/<domain>/` (`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<string, unknown>` + 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).
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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, ''), <correlated MIN+concat subquery>,
|
||||
// <this>)` 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
|
||||
|
||||
@@ -86,10 +86,10 @@ function countryNamesFor(points: { country: string | null }[]): Record<string, s
|
||||
// `GALLERY_JOIN` raw-SQL-text consts these two comment paragraphs used to
|
||||
// document are GONE — every site that folded them into a `this.db.prepare(...)`
|
||||
// call now reads through `JourneyPhotosRepository`/`JourneyEntryPhotosRepository`
|
||||
// instead (JG15/19/72/88-102/108-116, `journey-share.service.ts`'s own
|
||||
// GALLERY_CHRONOLOGICAL_ORDER import for JS15 is unaffected — that constant
|
||||
// lives in `journey-gallery-order.ts`, a separate file this task does not
|
||||
// touch). The two comment paragraphs above are kept for the column-shape
|
||||
// instead (JG15/19/72/88-102/108-116; the legacy GALLERY_CHRONOLOGICAL_ORDER
|
||||
// text survives only as the parity tests' oracle in
|
||||
// tests/helpers/legacy-gallery-order.ts, read by no production code).
|
||||
// The two comment paragraphs above are kept for the column-shape
|
||||
// documentation; the repositories' own docstrings are the source of truth
|
||||
// for the exact column lists now.
|
||||
|
||||
|
||||
+5
-1
@@ -1,5 +1,9 @@
|
||||
/**
|
||||
* How the journey gallery is ordered (#2200).
|
||||
* How the journey gallery is ordered (#2200), as the raw SQLite text the gallery
|
||||
* reads used before JourneyPhotosRepository.galleryRead rebuilt it in Kysely.
|
||||
* Kept here as the oracle the parity tests compare that rebuild against; no
|
||||
* production code reads it, and raw SQL has no place in src/nest any more.
|
||||
*
|
||||
*
|
||||
* `journey_photos.sort_order` is nothing but MAX+1 at insert time and no surface
|
||||
* ever writes it again, so ordering by it is ordering by "whenever this got
|
||||
@@ -1128,7 +1128,7 @@ describe('sql-functions (sqlite)', () => {
|
||||
});
|
||||
|
||||
// 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, ''), <correlated MIN+concat
|
||||
* subquery over journey_entry_photos/journey_entries>, <unixEpochToIsoKysely
|
||||
* of gp.created_at>) ASC, gp.sort_order ASC, gp.id ASC`. NOT exported from
|
||||
|
||||
@@ -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<unknown> };
|
||||
declare const qb: { execute(mode: string): Promise<unknown> };
|
||||
declare const repo: { run(sql: string, params?: unknown[]): Promise<unknown>; 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([]);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user