Skip to content

Commit 4023e46

Browse files
authored
fix: harden input validation, path traversal, and sensitive file handling (#230)
* fix(gitlog): validate git references to prevent command injection * fix(pathutil): resolve symlinks for case-insensitive filesystem safety * fix(extensionmgr): skip sensitive files during copy and set working directory Add .env, .secrets, and .key files to skip list during extension copy. Set cmd.Dir to extension directory for script execution.
1 parent 70a848a commit 4023e46

5 files changed

Lines changed: 80 additions & 3 deletions

File tree

internal/extensionmgr/copy.go

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,14 +159,29 @@ var skipNames = map[string]struct{}{
159159
".git": {},
160160
".DS_Store": {},
161161
"node_modules": {},
162+
".env": {},
163+
".secrets": {},
162164
}
163165

166+
// skipSuffixes defines file suffixes to exclude during directory copying.
167+
var skipSuffixes = []string{".key"}
168+
164169
// copyDirFn is kept for backward compatibility during migration.
165170
var copyDirFn = func(src, dst string) error { return defaultFileCopier.CopyDir(src, dst) }
166171

167172
// shouldSkipEntry determines whether a file should be skipped or a directory subtree should be skipped.
168173
func shouldSkipEntry(info os.FileInfo) (skipFile bool, skipDir bool) {
169-
_, skip := skipNames[info.Name()]
174+
name := info.Name()
175+
_, skip := skipNames[name]
176+
if !skip {
177+
// Check suffix-based skip patterns (e.g., .key files)
178+
for _, suffix := range skipSuffixes {
179+
if strings.HasSuffix(name, suffix) {
180+
skip = true
181+
break
182+
}
183+
}
184+
}
170185
if !skip {
171186
return false, false
172187
}

internal/extensionmgr/copy_test.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,9 @@ func TestShouldSkipEntry(t *testing.T) {
198198
{"git directory", ".git", true, false, true},
199199
{"node_modules directory", "node_modules", true, false, true},
200200
{"regular file", "main.go", false, false, false},
201-
{"dotfile", ".env", false, false, false},
201+
{"env file skipped", ".env", false, true, false},
202+
{"secrets file skipped", ".secrets", false, true, false},
203+
{"key file skipped", "server.key", false, true, false},
202204
{"unknown dir", "config", true, false, false},
203205
}
204206

internal/extensionmgr/executor.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,9 @@ func (e *ScriptExecutor) Execute(ctx context.Context, scriptPath string, input *
102102
execCtx, cancel := context.WithTimeout(ctx, e.Timeout)
103103
defer cancel()
104104

105-
// Prepare command
105+
// Prepare command with working directory set to the script's directory
106106
cmd := exec.CommandContext(execCtx, absPath)
107+
cmd.Dir = filepath.Dir(absPath)
107108
cmd.Stdin = bytes.NewReader(inputJSON)
108109

109110
var stdout, stderr bytes.Buffer

internal/pathutil/pathutil.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,31 @@ import (
77
"github.com/indaco/sley/internal/apperrors"
88
)
99

10+
// resolveSymlinks attempts to resolve symlinks for both paths.
11+
// On case-insensitive filesystems (e.g., macOS), /var may be a symlink to /private/var.
12+
// To avoid false mismatches, both paths are resolved together:
13+
// if either resolution fails, the originals are returned unchanged.
14+
func resolveSymlinks(absBase, absPath string) (string, string) {
15+
resolvedBase, baseErr := filepath.EvalSymlinks(absBase)
16+
if baseErr != nil {
17+
return absBase, absPath
18+
}
19+
20+
// Try resolving the full path first
21+
if resolvedPath, err := filepath.EvalSymlinks(absPath); err == nil {
22+
return resolvedBase, resolvedPath
23+
}
24+
25+
// Path doesn't exist yet; try resolving parent directory and reattach the filename
26+
parent := filepath.Dir(absPath)
27+
if resolvedParent, err := filepath.EvalSymlinks(parent); err == nil {
28+
return resolvedBase, filepath.Join(resolvedParent, filepath.Base(absPath))
29+
}
30+
31+
// Cannot resolve path side — fall back to both unresolved to avoid mismatch
32+
return absBase, absPath
33+
}
34+
1035
// ValidatePath ensures a path is safe and within expected boundaries.
1136
// It rejects paths with directory traversal attempts and cleans the path.
1237
func ValidatePath(path string, baseDir string) (string, error) {
@@ -29,6 +54,9 @@ func ValidatePath(path string, baseDir string) (string, error) {
2954
return "", &apperrors.PathValidationError{Path: path, Reason: "invalid path"}
3055
}
3156

57+
// Resolve symlinks to normalize paths on case-insensitive filesystems
58+
absBase, absPath = resolveSymlinks(absBase, absPath)
59+
3260
// Check for directory traversal
3361
if !strings.HasPrefix(absPath, absBase+string(filepath.Separator)) && absPath != absBase {
3462
return "", &apperrors.PathValidationError{Path: path, Reason: "path traversal detected"}
@@ -39,6 +67,7 @@ func ValidatePath(path string, baseDir string) (string, error) {
3967
}
4068

4169
// IsWithinDir checks if a path is within a given directory.
70+
// Resolves symlinks to handle case-insensitive filesystems correctly.
4271
func IsWithinDir(path string, dir string) bool {
4372
absPath, err := filepath.Abs(path)
4473
if err != nil {
@@ -50,5 +79,8 @@ func IsWithinDir(path string, dir string) bool {
5079
return false
5180
}
5281

82+
// Resolve symlinks to normalize paths on case-insensitive filesystems
83+
absDir, absPath = resolveSymlinks(absDir, absPath)
84+
5385
return strings.HasPrefix(absPath, absDir+string(filepath.Separator)) || absPath == absDir
5486
}

internal/plugins/commitparser/gitlog/gitlog.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,28 @@ import (
44
"bytes"
55
"fmt"
66
"os/exec"
7+
"regexp"
78
"strings"
89
)
910

11+
// validGitRef matches safe git reference names: alphanumeric, dots, hyphens, slashes, tildes, carets.
12+
// Rejects shell metacharacters, spaces, and ".." sequences within ref names.
13+
var validGitRef = regexp.MustCompile(`^[a-zA-Z0-9._/~^@{}\-]+$`)
14+
15+
// validateGitRef checks that a git reference is safe to use in a revision range.
16+
func validateGitRef(ref string) error {
17+
if ref == "" {
18+
return fmt.Errorf("git reference cannot be empty")
19+
}
20+
if strings.Contains(ref, "..") {
21+
return fmt.Errorf("git reference %q contains invalid '..' sequence", ref)
22+
}
23+
if !validGitRef.MatchString(ref) {
24+
return fmt.Errorf("git reference %q contains invalid characters", ref)
25+
}
26+
return nil
27+
}
28+
1029
var (
1130
GetCommitsFn = getCommits
1231
execCommand = exec.Command
@@ -26,6 +45,14 @@ func getCommits(since string, until string) ([]string, error) {
2645
}
2746
}
2847

48+
// Validate git references to prevent unexpected behavior
49+
if err := validateGitRef(since); err != nil {
50+
return nil, fmt.Errorf("invalid 'since' reference: %w", err)
51+
}
52+
if err := validateGitRef(until); err != nil {
53+
return nil, fmt.Errorf("invalid 'until' reference: %w", err)
54+
}
55+
2956
revRange := since + ".." + until
3057
cmd := execCommand("git", "log", "--pretty=format:%s", revRange)
3158

0 commit comments

Comments
 (0)