fix(ci): stop script injection in the GitHub <-> Forgejo sync workflows #108

Merged

Grandiras merged 1 commit from fix/ci-workflow-script-injection into main 2026-09-24 12:37:51 +00:00

Conversation 2 Commits 1 Files changed 5 +134 -97

Grandiras commented 2026-09-24 12:25:14 +00:00

Owner

Copy link

Problem

The GitHub → Forgejo sync workflows pasted untrusted event fields straight into run: scripts with ${{ }}. GitHub expands those expressions into the script text before bash parses it, so a title like "; curl … $FORGEJO_TOKEN; " or a body containing the heredoc delimiter runs as code. Those steps have FORGEJO_TOKEN (write access to this repository) in their environment. Anyone who can open an issue, PR or comment on the public GitHub mirror could trigger it.

Workflow	Untrusted fields that were interpolated
.github/workflows/sync-issues.yml	issue title, body (in a heredoc), author, URL
.github/workflows/sync-prs.yml	PR title, body (in a heredoc), author, head branch name, base ref. Runs on pull_request_target, so secrets are always available
.github/workflows/sync-comments.yml	comment body (×2, heredocs), review body (heredoc), review-comment file path, review state
.forgejo/workflows/sync-to-github.yml	issue/PR title. A GitHub issue title reaches this verbatim through the mirror, so a hostile title opened on GitHub ran here with GH_MIRROR_TOKEN as soon as the mirrored issue was closed

Nothing suggests this was exploited. The GitHub API lists zero runs of the three sync workflows, and the mirror has never had an issue or PR. Both tokens should still be rotated (see below).

Changes

• Event fields go through env: only. Every event-controlled value is set in the step's env: block and used only as a quoted shell variable. JSON payloads are built with jq --arg, as before. The heredocs are gone.
• permissions: {} on all three GitHub workflows. None of them uses the GitHub token.
• sync-prs.yml no longer checks out the PR. actions/checkout of the PR head is replaced by a fetch into a bare repository under $RUNNER_TEMP, with no working tree and no persisted GitHub credentials. The PR's commits are only ever passed to git push. The Forgejo token is given to that push directly and is no longer saved as a git remote. The Forgejo branch name gh-pr-N/<branch> is reduced to [A-Za-z0-9._/-] and checked with git check-ref-format.
• sync-prs.yml won't mirror a PR that changes .forgejo/ or .gitea/. A mirrored PR is pushed as a branch of this repository, not a fork. Forgejo runs the workflow files of a pushed commit with this repository's secrets. Without this check, a GitHub PR adding a .forgejo/workflows/x.yml with on: push could read NUGET_ORG_API_KEY and publish to nuget.org. The sync now fails with an error on the GitHub PR whenever the PR's CI-config trees differ from its base branch, and a maintainer has to review and pull that PR by hand.
• sync-to-github.yml is fixed the same way (env vars, and printf instead of echo for the title).
• CHANGELOG: four bullets under [Unreleased] → Security.

Behaviour is otherwise unchanged. The merged-PR path still pushes head.sha to the base branch, as before.

Verification

I extracted each run: script and ran it locally with stubbed curl / git push and a local bare clone standing in for GitHub:

• Before: the original sync-issues.yml script, with ${{ }} expanded the way GitHub does it and a hostile title and body, created the canary file (injection reproduced).
• After: the same hostile values (command substitution, quote break-out, heredoc-delimiter break-out, $(touch${IFS}…) as a branch name and as a review-comment path), passed through env, created no canary in any of sync-issues, sync-comments (issue comment, review comment, review), sync-prs, or sync-to-github. Every payload was valid JSON, and titles and bodies arrived byte-for-byte.
• sync-prs paths:
  • opened with CI config unchanged: branch pushed as gh-pr-5/a__touch__IFS__CANARY_b and PR created.
  • synchronize with a .forgejo/ that differs from the base branch: refused with exit 1, nothing pushed.
  • closed + merged: head pushed to main, then close, comment, and branch delete.
  • In every case only a bare repo was on disk.
