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

Removed false positive for redirect_to in Rails 4 #1029

Merged
merged 3 commits into from Jun 23, 2017

Conversation

Projects
None yet
2 participants
@mario-areias
Contributor

mario-areias commented Apr 13, 2017

Fixes #1028

Not sure if this PR is correct, this is my first contribution to Breakeman. Hopefully is good enough to start a discussion and then move it to the right direction.

Rails 4 has changed how to get the hash of an ActionController::Parameter.
If the user needs the all hash parameters it needs to use the method to_unsafe_hash or to_unsafe_h.
Brakeman now recognizes those method as safe as long as they are followed by :only_path

More details: https://github.com/rails/rails/blob/4-2-stable/actionpack/CHANGELOG.md#rails-420-december-20-2014

mario-areias added some commits Apr 13, 2017

Removed false positive for redirect_to in Rails 4
Rails 4 has changed how to get the hash of an ActionController::Parameter.
If the user needs the all hash parameters it needs to use the method to_unsafe_hash or to_unsafe_h.
Brakeman now recognizes those method as safe as long as they are followed by :only_path

More details: https://github.com/rails/rails/blob/4-2-stable/actionpack/CHANGELOG.md#rails-420-december-20-2014
@presidentbeef

Hey Mário! Sorry this took so long.

Changes look pretty good! Thank you for the nice tests. My only concern is the code checking for to_unsafe_hash - please take a look.

Show outdated Hide outdated lib/brakeman/util.rb
@@ -124,6 +124,18 @@ def hash_access hash, key
nil
end
def call_has_param call, key

This comment has been minimized.

@presidentbeef

presidentbeef May 18, 2017

Owner

I believe this method is equivalent to

call? arg and call? arg.target and node_type? arg.target.target, :params

I don't think it should be a general purpose method living in Util.

@presidentbeef

presidentbeef May 18, 2017

Owner

I believe this method is equivalent to

call? arg and call? arg.target and node_type? arg.target.target, :params

I don't think it should be a general purpose method living in Util.

This comment has been minimized.

@mario-areias

mario-areias May 19, 2017

Contributor

Hum. Not sure if I quite understood what you meant. Where do we check if it's calling the function :to_unsafe_hash in your example? In my example, I am using the key parameter to do that, but I am cannot see something similar in yours. Could you please elaborate a bit so I can proper fix it? 😄

@mario-areias

mario-areias May 19, 2017

Contributor

Hum. Not sure if I quite understood what you meant. Where do we check if it's calling the function :to_unsafe_hash in your example? In my example, I am using the key parameter to do that, but I am cannot see something similar in yours. Could you please elaborate a bit so I can proper fix it? 😄

This comment has been minimized.

@presidentbeef

presidentbeef May 20, 2017

Owner

Yes, sorry. You can check with #method.

Something like:

if call? arg and call? arg.target and node_type? arg.target.target, :params
  arg.target.method == :to_unsafe_hash or arg.target.method == :to_unsafe_h
end

This is just an example. It would probably be nicer to use some variables in there to clarify what the different pieces are.

@presidentbeef

presidentbeef May 20, 2017

Owner

Yes, sorry. You can check with #method.

Something like:

if call? arg and call? arg.target and node_type? arg.target.target, :params
  arg.target.method == :to_unsafe_hash or arg.target.method == :to_unsafe_h
end

This is just an example. It would probably be nicer to use some variables in there to clarify what the different pieces are.

This comment has been minimized.

@mario-areias

mario-areias May 24, 2017

Contributor

Sorry for the late reply. That's what I tried to do:

def call_has_param call, key
    target = arg.target
    method = target.method
    
    return call? arg and call? target and node_type? target.target, :params and method == key
  end

But when I run the tests I get this error:

ect.rb:126: void value expression (SyntaxError)
... call? arg and call? target and node_type? target.target, :p...
...                               ^
rake aborted!
Command failed with status (1): [ruby test/test.rb --pride...]

There are two different node_type methods declared in brakeman. One in Util and one in Sexp. The Util one returns a type, but the Sexp one doesn't. So I tried to enforce to use the Util method by doing this: Util.node_type? but I still get the same error.

I also moved the method call_has_param method to the check_redirect file as you suggested. I have no ideas in how to fix that. Do you have any hints about this issue?

@mario-areias

mario-areias May 24, 2017

Contributor

Sorry for the late reply. That's what I tried to do:

def call_has_param call, key
    target = arg.target
    method = target.method
    
    return call? arg and call? target and node_type? target.target, :params and method == key
  end

But when I run the tests I get this error:

ect.rb:126: void value expression (SyntaxError)
... call? arg and call? target and node_type? target.target, :p...
...                               ^
rake aborted!
Command failed with status (1): [ruby test/test.rb --pride...]

There are two different node_type methods declared in brakeman. One in Util and one in Sexp. The Util one returns a type, but the Sexp one doesn't. So I tried to enforce to use the Util method by doing this: Util.node_type? but I still get the same error.

I also moved the method call_has_param method to the check_redirect file as you suggested. I have no ideas in how to fix that. Do you have any hints about this issue?

This comment has been minimized.

@presidentbeef

presidentbeef May 30, 2017

Owner

This is not related to the return value of any method. It's a syntax error because return has different precedence rules. Just remove return.

@presidentbeef

presidentbeef May 30, 2017

Owner

This is not related to the return value of any method. It's a syntax error because return has different precedence rules. Just remove return.

This comment has been minimized.

@mario-areias

mario-areias Jun 4, 2017

Contributor

Damn. Sorry, such a newbie mistake. I fixed it. It's almost there. Almost all tests are working. Except by this one: Rails4Tests#test_zero_errors [test/test.rb:89]:

The stack trace is this:

Unexpected warning found: [{:error=>"Expected call or attrasgn or safe_call or safe_attrasgn or super or zsuper or result but given s(:const, :User)", :backtrace=>["/home/user/brakeman/lib/brakeman/checks/check_redirect.rb:124:in `call_has_param'", "/home/user/brakeman/lib/brakeman/checks/check_redirect.rb:119:in `use_unsafe_hash_method?'", "/home/user/brakeman/lib/brakeman/checks/check_redirect.rb:111:in `only_path?'", "/home/user/brakeman/lib/brakeman/checks/check_redirect.rb:38:in `process_result'", "/home/user/brakeman/lib/brakeman/checks/check_redirect.rb:27:in `block in run_check'", "/home/user/brakeman/lib/brakeman/checks/check_redirect.rb:26:in `each'", "/home/user/brakeman/lib/brakeman/checks/check_redirect.rb:26:in `run_check'", "/home/user/brakeman/lib/brakeman/checks.rb:195:in `run_a_check'", "/home/user/brakeman/lib/brakeman/checks.rb:131:in `block (2 levels) in actually_run_checks'"]}].
Expected: 0
  Actual: 1

Not sure what this really means, but the error first line of error is method = target.method from the call_has_params method. Don't know how to fix that.

Thanks for your guidance, btw :)

@mario-areias

mario-areias Jun 4, 2017

Contributor

Damn. Sorry, such a newbie mistake. I fixed it. It's almost there. Almost all tests are working. Except by this one: Rails4Tests#test_zero_errors [test/test.rb:89]:

The stack trace is this:

Unexpected warning found: [{:error=>"Expected call or attrasgn or safe_call or safe_attrasgn or super or zsuper or result but given s(:const, :User)", :backtrace=>["/home/user/brakeman/lib/brakeman/checks/check_redirect.rb:124:in `call_has_param'", "/home/user/brakeman/lib/brakeman/checks/check_redirect.rb:119:in `use_unsafe_hash_method?'", "/home/user/brakeman/lib/brakeman/checks/check_redirect.rb:111:in `only_path?'", "/home/user/brakeman/lib/brakeman/checks/check_redirect.rb:38:in `process_result'", "/home/user/brakeman/lib/brakeman/checks/check_redirect.rb:27:in `block in run_check'", "/home/user/brakeman/lib/brakeman/checks/check_redirect.rb:26:in `each'", "/home/user/brakeman/lib/brakeman/checks/check_redirect.rb:26:in `run_check'", "/home/user/brakeman/lib/brakeman/checks.rb:195:in `run_a_check'", "/home/user/brakeman/lib/brakeman/checks.rb:131:in `block (2 levels) in actually_run_checks'"]}].
Expected: 0
  Actual: 1

Not sure what this really means, but the error first line of error is method = target.method from the call_has_params method. Don't know how to fix that.

Thanks for your guidance, btw :)

This comment has been minimized.

@presidentbeef

presidentbeef Jun 4, 2017

Owner

That means target is not a call sexp. Check first with if call? target, etc.

@presidentbeef

presidentbeef Jun 4, 2017

Owner

That means target is not a call sexp. Check first with if call? target, etc.

end
def use_unsafe_hash_method? arg
return call_has_param(arg, :to_unsafe_hash) || call_has_param(arg, :to_unsafe_h)

This comment has been minimized.

@presidentbeef

presidentbeef May 18, 2017

Owner

I'd pull the call_has_params logic (as noted below) into here.

@presidentbeef

presidentbeef May 18, 2017

Owner

I'd pull the call_has_params logic (as noted below) into here.

This comment has been minimized.

@mario-areias

mario-areias May 19, 2017

Contributor

Sure, I will do that.

@mario-areias

mario-areias May 19, 2017

Contributor

Sure, I will do that.

@mario-areias

This comment has been minimized.

Show comment
Hide comment
@mario-areias

mario-areias May 19, 2017

Contributor

Thanks for reviewing this! Unfortunately, I didn't quitie understand the change you asked me to do it. If you could elaborate a bit, it would be great so I can fix it properly!

Contributor

mario-areias commented May 19, 2017

Thanks for reviewing this! Unfortunately, I didn't quitie understand the change you asked me to do it. If you could elaborate a bit, it would be great so I can fix it properly!

@mario-areias

This comment has been minimized.

Show comment
Hide comment
@mario-areias

mario-areias Jun 6, 2017

Contributor

@presidentbeef thanks for your patience. I think now it's good to go!

Contributor

mario-areias commented Jun 6, 2017

@presidentbeef thanks for your patience. I think now it's good to go!

@presidentbeef presidentbeef merged commit 3e6ab81 into presidentbeef:master Jun 23, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Jun 23, 2017

Owner

Thank you for your patience!

Owner

presidentbeef commented Jun 23, 2017

Thank you for your patience!

@mario-areias mario-areias deleted the mario-areias:redirect_false_positive branch Jun 27, 2017

Repository owner locked and limited conversation to collaborators Aug 7, 2017

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