refactor(ci): make permission checks reusable - #4740
Conversation
Summary by CodeRabbit
WalkthroughThe pull request adds a shared CI workflow permission checker. It parses restricted YAML syntax, validates workflow and job permissions against reviewed policies, provides 60 self-tests, and integrates Core CI and stale-check validation. ChangesCI Permission Validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CIWorkflow
participant StaleChecker
participant PermissionChecker
participant WorkflowFile
CIWorkflow->>StaleChecker: validate stale workflow permissions
StaleChecker->>PermissionChecker: pass workflow and job policies
PermissionChecker->>WorkflowFile: read workflow YAML
WorkflowFile-->>PermissionChecker: permission records
PermissionChecker-->>StaleChecker: validation status
StaleChecker-->>CIWorkflow: pass or failure
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
.github/ci/check-core-ci-permissions.sh (1)
31-33: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a CLI regression test for repeated
--job-permissions. The parser appends each value and forwards the complete array. Existing direct policy tests cover both jobs, but no CLI test passes repeated values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/ci/check-core-ci-permissions.sh around lines 31 - 33, Add a CLI regression test for repeated --job-permissions arguments, passing both lint-police and security-codeql-scan values and asserting the parser forwards the complete ordered array. Reuse the existing direct policy expectations and test helpers without changing the permission definitions..github/ci/check-ci-permissions.sh (3)
31-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeparate the invocation forms in
usageand document--help.The fourth line has no separator, so the text reads as one invocation with
check-ci-permissions.sh --self-testappended as an operand.mainalso accepts-hand--help, which the text omits.📝 Proposed usage text
printf '%s\n' \ 'Usage: check-ci-permissions.sh --workflow-name NAME --workflow-path PATH' \ ' --workflow-permissions INVENTORY' \ ' [--job-permissions JOB=INVENTORY]...' \ - ' check-ci-permissions.sh --self-test' + ' or: check-ci-permissions.sh --self-test' \ + ' or: check-ci-permissions.sh --help' }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/ci/check-ci-permissions.sh around lines 31 - 37, Update usage() to separate the primary permission-check invocation from the --self-test form, ensuring the self-test command is displayed as its own usage line rather than an operand. Also document the -h/--help options accepted by main, preserving the existing option descriptions and formatting.
311-311: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFeed the workflow to
awkthrough stdin.If
workflow_pathcontains=,awktreats it as a variable assignment and reads stdin instead of the workflow file. Use input redirection:Proposed fix
- ' "${workflow_path}" + ' < "${workflow_path}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/ci/check-ci-permissions.sh at line 311, Update the awk invocation in the workflow-permission check to feed the file referenced by workflow_path through stdin using input redirection, rather than passing workflow_path as an awk argument. Preserve the existing awk processing while ensuring paths containing “=” are read as filenames, not variable assignments.
247-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAccept one leading YAML document start marker.
The yamllint default configuration requires
---. This parser classifies the marker asunparsed-rootand reports the incorrect error,Top-level workflow properties must use canonical unquoted keys. Accept---once at indent 0 and reject subsequent markers.♻️ Proposed parser change
if (indent == 0 && !is_canonical_property(content)) { + if (content == "---" && !seen_document_start) { + seen_document_start = 1 + next + } printf "unparsed-root\t%s\n", content next }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/ci/check-ci-permissions.sh around lines 247 - 250, Update the root-level parsing logic near is_canonical_property so it accepts exactly one YAML document start marker (`---`) at indent 0 without reporting it as unparsed-root, while continuing to reject any subsequent marker and preserving canonical-property validation for other root content.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/ci/check-ci-permissions.sh:
- Around line 31-37: Update usage() to separate the primary permission-check
invocation from the --self-test form, ensuring the self-test command is
displayed as its own usage line rather than an operand. Also document the
-h/--help options accepted by main, preserving the existing option descriptions
and formatting.
- Line 311: Update the awk invocation in the workflow-permission check to feed
the file referenced by workflow_path through stdin using input redirection,
rather than passing workflow_path as an awk argument. Preserve the existing awk
processing while ensuring paths containing “=” are read as filenames, not
variable assignments.
- Around line 247-250: Update the root-level parsing logic near
is_canonical_property so it accepts exactly one YAML document start marker
(`---`) at indent 0 without reporting it as unparsed-root, while continuing to
reject any subsequent marker and preserving canonical-property validation for
other root content.
In @.github/ci/check-core-ci-permissions.sh:
- Around line 31-33: Add a CLI regression test for repeated --job-permissions
arguments, passing both lint-police and security-codeql-scan values and
asserting the parser forwards the complete ordered array. Reuse the existing
direct policy expectations and test helpers without changing the permission
definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a235ecc0-f7e5-4fe0-a97c-974e2262b2e4
📒 Files selected for processing (4)
.github/ci/check-ci-permissions.sh.github/ci/check-core-ci-permissions.sh.github/ci/check-stale-ci-permissions.sh.github/workflows/ci.yaml
The Core permission checker knew both how to read a workflow and the exact permissions Core allows, which meant using it anywhere else would mean copying the whole thing. So, move the parser and fail-closed comparison into `check-ci-permissions.sh`, keep Core's policy in its own small wrapper, and add the existing `Stale PRs` policy as the second caller. Neither workflow gets new token access. Expected green-run effect: No speedup; the shared self-test plus both policy checks add less than one second to the early Core gate. What it really buys us: Another workflow can opt into the same exact enforcement with a small reviewable policy, instead of copying a roughly 600-line parser. This supports NVIDIA#4737 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai Thanks for the eyes. Updated -- split the usage forms and documented |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4740.docs.buildwithfern.com/infra-controller |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Changed files ==='
git diff --name-status HEAD^ HEAD
printf '%s\n' '=== Diff summary ==='
git diff --stat HEAD^ HEAD
printf '%s\n' '=== Script outline ==='
ast-grep outline .github/ci/check-ci-permissions.sh --items all --view expanded || true
ast-grep outline .github/ci/check-core-ci-permissions.sh --items all --view expanded || true
ast-grep outline .github/ci/check-stale-ci-permissions.sh --items all --view expanded || true
printf '%s\n' '=== Relevant checker code ==='
rg -n -C 12 -- '--help|-h|awk|job-permissions|---|extract_permission_records|validate_from_arguments|run_cli_fixture|run_fixture_tests' .github/ci/check-ci-permissions.sh
printf '%s\n' '=== Wrapper and workflow code ==='
cat -n .github/ci/check-core-ci-permissions.sh
cat -n .github/ci/check-stale-ci-permissions.sh
rg -n -C 8 -- 'check-ci-permissions|check-core-ci-permissions|check-stale-ci-permissions' .github/workflows/ci.yaml
printf '%s\n' '=== Current patch ==='
git diff --no-ext-diff --unified=60 HEAD^ HEAD -- \
.github/ci/check-ci-permissions.sh \
.github/ci/check-core-ci-permissions.sh \
.github/ci/check-stale-ci-permissions.sh \
.github/workflows/ci.yamlLength of output: 83113 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Workflow permission declarations ==='
rg -n -C 5 -- '^[[:space:]]{0,6}permissions:|^[[:space:]]{2}[A-Za-z_][A-Za-z0-9_-]*:|^[[:space:]]{6}[a-z][a-z-]*:[[:space:]]+(read|write|none)[[:space:]]*$' \
.github/workflows/ci.yaml .github/workflows/stale-check.yml
printf '%s\n' '=== Jobs with permissions blocks ==='
awk '
/^[[:space:]]{0,2}jobs:[[:space:]]*$/ { in_jobs=1; next }
in_jobs && /^[^[:space:]]/ { in_jobs=0 }
in_jobs && /^ [A-Za-z_][A-Za-z0-9_-]*:[[:space:]]*$/ { job=$1; sub(/:$/, "", job) }
in_jobs && /^ permissions:[[:space:]]*$/ { print FILENAME ": " job " has permissions block" }
' .github/workflows/ci.yaml .github/workflows/stale-check.yml
printf '%s\n' '=== Pull request review threads and checks ==='
gh api graphql \
-F owner='NVIDIA' \
-F name='infra-controller' \
-F number=4740 \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
comments(first: 20) {
nodes { author { login } body path line originalLine }
}
}
}
commits(last: 1) {
nodes {
commit {
statusCheckRollup {
contexts(first: 100) {
nodes {
... on CheckRun { name conclusion status }
... on StatusContext { context state }
}
}
}
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest'Length of output: 51216
The usage and help forms are clear. The AWK stdin change supports workflow paths that contain The Core CI and Stale PR inventories match their workflow permission blocks. GitHub shows no open review threads. CI still has queued checks. 🐇 ✅ [resolve] You are interacting with an AI system. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yaml (1)
1188-1188: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftReplace
secrets: inheritwith explicit registry secret mappings.Declare and pass only
NICO_SOURCE_REGISTRY_USERNAME,NICO_SOURCE_REGISTRY_TOKEN,NICO_TARGET_REGISTRY_USERNAME,NICO_TARGET_REGISTRY_TOKEN,NVCR_USERNAME, andNVCR_TOKENthroughworkflow_call.secrets. The callee does not use other caller secrets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yaml at line 1188, Update the workflow call around `secrets: inherit` to pass only the six specified registry secrets through explicit mappings: `NICO_SOURCE_REGISTRY_USERNAME`, `NICO_SOURCE_REGISTRY_TOKEN`, `NICO_TARGET_REGISTRY_USERNAME`, `NICO_TARGET_REGISTRY_TOKEN`, `NVCR_USERNAME`, and `NVCR_TOKEN`. Remove inherited-secret access and declare these entries under `workflow_call.secrets` as required by the callee.Sources: Path instructions, Linters/SAST tools
🧹 Nitpick comments (3)
.github/ci/check-ci-permissions.sh (3)
886-901: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the implicit fixture-file ordering contract.
The CLI cases read
${fixture_dir}/read-only-policy.yamland${fixture_dir}/complete.yaml. Those files exist only because the earlierrun_counted_fixturecalls at Lines 714 and 824 wrote them. The dependency is positional and unstated.If a future change reorders or renames those earlier cases, these CLI cases fail with
Workflow not foundrather than naming the real cause. Add a short comment that records the dependency.♻️ Proposed comment
+ # The CLI cases below reuse workflow files written by the fixture cases + # above: `read-only-policy.yaml` (line 824) and `complete.yaml` (line 714). + # Keep those cases ahead of this block. run_counted_cli_fixture "valid-workflow" 0 "Checked 1 Read-only fixture job" \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/ci/check-ci-permissions.sh around lines 886 - 901, Add a short comment immediately before the CLI fixture cases using read-only-policy.yaml and complete.yaml, documenting that these files are generated by the earlier run_counted_fixture calls and that their ordering/names must remain aligned. Do not alter the test behavior or command arguments.
765-769: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a fixture for the indent-6
permissionsbranch.Both indentation fixtures relocate the entire
ordinaryjob body, so the first property of the job is already off-indent. The parser therefore reports them through the first-property check at Lines 270-277.The dedicated branch at Lines 295-300 handles a different shape: a valid indent-4 first property followed by a
permissions:key at indent 6. No fixture reaches that branch, so themisplacedrecord it emits is untested.If you add the case, increment
expected_fixture_countat Line 622 to keep the count assertion accurate.♻️ Proposed fixture and case
deep_off_indent="${valid_fixture/$' ordinary:\n runs-on: ubuntu-latest'/$' ordinary:\n permissions:\n contents: read\n runs-on: ubuntu-latest'}" + late_off_indent="${valid_fixture/$' ordinary:\n runs-on: ubuntu-latest'/$' ordinary:\n runs-on: ubuntu-latest\n permissions:\n contents: read'}"run_counted_fixture "${fixture_dir}" "deep-off-indent-permissions" "fail" \ "Job properties on job ordinary must use four-space indentation; found 8 spaces" \ "${deep_off_indent}" + run_counted_fixture "${fixture_dir}" "late-off-indent-permissions" "fail" \ + "Job properties on job ordinary must use four-space indentation; found 6 spaces" \ + "${late_off_indent}"Declare
late_off_indentwith the other fixture locals and setexpected_fixture_countto 61.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/ci/check-ci-permissions.sh around lines 765 - 769, Add a `late_off_indent` fixture alongside the existing fixture locals, shaping it with a valid four-space first job property followed by a `permissions` key indented six spaces so the dedicated misplaced-permissions branch is exercised. Register this fixture in the counted cases with the expected failure and update `expected_fixture_count` to 61.
52-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReject empty inventory fields explicitly.
read -r -awithIFS=','discards a trailing empty field.contents=read,therefore passes the per-entry regex loop. Theprintf | tr | sort | pastepipeline at Lines 71-74 still sees the empty field and produces,contents=read. The comparison then fails closed, but the reported inventory is misleading.Add an explicit guard so the error names the real defect.
♻️ Proposed guard for empty fields
- IFS=',' read -r -a entries <<< "${permissions}" + if [[ "${permissions}" == *,, || "${permissions}" == ,* || "${permissions}" == *, ]]; then + printf 'Permission inventory must not contain empty entries: %s.\n' \ + "${permissions}" >&2 + return 1 + fi + IFS=',' read -r -a entries <<< "${permissions}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/ci/check-ci-permissions.sh around lines 52 - 69, Update the permission-entry validation loop around entries and before the per-entry regex check to explicitly reject empty inventory fields, including trailing fields discarded by read -a. Emit the existing invalid-entry error with the empty field context and return 1 before continuing to scope validation or comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/ci.yaml:
- Line 1188: Update the workflow call around `secrets: inherit` to pass only the
six specified registry secrets through explicit mappings:
`NICO_SOURCE_REGISTRY_USERNAME`, `NICO_SOURCE_REGISTRY_TOKEN`,
`NICO_TARGET_REGISTRY_USERNAME`, `NICO_TARGET_REGISTRY_TOKEN`, `NVCR_USERNAME`,
and `NVCR_TOKEN`. Remove inherited-secret access and declare these entries under
`workflow_call.secrets` as required by the callee.
---
Nitpick comments:
In @.github/ci/check-ci-permissions.sh:
- Around line 886-901: Add a short comment immediately before the CLI fixture
cases using read-only-policy.yaml and complete.yaml, documenting that these
files are generated by the earlier run_counted_fixture calls and that their
ordering/names must remain aligned. Do not alter the test behavior or command
arguments.
- Around line 765-769: Add a `late_off_indent` fixture alongside the existing
fixture locals, shaping it with a valid four-space first job property followed
by a `permissions` key indented six spaces so the dedicated
misplaced-permissions branch is exercised. Register this fixture in the counted
cases with the expected failure and update `expected_fixture_count` to 61.
- Around line 52-69: Update the permission-entry validation loop around entries
and before the per-entry regex check to explicitly reject empty inventory
fields, including trailing fields discarded by read -a. Emit the existing
invalid-entry error with the empty field context and return 1 before continuing
to scope validation or comparison.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 889298fe-2724-4b61-bdd7-906fe2453c2f
📒 Files selected for processing (2)
.github/ci/check-ci-permissions.sh.github/workflows/ci.yaml
1 participant
This is a follow-up to #4656, where we made Core's GitHub token read-only by default and added a checker to enforce that exact policy, but left the checker built specifically around Core. Its Core policy, messages, and fixtures were woven into the parser, so another workflow would have to copy roughly 600 lines or make the Core script understand every workflow.
So, this moves the parser and fail-closed comparison into
check-ci-permissions.sh, keeps Core's exact policy in a small wrapper, and adds the existingStale PRspolicy as the second caller. Neither workflow gets new token access.Related issues
This supports #4737
Part of #4572
Type of Change
Breaking Changes
Testing
Tested by running the shared checker's 60 fixtures, both live workflow policies, Bash syntax checks, Actionlint, the merged Core final-gate tests/inventory,
cargo make format-nightly,cargo make clippy, and the cached Carbide-lints workflow.The live policy summaries remain:
Additional Notes
This deliberately does not enroll every workflow at once. Workflows with root-level writes or nested reusable calls need their permissions reviewed and tightened before they can use the shared checker safely.