Skip to content

Conversation

ssbarnea
Copy link
Member

No description provided.

Copy link

github-actions bot commented Nov 11, 2024

Label error. Requires exactly 1 of: breaking, build, chore, ci, docs, fix, perf, refactor, style, test. Found: upstream, feat. Follow https://www.conventionalcommits.org to get auto-labeling to work correctly.

checks: read
contents: read
id-token: write
id-token: write # release
Copy link
Member

Choose a reason for hiding this comment

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

Setting the OIDC privilege is highly discouraged because giving such elevated privileges to random scripts like transitive build dependencies, testing automation and other things can lead to privilege elevation and impersonation attacks and who knows what else. For this reason, I emphasize this in all the docs for pypi-publish that are in my reach. We even managed to update GitHub's own PyPI+OIDC doc to show that it's only acceptable in the publishing job that only downloads the dists and calls pypi-publish but nothing else.
So whenever I see id-token: write anywhere outside of the narrow scope of a single job doing this single thing, I know immediately that it's a security vulnerability.

fetch-depth: 0 # needed by setuptools-scm

- name: Build dists
run: python3 -m tox -e pkg
Copy link
Member

Choose a reason for hiding this comment

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

Following the previous comment, building within the publishing job with OIDC is a popular example of letting the possibility of being impersonated over OIDC into your life. Don't do this, it's not supported.

name: release ${{ github.event.ref }}
# if: github.ref_type == 'tag' || inputs.publish_pypi == 'true'
needs: check
environment: release # keep it here to allow users to prompt for release
Copy link
Member

Choose a reason for hiding this comment

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

I finally updated PyPI and its docs to suggest using pypi and testpypi recently. So that people would stop thinking of deployment targets (which what the environments are) as processes (which they are not). So I recommend those name in place of cryptic generic terms.

PYPI_API_TOKEN: ${{ secrets.PYPI_API_TOKEN }}
run: |
if [ -z "${PYPI_API_TOKEN}" ]; then
echo "PYPI_API_TOKEN is not set, please add it to your repository environment named 'release'."
Copy link
Member

Choose a reason for hiding this comment

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

I think the current environment name is available in the context vars somewhere, it's usually better than hardcoding.

@ssbarnea ssbarnea added the upstream Blocked due to upstream dependencies label Nov 11, 2024
@ssbarnea ssbarnea marked this pull request as draft November 11, 2024 15:25
@ssbarnea ssbarnea changed the title Add optional pypi release job inside the reusable tox workflow [WIP] Add optional pypi release job inside the reusable tox workflow Nov 11, 2024
@ssbarnea ssbarnea changed the title [WIP] Add optional pypi release job inside the reusable tox workflow feat: [WIP] Add optional pypi release job inside the reusable tox workflow Sep 2, 2025
@ssbarnea ssbarnea added the feat label Sep 2, 2025
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
6 Security Hotspots
E Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

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

Labels

feat upstream Blocked due to upstream dependencies

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants