From 39bdbc18db3cf3fd2946d0ca0b48f0f0207f6588 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 25 Oct 2026 06:06:21 -0700 Subject: [PATCH] admin: keep read-only sessions out of the filer /etc tree (#12057) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * admin: keep read-only sessions out of the filer /etc tree The file browser read routes are open to any authenticated session, so a read-only login could download or view arbitrary filer paths. The filer stores the credential and cluster configuration under /etc (identity JSON with plaintext S3 secret keys, SSE-S3 keys, remote storage config), which let a view-only session recover live secrets that the user API redacts. Gate the read paths on the session role: read-only sessions get 403 for /api/files/download, /view, /properties, /metadata on /etc paths, the browser page redirects away, and recursive metadata exports skip the /etc subtree for read-only sessions. Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * admin: only reject real .. path components in file browser paths path.Clean already resolves interior parent references, so any '..' surviving in the cleaned path can only be a leading escape from a relative input or part of a valid name. Matching on the substring rejected legitimate entries like /buckets/reports/report..csv. Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> * admin: cover the read-only /etc export exclusion with a test Pull the excluded-prefix selection into exportExcludedPrefixes so the role-dependent /etc skip is exercised directly — the cleanReadPath tests alone would still pass if the exclusion were dropped. Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- weed/admin/handlers/file_browser_handlers.go | 80 ++++++++++++------ .../handlers/file_browser_handlers_test.go | 82 ++++++++++++++++++- 2 files changed, 136 insertions(+), 26 deletions(-) diff --git a/weed/admin/handlers/file_browser_handlers.go b/weed/admin/handlers/file_browser_handlers.go index 15d7a24b03..085679b768 100644 --- a/weed/admin/handlers/file_browser_handlers.go +++ b/weed/admin/handlers/file_browser_handlers.go @@ -11,6 +11,7 @@ import ( "net/http" "os" "path" + "slices" "strconv" "strings" "time" @@ -50,9 +51,14 @@ func NewFileBrowserHandlers(adminServer *dash.AdminServer) *FileBrowserHandlers // ShowFileBrowser renders the file browser page func (h *FileBrowserHandlers) ShowFileBrowser(w http.ResponseWriter, r *http.Request) { // Get path from query parameter, default to root - path := defaultQuery(r.URL.Query().Get("path"), "/") - // Normalize Windows-style paths for consistency - path = util.CleanWindowsPath(path) + dirPath := path.Clean(util.CleanWindowsPath(defaultQuery(r.URL.Query().Get("path"), "/"))) + if !strings.HasPrefix(dirPath, "/") { + dirPath = "/" + dirPath + } + if dash.IsReadOnlyRole(dash.RoleFromContext(r.Context())) && restrictedBrowserPath(dirPath) { + http.Redirect(w, r, dash.P(r.Context(), "/admin?error=Insufficient permissions"), http.StatusSeeOther) + return + } // Get pagination parameters lastFileName := r.URL.Query().Get("lastFileName") @@ -66,7 +72,7 @@ func (h *FileBrowserHandlers) ShowFileBrowser(w http.ResponseWriter, r *http.Req } // Get file browser data with cursor-based pagination - browserData, err := h.adminServer.GetFileBrowser(path, "", lastFileName, pageSize) + browserData, err := h.adminServer.GetFileBrowser(dirPath, "", lastFileName, pageSize) if err != nil { writeJSONError(w, http.StatusInternalServerError, "Failed to get file browser data: "+err.Error()) return @@ -356,14 +362,46 @@ func (h *FileBrowserHandlers) validateAndCleanFilePath(filePath string) (string, cleanPath = "/" + cleanPath } - // Prevent path traversal attacks - if strings.Contains(cleanPath, "..") { + // Reject parent-directory components; names merely containing ".." are valid + if slices.Contains(strings.Split(cleanPath, "/"), "..") { return "", fmt.Errorf("path traversal not allowed") } return cleanPath, nil } +// restrictedBrowserPath reports whether a cleaned path lies inside the filer's +// /etc tree, which holds the credential store and cluster configuration. +func restrictedBrowserPath(cleanPath string) bool { + return cleanPath == strings.TrimSuffix(filer.DirectoryEtcRoot, "/") || + strings.HasPrefix(cleanPath, filer.DirectoryEtcRoot) +} + +// cleanReadPath validates the request's path parameter; read-only sessions are +// denied the /etc tree so the file browser cannot serve stored credentials. +func (h *FileBrowserHandlers) cleanReadPath(w http.ResponseWriter, r *http.Request) (string, bool) { + cleanPath, err := h.validateAndCleanFilePath(r.URL.Query().Get("path")) + if err != nil { + writeJSONError(w, http.StatusBadRequest, err.Error()) + return "", false + } + if dash.IsReadOnlyRole(dash.RoleFromContext(r.Context())) && restrictedBrowserPath(cleanPath) { + writeJSONError(w, http.StatusForbidden, "Access to this path requires admin access") + return "", false + } + return cleanPath, true +} + +// exportExcludedPrefixes lists the subtrees a metadata export skips; read-only +// sessions also lose /etc so stored credentials stay out of the dump. +func exportExcludedPrefixes(r *http.Request) []string { + prefixes := []string{filer.SystemLogDir} + if dash.IsReadOnlyRole(dash.RoleFromContext(r.Context())) { + prefixes = append(prefixes, strings.TrimSuffix(filer.DirectoryEtcRoot, "/")) + } + return prefixes +} + // fetchFileContent fetches file content via the filer gRPC service. It is // used for the "view as text" path, so the maxBytes cap matches the 1 MB // limit the caller already applies before invoking us. @@ -377,9 +415,8 @@ func (h *FileBrowserHandlers) fetchFileContent(filePath string, timeout time.Dur // gRPC service, so the admin file browser keeps working even when the filer // is started with -disableHttp=true. func (h *FileBrowserHandlers) DownloadFile(w http.ResponseWriter, r *http.Request) { - filePath := r.URL.Query().Get("path") - if filePath == "" { - writeJSONError(w, http.StatusBadRequest, "File path is required") + filePath, ok := h.cleanReadPath(w, r) + if !ok { return } inline := r.URL.Query().Get("inline") == "true" @@ -419,9 +456,8 @@ func (t *responseWriteTracker) Write(p []byte) (int, error) { // ViewFile handles file viewing requests (for text files, images, etc.) func (h *FileBrowserHandlers) ViewFile(w http.ResponseWriter, r *http.Request) { - filePath := r.URL.Query().Get("path") - if filePath == "" { - writeJSONError(w, http.StatusBadRequest, "File path is required") + filePath, ok := h.cleanReadPath(w, r) + if !ok { return } @@ -524,9 +560,8 @@ func (h *FileBrowserHandlers) ViewFile(w http.ResponseWriter, r *http.Request) { // GetFileProperties handles file properties requests func (h *FileBrowserHandlers) GetFileProperties(w http.ResponseWriter, r *http.Request) { - filePath := r.URL.Query().Get("path") - if filePath == "" { - writeJSONError(w, http.StatusBadRequest, "File path is required") + filePath, ok := h.cleanReadPath(w, r) + if !ok { return } @@ -630,23 +665,18 @@ func (h *FileBrowserHandlers) GetFileProperties(w http.ResponseWriter, r *http.R // FullEntry stream — the format weed shell fs.meta.load reads. Directories are // walked recursively via the filer BFS metadata stream. func (h *FileBrowserHandlers) ExportMetadata(w http.ResponseWriter, r *http.Request) { - filePath := r.URL.Query().Get("path") - if filePath == "" { - writeJSONError(w, http.StatusBadRequest, "File path is required") - return - } - cleanPath, err := h.validateAndCleanFilePath(filePath) - if err != nil { - writeJSONError(w, http.StatusBadRequest, err.Error()) + cleanPath, ok := h.cleanReadPath(w, r) + if !ok { return } + excludedPrefixes := exportExcludedPrefixes(r) tracker := &responseWriteTracker{ResponseWriter: w} - err = h.adminServer.WithFilerClient(func(client filer_pb.SeaweedFilerClient) error { + err := h.adminServer.WithFilerClient(func(client filer_pb.SeaweedFilerClient) error { stream, err := client.TraverseBfsMetadata(r.Context(), &filer_pb.TraverseBfsMetadataRequest{ Directory: cleanPath, - ExcludedPrefixes: []string{filer.SystemLogDir}, + ExcludedPrefixes: excludedPrefixes, }) if err != nil { return err diff --git a/weed/admin/handlers/file_browser_handlers_test.go b/weed/admin/handlers/file_browser_handlers_test.go index 65d05dad9b..f02fead273 100644 --- a/weed/admin/handlers/file_browser_handlers_test.go +++ b/weed/admin/handlers/file_browser_handlers_test.go @@ -3,9 +3,12 @@ package handlers import ( "net/http" "net/http/httptest" + "slices" + "strings" "testing" "github.com/seaweedfs/seaweedfs/weed/admin/dash" + "github.com/seaweedfs/seaweedfs/weed/filer" ) func TestValidateAndCleanFilePath_AllowsControlChars(t *testing.T) { @@ -26,6 +29,9 @@ func TestValidateAndCleanFilePath_AllowsControlChars(t *testing.T) { {"relative/path.txt", "/relative/path.txt"}, // Duplicate slashes should be collapsed by path.Clean. {"/a//b", "/a/b"}, + // ".." inside a file or directory name is not a parent component. + {"/buckets/reports/report..csv", "/buckets/reports/report..csv"}, + {"/a../b", "/a../b"}, } for _, tc := range cases { got, err := h.validateAndCleanFilePath(tc.in) @@ -46,6 +52,81 @@ func TestValidateAndCleanFilePath_RejectsEmpty(t *testing.T) { } } +func TestValidateAndCleanFilePath_RejectsParentComponents(t *testing.T) { + h := &FileBrowserHandlers{} + for _, p := range []string{"../etc", "../../etc/iam", "..", "a/../../b"} { + if _, err := h.validateAndCleanFilePath(p); err == nil { + t.Errorf("validateAndCleanFilePath(%q): expected traversal rejection", p) + } + } +} + +func TestCleanReadPath_DeniesEtcForReadOnly(t *testing.T) { + h := &FileBrowserHandlers{} + + denied := []string{"/etc", "/etc/iam", "/etc/iam/identities/root.json", + "/etc/s3/kek.json", "/etc/../etc/iam/identity.json", "/a/../../etc/x"} + for _, p := range denied { + req := httptest.NewRequest(http.MethodGet, "/api/files/download?path="+p, nil) + req = req.WithContext(dash.WithAuthContext(req.Context(), "ro", dash.RoleReadOnly, "csrf")) + w := httptest.NewRecorder() + + if _, ok := h.cleanReadPath(w, req); ok { + t.Errorf("path %q: expected denial for read-only session", p) + continue + } + if w.Code != http.StatusForbidden { + t.Errorf("path %q: expected 403, got %d", p, w.Code) + } + } +} + +func TestCleanReadPath_AllowsEtcForAdminAndNorRole(t *testing.T) { + h := &FileBrowserHandlers{} + + for _, role := range []string{"admin", ""} { + req := httptest.NewRequest(http.MethodGet, "/api/files/download?path=/etc/iam/identities/root.json", nil) + req = req.WithContext(dash.WithAuthContext(req.Context(), "u", role, "csrf")) + w := httptest.NewRecorder() + + if _, ok := h.cleanReadPath(w, req); !ok { + t.Errorf("role %q: expected /etc path to be allowed, got %d", role, w.Code) + } + } +} + +func TestCleanReadPath_AllowsReadOnlyOutsideEtc(t *testing.T) { + h := &FileBrowserHandlers{} + + for _, p := range []string{"/buckets/foo/bar.txt", "/etcx/file", "/etcetera", "/"} { + req := httptest.NewRequest(http.MethodGet, "/api/files/download?path="+p, nil) + req = req.WithContext(dash.WithAuthContext(req.Context(), "ro", dash.RoleReadOnly, "csrf")) + w := httptest.NewRecorder() + + if _, ok := h.cleanReadPath(w, req); !ok { + t.Errorf("path %q: expected allowed for read-only session, got %d", p, w.Code) + } + } +} + +func TestExportExcludedPrefixes_ReadOnlyDropsEtc(t *testing.T) { + etcPrefix := strings.TrimSuffix(filer.DirectoryEtcRoot, "/") + + req := httptest.NewRequest(http.MethodGet, "/api/files/metadata/export?path=/", nil) + req = req.WithContext(dash.WithAuthContext(req.Context(), "ro", dash.RoleReadOnly, "csrf")) + if got := exportExcludedPrefixes(req); !slices.Contains(got, etcPrefix) { + t.Errorf("read-only export should exclude %q, got %v", etcPrefix, got) + } + + for _, role := range []string{"admin", ""} { + req := httptest.NewRequest(http.MethodGet, "/api/files/metadata/export?path=/", nil) + req = req.WithContext(dash.WithAuthContext(req.Context(), "u", role, "csrf")) + if got := exportExcludedPrefixes(req); slices.Contains(got, etcPrefix) { + t.Errorf("role %q: export should keep %q, got %v", role, etcPrefix, got) + } + } +} + func TestListFolders_RejectsPathOutsideBuckets(t *testing.T) { h := &FileBrowserHandlers{adminServer: &dash.AdminServer{}} @@ -62,4 +143,3 @@ func TestListFolders_RejectsPathOutsideBuckets(t *testing.T) { } } } -