-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: enforce actor-authorization on interactive callbacks & tool approvals #2307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,12 @@ class InteractiveContext: | |
|
|
||
| InteractiveHandler = Callable[[InteractiveContext], Awaitable[Optional[str]]] | ||
|
|
||
| # An authorizer decides whether the resolving actor (context.user_id) is | ||
| # permitted to act on a given interactive callback. Returning False rejects | ||
| # the callback before the handler runs, so unauthorized clicks never resolve | ||
| # privileged actions (e.g. tool approvals). | ||
| InteractiveAuthorizer = Callable[[InteractiveContext], bool] | ||
|
|
||
|
|
||
| def encode_action(namespace: str, action: "PresentationAction") -> str: | ||
| """Encode an action with namespace for callback data. | ||
|
|
@@ -111,22 +117,35 @@ class InteractiveRegistry: | |
| def __init__(self): | ||
| """Initialize the registry.""" | ||
| self._handlers: Dict[str, InteractiveHandler] = {} | ||
| self._authorizers: Dict[str, "InteractiveAuthorizer"] = {} | ||
| self._fallback_handler: Optional[InteractiveHandler] = None | ||
|
|
||
| def register( | ||
| self, | ||
| namespace: str, | ||
| handler: InteractiveHandler | ||
| handler: InteractiveHandler, | ||
| authorize: Optional["InteractiveAuthorizer"] = None, | ||
| ) -> None: | ||
|
Comment on lines
+127
to
128
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new dispatch check only runs when a namespace is registered with |
||
| """Register a handler for a namespace. | ||
|
|
||
| Args: | ||
| namespace: The namespace to handle (e.g., "approval", "menu") | ||
| handler: Async function to handle callbacks in this namespace | ||
| authorize: Optional callable that receives the InteractiveContext | ||
| and returns True if the resolving actor (``context.user_id``) | ||
| is permitted to act on this callback. When provided, an | ||
| unauthorized click is rejected before the handler runs, so it | ||
| never resolves the underlying action. When omitted, behaviour | ||
| is unchanged (any clicker is allowed) for backward | ||
| compatibility. | ||
| """ | ||
| if namespace in self._handlers: | ||
| logger.warning(f"Overwriting existing handler for namespace: {namespace}") | ||
| self._handlers[namespace] = handler | ||
| if authorize is not None: | ||
| self._authorizers[namespace] = authorize | ||
| else: | ||
| self._authorizers.pop(namespace, None) | ||
| logger.debug(f"Registered handler for namespace: {namespace}") | ||
|
|
||
| def unregister(self, namespace: str) -> None: | ||
|
|
@@ -137,6 +156,7 @@ def unregister(self, namespace: str) -> None: | |
| """ | ||
| if namespace in self._handlers: | ||
| del self._handlers[namespace] | ||
| self._authorizers.pop(namespace, None) | ||
| logger.debug(f"Unregistered handler for namespace: {namespace}") | ||
|
|
||
| def set_fallback(self, handler: InteractiveHandler) -> None: | ||
|
|
@@ -162,6 +182,28 @@ async def dispatch(self, context: InteractiveContext) -> bool: | |
| handler = self._handlers.get(namespace) | ||
|
|
||
| if handler: | ||
| # Enforce actor authorization before resolving the action. | ||
| # This is the security boundary: an unauthorized clicker must | ||
| # never resolve a privileged callback (e.g. a tool approval). | ||
| authorizer = self._authorizers.get(namespace) | ||
| if authorizer is not None: | ||
| try: | ||
| allowed = authorizer(context) | ||
| except Exception as e: | ||
| logger.error( | ||
| f"Authorizer for namespace '{namespace}' raised; " | ||
| f"denying callback: {e}" | ||
| ) | ||
| allowed = False | ||
| if not allowed: | ||
| logger.warning( | ||
| f"Unauthorized interactive callback: actor " | ||
| f"'{context.user_id}' rejected for namespace " | ||
| f"'{namespace}'" | ||
| ) | ||
| context.platform_data["authorized"] = False | ||
| return False | ||
|
|
||
| try: | ||
| # Add decoded payload to context | ||
| context.platform_data["decoded_namespace"] = namespace | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| """Tests for actor-authorization enforcement in the interactive registry.""" | ||
|
|
||
| import asyncio | ||
|
|
||
| import pytest | ||
|
|
||
| from praisonaiagents.bots.interactive import ( | ||
| InteractiveContext, | ||
| InteractiveRegistry, | ||
| ) | ||
|
|
||
|
|
||
| def _ctx(user_id: str, callback_data: str = "approval:1a2b3c:allow") -> InteractiveContext: | ||
| return InteractiveContext(callback_data=callback_data, user_id=user_id) | ||
|
|
||
|
|
||
| def test_dispatch_without_authorizer_allows_any_actor(): | ||
| registry = InteractiveRegistry() | ||
| seen = [] | ||
|
|
||
| async def handler(ctx): | ||
| seen.append(ctx.user_id) | ||
| return "ok" | ||
|
|
||
| registry.register("approval", handler) | ||
| handled = asyncio.run(registry.dispatch(_ctx("999"))) | ||
|
|
||
| assert handled is True | ||
| assert seen == ["999"] | ||
|
|
||
|
|
||
| def test_dispatch_rejects_unauthorized_actor(): | ||
| registry = InteractiveRegistry() | ||
| allowed = {"requester", "admin"} | ||
| seen = [] | ||
|
|
||
| async def handler(ctx): | ||
| seen.append(ctx.user_id) | ||
| return "ok" | ||
|
|
||
| registry.register( | ||
| "approval", handler, authorize=lambda ctx: ctx.user_id in allowed | ||
| ) | ||
|
|
||
| handled = asyncio.run(registry.dispatch(_ctx("999"))) | ||
|
|
||
| assert handled is False | ||
| assert seen == [] # handler must never run for unauthorized actor | ||
|
|
||
|
|
||
| def test_dispatch_allows_authorized_actor(): | ||
| registry = InteractiveRegistry() | ||
| allowed = {"requester", "admin"} | ||
| seen = [] | ||
|
|
||
| async def handler(ctx): | ||
| seen.append(ctx.user_id) | ||
| return "ok" | ||
|
|
||
| registry.register( | ||
| "approval", handler, authorize=lambda ctx: ctx.user_id in allowed | ||
| ) | ||
|
|
||
| handled = asyncio.run(registry.dispatch(_ctx("admin"))) | ||
|
|
||
| assert handled is True | ||
| assert seen == ["admin"] | ||
|
|
||
|
|
||
| def test_authorizer_exception_denies(): | ||
| registry = InteractiveRegistry() | ||
| seen = [] | ||
|
|
||
| async def handler(ctx): | ||
| seen.append(ctx.user_id) | ||
| return "ok" | ||
|
|
||
| def boom(ctx): | ||
| raise RuntimeError("authorizer failure") | ||
|
|
||
| registry.register("approval", handler, authorize=boom) | ||
|
|
||
| handled = asyncio.run(registry.dispatch(_ctx("admin"))) | ||
|
|
||
| assert handled is False | ||
| assert seen == [] | ||
|
|
||
|
|
||
| def test_unregister_clears_authorizer(): | ||
| registry = InteractiveRegistry() | ||
|
|
||
| async def handler(ctx): | ||
| return "ok" | ||
|
|
||
| registry.register("approval", handler, authorize=lambda ctx: False) | ||
| registry.unregister("approval") | ||
| # Re-register with no authorizer -> should allow again | ||
| registry.register("approval", handler) | ||
|
|
||
| handled = asyncio.run(registry.dispatch(_ctx("999"))) | ||
| assert handled is True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move the public authorizer contract to a Protocol.
InteractiveAuthorizeris now exported API, but core SDK contracts should be protocol-driven. Consider definingInteractiveAuthorizerProtocolin the module’sprotocols.pyand importing it here. As per coding guidelines, “Protocol naming must ALWAYS suffix with 'Protocol' ... and be placed in protocols.py files within their respective modules.”🤖 Prompt for AI Agents
Source: Coding guidelines