diff --git a/src/github/validation/actor.ts b/src/github/validation/actor.ts index c21481c6..110048b3 100644 --- a/src/github/validation/actor.ts +++ b/src/github/validation/actor.ts @@ -32,35 +32,32 @@ export async function checkHumanActor( githubContext: GitHubContext, ) { const allowedBots = githubContext.inputs.allowedBots; + const actor = githubContext.actor; - // 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 + // Resolve the actor's account type before consulting allowed_bots so the + // allow-list only ever applies to non-User accounts. Some app actors + // (e.g. GitHub Copilot with GITHUB_ACTOR="Copilot") are not resolvable + // via the Users API and 404 — that path is handled in the catch below. let actorType: string; try { const { data: userData } = await octokit.users.getByUsername({ - username: githubContext.actor, + username: 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\]$/, ""); + // Unresolvable actors are GitHub Apps without a backing user account. + if (isAllowedBot(actor, allowedBots)) { + console.log( + `Actor ${actor} is in allowed_bots list, skipping human actor check`, + ); + return; + } + const botName = 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.`, ); @@ -70,15 +67,22 @@ export async function checkHumanActor( console.log(`Actor type: ${actorType}`); - // Check bot permissions if actor is not a User if (actorType !== "User") { - const botName = githubContext.actor.toLowerCase().replace(/\[bot\]$/, ""); - - // Bot not allowed (we already checked allowed_bots above) + // GitHub Apps and other bot accounts. + if (isAllowedBot(actor, allowedBots)) { + console.log( + `Actor ${actor} is in allowed_bots list, skipping human actor check`, + ); + return; + } + const botName = actor.toLowerCase().replace(/\[bot\]$/, ""); throw new Error( `Workflow initiated by non-human actor: ${botName} (type: ${actorType}). Add bot to allowed_bots list or use '*' to allow all bots.`, ); } - console.log(`Verified human actor: ${githubContext.actor}`); + // Regular User account. allowed_bots is only for bot actors and is not + // consulted here; write-access enforcement for users happens separately + // in checkWritePermissions. + console.log(`Verified human actor: ${actor}`); } diff --git a/src/github/validation/permissions.ts b/src/github/validation/permissions.ts index 502db0ff..9b6600a3 100644 --- a/src/github/validation/permissions.ts +++ b/src/github/validation/permissions.ts @@ -66,22 +66,19 @@ export async function checkWritePermissions( } } - // Check if the actor is a GitHub App (bot user with [bot] suffix) + // Check if the actor is a GitHub App (bot user with [bot] suffix). + // Usernames cannot contain "[" or "]", so the suffix is a reliable + // bot signal that doesn't require an API lookup. 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 + // For all other actors, resolve the account via the collaborator + // permission endpoint. allowed_bots is only consulted in the catch + // block below, after the API has confirmed the actor is not a regular + // user account (e.g. GitHub Apps like Copilot whose GITHUB_ACTOR is + // "Copilot" rather than "Copilot[bot]"). const response = await octokit.repos.getCollaboratorPermissionLevel({ owner: repository.owner, repo: repository.repo, diff --git a/test/actor.test.ts b/test/actor.test.ts index 22df940b..408a6e7c 100644 --- a/test/actor.test.ts +++ b/test/actor.test.ts @@ -97,7 +97,8 @@ describe("checkHumanActor", () => { 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. + // Users API. allowed_bots is applied once the API has resolved the + // actor as not being a regular user account. function createMockOctokitThat404s(): Octokit { return { @@ -117,7 +118,6 @@ describe("checkHumanActor", () => { 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(); @@ -167,4 +167,52 @@ describe("checkHumanActor", () => { ).resolves.toBeUndefined(); }); }); + + describe("account type resolution", () => { + // The Users API resolves the actor's account type before allowed_bots + // is consulted. allowed_bots is only relevant for Bot accounts and + // unresolvable app actors; it does not change behavior for regular + // User accounts. + + test("should pass for a User account whose name matches allowed_bots", async () => { + const mockOctokit = createMockOctokit("User"); + const context = createMockContext(); + context.actor = "renovate"; + context.inputs.allowedBots = "renovate"; + + await expect( + checkHumanActor(mockOctokit, context), + ).resolves.toBeUndefined(); + }); + + test("should pass for a User account when allowed_bots is '*'", async () => { + const mockOctokit = createMockOctokit("User"); + const context = createMockContext(); + context.actor = "some-user"; + context.inputs.allowedBots = "*"; + + await expect( + checkHumanActor(mockOctokit, context), + ).resolves.toBeUndefined(); + }); + + test("should resolve account type even when actor name appears in allowed_bots", async () => { + // The Users API call should not be short-circuited by allowed_bots, + // so an unexpected API error propagates instead of being swallowed. + const mockOctokit = { + users: { + getByUsername: async () => { + throw new Error("Internal Server Error"); + }, + }, + } as unknown as Octokit; + const context = createMockContext(); + context.actor = "some-user"; + context.inputs.allowedBots = "some-user"; + + await expect(checkHumanActor(mockOctokit, context)).rejects.toThrow( + "Internal Server Error", + ); + }); + }); }); diff --git a/test/permissions.test.ts b/test/permissions.test.ts index 94aeaaf1..f89a7716 100644 --- a/test/permissions.test.ts +++ b/test/permissions.test.ts @@ -307,7 +307,9 @@ 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". + // permission API returns 404 with "is not a user". allowed_bots is + // applied in that catch path once the API has confirmed the actor is + // not a regular user account. const createMockOctokitThat404s = () => ({ @@ -322,9 +324,7 @@ describe("checkWritePermissions", () => { }, }) 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. + test("should return true for non-[bot] app actor in allowed_bots", async () => { const mockOctokit = createMockOctokitThat404s(); const context = createContext(); context.actor = "Copilot"; @@ -334,11 +334,11 @@ describe("checkWritePermissions", () => { expect(result).toBe(true); expect(coreInfoSpy).toHaveBeenCalledWith( - "Actor Copilot is in allowed_bots list, skipping permission check", + "Non-user actor Copilot is in allowed_bots list, granting access", ); }); - test("should return true for non-[bot] actor when allowed_bots is '*' (pre-API check)", async () => { + test("should return true for non-[bot] app actor when allowed_bots is '*'", async () => { const mockOctokit = createMockOctokitThat404s(); const context = createContext(); context.actor = "Copilot"; @@ -349,20 +349,18 @@ describe("checkWritePermissions", () => { 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. + test("should match config entries written with the [bot] suffix", async () => { const mockOctokit = createMockOctokitThat404s(); const context = createContext(); context.actor = "SomeNewBot"; - context.inputs.allowedBots = "somenewbot"; + context.inputs.allowedBots = "somenewbot[bot]"; 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 () => { + test("should return false for non-[bot] app actor that is not in allowed_bots", async () => { const mockOctokit = createMockOctokitThat404s(); const context = createContext(); context.actor = "Copilot"; @@ -376,7 +374,7 @@ describe("checkWritePermissions", () => { ); }); - test("should return false for non-[bot] actor that 404s with empty allowed_bots", async () => { + test("should return false for non-[bot] app actor with empty allowed_bots", async () => { const mockOctokit = createMockOctokitThat404s(); const context = createContext(); context.actor = "Copilot"; @@ -404,4 +402,57 @@ describe("checkWritePermissions", () => { ); }); }); + + describe("allowed_bots only applies to non-user actors", () => { + // The permission endpoint resolves the actor's account type. Actors + // that resolve to a regular user account go through the standard write + // permission check; allowed_bots does not short-circuit it for them. + + test("should require write permission for a user account whose name matches allowed_bots", async () => { + const mockOctokit = createMockOctokit("read"); + const context = createContext(); + context.actor = "renovate"; + context.inputs.allowedBots = "renovate"; + + const result = await checkWritePermissions(mockOctokit, context); + + expect(result).toBe(false); + expect(coreWarningSpy).toHaveBeenCalledWith( + "Actor has insufficient permissions: read", + ); + }); + + test("should require write permission for a user account when allowed_bots uses the [bot] form", async () => { + const mockOctokit = createMockOctokit("read"); + const context = createContext(); + context.actor = "renovate"; + context.inputs.allowedBots = "renovate[bot]"; + + const result = await checkWritePermissions(mockOctokit, context); + + expect(result).toBe(false); + }); + + test("should require write permission for a user account when allowed_bots is '*'", async () => { + const mockOctokit = createMockOctokit("none"); + const context = createContext(); + context.actor = "some-user"; + context.inputs.allowedBots = "*"; + + const result = await checkWritePermissions(mockOctokit, context); + + expect(result).toBe(false); + }); + + test("should still grant access for a user account with write permission", async () => { + const mockOctokit = createMockOctokit("write"); + const context = createContext(); + context.actor = "renovate"; + context.inputs.allowedBots = "renovate"; + + const result = await checkWritePermissions(mockOctokit, context); + + expect(result).toBe(true); + }); + }); });