Skip to content

Commit 866bd91

Browse files
steipetesteipete
committed
refactor: harden msteams lifecycle and attachment flows
1 parent d98a61a commit 866bd91

15 files changed

+459
-112
lines changed

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

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,12 @@ import {
66
isDownloadableAttachment,
77
isRecord,
88
isUrlAllowed,
9+
type MSTeamsAttachmentFetchPolicy,
910
normalizeContentType,
1011
resolveMediaSsrfPolicy,
12+
resolveAttachmentFetchPolicy,
1113
resolveRequestUrl,
12-
resolveAuthAllowedHosts,
13-
resolveAllowedHosts,
14-
safeFetch,
14+
safeFetchWithPolicy,
1515
} from "./shared.js";
1616
import type {
1717
MSTeamsAccessTokenProvider,
@@ -95,12 +95,11 @@ async function fetchWithAuthFallback(params: {
9595
tokenProvider?: MSTeamsAccessTokenProvider;
9696
fetchFn?: typeof fetch;
9797
requestInit?: RequestInit;
98-
allowHosts: string[];
99-
authAllowHosts: string[];
98+
policy: MSTeamsAttachmentFetchPolicy;
10099
}): Promise<Response> {
101-
const firstAttempt = await safeFetch({
100+
const firstAttempt = await safeFetchWithPolicy({
102101
url: params.url,
103-
allowHosts: params.allowHosts,
102+
policy: params.policy,
104103
fetchFn: params.fetchFn,
105104
requestInit: params.requestInit,
106105
});
@@ -113,7 +112,7 @@ async function fetchWithAuthFallback(params: {
113112
if (firstAttempt.status !== 401 && firstAttempt.status !== 403) {
114113
return firstAttempt;
115114
}
116-
if (!isUrlAllowed(params.url, params.authAllowHosts)) {
115+
if (!isUrlAllowed(params.url, params.policy.authAllowHosts)) {
117116
return firstAttempt;
118117
}
119118

@@ -124,10 +123,9 @@ async function fetchWithAuthFallback(params: {
124123
const token = await params.tokenProvider.getAccessToken(scope);
125124
const authHeaders = new Headers(params.requestInit?.headers);
126125
authHeaders.set("Authorization", `Bearer ${token}`);
127-
const authAttempt = await safeFetch({
126+
const authAttempt = await safeFetchWithPolicy({
128127
url: params.url,
129-
allowHosts: params.allowHosts,
130-
authorizationAllowHosts: params.authAllowHosts,
128+
policy: params.policy,
131129
fetchFn,
132130
requestInit: {
133131
...params.requestInit,
@@ -171,8 +169,11 @@ export async function downloadMSTeamsAttachments(params: {
171169
if (list.length === 0) {
172170
return [];
173171
}
174-
const allowHosts = resolveAllowedHosts(params.allowHosts);
175-
const authAllowHosts = resolveAuthAllowedHosts(params.authAllowHosts);
172+
const policy = resolveAttachmentFetchPolicy({
173+
allowHosts: params.allowHosts,
174+
authAllowHosts: params.authAllowHosts,
175+
});
176+
const allowHosts = policy.allowHosts;
176177
const ssrfPolicy = resolveMediaSsrfPolicy(allowHosts);
177178

178179
// Download ANY downloadable attachment (not just images)
@@ -249,8 +250,7 @@ export async function downloadMSTeamsAttachments(params: {
249250
tokenProvider: params.tokenProvider,
250251
fetchFn: params.fetchFn,
251252
requestInit: init,
252-
allowHosts,
253-
authAllowHosts,
253+
policy,
254254
}),
255255
});
256256
out.push(media);

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

Lines changed: 20 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3,16 +3,17 @@ import { getMSTeamsRuntime } from "../runtime.js";
33
import { downloadMSTeamsAttachments } from "./download.js";
44
import { downloadAndStoreMSTeamsRemoteMedia } from "./remote-media.js";
55
import {
6+
applyAuthorizationHeaderForUrl,
67
GRAPH_ROOT,
78
inferPlaceholder,
89
isRecord,
910
isUrlAllowed,
11+
type MSTeamsAttachmentFetchPolicy,
1012
normalizeContentType,
1113
resolveMediaSsrfPolicy,
14+
resolveAttachmentFetchPolicy,
1215
resolveRequestUrl,
13-
resolveAuthAllowedHosts,
14-
resolveAllowedHosts,
15-
safeFetch,
16+
safeFetchWithPolicy,
1617
} from "./shared.js";
1718
import type {
1819
MSTeamsAccessTokenProvider,
@@ -243,9 +244,11 @@ export async function downloadMSTeamsGraphMedia(params: {
243244
if (!params.messageUrl || !params.tokenProvider) {
244245
return { media: [] };
245246
}
246-
const allowHosts = resolveAllowedHosts(params.allowHosts);
247-
const authAllowHosts = resolveAuthAllowedHosts(params.authAllowHosts);
248-
const ssrfPolicy = resolveMediaSsrfPolicy(allowHosts);
247+
const policy: MSTeamsAttachmentFetchPolicy = resolveAttachmentFetchPolicy({
248+
allowHosts: params.allowHosts,
249+
authAllowHosts: params.authAllowHosts,
250+
});
251+
const ssrfPolicy = resolveMediaSsrfPolicy(policy.allowHosts);
249252
const messageUrl = params.messageUrl;
250253
let accessToken: string;
251254
try {
@@ -291,7 +294,7 @@ export async function downloadMSTeamsGraphMedia(params: {
291294
try {
292295
// SharePoint URLs need to be accessed via Graph shares API
293296
const shareUrl = att.contentUrl!;
294-
if (!isUrlAllowed(shareUrl, allowHosts)) {
297+
if (!isUrlAllowed(shareUrl, policy.allowHosts)) {
295298
continue;
296299
}
297300
const encodedUrl = Buffer.from(shareUrl).toString("base64url");
@@ -307,15 +310,15 @@ export async function downloadMSTeamsGraphMedia(params: {
307310
fetchImpl: async (input, init) => {
308311
const requestUrl = resolveRequestUrl(input);
309312
const headers = new Headers(init?.headers);
310-
if (isUrlAllowed(requestUrl, authAllowHosts)) {
311-
headers.set("Authorization", `Bearer ${accessToken}`);
312-
} else {
313-
headers.delete("Authorization");
314-
}
315-
return await safeFetch({
313+
applyAuthorizationHeaderForUrl({
314+
headers,
315+
url: requestUrl,
316+
authAllowHosts: policy.authAllowHosts,
317+
bearerToken: accessToken,
318+
});
319+
return await safeFetchWithPolicy({
316320
url: requestUrl,
317-
allowHosts,
318-
authorizationAllowHosts: authAllowHosts,
321+
policy,
319322
fetchFn,
320323
requestInit: {
321324
...init,
@@ -373,8 +376,8 @@ export async function downloadMSTeamsGraphMedia(params: {
373376
attachments: filteredAttachments,
374377
maxBytes: params.maxBytes,
375378
tokenProvider: params.tokenProvider,
376-
allowHosts,
377-
authAllowHosts,
379+
allowHosts: policy.allowHosts,
380+
authAllowHosts: policy.authAllowHosts,
378381
fetchFn: params.fetchFn,
379382
preserveFilenames: params.preserveFilenames,
380383
});

‎extensions/msteams/src/attachments/shared.test.ts‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,15 @@
11
import { describe, expect, it, vi } from "vitest";
22
import {
3+
applyAuthorizationHeaderForUrl,
34
isPrivateOrReservedIP,
45
isUrlAllowed,
56
resolveAndValidateIP,
7+
resolveAttachmentFetchPolicy,
68
resolveAllowedHosts,
79
resolveAuthAllowedHosts,
810
resolveMediaSsrfPolicy,
911
safeFetch,
12+
safeFetchWithPolicy,
1013
} from "./shared.js";
1114

1215
const publicResolve = async () => ({ address: "13.107.136.10" });
@@ -34,6 +37,18 @@ describe("msteams attachment allowlists", () => {
3437
expect(resolveAuthAllowedHosts(["*", "graph.microsoft.com"])).toEqual(["*"]);
3538
});
3639

40+
it("resolves a normalized attachment fetch policy", () => {
41+
expect(
42+
resolveAttachmentFetchPolicy({
43+
allowHosts: ["sharepoint.com"],
44+
authAllowHosts: ["graph.microsoft.com"],
45+
}),
46+
).toEqual({
47+
allowHosts: ["sharepoint.com"],
48+
authAllowHosts: ["graph.microsoft.com"],
49+
});
50+
});
51+
3752
it("requires https and host suffix match", () => {
3853
const allowHosts = resolveAllowedHosts(["sharepoint.com"]);
3954
expect(isUrlAllowed("https://contoso.sharepoint.com/file.png", allowHosts)).toBe(true);
@@ -294,4 +309,70 @@ describe("safeFetch", () => {
294309
}),
295310
).rejects.toThrow("blocked by allowlist");
296311
});
312+
313+
it("strips authorization across redirects outside auth allowlist", async () => {
314+
const seenAuth: string[] = [];
315+
const fetchMock = vi.fn(async (url: string, init?: RequestInit) => {
316+
const auth = new Headers(init?.headers).get("authorization") ?? "";
317+
seenAuth.push(`${url}|${auth}`);
318+
if (url === "https://teams.sharepoint.com/file.pdf") {
319+
return new Response(null, {
320+
status: 302,
321+
headers: { location: "https://cdn.sharepoint.com/storage/file.pdf" },
322+
});
323+
}
324+
return new Response("ok", { status: 200 });
325+
});
326+
327+
const headers = new Headers({ Authorization: "Bearer secret" });
328+
const res = await safeFetch({
329+
url: "https://teams.sharepoint.com/file.pdf",
330+
allowHosts: ["sharepoint.com"],
331+
authorizationAllowHosts: ["graph.microsoft.com"],
332+
fetchFn: fetchMock as unknown as typeof fetch,
333+
requestInit: { headers },
334+
resolveFn: publicResolve,
335+
});
336+
expect(res.status).toBe(200);
337+
expect(seenAuth[0]).toContain("Bearer secret");
338+
expect(seenAuth[1]).toMatch(/\|$/);
339+
});
340+
});
341+
342+
describe("attachment fetch auth helpers", () => {
343+
it("sets and clears authorization header by auth allowlist", () => {
344+
const headers = new Headers();
345+
applyAuthorizationHeaderForUrl({
346+
headers,
347+
url: "https://graph.microsoft.com/v1.0/me",
348+
authAllowHosts: ["graph.microsoft.com"],
349+
bearerToken: "token-1",
350+
});
351+
expect(headers.get("authorization")).toBe("Bearer token-1");
352+
353+
applyAuthorizationHeaderForUrl({
354+
headers,
355+
url: "https://evil.example.com/collect",
356+
authAllowHosts: ["graph.microsoft.com"],
357+
bearerToken: "token-1",
358+
});
359+
expect(headers.get("authorization")).toBeNull();
360+
});
361+
362+
it("safeFetchWithPolicy forwards policy allowlists", async () => {
363+
const fetchMock = vi.fn(async (_url: string, _init?: RequestInit) => {
364+
return new Response("ok", { status: 200 });
365+
});
366+
const res = await safeFetchWithPolicy({
367+
url: "https://teams.sharepoint.com/file.pdf",
368+
policy: resolveAttachmentFetchPolicy({
369+
allowHosts: ["sharepoint.com"],
370+
authAllowHosts: ["graph.microsoft.com"],
371+
}),
372+
fetchFn: fetchMock as unknown as typeof fetch,
373+
resolveFn: publicResolve,
374+
});
375+
expect(res.status).toBe(200);
376+
expect(fetchMock).toHaveBeenCalledOnce();
377+
});
297378
});

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

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,10 +266,42 @@ export function resolveAuthAllowedHosts(input?: string[]): string[] {
266266
return normalizeHostnameSuffixAllowlist(input, DEFAULT_MEDIA_AUTH_HOST_ALLOWLIST);
267267
}
268268

269+
export type MSTeamsAttachmentFetchPolicy = {
270+
allowHosts: string[];
271+
authAllowHosts: string[];
272+
};
273+
274+
export function resolveAttachmentFetchPolicy(params?: {
275+
allowHosts?: string[];
276+
authAllowHosts?: string[];
277+
}): MSTeamsAttachmentFetchPolicy {
278+
return {
279+
allowHosts: resolveAllowedHosts(params?.allowHosts),
280+
authAllowHosts: resolveAuthAllowedHosts(params?.authAllowHosts),
281+
};
282+
}
283+
269284
export function isUrlAllowed(url: string, allowlist: string[]): boolean {
270285
return isHttpsUrlAllowedByHostnameSuffixAllowlist(url, allowlist);
271286
}
272287

288+
export function applyAuthorizationHeaderForUrl(params: {
289+
headers: Headers;
290+
url: string;
291+
authAllowHosts: string[];
292+
bearerToken?: string;
293+
}): void {
294+
if (!params.bearerToken) {
295+
params.headers.delete("Authorization");
296+
return;
297+
}
298+
if (isUrlAllowed(params.url, params.authAllowHosts)) {
299+
params.headers.set("Authorization", `Bearer ${params.bearerToken}`);
300+
return;
301+
}
302+
params.headers.delete("Authorization");
303+
}
304+
273305
export function resolveMediaSsrfPolicy(allowHosts: string[]): SsrFPolicy | undefined {
274306
return buildHostnameAllowlistPolicyFromSuffixAllowlist(allowHosts);
275307
}
@@ -408,3 +440,20 @@ export async function safeFetch(params: {
408440

409441
throw new Error(`Too many redirects (>${MAX_SAFE_REDIRECTS})`);
410442
}
443+
444+
export async function safeFetchWithPolicy(params: {
445+
url: string;
446+
policy: MSTeamsAttachmentFetchPolicy;
447+
fetchFn?: typeof fetch;
448+
requestInit?: RequestInit;
449+
resolveFn?: (hostname: string) => Promise<{ address: string }>;
450+
}): Promise<Response> {
451+
return await safeFetch({
452+
url: params.url,
453+
allowHosts: params.policy.allowHosts,
454+
authorizationAllowHosts: params.policy.authAllowHosts,
455+
fetchFn: params.fetchFn,
456+
requestInit: params.requestInit,
457+
resolveFn: params.resolveFn,
458+
});
459+
}

0 commit comments

Comments
 (0)