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

@@ -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 {

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
 	}
 

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 }
 

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 {

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 {

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)
 	}

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.