fix: block unvalidated subcommand arguments in execute_atmos_command AI tool (CWE-88)#2275
fix: block unvalidated subcommand arguments in execute_atmos_command AI tool (CWE-88)#2275
Conversation
…AI tool (CWE-88) Agent-Logs-Url: https://github.com/cloudposse/atmos/sessions/2cc53e4b-f449-4919-bbdb-21ae7d47670e Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
… execute_atmos_command Agent-Logs-Url: https://github.com/cloudposse/atmos/sessions/1d206f86-263e-45b7-a67c-16e65c83ed50 Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
… in execute_command Agent-Logs-Url: https://github.com/cloudposse/atmos/sessions/f1fc3701-122a-467f-9245-42ced8eec31a Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
nitrocode
added
patch
Dependency Review✅ No vulnerabilities or license issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2275 +/- ##
==========================================
+ Coverage 77.16% 77.21% +0.04%
==========================================
Files 1034 1034
Lines 97542 97632 +90
==========================================
+ Hits 75271 75384 +113
+ Misses 18064 18043 -21
+ Partials 4207 4205 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR introduces security controls to prevent the AI tool from executing destructive Terraform commands without explicit user confirmation. It adds destructive command detection, permission mode gating, a sentinel error, comprehensive test coverage, and documentation of the security fix. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Tool as ExecuteAtmosCommandTool
participant Detector as isDestructiveAtmosCommand
participant Executor as Atmos/Terraform
Client->>Tool: Execute(command)
Tool->>Detector: Check if destructive
Detector-->>Tool: isDestructive (bool)
alt Destructive Command
Tool->>Tool: Check permissionMode
alt permissionMode == ModePrompt
Tool->>Executor: Execute subprocess
Executor-->>Tool: Result
Tool-->>Client: Result
else Other Mode
Tool-->>Client: ErrAICommandDestructive
end
else Safe/Read-only Command
Tool->>Executor: Execute subprocess
Executor-->>Tool: Result
Tool-->>Client: Result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/fixes/2026-04-01-ai-tool-unvalidated-subcommand-arguments.md`:
- Around line 22-26: The fenced code block containing the Terraform/shell
commands needs a language tag to satisfy MD040; update the opening fence from
``` to ```shell (or ```text) so the block is annotated (the block showing
"terraform apply vpc -s prod -var-file=/etc/passwd" / "terraform destroy ..." /
"terraform state rm ...").
In `@pkg/ai/tools/atmos/execute_command_test.go`:
- Around line 232-245: The tests (e.g.,
TestExecuteAtmosCommandTool_DestructiveAllowedInPromptMode and the related tests
around lines 249-289) use the platform-specific "echo" binary which fails on
Windows; replace that with the existing os.Executable()+TestMain test helper or
a fake Command runner so the subprocess is a Go-built test helper executable (or
an injected runner) instead of relying on shell-builtins. Update
NewExecuteAtmosCommandToolWithPermission test setup to set tool.binaryPath to
the helper executable (os.Executable() path) and pass arguments that the
TestMain helper recognizes for the success path, or refactor the tool to accept
an injected Runner interface and use a fake runner in these tests.
In `@pkg/ai/tools/atmos/execute_command.go`:
- Around line 75-81: The comment text incorrectly says safe read-only commands
are "always allowed"; update the description returned by the function that
builds the help/description (the string literal in execute_command.go) to say
these commands are "not blocked by this validator" (or similar) and clarify that
RequiresPermission() still returns true and the global permission checker
(ModePrompt/ModeDeny) will still prompt or deny as appropriate; reference
RequiresPermission(), ModePrompt and ModeDeny in the updated sentence so readers
know the validator only skips blocking them, not bypassing global permission
checks.
- Around line 54-65: The default constructor NewExecuteAtmosCommandTool
currently hard-codes permissionMode to permission.ModePrompt; thread the real
permission into that path by changing NewExecuteAtmosCommandTool to accept a
permission.Mode (or have it call NewExecuteAtmosCommandToolWithPermission) and
set ExecuteAtmosCommandTool.permissionMode from the passed mode, then update the
registration in setup.go to pass the actual runtime permission mode (the
global/checker-provided mode) when constructing the tool so ModeAllow/ModeYOLO
cases are respected; also update NewExecuteAtmosCommandToolWithPermission to
delegate to the new signature to avoid duplication.
- Around line 96-123: isDestructiveAtmosCommand assumes subcommands live at
fixed positions (args[1], args[2]) which breaks when flags precede the
subcommand (e.g., terraform -s prod apply ...); update isDestructiveAtmosCommand
to locate the real subcommand by first confirming args[0] == "terraform" then
skipping any leading flags (tokens that start with "-") to find the first
non-flag token as subCmd, and when handling nested verbs like "state" or
"workspace" similarly advance past any flags to find the nested subcommand (use
the existing destructiveTerraformSubcmds, destructiveTerraformStateSubcmds,
destructiveTerraformWorkspaceSubcmds lookups); add tests for cases like
"terraform -s prod apply vpc" and "terraform --stack prod state rm module.vpc"
to prevent regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 21dd0dba-af53-4c27-8dcc-dd4859b8ebbb
📒 Files selected for processing (5)
docs/fixes/2026-04-01-ai-tool-unvalidated-subcommand-arguments.mderrors/errors.gopkg/ai/tools/atmos/execute_command.gopkg/ai/tools/atmos/execute_command_test.gopkg/ai/tools/atmos/testmain_test.go
| ``` | ||
| terraform apply vpc -s prod -var-file=/etc/passwd | ||
| terraform destroy vpc -s prod --auto-approve | ||
| terraform state rm module.vpc | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to this fenced example.
Line 22 currently trips MD040. text or shell is enough here.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/fixes/2026-04-01-ai-tool-unvalidated-subcommand-arguments.md` around
lines 22 - 26, The fenced code block containing the Terraform/shell commands
needs a language tag to satisfy MD040; update the opening fence from ``` to
```shell (or ```text) so the block is annotated (the block showing "terraform
apply vpc -s prod -var-file=/etc/passwd" / "terraform destroy ..." / "terraform
state rm ...").
| func TestExecuteAtmosCommandTool_DestructiveAllowedInPromptMode(t *testing.T) { | ||
| config := &schema.AtmosConfiguration{BasePath: t.TempDir()} | ||
| tool := NewExecuteAtmosCommandToolWithPermission(config, permission.ModePrompt) | ||
| // Use echo consistent with the existing ValidCommand test in this file. | ||
| tool.binaryPath = "echo" | ||
|
|
||
| result, err := tool.Execute(context.Background(), map[string]interface{}{ | ||
| "command": "terraform apply vpc -s prod", | ||
| }) | ||
|
|
||
| require.NoError(t, err) | ||
| // echo always succeeds; we just verify the command was NOT blocked by the validator. | ||
| assert.True(t, result.Success, "destructive command should not be pre-blocked in ModePrompt") | ||
| } |
There was a problem hiding this comment.
echo keeps these new tests non-portable.
exec.Command("echo", ...) works on Unix-like systems but not on Windows, where echo is a shell builtin rather than an executable. Reuse the os.Executable()+TestMain helper for the success path too, or inject a fake runner, so this package stays green on all supported platforms.
As per coding guidelines, "Code must be Linux/macOS/Windows compatible. Use SDKs over binaries. ... Never use bash/shell commands in tests—use Go stdlib (os, filepath, io) for file operations." and "Never use platform-specific binaries in tests (e.g., false, true, sh on Unix). ... Use Go-native test helpers: subprocess via os.Executable() plus TestMain."
Also applies to: 249-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ai/tools/atmos/execute_command_test.go` around lines 232 - 245, The tests
(e.g., TestExecuteAtmosCommandTool_DestructiveAllowedInPromptMode and the
related tests around lines 249-289) use the platform-specific "echo" binary
which fails on Windows; replace that with the existing os.Executable()+TestMain
test helper or a fake Command runner so the subprocess is a Go-built test helper
executable (or an injected runner) instead of relying on shell-builtins. Update
NewExecuteAtmosCommandToolWithPermission test setup to set tool.binaryPath to
the helper executable (os.Executable() path) and pass arguments that the
TestMain helper recognizes for the success path, or refactor the tool to accept
an injected Runner interface and use a fake runner in these tests.
| return &ExecuteAtmosCommandTool{ | ||
| atmosConfig: atmosConfig, | ||
| binaryPath: binary, | ||
| atmosConfig: atmosConfig, | ||
| binaryPath: binary, | ||
| permissionMode: permission.ModePrompt, // default to safest mode | ||
| } | ||
| } | ||
|
|
||
| // NewExecuteAtmosCommandToolWithPermission creates a new Atmos command execution tool with explicit permission mode. | ||
| func NewExecuteAtmosCommandToolWithPermission(atmosConfig *schema.AtmosConfiguration, mode permission.Mode) *ExecuteAtmosCommandTool { | ||
| t := NewExecuteAtmosCommandTool(atmosConfig) | ||
| t.permissionMode = mode | ||
| return t |
There was a problem hiding this comment.
Thread the real permission mode into the default constructor path.
pkg/ai/tools/atmos/setup.go still registers this tool via NewExecuteAtmosCommandTool(atmosConfig), which hard-codes permissionMode to ModePrompt. In ModeAllow and ModeYOLO, the global checker auto-allows the tool, so destructive commands still skip this new block and execute.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ai/tools/atmos/execute_command.go` around lines 54 - 65, The default
constructor NewExecuteAtmosCommandTool currently hard-codes permissionMode to
permission.ModePrompt; thread the real permission into that path by changing
NewExecuteAtmosCommandTool to accept a permission.Mode (or have it call
NewExecuteAtmosCommandToolWithPermission) and set
ExecuteAtmosCommandTool.permissionMode from the passed mode, then update the
registration in setup.go to pass the actual runtime permission mode (the
global/checker-provided mode) when constructing the tool so ModeAllow/ModeYOLO
cases are respected; also update NewExecuteAtmosCommandToolWithPermission to
delegate to the new signature to avoid duplication.
| return "LAST RESORT: Execute an Atmos CLI command as a subprocess. " + | ||
| "Only use this for commands that do NOT have a dedicated tool. " + | ||
| "Do NOT use this for: listing stacks (use atmos_list_stacks), describing components (use atmos_describe_component), " + | ||
| "describing affected (use atmos_describe_affected), or validating stacks (use atmos_validate_stacks). " + | ||
| "Safe read-only commands (terraform plan, show, output, validate, state list, workspace list, describe, list) are always allowed. " + | ||
| "State-modifying operations (terraform apply, destroy, import, force-unlock, state rm/mv/push, workspace new/delete) " + | ||
| "require ModePrompt for user confirmation; all other modes block them at the tool layer." |
There was a problem hiding this comment.
The description overstates what “always allowed” means.
RequiresPermission() still returns true, and the global checker still prompts in ModePrompt or denies in ModeDeny. Calling safe commands “always allowed” will mislead the model about the actual execution flow; I’d scope this to “not blocked by this validator” instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ai/tools/atmos/execute_command.go` around lines 75 - 81, The comment text
incorrectly says safe read-only commands are "always allowed"; update the
description returned by the function that builds the help/description (the
string literal in execute_command.go) to say these commands are "not blocked by
this validator" (or similar) and clarify that RequiresPermission() still returns
true and the global permission checker (ModePrompt/ModeDeny) will still prompt
or deny as appropriate; reference RequiresPermission(), ModePrompt and ModeDeny
in the updated sentence so readers know the validator only skips blocking them,
not bypassing global permission checks.
| // isDestructiveAtmosCommand reports whether args represent a state-modifying Terraform operation. | ||
| func isDestructiveAtmosCommand(args []string) bool { | ||
| if len(args) < 2 { | ||
| return false | ||
| } | ||
|
|
||
| if strings.ToLower(args[0]) != "terraform" { | ||
| return false | ||
| } | ||
|
|
||
| subCmd := strings.ToLower(args[1]) | ||
|
|
||
| if destructiveTerraformSubcmds[subCmd] { | ||
| return true | ||
| } | ||
|
|
||
| if subCmd == "state" && len(args) >= 3 { | ||
| stateSubCmd := strings.ToLower(args[2]) | ||
| return destructiveTerraformStateSubcmds[stateSubCmd] | ||
| } | ||
|
|
||
| if subCmd == "workspace" && len(args) >= 3 { | ||
| wsSubCmd := strings.ToLower(args[2]) | ||
| return destructiveTerraformWorkspaceSubcmds[wsSubCmd] | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does spf13/cobra accept persistent flags before subcommands, e.g. can terraform -s prod apply vpcorterraform --stack prod state rm module.vpcparse withapply/state as the subcommand after the parent flag?
💡 Result:
Yes, spf13/cobra accepts persistent flags before subcommands. Cobra's parsing algorithm first strips flags from the argument list using the command tree's flags (including persistent flags from parents), then matches the remaining positional arguments to subcommands. This allows persistent flags anywhere before/after the subcommand name, as long as they are valid for the final command path. For example, terraform -s prod apply vpc parses if -s is a persistent flag on root (available to apply), with apply and vpc as args to apply. Similarly for --stack prod state rm module.vpc, assuming --stack is persistent. Terraform uses Cobra and supports global options like -chdir before subcommands, confirming this behavior (e.g., terraform -chdir=foo plan). Issue #10 in Cobra repo fixed early flag parsing issues, enabling this POSIX-style flexibility.
Citations:
- 1: Parse commands and flags irregardless of order spf13/cobra#10
- 2: https://developer.hashicorp.com/terraform/cli/commands
- 3: https://github.com/spf13/cobra
- 4: https://cobra.dev/docs/how-to-guides/working-with-flags/
- 5: https://cobra.dev/docs/how-to-guides/working-with-commands
- 6: https://chromium.googlesource.com/external/github.com/spf13/cobra/%2B/a4ab3fa09e3d6c4ac3fb7deece97a910957305ab/command.go
- 7: https://github.com/kubernetes/kubernetes/blob/master/vendor/github.com/spf13/cobra/command.go
🏁 Script executed:
# Find and examine the execute_command.go file
fd execute_command.go -type fRepository: cloudposse/atmos
Length of output: 230
🏁 Script executed:
# Check how isDestructiveAtmosCommand is called
rg "isDestructiveAtmosCommand" -A 3 -B 3Repository: cloudposse/atmos
Length of output: 4948
🏁 Script executed:
# Look at cmd/terraform.go to see what persistent flags are defined
head -200 cmd/terraform.goRepository: cloudposse/atmos
Length of output: 135
🏁 Script executed:
# Read the execute_command.go file
cat -n pkg/ai/tools/atmos/execute_command.goRepository: cloudposse/atmos
Length of output: 8075
🏁 Script executed:
# Check the test file to understand how args are passed
cat -n pkg/ai/tools/atmos/execute_command_test.go | head -150Repository: cloudposse/atmos
Length of output: 5757
🏁 Script executed:
# Find terraform command definition
fd terraform.goRepository: cloudposse/atmos
Length of output: 139
🏁 Script executed:
# Check more test cases to see if there are cases with flags before subcommands
cat -n pkg/ai/tools/atmos/execute_command_test.go | tail -100Repository: cloudposse/atmos
Length of output: 4199
🏁 Script executed:
# Look for how command strings are constructed - check if there's any parsing/normalization
rg "strings.Fields" pkg/ai/tools/atmos/execute_command.go -B 5 -A 5Repository: cloudposse/atmos
Length of output: 297
🏁 Script executed:
# Check if there are test cases that would expose the issue
rg "apply\|state rm\|workspace" pkg/ai/tools/atmos/execute_command_test.go -B 2 -A 2 | head -80Repository: cloudposse/atmos
Length of output: 42
🏁 Script executed:
# Check documentation or examples for terraform command usage
rg "terraform.*apply\|terraform.*destroy\|terraform.*state" docs/ -l | head -5Repository: cloudposse/atmos
Length of output: 42
🏁 Script executed:
# Look at the fix document to see if this was addressed
cat docs/fixes/2026-04-01-ai-tool-unvalidated-subcommand-arguments.mdRepository: cloudposse/atmos
Length of output: 7426
Handle flags preceding subcommands in classification.
The current implementation assumes the destructive subcommand is always at args[1] or args[2], but strings.Fields() does simple whitespace splitting with no flag parsing. Commands like terraform -s prod apply vpc result in args=["terraform", "-s", "prod", "apply", "vpc"], causing isDestructiveAtmosCommand to check args[1]="-s" instead of the actual subcommand at args[3]. This allows destructive operations to bypass the blocker.
Fix by either:
- Stripping leading flags before checking subcommand position, or
- Reusing the actual CLI parser for the
terraformsubcommand shape instead of positional assumptions.
Test cases should cover terraform -s prod apply vpc, terraform --stack prod state rm module.vpc, and similar patterns to prevent regression.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ai/tools/atmos/execute_command.go` around lines 96 - 123,
isDestructiveAtmosCommand assumes subcommands live at fixed positions (args[1],
args[2]) which breaks when flags precede the subcommand (e.g., terraform -s prod
apply ...); update isDestructiveAtmosCommand to locate the real subcommand by
first confirming args[0] == "terraform" then skipping any leading flags (tokens
that start with "-") to find the first non-flag token as subCmd, and when
handling nested verbs like "state" or "workspace" similarly advance past any
flags to find the nested subcommand (use the existing
destructiveTerraformSubcmds, destructiveTerraformStateSubcmds,
destructiveTerraformWorkspaceSubcmds lookups); add tests for cases like
"terraform -s prod apply vpc" and "terraform --stack prod state rm module.vpc"
to prevent regressions.
…ag-skipping, cross-platform tests Agent-Logs-Url: https://github.com/cloudposse/atmos/sessions/d44c3e97-4198-440c-8f26-c99a314e1d55 Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
shelllanguage tag to fenced code block (MD040)permissionModeFromConfighelper;NewExecuteAtmosCommandToolderives mode from config instead of hardcoding ModePromptisDestructiveAtmosCommand: addfirstNonFlaghelper that skips both--flag=value(embedded) and--flag value(space-separated) flag tokens to reach real subcommand_ATMOS_TEST_EXIT_ZERO=1support for cross-platform success subprocessechowith test binary; add flag-prefix test cases (both forms); addTestPermissionModeFromConfig(6 cases)