-
Notifications
You must be signed in to change notification settings - Fork 295
fix: canonicalize bot identifiers so <slug> and <slug>[bot] match in on.bots
#20059
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 all commits
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 |
|---|---|---|
|
|
@@ -25,30 +25,53 @@ function parseAllowedBots() { | |
| } | ||
|
|
||
| /** | ||
| * Check if the actor is a bot and if it's active on the repository | ||
| * Canonicalize a bot/App identifier by stripping the [bot] suffix. | ||
| * Both "my-app" and "my-app[bot]" normalize to "my-app". | ||
| * @param {string} name - Bot identifier (with or without [bot] suffix) | ||
| * @returns {string} The base slug without [bot] suffix | ||
| */ | ||
| function canonicalizeBotIdentifier(name) { | ||
| return name.endsWith("[bot]") ? name.slice(0, -5) : name; | ||
| } | ||
|
|
||
| /** | ||
| * Check if an actor matches any entry in the allowed bots list, | ||
| * treating <slug> and <slug>[bot] as equivalent App identities. | ||
| * @param {string} actor - The runtime actor name | ||
| * @param {string[]} allowedBots - Array of allowed bot identifiers | ||
| * @returns {boolean} | ||
| */ | ||
| function isAllowedBot(actor, allowedBots) { | ||
| const canonicalActor = canonicalizeBotIdentifier(actor); | ||
| return allowedBots.some(bot => canonicalizeBotIdentifier(bot) === canonicalActor); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| /** | ||
| * Check if the actor is a bot and if it's active on the repository. | ||
| * Accepts both <slug> and <slug>[bot] actor forms, since GitHub Apps | ||
| * may appear either way depending on the event context. | ||
| * @param {string} actor - GitHub username to check | ||
| * @param {string} owner - Repository owner | ||
| * @param {string} repo - Repository name | ||
| * @returns {Promise<{isBot: boolean, isActive: boolean, error?: string}>} | ||
| */ | ||
| async function checkBotStatus(actor, owner, repo) { | ||
| try { | ||
| // Check if the actor looks like a bot (ends with [bot]) | ||
| const isBot = actor.endsWith("[bot]"); | ||
|
|
||
| if (!isBot) { | ||
| return { isBot: false, isActive: false }; | ||
| } | ||
| // GitHub Apps can appear as either <slug> or <slug>[bot]. | ||
| // Treat both forms as a bot identity; always query the API with the [bot] form. | ||
| const actorHasBotSuffix = actor.endsWith("[bot]"); | ||
| const actorForApi = actorHasBotSuffix ? actor : `${actor}[bot]`; | ||
|
|
||
| core.info(`Checking if bot '${actor}' is active on ${owner}/${repo}`); | ||
|
|
||
| // Try to get the bot's permission level to verify it's installed/active on the repo | ||
| // GitHub Apps/bots that are installed on a repository show up in the collaborators | ||
| // Try to get the bot's permission level to verify it's installed/active on the repo. | ||
| // GitHub Apps/bots that are installed on a repository show up in the collaborators. | ||
| // Use the [bot]-suffixed form since that is how GitHub App identities are listed. | ||
| try { | ||
| const botPermission = await github.rest.repos.getCollaboratorPermissionLevel({ | ||
| owner, | ||
| repo, | ||
| username: actor, | ||
| username: actorForApi, | ||
| }); | ||
|
|
||
| core.info(`Bot '${actor}' is active with permission level: ${botPermission.data.permission}`); | ||
|
|
@@ -114,6 +137,8 @@ async function checkRepositoryPermission(actor, owner, repo, requiredPermissions | |
| module.exports = { | ||
| parseRequiredPermissions, | ||
| parseAllowedBots, | ||
| canonicalizeBotIdentifier, | ||
| isAllowedBot, | ||
| checkRepositoryPermission, | ||
| checkBotStatus, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,8 @@ global.github = mockGithub; | |
| describe("check_permissions_utils", () => { | ||
| let parseRequiredPermissions; | ||
| let parseAllowedBots; | ||
| let canonicalizeBotIdentifier; | ||
| let isAllowedBot; | ||
| let checkRepositoryPermission; | ||
| let checkBotStatus; | ||
| let originalEnv; | ||
|
|
@@ -47,6 +49,8 @@ describe("check_permissions_utils", () => { | |
| const module = await import("./check_permissions_utils.cjs"); | ||
| parseRequiredPermissions = module.parseRequiredPermissions; | ||
| parseAllowedBots = module.parseAllowedBots; | ||
| canonicalizeBotIdentifier = module.canonicalizeBotIdentifier; | ||
| isAllowedBot = module.isAllowedBot; | ||
| checkRepositoryPermission = module.checkRepositoryPermission; | ||
| checkBotStatus = module.checkBotStatus; | ||
| }); | ||
|
|
@@ -100,6 +104,54 @@ describe("check_permissions_utils", () => { | |
| }); | ||
| }); | ||
|
Comment on lines
104
to
105
|
||
|
|
||
| describe("canonicalizeBotIdentifier", () => { | ||
| it("should strip [bot] suffix", () => { | ||
| expect(canonicalizeBotIdentifier("dependabot[bot]")).toBe("dependabot"); | ||
| }); | ||
|
|
||
| it("should return name unchanged when no [bot] suffix", () => { | ||
| expect(canonicalizeBotIdentifier("my-pipeline-app")).toBe("my-pipeline-app"); | ||
| }); | ||
|
|
||
| it("should handle names with [bot] suffix only once", () => { | ||
| expect(canonicalizeBotIdentifier("github-actions[bot]")).toBe("github-actions"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("isAllowedBot", () => { | ||
| it("should match exact slug to slug", () => { | ||
| expect(isAllowedBot("my-app", ["my-app"])).toBe(true); | ||
| }); | ||
|
|
||
| it("should match slug to slug[bot]", () => { | ||
| expect(isAllowedBot("my-app[bot]", ["my-app"])).toBe(true); | ||
| }); | ||
|
|
||
| it("should match slug[bot] to slug", () => { | ||
| expect(isAllowedBot("my-app", ["my-app[bot]"])).toBe(true); | ||
| }); | ||
|
|
||
| it("should match slug[bot] to slug[bot]", () => { | ||
| expect(isAllowedBot("my-app[bot]", ["my-app[bot]"])).toBe(true); | ||
| }); | ||
|
|
||
| it("should return false when actor is not in the list", () => { | ||
| expect(isAllowedBot("other-app", ["my-app"])).toBe(false); | ||
| }); | ||
|
|
||
| it("should return false for empty allowed bots list", () => { | ||
| expect(isAllowedBot("my-app", [])).toBe(false); | ||
| }); | ||
|
|
||
| it("should match against any entry in the list", () => { | ||
| expect(isAllowedBot("renovate[bot]", ["dependabot[bot]", "renovate", "github-actions[bot]"])).toBe(true); | ||
| }); | ||
|
|
||
| it("should not match partial slug names", () => { | ||
| expect(isAllowedBot("my-app-extra[bot]", ["my-app"])).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe("parseRequiredPermissions", () => { | ||
| it("should parse comma-separated permissions", () => { | ||
| process.env.GH_AW_REQUIRED_ROLES = "admin,write,read"; | ||
|
|
@@ -287,15 +339,48 @@ describe("check_permissions_utils", () => { | |
| expect(mockCore.info).toHaveBeenCalledWith("Bot 'dependabot[bot]' is active with permission level: write"); | ||
| }); | ||
|
|
||
| it("should return false for non-bot users", async () => { | ||
| const result = await checkBotStatus("regularuser", "testowner", "testrepo"); | ||
| it("should identify active bot by slug without [bot] suffix", async () => { | ||
| mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({ | ||
| data: { permission: "write" }, | ||
| }); | ||
|
|
||
| const result = await checkBotStatus("my-pipeline-app", "testowner", "testrepo"); | ||
|
|
||
| expect(result).toEqual({ | ||
| isBot: true, | ||
| isActive: true, | ||
| }); | ||
|
|
||
| // API should be called with the [bot]-suffixed form | ||
| expect(mockGithub.rest.repos.getCollaboratorPermissionLevel).toHaveBeenCalledWith({ | ||
| owner: "testowner", | ||
| repo: "testrepo", | ||
| username: "my-pipeline-app[bot]", | ||
| }); | ||
|
|
||
| expect(mockCore.info).toHaveBeenCalledWith("Checking if bot 'my-pipeline-app' is active on testowner/testrepo"); | ||
| expect(mockCore.info).toHaveBeenCalledWith("Bot 'my-pipeline-app' is active with permission level: write"); | ||
| }); | ||
|
|
||
| it("should return inactive bot when slug without [bot] suffix is not installed", async () => { | ||
| const apiError = { status: 404, message: "Not Found" }; | ||
| mockGithub.rest.repos.getCollaboratorPermissionLevel.mockRejectedValue(apiError); | ||
|
|
||
| const result = await checkBotStatus("my-pipeline-app", "testowner", "testrepo"); | ||
|
|
||
| expect(result).toEqual({ | ||
| isBot: false, | ||
| isBot: true, | ||
| isActive: false, | ||
| }); | ||
|
|
||
| expect(mockGithub.rest.repos.getCollaboratorPermissionLevel).not.toHaveBeenCalled(); | ||
| // API should still be called with the [bot]-suffixed form | ||
| expect(mockGithub.rest.repos.getCollaboratorPermissionLevel).toHaveBeenCalledWith({ | ||
| owner: "testowner", | ||
| repo: "testrepo", | ||
| username: "my-pipeline-app[bot]", | ||
| }); | ||
|
|
||
| expect(mockCore.warning).toHaveBeenCalledWith("Bot 'my-pipeline-app' is not active/installed on testowner/testrepo"); | ||
| }); | ||
|
|
||
| it("should handle 404 error for inactive bot", async () => { | ||
|
|
@@ -357,7 +442,7 @@ describe("check_permissions_utils", () => { | |
| }); | ||
| }); | ||
|
|
||
| it("should verify bot is installed on repository", async () => { | ||
| it("should verify bot is installed on repository using [bot] form", async () => { | ||
| mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({ | ||
| data: { permission: "admin" }, | ||
| }); | ||
|
|
||
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.
Good implementation of
canonicalizeBotIdentifier— stripping the[bot]suffix cleanly handles both actor forms. Consider adding a JSDoc@exampletag to make usage even clearer.