fix: path-scope shell approval targets for out-of-workspace paths - #3591
fix: path-scope shell approval targets for out-of-workspace paths#3591praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
#3589) Make build_permission_target emit a distinct shell:external-path:<path> target when a shell command touches a path outside the workspace root, so a broad bash:*/"allow shell"/session grant cannot silently authorise out-of-workspace access. Reuses the existing command_parser and path_safety.resolve_within_root primitives (no new modules/params). Default-on and fail-closed, with a PRAISONAI_SHELL_WORKSPACE_BOUNDARY opt-out for trusted sandboxed/CI runs. 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? |
MervinPraison
added
pipeline/blocked:manual-review
✅ Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change adds workspace-boundary detection for shell permission targets. Commands that access external paths receive path-specific targets. Boundary checks can be disabled through an environment variable. Tests cover in-workspace, external, redirect, and opt-out cases. ChangesWorkspace-bound shell approval
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThe revision preserves shell command identity in approval targets so command-specific deny patterns continue to match external-path commands.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported command-deny bypass is resolved by retaining the bash command target.
|
| Filename | Overview |
|---|---|
| src/praisonai-agents/praisonaiagents/approval/utils.py | Preserves command-bearing shell targets, resolving the previously reported namespace-based deny bypass. |
| src/praisonai-agents/tests/unit/approval/test_scoped_approval.py | Adds focused regression tests showing external commands retain bash identity and command-specific denies still win. |
Reviews (2): Last reviewed commit: "fix: preserve shell command identity in ..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/approval/utils.py`:
- Around line 107-108: Update the path-inspection exception handling in the
relevant target-derivation function so parser, resolver, or lazy-import failures
return a distinct unresolved external target rather than an empty list. Ensure
the downstream logic at the bash target construction does not convert that
failure sentinel into a bash:<command> target or grant approval, and add tests
covering forced parser and resolver failures.
In `@src/praisonai-agents/tests/unit/approval/test_scoped_approval.py`:
- Around line 97-143: Add feature-level coverage alongside the existing scoped
approval tests: include a smoke test and a real agentic test that instantiate
Agent, call agent.start() with a real prompt, invoke the LLM, and print the
complete output. Keep the deterministic unit tests unchanged and exercise the
workspace-boundary behavior through the public agent flow.
🪄 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 Plus
Run ID: c10d7b8a-8f44-41c7-a4b9-c74a493e5be5
📒 Files selected for processing (2)
src/praisonai-agents/praisonaiagents/approval/utils.pysrc/praisonai-agents/tests/unit/approval/test_scoped_approval.py
| except Exception: # noqa: BLE001 — never break target derivation | ||
| return [] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed when path inspection fails.
Lines 107-108 convert a parser, resolver, or lazy-import failure into []. Lines 151-154 then produce bash:<command>. A broad bash:* or session grant can then approve a command that accesses an external path.
Return a distinct unresolved external target on inspection failure. Do not fall back to bash:. Add tests that force parser and resolver failures.
🤖 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/approval/utils.py` around lines 107 -
108, Update the path-inspection exception handling in the relevant
target-derivation function so parser, resolver, or lazy-import failures return a
distinct unresolved external target rather than an empty list. Ensure the
downstream logic at the bash target construction does not convert that failure
sentinel into a bash:<command> target or grant approval, and add tests covering
forced parser and resolver failures.
| def test_in_workspace_shell_stays_bash(self, tmp_path, monkeypatch): | ||
| # A command touching only in-workspace paths keeps its ``bash:`` target. | ||
| from praisonaiagents.approval.utils import build_permission_target | ||
|
|
||
| monkeypatch.chdir(tmp_path) | ||
| target = build_permission_target( | ||
| "execute_command", {"command": "cat ./notes.txt"} | ||
| ) | ||
| assert target == "bash:cat ./notes.txt" | ||
|
|
||
| def test_out_of_workspace_shell_uses_external_target(self, tmp_path, monkeypatch): | ||
| # A command touching a path OUTSIDE the workspace root must earn a | ||
| # distinct ``shell:external-path:<path>`` target so a broad ``bash:*`` | ||
| # / session grant cannot silently authorise it. | ||
| from praisonaiagents.approval.utils import build_permission_target | ||
|
|
||
| monkeypatch.chdir(tmp_path) | ||
| target = build_permission_target( | ||
| "execute_command", {"command": "cat /etc/passwd"} | ||
| ) | ||
| assert target == "shell:external-path:/etc/passwd" | ||
| # A different escaping path yields a different target (path-scoped). | ||
| other = build_permission_target( | ||
| "execute_command", {"command": "cat ~/.ssh/id_rsa"} | ||
| ) | ||
| assert other.startswith("shell:external-path:") | ||
| assert other != target | ||
|
|
||
| def test_redirect_out_of_workspace_flagged(self, tmp_path, monkeypatch): | ||
| from praisonaiagents.approval.utils import build_permission_target | ||
|
|
||
| monkeypatch.chdir(tmp_path) | ||
| target = build_permission_target( | ||
| "execute_command", {"command": "echo hi > /tmp/evil.txt"} | ||
| ) | ||
| assert target == "shell:external-path:/tmp/evil.txt" | ||
|
|
||
| def test_boundary_opt_out_env(self, tmp_path, monkeypatch): | ||
| # The explicit opt-out (sandboxed/CI) restores the plain ``bash:`` target. | ||
| from praisonaiagents.approval.utils import build_permission_target | ||
|
|
||
| monkeypatch.chdir(tmp_path) | ||
| monkeypatch.setenv("PRAISONAI_SHELL_WORKSPACE_BOUNDARY", "0") | ||
| target = build_permission_target( | ||
| "execute_command", {"command": "cat /etc/passwd"} | ||
| ) | ||
| assert target == "bash:cat /etc/passwd" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add the required feature-level tests.
This PR adds deterministic unit tests. It does not add the required smoke test or real agentic test for this feature. Add a test where an Agent calls agent.start() with a real prompt, invokes the LLM, and prints the full output.
As per coding guidelines, “Every feature requires both smoke tests and a real agentic test in which an Agent calls agent.start() with a real prompt, invokes the LLM, and prints the full output.”
🤖 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/approval/test_scoped_approval.py` around
lines 97 - 143, Add feature-level coverage alongside the existing scoped
approval tests: include a smoke test and a real agentic test that instantiate
Agent, call agent.start() with a real prompt, invoke the LLM, and print the
complete output. Keep the deterministic unit tests unchanged and exercise the
workspace-boundary behavior through the public agent flow.
Source: Coding guidelines
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if external: | ||
| return f"{_SHELL_EXTERNAL_PREFIX}:{','.join(external)}" |
There was a problem hiding this comment.
When a command covered by a command-specific deny rule such as bash:rm * references an out-of-workspace path, this replaces the command-bearing target with shell:external-path:<path>, so the deny pattern no longer matches and the prohibited operation can be authorized through the approval path.
How this was verified: The generated shell:external-path:/tmp/file target cannot match bash:rm * under the full-target fnmatch rule matching.
Knowledge Base Used: praisonai-agents Core Library
|
@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 |
MervinPraison
added
pipeline/awaiting-merge-gate
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #3591 ✅ Fixed & pushedVerdict: the reviewers found a real security regression that this PR introduced. Fixed by reverting to the correct, already-existing architecture. What was actually wrong (verified empirically)
Root cause / why the fix is a revert, not more codeThe out-of-workspace boundary is already fully implemented in
The PR re-implemented boundary detection at the wrong layer (the approval-target string) and broke deny in the process. The correct SDK-lightweight fix is to keep I also rejected two tempting-but-wrong expansions as scope creep / regressions:
Files modified
Skipped (with reason)
Tests
Verdict: ✅ ApproveThe introduced deny-bypass and namespace collision are resolved; the change is now minimal, backward-compatible, and relies on the existing hardened |
The previous approach replaced the bash:<command> approval target with a shell:external-path:<path> target for out-of-workspace commands. This both broke command-specific deny rules (a hard `deny: bash:rm *` no longer matched, regressing DENY -> ASK and letting a user approve a denied command) and collided with the existing `shell:` shell-prefix namespace in PermissionManager.check(). The out-of-workspace boundary is already fully implemented in PermissionManager (`_check_shell_command` + `_external_dir_target`): with a workspace_root configured, a broad `bash:*` allow escalates external paths to ASK while a command-specific deny still fires. build_permission_target now keeps the verbatim `bash:<command>` target so that machinery applies and command identity is preserved. - approval/utils.py: drop the redundant shell:external-path namespace and the _shell_external_paths helper; shell tools map back to bash:<command>. - tests: replace target-mangling assertions with boundary-enforcement tests that verify (a) broad bash:* does not cover external paths, (b) in-workspace stays allowed, (c) command-specific deny still fires on external paths, and (d) backward-compat with no workspace_root. Fixes the deny-bypass flagged by Greptile and CodeRabbit. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
praisonai-triage-agent
Bot
removed
pipeline/blocked:ci
1 participant
Fixes #3589
Summary
The built-in
execute_commandshell tool bypassed the workspace-containment boundary that file tools enforce: its approval scope/session grants flowed throughbuild_permission_targetas a flatbash:<command>, so a broadbash:*/ "allow shell" / session grant silently authorised commands touching paths anywhere on the host (~/.ssh,/etc, sibling repos).This makes
build_permission_target(approval/utils.py) emit a distinctshell:external-path:<path>target whenever a shell command touches a path that resolves outside the workspace root. Because the target lives in a different namespace frombash:, a broadbash:*/"allow shell"/session grant no longer covers it, and the offending path(s) are named so the grant is path-scoped — mirroring the file tools'edit:<path>/write:<path>targets.Why minimal / lightweight
PermissionManageralready has full shell workspace-boundary machinery (_check_shell_command+_external_dir_target+command_parser). The only gap was the scope/session-target layer used by the built-in shell tool.permissions.command_parser.parse_command(path extraction) andtools.path_safety.resolve_within_root(the same resolver file tools use). No new modules, no new Agent params, no new exports.[]so target derivation never breaks a call; the downstream boundary gate still applies). Explicit opt-out viaPRAISONAI_SHELL_WORKSPACE_BOUNDARY=0for trusted sandboxed/CI runs. Workspace root defaults to$PRAISONAI_WORKSPACE_ROOTor cwd.Behaviour
git status -sbash:git status -s(unchanged)cat ./notes.txtbash:cat ./notes.txt(in-workspace)cat /etc/passwdshell:external-path:/etc/passwdecho hi > /tmp/evil.txtshell:external-path:/tmp/evil.txtTest plan
tests/unit/approval/test_scoped_approval.pycover in-workspace (staysbash:), out-of-workspace read, redirect target, distinct-path scoping, and env opt-out.tests/unit/approval+tests/unit/permissionspass (3 pre-existing unrelatedConsoleBackendnaming failures confirmed present on a clean tree).Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes