Repository: oras-project/oras (go)

Issue:
oras push/attach should not allow colon in media type only on Windows
Using oras 0.16.0, I can use below command to upload a layer with colon in the media type on Windows:
```
oras push localhost:5000/test:path .\KEYS:test:test:test
```

See layers.mediatype below, this command will generate a layer with `test:test:test` as an invalid media type
```
> oras manifest fetch localhost:5000/test:path --pretty
{
  "schemaVersion": 2,
  "mediaType": "application/vnd.oci.image.manifest.v1+json",
  "config": {
    "mediaType": "application/vnd.unknown.config.v1+json",
    "digest": "sha256:44136fa355b3678a1146ad16f7e8649e94fb4fc21fe77e8310c060f61caaff8a",
    "size": 2
  },
  "layers": [
    {
      "mediaType": "test:test:test",
      "digest": "sha256:b79b04d85ebb8b289bde13387a5c6f3d98448d5a51937e6f94239303ffc2add8",
      "size": 15020,
      "annotations": {
        "org.opencontainers.image.title": "KEYS"
      }
    }
  ]
}
```

According to [rfc2045 ](https://datatracker.ietf.org/doc/html/rfc2045#section-5.1), colon is a special character and should not be in the media type. So the push command should show an error same to linux/darwin build:
```
> oras push localhost:5000/test:path .\KEYS:test:test:test
Error: failed to stat .KEYS:test:test: stat .KEYS:test:test: no such file or directory
```

Pull request description:
fix!: fix colon in media type for windows
Fixes #697 
Signed-off-by: Billy Zha <jinzha1@microsoft.com>

Patch:
diff --git a/cmd/oras/file.go b/cmd/oras/file.go
index 5b5d1ac..ca90a54 100644
--- a/cmd/oras/file.go
+++ b/cmd/oras/file.go
@@ -22,12 +22,16 @@ import (
 
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"oras.land/oras-go/v2/content/file"
+	"oras.land/oras/cmd/oras/internal/fileref"
 )
 
 func loadFiles(ctx context.Context, store *file.Store, annotations map[string]map[string]string, fileRefs []string, verbose bool) ([]ocispec.Descriptor, error) {
 	var files []ocispec.Descriptor
 	for _, fileRef := range fileRefs {
-		filename, mediaType := parseFileReference(fileRef, "")
+		filename, mediaType, err := fileref.Parse(fileRef, "")
+		if err != nil {
+			return nil, err
+		}
 
 		// get shortest absolute path as unique name
 		name := filepath.Clean(filename)
diff --git a/cmd/oras/file_windows.go b/cmd/oras/file_windows.go
deleted file mode 100644
index 9cbef78..0000000
--- a/cmd/oras/file_windows.go
+++ /dev/null
@@ -1,40 +0,0 @@
-/*
-Copyright The ORAS Authors.
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-package main
-
-import (
-	"strings"
-	"unicode"
-)
-
-// parseFileReference parses file reference on windows.
-// Windows systems does not allow ':' in the file path except for drive letter.
-func parseFileReference(reference string, mediaType string) (filePath, mediatype string) {
-	i := strings.Index(reference, ":")
-	if i < 0 {
-		return reference, mediaType
-	}
-
-	// In case it is C:\
-	if i == 1 && len(reference) > 2 && reference[2] == '\\' && unicode.IsLetter(rune(reference[0])) {
-		i = strings.Index(reference[3:], ":")
-		if i < 0 {
-			return reference, mediaType
-		}
-		i += 3
-	}
-	return reference[:i], reference[i+1:]
-}
diff --git a/cmd/oras/file_unix.go b/cmd/oras/internal/fileref/unix.go
similarity index 59%
rename from cmd/oras/file_unix.go
rename to cmd/oras/internal/fileref/unix.go
index 03010dd..82494f0 100644
--- a/cmd/oras/file_unix.go
+++ b/cmd/oras/internal/fileref/unix.go
@@ -15,16 +15,23 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package main
+package fileref
 
-import "strings"
+import (
+	"fmt"
+	"strings"
+)
 
-// parseFileReference parses file reference on unix.
-func parseFileReference(reference string, mediaType string) (filePath, mediatype string) {
+// Parse parses file reference on unix.
+func Parse(reference string, defaultMediaType string) (filePath, mediaType string, err error) {
 	i := strings.LastIndex(reference, ":")
 	if i < 0 {
-		return reference, mediaType
+		filePath, mediaType = reference, defaultMediaType
+	} else {
+		filePath, mediaType = reference[:i], reference[i+1:]
 	}
-	return reference[:i], reference[i+1:]
-
+	if filePath == "" {
+		return "", "", fmt.Errorf("found empty file path in %q", reference)
+	}
+	return filePath, mediaType, nil
 }
diff --git a/cmd/oras/internal/fileref/windows.go b/cmd/oras/internal/fileref/windows.go
new file mode 100644
index 0000000..9ea0597
--- /dev/null
+++ b/cmd/oras/internal/fileref/windows.go
@@ -0,0 +1,46 @@
+//go:build windows
+
+/*
+Copyright The ORAS Authors.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package fileref
+
+import (
+	"fmt"
+	"strings"
+	"unicode"
+)
+
+// Parse parses file reference on windows.
+func Parse(reference string, defaultMediaType string) (filePath, mediaType string, err error) {
+	filePath, mediaType = doParse(reference, defaultMediaType)
+	if filePath == "" {
+		return "", "", fmt.Errorf("found empty file path in %q", reference)
+	}
+	if strings.ContainsAny(filePath, `<>:"|?*`) {
+		// Reference: https://learn.microsoft.com/windows/win32/fileio/naming-a-file#naming-conventions
+		return "", "", fmt.Errorf("reserved characters found in the file path: %s", filePath)
+	}
+	return filePath, mediaType, nil
+}
+
+func doParse(reference string, mediaType string) (filePath, mediatype string) {
+	i := strings.LastIndex(reference, ":")
+	if i < 0 || (i == 1 && len(reference) > 2 && unicode.IsLetter(rune(reference[0])) && reference[2] == '\\') {
+		// Relative file path with disk prefix is NOT supported, e.g. `c:file1`
+		return reference, mediaType
+	}
+	return reference[:i], reference[i+1:]
+}
diff --git a/cmd/oras/pull.go b/cmd/oras/pull.go
index a2c1433..c57cc6e 100644
--- a/cmd/oras/pull.go
+++ b/cmd/oras/pull.go
@@ -28,6 +28,7 @@ import (
 	"oras.land/oras-go/v2/content/file"
 	"oras.land/oras/cmd/oras/internal/display"
 	"oras.land/oras/cmd/oras/internal/errors"
+	"oras.land/oras/cmd/oras/internal/fileref"
 	"oras.land/oras/cmd/oras/internal/option"
 	"oras.land/oras/internal/graph"
 )
@@ -117,7 +118,13 @@ func runPull(opts pullOptions) error {
 	// Copy Options
 	copyOptions := oras.DefaultCopyOptions
 	copyOptions.Concurrency = opts.concurrency
-	configPath, configMediaType := parseFileReference(opts.ManifestConfigRef, "")
+	var configPath, configMediaType string
+	if opts.ManifestConfigRef != "" {
+		configPath, configMediaType, err = fileref.Parse(opts.ManifestConfigRef, "")
+		if err != nil {
+			return err
+		}
+	}
 	if targetPlatform != nil {
 		copyOptions.WithTargetPlatform(targetPlatform)
 	}
diff --git a/cmd/oras/push.go b/cmd/oras/push.go
index 0d87ffc..e43c28d 100644
--- a/cmd/oras/push.go
+++ b/cmd/oras/push.go
@@ -32,6 +32,7 @@ import (
 	"oras.land/oras-go/v2/registry/remote"
 	"oras.land/oras-go/v2/registry/remote/errcode"
 	"oras.land/oras/cmd/oras/internal/display"
+	"oras.land/oras/cmd/oras/internal/fileref"
 	"oras.land/oras/cmd/oras/internal/option"
 )
 
@@ -129,7 +130,10 @@ func runPush(opts pushOptions) error {
 	defer store.Close()
 	store.AllowPathTraversalOnWrite = opts.PathValidationDisabled
 	if opts.manifestConfigRef != "" {
-		path, cfgMediaType := parseFileReference(opts.manifestConfigRef, oras.MediaTypeUnknownConfig)
+		path, cfgMediaType, err := fileref.Parse(opts.manifestConfigRef, oras.MediaTypeUnknownConfig)
+		if err != nil {
+			return err
+		}
 		desc, err := store.Add(ctx, option.AnnotationConfig, cfgMediaType, path)
 		if err != nil {
 			return err


Test patch:
diff --git a/cmd/oras/internal/fileref/unix_test.go b/cmd/oras/internal/fileref/unix_test.go
new file mode 100644
index 0000000..5bf255c
--- /dev/null
+++ b/cmd/oras/internal/fileref/unix_test.go
@@ -0,0 +1,90 @@
+//go:build !windows
+
+/*
+Copyright The ORAS Authors.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package fileref
+
+import "testing"
+
+func Test_ParseFileReference(t *testing.T) {
+	type args struct {
+		reference string
+		mediaType string
+	}
+	tests := []struct {
+		name          string
+		args          args
+		wantFilePath  string
+		wantMediatype string
+	}{
+		{"file name and media type", args{"az:b", ""}, "az", "b"},
+		{"file name and empty media type", args{"az:", ""}, "az", ""},
+		{"file name and default media type", args{"az", "c"}, "az", "c"},
+		{"file name and media type, default type ignored", args{"az:b", "c"}, "az", "b"},
+		{"file name and empty media type, default type ignored", args{"az:", "c"}, "az", ""},
+		{"colon file name and media type", args{"az:b:c", "d"}, "az:b", "c"},
+		{"colon file name and empty media type", args{"az:b:", "c"}, "az:b", ""},
+		{"colon-prefix file name and media type", args{":az:b:c", "d"}, ":az:b", "c"},
+
+		{"pure colon file name and media type", args{"::a", "b"}, ":", "a"},
+		{"pure colon file name and empty media type", args{"::", "a"}, ":", ""},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotFilePath, gotMediatype, _ := Parse(tt.args.reference, tt.args.mediaType)
+			if gotFilePath != tt.wantFilePath {
+				t.Errorf("Parse() gotFilePath = %v, want %v", gotFilePath, tt.wantFilePath)
+			}
+			if gotMediatype != tt.wantMediatype {
+				t.Errorf("Parse() gotMediatype = %v, want %v", gotMediatype, tt.wantMediatype)
+			}
+		})
+	}
+}
+
+func TestParse(t *testing.T) {
+	type args struct {
+		reference string
+		mediaType string
+	}
+	tests := []struct {
+		name          string
+		args          args
+		wantFilePath  string
+		wantMediatype string
+		wantErr       bool
+	}{
+
+		{"no input", args{"", ""}, "", "", true},
+		{"empty file name and media type", args{":", ""}, "", "", true},
+		{"empty file name with media type", args{":a", "b"}, "", "", true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotFilePath, gotMediatype, err := Parse(tt.args.reference, tt.args.mediaType)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("Parse() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if gotFilePath != tt.wantFilePath {
+				t.Errorf("Parse() gotFilePath = %v, want %v", gotFilePath, tt.wantFilePath)
+			}
+			if gotMediatype != tt.wantMediatype {
+				t.Errorf("Parse() gotMediatype = %v, want %v", gotMediatype, tt.wantMediatype)
+			}
+		})
+	}
+}
diff --git a/cmd/oras/internal/fileref/windows_test.go b/cmd/oras/internal/fileref/windows_test.go
new file mode 100644
index 0000000..3cd60b2
--- /dev/null
+++ b/cmd/oras/internal/fileref/windows_test.go
@@ -0,0 +1,117 @@
+//go:build windows
+
+/*
+Copyright The ORAS Authors.
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package fileref
+
+import (
+	"testing"
+)
+
+func Test_doParse(t *testing.T) {
+	type args struct {
+		reference string
+		mediaType string
+	}
+	tests := []struct {
+		name          string
+		args          args
+		wantFilePath  string
+		wantMediatype string
+	}{
+		{"file name and media type", args{"az:b", ""}, "az", "b"},
+		{"file name and empty media type", args{"az:", ""}, "az", ""},
+		{"file name and default media type", args{"az", "c"}, "az", "c"},
+		{"file name and media type, default type ignored", args{"az:b", "c"}, "az", "b"},
+		{"file name and empty media type, default type ignored", args{"az:", "c"}, "az", ""},
+
+		{"empty file name and media type", args{":a", "b"}, "", "a"},
+		{"empty file name and empty media type", args{":", "a"}, "", ""},
+		{"empty name and default media type", args{"", "a"}, "", "a"},
+
+		{"colon file name and media type", args{"az:b:c", "d"}, "az:b", "c"},
+		{"colon file name and empty media type", args{"az:b:", "c"}, "az:b", ""},
+		{"colon-prefix file name and media type", args{":az:b:c", "d"}, ":az:b", "c"},
+
+		{"pure colon file name and media type", args{"::a", "b"}, ":", "a"},
+		{"pure colon file name and empty media type", args{"::", "a"}, ":", ""},
+
+		{"windows file name1 and default type", args{`a:\b`, "c"}, `a:\b`, "c"},
+		{"windows file name2 and default type", args{`z:b`, "c"}, `z`, "b"},
+		{"windows file name and media type", args{`a:\b:c`, "d"}, `a:\b`, "c"},
+		{"windows file name and empty media type", args{`a:\b:`, "c"}, `a:\b`, ""},
+		{"numeric file name and media type", args{`1:\a`, "b"}, `1`, `\a`},
+		{"non-windows file name and media type", args{`ab:\c`, ""}, `ab`, `\c`},
+		{"non-windows file name and media type, default type ignored", args{`1:\a`, "b"}, `1`, `\a`},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotFilePath, gotMediatype := doParse(tt.args.reference, tt.args.mediaType)
+			if gotFilePath != tt.wantFilePath {
+				t.Errorf("doParse() gotFilePath = %v, want %v", gotFilePath, tt.wantFilePath)
+			}
+			if gotMediatype != tt.wantMediatype {
+				t.Errorf("doParse() gotMediatype = %v, want %v", gotMediatype, tt.wantMediatype)
+			}
+		})
+	}
+}
+
+func TestParse(t *testing.T) {
+	type args struct {
+		reference string
+		mediaType string
+	}
+	tests := []struct {
+		name          string
+		args          args
+		wantFilePath  string
+		wantMediatype string
+		wantErr       bool
+	}{
+		{"no input", args{"", ""}, "", "", true},
+		{"empty file name", args{":", ""}, "", "", true},
+		{"reserved character1 in file name", args{"<", "a"}, "", "", true},
+		{"reserved character2 in file name", args{">", "a"}, "", "", true},
+		{"reserved character3 in file name", args{"*", "a"}, "", "", true},
+		{"reserved character4 in file name", args{`"`, "a"}, "", "", true},
+		{"reserved character5 in file name", args{"|", "a"}, "", "", true},
+		{"reserved character6 in file name", args{"?", "a"}, "", "", true},
+		{"empty file name, with media type", args{":", "a"}, "", "", true},
+		{"reserved character1 in file name, with media type", args{"<:", "a"}, "", "", true},
+		{"reserved character2 in file name, with media type", args{">:", "a"}, "", "", true},
+		{"reserved character3 in file name, with media type", args{"*:", "a"}, "", "", true},
+		{"reserved character4 in file name, with media type", args{`":`, "a"}, "", "", true},
+		{"reserved character5 in file name, with media type", args{"|:", "a"}, "", "", true},
+		{"reserved character6 in file name, with media type", args{"?:", "a"}, "", "", true},
+		{"reserved character7 in file name, with media type", args{"::", "a"}, "", "", true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotFilePath, gotMediatype, err := Parse(tt.args.reference, tt.args.mediaType)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("Parse() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if gotFilePath != tt.wantFilePath {
+				t.Errorf("Parse() gotFilePath = %v, want %v", gotFilePath, tt.wantFilePath)
+			}
+			if gotMediatype != tt.wantMediatype {
+				t.Errorf("Parse() gotMediatype = %v, want %v", gotMediatype, tt.wantMediatype)
+			}
+		})
+	}
+}