Skip to content

chore(ci): validate bumper version input before downstream use#60

Merged
dewet22 merged 2 commits into
mainfrom
chore/harden-bumper-input-validation
May 27, 2026
Merged

chore(ci): validate bumper version input before downstream use#60
dewet22 merged 2 commits into
mainfrom
chore/harden-bumper-input-validation

Conversation

@dewet22

@dewet22 dewet22 commented May 27, 2026

Copy link
Copy Markdown
Owner

What

Adds a single regex validation guard near the top of bump-givenergy-modbus.yml's "Determine target version and base branch" step. Rejects any NEW that doesn't match ^[0-9]+\.[0-9]+\.[0-9]+(-?[a-z0-9.]+)?$, with ::error:: + exit 1.

Why

NEW is sourced from github.event.client_payload.version / inputs.version and flows into several shell-interpolated contexts downstream:

Site Vector
branch="bump/givenergy-modbus-${NEW}" + git checkout -B "$branch" + git push -f origin "$branch" Git ref injection (:, ^, ..)
echo "new=$NEW" >> "$GITHUB_OUTPUT" GITHUB_OUTPUT injection via newline
Python pattern.subn(rf'\g<1>{new}\g<2>', text) in the bump step Regex backref subversion via \g<...>
git commit -m "...${NEW}", gh pr create --title "..." --body "...@${NEW}](.../v${NEW})..." Log/markdown injection

The trust boundary is the BUMP_PAT used to fire the repository_dispatch — small but non-zero attack surface (PAT compromise, supply-chain). A single validation guard at the top defends every downstream use rather than scattering escapes everywhere.

Acceptance behaviour (verified locally)

Accepts every historical modbus release tag — 2.0.4, 2.0.0rc1, 2.0.0a6, 2.0.0a3 — plus forward-looking PEP 440 forms (.dev0, .post1, -alpha.1).

Rejects empty string; every shell metacharacter (;, \$(), backticks, |, &); path traversal (../etc); newline injection; regex backref injection (\g<0>); quote injection; git ref operators (:, ^, ..); branch-style strings (main, v2.0.0).

What's not in scope

  • The two stale orphan branches on origin (bump-python-floor-for-modbus-v2, claude/beautiful-chaplygin-c3f28d) — separate cleanup.
  • Pre-existing dashboard/generate.py ruff format drift on main — unrelated.

Refs: GitHub Actions workflow injection vulnerabilities

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Chores
    • Added early validation in the release workflow to enforce a PEP 440–like X.Y.Z (optional suffix) version format, emitting an error and halting the job for invalid versions to prevent malformed version strings from progressing.

Review Change Stack

`NEW` is sourced from `github.event.client_payload.version` /
`inputs.version` and flows into git refs (`branch=bump/givenergy-modbus-${NEW}`,
`git checkout -B`, `git push -f origin`), commit messages, PR titles and
bodies, `GITHUB_OUTPUT`, and — most subtly — a Python `re.sub` backref
template (`\g<1>{new}\g<2>`) in the bump step. The trust boundary is
the BUMP_PAT used to fire the dispatch; a compromised PAT shouldn't be
able to inject shell metacharacters, newlines (GITHUB_OUTPUT injection),
git ref operators (`:`, `^`, `..`), or `\g<...>` regex backrefs.

Validates against a SemVer-ish regex that accepts every modbus release
tag in history (`X.Y.Z`, `X.Y.ZrcN`, `X.Y.ZaN`) plus forward-looking
PEP 440 `.dev`/`.post` suffixes, rejecting everything else. A single
guard at the top of the step defends every downstream use.

Refs: https://github.blog/security/vulnerability-research/how-to-catch-github-actions-workflow-injections-before-attackers-do/

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@gemini-code-assist

Copy link
Copy Markdown

Note

Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported.

@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e7126e85-d1af-4303-ac65-14ae3c921a06

📥 Commits

Reviewing files that changed from the base of the PR and between fba6bfc and 43ea36f.

📒 Files selected for processing (1)
  • .github/workflows/bump-givenergy-modbus.yml
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/bump-givenergy-modbus.yml

