Fix stored XSS in retune modal

CHANGELOG.md

@@ -7,10 +7,50 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
-### Added
-- Library card actions can now provide per-song label and icon callbacks, so
-  plugins can render dynamic card badges without DOM patching.
-- **Core reader for source rigs (feedpak 1.18.0).** A pack can declare what a
+### Security
+
+- **Fixed stored XSS in the retune modal.** `retuneSong()` injected
+  `title`/`target`/`msg.filename`/`msg.error` into a modal's `innerHTML` via
+  unescaped template literals. `song.title` is attacker-influenceable
+  (imported GP/MusicXML/sloppak metadata) and reaches this sink directly
+  from the library card's "Convert to E Standard" menu action. Wrapped all
+  four values in the existing `esc()` helper, matching the escaping pattern
+  used everywhere else in the file.
+- **Hardened GP/arrangement XML parsing against entity-expansion ("billion
+  laughs") DoS** (#45). `xml.etree.ElementTree` has no built-in protection
+  against maliciously nested XML entities on untrusted input; a crafted
+  imported GP or arrangement XML file could exhaust memory/CPU on the
+  request thread. Added `lib/safe_xml.py`, a shared hardened-parse helper
+  using `defusedxml` (now a `requirements.txt` dependency, falling back to
+  stdlib with a logged warning if somehow absent), and switched every
+  untrusted-XML parse call site (`lib/gp2rs_gpx.py`, `lib/loosefolder.py`,
+  `lib/song.py`, `lib/routers/ws_highway.py`) to use it. Rejections are
+  normalised to `ET.ParseError` so existing `except ET.ParseError:` call
+  sites keep working unmodified.
+- **Capped decompressed size on sloppak zip extraction** (#46). Nothing
+  bounded a `.sloppak` zip's total decompressed size during extraction — a
+  highly-compressed malicious/corrupt pack could exhaust disk space (a "zip
+  bomb"), independent of the unpack-cache LRU eviction (which only bounds
+  the aggregate cache after the fact). `_unpack_zip()` now sums each
+  member's declared size against an 8 GB default cap (no decompression
+  needed to check it), aborting and cleaning up any partial extraction if
+  exceeded. Override with `FEEDBACK_SLOPPAK_MAX_UNPACK_MB` (`0` disables).
+- **Added baseline security headers to every response** (#47):
+  `X-Content-Type-Options: nosniff`, `X-Frame-Options: DENY`,
+  `Referrer-Policy: strict-origin-when-cross-origin`, and a
+  `Content-Security-Policy` restricting `object-src`, `base-uri`, and
+  `frame-ancestors`, and requiring `'self'` or `https:` for
+  script/style/media/connect origins. `script-src`/`style-src` still permit
+  `'unsafe-inline'` — the v3 UI's own HTML uses `onclick="..."` attributes
+  throughout and inline `<script>`/`<style>` blocks, none of it nonce'd or
+  externalized today, so a stricter policy would need that rewritten first.
+  Defense-in-depth: this would have limited the blast radius of the retune
+  XSS above (and any future/residual one) even before that fix landed.
+
+### Added
+- Library card actions can now provide per-song label and icon callbacks, so
+  plugins can render dynamic card badges without DOM patching.
+- **Core reader for source rigs (feedpak 1.18.0).** A pack can declare what a
   MIDI part should sound like by binding a rig; core now reads that binding and
   hands it to the client instead of dropping it. Three parts: the
   `tone_changes` WS message carries the pack's rig bindings (`base_rig`, and

lib/gp2rs_gpx.py

@@ -19,6 +19,7 @@
 from pathlib import Path
 
 from safepath import safe_join
+from safe_xml import safe_fromstring
 
 _log = logging.getLogger("feedBack.lib.gp2rs_gpx")
 
@@ -158,7 +159,7 @@ def _load_gpif(gp_path: str) -> ET.Element:
         with zipfile.ZipFile(_io.BytesIO(raw)) as zf:
             if 'Content/score.gpif' not in zf.namelist():
                 raise ValueError("Content/score.gpif not found in GP7/GP8 ZIP container")
-            return ET.fromstring(zf.read('Content/score.gpif'))
+            return safe_fromstring(zf.read('Content/score.gpif'))
 
     # GP6 (.gpx): BCFZ compressed or raw BCFS
     if raw[:4] == b'BCFZ':
@@ -173,7 +174,7 @@ def _load_gpif(gp_path: str) -> ET.Element:
     fs = _parse_bcfs(bcfs)
     if 'score.gpif' not in fs:
         raise ValueError("score.gpif not found in GPX container")
-    return ET.fromstring(fs['score.gpif'])
+    return safe_fromstring(fs['score.gpif'])
 
 
 # ---------------------------------------------------------------------------

lib/loosefolder.py

@@ -23,9 +23,10 @@
 
 import json
 import math
-import xml.etree.ElementTree as ET
 from pathlib import Path
 
+from safe_xml import safe_parse
+
 AUDIO_NAMES = ["audio.wem", "song.wem"]
 # Match every extension server.get_song_art is prepared to serve
 # (jpeg/png/webp). Without `.jpeg`/`.webp`, loose folders shipping
@@ -109,7 +110,7 @@ def _named_audio_ok(name: str) -> bool:
         return False
     for xml in _iter_local_xmls(path):
         try:
-            root_tag = ET.parse(str(xml)).getroot().tag
+            root_tag = safe_parse(str(xml)).getroot().tag
         except Exception:
             continue
         if root_tag == "song":
@@ -178,7 +179,7 @@ def _arr_type_from_filename(stem: str) -> tuple:
 def _parse_xml_meta(xml_path: Path) -> dict:
     """Parse a chart arrangement XML and return song-level metadata."""
     try:
-        root = ET.parse(str(xml_path)).getroot()
+        root = safe_parse(str(xml_path)).getroot()
         if root.tag != "song":
             return {}
 
@@ -299,7 +300,7 @@ def _has_lyrics(path: Path) -> bool:
     """Return True if any XML in the folder is a vocals track."""
     for xml in _iter_local_xmls(path):
         try:
-            if ET.parse(str(xml)).getroot().tag == "vocals":
+            if safe_parse(str(xml)).getroot().tag == "vocals":
                 return True
         except Exception:
             pass

lib/routers/ws_highway.py

@@ -47,6 +47,7 @@
 import loosefolder as loosefolder_mod
 from metadata_db import _arr_smart_sort_key
 from dlc_paths import _get_dlc_dir, _resolve_dlc_path
+from safe_xml import safe_parse
 
 import appstate
 
@@ -745,7 +746,7 @@ def _evict_audio_cache():
         else:
             for xml_path in sorted(_xml_walk("*.xml")):
                 try:
-                    root = ET.parse(xml_path).getroot()
+                    root = safe_parse(xml_path).getroot()
                     if root.tag == "vocals":
                         # An empty <vocals/> shell would otherwise
                         # short-circuit later XML files, so only stop
@@ -911,7 +912,7 @@ def _xml_rank(xp):
             tone_base = ""  # <tonebase> of the preferred arrangement XML
             for xml_path in sorted_xml:
                 try:
-                    root = ET.parse(xml_path).getroot()
+                    root = safe_parse(xml_path).getroot()
                     if root.tag != "song":
                         continue
                     if _suppress_fallback and _xml_rank(xml_path) == 2:

lib/safe_xml.py

@@ -0,0 +1,54 @@
+"""Hardened XML parsing shared by every GP/arrangement-XML import path
+(security audit, issue #45).
+
+``xml.etree.ElementTree`` has no built-in protection against
+entity-expansion ("billion laughs") DoS on untrusted input.
+``defusedxml`` hardens both ``parse()`` and ``fromstring()`` against it
+when installed. Centralised here so every call site that parses
+attacker-influenceable XML (imported Guitar Pro / arrangement files) uses
+the same guarded fallback, instead of each duplicating the inline
+try/except that lib/gp8_audio_sync.py and lib/gp_autosync.py used locally
+before this module existed.
+
+Attacks defusedxml rejects (``EntitiesForbidden``, ``DTDForbidden``, ...)
+are normalised to ``ET.ParseError`` so existing ``except ET.ParseError:``
+call sites keep working unmodified — a rejected malicious file should be
+treated the same as a malformed one, not crash the caller.
+"""
+import logging
+import xml.etree.ElementTree as ET
+
+log = logging.getLogger("feedBack.lib.safe_xml")
+
+try:
+    import defusedxml.ElementTree as _safe_ET
+    from defusedxml.common import DefusedXmlException as _DefusedXmlException
+    _HAVE_DEFUSEDXML = True
+except ImportError:
+    _safe_ET = None
+    _DefusedXmlException = ()
+    _HAVE_DEFUSEDXML = False
+    log.warning(
+        "safe_xml: defusedxml not installed; parsing untrusted XML with "
+        "stdlib xml.etree (install defusedxml for hardened parsing)"
+    )
+
+
+def safe_parse(source):
+    """Hardened equivalent of ``ET.parse(source)``."""
+    if not _HAVE_DEFUSEDXML:
+        return ET.parse(source)
+    try:
+        return _safe_ET.parse(source)
+    except _DefusedXmlException as e:
+        raise ET.ParseError(str(e)) from e
+
+
+def safe_fromstring(text):
+    """Hardened equivalent of ``ET.fromstring(text)``."""
+    if not _HAVE_DEFUSEDXML:
+        return ET.fromstring(text)
+    try:
+        return _safe_ET.fromstring(text)
+    except _DefusedXmlException as e:
+        raise ET.ParseError(str(e)) from e

lib/sloppak.py

@@ -351,19 +351,53 @@ def _unpack_lock_for(filename: str) -> threading.Lock:
         return lk
 
 
+
+# Bounds a single zip's total DECOMPRESSED size during extraction — a highly
+# compressed malicious/corrupt .sloppak could otherwise exhaust disk space
+# (a "zip bomb") before anyone notices, independent of the unpack-cache
+# eviction above (which only bounds the aggregate cache after the fact, not
+# a single extraction in progress). Default 8 GB is generous for a real
+# multi-stem pack while still bounding the worst case. Override with
+# FEEDBACK_SLOPPAK_MAX_UNPACK_MB (0 disables the cap).
+def _unpack_max_bytes() -> int:
+    raw = os.environ.get("FEEDBACK_SLOPPAK_MAX_UNPACK_MB", "").strip()
+    try:
+        mb = int(raw) if raw else 8192
+    except ValueError:
+        mb = 8192
+    return max(0, mb) * 1024 * 1024
+
+
 def _unpack_zip(zip_path: Path, dest: Path) -> None:
     """Extract a sloppak zip archive into dest, replacing any previous contents.
 
     Members whose names escape ``dest`` via ``..`` segments, absolute paths, or
     Windows-style separators are skipped with a warning so a crafted sloppak
     can't write outside the unpack cache (zip-slip).
+
+    Raises ValueError if the archive's total declared decompressed size
+    exceeds the cap, aborting and cleaning up any partial extraction.
     """
     if dest.exists():
         shutil.rmtree(dest, ignore_errors=True)
     dest.mkdir(parents=True, exist_ok=True)
     dest_resolved = dest.resolve()
+    max_bytes = _unpack_max_bytes()
+    written = 0
     with zipfile.ZipFile(str(zip_path), "r") as zf:
         for member in zf.infolist():
+            # Declared size from the central directory — no decompression
+            # needed, so an oversized archive is caught before doing any
+            # real work on it.
+            if max_bytes and not member.is_dir():
+                written += member.file_size
+                if written > max_bytes:
+                    shutil.rmtree(dest, ignore_errors=True)
+                    raise ValueError(
+                        f"sloppak archive exceeds the {max_bytes} byte decompressed-size "
+                        f"cap (FEEDBACK_SLOPPAK_MAX_UNPACK_MB) — refusing to extract "
+                        f"what looks like a corrupt or malicious pack"
+                    )
             target = safe_join(dest_resolved, member.filename)
             if target is None:
                 log.warning("sloppak: rejected unsafe zip member %r", member.filename)

lib/song.py

@@ -8,6 +8,8 @@
 import math
 import xml.etree.ElementTree as ET
 
+from safe_xml import safe_parse
+
 log = logging.getLogger("feedBack.lib.song")
 
 
@@ -1118,7 +1120,7 @@ def _parse_note(n) -> Note:
 
 def parse_arrangement(xml_path: str) -> Arrangement:
     """Parse a chart arrangement XML file."""
-    tree = ET.parse(xml_path)
+    tree = safe_parse(xml_path)
     root = tree.getroot()
 
     # Name
@@ -1566,7 +1568,7 @@ def _mprop_int(key: str, props: dict) -> int:
     metadata_loaded = False
     for xml_path in xml_files:
         try:
-            tree = ET.parse(xml_path)
+            tree = safe_parse(xml_path)
             root = tree.getroot()
         except ET.ParseError:
             continue

requirements.txt

@@ -29,3 +29,10 @@ requests>=2.31
 # Linux bundles, causing "backend failed before startup"). Declare it
 # explicitly, same as requests above.
 sniffio>=1.3.1
+
+# Hardened XML parsing for untrusted GP/arrangement imports (security audit,
+# issue #45) — guards xml.etree.ElementTree's entity-expansion ("billion
+# laughs") DoS. Used by lib/safe_xml.py, lib/gp8_audio_sync.py, and
+# lib/gp_autosync.py, all of which fall back to stdlib xml.etree with a
+# logged warning if this is somehow absent at runtime.
+defusedxml>=0.7.1

server.py

@@ -102,6 +102,45 @@
 # opaque proxy-generated hex strings, not just RFC-4122 UUIDs.
 app.add_middleware(CorrelationIdMiddleware, validator=None)
 
+# Baseline security headers (security audit, issue #47) — defense-in-depth
+# against any future/residual XSS, clickjacking, and MIME-sniffing, applied
+# to every response. The CSP can't go stricter than 'unsafe-inline' for
+# script-src/style-src without a large rewrite: the v3 UI's own HTML uses
+# `onclick="..."` attributes throughout (static/v3/index.html), and inline
+# <script>/<style> blocks — none of that is nonce'd or externalized today.
+# script-src/style-src still restrict *which origins* may be loaded from to
+# 'self' plus https:, so an injected `<script src="http://evil...">` (or any
+# non-https origin) stays blocked; only inline execution remains open, same
+# as before this header existed. https: is intentionally broad rather than
+# an explicit CDN allowlist: plugins are user-installed and each may load
+# from a different CDN (see feedBack-plugin-piano/-staffview/-tabview),
+# which a fixed allowlist can't anticipate.
+_SECURITY_HEADERS = {
+    "X-Content-Type-Options": "nosniff",
+    "X-Frame-Options": "DENY",
+    "Referrer-Policy": "strict-origin-when-cross-origin",
+    "Content-Security-Policy": (
+        "default-src 'self'; "
+        "script-src 'self' 'unsafe-inline' https:; "
+        "style-src 'self' 'unsafe-inline' https:; "
+        "img-src 'self' data: blob: https:; "
+        "font-src 'self' data: https:; "
+        "media-src 'self' blob: https:; "
+        "connect-src 'self' https: wss: ws:; "
+        "object-src 'none'; "
+        "base-uri 'self'; "
+        "frame-ancestors 'none'"
+    ),
+}
+
+
+@app.middleware("http")
+async def _security_headers(request: Request, call_next):
+    response = await call_next(request)
+    for header, value in _SECURITY_HEADERS.items():
+        response.headers.setdefault(header, value)
+    return response
+
 STATIC_DIR = Path(__file__).parent / "static"
 try:
     STATIC_DIR.mkdir(exist_ok=True)

static/app.js

@@ -645,8 +645,8 @@
     modal.className = 'fixed inset-0 z-[200] flex items-center justify-center bg-black/70 backdrop-blur-sm';
     modal.innerHTML = `
         <div class="bg-dark-700 border border-gray-700 rounded-2xl p-8 w-full max-w-md mx-4 shadow-2xl">
-            <h3 class="text-lg font-bold text-white mb-1">Converting to ${target}</h3>
-            <p class="text-sm text-gray-400 mb-5">${title}</p>
+            <h3 class="text-lg font-bold text-white mb-1">Converting to ${esc(target)}</h3>
+            <p class="text-sm text-gray-400 mb-5">${esc(title)}</p>
             <div class="progress-bar mb-3"><div class="fill" id="retune-bar" style="width:0%"></div></div>
             <p class="text-xs text-gray-500" id="retune-stage">Connecting...</p>
         </div>`;
@@ -666,7 +666,7 @@
                 <div class="text-center">
                     <div class="text-3xl mb-3">✓</div>
                     <h3 class="text-lg font-bold text-white mb-1">Done!</h3>
-                    <p class="text-sm text-gray-400 mb-5">${msg.filename}</p>
+                    <p class="text-sm text-gray-400 mb-5">${esc(msg.filename)}</p>
                     <button onclick="document.getElementById('retune-modal').remove();loadLibrary()"
                         class="bg-accent hover:bg-accent-light px-6 py-2 rounded-xl text-sm font-semibold text-white transition">OK</button>
                 </div>`;
@@ -676,7 +676,7 @@
                 <div class="text-center">
                     <div class="text-3xl mb-3">✕</div>
                     <h3 class="text-lg font-bold text-red-400 mb-1">Failed</h3>
-                    <p class="text-sm text-gray-400 mb-5">${msg.error}</p>
+                    <p class="text-sm text-gray-400 mb-5">${esc(msg.error)}</p>
                     <button onclick="document.getElementById('retune-modal').remove()"
                         class="bg-dark-600 hover:bg-dark-500 px-6 py-2 rounded-xl text-sm text-gray-300 transition">Close</button>
                 </div>`;
@@ -1498,7 +1498,7 @@
         'background:rgba(0,0,0,0.6)',
         'font:14px/1.4 system-ui,-apple-system,"Segoe UI",Roboto,sans-serif',
     ].join(';');
 
     const card = document.createElement('div');
     card.style.cssText = [
         'max-width:min(92vw,360px)', 'padding:18px 18px 14px',