diff --git a/server/src/db/repositories/TodoItems.repository.ts b/server/src/db/repositories/TodoItems.repository.ts index 0d1cdf790..3c6d023c8 100644 --- a/server/src/db/repositories/TodoItems.repository.ts +++ b/server/src/db/repositories/TodoItems.repository.ts @@ -98,14 +98,21 @@ export class TodoItemsRepository extends TrekRepository { return await this.kysely().selectFrom('todo_items').selectAll().where('id', '=', id as number).executeTakeFirst(); } - /** TD5 (`updateItem`'s trip-scoping guard) — `SELECT * FROM todo_items WHERE id = ? AND trip_id = ?`. */ - async findInTrip(id: number | string, trip_id: number | string): Promise { - return await this.kysely().selectFrom('todo_items').selectAll().where('id', '=', id as number).where('trip_id', '=', trip_id as number).executeTakeFirst(); + /** + * TD5 (`updateItem`'s trip-scoping guard) — `SELECT * FROM todo_items + * WHERE id = ? AND trip_id = ?`. `id: number` (Plan 4 Task 8b, U6 — the + * program's own gate-level id parsing carry: narrowed from `number | + * string` now that `TodoController.update`/`.remove` parse `:id` once via + * `toRowId` and thread the number down; `trip_id` stays `number | string`, + * a separate, still-accepted carry — see `todo.service.ts`'s own note). + */ + async findInTrip(id: number, trip_id: number | string): Promise { + return await this.kysely().selectFrom('todo_items').selectAll().where('id', '=', id).where('trip_id', '=', trip_id as number).executeTakeFirst(); } - /** TD8 (`deleteItem`'s trip-scoping guard) — `SELECT id FROM todo_items WHERE id = ? AND trip_id = ?`. */ - async existsInTrip(id: number | string, trip_id: number | string): Promise<{ id: number } | undefined> { - return await this.kysely().selectFrom('todo_items').select('id').where('id', '=', id as number).where('trip_id', '=', trip_id as number).executeTakeFirst(); + /** TD8 (`deleteItem`'s trip-scoping guard) — `SELECT id FROM todo_items WHERE id = ? AND trip_id = ?`. `id: number`, same Plan 4 Task 8b narrowing as {@link findInTrip}. */ + async existsInTrip(id: number, trip_id: number | string): Promise<{ id: number } | undefined> { + return await this.kysely().selectFrom('todo_items').select('id').where('id', '=', id).where('trip_id', '=', trip_id as number).executeTakeFirst(); } /** @@ -117,7 +124,7 @@ export class TodoItemsRepository extends TrekRepository { * exactly); every other column is a true presence sentinel bound off the * legacy `bodyKeys` array (`present = bodyKeys.includes('')`). */ - async update(id: number | string, write: { + async update(id: number, write: { name?: readonly [present: boolean, value: string]; checked?: readonly [present: boolean, value: number]; category?: readonly [present: boolean, value: string | null]; @@ -131,12 +138,12 @@ export class TodoItemsRepository extends TrekRepository { description: string | null; assigned_user_id: number | null; priority: number; }>(write); if (Object.keys(data).length === 0) return; - await this.kysely().updateTable('todo_items').set(data).where('id', '=', id as number).execute(); + await this.kysely().updateTable('todo_items').set(data).where('id', '=', id).execute(); } - /** TD9 (`deleteItem`) — `DELETE FROM todo_items WHERE id = ?` (no trip scoping in the statement itself — relies on the caller's prior {@link existsInTrip} gate, matching legacy). */ - async deleteById(id: number | string): Promise { - await this.kysely().deleteFrom('todo_items').where('id', '=', id as number).execute(); + /** TD9 (`deleteItem`) — `DELETE FROM todo_items WHERE id = ?` (no trip scoping in the statement itself — relies on the caller's prior {@link existsInTrip} gate, matching legacy). `id: number`, same Plan 4 Task 8b narrowing as {@link findInTrip}. */ + async deleteById(id: number): Promise { + await this.kysely().deleteFrom('todo_items').where('id', '=', id).execute(); } /** TD10 (`reorderItems`, looped) — `UPDATE todo_items SET sort_order = ? WHERE id = ? AND trip_id = ?`. */ diff --git a/server/src/nest/todo/todo.controller.ts b/server/src/nest/todo/todo.controller.ts index 50c0b6c06..eb410304d 100644 --- a/server/src/nest/todo/todo.controller.ts +++ b/server/src/nest/todo/todo.controller.ts @@ -16,6 +16,7 @@ import { TodoCreateItemDto, TodoUpdateItemDto, TodoReorderDto, TodoCategoryAssig import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; import { RequirePermission, TripAccessGuard } from '../permissions/trip-access.guard'; +import { toRowId } from '../common/row-id'; /** * /api/trips/:tripId/todo — trip-scoped task list. @@ -77,12 +78,20 @@ export class TodoController { @Body() body: TodoUpdateItemDto, @Headers('x-socket-id') socketId?: string, ) { + // Plan 4 Task 8b (U6) — :id is parsed ONCE here (toRowId, not Number(): + // rule 15's NaN-into-SQL trap), and the parsed number is what + // updateItem uses below, instead of the raw string reaching the + // repository and relying on SQLite's affinity CAST to match it. + const itemId = toRowId(id); + if (itemId === null) { + throw new HttpException({ error: 'Item not found' }, 404); + } const { name, checked, category, due_date, description, assigned_user_id, priority } = body; // bodyKeys carries which keys the request actually provided (the null-clear // protocol); the parsed body only ever holds known schema keys. const updated = await this.todo.updateItem( tripId, - id, + itemId, // checked arrives as boolean or legacy 0/1 — normalize to the 0/1 the SQL binds. { name, checked: checked === undefined ? undefined : checked ? 1 : 0, category, due_date, description, assigned_user_id, priority }, Object.keys(body), @@ -102,10 +111,15 @@ export class TodoController { @Param('id') id: string, @Headers('x-socket-id') socketId?: string, ) { - if (!(await this.todo.deleteItem(tripId, id))) { + // Plan 4 Task 8b (U6) — same single gate-level parse as update above. + const itemId = toRowId(id); + if (itemId === null) { throw new HttpException({ error: 'Item not found' }, 404); } - this.todo.broadcast(tripId, 'todo:deleted', { itemId: Number(id) }, socketId); + if (!(await this.todo.deleteItem(tripId, itemId))) { + throw new HttpException({ error: 'Item not found' }, 404); + } + this.todo.broadcast(tripId, 'todo:deleted', { itemId }, socketId); return { success: true }; } diff --git a/server/src/nest/todo/todo.rpc.ts b/server/src/nest/todo/todo.rpc.ts index 9b2fc9b94..996977224 100644 --- a/server/src/nest/todo/todo.rpc.ts +++ b/server/src/nest/todo/todo.rpc.ts @@ -54,7 +54,7 @@ export class TodoRpc { const actor = this.guards.requireActor(ctx, 'todo'); await this.guards.requireTripEdit(tripId, actor, TODO_EDIT_ACTION); const input = asPayload(params.input); - const updated = await this.todos.updateItem(String(tripId), String(todoId), input as never, Object.keys(input)); + const updated = await this.todos.updateItem(String(tripId), todoId, input as never, Object.keys(input)); if (!updated) throw new ForbiddenResource(`no todo ${todoId} on trip ${tripId}`); this.realtime.broadcast(tripId, 'todo:updated', { item: updated }, undefined); return updated; @@ -66,7 +66,7 @@ export class TodoRpc { const todoId = num(params.todoId, 'todoId'); const actor = this.guards.requireActor(ctx, 'todo'); await this.guards.requireTripEdit(tripId, actor, TODO_EDIT_ACTION); - if (!(await this.todos.deleteItem(String(tripId), String(todoId)))) { + if (!(await this.todos.deleteItem(String(tripId), todoId))) { throw new ForbiddenResource(`no todo ${todoId} on trip ${tripId}`); } this.realtime.broadcast(tripId, 'todo:deleted', { itemId: todoId }, undefined); diff --git a/server/src/nest/todo/todo.service.ts b/server/src/nest/todo/todo.service.ts index 0efa166fc..527e29f42 100644 --- a/server/src/nest/todo/todo.service.ts +++ b/server/src/nest/todo/todo.service.ts @@ -76,9 +76,22 @@ export class TodoService { return this.todoItemsRepo.findById(id); } + /** + * `id: number` (Plan 4 Task 8b, U6 — program rule 21's gate-level id + * parsing carry: `TodoController.update` now parses `:id` once via + * `toRowId` and threads the number here, instead of the raw route string + * reaching `TodoItemsRepository.findInTrip`/`.update` and relying on + * SQLite's column-affinity CAST to match it; `todos.rpc.ts`'s `num()` and + * `todo.mcp.ts`'s Zod-typed `itemId` were already real numbers). `tripId` + * stays `string | number` — a separate, still-accepted carry (the trip id + * is gated for ACCESS by `TripAccessGuard`, not threaded as a parsed + * number to every downstream call — see `places.service.ts + * #verifyTripAccess`'s own docstring, Plan 4's 23c292f2f, for why that + * second parse is out of scope here too). + */ async updateItem( tripId: string | number, - id: string | number, + id: number, data: { name?: string; checked?: number; category?: string | null; due_date?: string | null; description?: string | null; assigned_user_id?: number | null; priority?: number | null }, bodyKeys: string[] ) { @@ -98,7 +111,8 @@ export class TodoService { return this.todoItemsRepo.findById(id); } - async deleteItem(tripId: string | number, id: string | number): Promise { + /** `id: number` — same Plan 4 Task 8b (U6) narrowing as {@link updateItem}. */ + async deleteItem(tripId: string | number, id: number): Promise { const item = await this.todoItemsRepo.existsInTrip(id, tripId); if (!item) return false; diff --git a/server/tests/e2e/todo.e2e.test.ts b/server/tests/e2e/todo.e2e.test.ts index 9efa89d4b..6755fffee 100644 --- a/server/tests/e2e/todo.e2e.test.ts +++ b/server/tests/e2e/todo.e2e.test.ts @@ -195,6 +195,23 @@ describe('To-do e2e (real auth guard + real SQL over temp SQLite)', () => { expect(res.body).toEqual({ error: 'Item not found' }); }); + // Plan 4 Task 8b (U6) — :id is now parsed ONCE at the controller gate + // (toRowId), so a non-numeric id 404s cleanly through that guard instead + // of falling through to the repository and depending on SQLite's + // column-affinity CAST to simply not match (the legacy outcome was also a + // 404, same status — this pins the gate itself, not just the status). + it('404 (not 500) on update with a non-numeric :id', async () => { + const res = await request(server).put(`/api/trips/${tripId}/todo/abc`).set('Cookie', sessionCookie(1)).send({ name: 'X' }); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Item not found' }); + }); + + it('404 (not 500) on delete with a non-numeric :id', async () => { + const res = await request(server).delete(`/api/trips/${tripId}/todo/abc`).set('Cookie', sessionCookie(1)); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Item not found' }); + }); + it('200 on reorder, persisting the new sort_order', async () => { const a = insertItem(tripId, 'A', { sort_order: 0 }); const b = insertItem(tripId, 'B', { sort_order: 1 });