From 8f8e5af63239c63d79bd52a2b620c782fdf737f0 Mon Sep 17 00:00:00 2001 From: jubnl Date: Thu, 24 Sep 2026 06:37:40 +0200 Subject: [PATCH] =?UTF-8?q?refactor(server):=20plan=204=20=E2=80=94=20narr?= =?UTF-8?q?ow=20TripMembersRepository.remove/TripsRepository.setOwner=20to?= =?UTF-8?q?=20number?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both accepted trip_id: number | string per 3c's carry note (their "stays loose for other callers" reasoning), pending the guard-level single parse landing first. Re-checked every caller directly: setOwner's one production caller (trip-members.service.ts#transferOwnership) already passes trip.id, a real number resolved earlier in the same transaction; remove's callers are trips.rpc.ts (num(), already a validated number), trips.mcp.ts (Zod- typed tool input, already a number) and TripMembersController.removeMember (the one genuine raw-string caller, :id off the route with no class-level TripAccessGuard on this controller by design). TripMembersController.removeMember now parses :id once (toRowId, not Number() — rule 15's NaN-into-SQL trap) and threads the parsed number to both its own access check and removeMember, instead of passing the raw string through twice. TripMembersService.removeMember and both repository methods narrow to number accordingly; setOwner's WHERE also moves off its raw-bind escape hatch onto nativeUpdate now that trip_id is typed, and remove's trip_id half moves onto a typed .where({ trip: trip_id }) filter (user_id keeps its own raw bind — a different, still-live NaN-safety seam, unrelated to this narrowing). TRIPREPO-018 and TMEMREPO-023, which pinned the retired string-bind seam on setOwner/remove, no longer typecheck against the narrowed signatures and are removed with an explanatory comment in their place. --- .../db/repositories/TripMembers.repository.ts | 20 +++++++++------- .../src/db/repositories/Trips.repository.ts | 18 +++++++-------- .../trip-members/trip-members.controller.ts | 14 +++++++++-- .../nest/trip-members/trip-members.service.ts | 23 ++++++++++++------- .../TripMembers.repository.test.ts | 16 ++++++------- .../db/repositories/Trips.repository.test.ts | 13 +++++------ 6 files changed, 62 insertions(+), 42 deletions(-) diff --git a/server/src/db/repositories/TripMembers.repository.ts b/server/src/db/repositories/TripMembers.repository.ts index ffba34440..6e94289b1 100644 --- a/server/src/db/repositories/TripMembers.repository.ts +++ b/server/src/db/repositories/TripMembers.repository.ts @@ -231,12 +231,16 @@ export class TripMembersRepository extends TrekRepository { * `DELETE FROM trip_members WHERE trip_id = ? AND user_id = ?` * (`trip-members.service.ts:166`, TM8 `removeMember`; `:198`, TM14 * `transferOwnership`'s "no longer a plain member" step — - * security-sensitive). Raw-bind on BOTH `trip_id` and `user_id` — not just - * `trip_id` (D4's T5 escape hatch, the same `number | string` seam - * `rosterUserIds`/`listUserIdsByTrip` above preserve): `TripMembersController - * .removeMember` still does a bare `Number.parseInt(userId)` on the route's - * `:userId` before calling this, unvalidated (rule 15's exact trap — a - * non-numeric id parses to `NaN`). + * security-sensitive). `trip_id: number` (Plan 4 Task 8a, narrowed from + * `number | string` — its last raw-string caller, + * `TripMembersController.removeMember`, now parses its route param once + * itself before calling this), so `trip_id` is a typed `.where({ trip: + * trip_id })` filter below; `user_id` stays a raw `m.user_id = ?` + * placeholder — NOT the same seam, a different reason entirely: + * `TripMembersController.removeMember` still does a bare + * `Number.parseInt(userId)` on the route's `:userId`, unvalidated (rule + * 15's exact trap — a non-numeric id parses to `NaN`), and a typed filter + * cannot carry a `NaN` through this ORM layer intact (see below). * * **`Number.isFinite` guard, checked directly against a compiled boot, not * assumed:** a typed `.andWhere({ user: user_id })` filter renders `NaN` as @@ -257,11 +261,11 @@ export class TripMembersRepository extends TrekRepository { * that, byte for byte, since neither the typed filter nor the raw * placeholder route can pass `NaN` through the ORM layer intact. */ - async remove(trip_id: number | string, user_id: number): Promise { + async remove(trip_id: number, user_id: number): Promise { if (!Number.isFinite(user_id)) return; await this.qb('m') .delete() - .where('m.trip_id = ?', [trip_id]) + .where({ trip: trip_id }) .andWhere('m.user_id = ?', [user_id]) .execute('run'); } diff --git a/server/src/db/repositories/Trips.repository.ts b/server/src/db/repositories/Trips.repository.ts index 89357e9af..aec7ff459 100644 --- a/server/src/db/repositories/Trips.repository.ts +++ b/server/src/db/repositories/Trips.repository.ts @@ -309,16 +309,16 @@ export class TripsRepository extends TrekRepository { /** * `UPDATE trips SET user_id = ? WHERE id = ?` (`trip-members.service.ts:196`, * TM13) — security-sensitive: the ownership handover itself, the first of - * `transferOwnership`'s three transactional statements. `qb().update()` - * rather than `nativeUpdate`: `nativeUpdate`'s typed `FilterQuery` rejects - * a `string` against `id`'s branded `number` type the same way `find`'s - * does, and `trip_id` here is still the route's unconverted string. + * `transferOwnership`'s three transactional statements. `trip_id: number` + * (Plan 4 Task 8a, narrowed from `number | string`): its one production + * caller already passes `trip.id`, the real row `findIdTitleOwner` + * resolved earlier in the same transaction, never the route's raw string + * (see `transferOwnership`'s own comment on why). `nativeUpdate`, typed + * filter throughout — nothing here needs the raw-bind escape hatch any + * more. */ - async setOwner(trip_id: number | string, user_id: number): Promise { - await this.qb('t') - .update({ user: user_id }) - .where('t.id = ?', [trip_id]) - .execute('run'); + async setOwner(trip_id: number, user_id: number): Promise { + await this.nativeUpdate({ id: trip_id }, { user: user_id }); } /** diff --git a/server/src/nest/trip-members/trip-members.controller.ts b/server/src/nest/trip-members/trip-members.controller.ts index 281c1bb81..d99f9b157 100644 --- a/server/src/nest/trip-members/trip-members.controller.ts +++ b/server/src/nest/trip-members/trip-members.controller.ts @@ -21,6 +21,7 @@ import { RequireTripOwner, TripOwnerGuard } from '../permissions/trip-owner.guar import { getClientIp } from '../audit/client-ip'; import { AuditService } from '../audit/audit.service'; import { NotFoundError, ValidationError } from '../common/domain-errors'; +import { toRowId } from '../common/row-id'; import { TripAddMemberDto, TripTransferOwnershipDto, TripCreateGuestDto, TripRenameGuestDto } from '../trips/trips.dto'; /** @@ -80,7 +81,16 @@ export class TripMembersController { @Delete(':id/members/:userId') async removeMember(@CurrentUser() user: User, @Param('id') id: string, @Param('userId') userId: string) { - const access = await this.roster.canAccessTrip(id, user.id); + // Plan 4 Task 8a — parsed ONCE here (toRowId, not Number(): rule 15's + // NaN-into-SQL trap), and the parsed number is what both the access + // check and the removal itself use below — this route has no class-level + // TripAccessGuard (this controller's own docstring explains why), so it + // owns this parse itself rather than inheriting the guard's. + const tripId = toRowId(id); + if (tripId === null) { + throw new HttpException({ error: 'Trip not found' }, 404); + } + const access = await this.roster.canAccessTrip(tripId, user.id); if (!access) { throw new HttpException({ error: 'Trip not found' }, 404); } @@ -88,7 +98,7 @@ export class TripMembersController { if (targetId !== user.id && !(await this.roster.can('member_manage', user.role, access.user_id, user.id, access.user_id !== user.id))) { throw new HttpException({ error: 'No permission to remove members' }, 403); } - await this.roster.removeMember(id, targetId); + await this.roster.removeMember(tripId, targetId); return { success: true }; } diff --git a/server/src/nest/trip-members/trip-members.service.ts b/server/src/nest/trip-members/trip-members.service.ts index 32f7226d8..055d4597f 100644 --- a/server/src/nest/trip-members/trip-members.service.ts +++ b/server/src/nest/trip-members/trip-members.service.ts @@ -170,7 +170,14 @@ export class TripMembersService { }; } - async removeMember(tripId: string | number, targetUserId: number): Promise { + /** + * `tripId: number` (Plan 4 Task 8a — narrowed from `string | number`): + * every caller now passes an already-parsed id — `trips.rpc.ts`'s `num()`, + * `trips.mcp.ts`'s Zod-typed tool input, and `TripMembersController + * .removeMember`'s own `toRowId` parse (added alongside this narrowing, + * the route's one remaining raw-string caller). + */ + async removeMember(tripId: number, targetUserId: number): Promise { await this.tripMembersRepo.remove(tripId, targetUserId); } @@ -202,15 +209,15 @@ export class TripMembersService { // Task 9 fix wave (B-L4 / A-L1): all THREE writes below now take // `trip.id` — the real `number` `findIdTitleOwner` already resolved - // above, not the route's raw `tripId` string. The comment this replaces + // above, not the route's raw `tripId` string. The comment this replaced // claimed "`Number(tripId)` at the two writes below", but only // `addIgnoringConflict` (which requires a real `Primary`) ever - // did that; `setOwner`/`remove` kept binding the unconverted string — - // not a live divergence (both repository methods document and test - // that raw-bind seam for their OTHER callers, so their signatures stay - // `number | string`), but the letter of rule 21 says one id, parsed - // once, threaded everywhere — and `trip.id` is already sitting right - // here, already proven to be the real row. + // did that; `setOwner`/`remove` used to keep binding the unconverted + // string for their OTHER callers' sake. Plan 4 Task 8a narrowed both to + // `number` (their last raw-string caller, `TripMembersController + // .removeMember`, now parses once itself) — the letter of rule 21 says + // one id, parsed once, threaded everywhere, and `trip.id` is already + // sitting right here, already proven to be the real row. await this.uow.transactional(async () => { await this.tripsRepo.setOwner(trip.id, newOwnerId); // The new owner is no longer a plain member… diff --git a/server/tests/unit/db/repositories/TripMembers.repository.test.ts b/server/tests/unit/db/repositories/TripMembers.repository.test.ts index dc9ecb9b8..7e985e314 100644 --- a/server/tests/unit/db/repositories/TripMembers.repository.test.ts +++ b/server/tests/unit/db/repositories/TripMembers.repository.test.ts @@ -306,14 +306,14 @@ describe('TripMembersRepository.addIgnoringConflict / remove (TM8/TM14/TM15, sec await expect(tripMembers.remove(trip.id, 999999)).resolves.toBeUndefined(); }); - it('TMEMREPO-023: remove binds a string trip id raw, the same seam rosterUserIds/listUserIdsByTrip document', async () => { - const { user: owner } = createUser(testDb); - const { user: member } = createUser(testDb); - const trip = createTrip(testDb, owner.id); - addTripMember(testDb, trip.id, member.id); - await tripMembers.remove(String(trip.id), member.id); - expect(testDb.prepare('SELECT id FROM trip_members WHERE trip_id = ? AND user_id = ?').get(trip.id, member.id)).toBeUndefined(); - }); + // TMEMREPO-023 used to pin remove's `number | string` raw-bind seam on + // `trip_id` (`remove(String(trip.id), ...)`) — retired by Plan 4 Task 8a's + // narrowing to `trip_id: number` (its own docstring covers why: its last + // raw-string caller, TripMembersController.removeMember, now parses its + // route param once itself). `String(trip.id)` no longer typechecks + // against `remove`'s signature, so there is nothing left to pin here. + // `user_id` keeps its own raw bind (TMEMREPO-024 below), a different, + // still-live seam — see remove's own docstring. }); // Rule 15's exact trap, caught on a compiled boot (`DELETE /api/trips/:id/ diff --git a/server/tests/unit/db/repositories/Trips.repository.test.ts b/server/tests/unit/db/repositories/Trips.repository.test.ts index a419fd33f..f6eef575e 100644 --- a/server/tests/unit/db/repositories/Trips.repository.test.ts +++ b/server/tests/unit/db/repositories/Trips.repository.test.ts @@ -193,13 +193,12 @@ describe('TripsRepository — TripMembersService / TripReadModelService (Plan 3c expect((testDb.prepare('SELECT user_id FROM trips WHERE id = ?').get(trip.id) as { user_id: number }).user_id).toBe(newOwner.id); }); - it('TRIPREPO-018: setOwner binds a string trip id raw — the same seam its docstring documents', async () => { - const { user: owner } = createUser(testDb); - const { user: newOwner } = createUser(testDb); - const trip = createTrip(testDb, owner.id); - await trips.setOwner(String(trip.id), newOwner.id); - expect((testDb.prepare('SELECT user_id FROM trips WHERE id = ?').get(trip.id) as { user_id: number }).user_id).toBe(newOwner.id); - }); + // TRIPREPO-018 used to pin setOwner's `number | string` raw-bind seam + // (`setOwner(String(trip.id), ...)`) — retired by Plan 4 Task 8a's + // narrowing to `trip_id: number` (its own docstring covers why: its one + // production caller already passes a real `trip.id` number, never the + // route's raw string). `String(trip.id)` no longer typechecks against + // `setOwner`'s signature, so there is nothing left to pin here. describe('findRaw (TR-B, shared with TripsService.getRaw once Task 7 lands)', () => { it('TRIPREPO-019: every scalar column comes back, feed_token included (the JS strip is the caller\'s job)', async () => {