From 8efb3c8623258f7daf6c6af180b041001239c7eb Mon Sep 17 00:00:00 2001 From: jubnl Date: Wed, 23 Sep 2026 01:48:27 +0200 Subject: [PATCH] =?UTF-8?q?fix(server):=20plan=203b=20close-out=20?= =?UTF-8?q?=E2=80=94=20case=20folding=20on=20both=20sides=20for=20identifi?= =?UTF-8?q?er=20lookups=20(non-ASCII=20login=20regression);=20coverage=20r?= =?UTF-8?q?atchet=20restored;=20repository=20scaffold=20and=20instanceof?= =?UTF-8?q?=20ratchet=20on=20TrekRepository;=20D-shape=20tests=20load-bear?= =?UTF-8?q?ing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- server/scripts/generate-entities.ts | 11 +- server/src/db/dialect/sql-functions.ts | 47 ++++- .../repositories/OauthClients.repository.ts | 9 + .../repositories/OauthConsents.repository.ts | 9 + .../src/db/repositories/Users.repository.ts | 93 ++++++++-- .../db/repositories/_shared/owned-lookup.ts | 25 ++- .../repositories/_shared/trek-repository.ts | 78 ++++++++ server/src/nest/common/row-id.ts | 19 ++ server/src/nest/oauth/oauth.service.ts | 6 + server/src/nest/oidc/oidc.service.ts | 5 +- server/src/nest/realtime/trek-ws.adapter.ts | 18 +- server/tests/integration/auth.test.ts | 46 +++++ server/tests/integration/profile.test.ts | 28 +++ .../unit/db/dialect/sql-functions.test.ts | 47 ++++- server/tests/unit/db/entities-index.test.ts | 21 +++ .../tests/unit/db/generate-entities.test.ts | 20 ++ .../Categories.repository.test.ts | 12 ++ .../repositories/DayNotes.repository.test.ts | 12 +- .../db/repositories/Days.repository.test.ts | 11 +- .../InviteTokens.repository.test.ts | 38 ++++ .../repositories/McpTokens.repository.test.ts | 24 +++ .../OauthClients.repository.test.ts | 42 ++++- .../OauthTokens.repository.test.ts | 22 +++ .../db/repositories/Tags.repository.test.ts | 41 ++++- .../db/repositories/Users.repository.test.ts | 171 +++++++++++++++++- .../WebauthnCredentials.repository.test.ts | 50 +++-- .../db/repositories/trek-repository.test.ts | 89 +++++++++ .../unit/nest/categories.service.test.ts | 18 ++ server/tests/unit/nest/common/row-id.test.ts | 4 + server/tests/unit/nest/oidc.service.test.ts | 8 + server/tests/unit/nest/tags.service.test.ts | 18 ++ .../unit/nest/to-client-user.parity.test.ts | 87 +++++++++ .../tests/unit/nest/trek-ws-adapter.test.ts | 77 ++++++++ 33 files changed, 1149 insertions(+), 57 deletions(-) create mode 100644 server/tests/unit/nest/to-client-user.parity.test.ts diff --git a/server/scripts/generate-entities.ts b/server/scripts/generate-entities.ts index 9f78d7e09..8e09c7137 100644 --- a/server/scripts/generate-entities.ts +++ b/server/scripts/generate-entities.ts @@ -1737,10 +1737,17 @@ export function writeRepositoriesIfMissing(repositoriesDir: string, files: Reado const className = fileName.replace(/\.entity\.ts$/, ''); const repoFile = path.join(repositoriesDir, `${className}.repository.ts`); if (fs.existsSync(repoFile)) continue; + // `TrekRepository`, never the raw `EntityRepository` (program RULING, + // Plan 3b Task 1 — every repository validates the request context and + // defaults reads to `disableIdentityMap: true`; Task 7 review H2): a + // scaffold on the bare base class silently opts the next table out of + // both guarantees, with nothing to catch it. See + // `_shared/trek-repository.ts` and the `instanceof TrekRepository` + // ratchet in `tests/unit/db/entities-index.test.ts`. const content = `import type { ${className} } from '../entities/${className}.entity';\n` + - `import { EntityRepository } from '@mikro-orm/sql';\n\n` + - `export class ${className}Repository extends EntityRepository<${className}> {}\n`; + `import { TrekRepository } from './_shared/trek-repository';\n\n` + + `export class ${className}Repository extends TrekRepository<${className}> {}\n`; fs.writeFileSync(repoFile, content); created.push(repoFile); } diff --git a/server/src/db/dialect/sql-functions.ts b/server/src/db/dialect/sql-functions.ts index 983490cd2..3cbc954a0 100644 --- a/server/src/db/dialect/sql-functions.ts +++ b/server/src/db/dialect/sql-functions.ts @@ -77,16 +77,26 @@ export function columnIncrementedBy(platform: Platform, ref: string, amount: num /** * A case-insensitive column reference, for use as a filter's object key: - * `{ [lower(platform, 'email')]: value.toLowerCase() }` produces - * `WHERE LOWER(email) = ?`. This is the parity-preserving equivalent of the - * legacy `LOWER(col) = LOWER(?)` sites (`AuthService`/`OidcService`/ - * `UserProfileService`'s username/email CI lookups): the caller lowercases - * the bound value itself (every one of this migration's call sites already - * does, via `String.prototype.toLowerCase`), so the comparison is between - * two already-lowercased values either way — SQLite's `LOWER()` is ASCII-only - * exactly like `toLowerCase()` is for the identifier characters these - * columns hold (usernames/emails), so the two spellings agree on every - * input this codebase stores. + * `{ [lower(platform, 'email')]: }` produces + * `WHERE LOWER(email) = `. + * + * **SQLite's `LOWER()` is ASCII-only; JavaScript's `String.prototype + * .toLowerCase()` is full-Unicode.** They do NOT agree on every input this + * codebase stores — a locally-registered e-mail can carry any non-whitespace + * character (`EMAIL_REGEX`), including non-ASCII letters (program rule 18; + * Plan 3b Task 7 review, H1: `JOSÉ@x.com` proved a live login lockout). Two + * value-side shapes are legitimate, and they are NOT interchangeable: + * + * - `LOWER(col) = LOWER(?)` — pair with `lowerParam()` below, binding the + * RAW value. Both sides are folded by the SAME engine (SQLite's), so they + * agree on every input. This is the shape for every legacy + * `LOWER(col) = LOWER(?)` statement (AU9, AU12, UP5, UP6, O12). + * - `LOWER(col) = ?` — pair with a value the CALLER has already lowered in + * JS (`value.toLowerCase()`) as a plain string, matching a legacy + * `LOWER(col) = ?` statement exactly (only O4 today — + * `OidcService.findOrCreateUser` JS-lowers the email once upfront and the + * legacy statement it replaced never re-lowered it in SQL). Do not use + * this shape for a value the caller has not already lowered. * * Typed `RawQueryFragment & symbol` (not the bare `RawQueryFragment` the * other helpers in this file return): TypeScript only accepts a @@ -100,3 +110,20 @@ export function lower(platform: Platform, ref: string): RawQueryFragment & symbo return unsupported(platform); } +/** + * The value-side twin of `lower()`: `LOWER(?)` bound with the RAW, + * untransformed value, for the right-hand side of a `lower(platform, col)` + * filter key. Together they render `LOWER() = LOWER(?)`, + * byte-for-byte a legacy `LOWER(col) = LOWER(?)` statement, because SQLite + * folds BOTH sides with the same (ASCII-only) engine — proven directly + * against `better-sqlite3`: `SELECT LOWER(?)` on `'JOSÉ@x.com'` yields + * `'josÉ@x.com'` (only the ASCII letters fold; the `É` is untouched), the + * same transform SQLite would apply to a stored column. Do NOT pre-lower + * the value in JS before calling this (program rule 18) — that reintroduces + * the mixed-engine bug this helper exists to close. + */ +export function lowerParam(platform: Platform, value: string): RawQueryFragment { + if (platform instanceof SqlitePlatform) return raw('LOWER(?)', [value]); + return unsupported(platform); +} + diff --git a/server/src/db/repositories/OauthClients.repository.ts b/server/src/db/repositories/OauthClients.repository.ts index 938908bae..a90e39ed6 100644 --- a/server/src/db/repositories/OauthClients.repository.ts +++ b/server/src/db/repositories/OauthClients.repository.ts @@ -135,6 +135,15 @@ export class OauthClientsRepository extends TrekRepository { ); return rows.map((row) => ({ id: row.id as string, + // `row.user?.id`'s `?? null` fallback is unreachable THROUGH THIS + // METHOD specifically: the query already filters `{ user: userId }` + // on a concrete, non-null `userId`, so every returned row's `user` + // relation is populated by construction — `findPublicById` below is + // the id-scoped sibling that genuinely reads an anonymous (NULL + // `user_id`) client and exercises this same shape's null branch. + // Kept for type-shape symmetry with that sibling, not dead by + // mistake (the `countNonGuest`/`COALESCE(is_guest, 0)` precedent + // above is the same kind of defensive-but-unreachable fallback). user_id: row.user?.id ?? null, name: row.name, client_id: row.client_id, diff --git a/server/src/db/repositories/OauthConsents.repository.ts b/server/src/db/repositories/OauthConsents.repository.ts index ec8ed7064..9746eb5b0 100644 --- a/server/src/db/repositories/OauthConsents.repository.ts +++ b/server/src/db/repositories/OauthConsents.repository.ts @@ -93,6 +93,15 @@ export class OauthConsentsRepository extends TrekRepository { * Promise`); shadowing it would either fail to compile or silently * hide the base method, the same collision `Users.repository.ts::insertUser` * avoided with `create`. + * + * One more divergence from the legacy statement (Task 7 review, B-I1), + * unobservable and strictly better: the legacy `INSERT OR REPLACE` + * deletes-then-inserts, so a re-consent always assigned a NEW surrogate + * `id`; `em.upsert` with `onConflictAction: 'merge'` keeps the existing + * one. `oauth_consents` has only `id`/`client_id`/`user_id`/`scopes`/ + * `updated_at` and nothing reads `id`, so this is unobservable — and it + * avoids the FK churn a delete+insert would otherwise cause if anything + * ever did reference this row's surrogate key. */ async upsertGrant(clientId: string, userId: number, scopes: string): Promise { const platform = this.getEntityManager().getPlatform(); diff --git a/server/src/db/repositories/Users.repository.ts b/server/src/db/repositories/Users.repository.ts index 4915fed57..31159f280 100644 --- a/server/src/db/repositories/Users.repository.ts +++ b/server/src/db/repositories/Users.repository.ts @@ -1,6 +1,6 @@ import { Users } from '../entities/Users.entity'; import { toRow, type AssertRowKeys } from './_shared/rows'; -import { columnIncrementedBy, currentTimestamp, lower } from '../dialect/sql-functions'; +import { columnIncrementedBy, currentTimestamp, lower, lowerParam } from '../dialect/sql-functions'; import { TrekRepository } from './_shared/trek-repository'; /** @@ -278,12 +278,21 @@ export class UsersRepository extends TrekRepository { /** * `SELECT id FROM users WHERE (LOWER(email) = LOWER(?) OR LOWER(username) = LOWER(?)) * AND COALESCE(is_guest, 0) = 0` + * + * Both operands of each comparison are folded by SQLite's own `LOWER()` + * (`lowerParam` binds the RAW value) — never a JS-lowered bind. SQLite's + * `LOWER()` is ASCII-only; `String.prototype.toLowerCase()` is + * full-Unicode, and mixing them locks out any non-ASCII cased identifier + * (program rule 18; Plan 3b Task 7 review, H1). */ async findIdByEmailOrUsernameCI(email: string, username: string): Promise { const platform = this.getEntityManager().getPlatform(); const row = await this.findOne( { - $or: [{ [lower(platform, 'email')]: email.toLowerCase() }, { [lower(platform, 'username')]: username.toLowerCase() }], + $or: [ + { [lower(platform, 'email')]: lowerParam(platform, email) }, + { [lower(platform, 'username')]: lowerParam(platform, username) }, + ], is_guest: 0, }, { fields: ['id'] }, @@ -342,10 +351,39 @@ export class UsersRepository extends TrekRepository { // AU12 / O4 — email lookup (login + OIDC email-fallback) // --------------------------------------------------------------------- - /** `SELECT * FROM users WHERE LOWER(email) = LOWER(?) AND COALESCE(is_guest, 0) = 0` */ + /** + * `SELECT * FROM users WHERE LOWER(email) = LOWER(?) AND COALESCE(is_guest, 0) = 0` + * — AU9's sibling for AU12 (`AuthService.loginUser`, `auth.service.ts`). + * + * Both sides folded by SQLite's own `LOWER()` (`lowerParam` binds the + * RAW, untransformed email) — the legacy statement lowered BOTH operands + * in SQL, so a JS-lowered bind here would disagree with the column on + * every non-ASCII cased character (program rule 18; Plan 3b Task 7 + * review, H1: a `JOSÉ@x.com` account could register but never log back + * in). Distinct from `findByEmailLoweredBind` below, which mirrors O4's + * different legacy statement — do not collapse the two back into one + * method (D4). + */ async findByEmailCI(email: string): Promise { const platform = this.getEntityManager().getPlatform(); - const user = await this.findOne({ [lower(platform, 'email')]: email.toLowerCase(), is_guest: 0 }); + const user = await this.findOne({ [lower(platform, 'email')]: lowerParam(platform, email), is_guest: 0 }); + return user ? (toRow(user) as UserRow) : null; + } + + /** + * `SELECT * FROM users WHERE LOWER(email) = ? AND COALESCE(is_guest, 0) = 0` + * — O4 (`OidcService.findOrCreateUser`, `oidc.service.ts:655`), whose only + * caller JS-lowers the email itself (`oidc.service.ts:620`, + * `userInfo.email.trim().toLowerCase()`) before calling this method, + * exactly mirroring the legacy statement's `LOWER(email) = ?` (SQL + * `LOWER()` on the column, a bind the CALLER already lowered — not a + * second SQL `LOWER()` on the value side). Do not point a caller that has + * NOT already lowered its value at this method — use `findByEmailCI` + * above instead. + */ + async findByEmailLoweredBind(alreadyLoweredEmail: string): Promise { + const platform = this.getEntityManager().getPlatform(); + const user = await this.findOne({ [lower(platform, 'email')]: alreadyLoweredEmail, is_guest: 0 }); return user ? (toRow(user) as UserRow) : null; } @@ -595,7 +633,16 @@ export class UsersRepository extends TrekRepository { // PK15 — re-auth before passkey deletion // --------------------------------------------------------------------- - /** `SELECT password_hash FROM users WHERE id = ?` */ + /** + * `SELECT password_hash FROM users WHERE id = ?` + * + * The `?? null` fallback's own "row present but `password_hash` itself + * null" branch is unreachable: `password_hash TEXT NOT NULL` in the + * baseline schema (never relaxed by a later migration) — the same + * defensive-but-unreachable shape as `countNonGuest`'s `COALESCE(is_guest, + * 0)` above. The `?.`/`??` combination still earns its keep for the + * "row missing" case (`?.` alone would leave `undefined`, not `null`). + */ async getPasswordHash(id: number): Promise { const row = await this.findOne({ id }, { fields: ['password_hash'] }); return row?.password_hash ?? null; @@ -665,11 +712,14 @@ export class UsersRepository extends TrekRepository { * `SELECT id FROM users WHERE LOWER(username) = LOWER(?) AND id != ? * AND COALESCE(is_guest, 0) = 0` (UP5, `updateSettings`'s own-username * collision check). + * + * Both sides folded by SQLite's own `LOWER()` (`lowerParam` binds the RAW + * value) — program rule 18. */ async findIdByUsernameCI(username: string, excludeId: number): Promise { const platform = this.getEntityManager().getPlatform(); const row = await this.findOne( - { [lower(platform, 'username')]: username.toLowerCase(), id: { $ne: excludeId }, is_guest: 0 }, + { [lower(platform, 'username')]: lowerParam(platform, username), id: { $ne: excludeId }, is_guest: 0 }, { fields: ['id'] }, ); return row?.id ?? null; @@ -681,11 +731,17 @@ export class UsersRepository extends TrekRepository { * a genuinely different WHERE than UP5's, so a separate method per D4 * rather than an optional parameter that would silently change UP5's * shape. + * + * Both sides folded by SQLite's own `LOWER()` (`lowerParam` binds the RAW + * value) — program rule 18. The only caller sanitises the username to + * `[a-zA-Z0-9_.-]` before calling (`oidc.service.ts`), so this cannot + * diverge in practice today; fixed anyway so the guarantee lives in the + * method, not the caller. */ async findIdByUsernameCIAny(username: string): Promise { const platform = this.getEntityManager().getPlatform(); const row = await this.findOne( - { [lower(platform, 'username')]: username.toLowerCase() }, + { [lower(platform, 'username')]: lowerParam(platform, username) }, { fields: ['id'] }, ); return row?.id ?? null; @@ -771,11 +827,16 @@ export class UsersRepository extends TrekRepository { // UP6 — email collision check // --------------------------------------------------------------------- - /** `SELECT id FROM users WHERE LOWER(email) = LOWER(?) AND id != ? AND COALESCE(is_guest, 0) = 0` */ + /** + * `SELECT id FROM users WHERE LOWER(email) = LOWER(?) AND id != ? AND COALESCE(is_guest, 0) = 0` + * + * Both sides folded by SQLite's own `LOWER()` (`lowerParam` binds the RAW + * value) — program rule 18. + */ async findIdByEmailCI(email: string, excludeId: number): Promise { const platform = this.getEntityManager().getPlatform(); const row = await this.findOne( - { [lower(platform, 'email')]: email.toLowerCase(), id: { $ne: excludeId }, is_guest: 0 }, + { [lower(platform, 'email')]: lowerParam(platform, email), id: { $ne: excludeId }, is_guest: 0 }, { fields: ['id'] }, ); return row?.id ?? null; @@ -870,11 +931,15 @@ export class UsersRepository extends TrekRepository { * `DELETE FROM users WHERE id = ?` — `UserCleanupService.deleteUserCompletely`'s * final statement, the root of its 13-table transaction * (**security-sensitive**: account erasure). Named `deleteById`, not - * `remove`/`delete`: `EntityRepository#remove` is a real (persist-marking, - * needs-a-`flush()`) method on the base class — shadowing it would change - * its signature and behaviour; `deleteById` matches the naming this - * program already uses for a native, immediate delete - * (`McpTokensRepository.deleteById`). + * `remove`/`delete`: `remove` is `EntityManager`'s (persist-marking, + * needs-a-`flush()`), not `EntityRepository`'s — MikroORM 7.2.1's + * `EntityRepository` has no `remove` method at all (Task 7 review, L1; + * verified against the installed `.d.ts`, not assumed — that is why + * `OauthClientsRepository.remove`/`TagsRepository.remove`/ + * `CategoriesRepository.remove` compile as their own, unrelated methods). + * `deleteById` is chosen for naming consistency with this program's other + * native, immediate deletes (`McpTokensRepository.deleteById`), not to + * avoid a compiler conflict that does not exist. */ async deleteById(id: number): Promise { await this.nativeDelete({ id }); diff --git a/server/src/db/repositories/_shared/owned-lookup.ts b/server/src/db/repositories/_shared/owned-lookup.ts index 4a12bd783..bee215f0a 100644 --- a/server/src/db/repositories/_shared/owned-lookup.ts +++ b/server/src/db/repositories/_shared/owned-lookup.ts @@ -1,4 +1,5 @@ -import type { EntityRepository, FilterQuery, FilterValue, OrderDefinition } from '@mikro-orm/core'; +import type { FilterQuery, FilterValue, OrderDefinition } from '@mikro-orm/core'; +import type { TrekRepository } from './trek-repository'; /** * Shared lookup shapes for entities scoped to a user. @@ -20,6 +21,16 @@ import type { EntityRepository, FilterQuery, FilterValue, OrderDefinition } from * missing helper"). If a future entity needs that shape, recover it from * git history rather than reintroducing it speculatively. * + * Typed `repo: TrekRepository` (Task 7 review, A-L3/B-L4 — was the core + * `EntityRepository` until this task, the last two places in the + * repository tree still restating `disableIdentityMap: true` per call + * because the type couldn't see `TrekRepository`'s own validation and read + * default): at runtime the argument was always a `TrekRepository` (every + * concrete repository extends it), so the guard always fired — this closed + * a typing gap, not a safety gap. The two per-call `disableIdentityMap: + * true` literals below are gone now that the base class's `find`/`findOne` + * overrides supply the same default. + * * These helpers return live MikroORM entities, not rows — unlike every other * repository method in this codebase (D4: "repositories return rows, never a * live entity"). They are an internal building block for `TagsRepository`, @@ -37,8 +48,9 @@ import type { EntityRepository, FilterQuery, FilterValue, OrderDefinition } from * listForOwner(tags, 'user', userId, 'name'); * findOwnedByUser(tags, id, 'user', userId); * - * Both helpers pass `disableIdentityMap: true` (Plan 3b Task 1 fix round, - * supersedes Plan 3a's I1 "`refresh: true` on every PK-only `findOne`" — see + * Both helpers get `disableIdentityMap: true` from `TrekRepository`'s own + * `find`/`findOne` default now (Plan 3b Task 1 fix round, supersedes Plan + * 3a's I1 "`refresh: true` on every PK-only `findOne`" — see * `Users.repository.ts`'s class-level docstring and * `.superpowers/sdd/2026-09-22-orm-phase3b/task-1-review.md` B1): both are * "rows out" reads whose entity is converted via `toRow` and discarded, never @@ -63,14 +75,13 @@ import type { EntityRepository, FilterQuery, FilterValue, OrderDefinition } from * `SELECT * FROM WHERE = ? ORDER BY ASC`. */ export async function listForOwner( - repo: EntityRepository, + repo: TrekRepository, ownerField: K, ownerId: FilterValue, orderField: OF, ): Promise { return repo.find({ [ownerField]: ownerId } as FilterQuery, { orderBy: { [orderField]: 'asc' } as OrderDefinition, - disableIdentityMap: true, }); } @@ -83,10 +94,10 @@ export async function listForOwner( - repo: EntityRepository, + repo: TrekRepository, id: T['id'], ownerField: K, ownerId: FilterValue, ): Promise { - return repo.findOne({ id, [ownerField]: ownerId } as FilterQuery, { disableIdentityMap: true }); + return repo.findOne({ id, [ownerField]: ownerId } as FilterQuery); } diff --git a/server/src/db/repositories/_shared/trek-repository.ts b/server/src/db/repositories/_shared/trek-repository.ts index 2df2670f7..6caba40a4 100644 --- a/server/src/db/repositories/_shared/trek-repository.ts +++ b/server/src/db/repositories/_shared/trek-repository.ts @@ -1,11 +1,17 @@ import { EntityRepository, + type CountByOptions, type CountOptions, + type Cursor, type DeleteOptions, + type Dictionary, type EntityData, + type EntityKey, type FilterQuery, type FindAllOptions, + type FindByCursorOptions, type FindOneOptions, + type FindOneOrFailOptions, type FindOptions, type GetKyselyOptions, type Loaded, @@ -14,7 +20,9 @@ import { type QueryBuilder, type RequiredEntityData, type SqlEntityManager, + type StreamOptions, type UpdateOptions, + type UpsertManyOptions, type UpsertOptions, type WithUsingOptions, } from '@mikro-orm/sql'; @@ -157,6 +165,76 @@ export abstract class TrekRepository extends EntityReposi return super.count(where, options); } + /** + * Task 7 review, M4: `TrekRepository` left seven DB-reaching base-class + * methods unoverridden (none used anywhere in this program today — + * `grep -rnE "\.(countBy|findAndCount|findByCursor|findOneOrFail| + * insertMany|upsertMany|stream)\(" src/db/repositories/` was 0 hits — but + * the FIRST one used would silently lose both guarantees: no + * `validateRequestContext()`, and — for the read-shaped ones — no + * `disableIdentityMap: true` default). Same shape as `findOne`/`find`/ + * `findAll` above: `Using` dropped for the same override-compatibility + * reason documented on `findOne`. + */ + override async findOneOrFail( + where: FilterQuery, + options?: FindOneOrFailOptions, + ): Promise> { + this.validateRequestContext(); + const disableIdentityMap = options?.disableIdentityMap ?? true; + return super.findOneOrFail(where, { ...options, disableIdentityMap }); + } + + override async findAndCount( + where: FilterQuery, + options?: FindOptions, + ): Promise<[Loaded[], number]> { + this.validateRequestContext(); + const disableIdentityMap = options?.disableIdentityMap ?? true; + return super.findAndCount(where, { ...options, disableIdentityMap }); + } + + override async findByCursor< + Hint extends string = never, + Fields extends string = never, + Excludes extends string = never, + IncludeCount extends boolean = true, + Using extends string = never, + >( + options: WithUsingOptions, Entity, Using>, + ): Promise> { + this.validateRequestContext(); + const disableIdentityMap = options?.disableIdentityMap ?? true; + return super.findByCursor({ ...options, disableIdentityMap }); + } + + /** Grouping counts don't hydrate an entity — no identity map to isolate — so this only needs the fail-closed guard, matching `count` above. */ + override async countBy(groupBy: EntityKey | readonly EntityKey[], options?: CountByOptions): Promise> { + this.validateRequestContext(); + return super.countBy(groupBy, options); + } + + override stream( + options?: WithUsingOptions, Entity, Using>, + ): AsyncIterableIterator> { + this.validateRequestContext(); + const disableIdentityMap = options?.disableIdentityMap ?? true; + return super.stream({ ...options, disableIdentityMap }); + } + + override async insertMany(data: Entity[] | RequiredEntityData[], options?: NativeInsertUpdateOptions): Promise[]> { + this.validateRequestContext(); + return super.insertMany(data, options); + } + + override async upsertMany( + entitiesOrData?: EntityData[] | Entity[], + options?: UpsertManyOptions, + ): Promise { + this.validateRequestContext(); + return super.upsertMany(entitiesOrData, options); + } + override async nativeUpdate(where: FilterQuery, data: EntityData, options?: UpdateOptions): Promise { this.validateRequestContext(); return super.nativeUpdate(where, data, options); diff --git a/server/src/nest/common/row-id.ts b/server/src/nest/common/row-id.ts index 9980e3534..95cf21d1d 100644 --- a/server/src/nest/common/row-id.ts +++ b/server/src/nest/common/row-id.ts @@ -62,6 +62,25 @@ * not "parity with the legacy". Check which kind of seam a route's legacy * handler was (`git show :` on the actual statement, not the * route) before writing that test. + * + * **A THIRD shape narrows nothing at all: a PRE-COERCED `Number()` seam** + * (Task 7 review, M2/A-M2). The two `Number()`-seam routes above still hand + * `toRowId` the RAW STRING — the legacy code's own `Number(id)` conversion + * moved to a repository-facing call site, so `toRowId` sees what the legacy + * `Number()` used to see and narrows exactly as described. But on some + * routes the legacy's `Number(id)` conversion itself survives UPSTREAM of + * `toRowId`, and `toRowId` receives an already-coerced `number`, not the + * original string — `oauth-api.controller.ts::revokeSession`'s `DELETE + * /api/oauth/sessions/:id` is that shape on both the legacy and the + * converted tree: `Number(id)` runs first, `toRowId(Number(id))` only + * checks `Number.isSafeInteger`, and the legacy statement never matched a + * non-safe-integer either — so `'0x10'`, `'1e1'`, `'16.0'` and `' 16'` all + * reach the SAME row on both trees (verified live, Task 7 review security + * report). There is no narrowing to name here, and no regression test + * should claim one for this shape — record it as "the pre-coerced + * `Number()` seam: full parity" and move on, the same way `passkey.service + * .ts::adminResetPasskeys` (`:502-507`) already documents its own copy of + * this exact shape. */ export function toRowId(value: unknown): number | null { if (typeof value === 'number') { diff --git a/server/src/nest/oauth/oauth.service.ts b/server/src/nest/oauth/oauth.service.ts index 5d67de26f..190b01c9b 100644 --- a/server/src/nest/oauth/oauth.service.ts +++ b/server/src/nest/oauth/oauth.service.ts @@ -548,6 +548,12 @@ export class OauthService { sessionId: number, ip?: string | null, ): Promise<{ error?: string; status?: number; success?: boolean }> { + // `sessionId` is already a `number` here — `oauth-api.controller.ts`'s + // `DELETE /api/oauth/sessions/:id` calls `Number(id)` before this method + // (byte-identical to the legacy route), so `toRowId` only guards against + // `NaN`/a non-safe-integer, never narrows a string shape (Task 7 review, + // B-I2/A-M2's "pre-coerced Number() seam" — see `row-id.ts`'s own + // docstring). This is full parity, not the accepted-narrowing shape. const id = toRowId(sessionId); if (id === null) return { error: 'Session not found', status: 404 }; diff --git a/server/src/nest/oidc/oidc.service.ts b/server/src/nest/oidc/oidc.service.ts index 3e930508d..afb25bb6e 100644 --- a/server/src/nest/oidc/oidc.service.ts +++ b/server/src/nest/oidc/oidc.service.ts @@ -652,7 +652,10 @@ export class OidcService implements OnModuleDestroy { let user = await this.usersRepo.findByOidcIdentity(sub, config.issuer); if (!user) { // Never link/log-in to a guest (#1362) via its synthetic email. - user = await this.usersRepo.findByEmailCI(email); + // `email` is already JS-lowered above (O4 shape) — findByEmailLoweredBind + // mirrors the legacy `LOWER(email) = ?` statement exactly; do not use + // findByEmailCI here, it is AU12's `LOWER(email) = LOWER(?)` shape. + user = await this.usersRepo.findByEmailLoweredBind(email); } if (user) { diff --git a/server/src/nest/realtime/trek-ws.adapter.ts b/server/src/nest/realtime/trek-ws.adapter.ts index 14cd87ee6..5434860e2 100644 --- a/server/src/nest/realtime/trek-ws.adapter.ts +++ b/server/src/nest/realtime/trek-ws.adapter.ts @@ -189,7 +189,16 @@ export class TrekWsAdapter extends WsAdapter { // The request rides along: the handshake reads the ws token off its query // string, so dropping it here would leave handleConnection with nothing // to authenticate. - withRequestContext(this.orm, () => callback(socket, request)); + // + // `handleConnection` is async and this listener is not — a rejection + // inside it (or inside `withRequestContext` itself) would otherwise be + // an unhandled rejection (T1-F7b; `applyPlatformUploads`'s `.catch(next)` + // in `platform.routes.ts` is the sibling pattern for the pre-init HTTP + // path). Logged through the adapter's own error path, not thrown — + // there is no request/response here to propagate a failure to. + Promise.resolve(withRequestContext(this.orm, () => callback(socket, request))).catch((err: unknown) => + logError(`ws connection handler error: ${err instanceof Error ? err.message : String(err)}`), + ); }); } @@ -209,7 +218,12 @@ export class TrekWsAdapter extends WsAdapter { if (!this.orm) { throw new Error('TrekWsAdapter: no MikroORM available to build a request context for this disconnect'); } - withRequestContext(this.orm, () => callback(...args)); + // Same T1-F7b shape as bindClientConnect above — handleDisconnect is + // async and this listener is not, so an unhandled rejection here would + // otherwise be silent. + Promise.resolve(withRequestContext(this.orm, () => callback(...args))).catch((err: unknown) => + logError(`ws disconnect handler error: ${err instanceof Error ? err.message : String(err)}`), + ); }); } diff --git a/server/tests/integration/auth.test.ts b/server/tests/integration/auth.test.ts index a1a095e44..9ce40f9dc 100644 --- a/server/tests/integration/auth.test.ts +++ b/server/tests/integration/auth.test.ts @@ -257,6 +257,52 @@ describe('Registration — whitespace normalization', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// Registration / login — non-ASCII case folding (program rule 18, Plan 3b +// Task 7 review H1: register('JOSÉ@x.com') then login with the exact stored +// spelling used to 401, and a duplicate registration used to 500 instead of +// 409, because the repository lowered the column with SQLite's ASCII-only +// LOWER() and the bound value with JS's full-Unicode toLowerCase(). +// ───────────────────────────────────────────────────────────────────────────── + +describe('Registration / login — non-ASCII case folding (H1)', () => { + it('AUTH-NONASCII-1 — register a non-ASCII email, then log in with the exact stored spelling: 201, then 200', async () => { + const register = await request(app).post('/api/auth/register').send({ + username: 'joseuser', + email: 'JOSÉ@x.com', + password: 'Str0ng!Pass', + }); + expect(register.status).toBe(201); + + const login = await request(app).post('/api/auth/login').send({ email: 'JOSÉ@x.com', password: 'Str0ng!Pass' }); + expect(login.status).toBe(200); + expect(login.body.user.email).toBe('JOSÉ@x.com'); + }); + + it('AUTH-NONASCII-2 — registering the same non-ASCII email again returns 409, not 500', async () => { + const first = await request(app).post('/api/auth/register').send({ + username: 'joseuser2', + email: 'JOSÉ2@x.com', + password: 'Str0ng!Pass', + }); + expect(first.status).toBe(201); + + const second = await request(app).post('/api/auth/register').send({ + username: 'differentname', + email: 'JOSÉ2@x.com', + password: 'Str0ng!Pass', + }); + expect(second.status).toBe(409); + expect(second.body.error).toBeDefined(); + }); + + it('AUTH-NONASCII-3 — an ASCII-only account is unaffected (control)', async () => { + const { user, password } = createUser(testDb, { email: 'ROOT@example.com' }); + const login = await request(app).post('/api/auth/login').send({ email: user.email, password }); + expect(login.status).toBe(200); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // Session / Me // ───────────────────────────────────────────────────────────────────────────── diff --git a/server/tests/integration/profile.test.ts b/server/tests/integration/profile.test.ts index 56cc85a94..45bf7e243 100644 --- a/server/tests/integration/profile.test.ts +++ b/server/tests/integration/profile.test.ts @@ -100,6 +100,34 @@ describe('PUT /api/auth/me/settings (F3)', () => { expect(row.username).toBe('after-name'); expect(row.email).toBe('after@example.test'); }); + + // Program rule 18 / Plan 3b Task 7 review H1 — UP6's own-rename collision + // check (findIdByEmailCI) must fold both sides of a non-ASCII identifier + // with the same engine. (UP5's username collision check, findIdByUsernameCI, + // is exercised at the repository level — USERSREPO-044b — since the + // username field itself is ASCII-only by the service's own validation + // regex, `^[a-zA-Z0-9_.-]+$`, so a non-ASCII collision can never reach + // this route.) + it('PROFILE-016b — renaming to a non-ASCII email that collides with ANOTHER user still 409s; renaming to one\'s own exact non-ASCII spelling succeeds', async () => { + createUser(testDb, { email: 'JOSÉ-OTHER@x.com' }); + const { user } = createUser(testDb, { email: 'plain-self@example.test' }); + + // Differs from the stored spelling only in ASCII-letter case (the accented + // 'É' is kept as-is — SQLite's LOWER() never touches it): SQLite's own + // LOWER() folds both to the same string, so this must still collide. + const collideEmail = await request(app) + .put('/api/auth/me/settings') + .set('Cookie', authCookie(user.id)) + .send({ email: 'JOSÉ-OTHER@X.COM' }); + expect(collideEmail.status).toBe(409); + + // Renaming self to a non-ASCII spelling that collides with nobody else succeeds. + const ownRename = await request(app) + .put('/api/auth/me/settings') + .set('Cookie', authCookie(user.id)) + .send({ email: 'JOSÉ-SELF@x.com' }); + expect(ownRename.status).toBe(200); + }); }); describe('Avatar', () => { diff --git a/server/tests/unit/db/dialect/sql-functions.test.ts b/server/tests/unit/db/dialect/sql-functions.test.ts index e8e6f8413..536ae2ed9 100644 --- a/server/tests/unit/db/dialect/sql-functions.test.ts +++ b/server/tests/unit/db/dialect/sql-functions.test.ts @@ -5,7 +5,7 @@ import { resetTestDb } from '../../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; import { createUser } from '../../../helpers/factories'; import { Users } from '../../../../src/db/entities/Users.entity'; -import { columnIncrementedBy, columnRef, currentTimestamp, dateAdd, dateOf, lower } from '../../../../src/db/dialect/sql-functions'; +import { columnIncrementedBy, columnRef, currentTimestamp, dateAdd, dateOf, lower, lowerParam } from '../../../../src/db/dialect/sql-functions'; const testDb = createSnapshotTestDb(); let t: TestOrm; @@ -74,6 +74,7 @@ describe('sql-functions (sqlite)', () => { expect(() => columnRef(foreign, 'u.max_uses')).toThrow(/no implementation for platform FakePlatform/); expect(() => columnIncrementedBy(foreign, 'u.used_count', 1)).toThrow(/no implementation for platform FakePlatform/); expect(() => lower(foreign, 'u.email')).toThrow(/no implementation for platform FakePlatform/); + expect(() => lowerParam(foreign, 'x')).toThrow(/no implementation for platform FakePlatform/); }); // Plan 3b Task 1: UsersRepository's CI (case-insensitive) lookups @@ -174,4 +175,48 @@ describe('sql-functions (sqlite)', () => { const stored = testDb.prepare('SELECT created_at FROM users WHERE id = ?').get(user.id) as { created_at: string }; expect(stored.created_at).toMatch(/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/); }); + + // Program rule 18 / Plan 3b Task 7 review H1: SQLite's LOWER() is + // ASCII-only; JavaScript's toLowerCase() is full-Unicode. lowerParam() + // binds the RAW value so SQLite folds both sides of the comparison with + // the same engine, matching a legacy `LOWER(col) = LOWER(?)` statement. + it('SQLF-014: SQLite LOWER() is ASCII-only, unlike JS toLowerCase() — verified directly', () => { + const cases: Array<[string, string]> = [ + ['JOSÉ@x.com', 'josÉ@x.com'], + ['ÄNNA@x.com', 'Änna@x.com'], + ['ΣIGMA@x.com', 'Σigma@x.com'], + ['İSTANBUL@x.com', 'İstanbul@x.com'], + ['ASCII@x.com', 'ascii@x.com'], + ]; + for (const [input, sqliteLowered] of cases) { + const row = testDb.prepare('SELECT LOWER(?) as l').get(input) as { l: string }; + expect(row.l).toBe(sqliteLowered); + // Every non-ASCII case proves the two engines disagree. + if (input !== 'ASCII@x.com') { + expect(row.l).not.toBe(input.toLowerCase()); + } + } + }); + + it('SQLF-015: lowerParam pairs with lower() to fold BOTH sides in SQLite, matching a non-ASCII stored spelling that toLowerCase() would miss', async () => { + const { user } = createUser(testDb, { email: 'JOSÉ@x.com' }); + const platform = t.em.getPlatform(); + + // The exact stored spelling matches (SQLite's LOWER() is a no-op on both + // sides for this input, so this is really an equality check — the real + // proof is the next assertion). + const exact = await t.em.findOne(Users, { [lower(platform, 'email')]: lowerParam(platform, 'JOSÉ@x.com') }); + expect(exact?.id).toBe(user.id); + + // A JS-lowered value-side bind (the bug: mixing engines) must NOT match + // — this is the mutation-proof: reverting `lowerParam(platform, email)` + // to a plain `email.toLowerCase()` bind makes this assertion fail + // because SQLite's LOWER(column) still reads 'josÉ@x.com', which does + // not equal the JS-lowered bind 'josé@x.com'. + const jsLowered = await t.em.findOne(Users, { [lower(platform, 'email')]: 'josé@x.com' }); + expect(jsLowered).toBeNull(); + + const none = await t.em.findOne(Users, { [lower(platform, 'email')]: lowerParam(platform, 'nobody@example.com') }); + expect(none).toBeNull(); + }); }); diff --git a/server/tests/unit/db/entities-index.test.ts b/server/tests/unit/db/entities-index.test.ts index f90656258..b46c1a958 100644 --- a/server/tests/unit/db/entities-index.test.ts +++ b/server/tests/unit/db/entities-index.test.ts @@ -5,6 +5,7 @@ import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { ALL_ENTITIES } from '../../../src/db/entities'; import { createSnapshotTestDb } from '../../helpers/db-mock'; import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; +import { TrekRepository } from '../../../src/db/repositories/_shared/trek-repository'; const ENTITIES_DIR = path.join(__dirname, '../../../src/db/entities'); const REPOSITORIES_DIR = path.join(__dirname, '../../../src/db/repositories'); @@ -80,8 +81,28 @@ describe('ALL_ENTITIES', () => { if (!(repo instanceof (RepoClass as abstract new (...args: never[]) => unknown))) { failures.push(`${className}: em.getRepository() did not return a ${className}Repository instance`); } + // Task 7 review, H2 carry-list item 1: the `extends EntityRepository` + // ratchet made self-enforcing — every bound repository must also be + // a TrekRepository (validateRequestContext + the disableIdentityMap + // read default), not just an instance of its own named class. + if (!(repo instanceof TrekRepository)) { + failures.push(`${className}: em.getRepository() did not return a TrekRepository instance`); + } } expect(failures).toEqual([]); }); + + // Task 7 review, H2: a source-scan companion to the instanceof check + // above — belt and suspenders against a repository that extends + // TrekRepository indirectly (or not through this loop's import path). + it('SCAN-001: exactly one repository file extends EntityRepository directly — the base class itself', () => { + const files = fs.readdirSync(REPOSITORIES_DIR).filter((f) => f.endsWith('.repository.ts')); + const hits: string[] = []; + for (const file of files) { + const content = fs.readFileSync(path.join(REPOSITORIES_DIR, file), 'utf-8'); + if (/\bextends\s+EntityRepository\b/.test(content)) hits.push(file); + } + expect(hits).toEqual([]); // the base class lives in _shared/, not matched by *.repository.ts + }); }); }); diff --git a/server/tests/unit/db/generate-entities.test.ts b/server/tests/unit/db/generate-entities.test.ts index 1b013e1fd..a9633114f 100644 --- a/server/tests/unit/db/generate-entities.test.ts +++ b/server/tests/unit/db/generate-entities.test.ts @@ -45,6 +45,7 @@ import { regenerateEntitiesIndex, stripHiddenTypeAnnotation, stripRedundantColumnType, + writeRepositoriesIfMissing, type DefaultFixup, } from '../../../scripts/generate-entities'; @@ -1046,6 +1047,25 @@ describe('checkEntities', () => { fs.rmSync(repoDir, { recursive: true, force: true }); } }); + + // Plan 3b Task 7 review, H2: the generator's scaffold for a NEW repository + // must extend TrekRepository, never the bare EntityRepository — the base + // class is what wires validateRequestContext() and the + // disableIdentityMap:true read default; a repository scaffolded on the + // raw base silently opts out of both, with nothing else to catch it. + it('SCAFFOLD-001: writeRepositoriesIfMissing scaffolds a NEW repository on TrekRepository, not the bare EntityRepository', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gen-entities-scaffold-')); + try { + const created = writeRepositoriesIfMissing(repoDir, new Map([['Widget.entity.ts', 'content']])); + expect(created).toEqual([path.join(repoDir, 'Widget.repository.ts')]); + const content = fs.readFileSync(path.join(repoDir, 'Widget.repository.ts'), 'utf-8'); + expect(content).toContain("import { TrekRepository } from './_shared/trek-repository';"); + expect(content).toContain('export class WidgetRepository extends TrekRepository {}'); + expect(content).not.toContain('EntityRepository'); + } finally { + fs.rmSync(repoDir, { recursive: true, force: true }); + } + }); }); describe('assertFilesGenerated', () => { diff --git a/server/tests/unit/db/repositories/Categories.repository.test.ts b/server/tests/unit/db/repositories/Categories.repository.test.ts index 969b47e4a..a707a472b 100644 --- a/server/tests/unit/db/repositories/Categories.repository.test.ts +++ b/server/tests/unit/db/repositories/Categories.repository.test.ts @@ -121,6 +121,14 @@ describe('CategoriesRepository', () => { expect(row.color).toBe('literal-color'); expect(row.icon).toBe('literal-icon'); }); + + it('CATREPO-010b: throws when the read-back after insert finds no row (coverage: the guard branch)', async () => { + const { user } = createUser(testDb); + const spy = vi.spyOn(categories, 'findById').mockResolvedValueOnce(null); + await expect(categories.createCategory({ name: 'Ghost', color: '#000000', icon: '👻', user_id: user.id })) + .rejects.toThrow('createCategory: read-back after insert found no row'); + spy.mockRestore(); + }); }); describe('patch', () => { @@ -152,6 +160,10 @@ describe('CategoriesRepository', () => { expect(await categories.patch(99999999, { name: 'Nope' })).toBeNull(); }); + it('CATREPO-014b: an empty changes object against a non-existent id returns null via the existence-check branch (coverage)', async () => { + expect(await categories.patch(99999999, {})).toBeNull(); + }); + // Task 3 review, Important 1, consequence (a) — and Minor 1: the // identity map is populated through `list()` here, not `findById`, so // this doubles as the "list → raw UPDATE → patch" case the review diff --git a/server/tests/unit/db/repositories/DayNotes.repository.test.ts b/server/tests/unit/db/repositories/DayNotes.repository.test.ts index 4b1d9d89a..bf7b168e8 100644 --- a/server/tests/unit/db/repositories/DayNotes.repository.test.ts +++ b/server/tests/unit/db/repositories/DayNotes.repository.test.ts @@ -1,4 +1,4 @@ -import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; import { createSnapshotTestDb } from '../../../helpers/db-mock'; import { resetTestDb } from '../../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; @@ -48,4 +48,14 @@ describe('DayNotesRepository timestamps', () => { const d = testDb.prepare('SELECT date(created_at) AS d FROM day_notes WHERE id = ?').get(row.id) as { d: string }; expect(d.d).toMatch(/^\d{4}-\d{2}-\d{2}$/); }); + + it('NOTEREPO-004: throws when the read-back after insert finds no row (coverage: the guard branch)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const spy = vi.spyOn(notes, 'findOne').mockResolvedValueOnce(null); + await expect(notes.createNote({ day_id: day.id, trip_id: trip.id, text: 'Ghost', time: null, icon: null, sort_order: 0, color: null })) + .rejects.toThrow('createNote: read-back after insert found no row'); + spy.mockRestore(); + }); }); diff --git a/server/tests/unit/db/repositories/Days.repository.test.ts b/server/tests/unit/db/repositories/Days.repository.test.ts index 6e1a855d3..16f081dc2 100644 --- a/server/tests/unit/db/repositories/Days.repository.test.ts +++ b/server/tests/unit/db/repositories/Days.repository.test.ts @@ -1,4 +1,4 @@ -import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; import { createSnapshotTestDb } from '../../../helpers/db-mock'; import { resetTestDb } from '../../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; @@ -76,4 +76,13 @@ describe('DaysRepository', () => { expect(row.notes).toBeNull(); expect(row.trip_id).toBe(trip.id); }); + + it('DAYREPO-006b: throws when the read-back after insert finds no row (coverage: the guard branch)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const spy = vi.spyOn(days, 'findOne').mockResolvedValueOnce(null); + await expect(days.createDay({ trip_id: trip.id, day_number: 1, date: null, notes: null })) + .rejects.toThrow('createDay: read-back after insert found no row'); + spy.mockRestore(); + }); }); diff --git a/server/tests/unit/db/repositories/InviteTokens.repository.test.ts b/server/tests/unit/db/repositories/InviteTokens.repository.test.ts index 00e0ce392..822a2e3a0 100644 --- a/server/tests/unit/db/repositories/InviteTokens.repository.test.ts +++ b/server/tests/unit/db/repositories/InviteTokens.repository.test.ts @@ -5,6 +5,8 @@ import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; import { createInviteToken, createTrip, createUser } from '../../../helpers/factories'; import { InviteTokens } from '../../../../src/db/entities/InviteTokens.entity'; import type { InviteTokensRepository } from '../../../../src/db/repositories/InviteTokens.repository'; +import { UnitOfWork } from '../../../../src/nest/database/unit-of-work'; +import { withRequestContext } from '../../../../src/nest/database/request-context'; const testDb = createSnapshotTestDb(); let t: TestOrm; @@ -200,4 +202,40 @@ describe('InviteTokensRepository', () => { expect(rawInvite(keep.id)).toBeDefined(); }); }); + + // Task 7 review, M2: InviteTokensRepository had no D-shape identity-map + // test at all, although `findByToken`/`insertInvite`'s own docstrings + // both cite the disableIdentityMap-not-refresh ruling. The shape: wide + // findByToken (carries used_count) first, narrow findIdById (`fields: + // ['id']`, no used_count) second, then a nativeUpdate-equivalent write + // (incrementUsedCount, via the QueryBuilder) inside uow.transactional — + // this repository is a Plan-3b-created one on a public, unauthenticated + // route (GET /api/auth/invite/:token) that also runs inside + // registerUser's transaction. + describe('identity-map isolation (D-shape, program RULING)', () => { + it('INVREPO-017: incrementUsedCount inside uow.transactional is not discarded by a stale entity read earlier under a different projection', async () => { + const { user: admin } = createUser(testDb, { username: 'invrepo-017-admin' }); + const invite = createInviteToken(testDb, { token: 'invrepo-017', max_uses: 5, created_by: admin.id }); + const uow = new UnitOfWork(t.em); + + await withRequestContext(t.orm, async () => { + // Projection A — wide, full row, carries used_count. + const before = await invites.findByToken('invrepo-017'); + expect(before!.used_count).toBe(0); + // Projection B — narrow, `fields: ['id']`, no used_count. + await invites.findIdById(invite.id); + + await uow.transactional(async () => { + await invites.incrementUsedCount('invrepo-017'); + }); + }); + + // A column only the wide read carried (`max_uses`) must be unchanged, + // and the write itself must have stuck. + const after = await invites.findByToken('invrepo-017'); + expect(after!.used_count).toBe(1); + expect(after!.max_uses).toBe(5); + expect((rawInvite(invite.id) as { used_count: number }).used_count).toBe(1); + }); + }); }); diff --git a/server/tests/unit/db/repositories/McpTokens.repository.test.ts b/server/tests/unit/db/repositories/McpTokens.repository.test.ts index 11306a6b9..826d946b9 100644 --- a/server/tests/unit/db/repositories/McpTokens.repository.test.ts +++ b/server/tests/unit/db/repositories/McpTokens.repository.test.ts @@ -52,6 +52,14 @@ describe('McpTokensRepository', () => { const { user } = createUser(testDb); expect(await tokens.listByUserAndKind(user.id, 'api')).toEqual([]); }); + + it('MCPTOKREPO-002b: created_at NULL comes back null, not undefined (coverage: rule 16)', async () => { + const { user } = createUser(testDb); + const token = createMcpToken(testDb, user.id, { name: 'null-created', kind: 'mcp' }); + testDb.prepare('UPDATE mcp_tokens SET created_at = NULL WHERE id = ?').run(token.id); + const [row] = await tokens.listByUserAndKind(user.id, 'mcp'); + expect(row.created_at).toBeNull(); + }); }); describe('countByUserAndKind', () => { @@ -165,6 +173,14 @@ describe('McpTokensRepository', () => { expect(await tokens.findBasic(999999)).toBeNull(); }); + it('MCPTOKREPO-007c: created_at NULL comes back null, not undefined (coverage: rule 16)', async () => { + const { user } = createUser(testDb); + const created = createMcpToken(testDb, user.id, { name: 'null-created-basic' }); + testDb.prepare('UPDATE mcp_tokens SET created_at = NULL WHERE id = ?').run(created.id); + const row = await tokens.findBasic(created.id); + expect(row?.created_at).toBeNull(); + }); + it('MCPTOKREPO-007b: identity-map regression — sees a raw UPDATE on the same id in the same request', async () => { const { user } = createUser(testDb); const created = createMcpToken(testDb, user.id, { name: 'before' }); @@ -234,6 +250,14 @@ describe('McpTokensRepository', () => { const rows = await tokens.listAllWithUsername(); expect(rows.every((r) => !('token_hash' in r))).toBe(true); }); + + it('MCPTOKREPO-012c: created_at NULL comes back null, not undefined (coverage: rule 16)', async () => { + const { user } = createUser(testDb, { username: 'null-created-user' }); + const created = createMcpToken(testDb, user.id, { name: 'null-created-list' }); + testDb.prepare('UPDATE mcp_tokens SET created_at = NULL WHERE id = ?').run(created.id); + const rows = await tokens.listAllWithUsername(); + expect(rows.find((r) => r.id === created.id)?.created_at).toBeNull(); + }); }); describe('findGrantByHash', () => { diff --git a/server/tests/unit/db/repositories/OauthClients.repository.test.ts b/server/tests/unit/db/repositories/OauthClients.repository.test.ts index bba4f8723..1f3a46155 100644 --- a/server/tests/unit/db/repositories/OauthClients.repository.test.ts +++ b/server/tests/unit/db/repositories/OauthClients.repository.test.ts @@ -1,4 +1,4 @@ -import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; import { createSnapshotTestDb } from '../../../helpers/db-mock'; import { resetTestDb } from '../../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; @@ -65,6 +65,14 @@ describe('OauthClientsRepository', () => { const { user } = createUser(testDb); expect(await clients.listByUser(user.id)).toEqual([]); }); + + it('OAUTHCLIENTREPO-002b: created_at NULL comes back null, not undefined (coverage: rule 16)', async () => { + const { user } = createUser(testDb); + const { id } = seedClient({ userId: user.id, name: 'null-created' }); + testDb.prepare('UPDATE oauth_clients SET created_at = NULL WHERE id = ?').run(id); + const rows = await clients.listByUser(user.id); + expect(rows[0].created_at).toBeNull(); + }); }); describe('countByUser / countAnonymous (OA2/OA3)', () => { @@ -117,6 +125,17 @@ describe('OauthClientsRepository', () => { expect(raw.client_secret_hash).toBe('secrethash'); }); + it('OAUTHCLIENTREPO-005b: findPublicById is null for an unknown id', async () => { + expect(await clients.findPublicById('nope')).toBeNull(); + }); + + it('OAUTHCLIENTREPO-005c: findPublicById — created_at NULL comes back null, not undefined (coverage: rule 16)', async () => { + const { id } = seedClient({ name: 'null-created-public' }); + testDb.prepare('UPDATE oauth_clients SET created_at = NULL WHERE id = ?').run(id); + const row = await clients.findPublicById(id); + expect(row?.created_at).toBeNull(); + }); + it('OAUTHCLIENTREPO-006: insertClient writes a NULL user_id for anonymous DCR clients', async () => { const row = await clients.insertClient({ id: 'client-row-2', @@ -132,6 +151,23 @@ describe('OauthClientsRepository', () => { }); expect(row.user_id).toBeNull(); }); + + it('OAUTHCLIENTREPO-006b: throws when the read-back after insert finds no row (coverage: the guard branch)', async () => { + const spy = vi.spyOn(clients, 'findPublicById').mockResolvedValueOnce(null); + await expect(clients.insertClient({ + id: 'client-row-ghost', + user_id: null, + name: 'Ghost Client', + client_id: 'proto-ghost', + client_secret_hash: 'x', + redirect_uris: '[]', + allowed_scopes: '[]', + is_public: 1, + created_via: 'dcr', + allows_client_credentials: 0, + })).rejects.toThrow('OauthClientsRepository.insertClient: row client-row-ghost not found immediately after insert'); + spy.mockRestore(); + }); }); describe('findOwned (OA6/OA9)', () => { @@ -193,6 +229,10 @@ describe('OauthClientsRepository', () => { const row = await clients.findAuthRow(clientId); expect(row).toEqual({ client_id: clientId, client_secret_hash: 'auth-secret', is_public: 0 }); }); + + it('OAUTHCLIENTREPO-013b: null for unknown client_id', async () => { + expect(await clients.findAuthRow('nope')).toBeNull(); + }); }); describe('findByClientIdFull (OA29/OA30)', () => { diff --git a/server/tests/unit/db/repositories/OauthTokens.repository.test.ts b/server/tests/unit/db/repositories/OauthTokens.repository.test.ts index 2abcaf030..fd8658d79 100644 --- a/server/tests/unit/db/repositories/OauthTokens.repository.test.ts +++ b/server/tests/unit/db/repositories/OauthTokens.repository.test.ts @@ -165,6 +165,10 @@ describe('OauthTokensRepository', () => { expect(await tokens.findParent(childId)).toEqual({ id: childId, parent_token_id: rootId }); expect(await tokens.findParent(rootId)).toEqual({ id: rootId, parent_token_id: null }); }); + + it('OAUTHTOKREPO-007b: an unknown id is null', async () => { + expect(await tokens.findParent(999999)).toBeNull(); + }); }); describe('collectChainIds (OA18) — recursive CTE, proven against the legacy raw statement', () => { @@ -366,6 +370,15 @@ describe('OauthTokensRepository', () => { expect(rows.map((r) => r.client_name)).toEqual(['Client B', 'Client A']); expect(rows[0].scopes).toBeDefined(); }); + + it('OAUTHTOKREPO-022c: created_at NULL comes back null, not undefined (coverage: rule 16)', async () => { + const { user } = createUser(testDb); + seedClient(user.id, 'proto-22null'); + const id = seedToken({ clientId: 'proto-22null', userId: user.id }); + testDb.prepare('UPDATE oauth_tokens SET created_at = NULL WHERE id = ?').run(id); + const rows = await tokens.listActiveByUser(user.id); + expect(rows[0].created_at).toBeNull(); + }); }); describe('findOwnedById (OA27)', () => { @@ -398,6 +411,15 @@ describe('OauthTokensRepository', () => { expect(rows).toHaveLength(1); expect(rows[0]).toMatchObject({ client_name: 'Client A', username: 'admin-panel-a', user_id: a.id }); }); + + it('OAUTHTOKREPO-025b: created_at NULL comes back null, not undefined (coverage: rule 16)', async () => { + const { user } = createUser(testDb, { username: 'admin-panel-null' }); + seedClient(user.id, 'proto-25null', 'Client Null'); + const id = seedToken({ clientId: 'proto-25null', userId: user.id }); + testDb.prepare('UPDATE oauth_tokens SET created_at = NULL WHERE id = ?').run(id); + const rows = await tokens.listAllActiveWithClientAndUser(); + expect(rows.find((r) => r.id === id)?.created_at).toBeNull(); + }); }); describe('findById (OA32)', () => { diff --git a/server/tests/unit/db/repositories/Tags.repository.test.ts b/server/tests/unit/db/repositories/Tags.repository.test.ts index eb834af0c..f8f7e4e4a 100644 --- a/server/tests/unit/db/repositories/Tags.repository.test.ts +++ b/server/tests/unit/db/repositories/Tags.repository.test.ts @@ -92,10 +92,15 @@ describe('TagsRepository', () => { expect(entity?.user.isInitialized()).toBe(false); }); - // Plan 3b Task 1 fix round (task-1-review.md B1): findByIdAndUser - // carries `disableIdentityMap: true` (via the shared findOwnedByUser - // helper) — always re-queries and merges fresh data, in one query, and - // never lands in the request's identity map. + // Task 7 review, B-L3: this pins "one query, fresh value" — it does NOT + // isolate the `disableIdentityMap: true` ruling from a plain `refresh: + // true` re-query, and it should not claim to. `findByIdAndUser`'s + // filter is `(id, user_id)`, not the bare PK (`owned-lookup.ts`'s + // `findOwnedByUser`), so it is not PK-only and always re-queries either + // way — the real D-shape (identity-map write-back) proof lives on + // `WEBAUTHN-CRED-REPO-017`/`OAUTHCLIENTREPO-016`/`INVREPO-017`, which + // mutation-prove `disableIdentityMap: true` specifically. Same honesty + // `SETTINGSREPO-012`'s comment already carries for the same reason. it('TAGREPO-009: a raw UPDATE on the same row then findByIdAndUser reads the new value, in one query', async () => { const { user } = createUser(testDb); const created = createTag(testDb, user.id, { name: 'Stale', color: '#111111' }); @@ -123,6 +128,14 @@ describe('TagsRepository', () => { const row = await tags.createTag({ user_id: user.id, name: 'NoDefault', color: 'literal-color' }); expect(row.color).toBe('literal-color'); }); + + it('TAGREPO-011b: throws when the read-back after insert finds no row (coverage: the guard branch)', async () => { + const { user } = createUser(testDb); + const spy = vi.spyOn(tags, 'findOne').mockResolvedValueOnce(null); + await expect(tags.createTag({ user_id: user.id, name: 'Ghost', color: '#000000' })) + .rejects.toThrow('createTag: read-back after insert found no row'); + spy.mockRestore(); + }); }); describe('patch', () => { @@ -154,6 +167,26 @@ describe('TagsRepository', () => { expect(await tags.patch(99999999, { name: 'Nope' })).toBeNull(); }); + it('TAGREPO-015b: an empty changes object leaves every column untouched (coverage: the existence-check branch)', async () => { + const { user } = createUser(testDb); + const created = createTag(testDb, user.id, { name: 'Unchanged', color: '#eeeeee' }); + const before = rawTag(created.id); + const updated = await tags.patch(created.id, {}); + expect(updated).toStrictEqual(before); + }); + + it('TAGREPO-015c: an empty changes object against a non-existent id returns null via the existence-check branch (coverage)', async () => { + expect(await tags.patch(99999999, {})).toBeNull(); + }); + + it('TAGREPO-015d: the row vanishing between the update and the final re-read returns null (race-condition guard, coverage)', async () => { + const { user } = createUser(testDb); + const created = createTag(testDb, user.id, { name: 'RaceTarget' }); + const spy = vi.spyOn(tags, 'findOne').mockResolvedValueOnce(null); + expect(await tags.patch(created.id, { name: 'ShouldNotMatter' })).toBeNull(); + spy.mockRestore(); + }); + // Task 3 review, Important 1, consequence (a) — and Minor 1: the // identity map is populated through `listByUser()` here, not // `findByIdAndUser`, so this doubles as the "list → raw UPDATE → diff --git a/server/tests/unit/db/repositories/Users.repository.test.ts b/server/tests/unit/db/repositories/Users.repository.test.ts index 8b7edb878..ac9ecd099 100644 --- a/server/tests/unit/db/repositories/Users.repository.test.ts +++ b/server/tests/unit/db/repositories/Users.repository.test.ts @@ -161,6 +161,19 @@ describe('UsersRepository', () => { testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(user.id); expect(await users.findIdByEmailOrUsernameCI('guest1@example.com', 'guest1')).toBeNull(); }); + + // Program rule 18 / Plan 3b Task 7 review H1: SQLite's LOWER() is + // ASCII-only; JS's toLowerCase() is full-Unicode. Mutation-proved by + // reverting the repository's `lowerParam(platform, email)` bind back to + // a plain `email.toLowerCase()` — this then fails, because the column + // stays folded by SQLite's LOWER() ('josÉ@x.com') while the JS-lowered + // bind is 'josé@x.com'. + it('USERSREPO-014b: matches the exact stored non-ASCII spelling; a JS-lowered spelling does not match', async () => { + const { user } = createUser(testDb, { email: 'JOSÉ@x.com', username: 'plainname' }); + expect(await users.findIdByEmailOrUsernameCI('JOSÉ@x.com', 'no-match')).toBe(user.id); + expect(await users.findIdByEmailOrUsernameCI('JOSÉ@X.COM', 'no-match')).toBe(user.id); + expect(await users.findIdByEmailOrUsernameCI('josé@x.com', 'no-match')).toBeNull(); + }); }); describe('insertUser (AU10/O14)', () => { @@ -234,9 +247,21 @@ describe('UsersRepository', () => { t.em.clear(); // discard the still-pending unflushed entity before the next test }); + + it('USERSREPO-057D: throws when the read-back after insert finds no row (coverage: the guard branch)', async () => { + const spy = vi.spyOn(users, 'findById').mockResolvedValueOnce(null); + await expect(users.insertUser({ + username: 'ghostuser', + email: 'ghost@example.com', + password_hash: 'hashed', + role: 'user', + first_seen_version: '1.2.3', + })).rejects.toThrow('insertUser: read-back after insert found no row'); + spy.mockRestore(); + }); }); - describe('findByEmailCI (AU12/O4)', () => { + describe('findByEmailCI (AU12) — AuthService.loginUser', () => { it('USERSREPO-018: matches case-insensitively, excludes guests, returns the full row', async () => { const { user } = createUser(testDb, { email: 'Case.Test@Example.com' }); const row = await users.findByEmailCI('case.test@example.com'); @@ -250,6 +275,48 @@ describe('UsersRepository', () => { testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(user.id); expect(await users.findByEmailCI('guest2@example.com')).toBeNull(); }); + + // Program rule 18 / Plan 3b Task 7 review H1. Mutation-proved by + // reverting the repository's `lowerParam(platform, email)` bind back to + // `email.toLowerCase()` — then USERSREPO-018b's second assertion fails + // (401 lockout reproduced at the repository level: the exact stored + // spelling stops matching). + it('USERSREPO-018b: matches the exact stored non-ASCII spelling; a JS-lowered spelling does not match', async () => { + const { user } = createUser(testDb, { email: 'JOSÉ@x.com' }); + const exact = await users.findByEmailCI('JOSÉ@x.com'); + expect(exact?.id).toBe(user.id); + const asciiRecase = await users.findByEmailCI('JOSÉ@X.COM'); + expect(asciiRecase?.id).toBe(user.id); + expect(await users.findByEmailCI('josé@x.com')).toBeNull(); + }); + }); + + describe('findByEmailLoweredBind (O4) — OidcService.findOrCreateUser, caller already JS-lowers', () => { + it('USERSREPO-018c: matches an already-lowered bind against a mixed-case stored email, excludes guests', async () => { + const { user } = createUser(testDb, { email: 'Case.Test@Example.com' }); + const row = await users.findByEmailLoweredBind('case.test@example.com'); + expect(row?.id).toBe(user.id); + expect(await users.findByEmailLoweredBind('nope@example.com')).toBeNull(); + }); + + it('USERSREPO-018d: a guest with a matching email is not returned', async () => { + const { user } = createUser(testDb, { email: 'guest2b@example.com' }); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(user.id); + expect(await users.findByEmailLoweredBind('guest2b@example.com')).toBeNull(); + }); + + // O4's own legacy shape: `LOWER(email) = ?` — SQLite only lowers the + // COLUMN; the caller's already-lowered bind is compared as-is. A stored + // non-ASCII email folds to 'josÉ@x.com' under SQLite's ASCII-only + // LOWER(); a JS-lowered bind of the exact same spelling is + // 'josé@x.com' — genuinely different strings, so THIS method correctly + // does not match it. That is O4's real (pre-existing, unchanged) + // behaviour, not a regression — findByEmailCI above is the method + // OidcService's non-JS-lowered callers must use instead. + it('USERSREPO-018e: a JS-lowered bind against a non-ASCII stored email does not match (this is O4 parity, not a bug)', async () => { + createUser(testDb, { email: 'JOSÉ@x.com' }); + expect(await users.findByEmailLoweredBind('josé@x.com')).toBeNull(); + }); }); // Plan 3b Task 1 fix round item 7 — AuthService.demoLogin's exact statement @@ -300,6 +367,17 @@ describe('UsersRepository', () => { expect(await users.findMeRow(999999)).toBeNull(); }); + it('USERSREPO-022b: findMeRow — every nullable column comes back null, not undefined, when NULL in the row (coverage: rule 16)', async () => { + const { user } = createUser(testDb); + testDb.prepare('UPDATE users SET avatar = NULL, oidc_issuer = NULL, created_at = NULL, mfa_enabled = NULL, must_change_password = NULL WHERE id = ?').run(user.id); + const row = await users.findMeRow(user.id); + expect(row?.avatar).toBeNull(); + expect(row?.oidc_issuer).toBeNull(); + expect(row?.created_at).toBeNull(); + expect(row?.mfa_enabled).toBeNull(); + expect(row?.must_change_password).toBeNull(); + }); + it('USERSREPO-023: getPasswordHashAndVersion (AU15) reads both columns together', async () => { const { user, password: _password } = createUser(testDb); testDb.prepare('UPDATE users SET password_version = 2 WHERE id = ?').run(user.id); @@ -308,6 +386,10 @@ describe('UsersRepository', () => { expect(row?.password_version).toBe(2); }); + it('USERSREPO-023b: getPasswordHashAndVersion returns null for a missing user', async () => { + expect(await users.getPasswordHashAndVersion(999999)).toBeNull(); + }); + it('USERSREPO-024: setPassword (AU16/AU43) writes hash+version, clears must_change_password, stamps updated_at', async () => { const { user } = createUser(testDb); testDb.prepare('UPDATE users SET must_change_password = 1 WHERE id = ?').run(user.id); @@ -345,6 +427,12 @@ describe('UsersRepository', () => { it('USERSREPO-028: returns null (not a row with a null field) when the user does not exist', async () => { expect(await users.getMfaEnabled(999999)).toBeNull(); }); + + it('USERSREPO-028b: mfa_enabled NULL on the row comes back null on the wrapper (coverage: rule 16)', async () => { + const { user } = createUser(testDb); + testDb.prepare('UPDATE users SET mfa_enabled = NULL WHERE id = ?').run(user.id); + expect(await users.getMfaEnabled(user.id)).toEqual({ mfa_enabled: null }); + }); }); it('USERSREPO-029: enableMfa (AU28) sets mfa_enabled=1, writes the already-encrypted secret and codes, stamps updated_at', async () => { @@ -425,6 +513,13 @@ describe('UsersRepository', () => { testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(user.id); expect(await users.findForPasswordReset('guest3@example.com')).toBeNull(); }); + + it('USERSREPO-036b: oidc_sub NULL on the row comes back null, not undefined (coverage: rule 16)', async () => { + const { user } = createUser(testDb, { email: 'reset-null@example.com' }); + testDb.prepare('UPDATE users SET oidc_sub = NULL WHERE id = ?').run(user.id); + const row = await users.findForPasswordReset('reset-null@example.com'); + expect(row?.oidc_sub).toBeNull(); + }); }); it('USERSREPO-037: findResetTarget (AU40) reads the reset-branch projection', async () => { @@ -434,6 +529,19 @@ describe('UsersRepository', () => { expect(row).toEqual({ id: user.id, email: user.email, mfa_enabled: 1, mfa_secret: 's', mfa_backup_codes: 'c', password_version: 4 }); }); + it('USERSREPO-037b: findResetTarget — mfa_enabled/mfa_secret/mfa_backup_codes NULL come back null, not undefined (coverage: rule 16)', async () => { + const { user } = createUser(testDb); + testDb.prepare('UPDATE users SET mfa_enabled = NULL, mfa_secret = NULL, mfa_backup_codes = NULL WHERE id = ?').run(user.id); + const row = await users.findResetTarget(user.id); + expect(row?.mfa_enabled).toBeNull(); + expect(row?.mfa_secret).toBeNull(); + expect(row?.mfa_backup_codes).toBeNull(); + }); + + it('USERSREPO-037c: findResetTarget returns null for a missing user', async () => { + expect(await users.findResetTarget(999999)).toBeNull(); + }); + it('USERSREPO-038: getPasswordHash (PK15) reads password_hash', async () => { const { user } = createUser(testDb); expect(await users.getPasswordHash(user.id)).toBe(user.password_hash); @@ -500,6 +608,22 @@ describe('UsersRepository', () => { testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(user.id); expect(await users.findIdByUsernameCIAny('anyname')).toBe(user.id); // a guest still matches here }); + + // Program rule 18 / Plan 3b Task 7 review H1. Mutation-proved by + // reverting each method's `lowerParam(platform, username)` bind back to + // `username.toLowerCase()` — the exact-spelling assertion then fails. + it('USERSREPO-044b: findIdByUsernameCI matches the exact stored non-ASCII spelling; a JS-lowered spelling does not match', async () => { + const { user: a } = createUser(testDb, { username: 'ÄNNA' }); + const { user: b } = createUser(testDb, { username: 'other2' }); + expect(await users.findIdByUsernameCI('ÄNNA', b.id)).toBe(a.id); + expect(await users.findIdByUsernameCI('änna', b.id)).toBeNull(); + }); + + it('USERSREPO-045b: findIdByUsernameCIAny matches the exact stored non-ASCII spelling; a JS-lowered spelling does not match', async () => { + const { user } = createUser(testDb, { username: 'ÄNNA2' }); + expect(await users.findIdByUsernameCIAny('ÄNNA2')).toBe(user.id); + expect(await users.findIdByUsernameCIAny('änna2')).toBeNull(); + }); }); it('USERSREPO-046: getApiKeyColumns (UP1) reads role + the four key columns', async () => { @@ -509,6 +633,13 @@ describe('UsersRepository', () => { expect(await users.getApiKeyColumns(user.id)).toEqual({ role: 'admin', maps_api_key: 'm', openweather_api_key: 'o', unsplash_api_key: 'u', amap_api_key: 'a' }); }); + it('USERSREPO-046b: getApiKeyColumns returns null for a missing user; NULL columns come back null (coverage: rule 16)', async () => { + expect(await users.getApiKeyColumns(999999)).toBeNull(); + const { user } = createUser(testDb); + // maps_api_key/openweather_api_key/unsplash_api_key/amap_api_key are NULL by default on a fresh user. + expect(await users.getApiKeyColumns(user.id)).toEqual({ role: 'user', maps_api_key: null, openweather_api_key: null, unsplash_api_key: null, amap_api_key: null }); + }); + it('USERSREPO-047: updateMapsKey (UP2) writes maps_api_key + updated_at', async () => { const { user } = createUser(testDb); await users.updateMapsKey(user.id, 'encrypted'); @@ -532,6 +663,16 @@ describe('UsersRepository', () => { expect(row).toMatchObject({ id: user.id, username: user.username, email: user.email, role: 'user', maps_api_key: 'm', mfa_enabled: 1 }); }); + it('USERSREPO-049b: findProfileWithKeys returns null for a missing user; every nullable column comes back null (coverage: rule 16)', async () => { + expect(await users.findProfileWithKeys(999999)).toBeNull(); + const { user } = createUser(testDb); + testDb.prepare('UPDATE users SET mfa_enabled = NULL WHERE id = ?').run(user.id); + const row = await users.findProfileWithKeys(user.id); + expect(row).toMatchObject({ + maps_api_key: null, openweather_api_key: null, unsplash_api_key: null, amap_api_key: null, avatar: null, mfa_enabled: null, + }); + }); + it('USERSREPO-050: findIdByEmailCI (UP6) matches case-insensitively, excludes self and guests', async () => { const { user: a } = createUser(testDb, { email: 'Dup@Example.com' }); const { user: b } = createUser(testDb, { email: 'other@example.com' }); @@ -539,6 +680,16 @@ describe('UsersRepository', () => { expect(await users.findIdByEmailCI('dup@example.com', a.id)).toBeNull(); }); + // Program rule 18 / Plan 3b Task 7 review H1 (UP6 profile-rename collision + // case). Mutation-proved by reverting `lowerParam(platform, email)` back + // to `email.toLowerCase()` — the exact-spelling assertion then fails. + it('USERSREPO-050b: findIdByEmailCI matches the exact stored non-ASCII spelling; a JS-lowered spelling does not match', async () => { + const { user: a } = createUser(testDb, { email: 'JOSÉ2@x.com' }); + const { user: b } = createUser(testDb, { email: 'other2@example.com' }); + expect(await users.findIdByEmailCI('JOSÉ2@x.com', b.id)).toBe(a.id); + expect(await users.findIdByEmailCI('josé2@x.com', b.id)).toBeNull(); + }); + it('USERSREPO-051: patchProfile (UP7) writes only the given bounded columns + updated_at, in one statement', async () => { const { user } = createUser(testDb, { username: 'patchme', email: 'patchme@example.com' }); await users.patchProfile(user.id, { username: 'patched', maps_api_key: 'newkey' }); @@ -578,6 +729,12 @@ describe('UsersRepository', () => { expect(await users.findProfileBasic(user.id)).toEqual({ id: user.id, username: user.username, email: user.email, role: 'user', avatar: 'a.png' }); }); + it('USERSREPO-054b: findProfileBasic returns null for a missing user; NULL avatar comes back null (coverage: rule 16)', async () => { + expect(await users.findProfileBasic(999999)).toBeNull(); + const { user } = createUser(testDb); + expect((await users.findProfileBasic(user.id))?.avatar).toBeNull(); + }); + describe('listOthersNonGuest (UP14)', () => { it('USERSREPO-055: excludes the given id and every guest, ordered by username', async () => { const { user: me } = createUser(testDb, { username: 'me' }); @@ -595,4 +752,16 @@ describe('UsersRepository', () => { testDb.prepare('UPDATE users SET openweather_api_key = ? WHERE id = ?').run('w', user.id); expect(await users.getRoleAndWeatherKey(user.id)).toEqual({ role: 'admin', openweather_api_key: 'w' }); }); + + it('USERSREPO-056b: getRoleAndWeatherKey returns null for a missing user; NULL key comes back null (coverage: rule 16)', async () => { + expect(await users.getRoleAndWeatherKey(999999)).toBeNull(); + const { user } = createUser(testDb); + expect(await users.getRoleAndWeatherKey(user.id)).toEqual({ role: 'user', openweather_api_key: null }); + }); + + it('USERSREPO-058: deleteById (UC11) deletes the row', async () => { + const { user } = createUser(testDb); + await users.deleteById(user.id); + expect(testDb.prepare('SELECT id FROM users WHERE id = ?').get(user.id)).toBeUndefined(); + }); }); diff --git a/server/tests/unit/db/repositories/WebauthnCredentials.repository.test.ts b/server/tests/unit/db/repositories/WebauthnCredentials.repository.test.ts index a70961a62..da4134bc2 100644 --- a/server/tests/unit/db/repositories/WebauthnCredentials.repository.test.ts +++ b/server/tests/unit/db/repositories/WebauthnCredentials.repository.test.ts @@ -6,6 +6,7 @@ import { createUser } from '../../../helpers/factories'; import { WebauthnCredentials } from '../../../../src/db/entities/WebauthnCredentials.entity'; import type { WebauthnCredentialsRepository } from '../../../../src/db/repositories/WebauthnCredentials.repository'; import { UnitOfWork } from '../../../../src/nest/database/unit-of-work'; +import { withRequestContext } from '../../../../src/nest/database/request-context'; const testDb = createSnapshotTestDb(); let t: TestOrm; @@ -154,6 +155,15 @@ describe('WebauthnCredentialsRepository', () => { expect(row).not.toHaveProperty('public_key'); }); + it('WEBAUTHN-CRED-REPO-006b: name/created_at NULL come back null, not undefined (coverage: rule 16)', async () => { + const { user } = createUser(testDb); + const cred = insertCredential(user.id, { name: null }); + testDb.prepare('UPDATE webauthn_credentials SET created_at = NULL WHERE id = ?').run(cred.id); + const row = await creds.findCreatedCredential(cred.credential_id as string); + expect(row?.name).toBeNull(); + expect(row?.created_at).toBeNull(); + }); + it('WEBAUTHN-CRED-REPO-007: findCreatedCredential is null for an unknown credential_id', async () => { expect(await creds.findCreatedCredential('no-such-cred')).toBeNull(); }); @@ -243,24 +253,40 @@ describe('WebauthnCredentialsRepository', () => { }); describe('identity-map isolation (coordinator ruling — stale-write-back bug class)', () => { + // Task 7 review, M1: the D-shape needs the nativeUpdate's target column + // (`counter`) in the FIRST, WIDER projection and absent from the + // SECOND, narrower one — a stale write-back only reverts a column the + // second read's snapshot doesn't already carry fresh. The original + // ordering here (narrow existsByCredentialId first, wide + // findByCredentialId second) had it backwards: findByCredentialId's own + // re-snapshot of `counter` left that column clean, so the mutation this + // test exists to catch (reverting `disableIdentityMap: true` back to + // `refresh: true` in the base class's three read overrides) passed + // anyway. `findByCredentialId` (wide, carries `counter`) now goes FIRST + // and `existsByCredentialId` (narrow, `fields: ['id']`, no `counter`) + // second — mutation-proven load-bearing in the fix report. Also now + // wrapped in `withRequestContext` (the reads previously ran at + // describe-body top level, outside any request context). it('WEBAUTHN-CRED-REPO-017: a nativeUpdate inside uow.transactional is not discarded by a stale entity read earlier under a different projection', async () => { const { user } = createUser(testDb); const cred = insertCredential(user.id, { counter: 1 }); const uow = new UnitOfWork(t.em); - // Projection A — narrow existence probe (PK6's shape: existsByCredentialId). - await creds.existsByCredentialId(cred.credential_id as string); - // Projection B — full row, same request context (PK9's shape: findByCredentialId). - const before = await creds.findByCredentialId(cred.credential_id as string); - expect(before!.counter).toBe(1); + await withRequestContext(t.orm, async () => { + // Projection A — wide, full row, carries `counter` (PK9's shape: findByCredentialId). + const before = await creds.findByCredentialId(cred.credential_id as string); + expect(before!.counter).toBe(1); + // Projection B — narrow existence probe, no `counter` (PK6's shape: existsByCredentialId). + await creds.existsByCredentialId(cred.credential_id as string); - // The write these two reads sit alongside in the same request — PK11's - // shape: a nativeUpdate on a row this request already read twice under - // different projections, inside uow.transactional (whose closing flush - // is exactly where the coordinator's ruling says a stale identity-mapped - // entity would silently overwrite this write). - await uow.transactional(async () => { - await creds.updateCounterAndLastUsed(cred.id, 99); + // The write these two reads sit alongside in the same request — PK11's + // shape: a nativeUpdate on a row this request already read twice under + // different projections, inside uow.transactional (whose closing flush + // is exactly where the coordinator's ruling says a stale identity-mapped + // entity would silently overwrite this write). + await uow.transactional(async () => { + await creds.updateCounterAndLastUsed(cred.id, 99); + }); }); // Read again through a THIRD projection after the transaction commits diff --git a/server/tests/unit/db/repositories/trek-repository.test.ts b/server/tests/unit/db/repositories/trek-repository.test.ts index 1bbf0dd64..5ea5f81b6 100644 --- a/server/tests/unit/db/repositories/trek-repository.test.ts +++ b/server/tests/unit/db/repositories/trek-repository.test.ts @@ -94,6 +94,14 @@ describe('TrekRepository — every overridden path fails closed outside a reques await expect(users.findAll()).rejects.toThrow(GLOBAL_CONTEXT_ERROR); }); + // Task 7 review, B-L5: mutating `validateRequestContext()` to a no-op + // fails 11 of the other 12 fail-closed cases in this describe block — this + // one still passes, because MikroORM's own `count()` path throws + // `cannotUseGlobalContext` downstream regardless (it reaches the EM's + // validating context on its own). The override here is defensive- + // redundant for `count` specifically: this test proves end-to-end + // behaviour, not that the override itself is load-bearing for this one + // method — every other case in this file IS proof the override matters. it('TREKREPO-004: count rejects', async () => { await expect(users.count({})).rejects.toThrow(GLOBAL_CONTEXT_ERROR); }); @@ -133,6 +141,42 @@ describe('TrekRepository — every overridden path fails closed outside a reques it('TREKREPO-012: getEntityManager() itself rejects', () => { expect(() => users.getEntityManager()).toThrow(GLOBAL_CONTEXT_ERROR); }); + + // Task 7 review, M4: the seven previously-unoverridden base-class methods + // (findOneOrFail/findAndCount/findByCursor/countBy/stream/insertMany/ + // upsertMany) — none used anywhere in this program today, but the first + // one used would silently lose both guarantees with nothing to notice. + it('TREKREPO-016: findOneOrFail rejects', async () => { + await expect(users.findOneOrFail({ id: 1 })).rejects.toThrow(GLOBAL_CONTEXT_ERROR); + }); + + it('TREKREPO-017: findAndCount rejects', async () => { + await expect(users.findAndCount({})).rejects.toThrow(GLOBAL_CONTEXT_ERROR); + }); + + it('TREKREPO-018: findByCursor rejects', async () => { + await expect(users.findByCursor({ first: 1, orderBy: { id: 'asc' } })).rejects.toThrow(GLOBAL_CONTEXT_ERROR); + }); + + it('TREKREPO-019: countBy rejects', async () => { + await expect(users.countBy('role')).rejects.toThrow(GLOBAL_CONTEXT_ERROR); + }); + + it('TREKREPO-020: stream rejects', () => { + expect(() => users.stream()).toThrow(GLOBAL_CONTEXT_ERROR); + }); + + it('TREKREPO-021: insertMany rejects', async () => { + await expect( + users.insertMany([{ username: 'outside-context-many', email: 'outside-many@example.test', password_hash: 'h', role: 'user', first_seen_version: '1.0' }]), + ).rejects.toThrow(GLOBAL_CONTEXT_ERROR); + }); + + it('TREKREPO-022: upsertMany rejects', async () => { + await expect( + users.upsertMany([{ id: 1, username: 'outside-context-upsert-many' }], { onConflictFields: ['id'], onConflictAction: 'merge' }), + ).rejects.toThrow(GLOBAL_CONTEXT_ERROR); + }); }); describe('TrekRepository — every overridden path succeeds inside withRequestContext', () => { @@ -185,6 +229,51 @@ describe('TrekRepository — every overridden path succeeds inside withRequestCo const row = testDb.prepare('SELECT username FROM users WHERE id = ?').get(user.id) as { username: string }; expect(row.username).toBe('inside-upserted'); // the upsert's merge is the last write and must stick }); + + // Task 7 review, M4 — the same seven methods, inside a request context. + it('TREKREPO-023: findOneOrFail/findAndCount/findByCursor/countBy/stream/insertMany/upsertMany all succeed', async () => { + const { user } = createUser(testDb, { username: 'm4-before', email: 'm4-before@example.test' }); + + await withRequestContext(t.orm, async () => { + await expect(users.findOneOrFail({ id: user.id })).resolves.not.toBeNull(); + + const [rows, count] = await users.findAndCount({ id: user.id }); + expect(rows).toHaveLength(1); + expect(count).toBe(1); + + const cursor = await users.findByCursor({ first: 1, orderBy: { id: 'asc' } }); + expect(cursor.items.length).toBeGreaterThan(0); + + const counts = await users.countBy('role'); + expect(counts['user']).toBeGreaterThan(0); + + const streamed: number[] = []; + for await (const row of users.stream({ where: { id: user.id } })) { + streamed.push(row.id); + } + expect(streamed).toEqual([user.id]); + + // SQLite (via better-sqlite3) does not return one primary key per row + // for a batch insert — only that it does not throw inside a request + // context is what this test proves; the row count is verified below + // from the raw table, outside the context. + await users.insertMany([ + { username: 'm4-many-1', email: 'm4-many-1@example.test', password_hash: 'h', role: 'user', first_seen_version: '1.0' }, + { username: 'm4-many-2', email: 'm4-many-2@example.test', password_hash: 'h', role: 'user', first_seen_version: '1.0' }, + ]); + + const upserted = await users.upsertMany( + [{ id: user.id, username: 'm4-upserted', email: user.email, password_hash: 'h', role: 'user', first_seen_version: '1.0' }], + { onConflictFields: ['id'], onConflictAction: 'merge' }, + ); + expect(upserted).toHaveLength(1); + }); + + const row = testDb.prepare('SELECT username FROM users WHERE id = ?').get(user.id) as { username: string }; + expect(row.username).toBe('m4-upserted'); + const manyRows = testDb.prepare("SELECT username FROM users WHERE username LIKE 'm4-many-%'").all() as { username: string }[]; + expect(manyRows).toHaveLength(2); + }); }); describe('TrekRepository — inside uow.transactional', () => { diff --git a/server/tests/unit/nest/categories.service.test.ts b/server/tests/unit/nest/categories.service.test.ts index 4582173e7..6bf1efa44 100644 --- a/server/tests/unit/nest/categories.service.test.ts +++ b/server/tests/unit/nest/categories.service.test.ts @@ -161,6 +161,16 @@ describe('update', () => { expect(updated.name).toBe('NewName'); expect(updated.color).toBe('#dddddd'); }); + + // Coverage: B-H1 Class C (Plan 3b Task 7 review). The route's own getById + // pre-check 404s a non-numeric id before update()/remove() are ever + // reached in production — this calls the service method directly, + // bypassing that pre-check, to exercise the toRowId guard's own no-op + // branch (the race-condition path the pre-check doesn't cover). + it('CAT-SVC-013b — update with a non-numeric id no-ops (toRowId guard, called directly)', async () => { + const updated = await svc.update('abc', 'ShouldNotApply', '#000000', '🚫'); + expect(updated).toBeUndefined(); + }); }); // ── remove ──────────────────────────────────────────────────────────────────── @@ -176,4 +186,12 @@ describe('remove', () => { it('CAT-SVC-015 — deleting a non-existent category does not throw', async () => { await expect(svc.remove(99999)).resolves.not.toThrow(); }); + + // Coverage: B-H1 Class C, see CAT-SVC-013b's comment. + it('CAT-SVC-015b — remove with a non-numeric id no-ops (toRowId guard, called directly)', async () => { + const { user } = createUser(testDb); + const cat = await svc.create(user.id, 'Untouched'); + await expect(svc.remove('abc')).resolves.not.toThrow(); + expect(await svc.getById(cat.id)).toBeDefined(); + }); }); diff --git a/server/tests/unit/nest/common/row-id.test.ts b/server/tests/unit/nest/common/row-id.test.ts index 1c9ef9b03..4c9a8f197 100644 --- a/server/tests/unit/nest/common/row-id.test.ts +++ b/server/tests/unit/nest/common/row-id.test.ts @@ -58,4 +58,8 @@ describe('toRowId', () => { it('ROWID-011: rejects a number one past MAX_SAFE_INTEGER (2**53)', () => { expect(toRowId(2 ** 53)).toBeNull(); }); + + it('ROWID-012: rejects a digits-only STRING one past MAX_SAFE_INTEGER (coverage: the string branch\'s own safe-integer check)', () => { + expect(toRowId('9007199254740993')).toBeNull(); + }); }); diff --git a/server/tests/unit/nest/oidc.service.test.ts b/server/tests/unit/nest/oidc.service.test.ts index edf500fa6..3c333e11c 100644 --- a/server/tests/unit/nest/oidc.service.test.ts +++ b/server/tests/unit/nest/oidc.service.test.ts @@ -784,6 +784,12 @@ describe('findOrCreateUser', () => { it('OIDC-SVC-090: identity linking (O5) leaves the rest of the users row byte-identical', async () => { const { user } = createUser(testDb, { email: 'linkrow@example.com', username: 'linkrowuser' }); testDb.prepare('UPDATE users SET oidc_sub = NULL, oidc_issuer = NULL WHERE id = ?').run(user.id); + // Task 6 review L1 / Task 7 review, Task 6 L1: pin updated_at to a fixed + // past value before the snapshot so a future `linkOidcIdentity` that + // starts stamping updated_at (a CURRENT_TIMESTAMP drift) makes this test + // fail loudly, instead of the before/after snapshot racing the mutation + // inside the same second and passing by accident. + testDb.prepare("UPDATE users SET updated_at = '2020-01-01 00:00:00' WHERE id = ?").run(user.id); const before = testDb.prepare('SELECT * FROM users WHERE id = ?').get(user.id) as Record; await svc.findOrCreateUser( @@ -799,6 +805,8 @@ describe('findOrCreateUser', () => { const { user } = createUser(testDb, { email: 'switchrow@example.com', username: 'switchrowuser' }); testDb.prepare('UPDATE users SET oidc_sub = ?, oidc_issuer = ? WHERE id = ?') .run('sub-old-row', 'https://old-idp.example.com', user.id); + // Task 6 review L1 / Task 7 review, Task 6 L1: see OIDC-SVC-090's comment. + testDb.prepare("UPDATE users SET updated_at = '2020-01-01 00:00:00' WHERE id = ?").run(user.id); const before = testDb.prepare('SELECT * FROM users WHERE id = ?').get(user.id) as Record; await svc.findOrCreateUser( diff --git a/server/tests/unit/nest/tags.service.test.ts b/server/tests/unit/nest/tags.service.test.ts index f17e59729..a6ee9ba2c 100644 --- a/server/tests/unit/nest/tags.service.test.ts +++ b/server/tests/unit/nest/tags.service.test.ts @@ -148,6 +148,16 @@ describe('update', () => { expect(updated.name).toBe('NewName'); expect(updated.color).toBe('#dddddd'); }); + + // Coverage: B-H1 Class C (Plan 3b Task 7 review). The route's own + // getByIdAndUser pre-check 404s a non-numeric id before update()/remove() + // are ever reached in production — these call the service methods + // directly, bypassing that pre-check, to exercise the toRowId guard's own + // no-op branch (the race-condition path the pre-check doesn't cover). + it('TAG-SVC-012b — update with a non-numeric id no-ops (toRowId guard, called directly)', async () => { + const updated = await svc.update('abc', 'ShouldNotApply', '#000000'); + expect(updated).toBeUndefined(); + }); }); // ── remove ──────────────────────────────────────────────────────────────────── @@ -173,4 +183,12 @@ describe('remove', () => { expect(remaining).toHaveLength(1); expect(remaining[0].id).toBe(t1.id); }); + + // Coverage: B-H1 Class C (Plan 3b Task 7 review), see TAG-SVC-012b's comment. + it('TAG-SVC-015b — remove with a non-numeric id no-ops (toRowId guard, called directly)', async () => { + const { user } = createUser(testDb); + const tag = await svc.create(user.id, 'Untouched'); + await expect(svc.remove('abc')).resolves.not.toThrow(); + expect(await svc.getByIdAndUser(tag.id, user.id)).toBeDefined(); + }); }); diff --git a/server/tests/unit/nest/to-client-user.parity.test.ts b/server/tests/unit/nest/to-client-user.parity.test.ts new file mode 100644 index 000000000..2e4be1ada --- /dev/null +++ b/server/tests/unit/nest/to-client-user.parity.test.ts @@ -0,0 +1,87 @@ +/** + * TOCLIENTUSER-PARITY-001 — the three hand-mirrored `toClientUser` copies + * stay byte-identical. + * + * `auth.service.ts`, `passkey.service.ts` and `oidc.service.ts` each carry a + * file-local, unexported `function toClientUser(row: UserRow): User { ... }` + * that maps the repository's full-row shape onto the client-facing `User` + * contract (`role` narrowed to `'admin' | 'user'`, four nullable columns + * coerced `T | null` -> `T | undefined` so `JSON.stringify` drops the key + * instead of emitting `null`). + * + * Plan 3b Task 7 review, M3 (reversing the Task 6 review's recommendation): + * a shared, exported helper would entrench that `?? undefined` coercion — + * the real fix is widening `User`'s optional fields to `T | null | + * undefined` and deleting all three copies, which is Plan 3c's own task + * with its own boot diff. Until then, CLAUDE.md's "guard the copy with a + * parity test that cannot silently skip" applies: this test reads the three + * files directly (source-scanning, the `mcp-gated-addons.parity.test.ts` + * precedent) and fails loudly the moment one copy drifts from the others. + * + * Exit condition for this test (delete it, not skip it): the `User` + * contract's optional fields are widened to `T | null | undefined` and the + * three `toClientUser` functions are deleted in favour of passing the + * repository row straight through. + */ +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'fs'; +import { join } from 'path'; + +const SRC = join(__dirname, '../../../src'); + +const COPIES = [ + join(SRC, 'nest/auth/auth.service.ts'), + join(SRC, 'nest/auth/passkey.service.ts'), + join(SRC, 'nest/oidc/oidc.service.ts'), +]; + +/** + * Extracts the body of the file-local `function toClientUser(...) { ... }` + * declaration, brace-matched (not regex-greedy) so the extraction survives + * unrelated formatting drift elsewhere in the file. + */ +function extractToClientUser(filePath: string): string { + const text = readFileSync(filePath, 'utf8'); + const start = text.indexOf('function toClientUser('); + if (start === -1) { + throw new Error(`toClientUser not found in ${filePath} — has it been renamed, exported, or deleted?`); + } + const braceStart = text.indexOf('{', start); + if (braceStart === -1) throw new Error(`toClientUser in ${filePath}: no opening brace found`); + let depth = 0; + let i = braceStart; + for (; i < text.length; i++) { + if (text[i] === '{') depth++; + else if (text[i] === '}') { + depth--; + if (depth === 0) break; + } + } + if (depth !== 0) throw new Error(`toClientUser in ${filePath}: unbalanced braces`); + // The signature + body together, normalised of surrounding whitespace so a + // trailing-newline-only diff between files doesn't fail this test. + return text.slice(start, i + 1).trim(); +} + +describe('toClientUser parity', () => { + it('TOCLIENTUSER-PARITY-001: all three copies are byte-identical', () => { + const bodies = COPIES.map((f) => ({ file: f, body: extractToClientUser(f) })); + const [first, ...rest] = bodies; + const mismatched = rest.filter((b) => b.body !== first.body); + expect( + mismatched.map((m) => m.file), + mismatched.length + ? `toClientUser drifted between copies:\n${bodies.map((b) => `--- ${b.file} ---\n${b.body}`).join('\n\n')}` + : undefined, + ).toEqual([]); + }); + + it('TOCLIENTUSER-PARITY-002: the body maps exactly the four documented nullable-to-undefined columns plus role', () => { + const body = extractToClientUser(COPIES[0]); + expect(body).toContain("role: row.role === 'admin' ? 'admin' : 'user'"); + expect(body).toContain('mfa_enabled: row.mfa_enabled ?? undefined'); + expect(body).toContain('must_change_password: row.must_change_password ?? undefined'); + expect(body).toContain('created_at: row.created_at ?? undefined'); + expect(body).toContain('updated_at: row.updated_at ?? undefined'); + }); +}); diff --git a/server/tests/unit/nest/trek-ws-adapter.test.ts b/server/tests/unit/nest/trek-ws-adapter.test.ts index 4ecac6815..a2e32ae62 100644 --- a/server/tests/unit/nest/trek-ws-adapter.test.ts +++ b/server/tests/unit/nest/trek-ws-adapter.test.ts @@ -313,6 +313,46 @@ describe('TrekWsAdapter D6 request context on connect (Plan 3b Task 0)', () => { await t.close(); } }); + + // Task 7 review, A-L2 (T1-F7b, open since Task 1): handleConnection is + // async and the 'connection' listener is not — a rejection inside the + // wrapped callback used to be an unhandled rejection with no trace. + it('WSAD-054: a rejecting connection callback is caught and logged through the adapter\'s error path, not an unhandled rejection', async () => { + const t = await createTestOrm(testDb, { allowGlobalContext: false }); + try { + logError.mockClear(); + const ad = new TrekWsAdapter({} as HttpServer, t.orm); + const socket = fakeSocket(); + ad.bindClientConnect(connectServer(socket, { url: '/ws?token=abc' }) as never, async () => { + throw new Error('handleConnection blew up'); + }); + // The listener itself must not throw synchronously (the rejection + // surfaces asynchronously, through .catch(), not as a thrown error). + await new Promise((resolve) => setImmediate(resolve)); + expect(logError).toHaveBeenCalledWith(expect.stringContaining('handleConnection blew up')); + } finally { + await t.close(); + } + }); + + // Coverage: the `err instanceof Error ? … : String(err)` fallback branch, + // never exercised by WSAD-054's real Error throw. + it('WSAD-054b: a connection callback rejecting with a non-Error value is still caught and logged (String(err) fallback)', async () => { + const t = await createTestOrm(testDb, { allowGlobalContext: false }); + try { + logError.mockClear(); + const ad = new TrekWsAdapter({} as HttpServer, t.orm); + const socket = fakeSocket(); + ad.bindClientConnect(connectServer(socket, { url: '/ws?token=abc' }) as never, async () => { + // Deliberately a non-Error throw, to prove the String(err) fallback. + throw 'not-an-error-value'; + }); + await new Promise((resolve) => setImmediate(resolve)); + expect(logError).toHaveBeenCalledWith(expect.stringContaining('not-an-error-value')); + } finally { + await t.close(); + } + }); }); /** @@ -361,6 +401,43 @@ describe('TrekWsAdapter D6 request context on disconnect (Task 0 review addendum await t.close(); } }); + + // Task 7 review, A-L2 (T1-F7b) — the disconnect twin of WSAD-054. + it('WSAD-055: a rejecting disconnect callback is caught and logged through the adapter\'s error path, not an unhandled rejection', async () => { + const t = await createTestOrm(testDb, { allowGlobalContext: false }); + try { + logError.mockClear(); + const ad = new TrekWsAdapter({} as HttpServer, t.orm); + const socket = fakeSocket(); + ad.bindClientDisconnect(socket as never, async () => { + throw new Error('handleDisconnect blew up'); + }); + socket.emit('close', Buffer.from('')); + await new Promise((resolve) => setImmediate(resolve)); + expect(logError).toHaveBeenCalledWith(expect.stringContaining('handleDisconnect blew up')); + } finally { + await t.close(); + } + }); + + // Coverage: the disconnect twin of WSAD-054b's String(err) fallback branch. + it('WSAD-055b: a disconnect callback rejecting with a non-Error value is still caught and logged (String(err) fallback)', async () => { + const t = await createTestOrm(testDb, { allowGlobalContext: false }); + try { + logError.mockClear(); + const ad = new TrekWsAdapter({} as HttpServer, t.orm); + const socket = fakeSocket(); + ad.bindClientDisconnect(socket as never, async () => { + // Deliberately a non-Error throw, to prove the String(err) fallback. + throw 'not-an-error-value-disconnect'; + }); + socket.emit('close', Buffer.from('')); + await new Promise((resolve) => setImmediate(resolve)); + expect(logError).toHaveBeenCalledWith(expect.stringContaining('not-an-error-value-disconnect')); + } finally { + await t.close(); + } + }); }); /**