From cc5542133dca5aacef82dffea315364231aba65e Mon Sep 17 00:00:00 2001 From: Optio Agent Date: Fri, 24 Jul 2026 16:12:44 +0000 Subject: [PATCH] fix(contacts): stop edit route from reassigning QSO owner or injecting SQL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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": }` — 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) --- src/lib/contact-update.ts | 67 +++++++++++++++++++++++ src/models/Contact.ts | 24 ++++----- tests/contact-update.spec.ts | 102 +++++++++++++++++++++++++++++++++++ 3 files changed, 179 insertions(+), 14 deletions(-) create mode 100644 src/lib/contact-update.ts create mode 100644 tests/contact-update.spec.ts diff --git a/src/lib/contact-update.ts b/src/lib/contact-update.ts new file mode 100644 index 0000000..ca8619a --- /dev/null +++ b/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": }` (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([ + '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, +): 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 }; +} diff --git a/src/models/Contact.ts b/src/models/Contact.ts index ce13de0..7590d9c 100644 --- a/src/models/Contact.ts +++ b/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,17 +154,12 @@ export class Contact { } static async update(id: number, contactData: Partial): Promise { - 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); if (fields.length === 0) { return null; @@ -171,9 +167,9 @@ export class Contact { values.push(id); const sql = ` - UPDATE contacts - SET ${fields.join(', ')} - WHERE id = $${paramCount} + UPDATE contacts + SET ${fields.join(', ')} + WHERE id = $${values.length} RETURNING * `; diff --git a/tests/contact-update.spec.ts b/tests/contact-update.spec.ts new file mode 100644 index 0000000..5f7dd4b --- /dev/null +++ b/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); + } + }); +});