Fix config.secret_key_base warning about secrets

[skipkayhil]

Motivation / Background

Using config.secret_key_base currently raises a deprecation warning when used in production because config.secret_key_base gets merged into the secrets hash instead of being looked up specifically in the secret_key_base method.

Detail

This commit addresses this by not raising a deprecation warning if secrets.secret_key_base and config.secret_key_base are the same object (meaning config.secret_key_base was merged into `secrets).

Additionally, an improved deprecation warning is added for apps that continue to set secret_key_base in their secrets. The current warning is not great because it isn't directly actionable for users. Currently they will see the warning, not see secrets being referenced in their app, and potentially end up confused. The new warning helps users understand the actual change they need to make: not removing a reference to secrets but moving secret_key_base out of secrets.

Additional Information

I based this on main because the deprecation hasn't been removed yet, but I can change the base to 7-1-stable since that's really where this change should be made.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[p8]

I'm not sure this is the correct location to check if secrets.secret_key_base equals config.secret_key_base.
I think a better location is where we do the assignment of secrets.secret_key_base:

rails/railties/lib/rails/application.rb

Line 451 in 1abd331

secrets.secret_key_base ||= config.secret_key_base

I think it also might make sense to move the updated deprecation in the memoized code block.
Maybe change that to the following:

  if secrets.secret_key_base.equal?(config.secret_key_base)
     Rails.deprecator.warn(<<~MSG.squish)
                Your `secret_key_base is configured in `Rails.application.secrets`,
                which is deprecated in favor of `Rails.application.credentials` and
                will be removed in Rails 7.2.
              MSG
            
  else
     secrets.secret_key_base ||= config.secret_key_base
  end

[skipkayhil]

Interesting, I think you're proposing something like this?

diff --git a/railties/lib/rails/application.rb b/railties/lib/rails/application.rb
index c3b0302a2c..50047507b9 100644
--- a/railties/lib/rails/application.rb
+++ b/railties/lib/rails/application.rb
@@ -448,7 +448,11 @@ def secrets
         secrets.merge! Rails::Secrets.parse(files, env: Rails.env)

         # Fallback to config.secret_key_base if secrets.secret_key_base isn't set
-        secrets.secret_key_base ||= config.secret_key_base
+        if secrets.secret_key_base
+          Rails.deprecator.warn("skb set in secrets")
+        else
+          secrets.secret_key_base = config.secret_key_base
+        end

         secrets
       end
@@ -477,7 +481,7 @@ def secret_key_base
         config.secret_key_base ||= generate_local_secret
       else
         validate_secret_key_base(
-          ENV["SECRET_KEY_BASE"] || credentials.secret_key_base || secrets.secret_key_base
+          ENV["SECRET_KEY_BASE"] || credentials.secret_key_base || secrets_secret_key_base
         )
       end
     end

This does pass the test I added, but I think this won't show the new deprecation warning if the only use of secrets is coming from secret_key_base (so I may need to add a second test for that). Alternatively if secrets_secret_key_base is kept as secrets.secret_key_base then the user will see both deprecation warnings which is part of what I want to avoid with this patch.

[p8]

Good point!
So we have two different deprecation warnings?

1. Using config.secrets directly in the app should use the existing deprecation warning as the user should be able to fix these.
2. Rails should output a different warning when the secret_key_base is still set/used in the secrets?

Maybe we can move the warnings to secrets_secret_key_base?
This would warn in development as well (although twice in the current implementation).

      def secrets_secret_key_base
        secret_key_base = Rails.deprecator.silence do
          secrets.secret_key_base
        end
        if secret_key_base
          Rails.deprecator.warn("skb set in secrets")
          secret_key_base
        end
      end

This also means we should use secrets_secret_key_base in the validate_secret_key_base call.