Skip to content

Commit 9497629

Browse files
pgondhi987pgondhi987
authored
fix(msteams): pin attachment fetch DNS
Route Microsoft Teams attachment downloads through the shared SSRF guarded fetch path so DNS validation is pinned into the dispatcher used for the actual request. Keep Teams auth fallback and allowlisted HTTPS Authorization redirect behavior while failing closed for custom fetch hooks that cannot accept dispatcher injection. Verification: - CI=1 OPENCLAW_VITEST_MAX_WORKERS=1 timeout 300 node scripts/run-vitest.mjs run extensions/msteams/src/attachments/shared.test.ts extensions/msteams/src/attachments/bot-framework.test.ts src/infra/net/fetch-guard.ssrf.test.ts - gh pr checks 87567 --repo openclaw/openclaw --watch=false PR: #87567
1 parent e5063f5 commit 9497629

12 files changed

Lines changed: 337 additions & 72 deletions

File tree

‎extensions/codex/src/app-server/run-attempt.test.ts‎

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -272,22 +272,18 @@ async function buildCodexTurnContextForTest(
272272
cwd: workspaceDir,
273273
appServer: resolveCodexAppServerRuntimeOptions({}),
274274
promptText: codexTurnPromptText,
275-
turnScopedDeveloperInstructions:
276-
workspaceBootstrapContext.turnScopedDeveloperInstructions,
275+
turnScopedDeveloperInstructions: workspaceBootstrapContext.turnScopedDeveloperInstructions,
277276
heartbeatCollaborationInstructions:
278277
workspaceBootstrapContext.heartbeatCollaborationInstructions,
279278
});
280279
const collaborationInstructions =
281280
turnStartParams.collaborationMode?.settings?.developer_instructions ?? "";
282-
const inputText =
283-
turnStartParams.input?.find((item) => item.type === "text")?.text ?? "";
281+
const inputText = turnStartParams.input?.find((item) => item.type === "text")?.text ?? "";
284282
const systemPromptReport = buildCodexSystemPromptReport({
285283
attempt: params,
286284
sessionKey: params.sessionKey ?? params.sessionId,
287285
workspaceDir,
288-
developerInstructions: [threadDeveloperInstructions, collaborationInstructions].join(
289-
"\n\n",
290-
),
286+
developerInstructions: [threadDeveloperInstructions, collaborationInstructions].join("\n\n"),
291287
workspaceBootstrapContext,
292288
skillsPrompt: "",
293289
tools: dynamicTools,
@@ -1973,9 +1969,7 @@ describe("runCodexAppServerAttempt", () => {
19731969
developerInstructions?: string;
19741970
};
19751971
expect(threadStartParams.config?.instructions).toBeUndefined();
1976-
expect(threadStartParams.developerInstructions).toContain(
1977-
"OpenClaw Workspace Instructions",
1978-
);
1972+
expect(threadStartParams.developerInstructions).toContain("OpenClaw Workspace Instructions");
19791973
expect(threadStartParams.developerInstructions).toContain(toolGuidance);
19801974
expect(threadStartParams.developerInstructions).not.toContain(agentsGuidance);
19811975
expect(threadStartParams.developerInstructions).not.toContain(soulGuidance);
@@ -2003,10 +1997,8 @@ describe("runCodexAppServerAttempt", () => {
20031997
expect(inputText).toBe("hello");
20041998
expect(inputText).not.toContain(agentsGuidance);
20051999
expect(result.systemPromptReport?.systemPrompt.chars).toBe(
2006-
[
2007-
threadStartParams.developerInstructions ?? "",
2008-
collaborationInstructions,
2009-
].join("\n\n").length,
2000+
[threadStartParams.developerInstructions ?? "", collaborationInstructions].join("\n\n")
2001+
.length,
20102002
);
20112003
});
20122004

‎extensions/msteams/src/attachments/bot-framework.test.ts‎

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ function installRuntime(): MockRuntime {
8181
}
8282

8383
function createMockFetch(entries: Array<{ match: RegExp; response: Response }>): typeof fetch {
84-
return (async (input: RequestInfo | URL) => {
84+
return vi.fn(async (input: RequestInfo | URL) => {
8585
const url =
8686
typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url;
8787
const entry = entries.find((e) => e.match.test(url));
@@ -175,6 +175,7 @@ describe("downloadMSTeamsBotFrameworkAttachment", () => {
175175
tokenProvider: buildTokenProvider(),
176176
maxBytes: 10_000_000,
177177
fetchFn,
178+
fetchFnSupportsDispatcher: true,
178179
resolveFn: resolvePublicHost,
179180
});
180181

@@ -198,6 +199,7 @@ describe("downloadMSTeamsBotFrameworkAttachment", () => {
198199
tokenProvider: buildTokenProvider(),
199200
maxBytes: 10_000_000,
200201
fetchFn,
202+
fetchFnSupportsDispatcher: true,
201203
resolveFn: resolvePublicHost,
202204
});
203205

@@ -218,6 +220,7 @@ describe("downloadMSTeamsBotFrameworkAttachment", () => {
218220
tokenProvider: buildTokenProvider(),
219221
maxBytes: 10_000_000,
220222
fetchFn,
223+
fetchFnSupportsDispatcher: true,
221224
resolveFn: resolvePublicHost,
222225
});
223226

@@ -258,6 +261,7 @@ describe("downloadMSTeamsBotFrameworkAttachment", () => {
258261
tokenProvider: buildTokenProvider(),
259262
maxBytes: 10_000_000,
260263
fetchFn,
264+
fetchFnSupportsDispatcher: true,
261265
resolveFn: resolvePublicHost,
262266
});
263267

@@ -325,15 +329,8 @@ describe("downloadMSTeamsBotFrameworkAttachment", () => {
325329
expect(fetchFn).not.toHaveBeenCalled();
326330
});
327331

328-
describe("Node 24+ dispatcher bypass (issue #63396)", () => {
329-
it("drives the caller's fetchFn directly without the pinned undici dispatcher", async () => {
330-
// Regression: before the fix, fetchBotFrameworkAttachment* routed
331-
// through `fetchWithSsrFGuard`, which installs a `createPinnedDispatcher`
332-
// incompatible with Node 24+'s built-in undici v7. Downloads failed with
333-
// "invalid onRequestStart method". The fix switches to
334-
// `safeFetchWithPolicy`, which calls the supplied `fetchFn` directly
335-
// and never attaches a pinned dispatcher. Verify the caller's `fetchFn`
336-
// is invoked (no dispatcher in init).
332+
describe("guarded attachment fetches", () => {
333+
it("drives dispatcher-aware caller fetchFn hooks through a pinned dispatcher", async () => {
337334
const fileBytes = Buffer.from("BFBYTES", "utf-8");
338335
const fetchCalls: Array<{ url: string; init?: RequestInit }> = [];
339336
const fetchFn: typeof fetch = (async (input: RequestInfo | URL, init?: RequestInit) => {
@@ -365,20 +362,20 @@ describe("downloadMSTeamsBotFrameworkAttachment", () => {
365362
tokenProvider: buildTokenProvider(),
366363
maxBytes: 10_000_000,
367364
fetchFn,
365+
fetchFnSupportsDispatcher: true,
368366
resolveFn: resolvePublicHost,
369367
});
370368

371369
expect(media?.path).toBe(runtime.savePath);
372370
expect(media?.contentType).toBe(runtime.savedContentType);
373371
// Both the attachment info call and the view call should be observed,
374-
// confirming the direct fetch path was taken (no dispatcher interception).
372+
// confirming the guarded fetch path still preserves caller fetch hooks.
375373
expect(fetchCalls).toHaveLength(2);
376374
expect(fetchCalls[0].url.endsWith("/v3/attachments/att-1")).toBe(true);
377375
expect(fetchCalls[1].url.endsWith("/v3/attachments/att-1/views/original")).toBe(true);
378-
// Verify no pinned undici dispatcher is attached on either request.
379376
for (const call of fetchCalls) {
380377
const init = call.init as RequestInit & { dispatcher?: unknown };
381-
expect(init?.dispatcher).toBeUndefined();
378+
expect(init?.dispatcher).toBeDefined();
382379
}
383380
});
384381

@@ -396,6 +393,7 @@ describe("downloadMSTeamsBotFrameworkAttachment", () => {
396393
tokenProvider: buildTokenProvider(),
397394
maxBytes: 10_000_000,
398395
fetchFn,
396+
fetchFnSupportsDispatcher: true,
399397
resolveFn: resolvePublicHost,
400398
logger,
401399
});
@@ -433,6 +431,7 @@ describe("downloadMSTeamsBotFrameworkAttachment", () => {
433431
tokenProvider: buildTokenProvider(),
434432
maxBytes: 10_000_000,
435433
fetchFn,
434+
fetchFnSupportsDispatcher: true,
436435
resolveFn: resolvePublicHost,
437436
logger,
438437
});

‎extensions/msteams/src/attachments/bot-framework.ts‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -75,23 +75,18 @@ async function fetchBotFrameworkAttachmentInfo(params: {
7575
accessToken: string;
7676
policy: MSTeamsAttachmentFetchPolicy;
7777
fetchFn?: typeof fetch;
78+
fetchFnSupportsDispatcher?: boolean;
7879
resolveFn?: MSTeamsAttachmentResolveFn;
7980
logger?: MSTeamsAttachmentDownloadLogger;
8081
}): Promise<BotFrameworkAttachmentInfo | undefined> {
8182
const url = `${normalizeServiceUrl(params.serviceUrl)}/v3/attachments/${encodeURIComponent(params.attachmentId)}`;
82-
// Use `safeFetchWithPolicy` instead of `fetchWithSsrFGuard`. The strict
83-
// pinned undici dispatcher used by `fetchWithSsrFGuard` is incompatible
84-
// with Node 24+'s built-in undici v7 and silently breaks Bot Framework
85-
// attachment downloads (same root cause as the SharePoint fix in #63396).
86-
// `safeFetchWithPolicy` already enforces hostname allowlist validation
87-
// across every redirect hop, which is sufficient for these attachment
88-
// service URLs.
8983
let response: Response;
9084
try {
9185
response = await safeFetchWithPolicy({
9286
url,
9387
policy: params.policy,
9488
fetchFn: params.fetchFn,
89+
fetchFnSupportsDispatcher: params.fetchFnSupportsDispatcher,
9590
resolveFn: params.resolveFn,
9691
requestInit: {
9792
headers: buildBotFrameworkAttachmentHeaders({
@@ -108,6 +103,7 @@ async function fetchBotFrameworkAttachmentInfo(params: {
108103
return undefined;
109104
}
110105
if (!response.ok) {
106+
await response.body?.cancel();
111107
params.logger?.warn?.("msteams botFramework attachmentInfo non-ok", {
112108
status: response.status,
113109
});
@@ -134,18 +130,18 @@ async function saveBotFrameworkAttachmentView(params: {
134130
preserveFilenames?: boolean;
135131
policy: MSTeamsAttachmentFetchPolicy;
136132
fetchFn?: typeof fetch;
133+
fetchFnSupportsDispatcher?: boolean;
137134
resolveFn?: MSTeamsAttachmentResolveFn;
138135
logger?: MSTeamsAttachmentDownloadLogger;
139136
}): Promise<{ path: string; contentType?: string } | undefined> {
140137
const url = `${normalizeServiceUrl(params.serviceUrl)}/v3/attachments/${encodeURIComponent(params.attachmentId)}/views/${encodeURIComponent(params.viewId)}`;
141-
// See `fetchBotFrameworkAttachmentInfo` for why this uses
142-
// `safeFetchWithPolicy` instead of `fetchWithSsrFGuard` on Node 24+ (#63396).
143138
let response: Response;
144139
try {
145140
response = await safeFetchWithPolicy({
146141
url,
147142
policy: params.policy,
148143
fetchFn: params.fetchFn,
144+
fetchFnSupportsDispatcher: params.fetchFnSupportsDispatcher,
149145
resolveFn: params.resolveFn,
150146
requestInit: {
151147
headers: buildBotFrameworkAttachmentHeaders({
@@ -162,13 +158,15 @@ async function saveBotFrameworkAttachmentView(params: {
162158
return undefined;
163159
}
164160
if (!response.ok) {
161+
await response.body?.cancel();
165162
params.logger?.warn?.("msteams botFramework attachmentView non-ok", {
166163
status: response.status,
167164
});
168165
return undefined;
169166
}
170167
const contentLength = response.headers.get("content-length");
171168
if (contentLength && Number(contentLength) > params.maxBytes) {
169+
await response.body?.cancel();
172170
return undefined;
173171
}
174172
try {
@@ -202,6 +200,7 @@ export async function downloadMSTeamsBotFrameworkAttachment(params: {
202200
allowHosts?: string[];
203201
authAllowHosts?: string[];
204202
fetchFn?: typeof fetch;
203+
fetchFnSupportsDispatcher?: boolean;
205204
resolveFn?: MSTeamsAttachmentResolveFn;
206205
fileNameHint?: string | null;
207206
contentTypeHint?: string | null;
@@ -239,6 +238,7 @@ export async function downloadMSTeamsBotFrameworkAttachment(params: {
239238
accessToken,
240239
policy,
241240
fetchFn: params.fetchFn,
241+
fetchFnSupportsDispatcher: params.fetchFnSupportsDispatcher,
242242
resolveFn: params.resolveFn,
243243
logger: params.logger,
244244
});
@@ -286,6 +286,7 @@ export async function downloadMSTeamsBotFrameworkAttachment(params: {
286286
preserveFilenames: params.preserveFilenames,
287287
policy,
288288
fetchFn: params.fetchFn,
289+
fetchFnSupportsDispatcher: params.fetchFnSupportsDispatcher,
289290
resolveFn: params.resolveFn,
290291
logger: params.logger,
291292
});
@@ -314,6 +315,7 @@ export async function downloadMSTeamsBotFrameworkAttachments(params: {
314315
allowHosts?: string[];
315316
authAllowHosts?: string[];
316317
fetchFn?: typeof fetch;
318+
fetchFnSupportsDispatcher?: boolean;
317319
resolveFn?: MSTeamsAttachmentResolveFn;
318320
fileNameHint?: string | null;
319321
contentTypeHint?: string | null;
@@ -348,6 +350,7 @@ export async function downloadMSTeamsBotFrameworkAttachments(params: {
348350
allowHosts: params.allowHosts,
349351
authAllowHosts: params.authAllowHosts,
350352
fetchFn: params.fetchFn,
353+
fetchFnSupportsDispatcher: params.fetchFnSupportsDispatcher,
351354
resolveFn: params.resolveFn,
352355
fileNameHint: params.fileNameHint,
353356
contentTypeHint: params.contentTypeHint,

‎extensions/msteams/src/attachments/download.ts‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,7 @@ async function fetchWithAuthFallback(params: {
124124
url: string;
125125
tokenProvider?: MSTeamsAccessTokenProvider;
126126
fetchFn?: typeof fetch;
127+
fetchFnSupportsDispatcher?: boolean;
127128
requestInit?: RequestInit;
128129
resolveFn?: MSTeamsAttachmentResolveFn;
129130
policy: MSTeamsAttachmentFetchPolicy;
@@ -132,6 +133,7 @@ async function fetchWithAuthFallback(params: {
132133
url: params.url,
133134
policy: params.policy,
134135
fetchFn: params.fetchFn,
136+
fetchFnSupportsDispatcher: params.fetchFnSupportsDispatcher,
135137
requestInit: params.requestInit,
136138
resolveFn: params.resolveFn,
137139
});
@@ -147,6 +149,7 @@ async function fetchWithAuthFallback(params: {
147149
if (!isUrlAllowed(params.url, params.policy.authAllowHosts)) {
148150
return firstAttempt;
149151
}
152+
await firstAttempt.body?.cancel();
150153

151154
const scopes = scopeCandidatesForUrl(params.url);
152155
const fetchFn = params.fetchFn ?? fetch;
@@ -159,6 +162,7 @@ async function fetchWithAuthFallback(params: {
159162
url: params.url,
160163
policy: params.policy,
161164
fetchFn,
165+
fetchFnSupportsDispatcher: params.fetchFnSupportsDispatcher,
162166
requestInit: {
163167
...params.requestInit,
164168
headers: authHeaders,
@@ -174,8 +178,10 @@ async function fetchWithAuthFallback(params: {
174178
}
175179
if (authAttempt.status !== 401 && authAttempt.status !== 403) {
176180
// Preserve scope fallback semantics for non-auth failures.
181+
await authAttempt.body?.cancel();
177182
continue;
178183
}
184+
await authAttempt.body?.cancel();
179185
} catch {
180186
// Try the next scope.
181187
}
@@ -195,6 +201,7 @@ export async function downloadMSTeamsAttachments(params: {
195201
allowHosts?: string[];
196202
authAllowHosts?: string[];
197203
fetchFn?: typeof fetch;
204+
fetchFnSupportsDispatcher?: boolean;
198205
resolveFn?: MSTeamsAttachmentResolveFn;
199206
/** When true, embeds original filename in stored path for later extraction. */
200207
preserveFilenames?: boolean;
@@ -293,16 +300,15 @@ export async function downloadMSTeamsAttachments(params: {
293300
placeholder: candidate.placeholder,
294301
preserveFilenames: params.preserveFilenames,
295302
ssrfPolicy,
296-
// `fetchImpl` below already validates each hop against the hostname
297-
// allowlist via `safeFetchWithPolicy`, so skip `readRemoteMediaBuffer`'s
298-
// strict SSRF dispatcher (incompatible with Node 24+ / undici v7;
299-
// see issue #63396).
303+
// `fetchImpl` below owns Teams auth fallback and enforces the
304+
// attachment fetch policy through `safeFetchWithPolicy`.
300305
useDirectFetch: true,
301306
fetchImpl: (input, init) =>
302307
fetchWithAuthFallback({
303308
url: resolveRequestUrl(input),
304309
tokenProvider: params.tokenProvider,
305310
fetchFn: params.fetchFn,
311+
fetchFnSupportsDispatcher: params.fetchFnSupportsDispatcher,
306312
requestInit: init,
307313
resolveFn: params.resolveFn,
308314
policy,

‎extensions/msteams/src/attachments/graph.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,7 @@ export async function downloadMSTeamsGraphMedia(params: {
287287
allowHosts?: string[];
288288
authAllowHosts?: string[];
289289
fetchFn?: typeof fetch;
290+
fetchFnSupportsDispatcher?: boolean;
290291
resolveFn?: MSTeamsAttachmentResolveFn;
291292
/** When true, embeds original filename in stored path for later extraction. */
292293
preserveFilenames?: boolean;
@@ -398,6 +399,7 @@ export async function downloadMSTeamsGraphMedia(params: {
398399
url: requestUrl,
399400
policy,
400401
fetchFn,
402+
fetchFnSupportsDispatcher: params.fetchFnSupportsDispatcher,
401403
requestInit: {
402404
...init,
403405
headers,
@@ -468,6 +470,7 @@ export async function downloadMSTeamsGraphMedia(params: {
468470
allowHosts: policy.allowHosts,
469471
authAllowHosts: policy.authAllowHosts,
470472
fetchFn: params.fetchFn,
473+
fetchFnSupportsDispatcher: params.fetchFnSupportsDispatcher,
471474
resolveFn: params.resolveFn,
472475
preserveFilenames: params.preserveFilenames,
473476
logger: params.logger,

‎extensions/msteams/src/attachments/remote-media.ts‎

Lines changed: 5 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,9 @@ import type { MSTeamsInboundMedia } from "./types.js";
77
type FetchLike = (input: RequestInfo | URL, init?: RequestInit) => Promise<Response>;
88

99
/**
10-
* Direct fetch path used when the caller's `fetchImpl` has already validated
11-
* the URL against a hostname allowlist (for example `safeFetchWithPolicy`).
12-
*
13-
* Bypasses the strict SSRF dispatcher on `readRemoteMediaBuffer` because:
14-
* 1. The pinned undici dispatcher used by `readRemoteMediaBuffer` is incompatible
15-
* with Node 24+'s built-in undici v7 (fails with "invalid onRequestStart
16-
* method"), which silently breaks SharePoint/OneDrive downloads. See
17-
* issue #63396.
18-
* 2. SSRF protection is already enforced by the caller's `fetchImpl`
19-
* (`safeFetch` validates every redirect hop against the hostname
20-
* allowlist before following).
10+
* Direct save path used when the caller supplies the already-guarded fetch
11+
* implementation. This lets Teams-specific auth fallback own the request
12+
* sequence while keeping redirect and DNS pinning inside `safeFetchWithPolicy`.
2113
*/
2214
async function saveRemoteMediaDirect(params: {
2315
url: string;
@@ -47,10 +39,8 @@ export async function downloadAndStoreMSTeamsRemoteMedia(params: {
4739
placeholder?: string;
4840
preserveFilenames?: boolean;
4941
/**
50-
* Opt into a direct fetch path that bypasses `readRemoteMediaBuffer`'s strict
51-
* SSRF dispatcher. Required for SharePoint/OneDrive downloads on Node 24+
52-
* (see issue #63396). Only safe when the supplied `fetchImpl` has already
53-
* validated the URL against a hostname allowlist.
42+
* Opt into the Teams-specific guarded fetch path. Only safe when the
43+
* supplied `fetchImpl` enforces the attachment fetch policy itself.
5444
*/
5545
useDirectFetch?: boolean;
5646
}): Promise<MSTeamsInboundMedia> {

0 commit comments

Comments
 (0)