fix: resolve GitHub bot permissions validation security issues

lroolle · claude · lroolle · commit 71fe6a285b4b · 2025-07-16T07:17:29.000-07:00
- Fix bot permission validation flaw: replace read-only repos.get() with proper write permission checks - Standardize bot detection: use GitHub API userData.type consistently across files - Add comprehensive test coverage for bot permission scenarios - Remove unnecessary code comments for cleaner code Resolves BugBot security findings: - Prevents read-only bots from bypassing write permission validation - Eliminates inconsistent bot detection methods - Ensures proper authorization validation for GitHub Apps 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
diff --git a/src/github/validation/permissions.ts b/src/github/validation/permissions.ts
@@ -8,6 +8,21 @@ import type { Octokit } from "@octokit/rest";
  * @param context - The GitHub context
  * @returns true if the actor has write permissions, false otherwise
  */
+async function getBotActorType(
+  octokit: Octokit,
+  actor: string,
+): Promise<string | null> {
+  try {
+    const { data: userData } = await octokit.users.getByUsername({
+      username: actor,
+    });
+    return userData.type;
+  } catch (error) {
+    core.warning(`Failed to get user data for ${actor}: ${error}`);
+    return null;
+  }
+}
+
 export async function checkWritePermissions(
   octokit: Octokit,
   context: ParsedGitHubContext,
@@ -17,26 +32,60 @@ export async function checkWritePermissions(
   try {
     core.info(`Checking permissions for actor: ${actor}`);
 
-    // For GitHub Apps (like claude-yolo[bot]), check if we can perform write operations
-    if (actor.endsWith("[bot]")) {
-      core.info(`GitHub App detected: ${actor}, checking app installation`);
+    const actorType = await getBotActorType(octokit, actor);
+
+    if (actorType === "Bot") {
+      core.info(`GitHub App detected: ${actor}, checking write permissions`);
 
       try {
-        // Try to get the repository to verify the app has access
-        await octokit.repos.get({
+        const response = await octokit.repos.getCollaboratorPermissionLevel({
           owner: repository.owner,
           repo: repository.repo,
+          username: actor,
         });
 
-        core.info(`App has repository access: ${actor}`);
-        return true;
+        const permissionLevel = response.data.permission;
+        core.info(`App permission level: ${permissionLevel}`);
+
+        if (permissionLevel === "admin" || permissionLevel === "write") {
+          core.info(`App has write access: ${permissionLevel}`);
+          return true;
+        } else {
+          core.warning(`App has insufficient permissions: ${permissionLevel}`);
+          return false;
+        }
       } catch (error) {
-        core.warning(`App lacks repository access: ${error}`);
-        return false;
+        core.warning(
+          `Could not check collaborator permissions for bot, checking app installation: ${error}`,
+        );
+
+        try {
+          const installation = await octokit.apps.getRepoInstallation({
+            owner: repository.owner,
+            repo: repository.repo,
+          });
+
+          core.info(`App installation found: ${installation.data.id}`);
+
+          const permissions = installation.data.permissions;
+          if (
+            permissions &&
+            (permissions.contents === "write" ||
+              permissions.contents === "admin")
+          ) {
+            core.info(`App has write permissions via installation`);
+            return true;
+          } else {
+            core.warning(`App lacks write permissions in installation`);
+            return false;
+          }
+        } catch (installationError) {
+          core.warning(`App lacks repository access: ${installationError}`);
+          return false;
+        }
       }
     }
 
-    // For human users, check permissions using the collaborator endpoint
     const response = await octokit.repos.getCollaboratorPermissionLevel({
       owner: repository.owner,
       repo: repository.repo,
diff --git a/test/permissions-validation.test.ts b/test/permissions-validation.test.ts
@@ -0,0 +1,244 @@
+import { describe, expect, test, spyOn, beforeEach, afterEach } from "bun:test";
+import { checkWritePermissions } from "../src/github/validation/permissions";
+import type { ParsedGitHubContext } from "../src/github/context";
+
+describe("checkWritePermissions", () => {
+  let coreSpy: any;
+
+  beforeEach(() => {
+    coreSpy = {
+      info: spyOn(console, "log").mockImplementation(() => {}),
+      warning: spyOn(console, "warn").mockImplementation(() => {}),
+      error: spyOn(console, "error").mockImplementation(() => {}),
+    };
+  });
+
+  afterEach(() => {
+    coreSpy.info.mockRestore();
+    coreSpy.warning.mockRestore();
+    coreSpy.error.mockRestore();
+  });
+
+  const createContext = (actor: string = "test-user"): ParsedGitHubContext => ({
+    runId: "1234567890",
+    eventName: "issue_comment",
+    eventAction: "created",
+    repository: {
+      full_name: "test-owner/test-repo",
+      owner: "test-owner",
+      repo: "test-repo",
+    },
+    actor,
+    payload: {
+      action: "created",
+      issue: {
+        number: 1,
+        title: "Test Issue",
+        body: "Test body",
+        user: { login: actor },
+      },
+      comment: {
+        id: 123,
+        body: "@claude test",
+        user: { login: actor },
+        html_url:
+          "https://github.com/test-owner/test-repo/issues/1#issuecomment-123",
+      },
+    } as any,
+    entityNumber: 1,
+    isPR: false,
+    inputs: {
+      triggerPhrase: "@claude",
+      assigneeTrigger: "",
+      labelTrigger: "",
+      allowedTools: [],
+      disallowedTools: [],
+      customInstructions: "",
+      directPrompt: "",
+      branchPrefix: "claude/",
+      useStickyComment: false,
+    },
+  });
+
+  test("should grant write permissions to human users with write access", async () => {
+    const mockOctokit = {
+      users: {
+        getByUsername: async () => ({
+          data: { type: "User" },
+        }),
+      },
+      repos: {
+        getCollaboratorPermissionLevel: async () => ({
+          data: { permission: "write" },
+        }),
+      },
+    } as any;
+
+    const context = createContext("human-user");
+    const result = await checkWritePermissions(mockOctokit, context);
+
+    expect(result).toBe(true);
+  });
+
+  test("should deny write permissions to human users with read access", async () => {
+    const mockOctokit = {
+      users: {
+        getByUsername: async () => ({
+          data: { type: "User" },
+        }),
+      },
+      repos: {
+        getCollaboratorPermissionLevel: async () => ({
+          data: { permission: "read" },
+        }),
+      },
+    } as any;
+
+    const context = createContext("human-user");
+    const result = await checkWritePermissions(mockOctokit, context);
+
+    expect(result).toBe(false);
+  });
+
+  test("should grant write permissions to bots with write access via collaborator check", async () => {
+    const mockOctokit = {
+      users: {
+        getByUsername: async () => ({
+          data: { type: "Bot" },
+        }),
+      },
+      repos: {
+        getCollaboratorPermissionLevel: async () => ({
+          data: { permission: "write" },
+        }),
+      },
+    } as any;
+
+    const context = createContext("claude-bot");
+    const result = await checkWritePermissions(mockOctokit, context);
+
+    expect(result).toBe(true);
+  });
+
+  test("should deny write permissions to bots with read access via collaborator check", async () => {
+    const mockOctokit = {
+      users: {
+        getByUsername: async () => ({
+          data: { type: "Bot" },
+        }),
+      },
+      repos: {
+        getCollaboratorPermissionLevel: async () => ({
+          data: { permission: "read" },
+        }),
+      },
+    } as any;
+
+    const context = createContext("claude-bot");
+    const result = await checkWritePermissions(mockOctokit, context);
+
+    expect(result).toBe(false);
+  });
+
+  test("should fallback to app installation check for bots when collaborator check fails", async () => {
+    const mockOctokit = {
+      users: {
+        getByUsername: async () => ({
+          data: { type: "Bot" },
+        }),
+      },
+      repos: {
+        getCollaboratorPermissionLevel: async () => {
+          throw new Error("Not found");
+        },
+      },
+      apps: {
+        getRepoInstallation: async () => ({
+          data: {
+            id: 123,
+            permissions: { contents: "write" },
+          },
+        }),
+      },
+    } as any;
+
+    const context = createContext("claude-bot");
+    const result = await checkWritePermissions(mockOctokit, context);
+
+    expect(result).toBe(true);
+  });
+
+  test("should deny write permissions to bots with read-only installation", async () => {
+    const mockOctokit = {
+      users: {
+        getByUsername: async () => ({
+          data: { type: "Bot" },
+        }),
+      },
+      repos: {
+        getCollaboratorPermissionLevel: async () => {
+          throw new Error("Not found");
+        },
+      },
+      apps: {
+        getRepoInstallation: async () => ({
+          data: {
+            id: 123,
+            permissions: { contents: "read" },
+          },
+        }),
+      },
+    } as any;
+
+    const context = createContext("claude-bot");
+    const result = await checkWritePermissions(mockOctokit, context);
+
+    expect(result).toBe(false);
+  });
+
+  test("should deny write permissions to bots with no installation", async () => {
+    const mockOctokit = {
+      users: {
+        getByUsername: async () => ({
+          data: { type: "Bot" },
+        }),
+      },
+      repos: {
+        getCollaboratorPermissionLevel: async () => {
+          throw new Error("Not found");
+        },
+      },
+      apps: {
+        getRepoInstallation: async () => {
+          throw new Error("Installation not found");
+        },
+      },
+    } as any;
+
+    const context = createContext("claude-bot");
+    const result = await checkWritePermissions(mockOctokit, context);
+
+    expect(result).toBe(false);
+  });
+
+  test("should handle API errors gracefully", async () => {
+    const mockOctokit = {
+      users: {
+        getByUsername: async () => {
+          throw new Error("API Error");
+        },
+      },
+      repos: {
+        getCollaboratorPermissionLevel: async () => {
+          throw new Error("API Error");
+        },
+      },
+    } as any;
+
+    const context = createContext("test-user");
+
+    await expect(checkWritePermissions(mockOctokit, context)).rejects.toThrow(
+      "Failed to check permissions for test-user:",
+    );
+  });
+});