📝 Walkthrough

Walkthrough

Adds version format validation to the givenergy-modbus bump workflow. A bash regex check now enforces PEP 440-ish format (X.Y.Z with optional -suffix) for the target NEW variable before it flows into downstream git operations, commit messages, and Python substitutions, failing early with an error if the format is invalid.

Changes

Version Validation in Bump Workflow

Layer / File(s) Summary
Target version format validation
.github/workflows/bump-givenergy-modbus.yml
Adds a bash regex validation step that checks the NEW version variable matches X.Y.Z (optionally with -suffix) before it is used in git refs, commit messages, PR bodies, and Python substitution, with early error exit on mismatch and inline comments describing the validation boundary and injection-risk mitigation.

🎯 4 (Complex) | ⏱️ ~45 minutes

"🐰 I sniffed a version string tonight,
X.Y.Z I asked—was it dressed up right?
If suffixes wander or formats stray,
I hop back home and send them away.
Clean gates keep branches calm and bright."

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: adding version input validation before downstream use in the CI workflow.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/harden-bumper-input-validation

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

@dewet22

dewet22 commented May 27, 2026

Copy link
Copy Markdown
Owner Author

I found one edge case worth tightening before this lands: the new validation still allows consecutive dots in the suffix because [a-z0-9.]+ accepts empty dot-separated segments.

For example, these pass the regex locally:

2.0.0..
2.0.0.post..1

They then flow into branch="bump/givenergy-modbus-${NEW}", where git rejects them as invalid branch names:

fatal: 'bump/givenergy-modbus-2.0.0..' is not a valid branch name

That also means the current PR description overstates the coverage a little, since .. is not fully rejected yet. I would either tighten the pattern so dot-separated suffix segments cannot be empty, or add an explicit [[ "$NEW" == *..* ]] rejection before writing to GITHUB_OUTPUT.

Everything else I checked in the workflow looked aligned with the intended hardening, and the GitHub checks are green.

The original `[a-z0-9.]+` class accepted `2.0.0..`,
`2.0.0.post..1`, `2.0.0.`, and similar — values that pass the
regex but then break `git checkout -B "bump/givenergy-modbus-${NEW}"`
downstream with "not a valid branch name".

Tighten the suffix to `[-.]?[a-z0-9]+(\.[a-z0-9]+)*` so each
dot-separated segment is required to contain at least one
alphanumeric character. All historical and forward-looking PEP 440
tags still pass; the `..` / trailing-`.` family no longer does.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dewet22

dewet22 commented May 27, 2026

Copy link
Copy Markdown
Owner Author

Fixed in 43ea36f — tightened the suffix to [-.]?[a-z0-9]+(\.[a-z0-9]+)* so each dot-separated segment must contain at least one alphanumeric. The 2.0.0.. / 2.0.0.post..1 / trailing-. family all reject at validation time now, before GITHUB_OUTPUT is written or git checkout -B is invoked. The legitimate release-tag set (every modbus tag in history + PEP 440 .dev/.post/-alpha.N) is unchanged — verified locally against the same test corpus plus the two examples you flagged.

Also fleshed out the inline comment to call out the empty-segment carve-out explicitly so future-us doesn't loosen it again without thinking.

@dewet22 dewet22 merged commit 323a552 into main May 27, 2026
8 checks passed
@dewet22 dewet22 deleted the chore/harden-bumper-input-validation branch May 27, 2026 20:48
@dewet22 dewet22 mentioned this pull request May 28, 2026
3 tasks
dewet22 added a commit that referenced this pull request May 28, 2026
Squash-merges the release/1.1.0 branch into main.

Changes since v1.0.2:
- feat: persist PlantCapabilities across restarts (#48)
- feat: expose_recommended_entities service + voice/LLM docs (#65)
- docs: GivTCP → givenergy_local migration guide and script (#67)
- docs: passive mode and parallel-running notes
- build: bump requires-python floor to >=3.14.2 (#61)
- chore: retire v1.0 branch, bump givenergy-modbus to >=2.0.4,<2.1 (#59)
- chore: auto-bumper version-format validation (#60)
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