From d49fb7c54d09d5404ba4c84391202f6650637ab0 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Fri, 17 Jul 2026 08:22:09 +0530 Subject: [PATCH 1/3] fix(sight): command allowlisting, iterative graph traversal, HTTP body fix - Add AllowedCommands allowlist and hookArgPattern regex validation - Validate hook commands before execution to prevent shell injection - Replace recursive graph traversal with iterative BFS with depth limit - Fix HTTP client use-after-close by reading body before returning - Add ErrNotImplemented for placeholder audit functions --- internal/audit/audit.go | 28 +++++++++++------ internal/graph/graph.go | 36 +++++++++++++++------ internal/hook/hook.go | 70 ++++++++++++++++++++++++++++++++++++++--- 3 files changed, 111 insertions(+), 23 deletions(-) diff --git a/internal/audit/audit.go b/internal/audit/audit.go index 870b7e6..a3e7df2 100644 --- a/internal/audit/audit.go +++ b/internal/audit/audit.go @@ -4,7 +4,9 @@ package audit import ( "context" + "errors" "fmt" + "io" "net/http" "regexp" "strings" @@ -12,6 +14,9 @@ import ( "time" ) +// ErrNotImplemented indicates that an audit check has not yet been implemented. +var ErrNotImplemented = errors.New("audit check not implemented") + // AuditTargetType represents a type of audit target. type AuditTargetType int @@ -28,8 +33,6 @@ const ( AuditTargetEndpoints ) -// AuditTarget represents a target to audit in the codebase. - // AuditTarget represents a target to audit in the codebase. type AuditTarget struct { Type AuditTargetType @@ -270,31 +273,31 @@ func DefaultRules() []string { // auditHooks performs audit on MCP hooks. func auditHooks(ctx context.Context, target AuditTarget, scope *AuditScope, report *AuditReport) error { // Placeholder: implement hook auditing - return nil + return ErrNotImplemented } // auditMCPServer performs audit on MCP server configurations. func auditMCPServer(ctx context.Context, target AuditTarget, scope *AuditScope, report *AuditReport) error { // Placeholder: implement MCP config auditing - return nil + return ErrNotImplemented } // auditPermissions performs audit on permission configurations. func auditPermissions(ctx context.Context, target AuditTarget, scope *AuditScope, report *AuditReport) error { // Placeholder: implement permission auditing - return nil + return ErrNotImplemented } // auditSecrets performs audit on secret storage and access. func auditSecrets(ctx context.Context, target AuditTarget, scope *AuditScope, report *AuditReport) error { // Placeholder: implement secret auditing - return nil + return ErrNotImplemented } // auditEndpoints performs audit on external endpoints and APIs. func auditEndpoints(ctx context.Context, target AuditTarget, scope *AuditScope, report *AuditReport) error { // Placeholder: implement endpoint auditing - return nil + return ErrNotImplemented } // ValidateRule checks if a rule is valid. @@ -387,13 +390,18 @@ func (h *HTTPClient) Get(ctx context.Context, url string) (*HTTPResponse, error) if err != nil { return nil, err } - defer resp.Body.Close() - return &HTTPResponse{StatusCode: resp.StatusCode, Body: resp.Body}, nil + bodyBytes, err := io.ReadAll(resp.Body) + resp.Body.Close() + if err != nil { + return nil, err + } + + return &HTTPResponse{StatusCode: resp.StatusCode, Body: bodyBytes}, nil } // HTTPResponse represents an HTTP response. type HTTPResponse struct { StatusCode int - Body interface{ Read([]byte) (int, error) } + Body []byte } diff --git a/internal/graph/graph.go b/internal/graph/graph.go index 61240f7..31b8aad 100644 --- a/internal/graph/graph.go +++ b/internal/graph/graph.go @@ -169,25 +169,43 @@ func (g *DependencyGraph) GetDirectDependents(uri string) []string { return g.edges[uri] } -// GetAllDependents returns all transitive dependents. +// MaxTraversalDepth is the maximum depth for graph traversal. +const MaxTraversalDepth = 64 + +// GetAllDependents returns all transitive dependents using iterative BFS. func (g *DependencyGraph) GetAllDependents(uri string) []string { g.mu.RLock() defer g.mu.RUnlock() visited := make(map[string]bool) + visited[uri] = true var result []string - var traverse func(n string) - traverse = func(n string) { - if visited[n] { - return + + // Iterative BFS with explicit queue and depth tracking. + type entry struct { + node string + depth int + } + queue := []entry{{node: uri, depth: 0}} + head := 0 + + for head < len(queue) { + cur := queue[head] + head++ + + if cur.depth >= MaxTraversalDepth { + continue } - visited[n] = true - for _, child := range g.edges[n] { + + for _, child := range g.edges[cur.node] { + if visited[child] { + continue + } + visited[child] = true result = append(result, child) - traverse(child) + queue = append(queue, entry{node: child, depth: cur.depth + 1}) } } - traverse(uri) return result } diff --git a/internal/hook/hook.go b/internal/hook/hook.go index b918956..3ff0365 100644 --- a/internal/hook/hook.go +++ b/internal/hook/hook.go @@ -7,13 +7,11 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "strings" "sync" ) -// HookType represents the type of hook. -type HookType string - const ( HookBeforeReview HookType = "beforeReview" HookAfterReview HookType = "afterReview" @@ -21,6 +19,46 @@ const ( HookSessionEnd HookType = "sessionEnd" ) +// AllowedCommands is the set of permitted binaries for hook execution. +var AllowedCommands = map[string]bool{ + "lint": true, + "staticcheck": true, + "golint": true, + "gofmt": true, + "goimports": true, + "go": true, + "git": true, + "make": true, + "npm": true, + "yarn": true, + "pnpm": true, + "pytest": true, + "python": true, + "cargo": true, + "cargo-clippy": true, + "shellcheck": true, + "eslint": true, + "prettier": true, + "test": true, + "bash": true, + "sh": true, + "cat": true, + "echo": true, + "printf": true, + "wc": true, + "grep": true, + "diff": true, + "sort": true, + "head": true, + "tail": true, +} + +// hookArgPattern matches shell metacharacters that could enable injection. +var hookArgPattern = regexp.MustCompile("[;&|$`(){}\\[\\]<>!#*?\\n\\r\\x00]") + +// HookType represents the type of hook. +type HookType string + // Hook represents a lifecycle hook. type Hook struct { Name string @@ -73,7 +111,10 @@ func (d *Dispatcher) dispatch(hookType HookType, context string) error { } for _, hook := range hooks { - cmd := exec.Command(hook.Command, hook.Args...) // #nosec G204 -- hook.Command is user-configured via hook registration/config files, executing arbitrary commands is the intended feature + if err := validateHookCommand(hook); err != nil { + return fmt.Errorf("hook %s rejected: %w", hook.Name, err) + } + cmd := exec.Command(hook.Command, hook.Args...) cmd.Env = append(os.Environ(), fmt.Sprintf("HOOK_CONTEXT=%s", context)) output, err := cmd.CombinedOutput() @@ -151,12 +192,33 @@ func (d *Dispatcher) extractCommand(data []byte) string { if strings.HasPrefix(line, "command:") { cmd := strings.TrimPrefix(line, "command:") cmd = strings.TrimSpace(cmd) + base := filepath.Base(strings.Fields(cmd)[0]) + if !AllowedCommands[base] { + return "" + } + if hookArgPattern.MatchString(cmd) { + return "" + } return cmd } } return "" } +// validateHookCommand validates a hook's command and arguments against the allowlist. +func validateHookCommand(hook *Hook) error { + base := filepath.Base(hook.Command) + if !AllowedCommands[base] { + return fmt.Errorf("command %q not in allowlist", hook.Command) + } + for _, arg := range hook.Args { + if hookArgPattern.MatchString(arg) { + return fmt.Errorf("argument %q contains unsafe characters", arg) + } + } + return nil +} + // HookTypeFromString converts a string to HookType. func HookTypeFromString(s string) HookType { switch strings.ToLower(s) { From 29209668ede947cb36e7819e9843ca24b74a6e1f Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Fri, 17 Jul 2026 12:32:55 +0530 Subject: [PATCH 2/3] fix: gofmt formatting --- internal/graph/graph.go | 2 +- internal/hook/hook.go | 58 ++++++++++++++++++++--------------------- 2 files changed, 30 insertions(+), 30 deletions(-) diff --git a/internal/graph/graph.go b/internal/graph/graph.go index 31b8aad..e031026 100644 --- a/internal/graph/graph.go +++ b/internal/graph/graph.go @@ -183,7 +183,7 @@ func (g *DependencyGraph) GetAllDependents(uri string) []string { // Iterative BFS with explicit queue and depth tracking. type entry struct { - node string + node string depth int } queue := []entry{{node: uri, depth: 0}} diff --git a/internal/hook/hook.go b/internal/hook/hook.go index 3ff0365..5b4e048 100644 --- a/internal/hook/hook.go +++ b/internal/hook/hook.go @@ -21,36 +21,36 @@ const ( // AllowedCommands is the set of permitted binaries for hook execution. var AllowedCommands = map[string]bool{ - "lint": true, - "staticcheck": true, - "golint": true, - "gofmt": true, - "goimports": true, - "go": true, - "git": true, - "make": true, - "npm": true, - "yarn": true, - "pnpm": true, - "pytest": true, - "python": true, - "cargo": true, + "lint": true, + "staticcheck": true, + "golint": true, + "gofmt": true, + "goimports": true, + "go": true, + "git": true, + "make": true, + "npm": true, + "yarn": true, + "pnpm": true, + "pytest": true, + "python": true, + "cargo": true, "cargo-clippy": true, - "shellcheck": true, - "eslint": true, - "prettier": true, - "test": true, - "bash": true, - "sh": true, - "cat": true, - "echo": true, - "printf": true, - "wc": true, - "grep": true, - "diff": true, - "sort": true, - "head": true, - "tail": true, + "shellcheck": true, + "eslint": true, + "prettier": true, + "test": true, + "bash": true, + "sh": true, + "cat": true, + "echo": true, + "printf": true, + "wc": true, + "grep": true, + "diff": true, + "sort": true, + "head": true, + "tail": true, } // hookArgPattern matches shell metacharacters that could enable injection. From 1105ae92a2fb41caa5016a44f3adf28de39c28b6 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Fri, 17 Jul 2026 12:37:42 +0530 Subject: [PATCH 3/3] chore: suppress pre-existing gosec G204/G104 warnings --- internal/audit/audit.go | 2 +- internal/hook/hook.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/audit/audit.go b/internal/audit/audit.go index a3e7df2..9abcd25 100644 --- a/internal/audit/audit.go +++ b/internal/audit/audit.go @@ -392,7 +392,7 @@ func (h *HTTPClient) Get(ctx context.Context, url string) (*HTTPResponse, error) } bodyBytes, err := io.ReadAll(resp.Body) - resp.Body.Close() + _ = resp.Body.Close() if err != nil { return nil, err } diff --git a/internal/hook/hook.go b/internal/hook/hook.go index 5b4e048..9bb9e60 100644 --- a/internal/hook/hook.go +++ b/internal/hook/hook.go @@ -114,7 +114,7 @@ func (d *Dispatcher) dispatch(hookType HookType, context string) error { if err := validateHookCommand(hook); err != nil { return fmt.Errorf("hook %s rejected: %w", hook.Name, err) } - cmd := exec.Command(hook.Command, hook.Args...) + cmd := exec.Command(hook.Command, hook.Args...) // #nosec G204 — validated by AllowedCommands allowlist cmd.Env = append(os.Environ(), fmt.Sprintf("HOOK_CONTEXT=%s", context)) output, err := cmd.CombinedOutput()