From a942ac7c59602abf126b192ddaa3737221d0fcf6 Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Fri, 15 Mar 2024 22:00:13 +0100
Subject: [PATCH] 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 <github@gone.nl>
---
 database.go       |  2 ++
 platforms.go      |  8 +++++---
 platforms_test.go | 45 +++++++++++++++++++++++++++++++++++++--------
 3 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/database.go b/database.go
index 2e26fd3..5cc0fd0 100644
--- a/database.go
+++ b/database.go
@@ -59,6 +59,8 @@ func isKnownArch(arch string) bool {
 	return false
 }
 
+// normalizeOS returns the normalized OS. If the os is empty, it returns
+// [runtime.GOOS].
 func normalizeOS(os string) string {
 	if os == "" {
 		return runtime.GOOS
diff --git a/platforms.go b/platforms.go
index 1bbbdb9..7931cf2 100644
--- a/platforms.go
+++ b/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
diff --git a/platforms_test.go b/platforms_test.go
index 8a26f5c..5727af3 100644
--- a/platforms_test.go
+++ b/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 {