Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 93 additions & 6 deletions cli/src/policy_federation/claude_hooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import sys
from pathlib import Path

from .interceptor import intercept_command
from .interceptor import DENY_EXIT_CODE, intercept_command
from .runtime_context import infer_repo_name_from_cwd

READ_ONLY_TOOLS = {"Glob", "Grep", "LS", "Read"}
Expand All @@ -27,7 +27,7 @@
re.DOTALL,
),
),
("shell-redirect-write", re.compile(r"(?:^|&&|;|\|)\s*[^2]?[>]\s*/", re.MULTILINE)),
("shell-redirect-write", re.compile(r"(?:^|&&|;|\|)\s*[^2]?>\s*/|(?<![12=&])>\s*/\S", re.MULTILINE)),
("tee-write", re.compile(r"\btee\b")),
("dd-write", re.compile(r"\bdd\b.*\bof=")),
("heredoc-write", re.compile(r'<<\s*[\'"]?EOF')),
Expand Down Expand Up @@ -198,10 +198,19 @@

def _resolve_target_path(target_path: str, cwd: str) -> str:
"""Resolve a possibly-relative target path against the command cwd."""
path = Path(target_path)
if path.is_absolute():
return str(path)
return str((Path(cwd) / path).resolve())
normalized_target = target_path.replace("\\", "/")
normalized_cwd = cwd.replace("\\", "/")

if normalized_target.startswith("/"):
return normalized_target

if len(normalized_target) > 1 and normalized_target[1] == ":":
return Path(normalized_target).as_posix()

if normalized_cwd.startswith("/"):
return f"{normalized_cwd.rstrip('/')}/{normalized_target.lstrip('/')}"
Comment on lines +210 to +211

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Relative target paths are now concatenated with the cwd without normalization, so traversal segments like ../ are preserved. This lets writes such as ../../etc/shadow avoid sensitive-prefix detection because the resulting string starts with the cwd instead of /etc/. Resolve and normalize the joined path before policy checks so traversal cannot bypass the deny logic. [security]

Severity Level: Critical 🚨
- ❌ Sensitive writes under /etc bypass prefix-based blocking.
- ❌ Claude Bash hooks fail to block ../etc writes.
- ⚠️ Enforcement logs misrepresent actual sensitive filesystem targets.
Steps of Reproduction ✅
1. Run the Claude pre-tool hook entrypoint `main()` in
`cli/src/policy_federation/claude_hooks.py:116-124`, which reads a JSON payload from stdin
and calls `evaluate_claude_pretool_payload(payload)` at `claude_hooks.py:389`.

2. Provide a payload with `tool_name: "Bash"`, `cwd: "/tmp/worktrees/repo"` and
`tool_input["command"]` set to `python -c "open('../../etc/shadow','w').write('x')"` so
that `_extract_request()` at `claude_hooks.py:248-297` classifies this as a write-via-exec
and extracts `target_paths` using the `open()` regex at `claude_hooks.py:283-285`.

3. Inside `_extract_request()`, the `open()` match `m.group(1)` is `"../../etc/shadow"`
and `_resolve_target_path(m.group(1), resolved_cwd)` is called at `claude_hooks.py:284`
with `resolved_cwd="/tmp/worktrees/repo"`; `_resolve_target_path()` at
`claude_hooks.py:199-213` takes the branch `if normalized_cwd.startswith("/")` and returns
the non-canonical string `"/tmp/worktrees/repo/../../etc/shadow"` (line 210-211),
preserving the `"../"` segments.

4. Back in `evaluate_claude_pretool_payload()` at `claude_hooks.py:389-427`, the `result`
from `intercept_command()` is post-processed by `_should_deny_unapproved_write()` at
`claude_hooks.py:372-387`; this function computes `paths = target_paths or [cwd]` (here
`["/tmp/worktrees/repo/../../etc/shadow"]`) and calls `_targets_sensitive_write_paths()`
at `claude_hooks.py:364-369`, which checks `path.startswith(prefix)` for prefixes like
`"/etc/"`, `"/var/"`, `"/tmp/"` but never normalizes `"../"` segments, so
`"/tmp/worktrees/repo/../../etc/shadow"` is not recognized as starting with `"/etc/"` and
the synthetic deny rule is not triggered even though the real filesystem target resolves
to `/etc/shadow`.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** cli/src/policy_federation/claude_hooks.py
**Line:** 210:211
**Comment:**
	*Security: Relative target paths are now concatenated with the cwd without normalization, so traversal segments like `../` are preserved. This lets writes such as `../../etc/shadow` avoid sensitive-prefix detection because the resulting string starts with the cwd instead of `/etc/`. Resolve and normalize the joined path before policy checks so traversal cannot bypass the deny logic.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎


return str((Path(cwd) / target_path).resolve().as_posix())


def _extract_sed_target_paths(command: str, cwd: str) -> list[str]:
Expand Down Expand Up @@ -328,6 +337,55 @@
}


_SENSITIVE_WRITE_PREFIXES = (
"/etc/",
"/usr/",
"/bin/",
"/sbin/",
"/var/",
"/sys/",
"/proc/",
"/tmp/",

Check failure on line 348 in cli/src/policy_federation/claude_hooks.py

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Make sure publicly writable directories are used safely here.

See more on https://sonarcloud.io/project/issues?id=KooshaPari_PolicyStack&issues=AZ6-XqTWikltiDWnYBWx&open=AZ6-XqTWikltiDWnYBWx&pullRequest=54
)


def _normalize_policy_path(path: str) -> str:
return path.replace("\\", "/")


def _is_worktree_path(path: str) -> bool:
normalized = _normalize_policy_path(path)
return any(
marker in normalized
for marker in ("-wtrees/", "/.worktrees/", "/worktrees/", "PROJECT-wtrees/")
)
Comment on lines +356 to +361

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Worktree detection uses substring matching on arbitrary path text, so attacker-controlled paths like /tmp/worktrees/evil.txt are treated as approved worktree paths even when they are outside any real approved checkout. This can suppress the synthetic deny for sensitive writes; replace this with strict path-root validation against known approved worktree directories. [incorrect condition logic]

Severity Level: Critical 🚨
- ❌ Synthetic deny skipped for /tmp/worktrees outside repos.
- ❌ Attackers craft paths mimicking worktrees to avoid checks.
- ⚠️ Risk assessment treats untrusted temporary directories as worktrees.
Steps of Reproduction ✅
1. Note the worktree detection helper `_is_worktree_path()` in
`cli/src/policy_federation/claude_hooks.py:356-361`, which normalizes separators then
returns `True` if any of the markers `"-wtrees/"`, `"/.worktrees/"`, `"/worktrees/"`, or
`"PROJECT-wtrees/"` appear as a substring anywhere in the path, without tying this to an
actual repository root or approved worktree list.

2. Observe `_should_deny_unapproved_write()` at `claude_hooks.py:372-387`, which is called
from `evaluate_claude_pretool_payload()` at `claude_hooks.py:389-427` after
`intercept_command()` returns, and is responsible for applying a synthetic deny when
`action == "write"`, `final_decision == "ask"`, and the target paths hit sensitive
prefixes listed in `_SENSITIVE_WRITE_PREFIXES` at `claude_hooks.py:340-349`.

3. Supply a payload to `main()` (`claude_hooks.py:116-124`) where `_extract_request()` at
`claude_hooks.py:307-320` sets `action: "write"` and `target_paths:
["/tmp/worktrees/evil.txt"]` (for example by using a write tool targeting
`/tmp/worktrees/evil.txt`) so that `evaluate_claude_pretool_payload()` later calls
`_should_deny_unapproved_write(action="write", target_paths=["/tmp/worktrees/evil.txt"],
cwd="/tmp", final_decision="ask")`.

4. Inside `_should_deny_unapproved_write()`, `paths` becomes `["/tmp/worktrees/evil.txt"]`
and the guard `if paths and all(_is_worktree_path(path) for path in paths): return False`
at `claude_hooks.py:382-384` calls `_is_worktree_path("/tmp/worktrees/evil.txt")`, which
returns `True` because the substring `"/worktrees/"` is present, so
`_should_deny_unapproved_write()` exits early and never calls
`_targets_sensitive_write_paths()` at `claude_hooks.py:364-369`; as a result, the
synthetic deny for sensitive prefixes such as `"/tmp/"` is skipped even though
`/tmp/worktrees/evil.txt` is not an approved repository worktree path.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** cli/src/policy_federation/claude_hooks.py
**Line:** 356:361
**Comment:**
	*Incorrect Condition Logic: Worktree detection uses substring matching on arbitrary path text, so attacker-controlled paths like `/tmp/worktrees/evil.txt` are treated as approved worktree paths even when they are outside any real approved checkout. This can suppress the synthetic deny for sensitive writes; replace this with strict path-root validation against known approved worktree directories.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎



def _targets_sensitive_write_paths(target_paths: list[str]) -> bool:
for raw in target_paths:
path = _normalize_policy_path(raw)
if any(path.startswith(prefix) for prefix in _SENSITIVE_WRITE_PREFIXES):
return True

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows paths bypass sensitive deny

Medium Severity

_SENSITIVE_WRITE_PREFIXES and _targets_sensitive_write_paths only recognize Unix-style absolute prefixes like /tmp/ and /etc/. On Windows, writes to typical sensitive locations (for example C:/Windows/... or user Temp paths) never match, so the new ask-to-deny hardening does not apply despite cross-platform path work elsewhere in the PR.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 261103c. Configure here.

return False


def _should_deny_unapproved_write(
*,
action: str,
target_paths: list[str],
cwd: str,
final_decision: str,
) -> bool:
if action != "write" or final_decision != "ask":
return False

paths = target_paths or [cwd]
if paths and all(_is_worktree_path(path) for path in paths):
return False

return _targets_sensitive_write_paths(paths)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sensitive deny skips resolved paths

High Severity

The new _should_deny_unapproved_write gate checks raw target_paths against Unix /etc/, /tmp/, etc., but Write-tool paths are passed through unchanged and never run through _resolve_target_path. Relative targets such as ../etc/passwd can stay in ask instead of being upgraded to deny even when they resolve outside approved worktrees to sensitive locations.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 261103c. Configure here.



def evaluate_claude_pretool_payload(
payload: dict, repo_root: Path | None = None,
) -> dict:
Expand All @@ -352,6 +410,35 @@
ask_mode=os.environ.get("POLICY_ASK_MODE", "fail"),
)

if request.get("bypass_indicators") and result["final_decision"] == "ask":
result = {
**result,
"allowed": False,
"exit_code": DENY_EXIT_CODE,
"final_decision": "deny",
}

if _should_deny_unapproved_write(
action=request["action"],
target_paths=request.get("target_paths", []),
cwd=cwd,
final_decision=result["final_decision"],
):
evaluation = dict(result.get("evaluation") or {})
evaluation["winning_rule"] = {
"id": "deny-write-outside-worktrees",
"effect": "deny",
"priority": 0,
"description": "Write blocked outside approved worktrees to sensitive targets",
}
result = {
**result,
"allowed": False,
"exit_code": DENY_EXIT_CODE,
"final_decision": "deny",
"evaluation": evaluation,
}

if result["final_decision"] == "allow":
evaluation = result.get("evaluation") or {}
headless_review = evaluation.get("headless_review")
Expand Down
4 changes: 3 additions & 1 deletion cli/src/policy_federation/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ def _default_audit_log_path() -> Path | None:


def _emit_json(payload: object) -> None:
pass
import json

print(json.dumps(payload, ensure_ascii=True, sort_keys=True))


def resolve_command(args: argparse.Namespace) -> None:
Expand Down
7 changes: 5 additions & 2 deletions cli/src/policy_federation/config_loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
from pathlib import Path
from typing import Any

from .delegate import _auto_detect_harness


@dataclass
class PlatformConfig:
Expand All @@ -30,6 +32,7 @@ class RiskTierConfig:
enabled: bool = True
extra_patterns: list[str] = field(default_factory=list)
patterns: list[str] = field(default_factory=list)
max_risk_score: float | None = None


@dataclass
Expand Down Expand Up @@ -244,6 +247,8 @@ def _merge_dict_into_config(config: PolicyConfig, data: dict[str, Any]) -> None:
tier_config.extra_patterns = tier_data["extra_patterns"]
if "patterns" in tier_data:
tier_config.patterns = tier_data["patterns"]
if "max_risk_score" in tier_data:
tier_config.max_risk_score = tier_data["max_risk_score"]

if "cache" in data:
cache_data = data["cache"]
Expand Down Expand Up @@ -296,8 +301,6 @@ def get_active_harness(config: PolicyConfig) -> str:
return config.platform.preferred_harness

# Auto-detect from environment
from .delegate import _auto_detect_harness

detected = _auto_detect_harness()
if detected:
return detected
Expand Down
Loading
Loading