From 55cbcd829d387ccd13202dcc5688950e93d59b8b Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Wed, 8 Apr 2026 08:11:27 +0100 Subject: [PATCH] fix(slack): preserve auth on same-origin media redirects (#62996) (thanks @vincentkoc) - Verified: pnpm build\n- Verified: pnpm test extensions/slack/src/monitor/media.test.ts\n- Verified: pnpm exec oxlint extensions/slack/src/monitor/media.ts extensions/slack/src/monitor/media.test.ts\n- Verified: pnpm exec oxfmt --check extensions/slack/src/monitor/media.ts extensions/slack/src/monitor/media.test.ts CHANGELOG.md\n\nRepo-wide pnpm lint and pnpm test were not clean on current main outside this fix, and the first full-suite test attempt from the default core sparse profile was additionally contaminated by missing ui/packages/OpenClawKit paths until they were materialized. --- CHANGELOG.md | 4 + extensions/slack/src/monitor/media.test.ts | 91 ++++++++++++++++++++-- extensions/slack/src/monitor/media.ts | 76 +++++++++--------- 3 files changed, 129 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e6814c3f4c2..018812d44e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ Docs: https://docs.openclaw.ai ## Unreleased +### Fixes + +- Slack/media: preserve bearer auth across same-origin `files.slack.com` redirects while still stripping it on cross-origin Slack CDN hops, so `url_private_download` image attachments load again. (#62960) Thanks @vincentkoc. + ## 2026.4.8 ### Fixes diff --git a/extensions/slack/src/monitor/media.test.ts b/extensions/slack/src/monitor/media.test.ts index 705a74f7d00..0cc0df0acd8 100644 --- a/extensions/slack/src/monitor/media.test.ts +++ b/extensions/slack/src/monitor/media.test.ts @@ -22,6 +22,11 @@ const createSavedMedia = (filePath: string, contentType: string): SavedMedia => contentType, }); +function getRequestHeader(callIndex: number, headerName: string): string | null { + const init = mockFetch.mock.calls[callIndex]?.[1]; + return new Headers(init?.headers).get(headerName); +} + describe("fetchWithSlackAuth", () => { beforeEach(() => { // Create a new mock for each test @@ -65,7 +70,7 @@ describe("fetchWithSlackAuth", () => { expect(mockFetch).not.toHaveBeenCalled(); }); - it("follows redirects without Authorization header", async () => { + it("strips Authorization header on cross-origin redirects", async () => { // First call: redirect response from Slack const redirectResponse = new Response(null, { status: 302, @@ -99,8 +104,7 @@ describe("fetchWithSlackAuth", () => { ); }); - it("handles relative redirect URLs", async () => { - // Redirect with relative URL + it("preserves Authorization header on same-origin redirects", async () => { const redirectResponse = new Response(null, { status: 302, headers: { location: "/files/redirect-target" }, @@ -115,8 +119,8 @@ describe("fetchWithSlackAuth", () => { await fetchWithSlackAuth("https://files.slack.com/original.jpg", "xoxb-test-token"); - // Second call should resolve the relative URL against the original expect(mockFetch).toHaveBeenNthCalledWith(2, "https://files.slack.com/files/redirect-target", { + headers: { Authorization: "Bearer xoxb-test-token" }, redirect: "follow", }); }); @@ -212,6 +216,74 @@ describe("resolveSlackMedia", () => { ); }); + it("preserves Authorization on same-origin redirects for private downloads", async () => { + vi.spyOn(mediaStore, "saveMediaBuffer").mockResolvedValue( + createSavedMedia("/tmp/test.jpg", "image/jpeg"), + ); + + mockFetch + .mockResolvedValueOnce( + new Response(null, { + status: 302, + headers: { location: "/files/redirect-target" }, + }), + ) + .mockResolvedValueOnce( + new Response(Buffer.from("image data"), { + status: 200, + headers: { "content-type": "image/jpeg" }, + }), + ); + + const result = await resolveSlackMedia({ + files: [{ url_private_download: "https://files.slack.com/download.jpg", name: "test.jpg" }], + token: "xoxb-test-token", + maxBytes: 1024 * 1024, + }); + + expect(result).not.toBeNull(); + expect(mockFetch).toHaveBeenCalledTimes(2); + expect(mockFetch.mock.calls[0]?.[0]).toBe("https://files.slack.com/download.jpg"); + expect(mockFetch.mock.calls[1]?.[0]).toBe("https://files.slack.com/files/redirect-target"); + expect(getRequestHeader(0, "Authorization")).toBe("Bearer xoxb-test-token"); + expect(getRequestHeader(1, "Authorization")).toBe("Bearer xoxb-test-token"); + }); + + it("strips Authorization on cross-origin redirects for private downloads", async () => { + vi.spyOn(mediaStore, "saveMediaBuffer").mockResolvedValue( + createSavedMedia("/tmp/test.jpg", "image/jpeg"), + ); + + mockFetch + .mockResolvedValueOnce( + new Response(null, { + status: 302, + headers: { location: "https://downloads.slack-edge.com/presigned-url?sig=abc123" }, + }), + ) + .mockResolvedValueOnce( + new Response(Buffer.from("image data"), { + status: 200, + headers: { "content-type": "image/jpeg" }, + }), + ); + + const result = await resolveSlackMedia({ + files: [{ url_private_download: "https://files.slack.com/download.jpg", name: "test.jpg" }], + token: "xoxb-test-token", + maxBytes: 1024 * 1024, + }); + + expect(result).not.toBeNull(); + expect(mockFetch).toHaveBeenCalledTimes(2); + expect(mockFetch.mock.calls[0]?.[0]).toBe("https://files.slack.com/download.jpg"); + expect(mockFetch.mock.calls[1]?.[0]).toBe( + "https://downloads.slack-edge.com/presigned-url?sig=abc123", + ); + expect(getRequestHeader(0, "Authorization")).toBe("Bearer xoxb-test-token"); + expect(getRequestHeader(1, "Authorization")).toBeNull(); + }); + it("returns null when download fails", async () => { // Simulate a network error mockFetch.mockRejectedValueOnce(new Error("Network error")); @@ -481,9 +553,7 @@ describe("resolveSlackMedia", () => { }) as typeof fetch; const runtimeFetchSpy = vi .spyOn(ssrf, "fetchWithRuntimeDispatcher") - .mockImplementation(async (_input: RequestInfo | URL, init?: RequestInit) => { - expect(init).toMatchObject({ redirect: "manual" }); - expect(init && "dispatcher" in init).toBe(true); + .mockImplementation(async () => { return new Response(Buffer.from("image data"), { status: 200, headers: { "content-type": "image/jpeg" }, @@ -498,6 +568,13 @@ describe("resolveSlackMedia", () => { expect(result).not.toBeNull(); expect(runtimeFetchSpy).toHaveBeenCalled(); + expect(runtimeFetchSpy.mock.calls[0]?.[1]).toMatchObject({ redirect: "manual" }); + expect( + runtimeFetchSpy.mock.calls[0]?.[1] && "dispatcher" in runtimeFetchSpy.mock.calls[0][1], + ).toBe(true); + expect(new Headers(runtimeFetchSpy.mock.calls[0]?.[1]?.headers).get("Authorization")).toBe( + "Bearer xoxb-test-token", + ); }); }); diff --git a/extensions/slack/src/monitor/media.ts b/extensions/slack/src/monitor/media.ts index 40882e932c8..42f287f1883 100644 --- a/extensions/slack/src/monitor/media.ts +++ b/extensions/slack/src/monitor/media.ts @@ -43,6 +43,26 @@ function assertSlackFileUrl(rawUrl: string): URL { return parsed; } +function createSlackAuthHeaders(token: string): HeadersInit { + return { Authorization: `Bearer ${token}` }; +} + +function createSlackMediaRequest( + url: string, + token: string, +): { + url: string; + requestInit: RequestInit; +} { + const parsed = assertSlackFileUrl(url); + return { + url: parsed.href, + // Let the shared guarded-fetch redirect logic preserve auth on same-origin + // Slack hops and strip it once the redirect crosses origins. + requestInit: { headers: createSlackAuthHeaders(token) }, + }; +} + function isMockedFetch(fetchImpl: typeof fetch | undefined): boolean { if (typeof fetchImpl !== "function") { return false; @@ -50,68 +70,53 @@ function isMockedFetch(fetchImpl: typeof fetch | undefined): boolean { return typeof (fetchImpl as typeof fetch & { mock?: unknown }).mock === "object"; } -function createSlackMediaFetch(token: string): FetchLike { - let includeAuth = true; +function createSlackMediaFetch(): FetchLike { return async (input, init) => { const url = resolveRequestUrl(input); if (!url) { throw new Error("Unsupported fetch input: expected string, URL, or Request"); } - const { headers: initHeaders, redirect: _redirect, ...rest } = init ?? {}; - const headers = new Headers(initHeaders); + const parsed = assertSlackFileUrl(url); const fetchImpl = "dispatcher" in (init ?? {}) && !isMockedFetch(globalThis.fetch) ? fetchWithRuntimeDispatcher : globalThis.fetch; - - if (includeAuth) { - includeAuth = false; - const parsed = assertSlackFileUrl(url); - headers.set("Authorization", `Bearer ${token}`); - return fetchImpl(parsed.href, { ...rest, headers, redirect: "manual" }); - } - - headers.delete("Authorization"); - return fetchImpl(url, { ...rest, headers, redirect: "manual" }); + return fetchImpl(parsed.href, { ...init, redirect: "manual" }); }; } /** - * Fetches a URL with Authorization header, handling cross-origin redirects. - * Node.js fetch strips Authorization headers on cross-origin redirects for security. - * Slack's file URLs redirect to CDN domains with pre-signed URLs that don't need the - * Authorization header, so we handle the initial auth request manually. + * Fetches a URL with Authorization header while keeping same-origin redirects + * authenticated and dropping auth once the redirect crosses origins. */ export async function fetchWithSlackAuth(url: string, token: string): Promise { const parsed = assertSlackFileUrl(url); + const authHeaders = createSlackAuthHeaders(token); - // Initial request with auth and manual redirect handling const initialRes = await fetch(parsed.href, { - headers: { Authorization: `Bearer ${token}` }, + headers: authHeaders, redirect: "manual", }); - // If not a redirect, return the response directly if (initialRes.status < 300 || initialRes.status >= 400) { return initialRes; } - // Handle redirect - the redirected URL should be pre-signed and not need auth const redirectUrl = initialRes.headers.get("location"); if (!redirectUrl) { return initialRes; } - // Resolve relative URLs against the original const resolvedUrl = new URL(redirectUrl, parsed.href); - - // Only follow safe protocols (we do NOT include Authorization on redirects). if (resolvedUrl.protocol !== "https:") { return initialRes; } - - // Follow the redirect without the Authorization header - // (Slack's CDN URLs are pre-signed and don't need it) + if (resolvedUrl.origin === parsed.origin) { + return fetch(resolvedUrl.toString(), { + headers: authHeaders, + redirect: "follow", + }); + } return fetch(resolvedUrl.toString(), { redirect: "follow" }); } @@ -223,13 +228,12 @@ export async function resolveSlackMedia(params: { return null; } try { - // Note: fetchRemoteMedia calls fetchImpl(url) with the URL string today and - // handles size limits internally. Provide a fetcher that uses auth once, then lets - // the redirect chain continue without credentials. - const fetchImpl = createSlackMediaFetch(params.token); + const { url: slackUrl, requestInit } = createSlackMediaRequest(url, params.token); + const fetchImpl = createSlackMediaFetch(); const fetched = await fetchRemoteMedia({ - url, + url: slackUrl, fetchImpl, + requestInit, filePathHint: file.name, maxBytes: params.maxBytes, ssrfPolicy: SLACK_MEDIA_SSRF_POLICY, @@ -307,10 +311,12 @@ export async function resolveSlackAttachmentContent(params: { const imageUrl = resolveForwardedAttachmentImageUrl(att); if (imageUrl) { try { - const fetchImpl = createSlackMediaFetch(params.token); + const { url: slackUrl, requestInit } = createSlackMediaRequest(imageUrl, params.token); + const fetchImpl = createSlackMediaFetch(); const fetched = await fetchRemoteMedia({ - url: imageUrl, + url: slackUrl, fetchImpl, + requestInit, maxBytes: params.maxBytes, ssrfPolicy: SLACK_MEDIA_SSRF_POLICY, });