Repository: regclient/regclient
Pull request #707 (2024-03-30 16:11:30)

Validate media types in created artifacts
<!--

Commits must be signed indicating your agreement to the [DCO](https://developercertificate.org/).
See [DCO missing](https://github.com/src-d/guide/blob/master/developer-community/fix-DCO.md) for steps to fix a missing signoff.

-->

### Fixes issue

It's possible with regctl to create artifacts with invalid media types in the artifact type, layers, and config descriptor.
<!-- If this is a bug fix, include "fixes #xxxx", or "closes #xxxx" -->

### Describe the change

This adds media type validation required by OCI, but only on the creation of artifacts with regctl. Trying to implement this at a lower level in regclient may break the ability to ingest and copy poorly formatted artifacts. This attempts to follow the robustness principle, conservative in what we generate, but liberal in what we accept from others.
<!-- Include the type of change: bug fix, new feature, breaking change, documentation update -->
<!-- Describe what was changed, why the change was made, and how it was implemented -->

### How to verify it

```shell
echo hello | regctl artifact put --artifact-type broken localhost:5000/artifact:bad
```
<!-- Include steps that can be taken to verify the change -->

### Changelog text

- Fix: Validate media types on `regctl artifact put`.
<!-- If the release changelog should have an entry for this, include it here -->

### Please verify and check that the pull request fulfills the following requirements

<!-- Mark the following with an [X] to verify they are included -->

- [X] Tests have been added or not applicable
- [X] Documentation has been added, updated, or not applicable
- [X] Changes have been rebased to main
- [X] Multiple commits to the same code have been squashed

<!-- markdownlint-disable-file MD041 -->



Patch:
diff --git a/cmd/regctl/artifact.go b/cmd/regctl/artifact.go
index ceaeb1b..5c6aee8 100644
--- a/cmd/regctl/artifact.go
+++ b/cmd/regctl/artifact.go
@@ -622,7 +622,7 @@ func (artifactOpts *artifactCmd) runArtifactPut(cmd *cobra.Command, args []strin
 	case "", mediatype.OCI1Manifest:
 		hasConfig = true
 	default:
-		return fmt.Errorf("unsupported manifest media type: %s", artifactOpts.artifactMT)
+		return fmt.Errorf("unsupported manifest media type: %s%.0w", artifactOpts.artifactMT, errs.ErrUnsupportedMediaType)
 	}
 
 	// validate inputs
@@ -656,6 +656,17 @@ func (artifactOpts *artifactCmd) runArtifactPut(cmd *cobra.Command, args []strin
 	}
 
 	// validate/set artifactType and config.mediaType
+	if artifactOpts.artifactConfigMT != "" && !mediatype.Valid(artifactOpts.artifactConfigMT) {
+		return fmt.Errorf("invalid media type: %s%.0w", artifactOpts.artifactConfigMT, errs.ErrUnsupportedMediaType)
+	}
+	if artifactOpts.artifactType != "" && !mediatype.Valid(artifactOpts.artifactType) {
+		return fmt.Errorf("invalid media type: %s%.0w", artifactOpts.artifactType, errs.ErrUnsupportedMediaType)
+	}
+	for _, mt := range artifactOpts.artifactFileMT {
+		if !mediatype.Valid(mt) {
+			return fmt.Errorf("invalid media type: %s%.0w", mt, errs.ErrUnsupportedMediaType)
+		}
+	}
 	if hasConfig && artifactOpts.artifactConfigMT == "" {
 		if artifactOpts.artifactConfig == "" {
 			artifactOpts.artifactConfigMT = mediatype.OCI1Empty
diff --git a/types/mediatype/mediatype.go b/types/mediatype/mediatype.go
index 914ac53..5f22724 100644
--- a/types/mediatype/mediatype.go
+++ b/types/mediatype/mediatype.go
@@ -1,7 +1,10 @@
 // Package mediatype defines well known media types.
 package mediatype
 
-import "strings"
+import (
+	"regexp"
+	"strings"
+)
 
 const (
 	// Docker1Manifest deprecated media type for docker schema1 manifests.
@@ -49,3 +52,10 @@ func Base(orig string) string {
 	base, _, _ := strings.Cut(orig, ";")
 	return strings.TrimSpace(strings.ToLower(base))
 }
+
+// Valid returns true if the media type matches the rfc6838 4.2 naming requirements.
+func Valid(mt string) bool {
+	return validateRegexp.MatchString(mt)
+}
+
+var validateRegexp = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9!#$&^_.+-]{0,126}/[A-Za-z0-9][A-Za-z0-9!#$&^_.+-]{0,126}$`)


Test patch:
diff --git a/cmd/regctl/artifact_test.go b/cmd/regctl/artifact_test.go
index c049ca8..1d57c3b 100644
--- a/cmd/regctl/artifact_test.go
+++ b/cmd/regctl/artifact_test.go
@@ -195,17 +195,17 @@ func TestArtifactPut(t *testing.T) {
 		},
 		{
 			name: "Put artifact example conf data",
-			args: []string{"artifact", "put", "--artifact-type", "", "--config-type", "application/vnd.example", "--config-file", testConfName, "ocidir://" + testDir + ":put-example-conf-data"},
+			args: []string{"artifact", "put", "--config-type", "application/vnd.example", "--config-file", testConfName, "ocidir://" + testDir + ":put-example-conf-data"},
 			in:   testData,
 		},
 		{
 			name: "Put artifact example conf data and file",
-			args: []string{"artifact", "put", "--artifact-type", "", "--config-type", "application/vnd.example", "--config-file", testConfName, "--file", testFileName, "ocidir://" + testDir + ":put-example-file-data"},
+			args: []string{"artifact", "put", "--config-type", "application/vnd.example", "--config-file", testConfName, "--file", testFileName, "ocidir://" + testDir + ":put-example-file-data"},
 			in:   testData,
 		},
 		{
 			name: "Put artifact example conf data and file stripped",
-			args: []string{"artifact", "put", "--artifact-type", "", "--config-type", "application/vnd.example", "--config-file", testConfName, "--file", testFileName, "--file-title", "--strip-dirs", "ocidir://" + testDir + ":put-example-file-data"},
+			args: []string{"artifact", "put", "--config-type", "application/vnd.example", "--config-file", testConfName, "--file", testFileName, "--file-title", "--strip-dirs", "ocidir://" + testDir + ":put-example-file-data"},
 			in:   testData,
 		},
 		{
@@ -223,6 +223,24 @@ func TestArtifactPut(t *testing.T) {
 			args: []string{"artifact", "put", "--artifact-type", "application/vnd.example", "--annotation", "test=b", "--platform", "linux/arm64", "--index", "ocidir://" + testDir + ":index"},
 			in:   testData,
 		},
+		{
+			name:      "Invalid-artifact-media-type",
+			args:      []string{"artifact", "put", "--artifact-type", "application/vnd.example;version=1.0", "ocidir://" + testDir + ":err"},
+			in:        testData,
+			expectErr: errs.ErrUnsupportedMediaType,
+		},
+		{
+			name:      "Invalid-config-media-type",
+			args:      []string{"artifact", "put", "--config-type", "application/vnd.example;version=1.0", "--config-file", testConfName, "ocidir://" + testDir + ":err"},
+			in:        testData,
+			expectErr: errs.ErrUnsupportedMediaType,
+		},
+		{
+			name:      "Invalid-file-media-type",
+			args:      []string{"artifact", "put", "--artifact-type", "application/vnd.example", "--file", testFileName, "--file-media-type", "application/vnd.example;version=1.0", "ocidir://" + testDir + ":err"},
+			in:        testData,
+			expectErr: errs.ErrUnsupportedMediaType,
+		},
 	}
 	for _, tc := range tt {
 		t.Run(tc.name, func(t *testing.T) {
@@ -233,7 +251,7 @@ func TestArtifactPut(t *testing.T) {
 			out, err := cobraTest(t, &cobraOpts, tc.args...)
 			if tc.expectErr != nil {
 				if err == nil {
-					t.Errorf("did not receive expected error: %v", tc.expectErr)
+					t.Errorf("did not fail with error: %v", tc.expectErr)
 				} else if !errors.Is(err, tc.expectErr) && err.Error() != tc.expectErr.Error() {
 					t.Errorf("unexpected error, received %v, expected %v", err, tc.expectErr)
 				}
diff --git a/types/mediatype/mediatype_test.go b/types/mediatype/mediatype_test.go
index 3fd97ad..bad91b0 100644
--- a/types/mediatype/mediatype_test.go
+++ b/types/mediatype/mediatype_test.go
@@ -1,8 +1,11 @@
 package mediatype
 
-import "testing"
+import (
+	"strings"
+	"testing"
+)
 
-func TestMediaTypeBase(t *testing.T) {
+func TestBase(t *testing.T) {
 	t.Parallel()
 	tt := []struct {
 		name   string
@@ -30,3 +33,61 @@ func TestMediaTypeBase(t *testing.T) {
 		})
 	}
 }
+
+func TestValid(t *testing.T) {
+	t.Parallel()
+	tt := []struct {
+		name   string
+		mt     string
+		expect bool
+	}{
+		{
+			name:   "Empty",
+			mt:     "",
+			expect: false,
+		},
+		{
+			name:   "OCI-Index",
+			mt:     OCI1ManifestList,
+			expect: true,
+		},
+		{
+			name:   "OCI-Index-param",
+			mt:     "application/vnd.oci.image.index.v1+json; charset=utf-8",
+			expect: false,
+		},
+		{
+			name:   "no-slash",
+			mt:     "application",
+			expect: false,
+		},
+		{
+			name:   "no-subtype",
+			mt:     "application/",
+			expect: false,
+		},
+		{
+			name:   "invalid-character",
+			mt:     "application/star.*",
+			expect: false,
+		},
+		{
+			name:   "missing-major-type",
+			mt:     "/json",
+			expect: false,
+		},
+		{
+			name:   "too-long",
+			mt:     "application/" + strings.Repeat("a", 128),
+			expect: false,
+		},
+	}
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			valid := Valid(tc.mt)
+			if tc.expect != valid {
+				t.Errorf("invalid result: expected \"%t\", received \"%t\"", tc.expect, valid)
+			}
+		})
+	}
+}