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 () => {