Skip to content

ci: resolve CodeQL code-scanning alerts in workflows - #1825

Merged
sbryngelson merged 3 commits into
masterfrom
ci/codeql-workflow-permissions
Sep 5, 2026
Merged

ci: resolve CodeQL code-scanning alerts in workflows#1825
sbryngelson merged 3 commits into
masterfrom
ci/codeql-workflow-permissions

Conversation

@sbryngelson

@sbryngelson sbryngelson commented Sep 5, 2026

Copy link
Copy Markdown
Member

Clears all 27 open code-scanning alerts. Both classes live in GitHub Actions workflows — no Fortran or toolchain source is touched.

missing-workflow-permissions — 26 alerts, medium

No workflow declared a permissions: block, so every job inherited the repository-default GITHUB_TOKEN scope. That's read-only under current settings, but it's settings-dependent and would silently become write if the org default ever changed.

I checked what each flagged job actually does with the token: none of the 26 write to the repo via GITHUB_TOKEN. The three jobs that do publish use their own secrets instead — docs.yml clones secrets.DOC_PUSH_URL, homebrew-release.yml uses secrets.TAP_REPO_TOKEN, coverage.yml uses secrets.CODECOV_TOKEN. So a workflow-level read default is sufficient everywhere:

# Least-privilege default: no job in this workflow writes to the repo.
permissions:
  contents: read

Applied to bench, cleanliness, convergence, coverage-health, coverage, docs, formatting, fp-stability, homebrew-release, homebrew, lint-toolchain, pmd, spelling, test-toolchain-compat, test. All 21 workflows now declare permissions at workflow or job level, and every file still parses as YAML.

