feat(#630): retire blanket rm deny + aipass doctor migration
Phase 2 of #630. The rm_gate hook + drone rm now own destructive-delete protection (cross-provider, path-aware, teaching), so the Claude-only blanket deny is redundant AND harmful (it short-circuits before the hook, suppressing the teaching message). - setup.sh: removed Bash(rm -rf*) + Bash(rm -r *) from git_deny (new installs) - bootstrap.py: removed Bash(rm -rf *) shipped via aipass init (project settings) - doctor_wire.reconcile_stale_deny(): aipass doctor WARNs on stale rules; --fix removes them (idempotent, preserves all else) — migration for existing installs (the 'aipass update should be trusted' goal) - .aipass/project_hooks.json template: added rm_gate (new projects get it) Tests: 8 reconcile + 432 aipass total, seedgo 99%. CHANGELOG W23.
This commit is contained in:
@@ -40,6 +40,11 @@
|
||||
"enabled": true,
|
||||
"handler": "aipass.hooks.apps.handlers.security.git_gate.handle",
|
||||
"matcher": "Bash|Edit|MultiEdit|Write|NotebookEdit"
|
||||
},
|
||||
"rm_gate": {
|
||||
"enabled": true,
|
||||
"handler": "aipass.hooks.apps.handlers.security.rm_gate.handle",
|
||||
"matcher": "Bash"
|
||||
}
|
||||
},
|
||||
|
||||
|
||||
@@ -28,6 +28,17 @@ and this project uses [Calendar Versioning](https://calver.org/) in the format
|
||||
(unparseable targets are blocked, not allowed), and skips `drone rm` itself.
|
||||
This makes the safe-delete path discoverable at the moment of friction. (#630)
|
||||
|
||||
### Changed
|
||||
|
||||
- **Retired the blanket `rm` deny from provider settings** — `setup.sh` and
|
||||
`aipass init` no longer ship `Bash(rm -rf*)` / `Bash(rm -r *)` deny rules
|
||||
(they were mis-filed among git rules, blocked all `/tmp` cleanup, and gave a
|
||||
bare "permission denied" with no guidance). The `rm_gate` hook + `drone rm`
|
||||
now own this — cross-provider, path-aware, and they teach. `aipass doctor`
|
||||
detects the stale rules on existing installs and `aipass doctor --fix` removes
|
||||
them (idempotent, preserves all other rules). Claude Code still natively
|
||||
circuit-breaks `rm -rf /` and `rm -rf ~`. (#630)
|
||||
|
||||
### Fixed
|
||||
|
||||
- **A release merge can no longer destroy the `dev` branch** — `drone @git merge`
|
||||
|
||||
@@ -606,7 +606,6 @@ git_deny = [
|
||||
"Bash(git push -f *)",
|
||||
"Bash(git rebase*)",
|
||||
"Bash(git clean*)",
|
||||
"Bash(rm -rf*)",
|
||||
"Bash(git reset*)",
|
||||
"Bash(git merge*)",
|
||||
"Bash(git config*)",
|
||||
@@ -617,7 +616,6 @@ git_deny = [
|
||||
"Bash(git branch -D*)",
|
||||
"Bash(git stash drop*)",
|
||||
"Bash(git stash clear*)",
|
||||
"Bash(rm -r *)",
|
||||
"Bash(git checkout -b*)",
|
||||
"Bash(git switch -c*)",
|
||||
"Bash(git switch --create*)",
|
||||
|
||||
@@ -213,7 +213,6 @@ def _claude_settings(aipass_home: str | None = None) -> str:
|
||||
|
||||
data["permissions"] = {
|
||||
"deny": [
|
||||
"Bash(rm -rf *)",
|
||||
"Bash(git push --force*)",
|
||||
"Bash(git reset --hard*)",
|
||||
"EnterPlanMode",
|
||||
|
||||
@@ -38,6 +38,7 @@ from aipass.aipass.apps.modules.doctor_fix import (
|
||||
from aipass.aipass.apps.modules.doctor_wire import (
|
||||
_auto_wire_provider,
|
||||
prompt_auto_wire,
|
||||
reconcile_stale_deny,
|
||||
)
|
||||
from aipass.aipass.apps.handlers.system_detect.system_detector import (
|
||||
detect_cpu,
|
||||
@@ -68,9 +69,6 @@ class CheckResult(NamedTuple):
|
||||
remediation: str
|
||||
|
||||
|
||||
# --- Identity helpers ---
|
||||
|
||||
|
||||
def _find_registry() -> Path | None:
|
||||
"""Walk up from CWD first (user's project), then branch root."""
|
||||
cwd = Path.cwd()
|
||||
@@ -87,9 +85,6 @@ def _find_registry() -> Path | None:
|
||||
return None
|
||||
|
||||
|
||||
# --- Check groups ---
|
||||
|
||||
|
||||
def _check_system() -> List[CheckResult]:
|
||||
"""Run System group checks."""
|
||||
results: List[CheckResult] = []
|
||||
@@ -147,11 +142,10 @@ def _check_identity() -> List[CheckResult]:
|
||||
"""Run Identity group checks."""
|
||||
results: List[CheckResult] = []
|
||||
|
||||
# Project root — derived from registry location
|
||||
reg = _find_registry()
|
||||
project_root = str(reg.parent) if reg else ""
|
||||
if project_root:
|
||||
results.append(CheckResult("AIPASS_HOME", GLYPH_PASS, project_root, ""))
|
||||
# Project root + registry — single lookup
|
||||
reg_path = _find_registry()
|
||||
if reg_path:
|
||||
results.append(CheckResult("AIPASS_HOME", GLYPH_PASS, str(reg_path.parent), ""))
|
||||
else:
|
||||
home = os.environ.get("AIPASS_HOME", "")
|
||||
if home:
|
||||
@@ -166,8 +160,6 @@ def _check_identity() -> List[CheckResult]:
|
||||
)
|
||||
)
|
||||
|
||||
# Registry present
|
||||
reg_path = _find_registry()
|
||||
if reg_path is None:
|
||||
results.append(CheckResult("registry", GLYPH_FAIL, "not found", "Run 'aipass init' to create registry"))
|
||||
return results
|
||||
@@ -451,6 +443,10 @@ def _check_services(verbose: bool = False) -> List[CheckResult]:
|
||||
manifest_checks = _check_provider_manifest()
|
||||
results.extend(manifest_checks)
|
||||
|
||||
# stale rm deny rules — detect only (fix runs in run_doctor when --fix)
|
||||
for tup in reconcile_stale_deny(fix=False):
|
||||
results.append(CheckResult(*tup))
|
||||
|
||||
return results
|
||||
|
||||
|
||||
@@ -588,6 +584,12 @@ def run_doctor(verbose: bool = False, interactive: bool = False, fix: bool = Fal
|
||||
r for r in groups.get("Services", []) if r.label not in ("hooks", "env vars", "permissions")
|
||||
] + manifest_results
|
||||
|
||||
if fix:
|
||||
stale_results = [CheckResult(*tup) for tup in reconcile_stale_deny(fix=True)]
|
||||
if stale_results:
|
||||
services = groups.get("Services", [])
|
||||
groups["Services"] = [r for r in services if r.label != "rm deny migration"] + stale_results
|
||||
|
||||
pass_count = 0
|
||||
warn_count = 0
|
||||
error_count = 0
|
||||
|
||||
@@ -51,6 +51,56 @@ ENV_DESCRIPTIONS: Dict[str, str] = {
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# STALE DENY RULE MIGRATION
|
||||
# =============================================================================
|
||||
|
||||
_STALE_RM_DENY_RULES = frozenset({"Bash(rm -rf*)", "Bash(rm -r *)"})
|
||||
|
||||
|
||||
def reconcile_stale_deny(fix: bool = False) -> list:
|
||||
"""Detect and optionally remove stale rm deny rules from provider settings.
|
||||
|
||||
Returns list of (label, glyph, detail, remediation) tuples matching
|
||||
doctor.CheckResult shape — imported as tuples to avoid circular import.
|
||||
"""
|
||||
from aipass.aipass.apps.handlers.ui.progress import GLYPH_PASS, GLYPH_WARN
|
||||
|
||||
results: list = []
|
||||
settings_path = Path.home() / ".claude" / "settings.json"
|
||||
if not settings_path.exists():
|
||||
return results
|
||||
|
||||
data = json_handler.load_path(settings_path)
|
||||
if data is None:
|
||||
return results
|
||||
|
||||
deny = data.get("permissions", {}).get("deny", [])
|
||||
stale = [r for r in deny if r in _STALE_RM_DENY_RULES]
|
||||
|
||||
if not stale:
|
||||
results.append(("rm deny migration", GLYPH_PASS, "no stale rules", ""))
|
||||
return results
|
||||
|
||||
if fix:
|
||||
deny_cleaned = [r for r in deny if r not in _STALE_RM_DENY_RULES]
|
||||
data.setdefault("permissions", {})["deny"] = deny_cleaned
|
||||
json_handler.save_path(settings_path, data)
|
||||
removed = ", ".join(stale)
|
||||
results.append(("rm deny migration", GLYPH_PASS, f"removed: {removed}", ""))
|
||||
logger.info("[doctor] removed stale deny rules: %s", stale)
|
||||
else:
|
||||
found = ", ".join(stale)
|
||||
results.append(
|
||||
(
|
||||
"rm deny migration",
|
||||
GLYPH_WARN,
|
||||
f"stale rules: {found}",
|
||||
"Run aipass doctor --fix to remove (rm_gate + drone rm replace these)",
|
||||
)
|
||||
)
|
||||
|
||||
return results
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# AUTO-WIRE
|
||||
|
||||
@@ -612,3 +612,133 @@ class TestHooksJsonCheck:
|
||||
assert len(hooks_results) == 1
|
||||
assert hooks_results[0].glyph == GLYPH_WARN
|
||||
assert "init update" in hooks_results[0].remediation
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# TestReconcileStaleDeny
|
||||
# =============================================================================
|
||||
|
||||
|
||||
class TestReconcileStaleDeny:
|
||||
"""Tests for stale rm deny rule migration (DPLAN-0192 Phase 2)."""
|
||||
|
||||
def test_no_settings_file_returns_empty(self, tmp_path) -> None:
|
||||
"""Missing settings.json returns no results."""
|
||||
from aipass.aipass.apps.modules.doctor_wire import reconcile_stale_deny
|
||||
|
||||
with patch("aipass.aipass.apps.modules.doctor_wire.Path.home", return_value=tmp_path):
|
||||
results = reconcile_stale_deny(fix=False)
|
||||
assert results == []
|
||||
|
||||
def test_no_stale_rules_returns_pass(self, tmp_path) -> None:
|
||||
"""Settings with no stale rm rules returns PASS."""
|
||||
from aipass.aipass.apps.modules.doctor_wire import reconcile_stale_deny
|
||||
|
||||
settings = tmp_path / ".claude" / "settings.json"
|
||||
settings.parent.mkdir(parents=True)
|
||||
settings.write_text(
|
||||
json.dumps({"permissions": {"deny": ["Bash(git push --force*)", "Bash(git reset --hard*)"]}}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
with patch("aipass.aipass.apps.modules.doctor_wire.Path.home", return_value=tmp_path):
|
||||
results = reconcile_stale_deny(fix=False)
|
||||
assert len(results) == 1
|
||||
assert results[0][1] == GLYPH_PASS
|
||||
assert "no stale" in results[0][2]
|
||||
|
||||
def test_stale_rules_detected_without_fix(self, tmp_path) -> None:
|
||||
"""Stale rm rules present returns WARN when fix=False."""
|
||||
from aipass.aipass.apps.modules.doctor_wire import reconcile_stale_deny
|
||||
|
||||
settings = tmp_path / ".claude" / "settings.json"
|
||||
settings.parent.mkdir(parents=True)
|
||||
settings.write_text(
|
||||
json.dumps({"permissions": {"deny": ["Bash(rm -rf*)", "Bash(git push --force*)", "Bash(rm -r *)"]}}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
with patch("aipass.aipass.apps.modules.doctor_wire.Path.home", return_value=tmp_path):
|
||||
results = reconcile_stale_deny(fix=False)
|
||||
assert len(results) == 1
|
||||
assert results[0][1] == GLYPH_WARN
|
||||
assert "rm -rf" in results[0][2]
|
||||
assert "rm -r " in results[0][2]
|
||||
|
||||
def test_fix_removes_stale_rules(self, tmp_path) -> None:
|
||||
"""fix=True removes stale rules and preserves others."""
|
||||
from aipass.aipass.apps.modules.doctor_wire import reconcile_stale_deny
|
||||
|
||||
settings = tmp_path / ".claude" / "settings.json"
|
||||
settings.parent.mkdir(parents=True)
|
||||
original = {
|
||||
"permissions": {"deny": ["Bash(rm -rf*)", "Bash(git push --force*)", "Bash(rm -r *)"]},
|
||||
"env": {"AIPASS_HOME": "/test"},
|
||||
}
|
||||
settings.write_text(json.dumps(original), encoding="utf-8")
|
||||
with patch("aipass.aipass.apps.modules.doctor_wire.Path.home", return_value=tmp_path):
|
||||
results = reconcile_stale_deny(fix=True)
|
||||
assert len(results) == 1
|
||||
assert results[0][1] == GLYPH_PASS
|
||||
assert "removed" in results[0][2]
|
||||
updated = json.loads(settings.read_text(encoding="utf-8"))
|
||||
assert "Bash(rm -rf*)" not in updated["permissions"]["deny"]
|
||||
assert "Bash(rm -r *)" not in updated["permissions"]["deny"]
|
||||
assert "Bash(git push --force*)" in updated["permissions"]["deny"]
|
||||
assert updated["env"]["AIPASS_HOME"] == "/test"
|
||||
|
||||
def test_fix_single_stale_rule(self, tmp_path) -> None:
|
||||
"""fix=True works when only one of two stale rules is present."""
|
||||
from aipass.aipass.apps.modules.doctor_wire import reconcile_stale_deny
|
||||
|
||||
settings = tmp_path / ".claude" / "settings.json"
|
||||
settings.parent.mkdir(parents=True)
|
||||
settings.write_text(
|
||||
json.dumps({"permissions": {"deny": ["Bash(rm -rf*)", "Bash(git reset --hard*)"]}}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
with patch("aipass.aipass.apps.modules.doctor_wire.Path.home", return_value=tmp_path):
|
||||
results = reconcile_stale_deny(fix=True)
|
||||
assert len(results) == 1
|
||||
assert results[0][1] == GLYPH_PASS
|
||||
updated = json.loads(settings.read_text(encoding="utf-8"))
|
||||
assert updated["permissions"]["deny"] == ["Bash(git reset --hard*)"]
|
||||
|
||||
def test_fix_idempotent(self, tmp_path) -> None:
|
||||
"""Running fix twice is safe — second run returns PASS with no stale rules."""
|
||||
from aipass.aipass.apps.modules.doctor_wire import reconcile_stale_deny
|
||||
|
||||
settings = tmp_path / ".claude" / "settings.json"
|
||||
settings.parent.mkdir(parents=True)
|
||||
settings.write_text(
|
||||
json.dumps({"permissions": {"deny": ["Bash(rm -rf*)", "Bash(rm -r *)"]}}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
with patch("aipass.aipass.apps.modules.doctor_wire.Path.home", return_value=tmp_path):
|
||||
reconcile_stale_deny(fix=True)
|
||||
results = reconcile_stale_deny(fix=True)
|
||||
assert len(results) == 1
|
||||
assert results[0][1] == GLYPH_PASS
|
||||
assert "no stale" in results[0][2]
|
||||
|
||||
def test_empty_deny_list_returns_pass(self, tmp_path) -> None:
|
||||
"""Empty deny list returns PASS."""
|
||||
from aipass.aipass.apps.modules.doctor_wire import reconcile_stale_deny
|
||||
|
||||
settings = tmp_path / ".claude" / "settings.json"
|
||||
settings.parent.mkdir(parents=True)
|
||||
settings.write_text(json.dumps({"permissions": {"deny": []}}), encoding="utf-8")
|
||||
with patch("aipass.aipass.apps.modules.doctor_wire.Path.home", return_value=tmp_path):
|
||||
results = reconcile_stale_deny(fix=False)
|
||||
assert len(results) == 1
|
||||
assert results[0][1] == GLYPH_PASS
|
||||
|
||||
def test_no_permissions_key_returns_pass(self, tmp_path) -> None:
|
||||
"""Settings without permissions key returns PASS."""
|
||||
from aipass.aipass.apps.modules.doctor_wire import reconcile_stale_deny
|
||||
|
||||
settings = tmp_path / ".claude" / "settings.json"
|
||||
settings.parent.mkdir(parents=True)
|
||||
settings.write_text(json.dumps({"env": {"FOO": "bar"}}), encoding="utf-8")
|
||||
with patch("aipass.aipass.apps.modules.doctor_wire.Path.home", return_value=tmp_path):
|
||||
results = reconcile_stale_deny(fix=False)
|
||||
assert len(results) == 1
|
||||
assert results[0][1] == GLYPH_PASS
|
||||
|
||||
Reference in New Issue
Block a user