fix: replace hand-rolled cmdline escaping with JSON-encoded --agent-config#38
fix: replace hand-rolled cmdline escaping with JSON-encoded --agent-config#38yeelam-gordon wants to merge 23 commits into
Conversation
- Extract build_wt_commandline() from shell_manager into cmdline.rs - Fix quoting bug: embedded double-quotes in args were not escaped, allowing argument injection via CommandLineToArgvW parsing - Add 26 unit tests that round-trip through the real OS parser (CommandLineToArgvW) covering: quotes, backslashes, whitespace, empty args, shell metacharacters, NUL rejection, and realistic agent commands - Add Win32_UI_Shell feature to windows-sys for test-time CommandLineToArgvW access Future direction: replace hand-rolled commandline escaping with JSON-encoded structured args to eliminate the escaping problem by construction (serde_json is already a dependency). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace hand-rolled commandline escaping with a single JSON-encoded arg. C++ host serializes agent config into JSON, passes via --agent-config. Rust WTA deserializes with serde_json (backward compatible). Security: eliminates 6 hand-rolled escaping sites, reduces attack surface to one correctly-quoted argument boundary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR hardens WT↔WTA argument passing by replacing ad-hoc Windows CRT-style escaping with CommandLineToArgvW-compatible quoting, and by bundling agent-related launch configuration into a single JSON --agent-config argument to reduce injection surface.
Changes:
- Added a shared C++ quoting helper (
QuoteArgForCommandLine) and a C++ helper to emit a JSON--agent-configfragment for WTA launches. - Updated Terminal launch paths (TerminalPage + Settings Editor probe) to use correct quoting and JSON-based config passing.
- Added a Rust
cmdlinemodule withbuild_wt_commandlineand OS-roundtrip tests; updated WTA CLI parsing to accept and overlay--agent-config.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/wta/src/shell/shell_manager.rs | Uses the new Rust commandline builder when creating WT panes. |
| tools/wta/src/main.rs | Adds --agent-config parsing and overlays JSON fields onto legacy CLI args (including delegate subcommand support). |
| tools/wta/src/cmdline.rs | Introduces Windows commandline construction + CommandLineToArgvW round-trip tests (including JSON payload cases). |
| tools/wta/Cargo.toml | Enables windows-sys Shell APIs needed for CommandLineToArgvW tests. |
| src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp | Uses CommandLineToArgvW-compatible quoting when passing --agent to probe-models. |
| src/cascadia/TerminalApp/TerminalPage.cpp | Switches multiple WTA launch paths to BuildAgentConfigArg + correct argument quoting. |
| src/cascadia/inc/QuoteArgForCommandLine.h | Adds a reusable argument quoting helper and JSON --agent-config fragment builder. |
This comment has been minimized.
This comment has been minimized.
BuildAgentConfigArg uses manual RFC 8259 JSON construction, not JsonCpp. Updated comments to reflect the actual implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
- Add <cwchar> and <stdexcept> includes for self-contained header - Reject embedded NUL in QuoteArgForCommandLine (prevents silent truncation) - Add QuoteProgramPath() helper for argv[0] (simpler rules, rejects quotes) - Use QuoteProgramPath for wta executable path in delegate launch - Fix Rust docstring: 'JSON encoding' not 'JSON library' - Extract parse_agent_config() helper to eliminate overlay duplication Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Addresses all 5 Copilot review comments from round 2.
yeelam-gordon
force-pushed
the
dev/yeelam/cmdline-builder-tests
branch
from
a15387b to
ad520e5
Compare
| // Use correct CommandLineToArgvW quoting for the agent argument. | ||
| const std::wstring args = L"probe-models --agent " + | ||
| ::Microsoft::Terminal::CommandLine::QuoteArgForCommandLine(std::wstring_view{ agentCmdline }); |
There was a problem hiding this comment.
Fixed in 92e663d — QuoteArgForCommandLine returns std::optional. Probe path skips launch if quoting fails.
This comment has been minimized.
This comment has been minimized.
- Convert QuoteArgForCommandLine and QuoteProgramPath to return std::optional<std::wstring> instead of throwing on invalid input - Update all callers (TerminalPage, AIAgentsViewModel) to handle the optional gracefully with early-return/skip patterns - Fix shell_manager.rs comment about error propagation behavior - Remove <stdexcept> include (no longer needed) This prevents app-terminating exceptions from settings/user text containing embedded NUL, satisfying the non-throwing API requirement for UI code paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Make delegate prompt quoting failure a hard early-return (not silent) - Fix BuildAgentConfigArg doc: NUL is escaped by JSON, not rejected Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
- Use QuoteProgramPath(wtaPath) instead of raw path in agent pane creation (_AutoCreateHiddenAgentPane, _OpenOrReuseAgentPane) - Add diagnostic logging on all quoting failures for discoverability - Fixes potential argv[0] mis-parsing when wta is in a path with spaces Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Aligns with the convention used in other TerminalApp sources (e.g. AgentRegistry.h) and in AIAgentsViewModel.cpp. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
- Remove noexcept from QuoteArgForCommandLine and QuoteProgramPath; wrap bodies in try/catch to return nullopt on allocation failure instead of risking std::terminate. - Eliminate AgentConfigOverlay struct; parse_agent_config now returns AgentConfig directly since both structs had identical fields. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
- Add 'ffi', 'gpt', 'pwned' to expect.txt (standard Rust FFI, model names, and security test strings). - Add spaces after escape sequences in test strings (\tworld → \t world, \nline → \n line, \rline → \r line) to prevent the spell checker from seeing 'tworld', 'nline', 'rline' as words. Tests still pass — the quoting logic is character-agnostic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Use pattern destructuring instead of field-by-field access on the overlay variable. While Rust supports partial moves, destructuring makes the intent clearer and avoids potential confusion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@check-spelling-bot Report
|
| Dictionary | Entries | Covers | Uniquely |
|---|---|---|---|
| cspell:csharp/csharp.txt | 32 | 2 | 2 |
| cspell:aws/aws.txt | 232 | 2 | 2 |
| cspell:fonts/fonts.txt | 536 | 1 | 1 |
Consider adding to the extra_dictionaries array (in the .github/actions/spelling/config.json file):
"cspell:csharp/csharp.txt",
"cspell:aws/aws.txt",
"cspell:fonts/fonts.txt",
To stop checking additional dictionaries, put (in the .github/actions/spelling/config.json file):
"check_extra_dictionaries": []Forbidden patterns 🙅 (10)
In order to address this, you could change the content to not match the forbidden patterns (comments before forbidden patterns may help explain why they're forbidden), add patterns for acceptable instances, or adjust the forbidden patterns themselves.
These forbidden patterns matched content:
Should be nonexistent
\b[Nn]o[nt][- ]existent\b
Should be preexisting
[Pp]re[- ]existing
Should be ; otherwise or . Otherwise
https://study.com/learn/lesson/otherwise-in-a-sentence.html
, [Oo]therwise\b
Should probably be Otherwise,
(?<=\. )Otherwise\s
Complete sentences in parentheticals should not have a space before the period.
\s\.\)(?!.*\}\})
Should be set up (setup is a noun / set up is a verb)
\b[Ss]etup(?= (?:an?|the|to)\b)
Should be reentrancy
[Rr]e[- ]entrancy
Should be reentrant
[Rr]e[- ]entrant
Should be whether or not ...
(?i)\b(?:whe|ra)ther(?:\s\w+)+ or not\.
Should be WinGet
\bWinget\b
Pattern suggestions ✂️ (2)
You could add these patterns to .github/actions/spelling/patterns/d47c725f3fb35fa85bf78220f272b6b047c172af.txt:
# Automatically suggested patterns
# hit-count: 1 file-count: 1
# python
\b(?i)py(?!gment|gmy|lon|ramid|ro|th)(?=[a-z]{2,})
# hit-count: 1 file-count: 1
# node packages
(["'])@[^/'" ]+/[^/'" ]+\g{-1}
Alternatively, if a pattern suggestion doesn't make sense for this project, add a # to the beginning of the line in the candidates file with the pattern to stop suggesting it.
Errors, Warnings, and Notices ❌ (8)
See the 📂 files view, the 📜action log, 👼 SARIF report, or 📝 job summary for details.
See ❌ Event descriptions for more information.
✏️ Contributor please read this
By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
- ... misspelled, then please correct them instead of using the command.
- ... names, please add them to
.github/actions/spelling/allow/names.txt. - ... APIs, you can add them to a file in
.github/actions/spelling/allow/. - ... just things you're using, please add them to an appropriate file in
.github/actions/spelling/expect/. - ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in
.github/actions/spelling/patterns/.
See the README.md in each directory for more information.
🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉
If the flagged items are 🤯 false positives
If items relate to a ...
-
binary file (or some other file you wouldn't want to check at all).
Please add a file path to the
excludes.txtfile matching the containing file.File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
^refers to the file's path from the root of the repository, so^README\.md$would exclude README.md (on whichever branch you're using). -
well-formed pattern.
If you can write a pattern that would match it,
try adding it to thepatterns.txtfile.Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.
Note that patterns can't match multiline strings.
Problem
All WT-to-WTA argument passing used hand-rolled escaping (MSVCRT convention), which is not compatible with CommandLineToArgvW. This creates an injection surface for values containing quotes or backslashes.
Solution
Changes
Testing