diff --git a/.github/workflows/smoke-gemini.lock.yml b/.github/workflows/smoke-gemini.lock.yml index e9441659728..7db68a38c97 100644 --- a/.github/workflows/smoke-gemini.lock.yml +++ b/.github/workflows/smoke-gemini.lock.yml @@ -827,13 +827,15 @@ jobs: run: | mkdir -p "$GITHUB_WORKSPACE/.gemini" SETTINGS="$GITHUB_WORKSPACE/.gemini/settings.json" - BASE_CONFIG='{"context":{"includeDirectories":["/tmp/"]},"tools":{"core":["glob","grep_search","list_directory","read_file","read_many_files","replace","run_shell_command","write_file"]}}' + BASE_CONFIG="$GH_AW_GEMINI_BASE_CONFIG" if [ -f "$SETTINGS" ]; then MERGED=$(jq -n --argjson base "$BASE_CONFIG" --argjson existing "$(cat "$SETTINGS")" '$existing * $base') echo "$MERGED" > "$SETTINGS" else echo "$BASE_CONFIG" > "$SETTINGS" fi + env: + GH_AW_GEMINI_BASE_CONFIG: '{"context":{"includeDirectories":["/tmp/"]},"tools":{"core":["glob","grep_search","list_directory","read_file","read_many_files","replace","run_shell_command","write_file"]}}' - name: Execute Gemini CLI id: agentic_execution run: | @@ -1060,13 +1062,15 @@ jobs: run: | mkdir -p "$GITHUB_WORKSPACE/.gemini" SETTINGS="$GITHUB_WORKSPACE/.gemini/settings.json" - BASE_CONFIG='{"context":{"includeDirectories":["/tmp/"]},"tools":{"core":["glob","grep_search","list_directory","read_file","read_many_files","run_shell_command(cat)","run_shell_command(grep)","run_shell_command(head)","run_shell_command(jq)","run_shell_command(ls)","run_shell_command(tail)","run_shell_command(wc)"]}}' + BASE_CONFIG="$GH_AW_GEMINI_BASE_CONFIG" if [ -f "$SETTINGS" ]; then MERGED=$(jq -n --argjson base "$BASE_CONFIG" --argjson existing "$(cat "$SETTINGS")" '$existing * $base') echo "$MERGED" > "$SETTINGS" else echo "$BASE_CONFIG" > "$SETTINGS" fi + env: + GH_AW_GEMINI_BASE_CONFIG: '{"context":{"includeDirectories":["/tmp/"]},"tools":{"core":["glob","grep_search","list_directory","read_file","read_many_files","run_shell_command(cat)","run_shell_command(grep)","run_shell_command(head)","run_shell_command(jq)","run_shell_command(ls)","run_shell_command(tail)","run_shell_command(wc)"]}}' - name: Execute Gemini CLI if: always() && steps.detection_guard.outputs.run_detection == 'true' id: detection_agentic_execution diff --git a/actions/setup/js/push_to_pull_request_branch.cjs b/actions/setup/js/push_to_pull_request_branch.cjs index 3d0a4db8932..c8351960008 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -309,6 +309,65 @@ async function main(config = {}) { core.info(`PR title: ${prTitle}`); core.info(`PR labels: ${prLabels.join(", ")}`); + // SECURITY: Block pushing to the repository's default branch or any branch with + // protection rules. PR head branches must never be default or protected branches. + // This prevents agents from pushing directly to branches that should only receive + // changes through reviewed pull requests. + { + // Check whether the branch is the repository default branch + let defaultBranch = null; + try { + const { data: repoData } = await githubClient.rest.repos.get({ + owner: repoParts.owner, + repo: repoParts.repo, + }); + defaultBranch = repoData.default_branch; + } catch (repoError) { + core.warning(`Could not check repository default branch: ${getErrorMessage(repoError)}`); + } + + if (defaultBranch && branchName === defaultBranch) { + const msg = `Cannot push to branch "${branchName}": this is the repository's default branch. Agents must not push directly to the default branch.`; + core.error(msg); + return { success: false, error: msg }; + } + + // Check whether the branch has protection rules + let isBranchProtected = false; + try { + await githubClient.rest.repos.getBranchProtection({ + owner: repoParts.owner, + repo: repoParts.repo, + branch: branchName, + }); + // Successful response means branch protection rules exist + isBranchProtected = true; + } catch (protectionError) { + const protectionStatus = protectionError && typeof protectionError === "object" && "status" in protectionError ? protectionError.status : undefined; + if (protectionStatus === 404) { + // 404 means no protection rules – safe to proceed + core.info(`Branch "${branchName}" has no protection rules`); + } else if (protectionStatus === 403) { + // 403 means the token lacks permission to read branch protection rules. + // The GitHub platform will still enforce branch protection at push time, + // so warn and allow the push to proceed. + core.warning(`Could not check branch protection rules for "${branchName}" (insufficient permissions): ${getErrorMessage(protectionError)}`); + } else { + // Unexpected errors (5xx, network failures, etc.) – fail closed to + // avoid bypassing branch protection due to transient API issues. + const msg = `Cannot verify branch protection rules for "${branchName}": ${getErrorMessage(protectionError)}. Push blocked to prevent accidental writes to protected branches.`; + core.error(msg); + return { success: false, error: msg }; + } + } + + if (isBranchProtected) { + const msg = `Cannot push to branch "${branchName}": this branch has protection rules. Agents must not push directly to protected branches.`; + core.error(msg); + return { success: false, error: msg }; + } + } + // Validate title prefix if specified if (titlePrefix && !prTitle.startsWith(titlePrefix)) { return { success: false, error: `Pull request title "${prTitle}" does not start with required prefix "${titlePrefix}"` }; diff --git a/actions/setup/js/push_to_pull_request_branch.test.cjs b/actions/setup/js/push_to_pull_request_branch.test.cjs index 6c436211714..de63c54b8a8 100644 --- a/actions/setup/js/push_to_pull_request_branch.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.test.cjs @@ -145,6 +145,12 @@ describe("push_to_pull_request_branch.cjs", () => { }, }), }, + repos: { + get: vi.fn().mockResolvedValue({ + data: { default_branch: "main" }, + }), + getBranchProtection: vi.fn().mockRejectedValue(Object.assign(new Error("Branch not protected"), { status: 404 })), + }, }, graphql: vi.fn(), }; @@ -496,6 +502,117 @@ index 0000000..abc1234 }); }); + // ────────────────────────────────────────────────────── + // Protected Branch Detection Tests + // ────────────────────────────────────────────────────── + + describe("protected branch detection", () => { + it("should block push to the repository default branch", async () => { + // PR head branch is the repo default branch + mockGithub.rest.pulls.get.mockResolvedValue({ + data: { + head: { + ref: "main", + repo: { full_name: "test-owner/test-repo", fork: false }, + }, + base: { repo: { full_name: "test-owner/test-repo" } }, + title: "Test PR", + labels: [], + }, + }); + mockGithub.rest.repos.get.mockResolvedValue({ data: { default_branch: "main" } }); + + const patchPath = createPatchFile(); + const module = await loadModule(); + const handler = await module.main({ target: "triggering" }); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("default branch"); + expect(mockCore.error).toHaveBeenCalledWith(expect.stringContaining("default branch")); + // Should not attempt any git operations + expect(mockExec.exec).not.toHaveBeenCalled(); + }); + + it("should block push to a branch with protection rules", async () => { + // Branch has protection rules - getBranchProtection resolves successfully + mockGithub.rest.repos.getBranchProtection.mockResolvedValue({ data: {} }); + + const patchPath = createPatchFile(); + const module = await loadModule(); + const handler = await module.main({ target: "triggering" }); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("protection rules"); + expect(mockCore.error).toHaveBeenCalledWith(expect.stringContaining("protection rules")); + // Should not attempt any git operations + expect(mockExec.exec).not.toHaveBeenCalled(); + }); + + it("should allow push when branch is not default and has no protection rules", async () => { + // Default mock: repos.get returns "main", getBranchProtection returns 404 + // PR head is "feature-branch" (not the default) + const patchPath = createPatchFile(); + mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); + + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(true); + expect(result.branch_name).toBe("feature-branch"); + }); + + it("should warn and continue when repos.get fails", async () => { + // repos.get fails - cannot determine default branch, warn and continue + mockGithub.rest.repos.get.mockRejectedValue(new Error("API unavailable")); + const patchPath = createPatchFile(); + mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); + + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + // Should warn but still allow the push since we can't confirm it's the default branch + expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Could not check repository default branch")); + expect(result.success).toBe(true); + }); + + it("should warn and continue when getBranchProtection returns 403 (permission denied)", async () => { + // getBranchProtection returns 403 (no permission to read protection) + // Platform will still enforce protection at push time, so we warn and allow + mockGithub.rest.repos.getBranchProtection.mockRejectedValue(Object.assign(new Error("Forbidden"), { status: 403 })); + const patchPath = createPatchFile(); + mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); + + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + // Should warn but allow push since the platform still enforces protection at push time + expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Could not check branch protection rules")); + expect(result.success).toBe(true); + }); + + it("should block push when getBranchProtection returns an unexpected error (fail closed)", async () => { + // getBranchProtection returns 500 - unexpected error, fail closed for security + mockGithub.rest.repos.getBranchProtection.mockRejectedValue(Object.assign(new Error("Internal Server Error"), { status: 500 })); + const patchPath = createPatchFile(); + + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + // Should block the push when branch protection status cannot be verified + expect(result.success).toBe(false); + expect(result.error).toContain("Cannot verify branch protection rules"); + expect(mockCore.error).toHaveBeenCalledWith(expect.stringContaining("Cannot verify branch protection rules")); + // Should not attempt any git operations + expect(mockExec.exec).not.toHaveBeenCalled(); + }); + }); + // ────────────────────────────────────────────────────── // Repository State Scenarios // ────────────────────────────────────────────────────── diff --git a/pkg/workflow/engine_helpers.go b/pkg/workflow/engine_helpers.go index 33e3adb4d4d..5561b85a715 100644 --- a/pkg/workflow/engine_helpers.go +++ b/pkg/workflow/engine_helpers.go @@ -210,13 +210,31 @@ func FormatStepWithCommandAndEnv(stepLines []string, command string, env map[str for _, key := range envKeys { value := env[key] - stepLines = append(stepLines, fmt.Sprintf(" %s: %s", key, value)) + stepLines = append(stepLines, fmt.Sprintf(" %s: %s", key, yamlStringValue(value))) } } return stepLines } +// yamlStringValue returns a YAML-safe representation of a string value. +// If the value starts with a YAML flow indicator ('{' or '[') or other characters +// that would cause it to be misinterpreted by YAML parsers, it wraps the value +// in single quotes. Any embedded single quotes are escaped by doubling them (' becomes ”). +func yamlStringValue(value string) string { + if len(value) == 0 { + return value + } + // Values starting with YAML flow indicators need quoting to be treated as strings. + // '{' would be parsed as a YAML flow mapping, '[' as a YAML flow sequence. + first := value[0] + if first != '{' && first != '[' { + return value + } + // Single-quote the value, escaping any embedded single quotes by doubling them. + return "'" + strings.ReplaceAll(value, "'", "''") + "'" +} + // FilterEnvForSecrets filters environment variables to only include allowed secrets. // This is a security measure to ensure that only necessary secrets are passed to the execution step. // diff --git a/pkg/workflow/engine_helpers_test.go b/pkg/workflow/engine_helpers_test.go index 0f9a4c0a728..b33475755fb 100644 --- a/pkg/workflow/engine_helpers_test.go +++ b/pkg/workflow/engine_helpers_test.go @@ -363,3 +363,81 @@ func TestGetNpmBinPathSetup_NoGorootDoesNotBreakChain(t *testing.T) { t.Errorf("Expected command chain to continue when GOROOT is empty, got: %q", result) } } + +func TestYamlStringValue(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "plain string unchanged", + input: "hello world", + expected: "hello world", + }, + { + name: "empty string unchanged", + input: "", + expected: "", + }, + { + name: "github actions expression unchanged", + input: "${{ secrets.TOKEN }}", + expected: "${{ secrets.TOKEN }}", + }, + { + name: "json object gets single-quoted", + input: `{"key":"value"}`, + expected: `'{"key":"value"}'`, + }, + { + name: "json array gets single-quoted", + input: `["a","b"]`, + expected: `'["a","b"]'`, + }, + { + name: "json object with embedded single quote gets escaped", + input: `{"key":"it's"}`, + expected: `'{"key":"it''s"}'`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := yamlStringValue(tt.input) + if result != tt.expected { + t.Errorf("yamlStringValue(%q) = %q, want %q", tt.input, result, tt.expected) + } + }) + } +} + +func TestFormatStepWithCommandAndEnvYAMLSafe(t *testing.T) { + t.Run("json env var is single-quoted for valid YAML", func(t *testing.T) { + stepLines := []string{" - name: Test step"} + env := map[string]string{ + "MY_JSON": `{"key":"value","nested":{"a":1}}`, + } + result := FormatStepWithCommandAndEnv(stepLines, "echo test", env) + output := strings.Join(result, "\n") + + // The JSON value must be single-quoted so YAML treats it as a string + if !strings.Contains(output, `MY_JSON: '{"key":"value","nested":{"a":1}}'`) { + t.Errorf("Expected single-quoted JSON in env, got:\n%s", output) + } + }) + + t.Run("github expression env var is not quoted", func(t *testing.T) { + stepLines := []string{" - name: Test step"} + env := map[string]string{ + "MY_TOKEN": "${{ secrets.TOKEN }}", + } + result := FormatStepWithCommandAndEnv(stepLines, "echo test", env) + output := strings.Join(result, "\n") + + // GitHub Actions expressions should not be wrapped in extra quotes + if !strings.Contains(output, "MY_TOKEN: ${{ secrets.TOKEN }}") { + t.Errorf("Expected unquoted github expression in env, got:\n%s", output) + } + }) +} diff --git a/pkg/workflow/gemini_engine_test.go b/pkg/workflow/gemini_engine_test.go index b6ce867c88b..16c64ce3a99 100644 --- a/pkg/workflow/gemini_engine_test.go +++ b/pkg/workflow/gemini_engine_test.go @@ -513,4 +513,16 @@ func TestGenerateGeminiSettingsStep(t *testing.T) { assert.Contains(t, content, "write_file", "Should include write_file for edit tool") assert.Contains(t, content, "replace", "Should include replace for edit tool") }) + + t.Run("GH_AW_GEMINI_BASE_CONFIG env var is single-quoted for valid YAML", func(t *testing.T) { + workflowData := &WorkflowData{ + Name: "test-workflow", + Tools: map[string]any{}, + } + step := engine.generateGeminiSettingsStep(workflowData) + content := strings.Join(step, "\n") + + // The JSON value must be single-quoted so YAML doesn't treat it as an object + assert.Contains(t, content, "GH_AW_GEMINI_BASE_CONFIG: '", "JSON env var value must be single-quoted for valid YAML") + }) }