From 1dea4a21cf80ade988d15f7eb1a9efb60e88708b Mon Sep 17 00:00:00 2001 From: Goon Date: Mon, 15 Jun 2026 10:20:21 +0700 Subject: [PATCH] fix: harden media URL handling --- internal/tools/read_image.go | 15 ++++++++++++++- internal/tools/read_image_test.go | 18 +++++++++++++++++- internal/tools/read_video.go | 19 +++++++++++++++---- internal/tools/read_video_resolve.go | 22 ++++++++++++++++------ internal/tools/read_video_test.go | 27 ++++++++++++++++++++++++--- pkg/browser/browser_remote.go | 2 +- 6 files changed, 87 insertions(+), 16 deletions(-) diff --git a/internal/tools/read_image.go b/internal/tools/read_image.go index 9c90d0fc..8664184d 100644 --- a/internal/tools/read_image.go +++ b/internal/tools/read_image.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/security" usagecaps "github.com/nextlevelbuilder/goclaw/internal/usage/caps" ) @@ -62,7 +63,7 @@ func (t *ReadImageTool) SetUsageCapService(svc *usagecaps.Service) { func (t *ReadImageTool) Name() string { return "read_image" } func (t *ReadImageTool) Description() string { - return "Analyze images using vision AI. Works with: (1) images sent by the user ( tags), (2) workspace/generated image files (pass a file path)." + return "Analyze images using vision AI. Works with images sent by the user, workspace/generated image files, or public HTTP/HTTPS image URLs." } func (t *ReadImageTool) Parameters() map[string]any { @@ -111,6 +112,9 @@ func (t *ReadImageTool) Execute(ctx context.Context, args map[string]any) *Resul } images = fileImages } else if imgURL != "" { + if _, _, err := security.Validate(imgURL); err != nil { + return ErrorResult(fmt.Sprintf("Invalid image URL: %v", err)) + } images = []providers.ImageContent{{ URL: imgURL, }} @@ -153,6 +157,15 @@ func (t *ReadImageTool) callProvider(ctx context.Context, cp credentialProvider, prompt := GetParamString(params, "prompt", "Describe this image in detail.") images, _ := params["images"].([]providers.ImageContent) + for _, img := range images { + if img.URL == "" { + continue + } + if _, _, err := security.Validate(img.URL); err != nil { + return nil, nil, fmt.Errorf("invalid image URL: %w", err) + } + } + // Anthropic Claude does not support URL references and requires base64-encoded image data. if providerName == "anthropic" || providerName == "claude-cli" { for _, img := range images { diff --git a/internal/tools/read_image_test.go b/internal/tools/read_image_test.go index 1d61f86d..87ee177a 100644 --- a/internal/tools/read_image_test.go +++ b/internal/tools/read_image_test.go @@ -26,6 +26,22 @@ func TestReadImage_BothPathAndUrl_Error(t *testing.T) { } } +func TestReadImage_PrivateURL_Error(t *testing.T) { + tool := NewReadImageTool(nil) + + res := tool.Execute(context.Background(), map[string]any{ + "prompt": "describe this", + "url": "http://127.0.0.1/image.png", + }) + + if !res.IsError { + t.Fatalf("expected error for private image URL") + } + if !strings.Contains(res.ForLLM, "Invalid image URL") { + t.Errorf("unexpected error message: %s", res.ForLLM) + } +} + func TestReadImage_AnthropicURL_Error(t *testing.T) { tool := NewReadImageTool(nil) @@ -33,7 +49,7 @@ func TestReadImage_AnthropicURL_Error(t *testing.T) { "prompt": "describe this", "images": []providers.ImageContent{ { - URL: "https://example.com/image.png", + URL: "https://93.184.216.34/image.png", }, }, } diff --git a/internal/tools/read_video.go b/internal/tools/read_video.go index f65e0f06..48b1aaba 100644 --- a/internal/tools/read_video.go +++ b/internal/tools/read_video.go @@ -4,11 +4,13 @@ import ( "context" "fmt" "log/slog" + "net" "os" "path/filepath" "strings" "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/security" usagecaps "github.com/nextlevelbuilder/goclaw/internal/usage/caps" ) @@ -32,6 +34,8 @@ func MediaVideoRefsFromCtx(ctx context.Context) []providers.MediaRef { // videoMaxBytes is the max file size for video analysis (100MB). const videoMaxBytes = 100 * 1024 * 1024 +const videoURLPinnedIPParam = "_pinned_ip" + // videoProviderPriority is the order in which providers are tried for video analysis. // OpenAI excluded — no native video upload in chat completions. var videoProviderPriority = []string{"gemini", "openrouter"} @@ -101,13 +105,17 @@ func (t *ReadVideoTool) Execute(ctx context.Context, args map[string]any) *Resul var data []byte var videoMime string + var pinnedIP net.IP if videoURL != "" { - // Infer MIME type from URL extension - ext := filepath.Ext(videoURL) - if idx := strings.Index(ext, "?"); idx != -1 { - ext = ext[:idx] + validatedURL, validatedIP, err := security.Validate(videoURL) + if err != nil { + return ErrorResult(fmt.Sprintf("Invalid video URL: %v", err)) } + pinnedIP = validatedIP + + // Infer MIME type from URL extension + ext := filepath.Ext(validatedURL.Path) videoMime = mimeFromVideoExt(ext) } else { videoPath, mime, err := t.resolveVideoFile(ctx, mediaID) @@ -139,6 +147,9 @@ func (t *ReadVideoTool) Execute(ctx context.Context, args map[string]any) *Resul chain[i].Params["data"] = data chain[i].Params["url"] = videoURL chain[i].Params["mime"] = videoMime + if pinnedIP != nil { + chain[i].Params[videoURLPinnedIPParam] = pinnedIP + } } chainResult, err := ExecuteWithChain(ctx, chain, t.registry, t.callProvider) diff --git a/internal/tools/read_video_resolve.go b/internal/tools/read_video_resolve.go index 8e371bd1..646068cb 100644 --- a/internal/tools/read_video_resolve.go +++ b/internal/tools/read_video_resolve.go @@ -5,12 +5,14 @@ import ( "encoding/base64" "fmt" "log/slog" + "net" "net/http" "path/filepath" "strings" "time" "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/security" ) // resolveVideoFile finds the video file path from context MediaRefs. @@ -62,12 +64,19 @@ func (t *ReadVideoTool) resolveVideoFile(ctx context.Context, mediaID string) (p // callProvider dispatches video analysis to the appropriate provider API. // Gemini: uses File API (upload → poll → file_data in generateContent). -// Others: falls back to base64 or URL in image_url (OpenRouter routes to Gemini which handles video). +// Others: falls back to base64 or URL in video_url (OpenRouter routes to Gemini which handles video). func (t *ReadVideoTool) callProvider(ctx context.Context, cp credentialProvider, providerName, model string, params map[string]any) ([]byte, *providers.Usage, error) { prompt := GetParamString(params, "prompt", "Analyze this video and describe its contents.") data, _ := params["data"].([]byte) videoURL, _ := params["url"].(string) mime := GetParamString(params, "mime", "video/mp4") + pinnedIP, _ := params[videoURLPinnedIPParam].(net.IP) + if videoURL != "" && pinnedIP == nil { + var err error + if _, pinnedIP, err = security.Validate(videoURL); err != nil { + return nil, nil, fmt.Errorf("invalid video URL: %w", err) + } + } // Gemini: use File API (requires credentials). ptype := GetParamString(params, "_provider_type", providerTypeFromName(providerName)) @@ -87,18 +96,19 @@ func (t *ReadVideoTool) callProvider(ctx context.Context, cp credentialProvider, if videoURL != "" { slog.Info("read_video: streaming URL directly to Gemini File API", "provider", providerName, "model", model, "url", videoURL) - + // Send GET request to fetch the stream. - req, getErr := http.NewRequestWithContext(ctx, "GET", videoURL, nil) + reqCtx := security.WithPinnedIP(ctx, pinnedIP) + req, getErr := http.NewRequestWithContext(reqCtx, "GET", videoURL, nil) if getErr != nil { if reservation != nil { reservation.Reconcile(ctx, nil, getErr) } return nil, nil, fmt.Errorf("failed to create GET request for video URL: %w", getErr) } - - // No global Timeout on http.Client to allow piping large streams. - client := &http.Client{} + + // Use the shared SSRF-safe client so DNS stays pinned during streaming. + client := security.NewSafeClient(0) httpResp, getErr := client.Do(req) if getErr != nil { if reservation != nil { diff --git a/internal/tools/read_video_test.go b/internal/tools/read_video_test.go index 31d9ba99..3440f947 100644 --- a/internal/tools/read_video_test.go +++ b/internal/tools/read_video_test.go @@ -6,6 +6,8 @@ import ( "net/http/httptest" "strings" "testing" + + "github.com/nextlevelbuilder/goclaw/internal/security" ) type mockCredentialProvider struct { @@ -34,8 +36,27 @@ func TestReadVideo_BothMediaIdAndUrl_Error(t *testing.T) { } } +func TestReadVideo_PrivateURL_Error(t *testing.T) { + tool := NewReadVideoTool(nil, nil) + + res := tool.Execute(context.Background(), map[string]any{ + "prompt": "describe this video", + "url": "http://127.0.0.1/video.mp4", + }) + + if !res.IsError { + t.Fatalf("expected error for private video URL") + } + if !strings.Contains(res.ForLLM, "Invalid video URL") { + t.Errorf("unexpected error message: %s", res.ForLLM) + } +} + func TestReadVideo_GeminiURL_Validation(t *testing.T) { - // 1. Trường hợp không có Content-Length (ContentLength <= 0) + security.SetAllowLoopbackForTest(true) + defer security.SetAllowLoopbackForTest(false) + + // Missing Content-Length should fail before upload. ts1 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Transfer-Encoding", "chunked") w.Write([]byte("chunked data mock video")) @@ -59,7 +80,7 @@ func TestReadVideo_GeminiURL_Validation(t *testing.T) { t.Errorf("unexpected error for missing Content-Length: %v", err) } - // 2. Trường hợp Content-Length vượt quá 2 GB + // Content-Length over the Gemini File API limit should fail before upload. ts2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Length", "2147483649") // 2GB + 1 byte w.WriteHeader(http.StatusOK) @@ -80,7 +101,7 @@ func TestReadVideo_GeminiURL_Validation(t *testing.T) { t.Errorf("unexpected error for limit exceed: %v", err) } - // 3. Trường hợp HTTP status code lỗi (ví dụ 404) + // Non-2xx status should be reported before upload. ts3 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNotFound) })) diff --git a/pkg/browser/browser_remote.go b/pkg/browser/browser_remote.go index 68017bda..c3b5628a 100644 --- a/pkg/browser/browser_remote.go +++ b/pkg/browser/browser_remote.go @@ -254,7 +254,7 @@ func resolveRemoteCDP(remoteURL string) (string, error) { defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return "", fmt.Errorf("/json/version returned HTTP %d", resp.StatusCode) + return "", fmt.Errorf("query /json/version at %s returned HTTP %d", versionURL, resp.StatusCode) } var ver struct {