From ca99182e80c429404f4aabe0596f7c00c3c084b3 Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Tue, 4 Aug 2026 12:11:06 +0000 Subject: [PATCH] fix(forge): percent-encode asset names, and stop the docs naming a client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings are @codex's on !193 (#1583), and both are real. The asset name travels as a QUERY VALUE, and the artifact-hook contract permits any file the consumer drops in RELEASE_ASSETS_DIR. Raw interpolation meant `release asset.tgz` made curl reject the URL outright (exit 3), and '&', '#', '+', '%' silently changed the name or the query's shape. `gh release create` handled all of those, so a 1:1 port had to. Encoded through one boundary — jq's @uri, since jq is already a hard dependency of this backend and a hand-rolled sed class is how the next unescaped character gets through. Six backend cases cover it: the encoder on a space and on the delimiters, uploads under both names, the created release id in the path, and the multipart attachment. Mutation-checked: dropping the encoder fails exactly the two name assertions. docs/CONSUMERS.md's artifact-hook recovery still told operators to "run `gh release create` by hand" and described the hook as running "before `gh release create`" — on a Forgejo runner that is precisely the failure this PR fixes. It now names the forge-neutral tag-door recovery first and shows both clients for the manual path, without regressing the GitHub guidance. 1035 assertions, 22 suites, shellcheck-all and actionlint clean. Refs #191 Co-Authored-By: Claude Opus 5 (1M context) --- changelog.d/191.md | 11 +++++++++++ docs/CONSUMERS.md | 30 +++++++++++++++++++++++++----- lib/forge-forgejo.sh | 17 +++++++++++++++-- test/forge-backends.test.sh | 28 +++++++++++++++++++++++++++- 4 files changed, 78 insertions(+), 8 deletions(-) diff --git a/changelog.d/191.md b/changelog.d/191.md index 89cf80f..4ca1fcd 100644 --- a/changelog.d/191.md +++ b/changelog.d/191.md @@ -26,3 +26,14 @@ - `forgejo_api_base` refuses when `REPO` is empty. Every verb interpolates it and every call reaches the network through there, so `repos//…` — whose 404 reads as "no release" and "no PRs" — is now impossible (#191). + +- Release asset names are percent-encoded. The hook contract permits any + filename, and the name travels as a query value: a space made curl reject + the URL and `&`/`#`/`+`/`%` silently renamed the asset (#191). + +### Changed + +- `docs/CONSUMERS.md`'s artifact-hook recovery no longer tells operators to + run `gh release create` by hand — on a Forgejo runner there is no `gh`. + It names the forge-neutral tag-door path first, with both clients shown + (#191). diff --git a/docs/CONSUMERS.md b/docs/CONSUMERS.md index 672f09f..fb244c2 100644 --- a/docs/CONSUMERS.md +++ b/docs/CONSUMERS.md @@ -247,8 +247,9 @@ tag door instead (the known first-release edge, cast#111). ### The artifact hook If the repository contains `.github/actions/release-artifact/action.yml`, -both doors invoke it — after the tag exists, before `gh release create` — -with the release `version` as input and `RELEASE_ASSETS_DIR` exported. +both doors invoke it — after the tag exists, before the release is +published — with the release `version` as input and `RELEASE_ASSETS_DIR` +exported. Contract for hook authors: - Drop finished files into `$RELEASE_ASSETS_DIR`; every file there is @@ -259,9 +260,28 @@ Contract for hook authors: A failed hook leaves the tag created but no release published. Recovery is the tag door's semantics: fix the cause, then delete and re-push the same -tag (the tag door publishes for it), or run `gh release create` by hand from -a fixed tree. The merge door's nothing-exists assert will refuse a re-run of -the completed merge, by design. +tag — the tag door publishes for it. That path is forge-neutral and is the +one to prefer. + +If you must publish by hand instead, use whatever your forge provides; +ceremony itself no longer names a client here, because on a Forgejo runner +there is no `gh` to name (#191): + +```sh +# GitHub +gh release create "$VER" --verify-tag --title "$VER" \ + --notes-file notes.md -R "$OWNER/$REPO" + +# Forgejo / Gitea — POST /repos/{owner}/{repo}/releases +curl -sS -X POST -H "Authorization: token $TOKEN" \ + -H 'Content-Type: application/json' \ + -d "$(jq -nc --arg t "$VER" --rawfile b notes.md \ + '{tag_name:$t,name:$t,body:$b}')" \ + "$FORGE/api/v1/repos/$OWNER/$REPO/releases" +``` + +The merge door's nothing-exists assert will refuse a re-run of the +completed merge, by design. No hook → no assets: for a pure-bash tree, GitHub's source tarball for the tag IS the package. Worked examples land with the conversions: cast's tgz diff --git a/lib/forge-forgejo.sh b/lib/forge-forgejo.sh index 940751c..426776a 100644 --- a/lib/forge-forgejo.sh +++ b/lib/forge-forgejo.sh @@ -612,6 +612,13 @@ forge_tag_create() { "$(jq -nc --arg t "$tag" --arg s "$sha" '{tag_name:$t,target:$s}')" >/dev/null } +# forgejo_urlencode — percent-encode one query VALUE. jq is already +# a hard dependency of this backend, and @uri is its one correct answer; a +# hand-rolled sed class is how the next unescaped character gets through. +forgejo_urlencode() { + jq -rn --arg s "${1-}" '$s|@uri' +} + # forge_release_create <notes-file> [asset…] — publishes, then # uploads each asset to the created release. The release id comes back from # the create, so no second lookup is needed. @@ -627,12 +634,18 @@ forge_release_create() { [ "$#" -gt 0 ] || return 0 base="$(forgejo_api_base)" || return 1 token="${GH_TOKEN:-${GITHUB_TOKEN:-${FORGEJO_TOKEN:-}}}" - local f + local f name for f in "$@"; do [ -e "$f" ] || continue + # The asset name is a QUERY VALUE, and the hook contract permits any + # file the consumer drops in RELEASE_ASSETS_DIR. Raw interpolation broke + # on a space (curl exits 3 on the malformed URL) and silently changed + # the name on '&', '#', '+' and '%' — `gh release create` handled those, + # so a 1:1 port had to as well (#191, found by @codex on !193). + name="$(forgejo_urlencode "$(basename "$f")")" curl -sS -f -X POST -H "Authorization: token $token" \ -F "attachment=@$f" \ - "$base/repos/$REPO/releases/$id/assets?name=$(basename "$f")" >/dev/null \ + "$base/repos/$REPO/releases/$id/assets?name=$name" >/dev/null \ || { echo "forge_release_create: asset upload failed for '$f'" >&2; return 1; } done } diff --git a/test/forge-backends.test.sh b/test/forge-backends.test.sh index 2793231..d59f39e 100644 --- a/test/forge-backends.test.sh +++ b/test/forge-backends.test.sh @@ -649,7 +649,8 @@ release_stub() { -o) out="$2"; shift ;; -X) method="$2"; shift ;; -d) payload="$2"; shift ;; - -H | -F) shift ;; + -F) payload="$payload -F $2"; shift ;; + -H) shift ;; -*) ;; *) url="$1" ;; esac @@ -715,6 +716,31 @@ release_stub 201 '{"id":42}' check "forgejo: the publish POSTs to /releases with the notes as body" 0 '"body":"notes body' \ writes_after forge_release_create 1.2.3 1.2.3 "$TMP/notes.md" +# Assets: the hook contract permits any filename the consumer drops in +# RELEASE_ASSETS_DIR, and the asset name travels as a QUERY VALUE. Raw +# interpolation exits 3 on a space and silently renames on '&' / '#' / '+' / +# '%' — `gh release create` handled those, so the forgejo twin must too +# (#191, @codex on !193). +check "the encoder escapes a space" 0 "release%20asset.tgz" \ + forgejo_urlencode 'release asset.tgz' +check "the encoder escapes the query delimiters" 0 "a%26b%23c%2Bd%25e.tgz" \ + forgejo_urlencode 'a&b#c+d%e.tgz' + +printf 'x\n' >"$TMP/release asset.tgz" +printf 'y\n' >"$TMP/a&b.tgz" +release_stub 201 '{"id":42}' +check "an asset with a space uploads under the encoded name" 0 "assets?name=release%20asset.tgz" \ + writes_after forge_release_create 1.2.3 1.2.3 "$TMP/notes.md" "$TMP/release asset.tgz" +release_stub 201 '{"id":42}' +check "an asset with '&' does not become two parameters" 0 "assets?name=a%26b.tgz" \ + writes_after forge_release_create 1.2.3 1.2.3 "$TMP/notes.md" "$TMP/a&b.tgz" +release_stub 201 '{"id":42}' +check "the upload targets the created release id" 0 "releases/42/assets" \ + writes_after forge_release_create 1.2.3 1.2.3 "$TMP/notes.md" "$TMP/a&b.tgz" +release_stub 201 '{"id":42}' +check "the asset rides as a multipart attachment" 0 "attachment=@" \ + writes_after forge_release_create 1.2.3 1.2.3 "$TMP/notes.md" "$TMP/a&b.tgz" + # --- the github twins address their own paths ---------------------------- . "$ROOT/lib/forge-github.sh" GITHUB_REPOSITORY=o/r