Fix external threat detection to honor custom engine base URLs and allowlist propagation#46077
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.
Pull request overview
Fixes external threat detection to inherit custom provider endpoints and propagate related network domains.
Changes:
- Merges main and detection-specific engine environments.
- Generalizes custom provider URL detection.
- Adds regression coverage for endpoint and allowlist propagation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/threat_detection_helpers.go |
Adds environment cloning/merging and generalized URL checks. |
pkg/workflow/threat_detection_external.go |
Applies effective engine configuration to external detection. |
pkg/workflow/threat_detection_test.go |
Tests custom endpoints and allowed domains. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Medium
| hasCustomTarget := extractAPITargetHost(data, "OPENAI_BASE_URL") != "" || | ||
| extractAPITargetHost(data, "ANTHROPIC_BASE_URL") != "" || | ||
| extractAPITargetHost(data, constants.CopilotProviderBaseURL) != "" |
| } | ||
| threatDetectionData.EngineConfig = cloneThreatDetectionEngineConfig(engineID, data.SafeOutputs.ThreatDetection.EngineConfig) | ||
| } | ||
| threatDetectionData.EngineConfig.Env = mergeThreatDetectionEngineEnv(data, threatDetectionData.EngineConfig.Env) |
| if !strings.Contains(stepsContent, "apiProxy") { | ||
| t.Fatalf("expected external detector AWF config to include api-proxy target; got:\n%s", stepsContent) | ||
| } | ||
| if !strings.Contains(stepsContent, "llm-router.internal.example.com") { | ||
| t.Fatalf("expected external detector AWF config to include host from OPENAI_BASE_URL; got:\n%s", stepsContent) | ||
| } |
This comment has been minimized.
This comment has been minimized.
|
Great work on tracking down this subtle config-inheritance bug in the external threat detection path! The PR is well-scoped, clearly described, and ships focused regression tests alongside the fix. The engine-env merge logic and the generalised allowlist-propagation trigger both look like the right approach. This looks ready for review.
|
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
Skills-Based Review 🧠Applied 📋 Key Themes & HighlightsKey Themes
Positive Highlights
@copilot please address the review comments above.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
{"pull_request_number":46077,"event":"REQUEST_CHANGES","body":"### Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two aliasing correctness issues and one test coverage gap.
📋 Key Themes & Highlights
Key Themes
- Shallow-copy aliasing risk in
cloneThreatDetectionEngineConfig: map/slice fields (Config,Args) are not deep-copied, so mutations via the clone would corrupt the source engine config. - Partial aliasing protection in
mergeThreatDetectionEngineEnv: the empty-main-env branch returnsdetectionEnvby reference, undermining the aliasing-safety guarantee stated in the function doc. - Missing assertion in
TestExternalDetectorInheritsOpenAIBaseURL: the detection-specificCUSTOM_FLAGenv var is set up but never asserted, leaving the merge-precedence contract unchecked.
Positive Highlights
- ✅ Root cause correctly identified and fixed: env inheritance now flows main → detection with detection keys taking precedence.
- ✅ Allowlist trigger generalised cleanly across all three provider URL constants.
- ✅ The
maps.Clonein the empty-detectionEnv branch shows the right intent — just needs to be applied consistently. - ✅ PR description is detailed and maps directly to the fix.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 36.8 AIC · ⌖ 4.9 AIC · ⊞ 6.7K
Comment /matt to run again
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…rengthen test assertions Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all three mandatory review items in commit
Two new regression tests added: All |
🤖 PR Triage
Score breakdown: Impact 35 + Urgency 20 + Quality 12 Rationale: Threat detection silently fell back to default engine endpoints when custom
|
|
🎉 This pull request is included in a new release. Release: |
Threat detection in the external detector path could not use custom OpenAI-compatible endpoints configured via
engine.env.OPENAI_BASE_URL/ANTHROPIC_BASE_URL, because detection rebuiltEngineConfigwithout inherited env and AWF api-proxy target derivation fell back to defaults. The same path also under-propagatednetwork.allowedadditions by only keying offCOPILOT_PROVIDER_BASE_URL.Engine config inheritance in external detection
Allowlist propagation trigger generalized
getThreatDetectionAdditionalAllowedDomainsnow enables additional-domain propagation when any custom provider base URL is configured:OPENAI_BASE_URLANTHROPIC_BASE_URLCOPILOT_PROVIDER_BASE_URLRefactor for consistency
Regression coverage
OPENAI_BASE_URLinto AWF api-proxy target config