fix(tasks): escape Slack markup by default, closing link/label injection

[sroussey]

Follow-up to #744, targeting the same base branch. Branched independently of #800, which is the other half of the same review.

M3 — Slack link/label injection is live

SlackNotifyTask.ts's BROADCAST_SIGIL = /<!(?!date\^)/g escapes only <!. So <https://evil.example|Deploy succeeded> passes both the lexical escape and the deep walk untouched, and link_names: false governs bare @name text rather than control sequences. Slack renders the LABEL in place of the URL, so a notification assembled from a fetch result or a model summary — the exact threat model every other control on this task exists for — can display a phishing destination as a status line.

Two existing tests pinned that behaviour as a guarantee ("a caller-supplied <url|label> reaches Slack verbatim"). Under this task's own threat model that guarantee is the vulnerability, since the label is attacker-chosen. Both halves of each assertion survive, moved behind the new flag.

text and blocks need different policies

Full &/</> escaping cannot be applied by neutralizeSlackBroadcastsDeep to every string leaf. That walk is shape-agnostic by design and reaches url, image_url, value and action_id leaves as readily as text ones — escaping & there corrupts every query string it touches (?a=1&b=2 → ?a=1&b=2). <! is side-effect-free to escape everywhere precisely because it has no legitimate occurrence in those fields; & does.

So:

• text → escapeSlackText — & FIRST, then <, then >. Order is load-bearing: escaping the brackets first would have the & pass re-escape the ampersands they just introduced, and <c> would arrive as <c>.
• blocks → stripSlackLinkLabels — /<(?!!)([^<>|]*)\|[^<>]*>/g → <$1>. (?!!) exempts the <!…> control family (the date token is <!date^…|Nov 14>, whose | part is a fallback, not a masking label); [^<>|] keeps a match inside one sequence rather than spanning two independent links.
• neutralizeSlackBroadcastsDeep takes a { readonly stripLabels: boolean } policy, required and explicit at both call sites rather than defaulted.
• Order within a leaf: broadcast escape first. Reversed, <https://x/|<!channel>> survives — the label's inner < prevents MASKED_LINK from matching, so nothing is stripped and the masked link is merely escaped and left live. There is a test for exactly this.

Why the new port defaults to false

• The PR's own threat model is piped/model-generated content, and an opt-in control nobody sets protects nobody.
• Every other control here fails closed.
• These tasks do not exist on main, so default-on has zero released blast radius — this is the only moment the safe default is free.
• The escaped message stays perfectly readable; only clickability is lost.

Why a new port rather than reusing allow_mentions

Wanting a build link in a deploy notification is orthogonal to wanting @channel to ping four hundred people. allow_mentions implies allow_markup, so the ladder has no dead rung (live mentions with dead links is not a state anyone asks for) — there is a test for that too.

The comment at SlackNotifyTask.ts argued against a second port on the grounds it would have "no behavior behind it". That is no longer true, and the comment is updated to say so.

username / icon_emoji stay on the broadcast escape only: whether Slack un-escapes entities in a display name is unverified, so escaping there risks a literal & in a bot name for no attested gain — and neither field renders a link, so masked-link injection has nowhere to land. Commented in place, mirroring the existing "defence in depth, NOT a closed bypass" note.

Test changes

The two pinned tests are replaced, not deleted, each becoming a default-behaviour case plus an allow_markup case, with the justification in the comment:

• :986 → "Slack escapes markup in text by default" ("<https://x/|y> pinged <@U1>") + "allow_markup keeps links and single-user mentions intact".
• :1087 → "Slack strips masked link labels inside blocks by default" ("<https://x/> <@U1>") + an allow_markup twin, commented that a blocks leaf cannot be entity-escaped because the same walk visits url leaves.

The four date-token tests that use text add allow_markup: true under one shared comment — a date token is markup, and nothing about the (?!!) exemption itself changed. The date-token-in-blocks test needs no flag and is the load-bearing assertion for (?!!): blocks is delabeled by default, so the delabeler is live there. An explicit expect(...).toContain("|Nov 14") is added so the exemption cannot regress silently behind the whole-leaf toBe.

"Slack neutralizes channel-wide broadcasts by default" now asserts <!channel> rather than <!channel> — in text the total escape subsumes the narrow <! one, so the closing bracket is escaped too. The broadcast is just as dead; the narrow form is still what blocks and allow_markup produce and is asserted as such elsewhere. Commented in place.

New cases: the literal M3 repro escaped by default in text; the same string delabeled inside a blocks leaf; escapeSlackText escapes & before </> ("a & b <c>" → "a & b <c>", no &); a blocks url leaf containing ?a=1&b=2 is untouched — the regression the whole split exists to avoid; allow_mentions: true still implies markup; allow_markup does not re-enable broadcasts; and the escape/delabel ordering case above.

README.md: allow_mentions rewritten (it governs text, blocks, username, icon_emoji and implies allow_markup), allow_markup added, the Features bullet split into a broadcasts half and a markup half, and the residual stated explicitly — with allow_markup: true a masked link in text is live again, and blocks labels are stripped rather than escaped because the walk cannot tell a text leaf from a url leaf.

Lower-churn alternative, if default-on escaping of text is rejected

Apply stripSlackLinkLabels uniformly to text and blocks and skip escapeSlackText entirely. Date tokens then keep working with no test churn at all, and masked links still die — but injected <@U123> pings and bare <https://evil.example> links survive in text as well. The default-on version is what is implemented here; say the word and it is a small edit.

Verification

$ bun scripts/test.ts task vitest
Running all tests in sections [task] — 67 file(s)
 Test Files  67 passed (67)
      Tests  1093 passed | 24 skipped (1117)

(baseline on this branch's merge-base: 1084 passed | 24 skipped — +9 net, after the two pinned tests were each split in two)

$ npx eslint packages/tasks/src packages/test/src/test/task
(no output, exit 0)
$ npx prettier "packages/tasks/src/**/*.ts" "packages/test/src/test/task/*.ts" "packages/tasks/README.md" --check
All matched files use Prettier code style!

There is no root lint script; format is eslint --fix && prettier --check --write, run here in check-only form.

---

Generated by Claude Code