diff --git a/auth/hook.go b/auth/hook.go index 60c75461..a6dc25b8 100644 --- a/auth/hook.go +++ b/auth/hook.go @@ -68,7 +68,7 @@ func (a *HookAuth) Auth(r *http.Request, usr users.Store, stg *settings.Settings case "block": return nil, os.ErrPermission case "pass": - u, err := a.Users.Get(a.Server.Root, a.Cred.Username) + u, err := a.Users.Get(a.Server.Root, a.Server.FollowExternalSymlinks, a.Cred.Username) if err != nil || !users.CheckPwd(a.Cred.Password, u.Password) { return nil, os.ErrPermission } @@ -129,7 +129,7 @@ func (a *HookAuth) GetValues(s string) { // SaveUser updates the existing user or creates a new one when not found func (a *HookAuth) SaveUser() (*users.User, error) { - u, err := a.Users.Get(a.Server.Root, a.Cred.Username) + u, err := a.Users.Get(a.Server.Root, a.Server.FollowExternalSymlinks, a.Cred.Username) if err != nil && !errors.Is(err, fberrors.ErrNotExist) { return nil, err } diff --git a/auth/json.go b/auth/json.go index 2284dc7f..be8ad1dc 100644 --- a/auth/json.go +++ b/auth/json.go @@ -55,7 +55,7 @@ func (a JSONAuth) Auth(r *http.Request, usr users.Store, _ *settings.Settings, s } } - u, err := usr.Get(srv.Root, cred.Username) + u, err := usr.Get(srv.Root, srv.FollowExternalSymlinks, cred.Username) hash := dummyHash if err == nil { diff --git a/auth/none.go b/auth/none.go index c9381a83..30ea7129 100644 --- a/auth/none.go +++ b/auth/none.go @@ -15,7 +15,7 @@ type NoAuth struct{} // Auth uses authenticates user 1. func (a NoAuth) Auth(_ *http.Request, usr users.Store, _ *settings.Settings, srv *settings.Server) (*users.User, error) { - return usr.Get(srv.Root, uint(1)) + return usr.Get(srv.Root, srv.FollowExternalSymlinks, uint(1)) } // LoginPage tells that no auth doesn't require a login page. diff --git a/auth/proxy.go b/auth/proxy.go index 57eddd4a..ab6227d4 100644 --- a/auth/proxy.go +++ b/auth/proxy.go @@ -20,7 +20,7 @@ type ProxyAuth struct { // Auth authenticates the user via an HTTP header. func (a ProxyAuth) Auth(r *http.Request, usr users.Store, setting *settings.Settings, srv *settings.Server) (*users.User, error) { username := r.Header.Get(a.Header) - user, err := usr.Get(srv.Root, username) + user, err := usr.Get(srv.Root, srv.FollowExternalSymlinks, username) if errors.Is(err, fberrors.ErrNotExist) { return a.createUser(usr, setting, srv, username) } diff --git a/auth/proxy_test.go b/auth/proxy_test.go index 86fb7102..df520a2c 100644 --- a/auth/proxy_test.go +++ b/auth/proxy_test.go @@ -13,7 +13,7 @@ type mockUserStore struct { users map[string]*users.User } -func (m *mockUserStore) Get(_ string, id interface{}) (*users.User, error) { +func (m *mockUserStore) Get(_ string, _ bool, id interface{}) (*users.User, error) { if v, ok := id.(string); ok { if u, ok := m.users[v]; ok { return u, nil @@ -22,14 +22,14 @@ func (m *mockUserStore) Get(_ string, id interface{}) (*users.User, error) { return nil, fberrors.ErrNotExist } -func (m *mockUserStore) Gets(_ string) ([]*users.User, error) { return nil, nil } -func (m *mockUserStore) Update(_ *users.User, _ ...string) error { return nil } +func (m *mockUserStore) Gets(_ string, _ bool) ([]*users.User, error) { return nil, nil } +func (m *mockUserStore) Update(_ *users.User, _ ...string) error { return nil } func (m *mockUserStore) Save(user *users.User) error { m.users[user.Username] = user return nil } func (m *mockUserStore) Delete(_ interface{}) error { return nil } -func (m *mockUserStore) LastUpdate(_ uint) int64 { return 0 } +func (m *mockUserStore) LastUpdate(_ uint) int64 { return 0 } func TestProxyAuthCreateUserRestrictsDefaults(t *testing.T) { t.Parallel() diff --git a/cmd/config.go b/cmd/config.go index e3bb2b86..cf923eac 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -229,6 +229,7 @@ func printSettings(ser *settings.Server, set *settings.Settings, auther auth.Aut fmt.Fprintf(w, "\tThumbnails Enabled:\t%t\n", ser.EnableThumbnails) fmt.Fprintf(w, "\tResize Preview:\t%t\n", ser.ResizePreview) fmt.Fprintf(w, "\tType Detection by Header:\t%t\n", ser.TypeDetectionByHeader) + fmt.Fprintf(w, "\tFollow External Symlinks:\t%t\n", ser.FollowExternalSymlinks) fmt.Fprintln(w, "\nTUS:") fmt.Fprintf(w, "\tChunk size:\t%d\n", set.Tus.ChunkSize) diff --git a/cmd/root.go b/cmd/root.go index 13139a7e..d7f7ea50 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -111,6 +111,7 @@ func addServerFlags(flags *pflag.FlagSet) { flags.Bool("disableExec", true, "disables Command Runner feature") flags.Bool("disableTypeDetectionByHeader", false, "disables type detection by reading file headers") flags.Bool("disableImageResolutionCalc", false, "disables image resolution calculation by reading image files") + flags.Bool("followExternalSymlinks", false, "follow symlinks whose target is outside the user scope (unsafe)") } var rootCmd = &cobra.Command{ @@ -353,6 +354,10 @@ func getServerSettings(v *viper.Viper, st *storage.Storage) (*settings.Server, e server.EnableExec = !v.GetBool("disableExec") } + if v.IsSet("followExternalSymlinks") { + server.FollowExternalSymlinks = v.GetBool("followExternalSymlinks") + } + if isAddrSet && isSocketSet { return nil, errors.New("--socket flag cannot be used with --address, --port, --key nor --cert") } @@ -369,6 +374,13 @@ func getServerSettings(v *viper.Viper, st *storage.Storage) (*settings.Server, e log.Println("WARNING: read https://github.com/filebrowser/filebrowser/issues/5199") } + if server.FollowExternalSymlinks { + log.Println("WARNING: Following external symlinks enabled!") + log.Println("WARNING: Symlinks pointing outside a user's scope will be followed,") + log.Println("WARNING: which can expose files outside that scope. Only enable this if") + log.Println("WARNING: you fully understand and trust the contents of every user scope.") + } + return server, nil } @@ -446,19 +458,20 @@ func quickSetup(v *viper.Viper, s *storage.Storage) error { } ser := &settings.Server{ - BaseURL: v.GetString("baseURL"), - Port: v.GetString("port"), - Log: v.GetString("log"), - TLSKey: v.GetString("key"), - TLSCert: v.GetString("cert"), - Address: v.GetString("address"), - Root: v.GetString("root"), - TokenExpirationTime: v.GetString("tokenExpirationTime"), - EnableThumbnails: !v.GetBool("disableThumbnails"), - ResizePreview: !v.GetBool("disablePreviewResize"), - EnableExec: !v.GetBool("disableExec"), - TypeDetectionByHeader: !v.GetBool("disableTypeDetectionByHeader"), - ImageResolutionCal: !v.GetBool("disableImageResolutionCalc"), + BaseURL: v.GetString("baseURL"), + Port: v.GetString("port"), + Log: v.GetString("log"), + TLSKey: v.GetString("key"), + TLSCert: v.GetString("cert"), + Address: v.GetString("address"), + Root: v.GetString("root"), + TokenExpirationTime: v.GetString("tokenExpirationTime"), + EnableThumbnails: !v.GetBool("disableThumbnails"), + ResizePreview: !v.GetBool("disablePreviewResize"), + EnableExec: !v.GetBool("disableExec"), + TypeDetectionByHeader: !v.GetBool("disableTypeDetectionByHeader"), + ImageResolutionCal: !v.GetBool("disableImageResolutionCalc"), + FollowExternalSymlinks: v.GetBool("followExternalSymlinks"), } err = s.Settings.SaveServer(ser) diff --git a/cmd/rules.go b/cmd/rules.go index bdb1d1cf..f3e00bb6 100644 --- a/cmd/rules.go +++ b/cmd/rules.go @@ -36,7 +36,7 @@ func runRules(st *storage.Storage, cmd *cobra.Command, usersFn func(*users.User) } if id != nil { var user *users.User - user, err = st.Users.Get("", id) + user, err = st.Users.Get("", false, id) if err != nil { return err } diff --git a/cmd/users_export.go b/cmd/users_export.go index 9bbec6d8..768c381f 100644 --- a/cmd/users_export.go +++ b/cmd/users_export.go @@ -15,7 +15,7 @@ var usersExportCmd = &cobra.Command{ path to the file where you want to write the users.`, Args: jsonYamlArg, RunE: withStore(func(_ *cobra.Command, args []string, st *store) error { - list, err := st.Users.Gets("") + list, err := st.Users.Gets("", false) if err != nil { return err } diff --git a/cmd/users_find.go b/cmd/users_find.go index 09bc8d47..fc91af86 100644 --- a/cmd/users_find.go +++ b/cmd/users_find.go @@ -36,14 +36,14 @@ var findUsers = withStore(func(_ *cobra.Command, args []string, st *store) error if len(args) == 1 { username, id := parseUsernameOrID(args[0]) if username != "" { - user, err = st.Users.Get("", username) + user, err = st.Users.Get("", false, username) } else { - user, err = st.Users.Get("", id) + user, err = st.Users.Get("", false, id) } list = []*users.User{user} } else { - list, err = st.Users.Gets("") + list, err = st.Users.Gets("", false) } if err != nil { diff --git a/cmd/users_import.go b/cmd/users_import.go index 73effca6..0db3704f 100644 --- a/cmd/users_import.go +++ b/cmd/users_import.go @@ -40,7 +40,7 @@ list or set it to 0.`, } for _, user := range list { - err = user.Clean("") + err = user.Clean("", false) if err != nil { return err } @@ -52,7 +52,7 @@ list or set it to 0.`, } if replace { - oldUsers, userImportErr := st.Users.Gets("") + oldUsers, userImportErr := st.Users.Gets("", false) if userImportErr != nil { return userImportErr } @@ -76,7 +76,7 @@ list or set it to 0.`, } for _, user := range list { - onDB, err := st.Users.Get("", user.ID) + onDB, err := st.Users.Get("", false, user.ID) // User exists in DB. if err == nil { @@ -88,7 +88,7 @@ list or set it to 0.`, // with the new username. If there is, print an error and cancel the // operation if user.Username != onDB.Username { - if conflictuous, err := st.Users.Get("", user.Username); err == nil { + if conflictuous, err := st.Users.Get("", false, user.Username); err == nil { return usernameConflictError(user.Username, conflictuous.ID, user.ID) } } diff --git a/cmd/users_update.go b/cmd/users_update.go index e9a484fc..12484342 100644 --- a/cmd/users_update.go +++ b/cmd/users_update.go @@ -43,9 +43,9 @@ options you want to change.`, user *users.User ) if id != 0 { - user, err = st.Users.Get("", id) + user, err = st.Users.Get("", false, id) } else { - user, err = st.Users.Get("", username) + user, err = st.Users.Get("", false, username) } if err != nil { return err diff --git a/files/fs_test.go b/files/fs_test.go new file mode 100644 index 00000000..5974b20a --- /dev/null +++ b/files/fs_test.go @@ -0,0 +1,121 @@ +package files + +import ( + "os" + "path/filepath" + "testing" + + "github.com/spf13/afero" +) + +// TestNewFs verifies that NewFs picks the right implementation and that the +// follow-external-symlinks toggle flips whether a symlink pointing outside the +// scope is honored. +func TestNewFs(t *testing.T) { + base := t.TempDir() + scope := filepath.Join(base, "srv") + outside := filepath.Join(base, "outside") + for _, d := range []string{scope, outside} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(outside, "secret.txt"), []byte("secret"), 0o644); err != nil { + t.Fatal(err) + } + // A symlink lexically inside the scope whose target resolves outside it. + if err := os.Symlink(outside, filepath.Join(scope, "escape")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + + t.Run("disabled returns a ScopedFs that rejects the escaping symlink", func(t *testing.T) { + fs := NewFs(afero.NewOsFs(), scope, false) + if _, ok := fs.(*ScopedFs); !ok { + t.Fatalf("expected *ScopedFs, got %T", fs) + } + if _, err := fs.Stat("/escape"); !os.IsPermission(err) { + t.Fatalf("expected stat of escaping symlink to be rejected, got %v", err) + } + }) + + t.Run("enabled returns a BasePathFs that follows the escaping symlink", func(t *testing.T) { + fs := NewFs(afero.NewOsFs(), scope, true) + if _, ok := fs.(*afero.BasePathFs); !ok { + t.Fatalf("expected *afero.BasePathFs, got %T", fs) + } + if _, err := fs.Stat("/escape"); err != nil { + t.Fatalf("expected escaping symlink to be followed, got %v", err) + } + b, err := afero.ReadFile(fs, "/escape/secret.txt") + if err != nil { + t.Fatalf("expected to read through escaping symlink, got %v", err) + } + if string(b) != "secret" { + t.Fatalf("got %q, want %q", b, "secret") + } + + // The link must also appear in a directory listing (the symptom in #5998). + entries, err := afero.ReadDir(fs, "/") + if err != nil { + t.Fatal(err) + } + var found bool + for _, e := range entries { + if e.Name() == "escape" { + found = true + } + } + if !found { + t.Fatal("expected escaping symlink to appear in the listing") + } + }) +} + +// TestBasePath verifies BasePath extracts the underlying *afero.BasePathFs from +// either filesystem NewFs may return, so User.FullPath keeps working. +func TestBasePath(t *testing.T) { + root := t.TempDir() + osFs := afero.NewOsFs() + + for _, tc := range []struct { + name string + followExternal bool + }{ + {"ScopedFs", false}, + {"BasePathFs", true}, + } { + t.Run(tc.name, func(t *testing.T) { + fs := NewFs(osFs, root, tc.followExternal) + base := BasePath(fs) + if base == nil { + t.Fatalf("expected non-nil base for %T", fs) + } + got := afero.FullBaseFsPath(base, "/x") + want := filepath.Join(root, "x") + if got != want { + t.Fatalf("FullBaseFsPath: got %q, want %q", got, want) + } + }) + } + + if got := BasePath(osFs); got != nil { + t.Fatalf("expected nil base for a plain OsFs, got %v", got) + } +} + +// TestFileInfoRealPathUsesBasePathFsRealPath mirrors +// TestFileInfoRealPathUsesScopedFsRealPath for the follow-external-symlinks case, +// where the user filesystem is a bare BasePathFs. +func TestFileInfoRealPathUsesBasePathFsRealPath(t *testing.T) { + root := t.TempDir() + file := &FileInfo{ + Fs: NewFs(afero.NewOsFs(), root, true), + Path: "/root/downloads", + } + + got := file.RealPath() + want := filepath.Join(root, "root", "downloads") + if got != want { + t.Fatalf("got %q, want %q", got, want) + } +} diff --git a/files/scoped.go b/files/scoped.go index dcc27a12..97ce32d0 100644 --- a/files/scoped.go +++ b/files/scoped.go @@ -37,8 +37,31 @@ func NewScopedFs(source afero.Fs, path string) *ScopedFs { return &ScopedFs{base: afero.NewBasePathFs(source, path).(*afero.BasePathFs)} } -// Base returns the underlying *afero.BasePathFs. -func (s *ScopedFs) Base() *afero.BasePathFs { return s.base } +// NewFs builds a user filesystem rooted at path. When followExternal is true it +// returns a bare BasePathFs, so symlinks whose target resolves outside the scope +// are followed; otherwise it returns a ScopedFs that refuses to follow them. +func NewFs(source afero.Fs, path string, followExternal bool) afero.Fs { + if followExternal { + return afero.NewBasePathFs(source, path) + } + return NewScopedFs(source, path) +} + +// BasePath returns the underlying *afero.BasePathFs of a user filesystem built +// by NewFs, whether it is a *ScopedFs or a bare *afero.BasePathFs, or nil if it +// is neither. +func BasePath(fs afero.Fs) *afero.BasePathFs { + switch f := fs.(type) { + case *ScopedFs: + return f.BasePathFs() + case *afero.BasePathFs: + return f + } + return nil +} + +// BasePathFs returns the underlying *afero.BasePathFs. +func (s *ScopedFs) BasePathFs() *afero.BasePathFs { return s.base } // RealPath resolves a scoped path to the real on-disk path by delegating to // the underlying BasePathFs. This is needed by callers that need the actual diff --git a/http/auth.go b/http/auth.go index 4381e86c..137824d5 100644 --- a/http/auth.go +++ b/http/auth.go @@ -102,7 +102,7 @@ func withUser(fn handleFunc) handleFunc { w.Header().Add("X-Renew-Token", "true") } - d.user, err = d.store.Users.Get(d.server.Root, tk.User.ID) + d.user, err = d.store.Users.Get(d.server.Root, d.server.FollowExternalSymlinks, tk.User.ID) if err != nil { return http.StatusInternalServerError, err } diff --git a/http/public.go b/http/public.go index a882f2f7..211f8a87 100644 --- a/http/public.go +++ b/http/public.go @@ -27,7 +27,7 @@ var withHashFile = func(fn handleFunc) handleFunc { return status, err } - user, err := d.store.Users.Get(d.server.Root, link.UserID) + user, err := d.store.Users.Get(d.server.Root, d.server.FollowExternalSymlinks, link.UserID) if err != nil { return errToStatus(err), err } @@ -63,11 +63,12 @@ var withHashFile = func(fn handleFunc) handleFunc { filePath = ifPath } - // 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) + // set fs root to the shared file/folder. Unless external symlinks are + // explicitly allowed, this is a 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.NewFs(d.user.Fs, basePath, d.server.FollowExternalSymlinks) // 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 diff --git a/http/public_symlink_test.go b/http/public_symlink_test.go index fa631c82..0343046e 100644 --- a/http/public_symlink_test.go +++ b/http/public_symlink_test.go @@ -126,6 +126,86 @@ func TestPublicShareSymlinkListingOmitsEscapingLink(t *testing.T) { } } +// With Server.FollowExternalSymlinks enabled (the opt-in for issue #5998), a +// symlink inside a public share that points outside it is followed: the file +// behind it downloads and the link shows up in the share listing. This is the +// inverse of TestPublicShareSymlinkDescendantDisclosure / ...ListingOmitsEscapingLink. +func TestPublicShareSymlinkFollowedWhenEnabled(t *testing.T) { + scope := t.TempDir() + if err := os.MkdirAll(filepath.Join(scope, "shared"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(scope, "outside"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(scope, "outside", "data.txt"), []byte("payload"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(scope, "outside"), filepath.Join(scope, "shared", "link")); err != nil { + t.Skipf("cannot create symlink on this platform: %v", err) + } + + db, err := storm.Open(filepath.Join(t.TempDir(), "db")) + if err != nil { + t.Fatalf("failed to open db: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + + st, err := bolt.NewStorage(db) + if err != nil { + t.Fatalf("failed to get storage: %v", err) + } + if err := st.Share.Save(&share.Link{Hash: "h", UserID: 1, Path: "/shared"}); err != nil { + t.Fatalf("failed to save share: %v", err) + } + if err := st.Users.Save(&users.User{ + Username: "username", + Password: "pw", + Perm: users.Permissions{Share: true, Download: true}, + }); err != nil { + t.Fatalf("failed to save user: %v", err) + } + if err := st.Settings.Save(&settings.Settings{Key: []byte("key")}); err != nil { + t.Fatalf("failed to save settings: %v", err) + } + // Follow-external mode: the user filesystem is a bare BasePathFs. + st.Users = &customFSUser{ + Store: st.Users, + fs: files.NewFs(afero.NewOsFs(), scope, true), + followExternal: true, + } + + srv := &settings.Server{FollowExternalSymlinks: true} + + // The file behind the symlink downloads. + req := newHTTPRequest(t, func(r *http.Request) { r.URL.Path = "h/link/data.txt" }) + recorder := httptest.NewRecorder() + handle(publicDlHandler, "", st, srv).ServeHTTP(recorder, req) + result := recorder.Result() + defer result.Body.Close() + body, _ := io.ReadAll(result.Body) + if result.StatusCode != http.StatusOK { + t.Fatalf("expected to download file behind followed symlink, got status=%d body=%q", result.StatusCode, string(body)) + } + if !strings.Contains(string(body), "payload") { + t.Fatalf("expected payload content, got %q", string(body)) + } + + // The link shows up in the share root listing. + req = newHTTPRequest(t, func(r *http.Request) { r.URL.Path = "h/" }) + recorder = httptest.NewRecorder() + handle(publicShareHandler, "", st, srv).ServeHTTP(recorder, req) + result = recorder.Result() + defer result.Body.Close() + body, _ = io.ReadAll(result.Body) + if result.StatusCode != http.StatusOK { + t.Fatalf("share root listing failed: status=%d body=%q", result.StatusCode, string(body)) + } + if !strings.Contains(string(body), "\"link\"") { + t.Fatalf("expected followed symlink to appear in listing: %s", string(body)) + } +} + // Reproduces the archive variant of GHSA-hf77-9m7w-fq8q: downloading the whole // public share as a zip must not pull in files reached through a symlinked // descendant. diff --git a/http/public_test.go b/http/public_test.go index 0b0b7677..c728102d 100644 --- a/http/public_test.go +++ b/http/public_test.go @@ -269,16 +269,24 @@ func newHTTPRequest(t *testing.T, requestModifiers ...func(*http.Request)) *http type customFSUser struct { users.Store fs afero.Fs + // followExternal mirrors Server.FollowExternalSymlinks: when set, the + // provided fs is used as-is (a bare BasePathFs that follows symlinks); + // otherwise it is wrapped in a symlink-confining ScopedFs. + followExternal bool } -func (cu *customFSUser) Get(baseScope string, id interface{}) (*users.User, error) { - user, err := cu.Store.Get(baseScope, id) +func (cu *customFSUser) Get(baseScope string, followExternalSymlinks bool, id interface{}) (*users.User, error) { + user, err := cu.Store.Get(baseScope, followExternalSymlinks, id) if err != nil { return nil, err } - // 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, "/") + // Inject a filesystem rooted at the test's temp scope, standing in for the + // one users.User.Clean would build in production. + if cu.followExternal { + user.Fs = cu.fs + } else { + user.Fs = files.NewScopedFs(cu.fs, "/") + } return user, nil } diff --git a/http/users.go b/http/users.go index e61ab00b..13092e3e 100644 --- a/http/users.go +++ b/http/users.go @@ -71,7 +71,7 @@ func withSelfOrAdmin(fn handleFunc) handleFunc { } var usersGetHandler = withAdmin(func(w http.ResponseWriter, r *http.Request, d *data) (int, error) { - users, err := d.store.Users.Gets(d.server.Root) + users, err := d.store.Users.Gets(d.server.Root, d.server.FollowExternalSymlinks) if err != nil { return http.StatusInternalServerError, err } @@ -88,7 +88,7 @@ var usersGetHandler = withAdmin(func(w http.ResponseWriter, r *http.Request, d * }) var userGetHandler = withSelfOrAdmin(func(w http.ResponseWriter, r *http.Request, d *data) (int, error) { - u, err := d.store.Users.Get(d.server.Root, d.raw.(uint)) + u, err := d.store.Users.Get(d.server.Root, d.server.FollowExternalSymlinks, d.raw.(uint)) if errors.Is(err, fberrors.ErrNotExist) { return http.StatusNotFound, err } @@ -228,7 +228,7 @@ var userPutHandler = withSelfOrAdmin(func(w http.ResponseWriter, r *http.Request } } else { var suser *users.User - suser, err = d.store.Users.Get(d.server.Root, d.raw.(uint)) + suser, err = d.store.Users.Get(d.server.Root, d.server.FollowExternalSymlinks, d.raw.(uint)) if err != nil { return http.StatusInternalServerError, err } diff --git a/settings/settings.go b/settings/settings.go index d71be16a..44297fd5 100644 --- a/settings/settings.go +++ b/settings/settings.go @@ -47,21 +47,22 @@ func (s *Settings) GetRules() []rules.Rule { // Server specific settings. type Server struct { - Root string `json:"root"` - BaseURL string `json:"baseURL"` - Socket string `json:"socket"` - TLSKey string `json:"tlsKey"` - TLSCert string `json:"tlsCert"` - Port string `json:"port"` - Address string `json:"address"` - Log string `json:"log"` - EnableThumbnails bool `json:"enableThumbnails"` - ResizePreview bool `json:"resizePreview"` - EnableExec bool `json:"enableExec"` - TypeDetectionByHeader bool `json:"typeDetectionByHeader"` - ImageResolutionCal bool `json:"imageResolutionCalculation"` - AuthHook string `json:"authHook"` - TokenExpirationTime string `json:"tokenExpirationTime"` + Root string `json:"root"` + BaseURL string `json:"baseURL"` + Socket string `json:"socket"` + TLSKey string `json:"tlsKey"` + TLSCert string `json:"tlsCert"` + Port string `json:"port"` + Address string `json:"address"` + Log string `json:"log"` + EnableThumbnails bool `json:"enableThumbnails"` + ResizePreview bool `json:"resizePreview"` + EnableExec bool `json:"enableExec"` + TypeDetectionByHeader bool `json:"typeDetectionByHeader"` + ImageResolutionCal bool `json:"imageResolutionCalculation"` + AuthHook string `json:"authHook"` + TokenExpirationTime string `json:"tokenExpirationTime"` + FollowExternalSymlinks bool `json:"followExternalSymlinks"` } // Clean cleans any variables that might need cleaning. diff --git a/users/storage.go b/users/storage.go index 33cfc9c4..32f10e4d 100644 --- a/users/storage.go +++ b/users/storage.go @@ -19,8 +19,8 @@ type StorageBackend interface { } type Store interface { - Get(baseScope string, id interface{}) (user *User, err error) - Gets(baseScope string) ([]*User, error) + Get(baseScope string, followExternalSymlinks bool, id interface{}) (user *User, err error) + Gets(baseScope string, followExternalSymlinks bool) ([]*User, error) Update(user *User, fields ...string) error Save(user *User) error Delete(id interface{}) error @@ -45,26 +45,26 @@ func NewStorage(back StorageBackend) *Storage { // Get allows you to get a user by its name or username. The provided // id must be a string for username lookup or a uint for id lookup. If id // is neither, a ErrInvalidDataType will be returned. -func (s *Storage) Get(baseScope string, id interface{}) (user *User, err error) { +func (s *Storage) Get(baseScope string, followExternalSymlinks bool, id interface{}) (user *User, err error) { user, err = s.back.GetBy(id) if err != nil { return } - if err := user.Clean(baseScope); err != nil { + if err := user.Clean(baseScope, followExternalSymlinks); err != nil { return nil, err } return } // Gets gets a list of all users. -func (s *Storage) Gets(baseScope string) ([]*User, error) { +func (s *Storage) Gets(baseScope string, followExternalSymlinks bool) ([]*User, error) { users, err := s.back.Gets() if err != nil { return nil, err } for _, user := range users { - if err := user.Clean(baseScope); err != nil { + if err := user.Clean(baseScope, followExternalSymlinks); err != nil { return nil, err } } @@ -74,7 +74,7 @@ func (s *Storage) Gets(baseScope string) ([]*User, error) { // Update updates a user in the database. func (s *Storage) Update(user *User, fields ...string) error { - err := user.Clean("", fields...) + err := user.Clean("", false, fields...) if err != nil { return err } @@ -92,7 +92,7 @@ func (s *Storage) Update(user *User, fields ...string) error { // Save saves the user in a storage. func (s *Storage) Save(user *User) error { - if err := user.Clean(""); err != nil { + if err := user.Clean("", false); err != nil { return err } diff --git a/users/users.go b/users/users.go index 6be3b09d..09db995a 100644 --- a/users/users.go +++ b/users/users.go @@ -19,23 +19,23 @@ const ( // User describes a user. type User struct { - ID uint `storm:"id,increment" json:"id"` - Username string `storm:"unique" json:"username"` - Password string `json:"password"` - Scope string `json:"scope"` - Locale string `json:"locale"` - LockPassword bool `json:"lockPassword"` - ViewMode ViewMode `json:"viewMode"` - SingleClick bool `json:"singleClick"` - RedirectAfterCopyMove bool `json:"redirectAfterCopyMove"` - Perm Permissions `json:"perm"` - Commands []string `json:"commands"` - Sorting files.Sorting `json:"sorting"` - Fs *files.ScopedFs `json:"-" yaml:"-"` - Rules []rules.Rule `json:"rules"` - HideDotfiles bool `json:"hideDotfiles"` - DateFormat bool `json:"dateFormat"` - AceEditorTheme string `json:"aceEditorTheme"` + ID uint `storm:"id,increment" json:"id"` + Username string `storm:"unique" json:"username"` + Password string `json:"password"` + Scope string `json:"scope"` + Locale string `json:"locale"` + LockPassword bool `json:"lockPassword"` + ViewMode ViewMode `json:"viewMode"` + SingleClick bool `json:"singleClick"` + RedirectAfterCopyMove bool `json:"redirectAfterCopyMove"` + Perm Permissions `json:"perm"` + Commands []string `json:"commands"` + Sorting files.Sorting `json:"sorting"` + Fs afero.Fs `json:"-" yaml:"-"` + Rules []rules.Rule `json:"rules"` + HideDotfiles bool `json:"hideDotfiles"` + DateFormat bool `json:"dateFormat"` + AceEditorTheme string `json:"aceEditorTheme"` } // GetRules implements rules.Provider. @@ -55,7 +55,7 @@ var checkableFields = []string{ // Clean cleans up a user and verifies if all its fields // are alright to be saved. -func (u *User) Clean(baseScope string, fields ...string) error { +func (u *User) Clean(baseScope string, followExternalSymlinks bool, fields ...string) error { if len(fields) == 0 { fields = checkableFields } @@ -92,7 +92,7 @@ func (u *User) Clean(baseScope string, fields ...string) error { if u.Fs == nil { scope := u.Scope scope = filepath.Join(baseScope, filepath.Join("/", scope)) - u.Fs = files.NewScopedFs(afero.NewOsFs(), scope) + u.Fs = files.NewFs(afero.NewOsFs(), scope, followExternalSymlinks) } return nil @@ -100,5 +100,5 @@ func (u *User) Clean(baseScope string, fields ...string) error { // FullPath gets the full path for a user's relative path. func (u *User) FullPath(path string) string { - return afero.FullBaseFsPath(u.Fs.Base(), path) + return afero.FullBaseFsPath(files.BasePath(u.Fs), path) } diff --git a/users/users_test.go b/users/users_test.go new file mode 100644 index 00000000..87171aaf --- /dev/null +++ b/users/users_test.go @@ -0,0 +1,43 @@ +package users + +import ( + "path/filepath" + "testing" + + "github.com/filebrowser/filebrowser/v2/files" + "github.com/spf13/afero" +) + +// TestUserCleanFs verifies that Clean builds the user filesystem according to the +// followExternalSymlinks flag and that FullPath resolves correctly for either +// implementation. +func TestUserCleanFs(t *testing.T) { + base := t.TempDir() + want := filepath.Join(base, "data", "x") + + t.Run("default builds a symlink-confining ScopedFs", func(t *testing.T) { + u := &User{Username: "u", Password: "p", Scope: "data"} + if err := u.Clean(base, false); err != nil { + t.Fatal(err) + } + if _, ok := u.Fs.(*files.ScopedFs); !ok { + t.Fatalf("expected *files.ScopedFs, got %T", u.Fs) + } + if got := u.FullPath("/x"); got != want { + t.Fatalf("FullPath: got %q, want %q", got, want) + } + }) + + t.Run("followExternalSymlinks builds a bare BasePathFs", func(t *testing.T) { + u := &User{Username: "u", Password: "p", Scope: "data"} + if err := u.Clean(base, true); err != nil { + t.Fatal(err) + } + if _, ok := u.Fs.(*afero.BasePathFs); !ok { + t.Fatalf("expected *afero.BasePathFs, got %T", u.Fs) + } + if got := u.FullPath("/x"); got != want { + t.Fatalf("FullPath: got %q, want %q", got, want) + } + }) +}