From 371604de3f3ed41ec00cfd18140add1154f761a9 Mon Sep 17 00:00:00 2001 From: itqop Date: Tue, 30 Dec 2025 16:00:44 +0300 Subject: [PATCH] fixes third pass --- .claude/settings.local.json | 3 +- FIXES.md | 161 ++++++++++++++++++ backend/src/app/api/schemas.py | 4 +- backend/src/app/domain/models.py | 4 +- backend/src/app/infra/s3_client.py | 4 +- .../src/app/repositories/asset_repository.py | 4 +- .../src/app/repositories/share_repository.py | 8 +- backend/src/app/services/asset_service.py | 7 + frontend/src/components/ViewerModal.tsx | 14 +- 9 files changed, 193 insertions(+), 16 deletions(-) diff --git a/.claude/settings.local.json b/.claude/settings.local.json index fed9a2c..147ac2f 100644 --- a/.claude/settings.local.json +++ b/.claude/settings.local.json @@ -1,7 +1,8 @@ { "permissions": { "allow": [ - "Bash(dir:*)" + "Bash(dir:*)", + "Bash(grep:*)" ] } } diff --git a/FIXES.md b/FIXES.md index 8fa8fb3..f0eb679 100644 --- a/FIXES.md +++ b/FIXES.md @@ -92,10 +92,171 @@ if (response.asset) { 5. **Unit tests** - тесты для backend и frontend 6. **Integration tests** - E2E тесты +## Второй проход исправлений + +### 8. Исправлена зависимость useEffect в ViewerModal +**Файл:** `frontend/src/components/ViewerModal.tsx` +**Проблема:** +- Неполный массив зависимостей в useEffect для обработки клавиатуры +- Отсутствовали `assets` и `onClose` в зависимостях +- Это приводило к stale closures и некорректной работе навигации + +**Исправление:** +```typescript +useEffect(() => { + const handleKeyPress = (e: KeyboardEvent) => { + if (!asset) return; + // Inline keyboard handlers to avoid stale closures + if (e.key === 'Escape') { + onClose(); + } else if (e.key === 'ArrowLeft') { + if (currentIndex > 0) { + const prevAsset = assets[currentIndex - 1]; + loadMedia(prevAsset); + setCurrentIndex(currentIndex - 1); + } + } else if (e.key === 'ArrowRight') { + if (currentIndex < assets.length - 1) { + const nextAsset = assets[currentIndex + 1]; + loadMedia(nextAsset); + setCurrentIndex(currentIndex + 1); + } + } + }; + window.addEventListener('keydown', handleKeyPress); + return () => window.removeEventListener('keydown', handleKeyPress); +}, [asset, currentIndex, assets, onClose]); // Added assets, onClose +``` + +### 9. Добавлена валидация в restore_asset() +**Файл:** `backend/src/app/services/asset_service.py` +**Проблема:** Метод `restore_asset()` не проверял, был ли файл действительно удален +**Исправление:** Добавлена проверка перед восстановлением: +```python +if not asset.deleted_at: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail="Asset is not deleted", + ) +``` + +### 10. Исправлено использование устаревшего datetime.utcnow() +**Файлы:** +- `backend/src/app/infra/s3_client.py` +- `backend/src/app/repositories/share_repository.py` +- `backend/src/app/repositories/asset_repository.py` + +**Проблема:** +- `datetime.utcnow()` объявлен устаревшим в Python 3.12+ +- Рекомендуется использовать `datetime.now(timezone.utc)` + +**Исправление:** +Обновлены импорты: +```python +from datetime import datetime, timezone +``` + +Заменены все вызовы: +- `s3_client.py:45` - `now = datetime.now(timezone.utc)` +- `share_repository.py:53` - `expires_at = datetime.now(timezone.utc) + timedelta(...)` +- `share_repository.py:104` - `share.revoked_at = datetime.now(timezone.utc)` +- `share_repository.py:121` - `if share.expires_at and share.expires_at < datetime.now(timezone.utc):` +- `asset_repository.py:137` - `asset.deleted_at = datetime.now(timezone.utc)` + ## Итого исправлений +### Первый проход - **Backend:** 3 исправления + 1 архитектурное улучшение - **Frontend:** 3 исправления - **Всего:** 7 улучшений +### Второй проход +- **Backend:** 2 исправления (валидация restore_asset + datetime.utcnow в 3 файлах) +- **Frontend:** 1 исправление (ViewerModal useEffect dependencies) +- **Всего:** 3 улучшения + +## Третий проход исправлений + +### 11. Исправлен тип колонки size_bytes на BigInteger +**Файл:** `backend/src/app/domain/models.py` +**Проблема:** +- `size_bytes` использовал тип `Integer` (32-bit, максимум ~2GB) +- В конфигурации разрешены загрузки до 20GB +- Это приводило бы к ошибкам переполнения для файлов больше 2GB + +**Исправление:** +```python +from sqlalchemy import BigInteger, Boolean, DateTime, Enum, Float, Integer, String, Text, func + +# ... + +size_bytes: Mapped[int] = mapped_column(BigInteger, nullable=False) +``` + +### 12. Добавлена валидация максимального размера файла +**Файл:** `backend/src/app/api/schemas.py` +**Проблема:** +- `CreateUploadRequest` проверял только `size_bytes > 0` +- Не было верхнего ограничения +- Пользователи могли запросить загрузку терабайтов данных + +**Исправление:** +```python +class CreateUploadRequest(BaseModel): + """Request to create an upload.""" + + original_filename: str = Field(max_length=512) + content_type: str = Field(max_length=100) + size_bytes: int = Field(gt=0, le=21474836480) # Max 20GB +``` + +### 13. Добавлена валидация минимальной длины пароля для share +**Файл:** `backend/src/app/api/schemas.py` +**Проблема:** +- Поле `password` не имело валидации +- Пользователи могли создавать share с паролем "1" или другим слабым паролем +- Это создавало уязвимость безопасности + +**Исправление:** +```python +class CreateShareRequest(BaseModel): + """Request to create a share link.""" + + asset_id: Optional[str] = None + album_id: Optional[str] = None + expires_in_seconds: Optional[int] = Field(None, gt=0) + password: Optional[str] = Field(None, min_length=6, max_length=100) +``` + +### Общий итог + +#### Первый проход +- **Backend:** 3 исправления + 1 архитектурное улучшение +- **Frontend:** 3 исправления +- **Всего:** 7 улучшений + +#### Второй проход +- **Backend:** 2 исправления (валидация restore_asset + datetime.utcnow в 3 файлах) +- **Frontend:** 1 исправление (ViewerModal useEffect dependencies) +- **Всего:** 3 улучшения + +#### Третий проход +- **Backend:** 3 критических исправления (BigInteger, upload size validation, share password validation) +- **Frontend:** 0 исправлений +- **Всего:** 3 улучшения + +### Итоговая статистика +- **Backend:** 8 исправлений + 1 архитектурное улучшение +- **Frontend:** 4 исправления +- **Всего:** 13 улучшений за 3 прохода + +## Критические улучшения безопасности и надежности + +1. ✅ Исправлен переполнение Integer для больших файлов (БД) +2. ✅ Добавлена валидация размера файлов (защита от DoS) +3. ✅ Добавлена валидация пароля share (безопасность) +4. ✅ Исправлена работа публичных ссылок без аутентификации +5. ✅ Устранены проблемы с устаревшими API (datetime, regex) +6. ✅ Исправлены stale closures в React компонентах + Все критические ошибки исправлены. Проект готов к запуску! diff --git a/backend/src/app/api/schemas.py b/backend/src/app/api/schemas.py index ddeda11..5ffc73f 100644 --- a/backend/src/app/api/schemas.py +++ b/backend/src/app/api/schemas.py @@ -79,7 +79,7 @@ class CreateUploadRequest(BaseModel): original_filename: str = Field(max_length=512) content_type: str = Field(max_length=100) - size_bytes: int = Field(gt=0) + size_bytes: int = Field(gt=0, le=21474836480) # Max 20GB class CreateUploadResponse(BaseModel): @@ -113,7 +113,7 @@ class CreateShareRequest(BaseModel): asset_id: Optional[str] = None album_id: Optional[str] = None expires_in_seconds: Optional[int] = Field(None, gt=0) - password: Optional[str] = None + password: Optional[str] = Field(None, min_length=6, max_length=100) class ShareResponse(BaseModel): diff --git a/backend/src/app/domain/models.py b/backend/src/app/domain/models.py index ed8c3e3..d39bf68 100644 --- a/backend/src/app/domain/models.py +++ b/backend/src/app/domain/models.py @@ -5,7 +5,7 @@ from datetime import datetime from typing import Optional from uuid import uuid4 -from sqlalchemy import Boolean, DateTime, Enum, Float, Integer, String, Text, func +from sqlalchemy import BigInteger, Boolean, DateTime, Enum, Float, Integer, String, Text, func from sqlalchemy.orm import Mapped, mapped_column from app.infra.database import Base @@ -62,7 +62,7 @@ class Asset(Base): ) original_filename: Mapped[str] = mapped_column(String(512), nullable=False) content_type: Mapped[str] = mapped_column(String(100), nullable=False) - size_bytes: Mapped[int] = mapped_column(Integer, nullable=False) + size_bytes: Mapped[int] = mapped_column(BigInteger, nullable=False) sha256: Mapped[Optional[str]] = mapped_column(String(64), nullable=True) # Metadata diff --git a/backend/src/app/infra/s3_client.py b/backend/src/app/infra/s3_client.py index ecc47c1..e1b7ba1 100644 --- a/backend/src/app/infra/s3_client.py +++ b/backend/src/app/infra/s3_client.py @@ -1,6 +1,6 @@ """S3 client for file storage operations.""" -from datetime import datetime +from datetime import datetime, timezone from typing import Optional import boto3 @@ -42,7 +42,7 @@ class S3Client: Returns: Storage key """ - now = datetime.utcnow() + now = datetime.now(timezone.utc) year = now.strftime("%Y") month = now.strftime("%m") return f"u/{user_id}/{prefix}/{year}/{month}/{asset_id}{extension}" diff --git a/backend/src/app/repositories/asset_repository.py b/backend/src/app/repositories/asset_repository.py index 29885b0..8ed237f 100644 --- a/backend/src/app/repositories/asset_repository.py +++ b/backend/src/app/repositories/asset_repository.py @@ -1,6 +1,6 @@ """Asset repository for database operations.""" -from datetime import datetime +from datetime import datetime, timezone from typing import Optional from sqlalchemy import desc, select @@ -134,7 +134,7 @@ class AssetRepository: Returns: Updated asset """ - asset.deleted_at = datetime.utcnow() + asset.deleted_at = datetime.now(timezone.utc) asset.status = AssetStatus.DELETED return await self.update(asset) diff --git a/backend/src/app/repositories/share_repository.py b/backend/src/app/repositories/share_repository.py index 05d8d1b..d95143b 100644 --- a/backend/src/app/repositories/share_repository.py +++ b/backend/src/app/repositories/share_repository.py @@ -1,7 +1,7 @@ """Share repository for database operations.""" import secrets -from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone from typing import Optional from sqlalchemy import select @@ -50,7 +50,7 @@ class ShareRepository: token = self._generate_token() expires_at = None if expires_in_seconds: - expires_at = datetime.utcnow() + timedelta(seconds=expires_in_seconds) + expires_at = datetime.now(timezone.utc) + timedelta(seconds=expires_in_seconds) share = Share( owner_user_id=owner_user_id, @@ -101,7 +101,7 @@ class ShareRepository: Returns: Updated share """ - share.revoked_at = datetime.utcnow() + share.revoked_at = datetime.now(timezone.utc) await self.session.flush() await self.session.refresh(share) return share @@ -118,6 +118,6 @@ class ShareRepository: """ if share.revoked_at: return False - if share.expires_at and share.expires_at < datetime.utcnow(): + if share.expires_at and share.expires_at < datetime.now(timezone.utc): return False return True diff --git a/backend/src/app/services/asset_service.py b/backend/src/app/services/asset_service.py index ced2b7a..2e35182 100644 --- a/backend/src/app/services/asset_service.py +++ b/backend/src/app/services/asset_service.py @@ -250,6 +250,13 @@ class AssetService: status_code=status.HTTP_404_NOT_FOUND, detail="Asset not found", ) + + if not asset.deleted_at: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail="Asset is not deleted", + ) + return await self.asset_repo.restore(asset) async def purge_asset(self, user_id: str, asset_id: str) -> None: diff --git a/frontend/src/components/ViewerModal.tsx b/frontend/src/components/ViewerModal.tsx index e4bc42d..3ffb1ff 100644 --- a/frontend/src/components/ViewerModal.tsx +++ b/frontend/src/components/ViewerModal.tsx @@ -51,15 +51,23 @@ export default function ViewerModal({ if (e.key === 'Escape') { onClose(); } else if (e.key === 'ArrowLeft') { - handlePrev(); + if (currentIndex > 0) { + const prevAsset = assets[currentIndex - 1]; + loadMedia(prevAsset); + setCurrentIndex(currentIndex - 1); + } } else if (e.key === 'ArrowRight') { - handleNext(); + if (currentIndex < assets.length - 1) { + const nextAsset = assets[currentIndex + 1]; + loadMedia(nextAsset); + setCurrentIndex(currentIndex + 1); + } } }; window.addEventListener('keydown', handleKeyPress); return () => window.removeEventListener('keydown', handleKeyPress); - }, [asset, currentIndex]); + }, [asset, currentIndex, assets, onClose]); const loadMedia = async (asset: Asset) => { try {