From b2568a5431160f4e0867115bf3cc1e966f26843d Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Mon, 22 Jun 2026 11:14:08 -0500 Subject: [PATCH 1/3] [Java.Interop.Tools.Maven] Validate Artifact coordinates Context: https://github.com/dotnet/android (the `` 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 `` 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> --- .../Models/Artifact.cs | 53 ++++++++ .../ArtifactTests.cs | 124 ++++++++++++++++++ 2 files changed, 177 insertions(+) create mode 100644 tests/Java.Interop.Tools.Maven-Tests/ArtifactTests.cs diff --git a/src/Java.Interop.Tools.Maven/Models/Artifact.cs b/src/Java.Interop.Tools.Maven/Models/Artifact.cs index 0e16d3cfd..0a6c18fb3 100644 --- a/src/Java.Interop.Tools.Maven/Models/Artifact.cs +++ b/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; @@ -35,15 +51,52 @@ public static bool TryParse (string value, [NotNullWhen (true)]out Artifact? art { artifact = null; + if (value is null) + return false; + var parts = value.Split (':'); 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; } diff --git a/tests/Java.Interop.Tools.Maven-Tests/ArtifactTests.cs b/tests/Java.Interop.Tools.Maven-Tests/ArtifactTests.cs new file mode 100644 index 000000000..6ecd338e5 --- /dev/null +++ b/tests/Java.Interop.Tools.Maven-Tests/ArtifactTests.cs @@ -0,0 +1,124 @@ +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: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 (() => 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 (() => 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 (() => 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 is omitted and + // inherited from a parent POM. + var artifact = new Artifact ("com.example", "lib", ""); + Assert.AreEqual ("", artifact.Version); + } +} From 9caec241dfa2ed718f9b8caf3c3476ad8c467976 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Mon, 22 Jun 2026 11:21:09 -0500 Subject: [PATCH 2/3] Address PR feedback: nullable TryParse param + bounded Split Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Java.Interop.Tools.Maven/Models/Artifact.cs | 7 +++++-- tests/Java.Interop.Tools.Maven-Tests/ArtifactTests.cs | 1 + 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Java.Interop.Tools.Maven/Models/Artifact.cs b/src/Java.Interop.Tools.Maven/Models/Artifact.cs index 0a6c18fb3..a90710e9a 100644 --- a/src/Java.Interop.Tools.Maven/Models/Artifact.cs +++ b/src/Java.Interop.Tools.Maven/Models/Artifact.cs @@ -47,14 +47,17 @@ 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; if (value is null) return false; - var parts = value.Split (':'); + // Limit the split to 4 so adversarial input with many ':' characters + // can't trigger an unbounded allocation; anything with extras is + // rejected by the Length check below. + var parts = value.Split (new [] { ':' }, 4); if (parts.Length != 3) return false; diff --git a/tests/Java.Interop.Tools.Maven-Tests/ArtifactTests.cs b/tests/Java.Interop.Tools.Maven-Tests/ArtifactTests.cs index 6ecd338e5..186b25650 100644 --- a/tests/Java.Interop.Tools.Maven-Tests/ArtifactTests.cs +++ b/tests/Java.Interop.Tools.Maven-Tests/ArtifactTests.cs @@ -44,6 +44,7 @@ public void Parse_Valid (string value, string groupId, string artifactId, string [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")] From bc40450c933c79e0ad8844ea7cf8be7c64769b9b Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Mon, 22 Jun 2026 13:14:05 -0500 Subject: [PATCH 3/3] Address review feedback: collection expression, drop comment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Java.Interop.Tools.Maven/Models/Artifact.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Java.Interop.Tools.Maven/Models/Artifact.cs b/src/Java.Interop.Tools.Maven/Models/Artifact.cs index a90710e9a..9c10518a1 100644 --- a/src/Java.Interop.Tools.Maven/Models/Artifact.cs +++ b/src/Java.Interop.Tools.Maven/Models/Artifact.cs @@ -54,10 +54,7 @@ public static bool TryParse (string? value, [NotNullWhen (true)]out Artifact? ar if (value is null) return false; - // Limit the split to 4 so adversarial input with many ':' characters - // can't trigger an unbounded allocation; anything with extras is - // rejected by the Length check below. - var parts = value.Split (new [] { ':' }, 4); + var parts = value.Split ([':'], 4); if (parts.Length != 3) return false;