Repository: scala-steward-org/scala-steward
Pull request #1703 (2020-10-30 21:19:16)

Allow passing environment variables to execSandboxed
This allows to pass extra environment variables to `execSandboxed` in
preparation for a fix for #1698. It also changes how environment
variables are used if firejail is enabled. All environment variables
(those from the config and those passed to `execSandboxed`) are
now passed via the `--env` parameter of the firejail command instead of
injecting them into the environment. This has been suggested earlier in
https://github.com/scala-steward-org/scala-steward/pull/337#discussion_r264052558 by @mwz.


Patch:
diff --git a/build.sbt b/build.sbt
index d22bff96..77295f01 100644
--- a/build.sbt
+++ b/build.sbt
@@ -265,7 +265,6 @@ moduleRootPkg := rootPkg
 lazy val runSteward = taskKey[Unit]("")
 runSteward := Def.taskDyn {
   val home = System.getenv("HOME")
-  val myJavaHome = System.getenv("JAVA_HOME")
   val projectDir = (LocalRootProject / baseDirectory).value
   val args = Seq(
     Seq("--workspace", s"$projectDir/workspace"),
@@ -278,10 +277,9 @@ runSteward := Def.taskDyn {
     Seq("--whitelist", s"$home/.cache/JNA"),
     Seq("--whitelist", s"$home/.cache/mill"),
     Seq("--whitelist", s"$home/.ivy2"),
+    Seq("--whitelist", s"$home/.m2"),
     Seq("--whitelist", s"$home/.mill"),
-    Seq("--whitelist", s"$home/.sbt"),
-    Seq("--whitelist", myJavaHome),
-    Seq("--read-only", myJavaHome)
+    Seq("--whitelist", s"$home/.sbt")
   ).flatten.mkString(" ", " ", "")
   (core.jvm / Compile / run).toTask(args)
 }.value
diff --git a/modules/core/src/main/scala/org/scalasteward/core/io/ProcessAlg.scala b/modules/core/src/main/scala/org/scalasteward/core/io/ProcessAlg.scala
index 5168370b..7ea98bd3 100644
--- a/modules/core/src/main/scala/org/scalasteward/core/io/ProcessAlg.scala
+++ b/modules/core/src/main/scala/org/scalasteward/core/io/ProcessAlg.scala
@@ -20,53 +20,70 @@ import better.files.File
 import cats.effect.{Blocker, Concurrent, ContextShift, Timer}
 import cats.syntax.all._
 import io.chrisdavenport.log4cats.Logger
-import org.scalasteward.core.application.Config.{ProcessCfg, SandboxCfg}
+import org.scalasteward.core.application.Config.ProcessCfg
+import org.scalasteward.core.io.process.Args
 import org.scalasteward.core.util.Nel
 
 trait ProcessAlg[F[_]] {
-  def exec(command: Nel[String], cwd: File, extraEnv: (String, String)*): F[List[String]]
+  def exec(
+      command: Nel[String],
+      workingDirectory: File,
+      extraEnv: (String, String)*
+  ): F[List[String]]
 
-  def execSandboxed(command: Nel[String], cwd: File): F[List[String]]
+  def execSandboxed(
+      command: Nel[String],
+      workingDirectory: File,
+      extraEnv: (String, String)*
+  ): F[List[String]] =
+    exec(command, workingDirectory, extraEnv: _*)
 }
 
 object ProcessAlg {
-  abstract class UsingFirejail[F[_]](config: SandboxCfg) extends ProcessAlg[F] {
-    override def execSandboxed(command: Nel[String], cwd: File): F[List[String]] =
-      if (config.disableSandbox)
-        exec(command, cwd)
-      else {
-        val whitelisted = (cwd.pathAsString :: config.whitelistedDirectories)
-          .map(dir => s"--whitelist=$dir")
-        val readOnly = config.readOnlyDirectories
-          .map(dir => s"--read-only=$dir")
-        exec(Nel("firejail", whitelisted ++ readOnly) ::: command, cwd)
-      }
+  private class NoSandbox[F[_]](config: ProcessCfg)(execImpl: Args => F[List[String]])
+      extends ProcessAlg[F] {
+    val configEnv: List[(String, String)] = config.envVars.map(v => (v.name, v.value))
+
+    override def exec(
+        command: Nel[String],
+        workingDirectory: File,
+        extraEnv: (String, String)*
+    ): F[List[String]] =
+      execImpl(Args(command, Some(workingDirectory), extraEnv.toList ++ configEnv))
+  }
+
+  private class WithFirejail[F[_]](config: ProcessCfg)(execImpl: Args => F[List[String]])
+      extends NoSandbox[F](config)(execImpl) {
+    override def execSandboxed(
+        command: Nel[String],
+        workingDirectory: File,
+        extraEnv: (String, String)*
+    ): F[List[String]] = {
+      val whitelisted = (workingDirectory.toString :: config.sandboxCfg.whitelistedDirectories)
+        .map(dir => s"--whitelist=$dir")
+      val readOnly = config.sandboxCfg.readOnlyDirectories
+        .map(dir => s"--read-only=$dir")
+      val envVars = (extraEnv ++ configEnv)
+        .map { case (k, v) => s"--env=$k=$v" }
+      val firejail = Nel("firejail", whitelisted ++ readOnly ++ envVars) ::: command
+      execImpl(Args(firejail, Some(workingDirectory), clearEnv = true))
+    }
   }
 
+  def fromExecImpl[F[_]](config: ProcessCfg)(execImpl: Args => F[List[String]]): ProcessAlg[F] =
+    if (config.sandboxCfg.disableSandbox)
+      new NoSandbox[F](config)(execImpl)
+    else
+      new WithFirejail[F](config)(execImpl)
+
   def create[F[_]](blocker: Blocker, config: ProcessCfg)(implicit
       contextShift: ContextShift[F],
       logger: Logger[F],
       timer: Timer[F],
       F: Concurrent[F]
-  ): ProcessAlg[F] = {
-    val configEnv = config.envVars.map(v => (v.name, v.value))
-    new UsingFirejail[F](config.sandboxCfg) {
-      override def exec(
-          command: Nel[String],
-          cwd: File,
-          extraEnv: (String, String)*
-      ): F[List[String]] = {
-        val envVars = extraEnv.toList ::: configEnv
-        logger.debug(s"Execute ${process.showCmd(command, envVars)}") >>
-          process.slurp[F](
-            command,
-            Some(cwd.toJava),
-            envVars,
-            config.processTimeout,
-            logger.trace(_),
-            blocker
-          )
-      }
+  ): ProcessAlg[F] =
+    fromExecImpl(config) { args =>
+      logger.debug(s"Execute ${process.showCmd(args)}") >>
+        process.slurp[F](args, config.processTimeout, logger.trace(_), blocker)
     }
-  }
 }
diff --git a/modules/core/src/main/scala/org/scalasteward/core/io/process.scala b/modules/core/src/main/scala/org/scalasteward/core/io/process.scala
index 038b09dd..689eabda 100644
--- a/modules/core/src/main/scala/org/scalasteward/core/io/process.scala
+++ b/modules/core/src/main/scala/org/scalasteward/core/io/process.scala
@@ -16,25 +16,31 @@
 
 package org.scalasteward.core.io
 
+import better.files.File
 import cats.effect._
 import cats.syntax.all._
 import fs2.Stream
-import java.io.{File, IOException, InputStream}
+import java.io.{IOException, InputStream}
 import org.scalasteward.core.util._
 import scala.collection.mutable.ListBuffer
 import scala.concurrent.TimeoutException
 import scala.concurrent.duration.FiniteDuration
 
 object process {
+  final case class Args(
+      command: Nel[String],
+      workingDirectory: Option[File] = None,
+      extraEnv: List[(String, String)] = Nil,
+      clearEnv: Boolean = false
+  )
+
   def slurp[F[_]](
-      cmd: Nel[String],
-      cwd: Option[File],
-      extraEnv: List[(String, String)],
+      args: Args,
       timeout: FiniteDuration,
       log: String => F[Unit],
       blocker: Blocker
   )(implicit contextShift: ContextShift[F], timer: Timer[F], F: Concurrent[F]): F[List[String]] =
-    createProcess(cmd, cwd, extraEnv).flatMap { process =>
+    createProcess(args).flatMap { process =>
       F.delay(new ListBuffer[String]).flatMap { buffer =>
         val readOut = {
           val out = readInputStream[F](process.getInputStream, blocker)
@@ -44,13 +50,13 @@ object process {
         val result = readOut >> F.delay(process.waitFor()) >>= { exitValue =>
           if (exitValue === 0) F.pure(buffer.toList)
           else {
-            val msg = s"'${showCmd(cmd, extraEnv)}' exited with code $exitValue"
+            val msg = s"'${showCmd(args)}' exited with code $exitValue"
             F.raiseError[List[String]](new IOException(makeMessage(msg, buffer.toList)))
           }
         }
 
         val fallback = F.delay(process.destroyForcibly()) >> {
-          val msg = s"'${showCmd(cmd, extraEnv)}' timed out after ${timeout.toString}"
+          val msg = s"'${showCmd(args)}' timed out after ${timeout.toString}"
           F.raiseError[List[String]](new TimeoutException(makeMessage(msg, buffer.toList)))
         }
 
@@ -58,19 +64,16 @@ object process {
       }
     }
 
-  def showCmd(cmd: Nel[String], extraEnv: List[(String, String)]): String =
-    (extraEnv.map { case (k, v) => s"$k=$v" } ++ cmd.toList).mkString_(" ")
+  def showCmd(args: Args): String =
+    (args.extraEnv.map { case (k, v) => s"$k=$v" } ++ args.command.toList).mkString_(" ")
 
-  private def createProcess[F[_]](
-      cmd: Nel[String],
-      cwd: Option[File],
-      extraEnv: List[(String, String)]
-  )(implicit F: Sync[F]): F[Process] =
+  private def createProcess[F[_]](args: Args)(implicit F: Sync[F]): F[Process] =
     F.delay {
-      val pb = new ProcessBuilder(cmd.toList: _*)
+      val pb = new ProcessBuilder(args.command.toList: _*)
+      args.workingDirectory.foreach(file => pb.directory(file.toJava))
       val env = pb.environment()
-      cwd.foreach(pb.directory)
-      extraEnv.foreach { case (key, value) => env.put(key, value) }
+      if (args.clearEnv) env.clear()
+      args.extraEnv.foreach { case (key, value) => env.put(key, value) }
       pb.redirectErrorStream(true)
       pb.start()
     }


Test patch:
diff --git a/modules/core/src/test/scala/org/scalasteward/core/buildtool/BuildToolDispatcherTest.scala b/modules/core/src/test/scala/org/scalasteward/core/buildtool/BuildToolDispatcherTest.scala
index 1eb94639..3b114388 100644
--- a/modules/core/src/test/scala/org/scalasteward/core/buildtool/BuildToolDispatcherTest.scala
+++ b/modules/core/src/test/scala/org/scalasteward/core/buildtool/BuildToolDispatcherTest.scala
@@ -27,11 +27,11 @@ class BuildToolDispatcherTest extends AnyFunSuite with Matchers {
         List("test", "-f", s"$repoDir/build.sc"),
         List("test", "-f", s"$repoDir/build.sbt"),
         List(
-          "VAR1=val1",
-          "VAR2=val2",
           repoDir.toString,
           "firejail",
           s"--whitelist=$repoDir",
+          "--env=VAR1=val1",
+          "--env=VAR2=val2",
           "sbt",
           "-batch",
           "-no-colors",
diff --git a/modules/core/src/test/scala/org/scalasteward/core/buildtool/maven/MavenAlgTest.scala b/modules/core/src/test/scala/org/scalasteward/core/buildtool/maven/MavenAlgTest.scala
index 630526a6..3fe29c43 100644
--- a/modules/core/src/test/scala/org/scalasteward/core/buildtool/maven/MavenAlgTest.scala
+++ b/modules/core/src/test/scala/org/scalasteward/core/buildtool/maven/MavenAlgTest.scala
@@ -21,21 +21,21 @@ class MavenAlgTest extends AnyFunSuite with Matchers {
       logs = Vector.empty,
       commands = Vector(
         List(
-          "VAR1=val1",
-          "VAR2=val2",
           repoDir.toString,
           "firejail",
           s"--whitelist=$repoDir",
+          "--env=VAR1=val1",
+          "--env=VAR2=val2",
           "mvn",
           "--batch-mode",
           command.listDependencies
         ),
         List(
-          "VAR1=val1",
-          "VAR2=val2",
           repoDir.toString,
           "firejail",
           s"--whitelist=$repoDir",
+          "--env=VAR1=val1",
+          "--env=VAR2=val2",
           "mvn",
           "--batch-mode",
           command.listRepositories
diff --git a/modules/core/src/test/scala/org/scalasteward/core/buildtool/mill/MillAlgTest.scala b/modules/core/src/test/scala/org/scalasteward/core/buildtool/mill/MillAlgTest.scala
index 58dda7a7..cd508851 100644
--- a/modules/core/src/test/scala/org/scalasteward/core/buildtool/mill/MillAlgTest.scala
+++ b/modules/core/src/test/scala/org/scalasteward/core/buildtool/mill/MillAlgTest.scala
@@ -12,14 +12,24 @@ class MillAlgTest extends AnyFunSuite with Matchers {
     val repo = Repo("lihaoyi", "fastparse")
     val repoDir = config.workspace / repo.show
     val predef = s"$repoDir/scala-steward.sc"
-    val millCmd =
-      List("firejail", s"--whitelist=$repoDir", "mill", "-i", "-p", predef, "show", extractDeps)
+    val millCmd = List(
+      "firejail",
+      s"--whitelist=$repoDir",
+      "--env=VAR1=val1",
+      "--env=VAR2=val2",
+      "mill",
+      "-i",
+      "-p",
+      predef,
+      "show",
+      extractDeps
+    )
     val initial = MockState.empty.copy(commandOutputs = Map(millCmd -> List("""{"modules":[]}""")))
     val state = millAlg.getDependencies(repo).runS(initial).unsafeRunSync()
     state shouldBe initial.copy(
       commands = Vector(
         List("write", predef),
-        List("VAR1=val1", "VAR2=val2", repoDir.toString) ++ millCmd,
+        repoDir.toString :: millCmd,
         List("rm", "-rf", predef)
       )
     )
diff --git a/modules/core/src/test/scala/org/scalasteward/core/buildtool/sbt/SbtAlgTest.scala b/modules/core/src/test/scala/org/scalasteward/core/buildtool/sbt/SbtAlgTest.scala
index ef131ec5..a0ef29cc 100644
--- a/modules/core/src/test/scala/org/scalasteward/core/buildtool/sbt/SbtAlgTest.scala
+++ b/modules/core/src/test/scala/org/scalasteward/core/buildtool/sbt/SbtAlgTest.scala
@@ -39,11 +39,11 @@ class SbtAlgTest extends AnyFunSuite with Matchers {
     state shouldBe initial.copy(
       commands = Vector(
         List(
-          "VAR1=val1",
-          "VAR2=val2",
           repoDir.toString,
           "firejail",
           s"--whitelist=$repoDir",
+          "--env=VAR1=val1",
+          "--env=VAR2=val2",
           "sbt",
           "-batch",
           "-no-colors",
@@ -74,11 +74,11 @@ class SbtAlgTest extends AnyFunSuite with Matchers {
         List("write", "/tmp/steward/.sbt/0.13/plugins/scala-steward-scalafix.sbt"),
         List("write", "/tmp/steward/.sbt/1.0/plugins/scala-steward-scalafix.sbt"),
         List(
-          "VAR1=val1",
-          "VAR2=val2",
           repoDir.toString,
           "firejail",
           s"--whitelist=$repoDir",
+          "--env=VAR1=val1",
+          "--env=VAR2=val2",
           "sbt",
           "-batch",
           "-no-colors",
@@ -111,11 +111,11 @@ class SbtAlgTest extends AnyFunSuite with Matchers {
         List("write", "/tmp/steward/.sbt/1.0/plugins/scala-steward-scalafix.sbt"),
         List("write", s"$repoDir/scala-steward-scalafix-options.sbt"),
         List(
-          "VAR1=val1",
-          "VAR2=val2",
           repoDir.toString,
           "firejail",
           s"--whitelist=$repoDir",
+          "--env=VAR1=val1",
+          "--env=VAR2=val2",
           "sbt",
           "-batch",
           "-no-colors",
diff --git a/modules/core/src/test/scala/org/scalasteward/core/io/MockProcessAlg.scala b/modules/core/src/test/scala/org/scalasteward/core/io/MockProcessAlg.scala
index 5a23ab53..bdc34f8b 100644
--- a/modules/core/src/test/scala/org/scalasteward/core/io/MockProcessAlg.scala
+++ b/modules/core/src/test/scala/org/scalasteward/core/io/MockProcessAlg.scala
@@ -1,23 +1,16 @@
 package org.scalasteward.core.io
 
-import better.files.File
 import org.scalasteward.core.application.Config.ProcessCfg
-import org.scalasteward.core.io.ProcessAlg.UsingFirejail
 import org.scalasteward.core.mock.{applyPure, MockEff}
-import org.scalasteward.core.util.Nel
 
-class MockProcessAlg(config: ProcessCfg) extends UsingFirejail[MockEff](config.sandboxCfg) {
-  private val envVars = config.envVars.map(v => (v.name, v.value))
-
-  override def exec(
-      command: Nel[String],
-      cwd: File,
-      extraEnv: (String, String)*
-  ): MockEff[List[String]] =
-    applyPure { s =>
-      (
-        s.exec(cwd.toString :: command.toList, extraEnv ++ envVars: _*),
-        s.commandOutputs.getOrElse(command.toList, List.empty)
-      )
+object MockProcessAlg {
+  def create(config: ProcessCfg): ProcessAlg[MockEff] =
+    ProcessAlg.fromExecImpl(config) { args =>
+      applyPure { s =>
+        val cmd = args.workingDirectory.map(_.toString).toList ++ args.command.toList
+        val s1 = s.exec(cmd, args.extraEnv: _*)
+        val a = s.commandOutputs.getOrElse(args.command.toList, List.empty)
+        (s1, a)
+      }
     }
 }
diff --git a/modules/core/src/test/scala/org/scalasteward/core/io/ProcessAlgTest.scala b/modules/core/src/test/scala/org/scalasteward/core/io/ProcessAlgTest.scala
index 34a16939..c55dc342 100644
--- a/modules/core/src/test/scala/org/scalasteward/core/io/ProcessAlgTest.scala
+++ b/modules/core/src/test/scala/org/scalasteward/core/io/ProcessAlgTest.scala
@@ -30,7 +30,8 @@ class ProcessAlgTest extends AnyFunSuite with Matchers {
 
   test("execSandboxed: echo with disableSandbox = true") {
     val cfg = ProcessCfg(Nil, Duration.Zero, SandboxCfg(Nil, Nil, disableSandbox = true))
-    val state = new MockProcessAlg(cfg)
+    val state = MockProcessAlg
+      .create(cfg)
       .execSandboxed(Nel.of("echo", "hello"), File.temp)
       .runS(MockState.empty)
       .unsafeRunSync()
@@ -44,7 +45,8 @@ class ProcessAlgTest extends AnyFunSuite with Matchers {
 
   test("execSandboxed: echo with disableSandbox = false") {
     val cfg = ProcessCfg(Nil, Duration.Zero, SandboxCfg(Nil, Nil, disableSandbox = false))
-    val state = new MockProcessAlg(cfg)
+    val state = MockProcessAlg
+      .create(cfg)
       .execSandboxed(Nel.of("echo", "hello"), File.temp)
       .runS(MockState.empty)
       .unsafeRunSync()
diff --git a/modules/core/src/test/scala/org/scalasteward/core/io/processTest.scala b/modules/core/src/test/scala/org/scalasteward/core/io/processTest.scala
index d6b84aad..7e027226 100644
--- a/modules/core/src/test/scala/org/scalasteward/core/io/processTest.scala
+++ b/modules/core/src/test/scala/org/scalasteward/core/io/processTest.scala
@@ -3,6 +3,7 @@ package org.scalasteward.core.io
 import cats.effect.IO
 import org.scalasteward.core.TestInstances._
 import org.scalasteward.core.io.ProcessAlgTest.blocker
+import org.scalasteward.core.io.process.Args
 import org.scalasteward.core.util.Nel
 import org.scalatest.funsuite.AnyFunSuite
 import org.scalatest.matchers.should.Matchers
@@ -10,10 +11,10 @@ import scala.concurrent.duration._
 
 class processTest extends AnyFunSuite with Matchers {
   def slurp1(cmd: Nel[String]): IO[List[String]] =
-    process.slurp[IO](cmd, None, List.empty, 1.minute, _ => IO.unit, blocker)
+    process.slurp[IO](Args(cmd), 1.minute, _ => IO.unit, blocker)
 
   def slurp2(cmd: Nel[String], timeout: FiniteDuration): IO[List[String]] =
-    process.slurp[IO](cmd, None, List.empty, timeout, _ => IO.unit, blocker)
+    process.slurp[IO](Args(cmd), timeout, _ => IO.unit, blocker)
 
   test("echo hello") {
     slurp1(Nel.of("echo", "-n", "hello")).unsafeRunSync() shouldBe List("hello")
diff --git a/modules/core/src/test/scala/org/scalasteward/core/mock/MockContext.scala b/modules/core/src/test/scala/org/scalasteward/core/mock/MockContext.scala
index bd03b82c..6cad7028 100644
--- a/modules/core/src/test/scala/org/scalasteward/core/mock/MockContext.scala
+++ b/modules/core/src/test/scala/org/scalasteward/core/mock/MockContext.scala
@@ -3,6 +3,7 @@ package org.scalasteward.core.mock
 import better.files.File
 import cats.Parallel
 import cats.effect.Sync
+import io.chrisdavenport.log4cats.Logger
 import org.http4s.Uri
 import org.scalasteward.core.TestInstances.ioContextShift
 import org.scalasteward.core.application.Cli.EnvVar
@@ -14,7 +15,7 @@ import org.scalasteward.core.buildtool.sbt.SbtAlg
 import org.scalasteward.core.coursier.{CoursierAlg, VersionsCache}
 import org.scalasteward.core.edit.EditAlg
 import org.scalasteward.core.git.GitAlg
-import org.scalasteward.core.io.{MockFileAlg, MockProcessAlg, MockWorkspaceAlg}
+import org.scalasteward.core.io._
 import org.scalasteward.core.nurture.PullRequestRepository
 import org.scalasteward.core.persistence.JsonKeyValueStore
 import org.scalasteward.core.repocache.RepoCacheRepository
@@ -52,10 +53,10 @@ object MockContext {
   implicit val mockEffBracketThrow: BracketThrow[MockEff] = Sync[MockEff]
   implicit val mockEffParallel: Parallel[MockEff] = Parallel.identity
 
-  implicit val fileAlg: MockFileAlg = new MockFileAlg
-  implicit val mockLogger: MockLogger = new MockLogger
-  implicit val processAlg: MockProcessAlg = new MockProcessAlg(config.processCfg)
-  implicit val workspaceAlg: MockWorkspaceAlg = new MockWorkspaceAlg
+  implicit val fileAlg: FileAlg[MockEff] = new MockFileAlg
+  implicit val mockLogger: Logger[MockEff] = new MockLogger
+  implicit val processAlg: ProcessAlg[MockEff] = MockProcessAlg.create(config.processCfg)
+  implicit val workspaceAlg: WorkspaceAlg[MockEff] = new MockWorkspaceAlg
 
   implicit val coursierAlg: CoursierAlg[MockEff] = CoursierAlg.create
   implicit val dateTimeAlg: DateTimeAlg[MockEff] = DateTimeAlg.create