[Java.Interop.Tools.Maven] Validate Artifact coordinates (#1479)

Context: https://github.com/dotnet/android (the `<AndroidMavenLibrary>`
MSBuild item relies on `Artifact`/`Artifact.TryParse` to validate Maven
coordinates supplied by users, but today the constructor blindly assigns
its fields and `TryParse` only checks for three colon-separated parts.
Empty strings, whitespace, and characters that are illegal per the Maven
coordinate spec all slip through.

Add structural validation to `Artifact`:

  * `groupId` and `artifactId` must match `[A-Za-z0-9_\-.]+` per
    https://maven.apache.org/pom.html#Maven_Coordinates.
  * `version` rejects whitespace, `:`, and path separators (`/`, `\`).
    Maven versions are otherwise permissive, so no further parsing.
  * The constructor throws `ArgumentNullException` / `ArgumentException`
    with the offending parameter and value.
  * `TryParse` returns `false` for any invalid input (and requires all
    three parts to be non-empty) without throwing.
  * `Parse` continues to throw `ArgumentException` with the existing
    message format.

The constructor still allows an empty `version` so that existing internal
callers - `Dependency.ToArtifact ()` and `Project.TryGetParentPomArtifact ()` -
can keep producing partial coordinates when a POM omits `<version>` and
inherits it from a parent. `TryParse`/`Parse` (the user-input path) still
require a non-empty version.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: jonathanpeppers

external/Java.Interop/src/Java.Interop.Tools.Maven/Models/Artifact.cs

@@ -18,6 +18,22 @@ public class Artifact
 
 	public Artifact (string groupId, string artifactId, string version)
 	{
+		if (groupId is null)
+			throw new ArgumentNullException (nameof (groupId));
+		if (artifactId is null)
+			throw new ArgumentNullException (nameof (artifactId));
+		if (version is null)
+			throw new ArgumentNullException (nameof (version));
+		if (!IsValidCoordinate (groupId))
+			throw new ArgumentException ($"Invalid Maven groupId: '{groupId}'", nameof (groupId));
+		if (!IsValidCoordinate (artifactId))
+			throw new ArgumentException ($"Invalid Maven artifactId: '{artifactId}'", nameof (artifactId));
+		// Allow empty version (callers may construct a partial coordinate when
+		// the version is to be inherited from a parent POM), but reject
+		// whitespace-only or otherwise malformed non-empty values.
+		if (version.Length > 0 && !IsValidVersion (version))
+			throw new ArgumentException ($"Invalid Maven version: '{version}'", nameof (version));
+
 		Id = artifactId;
 		GroupId = groupId;
 		Version = version;
@@ -31,19 +47,56 @@ public static Artifact Parse (string value)
 		throw new ArgumentException ($"Invalid artifact format: {value}");
 	}
 
-	public static bool TryParse (string value, [NotNullWhen (true)]out Artifact? artifact)
+	public static bool TryParse (string? value, [NotNullWhen (true)]out Artifact? artifact)
 	{
 		artifact = null;
 
-		var parts = value.Split (':');
+		if (value is null)
+			return false;
+
+		var parts = value.Split ([':'], 4);
 
 		if (parts.Length != 3)
 			return false;
 
+		// Parsed coordinates must have all three parts fully populated.
+		if (!IsValidCoordinate (parts [0]) || !IsValidCoordinate (parts [1]) || !IsValidVersion (parts [2]))
+			return false;
+
 		artifact = new Artifact (parts [0], parts [1], parts [2]);
 
 		return true;
 	}
 
+	// Per https://maven.apache.org/pom.html#Maven_Coordinates groupId/artifactId
+	// must match [A-Za-z0-9_\-.]+
+	static bool IsValidCoordinate (string value)
+	{
+		if (string.IsNullOrWhiteSpace (value))
+			return false;
+		foreach (var c in value) {
+			if (!((c >= 'A' && c <= 'Z') ||
+				(c >= 'a' && c <= 'z') ||
+				(c >= '0' && c <= '9') ||
+				c == '_' || c == '-' || c == '.'))
+				return false;
+		}
+		return true;
+	}
+
+	// Maven versions are permissive; reject only obviously broken values
+	// (empty/whitespace, embedded whitespace, path separators, or ':' which
+	// would break parsing).
+	static bool IsValidVersion (string value)
+	{
+		if (string.IsNullOrWhiteSpace (value))
+			return false;
+		foreach (var c in value) {
+			if (c == ':' || c == '/' || c == '\\' || char.IsWhiteSpace (c))
+				return false;
+		}
+		return true;
+	}
+
 	public override string ToString () => VersionedArtifactString;
 }

external/Java.Interop/tests/Java.Interop.Tools.Maven-Tests/ArtifactTests.cs

@@ -0,0 +1,125 @@
+using System;
+using Java.Interop.Tools.Maven.Models;
+
+namespace Java.Interop.Tools.Maven_Tests;
+
+public class ArtifactTests
+{
+	[TestCase ("com.google.guava:guava:31.1-jre", "com.google.guava", "guava", "31.1-jre")]
+	[TestCase ("androidx.core:core:1.9.0", "androidx.core", "core", "1.9.0")]
+	[TestCase ("a:b:1", "a", "b", "1")]
+	[TestCase ("group_1-x.y:artifact-id_2:1.0.0-SNAPSHOT", "group_1-x.y", "artifact-id_2", "1.0.0-SNAPSHOT")]
+	public void TryParse_Valid (string value, string groupId, string artifactId, string version)
+	{
+		Assert.IsTrue (Artifact.TryParse (value, out var artifact));
+		Assert.AreEqual (groupId, artifact!.GroupId);
+		Assert.AreEqual (artifactId, artifact.Id);
+		Assert.AreEqual (version, artifact.Version);
+		Assert.AreEqual ($"{groupId}:{artifactId}", artifact.ArtifactString);
+		Assert.AreEqual (value, artifact.VersionedArtifactString);
+		Assert.AreEqual (value, artifact.ToString ());
+	}
+
+	[TestCase ("com.google.guava:guava:31.1-jre", "com.google.guava", "guava", "31.1-jre")]
+	public void Parse_Valid (string value, string groupId, string artifactId, string version)
+	{
+		var artifact = Artifact.Parse (value);
+		Assert.AreEqual (groupId, artifact.GroupId);
+		Assert.AreEqual (artifactId, artifact.Id);
+		Assert.AreEqual (version, artifact.Version);
+	}
+
+	[TestCase ("")]
+	[TestCase ("foo")]
+	[TestCase ("foo:bar")]
+	[TestCase ("a:b:c:d")]
+	[TestCase ("::")]
+	[TestCase ("a::1")]
+	[TestCase (":b:1")]
+	[TestCase ("a:b:")]
+	[TestCase ("  :b:1")]
+	[TestCase ("a b:c:1")]
+	[TestCase ("a:b c:1")]
+	[TestCase ("a:b:1 0")]
+	[TestCase ("a/b:c:1")]
+	[TestCase ("a:b@c:1")]
+	[TestCase ("a:b!:1")]
+	[TestCase ("a:b:c:d:e:f:g")]
+	[TestCase ("../a:b:1")]
+	[TestCase ("a:../b:1")]
+	[TestCase ("a:b:../1")]
+	[TestCase ("a:b:1.0/../")]
+	[TestCase ("a/../b:c:1")]
+	[TestCase ("..\\a:b:1")]
+	[TestCase ("a:b:..\\1.0")]
+	public void TryParse_Invalid (string value)
+	{
+		Assert.IsFalse (Artifact.TryParse (value, out var artifact));
+		Assert.IsNull (artifact);
+	}
+
+	[Test]
+	public void TryParse_Null ()
+	{
+		Assert.IsFalse (Artifact.TryParse (null!, out var artifact));
+		Assert.IsNull (artifact);
+	}
+
+	[TestCase ("")]
+	[TestCase ("foo")]
+	[TestCase ("a:b c:1")]
+	public void Parse_Invalid_Throws (string value)
+	{
+		Assert.Throws<ArgumentException> (() => Artifact.Parse (value));
+	}
+
+	[Test]
+	public void Ctor_Valid ()
+	{
+		var artifact = new Artifact ("com.example", "lib", "1.0.0");
+		Assert.AreEqual ("com.example", artifact.GroupId);
+		Assert.AreEqual ("lib", artifact.Id);
+		Assert.AreEqual ("1.0.0", artifact.Version);
+	}
+
+	[TestCase (null, "lib", "1.0")]
+	[TestCase ("com.example", null, "1.0")]
+	[TestCase ("com.example", "lib", null)]
+	public void Ctor_Null_Throws (string? groupId, string? artifactId, string? version)
+	{
+		Assert.Throws<ArgumentNullException> (() => new Artifact (groupId!, artifactId!, version!));
+	}
+
+	[TestCase ("", "lib", "1.0")]
+	[TestCase ("   ", "lib", "1.0")]
+	[TestCase ("a b", "lib", "1.0")]
+	[TestCase ("a/b", "lib", "1.0")]
+	[TestCase ("a@b", "lib", "1.0")]
+	[TestCase ("com.example", "", "1.0")]
+	[TestCase ("com.example", "   ", "1.0")]
+	[TestCase ("com.example", "li b", "1.0")]
+	[TestCase ("com.example", "lib!", "1.0")]
+	[TestCase ("com.example", "lib", "   ")]
+	[TestCase ("com.example", "lib", "1 0")]
+	[TestCase ("com.example", "lib", "1:0")]
+	[TestCase ("../com.example", "lib", "1.0")]
+	[TestCase ("com.example", "../lib", "1.0")]
+	[TestCase ("com.example", "lib", "../1.0")]
+	[TestCase ("com.example", "lib", "1.0/../")]
+	[TestCase ("com.example", "lib", "..\\1.0")]
+	[TestCase ("com/example", "lib", "1.0")]
+	public void Ctor_Invalid_Throws (string groupId, string artifactId, string version)
+	{
+		Assert.Throws<ArgumentException> (() => new Artifact (groupId, artifactId, version));
+	}
+
+	[Test]
+	public void Ctor_AllowsEmptyVersion ()
+	{
+		// Empty version is permitted in the constructor to support partial
+		// coordinates produced from POM XML where <version> is omitted and
+		// inherited from a parent POM.
+		var artifact = new Artifact ("com.example", "lib", "");
+		Assert.AreEqual ("", artifact.Version);
+	}
+}