Close the PyPI-name exposure, drop the unnecessary OAuth secret, add release - #4
Merged
Conversation
…release
Three findings from a post-publication review, in descending order of how
badly they can bite.
THE PYPI NAME IS UNCLAIMED AND THE README TELLS PEOPLE TO INSTALL IT
pypi.org/pypi/da-cli/json -> 404 (free, anyone can register it)
pypi.org/pypi/dacli/json -> 200 (an unrelated project)
README.md:112 -> `pipx install da-cli`
The line was captioned "not yet available", but the command sat right
there to copy, and the name is registrable by anyone today. This package
installs a `da` console script onto PATH and handles DeviantArt OAuth
tokens in the macOS Keychain, so an impostor under that name is not a
harmless mistake — it is a credential-handling binary arriving by the
route the project's own README recommended.
The section now states plainly that the name is unregistered, that
anything currently under it is not this project, and why that matters
here specifically. It also notes the near-miss `dacli`, since a
mistyped install already lands on someone else's package.
The real fix is to claim the name, which is what release.yml is for.
CLIENT TYPE: RECOMMEND PUBLIC, NOT CONFIDENTIAL
The setup guide's recommended value was Confidential, and §3c then walks
the user through storing a client_secret. That is the wrong default for
what this tool is. DeviantArt's own registration form — visible in the
screenshot the guide tells you to match — says: "Choose Public if your
code is visible to users or runs on their device. Examples: desktop apps
... Your app will authenticate with client_id and PKCE — no secret
required."
da-cli is a desktop CLI on the user's machine, and the code already
agrees: `client_secret` is guarded at both call sites (auth.py:122, 830)
while PKCE with S256 does the actual work (auth.py:781, 828). Verified
both before changing the recommendation.
So the happy path was creating a long-lived secret that PKCE exists to
make unnecessary — expanding the credential surface of a tool whose
entire threat model is about local credential storage. Recommending
Public removes one secret at rest from every new install. Confidential
is still supported and still documented, as the aside it should be.
RELEASE WORKFLOW
Tag-triggered, publishing to PyPI via Trusted Publishing (OIDC) rather
than a stored API token — a PyPI token is valid indefinitely and lives
in repo secrets, while an OIDC token is minted per run and expires in 15
minutes. PEP 740 attestations come free: they default to on for trusted
publishing, so there is no second signing step to maintain.
`id-token: write` is scoped to the publish job alone, which does nothing
but download an artifact and upload it. Nothing else in the workflow can
reach a publishing credential.
Two guards worth calling out:
- The tag must equal `dacli.__version__`. The version is `dynamic`, so
nothing otherwise couples the two, and without the check you can ship
v0.5.0 containing a package that reports 0.4.0. Tested both ways: it
passes on v0.3.0 and fires on v0.4.0.
- The wheel is installed into a clean venv and every submodule imported
via pkgutil, plus a py.typed assertion. Every other job runs `da` out
of the checkout, where `import dacli` always resolves — which masks
packaging faults completely.
Release notes come from CHANGELOG.md rather than --generate-notes:
generated notes are a list of merged PRs, and this project has three.
That extraction had a bug I caught by testing it rather than assuming —
`##+` matched the `### Added` subsection directly beneath the heading,
collapsing the body to zero characters. Anchored to exactly two hashes it
returns 1551 characters for 0.3.0, and falls back cleanly for a tag with
no section.
The file must not be renamed: the workflow filename is part of the
identity PyPI validates the OIDC token against. Header says so.
Setup that cannot live in the repo — a PyPI pending publisher, the `pypi`
environment with a v* tag rule, and tag protection — is documented in the
workflow header, including why the environment is worth having on a solo
project (PyPI refuses a token minted outside it) and why required
reviewers are not (you would approve your own deploy, or deadlock).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL's `actions` queries flagged release.yml on the PR that introduced it — "Unpinned tag for a non-immutable Action" — and blocked the merge. The finding is correct and worth acting on rather than dismissing. release.yml is the only workflow that mints a publishing credential (`id-token: write`). A tag is mutable: whoever controls an action's repo can repoint `@v7` at new code, and that code would then run in the job that publishes to PyPI. This is not hypothetical — it is the shape of both the tj-actions and reviewdog compromises. All seven references are now pinned to a full commit SHA, each with a `# vX.Y.Z` comment. The comment is load-bearing, not decoration: Renovate disables updates for a bare SHA it cannot attribute to a version, so an uncommented pin goes stale silently and forever. With the comment, Renovate keeps it current and rewrites both together. Worth stating what this does NOT fix, since SHA-pinning is easy to over-trust: it locks the outer reference only. A composite action that internally does `uses: some/action@v1` still resolves a mutable tag the caller cannot reach. That is the reason the publish job is kept to two steps — download an artifact, upload it — and does nothing else. CI's own workflows are deliberately left on tags for now: those jobs run with `contents: read` and hold no secrets, and Renovate is configured with `helpers:pinGitHubActionDigests` to pin them once the app is installed. Pinning by hand what a bot is configured to pin correctly would just create a second source of truth.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three findings from a post-publication review, in descending order of how
badly they can bite.
THE PYPI NAME IS UNCLAIMED AND THE README TELLS PEOPLE TO INSTALL IT
pypi.org/pypi/da-cli/json -> 404 (free, anyone can register it)
pypi.org/pypi/dacli/json -> 200 (an unrelated project)
README.md:112 ->
pipx install da-cliThe line was captioned "not yet available", but the command sat right
there to copy, and the name is registrable by anyone today. This package
installs a
daconsole script onto PATH and handles DeviantArt OAuthtokens in the macOS Keychain, so an impostor under that name is not a
harmless mistake — it is a credential-handling binary arriving by the
route the project's own README recommended.
The section now states plainly that the name is unregistered, that
anything currently under it is not this project, and why that matters
here specifically. It also notes the near-miss
dacli, since amistyped install already lands on someone else's package.
The real fix is to claim the name, which is what release.yml is for.
CLIENT TYPE: RECOMMEND PUBLIC, NOT CONFIDENTIAL
The setup guide's recommended value was Confidential, and §3c then walks
the user through storing a client_secret. That is the wrong default for
what this tool is. DeviantArt's own registration form — visible in the
screenshot the guide tells you to match — says: "Choose Public if your
code is visible to users or runs on their device. Examples: desktop apps
... Your app will authenticate with client_id and PKCE — no secret
required."
da-cli is a desktop CLI on the user's machine, and the code already
agrees:
client_secretis guarded at both call sites (auth.py:122, 830)while PKCE with S256 does the actual work (auth.py:781, 828). Verified
both before changing the recommendation.
So the happy path was creating a long-lived secret that PKCE exists to
make unnecessary — expanding the credential surface of a tool whose
entire threat model is about local credential storage. Recommending
Public removes one secret at rest from every new install. Confidential
is still supported and still documented, as the aside it should be.
RELEASE WORKFLOW
Tag-triggered, publishing to PyPI via Trusted Publishing (OIDC) rather
than a stored API token — a PyPI token is valid indefinitely and lives
in repo secrets, while an OIDC token is minted per run and expires in 15
minutes. PEP 740 attestations come free: they default to on for trusted
publishing, so there is no second signing step to maintain.
id-token: writeis scoped to the publish job alone, which does nothingbut download an artifact and upload it. Nothing else in the workflow can
reach a publishing credential.
Two guards worth calling out:
dacli.__version__. The version isdynamic, sonothing otherwise couples the two, and without the check you can ship
v0.5.0 containing a package that reports 0.4.0. Tested both ways: it
passes on v0.3.0 and fires on v0.4.0.
via pkgutil, plus a py.typed assertion. Every other job runs
daoutof the checkout, where
import daclialways resolves — which maskspackaging faults completely.
Release notes come from CHANGELOG.md rather than --generate-notes:
generated notes are a list of merged PRs, and this project has three.
That extraction had a bug I caught by testing it rather than assuming —
##+matched the### Addedsubsection directly beneath the heading,collapsing the body to zero characters. Anchored to exactly two hashes it
returns 1551 characters for 0.3.0, and falls back cleanly for a tag with
no section.
The file must not be renamed: the workflow filename is part of the
identity PyPI validates the OIDC token against. Header says so.
Setup that cannot live in the repo — a PyPI pending publisher, the
pypienvironment with a v* tag rule, and tag protection — is documented in the
workflow header, including why the environment is worth having on a solo
project (PyPI refuses a token minted outside it) and why required
reviewers are not (you would approve your own deploy, or deadlock).