fix(auth): guard nil user entries and serialize file-user swaps

auth.go

@@ -130,6 +130,9 @@ func newAuthStore(cfg *Config) (*authStore, error) {
 // goes through resolveSecret before digesting, so env:/file: references
 // work the same as for provider keys.
 func (a *authStore) buildEntry(uc *UserConfig) (*authEntry, error) {
+	if uc == nil {
+		return nil, fmt.Errorf("llmgateway: user config entry must not be nil")
+	}
 	grp, ok := a.groups[uc.Group]
 	if !ok {
 		return nil, fmt.Errorf("llmgateway: user %q references unknown group %q", uc.Name, uc.Group)
@@ -154,7 +157,16 @@ func (a *authStore) buildEntry(uc *UserConfig) (*authEntry, error) {
 // (Task 4) to pick up changes to Users.File without restarting the plugin.
 // On error the store is left exactly as it was before the call — the new
 // set is validated and built in full before it replaces the old one.
+//
+// mu is held for the whole rebuild, not just the final swap. That serializes
+// concurrent callers (a reload timer must never race itself), so one call's
+// result can never be partially overwritten or interleaved with another's.
+// identify's reads stay fast (RLock) and reloads are rare, so the extra hold
+// time is a good trade.
 func (a *authStore) replaceFileUsers(us []*UserConfig) error {
+	a.mu.Lock()
+	defer a.mu.Unlock()
+
 	next := make(map[[32]byte]*authEntry, len(a.inline)+len(us))
 	for digest, entry := range a.inline {
 		next[digest] = entry
@@ -170,9 +182,7 @@ func (a *authStore) replaceFileUsers(us []*UserConfig) error {
 		next[entry.digest] = entry
 	}
 
-	a.mu.Lock()
 	a.byDigest = next
-	a.mu.Unlock()
 	return nil
 }
 

auth_test.go

@@ -1,7 +1,9 @@
 package traefikllmgateway
 
 import (
+	"fmt"
 	"net/http/httptest"
+	"sync"
 	"testing"
 )
 
@@ -280,3 +282,95 @@ func TestAuthStore_ReplaceFileUsers_UnknownGroup_ReturnsError(t *testing.T) {
 		t.Fatal("want error for file user referencing unknown group")
 	}
 }
+
+// TestAuthStore_ReplaceFileUsers_NilElement_ReturnsErrorNotPanic is the
+// regression for a nil *UserConfig slice element reaching buildEntry. This
+// path is reachable at runtime: json.Unmarshal of a `null` array element
+// into []*UserConfig yields a nil entry, and the config-reload path (Task 4)
+// that calls replaceFileUsers is NOT wrapped by recoverPanic — that guard
+// only covers ServeHTTP. An unguarded nil dereference here would take down
+// the whole process instead of failing one reload.
+func TestAuthStore_ReplaceFileUsers_NilElement_ReturnsErrorNotPanic(t *testing.T) {
+	a, err := newAuthStore(testAuthCfg())
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := a.replaceFileUsers([]*UserConfig{nil}); err == nil {
+		t.Fatal("want error for a nil user config element, got nil")
+	}
+
+	// The store must still be usable — the rejected replace must not have
+	// partially mutated it.
+	r := httptest.NewRequest("POST", "/v1/chat/completions", nil)
+	r.Header.Set("Authorization", "Bearer sk-secret")
+	if _, _, ok := a.identify(r); !ok {
+		t.Fatal("want inline user still resolvable after a rejected nil-element replace")
+	}
+}
+
+// TestNewAuthStore_NilInlineElement_ReturnsErrorNotPanic covers the same
+// nil-element hazard reachable from newAuthStore's inline-user loop, which
+// runs at plugin construction time — also outside recoverPanic's coverage.
+func TestNewAuthStore_NilInlineElement_ReturnsErrorNotPanic(t *testing.T) {
+	cfg := &Config{
+		Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}},
+		Groups:    map[string]*GroupConfig{"eng": {}},
+		Users:     &UsersConfig{Inline: []*UserConfig{nil}},
+	}
+	if _, err := newAuthStore(cfg); err == nil {
+		t.Fatal("want error for a nil inline user config element, got nil")
+	}
+}
+
+// TestAuthStore_ReplaceFileUsers_ConcurrentWritersAndReaders exercises
+// replaceFileUsers and identify from many goroutines simultaneously under
+// -race. Each writer swaps in a single, uniquely-keyed file user, so
+// whichever writer's rebuild is applied last, the store must always hold
+// exactly the inline users plus one file user's worth of entries — never a
+// partial mix from two overlapping rebuilds, and never a state where the
+// always-present inline user briefly disappears.
+func TestAuthStore_ReplaceFileUsers_ConcurrentWritersAndReaders(t *testing.T) {
+	a, err := newAuthStore(testAuthCfg()) // inline user "a" / sk-secret, group eng
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	const writers = 20
+	const readers = 10
+	const readsPerReader = 50
+
+	var wg sync.WaitGroup
+	wg.Add(writers + readers)
+
+	for i := 0; i < writers; i++ {
+		go func(i int) {
+			defer wg.Done()
+			uc := &UserConfig{Name: fmt.Sprintf("f%d", i), Group: "eng", APIKey: fmt.Sprintf("sk-file-%d", i)}
+			if err := a.replaceFileUsers([]*UserConfig{uc}); err != nil {
+				t.Errorf("replaceFileUsers(%d): %v", i, err)
+			}
+		}(i)
+	}
+	for i := 0; i < readers; i++ {
+		go func() {
+			defer wg.Done()
+			r := httptest.NewRequest("POST", "/v1/chat/completions", nil)
+			r.Header.Set("Authorization", "Bearer sk-secret")
+			for j := 0; j < readsPerReader; j++ {
+				if _, _, ok := a.identify(r); !ok {
+					t.Error("inline user must remain identifiable during concurrent file-user swaps")
+					return
+				}
+			}
+		}()
+	}
+	wg.Wait()
+
+	a.mu.RLock()
+	got := len(a.byDigest)
+	a.mu.RUnlock()
+	want := len(a.inline) + 1 // inline users plus exactly one winning file-user batch
+	if got != want {
+		t.Fatalf("want exactly one file-user batch to survive concurrent swaps (size %d), got size %d — a rebuild was lost or merged with another", want, got)
+	}
+}