From a0bcd93a88ab54e498f569fa1e51626cb69ef252 Mon Sep 17 00:00:00 2001 From: cg33 Date: Wed, 18 Mar 2026 15:08:53 +0800 Subject: [PATCH] fix(slack): improve file download error handling and auth diagnostics (#204) When downloading Slack files via url_private_download, the code was already setting the Bearer token correctly. However, there was no error detection for cases where Slack returns an HTML login page instead of the actual file content. This commit adds: - Fallback to url_private when url_private_download is empty - Status code validation (fail on non-200 responses) - HTML content detection (fail with clear error message) - Improved error logging with redacted URL - Unit tests for the downloadSlackFile function The HTML detection is particularly important because when auth fails (e.g., wrong bot token, expired token), Slack returns a login page with HTTP 200 instead of an error code. Without this check, the HTML content would be passed to the AI agent as if it were an image, causing confusing errors. Fixes #185 Co-authored-by: Shawn Co-authored-by: Claude Opus 4.6 --- platform/slack/slack.go | 38 +++++++++++++++--- platform/slack/slack_test.go | 74 +++++++++++++++++++++++++++++++++++- 2 files changed, 106 insertions(+), 6 deletions(-) diff --git a/platform/slack/slack.go b/platform/slack/slack.go index 57b305a..36729ba 100644 --- a/platform/slack/slack.go +++ b/platform/slack/slack.go @@ -1,6 +1,7 @@ package slack import ( + "bytes" "context" "fmt" "io" @@ -186,10 +187,20 @@ func (p *Platform) handleEvent(evt socketmode.Event) { var images []core.ImageAttachment var audio *core.AudioAttachment for _, f := range ev.Files { + // Prefer URLPrivateDownload, fall back to URLPrivate + fileURL := f.URLPrivateDownload + if fileURL == "" { + fileURL = f.URLPrivate + } + if fileURL == "" { + slog.Warn("slack: file has no download URL", "file_id", f.ID, "name", f.Name) + continue + } + if f.Mimetype != "" && strings.HasPrefix(f.Mimetype, "audio/") { - data, err := p.downloadSlackFile(f.URLPrivateDownload) + data, err := p.downloadSlackFile(fileURL) if err != nil { - slog.Error("slack: download audio failed", "error", err) + slog.Error("slack: download audio failed", "error", err, "url", core.RedactToken(fileURL, p.botToken)) continue } format := "mp3" @@ -200,9 +211,9 @@ func (p *Platform) handleEvent(evt socketmode.Event) { MimeType: f.Mimetype, Data: data, Format: format, } } else if f.Mimetype != "" && strings.HasPrefix(f.Mimetype, "image/") { - imgData, err := p.downloadSlackFile(f.URLPrivateDownload) + imgData, err := p.downloadSlackFile(fileURL) if err != nil { - slog.Error("slack: download file failed", "error", err) + slog.Error("slack: download file failed", "error", err, "url", core.RedactToken(fileURL, p.botToken)) continue } images = append(images, core.ImageAttachment{ @@ -327,7 +338,24 @@ func (p *Platform) downloadSlackFile(url string) ([]byte, error) { return nil, fmt.Errorf("%s", core.RedactToken(err.Error(), p.botToken)) } defer resp.Body.Close() - return io.ReadAll(resp.Body) + + // Check if we got an unexpected status code (e.g., redirect to login page) + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024)) + return nil, fmt.Errorf("download failed with status %d: %s", resp.StatusCode, string(body)) + } + + data, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("read response body: %w", err) + } + + // Basic sanity check: detect if we received HTML instead of binary data + if len(data) > 0 && (bytes.HasPrefix(data, []byte("Please login")) + })) + defer ts.Close() + + p := &Platform{botToken: "xoxb-test-token"} + _, err := p.downloadSlackFile(ts.URL) + if err == nil { + t.Fatal("expected error for HTML response, got nil") + } + // Should detect HTML prefix + if err != nil && err.Error() == "" { + t.Fatal("expected non-empty error message") + } +} + +func TestDownloadSlackFile_MissingAuth(t *testing.T) { + // Test that we return an error for non-200 status codes + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + w.Write([]byte("unauthorized")) + })) + defer ts.Close() + + p := &Platform{botToken: "xoxb-test-token"} + _, err := p.downloadSlackFile(ts.URL) + if err == nil { + t.Fatal("expected error for 401 response, got nil") + } +} + +func TestDownloadSlackFile_Success(t *testing.T) { + // Test successful binary download + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Verify Authorization header is set + auth := r.Header.Get("Authorization") + if auth != "Bearer xoxb-test-token" { + t.Errorf("expected Authorization header 'Bearer xoxb-test-token', got %q", auth) + } + w.Header().Set("Content-Type", "image/png") + w.WriteHeader(http.StatusOK) + w.Write([]byte("\x89PNG\r\n\x1a\n")) // PNG magic bytes + })) + defer ts.Close() + + p := &Platform{botToken: "xoxb-test-token"} + data, err := p.downloadSlackFile(ts.URL) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(data) != 8 { + t.Errorf("expected 8 bytes, got %d", len(data)) + } +} + +func TestDownloadSlackFile_EmptyURL(t *testing.T) { + p := &Platform{botToken: "xoxb-test-token"} + _, err := p.downloadSlackFile("") + if err == nil { + t.Fatal("expected error for empty URL, got nil") + } +}