Skip to content

ci: Harden semantic PR check against pull_request_target risks - #2741

Merged
matthewfeickert merged 1 commit into
scikit-hep:mainfrom
matthewfeickert:ci/fix-security-issue
Aug 11, 2026
Merged

matthewfeickert merged 1 commit into
scikit-hep:mainfrom
matthewfeickert:ci/fix-security-issue

Conversation

@matthewfeickert

@matthewfeickert matthewfeickert commented Aug 11, 2026 •

Copy link
Copy Markdown
Member

Description

  • While the pull_request_target trigger is required by https://github.com/amannn/action-semantic-pull-request to validate PRs from forks with the workflow definition taken from the default branch, its elevated GITHUB_TOKEN is hardened.
    • Pin amannn/action-semantic-pull-request to the full commit SHA so a moved tag can't run unreviewed code with the elevated token.
    • Removed 'statuses: write' job permission as unused in v6+ of the action, and so use 'pull-requests: read'.
    • Set the workflow-level default permissions to deny-all ('{}'). The only job declares its own permissions block, so this only guards any future jobs added without one.
    • Note why the trigger is acceptable here (no checkout or execution of PR-controlled content) and that future revisions must not change this.
  • GitHub actions have also hardened pull_request_target in general so that it
    is no longer a direct security threat.

Assisted-by: ClaudeCode:claude-fable-5

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
ci: Harden semantic PR check against pull_request_target risks 

* While the pull_request_target trigger is required by
  amannn/action-semantic-pull-request to validate PRs from forks with the
  workflow definition taken from the default branch, its elevated
  GITHUB_TOKEN is hardened.
   -  Pin amannn/action-semantic-pull-request to the full commit SHA so a
      moved tag can't run unreviewed code with the elevated token.
   -  Removed 'statuses: write' job permission as unused in v6+ of the action,
      and so use 'pull-requests: read'.
   -  Set the workflow-level default permissions to deny-all ('{}'). The only
      job declares its own permissions block, so this only guards any
      future jobs added without one.
   -  Note why the trigger is acceptable here (no checkout or execution of
      PR-controlled content) and that future revisions must not change this.
* GitHub actions have also hardened pull_request_target in general so that it
  is no longer a direct security threat.
   - https://github.blog/changelog/2026-06-18-safer-pull_request_target-defaults-for-github-actions-checkout/

Assisted-by: ClaudeCode:claude-fable-5

Summary by CodeRabbit

Summary by CodeRabbit

  • Chores
    • Improved pull request validation workflow security.
    • Restricted workflow permissions to the minimum required access.
    • Pinned the validation action to a specific version for more predictable execution.
    • Added documentation clarifying security considerations.
    • Reduced the risk of unintended access during automated validation.

@matthewfeickert matthewfeickert self-assigned this Aug 11, 2026
@matthewfeickert matthewfeickert added the CI CI systems, GitHub Actions label Aug 11, 2026
@matthewfeickert matthewfeickert added the security Improving repository security measures label Aug 11, 2026
@github-project-automation github-project-automation Bot moved this to In progress in pyhf v0.8.0 Aug 11, 2026
@coderabbitai

coderabbitai Bot commented Aug 11, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5d411b52-2b53-4c59-be84-98df93f374c5

📥 Commits

Reviewing files that changed from the base of the PR and between 1264e41 and 1f841a7.

📒 Files selected for processing (1)
  • .github/workflows/semantic-pr-check.yml

📝 Walkthrough

Walkthrough

The semantic PR workflow documents pull_request_target security constraints, limits GitHub token permissions, removes status write access, and pins the semantic PR action to a commit.

Changes

Semantic PR workflow security

Layer / File(s) Summary
Workflow security and action pinning
.github/workflows/semantic-pr-check.yml
The workflow documents safe pull_request_target usage. It retains only pull-requests: read, removes contents: read and statuses: write, and pins the semantic PR action to commit 48f256284bd46cdaab1048c3721360e808335d50.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: kratsg, henryiii

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the pull request's main change: hardening the semantic PR check against pull_request_target risks.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 @.github/workflows/semantic-pr-check.yml:
- Around line 4-5: Update the explanatory comment in the pull_request_target
workflow to state that it uses the workflow definition from the base
repository’s default branch, replacing the inaccurate “base branch” wording
without changing the workflow behavior.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9c9d4a72-3755-488b-af49-f1440ce30b5a

