From 6b0302d22680d921ac0a4572fe71dd25bdfd515d Mon Sep 17 00:00:00 2001 From: Vlad Mocanu Date: Thu, 26 Feb 2026 12:31:12 +0200 Subject: [PATCH] =?UTF-8?q?fix(prd-7):=20M3=20security=20review=20?= =?UTF-8?q?=E2=80=94=20self-trigger=20guard,=20actor=20permissions,=20trig?= =?UTF-8?q?ger=20bypass,=20platform=20validation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- prds/7-senior-review-forgejo.md | 2 +- src/forgejo/comment-logic.ts | 6 ++ src/github/validation/actor.ts | 14 +++- src/github/validation/permissions.ts | 45 ++++++++-- src/github/validation/trigger.ts | 12 +-- src/platform/detector.ts | 12 ++- test/actor.test.ts | 39 ++++++++- test/install-mcp-server.test.ts | 2 + test/m7-integration.test.ts | 1 + test/permissions.test.ts | 120 +++++++++++++++------------ test/platform-detector.test.ts | 13 +++ test/trigger-validation.test.ts | 72 +++++++++++++++- 12 files changed, 270 insertions(+), 68 deletions(-) diff --git a/prds/7-senior-review-forgejo.md b/prds/7-senior-review-forgejo.md index 1607be0..27d38cf 100644 --- a/prds/7-senior-review-forgejo.md +++ b/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? diff --git a/src/forgejo/comment-logic.ts b/src/forgejo/comment-logic.ts index 7c1b458..1846dfd 100644 --- a/src/forgejo/comment-logic.ts +++ b/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, diff --git a/src/github/validation/actor.ts b/src/github/validation/actor.ts index ed7f5b4..763d704 100644 --- a/src/github/validation/actor.ts +++ b/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; } diff --git a/src/github/validation/permissions.ts b/src/github/validation/permissions.ts index 59a6742..b0666b2 100644 --- a/src/github/validation/permissions.ts +++ b/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 { + // 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; } diff --git a/src/github/validation/trigger.ts b/src/github/validation/trigger.ts index 031feb8..ac4782d 100644 --- a/src/github/validation/trigger.ts +++ b/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; } } diff --git a/src/platform/detector.ts b/src/platform/detector.ts index 9938939..fd6af93 100644 --- a/src/platform/detector.ts +++ b/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") { - result = "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"; } diff --git a/test/actor.test.ts b/test/actor.test.ts index 1c216ad..79e75d6 100644 --- a/test/actor.test.ts +++ b/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", + ); + }); }); }); diff --git a/test/install-mcp-server.test.ts b/test/install-mcp-server.test.ts index b5322b5..9c79fe1 100644 --- a/test/install-mcp-server.test.ts +++ b/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(() => { diff --git a/test/m7-integration.test.ts b/test/m7-integration.test.ts index d717265..eaeef97 100644 --- a/test/m7-integration.test.ts +++ b/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"); }); diff --git a/test/permissions.test.ts b/test/permissions.test.ts index 2ca5eee..ac456da 100644 --- a/test/permissions.test.ts +++ b/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( diff --git a/test/platform-detector.test.ts b/test/platform-detector.test.ts index d3b1a30..48e9d03 100644 --- a/test/platform-detector.test.ts +++ b/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", () => { diff --git a/test/trigger-validation.test.ts b/test/trigger-validation.test.ts index 36c41f2..2fccd04 100644 --- a/test/trigger-validation.test.ts +++ b/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(