fix(tasks): stop notify tasks leaking webhook tokens and mass-pinging

[sroussey]

Security follow-up on the webhook / Slack / Discord notification tasks added in the base branch. Two HIGH findings plus five hardening fixes.

(Angle brackets below are written as HTML entities — an earlier revision of this description had them eaten by the API.)

HIGH — credential disclosure via error.stack

BaseError calls Error.captureStackTrace and never overrides stack, so V8 materializes it as name: message followed by newline-separated at … frames — the message is baked into the string. SafeFetch / SafeFetch.server interpolate the raw URL into seven message templates, and toRedactedWebhookError rewrote .message and .url but then did rebuilt.stack = error.stack, re-importing the very URL the surrounding function had just stripped.

Concrete scenario: a Slack webhook whose host resolves into RFC1918 space throws PRIVATE_DENIED. .message correctly reads https://hooks.slack.com, but .stack line 1 contains .../services/T00/B00/SECRETTOKEN. This is not a "logs are trusted" problem — formatErrorChainForDiagnostics walks the error and its .cause chain and pushes every .message + .stack into a persisted job-error string.

Fix — redactedStackFrom(original, rebuilt, url) in WebhookPost.ts:

1. splits the original stack and discards everything before the first frame matching /^\s+at / (the header can span several lines when a message contains newlines, so the split point is the first frame, not the first line);
2. re-prefixes the rebuilt error's own name: message;
3. runs the whole result through redactWebhookUrlIn as a second pass, covering frames that embed the URL independently;
4. fails closed on runtimes whose stacks carry no at frames — header only, never the original string.

error.cause is deliberately not copied onto the rebuilt error, with a comment at the rebuild site so it is not "helpfully" added later.

Why here and not in SafeFetch.server.ts: pushing redaction lower means editing message templates shared with FetchUrlTask, where the URL is not a secret and is the most useful diagnostic there is — and it would still miss undici/DNS errors that carry the hostname independently. WebhookPost.ts is the only layer that knows "this URL is the credential", and a comment now says so.

HIGH — mention injection / mass-notification amplifier

content / text was forwarded verbatim with no mention controls. Because both input schemas are additionalProperties: false and Task.setInput only copies declared properties, a caller could not supply allowed_mentions even knowing they should.

Concrete scenario: a workflow pipes FetchUrlTask output or an LLM summary into content. Any @everyone in that text pings the whole server on every run, and a retry loop makes it an unmutable mass-notification amplifier.

• Discord — default allowed_mentions: { parse: [] }, which suppresses @everyone / @here, roles and users.
• Slack — Slack has no allowed_mentions, and link_names only governs auto-linking of bare @name text, so it is insufficient. Slack's documented control is HTML-entity escaping. This uses the narrow form: the literal two-character sequence <! is replaced with &lt;!, which kills all four broadcast forms (<!channel>, <!here>, <!everyone>, <!subteam^ID>) while preserving <https://…|label> links and <@u123> single-user mentions that full escaping would break. link_names: false is also sent explicitly. blocks is a caller-authored structure rather than a piped string and is not rewritten — documented as a limitation in the README.
• Both tasks gain an opt-in allow_mentions boolean (default false). When true, Discord omits allowed_mentions entirely and Slack sends text unescaped with no link_names.

Decision 3 — no response echo for private destinations

Reachability parity with FetchUrlTask is deliberate prior art and is kept. What is removed is the read primitive the response output port created: a POST to http://169.254.169.254/latest/meta-data/iam/security-credentials/ returned up to 1KB of the reply straight back into the graph. WebhookNotifyTask.execute now classifies the resolved URL and, for a private classification, passes readSuccessBody: false and returns an empty response. That costs nothing for a notification. Documented in the README Features list.

Other hardening in this PR

• Body cap. readBodyText called response.text() with no ceiling; the truncation constants applied only after the whole body was in memory, so a 500 with a multi-GB body OOMed the runner — and the failure path buffers unconditionally for all three tasks. It now streams response.body via getReader(), decodes with a streaming TextDecoder, and cancels the reader past SECURITY_LIMITS.webhookMaxResponseBodyBytes (1MB, new).
• Abort classification. fetch rejects an aborted/timed-out request with a DOMException, which previously fell through to a retryable FETCH_NETWORK_ERROR — so cancelling a workflow looked like a transient failure. AbortError now yields createFetchUrlAbortedError(); TimeoutError yields an abort error when the caller's own signal is aborted and a NETWORK_ERROR timeout otherwise.
• Timeouts. Slack and Discord hard-coded timeout: undefined and exposed no port, and WebhookNotifyTask.timeout had no default, so an endpoint that completed the handshake and never answered hung the task and held a slot forever. All three now default to 30000 ms and Slack/Discord gain timeout ports.
• Credential misconfiguration. A resolved url_credential_key that is not an absolute http(s) URL now fails with FETCH_CONFIGURATION naming the likely mistake ("…use FetchUrlTask instead") and never echoing the value. Previously a bearer token wired into that port failed with a confusing runtime INVALID_URL.
• Entitlement. The credential entitlement was declared optional: true, which evaluatePolicy skips outright — decorative. It is now upgraded to enforced by webhookPrivateEntitlements whenever url_credential_key is set. Checked for grant sets in packages/test: these three tasks have no consumers outside packages/tasks/src and their own test file, so nothing that only grants network:http breaks.
• Descriptions. success is now documented as "Always true; a non-2xx response throws" (no non-throwing mode added). url port descriptions no longer imply the value is safe to set inline — they say it is stored verbatim in the graph JSON and point at url_credential_key. No x-ui-hidden on url; it is the primary field in the builder.

