diff --git a/CHANGELOG.md b/CHANGELOG.md index dc052cf433..0188fe0bc6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ Docs: https://docs.openclaw.ai - Auto-reply: avoid referencing workspace files in /new greeting prompt. (#5706) Thanks @bravostation. - Tools: treat `"*"` tool allowlist entries as valid to avoid spurious unknown-entry warnings. +- Slack: harden media fetch limits and Slack file URL validation. (#6639) Thanks @davidiach. - Process: resolve Windows `spawn()` failures for npm-family CLIs by appending `.cmd` when needed. (#5815) Thanks @thejhinvirtuoso. - Discord: resolve PluralKit proxied senders for allowlists and labels. (#5838) Thanks @thewilloftheshadow. - Agents: ensure OpenRouter attribution headers apply in the embedded runner. diff --git a/src/agents/pi-embedded-runner/system-prompt.ts b/src/agents/pi-embedded-runner/system-prompt.ts index 70f85a74a7..8aa234e4ad 100644 --- a/src/agents/pi-embedded-runner/system-prompt.ts +++ b/src/agents/pi-embedded-runner/system-prompt.ts @@ -74,11 +74,8 @@ export function buildEmbeddedSystemPrompt(params: { }); } -export function createSystemPromptOverride( - systemPrompt: string, -): (defaultPrompt?: string) => string { - const trimmed = systemPrompt.trim(); - return () => trimmed; +export function createSystemPromptOverride(systemPrompt: string): string { + return systemPrompt.trim(); } export function applySystemPromptOverrideToSession( diff --git a/src/slack/monitor/media.test.ts b/src/slack/monitor/media.test.ts index bfe70f0058..b9c6049842 100644 --- a/src/slack/monitor/media.test.ts +++ b/src/slack/monitor/media.test.ts @@ -40,6 +40,17 @@ describe("fetchWithSlackAuth", () => { }); }); + it("rejects non-Slack hosts to avoid leaking tokens", async () => { + const { fetchWithSlackAuth } = await import("./media.js"); + + 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("follows redirects without Authorization header", async () => { const { fetchWithSlackAuth } = await import("./media.js"); diff --git a/src/slack/monitor/media.ts b/src/slack/monitor/media.ts index 161237edcd..666fe1f272 100644 --- a/src/slack/monitor/media.ts +++ b/src/slack/monitor/media.ts @@ -4,15 +4,57 @@ import type { SlackFile } from "../types.js"; import { fetchRemoteMedia } from "../../media/fetch.js"; import { saveMediaBuffer } from "../../media/store.js"; +function normalizeHostname(hostname: string): string { + const normalized = hostname.trim().toLowerCase().replace(/\.$/, ""); + if (normalized.startsWith("[") && normalized.endsWith("]")) { + return normalized.slice(1, -1); + } + return normalized; +} + +function isSlackHostname(hostname: string): boolean { + const normalized = normalizeHostname(hostname); + if (!normalized) { + return false; + } + // Slack-hosted files typically come from *.slack.com and redirect to Slack CDN domains. + // Include a small allowlist of known Slack domains to avoid leaking tokens if a file URL + // is ever spoofed or mishandled. + const allowedSuffixes = ["slack.com", "slack-edge.com", "slack-files.com"]; + return allowedSuffixes.some( + (suffix) => normalized === suffix || normalized.endsWith(`.${suffix}`), + ); +} + +function assertSlackFileUrl(rawUrl: string): URL { + let parsed: URL; + try { + parsed = new URL(rawUrl); + } catch { + throw new Error(`Invalid Slack file URL: ${rawUrl}`); + } + if (parsed.protocol !== "https:") { + throw new Error(`Refusing Slack file URL with non-HTTPS protocol: ${parsed.protocol}`); + } + if (!isSlackHostname(parsed.hostname)) { + throw new Error( + `Refusing to send Slack token to non-Slack host "${parsed.hostname}" (url: ${rawUrl})`, + ); + } + return parsed; +} + /** * Fetches a URL with Authorization header, handling cross-origin redirects. * Node.js fetch strips Authorization headers on cross-origin redirects for security. - * Slack's files.slack.com URLs redirect to CDN domains with pre-signed URLs that - * don't need the Authorization header, so we handle the initial auth request manually. + * 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. */ export async function fetchWithSlackAuth(url: string, token: string): Promise { + const parsed = assertSlackFileUrl(url); + // Initial request with auth and manual redirect handling - const initialRes = await fetch(url, { + const initialRes = await fetch(parsed.href, { headers: { Authorization: `Bearer ${token}` }, redirect: "manual", }); @@ -29,11 +71,16 @@ export async function fetchWithSlackAuth(url: string, token: string): Promise { const inputUrl = typeof input === "string" ? input : input instanceof URL ? input.href : input.url; @@ -63,6 +111,7 @@ export async function resolveSlackMedia(params: { url, fetchImpl, filePathHint: file.name, + maxBytes: params.maxBytes, }); if (fetched.buffer.byteLength > params.maxBytes) { continue; diff --git a/src/web/media.test.ts b/src/web/media.test.ts index 21492db13a..b16e0dff42 100644 --- a/src/web/media.test.ts +++ b/src/web/media.test.ts @@ -4,7 +4,7 @@ import path from "node:path"; import sharp from "sharp"; import { afterEach, describe, expect, it, vi } from "vitest"; import { optimizeImageToPng } from "../media/image-ops.js"; -import { loadWebMedia, optimizeImageToJpeg } from "./media.js"; +import { loadWebMedia, loadWebMediaRaw, optimizeImageToJpeg } from "./media.js"; const tmpFiles: string[] = []; @@ -106,6 +106,22 @@ describe("web media loading", () => { fetchMock.mockRestore(); }); + it("respects maxBytes for raw URL fetches", async () => { + const fetchMock = vi.spyOn(globalThis, "fetch").mockResolvedValueOnce({ + ok: true, + body: true, + arrayBuffer: async () => Buffer.alloc(2048).buffer, + headers: { get: () => "image/png" }, + status: 200, + } as Response); + + await expect(loadWebMediaRaw("https://example.com/too-big.png", 1024)).rejects.toThrow( + /exceeds maxBytes 1024/i, + ); + + fetchMock.mockRestore(); + }); + it("uses content-disposition filename when available", async () => { const fetchMock = vi.spyOn(globalThis, "fetch").mockResolvedValueOnce({ ok: true, diff --git a/src/web/media.ts b/src/web/media.ts index 52d2ca6bf0..f3f74c779c 100644 --- a/src/web/media.ts +++ b/src/web/media.ts @@ -200,7 +200,16 @@ async function loadWebMediaInternal( }; if (/^https?:\/\//i.test(mediaUrl)) { - const fetched = await fetchRemoteMedia({ url: mediaUrl }); + // Enforce a download cap during fetch to avoid unbounded memory usage. + // For optimized images, allow fetching larger payloads before compression. + const defaultFetchCap = maxBytesForKind("unknown"); + const fetchCap = + maxBytes === undefined + ? defaultFetchCap + : optimizeImages + ? Math.max(maxBytes, defaultFetchCap) + : maxBytes; + const fetched = await fetchRemoteMedia({ url: mediaUrl, maxBytes: fetchCap }); const { buffer, contentType, fileName } = fetched; const kind = mediaKindFromMime(contentType); return await clampAndFinalize({ buffer, contentType, kind, fileName });