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

Add support for RuboCop config pre-processing #1809

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

joehorsnell
Copy link

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 pwd
is 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?

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
@@ -22,6 +22,10 @@ def parse_inherit_from(config)
end
end

def parse(content)
super(ERB.new(content).result)
Copy link
Member

Choose a reason for hiding this comment

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

What are the security implications of doing this? Can any arbitrary code be executed via ERB?

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

FYI @gylaz, in case you hadn't seen it, I opened an issue with RuboCop to ask about this.

Copy link
Author

Choose a reason for hiding this comment

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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants