From a74591d7ccf24a890449b77f5921fbe340375384 Mon Sep 17 00:00:00 2001 From: bot-backend Date: Sat, 30 May 2026 15:52:34 +0000 Subject: [PATCH] =?UTF-8?q?fix(tradein):=20atomic=20conditional=20quota=20?= =?UTF-8?q?increment=20=E2=80=94=20close=20TOCTOU=20(#747)=20(#749)=20Co-a?= =?UTF-8?q?uthored-by:=20bot-backend=20=20Co-co?= =?UTF-8?q?mmitted-by:=20bot-backend=20?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tradein-mvp/backend/app/api/v1/trade_in.py | 6 +- .../backend/app/services/account_quota.py | 47 ++++++---- .../backend/tests/test_account_quota.py | 85 ++++++++++++++++++- 3 files changed, 120 insertions(+), 18 deletions(-) diff --git a/tradein-mvp/backend/app/api/v1/trade_in.py b/tradein-mvp/backend/app/api/v1/trade_in.py index cb358668..27319762 100644 --- a/tradein-mvp/backend/app/api/v1/trade_in.py +++ b/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 diff --git a/tradein-mvp/backend/app/services/account_quota.py b/tradein-mvp/backend/app/services/account_quota.py index 70354e2e..5dbb87c0 100644 --- a/tradein-mvp/backend/app/services/account_quota.py +++ b/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 diff --git a/tradein-mvp/backend/tests/test_account_quota.py b/tradein-mvp/backend/tests/test_account_quota.py index 732adda0..33570ed0 100644 --- a/tradein-mvp/backend/tests/test_account_quota.py +++ b/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 # не тронут