Skip to content

Harden release.yml sed version substitutions against metacharacter breakage - #261

Merged
mikebronner merged 4 commits into
mainfrom
chore/243-harden-releaseyml-sed-version-substitutions-agains
Jul 14, 2026
Merged

mikebronner merged 4 commits into
mainfrom
chore/243-harden-releaseyml-sed-version-substitutions-agains

Conversation

@mikebronner

@mikebronner mikebronner commented Jul 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Implements #243 — defense-in-depth follow-up from the review of #240 (PR #242). The "Update versions and lock files" step embedded $VERSION (tag-derived) unescaped into three sed substitution expressions; a tag containing sed metacharacters (/, &, \) would break them.

Changes

  • Add a fail-fast semver guard in the extract_version step of .github/workflows/release.yml, immediately after VERSION="${TAG#v}" and before the $GITHUB_OUTPUT write (so a hostile workflow_dispatch tag can't inject step outputs either).
  • Guard regex is exactly the one specified in the AC: ^[0-9]+\.[0-9]+\.[0-9]+([-.][0-9A-Za-z.-]+)?$ — permits only digits, dots, and [-.][0-9A-Za-z.-] suffixes, so no sed metacharacter, quote, space, or newline can pass.
  • All three sed call-sites inherit the guarantee from the single guard — no per-site escaping needed.
  • CI unblock (unrelated to Harden release.yml sed version substitutions against metacharacter breakage #243's subject but required for a green gate): clippy 1.97 (current stable on CI runners) newly flags three clippy::question_mark sites on main (livewire_resolver.rs:210, query_chain/cursor.rs:393, main.rs:8166), and CI runs clippy --all-targets -- -D warnings, so every PR was red. Replaced the three match/if-let blocks with the equivalent ? expressions — behavior unchanged, two atomic style commits.

Acceptance Criteria

  • release.yml — Cargo.toml sed: $VERSION substitution safe (strict semver validation up front)
  • release.yml — extension.toml sed: same hardening
  • release.yml — laravel-lsp/Cargo.toml sed: same hardening
  • Single up-front guard: $VERSION validated against ^[0-9]+\.[0-9]+\.[0-9]+([-.][0-9A-Za-z.-]+)?$ immediately after VERSION="${TAG#v}", step fails on mismatch, all three call-sites inherit the guarantee
  • Confirmed no other unescaped interpolation of tag-derived values into shell tools remains: TAG_NAME in the "Update tag and release" curl calls runs in the same job after the guard step (guard failure kills the job first), and a guarded VERSION constrains the tag itself to v? + [0-9A-Za-z.-] — no /, &, \, or " can reach the URL or JSON body. matrix.*/runner.os are workflow constants; the softprops tag_name: is an action input, not shell interpolation.

Test Plan

  • Regex matrix (local bash, same [[ =~ ]] semantics as the runner): accepts 1.2.3, 0.6.0, 1.2.3-beta.1, 1.2.3.4, 10.20.30-rc-2; rejects 1.2, v1.2.3, 1.2.3/x, 1.2.3&, 1.2.3\1e, 1.2.3"; curl evil, trailing space, empty string, newline-injection payload
  • Full step simulation under bash -e: benign tags (v0.6.1, v1.2.3-beta.1) → rc=0, version= output written; hostile tags (v1.2.3/&\, v1.2.3"; curl evil) → rc=1, ::error:: emitted, nothing written to $GITHUB_OUTPUT
  • YAML parses cleanly (ruby YAML.load_file)
  • Clippy fixes verified locally on clippy 1.97 with --all-targets -- -D warnings (same invocation as CI); cargo fmt --check clean; all 2553 LSP tests pass
  • CI green

Fixes #243

Validate $VERSION against a strict semver regex immediately after
extraction in release.yml and fail the step on mismatch, before anything
is written to $GITHUB_OUTPUT. The three sed version substitutions, the
version-bump commit message, and the tag/release curl calls all consume
this value (or the tag it derives from), so the single up-front guard
keeps sed metacharacters (/, &, \) and quote-breakers out of every
downstream call-site.

Fixes: #243
@mikebronner
mikebronner marked this pull request as ready for review July 14, 2026 18:27
Clippy 1.97 (current stable on CI runners) flags both match-on-Option
blocks under the question_mark lint, and CI runs clippy with
-D warnings, so main is red on every PR. Behavior is unchanged —
both sites collapse to the equivalent ? expression.
Third site clippy 1.97 flags under the question_mark lint (this one
only surfaced once the first two stopped aborting compilation).
Verified clean locally on clippy 1.97 with --all-targets -D warnings —
the same invocation CI runs. Behavior unchanged; all 2553 tests pass.

@mr-sherlock-holmes mr-sherlock-holmes 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.

✅ Approved

Review Summary

Reviewed PR #261 against #243's five acceptance criteria — fanned out AC-conformance, correctness, security, and test-honesty lenses over the checkout, then adversarially verified the one in-PR security candidate.

  • All 5 AC met. The single up-front guard (release.yml:51-55) uses the AC's exact regex ^[0-9]+\.[0-9]+\.[0-9]+([-.][0-9A-Za-z.-]+)?$, placed immediately after VERSION="${TAG#v}", with ::error:: + exit 1 on mismatch. All three sed sites (Cargo.toml, extension.toml, laravel-lsp/Cargo.toml), the commit message, and the curl calls inherit a validated value — AC #4's "single guard, all call-sites inherit" strategy, exactly as specified. AC #1–#3 are met by the validation branch the AC explicitly permits ("strict semver validation and/or escaped replacement"). AC #5 confirmed by a full-file scan: no residual unescaped tag-derived value reaches a shell tool.
  • Guard verified against the threat: it rejects /, &, \, ;, spaces, and newline payloads (anchored ^…$, charset [0-9A-Za-z.-]) while accepting every existing and legitimate tag (0.1.0…0.6.0, 1.2.3-beta.1). A crafted tag can no longer break the sed substitutions.
  • CI green — clippy -D warnings, LSP tests, extension wasm check all pass.

What's Good

  • Chose the AC's preferred single-guard approach: one validation covers all five downstream consumers instead of re-delimiting/escaping each sed site independently. Correct, minimal, and maintainable.
  • The three Rust ?-operator refactors (livewire_resolver.rs:210, main.rs:8166, query_chain/cursor.rs:396) are mechanical clippy question_mark fixes and are appropriate to include here: the floating dtolnay/rust-toolchain@stable surfaced pre-existing lints that -D warnings now fails on, so this PR's CI cannot go green without them. I verified each is an exact behavioral equivalent — same return type, same early-return-None cases, same clone semantics, and in main.rs the loop body and trailing if !found_in_previous { return None; } moved correctly out of the if let.

📋 Non-blocking follow-ups

  • Curl step re-derives the raw tag instead of the validated output — release.yml:136-161. The "Update tag and release" curl interpolates raw ${TAG_NAME} (github.event.release.tag_name), not the guard-validated steps.extract_version.outputs.version. Safe today (the guard runs earlier in the same job and exit 1 aborts before this step, and for a release event TAG_NAME is the same value the guard validated), but the safety rides on same-job ordering rather than on consuming the validated value — a future reorder or job-split would silently re-open it. Routing curl through the validated output makes the guarantee structural. Outside this PR's changed lines and beyond AC #5 (which is met by confirmation), so non-blocking.
  • Guard's error path echoes the raw tag — release.yml:53. echo "::error::Tag '${TAG}' …" prints the pre-strip raw tag; a newline in a workflow_dispatch tag input could inject an extra annotation-class workflow command. Adversarially verified as not a blocker: env-var indirection blocks any shell/sed injection, only non-executing annotation commands survive (the RCE-capable set-env/add-path/set-output were disabled in 2020), ::stop-commands:: is inert because exit 1 follows immediately, and exploitation requires an already-write-privileged actor. Cosmetic log-hygiene — worth neutralizing while touching this file, but not a merge gate.

(FYI, no action needed: the regex is marginally looser than "strict semver" — it accepts 1.2.3.4 and 01.2.3 — but that is the AC's own verbatim pattern, and both forms contain only [0-9.], so metacharacter-safety is fully preserved. Not a defect against this PR.)

Both follow-ups are the same theme (residual tag-derived-value handling on non-critical release.yml paths) and are being tracked as one defense-in-depth issue.

Ready for @mikebronner to merge.

@mikebronner
mikebronner merged commit 132f05a into main Jul 14, 2026
5 checks passed
@mikebronner
mikebronner deleted the chore/243-harden-releaseyml-sed-version-substitutions-agains branch July 14, 2026 19:46
mikebronner added a commit that referenced this pull request Jul 15, 2026
Two defense-in-depth tightenings from Holmes's review of #243 (PR #261),
turning "safe-by-ordering" into "safe-by-construction":

- The "Update tag and release" curl step now consumes the guard-validated
  steps.extract_version.outputs.tag instead of the raw
  github.event.release.tag_name, so the URL/JSON interpolations stay
  hardened even if steps are reordered or split across jobs. The guard
  step exports the tag as an output only after the semver check passes
  (TAG is structurally VERSION or v${VERSION} at that point).
- The guard's ::error:: echo strips CR/LF from the raw tag/version before
  interpolation, so a newline in a workflow_dispatch tag input can't
  split the annotation and forge an additional workflow command.

No change to the guard's pass/fail logic, the sed substitutions, or the
commit step.

Fixes #262
mikebronner added a commit that referenced this pull request Jul 15, 2026
…dated version output (#268)

* chore: start work on #262

* ci: 🔒️ harden residual tag-derived values in release.yml

Two defense-in-depth tightenings from Holmes's review of #243 (PR #261),
turning "safe-by-ordering" into "safe-by-construction":

- The "Update tag and release" curl step now consumes the guard-validated
  steps.extract_version.outputs.tag instead of the raw
  github.event.release.tag_name, so the URL/JSON interpolations stay
  hardened even if steps are reordered or split across jobs. The guard
  step exports the tag as an output only after the semver check passes
  (TAG is structurally VERSION or v${VERSION} at that point).
- The guard's ::error:: echo strips CR/LF from the raw tag/version before
  interpolation, so a newline in a workflow_dispatch tag input can't
  split the annotation and forge an additional workflow command.

No change to the guard's pass/fail logic, the sed substitutions, or the
commit step.

Fixes #262
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.

Harden release.yml sed version substitutions against metacharacter breakage

1 participant