Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions .github/workflows/smoke-gemini.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

59 changes: 59 additions & 0 deletions actions/setup/js/push_to_pull_request_branch.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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)}`);
}

Comment on lines +326 to +328
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default-branch protection check is currently fail-open: if repos.get() fails, the code only warns and continues, which can still allow pushes to the repo's default branch when it is not protected (getBranchProtection would return 404). Given the stated security requirement (agents must not push to default branches), this should fail closed or use a non-network fallback source for default_branch (e.g., default_branch from the event payload / PR base repo data) before proceeding.

Suggested change
core.warning(`Could not check repository default branch: ${getErrorMessage(repoError)}`);
}
core.warning(`Could not check repository default branch via API: ${getErrorMessage(repoError)}`);
// Fallback: try to derive the default branch from the GitHub event payload
try {
const eventPath = process.env.GITHUB_EVENT_PATH;
if (eventPath && fs.existsSync(eventPath)) {
const eventRaw = fs.readFileSync(eventPath, "utf8");
const eventPayload = JSON.parse(eventRaw);
if (
eventPayload &&
eventPayload.pull_request &&
eventPayload.pull_request.base &&
eventPayload.pull_request.base.repo &&
eventPayload.pull_request.base.repo.default_branch
) {
defaultBranch = eventPayload.pull_request.base.repo.default_branch;
core.info(`Resolved default branch from pull_request.base.repo.default_branch: "${defaultBranch}"`);
} else if (
eventPayload &&
eventPayload.repository &&
eventPayload.repository.default_branch
) {
defaultBranch = eventPayload.repository.default_branch;
core.info(`Resolved default branch from repository.default_branch: "${defaultBranch}"`);
} else {
core.warning("Default branch not present in event payload (pull_request.base.repo.default_branch or repository.default_branch).");
}
} else {
core.warning("GITHUB_EVENT_PATH is not set or does not point to an existing file; cannot derive default branch from event payload.");
}
} catch (eventError) {
core.warning(`Failed to read event payload for default-branch fallback: ${getErrorMessage(eventError)}`);
}
}
// Fail closed if we still cannot determine the repository's default branch
if (!defaultBranch) {
const msg = `Cannot determine repository default branch for "${repoParts.owner}/${repoParts.repo}". Push blocked to prevent accidental writes to the default branch.`;
core.error(msg);
return { success: false, error: msg };
}

Copilot uses AI. Check for mistakes.
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}"` };
Expand Down
117 changes: 117 additions & 0 deletions actions/setup/js/push_to_pull_request_branch.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
};
Expand Down Expand Up @@ -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
// ──────────────────────────────────────────────────────
Expand Down
20 changes: 19 additions & 1 deletion pkg/workflow/engine_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 ”).
Comment on lines +221 to +223
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc comment for yamlStringValue is inaccurate and contains a broken quote escape example: it says values starting with "{" or "[" or other characters are quoted, but the implementation only checks '{' and '['. It also says ("' becomes ”"), but YAML single-quote escaping is done by doubling the quote ("'" -> "''"). Please update the comment to match the actual behavior and fix the escape example character.

Suggested change
// 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 ).
// If the value starts with a YAML flow indicator ('{' or '['), it wraps the value
// in single quotes so it is treated as a plain string rather than a flow collection.
// Any embedded single quotes are escaped by doubling them (' becomes '').

Copilot uses AI. Check for mistakes.
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.
//
Expand Down
78 changes: 78 additions & 0 deletions pkg/workflow/engine_helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})
}
12 changes: 12 additions & 0 deletions pkg/workflow/gemini_engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{},
}
Comment on lines +517 to +521
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR introduces YAML-quoting behavior changes for Gemini settings/env rendering (and updates the smoke-gemini workflow accordingly), but the PR title/description only discuss blocking pushes to default/protected branches. Please update the PR description to cover the Gemini/YAML changes (or split them into a separate PR) so reviewers can validate scope and rationale appropriately.

Copilot uses AI. Check for mistakes.
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")
})
}
Loading