fix: make sure that helm downloader does not overwrite chart with provenance data (#1481)

api/ocm/extensions/download/handlers/helm/download_test.go

@@ -4,12 +4,12 @@ import (
 	. "github.com/mandelsoft/goutils/testutils"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
-
-	. "ocm.software/ocm/api/helper/builder"
+	"helm.sh/helm/v3/pkg/registry"
 
 	"github.com/mandelsoft/filepath/pkg/filepath"
 	"github.com/mandelsoft/vfs/pkg/vfs"
 
+	. "ocm.software/ocm/api/helper/builder"
 	envhelper "ocm.software/ocm/api/helper/env"
 	"ocm.software/ocm/api/oci/artdesc"
 	"ocm.software/ocm/api/oci/extensions/repositories/artifactset"
@@ -21,6 +21,7 @@ import (
 	"ocm.software/ocm/api/ocm/selectors"
 	"ocm.software/ocm/api/utils/accessio"
 	"ocm.software/ocm/api/utils/accessobj"
+	"ocm.software/ocm/api/utils/blobaccess/dirtree"
 	"ocm.software/ocm/api/utils/tarutils"
 )
 
@@ -30,10 +31,11 @@ const (
 	Version     = "v1.0.0"
 	OCIResource = "helm"
 
-	ArtifactType       = "NotHelmChart"
-	SpecialOCIResource = "specialhelm"
-	UnusualOCIResource = "unusualhelm"
-	LegacyOCIResource  = "legacyhelm"
+	ArtifactType          = "NotHelmChart"
+	SpecialOCIResource    = "specialhelm"
+	UnusualOCIResource    = "unusualhelm"
+	LegacyOCIResource     = "legacyhelm"
+	HelmViaChartMediaType = "helmviaaccess"
 )
 
 var _ = Describe("upload", func() {
@@ -57,6 +59,9 @@ var _ = Describe("upload", func() {
 					env.Resource(LegacyOCIResource, Version, resourcetypes.HELM_CHART, v1.LocalRelation, func() {
 						env.BlobFromFile(artifactset.MediaType(artdesc.MediaTypeImageManifest), filepath.Join("/testdata/legacy-pre-hip-helm-chart.tgz"))
 					})
+					env.Resource(HelmViaChartMediaType, Version, resourcetypes.HELM_CHART, v1.LocalRelation, func() {
+						env.BlobFromDirTree("/testdata/test-chart", dirtree.WithMimeType(registry.ChartLayerMediaType), dirtree.WithPreserveDir(true), dirtree.WithCompressWithGzip(true))
+					})
 				})
 			})
 		})
