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

src/lib/contact-update.ts

@@ -0,0 +1,67 @@
+// Allowlist + SET-clause builder for updating a contact (PUT /api/contacts/[id],
+// via Contact.update).
+//
+// Kept free of server-only imports (no `pg`, no db pool) so it can be unit
+// tested directly, like @/lib/contact-search. Contact.update feeds the result
+// straight into `query(sql, values)`.
+//
+// Why an allowlist: Contact.update used to interpolate *every* key of the input
+// object straight into the SQL as a column name (`SET ${key} = $n`), and the
+// PUT route hands it the raw JSON request body with no filtering. Two problems
+// followed from that:
+//   1. Mass assignment — a caller could reassign one of their QSOs to another
+//      operator by sending `{ "user_id": <someone-else> }` (only id/created_at/
+//      updated_at were excluded), silently moving the contact out of their log.
+//   2. 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.
+// Restricting writes to a fixed set of real, user-editable columns closes both:
+// unknown/forbidden keys are dropped, and every emitted column name comes from
+// this constant — never from caller input.
+
+// Every editable column on the `contacts` table (see drizzle/schema.ts) EXCEPT
+// the identity/ownership/bookkeeping ones — `id`, `user_id`, `created_at`,
+// `updated_at` — which must never be reassigned through an edit.
+export const UPDATABLE_CONTACT_COLUMNS = new Set<string>([
+  'station_id', 'callsign', 'name', 'frequency', 'mode', 'band', 'datetime',
+  'rst_sent', 'rst_received', 'qth', 'grid_locator', 'latitude', 'longitude',
+  'country', 'dxcc', 'cont', 'cqz', 'ituz', 'state', 'cnty',
+  'qsl_rcvd', 'qsl_sent', 'qsl_via', 'eqsl_qsl_rcvd', 'eqsl_qsl_sent',
+  'lotw_qsl_rcvd', 'lotw_qsl_sent', 'qrz_qsl_sent', 'qrz_qsl_rcvd',
+  'qrz_qsl_sent_date', 'qrz_qsl_rcvd_date', 'qso_date_off', 'time_off',
+  'operator', 'distance', 'tx_pwr', 'notes', 'qsl_lotw', 'qsl_lotw_date',
+  'lotw_match_status', 'prop_mode', 'sat_name', 'band_rx', 'freq_rx', 'iota',
+  'lotw_qslrdate',
+]);
+
+export interface ContactUpdateAssignments {
+  /** `col = $n` fragments, in insertion order. */
+  fields: string[];
+  /** Bound values, index N-1 corresponding to placeholder `$N`. */
+  values: unknown[];
+}
+
+/**
+ * Build the `col = $n, …` assignments and their bound values for a contact
+ * update from a partial contact object. Only keys in
+ * UPDATABLE_CONTACT_COLUMNS are emitted; unknown keys (including id/user_id/
+ * created_at/updated_at and any crafted injection key) and `undefined` values
+ * are skipped. Placeholders start at $1 and count only the fields actually
+ * included, so they can never drift out of step with `values`. A `null` value
+ * is kept — that's how the edit form clears a field.
+ */
+export function buildContactUpdate(
+  contactData: Record<string, unknown>,
+): ContactUpdateAssignments {
+  const fields: string[] = [];
+  const values: unknown[] = [];
+
+  for (const [key, value] of Object.entries(contactData)) {
+    if (value === undefined) continue;
+    if (!UPDATABLE_CONTACT_COLUMNS.has(key)) continue;
+    fields.push(`${key} = $${values.length + 1}`);
+    values.push(value);
+  }
+
+  return { fields, values };
+}

src/models/Contact.ts

@@ -1,4 +1,5 @@
 import { query } from '@/lib/db';
