-
Notifications
You must be signed in to change notification settings - Fork 0
fix(model): use user shell for ccusage command execution #128
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 |
|---|---|---|
|
|
@@ -9,6 +9,8 @@ import ( | |
| "os" | ||
| "os/exec" | ||
| "os/user" | ||
| "runtime" | ||
| "strings" | ||
| "time" | ||
| ) | ||
|
|
||
|
|
@@ -231,16 +233,21 @@ func (s *ccUsageService) collectData(ctx context.Context, since time.Time) (*CCU | |
| slog.Debug("Using since parameter", "sinceDate", sinceDate, "since", since) | ||
| } | ||
|
|
||
| // Get user's shell to run command with proper environment | ||
| shell := getUserShell() | ||
|
|
||
| var cmd *exec.Cmd | ||
| if bunxErr == nil { | ||
| // Use bunx if available | ||
| cmd = exec.CommandContext(ctx, bunxPath, args...) | ||
| slog.Debug("Using bunx to collect ccusage data") | ||
| cmdStr := bunxPath + " " + shellEscapeArgs(args) | ||
| cmd = exec.CommandContext(ctx, shell, "-c", cmdStr) | ||
| slog.Debug("Using bunx to collect ccusage data", "shell", shell) | ||
| } else { | ||
| // Fall back to npx with --yes flag to auto-accept prompts | ||
| npxArgs := append([]string{"--yes"}, args...) | ||
| cmd = exec.CommandContext(ctx, npxPath, npxArgs...) | ||
| slog.Debug("Using npx to collect ccusage data") | ||
| cmdStr := npxPath + " " + shellEscapeArgs(npxArgs) | ||
| cmd = exec.CommandContext(ctx, shell, "-c", cmdStr) | ||
| slog.Debug("Using npx to collect ccusage data", "shell", shell) | ||
| } | ||
|
|
||
| // Execute the command | ||
|
|
@@ -410,3 +417,38 @@ func (s *ccUsageService) sendData(ctx context.Context, endpoint Endpoint, data * | |
| slog.Debug("CCUsage data sent successfully", "successCount", resp.SuccessCount, "totalCount", resp.TotalCount) | ||
| return nil | ||
| } | ||
|
|
||
| // getUserShell returns the user's shell executable path | ||
| // It checks the SHELL environment variable first, then falls back to sensible defaults | ||
| func getUserShell() string { | ||
| // Try to get the shell from environment variable | ||
| shell := os.Getenv("SHELL") | ||
| if shell != "" { | ||
| return shell | ||
| } | ||
|
|
||
| // Fall back to platform-specific defaults | ||
| if runtime.GOOS == "windows" { | ||
| // On Windows, prefer PowerShell, fall back to cmd | ||
| if pwsh, err := exec.LookPath("pwsh"); err == nil { | ||
| return pwsh | ||
| } | ||
| if powershell, err := exec.LookPath("powershell"); err == nil { | ||
| return powershell | ||
| } | ||
| return "cmd" | ||
| } | ||
|
|
||
| // On Unix-like systems, default to sh (POSIX shell) | ||
| return "/bin/sh" | ||
| } | ||
|
|
||
| // shellEscapeArgs joins arguments with spaces and escapes them for safe shell execution | ||
| func shellEscapeArgs(args []string) string { | ||
| escaped := make([]string, len(args)) | ||
| for i, arg := range args { | ||
| // Simple shell escaping: wrap in single quotes and escape single quotes | ||
| escaped[i] = "'" + strings.ReplaceAll(arg, "'", "'\"'\"'") + "'" | ||
| } | ||
| return strings.Join(escaped, " ") | ||
| } | ||
|
Comment on lines
+446
to
+454
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. This shell escaping implementation is specific to POSIX-compliant shells (like Using this POSIX-specific escaping on Windows will lead to command execution failures for arguments containing spaces or special characters. More critically, it can introduce command injection vulnerabilities. This function must be made platform-aware. You should check |
||
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.
The command construction and execution logic has a couple of issues that will cause it to fail on Windows and on any OS if paths contain spaces:
bunxPathornpxPath) is not escaped. If the path contains spaces (which is common on Windows, e.g.,C:\Program Files\...), the shell will misinterpret the command, leading to execution failure.-cflag is hardcoded to pass the command string to the shell. This is incorrect for PowerShell, which expects-Command, and forcmd.exe, which expects/c. SincegetUserShellcan return any of these on Windows, command execution will fail.To fix this, you should treat the executable path as the first argument to be escaped along with the other arguments. You also need to use the correct flag for the detected shell. A good approach would be to modify
getUserShellto return both the shell path and the appropriate flag.This issue applies to both the
bunxandnpxcommand construction.