From 2db06e0c2f3418bb7c79ae091f0098f8b70f81cc Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 25 Aug 2026 13:40:14 +0200 Subject: [PATCH 1/4] fix(permissions): validate set --allow/--deny patterns against known operations (#688) kbagent permissions set previously accepted any pattern string with zero validation, so a typo like tool.admin or stroage.upload-table silently persisted a dead rule. permissions set now rejects any --allow/--deny pattern that is not a cli:* category, an exact OPERATION_REGISTRY / FLAG_ESCALATIONS key, or a glob matching at least one known operation -- VALIDATION_ERROR, exit 2, every offending pattern listed, checked BEFORE the interactive confirmation so a bad call never reaches the prompt. find_inert_patterns() is generalized the same way: it now flags any pattern matching zero known operations, not only the retired tool: namespace, so permissions show and kbagent doctor surface any dead rule in an already-persisted policy (e.g. one written before this gate existed). PermissionEngine stays lenient at evaluation time -- only permissions set is strict. CLI_CATEGORY_PATTERNS is extracted so _matches_pattern and the new pattern_matches_known_operation() cannot drift apart, and a new UNMATCHED_PATTERN_HINT covers the generic (non-tool:) case alongside the existing tool:/MCP-migration hint. --- src/keboola_agent_cli/commands/permissions.py | 55 ++++++- src/keboola_agent_cli/permissions.py | 50 +++++- .../services/doctor_service.py | 59 +++++-- tests/test_doctor_service.py | 33 ++++ tests/test_permissions.py | 83 +++++++++- tests/test_permissions_cli.py | 144 ++++++++++++++++++ 6 files changed, 394 insertions(+), 30 deletions(-) diff --git a/src/keboola_agent_cli/commands/permissions.py b/src/keboola_agent_cli/commands/permissions.py index cd19eba3..d3a4669c 100644 --- a/src/keboola_agent_cli/commands/permissions.py +++ b/src/keboola_agent_cli/commands/permissions.py @@ -19,9 +19,12 @@ from ..models import PermissionPolicy from ..permissions import ( INERT_PATTERN_HINT, + INERT_PATTERN_PREFIX, INERT_SINCE_VERSION, + UNMATCHED_PATTERN_HINT, PermissionEngine, find_inert_patterns, + pattern_matches_known_operation, ) from ._helpers import get_formatter, get_service, require_random_code_confirmation @@ -202,10 +205,31 @@ def permissions_show( if persisted.deny: formatter.console.print(f"[bold]Deny:[/bold] {', '.join(persisted.deny)}") if inert_patterns: + # Generalized (issue #688): ANY pattern matching no known operation + # is reported here, not only the retired `tool:` namespace. The + # `tool:`-specific MCP-migration hint is appended only when at + # least one offending pattern starts with that prefix; the + # generic typo hint is appended whenever any other kind is + # present too -- both can show up together in a mixed policy. + has_tool_prefix = any(p.startswith(INERT_PATTERN_PREFIX) for p in inert_patterns) + has_other = any(not p.startswith(INERT_PATTERN_PREFIX) for p in inert_patterns) + since_clause = ( + f" since v{INERT_SINCE_VERSION} (the 'tool:' namespace was removed " + "with the MCP passthrough)" + if has_tool_prefix + else "" + ) + hints = [ + h + for h, present in ( + (INERT_PATTERN_HINT, has_tool_prefix), + (UNMATCHED_PATTERN_HINT, has_other), + ) + if present + ] formatter.console.print( - f"[yellow]{len(inert_patterns)} inert pattern(s) since v{INERT_SINCE_VERSION} " - "(the 'tool:' namespace was removed with the MCP passthrough): " - f"{', '.join(inert_patterns)}. {INERT_PATTERN_HINT}[/yellow]" + f"[yellow]{len(inert_patterns)} inert pattern(s){since_clause} match no " + f"known operation: {', '.join(inert_patterns)}. {' '.join(hints)}[/yellow]" ) else: formatter.console.print("[dim]No persisted permission policy (config.json is clean).[/dim]") @@ -244,6 +268,13 @@ def permissions_set( Requires interactive confirmation (type a random code) to prevent AI agents from modifying permissions programmatically. + Every ``--allow`` / ``--deny`` pattern must be a ``cli:*`` category, an + exact operation name, or a glob matching at least one known operation + (issue #688) -- rejected up front (VALIDATION_ERROR, exit 2) BEFORE the + interactive confirmation, so a typo'd pattern never gets silently + persisted as a dead rule and no confirmation prompt is wasted on a call + that was going to fail anyway. + Examples: # Block all write operations (Vojta's use case): kbagent permissions set --mode allow --deny "cli:write" @@ -263,6 +294,24 @@ def permissions_set( ) raise typer.Exit(code=2) from None + invalid: list[str] = [] + for pattern in [*(allow or []), *(deny or [])]: + if not pattern_matches_known_operation(pattern) and pattern not in invalid: + invalid.append(pattern) + if invalid: + formatter.error( + message=( + "Unknown operation pattern(s): " + + ", ".join(invalid) + + ". Patterns must be a cli:* category (cli:read, cli:write, " + "cli:destructive, cli:admin), an exact operation name, or a glob " + "matching at least one operation. See `kbagent permissions list`." + ), + error_code=ErrorCode.VALIDATION_ERROR, + details={"invalid_patterns": invalid}, + ) + raise typer.Exit(code=2) from None + require_random_code_confirmation("update permission policy") config_store: ConfigStore = get_service(ctx, "config_store") diff --git a/src/keboola_agent_cli/permissions.py b/src/keboola_agent_cli/permissions.py index c46fa16d..79cb9b64 100644 --- a/src/keboola_agent_cli/permissions.py +++ b/src/keboola_agent_cli/permissions.py @@ -412,6 +412,21 @@ # show` and `kbagent doctor` cannot drift apart. INERT_PATTERN_HINT = "Rewrite the intent with cli:* categories -- see docs/mcp-migration.md." +# Generic hint for a pattern that matches nothing for a reason other than the +# retired `tool:` namespace (most commonly a typo). Kept distinct from +# INERT_PATTERN_HINT so `permissions show` / `doctor` can point at the right +# fix instead of always mentioning the MCP migration. +UNMATCHED_PATTERN_HINT = ( + "Check for typos against `kbagent permissions list`, or use cli:* categories." +) + +# The four risk-category patterns `_matches_pattern` special-cases. Exported +# so `pattern_matches_known_operation` cannot drift from the engine's own +# notion of "valid category pattern". +CLI_CATEGORY_PATTERNS: frozenset[str] = frozenset( + {"cli:read", "cli:write", "cli:destructive", "cli:admin"} +) + def apply_firewall_flags( persisted: PermissionPolicy | None, @@ -469,13 +484,36 @@ def apply_firewall_flags( ) +def pattern_matches_known_operation(pattern: str) -> bool: + """True if the pattern is a cli:* category or matches >=1 known operation. + + Known operations are ``OPERATION_REGISTRY`` keys plus ``FLAG_ESCALATIONS`` + keys (flag-escalated strings like ``auth.logout --remove-projects`` are + real operation strings passed to ``PermissionEngine.is_allowed()``, see + ``commands/auth.py``). A glob pattern counts as valid when it matches at + least one of those operation strings via ``fnmatch.filter``. + + Used both to reject unknown patterns at write time (``permissions set``) + and to flag ones already on disk (``find_inert_patterns``) -- kept as a + single source of truth so the two checks cannot drift apart. + """ + if pattern in CLI_CATEGORY_PATTERNS: + return True + return bool(fnmatch.filter([*OPERATION_REGISTRY, *FLAG_ESCALATIONS], pattern)) + + def find_inert_patterns(policy: PermissionPolicy | None) -> list[str]: """Patterns in a persisted policy that can no longer match any operation. - The MCP passthrough was removed in 0.85.0 and with it the ``tool:`` - operation namespace; patterns targeting it fall through to fnmatch and - match nothing. Surfaced by ``permissions show`` and ``kbagent doctor`` - so a pre-0.85 policy does not silently carry dead rules. + Generalized (issue #688): flags ANY pattern that matches zero known + operations, not only the retired ``tool:`` namespace. That namespace -- + gone since the MCP passthrough was removed in 0.85.0 -- is simply the + most common historical cause; a typo'd operation name or a stale glob is + just as inert and just as worth surfacing. Surfaced by ``permissions + show`` and ``kbagent doctor`` so a policy does not silently carry dead + rules. ``permissions set`` (issue #688) additionally REJECTS such + patterns at write time -- this function stays for what is already + persisted (pre-fix policies, or the engine's own leniency). Returns the offending patterns in policy order (allow first, then deny), de-duplicated -- the same dead pattern listed twice is one problem. @@ -485,7 +523,7 @@ def find_inert_patterns(policy: PermissionPolicy | None) -> list[str]: inert: list[str] = [] for pattern in [*policy.allow, *policy.deny]: - if pattern.startswith(INERT_PATTERN_PREFIX) and pattern not in inert: + if not pattern_matches_known_operation(pattern) and pattern not in inert: inert.append(pattern) return inert @@ -506,7 +544,7 @@ def _matches_pattern(operation: str, pattern: str) -> bool: match nothing, and stay inert instead of raising. """ # Category patterns: cli:read, cli:write, cli:destructive, cli:admin - if pattern in ("cli:read", "cli:write", "cli:destructive", "cli:admin"): + if pattern in CLI_CATEGORY_PATTERNS: target_category = pattern.split(":")[1] # Fail-closed: unknown CLI ops default to 'write' so they are # blocked by cli:write policies. This prevents new commands from diff --git a/src/keboola_agent_cli/services/doctor_service.py b/src/keboola_agent_cli/services/doctor_service.py index 1016618d..05b2290c 100644 --- a/src/keboola_agent_cli/services/doctor_service.py +++ b/src/keboola_agent_cli/services/doctor_service.py @@ -23,7 +23,13 @@ from ..constants import ENV_CONVERSATION_ID from ..errors import KeboolaApiError from ..models import AppConfig -from ..permissions import INERT_PATTERN_HINT, INERT_SINCE_VERSION, find_inert_patterns +from ..permissions import ( + INERT_PATTERN_HINT, + INERT_PATTERN_PREFIX, + INERT_SINCE_VERSION, + UNMATCHED_PATTERN_HINT, + find_inert_patterns, +) from .base import ClientFactory, make_client_factory # Cap on how many offending items a single check names inline; the rest are @@ -279,13 +285,17 @@ def _check_mcp_tool_tasks(self) -> dict[str, Any]: def _check_inert_permission_patterns(config: AppConfig | None) -> dict[str, Any]: """Check 9: flag persisted permission patterns that can never match. - The ``tool:`` operation namespace went away with the MCP passthrough in - v0.85.0. A policy written before that still LOADS, but every ``tool:*`` - pattern in it now matches nothing: harmless in a ``mode="allow"`` deny - list, but in ``mode="deny"`` an allowance like ``tool:read`` silently - stops allowing anything. WARN rather than FAIL -- the policy is still - enforced, it is just narrower than its author intended. Read-only: - config.json only, no API call. + Generalized (issue #688): any pattern matching zero known operations is + flagged, not only the retired ``tool:`` namespace -- ``permissions set`` + now rejects such patterns at write time, but a policy persisted before + that gate existed can still carry dead rules of either kind. WARN rather + than FAIL -- the policy is still enforced, it is just narrower than its + author intended. Read-only: config.json only, no API call. + + ``details["inert_since"]`` is included ONLY when at least one offending + pattern starts with ``tool:`` -- that key names the version the MCP + passthrough (and with it the ``tool:`` namespace) was removed, which is + meaningless context for an unrelated typo like ``stroage.upload-table``. """ policy = config.permissions if config is not None else None if policy is None: @@ -305,20 +315,37 @@ def _check_inert_permission_patterns(config: AppConfig | None) -> dict[str, Any] "message": "No inert patterns in the persisted permission policy.", } + has_tool_prefix = any(p.startswith(INERT_PATTERN_PREFIX) for p in inert) + has_other = any(not p.startswith(INERT_PATTERN_PREFIX) for p in inert) + + since_clause = ( + f" have been inert since v{INERT_SINCE_VERSION} (the 'tool:' namespace was " + "removed with the MCP passthrough) and" + if has_tool_prefix + else "" + ) + hints = [ + hint + for hint, present in ( + (INERT_PATTERN_HINT, has_tool_prefix), + (UNMATCHED_PATTERN_HINT, has_other), + ) + if present + ] + + details: dict[str, Any] = {"mode": policy.mode, "patterns": inert} + if has_tool_prefix: + details["inert_since"] = INERT_SINCE_VERSION + return { "check": "inert_permission_patterns", "name": "Inert permission patterns", "status": "warn", "message": ( - f"{len(inert)} pattern(s) in the persisted permission policy have been inert " - f"since v{INERT_SINCE_VERSION} (the 'tool:' namespace was removed with the " - f"MCP passthrough) and match nothing: {', '.join(inert)}. {INERT_PATTERN_HINT}" + f"{len(inert)} pattern(s) in the persisted permission policy{since_clause} " + f"match no known operation: {', '.join(inert)}. {' '.join(hints)}" ), - "details": { - "inert_since": INERT_SINCE_VERSION, - "mode": policy.mode, - "patterns": inert, - }, + "details": details, } def _check_config_source(self) -> dict[str, Any]: diff --git a/tests/test_doctor_service.py b/tests/test_doctor_service.py index 4902d479..f27a14cb 100644 --- a/tests/test_doctor_service.py +++ b/tests/test_doctor_service.py @@ -872,6 +872,39 @@ def test_warn_lists_the_inert_patterns(self) -> None: assert result["details"]["inert_since"] == "0.85.0" assert result["details"]["mode"] == "deny" + def test_warn_lists_generic_dead_patterns_without_tool_prefix(self) -> None: + """Generalized detection (issue #688): a typo'd pattern is flagged too, + with no MCP-migration hint or `inert_since` (that context is meaningless + for a pattern that never had anything to do with the retired `tool:` + namespace).""" + config = AppConfig( + permissions=PermissionPolicy(mode="allow", deny=["stroage.upload-table", "cli:reed"]) + ) + + result = DoctorService._check_inert_permission_patterns(config) + + assert result["status"] == "warn" + assert "stroage.upload-table" in result["message"] + assert "cli:reed" in result["message"] + assert "match no known operation" in result["message"] + assert "docs/mcp-migration.md" not in result["message"] + assert result["details"]["patterns"] == ["stroage.upload-table", "cli:reed"] + assert "inert_since" not in result["details"] + assert result["details"]["mode"] == "allow" + + def test_warn_mentions_both_hints_for_a_mixed_policy(self) -> None: + """A policy carrying both a `tool:` pattern and a plain typo gets both hints.""" + config = AppConfig( + permissions=PermissionPolicy(mode="allow", deny=["tool:write", "cli:reed"]) + ) + + result = DoctorService._check_inert_permission_patterns(config) + + assert result["status"] == "warn" + assert "docs/mcp-migration.md" in result["message"] + assert "kbagent permissions list" in result["message"] + assert result["details"]["inert_since"] == "0.85.0" + def test_check_is_registered_in_run_checks(self, tmp_config_dir: Path) -> None: store = ConfigStore(config_dir=tmp_config_dir) config = store.load() diff --git a/tests/test_permissions.py b/tests/test_permissions.py index 5bdeb771..a87c6205 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -7,10 +7,12 @@ from keboola_agent_cli.errors import PermissionDeniedError from keboola_agent_cli.models import PermissionPolicy from keboola_agent_cli.permissions import ( + CLI_CATEGORY_PATTERNS, FLAG_ESCALATIONS, OPERATION_REGISTRY, PermissionEngine, find_inert_patterns, + pattern_matches_known_operation, ) @@ -109,6 +111,19 @@ def test_persisted_tool_patterns_are_inert(self) -> None: assert engine.is_allowed("job.run") is True assert engine.is_allowed("config.list") is True + def test_persisted_typo_patterns_are_inert(self) -> None: + """Runtime evaluation stays lenient for ANY unmatched pattern, not just tool:*. + + `permissions set` gates NEW writes against `pattern_matches_known_operation`, + but a policy already on disk from before that gate existed must still + load and evaluate without raising -- a dead pattern is a no-op, not a + crash. Only `permissions set` is strict; `PermissionEngine` stays lenient. + """ + policy = PermissionPolicy(mode="allow", deny=["stroage.upload-table", "cli:reed"]) + engine = PermissionEngine(policy) + assert engine.is_allowed("storage.upload-table") is True + assert engine.is_allowed("config.list") is True + def test_deny_with_allow_override(self) -> None: """Allow list can override deny for specific operations.""" policy = PermissionPolicy( @@ -417,24 +432,82 @@ def test_listed_so_a_caller_can_see_the_higher_class(self) -> None: assert entry["status"] == "denied" +class TestPatternMatchesKnownOperation: + """Tests for pattern_matches_known_operation() -- the shared validity check + used by both `permissions set` (to reject unknown patterns at write time) + and `find_inert_patterns` (to flag dead patterns already on disk). + """ + + def test_cli_category_patterns_are_valid(self) -> None: + for pattern in CLI_CATEGORY_PATTERNS: + assert pattern_matches_known_operation(pattern) is True + + def test_exact_registry_key_is_valid(self) -> None: + assert pattern_matches_known_operation("config.list") is True + assert pattern_matches_known_operation("branch.delete") is True + + def test_glob_matching_at_least_one_operation_is_valid(self) -> None: + # "storage.delete-*" matches storage.delete-table/-column/-bucket. + assert pattern_matches_known_operation("storage.delete-*") is True + # "sync.*" matches sync.push, sync.pull, ... + assert pattern_matches_known_operation("sync.*") is True + + def test_flag_escalation_key_is_valid(self) -> None: + # Flag-escalated strings are real operation strings passed to + # is_allowed() (see commands/auth.py), so an exact match is valid. + assert pattern_matches_known_operation("auth.logout --remove-projects") is True + + def test_unmatched_patterns_are_invalid(self) -> None: + assert pattern_matches_known_operation("tool.admin") is False + assert pattern_matches_known_operation("tool:read") is False + assert pattern_matches_known_operation("stroage.upload-table") is False + assert pattern_matches_known_operation("cli:reed") is False + assert pattern_matches_known_operation("") is False + assert pattern_matches_known_operation(" ") is False + + class TestFindInertPatterns: - """Tests for find_inert_patterns() -- dead rules in a pre-0.85 policy.""" + """Tests for find_inert_patterns() -- dead rules in a persisted policy. + + Generalized (issue #688): any pattern matching zero known operations is + flagged, not only the retired `tool:` namespace -- that namespace is now + just the most common historical cause. + """ def test_none_policy_returns_empty(self) -> None: assert find_inert_patterns(None) == [] - def test_returns_only_tool_namespace_patterns(self) -> None: + def test_flags_any_pattern_matching_no_operation(self) -> None: + """A typo'd operation is flagged exactly like a `tool:` pattern. + + Order is preserved (allow first, then deny) and each dead pattern + that is well-formed (`cli:read`, `config.list`, `storage.*`) is left + out entirely. + """ policy = PermissionPolicy( mode="deny", - allow=["tool:read", "cli:read", "config.list"], - deny=["tool:write", "branch.delete", "storage.*"], + allow=["tool:read", "cli:read", "config.list", "stroage.upload-table"], + deny=["tool:write", "branch.delete", "storage.*", "cli:reed"], ) - assert find_inert_patterns(policy) == ["tool:read", "tool:write"] + assert find_inert_patterns(policy) == [ + "tool:read", + "stroage.upload-table", + "tool:write", + "cli:reed", + ] def test_clean_policy_returns_empty(self) -> None: policy = PermissionPolicy(mode="allow", deny=["cli:write", "branch.delete"]) assert find_inert_patterns(policy) == [] + def test_valid_globs_and_categories_are_not_flagged(self) -> None: + policy = PermissionPolicy( + mode="allow", + allow=["cli:admin", "sync.*"], + deny=["cli:write", "storage.delete-*"], + ) + assert find_inert_patterns(policy) == [] + def test_duplicates_are_reported_once(self) -> None: policy = PermissionPolicy(mode="deny", allow=["tool:read"], deny=["tool:read"]) assert find_inert_patterns(policy) == ["tool:read"] diff --git a/tests/test_permissions_cli.py b/tests/test_permissions_cli.py index 5033d57b..b0ae7b7d 100644 --- a/tests/test_permissions_cli.py +++ b/tests/test_permissions_cli.py @@ -229,6 +229,28 @@ def test_show_human_mode_warns_about_inert_patterns(self, tmp_path: Path) -> Non assert "tool:read" in result.output assert "docs/mcp-migration.md" in result.output + def test_show_warns_for_a_typo_pattern_not_just_tool_namespace(self, tmp_path: Path) -> None: + """Generalized detection (issue #688): a plain typo is flagged too. + + The policy is written directly via the ConfigStore fixture, simulating + a pattern persisted before `permissions set` started validating writes. + """ + policy = PermissionPolicy(mode="allow", deny=["stroage.upload-table"]) + store = _make_store(tmp_path, policy) + with patch("keboola_agent_cli.cli.ConfigStore") as MockStore: + MockStore.return_value = store + result = runner.invoke(app, ["permissions", "show"]) + json_result = runner.invoke(app, ["--json", "permissions", "show"]) + assert result.exit_code == 0 + assert "stroage.upload-table" in result.output + assert "match no known operation" in result.output + # No tool:/MCP-migration hint for a plain typo -- the generic hint instead. + assert "docs/mcp-migration.md" not in result.output + assert "kbagent permissions list" in result.output + + data = json.loads(json_result.output)["data"] + assert data["inert_patterns"] == ["stroage.upload-table"] + class TestPermissionsSet: """Tests for `kbagent permissions set`.""" @@ -315,6 +337,128 @@ def test_set_invalid_mode(self, tmp_path: Path) -> None: result = runner.invoke(app, ["--json", "permissions", "set", "--mode", "invalid"]) assert result.exit_code == 2 + def test_set_rejects_unknown_allow_pattern(self, tmp_path: Path) -> None: + """A pattern matching no known operation is rejected up front (issue #688). + + Exit 2 + VALIDATION_ERROR, the offending pattern named in the message, + and the interactive confirmation must never fire -- the rejection is + pre-confirmation so a bad call is never even offered the prompt. + """ + store = _make_store(tmp_path) + with ( + patch("keboola_agent_cli.cli.ConfigStore") as MockStore, + patch( + "keboola_agent_cli.commands.permissions.require_random_code_confirmation", + ) as mock_confirm, + ): + MockStore.return_value = store + result = runner.invoke( + app, + ["--json", "permissions", "set", "--mode", "deny", "--allow", "tool.admin"], + ) + assert result.exit_code == 2 + data = json.loads(result.output) + assert data["status"] == "error" + assert data["error"]["code"] == "VALIDATION_ERROR" + assert "tool.admin" in data["error"]["message"] + mock_confirm.assert_not_called() + # Nothing persisted. + config = store.load() + assert config.permissions is None + + def test_set_rejects_unknown_deny_pattern(self, tmp_path: Path) -> None: + """The same rejection applies to --deny patterns, not just --allow.""" + store = _make_store(tmp_path) + with ( + patch("keboola_agent_cli.cli.ConfigStore") as MockStore, + patch( + "keboola_agent_cli.commands.permissions.require_random_code_confirmation", + ) as mock_confirm, + ): + MockStore.return_value = store + result = runner.invoke( + app, + [ + "--json", + "permissions", + "set", + "--mode", + "allow", + "--deny", + "stroage.upload-table", + ], + ) + assert result.exit_code == 2 + data = json.loads(result.output) + assert "stroage.upload-table" in data["error"]["message"] + mock_confirm.assert_not_called() + + def test_set_lists_every_bad_pattern_across_allow_and_deny(self, tmp_path: Path) -> None: + """Multiple bad patterns are all listed in one message -- one round trip to fix.""" + store = _make_store(tmp_path) + with ( + patch("keboola_agent_cli.cli.ConfigStore") as MockStore, + patch( + "keboola_agent_cli.commands.permissions.require_random_code_confirmation", + ) as mock_confirm, + ): + MockStore.return_value = store + result = runner.invoke( + app, + [ + "--json", + "permissions", + "set", + "--mode", + "allow", + "--allow", + "tool.admin", + "--deny", + "cli:reed", + "--deny", + "tool.admin", + ], + ) + assert result.exit_code == 2 + data = json.loads(result.output) + message = data["error"]["message"] + assert "tool.admin" in message + assert "cli:reed" in message + # The duplicate ("tool.admin" in both --allow and --deny) is listed once. + assert data["error"]["details"]["invalid_patterns"] == ["tool.admin", "cli:reed"] + mock_confirm.assert_not_called() + + def test_set_accepts_valid_categories_and_globs(self, tmp_path: Path) -> None: + """A call using only valid cli:* categories / exact ops / globs still persists.""" + store = _make_store(tmp_path) + with ( + patch("keboola_agent_cli.cli.ConfigStore") as MockStore, + patch( + "keboola_agent_cli.commands.permissions.require_random_code_confirmation", + return_value=None, + ), + ): + MockStore.return_value = store + result = runner.invoke( + app, + [ + "--json", + "permissions", + "set", + "--mode", + "deny", + "--allow", + "cli:read", + "--deny", + "storage.delete-*", + ], + ) + assert result.exit_code == 0 + config = store.load() + assert config.permissions is not None + assert config.permissions.allow == ["cli:read"] + assert config.permissions.deny == ["storage.delete-*"] + class TestPermissionsReset: """Tests for `kbagent permissions reset`.""" From 4734a40e9e31d64dc20b2b5cffa3f3c1898b4615 Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 25 Aug 2026 13:45:42 +0200 Subject: [PATCH 2/4] docs(permissions): document set pattern validation across drift surfaces (#688) permissions set now validates every --allow/--deny pattern before the interactive confirmation (commit 8ee3f13), and find_inert_patterns() generalized detection to any dead pattern, not only tool:*. Sync the silent-drift doc surfaces (convention #17) that describe permissions set: - CLAUDE.md `## All CLI Commands` permissions block - plugins/kbagent/skills/kbagent/references/gotchas.md: new (since vNEXT) entry - plugins/kbagent/skills/kbagent/references/commands-reference.md: extend the permissions set / show bullets - src/keboola_agent_cli/commands/context.py (AGENT_CONTEXT): note the validation and generalized dead-pattern detection keboola-expert.md was checked but left untouched -- its permissions mentions (403 handling, "permission handling built into kbagent") do not describe permissions set's pattern acceptance, so nothing there is falsified by this change. Feature PR: tagged (since vNEXT), no version bump, no changelog entry. --- CLAUDE.md | 8 +++++ .../kbagent/references/commands-reference.md | 4 +-- .../skills/kbagent/references/gotchas.md | 33 +++++++++++++++++++ src/keboola_agent_cli/commands/context.py | 6 ++++ 4 files changed, 49 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 99258945..913a4182 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -687,6 +687,14 @@ kbagent token refresh --project NAME --token-id ID [--yes] # like cli:read, cli:write, cli:destructive). `tool:*` patterns are INERT since 0.85.0 (the MCP # passthrough is gone): they load but match nothing, so a mode=deny policy whose only allowance was # tool:read now denies everything. The agent guards rails against mistakes; not a sandbox. +# `permissions set` (since vNEXT, issue #688) validates every --allow/--deny pattern BEFORE the +# interactive confirmation: each must be a cli:* category, an exact operation name (incl. a +# flag-escalated string like "auth.logout --remove-projects"), or a glob matching >=1 known +# operation -- an unknown pattern (typo, fabricated category) is rejected with VALIDATION_ERROR, +# exit 2, listing every offending pattern (--json: error.details.invalid_patterns), and nothing +# is persisted. `permissions show` / `kbagent doctor` also generalized: they now flag ANY +# persisted pattern matching zero known operations, not only the retired tool:* namespace. +# PermissionEngine itself stays lenient at evaluation time -- only `permissions set` is strict. kbagent permissions list [--category read|write|destructive|admin] kbagent permissions show kbagent permissions set --mode allow|deny [--allow PATTERN ...] [--deny PATTERN ...] diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index e8bc8b79..87802fac 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -93,8 +93,8 @@ All seven commands authenticate via `KBC_MANAGE_API_TOKEN` (Manage API), not the ## Permissions (session firewall commands) The `permissions` subcommands persist a write/destructive policy to config.json (the `--deny-*` flags above are the one-shot form). The engine guards against agent mistakes; it is not a sandbox. - `permissions list [--category read|write|destructive|admin]` -- list all operations with their risk category and current allowed/denied status -- `permissions show` -- show the current active permission policy -- `permissions set --mode allow|deny [--allow PATTERN ...] [--deny PATTERN ...]` -- set the permission policy (firewall rules); patterns like `cli:read`, `cli:write`, `cli:destructive`. `tool:*` patterns are INERT since v0.85.0 (the MCP passthrough is gone) -- they load but match nothing, so a `--mode deny` policy whose only allowance was `tool:read` now denies everything +- `permissions show` -- show the current active permission policy. *(since vNEXT)* also flags ANY persisted pattern matching zero known operations, not only the retired `tool:*` namespace (same generalization applies to `kbagent doctor`) -- a typo like `cli:reed` or `stroage.upload-table` is surfaced too, with a generic "check for typos" hint instead of the MCP-migration one +- `permissions set --mode allow|deny [--allow PATTERN ...] [--deny PATTERN ...]` -- set the permission policy (firewall rules); patterns like `cli:read`, `cli:write`, `cli:destructive`. `tool:*` patterns are INERT since v0.85.0 (the MCP passthrough is gone) -- they load but match nothing, so a `--mode deny` policy whose only allowance was `tool:read` now denies everything. *(since vNEXT, issue #688)* Every pattern is validated BEFORE the interactive confirmation: it must be a `cli:*` category, an exact operation name, or a glob matching at least one known operation, or the call fails fast with `VALIDATION_ERROR` (exit 2, `error.details.invalid_patterns` in `--json`) and nothing is written - `permissions reset` -- remove all permission restrictions - `permissions check OPERATION` -- check if a specific operation is allowed (e.g. `permissions check storage.delete-table`) diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 75236891..a771e1d6 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -4601,3 +4601,36 @@ directly to a workspace via the `KBC__` shared database -- `workspace query` runs against it with zero load and zero extra storage. Reach for `workspace load` only when the workflow actually needs the data materialized inside the workspace. + +## `permissions set` now rejects unknown patterns instead of silently persisting them (since vNEXT) + +Before this fix (issue #688), `kbagent permissions set --allow/--deny PATTERN` +accepted any string with zero validation. A typo like `tool.admin` or +`stroage.upload-table` (missing the `-` in `storage`) was written straight to +`config.json` as a dead rule that would never match anything -- the failure +was silent, and the only way to notice was reading `permissions show` (or +`kbagent doctor`) after the fact. + +- **Now validated up front, before the interactive confirmation.** Every + `--allow`/`--deny` pattern must be one of: a `cli:*` category + (`cli:read`/`cli:write`/`cli:destructive`/`cli:admin`), an exact operation + name (an `OPERATION_REGISTRY` key, or a flag-escalated string like + `"auth.logout --remove-projects"`), or a glob that matches at least one + known operation. Anything else fails fast with `VALIDATION_ERROR`, exit 2 -- + `--json` lists every offending pattern (deduplicated) in + `error.details.invalid_patterns`. Nothing is persisted, and the random-code + confirmation prompt is never shown for a call that was going to fail + anyway. +- **`permissions show` / `kbagent doctor` detection is generalized the same + way.** Both used to flag only the retired `tool:*` namespace (see the MCP + removal gotcha above); they now flag ANY persisted pattern matching zero + known operations, so a policy written before this gate existed (or one + edited directly in `config.json`) still surfaces its typos. The `tool:` + patterns keep their specific "MCP passthrough removed, see + docs/mcp-migration.md" hint; every other dead pattern gets a generic + "check for typos against `kbagent permissions list`" hint instead. A policy + mixing both kinds shows both hints. +- **`PermissionEngine` itself stays lenient at evaluation time** -- a dead + pattern already on disk is a silent no-op when the policy is enforced, not + a crash. Only `permissions set` is strict; this preserves backward + compatibility for a pre-existing policy that happens to carry a typo. diff --git a/src/keboola_agent_cli/commands/context.py b/src/keboola_agent_cli/commands/context.py index 22dd8f18..7998f7f6 100644 --- a/src/keboola_agent_cli/commands/context.py +++ b/src/keboola_agent_cli/commands/context.py @@ -1941,6 +1941,12 @@ they matched is gone. A persisted policy still loads with them, but they match nothing, so a mode=deny policy whose only allowance was `tool:read` now denies everything. Rewrite such a policy with `cli:read`. + Since vNEXT (issue #688): every pattern is validated BEFORE the + interactive confirmation -- it must be a cli:* category, an exact + operation name, or a glob matching >=1 known operation, or the whole + call fails with VALIDATION_ERROR (exit 2, invalid patterns listed) and + nothing is written. `permissions show` / `kbagent doctor` likewise flag + ANY dead pattern already on disk, not only `tool:*`. kbagent permissions reset Remove all restrictions. From f8932af8c99cc1c7534a7252391c17f7776469e0 Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 25 Aug 2026 13:56:45 +0200 Subject: [PATCH 3/4] docs+test(permissions): close final-review findings for #688 validation Generalize the two missed drift surfaces (permissions-workflow.md, docs/guide.md) that still described tool:* as the only inert pattern -- `permissions set` now validates every --allow/--deny pattern against all four categories (cli:* category, exact operation, or glob), and find_inert_patterns() flags any dead pattern, not only tool:*. Enumerate the four cli:* categories in UNMATCHED_PATTERN_HINT so the message is actionable instead of vague. Add a validator/engine parity test (TestValidatorEngineParity) pinning pattern_matches_known_operation against _matches_pattern for a sample of accepted and rejected patterns, so a future change reintroducing the #688 drift between the two functions fails in tests instead of shipping. Note in the gotchas.md #688 entry that a pattern naming an operation that only exists on a newer kbagent version is rejected on an older build too -- it would be inert there -- so persist it after upgrading. --- docs/guide.md | 2 +- .../skills/kbagent/references/gotchas.md | 5 ++ .../references/permissions-workflow.md | 12 ++++- src/keboola_agent_cli/permissions.py | 3 +- tests/test_permissions.py | 53 +++++++++++++++++++ 5 files changed, 72 insertions(+), 3 deletions(-) diff --git a/docs/guide.md b/docs/guide.md index 0c740bf4..585d091c 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -68,7 +68,7 @@ The agent can browse configs, list jobs and trace lineage -- but cannot create b | `cli:read` | All read-only CLI commands | | `branch.delete` | Exact command | | `sync.*` | All sync subcommands | -| `tool:*` | Nothing -- inert since 0.85.0 (the MCP passthrough is gone); a `--mode deny` policy that only allowed `tool:read` now denies everything | +| `tool:*` | Nothing -- inert since 0.85.0 (the MCP passthrough is gone); a `--mode deny` policy that only allowed `tool:read` now denies everything; `permissions set` REJECTS it as input (since vNEXT) -- only an already-persisted `tool:*` sticks around | ### Management commands diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index a771e1d6..20b680cd 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -4634,3 +4634,8 @@ was silent, and the only way to notice was reading `permissions show` (or pattern already on disk is a silent no-op when the policy is enforced, not a crash. Only `permissions set` is strict; this preserves backward compatibility for a pre-existing policy that happens to carry a typo. +- **Version skew is a real rejection case, not just a typo.** A pattern + naming an operation that only exists on a newer kbagent version (a command + added after the running build was installed) is rejected the same as a + typo -- it would be inert on this build anyway -- so upgrade first, then + persist the pattern. diff --git a/plugins/kbagent/skills/kbagent/references/permissions-workflow.md b/plugins/kbagent/skills/kbagent/references/permissions-workflow.md index 2ca30f64..85acf0a2 100644 --- a/plugins/kbagent/skills/kbagent/references/permissions-workflow.md +++ b/plugins/kbagent/skills/kbagent/references/permissions-workflow.md @@ -27,6 +27,16 @@ The agent CANNOT: > denies everything. Rewrite such a policy with `cli:read`. `kbagent permissions > show` names any such pattern (key `inert_patterns` in `--json`) and `kbagent > doctor` WARNs via its `inert_permission_patterns` check. +> +> **(since vNEXT)** `permissions set` now validates every `--allow`/`--deny` +> pattern before persisting it: it must be a `cli:*` category, an exact +> operation name, or a glob matching >=1 known operation, else the call fails +> with `VALIDATION_ERROR`, exit 2, before the confirmation prompt is even +> shown. `permissions show` / `doctor` are generalized the same way -- they +> now flag ANY persisted pattern matching zero operations (a typo included), +> not only `tool:*`. `tool:*` remains the one historical case with its own +> "MCP passthrough removed" hint; every other dead pattern gets a generic +> "check for typos" hint instead. ## Common restriction recipes @@ -96,7 +106,7 @@ kbagent --json permissions list | `cli:read` | All read-only CLI commands | | `branch.delete` | Exact command match | | `sync.*` | All sync subcommands (glob) | -| `tool:*` | Nothing -- inert since v0.85.0 (the MCP passthrough is gone) | +| `tool:*` | Nothing -- inert since v0.85.0 (the MCP passthrough is gone); `permissions set` REJECTS it as input (since vNEXT) -- only an already-persisted `tool:*` sticks around | ## Session firewall flags diff --git a/src/keboola_agent_cli/permissions.py b/src/keboola_agent_cli/permissions.py index 79cb9b64..b9c60629 100644 --- a/src/keboola_agent_cli/permissions.py +++ b/src/keboola_agent_cli/permissions.py @@ -417,7 +417,8 @@ # INERT_PATTERN_HINT so `permissions show` / `doctor` can point at the right # fix instead of always mentioning the MCP migration. UNMATCHED_PATTERN_HINT = ( - "Check for typos against `kbagent permissions list`, or use cli:* categories." + "Check for typos against `kbagent permissions list`, or use a cli:* category " + "(cli:read, cli:write, cli:destructive, cli:admin)." ) # The four risk-category patterns `_matches_pattern` special-cases. Exported diff --git a/tests/test_permissions.py b/tests/test_permissions.py index a87c6205..10bd7cf6 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -11,6 +11,7 @@ FLAG_ESCALATIONS, OPERATION_REGISTRY, PermissionEngine, + _matches_pattern, find_inert_patterns, pattern_matches_known_operation, ) @@ -511,3 +512,55 @@ def test_valid_globs_and_categories_are_not_flagged(self) -> None: def test_duplicates_are_reported_once(self) -> None: policy = PermissionPolicy(mode="deny", allow=["tool:read"], deny=["tool:read"]) assert find_inert_patterns(policy) == ["tool:read"] + + +class TestValidatorEngineParity: + """`pattern_matches_known_operation` (the `permissions set` validator) must + never drift from `_matches_pattern` (the `PermissionEngine` matcher) -- + that drift is exactly what issue #688 was: a pattern the validator judged + one way while the engine judged another, so a rejected-looking pattern + still silently matched nothing at runtime (or vice versa). This test pins + both sides against the same sample patterns so a future change to either + function that reopens the gap fails here, not in production. + """ + + ACCEPTED_GLOB_OR_EXACT: ClassVar[list[str]] = [ + "sync.*", + "storage.delete-*", + "config.list", # exact OPERATION_REGISTRY key + "auth.logout --remove-projects", # exact FLAG_ESCALATIONS key + "*--remove-projects", # glob matching a FLAG_ESCALATIONS key + ] + + REJECTED_PATTERNS: ClassVar[list[str]] = [ + "cli:*", # looks like a category but is not one of the four literals + "tool:*", # retired MCP-passthrough namespace + "tool.admin", + "stroage.upload-table", # typo (missing the '-' in storage) + "", + ] + + def test_accepted_glob_or_exact_patterns_match_at_least_one_known_operation(self) -> None: + """For non-category patterns, validator acceptance implies the engine + actually matches >=1 known operation string with the same pattern.""" + known_ops = {**OPERATION_REGISTRY, **FLAG_ESCALATIONS} + for pattern in self.ACCEPTED_GLOB_OR_EXACT: + assert pattern_matches_known_operation(pattern) is True, pattern + assert any(_matches_pattern(op, pattern) for op in known_ops), pattern + + def test_accepted_cli_categories_are_valid_via_the_category_branch(self) -> None: + """cli:* categories are valid by construction (CLI_CATEGORY_PATTERNS), + not by matching any operation string -- they are evaluated by + `_matches_pattern`'s dedicated category branch instead.""" + for pattern in CLI_CATEGORY_PATTERNS: + assert pattern_matches_known_operation(pattern) is True, pattern + + def test_rejected_patterns_are_invalid_and_never_match_any_operation(self) -> None: + """Every pattern `permissions set` refuses must also be one the engine + would never match -- otherwise the validator is rejecting something + that would actually work, or (for cli:*/tool:*) the rejection would + not mirror the engine's own "this never matches" behavior.""" + known_ops = {**OPERATION_REGISTRY, **FLAG_ESCALATIONS} + for pattern in self.REJECTED_PATTERNS: + assert pattern_matches_known_operation(pattern) is False, pattern + assert not any(_matches_pattern(op, pattern) for op in known_ops), pattern From 7317088a177b97d7e01584f81965d7d78d1cd979 Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 25 Aug 2026 14:08:16 +0200 Subject: [PATCH 4/4] =?UTF-8?q?docs(permissions):=20address=20review=20NB?= =?UTF-8?q?=20findings=20=E2=80=94=20doctor=20JSON=20shape=20callout=20+?= =?UTF-8?q?=20expert=20gotcha=20(#693)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Document that doctor --json's inert_permission_patterns.details.inert_since is only present when a tool:-prefixed pattern is among the offenders, and add a keboola-expert.md §3 gotcha for permissions set pattern validation. --- plugins/kbagent/agents/keboola-expert.md | 6 ++++++ plugins/kbagent/skills/kbagent/references/gotchas.md | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/plugins/kbagent/agents/keboola-expert.md b/plugins/kbagent/agents/keboola-expert.md index cf096aa0..40c02248 100644 --- a/plugins/kbagent/agents/keboola-expert.md +++ b/plugins/kbagent/agents/keboola-expert.md @@ -372,6 +372,12 @@ its absence is NOT a promise the entry is version-independent (see §1 Rule 6). (`fallback_used: "heuristic"`), not the full AI wizard (that is the `sl-build` skill). +**`permissions set --allow/--deny` validates patterns (since vNEXT)** +- A typo'd or fabricated pattern (`tool.admin`, `stroage.*`) fails fast with + `VALIDATION_ERROR`, exit 2, instead of persisting silently -- valid inputs + are `cli:*` categories, exact operation names, or globs matching >=1 + operation (check with `kbagent permissions list`). + --- ## 4. WORKFLOWS (reference playbooks) diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 20b680cd..891ca792 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -4639,3 +4639,8 @@ was silent, and the only way to notice was reading `permissions show` (or added after the running build was installed) is rejected the same as a typo -- it would be inert on this build anyway -- so upgrade first, then persist the pattern. +- **`doctor --json`'s `inert_permission_patterns` check carries + `details.inert_since` only when at least one offending pattern is + `tool:`-prefixed** (since vNEXT) -- a purely typo'd policy (e.g. + `stroage.upload-table`) omits the key entirely, so JSON consumers must not + assume `inert_since` is always present on `status: "warn"`.