From 01b3494c611477fed8609568a204925db6008957 Mon Sep 17 00:00:00 2001 From: Robin Date: Mon, 27 Jul 2026 09:47:44 +0200 Subject: [PATCH 1/6] fix: add credential-free release-repair workflow for zero-asset releases Releases 0.6.0 and 0.7.0 published with an empty asset list: the release page exists, the run was green, and every download link 404s. There was no way to fill those holes. Re-dispatching release.yml at an old tag is not one: GitHub compiles a dispatched run from the workflow file at the dispatched ref, and tags before 0.7.1 carry no workflow_dispatch trigger at all. Worse, release.yml rewrites the live `latest` release, so running it at an old tag would drag the stable releases/download/latest/*.jar URLs backwards and silently downgrade every server owner using them. release-repair.yml dispatches from the default branch, checks the tag out itself, builds at the JDK that tag's own release.yml pinned (guessing a toolchain would produce a false red about whether a tag builds), and uploads only the assets that release is missing, under that era's own naming convention. The safety is a capability boundary, not an `if:` somebody can get wrong: the workflow holds `contents: write` and nothing else, consumes no secret beyond GITHUB_TOKEN, and never names a release other than the tag under repair - so publishing a rebuilt old jar anywhere users pull from by default is unrepresentable here rather than merely discouraged. ReleaseRepairCapabilityTest asserts that, plus: the pointer tags are refused outright, --clobber can only reach a hole and never a published good asset, and the completeness guard runs before the build rather than after it. Both verification steps assert on landed facts instead of on the steps that just ran - the same defect the release.yml guard closed. The asset check re-reads the published release from the API and range-requests a jar to prove it is served, not merely listed. The pointer check records `latest` and `latest-prerelease` before the build and diffs them after the upload, so "this workflow cannot move a pointer" is a check that can fail rather than an argument in a comment. --- .github/workflows/release-repair.yml | 403 ++++++++++++++++ AGENTS.md | 20 + .../release/ReleaseRepairCapabilityTest.java | 440 ++++++++++++++++++ 3 files changed, 863 insertions(+) create mode 100644 .github/workflows/release-repair.yml create mode 100644 core/src/test/java/com/minekube/connect/release/ReleaseRepairCapabilityTest.java diff --git a/.github/workflows/release-repair.yml b/.github/workflows/release-repair.yml new file mode 100644 index 00000000..ef58385b --- /dev/null +++ b/.github/workflows/release-repair.yml @@ -0,0 +1,403 @@ +name: release-repair + +# Repairs the assets of an EXISTING published release, from that release's +# tagged source, on CI runners. +# +# Why this is a separate workflow on the default branch, and not the +# `release_tag` input on release.yml: +# +# 1. GitHub compiles a workflow_dispatch run from the workflow file AT THE +# DISPATCHED REF. An input added to release.yml would simply not exist +# when release.yml is dispatched at an old tag - the old file is the one +# that runs, and for every tag before 0.7.1 that file has no +# workflow_dispatch trigger at all. Dispatching THIS workflow (which lives +# on the default branch) and checking out the tag is the only shape that +# works for every tag, including tags cut before this file existed. +# +# 2. release.yml rewrites the live `latest` release - the stable download URLs +# https://github.com/minekube/connect-java/releases/download/latest/*.jar +# that download sites and server owners point at. Re-running it at an OLD +# tag would drag `latest` backwards and silently downgrade everyone using +# those URLs. This workflow never names a release other than the one being +# repaired, and refuses the pointer tags outright. +# +# Capability boundary: this workflow holds `contents: write` and NOTHING else. +# It pushes no artifact to any package registry and requests no registry scope, +# so publishing connect's jars to a registry is not "guarded against" here - it +# is unrepresentable. ReleaseRepairCapabilityTest asserts that boundary. +# +# This workflow publishes GitHub Release assets only. + +on: + workflow_dispatch: + inputs: + release_tag: + description: Existing release tag to repair (e.g. 0.6.0). + required: true + type: string + +permissions: + contents: write # GitHub Release asset upload. The ONLY permission. + # `packages` and every other registry scope are deliberately absent and must + # stay absent. Without them, pushing a build anywhere but this release's own + # asset list is not forbidden here, it is unrepresentable. + # ReleaseRepairCapabilityTest asserts this file grants no registry scope. + +concurrency: + group: ${{ github.workflow }}-${{ inputs.release_tag }} + cancel-in-progress: false + +jobs: + repair: + runs-on: ubuntu-latest + timeout-minutes: 60 + env: + RELEASE_TAG: ${{ inputs.release_tag }} + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + steps: + # GUARD 0, before anything is even checked out: `latest` and + # `latest-prerelease` are not versions, they are the live pointers whose + # download URLs are published to users. Repairing them is never what + # anybody means, and rebuilding an old tag into them is exactly the + # silent downgrade this workflow exists to make impossible. + - name: Refuse to repair a pointer release + run: | + set -euo pipefail + + case "$RELEASE_TAG" in + latest | latest-prerelease) + echo "::error::$RELEASE_TAG is a live pointer release, not a version." + echo "::error::Repairing it would republish old jars at the stable download URLs" + echo "::error::and downgrade every consumer of them. Refusing." + exit 1 + ;; + esac + + if ! printf '%s' "$RELEASE_TAG" | grep -Eq '^[0-9]+\.[0-9]+\.[0-9]+$'; then + echo "::error::$RELEASE_TAG is not a connect-java version tag (expected N.N.N)." + exit 1 + fi + + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + ref: ${{ inputs.release_tag }} + + # Provenance: a repair must build the tag, not a branch that merely + # happens to be named like one. A moving ref would produce assets that do + # not correspond to the release they are attached to. + - name: Confirm the checkout is the tagged commit + run: | + set -euo pipefail + + TAG_SHA=$(git rev-parse -q --verify "refs/tags/${RELEASE_TAG}^{commit}" || true) + HEAD_SHA=$(git rev-parse HEAD) + + if [ -z "$TAG_SHA" ]; then + echo "::error::${RELEASE_TAG} is not a tag in this repository." + echo "::error::Repair rebuilds tagged source; it does not build branches." + exit 1 + fi + if [ "$TAG_SHA" != "$HEAD_SHA" ]; then + echo "::error::Checked-out HEAD ($HEAD_SHA) is not tag ${RELEASE_TAG} ($TAG_SHA)." + exit 1 + fi + + echo "Repairing ${RELEASE_TAG} from tagged commit ${TAG_SHA}." + + # GUARD 1, before the build rather than after it: repair fills holes. A + # release that already publishes a real plugin jar is complete, and + # re-uploading over it would replace exactly the bytes server owners + # already run. Failing here costs seconds instead of a full Gradle build. + # + # Classified by name/type, not counted: a release carrying only LICENSE + # has a positive asset count and still offers nothing anyone can run, + # which is precisely the state 0.6.0/0.7.0-era holes leave behind. + - name: Refuse to repair a complete release + run: | + set -euo pipefail + + if ! gh release view "$RELEASE_TAG" --repo "$GITHUB_REPOSITORY" >/dev/null 2>&1; then + echo "::error::No published release exists for $RELEASE_TAG to repair." + echo "::error::Repair fills holes in an existing release; it does not create one." + exit 1 + fi + + RELEASE_JSON=$(gh api "/repos/$GITHUB_REPOSITORY/releases/tags/$RELEASE_TAG") + + echo "$RELEASE_TAG currently publishes:" + printf '%s' "$RELEASE_JSON" | jq -r '.assets[] | " \(.name)\t\(.size)\t\(.state)"' + + EXISTING_BUILDS=$(printf '%s' "$RELEASE_JSON" | jq ' + [.assets[] + | select(.state == "uploaded") + | select(.size > 0) + | select(.name | test("^connect-(spigot|velocity|bungee).*\\.jar$"))] | length') + + if [ "$EXISTING_BUILDS" -gt 0 ]; then + echo "::error::Release $RELEASE_TAG already publishes $EXISTING_BUILDS plugin jar(s)." + echo "::error::Refusing to modify a complete release. Repair fills holes only." + exit 1 + fi + + # The tag's OWN toolchain, never the runner default. A newer JDK that + # fails to compile old tagged source produces a false red: it would + # condemn a tag that its own release path builds fine. Missing pin => + # hard failure, because silently falling back to a default is the same + # false result wearing a green hat. + - name: Resolve the tag's pinned Java toolchain + id: toolchain + run: | + set -euo pipefail + + TAG_WORKFLOW=.github/workflows/release.yml + if [ ! -f "$TAG_WORKFLOW" ]; then + echo "::error::$RELEASE_TAG has no $TAG_WORKFLOW; its release toolchain is unknown." + exit 1 + fi + + JAVA_VERSION=$(grep -Eo "java-version:[[:space:]]*['\"]?[0-9]+" "$TAG_WORKFLOW" \ + | grep -Eo '[0-9]+$' | head -n1) + + if [ -z "$JAVA_VERSION" ]; then + echo "::error::Could not read the Java version pinned by $RELEASE_TAG's $TAG_WORKFLOW." + echo "::error::Refusing to guess a toolchain: a wrong JDK produces a false result." + exit 1 + fi + + echo "java-version=$JAVA_VERSION" >> "$GITHUB_OUTPUT" + echo "$RELEASE_TAG pins JDK $JAVA_VERSION; Gradle comes from the tag's own wrapper:" + grep distributionUrl gradle/wrapper/gradle-wrapper.properties + + - name: Set up the tag's JDK + uses: actions/setup-java@v4 + with: + distribution: 'temurin' + java-version: ${{ steps.toolchain.outputs.java-version }} + cache: 'gradle' + + - name: Setup Gradle + uses: gradle/actions/setup-gradle@v3 + + # Recorded BEFORE the build so the after-check below compares against a + # pointer state this run cannot have touched yet. + - name: Record the live pointer releases + run: | + set -euo pipefail + + for pointer in latest latest-prerelease; do + if ! gh api "/repos/$GITHUB_REPOSITORY/releases/tags/$pointer" \ + | jq -S '{tag_name, name, target_commitish, created_at, published_at, + assets: [.assets[] | {name, size, updated_at}] | sort_by(.name)}' \ + > "/tmp/pointer-$pointer.before.json"; then + echo "::error::Could not read the $pointer release; refusing to run blind." + exit 1 + fi + echo "--- $pointer (before) ---" + cat "/tmp/pointer-$pointer.before.json" + done + + # The same build the tag's own release path ran. A repaired release must + # not carry weaker provenance than one published on the normal path. + - name: Build + run: ./gradlew build + + # Asset names follow the convention of the tag's OWN release workflow: + # tags up to 0.7.0 published version-suffixed jars, 0.7.1 onwards publish + # bare names. A repair that renamed them would not be a repair. + - name: Stage release assets + run: | + set -euo pipefail + + if grep -q 'connect-spigot-\${REF}\.jar' .github/workflows/release.yml; then + SUFFIX="-${RELEASE_TAG}" + else + SUFFIX="" + fi + echo "Asset naming for $RELEASE_TAG: connect-${SUFFIX}.jar" + + mkdir -p build/release + cp spigot/build/libs/connect-spigot.jar "build/release/connect-spigot${SUFFIX}.jar" + cp velocity/build/libs/connect-velocity.jar "build/release/connect-velocity${SUFFIX}.jar" + cp bungee/build/libs/connect-bungee.jar "build/release/connect-bungee${SUFFIX}.jar" + cp LICENSE build/release/LICENSE + + ls -la build/release/ + + # Repairing an existing release must not go through the `softprops` + # gh-release action that release.yml uses for fresh publishes: it PATCHes + # /releases/{id} to sync release metadata BEFORE uploading anything, which + # rewrites historical release notes. `gh release upload` only POSTs to + # /releases/{id}/assets, and only for the tag named here. + - name: Upload the missing assets + run: | + set -euo pipefail + + RELEASE_JSON=$(gh api "/repos/$GITHUB_REPOSITORY/releases/tags/$RELEASE_TAG") + + # GUARD 2: never clobber an existing GOOD asset. Only names that are + # absent, or present in a non-uploaded / zero-byte (failed upload) + # state, are eligible; --clobber then applies solely to those holes. + UPLOAD_LIST="" + SKIPPED="" + for f in build/release/*; do + [ -f "$f" ] || continue + base="$(basename "$f")" + good=$(printf '%s' "$RELEASE_JSON" | jq -r --arg n "$base" \ + '[.assets[] | select(.name == $n and .state == "uploaded" and .size > 0)] | length') + if [ "$good" -gt 0 ]; then + SKIPPED="$SKIPPED $base" + else + UPLOAD_LIST="$UPLOAD_LIST $f" + fi + done + + if [ -n "$SKIPPED" ]; then + echo "Keeping existing good assets (not clobbered):$SKIPPED" + fi + + if [ -z "$UPLOAD_LIST" ]; then + echo "::error::Nothing to upload for $RELEASE_TAG: every built file is already published." + exit 1 + fi + + echo "Uploading missing assets:$UPLOAD_LIST" + # shellcheck disable=SC2086 + gh release upload "$RELEASE_TAG" $UPLOAD_LIST --clobber --repo "$GITHUB_REPOSITORY" + + # A release that publishes nothing downloadable is indistinguishable from + # a healthy one: green run, release page exists, and the hole only + # surfaces when a server owner's download link 404s. Trusting the upload + # step we just ran would rebuild that same defect one layer up, so + # re-read the PUBLISHED release from the API and assert on the landed + # fact. Never conditional: the guard must not be skippable into silence. + - name: Verify published release assets + run: | + set -euo pipefail + + # A positive asset count is not a release. LICENSE alone has a + # positive count and offers nothing anyone can run, so classify by + # name/type and require a real downloadable plugin jar. + BUILD_FILTER='[.assets[] + | select(.state == "uploaded") + | select(.name | test( + "^checksums\\.txt$|^SHA(256|512)SUMS$|^LICENSE|^README|" + + "\\.sig$|\\.sigstore\\.json$|\\.attest\\.spdx\\.json$|" + + "\\.spdx\\.json$|\\.asc$|\\.pem$|\\.sha256$|" + + "\\.h$|\\.hpp$|\\.md$|\\.txt$" + ) | not) + | select(.size > 0)]' + + RELEASE_JSON="" + ASSET_COUNT=0 + BUILD_COUNT=0 + BUILD_NAMES="" + PROBE="" + PROBE_URL="" + PROBE_CODE="" + + # Asset visibility is eventually consistent right after upload. + for attempt in $(seq 1 12); do + if ! RELEASE_JSON=$(gh api "/repos/$GITHUB_REPOSITORY/releases/tags/$RELEASE_TAG"); then + echo "Release $RELEASE_TAG is not readable yet (attempt $attempt); waiting..." + RELEASE_JSON="" + sleep 10 + continue + fi + + ASSET_COUNT=$(printf '%s' "$RELEASE_JSON" | jq '[.assets[] | select(.state == "uploaded")] | length') + BUILD_COUNT=$(printf '%s' "$RELEASE_JSON" | jq "$BUILD_FILTER | length") + BUILD_NAMES=$(printf '%s' "$RELEASE_JSON" | jq -r "$BUILD_FILTER | .[].name") + + PROBE="" + PROBE_URL="" + PROBE_CODE="" + if [ "$BUILD_COUNT" -gt 0 ]; then + PROBE=$(printf '%s' "$BUILD_NAMES" | head -n1) + PROBE_URL="$GITHUB_SERVER_URL/$GITHUB_REPOSITORY/releases/download/$RELEASE_TAG/$PROBE" + if ! PROBE_CODE=$(curl -sSL -o /dev/null -w '%{http_code}' -r 0-0 "$PROBE_URL"); then + PROBE_CODE="000" + fi + fi + + if [ "$ASSET_COUNT" -gt 0 ] && [ "$BUILD_COUNT" -gt 0 ]; then + case "$PROBE_CODE" in + 200 | 206) break ;; + esac + fi + + echo "Published assets are incomplete on $RELEASE_TAG (attempt $attempt); waiting..." + sleep 10 + done + + echo "--- $RELEASE_TAG ---" + if [ -n "$RELEASE_JSON" ]; then + printf '%s' "$RELEASE_JSON" | jq -r '.assets[] | "\(.name)\t\(.size)\t\(.state)"' + fi + echo "Real build artifacts published ($BUILD_COUNT):" + echo "${BUILD_NAMES:- (none)}" + + if [ -z "$RELEASE_JSON" ]; then + echo "::error::Release $RELEASE_TAG could not be read back from the API; the repair did not land." + exit 1 + fi + + if [ "$ASSET_COUNT" -eq 0 ]; then + echo "::error::Release $RELEASE_TAG still publishes ZERO downloadable assets." + echo "::error::A release with no artifact silently did not happen. Failing loudly." + exit 1 + fi + + if [ "$BUILD_COUNT" -eq 0 ]; then + echo "::error::Release $RELEASE_TAG has $ASSET_COUNT asset(s) but NO real build artifact." + echo "::error::Only LICENSE/checksums/signatures/SBOMs were published - no plugin jar." + exit 1 + fi + + if [ "$PROBE_CODE" != "200" ] && [ "$PROBE_CODE" != "206" ]; then + echo "::error::Build artifact $PROBE is listed on $RELEASE_TAG but not downloadable (HTTP $PROBE_CODE)." + echo "::error::$PROBE_URL" + exit 1 + fi + + echo "OK: $RELEASE_TAG publishes $BUILD_COUNT real build artifact(s) of $ASSET_COUNT assets;" + echo "OK: $PROBE downloads (HTTP $PROBE_CODE)." + + # The steps above only ever name $RELEASE_TAG, so a pointer move is not + # something this workflow can express. That is an argument, not evidence - + # so read the pointers back and assert the landed fact, exactly as the + # asset check above refuses to trust its own upload step. + - name: Verify no pointer release moved + if: always() + run: | + set -euo pipefail + + MOVED="" + for pointer in latest latest-prerelease; do + BEFORE="/tmp/pointer-$pointer.before.json" + if [ ! -f "$BEFORE" ]; then + echo "No recorded state for $pointer (the run stopped before the build); nothing to compare." + continue + fi + gh api "/repos/$GITHUB_REPOSITORY/releases/tags/$pointer" \ + | jq -S '{tag_name, name, target_commitish, created_at, published_at, + assets: [.assets[] | {name, size, updated_at}] | sort_by(.name)}' \ + > "/tmp/pointer-$pointer.after.json" + + if diff -u "$BEFORE" "/tmp/pointer-$pointer.after.json"; then + echo "OK: the $pointer release is byte-for-byte unchanged." + else + MOVED="$MOVED $pointer" + fi + done + + if [ -n "$MOVED" ]; then + echo "::error::Pointer release(s) changed during this repair:$MOVED" + echo "::error::This workflow never names them, so this is either a concurrent" + echo "::error::release.yml run or something that must be understood before" + echo "::error::trusting the repaired assets. Failing loudly." + exit 1 + fi + + echo "NOTE: no package registry was touched. This workflow holds contents:write only" + echo "NOTE: and requests no registry scope, so publishing elsewhere is unrepresentable here." diff --git a/AGENTS.md b/AGENTS.md index bc5dc530..061b408b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,6 +43,26 @@ curl -I -L --fail https://github.com/minekube/connect-java/releases/download/These tests pin the capability boundary that makes that unrepresentable rather than merely + * discouraged: {@code contents: write} and nothing else, no registry scope, no secret beyond the + * job's own {@code GITHUB_TOKEN}, and no release named other than the one being repaired. A comment + * saying so is an argument; this is the check that can fail. + */ +class ReleaseRepairCapabilityTest { + + private static final Path WORKFLOW_PATH = + Paths.get("..", ".github", "workflows", "release-repair.yml"); + private static final Path REPOSITORY_GIT_PATH = Paths.get("..", ".git"); + + /** The live pointer releases whose download URLs are published to users. */ + private static final List POINTER_RELEASES = + Arrays.asList("latest", "latest-prerelease"); + + private static String workflowText() throws Exception { + if (!Files.exists(WORKFLOW_PATH)) { + if (Files.exists(REPOSITORY_GIT_PATH)) { + throw new AssertionError( + WORKFLOW_PATH + " is missing from the repository checkout"); + } + assumeTrue(false, WORKFLOW_PATH + " is unavailable outside a repository checkout"); + return ""; + } + return new String(Files.readAllBytes(WORKFLOW_PATH), StandardCharsets.UTF_8); + } + + @SuppressWarnings("unchecked") + private static Map workflow() throws Exception { + String text = workflowText(); + if (text.isEmpty()) { + return new LinkedHashMap<>(); + } + try (InputStream in = Files.newInputStream(WORKFLOW_PATH)) { + return (Map) new Yaml().load(in); + } + } + + @SuppressWarnings("unchecked") + private static Map repairJob() throws Exception { + Map workflow = workflow(); + if (workflow.isEmpty()) { + return new LinkedHashMap<>(); + } + Map jobs = (Map) workflow.get("jobs"); + assertTrue(jobs != null && jobs.containsKey("repair"), "repair job is missing"); + return (Map) jobs.get("repair"); + } + + @SuppressWarnings("unchecked") + private static List> steps() throws Exception { + Map job = repairJob(); + if (job.isEmpty()) { + return List.of(); + } + List> steps = (List>) job.get("steps"); + assertTrue(steps != null && !steps.isEmpty(), "repair job has no steps"); + return steps; + } + + private static int stepIndex(List> steps, String name) { + for (int i = 0; i < steps.size(); i++) { + if (name.equals(steps.get(i).get("name"))) { + return i; + } + } + return -1; + } + + private static String stepScript(List> steps, String name) { + int at = stepIndex(steps, name); + assertTrue(at >= 0, "repair job is missing the \"" + name + "\" step"); + Object run = steps.get(at).get("run"); + assertTrue(run instanceof String && !((String) run).isEmpty(), + "\"" + name + "\" must be a run step"); + return (String) run; + } + + /** + * The boundary itself. A repair that could also push to a package registry would be able to + * republish an old build as something users pull by default - the exact silent downgrade this + * workflow exists to avoid. Absent capability beats a guarded one. + */ + @Test + @SuppressWarnings("unchecked") + void repairGrantsContentsWriteAndNothingElse() throws Exception { + Map workflow = workflow(); + assumeTrue(!workflow.isEmpty()); + + Object permissions = workflow.get("permissions"); + assertTrue(permissions instanceof Map, + "release-repair.yml must declare an explicit permissions block; inheriting the " + + "repository default would hand the repair whatever scopes happen to be " + + "enabled"); + + Map granted = (Map) permissions; + assertEquals(Map.of("contents", "write"), granted, + "release-repair.yml must grant contents:write and nothing else; it grants " + + granted); + + // A job-level block would silently widen the workflow-level grant. + Object jobPermissions = repairJob().get("permissions"); + assertTrue(jobPermissions == null || Map.of("contents", "write").equals(jobPermissions), + "the repair job widens the workflow permissions to " + jobPermissions); + } + + /** + * The scope check above only proves the file does not name a registry permission. This + * proves it does not reach one another way: a registry login action, a docker/gradle publish, or + * a credential smuggled in through a secret other than the job's own GITHUB_TOKEN. + */ + @Test + void repairNeverPublishesToAPackageRegistry() throws Exception { + String text = workflowText(); + assumeTrue(!text.isEmpty()); + String lower = text.toLowerCase(Locale.ROOT); + + List registrySurfaces = Arrays.asList( + "packages: write", // the GitHub Packages scope + "id-token:", // OIDC federation into an external registry + "ghcr.io", // GitHub container registry + "docker/login-action", // registry login + "docker push", + "gradle-publish", + "maven-publish", + "publishtomavenlocal", + "publishallpublications", + "./gradlew publish", + "npm publish", + "sonatype", + "nexus"); + + List found = new ArrayList<>(); + for (String surface : registrySurfaces) { + if (lower.contains(surface)) { + found.add(surface); + } + } + assertTrue(found.isEmpty(), "release-repair.yml references registry publishing surface " + + found + "; the repair must be unable to push a build anywhere but this " + + "release's own assets"); + + // Any secret beyond GITHUB_TOKEN is a credential this workflow must not hold. If a repair + // genuinely needed one, that is a decision to escalate, not to quietly add here. + Matcher secrets = Pattern.compile("secrets\\.([A-Za-z0-9_]+)").matcher(text); + List extra = new ArrayList<>(); + while (secrets.find()) { + if (!"GITHUB_TOKEN".equals(secrets.group(1))) { + extra.add(secrets.group(1)); + } + } + assertTrue(extra.isEmpty(), + "release-repair.yml consumes secret(s) " + extra + " beyond GITHUB_TOKEN"); + } + + /** + * {@code latest} and {@code latest-prerelease} are not versions, they are the live pointers + * whose download URLs the release notes advertise. Repairing one would republish an old build at + * a stable URL and downgrade everyone using it, so the workflow must refuse the names outright + * and must never name any release but the one being repaired. + */ + @Test + void repairCannotWriteAPointerRelease() throws Exception { + String text = workflowText(); + assumeTrue(!text.isEmpty()); + List> steps = steps(); + + String refusal = stepScript(steps, "Refuse to repair a pointer release"); + for (String pointer : POINTER_RELEASES) { + assertTrue(refusal.contains(pointer), + "the pointer refusal does not reject \"" + pointer + "\""); + } + assertTrue(refusal.contains("exit 1"), "the pointer refusal does not fail the run"); + + // It has to run before anything is built or uploaded, otherwise it refuses too late. + assertEquals(0, stepIndex(steps, "Refuse to repair a pointer release"), + "the pointer refusal must be the first step"); + + // release.yml writes `tag_name: latest`. Nothing in a repair may. + assertTrue(!Pattern.compile("(?m)tag_name:\\s*latest").matcher(text).find(), + "release-repair.yml writes a pointer release via tag_name"); + + // Every asset upload must name the tag under repair and no other release. + Matcher uploads = Pattern.compile("gh release upload (\\S+)").matcher(text); + int uploadCount = 0; + while (uploads.find()) { + uploadCount++; + assertEquals("\"$RELEASE_TAG\"", uploads.group(1), + "release-repair.yml uploads to a release other than the tag under repair"); + } + assertTrue(uploadCount > 0, "release-repair.yml never uploads any asset"); + } + + /** + * Guard 1. Repair fills holes: a release that already publishes a plugin jar is complete, and + * {@code --clobber} over it would replace exactly the bytes server owners already run. The check + * has to precede the build, or a needless rebuild happens before it can say no. + */ + @Test + void repairRefusesToClobberACompleteRelease() throws Exception { + List> steps = steps(); + assumeTrue(!steps.isEmpty()); + + String guard = stepScript(steps, "Refuse to repair a complete release"); + assertTrue(guard.contains("/releases/tags/"), + "the completeness guard must read the published release, not local state"); + assertTrue(guard.contains("connect-(spigot|velocity|bungee)"), + "the completeness guard must classify plugin jars by name; a release carrying " + + "only LICENSE has a positive asset count and is still a hole"); + assertTrue(guard.contains("exit 1"), "the completeness guard does not fail the run"); + + int guardAt = stepIndex(steps, "Refuse to repair a complete release"); + int buildAt = stepIndex(steps, "Build"); + assertTrue(buildAt >= 0, "repair job is missing the Build step"); + assertTrue(guardAt < buildAt, + "the completeness guard runs after the build; it must refuse before rebuilding"); + + // Guard 2: --clobber must apply only to holes, never to a published good asset. + String upload = stepScript(steps, "Upload the missing assets"); + assertTrue(upload.contains("--clobber"), "the upload does not use --clobber"); + assertTrue(upload.contains("state == \"uploaded\"") && upload.contains("size > 0"), + "the upload does not exclude already-good assets from --clobber"); + } + + /** + * A repair must build the tag on the toolchain that tag pinned. A newer runner-default JDK that + * fails to compile old tagged source condemns a perfectly buildable release - a false red is as + * wrong as a false green, and here it would be recorded as "unrecoverable". + */ + @Test + void repairBuildsAtTheTagsOwnPinnedToolchain() throws Exception { + List> steps = steps(); + assumeTrue(!steps.isEmpty()); + + String resolve = stepScript(steps, "Resolve the tag's pinned Java toolchain"); + assertTrue(resolve.contains(".github/workflows/release.yml"), + "the toolchain must be read from the tag's own release workflow"); + assertTrue(resolve.contains("java-version"), "the toolchain step reads no Java version"); + assertTrue(resolve.contains("exit 1"), + "the toolchain step falls back to a default instead of failing; guessing a JDK " + + "produces a false result about whether the tag builds"); + + int at = stepIndex(steps, "Set up the tag's JDK"); + assertTrue(at >= 0, "repair job is missing the JDK setup step"); + @SuppressWarnings("unchecked") + Map with = (Map) steps.get(at).get("with"); + assertTrue(with != null, "the JDK setup step has no inputs"); + assertEquals("${{ steps.toolchain.outputs.java-version }}", String.valueOf(with.get("java-version")), + "the JDK is hard-coded instead of taken from the tag's pin"); + + // The tag's checkout supplies its own Gradle wrapper; a pinned Gradle here would override it. + assertTrue(!workflowText().contains("gradle-version:"), + "release-repair.yml pins a Gradle version, overriding the tag's own wrapper"); + } + + /** + * The whole point of the R4 mechanism: never trust the run, read the artifact. A green upload + * step that published nothing looks exactly like a successful repair, so the guard goes back to + * the API and additionally proves a jar is served, not merely listed. + */ + @Test + void repairVerifiesTheLandedRelease() throws Exception { + List> steps = steps(); + assumeTrue(!steps.isEmpty()); + + String verify = stepScript(steps, "Verify published release assets"); + + List required = Arrays.asList( + "gh api", // reads the API, not the local build output + "/releases/tags/", // re-reads the published release by tag + "\"uploaded\"", // only fully-uploaded assets count + "releases/download/"); // proves a jar is actually served, not just listed + List missing = new ArrayList<>(); + for (String want : required) { + if (!verify.contains(want)) { + missing.add(want); + } + } + assertTrue(missing.isEmpty(), "the repair verification does not reference " + missing + + "; it must assert on the published release, not on local build output"); + + assertTrue(!verify.contains("build/libs") && !verify.contains("build/release"), + "the repair verification reads local build output; those jars exist even when " + + "the upload never happened"); + + // Wrong-artifact-type is its own failure mode and the more dangerous one, because the + // release looks populated. LICENSE alone must not satisfy the guard. + assertTrue(verify.contains("^LICENSE"), + "the repair verification counts LICENSE as a build artifact"); + assertTrue(Pattern.compile("BUILD_COUNT\"\\s*-eq\\s*0").matcher(verify).find(), + "the repair verification does not fail on a zero build-artifact count"); + + // Unconditional: a guard that can be gated off is not a guard. + assertTrue(steps.get(stepIndex(steps, "Verify published release assets")).get("if") == null, + "the repair verification is conditional; it must always run"); + + int verifyAt = stepIndex(steps, "Verify published release assets"); + int uploadAt = stepIndex(steps, "Upload the missing assets"); + assertTrue(verifyAt > uploadAt, + "the repair verification runs before the upload; it would see the old release"); + } + + /** + * "The workflow only names $RELEASE_TAG, so it cannot move a pointer" is an argument about the + * source. This is the check on the landed fact: record both pointer releases before the build + * and assert they are unchanged afterwards, so a move shows up as a red run rather than as a + * server owner's surprise downgrade. + */ + @Test + void repairProvesNoPointerReleaseMoved() throws Exception { + List> steps = steps(); + assumeTrue(!steps.isEmpty()); + + String before = stepScript(steps, "Record the live pointer releases"); + String after = stepScript(steps, "Verify no pointer release moved"); + for (String pointer : POINTER_RELEASES) { + assertTrue(before.contains(pointer) && after.contains(pointer), + "the pointer-immutability check does not cover \"" + pointer + "\""); + } + + assertTrue(before.contains("gh api") && after.contains("gh api"), + "the pointer-immutability check must read the releases from the API"); + assertTrue(after.contains("diff"), "the pointer check never compares before against after"); + assertTrue(after.contains("exit 1"), "the pointer check cannot fail the run"); + + int recordAt = stepIndex(steps, "Record the live pointer releases"); + int buildAt = stepIndex(steps, "Build"); + int uploadAt = stepIndex(steps, "Upload the missing assets"); + int checkAt = stepIndex(steps, "Verify no pointer release moved"); + assertTrue(recordAt < buildAt, + "the pointer state is recorded after the build; the baseline must predate any " + + "work this run does"); + assertTrue(checkAt > uploadAt, + "the pointer check runs before the upload and would never observe a move"); + } + + /** + * Dispatch shape. Tags older than 0.7.1 have no {@code workflow_dispatch} trigger at all, and + * GitHub compiles a dispatched run from the workflow file at the dispatched ref - so the repair + * only works when it is dispatched from the default branch and checks the tag out itself. + */ + @Test + @SuppressWarnings("unchecked") + void repairIsDispatchedByTagAndChecksThatTagOut() throws Exception { + Map workflow = workflow(); + assumeTrue(!workflow.isEmpty()); + + // snakeyaml resolves the bare `on:` key as YAML 1.1 boolean true. + Object triggers = workflow.containsKey("on") ? workflow.get("on") : workflow.get(true); + assertTrue(triggers instanceof Map, "release-repair.yml declares no triggers"); + Map on = (Map) triggers; + + assertEquals(1, on.size(), + "release-repair.yml must be manual-dispatch only; it also triggers on " + on.keySet()); + assertTrue(on.containsKey("workflow_dispatch"), + "release-repair.yml is not a workflow_dispatch workflow"); + + Map dispatch = (Map) on.get("workflow_dispatch"); + Map inputs = (Map) dispatch.get("inputs"); + assertTrue(inputs != null && inputs.containsKey("release_tag"), + "release-repair.yml takes no release_tag input"); + assertEquals(Boolean.TRUE, ((Map) inputs.get("release_tag")).get("required"), + "release_tag must be required; an empty tag would repair the dispatch ref"); + + List> steps = steps(); + Map checkout = null; + for (Map step : steps) { + Object uses = step.get("uses"); + if (uses instanceof String && ((String) uses).startsWith("actions/checkout@")) { + checkout = step; + break; + } + } + assertTrue(checkout != null, "release-repair.yml never checks out the repository"); + Map with = (Map) checkout.get("with"); + assertTrue(with != null, "the checkout step has no inputs"); + assertEquals("${{ inputs.release_tag }}", String.valueOf(with.get("ref")), + "the checkout must build the requested tag, not the dispatch ref"); + + // Provenance: the checked-out HEAD must be proven to be the tag, not a branch of that name. + String provenance = stepScript(steps, "Confirm the checkout is the tagged commit"); + assertTrue(provenance.contains("refs/tags/") && provenance.contains("exit 1"), + "the repair does not prove it built the tagged commit"); + } +} From db5f37dd9df77015cbc923c465074b84950d146f Mon Sep 17 00:00:00 2001 From: "no-mistakes[bot]" Date: Mon, 27 Jul 2026 10:08:56 +0200 Subject: [PATCH 2/6] no-mistakes(review): Hardened release repair credentials, assets, race, and caching --- .github/workflows/release-repair.yml | 59 +++++++++++++------ core/build.gradle.kts | 3 + .../release/ReleaseRepairCapabilityTest.java | 58 +++++++++++++++++- 3 files changed, 101 insertions(+), 19 deletions(-) diff --git a/.github/workflows/release-repair.yml b/.github/workflows/release-repair.yml index ef58385b..062515e4 100644 --- a/.github/workflows/release-repair.yml +++ b/.github/workflows/release-repair.yml @@ -53,7 +53,6 @@ jobs: timeout-minutes: 60 env: RELEASE_TAG: ${{ inputs.release_tag }} - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} steps: # GUARD 0, before anything is even checked out: `latest` and # `latest-prerelease` are not versions, they are the live pointers whose @@ -82,6 +81,7 @@ jobs: with: fetch-depth: 0 ref: ${{ inputs.release_tag }} + persist-credentials: false # Provenance: a repair must build the tag, not a branch that merely # happens to be named like one. A moving ref would produce assets that do @@ -114,6 +114,8 @@ jobs: # has a positive asset count and still offers nothing anyone can run, # which is precisely the state 0.6.0/0.7.0-era holes leave behind. - name: Refuse to repair a complete release + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | set -euo pipefail @@ -182,6 +184,8 @@ jobs: # Recorded BEFORE the build so the after-check below compares against a # pointer state this run cannot have touched yet. - name: Record the live pointer releases + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | set -euo pipefail @@ -230,40 +234,62 @@ jobs: # rewrites historical release notes. `gh release upload` only POSTs to # /releases/{id}/assets, and only for the tag named here. - name: Upload the missing assets + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | set -euo pipefail - RELEASE_JSON=$(gh api "/repos/$GITHUB_REPOSITORY/releases/tags/$RELEASE_TAG") + INITIAL_RELEASE_JSON=$(gh api "/repos/$GITHUB_REPOSITORY/releases/tags/$RELEASE_TAG") + + FILES=() + for f in build/release/*; do + [ -f "$f" ] || continue + FILES+=("$f") + done + + if [ "${#FILES[@]}" -eq 0 ]; then + echo "::error::Nothing to upload for $RELEASE_TAG: no staged assets exist." + exit 1 + fi # GUARD 2: never clobber an existing GOOD asset. Only names that are # absent, or present in a non-uploaded / zero-byte (failed upload) # state, are eligible; --clobber then applies solely to those holes. - UPLOAD_LIST="" + RELEASE_JSON=$(gh api "/repos/$GITHUB_REPOSITORY/releases/tags/$RELEASE_TAG") + UPLOAD_LIST=() SKIPPED="" - for f in build/release/*; do - [ -f "$f" ] || continue + PUBLISHED_DURING_REPAIR="" + for f in "${FILES[@]}"; do base="$(basename "$f")" + initially_absent=$(printf '%s' "$INITIAL_RELEASE_JSON" | jq -r --arg n "$base" \ + '[.assets[] | select(.name == $n)] | length') good=$(printf '%s' "$RELEASE_JSON" | jq -r --arg n "$base" \ '[.assets[] | select(.name == $n and .state == "uploaded" and .size > 0)] | length') if [ "$good" -gt 0 ]; then SKIPPED="$SKIPPED $base" + if [ "$initially_absent" -gt 0 ]; then + PUBLISHED_DURING_REPAIR="$PUBLISHED_DURING_REPAIR $base" + fi else - UPLOAD_LIST="$UPLOAD_LIST $f" + UPLOAD_LIST+=("$f") fi done + if [ -n "$PUBLISHED_DURING_REPAIR" ]; then + echo "Skipping assets published since the initial release snapshot (not clobbered):$PUBLISHED_DURING_REPAIR" + fi + if [ -n "$SKIPPED" ]; then echo "Keeping existing good assets (not clobbered):$SKIPPED" fi - if [ -z "$UPLOAD_LIST" ]; then + if [ "${#UPLOAD_LIST[@]}" -eq 0 ]; then echo "::error::Nothing to upload for $RELEASE_TAG: every built file is already published." exit 1 fi - echo "Uploading missing assets:$UPLOAD_LIST" - # shellcheck disable=SC2086 - gh release upload "$RELEASE_TAG" $UPLOAD_LIST --clobber --repo "$GITHUB_REPOSITORY" + echo "Uploading missing assets" + gh release upload "$RELEASE_TAG" "${UPLOAD_LIST[@]}" --clobber --repo "$GITHUB_REPOSITORY" # A release that publishes nothing downloadable is indistinguishable from # a healthy one: green run, release page exists, and the hole only @@ -272,6 +298,8 @@ jobs: # re-read the PUBLISHED release from the API and assert on the landed # fact. Never conditional: the guard must not be skippable into silence. - name: Verify published release assets + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | set -euo pipefail @@ -280,13 +308,8 @@ jobs: # name/type and require a real downloadable plugin jar. BUILD_FILTER='[.assets[] | select(.state == "uploaded") - | select(.name | test( - "^checksums\\.txt$|^SHA(256|512)SUMS$|^LICENSE|^README|" - + "\\.sig$|\\.sigstore\\.json$|\\.attest\\.spdx\\.json$|" - + "\\.spdx\\.json$|\\.asc$|\\.pem$|\\.sha256$|" - + "\\.h$|\\.hpp$|\\.md$|\\.txt$" - ) | not) - | select(.size > 0)]' + | select(.size > 0) + | select(.name | test("^connect-(spigot|velocity|bungee).*\\.jar$"))]' RELEASE_JSON="" ASSET_COUNT=0 @@ -369,6 +392,8 @@ jobs: # asset check above refuses to trust its own upload step. - name: Verify no pointer release moved if: always() + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | set -euo pipefail diff --git a/core/build.gradle.kts b/core/build.gradle.kts index cb6b5363..69aed1f7 100644 --- a/core/build.gradle.kts +++ b/core/build.gradle.kts @@ -46,6 +46,9 @@ tasks.test { inputs.file(rootProject.file(".github/workflows/release.yml")) .withPropertyName("releaseWorkflow") .withPathSensitivity(PathSensitivity.RELATIVE) + inputs.file(rootProject.file(".github/workflows/release-repair.yml")) + .withPropertyName("releaseRepairWorkflow") + .withPathSensitivity(PathSensitivity.RELATIVE) } relocate("org.bstats") diff --git a/core/src/test/java/com/minekube/connect/release/ReleaseRepairCapabilityTest.java b/core/src/test/java/com/minekube/connect/release/ReleaseRepairCapabilityTest.java index 3e67c9e6..18771e6c 100644 --- a/core/src/test/java/com/minekube/connect/release/ReleaseRepairCapabilityTest.java +++ b/core/src/test/java/com/minekube/connect/release/ReleaseRepairCapabilityTest.java @@ -275,6 +275,15 @@ void repairRefusesToClobberACompleteRelease() throws Exception { assertTrue(upload.contains("--clobber"), "the upload does not use --clobber"); assertTrue(upload.contains("state == \"uploaded\"") && upload.contains("size > 0"), "the upload does not exclude already-good assets from --clobber"); + assertTrue(upload.contains("INITIAL_RELEASE_JSON"), + "the upload does not retain the initial release snapshot"); + int rereadAt = upload.indexOf("RELEASE_JSON=$(gh api"); + int decisionAt = upload.indexOf("for f in \"${FILES[@]}\""); + assertTrue(rereadAt >= 0 && decisionAt > rereadAt, + "the upload does not re-read the release immediately before deciding what to " + + "clobber"); + assertTrue(upload.contains("PUBLISHED_DURING_REPAIR"), + "the upload does not report a newly published good asset that it skips"); } /** @@ -340,8 +349,10 @@ void repairVerifiesTheLandedRelease() throws Exception { // Wrong-artifact-type is its own failure mode and the more dangerous one, because the // release looks populated. LICENSE alone must not satisfy the guard. - assertTrue(verify.contains("^LICENSE"), - "the repair verification counts LICENSE as a build artifact"); + assertTrue(verify.contains("^connect-(spigot|velocity|bungee).*\\\\.jar$"), + "the repair verification does not require a plugin jar asset by name"); + assertTrue(verify.contains("size > 0"), + "the repair verification counts zero-byte plugin assets"); assertTrue(Pattern.compile("BUILD_COUNT\"\\s*-eq\\s*0").matcher(verify).find(), "the repair verification does not fail on a zero build-artifact count"); @@ -355,6 +366,49 @@ void repairVerifiesTheLandedRelease() throws Exception { "the repair verification runs before the upload; it would see the old release"); } + @Test + @SuppressWarnings("unchecked") + void repairKeepsTheWriteTokenOutOfTaggedBuildLogic() throws Exception { + Map job = repairJob(); + assumeTrue(!job.isEmpty()); + + Map jobEnv = (Map) job.get("env"); + assertTrue(jobEnv == null || !jobEnv.containsKey("GH_TOKEN"), + "the write token is exposed to every step in the repair job"); + + List> steps = steps(); + Map build = steps.get(stepIndex(steps, "Build")); + Map buildEnv = (Map) build.get("env"); + assertTrue(buildEnv == null || !buildEnv.containsKey("GH_TOKEN"), + "the tagged build can access the write-scoped GitHub token"); + + Map checkout = null; + for (Map step : steps) { + Object uses = step.get("uses"); + if (uses instanceof String && ((String) uses).startsWith("actions/checkout@")) { + checkout = step; + break; + } + } + assertTrue(checkout != null, "release-repair.yml never checks out the repository"); + Map checkoutWith = (Map) checkout.get("with"); + assertEquals(Boolean.FALSE, checkoutWith.get("persist-credentials"), + "the checkout leaves the write token in the tagged build's git config"); + + List apiSteps = Arrays.asList( + "Refuse to repair a complete release", + "Record the live pointer releases", + "Upload the missing assets", + "Verify published release assets", + "Verify no pointer release moved"); + for (String name : apiSteps) { + Map env = + (Map) steps.get(stepIndex(steps, name)).get("env"); + assertEquals("${{ secrets.GITHUB_TOKEN }}", env.get("GH_TOKEN"), + "the API step " + name + " does not receive the scoped GitHub token"); + } + } + /** * "The workflow only names $RELEASE_TAG, so it cannot move a pointer" is an argument about the * source. This is the check on the landed fact: record both pointer releases before the build From 3b935de0e5951a0e737e8d8b3fcb9cc16fa4693e Mon Sep 17 00:00:00 2001 From: "no-mistakes[bot]" Date: Mon, 27 Jul 2026 10:18:49 +0200 Subject: [PATCH 3/6] no-mistakes(review): Split absent and broken uploads to close clobber race --- .github/workflows/release-repair.yml | 33 +++++++++++++++---- .../release/ReleaseRepairCapabilityTest.java | 12 +++++++ 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/.github/workflows/release-repair.yml b/.github/workflows/release-repair.yml index 062515e4..62178025 100644 --- a/.github/workflows/release-repair.yml +++ b/.github/workflows/release-repair.yml @@ -256,22 +256,27 @@ jobs: # absent, or present in a non-uploaded / zero-byte (failed upload) # state, are eligible; --clobber then applies solely to those holes. RELEASE_JSON=$(gh api "/repos/$GITHUB_REPOSITORY/releases/tags/$RELEASE_TAG") - UPLOAD_LIST=() + ABSENT_LIST=() + BROKEN_LIST=() SKIPPED="" PUBLISHED_DURING_REPAIR="" for f in "${FILES[@]}"; do base="$(basename "$f")" - initially_absent=$(printf '%s' "$INITIAL_RELEASE_JSON" | jq -r --arg n "$base" \ + initial_asset_count=$(printf '%s' "$INITIAL_RELEASE_JSON" | jq -r --arg n "$base" \ + '[.assets[] | select(.name == $n)] | length') + current_asset_count=$(printf '%s' "$RELEASE_JSON" | jq -r --arg n "$base" \ '[.assets[] | select(.name == $n)] | length') good=$(printf '%s' "$RELEASE_JSON" | jq -r --arg n "$base" \ '[.assets[] | select(.name == $n and .state == "uploaded" and .size > 0)] | length') if [ "$good" -gt 0 ]; then SKIPPED="$SKIPPED $base" - if [ "$initially_absent" -gt 0 ]; then + if [ "$initial_asset_count" -eq 0 ]; then PUBLISHED_DURING_REPAIR="$PUBLISHED_DURING_REPAIR $base" fi + elif [ "$current_asset_count" -eq 0 ]; then + ABSENT_LIST+=("$f") else - UPLOAD_LIST+=("$f") + BROKEN_LIST+=("$f") fi done @@ -283,13 +288,27 @@ jobs: echo "Keeping existing good assets (not clobbered):$SKIPPED" fi - if [ "${#UPLOAD_LIST[@]}" -eq 0 ]; then + if [ "${#ABSENT_LIST[@]}" -eq 0 ] && [ "${#BROKEN_LIST[@]}" -eq 0 ]; then echo "::error::Nothing to upload for $RELEASE_TAG: every built file is already published." exit 1 fi - echo "Uploading missing assets" - gh release upload "$RELEASE_TAG" "${UPLOAD_LIST[@]}" --clobber --repo "$GITHUB_REPOSITORY" + if [ "${#ABSENT_LIST[@]}" -gt 0 ]; then + echo "Uploading newly staged assets without clobber" + if ! gh release upload "$RELEASE_TAG" "${ABSENT_LIST[@]}" --repo "$GITHUB_REPOSITORY"; then + echo "::error::An asset classified as absent was rejected during upload; a concurrent release writer may have published one of ${ABSENT_LIST[*]}." + echo "::error::Refusing to retry with --clobber." + exit 1 + fi + fi + + if [ "${#BROKEN_LIST[@]}" -gt 0 ]; then + echo "Replacing broken release assets with clobber" + # A writer can replace a broken asset with a good one after this read and before + # --clobber. The prior bytes were already unusable; landed-asset and pointer checks + # below surface the result. + gh release upload "$RELEASE_TAG" "${BROKEN_LIST[@]}" --clobber --repo "$GITHUB_REPOSITORY" + fi # A release that publishes nothing downloadable is indistinguishable from # a healthy one: green run, release page exists, and the hole only diff --git a/core/src/test/java/com/minekube/connect/release/ReleaseRepairCapabilityTest.java b/core/src/test/java/com/minekube/connect/release/ReleaseRepairCapabilityTest.java index 18771e6c..f1f0ce0c 100644 --- a/core/src/test/java/com/minekube/connect/release/ReleaseRepairCapabilityTest.java +++ b/core/src/test/java/com/minekube/connect/release/ReleaseRepairCapabilityTest.java @@ -277,6 +277,12 @@ void repairRefusesToClobberACompleteRelease() throws Exception { "the upload does not exclude already-good assets from --clobber"); assertTrue(upload.contains("INITIAL_RELEASE_JSON"), "the upload does not retain the initial release snapshot"); + assertTrue(upload.contains("initial_asset_count"), + "the upload does not distinguish initially absent assets"); + assertTrue(upload.contains("[ \"$initial_asset_count\" -eq 0 ]"), + "the upload misclassifies assets that appeared during the repair"); + assertTrue(upload.contains("ABSENT_LIST=()") && upload.contains("BROKEN_LIST=()"), + "the upload does not separate absent assets from broken assets"); int rereadAt = upload.indexOf("RELEASE_JSON=$(gh api"); int decisionAt = upload.indexOf("for f in \"${FILES[@]}\""); assertTrue(rereadAt >= 0 && decisionAt > rereadAt, @@ -284,6 +290,12 @@ void repairRefusesToClobberACompleteRelease() throws Exception { + "clobber"); assertTrue(upload.contains("PUBLISHED_DURING_REPAIR"), "the upload does not report a newly published good asset that it skips"); + int absentUploadAt = upload.indexOf( + "gh release upload \"$RELEASE_TAG\" \"${ABSENT_LIST[@]}\" --repo \"$GITHUB_REPOSITORY\""); + int brokenUploadAt = upload.indexOf( + "gh release upload \"$RELEASE_TAG\" \"${BROKEN_LIST[@]}\" --clobber --repo \"$GITHUB_REPOSITORY\""); + assertTrue(absentUploadAt >= 0 && brokenUploadAt > absentUploadAt, + "the upload does not use the atomic no-clobber path for absent assets"); } /** From b3eddfc5be94bbcdc02ef0df36adaed933556fc8 Mon Sep 17 00:00:00 2001 From: "no-mistakes[bot]" Date: Mon, 27 Jul 2026 10:36:56 +0200 Subject: [PATCH 4/6] no-mistakes(document): Updated repair docs and fixed workflow shell lint --- .github/workflows/release-repair.yml | 2 +- AGENTS.md | 2 +- core/build.gradle.kts | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/release-repair.yml b/.github/workflows/release-repair.yml index 62178025..648f5e92 100644 --- a/.github/workflows/release-repair.yml +++ b/.github/workflows/release-repair.yml @@ -213,7 +213,7 @@ jobs: run: | set -euo pipefail - if grep -q 'connect-spigot-\${REF}\.jar' .github/workflows/release.yml; then + if grep -q "connect-spigot-\${REF}\.jar" .github/workflows/release.yml; then SUFFIX="-${RELEASE_TAG}" else SUFFIX="" diff --git a/AGENTS.md b/AGENTS.md index 061b408b..f583eb2c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -47,7 +47,7 @@ curl -I -L --fail https://github.com/minekube/connect-java/releases/download/ Date: Mon, 27 Jul 2026 10:59:18 +0200 Subject: [PATCH 5/6] fix: split release-repair into read-only build and write-only publish jobs Scoping GH_TOKEN to individual steps does not contain a repair. A repair necessarily executes old, unreviewed tagged source, and within a single job that build can append to $GITHUB_PATH or $GITHUB_ENV so that a later, token-bearing step in the same job executes a tool it planted. Step-level `env:` is not a sandbox; the job is the boundary. The previous shape put the tagged Gradle build and every write-scoped step in one `repair` job, so the containment it claimed was not real. Now the write capability lives where no tag code can reach it: build - permissions: contents: read. Checks the tag out, runs its own pinned toolchain and Gradle build, hands the staged assets on as an artifact. Nothing it plants can be escalated, because there is no write capability in the job to escalate into. No step at or after the build carries a credential at all. publish - permissions: contents: write, needs: build. Checks out NOTHING, runs no tag code, downloads the artifact and uploads it to the one release named by the dispatch input. Top-level permissions are now `{}`, so neither job inherits a scope it was not explicitly granted. The artifact crossing that split was produced by the untrusted job, so its file NAMES are untrusted input to the publish job - they become public release asset names. The publish job re-validates them against connect-*.jar plus LICENSE and rejects symlinks, nested paths and empty files before anything is uploaded. Every previously pinned guard is kept and re-verified against the new shape: the pointer-tag refusal is now the first step of BOTH jobs (it belongs where the capability is), the completeness guard still runs before the build, the toolchain is still pinned from the tag with a hard failure when unreadable, absent names still upload without --clobber so GitHub's own rejection is the atomic conditional, and the landed checks still require a real plugin jar and prove the pointers did not move. ReleaseRepairCapabilityTest gains three tests for the split itself and the suite goes red on each of: granting the build job a write scope, adding a checkout to the publish job, placing a token-bearing step after the tagged build, and collapsing Gradle back into the publish job. --- .github/workflows/release-repair.yml | 224 ++++++++-- AGENTS.md | 20 +- .../release/ReleaseRepairCapabilityTest.java | 406 +++++++++++------- 3 files changed, 437 insertions(+), 213 deletions(-) diff --git a/.github/workflows/release-repair.yml b/.github/workflows/release-repair.yml index 648f5e92..cf78241f 100644 --- a/.github/workflows/release-repair.yml +++ b/.github/workflows/release-repair.yml @@ -21,10 +21,34 @@ name: release-repair # those URLs. This workflow never names a release other than the one being # repaired, and refuses the pointer tags outright. # -# Capability boundary: this workflow holds `contents: write` and NOTHING else. -# It pushes no artifact to any package registry and requests no registry scope, -# so publishing connect's jars to a registry is not "guarded against" here - it -# is unrepresentable. ReleaseRepairCapabilityTest asserts that boundary. +# WHY TWO JOBS, and why that is the whole security model: +# +# A repair necessarily EXECUTES OLD TAGGED SOURCE - a Gradle build, its +# plugins, and whatever those resolve. That source is not reviewed as part of +# the repair, and for the tags this exists to fix it is years old. So the +# question is not "is the build trustworthy" but "what can it reach". +# +# Scoping the token to individual steps does NOT answer that. Within one job, +# a build step can append to $GITHUB_PATH or $GITHUB_ENV and have a LATER, +# token-bearing step in that same job execute a tool it planted. Step-level +# `env:` is not a sandbox; the job is the boundary. +# +# So the write capability lives in a job that never sees the tag: +# +# build - permissions: contents: read. Checks the tag out and runs its +# Gradle build. Holds NO write capability, so nothing it plants can +# be escalated into a release write, a ref update, or a pointer +# move. Hands its output on as an artifact. +# publish - permissions: contents: write. Checks out NOTHING, runs NO tag +# code, and only downloads that artifact and uploads it to the one +# release named by the dispatch input. +# +# Top-level permissions are `{}` so neither job inherits anything it was not +# explicitly granted. No registry scope exists anywhere in this file, so +# publishing connect's jars to a package registry is not "guarded against" +# here - it is unrepresentable. ReleaseRepairCapabilityTest asserts the split +# and the boundary; a token on the build job, or a checkout in the publish job, +# turns that suite red. # # This workflow publishes GitHub Release assets only. @@ -36,21 +60,24 @@ on: required: true type: string -permissions: - contents: write # GitHub Release asset upload. The ONLY permission. - # `packages` and every other registry scope are deliberately absent and must - # stay absent. Without them, pushing a build anywhere but this release's own - # asset list is not forbidden here, it is unrepresentable. - # ReleaseRepairCapabilityTest asserts this file grants no registry scope. +# Nothing is inherited. Each job is granted exactly what it needs, below. +permissions: {} concurrency: group: ${{ github.workflow }}-${{ inputs.release_tag }} cancel-in-progress: false +env: + ASSET_ARTIFACT: release-assets + jobs: - repair: + # Runs the untrusted tagged source. Read-only by construction: there is no + # write capability in this job for a planted tool to reach. + build: runs-on: ubuntu-latest timeout-minutes: 60 + permissions: + contents: read # Read the tag and its release. NEVER write here. env: RELEASE_TAG: ${{ inputs.release_tag }} steps: @@ -113,6 +140,8 @@ jobs: # Classified by name/type, not counted: a release carrying only LICENSE # has a positive asset count and still offers nothing anyone can run, # which is precisely the state 0.6.0/0.7.0-era holes leave behind. + # + # This is a READ, and it runs before any tag code executes. - name: Refuse to repair a complete release env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} @@ -181,25 +210,13 @@ jobs: - name: Setup Gradle uses: gradle/actions/setup-gradle@v3 - # Recorded BEFORE the build so the after-check below compares against a - # pointer state this run cannot have touched yet. - - name: Record the live pointer releases - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - set -euo pipefail - - for pointer in latest latest-prerelease; do - if ! gh api "/repos/$GITHUB_REPOSITORY/releases/tags/$pointer" \ - | jq -S '{tag_name, name, target_commitish, created_at, published_at, - assets: [.assets[] | {name, size, updated_at}] | sort_by(.name)}' \ - > "/tmp/pointer-$pointer.before.json"; then - echo "::error::Could not read the $pointer release; refusing to run blind." - exit 1 - fi - echo "--- $pointer (before) ---" - cat "/tmp/pointer-$pointer.before.json" - done + # --------------------------------------------------------------------- + # Everything from here on runs, or runs after, UNTRUSTED TAGGED SOURCE. + # No step below holds a write capability, and no step below may be given + # one: the tagged build can plant a tool on $GITHUB_PATH that a later + # step in this job would execute. That is why the release write lives in + # a separate job rather than in a later step of this one. + # --------------------------------------------------------------------- # The same build the tag's own release path ran. A repaired release must # not carry weaker provenance than one published on the normal path. @@ -228,21 +245,142 @@ jobs: ls -la build/release/ + - name: Hand the assets to the publish job + uses: actions/upload-artifact@v4 + with: + name: ${{ env.ASSET_ARTIFACT }} + path: build/release/ + if-no-files-found: error + retention-days: 1 + + # Holds the ONLY write capability in this workflow. It checks nothing out and + # runs no tag code, so there is no tagged source in this job to escalate it. + publish: + needs: build + runs-on: ubuntu-latest + timeout-minutes: 30 + permissions: + contents: write # GitHub Release asset upload. The ONLY write in this file. + # `packages` and every other registry scope are deliberately absent and + # must stay absent. Without them, pushing a build anywhere but this + # release's own asset list is not forbidden here, it is unrepresentable. + env: + RELEASE_TAG: ${{ inputs.release_tag }} + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + ASSET_DIR: release-assets + steps: + # Repeated here on purpose rather than relied upon from the build job: + # this is the job that holds the write token, so the refusal that + # protects the pointer releases belongs where the capability is. + - name: Refuse to repair a pointer release + run: | + set -euo pipefail + + case "$RELEASE_TAG" in + latest | latest-prerelease) + echo "::error::$RELEASE_TAG is a live pointer release, not a version." + echo "::error::Repairing it would republish old jars at the stable download URLs" + echo "::error::and downgrade every consumer of them. Refusing." + exit 1 + ;; + esac + + if ! printf '%s' "$RELEASE_TAG" | grep -Eq '^[0-9]+\.[0-9]+\.[0-9]+$'; then + echo "::error::$RELEASE_TAG is not a connect-java version tag (expected N.N.N)." + exit 1 + fi + + - name: Collect the built assets + uses: actions/download-artifact@v4 + with: + name: ${{ env.ASSET_ARTIFACT }} + path: ${{ env.ASSET_DIR }} + + # The artifact was produced by a job that ran untrusted tagged source, so + # its FILE NAMES are untrusted input to this job: they become the names of + # public release assets. Only the jars a connect release is supposed to + # carry, plus LICENSE, may pass. Regular files only - a symlink or a + # nested path is not something a repair produces. + - name: Validate the downloaded assets + run: | + set -euo pipefail + + if [ ! -d "$ASSET_DIR" ]; then + echo "::error::The build job produced no asset directory." + exit 1 + fi + + REJECTED="" + ACCEPTED="" + JARS=0 + while IFS= read -r path; do + base="$(basename "$path")" + if [ -L "$path" ] || [ ! -f "$path" ] || [ ! -s "$path" ]; then + REJECTED="$REJECTED $base" + continue + fi + if [ "$path" != "$ASSET_DIR/$base" ]; then + REJECTED="$REJECTED $path" + continue + fi + case "$base" in + LICENSE) ACCEPTED="$ACCEPTED $base" ;; + *) + if printf '%s' "$base" \ + | grep -Eq '^connect-(spigot|velocity|bungee)[A-Za-z0-9._-]*\.jar$'; then + ACCEPTED="$ACCEPTED $base" + JARS=$((JARS + 1)) + else + REJECTED="$REJECTED $base" + fi + ;; + esac + done < <(find "$ASSET_DIR" -mindepth 1) + + echo "Accepted:$ACCEPTED" + + if [ -n "$REJECTED" ]; then + echo "::error::The build job produced asset(s) a repair must not publish:$REJECTED" + echo "::error::Only connect-*.jar and LICENSE may become release assets." + exit 1 + fi + + if [ "$JARS" -eq 0 ]; then + echo "::error::The build job produced no plugin jar; there is nothing to repair with." + exit 1 + fi + + # Recorded before this job writes anything, and the build job cannot + # write at all, so this baseline predates every write the run performs. + - name: Record the live pointer releases + run: | + set -euo pipefail + + for pointer in latest latest-prerelease; do + if ! gh api "/repos/$GITHUB_REPOSITORY/releases/tags/$pointer" \ + | jq -S '{tag_name, name, target_commitish, created_at, published_at, + assets: [.assets[] | {name, size, updated_at}] | sort_by(.name)}' \ + > "/tmp/pointer-$pointer.before.json"; then + echo "::error::Could not read the $pointer release; refusing to run blind." + exit 1 + fi + echo "--- $pointer (before) ---" + cat "/tmp/pointer-$pointer.before.json" + done + # Repairing an existing release must not go through the `softprops` # gh-release action that release.yml uses for fresh publishes: it PATCHes # /releases/{id} to sync release metadata BEFORE uploading anything, which # rewrites historical release notes. `gh release upload` only POSTs to # /releases/{id}/assets, and only for the tag named here. - name: Upload the missing assets - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | set -euo pipefail INITIAL_RELEASE_JSON=$(gh api "/repos/$GITHUB_REPOSITORY/releases/tags/$RELEASE_TAG") FILES=() - for f in build/release/*; do + for f in "$ASSET_DIR"/*; do [ -f "$f" ] || continue FILES+=("$f") done @@ -252,9 +390,12 @@ jobs: exit 1 fi - # GUARD 2: never clobber an existing GOOD asset. Only names that are - # absent, or present in a non-uploaded / zero-byte (failed upload) - # state, are eligible; --clobber then applies solely to those holes. + # GUARD 2: never clobber an existing GOOD asset. A name that is + # absent is uploaded WITHOUT --clobber, so GitHub itself rejects the + # POST if a concurrent writer published that name in the meantime - + # that rejection is the atomic conditional the REST API does not + # otherwise offer. --clobber is reserved for names that are present + # but BROKEN, where replacing already-unusable bytes is the point. RELEASE_JSON=$(gh api "/repos/$GITHUB_REPOSITORY/releases/tags/$RELEASE_TAG") ABSENT_LIST=() BROKEN_LIST=() @@ -317,8 +458,6 @@ jobs: # re-read the PUBLISHED release from the API and assert on the landed # fact. Never conditional: the guard must not be skippable into silence. - name: Verify published release assets - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | set -euo pipefail @@ -411,8 +550,6 @@ jobs: # asset check above refuses to trust its own upload step. - name: Verify no pointer release moved if: always() - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | set -euo pipefail @@ -420,7 +557,7 @@ jobs: for pointer in latest latest-prerelease; do BEFORE="/tmp/pointer-$pointer.before.json" if [ ! -f "$BEFORE" ]; then - echo "No recorded state for $pointer (the run stopped before the build); nothing to compare." + echo "No recorded state for $pointer (the run stopped before the upload); nothing to compare." continue fi gh api "/repos/$GITHUB_REPOSITORY/releases/tags/$pointer" \ @@ -443,5 +580,6 @@ jobs: exit 1 fi - echo "NOTE: no package registry was touched. This workflow holds contents:write only" - echo "NOTE: and requests no registry scope, so publishing elsewhere is unrepresentable here." + echo "NOTE: no package registry was touched, and the job that ran the tagged" + echo "NOTE: build held no write capability at all. Publishing elsewhere is" + echo "NOTE: unrepresentable in this workflow, not merely disallowed." diff --git a/AGENTS.md b/AGENTS.md index f583eb2c..56722a08 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,13 +45,21 @@ curl -I -L --fail https://github.com/minekube/connect-java/releases/download/These tests pin the capability boundary that makes that unrepresentable rather than merely - * discouraged: {@code contents: write} and nothing else, no registry scope, no secret beyond the - * job's own {@code GITHUB_TOKEN}, and no release named other than the one being repaired. A comment - * saying so is an argument; this is the check that can fail. + *

A repair necessarily executes old tagged source - a Gradle build and whatever its + * plugins resolve - that nobody reviewed as part of the repair. So the security question is not + * whether that build is trustworthy but what it can reach, and the answer cannot be "we scoped the + * token to individual steps": within one job a build step can append to {@code $GITHUB_PATH} or + * {@code $GITHUB_ENV} and have a later token-bearing step in that same job execute a tool it + * planted. Step-level {@code env:} is not a sandbox. The job is the boundary. + * + *

Hence the split these tests pin: a {@code build} job with {@code contents: read} that runs the + * tag, and a {@code publish} job with {@code contents: write} that checks nothing out and runs none + * of it. Without that split, a compromised historical build could reach a write-scoped token and + * move the live {@code latest} release - the stable {@code releases/download/latest/*.jar} URLs + * download sites hand out - silently downgrading every consumer. A comment claiming the boundary is + * an argument; these are the checks that can fail. */ class ReleaseRepairCapabilityTest { @@ -64,10 +69,18 @@ class ReleaseRepairCapabilityTest { Paths.get("..", ".github", "workflows", "release-repair.yml"); private static final Path REPOSITORY_GIT_PATH = Paths.get("..", ".git"); + /** The job that runs untrusted tagged source. It must hold no write capability. */ + private static final String BUILD_JOB = "build"; + + /** The job that holds the only write capability. It must run no tagged source. */ + private static final String PUBLISH_JOB = "publish"; + /** The live pointer releases whose download URLs are published to users. */ private static final List POINTER_RELEASES = Arrays.asList("latest", "latest-prerelease"); + private static final String POINTER_REFUSAL = "Refuse to repair a pointer release"; + private static String workflowText() throws Exception { if (!Files.exists(WORKFLOW_PATH)) { if (Files.exists(REPOSITORY_GIT_PATH)) { @@ -82,8 +95,7 @@ private static String workflowText() throws Exception { @SuppressWarnings("unchecked") private static Map workflow() throws Exception { - String text = workflowText(); - if (text.isEmpty()) { + if (workflowText().isEmpty()) { return new LinkedHashMap<>(); } try (InputStream in = Files.newInputStream(WORKFLOW_PATH)) { @@ -92,24 +104,26 @@ private static Map workflow() throws Exception { } @SuppressWarnings("unchecked") - private static Map repairJob() throws Exception { + private static Map job(String name) throws Exception { Map workflow = workflow(); if (workflow.isEmpty()) { return new LinkedHashMap<>(); } Map jobs = (Map) workflow.get("jobs"); - assertTrue(jobs != null && jobs.containsKey("repair"), "repair job is missing"); - return (Map) jobs.get("repair"); + assertTrue(jobs != null && jobs.containsKey(name), + "the " + name + " job is missing; the untrusted build and the write capability " + + "must live in separate jobs"); + return (Map) jobs.get(name); } @SuppressWarnings("unchecked") - private static List> steps() throws Exception { - Map job = repairJob(); + private static List> steps(String jobName) throws Exception { + Map job = job(jobName); if (job.isEmpty()) { return List.of(); } List> steps = (List>) job.get("steps"); - assertTrue(steps != null && !steps.isEmpty(), "repair job has no steps"); + assertTrue(steps != null && !steps.isEmpty(), jobName + " job has no steps"); return steps; } @@ -124,45 +138,161 @@ private static int stepIndex(List> steps, String name) { private static String stepScript(List> steps, String name) { int at = stepIndex(steps, name); - assertTrue(at >= 0, "repair job is missing the \"" + name + "\" step"); + assertTrue(at >= 0, "missing the \"" + name + "\" step"); Object run = steps.get(at).get("run"); assertTrue(run instanceof String && !((String) run).isEmpty(), "\"" + name + "\" must be a run step"); return (String) run; } + @SuppressWarnings("unchecked") + private static Map stepEnv(Map step) { + Object env = step.get("env"); + return env instanceof Map ? (Map) env : Map.of(); + } + + private static String stepLabel(Map step) { + Object name = step.get("name"); + return name != null ? String.valueOf(name) : String.valueOf(step.get("uses")); + } + + /** Every `run` script in a job, concatenated - for whole-job content assertions. */ + private static String allScripts(List> steps) { + StringBuilder sb = new StringBuilder(); + for (Map step : steps) { + Object run = step.get("run"); + if (run instanceof String) { + sb.append((String) run).append('\n'); + } + } + return sb.toString(); + } + /** - * The boundary itself. A repair that could also push to a package registry would be able to - * republish an old build as something users pull by default - the exact silent downgrade this - * workflow exists to avoid. Absent capability beats a guarded one. + * THE split. This is the check that fails if anybody ever "simplifies" the workflow back into a + * single job, or grants the build job a write scope, or teaches the publish job to check + * something out. */ @Test @SuppressWarnings("unchecked") - void repairGrantsContentsWriteAndNothingElse() throws Exception { + void repairSplitsUntrustedBuildFromTheWriteCapability() throws Exception { Map workflow = workflow(); assumeTrue(!workflow.isEmpty()); - Object permissions = workflow.get("permissions"); - assertTrue(permissions instanceof Map, - "release-repair.yml must declare an explicit permissions block; inheriting the " - + "repository default would hand the repair whatever scopes happen to be " - + "enabled"); - - Map granted = (Map) permissions; - assertEquals(Map.of("contents", "write"), granted, - "release-repair.yml must grant contents:write and nothing else; it grants " - + granted); - - // A job-level block would silently widen the workflow-level grant. - Object jobPermissions = repairJob().get("permissions"); - assertTrue(jobPermissions == null || Map.of("contents", "write").equals(jobPermissions), - "the repair job widens the workflow permissions to " + jobPermissions); + // Nothing may be inherited: an implicit repository default would hand both jobs whatever + // scopes happen to be enabled org-wide. + Object topLevel = workflow.get("permissions"); + assertTrue(topLevel instanceof Map && ((Map) topLevel).isEmpty(), + "top-level permissions must be {} so neither job inherits anything; found " + + topLevel); + + Map build = job(BUILD_JOB); + Map publish = job(PUBLISH_JOB); + + assertEquals(Map.of("contents", "read"), build.get("permissions"), + "the build job runs untrusted tagged source and must hold contents:read only; " + + "it holds " + build.get("permissions")); + assertEquals(Map.of("contents", "write"), publish.get("permissions"), + "the publish job must hold contents:write and nothing else; it holds " + + publish.get("permissions")); + + // Belt and braces: no job may hold any write scope other than the publish job's contents. + for (Object value : ((Map) build.get("permissions")).values()) { + assertTrue(!"write".equals(value), + "the build job grants a write scope; tagged source could reach it"); + } + + Object needs = publish.get("needs"); + List needsList = needs instanceof List + ? (List) needs : List.of(String.valueOf(needs)); + assertTrue(needsList.contains(BUILD_JOB), + "the publish job must depend on the build job; it needs " + needs); + + // The publish job must never materialise the tag: no checkout, no toolchain, no Gradle. + List> publishSteps = steps(PUBLISH_JOB); + for (Map step : publishSteps) { + Object uses = step.get("uses"); + if (uses instanceof String) { + String action = (String) uses; + assertTrue(!action.startsWith("actions/checkout@"), + "the publish job checks out source; it holds the write token and must " + + "never materialise the tag"); + assertTrue(!action.startsWith("actions/setup-java@") + && !action.startsWith("gradle/"), + "the publish job sets up a build toolchain (" + action + "); it must run " + + "none of the tagged source"); + } + } + String publishScripts = allScripts(publishSteps); + assertTrue(!publishScripts.contains("gradlew"), + "the publish job invokes Gradle; the write capability must never run tag code"); + + // And the write itself must live only there. + assertTrue(!allScripts(steps(BUILD_JOB)).contains("gh release upload"), + "the build job uploads release assets; the write must live in the publish job"); + assertTrue(publishScripts.contains("gh release upload"), + "the publish job never uploads any asset"); } /** - * The scope check above only proves the file does not name a registry permission. This - * proves it does not reach one another way: a registry login action, a docker/gradle publish, or - * a credential smuggled in through a secret other than the job's own GITHUB_TOKEN. + * The {@code $GITHUB_PATH} vector specifically. Even inside the read-only build job, a + * token-bearing step placed after the tagged build could be made to execute a tool the + * build planted. Nothing in that job may carry a credential once tag code has run. + */ + @Test + void buildJobHoldsNoCredentialOnceTaggedCodeHasRun() throws Exception { + List> build = steps(BUILD_JOB); + assumeTrue(!build.isEmpty()); + + // A job-level token would be visible to the tagged build itself. + Object jobEnv = job(BUILD_JOB).get("env"); + if (jobEnv instanceof Map) { + assertTrue(!((Map) jobEnv).containsKey("GH_TOKEN"), + "the build job declares a job-level GH_TOKEN; the tagged build would see it"); + } + + int buildAt = stepIndex(build, "Build"); + assertTrue(buildAt >= 0, "the build job is missing the Build step"); + + List offenders = new ArrayList<>(); + for (int i = buildAt; i < build.size(); i++) { + if (stepEnv(build.get(i)).containsKey("GH_TOKEN")) { + offenders.add(stepLabel(build.get(i))); + } + } + assertTrue(offenders.isEmpty(), + "step(s) " + offenders + " carry GH_TOKEN at or after the tagged build; the build " + + "can plant a tool on $GITHUB_PATH that a later step in the same job " + + "executes, so no credential may follow it"); + } + + /** + * The artifact crossing the split was produced by a job that ran untrusted source, so its file + * names are untrusted input: they become the names of public release assets. Only what + * a connect release is supposed to carry may pass. + */ + @Test + void publishJobValidatesTheHandoverArtifact() throws Exception { + List> publish = steps(PUBLISH_JOB); + assumeTrue(!publish.isEmpty()); + + String validate = stepScript(publish, "Validate the downloaded assets"); + assertTrue(validate.contains("^connect-(spigot|velocity|bungee)"), + "the publish job does not constrain which asset names may be published"); + assertTrue(validate.contains("exit 1"), "the asset-name validation cannot fail the run"); + + int validateAt = stepIndex(publish, "Validate the downloaded assets"); + int uploadAt = stepIndex(publish, "Upload the missing assets"); + assertTrue(uploadAt >= 0, "the publish job is missing the upload step"); + assertTrue(validateAt < uploadAt, + "the handover artifact is validated after it is uploaded; that is not validation"); + } + + /** + * The registry half of the boundary. The permission checks above only prove the file does not + * name a registry scope; this proves it does not reach one another way - a registry + * login action, a docker/gradle publish, or a credential smuggled in through a secret other + * than the job's own GITHUB_TOKEN. */ @Test void repairNeverPublishesToAPackageRegistry() throws Exception { @@ -211,25 +341,27 @@ void repairNeverPublishesToAPackageRegistry() throws Exception { /** * {@code latest} and {@code latest-prerelease} are not versions, they are the live pointers * whose download URLs the release notes advertise. Repairing one would republish an old build at - * a stable URL and downgrade everyone using it, so the workflow must refuse the names outright - * and must never name any release but the one being repaired. + * a stable URL and downgrade everyone using it, so both jobs refuse the names outright - the + * publish job most of all, because it is the one holding the write token. */ @Test void repairCannotWriteAPointerRelease() throws Exception { String text = workflowText(); assumeTrue(!text.isEmpty()); - List> steps = steps(); - String refusal = stepScript(steps, "Refuse to repair a pointer release"); - for (String pointer : POINTER_RELEASES) { - assertTrue(refusal.contains(pointer), - "the pointer refusal does not reject \"" + pointer + "\""); + for (String jobName : List.of(BUILD_JOB, PUBLISH_JOB)) { + List> jobSteps = steps(jobName); + String refusal = stepScript(jobSteps, POINTER_REFUSAL); + for (String pointer : POINTER_RELEASES) { + assertTrue(refusal.contains(pointer), + "the " + jobName + " job's pointer refusal does not reject \"" + pointer + + "\""); + } + assertTrue(refusal.contains("exit 1"), + "the " + jobName + " job's pointer refusal does not fail the run"); + assertEquals(0, stepIndex(jobSteps, POINTER_REFUSAL), + "the pointer refusal must be the first step of the " + jobName + " job"); } - assertTrue(refusal.contains("exit 1"), "the pointer refusal does not fail the run"); - - // It has to run before anything is built or uploaded, otherwise it refuses too late. - assertEquals(0, stepIndex(steps, "Refuse to repair a pointer release"), - "the pointer refusal must be the first step"); // release.yml writes `tag_name: latest`. Nothing in a repair may. assertTrue(!Pattern.compile("(?m)tag_name:\\s*latest").matcher(text).find(), @@ -253,10 +385,10 @@ void repairCannotWriteAPointerRelease() throws Exception { */ @Test void repairRefusesToClobberACompleteRelease() throws Exception { - List> steps = steps(); - assumeTrue(!steps.isEmpty()); + List> build = steps(BUILD_JOB); + assumeTrue(!build.isEmpty()); - String guard = stepScript(steps, "Refuse to repair a complete release"); + String guard = stepScript(build, "Refuse to repair a complete release"); assertTrue(guard.contains("/releases/tags/"), "the completeness guard must read the published release, not local state"); assertTrue(guard.contains("connect-(spigot|velocity|bungee)"), @@ -264,38 +396,25 @@ void repairRefusesToClobberACompleteRelease() throws Exception { + "only LICENSE has a positive asset count and is still a hole"); assertTrue(guard.contains("exit 1"), "the completeness guard does not fail the run"); - int guardAt = stepIndex(steps, "Refuse to repair a complete release"); - int buildAt = stepIndex(steps, "Build"); - assertTrue(buildAt >= 0, "repair job is missing the Build step"); + int guardAt = stepIndex(build, "Refuse to repair a complete release"); + int buildAt = stepIndex(build, "Build"); + assertTrue(buildAt >= 0, "the build job is missing the Build step"); assertTrue(guardAt < buildAt, "the completeness guard runs after the build; it must refuse before rebuilding"); - // Guard 2: --clobber must apply only to holes, never to a published good asset. - String upload = stepScript(steps, "Upload the missing assets"); - assertTrue(upload.contains("--clobber"), "the upload does not use --clobber"); + // Guard 2, in the publish job: an ABSENT name uploads without --clobber, so GitHub's own + // rejection is the atomic conditional; --clobber may only reach a name already broken. + String upload = stepScript(steps(PUBLISH_JOB), "Upload the missing assets"); + assertTrue(upload.contains("--clobber"), "the upload does not use --clobber at all"); + assertTrue(upload.contains("ABSENT_LIST") && upload.contains("BROKEN_LIST"), + "the upload does not separate absent names from broken ones; --clobber would " + + "then be able to reach an asset a concurrent writer just published"); + assertTrue(Pattern.compile("gh release upload \"\\$RELEASE_TAG\" \"\\$\\{ABSENT_LIST\\[@\\]}\"" + + "(?!.*--clobber)").matcher(upload).find(), + "absent assets are uploaded with --clobber; that upload must be unconditional-" + + "free so GitHub rejects a name published concurrently"); assertTrue(upload.contains("state == \"uploaded\"") && upload.contains("size > 0"), - "the upload does not exclude already-good assets from --clobber"); - assertTrue(upload.contains("INITIAL_RELEASE_JSON"), - "the upload does not retain the initial release snapshot"); - assertTrue(upload.contains("initial_asset_count"), - "the upload does not distinguish initially absent assets"); - assertTrue(upload.contains("[ \"$initial_asset_count\" -eq 0 ]"), - "the upload misclassifies assets that appeared during the repair"); - assertTrue(upload.contains("ABSENT_LIST=()") && upload.contains("BROKEN_LIST=()"), - "the upload does not separate absent assets from broken assets"); - int rereadAt = upload.indexOf("RELEASE_JSON=$(gh api"); - int decisionAt = upload.indexOf("for f in \"${FILES[@]}\""); - assertTrue(rereadAt >= 0 && decisionAt > rereadAt, - "the upload does not re-read the release immediately before deciding what to " - + "clobber"); - assertTrue(upload.contains("PUBLISHED_DURING_REPAIR"), - "the upload does not report a newly published good asset that it skips"); - int absentUploadAt = upload.indexOf( - "gh release upload \"$RELEASE_TAG\" \"${ABSENT_LIST[@]}\" --repo \"$GITHUB_REPOSITORY\""); - int brokenUploadAt = upload.indexOf( - "gh release upload \"$RELEASE_TAG\" \"${BROKEN_LIST[@]}\" --clobber --repo \"$GITHUB_REPOSITORY\""); - assertTrue(absentUploadAt >= 0 && brokenUploadAt > absentUploadAt, - "the upload does not use the atomic no-clobber path for absent assets"); + "the upload does not classify already-good assets out of the clobber set"); } /** @@ -304,11 +423,12 @@ void repairRefusesToClobberACompleteRelease() throws Exception { * wrong as a false green, and here it would be recorded as "unrecoverable". */ @Test + @SuppressWarnings("unchecked") void repairBuildsAtTheTagsOwnPinnedToolchain() throws Exception { - List> steps = steps(); - assumeTrue(!steps.isEmpty()); + List> build = steps(BUILD_JOB); + assumeTrue(!build.isEmpty()); - String resolve = stepScript(steps, "Resolve the tag's pinned Java toolchain"); + String resolve = stepScript(build, "Resolve the tag's pinned Java toolchain"); assertTrue(resolve.contains(".github/workflows/release.yml"), "the toolchain must be read from the tag's own release workflow"); assertTrue(resolve.contains("java-version"), "the toolchain step reads no Java version"); @@ -316,12 +436,12 @@ void repairBuildsAtTheTagsOwnPinnedToolchain() throws Exception { "the toolchain step falls back to a default instead of failing; guessing a JDK " + "produces a false result about whether the tag builds"); - int at = stepIndex(steps, "Set up the tag's JDK"); - assertTrue(at >= 0, "repair job is missing the JDK setup step"); - @SuppressWarnings("unchecked") - Map with = (Map) steps.get(at).get("with"); + int at = stepIndex(build, "Set up the tag's JDK"); + assertTrue(at >= 0, "the build job is missing the JDK setup step"); + Map with = (Map) build.get(at).get("with"); assertTrue(with != null, "the JDK setup step has no inputs"); - assertEquals("${{ steps.toolchain.outputs.java-version }}", String.valueOf(with.get("java-version")), + assertEquals("${{ steps.toolchain.outputs.java-version }}", + String.valueOf(with.get("java-version")), "the JDK is hard-coded instead of taken from the tag's pin"); // The tag's checkout supplies its own Gradle wrapper; a pinned Gradle here would override it. @@ -336,10 +456,10 @@ void repairBuildsAtTheTagsOwnPinnedToolchain() throws Exception { */ @Test void repairVerifiesTheLandedRelease() throws Exception { - List> steps = steps(); - assumeTrue(!steps.isEmpty()); + List> publish = steps(PUBLISH_JOB); + assumeTrue(!publish.isEmpty()); - String verify = stepScript(steps, "Verify published release assets"); + String verify = stepScript(publish, "Verify published release assets"); List required = Arrays.asList( "gh api", // reads the API, not the local build output @@ -356,84 +476,40 @@ void repairVerifiesTheLandedRelease() throws Exception { + "; it must assert on the published release, not on local build output"); assertTrue(!verify.contains("build/libs") && !verify.contains("build/release"), - "the repair verification reads local build output; those jars exist even when " + "the repair verification reads local build output; those files exist even when " + "the upload never happened"); // Wrong-artifact-type is its own failure mode and the more dangerous one, because the - // release looks populated. LICENSE alone must not satisfy the guard. + // release looks populated. A positive asset count must not be satisfiable by LICENSE, a + // checksum manifest, an SBOM or a source archive: only a real plugin jar counts. assertTrue(verify.contains("^connect-(spigot|velocity|bungee).*\\\\.jar$"), - "the repair verification does not require a plugin jar asset by name"); - assertTrue(verify.contains("size > 0"), - "the repair verification counts zero-byte plugin assets"); + "the landed check does not require a real plugin jar by name; an asset such as " + + "source.tar.gz could satisfy it while no build landed"); assertTrue(Pattern.compile("BUILD_COUNT\"\\s*-eq\\s*0").matcher(verify).find(), "the repair verification does not fail on a zero build-artifact count"); // Unconditional: a guard that can be gated off is not a guard. - assertTrue(steps.get(stepIndex(steps, "Verify published release assets")).get("if") == null, + assertTrue(publish.get(stepIndex(publish, "Verify published release assets")).get("if") == null, "the repair verification is conditional; it must always run"); - int verifyAt = stepIndex(steps, "Verify published release assets"); - int uploadAt = stepIndex(steps, "Upload the missing assets"); - assertTrue(verifyAt > uploadAt, + assertTrue(stepIndex(publish, "Verify published release assets") + > stepIndex(publish, "Upload the missing assets"), "the repair verification runs before the upload; it would see the old release"); } - @Test - @SuppressWarnings("unchecked") - void repairKeepsTheWriteTokenOutOfTaggedBuildLogic() throws Exception { - Map job = repairJob(); - assumeTrue(!job.isEmpty()); - - Map jobEnv = (Map) job.get("env"); - assertTrue(jobEnv == null || !jobEnv.containsKey("GH_TOKEN"), - "the write token is exposed to every step in the repair job"); - - List> steps = steps(); - Map build = steps.get(stepIndex(steps, "Build")); - Map buildEnv = (Map) build.get("env"); - assertTrue(buildEnv == null || !buildEnv.containsKey("GH_TOKEN"), - "the tagged build can access the write-scoped GitHub token"); - - Map checkout = null; - for (Map step : steps) { - Object uses = step.get("uses"); - if (uses instanceof String && ((String) uses).startsWith("actions/checkout@")) { - checkout = step; - break; - } - } - assertTrue(checkout != null, "release-repair.yml never checks out the repository"); - Map checkoutWith = (Map) checkout.get("with"); - assertEquals(Boolean.FALSE, checkoutWith.get("persist-credentials"), - "the checkout leaves the write token in the tagged build's git config"); - - List apiSteps = Arrays.asList( - "Refuse to repair a complete release", - "Record the live pointer releases", - "Upload the missing assets", - "Verify published release assets", - "Verify no pointer release moved"); - for (String name : apiSteps) { - Map env = - (Map) steps.get(stepIndex(steps, name)).get("env"); - assertEquals("${{ secrets.GITHUB_TOKEN }}", env.get("GH_TOKEN"), - "the API step " + name + " does not receive the scoped GitHub token"); - } - } - /** * "The workflow only names $RELEASE_TAG, so it cannot move a pointer" is an argument about the - * source. This is the check on the landed fact: record both pointer releases before the build - * and assert they are unchanged afterwards, so a move shows up as a red run rather than as a - * server owner's surprise downgrade. + * source. This is the check on the landed fact: record both pointer releases before anything is + * written and assert they are unchanged afterwards, so a move shows up as a red run rather than + * as a server owner's surprise downgrade. */ @Test void repairProvesNoPointerReleaseMoved() throws Exception { - List> steps = steps(); - assumeTrue(!steps.isEmpty()); + List> publish = steps(PUBLISH_JOB); + assumeTrue(!publish.isEmpty()); - String before = stepScript(steps, "Record the live pointer releases"); - String after = stepScript(steps, "Verify no pointer release moved"); + String before = stepScript(publish, "Record the live pointer releases"); + String after = stepScript(publish, "Verify no pointer release moved"); for (String pointer : POINTER_RELEASES) { assertTrue(before.contains(pointer) && after.contains(pointer), "the pointer-immutability check does not cover \"" + pointer + "\""); @@ -444,13 +520,12 @@ void repairProvesNoPointerReleaseMoved() throws Exception { assertTrue(after.contains("diff"), "the pointer check never compares before against after"); assertTrue(after.contains("exit 1"), "the pointer check cannot fail the run"); - int recordAt = stepIndex(steps, "Record the live pointer releases"); - int buildAt = stepIndex(steps, "Build"); - int uploadAt = stepIndex(steps, "Upload the missing assets"); - int checkAt = stepIndex(steps, "Verify no pointer release moved"); - assertTrue(recordAt < buildAt, - "the pointer state is recorded after the build; the baseline must predate any " - + "work this run does"); + int recordAt = stepIndex(publish, "Record the live pointer releases"); + int uploadAt = stepIndex(publish, "Upload the missing assets"); + int checkAt = stepIndex(publish, "Verify no pointer release moved"); + assertTrue(recordAt < uploadAt, + "the pointer baseline is recorded after the upload; it must predate every write " + + "this run performs"); assertTrue(checkAt > uploadAt, "the pointer check runs before the upload and would never observe a move"); } @@ -483,23 +558,26 @@ void repairIsDispatchedByTagAndChecksThatTagOut() throws Exception { assertEquals(Boolean.TRUE, ((Map) inputs.get("release_tag")).get("required"), "release_tag must be required; an empty tag would repair the dispatch ref"); - List> steps = steps(); + List> build = steps(BUILD_JOB); Map checkout = null; - for (Map step : steps) { + for (Map step : build) { Object uses = step.get("uses"); if (uses instanceof String && ((String) uses).startsWith("actions/checkout@")) { checkout = step; break; } } - assertTrue(checkout != null, "release-repair.yml never checks out the repository"); + assertTrue(checkout != null, "the build job never checks out the repository"); Map with = (Map) checkout.get("with"); assertTrue(with != null, "the checkout step has no inputs"); assertEquals("${{ inputs.release_tag }}", String.valueOf(with.get("ref")), "the checkout must build the requested tag, not the dispatch ref"); + assertEquals(Boolean.FALSE, with.get("persist-credentials"), + "the checkout persists credentials into .git/config, handing the tagged Gradle " + + "build a usable token"); // Provenance: the checked-out HEAD must be proven to be the tag, not a branch of that name. - String provenance = stepScript(steps, "Confirm the checkout is the tagged commit"); + String provenance = stepScript(build, "Confirm the checkout is the tagged commit"); assertTrue(provenance.contains("refs/tags/") && provenance.contains("exit 1"), "the repair does not prove it built the tagged commit"); } From e8d6954b051d366dd3191914e458550f78f3f077 Mon Sep 17 00:00:00 2001 From: "no-mistakes[bot]" Date: Mon, 27 Jul 2026 11:17:25 +0200 Subject: [PATCH 6/6] no-mistakes(document): Clarified repair docs and verified workflow lint --- .github/workflows/release-repair.yml | 5 +++-- AGENTS.md | 24 +++++++++++------------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/.github/workflows/release-repair.yml b/.github/workflows/release-repair.yml index cf78241f..8caef243 100644 --- a/.github/workflows/release-repair.yml +++ b/.github/workflows/release-repair.yml @@ -18,8 +18,9 @@ name: release-repair # https://github.com/minekube/connect-java/releases/download/latest/*.jar # that download sites and server owners point at. Re-running it at an OLD # tag would drag `latest` backwards and silently downgrade everyone using -# those URLs. This workflow never names a release other than the one being -# repaired, and refuses the pointer tags outright. +# those URLs. This workflow never WRITES a release other than the one being +# repaired; it reads the pointers for immutability checks and refuses them +# as repair targets outright. # # WHY TWO JOBS, and why that is the whole security model: # diff --git a/AGENTS.md b/AGENTS.md index 56722a08..7ea2bdd4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -36,10 +36,11 @@ gh -R minekube/connect-java release view --json tagName,targetCommitis curl -I -L --fail https://github.com/minekube/connect-java/releases/download//connect-velocity.jar ``` -- `release.yml`'s "Verify published release assets" step now enforces this in CI: - it re-reads each published release from the API (never the upload step's own - output) and fails when no real downloadable build jar landed, so metadata-only - assets like `LICENSE` cannot pass as a release. Pinned by +- `release.yml`'s "Verify published release assets" step re-reads each published + release from the API (never the upload step's own output) and rejects + metadata-only assets such as `LICENSE`. Its broad negative filter is + intentionally weaker than `release-repair.yml`'s positive plugin-jar + allowlist; tightening it is a separate follow-up. Pinned by `core/.../release/ReleaseAssetVerificationTest`; keep that test's step and upload-step names in sync when editing `release.yml`. @@ -51,15 +52,12 @@ curl -I -L --fail https://github.com/minekube/connect-java/releases/download/