fix(prd-7): M3 security review — self-trigger guard, actor permissions, trigger bypass, platform validation

- Add self-mention/bot loop prevention in actor.ts (blocks actor matching bot_name)
- Switch Forgejo permission check to actor-level via getCollaboratorPermissionLevel
  with fallback to token-level repos.get() when collaborator API returns 403
- Close empty-body PR review comment trigger bypass (now returns false instead of true)
- Add platform detection cross-validation (reject PLATFORM=forgejo on github.com)
- Document URL builder defense-in-depth rationale in comment-logic.ts
- Update/add tests across 6 test files (actor, permissions, trigger, platform, mcp, m7)

Progress: 50% complete (3/6 milestones) — next: M4 test coverage review

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

prds/7-senior-review-forgejo.md

@@ -76,7 +76,7 @@ Perform a thorough, documented code review of all Forgejo-related changes. The r
   - Review fallback behavior (e.g., `repos.get()` fallback for permissions)
   - **Deliverable**: List of error handling gaps with fixes
 
-- [ ] **M3: Security review**
+- [x] **M3: Security review**
 
   - Token handling: is the token properly scoped and never logged?
   - Input sanitization: can webhook payloads inject content?

src/forgejo/comment-logic.ts

@@ -51,6 +51,12 @@ export const FORGEJO_SPINNER_HTML = "⏳";
 // URL Builders
 // ---------------------------------------------------------------------------
 
