Skip to content

Conversation

KevinHock
Copy link
Collaborator

@KevinHock KevinHock commented Feb 24, 2017

Also, we should add pylint to pre-commit hooks.

@KevinHock KevinHock changed the title [style]pretty imports and README [style]pretty imports and README + Fixed Travis CI :D Feb 24, 2017
@KevinHock
Copy link
Collaborator Author

KevinHock commented Feb 24, 2017

Current pyt notes:

DONE Fix 'sql/sqli.py' test
Add directory option from JunkHacker/bandit
JSON output file

@KevinHock KevinHock mentioned this pull request Feb 25, 2017
@KevinHock KevinHock changed the title [style]pretty imports and README + Fixed Travis CI :D [style]beautiful relative imports and README + Fixed Travis CI :D Feb 25, 2017
@KevinHock KevinHock changed the title [style]beautiful relative imports and README + Fixed Travis CI :D [style]beautiful relative imports + Edited README + Fixed Travis CI :D Feb 25, 2017
@KevinHock KevinHock changed the title [style]beautiful relative imports + Edited README + Fixed Travis CI :D Beautiful relative imports + Edited README + Fixed Travis CI :D Feb 25, 2017
Copy link
Contributor

@Thalmann Thalmann left a comment

Choose a reason for hiding this comment

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

67d2b1f:
A lot of the stuff pylint does is great.
But as you can see in the github_search module pylint does not track what names are in the globals. This means when going from module.class_name import to from module import class_name .

Stefan and i agreed on that we want PyT to be able to do what pylint does but better. As PyT actually tracks the global names so we can manage the above problem.
More on that in a task i will define.

import requests
import repo_runner
from reaching_definitions_taint import ReachingDefinitionsTaintAnalysis
from repo_runner import add_repo_to_csv, NoEntryPathError, Repo
Copy link
Contributor

Choose a reason for hiding this comment

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

This one breaks the github_search module as there is a Repo defined in this module aswell.
This means we have two Repo definitions in globals so the last imported overwrites the first.

@@ -209,7 +210,7 @@ def scan_github(search_string, start_date, analysis_type, analyse_repo_func, csv
Languages.python, repo)
s = SearchCode(q)
if s.results:
r = repo_runner.Repo(repo.url)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the line we want back.

@@ -209,7 +210,7 @@ def scan_github(search_string, start_date, analysis_type, analyse_repo_func, csv
Languages.python, repo)
s = SearchCode(q)
if s.results:
r = repo_runner.Repo(repo.url)
r = Repo(repo.url)
Copy link
Contributor

Choose a reason for hiding this comment

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

This Repo is referencing the definition of Repo in this module and not the one in the repo_runner.

scan_github('flask', ReachingDefinitionsTaintAnalysis)
exit()
q = Query(SEARCH_REPO_URL, 'flask')
s = SearchRepo(q)
for repo in s.results[:3]:
q = Query(SEARCH_CODE_URL, 'app = Flask(__name__)', Languages.python, repo)
s = SearchCode(q)
r = repo_runner.Repo(repo.url)
r = Repo(repo.url)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, I will change it.

@Thalmann
Copy link
Contributor

Thalmann commented Feb 26, 2017

997796d
The additions to the README is great, we definitely need more of this. And some stuff from our master thesis can probably be used.
Regarding the changes where we run PyT as a module, why do you think this is better?

@KevinHock
Copy link
Collaborator Author

KevinHock commented Feb 26, 2017

It's necessary for the relative imports to work, I don't love having to use -m but the relative imports are worth it IMO and there's no longer any sys ../pyt hacks.

Regarding the switch to __main__.py from pyt.py and test.py, that's so we can do e.g. -m pyt instead of pyt.pyt

I'm on my phone so I don't have the succinct stackoverflow link I had yesterday but David Beazley's awesome talk may convince you, around 41 minutes in https://www.youtube.com/watch?v=0oTh1CXRaQ0&t=2490

[edit]
Here are some other links of relative import problems needing the -m http://stackoverflow.com/questions/11536764/how-to-fix-attempted-relative-import-in-non-package-even-with-init-py http://stackoverflow.com/questions/72852/how-to-do-relative-imports-in-python/73149#73149

README.md Outdated

Clone the project into the directory

`git clone https://github.com/python-security/pyt.git`

Choose a reason for hiding this comment

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

This will make a pyt/pyt dir. You can clone right from ~ and it'll create a pyt dir.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I realize pyt/pyt is awkward, but I want python3 -m venv ~/somefolder/ that's not ~ or the Git Repository itself to hold the env files. Do you think I should I change it to mkdir ~/somefolder to make it less confusing?

Choose a reason for hiding this comment

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

Naw it works, just wanted to make sure you were accounting for it. You could change the README so that it's clear that the directory for the venv doesn't matter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point :)

README.md Outdated
`pip install -r requirements.txt`

`pip list` sample output
```

Choose a reason for hiding this comment

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

Want an extra blank line here

README.md Outdated
`pip list` sample output
```
gitdb (0.6.4)

Choose a reason for hiding this comment

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

extra lines shouldn't be needed if the triple backticks work

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't know that, thanks.

import os
import sys

sys.path.insert(0, os.path.abspath('../pyt'))

Choose a reason for hiding this comment

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

Great to get rid of this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

@@ -1,5 +1,5 @@
"""Thos module contains a base class for the analysis component used in PyT."""

Choose a reason for hiding this comment

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

fix typo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do

pyt/base_cfg.py Outdated
@@ -166,12 +166,12 @@ def __init__(self, label, left_hand_side, right_hand_side_variables, ast_node, *
right_hand_side_variables(list[str]): A list of variables on the right hand side.
line_number(Optional[int]): The line of the expression the Node represents.
"""
super(ReturnNode, self).__init__(label, left_hand_side, ast_node, right_hand_side_variables, line_number=line_number, path=path)
super(ReturnNode, self).__init__(label, left_hand_side, ast_node, right_hand_side_variables, line_number=line_number, path=path)

Choose a reason for hiding this comment

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

Why not use python3 super syntax?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TIL :) Thanks Graz

@Thalmann
Copy link
Contributor

Great talk, i have watched it before.
But as this is the first time i am packaging a python program i was following the pypi guide to get it there.
I have some local commits with that, but if this method with main.py is possible to get PyT on pypi i will support the changes.
Is this possible? :)

@KevinHock
Copy link
Collaborator Author

KevinHock commented Feb 28, 2017

Okay, that is the last commit (barring a fix from feedback.) That I will make, this PR is not getting any bigger :)

re:"PyT on pypi" I do not know but I will look into it.

@Thalmann
Copy link
Contributor

Thalmann commented Mar 2, 2017

I think it will not be a problem or this is definitely not a step in the wrong direction in regards to getting PyT on pypi :)

@Thalmann Thalmann merged commit 797adbf into python-security:master Mar 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants