Fix child processes having access to all of samsons env, including se…

…crets

Also fixes race condition where with_clean_env might have just ended in another thread and even .env values get exposed

.env.example

@@ -45,6 +45,8 @@ GITHUB_TOKEN={fill me in}
 
 # FORCE_SSL={optional, set to 1 to require SSL}
 
+# ENV_WHITELIST={optional, comma separated list of env values that should be used when deploying }
+
 ## JIRA_BASE_URL, if set, would enable the auto-detection of JIRA issue keys
 ## (e.g., KEY-123, SAMSON-456) in the titles and bodies of the pull requests
 ## associated with a deploy. The auto-detected JIRA issues will be displayed

app/models/git_repository.rb

@@ -102,8 +102,8 @@ def valid_url?
   def downstream_commit?(old_commit, new_commit)
     return true if old_commit == new_commit
     cmd = "git merge-base --is-ancestor #{old_commit} #{new_commit}"
-    status, output = run_single_command(cmd) { |line| line.strip }
-    $?.exitstatus == 1
+    status, output = run_single_command(cmd) { |l| l }
+    !status && !output[0].to_s.include?("fatal")
   end
 
   def executor

app/models/terminal_executor.rb

@@ -1,5 +1,4 @@
-require 'pty'
-require 'shellwords'
+require 'open3'
 
 # Executes commands in a fake terminal. The output will be streamed to a
 # specified IO-like object.
@@ -34,26 +33,30 @@ def stop!(signal)
   private
 
   def execute_command!(command)
-    output, input, @pid = Bundler.with_clean_env do
-      PTY.spawn(command, in: '/dev/null')
-    end
+    Open3.popen2e(whitelisted_env, command, in: '/dev/null', unsetenv_others: true, pgroup: true)  do |stdin, oe, wait_thr|
+      @pid = wait_thr.pid
 
-    @pgid = begin
-      Process.getpgid(pid)
-    rescue Errno::ESRCH
-      nil
-    end
+      @pgid = begin
+        Process.getpgid(@pid)
+      rescue Errno::ESRCH
+        nil
+      end
 
-    begin
-      output.each(256) {|line| @output.write(line) }
-    rescue Errno::EIO
-      # The IO has been closed.
-    end
+      oe.each(256) do |line|
+        @output.write line.gsub("\n", "\r\n")
+      end
 
-    _, status = Process.wait2(pid)
-
-    input.close
+      wait_thr.value
+    end.success?
+  end
 
-    return status.success?
+  def whitelisted_env
+    whitelist = [
+      'PATH', 'HOME', 'TMPDIR', 'CACHE_DIR', 'TERM', 'SHELL', # general
+      'RBENV_ROOT', 'RBENV_HOOK_PATH', 'RBENV_DIR', # ruby
+      'DOCKER_URL', 'DOCKER_REGISTRY' # docker
+    ] + ENV['ENV_WHITELIST'].to_s.split(/, ?/)
+    ENV.to_h.slice(*whitelist)
   end
 end
+

test/models/git_repository_test.rb

@@ -218,5 +218,4 @@
       repository.clean!
     end
   end
-
 end

test/models/terminal_executor_test.rb

@@ -1,6 +1,18 @@
 require_relative '../test_helper'
 
 describe TerminalExecutor do
+  def with_env(env)
+    old = env.map do |k, v|
+      k = k.to_s
+      o = ENV[k]
+      ENV[k] = v
+      [k, o]
+    end
+    yield
+  ensure
+    old.each { |k, v| ENV[k] = v }
+  end
+
   let(:output) { StringIO.new }
   subject { TerminalExecutor.new(output) }
 
@@ -28,6 +40,28 @@
       subject.execute!('echo "hi"').must_equal(true)
     end
 
+    it 'does not expose env secrets' do
+      with_env MY_SECRET: 'XYZ' do
+        subject.execute!('env')
+        output.string.wont_include("SECRET")
+      end
+    end
+
+    it "keeps CACHE_DIR" do
+      with_env CACHE_DIR: 'XYZ' do
+        subject.execute!('env')
+        output.string.must_include("CACHE_DIR=XYZ")
+      end
+    end
+
+    it "keeps custom env vars from ENV_WHITELIST" do
+      with_env ENV_WHITELIST: 'ABC, XYZ,ZZZ', XYZ: 'FOO', ZZZ: 'FOO' do
+        subject.execute!('env')
+        output.string.must_include("XYZ=FOO")
+        output.string.must_include("ZZZ=FOO")
+      end
+    end
+
     describe 'in verbose mode' do
       subject { TerminalExecutor.new(output, verbose: true) }