fix: Resolve critical pairing system issues

- Fix callback secret caching with @lru_cache to prevent verification failures
- Add local InlineKeyboardMarkup import to fix Telegram NameError
- Fix requester binding by including user_id in signed callback payloads
- Update policy routing to handle empty allowlist correctly across all platforms
- Improve Discord reply method with proper channel/user/fetch fallback order
- Add config validation with __post_init__ to fail-fast on invalid policies
- Fix 'allow' policy to avoid invalid verify_and_pair call
- Ensure denied codes are consumed to prevent reuse
- Update test to use real signed callback path instead of bypassing verification

Resolves CodeRabbit's 9 critical/major architectural issues.

Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>

Author: praisonai-triage-agent[bot]

src/praisonai-agents/praisonaiagents/bots/config.py

@@ -82,6 +82,12 @@ class BotConfig:
 
     # Owner user ID for pairing approvals (platform-specific format)
     owner_user_id: Optional[str] = None
+
+    def __post_init__(self) -> None:
+        if self.unknown_user_policy not in {"deny", "pair", "allow"}:
+            raise ValueError(
+                f"unknown_user_policy must be one of: deny, pair, allow. Got: {self.unknown_user_policy}"
+            )
 
     metadata: Dict[str, Any] = field(default_factory=dict)
 

src/praisonai/praisonai/bots/_pairing_ui.py

@@ -9,18 +9,20 @@
 import hmac
 import json
 import logging
+from functools import lru_cache
 from typing import Any, Dict, Optional
 
 from praisonaiagents.bots.pairing_types import PairingApprovalResult
 
 logger = logging.getLogger(__name__)
 
 
+@lru_cache(maxsize=1)
 def _get_callback_secret() -> str:
     """Get HMAC secret for callback payload verification."""
     import os
     import secrets
-    return os.environ.get("PRAISONAI_CALLBACK_SECRET", "") or secrets.token_hex(16)
+    return os.environ.get("PRAISONAI_CALLBACK_SECRET", "") or secrets.token_hex(32)
 
 
 class PairingUIBuilder:
@@ -29,8 +31,8 @@ class PairingUIBuilder:
     @staticmethod
     def create_telegram_keyboard(user_name: str, code: str, channel: str, user_id: str) -> Dict[str, Any]:
         """Create Telegram inline keyboard for approval."""
-        approve_data = f"pair:approve:{channel}:{code}"
-        deny_data = f"pair:deny:{channel}:{code}"
+        approve_data = f"pair:approve:{channel}:{user_id}:{code}"
+        deny_data = f"pair:deny:{channel}:{user_id}:{code}"
 
         # Add HMAC signature to prevent tampering
         secret = _get_callback_secret().encode()
