From ce7242ae0cbb5c4b2bb49dc32feca0d177394ea5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 21 Jul 2026 08:00:39 +0000 Subject: [PATCH] fix(security): resolve CodeQL code-scanning alerts Address the open CodeQL findings on main: - Path traversal (High): confine the untrusted `dir`/`cwd` query parameter of the permission-catalog handler to the workspace root via resolveCatalogDir, rejecting `../` and absolute escapes before any filesystem access. - Allocation size overflow (High): route make() size/capacity hints that add two lengths through a new collections.SafeAdd helper that saturates at math.MaxInt instead of wrapping (fuzzy matrix, model candidates, and the prompt-entity merge helpers). - Shell command from environment values (Medium): build the git command in vite.config.ts with execFileSync + an argv array so no shell is spawned. - Workflow permissions (Medium): add a least-privilege top-level `permissions: contents: read` block to the Test workflow. Adds a resolveCatalogDir unit test covering the traversal cases. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_0186pQzdDXNY1jsbozM7Ed3B --- .github/workflows/test.yml | 3 +++ pkg/api/model.go | 4 ++- pkg/cli/permission_catalog.go | 40 +++++++++++++++++++++++++----- pkg/cli/permission_catalog_test.go | 29 ++++++++++++++++++++++ pkg/cli/prompt_entity.go | 9 ++++--- pkg/cli/webapp/vite.config.ts | 12 +++++---- pkg/collections/alloc.go | 16 ++++++++++++ pkg/collections/fuzzy.go | 4 +-- 8 files changed, 99 insertions(+), 18 deletions(-) create mode 100644 pkg/collections/alloc.go diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 009646e..dea2e5f 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -6,6 +6,9 @@ on: pull_request: workflow_dispatch: +permissions: + contents: read + jobs: lint: runs-on: ubuntu-latest diff --git a/pkg/api/model.go b/pkg/api/model.go index b347800..c22ac42 100644 --- a/pkg/api/model.go +++ b/pkg/api/model.go @@ -3,6 +3,8 @@ package api import ( "fmt" "strings" + + "github.com/flanksource/captain/pkg/collections" ) // Model identifies which LLM serves a request plus the per-request inference @@ -120,7 +122,7 @@ func (m Model) Candidates() []Model { m = m.ExpandCSV() primary := m primary.Fallbacks = nil - out := make([]Model, 0, 1+len(m.Fallbacks)) + out := make([]Model, 0, collections.SafeAdd(1, len(m.Fallbacks))) out = append(out, primary) for _, fb := range m.Fallbacks { fb.Fallbacks = nil diff --git a/pkg/cli/permission_catalog.go b/pkg/cli/permission_catalog.go index 30978be..da3c048 100644 --- a/pkg/cli/permission_catalog.go +++ b/pkg/cli/permission_catalog.go @@ -2,6 +2,7 @@ package cli import ( "encoding/json" + "fmt" "net/http" "os" "path/filepath" @@ -17,19 +18,46 @@ func handlePermissionCatalog(baseCwd string) http.HandlerFunc { if dir == "" { dir = strings.TrimSpace(r.URL.Query().Get("cwd")) } - if dir == "" { - dir = baseCwd - } - if !filepath.IsAbs(dir) { - dir = filepath.Join(baseCwd, dir) + resolved, err := resolveCatalogDir(baseCwd, dir) + if err != nil { + http.Error(w, "invalid dir", http.StatusBadRequest) + return } w.Header().Set("Content-Type", "application/json") - if err := json.NewEncoder(w).Encode(buildPermissionCatalog(dir)); err != nil { + if err := json.NewEncoder(w).Encode(buildPermissionCatalog(resolved)); err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) } } } +// resolveCatalogDir resolves the caller-supplied directory against baseCwd and +// guarantees the result stays within baseCwd. The dir value comes straight from +// an untrusted query parameter, so it must be confined to the workspace root to +// prevent path traversal (e.g. "../../etc") into arbitrary parts of the +// filesystem. +func resolveCatalogDir(baseCwd, dir string) (string, error) { + base, err := filepath.Abs(baseCwd) + if err != nil { + return "", err + } + base = filepath.Clean(base) + + if dir == "" { + return base, nil + } + + target := dir + if !filepath.IsAbs(target) { + target = filepath.Join(base, target) + } + target = filepath.Clean(target) + + if target != base && !strings.HasPrefix(target, base+string(os.PathSeparator)) { + return "", fmt.Errorf("dir %q escapes workspace root", dir) + } + return target, nil +} + func buildPermissionCatalog(dir string) api.PermissionCatalog { home, _ := os.UserHomeDir() catalog := api.PermissionCatalog{ diff --git a/pkg/cli/permission_catalog_test.go b/pkg/cli/permission_catalog_test.go index 25723e1..cb7ceed 100644 --- a/pkg/cli/permission_catalog_test.go +++ b/pkg/cli/permission_catalog_test.go @@ -46,6 +46,35 @@ func TestBuildPermissionCatalog(t *testing.T) { } } +func TestResolveCatalogDir(t *testing.T) { + base := t.TempDir() + nested := filepath.Join(base, "sub", "child") + if err := os.MkdirAll(nested, 0o755); err != nil { + t.Fatalf("mkdir nested: %v", err) + } + + // Empty dir resolves to the workspace root. + if got, err := resolveCatalogDir(base, ""); err != nil || got != filepath.Clean(base) { + t.Fatalf("empty dir: got %q err %v, want %q", got, err, filepath.Clean(base)) + } + + // Relative paths that stay inside the workspace are allowed. + if got, err := resolveCatalogDir(base, filepath.Join("sub", "child")); err != nil || got != nested { + t.Fatalf("relative dir: got %q err %v, want %q", got, err, nested) + } + + // Traversal attempts must be rejected. + for _, dir := range []string{ + "../../etc", + filepath.Join("sub", "..", "..", "etc"), + "/etc", + } { + if got, err := resolveCatalogDir(base, dir); err == nil { + t.Fatalf("expected %q to be rejected, got %q", dir, got) + } + } +} + func mustWrite(t *testing.T, path, data string) { t.Helper() if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { diff --git a/pkg/cli/prompt_entity.go b/pkg/cli/prompt_entity.go index 14497bf..b712b04 100644 --- a/pkg/cli/prompt_entity.go +++ b/pkg/cli/prompt_entity.go @@ -23,6 +23,7 @@ import ( promptlib "github.com/flanksource/captain/pkg/ai/prompt" "github.com/flanksource/captain/pkg/api" "github.com/flanksource/captain/pkg/captainconfig" + "github.com/flanksource/captain/pkg/collections" "github.com/flanksource/clicky" clickyapi "github.com/flanksource/clicky/api" clickyrpc "github.com/flanksource/clicky/rpc" @@ -606,7 +607,7 @@ func mergeStringMaps(base, overlay map[string]string) map[string]string { if len(overlay) == 0 { return base } - out := make(map[string]string, len(base)+len(overlay)) + out := make(map[string]string, collections.SafeAdd(len(base), len(overlay))) for k, v := range base { out[k] = v } @@ -620,7 +621,7 @@ func mergeToolModes(base, overlay map[string]api.ToolMode) map[string]api.ToolMo if len(overlay) == 0 { return base } - out := make(map[string]api.ToolMode, len(base)+len(overlay)) + out := make(map[string]api.ToolMode, collections.SafeAdd(len(base), len(overlay))) for k, v := range base { out[k] = v } @@ -634,8 +635,8 @@ func mergePresets(base, overlay []api.Preset) []api.Preset { if len(overlay) == 0 { return base } - seen := make(map[api.Preset]bool, len(base)+len(overlay)) - out := make([]api.Preset, 0, len(base)+len(overlay)) + seen := make(map[api.Preset]bool, collections.SafeAdd(len(base), len(overlay))) + out := make([]api.Preset, 0, collections.SafeAdd(len(base), len(overlay))) for _, preset := range base { if seen[preset] { continue diff --git a/pkg/cli/webapp/vite.config.ts b/pkg/cli/webapp/vite.config.ts index 4a4094f..aafc393 100644 --- a/pkg/cli/webapp/vite.config.ts +++ b/pkg/cli/webapp/vite.config.ts @@ -1,5 +1,5 @@ import path from "node:path"; -import { execSync } from "node:child_process"; +import { execFileSync } from "node:child_process"; import { readFileSync } from "node:fs"; import tailwindcss from "@tailwindcss/vite"; import react from "@vitejs/plugin-react"; @@ -103,10 +103,10 @@ function clickySourceAliases(enabled: boolean) { function clickyVersionDefines() { const version = readClickyPackageVersion(); return { - __CLICKY_COMMIT__: JSON.stringify(git("rev-parse --short HEAD", "")), + __CLICKY_COMMIT__: JSON.stringify(git(["rev-parse", "--short", "HEAD"], "")), __CLICKY_TAG__: JSON.stringify(`clicky-ui@${version}`), __CLICKY_DATE__: JSON.stringify(new Date().toISOString()), - __CLICKY_DIRTY__: JSON.stringify(git("status --porcelain", "").length > 0), + __CLICKY_DIRTY__: JSON.stringify(git(["status", "--porcelain"], "").length > 0), }; } @@ -120,9 +120,11 @@ function readClickyPackageVersion() { } } -function git(args: string, fallback: string) { +function git(args: string[], fallback: string) { try { - return execSync(`git -C ${JSON.stringify(clickyPackageRoot)} ${args}`, { + // Pass arguments as an argv array via execFileSync so no shell is spawned, + // avoiding shell interpretation of the (filesystem-derived) repo path. + return execFileSync("git", ["-C", clickyPackageRoot, ...args], { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"], }).trim(); diff --git a/pkg/collections/alloc.go b/pkg/collections/alloc.go new file mode 100644 index 0000000..1c47165 --- /dev/null +++ b/pkg/collections/alloc.go @@ -0,0 +1,16 @@ +package collections + +import "math" + +// SafeAdd returns a + b, saturating at math.MaxInt if the sum would overflow. +// +// It is intended for computing make() size or capacity hints from independent +// lengths (e.g. len(a)+len(b)) without risking an integer-overflow wraparound, +// which could otherwise yield a tiny allocation followed by out-of-bounds +// writes. Negative operands are treated as an overflow guard as well. +func SafeAdd(a, b int) int { + if a < 0 || b < 0 || a > math.MaxInt-b { + return math.MaxInt + } + return a + b +} diff --git a/pkg/collections/fuzzy.go b/pkg/collections/fuzzy.go index 463cd8e..21d43fa 100644 --- a/pkg/collections/fuzzy.go +++ b/pkg/collections/fuzzy.go @@ -39,9 +39,9 @@ func Levenshtein(s1, s2 string) int { return len(s1) } - matrix := make([][]int, len(s1)+1) + matrix := make([][]int, SafeAdd(len(s1), 1)) for i := range matrix { - matrix[i] = make([]int, len(s2)+1) + matrix[i] = make([]int, SafeAdd(len(s2), 1)) matrix[i][0] = i } for j := range len(s2) + 1 {