fix: resolve relative folder paths against anime dir in delete_series

The folder column in anime_series stores the relative folder name
(e.g. 'Beyblade Burst (2016)'), not an absolute path. The old
delete_series code called os.path.abspath(folder_path) directly,
which joins a relative path against the process current working
directory. Inside the container the FastAPI app runs with CWD=/app
while the anime directory is /data, so 'Beyblade Burst (2016)'
resolved to '/app/Beyblade Burst (2016)' and the is_safe_path check
correctly (but unhelpfully) flagged it as outside the /data base,
skipping the folder delete while still removing the row.

Fix: resolve relative folder paths against the configured anime
directory before validating against the base. Absolute paths still
work unchanged.

Also harden is_safe_path() the same way so a relative target is
treated as relative to base_path, not to the process CWD. Path
traversal ('../etc/passwd') is still rejected.

Adds two regression tests:
- is_safe_path with chdir to '/' and relative target resolves
  against the base
- delete_series with chdir to '/', relative folder in DB,
  succeeds and removes the folder
This commit is contained in:
2026-08-28 21:01:10 +02:00
parent 7df9a84ae9
commit 14f12e55e7
4 changed files with 139 additions and 12 deletions

View File

@@ -1749,9 +1749,22 @@ class AnimeService:
# --- Filesystem deletion --- # --- Filesystem deletion ---
if delete_folder and folder_path: if delete_folder and folder_path:
# Resolve absolute path and validate it is within base directory # Resolve absolute path and validate it is within base directory.
abs_folder = _os.path.abspath(folder_path) #
# 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) 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): if not is_safe_path(base_dir, abs_folder):
logger.warning( logger.warning(

View File

@@ -117,9 +117,23 @@ def is_safe_path(base_path: str, target_path: str) -> bool:
Prevents path traversal attacks by ensuring the target path Prevents path traversal attacks by ensuring the target path
is actually within the base path after resolution. 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: Args:
base_path: The base directory that should contain the target 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: Returns:
bool: True if target_path is safely within base_path bool: True if target_path is safely within base_path
@@ -127,18 +141,26 @@ def is_safe_path(base_path: str, target_path: str) -> bool:
Example: Example:
>>> is_safe_path("/anime", "/anime/Attack on Titan") >>> is_safe_path("/anime", "/anime/Attack on Titan")
True True
>>> is_safe_path("/anime", "Attack on Titan") # relative -> /anime/Attack on Titan
True
>>> is_safe_path("/anime", "/anime/../etc/passwd") >>> is_safe_path("/anime", "/anime/../etc/passwd")
False False
""" """
# Resolve to absolute paths # Resolve base to an absolute path
base_resolved = os.path.abspath(base_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) # Check that target starts with base (with trailing separator)
base_with_sep = base_resolved + os.sep base_with_sep = base_resolved + os.sep
return ( return (
target_resolved == base_resolved or target_resolved == base_resolved
target_resolved.startswith(base_with_sep) or target_resolved.startswith(base_with_sep)
) )

View File

@@ -1,6 +1,7 @@
"""Unit tests for AnimeService.delete_series().""" """Unit tests for AnimeService.delete_series()."""
from __future__ import annotations from __future__ import annotations
import os
from pathlib import Path from pathlib import Path
from unittest.mock import AsyncMock, MagicMock, patch from unittest.mock import AsyncMock, MagicMock, patch
@@ -328,6 +329,69 @@ class TestDeleteSeriesService:
assert result.folder_error is not None assert result.folder_error is not None
assert "outside" in result.folder_error.lower() 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 # Error handling
# ------------------------------------------------------------------ # ------------------------------------------------------------------

View File

@@ -213,6 +213,34 @@ class TestIsSafePath:
"/anime/Attack on Titan/Season 1/Episode 1" "/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: class TestCreateSafeFolder:
"""Test create_safe_folder function.""" """Test create_safe_folder function."""