From 28d1ef24ec0b04937186c4b64896c251fe7b5e47 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Mon, 13 Apr 2026 01:15:35 -0700 Subject: [PATCH] fix(admin): allow control chars in file paths when browsing filer (#9043) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(admin): allow control chars in file paths when browsing filer The admin UI rejected any path containing \x00, \r, or \n as "path contains invalid characters". These bytes are legal in S3 object keys, so objects created through the S3 API (or replicated via filer.sync) could exist on the filer but be unreachable from the admin UI — browse, download, and upload all failed with "Invalid file path". Drop the control-character rejection and instead URL-escape the path when constructing filer request URLs, so that such bytes cannot inject into the HTTP request target. Path traversal protection via path.Clean is unchanged. * test(admin): strengthen file path tests with byte-preserving checks Assert full expected output for validateAndCleanFilePath so silent stripping of control characters would fail the test, and cover \r and \x00 escaping in filerFileURL in addition to \n and space. --- weed/admin/handlers/file_browser_handlers.go | 21 ++++--- .../handlers/file_browser_handlers_test.go | 62 +++++++++++++++++++ 2 files changed, 74 insertions(+), 9 deletions(-) create mode 100644 weed/admin/handlers/file_browser_handlers_test.go 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) + } + } +}