mirror of
https://github.com/openclaw/openclaw.git
synced 2026-03-12 07:20:45 +00:00
fix(msteams): scope auth across media redirects
This commit is contained in:
committed by
Peter Steinberger
parent
da22a9113c
commit
4a414c5e53
@@ -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<string>) = "token",
|
||||
) => ({
|
||||
getAccessToken: vi.fn(async (scope: string) =>
|
||||
typeof tokenOrResolver === "function" ? await tokenOrResolver(scope) : tokenOrResolver,
|
||||
),
|
||||
});
|
||||
const asSingleItemArray = <T>(value: T) => [value];
|
||||
const withLabel = <T extends object>(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(
|
||||
|
||||
@@ -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.
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
});
|
||||
|
||||
@@ -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<string, unknown>),
|
||||
);
|
||||
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.
|
||||
|
||||
Reference in New Issue
Block a user