+// Note: owner, repo, runId, and branchName are NOT URI-encoded in path
+// segments because: (1) owner/repo are validated by Forgejo (alphanumeric +
+// .-_), (2) branchName is validated by validateBranchName() which restricts
+// to [a-zA-Z0-9._/-], and (3) encoding "/" in branch names breaks Forgejo
+// URLs (e.g., "claude/fix-bug" → "claude%2Ffix-bug" is invalid).
+
 export function createForgejoJobRunLink(
   owner: string,
   repo: string,

src/github/validation/actor.ts

@@ -14,10 +14,20 @@ export async function checkHumanActor(
   githubContext: GitHubContext,
 ) {
   // Forgejo doesn't have GitHub-style bot/app accounts, and the user API
-  // requires read:user scope which may not be available. Skip entirely.
+  // requires read:user scope which may not be available. Skip the bot-type
+  // check but still guard against self-triggering (infinite loop prevention).
   if (detectPlatform() === "forgejo") {
+    const botName = githubContext.inputs.botName.replace(/\[bot\]$/, "");
+    if (
+      githubContext.actor === botName ||
+      githubContext.actor === githubContext.inputs.botName
+    ) {
+      throw new Error(
+        `Blocking self-trigger on Forgejo: actor '${githubContext.actor}' matches bot_name '${githubContext.inputs.botName}'. This prevents infinite loops.`,
+      );
+    }
     console.log(
-      `Skipping human actor check on Forgejo for: ${githubContext.actor}`,
+      `Forgejo actor check passed for: ${githubContext.actor} (bot_name: ${githubContext.inputs.botName})`,
     );
     return;
   }

src/github/validation/permissions.ts

@@ -83,16 +83,51 @@ export async function checkWritePermissions(
 }
 
 /**
- * Forgejo permission check using the repo endpoint.
- * GET /repos/{owner}/{repo} returns a `permissions` object for the
- * authenticated token with { admin, push, pull } booleans. We check
- * that the token has push (write) access.
+ * Forgejo permission check using the collaborator permission endpoint.
+ *
+ * Primary: GET /repos/{owner}/{repo}/collaborators/{username}/permission
+ * Returns { "permission": "admin"|"write"|"read"|"none" } — checks the
+ * actor's permission, not the token's.
+ *
+ * Fallback: GET /repos/{owner}/{repo} — checks the token's permission
+ * (less precise but works if the collaborator endpoint returns 403).
  */
 async function checkForgejoPermissions(
   octokit: Octokit,
   repository: { owner: string; repo: string },
   actor: string,
 ): Promise<boolean> {
+  // Try actor-level permission check first (requires repo admin or token with
+  // sufficient scope to read collaborators).
+  try {
+    const response = await octokit.repos.getCollaboratorPermissionLevel({
+      owner: repository.owner,
+      repo: repository.repo,
+      username: actor,
+    });
+
+    const permissionLevel = response.data.permission;
+    core.info(`Forgejo actor '${actor}' permission level: ${permissionLevel}`);
+
+    if (permissionLevel === "admin" || permissionLevel === "write") {
+      return true;
+    }
+
+    core.warning(
+      `Forgejo actor '${actor}' has insufficient permissions: ${permissionLevel}`,
+    );
+    return false;
+  } catch (error) {
+    // Forgejo may return 403 if the token lacks admin scope. Fall back to
+    // token-level check and log a warning about the reduced security.
+    core.warning(
+      `Forgejo collaborator permission check failed for '${actor}' (${error instanceof Error ? error.message : String(error)}). ` +
+        `Falling back to token-level permission check. ` +
+        `For actor-level checks, use a token with read:organization scope.`,
+    );
+  }
+
+  // Fallback: check the token's own permission on the repo
   const response = await octokit.repos.get({
     owner: repository.owner,
     repo: repository.repo,
@@ -111,7 +146,7 @@ async function checkForgejoPermissions(
 
   if (permissions.admin || permissions.push) {
     core.info(
-      `Forgejo token has write access (admin=${permissions.admin}, push=${permissions.push}) for actor: ${actor}`,
+      `Forgejo token has write access (admin=${permissions.admin}, push=${permissions.push}) for actor: ${actor} (token-level fallback)`,
     );
     return true;
   }

src/github/validation/trigger.ts

@@ -145,18 +145,20 @@ export function checkContainsTrigger(context: ParsedGitHubContext): boolean {
       return true;
     }
 
-    // On Forgejo, if no body was found in the payload at all, still trigger —
-    // the actual comment data will be fetched via REST in the data fetcher.
-    // This handles edge cases where Forgejo strips body from the webhook payload.
+    // On Forgejo, pull_request_review_comment payloads may arrive with an
+    // empty body. Rather than triggering unconditionally (which bypasses the
+    // trigger phrase check), we skip. The user can re-trigger with a comment
+    // that includes the trigger phrase. This prevents Claude from responding
+    // to comments not directed at it.
     if (
       isPullRequestReviewCommentEvent(context) &&
       detectPlatform() === "forgejo" &&
       !commentBody
     ) {
       console.log(
-        "Forgejo pull_request_review_comment: no body in payload, triggering (data fetched via REST)",
+        "Forgejo pull_request_review_comment: no body in payload, skipping (cannot verify trigger phrase)",
       );
-      return true;
+      return false;
     }
   }
 

src/platform/detector.ts

@@ -23,7 +23,17 @@ export function detectPlatform(): Platform {
   // 1. Explicit platform from action.yml detect step or user input
   const platformEnv = process.env.PLATFORM;
   if (platformEnv === "forgejo") {
+    // Cross-validate: prevent forcing Forgejo mode on GitHub.com, which would
+    // skip actor validation and use a weaker permission model.
+    const serverUrl = process.env.GITHUB_SERVER_URL || "";
+    if (serverUrl.includes("github.com")) {
+      console.warn(
+        `Platform override 'forgejo' ignored: GITHUB_SERVER_URL (${serverUrl}) indicates GitHub.com. ` +
+          `Falling through to standard detection.`,
+      );
+    } else {
       result = "forgejo";
+    }
   } else if (platformEnv === "github") {
     result = "github";
   }

test/actor.test.ts

@@ -121,6 +121,8 @@ describe("checkHumanActor", () => {
       savedPlatform = process.env.PLATFORM;
       savedServerUrl = process.env.GITHUB_SERVER_URL;
       process.env.PLATFORM = "forgejo";
+      // Ensure cross-validation doesn't reject PLATFORM=forgejo
+      process.env.GITHUB_SERVER_URL = "https://git.example.com";
     });
 
     afterEach(() => {
@@ -132,7 +134,7 @@ describe("checkHumanActor", () => {
       _resetPlatformCache();
     });
 
-    test("should skip actor check entirely on Forgejo", async () => {
+    test("should skip GitHub API actor check on Forgejo for regular users", async () => {
       let apiCalled = false;
       const mockOctokit = {
         users: {
@@ -151,7 +153,7 @@ describe("checkHumanActor", () => {
       expect(apiCalled).toBe(false);
     });
 
-    test("should skip actor check for bot-like names on Forgejo", async () => {
+    test("should skip actor check for bot-like names that don't match bot_name", async () => {
       let apiCalled = false;
       const mockOctokit = {
         users: {
@@ -170,5 +172,38 @@ describe("checkHumanActor", () => {
       ).resolves.toBeUndefined();
       expect(apiCalled).toBe(false);
     });
+
+    test("should block self-trigger when actor matches bot_name (with [bot] suffix)", async () => {
+      const mockOctokit = createMockOctokit("User");
+      const context = createMockContext();
+      // Default botName is "claude[bot]" from CLAUDE_BOT_LOGIN
+      context.actor = "claude[bot]";
+
+      await expect(checkHumanActor(mockOctokit, context)).rejects.toThrow(
+        "Blocking self-trigger on Forgejo",
+      );
+    });
+
+    test("should block self-trigger when actor matches bot_name without [bot] suffix", async () => {
+      const mockOctokit = createMockOctokit("User");
+      const context = createMockContext();
+      context.inputs.botName = "claude[bot]";
+      context.actor = "claude";
+
+      await expect(checkHumanActor(mockOctokit, context)).rejects.toThrow(
+        "Blocking self-trigger on Forgejo",
+      );
+    });
+
+    test("should block self-trigger with custom bot_name", async () => {
+      const mockOctokit = createMockOctokit("User");
+      const context = createMockContext();
+      context.inputs.botName = "my-assistant";
+      context.actor = "my-assistant";
+
+      await expect(checkHumanActor(mockOctokit, context)).rejects.toThrow(
+        "Blocking self-trigger on Forgejo",
+      );
+    });
   });
 });

test/install-mcp-server.test.ts

@@ -328,6 +328,8 @@ describe("prepareMcpConfig", () => {
       savedServerUrl = process.env.GITHUB_SERVER_URL;
       // Set platform to forgejo for all tests in this block
       process.env.PLATFORM = "forgejo";
+      // Ensure cross-validation doesn't reject PLATFORM=forgejo
+      process.env.GITHUB_SERVER_URL = "https://git.example.com";
     });
 
     afterEach(() => {

test/m7-integration.test.ts

@@ -52,6 +52,7 @@ describe("M7: Platform routing for mode handlers", () => {
   describe("detectPlatform() — used by tag mode and update-comment-link", () => {
     it('returns "forgejo" when PLATFORM=forgejo', () => {
       process.env.PLATFORM = "forgejo";
+      process.env.GITHUB_SERVER_URL = "https://git.example.com";
       expect(detectPlatform()).toBe("forgejo");
     });
 

test/permissions.test.ts

@@ -316,6 +316,8 @@ describe("checkWritePermissions", () => {
       savedForgejo = process.env.FORGEJO;
       savedServerUrl = process.env.GITHUB_SERVER_URL;
       process.env.PLATFORM = "forgejo";
+      // Ensure cross-validation doesn't reject PLATFORM=forgejo
+      process.env.GITHUB_SERVER_URL = "https://git.example.com";
     });
 
     afterEach(() => {
@@ -329,25 +331,75 @@ describe("checkWritePermissions", () => {
       _resetPlatformCache();
     });
 
-    const createForgejoOctokit = (permissions: {
+    // Helper: Forgejo octokit where collaborator API works (actor-level check)
+    const createForgejoActorOctokit = (permission: string) => {
+      return {
+        repos: {
+          getCollaboratorPermissionLevel: async () => ({
+            data: { permission },
+          }),
+          get: async () => {
+            throw new Error("Should not fall back to repos.get");
+          },
+        },
+      } as any;
+    };
+
+    // Helper: Forgejo octokit where collaborator API fails (falls back to token-level)
+    const createForgejoFallbackOctokit = (permissions: {
       admin?: boolean;
       push?: boolean;
       pull?: boolean;
     }) => {
       return {
         repos: {
+          getCollaboratorPermissionLevel: async () => {
+            throw new Error("403 Forbidden");
+          },
           get: async () => ({
             data: { permissions },
           }),
-          getCollaboratorPermissionLevel: async () => {
-            throw new Error("Should not be called on Forgejo");
-          },
         },
       } as any;
     };
 
-    test("should return true when Forgejo token has push access", async () => {
-      const mockOctokit = createForgejoOctokit({
+    test("should use actor-level permission check on Forgejo (write)", async () => {
+      const mockOctokit = createForgejoActorOctokit("write");
+      const context = createContext();
+
+      const result = await checkWritePermissions(mockOctokit, context);
+
+      expect(result).toBe(true);
+      expect(coreInfoSpy).toHaveBeenCalledWith(
+        expect.stringContaining(
+          "Forgejo actor 'test-user' permission level: write",
+        ),
+      );
+    });
+
+    test("should use actor-level permission check on Forgejo (admin)", async () => {
+      const mockOctokit = createForgejoActorOctokit("admin");
+      const context = createContext();
+
+      const result = await checkWritePermissions(mockOctokit, context);
+
+      expect(result).toBe(true);
+    });
+
+    test("should reject actor with read-only permission on Forgejo", async () => {
+      const mockOctokit = createForgejoActorOctokit("read");
+      const context = createContext();
+
+      const result = await checkWritePermissions(mockOctokit, context);
+
+      expect(result).toBe(false);
+      expect(coreWarningSpy).toHaveBeenCalledWith(
+        expect.stringContaining("insufficient permissions: read"),
+      );
+    });
+
+    test("should fall back to token-level check when collaborator API fails", async () => {
+      const mockOctokit = createForgejoFallbackOctokit({
         admin: false,
         push: true,
         pull: true,
@@ -357,26 +409,16 @@ describe("checkWritePermissions", () => {
       const result = await checkWritePermissions(mockOctokit, context);
 
       expect(result).toBe(true);
+      expect(coreWarningSpy).toHaveBeenCalledWith(
+        expect.stringContaining("Falling back to token-level permission check"),
+      );
       expect(coreInfoSpy).toHaveBeenCalledWith(
-        expect.stringContaining("Forgejo token has write access"),
+        expect.stringContaining("token-level fallback"),
       );
     });
 
-    test("should return true when Forgejo token has admin access", async () => {
-      const mockOctokit = createForgejoOctokit({
-        admin: true,
-        push: false,
-        pull: true,
-      });
-      const context = createContext();
-
-      const result = await checkWritePermissions(mockOctokit, context);
-
-      expect(result).toBe(true);
-    });
-
-    test("should return false when Forgejo token has only pull access", async () => {
-      const mockOctokit = createForgejoOctokit({
+    test("should return false on fallback when token has only pull access", async () => {
+      const mockOctokit = createForgejoFallbackOctokit({
         admin: false,
         push: false,
         pull: true,
@@ -386,14 +428,14 @@ describe("checkWritePermissions", () => {
       const result = await checkWritePermissions(mockOctokit, context);
 
       expect(result).toBe(false);
-      expect(coreWarningSpy).toHaveBeenCalledWith(
-        expect.stringContaining("insufficient permissions"),
-      );
     });
 
-    test("should return false when permissions object is missing", async () => {
+    test("should return false on fallback when permissions object is missing", async () => {
       const mockOctokit = {
         repos: {
+          getCollaboratorPermissionLevel: async () => {
+            throw new Error("403 Forbidden");
+          },
           get: async () => ({ data: {} }),
         },
       } as any;
@@ -407,32 +449,8 @@ describe("checkWritePermissions", () => {
       );
     });
 
-    test("should not call getCollaboratorPermissionLevel on Forgejo", async () => {
-      let collaboratorCalled = false;
-      const mockOctokit = {
-        repos: {
-          get: async () => ({
-            data: { permissions: { admin: false, push: true, pull: true } },
-          }),
-          getCollaboratorPermissionLevel: async () => {
-            collaboratorCalled = true;
-            return { data: { permission: "write" } };
-          },
-        },
-      } as any;
-      const context = createContext();
-
-      await checkWritePermissions(mockOctokit, context);
-
-      expect(collaboratorCalled).toBe(false);
-    });
-
     test("should still bypass for allowed_non_write_users on Forgejo", async () => {
-      const mockOctokit = createForgejoOctokit({
-        admin: false,
-        push: false,
-        pull: true,
-      });
+      const mockOctokit = createForgejoActorOctokit("read");
       const context = createContext();
 
       const result = await checkWritePermissions(

test/platform-detector.test.ts

@@ -41,6 +41,19 @@ describe("detectPlatform", () => {
       // No other env vars set, should default to github
       expect(detectPlatform()).toBe("github");
     });
+
+    it("should reject PLATFORM=forgejo when GITHUB_SERVER_URL is github.com", () => {
+      process.env.PLATFORM = "forgejo";
+      process.env.GITHUB_SERVER_URL = "https://github.com";
+      // Cross-validation guard: prevent forcing Forgejo mode on GitHub.com
+      expect(detectPlatform()).toBe("github");
+    });
+
+    it("should accept PLATFORM=forgejo when GITHUB_SERVER_URL is not github.com", () => {
+      process.env.PLATFORM = "forgejo";
+      process.env.GITHUB_SERVER_URL = "https://git.example.com";
+      expect(detectPlatform()).toBe("forgejo");
+    });
   });
 
   describe("FORGEJO env var", () => {

test/trigger-validation.test.ts

@@ -2,7 +2,8 @@ import {
   checkContainsTrigger,
   escapeRegExp,
 } from "../src/github/validation/trigger";
-import { describe, it, expect } from "bun:test";
+import { describe, it, expect, beforeEach, afterEach } from "bun:test";
+import { _resetPlatformCache } from "../src/platform/detector";
 import {
   createMockContext,
   mockIssueAssignedContext,
@@ -451,6 +452,75 @@ describe("checkContainsTrigger", () => {
   });
 });
 
+describe("Forgejo empty-body trigger bypass", () => {
+  let savedPlatform: string | undefined;
+  let savedServerUrl: string | undefined;
+
+  beforeEach(() => {
+    _resetPlatformCache();
+    savedPlatform = process.env.PLATFORM;
+    savedServerUrl = process.env.GITHUB_SERVER_URL;
+    process.env.PLATFORM = "forgejo";
+    process.env.GITHUB_SERVER_URL = "https://git.example.com";
+  });
+
+  afterEach(() => {
+    if (savedPlatform !== undefined) process.env.PLATFORM = savedPlatform;
+    else delete process.env.PLATFORM;
+    if (savedServerUrl !== undefined)
+      process.env.GITHUB_SERVER_URL = savedServerUrl;
+    else delete process.env.GITHUB_SERVER_URL;
+    _resetPlatformCache();
+  });
+
+  it("should NOT trigger when Forgejo PR review comment has no body", () => {
+    // Previously this returned true unconditionally, bypassing trigger check
+    const context = {
+      ...mockPullRequestReviewCommentContext,
+      payload: {
+        ...mockPullRequestReviewCommentContext.payload,
+        comment: {
+          ...(mockPullRequestReviewCommentContext.payload as any).comment,
+          body: "",
+        },
+      },
+    } as ParsedGitHubContext;
+    expect(checkContainsTrigger(context)).toBe(false);
+  });
+
+  it("should NOT trigger when Forgejo PR review comment body is missing", () => {
+    const context = {
+      ...mockPullRequestReviewCommentContext,
+      payload: {
+        ...mockPullRequestReviewCommentContext.payload,
+        comment: {
+          ...(mockPullRequestReviewCommentContext.payload as any).comment,
+          body: undefined,
+        },
+      },
+    } as ParsedGitHubContext;
+    expect(checkContainsTrigger(context)).toBe(false);
+  });
+
+  it("should still trigger when Forgejo PR review comment contains trigger phrase", () => {
+    const context = {
+      ...mockPullRequestReviewCommentContext,
+      inputs: {
+        ...mockPullRequestReviewCommentContext.inputs,
+        triggerPhrase: "@claude",
+      },
+      payload: {
+        ...mockPullRequestReviewCommentContext.payload,
+        comment: {
+          ...(mockPullRequestReviewCommentContext.payload as any).comment,
+          body: "@claude please review this line",
+        },
+      },
+    } as ParsedGitHubContext;
+    expect(checkContainsTrigger(context)).toBe(true);
+  });
+});
+
 describe("escapeRegExp", () => {
   it("should escape special regex characters", () => {
     expect(escapeRegExp(".*+?^${}()|[]\\")).toBe(