feat(cli): Add --model flag and WARDEN_MODEL env var#16
Conversation
Allow users to override the model used for analysis via CLI flag or environment variable, with clear precedence: 1. --model CLI flag (highest) 2. trigger.model (warden.toml) 3. defaults.model (warden.toml) 4. WARDEN_MODEL env var 5. SDK default (lowest) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
src/cli/main.ts
Outdated
| // Build skill tasks | ||
| const runnerOptions: SkillRunnerOptions = { apiKey, abortController }; | ||
| // Model precedence: CLI flag > WARDEN_MODEL env var > SDK default | ||
| const model = options.model ?? process.env['WARDEN_MODEL']; |
There was a problem hiding this comment.
🟠 Unvalidated environment variable used for model selection
The code reads from process.env['WARDEN_MODEL'] without validation and passes it directly to the SDK. If this model parameter is used in API calls, command construction, or file operations, an attacker with control over environment variables could potentially inject malicious values. While the impact depends on how the SDK uses the 'model' parameter, environment variables should generally be validated before use, especially when they influence external API calls or system behavior.
Suggested fix: Validate the model parameter against an allowlist of known valid model names, or at minimum sanitize the input to ensure it matches expected patterns (e.g., alphanumeric with hyphens).
| const model = options.model ?? process.env['WARDEN_MODEL']; | |
| let model = options.model ?? process.env['WARDEN_MODEL']; | |
| // Validate model parameter if provided | |
| if (model && !/^[a-zA-Z0-9_\-\.]+$/.test(model)) { | |
| reporter.error('Invalid model name format'); | |
| throw new Error('Model name contains invalid characters'); | |
| } |
warden: security-review
src/config/loader.ts
Outdated
| @@ -73,6 +85,6 @@ export function resolveTrigger(trigger: Trigger, config: WardenConfig): Resolved | |||
| maxFindings: trigger.output?.maxFindings ?? defaults?.output?.maxFindings, | |||
| labels: trigger.output?.labels ?? defaults?.output?.labels, | |||
| }, | |||
| model: trigger.model ?? defaults?.model, | |||
| model: cliModel ?? trigger.model ?? defaults?.model ?? envModel, | |||
There was a problem hiding this comment.
🟠 Unvalidated environment variable used in model configuration
The code reads from process.env['WARDEN_MODEL'] and uses it directly without validation or sanitization. While this is used for model selection (likely a string identifier), there's no validation that the value is from an expected set of models. If this model identifier is later used in command execution, path construction, or direct interpolation into commands/queries, it could lead to injection vulnerabilities. Additionally, an attacker with environment variable control could potentially cause unexpected behavior by providing malformed or malicious model identifiers.
Suggested fix: Add validation for the environment variable value against a whitelist of allowed model identifiers, or at minimum, sanitize the input to ensure it contains only expected characters (e.g., alphanumeric and hyphens). Consider using a validation schema similar to how WardenConfigSchema validates other configuration.
| let envModel = process.env['WARDEN_MODEL']; | |
| // Validate environment variable contains only safe characters | |
| if (envModel && !/^[a-zA-Z0-9._-]+$/.test(envModel)) { | |
| throw new ConfigLoadError('Invalid WARDEN_MODEL environment variable: contains unsafe characters'); | |
| } |
warden: security-review
src/config/loader.ts
Outdated
| labels: trigger.output?.labels ?? defaults?.output?.labels, | ||
| }, | ||
| model: trigger.model ?? defaults?.model, | ||
| model: cliModel ?? trigger.model ?? defaults?.model ?? envModel, |
There was a problem hiding this comment.
🟠 Potential Environment Variable Injection via WARDEN_MODEL
The code reads the WARDEN_MODEL environment variable and uses it directly to set the model configuration without validation or sanitization. If an attacker can control environment variables (e.g., in CI/CD pipelines, containerized environments, or shared hosting), they could potentially inject malicious model names or cause unexpected behavior. The precedence order places cliModel highest, but envModel is used as a fallback, which could override default configurations unexpectedly.
Suggested fix: Add validation for the WARDEN_MODEL environment variable to ensure it matches expected model names. Consider maintaining an allowlist of valid model identifiers and rejecting or warning on unexpected values.
| model: cliModel ?? trigger.model ?? defaults?.model ?? envModel, | |
| let envModel = process.env['WARDEN_MODEL']; | |
| // Validate environment variable input | |
| const ALLOWED_MODELS = ['claude-3-opus', 'claude-3-sonnet', 'claude-3-haiku', 'gpt-4', 'gpt-3.5-turbo']; | |
| if (envModel && !ALLOWED_MODELS.includes(envModel)) { | |
| console.warn(`Warning: Unknown model '${envModel}' specified in WARDEN_MODEL. Ignoring.`); | |
| envModel = undefined; | |
| } |
warden: security-review
Model precedence is now: trigger.model > defaults.model > --model flag > WARDEN_MODEL env var This ensures consistent behavior between config mode and file/git-ref modes, where config settings take priority as the project-level defaults. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| const model = defaultsModel ?? options.model ?? process.env['WARDEN_MODEL']; | ||
| const runnerOptions: SkillRunnerOptions = { apiKey, model, abortController }; |
There was a problem hiding this comment.
🟠 Environment Variable Injection in Model Selection
The code reads from process.env['WARDEN_MODEL'] without validation and passes it directly to the model configuration. If an attacker can control environment variables (e.g., in a CI/CD pipeline or shared hosting environment), they could potentially inject malicious model names or attempt to manipulate the behavior of the application. While the actual exploitability depends on how the 'model' parameter is validated downstream, it's a best practice to validate or sanitize environment variable inputs, especially when they control application behavior.
Suggested fix: Add validation to ensure the model name matches expected patterns or is from an allowlist of valid model names before using it.
| const model = defaultsModel ?? options.model ?? process.env['WARDEN_MODEL']; | |
| const runnerOptions: SkillRunnerOptions = { apiKey, model, abortController }; | |
| const envModel = process.env['WARDEN_MODEL']; | |
| // Validate model name if from environment | |
| if (envModel && !/^[a-z0-9-]+$/.test(envModel)) { | |
| reporter.error('Invalid model name in WARDEN_MODEL environment variable'); | |
| return 1; | |
| } | |
| const model = defaultsModel ?? options.model ?? envModel; |
warden: security-review
| config: WardenConfig, | ||
| cliModel?: string | ||
| ): ResolvedTrigger { | ||
| const defaults = config.defaults; |
There was a problem hiding this comment.
🟠 Environment variable used directly without validation
The code reads process.env['WARDEN_MODEL'] and uses it directly in the model precedence chain without validation. If an attacker can control environment variables (e.g., in CI/CD pipelines, containerized environments, or through process spawning), they could potentially inject malicious model names or values that might be interpreted by downstream code in unintended ways. While the impact depends on how the model value is used later, accepting arbitrary strings from environment variables without validation is a security risk.
Suggested fix: Add validation to ensure the environment variable value matches expected patterns (e.g., allowed model names, format validation). Consider using a whitelist of allowed model names or at minimum, validate that it matches expected patterns.
| const defaults = config.defaults; | |
| const envModelRaw = process.env['WARDEN_MODEL']; | |
| // Validate env var: alphanumeric, hyphens, dots, max 100 chars | |
| const envModel = envModelRaw && /^[a-zA-Z0-9._-]{1,100}$/.test(envModelRaw) | |
| ? envModelRaw : undefined; |
warden: security-review
| labels: trigger.output?.labels ?? defaults?.output?.labels, | ||
| }, | ||
| model: trigger.model ?? defaults?.model, | ||
| model: trigger.model ?? defaults?.model ?? cliModel ?? envModel, |
There was a problem hiding this comment.
🟠 Unvalidated environment variable used in model configuration
The WARDEN_MODEL environment variable is read from process.env and used directly without validation or sanitization. If this model value is later passed to an external API, command execution, or used in security-sensitive contexts, an attacker with control over environment variables could potentially inject malicious values. The impact depends on how the model value is used downstream - if it's passed to shell commands, file paths, or external services without validation, this could lead to injection vulnerabilities.
Suggested fix: Validate the environment variable against an allowlist of acceptable model values before using it. If the model names are known, use a whitelist approach. Otherwise, validate the format and reject suspicious characters.
| model: trigger.model ?? defaults?.model ?? cliModel ?? envModel, | |
| let envModel = process.env['WARDEN_MODEL']; | |
| // Validate model name to prevent injection | |
| if (envModel && !/^[a-zA-Z0-9._-]+$/.test(envModel)) { | |
| console.warn(`Invalid WARDEN_MODEL format: ${envModel}. Ignoring.`); | |
| envModel = undefined; | |
| } |
warden: security-review
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| // Build skill tasks | ||
| const runnerOptions: SkillRunnerOptions = { apiKey, abortController }; | ||
| // Model precedence: defaults.model > CLI flag > WARDEN_MODEL env var > SDK default | ||
| const model = defaultsModel ?? options.model ?? process.env['WARDEN_MODEL']; |
There was a problem hiding this comment.
Model precedence order is inverted from stated intent
High Severity
The model precedence implementation contradicts the PR description. The PR states "CLI flag > trigger config > defaults config > env var" but the code implements the opposite: defaultsModel ?? options.model ?? process.env['WARDEN_MODEL'] gives config higher priority than the CLI flag. When a user passes --model claude-opus-4-5-20250514, they expect it to override config settings, but currently config values win.


--model/-mCLI flag to override the model used for analysisWARDEN_MODELenvironment variable support as a fallback