diff --git a/.changeset/patch-cli-proxy-default-policy-respect-repo-visibility.md b/.changeset/patch-cli-proxy-default-policy-respect-repo-visibility.md new file mode 100644 index 00000000000..6c4e2805b3c --- /dev/null +++ b/.changeset/patch-cli-proxy-default-policy-respect-repo-visibility.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Fixed the CLI proxy default policy so public repositories no longer default to `repos="all"`, while preserving the explicit private-to-public opt-in. diff --git a/actions/setup/js/determine_automatic_lockdown.cjs b/actions/setup/js/determine_automatic_lockdown.cjs index 2bde6ffc068..9615190c02f 100644 --- a/actions/setup/js/determine_automatic_lockdown.cjs +++ b/actions/setup/js/determine_automatic_lockdown.cjs @@ -8,9 +8,14 @@ const { getErrorMessage } = require("./error_helpers.cjs"); * This step always sets `min_integrity` and `repos` outputs so that the GitHub MCP * `guard-policies` block is never populated with empty values: * - * - Public repositories: defaults to `min_integrity=approved`, `repos=all` + * - Public repositories: defaults to `min_integrity=approved`, `repos=public` * - Private/internal repositories: defaults to `min_integrity=none`, `repos=all` * + * When `GH_AW_PRIVATE_TO_PUBLIC_FLOWS=allow` is set (from `tools.github.private-to-public-flows: + * allow` in the workflow frontmatter), the `repos` default for public repositories is overridden + * to `all`, matching the behavior of private repositories, because the workflow author has + * explicitly opted in to cross-visibility data flows. + * * Whether a field is "already configured" is determined by the environment variables * GH_AW_GITHUB_MIN_INTEGRITY and GH_AW_GITHUB_REPOS, which are set at compile time * from the workflow's tools.github guard policy configuration. Pre-configured values @@ -55,7 +60,11 @@ async function determineAutomaticLockdown(github, context, core) { // Private/internal repos default to min_integrity=none; public repos to approved. // Either way, always emit outputs so guard-policies values are never empty. const defaultMinIntegrity = isPrivate ? "none" : "approved"; - const defaultRepos = "all"; + // Public repos default to repos=public to block access to private repos unless the + // workflow author has explicitly opted in via private-to-public-flows: allow. + // Private/internal repos default to repos=all (no cross-visibility restriction). + const privateToPublicFlows = process.env.GH_AW_PRIVATE_TO_PUBLIC_FLOWS || ""; + const defaultRepos = isPrivate || privateToPublicFlows === "allow" ? "all" : "public"; // Set min_integrity if not already configured const resolvedMinIntegrity = configuredMinIntegrity || defaultMinIntegrity; @@ -79,7 +88,7 @@ async function determineAutomaticLockdown(github, context, core) { core.info("Automatic guard policy determination complete for private/internal repository"); } else { core.info("Automatic guard policy determination complete for public repository"); - core.info("GitHub MCP guard policy automatically applied for public repository. " + "min-integrity='approved' and repos='all' ensure only approved-integrity content is accessible."); + core.info("GitHub MCP guard policy automatically applied for public repository. " + "min-integrity='approved' and repos='public' restrict access to public repositories only."); } // Write resolved guard policy values to the step summary @@ -108,9 +117,9 @@ async function determineAutomaticLockdown(github, context, core) { core.error(`Failed to determine automatic guard policy: ${errorMessage}`); // Default to safe guard policy for public repos on error core.setOutput("min_integrity", "approved"); - core.setOutput("repos", "all"); + core.setOutput("repos", "public"); core.setOutput("visibility", "public"); - core.warning("Failed to determine repository visibility. Defaulting to visibility='public' (conservative), min-integrity='approved', repos='all' for security."); + core.warning("Failed to determine repository visibility. Defaulting to visibility='public' (conservative), min-integrity='approved', repos='public' for security."); } } diff --git a/actions/setup/js/determine_automatic_lockdown.test.cjs b/actions/setup/js/determine_automatic_lockdown.test.cjs index 1e5bbb3402e..c73dac04305 100644 --- a/actions/setup/js/determine_automatic_lockdown.test.cjs +++ b/actions/setup/js/determine_automatic_lockdown.test.cjs @@ -44,12 +44,13 @@ describe("determine_automatic_lockdown", () => { delete process.env.CUSTOM_GITHUB_TOKEN; delete process.env.GH_AW_GITHUB_MIN_INTEGRITY; delete process.env.GH_AW_GITHUB_REPOS; + delete process.env.GH_AW_PRIVATE_TO_PUBLIC_FLOWS; // Import the module determineAutomaticLockdown = (await import("./determine_automatic_lockdown.cjs")).default; }); - it("should set min_integrity=approved and repos=all for public repository (no guard policy configured)", async () => { + it("should set min_integrity=approved and repos=public for public repository (no guard policy configured)", async () => { mockGithub.rest.repos.get.mockResolvedValue({ data: { private: false, @@ -64,7 +65,7 @@ describe("determine_automatic_lockdown", () => { repo: "test-repo", }); expect(mockCore.setOutput).toHaveBeenCalledWith("min_integrity", "approved"); - expect(mockCore.setOutput).toHaveBeenCalledWith("repos", "all"); + expect(mockCore.setOutput).toHaveBeenCalledWith("repos", "public"); expect(mockCore.setOutput).toHaveBeenCalledWith("visibility", "public"); expect(mockCore.setOutput).not.toHaveBeenCalledWith("lockdown", expect.anything()); }); @@ -82,7 +83,7 @@ describe("determine_automatic_lockdown", () => { await determineAutomaticLockdown(mockGithub, mockContext, mockCore); expect(mockCore.setOutput).toHaveBeenCalledWith("min_integrity", "merged"); - expect(mockCore.setOutput).toHaveBeenCalledWith("repos", "all"); + expect(mockCore.setOutput).toHaveBeenCalledWith("repos", "public"); expect(mockCore.setOutput).toHaveBeenCalledWith("visibility", "public"); expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("min-integrity already configured as 'merged'")); }); @@ -148,7 +149,7 @@ describe("determine_automatic_lockdown", () => { expect(mockCore.error).toHaveBeenCalledWith("Failed to determine automatic guard policy: API request failed"); expect(mockCore.setOutput).toHaveBeenCalledWith("min_integrity", "approved"); - expect(mockCore.setOutput).toHaveBeenCalledWith("repos", "all"); + expect(mockCore.setOutput).toHaveBeenCalledWith("repos", "public"); expect(mockCore.setOutput).toHaveBeenCalledWith("visibility", "public"); expect(mockCore.setOutput).not.toHaveBeenCalledWith("lockdown", expect.anything()); expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Failed to determine repository visibility")); @@ -165,7 +166,7 @@ describe("determine_automatic_lockdown", () => { await determineAutomaticLockdown(mockGithub, mockContext, mockCore); expect(mockCore.setOutput).toHaveBeenCalledWith("min_integrity", "approved"); - expect(mockCore.setOutput).toHaveBeenCalledWith("repos", "all"); + expect(mockCore.setOutput).toHaveBeenCalledWith("repos", "public"); expect(mockCore.setOutput).toHaveBeenCalledWith("visibility", "public"); }); @@ -231,7 +232,7 @@ describe("determine_automatic_lockdown", () => { expect(publicSummaryArg).toContain("approved"); expect(publicSummaryArg).toContain("automatic (public repo)"); expect(publicSummaryArg).toContain("repos"); - expect(publicSummaryArg).toContain("all"); + expect(publicSummaryArg).toContain("public"); expect(mockCore.summary.write).toHaveBeenCalled(); }); @@ -287,4 +288,22 @@ describe("determine_automatic_lockdown", () => { expect(privateSummaryArg).toContain("all"); expect(mockCore.summary.write).toHaveBeenCalled(); }); + + it("should set repos=all for public repository when GH_AW_PRIVATE_TO_PUBLIC_FLOWS=allow (private-to-public opt-in)", async () => { + process.env.GH_AW_PRIVATE_TO_PUBLIC_FLOWS = "allow"; + + mockGithub.rest.repos.get.mockResolvedValue({ + data: { + private: false, + visibility: "public", + }, + }); + + await determineAutomaticLockdown(mockGithub, mockContext, mockCore); + + expect(mockCore.setOutput).toHaveBeenCalledWith("repos", "all"); + expect(mockCore.setOutput).toHaveBeenCalledWith("min_integrity", "approved"); + expect(mockCore.setOutput).toHaveBeenCalledWith("visibility", "public"); + expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("repos not configured")); + }); }); diff --git a/pkg/workflow/compiler_difc_proxy.go b/pkg/workflow/compiler_difc_proxy.go index e5dea577e7a..3bd9a5cbe1d 100644 --- a/pkg/workflow/compiler_difc_proxy.go +++ b/pkg/workflow/compiler_difc_proxy.go @@ -498,13 +498,17 @@ func (c *Compiler) generateStartCliProxyStep(yaml *strings.Builder, data *Workfl } } -// defaultCliProxyPolicyJSON is the fallback guard policy for the CLI proxy when no -// guard policy is explicitly configured in the workflow frontmatter. +// defaultCliProxyPolicyJSONTemplate is the fallback guard policy template for the CLI proxy +// when no guard policy is explicitly configured in the workflow frontmatter. // The DIFC proxy requires a --policy flag to forward requests; without it, all API // calls return HTTP 503 with body "proxy enforcement not configured". -// This default allows all repos with no integrity filtering — the most permissive -// policy that still satisfies the proxy's requirement. -const defaultCliProxyPolicyJSON = `{"allow-only":{"repos":"all","min-integrity":"none"}}` +// This template references the determine-automatic-lockdown step outputs so that the +// repos restriction matches the repository visibility at runtime: +// - Public repos → repos="public" (blocks access to private repos) +// - Private repos → repos="all" (no cross-visibility restriction) +// +// This mirrors how the MCP Gateway references $GITHUB_MCP_GUARD_REPOS. +const defaultCliProxyPolicyJSONTemplate = `{"allow-only":{"repos":"${{ steps.determine-automatic-lockdown.outputs.repos }}","min-integrity":"${{ steps.determine-automatic-lockdown.outputs.min_integrity }}"}}` // buildStartCliProxyStepYAML returns the YAML for the "Start CLI proxy" step, // or an empty string if the proxy cannot be configured. @@ -519,13 +523,15 @@ func (c *Compiler) buildStartCliProxyStepYAML(data *WorkflowData) string { // Build the guard policy JSON (static fields only, plus reaction fields when enabled). // The CLI proxy requires a policy to forward requests — without one, all API - // calls return HTTP 503 ("proxy enforcement not configured"). Use the default - // permissive policy when no guard policy is configured in the frontmatter. + // calls return HTTP 503 ("proxy enforcement not configured"). When no guard policy + // is explicitly configured, reference the determine-automatic-lockdown step outputs + // so the repos restriction reflects repo visibility at runtime, matching the MCP + // Gateway's behavior. ensureDefaultMCPGatewayConfig(data) policyJSON := getDIFCProxyPolicyJSON(githubToolConfig, data, data.SandboxConfig.MCP) if policyJSON == "" { - policyJSON = defaultCliProxyPolicyJSON - difcProxyLog.Print("No guard policy configured, using default CLI proxy policy") + policyJSON = defaultCliProxyPolicyJSONTemplate + difcProxyLog.Print("No guard policy configured, using visibility-aware default CLI proxy policy") } // Resolve the container image from the MCP gateway configuration @@ -539,7 +545,7 @@ func (c *Compiler) buildStartCliProxyStepYAML(data *WorkflowData) string { if isAWFNetworkIsolationEnabled(data) { sb.WriteString(" GH_AW_NETWORK_ISOLATION: 'true'\n") } - fmt.Fprintf(&sb, " CLI_PROXY_POLICY: '%s'\n", policyJSON) + fmt.Fprintf(&sb, " CLI_PROXY_POLICY: %s\n", quoteYAMLEnvValue(policyJSON)) fmt.Fprintf(&sb, " CLI_PROXY_IMAGE: '%s'\n", containerImage) sb.WriteString(" run: |\n") sb.WriteString(" bash \"${RUNNER_TEMP}/gh-aw/actions/start_cli_proxy.sh\"\n") diff --git a/pkg/workflow/compiler_difc_proxy_test.go b/pkg/workflow/compiler_difc_proxy_test.go index aa3f6510c5a..48515151e5a 100644 --- a/pkg/workflow/compiler_difc_proxy_test.go +++ b/pkg/workflow/compiler_difc_proxy_test.go @@ -870,12 +870,12 @@ func TestInjectProxyEnvIntoCustomSteps(t *testing.T) { } // TestBuildStartCliProxyStepYAML verifies that the CLI proxy step always emits -// CLI_PROXY_POLICY, using the default permissive policy when no guard policy is +// CLI_PROXY_POLICY, using a visibility-aware default when no guard policy is // configured in the frontmatter. func TestBuildStartCliProxyStepYAML(t *testing.T) { c := &Compiler{} - t.Run("emits default policy when no guard policy is configured", func(t *testing.T) { + t.Run("emits visibility-aware default policy when no guard policy is configured", func(t *testing.T) { data := &WorkflowData{ Tools: map[string]any{ "github": map[string]any{"toolsets": []string{"default"}}, @@ -886,11 +886,14 @@ func TestBuildStartCliProxyStepYAML(t *testing.T) { require.NotEmpty(t, result, "should emit CLI proxy step even without guard policy") assert.Contains(t, result, "CLI_PROXY_POLICY", "should always emit CLI_PROXY_POLICY") assert.Contains(t, result, `"allow-only"`, "default policy should contain allow-only") - assert.Contains(t, result, `"repos":"all"`, "default policy should allow all repos") - assert.Contains(t, result, `"min-integrity":"none"`, "default policy should have min-integrity none") + // Default policy references step output for repos (visibility-aware at runtime) + assert.Contains(t, result, `steps.determine-automatic-lockdown.outputs.repos`, "default policy should reference step output for repos") + assert.Contains(t, result, `steps.determine-automatic-lockdown.outputs.min_integrity`, "default policy should reference step output for min-integrity") + // Should NOT contain hardcoded static repos value + assert.NotContains(t, result, `"repos":"all"`, "default policy should not have hardcoded repos") }) - t.Run("emits default policy when github tool is nil", func(t *testing.T) { + t.Run("emits visibility-aware default policy when github tool is nil", func(t *testing.T) { data := &WorkflowData{ Tools: map[string]any{}, } @@ -898,7 +901,8 @@ func TestBuildStartCliProxyStepYAML(t *testing.T) { result := c.buildStartCliProxyStepYAML(data) require.NotEmpty(t, result, "should emit CLI proxy step even without github tool") assert.Contains(t, result, "CLI_PROXY_POLICY", "should always emit CLI_PROXY_POLICY") - assert.Contains(t, result, `"min-integrity":"none"`, "should use default min-integrity") + assert.Contains(t, result, `steps.determine-automatic-lockdown.outputs.repos`, "default policy should reference step output for repos") + assert.Contains(t, result, `steps.determine-automatic-lockdown.outputs.min_integrity`, "default policy should reference step output for min-integrity") }) t.Run("uses configured guard policy when present", func(t *testing.T) { diff --git a/pkg/workflow/compiler_github_mcp_steps.go b/pkg/workflow/compiler_github_mcp_steps.go index d9e0915dc66..9a7ea43dfd7 100644 --- a/pkg/workflow/compiler_github_mcp_steps.go +++ b/pkg/workflow/compiler_github_mcp_steps.go @@ -51,6 +51,7 @@ func (c *Compiler) generateGitHubMCPLockdownDetectionStep(yaml *strings.Builder, // detect whether each field is already configured and avoid overriding it. configuredMinIntegrity := "" configuredRepos := "" + privateToPublicFlowsAllow := false if toolConfig, ok := githubTool.(map[string]any); ok { if v, exists := toolConfig["min-integrity"]; exists { configuredMinIntegrity = serializeEnvStringValue(v) @@ -61,6 +62,13 @@ func (c *Compiler) generateGitHubMCPLockdownDetectionStep(yaml *strings.Builder, } else if v, exists := toolConfig["repos"]; exists { configuredRepos = serializeEnvStringValue(v) } + // Detect private-to-public-flows: allow to inform the default repos value. + // When set to "allow", the user has explicitly opted in to cross-visibility data + // flows, so the repos default should be "all" rather than "public" even for + // public repositories. + if ptpFlows, _ := toolConfig["private-to-public-flows"].(string); ptpFlows == "allow" { + privateToPublicFlowsAllow = true + } } // Generate the step using the determine_automatic_lockdown.cjs action @@ -76,6 +84,9 @@ func (c *Compiler) generateGitHubMCPLockdownDetectionStep(yaml *strings.Builder, if configuredRepos != "" { fmt.Fprintf(yaml, " GH_AW_GITHUB_REPOS: %s\n", quoteYAMLEnvValue(configuredRepos)) } + if privateToPublicFlowsAllow { + yaml.WriteString(" GH_AW_PRIVATE_TO_PUBLIC_FLOWS: allow\n") + } yaml.WriteString(" with:\n") yaml.WriteString(" script: |\n") yaml.WriteString(" const determineAutomaticLockdown = require('${{ runner.temp }}/gh-aw/actions/determine_automatic_lockdown.cjs');\n") diff --git a/pkg/workflow/compiler_github_mcp_steps_test.go b/pkg/workflow/compiler_github_mcp_steps_test.go index 687bdf4d6fc..fab326943f5 100644 --- a/pkg/workflow/compiler_github_mcp_steps_test.go +++ b/pkg/workflow/compiler_github_mcp_steps_test.go @@ -83,4 +83,26 @@ func TestGenerateGitHubMCPLockdownDetectionStepGeneratedWhenNoGuardPolicy(t *tes assert.Contains(t, output, "determine-automatic-lockdown", "detection step should be generated when no explicit guard policy") assert.NotContains(t, output, "GH_AW_GITHUB_MIN_INTEGRITY", "env var should not be present when min-integrity is not set") assert.NotContains(t, output, "GH_AW_GITHUB_REPOS", "env var should not be present when repos is not set") + assert.NotContains(t, output, "GH_AW_PRIVATE_TO_PUBLIC_FLOWS", "env var should not be present when private-to-public-flows is not set") +} + +func TestGenerateGitHubMCPLockdownDetectionStepEmitsPrivateToPublicFlowsEnv(t *testing.T) { + t.Parallel() + + var yaml strings.Builder + data := &WorkflowData{ + Tools: map[string]any{ + "github": map[string]any{ + "private-to-public-flows": "allow", + }, + }, + } + + NewCompiler().generateGitHubMCPLockdownDetectionStep(&yaml, data) + output := yaml.String() + + assert.Contains(t, output, "determine-automatic-lockdown", "detection step should be generated") + // When private-to-public-flows: allow is set, the step must receive the env var so the + // determine_automatic_lockdown.cjs script can output repos=all instead of repos=public. + assert.Contains(t, output, "GH_AW_PRIVATE_TO_PUBLIC_FLOWS: allow", "env var should be emitted when private-to-public-flows is allow") } diff --git a/pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden b/pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden index 6aba3150c70..8a4b05e943d 100644 --- a/pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden +++ b/pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden @@ -451,7 +451,7 @@ jobs: GITHUB_GRAPHQL_URL: ${{ env.GITHUB_GRAPHQL_URL }} GITHUB_COPILOT_BASE_URL: ${{ env.GITHUB_COPILOT_BASE_URL }} GH_AW_NETWORK_ISOLATION: 'true' - CLI_PROXY_POLICY: '{"allow-only":{"repos":"all","min-integrity":"none"}}' + CLI_PROXY_POLICY: '{"allow-only":{"repos":"${{ steps.determine-automatic-lockdown.outputs.repos }}","min-integrity":"${{ steps.determine-automatic-lockdown.outputs.min_integrity }}"}}' CLI_PROXY_IMAGE: 'ghcr.io/github/gh-aw-mcpg:v0.4.1' run: | bash "${RUNNER_TEMP}/gh-aw/actions/start_cli_proxy.sh"