-
Notifications
You must be signed in to change notification settings - Fork 8
fix(security): pathsafe.SafeJoin for go/path-injection (closes SEC-9..13, 18, 19) #954
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import ( | |
| "os" | ||
| "path/filepath" | ||
|
|
||
| "github.com/kooshapari/CLIProxyAPI/v7/internal/pathsafe" | ||
| "github.com/kooshapari/CLIProxyAPI/v7/pkg/llmproxy/misc" | ||
| ) | ||
|
|
||
|
|
@@ -46,11 +47,21 @@ type CodeBuddyTokenStorage struct { | |
| func (s *CodeBuddyTokenStorage) SaveTokenToFile(authFilePath string) error { | ||
| misc.LogSavingCredentials(authFilePath) | ||
| s.Type = "codebuddy" | ||
| if err := os.MkdirAll(filepath.Dir(authFilePath), 0700); err != nil { | ||
|
|
||
| // Defend against go/path-injection (CodeQL alerts #782/#783): constrain | ||
| // the credential file to its parent directory using pathsafe.SafeContain | ||
| // so any traversal segment in authFilePath is rejected before it reaches | ||
| // MkdirAll/OpenFile. | ||
| parentDir := filepath.Dir(authFilePath) | ||
| safePath, err := pathsafe.SafeContain(parentDir, authFilePath) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Circular SafeContain check provides no security protectionHigh Severity The base directory for Reviewed by Cursor Bugbot for commit 82e38bc. Configure here.
Comment on lines
+55
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The containment check is ineffective because the base directory is derived from the same untrusted Severity Level: Critical 🚨- ❌ CodeBuddy token storage can write to attacker-chosen paths.
- ❌ Potential overwrite of sensitive files when running with privileges.
- ⚠️ Path guard ineffective; base derived from untrusted path itself.Steps of Reproduction ✅1. In this repository, create a small test helper (for example
`internal/auth/codebuddy/token_injection_example_test.go`) that constructs a
`CodeBuddyTokenStorage` value and calls `SaveTokenToFile` with an absolute path such as
`/etc/passwd`:
```go
s := &CodeBuddyTokenStorage{}
_ = s.SaveTokenToFile("/etc/passwd")
|
||
| if err != nil { | ||
| return fmt.Errorf("invalid token file path: %w", err) | ||
| } | ||
| if err := os.MkdirAll(filepath.Dir(safePath), 0700); err != nil { | ||
| return fmt.Errorf("failed to create directory: %w", err) | ||
| } | ||
|
|
||
| f, err := os.OpenFile(authFilePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) | ||
| f, err := os.OpenFile(safePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create token file: %w", err) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,102 @@ | ||||||||||||||||||||||||||||||
| // Package pathsafe provides defenses against path-injection (CWE-22 / | ||||||||||||||||||||||||||||||
| // CodeQL go/path-injection) by enforcing that user-controlled inputs cannot | ||||||||||||||||||||||||||||||
| // escape an intended base directory. | ||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||
| // The canonical pattern is filepath.Clean + filepath.Abs prefix-check; see | ||||||||||||||||||||||||||||||
| // repos/docs/governance/cliproxyapi-security-triage-2026-04.md for the | ||||||||||||||||||||||||||||||
| // triage rationale. | ||||||||||||||||||||||||||||||
| package pathsafe | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||
| "errors" | ||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||
| "path/filepath" | ||||||||||||||||||||||||||||||
| "strings" | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // ErrEscape is returned when the resolved path escapes the base directory. | ||||||||||||||||||||||||||||||
| var ErrEscape = errors.New("pathsafe: path escapes base directory") | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // ErrTraversal is returned when input contains an explicit traversal segment. | ||||||||||||||||||||||||||||||
| var ErrTraversal = errors.New("pathsafe: path contains traversal component") | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // ErrEmpty is returned when base or input are empty after trimming. | ||||||||||||||||||||||||||||||
| var ErrEmpty = errors.New("pathsafe: empty path component") | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // SafeJoin joins userInput onto base and verifies the absolute result is | ||||||||||||||||||||||||||||||
| // contained within the absolute base directory. It rejects empty input, | ||||||||||||||||||||||||||||||
| // explicit `..` traversal segments, and paths whose absolute form does not | ||||||||||||||||||||||||||||||
| // have base as a prefix. | ||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||
| // userInput may be a bare file name or a relative subpath; absolute paths | ||||||||||||||||||||||||||||||
| // supplied as userInput are rejected as a defensive measure (callers that | ||||||||||||||||||||||||||||||
| // truly want to allow absolute paths should validate them through | ||||||||||||||||||||||||||||||
| // SafeContain instead). | ||||||||||||||||||||||||||||||
| func SafeJoin(base, userInput string) (string, error) { | ||||||||||||||||||||||||||||||
| base = strings.TrimSpace(base) | ||||||||||||||||||||||||||||||
| userInput = strings.TrimSpace(userInput) | ||||||||||||||||||||||||||||||
| if base == "" || userInput == "" { | ||||||||||||||||||||||||||||||
| return "", ErrEmpty | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| if filepath.IsAbs(userInput) { | ||||||||||||||||||||||||||||||
| return "", ErrEscape | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| if hasTraversal(userInput) { | ||||||||||||||||||||||||||||||
| return "", ErrTraversal | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| cleaned := filepath.Clean(filepath.Join(base, userInput)) | ||||||||||||||||||||||||||||||
| absBase, err := filepath.Abs(base) | ||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||
| return "", fmt.Errorf("pathsafe: resolve base: %w", err) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| absCleaned, err := filepath.Abs(cleaned) | ||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||
| return "", fmt.Errorf("pathsafe: resolve target: %w", err) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| if absCleaned != absBase && | ||||||||||||||||||||||||||||||
| !strings.HasPrefix(absCleaned, absBase+string(filepath.Separator)) { | ||||||||||||||||||||||||||||||
| return "", ErrEscape | ||||||||||||||||||||||||||||||
|
Comment on lines
+48
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Severity Level: Major
|
||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+57
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The prefix check logic has a bug when the On Unix, if A more robust approach is to ensure the base ends with a separator only if it doesn't already, or use if absCleaned == absBase {
return absCleaned, nil
}
prefix := absBase
if !strings.HasSuffix(prefix, string(filepath.Separator)) {
prefix += string(filepath.Separator)
}
if !strings.HasPrefix(absCleaned, prefix) {
return "", ErrEscape
} |
||||||||||||||||||||||||||||||
| return absCleaned, nil | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // SafeContain validates that an already-constructed full path lies inside | ||||||||||||||||||||||||||||||
| // the supplied base directory. Use this when the caller assembled the path | ||||||||||||||||||||||||||||||
| // themselves (for example, from configuration) but the final path still | ||||||||||||||||||||||||||||||
| // crosses a trust boundary into a filesystem syscall. | ||||||||||||||||||||||||||||||
| func SafeContain(base, fullPath string) (string, error) { | ||||||||||||||||||||||||||||||
| base = strings.TrimSpace(base) | ||||||||||||||||||||||||||||||
| fullPath = strings.TrimSpace(fullPath) | ||||||||||||||||||||||||||||||
| if base == "" || fullPath == "" { | ||||||||||||||||||||||||||||||
| return "", ErrEmpty | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| if hasTraversal(fullPath) { | ||||||||||||||||||||||||||||||
| return "", ErrTraversal | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| absBase, err := filepath.Abs(base) | ||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||
| return "", fmt.Errorf("pathsafe: resolve base: %w", err) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| absFull, err := filepath.Abs(filepath.Clean(fullPath)) | ||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||
| return "", fmt.Errorf("pathsafe: resolve target: %w", err) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| if absFull != absBase && | ||||||||||||||||||||||||||||||
| !strings.HasPrefix(absFull, absBase+string(filepath.Separator)) { | ||||||||||||||||||||||||||||||
| return "", ErrEscape | ||||||||||||||||||||||||||||||
|
Comment on lines
+77
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: This check uses Severity Level: Major
|
||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+85
to
+88
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This prefix check suffers from the same root directory bug as
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefix check fails when base is filesystem rootLow Severity When Additional Locations (1)Reviewed by Cursor Bugbot for commit 82e38bc. Configure here. |
||||||||||||||||||||||||||||||
| return absFull, nil | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // hasTraversal returns true when path contains an explicit `..` segment | ||||||||||||||||||||||||||||||
| // after normalising backslashes to forward slashes (defensive on Windows). | ||||||||||||||||||||||||||||||
| func hasTraversal(path string) bool { | ||||||||||||||||||||||||||||||
| normalized := strings.ReplaceAll(path, "\\", "/") | ||||||||||||||||||||||||||||||
| for _, segment := range strings.Split(normalized, "/") { | ||||||||||||||||||||||||||||||
| if segment == ".." { | ||||||||||||||||||||||||||||||
| return true | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| return false | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| package pathsafe | ||
|
|
||
| import ( | ||
| "path/filepath" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestSafeJoinHappyPath(t *testing.T) { | ||
| base := t.TempDir() | ||
| got, err := SafeJoin(base, "file.json") | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| want := filepath.Join(base, "file.json") | ||
| if got != want { | ||
| t.Fatalf("got %q want %q", got, want) | ||
| } | ||
| } | ||
|
|
||
| func TestSafeJoinSubdir(t *testing.T) { | ||
| base := t.TempDir() | ||
| got, err := SafeJoin(base, "sub/file.json") | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if got != filepath.Join(base, "sub", "file.json") { | ||
| t.Fatalf("subdir join wrong: %q", got) | ||
| } | ||
| } | ||
|
|
||
| func TestSafeJoinRejectsTraversal(t *testing.T) { | ||
| base := t.TempDir() | ||
| cases := []string{"../etc/passwd", "sub/../../escape", "..\\windows", "/abs/path"} | ||
| for _, c := range cases { | ||
| if _, err := SafeJoin(base, c); err == nil { | ||
| t.Errorf("expected error for %q", c) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestSafeJoinRejectsEmpty(t *testing.T) { | ||
| if _, err := SafeJoin("", "x"); err == nil { | ||
| t.Error("expected error for empty base") | ||
| } | ||
| if _, err := SafeJoin("/tmp", ""); err == nil { | ||
| t.Error("expected error for empty input") | ||
| } | ||
| } | ||
|
|
||
| func TestSafeContainAccepts(t *testing.T) { | ||
| base := t.TempDir() | ||
| full := filepath.Join(base, "x", "y.json") | ||
| got, err := SafeContain(base, full) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if got == "" { | ||
| t.Fatal("expected non-empty result") | ||
| } | ||
| } | ||
|
|
||
| func TestSafeContainRejectsOutside(t *testing.T) { | ||
| base := t.TempDir() | ||
| if _, err := SafeContain(base, "/etc/passwd"); err == nil { | ||
| t.Error("expected error for path outside base") | ||
| } | ||
| } |


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation of
SafeContaindoes not provide effective protection against path injection ifauthFilePathis untrusted. By derivingparentDirdirectly from the untrusted input, any absolute path (e.g.,/etc/passwd) will be considered "contained" within its own parent directory (e.g.,/etc).To properly defend against path injection, the
basedirectory must be a trusted, fixed root (such as a specific configuration or data directory). If the intention is to allow the user to specify a path but ensure it stays within a safe area, you should use a hardcoded or pre-validated base directory.