-
Notifications
You must be signed in to change notification settings - Fork 429
Propagate Copilot BYOK provider hosts into threat-detection allowlists #39653
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -865,6 +865,96 @@ Test workflow with GHE data residency api-target and threat detection. | |
| } | ||
| } | ||
|
|
||
| func TestCopilotProviderBaseURLInThreatDetectionStep(t *testing.T) { | ||
| workflow := `--- | ||
| on: push | ||
| permissions: | ||
| contents: read | ||
| issues: read | ||
| pull-requests: read | ||
| engine: | ||
| id: copilot | ||
| env: | ||
| COPILOT_PROVIDER_BASE_URL: ${{ secrets.PROVIDER_BASE_URL }} | ||
| network: | ||
| allowed: | ||
| - defaults | ||
| - llm.corp.example.com | ||
| strict: false | ||
| safe-outputs: | ||
| create-issue: | ||
| --- | ||
|
|
||
| # Test Workflow | ||
|
|
||
| Test workflow with COPILOT_PROVIDER_BASE_URL in engine.env and provider host in network.allowed. | ||
| ` | ||
|
|
||
| tmpDir := testutil.TempDir(t, "copilot-provider-threat-detection-test") | ||
| testFile := filepath.Join(tmpDir, "test-workflow.md") | ||
| if err := os.WriteFile(testFile, []byte(workflow), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| compiler := NewCompiler() | ||
| if err := compiler.CompileWorkflow(testFile); err != nil { | ||
| t.Fatalf("Failed to compile workflow: %v", err) | ||
| } | ||
|
|
||
| lockFile := stringutil.MarkdownToLockFile(testFile) | ||
| lockContent, err := os.ReadFile(lockFile) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read lock file: %v", err) | ||
| } | ||
| lockStr := string(lockContent) | ||
|
|
||
| requiredDomain := "llm.corp.example.com" | ||
| allowDomainsPrefix := `"allowDomains":[` | ||
| allowDomainsPrefixEscaped := `\"allowDomains\":[` | ||
| if !strings.Contains(lockStr, allowDomainsPrefix) { | ||
| allowDomainsPrefix = allowDomainsPrefixEscaped | ||
| } | ||
|
|
||
| remaining := lockStr | ||
| occurrenceIdx := 0 | ||
| for { | ||
| idx := strings.Index(remaining, allowDomainsPrefix) | ||
| if idx < 0 { | ||
| break | ||
| } | ||
| occurrenceIdx++ | ||
| arrayStart := idx + len(allowDomainsPrefix) | ||
| arrayEnd := strings.Index(remaining[arrayStart:], "]") | ||
| if arrayEnd < 0 { | ||
| arrayEnd = len(remaining) - arrayStart | ||
| } | ||
| section := remaining[arrayStart : arrayStart+arrayEnd] | ||
| if !strings.Contains(section, `"`+requiredDomain+`"`) && !strings.Contains(section, `\"`+requiredDomain+`\"`) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test asserts BYOK domain is present in ALL 💡 Detail and fixThe loop fails the test if ANY The test should verify that the agent job and detection job include the domain, not that every single // Look for allowDomains only inside the agent and detection job blocks.
// Or: check that at least 2 specific named occurrences contain the domain,
// rather than asserting it for every occurrence indiscriminately.
if !strings.Contains(lockStr, `"allowDomains":["llm.corp.example.com`) &&
!strings.Contains(lockStr, `"llm.corp.example.com"`) {
t.Error("BYOK provider domain not found in any allowDomains array")
}Alternatively, parse the lock file as YAML, find the agent and detection steps by step ID, and check those |
||
| t.Errorf("allowDomains occurrence #%d is missing BYOK provider domain %q.\nSection: %s", occurrenceIdx, requiredDomain, section) | ||
| } | ||
| remaining = remaining[arrayStart+arrayEnd:] | ||
| } | ||
|
|
||
| if occurrenceIdx < 2 { | ||
| t.Errorf("Expected at least 2 allowDomains occurrences (main agent + threat detection), found %d", occurrenceIdx) | ||
| } | ||
|
|
||
| lines := strings.Split(lockStr, "\n") | ||
| var domainsLine string | ||
| for _, line := range lines { | ||
| if strings.Contains(line, "GH_AW_ALLOWED_DOMAINS:") { | ||
| domainsLine = line | ||
| break | ||
| } | ||
| } | ||
| if domainsLine == "" { | ||
| t.Fatal("GH_AW_ALLOWED_DOMAINS not found in compiled lock file") | ||
| } | ||
| if !strings.Contains(domainsLine, requiredDomain) { | ||
| t.Errorf("Expected BYOK provider hostname in GH_AW_ALLOWED_DOMAINS.\nLine: %s", domainsLine) | ||
| } | ||
| } | ||
|
|
||
| // TestAllowedDomainsUnionWithNetworkConfig tests that safe-outputs.allowed-domains | ||
| // is unioned with network.allowed and always includes localhost and github.com | ||
| func TestAllowedDomainsUnionWithNetworkConfig(t *testing.T) { | ||
|
|
||
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.
[/tdd] The hand-rolled
allowDomainsscan — handling both unescaped and JSON-escaped variants, then manually slicing to find array boundaries — is fragile. If the lock file format changes (e.g., pretty-printed JSON, double-escaped), this silently passes without checking anything meaningful.💡 Alternative approach
Parse the AWF JSON blob from the lock file with
encoding/jsonor use a helper already in the test suite to extract the firewall config, then assert on the parsedallowDomainsslice directly. If structured parsing is overkill here, at minimum useassert.Contains(lockStr,"allowDomains":[+ ... +"llm.corp.example.com")on a known canonical form rather than the dual-format branching.The
occurrenceIdx >= 2bound check would also benefit fromassert.Equal(t, 2, occurrenceIdx)to catch regressions where the lock file grows unexpected extraallowDomainsblobs.