From e07c59df0b850f5924d5b1683e8609661ddcf534 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Wed, 3 Jun 2026 10:24:05 +0200 Subject: [PATCH] fix: incorrect access control in public directory shares via rule path rebasing --- http/data.go | 16 +++++++ http/public.go | 5 +++ http/public_test.go | 105 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 126 insertions(+) diff --git a/http/data.go b/http/data.go index d591053e..37ad5ef0 100644 --- a/http/data.go +++ b/http/data.go @@ -3,6 +3,7 @@ package fbhttp import ( "log" "net/http" + gopath "path" "strconv" "github.com/tomasen/realip" @@ -23,10 +24,25 @@ type data struct { store *storage.Storage user *users.User raw interface{} + + // checkerPrefix is prepended to every path before evaluating rules. It is + // set when the user's filesystem has been rebased onto a subdirectory (as + // done for public shares), so that rules — which are relative to the user's + // original scope — are still matched against the real path instead of the + // rebased one. Empty for regular requests. + checkerPrefix string } // Check implements rules.Checker. func (d *data) Check(path string) bool { + // When the filesystem has been rebased (e.g. a public share rooted at a + // subdirectory), the incoming path is relative to that root. Resolve it + // back to the user's original scope before matching rules, otherwise rules + // targeting paths below the share root would be silently bypassed. + if d.checkerPrefix != "" { + path = gopath.Join(d.checkerPrefix, path) + } + if d.user.HideDotfiles && rules.MatchHidden(path) { return false } diff --git a/http/public.go b/http/public.go index 29679be8..bf49927c 100644 --- a/http/public.go +++ b/http/public.go @@ -67,6 +67,11 @@ var withHashFile = func(fn handleFunc) handleFunc { // set fs root to the shared file/folder d.user.Fs = afero.NewBasePathFs(d.user.Fs, basePath) + // the filesystem is now rebased onto basePath, so paths handed to the + // rule checker are relative to it. Resolve them back to the user's + // original scope so deny rules below the share root keep applying. + d.checkerPrefix = basePath + file, err = files.NewFileInfo(&files.FileOptions{ Fs: d.user.Fs, Path: filePath, diff --git a/http/public_test.go b/http/public_test.go index ea38ce51..bc57a9ca 100644 --- a/http/public_test.go +++ b/http/public_test.go @@ -10,6 +10,7 @@ import ( "github.com/asdine/storm/v3" "github.com/spf13/afero" + "github.com/filebrowser/filebrowser/v2/rules" "github.com/filebrowser/filebrowser/v2/settings" "github.com/filebrowser/filebrowser/v2/share" "github.com/filebrowser/filebrowser/v2/storage/bolt" @@ -143,6 +144,110 @@ func TestPublicShareHandlerAuthentication(t *testing.T) { } } +// TestPublicShareHandlerRules ensures that owner rules keep applying to paths +// below a shared directory, even though the share rebases the filesystem onto +// that directory. A deny rule relative to the owner's scope must not be +// bypassable by requesting the blocked path through the public share. +func TestPublicShareHandlerRules(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + handler handleFunc + path string + expectedStatusCode int + }{ + "blocked file via dl handler, 403": { + handler: publicDlHandler, + path: "h/private/secret.txt", + expectedStatusCode: 403, + }, + "blocked dir listing via share handler, 403": { + handler: publicShareHandler, + path: "h/private/", + expectedStatusCode: 403, + }, + "blocked dir download via dl handler, 403": { + handler: publicDlHandler, + path: "h/private/", + expectedStatusCode: 403, + }, + "allowed file via dl handler, 200": { + handler: publicDlHandler, + path: "h/public/readme.txt", + expectedStatusCode: 200, + }, + "allowed dir listing via share handler, 200": { + handler: publicShareHandler, + path: "h/public/", + expectedStatusCode: 200, + }, + } + + for name, tc := range testCases { + name, tc := name, tc + t.Run(name, func(t *testing.T) { + t.Parallel() + + dbPath := filepath.Join(t.TempDir(), "db") + db, err := storm.Open(dbPath) + if err != nil { + t.Fatalf("failed to open db: %v", err) + } + t.Cleanup(func() { + if err := db.Close(); err != nil { + t.Errorf("failed to close db: %v", err) + } + }) + + storage, err := bolt.NewStorage(db) + if err != nil { + t.Fatalf("failed to get storage: %v", err) + } + if err := storage.Share.Save(&share.Link{Hash: "h", UserID: 1, Path: "/projects"}); err != nil { + t.Fatalf("failed to save share: %v", err) + } + if err := storage.Users.Save(&users.User{ + Username: "username", + Password: "pw", + Perm: users.Permissions{Share: true, Download: true}, + Rules: []rules.Rule{ + {Allow: false, Path: "/projects/private"}, + }, + }); err != nil { + t.Fatalf("failed to save user: %v", err) + } + if err := storage.Settings.Save(&settings.Settings{Key: []byte("key")}); err != nil { + t.Fatalf("failed to save settings: %v", err) + } + + fs := afero.NewMemMapFs() + if err := afero.WriteFile(fs, "/projects/private/secret.txt", []byte("top secret"), 0o600); err != nil { + t.Fatalf("failed to write secret file: %v", err) + } + if err := afero.WriteFile(fs, "/projects/public/readme.txt", []byte("hello"), 0o600); err != nil { + t.Fatalf("failed to write public file: %v", err) + } + + storage.Users = &customFSUser{ + Store: storage.Users, + fs: fs, + } + + req := newHTTPRequest(t, func(r *http.Request) { r.URL.Path = tc.path }) + + recorder := httptest.NewRecorder() + handler := handle(tc.handler, "", storage, &settings.Server{}) + + handler.ServeHTTP(recorder, req) + result := recorder.Result() + defer result.Body.Close() + if result.StatusCode != tc.expectedStatusCode { + t.Errorf("expected status code %d, got status code %d", tc.expectedStatusCode, result.StatusCode) + } + }) + } +} + func newHTTPRequest(t *testing.T, requestModifiers ...func(*http.Request)) *http.Request { t.Helper() r, err := http.NewRequest(http.MethodGet, "h", http.NoBody)