@@ -85,21 +90,47 @@ var _ = Describe("upload", func() {
 
 	It("successfully download unusual artifacts with non-defacto helm chart order", func() {
 		MustBeSuccessful(download.RegisterHandlerByName(env, helm.PATH, nil, download.ForArtifactType(ArtifactType)))
-
 		repo := Must(ctf.Open(env.OCMContext(), accessobj.ACC_READONLY, CTFPath, 0o777, env))
 		cv := Must(repo.LookupComponentVersion(Component, Version))
 		path := Must(download.DownloadResource(env.OCMContext(), Must(cv.SelectResources(selectors.Identity(v1.Identity{"name": UnusualOCIResource})))[0], "/resource", download.WithFileSystem(env.FileSystem())))
 		MustBeSuccessful(tarutils.ExtractArchiveToFs(env.FileSystem(), path, env.FileSystem()))
 		Expect(Must(vfs.FileExists(env.FileSystem(), "/postgresql/Chart.yaml"))).To(BeTrue())
 	})
 
-	It("successfully download artifacts with helm chart content with legacy content type", func() {
+	It("successfully download unusual artifacts with non-defacto helm chart order and path given to tgz", func() {
 		MustBeSuccessful(download.RegisterHandlerByName(env, helm.PATH, nil, download.ForArtifactType(ArtifactType)))
+		repo := Must(ctf.Open(env.OCMContext(), accessobj.ACC_READONLY, CTFPath, 0o777, env))
+		cv := Must(repo.LookupComponentVersion(Component, Version))
+		path := Must(download.DownloadResource(env.OCMContext(), Must(cv.SelectResources(selectors.Identity(v1.Identity{"name": UnusualOCIResource})))[0], "/path.tgz", download.WithFileSystem(env.FileSystem())))
+		MustBeSuccessful(tarutils.ExtractArchiveToFs(env.FileSystem(), path, env.FileSystem()))
+		Expect(Must(vfs.FileExists(env.FileSystem(), "/postgresql/Chart.yaml"))).To(BeTrue())
+	})
 
+	It("successfully download unusual artifacts with non-defacto helm chart order and path given to tar.gz", func() {
+		MustBeSuccessful(download.RegisterHandlerByName(env, helm.PATH, nil, download.ForArtifactType(ArtifactType)))
+		repo := Must(ctf.Open(env.OCMContext(), accessobj.ACC_READONLY, CTFPath, 0o777, env))
+		cv := Must(repo.LookupComponentVersion(Component, Version))
+		path := Must(download.DownloadResource(env.OCMContext(), Must(cv.SelectResources(selectors.Identity(v1.Identity{"name": UnusualOCIResource})))[0], "/path.tar.gz", download.WithFileSystem(env.FileSystem())))
+		MustBeSuccessful(tarutils.ExtractArchiveToFs(env.FileSystem(), path, env.FileSystem()))
+		Expect(Must(vfs.FileExists(env.FileSystem(), "/postgresql/Chart.yaml"))).To(BeTrue())
+	})
+
+	It("successfully download unusual artifacts with helm media type", func() {
+		MustBeSuccessful(download.RegisterHandlerByName(env, helm.PATH, nil, download.ForArtifactType(ArtifactType)))
+		repo := Must(ctf.Open(env.OCMContext(), accessobj.ACC_READONLY, CTFPath, 0o777, env))
+		cv := Must(repo.LookupComponentVersion(Component, Version))
+		path := Must(download.DownloadResource(env.OCMContext(), Must(cv.SelectResources(selectors.Identity(v1.Identity{"name": HelmViaChartMediaType})))[0], "/resource", download.WithFileSystem(env.FileSystem())))
+		MustBeSuccessful(tarutils.ExtractArchiveToFs(env.FileSystem(), path, env.FileSystem()))
+		Expect(Must(vfs.FileExists(env.FileSystem(), "test-chart/Chart.yaml"))).To(BeTrue())
+	})
+
+	It("successfully download artifacts with helm chart content with legacy content type", func() {
+		MustBeSuccessful(download.RegisterHandlerByName(env, helm.PATH, nil, download.ForArtifactType(ArtifactType)))
 		repo := Must(ctf.Open(env.OCMContext(), accessobj.ACC_READONLY, CTFPath, 0o777, env))
 		cv := Must(repo.LookupComponentVersion(Component, Version))
 		path := Must(download.DownloadResource(env.OCMContext(), Must(cv.SelectResources(selectors.Identity(v1.Identity{"name": LegacyOCIResource})))[0], "/resource", download.WithFileSystem(env.FileSystem())))
 		MustBeSuccessful(tarutils.ExtractArchiveToFs(env.FileSystem(), path, env.FileSystem()))
 		Expect(Must(vfs.FileExists(env.FileSystem(), "/ingress-nginx/Chart.yaml"))).To(BeTrue())
 	})
+
 })

api/ocm/extensions/download/handlers/helm/handler.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"path/filepath"
 	"strings"
 
 	"github.com/mandelsoft/goutils/finalizer"
@@ -155,9 +156,8 @@ func download(p common.Printer, art oci.ArtifactAccess, path string, fs vfs.File
 			return "", "", err
 		}
 		finalize.Close(provBlob)
-
-		prov = chart[:len(chart)-3] + "prov"
-		if err := write(p, provBlob, path, fs); err != nil {
+		prov = trimExtN(chart, 2) + ".prov"
+		if err := write(p, provBlob, prov, fs); err != nil {
 			return "", "", err
 		}
 	} else if !errors.Is(err, ErrNoMatchingLayer) { // Ignore if no provenance layer is found, because its optional.
@@ -167,6 +167,19 @@ func download(p common.Printer, art oci.ArtifactAccess, path string, fs vfs.File
 	return chart, prov, nil
 }
 
+// trimExtN trims the file extension from the path, at most n times.
+// This is useful for removing the ".tgz" or ".tar.gz" suffix from a file name.
+func trimExtN(path string, n int) string {
+	for i := 0; i < n; i++ {
+		trimmed := strings.TrimSuffix(path, filepath.Ext(path))
+		if trimmed == path {
+			break
+		}
+		path = trimmed
+	}
+	return path
+}
+
 func findLayer(layers []ocispec.Descriptor, mediaType string) (*ocispec.Descriptor, error) {
 	var candidates []*ocispec.Descriptor
 

api/ocm/extensions/download/handlers/helm/handler_test.go

@@ -0,0 +1,45 @@
+package helm
+
+import (
+	"testing"
+)
+
+func TestAssureArchiveSuffix(t *testing.T) {
+	tests := []struct {
+		input    string
+		expected string
+	}{
+		{"chart", "chart.tgz"},
+		{"chart.tgz", "chart.tgz"},
+		{"chart.tar.gz", "chart.tar.gz"},
+		{"archive.zip", "archive.zip.tgz"},
+	}
+
+	for _, tt := range tests {
+		result := AssureArchiveSuffix(tt.input)
+		if result != tt.expected {
+			t.Errorf("AssureArchiveSuffix(%q) = %q; want %q", tt.input, result, tt.expected)
+		}
+	}
+}
+
+func TestTrimExtN(t *testing.T) {
+	tests := []struct {
+		input    string
+		n        int
+		expected string
+	}{
+		{"chart.tgz", 1, "chart"},
+		{"chart.tar.gz", 1, "chart.tar"},
+		{"chart.tar.gz", 2, "chart"},
+		{"archive.zip", 1, "archive"},
+		{"archive", 1, "archive"},
+	}
+
+	for _, tt := range tests {
+		result := trimExtN(tt.input, tt.n)
+		if result != tt.expected {
+			t.Errorf("trimExtN(%q, %d) = %q; want %q", tt.input, tt.n, result, tt.expected)
+		}
+	}
+}