From c47c8fc8689e6056a274fdfda3b36e4aaf73b525 Mon Sep 17 00:00:00 2001 From: Omri Steiner Date: Thu, 15 May 2025 19:55:55 +0200 Subject: [PATCH] fix: correctly concat absolute URIs in repo cache There used to be two implemenations for concatenating the repo URL with the chart URI / URL. In case the chart specified an absolute URI, one of the implementations performed an incorrect concatenation between the two, resulting in a URL which looks like . This commit removes the faulty implementation and uses the other correct one instead. Signed-off-by: Omri Steiner --- pkg/downloader/chart_downloader_test.go | 1 + pkg/downloader/manager.go | 22 +-------- pkg/downloader/manager_test.go | 45 ++++++++++--------- .../repository/testing-relative-index.yaml | 13 ++++++ pkg/repo/chartrepo_test.go | 4 ++ 5 files changed, 44 insertions(+), 41 deletions(-) diff --git a/pkg/downloader/chart_downloader_test.go b/pkg/downloader/chart_downloader_test.go index 26dcc58ff..766afede1 100644 --- a/pkg/downloader/chart_downloader_test.go +++ b/pkg/downloader/chart_downloader_test.go @@ -46,6 +46,7 @@ func TestResolveChartRef(t *testing.T) { {name: "reference, querystring repo", ref: "testing-querystring/alpine", expect: "http://example.com/alpine-1.2.3.tgz?key=value"}, {name: "reference, testing-relative repo", ref: "testing-relative/foo", expect: "http://example.com/helm/charts/foo-1.2.3.tgz"}, {name: "reference, testing-relative repo", ref: "testing-relative/bar", expect: "http://example.com/helm/bar-1.2.3.tgz"}, + {name: "reference, testing-relative repo", ref: "testing-relative/baz", expect: "http://example.com/path/to/baz-1.2.3.tgz"}, {name: "reference, testing-relative-trailing-slash repo", ref: "testing-relative-trailing-slash/foo", expect: "http://example.com/helm/charts/foo-1.2.3.tgz"}, {name: "reference, testing-relative-trailing-slash repo", ref: "testing-relative-trailing-slash/bar", expect: "http://example.com/helm/bar-1.2.3.tgz"}, {name: "encoded URL", ref: "encoded-url/foobar", expect: "http://example.com/with%2Fslash/charts/foobar-4.2.1.tgz"}, diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index e884e12d4..348c78edb 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -25,7 +25,6 @@ import ( "log" "net/url" "os" - "path" "path/filepath" "regexp" "strings" @@ -728,7 +727,6 @@ func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]* } for _, cr := range repos { - if urlutil.Equal(repoURL, cr.Config.URL) { var entry repo.ChartVersions entry, err = findEntryByName(name, cr) @@ -745,7 +743,7 @@ func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]* //nolint:nakedret return } - url, err = normalizeURL(repoURL, ve.URLs[0]) + url, err = repo.ResolveReferenceURL(repoURL, ve.URLs[0]) if err != nil { //nolint:nakedret return @@ -811,24 +809,6 @@ func versionEquals(v1, v2 string) bool { return sv1.Equal(sv2) } -func normalizeURL(baseURL, urlOrPath string) (string, error) { - u, err := url.Parse(urlOrPath) - if err != nil { - return urlOrPath, err - } - if u.IsAbs() { - return u.String(), nil - } - u2, err := url.Parse(baseURL) - if err != nil { - return urlOrPath, fmt.Errorf("base URL failed to parse: %w", err) - } - - u2.RawPath = path.Join(u2.RawPath, urlOrPath) - u2.Path = path.Join(u2.Path, urlOrPath) - return u2.String(), nil -} - // loadChartRepositories reads the repositories.yaml, and then builds a map of // ChartRepositories. // diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index fecc8fbef..590686fd5 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -53,26 +53,6 @@ func TestVersionEquals(t *testing.T) { } } -func TestNormalizeURL(t *testing.T) { - tests := []struct { - name, base, path, expect string - }{ - {name: "basic URL", base: "https://example.com", path: "http://helm.sh/foo", expect: "http://helm.sh/foo"}, - {name: "relative path", base: "https://helm.sh/charts", path: "foo", expect: "https://helm.sh/charts/foo"}, - {name: "Encoded path", base: "https://helm.sh/a%2Fb/charts", path: "foo", expect: "https://helm.sh/a%2Fb/charts/foo"}, - } - - for _, tt := range tests { - got, err := normalizeURL(tt.base, tt.path) - if err != nil { - t.Errorf("%s: error %s", tt.name, err) - continue - } else if got != tt.expect { - t.Errorf("%s: expected %q, got %q", tt.name, tt.expect, got) - } - } -} - func TestFindChartURL(t *testing.T) { var b bytes.Buffer m := &Manager{ @@ -134,6 +114,31 @@ func TestFindChartURL(t *testing.T) { if passcredentialsall != false { t.Errorf("Unexpected passcredentialsall %t", passcredentialsall) } + + name = "baz" + version = "1.2.3" + repoURL = "http://example.com/helm" + + churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(name, version, repoURL, repos) + if err != nil { + t.Fatal(err) + } + + if churl != "http://example.com/path/to/baz-1.2.3.tgz" { + t.Errorf("Unexpected URL %q", churl) + } + if username != "" { + t.Errorf("Unexpected username %q", username) + } + if password != "" { + t.Errorf("Unexpected password %q", password) + } + if passcredentialsall != false { + t.Errorf("Unexpected passcredentialsall %t", passcredentialsall) + } + if insecureSkipTLSVerify { + t.Errorf("Unexpected insecureSkipTLSVerify %t", insecureSkipTLSVerify) + } } func TestGetRepoNames(t *testing.T) { diff --git a/pkg/downloader/testdata/repository/testing-relative-index.yaml b/pkg/downloader/testdata/repository/testing-relative-index.yaml index ba27ed257..9524daf6e 100644 --- a/pkg/downloader/testdata/repository/testing-relative-index.yaml +++ b/pkg/downloader/testdata/repository/testing-relative-index.yaml @@ -26,3 +26,16 @@ entries: version: 1.2.3 checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d apiVersion: v2 + baz: + - name: baz + description: Baz Chart With Absolute Path + home: https://helm.sh/helm + keywords: [] + maintainers: [] + sources: + - https://github.com/helm/charts + urls: + - /path/to/baz-1.2.3.tgz + version: 1.2.3 + checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d + apiVersion: v2 diff --git a/pkg/repo/chartrepo_test.go b/pkg/repo/chartrepo_test.go index c29c95a7e..bc15560c9 100644 --- a/pkg/repo/chartrepo_test.go +++ b/pkg/repo/chartrepo_test.go @@ -224,11 +224,15 @@ func TestResolveReferenceURL(t *testing.T) { for _, tt := range []struct { baseURL, refURL, chartURL string }{ + {"http://localhost:8123/", "/nginx-0.2.0.tgz", "http://localhost:8123/nginx-0.2.0.tgz"}, {"http://localhost:8123/charts/", "nginx-0.2.0.tgz", "http://localhost:8123/charts/nginx-0.2.0.tgz"}, + {"http://localhost:8123/charts/", "/nginx-0.2.0.tgz", "http://localhost:8123/nginx-0.2.0.tgz"}, {"http://localhost:8123/charts-with-no-trailing-slash", "nginx-0.2.0.tgz", "http://localhost:8123/charts-with-no-trailing-slash/nginx-0.2.0.tgz"}, {"http://localhost:8123", "https://charts.helm.sh/stable/nginx-0.2.0.tgz", "https://charts.helm.sh/stable/nginx-0.2.0.tgz"}, {"http://localhost:8123/charts%2fwith%2fescaped%2fslash", "nginx-0.2.0.tgz", "http://localhost:8123/charts%2fwith%2fescaped%2fslash/nginx-0.2.0.tgz"}, + {"http://localhost:8123/charts%2fwith%2fescaped%2fslash", "/nginx-0.2.0.tgz", "http://localhost:8123/nginx-0.2.0.tgz"}, {"http://localhost:8123/charts?with=queryparameter", "nginx-0.2.0.tgz", "http://localhost:8123/charts/nginx-0.2.0.tgz?with=queryparameter"}, + {"http://localhost:8123/charts?with=queryparameter", "/nginx-0.2.0.tgz", "http://localhost:8123/nginx-0.2.0.tgz?with=queryparameter"}, } { chartURL, err := ResolveReferenceURL(tt.baseURL, tt.refURL) if err != nil {