diff --git a/.github/workflows/compat.yml b/.github/workflows/compat.yml index 38350d9..8d26ca3 100644 --- a/.github/workflows/compat.yml +++ b/.github/workflows/compat.yml @@ -56,6 +56,17 @@ jobs: python3 tools/compat.py --matrix --json > /tmp/matrix.json || rc=$? echo "shipped_broken=$rc" >> "$GITHUB_OUTPUT" + # The report body is written to a FILE, and never becomes a step output. + # + # An earlier version did `echo "${{ steps.report.outputs.body }}"`, which + # splices the text into the shell script itself -- and the report is full of + # backticks, so bash ran `create-snapshot`, `def` and `async` as commands. It + # is also an injection vector: the report is built from iX's source, so + # anything that lands in middleware would execute on the runner. + # + # The rule that avoids the whole class: never interpolate ${{ }} into a `run:` + # body. Files for data, `env:` for scalars (the runner sets those, rather than + # pasting them into the script). - name: build the report id: report run: | @@ -76,14 +87,18 @@ jobs: if any(not m["ok"] and not m["native"] for m in r["modules"].values()) ] native = [ - r for r in rows - if any(m["native"] for m in r["modules"].values()) + (r["ref"], mod) + for r in rows + for mod, m in sorted(r["modules"].items()) if m["native"] ] - print(f"broken={'1' if broken else '0'}") - print(f"refs={','.join(r['ref'] for r in broken)}") - - lines = [] + lines = [ + "`tools/compat.py` found that the patch's assumptions about " + "middlewared no longer hold.", + "", + compat.render_markdown(rows), + "", + ] for r in broken: lines.append(f"### {r['ref']}") lines.append("") @@ -96,21 +111,19 @@ jobs: lines.append(f"- `{p['id']}`: {p['detail']}") lines.append(f" - why it matters: {p['why']}") lines.append("") - if native: - lines.append("### Native support detected") - lines.append("") - for r in native: - for mod, m in sorted(r["modules"].items()): - if m["native"]: - lines.append( - f"- `{r['ref']}`: **{mod}** appears to be native now — " - f"retire the module rather than fixing it." - ) + for ref, mod in native: + lines.append( + f"- `{ref}`: **{mod}** appears to be NATIVE now — retire the " + f"module rather than fixing it." + ) + lines += ["", "_Filed automatically by `.github/workflows/compat.yml`._"] - # GITHUB_OUTPUT is line-based; a multi-line value needs a heredoc marker. - print("body<<__EOF__") - print("\n".join(lines)) - print("__EOF__") + with open("/tmp/issue.md", "w") as fh: + fh.write("\n".join(lines)) + + # Scalars only. The body stays in the file. + print(f"broken={'1' if broken else '0'}") + print(f"refs={','.join(r['ref'] for r in broken)}") PY - name: matrix @@ -132,25 +145,15 @@ jobs: # One issue per set of broken refs, reopened/updated rather than duplicated # daily -- a bot that files the same issue every morning gets muted, and # then it is not a warning system any more. - existing="$(gh issue list --state all --label compat --search "$TITLE" \ - --json number,title,state \ - --jq ".[] | select(.title == \"$TITLE\") | .number" | head -1)" - - { - echo "\`tools/compat.py\` found that the patch's assumptions no longer hold." - echo - cat /tmp/matrix.md - echo - echo "${{ steps.report.outputs.body }}" - echo - echo "_Filed automatically by \`.github/workflows/compat.yml\`._" - } > /tmp/issue.md + existing="$(gh issue list --state all --search "$TITLE" \ + --json number,title \ + --jq '.[] | select(.title == env.TITLE) | .number' | head -1)" if [ -n "$existing" ]; then gh issue comment "$existing" --body-file /tmp/issue.md gh issue reopen "$existing" 2>/dev/null || true else - gh issue create --title "$TITLE" --body-file /tmp/issue.md --label compat + gh issue create --title "$TITLE" --body-file /tmp/issue.md fi - name: file a bug report (Gitea) @@ -160,16 +163,6 @@ jobs: API: ${{ github.server_url }}/api/v1/repos/${{ github.repository }} TITLE: "Incompatible with upcoming TrueNAS: ${{ steps.report.outputs.refs }}" run: | - { - echo "\`tools/compat.py\` found that the patch's assumptions no longer hold." - echo - cat /tmp/matrix.md - echo - echo "${{ steps.report.outputs.body }}" - echo - echo "_Filed automatically by \`.github/workflows/compat.yml\`._" - } > /tmp/issue.md - body="$(jq -Rs . < /tmp/issue.md)" title="$(printf '%s' "$TITLE" | jq -Rs .)" diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b4ab6a3..b529844 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -30,15 +30,30 @@ jobs: release: runs-on: ubuntu-latest steps: + # Via `env:`, never spliced into the script. `inputs.tag` is attacker-chosen on + # a workflow_dispatch, and a ${{ }} in a `run:` body is pasted into the shell + # TEXT -- a tag of `$(...)` would simply execute. env: is safe: the runner sets + # the variable instead of rewriting the script. - name: Resolve tag id: tag + env: + EVENT: ${{ github.event_name }} + INPUT_TAG: ${{ inputs.tag }} run: | - if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then - echo "tag=${{ inputs.tag }}" >> "$GITHUB_OUTPUT" + if [ "$EVENT" = "workflow_dispatch" ]; then + tag="$INPUT_TAG" else - echo "tag=${GITHUB_REF#refs/tags/}" >> "$GITHUB_OUTPUT" + tag="${GITHUB_REF#refs/tags/}" fi + # Whatever it came from, it has to look like a tag we cut. + case "$tag" in + v[0-9]*.[0-9]*.[0-9]*) ;; + *) echo "::error::refusing to release a tag that is not vX.Y.Z[-rcN]: $tag"; exit 1 ;; + esac + + echo "tag=$tag" >> "$GITHUB_OUTPUT" + - uses: actions/checkout@v4 with: ref: ${{ steps.tag.outputs.tag }} @@ -71,7 +86,9 @@ jobs: # Catches the failure mode this repo actually had: VERSION= drifted to # three different values across the scripts, and nothing noticed. - name: "gate: version matches tag, CHANGELOG complete, nothing stranded" - run: python3 tools/release_notes.py check "${{ steps.tag.outputs.tag }}" + env: + TAG: ${{ steps.tag.outputs.tag }} + run: python3 tools/release_notes.py check "$TAG" # THE BARRIER. A stable release must have been a release candidate on this # exact commit. Candidates are invisible to users (update.sh and the alert @@ -83,7 +100,9 @@ jobs: # you find out. It is here because this is the only place that cannot be # bypassed: it holds the token that publishes. - name: "gate: this commit was a release candidate" - run: python3 tools/release_gate.py "${{ steps.tag.outputs.tag }}" -C . + env: + TAG: ${{ steps.tag.outputs.tag }} + run: python3 tools/release_gate.py "$TAG" -C . # There is deliberately NO "did the candidate's CI run pass?" gate here. # @@ -98,8 +117,10 @@ jobs: # that; it cannot verify it. - name: extract release notes from CHANGELOG + env: + TAG: ${{ steps.tag.outputs.tag }} run: | - python3 tools/release_notes.py notes "${{ steps.tag.outputs.tag }}" > /tmp/notes.md + python3 tools/release_notes.py notes "$TAG" > /tmp/notes.md echo "--- release body ---" cat /tmp/notes.md diff --git a/tests/test_workflows.py b/tests/test_workflows.py new file mode 100644 index 0000000..e015359 --- /dev/null +++ b/tests/test_workflows.py @@ -0,0 +1,107 @@ +"""Guards on the CI workflows themselves. + +The workflows run on TWO forges -- Gitea (canonical) and GitHub (mirror), because +Gitea reads .github/workflows too -- and they hold tokens. A mistake here is not a +failed build, it is a bug report nobody files or a command nobody meant to run. +""" + +import os +import re + +import pytest + +WORKFLOWS = os.path.join(os.path.dirname(__file__), "..", ".github", "workflows") + + +def workflow_files(): + return [ + os.path.join(WORKFLOWS, f) + for f in sorted(os.listdir(WORKFLOWS)) + if f.endswith((".yml", ".yaml")) + ] + + +def run_bodies(path): + """Every `run:` block's text, with its line number.""" + with open(path, encoding="utf-8") as fh: + lines = fh.readlines() + + out = [] + i = 0 + while i < len(lines): + m = re.match(r"^(\s*)run:\s*\|", lines[i]) + if not m: + i += 1 + continue + indent = len(m.group(1)) + start = i + 1 + body = [] + i += 1 + while i < len(lines): + line = lines[i] + if line.strip() and (len(line) - len(line.lstrip())) <= indent: + break + body.append(line) + i += 1 + out.append((start + 1, "".join(body))) + return out + + +class TestNoExpressionInterpolationIntoShell: + """`${{ ... }}` inside a `run:` body is spliced into the SCRIPT TEXT. + + This is not theoretical. `echo "${{ steps.report.outputs.body }}"` in the compat + workflow pasted the report -- which is full of backticks -- straight into bash, + which promptly ran `create-snapshot`, `def` and `async` as commands. And because + that report is built from iX's middleware source, anything landing in their tree + would have executed on our runner. + + The rule: files for data, `env:` for scalars. `env:` is safe because the runner + sets the variable rather than pasting it into the script. + """ + + @pytest.mark.parametrize("path", workflow_files(), ids=os.path.basename) + def test_no_github_expression_in_a_run_body(self, path): + offenders = [] + for lineno, body in run_bodies(path): + for m in re.finditer(r"\$\{\{[^}]*\}\}", body): + offenders.append(f"{os.path.basename(path)}:~{lineno}: {m.group(0)}") + assert not offenders, ( + "GitHub/Gitea expressions interpolate into the shell script text, so " + "backticks and $() in the value EXECUTE. Pass data via a file, or a " + "scalar via `env:`.\n " + "\n ".join(offenders) + ) + + +class TestBothForges: + """Gitea is canonical; GitHub is a mirror. Both run these files.""" + + def test_release_publishes_on_each_forge_exactly_once(self): + with open(os.path.join(WORKFLOWS, "release.yml"), encoding="utf-8") as fh: + src = fh.read() + # One step gated ON github.com, one gated OFF it. Without the pair, a release + # either double-publishes or silently never publishes on the canonical host. + assert "if: ${{ contains(github.server_url, 'github.com') }}" in src + assert "if: ${{ !contains(github.server_url, 'github.com') }}" in src + + def test_compat_files_an_issue_on_each_forge(self): + with open(os.path.join(WORKFLOWS, "compat.yml"), encoding="utf-8") as fh: + src = fh.read() + assert "file a bug report (GitHub)" in src + assert "file a bug report (Gitea)" in src + + +class TestCompatCannotSilentlyPass: + def test_the_exit_code_is_captured_not_swallowed(self): + # Actions runs `bash -e`: `cmd > out` followed by `echo $?` never reaches the + # echo, so the "a shipped release is broken" signal would be lost and the job + # would go green while users were broken. + with open(os.path.join(WORKFLOWS, "compat.yml"), encoding="utf-8") as fh: + src = fh.read() + assert "|| rc=$?" in src + assert "shipped_broken=$rc" in src + + def test_a_broken_shipped_release_fails_the_job(self): + with open(os.path.join(WORKFLOWS, "compat.yml"), encoding="utf-8") as fh: + src = fh.read() + assert "steps.check.outputs.shipped_broken != '0'" in src