Просмотр исходного кода

fix: 35 проблем кода — исправлены критические, высокие и средние

Критические:
- Отсутствие go-playground/validator в go.mod
- Data race на WSHub.clients (+sync.Mutex)
- Утечка маркеров Leaflet (removeMarker перед addMarker)

Высокие:
- Утечка внутреннего MinIO URL при ошибке url.Parse
- Игнорирование ошибки GetByEmail (auth + users)
- Stale closure user в геолокации MapView
- Пропущен user в deps [places]/[visitors] эффектов
- WS reconnect после unmount (mountedRef)
- Presigned upload без проверки res.ok

Средние:
- Ошибка тегов сервиса логируется, не глотается
- Tags сервиса теперь обновляются в PATCH
- detail → details в api.ts
- deleted добавлен в PlaceStatus
- NaN валидация lat/lng перед отправкой
- hourlyRate falsy-zero исправлен
- type не шлётся в PATCH
- Silent catches показывают ошибки
- Синхронизирована Obsidian-заметка
neyrogovnarik 1 месяц назад
Родитель
Сommit
7b33a341f1

+ 1 - 0
backend/go.mod

@@ -5,6 +5,7 @@ go 1.22
 require (
 	github.com/go-chi/chi/v5 v5.0.12
 	github.com/go-chi/cors v1.2.1
+	github.com/go-playground/validator/v10 v10.11.1
 	github.com/golang-jwt/jwt/v5 v5.2.0
 	github.com/google/uuid v1.6.0
 	github.com/gorilla/websocket v1.5.1

+ 16 - 1
backend/internal/handlers/services.go

@@ -3,6 +3,7 @@ package handlers
 
 import (
 	"encoding/json"
+	"log/slog"
 	"net/http"
 	"strconv"
 
@@ -77,7 +78,9 @@ func (h *ServiceHandler) GetByID(w http.ResponseWriter, r *http.Request) {
 	}
 
 	tags, err := h.serviceRepo.GetTags(r.Context(), id)
-	if err == nil {
+	if err != nil {
+		slog.Error("failed to fetch service tags", "service_id", id, "error", err)
+	} else {
 		svc.Tags = tags
 	}
 
@@ -190,6 +193,18 @@ func (h *ServiceHandler) Update(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	if req.Tags != nil {
+		tags := make([]models.Tag, len(req.Tags))
+		for i, t := range req.Tags {
+			tags[i] = models.Tag{ID: t}
+		}
+		if err := h.serviceRepo.SetTags(r.Context(), id, tags); err != nil {
+			slog.Error("failed to update service tags", "service_id", id, "error", err)
+		} else {
+			svc.Tags = tags
+		}
+	}
+
 	writeJSON(w, http.StatusOK, svc)
 }
 

+ 4 - 4
backend/internal/handlers/upload.go

@@ -156,11 +156,11 @@ func (h *UploadHandler) PresignedURL(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	// Replace internal endpoint (minio:9000) with public endpoint for browser uploads
-	publicUploadURL := uploadURL
 	uploadURLStr := h.publicEndpoint + uploadURL.Path + "?" + uploadURL.RawQuery
-	if parsed, err := url.Parse(uploadURLStr); err == nil {
-		publicUploadURL = parsed
+	publicUploadURL, err := url.Parse(uploadURLStr)
+	if err != nil {
+		writeError(w, http.StatusInternalServerError, "failed to build public upload URL", err)
+		return
 	}
 
 	fileURL := h.publicEndpoint + "/" + h.bucket + "/" + objectName

+ 5 - 1
backend/internal/handlers/users.go

@@ -110,7 +110,11 @@ func (h *UserHandler) AdminCreateUser(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	existing, _ := h.userRepo.GetByEmail(r.Context(), req.Email)
+	existing, err := h.userRepo.GetByEmail(r.Context(), req.Email)
+	if err != nil {
+		writeError(w, http.StatusInternalServerError, "failed to check existing email", err)
+		return
+	}
 	if existing != nil {
 		writeError(w, http.StatusConflict, "email already exists", nil)
 		return

+ 21 - 1
backend/internal/handlers/websocket.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"log/slog"
 	"net/http"
+	"sync"
 
 	"github.com/gorilla/websocket"
 	"golang.org/x/time/rate"
@@ -25,6 +26,7 @@ type visitorsPayload struct {
 const maxWSConnections = 1000
 
 type WSHub struct {
+	mu             sync.Mutex
 	clients        map[*Client]bool
 	register       chan *Client
 	unregister     chan *Client
@@ -52,22 +54,30 @@ func (h *WSHub) Run() {
 	for {
 		select {
 		case client := <-h.register:
+			h.mu.Lock()
 			if len(h.clients) >= maxWSConnections {
+				h.mu.Unlock()
 				slog.Warn("ws max connections reached, rejecting")
 				client.conn.Close()
 				continue
 			}
 			h.clients[client] = true
+			h.mu.Unlock()
 			h.broadcastVisitors()
 
 		case client := <-h.unregister:
+			h.mu.Lock()
 			if _, ok := h.clients[client]; ok {
 				delete(h.clients, client)
+				h.mu.Unlock()
 				close(client.send)
 				h.broadcastVisitors()
+			} else {
+				h.mu.Unlock()
 			}
 
 		case message := <-h.broadcast:
+			h.mu.Lock()
 			for client := range h.clients {
 				select {
 				case client.send <- message:
@@ -76,11 +86,13 @@ func (h *WSHub) Run() {
 					delete(h.clients, client)
 				}
 			}
+			h.mu.Unlock()
 		}
 	}
 }
 
 func (h *WSHub) broadcastVisitors() {
+	h.mu.Lock()
 	visitors := make([]VisitorDot, 0, len(h.clients))
 	for c := range h.clients {
 		visitors = append(visitors, VisitorDot{
@@ -90,6 +102,12 @@ func (h *WSHub) broadcastVisitors() {
 		})
 	}
 
+	clients := make([]*Client, 0, len(h.clients))
+	for c := range h.clients {
+		clients = append(clients, c)
+	}
+	h.mu.Unlock()
+
 	data, err := json.Marshal(visitorsPayload{Visitors: visitors})
 	if err != nil {
 		return
@@ -99,12 +117,14 @@ func (h *WSHub) broadcastVisitors() {
 		return
 	}
 
-	for client := range h.clients {
+	for _, client := range clients {
 		select {
 		case client.send <- msg:
 		default:
+			h.mu.Lock()
 			close(client.send)
 			delete(h.clients, client)
+			h.mu.Unlock()
 		}
 	}
 }

+ 5 - 1
backend/internal/services/auth.go

@@ -82,7 +82,11 @@ type RegisterInput struct {
 func (s *AuthService) Register(ctx context.Context, input RegisterInput) (*AuthResult, string, error) {
 	logger := log.FromContext(ctx)
 
-	existing, _ := s.userRepo.GetByEmail(ctx, input.Email)
+	existing, err := s.userRepo.GetByEmail(ctx, input.Email)
+	if err != nil {
+		logger.ErrorContext(ctx, "failed to check existing email", log.WithError(err))
+		return nil, "", fmt.Errorf("check existing email: %w", err)
+	}
 	if existing != nil {
 		logger.WarnContext(ctx, "registration attempt with existing email", slog.String("email", input.Email))
 		return nil, "", ErrEmailExists

+ 3 - 3
frontend/src/app/places/my/page.tsx

@@ -56,8 +56,8 @@ function PlaceEditModal({ placeId, onClose, onSaved }: { placeId: string; onClos
       .catch(() => onClose())
       .finally(() => setLoadingPlace(false))
 
-    api.get<Tag[]>('/tags').then(setTags).catch(() => {})
-    api.get<Feature[]>('/features').then(setFeatures).catch(() => {})
+    api.get<Tag[]>('/tags').then(setTags).catch(() => setError('Ошибка загрузки тегов'))
+    api.get<Feature[]>('/features').then(setFeatures).catch(() => setError('Ошибка загрузки характеристик'))
   }, [placeId, onClose])
 
   const handleMapPick = (pickLat: number, pickLng: number, pickAddress?: string) => {
@@ -331,7 +331,7 @@ export default function MyPlacesPage() {
     setLoading(true)
     api.get<{ data: Place[] }>('/places/my')
       .then((res) => setPlaces(res.data || []))
-      .catch(() => {})
+      .catch(() => setToast('Ошибка загрузки мест'))
       .finally(() => setLoading(false))
   }
 

+ 5 - 2
frontend/src/app/services/add/page.tsx

@@ -25,7 +25,7 @@ export default function AddServicePage() {
   const [portfolioFiles, setPortfolioFiles] = useState<File[]>([])
 
   useEffect(() => {
-    api.get<Tag[]>('/tags').then(setTags).catch(() => {})
+    api.get<Tag[]>('/tags').then(setTags).catch(() => setError('Ошибка загрузки тегов'))
   }, [])
 
   const handleSubmit = async (e: React.FormEvent) => {
@@ -44,7 +44,10 @@ export default function AddServicePage() {
         const formData = new FormData()
         Object.entries(presigned.fields).forEach(([k, v]) => formData.append(k, v))
         formData.append('file', file)
-        await fetch(presigned.upload_url, { method: 'POST', body: formData })
+        const uploadRes = await fetch(presigned.upload_url, { method: 'POST', body: formData })
+        if (!uploadRes.ok) {
+          throw new Error(`Ошибка загрузки изображения: ${uploadRes.statusText}`)
+        }
         portfolioImages.push(presigned.file_url)
       }
 

+ 7 - 5
frontend/src/components/MapView.tsx

@@ -9,7 +9,7 @@ import type { Place } from '@/types'
 import { api } from '@/lib/api'
 
 /**
- * Основной компонент карты. Инициализирует Яндекс.Карту, загружает места,
+ * Основной компонент карты. Инициализирует Leaflet, загружает места,
  * отображает маркеры и показывает карточку места при клике.
  * Для неавторизованных пользователей через WebSocket показывает точки других посетителей на карте.
  */
@@ -20,6 +20,8 @@ export default function MapView() {
   const [places, setPlaces] = useState<Place[]>([])
   const [selectedPlace, setSelectedPlace] = useState<Place | null>(null)
   const [mapError, setMapError] = useState(false)
+  const userRef = useRef(user)
+  userRef.current = user
   const userPosRef = useRef<{ lat: number; lng: number } | null>(null)
 
   const fetchPlaces = useCallback(async (bounds?: MapBounds) => {
@@ -62,7 +64,7 @@ export default function MapView() {
             provider.setCenter(lat, lng)
             provider.addMarker('_user_location', lat, lng, {
               type: 'location',
-              color: user ? '#FF4B4B' : '#FFF82A',
+              color: userRef.current ? '#FF4B4B' : '#FFF82A',
             })
           },
           () => {},
@@ -73,7 +75,7 @@ export default function MapView() {
         fetchPlacesRef.current(bounds)
       })
 
-      const b = provider.getBounds?.()
+      const b = provider.getBounds()
       if (b) fetchPlaces(b)
     }).catch(() => {
       setMapError(true)
@@ -114,7 +116,7 @@ export default function MapView() {
     return () => {
       places.forEach((place) => provider.removeMarker(place.id))
     }
-  }, [places])
+  }, [places, user])
 
   const { visitors } = useWebSocket(!user)
 
@@ -130,7 +132,7 @@ export default function MapView() {
     return () => {
       visitors.forEach((_, i) => provider.removeMarker(`ws-visitor-${i}`))
     }
-  }, [visitors])
+  }, [visitors, user])
 
   return (
     <div className="relative h-full w-full">

+ 16 - 7
frontend/src/components/PlaceForm.tsx

@@ -47,8 +47,8 @@ export default function PlaceForm({ type, onSuccess, place }: PlaceFormProps) {
   const [saved, setSaved] = useState<'draft' | 'submitted' | null>(null)
 
   useEffect(() => {
-    api.get<Tag[]>('/tags').then(setTags).catch(() => {})
-    api.get<Feature[]>('/features').then(setFeatures).catch(() => {})
+    api.get<Tag[]>('/tags').then(setTags).catch(() => setError('Ошибка загрузки тегов'))
+    api.get<Feature[]>('/features').then(setFeatures).catch(() => setError('Ошибка загрузки характеристик'))
   }, [])
 
   /** Освобождает blob-URL обложки при размонтировании или смене preview */
@@ -81,23 +81,32 @@ export default function PlaceForm({ type, onSuccess, place }: PlaceFormProps) {
         coverImage = res.file_url
       }
 
+      const parsedLat = parseFloat(lat)
+      const parsedLng = parseFloat(lng)
+      if (isNaN(parsedLat) || isNaN(parsedLng)) {
+        setError('Укажите корректные координаты')
+        return
+      }
+
       const body: Record<string, unknown> = {
         title,
         description: description || undefined,
         address: address || undefined,
-        lat: parseFloat(lat),
-        lng: parseFloat(lng),
+        lat: parsedLat,
+        lng: parsedLng,
         access_info: accessInfo || undefined,
         tags: selectedTags,
         features: selectedFeatures,
-        type,
       }
 
+      if (!isEdit) body.type = type
+
       if (coverImage) body.cover_image = coverImage
 
       if (type === 'studio') {
-        body.hourly_rate = hourlyRate ? parseInt(hourlyRate) : undefined
-        body.min_hours = parseInt(minHours) || 1
+        body.hourly_rate = hourlyRate !== '' ? parseInt(hourlyRate) : undefined
+        const parsedMinHours = parseInt(minHours)
+        body.min_hours = isNaN(parsedMinHours) ? 1 : parsedMinHours
         body.currency = 'RUB'
       }
 

+ 6 - 2
frontend/src/hooks/useWebSocket.ts

@@ -18,6 +18,7 @@ export function useWebSocket(enabled: boolean) {
   const reconnectTimerRef = useRef<ReturnType<typeof setTimeout>>()
   const reconnectAttemptRef = useRef(0)
   const enabledRef = useRef(enabled)
+  const mountedRef = useRef(true)
   enabledRef.current = enabled
 
   const sendPosition = useCallback(() => {
@@ -45,7 +46,7 @@ export function useWebSocket(enabled: boolean) {
     const wsUrl = apiUrl.replace(/^http/, 'ws') + '/ws/visitors'
 
     function connect() {
-      if (!enabledRef.current) return
+      if (!enabledRef.current || !mountedRef.current) return
 
       let ws: WebSocket
       try {
@@ -57,6 +58,7 @@ export function useWebSocket(enabled: boolean) {
       wsRef.current = ws
 
       ws.onopen = () => {
+        if (!mountedRef.current) return
         setConnected(true)
         reconnectAttemptRef.current = 0
         sendPosition()
@@ -72,6 +74,7 @@ export function useWebSocket(enabled: boolean) {
       }
 
       ws.onclose = () => {
+        if (!mountedRef.current) return
         setConnected(false)
         wsRef.current = null
         scheduleReconnect()
@@ -83,7 +86,7 @@ export function useWebSocket(enabled: boolean) {
     }
 
     function scheduleReconnect() {
-      if (!enabledRef.current) return
+      if (!enabledRef.current || !mountedRef.current) return
       const delay = Math.min(
         INITIAL_RECONNECT_DELAY * Math.pow(2, reconnectAttemptRef.current),
         MAX_RECONNECT_DELAY,
@@ -95,6 +98,7 @@ export function useWebSocket(enabled: boolean) {
     connect()
 
     return () => {
+      mountedRef.current = false
       clearTimeout(reconnectTimerRef.current)
       if (wsRef.current) {
         wsRef.current.onclose = null

+ 1 - 1
frontend/src/lib/api.ts

@@ -82,7 +82,7 @@ async function request<T>(
 
   if (!res.ok) {
     const errBody = await res.json().catch(() => ({ error: res.statusText }))
-    const message = errBody.error || errBody.detail || 'Unknown error'
+    const message = errBody.error || errBody.details || 'Unknown error'
     throw new ApiRequestError(res.status, message, errBody)
   }
 

+ 2 - 0
frontend/src/lib/map.ts

@@ -104,6 +104,8 @@ export function createLeafletMapProvider(): MapProvider {
 
     addMarker(id, lat, lng, options) {
       if (!map) return
+      const existing = markers.get(id)
+      if (existing) existing.remove()
       const ghost = options?.ghost
       const type = options?.type || 'place'
       const color = options?.color || MARKER_COLORS[type] || '#FFF82A'

+ 2 - 2
frontend/src/types/index.ts

@@ -33,8 +33,8 @@ export interface User {
 /** Тип объекта: обычное место или студия */
 export type PlaceType = 'place' | 'studio'
 
-/** Статус места: черновик, на модерации, опубликовано, отклонено, на доработке или в архиве */
-export type PlaceStatus = 'draft' | 'pending_moderation' | 'published' | 'rejected' | 'revision' | 'archived'
+/** Статус места: черновик, на модерации, опубликовано, отклонено, на доработке, в архиве или удалено */
+export type PlaceStatus = 'draft' | 'pending_moderation' | 'published' | 'rejected' | 'revision' | 'archived' | 'deleted'
 
 /**
  * Изображение места

+ 5 - 3
obsidian_data/Photoplaces_data/.obsidian/workspace.json

@@ -13,12 +13,12 @@
             "state": {
               "type": "markdown",
               "state": {
-                "file": "architecture-overview.md",
+                "file": "MOC-security-patterns.md",
                 "mode": "source",
                 "source": false
               },
               "icon": "lucide-file",
-              "title": "architecture-overview"
+              "title": "MOC-security-patterns"
             }
           }
         ]
@@ -40,7 +40,7 @@
             "state": {
               "type": "file-explorer",
               "state": {
-                "sortOrder": "alphabetical",
+                "sortOrder": "byModifiedTime",
                 "autoReveal": false
               },
               "icon": "lucide-folder-closed",
@@ -184,6 +184,8 @@
   },
   "active": "3d01ec58e39ece45",
   "lastOpenFiles": [
+    "atomic-code-audit-2025-06-29.md",
+    "atomic-file-upload-magic-bytes.md",
     "architecture-overview.md",
     "Добро пожаловать.md"
   ]

+ 134 - 0
obsidian_data/Photoplaces_data/atomic-code-audit-2025-06-29.md

@@ -0,0 +1,134 @@
+---
+tags: [audit, bug, security, frontend, backend, code-review]
+date: 2025-06-29
+---
+
+# Code Audit: 35 найденных проблем (2025-06-29)
+
+## Критические (3)
+
+### 1. Отсутствует go-playground/validator в go.mod
+- `backend/go.mod` и `backend/internal/validator/validator.go:11`
+- Пакет импортируется, но отсутствует в `go.mod`.
+- **Фикс:** `go mod tidy` или ручное добавление.
+
+### 2. Data race на WSHub.clients
+- `backend/internal/handlers/websocket.go:83-109`
+- `broadcastVisitors()` итерация по `h.clients` без мьютекса из двух горутин.
+- **Фикс:** `sync.RWMutex` при чтении/записи clients.
+
+### 3. Утечка маркеров Leaflet
+- `frontend/src/lib/map.ts:105-122`
+- `addMarker` не удаляет старый маркер с тем же ID — дубликаты в DOM.
+- **Фикс:** `removeMarker(id)` перед `addMarker`.
+
+## Высокие (8)
+
+### 4. Утечка внутреннего URL MinIO
+- `backend/internal/handlers/upload.go:159-164`
+- При ошибке `url.Parse` клиент получает `http://minio:9000/...`.
+
+### 5. Игнорирование ошибки GetByEmail
+- `backend/internal/services/auth.go:85`
+- `backend/internal/handlers/users.go:113`
+- При недоступности БД дубликаты email не проверяются.
+
+### 6-8. Stale closures в MapView
+- `frontend/src/components/MapView.tsx:57-69` — геолокация
+- `frontend/src/components/MapView.tsx:100-117` — маркеры мест
+- `frontend/src/components/MapView.tsx:121-133` — маркеры посетителей
+- Пропущен `user` в deps, stale `user` в замыкании.
+
+### 9. WebSocket reconnect после unmount
+- `frontend/src/hooks/useWebSocket.ts:74-104`
+- `onclose` может сработать после cleanup и запустить reconnect.
+
+### 10. Presigned upload без проверки ответа
+- `frontend/src/app/services/add/page.tsx:39-48`
+- `fetch()` на presigned URL без `res.ok`.
+
+### 11. Логи без контекста запроса
+- `backend/internal/handlers/errors.go:27`
+- Глобальный `slog` без request_id/user_id.
+
+## Средние (10)
+
+### 12. Игнорирование ошибки тегов сервиса
+- `backend/internal/handlers/services.go:79-82`
+
+### 13. Теги сервиса не обновляются
+- `backend/internal/handlers/services.go:96-103,167-193`
+
+### 14. Неиспользуемый атомарный rotate token
+- `backend/internal/repository/refresh_tokens.go:66-125`
+
+### 15. Нет LIMIT в запросах броней
+- `backend/internal/repository/bookings.go:83-107`
+
+### 16. Опчатка `detail` → `details`
+- `frontend/src/lib/api.ts:86`
+
+### 17. `deleted` не в PlaceStatus
+- `frontend/src/app/admin/page.tsx:7`
+
+### 18. Falsy-zero для hourlyRate
+- `frontend/src/components/PlaceForm.tsx:98-99`
+
+### 19. `type` в PATCH
+- `frontend/src/components/PlaceForm.tsx:93`
+
+### 20. Глотание ошибки API
+- `frontend/src/app/places/my/page.tsx:330-336`
+
+### 21. Нет валидации NaN координат
+- `frontend/src/components/PlaceForm.tsx:88-89`
+
+## Низкие (14)
+
+### 22. useSSL захардкожен
+- `backend/cmd/api/main.go:64`
+
+### 23. Неиспользуемая ErrPlaceNotFound
+- `backend/internal/services/places.go:47`
+
+### 24. Неиспользуемые валидаторы
+- `backend/internal/validator/validator.go:34-35`
+
+### 25. Игнорирование ошибок парсинга query
+- `backend/internal/handlers/places.go:62-95`
+
+### 26. cleanup горутина не останавливается
+- `backend/internal/middleware/ratelimit.go:29`
+
+### 27. Soft delete сервиса без смены статуса
+- `backend/internal/repository/services.go:103`
+
+### 28. Лишний ?. на getBounds
+- `frontend/src/components/MapView.tsx:76`
+
+### 29. JSDoc: Яндекс вместо Leaflet
+- `frontend/src/components/MapView.tsx:13`
+
+### 30. Избыточный ref
+- `frontend/src/components/MapView.tsx:41-42`
+
+### 31. fetchPlaces не в deps
+- `frontend/src/app/places/my/page.tsx:338`
+
+### 32. Двойной revokeObjectURL
+- `frontend/src/components/PlaceForm.tsx:55-61`
+
+### 33. SetupHandler без интерфейса
+- `backend/internal/handlers/setup.go:13-17`
+
+### 34. PlaceHandler хранит authSvc для парсинга
+- `backend/internal/handlers/places.go:19-26`
+
+### 35. pointer.Str("") → nil
+- `backend/internal/pointer/pointer.go:3-7`
+
+## Связанные заметки
+- [[MOC-security-patterns]]
+- [[MOC-backend-patterns]]
+- [[atomic-refresh-token-race-condition]]
+- [[atomic-error-swallowing-frontend]]