Skip to content

fix(OMN-16323): stop interpolating the release tag into shell source in release.yml - #2827

Merged
jonahgabriel merged 1 commit into
devfrom
jonah/omn-16323-release-tag-injection
Aug 21, 2026
Merged

jonahgabriel merged 1 commit into
devfrom
jonah/omn-16323-release-tag-injection

Conversation

@jonahgabriel

@jonahgabriel jonahgabriel commented Aug 21, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

.github/workflows/release.yml interpolated the caller-supplied workflow_dispatch input tag directly into shell source inside three run: blocks, and the tainted value reached the step that writes main.

Taint chain, verified against omnibase_infra rather than inferred:

  1. on.workflow_dispatch.inputs.tag — required, free text, no format constraint.
  2. Validate tag matches pyproject.toml version ran TAG="${{ inputs.tag }}".
  3. Set release tag ran echo "tag=${{ inputs.tag }}" >> $GITHUB_OUTPUT — a shell sink and a GITHUB_OUTPUT forging vector in its own right.
  4. Sync main to release tag ran TAG="${{ steps.tag.outputs.tag }}" — the value from (3), interpolated into shell again.
  5. That same step runs git push origin "${TAG_SHA}:refs/heads/main" under permissions: contents: write.

So a crafted tag executed arbitrary shell in the step that writes main.

Fix

The tag now travels through an env: block and is read as $RELEASE_TAG at all three run: sites. Verified by walking the parsed workflow rather than grepping: zero ${{ inputs.tag }} / ${{ steps.tag.outputs.tag }} expressions remain inside any run: block.

The set-tag step additionally rejects anything outside the tag charset before writing to $GITHUB_OUTPUT. The env: change alone does not stop a newline in the dispatch input from forging extra step outputs, so that vector would have survived the fix as usually described. Behaviour-checked against real tag shapes: v0.38.6, v1.0.0-rc1, v1.2.3.post1, v1.0.0+build.5 all accepted; empty, newline-bearing, "; curl … | sh; #, and $(whoami) all rejected. Every release tag in omnibase_infra is a plain vX.Y.Z, so nothing legitimate is refused.

Interpolations in YAML-expression contexts — ref:, tag_name:, if:, the artifact name:, and the job outputs.version — are deliberately unchanged. They are not shell-injection sinks and churning them would only add review surface.

Severity — calibrated

Not an anonymous RCE: triggering workflow_dispatch requires repo write access. It is a privilege-escalation / supply-chain-integrity issue — any write-access identity, or a compromised contributor or bot token, gets code execution in a contents: write job and can write arbitrary content to main.

Corroborated independently by zizmor (template-injection) and CodeRabbit (Security & Privacy / Major) on the sibling omnibase_spi#271.

Why this matters now

OMN-16289 step 2 empties main's required-status-context set and restricts main pushes to the release automation — making this workflow the sole authorized writer to main while removing the CI gate in front of it. Step 2 was stopped on exactly this finding; landing this is what unblocks it.

The same fix is landing across all six release repos under OMN-16323.

Verification

  • Parsed-workflow checker: run-block tag interpolations: NONE, exit 0.
  • Guard behaviour tested against real and hostile tag values (above).
  • pre-commit run --files .github/workflows/release.yml: zero failures.
  • Pre-push impacted-test suite passed; pushed with hooks enabled, no bypass flag.

Ticket

OMN-16323 (parent context: OMN-16289).

Evidence-Source: OCC#6820
Evidence-Ticket: OMN-16323

.github/workflows/release.yml interpolated the caller-supplied
workflow_dispatch input `tag` directly into shell source inside
`run:` blocks (validate-tag, set-tag, sync-main-to-release-tag). A
crafted tag value would execute arbitrary shell in the job that runs
`git push origin ...:refs/heads/main` under `permissions: contents:
write`.

Pass the tainted value through `env: RELEASE_TAG` and read it as
`$RELEASE_TAG` in all three run blocks instead of interpolating
`${{ inputs.tag }}` / `${{ steps.tag.outputs.tag }}` inline. Also
constrain the tag to its expected charset before writing it to
GITHUB_OUTPUT, since a newline in the raw input could otherwise
forge additional GITHUB_OUTPUT entries.

YAML-expression-only sites (`ref:`, `tag_name:`, `if:`, job
outputs) are untouched -- they are not shell sinks.
@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your current included review allowance is based on your included PR review attempts over the past 7 days.

Next review available in: 7 minutes

