diff --git a/cmd/container-use/logger.go b/cmd/container-use/logger.go index cea1df56..2c7d676c 100644 --- a/cmd/container-use/logger.go +++ b/cmd/container-use/logger.go @@ -36,7 +36,7 @@ func setupLogger() error { logFile = v } - file, err := os.OpenFile(logFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644) + file, err := os.OpenFile(logFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600) if err != nil { return fmt.Errorf("failed to open log file %s: %w", logFile, err) } diff --git a/cmd/container-use/watch_windows.go b/cmd/container-use/watch_windows.go index 108cef7e..15cecd96 100644 --- a/cmd/container-use/watch_windows.go +++ b/cmd/container-use/watch_windows.go @@ -122,21 +122,28 @@ func runGitLogWindows(ctx context.Context) error { // Start the command if err := cmd.Start(); err != nil { - pw.Close() - pr.Close() + pw.Close() //nolint:errcheck // best effort cleanup on start failure + pr.Close() //nolint:errcheck return fmt.Errorf("failed to start git log: %w", err) } // Close write end so we can read - pw.Close() + if err := pw.Close(); err != nil { + return fmt.Errorf("failed to close pipe writer: %w", err) + } // Read all output into buffer scanner := bufio.NewScanner(pr) for scanner.Scan() { - buf.WriteString(scanner.Text() + "\n") + buf.WriteString(scanner.Text() + "\n") //nolint:errcheck // scanner text write never fails in practice + } + if err := scanner.Err(); err != nil { + return fmt.Errorf("failed to read git log output: %w", err) } - pr.Close() + if err := pr.Close(); err != nil { + return fmt.Errorf("failed to close pipe reader: %w", err) + } // Wait for command to complete if err := cmd.Wait(); err != nil { @@ -144,7 +151,9 @@ func runGitLogWindows(ctx context.Context) error { } // Output everything at once for smooth rendering - os.Stdout.Write(buf.Bytes()) + if _, err := os.Stdout.Write(buf.Bytes()); err != nil { + return fmt.Errorf("failed to write output: %w", err) + } return nil } diff --git a/environment/environment.go b/environment/environment.go index b421fc81..3331356c 100644 --- a/environment/environment.go +++ b/environment/environment.go @@ -144,6 +144,15 @@ func (env *Environment) apply(ctx context.Context, newState *dagger.Container) e return nil } +func isAllowedShell(shell string) bool { + switch shell { + case "sh", "/bin/sh", "bash", "/bin/bash", "zsh", "/bin/zsh", "ash", "/bin/ash": + return true + default: + return false + } +} + func containerWithEnvAndSecrets(dag *dagger.Client, container *dagger.Container, envs, secrets []string) (*dagger.Container, error) { for _, env := range envs { k, v, found := strings.Cut(env, "=") @@ -252,6 +261,10 @@ func (env *Environment) UpdateConfig(ctx context.Context, newConfig *Environment } func (env *Environment) Run(ctx context.Context, command, shell string, useEntrypoint bool) (string, error) { + if !isAllowedShell(shell) { + return "", fmt.Errorf("unsupported shell: %s", shell) + } + args := []string{} if command != "" { args = []string{shell, "-c", command} diff --git a/environment/environment_test.go b/environment/environment_test.go new file mode 100644 index 00000000..f5981e5d --- /dev/null +++ b/environment/environment_test.go @@ -0,0 +1,31 @@ +package environment + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestIsAllowedShell(t *testing.T) { + allowed := []string{ + "sh", "/bin/sh", + "bash", "/bin/bash", + "zsh", "/bin/zsh", + "ash", "/bin/ash", + } + for _, shell := range allowed { + assert.True(t, isAllowedShell(shell), "expected %q to be allowed", shell) + } + + blocked := []string{ + "/bin/echo", + "/bin/cat", + "/bin/rm", + "python3", + "../../bin/sh", + "", + } + for _, shell := range blocked { + assert.False(t, isAllowedShell(shell), "expected %q to be blocked", shell) + } +} diff --git a/repository/git.go b/repository/git.go index 55dacfbb..9145f49d 100644 --- a/repository/git.go +++ b/repository/git.go @@ -26,8 +26,29 @@ const ( var ( urlSchemeRegExp = regexp.MustCompile(`^[^:]+://`) scpLikeURLRegExp = regexp.MustCompile(`^(?:(?P[^@]+)@)?(?P[^:\s]+):(?:(?P[0-9]{1,5})(?:\/|:))?(?P[^\\].*\/[^\\].*)$`) + // validGitRefComponent rejects ref/branch names that git would interpret as + // flags (leading "-") or that contain characters likely to break argument + // parsing (whitespace, control characters, NUL). It is intentionally + // conservative; real branch names use the petname generator today. + validGitRefComponent = regexp.MustCompile(`^[a-zA-Z0-9._~/-]+$`) ) +// validateGitRefComponent rejects ref names that could be interpreted by git as +// options or that contain unsafe characters. It returns an error describing why +// the name was rejected. +func validateGitRefComponent(name string) error { + if name == "" { + return fmt.Errorf("ref name is empty") + } + if strings.HasPrefix(name, "-") { + return fmt.Errorf("ref name %q starts with '-', which git would interpret as an option", name) + } + if !validGitRefComponent.MatchString(name) { + return fmt.Errorf("ref name %q contains characters not allowed in a git ref component", name) + } + return nil +} + // RunGitCommand executes a git command in the specified directory. // This is exported for use in tests and other packages that need direct git access. func RunGitCommand(ctx context.Context, dir string, args ...string) (out string, rerr error) { @@ -120,8 +141,13 @@ func (r *Repository) deleteLocalRemoteBranch(id string) error { // It pushes the specified gitRef to create a new branch with the given id, then creates a worktree from that branch. // Returns the worktree path, any submodule warning, and an error. func (r *Repository) initializeWorktree(ctx context.Context, id, gitRef string) (string, string, error) { + if err := validateGitRefComponent(id); err != nil { + return "", "", fmt.Errorf("invalid environment id: %w", err) + } if gitRef == "" { gitRef = "HEAD" + } else if err := validateGitRefComponent(gitRef); err != nil { + return "", "", fmt.Errorf("invalid git ref: %w", err) } worktreePath, err := r.WorktreePath(id) @@ -373,8 +399,17 @@ func (r *Repository) exportEnvironmentFile(ctx context.Context, env *environment return fmt.Errorf("failed to get worktree path: %w", err) } + // Reject absolute paths and paths that escape the worktree via "..". + if filepath.IsAbs(filePath) { + return fmt.Errorf("file path must be relative to the workdir: %s", filePath) + } + clean := filepath.Clean(filePath) + if strings.HasPrefix(clean, "..") { + return fmt.Errorf("file path escapes workdir: %s", filePath) + } + // Get the absolute path for the file in the worktree - absoluteFilePath := filepath.Join(worktreePath, filePath) + absoluteFilePath := filepath.Join(worktreePath, clean) // Ensure the directory exists if err := os.MkdirAll(filepath.Dir(absoluteFilePath), 0755); err != nil { @@ -382,7 +417,7 @@ func (r *Repository) exportEnvironmentFile(ctx context.Context, env *environment } // Export the single file from the environment - _, err = env.WorkdirFile(filePath).Export(ctx, absoluteFilePath) + _, err = env.WorkdirFile(clean).Export(ctx, absoluteFilePath) if err != nil { return fmt.Errorf("failed to export file %s: %w", filePath, err) } @@ -429,6 +464,9 @@ func (r *Repository) saveState(ctx context.Context, env *environment.Environment if _, err := f.Write(state); err != nil { return err } + if err := f.Close(); err != nil { + return fmt.Errorf("failed to close temporary state file: %w", err) + } return r.lockManager.WithLock(ctx, LockTypeNotes, func() error { _, err = RunGitCommand(ctx, worktreePath, "notes", "--ref", gitNotesStateRef, "add", "-f", "-F", f.Name()) diff --git a/repository/git_test.go b/repository/git_test.go index 702f0874..8727ca55 100644 --- a/repository/git_test.go +++ b/repository/git_test.go @@ -202,3 +202,44 @@ func createDir(t *testing.T, dir, name string) { err := os.MkdirAll(path, 0755) require.NoError(t, err) } + +func TestValidateGitRefComponent(t *testing.T) { + valid := []string{ + "main", + "cu-adverb-animal", + "v1.0.0", + "feature/test_123", + "HEAD", + } + for _, name := range valid { + assert.NoError(t, validateGitRefComponent(name), "expected %q to be valid", name) + } + + invalid := []string{ + "", + "--help", + "-foo", + "feature test", + "foo\nbar", + "foo\x00bar", + } + for _, name := range invalid { + assert.Error(t, validateGitRefComponent(name), "expected %q to be invalid", name) + } +} + +func TestExportEnvironmentFileRejectsPathTraversal(t *testing.T) { + tmp := t.TempDir() + worktreePath := filepath.Join(tmp, "worktree") + require.NoError(t, os.MkdirAll(worktreePath, 0755)) + + for _, filePath := range []string{ + "../../../etc/cron.d/evil", + "/etc/passwd", + "foo/../../etc/passwd", + "../secret.txt", + } { + assert.True(t, filepath.IsAbs(filePath) || strings.HasPrefix(filepath.Clean(filePath), ".."), + "test case %q should be classified as escaping the workdir", filePath) + } +} diff --git a/repository/repository.go b/repository/repository.go index dac0cfa2..361dcebf 100644 --- a/repository/repository.go +++ b/repository/repository.go @@ -451,12 +451,17 @@ func (r *Repository) Delete(ctx context.Context, id string) error { // Checkout changes the user's current branch to that of the identified environment. // It attempts to get the most recent commit from the environment without discarding any user changes. func (r *Repository) Checkout(ctx context.Context, id, branch string) (string, error) { + if err := validateGitRefComponent(id); err != nil { + return "", fmt.Errorf("invalid environment id: %w", err) + } if err := r.exists(ctx, id); err != nil { return "", err } if branch == "" { branch = "cu-" + id + } else if err := validateGitRefComponent(branch); err != nil { + return "", fmt.Errorf("invalid branch name: %w", err) } // set up remote tracking branch if it's not already there @@ -549,6 +554,9 @@ func (r *Repository) Diff(ctx context.Context, id string, w io.Writer) error { } func (r *Repository) Merge(ctx context.Context, id string, w io.Writer) error { + if err := validateGitRefComponent(id); err != nil { + return fmt.Errorf("invalid environment id: %w", err) + } envInfo, err := r.Info(ctx, id) if err != nil { return err @@ -558,6 +566,9 @@ func (r *Repository) Merge(ctx context.Context, id string, w io.Writer) error { } func (r *Repository) Apply(ctx context.Context, id string, w io.Writer) error { + if err := validateGitRefComponent(id); err != nil { + return fmt.Errorf("invalid environment id: %w", err) + } envInfo, err := r.Info(ctx, id) if err != nil { return err