Skip to content

fix(workflows): randomize $GITHUB_OUTPUT heredoc delimiter (defense in depth) - #545

Merged
marktoda merged 1 commit into
mainfrom
fix/gh-output-delim
May 20, 2026
Merged

marktoda merged 1 commit into
mainfrom
fix/gh-output-delim

Conversation

@dgilmanuni

Copy link
Copy Markdown
Contributor

Summary

Shell-driven steps in this repo write multiline values to $GITHUB_OUTPUT using static heredoc delimiters (EOF, CHANGELOG, COVERAGE, etc.). Per the multiline-string spec, the parser terminates a value the moment it reads a line equal to the active delimiter and resolves duplicate keys as last-write-wins. A static delimiter inside any caller-controlled heredoc body is a structural injection sink.

None of the patched sites in this repo are currently exploitable — their heredoc bodies are derived from deterministic sources (git log, git diff, forge coverage, internal Pulumi config, etc.) with no current path for attacker-controlled freeform text. Patched as defense-in-depth so a future edit that splices caller-controlled content into any of these bodies does not silently introduce the class. Mirrors the per-call random delimiter already shipped in Uniswap/ai-toolkit/.github/scripts/build-prompt.ts:generateUniqueDelimiter ("to prevent injection attacks", commit 7159a4f).

What changed

Each affected site now:

  • Generates a delimiter as <PREFIX>_DELIMITER_<date +%s%N>_<openssl rand -hex 16> (128-bit CSPRNG + nanosecond timestamp).
  • Asserts the random component is non-empty before use (bash -e does not abort on a failing command substitution inside a variable assignment; without this, an openssl regression would silently degrade the delimiter to a predictable prefix+timestamp).
  • Refuses to write values exceeding 900KB (conservative cap below the $GITHUB_OUTPUT 1MB per-value limit; >1MB writes truncate mid-body and leave an unclosed heredoc that the parser misinterprets).

Session context

Discovered during the bug bounty review that produced Uniswap/default-token-list#2484 and Uniswap/ai-toolkit#509 — the exploitable instances of the same class.

A wider grep across all Uniswap public repos found this anti-pattern in this repo, where the heredoc body is a deterministic value with no current attacker input path. Including the fix here is hygiene + future-proofing.

Test plan

The pattern was pressure-tested locally in the context of the exploitable repos:

  • Attacker injection contained — embedded inside the legitimate body, not promoted to a duplicate key.
  • Benign body round-trips correctly.
  • Delimiter uniqueness across runs (128-bit CSPRNG).
  • openssl-empty abort (simulated via PATH override; step exits non-zero, zero bytes written).
  • Oversize abort (900001-byte body; step exits non-zero, zero bytes written).
  • Just-under-cap success (899999-byte body writes successfully).

All assertions pass. YAML in this PR validates with yaml.safe_load.

To verify post-merge: trigger the relevant workflow run and confirm the step output is consumed correctly downstream.

Shell-driven steps in this repo write multiline values to $GITHUB_OUTPUT
using static heredoc delimiters (`EOF`, `CHANGELOG`, `COVERAGE`, etc).
Per the multiline-string spec
(https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#multiline-strings)
the parser ends a multiline value as soon as it reads a line equal to
the active delimiter, and resolves duplicate keys as last-write-wins.
A static delimiter inside any caller-controlled heredoc body is a
structural injection sink.

None of the patched sites in this repo are currently exploitable —
their heredoc bodies are derived from deterministic sources (git log,
git diff --name-only, forge coverage, internal Pulumi config, JSON
discovery output). Patched as defense-in-depth so a future edit that
splices caller-controlled freeform text into any of these bodies does
not silently introduce the class. Mirrors the per-call random delimiter
already shipped in Uniswap/ai-toolkit/.github/scripts/build-prompt.ts
(generateUniqueDelimiter, "to prevent injection attacks", commit 7159a4f).

Each site now:
- Generates a delimiter as PROMPT_DELIMITER_<date +%s%N>_<openssl rand -hex 16>
  (128-bit CSPRNG + nanosecond timestamp).
- Asserts the random component is non-empty before use (bash -e does not
  abort on a failing command substitution inside a variable assignment;
  without this, an openssl regression would silently degrade the
  delimiter to a predictable prefix+timestamp).
- Refuses to write values exceeding 900KB (conservative cap below the
  $GITHUB_OUTPUT 1MB per-value limit; >1MB writes truncate mid-body
  and leave an unclosed heredoc that the parser misinterprets).

Discovered during the bug bounty review that produced
Uniswap/default-token-list#2484 and Uniswap/ai-toolkit#509 — the
exploitable instances of the same class. This PR addresses the same
anti-pattern in this repo where it appears as hygiene (no current
attacker input path).

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

vercel Bot commented May 18, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hooklist Ready Ready Preview, Comment May 18, 2026 6:39pm

Request Review

@marktoda marktoda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verified the diff:

  • Per-call delimiter HOOK_FILES_DELIMITER_$(date +%s%N)_$(openssl rand -hex 16) (128 bits of CSPRNG + nanosecond timestamp).
  • [ -n "$HF_DELIM_RAND" ] guard handles the silent openssl failure case (bash -e doesn't abort on a failed command substitution inside variable assignment).
  • 900KB cap below the 1MB $GITHUB_OUTPUT per-value limit.

Defense-in-depth is meaningful here: $HOOK_FILES is gh api .../pulls/{n}/files --jq '.[].filename', and on a PR a malicious fork can in principle add a file whose path contains \n EOF \n to break the heredoc. CI is green. LGTM.

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.

2 participants