mirror of
https://github.com/lukaszraczylo/claude-mnemonic.git
synced 2026-10-20 06:27:50 +00:00
fix(sdk): improve path traversal protection and allocation safety
- [x] Enhance safeResolvePath with stricter validation using filepath.Rel - [x] Reject paths containing ".." after cleaning to prevent traversal - [x] Validate absolute paths are within cwd when cwd is specified - [x] Apply safeResolvePath validation to GetFileContent for consistency - [x] Add comprehensive test coverage for path traversal protection - [x] Fix allocation safety in getRecentSearchQueries by using constant capacity
This commit is contained in:
@@ -784,24 +784,41 @@ func toJSONString(v any) string {
|
|||||||
|
|
||||||
// safeResolvePath resolves a path relative to cwd and validates it doesn't escape the cwd directory.
|
// safeResolvePath resolves a path relative to cwd and validates it doesn't escape the cwd directory.
|
||||||
// Returns the resolved absolute path and true if valid, or empty string and false if path traversal detected.
|
// Returns the resolved absolute path and true if valid, or empty string and false if path traversal detected.
|
||||||
|
// This function is a security sanitizer for path traversal attacks.
|
||||||
func safeResolvePath(path, cwd string) (string, bool) {
|
func safeResolvePath(path, cwd string) (string, bool) {
|
||||||
if filepath.IsAbs(path) {
|
// Clean the input path to normalize any .. or . components
|
||||||
// Absolute paths are allowed as-is
|
cleanPath := filepath.Clean(path)
|
||||||
return filepath.Clean(path), true
|
|
||||||
|
// Reject paths that explicitly contain parent directory traversal after cleaning
|
||||||
|
if strings.Contains(cleanPath, "..") {
|
||||||
|
return "", false
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if filepath.IsAbs(cleanPath) {
|
||||||
|
// For absolute paths, verify they're within cwd if cwd is specified
|
||||||
|
if cwd != "" {
|
||||||
|
cleanCwd := filepath.Clean(cwd)
|
||||||
|
if !strings.HasPrefix(cleanPath, cleanCwd+string(filepath.Separator)) && cleanPath != cleanCwd {
|
||||||
|
return "", false
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return cleanPath, true
|
||||||
|
}
|
||||||
|
|
||||||
if cwd == "" {
|
if cwd == "" {
|
||||||
return filepath.Clean(path), true
|
return cleanPath, true
|
||||||
}
|
}
|
||||||
|
|
||||||
// Clean the cwd first
|
// Clean the cwd first
|
||||||
cleanCwd := filepath.Clean(cwd)
|
cleanCwd := filepath.Clean(cwd)
|
||||||
|
|
||||||
// Join and clean the path
|
// Join and clean the path
|
||||||
absPath := filepath.Clean(filepath.Join(cleanCwd, path))
|
absPath := filepath.Join(cleanCwd, cleanPath)
|
||||||
|
|
||||||
// Verify the resolved path is still within cwd (prevents path traversal via ..)
|
// Use filepath.Rel to verify the path is actually within cwd
|
||||||
// Use HasPrefix on cleaned paths to detect escapes
|
// If Rel returns a path starting with "..", it escapes the base
|
||||||
if !strings.HasPrefix(absPath, cleanCwd+string(filepath.Separator)) && absPath != cleanCwd {
|
rel, err := filepath.Rel(cleanCwd, absPath)
|
||||||
|
if err != nil || strings.HasPrefix(rel, "..") {
|
||||||
return "", false
|
return "", false
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -905,12 +922,13 @@ func GetFileMtimes(paths []string, cwd string) map[string]int64 {
|
|||||||
// GetFileContent reads file content for verification purposes.
|
// GetFileContent reads file content for verification purposes.
|
||||||
// Returns content and ok status.
|
// Returns content and ok status.
|
||||||
func GetFileContent(path, cwd string) (string, bool) {
|
func GetFileContent(path, cwd string) (string, bool) {
|
||||||
absPath := path
|
absPath, ok := safeResolvePath(path, cwd)
|
||||||
if !filepath.IsAbs(path) && cwd != "" {
|
if !ok {
|
||||||
absPath = filepath.Join(cwd, path)
|
// Reject paths that attempt directory traversal
|
||||||
|
return "", false
|
||||||
}
|
}
|
||||||
|
|
||||||
content, err := os.ReadFile(absPath) // #nosec G304 -- intentional file read for verification
|
content, err := os.ReadFile(absPath) // #nosec G304 -- path validated by safeResolvePath
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return "", false
|
return "", false
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1081,3 +1081,100 @@ func BenchmarkSanitizePromptWithControlChars(b *testing.B) {
|
|||||||
sanitizePrompt(prompt)
|
sanitizePrompt(prompt)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestSafeResolvePath tests the path traversal protection.
|
||||||
|
func TestSafeResolvePath(t *testing.T) {
|
||||||
|
// Create a temporary directory for testing
|
||||||
|
tmpDir := t.TempDir()
|
||||||
|
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
path string
|
||||||
|
cwd string
|
||||||
|
wantPath string
|
||||||
|
wantOk bool
|
||||||
|
}{
|
||||||
|
{
|
||||||
|
name: "simple relative path",
|
||||||
|
path: "file.txt",
|
||||||
|
cwd: tmpDir,
|
||||||
|
wantOk: true,
|
||||||
|
wantPath: filepath.Join(tmpDir, "file.txt"),
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "nested relative path",
|
||||||
|
path: "subdir/file.txt",
|
||||||
|
cwd: tmpDir,
|
||||||
|
wantOk: true,
|
||||||
|
wantPath: filepath.Join(tmpDir, "subdir", "file.txt"),
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "path traversal with ..",
|
||||||
|
path: "../etc/passwd",
|
||||||
|
cwd: tmpDir,
|
||||||
|
wantOk: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "path traversal with multiple ..",
|
||||||
|
path: "../../etc/passwd",
|
||||||
|
cwd: tmpDir,
|
||||||
|
wantOk: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "path traversal hidden in middle",
|
||||||
|
path: "subdir/../../../etc/passwd",
|
||||||
|
cwd: tmpDir,
|
||||||
|
wantOk: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "just parent directory",
|
||||||
|
path: "..",
|
||||||
|
cwd: tmpDir,
|
||||||
|
wantOk: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "absolute path without cwd",
|
||||||
|
path: "/some/absolute/path",
|
||||||
|
cwd: "",
|
||||||
|
wantOk: true,
|
||||||
|
wantPath: "/some/absolute/path",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "relative path without cwd",
|
||||||
|
path: "relative/path",
|
||||||
|
cwd: "",
|
||||||
|
wantOk: true,
|
||||||
|
wantPath: "relative/path",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "current directory reference",
|
||||||
|
path: "./file.txt",
|
||||||
|
cwd: tmpDir,
|
||||||
|
wantOk: true,
|
||||||
|
wantPath: filepath.Join(tmpDir, "file.txt"),
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "absolute path outside cwd",
|
||||||
|
path: "/etc/passwd",
|
||||||
|
cwd: tmpDir,
|
||||||
|
wantOk: false,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "absolute path inside cwd",
|
||||||
|
path: filepath.Join(tmpDir, "inside.txt"),
|
||||||
|
cwd: tmpDir,
|
||||||
|
wantOk: true,
|
||||||
|
wantPath: filepath.Join(tmpDir, "inside.txt"),
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
|
gotPath, gotOk := safeResolvePath(tt.path, tt.cwd)
|
||||||
|
assert.Equal(t, tt.wantOk, gotOk, "ok status mismatch")
|
||||||
|
if tt.wantPath != "" && gotOk {
|
||||||
|
assert.Equal(t, tt.wantPath, gotPath, "path mismatch")
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -1443,12 +1443,10 @@ func (s *Service) getRecentSearchQueries(project string, limit int) []RecentSear
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Filter by project (iterate from newest to oldest)
|
// Filter by project (iterate from newest to oldest)
|
||||||
// Cap capacity to maxRecentQueries to prevent excessive allocation from user input
|
// Use constant capacity to prevent excessive allocation from user input
|
||||||
capacity := limit
|
// limit is already bounded to maxRecentQueries above, but we use the constant
|
||||||
if capacity > maxRecentQueries {
|
// directly here to satisfy static analysis tools
|
||||||
capacity = maxRecentQueries
|
result := make([]RecentSearchQuery, 0, maxRecentQueries)
|
||||||
}
|
|
||||||
result := make([]RecentSearchQuery, 0, capacity)
|
|
||||||
for i := 0; i < s.recentQueriesLen; i++ {
|
for i := 0; i < s.recentQueriesLen; i++ {
|
||||||
idx := (s.recentQueriesHead + i) % maxRecentQueries
|
idx := (s.recentQueriesHead + i) % maxRecentQueries
|
||||||
q := s.recentQueriesBuf[idx]
|
q := s.recentQueriesBuf[idx]
|
||||||
|
|||||||
Reference in New Issue
Block a user