Notify tasks: merge main, real-transport tests, Slack structural broadcasts, webhook redaction - #744
Draft
sroussey wants to merge 50 commits into
Draft
Notify tasks: merge main, real-transport tests, Slack structural broadcasts, webhook redaction#744sroussey wants to merge 50 commits into
sroussey wants to merge 50 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
…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 was referenced Aug 15, 2026
…ion (#805) `BROADCAST_SIGIL` escapes only `<!`, so `<https://evil.example|Deploy succeeded>` passed 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 could display a phishing destination as a status line. Two tests pinned that behaviour as a guarantee; under this task's own threat model the guarantee IS the vulnerability. `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, where escaping `&` corrupts every query string (`?a=1&b=2` → `?a=1&b=2`). `<!` is safe to escape everywhere precisely because it has no legitimate occurrence there; `&` does. So `text` gets `escapeSlackText` (`&` FIRST, or the next two passes double-escape the ampersands they introduce) and `blocks` gets `stripSlackLinkLabels`, `<(?!!)([^<>|]*)\|[^<>]*>` → `<$1>`. `(?!!)` exempts the `<!…>` control family — the date token's `|Nov 14` is a fallback, not a masking label — and `[^<>|]` keeps a match inside one sequence. The deep walk takes an explicit `{ stripLabels }` policy at both call sites. Within a leaf the broadcast escape runs first: reversed, `<https://x/|<!channel>>` survives, since the label's inner `<` blocks the delabeler. The new `allow_markup` port defaults to FALSE. The threat model is piped and model-generated content, an opt-in control nobody sets protects nobody, every other control here fails closed, and these tasks do not exist on main — so default-on has zero released blast radius and this is the only moment the safe default is free. The escaped message stays readable; only clickability is lost. It is a separate port from `allow_mentions` because wanting a build link is orthogonal to wanting `@channel` to ping four hundred people; `allow_mentions` implies it, so the ladder has no dead rung. `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. Co-authored-by: Claude <noreply@anthropic.com>
…r credential seam (#800) An invalid request header was reported as a RETRYABLE network error. undici builds the `Headers` object inside `fetch` and rejects a malformed name or value with a bare `TypeError` — name "TypeError", no `code`, not a `FetchUrlJobError` — so `toRedactedWebhookError` matched none of its named branches and fell through to `NETWORK_ERROR`. A queued consumer retried a typo forever. undici's message also quotes the offending header VALUE back, and that text is spliced into a PERSISTED job-error string, so `assertValidRequestHeaders` names the header but never echoes its value. `withJsonContentType` merges the JSON content type case-insensitively: `{ "Content-Type": …, ...headers }` left a caller's lowercase `content-type` as a second property that `Headers` folds into one comma-joined field, sending both types. Adds `credential_key` / `credential_scheme` / `credential_header` to `WebhookNotifyTask`, so a bearer/HMAC endpoint no longer forces the secret into the `headers` port — which `Task.toJSON` writes verbatim into the graph JSON. The credential is placed BEFORE the header validation, so the credential-produced header is validated too; that ordering is why the two changes ship together. `webhookPrivateEntitlements` takes an options object: either credential key enforces the `credential` entitlement, but only `url_credential_key` unscopes a declared `network:private` grant — a header credential changes what the request carries, not where it goes. Also gates the body-derived retry hint behind `!allowPrivate` (a declared private destination's reply is never surfaced, and `retry_after` is read from the body; the `Retry-After` header is transport-level and stays), and exports `./util/RetryAfter` and `./util/WebhookPost` from the barrel. Co-authored-by: Claude <noreply@anthropic.com>
…le in Slack blocks Block Kit says `<url|label>` a second way, structurally, and the delabeling remedy was purely lexical: a button, a rich_text `link` element and an overflow `option` each carry a `url` beside a label field with no `<` in the payload at all, so an attacker-chosen label masked an attacker-chosen destination in the DEFAULT config while the README claimed blocks links were reduced to their bare URL. The reduction now runs in the object branch of the deep walk, under the same `policy.stripLabels` that drives the lexical one, and keys on SHAPE rather than on a list of element types: an overflow `option` carries no `type` discriminator, so a type set cannot reach it, and a shape rule covers whatever url-bearing element Slack adds next. It is narrow because only Block Kit objects that ARE links carry a plain `url` beside a label — `image` uses `image_url`/`alt_text`, `video` uses `title_url`. The destination is kept and the label overwritten, never the reverse: a button stripped of its `url` with no live `action_id` behind it is an availability change, the same argument the broadcast rewrite already makes for rewriting rather than deleting. The date-token exemption had the same shape of hole. Slack's token is `<!date^ts^token_string^optional_link|fallback>`, so the four-field form carries both a label and a destination, and both regexes exempted it on a two-character prefix. The exemption is now the safe ARITY, built from one shared source string so the broadcast escape and the delabeler cannot drift, and each rule is independently correct because `stripSlackLinkLabels` is exported and callable alone. Its structural twin — the rich_text `date` element's optional `url` — is the one case where deleting is right, since the label comes from `format`/`fallback` and a date renders fine unlinked. Two deliberate behavior tightenings: a legitimately linked date token now needs `allow_markup`, and a date token whose fallback contains a `<` is a shape the matcher cannot finish verifying, so it is escaped whole rather than exempted with something unverified inside it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lgxtp7mQECdh7F2UT9CVwN
…annot be resolved `url_credential_key` already refuses to post when the store cannot answer a configured key; `credential_key` failed OPEN on the identical case. `applyCredentialToHeaders` returns the caller's headers unchanged when the resolved credential is `undefined`, and the input resolver yields exactly that on a miss — so a locked store or a mistyped key sent the notification unauthenticated (or unsigned) and reported success. `resolveWebhookCredential` mirrors `resolveWebhookUrl` beside it, and `WebhookNotifyTask` calls it before applying the credential. `Object.hasOwn(input, "credential_key")` is the discriminator: the resolver writes `undefined` over a missed key, so the port is present either way and only its presence separates "no credential wanted" from "credential wanted but the store could not answer". `applyCredentialToHeaders` itself is left fail-open and untouched. It is pure, carries no `configured` signal, and is shared with `FetchUrlTask`, which must not inherit an unasked-for behavior change. The guard also fires under `credential_scheme: "none"`: resolve-but-don't-send is a debug affordance, not a reason to swallow a locked store, since the operator configured a key either way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lgxtp7mQECdh7F2UT9CVwN
…s and stacks `WebhookNotifyTask` sets `readSuccessBody: true`, and the only redaction on a success body was `redactWebhookUrlIn`, which knows the URL and nothing else. A secret placed on `Authorization` (or a signing header) was therefore never a redaction candidate, so an echoing endpoint — webhook.site, RequestBin, a chatty 200 — returned up to 1 KB of it verbatim into the `response` output port: task output, pipeable and persisted with the run. The module header claimed the URL secret never reaches task output; the second secret shape had no equivalent guard. `createWebhookRedactor(url, secrets)` is now the single function every echoed string goes through — success body, failure body suffix, reason phrase, stringify detail, and every rewritten error message and stack. Order inside it is load-bearing: exact secrets first, longest-first, then the URL pass, because reversed the URL pass can chop a substring out of a secret and leave the remainder unmatched. Each secret is admitted raw and percent-encoded, mirroring the URL pass. There is no minimum length, unlike URL path segments — a header secret is a known exact value rather than a guess, and a mangled diagnostic beats a leaked short API key; the JSDoc states that residual. `secrets` is declared (not optional) on `WebhookPostRequest`, so all three call sites answer the question: `WebhookNotifyTask` passes its resolved credential, Slack and Discord pass `undefined`. It is redacted unconditionally, including under `credential_scheme: "none"` — a value never sent cannot be echoed, so redacting costs nothing and keeps a scheme-dependent branch out of a security path. `redactedStackFrom` and `toRedactedWebhookError` take the redactor instead of the raw url. `redactedStackFrom` is exported from the package root and `toRedactedWebhookError` is module-private; `git grep` finds no consumer of either outside this file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lgxtp7mQECdh7F2UT9CVwN
Two conflicts, both in files main's streaming rework rewrote. `SafeFetch.server.ts` — an import list where each side added one name: `createSafeFetchRedirectError` (this branch) and `applyCrossOriginHeaderStrip` (main). Union; both are used. `FetchUrlTask.ts` — main's side taken wholesale. The conflicting block on this branch is the pre-streaming body-parsing code (`resolvedResponseType`, the json/text/blob/arraybuffer switch), which main deleted outright: that identifier occurs 13 times here and 0 times on main. Keeping it would have reinstated a shape the surrounding code no longer supports. Taking main's side would have dropped this branch's own contribution, so it is re-applied rather than lost. #758 bounded the endpoint-supplied `Retry-After` into `util/RetryAfter`, and main's `buildHttpError` still carries the unbounded form — `Number.isFinite(seconds) && seconds > 0` with no ceiling, which is the value that parks a job for millennia or overflows into an Invalid Date the queue cannot reschedule from. `buildHttpError` now calls `retryDateFromRetryAfterHeader`; the module itself merged cleanly and is unchanged. Verified: bun scripts/test.ts task vitest -> 68 files, 1193 passed, 24 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lgxtp7mQECdh7F2UT9CVwN
…-slack-structural-links The base branch merged origin/main (main's streaming rework of FetchUrlTask, plus the re-applied bounded Retry-After parser). No conflicts here. Verified: bun scripts/test.ts task vitest -> 68 files green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lgxtp7mQECdh7F2UT9CVwN
…-webhook-credential-lifecycle The base branch merged origin/main (main's streaming rework of FetchUrlTask, plus the re-applied bounded Retry-After parser). No conflicts here. Verified: bun scripts/test.ts task vitest -> 68 files green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lgxtp7mQECdh7F2UT9CVwN
…d74u5n-slack-structural-links fix(tasks): close structural masked links and the date-token arity hole in Slack blocks
…d74u5n-webhook-credential-lifecycle fix(tasks): give the webhook header credential a lifecycle — fail closed on a store miss, redact it out of output
…RL's own classification BREAKING CHANGE: setting `allow_private_destination` on a destination whose URL does not classify private now requires a registered `ENTITLEMENT_ENFORCER` granting `network:private` for that origin. Loopback / RFC1918 / link-local URLs are unaffected — a declaration on one of those is still authorized by the declaration alone. `19354ebaa` did not newly expose the literal-`169.254.169.254`-plus-flag case: with `allowPrivate = classification.kind === "private"` a statically private URL already received `allowPrivate: true`. What it newly widened is the PUBLIC-LOOKING URL — setting the flag there now disables `SafeFetch.server.ts`'s resolved-address check for a name `classifyUrl` reads as public. That check is the only DNS-rebinding defence in the stack: the scope re-check is tautological (`privateResourceScopes` is `urlResourcePattern(url)` of the same URL) and `redirect: "error"` means there is no second hop. Meanwhile `assertPrivateDestinationGranted` returned immediately when the registry carried no `ENTITLEMENT_ENFORCER`, and that token has no default registration — `TaskGraphRunner` is the only registrar and is guarded by `enforceEntitlements`, which defaults to false — while `TaskRunner.registry` defaults to `globalServiceRegistry`. So the documented guarantee that "every widened request is entitlement-checked" was false for the common case. The rule now enforced: a declaration may widen the transport only as far as the URL itself already declares. A private-reading URL is visible in the operator's configured value, so the flag on it authorizes nothing reading that configuration would not already show. A public-reading one is invisible in configuration AND buys the DNS-guard bypass, so it must be graded by a policy — and with no enforcer to grade it, the post is refused with `PRIVATE_DENIED` naming both remedies. Failing closed on EVERY declaration was rejected: with no default enforcer it would make the flag inert for approximately every current user, remove the stock "post to my internal webhook" case, and contradict `FetchUrlTask`'s shipped treatment of a declared private URL. Documenting the gap alone was rejected too: the DNS bypass on a public-looking hostname is genuinely new. `assertPrivateDestinationGranted` now returns whether a policy actually graded the destination (`"granted" | "unenforced"`) rather than `void`, so the caller can tell "allowed by policy" from "no policy exists". Widening the return type is source-compatible for external callers. Residual, unfixed here: with no enforcer registered the flag on an already-private URL is authorized by configuration alone, and if that URL arrives by dataflow the operator authorized nothing. The right shape is a config-only port, which is a breaking schema change affecting saved graph JSON and the builder UI's port rendering; tracked separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT
…t only on a whole-URL match `toRedactedWebhookError` decided whether to rewrite a typed transport error with `leaksUrl`, which consulted only `url` and only as a whole string. `request.secrets` was never consulted at all. Both halves are narrower than the invariant this module's own header states — no VALUE it was handed as a secret reaches a message, an output port or a stack. Two shapes walked straight out untouched: - an endpoint echoes a FRAGMENT, never the whole URL. Express answers an unknown route with `Cannot POST /services/T0…/SECRETTOKEN`, naming the path and no origin, so a typed error carrying exactly that matched nothing; - a HEADER credential (`credential_key`) is a secret the post was handed, and `leaksUrl` could not see it under any circumstances. The gate now asks over values: compute `redact(message)`, `redact(stack)` and `error.url === url || redact(error.url) !== error.url`, and return the error unchanged only when all three are no-ops. `.url` is asked separately because a transport may pair a clean message with a `url` field that IS the credential; `redactedStack` is only a trigger, since `redactedStackFrom` still builds the rebuilt stack from the rebuilt message. Accepted cost: an error that previously passed through by IDENTITY may now be rebuilt when the redactor touches an incidental substring — the same trade already documented on `redactWebhookUrlIn`. The rebuild preserves `code`, so the retryable/permanent classification survives; `a typed error carrying no secret keeps its identity` pins that this did not turn every error into a copy. Cost in work: 5-8 short-string passes plus one per `secrets` entry, on a path that has already paid a network round-trip. The pre-existing header-secret transport case rejects an UNTYPED `TypeError`, which takes the generic rebuild-everything branch, so the typed branch with a header secret had no coverage at all. Its new twin passes `url` as the ORIGIN rather than the whole webhook URL: with the full URL the old whole-string test fires for an unrelated reason and the case would pass against the unfixed code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT
…the coverage claim `reduceStructuralLink` keyed on a property named exactly `url`, so Slack's `video` block — `title_url` beside a required `title` — was uncovered. A caller-supplied title masking a caller-supplied destination is the same phishing primitive the button / rich_text-link / overflow-option cases exist for, and the doc comment was citing "`video` uses `title_url`" as EVIDENCE of narrowness while that shape was the gap. Deliberately NOT generalized to "any `*_url` with a sibling label". An `image` block legitimately carries `image_url` AND an optional `title`, so a wildcard would rewrite an image's caption to its own CDN URL — a false positive in the commonest block type there is. `MASKED_LINK_PAIRS` enumerates the pairs instead (`url`+`text`, `title_url`+`title`), with `image_url` absent because it is the picture, not a destination a label can mask. Within a pair the rule stays shape-driven, which is what reaches an overflow `option` carrying no `type` discriminator. `overwriteLabel` handles both label shapes — a bare string and a text composition — so a video block's `plain_text` title is covered by the same branch a button's label is. The `date` deletion stays scoped to `url`: a `title_url` with no `title` is not a video block, because Slack requires one. Accepted cost: a `title` overwritten with a long `title_url` can exceed Slack's 200-character limit and draw `invalid_blocks` — the same trade the button path already makes against its 75-character `text` limit. Recorded as a residual rather than treated as a reason to skip the shape: a video block additionally requires `links.embed:write` and a registered unfurl domain, and Slack's docs do not say whether an incoming webhook can satisfy that, so exploitability is bounded but unresolved. The coverage claim in code and README was wrong either way, and both now name the shapes actually covered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT
…t private denial reason Three small corrections, none of which changes behaviour beyond the strings they report. `webhookPrivateEntitlements`' unscoped-grant reason blamed the credential store in two distinct situations. `scoped` is one decision about the RESOURCE, but it is false both when the URL IS a credential and when the instance configures no `url` at all — and in the second case the destination arrives as a dataflow or run-input value AFTER entitlements are evaluated, which is a different problem with a different fix. The reason is now three-way. That second shape is exactly what the graph-root smuggling case constructs, and nothing asserted these strings before. `WebhookNotifyTask` declared two credential ports both titled `Credential Key`, which a UI rendering by title cannot tell apart even though one IS the destination and the other only rides on a request header. They are now `Webhook URL Credential Key` and `Header Credential Key`, with the former applied to the Slack and Discord tasks too. `FetchUrlTask`'s single `credential_key` is left alone — it has no sibling to be confused with. The README quickstart imported `fetch` from `@workglow/tasks`, which does not exist; the helper is `fetchUrl`. The two other `fetch(` uses further down are the GLOBAL fetch inside a LambdaTask body and a config-loading snippet, and are correct as written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HJRf3YFa8DjmjsZvXz8xDT
…ing-fail-closed fix(tasks)!: require a graded network:private grant to widen past a URL's own classification
…-block-coverage fix(tasks): redact webhook errors over values, cover the video block's masked link, and three low-severity corrections
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.