From 68b7d6737dba470e7bbe6957ad36014f4ec3e899 Mon Sep 17 00:00:00 2001 From: dj-oyu <68707227+dj-oyu@users.noreply.github.com> Date: Sun, 22 Mar 2026 18:09:59 +0900 Subject: [PATCH] fix: use HTTP response headers for URL download filename/extension URLs without file extensions (e.g. /mcp/photos/23) resulted in temp files with no extension, meaningless display filenames, and fallback to application/octet-stream. Now downloadToTemp returns Content-Type and Content-Disposition from the HTTP response, which are used to: - Add correct extension to temp files from Content-Type - Derive display filename via Content-Disposition > URL basename > fallback - Use response Content-Type as mediaType when magic bytes fail Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/tools/send_file.go | 126 ++++++++++++++++++++++++++++++------ pkg/tools/send_file_test.go | 90 ++++++++++++++++++++++---- 2 files changed, 182 insertions(+), 34 deletions(-) diff --git a/pkg/tools/send_file.go b/pkg/tools/send_file.go index b9655fa0b..d03f3e0e1 100644 --- a/pkg/tools/send_file.go +++ b/pkg/tools/send_file.go @@ -124,13 +124,15 @@ func (t *SendFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe // Handle URL downloads var resolved string + var dlResult downloadResult var isURL bool if isHTTPURL(path) { var err error - resolved, err = downloadToTemp(ctx, path) + dlResult, err = downloadToTemp(ctx, path) if err != nil { return ErrorResult(fmt.Sprintf("download failed: %v", err)) } + resolved = dlResult.Path isURL = true // Do NOT delete the temp file here — MediaStore only stores a path // reference, so the file must survive until the channel sends it. @@ -160,13 +162,17 @@ func (t *SendFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe filename, _ := args["filename"].(string) if filename == "" { if isURL { - filename = filenameFromURL(path) + filename = filenameForDownload(path, dlResult.ContentDisposition, dlResult.ContentType) } else { filename = filepath.Base(resolved) } } mediaType := detectMediaType(resolved) + // If magic bytes and extension both failed, use Content-Type from HTTP response + if isURL && mediaType == "application/octet-stream" && dlResult.ContentType != "" { + mediaType = dlResult.ContentType + } scope := fmt.Sprintf("tool:send_file:%s:%s", channel, chatID) ref, err := t.mediaStore.Store(resolved, media.MediaMeta{ @@ -186,41 +192,57 @@ func isHTTPURL(path string) bool { return strings.HasPrefix(path, "http://") || strings.HasPrefix(path, "https://") } +// downloadResult holds the downloaded temp file path and HTTP response metadata. +type downloadResult struct { + Path string + ContentType string // from Content-Type header (media type only, no params) + ContentDisposition string // raw Content-Disposition header +} + // downloadToTemp downloads a URL to a temporary file under sendFileTempDir(). // The file must persist until the channel has sent it; cleanup is handled by // CleanupTempFiles (periodic) rather than the caller. -func downloadToTemp(ctx context.Context, rawURL string) (string, error) { +func downloadToTemp(ctx context.Context, rawURL string) (downloadResult, error) { dir := sendFileTempDir() if err := os.MkdirAll(dir, 0o700); err != nil { - return "", fmt.Errorf("create temp dir: %w", err) + return downloadResult{}, fmt.Errorf("create temp dir: %w", err) } req, err := http.NewRequestWithContext(ctx, http.MethodGet, rawURL, nil) if err != nil { - return "", fmt.Errorf("build request: %w", err) + return downloadResult{}, fmt.Errorf("build request: %w", err) } resp, err := http.DefaultClient.Do(req) if err != nil { - return "", fmt.Errorf("HTTP GET: %w", err) + return downloadResult{}, fmt.Errorf("HTTP GET: %w", err) } defer resp.Body.Close() if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return "", fmt.Errorf("HTTP %d %s", resp.StatusCode, resp.Status) + return downloadResult{}, fmt.Errorf("HTTP %d %s", resp.StatusCode, resp.Status) + } + + // Extract Content-Type (media type only) + respCT := resp.Header.Get("Content-Type") + mediaType, _, _ := mime.ParseMediaType(respCT) + + // Determine file extension: prefer URL path, then Content-Type header + ext := extFromURL(rawURL) + if ext == "" && mediaType != "" { + ext = preferredExtension(mediaType) } // Generate a random temp filename to avoid collisions var randBytes [8]byte if _, err := rand.Read(randBytes[:]); err != nil { - return "", err + return downloadResult{}, err } - ext := extFromURL(rawURL) tmpPath := filepath.Join(dir, fmt.Sprintf("dl_%x%s", randBytes, ext)) f, err := os.OpenFile(tmpPath, os.O_CREATE|os.O_WRONLY|os.O_EXCL, 0o600) if err != nil { - return "", fmt.Errorf("create temp file: %w", err) + return downloadResult{}, fmt.Errorf("create temp file: %w", err) } n, copyErr := io.Copy(f, io.LimitReader(resp.Body, sendFileDownloadLimit+1)) @@ -229,14 +251,18 @@ func downloadToTemp(ctx context.Context, rawURL string) (string, error) { } if copyErr != nil { os.Remove(tmpPath) - return "", fmt.Errorf("write temp file: %w", copyErr) + return downloadResult{}, fmt.Errorf("write temp file: %w", copyErr) } if n > sendFileDownloadLimit { os.Remove(tmpPath) - return "", fmt.Errorf("download exceeds %d byte limit", sendFileDownloadLimit) + return downloadResult{}, fmt.Errorf("download exceeds %d byte limit", sendFileDownloadLimit) } - return tmpPath, nil + return downloadResult{ + Path: tmpPath, + ContentType: mediaType, + ContentDisposition: resp.Header.Get("Content-Disposition"), + }, nil } // extFromURL extracts a file extension from a URL path, e.g. ".jpg". @@ -248,17 +274,75 @@ func extFromURL(rawURL string) string { return filepath.Ext(u.Path) } -// filenameFromURL derives a display filename from a URL. -func filenameFromURL(rawURL string) string { +// filenameForDownload derives a display filename using Content-Disposition, +// the URL path, and Content-Type as progressive fallbacks. +func filenameForDownload(rawURL, contentDisposition, contentType string) string { + // 1. Try Content-Disposition header + if contentDisposition != "" { + _, params, err := mime.ParseMediaType(contentDisposition) + if err == nil { + if fn := params["filename"]; fn != "" { + return fn + } + } + } + + // 2. Try URL path basename u, err := url.Parse(rawURL) - if err != nil { - return "download" + if err == nil { + base := filepath.Base(u.Path) + if base != "" && base != "." && base != "/" { + // If basename has no extension, try to add one from Content-Type + if filepath.Ext(base) == "" && contentType != "" { + if ext := preferredExtension(contentType); ext != "" { + return base + ext + } + } + return base + } } - base := filepath.Base(u.Path) - if base == "" || base == "." || base == "/" { - return "download" + + // 3. Fallback + if contentType != "" { + if ext := preferredExtension(contentType); ext != "" { + return "download" + ext + } } - return base + return "download" +} + +// preferredExtension returns a file extension (with dot) for a MIME type. +// Uses a short map of common types for deterministic results, then falls back +// to mime.ExtensionsByType. +func preferredExtension(mediaType string) string { + // mime.ExtensionsByType returns multiple options in undefined order; + // hardcode the most common ones for determinism. + preferred := map[string]string{ + "image/png": ".png", + "image/jpeg": ".jpg", + "image/gif": ".gif", + "image/webp": ".webp", + "image/svg+xml": ".svg", + "image/bmp": ".bmp", + "image/tiff": ".tiff", + "video/mp4": ".mp4", + "video/webm": ".webm", + "audio/mpeg": ".mp3", + "audio/ogg": ".ogg", + "application/pdf": ".pdf", + "application/zip": ".zip", + "text/plain": ".txt", + "text/html": ".html", + "application/json": ".json", + } + if ext, ok := preferred[mediaType]; ok { + return ext + } + exts, err := mime.ExtensionsByType(mediaType) + if err == nil && len(exts) > 0 { + return exts[0] + } + return "" } // CleanupTempFiles removes old temp files from the sendfile temp directory. diff --git a/pkg/tools/send_file_test.go b/pkg/tools/send_file_test.go index c8e62d5a1..38d10fde7 100644 --- a/pkg/tools/send_file_test.go +++ b/pkg/tools/send_file_test.go @@ -198,6 +198,7 @@ func TestSendFileTool_URLDownload(t *testing.T) { func TestSendFileTool_URLDownloadDefaultFilename(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "image/png") w.Write([]byte("data")) })) defer srv.Close() @@ -212,12 +213,69 @@ func TestSendFileTool_URLDownloadDefaultFilename(t *testing.T) { if result.IsError { t.Fatalf("unexpected error: %s", result.ForLLM) } - // filename should be derived from URL path: "42" - if !strings.Contains(result.ForLLM, `"42"`) { - t.Errorf("expected filename '42' in result, got %q", result.ForLLM) + // filename should be "42.png" (URL basename + extension from Content-Type) + if !strings.Contains(result.ForLLM, `"42.png"`) { + t.Errorf("expected filename '42.png' in result, got %q", result.ForLLM) } } +func TestSendFileTool_URLDownloadContentDisposition(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "image/jpeg") + w.Header().Set("Content-Disposition", `attachment; filename="sunset.jpg"`) + w.Write([]byte("data")) + })) + defer srv.Close() + + store := media.NewFileMediaStore() + tool := NewSendFileTool(t.TempDir(), false, 0, store) + tool.SetContext("telegram", "chat123") + + result := tool.Execute(context.Background(), map[string]any{ + "path": srv.URL + "/mcp/photos/99", + }) + if result.IsError { + t.Fatalf("unexpected error: %s", result.ForLLM) + } + // filename should come from Content-Disposition + if !strings.Contains(result.ForLLM, `"sunset.jpg"`) { + t.Errorf("expected filename 'sunset.jpg' from Content-Disposition, got %q", result.ForLLM) + } +} + +func TestSendFileTool_URLDownloadContentTypeAsMIME(t *testing.T) { + // Server returns image/png Content-Type but file has no magic bytes match + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "image/png") + w.Write([]byte("not-real-png-data")) + })) + defer srv.Close() + + store := media.NewFileMediaStore() + tool := NewSendFileTool(t.TempDir(), false, 0, store) + tool.SetContext("telegram", "chat123") + + result := tool.Execute(context.Background(), map[string]any{ + "path": srv.URL + "/api/image/5", + }) + if result.IsError { + t.Fatalf("unexpected error: %s", result.ForLLM) + } + // The media should have been registered — check it resolves + if len(result.Media) != 1 { + t.Fatalf("expected 1 media ref, got %d", len(result.Media)) + } + resolved, err := store.Resolve(result.Media[0]) + if err != nil { + t.Fatalf("Resolve failed: %v", err) + } + // Temp file should have .png extension from Content-Type + if filepath.Ext(resolved) != ".png" { + t.Errorf("expected temp file to have .png extension, got %q", filepath.Base(resolved)) + } + t.Cleanup(func() { os.Remove(resolved) }) +} + func TestSendFileTool_URLDownloadHTTPError(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNotFound) @@ -299,21 +357,27 @@ func TestCleanupTempFiles(t *testing.T) { } } -func TestFilenameFromURL(t *testing.T) { +func TestFilenameForDownload(t *testing.T) { tests := []struct { - url string - want string + name string + url string + contentDisposition string + contentType string + want string }{ - {"https://example.com/photos/cat.jpg", "cat.jpg"}, - {"https://example.com/mcp/photos/42", "42"}, - {"https://example.com/", "download"}, - {"https://example.com", "download"}, + {"url with extension", "https://example.com/photos/cat.jpg", "", "", "cat.jpg"}, + {"url no ext + content-type", "https://example.com/mcp/photos/42", "", "image/png", "42.png"}, + {"url no ext no content-type", "https://example.com/mcp/photos/42", "", "", "42"}, + {"root path", "https://example.com/", "", "image/jpeg", "download.jpg"}, + {"root no content-type", "https://example.com", "", "", "download"}, + {"content-disposition wins", "https://example.com/mcp/photos/42", `attachment; filename="photo.png"`, "image/jpeg", "photo.png"}, + {"content-disposition inline", "https://example.com/x", `inline; filename="report.pdf"`, "application/pdf", "report.pdf"}, } for _, tt := range tests { - t.Run(tt.url, func(t *testing.T) { - got := filenameFromURL(tt.url) + t.Run(tt.name, func(t *testing.T) { + got := filenameForDownload(tt.url, tt.contentDisposition, tt.contentType) if got != tt.want { - t.Errorf("filenameFromURL(%q) = %q, want %q", tt.url, got, tt.want) + t.Errorf("filenameForDownload() = %q, want %q", got, tt.want) } }) }