fix(slack): reject HTML responses when downloading media (openclaw#4665)

tumf · Takhoffman · web-flow · commit e0571399acd3 · 2026-03-01T11:20:25.000-06:00
* fix(slack): reject HTML responses when downloading media

Slack sometimes returns HTML login pages instead of binary media when
authentication fails or URLs expire. This change detects HTML responses
by checking content-type header and buffer content, then skips to the
next available file URL.

* fix: format import order and add braces to continue statement

* chore: format Slack media tests

* chore: apply formatter to Slack media tests

* fix(slack): merge auth-header forwarding and html media guard

---------

Co-authored-by: Tak Hoffman &lt;781889+Takhoffman@users.noreply.github.com&gt;
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -84,6 +84,7 @@ Docs: https://docs.openclaw.ai
 ### Fixes
 
 - Slack/Bot attachment-only messages: when `allowBots: true`, bot messages with empty `text` now include non-forwarded attachment `text`/`fallback` content so webhook alerts are not silently dropped. (#27616)
+- Slack/Inbound media auth + HTML guard: keep Slack auth headers on forwarded shared attachment image downloads, and reject login/error HTML payloads (while allowing expected `.html` uploads) when resolving Slack media so auth failures do not silently pass as files. (#18642)
 - Slack/Security ingress mismatch guard: drop slash-command and interaction payloads when app/team identifiers do not match the active Slack account context (including nested `team.id` interaction payloads), preventing cross-app or cross-workspace payload injection into system-event handling. (#29091) Thanks @Solvely-Colin.
 - Cron/Failure alerts: add configurable repeated-failure alerting with per-job overrides and Web UI cron editor support (`inherit|disabled|custom` with threshold/cooldown/channel/target fields). (#24789) Thanks xbrak.
 - Cron/Isolated model defaults: resolve isolated cron `subagents.model` (including object-form `primary`) through allowlist-aware model selection so isolated cron runs honor subagent model defaults unless explicitly overridden by job payload model. (#11474) Thanks @AnonO6.
diff --git a/src/slack/monitor/media.test.ts b/src/slack/monitor/media.test.ts
@@ -245,6 +245,52 @@ describe("resolveSlackMedia", () => {
     expect(mockFetch).not.toHaveBeenCalled();
   });
 
+  it("rejects HTML auth pages for non-HTML files", async () => {
+    const saveMediaBufferMock = vi.spyOn(mediaStore, "saveMediaBuffer");
+    mockFetch.mockResolvedValueOnce(
+      new Response("<!DOCTYPE html><html><body>login</body></html>", {
+        status: 200,
+        headers: { "content-type": "text/html; charset=utf-8" },
+      }),
+    );
+
+    const result = await resolveSlackMedia({
+      files: [{ url_private: "https://files.slack.com/test.jpg", name: "test.jpg" }],
+      token: "xoxb-test-token",
+      maxBytes: 1024 * 1024,
+    });
+
+    expect(result).toBeNull();
+    expect(saveMediaBufferMock).not.toHaveBeenCalled();
+  });
+
+  it("allows expected HTML uploads", async () => {
+    vi.spyOn(mediaStore, "saveMediaBuffer").mockResolvedValue(
+      createSavedMedia("/tmp/page.html", "text/html"),
+    );
+    mockFetch.mockResolvedValueOnce(
+      new Response("<!doctype html><html><body>ok</body></html>", {
+        status: 200,
+        headers: { "content-type": "text/html" },
+      }),
+    );
+
+    const result = await resolveSlackMedia({
+      files: [
+        {
+          url_private: "https://files.slack.com/page.html",
+          name: "page.html",
+          mimetype: "text/html",
+        },
+      ],
+      token: "xoxb-test-token",
+      maxBytes: 1024 * 1024,
+    });
+
+    expect(result).not.toBeNull();
+    expect(result?.[0]?.path).toBe("/tmp/page.html");
+  });
+
   it("overrides video/* MIME to audio/* for slack_audio voice messages", async () => {
     // saveMediaBuffer re-detects MIME from buffer bytes, so it may return
     // video/mp4 for MP4 containers.  Verify resolveSlackMedia preserves
@@ -525,6 +571,11 @@ describe("resolveSlackAttachmentContent", () => {
         },
       ],
     });
+    const firstCall = mockFetch.mock.calls[0];
+    expect(firstCall?.[0]).toBe("https://files.slack.com/forwarded.jpg");
+    const firstInit = firstCall?.[1];
+    expect(firstInit?.redirect).toBe("manual");
+    expect(new Headers(firstInit?.headers).get("Authorization")).toBe("Bearer xoxb-test-token");
   });
 });
 
diff --git a/src/slack/monitor/media.ts b/src/slack/monitor/media.ts
@@ -125,6 +125,11 @@ function resolveSlackMediaMimetype(
   return mime;
 }
 
+function looksLikeHtmlBuffer(buffer: Buffer): boolean {
+  const head = buffer.subarray(0, 512).toString("utf-8").replace(/^\s+/, "").toLowerCase();
+  return head.startsWith("<!doctype html") || head.startsWith("<html");
+}
+
 export type SlackMediaResult = {
   path: string;
   contentType?: string;
@@ -217,6 +222,20 @@ export async function resolveSlackMedia(params: {
         if (fetched.buffer.byteLength > params.maxBytes) {
           return null;
         }
+
+        // Guard against auth/login HTML pages returned instead of binary media.
+        // Allow user-provided HTML files through.
+        const fileMime = file.mimetype?.toLowerCase();
+        const fileName = file.name?.toLowerCase() ?? "";
+        const isExpectedHtml =
+          fileMime === "text/html" || fileName.endsWith(".html") || fileName.endsWith(".htm");
+        if (!isExpectedHtml) {
+          const detectedMime = fetched.contentType?.split(";")[0]?.trim().toLowerCase();
+          if (detectedMime === "text/html" || looksLikeHtmlBuffer(fetched.buffer)) {
+            return null;
+          }
+        }
+
         const effectiveMime = resolveSlackMediaMimetype(file, fetched.contentType);
         const saved = await saveMediaBuffer(
           fetched.buffer,
@@ -273,8 +292,10 @@ export async function resolveSlackAttachmentContent(params: {
     const imageUrl = resolveForwardedAttachmentImageUrl(att);
     if (imageUrl) {
       try {
+        const fetchImpl = createSlackMediaFetch(params.token);
         const fetched = await fetchRemoteMedia({
           url: imageUrl,
+          fetchImpl,
           maxBytes: params.maxBytes,
         });
         if (fetched.buffer.byteLength <= params.maxBytes) {
