From 49da3022cf71987dc07e84bbd66e9aedcc571d0b Mon Sep 17 00:00:00 2001 From: byrongamatos Date: Wed, 15 Jul 2026 00:31:07 +0200 Subject: [PATCH] fix(career): path-traversal guard on prepare; a cap test that actually tests the cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit again, and the first one is a real hole I put there. 1. PATH TRAVERSAL. sloppak.resolve_source_dir() does a bare `dlc_root / filename` with NO containment guard — so `../../x` walks straight out of the library, and my new endpoint handed it attacker-supplied filenames. Every filename now goes through _resolve_dlc_path first, the same check every other filename-bound handler applies. Pinned: `..`, backslash traversal, an absolute POSIX path and a Windows drive path are all refused, and nothing outside the library is unpacked. 2. THE CAP TEST WAS VACUOUS. It asserted `prepared == 0` against a fixture with no library — where the endpoint exits before extraction — so it passed whether or not MAX_GIG_SONGS existed. It now runs against a real library and asserts the endpoint CONSIDERED at most MAX_GIG_SONGS of the 82 it was handed. Verified to fail when the cap is removed. Same class of mistake as the notedetect gigBlock: a test that passes for the wrong reason. Worth saying out loud since it is twice in one day. 3. E702 — semicolon-joined statements in the new tests, split. 51 career tests; full suite green. --- plugins/career/routes.py | 13 ++++- tests/plugins/career/test_routes.py | 79 ++++++++++++++++++++++------- 2 files changed, 74 insertions(+), 18 deletions(-) diff --git a/plugins/career/routes.py b/plugins/career/routes.py index 35f934b..7f8b423 100644 --- a/plugins/career/routes.py +++ b/plugins/career/routes.py @@ -47,6 +47,7 @@ from fastapi import Body, HTTPException from fastapi.responses import FileResponse import sloppak +from dlc_paths import _resolve_dlc_path from progression import instrument_for_arrangement PLUGIN_ID = "career" @@ -774,10 +775,20 @@ def setup(app, context): if dlc_root is None or cache_root is None: return {"ok": False, "prepared": 0, "failed": files, "error": "no library"} + root = Path(dlc_root) prepared, failed = 0, [] for fn in files: + # CONTAINMENT FIRST. resolve_source_dir() does a bare + # `dlc_root / filename` with no guard, so a crafted `../..` would + # walk straight out of the library. Every other filename-bound + # handler validates through _resolve_dlc_path; so does this one. + safe = _resolve_dlc_path(root, fn) + if safe is None: + _state["log"].warning("career: gig pre-extract rejected unsafe path %r", fn) + failed.append(fn) + continue try: - sloppak.resolve_source_dir(fn, Path(dlc_root), Path(cache_root)) + sloppak.resolve_source_dir(fn, root, Path(cache_root)) prepared += 1 except Exception as exc: # noqa: BLE001 — one bad pak can't sink the set _state["log"].warning("career: gig pre-extract failed for %s: %s", fn, exc) diff --git a/tests/plugins/career/test_routes.py b/tests/plugins/career/test_routes.py index 39c30dc..1830f7a 100644 --- a/tests/plugins/career/test_routes.py +++ b/tests/plugins/career/test_routes.py @@ -208,8 +208,10 @@ def _write_feedpak(dlc, name, title="T"): def test_gig_prepare_extracts_every_song_up_front(tmp_path, meta_db): - dlc = tmp_path / "dlc"; dlc.mkdir() - cache = tmp_path / "cache"; cache.mkdir() + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() for n in ("one.feedpak", "two.feedpak", "three.feedpak"): _write_feedpak(dlc, n) @@ -229,8 +231,10 @@ def test_gig_prepare_extracts_every_song_up_front(tmp_path, meta_db): def test_gig_prepare_is_idempotent_on_a_warm_cache(tmp_path, meta_db): - dlc = tmp_path / "dlc"; dlc.mkdir() - cache = tmp_path / "cache"; cache.mkdir() + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() _write_feedpak(dlc, "one.feedpak") client = _career_client_with_library(tmp_path, meta_db, dlc, cache) @@ -243,8 +247,10 @@ def test_gig_prepare_is_idempotent_on_a_warm_cache(tmp_path, meta_db): def test_one_bad_feedpak_does_not_stop_the_set(tmp_path, meta_db): # A corrupt pak in the setlist must not block the gig: the play itself will # surface the error exactly as it does outside a gig. Slow beats blocked. - dlc = tmp_path / "dlc"; dlc.mkdir() - cache = tmp_path / "cache"; cache.mkdir() + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() _write_feedpak(dlc, "good.feedpak") (dlc / "bad.feedpak").write_bytes(b"not a zip at all") @@ -281,8 +287,10 @@ def test_prepare_rejects_a_non_list_songs_value(tmp_path, meta_db, client): def test_prepare_ignores_non_string_and_blank_entries(tmp_path, meta_db): - dlc = tmp_path / "dlc"; dlc.mkdir() - cache = tmp_path / "cache"; cache.mkdir() + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() _write_feedpak(dlc, "good.feedpak") client = _career_client_with_library(tmp_path, meta_db, dlc, cache) body = client.post("/api/plugins/career/gigs/prepare", @@ -291,14 +299,51 @@ def test_prepare_ignores_non_string_and_blank_entries(tmp_path, meta_db): assert body["failed"] == [] -def test_prepare_caps_the_setlist(tmp_path, meta_db, client): - # This endpoint unpacks zips — an arbitrary caller must not be able to ask - # for unbounded work. A setlist is a handful of songs. +def test_prepare_caps_the_setlist(tmp_path, meta_db): + # This endpoint unpacks zips — an arbitrary caller must not be able to ask for + # unbounded work. + # + # The first version of this test asserted `prepared == 0` against a fixture + # with NO library: the endpoint exits before extraction there, so it passed + # whether or not the cap existed. Give it a real library, ask for far more than + # the cap, and assert the endpoint only ever considered MAX_GIG_SONGS of them. import routes as career_routes assert career_routes.MAX_GIG_SONGS <= 64 - res = client.post("/api/plugins/career/gigs/prepare", - json={"songs": [f"s{i}.feedpak" for i in range(500)]}) - assert res.status_code == 200 - # No library in this fixture, so nothing prepares — the point is it did not - # try to walk 500 entries. - assert res.json()["prepared"] == 0 + + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + client = _career_client_with_library(tmp_path, meta_db, dlc, cache) + + n = career_routes.MAX_GIG_SONGS + 50 + # None of these exist, so every song the endpoint LOOKS AT lands in `failed`. + # That makes `failed` an exact count of how many it considered. + body = client.post("/api/plugins/career/gigs/prepare", + json={"songs": [f"missing{i}.feedpak" for i in range(n)]}).json() + assert body["prepared"] == 0 + assert len(body["failed"]) == career_routes.MAX_GIG_SONGS, ( + f"the endpoint must consider at most MAX_GIG_SONGS " + f"({career_routes.MAX_GIG_SONGS}), not all {n}" + ) + + +def test_prepare_refuses_to_escape_the_library(tmp_path, meta_db): + # resolve_source_dir() does a bare `dlc_root / filename` with no containment + # guard, so a crafted path would walk straight out of the library. Every + # filename must go through _resolve_dlc_path first. + dlc = tmp_path / "dlc" + dlc.mkdir() + cache = tmp_path / "cache" + cache.mkdir() + (tmp_path / "outside.feedpak").write_bytes(b"secret") + client = _career_client_with_library(tmp_path, meta_db, dlc, cache) + + for evil in ("../outside.feedpak", "..\\outside.feedpak", + "a/../../outside.feedpak", "/etc/passwd", "C:/Windows/x.feedpak"): + body = client.post("/api/plugins/career/gigs/prepare", + json={"songs": [evil]}).json() + assert body["prepared"] == 0, f"{evil!r} must never be prepared" + assert body["failed"] == [evil] + # Nothing outside the library may have been unpacked. + assert list(cache.iterdir()) == []