diff --git a/extensions/msteams/src/attachments.test.ts b/extensions/msteams/src/attachments.test.ts index 3ba1c3a971f..815659fbdb7 100644 --- a/extensions/msteams/src/attachments.test.ts +++ b/extensions/msteams/src/attachments.test.ts @@ -164,8 +164,12 @@ const IMAGE_ATTACHMENT = { contentType: CONTENT_TYPE_IMAGE_PNG, contentUrl: TEST const PNG_BUFFER = Buffer.from("png"); const PNG_BASE64 = PNG_BUFFER.toString("base64"); const PDF_BUFFER = Buffer.from("pdf"); -const createTokenProvider = (token = "token") => ({ - getAccessToken: vi.fn(async () => token), +const createTokenProvider = ( + tokenOrResolver: string | ((scope: string) => string | Promise) = "token", +) => ({ + getAccessToken: vi.fn(async (scope: string) => + typeof tokenOrResolver === "function" ? await tokenOrResolver(scope) : tokenOrResolver, + ), }); const asSingleItemArray = (value: T) => [value]; const withLabel = (label: string, fields: T): T & LabeledCase => ({ @@ -744,6 +748,73 @@ describe("msteams attachments", () => { expect(fetchMock.mock.calls.map(([calledUrl]) => String(calledUrl))).toContain(redirectedUrl); }); + it("continues scope fallback after non-auth failure and succeeds on later scope", async () => { + let authAttempt = 0; + const tokenProvider = createTokenProvider((scope) => `token:${scope}`); + const fetchMock = vi.fn(async (_url: string, opts?: RequestInit) => { + const auth = new Headers(opts?.headers).get("Authorization"); + if (!auth) { + return createTextResponse("unauthorized", 401); + } + authAttempt += 1; + if (authAttempt === 1) { + return createTextResponse("upstream transient", 500); + } + return createBufferResponse(PNG_BUFFER, CONTENT_TYPE_IMAGE_PNG); + }); + + const media = await downloadAttachmentsWithFetch( + createImageAttachments(TEST_URL_IMAGE), + fetchMock, + { tokenProvider, authAllowHosts: [TEST_HOST] }, + ); + + expectAttachmentMediaLength(media, 1); + expect(tokenProvider.getAccessToken).toHaveBeenCalledTimes(2); + }); + + it("does not forward Authorization to redirects outside auth allowlist", async () => { + const tokenProvider = createTokenProvider("top-secret-token"); + const graphFileUrl = createUrlForHost(GRAPH_HOST, "file"); + const seen: Array<{ url: string; auth: string }> = []; + const fetchMock = vi.fn(async (url: string, opts?: RequestInit) => { + const auth = new Headers(opts?.headers).get("Authorization") ?? ""; + seen.push({ url, auth }); + if (url === graphFileUrl && !auth) { + return new Response("unauthorized", { status: 401 }); + } + if (url === graphFileUrl && auth) { + return new Response("", { + status: 302, + headers: { location: "https://attacker.azureedge.net/collect" }, + }); + } + if (url === "https://attacker.azureedge.net/collect") { + return new Response(Buffer.from("png"), { + status: 200, + headers: { "content-type": CONTENT_TYPE_IMAGE_PNG }, + }); + } + return createNotFoundResponse(); + }); + + const media = await downloadMSTeamsAttachments( + buildDownloadParams([{ contentType: CONTENT_TYPE_IMAGE_PNG, contentUrl: graphFileUrl }], { + tokenProvider, + allowHosts: [GRAPH_HOST, AZUREEDGE_HOST], + authAllowHosts: [GRAPH_HOST], + fetchFn: asFetchFn(fetchMock), + }), + ); + + expectSingleMedia(media); + const redirected = seen.find( + (entry) => entry.url === "https://attacker.azureedge.net/collect", + ); + expect(redirected).toBeDefined(); + expect(redirected?.auth).toBe(""); + }); + it("skips urls outside the allowlist", async () => { const fetchMock = vi.fn(); const media = await downloadAttachmentsWithFetch( diff --git a/extensions/msteams/src/attachments/download.ts b/extensions/msteams/src/attachments/download.ts index cce522d2183..f40de08ece6 100644 --- a/extensions/msteams/src/attachments/download.ts +++ b/extensions/msteams/src/attachments/download.ts @@ -86,6 +86,10 @@ function scopeCandidatesForUrl(url: string): string[] { } } +function isRedirectStatus(status: number): boolean { + return status === 301 || status === 302 || status === 303 || status === 307 || status === 308; +} + async function fetchWithAuthFallback(params: { url: string; tokenProvider?: MSTeamsAccessTokenProvider; @@ -123,6 +127,7 @@ async function fetchWithAuthFallback(params: { const authAttempt = await safeFetch({ url: params.url, allowHosts: params.allowHosts, + authorizationAllowHosts: params.authAllowHosts, fetchFn, requestInit: { ...params.requestInit, @@ -132,11 +137,14 @@ async function fetchWithAuthFallback(params: { if (authAttempt.ok) { return authAttempt; } - if (authAttempt.status !== 401 && authAttempt.status !== 403) { - // Non-auth failures (including redirects in guarded fetch mode) should - // be handled by the caller's redirect/error policy. + if (isRedirectStatus(authAttempt.status)) { + // Redirects in guarded fetch mode must propagate to the outer guard. return authAttempt; } + if (authAttempt.status !== 401 && authAttempt.status !== 403) { + // Preserve scope fallback semantics for non-auth failures. + continue; + } } catch { // Try the next scope. } diff --git a/extensions/msteams/src/attachments/graph.ts b/extensions/msteams/src/attachments/graph.ts index 3a026680dac..f921a2cfa04 100644 --- a/extensions/msteams/src/attachments/graph.ts +++ b/extensions/msteams/src/attachments/graph.ts @@ -10,7 +10,9 @@ import { normalizeContentType, resolveMediaSsrfPolicy, resolveRequestUrl, + resolveAuthAllowedHosts, resolveAllowedHosts, + safeFetch, } from "./shared.js"; import type { MSTeamsAccessTokenProvider, @@ -242,6 +244,7 @@ export async function downloadMSTeamsGraphMedia(params: { return { media: [] }; } const allowHosts = resolveAllowedHosts(params.allowHosts); + const authAllowHosts = resolveAuthAllowedHosts(params.authAllowHosts); const ssrfPolicy = resolveMediaSsrfPolicy(allowHosts); const messageUrl = params.messageUrl; let accessToken: string; @@ -304,8 +307,21 @@ export async function downloadMSTeamsGraphMedia(params: { fetchImpl: async (input, init) => { const requestUrl = resolveRequestUrl(input); const headers = new Headers(init?.headers); - headers.set("Authorization", `Bearer ${accessToken}`); - return await fetchFn(requestUrl, { ...init, headers }); + if (isUrlAllowed(requestUrl, authAllowHosts)) { + headers.set("Authorization", `Bearer ${accessToken}`); + } else { + headers.delete("Authorization"); + } + return await safeFetch({ + url: requestUrl, + allowHosts, + authorizationAllowHosts: authAllowHosts, + fetchFn, + requestInit: { + ...init, + headers, + }, + }); }, }); sharePointMedia.push(media); @@ -313,35 +329,6 @@ export async function downloadMSTeamsGraphMedia(params: { } catch { // Ignore SharePoint download failures. } - const encodedUrl = Buffer.from(shareUrl).toString("base64url"); - const sharesUrl = `${GRAPH_ROOT}/shares/u!${encodedUrl}/driveItem/content`; - - const media = await downloadAndStoreMSTeamsRemoteMedia({ - url: sharesUrl, - filePathHint: name, - maxBytes: params.maxBytes, - contentTypeHint: "application/octet-stream", - preserveFilenames: params.preserveFilenames, - fetchImpl: async (input, init) => { - const requestUrl = resolveRequestUrl(input); - const headers = new Headers(init?.headers); - headers.set("Authorization", `Bearer ${accessToken}`); - return await safeFetch({ - url: requestUrl, - allowHosts, - authorizationAllowHosts: params.authAllowHosts, - fetchFn, - requestInit: { - ...init, - headers, - }, - }); - }, - }); - sharePointMedia.push(media); - downloadedReferenceUrls.add(shareUrl); - } catch { - // Ignore SharePoint download failures. } } } finally { @@ -387,7 +374,7 @@ export async function downloadMSTeamsGraphMedia(params: { maxBytes: params.maxBytes, tokenProvider: params.tokenProvider, allowHosts, - authAllowHosts: params.authAllowHosts, + authAllowHosts, fetchFn: params.fetchFn, preserveFilenames: params.preserveFilenames, }); diff --git a/extensions/msteams/src/attachments/shared.ts b/extensions/msteams/src/attachments/shared.ts index d78b03136c2..88ff64970b6 100644 --- a/extensions/msteams/src/attachments/shared.ts +++ b/extensions/msteams/src/attachments/shared.ts @@ -319,6 +319,12 @@ const MAX_SAFE_REDIRECTS = 5; export async function safeFetch(params: { url: string; allowHosts: string[]; + /** + * Optional allowlist for forwarding Authorization across redirects. + * When set, Authorization is stripped before following redirects to hosts + * outside this list. + */ + authorizationAllowHosts?: string[]; fetchFn?: typeof fetch; requestInit?: RequestInit; resolveFn?: (hostname: string) => Promise<{ address: string }>; @@ -330,6 +336,7 @@ export async function safeFetch(params: { typeof params.requestInit === "object" && "dispatcher" in (params.requestInit as Record), ); + const currentHeaders = new Headers(params.requestInit?.headers); let currentUrl = params.url; if (!isUrlAllowed(currentUrl, params.allowHosts)) { @@ -348,6 +355,7 @@ export async function safeFetch(params: { for (let i = 0; i <= MAX_SAFE_REDIRECTS; i++) { const res = await fetchFn(currentUrl, { ...params.requestInit, + headers: currentHeaders, redirect: "manual", }); @@ -372,6 +380,16 @@ export async function safeFetch(params: { throw new Error(`Media redirect target blocked by allowlist: ${redirectUrl}`); } + // Prevent credential bleed: only keep Authorization on redirect hops that + // are explicitly auth-allowlisted. + if ( + currentHeaders.has("authorization") && + params.authorizationAllowHosts && + !isUrlAllowed(redirectUrl, params.authorizationAllowHosts) + ) { + currentHeaders.delete("authorization"); + } + // When a pinned dispatcher is already injected by an upstream guard // (for example fetchWithSsrFGuard), let that guard own redirect handling // after this allowlist validation step.