From d9529fc801ecf7234f8c3cc21e8bc154376691bc Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Fri, 15 May 2026 09:33:15 -0700 Subject: [PATCH] Token leak round 2: artist endpoint + playlist sync + URL-encoded redaction The first token-leak fix scrubbed the artwork URL fixer's own log calls. This catches three more sites that ALSO leaked tokens, plus one upstream gap that let URL-encoded tokens slip through the redactor. Three sites in `web_server.py` (artist endpoint at line 8765-8773): - "Artist image before fix: '...'" -- logged the raw image_url with the auth token in plain form. - "Artist image after fix: '...'" -- logged the URL-encoded form after it had been wrapped in the image proxy (`/api/image-proxy?url=`). - "Final artist data being sent: {...}" -- dumped the entire artist_info dict on every render, including the image_url field. All three were dev-time debug noise. Removed entirely. The "No artist image URL found" warning at line 8770 stays (no URL, just the artist name). One site in `core/discovery/sync.py:402`: - "[PLAYLIST IMAGE] image_url=..." -- logged the playlist poster URL during sync. Same auth-token leak risk for Plex / Jellyfin playlists. Changed to log only `has_image=True/False`. Upstream gap in `_redact_url_secrets`: - The original regex only matched plain query params (`?key=value`). When an auth-bearing URL gets wrapped inside another URL's query string (our `/api/image-proxy?url=` flow) the auth params end up percent-encoded -- `%3FX-Plex-Token%3D...` -- and slipped through. - New second pattern catches the URL-encoded form. Both passes run on every redact call; idempotent. Verified manually: /api/image-proxy?url=...%3FX-Plex-Token%3DABC... -> /api/image-proxy?url=...%3FX-Plex-Token%3D***REDACTED*** 6 artwork tests pass. --- core/discovery/sync.py | 7 +++++-- core/metadata/artwork.py | 33 +++++++++++++++++++++++++++------ web_server.py | 12 ++++++------ webui/static/helper.js | 1 + 4 files changed, 39 insertions(+), 14 deletions(-) diff --git a/core/discovery/sync.py b/core/discovery/sync.py index 7d63a51f..a77b5d3e 100644 --- a/core/discovery/sync.py +++ b/core/discovery/sync.py @@ -397,9 +397,12 @@ def run_sync_task(playlist_id, playlist_name, tracks_json, automation_id=None, p } logger.info(f"Sync finished for {playlist_id} - state updated") - # Set playlist poster image if available (Plex, Jellyfin, Emby) + # Set playlist poster image if available (Plex, Jellyfin, Emby). + # Don't log the URL itself — it may carry an auth token (Plex + # X-Plex-Token / Jellyfin X-Emby-Token / Subsonic auth) that we + # don't want persisted to app.log. _synced = getattr(result, 'synced_tracks', 0) - logger.info(f"[PLAYLIST IMAGE] image_url={playlist_image_url!r}, synced_tracks={_synced}") + logger.info(f"[PLAYLIST IMAGE] has_image={bool(playlist_image_url)}, synced_tracks={_synced}") if playlist_image_url and _synced > 0: try: active_server = deps.config_manager.get_active_media_server() diff --git a/core/metadata/artwork.py b/core/metadata/artwork.py index 0508122f..98af8431 100644 --- a/core/metadata/artwork.py +++ b/core/metadata/artwork.py @@ -37,22 +37,43 @@ _REDACT_QUERY_KEYS = ( 'x-plex-token', 'x-emby-token', 'api_key', 'apikey', 't', 's', 'p', 'token', 'password', ) -# Anchor on `?` / `&` (or string start) so short keys like `t` only -# match at parameter boundaries — not as a substring of `format=Jpg`. +_REDACT_KEYS_ALT = '|'.join(re.escape(k) for k in _REDACT_QUERY_KEYS) +# Plain form: `?key=value` or `&key=value`. Anchored on `?` / `&` (or +# string start) so short keys like `t` only match at parameter +# boundaries — not as a substring of `format=Jpg`. _REDACT_QUERY_RE = re.compile( - r'(?i)(?P^|[?&])(?P' + '|'.join(re.escape(k) for k in _REDACT_QUERY_KEYS) + r')=([^&\s]+)' + r'(?i)(?P^|[?&])(?P' + _REDACT_KEYS_ALT + r')=(?P[^&\s]+)' +) +# URL-encoded form: `%3Fkey%3Dvalue` or `%26key%3Dvalue`. The image +# proxy wraps the original URL via `?url=`, so the auth +# params end up encoded inside another URL. Without this second pass +# the encoded form survives plain redaction and ships to logs intact. +_REDACT_QUERY_RE_ENCODED = re.compile( + r'(?i)(?P%3F|%26)(?P' + _REDACT_KEYS_ALT + r')%3D(?P[^%&\s]+?)(?=%26|&|\s|$)' ) def _redact_url_secrets(url: str | None) -> str: """Mask sensitive query parameters in a URL so the result is safe - to log. Returns ``''`` for None/empty input. Idempotent.""" + to log. Handles both the plain form (``?token=abc``) and the URL- + encoded form (``%3Ftoken%3Dabc``) — the latter shows up when an + auth-bearing URL is wrapped inside another URL's query string + (e.g. our `/api/image-proxy?url=` flow). + + Returns ``''`` for None/empty input. Idempotent (safe to call on + already-redacted strings).""" if not url: return '' - return _REDACT_QUERY_RE.sub( + out = str(url) + out = _REDACT_QUERY_RE.sub( lambda m: f"{m.group('lead')}{m.group('key')}=***REDACTED***", - str(url), + out, ) + out = _REDACT_QUERY_RE_ENCODED.sub( + lambda m: f"{m.group('lead')}{m.group('key')}%3D***REDACTED***", + out, + ) + return out def normalize_image_url(thumb_url: str | None) -> str | None: diff --git a/web_server.py b/web_server.py index 4d2f136a..557d7c5a 100644 --- a/web_server.py +++ b/web_server.py @@ -8761,17 +8761,17 @@ def get_artist_detail(artist_id): logger.info(f"Found artist: {artist_info['name']} with {len(owned_releases['albums'])} albums") - # Fix artist image URL - logger.info(f"Artist image before fix: '{artist_info.get('image_url')}'") + # Fix artist image URL. + # NOTE: don't log image_url or the full artist_info dict here. + # The fixed URL embeds the media-server token (and the proxy + # variant URL-encodes it), so logging at INFO writes the token + # straight into app.log. Issue: tokens leaked to disk on every + # artist-page render until this was scrubbed. if artist_info.get('image_url'): artist_info['image_url'] = fix_artist_image_url(artist_info['image_url']) - logger.info(f"Artist image after fix: '{artist_info['image_url']}'") else: logger.warning(f"No artist image URL found for {artist_info['name']}") - # Debug final artist data being sent - logger.info(f"Final artist data being sent: {artist_info}") - # Fix image URLs for all albums for album in owned_releases['albums']: if album.get('image_url'): diff --git a/webui/static/helper.js b/webui/static/helper.js index 9a565df3..19226949 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3416,6 +3416,7 @@ const WHATS_NEW = { '2.5.2': [ // --- May 13, 2026 — 2.5.2 release --- { date: 'May 13, 2026 — 2.5.2 release' }, + { title: 'Token Leak Round 2: URL-Encoded Form In Artist Endpoint + Playlist Sync', desc: 'security follow-up to the prior token-leak fix. found three sites in `web_server.py` (artist endpoint) that logged the full `image_url` and the entire artist_info dict at INFO on every artist-page render — the dict contained the `image_url` field routed through the image proxy (`/api/image-proxy?url=`), URL-encoding the X-Plex-Token / X-Emby-Token / Subsonic auth straight into the log line. also one site in `core/discovery/sync.py` logged the playlist poster URL during sync. fixes: dropped the three artist-endpoint dev-time debug log lines entirely (before-fix, after-fix, "Final artist data being sent"). playlist-image log now logs `has_image=True/False`, not the URL. strengthened `_redact_url_secrets` with a second regex pattern that matches the URL-encoded form (`%3FX-Plex-Token%3D...`) so any future log-through-redactor catches both plain and encoded shapes. wipe your existing app.log if it captured tokens in either form, and rotate Plex / Jellyfin / Navidrome credentials.', page: 'settings' }, { title: 'Stop Leaking Plex / Jellyfin / Navidrome Tokens Into app.log', desc: 'security: artwork URL fixer was logging full media-server URLs (including the X-Plex-Token / X-Emby-Token / Subsonic auth params) at INFO level on every cover-art lookup. tokens piled up in app.log on disk — anyone with read access to the log file gained full read access to the user\'s media server. fix: log lines moved to DEBUG (so they don\'t persist by default) and routed through a new `_redact_url_secrets` helper that masks the values of `X-Plex-Token` / `X-Emby-Token` / `api_key` / `apikey` / Subsonic `t` / `s` / `p` / generic `token` / `password` query params. anchor regex on `?` or `&` boundary so short keys like `t` don\'t false-match inside `format=Jpg`. also dropped the noisy per-call "Plex/Jellyfin/Navidrome config - base_url: ..., token: ..." INFO lines that fired on every thumbnail. wipe your existing app.log if your config has been logged.', page: 'settings' }, { title: 'AcoustID + Quarantine Modal: Three Bug Fixes', desc: 'github issues #607 + #608. (1) live recordings no longer false-quarantine when AcoustID returns the live recording with a bare title — common case where venue/live annotation lives on the release entity, not the recording entity itself ("Clarity (Live at ...)" expected, AcoustID returns bare "Clarity"). new pure helper `core/matching/version_mismatch.py:is_acceptable_version_mismatch` accepts the mismatch only when one-sided live + bare AND fingerprint score >= 0.85 AND bare titles agree (>=0.7) AND artist matches (>=0.6). other version mismatches (instrumental, remix, acoustic, demo) stay strict — those have distinct fingerprints + MB always annotates them in the recording title. 23 boundary tests pin every shape; existing test_acoustid_version_mismatch suite passes unchanged. (2) audio-mismatch failure message no longer reports "identified as \'\' by \'\' (artist=100%)" when AcoustID returns multiple recordings — prior code mixed recordings[0]\'s strings (which can be empty) with best_rec\'s scores. now uses matched_title/matched_artist consistently in both the high-confidence-skip path and the final fail message. (3) quarantine modal Approve/Delete buttons no longer silently no-op when filename contains an apostrophe — id is now wrapped via `escapeHtml(JSON.stringify(id))` so quotes / backslashes / unicode all round-trip safely through the HTML attribute → JS string boundary. (4) bonus UX: quarantine entry expanded view now shows source uploader (username) + original soulseek filename when the sidecar carries that context, helping trace which uploader the bad file came from.', page: 'downloads' }, { title: 'Reorganize: Read Embedded Tags Instead Of Metadata API', desc: 'github issue #592: optional reorganize mode that uses each file\'s embedded tags as the canonical source of truth instead of doing a fresh metadata-source api lookup. useful for well-tagged libraries where api drift can produce inconsistent renames. zero api calls. opt-in via new "metadata mode" dropdown in the per-album reorganize modal + bulk reorganize-all modal — default stays "api" so existing pipelines are untouched. tag-mode reads via the same mutagen path the audit-trail modal uses (id3 / vorbis / mp4 all covered). honors id3-style "5/12" track and disc shapes, multi-value Artists tags, year normalization across 5 date formats, releasetype canonical tokens (album/single/ep/compilation). partial-album reorganize respects "totaldiscs" tag so disc 1 of a 2-disc set still routes into "Disc 1/" subfolder. plan items carry per-item `api_album` so each file\'s own album metadata flows through post-process, not a shared one. logic lifted to pure helper `core/library/reorganize_tag_source.py` with 49 boundary tests + 6 planner-level integration tests pinning every shape: missing essentials, multi-artist split across 9 separators, defensive paths, api-mode regression guard. 3171 tests pass — no regression.', page: 'tools' },