diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 96200b9ff..76626f2e9 100644 --- a/pkg/tools/shell.go +++ b/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 { diff --git a/pkg/tools/skills_install.go b/pkg/tools/skills_install.go index 6bc9e5eca..74585adb6 100644 --- a/pkg/tools/skills_install.go +++ b/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" - for _, pattern := range t.denyWritePaths { - if pattern.MatchString(relativePath) { - return ErrorResult("access denied: cannot write to skills directory") + pathsToCheck := []string{"skills", filepath.Join("skills", slug)} + for _, path := range pathsToCheck { + for _, pattern := range t.denyWritePaths { + if pattern.MatchString(path) { + return ErrorResult(fmt.Sprintf("access denied: cannot write to %q", path)) + } } } } diff --git a/web/backend/api/skills.go b/web/backend/api/skills.go index 329225ce6..4bc9d352e 100644 --- a/web/backend/api/skills.go +++ b/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 !cfg.Tools.IsToolEnabled(toolName) { - return fmt.Errorf("%s is disabled", toolName) + 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 }