From 44cf31c9585ee5e5cea398c8d3bff995d4aa0c48 Mon Sep 17 00:00:00 2001 From: jubnl Date: Tue, 22 Sep 2026 21:08:06 +0200 Subject: [PATCH] fix(server): non-numeric route ids answer the legacy not-found, never NaN (tokens, categories, tags); mint path back to two statements; identity-map regression pinned --- .../db/repositories/McpTokens.repository.ts | 50 ++++-- .../src/nest/categories/categories.service.ts | 37 +++- server/src/nest/common/row-id.ts | 38 ++++ server/src/nest/tags/tags.service.ts | 38 +++- server/src/nest/tokens/token.service.ts | 30 +++- server/tests/integration/admin.test.ts | 10 ++ server/tests/integration/auth.test.ts | 18 ++ server/tests/integration/categories.test.ts | 19 ++ server/tests/integration/mcp.test.ts | 65 ++++--- server/tests/integration/tags.test.ts | 19 ++ .../repositories/McpTokens.repository.test.ts | 167 +++++++++++++++--- server/tests/unit/nest/common/row-id.test.ts | 49 +++++ server/tests/unit/nest/token.service.test.ts | 29 ++- 13 files changed, 470 insertions(+), 99 deletions(-) create mode 100644 server/src/nest/common/row-id.ts create mode 100644 server/tests/unit/nest/common/row-id.test.ts diff --git a/server/src/db/repositories/McpTokens.repository.ts b/server/src/db/repositories/McpTokens.repository.ts index 80d5985b6..18664ac04 100644 --- a/server/src/db/repositories/McpTokens.repository.ts +++ b/server/src/db/repositories/McpTokens.repository.ts @@ -1,5 +1,5 @@ import { McpTokens } from '../entities/McpTokens.entity'; -import { toRow, type AssertRowKeys } from './_shared/rows'; +import type { AssertRowKeys } from './_shared/rows'; import { columnRef, currentTimestamp } from '../dialect/sql-functions'; import { EntityRepository } from '@mikro-orm/sql'; @@ -169,12 +169,21 @@ export class McpTokensRepository extends EntityRepository { * transaction (a later task's caller), a `flush()` here would commit * whatever else that transaction was still assembling. `em.insert()` fires * one native INSERT with no side effects on the context/identity map, - * matching `AuditLogRepository.insertEntry`'s precedent. Followed by a - * `disableIdentityMap: true` re-select (D4's create rule, this - * repository's isolated-read shape) rather than `em.refresh`, which needs - * a managed entity `em.insert` never produces. + * matching `AuditLogRepository.insertEntry`'s precedent. + * + * Returns only the generated `id` (`EntityManager#insert`'s own return + * type, `Primary` — a bare `number` for this entity's PK, + * confirmed against the installed 7.2.1 typings), not a re-selected row + * (Plan 3b Task 2 review, F4): an earlier version re-queried the full row + * here via a `disableIdentityMap: true` `findOne` and discarded it, which + * made `TokenService.createToken`'s own follow-up `findBasic` call a + * *third* statement for what the legacy code did in two (INSERT, then one + * re-select) — and the discarded middle read was the only place on the + * mint path that loaded `token_hash` into memory for nothing. `findBasic` + * (TK4) stays the one post-insert read; this method fires the INSERT and + * nothing else. */ - async insertToken(row: NewMcpTokenRow): Promise { + async insertToken(row: NewMcpTokenRow): Promise<{ id: number }> { const id = await this.getEntityManager().insert(McpTokens, { user: row.user_id, name: row.name, @@ -184,23 +193,30 @@ export class McpTokensRepository extends EntityRepository { scope_mode: row.scope_mode, api_scopes: row.api_scopes, }); - const inserted = await this.findOne({ id }, { disableIdentityMap: true }); - if (!inserted) { - throw new Error(`McpTokensRepository.insertToken: row ${String(id)} not found immediately after insert`); - } - return toRow(inserted) as McpTokenRow; + return { id }; } /** * `SELECT id, name, token_prefix, created_at, last_used_at FROM * mcp_tokens WHERE id = ?` (TK4, `createToken`'s re-select) and * `SELECT id, user_id FROM mcp_tokens WHERE id = ?` (TK9, - * `adminDeleteMcpToken`'s 404 check) — ONE method: TK9's two columns - * (`id`, `user_id`) are a subset of TK4's five, and both statements are a - * plain by-id lookup with no other filter, so a single narrower-than-`*` - * projection covering the union of both column sets serves both call - * sites without a second near-identical statement (same economy as - * `deleteById` below covering TK6/TK10). PK-only `findOne` -> + * `adminDeleteMcpToken`'s 404 check) — ONE method, but NOT the same + * byte-identical-statement merge `deleteById` below does for TK6/TK10 (a + * real D4 deviation, corrected here per the Plan 3b Task 2 review, F3): + * TK4 and TK9 select two genuinely different column sets (`id, name, + * token_prefix, created_at, last_used_at` vs. `id, user_id`), and this + * method returns their UNION — a superset projection that answers both + * call shapes rather than one of them plus an extra column nobody asked + * for. That is safe here because the client-visible payload is unchanged + * (`createToken` destructures `user_id` back out before responding, and + * `adminDeleteMcpToken` reads only `user_id`) and both call sites are + * plain by-id lookups with no other filter — two near-identical + * `findOne`s here is exactly the Sonar-duplication shape the plan warns + * about. This precedent does NOT transfer automatically to two methods + * whose difference is an order of magnitude larger (e.g. `SELECT *` vs. a + * narrow named set) — evaluate the actual column-set delta each time, + * don't cite this merge to justify collapsing a bigger gap. PK-only + * `findOne` -> * `disableIdentityMap: true`, NOT `refresh: true` (controller ruling on * Task 1's review, superseding Plan 3a's "PK-only `findOne` -> `refresh: * true`" ruling everywhere in this repository): `refresh` still re-queries diff --git a/server/src/nest/categories/categories.service.ts b/server/src/nest/categories/categories.service.ts index 896623303..b201288e5 100644 --- a/server/src/nest/categories/categories.service.ts +++ b/server/src/nest/categories/categories.service.ts @@ -3,6 +3,7 @@ import { InjectRepository } from '@mikro-orm/nestjs'; import type { Category } from '@trek/shared'; import { Categories } from '../../db/entities/Categories.entity'; import type { CategoriesRepository } from '../../db/repositories/Categories.repository'; +import { toRowId } from '../common/row-id'; /** * Categories domain service — owns the category business rules (moved off @@ -27,7 +28,16 @@ export class CategoriesService { } async getById(id: string | number): Promise { - const category = await this.categories.findById(Number(id)); + // Convert, VALIDATE, and answer the legacy not-found before any + // repository call (program rule 15): the legacy raw-SQL statement bound + // `id` straight into `WHERE id = ?` and let SQLite's affinity rules miss + // on a non-numeric string, returning `undefined`; a typed repository + // filter has no such leniency — a bare `Number()` renders `NaN` as an + // unquoted column reference and 500s (Plan 3b Task 2 review, F1's class + // of bug, same fix applied here per the fix round's item 1b). + const rowId = toRowId(id); + if (rowId === null) return undefined; + const category = await this.categories.findById(rowId); return (category as unknown as Category | null) ?? undefined; } @@ -51,14 +61,27 @@ export class CategoriesService { if (name) changes.name = name; if (color) changes.color = color; if (icon) changes.icon = icon; - // `patch` returns `null` for a non-existent id (Task 3 review, Minor 2); - // the legacy re-select returned `undefined` for the same case, and every - // caller pre-checks with a 404 first, so `?? undefined` restores that - // exact parity for the race-condition path the pre-check doesn't cover. - return ((await this.categories.patch(Number(id), changes)) ?? undefined) as unknown as Category; + // Every caller pre-checks with `getById` (a 404 for both a missing row + // and a non-numeric id) first, so this path only runs for an id already + // known valid — but a non-numeric id here still must not reach the + // repository as `NaN` (F1's class of bug): the legacy `UPDATE ... WHERE + // id = ?` bound the raw string, matched nothing via SQLite's affinity + // rules, and the re-select likewise returned `undefined`. `toRowId` + // reproduces that exact no-op for the race-condition path the pre-check + // doesn't cover, and `patch` returns `null` for a non-existent numeric + // id (Task 3 review, Minor 2) — both fold into the same `?? undefined` + // parity the legacy re-select had. + const rowId = toRowId(id); + if (rowId === null) return undefined as unknown as Category; + return ((await this.categories.patch(rowId, changes)) ?? undefined) as unknown as Category; } async remove(id: string | number): Promise { - await this.categories.remove(Number(id)); + // Same guard: a non-numeric id must no-op (matching the legacy `DELETE + // ... WHERE id = ?` binding a string that never matched any row) rather + // than reach the repository as `NaN`. + const rowId = toRowId(id); + if (rowId === null) return; + await this.categories.remove(rowId); } } diff --git a/server/src/nest/common/row-id.ts b/server/src/nest/common/row-id.ts new file mode 100644 index 000000000..58e07a924 --- /dev/null +++ b/server/src/nest/common/row-id.ts @@ -0,0 +1,38 @@ +/** + * The one place an external, string-shaped route/tool id is turned into a + * numeric primary key before it reaches a typed repository filter. + * + * The legacy raw-SQL layer bound a route param straight into `WHERE id = ?` + * and let SQLite's column-affinity rules do the work: a well-formed numeric + * string ('5', ' 5', '5.0') converts and matches, anything else ('abc', '', + * '0x10', '1e3') stays TEXT, can never equal an INTEGER column, and the + * lookup simply misses — answered with the route's ordinary not-found. A + * typed MikroORM filter has no such affinity: `Number('abc')` is `NaN`, and + * the ORM renders that as the bare, unquoted token `NaN` in the generated + * SQL, which SQLite parses as a column reference and throws + * `InvalidFieldNameException: no such column: NaN` — a 500 where the legacy + * route 404'd (Plan 3b Task 2 review, F1). + * + * `toRowId` is the guard every converted service calls instead of a bare + * `Number(id)`: it accepts only what the legacy binding actually matched — + * digits only, no sign, no decimal point, no exponent, no hex — and returns + * `null` for everything else, which the caller answers with the route's own + * legacy not-found result BEFORE any repository call (program rule 15, + * `docs/superpowers/plans/2026-09-21-orm-migration-program.md`). + * + * A `/^\d+$/` string match is deliberately stricter than `Number.isSafeInteger` + * alone: `Number('0x10')` is `16` and `Number('1e3')` is `1000`, both safe + * integers a bare `Number()` conversion would have accepted even though + * SQLite's affinity conversion never would have matched either shape against + * an INTEGER column. Requiring the digits-only shape first closes that gap. + */ +export function toRowId(value: unknown): number | null { + if (typeof value === 'number') { + return Number.isSafeInteger(value) ? value : null; + } + if (typeof value === 'string' && /^\d+$/.test(value)) { + const parsed = Number(value); + return Number.isSafeInteger(parsed) ? parsed : null; + } + return null; +} diff --git a/server/src/nest/tags/tags.service.ts b/server/src/nest/tags/tags.service.ts index c20f2d9f3..ce5bbce58 100644 --- a/server/src/nest/tags/tags.service.ts +++ b/server/src/nest/tags/tags.service.ts @@ -3,6 +3,7 @@ import { InjectRepository } from '@mikro-orm/nestjs'; import type { Tag } from '@trek/shared'; import { Tags } from '../../db/entities/Tags.entity'; import type { TagsRepository } from '../../db/repositories/Tags.repository'; +import { toRowId } from '../common/row-id'; /** * Tags domain service — owns the tag business rules (moved off @@ -27,7 +28,17 @@ export class TagsService { } async getByIdAndUser(id: string | number, userId: number): Promise { - const tag = await this.tags.findByIdAndUser(Number(id), userId); + // Convert, VALIDATE, and answer the legacy not-found before any + // repository call (program rule 15): the legacy raw-SQL statement bound + // `id` straight into `WHERE id = ? AND user_id = ?` and let SQLite's + // affinity rules miss on a non-numeric string, returning `undefined`; a + // typed repository filter has no such leniency — a bare `Number()` + // renders `NaN` as an unquoted column reference and 500s (Plan 3b Task 2 + // review, F1's class of bug, same fix applied here per the fix round's + // item 1b). + const rowId = toRowId(id); + if (rowId === null) return undefined; + const tag = await this.tags.findByIdAndUser(rowId, userId); return (tag as unknown as Tag | null) ?? undefined; } @@ -49,14 +60,27 @@ export class TagsService { const changes: { name?: string; color?: string } = {}; if (name) changes.name = name; if (color) changes.color = color; - // `patch` returns `null` for a non-existent id (Task 3 review, Minor 2); - // the legacy re-select returned `undefined` for the same case, and every - // caller pre-checks with a 404 first, so `?? undefined` restores that - // exact parity for the race-condition path the pre-check doesn't cover. - return ((await this.tags.patch(Number(id), changes)) ?? undefined) as unknown as Tag; + // Every caller pre-checks with `getByIdAndUser` (a 404 for both a + // missing/foreign row and a non-numeric id) first, so this path only + // runs for an id already known valid — but a non-numeric id here still + // must not reach the repository as `NaN` (F1's class of bug): the + // legacy `UPDATE ... WHERE id = ?` bound the raw string, matched + // nothing via SQLite's affinity rules, and the re-select likewise + // returned `undefined`. `toRowId` reproduces that exact no-op for the + // race-condition path the pre-check doesn't cover, and `patch` returns + // `null` for a non-existent numeric id (Task 3 review, Minor 2) — both + // fold into the same `?? undefined` parity the legacy re-select had. + const rowId = toRowId(id); + if (rowId === null) return undefined as unknown as Tag; + return ((await this.tags.patch(rowId, changes)) ?? undefined) as unknown as Tag; } async remove(id: string | number): Promise { - await this.tags.remove(Number(id)); + // Same guard: a non-numeric id must no-op (matching the legacy `DELETE + // ... WHERE id = ?` binding a string that never matched any row) rather + // than reach the repository as `NaN`. + const rowId = toRowId(id); + if (rowId === null) return; + await this.tags.remove(rowId); } } diff --git a/server/src/nest/tokens/token.service.ts b/server/src/nest/tokens/token.service.ts index 2287bee66..70a1addec 100644 --- a/server/src/nest/tokens/token.service.ts +++ b/server/src/nest/tokens/token.service.ts @@ -11,6 +11,7 @@ import type { McpTokensRepository, McpTokenBasicRow } from '../../db/repositorie import { Users } from '../../db/entities/Users.entity'; import type { UsersRepository } from '../../db/repositories/Users.repository'; import { EphemeralTokenService } from '../auth/ephemeral-token.service'; +import { toRowId } from '../common/row-id'; // Import from sessionManager directly, NOT the ../../mcp barrel: the barrel pulls // the whole tools fan-out (and via the domain bridges, the Nest services) into // every consumer of this module — a nest→mcp→nest module cycle. @@ -133,10 +134,11 @@ export class TokenService { }); // A separate re-select, matching the legacy INSERT-then-SELECT shape - // exactly (TK3 then TK4) rather than reusing `inserted`'s full row — - // `findBasic` also serves TK9's admin lookup, so its `user_id` field is - // dropped here: TK4's response never carried it, and leaking it would be - // a new field on the client-facing token payload, not a refactor. + // exactly (TK3 then TK4 — two statements, `insertToken` returns only + // the generated id and never re-queries the row itself) — `findBasic` + // also serves TK9's admin lookup, so its `user_id` field is dropped + // here: TK4's response never carried it, and leaking it would be a new + // field on the client-facing token payload, not a refactor. const basic = (await this.tokens.findBasic(inserted.id)) as McpTokenBasicRow; const { user_id: _userId, ...token } = basic; @@ -160,7 +162,14 @@ export class TokenService { * missing from a screen they never opened. */ private async deleteToken(userId: number, tokenId: string, kind: TokenKind): Promise<{ error?: string; status?: number; success?: boolean }> { - const id = Number(tokenId); + // Convert, VALIDATE, and answer the legacy not-found before any + // repository call (program rule 15): the legacy statement bound + // `tokenId` straight into `WHERE id = ?` and let SQLite's affinity rules + // miss on a non-numeric string; a typed repository filter has no such + // leniency, so a bare `Number()` turned a 404 into a 500 (Plan 3b Task 2 + // review, F1). + const id = toRowId(tokenId); + if (id === null) return { error: 'Token not found', status: 404 }; const token = await this.tokens.findOwnedByKind(id, userId, kind); if (!token) return { error: 'Token not found', status: 404 }; await this.tokens.deleteById(id); @@ -197,7 +206,7 @@ export class TokenService { // Admin view // // The same table, seen across all users. Moved here from AdminService, which - // only owned a second copy of the delete purely because the admin route lived + // owned a second copy of the delete purely because the admin route lived // there. Note the two are genuinely different queries, not duplicates: the // user-facing ones scope every statement by user_id, these deliberately do // not, and the admin delete revokes sessions unconditionally where the @@ -209,7 +218,10 @@ export class TokenService { } async adminDeleteMcpToken(id: string) { - const numericId = Number(id); + // Same guard as `deleteToken` above — convert, VALIDATE, answer the + // legacy 404 before any repository call (F1). + const numericId = toRowId(id); + if (numericId === null) return { error: 'Token not found', status: 404 }; const token = await this.tokens.findBasic(numericId); if (!token) return { error: 'Token not found', status: 404 }; await this.tokens.deleteById(numericId); @@ -260,8 +272,8 @@ export class TokenService { if (row) { await this.tokens.touchLastUsedByHash(hash); // `role` is `users.role TEXT`, narrower at runtime than the repository's - // row type states — same trust boundary the legacy `this.db.get()` - // generic asserted without a runtime check. + // row type states — same trust boundary the pre-ORM raw-SQL lookup's + // generic type parameter asserted without a runtime check. return row as User; } return null; diff --git a/server/tests/integration/admin.test.ts b/server/tests/integration/admin.test.ts index 9f8875ed8..67e6d2a5c 100644 --- a/server/tests/integration/admin.test.ts +++ b/server/tests/integration/admin.test.ts @@ -778,6 +778,16 @@ describe('MCP token management', () => { .set('Cookie', authCookie(admin.id)); expect(res.status).toBe(404); }); + + it('ADMIN-025 — DELETE /admin/mcp-tokens/abc (non-numeric id) returns the legacy 404, not a 500 (Plan 3b Task 2 review, F1)', async () => { + const { user: admin } = createAdmin(testDb); + + const res = await request(app) + .delete('/api/admin/mcp-tokens/abc') + .set('Cookie', authCookie(admin.id)); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Token not found' }); + }); }); // ───────────────────────────────────────────────────────────────────────────── diff --git a/server/tests/integration/auth.test.ts b/server/tests/integration/auth.test.ts index 84e18e091..77635233e 100644 --- a/server/tests/integration/auth.test.ts +++ b/server/tests/integration/auth.test.ts @@ -890,4 +890,22 @@ describe('MCP token management', () => { const res = await request(app).get('/api/auth/mcp-tokens'); expect(res.status).toBe(401); }); + + it('AUTH-040 — DELETE /auth/mcp-tokens/abc (non-numeric id) returns the legacy 404, not a 500 (Plan 3b Task 2 review, F1)', async () => { + const { user } = createUser(testDb); + const res = await request(app) + .delete('/api/auth/mcp-tokens/abc') + .set('Cookie', authCookie(user.id)); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Token not found' }); + }); + + it('AUTH-041 — DELETE /auth/api-tokens/abc (non-numeric id) returns the legacy 404, not a 500 (Plan 3b Task 2 review, F1)', async () => { + const { user } = createUser(testDb); + const res = await request(app) + .delete('/api/auth/api-tokens/abc') + .set('Cookie', authCookie(user.id)); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Token not found' }); + }); }); diff --git a/server/tests/integration/categories.test.ts b/server/tests/integration/categories.test.ts index 847bf8462..4a928df0d 100644 --- a/server/tests/integration/categories.test.ts +++ b/server/tests/integration/categories.test.ts @@ -161,4 +161,23 @@ describe('Categories', () => { const res = await request(app).get('/api/categories'); expect(res.status).toBe(401); }); + + it('CAT-011: PUT /api/categories/abc - non-numeric id returns the legacy 404, not a 500 (Plan 3b Task 2 fix round, item 1b)', async () => { + const { user: admin } = createAdmin(testDb); + const res = await request(app) + .put('/api/categories/abc') + .set('Cookie', authCookie(admin.id)) + .send({ name: 'Ghost' }); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Category not found' }); + }); + + it('CAT-012: DELETE /api/categories/abc - non-numeric id returns the legacy 404, not a 500 (Plan 3b Task 2 fix round, item 1b)', async () => { + const { user: admin } = createAdmin(testDb); + const res = await request(app) + .delete('/api/categories/abc') + .set('Cookie', authCookie(admin.id)); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Category not found' }); + }); }); diff --git a/server/tests/integration/mcp.test.ts b/server/tests/integration/mcp.test.ts index 4272fdc10..0b7000fb3 100644 --- a/server/tests/integration/mcp.test.ts +++ b/server/tests/integration/mcp.test.ts @@ -37,16 +37,16 @@ import { setPluginMcpToolSource } from '../../src/plugin-mcp-tools'; import type { McpDynamicTool } from '../../src/nest-mcp'; import { getMcpSafeUrl } from '../../src/app-config'; import { OauthService } from '../../src/nest/oauth/oauth.service'; -import { DatabaseService } from '../../src/nest/database/database.service'; import { createTestAddonsService } from '../helpers/test-addons'; import { AuditService } from '../../src/nest/audit/audit.service'; import { createTestOrm, type TestOrm } from '../helpers/test-orm'; import { AuditLog } from '../../src/db/entities/AuditLog.entity'; import { Users } from '../../src/db/entities/Users.entity'; -import { TokenService } from '../../src/nest/tokens/token.service'; +import { OauthClients } from '../../src/db/entities/OauthClients.entity'; +import { OauthTokens } from '../../src/db/entities/OauthTokens.entity'; +import { OauthConsents } from '../../src/db/entities/OauthConsents.entity'; import { MikroORM } from '@mikro-orm/core'; -const oauthDbs = new DatabaseService(testDb); let oauthSvc: OauthService; /** Mint a trekoa_ access token for the user via a fresh OAuth client. */ @@ -76,7 +76,7 @@ beforeAll(async () => { nestApp = await buildApp(); app = nestApp.getHttpAdapter().getInstance(); t = await createTestOrm(testDb); - oauthSvc = new OauthService(oauthDbs, await createTestAddonsService(testDb, oauthDbs), new AuditService(t.repo(AuditLog), t.repo(Users))); + oauthSvc = new OauthService(t.repo(OauthClients), t.repo(OauthTokens), t.repo(OauthConsents), await createTestAddonsService(testDb), new AuditService(t.repo(AuditLog), t.repo(Users))); }); beforeEach(() => { @@ -769,10 +769,15 @@ describe('MCP rate limiting', () => { * (`MikroOrmModule.forRoot`'s default `registerRequestContext`, registered * via `NestModule.configure()`/`forRoutes(ALL)`) applies to it exactly like * every other route, BEFORE any guard runs. Verified here rather than - * assumed: `TokenService.verifyMcpToken` (TK13 in the inventory) is not yet - * repository-backed (Task 2), so this forces a genuine repository read at - * the exact point that method will occupy once converted, through the same - * DI-provided `TokenService` singleton the real `/mcp` route calls. + * assumed: `TokenService.verifyMcpToken` (TK13 in the inventory) is now + * repository-backed (`McpTokensRepository.findUserByHashAndKind` + + * `touchLastUsedByHash`, Plan 3b Task 2) — MCP-CTX-002 drives a real `trek_` + * bearer token through the genuine DI-provided `TokenService` singleton the + * `/mcp` route calls (no mock of `verifyMcpToken` itself, per the Task 2 + * review's F5: a spy on the very method under test does not exercise the + * repository path it exists to guard) and asserts the repository's write + * (`last_used_at`) actually moved, which is only possible if the read ran + * inside the forked context. */ describe('MCP bearer-token verification runs inside the HTTP request context (Plan 3b Task 0, D6)', () => { it('MCP-CTX-001: outside of any request, the SAME ORM refuses a query (allowGlobalContext: false is genuinely enforced, not just permissively configured)', async () => { @@ -780,32 +785,26 @@ describe('MCP bearer-token verification runs inside the HTTP request context (Pl await expect(orm.em.getRepository(Users).findOne({ id: 1 })).rejects.toThrow(/context/i); }); - it('MCP-CTX-002: a repository read forced inside verifyMcpToken (the trek_ bearer-token branch) succeeds — the /mcp route already forks a request context before the auth step runs', async () => { + it('MCP-CTX-002: a repository read inside verifyMcpToken (the trek_ bearer-token branch) succeeds — the /mcp route already forks a request context before the auth step runs', async () => { testDb.prepare("UPDATE addons SET enabled = 1 WHERE id = 'mcp'").run(); const { user } = createUser(testDb); - const orm = nestApp.get(MikroORM); - const tokenService = nestApp.get(TokenService); - let found: unknown; - let caught: unknown; - const spy = vi.spyOn(tokenService, 'verifyMcpToken').mockImplementation(async () => { - try { - found = await orm.em.getRepository(Users).findOne({ id: user.id }); - } catch (e) { - caught = e; - } - return found ? { id: user.id, username: user.username, email: user.email, role: user.role } : null; - }); - try { - const res = await request(app) - .post('/mcp') - .set('Authorization', 'Bearer trek_forces_a_repository_read') - .set('Accept', 'application/json, text/event-stream') - .send({ jsonrpc: '2.0', method: 'initialize', id: 1, params: { protocolVersion: '2024-11-05', capabilities: {}, clientInfo: { name: 'test', version: '1' } } }); - expect(caught).toBeUndefined(); - expect(found).toBeTruthy(); - expect(res.status).toBe(200); - } finally { - spy.mockRestore(); - } + const { rawToken, id: tokenId } = createMcpToken(testDb, user.id); + const before = (testDb.prepare('SELECT last_used_at FROM mcp_tokens WHERE id = ?').get(tokenId) as { last_used_at: string | null }).last_used_at; + expect(before).toBeNull(); + + const res = await request(app) + .post('/mcp') + .set('Authorization', `Bearer ${rawToken}`) + .set('Accept', 'application/json, text/event-stream') + .send({ jsonrpc: '2.0', method: 'initialize', id: 1, params: { protocolVersion: '2024-11-05', capabilities: {}, clientInfo: { name: 'test', version: '1' } } }); + expect(res.status).toBe(200); + + // The real `TokenService.verifyMcpToken` ran `McpTokensRepository`'s + // `findUserByHashAndKind` (TK13) then `touchLastUsedByHash` (TK14) — + // `last_used_at` moving is only possible if both reached the DB, which + // MCP-CTX-001 proves the ORM refuses outside a forked request context. + const after = (testDb.prepare('SELECT last_used_at FROM mcp_tokens WHERE id = ?').get(tokenId) as { last_used_at: string | null }).last_used_at; + expect(after).not.toBeNull(); + expect(after).not.toBe(before); }); }); diff --git a/server/tests/integration/tags.test.ts b/server/tests/integration/tags.test.ts index c4fe6f48e..2114000bc 100644 --- a/server/tests/integration/tags.test.ts +++ b/server/tests/integration/tags.test.ts @@ -177,4 +177,23 @@ describe('Tags', () => { const res = await request(app).get('/api/tags'); expect(res.status).toBe(401); }); + + it('TAG-011: PUT /api/tags/abc - non-numeric id returns the legacy 404, not a 500 (Plan 3b Task 2 fix round, item 1b)', async () => { + const { user } = createUser(testDb); + const res = await request(app) + .put('/api/tags/abc') + .set('Cookie', authCookie(user.id)) + .send({ name: 'Ghost' }); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Tag not found' }); + }); + + it('TAG-012: DELETE /api/tags/abc - non-numeric id returns the legacy 404, not a 500 (Plan 3b Task 2 fix round, item 1b)', async () => { + const { user } = createUser(testDb); + const res = await request(app) + .delete('/api/tags/abc') + .set('Cookie', authCookie(user.id)); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Tag not found' }); + }); }); diff --git a/server/tests/unit/db/repositories/McpTokens.repository.test.ts b/server/tests/unit/db/repositories/McpTokens.repository.test.ts index 187c408e7..11306a6b9 100644 --- a/server/tests/unit/db/repositories/McpTokens.repository.test.ts +++ b/server/tests/unit/db/repositories/McpTokens.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 { createHash } from 'crypto'; import { createSnapshotTestDb } from '../../../helpers/db-mock'; import { resetTestDb } from '../../../helpers/test-db'; @@ -66,9 +66,9 @@ describe('McpTokensRepository', () => { }); describe('insertToken', () => { - it('MCPTOKREPO-004: inserts and returns exactly the row a legacy re-select would return', async () => { + it('MCPTOKREPO-004: inserts the row a legacy re-select would return, and returns only the generated id (Plan 3b Task 2 review, F4)', async () => { const { user } = createUser(testDb); - const row = await tokens.insertToken({ + const inserted = await tokens.insertToken({ user_id: user.id, name: 'My Token', token_hash: 'hash-value', @@ -77,7 +77,12 @@ describe('McpTokensRepository', () => { scope_mode: 'all', api_scopes: null, }); - expect(row).toStrictEqual(rawToken(row.id)); + // Only `{ id }` comes back — no full-row re-select is folded into + // insertToken itself, so it cannot leak `token_hash` (or anything + // else) to a caller that only asked to mint a row. `findBasic` (TK4) + // stays the one place a caller reads the row back. + expect(Object.keys(inserted)).toEqual(['id']); + const row = rawToken(inserted.id) as Record; expect(row.user_id).toBe(user.id); expect(row.name).toBe('My Token'); expect(row.token_hash).toBe('hash-value'); @@ -90,7 +95,7 @@ describe('McpTokensRepository', () => { it('MCPTOKREPO-005: api_scopes is written when narrowed', async () => { const { user } = createUser(testDb); - const row = await tokens.insertToken({ + const inserted = await tokens.insertToken({ user_id: user.id, name: 'Narrowed', token_hash: 'hash-2', @@ -99,9 +104,46 @@ describe('McpTokensRepository', () => { scope_mode: 'limited', api_scopes: '["stats","trips"]', }); + const row = rawToken(inserted.id) as { scope_mode: string; api_scopes: string }; expect(row.scope_mode).toBe('limited'); expect(row.api_scopes).toBe('["stats","trips"]'); }); + + it('MCPTOKREPO-024: the mint path (insertToken + findBasic) issues exactly two statements and never selects token_hash (Plan 3b Task 2 review, F4)', async () => { + const { user } = createUser(testDb); + const statements: string[] = []; + const original = testDb.prepare.bind(testDb); + const spy = vi.spyOn(testDb, 'prepare').mockImplementation((sql: string) => { + statements.push(sql); + return original(sql); + }); + + let insertedId: number; + try { + const inserted = await tokens.insertToken({ + user_id: user.id, + name: 'Statement Count', + token_hash: 'hash-count', + token_prefix: 'trek_countpr', + kind: 'mcp', + scope_mode: 'all', + api_scopes: null, + }); + insertedId = inserted.id; + await tokens.findBasic(insertedId); + } finally { + spy.mockRestore(); + } + + expect(statements).toHaveLength(2); + expect(statements[0].toLowerCase()).toContain('insert into'); + expect(statements[1].toLowerCase()).toContain('select'); + // The INSERT necessarily names `token_hash` as a column to write; the + // point is that no SELECT on the mint path reads it back into memory — + // the discarded middle re-select an earlier version issued did. + const selects = statements.filter((sql) => sql.trim().toLowerCase().startsWith('select')); + for (const sql of selects) expect(sql.toLowerCase()).not.toContain('token_hash'); + }); }); describe('findBasic', () => { @@ -247,6 +289,35 @@ describe('McpTokensRepository', () => { it('MCPTOKREPO-018: an unknown hash is null', async () => { expect(await tokens.findUserByHashAndKind('nope', 'api')).toBeNull(); }); + + it('MCPTOKREPO-025: kind is genuinely inside the generated WHERE clause, not a post-filter on the result (Plan 3b Task 2 review, F6)', async () => { + const { user } = createUser(testDb); + createMcpToken(testDb, user.id, { kind: 'mcp', rawToken: 'trek_sql_shape' }); + const hash = createHash('sha256').update('trek_sql_shape').digest('hex'); + + const statements: string[] = []; + const original = testDb.prepare.bind(testDb); + const spy = vi.spyOn(testDb, 'prepare').mockImplementation((sql: string) => { + statements.push(sql); + return original(sql); + }); + try { + await tokens.findUserByHashAndKind(hash, 'mcp'); + } finally { + spy.mockRestore(); + } + + expect(statements).toHaveLength(1); + const select = statements[0].toLowerCase(); + const whereIndex = select.indexOf('where'); + expect(whereIndex).toBeGreaterThan(-1); + // `kind` must appear on the SQL side of the WHERE keyword — a + // post-filter shape (select every column, compare `kind` in JS) would + // still pass every behavioural test in this describe block (M1 in the + // review's mutation table: 56/56 green with kind moved to a JS + // check), so the SQL text itself is the only thing that can catch it. + expect(select.slice(whereIndex)).toContain('kind'); + }); }); describe('touchLastUsedByHash', () => { @@ -300,26 +371,34 @@ describe('McpTokensRepository', () => { // --------------------------------------------------------------------- describe('identity-map isolation (disableIdentityMap regression)', () => { /** - * The precise, mutation-proven form of the proof: `disableIdentityMap: - * true` means the returned entity is never merged into - * `EntityManager.getUnitOfWork()`'s tracked set at all — checked + * `disableIdentityMap: true` means the returned entity is never merged + * into `EntityManager.getUnitOfWork()`'s tracked set at all — checked * directly via `getById`, which is the same lookup MikroORM's own - * `flush()` consults to decide what to write. This is deliberately NOT - * a black-box "does a stale value get written back" behavioural test: - * hand-mutation-testing this repository (reverting `findBasic` to - * `refresh: true` and `findOwnedByKind`/`listByUserAndKind` to no - * `disableIdentityMap`, five different projection-pairing/nativeUpdate/ - * flush orderings tried by hand) never reproduced a stale write-back on - * the installed MikroORM 7.2.1 + `better-sqlite3` combination — flush()'s - * dirty-checking finds nothing to write for an entity nothing ever - * mutated via `assign()`/direct property set, which is exactly the - * shape this repository's methods have (`nativeUpdate` only, never - * `assign()+flush()`, unlike `UsersRepository.patchProfile`'s callers). - * `getById` is the one assertion that DOES fail on the un-fixed code - * (verified: reverting `findBasic` to `refresh: true` flips `tracked1` - * below from `false` to `true`) — the tracking itself is the hazard the - * ruling targets, whether or not this particular table's call shapes - * manage to turn it into an observable stale write today. + * `flush()` consults to decide what to write. + * + * CORRECTION (Plan 3b Task 2 review, F2): an earlier version of this + * docstring claimed the stale write-back this ruling guards against + * could not be reproduced as an observable black-box failure on this + * repository's own call shapes, and settled for this structural + * `getById` assertion alone. That claim was wrong — it IS reproducible. + * `MCPTOKREPO-023` below is the request-shaped reproduction: read + * projection A (`listByUserAndKind`, which selects `scope_mode`), read + * projection B (`findBasic`, which does not), then a `nativeUpdate` on + * `scope_mode` inside `uow.transactional`. On the fixed code (every read + * `disableIdentityMap: true`) the `nativeUpdate` survives the block's + * closing flush. Reverting `listByUserAndKind` to a bare `find` (no + * `disableIdentityMap`) and `findBasic` to `refresh: true` instead + * reproduces the exact hazard the ruling describes: both reads merge + * into the same managed entity, the `nativeUpdate` bypasses it, and the + * closing `flush()` writes the merged entity's stale `scope_mode: 'all'` + * back over the intended `'limited'` — `update mcp_tokens set + * scope_mode='all', api_scopes=null where id=?` observed immediately + * after `update mcp_tokens set scope_mode='limited' where id=?` in the + * query log, reverting the very write the test just made. `getById` + * (this test, `MCPTOKREPO-021`) stays as the structural companion: it is + * the assertion that fails first and pinpoints which method regressed, + * while `MCPTOKREPO-023` pins the actual end-to-end behaviour the + * ruling exists to protect. */ it('MCPTOKREPO-021: every entity-hydrating read leaves nothing behind in the shared identity map', async () => { const { user } = createUser(testDb); @@ -385,5 +464,45 @@ describe('McpTokensRepository', () => { expect((rawToken(untouched.id) as { name: string }).name).toBe('do-not-flush-me'); expect(testDb.prepare('SELECT id FROM mcp_tokens WHERE token_hash = ?').get('insert-no-side-effect-hash')).toBeDefined(); }); + + /** + * The request-shaped reproduction (Plan 3b Task 2 review, F2, shape D): + * projection A (`listByUserAndKind`, which selects `scope_mode`), + * projection B (`findBasic`, which does not), then a `nativeUpdate` on + * `scope_mode` — the column only projection A touched — inside + * `uow.transactional`. On the fixed code (every read + * `disableIdentityMap: true`) neither read is ever merged into the + * shared identity map, so the transaction's closing `flush()` has + * nothing stale to write and the `nativeUpdate` survives untouched. + * + * Mutation-proven by hand (not committed as a variant, per the brief — + * see the fix report for the transcript): reverting + * `listByUserAndKind`'s `find` to drop `disableIdentityMap` and + * `findBasic`'s `findOne` from `disableIdentityMap: true` to + * `refresh: true` makes this test fail — the row comes back + * `scope_mode: 'all'`, and the captured query log shows `update + * mcp_tokens set scope_mode='all', api_scopes=null where id=?` executed + * immediately AFTER `update mcp_tokens set scope_mode='limited' where + * id=?`, i.e. the transaction's closing flush silently reverting the + * intended write. + */ + it('MCPTOKREPO-023: projection A then projection B then a nativeUpdate on an A-only column, inside uow.transactional — the nativeUpdate survives the closing flush', async () => { + const { user } = createUser(testDb); + const created = createMcpToken(testDb, user.id, { name: 'stays-original', scope_mode: 'all' }); + const uow = new UnitOfWork(t.em); + + await uow.transactional(async () => { + // Projection A: selects `scope_mode` among its columns. + await tokens.listByUserAndKind(user.id, 'mcp'); + // Projection B: a disjoint column set that does NOT select `scope_mode`. + await tokens.findBasic(created.id); + // A native write on the column only projection A touched. + await t.em.nativeUpdate(McpTokens, { id: created.id }, { scope_mode: 'limited' }); + }); + + const row = rawToken(created.id) as { scope_mode: string; name: string }; + expect(row.scope_mode).toBe('limited'); + expect(row.name).toBe('stays-original'); + }); }); }); diff --git a/server/tests/unit/nest/common/row-id.test.ts b/server/tests/unit/nest/common/row-id.test.ts new file mode 100644 index 000000000..29412e9fd --- /dev/null +++ b/server/tests/unit/nest/common/row-id.test.ts @@ -0,0 +1,49 @@ +import { describe, it, expect } from 'vitest'; + +import { toRowId } from '../../../../src/nest/common/row-id'; + +describe('toRowId', () => { + it('ROWID-001: rejects a non-numeric string', () => { + expect(toRowId('abc')).toBeNull(); + }); + + it('ROWID-002: rejects a hex-shaped string — Number() would accept it, SQLite affinity never would', () => { + expect(toRowId('0x10')).toBeNull(); + }); + + it('ROWID-003: rejects an exponent-shaped string — Number() would accept it, SQLite affinity never would', () => { + expect(toRowId('1e3')).toBeNull(); + }); + + it('ROWID-004: rejects a negative sign', () => { + expect(toRowId('-1')).toBeNull(); + }); + + it('ROWID-005: rejects a leading space', () => { + expect(toRowId(' 1')).toBeNull(); + }); + + it('ROWID-006: rejects a decimal point', () => { + expect(toRowId('1.0')).toBeNull(); + }); + + it('ROWID-007: rejects an empty string', () => { + expect(toRowId('')).toBeNull(); + }); + + it('ROWID-008: rejects undefined', () => { + expect(toRowId(undefined)).toBeNull(); + }); + + it('ROWID-009: accepts a plain digit string', () => { + expect(toRowId('42')).toBe(42); + }); + + it('ROWID-010: accepts an already-numeric safe integer', () => { + expect(toRowId(42)).toBe(42); + }); + + it('ROWID-011: rejects a number one past MAX_SAFE_INTEGER (2**53)', () => { + expect(toRowId(2 ** 53)).toBeNull(); + }); +}); diff --git a/server/tests/unit/nest/token.service.test.ts b/server/tests/unit/nest/token.service.test.ts index 68e04cbf0..e072ae0d9 100644 --- a/server/tests/unit/nest/token.service.test.ts +++ b/server/tests/unit/nest/token.service.test.ts @@ -153,6 +153,16 @@ describe('MCP token service', () => { expect(await svc.deleteMcpToken(user.id, tokenId)).toEqual({ error: 'Token not found', status: 404 }); expect(testDb.prepare('SELECT id FROM mcp_tokens WHERE id = ?').get(tokenId)).toBeDefined(); }); + + it('TOKEN-017: deleteMcpToken 404s (not 500) on a non-numeric id — Plan 3b Task 2 review, F1', async () => { + const { user } = createUser(testDb); + expect(await svc.deleteMcpToken(user.id, 'abc')).toEqual({ error: 'Token not found', status: 404 }); + }); + + it('TOKEN-018: deleteMcpToken 404s on a hex-shaped id — Number("0x10") is 16, a safe integer a bare Number() conversion would accept, but SQLite affinity never would (Plan 3b Task 2 review, F1)', async () => { + const { user } = createUser(testDb); + expect(await svc.deleteMcpToken(user.id, '0x10')).toEqual({ error: 'Token not found', status: 404 }); + }); }); // --------------------------------------------------------------------------- @@ -267,10 +277,15 @@ describe('MCP token service (admin view)', () => { await svc.createMcpToken(user.id, 'a'); await svc.createMcpToken(other.id, 'b'); - const all = await svc.listAllMcpTokens() as unknown as Record[]; + // `listAllMcpTokens` returns the repository's own typed + // `McpTokenWithUsernameRow[]` now (Plan 3b Task 2) — no cast needed + // (Task 2 review, F7: the earlier `as unknown as Record[]` was a double cast kept only to compile against the old + // `unknown` return type). + const all = await svc.listAllMcpTokens(); expect(all).toHaveLength(2); expect(all.every(t => typeof t.username === 'string')).toBe(true); - expect(all.some(t => t.token_hash !== undefined)).toBe(false); + expect(all.every(t => !('token_hash' in t))).toBe(true); }); it('TOKEN-004: adminDeleteMcpToken removes any user token and revokes that user', async () => { @@ -287,6 +302,16 @@ describe('MCP token service (admin view)', () => { expect(await svc.adminDeleteMcpToken('99999')).toEqual({ error: 'Token not found', status: 404 }); expect(revokeUserSessions).not.toHaveBeenCalled(); }); + + it('TOKEN-019: adminDeleteMcpToken 404s (not 500) on a non-numeric id, without revoking anyone — Plan 3b Task 2 review, F1', async () => { + expect(await svc.adminDeleteMcpToken('abc')).toEqual({ error: 'Token not found', status: 404 }); + expect(revokeUserSessions).not.toHaveBeenCalled(); + }); + + it('TOKEN-020: adminDeleteMcpToken 404s on a hex-shaped id (Plan 3b Task 2 review, F1)', async () => { + expect(await svc.adminDeleteMcpToken('0x10')).toEqual({ error: 'Token not found', status: 404 }); + expect(revokeUserSessions).not.toHaveBeenCalled(); + }); }); // ---------------------------------------------------------------------------