diff --git a/actions/setup/js/check_membership.cjs b/actions/setup/js/check_membership.cjs index 712f3606c04..64b8a31030d 100644 --- a/actions/setup/js/check_membership.cjs +++ b/actions/setup/js/check_membership.cjs @@ -1,7 +1,7 @@ // @ts-check /// -const { parseRequiredPermissions, parseAllowedBots, checkRepositoryPermission, checkBotStatus, isAllowedBot } = require("./check_permissions_utils.cjs"); +const { parseRequiredPermissions, parseAllowedBots, checkRepositoryPermission, checkBotStatus, isAllowedBot, isConfusedDeputyAttack } = require("./check_permissions_utils.cjs"); const { writeDenialSummary } = require("./pre_activation_summary.cjs"); async function main() { @@ -51,6 +51,21 @@ async function main() { return; } + // Guard against Dependabot Confused Deputy attacks. + // An attacker can trigger @dependabot recreate (for pull_request events) or + // @dependabot show (for issue_comment events) to make dependabot appear as the + // actor, bypassing permission checks that rely solely on github.actor. + // Reference: https://labs.boostsecurity.io/articles/weaponizing-dependabot-pwn-request-at-its-finest/ + if (isConfusedDeputyAttack(actor, eventName, context.payload)) { + const errorMessage = `Access denied: Potential confused deputy attack detected. Actor '${actor}' does not match the event author. The workflow may have been triggered indirectly via a bot command.`; + core.warning(errorMessage); + core.setOutput("is_team_member", "false"); + core.setOutput("result", "confused_deputy"); + core.setOutput("error_message", errorMessage); + await writeDenialSummary(errorMessage, "This can occur when a bot command (e.g. @dependabot recreate) causes a bot to appear as the actor on a PR or comment that was originally authored by a different user."); + return; + } + // Check if the actor has the required repository permissions const result = await checkRepositoryPermission(actor, owner, repo, requiredPermissions); diff --git a/actions/setup/js/check_membership.test.cjs b/actions/setup/js/check_membership.test.cjs index 3432f24162a..775afc29e7b 100644 --- a/actions/setup/js/check_membership.test.cjs +++ b/actions/setup/js/check_membership.test.cjs @@ -257,6 +257,123 @@ describe("check_membership.cjs", () => { }); }); + describe("confused deputy attack protection", () => { + beforeEach(() => { + process.env.GH_AW_REQUIRED_ROLES = "write"; + }); + + it("should deny access when actor differs from PR author (pull_request synchronize event)", async () => { + mockContext.actor = "dependabot[bot]"; + mockContext.eventName = "pull_request"; + mockContext.payload = { action: "synchronize", pull_request: { user: { login: "attacker" } } }; + + await runScript(); + + expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Potential confused deputy attack detected")); + expect(mockCore.setOutput).toHaveBeenCalledWith("is_team_member", "false"); + expect(mockCore.setOutput).toHaveBeenCalledWith("result", "confused_deputy"); + }); + + it("should allow access when actor matches PR author (genuine dependabot PR synchronize)", async () => { + mockContext.actor = "dependabot[bot]"; + mockContext.eventName = "pull_request"; + mockContext.payload = { action: "synchronize", pull_request: { user: { login: "dependabot[bot]" } } }; + + mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({ + data: { permission: "write" }, + }); + + await runScript(); + + expect(mockCore.setOutput).toHaveBeenCalledWith("is_team_member", "true"); + expect(mockCore.setOutput).toHaveBeenCalledWith("result", "authorized"); + }); + + it("should deny access when actor differs from comment author (issue_comment event)", async () => { + mockContext.actor = "dependabot[bot]"; + mockContext.eventName = "issue_comment"; + mockContext.payload = { comment: { user: { login: "attacker" } } }; + + await runScript(); + + expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Potential confused deputy attack detected")); + expect(mockCore.setOutput).toHaveBeenCalledWith("is_team_member", "false"); + expect(mockCore.setOutput).toHaveBeenCalledWith("result", "confused_deputy"); + }); + + it("should not trigger confused deputy for pull_request:labeled even when actor differs from PR author", async () => { + // A team member labeling a PR is legitimate — confused deputy only fires on synchronize + mockContext.actor = "pelikhan"; + mockContext.eventName = "pull_request"; + mockContext.payload = { action: "labeled", pull_request: { user: { login: "copilot[bot]" } } }; + process.env.GH_AW_REQUIRED_ROLES = "write"; + + mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({ + data: { permission: "write" }, + }); + + await runScript(); + + // Should NOT be denied as confused deputy — should proceed to normal permission check + expect(mockCore.setOutput).not.toHaveBeenCalledWith("result", "confused_deputy"); + expect(mockCore.setOutput).toHaveBeenCalledWith("is_team_member", "true"); + expect(mockCore.setOutput).toHaveBeenCalledWith("result", "authorized"); + }); + + it("should not trigger confused deputy check for safe events (schedule)", async () => { + mockContext.actor = "dependabot[bot]"; + mockContext.eventName = "schedule"; + mockContext.payload = { pull_request: { user: { login: "attacker" } } }; + + await runScript(); + + expect(mockCore.setOutput).toHaveBeenCalledWith("is_team_member", "true"); + expect(mockCore.setOutput).toHaveBeenCalledWith("result", "safe_event"); + }); + + it("should not trigger confused deputy check for issues event (no PR/comment context)", async () => { + mockContext.actor = "dependabot[bot]"; + mockContext.eventName = "issues"; + mockContext.payload = { issue: { user: { login: "someone-else" } } }; + process.env.GH_AW_REQUIRED_ROLES = "write"; + + mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({ + data: { permission: "write" }, + }); + + await runScript(); + + // issues events don't trigger confused deputy detection + expect(mockCore.setOutput).toHaveBeenCalledWith("is_team_member", "true"); + expect(mockCore.setOutput).toHaveBeenCalledWith("result", "authorized"); + }); + + it("should work correctly for workflow_call events with aw_context (no false positive)", async () => { + // In workflow_call, context.payload = { inputs: { aw_context: "..." } } + // The aw_context carries event_type but NOT pull_request.user.login + // Confused deputy check must NOT trigger - this is a legitimate reusable workflow call + mockContext.actor = "dependabot[bot]"; + mockContext.eventName = "workflow_call"; + mockContext.payload = { + inputs: { + aw_context: JSON.stringify({ event_type: "pull_request", item_number: "42", actor: "attacker" }), + }, + }; + process.env.GH_AW_REQUIRED_ROLES = "write"; + + mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({ + data: { permission: "write" }, + }); + + await runScript(); + + // workflow_call proceeds to normal permission check - no confused_deputy denial + expect(mockCore.setOutput).not.toHaveBeenCalledWith("result", "confused_deputy"); + expect(mockCore.setOutput).toHaveBeenCalledWith("is_team_member", "true"); + expect(mockCore.setOutput).toHaveBeenCalledWith("result", "authorized"); + }); + }); + describe("API error handling", () => { beforeEach(() => { process.env.GH_AW_REQUIRED_ROLES = "admin"; diff --git a/actions/setup/js/check_permissions_utils.cjs b/actions/setup/js/check_permissions_utils.cjs index d520543d183..1ab613eba9f 100644 --- a/actions/setup/js/check_permissions_utils.cjs +++ b/actions/setup/js/check_permissions_utils.cjs @@ -46,6 +46,71 @@ function isAllowedBot(actor, allowedBots) { return allowedBots.some(bot => canonicalizeBotIdentifier(bot) === canonicalActor); } +/** + * Detect a potential Dependabot Confused Deputy attack. + * + * Attack vectors defended against: + * - @dependabot recreate on a PR not authored by dependabot causes + * github.actor = 'dependabot[bot]' while pull_request.user.login remains + * the original (human) PR author. + * - @dependabot show on an issue causes github.actor = 'dependabot[bot]' + * while comment.user.login differs from the actor. + * + * Reference: https://labs.boostsecurity.io/articles/weaponizing-dependabot-pwn-request-at-its-finest/ + * + * @param {string} actor - The current github.actor + * @param {string} eventName - The GitHub event name (e.g. "pull_request", "issue_comment") + * @param {object|undefined} payload - The GitHub event payload (context.payload) + * @returns {boolean} true if the event looks like a confused deputy attack + */ +function isConfusedDeputyAttack(actor, eventName, payload) { + if (!payload) return false; + + // For pull_request events, only check on the `synchronize` action. + // The confused deputy attack (@dependabot recreate) triggers a synchronize event + // with actor=dependabot[bot] but pull_request.user = original human author. + // Other pull_request actions (labeled, unlabeled, assigned, review_requested, etc.) + // legitimately have actor != pr_author — the actor is whoever performed the action, + // not the PR author — so checking those would cause false positives. + if (eventName === "pull_request" && payload.action === "synchronize") { + const prAuthor = payload.pull_request?.user?.login; + if (prAuthor !== undefined && prAuthor !== actor) { + return true; + } + } + + // For pull_request_review events, the reviewer must match the actor. + // The PR author (pull_request.user.login) is irrelevant here — the actor + // is the person submitting the review, not the PR author. + if (eventName === "pull_request_review") { + const reviewAuthor = payload.review?.user?.login; + if (reviewAuthor !== undefined && reviewAuthor !== actor) { + return true; + } + } + + // For pull_request_review_comment events, the comment author must match the actor. + // The PR author (pull_request.user.login) is irrelevant here — the actor + // is the person writing the review comment, not the PR author. + if (eventName === "pull_request_review_comment") { + const commentAuthor = payload.comment?.user?.login; + if (commentAuthor !== undefined && commentAuthor !== actor) { + return true; + } + } + + // For issue_comment events, @dependabot show can trigger a comment from dependabot + // with actor=dependabot[bot]. Verify the comment itself was authored by the actor. + if (eventName === "issue_comment") { + const commentAuthor = payload.comment?.user?.login; + if (commentAuthor !== undefined && commentAuthor !== actor) { + return true; + } + } + + return false; +} + /** * Check if the actor is a bot and if it's active on the repository. * Accepts both and [bot] actor forms, since GitHub Apps @@ -155,6 +220,7 @@ module.exports = { parseAllowedBots, canonicalizeBotIdentifier, isAllowedBot, + isConfusedDeputyAttack, checkRepositoryPermission, checkBotStatus, }; diff --git a/actions/setup/js/check_permissions_utils.test.cjs b/actions/setup/js/check_permissions_utils.test.cjs index 98ca23b5b8f..2df8f2acbbb 100644 --- a/actions/setup/js/check_permissions_utils.test.cjs +++ b/actions/setup/js/check_permissions_utils.test.cjs @@ -31,6 +31,7 @@ describe("check_permissions_utils", () => { let parseAllowedBots; let canonicalizeBotIdentifier; let isAllowedBot; + let isConfusedDeputyAttack; let checkRepositoryPermission; let checkBotStatus; let originalEnv; @@ -53,6 +54,7 @@ describe("check_permissions_utils", () => { isAllowedBot = module.isAllowedBot; checkRepositoryPermission = module.checkRepositoryPermission; checkBotStatus = module.checkBotStatus; + isConfusedDeputyAttack = module.isConfusedDeputyAttack; }); afterEach(() => { @@ -530,4 +532,167 @@ describe("check_permissions_utils", () => { expect(mockCore.warning).toHaveBeenCalledWith("Failed to check bot status: API rate limit exceeded"); }); }); + + describe("isConfusedDeputyAttack", () => { + describe("pull_request events", () => { + it("should return false when actor matches PR author on synchronize (genuine dependabot PR)", () => { + const payload = { action: "synchronize", pull_request: { user: { login: "dependabot[bot]" } } }; + expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request", payload)).toBe(false); + }); + + it("should return true when actor differs from PR author on synchronize (confused deputy via @dependabot recreate)", () => { + const payload = { action: "synchronize", pull_request: { user: { login: "attacker" } } }; + expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request", payload)).toBe(true); + }); + + it("should return false when actor matches PR author for a human PR on synchronize", () => { + const payload = { action: "synchronize", pull_request: { user: { login: "octocat" } } }; + expect(isConfusedDeputyAttack("octocat", "pull_request", payload)).toBe(false); + }); + + it("should return false when PR author is absent from payload on synchronize", () => { + const payload = { action: "synchronize", pull_request: {} }; + expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request", payload)).toBe(false); + }); + + it("should return false when pull_request is absent from payload on synchronize", () => { + const payload = { action: "synchronize" }; + expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request", payload)).toBe(false); + }); + + it("should return false for pull_request:labeled even if actor differs from PR author", () => { + // A team member can label a PR authored by someone else — NOT a confused deputy attack + const payload = { action: "labeled", pull_request: { user: { login: "pr-author" } } }; + expect(isConfusedDeputyAttack("pelikhan", "pull_request", payload)).toBe(false); + }); + + it("should return false for pull_request:opened even if actor differs from PR author", () => { + // opened is not the synchronize attack vector — skip the check + const payload = { action: "opened", pull_request: { user: { login: "attacker" } } }; + expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request", payload)).toBe(false); + }); + + it("should return false for pull_request:unlabeled even if actor differs from PR author", () => { + const payload = { action: "unlabeled", pull_request: { user: { login: "pr-author" } } }; + expect(isConfusedDeputyAttack("pelikhan", "pull_request", payload)).toBe(false); + }); + + it("should return false for pull_request:review_requested even if actor differs from PR author", () => { + const payload = { action: "review_requested", pull_request: { user: { login: "pr-author" } } }; + expect(isConfusedDeputyAttack("pelikhan", "pull_request", payload)).toBe(false); + }); + + it("should return false for pull_request_review when actor matches review author (genuine review)", () => { + const payload = { + pull_request: { user: { login: "pr-author" } }, + review: { user: { login: "dependabot[bot]" } }, + }; + expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request_review", payload)).toBe(false); + }); + + it("should return true for pull_request_review when actor differs from review author", () => { + const payload = { + pull_request: { user: { login: "pr-author" } }, + review: { user: { login: "attacker" } }, + }; + expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request_review", payload)).toBe(true); + }); + + it("should return false for pull_request_review when actor is reviewer even if PR author differs", () => { + // Normal case: reviewer is different from PR author — must NOT be false positive + const payload = { + pull_request: { user: { login: "pr-author" } }, + review: { user: { login: "dependabot[bot]" } }, + }; + expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request_review", payload)).toBe(false); + }); + + it("should return false for pull_request_review_comment when actor matches comment author (genuine comment)", () => { + const payload = { + pull_request: { user: { login: "pr-author" } }, + comment: { user: { login: "dependabot[bot]" } }, + }; + expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request_review_comment", payload)).toBe(false); + }); + + it("should return true for pull_request_review_comment when actor differs from comment author", () => { + const payload = { + pull_request: { user: { login: "pr-author" } }, + comment: { user: { login: "attacker" } }, + }; + expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request_review_comment", payload)).toBe(true); + }); + + it("should return false for pull_request_review_comment when actor is commenter even if PR author differs", () => { + // Normal case: commenter is different from PR author — must NOT be false positive + const payload = { + pull_request: { user: { login: "pr-author" } }, + comment: { user: { login: "dependabot[bot]" } }, + }; + expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request_review_comment", payload)).toBe(false); + }); + }); + + describe("issue_comment events", () => { + it("should return false when actor matches comment author (genuine bot comment)", () => { + const payload = { comment: { user: { login: "dependabot[bot]" } } }; + expect(isConfusedDeputyAttack("dependabot[bot]", "issue_comment", payload)).toBe(false); + }); + + it("should return true when actor differs from comment author", () => { + const payload = { comment: { user: { login: "attacker" } } }; + expect(isConfusedDeputyAttack("dependabot[bot]", "issue_comment", payload)).toBe(true); + }); + + it("should return false when comment author is absent from payload", () => { + const payload = { comment: {} }; + expect(isConfusedDeputyAttack("dependabot[bot]", "issue_comment", payload)).toBe(false); + }); + + it("should return false when comment is absent from payload", () => { + const payload = {}; + expect(isConfusedDeputyAttack("dependabot[bot]", "issue_comment", payload)).toBe(false); + }); + }); + + describe("other event types", () => { + it("should return false for push events (no PR/comment context)", () => { + const payload = { sender: { login: "dependabot[bot]" } }; + expect(isConfusedDeputyAttack("dependabot[bot]", "push", payload)).toBe(false); + }); + + it("should return false for issues events", () => { + const payload = { issue: { user: { login: "attacker" } } }; + expect(isConfusedDeputyAttack("dependabot[bot]", "issues", payload)).toBe(false); + }); + + it("should return false for schedule events", () => { + expect(isConfusedDeputyAttack("github-actions[bot]", "schedule", {})).toBe(false); + }); + + it("should return false for workflow_call events (no PR/comment in payload)", () => { + // In workflow_call, context.payload = { inputs: { aw_context: "..." } } + // aw_context carries event_type/item_number but NOT pull_request.user.login + const payload = { inputs: { aw_context: '{"event_type":"pull_request","item_number":"42","actor":"attacker"}' } }; + expect(isConfusedDeputyAttack("dependabot[bot]", "workflow_call", payload)).toBe(false); + }); + + it("should return false for workflow_call even if payload contains unrelated pull_request data", () => { + // Even if someone injects pull_request data in the payload, the eventName check + // guards against false positives: workflow_call is never in prEvents + const payload = { pull_request: { user: { login: "attacker" } }, inputs: {} }; + expect(isConfusedDeputyAttack("dependabot[bot]", "workflow_call", payload)).toBe(false); + }); + }); + + describe("edge cases", () => { + it("should return false when payload is null", () => { + expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request", null)).toBe(false); + }); + + it("should return false when payload is undefined", () => { + expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request", undefined)).toBe(false); + }); + }); + }); }); diff --git a/actions/setup/js/check_skip_bots.cjs b/actions/setup/js/check_skip_bots.cjs index 78c53294c59..a7fbee760b6 100644 --- a/actions/setup/js/check_skip_bots.cjs +++ b/actions/setup/js/check_skip_bots.cjs @@ -1,6 +1,7 @@ // @ts-check /// +const { isConfusedDeputyAttack } = require("./check_permissions_utils.cjs"); const { writeDenialSummary } = require("./pre_activation_summary.cjs"); /** @@ -29,6 +30,19 @@ async function main() { .filter(u => u); core.info(`Checking if user '${actor}' is in skip-bots: ${skipBots.join(", ")}`); + // Guard against Dependabot Confused Deputy attacks. + // An attacker can trigger @dependabot recreate (for pull_request events) or + // @dependabot show (for issue_comment events) to make dependabot appear as the + // actor, causing a legitimate bot's skip-bots rule to suppress the workflow for + // the attacker's PR/issue. + // Reference: https://labs.boostsecurity.io/articles/weaponizing-dependabot-pwn-request-at-its-finest/ + if (isConfusedDeputyAttack(actor, eventName, context.payload)) { + core.info(`Potential confused deputy attack detected: actor '${actor}' does not match the event author. Skipping skip-bots check.`); + core.setOutput("skip_bots_ok", "true"); + core.setOutput("result", "not_skipped"); + return; + } + // Check if the actor is in the skip-bots list // Match both exact username and username with [bot] suffix // e.g., "github-actions" matches both "github-actions" and "github-actions[bot]" diff --git a/actions/setup/js/check_skip_bots.test.cjs b/actions/setup/js/check_skip_bots.test.cjs index 151392f5bd3..ab5542ccc2d 100644 --- a/actions/setup/js/check_skip_bots.test.cjs +++ b/actions/setup/js/check_skip_bots.test.cjs @@ -141,4 +141,96 @@ describe("check_skip_bots.cjs", () => { expect(mockCore.setOutput).toHaveBeenCalledWith("skip_bots_ok", "false"); expect(mockCore.setOutput).toHaveBeenCalledWith("result", "skipped"); }); + + describe("confused deputy attack protection", () => { + it("should not skip workflow when actor differs from PR author (pull_request synchronize event)", async () => { + process.env.GH_AW_SKIP_BOTS = "dependabot[bot]"; + mockContext.actor = "dependabot[bot]"; + mockContext.eventName = "pull_request"; + mockContext.payload = { action: "synchronize", pull_request: { user: { login: "attacker" } } }; + + const { main } = await import("./check_skip_bots.cjs"); + await main(); + + // Even though dependabot[bot] is in skip-bots, confused deputy prevents skipping + expect(mockCore.setOutput).toHaveBeenCalledWith("skip_bots_ok", "true"); + expect(mockCore.setOutput).toHaveBeenCalledWith("result", "not_skipped"); + expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Potential confused deputy attack detected")); + }); + + it("should skip workflow when actor matches PR author (genuine dependabot PR synchronize)", async () => { + process.env.GH_AW_SKIP_BOTS = "dependabot[bot]"; + mockContext.actor = "dependabot[bot]"; + mockContext.eventName = "pull_request"; + mockContext.payload = { action: "synchronize", pull_request: { user: { login: "dependabot[bot]" } } }; + + const { main } = await import("./check_skip_bots.cjs"); + await main(); + + expect(mockCore.setOutput).toHaveBeenCalledWith("skip_bots_ok", "false"); + expect(mockCore.setOutput).toHaveBeenCalledWith("result", "skipped"); + }); + + it("should not skip workflow when actor differs from comment author (issue_comment event)", async () => { + process.env.GH_AW_SKIP_BOTS = "dependabot[bot]"; + mockContext.actor = "dependabot[bot]"; + mockContext.eventName = "issue_comment"; + mockContext.payload = { comment: { user: { login: "attacker" } } }; + + const { main } = await import("./check_skip_bots.cjs"); + await main(); + + expect(mockCore.setOutput).toHaveBeenCalledWith("skip_bots_ok", "true"); + expect(mockCore.setOutput).toHaveBeenCalledWith("result", "not_skipped"); + }); + + it("should skip workflow for pull_request:labeled even when actor differs from PR author", async () => { + // A team member labeling a PR is legitimate — confused deputy only fires on synchronize + process.env.GH_AW_SKIP_BOTS = "pelikhan"; + mockContext.actor = "pelikhan"; + mockContext.eventName = "pull_request"; + mockContext.payload = { action: "labeled", pull_request: { user: { login: "copilot[bot]" } } }; + + const { main } = await import("./check_skip_bots.cjs"); + await main(); + + // Not a confused deputy — apply skip-bots normally + expect(mockCore.setOutput).toHaveBeenCalledWith("skip_bots_ok", "false"); + expect(mockCore.setOutput).toHaveBeenCalledWith("result", "skipped"); + }); + + it("should skip workflow normally when no payload (non-PR events)", async () => { + process.env.GH_AW_SKIP_BOTS = "dependabot[bot]"; + mockContext.actor = "dependabot[bot]"; + mockContext.eventName = "issues"; + // No payload field (existing test behavior) + + const { main } = await import("./check_skip_bots.cjs"); + await main(); + + expect(mockCore.setOutput).toHaveBeenCalledWith("skip_bots_ok", "false"); + expect(mockCore.setOutput).toHaveBeenCalledWith("result", "skipped"); + }); + + it("should apply skip-bots normally for workflow_call events (no false confused deputy positive)", async () => { + // In workflow_call, context.payload = { inputs: { aw_context: "..." } } + // aw_context carries event_type but NOT pull_request.user.login + // Confused deputy check must NOT trigger - apply skip-bots rule normally + process.env.GH_AW_SKIP_BOTS = "dependabot[bot]"; + mockContext.actor = "dependabot[bot]"; + mockContext.eventName = "workflow_call"; + mockContext.payload = { + inputs: { + aw_context: JSON.stringify({ event_type: "pull_request", item_number: "42", actor: "attacker" }), + }, + }; + + const { main } = await import("./check_skip_bots.cjs"); + await main(); + + // workflow_call: no confused deputy - normal skip-bots logic applies + expect(mockCore.setOutput).toHaveBeenCalledWith("skip_bots_ok", "false"); + expect(mockCore.setOutput).toHaveBeenCalledWith("result", "skipped"); + }); + }); }); diff --git a/docs/adr/29450-guard-bot-filter-against-dependabot-confused-deputy-injection.md b/docs/adr/29450-guard-bot-filter-against-dependabot-confused-deputy-injection.md new file mode 100644 index 00000000000..0fc1896bb89 --- /dev/null +++ b/docs/adr/29450-guard-bot-filter-against-dependabot-confused-deputy-injection.md @@ -0,0 +1,94 @@ +# ADR-29450: Guard Bot-Filter System Against Dependabot Confused Deputy Injection + +**Date**: 2026-05-01 +**Status**: Draft +**Deciders**: pelikhan, Copilot + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +Every compiled gh-aw workflow runs a `pre_activation` job that executes two JavaScript checks before the agent starts: `check_membership.cjs`, which verifies that `github.actor` holds the required repository permission (or appears in the bot allowlist), and `check_skip_bots.cjs`, which suppresses the workflow when `github.actor` is in the workflow's `skip-bots` list. Both scripts treated `github.actor` — the account that *triggered* the run — as the sole identity signal, without verifying that the triggering account is also the account that *authored* the underlying pull request or comment. GitHub Actions sets `github.actor` to whichever account caused the most-recent workflow trigger, which can differ from the PR author when a third party (such as Dependabot) reacts to a comment on a fork PR. This discrepancy is exploitable via the [Dependabot Confused Deputy Injection](https://labs.boostsecurity.io/articles/weaponizing-dependabot-pwn-request-at-its-finest/) technique, where an attacker manipulates Dependabot into re-triggering a run with `actor = dependabot[bot]` so that the actor's elevated permissions — not the attacker's — are used for the permission check, causing the agent to execute against the attacker's untrusted code. + +### Decision + +We will introduce a `isConfusedDeputyAttack(actor, eventName, payload)` pure helper in `check_permissions_utils.cjs` that detects when `github.actor` does not match the event-specific author field in the webhook payload. For `pull_request` events, the check is intentionally scoped to the `synchronize` action only — the documented attack vector — because other `pull_request` actions such as `labeled`, `unlabeled`, `assigned`, and `review_requested` legitimately have the actor differ from the PR author (the actor is whoever performed that action, not the person who opened the PR). For `pull_request_review` events, the check compares against `payload.review.user.login` (the reviewer). For `pull_request_review_comment` and `issue_comment` events, the check compares against `payload.comment.user.login` (the comment author). For all other event types the helper returns `false`. `check_membership.cjs` will call this helper immediately before the `checkRepositoryPermission` call and deny any run where the check returns `true`. `check_skip_bots.cjs` will call the same helper after the "no skip-bots" early return and, if a confused deputy is detected, will *not* suppress the workflow — allowing the run to proceed to `check_membership.cjs` where it will be denied explicitly rather than silently skipped. In addition, the compiled condition for the `"dependabot pull request"` trigger shorthand in `trigger_parser.go` will be tightened from `github.actor == 'dependabot[bot]'` to `github.actor == 'dependabot[bot]' && github.event.pull_request.user.login == 'dependabot[bot]'`, ensuring the shorthand only activates when the PR was genuinely authored by Dependabot. + +### Alternatives Considered + +#### Alternative 1: Allowlist-Only Defence (Opt-In per Repository) + +Restrict `dependabot[bot]` to the bot allowlist only for repositories that explicitly opt in, requiring a configuration change for any repo that wants Dependabot PRs handled by the agent. This was rejected because it places the security burden on individual repository owners, does not address the `skip-bots` bypass variant (where the attacker impersonates Dependabot to suppress the workflow for their own PR), and still leaves unenrolled repositories vulnerable. It also provides no protection for `issue_comment` or `pull_request_review` confused-deputy variants. + +#### Alternative 2: Use `GITHUB_TRIGGERING_ACTOR` Instead of `github.actor` + +Replace all references to `github.actor` with the `GITHUB_TRIGGERING_ACTOR` environment variable, which GitHub sets to the account that initiated a re-run. This was rejected because `GITHUB_TRIGGERING_ACTOR` is only populated during re-run scenarios; it is absent for the original confused-deputy trigger (the first run that Dependabot initiates in response to a comment), making it an incomplete defence. + +#### Alternative 3: Deny All `[bot]`-Suffixed Actors on PR Events + +Reject any actor whose login ends in `[bot]` for `pull_request` and related events. This was rejected because legitimate bot-authored PRs — from Renovate, automated release tooling, or custom GitHub Apps — should remain functional without per-repository exemption lists, and the blanket denial would break those workflows. + +#### Alternative 4: Compile-Time Fix Only (`trigger_parser.go`) + +Tighten only the `"dependabot pull request"` compiled condition and make no runtime changes to `check_membership.cjs` or `check_skip_bots.cjs`. This was rejected because the compile-time condition protects only that specific trigger shorthand; it leaves `check_membership.cjs` vulnerable whenever the workflow's activation condition passes through a path other than the `"dependabot pull request"` shorthand, and it provides no coverage for `issue_comment` or `pull_request_review` events. + +### Consequences + +#### Positive +- Closes the confused deputy attack vector for `pull_request`, `issue_comment`, `pull_request_review`, and `pull_request_review_comment` events without any per-repository configuration change. +- The `"dependabot pull request"` trigger shorthand is made safe at compile time independently of the runtime check, providing defence-in-depth. +- `workflow_call`, `push`, `schedule`, `workflow_dispatch`, and `merge_group` events are entirely unaffected: `isConfusedDeputyAttack` returns `false` for all of them, producing no false positives. +- The confused deputy result is surfaced as an explicit `core.warning` and written to the job summary, making attacks visible in workflow logs rather than silently ignored. + +#### Negative +- The `pull_request:synchronize` guard assumes that whenever a bot is the actor on a sync event, the bot should also be the PR author. Legitimate scenarios where a bot force-pushes to a PR it did not open (e.g., an automated rebase bot) would be denied. Such bots should open their own PRs rather than rebasing others' PRs; if they do not, they would need to be exempted via the bot allowlist. +- `check_permissions_utils.cjs` gains a new export (`isConfusedDeputyAttack`) and both `check_membership.cjs` and `check_skip_bots.cjs` gain a new import and call-site, modestly increasing the size and coupling of the pre-activation check surface. + +#### Neutral +- The guard runs exclusively in the `pre_activation` job; the agent execution job is structurally unchanged. +- The `isConfusedDeputyAttack` function performs no network I/O and adds negligible latency to the pre-activation job. +- Existing compiled workflows do not need to be recompiled; the protection takes effect as soon as the updated `actions/setup/js/` helpers are deployed, because the JavaScript files are copied to `/tmp/gh-aw/actions` at runtime rather than embedded in compiled lock files. + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### `isConfusedDeputyAttack` Helper + +1. The `check_permissions_utils.cjs` module **MUST** export a pure function named `isConfusedDeputyAttack(actor, eventName, payload)` that returns a boolean. +2. For `pull_request` events, the function **MUST** return `true` if and only if `payload.action` strictly equals `"synchronize"` **and** `actor` does not strictly equal `payload.pull_request.user.login`. For all other `pull_request` actions (e.g., `labeled`, `unlabeled`, `opened`, `assigned`, `review_requested`), the function **MUST** return `false`, because for those actions the actor is legitimately the person who performed the action rather than the PR author. +3. For `pull_request_review` events, the function **MUST** return `true` if and only if `actor` does not strictly equal `payload.review.user.login`. +4. For `pull_request_review_comment` events, the function **MUST** return `true` if and only if `actor` does not strictly equal `payload.comment.user.login`. +5. For `issue_comment` events, the function **MUST** return `true` if and only if `actor` does not strictly equal `payload.comment.user.login`. +6. For all other event types (`push`, `schedule`, `workflow_dispatch`, `workflow_call`, `merge_group`, etc.), the function **MUST** return `false`. +7. If the event-specific author field is absent or `undefined` in the payload for an otherwise-covered event type, the function **MUST** return `false` to avoid false positives. +8. The function **MUST NOT** perform any network I/O, file I/O, or external side effects. + +### Membership Check (`check_membership.cjs`) + +1. After the safe-events bypass and before any call to `checkRepositoryPermission`, `check_membership.cjs` **MUST** call `isConfusedDeputyAttack` with the current `github.actor`, `github.event_name`, and `github.event` payload. +2. If `isConfusedDeputyAttack` returns `true`, the membership check **MUST** set `result` to `confused_deputy`, set `is_team_member` to `false`, emit a `core.warning` identifying the actor mismatch, write a denial summary to the job summary, and **MUST NOT** proceed to `checkRepositoryPermission`. +3. A run denied with `result = confused_deputy` **MUST** be treated identically to a `no_permission` denial by all downstream consumers of the `result` output. + +### Skip-Bots Check (`check_skip_bots.cjs`) + +1. After the "no skip-bots configured" early return and before the skip-bots matching loop, `check_skip_bots.cjs` **MUST** call `isConfusedDeputyAttack` with the current actor, event name, and payload. +2. If `isConfusedDeputyAttack` returns `true`, `check_skip_bots.cjs` **MUST** set `skip_bots_ok` to `true`, set `result` to `not_skipped`, and return immediately — **MUST NOT** suppress the workflow — so that the run proceeds to `check_membership.cjs` where it will be denied with an explicit result. +3. `check_skip_bots.cjs` **MUST NOT** suppress the workflow solely because `github.actor` appears in the `skip-bots` list when the confused deputy check has fired; suppressing at this stage **MUST NOT** be used as a substitute for the explicit denial in `check_membership.cjs`. + +### Dependabot Pull Request Trigger Shorthand + +1. The compiled condition for the `"dependabot pull request"` trigger shorthand **MUST** be `github.actor == 'dependabot[bot]' && github.event.pull_request.user.login == 'dependabot[bot]'`. +2. Implementations **MUST NOT** emit a condition that checks only `github.actor == 'dependabot[bot]'` without the corresponding `pull_request.user.login` guard for this shorthand. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. + +--- + +*This is a DRAFT ADR generated by the [adr-writer agent]. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/pkg/workflow/trigger_parser.go b/pkg/workflow/trigger_parser.go index da3ea5d8fb6..69815e9f08b 100644 --- a/pkg/workflow/trigger_parser.go +++ b/pkg/workflow/trigger_parser.go @@ -559,11 +559,14 @@ func parseSecurityTrigger(input string) (*TriggerIR, error) { } if tokens[0] == "dependabot" && len(tokens) >= 3 && tokens[1] == "pull" && tokens[2] == "request" { - // "dependabot pull request" - filter pull requests by Dependabot author + // "dependabot pull request" - filter pull requests by Dependabot author. + // Guard against the Dependabot Confused Deputy attack (@dependabot recreate) by + // requiring the PR author to also be dependabot[bot], not just the current actor. + // Reference: https://labs.boostsecurity.io/articles/weaponizing-dependabot-pwn-request-at-its-finest/ return &TriggerIR{ Event: "pull_request", Types: []string{"opened", "synchronize", "reopened"}, - Conditions: []string{"github.actor == 'dependabot[bot]'"}, + Conditions: []string{"github.actor == 'dependabot[bot]' && github.event.pull_request.user.login == 'dependabot[bot]'"}, AdditionalEvents: map[string]any{ "workflow_dispatch": nil, }, diff --git a/pkg/workflow/trigger_parser_test.go b/pkg/workflow/trigger_parser_test.go index 685d67ede8c..5c9098761a1 100644 --- a/pkg/workflow/trigger_parser_test.go +++ b/pkg/workflow/trigger_parser_test.go @@ -194,7 +194,7 @@ func TestParseTriggerShorthand(t *testing.T) { input: "dependabot pull request", wantEvent: "pull_request", wantTypes: []string{"opened", "synchronize", "reopened"}, - wantConds: []string{"github.actor == 'dependabot[bot]'"}, + wantConds: []string{"github.actor == 'dependabot[bot]' && github.event.pull_request.user.login == 'dependabot[bot]'"}, }, { name: "security alert",