fix(slack): strip SSRF dispatcher from media fetch to prevent undici version conflict by mjamiv · Pull Request #64022 · openclaw/openclaw

mjamiv · 2026-04-10T02:14:14Z

Summary

Fixes #63905 — Slack inbound attachments silently fail with invalid onRequestStart method in container/sandbox deployments.

Root cause: fetchWithSsrFGuard injects a pinned undici 8.x dispatcher into the init object, then delegates to createSlackMediaFetch(). That function spreads ...init (including the dispatcher) into fetchWithRuntimeDispatcher, which creates its own dispatcher. The two dispatchers from different undici major versions (bundled 8.x vs Node's built-in 7.x) collide, throwing the error. Every Slack attachment download silently fails and the agent only sees placeholder text.

Fix: Use the presence of dispatcher in init as a routing signal (runtime fetch vs global fetch) but strip the dispatcher itself before forwarding. Same pattern as #63984 (BlueBubbles).

Changes

extensions/slack/src/monitor/media.ts — Destructure dispatcher out of init in createSlackMediaFetch before delegating
extensions/slack/src/monitor/media.test.ts — Update assertion: dispatcher should NOT be forwarded to runtime fetch

Verification

Tested on a production OpenClaw v2026.4.9 instance running in an OpenShell sandbox with Slack channel integration. Before the fix, all inbound PDF/image attachments produced only placeholder text. After applying the equivalent patch to the dist bundle (actions-BsuJQp72.js), attachments download and the agent analyzes full file content with zero fetch errors.

greptile-apps · 2026-04-10T02:15:43Z

Greptile Summary

This PR fixes a silent failure in Slack inbound attachment downloads caused by two undici dispatcher instances from different major versions colliding. The fix in createSlackMediaFetch now destructures and discards the upstream SSRF guard's dispatcher from init before forwarding to fetchWithRuntimeDispatcher, while still using its presence as the routing signal. The accompanying test assertion is correctly updated to verify the dispatcher is absent in the forwarded call.

Confidence Score: 5/5

Safe to merge — targeted two-line logic change with a matching test update and no side effects on other code paths.

The fix is minimal and correct: it strips the upstream dispatcher before delegating (preventing the undici collision) while preserving the routing signal. Both the happy path and the fallback (mocked fetch in tests) remain correct. No other files are affected.

No files require special attention.

_{Reviews (1): Last reviewed commit: "fix(slack): strip SSRF dispatcher from S..." | Re-trigger Greptile}

mjamiv · 2026-04-10T02:41:34Z

Note: this fix also covers the code path addressed by #63401 (stripping dispatcher before globalThis.fetch). Since we destructure dispatcher out before choosing the fetch impl, both the fetchWithRuntimeDispatcher and globalThis.fetch branches receive clean init. These two PRs are complementary but this one is the broader fix.

cc @martingarramon — your confirmation of the root cause on #63905 matches exactly. The #62551 / #62530 / #62239 lineage you mentioned is helpful context.

mjamiv · 2026-04-11T15:16:04Z

Cross-link for reviewer context: PR #64766 was merged on 2026-04-11 and touches the same fetchWithSsrFGuard surface this PR modifies — it disables the pinned DNS dispatcher on the FormData transcription code path and tightens hostname validation on the pinDns: false branch. The two changes are complementary (#64766 covers multipart FormData; this PR covers the Slack inbound media onRequestStart case), but they live in the same file, so reviewing them together may be efficient. Happy to rebase if there's a conflict after #64766 landed.

…ndici version conflict When `fetchWithSsrFGuard` delegates to `createSlackMediaFetch`, it injects a pinned undici 8.x dispatcher into the `init` object. `createSlackMediaFetch` then spreads `...init` (including that dispatcher) into `fetchWithRuntimeDispatcher`, which creates its own dispatcher. The two dispatchers from different undici versions collide, causing "invalid onRequestStart method" errors that silently drop all inbound Slack attachments. Fix: use the presence of `dispatcher` in `init` as a routing signal (runtime fetch vs global fetch) but strip it before forwarding. This matches the approach taken in openclaw#63984 (BlueBubbles) for the same root cause. Fixes openclaw#63905

The dispatcher-strip fix added 7 lines to createSlackMediaFetch, shifting the three pre-existing fetch() callsites in fetchWithSlackAuth from lines 96/115/120 to 103/122/127.

clawsweeper · 2026-04-27T02:21:53Z

Codex review: found issues before merge.

Summary
The PR strips dispatcher from Slack media fetch init before delegating, updates the Slack media regression assertion, and adjusts raw-fetch allowlist line numbers.

Reproducibility: yes. The linked Slack report gives concrete container/sandbox steps and invalid onRequestStart method logs, and current main still routes Slack inbound media through createSlackMediaFetch with a guard-created dispatcher; I did not run a live Slack workspace in this read-only review.

Next step before merge
This is a real, narrow Slack attachment failure, but the remaining blocker is a maintainer/security decision about Slack's SSRF transport contract plus a branch rebuild.

Security
Needs attention: The diff has no dependency, workflow, secret, or supply-chain changes, but it changes SSRF pinned-dispatcher enforcement for Slack media downloads.

Review findings

[P2] Preserve the guarded dispatcher contract — extensions/slack/src/monitor/media.ts:88-96
[P2] Refresh the raw-fetch allowlist against main — scripts/check-no-raw-channel-fetch.mjs:61-63

Review details

Best possible solution:

Rebuild from current main and make the Slack media transport contract explicit: either approve a reviewed preflight-only/pinDns: false path with focused tests and fresh allowlist lines, or fix the Undici conflict while preserving pinned dispatcher enforcement.

Do we have a high-confidence way to reproduce the issue?

Yes. The linked Slack report gives concrete container/sandbox steps and invalid onRequestStart method logs, and current main still routes Slack inbound media through createSlackMediaFetch with a guard-created dispatcher; I did not run a live Slack workspace in this read-only review.

Is this the best way to solve the issue?

No, not as currently proposed. The source/test change is narrow and has a BlueBubbles precedent, but it silently removes the pinned dispatcher from the final Slack request and the branch carries a stale allowlist update.

Full review comments:

[P2] Preserve the guarded dispatcher contract — extensions/slack/src/monitor/media.ts:88-96
This strips the dispatcher that fetchWithSsrFGuard attached before the actual Slack media request. On current main that dispatcher is the pinned-DNS enforcement path; treating it only as a routing signal changes Slack media downloads to preflight validation without an explicit reviewed pinDns: false contract.
Confidence: 0.82
[P2] Refresh the raw-fetch allowlist against main — scripts/check-no-raw-channel-fetch.mjs:61-63
The branch updates Slack raw-fetch allowlist entries to 103, 122, and 127, but current main's Slack entries are 106, 125, and 130. Drop or regenerate this commit during the rebuild, otherwise the raw-fetch check will be stale after rebasing.
Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.82

Security concerns:

[medium] Pinned DNS dispatcher stripped — extensions/slack/src/monitor/media.ts:88
The patch removes the guard-created dispatcher from the final Slack media fetch. That may be acceptable only after maintainers approve Slack's media path as preflight-only, because the current dispatcher is the DNS pinning enforcement mechanism.
Confidence: 0.82

Acceptance criteria:

pnpm test extensions/slack/src/monitor/media.test.ts
pnpm exec oxfmt --check --threads=1 extensions/slack/src/monitor/media.ts extensions/slack/src/monitor/media.test.ts scripts/check-no-raw-channel-fetch.mjs
pnpm check:changed

What I checked:

Current Slack media forwards dispatcher: createSlackMediaFetch still selects fetchWithRuntimeDispatcher when init contains dispatcher, then forwards { ...init, redirect: "manual" }. (extensions/slack/src/monitor/media.ts:96, f523620abe7f)
Current test asserts dispatcher forwarding: The Slack media test expects runtime fetch init to contain dispatcher; this PR reverses that assertion. (extensions/slack/src/monitor/media.test.ts:693, f523620abe7f)
Reported path is guarded Slack media: resolveSlackMedia builds createSlackMediaFetch() and passes it into fetchRemoteMedia with SLACK_MEDIA_SSRF_POLICY, so the dispatcher handoff is directly on inbound Slack attachment downloads. (extensions/slack/src/monitor/media.ts:335, f523620abe7f)
SSRF guard attaches pinned dispatcher: fetchWithSsrFGuard creates request init with dispatcher, treats custom fetch implementations as dispatcher-capable, and calls the provided fetch implementation with that init. (src/infra/net/fetch-guard.ts:406, f523620abe7f)
Runtime fetch does not replace stripped dispatcher: fetchWithRuntimeDispatcher loads the repo Undici runtime fetch and passes normalized init through; if Slack strips dispatcher, this function does not create a replacement pinned dispatcher. (src/infra/net/runtime-fetch.ts:79, f523620abe7f)
BlueBubbles precedent exists: BlueBubbles currently strips an upstream dispatcher and documents the same invalid onRequestStart method failure mode, which supports the diagnosis but does not by itself approve Slack's transport contract. (extensions/bluebubbles/src/types.ts:205, f523620abe7f)

Likely related people:

steipete: Recent GitHub history shows several April 2026 commits in extensions/slack/src/monitor/media.ts, including bounded inbound media downloads, non-image file returns, and routing Slack media auth fetch through runtime fetch. (role: recent Slack media maintainer; confidence: high; commits: 5a1ff1347dfa, d1cc54866d6d, b9a0795761aa; files: extensions/slack/src/monitor/media.ts, extensions/slack/src/monitor/media.test.ts)
openperf: Merged PR fix(slack): prevent undici dispatcher leak to globalThis.fetch causing media download failure Markdown #62239 addressed a closely related Slack dispatcher leak failure in the same media path and added the runtime-fetch routing/test context. (role: adjacent behavior introducer; confidence: high; commits: e8fb14064207; files: extensions/slack/src/monitor/media.ts, extensions/slack/src/monitor/media.test.ts)
GodsBoy: PR fix(media): disable pinned DNS dispatcher for FormData transcription requests #64766 changed the same fetchWithSsrFGuard pinDns/dispatcher surface and documented the preflight-only security tradeoff for another media path. (role: adjacent fetch-guard maintainer; confidence: medium; commits: c159d22b34fa, 43bd5545f839; files: src/infra/net/fetch-guard.ts, src/media-understanding/shared.test.ts)
martingarramon: Contributed nearby Slack thread fetch diagnostics and was explicitly mentioned in the discussion as confirming the dispatcher-wrapping diagnosis. (role: adjacent Slack diagnostics contributor; confidence: medium; commits: 31e5cd6376ee, 6368559c024d; files: extensions/slack/src/monitor/media.ts, scripts/check-no-raw-channel-fetch.mjs)

Remaining risk / open question:

Maintainers still need to decide whether Slack media may use a preflight-only or pinDns: false contract instead of preserving pinned dispatcher enforcement.
The branch is stale against current main; the raw-fetch allowlist update should be dropped or regenerated during a rebuild.

Codex review notes: model gpt-5.5, reasoning high; reviewed against f523620abe7f.

mjamiv · 2026-05-01T02:33:40Z

Conflict review update:

The branch currently has no merge-base with current main, so GitHub's CONFLICTING state is not a small ordinary merge conflict. The clean refresh path is to rebuild from current main.
Cherry-picking 757635ac55 (fix(slack): strip SSRF dispatcher from Slack media fetch to prevent undici version conflict) onto current main applies cleanly.
Cherry-picking b6cdc58c73 conflicts only in scripts/check-no-raw-channel-fetch.mjs.
For that script conflict, current main appears to be the right side to keep: Slack allowlist lines are now 106, 125, and 130. The PR side has stale Slack line numbers 103, 122, and 127, plus stale Tlon entries. I did not find raw fetch( calls under extensions/tlon/src on current main, so the Tlon additions do not look needed.
In a temporary worktree, keeping current main's allowlist entries left no conflict markers and git diff --check passed.

I am not pushing a refresh from automation because the security decision is still the blocker: this patch strips the SSRF guard's pinned dispatcher from the actual Slack media request. That may be acceptable only if maintainers explicitly approve a Slack-specific preflight-only contract. Otherwise the safer direction is to preserve pinned dispatcher enforcement while fixing the Undici version collision.

Recommended next step: maintainer decides the Slack transport contract first; after that, rebuild the PR on current main, keep the Slack source/test change if approved, drop/regenerate the stale allowlist commit, then run:

pnpm test extensions/slack/src/monitor/media.test.ts
pnpm exec oxfmt --check --threads=1 extensions/slack/src/monitor/media.ts extensions/slack/src/monitor/media.test.ts scripts/check-no-raw-channel-fetch.mjs
pnpm check:changed

mjamiv mentioned this pull request Apr 10, 2026

bug(slack): inbound attachments in container sandbox fail with placeholder-only turn and fetch error 'invalid onRequestStart method' #63905

Open

openclaw-barnacle Bot added channel: slack Channel integration: slack size: XS labels Apr 10, 2026

mjamiv mentioned this pull request Apr 10, 2026

fix(bluebubbles): strip SSRF dispatcher from fetchImpl to fix attachment downloads #63984

Closed

Molt added 2 commits April 13, 2026 11:48

fix(ci): update raw-fetch allowlist line numbers for slack/media.ts

b6cdc58

The dispatcher-strip fix added 7 lines to createSlackMediaFetch, shifting the three pre-existing fetch() callsites in fetchWithSlackAuth from lines 96/115/120 to 103/122/127.

mjamiv force-pushed the fix/slack-media-dispatcher-conflict branch from ff59c9c to b6cdc58 Compare April 13, 2026 11:51

openclaw-barnacle Bot added the scripts Repository scripts label Apr 13, 2026

clawsweeper Bot mentioned this pull request Apr 27, 2026

fix(slack): strip dispatcher option before passing to globalThis.fetch #63401

Closed

BingqingLyu mentioned this pull request Apr 28, 2026

fix(slack): strip SSRF dispatcher from media fetch to prevent undici version conflict BingqingLyu/openclaw#2240

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(slack): strip SSRF dispatcher from media fetch to prevent undici version conflict#64022

fix(slack): strip SSRF dispatcher from media fetch to prevent undici version conflict#64022
mjamiv wants to merge 2 commits intoopenclaw:mainfrom
mjamiv:fix/slack-media-dispatcher-conflict

mjamiv commented Apr 10, 2026

Uh oh!

greptile-apps Bot commented Apr 10, 2026

Uh oh!

mjamiv commented Apr 10, 2026

Uh oh!

mjamiv commented Apr 11, 2026

Uh oh!

clawsweeper Bot commented Apr 27, 2026 •

edited

Loading

Uh oh!

mjamiv commented May 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

mjamiv commented Apr 10, 2026

Summary

Changes

Verification

Uh oh!

greptile-apps Bot commented Apr 10, 2026

Greptile Summary

Confidence Score: 5/5

Uh oh!

mjamiv commented Apr 10, 2026

Uh oh!

mjamiv commented Apr 11, 2026

Uh oh!

clawsweeper Bot commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjamiv commented May 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

clawsweeper Bot commented Apr 27, 2026 •

edited

Loading