Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Rails.application.secret_key_base directly #3043

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

jpawlyn
Copy link
Contributor

@jpawlyn jpawlyn commented Jul 22, 2024

Description

Port of #2417 to the main branch

Fixes # (issue)

Removes the deprecation message output when running tests:

DEPRECATION WARNING: `Rails.application.secrets` is deprecated in favor of `Rails.application.credentials` and will be removed in Rails 7.2. (called from secret_key_base at /Users/jpawlyn/.rbenv/versions/3.3.4/lib/ruby/gems/3.3.0/gems/avo-3.10.6/lib/avo/services/encryption_service.rb:42)

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

Manual review steps

Copy link

codeclimate bot commented Jul 22, 2024

Code Climate has analyzed commit c084954 and detected 0 issues on this pull request.

View more on Code Climate.

@Paul-Bob
Copy link
Contributor

Paul-Bob commented Jul 22, 2024

Thank you for the contribution @jpawlyn!

What if ENV["SECRET_KEY_BASE"] || credentials.secret_key_base || secrets.secret_key_base (called by Rails.application.secret_key_base in production) returns nil?

Rails.application.secret_key_base[0..31] will break right?

Let's add a rescue, which generates a random key on production just to keep the app running and show an error on development.


raise "Unable to fetch secret key base. Please set it in your credentials or environment variables\n" \
"For more information check https://docs.avohq.io/3.0/encryption-service.html#secret-key-base"
Rails.application.secret_key_base[0..31]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Rails.application.secret_key_base[0..31]
Rails.application.secret_key_base[0..31]
rescue
# Avoid breaking in production
# All features relying on encryption will not work properly without a configured secret key base
return SecureRandom.random_bytes(32) if Rails.env.production?
raise "Unable to fetch secret key base. Please set it in your credentials or environment variables\n" \
"For more information check https://docs.avohq.io/3.0/encryption-service.html#secret-key-base"

@jpawlyn
Copy link
Contributor Author

jpawlyn commented Jul 23, 2024

Thank you for the contribution @jpawlyn!

What if ENV["SECRET_KEY_BASE"] || credentials.secret_key_base || secrets.secret_key_base (called by Rails.application.secret_key_base in production) returns nil?

Rails.application.secret_key_base[0..31] will break right?

Let's add a rescue, which generates a random key on production just to keep the app running and show an error on development.

Looks like Rails takes care of this scenario - please see https://github.com/rails/rails/blob/a17aa6376d7974e38af90a2fe28e548b201baa63/railties/lib/rails/application/configuration.rb#L529

And when I ran my Rails 7.1.3.4 app server in production mode without a secret key base configured, puma never started due to the error:

3.3.0/gems/railties-7.1.3.4/lib/rails/application.rb:658:in `validate_secret_key_base': Missing `secret_key_base` for 'production' environment, set this string with `bin/rails credentials:edit` (ArgumentError)

@adrianthedev adrianthedev merged commit d3e51a9 into avo-hq:main Jul 24, 2024
23 checks passed
@adrianthedev
Copy link
Collaborator

Looks great! Thanks @jpawlyn!

@Paul-Bob Paul-Bob added the Chore label Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants