From 26f05db84fdf96c1df2aeb1598070f70a31ff6d4 Mon Sep 17 00:00:00 2001 From: Santiago Greco Date: Tue, 21 Jul 2026 11:13:16 +0200 Subject: [PATCH] OCPBUGS-115003: Validate chart URL in /api/helm/verify to prevent SSRF The /api/helm/verify endpoint accepted arbitrary chart_url values without validation, allowing authenticated users to make the backend issue requests to internal network addresses. Reuse the existing IsValidChartURL check (already enforced on install and get-chart paths) to reject URLs that are not oci:// or http(s)://*.tgz before the backend attempts any connection. --- pkg/helm/actions/get_chart.go | 2 +- pkg/helm/actions/install_chart.go | 6 ++-- pkg/helm/actions/install_chart_test.go | 4 +-- pkg/helm/handlers/handlerChartVerifier.go | 4 +++ .../handlers/handler_chartVerifier_test.go | 34 ++++++++++++++++++- 5 files changed, 43 insertions(+), 7 deletions(-) diff --git a/pkg/helm/actions/get_chart.go b/pkg/helm/actions/get_chart.go index 18e80ecbf49..92506248811 100644 --- a/pkg/helm/actions/get_chart.go +++ b/pkg/helm/actions/get_chart.go @@ -65,7 +65,7 @@ func GetChart(url string, conf *action.Configuration, repositoryNamespace string func GetChartFromURL(url string, conf *action.Configuration, namespace string, client dynamic.Interface, coreClient corev1client.CoreV1Interface, filesCleanup bool) (*chart.Chart, error) { - if !isValidChartURL(url) { + if !IsValidChartURL(url) { return nil, fmt.Errorf("invalid chart URL: %s, must be oci:// URL or http(s)://*.tgz", url) } cmd := action.NewInstall(conf) diff --git a/pkg/helm/actions/install_chart.go b/pkg/helm/actions/install_chart.go index dadfd8910b6..99791dd8cd9 100644 --- a/pkg/helm/actions/install_chart.go +++ b/pkg/helm/actions/install_chart.go @@ -48,9 +48,9 @@ var ( httpURLRe = regexp.MustCompile(`(?i)^https?://` + hostPort + `/.+\.(?:tar\.gz|tgz)$`) ) -// isValidChartURL validates chart URLs using RFC-compliant hostname labels. +// IsValidChartURL validates chart URLs using RFC-compliant hostname labels. // Accepts oci:/// and http(s):///.tgz|tar.gz URLs. -func isValidChartURL(raw string) bool { +func IsValidChartURL(raw string) bool { return ociURLRe.MatchString(raw) || httpURLRe.MatchString(raw) } @@ -263,7 +263,7 @@ func InstallChartAsync(ns, name, url string, vals map[string]interface{}, conf * // If not provided, version is extracted from the OCI URL tag when applicable. func InstallChartFromURL(ns, name, url string, vals map[string]interface{}, conf *action.Configuration, coreClient corev1client.CoreV1Interface, version string) (*kv1.Secret, error) { - if !isValidChartURL(url) { + if !IsValidChartURL(url) { return nil, fmt.Errorf("invalid chart URL: %s, must be oci:// URL or http(s)://*.tgz", url) } diff --git a/pkg/helm/actions/install_chart_test.go b/pkg/helm/actions/install_chart_test.go index efa5be480e5..9e1cc64fc02 100644 --- a/pkg/helm/actions/install_chart_test.go +++ b/pkg/helm/actions/install_chart_test.go @@ -530,9 +530,9 @@ func TestIsValidChartURL(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := isValidChartURL(tt.url) + got := IsValidChartURL(tt.url) if got != tt.valid { - t.Errorf("isValidChartURL(%q) = %v, want %v", tt.url, got, tt.valid) + t.Errorf("IsValidChartURL(%q) = %v, want %v", tt.url, got, tt.valid) } }) } diff --git a/pkg/helm/handlers/handlerChartVerifier.go b/pkg/helm/handlers/handlerChartVerifier.go index fa1fc397c82..604dfb0186c 100644 --- a/pkg/helm/handlers/handlerChartVerifier.go +++ b/pkg/helm/handlers/handlerChartVerifier.go @@ -55,6 +55,10 @@ func (h *verifierHandlers) HandleChartVerifier(user *auth.User, w http.ResponseW serverutils.SendResponse(w, http.StatusBadRequest, serverutils.ApiError{Err: fmt.Sprintf("Failed to parse request: %v", err)}) return } + if !actions.IsValidChartURL(req.ChartUrl) { + serverutils.SendResponse(w, http.StatusBadRequest, serverutils.ApiError{Err: "invalid chart URL: must be oci:// or http(s)://*.tgz"}) + return + } conf := h.getActionConfigurations(h.ApiServerHost, "default", user.Token, &h.Transport) resp, err := h.chartVerifier(req.ChartUrl, req.Values, conf) if err != nil { diff --git a/pkg/helm/handlers/handler_chartVerifier_test.go b/pkg/helm/handlers/handler_chartVerifier_test.go index 7eed97f3786..5912a5dacd5 100644 --- a/pkg/helm/handlers/handler_chartVerifier_test.go +++ b/pkg/helm/handlers/handler_chartVerifier_test.go @@ -25,8 +25,11 @@ func fakeChartVerification(reportSummary string, err error) func(chartUrl string } } func TestHelmHandlers_HandleChartVerifier(t *testing.T) { + validBody := `{"chart_url":"https://example.com/charts/mychart-1.0.0.tgz"}` + tests := []struct { name string + body string expectedResponse string ReportSummary string error @@ -34,12 +37,14 @@ func TestHelmHandlers_HandleChartVerifier(t *testing.T) { }{ { name: "Error occurred", + body: validBody, expectedResponse: `{"error":"Failed to verify chart: Chart path is invalid"}`, error: errors.New("Chart path is invalid"), httpStatusCode: http.StatusBadGateway, }, { name: "Successful chart verification", + body: validBody, ReportSummary: fakeReportSummary, httpStatusCode: http.StatusOK, expectedResponse: ``, @@ -50,7 +55,7 @@ func TestHelmHandlers_HandleChartVerifier(t *testing.T) { handlers := fakeVerifierHandler() handlers.chartVerifier = fakeChartVerification(tt.ReportSummary, tt.error) - request := httptest.NewRequest("", "/foo", strings.NewReader("{}")) + request := httptest.NewRequest("", "/foo", strings.NewReader(tt.body)) response := httptest.NewRecorder() handlers.HandleChartVerifier(&auth.User{}, response, request) @@ -66,3 +71,30 @@ func TestHelmHandlers_HandleChartVerifier(t *testing.T) { }) } } + +func TestHelmHandlers_HandleChartVerifier_RejectsInvalidURLs(t *testing.T) { + tests := []struct { + name string + body string + }{ + {"rejects internal IP without tgz", `{"chart_url":"http://172.28.1.76:8849/nacos"}`}, + {"rejects non-tgz HTTP URL", `{"chart_url":"http://example.com/charts/mychart"}`}, + {"rejects empty chart_url", `{"chart_url":""}`}, + {"rejects ftp scheme", `{"chart_url":"ftp://example.com/chart.tgz"}`}, + {"rejects file scheme", `{"chart_url":"file:///etc/passwd"}`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + handlers := fakeVerifierHandler() + handlers.chartVerifier = fakeChartVerification("", nil) + + request := httptest.NewRequest("POST", "/api/helm/verify", strings.NewReader(tt.body)) + response := httptest.NewRecorder() + + handlers.HandleChartVerifier(&auth.User{}, response, request) + if response.Code != http.StatusBadRequest { + t.Errorf("expected status 400 but got %v", response.Code) + } + }) + } +}