Repository navigation
security: guard bot filtering against dependabot confused deputy attacks #29432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
227d940
94dd4d3
7df1e43
fadfcf5
108cfce
4479d66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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,105 @@ 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 (genuine dependabot PR)", () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const payload = { 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 (confused deputy via @dependabot recreate)", () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const payload = { 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", () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const payload = { pull_request: { user: { login: "octocat" } } }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(isConfusedDeputyAttack("octocat", "pull_request", payload)).toBe(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it("should return false when PR author is absent from payload", () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const payload = { pull_request: {} }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request", payload)).toBe(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it("should return false when pull_request is absent from payload", () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const payload = {}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request", payload)).toBe(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it("should return true for pull_request_review when actor differs from PR author", () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const payload = { pull_request: { user: { login: "attacker" } } }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request_review", payload)).toBe(true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it("should return true for pull_request_review_comment when actor differs from PR author", () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const payload = { pull_request: { user: { login: "attacker" } } }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| it("should return true for pull_request_review when actor differs from PR author", () => { | |
| const payload = { pull_request: { user: { login: "attacker" } } }; | |
| expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request_review", payload)).toBe(true); | |
| }); | |
| it("should return true for pull_request_review_comment when actor differs from PR author", () => { | |
| const payload = { pull_request: { user: { login: "attacker" } } }; | |
| it("should return false for pull_request_review when actor matches review author even if PR author differs", () => { | |
| const payload = { | |
| pull_request: { user: { login: "attacker" } }, | |
| 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 even if PR author matches", () => { | |
| const payload = { | |
| pull_request: { user: { login: "dependabot[bot]" } }, | |
| review: { user: { login: "attacker" } }, | |
| }; | |
| expect(isConfusedDeputyAttack("dependabot[bot]", "pull_request_review", payload)).toBe(true); | |
| }); | |
| it("should return false for pull_request_review_comment when actor matches comment author even if PR author differs", () => { | |
| const payload = { | |
| pull_request: { user: { login: "attacker" } }, | |
| 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 even if PR author matches", () => { | |
| const payload = { | |
| pull_request: { user: { login: "dependabot[bot]" } }, | |
| comment: { user: { login: "attacker" } }, | |
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion! Using event-specific author fields (payload.review.user.login for pull_request_review) instead of always comparing against the PR author is the correct approach to avoid false positives on normal review workflows.
📰 BREAKING: Report filed by Smoke Copilot · ● 1.3M
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isConfusedDeputyAttacktreatspull_request_reviewandpull_request_review_commentas if the PR author must matchgithub.actor, but for these events the actor is typically the reviewer/commenter and will not matchpayload.pull_request.user.loginin normal operation. This causes false positives that will denycheck_membershipand bypasscheck_skip_botsfor essentially all review/review-comment triggers. Use the event-specific author fields instead (e.g.,payload.review.user.loginforpull_request_reviewandpayload.comment.user.loginforpull_request_review_comment), and only fall back topayload.pull_request.user.loginforpull_requestevents.