[supervisor] fix #5104: don't truncate env values after = char

components/supervisor/pkg/supervisor/supervisor.go

@@ -194,7 +194,7 @@ func Run(options ...RunOption) {
 	go analyseConfigChanges(ctx, cfg, analytics, gitpodConfigService)
 
 	termMuxSrv.DefaultWorkdir = cfg.RepoRoot
-	termMuxSrv.Env = buildChildProcEnv(cfg)
+	termMuxSrv.Env = buildChildProcEnv(cfg, nil)
 	termMuxSrv.DefaultCreds = &syscall.Credential{
 		Uid: gitpodUID,
 		Gid: gitpodGID,
@@ -580,7 +580,7 @@ func prepareIDELaunch(cfg *Config) *exec.Cmd {
 			Gid: gitpodGID,
 		},
 	}
-	cmd.Env = buildChildProcEnv(cfg)
+	cmd.Env = buildChildProcEnv(cfg, nil)
 
 	// Here we must resist the temptation to "neaten up" the IDE output for headless builds.
 	// This would break the JSON parsing of the headless builds.
@@ -596,10 +596,16 @@ func prepareIDELaunch(cfg *Config) *exec.Cmd {
 	return cmd
 }
 
-func buildChildProcEnv(cfg *Config) []string {
+// buildChildProcEnv computes the environment variables passed to a child process, based on the total list
+// of envvars. If envvars is nil, os.Environ() is used.
+func buildChildProcEnv(cfg *Config, envvars []string) []string {
+	if envvars == nil {
+		envvars = os.Environ()
+	}
+
 	envs := make(map[string]string)
-	for _, e := range os.Environ() {
-		segs := strings.Split(e, "=")
+	for _, e := range envvars {
+		segs := strings.SplitN(e, "=", 2)
 		if len(segs) < 2 {
 			log.Printf("\"%s\" has invalid format, not including in IDE environment", e)
 			continue
@@ -633,7 +639,9 @@ func buildChildProcEnv(cfg *Config) []string {
 	envs["USER"] = "gitpod"
 
 	// Particular Java optimisation: Java pre v10 did not gauge it's available memory correctly, and needed explicitly setting "-Xmx" for all Hotspot/openJDK VMs
-	envs["JAVA_TOOL_OPTIONS"] += fmt.Sprintf(" -Xmx%sm", os.Getenv("GITPOD_MEMORY"))
+	if mem, ok := envs["GITPOD_MEMORY"]; ok {
+		envs["JAVA_TOOL_OPTIONS"] += fmt.Sprintf(" -Xmx%sm", mem)
+	}
 
 	var env, envn []string
 	for nme, val := range envs {
@@ -926,7 +934,7 @@ func startSSHServer(ctx context.Context, cfg *Config, wg *sync.WaitGroup) {
 	}
 
 	cmd := exec.Command(dropbear, "-F", "-E", "-w", "-s", "-p", fmt.Sprintf(":%d", cfg.SSHPort), "-r", hostkeyFN.Name())
-	cmd.Env = buildChildProcEnv(cfg)
+	cmd.Env = buildChildProcEnv(cfg, nil)
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
 	err = cmd.Start()

components/supervisor/pkg/supervisor/supervisor_test.go

@@ -0,0 +1,128 @@
+// Copyright (c) 2020 Gitpod GmbH. All rights reserved.
+// Licensed under the GNU Affero General Public License (AGPL).
+// See License-AGPL.txt in the project root for license information.
+
+package supervisor
+
+import (
+	"fmt"
+	"os"
+	"sort"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+)
+
+func TestBuildChildProcEnv(t *testing.T) {
+	withBaseline := func(i []string) []string {
+		return append(i,
+			"SUPERVISOR_ADDR=localhost:8080",
+			"HOME=/home/gitpod",
+			"USER=gitpod",
+		)
+	}
+
+	tests := []struct {
+		Name        string
+		Input       []string
+		Expectation []string
+		Assert      func(t *testing.T, act []string)
+	}{
+		{
+			Name:        "empty set",
+			Input:       []string{},
+			Expectation: withBaseline(nil),
+		},
+		{
+			Name:        "= in value",
+			Input:       []string{"FOO=BAR=BAZ"},
+			Expectation: withBaseline([]string{"FOO=BAR=BAZ"}),
+		},
+		{
+			Name: "override baseline",
+			Input: []string{
+				"SUPERVISOR_ADDR=foobar",
+				"HOME=foobar",
+				"USER=foobar",
+			},
+			Expectation: withBaseline(nil),
+		},
+		{
+			Name:        "removes blacklisted vars",
+			Input:       []string{"GITPOD_TOKENS=foobar"},
+			Expectation: withBaseline(nil),
+		},
+		{
+			Name:        "invalid env var",
+			Input:       []string{"FOOBAR"},
+			Expectation: withBaseline(nil),
+		},
+		{
+			// for testing purposes we can pass a set of envvars to buildChildProcEnv.
+			// When a caller passes nil, buildChildProcEnv calls os.Environ().
+			// This test case ensures we make that call and don't break on behaviour introduced
+			// just for testing.
+			Name:  "os.Environ",
+			Input: nil,
+			Assert: func(t *testing.T, act []string) {
+				// if we've called os.Environ, we expect PATH to be present
+				var (
+					hasPath bool
+					path    = fmt.Sprintf("PATH=%s", os.Getenv("PATH"))
+				)
+				for _, e := range act {
+					if e == path {
+						hasPath = true
+						break
+					}
+				}
+				if !hasPath {
+					t.Errorf("no PATH envvar found - probably did not call os.Environ()")
+				}
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.Name, func(t *testing.T) {
+			assert := test.Assert
+			if assert == nil {
+				assert = func(t *testing.T, act []string) {
+					exp := test.Expectation
+					sort.Strings(act)
+					sort.Strings(exp)
+					if diff := cmp.Diff(exp, act); diff != "" {
+						t.Errorf("unexpected buildChildProcEnv (-want +got):\n%s", diff)
+					}
+				}
+			}
+
+			act := buildChildProcEnv(&Config{StaticConfig: StaticConfig{APIEndpointPort: 8080}}, test.Input)
+			assert(t, act)
+		})
+	}
+}
+
+func TestIsBlacklistedEnvvar(t *testing.T) {
+	tests := []struct {
+		Name        string
+		Input       string
+		Expectation bool
+	}{
+		{Name: "deprecated theia envvars", Input: "THEIA_SUPERVISOR_FOOBAR", Expectation: true},
+		{Name: "gitpod tokens", Input: "GITPOD_TOKENS", Expectation: true},
+		{Name: "gitpod tokens child", Input: "GITPOD_TOKENS_GITHUB", Expectation: true},
+		{Name: "kubernetes services", Input: "KUBERNETES_SERVICE_FOOBAR", Expectation: true},
+		{Name: "kubernetes service ports", Input: "KUBERNETES_PORT_FOOBAR", Expectation: true},
+		{Name: "something with spaces", Input: "   I_DO_NOT_UNDERSTAND", Expectation: true},
+		{Name: "path", Input: "PATH", Expectation: false},
+	}
+	for _, test := range tests {
+		t.Run(test.Name, func(t *testing.T) {
+			act := isBlacklistedEnvvar(test.Input)
+			if diff := cmp.Diff(test.Expectation, act); diff != "" {
+				t.Errorf("unexpected isBlacklistedEnvvar (-want +got):\n%s", diff)
+			}
+		})
+	}
+}