Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/design-decision-gate.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions .github/workflows/mattpocock-skills-reviewer.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/workflows/mcp-inspector.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions .github/workflows/pr-code-quality-reviewer.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/workflows/smoke-otel-backends.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions .github/workflows/test-quality-sentinel.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions actions/setup/js/check_admin_permissions.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ describe("check admin permissions for copilot maintenance", () => {
expect(result).toEqual({
authorized: false,
error: "API Error: Not Found",
isGitHubApp: false,
});

expect(mockCore.warning).toHaveBeenCalledWith("Repository permission check failed: API Error: Not Found");
Expand All @@ -147,6 +148,7 @@ describe("check admin permissions for copilot maintenance", () => {
expect(result).toEqual({
authorized: false,
error: "Network error: Connection timeout",
isGitHubApp: false,
});
});
});
Expand Down
13 changes: 12 additions & 1 deletion actions/setup/js/check_membership.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,18 @@ async function main() {
core.setOutput("is_team_member", "false");
core.setOutput("result", "api_error");
core.setOutput("error_message", errorMessage);
await writeDenialSummary(errorMessage, "The permission check failed with a GitHub API error. Check the `pre_activation` job log for details.");
// When the GitHub API responds with "is not a user", the actor is a GitHub App.
// The bot allowlist was either empty or not checked — provide actionable guidance.
if (result.isGitHubApp) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] Great actionable error message! The conditional isGitHubApp check provides workflow authors with precise guidance ("add it to on.bots:") instead of a generic API error.

Positive highlight: The fallback path (line 215) is preserved for non-GitHub-App errors, maintaining backwards compatibility.

