Ревью: безопасность и корректность в сервисах бэкенда

Находки прохода /code-review high по backend/app:

- achievement_service: slug из URL шёл в путь без проверки, из-за чего
  DELETE /api/admin/achievements/%2E%2E удалял rmtree'ом родительскую папку
  каталога ачивок (в проде это /data — БД, uploads, ачивки целиком).
- match_service/attachment_service: версия партии = updated_at, но onupdate
  срабатывает лишь при реальном UPDATE строки matches. Правка одних участников
  и работа с вложениями его не вызывали, и оптимистичная блокировка молча
  пропускала конкурентную запись — бампаем updated_at явно.
- admin_service: удаление группы с партиями упиралось в RESTRICT и уходило
  наружу голым 500; теперь понятная ошибка. Админское удаление партии не
  чистило файлы вложений с тома — они оставались навсегда.
- user_service: при повторной загрузке аватара с тем же расширением avatar_path
  не менялся, updated_at не двигался, и кэш-бастер оставлял старую картинку до
  часа. Плюс версия считалась из наивного времени как из локального и
  разъезжалась с лидербордом, где то же поле считает SQL.
- membership_service: единственный владелец мог разжаловать сам себя и группа
  оставалась без владельца навсегда.
- notification_service: mark_read не слал SSE-сигнал, и бейдж непрочитанных на
  других устройствах висел до перезагрузки.
- routers/admin: created_at после правки пользователя отдавался без смещения,
  и дата «создан» прыгала на часовой пояс до следующего обновления списка.

#8

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0186Fk74jkkszahEHSjBzTjD

backend/app/routers/admin.py

@@ -114,7 +114,7 @@ def update_user(
         is_active=u.is_active,
         auth_provider=u.auth_provider,
         telegram_id=u.telegram_id,
-        created_at=u.created_at.isoformat(),
+        created_at=iso_utc(u.created_at),
     )
 
 

backend/app/services/achievement_service.py

@@ -42,6 +42,10 @@ def _root() -> Path:
     return Path(settings.achievements_dir)
 
 
+# Формат slug — то, что выдаёт _slugify: только латиница, цифры и дефис.
+_SLUG_RE = re.compile(r"[a-z0-9][a-z0-9-]{0,63}")
+
+
 def _slugify(name: str) -> str:
     text = "".join(_TRANSLIT.get(ch, ch) for ch in (name or "").strip().lower())
     slug = re.sub(r"[^a-z0-9]+", "-", text).strip("-")
@@ -49,6 +53,11 @@ def _slugify(name: str) -> str:
 
 
 def _dir(slug: str) -> Path:
+    """Папка ачивки. Slug приходит из URL, поэтому формат проверяем здесь: без этого
+    `..` или `a/b` увели бы файловые операции (вплоть до rmtree в delete) за пределы
+    каталога ачивок."""
+    if not _SLUG_RE.fullmatch(slug or ""):
+        raise NotFoundError("Ачивка не найдена.")
     return _root() / slug
 
 

backend/app/services/admin_service.py

@@ -6,6 +6,7 @@ from typing import Any
 from sqlmodel import Session, select
 
 from app.core.errors import (
+    ConflictError,
     InvalidCredentialsError,
     NicknameTakenError,
     NotFoundError,
@@ -75,6 +76,10 @@ def delete_group(session: Session, group_id: int) -> None:
     group = session.get(Group, group_id)
     if group is None:
         raise NotFoundError("Группа не найдена.")
+    # matches.group_id — ON DELETE RESTRICT, поэтому группу с партиями БД не отдаст.
+    # Проверяем сами, иначе IntegrityError уходит наружу голым 500 без AppError-конверта.
+    if session.exec(select(Match.id).where(Match.group_id == group_id)).first() is not None:
+        raise ConflictError("Нельзя удалить группу, в которой есть партии. Сначала удалите их.")
     session.delete(group)
     session.commit()
 
@@ -134,11 +139,16 @@ def rename_faction(session: Session, faction_id: int, name_ru: str) -> Faction:
 
 
 def delete_match(session: Session, match_id: int) -> None:
+    from app.services import attachment_service  # избегаем цикла импорта
+
     match = session.get(Match, match_id)
     if match is None:
         raise NotFoundError("Партия не найдена.")
     session.delete(match)
     session.commit()
+    # Как и в игроцком пути (match_service.delete_match): строки вложений уходят
+    # каскадом, а файлы с тома нужно убрать руками, иначе они остаются навсегда.
+    attachment_service.delete_match_files(match_id)
 
 
 # ─── Журнал аудита ───────────────────────────────────────────────────────────

backend/app/services/attachment_service.py

@@ -11,6 +11,7 @@ from sqlmodel import Session, select
 
 from app.core.config import settings
 from app.core.errors import ConflictError, NotFoundError
+from app.core.timeutil import utcnow
 from app.models import Match, MatchAttachment, User
 
 MAX_ATTACHMENTS = 10
@@ -39,6 +40,14 @@ def file_path(att: MatchAttachment) -> Path:
     return Path(settings.upload_dir) / att.storage_path
 
 
+def _touch(session: Session, match: Match) -> None:
+    """Двинуть версию партии: набор вложений виден в MatchRead, а сама строка matches
+    при работе с ними не меняется — без этого оптимистичная блокировка проспала бы
+    конкурентную правку."""
+    match.updated_at = utcnow()
+    session.add(match)
+
+
 def add_photo(
     session: Session, match: Match, user: User, content: bytes, ext: str, mime: str
 ) -> MatchAttachment:
@@ -60,6 +69,7 @@ def add_photo(
     abs_path.write_bytes(content)
     att.storage_path = rel
     session.add(att)
+    _touch(session, match)
     session.commit()
     session.refresh(att)
     return att
@@ -81,6 +91,7 @@ def delete(session: Session, match: Match, att_id: int) -> None:
         except OSError:
             pass
     session.delete(att)
+    _touch(session, match)
     session.commit()
 
 

backend/app/services/match_service.py

@@ -351,6 +351,10 @@ def update_match(
             )
         match.player_count = len(participants)
 
+    # Версия партии = updated_at, а onupdate срабатывает только при реальном UPDATE
+    # строки matches. Правка одних участников его не вызывает, и тогда оптимистичная
+    # блокировка молча пропускала бы конкурентную запись — поэтому бампаем явно.
+    match.updated_at = _utcnow()
     session.add(match)
     session.commit()
     session.refresh(match)

backend/app/services/membership_service.py

@@ -85,6 +85,16 @@ def change_role(session: Session, group: Group, user_id: int, role: str) -> Grou
     ).first()
     if member is None:
         raise NotFoundError("Игрок не состоит в группе.")
+    if member.role == "owner" and role != "owner":
+        # Без этого единственный владелец мог разжаловать сам себя, и группа
+        # оставалась без владельца навсегда: назначить нового уже некому.
+        owners = session.exec(
+            select(GroupMember).where(
+                GroupMember.group_id == group.id, GroupMember.role == "owner"
+            )
+        ).all()
+        if len(owners) <= 1:
+            raise ForbiddenError("Нельзя снять роль с последнего владельца группы.")
     member.role = role
     session.add(member)
     session.commit()

backend/app/services/notification_service.py

@@ -137,6 +137,9 @@ def mark_read(session: Session, user_id: int, ids: list[int] | None = None) -> i
         session.add(row)
     if rows:
         session.commit()
+        # Счётчик непрочитанных изменился — толкаем тот же сигнал, что и create_for,
+        # иначе вкладка на другом устройстве держит устаревший бейдж до перезагрузки.
+        notify.notifications_changed(user_id)
     return len(rows)
 
 

backend/app/services/user_service.py

@@ -3,7 +3,7 @@ from __future__ import annotations
 
 import os
 import re
-from datetime import datetime
+from datetime import datetime, timezone
 from pathlib import Path
 
 from sqlmodel import Session, select
@@ -11,6 +11,7 @@ from sqlmodel import Session, select
 from app.auth.provider import ExternalIdentity
 from app.core.config import settings
 from app.core.errors import NicknameTakenError, NotFoundError, ValidationError
+from app.core.timeutil import utcnow
 from app.models import AuthIdentity, Faction, GroupMember, User
 
 _NICK_RE = re.compile(r"^[\w .\-]{2,64}$", re.UNICODE)
@@ -170,7 +171,13 @@ def avatar_url_for(user_id: int, avatar_path: str | None, updated_at: datetime |
     подтягивал новую картинку после смены (файл перезаписывается по тому же пути)."""
     if not avatar_path:
         return None
-    version = int(updated_at.timestamp()) if updated_at else 0
+    # В БД время наивное и хранится в UTC. .timestamp() у наивного значения считает
+    # его локальным, и версия разъезжалась с лидербордом, где то же поле считает SQL
+    # (strftime('%s') читает его как UTC) — один аватар качался браузером дважды.
+    version = 0
+    if updated_at is not None:
+        aware = updated_at if updated_at.tzinfo else updated_at.replace(tzinfo=timezone.utc)
+        version = int(aware.timestamp())
     return f"/api/users/{user_id}/avatar?v={version}"
 
 
@@ -253,6 +260,10 @@ def set_avatar(session: Session, user: User, content: bytes, ext: str) -> User:
     rel = f"{_AVATAR_SUBDIR}/{user.id}.{ext}"
     (Path(settings.upload_dir) / rel).write_bytes(content)
     user.avatar_path = rel
+    # Файл перезаписывается по тому же пути, поэтому при том же расширении avatar_path
+    # не меняется, UPDATE не эмитится и onupdate не срабатывает. Без явного бампа
+    # кэш-бастер остаётся прежним, и браузер час показывает старую картинку.
+    user.updated_at = utcnow()
     session.add(user)
     session.commit()
     session.refresh(user)
@@ -267,6 +278,7 @@ def clear_avatar(session: Session, user: User) -> User:
         except OSError:
             pass
     user.avatar_path = None
+    user.updated_at = utcnow()
     session.add(user)
     session.commit()
     session.refresh(user)