pass PGPASSWORD via env directly, not via shell

lib/foreman_maintain/concerns/base_database.rb

@@ -55,22 +55,25 @@ def query_csv(sql, config = configuration)
 
       def psql(query, config = configuration)
         if ping(config)
-          execute(psql_command(config),
+          cmd, env = psql_command(config)
+          execute(cmd,
             :stdin => query,
-            :hidden_patterns => [config['password']])
+            :env => env)
         else
           raise_service_error
         end
       end
 
       def ping(config = configuration)
-        execute?(psql_command(config),
+        cmd, env = psql_command(config)
+        execute?(cmd,
           :stdin => 'SELECT 1 as ping',
-          :hidden_patterns => [config['password']])
+          :env => env)
       end
 
       def dump_db(file, config = configuration)
-        execute!(dump_command(config) + " > #{file}", :hidden_patterns => [config['password']])
+        cmd, env = dump_command(config)
+        execute!(cmd + " > #{file}", :env => env)
       end
 
       def restore_dump(file, localdb, config = configuration)
@@ -80,11 +83,10 @@ def restore_dump(file, localdb, config = configuration)
         else
           # TODO: figure out how to completely ignore errors. Currently this
           # sometimes exits with 1 even though errors are ignored by pg_restore
-          dump_cmd = base_command(config, 'pg_restore') +
-                     ' --no-privileges --clean --disable-triggers -n public ' \
-                     "-d #{config['database']} #{file}"
-          execute!(dump_cmd, :hidden_patterns => [config['password']],
-            :valid_exit_statuses => [0, 1])
+          cmd, env = base_command(config, 'pg_restore')
+          cmd += ' --no-privileges --clean --disable-triggers -n public ' \
+                 "-d #{config['database']} #{file}"
+          execute!(cmd, :valid_exit_statuses => [0, 1], :env => env)
         end
       end
 
@@ -110,7 +112,7 @@ def backup_dir
       end
 
       def dropdb(config = configuration)
-        if local?
+        if local?(config)
           execute!("runuser - postgres -c 'dropdb #{config['database']}'")
         else
           delete_statement = psql(<<-SQL)
@@ -125,8 +127,9 @@ def dropdb(config = configuration)
       def db_version(config = configuration)
         if ping(config)
           # Note - t removes headers, -A removes alignment whitespace
-          server_version_cmd = psql_command(config) + ' -c "SHOW server_version" -t -A'
-          version_string = execute!(server_version_cmd, :hidden_patterns => [config['password']])
+          cmd, env = psql_command(config)
+          cmd += ' -c "SHOW server_version" -t -A'
+          version_string = execute!(cmd, :env => env)
           version(version_string)
         else
           raise_service_error
@@ -146,17 +149,20 @@ def raise_psql_missing_error
       private
 
       def base_command(config, command = 'psql')
-        "PGPASSWORD='#{config[%(password)]}' "\
-        "#{command} -h #{config['host'] || 'localhost'} "\
+        env = { 'PGPASSWORD' => config['password'] }
+        cmd = "#{command} -h #{config['host'] || 'localhost'} "\
         " -p #{config['port'] || '5432'} -U #{config['username']}"
+        return cmd, env
       end
 
       def psql_command(config)
-        base_command(config, 'psql') + " -d #{config['database']}"
+        cmd, env = base_command(config, 'psql')
+        return cmd + " -d #{config['database']}", env
       end
 
       def dump_command(config)
-        base_command(config, 'pg_dump') + " -Fc #{config['database']}"
+        cmd, env = base_command(config, 'pg_dump')
+        return cmd + " -Fc #{config['database']}", env
       end
 
       def raise_service_error

lib/foreman_maintain/utils/command_runner.rb

@@ -8,26 +8,27 @@ class CommandRunner
       attr_reader :logger, :command
 
       def initialize(logger, command, options)
-        options.validate_options!(:stdin, :hidden_patterns, :interactive, :valid_exit_statuses)
+        options.validate_options!(:stdin, :interactive, :valid_exit_statuses, :env)
         options[:valid_exit_statuses] ||= [0]
+        options[:env] ||= {}
         @logger = logger
         @command = command
         @stdin = options[:stdin]
-        @hidden_patterns = Array(options[:hidden_patterns]).compact
         @interactive = options[:interactive]
         @options = options
         @valid_exit_statuses = options[:valid_exit_statuses]
+        @env = options[:env]
         raise ArgumentError, 'Can not pass stdin for interactive command' if @interactive && @stdin
       end
 
       def run
-        logger&.debug(hide_strings("Running command #{@command} with stdin #{@stdin.inspect}"))
+        logger&.debug("Running command #{@command} with stdin #{@stdin.inspect}")
         if @interactive
           run_interactively
         else
           run_non_interactively
         end
-        logger&.debug("output of the command:\n #{hide_strings(output)}")
+        logger&.debug("output of the command:\n #{output}")
       end
 
       def interactive?
@@ -49,10 +50,10 @@ def success?
       end
 
       def execution_error
-        raise Error::ExecutionError.new(hide_strings(@command),
+        raise Error::ExecutionError.new(@command,
           exit_status,
-          hide_strings(@stdin),
-          @interactive ? nil : hide_strings(@output))
+          @stdin,
+          @interactive ? nil : @output)
       end
 
       private
@@ -81,7 +82,7 @@ def run_interactively
       end
 
       def run_non_interactively
-        IO.popen(full_command, 'r+') do |f|
+        IO.popen(@env, full_command, 'r+') do |f|
           if @stdin
             f.puts(@stdin)
             f.close_write
@@ -94,13 +95,6 @@ def run_non_interactively
       def full_command
         "#{@command} 2>&1"
       end
-
-      def hide_strings(string)
-        return unless string
-        @hidden_patterns.reduce(string) do |result, hidden_pattern|
-          result.gsub(hidden_pattern, '[FILTERED]')
-        end
-      end
     end
   end
 end

test/lib/concerns/base_database_test.rb

@@ -0,0 +1,132 @@
+require 'test_helper'
+
+class FakeDatabase
+  include ForemanMaintain::Concerns::BaseDatabase
+  include ForemanMaintain::Concerns::SystemHelpers
+end
+
+module ForemanMaintain
+  describe Concerns::BaseDatabase do
+    let(:db) { FakeDatabase.new }
+    let(:local_config) do
+      {
+        'host' => 'localhost',
+        'database' => 'fakedb',
+        'username' => 'fakeuser',
+        'password' => 'fakepassword',
+      }
+    end
+    let(:remote_config) do
+      {
+        'host' => 'db.example.com',
+        'database' => 'fakedb',
+        'username' => 'fakeuser',
+        'password' => 'fakepassword',
+      }
+    end
+
+    it 'accepts localhost as local' do
+      assert db.local?(local_config)
+    end
+
+    it 'accepts db.example.com as remote' do
+      refute db.local?(remote_config)
+    end
+
+    it 'fetches server version' do
+      db.expects(:ping).with(local_config).returns(true)
+      db.expects(:execute!).with(
+        'psql -h localhost  -p 5432 -U fakeuser -d fakedb -c "SHOW server_version" -t -A',
+        env: { "PGPASSWORD" => "fakepassword" }
+      ).returns('13.16')
+
+      assert db.db_version(local_config)
+    end
+
+    it 'drops local db' do
+      db.expects(:execute!).with("runuser - postgres -c 'dropdb fakedb'").returns('')
+
+      assert db.dropdb(local_config)
+    end
+
+    it 'drops remote db' do
+      select_statement = <<-SQL
+            select string_agg('drop table if exists \"' || tablename || '\" cascade;', '')
+            from pg_tables
+            where schemaname = 'public';
+      SQL
+      delete_statement = 'drop table if exists \"faketable\"'
+      db.expects(:psql).with(select_statement).returns(delete_statement)
+      db.expects(:psql).with(delete_statement).returns('')
+      assert db.dropdb(remote_config)
+    end
+
+    it 'restores local db' do
+      file = '/backup/fake.dump'
+
+      db.expects(:execute!).with("runuser - postgres -c 'pg_restore -C -d postgres #{file}'").
+        returns('')
+
+      assert db.restore_dump(file, true, local_config)
+    end
+
+    it 'restores remote db' do
+      file = '/backup/fake.dump'
+      restore_cmd = <<~CMD.strip
+        pg_restore -h db.example.com  -p 5432 -U fakeuser --no-privileges --clean --disable-triggers -n public -d fakedb #{file}
+      CMD
+
+      db.expects(:execute!).with(
+        restore_cmd,
+        valid_exit_statuses: [0, 1],
+        env: { "PGPASSWORD" => "fakepassword" }
+      ).returns('')
+
+      assert db.restore_dump(file, false, remote_config)
+    end
+
+    it 'dumps local db' do
+      file = '/backup/fake.dump'
+
+      db.expects(:execute!).with(
+        "pg_dump -h localhost  -p 5432 -U fakeuser -Fc fakedb > /backup/fake.dump",
+        env: { "PGPASSWORD" => "fakepassword" }
+      ).returns('')
+
+      assert db.dump_db(file, local_config)
+    end
+
+    it 'dumps remote db' do
+      file = '/backup/fake.dump'
+
+      db.expects(:execute!).with(
+        "pg_dump -h db.example.com  -p 5432 -U fakeuser -Fc fakedb > /backup/fake.dump",
+        env: { "PGPASSWORD" => "fakepassword" }
+      ).returns('')
+
+      assert db.dump_db(file, remote_config)
+    end
+
+    it 'pings db' do
+      db.expects(:execute?).with("psql -h localhost  -p 5432 -U fakeuser -d fakedb",
+        stdin: "SELECT 1 as ping", env: { "PGPASSWORD" => "fakepassword" }).returns(true)
+
+      assert db.ping(local_config)
+    end
+
+    it 'runs db queries' do
+      psql_return = <<~PSQL
+         test
+        ------
+           42
+        (1 row)
+      PSQL
+
+      db.expects(:ping).with(local_config).returns(true)
+      db.expects(:execute).with("psql -h localhost  -p 5432 -U fakeuser -d fakedb",
+        stdin: "SELECT 42 as test", env: { "PGPASSWORD" => "fakepassword" }).returns(psql_return)
+
+      assert db.psql('SELECT 42 as test', local_config)
+    end
+  end
+end