diff --git a/docs/reviews/README.md b/docs/reviews/README.md new file mode 100644 index 0000000..088c7b9 --- /dev/null +++ b/docs/reviews/README.md @@ -0,0 +1,17 @@ +# Reviews + +Point-in-time review decks. Self-contained HTML — open directly in a +browser (arrow keys / click to navigate). Each is a snapshot of its +date; findings were triaged and tracked through PRs, so the deck reads +as the "why" behind a batch of follow-up work rather than a live +checklist. + +- **`architecture-review-2026-06-10.html`** — deepening opportunities + (shallow modules, implicit interfaces, duplicated choreography). The + 10 candidates were implemented across PRs that reference them as + "candidate N from the 2026-06-10 review". + +- **`security-review-2026-06-10.html`** — defensive review of the auth, + federation, secrets, and injection surfaces. Severity-calibrated + (several scanner false-positives were verified and dropped). All + actionable findings were fixed in follow-up PRs. diff --git a/docs/reviews/architecture-review-2026-06-10.html b/docs/reviews/architecture-review-2026-06-10.html new file mode 100644 index 0000000..e3d25b5 --- /dev/null +++ b/docs/reviews/architecture-review-2026-06-10.html @@ -0,0 +1,586 @@ + + + + + +trails.cool — Architecture Review (2026-06-10) + + + + +
+ + +
+
trails.cool · architecture review
+

Deepening opportunities

+

A broad repo scan for architectural friction — shallow modules, implicit interfaces, duplicated choreography — with sketches for turning them into deep modules.

+

2026-06-10 · four parallel codebase surveys (Journal, Planner, packages, cross-cutting) · key claims verified against source · checked against CONTEXT.md and ADR-0001…0006

+
+ + +
+
Method
+

How to read these slides

+
+
+

Vocabulary

+
    +
  • Module — anything with an interface and an implementation.
    • +
  • Deep module — a lot of behaviour behind a small interface.
    • +
  • Shallow module — interface nearly as complex as the implementation.
    • +
  • Seam — where an interface lives; behaviour can change without editing in place.
    • +
  • Locality — change, bugs, and knowledge concentrated in one place.
    • +
  • Leverage — what callers get from depth.
    • +
+
+
+

The deletion test

+

Imagine deleting the module. If complexity vanishes, it was a pass-through. If complexity reappears across N callers, it was earning its keep.

+

Already settled (not re-proposed)

+
    +
  • No unified SyncProvider (ADR-0002)
    • +
  • No AuthMethod polymorphism (ADR-0005)
    • +
  • gpx-save owns geometry atomically (ADR-0006)
    • +
+
+
+
+ + +
+
Overall map
+

Where the codebase is deep — and where it isn't

+
+
+

Deep, earning their keep

+
    +
  • Journal core seams — gpx-save, completeAuth, ConnectedServiceManager, capability seams (Importer / RoutePusher / WebhookReceiver).
    • +
  • Planner pure modules — BRouter client, route-merge, segment-cache, host election. Well tested.
    • +
  • packages/gpx & packages/db — load-bearing, most-imported, best-tested.
    • +
  • Federation — ~3,200 lines of deliberate Fedify isolation.
    • +
+
+
+

Shallow, causing friction

+
    +
  • Journal route-handler layer — 110 handlers, ~5 tests; scattered ownership checks, hand-rolled form parsing.
    • +
  • Planner Yjs document schema — the app's most important interface, enforced by convention only (~30 raw call sites).
    • +
  • Domain types — three drifting sources of truth (types / api / db).
    • +
  • Jobs, SSE, notifications — interfaces discovered by grep, not by type.
    • +
+
+
+
+ + +
+
Agenda
+

Ten candidates

+
+ + +
+
+ + +
+
candidate 1 / 10
+

Komoot bulk import smuggles credentials past the manager defect-adjacent

+
+
+

Files

+

apps/journal/app/routes/api.sync.komoot.import.ts:34 apps/journal/app/jobs/komoot-bulk-import.ts

+
+
+

Problem

+

Verified: the route enqueues creds: service.credentials — the raw JSONB — in the pg-boss payload. This bypasses withFreshCredentials, the ConnectedServiceManager's whole reason to exist:

