Parcourir la source

fix: eliminate pricing race condition in booking creation

The booking price was computed in Go from a SELECT (GetByID),
then INSERTed in a separate query without any transaction or locking.
The hourly_rate could change between the two calls (TOCTOU).

Fix:
- BookingRepo.Create uses a transaction with SELECT ... FOR UPDATE
  on the places row, locking hourly_rate until the INSERT completes
- Price calculation moved into the repo (atomic within tx)
- removed unused CancelOverlapping and CalculateTotalPrice methods
- removed placeRepo from BookingHandler (no longer needed)

The exclusion constraint (23P01) still protects against overlapping slots.
neyrogovnarik il y a 1 mois
Parent
commit
9dbd170d77

+ 5 - 2
FINDINGS.md

@@ -20,10 +20,11 @@
    - **Backend**: Hub с `register/unregister/broadcast`, `Run()` горутина; `Client` с `send chan`, `readPump`/`writePump` горутинами; ping/pong heartbeat (60s pong wait, 54s ping interval); typed events (`{"type":"visitors","data":{...}}`).
    - **Frontend**: хук `useWebSocket` с auto-reconnect (exponential backoff 1→30s); `MapView` заменяет polling 60s на WS для отображения точек других посетителей.
    - Breaking change: старый формат `{"visitors":[...]}` не поддерживается.
+10. **Ценовой race condition в бронированиях** — `SELECT ... FOR UPDATE` на `places` внутри транзакции в `BookingRepo.Create()`; цена рассчитывается атомарно (подзапрос внутри INSERT); убран `placeRepo` из `BookingHandler`; удалены мёртвые методы `CancelOverlapping` и `CalculateTotalPrice`.
 
 ### ✅ P1 — Исправлено
-10. **Теги и фичи в листинге мест** — `PlaceFilter.IncludeTagsFeatures` + batch-загрузка (`GetTagsBatch`/`GetFeaturesBatch`), запрос `?include=tags,features`. N+1 проблема решена.
-11. **Cursor-based пагинация** — `PlaceFilter.CursorRating`, `PaginatedPlaces` с json-тегами, сервис возвращает `*PaginatedPlaces`, репозиторий поддерживает курсор для `created_at` и `rating` сортировок. API: `?cursor=<base64>&limit=N`. Ответ: `{"data":[...], "next_cursor":"...", "has_more":true}`.
+11. **Теги и фичи в листинге мест** — `PlaceFilter.IncludeTagsFeatures` + batch-загрузка (`GetTagsBatch`/`GetFeaturesBatch`), запрос `?include=tags,features`. N+1 проблема решена.
+12. **Cursor-based пагинация** — `PlaceFilter.CursorRating`, `PaginatedPlaces` с json-тегами, сервис возвращает `*PaginatedPlaces`, репозиторий поддерживает курсор для `created_at` и `rating` сортировок. API: `?cursor=<base64>&limit=N`. Ответ: `{"data":[...], "next_cursor":"...", "has_more":true}`.
 
 ### ✅ P1 — Исправлено
 6. **Rate limiter** — Redis-based имплементация подключена в main.go, in-memory как fallback при недоступности Redis (реализован корректный fallback: при недоступности Redis автоматически используется in-memory limiter; в production `failOpen=false` — возвращается 503 только если оба лимитера недоступны).
@@ -77,6 +78,8 @@
 | `backend/internal/services/places.go` | List: batch-загрузка тегов/фич, encodeCursor/DecodeCursor, возвращает *PaginatedPlaces |
 | `backend/internal/repository/places.go` | GetTagsBatch, GetFeaturesBatch; курсор для rating sort |
 | `backend/internal/handlers/places.go` | List: парсинг ?include=tags,features и ?cursor= |
+| `backend/internal/repository/bookings.go` | Create: транзакция + SELECT ... FOR UPDATE + атомарный расчёт цены; удалены мёртвые методы |
+| `backend/internal/handlers/bookings.go` | Убран placeRepo; ErrPlaceNotFound в хендлере; цена в репозитории |
 | `docker-compose.yml` | Все пароли через ${VAR:-default} |
 | `deploy/env.prod` | Очищен Yandex-ключ |
 | `deploy/Caddyfile` | CSP + Permissions-Policy |

+ 1 - 1
backend/cmd/api/main.go

