fix: allowlist updatable columns to close SQL injection via column names
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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<T>` 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<T extends object>(
|
||||
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<Artist>): Promise<Artist> {
|
||||
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<Album>): Promise<Album> {
|
||||
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<Track>): Promise<Track> {
|
||||
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(', ');
|
||||
|
||||
Reference in New Issue
Block a user