Skip to content

Escape untrusted issue title/author in the Slack notification workflow#375

Merged
qing-ant merged 1 commit into
mainfrom
fix/slack-issue-notification-injection
Jul 14, 2026
Merged

Escape untrusted issue title/author in the Slack notification workflow#375
qing-ant merged 1 commit into
mainfrom
fix/slack-issue-notification-injection

Conversation

@qing-ant

@qing-ant qing-ant commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Problem

.github/workflows/slack-issue-notification.yml runs on issues: [opened] and builds the Slack chat.postMessage payload by splicing two attacker-controlled values directly into a hand-written JSON string literal:

  • ${{ github.event.issue.title }}
  • ${{ github.event.issue.user.login }}

Anyone can open an issue on a public repo, so both are untrusted free text. Two consequences:

  1. The notification silently drops. A title containing a double quote terminates the JSON string early, the payload stops being valid JSON, and Slack rejects it. Nobody is alerted — the failure reads as the notifier being flaky rather than as an input-handling bug.
  2. Issue text controls the rendered message. Slack mrkdwn treats <, > and & as control characters, and the title is placed inside a <url|label> link construct. Nothing escapes them, so title text is interpreted as markup rather than shown as text, and can restructure the message a reader sees rather than appearing as its content.

This is the standard untrusted-input hygiene problem for workflows that build a payload out of event data. Reported as HackerOne #3857924.

Ordering note

The notifier appears to have never successfully delivered, so this is currently latent rather than actively exploited. That is the only reason it has not been exercised, and it is what makes the ordering matter:

Escaping must land before Slack delivery is repaired. If delivery is fixed first, the unescaped payload goes live at that moment.

Fix

  • A Build Slack payload step runs first. Untrusted values reach the shell only via env: — never through ${{ }} interpolation inside a run: body or inside a payload string.
  • The whole payload is built by jq, and the mrkdwn escape (&&amp;, then <&lt;, then >&gt;, then control chars → space) lives inside the jq program. The payload is emitted as a single step output and handed to the Slack action verbatim.
  • A self-test guards the escape. Before building the payload the step runs the same esc definition over <&> and requires &lt;&amp;&gt;. If it ever fails, the step emits a ::error:: annotation and exits non-zero — the job fails instead of posting degraded output. There is one definition of esc, shared by the self-test and the payload, so the two cannot drift.
  • permissions: {} — no step uses GITHUB_TOKEN (no checkout, no gh/API calls); the only credential is the Slack bot token, passed as a secret. So the job needs no scopes.

The message's user-visible shape is unchanged: issue link, number, title, author. This is a hardening change, not a redesign.

Why the escape moved out of bash

