fix(bluebubbles): route fetchImpl through bundled undici fetch on Node 24+#66103
fix(bluebubbles): route fetchImpl through bundled undici fetch on Node 24+#66103coletebou wants to merge 1 commit intoopenclaw:mainfrom
Conversation
…hment downloads on Node 24+ The SSRF guard pinned-DNS dispatcher is an undici 8.x Agent, but Node 24 built-in globalThis.fetch uses undici 7.x internally. Passing the 8.x dispatcher to the native fetch causes 'invalid onRequestStart method' errors, breaking all BlueBubbles attachment downloads. Route through fetchWithRuntimeDispatcher (bundled undici own fetch) when a dispatcher is present, matching the approach used by the Slack extension (openclaw#62239). This preserves SSRF DNS pinning instead of stripping the dispatcher entirely.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44561f209d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return await useFetch( | ||
| resolveRequestUrl(input), | ||
| { ...init, method: init?.method ?? "GET" }, | ||
| opts.timeoutMs, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Restore timeout support for attachment downloads
downloadBlueBubblesAttachment previously used blueBubblesFetchWithTimeout, which enforced opts.timeoutMs (and a 10s default), but this new fetchImpl now calls fetchWithRuntimeDispatcher/globalThis.fetch directly with no timeout path. When the BlueBubbles host stalls (e.g., hung connection or non-responsive server), attachment fetches can now wait indefinitely and block message processing, so this change regresses reliability for slow/failing networks.
Useful? React with 👍 / 👎.
Greptile SummaryRoutes One P1 concern: Confidence Score: 3/5Fix is directionally correct but drops an existing 10 s timeout guard, leaving downloads able to hang indefinitely. The Node 24+ routing fix is sound and follows the established Slack pattern. However, opts.timeoutMs (previously forwarded via blueBubblesFetchWithTimeout) is now silently ignored — a real behavioral regression on all Node versions where the dispatcher path is taken. That P1 issue should be addressed before merging. extensions/bluebubbles/src/attachments.ts — the fetchImpl block at lines 125–133 Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/bluebubbles/src/attachments.ts
Line: 125-133
Comment:
**`opts.timeoutMs` is now silently dropped**
The previous `fetchImpl` passed `opts.timeoutMs` (default 10 s) to `blueBubblesFetchWithTimeout`, which created an `AbortController` and aborted the request on expiry. The new implementation calls `fetchWithRuntimeDispatcher` / `globalThis.fetch` with no timeout and no signal, so a stalled BlueBubbles server will cause the download to hang indefinitely. Any caller that passes a `timeoutMs` in `BlueBubblesAttachmentOpts` will find it has no effect.
A minimal fix is to forward a timeout signal (using the already-available `AbortSignal.timeout` in Node 17.3+), falling back to `opts.timeoutMs ?? 10_000`:
```typescript
fetchImpl: async (input, init) => {
const useFetch = init && "dispatcher" in init
? fetchWithRuntimeDispatcher
: globalThis.fetch;
const resolvedInit: RequestInit = {
...init,
method: init?.method ?? "GET",
...(!(init as RequestInit | undefined)?.signal
? { signal: AbortSignal.timeout(opts.timeoutMs ?? 10_000) }
: {}),
};
return await useFetch(resolveRequestUrl(input), resolvedInit);
},
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/bluebubbles/src/attachments.ts
Line: 126-128
Comment:
**Missing `isMockedFetch` guard (parity with Slack pattern)**
The Slack reference implementation (`extensions/slack/src/monitor/media.ts`) uses `!isMockedFetch(globalThis.fetch)` before routing to `fetchWithRuntimeDispatcher`, so that a test-installed mock on `globalThis.fetch` is honoured even when `init` carries a `dispatcher`. The BlueBubbles code omits this guard and will always call `fetchWithRuntimeDispatcher` when a dispatcher is present, bypassing any `globalThis.fetch` mock. Current tests are insulated because they fully replace `fetchRemoteMedia`, but future tests using the real `fetchRemoteMedia` with a `globalThis.fetch` mock would silently execute real network calls instead of the mock.
```suggestion
const useFetch = init && "dispatcher" in init && !isMockedFetch(globalThis.fetch)
? fetchWithRuntimeDispatcher
: globalThis.fetch;
```
(requires a local `isMockedFetch` helper, mirroring the one in `extensions/slack/src/monitor/media.ts`)
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(bluebubbles): route fetchImpl throug..." | Re-trigger Greptile |
| fetchImpl: async (input, init) => { | ||
| const useFetch = init && "dispatcher" in init | ||
| ? fetchWithRuntimeDispatcher | ||
| : globalThis.fetch; | ||
| return await useFetch( | ||
| resolveRequestUrl(input), | ||
| { ...init, method: init?.method ?? "GET" }, | ||
| opts.timeoutMs, | ||
| ), | ||
| ); | ||
| }, |
There was a problem hiding this comment.
opts.timeoutMs is now silently dropped
The previous fetchImpl passed opts.timeoutMs (default 10 s) to blueBubblesFetchWithTimeout, which created an AbortController and aborted the request on expiry. The new implementation calls fetchWithRuntimeDispatcher / globalThis.fetch with no timeout and no signal, so a stalled BlueBubbles server will cause the download to hang indefinitely. Any caller that passes a timeoutMs in BlueBubblesAttachmentOpts will find it has no effect.
A minimal fix is to forward a timeout signal (using the already-available AbortSignal.timeout in Node 17.3+), falling back to opts.timeoutMs ?? 10_000:
fetchImpl: async (input, init) => {
const useFetch = init && "dispatcher" in init
? fetchWithRuntimeDispatcher
: globalThis.fetch;
const resolvedInit: RequestInit = {
...init,
method: init?.method ?? "GET",
...(!(init as RequestInit | undefined)?.signal
? { signal: AbortSignal.timeout(opts.timeoutMs ?? 10_000) }
: {}),
};
return await useFetch(resolveRequestUrl(input), resolvedInit);
},Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/bluebubbles/src/attachments.ts
Line: 125-133
Comment:
**`opts.timeoutMs` is now silently dropped**
The previous `fetchImpl` passed `opts.timeoutMs` (default 10 s) to `blueBubblesFetchWithTimeout`, which created an `AbortController` and aborted the request on expiry. The new implementation calls `fetchWithRuntimeDispatcher` / `globalThis.fetch` with no timeout and no signal, so a stalled BlueBubbles server will cause the download to hang indefinitely. Any caller that passes a `timeoutMs` in `BlueBubblesAttachmentOpts` will find it has no effect.
A minimal fix is to forward a timeout signal (using the already-available `AbortSignal.timeout` in Node 17.3+), falling back to `opts.timeoutMs ?? 10_000`:
```typescript
fetchImpl: async (input, init) => {
const useFetch = init && "dispatcher" in init
? fetchWithRuntimeDispatcher
: globalThis.fetch;
const resolvedInit: RequestInit = {
...init,
method: init?.method ?? "GET",
...(!(init as RequestInit | undefined)?.signal
? { signal: AbortSignal.timeout(opts.timeoutMs ?? 10_000) }
: {}),
};
return await useFetch(resolveRequestUrl(input), resolvedInit);
},
```
How can I resolve this? If you propose a fix, please make it concise.
| const useFetch = init && "dispatcher" in init | ||
| ? fetchWithRuntimeDispatcher | ||
| : globalThis.fetch; |
There was a problem hiding this comment.
isMockedFetch guard (parity with Slack pattern)
The Slack reference implementation (extensions/slack/src/monitor/media.ts) uses !isMockedFetch(globalThis.fetch) before routing to fetchWithRuntimeDispatcher, so that a test-installed mock on globalThis.fetch is honoured even when init carries a dispatcher. The BlueBubbles code omits this guard and will always call fetchWithRuntimeDispatcher when a dispatcher is present, bypassing any globalThis.fetch mock. Current tests are insulated because they fully replace fetchRemoteMedia, but future tests using the real fetchRemoteMedia with a globalThis.fetch mock would silently execute real network calls instead of the mock.
| const useFetch = init && "dispatcher" in init | |
| ? fetchWithRuntimeDispatcher | |
| : globalThis.fetch; | |
| const useFetch = init && "dispatcher" in init && !isMockedFetch(globalThis.fetch) | |
| ? fetchWithRuntimeDispatcher | |
| : globalThis.fetch; |
(requires a local isMockedFetch helper, mirroring the one in extensions/slack/src/monitor/media.ts)
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/bluebubbles/src/attachments.ts
Line: 126-128
Comment:
**Missing `isMockedFetch` guard (parity with Slack pattern)**
The Slack reference implementation (`extensions/slack/src/monitor/media.ts`) uses `!isMockedFetch(globalThis.fetch)` before routing to `fetchWithRuntimeDispatcher`, so that a test-installed mock on `globalThis.fetch` is honoured even when `init` carries a `dispatcher`. The BlueBubbles code omits this guard and will always call `fetchWithRuntimeDispatcher` when a dispatcher is present, bypassing any `globalThis.fetch` mock. Current tests are insulated because they fully replace `fetchRemoteMedia`, but future tests using the real `fetchRemoteMedia` with a `globalThis.fetch` mock would silently execute real network calls instead of the mock.
```suggestion
const useFetch = init && "dispatcher" in init && !isMockedFetch(globalThis.fetch)
? fetchWithRuntimeDispatcher
: globalThis.fetch;
```
(requires a local `isMockedFetch` helper, mirroring the one in `extensions/slack/src/monitor/media.ts`)
How can I resolve this? If you propose a fix, please make it concise.|
Closing — review caught a dropped timeout regression and missing isMockedFetch guard. Resubmitting with fixes. |
Summary
BlueBubbles attachment downloads fail on Node 24+ because the SSRF guard's pinned-DNS dispatcher is an undici 8.x
Agent, but Node 24's built-inglobalThis.fetchuses undici 7.x internally. Passing the 8.x dispatcher causes"invalid onRequestStart method"errors.This routes through
fetchWithRuntimeDispatcher(bundled undici's own fetch) when a dispatcher is present ininit, matching the approach already used by the Slack extension (#62239). This preserves SSRF DNS pinning instead of stripping the dispatcher.Supersedes #63984 which took a less clean approach (stripping the dispatcher entirely, losing DNS pinning).
Changes
extensions/bluebubbles/src/attachments.ts: UsefetchWithRuntimeDispatcherwheninitcontains adispatcher, fall back toglobalThis.fetchotherwise. Remove unusedblueBubblesFetchWithTimeoutimport.Test plan