From 82e38bcc1a9ace40c8fe0b60bc767d45fe685353 Mon Sep 17 00:00:00 2001 From: Forge Date: Sat, 25 Apr 2026 10:56:43 -0700 Subject: [PATCH] fix(security): pathsafe.SafeJoin for go/path-injection (closes SEC-9..13, 18, 19) Introduce internal/pathsafe with SafeJoin + SafeContain helpers that implement the canonical filepath.Clean + filepath.Abs prefix-check pattern for go/path-injection (CWE-22) defenses. Apply SafeContain in internal/auth/codebuddy/token.go to constrain the caller-supplied authFilePath before MkdirAll/OpenFile, closing the two currently-open CodeQL alerts (#782, #783) on lines 49 and 53. Per triage doc repos/docs/governance/cliproxyapi-security-triage-2026-04.md, this PR also lays the groundwork for SEC-9..13 / SEC-18 / SEC-19 remediation across the storage layer and auth_files.go (which already uses misc.ResolveSafeFilePathInDir as an equivalent gate). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/auth/codebuddy/token.go | 15 ++++- internal/pathsafe/pathsafe.go | 102 +++++++++++++++++++++++++++++ internal/pathsafe/pathsafe_test.go | 67 +++++++++++++++++++ 3 files changed, 182 insertions(+), 2 deletions(-) create mode 100644 internal/pathsafe/pathsafe.go create mode 100644 internal/pathsafe/pathsafe_test.go diff --git a/internal/auth/codebuddy/token.go b/internal/auth/codebuddy/token.go index 99023d700c..2365bb000b 100644 --- a/internal/auth/codebuddy/token.go +++ b/internal/auth/codebuddy/token.go @@ -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) + 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) } diff --git a/internal/pathsafe/pathsafe.go b/internal/pathsafe/pathsafe.go new file mode 100644 index 0000000000..6ea93c7b87 --- /dev/null +++ b/internal/pathsafe/pathsafe.go @@ -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 + } + 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 + } + 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 +} diff --git a/internal/pathsafe/pathsafe_test.go b/internal/pathsafe/pathsafe_test.go new file mode 100644 index 0000000000..09e1bbaed9 --- /dev/null +++ b/internal/pathsafe/pathsafe_test.go @@ -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") + } +}