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.

Sign up for GitHub

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

Jump to bottom

Use secret_key_base from ENV or credentials when present locally #53705

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use secret_key_base from ENV or credentials when present locally #53705

wants to merge 1 commit into from
+72 −17

Conversation

p8
Copy link
Member

@p8 p8 commented Nov 22, 2024 •
edited
Loading

Motivation / Background

Since 0c76f17 a tmp/local_secret.txt is always used for Rails.config.secret_key_base in the test and development environments. Previously, ENV["SECRET_KEY_BASE"] or Rails.application.credentials.secret_key_base were used if present.

As the secret_key_base is used to generate signed IDs, it breaks Action Text attachments in development if they previously used the secret_key_base from ENV var or credentials. Accidentally deleting tmp/local_secret.txt will also break all local Action Text attachments.

The race condition in generating the local secret file, as reported in #53661, should be avoidable by setting ENV["SECRET_KEY_BASE"] or Rails.application.credentials.secret_key_base, but this currently doesn't work.

Details

When ENV["SECRET_KEY_BASE"] or Rails.application.credentials.secret_key_base is set for test or development, it should be used for the Rails.config.secret_key_base, instead of generating a tmp/local_secret.txt.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes 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.

@rails-bot rails-bot bot added the railties label Nov 22, 2024
@p8 p8 force-pushed the railties/use-local-secret-key-base-if-present branch 6 times, most recently from 3b3a992 to 339e38b Compare November 22, 2024 11:35
skipkayhil
skipkayhil reviewed Nov 27, 2024
View reviewed changes
Copy link
Member

@skipkayhil skipkayhil left a comment

Choose a reason for hiding this comment

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

The new complexity of generate_local_secret? makes me think the abstraction is wrong (especially since it only gets used in a single place).

What do you think about just making secret_key_base look more like the pre-secret removal:

def secret_key_base
  @secret_key_base || begin
    self.secret_key_base = if ENV["SECRET_KEY_BASE_DUMMY"]
      generate_local_secret
    else
      ENV["SECRET_KEY_BASE"] || Rails.application.credentials.secret_key_base || (generate_local_secret if Rails.env.local?)
    end
  end
end

railties/lib/rails/application/configuration.rb
@@ -514,7 +514,7 @@ def secret_key_base
end

def secret_key_base=(new_secret_key_base)
if new_secret_key_base.nil? && generate_local_secret?
if new_secret_key_base.nil? && Rails.env.local?
Copy link
Member

Choose a reason for hiding this comment

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

Does this still work if an app does secret_key_base = nil and has ENV["SECRET_KEY_BASE_DUMMY"] set?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, yes probably not. There isn't a test case for that now. I'll try to add it.

@p8
Copy link
Member Author

p8 commented Nov 30, 2024

The new complexity of generate_local_secret? makes me think the abstraction is wrong (especially since it only gets used in a single place).

What do you think about just making secret_key_base look more like the pre-secret removal:

Maybe that is a good direction. I'll try to update the PR.

@p8 p8 force-pushed the railties/use-local-secret-key-base-if-present branch from 339e38b to c3474dc Compare December 2, 2024 20:00
@p8
Use secret_key_base from ENV or credentials when present locally
0fb5371 
Since 0c76f17  a `tmp/local_secret.txt`
is always used for `Rails.config.secret_key_base` in the test and
development environments, where previously `ENV["SECRET_KEY_BASE"]` or
`Rails.application.credentials.secret_key_base` were used if present.

The race condition in generating the local secret file, as reported in
rails#53661, should be avoided by
setting `ENV["SECRET_KEY_BASE"]` or
`Rails.application.credentials.secret_key_base`, but this currently
doesn't work.

When ENV["SECRET_KEY_BASE"] or
`Rails.application.credentials.secret_key_base` is set for test or
development, it should be used for the `Rails.config.secret_key_base`,
instead of generating a `tmp/local_secret.txt`.
@p8 p8 force-pushed the railties/use-local-secret-key-base-if-present branch from c3474dc to 0fb5371 Compare December 2, 2024 20:01
@p8
Copy link
Member Author

p8 commented Dec 2, 2024

What do you think about just making secret_key_base look more like the pre-secret removal.

Thanks @skipkayhil ! This is much cleaner! ❤️

@frederikspang frederikspang mentioned this pull request Dec 19, 2024
Open
@huda-kh
Copy link

huda-kh commented Dec 19, 2024

When is this planned for release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skipkayhil skipkayhil skipkayhil left review comments

Assignees
No one assigned
Labels
railties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@p8 @huda-kh @skipkayhil