search

securesauce / precaution-beta

Precaution provides a simple, automated code review for GitHub projects by running code linters with a security focus on pull requests.
Other
1 stars 0 forks source link

Changed code several times in PR, and marked success when shouldn't #142

Closed ericwb closed 5 years ago

ericwb commented 5 years ago

Describe the bug Something in the precaution scan is messing up when multiple commits are being added to a PR. On the 3rd or so update of a file, Precaution all the sudden reported Success, when it should still have been fail.

To Reproduce https://github.com/ericwb/bandit/pull/2/checks

Expected behavior I still expect errors since not all problems were resolved. Namely in this case, it no longer shows the errors on using requests module with verify=False (line 896)

Screenshots

Additional context

ericwb commented 5 years ago

I believe the issue is there is a syntax error in the code:

browne-a02:travel-preapproval browne$ python test.py 
  File "test.py", line 193
    def Connect(host='localhost', port=443, user='root', pwd,
               ^
SyntaxError: non-default argument follows default argument

But Precaution shouldn't report overall status as Success for syntax errors. So still problem in the reporting here.

MVrachev commented 5 years ago

Until the third commit in your example you have a lot of issues - 20 to be exact: https://github.com/ericwb/bandit/pull/2/commits/f3b41ef88a427a336d561dff3a3fa9ed4b4387c7 the same amount of issues I found running Bandit locally.

Then when you look at the weak_crypto file from the fourth commit you don't get any errors by Precaution and the reason for that is because Bandit has a problem scanning the file "weak_crypto".

image

Overall our problem is that we don't get any feedback from Bandit when it has problems with parsing the AST.

MVrachev commented 5 years ago

It looks like this is not only a problem to code scanned by Bandit but also about code scanned by Gosec. This is my code: image

when I run go run I get: image

You expect that Precaution will catch this but it did not.

MVrachev commented 5 years ago

The problem is not that from more than 3 commits Precaution scan screws up but the problem is that Precaution doesn't catch problems from the ASTs used by Bandit and Gosec.

I created a new issue about it https://github.com/vmware/precaution/issues/144 and I think we can close this issue.

ericwb commented 5 years ago

@MVrachev I don't know why we need another issue for this bug. In my second comment I clearly state root cause related to the syntax error.

MVrachev commented 5 years ago

I just wanted to clarify the bug in the PR message.

MVrachev commented 5 years ago

Fixed by: https://github.com/vmware/precaution/pull/145 and https://github.com/vmware/precaution/pull/154