compat/release: stop interpolating ${{ }} into run: bodies

The report body is full of backticks, so 'echo "${{ steps.report.outputs.body }}"' pasted it into the shell text and bash executed create-snapshot, def and async as commands. The report is built from iX's middleware source, so that was an injection vector as well as a bug. inputs.tag on workflow_dispatch had the same shape. Data goes through files, scalars through env:. Tests enforce it across every workflow.
2026-07-13 17:30:50 +00:00
parent 9236aa0034
commit cd39489c7f
3 changed files with 171 additions and 50 deletions
@@ -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"]:
+          for ref, mod in native:
              lines.append(
-                              f"- `{r['ref']}`: **{mod}** appears to be native now — "
-                              f"retire the module rather than fixing it."
+                  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 .)"

@@ -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

@@ -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