BYOK: suppress COPILOT_GITHUB_TOKEN injection when COPILOT_PROVIDER_BASE_URL is set#35631
Conversation
…URL is set When engine.env contains COPILOT_PROVIDER_BASE_URL (BYOK mode), the Copilot execution step no longer injects COPILOT_GITHUB_TOKEN into the step environment. Previously the token was unconditionally set and could leak to the user's external provider via the api-proxy's fallback ****** causing HTTP 401 errors and constituting a credential-leak risk. Changes: - Detect isBYOKMode via engineEnvHasKey(COPILOT_PROVIDER_BASE_URL) at the top of GetExecutionSteps - Skip COPILOT_GITHUB_TOKEN injection when isBYOKMode is true - Remove COPILOT_GITHUB_TOKEN from AWF --exclude-env list in BYOK mode (nothing to hide when the token is not present) - Add TestCopilotEngineBYOKOmitsCopilotGitHubToken with three sub-tests: token absent with URL+key, token absent with URL only, token present in standard mode Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates Copilot engine execution so BYOK configurations using COPILOT_PROVIDER_BASE_URL do not automatically inject COPILOT_GITHUB_TOKEN, reducing the risk of forwarding a GitHub identity token to a third-party provider.
Changes:
- Adds BYOK detection in Copilot execution based on
COPILOT_PROVIDER_BASE_URL. - Suppresses default
COPILOT_GITHUB_TOKENenv injection and AWF core secret exclusion in BYOK mode. - Adds unit tests for BYOK and standard-mode token presence behavior.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/copilot_engine_execution.go |
Adds BYOK-mode gating for Copilot token injection and AWF exclude-env handling. |
pkg/workflow/copilot_engine_test.go |
Adds regression coverage for token omission in BYOK mode and presence in standard mode. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #35631 does not have the 'implementation' label, and default business logic additions (100) do not exceed the >100 threshold. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two blocking security issues need resolution before merge.
### Findings
1. (HIGH) engine.env merge re-injects COPILOT_GITHUB_TOKEN in BYOK mode — inline comment added at line 365.
The maps.Copy(env, workflowData.EngineConfig.Env) that runs after the conditional injection guard will silently overwrite the omission if the user has COPILOT_GITHUB_TOKEN set in engine.env (e.g. a leftover from a non-BYOK configuration). This defeats the entire purpose of the PR. Fix: delete(env, "COPILOT_GITHUB_TOKEN") after the merge when isBYOKMode.
2. (HIGH) BYOK detection predicate is inconsistent with GetSecretValidationStep — already flagged in the existing review thread at line 54.
isBYOKMode fires on COPILOT_PROVIDER_BASE_URL alone, but secret validation only skips when COPILOT_PROVIDER_API_KEY or COPILOT_PROVIDER_BEARER_TOKEN is present. A COPILOT_PROVIDER_BASE_URL-only workflow will skip token injection here but still fail the activation job's secret-validation step — or, if validation logic is later relaxed, will run with neither GitHub token nor BYOK credentials.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.6M
| "GITHUB_API_URL": "${{ github.api_url }}", | ||
| } | ||
| // Inject the GitHub token only when not in BYOK mode. The engine.env merge that | ||
| // happens later (maps.Copy(env, workflowData.EngineConfig.Env)) can still override |
There was a problem hiding this comment.
engine.env merge can silently re-inject COPILOT_GITHUB_TOKEN in BYOK mode: the maps.Copy(env, workflowData.EngineConfig.Env) that runs after this guard will overwrite the intentional omission if the user has COPILOT_GITHUB_TOKEN in engine.env — e.g. a leftover value from a prior non-BYOK config — forwarding the GitHub identity token to the third-party provider and recreating the exact credential-leak this PR aims to prevent.
💡 Suggested fix
After the maps.Copy merge, enforce the invariant unconditionally in BYOK mode:
maps.Copy(env, workflowData.EngineConfig.Env)
if isBYOKMode {
delete(env, "COPILOT_GITHUB_TOKEN")
}This makes the security property hold regardless of what the user put in engine.env, and avoids relying on the user knowing to clean up old config. The comment on line 362 currently acknowledges this gap ("can still override") but does not mitigate it.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — approving with two minor test-coverage suggestions.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Clean fix at the right abstraction level —
isBYOKModeis computed once and propagated consistently across all three mutation sites (env injection,--exclude-envlist, log message) - ✅ Regression guard in the standard-mode sub-test is exactly right; it would catch accidental removal of the token in the default path
- ✅
engineEnvHasKeyreturns false whenworkflowData/EngineConfigis nil — no null-pointer risk - ✅ Debug log line on the BYOK skip path aids future troubleshooting
- ✅ The
#nosec G101annotation is retained where needed
Suggested improvements (non-blocking)
--exclude-envpath not tested — env injection and AWF exclude-env are independent code paths; see inline comment on line 1903- User-override escape hatch not tested — the documented
engine.envoverride behaviour has no regression guard; see inline comment on line 1887
Residual exposure (known, deferred)
The PR correctly notes that the firewall-side fix (getAuthHeaders asymmetry) is a separate follow-up. The suppression here is the right first layer of defense.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.5M
| if !strings.Contains(stepContent, "COPILOT_GITHUB_TOKEN: ${{ secrets.COPILOT_GITHUB_TOKEN }}") { | ||
| t.Errorf("COPILOT_GITHUB_TOKEN should be present in standard (non-BYOK) mode, got:\n%s", stepContent) | ||
| } | ||
| }) |
There was a problem hiding this comment.
[/tdd] The copilotCoreSecrets/--exclude-env mutation site is not covered.
The BYOK subtests verify that COPILOT_GITHUB_TOKEN: is absent from the env block, but they don't check that --exclude-env COPILOT_GITHUB_TOKEN is also absent from the AWF command. These are two independent code paths — a regression in one would not be caught by the other.
💡 Suggested additions to each BYOK sub-test
// --exclude-env COPILOT_GITHUB_TOKEN must not appear in the AWF command in BYOK mode
if strings.Contains(stepContent, "--exclude-env COPILOT_GITHUB_TOKEN") {
t.Errorf("--exclude-env COPILOT_GITHUB_TOKEN should be absent in BYOK mode, got:\n%s", stepContent)
}And complement the standard-mode sub-test:
if !strings.Contains(stepContent, "--exclude-env COPILOT_GITHUB_TOKEN") {
t.Errorf("--exclude-env COPILOT_GITHUB_TOKEN should be present in standard mode")
}| } | ||
| }) | ||
|
|
||
| t.Run("COPILOT_GITHUB_TOKEN present when COPILOT_PROVIDER_BASE_URL is not set (standard mode)", func(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] Consider adding a sub-test for the documented user-override behaviour.
The PR description notes: "the engine.env maps.Copy that follows still lets users explicitly set [COPILOT_GITHUB_TOKEN] if needed." This intentional escape hatch isn't tested, so a future refactor could silently break it or inadvertently suppress it.
💡 Suggested sub-test skeleton
t.Run("COPILOT_GITHUB_TOKEN present when user explicitly sets it in BYOK engine.env", func(t *testing.T) {
workflowData := &WorkflowData{
Name: "test-workflow",
EngineConfig: &EngineConfig{
Env: map[string]string{
constants.CopilotProviderBaseURL: "(api.openai.com/redacted),
"COPILOT_GITHUB_TOKEN": "${{ secrets.MY_OVERRIDE }}",
},
},
}
steps := engine.GetExecutionSteps(workflowData, "/tmp/gh-aw/test.log")
stepContent := strings.Join([]string(steps[0]), "\n")
if !strings.Contains(stepContent, "COPILOT_GITHUB_TOKEN: ${{ secrets.MY_OVERRIDE }}") {
t.Errorf("explicit user override of COPILOT_GITHUB_TOKEN should survive BYOK suppression")
}
})
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics & Test Classification (3 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
The three sub-tests inside
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
|
|
1 similar comment
|
|
|
@copilot review all comments and reviews from copilot and GitHub-actions actor |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in commit 4cc6460.
|
When
engine: copilotis configured with a custom provider viaCOPILOT_PROVIDER_BASE_URL,COPILOT_GITHUB_TOKENwas unconditionally injected into the step env. The api-proxy's Copilot adapter falls back to this token asAuthorization: Bearerwhen BYOK key resolution fails, forwarding the GitHub identity token to the user's third-party host — a credential leak that also manifests as HTTP 401s.Changes
pkg/workflow/copilot_engine_execution.goGetExecutionSteps:COPILOT_GITHUB_TOKENinjection whenisBYOKModeis true; the engine.envmaps.Copythat follows still lets users explicitly set it if neededCOPILOT_GITHUB_TOKENfrom the AWF--exclude-envcore secret list in BYOK mode — no injection means nothing to hidepkg/workflow/copilot_engine_test.go—TestCopilotEngineBYOKOmitsCopilotGitHubTokenCOPILOT_PROVIDER_BASE_URL+COPILOT_PROVIDER_API_KEYCOPILOT_PROVIDER_BASE_URLonly (local/keyless provider)COPILOT_PROVIDER_BASE_URL(standard mode regression guard)The defense-in-depth fix on the
gh-aw-firewallside (getAuthHeaders()asymmetry vsgetModelsFetchConfig()) is a separate follow-up in that repo.