From 1e59d21dee5121a73619e0eadf6b8e24a68ffccc Mon Sep 17 00:00:00 2001 From: kami Date: Thu, 30 Jul 2026 23:47:20 +0400 Subject: [PATCH] fix: allowlist updatable columns to close SQL injection via column names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit updateTrack/updateArtist/updateAlbum built their SET clause from Object.keys(data) where data is `request.body as any`, interpolating request-supplied keys straight into SQL as quoted identifiers: fields.map((f, i) => `"${f}" = $${i + 2}`) A crafted body key closes the quoted identifier and injects into the SET list. Mitigated in practice only by the LAN/VPN-only proxy — which supplies the auth token automatically, so any device on the LAN could reach it from a browser. Adds per-table UPDATABLE_COLUMNS plus an allowedFields() helper, applied in all three methods. The allowlist lives in the service layer rather than the routes so it covers every caller. Unknown keys are dropped rather than rejected: the three routes do no error mapping, so a throw surfaces as a bare 500, and the pre-existing "No fields to update" error still fires for a payload rejected in its entirety. Also closes plain mass-assignment. Excluded: path/hash/mtime (scanner-owned; path is the only link to the read-only bind), state/quarantined_at/deleted_at (dislike lifecycle and integrity sweep), play_count/skip_count/dislike_count/last_played_at (learning signal — forgeable counters poison the engine), and identity/generated columns. The only callers are the three HTTP PUTs; the frontend's update* service exports are dead code, so nothing relied on writing an excluded column. REVIEW-2026-07-30.md finding 5. Co-Authored-By: Claude Opus 5 --- backend/src/services/db.service.ts | 49 ++++++++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/backend/src/services/db.service.ts b/backend/src/services/db.service.ts index 0da2109..d22f288 100644 --- a/backend/src/services/db.service.ts +++ b/backend/src/services/db.service.ts @@ -176,6 +176,49 @@ export interface RepetitionRule { // Add new entries at the END. Never edit or remove existing entries. // Convention for id: "YYYYMMDD_short_description" // --------------------------------------------------------------------------- +/** + * Per-table allowlists of columns the HTTP `PUT /artists|albums|tracks/:id` + * endpoints may write. + * + * These endpoints pass `request.body as any` straight through, and the update + * builders interpolate `Object.keys(body)` into the SET clause as quoted + * identifiers — so without an allowlist a crafted key both closes the quoted + * identifier (SQL injection) and mass-assigns columns that are not user-editable. + * + * Deliberately excluded: + * - `tracks.path` / `tracks.hash` / `tracks.mtime` — owned by the scanner; the + * path is the only link to the file on the read-only `/music` bind. + * - `tracks.state` / `quarantined_at` / `deleted_at` — owned by the dislike + * lifecycle and the integrity sweep, not by metadata editing. + * - `play_count` / `skip_count` / `dislike_count` / `last_played_at` — learning + * signal; forgeable counters would poison the recommendation engine. + * - `source_type` — decides library vs. recommendation semantics. + * - `id` / `created_at` / `updated_at` and every generated column + * (`normalized_name`, `normalized_artist` — Postgres rejects writes anyway). + */ +const UPDATABLE_COLUMNS = { + artists: ['name', 'canonical_name', 'sort_name', 'mbid', 'discogs_id', 'image_path'], + albums: ['artist_id', 'title', 'year', 'release_date', 'artwork_id', 'mbid'], + tracks: ['title', 'artist', 'album_id', 'duration', 'release_date'], +} as const; + +/** + * Filter a partial update payload down to the allowlisted, defined columns for + * `table`. Unknown keys are dropped silently rather than raising: the callers + * take `Partial` and the routes do no error mapping, so a 500 on an extra + * key would be worse than a no-op. The subsequent "No fields to update" throw + * still surfaces a payload that was *entirely* rejected. + */ +function allowedFields( + table: keyof typeof UPDATABLE_COLUMNS, + data: T +): (keyof T & string)[] { + const allowed = UPDATABLE_COLUMNS[table] as readonly string[]; + return Object.keys(data).filter( + (k) => allowed.includes(k) && (data as any)[k] !== undefined + ) as (keyof T & string)[]; +} + const MIGRATIONS: { id: string; sql: string }[] = [ { id: '20260608_track_artists', @@ -1244,7 +1287,7 @@ export class DbService { } async updateArtist(id: string, data: Partial): Promise { - const fields = Object.keys(data).filter(k => data[k as keyof Artist] !== undefined); + const fields = allowedFields('artists', data); if (fields.length === 0) throw new Error('No fields to update'); // Normalize name if it's being updated (the normalized_name generated column @@ -1277,7 +1320,7 @@ export class DbService { } async updateAlbum(id: string, data: Partial): Promise { - const fields = Object.keys(data).filter(k => data[k as keyof Album] !== undefined); + const fields = allowedFields('albums', data); if (fields.length === 0) throw new Error('No fields to update'); const setClause = fields.map((f, i) => `"${f}" = $${i + 2}`).join(', '); @@ -1320,7 +1363,7 @@ export class DbService { } async updateTrack(id: string, data: Partial): Promise { - const fields = Object.keys(data).filter(k => data[k as keyof Track] !== undefined); + const fields = allowedFields('tracks', data); if (fields.length === 0) throw new Error('No fields to update'); const setClause = fields.map((f, i) => `"${f}" = $${i + 2}`).join(', ');