serfaandClaude Opus 5
1808ad4cba
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>
77 lines
4.8 KiB
Markdown
77 lines
4.8 KiB
Markdown
# 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.
|