+
    +
  • Credentials sit at rest in the jobs table.
    • +
  • No refresh if they go stale between enqueue and execution.
    • +
  • markNeedsRelink never fires on failure.
    • +
+

The Wahoo path goes through the manager correctly; Komoot is the one defector from the seam.

+
+
+
+

Solution

+

Jobs carry only the serviceId. The handler resolves fresh credentials through ConnectedServiceManager at execution time — identical to every other capability caller.

+
+
+

Benefits

+
    +
  • Locality: all credential-lifecycle bugs live in one module again.
    • +
  • No credentials-at-rest in the jobs table.
    • +
  • Handler becomes testable with a fake manager instead of fixture credential blobs.
    • +
+
+
+
+
+ + +
+
candidate 2 / 10
+

A typed job seam — payloads are unknown end-to-end high leverage

+
+
+

Files

+

packages/jobs/src/types.ts apps/journal/app/jobs/ (~14 handlers) · ~7 enqueue sites

+
+
+

Problem

+

Verified: JobDefinition.handler is (jobs: Job<unknown>[]) => Promise<unknown>.

+
    +
  • Every handler opens with item.data as SomePayload; every enqueue passes a bare string queue name + unchecked object.
    • +
  • A typo or payload-shape change is a runtime failure in a background worker.
    • +
  • unknown exists for a documented contravariance reason — but that's an implementation constraint leaking into the interface.
    • +
  • enqueueOptional swallows queue failures: "saved but fan-out never queued" looks like success.
    • +
+
+
+
+

Solution

+

A JobPayloads map type ('notifications-fanout': { activityId: string }, …) with typed enqueue<K> and typed handler registration. The contravariance cast happens once, inside the package — not fourteen times at the edges.

+
+
+

Benefits

+
    +
  • Leverage: enqueue call sites can't drift from handlers.
    • +
  • Locality: retry/backoff policy and queue-name validity in one registry.
    • +
  • Handlers become unit-testable with typed fixtures.
    • +
+
+
+
+
+ + +
+
candidate 3 / 10
+

Planner: the Yjs routeData schema is an implicit interface high leverage

+
+
+

Files

+

apps/planner/app/lib/use-yjs.ts use-routing.ts use-waypoint-manager.ts use-elevation-data.ts + components (SessionView, ProfileSelector, ElevationChart, …)

+
+
+

Problem

+

Verified: ~15 string-keyed entries (geojson, coordinates, segmentBoundaries, surfaces, highways, maxspeeds, …) read at ~30 raw call sites.

+
    +
  • No module owns the schema — consumers do routeData.get("surfaces") as string | undefined and re-parse JSON.
    • +
  • parseJsonArray copy-pasted in two hooks with different signatures; waypoint extraction written four times.
    • +
  • Adding one road attribute touches ~7 files (enrichment → storage → parsing → rendering).
    • +
+

The document schema is the Planner's most important interface — enforced by convention only.

+
+
+
+

Solution

+

A routeData schema module: typed read/write of the whole document state, JSON encoding internal — analogous to what waypoint-ymap.ts already does well for waypoints. Hooks and components consume a RouteDataState, never raw Y.Map keys.

+
+
+

Benefits

+
    +
  • Locality: schema changes become one-file edits.
    • +
  • Hooks testable against a plain object instead of a live Yjs doc.
    • +
  • Deletion test passes clearly: delete it and key-string knowledge reappears at 30 call sites.
    • +
+
+
+
+
+ + +
+
candidate 4 / 10
+

Planner GPX assembly duplicated between Save and Export small win

+
+
+

Files

+

apps/planner/app/components/SaveToJournalButton.tsx apps/planner/app/components/ExportButton.tsx

+
+
+

Problem

+
    +
  • Both buttons independently extract geojson / waypoints / no-go areas / notes from Yjs and build GPX.
    • +
  • ExportButton additionally handles multi-day splitting.
    • +
  • Same extraction, two implementations — a change to what gets persisted (e.g. waypoint notes in GPX extensions) needs two synchronized edits.
    • +
  • Currently zero tests on either path.
    • +
+
+
+
+

Solution

+