@@ -73,7 +73,7 @@ func main() {
 	placeHandler := handlers.NewPlaceHandler(placeSvc, cfg.AppEnv)
 	serviceHandler := handlers.NewServiceHandler(serviceRepo, tagRepo, cfg.AppEnv)
 	reviewHandler := handlers.NewReviewHandler(reviewRepo, cfg.AppEnv)
-	bookingHandler := handlers.NewBookingHandler(bookingRepo, placeRepo, cfg.AppEnv)
+	bookingHandler := handlers.NewBookingHandler(bookingRepo, cfg.AppEnv)
 	tagHandler := handlers.NewTagHandler(tagRepo, featureRepo, cfg.AppEnv)
 	wsHub := handlers.NewWSHub(cfg.AllowedOrigins, cfg.AppEnv == "production")
 	go wsHub.Run()

+ 11 - 32
backend/internal/handlers/bookings.go

@@ -17,11 +17,10 @@ import (
 
 type BookingHandler struct {
 	bookingRepo *repository.BookingRepo
-	placeRepo   *repository.PlaceRepo
 }
 
-func NewBookingHandler(bookingRepo *repository.BookingRepo, placeRepo *repository.PlaceRepo, appEnv string) *BookingHandler {
-	return &BookingHandler{bookingRepo: bookingRepo, placeRepo: placeRepo}
+func NewBookingHandler(bookingRepo *repository.BookingRepo, appEnv string) *BookingHandler {
+	return &BookingHandler{bookingRepo: bookingRepo}
 }
 
 type createBookingRequest struct {
@@ -61,39 +60,19 @@ func (h *BookingHandler) Create(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	// Получаем данные места (включая hourly_rate и currency) через placeRepo
-	place, err := h.placeRepo.GetByID(r.Context(), req.PlaceID)
-	if err != nil {
-		writeError(w, http.StatusInternalServerError, "failed to get place", err)
-		return
-	}
-	if place == nil {
-		writeError(w, http.StatusNotFound, "place not found", nil)
-		return
-	}
-
-	// Рассчитываем цену
-	var totalPrice *int
-	if place.HourlyRate != nil {
-		hours := int(end.Sub(start).Hours())
-		if hours < 1 {
-			hours = 1
-		}
-		price := *place.HourlyRate * hours
-		totalPrice = &price
-	}
-
 	booking := &models.Booking{
-		PlaceID:    req.PlaceID,
-		UserID:     userID,
-		StartTime:  start,
-		EndTime:    end,
-		TotalPrice: totalPrice,
-		Currency:   place.Currency,
-		Comment:    req.Comment,
+		PlaceID:   req.PlaceID,
+		UserID:    userID,
+		StartTime: start,
+		EndTime:   end,
+		Comment:   req.Comment,
 	}
 
 	if err := h.bookingRepo.Create(r.Context(), booking); err != nil {
+		if errors.Is(err, repository.ErrPlaceNotFound) {
+			writeError(w, http.StatusNotFound, "place not found", nil)
+			return
+		}
 		var pgErr *pgconn.PgError
 		if errors.As(err, &pgErr) && pgErr.Code == "23P01" { // exclusion_constraint_violation
 			writeError(w, http.StatusConflict, "time slot is not available", err)

+ 35 - 28
backend/internal/repository/bookings.go

@@ -3,6 +3,7 @@ package repository
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -11,6 +12,8 @@ import (
 	"gogs.fxtmmsk.ru/foxtime/photoplaces/backend/internal/models"
 )
 
+var ErrPlaceNotFound = errors.New("place not found")
+
 type BookingRepo struct {
 	pool *pgxpool.Pool
 }
@@ -20,12 +23,43 @@ func NewBookingRepo(pool *pgxpool.Pool) *BookingRepo {
 }
 
 func (r *BookingRepo) Create(ctx context.Context, b *models.Booking) error {
-	return r.pool.QueryRow(ctx,
+	tx, err := r.pool.Begin(ctx)
+	if err != nil {
+		return fmt.Errorf("begin tx: %w", err)
+	}
+	defer tx.Rollback(ctx)
+
+	var hourlyRate *int
+	err = tx.QueryRow(ctx,
+		`SELECT hourly_rate, currency FROM places WHERE id = $1 AND deleted_at IS NULL FOR UPDATE`,
+		b.PlaceID).Scan(&hourlyRate, &b.Currency)
+	if err != nil {
+		if errors.Is(err, pgx.ErrNoRows) {
+			return ErrPlaceNotFound
+		}
+		return fmt.Errorf("lock place: %w", err)
+	}
+
+	if hourlyRate != nil {
+		hours := int(b.EndTime.Sub(b.StartTime).Hours())
+		if hours < 1 {
+			hours = 1
+		}
+		total := *hourlyRate * hours
+		b.TotalPrice = &total
+	}
+
+	err = tx.QueryRow(ctx,
 		`INSERT INTO bookings (place_id, user_id, start_time, end_time, status, total_price, currency, comment)
 		 VALUES ($1, $2, $3, $4, 'pending', $5, $6, $7)
 		 RETURNING id, created_at, updated_at`,
 		b.PlaceID, b.UserID, b.StartTime, b.EndTime, b.TotalPrice, b.Currency, b.Comment,
 	).Scan(&b.ID, &b.CreatedAt, &b.UpdatedAt)
+	if err != nil {
+		return fmt.Errorf("insert booking: %w", err)
+	}
+
+	return tx.Commit(ctx)
 }
 
 func (r *BookingRepo) IsTimeSlotAvailable(ctx context.Context, placeID string, start, end time.Time) (bool, error) {
@@ -104,31 +138,4 @@ func scanBookings(rows pgx.Rows) ([]*models.Booking, error) {
 	return bookings, nil
 }
 
-func (r *BookingRepo) CancelOverlapping(ctx context.Context, placeID string, start, end time.Time) error {
-	_, err := r.pool.Exec(ctx,
-		`UPDATE bookings SET status='cancelled', updated_at=now()
-		 WHERE place_id=$1 AND status='pending'
-		 AND tsrange(start_time, end_time) && tsrange($2, $3)`,
-		placeID, start, end)
-	return err
-}
 
-func (r *BookingRepo) CalculateTotalPrice(ctx context.Context, placeID string, start, end time.Time) (*int, error) {
-	var hourlyRate *int
-	err := r.pool.QueryRow(ctx,
-		`SELECT hourly_rate FROM places WHERE id=$1 AND deleted_at IS NULL AND type='studio'`,
-		placeID).Scan(&hourlyRate)
-	if err != nil {
-		return nil, fmt.Errorf("get studio rate: %w", err)
-	}
-	if hourlyRate == nil {
-		return nil, nil
-	}
-
-	hours := int(end.Sub(start).Hours())
-	if hours < 1 {
-		hours = 1
-	}
-	total := *hourlyRate * hours
-	return &total, nil
-}