fix(media): correct legacy folder resolution, ACL bypass, and review findings

* fix(media): correct fallback logic, ACL bypass, and migration cleanup

- Extract shared media_utils.py with proper arithmetic for Telegram
  marked-ID resolution (fixes channel fallback producing wrong paths)
- Fix ACL bypass: enforce access control against resolved path, not URL
- Fix entrypoint crash when media table doesn't exist on pre-v6 databases
- Remove dead _BATCH_SIZE constant and identical dialect branches in migration
- Add try/except for RuntimeError (symlink loops) in fallback paths
- Add debug logging to fallback and ACL legacy grant paths
- Fix except syntax to use parenthesized tuple form
- Add 25 unit tests for media_utils with roundtrip consistency checks
- Bump version to 7.10.15

* fix(media): handle non-numeric folder names in legacy_folder_alternates

Return empty list for non-numeric folders (e.g., "chat1") instead of
raising ValueError. Fixes test_web_thumbnails failure on CI.

* style: format test_media_utils.py

* fix(media): address review findings — PII logs, thumbnail ACL, boundary tests

- Strip chat IDs from all debug log statements (PII compliance)
- Fix thumbnail ACL bypass: ensure_thumbnail now returns resolved folder
  so serve_thumbnail checks ACL on the actual resolved path
- Simplify serve_media path reconstruction (str(relative_to) vs manual join)
- Guard folder_int <= 0 in legacy_folder_alternates
- Fix import style in thumbnails.py (absolute → relative)
- Add boundary tests for exact CHANNEL_ID_OFFSET inflection point
- Add integration tests for ACL bypass scenario (deny + allow)

* fix(media): restore early ACL check in serve_thumbnail for defense-in-depth

Keep the pre-resolution ACL check (prevents file existence leakage), and
add a secondary check only when the resolved folder differs from the
requested folder (prevents bypass via legacy fallback).

Author: GeiserX

alembic/versions/20260524_013_fix_media_file_paths.py

@@ -23,7 +23,9 @@
 branch_labels = None
 depends_on = None
 
-_BATCH_SIZE = 5000
+# Can't import from src.web.media_utils in migrations (different runtime context)
+# Define locally to keep migration self-contained
+_CHANNEL_ID_OFFSET = 1_000_000_000_000
 
 
 def _derive_stale_folder(chat_id: int) -> str | None:
@@ -36,71 +38,46 @@ def _derive_stale_folder(chat_id: int) -> str | None:
     if chat_id >= 0:
         return None
     raw = -chat_id
-    if raw > 1000000000000:
-        return str(raw - 1000000000000)
+    if raw > _CHANNEL_ID_OFFSET:
+        return str(raw - _CHANNEL_ID_OFFSET)
     return str(raw)
 
 
 def upgrade():
     conn = op.get_bind()
-    dialect = conn.dialect.name
-
-    if dialect == "postgresql":
-        # Get all distinct negative chat_ids that have media
-        result = conn.execute(sa.text("SELECT DISTINCT chat_id FROM media WHERE chat_id < 0 AND file_path IS NOT NULL"))
-        chat_ids = [row[0] for row in result]
-
-        for chat_id in chat_ids:
-            stale_folder = _derive_stale_folder(chat_id)
-            if stale_folder is None:
-                continue
-            correct_folder = str(chat_id)
-
-            # Only update rows where file_path contains the stale folder
-            # Use pattern: .../<stale_folder>/... → .../<correct_folder>/...
-            stale_pattern = f"%/{stale_folder}/%"
-            conn.execute(
-                sa.text(
-                    "UPDATE media SET file_path = REPLACE(file_path, :old_seg, :new_seg) "
-                    "WHERE chat_id = :cid AND file_path LIKE :pattern"
-                ),
-                {
-                    "old_seg": f"/{stale_folder}/",
-                    "new_seg": f"/{correct_folder}/",
-                    "cid": chat_id,
-                    "pattern": stale_pattern,
-                },
-            )
-
-    elif dialect == "sqlite":
-        result = conn.execute(sa.text("SELECT DISTINCT chat_id FROM media WHERE chat_id < 0 AND file_path IS NOT NULL"))
-        chat_ids = [row[0] for row in result]
-
-        for chat_id in chat_ids:
-            stale_folder = _derive_stale_folder(chat_id)
-            if stale_folder is None:
-                continue
-            correct_folder = str(chat_id)
-
-            stale_pattern = f"%/{stale_folder}/%"
-            conn.execute(
-                sa.text(
-                    "UPDATE media SET file_path = REPLACE(file_path, :old_seg, :new_seg) "
-                    "WHERE chat_id = :cid AND file_path LIKE :pattern"
-                ),
-                {
-                    "old_seg": f"/{stale_folder}/",
-                    "new_seg": f"/{correct_folder}/",
-                    "cid": chat_id,
-                    "pattern": stale_pattern,
-                },
-            )
+
+    # Get all distinct negative chat_ids that have media
+    result = conn.execute(sa.text("SELECT DISTINCT chat_id FROM media WHERE chat_id < 0 AND file_path IS NOT NULL"))
+    chat_ids = [row[0] for row in result]
+
+    for chat_id in chat_ids:
+        stale_folder = _derive_stale_folder(chat_id)
+        if stale_folder is None:
+            continue
+        correct_folder = str(chat_id)
+
+        # Only update rows where file_path contains the stale folder
+        # Use pattern: .../<stale_folder>/... → .../<correct_folder>/...
+        stale_pattern = f"%/{stale_folder}/%"
+        conn.execute(
+            sa.text(
+                "UPDATE media SET file_path = REPLACE(file_path, :old_seg, :new_seg) "
+                "WHERE chat_id = :cid AND file_path LIKE :pattern"
+            ),
+            {
+                "old_seg": f"/{stale_folder}/",
+                "new_seg": f"/{correct_folder}/",
+                "cid": chat_id,
+                "pattern": stale_pattern,
+            },
+        )
 
 
 def downgrade():
-    # Reversible: swap the folder components back
+    # WARNING: This reverses ALL negative-folder paths to positive, including rows
+    # created after the upgrade. This is intentional — old code expects positive
+    # folders in file_path. The runtime fallback handles disk resolution.
     conn = op.get_bind()
-    dialect = conn.dialect.name
 
     result = conn.execute(sa.text("SELECT DISTINCT chat_id FROM media WHERE chat_id < 0 AND file_path IS NOT NULL"))
     chat_ids = [row[0] for row in result]

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "telegram-archive"
-version = "7.10.14"
+version = "7.10.15"
 description = "Automated Telegram backup with Docker. Performs incremental backups of messages and media on a configurable schedule."
 readme = "README.md"
 requires-python = ">=3.14"

scripts/entrypoint.sh

@@ -93,15 +93,24 @@ if has_tables and not has_alembic:
         );
     \"\"\")
     # Check artifact from migration 013: file_path values use negative chat_id folders
-    # If any media row for a negative chat_id has a correctly-negative folder, 013 has run
+    # Guard: media table may not exist on very old databases
     cur.execute(\"\"\"
         SELECT EXISTS (
-            SELECT 1 FROM media
-            WHERE chat_id < 0 AND file_path LIKE '%/' || CAST(chat_id AS TEXT) || '/%'
-            LIMIT 1
+            SELECT FROM information_schema.tables
+            WHERE table_name = 'media'
         );
     \"\"\")
-    has_013_paths = cur.fetchone()[0]
+    has_media_table = cur.fetchone()[0]
+    has_013_paths = False
+    if has_media_table:
+        cur.execute(\"\"\"
+            SELECT EXISTS (
+                SELECT 1 FROM media
+                WHERE chat_id < 0 AND file_path LIKE '%/' || CAST(chat_id AS TEXT) || '/%'
+                LIMIT 1
+            );
+        \"\"\")
+        has_013_paths = cur.fetchone()[0]
 
     # Check artifact from migration 012: idx_media_chat_type index
     cur.execute(\"\"\"
@@ -300,8 +309,13 @@ if has_tables and not has_alembic:
     ''')
 
     # Check artifact from migration 013: file_path values use negative chat_id folders
-    cur.execute(\"SELECT EXISTS(SELECT 1 FROM media WHERE chat_id < 0 AND file_path LIKE '%/' || CAST(chat_id AS TEXT) || '/%' LIMIT 1)\")
-    has_013_paths = cur.fetchone()[0]
+    # Guard: media table may not exist on very old databases
+    cur.execute(\"SELECT name FROM sqlite_master WHERE type='table' AND name='media'\")
+    has_media_table = cur.fetchone() is not None
+    has_013_paths = False
+    if has_media_table:
+        cur.execute(\"SELECT EXISTS(SELECT 1 FROM media WHERE chat_id < 0 AND file_path LIKE '%/' || CAST(chat_id AS TEXT) || '/%' LIMIT 1)\")
+        has_013_paths = cur.fetchone()[0]
 
     # Check artifact from migration 012: idx_media_chat_type index
     cur.execute(\"SELECT name FROM sqlite_master WHERE type='index' AND name='idx_media_chat_type'\")

src/__init__.py

@@ -2,4 +2,4 @@
 Telegram Backup Automation - Main Package
 """
 
-__version__ = "7.10.14"
+__version__ = "7.10.15"

src/web/main.py

@@ -31,6 +31,7 @@
 from ..config import Config
 from ..db import DatabaseAdapter, close_database, get_db_manager, init_database
 from ..realtime import RealtimeListener
+from .media_utils import legacy_folder_alternates, legacy_marked_chat_ids
 
 if TYPE_CHECKING:
     from .push import PushNotificationManager
@@ -821,8 +822,8 @@ def _enforce_media_acl(path: str, user: UserContext, *, thumbnail: bool = False)
         raise HTTPException(status_code=403, detail="Access denied")
     if media_chat_id not in user_chat_ids:
         # Legacy fallback: positive folder may correspond to negative marked ID
-        if media_chat_id > 0 and (-media_chat_id in user_chat_ids or -(1000000000000 + media_chat_id) in user_chat_ids):
-            pass
+        if media_chat_id > 0 and any(mid in user_chat_ids for mid in legacy_marked_chat_ids(media_chat_id)):
+            logger.debug("ACL legacy grant: positive folder mapped to allowed chat via marked-ID convention")
         else:
             raise HTTPException(status_code=403, detail="Access denied")
 
@@ -885,7 +886,7 @@ async def serve_thumbnail(size: int, folder: str, filename: str, user: UserConte
     if user.no_download and not folder.startswith("avatars/"):
         raise HTTPException(status_code=403, detail="Downloads disabled for this account")
 
-    # Chat-level access check
+    # Early ACL check on requested path (prevents existence leakage)
     _enforce_media_acl(f"{folder}/{filename}", user, thumbnail=True)
 
     from .thumbnails import ensure_thumbnail, resolve_cache_dir
@@ -894,10 +895,15 @@ async def serve_thumbnail(size: int, folder: str, filename: str, user: UserConte
     if _thumb_cache_dir is None:
         _thumb_cache_dir = resolve_cache_dir(_media_root)
 
-    thumb_path = await ensure_thumbnail(_media_root, size, folder, filename, cache_dir=_thumb_cache_dir)
-    if not thumb_path:
+    result = await ensure_thumbnail(_media_root, size, folder, filename, cache_dir=_thumb_cache_dir)
+    if not result:
         raise HTTPException(status_code=404, detail="Thumbnail not available")
 
+    thumb_path, resolved_folder = result
+    # Secondary ACL on resolved path if it differs (prevents bypass via legacy fallback)
+    if resolved_folder != folder:
+        _enforce_media_acl(f"{resolved_folder}/{filename}", user, thumbnail=True)
+
     return FileResponse(thumb_path, media_type="image/webp", headers={"Cache-Control": "public, max-age=86400"})
 
 
@@ -928,23 +934,20 @@ async def serve_media(path: str, download: int = Query(0), user: UserContext = D
         resolved = None
         if len(parts) == 2:
             folder, rest = parts
-            alt_folders = []
-            if not folder.startswith("-"):
-                alt_folders = [f"-{folder}", f"-100{folder}"]
-            else:
-                alt_folders = [folder[1:]]
+            alt_folders = legacy_folder_alternates(folder)
             for alt in alt_folders:
                 try:
                     resolved = (_media_root / alt / rest).resolve(strict=True)
+                    logger.debug("Legacy fallback: served media via alternate folder resolution")
                     break
-                except OSError, ValueError:
+                except OSError, ValueError, RuntimeError:
                     continue
         if resolved is None:
             raise HTTPException(status_code=404, detail="File not found")
     if not resolved.is_relative_to(_media_root):
         raise HTTPException(status_code=403, detail="Access denied")
 
-    _enforce_media_acl(path, user)
+    _enforce_media_acl(str(resolved.relative_to(_media_root)), user)
 
     if not resolved.is_file():
         raise HTTPException(status_code=404, detail="File not found")

src/web/media_utils.py

@@ -0,0 +1,56 @@
+"""Shared utilities for legacy media path resolution.
+
+Centralizes the Telegram marked-ID convention so it's defined once
+and used consistently across serve_media, thumbnails, and ACL checks.
+"""
+
+CHANNEL_ID_OFFSET: int = 1_000_000_000_000
+
+
+def legacy_folder_alternates(folder: str) -> list[str]:
+    """Return alternate folder names for legacy positive/negative ID paths.
+
+    Forward (positive folder → possible negative marked IDs on disk):
+        "1234567890" → ["-1234567890", "-1001234567890"]
+
+    Reverse (negative folder → possible old positive folder on disk):
+        "-1234567890"    → ["1234567890"]           (basic group)
+        "-1001234567890" → ["1234567890"]           (channel)
+    """
+    try:
+        if not folder.startswith("-"):
+            folder_int = int(folder)
+            if folder_int <= 0:
+                return []
+            return [f"-{folder}", str(-(CHANNEL_ID_OFFSET + folder_int))]
+        folder_int = int(folder)
+    except ValueError:
+        return []
+    raw = -folder_int
+    if raw > CHANNEL_ID_OFFSET:
+        return [str(raw - CHANNEL_ID_OFFSET)]
+    return [str(raw)]
+
+
+def legacy_marked_chat_ids(positive_id: int) -> list[int]:
+    """Return possible marked chat_ids for a legacy positive folder ID.
+
+    Used by ACL checks to determine if a user has access to a chat
+    referenced by its old positive folder name.
+    """
+    return [-positive_id, -(CHANNEL_ID_OFFSET + positive_id)]
+
+
+def derive_stale_folder(chat_id: int) -> str | None:
+    """Derive the old positive folder name from a marked chat_id.
+
+    Basic groups: chat_id = -X  →  old folder = "X"
+    Channels:     chat_id = -(10^12 + X)  →  old folder = "X"
+    Users:        chat_id > 0  →  no mismatch possible, return None
+    """
+    if chat_id >= 0:
+        return None
+    raw = -chat_id
+    if raw > CHANNEL_ID_OFFSET:
+        return str(raw - CHANNEL_ID_OFFSET)
+    return str(raw)

src/web/thumbnails.py

@@ -15,6 +15,8 @@
 
 from PIL import Image
 
+from .media_utils import legacy_folder_alternates
+
 logger = logging.getLogger(__name__)
 
 # Limit decompression to prevent pixel-bomb OOM attacks (~50 megapixels)
@@ -87,11 +89,12 @@ def _generate_sync(source: Path, dest: Path, size: int) -> bool:
 
 async def ensure_thumbnail(
     media_root: Path, size: int, folder: str, filename: str, *, cache_dir: Path | None = None
-) -> Path | None:
-    """Return the path to a cached thumbnail, generating it if needed.
+) -> tuple[Path, str] | None:
+    """Return (thumb_path, resolved_folder) or None.
 
-    Returns None when the request is invalid or generation fails.
-    Includes path traversal protection.
+    resolved_folder is the actual folder the source was found in (may differ
+    from the requested folder due to legacy ID fallback). Callers use this
+    for ACL enforcement on the resolved path.
 
     When cache_dir is provided, thumbnails are written there instead of
     under {media_root}/.thumbs/ — this supports read-only media volumes.
@@ -120,28 +123,29 @@ async def ensure_thumbnail(
         if not dest.is_relative_to(thumbs_root):
             return None
 
+    resolved_folder = folder
+
     if dest.exists():
-        return dest
+        return dest, resolved_folder
 
     if not source.exists():
-        # Legacy fallback: pre-v4.0.5 paths used positive IDs, disk uses negative marked IDs.
-        # Try alternate folder names: X→-X (basic group), X→-100X (channel/supergroup)
-        alt_folders = []
-        if not folder.startswith("-"):
-            alt_folders = [f"-{folder}", f"-100{folder}"]
-        else:
-            alt_folders = [folder[1:]]
+        alt_folders = legacy_folder_alternates(folder)
         found = False
         for alt in alt_folders:
-            alt_source = (media_root / alt / filename).resolve()
-            if alt_source.is_relative_to(media_root_resolved) and alt_source.exists():
-                source = alt_source
-                found = True
-                break
+            try:
+                alt_source = (media_root / alt / filename).resolve()
+                if alt_source.is_relative_to(media_root_resolved) and alt_source.exists():
+                    logger.debug("Thumbnail legacy fallback resolved via alternate folder")
+                    source = alt_source
+                    resolved_folder = alt
+                    found = True
+                    break
+            except OSError, RuntimeError:
+                continue
         if not found:
             return None
 
     async with _generation_semaphore:
         loop = asyncio.get_running_loop()
         ok = await loop.run_in_executor(None, _generate_sync, source, dest, size)
-    return dest if ok else None
+    return (dest, resolved_folder) if ok else None