await writeDenialSummary(
errorMessage,
`Actor '${actorToValidate}' is a GitHub App, not a regular user. ` +
`To allow this app to trigger the workflow, add it to \`on.bots:\` in the workflow frontmatter. ` +
`Example: \`bots: [${actorToValidate}]\``,
);
Comment on lines +208 to +213
} else {
await writeDenialSummary(errorMessage, "The permission check failed with a GitHub API error. Check the `pre_activation` job log for details.");
}
} else {
const errorMessage =
`Access denied: User '${actorToValidate}' is not authorized. Required permissions: ${requiredPermissions.join(", ")}. ` +
Expand Down
16 changes: 16 additions & 0 deletions actions/setup/js/check_membership.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,22 @@ describe("check_membership.cjs", () => {
expect(mockCore.setOutput).toHaveBeenCalledWith("result", "api_error");
});

it("should provide actionable guidance when GitHub App actor is not in allowed bots list", async () => {
delete process.env.GH_AW_ALLOWED_BOTS;
mockContext.actor = "Copilot";

const notAUserError = { status: 404, message: "Copilot is not a user - https://docs.github.com/rest/collaborators/collaborators#get-repository-permissions-for-a-user" };
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockRejectedValue(notAUserError);

await runScript();

expect(mockCore.setOutput).toHaveBeenCalledWith("is_team_member", "false");
expect(mockCore.setOutput).toHaveBeenCalledWith("result", "api_error");
const errorMessageCall = mockCore.setOutput.mock.calls.find(([key]) => key === "error_message");
expect(errorMessageCall).toBeDefined();
expect(errorMessageCall[1]).toContain("Repository permission check failed");
});

it("should authorize a bot with [bot] suffix in the allowlist via slug fallback", async () => {
process.env.GH_AW_ALLOWED_BOTS = "copilot";
mockContext.actor = "copilot[bot]";
Expand Down
40 changes: 37 additions & 3 deletions actions/setup/js/check_permissions_utils.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,25 @@ function isConfusedDeputyAttack(actor, eventName, payload) {
return false;
}

/**
* Returns true when a GitHub API 404 error indicates the username refers to a
* GitHub App rather than a regular user account.
*
* The collaborators permission endpoint (`GET /repos/{owner}/{repo}/collaborators/{username}/permission`)
* responds with 404 and a message of the form "<login> is not a user" when called
* with a GitHub App actor name (e.g. "Copilot"). A plain "Not Found" 404 means
* the actor simply doesn't have repository access or doesn't exist.
*
* @param {unknown} error - The error to inspect
* @returns {boolean}
*/
function isGitHubAppNotUserError(error) {
if (!error || typeof error !== "object") return false;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] The regex /\bis not a user\b/ is a good defensive boundary check, but consider whether this pattern is stable across all GitHub API error messages.

Suggestion: Add a comment linking to the GitHub API documentation URL that appears in the error message (the one in the test: https://docs.github.com/rest/collaborators/collaborators#get-repository-permissions-for-a-user). This makes it easier to verify the pattern if GitHub changes their error format in the future.

// Regex matches GitHub API error message format documented at:
// https://docs.github.com/rest/collaborators/collaborators#get-repository-permissions-for-a-user
return typeof msg === "string" && /\bis not a user\b/.test(msg);

if (!("status" in error) || error.status !== 404) return false;
const msg = getErrorMessage(error);
return typeof msg === "string" && /\bis not a user\b/.test(msg);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential security issue: The regex /\bis not a user\b/ may be too permissive. If a future GitHub API change adds different error messages containing "is not a user" (e.g., "Repository is not a user workspace"), this function would incorrectly classify it as a GitHub App.

Suggestion: Make the pattern more specific to match the exact GitHub API response format:

return typeof msg === "string" && /^[\w-]+ is not a user\b/.test(msg);

This ensures we're matching "(login) is not a user" and not other unrelated messages.

}

/**
* 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
Expand Down Expand Up @@ -185,7 +204,16 @@ async function checkBotStatus(actor, owner, repo) {
core.info(`Bot '${actor}' is active with permission level: ${botPermission.data.permission}`);
return { isBot: true, isActive: true };
} catch (botError) {
// If we get a 404, the [bot]-suffixed form may not be listed as a collaborator.
// If we get a 404 "is not a user" response, GitHub is telling us this is a GitHub
// App identity (not a regular user). GitHub Apps trigger events via their installation,
// so if an app actor appears in an event payload it must already have repository access.
// Treat it as active to avoid false negatives for apps like Copilot.
if (isGitHubAppNotUserError(botError)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Excellent regression test coverage! The new isGitHubAppNotUserError() check is tested in both the [bot] form (line 541) and slug form (line 548) paths.

Minor observation: The slug-form fallback test creates a scenario where the first lookup returns plain 404 and the second returns "is not a user". This is a good edge case, but consider adding a test where both forms return "is not a user" to ensure the early-return path is also exercised.

Not blocking — current coverage is already strong.

core.info(`Bot '${actor}' is a GitHub App; treating as active based on event trigger`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic concern: This assumes that any GitHub App appearing in an event payload "must already have repository access." While this may be true for installation events, it's not necessarily true for all event types.

For example:

  • A GitHub App could trigger a webhook for a repository it doesn't have access to (e.g., organization-level webhooks)
  • The app's permissions could have been revoked between the event trigger and this check

Suggestion: Consider adding a comment explaining which specific event types this assumption applies to, or add a fallback verification step. At minimum, log the event name to aid debugging:

if (isGitHubAppNotUserError(botError)) {
  core.info(`Bot '${actor}' is a GitHub App (event: ${github.context.eventName}); treating as active based on event trigger`);
  return { isBot: true, isActive: true };
}

return { isBot: true, isActive: true };
}

// If we get a plain 404, the [bot]-suffixed form may not be listed as a collaborator.
// Fall back to checking the non-[bot] (slug) form, as some GitHub Apps appear
// under their plain slug name rather than the [bot]-suffixed form.
if (botError?.status === 404) {
Expand All @@ -199,6 +227,11 @@ async function checkBotStatus(actor, owner, repo) {
return { isBot: true, isActive: true };
} catch (slugError) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical bug: The slug-form fallback loses isGitHubApp tracking.

When the [bot] form returns a "not a user" error (line 211), the function correctly returns early with isActive: true. However, if the [bot] form returns a plain 404, the code falls back to checking the slug form (line 220-238).

The problem: If the slug-form lookup at line 228 also encounters the "is not a user" error, the function returns { isBot: true, isActive: true } (line 233) without preserving the isGitHubApp flag. This means checkRepositoryPermission will return { hasAccess: false, error: ... } with no isGitHubApp field, and the actionable GitHub App guidance in check_membership.cjs:207 will never trigger.

Why this matters: The entire point of this PR is to provide helpful error messages when GitHub Apps are blocked. This fallback path silently loses that context.

Suggested fix: Refactor the slug-form fallback to preserve isGitHubApp detection:

} catch (slugError) {
  if (isGitHubAppNotUserError(slugError)) {
    core.info(`Bot '${actor}' is a GitHub App (slug form); treating as active`);
    return { isBot: true, isActive: true };
  }
  if (slugError?.status === 404) {
    core.info(`Bot '${actor}' not found in repository collaborators (neither [bot] nor slug form)`);
    return { isBot: true, isActive: false };
  }
  // Other errors
  return { isBot: false, isActive: false, error: getErrorMessage(slugError) };
}

This ensures the GitHub App detection works consistently regardless of which API call triggers it.

if (slugError?.status === 404) {
// Same "is not a user" check for the slug form fallback.
if (isGitHubAppNotUserError(slugError)) {
core.info(`Bot '${actor}' is a GitHub App; treating as active based on event trigger`);
return { isBot: true, isActive: true };
}
core.warning(`Bot '${actor}' is not active/installed on ${owner}/${repo}`);
return { isBot: true, isActive: false };
}
Expand All @@ -225,7 +258,7 @@ async function checkBotStatus(actor, owner, repo) {
* @param {string} owner - Repository owner
* @param {string} repo - Repository name
* @param {string[]} requiredPermissions - Array of required permission levels
* @returns {Promise<{authorized: boolean, permission?: string, error?: string}>}
* @returns {Promise<{authorized: boolean, permission?: string, error?: string, isGitHubApp?: boolean}>}
*/
async function checkRepositoryPermission(actor, owner, repo, requiredPermissions) {
try {
Expand Down Expand Up @@ -254,7 +287,7 @@ async function checkRepositoryPermission(actor, owner, repo, requiredPermissions
} catch (repoError) {
const errorMessage = getErrorMessage(repoError);
core.warning(`Repository permission check failed: ${errorMessage}`);
return { authorized: false, error: errorMessage };
return { authorized: false, error: errorMessage, isGitHubApp: isGitHubAppNotUserError(repoError) };
}
}

Expand All @@ -263,6 +296,7 @@ module.exports = {
parseAllowedBots,
canonicalizeBotIdentifier,
isAllowedBot,
isGitHubAppNotUserError,
readAllowBotAuthoredTriggerComment,
isConfusedDeputyAttack,
checkRepositoryPermission,
Expand Down
49 changes: 49 additions & 0 deletions actions/setup/js/check_permissions_utils.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ describe("check_permissions_utils", () => {
expect(result).toEqual({
authorized: false,
error: "API Error: Not Found",
isGitHubApp: false,
});

expect(mockCore.warning).toHaveBeenCalledWith("Repository permission check failed: API Error: Not Found");
Expand All @@ -277,11 +278,25 @@ describe("check_permissions_utils", () => {
expect(result).toEqual({
authorized: false,
error: "String error",
isGitHubApp: false,
});

expect(mockCore.warning).toHaveBeenCalledWith("Repository permission check failed: String error");
});

it("should return isGitHubApp: true when the API says the actor is not a user", async () => {
const notAUserError = { status: 404, message: "Copilot is not a user - https://docs.github.com/rest/collaborators/collaborators#get-repository-permissions-for-a-user" };
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockRejectedValue(notAUserError);

const result = await checkRepositoryPermission("Copilot", "testowner", "testrepo", ["admin"]);

expect(result).toEqual({
authorized: false,
error: "Copilot is not a user - https://docs.github.com/rest/collaborators/collaborators#get-repository-permissions-for-a-user",
isGitHubApp: true,
});
});

it("should check multiple permissions and return true for any match", async () => {
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({
data: { permission: "write" },
Expand Down Expand Up @@ -521,6 +536,40 @@ describe("check_permissions_utils", () => {
expect(mockCore.warning).toHaveBeenCalledWith("Bot 'unknown-app' is not active/installed on testowner/testrepo");
});

it("should return active when [bot] form returns 404 'is not a user' (GitHub App like Copilot)", async () => {
const notUserError = { status: 404, message: "Copilot[bot] is not a user - https://docs.github.com/rest/collaborators/collaborators#get-repository-permissions-for-a-user" };
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockRejectedValue(notUserError);

const result = await checkBotStatus("Copilot[bot]", "testowner", "testrepo");

expect(result).toEqual({ isBot: true, isActive: true });
expect(mockCore.info).toHaveBeenCalledWith("Bot 'Copilot[bot]' is a GitHub App; treating as active based on event trigger");
});

it("should return active when slug form returns 404 'is not a user' (GitHub App Copilot without [bot] suffix)", async () => {
const notFoundError = { status: 404, message: "Not Found" };
const notUserError = { status: 404, message: "Copilot is not a user - https://docs.github.com/rest/collaborators/collaborators#get-repository-permissions-for-a-user" };
mockGithub.rest.repos.getCollaboratorPermissionLevel
.mockRejectedValueOnce(notFoundError) // [bot] form returns plain 404
.mockRejectedValueOnce(notUserError); // slug form returns "not a user"

const result = await checkBotStatus("Copilot", "testowner", "testrepo");

expect(result).toEqual({ isBot: true, isActive: true });
expect(mockCore.info).toHaveBeenCalledWith("Bot 'Copilot' is a GitHub App; treating as active based on event trigger");

expect(mockGithub.rest.repos.getCollaboratorPermissionLevel).toHaveBeenNthCalledWith(1, {
owner: "testowner",
repo: "testrepo",
username: "Copilot[bot]",
});
expect(mockGithub.rest.repos.getCollaboratorPermissionLevel).toHaveBeenNthCalledWith(2, {
owner: "testowner",
repo: "testrepo",
username: "Copilot",
});
});

it("should return inactive with error when slug form returns non-404 error", async () => {
const notFoundError = { status: 404, message: "Not Found" };
const rateLimit = new Error("API rate limit exceeded");
Expand Down
Loading
Loading