One buildGpxData(yjs) module — naturally layered on candidate 3 — consumed by both buttons and any future export path.

+
+
+

Benefits

+
    +
  • The Planner→Journal handoff payload and the file export can no longer diverge.
    • +
  • The assembly becomes a pure, testable function.
    • +
+
+
+
+
+ + +
+
candidate 5 / 10
+

Journal: createRoute / createActivity duplicate the GPX choreography high leverage

+
+
+

Files

+

apps/journal/app/lib/routes.server.ts (~24–78) apps/journal/app/lib/activities.server.ts (~68–122) apps/journal/app/lib/gpx-save.server.ts

+
+
+

Problem

+
    +
  • Both callers independently run validateGpx → extract distance/elevation stats → transaction → writeGeom, in near-identical ~45-line blocks.
    • +
  • Includes the "stats may be pre-supplied" handling the demo bot relies on — duplicated too.
    • +
  • The gpx-save module owns validation and geometry but not the stat extraction between them, so the choreography is copied at every save entry point. A third entity type would copy it again.
    • +
+
+
+
+

Solution

+

Deepen the gpx-save module to own the full validate-and-derive step: one function returning parsed GPX + stats (accepting precomputed stats), leaving callers with only entity-specific row shapes. Extends ADR-0006 rather than contradicting it.

+
Worth grilling: should basic validation move down into packages/gpx, which already owns parsing and stat computation?
+
+
+

Benefits

+
    +
  • New validation rules or stat definitions become one edit.
    • +
  • "Every saved GPX has consistent stats" gets a single testable home.
    • +
+
+
+
+
+ + +
+
candidate 6 / 10
+

Three drifting sources of truth for Route/Activity shapes high leverage

+
+
+

Files

+

packages/types/src/index.ts packages/api/src/routes.ts (Zod) packages/db/src/schema/journal.ts (Drizzle)

+
+
+

Problem

+
    +
  • Route exists three times: hand-written interface (types), Zod RouteSummary/RouteDetail (api), Drizzle columns (db) — each with different fields and nullability.
    • +
  • Route handlers hand-roll the mapping (e.g. api.v1.routes._index.ts constructs response JSON manually, not through the schema).
    • +
  • The api package's contracts are advisory: nothing fails if a handler's response drifts from RouteListResponse.
    • +
+
+
+
+

Solution

+

Pick one canonical source — likely derive TS types from the Drizzle schema, have the api Zod schemas reference them — and validate responses against the contract at the seam (typed response helpers, or validation in the v1 handlers).

+
+
+

Benefits

+
    +
  • Leverage for both apps and mobile: one definition of Route.
    • +
  • The 312 schema tests in packages/api start guarding real traffic instead of just the schemas themselves.
    • +
+
+
+
+
+ + +
+
candidate 7 / 10
+

Ownership checks scattered across handlers and lib modules testability

+
+
+

Files

+

routes.$id.edit.server.ts push-action.server.ts:54 activities.server.ts (mutators that assume the caller checked) · others

+
+
+

Problem

+
    +
  • route.ownerId !== user.id is re-checked in some loaders, re-checked in some lib functions, absent in others where the precondition is implicit.
    • +
  • Nothing in the types distinguishes "a Route" from "a Route this user may mutate" — every new call site re-derives, or forgets, the check.
    • +
  • Contrast: the Terms gate is properly centralized. Ownership deserves the same treatment.
    • +
+
+
+
+

Solution

+

A loadOwnedRoute(routeId, userId) (and activity twin) as the single enforcement point, returning a branded type that mutators require — making bypass a compile error rather than a code-review catch.

+
+
+

Benefits

+
    +
  • Authorization gets the locality that completeAuth gave session minting.
    • +
  • The 403 behavior becomes testable once, instead of per-handler.
    • +
+
+
+
+
+ + +
+
candidate 8 / 10
+

OAuth connect → callback → resume spread over three routes testability

+
+
+

Files

+

api.sync.connect.$provider.ts api.sync.callback.$provider.ts api.sync.push.$provider.$routeId.ts oauth-state.server.ts

+
+
+

Problem

