From 51afefc3d37f055931b0d8a7af88ee01953fb40f Mon Sep 17 00:00:00 2001 From: blanet Date: Mon, 11 Jan 2021 16:03:55 +0800 Subject: [PATCH 1/2] command: Fix improperly designed TestNewCommandProxyEnv command holds an env whitelist `exportedEnvVars` to filter and pass allowed envs from os.Environ() to internal exec.Cmd. TestNewCommandProxyEnv is designed to check whether http_proxy related envs can be whitelisted from os.Environ(), while the current implemention passed in these envs explicitly, making the test meaningless. This commit fixes TestNewCommandProxyEnv by simply setting http_proxy related envs through os.Setenv(). --- internal/command/command_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/command/command_test.go b/internal/command/command_test.go index 08f51a97a0a..297541f265f 100644 --- a/internal/command/command_test.go +++ b/internal/command/command_test.go @@ -91,13 +91,16 @@ func TestNewCommandProxyEnv(t *testing.T) { for _, tc := range testCases { t.Run(tc.key, func(t *testing.T) { - extraVar := fmt.Sprintf("%s=%s", tc.key, tc.value) + os.Setenv(tc.key, tc.value) + defer os.Unsetenv(tc.key) + buff := &bytes.Buffer{} - cmd, err := New(ctx, exec.Command("/usr/bin/env"), nil, buff, nil, extraVar) + cmd, err := New(ctx, exec.Command("/usr/bin/env"), nil, buff, nil) require.NoError(t, err) require.NoError(t, cmd.Wait()) + extraVar := fmt.Sprintf("%s=%s", tc.key, tc.value) require.Contains(t, strings.Split(buff.String(), "\n"), extraVar) }) } -- GitLab From 1070dcaa31463b4a7d204017f4c88c145e94efa4 Mon Sep 17 00:00:00 2001 From: blanet Date: Tue, 12 Jan 2021 10:00:50 +0800 Subject: [PATCH 2/2] command: Make passed in envs take higher priority This commit makes passed in envs take a higher priority over envs from environment, which avoids unexpected behaviors when we fall into env conflicts for spawned commands. And we should always respect args explicitly passed in anyway. We are lucky that there are no such env conflicts for gitaly at present, but we worth this additional safeguard. --- internal/command/command.go | 2 +- internal/command/command_test.go | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/internal/command/command.go b/internal/command/command.go index 9155751f47a..287fa6a7551 100644 --- a/internal/command/command.go +++ b/internal/command/command.go @@ -197,7 +197,7 @@ func New(ctx context.Context, cmd *exec.Cmd, stdin io.Reader, stdout, stderr io. env = append(env, "GIT_TERMINAL_PROMPT=0") // Export env vars - cmd.Env = append(env, AllowedEnvironment(os.Environ())...) + cmd.Env = append(AllowedEnvironment(os.Environ()), env...) cmd.Env = envInjector(ctx, cmd.Env) // Start the command in its own process group (nice for signalling) diff --git a/internal/command/command_test.go b/internal/command/command_test.go index 297541f265f..cc60394a3b0 100644 --- a/internal/command/command_test.go +++ b/internal/command/command_test.go @@ -106,6 +106,27 @@ func TestNewCommandProxyEnv(t *testing.T) { } } +func TestPassedInEnvsTakeHigherPriority(t *testing.T) { + envValueOs := "foobar_os" + envValuePassedIn := "foobar_passed_in" + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + oldTZ := os.Getenv("TZ") + defer os.Setenv("TZ", oldTZ) + + os.Setenv("TZ", envValueOs) + + buff := &bytes.Buffer{} + cmd, err := New(ctx, exec.Command("env"), nil, buff, nil, "TZ="+envValuePassedIn) + + require.NoError(t, err) + require.NoError(t, cmd.Wait()) + + require.Contains(t, strings.Split(buff.String(), "\n"), "TZ="+envValuePassedIn) +} + func TestRejectEmptyContextDone(t *testing.T) { defer func() { p := recover() -- GitLab