From 36809944cae95a0c0e24c6882321431c95fef903 Mon Sep 17 00:00:00 2001 From: Petr Date: Thu, 7 May 2026 21:59:35 +0200 Subject: [PATCH 1/2] fix(0.30.6): sec-20 follow-up -- ValueError -> ConfigError for clean envelope v0.30.5's sec-20 fix added a descriptive error message for malformed .keboola/branch-mapping.json but raised it as a bare ValueError. CLI commands didn't catch ValueError, so an end user with a hand-edited mapping file saw a multi-frame Python traceback dumped to stderr instead of the standard JSON error envelope. Found during v0.30.5 e2e smoke test against the kbagent-e2e project. Not a security regression -- the descriptive content was correct -- but a clear UX cleanup. Fix: load_branch_mapping() now raises ConfigError directly (with the same descriptive "Failed to parse : Invalid branch ID ..." message). Existing 'except ConfigError' handlers in commands/sync.py catch it via the standard path and emit: { "status": "error", "error": { "code": "CONFIG_ERROR", "message": "Failed to parse /.../branch-mapping.json: ...", ... } } with exit code 5 and no stack trace. cleanup_branch_id_from_mapping() extended to catch ConfigError alongside ValueError so its best-effort skip behavior is preserved. BranchMapping.from_dict() continues to raise ValueError (it's the data-parser layer; ConfigError requires filesystem context which only load_branch_mapping has). Test: - test_load_branch_mapping_invalid_id_raises_config_error: asserts ConfigError with the same descriptive message - E2E re-run: clean JSON envelope confirmed; exit 5; zero traceback lines Total suite: 2834 passed. --- .claude-plugin/marketplace.json | 2 +- plugins/kbagent/.claude-plugin/plugin.json | 2 +- pyproject.toml | 2 +- src/keboola_agent_cli/changelog.py | 4 ++++ src/keboola_agent_cli/sync/branch_mapping.py | 21 ++++++++++++++------ tests/test_sync_branch_mapping.py | 10 +++++++--- uv.lock | 2 +- 7 files changed, 30 insertions(+), 13 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 9ebded7e..1b5a43a2 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -10,7 +10,7 @@ "plugins": [ { "name": "kbagent", - "version": "0.30.5", + "version": "0.30.6", "source": "./plugins/kbagent", "description": "AI-friendly interface to Keboola Connection projects — explore configs, jobs, lineage, call MCP tools, manage dev branches, and debug SQL in workspaces", "category": "development" diff --git a/plugins/kbagent/.claude-plugin/plugin.json b/plugins/kbagent/.claude-plugin/plugin.json index 2df7c46f..5d976416 100644 --- a/plugins/kbagent/.claude-plugin/plugin.json +++ b/plugins/kbagent/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "kbagent", - "version": "0.30.5", + "version": "0.30.6", "description": "AI-friendly interface to Keboola Connection projects — explore configs, jobs, lineage, call MCP tools, manage dev branches, and debug SQL in workspaces", "author": { "name": "Keboola", diff --git a/pyproject.toml b/pyproject.toml index 399d7a81..5aed53cb 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "keboola-agent-cli" -version = "0.30.5" +version = "0.30.6" description = "AI-friendly CLI for managing Keboola projects" readme = "README.md" requires-python = ">=3.12" diff --git a/src/keboola_agent_cli/changelog.py b/src/keboola_agent_cli/changelog.py index e07c114f..403e1055 100644 --- a/src/keboola_agent_cli/changelog.py +++ b/src/keboola_agent_cli/changelog.py @@ -8,6 +8,10 @@ # Ordered newest-first. Each value is a list of brief one-line descriptions. CHANGELOG: dict[str, list[str]] = { + "0.30.6": [ + "UX (sec-20 follow-up): malformed `.keboola/branch-mapping.json` now surfaces as a clean JSON error envelope (exit 5, `CONFIG_ERROR`) instead of a raw Python traceback. v0.30.5 introduced the descriptive `Invalid branch ID in branch-mapping.json` message but `load_branch_mapping()` raised it as a bare `ValueError` -- which CLI commands did not catch, so an end user with a hand-edited mapping file saw a multi-frame traceback dumped to stderr instead of a one-line error. Fixed by raising `ConfigError` from `load_branch_mapping()` directly; existing CLI `except ConfigError` handlers now produce the standard error envelope. Found during v0.30.5 e2e smoke test against the kbagent-e2e project; not a security regression but a clear UX cleanup. The descriptive content of the error is unchanged; only the wrapper class differs.", + "Tests: `test_load_branch_mapping_invalid_id_includes_path` updated to assert `ConfigError` instead of `ValueError`. `cleanup_branch_id_from_mapping()` extended to catch `ConfigError` alongside the legacy `ValueError` so its best-effort behavior is preserved. `BranchMapping.from_dict()` continues to raise `ValueError` (it's the data-parser layer); only `load_branch_mapping()` (the filesystem-aware wrapper) was promoted to `ConfigError`.", + ], "0.30.5": [ "Security (critical): `kbagent sync pull` no longer permits API-controlled `component_id` or `component_type` to escape the sync workspace via path traversal. `naming.config_path()` now passes both fields through a new `sanitize_path_segment()` that rejects `/`, `\\`, and parent-directory references (`..`) while preserving the dots, hyphens, and underscores in legitimate component IDs (`keboola.ex-db-mysql`, `kds-team.app-custom-python`). `services/sync_service.py:pull()` adds a defense-in-depth confinement check that raises ConfigError if a resolved config path is not contained in the branch directory. Issue #269 sec-01 / sec-07; threat actor: compromised stack or supply-chain attack on the project token. Pre-fix, `component_id = '../../../etc'` would write outside the project root.", "Security (high): MCP HTTP transport subprocess no longer inherits Keboola tokens from the kbagent process environment. `mcp_transport.py:_start()` previously used `subprocess.Popen(cmd, ...)` with no `env=` argument, so when `KBAGENT_MCP_TRANSPORT=http` was set, the MCP server inherited `KBC_MASTER_TOKEN`, `KBC_MASTER_TOKEN_`, `KBC_MANAGE_API_TOKEN`, and `KBC_TOKEN`. New `_build_minimal_env()` allow-lists only the env vars needed for binary discovery and locale handling (PATH, HOME, USER, LANG, LC_*, UV_CACHE_DIR, PYTHONPATH, ...) and explicitly drops every `KBC_*` token. Per-project Storage tokens still flow through HTTP request headers as before. Issue #269 sec-02 / sec-08; closes the gap left by v0.29.0's manage-token default-deny on the HTTP transport path.", diff --git a/src/keboola_agent_cli/sync/branch_mapping.py b/src/keboola_agent_cli/sync/branch_mapping.py index acbbb389..1b4a8f28 100644 --- a/src/keboola_agent_cli/sync/branch_mapping.py +++ b/src/keboola_agent_cli/sync/branch_mapping.py @@ -11,6 +11,7 @@ from typing import Any from ..constants import BRANCH_MAPPING_FILENAME, KEBOOLA_DIR_NAME +from ..errors import ConfigError def _coerce_keboola_id(raw: Any) -> int | None: @@ -94,9 +95,13 @@ def load_branch_mapping(project_root: Path) -> BranchMapping: Raises: FileNotFoundError: If the mapping file does not exist. - ValueError: If the JSON cannot be parsed or contains a malformed + ConfigError: If the JSON cannot be parsed or contains a malformed branch ID. The descriptive message names the offending file - so the user can find and fix it (issue #269 sec-20). + so the user can find and fix it. Surfacing this as ConfigError + (rather than raw ValueError) lets CLI commands catch it via + the existing ``except ConfigError`` handler and emit a clean + JSON error envelope with exit code 5 instead of a Python + traceback (issue #269 sec-20 + smoke-test follow-up). """ path = project_root / KEBOOLA_DIR_NAME / BRANCH_MAPPING_FILENAME if not path.exists(): @@ -105,9 +110,10 @@ def load_branch_mapping(project_root: Path) -> BranchMapping: data = json.loads(path.read_text(encoding="utf-8")) return BranchMapping.from_dict(data) except ValueError as exc: - # _coerce_keboola_id raises ValueError on malformed IDs; wrap with - # path context so the user knows which file to fix. - raise ValueError(f"Failed to parse {path}: {exc}") from exc + # _coerce_keboola_id and json.loads both raise ValueError on + # malformed input; wrap with path context and convert to + # ConfigError so the CLI surfaces a clean error envelope. + raise ConfigError(f"Failed to parse {path}: {exc}") from exc def save_branch_mapping(project_root: Path, mapping: BranchMapping) -> None: @@ -150,7 +156,10 @@ def cleanup_branch_id_from_mapping(branch_id: int) -> dict[str, Any] | None: return None try: mapping = load_branch_mapping(project_root) - except (FileNotFoundError, ValueError): + except (FileNotFoundError, ConfigError, ValueError): + # ValueError preserved for backward compat with callers that may + # still trigger it via custom paths; ConfigError is the new shape + # raised by load_branch_mapping itself. return None removed: list[str] = [] diff --git a/tests/test_sync_branch_mapping.py b/tests/test_sync_branch_mapping.py index 011ab19e..a062aa34 100644 --- a/tests/test_sync_branch_mapping.py +++ b/tests/test_sync_branch_mapping.py @@ -12,6 +12,7 @@ import pytest from keboola_agent_cli.constants import BRANCH_MAPPING_FILENAME, KEBOOLA_DIR_NAME +from keboola_agent_cli.errors import ConfigError from keboola_agent_cli.sync.branch_mapping import ( BranchMapping, BranchMappingEntry, @@ -175,8 +176,11 @@ def test_branch_mapping_from_dict_invalid_id_descriptive_error(self) -> None: with pytest.raises(ValueError, match="Invalid branch ID"): BranchMapping.from_dict(data) - def test_load_branch_mapping_invalid_id_includes_path(self, tmp_path: Path) -> None: - """``load_branch_mapping`` wraps the descriptive error with the file path.""" + def test_load_branch_mapping_invalid_id_raises_config_error(self, tmp_path: Path) -> None: + """``load_branch_mapping`` wraps the descriptive error with the file + path AND raises ConfigError so CLI commands surface a clean exit-5 + envelope instead of a Python traceback (issue #269 sec-20 follow-up). + """ keboola_dir = tmp_path / KEBOOLA_DIR_NAME keboola_dir.mkdir() (keboola_dir / BRANCH_MAPPING_FILENAME).write_text( @@ -189,7 +193,7 @@ def test_load_branch_mapping_invalid_id_includes_path(self, tmp_path: Path) -> N } ) ) - with pytest.raises(ValueError, match=r"Failed to parse .*branch-mapping\.json"): + with pytest.raises(ConfigError, match=r"Failed to parse .*branch-mapping\.json"): load_branch_mapping(tmp_path) diff --git a/uv.lock b/uv.lock index 2ccb8ed7..f1101073 100644 --- a/uv.lock +++ b/uv.lock @@ -439,7 +439,7 @@ wheels = [ [[package]] name = "keboola-agent-cli" -version = "0.30.5" +version = "0.30.6" source = { editable = "." } dependencies = [ { name = "httpx" }, From ca94598fd96b6596e81fcecaac879cb8297ca03b Mon Sep 17 00:00:00 2001 From: Petr Date: Thu, 7 May 2026 22:09:01 +0200 Subject: [PATCH 2/2] review: address blocking finding from /kbagent:review of #273 Reviewer found that sync_branch_status (commands/sync.py:992) catches only FileNotFoundError, not ConfigError. After PR #273's load_branch_mapping ValueError -> ConfigError conversion, a corrupted .keboola/branch-mapping.json still produced a Python traceback when the user ran 'kbagent sync branch-status' (the diagnostic command they would naturally reach for to debug a corrupted workspace). All other sync commands (sync_pull, sync_push, sync_diff, sync_branch_link, sync_branch_unlink) already had the 'except ConfigError' handler -- only sync_branch_status was missing. Added it now with the same exit-5 + CONFIG_ERROR envelope shape. Test: new TestSyncBranchStatusCli :: test_sync_branch_status_corrupted_mapping_clean_envelope mocks the service raising ConfigError and asserts exit 5 + clean JSON envelope + no 'Traceback' string in output. E2E re-verified against /tmp/kbagent-e2e: corrupted mapping -> clean error envelope with descriptive message, exit 5, zero traceback lines. --- src/keboola_agent_cli/commands/sync.py | 8 +++++ tests/test_sync_cli.py | 41 ++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/src/keboola_agent_cli/commands/sync.py b/src/keboola_agent_cli/commands/sync.py index 3433e5a3..fb92a64c 100644 --- a/src/keboola_agent_cli/commands/sync.py +++ b/src/keboola_agent_cli/commands/sync.py @@ -994,6 +994,14 @@ def sync_branch_status( except FileNotFoundError as exc: formatter.error(message=str(exc), error_code=ErrorCode.NOT_INITIALIZED) raise typer.Exit(code=1) from None + except ConfigError as exc: + # Corrupted .keboola/branch-mapping.json -- surface as clean + # exit-5 envelope so the user sees the descriptive message + # ("Failed to parse ...: Invalid branch ID ...") instead of a + # Python traceback. Mirrors the handler other sync commands + # already have (issue #269 sec-20 follow-up). + formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) + raise typer.Exit(code=5) from None if formatter.json_mode: formatter.output(result) diff --git a/tests/test_sync_cli.py b/tests/test_sync_cli.py index 1190ca2f..3ade3529 100644 --- a/tests/test_sync_cli.py +++ b/tests/test_sync_cli.py @@ -1563,6 +1563,47 @@ def test_sync_branch_status_disabled_human(self, tmp_path: Path) -> None: assert result.exit_code == 0, f"Exit code {result.exit_code}: {result.output}" assert "not enabled" in result.output + def test_sync_branch_status_corrupted_mapping_clean_envelope(self, tmp_path: Path) -> None: + """A corrupted .keboola/branch-mapping.json must produce a clean + JSON error envelope (exit 5, CONFIG_ERROR), not a Python traceback + (issue #269 sec-20 follow-up + #273 reviewer feedback).""" + from keboola_agent_cli.errors import ConfigError + + config_dir = tmp_path / "config" + config_dir.mkdir() + + store = _setup_config(config_dir, {"prod": {"token": TEST_TOKEN}}) + + mock_sync = _make_sync_service_mock() + # Service raises ConfigError as load_branch_mapping now does for + # malformed mappings. CLI must catch it. + mock_sync.branch_status.side_effect = ConfigError( + "Failed to parse /tmp/.keboola/branch-mapping.json: " + "Invalid branch ID in branch-mapping.json: 'not-a-number'. " + "Expected null or an integer; got str." + ) + + with ( + patch("keboola_agent_cli.cli.ConfigStore") as MockStore, + patch("keboola_agent_cli.cli.ProjectService") as MockProjService, + patch("keboola_agent_cli.cli.SyncService") as MockSyncService, + ): + MockStore.return_value = store + MockProjService.return_value = ProjectService(config_store=store) + MockSyncService.return_value = mock_sync + + result = runner.invoke( + app, + ["--json", "sync", "branch-status", "--directory", str(tmp_path)], + ) + + assert result.exit_code == 5, f"expected exit 5, got {result.exit_code}: {result.output}" + assert "Traceback" not in result.output + data = json.loads(result.output) + assert data["status"] == "error" + assert data["error"]["code"] == "CONFIG_ERROR" + assert "Invalid branch ID" in data["error"]["message"] + class TestSyncInitAdoptExistingCli: """Tests for `kbagent sync init --adopt-existing`."""