fix(security): guard XML parsing against entity-expansion DoS

CHANGELOG.md

@@ -9,6 +9,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Security
 
+- **Added an XML entity-expansion ("billion laughs") guard.** `ET.parse()`
+  call sites that read XML from GP-conversion output / uploaded imports had
+  no defense against a crafted `<!ENTITY ...>` chain that could exhaust
+  memory/CPU on the request thread parsing it (classic XXE itself isn't
+  reachable — stdlib `expat` doesn't resolve external entities by default).
+  Added `_reject_entity_declarations`/`_safe_parse_xml_file` (same pattern
+  as `feedBack-plugin-musicxml-import`'s guard) and applied it at all three
+  `ET.parse()` call sites that read files from disk, and directly on the
+  raw upload in `parse-goplayalong-sync` — the most directly user-controlled
+  XML surface, since `goplayalong.py`'s `defusedxml` preference silently
+  falls back to the unguarded stdlib parser when `defusedxml` isn't
+  installed (it isn't, here). `_safe_parse_xml_file` parses the exact bytes
+  it validated (via `io.BytesIO`) rather than reopening the path, closing a
+  TOCTOU gap where a file swapped in between the two steps could have
+  bypassed the guard.
 - **Restricted `youtube-audio` to YouTube hostnames.** The route handed a
   caller-supplied URL straight to `yt_dlp`, which falls back to its generic
   extractor for anything it doesn't recognize — that extractor can fetch

plugin.json

@@ -1,7 +1,7 @@
 {
     "id": "editor",
     "name": "Arrangement Editor",
-    "version": "1.8.3",
+    "version": "1.8.4",
     "description": "Build and edit feedpak arrangements — import Guitar Pro tabs, sync audio, and tune every note.",
     "category": "creation",
     "icon": "assets/thumb.png",

routes.py

@@ -1,6 +1,7 @@
 """Arrangement Editor plugin — backend routes."""
 
 import asyncio
+import io
 import json
 import logging
 import math
@@ -17,6 +18,62 @@
 from xml.dom import minidom
 
 
+# ── XML entity-expansion ("billion laughs") DoS guard ───────────────────
+#
+# stdlib expat doesn't resolve external entities by default, so classic XXE
+# isn't reachable here, but a crafted <!ENTITY ...> chain (billion-laughs /
+# quadratic blowup) can still exhaust memory/CPU on the request thread that
+# parses it. Every ET.parse() call site in this file reads XML from files
+# this plugin didn't author itself (GP-conversion output, an uploaded
+# loose-folder import, or similar) — same threat and same fix as
+# feedBack-plugin-musicxml-import's _reject_entity_declarations guard.
+#
+# Real GP/RS arrangement XML never declares a custom entity, so rejecting
+# any file containing "<!ENTITY " blocks the attack while leaving every
+# legitimate file untouched. expat autodetects encoding (BOM / XML
+# declaration) and accepts UTF-16/UTF-32, not just UTF-8/ASCII, where a raw
+# ASCII byte scan would miss the interleaved-null-byte form of the same
+# token — so the guard also re-decodes under each such encoding and checks
+# the decoded text.
+_ENTITY_DECL_RE = re.compile(rb'<!ENTITY\s', re.IGNORECASE)
+_ENTITY_DECL_TEXT_RE = re.compile(r'<!ENTITY\s', re.IGNORECASE)
+_ENTITY_GUARD_ENCODINGS = (
+    'utf-16', 'utf-16-le', 'utf-16-be',
+    'utf-32', 'utf-32-le', 'utf-32-be',
+)
+
+
+def _reject_entity_declarations(xml_bytes):
+    """Raise ValueError if `xml_bytes` declares a custom XML entity."""
+    if _ENTITY_DECL_RE.search(xml_bytes):
+        raise ValueError(
+            'XML file declares a custom entity, which is not permitted '
+            '(entity-expansion / "billion laughs" protection).'
+        )
+    for encoding in _ENTITY_GUARD_ENCODINGS:
+        try:
+            text = xml_bytes.decode(encoding)
+        except (UnicodeDecodeError, LookupError):
+            continue
+        if _ENTITY_DECL_TEXT_RE.search(text):
+            raise ValueError(
+                'XML file declares a custom entity, which is not permitted '
+                '(entity-expansion / "billion laughs" protection).'
+            )
+
+
+def _safe_parse_xml_file(path):
+    """`ET.parse(path)` with the entity-expansion guard applied first.
+
+    Parses the SAME bytes the guard checked (via io.BytesIO) rather than
+    reopening `path` — reopening would let a file swapped in between the
+    read_bytes() check and the reparse bypass the guard entirely (TOCTOU).
+    """
+    xml_bytes = Path(path).read_bytes()
+    _reject_entity_declarations(xml_bytes)
+    return ET.parse(io.BytesIO(xml_bytes))  # noqa: S314 — entity declarations already rejected above
+
+
 def _filename_bpm(text):
     """Opportunistic tempo prior from an audio file name (TEMPO-ASSIST B).
 
@@ -99,8 +156,8 @@ def _pick_timeline_xml_root(xml_paths):
     first_root = None
     for p in xml_paths or []:
         try:
-            root = XET.parse(p).getroot()
-        except (XET.ParseError, OSError):
+            root = _safe_parse_xml_file(p).getroot()
+        except (XET.ParseError, OSError, ValueError):
             continue
         if first_root is None:
             first_root = root
@@ -2332,7 +2389,7 @@ def _arrangement_xml_candidates(tmp_dir):
     candidates = []
     for xf in Path(tmp_dir).rglob("*.xml"):
         try:
-            root = ET.parse(xf).getroot()
+            root = _safe_parse_xml_file(xf).getroot()
         except Exception:
             continue
         if root.tag != "song":
@@ -4061,6 +4118,10 @@ def _year_text():
             attrs["arpeggio"] = "1"
         ET.SubElement(hs_el, "handShape", **attrs)
 
+    # No entity-expansion guard needed here: xml_str is this function's own
+    # ET.tostring() output, not file/user input, and the serializer escapes
+    # `<` in every text/attribute value, so an "<!ENTITY " construct can
+    # never appear literally in it regardless of what any value contains.
     xml_str = ET.tostring(root, encoding="unicode")
     dom = minidom.parseString(xml_str)
     return dom.toprettyxml(indent="  ", encoding=None)
@@ -7406,6 +7467,10 @@ async def parse_goplayalong_sync(file: UploadFile = File(...)):
         ``audio_url`` tell the caller which files the project references.
         """
         raw = await file.read()
+        try:
+            _reject_entity_declarations(raw)
+        except ValueError as e:
+            return JSONResponse({"error": str(e)}, 400)
         gpa = _load_goplayalong()
         if not gpa.is_goplayalong_xml(raw):
             return JSONResponse(
@@ -8153,25 +8218,30 @@ def _load():
             #   * a positive <offset> with <startBeat> 0                → shift = offset
             # Summing them, shift = offset + startBeat, is correct for both
             # without inspecting which kind of file it is.
-            start_beat = 0.0
-            for _xf in sorted(Path(tmp).glob("*.xml")):
-                try:
-                    _root = ET.parse(_xf).getroot()
-                except Exception:
-                    continue
-                if _root.tag != "song":
-                    continue
-                _arr = _root.find("arrangement")
-                if _arr is None or not _arr.text or _arr.text.strip().lower() in (
-                        "vocals", "showlights", "jvocals"):
-                    continue
-                _sb = _root.find("startBeat")
-                if _sb is not None and _sb.text:
+            def _find_start_beat():
+                # Blocking file I/O + XML parsing — run off the event loop,
+                # same as the _load() call above.
+                for _xf in sorted(Path(tmp).glob("*.xml")):
                     try:
-                        start_beat = float(_sb.text)
-                    except ValueError:
-                        start_beat = 0.0
-                break
+                        _root = _safe_parse_xml_file(_xf).getroot()
+                    except Exception:
+                        continue
+                    if _root.tag != "song":
+                        continue
+                    _arr = _root.find("arrangement")
+                    if _arr is None or not _arr.text or _arr.text.strip().lower() in (
+                            "vocals", "showlights", "jvocals"):
+                        continue
+                    _sb = _root.find("startBeat")
+                    if _sb is not None and _sb.text:
+                        try:
+                            return float(_sb.text)
+                        except ValueError:
+                            return 0.0
+                    return 0.0
+                return 0.0
+
+            start_beat = await asyncio.get_event_loop().run_in_executor(None, _find_start_beat)
             shift = song.offset + start_beat
             if shift:
                 _apply_chart_offset(song, shift)

tests/test_entity_guard.py

@@ -0,0 +1,115 @@
+"""Regression tests for the XML entity-expansion ("billion laughs") guard
+(issue #15).
+
+routes._reject_entity_declarations() must block <!ENTITY declarations
+regardless of the byte encoding used to smuggle them past a naive ASCII scan
+(UTF-16 in particular, since expat/ET.parse autodetects and parses it
+transparently) while never rejecting a legitimate arrangement XML file.
+"""
+
+import io
+from unittest.mock import patch
+
+import pytest
+
+import routes
+from routes import _ENTITY_DECL_RE, _reject_entity_declarations, _safe_parse_xml_file
+
+
+_ENTITY_BOMB = '''<?xml version="1.0"?>
+<!DOCTYPE song [
+  <!ENTITY a "1234567890">
+  <!ENTITY b "&a;&a;&a;&a;&a;&a;&a;&a;&a;&a;">
+]>
+<song>&b;</song>
+'''
+
+_LEGIT_DOC = '''<?xml version="1.0"?>
+<song><title>Test Song</title><arrangement>Lead</arrangement></song>
+'''
+
+
+@pytest.mark.parametrize(
+    'encoding', ['utf-8', 'utf-16', 'utf-16-le', 'utf-16-be', 'utf-32', 'utf-32-le', 'utf-32-be']
+)
+def test_entity_declaration_rejected_across_encodings(encoding):
+    xml_bytes = _ENTITY_BOMB.encode(encoding)
+    with pytest.raises(ValueError, match='entity'):
+        _reject_entity_declarations(xml_bytes)
+
+
+def test_utf16_entity_bypasses_raw_ascii_byte_scan_but_is_still_caught():
+    """The raw ASCII byte regex alone must NOT catch this (regression guard
+    for the bypass itself), while the full guard function must."""
+    xml_bytes = _ENTITY_BOMB.encode('utf-16')
+    assert _ENTITY_DECL_RE.search(xml_bytes) is None
+    with pytest.raises(ValueError):
+        _reject_entity_declarations(xml_bytes)
+
+
+@pytest.mark.parametrize(
+    'encoding', ['utf-8', 'utf-16', 'utf-16-le', 'utf-16-be', 'utf-32', 'utf-32-le', 'utf-32-be']
+)
+def test_legitimate_document_not_rejected_across_encodings(encoding):
+    xml_bytes = _LEGIT_DOC.encode(encoding)
+    _reject_entity_declarations(xml_bytes)  # must not raise
+
+
+def test_safe_parse_xml_file_rejects_entity_bomb_before_expat_parses_it(tmp_path):
+    bomb_path = tmp_path / "bomb.xml"
+    bomb_path.write_bytes(_ENTITY_BOMB.encode('utf-16'))
+    with pytest.raises(ValueError, match='entity'):
+        _safe_parse_xml_file(bomb_path)
+
+
+def test_safe_parse_xml_file_parses_a_legitimate_file(tmp_path):
+    good_path = tmp_path / "song.xml"
+    good_path.write_bytes(_LEGIT_DOC.encode('utf-8'))
+    root = _safe_parse_xml_file(good_path).getroot()
+    assert root.tag == "song"
+    assert root.find("arrangement").text == "Lead"
+
+
+def test_safe_parse_xml_file_parses_the_validated_bytes_not_a_reopened_path(tmp_path):
+    # TOCTOU regression guard: parsing must use the exact bytes already
+    # read and validated (via io.BytesIO), never reopen `path` a second
+    # time -- reopening would let a file swapped in between the two steps
+    # bypass the guard entirely.
+    good_path = tmp_path / "song.xml"
+    good_path.write_bytes(_LEGIT_DOC.encode('utf-8'))
+    real_parse = routes.ET.parse
+    calls = []
+
+    def _spy_parse(source, *args, **kwargs):
+        calls.append(source)
+        return real_parse(source, *args, **kwargs)
+
+    with patch.object(routes.ET, "parse", side_effect=_spy_parse):
+        _safe_parse_xml_file(good_path)
+
+    assert len(calls) == 1
+    assert isinstance(calls[0], io.BytesIO), (
+        "ET.parse must be called with the already-validated bytes, not the path"
+    )
+
+
+# The GoPlayAlong sync-upload route (parse-goplayalong-sync) hands raw
+# user-uploaded bytes straight to goplayalong.py's ET.fromstring() calls,
+# whose defusedxml preference silently falls back to the vulnerable stdlib
+# parser when defusedxml isn't installed (it isn't, here) -- the most
+# directly user-controlled XML surface in this plugin. The route now calls
+# _reject_entity_declarations(raw) before either goplayalong call; this
+# documents that a <track>-shaped entity bomb is caught by the exact same
+# guard function that route wires in.
+_GOPLAYALONG_ENTITY_BOMB = '''<?xml version="1.0"?>
+<!DOCTYPE track [
+  <!ENTITY a "1234567890">
+  <!ENTITY b "&a;&a;&a;&a;&a;&a;&a;&a;&a;&a;">
+]>
+<track id="1" title="&b;"></track>
+'''
+
+
+def test_goplayalong_shaped_entity_bomb_is_rejected():
+    with pytest.raises(ValueError, match='entity'):
+        _reject_entity_declarations(_GOPLAYALONG_ENTITY_BOMB.encode('utf-8'))