From 23c292f2f22f79a7f15f35d31905f4506497a17c Mon Sep 17 00:00:00 2001
From: jubnl <jgunther021@gmail.com>
Date: Thu, 24 Sep 2026 06:26:45 +0200
Subject: [PATCH] =?UTF-8?q?fix(server):=20plan=204=20=E2=80=94=20guard-lev?=
 =?UTF-8?q?el=20single=20parse:=20verify=20+=20correct=20the=20overclaimin?=
 =?UTF-8?q?g=20verifyTripAccess=20docstring=20(L-2)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

TripAccessGuard/TripOwnerGuard already parse :tripId exactly once
(Number(), guarded by Number.isFinite) and hand the resolved TripAccess
row to the handler via request[TRIP_REQUEST_KEY], TripOwnerGuard reusing
TripAccessGuard's own lookup when both are on the same route rather than
re-querying — verified by reading both guards directly, no change needed
there.

PlacesService.verifyTripAccess's own docstring separately claimed its
returned `tid` gets "returned... for every downstream call to reuse
(never re-parsing)" (3c's Task 9 review, L-2, carried to Plan 4). Reading
every call site (not assumed) shows this is false: `.tid` has exactly one
reader (PlacesController.requireTrip itself), which discards it — none of
requireTrip's 13 callers read `.tid` off the trip it returns; each passes
the original raw tripId string on to the write/read method it calls next,
which toRowId-parses it again itself. That second parse cannot be
eliminated by this gate alone: the same service methods are called
directly by places.mcp.ts too, an entry point with no requireTrip gate at
all (its own numeric tripId is re-stringified, String(tripId), to call
them), so each method owns its own id validation regardless of entry
point. Threading `tid` all the way through would mean giving every one of
~15 methods a second, number-only call shape for the REST path alone to
use — a real refactor, out of this docstring fix's own scope. Corrected
the docstring to say so plainly instead of repeating the false claim.
---
 server/src/nest/places/places.service.ts | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts
index 33f481b64..3278d3b39 100644
--- a/server/src/nest/places/places.service.ts
+++ b/server/src/nest/places/places.service.ts
@@ -189,10 +189,26 @@ export class PlacesService {
    * writes behind this gate (`toRowId(tripId) ?? -1`) then refused, or sent
    * a bare `NaN` into `TripsRepository.findAccessible`'s raw bind and 500'd
    * (fixed program-wide at the platform too — see `NulSafeSqlitePlatform`).
-   * Parsing ONCE, here, and returning the parsed `tid` for every downstream
-   * call to reuse (never re-parsing) closes both: a non-canonical trip id
-   * now answers this gate's own 404 before any read or write runs, and every
-   * write behind it shares the exact id the gate authorised.
+   * Parsing here answers this gate's own 404 before any read or write runs.
+   *
+   * **The returned `tid` is NOT threaded to every downstream call, despite
+   * an earlier version of this docstring claiming it was** (Plan 4 Task 8a,
+   * L-2 — confirmed false by reading every call site, not assumed): `.tid`
+   * has exactly one reader today, `PlacesController.requireTrip` itself
+   * (`places.controller.ts:122`), which discards it — `requireTrip` returns
+   * the WHOLE `trip` object to its 13 callers, none of which read `.tid`
+   * off it; every one instead passes the ORIGINAL raw `tripId` string on to
+   * the write/read method it calls next (`create(tripId, …)`,
+   * `importGpx(tripId, …)`, …), which `toRowId`-parses it AGAIN itself.
+   * That second parse is not a bug this gate can close by itself: those
+   * same methods are also called directly by `places.mcp.ts` (an entry
+   * point with no `requireTrip` gate at all — its own `tripId: number`
+   * input is re-stringified, `String(tripId)`, to call them), so each
+   * method owns its own id validation regardless of which entry point
+   * reached it, and `tid` alone cannot remove that. Threading it all the
+   * way through would mean giving every one of those methods a second,
+   * number-only call shape for the REST path to use instead of its shared
+   * string-taking one — a real refactor, out of this gate's own scope.
    */
   async verifyTripAccess(tripId: string, userId: number): Promise<(TripAccess & { tid: number }) | undefined> {
     const tid = toRowId(tripId);