-
Notifications
You must be signed in to change notification settings - Fork 410
BYOK: suppress COPILOT_GITHUB_TOKEN injection when COPILOT_PROVIDER_BASE_URL is set #35631
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,11 @@ func (e *CopilotEngine) GetExecutionSteps(workflowData *WorkflowData, logFile st | |
| // Build copilot CLI arguments based on configuration | ||
| var copilotArgs []string | ||
| sandboxEnabled := isFirewallEnabled(workflowData) | ||
| // isBYOKMode is true when the user has set COPILOT_PROVIDER_BASE_URL in engine.env, | ||
| // which routes Copilot requests to a non-GitHub provider. In that mode the GitHub | ||
| // identity token (COPILOT_GITHUB_TOKEN) must NOT be injected into the step env: | ||
| // forwarding it to a third-party host would be a credential leak. | ||
| isBYOKMode := engineEnvHasKey(workflowData, constants.CopilotProviderBaseURL) | ||
| if sandboxEnabled { | ||
| // Simplified args for sandbox mode (AWF) | ||
| copilotArgs = []string{"--add-dir", "/tmp/gh-aw/", "--log-level", "all", "--log-dir", logsFolder} | ||
|
|
@@ -274,6 +279,14 @@ func (e *CopilotEngine) GetExecutionSteps(workflowData *WorkflowData, logFile st | |
| if customCommandScriptSetup != "" { | ||
| pathSetup = customCommandScriptSetup + "\n" + pathSetup | ||
| } | ||
| // Build the list of core secret var names to hide from the agent shell tools. | ||
| // In BYOK mode COPILOT_GITHUB_TOKEN is not injected into the step env at all, | ||
| // so there is nothing to exclude. Excluding it unconditionally would produce | ||
| // spurious --exclude-env flags when the token is absent. | ||
| var copilotCoreSecrets []string | ||
| if !isBYOKMode { | ||
| copilotCoreSecrets = []string{"COPILOT_GITHUB_TOKEN"} | ||
| } | ||
| command = BuildAWFCommand(AWFCommandConfig{ | ||
| EngineName: "copilot", | ||
| EngineCommand: engineCommand, | ||
|
|
@@ -295,7 +308,7 @@ func (e *CopilotEngine) GetExecutionSteps(workflowData *WorkflowData, logFile st | |
| PathSetup: pathSetup, | ||
| // Exclude every env var whose step-env value is a secret so the agent | ||
| // cannot read raw token values via bash tools (env / printenv). | ||
| ExcludeEnvVarNames: ComputeAWFExcludeEnvVarNames(workflowData, []string{"COPILOT_GITHUB_TOKEN"}), | ||
| ExcludeEnvVarNames: ComputeAWFExcludeEnvVarNames(workflowData, copilotCoreSecrets), | ||
|
pelikhan marked this conversation as resolved.
|
||
| }) | ||
| } else { | ||
| // Run copilot command without AWF wrapper. | ||
|
|
@@ -311,14 +324,19 @@ touch %s | |
| %s%s 2>&1 | tee %s`, AgentCLIStartMsPath, AgentStepSummaryPath, logFile, preCommandSetup, copilotCommand, logFile) | ||
| } | ||
|
|
||
| // Use COPILOT_GITHUB_TOKEN: when the copilot-requests feature is enabled, use the GitHub | ||
| // Actions token directly (${{ github.token }}). Otherwise use the COPILOT_GITHUB_TOKEN secret. | ||
| // COPILOT_GITHUB_TOKEN injection: in BYOK mode (COPILOT_PROVIDER_BASE_URL set), skip | ||
| // this entirely. The request goes to a third-party provider; forwarding the GitHub | ||
| // identity token would be a credential leak. The token is only needed for GitHub's | ||
| // own Copilot backend. When not in BYOK mode, use the GitHub Actions token when the | ||
| // copilot-requests feature is enabled, otherwise use the COPILOT_GITHUB_TOKEN secret. | ||
| // #nosec G101 -- These are NOT hardcoded credentials. They are GitHub Actions expression templates | ||
| // that the runtime replaces with actual values. The strings "${{ secrets.COPILOT_GITHUB_TOKEN }}" | ||
| // and "${{ github.token }}" are placeholders, not actual credentials. | ||
| var copilotGitHubToken string | ||
| useCopilotRequests := isFeatureEnabled(constants.CopilotRequestsFeatureFlag, workflowData) | ||
| if useCopilotRequests { | ||
| if isBYOKMode { | ||
| copilotExecLog.Print("Skipping COPILOT_GITHUB_TOKEN injection: BYOK mode active (COPILOT_PROVIDER_BASE_URL is set)") | ||
| } else if useCopilotRequests { | ||
| copilotGitHubToken = "${{ github.token }}" | ||
| copilotExecLog.Print("Using GitHub Actions token as COPILOT_GITHUB_TOKEN (copilot-requests feature enabled)") | ||
| } else { | ||
|
|
@@ -328,7 +346,6 @@ touch %s | |
| env := map[string]string{ | ||
| "XDG_CONFIG_HOME": "/home/runner", | ||
| "COPILOT_AGENT_RUNNER_TYPE": "STANDALONE", | ||
| "COPILOT_GITHUB_TOKEN": copilotGitHubToken, | ||
| // Override GITHUB_STEP_SUMMARY with a path that exists inside the sandbox. | ||
| // The runner's original path is unreachable within the AWF isolated filesystem; | ||
| // we create this file before the agent starts and append it to the real | ||
|
|
@@ -344,6 +361,12 @@ touch %s | |
| "GITHUB_SERVER_URL": "${{ github.server_url }}", | ||
| "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 | ||
|
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. engine.env merge can silently re-inject 💡 Suggested fixAfter the 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 |
||
| // or nullify this if the user explicitly sets COPILOT_GITHUB_TOKEN in engine.env. | ||
| if !isBYOKMode { | ||
| env["COPILOT_GITHUB_TOKEN"] = copilotGitHubToken | ||
| } | ||
| injectWorkflowCallNetworkAllowedEnv(env, workflowData) | ||
|
|
||
| // When copilot-requests feature is enabled, set S2STOKENS=true to allow the Copilot CLI | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1831,6 +1831,102 @@ func TestCopilotEngineEnvOverridesTokenExpression(t *testing.T) { | |
| }) | ||
| } | ||
|
|
||
| // TestCopilotEngineBYOKOmitsCopilotGitHubToken verifies that COPILOT_GITHUB_TOKEN is | ||
| // NOT injected into the execution step env when BYOK mode is active | ||
| // (i.e. COPILOT_PROVIDER_BASE_URL is set in engine.env). Forwarding the GitHub identity | ||
| // token to a third-party provider would be a credential leak. | ||
| func TestCopilotEngineBYOKOmitsCopilotGitHubToken(t *testing.T) { | ||
| engine := NewCopilotEngine() | ||
|
|
||
| t.Run("COPILOT_GITHUB_TOKEN absent when COPILOT_PROVIDER_BASE_URL is set (BYOK)", func(t *testing.T) { | ||
| workflowData := &WorkflowData{ | ||
| Name: "test-workflow", | ||
| EngineConfig: &EngineConfig{ | ||
| Env: map[string]string{ | ||
| constants.CopilotProviderBaseURL: "https://api.openai.com/v1", | ||
| constants.CopilotProviderAPIKey: "${{ secrets.PROVIDER_API_KEY }}", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| steps := engine.GetExecutionSteps(workflowData, "/tmp/gh-aw/test.log") | ||
| if len(steps) != 1 { | ||
| t.Fatalf("Expected 1 step, got %d", len(steps)) | ||
| } | ||
|
|
||
| stepContent := strings.Join([]string(steps[0]), "\n") | ||
|
|
||
| // COPILOT_GITHUB_TOKEN must not appear at all — not even its default expression. | ||
| if strings.Contains(stepContent, "COPILOT_GITHUB_TOKEN:") { | ||
| t.Errorf("COPILOT_GITHUB_TOKEN should be absent in BYOK mode, got:\n%s", stepContent) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("COPILOT_GITHUB_TOKEN absent with COPILOT_PROVIDER_BASE_URL only (no API key)", func(t *testing.T) { | ||
| workflowData := &WorkflowData{ | ||
| Name: "test-workflow", | ||
| EngineConfig: &EngineConfig{ | ||
| Env: map[string]string{ | ||
| constants.CopilotProviderBaseURL: "http://localhost:11434/v1", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| steps := engine.GetExecutionSteps(workflowData, "/tmp/gh-aw/test.log") | ||
| if len(steps) != 1 { | ||
| t.Fatalf("Expected 1 step, got %d", len(steps)) | ||
| } | ||
|
|
||
| stepContent := strings.Join([]string(steps[0]), "\n") | ||
|
|
||
| if strings.Contains(stepContent, "COPILOT_GITHUB_TOKEN:") { | ||
| t.Errorf("COPILOT_GITHUB_TOKEN should be absent in BYOK mode (local provider), got:\n%s", stepContent) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("COPILOT_GITHUB_TOKEN present when COPILOT_PROVIDER_BASE_URL is not set (standard mode)", func(t *testing.T) { | ||
|
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. [/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 skeletont.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")
}
}) |
||
| workflowData := &WorkflowData{ | ||
| Name: "test-workflow", | ||
| EngineConfig: &EngineConfig{}, | ||
| } | ||
|
|
||
| steps := engine.GetExecutionSteps(workflowData, "/tmp/gh-aw/test.log") | ||
| if len(steps) != 1 { | ||
| t.Fatalf("Expected 1 step, got %d", len(steps)) | ||
| } | ||
|
|
||
| stepContent := strings.Join([]string(steps[0]), "\n") | ||
|
|
||
| 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) | ||
| } | ||
| }) | ||
|
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. [/tdd] The The BYOK subtests verify that 💡 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("AWF command omits --exclude-env COPILOT_GITHUB_TOKEN in BYOK mode", func(t *testing.T) { | ||
| workflowData := &WorkflowData{ | ||
| Name: "test-workflow", | ||
| EngineConfig: &EngineConfig{ | ||
| Env: map[string]string{ | ||
| constants.CopilotProviderBaseURL: "http://localhost:11434/v1", | ||
| }, | ||
| }, | ||
| SandboxConfig: &SandboxConfig{ | ||
| Agent: &AgentSandboxConfig{Type: SandboxTypeAWF}, | ||
| }, | ||
| } | ||
|
|
||
| steps := engine.GetExecutionSteps(workflowData, "/tmp/gh-aw/test.log") | ||
| if len(steps) != 1 { | ||
| t.Fatalf("Expected 1 step, got %d", len(steps)) | ||
| } | ||
|
|
||
| stepContent := strings.Join([]string(steps[0]), "\n") | ||
| if strings.Contains(stepContent, "--exclude-env COPILOT_GITHUB_TOKEN") { | ||
| t.Errorf("AWF command should not exclude COPILOT_GITHUB_TOKEN in BYOK mode, got:\n%s", stepContent) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestCopilotEngineSetsDummyAPIKey(t *testing.T) { | ||
| engine := NewCopilotEngine() | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.