fix: enforce actor-authorization on interactive callbacks & tool approvals#2307
fix: enforce actor-authorization on interactive callbacks & tool approvals#2307praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
β¦ovals (fixes #2299) Bind interactive presentations (esp. tool approvals) to an authorized actor set and enforce it in the shared core registry so every channel inherits the check. Core (praisonaiagents.bots.interactive): - InteractiveRegistry.register() accepts an optional `authorize` callback; dispatch() rejects unauthorized clicks before the handler runs (fail-closed on authorizer errors). Backward compatible when no authorizer is given. - New InteractiveAuthorizer type, exported from bots package. Wrapper (praisonai.bots._presentation_approval): - request_approval() records `allowed_actors`; handle_approval_command() takes `actor`, enforces membership, and is single-use + audited. - Added replay protection (resolved ids), an audit_log, and is_authorized(). - Fixed pre-existing PresentationBlock.text/.context calls to make_text/make_context. Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more β On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
β Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the βοΈ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
π WalkthroughWalkthroughInteractive callback dispatch now supports namespace authorizers, and the presentation approval handler now records allowed actors, blocks unauthorized or duplicate resolutions, and logs timeout and resolution outcomes. ChangesInteractive authorization and approval audit
Estimated code review effortπ― 4 (Complex) | β±οΈ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
π₯ Pre-merge checks | β 4 | β 1β Failed checks (1 warning)
β Passed checks (4 passed)
β¨ Finishing Touchesπ§ͺ Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first β incorporate their findings. Review areas:
|
Greptile SummaryThis PR adds actor checks and replay tracking to interactive approval callbacks. The main changes are:
Confidence Score: 4/5This is close, but the replay-history behavior should be fixed before merging.
src/praisonai/praisonai/bots/_presentation_approval.py
|
| Filename | Overview |
|---|---|
| src/praisonai-agents/praisonaiagents/bots/interactive.py | Adds optional namespace authorizers and fail-closed dispatch handling. |
| src/praisonai-agents/praisonaiagents/bots/init.py | Exports the new InteractiveAuthorizer type. |
| src/praisonai/praisonai/bots/_presentation_approval.py | Adds actor authorization, audit logging, replay tracking, and bounded history storage. |
| src/praisonai-agents/tests/unit/test_interactive_authorization.py | Adds unit tests for registry authorization behavior. |
| src/praisonai/tests/unit/bots/test_presentation_approval_authorization.py | Adds unit tests for presentation approval authorization, audit logging, and bounded replay state. |
Reviews (2): Last reviewed commit: "fix: bound approval replay/audit retenti..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| authorize: Optional["InteractiveAuthorizer"] = None, | ||
| ) -> None: |
There was a problem hiding this comment.
The new dispatch check only runs when a namespace is registered with authorize=, but the existing Slack, Telegram, and Discord interactive registrations still call register() without it. Those callbacks keep legacy allow-anyone behavior at the registry boundary, so a shared-chat approval click can still reach the handler unless every downstream caller also supplies and enforces its own actor set.
| channel_send_func: Optional[Any] = None, | ||
| target: Optional[str] = None, | ||
| timeout: float = 60.0, | ||
| allowed_actors: Optional[Iterable[str]] = None, |
There was a problem hiding this comment.
allowed_actors defaults to None, and is_authorized() treats that state as unrestricted. Any current or future approval request that is not updated to pass the requester/admin set will still let any participant resolve the pending tool approval, which leaves the new actor binding fail-open on missed call sites.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
π§Ή Nitpick comments (2)
src/praisonai-agents/tests/unit/test_interactive_authorization.py (1)
17-101: π Maintainability & Code Quality | π΅ Trivial | ποΈ Heavy liftAdd or point to the required agentic coverage.
These unit tests cover the registry directly, but this feature also needs an agentic end-to-end test path where an agent performs the approval flow. As per coding guidelines, βEvery feature must include a real agentic test where the agent actually calls the LLM end-to-end, not just smoke tests.β
π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai-agents/tests/unit/test_interactive_authorization.py` around lines 17 - 101, The InteractiveRegistry unit tests cover authorization behavior, but this feature still lacks the required agentic end-to-end coverage. Add or point to a real agentic test path for the approval flow where an agent actually triggers the interaction through the LLM end-to-end, using the existing approval/dispatch setup rather than only direct registry calls. Keep the new coverage aligned with the current InteractiveRegistry and dispatch-based flow so the agentic path verifies authorization in a realistic run.Source: Coding guidelines
src/praisonai-agents/praisonaiagents/bots/interactive.py (1)
198-205: π Maintainability & Code Quality | π΅ Trivial | β‘ Quick winEmit an EventBus event for authorization denial.
This fail-closed path is a key security operation, but it currently only logs and mutates
platform_data. Emit an authorization-denied event with namespace, actor, and reason so adapters/monitoring can observe rejected callbacks centrally. As per coding guidelines, βAll key operations must emit events via EventBus for observability and integration with monitoring systems.βπ€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai-agents/praisonaiagents/bots/interactive.py` around lines 198 - 205, The unauthorized callback path in interactive.py currently only logs and updates context.platform_data, so add an EventBus emission in the authorization-denied branch of the interactive callback handling. Use the existing callback flow around the allowed check and include namespace, context.user_id, and a denial reason in the event payload so adapters and monitoring can observe rejected callbacks centrally; keep the existing logger.warning and return False behavior intact.Source: Coding guidelines
π€ Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai-agents/praisonaiagents/bots/interactive.py`:
- Around line 48-52: The public authorizer contract in InteractiveAuthorizer
should be moved from a plain Callable alias to a Protocol-based API. Define
InteractiveAuthorizerProtocol in the moduleβs protocols.py, give it the same
callable behavior over InteractiveContext, and import/use that protocol here in
interactive.py so the exported contract follows the SDKβs protocol naming and
placement guidelines.
In `@src/praisonai/praisonai/bots/_presentation_approval.py`:
- Around line 29-34: The replay/audit state in the approval flow is unbounded,
so `_resolved_ids` and `_audit_log` can grow forever in
`_presentation_approval.py`. Update the approval bookkeeping around
`self._resolved_ids` and `self._audit_log` to enforce a retention policy, such
as TTL-based expiry and/or capped size with pruning of old entries. Make sure
the logic that records resolutions, timeouts, and unauthorized attempts also
removes stale IDs and prunes audit entries consistently.
- Around line 315-322: The audit_log property in _presentation_approval
currently only copies the list container, so callers can still mutate the
underlying dict entries and ΦΥΈΦΥΈΥ alter the handlerβs audit trail. Update
audit_log to return copied records for each item in self._audit_log, preserving
the same fields while preventing external mutation of internal state. Use the
existing audit_log property and _audit_log storage as the key symbols to locate
the fix.
In `@src/praisonai/tests/unit/bots/test_presentation_approval_authorization.py`:
- Around line 52-53: The unit test for presentation approval authorization is
using a hardcoded /tmp path in the fixture data, which triggers Ruff S108.
Update the test data in test_presentation_approval_authorization to use a
non-temp placeholder path for the delete_file tool arguments so the test remains
lint-clean while preserving the intent of the fixture.
- Line 103: The asyncio.gather call in test_presentation_approval_authorization
currently assigns an intentionally unused value to result, which Ruff flags as
unused. Update the unpacking in the test coroutine that calls
asyncio.gather(request(), approver()) so the ignored value is named with a
leading underscore, keeping first and second unchanged and making the intent
explicit.
---
Nitpick comments:
In `@src/praisonai-agents/praisonaiagents/bots/interactive.py`:
- Around line 198-205: The unauthorized callback path in interactive.py
currently only logs and updates context.platform_data, so add an EventBus
emission in the authorization-denied branch of the interactive callback
handling. Use the existing callback flow around the allowed check and include
namespace, context.user_id, and a denial reason in the event payload so adapters
and monitoring can observe rejected callbacks centrally; keep the existing
logger.warning and return False behavior intact.
In `@src/praisonai-agents/tests/unit/test_interactive_authorization.py`:
- Around line 17-101: The InteractiveRegistry unit tests cover authorization
behavior, but this feature still lacks the required agentic end-to-end coverage.
Add or point to a real agentic test path for the approval flow where an agent
actually triggers the interaction through the LLM end-to-end, using the existing
approval/dispatch setup rather than only direct registry calls. Keep the new
coverage aligned with the current InteractiveRegistry and dispatch-based flow so
the agentic path verifies authorization in a realistic run.
πͺ Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
βΉοΈ Review info
βοΈ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f243891d-21be-49e7-b0de-1f1180a2c9ed
π Files selected for processing (5)
src/praisonai-agents/praisonaiagents/bots/__init__.pysrc/praisonai-agents/praisonaiagents/bots/interactive.pysrc/praisonai-agents/tests/unit/test_interactive_authorization.pysrc/praisonai/praisonai/bots/_presentation_approval.pysrc/praisonai/tests/unit/bots/test_presentation_approval_authorization.py
| # 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] |
There was a problem hiding this comment.
π Maintainability & Code Quality | π Major | β‘ Quick win
Move the public authorizer contract to a Protocol.
InteractiveAuthorizer is now exported API, but core SDK contracts should be protocol-driven. Consider defining InteractiveAuthorizerProtocol in the moduleβs protocols.py and 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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai-agents/praisonaiagents/bots/interactive.py` around lines 48 -
52, The public authorizer contract in InteractiveAuthorizer should be moved from
a plain Callable alias to a Protocol-based API. Define
InteractiveAuthorizerProtocol in the moduleβs protocols.py, give it the same
callable behavior over InteractiveContext, and import/use that protocol here in
interactive.py so the exported contract follows the SDKβs protocol naming and
placement guidelines.
Source: Coding guidelines
| # Audit trail of resolved approvals for replay protection and | ||
| # accountability. Records who resolved each approval, the decision, | ||
| # and when. A resolved id is single-use: subsequent callbacks for it | ||
| # are treated as no-ops. | ||
| self._audit_log: List[Dict[str, Any]] = [] | ||
| self._resolved_ids: Set[str] = set() |
There was a problem hiding this comment.
π©Ί Stability & Availability | π Major | ποΈ Heavy lift
Bound replay/audit retention.
_resolved_ids and _audit_log grow for every timeout, resolution, and unauthorized attempt, with no pruning. In long-running bots, this becomes an unbounded memory sink; add a TTL/max-size retention policy for resolved IDs and audit entries.
π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai/praisonai/bots/_presentation_approval.py` around lines 29 - 34,
The replay/audit state in the approval flow is unbounded, so `_resolved_ids` and
`_audit_log` can grow forever in `_presentation_approval.py`. Update the
approval bookkeeping around `self._resolved_ids` and `self._audit_log` to
enforce a retention policy, such as TTL-based expiry and/or capped size with
pruning of old entries. Make sure the logic that records resolutions, timeouts,
and unauthorized attempts also removes stale IDs and prunes audit entries
consistently.
| @property | ||
| def audit_log(self) -> List[Dict[str, Any]]: | ||
| """Return the immutable-ish audit trail of resolved approvals. | ||
|
|
||
| Each entry records ``approval_id``, ``tool_name``, ``actor``, | ||
| ``decision``, ``approved``, and ``timestamp``. | ||
| """ | ||
| return list(self._audit_log) |
There was a problem hiding this comment.
ποΈ Data Integrity & Integration | π‘ Minor | β‘ Quick win
Return copied audit entries, not mutable internal records.
list(self._audit_log) protects only the list container; callers can still mutate the dict entries and alter the handlerβs audit trail. Copy each entry before returning.
Proposed fix
- return list(self._audit_log)
+ return [dict(entry) for entry in self._audit_log]π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @property | |
| def audit_log(self) -> List[Dict[str, Any]]: | |
| """Return the immutable-ish audit trail of resolved approvals. | |
| Each entry records ``approval_id``, ``tool_name``, ``actor``, | |
| ``decision``, ``approved``, and ``timestamp``. | |
| """ | |
| return list(self._audit_log) | |
| `@property` | |
| def audit_log(self) -> List[Dict[str, Any]]: | |
| """Return the immutable-ish audit trail of resolved approvals. | |
| Each entry records ``approval_id``, ``tool_name``, ``actor``, | |
| ``decision``, ``approved``, and ``timestamp``. | |
| """ | |
| return [dict(entry) for entry in self._audit_log] |
π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai/praisonai/bots/_presentation_approval.py` around lines 315 -
322, The audit_log property in _presentation_approval currently only copies the
list container, so callers can still mutate the underlying dict entries and
ΦΥΈΦΥΈΥ alter the handlerβs audit trail. Update audit_log to return copied records
for each item in self._audit_log, preserving the same fields while preventing
external mutation of internal state. Use the existing audit_log property and
_audit_log storage as the key symbols to locate the fix.
| tool_name="delete_file", | ||
| arguments={"path": "/tmp/x"}, |
There was a problem hiding this comment.
π Maintainability & Code Quality | π‘ Minor | β‘ Quick win
Avoid the hardcoded /tmp test value flagged by Ruff.
This is only fixture data, but Ruff S108 flags it. Use a non-temp placeholder path so the test stays lint-clean.
Proposed fix
- arguments={"path": "/tmp/x"},
+ arguments={"path": "sample/path.txt"},π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tool_name="delete_file", | |
| arguments={"path": "/tmp/x"}, | |
| tool_name="delete_file", | |
| arguments={"path": "sample/path.txt"}, |
π§° Tools
πͺ ast-grep (0.44.0)
[info] 52-52: Do not hardcode temporary file or directory names
Context: "/tmp/x"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
πͺ Ruff (0.15.18)
[error] 53-53: Probable insecure usage of temporary file or directory: "/tmp/x"
(S108)
π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai/tests/unit/bots/test_presentation_approval_authorization.py`
around lines 52 - 53, The unit test for presentation approval authorization is
using a hardcoded /tmp path in the fixture data, which triggers Ruff S108.
Update the test data in test_presentation_approval_authorization to use a
non-temp placeholder path for the delete_file tool arguments so the test remains
lint-clean while preserving the intent of the fixture.
Source: Linters/SAST tools
| ) | ||
| return first, second | ||
|
|
||
| result, (first, second) = await asyncio.gather(request(), approver()) |
There was a problem hiding this comment.
π Maintainability & Code Quality | π‘ Minor | β‘ Quick win
Mark the intentionally unused result as ignored.
Ruff flags result as unused; rename it to _result to make the intent explicit.
Proposed fix
- result, (first, second) = await asyncio.gather(request(), approver())
+ _result, (first, second) = await asyncio.gather(request(), approver())π Committable suggestion
βΌοΈ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| result, (first, second) = await asyncio.gather(request(), approver()) | |
| _result, (first, second) = await asyncio.gather(request(), approver()) |
π§° Tools
πͺ Ruff (0.15.18)
[warning] 103-103: Unpacked variable result is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai/tests/unit/bots/test_presentation_approval_authorization.py` at
line 103, The asyncio.gather call in test_presentation_approval_authorization
currently assigns an intentionally unused value to result, which Ruff flags as
unused. Update the unpacking in the test coroutine that calls
asyncio.gather(request(), approver()) so the ignored value is named with a
leading underscore, keeping first and second unchanged and making the intent
explicit.
Source: Linters/SAST tools
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task ββ View job Final Architecture Review β PR #2307
Phase 1 β Architecture review (per AGENTS.md)
Phase 2 β Fixes pushed (commit
|
Address reviewer feedback (Greptile P2, CodeRabbit) on PR #2307: - Bound _resolved_ids and _audit_log with FIFO eviction (history_limit) so long-running bots don't grow unboundedly. - audit_log property now returns copied entries to prevent external mutation of internal audit state. - Lint fixes in tests: replace /tmp path (S108), underscore unused var (RUF059). - Add tests for bounded retention and audit-copy immutability. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task ββ View job Final Architecture Review β PR #2307
Phase 1 β Architecture review (per AGENTS.md)
Phase 2 β Verification of fixesThis is the second architecture pass. The valid blocking issues raised by Greptile and CodeRabbit were already implemented in commit
No new code changes were required in this pass β the tree is already clean and tests are green. Findings intentionally skipped (with reasons)
Test resultsPhase 3 β Verdictβ
Approve. All valid blocking issues are fixed and tested; remaining reviewer notes are intentional backward-compat design or non-blocking enhancements documented above. No |
| while len(self._resolved_order) > self._history_limit: | ||
| oldest = self._resolved_order.popleft() | ||
| self._resolved_ids.discard(oldest) |
There was a problem hiding this comment.
history_limit, while approval IDs are still only uuid4()[:8]. If a late callback arrives after its old ID has been evicted and a new pending approval has reused the same 8-character ID, handle_approval_command() no longer treats it as a replay and can resolve the new approval with the stale decision. That weakens the single-use approval guarantee in the bounded-history path; the cap should not make old callback IDs valid for future pending approvals.
1 participant
Fixes #2299
Summary
Interactive callbacks (especially tool approvals) previously had no actor-authorization binding: any participant in a shared chat could resolve another user's pending approval, with no audit trail or replay protection. This PR closes that gap in the core registry so every channel inherits enforcement.
Changes
Core β
praisonaiagents/bots/interactive.pyInteractiveRegistry.register()now accepts an optionalauthorizecallback.dispatch()enforces the authorizer before the handler runs; unauthorized clicks are rejected (returnFalse) and never resolve the action. Fail-closed if the authorizer raises.InteractiveAuthorizertype.Wrapper β
praisonai/bots/_presentation_approval.pyrequest_approval(..., allowed_actors=...)records who may decide (requester +owner_user_id/admin_users).handle_approval_command(..., actor=...)enforces membership; unauthorized attempts leave the pending approval intact for a legitimate actor.audit_logrecords who/what/when/decision (incl. timeouts).is_authorized()helper andaudit_logproperty.PresentationBlock.text/.contextcalls βmake_text/make_context(the approval presentation could never build before).Tests
src/praisonai-agents/tests/unit/test_interactive_authorization.py(5 tests) β registry enforcement, fail-closed, backward-compat.src/praisonai/tests/unit/bots/test_presentation_approval_authorization.py(5 tests) β actor binding, audit, replay protection, backward-compat.All new tests pass. Pre-existing unrelated failures (rate-limiter timing, invalid-policy validation) are untouched.
Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes