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

Add support for RuboCop config pre-processing #1809

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

Add support for RuboCop config pre-processing #1809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c8ae3b
Add support for RuboCop config pre-processing
joehorsnell Aug 4, 2020
dd5adc6
fixup! Add support for RuboCop config pre-processing
joehorsnell Aug 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/models/config/rubocop.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?

end

def safe_parse(content)
parse(content)
rescue Psych::Exception => exception
Expand Down
18 changes: 18 additions & 0 deletions spec/models/config/rubocop_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,24 @@
end
end

context "when the configuration uses pre-processing" do
it "runs the config through ERB" do
raw_config = <<~CONFIG
Style/Encoding:
Enabled: <%= 1 + 1 == 2 %>
<% 42 # This should be ignored and not cause an error %>
CONFIG

config = build_config(raw_config)

expect(config.content).to eq(
"Style/Encoding" => {
"Enabled" => true,
},
)
end
end

context "when the configuration uses `inherit_from`" do
it "returns the merged configuration using `inherit_from`" do
repo_config = <<~EOS
Expand Down