📥 Commits

Reviewing files that changed from the base of the PR and between 744041a and 5c92ac9.

📒 Files selected for processing (1)
  • .github/workflows/semantic-pr-check.yml

Comment thread .github/workflows/semantic-pr-check.yml Outdated
@matthewfeickert

matthewfeickert commented Aug 11, 2026 •

Copy link
Copy Markdown
Member Author

So, while I think we all agree that pull_request_target is worth avoiding, I think(?) this is safe (enough) as pull_request_target executes the main repository's default branch's copy of the workflow, not the PR's copy. For pull_request_target when an attacker's PR fires the opened/edited/synchronize events, GitHub runs the version of semantic-pr-check.yml that lives on the main branch of https://github.com/scikit-hep/pyhf/, and the elevated GITHUB_TOKEN is issued to that definition. A possible attacker's modified copy in their PR is never executed. An attacker would need to get changes to .github/workflows/semantic-pr-check.yml merged to be able to carry out a follow up attack, but that would require a maintainer to accept those changes.

@codecov

codecov Bot commented Aug 11, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.28%. Comparing base (744041a) to head (1f841a7).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2741   +/-   ##
=======================================
  Coverage   98.28%   98.28%           
=======================================
  Files          65       65           
  Lines        4312     4312           
  Branches      467      467           
=======================================
  Hits         4238     4238           
  Misses         46       46           
  Partials       28       28           
Flag Coverage Δ
contrib 98.16% <ø> (ø)
doctest 98.28% <ø> (ø)
unittests-3.10 96.47% <ø> (ø)
unittests-3.11 96.47% <ø> (ø)
unittests-3.12 96.47% <ø> (ø)
unittests-3.13 96.47% <ø> (ø)
unittests-3.14 96.47% <ø> (ø)
unittests-3.9 96.54% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

@matthewfeickert

Copy link
Copy Markdown
Member Author

@agriyakhetarpal pointed out on the Scientific Python Discord that

for the vast majority of use cases, if I understand correctly, pull_request_target is no longer a problem since actions/checkout@v7 as v7 blocks checkouts from forks entirely for runs triggered by it (and similarly for workflow_run). The change has been (or is being?, I am not sure) backported to even as old as actions/checkout@v4. The only ways this trigger can cause trouble are if you run something weird that bypasses actions/checkout and checks out code using manual Git commands, or something else that elevates access, or suchlike. See also: zizmorcore/zizmor#2134

* While the pull_request_target trigger is required by
  amannn/action-semantic-pull-request to validate PRs from forks with the
  workflow definition taken from the default branch, its elevated
  GITHUB_TOKEN is hardened.
   -  Pin amannn/action-semantic-pull-request to the full commit SHA so a
      moved tag can't run unreviewed code with the elevated token.
   -  Removed 'statuses: write' job permission as unused in v6+ of the action,
      and so use 'pull-requests: read'.
   -  Set the workflow-level default permissions to deny-all ('{}'). The only
      job declares its own permissions block, so this only guards any
      future jobs added without one.
   -  Note why the trigger is acceptable here (no checkout or execution of
      PR-controlled content) and that future revisions must not change this.
* GitHub actions have also hardened pull_request_target in general so that it
  is no longer a direct security threat.
   - https://github.blog/changelog/2026-06-18-safer-pull_request_target-defaults-for-github-actions-checkout/

Assisted-by: ClaudeCode:claude-fable-5
@matthewfeickert
matthewfeickert merged commit c2a5516 into scikit-hep:main Aug 11, 2026
24 checks passed
@matthewfeickert
matthewfeickert deleted the ci/fix-security-issue branch August 11, 2026 13:54
@github-project-automation github-project-automation Bot moved this from In progress to Done in pyhf v0.8.0 Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI CI systems, GitHub Actions security Improving repository security measures

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant