Prevent polluting ENV with postgres vars

rails · Jun 25, 2021 · 8478b26 · 8478b26
1 parent 93b0b30
commit 8478b26
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 23 deletions.
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
@@ -1,3 +1,13 @@
+*   Prevent polluting ENV during postgresql structure dump/load
+
+    Some configuration parameters were provided to pg_dump / psql via
+    environment variables which persisted beyond the command being run, and may
+    have caused subsequent commands and connections to fail. Tasks running
+    across multiple postgresql databases like `rails db:test:prepare` may have
+    been affected.
+
+    *Samuel Cochran*
+
 *   Set precision 6 by default for `datetime` columns
 
     By default, datetime columns will have microseconds precision instead of seconds precision.

diff --git a/activerecord/lib/active_record/tasks/postgresql_database_tasks.rb b/activerecord/lib/active_record/tasks/postgresql_database_tasks.rb
@@ -47,8 +47,6 @@ def purge
       end
 
       def structure_dump(filename, extra_flags)
-        set_psql_env
-
         search_path = \
           case ActiveRecord.dump_schemas
           when :schema_search_path
@@ -79,7 +77,6 @@ def structure_dump(filename, extra_flags)
       end
 
       def structure_load(filename, extra_flags)
-        set_psql_env
         args = ["--set", ON_ERROR_STOP_1, "--quiet", "--no-psqlrc", "--file", filename]
         args.concat(Array(extra_flags)) if extra_flags
         args << db_config.database
@@ -100,15 +97,17 @@ def establish_master_connection
           )
         end
 
-        def set_psql_env
-          ENV["PGHOST"]     = db_config.host                     if db_config.host
-          ENV["PGPORT"]     = configuration_hash[:port].to_s     if configuration_hash[:port]
-          ENV["PGPASSWORD"] = configuration_hash[:password].to_s if configuration_hash[:password]
-          ENV["PGUSER"]     = configuration_hash[:username].to_s if configuration_hash[:username]
+        def psql_env
+          {}.tap do |env|
+            env["PGHOST"]     = db_config.host                     if db_config.host
+            env["PGPORT"]     = configuration_hash[:port].to_s     if configuration_hash[:port]
+            env["PGPASSWORD"] = configuration_hash[:password].to_s if configuration_hash[:password]
+            env["PGUSER"]     = configuration_hash[:username].to_s if configuration_hash[:username]
+          end
         end
 
         def run_cmd(cmd, args, action)
-          fail run_cmd_error(cmd, args, action) unless Kernel.system(cmd, *args)
+          fail run_cmd_error(cmd, args, action) unless Kernel.system(psql_env, cmd, *args)
         end
 
         def run_cmd_error(cmd, args, action)

diff --git a/activerecord/test/cases/tasks/postgresql_rake_test.rb b/activerecord/test/cases/tasks/postgresql_rake_test.rb
@@ -361,7 +361,7 @@ def test_structure_dump
         assert_called_with(
           Kernel,
           :system,
-          ["pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "my-app-db"],
+          [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "my-app-db"],
           returns: true
         ) do
           ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, @filename)
@@ -377,8 +377,20 @@ def test_structure_dump_header_comments_removed
         end
       end
 
+      def test_structure_dump_with_env
+        expected_env = { "PGHOST" => "my.server.tld", "PGPORT" => "2345", "PGUSER" => "jane", "PGPASSWORD" => "s3cr3t" }
+        expected_command = [expected_env, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "my-app-db"]
+
+        assert_called_with(Kernel, :system, expected_command, returns: true) do
+          ActiveRecord::Tasks::DatabaseTasks.structure_dump(
+            @configuration.merge(host: "my.server.tld", port: 2345, username: "jane", password: "s3cr3t"),
+            @filename
+          )
+        end
+      end
+
       def test_structure_dump_with_extra_flags
