-
Notifications
You must be signed in to change notification settings - Fork 110
Add validation: allow-urls requires ssl-bump: true #14494
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+262
−1
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| // 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 | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,203 @@ | ||
| //go:build !integration | ||
|
|
||
| package workflow | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestValidateNetworkFirewallConfig_AllowURLsRequiresSSLBump(t *testing.T) { | ||
| t.Run("allow-urls without ssl-bump fails validation", func(t *testing.T) { | ||
| networkPermissions := &NetworkPermissions{ | ||
| Firewall: &FirewallConfig{ | ||
| Enabled: true, | ||
| SSLBump: false, | ||
| AllowURLs: []string{"https://github.com/githubnext/*"}, | ||
| }, | ||
| } | ||
|
|
||
| err := validateNetworkFirewallConfig(networkPermissions) | ||
|
|
||
| require.Error(t, err, "Expected validation error when allow-urls is specified without ssl-bump") | ||
| assert.Contains(t, err.Error(), "allow-urls requires ssl-bump: true", "Error should mention the ssl-bump requirement") | ||
| assert.Contains(t, err.Error(), "network.firewall.allow-urls", "Error should identify the field") | ||
| }) | ||
|
|
||
| t.Run("allow-urls with ssl-bump passes validation", func(t *testing.T) { | ||
| networkPermissions := &NetworkPermissions{ | ||
| Firewall: &FirewallConfig{ | ||
| Enabled: true, | ||
| SSLBump: true, | ||
| AllowURLs: []string{"https://github.com/githubnext/*"}, | ||
| }, | ||
| } | ||
|
|
||
| err := validateNetworkFirewallConfig(networkPermissions) | ||
|
|
||
| assert.NoError(t, err, "Should not return error when ssl-bump is enabled with allow-urls") | ||
| }) | ||
|
|
||
| t.Run("multiple allow-urls without ssl-bump fails validation", func(t *testing.T) { | ||
| networkPermissions := &NetworkPermissions{ | ||
| Firewall: &FirewallConfig{ | ||
| Enabled: true, | ||
| SSLBump: false, | ||
| AllowURLs: []string{ | ||
| "https://github.com/githubnext/*", | ||
| "https://api.github.com/repos/*", | ||
| "https://example.com/api/*", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| err := validateNetworkFirewallConfig(networkPermissions) | ||
|
|
||
| require.Error(t, err, "Expected validation error when multiple allow-urls are specified without ssl-bump") | ||
| assert.Contains(t, err.Error(), "allow-urls requires ssl-bump: true", "Error should mention the ssl-bump requirement") | ||
| }) | ||
|
|
||
| t.Run("multiple allow-urls with ssl-bump passes validation", func(t *testing.T) { | ||
| networkPermissions := &NetworkPermissions{ | ||
| Firewall: &FirewallConfig{ | ||
| Enabled: true, | ||
| SSLBump: true, | ||
| AllowURLs: []string{ | ||
| "https://github.com/githubnext/*", | ||
| "https://api.github.com/repos/*", | ||
| "https://example.com/api/*", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| err := validateNetworkFirewallConfig(networkPermissions) | ||
|
|
||
| assert.NoError(t, err, "Should not return error when ssl-bump is enabled with multiple allow-urls") | ||
| }) | ||
|
|
||
| t.Run("ssl-bump without allow-urls passes validation", func(t *testing.T) { | ||
| networkPermissions := &NetworkPermissions{ | ||
| Firewall: &FirewallConfig{ | ||
| Enabled: true, | ||
| SSLBump: true, | ||
| AllowURLs: nil, | ||
| }, | ||
| } | ||
|
|
||
| err := validateNetworkFirewallConfig(networkPermissions) | ||
|
|
||
| assert.NoError(t, err, "Should not return error when ssl-bump is enabled without allow-urls") | ||
| }) | ||
|
|
||
| t.Run("empty allow-urls with ssl-bump passes validation", func(t *testing.T) { | ||
| networkPermissions := &NetworkPermissions{ | ||
| Firewall: &FirewallConfig{ | ||
| Enabled: true, | ||
| SSLBump: true, | ||
| AllowURLs: []string{}, | ||
| }, | ||
| } | ||
|
|
||
| err := validateNetworkFirewallConfig(networkPermissions) | ||
|
|
||
| assert.NoError(t, err, "Should not return error when allow-urls is empty") | ||
| }) | ||
|
|
||
| t.Run("empty allow-urls without ssl-bump passes validation", func(t *testing.T) { | ||
| networkPermissions := &NetworkPermissions{ | ||
| Firewall: &FirewallConfig{ | ||
| Enabled: true, | ||
| SSLBump: false, | ||
| AllowURLs: []string{}, | ||
| }, | ||
| } | ||
|
|
||
| err := validateNetworkFirewallConfig(networkPermissions) | ||
|
|
||
| assert.NoError(t, err, "Should not return error when allow-urls is empty") | ||
| }) | ||
|
|
||
| t.Run("no firewall config passes validation", func(t *testing.T) { | ||
| networkPermissions := &NetworkPermissions{ | ||
| Firewall: nil, | ||
| } | ||
|
|
||
| err := validateNetworkFirewallConfig(networkPermissions) | ||
|
|
||
| assert.NoError(t, err, "Should not return error when firewall is not configured") | ||
| }) | ||
|
|
||
| t.Run("nil network permissions passes validation", func(t *testing.T) { | ||
| err := validateNetworkFirewallConfig(nil) | ||
|
|
||
| assert.NoError(t, err, "Should not return error when network permissions is nil") | ||
| }) | ||
|
|
||
| t.Run("firewall enabled false with allow-urls and no ssl-bump fails validation", func(t *testing.T) { | ||
| networkPermissions := &NetworkPermissions{ | ||
| Firewall: &FirewallConfig{ | ||
| Enabled: false, | ||
| SSLBump: false, | ||
| AllowURLs: []string{"https://github.com/*"}, | ||
| }, | ||
| } | ||
|
|
||
| err := validateNetworkFirewallConfig(networkPermissions) | ||
|
|
||
| require.Error(t, err, "Expected validation error even when firewall is disabled") | ||
| assert.Contains(t, err.Error(), "allow-urls requires ssl-bump: true", "Error should mention the ssl-bump requirement") | ||
| }) | ||
| } | ||
|
|
||
| func TestValidateNetworkFirewallConfig_Integration(t *testing.T) { | ||
| t.Run("compiler rejects workflow with allow-urls but no ssl-bump", func(t *testing.T) { | ||
| compiler := NewCompiler() | ||
| compiler.SetStrictMode(false) // Test in non-strict mode | ||
|
|
||
|
Comment on lines
+154
to
+158
|
||
| workflowData := &WorkflowData{ | ||
| Name: "test-workflow", | ||
| EngineConfig: &EngineConfig{ | ||
| ID: "copilot", | ||
| }, | ||
| NetworkPermissions: &NetworkPermissions{ | ||
| Firewall: &FirewallConfig{ | ||
| Enabled: true, | ||
| SSLBump: false, | ||
| AllowURLs: []string{"https://github.com/githubnext/*"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| // Manually call validation (simulating what happens in CompileWorkflowData) | ||
| err := validateNetworkFirewallConfig(workflowData.NetworkPermissions) | ||
|
|
||
| require.Error(t, err, "Compiler should reject workflow with allow-urls but no ssl-bump") | ||
| assert.Contains(t, err.Error(), "allow-urls requires ssl-bump: true", "Error should explain the requirement") | ||
| }) | ||
|
|
||
| t.Run("compiler accepts workflow with allow-urls and ssl-bump", func(t *testing.T) { | ||
| compiler := NewCompiler() | ||
| compiler.SetStrictMode(false) | ||
|
|
||
| workflowData := &WorkflowData{ | ||
| Name: "test-workflow", | ||
| EngineConfig: &EngineConfig{ | ||
| ID: "copilot", | ||
| }, | ||
| NetworkPermissions: &NetworkPermissions{ | ||
| Firewall: &FirewallConfig{ | ||
| Enabled: true, | ||
| SSLBump: true, | ||
| AllowURLs: []string{"https://github.com/githubnext/*"}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| // Manually call validation | ||
| err := validateNetworkFirewallConfig(workflowData.NetworkPermissions) | ||
|
|
||
| assert.NoError(t, err, "Compiler should accept workflow with allow-urls and ssl-bump enabled") | ||
| }) | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NewValidationError(field, value, reason, suggestion)is being called with the requirement text as thevalueargument. This causes the rendered error to showValue: requires ssl-bump: trueinstead of the actual invalid configuration (e.g.,ssl-bump: falseand/or the provided allow-urls). Swap/populate thevalueparameter with the offending config and keep the requirement inreasonso the error output is accurate and consistent with other validators.