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

Improve logic for handling conditionals within shell interpolation #1214

Merged
merged 2 commits into from May 25, 2018

Conversation

Projects
None yet
2 participants
@JacobEvelyn
Contributor

JacobEvelyn commented May 22, 2018

Fixes #1205

@JacobEvelyn

This comment has been minimized.

Contributor

JacobEvelyn commented May 22, 2018

I wasn't sure if this should also be done for SQL injection. Thoughts?

@presidentbeef

I believe this also needs a test case for one of the values being dangerous?

@@ -143,7 +143,11 @@ def dangerous? exp
next if SAFE_VALUES.include? e
next if shell_escape? e
if node_type? e, :or, :evstr, :dstr
if node_type? e, :if
if res = dangerous?(e.values[1..-1])

This comment has been minimized.

@presidentbeef

presidentbeef May 22, 2018

Owner

I would prefer

dangerous?(e.then_clause) or dangerous(e.else_clause)

This comment has been minimized.

@JacobEvelyn

JacobEvelyn May 22, 2018

Contributor

Ah neat, didn't know about those methods. Will change!

This comment has been minimized.

@JacobEvelyn

JacobEvelyn May 23, 2018

Contributor

Actually, this doesn't seem to work, I believe because dangerous? calls each_sexp on its argument and those clauses aren't "high-level" enough in the tree to have that work as we want. Feel free to fiddle around with the code in case there's a way to do it that I missed though!

@JacobEvelyn

This comment has been minimized.

Contributor

JacobEvelyn commented May 23, 2018

I just pushed a new commit which adds positive tests for this.

@presidentbeef presidentbeef merged commit dc53ef7 into presidentbeef:master May 25, 2018

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codeclimate/diff-coverage 100% (90% threshold)
Details
codeclimate/total-coverage 94% (0.0% change)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented May 25, 2018

I wasn't sure if this should also be done for SQL injection. Thoughts?

It already is: https://github.com/presidentbeef/brakeman/blob/master/lib/brakeman/checks/check_sql.rb#L454

@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented May 25, 2018

Thanks! 🍿

@JacobEvelyn

This comment has been minimized.

Contributor

JacobEvelyn commented May 25, 2018

Ah great, I missed that when looking through check_sql.rb. Thanks!

@JacobEvelyn JacobEvelyn deleted the JacobEvelyn:conditional-interpolation branch May 25, 2018

Repository owner locked and limited conversation to collaborators Jul 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.