diff --git a/backend/cmd/headlamp.go b/backend/cmd/headlamp.go index cca86f5b5d0..a0813468859 100644 --- a/backend/cmd/headlamp.go +++ b/backend/cmd/headlamp.go @@ -843,10 +843,15 @@ func createHeadlampHandler(ctx context.Context, config *HeadlampConfig) http.Han config.handleNodeDrainStatus).Methods("GET").Queries("cluster", "{cluster}", "nodeName", "{node}") r.HandleFunc("/oidc-callback", func(w http.ResponseWriter, r *http.Request) { + // Shadow createHeadlampHandler's outer-scope err so any log call in + // this handler is guaranteed to reference this closure's err and + // never the startup kubeconfig-load error. See issue #4839. + var err error + state := r.URL.Query().Get("state") if state == "" { - logger.Log(logger.LevelError, nil, err, "invalid request state is empty") + logger.Log(logger.LevelError, nil, nil, "invalid request state is empty") http.Error(w, "invalid request state is empty", http.StatusBadRequest) return @@ -869,8 +874,6 @@ func createHeadlampHandler(ctx context.Context, config *HeadlampConfig) http.Han var oauth2Token *oauth2.Token - var err error - // Exchange authorization code for token, with or without PKCE if config.OidcUsePKCE && oauthConfig.CodeVerifier != "" { // Use PKCE code verifier for token exchange diff --git a/backend/cmd/headlamp_test.go b/backend/cmd/headlamp_test.go index 662df5bda60..e1f222c2159 100644 --- a/backend/cmd/headlamp_test.go +++ b/backend/cmd/headlamp_test.go @@ -35,6 +35,7 @@ import ( "runtime" "strconv" "strings" + "sync" "testing" "time" @@ -44,6 +45,7 @@ import ( "github.com/kubernetes-sigs/headlamp/backend/pkg/config" "github.com/kubernetes-sigs/headlamp/backend/pkg/headlampconfig" "github.com/kubernetes-sigs/headlamp/backend/pkg/kubeconfig" + "github.com/kubernetes-sigs/headlamp/backend/pkg/logger" "github.com/kubernetes-sigs/headlamp/backend/pkg/telemetry" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -1407,6 +1409,87 @@ func TestGetOidcCallbackURL(t *testing.T) { } } +// Regression test for #4839: /oidc-callback's missing-state log must not +// carry the outer-scope kubeconfig-load err from createHeadlampHandler. +// +//nolint:funlen +func TestOidcCallbackEmptyStateDoesNotLogStaleError(t *testing.T) { + type logCall struct { + level uint + msg string + err interface{} + } + + var ( + logMu sync.Mutex + logCalls []logCall + ) + + originalLogFunc := logger.SetLogFunc(func(level uint, _ map[string]string, err interface{}, msg string) { + logMu.Lock() + defer logMu.Unlock() + + logCalls = append(logCalls, logCall{level: level, msg: msg, err: err}) + }) + defer logger.SetLogFunc(originalLogFunc) + + scratch := t.TempDir() + + // createHeadlampHandler overwrites config.StaticPluginDir from this env + // var, so set it deterministically rather than relying on the caller. + t.Setenv("HEADLAMP_STATIC_PLUGINS_DIR", scratch) + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + cfg := &HeadlampConfig{ + HeadlampConfig: &headlampconfig.HeadlampConfig{ + HeadlampCFG: &headlampconfig.HeadlampCFG{ + UseInCluster: false, + // Guaranteed-missing kubeconfig so the startup load fails and + // seeds the outer-scope err the callback closure would leak. + KubeConfigPath: filepath.Join(scratch, "missing-kubeconfig"), + KubeConfigStore: kubeconfig.NewContextStore(), + PluginDir: scratch, + UserPluginDir: scratch, + }, + Cache: cache.New[interface{}](), + TelemetryConfig: GetDefaultTestTelemetryConfig(), + TelemetryHandler: &telemetry.RequestHandler{}, + }, + } + + handler := createHeadlampHandler(ctx, cfg) + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, "/oidc-callback", nil) + require.NoError(t, err) + + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + assert.Equal(t, http.StatusBadRequest, rr.Code, + "empty state should still produce a 400 response") + + logMu.Lock() + + captured := append([]logCall(nil), logCalls...) + + logMu.Unlock() + + var stateLog *logCall + + for i := range captured { + if captured[i].msg == "invalid request state is empty" { + stateLog = &captured[i] + break + } + } + + require.NotNil(t, stateLog, "expected a log entry for the missing-state branch") + assert.Nil(t, stateLog.err, + "missing-state log must not carry a stale error from the outer createHeadlampHandler scope") +} + func TestOIDCTokenRefreshMiddleware(t *testing.T) { kubeConfigStore := kubeconfig.NewContextStore() config := &HeadlampConfig{