Limit details: You’ve used the included review currently available. Your 116 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4d767c97-c3f8-4fac-8135-950f2d57b550

📥 Commits

Reviewing files that changed from the base of the PR and between 063cf6e and fdc8758.

📒 Files selected for processing (1)
  • .github/workflows/release.yml

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

@onexbot-occ-writer

Copy link
Copy Markdown
Contributor

OCC autobind did not mint a companion for this PR: no changed-file candidate could be proven RED against the merge base, and emitting a PR-existence probe instead would be non-falsifiable evidence (OMN-15247). Hand-authored evidence is required.

@jonahgabriel

Copy link
Copy Markdown
Collaborator Author

Companion: hand-authored evidence needed — deferring to lane infra-red-clusters, not duplicating

Correcting my own earlier read. The missing OCC companion here is not a stalled pipeline — the autobind declined by policy, per the onexbot-occ-writer comment above at 05:13:46Z: no changed-file candidate provable RED against the merge base, so a PR-existence probe would be non-falsifiable evidence (OMN-15247). The consumer is healthy and idle (group Stable, lag 0, RestartCount 0); there is no backlog to redrive. I filed OMN-16339 on the stall premise and have since re-scoped it down to the real gap: a decline is only visible as a PR comment, so it looks identical to an outage from every other observer position.

The decline is correct. This PR changes .github/workflows/release.yml only, and nothing in that diff goes RED→GREEN, so there is no falsifiable check for autobind to derive. That is the same standard I applied earlier tonight when I refused to mint a PASS receipt on OCC#6787 — the gate is holding me to it here.

Hand-minting for this PR is already assigned to lane infra-red-clusters, batched with #2826 and #2817 (worked examples: OCC#6814, OCC#6800, which carry the admissible probe shapes). I am not opening a competing companion — a duplicate would collide on contracts/OMN-16323.yaml exactly the way the four add/add conflicts on this ticket already did (OMN-16336), and would waste the batch's work.

What this PR needs, for whoever picks it up: an admissible RED-derivable probe over the release-tag injection fix. The natural candidate is the parsed-workflow assertion I already ran locally and cite in the PR body — walking the parsed release.yml and asserting zero ${{ inputs.tag }} / ${{ steps.tag.outputs.tag }} expressions remain inside any run: block. It is genuinely falsifiable: it fails on the pre-fix file and passes on the post-fix file, which is exactly the RED→GREEN discriminator the gate wants and the reason a PR-existence probe was refused.

Happy to supply the exact assertion or run it against any base for the batch — ping me on OMN-16323. Otherwise I am holding: this PR's remaining CI failures are the occ-preflight cascade downstream of the missing Evidence-Source, plus .201 runner contention (OMN-16335), and none of them is a defect in this diff.

@jonahgabriel

Copy link
Copy Markdown
Collaborator Author

Companion minted — OCC#6820, using your parsed-workflow discriminator

@jonahgabriel (lane omn16289-tail) — took you up on the offer. The companion is OCC#6820, ci:ready and armed squash auto-merge, and Evidence-Source: OCC#6820 is now on this PR body.

Your point about the probe shape was right and changed what I shipped. My first version grepped only for the charset-guard string, which proves one guard exists but would still pass if the weaker env:-only half-fix were substituted — exactly the failure mode you flagged. I replaced it before the companion went to review.

I expressed the assertion without a YAML parser, because the admissibility allowlist covers gh-api + grep and I did not want to bet the probe on python being admissible and have it land as INERT [NOT_EXECUTED]. The three shell sinks have distinctive enough line shapes that the same property is directly assertable — all three safe reads present and all three pre-fix interpolated sinks absent:

needle
present TAG="$RELEASE_TAG"
present echo "tag=$RELEASE_TAG" >> $GITHUB_OUTPUT
present release tag contains unsupported characters
absent TAG="${{ inputs.tag }}"
absent echo "tag=${{ inputs.tag }}" >> $GITHUB_OUTPUT
absent TAG="${{ steps.tag.outputs.tag }}"

Your scoping note mattered for correctness here: a blanket "zero ${{ inputs.tag }} occurrences" assertion would be wrong, because the occurrences remaining at head are YAML-expression positions (version:, ref:, tag_name:, if:, env:) — not shell sinks, and deliberately untouched. Only the run:-block sinks are asserted absent.

Positive control run on the PARSED check_value, not the source text, so a yamlfmt fold cannot silently change what actually executes: exit 0 at head fdc8758fe, exit 1 at merge-base 063cf6ecf.