+import { buildContactUpdate } from '@/lib/contact-update';
 
 export interface ContactData {
   id: number;
@@ -153,27 +154,22 @@ export class Contact {
   }
 
   static async update(id: number, contactData: Partial<ContactData>): Promise<ContactData | null> {
-    const fields = [];
-    const values = [];
-    let paramCount = 1;
-
-    for (const [key, value] of Object.entries(contactData)) {
-      if (value !== undefined && key !== 'id' && key !== 'created_at' && key !== 'updated_at') {
-        fields.push(`${key} = $${paramCount}`);
-        values.push(value);
-        paramCount++;
-      }
-    }
+    // Only ever writes columns from a fixed allowlist (see @/lib/contact-update).
+    // The PUT route passes the raw request body straight in, so filtering here
+    // prevents a caller from reassigning the QSO's owner (user_id) or injecting
+    // SQL through a crafted key — both were possible when every key was
+    // interpolated as a column name.
+    const { fields, values } = buildContactUpdate(contactData as Record<string, unknown>);
 
     if (fields.length === 0) {
       return null;
     }
 
     values.push(id);
     const sql = `
-      UPDATE contacts 
-      SET ${fields.join(', ')} 
-      WHERE id = $${paramCount}
+      UPDATE contacts
+      SET ${fields.join(', ')}
+      WHERE id = $${values.length}
       RETURNING *
     `;
 

tests/contact-update.spec.ts

@@ -0,0 +1,102 @@
+import { test, expect } from '@playwright/test';
+import {
+  buildContactUpdate,
+  UPDATABLE_CONTACT_COLUMNS,
+} from '@/lib/contact-update';
+
+// Pure-function tests for the contact-update SET-clause builder. This is the
+// shared assignment builder behind PUT /api/contacts/[id] (via Contact.update)
+// — it turns a partial contact object into `col = $n` fragments plus their
+// bound values. No DB needed.
+//
+// The regression these guard: Contact.update used to interpolate *every* key of
+// the request body straight into the SQL as a column name (`SET ${key} = $n`),
+// and the PUT route hands it the raw JSON body. So a caller could reassign a
+// QSO to another operator by sending `user_id`, or inject SQL through a crafted
+// key (the key was concatenated, never bound). The builder must therefore only
+// ever emit column names drawn from a fixed allowlist.
+
+test.describe('buildContactUpdate', () => {
+  test('emits `col = $n` assignments in lockstep with their bound values', () => {
+    const { fields, values } = buildContactUpdate({
+      callsign: 'W1AW',
+      mode: 'CW',
+      frequency: 14.074,
+    });
+    expect(fields).toEqual([
+      'callsign = $1',
+      'mode = $2',
+      'frequency = $3',
+    ]);
+    expect(values).toEqual(['W1AW', 'CW', 14.074]);
+  });
+
+  test('skips undefined values without advancing the placeholder counter', () => {
+    const { fields, values } = buildContactUpdate({
+      callsign: 'W1AW',
+      name: undefined,
+      band: '20M',
+    });
+    expect(fields).toEqual(['callsign = $1', 'band = $2']);
+    expect(values).toEqual(['W1AW', '20M']);
+  });
+
+  test('includes null values so a field can be cleared on edit', () => {
+    const { fields, values } = buildContactUpdate({ grid_locator: null });
+    expect(fields).toEqual(['grid_locator = $1']);
+    expect(values).toEqual([null]);
+  });
+
+  test('drops ownership/identity columns — no reassigning a QSO to another user', () => {
+    const { fields, values } = buildContactUpdate({
+      user_id: 999,
+      id: 5,
+      created_at: '2020-01-01',
+      updated_at: '2020-01-01',
+      callsign: 'W1AW',
+    });
+    // Only the legitimate edit survives; user_id/id/timestamps are dropped.
+    expect(fields).toEqual(['callsign = $1']);
+    expect(values).toEqual(['W1AW']);
+  });
+
+  test('drops unknown / crafted keys instead of interpolating them as SQL', () => {
+    const { fields, values } = buildContactUpdate({
+      // A key crafted to inject SQL through the old column-name concatenation.
+      "user_id = 1, notes": 'pwned',
+      'not_a_real_column': 'x',
+      callsign: 'W1AW',
+    });
+    expect(fields).toEqual(['callsign = $1']);
+    expect(values).toEqual(['W1AW']);
+  });
+
+  test('returns no assignments when nothing updatable is provided', () => {
+    const { fields, values } = buildContactUpdate({
+      user_id: 999,
+      bogus: 'x',
+    });
+    expect(fields).toEqual([]);
+    expect(values).toEqual([]);
+  });
+
+  test('placeholders count only included fields when forbidden keys are interleaved', () => {
+    const { fields, values } = buildContactUpdate({
+      callsign: 'W1AW',
+      user_id: 999, // dropped — must not consume $2
+      band: '20M',
+    });
+    expect(fields).toEqual(['callsign = $1', 'band = $2']);
+    expect(values).toEqual(['W1AW', '20M']);
+  });
+
+  test('every editable contacts column is updatable, none of the identity ones', () => {
+    // Sanity-check the allowlist against the columns a user legitimately edits.
+    for (const col of ['callsign', 'mode', 'band', 'notes', 'tx_pwr', 'grid_locator', 'qsl_lotw']) {
+      expect(UPDATABLE_CONTACT_COLUMNS.has(col)).toBe(true);
+    }
+    for (const col of ['id', 'user_id', 'created_at', 'updated_at']) {
+      expect(UPDATABLE_CONTACT_COLUMNS.has(col)).toBe(false);
+    }
+  });
+});