Harden security: prevent shell bypasses in exec and synchronize Web API guards for skills

pkg/tools/shell.go

@@ -40,6 +40,7 @@ type ExecTool struct {
 	allowPatterns       []*regexp.Regexp
 	customAllowPatterns []*regexp.Regexp
 	allowedPathPatterns []*regexp.Regexp
+	denyWritePaths      []*regexp.Regexp
 	restrictToWorkspace bool
 	allowRemote         bool
 	sessionManager      *SessionManager
@@ -120,8 +121,18 @@ func NewExecTool(workingDir string, restrict bool, allowPaths ...[]*regexp.Regex
 func NewExecToolWithConfig(
 	workingDir string,
 	restrict bool,
-	config *config.Config,
+	cfg *config.Config,
 	allowPaths ...[]*regexp.Regexp,
+) (*ExecTool, error) {
+	return NewExecToolWithDenyPaths(workingDir, restrict, allowPaths, nil, cfg)
+}
+
+func NewExecToolWithDenyPaths(
+	workingDir string,
+	restrict bool,
+	allowPaths [][]*regexp.Regexp,
+	denyWritePaths []*regexp.Regexp,
+	cfg *config.Config,
 ) (*ExecTool, error) {
 	denyPatterns := make([]*regexp.Regexp, 0)
 	customAllowPatterns := make([]*regexp.Regexp, 0)
@@ -131,8 +142,8 @@ func NewExecToolWithConfig(
 		allowedPathPatterns = allowPaths[0]
 	}
 
-	if config != nil {
-		execConfig := config.Tools.Exec
+	if cfg != nil {
+		execConfig := cfg.Tools.Exec
 		enableDenyPatterns := execConfig.EnableDenyPatterns
 		allowRemote = execConfig.AllowRemote
 		if enableDenyPatterns {
@@ -163,8 +174,8 @@ func NewExecToolWithConfig(
 	}
 
 	var timeout time.Duration
-	if config != nil && config.Tools.Exec.TimeoutSeconds > 0 {
-		timeout = time.Duration(config.Tools.Exec.TimeoutSeconds) * time.Second
+	if cfg != nil && cfg.Tools.Exec.TimeoutSeconds > 0 {
+		timeout = time.Duration(cfg.Tools.Exec.TimeoutSeconds) * time.Second
 	}
 
 	return &ExecTool{
@@ -174,6 +185,7 @@ func NewExecToolWithConfig(
 		allowPatterns:       nil,
 		customAllowPatterns: customAllowPatterns,
 		allowedPathPatterns: allowedPathPatterns,
+		denyWritePaths:      denyWritePaths,
 		restrictToWorkspace: restrict,
 		allowRemote:         allowRemote,
 		sessionManager:      getSessionManager(),
@@ -1033,6 +1045,40 @@ func (t *ExecTool) guardCommand(command, cwd string) string {
 				return "Command blocked by safety guard (dangerous pattern detected)"
 			}
 		}
+
+		// Check deny write paths - block commands that reference protected directories or variables
+		// We perform a broad check on the entire command string to prevent variable bypasses.
+		if len(t.denyWritePaths) > 0 {
+			// First check: literal occurrences in the whole command
+			for _, pattern := range t.denyWritePaths {
+				if pattern.MatchString(cmd) {
+					return fmt.Sprintf("Command blocked: reference to restricted path detected")
+				}
+			}
+
+			// Second check: check individual words/arguments for deeper validation
+			words := strings.Fields(cmd)
+			for _, word := range words {
+				// Clean whitespace and common shell chars from word to find actual path candidates
+				cleanWord := strings.Trim(word, " ;&|><\"'$()")
+				if cleanWord == "" {
+					continue
+				}
+
+				for _, pattern := range t.denyWritePaths {
+					if pattern.MatchString(cleanWord) {
+						return fmt.Sprintf("Command blocked: cannot access protected path %q", cleanWord)
+					}
+					// Also check path components (e.g. "skills" in "mkdir -p skills/foo")
+					pathParts := strings.Split(cleanWord, "/")
+					for _, part := range pathParts {
+						if part != "" && pattern.MatchString(part) {
+							return fmt.Sprintf("Command blocked: cannot access protected path component %q", part)
+						}
+					}
+				}
+			}
+		}
 	}
 
 	if len(t.allowPatterns) > 0 {

pkg/tools/skills_install.go

@@ -115,13 +115,13 @@ func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]any) *To
 	force, _ := args["force"].(bool)
 
 	// Check deny write paths before proceeding with installation.
-	// Patterns are expected to match relative paths (e.g., "skills", "skills/foo"),
-	// so we check against the relative path from workspace.
 	if len(t.denyWritePaths) > 0 {
-		relativePath := "skills"
+		pathsToCheck := []string{"skills", filepath.Join("skills", slug)}
+		for _, path := range pathsToCheck {
 			for _, pattern := range t.denyWritePaths {
-			if pattern.MatchString(relativePath) {
-				return ErrorResult("access denied: cannot write to skills directory")
+				if pattern.MatchString(path) {
+					return ErrorResult(fmt.Sprintf("access denied: cannot write to %q", path))
+				}
 			}
 		}
 	}

web/backend/api/skills.go

@@ -127,9 +127,17 @@ func (h *Handler) handleListSkills(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// Filter based on security policy
+	filtered := make([]skillSupportItem, 0, len(items))
+	for _, item := range items {
+		if ensureSkillRegistryToolEnabled(cfg, "", item.Name) == nil {
+			filtered = append(filtered, item)
+		}
+	}
+
 	w.Header().Set("Content-Type", "application/json")
 	json.NewEncoder(w).Encode(skillSupportResponse{
-		Skills: items,
+		Skills: filtered,
 	})
 }
 
@@ -146,6 +154,12 @@ func (h *Handler) handleGetSkill(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	name := r.PathValue("name")
+
+	if registryErr := ensureSkillRegistryToolEnabled(cfg, "", name); registryErr != nil {
+		http.Error(w, registryErr.Error(), http.StatusBadRequest)
+		return
+	}
+
 	for _, skillItem := range skillItems {
 		if skillItem.Name != name {
 			continue
@@ -174,7 +188,7 @@ func (h *Handler) handleSearchSkills(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, fmt.Sprintf("Failed to load config: %v", loadErr), http.StatusInternalServerError)
 		return
 	}
-	if registryErr := ensureSkillRegistryToolEnabled(cfg, "find_skills"); registryErr != nil {
+	if registryErr := ensureSkillRegistryToolEnabled(cfg, "find_skills", ""); registryErr != nil {
 		http.Error(w, registryErr.Error(), http.StatusBadRequest)
 		return
 	}
@@ -278,17 +292,17 @@ func (h *Handler) handleInstallSkill(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, fmt.Sprintf("Failed to load config: %v", loadErr), http.StatusInternalServerError)
 		return
 	}
-	if registryErr := ensureSkillRegistryToolEnabled(cfg, "install_skill"); registryErr != nil {
-		http.Error(w, registryErr.Error(), http.StatusBadRequest)
-		return
-	}
-
 	var req installSkillRequest
 	if decodeErr := json.NewDecoder(r.Body).Decode(&req); decodeErr != nil {
 		http.Error(w, fmt.Sprintf("Invalid JSON: %v", decodeErr), http.StatusBadRequest)
 		return
 	}
 
+	if registryErr := ensureSkillRegistryToolEnabled(cfg, "install_skill", req.Slug); registryErr != nil {
+		http.Error(w, registryErr.Error(), http.StatusBadRequest)
+		return
+	}
+
 	req.Slug = strings.TrimSpace(req.Slug)
 	req.Registry = strings.TrimSpace(req.Registry)
 	req.Version = strings.TrimSpace(req.Version)
@@ -448,6 +462,11 @@ func (h *Handler) handleImportSkill(w http.ResponseWriter, r *http.Request) {
 	}
 	defer uploadedFile.Close()
 
+	if registryErr := ensureSkillRegistryToolEnabled(cfg, "write_file", fileHeader.Filename); registryErr != nil {
+		http.Error(w, registryErr.Error(), http.StatusBadRequest)
+		return
+	}
+
 	content, err := io.ReadAll(io.LimitReader(uploadedFile, maxImportedSkillSize+1))
 	if err != nil {
 		http.Error(w, fmt.Sprintf("Failed to read file: %v", err), http.StatusBadRequest)
@@ -479,6 +498,11 @@ func (h *Handler) handleDeleteSkill(w http.ResponseWriter, r *http.Request) {
 
 	loader := newSkillsLoader(cfg.WorkspacePath())
 	name := r.PathValue("name")
+
+	if registryErr := ensureSkillRegistryToolEnabled(cfg, "", name); registryErr != nil {
+		http.Error(w, registryErr.Error(), http.StatusBadRequest)
+		return
+	}
 	workspaceSkillWriteMu.Lock()
 	defer workspaceSkillWriteMu.Unlock()
 
@@ -531,13 +555,42 @@ func newSkillsRegistryManager(cfg *config.Config) *skills.RegistryManager {
 	})
 }
 
-func ensureSkillRegistryToolEnabled(cfg *config.Config, toolName string) error {
+func ensureSkillRegistryToolEnabled(cfg *config.Config, toolName string, skillName string) error {
 	if !cfg.Tools.IsToolEnabled("skills") {
 		return fmt.Errorf("tools.skills is disabled")
 	}
+	if toolName != "" {
 		if !cfg.Tools.IsToolEnabled(toolName) {
 			return fmt.Errorf("%s is disabled", toolName)
 		}
+	}
+
+	// Check whitelist for specific skill if enabled
+	if cfg.Tools.Skills.WhitelistEnabled && skillName != "" {
+		allowed := false
+		for _, s := range cfg.Tools.Skills.Whitelist {
+			if s == skillName {
+				allowed = true
+				break
+			}
+		}
+		if !allowed {
+			return fmt.Errorf("skill %q is not in the whitelist", skillName)
+		}
+	}
+
+	// Check deny paths
+	if skillName != "" {
+		// Path would be skills/skillName
+		pathCandidate := filepath.Join("skills", skillName)
+		for _, patternStr := range cfg.Tools.DenyWritePaths {
+			re, err := regexp.Compile(patternStr)
+			if err == nil && re.MatchString(pathCandidate) {
+				return fmt.Errorf("access to skill %q is blocked by security policy", skillName)
+			}
+		}
+	}
+
 	return nil
 }