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

Fix config.read_encrypted_secrets deprecation warning quoting #51739

Merged
merged 1 commit into from
May 6, 2024
Merged

Fix config.read_encrypted_secrets deprecation warning quoting #51739

merged 1 commit into from
May 6, 2024

Conversation

viralpraxis
Copy link
Contributor

@viralpraxis viralpraxis commented May 5, 2024 •
edited

This commit deprecated config.read_encrypted_secrets but, surprisingly, deprecation used back-ticks instead of normal quoting (which leads to excuting string payload via shell).

Motivation / Background

pry(main)> Rails::VERSION::STRING
"7.2.0.alpha"
pry(main)> Rails.application.config.read_encrypted_secrets
Errno::ENOENT: No such file or directory - config.read_encrypted_secrets

Detail

If I'm not missing anything, normal quotes should be used.

Additional information

After fix it works as expected:

[1] pry(main)> Rails.application.config.read_encrypted_secrets
ActiveSupport::DeprecationException: DEPRECATION WARNING: config.read_encrypted_secrets is deprecated and will be removed in Rails 7.3. (called from __pry__ at (pry):1)

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 May 5, 2024
akhilgkrishnan
akhilgkrishnan reviewed May 5, 2024
View reviewed changes
railties/lib/rails/application/configuration.rb Outdated
@@ -350,14 +350,13 @@ def enable_reloading=(value)
end

def read_encrypted_secrets
Rails.deprecator.warn(`config.read_encrypted_secrets is deprecated and will be removed in Rails 7.3.`)
Rails.deprecator.warn("config.read_encrypted_secrets is deprecated and will be removed in Rails 7.3.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Rails.deprecator.warn("config.read_encrypted_secrets is deprecated and will be removed in Rails 7.3.")
Rails.deprecator.warn("`config.read_encrypted_secrets` is deprecated and will be removed in Rails 7.3.")

We can highlight config.read_encrypted_secrets as a code snippet

Copy link
Member

Choose a reason for hiding this comment

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

See my review comment, I think the community consensus is against backquotes in error/warning messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced backquotes with single quotes

railties/lib/rails/application/configuration.rb Outdated
end

def read_encrypted_secrets=(value)
Rails.deprecator.warn(`config.read_encrypted_secrets is deprecated and will be removed in Rails 7.3.`)
Rails.deprecator.warn("config.read_encrypted_secrets is deprecated and will be removed in Rails 7.3.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Rails.deprecator.warn("config.read_encrypted_secrets is deprecated and will be removed in Rails 7.3.")
Rails.deprecator.warn("`config.read_encrypted_secrets` is deprecated and will be removed in Rails 7.3.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Applied separately to keep it a single-commit PR

@viralpraxis viralpraxis force-pushed the fix-read-ecrypted-secrets-deprecation-warning-quoting branch 2 times, most recently from 9ee9a04 to fbd4955 Compare May 5, 2024 09:51
akhilgkrishnan
akhilgkrishnan reviewed May 5, 2024
View reviewed changes
railties/lib/rails/application/configuration.rb Outdated
end

def read_encrypted_secrets=(value)
Rails.deprecator.warn(`config.read_encrypted_secrets is deprecated and will be removed in Rails 7.3.`)
Rails.deprecator.warn("`config.read_encrypted_secrets=` is deprecated and will be removed in Rails 7.3.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Rails.deprecator.warn("`config.read_encrypted_secrets=` is deprecated and will be removed in Rails 7.3.")
Rails.deprecator.warn("`config.read_encrypted_secrets` is deprecated and will be removed in Rails 7.3.")

typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure if config.read_encrypted_secrets (without =) was intentional is this case, since actually config.read_encrypted_secrets= is called. Do you think warning with config.read_encrypted_secrets in both cases is more appropriate?

Copy link
Member

Choose a reason for hiding this comment

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

I think keeping the = in this case is reasonable.

flavorjones
flavorjones suggested changes May 5, 2024
View reviewed changes
Copy link
Member

@flavorjones flavorjones left a comment

Choose a reason for hiding this comment

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

I think the backquotes should be removed entirely or replaced with single-quotes.

There was a recent ruby-core discussion about error messages and the consensus was that backticks in messages often confuse parsers (e.g., markdown) when pasted into forums and other sites: https://bugs.ruby-lang.org/issues/16495#note-32

@viralpraxis
Fix config.read_encrypted_secrets deprecation warning quoting
f36ad6a 
Commit 0c76f17 deprecated `Rails::Application::Configuration#read_encrypted_secrets`
but, suprisingly, deprecation used backticks instead of normal quoting.
@viralpraxis viralpraxis force-pushed the fix-read-ecrypted-secrets-deprecation-warning-quoting branch from fbd4955 to f36ad6a Compare May 5, 2024 14:39
@flavorjones flavorjones added the ready PRs ready to merge label May 5, 2024
@flavorjones flavorjones mentioned this pull request May 5, 2024
Closed
4 tasks
@flavorjones
Copy link
Member

Note that I've opened a PR with a related rubocop style cop enabled: #51741

@akhilgkrishnan
Copy link
Member

https://bugs.ruby-lang.org/issues/16495#note-32

Thanks @flavorjones for sharing this

@yahonda yahonda merged commit f6fd15c into rails:main May 6, 2024
4 checks passed
flavorjones added a commit to flavorjones/rails that referenced this pull request May 6, 2024
@flavorjones
Enable Style/CommandLiteral cop
6db0887 
PR rails#49624 contained commit 0c76f17 which mistakenly used backticks
instead of normal string quotes. This is easy to miss, but risks
introducing a security vulnerability into the codebase.

Forcing the use of `%x()` should make it more obvious visually where
the command literals are.

See related rails#51739
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flavorjones flavorjones flavorjones approved these changes

@akhilgkrishnan akhilgkrishnan akhilgkrishnan approved these changes

Assignees
No one assigned
Labels
railties ready PRs ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@viralpraxis @flavorjones @akhilgkrishnan @yahonda