Skip to content

[MNT] harden the PyPI release workflow against tag-name injection - #2385

Open
WAHIB-EL-KHADIRI wants to merge 2 commits into
sktime:mainfrom
WAHIB-EL-KHADIRI:mnt/harden-pypi-release-workflow
Open

[MNT] harden the PyPI release workflow against tag-name injection#2385
WAHIB-EL-KHADIRI wants to merge 2 commits into
sktime:mainfrom
WAHIB-EL-KHADIRI:mnt/harden-pypi-release-workflow

Conversation

@WAHIB-EL-KHADIRI

@WAHIB-EL-KHADIRI WAHIB-EL-KHADIRI commented Aug 19, 2026

Copy link
Copy Markdown

LLM generated content, by Claude (Anthropic) — drafted with Claude Code and reviewed by me before opening. I independently ran and verified the zizmor and PyYAML checks reported below.

Reference Issues/PRs

No open issue — found while auditing the workflows with zizmor. Touches the same file as #2376.

What does this implement/fix? Explain your changes.

1. Tag-name injection in check_tag

run: |
  TAG="${{ github.event.release.tag_name }}"

A ${{ ... }} expression is substituted as text into the script before bash parses it. A tag name containing shell metacharacters is therefore executed, not compared. Passing the value through env makes it an ordinary shell variable, which is only ever data:

env:
  RELEASE_TAG: ${{ github.event.release.tag_name }}
run: |
  TAG="${RELEASE_TAG}"

What makes this worth fixing is where the job sits rather than the job itself. check_tag gates build_wheels, which produces the artifact upload_wheels publishes to PyPI. Code executing anywhere in that chain can change what ships to users — and it runs on release: published, after a human has stopped watching.

The privilege required is a release creation, so this is not anonymous RCE; it is a way for one compromised or careless step to reach the publishing pipeline. That is exactly the class of thing worth closing in a release workflow, where the blast radius is every downstream install.

2. No permissions: block

The workflow declared none, so all four jobs inherited the repository default token scope. Added permissions: contents: read at workflow level.

upload_wheels is deliberately untouched: job-level permissions replace the default rather than extend it, so it keeps exactly its declared id-token: write for Trusted Publishing. check_tag, build_wheels and pytest-nosoftdeps only ever read the repository.

Deliberately not included

zizmor also reports 22 unpinned-uses findings across the workflows (actions/checkout@v7 and friends are tags, which are mutable). Pinning by digest is a real improvement, but it is a different change with a different trade-off — Dependabot is clearly managing these tags today (#2348, #2370) and switching to digests changes that workflow. Happy to open it separately if maintainers want it; it did not belong in a targeted fix.

Did you add any tests for the change?

No test is possible for a workflow file. Verified instead:

  • zizmor --min-severity high .github/workflows/pypi_release.yml — the template-injection finding on this file is gone (was 1, now 0). The remaining findings are all unpinned-uses, which this PR intentionally does not touch.
  • Parsed the file with PyYAML to confirm the structure is unchanged: same four jobs, upload_wheels still carrying {'id-token': 'write'}, top level now {'contents': 'read'}.
  • The change is behaviour-preserving: TAG holds the same string it did before, so the version comparison logic is untouched.

Any other comments?

PR checklist

  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG].
  • Added the relevant description of the changes.

@phoeenniixx

Copy link
Copy Markdown
Member

FYI @fkiraly

`check_tag` expanded the release tag straight into a shell body:

    TAG="${{ github.event.release.tag_name }}"

A `${{ ... }}` expression is substituted as text before bash parses the
script, so a tag name containing shell metacharacters is executed rather than
compared. Passing the value through `env` makes it an ordinary variable, which
is only ever data.

The job matters because of where it sits: `check_tag` gates `build_wheels`,
which produces the artifact `upload_wheels` publishes to PyPI. Code running in
that chain can alter what is released.

Also adds a workflow-level `permissions: contents: read`. There was no
`permissions:` block, so every job inherited the repository default. Job-level
permissions replace rather than extend the default, so `upload_wheels` keeps
exactly its declared `id-token: write` for Trusted Publishing and is
unaffected.

Verified with zizmor: the template-injection finding on this file is gone and
the YAML still parses with the same four jobs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@WAHIB-EL-KHADIRI
WAHIB-EL-KHADIRI force-pushed the mnt/harden-pypi-release-workflow branch from 37301c6 to c5dc7a2 Compare August 19, 2026 20:49
@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@ed4f27f). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2385   +/-   ##
=======================================
  Coverage        ?   88.12%           
=======================================
  Files           ?      196           
  Lines           ?    11051           
  Branches        ?        0           
=======================================
  Hits            ?     9739           
  Misses          ?     1312           
  Partials        ?        0           
Flag Coverage Δ
cpu 88.12% <ø> (?)
pytest 88.12% <ø> (?)

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.

@fkiraly fkiraly left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks reasonable, although it would be appreciated if you could remove the large amounts of AI generated comment text that makes only sense in the context of this PR but not in the file itself.

Addresses review feedback: the comments explained the reasoning behind
the change rather than the file, so they belong in the PR description.
The hardening itself is unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@WAHIB-EL-KHADIRI

Copy link
Copy Markdown
Author

Thanks @fkiraly — fair point, those comments explained the reasoning behind the change rather than the file itself. Removed both blocks in 5f37e20; the diff is now just the permissions: block and the env: indirection.

The rationale, for the record here rather than in the file: ${{ ... }} is substituted as text before bash parses the line, so a release tag containing shell metacharacters would be executed rather than compared. Passing it through env keeps it data. The permissions: contents: read default matters because upload_wheels carries id-token: write for Trusted Publishing.

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

Labels

maintenance Continuous integration, unit testing & package distribution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants