fix(serve): agent-runner hardening — isolate admin tokens + gate extra_args (GHSA-wm54 + GHSA-777j, 0.60.2) - #411
Conversation
…0.60.2) Scheduled ai_agent tasks (claude/codex/gemini) spawned by `kbagent serve` inherited the full serve `os.environ`, so a prompt-injectable AI agent -- meant only to e.g. summarize jobs -- could read KBC_MANAGE_API_TOKEN / KBC_MASTER_TOKEN* from its own environment and exfiltrate the highest-value credentials. `_build_subprocess_env(strip_admin_tokens=True)` now strips every KBC_MANAGE_* / KBC_MASTER_* key from the ai_agent child env, mirroring the MCP-child isolation in mcp_transport._build_minimal_env and the manage-token default-deny. KBC_TOKEN (per-project storage token) is retained so headless `--project __env__` reads still work; cli_command children -- which are `kbagent` itself and need the tokens for scheduled project-refresh/sharing tasks -- are unchanged. Private advisory GHSA-wm54-r2hh-cxm9. Behavior-preserving: no AI CLI legitimately needs the manage/master token.
…GS opt-in ai_agent extra_args were forwarded verbatim to the AI CLI (claude/codex/ gemini), so a task definition -- or any holder of the serve bearer token, including the immediate /agents/test endpoint -- could inject a rail-disabling flag (permission-skip / unrestricted-execution) and turn a contained headless agent into arbitrary host command execution. extra_args are now ignored unless the serve operator opts in via a truthy KBAGENT_ALLOW_AI_EXTRA_ARGS; otherwise they are dropped with a loud warning. Mirrors the --allow-env-manage-token opt-in. Both the one-shot and streaming ai_agent paths share `_resolve_ai_extra_args`. Private advisory GHSA-777j-6p95-qv3m.
padak
left a comment
There was a problem hiding this comment.
Review of #411 — fix(serve): agent-runner hardening — isolate admin tokens + gate extra_args (GHSA-wm54 + GHSA-777j, 0.60.2)
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
The PR ships two scoped security fixes in server/agent_runner.py: (M1) ai_agent children no longer inherit KBC_MANAGE_* / KBC_MASTER_* env vars (strip_admin_tokens=True on both the one-shot and streaming spawn paths, cli_command untouched), and (M3) extra_args are dropped unless KBAGENT_ALLOW_AI_EXTRA_ARGS is truthy, with a loud parent-process warning. Both fixes are correctly implemented and I verified both live end-to-end with a fake claude shim: admin tokens absent from the child env, KBC_TOKEN retained, injected --dangerously-skip-permissions dropped without opt-in, --model opus forwarded with opt-in. make check is green. Verdict is REQUEST CHANGES for one reason only: the extra_args gate is a user-visible behavior change shipping in 0.60.2, and none of the hand-maintained silent-drift surfaces (notably gotchas.md with a (since v0.60.2) tag) were updated — the agent-tasks workflow references still teach "extra_args": ["--print"] as unconditionally effective.
Verdict
- Verdict: REQUEST CHANGES
- Blocking findings: 1
- Non-blocking findings: 3
- Nits: 1
Blocking findings
[B-1] src/keboola_agent_cli/server/agent_runner.py:746 — extra_args opt-in gate shipped without updating any silent-drift doc surface
_resolve_ai_extra_args makes extra_args a no-op by default — a non-obvious behavior change an AI agent will get wrong, because the shipped references still teach the old behavior: plugins/kbagent/skills/kbagent/references/agent-tasks-cli-workflow.md:56 and agent-tasks-rest-workflow.md:69 both use "extra_args": ["--print"] as the canonical example, commands-reference.md:326,335 and src/keboola_agent_cli/commands/context.py:1058,1106 document --extra-arg unconditionally, and server/agents_store.py:36 calls them "optional CLI-specific flags". An agent following those docs creates a task whose args are silently ignored at run time. Per CONTRIBUTING.md > Plugin synchronization map, this requires at minimum a plugins/kbagent/skills/kbagent/references/gotchas.md entry tagged (since v0.60.2) (currently absent — verified by grep), plus one-line notes on the surfaces above. The M1 token strip needs no gotcha (legitimate AI children never used those tokens); M3 does.
Non-blocking findings
[NB-1] src/keboola_agent_cli/server/agent_runner.py:767 — drop warning is misleading on the local CLI path and invisible to the actual task owner
The warning text says "...is not set on the serve process", but services/agent_service.py reuses the same runner for local kbagent agent test/run/prompt-improve — I reproduced this: a serve-less local agent test printed the "serve process" warning verbatim. Worse, the warning goes only to the parent process logger, so a REST caller or Web UI user (web/frontend/src/pages/Agents.tsx:572 sends user-typed extra args) gets no in-band signal that their args were dropped — the run record/SSE stream carries nothing. Suggest (a) neutral wording ("on the kbagent process"), and (b) surfacing the drop as a structured warning in the run output/events so the task owner sees it, not just the operator tailing logs.
[NB-2] src/keboola_agent_cli/changelog.py:58 — "serve operator opts in" framing undersells the blast radius; "behavior-preserving for legitimate use" is not accurate for local flows
The changelog entry and PR description frame the gate as a serve-side control, but the gate fires in every consumer of the shared runner: local agent test/run --extra-arg, agent prompt-improve --extra-arg (services/agent_service.py:432), the Web UI ai-chat (server/routers/ai_chat.py:93) and SQL helper (server/routers/workspaces.py:235). A user passing --extra-arg "--model" --extra-arg "opus" on their own machine is a legitimate use that now requires an env opt-in in their own shell. This may well be the right call (safe-by-default), but the changelog should say so explicitly so users debugging "my --extra-arg stopped working" find the answer.
[NB-3] src/keboola_agent_cli/server/agent_runner.py:948 — streaming spawn path has no regression test for either fix
The new integration tests assert spawn kwargs only on the one-shot _run_ai_agent path. The streaming path (stream_ai_agent_events) is the production-dominant one (serve REST, run broadcaster, Web UI, and — verified live — local agent test), yet a revert of strip_admin_tokens=True on its create_subprocess_exec call (or of its _resolve_ai_extra_args use at line 921) would pass the current suite. One test mirroring test_ai_agent_subprocess_strips_admin_tokens against stream_ai_agent_events closes the gap.
Nits
[NIT-1]src/keboola_agent_cli/server/agent_runner.py:765— third copy of the truthy-env idiom("1", "true", "yes", "on")(alsoconfig_store.py:214,commands/version.py:186). Worth extracting a sharedenv_truthy()helper rather than adding instance #3.
Verification log
gh pr view 411 --json ...→ 8 files, +242/−14, state OPEN, conventionalfix(serve):✓git rev-parse --abbrev-ref HEAD→fix/agent-runner-token-isolation(matches PR head) ✓- Layer greps on the diff (typer/formatter in services, httpx in commands) → empty ✓; convention greps (magic numbers, raw error codes, bare except, print(), unmasked tokens) → clean; test tokens follow the
901-fake convention ✓ - Plugin sync map walk: no command added/removed →
OPERATION_REGISTRY, hints, routers, E2E N/A; version bump propagated (pyproject.toml/plugin.json/marketplace.json/uv.lockall 0.60.2); changelog entry present;grep extra_arg|ALLOW_AI gotchas.md keboola-expert.md CLAUDE.md→ no updates (basis of B-1) ✗ make check→ final stage3969 passed, 8 skipped, 124 deselected(lint/format/changelog gates precede tests in the target) ✓- Live repro (fake
claudeshim on PATH, fake901-*tokens in env, isolated--config-dir):kbagent --json agent test --type ai_agent --cli claude --prompt hello --extra-arg --dangerously-skip-permissions→ exit 0, argv WITHOUT the flag, stderr warningIgnoring 1 ai_agent extra_args because KBAGENT_ALLOW_AI_EXTRA_ARGS is not set on the serve process...(note: printed on a serve-less local run — NB-1) ✓- same with
KBAGENT_ALLOW_AI_EXTRA_ARGS=1and--extra-arg --model --extra-arg opus→ child argv contains--model opus✓ - child env probe →
KBC_MANAGE_API_TOKEN/KBC_MASTER_TOKEN/KBC_MASTER_TOKEN_PRODabsent,KBC_TOKENretained ✓ (M1 verified through the real streaming spawn path)
- Spawn-path audit: all AI-child spawns funnel through
_run_ai_agent/stream_ai_agent_events(callers:routers/agents.py,routers/ai_chat.py,routers/workspaces.py,run_broadcaster.py,services/agent_service.py) — no unpatched spawn left ✓; prefix strip covers every real env name (constants.py:176-177,commands/sharing.py:128-129) ✓; no server-internal caller relies onextra_args(all producers are request-body fields defaulting to[]) ✓
Open questions for the author
- Is gating the local
agent prompt-improve --extra-argpath intended, or collateral from sharing the runner? If intended, NB-2's changelog wording covers it; if not, the gate may want to be scoped to registry-with-serve contexts.
…, streaming test) Addresses the kbagent-pr-reviewer findings on #411: - B-1 (blocking): document the extra_args opt-in gate on every silent-drift surface -- new gotchas.md entry (since v0.60.2), agent-tasks-{cli,rest} workflow refs, commands-reference.md, and context.py AGENT_CONTEXT. - NB-1: the drop warning + docs no longer say "serve process" (the gate also fires on local `agent test/run/prompt-improve`); reworded to "kbagent environment". - NB-2: changelog now states the gate applies to local --extra-arg flows too, not just serve-scheduled tasks. - NB-3: add a streaming-path (stream_ai_agent_events) regression test covering both the admin-token strip and the extra_args gate. NIT-1 (extract a shared env_truthy helper) left as a follow-up to keep this PR scoped to the security fix + its docs.
Author response — review findings addressed (commit 0a03f4e)Thanks for the thorough pass, and for verifying both fixes live through the streaming path. All blocking + non-blocking findings are addressed; the one nit is deferred as a scoped follow-up.
Open question (gating local
|
Summary
Two related security fixes in the
kbagent servetask scheduler (agent_runner.py), both from the 2026-06-12 audit. Both are behavior-preserving for legitimate use.M1 — admin-token isolation (private advisory GHSA-wm54-r2hh-cxm9)
_build_subprocess_envcopied the fullos.environinto every spawned child. A scheduledai_agenttask (claude/codex/gemini) — autonomous and prompt-injectable by the Keboola data it reads — couldprintenv/ reados.environand exfiltrateKBC_MANAGE_API_TOKEN/KBC_MASTER_TOKEN*, the highest-value (super-admin / cross-project) credentials. Same class the project already closed twice (manage-token default-deny v0.29.0; MCP_build_minimal_env#269), applied everywhere except this riskiest child.Fix:
_build_subprocess_env(strip_admin_tokens=True)strips everyKBC_MANAGE_*/KBC_MASTER_*key (prefix match also catches per-aliasKBC_MASTER_TOKEN_<ALIAS>) from the ai_agent child only.kbagent http/ forkskbagentagainstKBAGENT_CONFIG_DIR).KBC_TOKEN(storage) is retained so headless--project __env__reads work.cli_commandchildren are unchanged — they arekbagentitself and need the tokens for scheduledproject refresh/sharingtasks.M3 — extra_args opt-in gate (private advisory GHSA-777j-6p95-qv3m)
ai_agentextra_argswere forwarded verbatim to the AI CLI, so a task definition — or any holder of the serve bearer token, including the immediate/agents/testendpoint — could inject a rail-disabling flag (permission-skip / unrestricted-execution) and turn a contained headless agent into arbitrary host command execution.Fix (option A, owner-chosen):
extra_argsare now ignored by default and only honored when the serve operator opts in via a truthyKBAGENT_ALLOW_AI_EXTRA_ARGS. When dropped, a loud warning is logged so the drop stays visible. Mirrors the--allow-env-manage-tokenopt-in. Both the one-shot and streaming ai_agent paths share_resolve_ai_extra_args.Tests
10 new tests in
test_agent_runner.py(4 M1 + 6 M3): admin-token stripping (unit + integration, retainsKBC_TOKEN/serve-token, cli_command keeps tokens) and the extra_args gate (dropped without opt-in, honored with it, empty/non-list handling, spawn-argv integration). Full suite green: 3969 passed, 132 skipped; lint/format/ty/changelog clean.Deliberately NOT in this PR (need a separate decision)
KBC_TOKENfrom ai_agent (breaks headless fork-kbagent) or stripping admin tokens fromcli_command(breaks scheduled admin tasks).