-r Recursive option #129

omergunal · 2018-05-13T08:43:09Z

Issue: #127
There is a few steps for completing this PR. Now we can get all ".py" files in directory and exclude some files with "-x" option.

omergunal · 2018-05-13T08:44:36Z

pyt/__main__.py

+    excluded_files = args.excluded_paths
+    test = discover_files(directory_path, excluded_files)
+
+    print(test)        


just to see if it works

KevinHock · 2018-06-06T04:25:51Z

Hi @omergunal, thanks for the soup! :D

Can you merge master into this branch and then resolve the merge conflicts? 👍

omergunal · 2018-06-06T11:30:47Z

No problem :) , ok i will do it

omergunal · 2018-06-06T11:58:22Z

in usage.py "filepath" is must required. should we do optional? because if we use "-r" we do not need it

KevinHock · 2018-06-07T00:41:49Z

Can you also add

    parser.add_argument(
        'targets', metavar='targets', type=str, nargs='*',
        help='source file(s) or directory(s) to be tested'
    )

KevinHock · 2018-06-07T00:50:53Z

Let's do this https://github.com/PyCQA/bandit/blob/master/bandit/cli/main.py#L153-L160 and then remove -f :)

KevinHock · 2018-06-07T00:55:59Z

I realize I'm totally changing my mind from what I said before, "This will enable a user to just give -r /path/to/files instead of -f file one at a time." but this seems cleaner.

KevinHock · 2018-06-07T00:57:26Z

i.e. -r means we recursively search directories, and is a bool. Whereas the targets, (positional arguments) pyt foo.py controllers/will be the targets we analyze.

omergunal · 2018-06-07T01:00:53Z

You mean, we will delete "-f" option and use "-r" for both single file scan and directory scan.And if user will not use any parameter, it will scan one file. Is that correct?

KevinHock · 2018-06-07T00:35:43Z

pyt/usage.py

+        action='store',
+        default='',
+        help='Separate files with commas'
+        )


De-dent )

KevinHock · 2018-06-07T00:50:09Z

pyt/usage.py

@@ -91,7 +91,18 @@ def _add_optional_group(parser):
        action='store_true',
        help='do not skip lines with # nosec comments'
    )
-
+    optional_group.add_argument(


Maybe make it

parser.add_argument( '-r', '--recursive', dest='recursive', action='store_true', help='find and process files in subdirectories' )

KevinHock · 2018-06-07T00:51:39Z

pyt/__main__.py

@@ -30,6 +30,19 @@
 )


+def discover_files(directory_path, excluded_files):
+    file_list = []


Nit: We mostly use list() everywhere in assignments in the codebase, just for consistency.

KevinHock · 2018-06-07T00:59:06Z

pyt/__main__.py

+            if os.path.splitext(fullpath)[1] == '.py' and fullpath.split("/")[-1] not in excluded_list:
+                file_list.append(fullpath)
+
+    return(file_list)


Nit: just for consistency, you can do return included_files (and I guess rename files to included_files)

KevinHock · 2018-06-07T01:14:36Z

re:"You mean, we will delete -f option and use -r for both single file scan and directory scan.And if user will not use any parameter, it will scan one file. Is that correct?"

Delete -f
Use targets for both file and directory scan.
Use -r for doing something like

def discover_files(targets, excluded_files, recursion):
    included_files = list()
    excluded_list = excluded_files.split(",")

    for target in targets:
        if target.endswith('.py'):
            included_files.append(target)
        else: 
            for root, dirs, files in os.walk(target):
                for f in files:
                    fullpath = os.path.join(root, f)
                    if os.path.splitext(fullpath)[1] == '.py' and fullpath.split("/")[-1] not in excluded_list:
                        included_files.append(fullpath)
                if not recursion:
                    break

    return included_files

KevinHock · 2018-06-07T01:20:40Z

(just updated the code, should be better now.)

omergunal · 2018-06-07T11:55:02Z

it seems good about returning "included_list". also "-x " parameter is available.

KevinHock · 2018-06-09T21:36:16Z

pyt/__main__.py

@@ -39,6 +57,14 @@ def main(command_line_args=sys.argv[1:]):
    elif args.trim_reassigned_in:
        ui_mode = UImode.TRIM