+
    +
  • PKCE verifier cookies, state encoding, and push-resumption are coordinated across three route handlers, each knowing part of the protocol (which cookie, when it's set, what's in the state blob).
    • +
  • State encode/decode is extracted — but the lifecycle isn't.
    • +
  • Adding Garmin or Strava OAuth means re-learning the choreography from the existing routes. CONTEXT.md forecasts three more OAuth providers.
    • +
+
+
+
+

Solution

+

An OAuth flow module owning initiate (returns redirect with cookies set) and complete (consumes request, returns exchange result + decoded state). Routes shrink to thin adapters.

+
+
+

Benefits

+
    +
  • Each future provider reuses the flow, not the pattern.
    • +
  • The PKCE handshake becomes testable without a browser.
    • +
+
+
+
+
+ + +
+
candidate 9 / 10
+

Shim & dead-weight packages: map and ui cleanup

+
+
+

Files

+

packages/map (79 lines: re-exports + a 35-line MapView) packages/ui (162 lines)

+
+
+

Problem

+

Both fail the deletion test in the telling direction — deleting them moves almost nothing:

+
    +
  • map adds an import-ambiguity tax: apps import from map and map-core inconsistently (~21 vs ~19 sites).
    • +
  • Verified: @trails-cool/ui is imported only by the two apps' root.tsx — for the stylesheet. Button/Input/Card have zero consumers; both apps roll their own buttons inline.
    • +
+
+
+
+

Solution

+
    +
  • Fold MapView into a clearly-React entry of map-core, or make map the only import surface for apps.
    • +
  • Either delete ui or commit to it as a real design system — the half-state is the worst option.
    • +
+
+
+

Benefits

+
    +
  • Less code falsely suggesting reuse.
    • +
  • One obvious import path for map code.
    • +
+
+
+
+
+ + +
+
candidate 10 / 10
+

E2E suite: 23 spec files re-derive their own setup testability

+
+
+

Files

+

e2e/ — seed-route boilerplate in 5+ files · auth helpers defined inside auth.test.ts · http://localhost:3000/3001 hardcoded in 10+ files

+
+
+

Problem

+
    +
  • The helpers module is nearly empty, so each spec carries its own fixtures.
    • +
  • Virtual-authenticator and register/login utilities live in a test file other specs can't cleanly import.
    • +
  • The stated e2e strategy (shared setup helpers, BRouter mocked by default) is documented — but not embodied in a module.
    • +
+
+
+
+

Solution

+

A real e2e/lib/ fixtures module: seedRoute(), registerAndLogin(), base-URL constants, authenticator setup — imported by every spec.

+
+
+

Benefits

+
    +
  • New specs start at the interesting assertion, not at boilerplate.
    • +
  • URL/port changes become one edit.
    • +
+
+
+
+
+ + +
+
Scope discipline
+

Noted but deliberately dropped

+ +
+ + +
+
Summary
+

Where to start

+ + + + + + +
#CandidateWhy it's first-tier
1Komoot credentials bypassClosest to a real defect — credentials at rest, no refresh, seam violated
3Planner routeData schema moduleHighest-leverage refactor — 30 call sites, ~7-file blast radius per attribute
2Typed job seamBest testability-per-effort — one registry, fourteen handlers gain types
7Branded ownership loadingBest safety-per-effort — bypass becomes a compile error
+

Then: 5 & 6 (one source of truth for GPX stats and domain types), 8 (before the next OAuth provider lands), 4 / 9 / 10 as opportunistic cleanups.

+
+ +
+ trails.cool architecture review · 2026-06-10 + + ←→ or click · Home/End +
+ + + + diff --git a/docs/reviews/security-review-2026-06-10.html b/docs/reviews/security-review-2026-06-10.html new file mode 100644 index 0000000..aea9c39 --- /dev/null +++ b/docs/reviews/security-review-2026-06-10.html @@ -0,0 +1,455 @@ + + + + + +trails.cool — Security Review (2026-06-10) + + + + +
+ + +
+
trails.cool · security review
+

Security review

+

Defensive review of the auth, federation, secret-management, and injection surfaces across both apps and the infrastructure — findings verified against source and severity-calibrated.

+

2026-06-10 · four parallel review sweeps (auth/session/JWT · federation/SSRF · secrets/crypto · injection/uploads/infra) · every load-bearing claim re-checked against the code before inclusion

