From fb6aeba9eae7b8eb401e0db325973781e1ffd08b Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Sat, 25 Jul 2026 22:15:27 +0200 Subject: [PATCH] fix(users): make the provisioned scope check atomic with the save The check that stops two usernames normalizing to the same home directory ran as a storage operation separate from the save, so two first-time users provisioned concurrently could both observe a free scope and both be saved into it, ending up sharing one home directory. Move the check into users.Storage.SaveProvisioned, which holds a single lock across the lookup and the save, and reduce CreateUserHome to deriving and creating the directory. This covers all three provisioning paths: signup, proxy auth and hook auth. Refs GHSA-j7jh-37pf-mf8h --- auth/hook.go | 5 +++-- auth/proxy.go | 5 +++-- auth/proxy_test.go | 21 +++++++++++++++++- http/auth.go | 8 +++---- settings/dir.go | 27 +++++++++------------- settings/dir_test.go | 53 ++++++++++---------------------------------- users/storage.go | 32 ++++++++++++++++++++++++++ 7 files changed, 83 insertions(+), 68 deletions(-) diff --git a/auth/hook.go b/auth/hook.go index e53768cb..6653cd9b 100644 --- a/auth/hook.go +++ b/auth/hook.go @@ -160,12 +160,13 @@ func (a *HookAuth) SaveUser() (*users.User, error) { // A scope explicitly returned by the hook takes precedence over the // automatic per-user home directory derivation. _, explicitScope := a.Fields.Values["user.scope"] - if err := a.Settings.CreateUserHome(u, a.Users, a.Server.Root, explicitScope); err != nil { + derivedScope, err := a.Settings.CreateUserHome(u, a.Server.Root, explicitScope) + if err != nil { return nil, err } log.Printf("user: %s, home dir: [%s].", u.Username, u.Scope) - if err := a.Users.Save(u); err != nil { + if err := a.Users.SaveProvisioned(u, derivedScope); err != nil { return nil, err } } else if p := !users.CheckPwd(a.Cred.Password, u.Password); len(a.Fields.Values) > 1 || p { diff --git a/auth/proxy.go b/auth/proxy.go index c655f8b6..5550f102 100644 --- a/auth/proxy.go +++ b/auth/proxy.go @@ -50,11 +50,12 @@ func (a ProxyAuth) createUser(usr users.Store, setting *settings.Settings, srv * user.Perm.Execute = false user.Commands = []string{} - if err = setting.CreateUserHome(user, usr, srv.Root, false); err != nil { + var derivedScope bool + if derivedScope, err = setting.CreateUserHome(user, srv.Root, false); err != nil { return nil, err } - if err = usr.Save(user); err != nil { + if err = usr.SaveProvisioned(user, derivedScope); err != nil { return nil, err } diff --git a/auth/proxy_test.go b/auth/proxy_test.go index 233e72c8..5d9b5ace 100644 --- a/auth/proxy_test.go +++ b/auth/proxy_test.go @@ -2,6 +2,7 @@ package auth import ( "net/http" + "strings" "testing" fberrors "github.com/filebrowser/filebrowser/v2/errors" @@ -22,13 +23,31 @@ func (m *mockUserStore) Get(_ string, _ bool, id interface{}) (*users.User, erro return nil, fberrors.ErrNotExist } -func (m *mockUserStore) GetByScope(_ string) (*users.User, error) { return nil, fberrors.ErrNotExist } +func (m *mockUserStore) GetByScope(scope string) (*users.User, error) { + for _, u := range m.users { + if strings.EqualFold(u.Scope, scope) { + return u, nil + } + } + return nil, fberrors.ErrNotExist +} + 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) SaveProvisioned(user *users.User, derivedScope bool) error { + if derivedScope { + if _, err := m.GetByScope(user.Scope); err == nil { + return fberrors.ErrExist + } + } + return m.Save(user) +} + func (m *mockUserStore) Delete(_ interface{}) error { return nil } func (m *mockUserStore) LastUpdate(_ uint) int64 { return 0 } diff --git a/http/auth.go b/http/auth.go index d4f31916..25a2f1e3 100644 --- a/http/auth.go +++ b/http/auth.go @@ -192,16 +192,14 @@ var signupHandler = func(w http.ResponseWriter, r *http.Request, d *data) (int, user.Password = pwd - switch err := d.settings.CreateUserHome(user, d.store.Users, d.server.Root, false); { - case errors.Is(err, fberrors.ErrExist): - return http.StatusConflict, fberrors.ErrExist - case err != nil: + derivedScope, err := d.settings.CreateUserHome(user, d.server.Root, false) + if err != nil { return http.StatusInternalServerError, err } log.Printf("new user: %s, home dir: [%s].", user.Username, user.Scope) - err = d.store.Users.Save(user) + err = d.store.Users.SaveProvisioned(user, derivedScope) if errors.Is(err, fberrors.ErrExist) { return http.StatusConflict, err } else if err != nil { diff --git a/settings/dir.go b/settings/dir.go index 1259d24a..1abce67c 100644 --- a/settings/dir.go +++ b/settings/dir.go @@ -11,7 +11,6 @@ import ( "github.com/spf13/afero" - fberrors "github.com/filebrowser/filebrowser/v2/errors" "github.com/filebrowser/filebrowser/v2/users" ) @@ -48,31 +47,25 @@ func (s *Settings) MakeUserDir(username, userScope, serverRoot string) (string, // supply an explicit scope, the scope is cleared so that MakeUserDir derives a // per-user home from the username instead of falling back to the default scope // (which normalizes to the server root, leaving every provisioned user sharing -// it). When a home directory is derived, it also rejects a scope already owned -// by another user, so that distinct usernames cannot silently share one home -// directory. -func (s *Settings) CreateUserHome(user *users.User, store users.Store, serverRoot string, explicitScope bool) error { - derived := s.CreateUserDir && !explicitScope +// it). +// +// It reports whether the scope was derived from the username. A derived scope +// must be persisted with users.Storage.SaveProvisioned, which rejects a scope +// already owned by another user so that distinct usernames cannot silently +// share one home directory. +func (s *Settings) CreateUserHome(user *users.User, serverRoot string, explicitScope bool) (derived bool, err error) { + derived = s.CreateUserDir && !explicitScope if derived { user.Scope = "" } userHome, err := s.MakeUserDir(user.Username, user.Scope, serverRoot) if err != nil { - return err + return false, err } user.Scope = userHome - if derived { - switch _, err := store.GetByScope(user.Scope); { - case err == nil: - return fberrors.ErrExist - case !errors.Is(err, fberrors.ErrNotExist): - return err - } - } - - return nil + return derived, nil } func cleanUsername(s string) string { diff --git a/settings/dir_test.go b/settings/dir_test.go index 0be07e9f..900f6512 100644 --- a/settings/dir_test.go +++ b/settings/dir_test.go @@ -1,73 +1,44 @@ package settings import ( - "errors" "testing" - fberrors "github.com/filebrowser/filebrowser/v2/errors" "github.com/filebrowser/filebrowser/v2/users" ) -// stubStore is a minimal users.Store used to exercise CreateUserHome. -type stubStore struct { - byScope map[string]*users.User -} - -func (s *stubStore) Get(_ string, _ bool, _ interface{}) (*users.User, error) { - return nil, fberrors.ErrNotExist -} - -func (s *stubStore) GetByScope(scope string) (*users.User, error) { - if u, ok := s.byScope[scope]; ok { - return u, nil - } - return nil, fberrors.ErrNotExist -} - -func (s *stubStore) Gets(_ string, _ bool) ([]*users.User, error) { return nil, nil } -func (s *stubStore) Update(_ *users.User, _ ...string) error { return nil } -func (s *stubStore) Save(_ *users.User) error { return nil } -func (s *stubStore) Delete(_ interface{}) error { return nil } -func (s *stubStore) LastUpdate(_ uint) int64 { return 0 } - // A user provisioned with CreateUserDir must receive a per-user home directory // derived from its username, not the default scope which normalizes to the // server root. func TestCreateUserHomeDerivesPerUserScope(t *testing.T) { s := &Settings{CreateUserDir: true, UserHomeBasePath: "/users"} - store := &stubStore{byScope: map[string]*users.User{}} user := &users.User{Username: "alice", Scope: "."} - if err := s.CreateUserHome(user, store, t.TempDir(), false); err != nil { + derived, err := s.CreateUserHome(user, t.TempDir(), false) + if err != nil { t.Fatalf("unexpected error: %v", err) } + if !derived { + t.Error("expected the scope to be reported as derived") + } if user.Scope != "/users/alice" { t.Errorf("expected derived scope /users/alice, got %q", user.Scope) } } -// When the derived scope is already owned by another user, provisioning must be -// rejected so distinct usernames cannot silently share one home directory. -func TestCreateUserHomeRejectsCollision(t *testing.T) { - s := &Settings{CreateUserDir: true, UserHomeBasePath: "/users"} - store := &stubStore{byScope: map[string]*users.User{"/users/alice": {Username: "alice"}}} - - user := &users.User{Username: "alice", Scope: "."} - if err := s.CreateUserHome(user, store, t.TempDir(), false); !errors.Is(err, fberrors.ErrExist) { - t.Fatalf("expected ErrExist on scope collision, got %v", err) - } -} - // A scope explicitly supplied by the caller (e.g. returned by an auth hook) must -// be preserved instead of being replaced by a derived home directory. +// be preserved instead of being replaced by a derived home directory, and must +// not be reported as derived: it is legitimate for several users to share it. func TestCreateUserHomePreservesExplicitScope(t *testing.T) { s := &Settings{CreateUserDir: true, UserHomeBasePath: "/users"} - store := &stubStore{byScope: map[string]*users.User{"/users/alice": {Username: "alice"}}} user := &users.User{Username: "alice", Scope: "/custom"} - if err := s.CreateUserHome(user, store, t.TempDir(), true); err != nil { + derived, err := s.CreateUserHome(user, t.TempDir(), true) + if err != nil { t.Fatalf("unexpected error: %v", err) } + if derived { + t.Error("an explicit scope must not be reported as derived") + } if user.Scope != "/custom" { t.Errorf("explicit scope should be preserved, got %q", user.Scope) } diff --git a/users/storage.go b/users/storage.go index ff10aeab..fe66ecac 100644 --- a/users/storage.go +++ b/users/storage.go @@ -1,6 +1,7 @@ package users import ( + "errors" "sync" "time" @@ -25,6 +26,7 @@ type Store interface { Gets(baseScope string, followExternalSymlinks bool) ([]*User, error) Update(user *User, fields ...string) error Save(user *User) error + SaveProvisioned(user *User, derivedScope bool) error Delete(id interface{}) error LastUpdate(id uint) int64 } @@ -34,6 +36,10 @@ type Storage struct { back StorageBackend updated map[uint]int64 mux sync.RWMutex + + // provision serializes the scope-collision check and the save of newly + // provisioned users, which must not interleave. See SaveProvisioned. + provision sync.Mutex } // NewStorage creates a users storage from a backend. @@ -108,6 +114,32 @@ func (s *Storage) Save(user *User) error { return s.back.Save(user) } +// SaveProvisioned saves a user that is being provisioned (via signup, proxy +// auth or hook auth). When its scope was derived from the username, it first +// rejects the save if another user already owns that scope, so that distinct +// usernames cannot silently share one home directory. +// +// The check and the save are held under a single lock. Performing them as two +// independent operations lets two concurrent provisioning requests both observe +// a free scope and both save, leaving two users sharing one home directory. +func (s *Storage) SaveProvisioned(user *User, derivedScope bool) error { + if !derivedScope { + return s.Save(user) + } + + s.provision.Lock() + defer s.provision.Unlock() + + switch _, err := s.back.GetByScope(user.Scope); { + case err == nil: + return fberrors.ErrExist + case !errors.Is(err, fberrors.ErrNotExist): + return err + } + + return s.Save(user) +} + // Delete allows you to delete 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.