untrusted-checkout/high — 1 alert, high (#57)

claude-code-review.yml runs privileged — pull_request_target + issue_comment, so it holds CLAUDE_CODE_OAUTH_TOKEN along with pull-requests: write and issues: write — and it fetched fork commits into that privileged workspace via git fetch origin pull/<n>/head.

That step turned out to be dead code: nothing read FETCH_HEAD, and the pr_head_ref output was written to $GITHUB_OUTPUT and never consumed. The review already sources its diff and per-file context through gh API calls that treat fork content as data rather than as checked-out code. So the step and the unused output are removed outright rather than sandboxed — the flagged pattern goes away with no behavior change.

A comment on the remaining actions/checkout records why the base repo is the only thing checked out, so the fetch doesn't get reintroduced later.

Follow-ups, not addressed here

Two things worth a separate look — neither is flagged by CodeQL and neither is fixed in this PR:

  • Prompt-injection surface remains in claude-code-review.yml. Claude reads an attacker-authored diff while running --dangerously-skip-permissions with Bash allowed, in a job holding write scopes. claude-code-action gating on the triggering actor's write access is the main protection. Since the review comment is already posted by a separate step, the job could be split — review at contents: read, publish at pull-requests: write — so the model's step holds no write token at all.
  • bench.yml's pull_request_review path may be a functional bug. Retracted — I checked and this was wrong. On pull_request_review, actions/checkout resolves to the PR merge ref (refs/remotes/pull/<n>/merge), not the base branch, so Clone - PR clones the PR as intended. Verified against run logs, and confirmed end-to-end: PR bug: m_bubbles_EL: delete non-finite bubbles in the boundary enforcement #1772's approval at 18:32:41Z triggered a bench run 4 seconds later in which all six Phoenix/Frontier jobs ran for 34–175 minutes. No bug here.

https://claude.ai/code/session_01XPqfEaUBG7ZaZVzeMWnKHd

Clears all 27 open code-scanning alerts. Both classes are in GitHub
Actions workflows; no Fortran or toolchain source is touched.

missing-workflow-permissions (26 alerts, medium)

No workflow declared a `permissions:` block, so every job inherited the
repository-default GITHUB_TOKEN scope. None of the flagged jobs write to
the repo through GITHUB_TOKEN -- the three that do publish use their own
secrets (DOC_PUSH_URL, TAP_REPO_TOKEN, CODECOV_TOKEN) -- so a
workflow-level `contents: read` default is sufficient everywhere.

untrusted-checkout/high (1 alert, high)

claude-code-review.yml runs privileged (pull_request_target and
issue_comment: it holds CLAUDE_CODE_OAUTH_TOKEN plus pull-requests and
issues write) and fetched fork commits into that workspace via
`git fetch origin pull/<n>/head`.

That step was dead code: nothing read FETCH_HEAD, and the pr_head_ref
output was written but never consumed. The review already sources its
diff and per-file context through gh API calls that treat fork content
as data rather than as checked-out code, so the step and the unused
output are removed instead of sandboxed. No behavior change.

Claude-Session: https://claude.ai/code/session_01XPqfEaUBG7ZaZVzeMWnKHd
Copilot AI lite review requested due to automatic review settings September 5, 2026 17:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Several workflows that run dorny/paths-filter@v4 will likely fail on PR events because pull-requests: read is not granted when permissions is set to only contents: read.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR addresses GitHub CodeQL code-scanning alerts in GitHub Actions workflows by explicitly declaring least-privilege GITHUB_TOKEN permissions and by removing an unsafe, unused PR-head fetch from the privileged Claude review workflow.

Changes:

  • Add explicit workflow-level permissions: contents: read defaults across CI workflows to avoid inheriting broader repo/org defaults.
  • Remove dead-code PR-head fetch (and an unused output) from the privileged claude-code-review.yml workflow, keeping fork content as “data” via gh API calls.
File summaries
File Description
.github/workflows/test.yml Adds explicit workflow-level token permissions (currently too restrictive for paths-filter; see comments).
.github/workflows/test-toolchain-compat.yml Adds explicit workflow-level token permissions.
.github/workflows/spelling.yml Adds explicit workflow-level token permissions.
.github/workflows/pmd.yml Adds explicit workflow-level token permissions.
.github/workflows/lint-toolchain.yml Adds explicit workflow-level token permissions.
.github/workflows/homebrew.yml Adds explicit workflow-level token permissions.
.github/workflows/homebrew-release.yml Adds explicit workflow-level token permissions.
.github/workflows/fp-stability.yml Adds explicit workflow-level token permissions (currently too restrictive for paths-filter; see comments).
.github/workflows/formatting.yml Adds explicit workflow-level token permissions.
.github/workflows/docs.yml Adds explicit workflow-level token permissions.
.github/workflows/coverage.yml Adds explicit workflow-level token permissions (currently too restrictive for paths-filter; see comments).
.github/workflows/coverage-health.yml Adds explicit workflow-level token permissions.
.github/workflows/convergence.yml Adds explicit workflow-level token permissions (currently too restrictive for paths-filter; see comments).
.github/workflows/cleanliness.yml Adds explicit workflow-level token permissions (currently too restrictive for paths-filter; see comments).
.github/workflows/claude-code-review.yml Removes unused PR-head resolution/fetch, avoiding untrusted checkout patterns in a privileged job.
.github/workflows/bench.yml Adds explicit workflow-level token permissions (currently too restrictive for paths-filter; see comments).
Review details
  • Files reviewed: 16/16 changed files
  • Comments generated: 6
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/workflows/bench.yml
Comment thread .github/workflows/cleanliness.yml
Comment thread .github/workflows/convergence.yml
Comment thread .github/workflows/coverage.yml
Comment thread .github/workflows/fp-stability.yml
Comment thread .github/workflows/test.yml
Declaring a `permissions:` block sets every unlisted scope to none, which
dropped `pull-requests` for the six workflows whose file-changes job runs
dorny/paths-filter. That action defaults to `token: ${{ github.token }}`
and reads the PR's changed-file list via pulls.listFiles on pull_request
events, so it needs pull-requests: read. test.yml also passes
`list-files: shell`, which depends on that list directly.

Applied to exactly the six paths-filter users: bench, cleanliness,
convergence, coverage, fp-stability, test. The other nine workflows
reference no GitHub API beyond contents and stay at contents: read.

Claude-Session: https://claude.ai/code/session_01XPqfEaUBG7ZaZVzeMWnKHd
@sbryngelson

Copy link
Copy Markdown
Member Author

Addressed in 5f4be21 — Copilot's finding was correct, and its list of six files was exactly right.

Declaring a permissions: block sets every unlisted scope to none, so contents: read alone dropped pull-requests for the six workflows whose file-changes job runs dorny/paths-filter@v4. That action defaults to token: ${{ github.token }} and reads the changed-file list via pulls.listFiles on pull_request events. test.yml additionally passes list-files: shell, which consumes that list directly. None of the six override token, so all of them take the API path.

Those six now declare:

permissions:
  contents: read
  pull-requests: read

I re-audited the other nine while I was in there, and they stay at contents: read:

  • None of the 15 modified workflows reference GITHUB_TOKEN or GH_TOKEN at all.
  • No SARIF upload, no release action, no gh write calls.
  • The two workflows that push (docs.ymlDOC_PUSH_URL, homebrew-release.ymlTAP_REPO_TOKEN) authenticate with their own secrets, so GITHUB_TOKEN scope doesn't affect them. Same for coverage.yml via CODECOV_TOKEN.
  • upload-artifact@v6 / download-artifact@v7 use ACTIONS_RUNTIME_TOKEN, not GITHUB_TOKEN, and docs.yml downloads from its own run (needs: build), so no actions: read is required.

Verified 1:1: the set of workflows using paths-filter and the set granted pull-requests: read are identical.

One note that reinforces the bench.yml follow-up in the description rather than anything in this diff: bench.yml's file-changes job also fires on pull_request_review, and paths-filter documents support for push / pull_request / pull_request_target, expecting an explicit base/ref otherwise. That's pre-existing and untouched here, but it's a second reason the review-triggered benchmark path deserves its own look.

sbryngelson added a commit that referenced this pull request Sep 5, 2026
CodeQL flagged the `nvhpc` job this PR adds for missing permissions. The
job itself is fine -- it checks out and pulls a container, nothing more --
but this branch is based on master, which has no `permissions:` block, so
every job in test.yml still inherits the repository default.

Add the workflow-level block here rather than relying on #1825 landing
first, so this PR clears the alert on its own in either merge order. The
block is byte-identical to the one #1825 adds to this file, so the two
merge without conflict (verified with git merge-tree).

Claude-Session: https://claude.ai/code/session_01XPqfEaUBG7ZaZVzeMWnKHd
Granting pull-requests: read at the workflow level handed it to every job
in these six files, including the multi-hour NVHPC container lanes and the
self-hosted Frontier and Phoenix runs, none of which touch the PR API.

Only file-changes needs it: it is the sole job running dorny/paths-filter,
it finishes in about ten seconds, and it holds no other privilege. Move the
grant there and return the workflow default to contents: read.

Job-level permissions replace the workflow default outright rather than
merging with it, so contents: read is restated alongside pull-requests:
read in the job block; a bare pull-requests: read there would have revoked
contents and broken the job's own checkout.

Claude-Session: https://claude.ai/code/session_01XPqfEaUBG7ZaZVzeMWnKHd
sbryngelson added a commit that referenced this pull request Sep 5, 2026
Mirrors the same change on #1825 so the two branches keep an identical
permissions region in test.yml and continue to merge in either order.

The nvhpc job this PR adds needs only contents: read -- it checks out and
pulls a container image, using no GitHub token -- so it now inherits
exactly that rather than a workflow-wide pull-requests grant it has no use
for. Same for the self-hosted Frontier and Phoenix lanes.

Claude-Session: https://claude.ai/code/session_01XPqfEaUBG7ZaZVzeMWnKHd
@sbryngelson

Copy link
Copy Markdown
Member Author

Tightened the scoping in 84cf49a (and mirrored on #1826 so the two files stay identical).

Previously pull-requests: read sat at the workflow level in all six paths-filter workflows, which handed it to every job in those files — the multi-hour NVHPC container lanes, the self-hosted Frontier and Phoenix runs, the benchmark jobs. None of them touch the PR API.

Only file-changes needs it: it's the sole job running dorny/paths-filter, it finishes in about ten seconds, and it holds no other privilege. So the workflow default returns to contents: read and the grant moves into the job:

jobs:
  file-changes:
    name: Detect File Changes
    permissions:
      contents: read
      pull-requests: read

The subtlety worth recording, and the reason contents: read is restated inside the job block: job-level permissions replace the workflow default outright rather than merging with it. A bare pull-requests: read there would have silently revoked contents and broken the job's own actions/checkout. That's the same failure mode as the original finding on this PR — declaring a permissions block sets every unlisted scope to none — just one level down.

Effective result across the six workflows, verified by parsing each file and resolving job-level against workflow-level:

Job Effective permissions
file-changes contents: read, pull-requests: read
every other job contents: read

Applied to bench, cleanliness, convergence, coverage, fp-stability, test. The other nine workflows in this PR have no paths-filter job and are unchanged at contents: read.

@sbryngelson
sbryngelson merged commit e9fd9c3 into master Sep 5, 2026
84 of 91 checks passed
@sbryngelson
sbryngelson deleted the ci/codeql-workflow-permissions branch September 5, 2026 20:42
@codecov

codecov Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.26%. Comparing base (22c2aae) to head (f62c5cd).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1825   +/-   ##
=======================================
  Coverage   62.26%   62.26%           
=======================================
  Files          84       84           
  Lines       21558    21558           
  Branches     3195     3195           
=======================================
  Hits        13423    13423           
  Misses       5937     5937           
  Partials     2198     2198           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants