add Grok Build as a detected local agent CLI - #150
modelpath-dev wants to merge 3 commits into
Conversation
PerishCode
left a comment
There was a problem hiding this comment.
The Grok CLI contract and streaming parser are otherwise consistent with the published headless interface, but the new argv prompt delivery creates a Windows command-injection path in both the Next app and standalone CLI. This needs to be resolved before the adapter is safe to ship.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| bin: "grok", | ||
| envOverride: "GROK_BIN", | ||
| vendor: "xAI", | ||
| protocol: "argv", |
There was a problem hiding this comment.
BLOCKING: Do not route the user-controlled Grok prompt through the generic argv protocol while Windows spawning uses a shell. This new protocol: "argv" makes invoke.ts append the complete prompt to argv; on Windows that function then calls spawn(..., { shell: true }). Node constructs a shell command from those arguments, so prompt content containing shell metacharacters can be interpreted by cmd.exe instead of remaining a literal -p value. The standalone CLI has the same combination in cli/src/agents-detect.ts and cli/src/agents-invoke.ts. This is remotely reachable from the conversion/draft input and can execute commands with the server user's privileges. Add a Grok-specific safe delivery path, such as writing the prompt to a securely created temporary file and invoking grok --prompt-file <path> with guaranteed cleanup on close/error/abort, or replace the Windows shell launch with a launcher that preserves an argument boundary for npm shims. Apply the same fix to both implementations and add a Windows-oriented regression test using metacharacters such as &, |, and %...% to prove the prompt is passed as data.
There was a problem hiding this comment.
Good catch, fixed in 7212c48. Grok now gets its own prompt-file protocol: the prompt goes into a private temp file (mkdtemp + 0600) and grok is launched with --prompt-file <path>, so nothing from the prompt ever lands on the cmd.exe line. The file gets removed on close, error, and abort. Same change in next/src/lib/agents/invoke.ts and cli/src/agents-invoke.ts, and both have a win32 test with &, |, %PATH%, ^ and <> in the prompt that checks the joined command line doesn't contain any of it and the file content comes through byte for byte.
Grok Build is a headless `grok -p` runtime, not a project build step. Detect it on PATH and invoke with the documented non-interactive flags so HTML Anything can reuse an existing grok login without a live session in CI.
Grok was the one agent taking a user prompt on argv while Windows spawns through cmd.exe, so prompt text could be parsed by the shell. Write it to a private temp file instead and hand grok --prompt-file, removing the file on close, error, or abort. Same change in the Next app and the CLI.
1261aec to
7212c48
Compare
|
Rebased on main (conflict was just the detect test from #129) and moved the grok prompt off argv, details in the thread above. Checked |
PerishCode
left a comment
There was a problem hiding this comment.
The prompt-file change closes the reported prompt injection path, but the new Grok adapter still puts another request-controlled value on the Windows shell command line. The model argument needs the same host-boundary treatment before this is safe to merge.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| "--output-format", | ||
| "streaming-json", | ||
| "--always-approve", | ||
| ...(model ? ["--model", model] : []), |
There was a problem hiding this comment.
BLOCKING: Keep the request-controlled model value off the Windows shell command line too. The prompt now uses a file, but both /api/convert and /api/draft pass the JSON body’s unvalidated model directly into invokeAgent; this changed line then appends it to Grok’s argv, while invoke.ts still spawns with shell: true on Windows. A request such as a model containing & or | can therefore still be interpreted by cmd.exe and execute with the server user’s privileges. The standalone CLI has the equivalent new interpolation in cli/src/agents-invoke.ts, although its local command-line input is lower risk. Validate the model against the selected agent’s declared model IDs (or enforce a strict model-ID character grammar) before argv construction, or change Windows process launching so every argument retains a real argument boundary; apply the safe construction to both implementations and extend the Windows regression tests to supply metacharacters through model, not only through prompt.
There was a problem hiding this comment.
Fixed in a3fbc60. buildArgv now refuses any model that isn't a plain model id (letters, digits, . _ / : -, no leading -, 128 chars max) before argv is built, in both next/src/lib/agents/argv.ts and cli/src/agents-invoke.ts. It covers every agent, since they all append --model the same way. The Windows tests now push &, |, %, ^, >, quotes, spaces and a --flag through model for grok and claude and check nothing spawns, and another test makes sure every model in the picker still passes. Also checked against a running dev server: /api/convert with model: "grok-build & calc.exe" comes back invalid model id without spawning.
|
Hi @modelpath-dev — thanks for rebasing and for moving the Grok prompt onto the prompt-file path; that directly addresses the first Windows shell-boundary concern. @PerishCode has left one remaining blocking note on this head about keeping the request-provided model value out of the Windows shell command line. Once that is addressed, the security review can continue. 💡 To drive this PR to merge hands-free, paste this to your AI coding agent (Claude Code / Codex / opencode / Cursor …): |
PerishCode
left a comment
There was a problem hiding this comment.
@modelpath-dev I reviewed the complete Grok Build integration and the follow-up security fixes on this head. The adapter now keeps prompt content out of the Windows shell command line through a private prompt file, validates model IDs before argv construction in both implementations, cleans temporary files across the relevant lifecycle paths, and includes focused coverage for detection, parsing, prompt delivery, cleanup, and rejected shell metacharacters. The surrounding UI, localization, store typing, CLI, and documentation changes are consistent with the new protocol. Nice work iterating carefully on the host-boundary concerns and carrying the fix through both app surfaces.
Local execution was unavailable in this reviewer image because Node and pnpm are not installed; source inspection and git diff --check completed successfully, while GitHub currently reports no checks for the branch.
Summary
grokbinary on PATH (andGROK_BIN) as Grok Build.grok --no-auto-update --output-format streaming-json --always-approve -p <prompt>. The prompt goes on argv, not stdin.streaming-jsontext/endevents and the finaljsonobject, so CI can verify the contract without a live Grok session.Fixes #136
Test plan
pnpm --filter @html-anything/next exec vitest run src/lib/agents/__tests__/argv.test.ts src/lib/agents/__tests__/detect.test.ts(18 passed)pnpm --filter @html-anything/cli exec vitest run src/__tests__/agents-detect.test.ts src/__tests__/agents-invoke.test.ts(48 passed)