[WIP] Fix check_permissions to handle missing inherited_role field - #50183
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Thanks for tackling the One note: this PR is still marked [WIP] and is a draft — before it's ready for review, please confirm the checklist items are fully complete (e.g., Once that's done, this should be in good shape for a maintainer to review. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
Triage: bug (security) / medium risk
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #50183 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
There was a problem hiding this comment.
Pull request overview
Fixes custom repository role resolution by replacing the nonexistent inherited_role field with organization role metadata.
Changes:
- Resolves custom roles through
listCustomRepoRoles. - Adds realistic success and fail-closed tests.
- Refreshes a generated workflow label.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/check_permissions_utils.cjs |
Adds custom role base-role resolution. |
actions/setup/js/check_permissions_utils.test.cjs |
Expands custom-role test coverage. |
.github/workflows/smoke-goose.lock.yml |
Updates generated step capitalization. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| */ | ||
| async function resolveCustomRoleBaseRole(org, roleName) { | ||
| try { | ||
| const response = await github.rest.orgs.listCustomRepoRoles({ org }); |
There was a problem hiding this comment.
Review: Fix check_permissions to handle missing inherited_role field
The approach is sound — removing the unreliable inherited_role field and instead calling GET /orgs/{org}/custom-repository-roles to resolve base_role is the correct design. The fail-closed behaviour, case-insensitive matching, and comprehensive test coverage are all good.
One blocking security issue found:
resolveCustomRoleBaseRole validates the resolved base_role against STANDARD_ROLES, which includes "admin". The GitHub API contract says custom role base_role can only be read | triage | write | maintain, never admin — but the code does not enforce this. If the API unexpectedly returns base_role: "admin", the function would return "admin" and a custom-role user could bypass an admin permission requirement. A narrower guard set (excluding admin) is needed, plus a test for this case.
See inline comment for the specific fix.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 38.9 AIC · ⌖ 12.3 AIC · ⊞ 5.4K
Comments that could not be inline-anchored
actions/setup/js/check_permissions_utils.cjs:45
Security: STANDARD_ROLES includes "admin" but base_role for custom roles should never be admin.
The GitHub API spec for GET /orgs/{org}/custom-repository-roles only allows base_role values of read, triage, write, or maintain. However, if an unexpected base_role: "admin" were returned (API change, malicious mock, or unexpected response), STANDARD_ROLES.has(baseRole) would pass and resolveCustomRoleBaseRole would return "admin". This would allow a custom-role user…
🧪 Test Quality Sentinel Report
📊 Metrics (6 tests)
📈 Test Inflation Analysis
Justified by:
Assessment: Verdict
Recommendation: Approve. Tests demonstrate strong design discipline and comprehensive behavioral coverage despite slight inflation.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness issues and two test-coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Case mismatch risk (correctness):
resolvedBaseRoleis always lowercased, butnormalizedRequiredis not. Mixed-case required permissions from env vars will fail to match. - No org-roles cache:
listCustomRepoRolesis called on every custom-role check per run; a simple module-levelMapwould prevent redundant API calls and rate-limit exposure. - Test spec gaps: The new real-world
"Project Lead"test lacks key assertions; there is no test for a missing/nullcustom_rolesfield in the API response.
Positive Highlights
- ✅ Excellent root-cause fix: removing the non-existent
inherited_rolefield and replacing it with a proper API call is the right approach. - ✅ Fail-closed behaviour is preserved and well-tested across multiple error scenarios.
- ✅ Case-insensitive role name matching is correctly implemented and tested.
- ✅ JSDoc types added to the new function for better tooling support.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 58.8 AIC · ⌖ 8.37 AIC · ⊞ 7.1K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/check_permissions_utils.cjs:319
[/diagnosing-bugs] normalizeRoleName does not lowercase its input, but resolvedBaseRole is always stored lowercased (via match.base_role.toLowerCase()). If a caller passes a mixed-case required permission such as "Write" from an unprocessed env var, normalizedRequired === resolvedBaseRole silently mismatches and the user is denied.
<details>
<summary>💡 Suggested fix</summary>
Lowercase before normalising:
const normalizedRequired = normalizeRoleName(requiredPerm.toLowerCa…
</details>
<details><summary>actions/setup/js/check_permissions_utils.cjs:303</summary>
**[/diagnosing-bugs]** `resolveCustomRoleBaseRole` is called unconditionally for every custom-role user — even when the same org/role combination was already resolved earlier in a workflow run. The `GET /orgs/{org}/custom-repository-roles` response is stable within a single run; caching it (even in a module-level `Map`) would eliminate redundant API calls and protect against rate-limit failures on large permission checks.
<details>
<summary>💡 Suggested approach</summary>
```js
const _customR…
</details>
<details><summary>actions/setup/js/check_permissions_utils.cjs:259</summary>
**[/diagnosing-bugs]** `normalizeRoleName(match.base_role.toLowerCase())` double-applies case transformation: `base_role` is already lowercased before `normalizeRoleName` is called (which maps `"maintainer"` → `"maintain"`), so the normalization works. However, the intent isn't obvious. Consider extracting the lowercasing inside `normalizeRoleName` directly so callers don't need to pre-lowercase.
This is low-severity today, but it establishes an inconsistent calling convention that could hide …
</details>
<details><summary>actions/setup/js/check_permissions_utils.test.cjs:371</summary>
**[/tdd]** The real-world `"Project Lead"` test (line ~355) is missing assertions on the debug log output and the `listCustomRepoRoles` mock call. Without them, the test only verifies the happy-path return value but wouldn't catch a regression in the role-resolution code path (e.g., if `listCustomRepoRoles` were accidentally skipped).
<details>
<summary>💡 Suggested additions</summary>
```js
expect(mockGithub.rest.orgs.listCustomRepoRoles).toHaveBeenCalledWith({ org: "example-org" });
expect(…
</details>
<details><summary>actions/setup/js/check_permissions_utils.cjs:252</summary>
**[/tdd]** There is no test for the case where `response.data.custom_roles` is `null` or absent (only the API-error path is tested). The `?? []` guard handles it silently, but a missing test means a regression (e.g., if the API changes its response shape) would go undetected.
<details>
<summary>💡 Suggested test</summary>
```js
it("should fail closed when custom_roles list is absent from org response", async () => {
mockGithub.rest.repos.getCollaboratorPermissionLevel.mockResolvedValue({
…
</details>Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Refreshed and re-ran the pr-finisher pass.
Still preventing merge: CI must be re-triggered by a maintainer (close/reopen the PR, |
|
@copilot add more logging using core.info |
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection results could not be parsed. Review the workflow run logs for details. Triage update: bug (security) / medium risk
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 1a3a6dc — the permission-resolution diagnostics now use
Tests updated accordingly; 102 tests pass and |
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
@copilot This PR needs a quick maintainer-ready refresh.
Run: https://github.com/github/gh-aw/actions/runs/30912825871
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness/clarity issues before merge.
📋 Key Themes & Highlights
Key Themes
debug→infopromotion: All diagnostic logging was bulk-promoted tocore.info. This loses theACTIONS_RUNNER_DEBUGgating and will clutter workflow logs for every permission check, not just failures or custom-role paths.- Log message clarity: The fail-closed message for
resolvedBaseRole === ''doesn't distinguish betweenpermission: 'none'(GitHub's explicit no-access sentinel) and a genuinely unknown value — the wording could mislead operators reading logs. - Real-world test coverage: The new
Project Leadtest is a good addition but leaves the code path under-specified — it only assertsauthorized: true, not the resolved role name logged.
Positive Highlights
- ✅ Root cause fix is clean and correct:
permissionis always present in the API response, whereasinherited_rolewas not. - ✅ Fail-closed semantics are preserved: non-standard permission values correctly deny access.
- ✅ Test suite is well-structured and the rename from
inherited_role→base_rolein variable names reads clearly. - ✅ Removing the
resolveCustomRoleBaseRole/listCustomRepoRolespath (mentioned in the PR checklist but not needed) keeps the change minimal.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 41 AIC · ⌖ 14 AIC · ⊞ 7.1K
Comment /matt to run again
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done in 79bb1c3.
Remaining blockers: a maintainer must re-trigger CI (close/reopen, |
|
@copilot This PR needs a quick maintainer-ready refresh.
Run: https://github.com/github/gh-aw/actions/runs/30917238977
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
inherited_rolefield usage fromcheckRepositoryPermissionresolveCustomRoleBaseRole(org, roleName)usinggithub.rest.orgs.listCustomRepoRoles, case-insensitive name match, fail closed on miss/errorbase_rolemaintainbase role, admin denial, case-insensitivity, fail-closed)inherited_role/inheritedRolereferencesmake agent-report-progressinherited_roledoes not exist in the GitHub API, so custom repository roles are always denied #50068Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.
Run: https://github.com/github/gh-aw/actions/runs/30909334614
Run: https://github.com/github/gh-aw/actions/runs/30912825871