fix security redundant denylist and refusal message always falling back to default

This commit is contained in:
Roger Oriol
2026-07-26 21:27:35 +02:00
parent 7ff6bf858d
commit f9a11bf4ca
2 changed files with 35 additions and 25 deletions

View File

@@ -148,14 +148,16 @@ def check_permission(
# Layer 2: always-confirm & irreversible patterns. # Layer 2: always-confirm & irreversible patterns.
# These override even dangerouslySkipPermissions for the worst cases. # These override even dangerouslySkipPermissions for the worst cases.
if tool_policy.always_confirm_required(tool_name, args): confirm_required, confirm_reason = tool_policy.always_confirm_required(
tool_name, args)
if confirm_required:
if mode == PermissionMode.DANGEROUSLY_SKIP_PERMISSIONS: if mode == PermissionMode.DANGEROUSLY_SKIP_PERMISSIONS:
# Irreversible calls are refused outright in skip mode; the # Irreversible calls are refused outright in skip mode; the
# user explicitly accepted risk for normal destructive ops, # user explicitly accepted risk for normal destructive ops,
# but not for e.g. `rm -rf /` or `git push --force`. # but not for e.g. `git push --force`.
return False, ( return False, (
f"Blocked even in dangerouslySkipPermissions: { f"Blocked even in dangerouslySkipPermissions: {
reason or 'irreversible action'}. " confirm_reason or 'irreversible action'}. "
"Run this command manually outside the agent if it is truly intended." "Run this command manually outside the agent if it is truly intended."
) )
# In default / acceptEdits, force an explicit prompt. # In default / acceptEdits, force an explicit prompt.

View File

@@ -54,24 +54,23 @@ ALWAYS_CONFIRM_TOOLS = frozenset({
}) })
# Argument patterns that make an otherwise-allowed tool require # Argument patterns that make an otherwise-allowed tool require
# confirmation regardless of mode. Each entry is (tool_name, regex). # confirmation regardless of mode. Each entry is (tool_name, regex, reason).
# The regex is matched (case-insensitive) against the JSON-serialized # The regex is matched (case-insensitive) against the JSON-serialized
# args dict. # args dict.
ALWAYS_CONFIRM_ARG_PATTERNS: list[tuple[str, re.Pattern[str]]] = [ #
# rm -rf with a broad or root target # NOTE: for ``run_bash``, anything already covered by
("run_bash", re.compile(r"\brm\s+-rf?\s+(/|~|\*|\$HOME|\.\.)", re.I)), # ``SHELL_DENYLIST_BINARIES`` or ``SHELL_DENYLIST_PATTERNS`` (rm -rf,
# force-push to git # sudo/su, docker, chmod, curl/wget/nc/netcat/ncat) is a hard block in
("run_bash", re.compile(r"\bgit\s+push\s+(-f|--force)", re.I)), # layer 1 (``check_tool_policy``) and never reaches this layer, so it
# privilege escalation # is deliberately NOT duplicated here. Only patterns that layer 1
("run_bash", re.compile(r"\bsudo\b|\bsu\b\s+", re.I)), # does *not* already hard-block belong in this list.
# any use of docker from inside the sandbox (escape risk) ALWAYS_CONFIRM_ARG_PATTERNS: list[tuple[str, re.Pattern[str], str]] = [
("run_bash", re.compile(r"\bdocker\b", re.I)), # force-push to git (git itself is not on the shell denylist)
# chmod world-writable ("run_bash", re.compile(r"\bgit\s+push\s+(-f|--force)", re.I),
("run_bash", re.compile(r"\bchmod\s+[-\d]*7\d\d?", re.I)), "force-push to git (rewrites remote history)"),
# network exfiltration tools
("run_bash", re.compile(r"\b(curl|wget|nc|netcat|ncat)\b", re.I)),
# overwriting a file with empty content = delete # overwriting a file with empty content = delete
("write_file", re.compile(r'"content"\s*:\s*"\s*"')), ("write_file", re.compile(r'"content"\s*:\s*"\s*"'),
"write_file with empty content on an existing file (delete-via-empty)"),
] ]
@@ -79,7 +78,7 @@ def is_destructive(tool_name: str, args: dict[str, Any]) -> bool:
"""Return True if the call is in the destructive class.""" """Return True if the call is in the destructive class."""
if tool_name in DESTRUCTIVE_TOOLS: if tool_name in DESTRUCTIVE_TOOLS:
return True return True
for name, pat in ALWAYS_CONFIRM_ARG_PATTERNS: for name, pat, _reason in ALWAYS_CONFIRM_ARG_PATTERNS:
if name == tool_name and pat.search(_args_blob(args)): if name == tool_name and pat.search(_args_blob(args)):
return True return True
# write_file emptying an existing file is treated as a delete in # write_file emptying an existing file is treated as a delete in
@@ -87,14 +86,23 @@ def is_destructive(tool_name: str, args: dict[str, Any]) -> bool:
return False return False
def always_confirm_required(tool_name: str, args: dict[str, Any]) -> bool: def always_confirm_required(
"""Return True if the call must prompt the user regardless of mode.""" tool_name: str, args: dict[str, Any]
) -> tuple[bool, str | None]:
"""Return (True, reason) if the call must prompt the user regardless
of mode, else (False, None).
Note: for ``run_bash`` this only needs to catch cases NOT already
hard-blocked by the shell policy (layer 1); anything on
``SHELL_DENYLIST_BINARIES`` / ``SHELL_DENYLIST_PATTERNS`` never
reaches this function at all.
"""
if tool_name in ALWAYS_CONFIRM_TOOLS: if tool_name in ALWAYS_CONFIRM_TOOLS:
return True return True, f"'{tool_name}' always requires confirmation."
for name, pat in ALWAYS_CONFIRM_ARG_PATTERNS: for name, pat, reason in ALWAYS_CONFIRM_ARG_PATTERNS:
if name == tool_name and pat.search(_args_blob(args)): if name == tool_name and pat.search(_args_blob(args)):
return True return True, reason
return False return False, None
def _args_blob(args: dict[str, Any]) -> str: def _args_blob(args: dict[str, Any]) -> str: