houseassassin/remnawave-bedolaga-telegram-bot
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: address review issues in PR #2829 webhook intentional deletion guard

Browse Source 
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.
This commit is contained in:
Fringg
2026-04-02 06:34:35 +03:00
parent 6f6b9fa039
commit 977950b97f
3 changed files with 56 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/services/remnawave_webhook_service.py
+25 -15
View File
@@ -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:
app/services/user_service.py
+10 -5
View File
@@ -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:
tests/webserver/test_remnawave_webhook.py
+21 -12
View File
@@ -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