docs/reviews/internal/ holds internal working notes we keep out of the
published repo. Commit the ignore rule so the folder is excluded for everyone,
not just on the local working tree.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Diagnosis (reproduced by looping cold-server runs): the recurring
"cold start" failures in the auth/follow specs were a hydration race,
not a Vite dep-reload (the server.warmup config already handles that —
no reload events in the logs). FollowButton is an onClick button, and
Playwright considers it actionable (visible + enabled) before React
attaches the handler on a freshly-navigated page. A click in that
window is dropped (or triggers a native form submit), so the expected
state change never happens — e.g. notifications.test.ts:59 clicked
"Request to follow" with no hydration wait and the button never
flipped to "Requested".
The existing waitForHydration guard was applied per-interaction and so
was easy to forget (test 1 had it, test 3 didn't). Fix it at the
navigation instead:
- add gotoHydrated(page, url) = goto + waitForHydration, documented as
the default for "navigate then interact with a React control"
- use it before every FollowButton interaction in notifications +
social
- consolidate the setProfileVisibility helper (copy-pasted in three
specs, two missing the hydration wait) into e2e/helpers/profile.ts,
built on gotoHydrated
Validation against a fresh Postgres, cold servers: notifications 0/10
failures (was the proven flaker); notifications+social+explore 0/5
(55 tests); planner unaffected (25/25).
Note: two other "cold-start"-attributed flakes are NOT this race and
are out of scope here — planner map-load latency (mitigated by warmup
+ CI retries) and the /explore directory assertion (a local artifact
of the scratch DB accumulating >20 public users; CI's fresh DB stays
under the page size, so it doesn't bite there).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both were sitting untracked in the repo root. They're the source for
the "candidate N from the 2026-06-10 review" references in this
batch of follow-up PRs, so keeping them in-repo makes those references
resolvable. Self-contained HTML; a short README indexes them.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
settings.test.ts, explore.test.ts, and social.test.ts weren't matched
by any Playwright project, so they had silently never run — which is
how they rotted. Register them (one project each) and repair the
selectors against the current UI:
- settings: the page was split into sibling sections
(/settings/{profile,security,account}); the spec assumed one page.
Navigate to the right sub-page per test, use the stable section-nav
links + #id input locators (the Vite dev server transiently
double-renders the profile form during hydration, breaking
getByLabel), and wait for hydration before interacting with
fetcher-backed forms and the avatar dropdown.
- explore: setProfileVisibility now targets /settings/profile and
waits for hydration so the visibility save uses the fetcher.
- social: already green once registered.
Also fixes an app bug the specs surfaced: deleting a passkey redirected
to /settings#security, a stale anchor that now resolves to
/settings/profile — so you'd land on the wrong section. It now
redirects to /settings/security.
Verified locally against Postgres/BRouter: green under CI-style
retries (the residual registration flake is the same cold-start class
the rest of the suite has, which is why CI runs retries=2).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The flagship Caddy ran its admin API on 0.0.0.0:2019, reachable by
every container on the Docker network. The admin API can rewrite
routes and proxy targets, so a journal/planner RCE could repoint
traffic with no further auth. (The brouter-host Caddyfile already
binds admin to localhost; the flagship didn't.)
The admin endpoint also served Prometheus metrics (global `metrics`
option → admin endpoint), and Prometheus scrapes caddy:2019
cross-container — so admin couldn't just move to loopback without
breaking metrics. Split them:
- admin localhost:2019 (loopback only). All reloads are in-container
(`docker compose exec caddy caddy reload`) so they use this endpoint
unaffected.
- a dedicated `:2020` server exposes the read-only metrics handler;
prometheus.yml now scrapes caddy:2020.
Validated with `caddy validate` + `caddy adapt` against caddy:2:
admin.listen=localhost:2019, srv on :2020 carries the metrics handler,
the :443 site is unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three Low/Info hardening items from the 2026-06-10 security review.
OAuth/credential log redaction:
- oauth-flow.server.ts logged the raw exception on code-exchange
failure; a provider error can embed the auth code / token response,
which would land in logs + Sentry. Log only e.message now.
- manager.markNeedsRelink bounded the provider-supplied reason string
to 200 chars before logging.
Magic-link account enumeration:
- createMagicToken now returns null (instead of throwing "No account
found for this email") when no account matches; the login route
always responds { step: "magic-link-sent" }, minting a token and
sending mail only for a real account. The public login form can no
longer be used to probe which emails are registered. Registration's
email/username "already in use/taken" messages are intentionally
unchanged — standard signup UX, and the passkey ceremony can't be
made to fake-succeed.
Federation remote-document size cap:
- assertRemoteDocSize rejects an actor/outbox document over 4 MB once
serialized, applied in the ingest fetchJson seam. Fedify owns the
transfer (with its own SSRF + redirect limits) and our poll uses the
authenticated loader for secure-mode instances, so this is a
downstream guard on iteration/persistence, complementing the existing
per-poll item cap.
Tests: assertRemoteDocSize unit cases; a gated (FEDERATION_INTEGRATION=1)
integration test asserting an unsigned POST to /users/:u/inbox is
rejected with 401 — a regression guard for Fedify's signature
verification.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
POST /api/v1/uploads minted an upload key from caller input with no
ownership check, no content-type allowlist, and the raw filename
interpolated into the S3 key.
- Ownership: a caller may only mint upload URLs for a route/activity
they own, enforced via the branded loadOwnedRoute/loadOwnedActivity
(404 on miss, no existence leak). Closes writing into another
user's resource key-space.
- Content type: the request schema now constrains contentType to an
image/gpx allowlist, so active content (HTML/SVG/JS) that would
execute if served inline is rejected at the request boundary.
- Filename: sanitizeUploadFilename() reduces the client filename to a
safe basename (charset-restricted, no path components, no leading
dots, length-bounded) before it becomes part of the key.
Tests: schema allow/deny + filename sanitization in @trails-cool/api;
handler tests covering owned-success, not-owner 404, disallowed
content-type, and the route-vs-activity ownership branch.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The session callbackUrl becomes a server-side fetch target in
api.save-to-journal (POSTed with the callback bearer token, and the
journal's response is reflected to the caller). The /new query-param
loader already validated it, but the programmatic POST /api/sessions
entry point — anonymous, since the Planner is stateless — stored it
unvalidated. An attacker could make the Planner backend POST to
arbitrary hosts, including 169.254.169.254 and other internal targets.
- validateFetchUrl now blocks private / loopback / link-local /
CGNAT / cloud-metadata hosts (IPv4, IPv6, IPv4-mapped) when an
explicit allowlist isn't set. Gated on NODE_ENV=production && !E2E
(the requireSecret idiom) so the dev/e2e journal-on-localhost save
flow is unaffected. An explicit PLANNER_CALLBACK_ALLOWED_HOSTS still
takes precedence and remains the full-closure control (it also stops
DNS-name-to-private rebinding, which literal blocking does not).
- POST /api/sessions now validates callbackUrl exactly as /new does.
- api.save-to-journal re-validates immediately before the fetch
(defense in depth: covers sessions persisted before this change and
narrows the create→save rebinding window).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both failed the deletion test in the telling direction:
- @trails-cool/ui: Button/Input/Card had zero consumers — both apps
roll their own elements inline. The only live part was a 6-line
styles.css (the Tailwind entry + one keyframe), which now lives in
each app as app/styles.css.
- @trails-cool/map: MapView and RouteLayer had zero consumers; the
package was otherwise a re-export of two map-core constants, and the
two import sites now use @trails-cool/map-core directly. The
"map components go in @trails-cool/map" convention had drifted long
ago — the real map components live in apps/planner/app/components.
CLAUDE.md's repository structure and conventions updated to match
reality (including pointing shared-type guidance at the post-#515
sources: db row types, api contracts, Waypoint in types). Dockerfiles
no longer COPY the deleted package manifests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The PKCE verifier cookie, state encoding, redirect-URI construction,
and code exchange were coordinated across three route handlers that
each knew part of the protocol. Two latent bugs lived in the gaps: a
push-initiated re-authorization skipped PKCE entirely (harmless today
only because the sole pusher, Wahoo, is not a PKCE provider), and the
push-resume callback path never cleared the spent verifier cookie.
oauth-flow.server.ts now owns the lifecycle: initiateOAuthFlow builds
the provider redirect (state + verifier cookie, on every entry path),
and completeOAuthFlow consumes the callback — state decode, verifier
recovery, code exchange, connection linking — returning a
discriminated result the route maps to redirects. The three routes are
thin adapters; the next OAuth provider reuses the flow, not the
pattern.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Route and Activity existed three times: hand-written interfaces in
packages/types, Zod contracts in packages/api, and Drizzle columns in
packages/db — each with different fields and nullability. The
hand-written ones had drifted so far they had zero importers; the Zod
contracts were advisory because v1 handlers hand-rolled Response.json
shapes nothing validated.
- packages/types keeps only what both apps actually share (Waypoint,
WaypointPoiTags) and documents where row types and wire contracts
live; the dead Route/RouteMetadata/RouteVersion/Activity interfaces
are gone
- packages/db exports canonical inferred row types (RouteRow,
ActivityRow, RouteVersionRow, UserRow)
- packages/api contracts are reconciled with the real wire format
(RouteVersionSchema gains the id and createdBy fields the endpoint
has always returned) and gain Create*ResponseSchemas
- apiJson(schema, payload) in api-guard parses every v1 response
through its contract: drift is now a thrown ZodError in tests/CI,
unknown keys are stripped, and payloads are compile-checked as
z.input of the schema
Enforcement immediately caught two real drifts: nullable DB
descriptions could ship null where the contract promises string (now
coalesced at the boundary), and GET /api/v1/activities/:id was missing
the routeName and photos fields its contract declares.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The virtual-authenticator setup and registerUser were copy-pasted into
six spec files and had drifted: the auth spec's copy lost the final
URL assertion, and the settings spec's variant skipped hydration and
the Terms checkbox entirely. Seed-route boilerplate and hardcoded
localhost URLs were repeated across three more files.
- helpers/auth.ts: setup/removeVirtualAuthenticator,
submitRegistration (no outcome assertion, for expected-failure
attempts), registerUser (asserts the signed-in redirect),
registerFreshUser, logout
- helpers/journal.ts: JOURNAL/PLANNER base URLs, seedRoute,
routeHasGeom, seedKomootConnection
- nine spec files now import the helpers instead of re-deriving them
Found while consolidating: settings.test.ts, explore.test.ts, and
social.test.ts are not matched by any playwright project and have
never run — which is how the settings spec's broken register helper
survived. Registering them (and fixing whatever has rotted) is a
follow-up; the config now carries a warning so the trap is at least
documented.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
RNTL v14 makes render() async; awaiting it is a no-op on v13, so this
works on both and unblocks the dependabot v14 bump (#508).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
createRoute, updateRoute, createActivity, createRouteFromActivity, and
the demo-bot each re-implemented the same choreography after
validateGpx: flatten tracks into [lon, lat] coords for writeGeom and
derive distance / elevation / dayBreaks / description / start time.
The stat derivation lived in a private computeRouteStats in
routes.server.ts that activities couldn't reach, so the two sides had
drifted (activities re-derived inline, with its own start-time logic).
processGpx() in gpx-save.server.ts now owns the whole step: parse +
validate (GpxValidationError as before), coords extraction, and stat
derivation, returning the parsed GpxData so nothing re-parses.
Callers keep their own precedence rules between derived and
caller-supplied stats — routes let explicit input win wholesale, the
activities importer prefers GPX distance unless it is zero. Extends
ADR-0006: the gpx-save module remains the only place that understands
GPX-to-database derivation.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Dependabot bumped react-native-reanimated (4.4.1), react-native-
safe-area-context (5.8.0), and react-native-worklets (0.9.1) past the
versions Expo SDK 56 pins, which broke the native iOS build: expo's
Swift macro API drifted between expo-modules-core patches and
expo-crypto stopped compiling (OptimizedFunction macro mismatch).
CI never caught it because nothing compiles native code; the first
EAS build since the SDK 56 upgrade (d627b3c1) failed with it.
Applied `npx expo install --fix` (expo ~56.0.9, reanimated 4.3.1,
safe-area-context ~5.7.0, worklets 0.8.3) + pnpm dedupe, and added
the SDK-pinned native packages to the dependabot ignore list so they
only move via SDK bumps.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
