diff --git a/pkg/workflow/strict_mode_llm_gateway_test.go b/pkg/workflow/strict_mode_llm_gateway_test.go index 1de7dd2bf91..42269771d23 100644 --- a/pkg/workflow/strict_mode_llm_gateway_test.go +++ b/pkg/workflow/strict_mode_llm_gateway_test.go @@ -83,11 +83,11 @@ func TestValidateStrictFirewall_LLMGatewaySupport(t *testing.T) { } }) - t.Run("copilot engine without LLM gateway support allows domains from known ecosystems", func(t *testing.T) { + t.Run("copilot engine without LLM gateway support rejects domains from known ecosystems but suggests ecosystem identifier", func(t *testing.T) { compiler := NewCompiler() compiler.strictMode = true - // These domains are from known ecosystems (python, node) + // These domains are from known ecosystems (python, node) but users should use ecosystem identifiers instead networkPerms := &NetworkPermissions{ Allowed: []string{"pypi.org", "registry.npmjs.org"}, Firewall: &FirewallConfig{ @@ -96,8 +96,15 @@ func TestValidateStrictFirewall_LLMGatewaySupport(t *testing.T) { } err := compiler.validateStrictFirewall("copilot", networkPerms, nil) - if err != nil { - t.Errorf("Expected no error for copilot engine with known ecosystem domains, got: %v", err) + if err == nil { + t.Fatal("Expected error for individual ecosystem domains in strict mode, got nil") + } + // Should suggest using ecosystem identifiers instead + if !strings.Contains(err.Error(), "python") { + t.Errorf("Error should suggest 'python' ecosystem, got: %v", err) + } + if !strings.Contains(err.Error(), "node") { + t.Errorf("Error should suggest 'node' ecosystem, got: %v", err) } }) @@ -118,11 +125,11 @@ func TestValidateStrictFirewall_LLMGatewaySupport(t *testing.T) { } }) - t.Run("codex engine with LLM gateway allows domains from known ecosystems", func(t *testing.T) { + t.Run("codex engine with LLM gateway rejects domains from known ecosystems but suggests ecosystem identifier", func(t *testing.T) { compiler := NewCompiler() compiler.strictMode = true - // These domains are from known ecosystems (python, node) + // These domains are from known ecosystems (python, node) but users should use ecosystem identifiers instead networkPerms := &NetworkPermissions{ Allowed: []string{"pypi.org", "registry.npmjs.org"}, Firewall: &FirewallConfig{ @@ -131,8 +138,15 @@ func TestValidateStrictFirewall_LLMGatewaySupport(t *testing.T) { } err := compiler.validateStrictFirewall("codex", networkPerms, nil) - if err != nil { - t.Errorf("Expected no error for codex engine with known ecosystem domains, got: %v", err) + if err == nil { + t.Fatal("Expected error for individual ecosystem domains in strict mode, got nil") + } + // Should suggest using ecosystem identifiers instead + if !strings.Contains(err.Error(), "python") { + t.Errorf("Error should suggest 'python' ecosystem, got: %v", err) + } + if !strings.Contains(err.Error(), "node") { + t.Errorf("Error should suggest 'node' ecosystem, got: %v", err) } }) @@ -324,3 +338,172 @@ func TestSupportsLLMGateway(t *testing.T) { }) } } + +// TestValidateStrictFirewall_EcosystemSuggestions tests ecosystem suggestions in error messages +func TestValidateStrictFirewall_EcosystemSuggestions(t *testing.T) { + t.Run("suggests ecosystem when individual domain from ecosystem is used", func(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = true + + networkPerms := &NetworkPermissions{ + Allowed: []string{"pypi.org"}, + Firewall: &FirewallConfig{ + Enabled: true, + }, + } + + err := compiler.validateStrictFirewall("copilot", networkPerms, nil) + if err == nil { + t.Fatal("Expected error for individual ecosystem domain in strict mode, got nil") + } + + // Should suggest using 'python' ecosystem identifier + if !strings.Contains(err.Error(), "pypi.org") { + t.Errorf("Error should mention domain 'pypi.org', got: %v", err) + } + if !strings.Contains(err.Error(), "python") { + t.Errorf("Error should suggest 'python' ecosystem, got: %v", err) + } + if !strings.Contains(err.Error(), "Did you mean") { + t.Errorf("Error should include 'Did you mean' suggestion, got: %v", err) + } + }) + + t.Run("suggests ecosystem for multiple domains from same ecosystem", func(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = true + + networkPerms := &NetworkPermissions{ + Allowed: []string{"npmjs.org", "registry.npmjs.com"}, + Firewall: &FirewallConfig{ + Enabled: true, + }, + } + + err := compiler.validateStrictFirewall("copilot", networkPerms, nil) + if err == nil { + t.Fatal("Expected error for individual ecosystem domains in strict mode, got nil") + } + + // Should suggest using 'node' ecosystem identifier for both + if !strings.Contains(err.Error(), "npmjs.org") { + t.Errorf("Error should mention domain 'npmjs.org', got: %v", err) + } + if !strings.Contains(err.Error(), "node") { + t.Errorf("Error should suggest 'node' ecosystem, got: %v", err) + } + if !strings.Contains(err.Error(), "Did you mean") { + t.Errorf("Error should include 'Did you mean' suggestion, got: %v", err) + } + }) + + t.Run("suggests ecosystem for domains from different ecosystems", func(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = true + + networkPerms := &NetworkPermissions{ + Allowed: []string{"pypi.org", "npmjs.org"}, + Firewall: &FirewallConfig{ + Enabled: true, + }, + } + + err := compiler.validateStrictFirewall("copilot", networkPerms, nil) + if err == nil { + t.Fatal("Expected error for individual ecosystem domains in strict mode, got nil") + } + + // Should suggest both 'python' and 'node' ecosystems + if !strings.Contains(err.Error(), "pypi.org") { + t.Errorf("Error should mention domain 'pypi.org', got: %v", err) + } + if !strings.Contains(err.Error(), "python") { + t.Errorf("Error should suggest 'python' ecosystem, got: %v", err) + } + if !strings.Contains(err.Error(), "npmjs.org") { + t.Errorf("Error should mention domain 'npmjs.org', got: %v", err) + } + if !strings.Contains(err.Error(), "node") { + t.Errorf("Error should suggest 'node' ecosystem, got: %v", err) + } + if !strings.Contains(err.Error(), "Did you mean") { + t.Errorf("Error should include 'Did you mean' suggestion, got: %v", err) + } + }) + + t.Run("no suggestion for truly custom domains", func(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = true + + networkPerms := &NetworkPermissions{ + Allowed: []string{"custom-domain.com"}, + Firewall: &FirewallConfig{ + Enabled: true, + }, + } + + err := compiler.validateStrictFirewall("copilot", networkPerms, nil) + if err == nil { + t.Fatal("Expected error for custom domain in strict mode, got nil") + } + + // Should NOT include "Did you mean" since this is a truly custom domain + if strings.Contains(err.Error(), "Did you mean") { + t.Errorf("Error should not include 'Did you mean' suggestion for truly custom domain, got: %v", err) + } + }) + + t.Run("mixed custom and ecosystem domains shows suggestions only for ecosystem domains", func(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = true + + networkPerms := &NetworkPermissions{ + Allowed: []string{"pypi.org", "custom-domain.com"}, + Firewall: &FirewallConfig{ + Enabled: true, + }, + } + + err := compiler.validateStrictFirewall("copilot", networkPerms, nil) + if err == nil { + t.Fatal("Expected error for mixed domains in strict mode, got nil") + } + + // Should suggest 'python' for pypi.org but not mention custom-domain.com in suggestions + if !strings.Contains(err.Error(), "pypi.org") { + t.Errorf("Error should mention domain 'pypi.org', got: %v", err) + } + if !strings.Contains(err.Error(), "python") { + t.Errorf("Error should suggest 'python' ecosystem, got: %v", err) + } + if !strings.Contains(err.Error(), "Did you mean") { + t.Errorf("Error should include 'Did you mean' suggestion, got: %v", err) + } + // custom-domain.com should NOT appear in the "Did you mean" part + errMsg := err.Error() + didYouMeanIdx := strings.Index(errMsg, "Did you mean") + if didYouMeanIdx != -1 { + didYouMeanPart := errMsg[didYouMeanIdx:] + if strings.Contains(didYouMeanPart, "custom-domain.com") { + t.Errorf("Error should not suggest ecosystem for custom-domain.com, got: %v", err) + } + } + }) + + t.Run("allows ecosystem identifiers without suggestions", func(t *testing.T) { + compiler := NewCompiler() + compiler.strictMode = true + + networkPerms := &NetworkPermissions{ + Allowed: []string{"python", "node"}, + Firewall: &FirewallConfig{ + Enabled: true, + }, + } + + err := compiler.validateStrictFirewall("copilot", networkPerms, nil) + if err != nil { + t.Errorf("Expected no error for ecosystem identifiers in strict mode, got: %v", err) + } + }) +} diff --git a/pkg/workflow/strict_mode_validation.go b/pkg/workflow/strict_mode_validation.go index 8c592ca0deb..e61af00e6ab 100644 --- a/pkg/workflow/strict_mode_validation.go +++ b/pkg/workflow/strict_mode_validation.go @@ -349,8 +349,14 @@ func (c *Compiler) validateStrictFirewall(engineID string, networkPermissions *N if networkPermissions != nil && len(networkPermissions.Allowed) > 0 { strictModeValidationLog.Printf("Validating network domains in strict mode for all engines") - // Check if allowed domains contain only known ecosystems or "defaults" - hasCustomDomain := false + // Check if allowed domains contain only known ecosystem identifiers + // Track domains that are not ecosystem identifiers (both individual ecosystem domains and truly custom domains) + type domainSuggestion struct { + domain string + ecosystem string // empty if no ecosystem found, non-empty if domain belongs to known ecosystem + } + var invalidDomains []domainSuggestion + for _, domain := range networkPermissions.Allowed { // Skip wildcards (handled below) if domain == "*" { @@ -360,27 +366,45 @@ func (c *Compiler) validateStrictFirewall(engineID string, networkPermissions *N // Check if this is a known ecosystem identifier ecosystemDomains := getEcosystemDomains(domain) if len(ecosystemDomains) > 0 { - // This is a known ecosystem identifier + // This is a known ecosystem identifier - allowed in strict mode strictModeValidationLog.Printf("Domain '%s' is a known ecosystem identifier", domain) continue } - // Check if this domain belongs to any ecosystem + // Not an ecosystem identifier - check if it belongs to any ecosystem ecosystem := GetDomainEcosystem(domain) if ecosystem != "" { - // This domain is from a known ecosystem - strictModeValidationLog.Printf("Domain '%s' belongs to ecosystem '%s'", domain, ecosystem) - continue + // This domain belongs to a known ecosystem, but user should use ecosystem identifier instead + strictModeValidationLog.Printf("Domain '%s' belongs to ecosystem '%s', but should use ecosystem identifier in strict mode", domain, ecosystem) + invalidDomains = append(invalidDomains, domainSuggestion{domain: domain, ecosystem: ecosystem}) + } else { + // This is a truly custom domain (not in any ecosystem) + strictModeValidationLog.Printf("Domain '%s' is a custom domain (not from known ecosystems)", domain) + invalidDomains = append(invalidDomains, domainSuggestion{domain: domain, ecosystem: ""}) } - - // This is a custom domain - strictModeValidationLog.Printf("Domain '%s' is a custom domain (not from known ecosystems)", domain) - hasCustomDomain = true } - if hasCustomDomain { - strictModeValidationLog.Printf("Engine '%s' has custom domains in strict mode, failing validation", engineID) - return fmt.Errorf("strict mode: network domains must be from known ecosystems (e.g., 'defaults', 'python', 'node') for all engines in strict mode. Custom domains are not allowed for security. Set 'strict: false' to use custom domains. See: https://github.github.com/gh-aw/reference/network/") + if len(invalidDomains) > 0 { + strictModeValidationLog.Printf("Engine '%s' has invalid domains in strict mode, failing validation", engineID) + + // Build error message with ecosystem suggestions + errorMsg := "strict mode: network domains must be from known ecosystems (e.g., 'defaults', 'python', 'node') for all engines in strict mode. Custom domains are not allowed for security." + + // Add suggestions for domains that belong to known ecosystems + var suggestions []string + for _, ds := range invalidDomains { + if ds.ecosystem != "" { + suggestions = append(suggestions, fmt.Sprintf("'%s' belongs to ecosystem '%s'", ds.domain, ds.ecosystem)) + } + } + + if len(suggestions) > 0 { + errorMsg += " Did you mean: " + strings.Join(suggestions, ", ") + "?" + } + + errorMsg += " Set 'strict: false' to use custom domains. See: https://github.github.com/gh-aw/reference/network/" + + return fmt.Errorf("%s", errorMsg) } }