diff --git a/pkg/workflow/compiler_github_mcp_steps.go b/pkg/workflow/compiler_github_mcp_steps.go index 9d22add76b9..8e7f87ecff4 100644 --- a/pkg/workflow/compiler_github_mcp_steps.go +++ b/pkg/workflow/compiler_github_mcp_steps.go @@ -23,6 +23,23 @@ func githubLockdownDetectionStepEnabled(data *WorkflowData) bool { if enclaveDynamicRepositoryPolicyEnabled(data) { return true } + // A static enclave-only GitHub backend still needs the target repository's visibility + // for its safe-outputs write-sink policy, even when the primary agent has no GitHub + // MCP access at all (tools.github: false). Generate the step in that case too, so the + // write-sink policy's sink-visibility field always has a valid producer step. + // + // This intentionally checks staticEnclaveWriteSinkGuardPolicy(data) != nil rather than + // the broader githubBackendIsEnclaveOnly(data) helper: the latter is true whenever an + // enclave-only backend exists at all, even one with no allowed repos (in which case + // staticEnclaveWriteSinkGuardPolicy returns nil because there is no write-sink policy to + // populate). Using the broader helper here would generate this step needlessly for those + // repo-less configurations. githubBackendIsEnclaveOnly remains the right check for the + // separate question of "does the primary GitHub MCP server's own guard policy come from + // this step's outputs" (see githubGuardPoliciesFromStep and collectMCPEnvironmentVariables), + // which is unrelated to whether the step itself needs to exist for sink-visibility. + if staticEnclaveWriteSinkGuardPolicy(data) != nil { + return true + } if githubTool, hasGitHub := data.Tools["github"]; hasGitHub { return githubTool != false } diff --git a/pkg/workflow/enclave_github_proxy_test.go b/pkg/workflow/enclave_github_proxy_test.go index ec738befe83..97df7876bd4 100644 --- a/pkg/workflow/enclave_github_proxy_test.go +++ b/pkg/workflow/enclave_github_proxy_test.go @@ -5,6 +5,7 @@ package workflow import ( "os" "path/filepath" + "regexp" "strings" "testing" @@ -358,12 +359,18 @@ Read the private repository's issues through the enclave. var doc any require.NoError(t, yaml.Unmarshal(lockBytes, &doc), "generated lock file must be valid YAML") - // The determine-automatic-lockdown step is not generated, so its outputs must not be - // referenced by the server-level guard policy or the gateway step environment. - assert.NotContains(t, lock, "Determine automatic lockdown mode") + // The determine-automatic-lockdown step IS generated in this configuration, solely to + // supply the target repository's visibility for the static enclave's write-sink policy + // (GH_AW_SINK_VISIBILITY). Its min_integrity/repos outputs must still not be referenced, + // because the server-level guard policy for this enclave-only backend is derived + // statically from the enclave declaration, not from the step outputs. + assert.Contains(t, lock, "Determine automatic lockdown mode") + assert.Contains(t, lock, "id: determine-automatic-lockdown") + assert.Contains(t, lock, "GH_AW_SINK_VISIBILITY: ${{ steps.determine-automatic-lockdown.outputs.visibility }}") assert.NotContains(t, lock, "$GITHUB_MCP_GUARD_MIN_INTEGRITY") assert.NotContains(t, lock, "$GITHUB_MCP_GUARD_REPOS") assert.NotContains(t, lock, "GITHUB_MCP_GUARD_MIN_INTEGRITY: ${{ steps.determine-automatic-lockdown.outputs.min_integrity }}") + assert.NotContains(t, lock, "GITHUB_MCP_GUARD_REPOS: ${{ steps.determine-automatic-lockdown.outputs.repos }}") // The server-level guard policy mirrors the enclave identity policy, so it never // broadens access beyond what the enclave identity already allows. @@ -372,6 +379,9 @@ Read the private repository's issues through the enclave. assert.Contains(t, lock, `"write-sink"`) assert.Contains(t, lock, `"private:octo-org/private-service"`) assert.Contains(t, lock, `"sink-visibility": "${GH_AW_SINK_VISIBILITY}"`) + // The sink-visibility env var must reference the step actually generated above — no + // dangling steps..outputs.* reference. + assert.Contains(t, lock, "steps.determine-automatic-lockdown.outputs.visibility") assert.Contains(t, lock, `"${AWF_ENCLAVE_GITHUB_MCP_AGENT_ID}":{"servers":["github"],"tools":{"github":["list_issues","issue_read"]},"allow-only":{"min-integrity":"none","repos":["octo-org/private-service"]}}`) assert.Contains(t, lock, `export GH_AW_MCP_GITHUB_CHECK_AGENT_ID="${AWF_ENCLAVE_GITHUB_MCP_AGENT_ID}"`) assert.NotContains(t, lock, `"${MCP_GATEWAY_AGENT_ID}":{"servers":["awf-enclave","github"`) @@ -382,6 +392,138 @@ Read the private repository's issues through the enclave. assert.Contains(t, lock, `"forcePublicRepos": false`) } +// stepOutputRefPattern matches steps..outputs. references, capturing the step id. +var stepOutputRefPattern = regexp.MustCompile(`steps\.([A-Za-z0-9_-]+)\.outputs\.[A-Za-z0-9_-]+`) + +// assertNoDanglingStepOutputReferences verifies that every `steps..outputs.*` reference +// in the generated lock file corresponds to a step id that is actually emitted in the same +// job. For references made from a step field, the producer step must also appear earlier in +// that job's step list. This guards against the class of bug described in gh-aw#60336, where a +// consumer (e.g. an environment variable or guard policy) references a step's outputs even +// though the producer step itself was never generated, expanding to an empty string at runtime. +func assertNoDanglingStepOutputReferences(t *testing.T, lock string) { + t.Helper() + + var workflow map[string]any + require.NoError(t, yaml.Unmarshal([]byte(lock), &workflow), "generated lock file must be valid YAML") + + jobs, ok := workflow["jobs"].(map[string]any) + require.True(t, ok, "generated lock file must contain jobs") + + for jobID, jobValue := range jobs { + job, ok := jobValue.(map[string]any) + require.True(t, ok, "job %q must be an object", jobID) + + steps, _ := job["steps"].([]any) + allStepIDs := make(map[string]bool) + for _, stepValue := range steps { + step, ok := stepValue.(map[string]any) + if !ok { + continue + } + if stepID, ok := step["id"].(string); ok { + allStepIDs[stepID] = true + } + } + + jobBytes, err := yaml.Marshal(jobValue) + require.NoError(t, err, "job %q must marshal for step reference checks", jobID) + for _, match := range stepOutputRefPattern.FindAllStringSubmatch(string(jobBytes), -1) { + stepID := match[1] + assert.True(t, allStepIDs[stepID], "job %q reference %q has no corresponding emitted step id %q", jobID, match[0], stepID) + } + + previousStepIDs := make(map[string]bool) + for stepIndex, stepValue := range steps { + step, ok := stepValue.(map[string]any) + if !ok { + continue + } + + stepBytes, err := yaml.Marshal(stepValue) + require.NoError(t, err, "job %q step %d must marshal for step reference checks", jobID, stepIndex) + for _, match := range stepOutputRefPattern.FindAllStringSubmatch(string(stepBytes), -1) { + stepID := match[1] + assert.True(t, previousStepIDs[stepID], "job %q step %d reference %q must refer to a previously emitted step id %q", jobID, stepIndex, match[0], stepID) + } + + if stepID, ok := step["id"].(string); ok { + previousStepIDs[stepID] = true + } + } + } +} + +// TestCompileStaticEnclaveOnlyGitHubDisabledSinkVisibility is a regression test for +// gh-aw#60336: a static GitHub enclave combined with `tools.github: false` and safe-outputs +// must not emit GH_AW_SINK_VISIBILITY (or any other value) referencing the +// determine-automatic-lockdown step unless that step is actually generated. Before the fix, +// the compiler emitted `GH_AW_SINK_VISIBILITY: ${{ steps.determine-automatic-lockdown.outputs.visibility }}` +// and `"sink-visibility": "${GH_AW_SINK_VISIBILITY}"` even though no `determine-automatic-lockdown` +// step existed, so the env var resolved to an empty string and MCP Gateway rejected the +// resulting `sink-visibility: ""` as invalid. +func TestCompileStaticEnclaveOnlyGitHubDisabledSinkVisibility(t *testing.T) { + tmp := t.TempDir() + workflowPath := filepath.Join(tmp, "roadmap-triage-enclave.md") + content := `--- +on: workflow_dispatch +strict: false +network: defaults +engine: copilot +tools: + github: false +enclaves: + - agent: + model: gpt-5 + tools: + github: + allowed: [list_issues, issue_read] + allowed-repos: [githubnext/gh-aw-enclave-demo-private] + min-integrity: none + repos: + - repo: githubnext/gh-aw-enclave-demo-private + sensitivity: confidential +safe-outputs: + add-comment: + max: 1 +--- + +Read the private repository's issues through the enclave and post a triage comment. +` + require.NoError(t, os.WriteFile(workflowPath, []byte(content), 0o600)) + compiler := NewCompiler() + compiler.SetSkipValidation(true) + require.NoError(t, compiler.CompileWorkflow(workflowPath)) + lockBytes, err := os.ReadFile(strings.TrimSuffix(workflowPath, ".md") + ".lock.yml") + require.NoError(t, err) + lock := string(lockBytes) + + var doc any + require.NoError(t, yaml.Unmarshal(lockBytes, &doc), "generated lock file must be valid YAML") + + // General invariant: every steps..outputs.* reference must correspond to an + // emitted step id somewhere in the generated workflow. + assertNoDanglingStepOutputReferences(t, lock) + + // The determine-automatic-lockdown step and its producer for GH_AW_SINK_VISIBILITY must + // either both be present, or both be absent — never a dangling reference to one without + // the other. Here, the step IS generated (solely to supply visibility for the static + // enclave's write-sink policy). + assert.Contains(t, lock, "id: determine-automatic-lockdown") + assert.Contains(t, lock, "GH_AW_SINK_VISIBILITY: ${{ steps.determine-automatic-lockdown.outputs.visibility }}") + assert.Contains(t, lock, `"sink-visibility": "${GH_AW_SINK_VISIBILITY}"`) + + // The primary agent must have no GitHub access: its own guard policy must not be + // automatically derived from the lockdown step's min_integrity/repos outputs. + assert.NotContains(t, lock, "GITHUB_MCP_GUARD_MIN_INTEGRITY: ${{ steps.determine-automatic-lockdown.outputs.min_integrity }}") + assert.NotContains(t, lock, "GITHUB_MCP_GUARD_REPOS: ${{ steps.determine-automatic-lockdown.outputs.repos }}") + + // The enclave identity retains its scoped repository access, and the static write-sink + // retains its narrow accepted secrecy labels. + assert.Contains(t, lock, `"private:githubnext/gh-aw-enclave-demo-private"`) + assert.Contains(t, lock, `"allow-only":{"min-integrity":"none","repos":["githubnext/gh-aw-enclave-demo-private"]}`) +} + // TestBuildMCPGatewayConfigForcePublicReposForStaticEnclave verifies that the gateway's // runtime public-repos override is disabled when the GitHub MCP server exists solely to // serve a static enclave agent identity, and left at its default when the primary agent @@ -424,7 +566,11 @@ func TestGitHubGuardPoliciesFromStepSkipsEnclaveOnlyBackend(t *testing.T) { delete(data.Tools, "github") data.ExplicitlyDisabledTools = map[string]struct{}{"github": {}} - assert.False(t, githubLockdownDetectionStepEnabled(data)) + // The determine-automatic-lockdown step is still generated for an enclave-only static + // backend, solely to supply GH_AW_SINK_VISIBILITY for the write-sink policy — but its + // min_integrity/repos outputs must never drive the primary GitHub MCP server's guard + // policy, which stays derived statically from the enclave declaration. + assert.True(t, githubLockdownDetectionStepEnabled(data)) assert.False(t, githubGuardPoliciesFromStep(data, nil)) assert.True(t, githubBackendIsStaticEnclaveDelegationOnly(data)) diff --git a/pkg/workflow/mcp_environment.go b/pkg/workflow/mcp_environment.go index e2abf65f4f6..8514ecfddbe 100644 --- a/pkg/workflow/mcp_environment.go +++ b/pkg/workflow/mcp_environment.go @@ -94,24 +94,33 @@ func collectMCPEnvironmentVariables(tools map[string]any, mcpTools []string, wor envVars["GITHUB_MCP_SERVER_TOKEN"] = effectiveToken } - // Add guard policy env vars if the determine-automatic-lockdown step will be generated. - // Skip only when guard policy is already explicitly set — in that case, the - // determine-automatic-lockdown step is not generated. + // Add guard policy env vars if the determine-automatic-lockdown step will be generated + // and its outputs are actually used to render the primary GitHub MCP server's guard + // policy. Skip when guard policy is already explicitly set, or when the GitHub MCP + // server exists solely to serve an enclave identity — enclave-only backends always + // derive their guard policy from the enclave declaration, never from the step outputs + // (see staticEnclaveGitHubGuardPolicies / dynamicEnclaveGitHubGuardPolicies), even + // though the step may still be generated to supply GH_AW_SINK_VISIBILITY. // Security: Pass step outputs through environment variables to prevent template injection. guardPoliciesExplicit := len(getGitHubGuardPolicies(toolConfig)) > 0 - if githubToolEnabledInTools && !guardPoliciesExplicit && githubLockdownDetectionStepEnabled(workflowData) { + if githubToolEnabledInTools && !guardPoliciesExplicit && !githubBackendIsEnclaveOnly(workflowData) && githubLockdownDetectionStepEnabled(workflowData) { envVars["GITHUB_MCP_GUARD_MIN_INTEGRITY"] = "${{ steps.determine-automatic-lockdown.outputs.min_integrity }}" envVars["GITHUB_MCP_GUARD_REPOS"] = "${{ steps.determine-automatic-lockdown.outputs.repos }}" } } // Emit GH_AW_SINK_VISIBILITY for all workflows where the determine-automatic-lockdown step - // runs (i.e., any workflow with a GitHub tool or a dynamic enclave). This avoids + // runs (i.e., any workflow with a GitHub tool, or an enclave-only GitHub backend — static + // or dynamic — whose write-sink policy needs the destination visibility). This avoids // embedding a ${{ }} expression directly in the run: heredoc, which zizmor flags as // template injection. The value is the raw step output (no toJSON), and the surrounding // JSON double-quotes in the heredoc produce a valid JSON string at runtime: // "sink-visibility": "${GH_AW_SINK_VISIBILITY}" → "sink-visibility": "public" - if githubToolEnabledInTools || enclaveDynamicRepositoryPolicyEnabled(workflowData) { + // githubBackendIsEnclaveOnly is the same shared helper used to decide whether the primary + // agent's automatic guard-policy env vars apply, keeping both decisions in sync. + // Gating on githubLockdownDetectionStepEnabled ensures this never references a step that + // isn't actually generated (a dangling steps..outputs.* reference). + if (githubToolEnabledInTools || githubBackendIsEnclaveOnly(workflowData)) && githubLockdownDetectionStepEnabled(workflowData) { envVars[sinkVisibilityEnvVar] = "${{ steps.determine-automatic-lockdown.outputs.visibility }}" } if enclaveDynamicRepositoryPolicyEnabled(workflowData) { diff --git a/pkg/workflow/mcp_github_config.go b/pkg/workflow/mcp_github_config.go index c52273dd3ac..4520eb1b247 100644 --- a/pkg/workflow/mcp_github_config.go +++ b/pkg/workflow/mcp_github_config.go @@ -141,6 +141,15 @@ func githubBackendIsStaticEnclaveDelegationOnly(workflowData *WorkflowData) bool return enclaveGitHubIssuesEnabled(workflowData) && !primaryGitHubMCPEnabled(workflowData) } +// githubBackendIsEnclaveOnly reports whether the GitHub MCP server is rendered solely to +// serve an enclave agent identity (static or dynamic), with no primary-agent GitHub access. +// Backends in this state always derive their guard/write-sink policies statically from the +// enclave declaration, never from the determine-automatic-lockdown step outputs, even though +// that step may still be generated for them solely to supply GH_AW_SINK_VISIBILITY. +func githubBackendIsEnclaveOnly(workflowData *WorkflowData) bool { + return githubBackendIsStaticEnclaveDelegationOnly(workflowData) || githubBackendIsDynamicDelegationOnly(workflowData) +} + func githubGuardPoliciesFromStep(workflowData *WorkflowData, explicitGuardPolicies map[string]any) bool { if len(explicitGuardPolicies) > 0 { return false @@ -148,11 +157,18 @@ func githubGuardPoliciesFromStep(workflowData *WorkflowData, explicitGuardPolici if workflowData == nil { return true } + if githubBackendIsEnclaveOnly(workflowData) { + return false + } githubTool, hasGitHub := workflowData.Tools["github"] if !hasGitHub { - // Default-tool resolution removes the "github" key when tools.github is false, so an - // absent key can still mean the GitHub MCP server is rendered for enclave delegation. - // Only reference the lockdown step outputs when that step is actually generated. + // Default-tool resolution removes the "github" key when tools.github is false. The + // static/dynamic enclave-only backends that could otherwise cause + // githubLockdownDetectionStepEnabled to return true here (via its own enclave checks) + // were already excluded above, so deferring to it below only covers the remaining + // case: a plain (non-enclave) GitHub MCP server whose "github" key was removed by + // default-tool resolution. This keeps the result in lockstep with whether the + // determine-automatic-lockdown step is actually generated. return githubLockdownDetectionStepEnabled(workflowData) } return githubTool != false