Skip to content

Commit 2007e3d

Browse files
Copilotillume
andcommitted
Fix critical nil pointer bug and improve validation
- Fix nil pointer dereference when GetInClusterContext returns error - Add else block to only use context when err == nil - Move validation before InClusterConfig for consistent error messages - Add Hostname() check to reject URLs like https://:443 - Add test case for empty hostname validation - Improve integration test to always test validation - Revert unintended frontend/package-lock.json changes Addresses critical bug and review comments. Co-authored-by: illume <9541+illume@users.noreply.github.com>
1 parent caad2ff commit 2007e3d

File tree

5 files changed

+37
-89
lines changed

5 files changed

+37
-89
lines changed

backend/cmd/headlamp.go

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -438,18 +438,18 @@ func createHeadlampHandler(config *HeadlampConfig) http.Handler {
438438
config.APIServerEndpoint)
439439
if err != nil {
440440
logger.Log(logger.LevelError, nil, err, "Failed to get in-cluster context")
441-
}
442-
443-
context.Source = kubeconfig.InCluster
441+
} else {
442+
context.Source = kubeconfig.InCluster
444443

445-
err = context.SetupProxy()
446-
if err != nil {
447-
logger.Log(logger.LevelError, nil, err, "Failed to setup proxy for in-cluster context")
448-
}
444+
err = context.SetupProxy()
445+
if err != nil {
446+
logger.Log(logger.LevelError, nil, err, "Failed to setup proxy for in-cluster context")
447+
}
449448

450-
err = config.KubeConfigStore.AddContext(context)
451-
if err != nil {
452-
logger.Log(logger.LevelError, nil, err, "Failed to add in-cluster context")
449+
err = config.KubeConfigStore.AddContext(context)
450+
if err != nil {
451+
logger.Log(logger.LevelError, nil, err, "Failed to add in-cluster context")
452+
}
453453
}
454454
}
455455

backend/cmd/headlamp_test.go

Lines changed: 9 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1735,33 +1735,25 @@ func TestHandleClusterServiceProxy(t *testing.T) {
17351735
// TestCustomAPIServerEndpoint tests the custom API server endpoint configuration
17361736
// with validation logic, simulating kube-oidc-proxy scenarios.
17371737
func TestCustomAPIServerEndpoint(t *testing.T) {
1738-
if os.Getenv("HEADLAMP_RUN_INTEGRATION_TESTS") != strconv.FormatBool(istrue) {
1739-
t.Skip("skipping integration test")
1740-
}
1741-
1742-
// Test https validation - should reject http URLs
1738+
// Test https validation directly - should reject http URLs
1739+
// Since validation now happens before InClusterConfig, this will always test validation
17431740
_, err := kubeconfig.GetInClusterContext(
17441741
"test-cluster",
17451742
"", "", "", "",
17461743
false, "",
17471744
"http://insecure-proxy.example.com:443", // http scheme should be rejected
17481745
)
17491746

1750-
// Should always fail - either with validation error or in-cluster config error
1751-
if err == nil {
1752-
t.Fatal("Expected error with http:// URL, but got nil")
1753-
}
1747+
// Should always fail with validation error (validation happens before in-cluster check)
1748+
require.Error(t, err, "Expected error with http:// URL")
1749+
assert.Contains(t, err.Error(), "must be a full https:// URL",
1750+
"Error should be about https requirement")
17541751

1755-
switch {
1756-
case strings.Contains(err.Error(), "must be a full https:// URL"):
1757-
t.Logf("https validation working correctly: %v", err)
1758-
case strings.Contains(err.Error(), "unable to load in-cluster configuration"):
1759-
t.Skip("not running in cluster environment, cannot test full functionality")
1760-
default:
1761-
t.Fatalf("unexpected error: %v", err)
1752+
// Test with valid https URL - will fail with in-cluster error if not actually in cluster
1753+
if os.Getenv("HEADLAMP_RUN_INTEGRATION_TESTS") != strconv.FormatBool(istrue) {
1754+
t.Skip("skipping full integration test (validation already tested above)")
17621755
}
17631756

1764-
// Test with valid https URL (will fail with in-cluster error if not in cluster)
17651757
ctx, err := kubeconfig.GetInClusterContext(
17661758
"test-cluster",
17671759
"", "", "", "",

backend/pkg/kubeconfig/kubeconfig.go

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1010,7 +1010,7 @@ func validateAPIServerEndpoint(endpoint string) (string, error) {
10101010
}
10111011

10121012
parsedURL, err := url.Parse(trimmed)
1013-
if err != nil || !parsedURL.IsAbs() || parsedURL.Host == "" {
1013+
if err != nil || !parsedURL.IsAbs() || parsedURL.Host == "" || parsedURL.Hostname() == "" {
10141014
return "", fmt.Errorf(
10151015
"invalid custom API server endpoint %q: must be an absolute URL with scheme and host",
10161016
trimmed,
@@ -1087,19 +1087,20 @@ func GetInClusterContext(
10871087
oidcCACert string,
10881088
customAPIServerEndpoint string,
10891089
) (*Context, error) {
1090-
clusterConfig, err := rest.InClusterConfig()
1090+
// Validate custom endpoint first, before attempting to load in-cluster config
1091+
customEndpoint, err := validateAPIServerEndpoint(customAPIServerEndpoint)
10911092
if err != nil {
10921093
return nil, err
10931094
}
10941095

1095-
// Use custom API server endpoint if provided, otherwise use default from in-cluster config
1096-
apiServerHost := clusterConfig.Host
1097-
1098-
customEndpoint, err := validateAPIServerEndpoint(customAPIServerEndpoint)
1096+
clusterConfig, err := rest.InClusterConfig()
10991097
if err != nil {
11001098
return nil, err
11011099
}
11021100

1101+
// Use custom API server endpoint if provided, otherwise use default from in-cluster config
1102+
apiServerHost := clusterConfig.Host
1103+
11031104
if customEndpoint != "" {
11041105
apiServerHost = customEndpoint
11051106
}

backend/pkg/kubeconfig/kubeconfig_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -883,6 +883,12 @@ func TestValidateAPIServerEndpoint(t *testing.T) {
883883
wantErr: true,
884884
errContains: "must not include user info (credentials)",
885885
},
886+
{
887+
name: "URL with empty hostname is rejected",
888+
endpoint: "https://:443",
889+
wantErr: true,
890+
errContains: "must be an absolute URL with scheme and host",
891+
},
886892
{
887893
name: "URL with query string is rejected",
888894
endpoint: "https://proxy.example.com:443?token=secret",

0 commit comments

Comments
 (0)