Add support for RuboCop config pre-processing #1809
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RuboCop [0.83.0 (2020-05-11)][1] [added support][2] for [configuration pre-processing][3] by running the config through ERB. This PR "fixes" the Hound CI RuboCop config parser to also run the config through ERB, although I'm not familiar with the internals of Hound to know whether this will work in practice (eg what `pwd` is used?). An alternative/better approach for the future would be use RuboCop's own code (specifically [`RuboCop::ConfigLoader`][4]) to parse the config? Obviously that would be a bigger change. Thoughts? [1]: https://github.com/rubocop-hq/rubocop/blob/master/CHANGELOG.md#0830-2020-05-11 [2]: rubocop/rubocop#7920 [3]: https://docs.rubocop.org/rubocop/configuration.html#pre-processing [4]: https://github.com/rubocop-hq/rubocop/blob/4943d5005b44c61973910b77adbb5fa42209bbfd/lib/rubocop/config_loader.rb#L56
gylaz
reviewed
Aug 4, 2020
| @@ -22,6 +22,10 @@ def parse_inherit_from(config) | |||
| end | |||
| end | |||
|
|
|||
| def parse(content) | |||
| super(ERB.new(content).result) | |||
There was a problem hiding this comment.
What are the security implications of doing this? Can any arbitrary code be executed via ERB?
There was a problem hiding this comment.
What are the security implications of doing this? Can any arbitrary code be executed via ERB?
That's a fair question. Yes, is the short answer, arbitrary code can be executed. But that's also kind of the point, eg. to allow executing a shell git command to determine which files to apply a cop to dynamically.
This is the way that RuboCop itself does the pre-processing, so is at least no worse than that?
Also, RuboCop is a tool that is only intended for use at development/CI time, at which point you are executing arbitrary code anyway, in order to test it.
What specific concerns did you have @gylaz?
There was a problem hiding this comment.
FYI @gylaz, in case you hadn't seen it, I opened an issue with RuboCop to ask about this.
There was a problem hiding this comment.
Ping @gylaz - can you take a look at this please?
joehorsnell
added a commit
to bambooengineering/hound
that referenced
this pull request
Aug 6, 2020
I noticed when doing an unrelated PR (houndci#1809) that running the specs locally requires `chromedriver`. ``` Failures: 1) Account user with Stripe Customer ID Failure/Error: visit root_path Selenium::WebDriver::Error::WebDriverError: Unable to find chromedriver. Please download the server from https://chromedriver.storage.googleapis.com/index.html and place it somewhere on your PATH. More info at https://github.com/SeleniumHQ/selenium/wiki/ChromeDriver. # ./spec/support/helpers/authentication_helper.rb:10:in `sign_in_as' # ./spec/features/account_spec.rb:17:in `block (2 levels) in <top (required)>' # ./spec/support/background_jobs.rb:4:in `block (3 levels) in <top (required)>' # ./spec/support/background_jobs.rb:22:in `block in run_background_jobs_immediately' # ./spec/support/background_jobs.rb:21:in `run_background_jobs_immediately' # ./spec/support/background_jobs.rb:3:in `block (2 levels) in <top (required)>' ``` This PR simplifies local development by using [webdrivers][1] to automatically download the correct version of `chromedriver`. [1]: https://github.com/titusfortner/webdrivers
joehorsnell
added a commit
to bambooengineering/hound
that referenced
this pull request
Aug 6, 2020
I noticed when doing an unrelated PR (houndci#1809) that running the specs locally requires `chromedriver`. ``` Failures: 1) Account user with Stripe Customer ID Failure/Error: visit root_path Selenium::WebDriver::Error::WebDriverError: Unable to find chromedriver. Please download the server from https://chromedriver.storage.googleapis.com/index.html and place it somewhere on your PATH. More info at https://github.com/SeleniumHQ/selenium/wiki/ChromeDriver. # ./spec/support/helpers/authentication_helper.rb:10:in `sign_in_as' # ./spec/features/account_spec.rb:17:in `block (2 levels) in <top (required)>' # ./spec/support/background_jobs.rb:4:in `block (3 levels) in <top (required)>' # ./spec/support/background_jobs.rb:22:in `block in run_background_jobs_immediately' # ./spec/support/background_jobs.rb:21:in `run_background_jobs_immediately' # ./spec/support/background_jobs.rb:3:in `block (2 levels) in <top (required)>' ``` This PR simplifies local development by using [webdrivers][1] to automatically download the correct version of `chromedriver`. [1]: https://github.com/titusfortner/webdrivers
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RuboCop 0.83.0 (2020-05-11) added support for configuration pre-processing by
running the config through ERB.
This PR "fixes" the Hound CI RuboCop config parser to also run the config through ERB, although I'm
not familiar with the internals of Hound to know whether this will work in practice (eg what
pwdis used?).
An alternative/better approach for the future would be use RuboCop's own code (specifically
RuboCop::ConfigLoader) to parse the config? Obviously that would be a bigger change.Thoughts?