diff --git a/editors/vscode/samples/exhaustive.dippy b/editors/vscode/samples/exhaustive.dippy index a5cc7b2..5381e6b 100644 --- a/editors/vscode/samples/exhaustive.dippy +++ b/editors/vscode/samples/exhaustive.dippy @@ -155,6 +155,21 @@ deny-mcp mcp__*__delete_* "Block all MCP delete operations" after-mcp mcp__github__create_* "Check the GitHub UI to verify" after-mcp mcp__github__create_pull_request * "Share the PR URL with the team" +# ----------------------------------------------------------------------------- +# ENVIRONMENT VARIABLE ALLOWLIST: allow-env +# ----------------------------------------------------------------------------- + +# A leading "VAR=value cmd" prefix is part of the literal command. By default +# only known-inert variables (locale, color, project name, ...) let a command +# inherit a bare "allow cmd" rule; a prefix that can change what runs +# (LD_PRELOAD=, GIT_SSH_COMMAND=, ...) still prompts. allow-env extends the +# inert set with project-specific variables. +allow-env COMPOSE_FILE +allow-env APP_DEBUG + +# Glob patterns match by variable name +allow-env MYAPP_* + # ----------------------------------------------------------------------------- # AFTER RULES: PostToolUse feedback # ----------------------------------------------------------------------------- diff --git a/editors/vscode/snippets.json b/editors/vscode/snippets.json index 99f0d00..40afbe8 100644 --- a/editors/vscode/snippets.json +++ b/editors/vscode/snippets.json @@ -54,6 +54,11 @@ "body": "after-mcp ${1:mcp__github__create_*} \"${2:message}\"", "description": "Post-action feedback for MCP tools" }, + "allow-env": { + "prefix": ["allow-env", "allowe"], + "body": "allow-env ${1:COMPOSE_FILE}", + "description": "Treat an env var prefix as inert when matching allow rules" + }, "set log": { "prefix": ["set log", "log"], "body": "set log ${1:~/.dippy/audit.log}", diff --git a/editors/vscode/syntaxes/dippy.tmLanguage.json b/editors/vscode/syntaxes/dippy.tmLanguage.json index d13cfec..0c58dc4 100644 --- a/editors/vscode/syntaxes/dippy.tmLanguage.json +++ b/editors/vscode/syntaxes/dippy.tmLanguage.json @@ -11,7 +11,7 @@ "name": "comment.line.number-sign.dippy" }, "rule": { - "begin": "^\\s*(allow-redirect|ask-redirect|deny-redirect|allow-mcp|ask-mcp|deny-mcp|after-mcp|allow|ask|deny|after|alias)\\b", + "begin": "^\\s*(allow-redirect|ask-redirect|deny-redirect|allow-mcp|ask-mcp|deny-mcp|after-mcp|allow-env|allow|ask|deny|after|alias)\\b", "beginCaptures": { "1": { "name": "keyword.control.dippy" } }, diff --git a/src/dippy/core/allowlists.py b/src/dippy/core/allowlists.py index 7b85a46..ed33a4a 100644 --- a/src/dippy/core/allowlists.py +++ b/src/dippy/core/allowlists.py @@ -263,3 +263,34 @@ "builtin", # run shell builtin } ) + + +# === Inert Environment Variables === +# Leading assignments of these variables don't change which executable runs +# or what it connects to, so "allow cmd" may match "VAR=value cmd". Anything +# that affects executable resolution, library loading, code execution, or +# connection targets (PATH, LD_*, BASH_ENV, PYTHON*, GIT_SSH_COMMAND, +# DOCKER_HOST, PAGER, EDITOR, ...) must never be added here. +# LC_* (LC_ALL, LC_CTYPE, ...) are locale settings, treated as inert via a +# prefix check at the call site rather than enumerated individually. + +INERT_ENV_VARS = frozenset( + { + "APP_ENV", # framework environment name (Symfony) + "CI", # continuous-integration marker + "CLICOLOR", # BSD color toggle + "CLICOLOR_FORCE", # force color output + "COLUMNS", # terminal width hint + "COMPOSE_PROJECT_NAME", # docker compose project label + "FORCE_COLOR", # force color output + "LANG", # locale + "LANGUAGE", # locale fallback list + "LINES", # terminal height hint + "NODE_ENV", # node environment name + "NO_COLOR", # disable color output + "PYTHONUNBUFFERED", # unbuffered python stdio + "RAILS_ENV", # rails environment name + "TERM", # terminal type + "TZ", # timezone + } +) diff --git a/src/dippy/core/analyzer.py b/src/dippy/core/analyzer.py index 8db68db..bf0691f 100644 --- a/src/dippy/core/analyzer.py +++ b/src/dippy/core/analyzer.py @@ -7,12 +7,13 @@ from __future__ import annotations +import fnmatch from dataclasses import dataclass, field from pathlib import Path from typing import Literal from dippy.core.config import Config, match_redirect -from dippy.core.allowlists import SIMPLE_SAFE, WRAPPER_COMMANDS +from dippy.core.allowlists import INERT_ENV_VARS, SIMPLE_SAFE, WRAPPER_COMMANDS from dippy.cli import get_handler, get_description, HandlerContext from dippy.vendor.parable import parse, ParseError @@ -387,6 +388,37 @@ def _analyze_redirects( return decisions +def _config_match_decision(match, base: str) -> Decision | None: + """Convert a config Match to a Decision, or None if no match.""" + if not match: + return None + if match.decision == "allow": + return Decision("allow", f"{base} ({match.pattern})") + msg = match.message or match.pattern + return Decision(match.decision, f"{base}: {msg}") + + +def _env_assignments_inert(assignments: list[str], config: Config) -> bool: + """True if every leading VAR=value assignment names an inert variable. + + Inert means the assignment doesn't change which executable runs or what it + connects to, so the command may inherit a bare-command allow rule. A + variable qualifies if it is in INERT_ENV_VARS, is an LC_* locale setting, + or matches a user "allow-env" pattern. + """ + for assignment in assignments: + name = assignment.split("=", 1)[0] + if name in INERT_ENV_VARS or name.startswith("LC_"): + continue + # Env var names are case-sensitive; fnmatchcase avoids the platform + # case-folding that plain fnmatch (and so _glob_match) applies, which + # would make matching diverge between Linux and macOS. + if any(fnmatch.fnmatchcase(name, pattern) for pattern in config.env_vars): + continue + return False + return True + + def _analyze_simple_command( words: list[str], config: Config, @@ -418,15 +450,31 @@ def _analyze_simple_command( cmd = SimpleCommand(words=words) config_match = match_command(cmd, config, cwd, remote=remote) - if config_match: - if config_match.decision == "allow": - return Decision("allow", f"{base} ({config_match.pattern})") - elif config_match.decision == "deny": - msg = config_match.message or config_match.pattern - return Decision("deny", f"{base}: {msg}") - else: # ask - msg = config_match.message or config_match.pattern - return Decision("ask", f"{base}: {msg}") + decision = _config_match_decision(config_match, base) + if decision: + return decision + + # 1.5. If no config match on raw tokens and the command carries leading + # environment variable assignments (FOO=bar cmd ...), retry matching with + # them stripped. Step 1 preserves explicit rules that include the + # assignment (e.g. "deny FOO=* cmd"). The retry is asymmetric: + # - deny/ask matches always apply (stripping only widens what is blocked), + # - an allow match applies only when every assignment names an inert + # variable, so a non-inert prefix (LD_PRELOAD=, GIT_SSH_COMMAND=, ...) + # can't ride in under a plain config "allow cmd" rule. + # Note: this gate covers the config-rule path only. The built-in safe-command, + # version/help, and wrapper paths below already strip env assignments before + # matching (pre-existing behavior), so a non-inert prefix on e.g. a SIMPLE_SAFE + # command is still allowed; tightening those is tracked as a follow-up. + if i > 0: + env_stripped_match = match_command( + SimpleCommand(words=tokens), config, cwd, remote=remote + ) + decision = _config_match_decision(env_stripped_match, base) + if decision and ( + decision.action != "allow" or _env_assignments_inert(words[:i], config) + ): + return decision # 2. Handle wrapper commands (time, timeout, etc.) - analyze inner command if base in WRAPPER_COMMANDS and len(tokens) > 1: diff --git a/src/dippy/core/config.py b/src/dippy/core/config.py index 0a04006..e2de7e4 100644 --- a/src/dippy/core/config.py +++ b/src/dippy/core/config.py @@ -32,6 +32,36 @@ def _parse_module_name(rest: str) -> str: return mod +# Valid env var name or glob pattern: identifier characters plus the glob +# metacharacters * ? [ ] ! ^ -. Notably excludes '=', so "allow-env FOO=bar" +# (a value, not a name) is rejected -- matching is by variable name only. +_ENV_VAR_RE = re.compile(r"^[A-Za-z0-9_*?\[\]!^-]+$") + + +def _parse_env_var_pattern(rest: str) -> str: + """Parse and validate an env var name or glob pattern from a directive arg. + + Strips inline comments (# ...) and validates the pattern. Raises ValueError + if the name is missing, has extra words, or contains invalid characters. + """ + # Strip inline comments + if "#" in rest: + rest = rest[: rest.index("#")].rstrip() + if not rest: + raise ValueError("requires an env var name or pattern") + parts = rest.split() + if len(parts) != 1: + raise ValueError(f"requires exactly one env var name, got: {rest!r}") + name = parts[0] + if not _ENV_VAR_RE.match(name): + raise ValueError(f"invalid env var name or pattern: {name!r}") + if name in ("*", "**"): + # A catch-all would make every prefix inert -- including LD_PRELOAD and + # friends -- silently defeating the allowlist. Require a real name/prefix. + raise ValueError("'*' is too broad; name a variable or use a prefix glob") + return name + + # Cache home directory at module load - fails fast if HOME is unset _HOME = Path.home() @@ -86,6 +116,10 @@ class Config: aliases: dict[str, str] = field(default_factory=dict) """Command aliases mapping source to target (e.g., ~/bin/gh -> gh).""" + env_vars: list[str] = field(default_factory=list) + """Env var names/patterns whose leading assignments may match a bare-command + allow rule (e.g. "allow-env COMPOSE_FILE").""" + python_allow_modules: list[str] = field(default_factory=list) """Extra modules to treat as safe for Python static analysis.""" @@ -152,6 +186,8 @@ def _merge_configs(base: Config, overlay: Config) -> Config: after_mcp_rules=base.after_mcp_rules + overlay.after_mcp_rules, # Aliases: overlay wins for conflicting keys aliases={**base.aliases, **overlay.aliases}, + # Env var allowlist accumulates + env_vars=base.env_vars + overlay.env_vars, # Python module lists accumulate python_allow_modules=base.python_allow_modules + overlay.python_allow_modules, python_deny_modules=base.python_deny_modules + overlay.python_deny_modules, @@ -242,6 +278,7 @@ def parse_config(text: str, source: str | None = None) -> Config: mcp_rules: list[Rule] = [] after_mcp_rules: list[Rule] = [] aliases: dict[str, str] = {} + env_vars: list[str] = [] python_allow_modules: list[str] = [] python_deny_modules: list[str] = [] settings: dict[str, bool | int | str | Path] = {} @@ -356,6 +393,9 @@ def parse_config(text: str, source: str | None = None) -> Config: ) aliases[expanded_source] = alias_target + elif directive == "allow-env": + env_vars.append(_parse_env_var_pattern(rest)) + elif directive == "python-allow-module": mod = _parse_module_name(rest) python_allow_modules.append(mod) @@ -380,6 +420,7 @@ def parse_config(text: str, source: str | None = None) -> Config: mcp_rules=mcp_rules, after_mcp_rules=after_mcp_rules, aliases=aliases, + env_vars=env_vars, python_allow_modules=python_allow_modules, python_deny_modules=python_deny_modules, default=settings.get("default", "ask"), diff --git a/tests/test_analyzer_bugs.py b/tests/test_analyzer_bugs.py index fba0059..981b806 100644 --- a/tests/test_analyzer_bugs.py +++ b/tests/test_analyzer_bugs.py @@ -7,7 +7,119 @@ import pytest from dippy.core.analyzer import analyze -from dippy.core.config import Config +from dippy.core.config import Config, Rule + + +class TestConfigEnvVarPrefixMatching: + """Config rules should match commands with leading env var assignments. + + The config matcher checks raw tokens first, where "FOO=bar cmd" doesn't + match the pattern "cmd" because the assignment sits before the base + command. A second pass with env assignments stripped lets a bare-command + rule match "FOO=bar cmd" -- the same command the permission prompt + displays. The retry is asymmetric: deny/ask matches always apply (the + safe direction -- they only widen what is blocked), but an allow match + applies only when every leading assignment names an inert variable + (INERT_ENV_VARS, LC_*, or a user "allow-env" pattern). A non-inert prefix + like LD_PRELOAD= can change what actually runs, so it must not silently + inherit a plain "allow cmd". + """ + + @pytest.fixture + def cwd(self): + return Path.cwd() + + def test_allow_matches_env_prefixed_command(self, cwd): + """allow symfony should match COMPOSE_PROJECT_NAME=x symfony ...""" + config = Config(rules=[Rule("allow", "symfony")]) + result = analyze("COMPOSE_PROJECT_NAME=app symfony php foo.php", config, cwd) + assert result.action == "allow" + + def test_allow_matches_without_env_prefix_unchanged(self, cwd): + """Baseline: the same rule matches without an env prefix.""" + config = Config(rules=[Rule("allow", "symfony")]) + result = analyze("symfony php foo.php", config, cwd) + assert result.action == "allow" + + def test_allow_matches_multiple_inert_env_prefixes(self, cwd): + """Multiple inert leading assignments are stripped before re-matching.""" + config = Config(rules=[Rule("allow", "symfony")]) + result = analyze("CI=1 TZ=UTC symfony console cache:clear", config, cwd) + assert result.action == "allow" + + def test_explicit_env_rule_still_wins_on_raw_pass(self, cwd): + """A rule written against the raw tokens still matches in step 1.""" + config = Config(rules=[Rule("deny", "FOO=* symfony *", message="nope")]) + result = analyze("FOO=bar symfony php foo.php", config, cwd) + assert result.action == "deny" + + def test_no_spurious_match_when_no_rule(self, cwd): + """With no matching rule, an env-prefixed command still falls through.""" + config = Config(rules=[Rule("allow", "rails")]) + result = analyze("FOO=bar symfony php foo.php", config, cwd) + assert result.action == "ask" + + def test_stripped_deny_rule_surfaces_via_retry(self, cwd): + """A deny rule on the bare command applies to its env-prefixed form too. + + The deny/ask retry is unconditional -- a non-inert prefix does not + exempt a command from a deny rule. + """ + config = Config(rules=[Rule("deny", "symfony", message="blocked")]) + result = analyze("FOO=bar symfony php foo.php", config, cwd) + assert result.action == "deny" + + def test_allow_blocked_for_non_inert_var(self, cwd): + """An unknown env var must not inherit a bare allow rule.""" + config = Config(rules=[Rule("allow", "symfony")]) + result = analyze("FOO=bar symfony php foo.php", config, cwd) + assert result.action == "ask" + + def test_allow_blocked_for_dangerous_var(self, cwd): + """A non-inert prefix that can change execution does not inherit allow. + + Uses "symfony" (no built-in handler and not in SIMPLE_SAFE) so the + config gate is the only thing that could allow the command -- a + SIMPLE_SAFE command like "ls" would be allowed by a later step + regardless, masking the gate. + """ + config = Config(rules=[Rule("allow", "symfony")]) + result = analyze("LD_PRELOAD=/tmp/evil.so symfony php foo.php", config, cwd) + assert result.action == "ask" + + def test_allow_env_config_extends_allowlist(self, cwd): + """A user "allow-env" pattern lets that var inherit a bare allow rule.""" + config = Config(rules=[Rule("allow", "symfony")], env_vars=["FOO"]) + result = analyze("FOO=bar symfony php foo.php", config, cwd) + assert result.action == "allow" + + def test_allow_env_glob_pattern(self, cwd): + """An "allow-env" glob pattern matches by variable name.""" + config = Config(rules=[Rule("allow", "symfony")], env_vars=["MYAPP_*"]) + result = analyze("MYAPP_DEBUG=1 symfony php foo.php", config, cwd) + assert result.action == "allow" + + def test_allow_env_glob_is_case_sensitive(self, cwd): + """Env var names are case-sensitive on every platform. + + A lowercase variable must not match an uppercase "allow-env" glob -- + matching uses fnmatchcase so it does not case-fold on macOS/Windows. + """ + config = Config(rules=[Rule("allow", "symfony")], env_vars=["MYAPP_*"]) + result = analyze("myapp_debug=1 symfony php foo.php", config, cwd) + assert result.action == "ask" + + def test_mixed_inert_and_non_inert_blocks_allow(self, cwd): + """One non-inert assignment poisons the whole prefix for allow.""" + config = Config(rules=[Rule("allow", "symfony")]) + result = analyze("CI=1 FOO=bar symfony php foo.php", config, cwd) + assert result.action == "ask" + + def test_lc_prefix_is_inert(self, cwd): + """LC_* locale variables are treated as inert via prefix match.""" + config = Config(rules=[Rule("allow", "symfony")]) + result = analyze("LC_ALL=C symfony php foo.php", config, cwd) + assert result.action == "allow" class TestEnvVarPrefixHandling: diff --git a/tests/test_config.py b/tests/test_config.py index 47a8e9d..7270e73 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1579,6 +1579,61 @@ def test_after_mcp_rules_concatenate(self): assert len(merged.after_mcp_rules) == 2 +class TestParseConfigEnvVars: + """Test parsing of the allow-env directive.""" + + def test_allow_env_directive_parsed(self): + cfg = parse_config("allow-env COMPOSE_FILE") + assert cfg.env_vars == ["COMPOSE_FILE"] + + def test_allow_env_glob_pattern_parsed(self): + cfg = parse_config("allow-env MYAPP_*") + assert cfg.env_vars == ["MYAPP_*"] + + def test_allow_env_multiple_directives_accumulate(self): + cfg = parse_config("allow-env FOO\nallow-env BAR") + assert cfg.env_vars == ["FOO", "BAR"] + + def test_allow_env_no_name_skipped(self): + cfg = parse_config("allow-env") + assert cfg.env_vars == [] + + def test_allow_env_multiple_names_skipped(self): + cfg = parse_config("allow-env FOO BAR") + assert cfg.env_vars == [] + + def test_allow_env_value_rejected(self): + # "allow-env FOO=bar" is a value, not a name -- matching is by name only. + cfg = parse_config("allow-env FOO=bar") + assert cfg.env_vars == [] + + def test_allow_env_catch_all_rejected(self): + # A bare "*" would make every prefix inert, defeating the allowlist. + cfg = parse_config("allow-env *") + assert cfg.env_vars == [] + cfg = parse_config("allow-env **") + assert cfg.env_vars == [] + + def test_allow_env_mixed_with_other_rules(self): + cfg = parse_config(""" +allow symfony * +allow-env COMPOSE_FILE +deny rm -rf /* +""") + assert len(cfg.rules) == 2 + assert cfg.env_vars == ["COMPOSE_FILE"] + + +class TestMergeConfigsEnvVars: + """Test allow-env merging across scopes.""" + + def test_env_vars_concatenate(self): + base = Config(env_vars=["FOO"]) + overlay = Config(env_vars=["BAR"]) + merged = _merge_configs(base, overlay) + assert merged.env_vars == ["FOO", "BAR"] + + class TestTagRulesMcp: """Test origin tagging for MCP rules."""