From b296550e8f40334333d56f48ef978d9be721e9d0 Mon Sep 17 00:00:00 2001 From: Govind Pandey Date: Sun, 17 May 2026 22:50:55 +0530 Subject: [PATCH 1/5] backend: oidc-callback: Stop logging unrelated outer-scope error on missing state The /oidc-callback handler logged a stray err variable when the state query parameter was missing: logger.Log(logger.LevelError, nil, err, "invalid request state is empty") That err was not declared inside the handler closure. It resolved to the err returned by the kubeconfig load that runs once when createHeadlampHandler is constructed at server startup. Every malformed callback request therefore surfaced whatever happened at boot, for example error loading kubeconfig files: error reading kubeconfig file: open /home/headlamp/.config/Headlamp/kubeconfigs/config: no such file or directory attached to an unrelated 400 response, making the log misleading and hard to interpret. Pass nil to logger.Log on this branch and leave a short comment so the next reader does not reintroduce the capture. Add a regression test that drives the handler with a deliberately broken KubeConfigPath, fires GET /oidc-callback with no state, captures logger output via logger.SetLogFunc, and asserts the missing-state log carries no error value. Issue: https://github.com/kubernetes-sigs/headlamp/issues/4839 Signed-off-by: Govind Pandey --- backend/cmd/headlamp.go | 4 +- backend/cmd/headlamp_test.go | 76 ++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/backend/cmd/headlamp.go b/backend/cmd/headlamp.go index cca86f5b5d0..f7f243be67e 100644 --- a/backend/cmd/headlamp.go +++ b/backend/cmd/headlamp.go @@ -846,7 +846,9 @@ func createHeadlampHandler(ctx context.Context, config *HeadlampConfig) http.Han state := r.URL.Query().Get("state") if state == "" { - logger.Log(logger.LevelError, nil, err, "invalid request state is empty") + // nil err: the closure-local err is declared below, so any err + // here would leak from createHeadlampHandler's outer scope. + logger.Log(logger.LevelError, nil, nil, "invalid request state is empty") http.Error(w, "invalid request state is empty", http.StatusBadRequest) return diff --git a/backend/cmd/headlamp_test.go b/backend/cmd/headlamp_test.go index 662df5bda60..edf9ed7bb0e 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,80 @@ 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. +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() + + 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, + StaticPluginDir: 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{ From b7d6729bbb26f277f14992b1bb6ae722da9eed00 Mon Sep 17 00:00:00 2001 From: Govind Pandey Date: Mon, 18 May 2026 12:50:27 +0530 Subject: [PATCH 2/5] backend: cmd: Satisfy golangci-lint funlen and wsl_v5 on new test The regression test added in the previous commit tripped two lint rules in CI: - funlen: the test body is 67 lines, over the 60-line cap. Other tests in the same file already use //nolint:funlen for the same reason (see TestHelmRouteReleaseHandlerTokenExtraction). Apply the same directive. - wsl_v5: missing blank line around the captured := append(...) line that copies logCalls out from under the mutex. Signed-off-by: Govind Pandey --- backend/cmd/headlamp_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/backend/cmd/headlamp_test.go b/backend/cmd/headlamp_test.go index edf9ed7bb0e..82947ecae0e 100644 --- a/backend/cmd/headlamp_test.go +++ b/backend/cmd/headlamp_test.go @@ -1411,6 +1411,8 @@ 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 @@ -1466,7 +1468,9 @@ func TestOidcCallbackEmptyStateDoesNotLogStaleError(t *testing.T) { "empty state should still produce a 400 response") logMu.Lock() + captured := append([]logCall(nil), logCalls...) + logMu.Unlock() var stateLog *logCall From b829fbefe8a02d6c137aad650a08c8eb6c9ffbbb Mon Sep 17 00:00:00 2001 From: Govind Pandey Date: Mon, 18 May 2026 22:19:22 +0530 Subject: [PATCH 3/5] backend: cmd: Pin HEADLAMP_STATIC_PLUGINS_DIR in oidc-callback test createHeadlampHandler unconditionally reassigns config.StaticPluginDir from the HEADLAMP_STATIC_PLUGINS_DIR environment variable, so the test setting cfg.HeadlampCFG.StaticPluginDir = scratch was a no-op and the plugin watcher could end up walking whatever directory the env var named in CI. Set the env var via t.Setenv so the static-plugin dir is deterministic for the duration of the test, and drop the dead field assignment. Signed-off-by: Govind Pandey --- backend/cmd/headlamp_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/backend/cmd/headlamp_test.go b/backend/cmd/headlamp_test.go index 82947ecae0e..e1f222c2159 100644 --- a/backend/cmd/headlamp_test.go +++ b/backend/cmd/headlamp_test.go @@ -1435,6 +1435,10 @@ func TestOidcCallbackEmptyStateDoesNotLogStaleError(t *testing.T) { 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) @@ -1448,7 +1452,6 @@ func TestOidcCallbackEmptyStateDoesNotLogStaleError(t *testing.T) { KubeConfigStore: kubeconfig.NewContextStore(), PluginDir: scratch, UserPluginDir: scratch, - StaticPluginDir: scratch, }, Cache: cache.New[interface{}](), TelemetryConfig: GetDefaultTestTelemetryConfig(), From e0e34ef787a890e9f145e63795f1391ea7b2e94a Mon Sep 17 00:00:00 2001 From: Govind Pandey Date: Tue, 19 May 2026 20:18:51 +0530 Subject: [PATCH 4/5] backend: oidc-callback: Shadow outer-scope err with a closure-local declaration A comment was load-bearing in the previous commit: it warned future editors that the early-branch logger.Log must not reference err, because err would resolve to createHeadlampHandler's outer-scope kubeconfig-load result. Hoist `var err error` to the top of the /oidc-callback closure so the shadow is enforced by the language, not by a comment. Any future log call inside the handler now references the closure-local err that begins life as nil. Signed-off-by: Govind Pandey --- backend/cmd/headlamp.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/backend/cmd/headlamp.go b/backend/cmd/headlamp.go index f7f243be67e..61e0544bfc1 100644 --- a/backend/cmd/headlamp.go +++ b/backend/cmd/headlamp.go @@ -843,12 +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 == "" { - // nil err: the closure-local err is declared below, so any err - // here would leak from createHeadlampHandler's outer scope. - logger.Log(logger.LevelError, nil, nil, "invalid request state is empty") + logger.Log(logger.LevelError, nil, err, "invalid request state is empty") http.Error(w, "invalid request state is empty", http.StatusBadRequest) return @@ -871,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 From 47bc03b6e73a90da1d1c912bf78f9c0a18a0a11d Mon Sep 17 00:00:00 2001 From: Govind Pandey Date: Tue, 19 May 2026 20:25:43 +0530 Subject: [PATCH 5/5] backend: oidc-callback: Pass nil instead of the shadowed nil err to logger.Log The shadow declaration on the previous commit reliably leaves err nil at the missing-state branch, so logger.Log was being passed nil indirectly. Pass nil directly to make the call self-evident; the shadow declaration above still protects against future log calls elsewhere in the closure. Signed-off-by: Govind Pandey --- backend/cmd/headlamp.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/cmd/headlamp.go b/backend/cmd/headlamp.go index 61e0544bfc1..a0813468859 100644 --- a/backend/cmd/headlamp.go +++ b/backend/cmd/headlamp.go @@ -851,7 +851,7 @@ func createHeadlampHandler(ctx context.Context, config *HeadlampConfig) http.Han 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