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

Baseline support #106

Merged
merged 12 commits into from
Apr 13, 2018
Merged

Baseline support #106

merged 12 commits into from
Apr 13, 2018

Conversation

omergunal
Copy link
Contributor

@omergunal omergunal commented Apr 9, 2018

Issue 101
I coded "baseline support" script but i couldnt implement it. i am taking this error: "ValueError: I/O operation on closed file."

#python3 -m pyt -f app.py --baseline test.json
Output:

Traceback (most recent call last):
File "/usr/lib/python3.5/runpy.py", line 184, in _run_module_as_main
"main", mod_spec)
File "/usr/lib/python3.5/runpy.py", line 85, in _run_code
exec(code, run_globals)
File "/home/gunal/pyt/pyt/main.py", line 370, in
main()
File "/home/gunal/pyt/pyt/main.py", line 316, in main
compare(json.report(vulnerabilities, sys.stdout),baseline)
File "/home/gunal/pyt/pyt/formatters/json.py", line 31, in report
with fileobj:
ValueError: I/O operation on closed file.

just for checking error. it's not totally done
pyt/baseline.py Outdated
@@ -0,0 +1,27 @@
from pprint import pprint
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not totally done.

@KevinHock KevinHock self-requested a review April 10, 2018 02:23
Copy link
Collaborator

@KevinHock KevinHock left a comment

Choose a reason for hiding this comment

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

I'm sorry if it's less 'comparing json' than we originally thought,

By 'loop through vulnerabilities and return "vulnerabilities - baseline"' I mean do something like

output = list()
for vuln in vulnerabilities:
   if vuln not in baseline['vulnerabilities']:
          output.append(vuln)

inside of a function named get_vulnerabilities_not_in_baseline, which will replace compare. In this way we can have a baseline, use the default text output, and still only report the vulnerabilities that are not in the baseline.

pyt/__main__.py Outdated

if args.baseline:
baseline = args.baseline
compare(json.report(vulnerabilities, sys.stdout),baseline)
Copy link
Collaborator

Choose a reason for hiding this comment

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

So what is happening (I think) is that the previous call to report closes sys.stdout.

     if args.json:
         json.report(vulnerabilities, sys.stdout)
     else:
         text.report(vulnerabilities, sys.stdout)

then does
with fileobj: which closes it.

The solution I think is to do

if args.baseline:
    vulnerabilities = get_vulnerabilities_not_in_baseline(vulnerabilities, baseline)

before we do if args.json.

You can loop through vulnerabilities and return "vulnerabilities - baseline". This will make it work both for json output and text output.

See https://github.com/Yelp/detect-secrets/blob/b16acf1e8dc1e05366a9bfbd7ce35ed611adb94d/detect_secrets/pre_commit_hook.py#L34-L37 for an example of how we have results and then just return results - baseline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, i will edit it

@omergunal
Copy link
Contributor Author

omergunal commented Apr 11, 2018

@KevinHock i edited code, it's not complete. I have a question. Is baseline file must be json format?

Copy link
Collaborator

@KevinHock KevinHock left a comment

Choose a reason for hiding this comment

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

Looking pretty good so far, pretty close 👍 Thanks for making this :)

pyt/baseline.py Outdated
output = list()
vulnerabilities =[vuln.as_dict() for vuln in vulnerabilities]
for vuln in vulnerabilities:
if vuln not in baseline['vulnerabilities']:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can do if vuln.as_dict() not in baseline['vulnerabilities']: so that you don't need to modify json.py and it's less lines of code.

pyt/__main__.py Outdated
@@ -300,9 +308,19 @@ def main(command_line_args=sys.argv[1:]):
)
)
if args.json:
json.report(vulnerabilities, sys.stdout)
if args.baseline:
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can do if args.baseline: before the if args.json: so that you only need to do if args.baseline: and

vulnerabilities = get_vulnerabilities_not_in_baseline(
    vulnerabilities,
    args.baseline
)

once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can the baseline file be normal text? Like that:

2 vulnerabilities found:
Vulnerability 1:
File: /home/gunal/kelime_kok_ayirici/app.py
 > User input at line 98, trigger word "form[":
	 _ID = request.form['_id']
Reassigned in:
	File: /home/gunal/kelime_kok_ayirici/app.py
	 > Line 64: save_1__ID = _ID
	File: /home/gunal/kelime_kok_ayirici/app.py
	 > Line 104: temp_1__ID = _ID
	File: /home/gunal/kelime_kok_ayirici/db.py
	 > Line 64: _ID = temp_1__ID
	File: /home/gunal/kelime_kok_ayirici/app.py
	 > Line 64: _ID = _ID
File: /home/gunal/kelime_kok_ayirici/db.py
 > reaches line 80, trigger word "execute(":
	~call_4 = ret_cursor.execute(sql, (int(_IsTrue), str(_UserSuggestion), datetime.datetime.now(), int(_ID)))

Vulnerability 2:
File: /home/gunal/kelime_kok_ayirici/app.py
 > User input at line 99, trigger word "form[":
	 _IsTrue = request.form['_isTrue']
Reassigned in:
	File: /home/gunal/kelime_kok_ayirici/app.py
	 > Line 64: save_1__IsTrue = _IsTrue
	File: /home/gunal/kelime_kok_ayirici/app.py
	 > Line 104: temp_1__IsTrue = _IsTrue
	File: /home/gunal/kelime_kok_ayirici/db.py
	 > Line 64: _IsTrue = temp_1__IsTrue
	File: /home/gunal/kelime_kok_ayirici/app.py
	 > Line 64: _IsTrue = _IsTrue
File: /home/gunal/kelime_kok_ayirici/db.py
 > reaches line 80, trigger word "execute(":
	~call_4 = ret_cursor.execute(sql, (int(_IsTrue), str(_UserSuggestion), datetime.datetime.now(), int(_ID)))

if it can be we should parse it differently

Copy link
Collaborator

Choose a reason for hiding this comment

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

So the baseline file can only be JSON 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, i got it.

pyt/__main__.py Outdated
@@ -50,6 +50,7 @@
vulnerabilities_to_file
)
from .vulnerabilities import find_vulnerabilities
from .baseline import get_vulnerabilities_not_in_baseline
Copy link
Collaborator

Choose a reason for hiding this comment

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

So I haven't written a pre-commit hook yet to do this automatically, but the imports are in alphabetical order, so this should go on the line before from .constraint_table import (

@@ -0,0 +1,11 @@
import json

Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't notice this before but from looking at the Code Climate issues at the bottom of the PR, there are a couple of pep8 ones. Normally people have 2 lines between imports and the next class or function call like in expr_visitor.py for example.

@KevinHock
Copy link
Collaborator

Just FYI there is one test you'll need to change (see FAIL: test_no_args (tests.command_line_test.CommandLineTest) from the Travis output) or run make test) that just has the expected description https://github.com/python-security/pyt/blob/master/tests/command_line_test.py#L25 that can be changed because of the baseline option.

@omergunal
Copy link
Contributor Author

@KevinHock You can review it again, i think its done.

Copy link
Collaborator

@KevinHock KevinHock left a comment

Choose a reason for hiding this comment

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

Looks great 😁

@KevinHock KevinHock merged commit e3de650 into python-security:master Apr 13, 2018
@KevinHock KevinHock added the cool label Apr 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants