Quote CLI arguments in `RunsConfig` instructions

Summary:
Refactored build instructions in `RunsConfig` to properly quote command
arguments injected from user data.

Established unit tests for `RunsConfig` instruction phases.

Refs T170285. Depends on D711

Omit ENV instruction when `runs.environment` is empty

Fixes T170285

Test Plan:
Run `arc unit`. Create a config without `runs.environment` defined and verify
that no bare `ENV` line ends up in the `Dockerfile` output.

Reviewers: thcipriani, hashar, mmodell, #release-engineering-team

Reviewed By: thcipriani, #release-engineering-team

Tags: #release-engineering-team

Maniphest Tasks: T170285

Differential Revision: https://phabricator.wikimedia.org/D715

2 files changed, 65 insertions, 6 deletions

diff --git a/config/runs.go b/config/runs.go
index c61bff3..b2b6623 100644
--- a/config/runs.go
+++ b/config/runs.go
@@ -1,6 +1,7 @@
 package config
 
 import (
+	"fmt"
 	"sort"
 	"strconv"
 
@@ -70,20 +71,19 @@ func (run RunsConfig) InstructionsForPhase(phase build.Phase) []build.Instructio
 	case build.PhasePrivileged:
 		if run.In != "" {
 			ins = append(ins, build.Instruction{build.Run, []string{
-				"mkdir -p ", run.In,
+				fmt.Sprintf("mkdir -p %q", run.In),
 			}})
 		}
 
 		if run.As != "" {
 			ins = append(ins, build.Instruction{build.Run, []string{
-				"groupadd -o -g ", strconv.Itoa(run.Gid), " -r ", run.As, " && ",
-				"useradd -o -m -d ", strconv.Quote(run.Home()), " -r -g ", run.As,
-				" -u ", strconv.Itoa(run.Uid), " ", run.As,
+				fmt.Sprintf("groupadd -o -g %d -r %q", run.Gid, run.As) + " && " +
+					fmt.Sprintf("useradd -o -m -d %q -r -g %q -u %d %q", run.Home(), run.As, run.Uid, run.As),
 			}})
 
 			if run.In != "" {
 				ins = append(ins, build.Instruction{build.Run, []string{
-					"chown ", run.As, ":", run.As, " ", run.In,
+					fmt.Sprintf("chown %q:%q %q", run.As, run.As, run.In),
 				}})
 			}
 		}
@@ -92,7 +92,9 @@ func (run RunsConfig) InstructionsForPhase(phase build.Phase) []build.Instructio
 			"HOME=" + strconv.Quote(run.Home()),
 		}})
 
-		ins = append(ins, build.Instruction{build.Env, run.EnvironmentDefinitions()})
+		if definitions := run.EnvironmentDefinitions(); len(definitions) > 0 {
+			ins = append(ins, build.Instruction{build.Env, definitions})
+		}
 	}
 
 	return ins
diff --git a/config/runs_test.go b/config/runs_test.go
index f8c626e..cef740c 100644
--- a/config/runs_test.go
+++ b/config/runs_test.go
@@ -5,6 +5,7 @@ import (
 
 	"gopkg.in/stretchr/testify.v1/assert"
 
+	"phabricator.wikimedia.org/source/blubber.git/build"
 	"phabricator.wikimedia.org/source/blubber.git/config"
 )
 
@@ -59,3 +60,59 @@ func TestEnvironmentDefinitionsIsSortedAndQuoted(t *testing.T) {
 		`quxname="quxvalue"`,
 	}, runs.EnvironmentDefinitions())
 }
+
+func TestRunsConfigInstructions(t *testing.T) {
+	cfg := config.RunsConfig{
+		As:  "someuser",
+		In:  "/some/directory",
+		Uid: 666,
+		Gid: 777,
+		Environment: map[string]string{
+			"fooname": "foovalue",
+			"barname": "barvalue",
+		},
+	}
+
+	t.Run("PhasePrivileged", func(t *testing.T) {
+		assert.Equal(t,
+			[]build.Instruction{
+				{build.Run, []string{`mkdir -p "/some/directory"`}},
+				{build.Run, []string{
+					`groupadd -o -g 777 -r "someuser" && ` +
+						`useradd -o -m -d "/home/someuser" -r -g "someuser" -u 666 "someuser"`,
+				}},
+				{build.Run, []string{`chown "someuser":"someuser" "/some/directory"`}},
+			},
+			cfg.InstructionsForPhase(build.PhasePrivileged),
+		)
+	})
+
+	t.Run("PhasePrivilegeDropped", func(t *testing.T) {
+		assert.Equal(t,
+			[]build.Instruction{
+				{build.Env, []string{`HOME="/home/someuser"`}},
+				{build.Env, []string{`barname="barvalue"`, `fooname="foovalue"`}},
+			},
+			cfg.InstructionsForPhase(build.PhasePrivilegeDropped),
+		)
+
+		t.Run("with empty Environment", func(t *testing.T) {
+			cfg.Environment = map[string]string{}
+
+			assert.Equal(t,
+				[]build.Instruction{
+					{build.Env, []string{`HOME="/home/someuser"`}},
+				},
+				cfg.InstructionsForPhase(build.PhasePrivilegeDropped),
+			)
+		})
+	})
+
+	t.Run("PhasePreInstall", func(t *testing.T) {
+		assert.Empty(t, cfg.InstructionsForPhase(build.PhasePreInstall))
+	})
+
+	t.Run("PhasePostInstall", func(t *testing.T) {
+		assert.Empty(t, cfg.InstructionsForPhase(build.PhasePostInstall))
+	})
+}