diff --git a/src/github/validation/actor.ts b/src/github/validation/actor.ts index 36be2ff6..92f5ede6 100644 --- a/src/github/validation/actor.ts +++ b/src/github/validation/actor.ts @@ -8,53 +8,75 @@ import type { Octokit } from "@octokit/rest"; import type { GitHubContext } from "../context"; +function isAllowedBot(actor: string, allowedBots: string): boolean { + const trimmed = allowedBots.trim(); + if (trimmed === "*") return true; + if (!trimmed) return false; + + const allowedList = trimmed + .split(",") + .map((bot) => + bot + .trim() + .toLowerCase() + .replace(/\[bot\]$/, ""), + ) + .filter((bot) => bot.length > 0); + + const normalizedActor = actor.toLowerCase().replace(/\[bot\]$/, ""); + return allowedList.includes(normalizedActor); +} + export async function checkHumanActor( octokit: Octokit, githubContext: GitHubContext, ) { - // Fetch user information from GitHub API - const { data: userData } = await octokit.users.getByUsername({ - username: githubContext.actor, - }); + const allowedBots = githubContext.inputs.allowedBots; - const actorType = userData.type; + // Check allowed_bots BEFORE calling the GitHub Users API. + // Some bot actors (e.g. GitHub Copilot with GITHUB_ACTOR="Copilot") are + // not resolvable via the Users API and would cause a 404 if we called it + // first. By checking the allow-list early we avoid the unnecessary API + // call and the resulting crash. + if (isAllowedBot(githubContext.actor, allowedBots)) { + console.log( + `Actor ${githubContext.actor} is in allowed_bots list, skipping human actor check`, + ); + return; + } + + // Fetch user information from GitHub API + let actorType: string; + try { + const { data: userData } = await octokit.users.getByUsername({ + username: githubContext.actor, + }); + actorType = userData.type; + } catch (error) { + // Handle 404 for non-user actors (GitHub Apps whose GITHUB_ACTOR + // doesn't match any user account, e.g. "Copilot"). + if ( + error instanceof Error && + (error.message.includes("Not Found") || + error.message.includes("is not a user")) + ) { + const botName = githubContext.actor + .toLowerCase() + .replace(/\[bot\]$/, ""); + throw new Error( + `Workflow initiated by non-human actor: ${botName} (actor not found on GitHub). Add bot to allowed_bots list or use '*' to allow all bots.`, + ); + } + throw error; + } console.log(`Actor type: ${actorType}`); // Check bot permissions if actor is not a User if (actorType !== "User") { - const allowedBots = githubContext.inputs.allowedBots; - - // Check if all bots are allowed - if (allowedBots.trim() === "*") { - console.log( - `All bots are allowed, skipping human actor check for: ${githubContext.actor}`, - ); - return; - } - - // Parse allowed bots list - const allowedBotsList = allowedBots - .split(",") - .map((bot) => - bot - .trim() - .toLowerCase() - .replace(/\[bot\]$/, ""), - ) - .filter((bot) => bot.length > 0); - const botName = githubContext.actor.toLowerCase().replace(/\[bot\]$/, ""); - // Check if specific bot is allowed - if (allowedBotsList.includes(botName)) { - console.log( - `Bot ${botName} is in allowed list, skipping human actor check`, - ); - return; - } - - // Bot not allowed + // Bot not allowed (we already checked allowed_bots above) throw new Error( `Workflow initiated by non-human actor: ${botName} (type: ${actorType}). Add bot to allowed_bots list or use '*' to allow all bots.`, ); diff --git a/src/github/validation/permissions.ts b/src/github/validation/permissions.ts index 731fcd41..9ade4bd5 100644 --- a/src/github/validation/permissions.ts +++ b/src/github/validation/permissions.ts @@ -2,6 +2,28 @@ import * as core from "@actions/core"; import type { ParsedGitHubContext } from "../context"; import type { Octokit } from "@octokit/rest"; +/** + * Check if a bot actor is in the allowed bots list. + */ +function isAllowedBot(actor: string, allowedBots: string): boolean { + const trimmed = allowedBots.trim(); + if (trimmed === "*") return true; + if (!trimmed) return false; + + const allowedList = trimmed + .split(",") + .map((bot) => + bot + .trim() + .toLowerCase() + .replace(/\[bot\]$/, ""), + ) + .filter((bot) => bot.length > 0); + + const normalizedActor = actor.toLowerCase().replace(/\[bot\]$/, ""); + return allowedList.includes(normalizedActor); +} + /** * Check if the actor has write permissions to the repository * @param octokit - The Octokit REST client @@ -17,6 +39,7 @@ export async function checkWritePermissions( githubTokenProvided?: boolean, ): Promise { const { repository, actor } = context; + const allowedBots = context.inputs.allowedBots ?? ""; try { core.info(`Checking permissions for actor: ${actor}`); @@ -43,12 +66,21 @@ export async function checkWritePermissions( } } - // Check if the actor is a GitHub App (bot user) + // Check if the actor is a GitHub App (bot user with [bot] suffix) if (actor.endsWith("[bot]")) { core.info(`Actor is a GitHub App: ${actor}`); return true; } + // Check if the actor is in the allowed bots list (handles non-[bot] actors + // like GitHub Copilot whose GITHUB_ACTOR is "Copilot", not "Copilot[bot]") + if (isAllowedBot(actor, allowedBots)) { + core.info( + `Actor ${actor} is in allowed_bots list, skipping permission check`, + ); + return true; + } + // Check permissions directly using the permission endpoint const response = await octokit.repos.getCollaboratorPermissionLevel({ owner: repository.owner, @@ -67,6 +99,28 @@ export async function checkWritePermissions( return false; } } catch (error) { + // Handle 404 errors for non-user actors (e.g. GitHub Apps like Copilot + // whose GITHUB_ACTOR doesn't end with [bot]). + // The collaborator permission API only works for user accounts. + if ( + error instanceof Error && + error.message.includes("is not a user") + ) { + core.info( + `Actor ${actor} is not a GitHub user (likely a GitHub App). Checking allowed_bots...`, + ); + if (isAllowedBot(actor, allowedBots)) { + core.info( + `Non-user actor ${actor} is in allowed_bots list, granting access`, + ); + return true; + } + core.warning( + `Non-user actor ${actor} is not in allowed_bots list. Add it to allowed_bots or use '*' to allow all bots.`, + ); + return false; + } + core.error(`Failed to check permissions: ${error}`); throw new Error(`Failed to check permissions for ${actor}: ${error}`); } diff --git a/test/actor.test.ts b/test/actor.test.ts index 4c9d206d..22df940b 100644 --- a/test/actor.test.ts +++ b/test/actor.test.ts @@ -93,4 +93,78 @@ describe("checkHumanActor", () => { "Workflow initiated by non-human actor: other-bot (type: Bot). Add bot to allowed_bots list or use '*' to allow all bots.", ); }); + + describe("non-[bot] actors (e.g. GitHub Copilot)", () => { + // GitHub Copilot SWE Agent sets GITHUB_ACTOR="Copilot" which is not a + // valid GitHub user and doesn't end with [bot], causing 404 on the + // Users API. These tests verify the fix handles this gracefully. + + function createMockOctokitThat404s(): Octokit { + return { + users: { + getByUsername: async () => { + const err = new Error("Not Found"); + (err as any).status = 404; + throw err; + }, + }, + } as unknown as Octokit; + } + + test("should pass for non-[bot] actor when in allowed_bots list", async () => { + const mockOctokit = createMockOctokitThat404s(); + const context = createMockContext(); + context.actor = "Copilot"; + context.inputs.allowedBots = "copilot,cursor"; + + // Should not even call the API — allowed_bots check happens first + await expect( + checkHumanActor(mockOctokit, context), + ).resolves.toBeUndefined(); + }); + + test("should pass for non-[bot] actor when all bots are allowed", async () => { + const mockOctokit = createMockOctokitThat404s(); + const context = createMockContext(); + context.actor = "Copilot"; + context.inputs.allowedBots = "*"; + + await expect( + checkHumanActor(mockOctokit, context), + ).resolves.toBeUndefined(); + }); + + test("should throw with clear message for non-[bot] actor that 404s and is not in allowed list", async () => { + const mockOctokit = createMockOctokitThat404s(); + const context = createMockContext(); + context.actor = "Copilot"; + context.inputs.allowedBots = "cursor"; + + await expect(checkHumanActor(mockOctokit, context)).rejects.toThrow( + "Workflow initiated by non-human actor: copilot (actor not found on GitHub). Add bot to allowed_bots list or use '*' to allow all bots.", + ); + }); + + test("should throw with clear message for non-[bot] actor that 404s and allowed_bots is empty", async () => { + const mockOctokit = createMockOctokitThat404s(); + const context = createMockContext(); + context.actor = "Copilot"; + context.inputs.allowedBots = ""; + + await expect(checkHumanActor(mockOctokit, context)).rejects.toThrow( + "Workflow initiated by non-human actor: copilot (actor not found on GitHub). Add bot to allowed_bots list or use '*' to allow all bots.", + ); + }); + + test("should match allowed_bots case-insensitively for non-[bot] actors", async () => { + const mockOctokit = createMockOctokitThat404s(); + const context = createMockContext(); + context.actor = "Copilot"; + context.inputs.allowedBots = "COPILOT"; + + await expect( + checkHumanActor(mockOctokit, context), + ).resolves.toBeUndefined(); + }); + }); }); diff --git a/test/permissions.test.ts b/test/permissions.test.ts index 1d654575..8b8bcc97 100644 --- a/test/permissions.test.ts +++ b/test/permissions.test.ts @@ -303,4 +303,104 @@ describe("checkWritePermissions", () => { ); }); }); + + describe("non-[bot] actors (e.g. GitHub Copilot)", () => { + // GitHub Copilot SWE Agent sets GITHUB_ACTOR="Copilot" which doesn't + // end with [bot] and is not a valid GitHub user, so the collaborator + // permission API returns 404 with "is not a user". + + const createMockOctokitThat404s = () => ({ + repos: { + getCollaboratorPermissionLevel: async () => { + const err = new Error( + "HttpError: Copilot is not a user - https://docs.github.com/rest/collaborators/collaborators#get-repository-permissions-for-a-user", + ); + (err as any).status = 404; + throw err; + }, + }, + } as any); + + test("should return true for non-[bot] actor in allowed_bots (pre-API check)", async () => { + // The allowed_bots check should happen BEFORE calling the API, + // so this should succeed even with a 404-ing mock. + const mockOctokit = createMockOctokitThat404s(); + const context = createContext(); + context.actor = "Copilot"; + context.inputs.allowedBots = "copilot,cursor"; + + const result = await checkWritePermissions(mockOctokit, context); + + expect(result).toBe(true); + expect(coreInfoSpy).toHaveBeenCalledWith( + "Actor Copilot is in allowed_bots list, skipping permission check", + ); + }); + + test("should return true for non-[bot] actor when allowed_bots is '*' (pre-API check)", async () => { + const mockOctokit = createMockOctokitThat404s(); + const context = createContext(); + context.actor = "Copilot"; + context.inputs.allowedBots = "*"; + + const result = await checkWritePermissions(mockOctokit, context); + + expect(result).toBe(true); + }); + + test("should return true for non-[bot] actor in allowed_bots via 404 fallback", async () => { + // Even if somehow we reach the API call (e.g. race condition or + // future refactor), the 404 catch path should also check allowed_bots. + const mockOctokit = createMockOctokitThat404s(); + const context = createContext(); + context.actor = "SomeNewBot"; + context.inputs.allowedBots = "somenewbot"; + + const result = await checkWritePermissions(mockOctokit, context); + + expect(result).toBe(true); + }); + + test("should return false for non-[bot] actor that 404s and is not in allowed_bots", async () => { + const mockOctokit = createMockOctokitThat404s(); + const context = createContext(); + context.actor = "Copilot"; + context.inputs.allowedBots = "cursor"; + + const result = await checkWritePermissions(mockOctokit, context); + + expect(result).toBe(false); + expect(coreWarningSpy).toHaveBeenCalledWith( + "Non-user actor Copilot is not in allowed_bots list. Add it to allowed_bots or use '*' to allow all bots.", + ); + }); + + test("should return false for non-[bot] actor that 404s with empty allowed_bots", async () => { + const mockOctokit = createMockOctokitThat404s(); + const context = createContext(); + context.actor = "Copilot"; + context.inputs.allowedBots = ""; + + const result = await checkWritePermissions(mockOctokit, context); + + expect(result).toBe(false); + }); + + test("should still throw for non-404 API errors", async () => { + const mockOctokit = { + repos: { + getCollaboratorPermissionLevel: async () => { + throw new Error("Internal Server Error"); + }, + }, + } as any; + const context = createContext(); + context.actor = "Copilot"; + context.inputs.allowedBots = ""; + + await expect(checkWritePermissions(mockOctokit, context)).rejects.toThrow( + "Failed to check permissions for Copilot", + ); + }); + }); });