fix(security): prevent arbitrary file write in machine config (P1+P2)#102
fix(security): prevent arbitrary file write in machine config (P1+P2)#102nvandessel merged 2 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughEnforces tilde-prefixed destinations that expand within the user's home directory and adds validation for expanded paths. Tightens file and directory creation permissions to 0600 (files) and 0700 (directories). Tests updated to use home-backed temp dirs, tilde helpers, and permission assertions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #102 +/- ##
==========================================
+ Coverage 47.99% 48.00% +0.01%
==========================================
Files 107 107
Lines 11718 11721 +3
==========================================
+ Hits 5624 5627 +3
Misses 6094 6094
🚀 New features to boost your workflow:
|
Greptile OverviewGreptile SummaryThis PR addresses critical security vulnerabilities (P1+P2 priority) by preventing arbitrary file write attacks in machine config functionality. Key changes:
The implementation uses a defense-in-depth approach:
Test plan coverage is comprehensive and all security test cases pass. Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| internal/validation/validation.go | New validation package with comprehensive security validators for path traversal, command injection, and flag injection prevention |
| internal/machine/templates.go | Updated expandPath to require ~/ prefix, validate paths with validation.ValidateDestinationPath(), and changed permissions to 0600/0700 |
Sequence Diagram
sequenceDiagram
participant User
participant RenderAndWrite
participant expandPath
participant ValidateDestinationPath
participant FileSystem
User->>RenderAndWrite: Write machine config<br/>(destination: "~/../../etc/shadow")
RenderAndWrite->>expandPath: Expand path
alt Path doesn't start with ~/
expandPath-->>RenderAndWrite: Error: must start with ~/
RenderAndWrite-->>User: Error returned
else Path starts with ~/
expandPath->>expandPath: Get home directory
expandPath->>expandPath: Join home + path[2:]<br/>Clean result
expandPath->>ValidateDestinationPath: Validate(expanded, home)
alt Path escapes home directory
ValidateDestinationPath->>ValidateDestinationPath: filepath.Rel(home, expanded)
ValidateDestinationPath->>ValidateDestinationPath: Check if starts with ".."
ValidateDestinationPath-->>expandPath: Error: escapes base directory
expandPath-->>RenderAndWrite: Error: invalid destination
RenderAndWrite-->>User: Error returned
else Path stays within home
ValidateDestinationPath-->>expandPath: OK
expandPath-->>RenderAndWrite: Valid expanded path
RenderAndWrite->>FileSystem: MkdirAll(parentDir, 0700)
RenderAndWrite->>FileSystem: WriteFile(path, content, 0600)
FileSystem-->>RenderAndWrite: Success
RenderAndWrite-->>User: File created securely
end
end
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| expanded := filepath.Clean(filepath.Join(home, path[2:])) | ||
|
|
||
| if err := validation.ValidateDestinationPath(expanded, home); err != nil { |
There was a problem hiding this comment.
Consider if internal/deps/external.go:304 should use similar validation logic, as it also has an expandPath function that handles ~/ paths but doesn't validate against path traversal.
nvandessel
force-pushed
the
fix/sl1-kss-machine-config-write
branch
from
c202581 to
46095fe
Compare
|
@greptile review |
Greptile OverviewGreptile SummaryThis change hardens machine config rendering/writing by requiring destinations to be specified as The main issue to address before merge is in the tests: several new assertions depend on exact error-message substrings (e.g., Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| internal/machine/templates.go | Tightens destination handling by requiring "~/" and validating expanded paths remain under $HOME; also changes mkdir/file modes to 0700/0600. |
| internal/machine/templates_test.go | Updates tests to use ~/ destinations under $HOME and adds new permission/traversal tests; introduces brittle assertions on specific error message substrings from ValidateDestinationPath. |
Sequence Diagram
sequenceDiagram
participant Caller as CLI/Caller
participant Machine as internal/machine/templates.go
participant OS as os/filesystem
participant Val as internal/validation
Caller->>Machine: RenderAndWrite(mc, values, opts)
Machine->>Machine: RenderMachineConfig(mc, values)
Machine->>Machine: expandPath(mc.Destination)
Machine->>OS: UserHomeDir()
Machine->>Machine: Clean(Join(home, path[2:]))
Machine->>Val: ValidateDestinationPath(expanded, home)
alt invalid path (no "~/" or traversal)
Val-->>Machine: error
Machine-->>Caller: error ("failed to expand..." / "invalid destination...")
else valid path
Val-->>Machine: nil
Machine->>OS: MkdirAll(parentDir, 0700)
Machine->>OS: WriteFile(dest, content, 0600)
Machine-->>Caller: RenderResult
end
| func TestExpandPathRejectsTraversal(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| input string | ||
| }{ | ||
| {name: "parent traversal", input: "~/../../etc/shadow"}, | ||
| {name: "deep traversal", input: "~/../../../tmp/evil"}, | ||
| {name: "single parent", input: "~/.."}, | ||
| {name: "dotdot in middle", input: "~/.config/../../etc/passwd"}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| _, err := expandPath(tt.input) | ||
| if err == nil { | ||
| t.Errorf("expandPath(%q) should have returned error for path traversal", tt.input) | ||
| } | ||
| if err != nil && !strings.Contains(err.Error(), "escapes base directory") { | ||
| t.Errorf("expandPath(%q) error = %q, expected 'escapes base directory' message", tt.input, err.Error()) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Brittle error string assertions
TestExpandPathRejectsTraversal asserts the error contains the exact substring "escapes base directory" (and TestExpandPathRejectsNonTildePaths asserts "must start with ~/"). This will fail as soon as validation.ValidateDestinationPath() changes its message wording, or if expandPath() wraps/augments the error differently. These tests should assert on behavior (i.e., that an error is returned) rather than a particular message text, or at least match on a stable sentinel (e.g., a typed error) if available.
Also appears at internal/machine/templates_test.go:469-476.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Require ~/ prefix for machine config destinations. Validate expanded paths stay within home directory. Change file permissions from 0644 to 0600 (not world-readable). Change directory permissions from 0755 to 0700. Beads: go4dot-sl1, go4dot-kss Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Test behavior (error returned) rather than coupling to specific error message text. This prevents test breakage when error messages are reworded in validation functions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nvandessel
force-pushed
the
fix/sl1-kss-machine-config-write
branch
from
c399d1f to
a2e10ad
Compare
1 participant
Summary
~/prefix for machine config destinationsvalidation.ValidateDestinationPath()Beads
Test plan
make build && make lint && make testpasses🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests