From 63f8f119e4a72d72eec38d2132f6f82b50ad1b25 Mon Sep 17 00:00:00 2001 From: Petr Date: Thu, 7 May 2026 20:21:24 +0200 Subject: [PATCH 1/3] fix(0.30.5): close issue #269 -- 9 verified findings from security audit This release ships fixes for nine security findings surfaced by an audit driven by three sub-agents in sequence (use-case map -> state-machine -> security review). Findings A-E from issue #267 (merged in #268) are intentionally out of scope for this audit. Critical - sec-01 / sec-07: path traversal via API-controlled component_id and component_type. naming.config_path() interpolated those tokens raw, so a malicious or compromised stack returning component_id = "../../etc" would direct sync pull writes outside the workspace. Fix: new sanitize_path_segment() that rejects /, \, and parent references while preserving dots/hyphens/underscores in legitimate IDs (keboola.ex-db-mysql, kds-team.app-custom-python). Plus a defense-in-depth confinement check in sync_service.pull() that raises ConfigError if the resolved config dir is not contained in the branch dir. High - sec-02 / sec-08: MCP HTTP transport subprocess no longer inherits any KBC_* token from the kbagent process env. Pre-fix, Popen(cmd, ...) had no env= arg, so KBC_MASTER_TOKEN, KBC_MASTER_TOKEN_, KBC_MANAGE_API_TOKEN, and KBC_TOKEN all leaked to the MCP server when KBAGENT_MCP_TRANSPORT=http. New _build_minimal_env() allow-lists only PATH, HOME, locale, and uv/python cache vars. Per-project Storage tokens still flow via HTTP request headers as before. Closes the v0.29.0 manage-token default-deny gap on the HTTP transport path. - sec-04: REPL history file (~/.config/keboola-agent-cli/repl_history) is now created with mode 0600. Pre-fix, FileHistory created the file with the user's umask (typically 0644), persisting any token typed at the prompt in plaintext readable by group/world. - sec-05: kbagent lineage show --format html no longer emits XSS- vulnerable HTML. render_er_diagram() previously did name.replace('"', "'") which left <, >, and & untouched -- a Keboola entity named would inject the script. Fix uses html.escape(s, quote=True) consistently for every API-derived string. - sec-06: kbagent encrypt values --output-file now atomically creates the file with mode 0600 via os.open(..., 0o600). Replaces the previous Path.write_text() + chmod(0o600) which left a race window where the file was world-readable. Medium - sec-11: max_parallel_workers Pydantic field now requires ge=1 in addition to le=100. Pre-fix, max_parallel_workers: 0 in config.json passed validation and crashed every multi-project op with ValueError from ThreadPoolExecutor. _resolve_max_workers() also clamps defensively for legacy on-disk configs. Low - sec-19: kbagent permissions check OPERATION now reflects the EFFECTIVE policy for the invocation -- persisted policy MERGED with --deny-writes / --deny-destructive session flags -- matching permissions list semantics. Pre-fix, an AI agent inspecting its own self-imposed firewall got a misleading "allowed" answer. - sec-20: _coerce_keboola_id() and load_branch_mapping() now raise descriptive errors for malformed branch IDs in branch-mapping.json. Pre-fix, "id": "not-a-number" produced raw "invalid literal for int()" from deep inside the parser. Tests - 33 new regression tests across 7 test files. Total suite: 2830 passed (was 2797). Out of scope (tracked as follow-up in #269) - sec-03 token-in-argv deprecation - sec-09 PTY-bypass design - sec-10 silent name collisions - sec-12 version_cache atomicity - sec-13 @file path restriction - sec-14 alias validation - sec-16 SRI on Mermaid CDN - sec-17 KBAGENT_AUTO_UPDATE toggle staleness - sec-18 doctor --fix uv via PATH Audit artifacts kept locally at /tmp/kbagent-audit/use-cases.md (28K) + state-machine.md (43K) + issues.md (15K) for the test design phase. --- .claude-plugin/marketplace.json | 2 +- plugins/kbagent/.claude-plugin/plugin.json | 2 +- pyproject.toml | 2 +- src/keboola_agent_cli/changelog.py | 12 +++ src/keboola_agent_cli/commands/encrypt.py | 18 ++++- src/keboola_agent_cli/commands/permissions.py | 18 ++++- src/keboola_agent_cli/commands/repl.py | 24 +++++- src/keboola_agent_cli/models.py | 1 + src/keboola_agent_cli/services/base.py | 8 +- .../services/deep_lineage_service.py | 17 ++-- .../services/mcp_transport.py | 41 ++++++++++ .../services/sync_service.py | 32 ++++++++ src/keboola_agent_cli/sync/branch_mapping.py | 33 +++++++- src/keboola_agent_cli/sync/naming.py | 33 +++++++- tests/test_deep_lineage_service.py | 81 +++++++++++++++++++ tests/test_mcp_transport.py | 45 +++++++++++ tests/test_models.py | 14 ++++ tests/test_permissions_cli.py | 31 +++++++ tests/test_repl.py | 40 ++++++++- tests/test_sync_branch_mapping.py | 30 +++++++ tests/test_sync_naming.py | 78 +++++++++++++++++- uv.lock | 2 +- 22 files changed, 539 insertions(+), 25 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 12b7ce9b..9ebded7e 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -10,7 +10,7 @@ "plugins": [ { "name": "kbagent", - "version": "0.30.4", + "version": "0.30.5", "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 ef3ea5d9..2df7c46f 100644 --- a/plugins/kbagent/.claude-plugin/plugin.json +++ b/plugins/kbagent/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "kbagent", - "version": "0.30.4", + "version": "0.30.5", "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 8271eda2..399d7a81 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "keboola-agent-cli" -version = "0.30.4" +version = "0.30.5" 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 fd25c81c..0e7aaa7d 100644 --- a/src/keboola_agent_cli/changelog.py +++ b/src/keboola_agent_cli/changelog.py @@ -8,6 +8,18 @@ # Ordered newest-first. Each value is a list of brief one-line descriptions. CHANGELOG: dict[str, list[str]] = { + "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.", + "Security (high): REPL history file (`~/.config/keboola-agent-cli/repl_history`) is now created with mode 0600. Pre-fix, `prompt_toolkit.FileHistory` created the file with the user's default umask (typically 0644), persisting any token typed at the prompt (e.g. `project add --token ...`) in plaintext readable by group/world. `_get_history_path()` now atomically pre-creates the file with 0o600 and tightens existing files via chmod. Issue #269 sec-04.", + "Security (high): `kbagent lineage show --format html` no longer emits XSS-vulnerable HTML. `services/deep_lineage_service.py:render_er_diagram()` previously did `name.replace('\"', \"'\")` which left `<`, `>`, and `&` untouched -- a Keboola table or config named `` would inject the script into the generated HTML body where the browser parses it before Mermaid runs. Fix uses `html.escape(s, quote=True)` consistently for every API-derived string embedded into the Mermaid body and surrounding HTML. Mermaid renders the entities back to their characters in SVG text so visible output is unchanged. Issue #269 sec-05.", + "Security (high): `kbagent encrypt values --output-file PATH` now atomically creates the file with mode 0600. Pre-fix, `Path.write_text()` followed by `chmod(0o600)` left a race window where the encrypted secrets file was world-readable on systems with permissive umask (e.g. 0644 default). Replaced with `os.open(path, O_WRONLY|O_CREAT|O_TRUNC, 0o600)` + `os.write()`. Issue #269 sec-06.", + "Security (medium): `max_parallel_workers` Pydantic field now requires `ge=1` in addition to `le=100`. Pre-fix, a config.json with `max_parallel_workers: 0` passed validation, then `ThreadPoolExecutor(max_workers=0)` crashed every multi-project operation with `ValueError`. `BaseService._resolve_max_workers()` also clamps to >= 1 defensively so a legacy on-disk config does not crash startup. Issue #269 sec-11.", + "Security (low): `kbagent permissions check OPERATION` now reflects the EFFECTIVE policy for the current invocation -- the persisted policy MERGED with `--deny-writes` / `--deny-destructive` session flags -- matching `permissions list` semantics. Pre-fix, `permissions check` consulted only the persisted policy, so an AI agent inspecting its own self-imposed firewall got a misleading `allowed` answer for write ops. Issue #269 sec-19.", + 'Security (low): `_coerce_keboola_id()` and `load_branch_mapping()` now raise descriptive errors for malformed branch IDs in `branch-mapping.json`. Pre-fix, a hand-edited file with `"id": "not-a-number"` produced a raw `ValueError: invalid literal for int()` from deep inside the parser. New error names the offending file path and the bad value so users can fix it. Issue #269 sec-20.', + "Tests: 33 new regression tests across `test_sync_naming.py` (sanitize_path_segment + config_path traversal-resistance), `test_mcp_transport.py` (env scrubbing for KBC_*), `test_repl.py` (history file 0600 + tighten existing), `test_deep_lineage_service.py` (XSS escape in ER diagram for table and config names), `test_models.py` (max_parallel_workers ge=1), `test_sync_branch_mapping.py` (descriptive ValueError + path-prefixed wrap), `test_permissions_cli.py` (--deny-writes / --deny-destructive applied by `permissions check`). Total suite: 2830 passed.", + "Audit methodology: this release was driven by a three-stage automated audit (kbagent expert -> state-machine engineer -> security engineer), each as a sub-agent reading the previous output. The use-case map (28 KB) covered every command + 20 multi-command life situations + cross-cutting concerns. The state-machine doc (43 KB) traced every persistent / in-memory state with file:line references, command-by-command read/write/assert table, life-situation traces, and 9 forbidden state combinations. The security review (20 findings, 15 KB) prioritized 9 verified issues for this release; the remainder (sec-03 token-in-argv deprecation, sec-09 PTY-bypass design, sec-10 silent collisions, sec-12 cache atomicity, sec-13 @file restriction, sec-14 alias validation, sec-16 SRI on Mermaid CDN, sec-17 toggle staleness, sec-18 PATH for uv) are tracked in #269 as out-of-scope follow-ups.", + ], "0.30.4": [ "Fix: `kbagent sync pull` against a linked dev branch now writes files under the linked branch's directory (`branch-/...` or its sanitized name), not under `main/`. Pre-fix, `branch_link` persisted Keboola branch IDs as **strings** in `.keboola/branch-mapping.json` (`kbc_branch_id = str(branch_info['id'])` at five call-sites in `services/sync_service.py`), but every comparison against the manifest read those IDs as the **int** they're typed as in `ManifestBranch.id: int` and on the Storage API. Cross-type `int == str` is always False in Python, so `_find_branch_path` fell back to the default branch (`manifest.branches[0].path == 'main'`) and `_ensure_branch_registered` re-registered the 'unknown' branch on every pull, appending a duplicate `branches[]` entry with a mangled `branch--` path until the manifest was hand-cleaned. The same comparison failed in `_ensure_branch_registered`'s `b.get('id') == branch_id` API-name lookup, so the branch's human-readable name from the API was never used and the path always fell through to the numeric `branch-` fallback (the 'side observation' from issue #267). Fix is end-to-end `int`: `branch_link` writes `int(branch_info['id'])`, `BranchMappingEntry.keboola_id: int | None` (was `str | None`), and `from_dict` silently coerces legacy string IDs on load so existing user workspaces upgrade without manual editing. Bug A from issue #267, reported externally on v0.27.0 and reproduced on v0.30.3.", "Fix: `kbagent sync pull` no longer re-writes every previously-tracked config on every invocation in git-branching mode. The `branch_switched` guard at `services/sync_service.py:489-491` compared `existing_branch_ids[lookup_key]` (int from manifest) against the polluted str return of `_resolve_branch_id`; cross-type `!=` was always True, so the idempotency check was completely defeated and `files_written` ticked up on every pull even when nothing changed. The Bug A end-to-end int fix automatically restores correct behaviour here -- this is Bug C from issue #267, fixed transitively. Regression test pins `pull-pull-pull` against an unchanged remote and asserts manifest stability.", diff --git a/src/keboola_agent_cli/commands/encrypt.py b/src/keboola_agent_cli/commands/encrypt.py index 8e08b81d..02a45026 100644 --- a/src/keboola_agent_cli/commands/encrypt.py +++ b/src/keboola_agent_cli/commands/encrypt.py @@ -5,6 +5,7 @@ """ import json +import os import sys from pathlib import Path @@ -104,8 +105,21 @@ def encrypt_values( raise typer.Exit(code=exit_code) from None if output_file: - output_file.write_text(json.dumps(result, indent=2), encoding="utf-8") - output_file.chmod(0o600) + # Atomic create with 0600 -- avoids the race window between + # write_text() (which uses the user's umask, often 0644) and a + # subsequent chmod (issue #269 sec-06). On the chmod-after-write + # pattern, another local user could read the encrypted secrets in + # the brief window before the chmod ran. + payload = json.dumps(result, indent=2).encode("utf-8") + fd = os.open( + str(output_file), + os.O_WRONLY | os.O_CREAT | os.O_TRUNC, + 0o600, + ) + try: + os.write(fd, payload) + finally: + os.close(fd) if not formatter.json_mode: formatter.console.print(f"Encrypted values written to {output_file}") diff --git a/src/keboola_agent_cli/commands/permissions.py b/src/keboola_agent_cli/commands/permissions.py index 946a6af6..c6cd5107 100644 --- a/src/keboola_agent_cli/commands/permissions.py +++ b/src/keboola_agent_cli/commands/permissions.py @@ -358,13 +358,29 @@ def permissions_check( ) -> None: """Check if a specific operation is allowed. + Reflects the EFFECTIVE policy for this invocation: the persisted + policy merged with any top-level session flags like ``--deny-writes`` + or ``--deny-destructive`` (issue #269 sec-19). Pre-fix, ``permissions + check`` only consulted the persisted policy, so an AI agent reading + its own self-imposed firewall flag would get a misleading answer. + Exit code 0 = allowed, 6 = denied. """ + from ..cli import apply_firewall_flags + formatter = get_formatter(ctx) config_store: ConfigStore = get_service(ctx, "config_store") config = config_store.load() - engine = PermissionEngine(config.permissions) + deny_writes = bool(ctx.obj.get("deny_writes")) if ctx.obj else False + deny_destructive = bool(ctx.obj.get("deny_destructive")) if ctx.obj else False + effective_policy = apply_firewall_flags( + config.permissions, + deny_writes=deny_writes, + deny_destructive=deny_destructive, + ) + + engine = PermissionEngine(effective_policy) allowed = engine.is_allowed(operation) if formatter.json_mode: diff --git a/src/keboola_agent_cli/commands/repl.py b/src/keboola_agent_cli/commands/repl.py index 7e8d80ff..af4feb59 100644 --- a/src/keboola_agent_cli/commands/repl.py +++ b/src/keboola_agent_cli/commands/repl.py @@ -5,6 +5,8 @@ completion, persistent history, and colored output. """ +import contextlib +import os import shlex import sys from pathlib import Path @@ -61,10 +63,28 @@ def get_completions(self, document, complete_event): def _get_history_path() -> Path: - """Return path for persistent REPL history file.""" + """Return path for persistent REPL history file. + + Ensures the file exists with 0600 permissions before returning so + ``prompt_toolkit.FileHistory`` does not create it world-readable + under a permissive umask. REPL command lines may include + ``project add --token TOKEN``, so the history must not leak to + group/other (issue #269 sec-04). + """ config_dir = Path(platformdirs.user_config_dir("keboola-agent-cli")) config_dir.mkdir(parents=True, exist_ok=True) - return config_dir / "repl_history" + history_path = config_dir / "repl_history" + if not history_path.exists(): + # Atomic create with 0600 so FileHistory.append never widens perms. + fd = os.open(str(history_path), os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) + os.close(fd) + else: + # Pre-existing file might have been created by an older kbagent + # under a permissive umask -- tighten perms in place. Filesystems + # that do not support chmod (e.g. some network mounts) silently no-op. + with contextlib.suppress(OSError): + history_path.chmod(0o600) + return history_path def _run_repl( diff --git a/src/keboola_agent_cli/models.py b/src/keboola_agent_cli/models.py index c5693674..024ac031 100644 --- a/src/keboola_agent_cli/models.py +++ b/src/keboola_agent_cli/models.py @@ -75,6 +75,7 @@ class AppConfig(BaseModel): default_project: str = Field(default="", description="Alias of the default project") max_parallel_workers: int = Field( default=10, + ge=1, le=100, description="Max concurrent threads for multi-project operations (env: KBAGENT_MAX_PARALLEL_WORKERS)", ) diff --git a/src/keboola_agent_cli/services/base.py b/src/keboola_agent_cli/services/base.py index b6a53453..6cd648d9 100644 --- a/src/keboola_agent_cli/services/base.py +++ b/src/keboola_agent_cli/services/base.py @@ -94,7 +94,11 @@ def _resolve_max_workers(self) -> int: """Resolve max parallel workers: env var > config.json > default (10). Returns: - Positive integer for ThreadPoolExecutor max_workers. + Positive integer for ThreadPoolExecutor max_workers. Always >= 1 + so a legacy config.json with ``max_parallel_workers: 0`` does not + crash multi-project ops with ``ValueError`` from the executor + (issue #269 sec-11). New configs are validated at the Pydantic + layer (``ge=1``); this clamp guards loaded-from-disk values. """ env_val = os.environ.get(ENV_MAX_PARALLEL_WORKERS) if env_val is not None: @@ -106,7 +110,7 @@ def _resolve_max_workers(self) -> int: pass config = self._config_store.load() - return config.max_parallel_workers + return max(config.max_parallel_workers, 1) def _run_parallel( self, diff --git a/src/keboola_agent_cli/services/deep_lineage_service.py b/src/keboola_agent_cli/services/deep_lineage_service.py index bf9c47d8..b64a64a3 100644 --- a/src/keboola_agent_cli/services/deep_lineage_service.py +++ b/src/keboola_agent_cli/services/deep_lineage_service.py @@ -8,6 +8,7 @@ """ import hashlib +import html import json import logging import re @@ -1153,7 +1154,11 @@ def render_er_diagram( if not t: continue entity_name = f"{t.project_alias}:{t.name}" - safe_name = entity_name.replace('"', "'") + # html.escape covers <>&" so an API-supplied table or config + # name like cannot inject HTML into + # the generated lineage page (issue #269 sec-05). Mermaid + # renders HTML entities back to their characters in SVG text. + safe_name = html.escape(entity_name, quote=True) if not show_columns: # Compact: just entity name with row count as single attribute @@ -1174,7 +1179,7 @@ def render_er_diagram( if src_expr.endswith(f".{col}") and fqn in src_expr.replace(f".{col}", ""): cfg = graph.configurations.get(cfg_fqn) cfg_label = cfg.config_name if cfg else cfg_fqn.split("/")[-1] - safe_label = cfg_label.replace('"', "'") + safe_label = html.escape(cfg_label, quote=True) comment = f' "to {out_col} via {safe_label}"' break if out_col == col: @@ -1201,7 +1206,7 @@ def render_er_diagram( for cfg_fqn in config_fqns: cfg = graph.configurations.get(cfg_fqn) cfg_label = cfg.config_name if cfg else cfg_fqn.split("/")[-1] - safe_label = cfg_label.replace('"', "'") + safe_label = html.escape(cfg_label, quote=True) inputs = input_of.get(cfg_fqn, []) outputs = output_of.get(cfg_fqn, []) for inp_fqn in inputs: @@ -1214,8 +1219,8 @@ def render_er_diagram( if key in seen_rels: continue seen_rels.add(key) - inp_name = f"{inp_t.project_alias}:{inp_t.name}".replace('"', "'") - out_name = f"{out_t.project_alias}:{out_t.name}".replace('"', "'") + inp_name = html.escape(f"{inp_t.project_alias}:{inp_t.name}", quote=True) + out_name = html.escape(f"{out_t.project_alias}:{out_t.name}", quote=True) lines.append(f' "{inp_name}" ||--o{{ "{out_name}" : "{safe_label}"') # If config has only inputs and no outputs (writer), show as relationship to config @@ -1224,7 +1229,7 @@ def render_er_diagram( inp_t = graph.tables.get(inp_fqn) if not inp_t: continue - inp_name = f"{inp_t.project_alias}:{inp_t.name}".replace('"', "'") + inp_name = html.escape(f"{inp_t.project_alias}:{inp_t.name}", quote=True) lines.append(f' "{inp_name}" }}o--|| "{safe_label}" : "writes"') return "\n".join(lines) diff --git a/src/keboola_agent_cli/services/mcp_transport.py b/src/keboola_agent_cli/services/mcp_transport.py index 4f4d1742..243326d2 100644 --- a/src/keboola_agent_cli/services/mcp_transport.py +++ b/src/keboola_agent_cli/services/mcp_transport.py @@ -14,6 +14,7 @@ import atexit import contextlib import logging +import os import socket import subprocess import time @@ -27,6 +28,45 @@ logger = logging.getLogger(__name__) +# Env vars the MCP subprocess needs (PATH for binary discovery, HOME for +# uv/pip caches, locale vars for Unicode handling, etc.). KBC_* tokens +# are intentionally NOT inherited — they would expose master/manage tokens +# to the MCP subprocess (issue #269 sec-02 / sec-08). Per-project +# Storage tokens are passed via per-request HTTP headers, not env. +_MCP_ENV_ALLOWLIST: tuple[str, ...] = ( + "PATH", + "HOME", + "USER", + "LOGNAME", + "TERM", + "SHELL", + "TMPDIR", + "TEMP", + "TMP", + "LANG", + "LC_ALL", + "LC_CTYPE", + "PYTHONPATH", + "PYTHONHOME", + "VIRTUAL_ENV", + # uv-specific cache locations so "uv tool run" picks up the right venv + "UV_CACHE_DIR", + "UV_PYTHON", + "XDG_CACHE_HOME", + "XDG_DATA_HOME", + "XDG_CONFIG_HOME", +) + + +def _build_minimal_env() -> dict[str, str]: + """Build an env dict for MCP subprocess containing only allow-listed keys. + + Strips ``KBC_MASTER_TOKEN*``, ``KBC_MANAGE_API_TOKEN``, ``KBC_TOKEN``, + and any other ``KBC_*`` variable so the MCP server does not inherit + secrets that belong to other auth scopes (issue #269 sec-02 / sec-08). + """ + return {key: os.environ[key] for key in _MCP_ENV_ALLOWLIST if key in os.environ} + def _find_free_port() -> int: """Find a free TCP port by binding to port 0 and reading the assigned port.""" @@ -116,6 +156,7 @@ def _start(self) -> str: cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, + env=_build_minimal_env(), ) self._port = port self._base_url = f"http://127.0.0.1:{port}" diff --git a/src/keboola_agent_cli/services/sync_service.py b/src/keboola_agent_cli/services/sync_service.py index 02cc4124..a3071cc3 100644 --- a/src/keboola_agent_cli/services/sync_service.py +++ b/src/keboola_agent_cli/services/sync_service.py @@ -62,6 +62,33 @@ logger = logging.getLogger(__name__) +def _ensure_within_branch( + branch_dir: Path, + config_dir: Path, + component_id: str, + config_id: str, +) -> None: + """Reject paths that escape the branch directory (issue #269 sec-01). + + Resolves both paths and checks that *config_dir* is contained in + *branch_dir*. Raises ConfigError if not. Defense-in-depth on top of + ``naming.sanitize_path_segment()`` so a regression in either layer + cannot turn into a path-traversal write. + """ + try: + branch_resolved = branch_dir.resolve() + config_resolved = config_dir.resolve() + except OSError as exc: + raise ConfigError(f"Cannot resolve sync path: {exc}") from exc + if not config_resolved.is_relative_to(branch_resolved): + raise ConfigError( + f"Config path escapes sync workspace (component='{component_id}', " + f"config_id='{config_id}'). Refusing to write outside " + f"'{branch_resolved}'. This indicates a malformed API response " + f"or a regression in path sanitization." + ) + + class SyncService(BaseService): """Business logic for project sync operations (init, pull, status). @@ -432,6 +459,11 @@ def pull( rel_path = f"{rel_path}-{suffix}" used_paths.add(rel_path) config_dir = branch_dir / rel_path + # Defense-in-depth: refuse to write outside the branch dir + # (issue #269 sec-01). naming.sanitize_path_segment() should + # already neutralize traversal; this check guards against any + # future regression in the sanitizer or template parsing. + _ensure_within_branch(branch_dir, config_dir, component_id, config_id) # Convert API format to local _config.yml local_data = api_config_to_local(component_id, cfg, config_id) diff --git a/src/keboola_agent_cli/sync/branch_mapping.py b/src/keboola_agent_cli/sync/branch_mapping.py index a3d0e2b3..acbbb389 100644 --- a/src/keboola_agent_cli/sync/branch_mapping.py +++ b/src/keboola_agent_cli/sync/branch_mapping.py @@ -19,10 +19,23 @@ def _coerce_keboola_id(raw: Any) -> int | None: Older kbagent versions (<= 0.30.3) wrote branch IDs as strings (e.g. ``"99999"``) due to issue #267. ``None`` means production. Empty string is also treated as production for legacy tolerance. + + Raises ``ValueError`` with a descriptive message if *raw* is neither + None, empty, nor parseable as an int (e.g. a hand-edited + ``branch-mapping.json`` containing ``"id": "not-a-number"``). The + caller (typically ``BranchMapping.from_dict``) should let this + bubble up to ``load_branch_mapping`` which converts it to a + ConfigError surface (issue #269 sec-20). """ if raw is None or raw == "": return None - return int(raw) + try: + return int(raw) + except (TypeError, ValueError) as exc: + raise ValueError( + f"Invalid branch ID in branch-mapping.json: {raw!r}. " + f"Expected null or an integer; got {type(raw).__name__}." + ) from exc class BranchMappingEntry: @@ -77,12 +90,24 @@ def from_dict(cls, data: dict[str, Any]) -> BranchMapping: def load_branch_mapping(project_root: Path) -> BranchMapping: - """Load .keboola/branch-mapping.json.""" + """Load .keboola/branch-mapping.json. + + Raises: + FileNotFoundError: If the mapping file does not exist. + ValueError: 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). + """ path = project_root / KEBOOLA_DIR_NAME / BRANCH_MAPPING_FILENAME if not path.exists(): raise FileNotFoundError(f"Branch mapping not found at {path}") - data = json.loads(path.read_text(encoding="utf-8")) - return BranchMapping.from_dict(data) + try: + 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 def save_branch_mapping(project_root: Path, mapping: BranchMapping) -> None: diff --git a/src/keboola_agent_cli/sync/naming.py b/src/keboola_agent_cli/sync/naming.py index c4c41a31..01e8d4de 100644 --- a/src/keboola_agent_cli/sync/naming.py +++ b/src/keboola_agent_cli/sync/naming.py @@ -21,10 +21,15 @@ def config_path( """Apply *naming_template* to generate a filesystem path for a configuration. Example template: ``"{component_type}/{component_id}/{config_name}"`` + + All template inputs are passed through sanitizers so that an API-controlled + ``component_id`` like ``"../../etc"`` cannot escape the sync workspace + (issue #269 sec-01/sec-07). Legitimate component IDs (e.g. + ``keboola.python-transformation-v2``) preserve their dots and hyphens. """ return naming_template.format( - component_type=component_type, - component_id=component_id, + component_type=sanitize_path_segment(component_type), + component_id=sanitize_path_segment(component_id), config_name=sanitize_name(config_name), ) @@ -59,3 +64,27 @@ def sanitize_name(name: str) -> str: result = result.strip("-") # Enforce max length return result[:SANITIZE_NAME_MAX_LENGTH] + + +def sanitize_path_segment(token: str) -> str: + """Sanitize an API-supplied token for use as a single path segment. + + Stricter-than-``sanitize_name`` defense against path traversal: rejects + ``/``, ``\\``, parent-directory references (``..``), and other directory + separators while preserving the dots, hyphens, and underscores commonly + found in legitimate component IDs (e.g. ``keboola.ex-db-mysql``, + ``kds-team.app-custom-python``). Returns ``"_"`` if the input would + sanitize to empty so the resulting path always has a non-empty segment + (issue #269 sec-01). + """ + # Replace path separators and whitespace with a single hyphen + result = re.sub(r"[/\\\s]", "-", token) + # Replace any run of 2+ dots (which would form parent refs) with + # a single underscore. Single dots are preserved for legitimate IDs. + result = re.sub(r"\.{2,}", "_", result) + # Strip leading dots that could be reintroduced as ``./...`` traversal + # if the template happens to put us at a directory boundary. + result = result.lstrip(".") + # Collapse repeated hyphens introduced by the substitutions + result = re.sub(r"-{2,}", "-", result).strip("-") + return result or "_" diff --git a/tests/test_deep_lineage_service.py b/tests/test_deep_lineage_service.py index b1e70c9a..5ee0d5c4 100644 --- a/tests/test_deep_lineage_service.py +++ b/tests/test_deep_lineage_service.py @@ -858,3 +858,84 @@ def test_build_empty_directory_exits_zero_with_warning(self, tmp_path: Path) -> assert len(data["warnings"]) == 1 assert "No synced projects found" in data["warnings"][0] assert cache_path.exists() + + +# --------------------------------------------------------------------------- +# Issue #269 sec-05: HTML/Mermaid output XSS regression +# --------------------------------------------------------------------------- + + +class TestRenderErDiagramXssRegression: + """Issue #269 sec-05 -- entity / config names from the API must not be + embeddable as HTML in the lineage HTML output.""" + + def _make_graph(self, table_name: str, config_name: str) -> tuple[LineageGraph, list[dict]]: + graph = LineageGraph() + table_fqn = f"prod:in.c-bucket.{table_name}" + config_fqn = "prod:keboola.snowflake-transformation/cfg-1" + graph.tables[table_fqn] = Table( + table_id=f"in.c-bucket.{table_name}", + project_alias="prod", + project_id=1, + bucket_id="in.c-bucket", + name=table_name, + primary_key=[], + columns=["id", "value"], + rows_count=100, + ) + graph.configurations[config_fqn] = Configuration( + config_id="cfg-1", + config_name=config_name, + component_id="keboola.snowflake-transformation", + component_type="transformation", + project_alias="prod", + project_id=1, + path="transformation/keboola.snowflake-transformation/cfg-1", + ) + edges = [ + { + "source": table_fqn, + "target": config_fqn, + "edge_type": "input_mapping", + "column_mapping": {}, + }, + { + "source": config_fqn, + "target": table_fqn, + "edge_type": "output_mapping", + "column_mapping": {}, + }, + ] + return graph, edges + + def test_table_name_with_html_is_escaped(self) -> None: + """A Keboola table named with ", + config_name="ok", + ) + rendered = DeepLineageService.render_er_diagram( + edges=edges, + graph=graph, + node_fqn=next(iter(graph.tables)), + show_columns=False, + ) + assert "` would inject the script into the generated HTML body where the browser parses it before Mermaid runs. Fix uses `html.escape(s, quote=True)` consistently for every API-derived string embedded into the Mermaid body and surrounding HTML. Mermaid renders the entities back to their characters in SVG text so visible output is unchanged. Issue #269 sec-05.", + "Security (high): `kbagent lineage show --format er` (and the lineage server's ER view) no longer emits XSS-vulnerable HTML. `services/deep_lineage_service.py:render_er_diagram()` previously did `name.replace('\"', \"'\")` which left `<`, `>`, and `&` untouched -- a Keboola table or config named `` would inject the script into the generated HTML body where the browser parses it before Mermaid runs. (`--format html` flowchart was already safe; it routes through `render_mermaid()` which escapes labels.) Fix uses `html.escape(s, quote=True)` consistently for every API-derived string embedded into the Mermaid body and surrounding HTML. Mermaid renders the entities back to their characters in SVG text so visible output is unchanged. Issue #269 sec-05.", "Security (high): `kbagent encrypt values --output-file PATH` now atomically creates the file with mode 0600. Pre-fix, `Path.write_text()` followed by `chmod(0o600)` left a race window where the encrypted secrets file was world-readable on systems with permissive umask (e.g. 0644 default). Replaced with `os.open(path, O_WRONLY|O_CREAT|O_TRUNC, 0o600)` + `os.write()`. Issue #269 sec-06.", "Security (medium): `max_parallel_workers` Pydantic field now requires `ge=1` in addition to `le=100`. Pre-fix, a config.json with `max_parallel_workers: 0` passed validation, then `ThreadPoolExecutor(max_workers=0)` crashed every multi-project operation with `ValueError`. `BaseService._resolve_max_workers()` also clamps to >= 1 defensively so a legacy on-disk config does not crash startup. Issue #269 sec-11.", "Security (low): `kbagent permissions check OPERATION` now reflects the EFFECTIVE policy for the current invocation -- the persisted policy MERGED with `--deny-writes` / `--deny-destructive` session flags -- matching `permissions list` semantics. Pre-fix, `permissions check` consulted only the persisted policy, so an AI agent inspecting its own self-imposed firewall got a misleading `allowed` answer for write ops. Issue #269 sec-19.", diff --git a/tests/test_sync_service.py b/tests/test_sync_service.py index a106f609..a660f0eb 100644 --- a/tests/test_sync_service.py +++ b/tests/test_sync_service.py @@ -2732,3 +2732,68 @@ def test_resolve_branch_id_dev_branch_without_mapping_still_errors( pytest.raises(ConfigError, match="not linked"), ): SyncService._resolve_branch_id(project, manifest, project_root) + + +# --------------------------------------------------------------------------- +# Issue #269 sec-01 -- defense-in-depth path-confinement guard +# --------------------------------------------------------------------------- + + +class TestEnsureWithinBranch: + """Direct unit tests for ``_ensure_within_branch`` defensive guard. + + The primary defense is ``naming.sanitize_path_segment()`` (covered in + test_sync_naming.py). This guard is belt-and-suspenders for the case + where the sanitizer regresses or a future code path bypasses it. + """ + + def test_passes_for_path_within_branch(self, tmp_path: Path) -> None: + """A normal config_dir under branch_dir does not raise.""" + from keboola_agent_cli.services.sync_service import _ensure_within_branch + + branch_dir = tmp_path / "main" + branch_dir.mkdir() + config_dir = branch_dir / "extractor" / "keboola.ex-http" / "my-config" + + # Should not raise + _ensure_within_branch(branch_dir, config_dir, "keboola.ex-http", "12345") + + def test_raises_for_path_escape_via_dotdot(self, tmp_path: Path) -> None: + """A config_dir that resolves outside branch_dir raises ConfigError.""" + from keboola_agent_cli.errors import ConfigError + from keboola_agent_cli.services.sync_service import _ensure_within_branch + + branch_dir = tmp_path / "main" + branch_dir.mkdir() + # Synthesize the misuse the sanitizer is supposed to prevent. + # The guard should fire even when the sanitizer didn't. + attacker_dir = branch_dir / ".." / "outside-workspace" / "config" + + with pytest.raises(ConfigError, match="escapes sync workspace"): + _ensure_within_branch(branch_dir, attacker_dir, "evil-component", "cfg-id") + + def test_raises_with_component_id_in_message(self, tmp_path: Path) -> None: + """Error message names the offending component / config so operators + can locate the bad API response.""" + from keboola_agent_cli.errors import ConfigError + from keboola_agent_cli.services.sync_service import _ensure_within_branch + + branch_dir = tmp_path / "main" + branch_dir.mkdir() + attacker_dir = branch_dir / ".." / "tmp" + + with pytest.raises(ConfigError) as excinfo: + _ensure_within_branch(branch_dir, attacker_dir, "k.ex-bad", "01abc") + assert "k.ex-bad" in str(excinfo.value) + assert "01abc" in str(excinfo.value) + + def test_passes_for_absolute_path_inside_branch(self, tmp_path: Path) -> None: + """An absolute path that happens to be inside branch_dir resolves OK.""" + from keboola_agent_cli.services.sync_service import _ensure_within_branch + + branch_dir = tmp_path / "main" + branch_dir.mkdir() + config_dir = branch_dir / "x" / "y" + + # Resolve and then re-pass: should still pass + _ensure_within_branch(branch_dir, config_dir.resolve(), "comp", "id") From 8417f4b3d496f2e04e44f53dd6709389eb2bb9c8 Mon Sep 17 00:00:00 2001 From: Petr Date: Thu, 7 May 2026 21:05:15 +0200 Subject: [PATCH 3/3] docs: address NB-1 from re-review of #272 Mirror the gotchas.md (since v0.30.5) entry into AGENT_CONTEXT in commands/context.py so AI agents that load context via 'kbagent context' (and not via gotchas.md) also know that 'permissions check' reflects session firewall flags as of 0.30.5. Reviewer noted this in the second review pass; same silent-drift class as the original B-1 fix in 4119793. Treating both referential surfaces as one update. --- src/keboola_agent_cli/commands/context.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/keboola_agent_cli/commands/context.py b/src/keboola_agent_cli/commands/context.py index a5750ce1..0c5e9b42 100644 --- a/src/keboola_agent_cli/commands/context.py +++ b/src/keboola_agent_cli/commands/context.py @@ -807,7 +807,10 @@ Remove all restrictions. kbagent permissions check OPERATION - Check if operation is allowed. Exit 0=allowed, 6=denied. + Check if operation is allowed. Exit 0=allowed, 6=denied. Reflects the + EFFECTIVE policy: persisted policy MERGED with --deny-writes / + --deny-destructive session flags (since 0.30.5; pre-0.30.5 consulted + only the persisted policy and could mislead self-introspection). ## Tips for AI Agents