Selaa lähdekoodia

fix: устранены баги аудита 2026-06-30 (upload-цикл, 403, статус брони, magic-bytes)

- frontend/api.ts: guard isRetry в requestFormData — нет бесконечного цикла на 401
- handlers/places.go: ErrNotYourPlace → 403 вместо 500
- handlers/bookings.go: Confirm только из статуса pending (409 иначе)
- handlers/upload.go: проверка magic-bytes файла (защита от подмены Content-Type)
- repository/bookings.go: удалён мёртвый код (IsTimeSlotAvailable, UpdateStatus)
- config.go: trim для ALLOWED_ORIGINS
- handlers/geocode.go: не отдаём upstream-тело наружу
- Obsidian: 4 новые atomic-заметки, обновлён MOC-security-patterns, FINDINGS.md

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
neyrogovnarik 1 kuukausi sitten
vanhempi
sitoutus
a9b3f2f150

+ 21 - 0
FINDINGS.md

@@ -138,3 +138,24 @@
 - [[atomic-booking-price-truncation]]
 - [[decision-validation-error-production]]
 - [[MOC-security-patterns]]
+
+---
+
+## Исправления от 2026-06-30 (аудит neyrogovnarik)
+
+| # | Проблема | Решение | Файл |
+|---|----------|---------|------|
+| 1 | **Бесконечный цикл при upload на 401** — `requestFormData` повторял запрос после refresh без guard'а; при повторном 401 уходил в плотный цикл к `/auth/refresh` | Добавлен флаг `isRetry` (как в `request`) — однократный повтор | `frontend/src/lib/api.ts` |
+| 2 | **`PATCH /places/{id}` отдавал 500 вместо 403** — sentinel `ErrNotYourPlace` не мапился в хендлере | `errors.Is(err, services.ErrNotYourPlace)` → 403 | `handlers/places.go` |
+| 3 | **`Confirm` брони без проверки статуса** — можно подтвердить отменённую бронь / 200 на несуществующий id | Переход через `UpdateStatusIfPending`; 409 если не `pending`. Удалён мёртвый `UpdateStatus` | `handlers/bookings.go`, `repository/bookings.go` |
+| 4 | **Upload доверял Content-Type клиента** — можно залить произвольный файл под видом изображения | Проверка magic-bytes (`http.DetectContentType` + ручной ISO-BMFF для heic), seek в начало | `handlers/upload.go` |
+| 5 | **Мёртвый код** — `IsTimeSlotAvailable` (не вызывается, `tsrange` vs `tstzrange`) | Удалён; защита от двойных броней работает через DB exclusion-constraint | `repository/bookings.go` |
+| 6 | **`ALLOWED_ORIGINS` без trim** — origin с пробелом не совпадал в CORS | `strings.TrimSpace` + отброс пустых | `config/config.go` |
+| 7 | **`geocode` светил upstream-тело** в `raw_error` | Тело отбрасывается, наружу только `display_name: null` | `handlers/geocode.go` |
+
+### Obsidian-заметки (добавлены)
+
+- [[atomic-fetch-retry-infinite-loop]]
+- [[atomic-sentinel-error-handler-mapping]]
+- [[atomic-booking-status-transition-guard]]
+- [[atomic-file-upload-magic-bytes]]

+ 8 - 1
backend/internal/config/config.go

@@ -85,7 +85,14 @@ func parseAllowedOrigins(val string, required bool) []string {
 		}
 		return []string{"http://localhost:3000"}
 	}
