Files
linkder/SECURITY-FINDINGS.md
T
serfaandClaude Opus 5 1808ad4cba Move the demo market to Mexico City, priced in US dollars
The showcase was a Barcelona market: Catalan names, +34 numbers, euro
rates and "Carrer Example 12" on every job. Presented to a Mexican
client, all of that reads as somebody else's product.

City comes from NEXT_PUBLIC_CITY_* as before, now Ciudad de México at
19.4326/-99.1332, with MAPBOX_COUNTRY=mx. The seed's fallbacks were
Barcelona literals, so an unset env quietly seeded a different city
than the app rendered — they now agree.

Two db tests pinned the Barcelona centre as a hardcoded constant, which
is why the deck returned zero cards on the first run here: every pro was
a continent outside the radius. They read the same env as the seed now,
so the trap cannot recur.

Money: formatCents defaults to USD/en-US, and the nine hardcoded euro
signs across the card, search rows, quote strip and forms are dollars.
The rate NUMBERS are unchanged and still read high for CDMX — that is a
pricing decision, not a currency one, and is left alone deliberately.

Seed people are Mexican, addressed on real Roma/Condesa streets rotated
by index rather than one placeholder repeated. Phones moved to +52 55,
which moves the demo login to +525500000000 / 000000.

Also in here, from the same session:
- Sending a job now confirms. The mutation always succeeded; the sheet
  just closed with no receipt, which from the customer's side is
  indistinguishable from a dead button. Dismissing that receipt resolves
  as 'sent', so the card does not return to the deck.
- Media moves to DigitalOcean Spaces, with the public origin derived
  from bucket and region instead of a second env var to keep in sync.
- Managed-Postgres TLS: DATABASE_CA_CERT takes a path or inline PEM.
- The client-facing project panel beside the running app.
- Two profiles removed and four renamed to match their photos.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-23 10:56:31 -04:00

4.8 KiB

Open security findings (M1 auth review)

An adversarial review of the M1 auth, session and authorization code produced 28 confirmed findings (~20 after deduplication). The ones fixed are listed first so nobody re-reports them; the rest are open and are the M1 exit criteria.

Fixed

# Issue Fix
1 pro.upsertProfile computed requiresReReview and never applied it — a verified plumber could silently become a verified electrician 40 km away Demotes a verified profile to pending in the same transaction, writes an audit row, and now counts a trade change as material (it did not before). Needed a new verified -> pending edge in VERIFICATION_GRAPH
2 phoneNumber() registers /phone-number/request-password-reset, /phone-number/reset-password and /sign-in/phone-number unconditionally — not gated on emailAndPassword.enabled: false. Together they mint a password credential with no SMS sent to the owner, then accept it forever with no OTP All three served as 404 via disabledPaths (checked in onRequest, before rate limiting)
3 bearer() accepted the raw plaintext sessions.token column as an Authorization: Bearer credential — one leaked DB row is a replayable login Plugin removed. The mobile client it was for does not exist yet
4 Phone numbers stored exactly as typed, so +34600111222 and 0034600111222 are two "unique" accounts — defeating the UNIQUE constraint, bans, and duplicate detection phoneNumberValidator pins E.164 on send-otp and sign-in; toE164/isE164 added to @linkdr/shared
5 NEXT_PUBLIC_APP_URL fell back to http://localhost:3000, which drops Secure and the __Secure- prefix from the production session cookie Throws at boot in production

Open — must close before M1 ships

Authorization / attribution

  • resolveSession discards session.impersonatedBy, so every admin action taken while impersonating is written to audit_log as the victim. The Session type has no field for it, so no procedure can tell. Admin impersonation is enabled (admin() plugin) and is currently invisible to the application. Fix: add impersonatedBy to Session, populate it in resolveSession, and record it on all three audit_log insert sites.
  • user.setRole is a check-then-write across two connections; a concurrent job.create permanently strands the job it was meant to protect.
  • deck.swipe checks job.status outside its transaction — a concurrent job.cancel loses the race and a pending request lands on a cancelled job.
  • deck.undo's "already reached the pro" guard is check-then-write: it deletes the tombstone for a request that really was sent, and reports success.

Storage / uploads

  • Government ID and insurance scans go to the public bucket; isPrivateKind is never called from production code.
  • Presigned PUT does not pin Content-Type, so a caller can store HTML under an image key in a public bucket.
  • pro.addMedia, pro.addCredential and job.create accept any URL the caller sends, with no binding to an object that caller actually uploaded — which bypasses the presign design entirely.

Abuse / spend

  • No per-number cap, destination allowlist, or global ceiling on /phone-number/send-otp. SMS spend is unmetered.
  • better-auth rate limiting is per-process in-memory and collapses to one global bucket behind a proxy chain — 10 req/min then locks out the whole platform. Needs the Redis-backed rateLimit.customStorage, not secondary-storage (which throws unless all five SecondaryStorage methods exist).

Lower severity

  • user.setEmail lets any authenticated user claim an arbitrary unverified address, locking the real owner out of Google sign-up and leaking which addresses are registered.
  • Open redirect: the next query param on the sign-in form goes straight to router.push.
  • audit_log.ip trusts the leftmost X-Forwarded-For entry verbatim — attacker-controlled.
  • findPossibleDuplicates has no call site; the guarantee it was written to provide never executes.
  • DeckClient renders a frozen initialCards prop and invalidates a query nothing subscribes to, capping the deck at 20 cards per page load.
  • A right swipe after a left swipe creates a live request while the stored swipe row still records a rejection.

Note on the OTP-in-plaintext decision

apps/web/src/lib/auth.ts argues the plaintext OTP is acceptable partly because "anyone who can read that table can already read session.token". With bearer() removed that argument is weaker than when it was written — a session token now requires an httpOnly browser cookie to use, an OTP does not. The decision still stands (300s window, 3 attempts, rate limited) but revisit it if OTP lifetime or attempt budget ever increases.