From 194985b5e16ad167f3e3f349b8d4d53d3609609b Mon Sep 17 00:00:00 2001 From: Vykos Date: Fri, 5 Jun 2026 10:29:11 +0200 Subject: [PATCH] Constrain gallery filenames to image root (#2828) --- routes/gallery_routes.py | 42 +++++++++++---- tests/test_gallery_filename_confinement.py | 63 ++++++++++++++++++++++ 2 files changed, 94 insertions(+), 11 deletions(-) create mode 100644 tests/test_gallery_filename_confinement.py diff --git a/routes/gallery_routes.py b/routes/gallery_routes.py index fdac5a4..d8d52b2 100644 --- a/routes/gallery_routes.py +++ b/routes/gallery_routes.py @@ -27,11 +27,32 @@ GALLERY_TRANSFORM_UPLOAD_MAX_BYTES = int(os.getenv("ODYSSEUS_GALLERY_TRANSFORM_U def _sanitize_gallery_filename(filename: str) -> str: """Return a local filename safe to join under generated_images.""" - safe_name = re.sub(r"[^A-Za-z0-9._-]", "_", Path(filename or "").name)[:128] + safe_name = re.sub(r"[^A-Za-z0-9._-]", "_", Path(str(filename or "")).name)[:128] if not safe_name or safe_name in {".", ".."}: safe_name = uuid.uuid4().hex[:12] return safe_name + +GALLERY_IMAGE_DIR = Path("data/generated_images") + + +def _gallery_image_path(filename: str) -> Path: + """Resolve a stored gallery filename without leaving generated_images.""" + if not isinstance(filename, str): + raise HTTPException(400, "Unsafe gallery filename") + safe_name = _sanitize_gallery_filename(filename) + original = str(filename or "") + root = GALLERY_IMAGE_DIR.resolve() + path = (GALLERY_IMAGE_DIR / safe_name).resolve() + try: + if os.path.commonpath([str(root), str(path)]) != str(root): + raise ValueError + except Exception: + raise HTTPException(400, "Unsafe gallery filename") + if safe_name != original: + raise HTTPException(400, "Unsafe gallery filename") + return path + def setup_gallery_routes() -> APIRouter: router = APIRouter(tags=["gallery"]) @@ -211,7 +232,7 @@ def setup_gallery_routes() -> APIRouter: if not user or img.owner != user: raise HTTPException(403, "Not your image") - img_path = Path("data/generated_images") / img.filename + img_path = _gallery_image_path(img.filename) if not img_path.exists(): raise HTTPException(404, "Image file not found") @@ -692,11 +713,11 @@ def setup_gallery_routes() -> APIRouter: used = set() with zipfile.ZipFile(buf, "w", zipfile.ZIP_DEFLATED) as zf: for img in imgs: - src = os.path.join("data", "generated_images", img.filename) - if not os.path.exists(src): + src = _gallery_image_path(img.filename) + if not src.exists(): continue - ext = os.path.splitext(img.filename)[1] or ".png" - base = (img.prompt or "").strip() or os.path.splitext(img.filename)[0] + ext = src.suffix or ".png" + base = (img.prompt or "").strip() or src.stem base = re.sub(r"[^\w\-. ]+", "", base)[:60].strip() or img.id name = f"{base}{ext}" i = 1 @@ -818,9 +839,9 @@ def setup_gallery_routes() -> APIRouter: img_filename = img.filename # Remove the file from disk - img_path = os.path.join("data", "generated_images", img_filename) - if os.path.exists(img_path): - os.remove(img_path) + img_path = _gallery_image_path(img_filename) + if img_path.exists(): + img_path.unlink() # Soft-delete the record img.is_active = False @@ -1708,7 +1729,7 @@ def setup_gallery_routes() -> APIRouter: try: img = _get_or_404_image(db, image_id, user) - img_path = Path("data/generated_images") / img.filename + img_path = _gallery_image_path(img.filename) if not img_path.exists(): raise HTTPException(404, "Image file not found") @@ -1807,4 +1828,3 @@ def setup_gallery_routes() -> APIRouter: db.close() return router - diff --git a/tests/test_gallery_filename_confinement.py b/tests/test_gallery_filename_confinement.py new file mode 100644 index 0000000..5e6c3f0 --- /dev/null +++ b/tests/test_gallery_filename_confinement.py @@ -0,0 +1,63 @@ +import os +from pathlib import Path + +import pytest +from fastapi import HTTPException + + +def _gallery_module(): + import routes.gallery_routes as gallery_routes + return gallery_routes + + +def test_gallery_image_path_allows_safe_filename(tmp_path, monkeypatch): + gallery_routes = _gallery_module() + image_dir = tmp_path / "generated_images" + image_dir.mkdir() + monkeypatch.setattr(gallery_routes, "GALLERY_IMAGE_DIR", image_dir) + + path = gallery_routes._gallery_image_path("abc123.png") + + assert path == image_dir / "abc123.png" + + +@pytest.mark.parametrize("filename", ["../../secret.png", "..\\secret.png", None, 12345]) +def test_gallery_image_path_rejects_unsafe_stored_filenames(tmp_path, monkeypatch, filename): + gallery_routes = _gallery_module() + image_dir = tmp_path / "generated_images" + image_dir.mkdir() + monkeypatch.setattr(gallery_routes, "GALLERY_IMAGE_DIR", image_dir) + + with pytest.raises(HTTPException) as exc: + gallery_routes._gallery_image_path(filename) + + assert exc.value.status_code == 400 + + +def test_gallery_image_path_rejects_symlink_escape(tmp_path, monkeypatch): + gallery_routes = _gallery_module() + image_dir = tmp_path / "generated_images" + image_dir.mkdir() + outside = tmp_path / "outside.png" + outside.write_bytes(b"outside image root") + link = image_dir / "escape.png" + try: + os.symlink(outside, link) + except (AttributeError, NotImplementedError, OSError) as exc: + pytest.skip(f"symlinks unavailable: {exc}") + monkeypatch.setattr(gallery_routes, "GALLERY_IMAGE_DIR", image_dir) + + with pytest.raises(HTTPException) as exc: + gallery_routes._gallery_image_path("escape.png") + + assert exc.value.status_code == 400 + + +def test_gallery_file_operations_use_confining_resolver(): + source = Path("routes/gallery_routes.py").read_text(encoding="utf-8") + + assert 'Path("data/generated_images") / img.filename' not in source + assert 'os.path.join("data", "generated_images", img.filename)' not in source + assert 'os.path.join("data", "generated_images", img_filename)' not in source + assert source.count("_gallery_image_path(img.filename)") >= 3 + assert "_gallery_image_path(img_filename)" in source