diff --git a/pkg/workflow/add_comment.go b/pkg/workflow/add_comment.go index 604aabb583..15cf784cc1 100644 --- a/pkg/workflow/add_comment.go +++ b/pkg/workflow/add_comment.go @@ -183,7 +183,7 @@ func (c *Compiler) parseCommentsConfig(outputMap map[string]any) *AddCommentsCon } // Validate target-repo (wildcard "*" is not allowed) - if validateTargetRepoSlug(config.TargetRepoSlug, addCommentLog) { + if err := validateTargetRepoSlug(config.TargetRepoSlug, addCommentLog); err != nil { return nil // Invalid configuration, return nil to cause validation error } diff --git a/pkg/workflow/add_reviewer.go b/pkg/workflow/add_reviewer.go index 243d734689..eadbf7b423 100644 --- a/pkg/workflow/add_reviewer.go +++ b/pkg/workflow/add_reviewer.go @@ -45,7 +45,7 @@ func (c *Compiler) parseAddReviewerConfig(outputMap map[string]any) *AddReviewer } // Validate target-repo (wildcard "*" is not allowed for safe outputs) - if validateTargetRepoSlug(config.TargetRepoSlug, addReviewerLog) { + if err := validateTargetRepoSlug(config.TargetRepoSlug, addReviewerLog); err != nil { return nil } diff --git a/pkg/workflow/agent_validation.go b/pkg/workflow/agent_validation.go index 09af8fed5a..6ad46dd5e4 100644 --- a/pkg/workflow/agent_validation.go +++ b/pkg/workflow/agent_validation.go @@ -153,8 +153,7 @@ func (c *Compiler) validateWebSearchSupport(tools map[string]any, engine CodingA // web-search is specified, check if the engine supports it if !engine.SupportsWebSearch() { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Engine '%s' does not support the web-search tool. See https://github.github.com/gh-aw/guides/web-search/ for alternatives.", engine.GetID()))) - c.IncrementWarningCount() + c.emitWarning(fmt.Sprintf("Engine '%s' does not support the web-search tool. See https://github.github.com/gh-aw/guides/web-search/ for alternatives.", engine.GetID())) } } diff --git a/pkg/workflow/close_entity_helpers.go b/pkg/workflow/close_entity_helpers.go index e00d9f7144..46be93722f 100644 --- a/pkg/workflow/close_entity_helpers.go +++ b/pkg/workflow/close_entity_helpers.go @@ -120,7 +120,7 @@ func (c *Compiler) parseCloseEntityConfig(outputMap map[string]any, params Close } // Validate target-repo (wildcard "*" is not allowed) - if validateTargetRepoSlug(config.TargetRepoSlug, logger) { + if err := validateTargetRepoSlug(config.TargetRepoSlug, logger); err != nil { logger.Printf("Invalid target-repo slug for %s", params.ConfigKey) return nil } diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index 900b60290b..998aa6c9c8 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -54,6 +54,12 @@ func formatCompilerMessage(filePath string, msgType string, message string) stri }) } +// emitWarning prints a warning message to stderr and increments the warning count. +func (c *Compiler) emitWarning(msg string) { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg)) + c.IncrementWarningCount() +} + // CompileWorkflow compiles a workflow markdown file into a GitHub Actions YAML file. // It reads the file from disk, parses frontmatter and markdown sections, and generates // the corresponding workflow YAML. Returns the compiled workflow data or an error. @@ -202,8 +208,7 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath // Emit warning for sandbox.agent: false (disables agent sandbox firewall) if isAgentSandboxDisabled(workflowData) { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("⚠️ WARNING: Agent sandbox disabled (sandbox.agent: false). This removes firewall protection. The AI agent will have direct network access without firewall filtering. The MCP gateway remains enabled. Only use this for testing or in controlled environments where you trust the AI agent completely.")) - c.IncrementWarningCount() + c.emitWarning("⚠️ WARNING: Agent sandbox disabled (sandbox.agent: false). This removes firewall protection. The AI agent will have direct network access without firewall filtering. The MCP gateway remains enabled. Only use this for testing or in controlled environments where you trust the AI agent completely.") } // Validate: threat detection requires sandbox.agent to be enabled (detection runs inside AWF) @@ -213,20 +218,17 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath // Emit experimental warning for safe-inputs feature if IsSafeInputsEnabled(workflowData.SafeInputs, workflowData) { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Using experimental feature: safe-inputs")) - c.IncrementWarningCount() + c.emitWarning("Using experimental feature: safe-inputs") } // Emit experimental warning for plugins feature if workflowData.PluginInfo != nil && len(workflowData.PluginInfo.Plugins) > 0 { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Using experimental feature: plugins")) - c.IncrementWarningCount() + c.emitWarning("Using experimental feature: plugins") } // Emit experimental warning for rate-limit feature if workflowData.RateLimit != nil { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Using experimental feature: rate-limit")) - c.IncrementWarningCount() + c.emitWarning("Using experimental feature: rate-limit") } // Validate workflow_run triggers have branch restrictions @@ -419,8 +421,7 @@ func (c *Compiler) generateAndValidateYAML(workflowData *WorkflowData, markdownP return "", formatCompilerError(markdownPath, "error", fmt.Sprintf("repository feature validation failed: %v", err), err) } } else if c.verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Schema validation available but skipped (use SetSkipValidation(false) to enable)")) - c.IncrementWarningCount() + c.emitWarning("Schema validation available but skipped (use SetSkipValidation(false) to enable)") } return yamlContent, nil diff --git a/pkg/workflow/compiler_orchestrator_tools.go b/pkg/workflow/compiler_orchestrator_tools.go index 9924d57edb..017c5d3d5d 100644 --- a/pkg/workflow/compiler_orchestrator_tools.go +++ b/pkg/workflow/compiler_orchestrator_tools.go @@ -2,11 +2,9 @@ package workflow import ( "fmt" - "os" "sort" "strings" - "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" "github.com/goccy/go-yaml" @@ -217,11 +215,9 @@ func (c *Compiler) processToolsAndMarkdown(result *parser.FrontmatterResult, cle if !agenticEngine.SupportsToolsAllowlist() { // For engines that don't support tool allowlists (like custom engine), ignore tools section and provide warnings - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Using experimental %s support (engine: %s)", agenticEngine.GetDisplayName(), agenticEngine.GetID()))) - c.IncrementWarningCount() + c.emitWarning(fmt.Sprintf("Using experimental %s support (engine: %s)", agenticEngine.GetDisplayName(), agenticEngine.GetID())) if _, hasTools := result.Frontmatter["tools"]; hasTools { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("'tools' section ignored when using engine: %s (%s doesn't support MCP tool allow-listing)", agenticEngine.GetID(), agenticEngine.GetDisplayName()))) - c.IncrementWarningCount() + c.emitWarning(fmt.Sprintf("'tools' section ignored when using engine: %s (%s doesn't support MCP tool allow-listing)", agenticEngine.GetID(), agenticEngine.GetDisplayName())) } tools = map[string]any{} // For now, we'll add a basic github tool (always uses docker MCP) diff --git a/pkg/workflow/create_discussion.go b/pkg/workflow/create_discussion.go index c88ff54944..4281487842 100644 --- a/pkg/workflow/create_discussion.go +++ b/pkg/workflow/create_discussion.go @@ -86,7 +86,7 @@ func (c *Compiler) parseDiscussionsConfig(outputMap map[string]any) *CreateDiscu } // Validate target-repo (wildcard "*" is not allowed) - if validateTargetRepoSlug(config.TargetRepoSlug, discussionLog) { + if err := validateTargetRepoSlug(config.TargetRepoSlug, discussionLog); err != nil { return nil // Invalid configuration, return nil to cause validation error } diff --git a/pkg/workflow/create_issue.go b/pkg/workflow/create_issue.go index aaccca04ef..bef5bd749b 100644 --- a/pkg/workflow/create_issue.go +++ b/pkg/workflow/create_issue.go @@ -79,7 +79,7 @@ func (c *Compiler) parseIssuesConfig(outputMap map[string]any) *CreateIssuesConf } // Validate target-repo (wildcard "*" is not allowed) - if validateTargetRepoSlug(config.TargetRepoSlug, createIssueLog) { + if err := validateTargetRepoSlug(config.TargetRepoSlug, createIssueLog); err != nil { return nil // Invalid configuration, return nil to cause validation error } diff --git a/pkg/workflow/create_pull_request.go b/pkg/workflow/create_pull_request.go index d0e6e474e6..4f2e6a4e82 100644 --- a/pkg/workflow/create_pull_request.go +++ b/pkg/workflow/create_pull_request.go @@ -307,7 +307,7 @@ func (c *Compiler) parsePullRequestsConfig(outputMap map[string]any) *CreatePull } // Validate target-repo (wildcard "*" is not allowed) - if validateTargetRepoSlug(config.TargetRepoSlug, createPRLog) { + if err := validateTargetRepoSlug(config.TargetRepoSlug, createPRLog); err != nil { return nil // Invalid configuration, return nil to cause validation error } diff --git a/pkg/workflow/dangerous_permissions_validation.go b/pkg/workflow/dangerous_permissions_validation.go index c38a01f17e..1932b23e9a 100644 --- a/pkg/workflow/dangerous_permissions_validation.go +++ b/pkg/workflow/dangerous_permissions_validation.go @@ -62,27 +62,16 @@ func findWritePermissions(permissions *Permissions) []PermissionScope { return nil } - var writePerms []PermissionScope - - // Check all permission scopes + // Collect scopes to check, excluding id-token and metadata + var scopes []PermissionScope for _, scope := range GetAllPermissionScopes() { - // Skip id-token as it's safe and used for OIDC authentication - if scope == PermissionIdToken { - continue - } - - // Skip metadata as it's a built-in read-only permission - if scope == PermissionMetadata { + if scope == PermissionIdToken || scope == PermissionMetadata { continue } - - level, exists := permissions.Get(scope) - if exists && level == PermissionWrite { - writePerms = append(writePerms, scope) - } + scopes = append(scopes, scope) } - return writePerms + return findWriteScopesForPolicy(permissions, scopes) } // formatDangerousPermissionsError formats an error message for write permissions violations diff --git a/pkg/workflow/data/action_pins.json b/pkg/workflow/data/action_pins.json index 8366ec146e..6cc217598c 100644 --- a/pkg/workflow/data/action_pins.json +++ b/pkg/workflow/data/action_pins.json @@ -110,6 +110,11 @@ "version": "v6", "sha": "b7c566a772e6b6bfb58ed0dc250532a479d7789f" }, + "anchore/sbom-action@v0": { + "repo": "anchore/sbom-action", + "version": "v0", + "sha": "17ae1740179002c89186b61233e0f892c3118b11" + }, "anchore/sbom-action@v0.22.2": { "repo": "anchore/sbom-action", "version": "v0.22.2", diff --git a/pkg/workflow/engine_firewall_support.go b/pkg/workflow/engine_firewall_support.go index 71df4cc962..1164745b5a 100644 --- a/pkg/workflow/engine_firewall_support.go +++ b/pkg/workflow/engine_firewall_support.go @@ -3,9 +3,7 @@ package workflow import ( "errors" "fmt" - "os" - "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" ) @@ -79,8 +77,7 @@ func (c *Compiler) checkNetworkSupport(engine CodingAgentEngine, networkPermissi } // In non-strict mode, emit a warning - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(message)) - c.IncrementWarningCount() + c.emitWarning(message) return nil } @@ -107,8 +104,7 @@ func (c *Compiler) checkFirewallDisable(engine CodingAgentEngine, networkPermiss } // In non-strict mode, emit a warning - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(message)) - c.IncrementWarningCount() + c.emitWarning(message) } // Also check if engine doesn't support firewall in strict mode when there are no restrictions diff --git a/pkg/workflow/features_validation.go b/pkg/workflow/features_validation.go index 9f074dbe13..becfe877ae 100644 --- a/pkg/workflow/features_validation.go +++ b/pkg/workflow/features_validation.go @@ -11,7 +11,6 @@ // // - validateFeatures() - Validates all feature flags in WorkflowData // - validateActionTag() - Validates action-tag is a full SHA -// - isValidFullSHA() - Checks if a string is a valid 40-character SHA // // # When to Add Validation Here // @@ -24,15 +23,12 @@ package workflow import ( "fmt" - "regexp" "github.com/github/gh-aw/pkg/logger" ) var featuresValidationLog = logger.New("workflow:features_validation") -var shaRegex = regexp.MustCompile("^[0-9a-f]{40}$") - // validateFeatures validates all feature flags in the workflow data func validateFeatures(data *WorkflowData) error { if data == nil || data.Features == nil { @@ -91,11 +87,3 @@ func validateActionTag(value any) error { return nil } - -// isValidFullSHA checks if a string is a valid 40-character hexadecimal SHA -func isValidFullSHA(s string) bool { - if len(s) != 40 { - return false - } - return shaRegex.MatchString(s) -} diff --git a/pkg/workflow/firewall_validation.go b/pkg/workflow/firewall_validation.go index d41e78c841..f47bff6102 100644 --- a/pkg/workflow/firewall_validation.go +++ b/pkg/workflow/firewall_validation.go @@ -3,6 +3,7 @@ // This file contains domain-specific validation functions for firewall configuration: // - validateFirewallConfig() - Validates the overall firewall configuration // - ValidateLogLevel() - Validates firewall log-level values +// - validateNetworkFirewallConfig() - Validates firewall configuration dependencies // // These validation functions are organized in a dedicated file following the validation // architecture pattern where domain-specific validation belongs in domain validation files. @@ -14,6 +15,7 @@ import ( "fmt" "slices" + "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" ) @@ -59,3 +61,35 @@ func ValidateLogLevel(level string) error { firewallValidationLog.Printf("Invalid log-level: %s", level) return fmt.Errorf("invalid log-level '%s', must be one of: %v", level, valid) } + +// validateNetworkFirewallConfig validates network firewall configuration dependencies +// Returns an error if the configuration is invalid +func validateNetworkFirewallConfig(networkPermissions *NetworkPermissions) error { + if networkPermissions == nil { + return nil + } + + firewallConfig := networkPermissions.Firewall + if firewallConfig == nil { + return nil + } + + firewallValidationLog.Print("Validating network firewall configuration") + + // Validate allow-urls requires ssl-bump + if len(firewallConfig.AllowURLs) > 0 && !firewallConfig.SSLBump { + firewallValidationLog.Printf("Validation error: allow-urls specified without ssl-bump: %d URLs", len(firewallConfig.AllowURLs)) + return NewValidationError( + "network.firewall.allow-urls", + "requires ssl-bump: true", + "allow-urls requires ssl-bump: true to function. SSL Bump enables HTTPS content inspection, which is necessary for URL path filtering", + "Enable SSL Bump in your firewall configuration:\n\nnetwork:\n firewall:\n ssl-bump: true\n allow-urls:\n - \"https://github.com/githubnext/*\"\n\nSee: "+string(constants.DocsNetworkURL), + ) + } + + if len(firewallConfig.AllowURLs) > 0 { + firewallValidationLog.Printf("Validated allow-urls: %d URLs with ssl-bump enabled", len(firewallConfig.AllowURLs)) + } + + return nil +} diff --git a/pkg/workflow/frontmatter_extraction_yaml.go b/pkg/workflow/frontmatter_extraction_yaml.go index 96ecf00b6d..0c7459aedf 100644 --- a/pkg/workflow/frontmatter_extraction_yaml.go +++ b/pkg/workflow/frontmatter_extraction_yaml.go @@ -2,11 +2,9 @@ package workflow import ( "fmt" - "os" "strconv" "strings" - "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" "github.com/goccy/go-yaml" @@ -638,8 +636,7 @@ func (c *Compiler) extractCommandConfig(frontmatter map[string]any) (commandName if hasCommand { // Show deprecation warning if using old field name if isDeprecated { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("The 'command:' trigger field is deprecated. Please use 'slash_command:' instead.")) - c.IncrementWarningCount() + c.emitWarning("The 'command:' trigger field is deprecated. Please use 'slash_command:' instead.") } // Check if command is a string (shorthand format) diff --git a/pkg/workflow/mcp_config_validation.go b/pkg/workflow/mcp_config_validation.go index 04f375bcac..bfc145491e 100644 --- a/pkg/workflow/mcp_config_validation.go +++ b/pkg/workflow/mcp_config_validation.go @@ -312,14 +312,8 @@ func validateMCPMountsSyntax(toolName string, mountsRaw any) error { return fmt.Errorf("tool '%s' mcp configuration 'mounts' must be an array of strings.\n\nExample:\ntools:\n %s:\n container: \"my-registry/my-tool\"\n mounts:\n - \"/host/path:/container/path:ro\"\n\nSee: %s", toolName, toolName, constants.DocsToolsURL) } - for i, mount := range mounts { - source, dest, mode, err := validateMountStringFormat(mount) - if err != nil { - if source == "" && dest == "" && mode == "" { - return fmt.Errorf("tool '%s' mcp configuration mounts[%d] must follow 'source:destination:mode' format, got: %q.\n\nExample:\ntools:\n %s:\n container: \"my-registry/my-tool\"\n mounts:\n - \"/host/path:/container/path:ro\"\n\nSee: %s", toolName, i, mount, toolName, constants.DocsToolsURL) - } - return fmt.Errorf("tool '%s' mcp configuration mounts[%d] mode must be 'ro' or 'rw', got: %q.\n\nExample:\ntools:\n %s:\n container: \"my-registry/my-tool\"\n mounts:\n - \"/host/path:/container/path:ro\" # read-only\n - \"/host/path:/container/path:rw\" # read-write\n\nSee: %s", toolName, i, mode, toolName, constants.DocsToolsURL) - } + if errs := validateMountStrings(mounts, string(constants.DocsToolsURL)); len(errs) > 0 { + return fmt.Errorf("tool '%s' mcp configuration %s\n\nExample:\ntools:\n %s:\n container: \"my-registry/my-tool\"\n mounts:\n - \"/host/path:/container/path:ro\"\n - \"/host/path:/container/path:rw\"", toolName, errs[0], toolName) } return nil diff --git a/pkg/workflow/network_firewall_validation.go b/pkg/workflow/network_firewall_validation.go deleted file mode 100644 index 0ba6453fb9..0000000000 --- a/pkg/workflow/network_firewall_validation.go +++ /dev/null @@ -1,49 +0,0 @@ -// This file provides network firewall validation functions for agentic workflow compilation. -// -// This file contains domain-specific validation functions for network firewall configuration: -// - validateNetworkFirewallConfig() - Validates firewall configuration dependencies -// -// These validation functions are organized in a dedicated file following the validation -// architecture pattern where domain-specific validation belongs in domain validation files. -// See validation.go for the complete validation architecture documentation. - -package workflow - -import ( - "github.com/github/gh-aw/pkg/constants" - "github.com/github/gh-aw/pkg/logger" -) - -var networkFirewallValidationLog = logger.New("workflow:network_firewall_validation") - -// validateNetworkFirewallConfig validates network firewall configuration dependencies -// Returns an error if the configuration is invalid -func validateNetworkFirewallConfig(networkPermissions *NetworkPermissions) error { - if networkPermissions == nil { - return nil - } - - firewallConfig := networkPermissions.Firewall - if firewallConfig == nil { - return nil - } - - networkFirewallValidationLog.Print("Validating network firewall configuration") - - // Validate allow-urls requires ssl-bump - if len(firewallConfig.AllowURLs) > 0 && !firewallConfig.SSLBump { - networkFirewallValidationLog.Printf("Validation error: allow-urls specified without ssl-bump: %d URLs", len(firewallConfig.AllowURLs)) - return NewValidationError( - "network.firewall.allow-urls", - "requires ssl-bump: true", - "allow-urls requires ssl-bump: true to function. SSL Bump enables HTTPS content inspection, which is necessary for URL path filtering", - "Enable SSL Bump in your firewall configuration:\n\nnetwork:\n firewall:\n ssl-bump: true\n allow-urls:\n - \"https://github.com/githubnext/*\"\n\nSee: "+string(constants.DocsNetworkURL), - ) - } - - if len(firewallConfig.AllowURLs) > 0 { - networkFirewallValidationLog.Printf("Validated allow-urls: %d URLs with ssl-bump enabled", len(firewallConfig.AllowURLs)) - } - - return nil -} diff --git a/pkg/workflow/npm_validation.go b/pkg/workflow/npm_validation.go index 59ba7e31b0..91c79b2715 100644 --- a/pkg/workflow/npm_validation.go +++ b/pkg/workflow/npm_validation.go @@ -68,7 +68,7 @@ func (c *Compiler) validateNpxPackages(workflowData *WorkflowData) error { ) } - var errors []string + collector := NewErrorCollector(false) for _, pkg := range packages { npmValidationLog.Printf("Validating npm package: %s", pkg) @@ -78,7 +78,7 @@ func (c *Compiler) validateNpxPackages(workflowData *WorkflowData) error { if err != nil { npmValidationLog.Printf("Package validation failed for %s: %v", pkg, err) - errors = append(errors, fmt.Sprintf("npx package '%s' not found on npm registry: %s", pkg, strings.TrimSpace(string(output)))) + _ = collector.Add(fmt.Errorf("npx package '%s' not found on npm registry: %s", pkg, strings.TrimSpace(string(output)))) } else { npmValidationLog.Printf("Package validated successfully: %s", pkg) if c.verbose { @@ -87,13 +87,13 @@ func (c *Compiler) validateNpxPackages(workflowData *WorkflowData) error { } } - if len(errors) > 0 { - npmValidationLog.Printf("npx package validation failed with %d errors", len(errors)) + if collector.HasErrors() { + npmValidationLog.Printf("npx package validation failed with %d errors", collector.Count()) return NewValidationError( "npx.packages", - fmt.Sprintf("%d packages not found", len(errors)), + fmt.Sprintf("%d packages not found", collector.Count()), "npx packages not found on npm registry", - fmt.Sprintf("Fix package names or verify they exist on npm:\n\n%s\n\nCheck package availability:\n$ npm view \n\nSearch for similar packages:\n$ npm search ", strings.Join(errors, "\n")), + fmt.Sprintf("Fix package names or verify they exist on npm:\n\n%s\n\nCheck package availability:\n$ npm view \n\nSearch for similar packages:\n$ npm search ", collector.Error().Error()), ) } diff --git a/pkg/workflow/package_extraction.go b/pkg/workflow/package_extraction.go index ced416490e..ad5098c035 100644 --- a/pkg/workflow/package_extraction.go +++ b/pkg/workflow/package_extraction.go @@ -340,3 +340,68 @@ func (pe *PackageExtractor) findPackageName(words []string, startIndex int) stri } return "" } + +// collectPackagesFromWorkflow is a generic helper that collects packages from workflow data +// using the provided extractor function. It deduplicates packages and optionally extracts +// from MCP tool configurations when toolCommand is provided. +func collectPackagesFromWorkflow( + workflowData *WorkflowData, + extractor func(string) []string, + toolCommand string, +) []string { + pkgLog.Printf("Collecting packages from workflow: toolCommand=%s", toolCommand) + var packages []string + seen := make(map[string]bool) + + // Extract from custom steps + if workflowData.CustomSteps != "" { + pkgs := extractor(workflowData.CustomSteps) + for _, pkg := range pkgs { + if !seen[pkg] { + packages = append(packages, pkg) + seen[pkg] = true + } + } + } + + // Extract from MCP server configurations (if toolCommand is provided) + if toolCommand != "" && workflowData.Tools != nil { + for _, toolConfig := range workflowData.Tools { + // Handle structured MCP config with command and args fields + if config, ok := toolConfig.(map[string]any); ok { + if command, hasCommand := config["command"]; hasCommand { + if cmdStr, ok := command.(string); ok && cmdStr == toolCommand { + // Extract package from args, skipping flags + if args, hasArgs := config["args"]; hasArgs { + if argsSlice, ok := args.([]any); ok { + for _, arg := range argsSlice { + if pkgStr, ok := arg.(string); ok { + // Skip flags (arguments starting with - or --) + if !strings.HasPrefix(pkgStr, "-") && !seen[pkgStr] { + packages = append(packages, pkgStr) + seen[pkgStr] = true + break // Only take the first non-flag argument + } + } + } + } + } + } + } + } else if cmdStr, ok := toolConfig.(string); ok { + // Handle string-format MCP tool (e.g., "npx -y package") + // Use the extractor function to parse the command string + pkgs := extractor(cmdStr) + for _, pkg := range pkgs { + if !seen[pkg] { + packages = append(packages, pkg) + seen[pkg] = true + } + } + } + } + } + + pkgLog.Printf("Collected %d unique packages", len(packages)) + return packages +} diff --git a/pkg/workflow/permissions_operations.go b/pkg/workflow/permissions_operations.go index dfc54df035..c42017d80d 100644 --- a/pkg/workflow/permissions_operations.go +++ b/pkg/workflow/permissions_operations.go @@ -276,3 +276,19 @@ func (p *Permissions) RenderToYAML() string { return strings.Join(lines, "\n") } + +// findWriteScopesForPolicy returns the subset of the given scopes that have write-level access. +// This is a shared helper for write-permission policy checks across validation functions. +func findWriteScopesForPolicy(permissions *Permissions, scopes []PermissionScope) []PermissionScope { + if permissions == nil { + return nil + } + var writeScopes []PermissionScope + for _, scope := range scopes { + level, exists := permissions.Get(scope) + if exists && level == PermissionWrite { + writeScopes = append(writeScopes, scope) + } + } + return writeScopes +} diff --git a/pkg/workflow/runtime_validation.go b/pkg/workflow/runtime_validation.go index 6106a52c6b..38d2df21b7 100644 --- a/pkg/workflow/runtime_validation.go +++ b/pkg/workflow/runtime_validation.go @@ -13,7 +13,6 @@ // - validateExpressionSizes() - Validates expression size limits (21KB max) // - validateContainerImages() - Validates Docker images exist // - validateRuntimePackages() - Validates npm, pip, uv packages -// - collectPackagesFromWorkflow() - Generic package collection helper // // # Validation Patterns // @@ -95,7 +94,7 @@ func (c *Compiler) validateContainerImages(workflowData *WorkflowData) error { } runtimeValidationLog.Printf("Validating container images for %d tools", len(workflowData.Tools)) - var errors []string + collector := NewErrorCollector(false) for toolName, toolConfig := range workflowData.Tools { if config, ok := toolConfig.(map[string]any); ok { // Get the MCP configuration to extract container info @@ -122,7 +121,7 @@ func (c *Compiler) validateContainerImages(workflowData *WorkflowData) error { // Validate the container image exists using docker if err := validateDockerImage(containerImage, c.verbose); err != nil { - errors = append(errors, fmt.Sprintf("tool '%s': %v", toolName, err)) + _ = collector.Add(fmt.Errorf("tool '%s': %w", toolName, err)) } else if c.verbose { fmt.Fprintln(os.Stderr, console.FormatInfoMessage("✓ Container image validated: "+containerImage)) } @@ -130,12 +129,12 @@ func (c *Compiler) validateContainerImages(workflowData *WorkflowData) error { } } - if len(errors) > 0 { + if collector.HasErrors() { return NewValidationError( "container.images", - fmt.Sprintf("%d images failed validation", len(errors)), + fmt.Sprintf("%d images failed validation", collector.Count()), "container image validation failed", - fmt.Sprintf("Fix the following container image issues:\n\n%s\n\nEnsure:\n1. Container images exist and are accessible\n2. Registry URLs are correct\n3. Image tags are specified\n4. You have pull permissions for private images", strings.Join(errors, "\n")), + fmt.Sprintf("Fix the following container image issues:\n\n%s\n\nEnsure:\n1. Container images exist and are accessible\n2. Registry URLs are correct\n3. Image tags are specified\n4. You have pull permissions for private images", collector.Error().Error()), ) } @@ -149,7 +148,7 @@ func (c *Compiler) validateRuntimePackages(workflowData *WorkflowData) error { requirements := DetectRuntimeRequirements(workflowData) runtimeValidationLog.Printf("Validating runtime packages: found %d runtime requirements", len(requirements)) - var errors []string + collector := NewErrorCollector(false) for _, req := range requirements { switch req.Runtime.ID { case "node": @@ -157,100 +156,35 @@ func (c *Compiler) validateRuntimePackages(workflowData *WorkflowData) error { runtimeValidationLog.Print("Validating npx packages") if err := c.validateNpxPackages(workflowData); err != nil { runtimeValidationLog.Printf("Npx package validation failed: %v", err) - errors = append(errors, err.Error()) + _ = collector.Add(err) } case "python": // Validate pip packages used in the workflow runtimeValidationLog.Print("Validating pip packages") if err := c.validatePipPackages(workflowData); err != nil { runtimeValidationLog.Printf("Pip package validation failed: %v", err) - errors = append(errors, err.Error()) + _ = collector.Add(err) } case "uv": // Validate uv packages used in the workflow runtimeValidationLog.Print("Validating uv packages") if err := c.validateUvPackages(workflowData); err != nil { runtimeValidationLog.Printf("Uv package validation failed: %v", err) - errors = append(errors, err.Error()) + _ = collector.Add(err) } } } - if len(errors) > 0 { - runtimeValidationLog.Printf("Runtime package validation completed with %d errors", len(errors)) + if collector.HasErrors() { + runtimeValidationLog.Printf("Runtime package validation completed with %d errors", collector.Count()) return NewValidationError( "runtime.packages", - fmt.Sprintf("%d package validation errors", len(errors)), + fmt.Sprintf("%d package validation errors", collector.Count()), "runtime package validation failed", - fmt.Sprintf("Fix the following package issues:\n\n%s\n\nEnsure:\n1. Package names are spelled correctly\n2. Packages exist in their respective registries (npm, PyPI)\n3. Package managers (npm, pip, uv) are installed\n4. Network access is available for registry checks", strings.Join(errors, "\n")), + fmt.Sprintf("Fix the following package issues:\n\n%s\n\nEnsure:\n1. Package names are spelled correctly\n2. Packages exist in their respective registries (npm, PyPI)\n3. Package managers (npm, pip, uv) are installed\n4. Network access is available for registry checks", collector.Error().Error()), ) } runtimeValidationLog.Print("Runtime package validation passed") return nil } - -// collectPackagesFromWorkflow is a generic helper that collects packages from workflow data -// using the provided extractor function. It deduplicates packages and optionally extracts -// from MCP tool configurations when toolCommand is provided. -func collectPackagesFromWorkflow( - workflowData *WorkflowData, - extractor func(string) []string, - toolCommand string, -) []string { - runtimeValidationLog.Printf("Collecting packages from workflow: toolCommand=%s", toolCommand) - var packages []string - seen := make(map[string]bool) - - // Extract from custom steps - if workflowData.CustomSteps != "" { - pkgs := extractor(workflowData.CustomSteps) - for _, pkg := range pkgs { - if !seen[pkg] { - packages = append(packages, pkg) - seen[pkg] = true - } - } - } - - // Extract from MCP server configurations (if toolCommand is provided) - if toolCommand != "" && workflowData.Tools != nil { - for _, toolConfig := range workflowData.Tools { - // Handle structured MCP config with command and args fields - if config, ok := toolConfig.(map[string]any); ok { - if command, hasCommand := config["command"]; hasCommand { - if cmdStr, ok := command.(string); ok && cmdStr == toolCommand { - // Extract package from args, skipping flags - if args, hasArgs := config["args"]; hasArgs { - if argsSlice, ok := args.([]any); ok { - for _, arg := range argsSlice { - if pkgStr, ok := arg.(string); ok { - // Skip flags (arguments starting with - or --) - if !strings.HasPrefix(pkgStr, "-") && !seen[pkgStr] { - packages = append(packages, pkgStr) - seen[pkgStr] = true - break // Only take the first non-flag argument - } - } - } - } - } - } - } - } else if cmdStr, ok := toolConfig.(string); ok { - // Handle string-format MCP tool (e.g., "npx -y package") - // Use the extractor function to parse the command string - pkgs := extractor(cmdStr) - for _, pkg := range pkgs { - if !seen[pkg] { - packages = append(packages, pkg) - seen[pkg] = true - } - } - } - } - } - - runtimeValidationLog.Printf("Collected %d unique packages", len(packages)) - return packages -} diff --git a/pkg/workflow/safe_outputs_target_validation.go b/pkg/workflow/safe_outputs_target_validation.go index 686245b339..abbc67850c 100644 --- a/pkg/workflow/safe_outputs_target_validation.go +++ b/pkg/workflow/safe_outputs_target_validation.go @@ -1,6 +1,7 @@ package workflow import ( + "errors" "fmt" "strings" @@ -159,3 +160,15 @@ func isGitHubExpression(s string) bool { // and there must be something between them return openIndex >= 0 && closeIndex > openIndex+3 } + +// validateTargetRepoSlug validates that a target-repo slug is not a wildcard. +// Returns an error if the value equals "*", which is not allowed for safe outputs. +func validateTargetRepoSlug(targetRepoSlug string, log *logger.Logger) error { + if targetRepoSlug == "*" { + if log != nil { + log.Print("Invalid target-repo: wildcard '*' is not allowed") + } + return errors.New("target-repo wildcard '*' is not allowed for safe outputs") + } + return nil +} diff --git a/pkg/workflow/strict_mode_validation.go b/pkg/workflow/strict_mode_validation.go index 373cad4841..627e22b198 100644 --- a/pkg/workflow/strict_mode_validation.go +++ b/pkg/workflow/strict_mode_validation.go @@ -63,15 +63,18 @@ func (c *Compiler) validateStrictPermissions(frontmatter map[string]any) error { } // Parse permissions using the PermissionsParser - perms := NewPermissionsParserFromValue(permissionsValue) - - // Check for write permissions on sensitive scopes - writePermissions := []string{"contents", "issues", "pull-requests"} - for _, scope := range writePermissions { - if perms.IsAllowed(scope, "write") { - strictModeValidationLog.Printf("Write permission validation failed: scope=%s", scope) - return fmt.Errorf("strict mode: write permission '%s: write' is not allowed for security reasons. Use 'safe-outputs.create-issue', 'safe-outputs.create-pull-request', 'safe-outputs.add-comment', or 'safe-outputs.update-issue' to perform write operations safely. See: https://github.github.com/gh-aw/reference/safe-outputs/", scope) - } + permissions := NewPermissionsParserFromValue(permissionsValue).ToPermissions() + + // Check for write permissions on sensitive scopes using the shared policy helper + sensitiveScopes := []PermissionScope{ + PermissionContents, + PermissionIssues, + PermissionPullRequests, + } + writeScopes := findWriteScopesForPolicy(permissions, sensitiveScopes) + for _, scope := range writeScopes { + strictModeValidationLog.Printf("Write permission validation failed: scope=%s", scope) + return fmt.Errorf("strict mode: write permission '%s: write' is not allowed for security reasons. Use 'safe-outputs.create-issue', 'safe-outputs.create-pull-request', 'safe-outputs.add-comment', or 'safe-outputs.update-issue' to perform write operations safely. See: https://github.github.com/gh-aw/reference/safe-outputs/", scope) } strictModeValidationLog.Printf("Permissions validation passed") diff --git a/pkg/workflow/validation_helpers.go b/pkg/workflow/validation_helpers.go index 86a84bb3e3..72e1735b2a 100644 --- a/pkg/workflow/validation_helpers.go +++ b/pkg/workflow/validation_helpers.go @@ -14,6 +14,7 @@ // - ValidatePositiveInt() - Validates that a value is a positive integer // - ValidateNonNegativeInt() - Validates that a value is a non-negative integer // - validateMountStringFormat() - Parses and validates a "source:dest:mode" mount string +// - isValidFullSHA() - Checks if a string is a valid 40-character hexadecimal SHA // // # Design Rationale // @@ -31,6 +32,7 @@ package workflow import ( "errors" "fmt" + "regexp" "slices" "strconv" "strings" @@ -40,6 +42,16 @@ import ( var validationHelpersLog = logger.New("workflow:validation_helpers") +var shaRegex = regexp.MustCompile("^[0-9a-f]{40}$") + +// isValidFullSHA checks if a string is a valid 40-character hexadecimal SHA +func isValidFullSHA(s string) bool { + if len(s) != 40 { + return false + } + return shaRegex.MatchString(s) +} + // validateIntRange validates that a value is within the specified inclusive range [min, max]. // It returns an error if the value is outside the range, with a descriptive message // including the field name and the actual value. @@ -166,6 +178,25 @@ func validateMountStringFormat(mount string) (source, dest, mode string, err err return parts[0], parts[1], parts[2], nil } +// validateMountStrings validates a list of mount strings against the expected format. +// It returns a slice of human-readable error strings describing any invalid mounts. +// The docsURL is included in error messages to help users find documentation. +// Both sandbox and MCP mount validation share this core loop. +func validateMountStrings(mounts []string, docsURL string) []string { + var errs []string + for i, mount := range mounts { + source, dest, mode, err := validateMountStringFormat(mount) + if err != nil { + if source == "" && dest == "" && mode == "" { + errs = append(errs, fmt.Sprintf("mounts[%d] %q must follow 'source:destination:mode' format. See: %s", i, mount, docsURL)) + } else { + errs = append(errs, fmt.Sprintf("mounts[%d] mode must be 'ro' or 'rw', got %q. See: %s", i, mode, docsURL)) + } + } + } + return errs +} + // formatList formats a list of strings as a comma-separated list with natural language conjunction func formatList(items []string) string { if len(items) == 0 { @@ -179,16 +210,3 @@ func formatList(items []string) string { } return fmt.Sprintf("%s, and %s", formatList(items[:len(items)-1]), items[len(items)-1]) } - -// validateTargetRepoSlug validates that a target-repo slug is not a wildcard. -// Returns true if the value is invalid (i.e., equals "*"). -// This helper is used when the target-repo has already been parsed into a struct field. -func validateTargetRepoSlug(targetRepoSlug string, log *logger.Logger) bool { - if targetRepoSlug == "*" { - if log != nil { - log.Print("Invalid target-repo: wildcard '*' is not allowed") - } - return true // Return true to indicate validation error - } - return false -}