Skip to content

fix(screenscraper): stop persisting SS media download URLs (#3612)#3616

Draft
Spinnich wants to merge 1 commit into
rommapp:masterfrom
Spinnich:fix/ss-media-url-storage-3612
Draft

fix(screenscraper): stop persisting SS media download URLs (#3612)#3616
Spinnich wants to merge 1 commit into
rommapp:masterfrom
Spinnich:fix/ss-media-url-storage-3612

Conversation

@Spinnich

@Spinnich Spinnich commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Description

ScreenScraper media URLs were saved into the database during a scan, even though every asset is also downloaded to a local file that the UI actually renders from. They lived in two places on the roms table:

  • the shared url_cover / url_manual / url_screenshots columns, and
  • ~18 *_url entries inside the ss_metadata JSON blob.

These URLs are ScreenScraper API queries that carry authentication parameters (devid/devpassword/ssid/sspassword) and are never read after the in-scan download. Persisting them writes authentication detail into the DB (and into /api/roms/* responses) for no functional gain — an avoidable risk for anyone who backs up or shares their database, logs, or support bundles. This closes #3612.

The approach: the SS URLs are now held only in memory long enough to drive the download, then dropped before anything is persisted. Because the assets are keyed by the stored ss_id (non-sensitive) and every scan that downloads SS art already re-queries ScreenScraper, the URLs are always re-derivable and never need to be stored.

The one thing the stored URL was still used for after a scan is the re-scan "has this asset changed?" check. To preserve that without storing a query, we now persist a tiny, non-sensitive variant tag instead — the resolved media descriptor, e.g. box-2D(wor) — in ss_metadata (cover_media / manual_media / screenshot_media). On a re-scan, comparing that tag detects when a config change (e.g. a new region priority) selects a different asset, so UPDATE-scan refresh keeps working. Other metadata providers' URLs (IGDB, Moby, …) are plain public CDN links and are left untouched.

What changed and why

  • Stop persisting SS URLs. ss_metadata.*_url is dropped at the persist boundary; SS-sourced url_cover/url_manual/url_screenshots are blanked. Non-SS provider URLs are kept as valid cover fallbacks.
  • Keep the UPDATE-scan change check via a non-sensitive variant tag rather than the URL, so config-driven art changes still re-download.
  • Re-attach credentials only at download time, host-gated to screenscraper.fr, and never store them anywhere.
  • Clean up existing rows with a data migration.

Files modified

  • backend/handler/metadata/ss_handler.py — Core logic. Strip the full SS auth param set (ssid/sspassword/devid/devpassword) from in-memory media URLs; re-add the full set in add_ss_auth_to_url at download time. New helpers: pop_ss_media_urls (drop the *_url keys), _ss_media_descriptor (extract the media tag), public is_screenscraper_url (host gate, renamed from a private helper). New SSMetadata tag fields and tag computation in build_ss_game.
  • backend/endpoints/sockets/scan.py — Capture the SS media URLs and blank the SS-sourced url columns before add_rom; download cover/manual/screenshots and special media from the captured URLs; change the cover/manual/screenshots overwrite decision to "stored URL changed (non-SS) OR variant tag changed (SS)."
  • backend/endpoints/roms/__init__.py — Manual-edit / ss_id-change flow: drop *_url and blank SS-sourced url_cover/url_manual before update_rom (after the download has consumed them).
  • backend/alembic/versions/0092_strip_ss_media_urls.py — Batched data migration: drop every *_url from ss_metadata, and blank url_cover/url_manual / drop SS entries from url_screenshots only when they point at ScreenScraper. Cross-dialect (typed CustomJSON). Downgrade is intentionally a no-op (the stripped data can't be reconstructed; local assets remain valid).
  • backend/tests/handler/metadata/test_ss_handler.py — Tests for auth stripping, dev-cred re-attachment, pop_ss_media_urls, is_screenscraper_url, _ss_media_descriptor, and build_ss_game tag output.
  • backend/tests/alembic/test_0092_strip_ss_media_urls.py (+ __init__.py) — Unit tests for the migration's transform helpers (host gating, URL blanking, *_url removal).
  • frontend/src/__generated__/models/RomSSMetadata.ts — Regenerated types; RomSSMetadata gained the three optional tag fields (additive, backward-compatible).

Testing notes

  • Full backend suite green (1817 passed); trunk fmt && trunk check (ruff/black/isort/mypy/bandit) clean.
  • Migration verified on the dev MariaDB: upgrade headdowngrade -1upgrade head.
  • Verified against full real scans (ScreenScraper enabled):
    • Initial/COMPLETE: art downloads succeed, credentials re-attached and redacted in logs, url_cover stored blank, cover_media tag stored, ss_metadata.*_url absent.
    • UPDATE with no config change: ScreenScraper re-queried for ~107 roms, zero redundant asset downloads (the tag check correctly skipped everything).
    • UPDATE after changing region priority: only the assets whose resolved region actually changed re-downloaded (all 200); covers stayed put as expected, since RomM's region selection is dominated by each rom's own filename region tag.

Reviewer attention

  • The overwrite decision in scan.py is the core new logic (url changed OR SS variant tag changed). Most relevant on UPDATE scans.
  • One-time self-heal: the migration does not back-fill the new tags (they can't be derived without re-querying SS), so the first UPDATE scan after upgrade re-downloads each SS rom's art once, then settles.
  • Deliberate scope boundary: SS "special" media (miximage, logo, bezel, marquee) still has no change check (store_media_file skips if the file exists), so it refreshes only on a Complete rescan while cover/screenshots refresh on UPDATE. This asymmetry is pre-existing and intentionally left out of scope; happy to file a follow-up.
  • v1 UI: for SS roms, the legacy "re-download cover/manual" buttons and the cover "source" link key off the now-blank URL and will hide. Viewing/downloading the assets is unaffected (those use path_*).
  • API shape: ss_metadata no longer contains *_url values and gains the three optional tag fields; existing consumers read *_path, not *_url.

Checklist

  • I've tested the changes locally
  • I've updated relevant comments
  • I've assigned reviewers for this PR
  • I've added unit tests that cover the changes

AI assistance disclosure: This PR — the code, the migration, the tests, and this description — was written primarily by Claude Code (Opus), under my direction and review. I verified the behavior against real scans locally as described above.

@Spinnich Spinnich requested a review from gantoine June 26, 2026 23:10
)

ScreenScraper media URLs were saved to roms.ss_metadata (the *_url entries)
and to url_cover/url_manual/url_screenshots during a scan, even though the
assets are downloaded to local files that the UI renders from. The stored
URLs are SS API queries carrying auth params and are never read after the
download, so they are an avoidable privacy/cleanliness liability.

- Drop ss_metadata.*_url entirely (pop_ss_media_urls at the persist boundary).
- Blank SS-sourced url_cover/url_manual/url_screenshots; other providers'
  public URLs are kept as valid cover fallbacks (is_screenscraper_url gate).
- Persist the resolved media variant tag (e.g. "box-2D(wor)") in ss_metadata
  (cover_media/manual_media/screenshot_media) so the re-scan change-check
  still detects region/media config changes without storing a URL.
- Download from the fresh in-memory URLs; credentials are re-added only at
  download time and never stored.
- Migration 0092 cleans existing rows; frontend types regenerated.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Spinnich Spinnich force-pushed the fix/ss-media-url-storage-3612 branch from c91d517 to f14abd6 Compare June 27, 2026 00:12
@gantoine gantoine self-assigned this Jun 28, 2026
@gantoine gantoine marked this pull request as draft June 28, 2026 18:09
@gantoine

Copy link
Copy Markdown
Member

after a quick glance this seems both too specific to ssfr and too complicated for what it's trying to accomplish. i've set it as draft and will revisit after 5.0

@gantoine gantoine removed their request for review June 28, 2026 18:10
@gantoine gantoine removed their assignment Jun 28, 2026
@gantoine gantoine added the on-hold Pending further research or blocked by another issue label Jun 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

on-hold Pending further research or blocked by another issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Vestigial ScreenScraper download URLs are still saved in the database after assets are downloaded locally

2 participants