From 6f65d6e20aea5adc3ddaeba0b7fc4c76699fc42b Mon Sep 17 00:00:00 2001 From: splitsec2 <35583321+splitsec2@users.noreply.github.com> Date: Tue, 20 Oct 2026 21:40:16 -0600 Subject: [PATCH] Merge commit from fork A release format such as /../../x was appended to the staging file name unchanged, so the direct-download and libgen handlers could write to, and on cancel unlink, a path outside TMP_DIR. Normalise the format to a plain extension at queue time and in build_filename, reject path-like source ids for the sources that use them as file names, and make both handlers resolve the staging path and refuse anything that is not a direct child of TMP_DIR. --- shelfmark/core/models.py | 33 ++++++++- shelfmark/download/orchestrator.py | 22 +++++- .../direct_download/handler.py | 11 ++- shelfmark/release_sources/libgen/handler.py | 6 +- tests/core/test_filename_traversal.py | 68 +++++++++++++++++++ tests/direct_download/test_handler.py | 63 +++++++++++++++++ .../download/test_orchestrator_pack_fields.py | 54 +++++++++++++++ tests/libgen/test_handler.py | 38 +++++++++++ 8 files changed, 284 insertions(+), 11 deletions(-) create mode 100644 tests/core/test_filename_traversal.py diff --git a/shelfmark/core/models.py b/shelfmark/core/models.py index 91657197..ee38d7c4 100644 --- a/shelfmark/core/models.py +++ b/shelfmark/core/models.py @@ -7,6 +7,34 @@ from enum import StrEnum from pathlib import Path from typing import Any +_FORMAT_RE = re.compile(r"[a-z0-9]{1,10}") + + +def normalize_format(fmt: object) -> str | None: + """Return ``fmt`` as a lower-case plain file extension, or None if it is not one. + + Accepts only 1 to 10 ASCII letters or digits (epub, mobi, m4b, azw3, cbz...). Anything + else, including separators, dots and control characters, is rejected rather than + cleaned, because the value ends up in a staging file name. + """ + if not isinstance(fmt, str): + return None + candidate = fmt.strip().lstrip(".").lower() + return candidate if _FORMAT_RE.fullmatch(candidate) else None + + +def resolve_staged_path(base_dir: Path, name: str) -> Path: + """Return ``base_dir / name`` after checking it is a direct child of ``base_dir``. + + Raises ValueError when ``name`` would land anywhere else (separators, ``..``, a + symlink pointing out, or the directory itself). + """ + resolved_base = base_dir.resolve() + path = base_dir / name + if not name or path.resolve().parent != resolved_base: + raise ValueError(f"Refusing staging path outside {base_dir}") + return path + def build_filename( title: str, @@ -26,8 +54,9 @@ def build_filename( filename = "".join(parts) filename = re.sub(r'[\\/:*?"<>|]', "_", filename.strip())[:245] - if fmt: - filename = f"{filename}.{fmt}" + safe_fmt = normalize_format(fmt) + if safe_fmt: + filename = f"{filename}.{safe_fmt}" return filename diff --git a/shelfmark/download/orchestrator.py b/shelfmark/download/orchestrator.py index b29bc5ed..a653a010 100644 --- a/shelfmark/download/orchestrator.py +++ b/shelfmark/download/orchestrator.py @@ -17,7 +17,7 @@ from typing import TYPE_CHECKING, Any from shelfmark.core.config import config from shelfmark.core.logger import setup_logger -from shelfmark.core.models import DownloadTask, QueueStatus, SearchMode +from shelfmark.core.models import DownloadTask, QueueStatus, SearchMode, normalize_format from shelfmark.core.queue import book_queue from shelfmark.core.request_helpers import ( normalize_optional_text, @@ -238,6 +238,18 @@ def _build_retry_resolution_fields( } +# Sources whose source_id becomes a staging file name (the handlers use it as one when +# FILE_ORGANIZATION is "none"), so it must not carry path components. +_FILENAME_ID_SOURCES = frozenset({"direct_download", "libgen"}) + + +def _source_id_is_unsafe_filename(source: str, source_id: object) -> bool: + if source not in _FILENAME_ID_SOURCES: + return False + text = str(source_id) + return any(ch in text for ch in "/\\\0") or ".." in text + + def queue_release( release_data: dict, priority: int = 0, @@ -252,6 +264,10 @@ def queue_release( logger.warning("Rejected queue request for unavailable source %s", source) return False, unavailable_message + if _source_id_is_unsafe_filename(source, release_data["source_id"]): + logger.warning("Rejected queue request with unsafe source_id for %s", source) + return False, "Invalid release identifier" + extra = release_data.get("extra", {}) raw_request_id = release_data.get("_request_id") request_id: int | None = None @@ -316,7 +332,7 @@ def queue_release( title=release_data.get("title", "Unknown"), author=author, year=year, - format=release_data.get("format"), + format=normalize_format(release_data.get("format")), size=release_data.get("size"), downloads=release_data.get("downloads") or extra.get("downloads"), preview=preview, @@ -613,7 +629,7 @@ def _restore_task_from_retry_payload(payload: object) -> DownloadTask | None: title=title, author=normalize_optional_text(payload.get("author")), year=normalize_optional_text(payload.get("year")), - format=normalize_optional_text(payload.get("format")), + format=normalize_format(payload.get("format")), size=normalize_optional_text(payload.get("size")), downloads=int(payload["downloads"]) if payload.get("downloads") is not None else None, preview=normalize_optional_text(payload.get("preview")), diff --git a/shelfmark/release_sources/direct_download/handler.py b/shelfmark/release_sources/direct_download/handler.py index 59d69834..b5b2d6ce 100644 --- a/shelfmark/release_sources/direct_download/handler.py +++ b/shelfmark/release_sources/direct_download/handler.py @@ -6,7 +6,12 @@ from typing import TYPE_CHECKING from shelfmark.config.env import TMP_DIR from shelfmark.core.config import config from shelfmark.core.logger import setup_logger -from shelfmark.core.models import DownloadTask, build_filename +from shelfmark.core.models import ( + DownloadTask, + build_filename, + normalize_format, + resolve_staged_path, +) from shelfmark.download import network from shelfmark.release_sources import ( BrowseRecord, @@ -114,7 +119,7 @@ class DirectDownloadHandler(DownloadHandler): # based on FILE_ORGANIZATION setting file_org = config.get("FILE_ORGANIZATION", "rename") if file_org == "none": - book_name = f"{book_info.id}.{book_info.format or 'bin'}" + book_name = f"{book_info.id}.{normalize_format(book_info.format) or 'bin'}" else: book_name = build_filename( book_info.title, @@ -122,7 +127,7 @@ class DirectDownloadHandler(DownloadHandler): book_info.year, book_info.format, ) - book_path = TMP_DIR / book_name + book_path = resolve_staged_path(TMP_DIR, book_name) # Check cancellation before download if cancel_flag.is_set(): diff --git a/shelfmark/release_sources/libgen/handler.py b/shelfmark/release_sources/libgen/handler.py index 4ca79825..bc18464a 100644 --- a/shelfmark/release_sources/libgen/handler.py +++ b/shelfmark/release_sources/libgen/handler.py @@ -11,7 +11,7 @@ from typing import TYPE_CHECKING from shelfmark.config.env import TMP_DIR from shelfmark.core.config import config from shelfmark.core.logger import setup_logger -from shelfmark.core.models import build_filename +from shelfmark.core.models import build_filename, normalize_format, resolve_staged_path from shelfmark.download import http as downloader from shelfmark.release_sources import DownloadHandler, register_handler from shelfmark.release_sources.libgen import scraper @@ -57,10 +57,10 @@ class LibgenHandler(DownloadHandler): md5 = task.task_id.split(":", 1)[-1].lower() if config.get("FILE_ORGANIZATION", "rename") == "none": - book_name = f"{md5}.{task.format or 'bin'}" + book_name = f"{md5}.{normalize_format(task.format) or 'bin'}" else: book_name = build_filename(task.title, task.author, task.year, task.format) - book_path = TMP_DIR / book_name + book_path = resolve_staged_path(TMP_DIR, book_name) for base in mirrors.get_libgen_mirrors(): if cancel_flag.is_set(): diff --git a/tests/core/test_filename_traversal.py b/tests/core/test_filename_traversal.py new file mode 100644 index 00000000..22f9afea --- /dev/null +++ b/tests/core/test_filename_traversal.py @@ -0,0 +1,68 @@ +import pytest + +from shelfmark.core.models import build_filename, normalize_format, resolve_staged_path + + +@pytest.mark.parametrize( + ("raw", "expected"), + [ + ("epub", "epub"), + ("EPUB", "epub"), + (".mobi", "mobi"), + (" pdf ", "pdf"), + ("azw3", "azw3"), + ("m4b", "m4b"), + ("mp3", "mp3"), + ("cbz", "cbz"), + ("fb2", "fb2"), + ("/../../../config/x", None), + ("../x", None), + ("a/b", None), + ("a\\b", None), + ("ep.ub", None), + ("epub\x00", None), + ("epub\n", "epub"), + ("", None), + (None, None), + (5, None), + ("abcdefghijk", None), + ], +) +def test_normalize_format(raw, expected): + assert normalize_format(raw) == expected + + +@pytest.mark.parametrize("fmt", ["/../../../config/x", "../x", "a/b", "a\\b", "x\x00y"]) +def test_build_filename_never_makes_path_component_from_format(fmt): + name = build_filename(".", None, None, fmt) + assert "/" not in name + assert "\\" not in name + assert "\x00" not in name + assert name == "." + + +def test_build_filename_keeps_legitimate_format(): + assert ( + build_filename("Dune", "Frank Herbert", "1965", "EPUB") + == "Frank Herbert - Dune (1965).epub" + ) + + +def test_resolve_staged_path_accepts_plain_child(tmp_path): + assert resolve_staged_path(tmp_path, "book.epub") == tmp_path / "book.epub" + + +@pytest.mark.parametrize("name", ["", ".", "..", "../x", "a/b", "/etc/passwd"]) +def test_resolve_staged_path_rejects_escapes(tmp_path, name): + with pytest.raises(ValueError, match="outside"): + resolve_staged_path(tmp_path, name) + + +def test_resolve_staged_path_rejects_symlink_out(tmp_path): + outside = tmp_path / "outside" + outside.mkdir() + base = tmp_path / "base" + base.mkdir() + (base / "link").symlink_to(outside / "file") + with pytest.raises(ValueError, match="outside"): + resolve_staged_path(base, "link") diff --git a/tests/direct_download/test_handler.py b/tests/direct_download/test_handler.py index 8b832e87..d3ca43bf 100644 --- a/tests/direct_download/test_handler.py +++ b/tests/direct_download/test_handler.py @@ -147,3 +147,66 @@ def test_direct_download_handler_removes_partial_file_when_cancelled_after_downl assert result is None assert not expected_path.exists() assert status_updates[-1] == ("cancelled", "Cancelled") + + +def _hostile_run(monkeypatch, tmp_path, *, file_org, task, cancel_after=False): + from shelfmark.release_sources.direct_download import handler as dd + + tmp_dir = tmp_path / "tmp" + tmp_dir.mkdir() + seen: list = [] + + def fake_download_book(book_info, book_path, progress_callback, cancel_flag, status_callback): + seen.append(book_path) + if cancel_after: + cancel_flag.set() + return None + + monkeypatch.setattr(dd, "_download_book", fake_download_book) + monkeypatch.setattr(dd, "TMP_DIR", tmp_dir) + monkeypatch.setattr( + config, + "get", + lambda key, default=None: file_org if key == "FILE_ORGANIZATION" else default, + ) + cancel_flag = Event() + result = DirectDownloadHandler().download( + task, cancel_flag, lambda _p: None, lambda _s, _m: None + ) + return tmp_dir, seen, result + + +def test_direct_download_handler_never_stages_outside_tmp_dir_via_format(monkeypatch, tmp_path): + (tmp_path / "config").mkdir() + victim = tmp_path / "config" / "x" + victim.write_text("keep") + task = DownloadTask( + task_id="id", + source="direct_download", + title=".", + format="/../config/x", + ) + tmp_dir, seen, _ = _hostile_run( + monkeypatch, tmp_path, file_org="rename", task=task, cancel_after=True + ) + for path in seen: + assert path.resolve().parent == tmp_dir.resolve() + assert victim.read_text() == "keep" + + +def test_direct_download_handler_refuses_hostile_source_id(monkeypatch, tmp_path): + (tmp_path / "config").mkdir() + victim = tmp_path / "config" / "x.bin" + victim.write_text("keep") + task = DownloadTask( + task_id="../config/x", + source="direct_download", + title="t", + format="bin", + ) + _tmp_dir, seen, result = _hostile_run( + monkeypatch, tmp_path, file_org="none", task=task, cancel_after=True + ) + assert result is None + assert seen == [] + assert victim.read_text() == "keep" diff --git a/tests/download/test_orchestrator_pack_fields.py b/tests/download/test_orchestrator_pack_fields.py index 61e19f9e..460ebdd2 100644 --- a/tests/download/test_orchestrator_pack_fields.py +++ b/tests/download/test_orchestrator_pack_fields.py @@ -1,5 +1,7 @@ """Pack fields (multi_book / book_plan) survive queueing and restart-safe retry.""" +import pytest + from shelfmark.core.models import DownloadTask from shelfmark.download import orchestrator @@ -60,3 +62,55 @@ def test_queue_release_reads_pack_fields(monkeypatch): assert ok, error assert captured["task"].multi_book is True assert captured["task"].book_plan == PLAN + + +def _queue(monkeypatch, release): + captured: dict[str, DownloadTask] = {} + + def fake_add(task: DownloadTask) -> bool: + captured["task"] = task + return True + + monkeypatch.setattr(orchestrator.config, "get", lambda _key, default=None, **_kw: default) + monkeypatch.setattr(orchestrator, "_source_unavailable_message", lambda _source: None) + monkeypatch.setattr(orchestrator.book_queue, "add", fake_add) + monkeypatch.setattr(orchestrator, "ws_manager", None) + ok, error = orchestrator.queue_release(release, 0) + return ok, error, captured.get("task") + + +def test_queue_release_drops_hostile_format(monkeypatch): + ok, error, task = _queue( + monkeypatch, + { + "source": "direct_download", + "source_id": "abc", + "title": ".", + "format": "/../../../config/x", + }, + ) + assert ok, error + assert task is not None + assert task.format is None + + +def test_queue_release_normalises_format(monkeypatch): + ok, _error, task = _queue( + monkeypatch, + {"source": "direct_download", "source_id": "abc", "title": "t", "format": "EPUB"}, + ) + assert ok + assert task is not None + assert task.format == "epub" + + +@pytest.mark.parametrize("source", ["direct_download", "libgen"]) +@pytest.mark.parametrize("source_id", ["../../x", "a/b", "a\\b", "x..y", "libgen:../../x"]) +def test_queue_release_rejects_path_source_id(monkeypatch, source, source_id): + ok, error, task = _queue( + monkeypatch, + {"source": source, "source_id": source_id, "title": "t", "format": "epub"}, + ) + assert not ok + assert error + assert task is None diff --git a/tests/libgen/test_handler.py b/tests/libgen/test_handler.py index 31aff762..cb4e2968 100644 --- a/tests/libgen/test_handler.py +++ b/tests/libgen/test_handler.py @@ -2,6 +2,7 @@ import io import threading +from pathlib import Path from unittest.mock import MagicMock, patch from shelfmark.core.models import DownloadTask @@ -129,3 +130,40 @@ def test_download_cancelled_before_start(tmp_path): ) assert result is None status.assert_any_call("cancelled", "Cancelled") + + +def test_hostile_format_cannot_escape_tmp_dir(tmp_path): + tmp_dir = tmp_path / "tmp" + tmp_dir.mkdir() + (tmp_path / "config").mkdir() + task = DownloadTask( + task_id=f"libgen:{html.MD5_A}", + source="libgen", + title=".", + format="/../config/x", + size="6 MB", + ) + result, _ = _run( + task, + tmp_dir, + mirrors_list=["https://libgen.li"], + fetch_page=lambda url, timeout=(5, 10): html.ADS_HTML, + download_url=lambda *a, **k: _buf(), + ) + assert not (tmp_path / "config" / "x").exists() + assert result is not None + assert Path(result).parent == tmp_dir + + +def test_hostile_md5_cannot_escape_tmp_dir(tmp_path): + tmp_dir = tmp_path / "tmp" + tmp_dir.mkdir() + result, _ = _run( + _task("libgen:../../evil", "epub"), + tmp_dir, + mirrors_list=["https://libgen.li"], + fetch_page=lambda url, timeout=(5, 10): html.ADS_HTML, + download_url=lambda *a, **k: _buf(), + ) + assert result is None + assert not (tmp_path / "evil.epub").exists()