@@ -55,8 +57,8 @@ def create_telegram_keyboard(user_name: str, code: str, channel: str, user_id: s
     @staticmethod
     def create_discord_components(user_name: str, code: str, channel: str, user_id: str) -> list:
         """Create Discord button components for approval."""
-        approve_id = f"pair:approve:{channel}:{code}"
-        deny_id = f"pair:deny:{channel}:{code}"
+        approve_id = f"pair:approve:{channel}:{user_id}:{code}"
+        deny_id = f"pair:deny:{channel}:{user_id}:{code}"
 
         # Add HMAC signature
         secret = _get_callback_secret().encode()
@@ -86,8 +88,8 @@ def create_discord_components(user_name: str, code: str, channel: str, user_id:
     @staticmethod
     def create_slack_blocks(user_name: str, code: str, channel: str, user_id: str) -> list:
         """Create Slack block kit for approval."""
-        approve_value = f"pair:approve:{channel}:{code}"
-        deny_value = f"pair:deny:{channel}:{code}"
+        approve_value = f"pair:approve:{channel}:{user_id}:{code}"
+        deny_value = f"pair:deny:{channel}:{user_id}:{code}"
 
         # Add HMAC signature
         secret = _get_callback_secret().encode()
@@ -140,23 +142,24 @@ def parse_and_verify_callback(self, callback_data: str) -> Optional[Dict[str, st
         """Parse and verify callback data from button press.
         
         Args:
-            callback_data: Raw callback data (e.g., "pair:approve:telegram:abc123:sig")
+            callback_data: Raw callback data (e.g., "pair:approve:telegram:user123:abc123:sig")
             
         Returns:
             Parsed data dict or None if invalid
         """
         try:
             parts = callback_data.split(":")
-            if len(parts) < 5 or parts[0] != "pair":
+            if len(parts) < 6 or parts[0] != "pair":
                 return None
 
-            action = parts[1]  # approve/deny
-            channel = parts[2]  # telegram/discord/slack
-            code = parts[3]     # pairing code
-            signature = parts[4] # HMAC signature
+            action = parts[1]    # approve/deny
+            channel = parts[2]   # telegram/discord/slack
+            user_id = parts[3]   # original requester user ID
+            code = parts[4]      # pairing code
+            signature = parts[5] # HMAC signature
 
             # Verify signature
-            payload = f"pair:{action}:{channel}:{code}"
+            payload = f"pair:{action}:{channel}:{user_id}:{code}"
             secret = _get_callback_secret().encode()
             expected_sig = hmac.new(secret, payload.encode(), hashlib.sha256).hexdigest()[:8]
 
@@ -166,7 +169,8 @@ def parse_and_verify_callback(self, callback_data: str) -> Optional[Dict[str, st
 
             return {
                 "action": action,
-                "channel": channel, 
+                "channel": channel,
+                "user_id": user_id,
                 "code": code
             }
 
@@ -200,12 +204,13 @@ async def handle_approval_callback(
         action = parsed["action"]
         channel = parsed["channel"]
         code = parsed["code"]
+        requester_user_id = parsed["user_id"]
 
         if action == "approve":
             # Approve the pairing
             success = self.pairing_store.verify_and_pair(
                 code=code,
-                channel_id=owner_user_id,  # This would be the original user_id
+                channel_id=requester_user_id,  # Use original requester's ID
                 channel_type=channel,
                 label=f"Approved by {owner_user_id}"
             )
@@ -214,7 +219,7 @@ async def handle_approval_callback(
                 # Notify original user
                 try:
                     await bot_adapter.reply(
-                        owner_user_id,  # This should be the original requester
+                        requester_user_id,  # Notify the original requester
                         "You've been approved! Send me a message."
                     )
                 except Exception as e:
@@ -223,7 +228,7 @@ async def handle_approval_callback(
                 return PairingApprovalResult(
                     success=True,
                     message="✅ Approved",
-                    user_id=owner_user_id,
+                    user_id=requester_user_id,
                     channel=channel
                 )
             else:
@@ -233,8 +238,19 @@ async def handle_approval_callback(
                 )
 
         elif action == "deny":
-            # Revoke if was temporarily added, or just ignore
-            # Note: In real implementation, we'd need to track the original user_id
+            # Consume the code to prevent reuse
+            try:
+                # Try to consume the code without pairing by calling verify_and_pair with dummy values
+                # This will consume the code and return False since it won't actually pair
+                self.pairing_store.verify_and_pair(
+                    code=code,
+                    channel_id="__denied__",  # Dummy value that won't be stored
+                    channel_type="__denied__",
+                    label="Denied pairing request"
+                )
+            except Exception as e:
+                logger.error(f"Failed to consume denied code: {e}")
+            
             return PairingApprovalResult(
                 success=True,
                 message="❌ Denied",

src/praisonai/praisonai/bots/_unknown_user.py

@@ -84,13 +84,8 @@ async def handle(cls, message: BotMessage, bot_ctx: BotContext) -> bool:
         unknown_policy = getattr(bot_ctx.config, 'unknown_user_policy', 'deny')
 
         if unknown_policy == "allow":
-            # Auto-approve and pair the user
-            bot_ctx.pairing_store.verify_and_pair(
-                code="auto", 
-                channel_id=user_id, 
-                channel_type=channel_type,
-                label=f"Auto-approved: {user_name}"
-            )
+            # Auto-approve without creating a persistent pair
+            # Users with "allow" policy don't need to be paired permanently
             return True
 
         elif unknown_policy == "deny":

src/praisonai/praisonai/bots/discord.py

@@ -180,7 +180,9 @@ async def on_message(message):
                 return
 
             # Handle unknown users with pairing system
-            if not self.config.is_user_allowed(bot_message.sender.user_id if bot_message.sender else ""):
+            user_id = bot_message.sender.user_id if bot_message.sender else ""
+            is_explicitly_allowed = bool(self.config.allowed_users) and self.config.is_user_allowed(user_id)
+            if not is_explicitly_allowed:
                 user_allowed = await UnknownUserHandler.handle(bot_message, self._bot_context)
                 if not user_allowed:
                     return
@@ -582,14 +584,22 @@ async def reply(self, chat_id: str, text: str) -> None:
 
         try:
             if chat_id.isdigit():
-                # DM to user
-                user = await self._client.fetch_user(int(chat_id))
-                await user.send(text)
-            else:
-                # Channel message
+                # Try channel first, then user, then fetch channel
                 channel = self._client.get_channel(int(chat_id))
                 if channel:
                     await channel.send(text)
+                    return
+                
+                try:
+                    user = await self._client.fetch_user(int(chat_id))
+                    await user.send(text)
+                    return
+                except Exception:
+                    # Last resort: fetch channel
+                    channel = await self._client.fetch_channel(int(chat_id))
+                    await channel.send(text)
+            else:
+                logger.error(f"Invalid chat_id format: {chat_id}")
         except Exception as e:
             logger.error(f"Failed to send reply: {e}")
 

src/praisonai/praisonai/bots/slack.py

@@ -194,7 +194,9 @@ async def handle_message(event, say):
                 return
 
             # Handle unknown users with pairing system
-            if not self.config.is_user_allowed(bot_message.sender.user_id if bot_message.sender else ""):
+            user_id = bot_message.sender.user_id if bot_message.sender else ""
+            is_explicitly_allowed = bool(self.config.allowed_users) and self.config.is_user_allowed(user_id)
+            if not is_explicitly_allowed:
                 user_allowed = await UnknownUserHandler.handle(bot_message, self._bot_context)
                 if not user_allowed:
                     return

src/praisonai/praisonai/bots/telegram.py

@@ -204,7 +204,9 @@ async def handle_message(update: Update, context: ContextTypes.DEFAULT_TYPE):
                 return
 
             # Handle unknown users with pairing system
-            if not self.config.is_user_allowed(message.sender.user_id if message.sender else ""):
+            user_id = message.sender.user_id if message.sender else ""
+            is_explicitly_allowed = bool(self.config.allowed_users) and self.config.is_user_allowed(user_id)
+            if not is_explicitly_allowed:
                 user_allowed = await UnknownUserHandler.handle(message, self._bot_context)
                 if not user_allowed:
                     return
@@ -787,6 +789,8 @@ async def send_approval_dm(
             return None
 
         try:
+            from telegram import InlineKeyboardMarkup
+            
             keyboard = PairingUIBuilder.create_telegram_keyboard(
                 user_name=user_name,
                 code=code, 

src/praisonai/tests/integration/bots/test_pairing_owner_dm.py

@@ -12,7 +12,7 @@
 from praisonaiagents.bots.config import BotConfig
 from praisonaiagents.bots.pairing_types import PairingApprovalResult
 from praisonai.bots._unknown_user import UnknownUserHandler, BotContext
-from praisonai.bots._pairing_ui import PairingCallbackHandler
+from praisonai.bots._pairing_ui import PairingCallbackHandler, PairingUIBuilder
 from praisonai.gateway.pairing import PairingStore
 
 
@@ -69,7 +69,7 @@ def setup_method(self):
 
     def create_test_message(self, user_id: str = "new-user", user_name: str = "Alice") -> BotMessage:
         """Create a test message from an unknown user."""
-        return BotMessage(
+        message = BotMessage(
             message_id="msg-1",
             content="hello",
             message_type=MessageType.TEXT,
@@ -83,8 +83,9 @@ def create_test_message(self, user_id: str = "new-user", user_name: str = "Alice
                 channel_type="dm"
             ),
             timestamp=1234567890.0,
-            _channel_type="telegram"  # Add channel type for pairing
         )
+        message._channel_type = "telegram"  # Add channel type for pairing
+        return message
 
     async def test_unknown_user_triggers_pairing_request(self):
         """Test that unknown user triggers pairing request to owner."""
@@ -119,19 +120,22 @@ async def test_owner_approval_allows_future_messages(self):
         approval_dm = self.adapter.approval_dms[0]
         code = approval_dm["code"]
 
-        # Simulate owner approval
-        callback_handler = PairingCallbackHandler(self.pairing_store)
-        callback_data = f"pair:approve:telegram:{code}:fake_sig"  # Signature check will fail but that's OK for test
-        
-        # We need to bypass signature verification for testing
-        # So let's directly approve via the pairing store
-        success = self.pairing_store.verify_and_pair(
+        # Simulate owner approval using real signed callback
+        keyboard = PairingUIBuilder.create_telegram_keyboard(
+            user_name="Alice",
             code=code,
-            channel_id="new-user",  
-            channel_type="telegram",
-            label="Test approval"
+            channel="telegram",
+            user_id="new-user",
+        )
+        callback_data = keyboard["inline_keyboard"][0][0]["callback_data"]  # Get approve button callback
+        
+        callback_handler = PairingCallbackHandler(self.pairing_store)
+        result = await callback_handler.handle_approval_callback(
+            callback_data=callback_data,
+            owner_user_id="owner-123",
+            bot_adapter=self.adapter,
         )
-        assert success
+        assert result.success
 
         # Now a second message from the same user should be allowed
         message2 = self.create_test_message()