Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions backend/cmd/headlamp.go
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,11 @@ 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 == "" {
Expand All @@ -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
Expand Down
83 changes: 83 additions & 0 deletions backend/cmd/headlamp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
"runtime"
"strconv"
"strings"
"sync"
"testing"
"time"

Expand All @@ -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"
Expand Down Expand Up @@ -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)
Comment on lines +1428 to +1434

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,
},
Comment thread
govindup63 marked this conversation as resolved.
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{
Expand Down
Loading