-	return strings.Split(val, ",")
+	parts := strings.Split(val, ",")
+	origins := make([]string, 0, len(parts))
+	for _, p := range parts {
+		if trimmed := strings.TrimSpace(p); trimmed != "" {
+			origins = append(origins, trimmed)
+		}
+	}
+	return origins
 }
 
 func (c *Config) validate() {

+ 8 - 1
backend/internal/handlers/bookings.go

@@ -150,10 +150,17 @@ func (h *BookingHandler) Cancel(w http.ResponseWriter, r *http.Request) {
 func (h *BookingHandler) Confirm(w http.ResponseWriter, r *http.Request) {
 	id := chi.URLParam(r, "id")
 
-	if err := h.bookingRepo.UpdateStatus(r.Context(), id, "confirmed"); err != nil {
+	// Подтверждаем только бронь в статусе 'pending': нельзя реанимировать
+	// отменённую бронь или подтвердить несуществующую.
+	ok, err := h.bookingRepo.UpdateStatusIfPending(r.Context(), id, "confirmed")
+	if err != nil {
 		writeError(w, http.StatusInternalServerError, "failed to confirm booking", err)
 		return
 	}
+	if !ok {
+		writeError(w, http.StatusConflict, "booking cannot be confirmed in its current state", nil)
+		return
+	}
 
 	writeJSON(w, http.StatusOK, map[string]string{"status": "confirmed"})
 }

+ 2 - 2
backend/internal/handlers/geocode.go

@@ -44,10 +44,10 @@ func GeocodeReverse(w http.ResponseWriter, r *http.Request) {
 	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusOK {
-		body, _ := io.ReadAll(resp.Body)
+		// Тело ответа вышестоящего сервиса наружу не отдаём, только логируем.
+		io.Copy(io.Discard, resp.Body)
 		writeJSON(w, http.StatusOK, map[string]interface{}{
 			"display_name": nil,
-			"raw_error":    string(body),
 		})
 		return
 	}

+ 5 - 0
backend/internal/handlers/places.go

@@ -3,6 +3,7 @@ package handlers
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"strconv"
@@ -312,6 +313,10 @@ func (h *PlaceHandler) Update(w http.ResponseWriter, r *http.Request) {
 
 	place, err := h.placeSvc.Update(r.Context(), input, isModerator)
 	if err != nil {
+		if errors.Is(err, services.ErrNotYourPlace) {
+			writeError(w, http.StatusForbidden, "not your place", nil)
+			return
+		}
 		writeError(w, http.StatusInternalServerError, "failed to update place", err)
 		return
 	}

+ 54 - 0
backend/internal/handlers/upload.go

@@ -2,9 +2,12 @@
 package handlers
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
+	"io"
 	"net/http"
 	"net/url"
 	"strings"
@@ -75,6 +78,14 @@ func (h *UploadHandler) UploadFile(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// Заголовок Content-Type подделывается клиентом — проверяем реальную
+	// сигнатуру файла по первым байтам, чтобы нельзя было залить произвольный
+	// контент под видом изображения.
+	if err := verifyImageSignature(file, contentType); err != nil {
+		writeError(w, http.StatusBadRequest, "file content does not match declared image type", err)
+		return
+	}
+
 	ext := ".jpg"
 	switch contentType {
 	case "image/png":
@@ -161,6 +172,49 @@ func (h *UploadHandler) PresignedURL(w http.ResponseWriter, r *http.Request) {
 	})
 }
 
+// verifyImageSignature читает первые байты файла и проверяет, что реальная
+// сигнатура соответствует заявленному content-type. После проверки возвращает
+// курсор файла в начало, чтобы последующая загрузка читала его целиком.
+func verifyImageSignature(file io.ReadSeeker, declaredType string) error {
+	head := make([]byte, 512)
+	n, err := io.ReadFull(file, head)
+	if err != nil && err != io.ErrUnexpectedEOF && err != io.EOF {
+		return fmt.Errorf("read file header: %w", err)
+	}
+	head = head[:n]
+	if _, err := file.Seek(0, io.SeekStart); err != nil {
+		return fmt.Errorf("seek file start: %w", err)
+	}
+
+	if !matchesImageType(head, declaredType) {
+		return errors.New("file signature mismatch")
+	}
+	return nil
+}
+
+// matchesImageType сверяет байтовую сигнатуру с заявленным типом.
+// net/http.DetectContentType умеет распознавать jpeg/png/webp, но не heic,
+// поэтому для heic проверяем ISO-BMFF бокс ftyp вручную.
+func matchesImageType(head []byte, declaredType string) bool {
+	if declaredType == "image/heic" {
+		return isHEIC(head)
+	}
+	return http.DetectContentType(head) == declaredType
+}
+
+// isHEIC проверяет, что файл — ISO Base Media File Format с HEIF-брендом.
+func isHEIC(head []byte) bool {
+	if len(head) < 12 || !bytes.Equal(head[4:8], []byte("ftyp")) {
+		return false
+	}
+	brand := string(head[8:12])
+	switch brand {
+	case "heic", "heix", "hevc", "heim", "heis", "mif1", "msf1":
+		return true
+	}
+	return false
+}
+
 func (h *UploadHandler) DeleteObjects(ctx context.Context, urls []string) error {
 	prefix := h.publicEndpoint + "/" + h.bucket + "/"
 	for _, fileURL := range urls {

+ 0 - 20
backend/internal/repository/bookings.go

@@ -6,7 +6,6 @@ import (
 	"errors"
 	"fmt"
 	"math"
-	"time"
 
 	"github.com/jackc/pgx/v5"
 	"github.com/jackc/pgx/v5/pgxpool"
@@ -63,19 +62,6 @@ func (r *BookingRepo) Create(ctx context.Context, b *models.Booking) error {
 	return tx.Commit(ctx)
 }
 
-func (r *BookingRepo) IsTimeSlotAvailable(ctx context.Context, placeID string, start, end time.Time) (bool, error) {
-	var count int
-	err := r.pool.QueryRow(ctx,
-		`SELECT COUNT(*) FROM bookings
-		 WHERE place_id = $1 AND status != 'cancelled'
-		 AND tsrange(start_time, end_time) && tsrange($2, $3)`,
-		placeID, start, end).Scan(&count)
-	if err != nil {
-		return false, err
-	}
-	return count == 0, nil
-}
-
 func (r *BookingRepo) GetByID(ctx context.Context, id string) (*models.Booking, error) {
 	row := r.pool.QueryRow(ctx,
 		`SELECT id, place_id, user_id, start_time, end_time, status, total_price, currency, comment,
@@ -120,12 +106,6 @@ func (r *BookingRepo) ListByPlace(ctx context.Context, placeID string) ([]*model
 	return scanBookings(rows)
 }
 
-func (r *BookingRepo) UpdateStatus(ctx context.Context, id, status string) error {
-	_, err := r.pool.Exec(ctx,
-		`UPDATE bookings SET status=$1, updated_at=now() WHERE id=$2`, status, id)
-	return err
-}
-
 func (r *BookingRepo) UpdateStatusIfPending(ctx context.Context, id, status string) (bool, error) {
 	tag, err := r.pool.Exec(ctx,
 		`UPDATE bookings SET status=$1, updated_at=now() WHERE id=$2 AND status='pending'`, status, id)

+ 4 - 3
frontend/src/lib/api.ts

@@ -129,7 +129,7 @@ export const api = {
     request<T>(path, { method: 'DELETE' }),
 }
 
-async function requestFormData<T>(path: string, formData: FormData): Promise<T> {
+async function requestFormData<T>(path: string, formData: FormData, isRetry = false): Promise<T> {
   const headers: HeadersInit = {
     ...(accessToken ? { Authorization: `Bearer ${accessToken}` } : {}),
   }
@@ -141,11 +141,12 @@ async function requestFormData<T>(path: string, formData: FormData): Promise<T>
     credentials: 'include',
   })
 
-  if (res.status === 401 && accessToken) {
+  // isRetry защищает от бесконечного цикла, если после refresh сервер снова отдаёт 401
+  if (res.status === 401 && !isRetry && accessToken) {
     try {
       const newToken = await refreshAccessToken()
       accessToken = newToken
-      return requestFormData<T>(path, formData)
+      return requestFormData<T>(path, formData, true)
     } catch {
       accessToken = null
       throw new ApiRequestError(401, 'Session expired', null)

+ 1 - 1
obsidian_data/Photoplaces_data/.obsidian/graph.json

@@ -17,6 +17,6 @@
   "repelStrength": 10,
   "linkStrength": 1,
   "linkDistance": 250,
-  "scale": 1,
+  "scale": 0.9999999999999986,
   "close": true
 }

+ 8 - 4
obsidian_data/Photoplaces_data/.obsidian/workspace.json

@@ -11,10 +11,14 @@
             "id": "3d01ec58e39ece45",
             "type": "leaf",
             "state": {
-              "type": "graph",
-              "state": {},
-              "icon": "lucide-git-fork",
-              "title": "Граф"
+              "type": "markdown",
+              "state": {
+                "file": "architecture-overview.md",
+                "mode": "source",
+                "source": false
+              },
+              "icon": "lucide-file",
+              "title": "architecture-overview"
             }
           }
         ]

+ 5 - 1
obsidian_data/Photoplaces_data/MOC-security-patterns.md

@@ -36,7 +36,7 @@ graph LR
 - **Rate limiter**: fail closed в production (503 при недоступности Redis)
 - **WebSocket**: проверка Origin, глобальный лимит 1000 соединений
 - **DB**: параметризованные запросы (pgx), soft delete через `deleted_at`
-- **File upload**: валидация content-type (jpeg, png, webp, heic), лимит 50MB
+- **File upload**: whitelist content-type (jpeg, png, webp, heic) + проверка magic-bytes ([[atomic-file-upload-magic-bytes]]), серверная генерация имени (uuid), лимит размера
 
 ### Антипаттерны (требуют исправления)
 
@@ -69,5 +69,9 @@ graph LR
 - [[atomic-error-swallowing-frontend]] — подавление ошибок
 - [[atomic-csp-hardening]] — CSP hardening
 - [[atomic-redis-rate-limiter-failopen-failclosed]] — fail-open/closed
+- [[atomic-file-upload-magic-bytes]] — проверка сигнатуры загружаемых файлов
+- [[atomic-sentinel-error-handler-mapping]] — маппинг sentinel-ошибок в HTTP-статусы
+- [[atomic-booking-status-transition-guard]] — условные переходы статуса
+- [[atomic-fetch-retry-infinite-loop]] — guard от рекурсии при refresh
 
 #security #MOC #backend #auth #production #anti-pattern

+ 25 - 0
obsidian_data/Photoplaces_data/atomic-booking-status-transition-guard.md

@@ -0,0 +1,25 @@
+## Переходы статуса брони должны быть условными (WHERE status=...)
+
+**Контекст:** `BookingHandler.Cancel` уже использовал `UpdateStatusIfPending` (атомарный `UPDATE ... WHERE status='pending'`), а `Confirm` вызывал безусловный `UpdateStatus(id, "confirmed")`. Это позволяло:
+- подтвердить уже **отменённую** бронь (`cancelled` → `confirmed`);
+- получить 200 при подтверждении несуществующего id (0 строк, но без ошибки).
+
+**Суть:** Изменение статуса — это переход в конечном автомате, а не «затирание поля». Допустимый переход нужно зашивать в `WHERE`, а число затронутых строк использовать как признак валидности перехода. Иначе появляются недопустимые состояния и race conditions.
+
+**Решение:** переиспользовать тот же условный апдейт и для подтверждения:
+
+```go
+ok, err := h.bookingRepo.UpdateStatusIfPending(r.Context(), id, "confirmed")
+if err != nil { /* 500 */ }
+if !ok {
+    writeError(w, http.StatusConflict, "booking cannot be confirmed in its current state", nil)
+    return
+}
+```
+
+Безусловный `BookingRepo.UpdateStatus` удалён как мёртвый и опасный код.
+
+**Связанные заметки:** [[atomic-refresh-token-race-condition]], [[backend-validation]]
+**Источник:** Code review 2026-06-30 (neyrogovnarik), fix в `handlers/bookings.go` + `repository/bookings.go`
+
+#golang #backend #state-machine #concurrency #bug

+ 25 - 0
obsidian_data/Photoplaces_data/atomic-fetch-retry-infinite-loop.md

@@ -0,0 +1,25 @@
+## Бесконечный цикл при повторе запроса после refresh-токена
+
+**Контекст:** В `frontend/src/lib/api.ts` функция `request<T>` имела параметр-флаг `isRetry`, защищающий от повторного входа в ветку обновления токена. А параллельная функция `requestFormData<T>` (загрузка файлов через multipart) проверяла только `res.status === 401 && accessToken`, без флага.
+
+**Суть:** Если после успешного refresh сервер **снова** отдаёт 401 (токен отозван/невалиден сразу), условие остаётся истинным → новый refresh → повторная загрузка → 401 → … Получается плотный цикл запросов к `/auth/refresh` и upload-эндпоинту (фактически self-DoS клиента и сервера).
+
+**Решение:** Прокинуть `isRetry` в `requestFormData`, как в `request`:
+
+```ts
+async function requestFormData<T>(path, formData, isRetry = false): Promise<T> {
+  // ...
+  if (res.status === 401 && !isRetry && accessToken) {
+    const newToken = await refreshAccessToken()
+    accessToken = newToken
+    return requestFormData<T>(path, formData, true) // только один повтор
+  }
+}
+```
+
+**Правило:** любой код «повтори запрос после обновления токена» обязан иметь однократный guard от рекурсии. Это должно быть единообразно во всех путях запроса (JSON и multipart).
+
+**Связанные заметки:** [[frontend-api-client]], [[atomic-api-contract-refresh-cookie]]
+**Источник:** Code review 2026-06-30 (neyrogovnarik), P1 fix в `lib/api.ts`
+
+#frontend #typescript #auth #best-practice #bug

+ 27 - 0
obsidian_data/Photoplaces_data/atomic-file-upload-magic-bytes.md

@@ -0,0 +1,27 @@
+## Проверять тип загружаемого файла по сигнатуре, а не по заголовку
+
+**Контекст:** `UploadHandler.UploadFile` валидировал только `header.Header.Get("Content-Type")` из multipart-части. Этот заголовок полностью контролируется клиентом, поэтому под видом `image/jpeg` можно было залить произвольный файл (HTML/JS/SVG/исполняемый контент) в общий бакет.
+
+**Суть:** Заявленный content-type — это утверждение клиента, а не факт. Доверять можно только реальным байтам файла. Проверяем сигнатуру по первым 512 байтам и затем возвращаем курсор в начало (`Seek(0,0)`), иначе загрузка прочитает усечённый файл.
+
+```go
+func verifyImageSignature(file io.ReadSeeker, declaredType string) error {
+    head := make([]byte, 512)
+    n, _ := io.ReadFull(file, head)
+    head = head[:n]
+    file.Seek(0, io.SeekStart) // вернуть курсор для PutObject
+    if !matchesImageType(head, declaredType) {
+        return errors.New("file signature mismatch")
+    }
+    return nil
+}
+```
+
+**Нюанс:** `http.DetectContentType` распознаёт jpeg/png/webp, но **не heic**. Для HEIC проверяем ISO-BMFF бокс вручную: байты `4:8 == "ftyp"` и бренд из набора `heic/heix/hevc/mif1/...`.
+
+**Правило:** любой пользовательский upload → whitelist типов + проверка magic-bytes + генерация имени на сервере (uuid+ext), никогда не доверять имени/типу от клиента.
+
+**Связанные заметки:** [[MOC-security-patterns]], [[backend-validation]]
+**Источник:** Code review 2026-06-30 (neyrogovnarik), security-fix в `handlers/upload.go`
+
+#golang #backend #security #file-upload #best-practice

+ 26 - 0
obsidian_data/Photoplaces_data/atomic-sentinel-error-handler-mapping.md

@@ -0,0 +1,26 @@
+## Sentinel-ошибку мало объявить — её нужно смапить в хендлере
+
+**Контекст:** В `services/places.go` корректно объявлен sentinel `ErrNotYourPlace` и возвращается через `%w`. Но хендлер `PlaceHandler.Update` мапил **любую** ошибку `Update` в `500 internal server error`. В итоге чужой пользователь при `PATCH /places/{id}` получал 500 вместо 403.
+
+**Суть:** Sentinel-error даёт пользу только если вызывающий слой действительно делает `errors.Is`. Объявление ошибки и её маппинг в HTTP-статус — две разные обязанности; пропуск второй превращает ожидаемую бизнес-ситуацию (нет прав) в «внутреннюю ошибку» и засоряет логи 500-ками.
+
+**Решение:**
+
+```go
+place, err := h.placeSvc.Update(r.Context(), input, isModerator)
+if err != nil {
+    if errors.Is(err, services.ErrNotYourPlace) {
+        writeError(w, http.StatusForbidden, "not your place", nil)
+        return
+    }
+    writeError(w, http.StatusInternalServerError, "failed to update place", err)
+    return
+}
+```
+
+**Правило:** при добавлении sentinel-ошибки сразу проверь все хендлеры, которые её могут получить — на каждый осмысленный sentinel должен быть свой статус (403/404/409), а не общий 500.
+
+**Связанные заметки:** [[atomic-sentinel-errors-go]], [[backend-auth-security]]
+**Источник:** Code review 2026-06-30 (neyrogovnarik), P1 fix в `handlers/places.go`
+
+#golang #backend #error-handling #http #bug