From c582a54554bd1360bb064cd19801af923fc7ab0c Mon Sep 17 00:00:00 2001 From: bmendonca3 <208517100+bmendonca3@users.noreply.github.com> Date: Fri, 27 Feb 2026 12:19:08 -0700 Subject: [PATCH] fix(msteams): preserve guarded dispatcher redirects --- .../msteams/src/attachments/download.ts | 32 ++++--------------- .../msteams/src/attachments/shared.test.ts | 31 +++++++++++++++++- extensions/msteams/src/attachments/shared.ts | 31 ++++++++++++------ 3 files changed, 58 insertions(+), 36 deletions(-) diff --git a/extensions/msteams/src/attachments/download.ts b/extensions/msteams/src/attachments/download.ts index a7444176d7d..cce522d2183 100644 --- a/extensions/msteams/src/attachments/download.ts +++ b/extensions/msteams/src/attachments/download.ts @@ -1,4 +1,3 @@ -import { fetchWithBearerAuthScopeFallback } from "openclaw/plugin-sdk"; import { getMSTeamsRuntime } from "../runtime.js"; import { downloadAndStoreMSTeamsRemoteMedia } from "./remote-media.js"; import { @@ -12,6 +11,7 @@ import { resolveRequestUrl, resolveAuthAllowedHosts, resolveAllowedHosts, + safeFetch, } from "./shared.js"; import type { MSTeamsAccessTokenProvider, @@ -91,16 +91,14 @@ async function fetchWithAuthFallback(params: { tokenProvider?: MSTeamsAccessTokenProvider; fetchFn?: typeof fetch; requestInit?: RequestInit; + allowHosts: string[]; authAllowHosts: string[]; }): Promise { - return await fetchWithBearerAuthScopeFallback({ + const firstAttempt = await safeFetch({ url: params.url, - scopes: scopeCandidatesForUrl(params.url), - tokenProvider: params.tokenProvider, + allowHosts: params.allowHosts, fetchFn: params.fetchFn, requestInit: params.requestInit, - requireHttps: true, - shouldAttachAuth: (url) => isUrlAllowed(url, params.authAllowHosts), }); if (firstAttempt.ok) { return firstAttempt; @@ -116,6 +114,7 @@ async function fetchWithAuthFallback(params: { } const scopes = scopeCandidatesForUrl(params.url); + const fetchFn = params.fetchFn ?? fetch; for (const scope of scopes) { try { const token = await params.tokenProvider.getAccessToken(scope); @@ -129,7 +128,6 @@ async function fetchWithAuthFallback(params: { ...params.requestInit, headers: authHeaders, }, - resolveFn: params.resolveFn, }); if (authAttempt.ok) { return authAttempt; @@ -139,25 +137,6 @@ async function fetchWithAuthFallback(params: { // be handled by the caller's redirect/error policy. return authAttempt; } - - const finalUrl = - typeof authAttempt.url === "string" && authAttempt.url ? authAttempt.url : ""; - if (!finalUrl || finalUrl === params.url || !isUrlAllowed(finalUrl, params.authAllowHosts)) { - continue; - } - const redirectedAuthAttempt = await safeFetch({ - url: finalUrl, - allowHosts: params.allowHosts, - fetchFn, - requestInit: { - ...params.requestInit, - headers: authHeaders, - }, - resolveFn: params.resolveFn, - }); - if (redirectedAuthAttempt.ok) { - return redirectedAuthAttempt; - } } catch { // Try the next scope. } @@ -262,6 +241,7 @@ export async function downloadMSTeamsAttachments(params: { tokenProvider: params.tokenProvider, fetchFn: params.fetchFn, requestInit: init, + allowHosts, authAllowHosts, }), }); diff --git a/extensions/msteams/src/attachments/shared.test.ts b/extensions/msteams/src/attachments/shared.test.ts index 02318cadf5f..4aa1c0e8bab 100644 --- a/extensions/msteams/src/attachments/shared.test.ts +++ b/extensions/msteams/src/attachments/shared.test.ts @@ -1,11 +1,33 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { + isPrivateOrReservedIP, isUrlAllowed, + resolveAndValidateIP, resolveAllowedHosts, resolveAuthAllowedHosts, resolveMediaSsrfPolicy, + safeFetch, } from "./shared.js"; +const publicResolve = async () => ({ address: "13.107.136.10" }); +const privateResolve = (ip: string) => async () => ({ address: ip }); +const failingResolve = async () => { + throw new Error("DNS failure"); +}; + +function mockFetchWithRedirect(redirectMap: Record, finalBody = "ok") { + return vi.fn(async (url: string, init?: RequestInit) => { + const target = redirectMap[url]; + if (target && init?.redirect === "manual") { + return new Response(null, { + status: 302, + headers: { location: target }, + }); + } + return new Response(finalBody, { status: 200 }); + }); +} + describe("msteams attachment allowlists", () => { it("normalizes wildcard host lists", () => { expect(resolveAllowedHosts(["*", "graph.microsoft.com"])).toEqual(["*"]); @@ -19,6 +41,13 @@ describe("msteams attachment allowlists", () => { expect(isUrlAllowed("https://evil.example.com/file.png", allowHosts)).toBe(false); }); + it("builds shared SSRF policy from suffix allowlist", () => { + expect(resolveMediaSsrfPolicy(["sharepoint.com"])).toEqual({ + hostnameAllowlist: ["sharepoint.com", "*.sharepoint.com"], + }); + expect(resolveMediaSsrfPolicy(["*"])).toBeUndefined(); + }); + it.each([ ["999.999.999.999", true], ["256.0.0.1", true], diff --git a/extensions/msteams/src/attachments/shared.ts b/extensions/msteams/src/attachments/shared.ts index 34888ac142c..d78b03136c2 100644 --- a/extensions/msteams/src/attachments/shared.ts +++ b/extensions/msteams/src/attachments/shared.ts @@ -1,6 +1,8 @@ +import { lookup } from "node:dns/promises"; import { buildHostnameAllowlistPolicyFromSuffixAllowlist, isHttpsUrlAllowedByHostnameSuffixAllowlist, + isPrivateIpAddress, normalizeHostnameSuffixAllowlist, } from "openclaw/plugin-sdk"; import type { SsrFPolicy } from "openclaw/plugin-sdk"; @@ -268,6 +270,10 @@ export function isUrlAllowed(url: string, allowlist: string[]): boolean { return isHttpsUrlAllowedByHostnameSuffixAllowlist(url, allowlist); } +export function resolveMediaSsrfPolicy(allowHosts: string[]): SsrFPolicy | undefined { + return buildHostnameAllowlistPolicyFromSuffixAllowlist(allowHosts); +} + /** * Returns true if the given IPv4 or IPv6 address is in a private, loopback, * or link-local range that must never be reached from media downloads. @@ -304,11 +310,11 @@ const MAX_SAFE_REDIRECTS = 5; /** * Fetch a URL with redirect: "manual", validating each redirect target - * against the hostname allowlist and DNS-resolved IP (anti-SSRF). + * against the hostname allowlist and optional DNS-resolved IP (anti-SSRF). * * This prevents: * - Auto-following redirects to non-allowlisted hosts - * - DNS rebinding attacks where an allowlisted domain resolves to a private IP + * - DNS rebinding attacks when a lookup function is provided */ export async function safeFetch(params: { url: string; @@ -326,14 +332,19 @@ export async function safeFetch(params: { ); let currentUrl = params.url; - // Validate the initial URL's resolved IP - try { - const initialHost = new URL(currentUrl).hostname; - await resolveAndValidateIP(initialHost, resolveFn); - } catch { + if (!isUrlAllowed(currentUrl, params.allowHosts)) { throw new Error(`Initial download URL blocked: ${currentUrl}`); } + if (resolveFn) { + try { + const initialHost = new URL(currentUrl).hostname; + await resolveAndValidateIP(initialHost, resolveFn); + } catch { + throw new Error(`Initial download URL blocked: ${currentUrl}`); + } + } + for (let i = 0; i <= MAX_SAFE_REDIRECTS; i++) { const res = await fetchFn(currentUrl, { ...params.requestInit, @@ -369,8 +380,10 @@ export async function safeFetch(params: { } // Validate redirect target's resolved IP - const redirectHost = new URL(redirectUrl).hostname; - await resolveAndValidateIP(redirectHost, resolveFn); + if (resolveFn) { + const redirectHost = new URL(redirectUrl).hostname; + await resolveAndValidateIP(redirectHost, resolveFn); + } currentUrl = redirectUrl; }