Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions .claude/skills/post-merge-cleanup/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,15 @@ Run this after squash-merging a PR to clean up the local repo.
```

3. Delete local branches whose remote tracking branch is gone:
First check which branches are gone:
```bash
git branch -vv | grep '\[.*: gone\]' | awk '{print $1}' | xargs -r git branch -D
git branch -vv | grep '\[.*: gone\]'
```
If no gone branches exist, skip this step.
If no gone branches exist, skip this step. Otherwise, delete each one individually:
```bash
git branch -D <branch-name>
```
Do NOT use a piped `xargs` command — it triggers unnecessary permission prompts. Use separate `git branch -D` calls for each gone branch (can be combined in one call: `git branch -D branch1 branch2`).
Comment thread
coderabbitai[bot] marked this conversation as resolved.

4. Check for any remaining non-main local branches and report them. Do NOT delete branches that still have a remote — only report them.

Expand Down
6 changes: 5 additions & 1 deletion cli/cmd/doctor.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,12 @@
text := report.FormatText()

// Save to file.
safeDir, err := safeStateDir(state)
if err != nil {
return err
Comment thread Dismissed
}
filename := fmt.Sprintf("synthorg-diagnostic-%s.txt", time.Now().Format("20060102-150405"))
savePath := filepath.Join(state.DataDir, filename)
savePath := filepath.Join(safeDir, filename)
if err := os.WriteFile(savePath, []byte(text), 0o600); err != nil {
_, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Warning: could not save diagnostic file: %v\n", err)
} else {
Expand Down
11 changes: 8 additions & 3 deletions cli/cmd/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,11 @@
}

func writeInitFiles(state config.State) error {
if err := config.EnsureDir(state.DataDir); err != nil {
safeDir, err := config.SecurePath(state.DataDir)
if err != nil {
return err
}
if err := config.EnsureDir(safeDir); err != nil {
return fmt.Errorf("creating data directory: %w", err)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There's a small redundancy here. You get a safeDir by calling config.SecurePath and then pass it to config.EnsureDir, which itself calls config.SecurePath on its input. This means the path is being validated and cleaned twice.

To avoid this, you could call os.MkdirAll directly with the safeDir. This would keep the logic of 'secure once, use many times' while removing the redundant check.

Suggested change
if err := config.EnsureDir(safeDir); err != nil {
return fmt.Errorf("creating data directory: %w", err)
}
if err := os.MkdirAll(safeDir, 0o700); err != nil {
return fmt.Errorf("creating data directory: %w", err)
}


Expand All @@ -191,7 +195,7 @@
return fmt.Errorf("generating compose file: %w", err)
}

composePath := filepath.Join(state.DataDir, "compose.yml")
composePath := filepath.Join(safeDir, "compose.yml")
if err := os.WriteFile(composePath, composeYAML, 0o600); err != nil {
return fmt.Errorf("writing compose file: %w", err)
}
Expand Down Expand Up @@ -228,7 +232,8 @@
}

func fileExists(path string) bool {
_, err := os.Stat(path)
clean := filepath.Clean(path)
_, err := os.Stat(clean)
Comment thread Fixed
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
return err == nil
}

Expand Down
10 changes: 7 additions & 3 deletions cli/cmd/logs.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,13 @@
return fmt.Errorf("loading config: %w", err)
}

composePath := filepath.Join(state.DataDir, "compose.yml")
safeDir, err := safeStateDir(state)
if err != nil {
Comment thread Dismissed
return err
}
composePath := filepath.Join(safeDir, "compose.yml")
if _, err := os.Stat(composePath); errors.Is(err, os.ErrNotExist) {
return fmt.Errorf("compose.yml not found in %s — run 'synthorg init' first", state.DataDir)
return fmt.Errorf("compose.yml not found in %s — run 'synthorg init' first", safeDir)
}

info, err := docker.Detect(ctx)
Expand Down Expand Up @@ -78,5 +82,5 @@
composeArgs = append(composeArgs, "--")
composeArgs = append(composeArgs, args...)

return composeRun(ctx, cmd, info, state.DataDir, composeArgs...)
return composeRun(ctx, cmd, info, safeDir, composeArgs...)
}
6 changes: 6 additions & 0 deletions cli/cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ func resolveDataDir() string {
return dir
}

// safeStateDir returns a validated absolute path from the loaded state's DataDir.
// This satisfies CodeQL's go/path-injection by applying SecurePath at the call site.
func safeStateDir(state config.State) (string, error) {
return config.SecurePath(state.DataDir)
}

// isInteractive returns true if stdin is a terminal (not piped or in CI).
func isInteractive() bool {
fi, err := os.Stdin.Stat()
Expand Down
12 changes: 8 additions & 4 deletions cli/cmd/start.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,13 @@
return fmt.Errorf("loading config: %w", err)
}

composePath := filepath.Join(state.DataDir, "compose.yml")
safeDir, err := safeStateDir(state)
if err != nil {
Comment thread Dismissed
return err
}
composePath := filepath.Join(safeDir, "compose.yml")
if _, err := os.Stat(composePath); errors.Is(err, os.ErrNotExist) {
return fmt.Errorf("compose.yml not found in %s — run 'synthorg init' first", state.DataDir)
return fmt.Errorf("compose.yml not found in %s — run 'synthorg init' first", safeDir)
}

info, err := docker.Detect(ctx)
Expand All @@ -52,13 +56,13 @@

// Pull latest images.
_, _ = fmt.Fprintln(cmd.OutOrStdout(), "Pulling images...")
if err := composeRun(ctx, cmd, info, state.DataDir, "pull"); err != nil {
if err := composeRun(ctx, cmd, info, safeDir, "pull"); err != nil {
return fmt.Errorf("pulling images: %w", err)
}

// Start containers.
_, _ = fmt.Fprintln(cmd.OutOrStdout(), "Starting containers...")
if err := composeRun(ctx, cmd, info, state.DataDir, "up", "-d"); err != nil {
if err := composeRun(ctx, cmd, info, safeDir, "up", "-d"); err != nil {
return fmt.Errorf("starting containers: %w", err)
}

Expand Down
6 changes: 5 additions & 1 deletion cli/cmd/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,11 @@

printVersionInfo(out, state)

composePath := filepath.Join(state.DataDir, "compose.yml")
safeDir, err := safeStateDir(state)
if err != nil {
Comment thread Dismissed
return err
}
composePath := filepath.Join(safeDir, "compose.yml")
if _, err := os.Stat(composePath); errors.Is(err, os.ErrNotExist) {
_, _ = fmt.Fprintln(out, "Not initialized — run 'synthorg init' first.")
return nil
Expand Down
10 changes: 7 additions & 3 deletions cli/cmd/stop.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,13 @@
return fmt.Errorf("loading config: %w", err)
}

composePath := filepath.Join(state.DataDir, "compose.yml")
safeDir, err := safeStateDir(state)
if err != nil {
Comment thread Dismissed
return err
}
composePath := filepath.Join(safeDir, "compose.yml")
if _, err := os.Stat(composePath); errors.Is(err, os.ErrNotExist) {
return fmt.Errorf("compose.yml not found in %s — run 'synthorg init' first", state.DataDir)
return fmt.Errorf("compose.yml not found in %s — run 'synthorg init' first", safeDir)
}

info, err := docker.Detect(ctx)
Expand All @@ -41,7 +45,7 @@
}

_, _ = fmt.Fprintln(cmd.OutOrStdout(), "Stopping containers...")
if err := composeRun(ctx, cmd, info, state.DataDir, "down"); err != nil {
if err := composeRun(ctx, cmd, info, safeDir, "down"); err != nil {
return fmt.Errorf("stopping containers: %w", err)
}

Expand Down
5 changes: 4 additions & 1 deletion cli/cmd/uninstall.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,13 @@
}

if removeData {
dir := state.DataDir
dir, err := config.SecurePath(state.DataDir)
if err != nil {
return err
Comment thread Dismissed
}
// Safety: refuse to remove root, home, or empty paths.
home, _ := os.UserHomeDir()
if dir == "" || dir == "/" || dir == home || (len(dir) == 3 && dir[1] == ':' && dir[2] == '\\') {
return fmt.Errorf("refusing to remove %q — does not look like an app data directory", dir)
}
if err := os.RemoveAll(dir); err != nil {
Expand Down
19 changes: 18 additions & 1 deletion cli/internal/config/paths.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package config

import (
"fmt"
"os"
"path/filepath"
"runtime"
Expand Down Expand Up @@ -40,7 +41,23 @@
}
}

// SecurePath validates that a path is absolute and returns a cleaned version.
// This satisfies static analysis (CodeQL go/path-injection) by ensuring
// environment-variable-derived paths are sanitized before filesystem use.
func SecurePath(path string) (string, error) {
clean := filepath.Clean(path)
if !filepath.IsAbs(clean) {
return "", fmt.Errorf("path must be absolute, got %q", path)
}
return clean, nil
Comment on lines +49 to +61
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// EnsureDir creates the directory (and parents) if it does not exist.
// The path must be absolute.
func EnsureDir(path string) error {
return os.MkdirAll(path, 0o700)
safe, err := SecurePath(path)
if err != nil {
return err
}
return os.MkdirAll(safe, 0o700)
Comment thread Fixed
Comment on lines +49 to +71
}
16 changes: 12 additions & 4 deletions cli/internal/config/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,12 @@
// Load reads State from disk. Returns a default state with the given dataDir
// if the file does not exist (so --data-dir is respected on bootstrap).
func Load(dataDir string) (State, error) {
path := StatePath(dataDir)
data, err := os.ReadFile(path)
safeDir, err := SecurePath(dataDir)
if err != nil {
return State{}, err
}
path := StatePath(safeDir)
data, err := os.ReadFile(path) //nolint:gosec // path validated by SecurePath
Comment thread Dismissed
if err != nil {
if errors.Is(err, os.ErrNotExist) {
defaults := DefaultState()
Expand Down Expand Up @@ -73,12 +77,16 @@

// Save writes State to disk as indented JSON.
func Save(s State) error {
if err := EnsureDir(s.DataDir); err != nil {
safeDir, err := SecurePath(s.DataDir)
if err != nil {
return err
}
if err := EnsureDir(safeDir); err != nil {
return err
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There's a redundant call to SecurePath here. EnsureDir already validates its input path, so calling it with an already-secured path means the validation happens twice.

You can call os.MkdirAll directly with safeDir to avoid this.

Suggested change
if err := EnsureDir(safeDir); err != nil {
return err
}
if err := os.MkdirAll(safeDir, 0o700); err != nil {
return err
}

data, err := json.MarshalIndent(s, "", " ")
if err != nil {
return err
}
return os.WriteFile(StatePath(s.DataDir), data, 0o600)
return os.WriteFile(StatePath(safeDir), data, 0o600) //nolint:gosec // path validated by SecurePath
Comment thread Dismissed
Comment on lines 75 to +88
}
Loading