-
-
Notifications
You must be signed in to change notification settings - Fork 4
feat(cli): Add --model flag and WARDEN_MODEL env var #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -98,9 +98,10 @@ | |||||||||||||||||||
| reporter.blank(); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Try to load config for custom skills | ||||||||||||||||||||
| // Try to load config for custom skills and defaults | ||||||||||||||||||||
| let repoPath: string | undefined; | ||||||||||||||||||||
| let skillsConfig: SkillDefinition[] | undefined; | ||||||||||||||||||||
| let defaultsModel: string | undefined; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| try { | ||||||||||||||||||||
| repoPath = getRepoRoot(cwd); | ||||||||||||||||||||
|
|
@@ -110,13 +111,16 @@ | |||||||||||||||||||
| if (existsSync(configPath)) { | ||||||||||||||||||||
| const config = loadWardenConfig(dirname(configPath)); | ||||||||||||||||||||
| skillsConfig = config.skills; | ||||||||||||||||||||
| defaultsModel = config.defaults?.model; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } catch { | ||||||||||||||||||||
| // Not in a git repo or no config - that's fine | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // 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']; | ||||||||||||||||||||
| const runnerOptions: SkillRunnerOptions = { apiKey, model, abortController }; | ||||||||||||||||||||
|
Check warning on line 123 in src/cli/main.ts
|
||||||||||||||||||||
|
Comment on lines
+122
to
+123
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 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.
Suggested change
warden: security-review |
||||||||||||||||||||
| const tasks: SkillTaskOptions[] = skillNames.map((skillName) => ({ | ||||||||||||||||||||
| name: skillName, | ||||||||||||||||||||
| failOn: options.failOn, | ||||||||||||||||||||
|
|
@@ -374,7 +378,7 @@ | |||||||||||||||||||
| reporter.success(`Loaded ${config.triggers.length} ${pluralize(config.triggers.length, 'trigger')}`); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Resolve triggers with defaults and match | ||||||||||||||||||||
| const resolvedTriggers = config.triggers.map((t) => resolveTrigger(t, config)); | ||||||||||||||||||||
| const resolvedTriggers = config.triggers.map((t) => resolveTrigger(t, config, options.model)); | ||||||||||||||||||||
| const matchedTriggers = resolvedTriggers.filter((t) => matchTrigger(t, context)); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Filter by skill if specified | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -57,9 +57,21 @@ | |||||||||||||||
| /** | ||||||||||||||||
| * Resolve a trigger's configuration by merging with defaults. | ||||||||||||||||
| * Trigger-specific values override defaults. | ||||||||||||||||
| * | ||||||||||||||||
| * Model precedence (highest to lowest): | ||||||||||||||||
| * 1. trigger.model (warden.toml trigger-level) | ||||||||||||||||
| * 2. defaults.model (warden.toml [defaults]) | ||||||||||||||||
| * 3. cliModel (--model flag) | ||||||||||||||||
| * 4. WARDEN_MODEL env var | ||||||||||||||||
| * 5. SDK default (not set here) | ||||||||||||||||
| */ | ||||||||||||||||
| export function resolveTrigger(trigger: Trigger, config: WardenConfig): ResolvedTrigger { | ||||||||||||||||
| export function resolveTrigger( | ||||||||||||||||
| trigger: Trigger, | ||||||||||||||||
| config: WardenConfig, | ||||||||||||||||
| cliModel?: string | ||||||||||||||||
| ): ResolvedTrigger { | ||||||||||||||||
| const defaults = config.defaults; | ||||||||||||||||
|
Check warning on line 73 in src/config/loader.ts
|
||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 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.
Suggested change
warden: security-review |
||||||||||||||||
| const envModel = process.env['WARDEN_MODEL']; | ||||||||||||||||
|
|
||||||||||||||||
| return { | ||||||||||||||||
| ...trigger, | ||||||||||||||||
|
|
@@ -73,6 +85,6 @@ | |||||||||||||||
| maxFindings: trigger.output?.maxFindings ?? defaults?.output?.maxFindings, | ||||||||||||||||
| labels: trigger.output?.labels ?? defaults?.output?.labels, | ||||||||||||||||
| }, | ||||||||||||||||
| model: trigger.model ?? defaults?.model, | ||||||||||||||||
| model: trigger.model ?? defaults?.model ?? cliModel ?? envModel, | ||||||||||||||||
|
Check warning on line 88 in src/config/loader.ts
|
||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 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.
Suggested change
warden: security-review |
||||||||||||||||
| }; | ||||||||||||||||
| } | ||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Additional Locations (1)
src/config/loader.ts#L87-L88