+
+ + +
+
Method & calibration
+

How to read this — and what I threw out

+

The automated sweeps over-flagged. I verified each high-severity claim against the source and downgraded or dropped the ones that didn't hold up. That recalibration is itself a finding: don't action the raw scanner output.

+
+
+

Dropped / downgraded after verification

+
    +
  • "Critical: inbox signatures not verified" → Fedify verifies HTTP Signatures on inbox listeners by default. Real item is only a missing regression test. downgraded → Info
    • +
  • "JWT alg-confusion / none" → jose with a symmetric key already rejects alg:none and asymmetric algs. Explicit allow-list is hardening only. → Info
    • +
  • "Magic-link 6-digit brute-force" → bounded by a per-IP catch-all (30/min) and per-email verify cap (10/15min) against a 15-min-rotating code. → Info
    • +
  • ".env secrets committed" → .env is gitignored and never appears in history; values are dev creds. → Info
    • +
+
+
+

What held up

+
    +
  • One High: unauthenticated SSRF via the Planner session callback URL.
    • +
  • A handful of Medium/Low hardening gaps (upload validation, log redaction, Caddy admin bind, user enumeration).
    • +
  • A broad base of genuinely strong fundamentals (next-to-last slide).
    • +
+

Severity legend

+

High Medium Low Info

+
+
+
+ + +
+
Findings
+

What to fix, in order

+
+ + +
+
+ + +
+
finding 1 / 8
+

SSRF — unauthenticated Planner session callback URL High

+
+
+

Files

+

apps/planner/app/routes/api.sessions.ts · apps/planner/app/routes/api.save-to-journal.ts · helper apps/planner/app/lib/url-validation.server.ts (exists, unused here)

+
+
+

Problem

+

Verified: POST /api/sessions is anonymous (the Planner is stateless) and stores the caller-supplied callbackUrl with no validation. On save, the Planner server fetches it:

