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

do not allow EnvironmentVariableAccess in initializers #1229

Merged
merged 1 commit into from
Jan 11, 2024

Conversation

markokajzer
Copy link
Contributor

@markokajzer markokajzer commented Jan 9, 2024

The description of the cop states Do not access ENV directly after initialization.. However in some ways, the application is already initialized when we get to config/**/*.rb:

  1. config_for(:service) is already available to read config from service.yml
  2. Rails.application.credentials is already available to read application credentials / secrets

I think it could make sense to push people to move their config in either of these two places. If they want to access their config / secrets in a Ruby file, let's say, config/initializers/service.rb they can use credentials or config_for, no need to access ENV directly again.

Also see https://guides.rubyonrails.org/initialization.html.

I understand this is a bit nuanced, so if this is not desired, that's completely fine ✌️


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
  • If this is a new cop, consider making a corresponding update to the Rails Style Guide.

@koic
Copy link
Member

koic commented Jan 10, 2024

config/**/*.rb seems to be too broad of a match. For example, in config/boot.rb, it appears that the Rails constant cannot yet be resolved. i.e., uninitialized constant Rails (NameError)

@markokajzer
Copy link
Contributor Author

markokajzer commented Jan 10, 2024

Hmmm... Guess I was a bit too quick.

How about we limit the include to config/initializers? seems like boot.rb, environment.rb, application.rb have already run at that point and Rails has been loaded, see https://guides.rubyonrails.org/configuring.html#using-initializer-files.

After loading the framework and any gems in your application

Would that be an option?

@markokajzer markokajzer changed the title do not allow EnvironmentVariableAccess in Ruby files within config do not allow EnvironmentVariableAccess in initializers Jan 10, 2024
config/default.yml Outdated Show resolved Hide resolved
@koic
Copy link
Member

koic commented Jan 10, 2024

I have left two comments. Can you update these and squash the commits?

@markokajzer
Copy link
Contributor Author

markokajzer commented Jan 10, 2024

Updated, squashed, and added specs for the new cases 👍

@koic koic merged commit 8bcd6fa into rubocop:master Jan 11, 2024
13 checks passed
@koic
Copy link
Member

koic commented Jan 11, 2024

Thanks!

@dogweather
Copy link

dogweather commented Jun 29, 2024

I'm confused: With this rule enabled, how can a Rails app read env vars at all?

E.g., in my initializers, I ENV.fetch() env vars and create constants that I access throughout my app. This seems to be the safest way to do it.

A couple people (or maybe just one person?) has said that the Rails configuration lib can read env vars and check them at boot time. But no one has provided code showing this, so the rule was set disabled by default. I'm skeptical the lib does this: I couldn't find mention of this check-env-var-at-boot feature in the Guide.

In a nutshell, this rule seemed to be on shaky ground before this change. And now that all initializers are off-limits to ENV access under the rule, how should environment variables be access when enabled?

@markokajzer
Copy link
Contributor Author

markokajzer commented Jul 1, 2024

From the PR description:

However in some ways, the application is already initialized when we get to config/**/*.rb:

config_for(:service) is already available to read config from service.yml
Rails.application.credentials is already available to read application credentials / secrets

So what you can do is

# In config/slack.yml
default: &default
  client_id: <%= ENV["SLACK_CLIENT_ID"] %>
  client_secret: <%= ENV["SLACK_CLIENT_SECRET"] %>
# In application.rb
module MyApp
  class Application
    config.slack = config_for(:slack)
  end
end

# In rest of the app
Rails.application.config.slack.client_id

Similarly, secrets should be in credentials.yml [docs], so given

# In config/credentials.yml.enc
secret_key_base: 3b7cd72...
some_api_key: SOMEKEY
system:
  access_key_id: 1234AB

You can do

# In rest of the app
Rails.application.secrets.some_api_key
Rails.application.secrets.system.access_key_id

That way

  1. Secrets are in credentials.yml.enc
  2. Config is in config/*.yml
    • Direct ENV access only happens here (and in config/*.rb since those files are excluded)
    • You can use ENV.fetch here since it's erb templates
  3. Access to secrets happens via Rails.application.secrets
  4. Access to config happens via Rails.application.config

If you cannot or don't want to use the Rails secrets/credential mechanisms for whatever reason (we don't), you can put everything in Rails.application.config instead.

Hope that helps!

@dogweather
Copy link

dogweather commented Jul 2, 2024

Thanks @markokajzer. So if I understand it correctly, under this rule, ENV access can only happen in the config/*.yml files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants