feat: cut the action release from CI, and propose its pin as a pull request - #312
feat: cut the action release from CI, and propose its pin as a pull request#312totollygeek wants to merge 7 commits into
Conversation
|
🤖 Zuke AI review 🔎 security review —
|
| Severity | Finding | Location |
|---|---|---|
| low | New action-release job exposes GitHub App key to an additional workflow path | .github/workflows/release.yml:28 |
Dismiss a false positive
Add a finding's ID to the suppress list to hide it next time:
lpimal2fs7bp— New action-release job exposes GitHub App key to an additional workflow path
The patch mostly adds safer path handling and retry logic, with one low-risk exposure in workflow secrets for a new action-release job.
|
🤖 Zuke AI review 🔎 generic review —
|
|
Both findings, and one fix answers both — b16aba0.
Not removable, I checked rather than assumed: the job pushes a branch, and What was wrong is that the job audited egress while holding a write-scoped token. This repository does not do that anywhere else — the gate job blocks, and It now blocks against the same endpoint list. The credential still exists; what changes is that anything reading it out of the environment has nowhere to send it.
Worth stating why this shape and not another, since the finding is really about whether the privilege is justified. Master has an active ruleset requiring a pull request, whose only bypass is a repository role. The alternative to opening a PR is granting the bot that bypass so it can push to master directly — a standing hole in that protection, for a change that happens a few times a year. Opening a pull request is strictly less privilege than the alternative, and it leaves the change reviewable, which a direct push would not. The credential's blast radius is now bounded by the egress block, which is the part of this that was genuinely missing. One detail worth recording: the endpoint list is renamed, since two jobs share it and its old name described only the first. Verified it is a rename and not a change — regenerating alters exactly one line across the workflows, the new job's, and the website sync's own allowlist is byte-identical.
|
|
The narrowing is correct and sharper than the previous wording. The credential is in git config from checkout onward, and The egress block bounds exfiltration, not use. The token is a GitHub credential and What it is, then: the same trade this repository already makes on its gate job, which holds What would actually remove it, if you want that: drop I have not done it here because it is a different shape of change from wiring the job up, and because it would leave this repository inconsistent with itself in the other direction. Happy to do it as a follow-up covering both jobs, which is where it would actually pay: the gate job holds the same credential for a much longer build with a much larger dependency graph. All 10 checks pass. |
Addresses AI review finding `2y2yj4phucoli`, raised against the copy of this code in #312 and fixed here, where it belongs. The finding asked what happens if the branch name is not validated. In that repository's own call path it always is — the value is machine-generated from a parsed version and a test asserts the validator rejects junk. But this is a published package, and a library that is safe only when every caller validates is not safe. It is worse than a malformed name reaching the API. These names go into request paths, and URL normalisation resolves `..` before the request is sent: /repos/o/n/git/ref/heads/../../../user/repos -> /repos/o/n/user/repos So a branch name could silently redirect a call to a different endpoint with a write-scoped token attached. Verified before fixing, and the test asserts the normalisation as well as the refusal, so the reason survives. Every entry point now checks what it is handed, by git's own rules, and refuses before anything is sent — a request that goes out and fails has already carried the token somewhere unintended. The test asserts zero calls were made. The control-character part is a codepoint check rather than a regex: spelling those out trips `no-control-regex`, and suppressing that rule to keep a check git itself makes would be the wrong way round. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
I traced it before deciding, and then found the real version of it. In this PR's call path it is not reachable. The branch comes from But the finding is about the library function, and that is the right thing to be about. And the consequence is not a malformed name. These names go into request paths, and URL normalisation resolves
A branch name could silently redirect the call to a different endpoint, with a write-scoped token attached. That is a genuine defect in code I wrote today, and the reviewer got to it first. Fixed in #313, where the durable copy lives: every entry point validates by git's own ref rules and refuses before anything is sent — a request that goes out and fails has already carried the token somewhere unintended, so the test asserts zero calls were made. It also asserts the URL normalisation itself, so a future reader sees why the check exists rather than just that it does. I have not duplicated the guard into this PR's |
|
Marked draft. Two adversarial reviews found blockers that compound into a silent, unrecoverable half-release. Converting back is one click once these are addressed; I would rather it not be merged in the meantime. Blocker 1 — the pull request cannot work with
|
…313) * feat(core): commit to GitHub through its API, with no credential on disk CI that commits usually checks out with `persist-credentials`, commits, and pushes. That writes the token into `.git/config`, where it outlives the step that needed it: every later step in the job can read it, and anything archiving the workspace carries it out. Blocking a job's egress does not answer this — the token is a GitHub credential and GitHub is necessarily reachable, so an allowlist bounds where it could be sent, not what it could do. Git's data API takes file contents inline, so a commit can be built server-side and a ref pointed at it. The token becomes a request header and nothing more. Three operations, because they are three different intents: commit onto an existing branch, create a branch and commit onto that, and tag a commit. The ref update is deliberately unforced for the first — a commit landing between reading the head and writing it is rejected rather than overwritten — and deliberately forced for a moving major tag, which is a non-fast-forward by definition. This does not claim the token is unreadable by the step that uses it. Nothing short of not having a token achieves that; what is removed is the credential's persistence beyond its use. Two supporting changes. `HttpOptions` gains `method` and `body`, since the helpers were GET-only. And `HttpError` now carries the start of the response body: a status alone rarely says which field or which permission was the problem, and this runs unattended, so the log is all anyone will have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(core): validate ref names before they reach a request path Addresses AI review finding `2y2yj4phucoli`, raised against the copy of this code in #312 and fixed here, where it belongs. The finding asked what happens if the branch name is not validated. In that repository's own call path it always is — the value is machine-generated from a parsed version and a test asserts the validator rejects junk. But this is a published package, and a library that is safe only when every caller validates is not safe. It is worse than a malformed name reaching the API. These names go into request paths, and URL normalisation resolves `..` before the request is sent: /repos/o/n/git/ref/heads/../../../user/repos -> /repos/o/n/user/repos So a branch name could silently redirect a call to a different endpoint with a write-scoped token attached. Verified before fixing, and the test asserts the normalisation as well as the refusal, so the reason survives. Every entry point now checks what it is handed, by git's own rules, and refuses before anything is sent — a request that goes out and fails has already carried the token somewhere unintended. The test asserts zero calls were made. The control-character part is a codepoint check rather than a regex: spelling those out trips `no-control-regex`, and suppressing that rule to keep a check git itself makes would be the wrong way round. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(gh): commit and tag through the API, with no credential on disk Moved from `@zuke/core`, where I put it first and should not have. The reasoning I gave for core was that `@zuke/ai` has no HTTP and would need a new dependency to reach it. That was simply wrong: `@zuke/ai` has had a `src/hosts/` directory of GitHub, GitLab, Azure and Bitbucket API clients all along, because the reviewer posts comments to whichever host it runs on. With that premise gone, the placement argument is one-sided. `@zuke/gh` is the GitHub package and already makes direct REST calls with a `fetch` seam — `uploadSarif` is the same shape as this. Core is the build engine: targets, graph, shell, CI generation. GitHub endpoints are not that. The move also fixes a guideline the core version broke. A package exposes its operations through a namespaced `*Tasks` object, configured by a settings lambda — not loose exported functions taking an options bag. So this is `GhTasks.commit((s) => s.branch(...).message(...).file(...))` and `GhTasks.tag((s) => s.name(...).move())`, with `owner/repo` and the token falling back to the Actions environment the way `uploadSarif` already does. What stays in core is what belongs there: `HttpOptions` gaining `method` and `body`, since the helpers were GET-only, and `HttpError` carrying the start of the response body. Both are generic HTTP, and the second is what makes a failed call diagnosable — a status alone rarely says which field or which permission was the problem, and this runs unattended. Ref-name validation comes along unchanged, including the test asserting that a `..` in a branch name would otherwise redirect the request to a different endpoint with the token attached, and that nothing is sent when one is refused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(gh): make the commit module self-contained, as its sibling already is The core floor check caught what a local run cannot: `@zuke/gh` declares `jsr:@zuke/core@^1.31.0`, and the `method` option this reached for on core's HTTP helper does not exist there. Raising that floor would have needed a core release first — the sequencing this repository has now hit three times. There was no need for any of it. `uploadSarif`, the sibling REST operation in this package, calls its own `fetch` seam directly and builds its own error. This now does the same, so the package needs nothing new from core and its floor stays where it is. The core changes go with it. `method` and `body` on `HttpOptions`, and the response body on `HttpError`, were only ever there to serve this — and with the module self-contained, adding to a published API for no remaining caller would be worse than not having. `@zuke/core` is byte-identical to master again. The error this builds says as much as the one core would have: the method, the path, the status, and GitHub's own message, which is the half that names the field or the permission. The token is not in it — it never leaves the header. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(gh): encode ref path segments, and stop a bare catch hiding an auth failure Two findings from the adversarial pass, both against code added in this branch. The ref validator was bypassable. It rejects a literal `..`, but `%` is legal in a git ref, so `%2e%2e` passed it — and the URL parser decodes that to a double-dot segment and resolves it, which is precisely the redirection the validator exists to prevent. Reproduced end to end: a branch name of `%2e%2e/%2e%2e/user/repos` sent a token-bearing GET and PATCH to a path outside the repository the caller named. Validation was the wrong instrument. Each slash-separated segment is now percent-encoded, which makes `%2e%2e` an ordinary segment name and covers the repository slug too — interpolated into the same path and never validated at all. Segment-wise rather than wholesale, because a slash inside a branch name is meaningful and `chore/action-v1.0.3` must stay three segments. The validator stays as well: a name git itself would refuse deserves the clearer error. The second: moving a tag recovered from a missing ref with a bare `catch`, which also swallowed 401 and 403. An expired token or a missing permission was retried as a create and reported as a ref conflict — the wrong error, hiding the one that mattered, in a path that runs unattended. Failures now carry their status and only a missing ref is recovered from. Also fixed a stray import block that an editing mistake had pasted inside this module's own documentation example. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(gh): check the API response rather than asserting its shape Addresses AI review finding `9d0nf96jpjg9`. The caller was generic over a response type and ended in a type assertion, which this repository forbids — and the ban is right here rather than merely stylistic. Asserting the shape meant a field that GitHub renamed, or omitted on some error path, would flow onward as `undefined`: into a request path, or into a tag ref, with nothing saying which call had returned something unexpected. It now returns `unknown`, and each field is read through a checked accessor that names the call it came from. A missing `sha` is an error that says which response lacked it, not a request to `/git/commits/undefined`. The last assertion, indexing a value after a `typeof` check, is a type guard now. That is what the guideline means by narrowing rather than forcing, and it is what makes the accessor safe rather than merely quiet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(gh): refuse a repository slug that is not owner/name Addresses AI review finding tunixxmwrzt7. The slug was percent-encoded but never validated, unlike the ref names beside it. Encoding is not the missing piece here: it already stops the slug climbing out of /repos/, since a dot segment survives as a literal name rather than resolving. What it does not stop is a slug carrying the wrong number of segments, which quietly changes which endpoint is called and sends a token-bearing request somewhere the caller never named. The check lives in the caller rather than in each settings class, because every request routes through it, so a check anywhere else could be one path short. Also moves a doc block back onto the function it describes, after an earlier edit left it attached to its neighbour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(gh): name the call when a 2xx body is not JSON Addresses AI review finding 3sxiscnkq8auw. The error path never parsed a body, so a non-JSON failure was already safe, but the success path parsed bare: a 2xx carrying HTML, which is a proxy or gateway answering instead of GitHub, surfaced as a SyntaxError naming no call. The wrapper is deliberately not a GhApiError. That type means GitHub refused, and tagCommit reads its status to decide whether a missing ref should be created instead. A parse failure is neither of those, and entering that branch would retry an unrelated failure as a tag create. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…equest `actionRelease` existed as a target nobody ran. Today's release happened because I typed it; the next one happens if someone remembers, and if nobody does, `v1` quietly stops tracking `action.yml` — which is the failure the whole pin arrangement exists to prevent. A reviewer flagged the missing wiring when the target was written and it was never added. The release workflow now runs it after release-please. What it can do unattended, it does: the tag is cut and `v1` is moved, so consumers have the fix without waiting for anyone. Tags are not covered by the ruleset on master, which is why that half works. The other half cannot be pushed. Master requires a pull request, and the only bypass is a repository role — so a token that could push directly would be a standing hole in that protection for a change that happens a few times a year. The job opens a pull request instead, carrying the pin and the workflows regenerated from it, the way the website sync already proposes its changes. The repository catching up with its own release can wait for a review; nobody is blocked while it does. Locally the behaviour is unchanged: the files are written and the human commits them after looking. `isCI()` is the only difference, because locally there is someone to look. The commit is a `chore:`, since this changes no package and release-please parses every merged subject — a `feat:` here would cut a package release for a commit that touched none. The body carries no fenced block, for the same reason `prBodyLint` exists. Both are asserted rather than left to habit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses AI review findings `2eyo60uhydy5t` and `3dnk9v0tnnexy`. `persist-credentials` is not removable here: the job pushes a branch, and the push needs a credential in git config to do it. What was wrong is that the job audited egress while holding one, which is not what this repository does with token-holding jobs. The gate blocks, and the website sync — the job this one is modelled on, pushing a branch and opening a pull request with a token — blocks too. This one now does the same, against the same endpoint list. That is the answer to both findings rather than one. The credential still has to exist; what changes is that anything reading it out of the environment has nowhere to send it. The endpoint list is renamed, since two jobs now share it and its old name described only the first. Verified the rename is a rename: regenerating changes one line in the workflows, the new job's, and the website sync's own allowlist is byte-identical to what it was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses AI review finding `26topmdip8rk2`. The previous shape checked out with `persist-credentials`, committed, and pushed. That writes the token into `.git/config`, where it outlives the step that needed it: every later step in the job can read it, and anything that archives the workspace carries it out. Blocking egress bounded where the token could be *sent* and not what it could *do* — it is a GitHub credential and GitHub is necessarily reachable — so the block was never the answer to this. The tags and the pull request are now built through git's data API. A tree takes file contents inline, so the commit is made server-side and there is nothing to push; the tags are a tag object and a ref. The token is a request header and nothing else, and the checkout no longer persists one. What this does not claim: the token is still readable by code running in the step that uses it. Nothing short of not having a token avoids that. What is gone is its persistence beyond that step, which was the whole of the finding. Local behaviour is deliberately unchanged. There is no token, and a developer cutting a release wants the tag in their own clone, so that path stays git — `isCI()` is the only difference. The tag message has to survive between `tag` and `push`, which git needs as two steps and the API takes as one; a small map holds it. And the type hierarchy was rearranged so the API options extend the ref options rather than being cast to them, since a cast is not something this repository allows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r landed Replaces build/propose_change.ts with GhTasks.commit, .tag and .pullRequest. That module was a second copy of the same transport, written before the fixes that landed in the package: it had no ref validation, no path encoding, a bare catch that reported an authorisation failure as a missing ref, and type assertions on the responses. Deleting it removes the copy rather than porting the fixes to it. Adds two operations the proposal needs. GhTasks.pullRequest opens a pull request or returns the one already open for that branch, and .replace() resets a branch a previous attempt left behind. Both exist because this job retries: without them a run that created the branch and then failed could never run again. Fixes the guard that decided whether anything was owed. It asked only whether action.yml had changed since the newest tag, which answers no once the tags are pushed — so a proposal that failed after that point was never retried, and every later run reported success while consumers ran a version this repository had no record of. It now also compares the committed pin against the newest tag, and re-proposes without re-cutting anything. Mints an app token rather than using GITHUB_TOKEN. Not a preference: there is no workflows permission to grant GITHUB_TOKEN, so it may not write the regenerated workflow files, and a pull request it opened would trigger no checks and so could never satisfy the ruleset that requires them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rkflow The job is generated and drift-checked, so this is not about the file matching the generator. It is about the properties that make it safe to run unattended — no GITHUB_TOKEN, no write permission, blocked egress, no persisted credential — which a later edit to the generator could remove while leaving everything green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adversarial review found a silent, permanent failure. A release is two tag writes with nothing binding them: vX.Y.Z is created, then v1 is moved. If the second fails, the release is half-published with v1 — the tag consumers actually use — left on the previous one, and nothing would ever notice. Once vX.Y.Z exists, changedSince compares against a tag naming that very commit and answers unchanged on every later run, so the release path is never re-entered; once the pin lands, the retry path stops too. Both report success. The old code asserted the opposite in a comment: no tag is moved here, they are already right. The pointer is now checked against the newest release on every run, before the gate that decides whether anything is owed — because the answer to that question is no in exactly the state this repairs. Repairing it needs the tag deps to name the commit they write rather than implying HEAD, since the fix points a tag at an earlier release. The pin branch was also force-reset on every push to master while its proposal was open, not once. The retry gate fires whenever the committed pin disagrees with the newest tag, which stays true until the pull request merges — so unrelated traffic would discard any commit a maintainer pushed to make it mergeable and move the head out from under a review. It now looks for an open proposal before writing anything, which is what GhTasks.findPullRequest is for. Also: the pull-request lookup filters on base as well as head, since GitHub allows several proposals open from one branch to different bases and would otherwise return one the caller never asked about; a lookup that fails no longer replaces the error that says what was refused; .replace() without .from() is refused rather than silently ignored; and the app-token mint validates and encodes the owner and repository it interpolates into a path carrying the private-key JWT. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7431fd9 to
f9db45b
Compare
Addresses AI review finding 2gbhthjuauvb6. The lookup that recovers from a 422 already filtered on head and base and verified the head against the response, but not the base — an asymmetry, since both are what make it the same proposal. Verifying one and not the other leaves open the confusion the base filter was added to close, for any case where the filter does not behave as this code assumes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Rewritten. The draft carried its own copy of the GitHub API transport — written before the fixes that landed in Four adversarial reviews ran against this branch. Two findings were serious, and both were silent. A half-published release was permanent and invisible. A release is two tag writes with nothing binding them: The pointer is now checked against the newest release on every run, ahead of the gate that decides whether anything is owed, because that gate answers "no" in precisely the state this repairs. That required the tag deps to name the commit they write rather than implying HEAD — repointing a major tag at an earlier release is not something the old signature could express, which is worth flagging as a trap for anyone calling The pin branch was force-reset on every push to master while its proposal was open, not once. The retry gate fires whenever the committed pin disagrees with the newest tag, and that stays true until the pull request merges — so unrelated traffic would have discarded any commit a maintainer pushed to make it mergeable, and moved the head out from under a review. My justification comment claimed "only this target ever writes it", which is true of the one-shot retry and false in the steady state. It now looks for an open proposal before writing anything. Smaller confirmed defects, each with a regression test: the pull-request lookup filters on base as well as head, since GitHub permits several proposals open from one branch to different bases and the duplicate 422 is raised for the pair; a failing lookup no longer replaces the error that says what was refused;
Two things a reviewer should weigh rather than take from me. The app token is not a preference —
|
|
The key now reaches a second job. That is real, and it is the same concern raised against What is bounded rather than removed: the job holds no What is not bounded, stated plainly: the mint happens inside build code running under If that is not an acceptable widening, the shape that avoids it is a separate workflow triggered on the tag push, holding the key in a job that runs no build code. Worth deciding deliberately either way; I have not built it, because it trades this exposure for a second automation surface and that is a judgement about the repository rather than about the code. All checks pass. The remaining |
1 participant
Cuts the Marketplace action's release from CI and proposes its pin as a pull request, so the only manual step left is the Marketplace tick GitHub gates behind a browser 2FA confirmation.
Why a pull request rather than a push
The ruleset on master requires one. The tags are the half that reaches consumers and they are pushed directly; the pin —
build/action_version.jsonplus the workflows regenerated from it — is this repository catching up with its own release, and it can wait for review without anyone being stuck.The credential
This job mints a GitHub App installation token rather than using
GITHUB_TOKEN. That is not a preference. There is noworkflowspermission to grantGITHUB_TOKEN, so it may not write the regenerated.github/workflows/*, and a pull request it opened would trigger no workflows — so the required checks could never run and the proposal could never be merged. The job itself is grantedcontents: readand holds noGITHUB_TOKENat all.Every write goes through the REST API, so nothing needs a credential in
.git/configwhere it would outlive the step that used it. Git is still used, but only to read: the tag list, the diff, and the SHA a tag resolves to. The key is withheld from thegenerate-cisubprocess, which has no use for it.What the earlier draft got wrong
It carried its own copy of the GitHub API transport, written before the fixes that landed in
@zuke/gh— no ref validation, no path encoding, a bare catch reporting an authorisation failure as a missing ref, type assertions on responses. That module is deleted; the operations come fromGhTasks.commit,.tagand.pullRequest. The duplication is what let the two copies diverge, so the shared transport now lives in one place,packages/gh/src/api.ts.What the adversarial review found
Two defects worth naming, both silent.
A release is two tag writes with nothing binding them together. If the move of
v1failed aftervX.Y.Zwas created, the release was half-published — and nothing would ever notice, becausechangedSincethen compares against a tag naming that very commit and answers unchanged on every later run. Both paths reported success while every@v1consumer stayed on the older action, which is precisely the harm the tag exists to prevent. The pointer is now reconciled against the newest release on every run, ahead of the gate that decides whether anything is owed, because that gate says no in exactly this state.The pin branch was force-reset on every push to master while its proposal was open, not once — so unrelated traffic would discard a commit a maintainer pushed to make it mergeable, and move the head out from under a review. It now looks for an open proposal before writing anything.
Smaller ones: the pull-request lookup filters on base as well as head, since GitHub allows several proposals open from one branch to different bases; a failing lookup no longer replaces the error that says what was refused;
.replace()without.from()is refused rather than ignored; and the app-token mint validates and encodes the owner and repository it interpolates into a path that carries the private-key JWT.Known limitation
If
action.ymlchanges again while a pin proposal is still open, a second one is opened for the newer version and the older is left alone. Merging them out of order regresses the pin until the next push to master, where the retry path re-proposes the correct one. Closing the superseded proposal automatically needs an operation this does not add.Testing
./zuke cigreen, 18/18. 32 tests over the release state machine, including the half-published release, the aligned pointer, the first release of a major, and a release tag that will not resolve; 4 pinning the generated job's credential shape; and the@zuke/ghsuites covering the new pull-request operation and.replace().The
isCI()half of this target executes for the first time in production, against a tag namespace with no undo. That is worth knowing before merging.