From 706443c79b1cb2f53aceaee08c3f3bedeb9ce8b5 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 4 Jul 2026 08:21:32 -0400 Subject: [PATCH] fix(slack): remove unused unsafe auth fetch helper (#99944) * fix(slack): remove unused unsafe auth fetch helper Co-authored-by: liyuanbin * fix(slack): remove unused unsafe auth fetch helper --------- Co-authored-by: liyuanbin --- extensions/slack/src/monitor/media.test.ts | 167 --------------------- extensions/slack/src/monitor/media.ts | 52 ------- 2 files changed, 219 deletions(-) diff --git a/extensions/slack/src/monitor/media.test.ts b/extensions/slack/src/monitor/media.test.ts index 2aa74800162b..7e7f8f206c7c 100644 --- a/extensions/slack/src/monitor/media.test.ts +++ b/extensions/slack/src/monitor/media.test.ts @@ -2,7 +2,6 @@ import type { WebClient } from "@slack/web-api"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { - fetchWithSlackAuth, resolveSlackAttachmentContent, resolveSlackMedia, resolveSlackThreadHistory, @@ -246,172 +245,6 @@ async function expectPrivateDownloadRedirect(params: { expect(getRequestHeader(1, "Authorization")).toBe(params.secondAuthorization); } -describe("fetchWithSlackAuth", () => { - beforeEach(() => { - // Create a new mock for each test - mockFetch = vi.fn( - async (_input: RequestInfo | URL, _init?: RequestInit) => new Response(), - ); - globalThis.fetch = withFetchPreconnect(mockFetch); - }); - - afterEach(() => { - // Restore original fetch - globalThis.fetch = originalFetch; - }); - - it("sends Authorization header on initial request with manual redirect", async () => { - // Simulate direct 200 response (no redirect) - const mockResponse = new Response(Buffer.from("image data"), { - status: 200, - headers: { "content-type": "image/jpeg" }, - }); - mockFetch.mockResolvedValueOnce(mockResponse); - - const result = await fetchWithSlackAuth("https://files.slack.com/test.jpg", "xoxb-test-token"); - - expect(result).toBe(mockResponse); - - // Verify fetch was called with correct params - expect(mockFetch).toHaveBeenCalledTimes(1); - expect(mockFetch).toHaveBeenCalledWith("https://files.slack.com/test.jpg", { - headers: { Authorization: "Bearer xoxb-test-token" }, - redirect: "manual", - }); - }); - - it("rejects non-Slack hosts to avoid leaking tokens", async () => { - await expect( - fetchWithSlackAuth("https://example.com/test.jpg", "xoxb-test-token"), - ).rejects.toThrow(/non-Slack host|non-Slack/i); - - // Should fail fast without attempting a fetch. - expect(mockFetch).not.toHaveBeenCalled(); - }); - - it("strips Authorization header on cross-origin redirects", async () => { - // First call: redirect response from Slack - const redirectResponse = new Response("redirect body", { - status: 302, - headers: { location: "https://cdn.slack-edge.com/presigned-url?sig=abc123" }, - }); - const cancel = vi.spyOn(redirectResponse.body!, "cancel").mockResolvedValue(undefined); - - // Second call: actual file content from CDN - const fileResponse = new Response(Buffer.from("actual image data"), { - status: 200, - headers: { "content-type": "image/jpeg" }, - }); - - mockFetch.mockResolvedValueOnce(redirectResponse).mockResolvedValueOnce(fileResponse); - - const result = await fetchWithSlackAuth("https://files.slack.com/test.jpg", "xoxb-test-token"); - - expect(result).toBe(fileResponse); - expect(mockFetch).toHaveBeenCalledTimes(2); - - // First call should have Authorization header and manual redirect - expect(mockFetch).toHaveBeenNthCalledWith(1, "https://files.slack.com/test.jpg", { - headers: { Authorization: "Bearer xoxb-test-token" }, - redirect: "manual", - }); - - // Second call should follow the redirect without Authorization - expect(mockFetch).toHaveBeenNthCalledWith( - 2, - "https://cdn.slack-edge.com/presigned-url?sig=abc123", - { redirect: "follow" }, - ); - expect(cancel).toHaveBeenCalledOnce(); - }); - - it("preserves Authorization header on same-origin redirects", async () => { - const redirectResponse = new Response("redirect body", { - status: 302, - headers: { location: "/files/redirect-target" }, - }); - const cancel = vi.spyOn(redirectResponse.body!, "cancel").mockResolvedValue(undefined); - - const fileResponse = new Response(Buffer.from("image data"), { - status: 200, - headers: { "content-type": "image/jpeg" }, - }); - - mockFetch.mockResolvedValueOnce(redirectResponse).mockResolvedValueOnce(fileResponse); - - await fetchWithSlackAuth("https://files.slack.com/original.jpg", "xoxb-test-token"); - - expect(mockFetch).toHaveBeenNthCalledWith(2, "https://files.slack.com/files/redirect-target", { - headers: { Authorization: "Bearer xoxb-test-token" }, - redirect: "follow", - }); - expect(cancel).toHaveBeenCalledOnce(); - }); - - it("returns redirect response when no location header is provided", async () => { - // Redirect without location header - const redirectResponse = new Response(null, { - status: 302, - // No location header - }); - - mockFetch.mockResolvedValueOnce(redirectResponse); - - const result = await fetchWithSlackAuth("https://files.slack.com/test.jpg", "xoxb-test-token"); - - // Should return the redirect response directly - expect(result).toBe(redirectResponse); - expect(mockFetch).toHaveBeenCalledTimes(1); - }); - - it("returns redirect response when location header is malformed", async () => { - const redirectResponse = new Response(null, { - status: 302, - headers: { location: "http://[::1" }, - }); - - mockFetch.mockResolvedValueOnce(redirectResponse); - - const result = await fetchWithSlackAuth("https://files.slack.com/test.jpg", "xoxb-test-token"); - - expect(result).toBe(redirectResponse); - expect(mockFetch).toHaveBeenCalledTimes(1); - }); - - it("returns 4xx/5xx responses directly without following", async () => { - const errorResponse = new Response("Not Found", { - status: 404, - }); - - mockFetch.mockResolvedValueOnce(errorResponse); - - const result = await fetchWithSlackAuth("https://files.slack.com/test.jpg", "xoxb-test-token"); - - expect(result).toBe(errorResponse); - expect(mockFetch).toHaveBeenCalledTimes(1); - }); - - it("handles 301 permanent redirects", async () => { - const redirectResponse = new Response(null, { - status: 301, - headers: { location: "https://cdn.slack.com/new-url" }, - }); - - const fileResponse = new Response(Buffer.from("image data"), { - status: 200, - }); - - mockFetch.mockResolvedValueOnce(redirectResponse).mockResolvedValueOnce(fileResponse); - - await fetchWithSlackAuth("https://files.slack.com/test.jpg", "xoxb-test-token"); - - expect(mockFetch).toHaveBeenCalledTimes(2); - expect(mockFetch).toHaveBeenNthCalledWith(2, "https://cdn.slack.com/new-url", { - redirect: "follow", - }); - }); -}); - describe("resolveSlackMedia", () => { beforeEach(() => { mockFetch = vi.fn(); diff --git a/extensions/slack/src/monitor/media.ts b/extensions/slack/src/monitor/media.ts index 93d87d5f5993..74b93d413013 100644 --- a/extensions/slack/src/monitor/media.ts +++ b/extensions/slack/src/monitor/media.ts @@ -100,58 +100,6 @@ function createSlackMediaFetch(): FetchLike { }; } -function resolveSlackFetchForRuntime(): typeof fetch { - return isMockedFetch(globalThis.fetch) ? globalThis.fetch : fetchWithRuntimeDispatcher; -} - -async function cancelUnreadResponseBody(response: Response): Promise { - if (!response.bodyUsed) { - await response.body?.cancel().catch(() => undefined); - } -} - -/** - * 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); - const fetchImpl = resolveSlackFetchForRuntime(); - - const initialRes = await fetchImpl(parsed.href, { - headers: authHeaders, - redirect: "manual", - }); - - if (initialRes.status < 300 || initialRes.status >= 400) { - return initialRes; - } - - const redirectUrl = initialRes.headers.get("location"); - if (!redirectUrl) { - return initialRes; - } - - let resolvedUrl: URL; - try { - resolvedUrl = new URL(redirectUrl, parsed.href); - } catch { - return initialRes; - } - if (resolvedUrl.protocol !== "https:") { - return initialRes; - } - await cancelUnreadResponseBody(initialRes); - if (resolvedUrl.origin === parsed.origin) { - return fetchImpl(resolvedUrl.toString(), { - headers: authHeaders, - redirect: "follow", - }); - } - return fetchImpl(resolvedUrl.toString(), { redirect: "follow" }); -} - const SLACK_MEDIA_SSRF_POLICY = { allowedHostnames: ["*.slack.com", "*.slack-edge.com", "*.slack-files.com"], hostnameAllowlist: ["*.slack.com", "*.slack-edge.com", "*.slack-files.com"],