fix(codex): TOML-escape MCP command/args/env in -c overrides#404
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #404 +/- ##
=======================================
Coverage ? 88.09%
=======================================
Files ? 132
Lines ? 16048
Branches ? 0
=======================================
Hits ? 14137
Misses ? 1911
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request updates how the bundled cao-mcp-server MCP server is represented and launched across providers, and hardens Codex -c TOML overrides against unescaped special characters (quotes/backslashes/newlines) that could otherwise break TOML parsing or split tmux send_keys across lines.
Changes:
- Add a shared
resolve_cao_mcp_command/resolve_mcp_server_confighelper and use it across providers and install flows to rewrite the bundledcao-mcp-servercommand into a PATH-independent invocation. - TOML-escape Codex
-coverrides for MCP servercommand,args,env, andenv_varsusing the existing_toml_scalarserializer (and use it fordeveloper_instructionsas well). - Update bundled profiles/examples/docs to declare
command: cao-mcp-server(nouvx --from git+https://...), remove the e2e uvx cache warmup, and add/extend unit tests to cover the new behavior.
Reviewed changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utils/test_opencode_config.py | Updates OpenCode MCP translation tests to distinguish bundled resolution from custom uvx passthrough. |
| test/utils/test_mcp_resolution.py | Adds unit tests for cao-mcp-server command resolution and config wrapping behavior. |
| test/utils/test_bundled_profiles.py | Adds regression tests to ensure bundled profiles don’t reintroduce uvx git+https network fetch. |
| test/services/test_install_service.py | Extends kiro MCP timeout detection tests for new cao-mcp-server command forms. |
| test/providers/test_codex_provider_unit.py | Adds tests for Codex MCP command resolution and TOML escaping of MCP overrides. |
| test/providers/test_antigravity_cli_unit.py | Adds test asserting persisted MCP config writes use resolved (PATH-independent) command. |
| test/e2e/conftest.py | Removes the now-unneeded uvx cache warmup fixture. |
| src/cli_agent_orchestrator/utils/opencode_config.py | Resolves bundled cao-mcp-server before flattening into OpenCode’s command list (persisted=True). |
| src/cli_agent_orchestrator/utils/mcp_resolution.py | Introduces shared resolver for rewriting bundled cao-mcp-server command to a concrete invocation. |
| src/cli_agent_orchestrator/services/install_service.py | Resolves MCP server commands during install so persisted provider configs don’t depend on PATH at runtime. |
| src/cli_agent_orchestrator/providers/kimi_cli.py | Applies shared MCP resolver when assembling Kimi CLI MCP config. |
| src/cli_agent_orchestrator/providers/cursor_cli.py | Applies shared MCP resolver when writing Cursor plugin MCP server configs. |
| src/cli_agent_orchestrator/providers/copilot_cli.py | Replaces inline cao-mcp-server resolution logic with the shared resolver. |
| src/cli_agent_orchestrator/providers/codex.py | Resolves bundled MCP server config and TOML-escapes MCP-related -c overrides (including developer_instructions). |
| src/cli_agent_orchestrator/providers/claude_code.py | Applies shared MCP resolver when assembling Claude Code MCP config. |
| src/cli_agent_orchestrator/providers/antigravity_cli.py | Resolves bundled cao-mcp-server before writing persisted mcp_config.json; updates related comment. |
| src/cli_agent_orchestrator/agent_store/workflow_scout.md | Switches bundled profile MCP server command from uvx git+https to cao-mcp-server. |
| src/cli_agent_orchestrator/agent_store/reviewer.md | Switches bundled profile MCP server command from uvx git+https to cao-mcp-server. |
| src/cli_agent_orchestrator/agent_store/memory_manager.md | Switches bundled profile MCP server command from uvx git+https to cao-mcp-server. |
| src/cli_agent_orchestrator/agent_store/developer.md | Switches bundled profile MCP server command from uvx git+https to cao-mcp-server. |
| src/cli_agent_orchestrator/agent_store/code_supervisor.md | Switches bundled profile MCP server command from uvx git+https to cao-mcp-server. |
| examples/orchestration/reviewer-opus.md | Updates example profile MCP server command to cao-mcp-server. |
| examples/orchestration/dev-sonnet.md | Updates example profile MCP server command to cao-mcp-server. |
| examples/orchestration/dev-opus.md | Updates example profile MCP server command to cao-mcp-server. |
| examples/orchestration/dev-kimi.md | Updates example profile MCP server command to cao-mcp-server. |
| examples/cross-provider/report_generator_codex.md | Updates example profile MCP server command to cao-mcp-server. |
| examples/cross-provider/README.md | Updates embedded example snippet to cao-mcp-server. |
| examples/cross-provider/data_analyst_kiro_cli.md | Updates example profile MCP server command to cao-mcp-server. |
| examples/cross-provider/data_analyst_kimi_cli.md | Updates example profile MCP server command to cao-mcp-server. |
| examples/cross-provider/data_analyst_copilot_cli.md | Updates example profile MCP server command to cao-mcp-server. |
| examples/cross-provider/data_analyst_codex.md | Updates example profile MCP server command to cao-mcp-server. |
| examples/cross-provider/data_analyst_claude_code.md | Updates example profile MCP server command to cao-mcp-server. |
| examples/cross-provider/cross_provider_supervisor.md | Updates example profile MCP server command to cao-mcp-server. |
| examples/codex-basic/codex_reviewer.md | Updates Codex-basic example MCP server command to cao-mcp-server. |
| examples/codex-basic/codex_documenter.md | Updates Codex-basic example MCP server command to cao-mcp-server. |
| examples/codex-basic/codex_developer.md | Updates Codex-basic example MCP server command to cao-mcp-server. |
| examples/aws/stepfunction/stepfunction-agent.md | Updates AWS example MCP server command to cao-mcp-server. |
| examples/aws/sqs-send/sqs-send-agent.md | Updates AWS example MCP server command to cao-mcp-server. |
| examples/aws/sqs-monitor/sqs-monitor-agent.md | Updates AWS example MCP server command to cao-mcp-server. |
| examples/aws/sqs-dlq-check/sqs-dlq-check-agent.md | Updates AWS example MCP server command to cao-mcp-server. |
| examples/aws/dynamodb-query/dynamodb-query-agent.md | Updates AWS example MCP server command to cao-mcp-server. |
| examples/aws/dynamodb-delete/dynamodb-delete-agent.md | Updates AWS example MCP server command to cao-mcp-server. |
| examples/aws/cloudwatch-logs/cloudwatch-logs-agent.md | Updates AWS example MCP server command to cao-mcp-server. |
| examples/assign/report_generator.md | Updates assign example MCP server command to cao-mcp-server. |
| examples/assign/README.md | Updates embedded assign example snippet to cao-mcp-server. |
| examples/assign/data_analyst.md | Updates assign example MCP server command to cao-mcp-server. |
| examples/assign/analysis_supervisor.md | Updates assign example MCP server command to cao-mcp-server. |
| docs/agent-profile.md | Updates docs example MCP server configuration to use cao-mcp-server directly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sibling = _sibling_script() | ||
| on_path = shutil.which(CAO_MCP_SERVER_COMMAND) | ||
| order = ( | ||
| [("PATH", on_path), ("sibling", sibling)] | ||
| if persisted | ||
| else [ | ||
| ("sibling", sibling), | ||
| ("PATH", on_path), | ||
| ] | ||
| ) | ||
| for label, candidate in order: | ||
| if candidate: | ||
| logger.debug("Resolved %s via %s: %s", command, label, candidate) | ||
| return candidate, [] | ||
|
|
||
| # Module entrypoint via the current interpreter — runnable without any | ||
| # console script on PATH. Falls back to a bare ``python3`` only if | ||
| # sys.executable is unavailable (best effort in degenerate environments). | ||
| interpreter = sys.executable or "python3" | ||
| logger.debug("Resolved %s to module entrypoint via %s", command, interpreter) | ||
| return interpreter, ["-m", CAO_MCP_SERVER_MODULE] |
| # Resolve the bundled cao-mcp-server console script to a | ||
| # PATH-independent invocation. | ||
| cfg = resolve_mcp_server_config(cfg) | ||
| if "command" in cfg: | ||
| command_parts.extend(["-c", f'{prefix}.command="{cfg["command"]}"']) | ||
| command_parts.extend( | ||
| ["-c", f"{prefix}.command={_toml_scalar(cfg['command'])}"] | ||
| ) |
There was a problem hiding this comment.
That's because this PR is stacked on #403 (noted in the description) — the resolver and provider changes are #403's commit, included here so the branch is testable standalone. The diff belonging to this PR is codex.py + its tests only; once #403 merges, GitHub will collapse this to just that. Happy to rebase directly onto main after #403 lands if you prefer.
tedswinyar
force-pushed
the
fix/cao-codex-toml-escape
branch
from
3bd15c1 to
b0b355b
Compare
gutosantos82
left a comment
gutosantos82
left a comment
There was a problem hiding this comment.
PR Review: #404 — fix(codex): TOML-escape MCP command/args/env in -c overrides
Summary
This PR TOML-escapes the MCP command/args/env/env_vars values that the Codex
provider injects as -c mcp_servers.<name>.<field>="<value>" launch overrides, routing them
all (plus developer_instructions) through the existing _toml_scalar helper so a value
containing "/\/newline can no longer break out of the TOML string or split the tmux
send_keys paste. Because it is stacked on #403, the diff vs main also includes a new
shared utils/mcp_resolution.py helper wired into six providers + install_service +
opencode_config, and the bundled agent profiles' switch from uvx --from git+… to the bare
cao-mcp-server console script. The core escaping fix is correct and independently verified;
one introduced regression and two hygiene gaps should be addressed before merge.
Blocking (must fix before merge)
None — no security-critical or build-breaking defect. The item below is a real introduced
regression but its blast radius is limited to custom non-stdio MCP servers; treating it as
top-priority Important.
Important (should fix)
- 🆕 [correctness] src/cli_agent_orchestrator/utils/mcp_resolution.py:996-1002 —
resolve_mcp_server_config()unconditionally writes backcommandandargs. For a
non-stdio server (http/sse, shaped{type, url}with nocommand),
resolved.get("command","")→"", and the returned config gainscommand=""/args=[]
that weren't in the input. Downstream this corrupts real configs:codex.py:286
(if "command" in cfg) becomes always-true → emits an invalid empty
mcp_servers.<name>.command="", andclaude_code.py:257/cursor_cli.py:510/
kimi_cli.pywrite"command": "", "args": []into JSON for URL servers. Before this PR
Codex used the config verbatim, so a URL-only server never emitted a command. Fix: guard
the write-back on the input actually having acommand(e.g.if "command" in config:/
skip when falsy). Path-weighted (feedsproviders/), a genuine regression — but only bites
custom http/sse MCP servers; the bundled stdiocao-mcp-serverpath is unaffected.
(introduced) - 🆕 [tests] resolution wiring untested for 4 of 6 providers — the resolver is asserted
for codex (test_bundled_mcp_command_is_resolved), antigravity
(test_mcp_registration_resolves_bundled_command), opencode, and install_service. But the
newresolve_mcp_server_config()calls atclaude_code.py:255,cursor_cli.py:508,
kimi_cli.py:342, and copilot's rewritten_build_runtime_mcp_confighave no test
asserting the barecao-mcp-serveris actually rewritten in the emitted config. A future
refactor could silently drop the resolve() call on those four and CI stays green. Add one
wiring assertion per provider. (introduced) - 🆕 [conventions] src/cli_agent_orchestrator/providers/copilot_cli.py:11,13 —
import shutilandimport sysare now dead (their only uses moved intomcp_resolution.py;
Pathis still used at line 140, keep it). CI won't catch this — the lint job runs only
black/isort/mypy, no flake8/ruff — so it must be caught in review. (introduced) - 🆕 [conventions] CHANGELOG.md — no entry for a user-facing behavior change (bundled
profiles now use the bare console script; providers resolve it to a PATH-independent
invocation). Repo convention is an## [Unreleased]entry per user-facing change; add a
Fixed entry referencing #404. (introduced)
Nits (optional)
- [security] mcp_resolution.py:964-971 —
persisted=Trueprefers the PATH launcher
(shutil.which) over the hijack-proof interpreter-sibling absolute path, and the result is
written to a config read at later launches. Acao-mcp-serverplanted earlier on the
cao-server process's PATH could be resolved and persisted. Documented upgrade-stability vs
hijack-resistance tradeoff, same trust model as any PATH lookup — acceptable, just noting
thepersisted=Falseruntime path is strictly safer. (introduced) - [security] codex.py:142-148 —
_toml_scalarescapes\t/\r/\nbut not other C0
controls (NUL,\x0b,\x0c, ESC). Not a breakout (shlex.join wraps the arg; only\n/\r
split the paste, both covered) but such a byte makes Codex's TOML parser reject the override
→ silent launch failure. Robustness only. (introduced) - [tests] test_codex_provider_unit.py (args escaping test) — asserts the escaped arg is
present and no raw newline, but doesn't assert the raw unescaped arg is absent — asymmetric
vs the command test, low risk. (introduced) - [tests] test/e2e/script_runner/conftest.py:24-27 — after the parent
warmup_mcp_server_cachefixture was removed, this child still defines a no-op override
(and its L7 docstring still lists it as a parent fixture). Harmless but dead/misleading;
remove it. (introduced)
Prior feedback (already raised — not restating)
None — the PR has no prior human review comments.
Pre-existing (adjacent — PR is a natural place to close, not blocking)
- [security] codex.py:278,294 — the
-ckey halves (server_name,env_key) are
interpolated raw, unlike the values. A name/key with"yields malformed TOML; one with a
newline isn't run through_toml_scalar(thoughshlex.joinat :328 blocks shell breakout).
The PR's owncodexConfigpath already validates keys via_CODEX_CONFIG_KEY_PATTERN
(codex.py:152,164) — the MCPserver_name/env_keydeserve the same guard. Low practical
risk (keys come from operator-installed profiles). - [security] clients/tmux.py:287 —
logger.info(f"send_keys: … keys: {keys}")logs the
full launch command at INFO, which includes every MCPenvvalue and the entire
developer_instructionssystem prompt in cleartext. A token in a profile'smcpServers.env
lands in CAO logs. (The newmcp_resolution.pylogger.debugcalls are clean — command
name + resolved paths only.) Consider redacting / gating full content behind DEBUG.
Tests
Strong. New test/utils/test_mcp_resolution.py (194L) covers all three tiers
(sibling/PATH/module), both persisted modes incl. fallbacks, passthrough, arg-copy
(non-aliasing), idempotency, and the empty-sys.executable frozen-interpreter edge. New
test/utils/test_bundled_profiles.py parametrizes over all agent_store/*.md asserting the
console-script command and a regression guard against reintroducing the uvx git+https fetch.
The codex escaping tests assert both the escaped form present and the raw form absent (for
command + newline). Main gap: the resolution-wiring assertion missing for four providers
(see Important). The removed e2e uvx-warmup fixture leaves no test broken (it primed the cache
for the fetch this PR eliminates).
Verification
Verifier ran in a clean Docker (uv, py3.12); worktree left clean.
- Baseline:
uv run pytestover the 6 touched test files → 267 passed, 0 failed. - ✓ VERIFIED TOML escaping prevents string-breakout / stays single-line — fed
_toml_scalar
",\, tab, CR, newline + a breakout payload; every output was a valid single-line TOML
basic string, round-tripped viatomllibback to the exact input for all 7 cases. Scalars
bool/int/float correct (bool checked before int); non-scalars raise TypeError (fails fast). - ✓ VERIFIED (differential vs origin/main) the fix changes behavior — the OLD unescaped
path let a crafted value inject a top-level TOML key (evil='pwn'); the NEW output parses
back to the original command with no injected key. OLD also missed tab/CR (line-split risk);
NEW escapes them. Proves a real fix, not just a passing test. - ✓ VERIFIED
resolve_cao_mcp_commandtiers match the docstring — all 6 orderings
(passthrough; non-persisted sibling-first; persisted PATH-first; module fallback appending
args after-m <module>) behaved exactly as documented.
Verdict
Request changes — the core escaping fix is correct, verified, and defense-in-depth (shlex.join
backstop). Before merge: (1) guard resolve_mcp_server_config against injecting an empty
command/args into non-stdio MCP configs, (2) drop the dead shutil/sys imports in
copilot_cli, (3) add a CHANGELOG entry, and ideally add the four missing provider wiring
assertions. All are small; the PR is close.
Consistency-angle review did not return; its scope (PR-description↔diff mismatch, cross-provider
persisted= split) is largely covered here — the PR body's "codex.py + its tests only" claim is
inaccurate against the 49-file diff (an artifact of stacking on #403), and the correctness review
independently confirmed the persisted=True/False split across providers is correct.
Code Review: TOML-escaping is incomplete —
|
tedswinyar
force-pushed
the
fix/cao-codex-toml-escape
branch
from
b0b355b to
e06465b
Compare
|
Thanks — I think the command-field finding may have raced a force-push: the head you reviewed against shows the pre-update code. Since if "command" in cfg:
command_parts.extend(
["-c", f"{prefix}.command={_toml_scalar(cfg['command'])}"]
)so On the minor env_vars finding: agreed, and the fail-fast is intentional — a malformed profile now fails at build time with a clear TypeError instead of emitting a silently-broken override. Added |
|
Thanks — the differential verification against origin/main (the injected-key proof) is a great way to review an escaping change. Point by point:
On the two pre-existing items (raw |
gutosantos82
left a comment
gutosantos82
left a comment
There was a problem hiding this comment.
PR Review: #404 — fix(codex): TOML-escape MCP command/args/env in -c overrides
Summary
This PR routes every value the Codex provider injects as a -c mcp_servers.<name>.<field>
launch override — command, each arg, each env value, the env_vars list, and
developer_instructions — through the existing _toml_scalar() helper instead of raw
f-string interpolation, so a value containing "/\/newline can no longer break out of the
TOML basic string or split the tmux send_keys paste. The core fix is correct, complete
for the exploitable vectors, and independently verified (differential proof below shows the
old path allowed a real top-level TOML-key injection and the new path closes it). Because it
is stacked on #403, GitHub shows a 49-file combined diff, but #404's own commit
(e06465b) is exactly providers/codex.py + test/providers/test_codex_provider_unit.py —
confirmed. No blocking defect; a few small should-fix/nit items remain.
Blocking (must fix before merge)
None — the escaping fix is correct and verified; no security-critical or build-breaking
defect introduced.
Important (should fix)
- 🆕 [conventions] CHANGELOG.md — no
## [Unreleased]entry for #404's own user-facing
fix (MCP-coverrides with quotes/backslashes/newlines previously emitted malformed TOML,
breaking launch). The only CHANGELOG change in the combined diff is #403's resolver entry.
Repo convention is one Unreleased### Fixedbullet per user-facing change with a PR ref.
Add e.g. "codex: TOML-escape MCP server command/args/env in-coverrides (#404)".
(introduced) - 🆕 [tests] test/providers/test_codex_provider_unit.py — the
env_varslist positive
escaping path (codex.py:305,", ".join(_toml_scalar(v) ...)) has no test feeding a
special-char string intoenv_varsand asserting it comes out escaped. Existingenv_vars
tests cover only the non-string fail-fast and plain names (HOME/PATH); theenvdict
path is escape-tested but the list path is not. A regression revertingenv_varsto a raw
f-string would still pass every current test. Add one special-charenv_varsassertion.
(introduced) Path-weighted (providers/) — lean toward addressing before merge.
Nits (optional)
- [tests] test_codex_provider_unit.py (command negative assertion) — the command test's
raw-absent check (assert '"/tmp/we"ird' not in command) is borderline vacuous: the emitted
override is...command="..."with the opening quote flush againstcommand=, so
"/tmp/we"ird(quote-immediately-before-slash) never appears even in an unescaped build.
Tighten to the truly-dangerous substring, e.g.assert r'/tmp/we"ird\path' not in command.
The args/env negatives are correctly constructed. (introduced) - [consistency] codex.py:267-269 — stale comment. The block comment "Escape backslashes,
double quotes, and newlines for TOML basic string…" now sits directly above a call to
_toml_scalar(system_prompt), which owns that escaping (and also escapes tab/CR, more than
the comment says). Trim it to note escaping is delegated to_toml_scalar. (introduced) - [correctness] codex.py:305 —
env_varsentries are passed to_toml_scalarwithout
thestr()coercion applied toenvvalues at :296. A dict entry correctly raises
TypeError(tested, intended), but anintentry would emit a bare int → a mixed-type TOML
array (env_vars=[123,"CAO_TERMINAL_ID"]) that Codex'sVec<String>deserialize rejects
rather than failing fast. Low risk (env var names are strings in practice). (introduced)
Prior feedback (already raised — not restating)
- ↩︎
commandfield left unescaped — raised by @fanhongy (Request changes), but against a
pre-force-push head; on the current heade06465bcommandgoes through_toml_scalar
(verified below) andtest_mcp_server_command_field_is_toml_escapedpasses. @tedswinyar
clarified the race; we concur — resolved, not a live finding. - ↩︎ non-string
env_varsfail-fast is untested — raised by @fanhongy; @tedswinyar added
test_mcp_env_vars_non_string_entry_fails_fastand documented the intentional fail-fast in
the PR body. We concur — resolved. - ↩︎
_toml_scalardoesn't escape C0 controls beyond tab/CR/LF — raised by @fanhongy;
@tedswinyar deliberately kept the helper minimal (filesystem paths can't carry NUL/VT/FF;
tomllibround-trip verified current behavior). Our security reviewer independently found
the same (residual silent-parse-failure, not a breakout). We concur it's out of #404's
scope; noting only. - ↩︎ raw
server_name/env_keyinterpolation into TOML dotted keys andtmux.py
logging the full launch command (incl. env values + system prompt) at INFO — both raised
as pre-existing in the thread by @fanhongy / @tedswinyar, who offered them as follow-up PRs.
Our security + correctness reviewers independently confirmed both exist and are worth
fixing. We concur — pre-existing, track as follow-ups; not blocking #404.
Pre-existing (adjacent — independently confirmed, not blocking)
- [security/correctness] codex.py:278,296 — the
-ckey halves (server_name,
env_key) are still interpolated raw, unlike the values #404 just fixed and unlike the
siblingcodexConfigpath which validates every key against_CODEX_CONFIG_KEY_PATTERN
(^[A-Za-z0-9_.-]+$). A key with", space,=, or.yields malformed TOML (DoS /
silent config refusal) or silently re-nests the config path. #404 correctly closed the value
side; the key side is now the only raw-interpolation path left in this method. Suggested
follow-up: runserver_name/env_keythrough the same key pattern. (Already discussed in
the thread as a follow-up.) - [security] clients/tmux.py:287 —
logger.info(f"send_keys: … keys: {keys}")logs the
full launch command at INFO, which now reliably includes every MCPenvvalue and the whole
developer_instructionsprompt in cleartext → credential leak to logs. Follow-up: log
len(keys)/ redactenv.*/ gate full content behind DEBUG.
Stacked-from-#403 (carried by this branch, not #404's own diff)
- [tests] copilot provider wiring untested — #403 adds
resolve_mcp_server_configwiring
to copilot_cli.py, but there is notest/providers/test_copilot*.pyat all, so the copilot
rewrite is entirely unasserted. claude_code / cursor_cli / kimi_cli each have a resolve
assertion. Belongs to #403. - [security nit] mcp_resolution.py (persisted PATH lookup) —
persisted=Trueprefers
shutil.which(PATH) over the hijack-proof interpreter-sibling absolute path and writes the
result to a reused config; acao-mcp-serverplanted earlier on PATH could be resolved and
persisted. Documented upgrade-stability tradeoff; low severity (presupposes a writable PATH
entry). Thepersisted=Falseruntime path is strictly safer. Belongs to #403. - [correctness — REFUTED]
resolve_mcp_server_configinjecting emptycommand/args—
a suspected #403 regression (raised in a prior dashboard pass). Not present: the
if "command" not in config: return dict(config)guard (mcp_resolution.py) means http/sse
entries pass through untouched; no emptycommand/argsis injected. Refuted.
Tests
Solid for #404's own change. Three new tests
(test_mcp_server_command_field_is_toml_escaped, test_mcp_server_args_and_env_are_toml_escaped,
test_mcp_env_vars_non_string_entry_fails_fast) correctly assert both escaped-present and
raw-absent for command/args/env-dict, and the non-scalar fail-fast. developer_instructions
escaping is covered by the pre-existing test_build_command_escapes_quotes /
_escapes_newlines. Two net-new gaps: the env_vars list positive-escape path (Important)
and the weak command negative-assertion (nit). Markers/mocking correct; fits the fast unit run.
Verification
Verifier ran on host python 3.12.8 (deps already resolvable; no Docker/uv needed).
editable install initially pointed imports at the main checkout, not the PR worktree — caught
via __file__ + diff -q, first-pass numbers discarded, all results below re-run with
PYTHONPATH=$WT/src to force the PR-head codex.py.
- Baseline:
pytest test/providers/test_codex_provider_unit.py→ 112 passed, 0 failed. - ✓ VERIFIED command field now escaped — codex.py:288 emits
{prefix}.command={_toml_scalar(cfg['command'])};test_mcp_server_command_field_is_toml_escaped
passes. args/env/env_vars/developer_instructions all routed through_toml_scalar. - ✓ VERIFIED escaping prevents TOML string-breakout / stays single-line — fed
_toml_scalar
",\, tab, CR, newline, a combo, and a breakout payload"\nevil='pwn'; all 7/7
round-trip viatomllib.loads('x = '+out)back to the exact input with no literal newline. - ✓ VERIFIED non-scalar
env_varsfails fast —_toml_scalar({...})raises
TypeError: … values must be scalars …; the named test passes. - ✓ VERIFIED (differential vs main) the fix is real, not just a green test — crafted env
valuex"\nmalicious_key = "pwned: OLD raw path parsed keys['mcp_servers','malicious_key']
→ injection succeeded; NEW_toml_scalarpath parses only['mcp_servers']→ injection
fails and the value round-trips exactly. Proves #404 closes a real TOML-key-injection vector. - ⁇ Full end-to-end codex launch not exercised (no live codex session), but the changed code
path itself is directly executed and verified.
Verdict
Approve with nits — the escaping fix is correct, complete for the exploitable vectors, and
verified with a differential injection proof; the earlier "command unescaped" concern was a
force-push race and is resolved on the current head. Before merge, ideally: (1) add a CHANGELOG
entry, and (2) add an env_vars-list positive escaping test. The pre-existing raw-key
interpolation and INFO-level launch-command logging are real but out of scope — worth tracking
as the follow-up PRs the author already offered.
The Codex provider injects each MCP server's command, args, and env into the launch command as `-c mcp_servers.<name>.<field>="<value>"` TOML overrides, but interpolated the values without escaping. A value containing a double-quote or backslash produces a malformed TOML string (the quote closes the string early), and a literal newline would split the tmux send_keys command across lines. Reuse the existing _toml_scalar helper (already used for codexConfig overrides and covering backslash, quote, tab, CR, and newline) for the MCP command, each arg, each env value, and the env_vars list, and switch the inline developer_instructions escaping to it as well, so the file has a single TOML-string escaping path.
tedswinyar
force-pushed
the
fix/cao-codex-toml-escape
branch
from
e06465b to
df304cb
Compare
|
Rebased onto main now that #403 landed (thanks for the merge and the v2.3.0 release!). The diff has collapsed to codex.py + its tests as promised. CI on the new head needs a workflow approval when someone has a moment. Thanks Augusto for the re-review and approval. |
What happens
The Codex provider injects each MCP server's command, args, and env into the launch command as
-c mcp_servers.<name>.<field>="<value>"TOML overrides, but interpolated the values without escaping. A value containing a double-quote or backslash produces a malformed TOML string (the quote closes the string early), and a literal newline would split the tmuxsend_keyscommand across lines.This matters more after #403: the resolved
cao-mcp-servercommand becomes an absolute path, and env values are caller-supplied, so TOML-special characters can reach these overrides.The fix
Reuse the existing
_toml_scalarhelper (already used forcodexConfigoverrides; escapes backslash, quote, tab, CR, and newline) for the MCP command, each arg, each env value, and theenv_varslist, and switch the inlinedeveloper_instructionsescaping to it as well, so the file has a single TOML-string escaping path.A non-string
env_varsentry now raisesTypeErrorat launch-command build time (previously it was silently interpolated raw). This fail-fast is intentional — a malformed profile fails with a clear error instead of emitting a silently-broken override — and is covered by a dedicated test.History
Originally stacked on #403 per the maintainers' preference to keep the escaping fix separate; now that #403 is merged, this PR is rebased onto main and the diff is
providers/codex.py+test/providers/test_codex_provider_unit.pyonly.Testing
env_varsentry raisesTypeError(documented fail-fast).