-
Notifications
You must be signed in to change notification settings - Fork 92
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
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
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.
Pull request overview
Adds compile-time validation to prevent misconfigured network firewall settings where network.firewall.allow-urls is set without enabling network.firewall.ssl-bump, aligning runtime behavior with the documented schema constraint.
Changes:
- Introduces
validateNetworkFirewallConfigto enforceallow-urls⇒ssl-bump: truewith a structured validation error. - Integrates the new validator into
Compiler.validateWorkflowData. - Adds a docs URL constant and accompanying unit tests; includes a small docs table correction.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/network_firewall_validation.go | New validator enforcing allow-urls requires ssl-bump: true, returning a WorkflowValidationError. |
| pkg/workflow/compiler.go | Wires the new network firewall validation into the compiler validation flow. |
| pkg/constants/constants.go | Adds DocsNetworkURL for consistent documentation links in validation errors. |
| pkg/workflow/network_firewall_validation_test.go | Adds test coverage for valid/invalid allow-urls/ssl-bump combinations and edge cases. |
| docs/src/content/docs/agent-factory-status.mdx | Updates the “Smoke Project” engine label in the status table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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), | ||
| ) |
Copilot
AI
Feb 8, 2026
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 the value argument. This causes the rendered error to show Value: requires ssl-bump: true instead of the actual invalid configuration (e.g., ssl-bump: false and/or the provided allow-urls). Swap/populate the value parameter with the offending config and keep the requirement in reason so the error output is accurate and consistent with other validators.
| 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 | ||
|
|
Copilot
AI
Feb 8, 2026
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.
This test is labeled as a compiler “integration” check, but it never exercises Compiler.validateWorkflowData/CompileWorkflowData; it calls validateNetworkFirewallConfig directly (and the compiler instance/strict-mode setting don’t affect the validation). Either rename the test to reflect that it’s a unit test for validateNetworkFirewallConfig, or restructure it to assert that compilation/validation fails via the compiler entrypoint to actually test the wiring in compiler.go.
The
allow-urlsnetwork configuration depends onssl-bump: truefor HTTPS content inspection, but this was only documented in the schema description without compile-time enforcement. Users could specifyallow-urlswithoutssl-bump, leading to silent failures.Changes
New validation:
pkg/workflow/network_firewall_validation.goallow-urlsrequiresssl-bump: trueCompiler integration: Added validation call in
validateWorkflowData()after network domain validationDocumentation constant: Added
DocsNetworkURLtopkg/constants/constants.gofor error messagesTest coverage: 13 test cases covering valid/invalid configurations and edge cases
Error Message
The validation runs regardless of
firewall.enabledstate to catch misconfiguration early.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.