diff --git a/src/server/services/anime_service.py b/src/server/services/anime_service.py index 9f76cdc..04d44a5 100644 --- a/src/server/services/anime_service.py +++ b/src/server/services/anime_service.py @@ -1749,9 +1749,22 @@ class AnimeService: # --- Filesystem deletion --- if delete_folder and folder_path: - # Resolve absolute path and validate it is within base directory - abs_folder = _os.path.abspath(folder_path) + # Resolve absolute path and validate it is within base directory. + # + # Important: `folder_path` stored in the database is the relative + # folder name (e.g. "Beyblade Burst (2016)"), not an absolute path. + # If we feed a relative path to os.path.abspath() it gets joined + # against the process's current working directory — which may be + # /app inside the container while the anime directory is /data, + # producing e.g. "/app/Beyblade Burst (2016)" and tripping the + # safe-path check below for what is actually a valid deletion. + # Resolve relative paths against the configured anime directory + # so the safety check operates on the real intended target. base_dir = _os.path.abspath(self._directory) + if _os.path.isabs(folder_path): + abs_folder = _os.path.abspath(folder_path) + else: + abs_folder = _os.path.abspath(_os.path.join(base_dir, folder_path)) if not is_safe_path(base_dir, abs_folder): logger.warning( diff --git a/src/server/utils/filesystem.py b/src/server/utils/filesystem.py index e3efb8a..5b5eef8 100644 --- a/src/server/utils/filesystem.py +++ b/src/server/utils/filesystem.py @@ -113,32 +113,54 @@ def sanitize_folder_name( def is_safe_path(base_path: str, target_path: str) -> bool: """Check if target_path is safely within base_path. - + Prevents path traversal attacks by ensuring the target path is actually within the base path after resolution. - + + Note on relative paths: a relative ``target_path`` is interpreted + as relative to ``base_path``, *not* to the process's current + working directory. This mirrors how callers use this helper: + they pass a configured base directory and a folder name stored + alongside it (e.g. the series ``folder`` column in the database + holds a relative name like ``"Beyblade Burst (2016)"``, and the + anime directory is configured separately). Without this, a + relative target would be resolved against the process CWD — + which can differ from ``base_path`` (the FastAPI app runs with + CWD=/app while the anime directory is mounted at /data), and + the helper would incorrectly reject the path as outside the + base. Absolute ``target_path`` values are validated against + ``base_path`` directly. + Args: base_path: The base directory that should contain the target - target_path: The path to validate - + target_path: The path to validate (absolute, or relative to base_path) + Returns: bool: True if target_path is safely within base_path - + Example: >>> is_safe_path("/anime", "/anime/Attack on Titan") True + >>> is_safe_path("/anime", "Attack on Titan") # relative -> /anime/Attack on Titan + True >>> is_safe_path("/anime", "/anime/../etc/passwd") False """ - # Resolve to absolute paths + # Resolve base to an absolute path base_resolved = os.path.abspath(base_path) - target_resolved = os.path.abspath(target_path) - + + # Resolve target relative to the base (not the process CWD) when it is + # supplied as a relative path. Absolute targets are validated as-is. + if os.path.isabs(target_path): + target_resolved = os.path.abspath(target_path) + else: + target_resolved = os.path.abspath(os.path.join(base_resolved, target_path)) + # Check that target starts with base (with trailing separator) base_with_sep = base_resolved + os.sep return ( - target_resolved == base_resolved or - target_resolved.startswith(base_with_sep) + target_resolved == base_resolved + or target_resolved.startswith(base_with_sep) ) diff --git a/tests/unit/test_delete_anime_service.py b/tests/unit/test_delete_anime_service.py index 33f0999..68a6267 100644 --- a/tests/unit/test_delete_anime_service.py +++ b/tests/unit/test_delete_anime_service.py @@ -1,6 +1,7 @@ """Unit tests for AnimeService.delete_series().""" from __future__ import annotations +import os from pathlib import Path from unittest.mock import AsyncMock, MagicMock, patch @@ -328,6 +329,69 @@ class TestDeleteSeriesService: assert result.folder_error is not None assert "outside" in result.folder_error.lower() + @pytest.mark.asyncio + async def test_delete_series_relative_folder_with_different_cwd( + self, anime_service, tmp_path + ): + """Regression: delete_series must work when the stored folder is relative + and the process CWD differs from directory_to_search. + + In the container the FastAPI app runs with CWD=/app while the anime + directory is /data. The DB stores the relative folder name (e.g. + "Beyblade Burst (2016)"). The old code called + ``os.path.abspath(folder)`` which joined against CWD=/app and + produced "/app/Beyblade Burst (2016)", which was then rejected as + outside the /data base. The fix resolves relative paths against + the configured anime directory instead. + """ + safe_base = tmp_path / "data" + safe_base.mkdir() + series_folder = safe_base / "Beyblade Burst (2016)" + series_folder.mkdir() + + anime_service._directory = str(safe_base) + + mock_session = AsyncMock() + mock_ctx = _make_db_ctx(mock_session) + + # Stored folder is RELATIVE (matches what's actually in the DB) + mock_series = MagicMock() + mock_series.key = "beyblade-burst" + mock_series.name = "Beyblade Burst" + mock_series.folder = "Beyblade Burst (2016)" + mock_series.id = 336 + + # Simulate process CWD differing from anime dir (container case: + # CWD=/app while anime dir is /data). Use "/" as a stable, always- + # existing CWD distinct from tmp_path. + old_cwd = os.getcwd() + try: + os.chdir("/") + with patch( + "src.server.database.connection.get_db_session", + return_value=mock_ctx, + ), patch( + "src.server.database.service.AnimeSeriesService.get_by_key", + new_callable=AsyncMock, + return_value=mock_series, + ): + result = await anime_service.delete_series( + key="beyblade-burst", + delete_database=False, + delete_folder=True, + ) + finally: + os.chdir(old_cwd) + + # Folder MUST be deleted successfully + assert result.deleted_folder is True, ( + f"folder delete failed: success={result.success} " + f"folder_error={result.folder_error!r}" + ) + assert result.folder_error is None + assert result.success is True + assert not series_folder.exists() + # ------------------------------------------------------------------ # Error handling # ------------------------------------------------------------------ diff --git a/tests/unit/test_filesystem_utils.py b/tests/unit/test_filesystem_utils.py index 71a571e..1902366 100644 --- a/tests/unit/test_filesystem_utils.py +++ b/tests/unit/test_filesystem_utils.py @@ -213,6 +213,34 @@ class TestIsSafePath: "/anime/Attack on Titan/Season 1/Episode 1" ) + def test_relative_target_resolved_against_base(self): + """Relative targets resolve against the base, not the process CWD. + + Regression test: previously `os.path.abspath(target_path)` would + join a relative target against the process's current working + directory. When the CWD differed from `base_path` (e.g. the + FastAPI app running with CWD=/app while the anime directory is + /data), a relative folder name like "Beyblade Burst (2016)" + would be resolved to "/app/Beyblade Burst (2016)" and + incorrectly rejected as outside the base. The helper now + treats a relative target as relative to `base_path`. + """ + with tempfile.TemporaryDirectory() as tmpdir: + base = os.path.abspath(tmpdir) + # Simulate a process CWD different from base + old_cwd = os.getcwd() + try: + os.chdir("/") + # Relative target inside base should be safe + assert is_safe_path(base, "Beyblade Burst (2016)") + # Nested relative target should also be safe + assert is_safe_path(base, "Beyblade Burst (2016)/Season 1") + # Relative traversal (../) must still be rejected even + # when resolved against the base + assert not is_safe_path(base, "../etc/passwd") + finally: + os.chdir(old_cwd) + class TestCreateSafeFolder: """Test create_safe_folder function."""