+
+
+    targets = args.targets


I think it might be more DRY if you did

files = discover_files( args.targets, args.excluded_paths, args.recursive )

KevinHock · 2018-06-09T21:37:55Z

pyt/usage.py

+        default='',
+        help='Separate files with commas'
+    )
+    optional_group.add_argument(


I guess targets will be part of _add_required_group b/c it's replacing -f files

KevinHock

Almost there, looking good :D

KevinHock · 2018-06-10T20:14:08Z

pyt/__main__.py

-        directory = os.path.dirname(path)
-    project_modules = get_modules(directory)
-    local_modules = get_directory_modules(directory)
+    for path in files:


Before this for loop, you can have a vulnerabilities = list(), and then do

vulnerabilities.append(find_vulnerabilities( cfg_list, ui_mode, args.blackbox_mapping_file, args.trigger_word_file, nosec_lines ))

I think it seems bad

there is no problem at the moment. why we should change like this?

Does it find vulnerabilities in all the files, or just the last file (i.e. last iteration of the loop)? If I'm reading it write I think it might do e.g. vulnerabilites = kitmap_vulns...then finally vulnerabilities = a.py_vulns, and only report the last list.

(As an aside, it seems strange it's not printing out the vulnerability info, and just seems to print the object.)

You are right, it taking last item on the list. have you idea for fix?

I think the fix is having a list outside of the for loop and adding the vulnerabilities of each file to it. The code from my first comment should do it, although now that I think about it it's probably extend and not append.

^Ahh, this was it! 👍 Just change append to extend and it'll all work! :D

KevinHock · 2018-06-10T20:16:01Z

pyt/__main__.py

        )

+        if args.baseline:


You can de-dent this, if args.baseline: as only one call to get_vulnerabilities_not_in_baseline, with the vulnerabilities of every file will work.

omergunal · 2018-06-16T15:15:24Z

pyt/__main__.py

    )
+    vulnerabilities = list()


i created list before loop as you said. the same problem about last item is still continue.

Hmm, That's odd, I'll checkout/test your code later today to try and find the issue. 👍

That's really weird, I'll look more in-depth on Monday 👍

I checked out your branch and it was append vs. extend

KevinHock

Super close, just the de-dent and append vs. extend and I think that's mostly it :)

KevinHock · 2018-06-19T01:37:17Z

pyt/__main__.py


-    if args.baseline:


You can keep this, but just de-dent it so that we only trim once.

KevinHock · 2018-06-19T01:54:01Z

So I looked at the tests that were failing and the Mock stuff

You can do e.g.

 class MainTest(BaseTestCase):
+    @mock.patch('pyt.__main__.discover_files')
     @mock.patch('pyt.__main__.parse_args')
     @mock.patch('pyt.__main__.find_vulnerabilities')
     @mock.patch('pyt.__main__.text')
-    def test_text_output(self, mock_text, mock_find_vulnerabilities, mock_parse_args):
+    def test_text_output(self, mock_text, mock_find_vulnerabilities, mock_parse_args, mock_discover_files):
         mock_find_vulnerabilities.return_value = 'stuff'
         example_file = 'examples/vulnerable_code/inter_command_injection.py'
         output_file = 'mocked_outfile'
 
+        mock_discover_files.return_value = [example_file]
         mock_parse_args.return_value = mock.Mock(
             autospec=True,
-            filepath=example_file,
             project_root=None,
             baseline=None,
             json=None,

and the same for the other tests.

This makes it so that in the tests, we don't really ever call discover_files, but instead "mock" it, and just use the return value that we want to.

omergunal · 2018-06-19T10:50:57Z

pyt/__main__.py

-    )
+        initialize_constraint_table(cfg_list)
+        analyse(cfg_list)
+        vulnerabilities.extend(find_vulnerabilities(


Look good to you?

No, there are no vulnerability in a.py b.py and c.py but it printing from xss.py

I didn't figure out the bug yet, gonna look more tomorrow 😁 This is harder than expected to track down

ok, i fixed it. its about vulnerabilities = list() location

KevinHock

Can you write tests for discover_files when you get a chance? 👍

KevinHock · 2018-06-20T01:28:31Z

pyt/__main__.py

+                            included_files.append(fullpath)
+        else:
+            if target not in excluded_list:
+                included_files.append(targets[0])


So if targets is a list of files, e.g. python -m pyt examples/vulnerable_code/command_injection.py examples/vulnerable_code/XSS.py, then discover_files will return the first file N times. (Where N is the len of targets.)

KevinHock · 2018-06-20T01:37:04Z

pyt/__main__.py

+
+    for target in targets:
+        if os.path.isdir(target):
+            if recursive:


So having if recursive: here it will make it so that if you don't have -r then you won't search directories.

You can change it to:

def discover_files(targets, excluded_files, recursive=False): included_files = list() excluded_list = excluded_files.split(",") for target in targets: if os.path.isdir(target): for root, dirs, files in os.walk(target): for f in files: fullpath = os.path.join(root, f) if os.path.splitext(fullpath)[1] == '.py' and fullpath.split("/")[-1] not in excluded_list: included_files.append(fullpath) if not recursive: break else: if target not in excluded_list: included_files.append(target) return included_files

omergunal · 2018-06-20T12:25:12Z

pyt/__main__.py

+        args.recursive
+    )
+    for path in files:
+        vulnerabilities = list()


i added the list inside to loop

Its ok now

So the bug I found yesterday, or more accurately the thing I don't understand 😕 , is that find_vulnerabilities returns the vulnerabilities for all the files previously analyzed, as if find_vulnerabilities knows all the vulnerabilities found for the other files we've looked at, how does it know this? 😱

So as far as the PR, you can change it to what you had originally, i.e. vulnerabilities = find_vulnerabilities(..), sorry I misunderstood the code, I'll still look into the reason why the code does this though.

Aha, figured it out, so I knew constraint_table etc. were global variables that keep state, and that they could be the culprit if they were used weirdly, however it is due to FrameworkAdaptor https://github.com/python-security/pyt/blob/master/pyt/web_frameworks/framework_adaptor.py#L88 adding all the past CFGs to the list :) I'll add a comment to __main__.py about it after we merge this PR

KevinHock

Almost there :)

KevinHock · 2018-06-21T01:29:18Z

pyt/__main__.py

+        if os.path.isdir(target):
+                for root, dirs, files in os.walk(target):
+                    for f in files:
+                        if not recursive:


It's important for the if not recursive: to be after the

fullpath = os.path.join(root, f) if os.path.splitext(fullpath)[1] == '.py' and fullpath.split("/")[-1] not in excluded_list: included_files.append(fullpath)

this is so that we only iterate through the for f in files: once. i.e. just one-level of depth and not recursively.

Ok, i did it

KevinHock · 2018-06-21T01:30:57Z

pyt/__main__.py

