Skip to content

fix(contacts): stop edit route from reassigning a QSO's owner or injecting SQL - #251

Merged
patrickrb merged 1 commit into
mainfrom
optio/task-2f348dd7-6734-47d4-8934-19dcb62eb12d
Jul 24, 2026
Merged

fix(contacts): stop edit route from reassigning a QSO's owner or injecting SQL#251
patrickrb merged 1 commit into
mainfrom
optio/task-2f348dd7-6734-47d4-8934-19dcb62eb12d

Conversation

@patrickrb

Copy link
Copy Markdown
Owner

Problem

Contact.update (behind PUT /api/contacts/[id], the contact edit flow) built its UPDATE statement by interpolating every key of the input object directly into the SQL as a column name:

for (const [key, value] of Object.entries(contactData)) {
  if (value !== undefined && key !== 'id' && key !== 'created_at' && key !== 'updated_at') {
    fields.push(`${key} = $${paramCount}`);   // key is concatenated, not bound

The route (src/app/api/contacts/[id]/route.ts) verifies ownership of the existing row, then passes the raw JSON request body straight into Contact.update. Two problems fell out of that:

  1. Mass assignment / IDOR — only id/created_at/updated_at were excluded, not user_id. A caller could move one of their own QSOs into another operator's log with { "user_id": <someone-else> }.
  2. SQL injection — the object key is concatenated into the statement (never bound), so a crafted key such as "user_id = 1, notes" becomes live SQL instead of a harmless bad-column error.

As a bonus, the old code would also throw a column ... does not exist error if the contact object ever carried a joined/derived field (e.g. station_callsign), since any stray key was treated as a real column.

Solution

Route the update through a fixed column allowlist at the model layer — the single choke point both callers (PUT /api/contacts/[id] and POST /api/contacts/populate-location) share.

New server-imports-free module src/lib/contact-update.ts (same pattern as src/lib/contact-search.ts) exposes:

  • UPDATABLE_CONTACT_COLUMNS — every editable contacts column (derived from drizzle/schema.ts) except the identity/ownership/bookkeeping columns id, user_id, created_at, updated_at.
  • buildContactUpdate(contactData) — a pure function that emits col = $n assignments and their bound values only for allowlisted keys, dropping unknown/forbidden keys and undefined values while keeping placeholders in lockstep with the values array. null is kept, so the edit form can still clear a field.

Contact.update now calls buildContactUpdate instead of iterating over arbitrary keys.

Backwards compatibility: the edit dialog spreads ...formData (initialized from ...contact), so it was already re-sending user_id unchanged on every save — that key is now simply dropped, and every field the form legitimately edits (callsign, mode, band, station_id, notes, tx_pwr, grid_locator, QSL flags, …) is in the allowlist. Normal edits behave identically. populate-location only sends grid_locator/latitude/longitude, all allowlisted.

Testing

  • New unit spec tests/contact-update.spec.ts (8 cases, pure-function like the search-builder spec) covering: lockstep placeholders, undefined skipping, null retained, ownership/identity columns dropped, crafted-injection keys dropped, empty result when nothing updatable, interleaved-forbidden-key numbering, and the allowlist membership sanity check. 8 passed.
  • npm run typecheck — clean
  • npm run lint — clean
  • npm run build — succeeds

Future follow-up

  • The two PUT /api/stations routes take the raw body too; they set explicit columns rather than dynamic keys, so they aren't vulnerable, but a shared allowlist helper there would be tidy.
  • Consider having the edit form send only changed/editable fields rather than the whole row.

🤖 Generated with Claude Code

…g SQL

`Contact.update` built its UPDATE by interpolating *every* key of the
input object straight into the SQL as a column name (`SET ${key} = $n`),
and `PUT /api/contacts/[id]` passes the raw JSON request body in with no
filtering. Two problems followed:

- Mass assignment: a caller could move one of their own QSOs into another
  operator's log by sending `{ "user_id": <someone-else> }` — only
  id/created_at/updated_at were excluded, not user_id.
- SQL injection: the key is concatenated, never bound, so a crafted key
  (e.g. `"user_id = 1, notes"`) becomes live SQL rather than a bad-column
  error.

Fix: route the update through a fixed column allowlist. New
server-imports-free `@/lib/contact-update` exposes `UPDATABLE_CONTACT_COLUMNS`
(every editable `contacts` column except id/user_id/created_at/updated_at)
and a pure `buildContactUpdate()` that emits `col = $n` assignments only
for allowlisted keys, dropping unknown/forbidden keys and `undefined`
values while keeping placeholders in lockstep with their bound values.
Unit-tested directly like `@/lib/contact-search`. The edit form's
legitimate fields are all allowlisted, so normal edits are unchanged; it
was already re-sending user_id unchanged, which is now simply dropped.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented Jul 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nodelog Ready Ready Preview, Comment Jul 24, 2026 4:24pm

Request Review

@patrickrb
patrickrb merged commit 97c77e5 into main Jul 24, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant