From 28b5e3309c73195232a11960274e653cdb8de884 Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Thu, 11 Jun 2026 16:17:31 -0700 Subject: [PATCH 1/2] permissions: default ToolContext to mode=default, not bypassPermissions (#274) The implicit bypassPermissions default silently disabled the workspace-root allowlist for every caller that didn't pass an explicit permission_context (SDK embedders, tests, the TUI fallback context). - ToolContext.permission_context now defaults to mode="default" - TS parity: read-only tools auto-allow in has_permissions_to_use_tool (the port was missing the checkReadPermissionForTool -> allow path, so flipping the default would have ask->denied every Read/Glob/Grep) - Write/Edit check_permissions no longer swallow the allowlist ToolPermissionError into passthrough; the inner permission flow re-raises it so out-of-workspace writes fail hard instead of soft-denying through the ask path - security_review's shell-preprocessing context passes an explicit bypass (it intentionally matched the old implicit default) - tests that relied on the implicit bypass now pass an explicit bypass context or a real PermissionAskReply handler; two stale tuple-shape handlers updated to the live protocol Closes #274, closes #168 Co-Authored-By: Claude Opus 4.7 --- src/command_system/security_review.py | 13 +++++++-- src/permissions/check.py | 27 +++++++++++++++++- src/tool_system/context.py | 5 +++- src/tool_system/tools/edit.py | 9 +++--- src/tool_system/tools/write.py | 9 +++--- tests/integration/test_query_integration.py | 11 ++++++-- tests/parity/test_e2e_edit_flow.py | 10 ++++++- tests/parity/test_permission_flow_parity.py | 3 ++ tests/tasks/test_agent_name_registry.py | 26 +++++++++++++---- tests/tasks/test_local_agent_lifecycle.py | 11 ++++++-- tests/tasks/test_local_agent_migration.py | 31 +++++++++++++++++---- tests/test_agent_loop.py | 6 +++- tests/test_agent_tool_async.py | 14 ++++++++-- tests/test_agent_tool_fork.py | 8 +++++- tests/test_bash_image_output.py | 9 +++++- tests/test_esc_cancel_propagation.py | 8 +++++- tests/test_esc_reject_message_dispatch.py | 8 +++++- tests/test_integration_permission_system.py | 7 +++-- tests/test_memdir_write_carve_out.py | 15 ++++------ tests/test_tool_registry_pipeline.py | 17 ++++++++--- tests/test_tool_system_tools.py | 9 +++++- 21 files changed, 201 insertions(+), 55 deletions(-) diff --git a/src/command_system/security_review.py b/src/command_system/security_review.py index c6ea6ec3d..454d21020 100644 --- a/src/command_system/security_review.py +++ b/src/command_system/security_review.py @@ -228,11 +228,18 @@ def _security_review_private_prompt( allowed_tools = parse_slash_command_tools_from_frontmatter( parsed.frontmatter.get("allowed-tools") ) - # Bridge CommandContext -> ToolContext (only workspace_root/cwd are needed; the - # default permission_context is bypassPermissions, matching the in-process path). + # Bridge CommandContext -> ToolContext. Explicit bypass: this executor only + # runs the frontmatter-allowlisted shell preprocessing for the slash + # command, matching the in-process path (the ToolContext default is no + # longer bypassPermissions — #274). + from src.permissions.types import ToolPermissionContext from src.tool_system.context import ToolContext # lazy: keep tool_system off import chain - tool_context = ToolContext(workspace_root=context.workspace_root, cwd=context.cwd) + tool_context = ToolContext( + workspace_root=context.workspace_root, + cwd=context.cwd, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) executor = make_bash_shell_executor( tool_context, allowed_tools, slash_command_name="security-review" ) diff --git a/src/permissions/check.py b/src/permissions/check.py index 3c728ae31..c435b827e 100644 --- a/src/permissions/check.py +++ b/src/permissions/check.py @@ -236,7 +236,16 @@ def has_permissions_to_use_tool_inner( ) try: tool_permission_result = tool.check_permissions(tool_input, tool_use_context) - except Exception: + except Exception as exc: + # Workspace-allowlist violations are hard structural failures, not + # ask-able prompts: propagate so dispatch callers surface the error + # instead of soft-denying through the ask path (#274). Lazy import: + # permissions/ must not import tool_system/ at module level (the + # DAG direction is tool_system -> permissions). + from src.tool_system.errors import ToolPermissionError + + if isinstance(exc, ToolPermissionError): + raise log.exception("Error in tool.check_permissions for %s", tool.name) if tool_permission_result.behavior == "deny": @@ -308,6 +317,22 @@ def has_permissions_to_use_tool_inner( decision_reason=RuleDecisionReason(rule=rule), ) + # Read-only tools don't require permission prompts (TS parity: + # read-only tools resolve via checkReadPermissionForTool → allow). + # Runs after deny/ask rules so explicit rules still win; the + # workspace-root allowlist is still enforced by ensure_allowed_path + # inside each tool's call. + try: + is_read_only = bool(tool.is_read_only(tool_input)) + except Exception: + is_read_only = False + if is_read_only: + return PermissionAllowDecision( + behavior="allow", + updated_input=_get_updated_input_or_fallback(tool_permission_result, tool_input), + decision_reason=OtherDecisionReason(reason="Tool is read-only"), + ) + if tool_permission_result.behavior == "passthrough": return _with_default_suggestions( PermissionAskDecision( diff --git a/src/tool_system/context.py b/src/tool_system/context.py index 5f4c2b726..018243074 100644 --- a/src/tool_system/context.py +++ b/src/tool_system/context.py @@ -66,8 +66,11 @@ class GlobLimits: @dataclass class ToolContext: workspace_root: Path + # "default" so the workspace-root allowlist is on unless a caller + # explicitly opts into bypass — an implicit bypassPermissions default + # silently disabled the sandbox for every SDK/embedder caller (#274). permission_context: ToolPermissionContext = field( - default_factory=lambda: ToolPermissionContext(mode="bypassPermissions") + default_factory=lambda: ToolPermissionContext(mode="default") ) cwd: Path | None = None read_file_fingerprints: dict[Path, tuple[int, int] | tuple[int, int, bool]] = field(default_factory=dict) diff --git a/src/tool_system/tools/edit.py b/src/tool_system/tools/edit.py index 841181737..cb2f62fd9 100644 --- a/src/tool_system/tools/edit.py +++ b/src/tool_system/tools/edit.py @@ -11,7 +11,7 @@ from ..build_tool import Tool, build_tool from ..context import ToolContext from ..diff_utils import unified_diff_hunks -from ..errors import ToolInputError, ToolPermissionError +from ..errors import ToolInputError from ..protocol import ToolResult from src.permissions.types import ( PermissionAskDecision, @@ -187,10 +187,9 @@ def _check_permissions(tool_input: dict[str, Any], context: ToolContext) -> Perm file_path = tool_input.get("file_path") if not isinstance(file_path, str): return PermissionPassthroughResult() - try: - path = context.ensure_allowed_path(file_path) - except ToolPermissionError: - return PermissionPassthroughResult() + # A path outside the workspace allowlist raises ToolPermissionError and + # propagates (hard structural failure, not an ask-able prompt) — see #274. + path = context.ensure_allowed_path(file_path) if path.suffix.lower() in {".md", ".markdown"} and not context.allow_docs: return PermissionAskDecision( message="Editing documentation files is blocked unless allow_docs is enabled", diff --git a/src/tool_system/tools/write.py b/src/tool_system/tools/write.py index b73f66cfd..85ae94094 100644 --- a/src/tool_system/tools/write.py +++ b/src/tool_system/tools/write.py @@ -158,11 +158,10 @@ def _check_permissions(tool_input: dict[str, Any], context: ToolContext) -> Perm if _is_auto_memory_write(file_path): return PermissionPassthroughResult() - # Path is already expanded by backfill_observable_input - try: - path = context.ensure_allowed_path(file_path) - except ToolPermissionError: - return PermissionPassthroughResult() + # Path is already expanded by backfill_observable_input. + # A path outside the workspace allowlist raises ToolPermissionError and + # propagates (hard structural failure, not an ask-able prompt) — see #274. + path = context.ensure_allowed_path(file_path) if path.suffix.lower() in {".md", ".markdown"} and not context.allow_docs: return PermissionAskDecision( diff --git a/tests/integration/test_query_integration.py b/tests/integration/test_query_integration.py index 762943fce..682abeabb 100644 --- a/tests/integration/test_query_integration.py +++ b/tests/integration/test_query_integration.py @@ -5,6 +5,7 @@ from unittest.mock import MagicMock from src.providers.base import ChatResponse +from src.permissions.types import ToolPermissionContext from src.tool_system.context import ToolContext from src.tool_system.defaults import build_default_registry from src.types.content_blocks import TextBlock, ToolResultBlock, ToolUseBlock @@ -24,7 +25,10 @@ def setUp(self): self.temp_dir = tempfile.TemporaryDirectory() self.workspace = Path(self.temp_dir.name) self.registry = build_default_registry() - self.context = ToolContext(workspace_root=self.workspace) + self.context = ToolContext( + workspace_root=self.workspace, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) def tearDown(self): self.temp_dir.cleanup() @@ -224,7 +228,10 @@ def setUp(self): self.temp_dir = tempfile.TemporaryDirectory() self.workspace = Path(self.temp_dir.name) self.registry = build_default_registry() - self.context = ToolContext(workspace_root=self.workspace) + self.context = ToolContext( + workspace_root=self.workspace, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) def tearDown(self): self.temp_dir.cleanup() diff --git a/tests/parity/test_e2e_edit_flow.py b/tests/parity/test_e2e_edit_flow.py index 17a46ab36..83b202ce8 100644 --- a/tests/parity/test_e2e_edit_flow.py +++ b/tests/parity/test_e2e_edit_flow.py @@ -9,13 +9,19 @@ import unittest from pathlib import Path -from src.permissions.types import ToolPermissionContext +from src.permissions.types import PermissionAskReply, ToolPermissionContext from src.tool_system.context import ToolContext from src.tool_system.defaults import build_default_registry from src.tool_system.errors import ToolPermissionError from src.tool_system.protocol import ToolCall +def _allow_all_handler(_request): + """Auto-approve permission prompts — the default-mode workspace + allowlist stays live (outside-root paths still raise before any ask).""" + return PermissionAskReply(behavior="allow") + + class TestE2EEditFlow(unittest.TestCase): """End-to-end edit tool flow.""" @@ -24,6 +30,7 @@ def setUp(self) -> None: self.root = Path(self.tmp.name).resolve() self.registry = build_default_registry(include_user_tools=False) self.ctx = ToolContext(workspace_root=self.root) + self.ctx.permission_handler = _allow_all_handler # Create a file to edit self.test_file = self.root / "target.py" @@ -119,6 +126,7 @@ def setUp(self) -> None: self.root = Path(self.tmp.name).resolve() self.registry = build_default_registry(include_user_tools=False) self.ctx = ToolContext(workspace_root=self.root) + self.ctx.permission_handler = _allow_all_handler def tearDown(self) -> None: self.tmp.cleanup() diff --git a/tests/parity/test_permission_flow_parity.py b/tests/parity/test_permission_flow_parity.py index c36515e76..838c838cc 100644 --- a/tests/parity/test_permission_flow_parity.py +++ b/tests/parity/test_permission_flow_parity.py @@ -43,6 +43,9 @@ def _make_mock_tool(name: str, is_mcp: bool = False): message=f"Claude wants to use {name}. Allow?", ) ) + # The TS vectors model a default (non-read-only) tool; a bare MagicMock + # would return a truthy Mock and trip the read-only auto-allow (#274). + tool.is_read_only = MagicMock(return_value=False) # Default: not user-interactive if hasattr(tool, "requires_user_interaction"): del tool.requires_user_interaction diff --git a/tests/tasks/test_agent_name_registry.py b/tests/tasks/test_agent_name_registry.py index 862f624d5..7b982f421 100644 --- a/tests/tasks/test_agent_name_registry.py +++ b/tests/tasks/test_agent_name_registry.py @@ -6,6 +6,7 @@ import pytest +from src.permissions.types import ToolPermissionContext from src.tool_system.context import ToolContext from src.tool_system.defaults import build_default_registry from src.tool_system.protocol import ToolCall @@ -33,7 +34,10 @@ def test_agent_input_schema_declares_name_field() -> None: def test_registry_field_default_empty(tmp_path: Path) -> None: - ctx = ToolContext(workspace_root=tmp_path) + ctx = ToolContext( + workspace_root=tmp_path, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) assert len(ctx.agent_name_registry) == 0 @@ -41,7 +45,10 @@ def test_named_async_agent_registers_name_to_id(tmp_path: Path) -> None: """Spawn with ``name="researcher"`` populates ``ctx.agent_name_registry["researcher"]``.""" registry = build_default_registry(provider=object()) - ctx = ToolContext(workspace_root=tmp_path) + ctx = ToolContext( + workspace_root=tmp_path, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) async def _fake(_params): yield AssistantMessage(content=[TextBlock(text="ok")]) @@ -67,7 +74,10 @@ async def _fake(_params): def test_unnamed_spawn_does_not_register(tmp_path: Path) -> None: """No ``name`` → registry stays empty.""" registry = build_default_registry(provider=object()) - ctx = ToolContext(workspace_root=tmp_path) + ctx = ToolContext( + workspace_root=tmp_path, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) async def _fake(_params): yield AssistantMessage(content=[TextBlock(text="ok")]) @@ -105,7 +115,10 @@ def test_collision_with_running_agent_raises(tmp_path: Path) -> None: from src.tool_system.errors import ToolInputError registry = build_default_registry(provider=object()) - ctx = ToolContext(workspace_root=tmp_path) + ctx = ToolContext( + workspace_root=tmp_path, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) # Pre-populate the registry as if a running agent was already # registered. @@ -213,7 +226,10 @@ def test_collision_with_terminal_agent_overwrites(tmp_path: Path) -> None: the new spawn overwrites the name → agent_id mapping. Older terminal holders remain reachable via raw task_id.""" registry = build_default_registry(provider=object()) - ctx = ToolContext(workspace_root=tmp_path) + ctx = ToolContext( + workspace_root=tmp_path, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) from src.tasks.local_agent import ( complete_agent_task, diff --git a/tests/tasks/test_local_agent_lifecycle.py b/tests/tasks/test_local_agent_lifecycle.py index 355f67cb1..93c5e2895 100644 --- a/tests/tasks/test_local_agent_lifecycle.py +++ b/tests/tasks/test_local_agent_lifecycle.py @@ -33,6 +33,7 @@ ) from src.tasks.progress import AgentProgress from src.tasks_core import generate_task_id +from src.permissions.types import ToolPermissionContext from src.tool_system.context import ToolContext from src.tool_system.defaults import build_default_registry from src.tool_system.protocol import ToolCall @@ -270,7 +271,10 @@ def test_async_agent_writes_jsonl_transcript_on_disk(tmp_path: Path) -> None: yielded message. This is the prerequisite for Phase 3 / WI-3.1 notification XML and Phase 7 / WI-7.4 auto-resume.""" registry = build_default_registry(provider=object()) - ctx = ToolContext(workspace_root=tmp_path) + ctx = ToolContext( + workspace_root=tmp_path, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) async def _fake(_params): yield AssistantMessage( @@ -320,7 +324,10 @@ def test_async_agent_finalize_total_tokens_is_no_longer_zero(tmp_path: Path) -> the chapter-correct latest_input + cumulative_output instead of the pre-WI-2.4 hard-coded ``0``.""" registry = build_default_registry(provider=object()) - ctx = ToolContext(workspace_root=tmp_path) + ctx = ToolContext( + workspace_root=tmp_path, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) async def _fake(_params): yield AssistantMessage( diff --git a/tests/tasks/test_local_agent_migration.py b/tests/tasks/test_local_agent_migration.py index 0fa629385..71db7b86f 100644 --- a/tests/tasks/test_local_agent_migration.py +++ b/tests/tasks/test_local_agent_migration.py @@ -13,6 +13,7 @@ from unittest.mock import patch from src.tasks.local_agent import LocalAgentTaskState, is_local_agent_task +from src.permissions.types import ToolPermissionContext from src.tool_system.context import ToolContext from src.tool_system.defaults import build_default_registry from src.tool_system.protocol import ToolCall @@ -39,7 +40,10 @@ def test_launch_async_agent_registers_on_runtime_tasks(tmp_path: Path) -> None: """Headline WI-1.5 assertion: state lands on ``runtime_tasks``, NOT on ``context.tasks`` (no more ``metadata._internal=True`` workaround).""" registry = build_default_registry(provider=object()) - ctx = ToolContext(workspace_root=tmp_path) + ctx = ToolContext( + workspace_root=tmp_path, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) async def _fake(_params): yield AssistantMessage(content=[TextBlock(text="ok")]) @@ -72,7 +76,10 @@ def test_async_agent_id_has_a_prefix(tmp_path: Path) -> None: """WI-1.5 prefixed-ID assertion: ``a<8 base36>`` instead of legacy 32-char ``uuid4().hex``.""" registry = build_default_registry(provider=object()) - ctx = ToolContext(workspace_root=tmp_path) + ctx = ToolContext( + workspace_root=tmp_path, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) async def _fake(_params): yield AssistantMessage(content=[TextBlock(text="ok")]) @@ -108,7 +115,10 @@ def test_completed_lifecycle_terminal_status_is_chapter_10_vocabulary( ``run_agent`` and reaches into the provider code. """ registry = build_default_registry(provider=object()) - ctx = ToolContext(workspace_root=tmp_path) + ctx = ToolContext( + workspace_root=tmp_path, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) async def _fake(_params): yield AssistantMessage(content=[TextBlock(text="async done")]) @@ -138,7 +148,10 @@ async def _fake(_params): def test_failed_lifecycle_records_error_on_state(tmp_path: Path) -> None: registry = build_default_registry(provider=object()) - ctx = ToolContext(workspace_root=tmp_path) + ctx = ToolContext( + workspace_root=tmp_path, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) async def _failing(_params): if False: @@ -173,7 +186,10 @@ def test_no_internal_metadata_dict_lingers(tmp_path: Path) -> None: runtime tasks live on the typed registry, not piggy-backed on the todo dict.""" registry = build_default_registry(provider=object()) - ctx = ToolContext(workspace_root=tmp_path) + ctx = ToolContext( + workspace_root=tmp_path, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) async def _fake(_params): yield AssistantMessage(content=[TextBlock(text="ok")]) @@ -203,7 +219,10 @@ def test_task_output_tool_reads_from_runtime_tasks(tmp_path: Path) -> None: runtime_tasks first and projects the typed state into the model-facing output shape.""" registry = build_default_registry(provider=object()) - ctx = ToolContext(workspace_root=tmp_path) + ctx = ToolContext( + workspace_root=tmp_path, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) async def _fake(_params): yield AssistantMessage(content=[TextBlock(text="hello there")]) diff --git a/tests/test_agent_loop.py b/tests/test_agent_loop.py index 1360685c6..89607b70e 100644 --- a/tests/test_agent_loop.py +++ b/tests/test_agent_loop.py @@ -8,6 +8,7 @@ from src.agent.conversation import Conversation from src.providers.base import ChatResponse from src.tool_system.defaults import build_default_registry +from src.permissions.types import ToolPermissionContext from src.tool_system.context import ToolContext from src.query.agent_loop_compat import run_query_as_agent_loop_sync as run_agent_loop from src.tool_system.renderers import AgentLoopResult @@ -21,7 +22,10 @@ def setUp(self): self.temp_dir = tempfile.TemporaryDirectory() self.workspace = Path(self.temp_dir.name) self.registry = build_default_registry() - self.context = ToolContext(workspace_root=self.workspace) + self.context = ToolContext( + workspace_root=self.workspace, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) def tearDown(self): """Clean up test fixtures.""" diff --git a/tests/test_agent_tool_async.py b/tests/test_agent_tool_async.py index 57d782286..6601b7d7b 100644 --- a/tests/test_agent_tool_async.py +++ b/tests/test_agent_tool_async.py @@ -4,6 +4,7 @@ from pathlib import Path from unittest.mock import patch +from src.permissions.types import ToolPermissionContext from src.tool_system.context import ToolContext from src.tool_system.defaults import build_default_registry from src.tool_system.protocol import ToolCall @@ -11,6 +12,15 @@ from src.types.messages import AssistantMessage +def _bypass_context(tmp_path: Path) -> ToolContext: + # Explicit bypass: these tests exercise async-agent task state, not + # permissions (the ToolContext default is no longer bypassPermissions). + return ToolContext( + workspace_root=tmp_path, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) + + def _wait_for_task_status(context: ToolContext, task_id: str, timeout_s: float = 2.0) -> str: """Poll the chapter-10 runtime_tasks registry for terminal status. @@ -46,7 +56,7 @@ def _task_output_text(context: ToolContext, task_id: str) -> str: def test_async_agent_launch_persists_completed_output(tmp_path: Path) -> None: registry = build_default_registry(provider=object()) - context = ToolContext(workspace_root=tmp_path) + context = _bypass_context(tmp_path) async def _fake_run_agent(_params): yield AssistantMessage(content=[TextBlock(text="async done")]) @@ -82,7 +92,7 @@ async def _fake_run_agent(_params): def test_async_agent_launch_marks_failed_output(tmp_path: Path) -> None: registry = build_default_registry(provider=object()) - context = ToolContext(workspace_root=tmp_path) + context = _bypass_context(tmp_path) async def _failing_run_agent(_params): if False: diff --git a/tests/test_agent_tool_fork.py b/tests/test_agent_tool_fork.py index 0636a80d0..104088c92 100644 --- a/tests/test_agent_tool_fork.py +++ b/tests/test_agent_tool_fork.py @@ -18,6 +18,7 @@ import pytest from src.agent.fork_subagent import FORK_AGENT +from src.permissions.types import ToolPermissionContext from src.tool_system.context import ToolContext, ToolUseOptions from src.tool_system.defaults import build_default_registry from src.tool_system.errors import ToolInputError @@ -42,7 +43,12 @@ def _set_fork_env(monkeypatch: pytest.MonkeyPatch, *, enabled: bool) -> None: def _make_interactive_context(tmp_path: Path) -> ToolContext: - ctx = ToolContext(workspace_root=tmp_path) + # Explicit bypass: these tests exercise fork routing, not permissions + # (the ToolContext default is no longer bypassPermissions — #274). + ctx = ToolContext( + workspace_root=tmp_path, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) ctx.options = ToolUseOptions(is_non_interactive_session=False) return ctx diff --git a/tests/test_bash_image_output.py b/tests/test_bash_image_output.py index 6c00f33e5..9e1b3c71a 100644 --- a/tests/test_bash_image_output.py +++ b/tests/test_bash_image_output.py @@ -174,7 +174,14 @@ def setUp(self) -> None: self.tmp = tempfile.TemporaryDirectory() self.root = Path(self.tmp.name).resolve() self.registry = build_default_registry(include_user_tools=False) - self.ctx = ToolContext(workspace_root=self.root) + # Explicit bypass: exercises image output, not permissions (the + # ToolContext default is no longer bypassPermissions — #274). + from src.permissions.types import ToolPermissionContext + + self.ctx = ToolContext( + workspace_root=self.root, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) def tearDown(self) -> None: self.tmp.cleanup() diff --git a/tests/test_esc_cancel_propagation.py b/tests/test_esc_cancel_propagation.py index 3d0b2619c..86e6a8e5e 100644 --- a/tests/test_esc_cancel_propagation.py +++ b/tests/test_esc_cancel_propagation.py @@ -31,6 +31,7 @@ import pytest +from src.permissions.types import ToolPermissionContext from src.providers.base import ChatResponse from src.query.agent_loop_compat import run_query_as_agent_loop_sync as run_agent_loop from src.tool_system.context import ToolContext @@ -95,7 +96,12 @@ def _patch_anthropic_check(monkeypatch: pytest.MonkeyPatch) -> None: def _make_context(workspace: Path) -> ToolContext: - return ToolContext(workspace_root=workspace) + # Explicit bypass: these tests exercise ESC/abort propagation, not + # permissions (the ToolContext default is no longer bypass — #274). + return ToolContext( + workspace_root=workspace, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) def _build_registry_with_blocking_tool( diff --git a/tests/test_esc_reject_message_dispatch.py b/tests/test_esc_reject_message_dispatch.py index e19baf4d2..e4213655a 100644 --- a/tests/test_esc_reject_message_dispatch.py +++ b/tests/test_esc_reject_message_dispatch.py @@ -34,6 +34,7 @@ _dispatch_single_tool, _is_user_cancelled_abort, ) +from src.permissions.types import ToolPermissionContext from src.tool_system.context import ToolContext from src.tool_system.protocol import ToolResult from src.tool_system.registry import ToolRegistry @@ -44,7 +45,12 @@ def _make_ctx(workspace: Path) -> ToolContext: - ctx = ToolContext(workspace_root=workspace) + # Explicit bypass: these tests exercise ESC/abort dispatch plumbing, + # not permissions (the ToolContext default is no longer bypass — #274). + ctx = ToolContext( + workspace_root=workspace, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) ctx.abort_controller = AbortController() return ctx diff --git a/tests/test_integration_permission_system.py b/tests/test_integration_permission_system.py index 2679d13b5..6fc96b666 100644 --- a/tests/test_integration_permission_system.py +++ b/tests/test_integration_permission_system.py @@ -30,6 +30,7 @@ ) from src.permissions.handler import handle_permission_ask from src.permissions.loader import apply_rules_to_context, settings_to_rules +from src.permissions.types import PermissionAskReply from src.tool_system.build_tool import Tool, build_tool from src.tool_system.context import ToolContext from src.tool_system.defaults import build_default_registry @@ -204,7 +205,9 @@ def _check(inp: dict[str, Any], ctx: Any) -> Any: check_permissions=_check, ) reg = ToolRegistry([tool]) - self.ctx.permission_handler = lambda name, msg, sug: (True, False) + # With the default-mode context (#274) the ask path actually runs: + # the handler must speak the PermissionAskReply protocol. + self.ctx.permission_handler = lambda request: PermissionAskReply(behavior="allow") result = reg.dispatch(ToolCall(name="NeedApproval", input={}), self.ctx) self.assertFalse(result.is_error) @@ -215,7 +218,7 @@ def test_dispatch_regular_tool_no_rules(self) -> None: call=_noop_call, ) reg = ToolRegistry([tool]) - self.ctx.permission_handler = lambda name, msg, sug: (True, False) + self.ctx.permission_handler = lambda request: PermissionAskReply(behavior="allow") result = reg.dispatch(ToolCall(name="Simple", input={}), self.ctx) self.assertFalse(result.is_error) diff --git a/tests/test_memdir_write_carve_out.py b/tests/test_memdir_write_carve_out.py index c0a23b0c4..096923083 100644 --- a/tests/test_memdir_write_carve_out.py +++ b/tests/test_memdir_write_carve_out.py @@ -17,6 +17,7 @@ PermissionPassthroughResult, ) from src.tool_system.context import ToolContext +from src.tool_system.errors import ToolPermissionError from src.tool_system.tools.write import _check_permissions, _write_call @@ -95,18 +96,12 @@ def test_check_permissions_passes_through_for_memory_md(self): self.assertIsInstance(result, PermissionPassthroughResult) def test_check_permissions_unchanged_for_outside_path(self): - # A path outside the workspace AND outside auto-mem - # should NOT bypass — falls through to allowlist/docs gate. + # A path outside the workspace AND outside auto-mem must NOT get the + # carve-out: the workspace allowlist raises (hard failure — #274). with tempfile.TemporaryDirectory() as outside: target = Path(outside) / "evil.md" - result = _check_permissions( - {"file_path": str(target)}, self.context - ) - # ensure_allowed_path raises -> passthrough - # OR the .md gate fires. Either way, NOT a bypass-of-everything. - self.assertIsInstance( - result, (PermissionPassthroughResult, PermissionAskDecision) - ) + with self.assertRaises(ToolPermissionError): + _check_permissions({"file_path": str(target)}, self.context) def test_call_writes_into_memory_dir(self): target = self._mem_dir / "test_carve_out_b.md" diff --git a/tests/test_tool_registry_pipeline.py b/tests/test_tool_registry_pipeline.py index 5fccbf01b..d566847a2 100644 --- a/tests/test_tool_registry_pipeline.py +++ b/tests/test_tool_registry_pipeline.py @@ -81,7 +81,12 @@ def test_init_with_tools(self) -> None: class TestRegistryDispatch(unittest.TestCase): def setUp(self) -> None: self.tmp = tempfile.TemporaryDirectory() - self.ctx = ToolContext(workspace_root=Path(self.tmp.name)) + # Explicit bypass for plain dispatch-mechanics tests; permission-path + # tests below build their own default-mode contexts (#274). + self.ctx = ToolContext( + workspace_root=Path(self.tmp.name), + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) self.reg = ToolRegistry() self.tool = _make_tool("Echo") self.reg.register(self.tool) @@ -148,15 +153,19 @@ def _ask(inp: dict, ctx: ToolContext): self.assertTrue(result.is_error) def test_dispatch_checks_permissions_ask_with_handler_allow(self) -> None: - from src.permissions.types import PermissionAskDecision + from src.permissions.types import PermissionAskDecision, PermissionAskReply def _ask(inp: dict, ctx: ToolContext): return PermissionAskDecision(message="confirm?") t = _make_tool("NeedApproval", check_permissions=_ask) reg = ToolRegistry([t]) - self.ctx.permission_handler = lambda name, msg, sug: (True, False) - result = reg.dispatch(ToolCall(name="NeedApproval", input={}), self.ctx) + ctx = ToolContext( + workspace_root=Path(self.tmp.name), + permission_context=ToolPermissionContext(mode="default"), + ) + ctx.permission_handler = lambda request: PermissionAskReply(behavior="allow") + result = reg.dispatch(ToolCall(name="NeedApproval", input={}), ctx) self.assertFalse(result.is_error) def test_dispatch_checks_permissions_ask_with_handler_deny(self) -> None: diff --git a/tests/test_tool_system_tools.py b/tests/test_tool_system_tools.py index ffd1fd406..ef2c3ec9a 100644 --- a/tests/test_tool_system_tools.py +++ b/tests/test_tool_system_tools.py @@ -59,7 +59,14 @@ class ToolSystemTests(unittest.TestCase): def setUp(self) -> None: self.tmp = tempfile.TemporaryDirectory() self.root = Path(self.tmp.name).resolve() - self.ctx = ToolContext(workspace_root=self.root) + # Explicit bypass: these tests exercise tool behavior, not + # permissions (the ToolContext default is no longer bypass — #274). + from src.permissions.types import ToolPermissionContext + + self.ctx = ToolContext( + workspace_root=self.root, + permission_context=ToolPermissionContext(mode="bypassPermissions"), + ) def tearDown(self) -> None: self.tmp.cleanup() From 5cf86f48537b000019a45a9704e40a81500dcaee Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Thu, 11 Jun 2026 16:46:09 -0700 Subject: [PATCH 2/2] permissions: gate read-only auto-allow on passthrough; WebFetch domain gating (#274) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups: the read-only auto-allow only applies when the tool's own check_permissions expressed no opinion, so a read-only tool's ask survives (TS shape: the read-only allow lives inside checkPermissions). WebFetch now returns allow for preapproved hosts and a real ask otherwise — without this, the auto-allow turned the old no-op check_permissions into a silent allow for arbitrary URLs. check_rule_based_permissions mirrors the allowlist re-raise, and the CheckPermissionsTool protocol declares is_read_only. Co-Authored-By: Claude Opus 4.7 --- src/permissions/check.py | 44 ++++++++++++++++++++---------- src/tool_system/tools/web_fetch.py | 15 ++++++++-- 2 files changed, 43 insertions(+), 16 deletions(-) diff --git a/src/permissions/check.py b/src/permissions/check.py index c435b827e..90dc9c890 100644 --- a/src/permissions/check.py +++ b/src/permissions/check.py @@ -48,6 +48,8 @@ def check_permissions( self, tool_input: dict[str, Any], context: Any ) -> PermissionResult: ... + def is_read_only(self, tool_input: dict[str, Any]) -> bool: ... + @runtime_checkable class RequiresInteractionTool(Protocol): @@ -319,19 +321,27 @@ def has_permissions_to_use_tool_inner( # Read-only tools don't require permission prompts (TS parity: # read-only tools resolve via checkReadPermissionForTool → allow). - # Runs after deny/ask rules so explicit rules still win; the - # workspace-root allowlist is still enforced by ensure_allowed_path - # inside each tool's call. - try: - is_read_only = bool(tool.is_read_only(tool_input)) - except Exception: - is_read_only = False - if is_read_only: - return PermissionAllowDecision( - behavior="allow", - updated_input=_get_updated_input_or_fallback(tool_permission_result, tool_input), - decision_reason=OtherDecisionReason(reason="Tool is read-only"), - ) + # Runs after deny/ask rules so explicit rules still win, and ONLY when + # the tool's own check_permissions expressed no opinion (passthrough) — + # a read-only tool that returns its own ask (e.g. WebFetch domain + # gating) must keep that ask, exactly as in TS where the read-only + # allow lives inside each tool's checkPermissions. + # Documented divergence: TS asks for out-of-workspace read paths (the + # user can approve a one-off); this port allows here and hard-fails in + # the tool's call via ensure_allowed_path, with + # additional_working_directories as the escape hatch. + if tool_permission_result.behavior == "passthrough": + try: + is_read_only = bool(tool.is_read_only(tool_input)) + except Exception: + log.debug("tool.is_read_only failed for %s", tool.name, exc_info=True) + is_read_only = False + if is_read_only: + return PermissionAllowDecision( + behavior="allow", + updated_input=_get_updated_input_or_fallback(tool_permission_result, tool_input), + decision_reason=OtherDecisionReason(reason="Tool is read-only"), + ) if tool_permission_result.behavior == "passthrough": return _with_default_suggestions( @@ -453,7 +463,13 @@ def check_rule_based_permissions( ) try: tool_permission_result = tool.check_permissions(tool_input, tool_use_context) - except Exception: + except Exception as exc: + # Same contract as the main inner flow: workspace-allowlist + # violations propagate instead of soft-resolving (#274). + from src.tool_system.errors import ToolPermissionError + + if isinstance(exc, ToolPermissionError): + raise log.exception("Error in tool.check_permissions for %s", tool.name) if tool_permission_result.behavior == "deny": diff --git a/src/tool_system/tools/web_fetch.py b/src/tool_system/tools/web_fetch.py index 456ce3da3..5ff02feb4 100644 --- a/src/tool_system/tools/web_fetch.py +++ b/src/tool_system/tools/web_fetch.py @@ -29,6 +29,8 @@ from ..errors import ToolInputError, ToolPermissionError from ..protocol import ToolResult from src.permissions.types import ( + PermissionAllowDecision, + PermissionAskDecision, PermissionPassthroughResult, PermissionResult, ) @@ -416,18 +418,27 @@ def _is_preapproved(hostname: str, pathname: str) -> bool: # -- Permission Check ---------------------------------------------------------- def _check_permissions(tool_input: dict[str, Any], context: ToolContext) -> PermissionResult: + """Domain gating (TS parity): preapproved hosts auto-allow; everything + else asks. Returning a real ask (not passthrough) keeps WebFetch out of + the central read-only auto-allow — an arbitrary-URL fetch is promptable, + unlike a local read (#274).""" url = tool_input.get("url", "") if not isinstance(url, str) or not url: + # Passthrough resolves to read-only auto-allow, which is inert here: + # _web_fetch_call raises ToolInputError on an empty url before any + # network I/O (and dispatch schema validation rejects a missing one). return PermissionPassthroughResult() try: parsed = urllib.parse.urlparse(url) hostname = parsed.hostname or "" pathname = parsed.path or "/" if _is_preapproved(hostname, pathname): - return PermissionPassthroughResult() + return PermissionAllowDecision(behavior="allow", updated_input=tool_input) except Exception: pass - return PermissionPassthroughResult() + return PermissionAskDecision( + message=f"Claude wants to fetch {url}. Allow?", + ) # -- Result Mapping ------------------------------------------------------------