-        expected_command = ["pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "--noop", "my-app-db"]
+        expected_command = [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "--noop", "my-app-db"]
 
         assert_called_with(Kernel, :system, expected_command, returns: true) do
           with_structure_dump_flags(["--noop"]) do
@@ -388,7 +400,7 @@ def test_structure_dump_with_extra_flags
       end
 
       def test_structure_dump_with_hash_extra_flags_for_a_different_driver
-        expected_command = ["pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "my-app-db"]
+        expected_command = [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "my-app-db"]
 
         assert_called_with(Kernel, :system, expected_command, returns: true) do
           with_structure_dump_flags({ mysql2: ["--noop"] }) do
@@ -398,7 +410,7 @@ def test_structure_dump_with_hash_extra_flags_for_a_different_driver
       end
 
       def test_structure_dump_with_hash_extra_flags_for_the_correct_driver
-        expected_command = ["pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "--noop", "my-app-db"]
+        expected_command = [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "--noop", "my-app-db"]
 
         assert_called_with(Kernel, :system, expected_command, returns: true) do
           with_structure_dump_flags({ postgresql: ["--noop"] }) do
@@ -416,7 +428,7 @@ def test_structure_dump_with_ignore_tables
           assert_called_with(
             Kernel,
             :system,
-            ["pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "-T", "foo", "-T", "bar", "my-app-db"],
+            [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "-T", "foo", "-T", "bar", "my-app-db"],
             returns: true
           ) do
             ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, @filename)
@@ -430,7 +442,7 @@ def test_structure_dump_with_schema_search_path
         assert_called_with(
           Kernel,
           :system,
-          ["pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "--schema=foo", "--schema=bar", "my-app-db"],
+          [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "--schema=foo", "--schema=bar", "my-app-db"],
           returns: true
         ) do
           ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, @filename)
@@ -443,7 +455,7 @@ def test_structure_dump_with_schema_search_path_and_dump_schemas_all
         assert_called_with(
           Kernel,
           :system,
-          ["pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename,  "my-app-db"],
+          [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename,  "my-app-db"],
           returns: true
         ) do
           with_dump_schemas(:all) do
@@ -456,7 +468,7 @@ def test_structure_dump_with_dump_schemas_string
         assert_called_with(
           Kernel,
           :system,
-          ["pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "--schema=foo", "--schema=bar", "my-app-db"],
+          [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", @filename, "--schema=foo", "--schema=bar", "my-app-db"],
           returns: true
         ) do
           with_dump_schemas("foo,bar") do
@@ -470,7 +482,7 @@ def test_structure_dump_execution_fails
         assert_called_with(
           Kernel,
           :system,
-          ["pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", filename, "my-app-db"],
+          [{}, "pg_dump", "--schema-only", "--no-privileges", "--no-owner", "--file", filename, "my-app-db"],
           returns: nil
         ) do
           e = assert_raise(RuntimeError) do
@@ -511,7 +523,7 @@ def test_structure_load
         assert_called_with(
           Kernel,
           :system,
-          ["psql", "--set", "ON_ERROR_STOP=1", "--quiet", "--no-psqlrc", "--file", filename, @configuration["database"]],
+          [{}, "psql", "--set", "ON_ERROR_STOP=1", "--quiet", "--no-psqlrc", "--file", filename, @configuration["database"]],
           returns: true
         ) do
           ActiveRecord::Tasks::DatabaseTasks.structure_load(@configuration, filename)
@@ -520,7 +532,7 @@ def test_structure_load
 
       def test_structure_load_with_extra_flags
         filename = "awesome-file.sql"
-        expected_command = ["psql", "--set", "ON_ERROR_STOP=1", "--quiet", "--no-psqlrc", "--file", filename, "--noop", @configuration["database"]]
+        expected_command = [{}, "psql", "--set", "ON_ERROR_STOP=1", "--quiet", "--no-psqlrc", "--file", filename, "--noop", @configuration["database"]]
 
         assert_called_with(Kernel, :system, expected_command, returns: true) do
           with_structure_load_flags(["--noop"]) do
@@ -529,9 +541,24 @@ def test_structure_load_with_extra_flags
         end
       end
 
+      def test_structure_load_with_env
+        filename = "awesome-file.sql"
+        expected_env = { "PGHOST" => "my.server.tld", "PGPORT" => "2345", "PGUSER" => "jane", "PGPASSWORD" => "s3cr3t" }
+        expected_command = [expected_env, "psql", "--set", "ON_ERROR_STOP=1", "--quiet", "--no-psqlrc", "--file", filename, "--noop", @configuration["database"]]
+
+        assert_called_with(Kernel, :system, expected_command, returns: true) do
+          with_structure_load_flags(["--noop"]) do
+            ActiveRecord::Tasks::DatabaseTasks.structure_load(
+              @configuration.merge(host: "my.server.tld", port: 2345, username: "jane", password: "s3cr3t"),
+              filename
+            )
+          end
+        end
+      end
+
       def test_structure_load_with_hash_extra_flags_for_a_different_driver
         filename = "awesome-file.sql"
-        expected_command = ["psql", "--set", "ON_ERROR_STOP=1", "--quiet", "--no-psqlrc", "--file", filename, @configuration["database"]]
+        expected_command = [{}, "psql", "--set", "ON_ERROR_STOP=1", "--quiet", "--no-psqlrc", "--file", filename, @configuration["database"]]
 
         assert_called_with(Kernel, :system, expected_command, returns: true) do
           with_structure_load_flags({ mysql2: ["--noop"] }) do
@@ -542,7 +569,7 @@ def test_structure_load_with_hash_extra_flags_for_a_different_driver
 
       def test_structure_load_with_hash_extra_flags_for_the_correct_driver
         filename = "awesome-file.sql"
-        expected_command = ["psql", "--set", "ON_ERROR_STOP=1", "--quiet", "--no-psqlrc", "--file", filename, "--noop", @configuration["database"]]
+        expected_command = [{}, "psql", "--set", "ON_ERROR_STOP=1", "--quiet", "--no-psqlrc", "--file", filename, "--noop", @configuration["database"]]
 
         assert_called_with(Kernel, :system, expected_command, returns: true) do
           with_structure_load_flags({ postgresql: ["--noop"] }) do
@@ -556,7 +583,7 @@ def test_structure_load_accepts_path_with_spaces
         assert_called_with(
           Kernel,
           :system,
-          ["psql", "--set", "ON_ERROR_STOP=1", "--quiet", "--no-psqlrc", "--file", filename, @configuration["database"]],
+          [{}, "psql", "--set", "ON_ERROR_STOP=1", "--quiet", "--no-psqlrc", "--file", filename, @configuration["database"]],
           returns: true
         ) do
           ActiveRecord::Tasks::DatabaseTasks.structure_load(@configuration, filename)