From 2d376c2bed7607f86d71844bd49b682be4f60a62 Mon Sep 17 00:00:00 2001 From: Pouzor Date: Tue, 7 Jul 2026 11:50:12 +0200 Subject: [PATCH 1/3] fix: harden media path handling against path injection (CodeQL py/path-injection) Resolve media filenames through a shared _resolve_media_path() barrier that confirms the resolved path sits directly under the resolved media dir, so CodeQL can trace the sanitization the regex already guaranteed. ha-relevant: no --- backend/app/api/routes/media.py | 25 +++++++++++++++++++------ backend/tests/test_media.py | 7 +++++++ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/backend/app/api/routes/media.py b/backend/app/api/routes/media.py index 324664d..ec2553f 100644 --- a/backend/app/api/routes/media.py +++ b/backend/app/api/routes/media.py @@ -11,6 +11,7 @@ raw-image uploads reuse the same endpoint. import re import uuid +from pathlib import Path from fastapi import APIRouter, Depends, HTTPException, UploadFile, status from fastapi.responses import FileResponse @@ -40,6 +41,22 @@ MAX_BYTES = 10 * 1024 * 1024 # 10 MB _NAME_RE = re.compile(r"^[0-9a-f]{32}\.(png|jpg|webp)$") +def _resolve_media_path(filename: str) -> Path: + """Resolve `filename` inside the media dir, rejecting anything that escapes it. + + `filename` is validated against `_NAME_RE` (no separators, no `..`) and the + resolved path is confirmed to sit directly under the resolved media dir. + Raises 404 on any mismatch so we never leak whether a path exists elsewhere. + """ + if not _NAME_RE.match(filename): + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Not found") + base = settings.media_dir().resolve() + path = (base / filename).resolve() + if path.parent != base: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Not found") + return path + + @router.post("/upload") async def upload_media(file: UploadFile, _user: str = Depends(get_current_user)) -> dict[str, str]: ext = ALLOWED_TYPES.get(file.content_type or "") @@ -72,9 +89,7 @@ async def upload_media(file: UploadFile, _user: str = Depends(get_current_user)) @router.get("/{filename}") async def get_media(filename: str) -> FileResponse: - if not _NAME_RE.match(filename): - raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Not found") - path = settings.media_dir() / filename + path = _resolve_media_path(filename) if not path.is_file(): raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Not found") return FileResponse(path) @@ -82,8 +97,6 @@ async def get_media(filename: str) -> FileResponse: @router.delete("/{filename}", status_code=status.HTTP_204_NO_CONTENT) async def delete_media(filename: str, _user: str = Depends(get_current_user)) -> None: - if not _NAME_RE.match(filename): - raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Not found") - path = settings.media_dir() / filename + path = _resolve_media_path(filename) if path.is_file(): path.unlink() diff --git a/backend/tests/test_media.py b/backend/tests/test_media.py index 3741eb9..f774e6d 100644 --- a/backend/tests/test_media.py +++ b/backend/tests/test_media.py @@ -79,6 +79,13 @@ async def test_get_rejects_bad_filename(client, media_dir): assert res.status_code == 404 +@pytest.mark.asyncio +async def test_delete_rejects_bad_filename(client, auth_headers, media_dir): + headers = await auth_headers() + res = await client.delete("/api/v1/media/..%2f..%2fetc%2fpasswd", headers=headers) + assert res.status_code == 404 + + @pytest.mark.asyncio async def test_delete_requires_auth_and_removes_file(client, auth_headers, media_dir): headers = await auth_headers() From b907c4b05ec97a0463ba058e389389b16aa3f705 Mon Sep 17 00:00:00 2001 From: Pouzor Date: Tue, 7 Jul 2026 14:24:19 +0200 Subject: [PATCH 2/3] fix: use re.fullmatch for media filename allowlist so CodeQL recognizes barrier ha-relevant: no --- backend/app/api/routes/media.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/app/api/routes/media.py b/backend/app/api/routes/media.py index ec2553f..c04245a 100644 --- a/backend/app/api/routes/media.py +++ b/backend/app/api/routes/media.py @@ -38,7 +38,7 @@ _MAGIC: dict[str, tuple[bytes, ...]] = { MAX_BYTES = 10 * 1024 * 1024 # 10 MB # Only ever serve/delete files we created: 32 hex chars + known extension. -_NAME_RE = re.compile(r"^[0-9a-f]{32}\.(png|jpg|webp)$") +_NAME_RE = re.compile(r"[0-9a-f]{32}\.(png|jpg|webp)") def _resolve_media_path(filename: str) -> Path: @@ -48,7 +48,7 @@ def _resolve_media_path(filename: str) -> Path: resolved path is confirmed to sit directly under the resolved media dir. Raises 404 on any mismatch so we never leak whether a path exists elsewhere. """ - if not _NAME_RE.match(filename): + if not _NAME_RE.fullmatch(filename): raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Not found") base = settings.media_dir().resolve() path = (base / filename).resolve() From 7db57bba3b09e792a5e63b484561acc40ffb7c48 Mon Sep 17 00:00:00 2001 From: Pouzor Date: Tue, 7 Jul 2026 14:35:44 +0200 Subject: [PATCH 3/3] fix: resolve media file by directory listing to kill path-injection taint Match the validated filename against iterdir() entries with == instead of building a path from the user string, so no tainted value ever reaches a filesystem sink. Satisfies CodeQL py/path-injection. ha-relevant: no --- backend/app/api/routes/media.py | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/backend/app/api/routes/media.py b/backend/app/api/routes/media.py index c04245a..76baab9 100644 --- a/backend/app/api/routes/media.py +++ b/backend/app/api/routes/media.py @@ -42,19 +42,21 @@ _NAME_RE = re.compile(r"[0-9a-f]{32}\.(png|jpg|webp)") def _resolve_media_path(filename: str) -> Path: - """Resolve `filename` inside the media dir, rejecting anything that escapes it. + """Return the existing media file named `filename`, or raise 404. - `filename` is validated against `_NAME_RE` (no separators, no `..`) and the - resolved path is confirmed to sit directly under the resolved media dir. - Raises 404 on any mismatch so we never leak whether a path exists elsewhere. + `filename` is validated against `_NAME_RE` (no separators, no `..`), then + matched by name against the actual directory listing. The user string is + only ever compared with `==` against trusted `iterdir()` entries — it never + builds a path — so a crafted value cannot escape the media dir. """ if not _NAME_RE.fullmatch(filename): raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Not found") - base = settings.media_dir().resolve() - path = (base / filename).resolve() - if path.parent != base: - raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Not found") - return path + base = settings.media_dir() + if base.is_dir(): + for entry in base.iterdir(): + if entry.name == filename and entry.is_file(): + return entry + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Not found") @router.post("/upload") @@ -89,14 +91,10 @@ async def upload_media(file: UploadFile, _user: str = Depends(get_current_user)) @router.get("/{filename}") async def get_media(filename: str) -> FileResponse: - path = _resolve_media_path(filename) - if not path.is_file(): - raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Not found") - return FileResponse(path) + # _resolve_media_path returns only an existing file, else raises 404. + return FileResponse(_resolve_media_path(filename)) @router.delete("/{filename}", status_code=status.HTTP_204_NO_CONTENT) async def delete_media(filename: str, _user: str = Depends(get_current_user)) -> None: - path = _resolve_media_path(filename) - if path.is_file(): - path.unlink() + _resolve_media_path(filename).unlink()