Skip to content

Commit 7f37580

Browse files
AdamMaguedclaude
andcommitted
fix(agent): make the client-side HITL hard-block self-enforcing (security)
_execute_tool's hard-block path (hitl_block_high_risk / KIM_HITL_BLOCK_HIGH_RISK) used to `pass` straight through to tool execution, on the assumption that the MCP server's own, separately-configured gate (a different config key — mcp_server's hitl_risk_threshold / KIM_HITL_RISK_THRESHOLD) would independently also classify this exact tool+args as needing approval and pause on the ApprovalBroker. That assumption doesn't hold in general: the server-side threshold/classification can disagree with the client-side one, so a tool the user believed was hard-blocked could execute with a human never actually asked. Mechanism: whenever InteractionPolicy.before_tool() hard-blocks with an HITL_REQUIRED message, _execute_tool now invokes the SAME resolver _make_approval_resolver() already builds for server-originated K1 approval requests — reusing its id generation, hitl_approval_request/result events, preview-mode auto-accept (preview's blanket confirm_action already asked about this exact call before _execute_tool was reached), and "no bridge -> decline" fail-closed behavior. Only "accept"/"acceptForSession" lets execution continue; anything else returns the blocking message, exactly like every other policy block. There is now exactly one approval mechanism in this process instead of two that could silently disagree. Non-HITL hard blocks (e.g. stale/unknown element_id) are untouched — they keep returning their message unconditionally, gated by the same "HITL_REQUIRED in message" check the old code used to select this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 56fa96a commit 7f37580

2 files changed

Lines changed: 131 additions & 12 deletions

File tree

‎orchestrator/agent.py‎

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1307,18 +1307,43 @@ async def _execute_tool(self, name: str, args: dict) -> str:
13071307
self._log(level, f"[POLICY] {decision.message}")
13081308

13091309
if not decision.allowed:
1310-
# HITL hard-blocks (block_high_risk mode) defer to the interactive
1311-
# server-side gate when one can actually fire (threshold + bridge,
1312-
# same conditions the old agent-side gate used): the MCP server
1313-
# pauses on the approval broker and the user decides. All other
1314-
# policy blocks (staleness, unknown IDs…) are enforced
1315-
# unconditionally.
1316-
if (decision.hard_block
1317-
and "HITL_REQUIRED" in decision.message
1318-
and self._hitl_risk_threshold
1319-
and self._ui_bridge is not None
1320-
and not self._is_preview_mode()):
1321-
pass
1310+
if decision.hard_block and "HITL_REQUIRED" in decision.message:
1311+
# Client-side hard-block (hitl_block_high_risk /
1312+
# KIM_HITL_BLOCK_HIGH_RISK). This used to fall through
1313+
# un-executed on the ASSUMPTION that the MCP server's own,
1314+
# separately-configured policy (mcp_server/policy.py, its
1315+
# own hitl_risk_threshold / KIM_HITL_RISK_THRESHOLD) would
1316+
# independently also classify this exact tool+args as
1317+
# needing approval and pause on the ApprovalBroker — but
1318+
# that server-side gate can disagree with this client-side
1319+
# one (different config key, different classification), so
1320+
# a tool the user believed was hard-blocked could execute
1321+
# with a human never actually asked (finding 2).
1322+
#
1323+
# This side now enforces its own block unconditionally: it
1324+
# invokes the SAME resolver _make_approval_resolver() builds
1325+
# for server-originated HITL requests (K1) — the identical
1326+
# id generation, hitl_approval_request/result events,
1327+
# preview-mode auto-accept (preview's blanket
1328+
# confirm_action already asked the user about this exact
1329+
# call before _execute_tool was reached — see the call
1330+
# sites above), and "no bridge → decline" fail-closed
1331+
# behavior — so there is exactly one approval mechanism in
1332+
# this process, not two that can disagree. Only a decision
1333+
# of "accept"/"acceptForSession" lets execution continue
1334+
# below; anything else (decline, no bridge) returns the
1335+
# blocking message like every other policy block.
1336+
resolver = self._make_approval_resolver()
1337+
hitl_decision = await resolver({
1338+
"tool": name,
1339+
"risk": "high",
1340+
"reason": decision.message,
1341+
"preview": json.dumps(args or {}, default=str)[:200],
1342+
"args": args or {},
1343+
})
1344+
if hitl_decision not in ("accept", "acceptForSession"):
1345+
return decision.message
1346+
# Approved — fall through and execute the tool below.
13221347
else:
13231348
return decision.message
13241349

‎tests/test_hitl_approval.py‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,5 +251,99 @@ def test_emits_request_and_result_events(self):
251251
self.assertTrue(result["approved"])
252252

253253

254+
# ---------------------------------------------------------------------------
255+
# _execute_tool's hard-block enforcement (finding 2)
256+
# ---------------------------------------------------------------------------
257+
#
258+
# Previously, when the client-side InteractionPolicy hard-blocked a tool
259+
# (hitl_block_high_risk / KIM_HITL_BLOCK_HIGH_RISK), _execute_tool fell
260+
# through un-executed WITHOUT ever calling the UI bridge, on the assumption
261+
# that the MCP server's own, separately-configured gate (a different config
262+
# key: mcp_server hitl_risk_threshold) would independently also classify
263+
# this tool+args as needing approval and pause on the ApprovalBroker. That
264+
# assumption doesn't hold in general, so a "hard-blocked" tool could execute
265+
# with a human never actually asked. These tests pin the fixed behavior:
266+
# _execute_tool now always resolves the hard-block itself through the same
267+
# resolver _make_approval_resolver() builds for server-originated requests.
268+
269+
270+
class ExecuteToolHardBlockTests(unittest.TestCase):
271+
272+
def test_hard_block_asks_the_ui_bridge_and_executes_on_accept(self):
273+
agent = _make_agent(block_high_risk=True)
274+
agent._ui_bridge.decide_action = AsyncMock(return_value="accept")
275+
result = _run(agent._execute_tool("run_command", {"cmd": "echo hi"}))
276+
self.assertEqual(result, "tool_output")
277+
agent._ui_bridge.decide_action.assert_called_once()
278+
call_args, _ = agent._ui_bridge.decide_action.call_args
279+
self.assertEqual(call_args[0], "run_command")
280+
agent.session.call_tool.assert_called_once()
281+
282+
def test_hard_block_denies_execution_on_decline(self):
283+
agent = _make_agent(block_high_risk=True)
284+
agent._ui_bridge.decide_action = AsyncMock(return_value="decline")
285+
result = _run(agent._execute_tool("run_command", {"cmd": "echo hi"}))
286+
self.assertIn("HITL_REQUIRED", result)
287+
agent._ui_bridge.decide_action.assert_called_once()
288+
agent.session.call_tool.assert_not_called()
289+
290+
def test_hard_block_accept_for_session_also_executes(self):
291+
agent = _make_agent(block_high_risk=True)
292+
agent._ui_bridge.decide_action = AsyncMock(return_value="acceptForSession")
293+
result = _run(agent._execute_tool("run_command", {"cmd": "echo hi"}))
294+
self.assertEqual(result, "tool_output")
295+
agent.session.call_tool.assert_called_once()
296+
297+
def test_hard_block_without_ui_bridge_fails_closed(self):
298+
# No UI bridge means nothing can ask a human — the resolver declines
299+
# (bridge is None) and the tool must NOT execute.
300+
agent = _make_agent(block_high_risk=True, has_bridge=False)
301+
result = _run(agent._execute_tool("run_command", {"cmd": "echo hi"}))
302+
self.assertIn("HITL_REQUIRED", result)
303+
agent.session.call_tool.assert_not_called()
304+
305+
def test_hard_block_fires_regardless_of_hitl_risk_threshold(self):
306+
# THE regression this finding is about: the old fallthrough also
307+
# required self._hitl_risk_threshold to be truthy — an unrelated,
308+
# separately-configured value (the MCP server's OWN gate) that has
309+
# nothing to do with whether this client-side hard-block should ask
310+
# for approval. With it unset (None), the tool must still go
311+
# through the approval gate — and be blocked on decline — instead
312+
# of silently executing.
313+
agent = _make_agent(block_high_risk=True, hitl_threshold=None)
314+
agent._ui_bridge.decide_action = AsyncMock(return_value="decline")
315+
result = _run(agent._execute_tool("run_command", {"cmd": "echo hi"}))
316+
self.assertIn("HITL_REQUIRED", result)
317+
agent._ui_bridge.decide_action.assert_called_once()
318+
agent.session.call_tool.assert_not_called()
319+
320+
def test_hard_block_preview_mode_auto_accepts_without_double_prompt(self):
321+
# Preview mode's blanket confirm_action already asked about this
322+
# exact call before _execute_tool was reached; the shared resolver's
323+
# own preview auto-accept (same as the server-originated path) must
324+
# let it through without asking a second time.
325+
agent = _make_agent(block_high_risk=True, preview_mode=True)
326+
agent._ui_bridge.decide_action = AsyncMock(return_value="decline")
327+
result = _run(agent._execute_tool("run_command", {"cmd": "echo hi"}))
328+
self.assertEqual(result, "tool_output")
329+
agent._ui_bridge.decide_action.assert_not_called()
330+
agent.session.call_tool.assert_called_once()
331+
332+
def test_non_hard_block_policy_denial_still_returns_message_unconditionally(self):
333+
# Non-HITL hard blocks (e.g. InteractionPolicy's stale/unknown
334+
# element_id POLICY_BLOCK for web_click) are NOT approval requests
335+
# and must keep being enforced unconditionally, with no detour
336+
# through the UI bridge — the "HITL_REQUIRED" in decision.message
337+
# check keeps this path (hard_block=True, but a different message)
338+
# from being routed through the new approval-resolver detour.
339+
agent = _make_agent(block_high_risk=False)
340+
agent._ui_bridge.decide_action = AsyncMock(return_value="accept")
341+
agent._interaction_policy.web_generation = 1 # simulate a prior web_observe
342+
result = _run(agent._execute_tool("web_click", {"element_id": "w99"}))
343+
self.assertIn("POLICY_BLOCK", result)
344+
agent._ui_bridge.decide_action.assert_not_called()
345+
agent.session.call_tool.assert_not_called()
346+
347+
254348
if __name__ == "__main__":
255349
unittest.main()

0 commit comments

Comments
 (0)