An earlier revision of this fix escaped with bash parameter substitution (${var//pat/repl}) plus a shopt -u patsub_replacement guard.

That guard is load-bearing. bash 5.2 — the default on ubuntu-latest — enables patsub_replacement, under which an unescaped & in the replacement half of ${var//pat/repl} expands to the matched text. The substitutions that break are the ones whose replacement starts with &: ${s//</&lt;} degrades to <lt; and ${s//>/&gt;} degrades to >gt;, leaving the raw < / > in the output — exactly the characters the escape exists to remove. (&&amp; survives unguarded only by coincidence: the text & matches is &, so &amp; re-expands to itself. Testing the guard with an ampersand therefore "passes" and proves nothing.)

Working code whose correctness depends on a shopt line three lines away is a trap: a future edit deletes the line as noise and the escape degrades with no signal. jq has no such option, so doing the escaping there removes the dependency entirely rather than documenting it. The shopt line and the bash escape function are gone.

Two details in the jq program are deliberate:

  • & must be escaped first, or the & in &lt; / &gt; gets double-escaped.
  • [[:cntrl:]], not an escaped codepoint range. jq's Oniguruma regex mangles the \u-range form — it matches ordinary letters and eats them ("Bug in <Foo> component" → all spaces). Verified on the runner's jq; [[:cntrl:]] is correct.

jq -c guarantees the payload contains no literal newline, so the key=value framing of $GITHUB_OUTPUT is safe by construction rather than by a separate stripping step.

Both SDK repos are now identical

A review found the Python and TypeScript patches had drifted (different control-char stripping, set -euo pipefail in one only, URL escaped in one only). The two workflow files are now byte-identical, and produce byte-identical payloads for every case below.

Verification

Defensive testing only; benign inputs, no attack payload constructed, nothing posted to the Slack channel.

The real workflow file was run under act on catthehacker/ubuntu:act-24.04 (bash 5.2.21, patsub_replacement on by default; jq 1.6), with only the Slack-posting step's uses: swapped for a local action that dumps the with: payload: value byte-identically. Synthetic issues.opened events:

case input title decoded blocks[0].text.text payload valid JSON 1 line py == ts
plain ascii Plain ASCII title …|#42 Plain ASCII title> yes yes byte-identical
double quote Client says "connection reset" on retry …|#42 Client says "connection reset" on retry> yes yes byte-identical
ampersand Tips & tricks …|#42 Tips &amp; tricks> yes yes byte-identical
angle brackets Bug in <Foo> component …|#42 Bug in &lt;Foo&gt; component> yes yes byte-identical
backslash Windows path C:\Users\dev fails …|#42 Windows path C:\Users\dev fails> yes yes byte-identical
newline Newlineembedded title …|#42 Newline embedded title> yes yes byte-identical
mixed Fix <A> & <B> …|#42 Fix &lt;A&gt; &amp; &lt;B&gt;> yes yes byte-identical

No case produced <lt; or >gt;. The only unescaped angle brackets in any output are the two link delimiters the workflow writes itself.

Option-independence proof. The escape block was run on bash 5.2.21 with shopt -u patsub_replacement and again with shopt -s patsub_replacement deliberately ON. Output was identical for all seven cases — which is the whole point of moving the escape into jq: there is no longer a shell option that can change the result.

Self-test fires. Deliberately corrupting the esc definition (&lt;&lt) makes the step fail:

❗  ::error::mrkdwn escape self-test failed (got '&lt&amp;&gt;'); refusing to post
❌  Failure - Main Build Slack payload
🏁  Job failed

The Slack step never runs — no payload is dumped.

Also: actionlint clean on both repos (it shellchecks the run: block too), and the YAML parses.

@qing-ant
qing-ant force-pushed the fix/slack-issue-notification-injection branch from e643292 to cb4b6af Compare July 13, 2026 16:54
@qing-ant
qing-ant marked this pull request as ready for review July 13, 2026 20:57
Issue titles and author logins on a public repo are attacker-controlled. The
workflow interpolated them with ${{ }} straight into a hand-written JSON payload
string, so a crafted title could break out of the JSON literal and rewrite the
Slack message.

Pass every untrusted value in via env: only, and build the payload with jq. The
mrkdwn escape lives inside the jq program rather than in a bash
${var//pat/repl} substitution: the bash form is only correct while
patsub_replacement is off (bash 5.2 turns it ON by default, which rewrites
`<` -> `&lt;` as `<lt;`), so its correctness would depend on a shopt line a
future edit could silently drop. jq has no such option. A self-test runs the
same esc definition the payload uses and hard-fails the job rather than posting
degraded output. jq -c also guarantees the payload is single-line, so the
key=value $GITHUB_OUTPUT framing is safe by construction.

No step needs GITHUB_TOKEN, so the job drops to permissions: {}.
@qing-ant
qing-ant force-pushed the fix/slack-issue-notification-injection branch from cb4b6af to 848afd0 Compare July 13, 2026 21:08
@qing-ant
qing-ant marked this pull request as draft July 13, 2026 22:51
@qing-ant
qing-ant marked this pull request as ready for review July 13, 2026 22:53
@qing-ant
qing-ant enabled auto-merge (squash) July 13, 2026 22:54
@qing-ant
qing-ant merged commit 34426a1 into main Jul 14, 2026
1 check passed
@qing-ant
qing-ant deleted the fix/slack-issue-notification-injection branch July 14, 2026 14:33
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