Notify tasks: merge main, real-transport tests, Slack structural broadcasts, webhook redaction - #744
Draft
sroussey wants to merge 34 commits into
Draft
Notify tasks: merge main, real-transport tests, Slack structural broadcasts, webhook redaction#744sroussey wants to merge 34 commits into
sroussey wants to merge 34 commits into
Conversation
Adds three side-effecting notification tasks to @workglow/tasks, following
the FetchUrlTask pattern: WebhookNotifyTask (generic JSON HTTP POST),
SlackNotifyTask (incoming webhook), and DiscordNotifyTask.
All three run inline through the SSRF-aware safeFetch wrapper and share a
single POST helper (util/WebhookPost.ts) covering typed error mapping,
Retry-After parsing, abort/timeout signals, and the private-network
entitlement plumbing. They declare cachePolicy { kind: "none" } since they
are side-effecting.
A Slack/Discord webhook URL is itself the credential — the token lives in
the URL path — so the URL is kept out of the output schema and every error
message, task output, and error field is redacted to the endpoint origin.
Claude-Session: https://claude.ai/code/session_01PwyJuFrJnibKvrrk8Fa4Fn
… credential resolveWebhookUrl prefers the resolved credential over the `url` port, but webhookPrivateEntitlements classified `url` alone — so a public decoy URL plus a credential holding an internal address graded as needing no network:private, while postWebhookJson then self-granted allowPrivate from the URL it actually used. A configured credential_key now forces the fail-closed branch regardless of `url`. Also cancel the unread success body on the 204/no-read path so Slack's `ok` response does not hold a pooled connection open. Claude-Session: https://claude.ai/code/session_01PwyJuFrJnibKvrrk8Fa4Fn
…port Move WebhookNotifyTask, SlackNotifyTask, and DiscordNotifyTask out of the catch-all "Utility" category into a dedicated "Notification" category. Rename their `credential_key` input port to `url_credential_key`. On these tasks the resolved credential is the entire webhook URL — the secret itself, since a Slack/Discord webhook token lives in the URL path — and it takes precedence over the `url` port. FetchUrlTask's identically-named port instead resolves to a bearer token layered onto a public URL, so sharing the name `credential_key` across both was misleading. FetchUrlTask is unchanged. The port keeps its `format: "credential"` and `x-ui-hidden` annotations, and the fail-closed `network:private` entitlement behavior is unaffected: a configured credential still forces the entitlement because the destination is unknowable at evaluation time. Claude-Session: https://claude.ai/code/session_01PwyJuFrJnibKvrrk8Fa4Fn
…#714) Two HIGH findings in the webhook/Slack/Discord notification tasks, plus five hardening fixes. Credential leak via `error.stack`. `BaseError` never overrides `stack`, so V8 bakes the original message into it. `toRedactedWebhookError` rewrote `.message` and `.url` but then copied `.stack` verbatim, re-importing the full webhook URL — and stacks are persisted (`formatErrorChainForDiagnostics` walks the cause chain into the stored job error), so "logs are trusted" was never an available defence. `redactedStackFrom` now rebuilds the stack from the rewritten header plus the original frames (split at the first ` at ` frame, not the first line, since a message may contain newlines) and runs the whole thing through a second redaction pass. It fails closed to a header-only stack on runtimes with no ` at ` frames. `error.cause` is deliberately not copied. Mention injection. `content`/`text` was forwarded verbatim with no mention controls, and `additionalProperties: false` meant a caller could not supply them either — so piping a fetch result or a model summary into a notification pinged a whole server on every run. Discord now defaults to `allowed_mentions: { parse: [] }`. Slack has no equivalent, so the literal `<!` is escaped to `<!` (defusing `<!channel>`, `<!here>`, `<!everyone>`, `<!subteam^ID>` while preserving `<https://…|label>` links and `<@u123>` mentions) and `link_names: false` is sent explicitly; `blocks` is caller-authored and is not rewritten. Both tasks gain an opt-in `allow_mentions` port. Also: - WebhookNotifyTask no longer echoes a private destination's response body: reachability parity with FetchUrlTask is kept, but the `response` port was a working SSRF read primitive against e.g. 169.254.169.254. - Response bodies stream with a 1MB ceiling (`SECURITY_LIMITS .webhookMaxResponseBodyBytes`) instead of buffering unbounded via `response.text()` — the failure path buffered unconditionally. - A fetch rejection named `AbortError`/`TimeoutError` is classified as an abort (or a timeout) instead of falling through to a retryable `FETCH_NETWORK_ERROR`, so a cancelled workflow no longer looks transient. - Slack/Discord gain `timeout` ports and all three default to 30s, so an endpoint that completes the handshake and never answers cannot hold a slot forever. - A resolved webhook credential that is not an absolute http(s) URL fails with a configuration error naming the likely mistake, never echoing the value. - A configured `url_credential_key` upgrades the `credential` entitlement from `optional: true` (which `evaluatePolicy` skips outright) to enforced. - `success` output descriptions now say "Always true; a non-2xx response throws"; `url` descriptions note the value is stored in the graph JSON. - README: 429/503 raise `RetryableJobError` but nothing retries them — these tasks run inline and task-graph has no retry consumer. Co-authored-by: Claude <noreply@anthropic.com>
A notification POST carries the payload plus any caller headers, and for
Slack/Discord the URL itself is the credential. `safeFetch` was called with
no `redirect` option, so the default `"follow"` re-issued that exact request
at every `Location` — up to 20 hops, no same-origin check, no 303 method
downgrade, each hop re-classifying as PUBLIC. One `302` from a partner
endpoint hands the payload (and an `Authorization` header, if configured) to
another origin while the task reports `success: true`. Pass
`redirect: "error"` and fail closed: a 3xx now raises a permanent
`INVALID_URL` telling the operator to configure the final URL, with the
endpoint redacted to its origin as everywhere else. The `Location` value is
never read, so it cannot reach the message.
Slack and Discord set `includeBodyInError: true` unconditionally, splicing up
to 256 chars of the endpoint's reply into the thrown error — which
`WebhookNotifyTask` already suppresses for private destinations precisely so
the task cannot serve as an SSRF read primitive. Entitlement enforcement is
opt-in and `postWebhookJson` self-grants `allowPrivate` from the URL it uses,
so `slackNotify({url: "http://127.0.0.1:9200/_search", ...})` returned that
service's detailed error. Gate it at the choke point that already computes
`isPrivate` rather than in the two task files: `readSuccessBody` and
`includeBodyInError` are now a ceiling, forced to `false` for a private
destination. The status still reports; only the body is withheld.
Also:
- serialize the payload and build the header map before the request `try`. A
circular or `BigInt` payload was reaching the catch, which labels anything
unrecognized `NETWORK_ERROR` — retryable — so a permanent caller mistake
was retried forever under a misleading message. It is now a permanent
`CONFIGURATION` error.
- give the three `timeout` ports `minimum: 1`. `timeout: 0` armed no
`AbortSignal.timeout` at all, letting a black-holed endpoint pin the task.
- flush the decoder on the capped body read, which dropped a trailing
multi-byte character.
The README's "Direct task usage" snippets passed task INPUTS as the
constructor's first argument, which is the CONFIG — `additionalProperties:
false`, so every one of them threw `TaskConfigurationError` before any
request. Rewritten to the exported helpers, with one `defaults` example
showing the config-vs-input distinction and a test asserting each documented
form actually runs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K6huUY7hSkRbjun1P9HKsz
…ening-p4d8qo fix(tasks): refuse webhook redirects and withhold private failure bodies
…ge shape PR #724 made `postWebhookJson` pass `redirect: "error"` so a secret-bearing webhook POST is never re-sent to another origin. Detection of that refusal was shape-based: `error.name === "TypeError" && /redirect/i.test(error.message)`, because both transports threw a bare `TypeError` at the first 3xx. That fails open. Reword the transport's message and the regex stops matching, the refusal falls through to the generic catch, and it is relabelled NETWORK_ERROR — a member of FETCH_URL_RETRYABLE_ERROR_CODES. The refused redirect silently becomes a retried one, with no test failing. Give the refusal a real discriminant instead. `FetchUrlErrorCode` is the established error convention on this path — every other SafeFetch refusal already throws through it — so add REDIRECT_NOT_FOLLOWED there rather than a parallel error class, plus a single `createSafeFetchRedirectError` factory both transports call and an exported `isSafeFetchRedirectError` guard consumers match on. The code is non-retryable by construction, survives job-queue persistence, and carries the requested URL and 3xx status; the `Location` is still never read, so it cannot reach a message or a stack. Behavior is unchanged: a refused webhook redirect stays a permanent INVALID_URL-class error, endpoint reduced to its origin, Location absent. Only the detection changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K6huUY7hSkRbjun1P9HKsz
…entinel-p4d8qo fix(tasks): detect safeFetch redirect refusals by sentinel, not message shape
`redactWebhookUrlIn` only matched the literal full URL, but endpoints routinely echo just a fragment of it: an Express 404 answers with the path and no origin, and a validation error may quote the token alone. Slack and Discord both set `includeBodyInError`, so such a body reached the error message, the stack and the persisted diagnostics verbatim. The path, the query and every individual path segment are now redacted too, applied longest-first so a short segment cannot break a longer candidate containing it. A candidate is admitted only when it clears `SECURITY_LIMITS.webhookMinRedactableSegmentChars` and is not an all-lowercase word — `services` and `webhooks` are real segments of the Slack and Discord paths, and redacting them would corrupt ordinary prose. Also adds `slackBlocksMaxDepth`, so `limits.ts` is touched once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013oVdDSRMJeALBPLDQf3DgH
`text` was escaped but `blocks` was passed through verbatim, so a `<!channel>` written into a section, a `fields[]` entry or an `elements[]` entry still notified the whole workspace — and `blocks` is as reachable from a pipe or a model as `text` is, which left the neutering one field away from being bypassed. `neutralizeSlackBroadcastsDeep` walks the structure and routes every string leaf through the same escape as `text`, so there is one rule rather than two. Escaping all leaves (not just rendered body text) is side-effect-free — `<!` has no legitimate use in a `type`, `block_id`, `action_id` or URL field — and covers block shapes without enumerating them. Both ports are gated on `allow_mentions`. Input structures are rebuilt, never mutated, and depth is capped by `SECURITY_LIMITS.slackBlocksMaxDepth`, which terminates a cycle with the same permanent configuration error serialization would raise anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013oVdDSRMJeALBPLDQf3DgH
The `response.ok` branch truncated the body straight into the task output port. Echo endpoints (webhook.site, RequestBin) reply 200 with the request line, which carries the whole webhook URL — the credential — into persisted, pipeable output. The failure path's helper now runs on the success body too, and runs BEFORE truncation: cutting first can slice a token in half so it no longer matches, leaving a usable prefix behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013oVdDSRMJeALBPLDQf3DgH
A `url_credential_key` the store could not answer fell through to the `url` port and posted anyway, reporting success — so a locked store or a mistyped key silently sent the notification to a different endpoint, with nothing anywhere saying the configured credential was never used. `resolveWebhookUrl` now takes whether the key was configured at all. The resolver overwrites the port in place, so `execute` cannot see the raw key; a store miss leaves the port present with value `undefined` while an unconfigured port is absent, and `Object.hasOwn` is that discriminator. The message names the port, never a key or a value, and is raised as a permanent configuration error so the job layer does not retry it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013oVdDSRMJeALBPLDQf3DgH
…g them BREAKING CHANGE: posting to a loopback/RFC1918 destination now requires the new `allow_private_destination` input on WebhookNotifyTask, SlackNotifyTask and DiscordNotifyTask. Without it the post fails with PRIVATE_DENIED before any request is made. The destination is not knowable at entitlement-evaluation time: `ITask.entitlements()` is synchronous and the enforcer checks it before the runner resolves `format: "credential"` inputs. Deriving the requirement from the URL therefore either failed open — grading a decoy public `url` while the request went wherever the credential pointed — or forced an unscoped `network:private` grant on every credential-using instance, public destination or not. And `postWebhookJson` then self-granted `allowPrivate` from whatever URL it had ended up with, so an arbitrary credential value chose its own reachability. The decision is now an explicit declared input, enforced at execute time against the URL actually resolved. The entitlement is declared only when the flag is set, scoped to the `url` port when that port is the destination and unscoped (with a reason naming the credential store) when it is not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013oVdDSRMJeALBPLDQf3DgH
…9x9lby-notify-secrets fix(tasks): close five secret-disclosure and gating holes in the notify tasks
… a private destination
`allow_private_destination` is an input, and a graph ROOT task's run-input is
applied by `TaskRunner.run()` — strictly after `TaskGraphRunner` has already
graded `task.entitlements()`. So a run-input carrying
`{ url: "http://169.254.169.254/...", allow_private_destination: true }` reached
`execute()` with a declaration the enforcer never saw, and the post went out
under a policy that denies `network:private`. The same window is what makes a
credential-resolved URL ungradeable at declaration time: the enforcer runs
before the credential resolver.
`postWebhookJson` now re-checks the `network:private` grant at execute time
against the URL actually resolved, via the registry the task is executing under
(`context.registry`). No enforcer registered means no policy to satisfy and the
post proceeds unchanged; a public destination is never checked.
Also tightens `webhookPrivateEntitlements` so only an explicit `false` opts out
of declaring `network:private`. The branch is unreachable while the input
schemas keep `default: false` (which is retained — every ordinary Slack/Discord
notification would otherwise demand the grant), but it means a later removal of
that default fails closed rather than open.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDvMMv78PuEw4T5atLeJeS
…anded to safeFetch The transport enforces against these two arguments, so the outcome assertions elsewhere only imply them. Pins that a public destination is fetched with `allowPrivate: false` and no scopes, that a declared private one is scoped to its own origin, and — the DNS-rebinding invariant — that `allow_private_destination` never widens the transport for a public hostname. All three notify tasks are covered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDvMMv78PuEw4T5atLeJeS
…gzrguj-notify-ssrf fix(tasks): enforce the network:private grant at execute time for webhook notify tasks
Picks up the fix for the unhandled rejection raised when a webhook success
body is cancelled unread, which landed on main after this branch was cut.
Two adjacent-insertion conflicts, both resolved by keeping both sides:
- packages/tasks/src/common.ts: DiscordNotifyTask and FetchUrlCredentials
exports inserted at the same point in the alphabetised list.
- packages/test/src/test/task/FetchUrlSsrf.test.ts: node: builtin imports
and the getTestingLogger import.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H797qbH356jjznKgUax63o
Every case in NotifyTask.test.ts runs against a process-global safeFetch mock installed in beforeAll, so none of them exercise the undici request, the passthrough TransformStream, or the dispatcher lifecycle. That matters because postWebhookJson cancels the success body unread for both Slack and Discord: cancelling the returned readable errors the writable behind it, pipeTo rejects, and an unhandled rejection terminates the process under Node's default --unhandled-rejections=throw. A mocked Response has no pipe behind it. Separate file rather than a block in NotifyTask.test.ts, so a real-transport case cannot be poisoned by that file's global mock depending on describe order. Carries the getSafeFetchImpl().name tripwire — without it the file is vacuous. Covers both cancel sites: the success-body cancel, and readBodyText abandoning an oversized failure body at webhookMaxResponseBodyBytes, which nothing else exercises against the real transport. Plus dispatcher release on the cancel path and the bodiless-204 branch that takes no TransformStream at all. Verified against the pre-merge transport: the three cancel-path cases fail (unhandled rejection observed), the tripwire and the 204 guard pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H797qbH356jjznKgUax63o
The Block Kit escape pass rewrites every string leaf, on the reasoning that `<!` is a broadcast sigil wherever Slack finds it. That is true of message text and false of rich_text: an @channel ping there is an element SHAPE, `{type: "broadcast", range: "channel"}`, with no `<!` anywhere in it. A group ping is `{type: "usergroup", usergroup_id: "S…"}`. Both survived the escape untouched, so caller-supplied or model-generated blocks could still notify a whole workspace with `allow_mentions` unset. Adds a structural pass ahead of the key-copy loop that rewrites such an element to a plain text node. Rewrite rather than delete: dropping the element can leave an `elements[]` empty, which Slack rejects — turning a security control into an availability bug. `link_names: false` is already sent whenever mentions are disallowed, so the literal `@channel` text cannot auto-link. Matching on `type` alone rather than rich_text ancestry is deliberate: the only false positive is a caller who wanted a live usergroup ping, which is exactly what the control exists to stop. The completeness claim was asserted in three places, all of which said the lexical escape covered everything; each now states both halves and the residual (a new structural element type Slack adds later is uncovered until listed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H797qbH356jjznKgUax63o
The webhook URL is the credential, and redactWebhookUrlIn is what keeps it out of a diagnostic that quotes an endpoint's reply. Two shapes escaped it. Query values were never candidates. Only `pathname+search`, `search` and the individual path segments were admitted, so a `?token=SUPERSECRET1` endpoint that answers `bad token SUPERSECRET1` matched nothing — the whole `?token=…` pair was a candidate but the echo quotes the value alone. `?token=` auth is an ordinary deployment shape, not a corner case. Query values are now admitted in both raw and decoded form; the raw query is split by hand rather than read from searchParams, whose decoder turns `+` into a space and so would not match the bytes an endpoint echoes. All-lowercase candidates were exempted outright, on the theory that such a run is a word rather than a token. A lowercase token is still a token. The exemption existed to protect `services` and `webhooks`, the routing segments of the two supported providers' paths, which are now named explicitly instead. Accepted cost, stated in the JSDoc and pinned by a test: a generic webhook whose path carries a long lowercase word has that word redacted from echoed diagnostics. Also folds in `response.statusText`, which was interpolated into the message and stored as `httpStatusText` with no redaction at all, and unlike the body was not withheld for a private destination — leaving the SSRF read open through a narrower channel, since a server can put anything in a reason phrase. The length floor is unchanged and now applies to query values too, so a short `?t=abc` still leaks. Same policy as segments, said out loud rather than silently special-cased. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H797qbH356jjznKgUax63o
sroussey
changed the title
This was referenced Aug 13, 2026
…d a job (#758) A `Retry-After` header (or Discord's JSON `retry_after`) is remote-controlled input, and both the webhook post path and FetchUrlTask turned it straight into a Date with no ceiling. `Retry-After: 1e20` overflows the maximum representable timestamp into an Invalid Date, and `Fri, 01 Jan 9999 00:00:00 GMT` parses perfectly well into a date that parks the job for millennia. The Invalid Date is the worse half: it passes `instanceof Date` in JobQueueWorker.rescheduleJob, its NaN reaches `delaySeconds`, and `new Date(NaN).toISOString()` throws a RangeError that rescheduleJob's own catch swallows — no retry is scheduled, the claim is dropped, and the job is stranded until lease expiry. Adds SECURITY_LIMITS.httpRetryAfterMaxSeconds (24h) and a single RetryAfter.ts funnel that every parse routes through, and makes the worker reject an invalid date defensively rather than clamp it: the parse site owns the policy, the worker only refuses the impossible. Claude-Session: https://claude.ai/code/session_01RomTUtZSTgUbFCYqFs4pcu Co-authored-by: Claude <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| limiter, | ||
| }); | ||
|
|
||
| const id = await storage.add({ |
…lack escape gap (#761) Four independent fixes in the webhook notification path. - The generic branch of toRedactedWebhookError built its detail from error.message alone, so every undici connection failure reported the same unactionable "fetch failed". One level of cause.message is now appended, redacted before truncation. The cause OBJECT is still never attached: formatErrorChainForDiagnostics persists every link's message and stack and would re-import the unredacted URL. - leaksUrl gated the redaction rewrite on message and url only, so a typed error whose stack embeds the webhook URL was returned untouched. Stacks are persisted, so this leaked the token. - readBodyText returned a mid-stream failure exactly as a complete read, and the fragment was surfaced as the task's response under success: true. It now reports completeness and the caller marks a short read. Not a throw: the POST already returned 2xx, so throwing would repost the message on retry. - The structural broadcast rewrite built its replacement text from the caller-controlled range and was the one string leaf in the traversal that skipped the lexical escape. Claude-Session: https://claude.ai/code/session_01RomTUtZSTgUbFCYqFs4pcu Co-authored-by: Claude <noreply@anthropic.com>
Coverage Report
File CoverageNo changed files found. |
…Error or fire after 1 ms `AbortSignal.timeout` validates its delay as a uint32 integer, but the `timeout` port was declared `type: "number", minimum: 1` with no maximum in all three notify tasks. A fractional value therefore passed schema validation and reached the timer, which threw a bare `RangeError` — not a `FetchUrlJobError` — so a queued consumer could not classify it and would retry a permanent configuration mistake forever. A value above the signed 32-bit range also passed validation and was silently clamped to 1 ms with a `TimeoutOverflowWarning`, so "effectively never time out" aborted instantly and the failure was reported against the endpoint. - declare the port `type: "integer"` with `maximum: 2147483647` in all three tasks, sharing one exported literal with the runtime guard so the two cannot drift - validate in `postWebhookJson` before the signal is armed, raising a classifiable `FetchUrlErrorCode.CONFIGURATION` error naming the bound The old `> 0` test silently meant "wait forever", contradicting the port's own documented behavior; `undefined` still means no timer and is only reachable from a direct `postWebhookJson` caller. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XKvKnyVeQQQCm6FhtaLyMa
…clares BREAKING CHANGE: `allow_private_destination` now widens the transport for any destination it is set on, and any such destination's reply body and reason phrase are withheld. `allowPrivate` was derived from `classifyUrl`, which is string-only by construction and never resolves DNS. A hostname that is no literal IP and matches no reserved suffix classifies public, so the flag was inert for exactly the case it names: under split-horizon DNS a `hooks.mycorp.com` resolving to 10.1.2.3 was refused at connect time with a message asking for the `network:private` grant the operator already held, and no configuration short of hard-coding the address made the post work. The declaration now governs the transport and the GRANT still authorizes it: `assertPrivateDestinationGranted` runs for every declared private destination rather than only for a URL that reads private, so the set of requests receiving the widened transport is a superset of the set that is entitlement-checked — where previously the public-looking case was checked at all. The resource pattern is computed once and used both for the grant check and for `privateResourceScopes`, so the pattern graded and the scope enforced cannot diverge. Redirects stay refused, so a `Location` cannot pivot off the granted origin. Body, failure-body and reason-phrase suppression move to the declaration for the same reason: the URL alone cannot say whether the host was internal, so a caller who declared it may be private never gets its reply back. `WebhookNotifyTask` no longer classifies the URL a second time — that duplicate was the drift — and passes `readSuccessBody: true` as the ceiling it is, leaving `postWebhookJson` the single decider. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XKvKnyVeQQQCm6FhtaLyMa
…hxoj5s-notify-timeout fix(tasks): bound the webhook timeout port so it cannot throw a RangeError or fire after 1 ms
…hxoj5s-notify-private-destination fix(tasks)!: let allow_private_destination govern the transport it declares
`neutralizeSlackBroadcasts` escaped every `<!` occurrence, but `<!` is
Slack's control-sequence sigil rather than a broadcast sigil: the documented
date-formatting token has the same shape
(`<!date^1700000000^{date_short}|Nov 14>`) and can notify nobody.
Escaping it is pure collateral damage. Slack un-escapes the entity for
display, so a message reading "Deploy at <!date^…>" showed the raw token
instead of a localized date, and the only opt-out — `allow_mentions: true` —
simultaneously disables the structural broadcast/usergroup rewrite and drops
`link_names: false`, so recovering a date meant accepting live channel-wide
pings in the same field.
The escape now skips a `<!` followed by `date^`. The exemption is an exact
lowercase prefix matched at the `<!` itself, so a `<!channel>` inside a date
token's fallback text is still escaped and a case variant (`<!DATE^`) is
escaped too — whether Slack accepts one is unverified, so it fails closed.
`neutralizeSlackBroadcastsDeep` calls this function, so the exemption reaches
every string leaf of `blocks` without a second copy of the rule.
No new port, and `allow_mentions` keeps its default of `false`: with the date
token exempt, everything the escape still removes is a mention, so the one
control governs one concern and splitting it would add a port with no
behavior behind it. That rationale is recorded in the JSDoc.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XKvKnyVeQQQCm6FhtaLyMa
…rg-hxoj5s-slack-date-token
…hxoj5s-slack-date-token fix(tasks): exempt Slack's date token from the broadcast escape
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #678, and consolidates #745 and #747 (both now closed — their content ships here). Four pieces, each with its own rationale below.
origin/mainis merged in as of67bed681.1. Merge
main— picks up the unhandled-rejection fixWhat
The webhook notification tasks cancel their success body unread —
postWebhookJsonpassesreadSuccessBody: falsefor both Slack and Discord, and Slack answers200with a shortokbody every single time. On the server transport, cancelling the returned readable errors the writable behind it, sobody.pipeTo(writable)rejects with the cancel reason (undefinedfor a barecancel()). #678's branch has no.catchon that promise, so under Node's default--unhandled-rejections=throwevery successful Slack post terminates the process.That fix already landed on
mainin241c810d, along withSafeFetchServerTransport.test.ts. #678 predated it (17 ahead / 105 behind at the time). So this branch does not re-patchSafeFetch.server.ts— it mergesmainand adds the coveragemain's file does not have.Why merge, not rebase
#678's tip is itself a merge commit (
Merge pull request #736); replaying 14 commits buys nothing and rewrites shared history. This was re-tested when the branch was brought up to currentmain:git rebase origin/mainre-hits, on commit 1 of 14, the verypackages/tasks/src/common.tsconflict the merge had already resolved, and would do so again for each subsequent commit that touches the same files — resolving each against an intermediate tree that never existed. The rebase was aborted and a merge used instead.The first merge produced exactly two conflicts, both trivial adjacent insertions into the same line of an alphabetised list, both resolved by keeping both sides:
packages/tasks/src/common.ts— branch'sDiscordNotifyTaskexport vs main'sFetchUrlCredentialsexport.packages/test/src/test/task/FetchUrlSsrf.test.ts— branch'snode:builtin imports vs main'sgetTestingLoggerimport.SafeFetch.server.tsmerged clean, and the merged file was verified to carry.catch(() => {})withpackages/tasks/src/node.tsstill side-effect-importing it. The second merge (to67bed681) was conflict-free.2. Real-transport tests for the notify tasks
What / why
Every case in
NotifyTask.test.tsruns against a process-globalsafeFetchmock installed viaregisterSafeFetchinbeforeAll. So none of them exercise the undici request, the passthrough TransformStream, or the dispatcher lifecycle — a mockedResponsehas no pipe behind it and can never see the rejection above.New
packages/test/src/test/task/NotifyTaskTransport.test.tstalks to a throwawaynode:httpserver on127.0.0.1:0and never mocks the transport. It is a separate file, not a block insideNotifyTask.test.ts: a real-transport block sharing that file's global mock would silently depend on vitest describe-ordering not to poison it. It carries thegetSafeFetchImpl().name === "serverSafeFetch"tripwire — without it the whole file is vacuous, since a leaked mock would make every case pass against nothing.serverSafeFetch200+okbody →{success:true,status:200}, no unhandled rejection — the production crash path200, thenwaitForNoConnections === 0— dispatcher released on the cancel path400with a >1 MB never-ended body →readBodyTextcancels atSECURITY_LIMITS.webhookMaxResponseBodyBytes; assertsPermanentJobError,httpStatus: 400, no token in the message204, no body → the non-TransformStream branchThe oversized-failure-body case is the second cancel site, which nothing anywhere exercises against the real transport, including main's new file.
Loopback is private address space, so every case sets
allow_private_destination: true; no entitlement enforcer is registered, soassertPrivateDestinationGrantedhas no policy to satisfy — the arrangement existing tests already rely on.3. Slack's structural Block Kit broadcasts (was #745)
What
SlackNotifyTaskneutralized channel-wide broadcasts by escaping the literal<!in every string leaf ofblocks, on the stated reasoning that<!is a broadcast sigil wherever Slack finds it and that walking to every leaf is therefore complete.True of message text, false of
rich_text. There,@channelis an element shape —{"type": "broadcast", "range": "channel"}— with no<!anywhere in it, and a group ping is{"type": "usergroup", "usergroup_id": "S…"}. Both passed through untouched. Ablockspayload assembled from a fetch result or a model completion could still notify an entire workspace withallow_mentionsunset. Confirmed against the pre-fix code: the body went out as[{"type":"broadcast","range":"channel"}], verbatim.The completeness claim was asserted in three places — the
neutralizeSlackBroadcastsDeepJSDoc, theblocksport description, andpackages/tasks/README.md— all wrong the same way.Why this fix
A structural pass ahead of the key-copy loop rewrites a recognized element to a plain text node.
Rewrite rather than delete. Dropping the element can leave an
elements[]empty, and Slack rejects that — turning a security control into an availability bug, a bad trade on a notification path. The replacement is literal text (@channel), andlink_names: falseis already sent whenever mentions are disallowed, so it cannot auto-link.Match on
typealone, notrich_textancestry. The only false positive is a caller who deliberately wanted a live usergroup ping — exactly what the control is for. Requiring ancestry would instead miss any future context where Slack accepts the same element.Residual, stated not papered over: unlike the lexical half this cannot be shape-agnostic — a structural broadcast is identified by nothing but its type name — so a new such type is uncovered until added to the set. All four docs now say so.
Tests (in
mention neutering){type:"broadcast",range:"channel"}→ no"type":"broadcast", no"range":"channel", contains@channel{type:"usergroup",usergroup_id:"S12345678"}→ neither field survives, contains@usergroupallow_mentions: true→ still verbatim{type:"section",…}untouchedThe two that pass on both sides are deliberate. The
allow_mentionscase currently passes for the wrong reason — nothing rewrites the element at all — so asserting it pins the gate rather than documenting it.4. Webhook query-value and reason-phrase redaction (was #747)
What
For these tasks the webhook URL is the credential, and
redactWebhookUrlInkeeps it out of diagnostics quoting the endpoint's reply. It reaches the caller because Slack and Discord both setincludeBodyInError: true. Two shapes walked through it.Query values were never candidates. Candidates came only from
pathname+search,search, and path segments. An endpoint authenticating with?token=SUPERSECRET1that answersbad token SUPERSECRET1matched nothing — the whole pair was a candidate, but the echo quotes the value alone.?token=auth is an ordinary deployment shape.All-lowercase candidates were exempted outright, on the theory that such a run is a word rather than a token. So
/hooks/supersecrettokenechoed asrejected: supersecrettokenwent out verbatim, despite being 16 characters and clearly a secret.Rider:
response.statusTextwas interpolated into the message and stored ashttpStatusTextwith no redaction, and unlike the failure body was not withheld for a private destination — leaving the SSRF read the body suppression exists to prevent open through a narrower channel, since a server puts whatever it likes in a reason phrase.Why this fix
Admission is now by length only. The
services/webhooksexemption the lowercase rule was really protecting is expressed directly as a named set of the providers' routing segments — what it always meant, in terms that do not also exempt every lowercase secret.Query values are admitted raw and decoded, and the raw query is split by hand rather than read from
searchParams: that decoder turns+into a space, so a decoded value would not match the bytes an endpoint echoes back.The 8-char floor is unchanged and now applies to query values too, so a short
?t=abcstill leaks. Same policy segments already had; the JSDoc now says so rather than leaving it to be rediscovered.Not folded in: the
REDIRECT_NOT_FOLLOWEDdiagnostic (same file, different function). It needs three baked-in assertions flipped and is a diagnostics-quality issue, not a leak. Deliberately deferred.Tests (in
secret redaction/private destination failure bodies)?token=SUPERSECRET1+ 403 body echoing it → absent from.messageand.stack/hooks/supersecrettoken+ 403 body echoing it → absent/notifications/deploy→notificationsis redacted (the accepted cost, pinned)token SECRETTOKEN bad→ absent from message andhttpStatusTextindex=cluster-secrets shard=3→ message has400, notcluster-secrets;httpStatusTextundefinedinvalid webhooks payload→webhookssurvivesThe last is the pre-existing over-redaction guard; its comment previously justified the exemption as "all-lowercase runs are words, not tokens" and now names the real reason, pinning the replacement for the deleted rule.
Consolidation
claude/notify-slack-structural-broadcastandclaude/notify-redaction-query-and-statuswere merged into this branch at9926f8ed. No conflict occurred — the two branches add to differentdescribeblocks ofNotifyTask.test.ts(mention neuteringvssecret redaction) and touch disjoint source files (SlackNotifyTask.ts+ README vsWebhookPost.ts), soortauto-merged. Both sets were verified present afterwards by grep and by a full suite run.Tests actually executed
bun install --frozen-lockfile+bun run use-sourcesucceeded, so the suite runs.packages/test/src/test/task/directory: 67 files, 1059 passed, 24 skipped.SafeFetch.server.ts(temporarily restored from46309a25): 3 failed / 2 passed, the three cancel-path cases each reporting[undefined]from theunhandledRejectioncollector.prettier --checkclean on every changed file.Risk / blast radius
Three behaviour changes a reviewer must accept:
rich_textusergroup ping must now setallow_mentions: true./notifications/deploy) has that word redacted out of echoed diagnostics. Stated cost of dropping the exemption; pinned by its own test.httpStatusTextis nowundefinedfor a private destination, where it previously carried the reason phrase.The merge pulls
maininto this PR, so CI re-runs in full. No pre-existing test asserted onstatusText(verified by the full-directory run).Reviewer note, not a work item: post-merge
packages/taskscarries two credential idioms — main'sFetchUrlCredentials.ts(public URL + separate bearer token) and this branch'sresolveWebhookUrl(the URL is the secret). They do not collide. Unifying them is a separate conversation.Unverified
use-sourcemode (dist re-export stubs), not against built bundles.rich_textbroadcast element arriving through an incoming webhook was not confirmed live. Slack documents the element forchat.postMessage; incoming webhooks share the Block Kit payload format. The fix and the doc correction are justified either way — the docs asserted coverage that demonstrably did not exist.tsc -p packages/tasksreports errors in this checkout, but the count is identical with and without the changes — unbuilt.d.tsartifacts ofuse-sourcemode, not a regression.httpStatusTextbeing present for a private destination was not audited outside this repo.