Review suggestions

- Split regex for first (os and version) and further components (arch, variant)
- Update regular expression to preserve the existing set of accepted characters
  for the OS part of the first element (avoid accepting ".", ")", "(").
- Make regular expression for osVersion more strict, and only accept dot-separated
  numbers.
- Add capture groups to the osAndVersion regular expression so that the result
  can be consumed directly, without having to split after validating.
- Now that we already separate OS asnd OSVersion ahead; simplify normalizeOS
  to only normalize the OS.
- Remove the OSAndVersionFormat variable, and inline it.
- Update Parse() to prevent un-bounded string splitting.

Signed-off-by: Sebastiaan van Stijn <[email protected]>

database.go

@@ -59,27 +59,8 @@ func isKnownArch(arch string) bool {
 	return false
 }
 
-// normalizeOSAndVersion splits the provided platform specifier OS segment
-// into an OS and OSVersion.
-// The expected format is `<OS>[(<OSVersion>)]`, e.g., `windows(10.0.17763)`.
-// If `<os>` is not provided, the current host OS is returned as the first value.
-// If optional `(<OSVersion>)` is not provided, an empty string is returned as the
-// second value.
-func normalizeOSAndVersion(OSAndVersion string) (OS string, OSVersion string) {
-	if OSAndVersion == "" {
-		return runtime.GOOS, ""
-	}
-
-	parts := osVersionRe.Split(OSAndVersion, -1)
-	OS = normalizeOS(parts[0])
-	OSVersion = ""
-	if len(parts) > 1 && parts[1] != "" {
-		OSVersion = parts[1]
-	}
-
-	return OS, OSVersion
-}
-
+// normalizeOS will return the normalized OS. If the os is empty, it
+// returns [runtime.GOOS].
 func normalizeOS(os string) string {
 	if os == "" {
 		return runtime.GOOS

platforms.go

@@ -65,9 +65,11 @@
 // While the OCI platform specifications provide a tool for components to
 // specify structured information, user input typically doesn't need the full
 // context and much can be inferred. To solve this problem, we introduced
-// "specifiers". A specifier has the format
-// `<os>|<arch>|<os>/<arch>[/<variant>]`.  The user can provide either the
-// operating system or the architecture or both.
+// "specifiers". A specifier has the format:
+//
+//	<os>[(<osVersion>)]|<arch>|<os>/<arch>[/<variant>]
+//
+// The user can provide either the operating system or the architecture or both.
 //
 // An example of a common specifier is `linux/amd64`. If the host has a default
 // of runtime that matches this, the user can simply provide the component that
@@ -122,8 +124,7 @@ import (
 
 var (
 	specifierRe    = regexp.MustCompile(`^[A-Za-z0-9_-]+$`)
-	osAndVersionRe = regexp.MustCompile(`^([A-Za-z0-9_-]+)\(?([A-Za-z0-9_.-]*)\)?$`)
-	osVersionRe    = regexp.MustCompile(`[()]`)
+	osAndVersionRe = regexp.MustCompile(`^([A-Za-z0-9_-]+)(?:\(([A-Za-z0-9_.-]*)\))?$`)
 )
 
 const osAndVersionFormat = "%s(%s)"
@@ -200,11 +201,12 @@ func Parse(specifier string) (specs.Platform, error) {
 	for i, part := range parts {
 		if i == 0 {
 			// First element is <os>[(<OSVersion>)]
-			if !osAndVersionRe.MatchString(part) {
+			osVer := osAndVersionRe.FindStringSubmatch(part)
+			if osVer == nil {
 				return specs.Platform{}, fmt.Errorf("%q is an invalid OS component of %q: OSAndVersion specifier component must match %q: %w", part, specifier, osAndVersionRe.String(), errInvalidArgument)
 			}
-
-			p.OS, p.OSVersion = normalizeOSAndVersion(part)
+			p.OS = normalizeOS(osVer[1])
+			p.OSVersion = osVer[2]
 		} else {
 			if !specifierRe.MatchString(part) {
 				return specs.Platform{}, fmt.Errorf("%q is an invalid component of %q: platform specifier component must match %q: %w", part, specifier, specifierRe.String(), errInvalidArgument)

platforms_test.go

@@ -343,6 +343,26 @@ func TestParseSelector(t *testing.T) {
 			formatted:   path.Join("windows(10.0.17763)", defaultArch, defaultVariant),
 			useV2Format: true,
 		},
+		{
+			input: "windows(10.0.17763)/amd64",
+			expected: specs.Platform{
+				OS:           "windows",
+				OSVersion:    "10.0.17763",
+				Architecture: "amd64",
+			},
+			formatted:   "windows(10.0.17763)/amd64",
+			useV2Format: true,
+		},
+		{
+			input: "macos(Abcd.Efgh.123-4)/aarch64",
+			expected: specs.Platform{
+				OS:           "darwin",
+				OSVersion:    "Abcd.Efgh.123-4",
+				Architecture: "arm64",
+			},
+			formatted:   "darwin(Abcd.Efgh.123-4)/arm64",
+			useV2Format: true,
+		},
 	} {
 		t.Run(testcase.input, func(t *testing.T) {
 			if testcase.skip {
@@ -370,10 +390,10 @@ func TestParseSelector(t *testing.T) {
 			}
 
 			formatted := ""
-			if testcase.useV2Format == false {
-				formatted = Format(p)
-			} else {
+			if testcase.useV2Format {
 				formatted = FormatAll(p)
+			} else {
+				formatted = Format(p)
 			}
 			if formatted != testcase.formatted {
 				t.Fatalf("unexpected format: %q != %q", formatted, testcase.formatted)
@@ -385,14 +405,14 @@ func TestParseSelector(t *testing.T) {
 				t.Fatalf("error parsing formatted output: %v", err)
 			}
 
-			if testcase.useV2Format == false {
-				if Format(reparsed) != formatted {
-					t.Fatalf("normalized output did not survive the round trip: %v != %v", Format(reparsed), formatted)
-				}
-			} else {
+			if testcase.useV2Format {
 				if FormatAll(reparsed) != formatted {
 					t.Fatalf("normalized output did not survive the round trip: %v != %v", FormatAll(reparsed), formatted)
 				}
+			} else {
+				if Format(reparsed) != formatted {
+					t.Fatalf("normalized output did not survive the round trip: %v != %v", Format(reparsed), formatted)
+				}
 			}
 		})
 	}
@@ -420,6 +440,15 @@ func TestParseSelectorInvalid(t *testing.T) {
 		{
 			input: "linux/arm/foo/bar", // too many components
 		},
+		{
+			input: "amd64/windows(10.0.17763)/foo", // only first element accepts os[(osVersion)]
+		},
+		{
+			input: "linux)()---()..../arm/foo",
+		},
+		{
+			input: "../arm/foo",
+		},
 	} {
 		t.Run(testcase.input, func(t *testing.T) {
 			if _, err := Parse(testcase.input); err == nil {