Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd support for RuboCop config pre-processing #1809
Conversation
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-hq/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
| @@ -22,6 +22,10 @@ def parse_inherit_from(config) | |||
| end | |||
| end | |||
|
|
|||
| def parse(content) | |||
| super(ERB.new(content).result) | |||
This comment has been minimized.
This comment has been minimized.
gylaz
Aug 4, 2020
Member
What are the security implications of doing this? Can any arbitrary code be executed via ERB?
This comment has been minimized.
This comment has been minimized.
joehorsnell
Aug 4, 2020
Author
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?
This comment has been minimized.
This comment has been minimized.
joehorsnell
Aug 12, 2020
Author
FYI @gylaz, in case you hadn't seen it, I opened an issue with RuboCop to ask about this.
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.
joehorsnell commentedAug 4, 2020
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?