refactor: ScopedFs to avoid escaping symlinks

This commit is contained in:
Henrique Dias 2026-06-07 18:16:46 +02:00
parent 3406d3d7f9
commit 7c2c0a11b3
No known key found for this signature in database
15 changed files with 277 additions and 210 deletions

View file

@ -9,11 +9,9 @@ import (
"path/filepath"
"strings"
"github.com/spf13/afero"
"golang.org/x/crypto/bcrypt"
"github.com/filebrowser/filebrowser/v2/files"
"github.com/filebrowser/filebrowser/v2/share"
"golang.org/x/crypto/bcrypt"
)
var withHashFile = func(fn handleFunc) handleFunc {
@ -65,8 +63,11 @@ var withHashFile = func(fn handleFunc) handleFunc {
filePath = ifPath
}
// set fs root to the shared file/folder
d.user.Fs = afero.NewBasePathFs(d.user.Fs, basePath)
// set fs root to the shared file/folder. ScopedFs (not a bare
// BasePathFs) so the share is also symlink-confined: a link inside the
// shared subtree that points elsewhere in the owner's scope — outside
// the share — must not be followed.
d.user.Fs = files.NewScopedFs(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

View file

@ -12,13 +12,13 @@ import (
"testing"
"github.com/asdine/storm/v3"
"github.com/spf13/afero"
"github.com/filebrowser/filebrowser/v2/files"
"github.com/filebrowser/filebrowser/v2/settings"
"github.com/filebrowser/filebrowser/v2/share"
"github.com/filebrowser/filebrowser/v2/storage"
"github.com/filebrowser/filebrowser/v2/storage/bolt"
"github.com/filebrowser/filebrowser/v2/users"
"github.com/spf13/afero"
)
// symlinkShareStorage builds a storage whose single user is rooted at a real
@ -65,7 +65,7 @@ func symlinkShareStorage(t *testing.T) *storage.Storage {
}
st.Users = &customFSUser{
Store: st.Users,
fs: afero.NewBasePathFs(afero.NewOsFs(), scope),
fs: files.NewScopedFs(afero.NewOsFs(), scope),
}
return st
}

View file

@ -8,13 +8,13 @@ import (
"testing"
"github.com/asdine/storm/v3"
"github.com/spf13/afero"
"github.com/filebrowser/filebrowser/v2/files"
"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"
"github.com/filebrowser/filebrowser/v2/users"
"github.com/spf13/afero"
)
func TestPublicShareHandlerAuthentication(t *testing.T) {
@ -220,7 +220,7 @@ func TestPublicShareHandlerRules(t *testing.T) {
t.Fatalf("failed to save settings: %v", err)
}
fs := afero.NewBasePathFs(afero.NewOsFs(), t.TempDir())
fs := files.NewScopedFs(afero.NewOsFs(), t.TempDir())
if err := fs.MkdirAll("/projects/private", 0o755); err != nil {
t.Fatalf("failed to create private dir: %v", err)
}
@ -276,7 +276,9 @@ func (cu *customFSUser) Get(baseScope string, id interface{}) (*users.User, erro
if err != nil {
return nil, err
}
user.Fs = cu.fs
// Mirror production (users.User init), where a user's filesystem is always a
// scoped, symlink-confining ScopedFs rather than a bare afero.Fs.
user.Fs = files.NewScopedFs(cu.fs, "/")
return user, nil
}

View file

@ -10,11 +10,10 @@ import (
"path/filepath"
"strings"
"github.com/mholt/archives"
"github.com/filebrowser/filebrowser/v2/files"
"github.com/filebrowser/filebrowser/v2/fileutils"
"github.com/filebrowser/filebrowser/v2/users"
"github.com/mholt/archives"
)
func slashClean(name string) string {
@ -115,10 +114,6 @@ func getFiles(d *data, path, commonPath string) ([]archives.FileInfo, error) {
return nil, nil
}
if ok, err := files.WithinScope(d.user.Fs, path); err != nil || !ok {
return nil, nil
}
info, err := d.user.Fs.Stat(path)
if err != nil {
return nil, err

View file

@ -15,12 +15,11 @@ import (
"strings"
"time"
"github.com/shirou/gopsutil/v4/disk"
"github.com/spf13/afero"
fberrors "github.com/filebrowser/filebrowser/v2/errors"
"github.com/filebrowser/filebrowser/v2/files"
"github.com/filebrowser/filebrowser/v2/fileutils"
"github.com/shirou/gopsutil/v4/disk"
"github.com/spf13/afero"
)
var resourceGetHandler = withUser(func(w http.ResponseWriter, r *http.Request, d *data) (int, error) {
@ -228,15 +227,6 @@ func resourcePatchHandler(fileCache FileCache) handleFunc {
return http.StatusForbidden, nil
}
for _, p := range []string{src, dst} {
if ok, scopeErr := files.WithinScope(d.user.Fs, p); scopeErr != nil || !ok {
if scopeErr != nil {
return errToStatus(scopeErr), scopeErr
}
return http.StatusForbidden, nil
}
}
err = checkParent(src, dst)
if err != nil {
return http.StatusBadRequest, err
@ -304,10 +294,6 @@ func addVersionSuffix(source string, afs afero.Fs) string {
}
func writeFile(afs afero.Fs, dst string, in io.Reader, fileMode, dirMode fs.FileMode) (os.FileInfo, error) {
if ok, err := files.WithinScope(afs, dst); err != nil || !ok {
return nil, os.ErrPermission
}
dir, _ := path.Split(dst)
err := afs.MkdirAll(dir, dirMode)
if err != nil {

View file

@ -12,11 +12,9 @@ import (
"strings"
"time"
"golang.org/x/crypto/bcrypt"
fberrors "github.com/filebrowser/filebrowser/v2/errors"
"github.com/filebrowser/filebrowser/v2/files"
"github.com/filebrowser/filebrowser/v2/share"
"golang.org/x/crypto/bcrypt"
)
func withPermShare(fn handleFunc) handleFunc {
@ -103,20 +101,14 @@ var sharePostHandler = withPermShare(func(w http.ResponseWriter, r *http.Request
// Only allow sharing paths that currently exist. Otherwise a share could be
// created for a non-existent path and would silently start exposing
// whatever file later appears there.
//
// d.user.Fs is scoped, so Stat also refuses to follow a symlink whose target
// escapes the user's scope: that returns a permission error here and so
// blocks creating a share that points out of scope.
if _, err := d.user.Fs.Stat(r.URL.Path); err != nil {
return errToStatus(err), err
}
// Refuse to create a share whose on-disk target escapes the user's scope
// (e.g. via a symlink), mirroring the read/write guards. The public serve
// path already rejects these, but blocking creation avoids dangling shares.
if ok, err := files.WithinScope(d.user.Fs, r.URL.Path); err != nil || !ok {
if err != nil {
return errToStatus(err), err
}
return http.StatusForbidden, nil
}
var s *share.Link
var body share.CreateBody
if r.Body != nil {

View file

@ -11,9 +11,8 @@ import (
"strings"
"time"
"github.com/spf13/afero"
"github.com/filebrowser/filebrowser/v2/files"
"github.com/spf13/afero"
)
// keepUploadActive periodically touches the cache entry to prevent eviction during transfer
@ -45,10 +44,6 @@ func tusPostHandler(cache UploadCache) handleFunc {
return http.StatusForbidden, nil
}
if ok, scopeErr := files.WithinScope(d.user.Fs, r.URL.Path); scopeErr != nil || !ok {
return http.StatusForbidden, nil
}
file, err := files.NewFileInfo(&files.FileOptions{
Fs: d.user.Fs,
Path: r.URL.Path,
@ -167,10 +162,6 @@ func tusPatchHandler(cache UploadCache) handleFunc {
return http.StatusUnsupportedMediaType, nil
}
if ok, scopeErr := files.WithinScope(d.user.Fs, r.URL.Path); scopeErr != nil || !ok {
return http.StatusForbidden, nil
}
uploadOffset, err := getUploadOffset(r)
if err != nil {
return http.StatusBadRequest, fmt.Errorf("invalid upload offset")

View file

@ -12,6 +12,7 @@ import (
"github.com/golang-jwt/jwt/v5"
"github.com/spf13/afero"
"github.com/filebrowser/filebrowser/v2/files"
"github.com/filebrowser/filebrowser/v2/settings"
"github.com/filebrowser/filebrowser/v2/storage/bolt"
"github.com/filebrowser/filebrowser/v2/users"
@ -58,7 +59,7 @@ func TestTusHandlersRejectSymlinkScopeEscape(t *testing.T) {
}
st.Users = &customFSUser{
Store: st.Users,
fs: afero.NewBasePathFs(afero.NewOsFs(), userScope),
fs: files.NewScopedFs(afero.NewOsFs(), userScope),
}
// Forge a valid auth token for user ID 1.