+ 
resp = await fetchWithTimeout(session.callbackUrl, {
+  method: "POST",
+  headers: { Authorization: `Bearer ${session.callbackToken}` },
+  ...
+

Anyone can make the Planner backend issue POSTs to arbitrary hosts — internal services, 169.254.169.254, localhost. A SAFE-scheme/host validator (validateFetchUrl) already exists in the repo but is not applied on this path.

+
+
+

Fix

+

Validate callbackUrl at session-create against the existing allowlist helper, rejecting non-HTTP(S) schemes and private/loopback/link-local resolutions:

+ 
const v = validateFetchUrl(callbackUrl, {
+  allowedHosts: getCallbackAllowedHosts(),
+});
+if (!v.ok) return data(
+  { error: "Invalid callback URL" },
+  { status: 400 },
+);
+

Re-validate at fetch time too (defends against DNS rebinding between create and save).

+
+
+
+ + +
+
finding 2 / 8
+

Upload validation — no content-type allowlist, raw filename in key Medium

+
+
+

Files

+

apps/journal/app/routes/api.v1.uploads.ts

+
+
+

Problem

+

Verified: the presigned-upload endpoint builds the S3 key from the raw client filename and accepts any content-type:

+ 
const key =
+  `${resourceType}/${resourceId}/${randomUUID()}-${filename}`;
+
    +
  • No content-type allowlist — a user can store HTML/SVG under an image resource; stored XSS if those bytes are ever served inline rather than as attachments.
    • +
  • Raw filename in the key lets a caller shape arbitrary key prefixes (the UUID prevents collision/overwrite, and S3 keys aren't a filesystem, so this is shaping not traversal).
    • +
  • Confirm resourceId ownership is checked so a user can't mint upload URLs under another user's resource path.
    • +
+
+
+

Fix

+
    +
  • Allowlist contentType (e.g. image/jpeg, image/png, application/gpx+xml) and bind it into the presign so the upload can't differ.
    • +
  • Sanitize filename to [A-Za-z0-9._-] or drop it from the key entirely (the UUID is enough).
    • +
  • Serve user content with Content-Disposition: attachment / from a separate origin, and verify resourceId belongs to the caller.
    • +
+
+
+
+ + +
+
finding 3 / 8
+

OAuth callback logs the raw exception Low

+
+
+

Files

+

apps/journal/app/lib/connected-services/oauth-flow.server.ts (~line 113) · pattern also in manager.ts (markNeedsRelink reason)

+
+
+

Problem

+

Verified: the code-exchange failure path logs the whole error object:

+ 
} catch (e) {
+  console.error(
+    `OAuth callback failed for ${manifest.id}:`, e);
+

If a provider's error response embeds the authorization code, a token, or other sensitive context in the thrown error, it lands in server logs and Sentry. The blast radius is small (only the failure branch, and the affected user is the token's owner), hence Low — but credentials in logs are worth closing.

+
+
+

Fix

+ 
console.error(
+  `OAuth callback failed for ${manifest.id}:`,
+  e instanceof Error ? e.message : String(e),
+);
+

Log a redacted shape (name + truncated message), never the raw object. Apply the same to markNeedsRelink's provider-supplied reason.

+
+
+
+ + +
+
finding 4 / 8
+

Caddy admin API bound to 0.0.0.0:2019 Low

+
+
+

Files

+

infrastructure/Caddyfile (line 3)

+
+
+

Problem

+

Verified: admin 0.0.0.0:2019. The port isn't published to the host, so it's not internet-reachable — but it is reachable by every container on the Docker network. The Caddy admin API can rewrite routes and reverse-proxy targets, so an RCE in the journal or planner container becomes "redirect all traffic" with no extra auth.

+
+
+

Fix

+ 
admin localhost:2019
+

Bind admin to loopback inside the Caddy container (or disable it with admin off if no live-reload is needed). Turns a one-step lateral move into a non-path.

+
+
+
+ + +
+
finding 5 / 8
+

User enumeration via distinct auth error messages Low

+
+
+

Files

+

apps/journal/app/lib/auth.server.ts (register + magic-link create paths)

+
+
+

Problem

+

Verified: the API returns distinguishable messages — "Email already in use" vs "Username already taken", and magic-link create throws "No account found for this email". An attacker can probe which emails/usernames are registered.

+

Calibrated to Low: username availability is intentionally visible at registration anyway, and this is a privacy/info-leak issue, not an account-takeover one.

+
+
+

Fix

+
    +
  • Magic-link request: return the same "if an account exists, we've sent a link" response whether or not the email matches; only send mail when it does.
    • +
  • Registration: keep username-availability UX, but make the email-taken path return a generic message (or fold into the same response and notify the existing owner by email instead).
    • +
+
+
+
+ + +
+
finding 6 / 8
+

Federation — hardening & missing regression tests Low

+
+
+

Files

+

apps/journal/app/lib/federation*.server.ts · apps/journal/app/jobs/poll-remote-*.ts

+
+
+

What's actually fine

+

Verified: Fedify verifies HTTP Signatures on inbox listeners by default; allowPrivateAddress is gated behind an env flag used only in e2e; private profiles don't federate (checked in every dispatcher); inbox replay is guarded via the Postgres KV store. The "unsigned activities accepted" alarm does not hold.

+
+
+

Worth doing anyway

+
    +
  • Regression test asserting an unsigned / wrongly-signed POST to /users/:u/inbox is rejected — so a future config change can't silently disable verification.
    • +
  • Response-size cap on remote actor/outbox fetches (BRouter already caps at 10 MB; federation fetches don't) — prevents an OOM from a hostile instance.
    • +
  • Treat DNS-rebinding on remote dereferences as an upstream Fedify concern to track; add resolve-then-connect guarding if it surfaces.
    • +
  • Range-check remote-supplied activity stats before storing.
    • +
+
+
+
+ + +
+
finding 7 / 8
+

Rate limiting is in-process and trusts X-Forwarded-For Low

+
+
+

Files

+

apps/journal/app/lib/rate-limit.server.ts

+
+
+

Problem

+
    +
  • Verified: buckets live in a per-process Map. Correct and safe for the single-instance flagship; horizontal scaling silently weakens every limit (each instance has its own counters).
    • +
  • clientIp() trusts the first X-Forwarded-For hop. Safe behind Caddy today (the app ports aren't published); becomes spoofable the moment the container is exposed directly.
    • +
+

Both are documented assumptions in the code, not oversights — recorded here so the assumption stays visible when the topology changes.

+
+
+

Fix (when scaling)

+
    +
  • Move buckets to Postgres/Redis before running a second journal instance.
    • +
  • Pin the trusted-proxy hop count (or read a Caddy-set, non-spoofable header) rather than blindly taking XFF[0].
    • +
  • Keep the app containers unpublished — never expose them without Caddy in front.
    • +
+
+
+
+ + +
+
finding 8 / 8
+

Komoot credential storage — by-design, minor edges Info

+
+
+

Files

+

apps/journal/app/lib/crypto.server.ts · connected-services/providers/komoot/* · api.sync.komoot.connect.ts

+
+
+

What's fine

+

Verified: the Komoot web-login password is encrypted at rest with AES-256-GCM, random 12-byte IV per encryption, scrypt-derived key. It must be reversibly stored because re-login replays it — that's inherent to web-login providers (ADR-recorded), not a flaw. The cipher usage is correct.

+
+
+

Minor edges

+
    +
  • The account email is stored plaintext in the credentials JSONB next to the encrypted password — minor PII at rest; consider encrypting it too.
    • +
  • The decrypted password lives briefly as a JS string during basic-auth construction — only matters under memory-dump threat models; low priority.
    • +
  • Pin explicit scrypt cost params (N, r, p) so a future Node default change can't alter derivation.
    • +
  • Tighten the Credentials type off the Record<string, unknown> catch-all so a future kind can't accidentally store a secret in the clear.
    • +
+
+
+
+ + +
+
Calibration
+

Strong fundamentals (verified) Good

+
+
+

Injection & input

+
    +
  • All raw SQL is parameterized — including PostGIS: the GeoJSON is bound, table names go through sql.identifier().
    • +
  • GPX/XML parsing is XXE-safe (DOMParser / linkedom, no external-entity or DTD expansion).
    • +
  • safeReturnTo() blocks open redirects (local paths only, no //).
    • +
+

Secrets

+
    +
  • requireSecret() refuses to boot prod on a dev-fallback secret.
    • +
  • AES-256-GCM + random IV; SOPS/age for secrets at rest; .env gitignored and never in history.
    • +
  • Sentry: sendDefaultPii: false, no session replay.
    • +
+
+
+

Auth & authz

+
    +
  • Single-use JWT (consumed_jwt_jti, atomic ON CONFLICT) and single-use magic tokens (atomic UPDATE … RETURNING) — both race-proof.
    • +
  • Session cookies: HttpOnly, Secure in prod, SameSite=Lax, signed.
    • +
  • Branded ownership loader makes IDOR a compile error (from the prior refactor).
    • +
  • Terms gate enforced on both web and bearer-token API.
    • +
+

Boundary & infra

+
    +
  • E2E backdoor endpoints gated on E2E env, never set in prod compose.
    • +
  • Strong headers + CSP (HSTS, nosniff, frame-deny, no unsafe-eval); Garmin webhook host allowlist; app ports unpublished behind Caddy; deploy secrets SOPS-encrypted, not echoed.
    • +
+
+
+
+ + +
+
Summary
+

Priorities

+ + + + + + + + + + +
#FindingSevDo
1SSRF via Planner session callback URLHighApply validateFetchUrl at create + fetch
2Upload content-type / filename / ownershipMediumAllowlist type, sanitize key, check owner
3OAuth callback logs raw exceptionLowLog redacted message only
4Caddy admin on 0.0.0.0:2019LowBind to localhost / admin off
5User enumeration via auth errorsLowGeneric responses on the email paths
6Federation tests + fetch capsLowAdd signature-rejection test, size cap
7In-process rate limit / XFF trustLowShared store before scaling out
8Komoot credential edgesInfoEncrypt email, pin scrypt params
+

No Critical issues. One High, externally-reachable and unauthenticated — fix first. Everything else is hardening on an already-solid base. Four scanner "Critical/High/Medium" alarms were verified false and dropped; trust the code, not the raw sweep.

+
+ +
+ trails.cool security review · 2026-06-10 · defensive, authorized + + ←→ or click · Home/End +
+ + + +