Skip to content

ci(ci-deep): validate fuzz_seconds before writing to GITHUB_ENV - #18

Merged
eilandert merged 2 commits into
mainfrom
ci/validate-fuzz-seconds-dispatch-input
Aug 4, 2026
Merged

eilandert merged 2 commits into
mainfrom
ci/validate-fuzz-seconds-dispatch-input

Conversation

@eilandert

Copy link
Copy Markdown
Member

Ported back from nginx-error-abuse-module, which hit this while adopting the skeleton's CI standard (its cp9). The template's own copy still has the gap, so every adopter inherits it.

TL;DR

ci-deep.yml lets you pass a fuzz duration when you trigger the workflow by hand. That value was written straight into $GITHUB_ENV, which GitHub reads one line at a time. Put a newline in the value and you get to define a second environment variable of your choosing, which every later step in the job then runs with. This validates the input first.

Why the existing guard doesn't cover it

The input already goes through an env var rather than ${{ }} inside run:, and the comment above it explains why: template expansion of event data into a shell script is an injection primitive. That part is correct and stays.

It solves a different problem, though. Keeping the value out of the shell's syntax does nothing about what the value means to $GITHUB_ENV, which is a line-oriented KEY=value file:

fuzz_seconds = $'3600\nPATH=/tmp/evil'

  $GITHUB_ENV:
    FUZZ_SECS=3600
    PATH=/tmp/evil        <- attacker-chosen, exported to every later step

The fix

Validate the whole value, range-check it, and fail loudly on rejection:

if ! printf '%s' "${DISPATCH_FUZZ_SECS}" | grep -qzE '^[0-9]+$'; then

grep -qz is the load-bearing part. It anchors ^...$ against the entire NUL-delimited buffer. A plain grep -qE '^[0-9]+$' treats the embedded newline as a line boundary, matches the benign 3600 on line one, and passes the payload through unharmed:

$ P=$'3600\nPATH=/tmp/evil'
$ printf '%s' "$P" | grep -qE  '^[0-9]+$' && echo MATCHES
MATCHES
$ printf '%s' "$P" | grep -qzE '^[0-9]+$' || echo rejects
rejects

A rejected value exits non-zero instead of falling back to 14400. Silently substituting the default would make an attempted injection indistinguishable from a normal scheduled run, which is the wrong thing to learn about six months later.

Scope and severity

workflow_dispatch requires write access to the repo, so this is defence in depth, not an open door. It matters here because this file exists to be copied: the fix costs eight lines in one repo and removes the footgun from every clone.

Unchanged: the schedule path, the 14400 default, and the existing env-var indirection.

Testing

Behaviour of the new block against the payload and the boundaries, run locally:

input=$'3600\nPATH=/tmp/evil'  rc=1  env=
input=3600                     rc=0  env=FUZZ_SECS=3600
input=0                        rc=1  env=
input=99999                    rc=1  env=
input=abc                      rc=1  env=
input=''                       rc=1  env=

Negative control: the pre-change one-liner run against the same payload writes PATH=/tmp/evil into the file, so the test distinguishes the two versions rather than passing against both.

ci/linter/run-all.sh clean, including actionlint and zizmor (pedantic) on the edited workflow.

@coderabbitai

coderabbitai Bot commented Aug 4, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ef806cdc-133b-4e7a-a4fe-97bb673346b1

📥 Commits

Reviewing files that changed from the base of the PR and between a21b308 and 64fbdf6.

📒 Files selected for processing (1)
  • .github/workflows/ci-deep.yml

Walkthrough

Changes

CI workflow hardening

Layer / File(s) Summary
Fuzz-duration input validation
.github/workflows/ci-deep.yml
Manual fuzz duration input is validated as a complete decimal value from 1 to 86400. Invalid input fails the job. FUZZ_SECS uses printf; scheduled runs continue using 14400 seconds.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies validation of the fuzz_seconds input before writing it to GITHUB_ENV.
Description check ✅ Passed The description directly explains the newline-injection risk, validation changes, preserved behavior, and testing performed.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ci/validate-fuzz-seconds-dispatch-input
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch ci/validate-fuzz-seconds-dispatch-input

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 85f5034e-8ff1-4f20-bb68-f49916a998b4

📥 Commits

Reviewing files that changed from the base of the PR and between 2f2a371 and a21b308.

📒 Files selected for processing (2)
  • .github/workflows/ci-deep.yml
  • README.md

Comment thread .github/workflows/ci-deep.yml Outdated
The dispatch input reached $GITHUB_ENV unvalidated. Routing it through an
env var stops template injection into the run: script, but $GITHUB_ENV is
parsed line by line, so a value containing a newline writes a second,
attacker-chosen variable into every later step of the job:

    fuzz_seconds = "3600\nPATH=/tmp/evil"
      -> FUZZ_SECS=3600
         PATH=/tmp/evil

Validate the whole buffer before the write. `grep -qzE '^[0-9]+$'` anchors
against the entire NUL-delimited input; a plain `grep -qE '^[0-9]+$'` treats
the embedded newline as a line boundary, matches the benign first line and
lets the payload through. Range-check follows, and a rejected value fails the
job loudly rather than falling back to the default, which would make an
attempted injection look like an ordinary run.

workflow_dispatch on this workflow requires write access, so this is
defence in depth rather than an open door.
@eilandert
eilandert force-pushed the ci/validate-fuzz-seconds-dispatch-input branch from a21b308 to 70f26ef Compare August 4, 2026 10:28
`^[0-9]+$` accepted a value too large for the shell's integer type, and
`[ "$v" -lt 1 ]` does not return false on one -- it errors. Inside `if !` and
`||` that error is consumed, so both range comparisons failed rather than
compared and the value reached $GITHUB_ENV unbounded:

    fuzz_seconds = 99999999999999999999999999
      -> passes ^[0-9]+$
      -> range check errors, not false
      -> FUZZ_SECS=99999999999999999999999999

`^[0-9]{1,5}$` covers the 86400 ceiling and keeps every accepted value inside
the range the arithmetic can actually evaluate. Boundaries 1 and 86400 still
pass; 0, 99999, both overflow shapes and the newline payload all reject.

Caught by CodeRabbit on PR #18.
@eilandert
eilandert merged commit 4cff5dd into main Aug 4, 2026
14 checks passed
@eilandert
eilandert deleted the ci/validate-fuzz-seconds-dispatch-input branch August 4, 2026 12:48
eilandert added a commit that referenced this pull request Aug 6, 2026
The env var kept the dispatch input out of shell syntax but not out of
$GITHUB_ENV, which is parsed line by line: an input containing a newline
wrote a second, attacker-chosen variable into every later step in the job.

Validates whole-buffer with grep -qzE so an embedded newline cannot match
as a benign first line, bounds the input to 1-5 digits, then range-checks
1..86400 and fails the job loudly rather than falling back to the default.

The length bound is load-bearing: [ "$v" -lt 1 ] errors rather than
returning false on a value too large for the shell integer type, and that
failure is consumed by the surrounding if!/||, so a bare ^[0-9]+$ let a
26-digit input through unbounded. (Caught by CodeRabbit on PR #18.)
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