diff --git a/weed/admin/handlers/file_browser_handlers.go b/weed/admin/handlers/file_browser_handlers.go index a4b8475c0..2ae64129b 100644 --- a/weed/admin/handlers/file_browser_handlers.go +++ b/weed/admin/handlers/file_browser_handlers.go @@ -9,6 +9,7 @@ import ( "mime/multipart" "net" "net/http" + "net/url" "os" "path" "path/filepath" @@ -398,7 +399,7 @@ func (h *FileBrowserHandlers) uploadFileToFiler(filePath string, fileHeader *mul // Create the upload URL - the httpClient will normalize to the correct scheme (http/https) // based on the https.client configuration in security.toml - uploadURL := fmt.Sprintf("%s%s", filerHttpAddress, cleanFilePath) + uploadURL := filerFileURL(filerHttpAddress, cleanFilePath) // Normalize the URL scheme based on TLS configuration uploadURL, err = h.httpClient.NormalizeHttpScheme(uploadURL) @@ -503,14 +504,16 @@ func (h *FileBrowserHandlers) validateAndCleanFilePath(filePath string) (string, return "", fmt.Errorf("path traversal not allowed") } - // Additional validation: ensure path doesn't contain dangerous characters - if strings.ContainsAny(cleanPath, "\x00\r\n") { - return "", fmt.Errorf("path contains invalid characters") - } - return cleanPath, nil } +// filerFileURL joins the filer HTTP address with a validated file path, URL-escaping +// the path so that control characters and other bytes that are legal in S3 object keys +// cannot inject into the HTTP request target. +func filerFileURL(filerHttpAddress, cleanFilePath string) string { + return filerHttpAddress + (&url.URL{Path: cleanFilePath}).EscapedPath() +} + // fetchFileContent fetches file content from the filer and returns the content or an error. func (h *FileBrowserHandlers) fetchFileContent(filePath string, timeout time.Duration) (string, error) { filerAddress := h.adminServer.GetFilerAddress() @@ -529,7 +532,7 @@ func (h *FileBrowserHandlers) fetchFileContent(filePath string, timeout time.Dur } // Create the file URL with proper scheme based on TLS configuration - fileURL := fmt.Sprintf("%s%s", filerHttpAddress, cleanFilePath) + fileURL := filerFileURL(filerHttpAddress, cleanFilePath) fileURL, err = h.httpClient.NormalizeHttpScheme(fileURL) if err != nil { return "", fmt.Errorf("failed to construct file URL: %w", err) @@ -597,7 +600,7 @@ func (h *FileBrowserHandlers) DownloadFile(w http.ResponseWriter, r *http.Reques } // Create the download URL with proper scheme based on TLS configuration - downloadURL := fmt.Sprintf("%s%s", filerHttpAddress, cleanFilePath) + downloadURL := filerFileURL(filerHttpAddress, cleanFilePath) downloadURL, err = h.httpClient.NormalizeHttpScheme(downloadURL) if err != nil { writeJSONError(w, http.StatusInternalServerError, "Failed to construct download URL: "+err.Error()) @@ -1043,7 +1046,7 @@ func (h *FileBrowserHandlers) isLikelyTextFile(filePath string, maxCheckSize int } // Create the file URL with proper scheme based on TLS configuration - fileURL := fmt.Sprintf("%s%s", filerHttpAddress, cleanFilePath) + fileURL := filerFileURL(filerHttpAddress, cleanFilePath) fileURL, err = h.httpClient.NormalizeHttpScheme(fileURL) if err != nil { glog.Errorf("Failed to normalize URL scheme: %v", err) diff --git a/weed/admin/handlers/file_browser_handlers_test.go b/weed/admin/handlers/file_browser_handlers_test.go new file mode 100644 index 000000000..583bcc142 --- /dev/null +++ b/weed/admin/handlers/file_browser_handlers_test.go @@ -0,0 +1,62 @@ +package handlers + +import ( + "testing" +) + +func TestValidateAndCleanFilePath_AllowsControlChars(t *testing.T) { + h := &FileBrowserHandlers{} + + // S3 object keys may legally contain any UTF-8 bytes, including control + // characters like \n, \r, and \x00. The admin UI must be able to browse + // and manage such entries rather than silently stripping or rejecting them. + cases := []struct { + in string + want string + }{ + {"/buckets/profilebuilder/3testGB.zip\n ", "/buckets/profilebuilder/3testGB.zip\n "}, + {"/foo\rbar", "/foo\rbar"}, + {"/foo\x00bar", "/foo\x00bar"}, + {"/normal/path.txt", "/normal/path.txt"}, + // Missing leading slash should be added back. + {"relative/path.txt", "/relative/path.txt"}, + // Duplicate slashes should be collapsed by path.Clean. + {"/a//b", "/a/b"}, + } + for _, tc := range cases { + got, err := h.validateAndCleanFilePath(tc.in) + if err != nil { + t.Errorf("validateAndCleanFilePath(%q) unexpected error: %v", tc.in, err) + continue + } + if got != tc.want { + t.Errorf("validateAndCleanFilePath(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} + +func TestValidateAndCleanFilePath_RejectsEmpty(t *testing.T) { + h := &FileBrowserHandlers{} + if _, err := h.validateAndCleanFilePath(""); err == nil { + t.Errorf("expected empty path rejection") + } +} + +func TestFilerFileURL_EscapesControlChars(t *testing.T) { + cases := []struct { + addr string + path string + want string + }{ + {"http://127.0.0.1:8888", "/buckets/profilebuilder/3testGB.zip\n ", "http://127.0.0.1:8888/buckets/profilebuilder/3testGB.zip%0A%20"}, + {"http://127.0.0.1:8888", "/buckets/profilebuilder/file\rname", "http://127.0.0.1:8888/buckets/profilebuilder/file%0Dname"}, + {"http://127.0.0.1:8888", "/buckets/profilebuilder/file\x00name", "http://127.0.0.1:8888/buckets/profilebuilder/file%00name"}, + // Plain path round-trips unchanged. + {"http://h:1", "/a/b.txt", "http://h:1/a/b.txt"}, + } + for _, tc := range cases { + if got := filerFileURL(tc.addr, tc.path); got != tc.want { + t.Errorf("filerFileURL(%q, %q) = %q, want %q", tc.addr, tc.path, got, tc.want) + } + } +}