Repository navigation
chore: stop release tag from reaching shell, sed, and manifests unvalidated - #191
Conversation
…idated
Sync Package Manifests and Post-release Verify pasted the release tag
(dispatch input or release name) into run: scripts via ${{ }}, so a tag
such as v1.2.3$(cmd) executed in a job holding CI_PUSH_TOKEN and
WINGET_GH_PAT. Bind the tag through env:, reject anything that is not
vMAJOR.MINOR.PATCH[-PRERELEASE], and use "$TAG" in gh release download.
Stop persisting CI_PUSH_TOKEN in .git/config: check out with
persist-credentials: false and hand the PAT only to the push command.
render.sh now validates the version, base URL and checksums before
splicing them into the |-delimited sed program and the Ruby/JSON/YAML
manifests, and looks checksums up by exact filename.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LE9LfkthhkrwR8PFLpAvWP
…regex Pass the CI_PUSH_TOKEN and WINGET_GH_PAT auth headers to git through GIT_CONFIG_* env for only the commands that need them, instead of on the command line or embedded in the winget-pkgs clone URL (which persisted the PAT in /tmp/winget-pkgs/.git/config). Reject empty or dot-only prerelease segments such as v1.2.3-.. that would only fail later as an invalid WinGet branch name. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LE9LfkthhkrwR8PFLpAvWP
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nit, I checked a couple of credential-scoping questions raised by this change and ruled them out: the CI_PUSH_TOKEN auth header is set with a broad http.https://github.com/.extraheader (no path), but it's only live for the single git push origin HEAD:main invocation against the already-checked-out repo, so it can't leak to another host in that command. I also confirmed the git fetch upstream master from microsoft/winget-pkgs runs outside the with_winget_auth wrapper, so WINGET_GH_PAT is never sent to upstream, only to the fork.
Extended reasoning...
The diff hardens CI_PUSH_TOKEN and WINGET_GH_PAT handling (header-based auth via GIT_CONFIG env vars instead of URL/argv embedding), adds strict tag/version/URL validation before values reach run: scripts, sed, and manifest templates, and tightens a checksum lookup to an exact match. This is security-sensitive CI credential and injection-prevention code touching release automation that can push to protected main and open a PR against microsoft/winget-pkgs. One confirmed nit (duplicated tag-validation regex across 5 jobs) was already posted inline; I additionally traced the scope of the two auth-header grants and the unauthenticated upstream fetch and found no leak, which is the fact recorded above.
Summary
Sync Package ManifestsandPost-release Verifyput the release tag (theworkflow_dispatchinput, or therelease: publishedtag name) intorun:scripts with${{ }}. Expressions are filled in before bash parses the script, so a crafted tag ran as shell. Insync-manifests.ymlthat shell runs in the job that holdsCI_PUSH_TOKEN, which can push to protectedmain, andWINGET_GH_PAT. The same tag also reachedpackaging/render.shunchecked, which puts it into a|-delimited sed program and into the Homebrew/Scoop/WinGet manifests.Changes
.github/workflows/sync-manifests.ymland.github/workflows/post-release-verify.yml(all four jobs)env:and are read as quoted"$VAR". No caller-supplied value is pasted into arun:block any more.gh release downloaduses"$TAG".^v[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z]+(\.[0-9A-Za-z]+)*)?$, or the job stops before the download and before the tag is written toGITHUB_OUTPUT..github/workflows/sync-manifests.yml(credentials)persist-credentials: falseand no longer takesCI_PUSH_TOKEN; it fetches with the defaultGITHUB_TOKEN.CI_PUSH_TOKENis set only on the commit step. git receives it as an auth header throughGIT_CONFIG_*env, for thegit push origin HEAD:maincommand only, and the header is masked.WINGET_GH_PATis no longer in the clone URL, so it isn't saved in the fork clone's.git/config. It is sent the same way, only for the clone and push to the fork.packaging/render.shMAJOR.MINOR.PATCH[-PRERELEASE], a base URL outsidehttps://[A-Za-z0-9._~/-]+, and a checksum that isn't 64 lowercase hex characters. That leaves no character that can change the sed program, the Ruby formula, or the JSON/YAML manifests.awk '$2 == f') instead of a regexgrep.Testing
v2.7.7gives a formula byte-for-byte identical to the old script's output. Av2.8.0-rc.1render passesruby -cand parses as valid JSON and YAML.v*tags match the new pattern.v0$(touch /tmp/pwned), a tag with an embedded newline,|e …sed payloads, a"breakout into Ruby, andv1.2.3-...git config --get-urlmatchconfirms that the push header applies togithub.com/doitintl/dci-cliand the WinGet header only to the fork, not tomicrosoft/winget-pkgs.workflow_dispatchof Sync Package Manifests with a bad tag such asv0$(touch /tmp/pwned)should fail at "Resolve release tag".Follow-ups outside this diff
CI_PUSH_TOKENandWINGET_GH_PATto fine-grained tokens scoped to this repo and to the winget fork.mainandv*tags, and addenvironment:torender-and-sync. Without that, a branch's own edited copy of the workflow can still read the secrets.release.ymlbuilds the scribe dispatch JSON by hand from the tag. Build it withjq --arginstead.🤖 Generated with Claude Code
https://claude.ai/code/session_01LE9LfkthhkrwR8PFLpAvWP
Generated by Claude Code