• The anonymous by-SHA fetch from the public GitHub mirror works (git fetch https://github.com/Grandiras/ATProto.NET.git +<sha>:refs/pr/head).
• No ${{ }} remains inside any run: in the four sync workflows. actionlint/shellcheck aren't installed here, so neither was run.

Action needed from the owner

1. Rotate FORGEJO_TOKEN (GitHub repo secret). It has write access here and, until this merges, can be taken by anyone who opens a GitHub issue.
2. Rotate GH_MIRROR_TOKEN (Forgejo repo secret), for the same reason via sync-to-github.yml.

Not fixed here (needs a decision)

• Mirrored PRs still build on Forgejo as same-repo branches. Forgejo CI (pull_request → dotnet build) runs the PR's own code. Since the branch isn't from a fork, that job may get a write-scoped automatic token, depending on the Forgejo version and settings. The robust fix is to push mirrored PRs to a separate fork repo, so Forgejo treats them as fork PRs (read-only token, no secrets, approval gate).
• opencode-bot auto-reviews mirrored PRs, because they are opened by Grandiras through FORGEJO_TOKEN. That hands contributor-controlled code and text to an agent with a toolchain. It is worth excluding [GH-PR-#…] PRs, or the gh-pr-* branches, in the bot config.
• Merged-on-GitHub sync pushes head.sha, not merge_commit_sha, to the base branch (pre-existing behaviour, not a security issue).
• .forgejo/workflows/release.yml still has ${{ inputs.version }} / ${{ github.ref_name }} in run:. Only writers can set those (workflow_dispatch, tag push), so I left them alone.

🤖 Generated with Claude Code

## Problem The GitHub → Forgejo sync workflows pasted untrusted event fields straight into `run:` scripts with `${{ }}`. GitHub expands those expressions into the script text **before** bash parses it, so a title like `"; curl … $FORGEJO_TOKEN; "` or a body containing the heredoc delimiter runs as code. Those steps have `FORGEJO_TOKEN` (write access to this repository) in their environment. Anyone who can open an issue, PR or comment on the public GitHub mirror could trigger it. | Workflow | Untrusted fields that were interpolated | |---|---| | `.github/workflows/sync-issues.yml` | issue title, body (in a heredoc), author, URL | | `.github/workflows/sync-prs.yml` | PR title, body (in a heredoc), author, **head branch name**, base ref. Runs on `pull_request_target`, so secrets are always available | | `.github/workflows/sync-comments.yml` | comment body (×2, heredocs), review body (heredoc), **review-comment file path**, review state | | `.forgejo/workflows/sync-to-github.yml` | issue/PR title. **A GitHub issue title reaches this verbatim through the mirror**, so a hostile title opened on GitHub ran here with `GH_MIRROR_TOKEN` as soon as the mirrored issue was closed | Nothing suggests this was exploited. The GitHub API lists **zero** runs of the three sync workflows, and the mirror has never had an issue or PR. Both tokens should still be rotated (see below). ## Changes - **Event fields go through `env:` only.** Every event-controlled value is set in the step's `env:` block and used only as a quoted shell variable. JSON payloads are built with `jq --arg`, as before. The heredocs are gone. - **`permissions: {}` on all three GitHub workflows.** None of them uses the GitHub token. - **`sync-prs.yml` no longer checks out the PR.** `actions/checkout` of the PR head is replaced by a fetch into a bare repository under `$RUNNER_TEMP`, with no working tree and no persisted GitHub credentials. The PR's commits are only ever passed to `git push`. The Forgejo token is given to that push directly and is no longer saved as a git remote. The Forgejo branch name `gh-pr-N/<branch>` is reduced to `[A-Za-z0-9._/-]` and checked with `git check-ref-format`. - **`sync-prs.yml` won't mirror a PR that changes `.forgejo/` or `.gitea/`.** A mirrored PR is pushed as a branch of *this* repository, not a fork. Forgejo runs the workflow files of a pushed commit with this repository's secrets. Without this check, a GitHub PR adding a `.forgejo/workflows/x.yml` with `on: push` could read `NUGET_ORG_API_KEY` and publish to nuget.org. The sync now fails with an error on the GitHub PR whenever the PR's CI-config trees differ from its base branch, and a maintainer has to review and pull that PR by hand. - **`sync-to-github.yml` is fixed the same way** (env vars, and `printf` instead of `echo` for the title). - CHANGELOG: four bullets under `[Unreleased]` → `Security`. Behaviour is otherwise unchanged. The merged-PR path still pushes `head.sha` to the base branch, as before. ## Verification I extracted each `run:` script and ran it locally with stubbed `curl` / `git push` and a local bare clone standing in for GitHub: - **Before:** the original `sync-issues.yml` script, with `${{ }}` expanded the way GitHub does it and a hostile title and body, **created the canary file** (injection reproduced). - **After:** the same hostile values (command substitution, quote break-out, heredoc-delimiter break-out, `$(touch${IFS}…)` as a branch name and as a review-comment path), passed through env, created **no canary** in any of `sync-issues`, `sync-comments` (issue comment, review comment, review), `sync-prs`, or `sync-to-github`. Every payload was valid JSON, and titles and bodies arrived byte-for-byte. - `sync-prs` paths: - `opened` with CI config unchanged: branch pushed as `gh-pr-5/a__touch__IFS__CANARY_b` and PR created. - `synchronize` with a `.forgejo/` that differs from the base branch: refused with exit 1, nothing pushed. - `closed` + merged: head pushed to `main`, then close, comment, and branch delete. - In every case only a bare repo was on disk. - The anonymous by-SHA fetch from the public GitHub mirror works (`git fetch https://github.com/Grandiras/ATProto.NET.git +<sha>:refs/pr/head`). - No `${{ }}` remains inside any `run:` in the four sync workflows. `actionlint`/`shellcheck` aren't installed here, so neither was run. ## Action needed from the owner 1. **Rotate `FORGEJO_TOKEN`** (GitHub repo secret). It has write access here and, until this merges, can be taken by anyone who opens a GitHub issue. 2. **Rotate `GH_MIRROR_TOKEN`** (Forgejo repo secret), for the same reason via `sync-to-github.yml`. ## Not fixed here (needs a decision) - **Mirrored PRs still build on Forgejo as same-repo branches.** Forgejo CI (`pull_request` → `dotnet build`) runs the PR's own code. Since the branch isn't from a fork, that job may get a write-scoped automatic token, depending on the Forgejo version and settings. The robust fix is to push mirrored PRs to a separate fork repo, so Forgejo treats them as fork PRs (read-only token, no secrets, approval gate). - **opencode-bot auto-reviews mirrored PRs**, because they are opened by `Grandiras` through `FORGEJO_TOKEN`. That hands contributor-controlled code and text to an agent with a toolchain. It is worth excluding `[GH-PR-#…]` PRs, or the `gh-pr-*` branches, in the bot config. - **Merged-on-GitHub sync pushes `head.sha`, not `merge_commit_sha`**, to the base branch (pre-existing behaviour, not a security issue). - `.forgejo/workflows/release.yml` still has `${{ inputs.version }}` / `${{ github.ref_name }}` in `run:`. Only writers can set those (workflow_dispatch, tag push), so I left them alone. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 1 commit 2026-09-24 12:25:14 +00:00

fix(ci): stop script injection in the GitHub <-> Forgejo sync workflows ... 7704e660ae

The sync workflows interpolated untrusted event fields (issue/PR titles and
bodies, comment bodies, review-comment paths, PR branch names) into run:
scripts with ${{ }}, which is expanded into the script text before bash
parses it. Anyone who could open an issue, PR or comment on the public
GitHub mirror could run commands with FORGEJO_TOKEN in the environment; the
quoted heredocs offered no protection because a body can contain the
delimiter.

- Every event field now reaches the script only through env: and a quoted
  variable; JSON payloads keep being built with jq --arg.
- permissions: {} on all three GitHub workflows; none uses the GitHub token.
- sync-prs.yml (pull_request_target) no longer checks out the PR. Its
  commits are fetched into a bare repository and only handed to git push,
  the Forgejo token is no longer saved as a remote, and the Forgejo branch
  name is reduced to [A-Za-z0-9._/-].
- sync-prs.yml refuses to mirror a PR whose .forgejo/ or .gitea/ differs
  from its base branch: the mirrored branch lives in the canonical repo, so
  Forgejo would run its workflow files with repository secrets.
- .forgejo/workflows/sync-to-github.yml had the same title injection, and a
  GitHub issue title reaches it verbatim through the mirror, exposing
  GH_MIRROR_TOKEN. Fixed the same way.

FORGEJO_TOKEN and GH_MIRROR_TOKEN should be rotated.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

opencode-bot commented 2026-09-24 12:25:16 +00:00

Collaborator

Copy link

🤖 Reviewing this PR — the review will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the review runs.

🤖 Reviewing this PR — the review will link the full session transcript. <!-- opencode-run:review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-108/run-2026-09-24T12-25-16-224Z.md) — updating every ~5 minutes while the review runs.

Grandiras referenced this pull request 2026-09-24 12:26:19 +00:00

ci: pass mirror-sync event fields through env instead of interpolating them #109

opencode-reviewer approved these changes 2026-09-24 12:33:53 +00:00

opencode-reviewer left a comment

Copy link

✅ Verdict: approve — no blocking issues found.

Solid, well-scoped fix for the script-injection hole. GitHub expands ${{ }} into the run: script before bash parses it, so the env-only + quoted-variable pattern used here is the correct remediation, and it is applied consistently. I verified with grep that no ${{ }} remains inside any run: in the four sync workflows; the only remaining interpolations are inside env: blocks (safe) and in release.yml, which only writers can trigger.

The extra hardening in sync-prs.yml is sound: dropping actions/checkout eliminates both the injection surface and the persisted GitHub credentials, the bare repo under $RUNNER_TEMP keeps the PR's commits out of any executed context, the tr whitelist plus git check-ref-format fails closed on hostile branch names, and the anonymous by-SHA fetch is valid on github.com (the same mechanism actions/checkout relies on for pull_request_target). The .forgejo/.gitea tree comparison is conservative and catches deletions, additions, and file-vs-directory swaps alike, and a later force-push is caught by the next synchronize run. permissions: {} is correct for all three GitHub workflows since none uses the GitHub token.

Two non-blocking notes:

1. The closed-and-merged path (line 72) pushes head.sha to the base branch without applying the same .forgejo/.gitea guard. That's acceptable — it only executes after a maintainer merges on GitHub, which is itself the manual review the guard demands — but running the identical check before that push would be cheap defense in depth.

2. Best-effort branch cleanup (.../branches/${FORGEJO_BRANCH} with / unescaped in the URL) is pre-existing and swallowed by || true; Gitea-style wildcard routing usually tolerates it, so no action needed.

The disclosed follow-ups (rotating both tokens, and eventually pushing mirrored PRs to a separate fork repo so Forgejo treats them as fork PRs) are the right remaining actions; the token rotation should happen before or promptly after this merges.

---

⏱ 8m 37s · 10 steps · $0.0303 · tokens: 111.8k in / 21.1k out (+97.3k cache-read) · Go: 5h 9% · wk 90% · mo 78%
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Solid, well-scoped fix for the script-injection hole. GitHub expands `${{ }}` into the `run:` script before bash parses it, so the env-only + quoted-variable pattern used here is the correct remediation, and it is applied consistently. I verified with grep that no `${{ }}` remains inside any `run:` in the four sync workflows; the only remaining interpolations are inside `env:` blocks (safe) and in `release.yml`, which only writers can trigger. The extra hardening in `sync-prs.yml` is sound: dropping `actions/checkout` eliminates both the injection surface and the persisted GitHub credentials, the bare repo under `$RUNNER_TEMP` keeps the PR's commits out of any executed context, the `tr` whitelist plus `git check-ref-format` fails closed on hostile branch names, and the anonymous by-SHA fetch is valid on github.com (the same mechanism `actions/checkout` relies on for `pull_request_target`). The `.forgejo`/`.gitea` tree comparison is conservative and catches deletions, additions, and file-vs-directory swaps alike, and a later force-push is caught by the next `synchronize` run. `permissions: {}` is correct for all three GitHub workflows since none uses the GitHub token. Two non-blocking notes: 1. The closed-and-merged path (line 72) pushes `head.sha` to the base branch without applying the same `.forgejo`/`.gitea` guard. That's acceptable — it only executes after a maintainer merges on GitHub, which is itself the manual review the guard demands — but running the identical check before that push would be cheap defense in depth. 2. Best-effort branch cleanup (`.../branches/${FORGEJO_BRANCH}` with `/` unescaped in the URL) is pre-existing and swallowed by `|| true`; Gitea-style wildcard routing usually tolerates it, so no action needed. The disclosed follow-ups (rotating both tokens, and eventually pushing mirrored PRs to a separate fork repo so Forgejo treats them as fork PRs) are the right remaining actions; the token rotation should happen before or promptly after this merges. --- ⏱ 8m 37s · 10 steps · $0.0303 · tokens: 111.8k in / 21.1k out (+97.3k cache-read) · Go: 5h 9% · wk 90% · mo 78% 📜 [Full session transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-108/run-2026-09-24T12-25-16-224Z.md) <!-- opencode-reviewed-head:7704e660ae449084c2c13b545b87e8d489be9b5a --> <!-- opencode-verdict:approve -->

Grandiras merged commit 308cb7cd90 into main 2026-09-24 12:37:51 +00:00

Grandiras deleted branch fix/ci-workflow-script-injection 2026-09-24 12:37:51 +00:00

Grandiras referenced this pull request from a commit 2026-09-24 12:37:52 +00:00

fix(ci): stop script injection in the GitHub <-> Forgejo sync workflows (#108)

Grandiras referenced this pull request 2026-09-24 12:39:33 +00:00

Tracking: v0.7.0 update cycle — protocol catch-up, optimization, debloat, API reshaping #141

Sign in to join this conversation.

Reviewers

No reviewers

opencode-reviewer

Labels

Clear labels   

breaking-change

Introduces a breaking API change

  

bug

Something is not working correctly

  

documentation

Documentation improvements

  

duplicate

This issue or PR already exists

  

enhancement

New feature or improvement

  

good first issue

Good for newcomers to the project

  

help wanted

Extra attention is needed

  

performance

Performance-related issue or improvement

  

question

Further information is requested

  

wontfix

This will not be worked on

No labels

breaking-change

bug

documentation

duplicate

enhancement

good first issue

help wanted

performance

question

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

3 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Grandiras/ATProto.NET!108

No description provided.