fix: resolve 11 CodeQL path-injection alerts in Go CLI#411
fix: resolve 11 CodeQL path-injection alerts in Go CLI#411
Conversation
Add config.SecurePath() helper that validates paths are absolute and applies filepath.Clean() — satisfies CodeQL's go/path-injection taint tracking. Apply at all filesystem operation call sites: - config: EnsureDir, Load, Save - cmd: start, stop, logs, doctor, status, uninstall, init Also update post-merge-cleanup skill to avoid piped xargs commands that trigger permission prompts.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds path sanitization via a new config.SecurePath and applies it across CLI commands and state handling, replacing direct uses of state.DataDir with validated absolute paths and updating init/uninstall/diagnostics/doctor flows and related tests and docs. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture of the CLI by addressing potential path injection vulnerabilities. It introduces a centralized mechanism for path sanitization and ensures that all filesystem operations involving user-controlled or environment-derived paths are properly validated and cleaned, thereby preventing malicious path manipulation. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses 11 CodeQL go/path-injection alerts by introducing a config.SecurePath helper for path sanitization and applying it across the CLI commands. The changes are well-targeted and improve the security of file system operations. I've identified a couple of places where the new helpers are used in a way that leads to redundant path validation. My review includes suggestions to streamline these calls for better performance and code clarity. The documentation update for the post-merge cleanup script is also a good improvement.
cli/cmd/init.go
Outdated
| if err := config.EnsureDir(safeDir); err != nil { | ||
| return fmt.Errorf("creating data directory: %w", err) | ||
| } |
There was a problem hiding this comment.
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.
| 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) | |
| } |
cli/internal/config/state.go
Outdated
| if err := EnsureDir(safeDir); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
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.
| if err := EnsureDir(safeDir); err != nil { | |
| return err | |
| } | |
| if err := os.MkdirAll(safeDir, 0o700); err != nil { | |
| return err | |
| } |
Greptile SummaryThis PR resolves all 11 CodeQL Key changes:
Issues to consider:
Confidence Score: 4/5
Important Files Changed
|
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 (2)
cli/cmd/status.go (1)
74-75:⚠️ Potential issue | 🟠 MajorInconsistent path handling:
printContainerStatesandprintResourceUsagestill use unsanitizedstate.DataDir.While
runStatuscorrectly validates the path viasafeStateDir, the helper functionsprintContainerStates(line 75) andprintResourceUsage(line 86) passstate.DataDirdirectly todocker.ComposeExecOutput. This inconsistency undermines the path sanitization effort for these code paths.Consider passing
safeDirto these functions instead ofstate.🔧 Proposed fix
- printContainerStates(ctx, out, info, state) - printResourceUsage(ctx, out, info, state) + printContainerStates(ctx, out, info, safeDir) + printResourceUsage(ctx, out, info, safeDir) printHealthStatus(ctx, out, state)Then update the function signatures:
-func printContainerStates(ctx context.Context, out io.Writer, info docker.Info, state config.State) { - psOut, err := docker.ComposeExecOutput(ctx, info, state.DataDir, "ps", "--format", "json") +func printContainerStates(ctx context.Context, out io.Writer, info docker.Info, dataDir string) { + psOut, err := docker.ComposeExecOutput(ctx, info, dataDir, "ps", "--format", "json")-func printResourceUsage(ctx context.Context, out io.Writer, info docker.Info, state config.State) { +func printResourceUsage(ctx context.Context, out io.Writer, info docker.Info, dataDir string) { // Get container IDs for this compose project only. - psOut, err := docker.ComposeExecOutput(ctx, info, state.DataDir, "ps", "-q") + psOut, err := docker.ComposeExecOutput(ctx, info, dataDir, "ps", "-q")Also applies to: 84-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/cmd/status.go` around lines 74 - 75, The helpers printContainerStates and printResourceUsage currently call docker.ComposeExecOutput with unsanitized state.DataDir; update runStatus to pass the validated safeDir (result of safeStateDir) into these helpers and change their signatures (printContainerStates, printResourceUsage) to accept a dataDir string (or safeDir) instead of the whole config.State, then use that dataDir when calling docker.ComposeExecOutput; ensure references to docker.ComposeExecOutput, printContainerStates, printResourceUsage, runStatus, and safeStateDir are updated accordingly.cli/internal/config/state.go (1)
51-59: 🛠️ Refactor suggestion | 🟠 MajorDuplicated path validation logic — reuse
SecurePathinstead.Lines 54-58 manually replicate the validation that
SecurePathperforms. SincedataDirwas already validated viaSecurePathat line 44 (returningsafeDir), you should usesafeDirhere instead of re-validatingdataDir.♻️ Proposed fix
if err != nil { if errors.Is(err, os.ErrNotExist) { defaults := DefaultState() - // Normalize the dataDir the same way we validate loaded paths. - clean := filepath.Clean(dataDir) - if !filepath.IsAbs(clean) { - return State{}, fmt.Errorf("data_dir must be an absolute path, got %q", dataDir) - } - defaults.DataDir = clean + defaults.DataDir = safeDir return defaults, nil } return State{}, err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/internal/config/state.go` around lines 51 - 59, Replace the duplicated manual validation with the already validated safeDir from SecurePath: when creating defaults (DefaultState()) use safeDir instead of re-cleaning/validating dataDir and assign defaults.DataDir = safeDir; remove the filepath.Clean/IsAbs check block so the code relies on the prior SecurePath validation (reference SecurePath, safeDir, DefaultState, and defaults.DataDir).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/post-merge-cleanup/SKILL.md:
- Around line 17-26: Add blank lines before and after each fenced code block in
the SKILL.md snippet so markdownlint MD031 is satisfied: ensure there is an
empty line before the ```bash block that contains "git branch -vv | grep '\[.*:
gone\]'" and an empty line after that closing fence, and likewise add empty
lines around the second ```bash block containing "git branch -D <branch-name>"
so both fenced blocks have a blank line separating them from the surrounding
prose.
In `@cli/cmd/init.go`:
- Around line 235-236: The fileExists helper currently passes unvalidated input
(after filepath.Clean) to os.Stat causing a path-injection finding; update
fileExists to run the same SecurePath validation used elsewhere before calling
os.Stat (e.g., call SecurePath on the cleaned path and return/handle any error
from SecurePath), then use the validated/secured path in the os.Stat call so
only approved paths reach os.Stat.
In `@cli/internal/config/paths.go`:
- Around line 44-53: SecurePath currently enforces absolute, cleaned paths but
does not restrict which absolute locations are allowed, which can trigger CodeQL
warnings; update the SecurePath function by adding a concise comment above it
that documents the security assumption: that paths originate from
user-controlled CLI flags/config (e.g., --data-dir) and are therefore trusted by
design, and note that intentional lack of filesystem containment is acceptable
for this CLI tool to avoid false positives from static analysis (mention CodeQL
go/path-injection). Keep the comment short and specific to SecurePath so future
maintainers understand the trust boundary and why no further containment checks
are performed.
---
Outside diff comments:
In `@cli/cmd/status.go`:
- Around line 74-75: The helpers printContainerStates and printResourceUsage
currently call docker.ComposeExecOutput with unsanitized state.DataDir; update
runStatus to pass the validated safeDir (result of safeStateDir) into these
helpers and change their signatures (printContainerStates, printResourceUsage)
to accept a dataDir string (or safeDir) instead of the whole config.State, then
use that dataDir when calling docker.ComposeExecOutput; ensure references to
docker.ComposeExecOutput, printContainerStates, printResourceUsage, runStatus,
and safeStateDir are updated accordingly.
In `@cli/internal/config/state.go`:
- Around line 51-59: Replace the duplicated manual validation with the already
validated safeDir from SecurePath: when creating defaults (DefaultState()) use
safeDir instead of re-cleaning/validating dataDir and assign defaults.DataDir =
safeDir; remove the filepath.Clean/IsAbs check block so the code relies on the
prior SecurePath validation (reference SecurePath, safeDir, DefaultState, and
defaults.DataDir).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bcee3b6b-7374-472a-a9a7-f983e2646fe6
📒 Files selected for processing (11)
.claude/skills/post-merge-cleanup/SKILL.mdcli/cmd/doctor.gocli/cmd/init.gocli/cmd/logs.gocli/cmd/root.gocli/cmd/start.gocli/cmd/status.gocli/cmd/stop.gocli/cmd/uninstall.gocli/internal/config/paths.gocli/internal/config/state.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Greptile Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (1)
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
cli/**/*.go: CLI Go code: Go 1.26+, dependencies incli/go.mod. Commands include: init, start, stop, status, logs, doctor, update, uninstall, version. Build with GoReleaser for cross-compile (linux/darwin/windows × amd64/arm64).
Go CLI: Go 1.26+, dependencies incli/go.mod. Use Cobra for commands, charmbracelet/huh for CLI UI. Build with GoReleaser.
Files:
cli/cmd/uninstall.gocli/cmd/root.gocli/internal/config/paths.gocli/cmd/stop.gocli/cmd/status.gocli/cmd/start.gocli/cmd/doctor.gocli/cmd/init.gocli/internal/config/state.gocli/cmd/logs.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T18:26:05.851Z
Learning: Go CLI build commands: `cd cli && go build -o synthorg ./main.go`, `cd cli && go test ./...`, `cd cli && go vet ./...`, `cd cli && golangci-lint run`
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T18:26:05.851Z
Learning: Applies to .github/workflows/cli.yml : CLI workflow (`.github/workflows/cli.yml`): Go lint (golangci-lint + go vet) + test (-race -coverprofile) + build (cross-compile matrix: linux/darwin/windows × amd64/arm64) + vulnerability check (govulncheck) on `cli/**` changes. GoReleaser release on `v*` tags (attaches assets to existing Release Please release). Post-release pins checksums in install scripts and appends to GitHub Release notes.
📚 Learning: 2026-03-14T18:26:05.851Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T18:26:05.851Z
Learning: Applies to cli/**/*.go : CLI Go code: Go 1.26+, dependencies in `cli/go.mod`. Commands include: init, start, stop, status, logs, doctor, update, uninstall, version. Build with GoReleaser for cross-compile (linux/darwin/windows × amd64/arm64).
Applied to files:
cli/cmd/start.go
🧬 Code graph analysis (4)
cli/cmd/uninstall.go (1)
cli/internal/config/paths.go (2)
SecurePath(47-53)DataDir(18-24)
cli/cmd/root.go (2)
cli/internal/config/state.go (1)
State(14-23)cli/internal/config/paths.go (2)
SecurePath(47-53)DataDir(18-24)
cli/cmd/init.go (1)
cli/internal/config/paths.go (3)
SecurePath(47-53)DataDir(18-24)EnsureDir(57-63)
cli/internal/config/state.go (1)
cli/internal/config/paths.go (3)
SecurePath(47-53)DataDir(18-24)EnsureDir(57-63)
🪛 GitHub Check: CodeQL
cli/internal/config/paths.go
[failure] 62-62: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
cli/cmd/stop.go
[failure] 34-34: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
cli/cmd/status.go
[failure] 44-44: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
cli/cmd/start.go
[failure] 38-38: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
cli/cmd/doctor.go
[failure] 44-44: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
cli/cmd/init.go
[failure] 236-236: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
cli/internal/config/state.go
[failure] 49-49: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
[failure] 91-91: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
cli/cmd/logs.go
[failure] 49-49: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
🪛 markdownlint-cli2 (0.21.0)
.claude/skills/post-merge-cleanup/SKILL.md
[warning] 19-19: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 21-21: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 23-23: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 25-25: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (12)
cli/internal/config/paths.go (1)
55-62: CodeQL alert on line 62 is expected but may be a false positive in this context.The static analysis tool flags
os.MkdirAll(safe, 0o700)because the path ultimately derives from user input. The mitigation applied (ensuring absolute paths) is appropriate for a CLI tool's trust model. The//nolint:goseccomment used elsewhere could be applied here if needed to suppress warnings, but the code is functionally correct.cli/internal/config/state.go (1)
44-49: CodeQL alerts on lines 49 and 91 are false positives givenSecurePathvalidation.The
//nolint:goseccomments appropriately document that the paths are validated bySecurePath. CodeQL's warnings stem from taint tracking that doesn't recognize the validation as sufficient sanitization. The implementation is correct for this CLI's trust model.Also applies to: 80-91
cli/cmd/root.go (1)
44-48: LGTM!Clean helper function that centralizes the
SecurePathcall for command handlers. Good documentation referencing the CodeQL alert it addresses.cli/cmd/status.go (1)
43-47: LGTM for the path sanitization changes inrunStatus.The use of
safeStateDircorrectly validates the path before filesystem operations.cli/cmd/doctor.go (1)
42-48: LGTM!The path sanitization is correctly applied. The
safeDiris used consistently for the diagnostic file save path. The CodeQL alert is expected but represents a false positive given theSecurePathvalidation.cli/cmd/stop.go (1)
33-48: LGTM!The path sanitization is correctly and consistently applied throughout the
runStopfunction. All filesystem and compose operations use the validatedsafeDir.cli/cmd/logs.go (2)
48-55: LGTM!The path sanitization is correctly applied and used consistently for compose path operations.
62-75: Good input validation for--tailand service names.The validation of
--tailvalues and service name pattern matching (serviceNamePattern) provides good defense against command injection via compose arguments.cli/cmd/uninstall.go (1)
109-112: Good hardening before destructive delete.Using
config.SecurePathbefore safety checks andos.RemoveAllcorrectly moves deletion to a validated/normalized path.cli/cmd/start.go (2)
37-43: Safe directory resolution is correctly centralized.The
safeStateDir(state)gate beforefilepath.Join/os.Statis the right mitigation pattern for these path operations.
59-65: Compose operations now consistently use the sanitized directory.Switching both
pullandup -dcalls tosafeDircloses the previous unsafe-path gap in execution context.cli/cmd/init.go (1)
184-199:writeInitFilesnow applies secure-path handling correctly.Resolving
safeDirviaconfig.SecurePathand writingcompose.ymlunder that directory is a solid fix.
There was a problem hiding this comment.
Pull request overview
This PR aims to eliminate CodeQL go/path-injection alerts in the Go CLI by introducing a central config.SecurePath() validator and applying it at filesystem path usage sites across config/state persistence and command implementations.
Changes:
- Add
config.SecurePath()(absolute-path validation +filepath.Clean) and integrate it intoEnsureDir. - Apply sanitized data directory usage in config load/save and multiple CLI commands via
safeStateDir(). - Update post-merge cleanup skill documentation to avoid piped
xargsbranch deletion.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
cli/internal/config/paths.go |
Introduces SecurePath() and routes EnsureDir() through it to enforce cleaned absolute paths. |
cli/internal/config/state.go |
Validates/cleans state paths on load/save before filesystem reads/writes. |
cli/cmd/root.go |
Adds safeStateDir() helper to centralize per-command path validation. |
cli/cmd/start.go |
Uses validated state dir for compose file checks and compose execution. |
cli/cmd/stop.go |
Uses validated state dir for compose file checks and compose execution. |
cli/cmd/logs.go |
Uses validated state dir for compose file checks and compose execution. |
cli/cmd/status.go |
Uses validated state dir for compose file existence check. |
cli/cmd/doctor.go |
Uses validated state dir for saving the diagnostics file. |
cli/cmd/init.go |
Uses validated state dir when creating directories and writing init artifacts. |
cli/cmd/uninstall.go |
Validates state dir before destructive RemoveAll. |
.claude/skills/post-merge-cleanup/SKILL.md |
Updates workflow guidance for deleting gone local branches without piped xargs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 | ||
| } | ||
| 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 |
| if err := config.EnsureDir(safeDir); err != nil { | ||
| return fmt.Errorf("creating data directory: %w", err) | ||
| } | ||
|
|
| safeDir, err := safeStateDir(state) | ||
| if err != nil { | ||
| return err | ||
| } |
| // 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 |
| // 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 | ||
| } | ||
|
|
||
| // 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) |
Fixes from go-reviewer, security-reviewer, CodeRabbit, Copilot, Gemini: - Fix 4 missed files: update.go, uninstall.go (stopAndRemoveVolumes), status.go (printContainerStates/printResourceUsage), diagnostics/collect.go - Remove dead redundant validation in state.go Load (use safeDir directly) - Use SecurePath in fileExists instead of bare filepath.Clean - Remove double-SecurePath in Save/writeInitFiles (use os.MkdirAll directly) - Normalize state.DataDir before marshaling in Save and writeInitFiles - Add trust boundary doc comment to SecurePath - Fix DataDir() fallback from "." to os.Getwd() (absolute path) - Add 5 unit tests for SecurePath (absolute, relative, empty, clean, traversal) - Fix SKILL.md markdown lint (blank lines around fenced code blocks)
| return err | ||
| } | ||
| state.DataDir = safeDir // normalize before persisting | ||
| if err := os.MkdirAll(safeDir, 0o700); err != nil { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Copilot Autofix
AI 21 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/cmd/uninstall.go (1)
113-127: 🧹 Nitpick | 🔵 TrivialDefense-in-depth validation before directory removal.
The combination of
SecurePathvalidation (line 113) plus the safety checks (line 119) provides robust protection against removing critical directories. The CodeQL alert on line 115 is a false positive since the path is validated.Note: This function calls
config.SecurePathdirectly rather than usingsafeStateDir(state)like other commands. While functionally equivalent, usingsafeStateDirwould be more consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/cmd/uninstall.go` around lines 113 - 127, The code uses config.SecurePath to validate the state.DataDir before removal but deviates from other commands by not using safeStateDir(state); update the uninstall command to obtain the directory via safeStateDir(state) (or call safeStateDir and use its returned dir/error) instead of calling config.SecurePath directly, keep the existing safety checks on dir (empty, "/", home, Windows root) and the os.RemoveAll(dir) removal logic, and ensure error handling and the final fmt.Fprintf call remain intact so behavior and defense-in-depth validation are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cli/cmd/uninstall.go`:
- Around line 113-127: The code uses config.SecurePath to validate the
state.DataDir before removal but deviates from other commands by not using
safeStateDir(state); update the uninstall command to obtain the directory via
safeStateDir(state) (or call safeStateDir and use its returned dir/error)
instead of calling config.SecurePath directly, keep the existing safety checks
on dir (empty, "/", home, Windows root) and the os.RemoveAll(dir) removal logic,
and ensure error handling and the final fmt.Fprintf call remain intact so
behavior and defense-in-depth validation are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e40d2093-64ab-40f4-9ec4-6fe4b2d4e29a
📒 Files selected for processing (9)
.claude/skills/post-merge-cleanup/SKILL.mdcli/cmd/init.gocli/cmd/status.gocli/cmd/uninstall.gocli/cmd/update.gocli/internal/config/paths.gocli/internal/config/paths_test.gocli/internal/config/state.gocli/internal/diagnostics/collect.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CLI Test (windows-latest)
- GitHub Check: Greptile Review
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (1)
cli/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
cli/**/*.go: CLI Go code: Go 1.26+, dependencies incli/go.mod. Commands include: init, start, stop, status, logs, doctor, update, uninstall, version. Build with GoReleaser for cross-compile (linux/darwin/windows × amd64/arm64).
Go CLI: Go 1.26+, dependencies incli/go.mod. Use Cobra for commands, charmbracelet/huh for CLI UI. Build with GoReleaser.
Files:
cli/cmd/init.gocli/cmd/uninstall.gocli/cmd/status.gocli/internal/config/paths.gocli/internal/config/state.gocli/internal/diagnostics/collect.gocli/internal/config/paths_test.gocli/cmd/update.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T18:26:05.851Z
Learning: Go CLI build commands: `cd cli && go build -o synthorg ./main.go`, `cd cli && go test ./...`, `cd cli && go vet ./...`, `cd cli && golangci-lint run`
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T18:26:05.851Z
Learning: Applies to .github/workflows/cli.yml : CLI workflow (`.github/workflows/cli.yml`): Go lint (golangci-lint + go vet) + test (-race -coverprofile) + build (cross-compile matrix: linux/darwin/windows × amd64/arm64) + vulnerability check (govulncheck) on `cli/**` changes. GoReleaser release on `v*` tags (attaches assets to existing Release Please release). Post-release pins checksums in install scripts and appends to GitHub Release notes.
📚 Learning: 2026-03-14T18:26:05.851Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T18:26:05.851Z
Learning: Applies to .pre-commit-config.yaml : Pre-commit hooks: trailing-whitespace, end-of-file-fixer, check-yaml, check-toml, check-json, check-merge-conflict, check-added-large-files, no-commit-to-branch (main), ruff check+format, gitleaks, hadolint (Dockerfile linting)
Applied to files:
.claude/skills/post-merge-cleanup/SKILL.md
📚 Learning: 2026-03-14T18:26:05.851Z
Learnt from: CR
Repo: Aureliolo/synthorg PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-14T18:26:05.851Z
Learning: When review agents find valid issues (including pre-existing issues in surrounding code, suggestions, and findings adjacent to the PR's changes), fix them all. No deferring, no 'out of scope' skipping.
Applied to files:
.claude/skills/post-merge-cleanup/SKILL.md
🧬 Code graph analysis (5)
cli/cmd/init.go (1)
cli/internal/config/paths.go (2)
SecurePath(56-62)DataDir(18-29)
cli/cmd/status.go (1)
cli/internal/docker/client.go (2)
Info(22-29)ComposeExecOutput(100-107)
cli/internal/config/state.go (1)
cli/internal/config/paths.go (2)
SecurePath(56-62)DataDir(18-29)
cli/internal/diagnostics/collect.go (2)
cli/internal/config/paths.go (2)
SecurePath(56-62)DataDir(18-29)cli/internal/docker/client.go (1)
ComposeExecOutput(100-107)
cli/internal/config/paths_test.go (1)
cli/internal/config/paths.go (1)
SecurePath(56-62)
🪛 GitHub Check: CodeQL
cli/cmd/init.go
[failure] 189-189: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
[failure] 240-240: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
cli/cmd/uninstall.go
[failure] 115-115: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
cli/cmd/status.go
[failure] 44-44: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
cli/internal/config/state.go
[failure] 49-49: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
[failure] 81-81: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
[failure] 88-88: Uncontrolled data used in path expression
This path depends on a user-provided value.
This path depends on a user-provided value.
This path depends on a user-provided value.
🔇 Additional comments (16)
.claude/skills/post-merge-cleanup/SKILL.md (1)
17-35: LGTM! Markdown formatting fixed and content improved.The blank lines around fenced code blocks have been correctly added, resolving the MD031 markdownlint warnings from the previous review. The two-step approach (check first with
grep, then delete individually) is a good defensive practice, and the explicit warning about avoiding pipedxargsaligns with the PR's stated objective. The addition of Step 5 for final workspace validation is a sensible best practice.cli/internal/config/paths.go (3)
49-62: Well-documented trust boundary for SecurePath.The security note clearly documents the CLI's trust model — users control their own installation directories. This addresses the CodeQL alerts appropriately since enforcing filesystem containment would be inappropriate for a user-facing CLI tool.
20-27: Reasonable fallback chain for DataDir.The fallback from
os.UserHomeDir()→os.Getwd()→"/"ensures DataDir always returns an absolute path, which aligns with SecurePath's requirements. The comment clearly explains the rationale.
64-71: EnsureDir correctly validates paths before filesystem operations.The integration of SecurePath ensures that only absolute, cleaned paths reach
os.MkdirAll. The CodeQL alert on line 71 is a false positive — the path is validated by SecurePath immediately prior.cli/internal/config/paths_test.go (1)
69-118: Good test coverage for SecurePath edge cases.The tests appropriately cover:
- Valid absolute paths (normalization)
- Relative path rejection
- Empty path rejection
- Path cleaning with
..components- Relative traversal rejection
The
absTestPathhelper ensures cross-platform compatibility.cli/internal/config/state.go (2)
43-70: Load correctly applies SecurePath validation at entry.The validation flow is sound:
- Input
dataDiris validated via SecurePath- If file doesn't exist, defaults use the validated
safeDir- If file exists, the persisted
DataDiris re-validated (lines 64-69)The re-validation at lines 64-69 is defense-in-depth against malicious config files containing relative paths — good practice.
73-88: Save correctly normalizes and validates before persisting.The flow ensures:
- DataDir is validated and cleaned via SecurePath
- The canonical path is stored back into
s.DataDir- Directory creation and file write use the validated path
The
//nolint:goseccomments are appropriate since the paths are validated by SecurePath. The CodeQL alerts on lines 81 and 88 are false positives.cli/cmd/update.go (2)
96-99: Proper integration of safeStateDir for path validation.The
safeDiris obtained early and error handling is correct. This validated path is then consistently used for all compose operations.
108-140: Consistent use of validated safeDir throughout compose operations.All
composeRunandComposeExecOutputcalls properly use the validatedsafeDirinstead of the rawstate.DataDir.cli/cmd/uninstall.go (1)
64-67: stopAndRemoveVolumes correctly validates path before compose operations.Path validation via
SecurePathis applied before thecomposeRuncall.Also applies to: 92-92
cli/internal/diagnostics/collect.go (2)
49-53: Appropriate error handling for path validation in diagnostics.The error is captured in the report's Errors field and the function returns early, preventing further operations with an invalid path. This graceful degradation is appropriate for a diagnostic collection function.
64-69: Consistent use of validated safeDir for all path-dependent operations.Container inspection, log collection, and disk info all correctly use the validated
safeDir.Also applies to: 104-104
cli/cmd/status.go (2)
43-47: Proper path validation before compose file existence check.The
safeDiris obtained viasafeStateDir(state)and used to construct thecomposePath. The CodeQL alert on line 44 is a false positive —safeStateDirinternally validates viaSecurePath.
74-86: Clean refactor: functions now accept validated dataDir directly.Changing from
state config.StatetodataDir stringreduces coupling — these functions only need the path, not the entire state. This aligns with the principle of passing minimal required data.cli/cmd/init.go (2)
183-207: writeInitFiles correctly validates and normalizes paths.The flow:
- Validates
state.DataDirvia SecurePath- Updates
state.DataDirto the canonical form (line 188)- Creates directory using validated path
- Writes compose file to validated path
- Saves state (which will use the normalized DataDir)
The CodeQL alert on line 189 is a false positive —
safeDiris validated by SecurePath.
235-242: fileExists now correctly uses SecurePath validation.This addresses the previous review feedback. If the path is invalid (non-absolute), the function returns
falsewhich is a safe fallback. The CodeQL alert on line 240 is a false positive.
|
Superseded — reopening with additional review fixes in a new PR. |
## Summary Resolves all 11 CodeQL `go/path-injection` alerts plus reviewer findings from two review rounds (6 reviewers: go-reviewer, security-reviewer, CodeRabbit, Copilot, Gemini, Greptile). - Add `config.SecurePath()` — validates absolute + `filepath.Clean` (with trust boundary doc comment) - Add `safeStateDir()` cmd helper for consistent usage across all commands - Apply at **all** filesystem call sites: `EnsureDir`, `Load`, `Save`, and all cmd files (`start`, `stop`, `logs`, `doctor`, `status`, `uninstall`, `init`, `update`) - Fix `diagnostics/collect.go` — don't early-return on path error (health/config collection still useful) - Fix `status.go` — thread `safeDir` through helper functions instead of passing raw `state` - Remove dead redundant validation in `state.go` Load - Use `SecurePath` consistently for loaded DataDir validation - Fix `DataDir()` fallback from "." to `os.Getwd()` (absolute path) - Add 5 unit tests for `SecurePath` - Fix SKILL.md markdown lint Supersedes #411 (closed). ## Test plan - [x] `go vet ./...` passes - [x] `go build ./...` passes - [x] `go test ./...` — all tests pass (including new SecurePath tests) - [x] All 11 CodeQL alert locations + 4 additional missed call sites covered - [x] Two rounds of review feedback addressed (18 findings total)
Summary
Resolves all 11 open CodeQL
go/path-injectionalerts. These flag filesystem operations using paths derived from environment variables or config files without explicit sanitization at the point of use.config.SecurePath()helper — validates path is absolute + appliesfilepath.Clean()(satisfies CodeQL taint tracking)SecurePathat all filesystem call sites:EnsureDir,Load,Save, and all cmd files (start,stop,logs,doctor,status,uninstall,init)safeStateDir()cmd helper to avoid repetitionAlso includes: post-merge-cleanup skill update (use individual
git branch -Dinstead of pipedxargs)Test plan
go vet ./...passesgo build ./...passesgo test ./...— all tests passCodeQL alerts addressed
cli/internal/config/paths.gocli/internal/config/state.gocli/cmd/logs.gocli/cmd/init.gocli/cmd/start.gocli/cmd/stop.gocli/cmd/doctor.gocli/cmd/status.gocli/cmd/uninstall.go