Skip to content

Changelog check interpolates github.base_ref into a shell body, and can never fail #45

Description

@konard

Two defects in the same step — changelog -> "Check for changelog fragments" in .github/workflows/release.yml (around lines 380-414 at the current main).

1. Script injection via github.base_ref

      - name: Check for changelog fragments
        run: |
          ...
          CHANGED_FILES=$(git diff --name-only origin/${{ github.base_ref }}...HEAD)

${{ }} expressions are substituted into the script before bash sees it, so the base branch name is spliced in as shell source. On a pull_request event the base ref is repository data rather than fork data, but it is still attacker-influenced in any repository where contributors can create branches, and a branch name may legally contain `, $(, ; and &.

Reproduction

git checkout -b '$(touch /tmp/pwned)'
git push -u origin '$(touch /tmp/pwned)'

Then open a pull request targeting that branch. The runner executes:

git diff --name-only origin/$(touch /tmp/pwned)...HEAD

and the command substitution runs. Substitute any command for the payload.

Suggested fix

Pass it as an environment variable, which is data rather than source, and quote the expansion. This is what the rust template already does — scripts/check-changelog-fragment.rs is invoked with env: GITHUB_BASE_REF: ${{ github.base_ref }}.

      - name: Check for changelog fragments
        env:
          GITHUB_BASE_REF: ${{ github.base_ref }}
        run: |
          set -euo pipefail
          ...
          CHANGED_FILES=$(git diff --name-only "origin/${GITHUB_BASE_REF}...HEAD")

2. The gate can never fail

          if [ "$SOURCE_CHANGED" -gt 0 ] && [ "$FRAGMENTS" -eq 0 ]; then
            echo "::warning::No changelog fragment found. ..."
            ...
            # Note: This is a warning, not a failure, to allow flexibility
            # Change 'exit 0' to 'exit 1' to make it required
            exit 0
          fi

The comment says this is deliberate, so treat this half as a suggestion rather than a bug report. The practical effect, though, is that a job named "Changelog Fragment Check" is always green and the annotation is easy to miss in a long log, so source changes routinely land without a fragment and the generated changelog silently drifts from reality. The rust template's equivalent check exits 1 (scripts/check-changelog-fragment.rs:157), and the js template's changeset check fails too, so python is the odd one out among the three templates.

Workaround

Configure a required status check on a separate job, or grep the log for ::warning::No changelog fragment.

Suggested fix

          if [ "$SOURCE_CHANGED" -gt 0 ] && [ "$FRAGMENTS" -eq 0 ]; then
            echo "::error::No changelog fragment found. Please run 'scriv create' and document your changes."
            echo "  pip install 'scriv[toml]'"
            echo "  scriv create"
            exit 1
          fi

Also worth adding set -euo pipefail to the step: SOURCE_CHANGED=$(echo "$CHANGED_FILES" | grep -E "$SOURCE_PATTERN" | wc -l) currently relies on grep's exit status being swallowed by the pipe, which is fragile if the pipeline is ever reordered — grep -cE "$SOURCE_PATTERN" || true is the safer form.

Context

Found while auditing CI/CD false positives and false negatives in a repository seeded from these templates: link-foundation/browser-commander#75. Both fixes are applied there in link-foundation/browser-commander#76.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions