Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Whitelist lines ending in # nosec #121

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

Merged
KevinHock merged 22 commits into python-security:master from omergunal:patch-2
Apr 28, 2018
Merged

Conversation

@omergunal
Copy link
Contributor

@omergunal omergunal commented Apr 22, 2018

Issue #108
I built nosec_lines but im not sure how to send to find_triggers and find_vulnerabilities

KevinHock reacted with hooray emoji
Copy link
Collaborator

Copy link
Contributor Author

Can you check main.py and vulnerabilities.py ?

@KevinHock KevinHock self-requested a review April 23, 2018 20:34
pyt/__main__.py Outdated
nosec_lines = set()
else:
file = open(path, "r")
lines = file.readlines()
Copy link
Collaborator

@KevinHock KevinHock Apr 22, 2018

Choose a reason for hiding this comment

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

Nice, I like this even more than .read()->.splitlines.

pyt/__main__.py Outdated
'(only JSON-formatted files are accepted)',
type=str,
default=False)
parser.add_argument('-in', '--ignore-nosec',
Copy link
Collaborator

@KevinHock KevinHock Apr 22, 2018

Choose a reason for hiding this comment

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

I kind of like how Bandit does this a little more https://github.com/openstack/bandit/blob/master/bandit/cli/main.py#L230

Copy link
Collaborator

@KevinHock KevinHock Apr 24, 2018

Choose a reason for hiding this comment

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

i.e.

 parser.add_argument(
 '--ignore-nosec', dest='ignore_nosec', action='store_true',
 help='do not skip lines with # nosec comments'
 )

looks really nice.

trigger_nodes.extend(iter(label_contains(node, trigger_words)))
if node.line_number not in nosec_lines:
trigger_nodes.extend(iter(label_contains(node, trigger_words)))
else:
Copy link
Collaborator

@KevinHock KevinHock Apr 24, 2018

Choose a reason for hiding this comment

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

The else: pass isn't needed

with open(vulnerability_files.blackbox_mapping) as infile:
blackbox_mapping = json.load(infile)
for cfg in cfg_list:

Copy link
Collaborator

@KevinHock KevinHock Apr 24, 2018

Choose a reason for hiding this comment

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

Nit: I kind of liked how the newlines were in this function.

pyt/__main__.py Outdated

analyse(cfg_list, analysis_type=analysis)

nosec_lines = set()
Copy link
Collaborator

@KevinHock KevinHock Apr 24, 2018

Choose a reason for hiding this comment

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

Can you move this code

 if args.ignore_nosec:
 nosec_lines = set()
 else:
 file = open(path, "r")
 lines = file.readlines()
 nosec_lines = set(
 lineno for
 (lineno, line) in enumerate(lines, start=1)
 if '#nosec' in line or '# nosec' in line)

to near the top, so we take into account nosec_lines at both call-sites to find_vulnerabilities.
https://github.com/omergunal/pyt/blob/fb88051e1d988d5890127ef3aa6867adf0db07de/pyt/__main__.py#L240
is a good spot, the same way UImode is set once and then passed to both call-sites https://github.com/python-security/pyt/blob/master/pyt/__main__.py#L232-L236


path = os.path.normpath(args.filepath)
cfg_list = list()
if args.ignore_nosec:
Copy link
Contributor Author

@omergunal omergunal Apr 24, 2018

Choose a reason for hiding this comment

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

i moved the code this place and deleted nosec_lines = set() .Because its already created in if-else statement.

pyt/__main__.py Outdated
elif args.trim_reassigned_in:
ui_mode = UImode.TRIM

path = os.path.normpath(args.filepath)
Copy link
Contributor Author

@omergunal omergunal Apr 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Its necessary for learning filename. i moved to above the ignore_nosec argument

Copy link
Contributor Author

@KevinHock can you check it again?

Copy link
Collaborator

So there are just a few things left, vulnerabilities.py looks good.

Can you pass nosec_lines to analyse_repo and then also to the call to find_vulnerabilities inside of that function?
Can you delete the find_vulnerabilities call in the else statement of if args.nosec_lines?

 

unnecessary codes deleted, passed nosec_lines on analyse_repo
Copy link
Contributor Author

Like that?
KevinHock reacted with thumbs up emoji

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.

LGTM

Copy link
Collaborator

Yup, shall I merge? 😀

Copy link
Contributor Author

Why not :)
KevinHock reacted with thumbs up emoji

Copy link
Collaborator

KevinHock commented Apr 25, 2018 •
edited
Loading

Oh whoops, it seems like tests are failing https://travis-ci.org/python-security/pyt/builds/371295436?utm_source=github_status&utm_medium=notification
Can you make nosec_lines default to being an empty set e.g. nosec_lines=set()? You can also delete the test_no_args (tests.command_line_test.CommandLineTest) as it's annoying lol

Copy link
Contributor Author

zzz

Looks like ok now.

Copy link
Collaborator

KevinHock commented Apr 26, 2018 •
edited
Loading

Sorry by Can you make nosec_lines default to being an empty set e.g. nosec_lines=set()? I meant

Before:

def find_vulnerabilities(
 cfg_list,
 analysis_type,
 ui_mode,
 vulnerability_files,
 nosec_lines
):

After:

def find_vulnerabilities(
 cfg_list,
 analysis_type,
 ui_mode,
 vulnerability_files,
 nosec_lines=set()
):

So that you won't have to manually pass an empty set to every find_vulnerabilities call, it makes things cleaner.

Copy link
Contributor Author

omergunal commented Apr 26, 2018 •
edited
Loading

You are right, my bad :)
I changed it

Copy link
Contributor Author

I think its ok now.
KevinHock reacted with thumbs up emoji

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.

Almost there 👍

nodes,
trigger_words
trigger_words,
nosec_lines = set()
Copy link
Collaborator

@KevinHock KevinHock Apr 28, 2018

Choose a reason for hiding this comment

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

Can you please do nosec_lines=set() instead of nosec_lines = set()? I thought find_vulnerabilities was the only function that needed this, but you're right since we do call find_triggers once from a test here

l = vulnerabilities.find_triggers(XSS1.nodes, trigger_words)
maybe since we just call it once from tests we can pass in the set in that test.

Copy link
Collaborator

@KevinHock KevinHock Apr 28, 2018

Choose a reason for hiding this comment

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

You can see some of these in the https://codeclimate.com/github/python-security/pyt/pull/121 output.

blackbox_mapping,
vulnerabilities_list
vulnerabilities_list,
nosec_lines = set()
Copy link
Collaborator

@KevinHock KevinHock Apr 28, 2018

Choose a reason for hiding this comment

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

So this shouldn't be necessary, (to make it default to empty set), we can just leave it as nosec_lines. This is b/c we only call find_vulnerabilities from tests, not this function.

from pyt.reaching_definitions_taint import ReachingDefinitionsTaintAnalysis
from pyt.vulnerabilities import find_vulnerabilities


Copy link
Collaborator

@KevinHock KevinHock Apr 28, 2018

Choose a reason for hiding this comment

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

We normally leave 2 lines between the imports and other code.

from pyt.node_types import Node
from pyt.reaching_definitions_taint import ReachingDefinitionsTaintAnalysis


Copy link
Collaborator

@KevinHock KevinHock Apr 28, 2018

Choose a reason for hiding this comment

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

Same here, sorry.

sinks,
lattice
lattice,
nosec_lines=set()
Copy link
Collaborator

@KevinHock KevinHock Apr 28, 2018

Choose a reason for hiding this comment

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

So identify_triggers( is never called in tests, so I don't think a default arg is needed.

omergunal reacted with thumbs up emoji
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.

LGTM

Copy link
Collaborator

Thanks @omergunal!
omergunal reacted with hooray emoji

@KevinHock KevinHock merged commit 76379d4 into python-security:master Apr 28, 2018
Copy link
Contributor Author

You are welcome 👍
KevinHock reacted with thumbs up emoji

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

Reviewers

@KevinHock KevinHock KevinHock approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /