Skip to content

fix(ci): reject newline injection in SOURCE_REF before GITHUB_ENV write - #141

Merged
theredspoon merged 1 commit into
ci-automationfrom
fix/mirror-workflow-github-env-injection
Aug 10, 2026
Merged

theredspoon merged 1 commit into
ci-automationfrom
fix/mirror-workflow-github-env-injection

Conversation

@theredspoon

Copy link
Copy Markdown
Owner

Summary

  • SOURCE_REF (from the source_ref workflow-dispatch input, or github.ref_name) is sanitized in the "Capture mirrored source revision" step before being interpolated into a line written to $GITHUB_ENV.
  • The existing sanitization (case "${SOURCE_REF}" in */*|"") ...) rejected values containing / or an empty string, but did not reject embedded newline (\n) or carriage-return (\r) characters.
  • Because $GITHUB_ENV is a line-based KEY=VALUE format, a SOURCE_REF containing a newline followed by another KEY=VALUE-shaped line could inject/override other environment variables (e.g. TARGET_BRANCH) consumed by later steps in the same job — e.g. redirecting the mirror push/PR target branch in the deployment repo.
  • Impact is contained (no privilege beyond the workflow's own token), but it's a real injection primitive with no legitimate use case, since SOURCE_REF is meant to be a single branch-name-shaped identifier.

Fix

Replaced the ad-hoc case check with a strict allowlist regex for valid single-segment ref characters (^[A-Za-z0-9._-]+$). This is a superset fix: it still rejects / and empty string (matching current behavior) and additionally rejects newlines, carriage returns, spaces, and any other unexpected characters — closing off the injection primitive rather than just patching the one payload shape.

Confirmed the allowlist doesn't break the only currently-valid SOURCE_REF values: the input's default (reborn-matrix-pilot) and typical branch names used on this automation branch (main, upstream-main, matrix-channel-clean, etc.) all match.

Test plan

This branch is intentionally minimal (see README/.gitignore: only workflows + README + gitignore are allowed), so no test file is committed. Instead, verified with an isolated shell reproduction of the exact sanitization logic before and after the fix:

  • Valid refs (reborn-matrix-pilot, main, release_1.2.3) are still accepted.
  • Previously-rejected cases (empty string, ref containing /) are still rejected.
  • A SOURCE_REF payload of reborn-matrix-pilot\nTARGET_BRANCH=attacker-controlled-branch is rejected by the new check.
  • The same CRLF variant (\r\n) is rejected.
  • Confirmed, for comparison, that the old case check accepted that same newline payload — reproducing the vulnerability before the fix.
== Valid refs that must keep working ==
PASS (accepted as expected): default branch name
PASS (accepted as expected): simple branch name
PASS (accepted as expected): branch with dots and underscores

== Previously-rejected cases (must still be rejected) ==
PASS (rejected as expected): empty string
PASS (rejected as expected): contains slash

== Newline injection payload (the bug this fix closes) ==
PASS (rejected as expected): embedded newline injecting a second GITHUB_ENV key
PASS (rejected as expected): embedded CRLF injecting a second GITHUB_ENV key

== Old (vulnerable) case-statement check, for comparison ==
CONFIRMED VULNERABLE (old check): newline payload was ACCEPTED by the old */*|"" check

Results: 7 passed, 0 failed

SOURCE_REF sanitization only rejected values containing "/" or an empty
string. A value with an embedded newline followed by a KEY=VALUE-shaped
line could inject or override other env vars (e.g. TARGET_BRANCH) for
later steps that read GITHUB_ENV, since it is a line-based format.

Replace the ad-hoc case-statement check with a strict allowlist regex
for valid single-segment ref characters, which also covers newlines
and carriage returns.
@theredspoon

Copy link
Copy Markdown
Owner Author

Independent review (Codex CLI, codex exec review --base ci-automation)

The allowlist validation prevents newline-based GITHUB_ENV injection while preserving the workflow's existing single-segment source-ref constraint. No functional or security regressions requiring a finding were identified.

No findings. Ready to merge.

@theredspoon
theredspoon merged commit aff022b into ci-automation Aug 10, 2026
@theredspoon
theredspoon deleted the fix/mirror-workflow-github-env-injection branch August 10, 2026 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant