Give priority to environment variables

If an environment variable exists for secret_key_base, use that -
always. But don't save it to secrets.yml.

Also ensure that we never write to secrets.yml if there's a non-blank
value there.

2 files changed, 88 insertions, 10 deletions

diff --git a/config/initializers/secret_token.rb b/config/initializers/secret_token.rb
index ac99dcb59fc..5d8124b30b2 100644
--- a/config/initializers/secret_token.rb
+++ b/config/initializers/secret_token.rb
@@ -7,7 +7,7 @@ def generate_new_secure_token
 end
 
 def warn_missing_secret(secret)
-  warn "Missing `#{secret}` for '#{Rails.env}' environment. The secret will be generated and stored in `config/secrets.yml`"
+  warn "Missing Rails.application.secrets.#{secret} for #{Rails.env} environment. The secret will be generated and stored in config/secrets.yml."
 end
 
 def create_tokens
@@ -16,8 +16,11 @@ def create_tokens
   env_key = ENV['SECRET_KEY_BASE']
   yaml_additions = {}
 
+  # Ensure environment variable always overrides secrets.yml.
+  Rails.application.secrets.secret_key_base = env_key if env_key.present?
+
   defaults = {
-    secret_key_base: env_key || file_key || generate_new_secure_token,
+    secret_key_base: file_key || generate_new_secure_token,
     otp_key_base: env_key || file_key || generate_new_secure_token,
     db_key_base: generate_new_secure_token
   }
@@ -34,9 +37,22 @@ def create_tokens
     secrets_yml = Rails.root.join('config/secrets.yml')
     all_secrets = YAML.load_file(secrets_yml) if File.exist?(secrets_yml)
     all_secrets ||= {}
-
     env_secrets = all_secrets[Rails.env.to_s] || {}
-    all_secrets[Rails.env.to_s] = env_secrets.merge(yaml_additions)
+
+    all_secrets[Rails.env.to_s] = env_secrets.merge(yaml_additions) do |key, old, new|
+      if old.present?
+        warn <<EOM
+Rails.application.secrets.#{key} was blank, but the literal value in config/secrets.yml was:
+  #{old}
+
+This probably isn't the expected value for this secret. To keep using a literal Erb string in config/secrets.yml, replace `<%` with `<%%`.
+EOM
+
+        exit 1
+      end
+
+      new
+    end
 
     File.write(secrets_yml, YAML.dump(all_secrets), mode: 'w', perm: 0600)
   end
diff --git a/spec/initializers/secret_token_spec.rb b/spec/initializers/secret_token_spec.rb
index aa97a8e1d42..837b0de9a4c 100644
--- a/spec/initializers/secret_token_spec.rb
+++ b/spec/initializers/secret_token_spec.rb
@@ -10,6 +10,8 @@ describe 'create_tokens', lib: true do
     allow(File).to receive(:delete)
     allow(Rails).to receive_message_chain(:application, :secrets).and_return(secrets)
     allow(Rails).to receive_message_chain(:root, :join) { |string| string }
+    allow(self).to receive(:warn)
+    allow(self).to receive(:exit)
   end
 
   context 'setting secret_key_base and otp_key_base' do
@@ -61,11 +63,36 @@ describe 'create_tokens', lib: true do
       before do
         secrets.db_key_base = 'db_key_base'
 
-        allow(ENV).to receive(:[]).with('SECRET_KEY_BASE').and_return('env_key')
         allow(File).to receive(:exist?).with('.secret').and_return(true)
         allow(File).to receive(:read).with('.secret').and_return('file_key')
       end
 
+      context 'when secret_key_base exists in the environment and secrets.yml' do
+        before do
+          allow(ENV).to receive(:[]).with('SECRET_KEY_BASE').and_return('env_key')
+          secrets.secret_key_base = 'secret_key_base'
+          secrets.otp_key_base = 'otp_key_base'
+        end
+
+        it 'does not issue a warning' do
+          expect(self).not_to receive(:warn)
+
+          create_tokens
+        end
+
+        it 'uses the environment variable' do
+          create_tokens
+
+          expect(secrets.secret_key_base).to eq('env_key')
+        end
+
+        it 'does not update secrets.yml' do
+          expect(File).not_to receive(:write)
+
+          create_tokens
+        end
+      end
+
       context 'when secret_key_base and otp_key_base exist' do
         before do
           secrets.secret_key_base = 'secret_key_base'
@@ -78,7 +105,7 @@ describe 'create_tokens', lib: true do
           create_tokens
         end
 
-        it 'sets the the keys to the values from secrets.yml' do
+        it 'sets the the keys to the values from the environment and secrets.yml' do
           create_tokens
 
           expect(secrets.secret_key_base).to eq('secret_key_base')
@@ -100,18 +127,18 @@ describe 'create_tokens', lib: true do
           allow(self).to receive(:warn_missing_secret)
         end
 
-        it 'uses the env secret' do
+        it 'uses the file secret' do
           expect(File).to receive(:write) do |filename, contents, options|
             new_secrets = YAML.load(contents)[Rails.env]
 
-            expect(new_secrets['secret_key_base']).to eq('env_key')
-            expect(new_secrets['otp_key_base']).to eq('env_key')
+            expect(new_secrets['secret_key_base']).to eq('file_key')
+            expect(new_secrets['otp_key_base']).to eq('file_key')
             expect(new_secrets['db_key_base']).to eq('db_key_base')
           end
 
           create_tokens
 
-          expect(secrets.otp_key_base).to eq('env_key')
+          expect(secrets.otp_key_base).to eq('file_key')
         end
 
         it 'keeps the other secrets as they were' do
@@ -134,5 +161,40 @@ describe 'create_tokens', lib: true do
         end
       end
     end
+
+    context 'when db_key_base is blank but exists in secrets.yml' do
+      before do
+        secrets.otp_key_base = 'otp_key_base'
+        secrets.secret_key_base = 'secret_key_base'
+        yaml_secrets = secrets.to_h.stringify_keys.merge('db_key_base' => '<%= an_erb_expression %>')
+
+        allow(File).to receive(:exist?).with('.secret').and_return(false)
+        allow(File).to receive(:exist?).with('config/secrets.yml').and_return(true)
+        allow(YAML).to receive(:load_file).with('config/secrets.yml').and_return('test' => yaml_secrets)
+        allow(self).to receive(:warn_missing_secret)
+      end
+
+      it 'warns about updating db_key_base' do
+        expect(self).to receive(:warn_missing_secret).with('db_key_base')
+
+        create_tokens
+      end
+
+      it 'warns about the blank value existing in secrets.yml and exits' do
+        expect(self).to receive(:warn) do |warning|
+          expect(warning).to include('db_key_base')
+          expect(warning).to include('<%= an_erb_expression %>')
+        end
+
+        create_tokens
+      end
+
+      it 'does not update secrets.yml' do
+        expect(self).to receive(:exit).with(1).and_call_original
+        expect(File).not_to receive(:write)
+
+        expect { create_tokens }.to raise_error(SystemExit)
+      end
+    end
   end
 end