-            vulnerabilities,
-            args.baseline
-        )
+            vulnerabilities = get_vulnerabilities_not_in_baseline(


Thank you for de-denting the if args.baseline: You can de-dent the vulnerabilities = get_vulnerabilities_not_in_baseline( too, 👍

KevinHock · 2018-06-21T01:32:18Z

pyt/__main__.py

+
+    for target in targets:
+        if os.path.isdir(target):
+                for root, dirs, files in os.walk(target):


I think this line, for root, dirs, files in os.walk(target): is indented one more level than it has to be.

i will try to do better for returning "included_files"

KevinHock

LGTM, just make the tests pass and I'll merge 👍 (Feel free to write tests for discover_files if you'd like to though.)

KevinHock · 2018-06-22T01:26:31Z

pyt/__main__.py

+    excluded_list = excluded_files.split(",")
+    for target in targets:
+        if os.path.isdir(target):
+                for root, dirs, files in os.walk(target):


Nit: You can de-dent from line 38 to line 44

KevinHock · 2018-06-22T01:27:02Z

pyt/__main__.py


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

+


Nit: You can delete this newline

omergunal · 2018-06-22T20:02:02Z

and its done. i will write test for discover_files later

KevinHock

So happy to merge 🎊 🎈 🎉 🎂

…ll versions, add travis commands to tox so this does not happen again

omergunal added 5 commits May 10, 2018 16:43

new args

2ebe595

added recursive args

ef3a21d

Update __main__.py

38be6e2

Update __main__.py

759f632

Created discover_files() function

ed38dbb

omergunal commented May 13, 2018

View reviewed changes

KevinHock self-requested a review June 6, 2018 04:22

Merge branch 'master' into patch-4

fcf4638

omergunal added 2 commits June 6, 2018 14:55

added recursive option

3ac883c

discover_files

e246104

KevinHock reviewed Jun 7, 2018

View reviewed changes

omergunal added 3 commits June 7, 2018 14:50

added recursive, targets

2cbac72

update discover_files()

7875c82

removed file_list

ca0b2d7

KevinHock reviewed Jun 9, 2018

View reviewed changes

omergunal added 3 commits June 10, 2018 15:06

"targets" must be required

d9db9dd

created loop for discover_files()

c35ae81

new params

9c54d8c

KevinHock reviewed Jun 10, 2018

View reviewed changes

omergunal added 3 commits June 16, 2018 18:05

Update __main__.py

40c0f8f

Update __main__.py

42759f0

Merge branch 'master' into patch-4

5931faf

omergunal commented Jun 16, 2018

View reviewed changes

KevinHock reviewed Jun 19, 2018

View reviewed changes

pyt/__main__.py Outdated

if args.baseline:

Copy link

Collaborator

KevinHock Jun 19, 2018

Choose a reason for hiding this comment

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

You can keep this, but just de-dent it so that we only trim once.

omergunal added 2 commits June 19, 2018 13:43

changed func. and added baseline

5546c3d

new parameters for discover_files

8d1d805

omergunal commented Jun 19, 2018

View reviewed changes

omergunal added 2 commits June 20, 2018 03:18

test_valid_args_but_no_targets()

35b8001

edited expected values

2e4d07a

KevinHock reviewed Jun 20, 2018

View reviewed changes

changed vulnerabilities list location

0c6b082

omergunal commented Jun 20, 2018

View reviewed changes

omergunal added 2 commits June 20, 2018 15:26

Update usage_test.py

ae84a44

Update usage_test.py

1944b4a

KevinHock reviewed Jun 21, 2018

View reviewed changes

KevinHock added cool important labels Jun 21, 2018

changed location of "recursive control"

ba3d438

KevinHock approved these changes Jun 22, 2018

View reviewed changes

omergunal added 4 commits June 22, 2018 22:54

Update usage.py

6a25e25

de-dent some lines

f42d283

test_no_args

c7b2f73

test_no_args passed

2afc177

KevinHock approved these changes Jun 23, 2018

View reviewed changes

KevinHock merged commit 853300b into python-security:master Jun 23, 2018

KevinHock added a commit that referenced this pull request Jun 23, 2018

Fix Travis after #129 merge, add mock to requirements-dev and unpin a…

cf393c9

…ll versions, add travis commands to tox so this does not happen again

-r Recursive option #129

-r Recursive option #129

Conversation

omergunal commented May 13, 2018 • edited Loading

Choose a reason for hiding this comment

KevinHock commented Jun 6, 2018

omergunal commented Jun 6, 2018

omergunal commented Jun 6, 2018

KevinHock commented Jun 7, 2018

KevinHock commented Jun 7, 2018

KevinHock commented Jun 7, 2018

KevinHock commented Jun 7, 2018 • edited Loading

omergunal commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinHock commented Jun 7, 2018 • edited Loading

KevinHock commented Jun 7, 2018

omergunal commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinHock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omergunal Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinHock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinHock commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinHock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omergunal Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

KevinHock Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinHock Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

KevinHock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinHock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omergunal commented Jun 22, 2018

KevinHock left a comment

Choose a reason for hiding this comment

omergunal commented May 13, 2018 •

edited

Loading

KevinHock commented Jun 7, 2018 •

edited

Loading

KevinHock commented Jun 7, 2018 •

edited

Loading

omergunal Jun 11, 2018 •

edited

Loading

omergunal Jun 20, 2018 •

edited

Loading

KevinHock Jun 21, 2018 •

edited

Loading

KevinHock Jun 21, 2018 •

edited

Loading