From b2589fe5b5346b1233b724edb5856dd73afcb3a4 Mon Sep 17 00:00:00 2001 From: "praisonai-triage-agent[bot]" <272766704+praisonai-triage-agent[bot]@users.noreply.github.com> Date: Sun, 2 Aug 2026 08:55:12 +0000 Subject: [PATCH 1/2] fix: path-scope shell approval targets for out-of-workspace paths (fixes #3589) Make build_permission_target emit a distinct shell:external-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 --- .../praisonaiagents/approval/utils.py | 65 ++++++++++++++++++- .../unit/approval/test_scoped_approval.py | 48 ++++++++++++++ 2 files changed, 110 insertions(+), 3 deletions(-) diff --git a/src/praisonai-agents/praisonaiagents/approval/utils.py b/src/praisonai-agents/praisonaiagents/approval/utils.py index 2ae6df018..afd819ab5 100644 --- a/src/praisonai-agents/praisonaiagents/approval/utils.py +++ b/src/praisonai-agents/praisonaiagents/approval/utils.py @@ -9,7 +9,8 @@ import concurrent.futures import hashlib import json -from typing import Any, Awaitable, Callable, Dict, Optional, TypeVar +import os +from typing import Any, Awaitable, Callable, Dict, List, Optional, TypeVar T = TypeVar('T') @@ -56,6 +57,57 @@ def hash_tool_args(arguments: Optional[Dict[str, Any]]) -> str: "copy_file": "copy", } +# Prefix for a shell command that touches a path *outside* the workspace root. +# Distinct from ``bash:`` so a broad ``bash:*`` / "allow shell" / session grant +# never silently authorises out-of-workspace access — the escaping path is named +# so the grant is path-scoped, mirroring the ``edit:`` file-tool targets. +_SHELL_EXTERNAL_PREFIX = "shell:external-path" + + +def _shell_external_paths(command: str) -> List[str]: + """Return the workspace-escaping paths referenced by a shell *command*. + + Reuses the existing command decomposition (``permissions.command_parser``) + and the shared containment resolver (``tools.path_safety``) — the very + primitives the file tools rely on — so shell path-scoping cannot diverge + from the SDK's file-tool workspace guarantee. The workspace root defaults + to ``$PRAISONAI_WORKSPACE_ROOT`` or the current working directory. + + Set ``PRAISONAI_SHELL_WORKSPACE_BOUNDARY`` to ``0``/``false``/``no`` to opt + out (e.g. trusted sandboxed/CI runs); the check then returns ``[]`` and the + command keeps its plain ``bash:`` target. Any parse/resolve failure + also returns ``[]`` so target derivation never breaks a tool call — the + downstream ``PermissionManager`` boundary gate still applies fail-closed. + """ + if os.environ.get( + "PRAISONAI_SHELL_WORKSPACE_BOUNDARY", "1" + ).lower() in ("0", "false", "no"): + return [] + try: + from ..permissions.command_parser import parse_command + from ..tools.path_safety import resolve_within_root + + root = os.environ.get("PRAISONAI_WORKSPACE_ROOT") or os.getcwd() + escaping: List[str] = [] + seen = set() + for op in parse_command(command): + candidates = list(op.write_targets) + list(op.path_args) + # An executable referenced by path runs code outside the workspace; + # a bare name (``rm``) is PATH-resolved and must not be flagged. + exe = op.executable + if exe and (exe.startswith(("/", "~", "./", "../", "$")) or "/" in exe): + candidates.append(exe) + for path in candidates: + if path in seen: + continue + seen.add(path) + if resolve_within_root(path, root) is None: + escaping.append(path) + return escaping + except Exception: # noqa: BLE001 — never break target derivation + return [] + + # Argument keys commonly holding the shell command / file path, in priority order. _COMMAND_KEYS = ("command", "cmd", "code", "query") # ``src`` covers ``move_file``/``copy_file`` (which take ``src``/``dst``) so a @@ -72,7 +124,10 @@ def build_permission_target( Maps a tool name + arguments to a target string the permission store can match against (and generalise via ``suggest_scope_pattern``): - * shell tools -> ``bash:`` + * shell tools -> ``bash:`` — but a command touching a path + *outside* the workspace root instead yields + ``shell:external-path:`` so a broad ``bash:*`` / "allow shell" / + session grant cannot silently authorise out-of-workspace access. * file tools -> ``:`` * everything else -> ``tool:`` @@ -92,7 +147,11 @@ def build_permission_target( for key in _COMMAND_KEYS: value = args.get(key) if isinstance(value, str) and value.strip(): - return f"bash:{value.strip()}" + command = value.strip() + external = _shell_external_paths(command) + if external: + return f"{_SHELL_EXTERNAL_PREFIX}:{','.join(external)}" + return f"bash:{command}" return f"tool:{tool_name}" prefix = _FILE_TOOL_PREFIXES.get(tool_name) diff --git a/src/praisonai-agents/tests/unit/approval/test_scoped_approval.py b/src/praisonai-agents/tests/unit/approval/test_scoped_approval.py index b88451381..39b538e17 100644 --- a/src/praisonai-agents/tests/unit/approval/test_scoped_approval.py +++ b/src/praisonai-agents/tests/unit/approval/test_scoped_approval.py @@ -94,6 +94,54 @@ def test_missing_argument_falls_back(self): assert build_permission_target("edit_file", {}) == "tool:edit_file" + 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:`` 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" + # ── ConsoleBackend scoped prompt ──────────────────────────────────────────── From 308d984438fe4f75d6493d3a3f4a3718cfbecd03 Mon Sep 17 00:00:00 2001 From: "praisonai-triage-agent[bot]" <272766704+praisonai-triage-agent[bot]@users.noreply.github.com> Date: Sun, 2 Aug 2026 09:09:00 +0000 Subject: [PATCH 2/2] fix: preserve shell command identity in approval target (security) The previous approach replaced the bash: approval target with a shell:external-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:` 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:. - 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 --- .../praisonaiagents/approval/utils.py | 73 ++--------- .../unit/approval/test_scoped_approval.py | 118 +++++++++++++----- 2 files changed, 95 insertions(+), 96 deletions(-) diff --git a/src/praisonai-agents/praisonaiagents/approval/utils.py b/src/praisonai-agents/praisonaiagents/approval/utils.py index afd819ab5..f56d1fc02 100644 --- a/src/praisonai-agents/praisonaiagents/approval/utils.py +++ b/src/praisonai-agents/praisonaiagents/approval/utils.py @@ -9,8 +9,7 @@ import concurrent.futures import hashlib import json -import os -from typing import Any, Awaitable, Callable, Dict, List, Optional, TypeVar +from typing import Any, Awaitable, Callable, Dict, Optional, TypeVar T = TypeVar('T') @@ -57,57 +56,6 @@ def hash_tool_args(arguments: Optional[Dict[str, Any]]) -> str: "copy_file": "copy", } -# Prefix for a shell command that touches a path *outside* the workspace root. -# Distinct from ``bash:`` so a broad ``bash:*`` / "allow shell" / session grant -# never silently authorises out-of-workspace access — the escaping path is named -# so the grant is path-scoped, mirroring the ``edit:`` file-tool targets. -_SHELL_EXTERNAL_PREFIX = "shell:external-path" - - -def _shell_external_paths(command: str) -> List[str]: - """Return the workspace-escaping paths referenced by a shell *command*. - - Reuses the existing command decomposition (``permissions.command_parser``) - and the shared containment resolver (``tools.path_safety``) — the very - primitives the file tools rely on — so shell path-scoping cannot diverge - from the SDK's file-tool workspace guarantee. The workspace root defaults - to ``$PRAISONAI_WORKSPACE_ROOT`` or the current working directory. - - Set ``PRAISONAI_SHELL_WORKSPACE_BOUNDARY`` to ``0``/``false``/``no`` to opt - out (e.g. trusted sandboxed/CI runs); the check then returns ``[]`` and the - command keeps its plain ``bash:`` target. Any parse/resolve failure - also returns ``[]`` so target derivation never breaks a tool call — the - downstream ``PermissionManager`` boundary gate still applies fail-closed. - """ - if os.environ.get( - "PRAISONAI_SHELL_WORKSPACE_BOUNDARY", "1" - ).lower() in ("0", "false", "no"): - return [] - try: - from ..permissions.command_parser import parse_command - from ..tools.path_safety import resolve_within_root - - root = os.environ.get("PRAISONAI_WORKSPACE_ROOT") or os.getcwd() - escaping: List[str] = [] - seen = set() - for op in parse_command(command): - candidates = list(op.write_targets) + list(op.path_args) - # An executable referenced by path runs code outside the workspace; - # a bare name (``rm``) is PATH-resolved and must not be flagged. - exe = op.executable - if exe and (exe.startswith(("/", "~", "./", "../", "$")) or "/" in exe): - candidates.append(exe) - for path in candidates: - if path in seen: - continue - seen.add(path) - if resolve_within_root(path, root) is None: - escaping.append(path) - return escaping - except Exception: # noqa: BLE001 — never break target derivation - return [] - - # Argument keys commonly holding the shell command / file path, in priority order. _COMMAND_KEYS = ("command", "cmd", "code", "query") # ``src`` covers ``move_file``/``copy_file`` (which take ``src``/``dst``) so a @@ -124,13 +72,18 @@ def build_permission_target( Maps a tool name + arguments to a target string the permission store can match against (and generalise via ``suggest_scope_pattern``): - * shell tools -> ``bash:`` — but a command touching a path - *outside* the workspace root instead yields - ``shell:external-path:`` so a broad ``bash:*`` / "allow shell" / - session grant cannot silently authorise out-of-workspace access. + * shell tools -> ``bash:`` * file tools -> ``:`` * everything else -> ``tool:`` + The command identity is preserved verbatim so command-specific rules + (e.g. ``deny: bash:rm *``) still match. The out-of-workspace boundary is + enforced downstream by :class:`~praisonaiagents.permissions.PermissionManager` + (its ``external_dir:`` gate), which decomposes the ``bash:`` target + and gates any escaping path — so a broad ``bash:*`` / "allow shell" / + session grant cannot silently authorise out-of-workspace access while a + command-specific ``deny`` still fires. + Falls back to ``tool:`` whenever the expected argument is missing so a target is always produced. @@ -147,11 +100,7 @@ def build_permission_target( for key in _COMMAND_KEYS: value = args.get(key) if isinstance(value, str) and value.strip(): - command = value.strip() - external = _shell_external_paths(command) - if external: - return f"{_SHELL_EXTERNAL_PREFIX}:{','.join(external)}" - return f"bash:{command}" + return f"bash:{value.strip()}" return f"tool:{tool_name}" prefix = _FILE_TOOL_PREFIXES.get(tool_name) diff --git a/src/praisonai-agents/tests/unit/approval/test_scoped_approval.py b/src/praisonai-agents/tests/unit/approval/test_scoped_approval.py index 39b538e17..52e0c45b8 100644 --- a/src/praisonai-agents/tests/unit/approval/test_scoped_approval.py +++ b/src/praisonai-agents/tests/unit/approval/test_scoped_approval.py @@ -94,53 +94,103 @@ def test_missing_argument_falls_back(self): assert build_permission_target("edit_file", {}) == "tool:edit_file" - def test_in_workspace_shell_stays_bash(self, tmp_path, monkeypatch): - # A command touching only in-workspace paths keeps its ``bash:`` target. + def test_shell_target_preserves_command_identity(self, tmp_path, monkeypatch): + # The shell target is always ``bash:`` verbatim — including for + # out-of-workspace commands — so command-specific rules (e.g. + # ``deny: bash:rm *``) can still match. The out-of-workspace boundary is + # enforced downstream by ``PermissionManager`` (see the boundary tests + # below), NOT by mangling the target into a path-only namespace (which + # would silently evade command-scoped deny rules). from praisonaiagents.approval.utils import build_permission_target monkeypatch.chdir(tmp_path) - target = build_permission_target( - "execute_command", {"command": "cat ./notes.txt"} + assert ( + build_permission_target("execute_command", {"command": "cat ./notes.txt"}) + == "bash:cat ./notes.txt" + ) + assert ( + build_permission_target("execute_command", {"command": "cat /etc/passwd"}) + == "bash:cat /etc/passwd" + ) + assert ( + build_permission_target( + "execute_command", {"command": "echo hi > /tmp/evil.txt"} + ) + == "bash:echo hi > /tmp/evil.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:`` 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"} +# ── Workspace-boundary enforcement (PermissionManager) ────────────────────── + + +class TestShellWorkspaceBoundary: + """The out-of-workspace boundary is enforced by ``PermissionManager`` on the + plain ``bash:`` target, so a broad ``bash:*`` / "allow shell" / + session grant cannot silently authorise external paths *and* a + command-specific ``deny`` still fires (no target-namespace evasion).""" + + def _mgr(self, tmp_path, rule_pattern, action): + from praisonaiagents.permissions import ( + PermissionManager, + PermissionAction, + ) + from praisonaiagents.permissions.rules import PermissionRule + + mgr = PermissionManager( + storage_dir=str(tmp_path / "perm"), + agent_name="w", + workspace_root=str(tmp_path / "ws"), ) - 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"} + mgr.add_rule( + PermissionRule(pattern=rule_pattern, action=PermissionAction(action)) ) - assert other.startswith("shell:external-path:") - assert other != target + return mgr - def test_redirect_out_of_workspace_flagged(self, tmp_path, monkeypatch): - from praisonaiagents.approval.utils import build_permission_target + def test_broad_allow_does_not_cover_external_path(self, tmp_path): + # PR core goal: ``bash:*`` allow must NOT silently authorise a command + # touching a path outside the workspace — it escalates to ASK. + from praisonaiagents.permissions import PermissionAction - monkeypatch.chdir(tmp_path) - target = build_permission_target( - "execute_command", {"command": "echo hi > /tmp/evil.txt"} - ) - assert target == "shell:external-path:/tmp/evil.txt" + mgr = self._mgr(tmp_path, "bash:*", "allow") + result = mgr.check("bash:cat /etc/passwd", agent_name="w") + assert result.action == PermissionAction.ASK - 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 + def test_broad_allow_still_covers_in_workspace(self, tmp_path): + # In-workspace commands under a broad allow are unchanged (no regression). + from praisonaiagents.permissions import PermissionAction - monkeypatch.chdir(tmp_path) - monkeypatch.setenv("PRAISONAI_SHELL_WORKSPACE_BOUNDARY", "0") - target = build_permission_target( - "execute_command", {"command": "cat /etc/passwd"} + ws = tmp_path / "ws" + ws.mkdir(parents=True, exist_ok=True) + mgr = self._mgr(tmp_path, "bash:*", "allow") + result = mgr.check(f"bash:cat {ws}/notes.txt", agent_name="w") + assert result.action == PermissionAction.ALLOW + + def test_command_deny_still_fires_on_external_path(self, tmp_path): + # Regression guard for the reviewer-reported bypass: an explicit + # ``deny: bash:rm *`` MUST still win even when the path is external — + # the target must keep its command identity so the deny matches. + from praisonaiagents.permissions import PermissionAction + + mgr = self._mgr(tmp_path, "bash:rm *", "deny") + result = mgr.check("bash:rm /tmp/evil.txt", agent_name="w") + assert result.action == PermissionAction.DENY + + def test_no_workspace_root_is_backward_compatible(self, tmp_path): + # Backward-compat: with no ``workspace_root`` configured, the boundary + # stays off and a broad allow covers the external command (unchanged). + from praisonaiagents.permissions import ( + PermissionManager, + PermissionAction, ) - assert target == "bash:cat /etc/passwd" + from praisonaiagents.permissions.rules import PermissionRule + + mgr = PermissionManager(storage_dir=str(tmp_path / "perm"), agent_name="w") + assert mgr.workspace_root is None + mgr.add_rule( + PermissionRule(pattern="bash:*", action=PermissionAction("allow")) + ) + result = mgr.check("bash:cat /etc/passwd", agent_name="w") + assert result.action == PermissionAction.ALLOW # ── ConsoleBackend scoped prompt ────────────────────────────────────────────