From 977950b97f07eecf089152d3f4e678fda373e1e6 Mon Sep 17 00:00:00 2001 From: Fringg Date: Thu, 2 Apr 2026 06:34:35 +0300 Subject: [PATCH] fix: address review issues in PR #2829 webhook intentional deletion guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 5 issues found by review agents and fixed: 1. Intentional deletion guard was at top of process_event, skipping ALL cleanup (subscription URLs, server counts, expired marking). Moved check into _handle_user_deleted so cleanup still runs but re-creation is suppressed via subscription_still_valid=False. 2. Variable shadowing: telegram_id loop var in any() generator shadowed the outer telegram_id local. Renamed to tid/uid. 3. No hard cap on in-memory dicts: added _MAX_INTENTIONAL_ENTRIES=10000 with early return in mark_intentional_panel_deletion. 4. mark_intentional_panel_deletion called inside for-loop with single UUID — race window if webhook from first delete arrives before second UUID is marked. Moved to before the loop with all UUIDs. 5. Tests: @pytest.mark.anyio → @pytest.mark.anyio('asyncio') for consistency. Replaced process_event(db=None) test with direct mark+detect unit tests + hard cap test. --- app/services/remnawave_webhook_service.py | 40 ++++++++++++++--------- app/services/user_service.py | 15 ++++++--- tests/webserver/test_remnawave_webhook.py | 33 ++++++++++++------- 3 files changed, 56 insertions(+), 32 deletions(-) diff --git a/app/services/remnawave_webhook_service.py b/app/services/remnawave_webhook_service.py index b5aba2c1..d5d79df4 100644 --- a/app/services/remnawave_webhook_service.py +++ b/app/services/remnawave_webhook_service.py @@ -125,14 +125,14 @@ _ADMIN_NODE_CONNECTION_EVENTS = frozenset({'node.connection_lost', 'node.connect class RemnaWaveWebhookService: """Processes incoming webhooks from RemnaWave backend.""" - # In-memory guard: tracks recent panel recreations per subscription_id. - # Prevents unbounded user.deleted → recreate → user.deleted loops. - # Key: subscription_id, Value: datetime of last recreation attempt. + # NOTE: In-memory guards. Only correct with a single-worker deployment. + # For multi-worker setups, move to Redis or another shared store. _recent_recreations: dict[int, datetime] = {} _RECREATION_GUARD_SECONDS: int = 120 # 2-minute cooldown _intentional_panel_deletions_by_uuid: dict[str, datetime] = {} _intentional_panel_deletions_by_telegram_id: dict[int, datetime] = {} _INTENTIONAL_PANEL_DELETION_GUARD_SECONDS: int = 300 + _MAX_INTENTIONAL_ENTRIES: int = 10_000 def __init__(self, bot: Bot) -> None: self.bot = bot @@ -203,6 +203,12 @@ class RemnaWaveWebhookService: telegram_id: int | None = None, ) -> None: cls._prune_intentional_panel_deletions() + + total = len(cls._intentional_panel_deletions_by_uuid) + len(cls._intentional_panel_deletions_by_telegram_id) + if total >= cls._MAX_INTENTIONAL_ENTRIES: + logger.warning('Intentional deletion guard at capacity, skipping', total=total) + return + now = datetime.now(UTC) for panel_uuid in panel_uuids or []: @@ -210,7 +216,7 @@ class RemnaWaveWebhookService: if normalized: cls._intentional_panel_deletions_by_uuid[normalized] = now - if telegram_id: + if telegram_id is not None: cls._intentional_panel_deletions_by_telegram_id[int(telegram_id)] = now @classmethod @@ -244,8 +250,8 @@ class RemnaWaveWebhookService: except (TypeError, ValueError): pass - return any(uuid in cls._intentional_panel_deletions_by_uuid for uuid in candidate_uuids) or any( - telegram_id in cls._intentional_panel_deletions_by_telegram_id for telegram_id in candidate_telegram_ids + return any(uid in cls._intentional_panel_deletions_by_uuid for uid in candidate_uuids) or any( + tid in cls._intentional_panel_deletions_by_telegram_id for tid in candidate_telegram_ids ) async def process_event(self, db: AsyncSession | None, event_name: str, data: dict) -> bool: @@ -254,14 +260,6 @@ class RemnaWaveWebhookService: Returns True if the event was processed, False if skipped/unknown. db may be None for admin events that don't require database access. """ - if event_name == 'user.deleted' and self._is_intentional_panel_deletion_event(data): - logger.info( - 'RemnaWave webhook: skipping intentional admin-triggered user.deleted event', - telegram_id=data.get('telegramId'), - remnawave_uuid=data.get('uuid') or data.get('userUuid'), - ) - return True - # Check admin-scoped handlers (no DB needed) if event_name in self._admin_handlers: return await self._process_admin_event(event_name, data) @@ -1047,10 +1045,22 @@ class RemnaWaveWebhookService: logger.error('Webhook: user not found after rollback', user_id=user_id) return + # Intentional admin deletion: cleanup runs (fields cleared above), but skip re-creation + is_intentional = self._is_intentional_panel_deletion_event(data) + if is_intentional: + logger.info( + 'Webhook user.deleted: intentional admin deletion, cleanup done, skipping re-creation', + sub_id=sub_id, + user_id=user_id, + ) + # Check if subscription has a future end_date — likely a spurious user.deleted # (e.g., RemnaWave sends user.deleted during panel resync when modifying another user) subscription_still_valid = ( - subscription is not None and subscription.end_date is not None and subscription.end_date > datetime.now(UTC) + not is_intentional + and subscription is not None + and subscription.end_date is not None + and subscription.end_date > datetime.now(UTC) ) if subscription: diff --git a/app/services/user_service.py b/app/services/user_service.py index a94c0097..7e1f9a33 100644 --- a/app/services/user_service.py +++ b/app/services/user_service.py @@ -817,18 +817,23 @@ class UserService: else: delete_mode = 'delete' if force_panel_delete else settings.get_remnawave_user_delete_mode() + # Помечаем ВСЕ UUID до цикла, чтобы webhook от первого удаления + # не пришёл раньше чем помечены остальные + if delete_mode == 'delete': + from app.services.remnawave_webhook_service import RemnaWaveWebhookService + + RemnaWaveWebhookService.mark_intentional_panel_deletion( + panel_uuids=panel_uuids, + telegram_id=int(user.telegram_id) if user.telegram_id else None, + ) + for panel_uuid in panel_uuids: try: from app.services.remnawave_service import RemnaWaveService - from app.services.remnawave_webhook_service import RemnaWaveWebhookService remnawave_service = RemnaWaveService() if delete_mode == 'delete': - RemnaWaveWebhookService.mark_intentional_panel_deletion( - panel_uuids=[panel_uuid], - telegram_id=int(user.telegram_id) if user.telegram_id else None, - ) async with remnawave_service.get_api_client() as api: delete_success = await api.delete_user(panel_uuid) if delete_success: diff --git a/tests/webserver/test_remnawave_webhook.py b/tests/webserver/test_remnawave_webhook.py index 33bdef5e..be5ab4f3 100644 --- a/tests/webserver/test_remnawave_webhook.py +++ b/tests/webserver/test_remnawave_webhook.py @@ -54,7 +54,7 @@ def _signature(body: bytes) -> str: return hmac.new(secret.encode('utf-8'), body, hashlib.sha256).hexdigest() -@pytest.mark.anyio +@pytest.mark.anyio('asyncio') async def test_remnawave_webhook_accepts_event_without_scope(monkeypatch: pytest.MonkeyPatch) -> None: bot = AsyncMock() process_event = AsyncMock(return_value=True) @@ -90,7 +90,7 @@ async def test_remnawave_webhook_accepts_event_without_scope(monkeypatch: pytest process_event.assert_awaited_once_with(None, 'user.modified', {'uuid': 'user-123'}) -@pytest.mark.anyio +@pytest.mark.anyio('asyncio') async def test_remnawave_webhook_rejects_payload_without_event() -> None: bot = AsyncMock() payload = {'data': {'uuid': 'user-123'}} @@ -111,20 +111,29 @@ async def test_remnawave_webhook_rejects_payload_without_event() -> None: assert json.loads(response.body.decode('utf-8')) == {'status': 'error', 'reason': 'missing_event'} -@pytest.mark.anyio -async def test_process_event_skips_intentional_admin_user_deleted() -> None: - bot = AsyncMock() - service = RemnaWaveWebhookService(bot) - +def test_intentional_panel_deletion_guard_marks_and_detects() -> None: + """Verify that mark + is_intentional round-trip works correctly.""" RemnaWaveWebhookService.mark_intentional_panel_deletion( panel_uuids=['panel-user-123'], telegram_id=8368498066, ) - processed = await service.process_event( - None, - 'user.deleted', - {'uuid': 'panel-user-123', 'telegramId': 8368498066}, + assert RemnaWaveWebhookService._is_intentional_panel_deletion_event( + {'uuid': 'panel-user-123', 'telegramId': 8368498066} ) - assert processed is True + # Unknown UUID should not match + assert not RemnaWaveWebhookService._is_intentional_panel_deletion_event( + {'uuid': 'unknown-uuid', 'telegramId': 99999} + ) + + +def test_intentional_panel_deletion_guard_respects_hard_cap(monkeypatch: pytest.MonkeyPatch) -> None: + """Verify that the guard stops accepting entries after hitting the cap.""" + monkeypatch.setattr(RemnaWaveWebhookService, '_MAX_INTENTIONAL_ENTRIES', 3) + + RemnaWaveWebhookService.mark_intentional_panel_deletion(panel_uuids=['a', 'b', 'c']) + # 3 entries — at capacity + RemnaWaveWebhookService.mark_intentional_panel_deletion(panel_uuids=['d']) + # 'd' should NOT be stored (cap reached) + assert 'd' not in RemnaWaveWebhookService._intentional_panel_deletions_by_uuid