Merge commit from fork

* Harden Windows VCS URI fragments against command injection

- Use Process(argv) for git/hg/svn without cmd /c on Windows
- Add VcsUriFragment.validate for fragments in clone/checkout/update
- Add tests

* Allowlist-based approach to VCS string sanitization

main/src/main/scala/sbt/Resolvers.scala

@@ -21,10 +21,9 @@ import java.util.Locale
 
 import scala.sys.process.{ BasicIO, Process }
 import scala.util.control.NonFatal
-import sbt.internal.util.Util
 import sbt.internal.{ BuildDependencies, LoadedBuild, LoadedBuildUnit }
 import sbt.util.Logger
-import sbt.internal.RetrieveUnit
+import sbt.internal.{ RetrieveUnit, VcsUriFragment }
 
 object Resolvers {
 
@@ -67,6 +66,7 @@ object Resolvers {
 
     if (uri.hasFragment) {
       val revision = uri.getFragment
+      VcsUriFragment.validate(revision)
       Some { () =>
         creates(localCopy) {
           run(cwd = None, log = None, "svn", "checkout", "-q", "-r", revision, from, to)
@@ -87,6 +87,7 @@ object Resolvers {
 
     if (uri.hasFragment) {
       val branch = uri.getFragment
+      VcsUriFragment.validate(branch)
       Some { () =>
         creates(localCopy) {
           run(cwd = None, log = None, "hg", "clone", "-q", from, localCopy.getAbsolutePath)
@@ -108,6 +109,7 @@ object Resolvers {
 
     if (uri.hasFragment) {
       val branch = uri.getFragment
+      VcsUriFragment.validate(branch)
       Some { () =>
         creates(localCopy) {
           run(cwd = None, log = None, "git", "clone", from, localCopy.getAbsolutePath)
@@ -132,11 +134,7 @@ object Resolvers {
   }
 
   private def run(cwd: Option[File], log: Option[Logger], command: String*): Unit = {
-    val process = Process(
-      if (Util.isNonCygwinWindows) "cmd" +: "/c" +: command
-      else command,
-      cwd
-    )
+    val process = Process(command, cwd)
     val result = (log match {
       case Some(log) =>
         val io = BasicIO(false, log).withInput(_.close())
@@ -295,7 +293,11 @@ object Resolvers {
         false
       } else {
         val headBefore = captureOutput(Some(localCopy), "git", "rev-parse", "HEAD")
-        val ref = if (fromURI(uri).hasFragment) uri.getFragment else "HEAD"
+        val ref = if (fromURI(uri).hasFragment) {
+          val f = uri.getFragment
+          VcsUriFragment.validate(f)
+          f
+        } else "HEAD"
         run(Some(localCopy), Some(log), "git", "fetch", "origin", ref)
         run(Some(localCopy), Some(log), "git", "reset", "--hard", "FETCH_HEAD")
         markUpdated(localCopy)
@@ -310,16 +312,14 @@ object Resolvers {
     }
 
   private def captureOutput(cwd: Option[File], command: String*): String =
-    Process(
-      if (Util.isNonCygwinWindows) "cmd" +: "/c" +: command else command,
-      cwd
-    ).!!.trim
+    Process(command, cwd).!!.trim
 
   private def updateMercurial(localCopy: File, uri: URI, log: Logger): Boolean =
     try {
       val idBefore = captureOutput(Some(localCopy), "hg", "id", "-i")
       if (fromURI(uri).hasFragment) {
         val branch = uri.getFragment
+        VcsUriFragment.validate(branch)
         run(Some(localCopy), Some(log), "hg", "pull")
         run(Some(localCopy), Some(log), "hg", "update", branch)
       } else {
@@ -340,6 +340,7 @@ object Resolvers {
       val revBefore = captureOutput(Some(localCopy), "svn", "info", "--show-item", "revision")
       if (fromURI(uri).hasFragment) {
         val revision = uri.getFragment
+        VcsUriFragment.validate(revision)
         run(Some(localCopy), Some(log), "svn", "update", "-q", "-r", revision)
       } else {
         run(Some(localCopy), Some(log), "svn", "update", "-q")

main/src/main/scala/sbt/internal/VcsUriFragment.scala

@@ -0,0 +1,32 @@
+/*
+ * sbt
+ * Copyright 2023, Scala center
+ * Copyright 2011 - 2022, Lightbend, Inc.
+ * Copyright 2008 - 2010, Mark Harrah
+ * Licensed under Apache License 2.0 (see LICENSE)
+ */
+
+package sbt
+package internal
+
+private[sbt] object VcsUriFragment {
+
+  def validate(fragment: String): Unit = {
+    if (fragment == null)
+      throw new IllegalArgumentException("VCS URI fragment must not be null")
+    if (fragment.isEmpty)
+      throw new IllegalArgumentException("VCS URI fragment must not be empty")
+    fragment.foreach { c =>
+      if (!isAllowed(c))
+        throw new IllegalArgumentException(
+          "Invalid character in VCS URI fragment (only ASCII letters, digits, and - _ . / + are allowed)"
+        )
+    }
+  }
+
+  private def isAllowed(c: Char): Boolean =
+    (c >= 'a' && c <= 'z') ||
+      (c >= 'A' && c <= 'Z') ||
+      (c >= '0' && c <= '9') ||
+      c == '-' || c == '_' || c == '.' || c == '/' || c == '+'
+}

main/src/test/scala/sbt/internal/ResolversVcsSecurityTest.scala

@@ -0,0 +1,85 @@
+/*
+ * sbt
+ * Copyright 2023, Scala center
+ * Copyright 2011 - 2022, Lightbend, Inc.
+ * Copyright 2008 - 2010, Mark Harrah
+ * Licensed under Apache License 2.0 (see LICENSE)
+ */
+
+package sbt
+package internal
+
+import hedgehog.*
+import hedgehog.runner.*
+
+import java.net.URI
+import scala.jdk.CollectionConverters.*
+
+import _root_.sbt.Resolvers
+import _root_.sbt.io.IO
+import _root_.sbt.internal.BuildLoader.ResolveInfo
+
+object ResolversVcsSecurityTest extends Properties:
+
+  override def tests: List[Test] = List(
+    example(
+      "git resolver rejects fragment containing & before running VCS",
+      testResolverRejects(Resolvers.git, vcsUri("git", "/repo.git", "main&evil"))
+    ),
+    example(
+      "git resolver rejects fragment containing |",
+      testResolverRejects(Resolvers.git, vcsUri("git", "/repo.git", "main|evil"))
+    ),
+    example(
+      "git resolver rejects fragment containing ;",
+      testResolverRejects(Resolvers.git, vcsUri("git", "/repo.git", "main;evil"))
+    ),
+    example(
+      "mercurial resolver rejects fragment containing & before running VCS",
+      testResolverRejects(Resolvers.mercurial, vcsUri("hg", "/repo", "main&evil"))
+    ),
+    example(
+      "subversion resolver rejects fragment containing & before running VCS",
+      testResolverRejects(Resolvers.subversion, vcsUri("svn", "/repo", "main&evil"))
+    ),
+    example(
+      "git resolver accepts safe branch fragment and returns Some",
+      testResolverAccepts(Resolvers.git, vcsUri("git", "/repo.git", "develop"))
+    ),
+    example(
+      "ProcessBuilder passes VCS ref as a single argv element (no shell parsing)",
+      testProcessBuilderKeepsMetacharactersInSingleArgument
+    ),
+  )
+
+  private def vcsUri(scheme: String, path: String, fragment: String): URI =
+    new URI(scheme, null, "example.com", -1, path, null, fragment)
+
+  private def testResolverRejects(resolver: Resolvers.Resolver, uri: URI): Result =
+    val staging = IO.createTemporaryDirectory
+    try
+      val info = new ResolveInfo(uri, staging, null, null)
+      try
+        resolver(info)
+        Result.failure.log(s"expected IllegalArgumentException for $uri")
+      catch case _: IllegalArgumentException => Result.success
+    finally IO.delete(staging)
+
+  private def testResolverAccepts(resolver: Resolvers.Resolver, uri: URI): Result =
+    val staging = IO.createTemporaryDirectory
+    try
+      val info = new ResolveInfo(uri, staging, null, null)
+      try
+        resolver(info) match
+          case Some(_) => Result.success
+          case None    => Result.failure.log(s"expected Some for $uri")
+      catch
+        case e: IllegalArgumentException =>
+          Result.failure.log(s"unexpected IllegalArgumentException for $uri: ${e.getMessage}")
+    finally IO.delete(staging)
+
+  private def testProcessBuilderKeepsMetacharactersInSingleArgument: Result =
+    val argv = new ProcessBuilder("git", "fetch", "origin", "topic&injection").command().asScala.toList
+    Result.assert(argv == List("git", "fetch", "origin", "topic&injection"))
+
+end ResolversVcsSecurityTest

main/src/test/scala/sbt/internal/VcsUriFragmentTest.scala

@@ -0,0 +1,74 @@
+/*
+ * sbt
+ * Copyright 2023, Scala center
+ * Copyright 2011 - 2022, Lightbend, Inc.
+ * Copyright 2008 - 2010, Mark Harrah
+ * Licensed under Apache License 2.0 (see LICENSE)
+ */
+
+package sbt
+package internal
+
+import hedgehog.*
+import hedgehog.runner.*
+
+object VcsUriFragmentTest extends Properties:
+  override def tests: List[Test] = List(
+    example("accepts typical branch and tag names", testAcceptsSafe),
+    example("accepts hex commit id fragment", testAcceptsHexSha),
+    example("rejects empty fragment", testRejectsEmpty),
+    example("rejects ampersand", testRejectsAmpersand),
+    example("rejects pipe", testRejectsPipe),
+    example("rejects semicolon", testRejectsSemicolon),
+    example("rejects space", testRejectsSpace),
+    example("rejects percent", testRejectsPercent),
+    example("rejects greater-than", testRejectsGreaterThan),
+    example("rejects newline", testRejectsNewline),
+    example("rejects DEL", testRejectsDel),
+  )
+
+  def testAcceptsSafe: Result =
+    VcsUriFragment.validate("develop")
+    VcsUriFragment.validate("v1.2.3")
+    VcsUriFragment.validate("feature/foo-bar")
+    VcsUriFragment.validate("release/1.0.0+build")
+    Result.success
+
+  def testAcceptsHexSha: Result =
+    VcsUriFragment.validate("abc123def4567890abcdef1234567890abcdef12")
+    Result.success
+
+  def testRejectsEmpty: Result =
+    interceptIllegal("")
+
+  def testRejectsAmpersand: Result =
+    interceptIllegal("a&b")
+
+  def testRejectsPipe: Result =
+    interceptIllegal("a|b")
+
+  def testRejectsSemicolon: Result =
+    interceptIllegal("a;b")
+
+  def testRejectsSpace: Result =
+    interceptIllegal("a b")
+
+  def testRejectsPercent: Result =
+    interceptIllegal("a%20b")
+
+  def testRejectsGreaterThan: Result =
+    interceptIllegal("a>b")
+
+  def testRejectsNewline: Result =
+    interceptIllegal("a\nb")
+
+  def testRejectsDel: Result =
+    interceptIllegal("a\u007fb")
+
+  private def interceptIllegal(s: String): Result =
+    try
+      VcsUriFragment.validate(s)
+      Result.failure.log(s"expected failure for ${s.map(_.toInt).mkString(",")}")
+    catch case _: IllegalArgumentException => Result.success
+
+end VcsUriFragmentTest