Skip to content

Commit 752161d

Browse files
Copilotpelikhangithub-actions[bot]gh-aw-bot
authored
[WIP] Fix check_permissions to handle missing inherited_role field (#50183)
* Initial plan * Resolve custom repository role base_role from org custom roles API Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Resolve custom role base role from GITHUB_TOKEN-readable permission field Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Promote repository permission diagnostics from core.debug to core.info Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Never let a custom repository role resolve to admin Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> * Tune permission check log levels for signal over noise Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
1 parent 1c28255 commit 752161d

2 files changed

Lines changed: 112 additions & 41 deletions

File tree

‎actions/setup/js/check_permissions_utils.cjs‎

Lines changed: 27 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,10 @@ const { getErrorMessage } = require("./error_helpers.cjs");
55

66
const STANDARD_ROLES = new Set(["admin", "maintain", "write", "triage", "read"]);
77

8+
// Base roles a custom organization repository role can be derived from. `admin` is
9+
// deliberately excluded: custom repository roles can never confer admin access.
10+
const CUSTOM_ROLE_BASE_ROLES = new Set(["maintain", "write", "triage", "read"]);
11+
812
/**
913
* Normalize GitHub permission/role aliases to the canonical values used by on.roles.
1014
* @param {string} role
@@ -253,39 +257,42 @@ async function checkRepositoryPermission(actor, owner, repo, requiredPermissions
253257
username: actor,
254258
});
255259

256-
/** @type {{ permission: string, role_name?: unknown, inherited_role?: unknown }} */
260+
/** @type {{ permission: string, role_name?: unknown }} */
257261
const repoPermissionData = repoPermission.data;
258262
const permission = repoPermissionData.permission;
259263
const rawRoleName = repoPermissionData.role_name;
260264
const roleName = rawRoleName == null ? "" : typeof rawRoleName === "string" ? rawRoleName : "";
261-
const rawInheritedRole = repoPermissionData.inherited_role;
262-
const inheritedRole = rawInheritedRole == null ? "" : typeof rawInheritedRole === "string" ? rawInheritedRole : "";
263265
const normalizedRoleName = normalizeRoleName(roleName);
264266
const normalizedPermission = normalizeRoleName(permission);
265-
const normalizedInheritedRole = normalizeRoleName(inheritedRole);
266267
const effectiveRole = normalizedRoleName || normalizedPermission;
267268
const logDetails = normalizedRoleName && normalizedRoleName !== normalizedPermission ? `${normalizedPermission} (role: ${normalizedRoleName})` : normalizedPermission;
268269
core.info(`Repository permission level: ${logDetails}`);
269270

270271
// Standard GitHub repository permission levels. Custom org repository roles (e.g.
271-
// "Security Champions") have a role_name that is not one of these — for those, use
272-
// the inherited standard role from GitHub's custom-role metadata so the actor is not
272+
// "Security Champions") have a role_name that is not one of these — for those, fall back
273+
// to the standard `permission` level reported by the same endpoint so the actor is not
273274
// blocked simply because their custom role name is not literally listed in on.roles.
275+
// A custom role can never grant admin: GitHub derives custom repository roles from the
276+
// read/triage/write/maintain base roles only, so `admin` is refused here even if the API
277+
// unexpectedly reports it for a custom role.
274278
const isCustomRole = normalizedRoleName !== "" && !STANDARD_ROLES.has(normalizedRoleName);
275-
const inheritedStandardRole = isCustomRole && STANDARD_ROLES.has(normalizedInheritedRole) ? normalizedInheritedRole : "";
279+
const resolvedBaseRole = isCustomRole && CUSTOM_ROLE_BASE_ROLES.has(normalizedPermission) ? normalizedPermission : "";
276280
const debugRoleName = normalizedRoleName || "<empty>";
277-
const debugInheritedRole = normalizedInheritedRole || "<empty>";
278-
const debugInheritedStandardRole = inheritedStandardRole || "<empty>";
279-
core.debug?.(`Repository permission API fields for '${actor}': permission='${normalizedPermission}', role='${debugRoleName}', inherited='${debugInheritedRole}'`);
280-
core.debug?.(`Repository permission computed roles for '${actor}': effective='${effectiveRole}', custom_role=${isCustomRole}, inherited_standard_role='${debugInheritedStandardRole}'`);
281-
if (isCustomRole && inheritedStandardRole === "") {
282-
core.debug?.(`Repository permission fallback unavailable for custom role '${normalizedRoleName}' because GitHub did not provide an inherited standard role`);
281+
const debugBaseRole = resolvedBaseRole || "<empty>";
282+
core.debug?.(`Repository permission API fields for '${actor}': permission='${normalizedPermission}', role='${debugRoleName}'`);
283+
core.debug?.(`Repository permission computed roles for '${actor}': effective='${effectiveRole}', custom_role=${isCustomRole}, base_role='${debugBaseRole}'`);
284+
if (isCustomRole && normalizedPermission === "admin") {
285+
core.warning(`Ignoring 'admin' permission reported for custom repository role '${normalizedRoleName}': custom roles cannot grant admin access`);
286+
}
287+
if (isCustomRole && resolvedBaseRole === "") {
288+
core.info(`Repository permission fallback unavailable for custom role '${normalizedRoleName}' because GitHub did not report a standard permission level`);
283289
}
284290

285291
// Check if user has one of the required permission levels.
286292
// For standard roles, use role_name (precise: maintain/triage are not collapsed to
287-
// write/read). For custom org roles, only fall back to the inherited standard role
288-
// from custom-role metadata; fail closed if GitHub does not provide it.
293+
// write/read). For custom org roles, fall back to the standard `permission` level that
294+
// GitHub already computes for the actor (readable with the repository-scoped
295+
// GITHUB_TOKEN); fail closed if it is not one of the non-admin base roles.
289296
/** @type {{ permission: string, roleMatchType: string }|null} */
290297
let permissionMatch = null;
291298
for (const requiredPerm of requiredPermissions) {
@@ -294,14 +301,16 @@ async function checkRepositoryPermission(actor, owner, repo, requiredPermissions
294301
permissionMatch = { permission: normalizedRequired, roleMatchType: "effective-role" };
295302
break;
296303
}
297-
if (normalizedRequired === inheritedStandardRole) {
298-
permissionMatch = { permission: normalizedRequired, roleMatchType: "inherited-standard-role" };
304+
if (resolvedBaseRole !== "" && normalizedRequired === resolvedBaseRole) {
305+
permissionMatch = { permission: normalizedRequired, roleMatchType: "base-role" };
299306
break;
300307
}
301308
}
302309

303310
if (permissionMatch) {
304-
core.debug?.(`Repository permission matched required role '${permissionMatch.permission}' via ${permissionMatch.roleMatchType}`);
311+
if (permissionMatch.roleMatchType === "base-role") {
312+
core.info(`Custom repository role '${normalizedRoleName}' satisfied required role '${permissionMatch.permission}' via base role`);
313+
}
305314
core.info(`✅ User has ${effectiveRole} access to repository`);
306315
return { authorized: true, permission: effectiveRole };
307316
}

‎actions/setup/js/check_permissions_utils.test.cjs‎

Lines changed: 85 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@ const mockGithub = {
1919
repos: {
2020
getCollaboratorPermissionLevel: vi.fn(),
2121
},
22+
orgs: {
23+
listCustomRepoRoles: vi.fn(),
24+
},
2225
},
2326
};
2427

@@ -325,9 +328,9 @@ describe("check_permissions_utils", () => {
325328
expect(mockCore.warning).toHaveBeenCalledWith("User permission 'maintain' does not meet requirements: write");
326329
});
327330

328-
it("should authorize custom org role via base permission when base permission matches", async () => {
331+
it("should authorize custom org role via the standard permission level", async () => {
329332
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({
330-
data: { permission: "write", role_name: "Security Champions", inherited_role: "write" },
333+
data: { permission: "write", role_name: "Security Champions" },
331334
});
332335

333336
const result = await checkRepositoryPermission("testuser", "testowner", "testrepo", ["admin", "maintain", "write"]);
@@ -336,29 +339,47 @@ describe("check_permissions_utils", () => {
336339
authorized: true,
337340
permission: "Security Champions",
338341
});
339-
expect(mockCore.debug).toHaveBeenCalledWith("Repository permission API fields for 'testuser': permission='write', role='Security Champions', inherited='write'");
340-
expect(mockCore.debug).toHaveBeenCalledWith("Repository permission computed roles for 'testuser': effective='Security Champions', custom_role=true, inherited_standard_role='write'");
341-
expect(mockCore.debug).toHaveBeenCalledWith("Repository permission matched required role 'write' via inherited-standard-role");
342+
expect(mockGithub.rest.orgs.listCustomRepoRoles).not.toHaveBeenCalled();
343+
expect(mockCore.debug).toHaveBeenCalledWith("Repository permission API fields for 'testuser': permission='write', role='Security Champions'");
344+
expect(mockCore.debug).toHaveBeenCalledWith("Repository permission computed roles for 'testuser': effective='Security Champions', custom_role=true, base_role='write'");
345+
expect(mockCore.info).toHaveBeenCalledWith("Custom repository role 'Security Champions' satisfied required role 'write' via base role");
342346
expect(mockCore.info).toHaveBeenCalledWith("✅ User has Security Champions access to repository");
343347
});
344348

345-
it("should reject maintain-based custom org role when only write is required", async () => {
349+
it("should authorize the real-world Project Lead payload with write permission", async () => {
346350
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({
347-
data: { permission: "write", role_name: "Security Champions", inherited_role: "maintain" },
351+
data: {
352+
permission: "write",
353+
role_name: "Project Lead",
354+
user: { login: "octocat", role_name: "Project Lead" },
355+
},
348356
});
349357

350-
const result = await checkRepositoryPermission("testuser", "testowner", "testrepo", ["write"]);
358+
const result = await checkRepositoryPermission("octocat", "example-org", "example-repo", ["admin", "maintain", "write"]);
359+
360+
expect(result).toEqual({
361+
authorized: true,
362+
permission: "Project Lead",
363+
});
364+
});
365+
366+
it("should reject a write-permission custom org role when maintain is required", async () => {
367+
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({
368+
data: { permission: "write", role_name: "Security Champions" },
369+
});
370+
371+
const result = await checkRepositoryPermission("testuser", "testowner", "testrepo", ["maintain"]);
351372

352373
expect(result).toEqual({
353374
authorized: false,
354375
permission: "Security Champions",
355376
});
356-
expect(mockCore.warning).toHaveBeenCalledWith("User permission 'Security Champions' does not meet requirements: write");
377+
expect(mockCore.warning).toHaveBeenCalledWith("User permission 'Security Champions' does not meet requirements: maintain");
357378
});
358379

359-
it("should authorize maintain-based custom org role when maintain is required", async () => {
380+
it("should authorize a maintain-permission custom org role when maintain is required", async () => {
360381
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({
361-
data: { permission: "write", role_name: "Security Champions", inherited_role: "maintain" },
382+
data: { permission: "maintain", role_name: "Security Champions" },
362383
});
363384

364385
const result = await checkRepositoryPermission("testuser", "testowner", "testrepo", ["maintain"]);
@@ -370,9 +391,51 @@ describe("check_permissions_utils", () => {
370391
expect(mockCore.info).toHaveBeenCalledWith("✅ User has Security Champions access to repository");
371392
});
372393

373-
it("should authorize read-based custom org role when read is required", async () => {
394+
it("should not authorize a write-permission custom org role for admin", async () => {
374395
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({
375-
data: { permission: "read", role_name: "Security Champions", inherited_role: "read" },
396+
data: { permission: "write", role_name: "Security Champions" },
397+
});
398+
399+
const result = await checkRepositoryPermission("testuser", "testowner", "testrepo", ["admin"]);
400+
401+
expect(result).toEqual({
402+
authorized: false,
403+
permission: "Security Champions",
404+
});
405+
});
406+
407+
it("should never authorize admin for a custom org role reporting admin permission", async () => {
408+
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({
409+
data: { permission: "admin", role_name: "Security Champions" },
410+
});
411+
412+
const result = await checkRepositoryPermission("testuser", "testowner", "testrepo", ["admin"]);
413+
414+
expect(result).toEqual({
415+
authorized: false,
416+
permission: "Security Champions",
417+
});
418+
expect(mockCore.warning).toHaveBeenCalledWith("Ignoring 'admin' permission reported for custom repository role 'Security Champions': custom roles cannot grant admin access");
419+
expect(mockCore.debug).toHaveBeenCalledWith("Repository permission computed roles for 'testuser': effective='Security Champions', custom_role=true, base_role='<empty>'");
420+
});
421+
422+
it("should not let an admin-permission custom org role satisfy a lesser required role", async () => {
423+
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({
424+
data: { permission: "admin", role_name: "Security Champions" },
425+
});
426+
427+
const result = await checkRepositoryPermission("testuser", "testowner", "testrepo", ["write"]);
428+
429+
expect(result).toEqual({
430+
authorized: false,
431+
permission: "Security Champions",
432+
});
433+
expect(mockCore.warning).toHaveBeenCalledWith("User permission 'Security Champions' does not meet requirements: write");
434+
});
435+
436+
it("should authorize read-permission custom org role when read is required", async () => {
437+
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({
438+
data: { permission: "read", role_name: "Security Champions" },
376439
});
377440

378441
const result = await checkRepositoryPermission("testuser", "testowner", "testrepo", ["read"]);
@@ -384,9 +447,9 @@ describe("check_permissions_utils", () => {
384447
expect(mockCore.info).toHaveBeenCalledWith("✅ User has Security Champions access to repository");
385448
});
386449

387-
it("should reject read-based custom org role when required permission does not match", async () => {
450+
it("should reject read-permission custom org role when required permission does not match", async () => {
388451
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({
389-
data: { permission: "read", role_name: "Security Champions", inherited_role: "read" },
452+
data: { permission: "read", role_name: "Security Champions" },
390453
});
391454

392455
const result = await checkRepositoryPermission("testuser", "testowner", "testrepo", ["write"]);
@@ -400,7 +463,7 @@ describe("check_permissions_utils", () => {
400463

401464
it("should authorize when required permissions include the exact custom role name", async () => {
402465
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({
403-
data: { permission: "write", role_name: "Security Champions", inherited_role: "maintain" },
466+
data: { permission: "read", role_name: "Security Champions" },
404467
});
405468

406469
const result = await checkRepositoryPermission("testuser", "testowner", "testrepo", ["Security Champions"]);
@@ -414,7 +477,7 @@ describe("check_permissions_utils", () => {
414477

415478
it("should not treat an empty role_name as a custom org role", async () => {
416479
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({
417-
data: { permission: "write", role_name: "", inherited_role: "maintain" },
480+
data: { permission: "write", role_name: "" },
418481
});
419482

420483
const result = await checkRepositoryPermission("testuser", "testowner", "testrepo", ["maintain"]);
@@ -423,12 +486,13 @@ describe("check_permissions_utils", () => {
423486
authorized: false,
424487
permission: "write",
425488
});
489+
expect(mockGithub.rest.orgs.listCustomRepoRoles).not.toHaveBeenCalled();
426490
expect(mockCore.warning).toHaveBeenCalledWith("User permission 'write' does not meet requirements: maintain");
427491
});
428492

429-
it("should fail closed for custom org role when inherited role metadata is unavailable", async () => {
493+
it("should fail closed for a custom org role with a non-standard permission value", async () => {
430494
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({
431-
data: { permission: "write", role_name: "Security Champions" },
495+
data: { permission: "none", role_name: "Security Champions" },
432496
});
433497

434498
const result = await checkRepositoryPermission("testuser", "testowner", "testrepo", ["write"]);
@@ -437,10 +501,8 @@ describe("check_permissions_utils", () => {
437501
authorized: false,
438502
permission: "Security Champions",
439503
});
440-
expect(mockCore.debug).toHaveBeenCalledWith("Repository permission API fields for 'testuser': permission='write', role='Security Champions', inherited='<empty>'");
441-
expect(mockCore.debug).toHaveBeenCalledWith("Repository permission computed roles for 'testuser': effective='Security Champions', custom_role=true, inherited_standard_role='<empty>'");
442-
expect(mockCore.debug).toHaveBeenCalledWith("Repository permission fallback unavailable for custom role 'Security Champions' because GitHub did not provide an inherited standard role");
443-
expect(mockCore.debug).toHaveBeenCalledWith("Repository permission did not match required roles: write");
504+
expect(mockCore.debug).toHaveBeenCalledWith("Repository permission computed roles for 'testuser': effective='Security Champions', custom_role=true, base_role='<empty>'");
505+
expect(mockCore.info).toHaveBeenCalledWith("Repository permission fallback unavailable for custom role 'Security Champions' because GitHub did not report a standard permission level");
444506
expect(mockCore.warning).toHaveBeenCalledWith("User permission 'Security Champions' does not meet requirements: write");
445507
});
446508

0 commit comments

Comments
 (0)