oci: address artifact extraction review feedback

Author: JAORMX

oci/artifact/tar.go

@@ -112,7 +112,8 @@ func ExtractTarWithLimit(data []byte, maxFileSize int64) ([]FileEntry, error) {
 		}
 
 		// Reject path traversal
-		if err := validateTarPath(hdr.Name); err != nil {
+		cleanedPath, err := validateTarPath(hdr.Name)
+		if err != nil {
 			return nil, err
 		}
 
@@ -148,7 +149,7 @@ func ExtractTarWithLimit(data []byte, maxFileSize int64) ([]FileEntry, error) {
 		}
 
 		files = append(files, FileEntry{
-			Path:    hdr.Name,
+			Path:    cleanedPath,
 			Content: content,
 			Mode:    hdr.Mode,
 		})
@@ -157,16 +158,20 @@ func ExtractTarWithLimit(data []byte, maxFileSize int64) ([]FileEntry, error) {
 	return files, nil
 }
 
-// validateTarPath checks that a tar entry path is safe.
-func validateTarPath(p string) error {
+// validateTarPath checks that a tar entry path is safe and returns its cleaned path.
+func validateTarPath(p string) (string, error) {
+	if strings.Contains(p, `\\`) {
+		return "", fmt.Errorf("backslash path separators not allowed in archive: %s", p)
+	}
+
 	// path.Clean resolves all ".." segments; any remaining leading ".."
 	// means the path escapes the archive root.
 	cleaned := path.Clean(p)
-	if strings.HasPrefix(cleaned, "..") {
-		return fmt.Errorf("path traversal detected in archive: %s", p)
+	if cleaned == ".." || strings.HasPrefix(cleaned, "../") {
+		return "", fmt.Errorf("path traversal detected in archive: %s", p)
 	}
 	if path.IsAbs(cleaned) {
-		return fmt.Errorf("absolute path not allowed in archive: %s", p)
+		return "", fmt.Errorf("absolute path not allowed in archive: %s", p)
 	}
-	return nil
+	return cleaned, nil
 }

oci/artifact/tar_test.go

@@ -205,6 +205,7 @@ func TestExtractTar_RejectsPathTraversal(t *testing.T) {
 		{name: "dotdot prefix", path: "../etc/passwd"},
 		{name: "dotdot in middle", path: "foo/../../etc/passwd"},
 		{name: "absolute path", path: "/etc/passwd"},
+		{name: "windows traversal", path: `foo\\..\\..\\etc\\passwd`},
 	}
 
 	for _, tt := range tests {
@@ -231,6 +232,30 @@ func TestExtractTar_RejectsPathTraversal(t *testing.T) {
 	}
 }
 
+func TestExtractTar_CleansPath(t *testing.T) {
+	t.Parallel()
+
+	var buf bytes.Buffer
+	tw := tar.NewWriter(&buf)
+
+	content := []byte("test")
+	require.NoError(t, tw.WriteHeader(&tar.Header{
+		Name:     "foo/./bar.txt",
+		Size:     int64(len(content)),
+		Typeflag: tar.TypeReg,
+		Mode:     0644,
+	}))
+	_, err := tw.Write(content)
+	require.NoError(t, err)
+	require.NoError(t, tw.Close())
+
+	files, err := ExtractTar(buf.Bytes())
+	require.NoError(t, err)
+	require.Len(t, files, 1)
+	assert.Equal(t, "foo/bar.txt", files[0].Path)
+	assert.Equal(t, content, files[0].Content)
+}
+
 func TestExtractTarWithLimit_RejectsOversized(t *testing.T) {
 	t.Parallel()
 

oci/artifact/validate.go

@@ -66,7 +66,7 @@ func (v *ValidatingTarget) Tag(ctx context.Context, desc ocispec.Descriptor, ref
 // Push validates size and structure limits before delegating to the inner target.
 func (v *ValidatingTarget) Push(ctx context.Context, desc ocispec.Descriptor, content io.Reader) error {
 	maxSize := MaxBlobSize
-	if IsManifestMediaType(desc.MediaType) {
+	if isManifestMediaType(desc.MediaType) {
 		maxSize = MaxManifestSize
 	}
 
@@ -101,20 +101,20 @@ func (v *ValidatingTarget) Push(ctx context.Context, desc ocispec.Descriptor, co
 	}
 
 	// Validate manifest/index structure limits
-	if err := ValidateManifestCounts(desc.MediaType, data); err != nil {
+	if err := validateManifestCounts(desc.MediaType, data); err != nil {
 		return err
 	}
 
 	return v.inner.Push(ctx, desc, bytes.NewReader(data))
 }
 
-// ValidateManifestCounts checks layer/manifest counts for resource exhaustion prevention.
+// validateManifestCounts checks layer/manifest counts for resource exhaustion prevention.
 //
 // It only inspects media types it recognizes (image index and image manifest).
 // For any other media type it returns nil without examining the data. A nil
 // return therefore means "no count violation was detected", not "this manifest
 // is safe" — callers must not treat a nil return as a general safety guarantee.
-func ValidateManifestCounts(mediaType string, data []byte) error {
+func validateManifestCounts(mediaType string, data []byte) error {
 	switch mediaType {
 	case ocispec.MediaTypeImageIndex:
 		var index ocispec.Index
@@ -142,8 +142,8 @@ func ValidateManifestCounts(mediaType string, data []byte) error {
 	return nil
 }
 
-// IsManifestMediaType returns true if the media type is a manifest or index type.
-func IsManifestMediaType(mediaType string) bool {
+// isManifestMediaType returns true if the media type is a manifest or index type.
+func isManifestMediaType(mediaType string) bool {
 	switch mediaType {
 	case ocispec.MediaTypeImageManifest, ocispec.MediaTypeImageIndex,
 		"application/vnd.docker.distribution.manifest.v2+json",

oci/artifact/validate_test.go

@@ -35,7 +35,7 @@ func TestIsManifestMediaType(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			assert.Equal(t, tt.want, IsManifestMediaType(tt.mediaType))
+			assert.Equal(t, tt.want, isManifestMediaType(tt.mediaType))
 		})
 	}
 }
@@ -198,7 +198,7 @@ func TestValidateManifestCounts(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			err := ValidateManifestCounts(tt.mediaType, tt.data)
+			err := validateManifestCounts(tt.mediaType, tt.data)
 			if tt.wantErr {
 				require.Error(t, err)
 				assert.Contains(t, err.Error(), tt.errSubstr)

oci/skills/artifact_aliases.go

@@ -18,69 +18,92 @@ import (
 // Type aliases for tar/gzip primitives.
 type (
 	// FileEntry represents a file to include in a tar archive.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.FileEntry.
 	FileEntry = artifact.FileEntry
 	// TarOptions configures reproducible tar archive creation.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.TarOptions.
 	TarOptions = artifact.TarOptions
 	// GzipOptions configures reproducible gzip compression.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.GzipOptions.
 	GzipOptions = artifact.GzipOptions
 )
 
 // Function forwarding for tar primitives.
 var (
 	// DefaultTarOptions returns default options for reproducible tar archives.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.DefaultTarOptions.
 	DefaultTarOptions = artifact.DefaultTarOptions
 	// CreateTar creates a reproducible tar archive from the given files.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.CreateTar.
 	CreateTar = artifact.CreateTar
 	// ExtractTar extracts files from a tar archive.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.ExtractTar.
 	ExtractTar = artifact.ExtractTar
 	// ExtractTarWithLimit extracts files from a tar archive with a per-file size limit.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.ExtractTarWithLimit.
 	ExtractTarWithLimit = artifact.ExtractTarWithLimit
 )
 
 // Function forwarding for gzip primitives.
 var (
 	// DefaultGzipOptions returns default options for reproducible gzip compression.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.DefaultGzipOptions.
 	DefaultGzipOptions = artifact.DefaultGzipOptions
 	// Compress creates a reproducible gzip compressed byte slice.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.Compress.
 	Compress = artifact.Compress
 	// Decompress decompresses gzip data.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.Decompress.
 	Decompress = artifact.Decompress
 	// DecompressWithLimit decompresses gzip data with a size limit.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.DecompressWithLimit.
 	DecompressWithLimit = artifact.DecompressWithLimit
 	// CompressTar creates a reproducible .tar.gz from the given files.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.CompressTar.
 	CompressTar = artifact.CompressTar
 	// DecompressTar extracts files from a .tar.gz archive.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.DecompressTar.
 	DecompressTar = artifact.DecompressTar
 )
 
 // Function forwarding for platform helpers.
 var (
 	// PlatformString returns the platform in "os/arch" or "os/arch/variant" format.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.PlatformString.
 	PlatformString = artifact.PlatformString
 	// ParsePlatform parses a platform string in "os/arch" or "os/arch/variant" format.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.ParsePlatform.
 	ParsePlatform = artifact.ParsePlatform
 	// DefaultPlatforms are the default platforms for artifacts.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.DefaultPlatforms.
 	DefaultPlatforms = artifact.DefaultPlatforms
 )
 
 // Size limit constants re-exported from the artifact package.
 const (
 	// MaxTarFileSize is the maximum size of a single file in a tar archive (100MB).
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.MaxTarFileSize.
 	MaxTarFileSize = artifact.MaxTarFileSize
 	// MaxDecompressedSize is the maximum size of decompressed data (100MB).
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.MaxDecompressedSize.
 	MaxDecompressedSize = artifact.MaxDecompressedSize
 	// MaxManifestSize is the maximum size of a manifest (1MB).
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.MaxManifestSize.
 	MaxManifestSize = artifact.MaxManifestSize
 	// MaxBlobSize is the maximum size of a blob (100MB).
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.MaxBlobSize.
 	MaxBlobSize = artifact.MaxBlobSize
 )
 
 // OS and architecture constants for OCI platform specifications.
 const (
 	// OSLinux is the Linux OS identifier used in OCI platform specs.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.OSLinux.
 	OSLinux = artifact.OSLinux
 	// ArchAMD64 is the x86-64 architecture identifier used in OCI platform specs.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.ArchAMD64.
 	ArchAMD64 = artifact.ArchAMD64
 	// ArchARM64 is the 64-bit ARM architecture identifier used in OCI platform specs.
+	// Deprecated: use github.com/stacklok/toolhive-core/oci/artifact.ArchARM64.
 	ArchARM64 = artifact.ArchARM64
 )