From da300ca450002a9d098f5d6c55414ec46f5ed442 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 12:31:14 +0000 Subject: [PATCH 1/2] fix(security): guard XML parsing against entity-expansion DoS stdlib expat doesn't resolve external entities by default, so classic XXE isn't reachable, but a crafted chain (billion laughs / quadratic blowup) could still exhaust memory/CPU on the request thread parsing it. Add _reject_entity_declarations/_safe_parse_xml_file (same pattern as feedBack-plugin-musicxml-import's guard, including the UTF-16/UTF-32 encoding check a raw ASCII byte scan would miss) and route all three real ET.parse() call sites that read files from disk through it. The minidom.parseString() site is left unguarded with a comment explaining why: it re-parses this plugin's own ET.tostring() output, which escapes "<" in every value, so an entity declaration can never appear in it regardless of input content. Closes #15. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_013Kh8gR75zYwFUT3crRoEbH --- CHANGELOG.md | 8 +++++ plugin.json | 2 +- routes.py | 63 +++++++++++++++++++++++++++++++++--- tests/test_entity_guard.py | 66 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 134 insertions(+), 5 deletions(-) create mode 100644 tests/test_entity_guard.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 81bef4b..b586cf8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,14 @@ 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 `` 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. - **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 diff --git a/plugin.json b/plugin.json index 9bae9dc..f85b808 100644 --- a/plugin.json +++ b/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", diff --git a/routes.py b/routes.py index 17f64a9..a304c07 100644 --- a/routes.py +++ b/routes.py @@ -17,6 +17,57 @@ 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 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 " + + +]> +&b; +''' + +_LEGIT_DOC = ''' +Test SongLead +''' + + +@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" From 00bdf55150eece4fb4158440f517256d13a9ae30 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 12:46:58 +0000 Subject: [PATCH 2/2] fix(security): address CodeRabbit/pullfrog review findings on PR #17 - Guard the raw upload in parse-goplayalong-sync before it reaches goplayalong.py's ET.fromstring() calls -- the most directly user-controlled XML surface in this plugin. goplayalong.py prefers defusedxml but silently falls back to the unguarded stdlib parser when defusedxml isn't installed (it isn't, here), so this path was actually unprotected. Found by pullfrog. - _safe_parse_xml_file now parses the exact bytes it validated (via io.BytesIO) instead of reopening the path, closing a TOCTOU gap where a file swapped in between the read and the reparse could bypass the guard. Found by CodeRabbit. - Move the startBeat XML scan in import-xml-project off the event loop (run_in_executor) -- it's blocking file I/O + XML parsing inside an async handler. Found by CodeRabbit. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_013Kh8gR75zYwFUT3crRoEbH --- CHANGELOG.md | 9 ++++++- routes.py | 55 ++++++++++++++++++++++++-------------- tests/test_entity_guard.py | 49 +++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b586cf8..3886fd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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. + `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 diff --git a/routes.py b/routes.py index a304c07..c1faf58 100644 --- a/routes.py +++ b/routes.py @@ -1,6 +1,7 @@ """Arrangement Editor plugin — backend routes.""" import asyncio +import io import json import logging import math @@ -62,10 +63,15 @@ def _reject_entity_declarations(xml_bytes): def _safe_parse_xml_file(path): - """`ET.parse(path)` with the entity-expansion guard applied first.""" + """`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(path) + return ET.parse(io.BytesIO(xml_bytes)) # noqa: S314 — entity declarations already rejected above def _filename_bpm(text): @@ -7461,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( @@ -8208,25 +8218,30 @@ def _load(): # * a positive with 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 = _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: + 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) diff --git a/tests/test_entity_guard.py b/tests/test_entity_guard.py index 271a9b2..6280cb3 100644 --- a/tests/test_entity_guard.py +++ b/tests/test_entity_guard.py @@ -7,8 +7,12 @@ 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 @@ -64,3 +68,48 @@ def test_safe_parse_xml_file_parses_a_legitimate_file(tmp_path): 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 -shaped entity bomb is caught by the exact same +# guard function that route wires in. +_GOPLAYALONG_ENTITY_BOMB = ''' + + +]> + +''' + + +def test_goplayalong_shaped_entity_bomb_is_rejected(): + with pytest.raises(ValueError, match='entity'): + _reject_entity_declarations(_GOPLAYALONG_ENTITY_BOMB.encode('utf-8'))