Intentional behavior changes worth a reviewer's attention

1. Slack broadcast escaping is on by default. A message that today pings @channel will stop doing so unless allow_mentions: true is set. This changes the wire payload (text, plus a new link_names: false), which existing payload-equality tests were updated for.
2. Requests now time out after 30 s by default. A previously-hanging call becomes a failure. AbortSignal.timeout is armed on every request, not only when a timeout was supplied.
3. Discord payloads now always carry allowed_mentions unless opted out.

README corrections

• "429/503 and 5xx responses raise retryable errors carrying a Retry-After retry date" was wrong on the second half: these tasks run inline and task-graph has no retry consumer (grep -rE 'RetryableJobError|retryDate' packages/task-graph/src returns nothing). Replaced with "…retries require a @workglow/job-queue consumer, which these inline tasks do not have". Same correction applied to the Discord retry_after bullet — the value is parsed onto the error, not acted on.
• "never appears in errors, output, or logs" → "…in error messages, error.url, error.stack, or task output" (true only after the stack fix above).
• New bullets for allow_mentions and the default neutering (noting blocks are not rewritten), the timeout defaults, the 1MB cap, and the private-destination no-echo rule.

Tests

packages/test/src/test/task/NotifyTask.test.ts grows from 26 to 45 tests, covering: the redacted .stack (incl. formatErrorChainForDiagnostics and an undefined error.cause), Discord/Slack default neutering and the allow_mentions opt-out, link and single-user-mention survival, the armed default timeout and a real short one, caller-abort classification, the 1MB cap on both the success and failure paths (with stream-cancellation assertions), the private-destination no-echo, and the bearer-token configuration error.

The pre-existing "204 without parsing a body" test's response.text() spy would have gone vacuous under getReader(); a sibling test now asserts, on a Slack 200 that does have a body, that neither text() nor getReader() is called, that body.cancel() is, and that a double-cancel does not throw.

Verified / not verified

Verified (commands run, output seen):

• bun install — clean.
• bunx vitest run packages/test/src/test/task/NotifyTask.test.ts — 45 passed.
• Same file under bun test (JavaScriptCore) — 45 passed. The stack rebuild is heuristic across engines, so it was run under both; Bun emits at frames, so it keeps them rather than degrading, and the fail-closed branch is the documented behavior for engines that do not.
• Every new test was confirmed to fail before the fix (git stash of packages/tasks/src + packages/util/src, re-run): 12 failures plus a worker crash on the uncapped-body test, which is the OOM the cap now prevents.
• bunx vitest run packages/test/src/test/task packages/test/src/test/task-graph — 2264 passed, 1 failed: OwnTask.test.ts → "owns and disowns a pipe function" (Class extends value undefined in Conversions.ts), confirmed pre-existing on the base branch with all changes stashed.
• bun run build:types — 41/41 packages successful.
• bunx eslint + prettier on every changed file — clean.

Adapted from the plan (and why):

• The plan's T3 specified fake timers. Vitest's fake timers do not drive AbortSignal.timeout (Node implements it on an internal timer, not the patched global) — probed and confirmed aborted === false after advancing 5 s past a 1 s signal. The default is therefore asserted by spying on AbortSignal.timeout (called with 30000 for all three tasks) and the behavior by a real timeout: 50.
• The plan's T2 expected the Slack body to contain the fully escaped <!channel>, which is inconsistent with the narrow <! transform the same plan prescribes (escaping the closing bracket too is what breaks links). The test asserts the half-escaped form is present and the raw broadcast absent — the broadcast is defused either way.
• The plan's T4 rejects with an AbortError while the caller's signal is not aborted, which its own "NETWORK_ERROR when request.signal was not aborted" rule would classify as retryable. Resolved by making the DOMException name the primary discriminator (AbortError = caller abort, TimeoutError = timeout) with request.signal.aborted as the refinement — which is what those names actually mean.
• T7 supplies the bearer token through the credential store (getGlobalCredentialStore().put(...)) rather than as a literal port value: the credential resolver returns undefined for an unknown key and never echoes the key back, so a raw string would have hit the "no webhook URL provided" path instead of the new one.

Not verified: nothing in this PR was left unrun. The one failing test in the wider sweep is pre-existing and unrelated.

---

🤖 Generated with Claude Code

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	63.34%	29562 / 46670
🔵	Statements	63.2%	30637 / 48471
🔵	Functions	63.59%	5614 / 8828
🔵	Branches	52.65%	14811 / 28130

File Coverage

No changed files found.

Generated in workflow #2906 for commit f5855a1 by the Vitest Coverage Report Action