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

@@ -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
 

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")),

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():

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():

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")

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"

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

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()