fix: make Core CI tokens read-only by default - #4656
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThe workflow applies a read-only ChangesCore CI permissions
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ChangesJob as changes job
participant Validator as check-core-ci-permissions.sh
participant Workflow as ci.yaml
ChangesJob->>Validator: invoke permission validation
Validator->>Workflow: parse permission mappings
Validator-->>ChangesJob: report validation result
🚥 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 (3)
scripts/check-core-ci-permissions.sh (3)
333-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify the cleanup trap and remove the glob risk.
rm -f "${fixture_dir}"/*.yamlexpands to/*.yamliffixture_diris ever empty.set -eprevents that today, but the construct is fragile and leaves any non-.yamlfile behind, which then makesrmdirfail inside the trap.Use a single recursive removal with an end-of-options guard.
♻️ Suggested fix
fixture_dir="$(mktemp -d)" - trap 'rm -f "${fixture_dir}"/*.yaml; rmdir "${fixture_dir}"' EXIT + trap 'rm -rf -- "${fixture_dir}"' EXITAs per path instructions: "Review scripts for shell safety, quoting, idempotency, dependency checks, error handling, and avoiding secret leakage in logs."
🤖 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 `@scripts/check-core-ci-permissions.sh` around lines 333 - 334, Replace the cleanup trap immediately after fixture_dir creation with a single recursive removal command that uses an end-of-options guard and preserves the quoted fixture_dir path. Remove the rm glob and separate rmdir so the trap safely handles any contents and an empty or unusual directory path.Source: Path instructions
114-117: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winClear
in_jobswhen a new top-level key starts.
in_jobsis set atjobs:and never reset. If any top-level mapping follows thejobs:block, its indent-2 keys matchis_job_keyand are counted as jobs. That inflatesjob_count, creates phantom owners inseen_jobs, and can attribute a laterpermissions:block to a non-existent job.The current
ci.yamlplacesjobs:last, so the defect is latent. Reset the flag so a future reorder does not silently change the result.♻️ Suggested fix
+ if (indent == 0 && content != "jobs:") { + in_jobs = 0 + current_job = "" + } + if (indent == 0 && content == "jobs:") { in_jobs = 1 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 `@scripts/check-core-ci-permissions.sh` around lines 114 - 117, Update the top-level key handling in the YAML parsing loop so encountering any top-level mapping key other than jobs: clears in_jobs before processing it. Preserve setting in_jobs for jobs:, ensuring subsequent top-level sections are not treated as job entries or associated permissions.
149-154: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope the
misplacedrule to job-level keys only.The rule flags any
permissions:key at an indent greater than 2 that is not exactly 4. Step inputs can legitimately contain a key namedpermissions:underwith:at indent 10. In that case the validator reports "must use four-space indentation" and fails CI with a misleading message.Constrain the rule to the job mapping level. Ignore keys nested below a
steps:sequence.♻️ Suggested narrowing
+ if (in_jobs && indent == 4 && content ~ /^steps:/) { + in_steps = 1 + } else if (in_jobs && indent <= 4) { + in_steps = 0 + } + - if (in_jobs && current_job != "" && indent > 2 && indent != 4 && + if (in_jobs && current_job != "" && !in_steps && indent > 2 && indent != 4 && content ~ /^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 `@scripts/check-core-ci-permissions.sh` around lines 149 - 154, Update the misplaced permissions check in the script’s job-processing logic to apply only to job-mapping-level permissions keys, excluding keys nested beneath a steps: sequence such as step with inputs. Preserve the existing indentation validation for actual job-level permissions declarations and avoid reporting nested step data.
🤖 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 `@scripts/check-core-ci-permissions.sh`:
- Around line 333-334: Replace the cleanup trap immediately after fixture_dir
creation with a single recursive removal command that uses an end-of-options
guard and preserves the quoted fixture_dir path. Remove the rm glob and separate
rmdir so the trap safely handles any contents and an empty or unusual directory
path.
- Around line 114-117: Update the top-level key handling in the YAML parsing
loop so encountering any top-level mapping key other than jobs: clears in_jobs
before processing it. Preserve setting in_jobs for jobs:, ensuring subsequent
top-level sections are not treated as job entries or associated permissions.
- Around line 149-154: Update the misplaced permissions check in the script’s
job-processing logic to apply only to job-mapping-level permissions keys,
excluding keys nested beneath a steps: sequence such as step with inputs.
Preserve the existing indentation validation for actual job-level permissions
declarations and avoid reporting nested step data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a73afe58-2586-4d24-aff6-0b744c8e525e
📒 Files selected for processing (2)
.github/workflows/ci.yamlscripts/check-core-ci-permissions.sh
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/check-core-ci-permissions.sh (3)
333-334: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the cleanup trap.
rmdirfails if the directory holds any file that does not match*.yaml.rm -rfremoves the directory unconditionally and keeps the trap free of that dependency.♻️ Proposed simplification
fixture_dir="$(mktemp -d)" - trap 'rm -f "${fixture_dir}"/*.yaml; rmdir "${fixture_dir}"' EXIT + trap 'rm -rf "${fixture_dir}"' EXIT🤖 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 `@scripts/check-core-ci-permissions.sh` around lines 333 - 334, Update the cleanup trap associated with fixture_dir to use recursive forced removal of the temporary directory instead of separately deleting YAML files and calling rmdir. Keep cleanup unconditional and independent of the directory’s contents.
362-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd fixtures for the two uncovered validator branches, and derive the fixture count.
Two rejection paths have no fixture:
- The
invalidrecord at line 218, reached by an entry such ascontents: readonly.- The duplicate-block check at lines 230 and 251, reached by two
permissions:blocks on the same owner.Line 381 also hardcodes
8. If a fixture is added and the literal is not updated, the summary reports a wrong count. Count the executed fixtures instead.♻️ Proposed fixtures and derived count
off_indent="${valid_fixture/$' ordinary:\n runs-on: ubuntu-latest'/$' ordinary:\n permissions:\n contents: read\n runs-on: ubuntu-latest'}" job_write_all="${valid_fixture/$' ordinary:\n runs-on: ubuntu-latest'/$' ordinary:\n permissions: write-all\n runs-on: ubuntu-latest'}" + invalid_access="${valid_fixture/$'permissions:\n contents: read'/$'permissions:\n contents: readonly'}" + duplicate_block="${valid_fixture/$'permissions:\n contents: read'/$'permissions:\n contents: read\npermissions:\n contents: read'}" + local fixture_count=0 - run_fixture "${fixture_dir}" "complete" "pass" "" "${valid_fixture}" || failed=1 + run_one() { + fixture_count=$((fixture_count + 1)) + run_fixture "$@" || failed=1 + } + + run_one "${fixture_dir}" "complete" "pass" "" "${valid_fixture}"Convert the remaining calls to
run_onein the same way, add the two new cases, then report the counter:+ run_one "${fixture_dir}" "invalid-access" "fail" \ + "Invalid permission entry" "${invalid_access}" + run_one "${fixture_dir}" "duplicate-workflow-block" "fail" \ + "Expected exactly one workflow permissions block" "${duplicate_block}" if (( failed )); then return 1 fi - printf 'Checked 8 Core CI permission fixtures.\n' + printf 'Checked %d Core CI permission fixtures.\n' "${fixture_count}"Declare
invalid_access,duplicate_block, andfixture_countbeside the other locals at lines 322-331.🤖 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 `@scripts/check-core-ci-permissions.sh` around lines 362 - 381, Add fixtures covering the invalid permission access path (for example, contents: readonly) and duplicate permissions blocks for one owner, using the existing run_one fixture pattern. In the fixture setup function, declare invalid_access, duplicate_block, and fixture_count with the other locals, convert remaining run_one calls consistently, increment fixture_count for each executed fixture, and replace the hardcoded 8 in the final summary with that counter.Source: Path instructions
93-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestrict misplaced-permissions detection to YAML permission mappings.
The condition at line 149 runs for every nested
permissions:line under the current job. Apermissions:line in arun: |block or nested step mapping can therefore produce a falsemisplacederror and fail CI. Track block-scalar state or parse the YAML structurally with a parser available on the runner.🤖 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 `@scripts/check-core-ci-permissions.sh` around lines 93 - 163, Update the AWK parsing logic around the misplaced-permissions check to recognize YAML block-scalar content and nested step mappings, so only actual job-level YAML permission mappings are reported as misplaced. Track and clear block-scalar state based on indentation (or use an available YAML parser), and ensure literal text such as permissions: inside run: | is ignored while genuine nested permission mappings still emit misplaced.Source: Path instructions
🤖 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 `@scripts/check-core-ci-permissions.sh`:
- Around line 333-334: Update the cleanup trap associated with fixture_dir to
use recursive forced removal of the temporary directory instead of separately
deleting YAML files and calling rmdir. Keep cleanup unconditional and
independent of the directory’s contents.
- Around line 362-381: Add fixtures covering the invalid permission access path
(for example, contents: readonly) and duplicate permissions blocks for one
owner, using the existing run_one fixture pattern. In the fixture setup
function, declare invalid_access, duplicate_block, and fixture_count with the
other locals, convert remaining run_one calls consistently, increment
fixture_count for each executed fixture, and replace the hardcoded 8 in the
final summary with that counter.
- Around line 93-163: Update the AWK parsing logic around the
misplaced-permissions check to recognize YAML block-scalar content and nested
step mappings, so only actual job-level YAML permission mappings are reported as
misplaced. Track and clear block-scalar state based on indentation (or use an
available YAML parser), and ensure literal text such as permissions: inside run:
| is ignored while genuine nested permission mappings still emit misplaced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fa0bc707-9f67-4431-969e-046d62f49ea0
📒 Files selected for processing (2)
.github/workflows/ci.yamlscripts/check-core-ci-permissions.sh
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/check-core-ci-permissions.sh (1)
366-389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider pinning the over-indented entries case with a fixture.
The suite covers an over-indented
permissions:key throughoff_indent. It does not cover the inverse shape: a correctly placedpermissions:key at indent 4 whose entries sit at indent 8.The parser records entries only at indent 6, at line 165. For that shape it emits the
blocksrecord but nopermissionrecords. The check still fails, because lines 256-261 find an empty permission set for a reviewed job, and lines 272-280 reject the block on an unreviewed job. The behavior is fail-closed today. A fixture would lock that guarantee against future parser edits.♻️ Proposed additional fixture
local job_write_all + local over_indent_entries local trailing_root_permissionsjob_write_all="${valid_fixture/$' ordinary:\n runs-on: ubuntu-latest'/$' ordinary:\n permissions: write-all\n runs-on: ubuntu-latest'}" + over_indent_entries="${valid_fixture/$' pull-requests: read'/$' pull-requests: read'}" trailing_root_permissions="${valid_fixture/$'permissions:\n contents: read\n'/}"run_fixture "${fixture_dir}" "job-write-all" "fail" \ "Inline permissions value write-all on ordinary is not allowed" "${job_write_all}" || failed=1 + run_fixture "${fixture_dir}" "over-indent-entries" "fail" \ + "Permissions for job lint-police must be" "${over_indent_entries}" || failed=1Update the count at line 395 to match the new total.
🤖 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 `@scripts/check-core-ci-permissions.sh` around lines 366 - 389, Add a fixture in the permission-check test setup for a job with a correctly indented permissions key but over-indented permission entries, and assert the existing fail-closed behavior with the appropriate error outcome. Register the new fixture with run_fixture alongside the other indentation cases, and update the expected fixture count near the test summary to include it.
🤖 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 `@scripts/check-core-ci-permissions.sh`:
- Around line 366-389: Add a fixture in the permission-check test setup for a
job with a correctly indented permissions key but over-indented permission
entries, and assert the existing fail-closed behavior with the appropriate error
outcome. Register the new fixture with run_fixture alongside the other
indentation cases, and update the expected fixture count near the test summary
to include it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2c2e55b5-dbef-430f-a9fa-23fecb5da2b6
📒 Files selected for processing (2)
.github/workflows/ci.yamlscripts/check-core-ci-permissions.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yaml
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
scripts/check-core-ci-permissions.sh (3)
371-372: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the positive fixtures against a silent substitution miss.
${var/pattern/replacement}returns the original string when the pattern misses. For thefailfixtures this is self-detecting, because the unchangedvalid_fixturepasses and the assertion expects a failure.The
passfixtures have no such protection. If a future edit tovalid_fixturechanges this indentation,quoted_jobssilently degrades tovalid_fixture. The test still passes, and the quoted-job-key paths at lines 78-79 stop being exercised.Both substitutions match today. Add an assertion so they keep matching.
♻️ Proposed guard
quoted_jobs="${valid_fixture/$' lint-police:'/$' "lint-police":'}" quoted_jobs="${quoted_jobs/$' security-codeql-scan:'/$' \047security-codeql-scan\047:'}" + if [[ "${quoted_jobs}" == "${valid_fixture}" ]]; then + printf 'Fixture quoted-job-keys did not diverge from the valid fixture.\n' >&2 + return 1 + fiApply the same check to
step_input_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 `@scripts/check-core-ci-permissions.sh` around lines 371 - 372, Add assertions in the positive fixture setup around quoted_jobs and step_input_permissions to verify each parameter substitution actually changed the source string before running the existing assertions. Fail clearly when the expected indentation pattern is absent, while preserving the current quoted-key transformations and test flow.
396-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a fixture for a workflow-wide
writein mapping form.The
workflow-write-allfixture replaces the whole block withpermissions: write-all. That is the inline form, so it exits through theinlinebranch at line 219. It never reaches the workflow-wide write check at lines 210-212.The remaining write fixtures,
unreviewed-job-writeandbroadened-exception, both exercise the job branch at line 213.The result is that lines 210-212 have no fixture coverage. That branch enforces the primary rule of this PR: no write scope at workflow level. Add a fixture in mapping form.
♻️ Proposed fixture
Add the fixture string beside the others:
invalid_access="${valid_fixture/$'permissions:\n contents: read'/$'permissions:\n contents: readonly'}" + workflow_mapping_write="${valid_fixture/$'permissions:\n contents: read'/$'permissions:\n contents: write'}"Declare it with the other locals:
local workflow_write + local workflow_mapping_writeRegister the assertion:
run_counted_fixture "${fixture_dir}" "workflow-write-all" "fail" \ "Inline permissions value write-all on workflow is not allowed" "${workflow_write}" + run_counted_fixture "${fixture_dir}" "workflow-mapping-write" "fail" \ + "Workflow-wide write permission contents:write is not allowed" "${workflow_mapping_write}"Update the count in the PR description from fifteen to sixteen fixtures.
🤖 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 `@scripts/check-core-ci-permissions.sh` around lines 396 - 397, Add a separate workflow-wide mapping-form write fixture alongside the existing fixture strings and declare it with the other locals. Register it with run_counted_fixture to exercise the workflow-level write validation branch, distinct from the inline workflow-write-all fixture, and update the documented fixture count from fifteen to sixteen.
194-233: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReserve the
workflowowner key so a job name cannot alias it.The awk stage emits the literal owner
workflowfor the root block. A job namedworkflowin.github/workflows/ci.yamlwould emit the same owner. The shell then merges both intoactual_permissions[workflow]andpermission_blocks[workflow], and theblocksrecord for the job overwrites the root count.I traced every variant of this collision. Each one still fails the gate, because the merged value stops matching
contents=read. The gate therefore remains fail-closed. The defect is diagnostic quality, not a bypass: the reported error names the workflow block while the real cause is the job.Use a sentinel that a YAML job key cannot produce. A leading colon is not valid in the job-key regex
^[A-Za-z_][A-Za-z0-9_-]*$.♻️ Proposed sentinel owner key
In the awk program, replace the three
workflowowner literals:- printf "inline\tworkflow\t%s\n", remainder + printf "inline\t:root\t%s\n", remainder- emit_permission("workflow", content) + emit_permission(":root", content)- printf "blocks\tworkflow\t%d\n", root_blocks + printf "blocks\t:root\t%d\n", root_blocksThen update the shell side:
- if [[ "${owner}" == "workflow" ]]; then + if [[ "${owner}" == ":root" ]]; then- if [[ "${permission_blocks[workflow]:-0}" != "1" ]]; then + if [[ "${permission_blocks[:root]:-0}" != "1" ]]; then- actual="$(normalize_permissions "${actual_permissions[workflow]:-}")" + actual="$(normalize_permissions "${actual_permissions[:root]:-}")"- if [[ "${job}" == "workflow" ]]; then + if [[ "${job}" == ":root" ]]; thenAdjust the user-facing message text so it still reads
on workflow.🤖 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 `@scripts/check-core-ci-permissions.sh` around lines 194 - 233, Reserve a sentinel owner key beginning with a colon for the root workflow block in the awk logic within extract_permission_records, replacing all three workflow owner literals. Update the shell-side comparisons and permission_blocks handling to use that sentinel, preventing a job named workflow from colliding with root records. Keep user-facing diagnostics rendering the sentinel as “workflow”..github/workflows/ci.yaml (1)
1518-1523: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe scopes are correct and minimal. Consider making a scope failure diagnose itself.
Both scopes are justified. The API call at line 1547 reads
.base.reffrom the pulls endpoint, which requirespull-requestson a private repository. The checkout and the latergit fetchrequirecontents. The merge at line 1553 is--no-commit --no-ffand purely local, so the comment is accurate: the job never edits the pull request.One rollout risk follows from the new declaration. This job now depends on an explicitly declared scope instead of an inherited default. If that scope is ever wrong,
curl -sfexits non-zero and prints nothing, andjqstill succeeds, so the pipeline status isjq's.TARGETbecomesnull, and the failure surfaces atgit fetchas a missing ref rather than as a permission error.A
pipefailand an explicit guard would name the real cause.♻️ Proposed diagnostic guard for the merge-validation step
run: | + set -o pipefail git config --global --add safe.directory "$GITHUB_WORKSPACE" PR_NUMBER="${GITHUB_REF_NAME##pull-request/}" TARGET=$(curl -sf \ -H "Authorization: Bearer $GITHUB_TOKEN" \ -H "Accept: application/vnd.github+json" \ "https://api.github.com/repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER" \ | jq -r '.base.ref') + if [[ -z "${TARGET}" || "${TARGET}" == "null" ]]; then + echo "Could not resolve the base ref for PR ${PR_NUMBER}." >&2 + echo "Confirm the lint-police job still declares 'pull-requests: read'." >&2 + exit 1 + fi git fetch --no-tags origin "${TARGET}:refs/remotes/origin/${TARGET}"🤖 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 around lines 1518 - 1523, Update the lint-police workflow’s merge-validation step to enable pipefail and explicitly validate the PR API response before using TARGET. Ensure curl failures or a null/missing base ref fail immediately with a clear permission/API diagnostic, while preserving the existing contents and pull-requests permission scopes.
🤖 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/workflows/ci.yaml:
- Around line 1518-1523: Update the lint-police workflow’s merge-validation step
to enable pipefail and explicitly validate the PR API response before using
TARGET. Ensure curl failures or a null/missing base ref fail immediately with a
clear permission/API diagnostic, while preserving the existing contents and
pull-requests permission scopes.
In `@scripts/check-core-ci-permissions.sh`:
- Around line 371-372: Add assertions in the positive fixture setup around
quoted_jobs and step_input_permissions to verify each parameter substitution
actually changed the source string before running the existing assertions. Fail
clearly when the expected indentation pattern is absent, while preserving the
current quoted-key transformations and test flow.
- Around line 396-397: Add a separate workflow-wide mapping-form write fixture
alongside the existing fixture strings and declare it with the other locals.
Register it with run_counted_fixture to exercise the workflow-level write
validation branch, distinct from the inline workflow-write-all fixture, and
update the documented fixture count from fifteen to sixteen.
- Around line 194-233: Reserve a sentinel owner key beginning with a colon for
the root workflow block in the awk logic within extract_permission_records,
replacing all three workflow owner literals. Update the shell-side comparisons
and permission_blocks handling to use that sentinel, preventing a job named
workflow from colliding with root records. Keep user-facing diagnostics
rendering the sentinel as “workflow”.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e0505d5a-8649-4f8c-8fb0-bdf2de990be4
📒 Files selected for processing (2)
.github/workflows/ci.yamlscripts/check-core-ci-permissions.sh
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@scripts/check-core-ci-permissions.sh`:
- Around line 179-184: Reserve the job name workflow in the validator before
permission records are collected, rejecting workflows that define a job with
that name so it cannot overwrite the workflow-level entry in permission_blocks
or actual_permissions. Add the corresponding negative fixture alongside the
existing negative cases to verify the validator fails closed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4c3f153f-ee34-4e7e-94d2-c4ab2a319543
📒 Files selected for processing (2)
.github/workflows/ci.yamlscripts/check-core-ci-permissions.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yaml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/check-core-ci-permissions.sh (1)
179-184: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftSecurity Misconfiguration (CWE-1188): Insecure Default Initialization of Resource
Reachability: External
The workflow-level namespace collision remains unresolved.
The
ENDblock still emits the root count asblocks\tworkflow\t<root_blocks>, and the job loop emitsblocks\t<job>\t<count>afterwards. Line 210 and line 216 store both record kinds in the same arrays, so a job namedworkflowoverwrites the workflow-level entry and the baseline check at line 244 passes silently.Reserve the name in
validate_permissions, or emit workflow-level records under a sentinel owner that no YAML job key can produce.🤖 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 `@scripts/check-core-ci-permissions.sh` around lines 179 - 184, Prevent workflow-level records from colliding with job records in validate_permissions by reserving the workflow owner name or emitting root counts under a sentinel owner that YAML job keys cannot produce. Update the END block’s root and job output consistently, and ensure the baseline checks use the same non-colliding key.
🧹 Nitpick comments (2)
.github/workflows/ci.yaml (1)
1548-1558: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the GitHub API call with a timeout and a retry.
curl -sfhas no time limit here. Ifapi.github.comstalls, the request blocks until the job timeout and holds alinux-amd64-cpu16runner. A transient 5xx or a connection reset also fails the whole lint job, although the operation is a read and is safe to retry.Add
--max-time,--connect-timeout, and a bounded retry. The change keeps the existingpipefailguard and the error branches intact.♻️ Proposed change
if ! TARGET="$( curl -sf \ + --connect-timeout 10 \ + --max-time 30 \ + --retry 3 \ + --retry-connrefused \ -H "Authorization: Bearer $GITHUB_TOKEN" \ -H "Accept: application/vnd.github+json" \ "https://api.github.com/repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER" \ | jq -r '.base.ref' )"; then🤖 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 around lines 1548 - 1558, Update the curl invocation in the PR base-ref lookup to include bounded connect and total-request timeouts plus a finite retry policy for transient failures. Preserve the existing pipefail guard, authorization and Accept headers, jq extraction, and both error branches unchanged.scripts/check-core-ci-permissions.sh (1)
378-407: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert every fixture substitution, not three of them.
Lines 384-387, 389-392, and 404-407 verify that the substitution changed the string. The other thirteen derived fixtures do not. If a pattern stops matching after an edit to
valid_fixture, those fixtures silently degrade to a copy ofvalid_fixture. The suite still fails, because each of them expectsfail, but the reported reason names the wrong defect.Extract one helper that performs the substitution and asserts the change. It removes the duplicated guard blocks and makes the remaining cases self-checking.
♻️ Proposed helper
+ substitute_fixture() { + local __out="$1" source="$2" pattern="$3" replacement="$4" + local result="${source/"${pattern}"/"${replacement}"}" + if [[ "${result}" == "${source}" ]]; then + printf 'Fixture substitution for %s did not apply.\n' "${__out}" >&2 + return 1 + fi + printf -v "${__out}" '%s' "${result}" + }Then replace each direct expansion, for example:
- broadened_exception="${valid_fixture/$' pull-requests: read'/$' pull-requests: write'}" + substitute_fixture broadened_exception "${valid_fixture}" \ + $' pull-requests: read' $' pull-requests: write' || failed=1🤖 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 `@scripts/check-core-ci-permissions.sh` around lines 378 - 407, Extract a helper for fixture substitutions that applies the replacement and fails when the resulting value is unchanged. Use this helper for every derived fixture in the surrounding test setup, including the existing guarded substitutions, removing their duplicated manual checks; ensure all substitution patterns remain self-validating.
🤖 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.
Duplicate comments:
In `@scripts/check-core-ci-permissions.sh`:
- Around line 179-184: Prevent workflow-level records from colliding with job
records in validate_permissions by reserving the workflow owner name or emitting
root counts under a sentinel owner that YAML job keys cannot produce. Update the
END block’s root and job output consistently, and ensure the baseline checks use
the same non-colliding key.
---
Nitpick comments:
In @.github/workflows/ci.yaml:
- Around line 1548-1558: Update the curl invocation in the PR base-ref lookup to
include bounded connect and total-request timeouts plus a finite retry policy
for transient failures. Preserve the existing pipefail guard, authorization and
Accept headers, jq extraction, and both error branches unchanged.
In `@scripts/check-core-ci-permissions.sh`:
- Around line 378-407: Extract a helper for fixture substitutions that applies
the replacement and fails when the resulting value is unchanged. Use this helper
for every derived fixture in the surrounding test setup, including the existing
guarded substitutions, removing their duplicated manual checks; ensure all
substitution patterns remain self-validating.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 76d5b105-472a-4bcc-b7e3-c12509179d85
📒 Files selected for processing (2)
.github/workflows/ci.yamlscripts/check-core-ci-permissions.sh
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
🐇
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/check-core-ci-permissions.sh (1)
205-251: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPropagate the parser exit status.
Process substitution discards the exit status of
extract_permission_records.set -o pipefaildoes not apply to< <(...). Ifawkaborts, the loop reads no records and the run reports "No Core CI jobs found in ...", which points at the workflow instead of the parser.Capture the records first, then feed the loop.
As per path instructions: "Review shell scripts for quoting, strict-mode assumptions, error propagation, portability, idempotency, and secret-safe logging."♻️ Proposed refactor: fail on parser errors
+ local records + + if ! records="$(extract_permission_records "${workflow_path}")"; then + printf 'Failed to parse permissions from %s.\n' "${workflow_path}" >&2 + return 1 + fi + while IFS=$'\t' read -r record owner scope access; do- done < <(extract_permission_records "${workflow_path}") + done <<< "${records}"Declare
recordswith the other locals at lines 191-203.🤖 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 `@scripts/check-core-ci-permissions.sh` around lines 205 - 251, Update the permission-record processing around extract_permission_records to capture its output in a records variable before iterating, and explicitly propagate a nonzero parser exit status. Feed the loop from the captured records rather than process substitution, while preserving the existing record handling and diagnostics.Source: Path instructions
🤖 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.
Inline comments:
In `@scripts/check-core-ci-permissions.sh`:
- Around line 137-150: Update the jobs parsing logic around is_job_key so every
indent-2 key under jobs is recorded, including unrecognized or flow-style job
mappings, instead of being silently skipped. Add the corresponding diagnostic
handling to reject that record, and add the flow_style_job negative fixture with
its declaration alongside the existing fixture locals to verify validation fails
closed.
---
Nitpick comments:
In `@scripts/check-core-ci-permissions.sh`:
- Around line 205-251: Update the permission-record processing around
extract_permission_records to capture its output in a records variable before
iterating, and explicitly propagate a nonzero parser exit status. Feed the loop
from the captured records rather than process substitution, while preserving the
existing record handling and diagnostics.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 61f2da85-4e2b-4a5d-994f-74ad38cd8819
📒 Files selected for processing (2)
.github/workflows/ci.yamlscripts/check-core-ci-permissions.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yaml
Core jobs without their own `permissions` block inherited the repository default for `GITHUB_TOKEN`, even when they only needed to read the checkout. This establishes `contents: read` for the whole workflow and keeps the two broader read-only jobs explicit. - **Expected green-run effect:** None. This changes token authority, not the commands or build work on a successful run. - **What it really buys us:** Ordinary Core jobs no longer inherit write authority they do not need, and a checked inventory makes a future permission increase visible in review. `security-codeql-scan` keeps `actions: read`, `lint-police` keeps `pull-requests: read`, and the pinned image-promotion workflow already fits under the new default. Tests added! This supports NVIDIA#4650 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/ci.yaml (1)
1546-1559: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout and a retry policy to the GitHub API call.
The
curlcall has no--max-timeand no retry. If the API stalls, the step blocks until the job timeout expires and holds alinux-amd64-cpu16runner. A transient 5xx also fails the whole lint job. Bound the call and retry idempotent read failures.Note:
shell: bashalready applies-eand-o pipefail, so line 1546 is redundant.♻️ Proposed change
run: | - set -o pipefail git config --global --add safe.directory "$GITHUB_WORKSPACE" PR_NUMBER="${GITHUB_REF_NAME##pull-request/}" if ! TARGET="$( curl -sf \ + --connect-timeout 10 --max-time 30 \ + --retry 3 --retry-delay 2 --retry-all-errors \ -H "Authorization: Bearer $GITHUB_TOKEN" \ -H "Accept: application/vnd.github+json" \ "https://api.github.com/repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER" \ | jq -r '.base.ref' )"; thenAs per path instructions: "Review shell scripts for quoting, strict-mode assumptions, error propagation, portability, idempotency, and secret-safe logging."
🤖 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 around lines 1546 - 1559, Update the curl invocation in the TARGET assignment to enforce a bounded request duration with --max-time and retry transient/idempotent read failures using curl’s retry options, including suitable retry delay behavior. Remove the redundant set -o pipefail line while preserving the existing error propagation and failure message around the GitHub API lookup.Source: Path instructions
scripts/check-core-ci-permissions.sh (1)
148-174: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider failing closed for a
permissions:key at any unexpected indent inside a job.The
job_property_seenguard evaluates only the first property line of each job. If the first property is well indented, a laterpermissions:key at an indent other than 4 or 6 matches no rule and produces no record. Example:ordinary: runs-on: ubuntu-latest permissions: contents: writeThe parser drops both lines and the validator exits 0. GitHub rejects that file as invalid YAML, so this is defense in depth rather than an exploitable bypass. A single guard keeps the parser fail-closed.
♻️ Proposed guard
if (in_jobs && current_job != "" && indent == 6 && content ~ /^permissions:/) { printf "misplaced\t%s\t%d\n", current_job, indent in_job_permissions = 0 next } + + if (in_jobs && current_job != "" && indent > 2 && indent != 4 && + content ~ /^permissions:/) { + printf "misplaced\t%s\t%d\n", current_job, indent + in_job_permissions = 0 + next + }As per path instructions: "Review scripts for shell safety, quoting, idempotency, dependency checks, error handling, and avoiding secret leakage in logs."
🤖 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 `@scripts/check-core-ci-permissions.sh` around lines 148 - 174, Update the permissions parsing logic around job_property_seen and the existing indent-4/indent-6 permissions checks so every permissions: key encountered inside a job is classified fail-closed when its indentation is not an accepted job-level indent. Emit the same misplaced record and reset in_job_permissions as appropriate for unexpected indents, rather than allowing later malformed keys to be silently ignored.Source: Path instructions
🤖 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/workflows/ci.yaml:
- Around line 1546-1559: Update the curl invocation in the TARGET assignment to
enforce a bounded request duration with --max-time and retry
transient/idempotent read failures using curl’s retry options, including
suitable retry delay behavior. Remove the redundant set -o pipefail line while
preserving the existing error propagation and failure message around the GitHub
API lookup.
In `@scripts/check-core-ci-permissions.sh`:
- Around line 148-174: Update the permissions parsing logic around
job_property_seen and the existing indent-4/indent-6 permissions checks so every
permissions: key encountered inside a job is classified fail-closed when its
indentation is not an accepted job-level indent. Emit the same misplaced record
and reset in_job_permissions as appropriate for unexpected indents, rather than
allowing later malformed keys to be silently ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d36bb9f8-84f8-48b9-88bd-fc396b5bdae3
📒 Files selected for processing (2)
.github/workflows/ci.yamlscripts/check-core-ci-permissions.sh
|
Thanks! I checked both remaining nitpicks and I'm leaving them out of this PR:
The exact-head review has no open threads. |
1 participant
Core jobs without their own
permissionsblock inherited the repository default forGITHUB_TOKEN, even when they only needed to read the checkout. This establishescontents: readfor the whole workflow and keeps the two broader read-only jobs explicit.security-codeql-scankeepsactions: readfor workflow metadata, andlint-policekeepspull-requests: readfor merge validation. TruffleHog's PR comments are disabled and that action does not upload SARIF, while the exact pinned image-promotion workflow requests onlycontents: read, so neither job needs a write exception.Related issues
This supports #4650
Type of Change
Breaking Changes
Testing
Additional Notes
The checked inventory covers all 51 current Core jobs: 49 inherit
contents: read, and two have exact, read-only exceptions. The validator exercises 19 focused fixtures, including nonstandard indentation, flow-style permission attempts, workflow-level writes, and reserved-owner collisions. External registry and publication credentials are unchanged; successfulmainand tag runs remain the compatibility proof for those event-only paths.