fix: incorrect access control in public directory shares via rule path rebasing
This commit is contained in:
@@ -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
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user