fix(tradein): atomic conditional quota increment — close TOCTOU (#747) (#749)

Co-authored-by: bot-backend <bot-backend@gendsgn.local>
Co-committed-by: bot-backend <bot-backend@gendsgn.local>

tradein-mvp/backend/app/api/v1/trade_in.py

@@ -174,7 +174,11 @@ async def estimate(
             status_code=503,
             detail="estimate temporarily unavailable — try again shortly",
         ) from None
-    account_quota.increment(db, x_authenticated_user)
+    # #747: атомарно-условный инкремент — источник истины по лимиту. check_and_raise
+    # выше остаётся быстрым pre-check (429 до дорогой оценки), но финальное решение
+    # тут: при гонке двух /estimate на used=MONTHLY_LIMIT-1 второй получит False.
+    if not account_quota.increment(db, x_authenticated_user):
+        raise HTTPException(status_code=429, detail=account_quota.LIMIT_EXHAUSTED_MESSAGE)
     return result
 
 

tradein-mvp/backend/app/services/account_quota.py

@@ -125,25 +125,30 @@ def check_and_raise(db: Session, username: str | None) -> None:
     if used >= MONTHLY_LIMIT:
         logger.warning(
             "quota exhausted: username=%r period=%s used=%d limit=%d",
-            username, period, used, MONTHLY_LIMIT,
+            username,
+            period,
+            used,
+            MONTHLY_LIMIT,
         )
         raise HTTPException(status_code=429, detail=LIMIT_EXHAUSTED_MESSAGE)
 
 
-def increment(db: Session, username: str | None) -> None:
-    """Инкрементирует счётчик успешных оценок для пользователя.
+def increment(db: Session, username: str | None) -> bool:
+    """Атомарно-условный инкремент счётчика успешных оценок (#747).
 
-    Если username is None или пользователь unlimited → no-op.
-    UPSERT: INSERT ... ON CONFLICT DO UPDATE SET used = used + 1.
+    Возвращает True если инкремент успешен; False если лимит исчерпан.
+    None / unlimited → True (no-op success).
+
+    Защита от TOCTOU: предикат `WHERE used < :lim` применяется к ветке DO UPDATE —
+    два параллельных запроса при used=MONTHLY_LIMIT-1 не могут оба инкрементировать
+    (второй упрётся в WHERE → RETURNING пуст → False). Свежая вставка (used=1) НЕ
+    задевается WHERE (он только для DO UPDATE), поэтому первая оценка месяца проходит.
     """
-    if username is None:
-        return
-
-    if is_unlimited(username):
-        return
+    if username is None or is_unlimited(username):
+        return True
 
     period = current_period()
-    db.execute(
+    row = db.execute(
         text(
             """
             INSERT INTO account_estimate_usage (username, period_month, used, updated_at)
@@ -152,11 +157,21 @@ def increment(db: Session, username: str | None) -> None:
             DO UPDATE SET
                 used = account_estimate_usage.used + 1,
                 updated_at = NOW()
+            WHERE account_estimate_usage.used < :lim
+            RETURNING used
             """
         ),
-        {"u": username, "p": period},
-    )
+        {"u": username, "p": period, "lim": MONTHLY_LIMIT},
+    ).fetchone()
     db.commit()
-    logger.debug(
-        "quota incremented: username=%r period=%s", username, period
-    )
+    ok = row is not None
+    if ok:
+        logger.debug("quota incremented: username=%r period=%s used=%s", username, period, row[0])
+    else:
+        logger.warning(
+            "quota increment refused (atomic, #747): username=%r period=%s limit=%d",
+            username,
+            period,
+            MONTHLY_LIMIT,
+        )
+    return ok

tradein-mvp/backend/tests/test_account_quota.py

@@ -217,7 +217,7 @@ def test_get_status_admin() -> None:
     status = get_status(db, "admin")
     assert status["unlimited"] is True
     assert status["remaining"] == MONTHLY_LIMIT
-    assert status["used"] == 7   # фактический used из БД
+    assert status["used"] == 7  # фактический used из БД
 
 
 def test_get_status_pilot_with_used() -> None:
@@ -389,3 +389,86 @@ def test_estimate_no_header_not_blocked(estimate_app_exhausted: FastAPI) -> None
         },
     )
     assert resp.status_code != 429
+
+
+# ---------------------------------------------------------------------------
+# (f) #747 — атомарно-условный increment (TOCTOU fix)
+# ---------------------------------------------------------------------------
+
+
+class _Row:
+    """Row stand-in поддерживающий и .used, и индексный доступ row[0]."""
+
+    def __init__(self, used: int) -> None:
+        self.used = used
+        self._t = (used,)
+
+    def __getitem__(self, i: int) -> int:
+        return self._t[i]
+
+
+class _Result:
+    def __init__(self, row: _Row | None) -> None:
+        self._row = row
+
+    def fetchone(self) -> _Row | None:
+        return self._row
+
+
+class _AtomicQuotaFakeDB:
+    """In-memory fake, моделирующий атомарный conditional UPSERT (#747).
+
+    Воспроизводит семантику `INSERT ... ON CONFLICT DO UPDATE SET used=used+1
+    WHERE used < :lim RETURNING used`:
+      - строки ещё нет (used=None) → INSERT used=1, RETURNING row (WHERE не для INSERT);
+      - used < lim → used+=1, RETURNING row;
+      - used >= lim → конфликтная строка НЕ обновлена, RETURNING пуст (None).
+    """
+
+    def __init__(self, *, used: int | None) -> None:
+        self.used = used
+        self.commits = 0
+
+    def execute(self, _stmt: object, params: dict) -> _Result:
+        lim = params["lim"]
+        if self.used is None:
+            self.used = 1
+        elif self.used < lim:
+            self.used += 1
+        else:
+            return _Result(None)
+        return _Result(_Row(self.used))
+
+    def commit(self) -> None:
+        self.commits += 1
+
+
+def test_increment_atomic_refuses_second_at_limit() -> None:
+    """used=MONTHLY_LIMIT-1: первый increment True (used→limit), второй False —
+    used НЕ растёт за лимит (TOCTOU-гонка двух /estimate не превышает лимит)."""
+    db = _AtomicQuotaFakeDB(used=MONTHLY_LIMIT - 1)
+    assert increment(db, "user1") is True
+    assert db.used == MONTHLY_LIMIT
+    assert increment(db, "user1") is False
+    assert db.used == MONTHLY_LIMIT  # не MONTHLY_LIMIT + 1
+
+
+def test_increment_atomic_fresh_insert_succeeds() -> None:
+    """Свежая вставка (строки ещё нет) НЕ блокируется WHERE → True, used=1."""
+    db = _AtomicQuotaFakeDB(used=None)
+    assert increment(db, "user1") is True
+    assert db.used == 1
+
+
+def test_increment_atomic_returns_bool() -> None:
+    """increment возвращает bool (контракт #747), а не None."""
+    db = _AtomicQuotaFakeDB(used=0)
+    assert increment(db, "user1") is True
+
+
+def test_increment_atomic_unlimited_noop_returns_true() -> None:
+    """unlimited (admin) → True без db.execute/commit."""
+    db = _AtomicQuotaFakeDB(used=MONTHLY_LIMIT)
+    assert increment(db, "admin") is True
+    assert db.commits == 0
+    assert db.used == MONTHLY_LIMIT  # не тронут