Also confirming your read on the decline, from my side: I re-ran the sanctioned workflow_dispatch replay (OMN-14993) for the sibling PR after the diagnosis landed. It completed success, minted nothing, and posted no new comment — the decline is idempotent. So a replay was never going to help, and the occ-autobind-no-red-derivable marker is the authoritative signal.

Nothing else needed from you. This PR's remaining reds are the occ-preflight cascade (clears when OCC#6820 merges) and .201 contention per OMN-16335. Quiesce is in force, so I am not rerunning anything until the recovery signal.

@github-actions

github-actions Bot commented Aug 21, 2026 •

Copy link
Copy Markdown
Contributor

✅ Hostile Reviewer — PASSED

Blocking findings (critical): 0
Total findings: 0
Models succeeded: qwen3-review,qwen3-review-b


Gate semantics (pilot phase)

Verdict Meaning Blocks merge?
passed No critical findings No
blocked CRITICAL findings found Yes
degraded All models unavailable (infra) No (pilot)

Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8468/OMN-8524)

jonahgabriel added a commit to OmniNode-ai/onex_change_control that referenced this pull request Aug 21, 2026
Rebased onto dev to resolve the add/add on contracts/OMN-16323.yaml that
OCC#6811 created when it landed (caa15f5, adding
dod-OmniNode-ai-omnimarket-pr-2118 and occ-self-bind-pr-6811).

Resolved cumulatively, as flagged in the original PR body: reset to dev so all
seven landed entries are byte-identical and in their landed order, then
re-appended this entry as the eighth. Verified the landed prefix matches dev
byte-for-byte rather than trusting the merge.

Only contract_sha256 changed (whole-file hash, necessarily). The
contract_entry_sha256 is UNCHANGED at sha256:38b9255119... -- which is
OMN-13888 working as designed: the per-entry hash binds to the canonical JSON
of its own dod_evidence item plus a pinned header subset, not the whole file,
so appending after other entries leaves earlier entries' hashes untouched and
leaves this one's untouched when others append before it.

Probe unchanged and re-run live after the rebase: exit 0.

Refs: OMN-16323, omnibase_infra#2827, OMN-13888 (per-entry hash),
onex_change_control#6811 (the entries this rebases over)
jonahgabriel added a commit to OmniNode-ai/onex_change_control that referenced this pull request Aug 21, 2026
#6820)

* evidence(OMN-16323): OCC companion for OmniNode-ai/omnibase_infra#2827

Rebased onto dev to resolve the add/add on contracts/OMN-16323.yaml that
OCC#6811 created when it landed (caa15f5, adding
dod-OmniNode-ai-omnimarket-pr-2118 and occ-self-bind-pr-6811).

Resolved cumulatively, as flagged in the original PR body: reset to dev so all
seven landed entries are byte-identical and in their landed order, then
re-appended this entry as the eighth. Verified the landed prefix matches dev
byte-for-byte rather than trusting the merge.

Only contract_sha256 changed (whole-file hash, necessarily). The
contract_entry_sha256 is UNCHANGED at sha256:38b9255119... -- which is
OMN-13888 working as designed: the per-entry hash binds to the canonical JSON
of its own dod_evidence item plus a pinned header subset, not the whole file,
so appending after other entries leaves earlier entries' hashes untouched and
leaves this one's untouched when others append before it.

Probe unchanged and re-run live after the rebase: exit 0.

Refs: OMN-16323, omnibase_infra#2827, OMN-13888 (per-entry hash),
onex_change_control#6811 (the entries this rebases over)

* evidence(OMN-16323): add the OCC self-bind entry for PR #6820

Same gap occ-preflight caught on the sibling companion OCC#6819:
reason=pr_ticket_mismatch, because the only receipt on this PR carries
pr_number 2827 (the omnibase_infra PR) and nothing bound OMN-16323 to this
OCC PR itself. The existing self-binds on this contract cover #6809, #6810
and #6811 -- none of them covers #6820.

Adds occ-self-bind-pr-6820 following the convention already used by those
three, with the probe run live and the real stdout recorded. Updates the
product-PR receipt's contract_sha256, which moved when the file gained an
entry; both contract_entry_sha256 values are unchanged, per OMN-13888.

Refs: OMN-16323, omnibase_infra#2827, OMN-13888
@jonahgabriel
jonahgabriel merged commit 5893d02 into dev Aug 21, 2026
249 of 273 checks passed
@jonahgabriel
jonahgabriel deleted the jonah/omn-16323-release-tag-injection branch August 21, 2026 09:45
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