DodgyBear.py: Add bear implementation #2246

manankalra · 2018-01-19T17:05:08Z

This bear checks Python code for possible
dodgy looking values such as secret keys or
passwords.

Closes #836

For short term contributors: we understand that getting your commits well
defined like we require is a hard task and takes some learning. If you
look to help without wanting to contribute long term there's no need
for you to learn this. Just drop us a message and we'll take care of brushing
up your stuff for merge!

Checklist

I read the commit guidelines and I've followed
them.
I ran coala over my code locally. (All commits have to pass
individually. It is not sufficient to have "fixup commits" on your PR,
our bot will still report the issues for the previous commit.) You will
likely receive a lot of bot comments and build failures if coala does not
pass on every single commit!

After you submit your pull request, DO NOT click the 'Update Branch' button.
When asked for a rebase, consult coala.io/rebase
instead.

Please consider helping us by reviewing other peoples pull requests as well:

pick up any PR at https://coala.io/review
review it (check https://coala.io/reviewing for more info)
if you are sure that it needs work, use cobot mark wip <URL> to get it out
of the review queue.

The more you review, the more your score will grow at coala.io and we will
review your PRs faster!

newbazz

You need to add tests, always see the coverage before submitting the PR.

manankalra · 2018-01-20T14:14:30Z

@newbazz I've added the tests.
These are the results of running pytest -k DodgyBearTest --cov:

I'm getting 100% coverage here. Unable to figure out where the problem is.

newbazz · 2018-01-20T14:24:27Z

You have these lines missing.

manankalra · 2018-01-20T14:57:33Z

@newbazz Yeah, I did see that. But these are the only lines that do the required work in DodgyBear.
As far as line 30 is concerned, if you look at it - it's the return call of create_arguments() which doesn't have anything to do with Dodgy as it's a global linter bear and doesn't even accept a --path argument.
During the build, all of my tests were skipped, any idea why did that happen?

newbazz · 2018-01-20T16:40:18Z

bears/python/DodgyBear.py

+
+from coalib.bearlib.abstractions.Linter import linter
+from coalib.results.Result import Result
+from dependency_management.requirements.PipRequirement import PipRequirement


import sud be in alphabetical order.

made the changes ;)
Initially I imported them alphabetically according to the package the imports were made from.

similar changes in DodgyBearTest.py too?

changed the import order in the test file too

newbazz · 2018-01-20T16:41:00Z

bears/python/DodgyBear.py

+        return []
+
+    def process_output(self, output, filename, file):
+        for issue in json.loads(output)['warnings']:


can you add a docstring to explain what you are doing.

I don't think it's needed. Just reading the JSON stdout call from dodgy and following what https://api.coala.io/en/latest/Developers/Linter_Bears_Advanced.html#custom-processing-functions-with-process-output has. Do you still want me to add it?

yup not everyones gonna read this.

I've added the docstring.

newbazz

You have tests failing.

newbazz

Isn't dodgy a npm package? I think you need to add it in package.json in case it is a npm package. ;)

manankalra · 2018-01-22T10:29:26Z

@newbazz No. What you're talking about is https://www.npmjs.com/package/dodgy I guess.
The one I need to include is https://pypi.python.org/pypi/dodgy.
For the failing tests - they're passing locally. https://postimg.org/image/72pghz4ol/
A possible reason can be that each test needs to create, write and later on remove the test files in the coala-bears directory. Maybe this isn't allowed during the ci build. Although I'm not sure. Thoughts?
Also, I guess this issue will be closed soon as dodgy lacks argument passing capabilities and is not being maintained.

newbazz · 2018-01-22T12:11:10Z

I dont think so, in that case the appveyor should also fail but it is not the case here.

manankalra · 2018-01-22T20:43:33Z

@newbazz If you look at the build, I'm getting this error <json.decoder.JSONDecoder object at 0x7f4e81315400>, s = '', idx = 0 which is raised only when either the string being parsed is non-JSON, empty or has incompatible character encoding. In this case, s='' suggests that there was no output to parse for json.loads().
But, if I log the output that the process_output() function receives to the console manually - there is a JSON string actually. (https://postimg.org/image/6gmr9pm6d/)

newbazz · 2018-01-23T08:01:50Z

If you want this issue to be closed you can close the PR or else ping @Makman2 for clarification, i am also not able to find what is going wrong ;)

This bear checks Python code for possible dodgy looking values such as secret keys or passwords. Closes coala#836

newbazz

This bear is there for windows system as well right?

manankalra · 2018-01-31T08:34:07Z

@newbazz Yes, I guess. But I haven't tested it on windows.

Makman2 · 2018-02-22T13:53:59Z

bears/python/DodgyBear.py

+
+@linter(executable='dodgy',
+        use_stdout=True,
+        use_stderr=False,


This is default, you can just omit use_stdout and use_stderr args ;)

Makman2 · 2018-02-22T13:54:36Z

bears/python/DodgyBear.py

+    AUTHORS_EMAILS = {'[email protected]'}
+    LICENSE = 'AGPL-3.0'
+    CAN_DETECT = {'Security', 'Hardcoded Secret',
+                  'SCM Diff Check-in', 'SSH Keys'}


SCM Diff Check-in?

yeah, pushing a code diff

Makman2 · 2018-02-22T13:55:33Z

bears/python/DodgyBear.py

+
+    @staticmethod
+    def create_arguments(config_file):
+        return []


rather return an empty tuple, then Python will reference always the same object and we save a little bit of memory and performance (minimal, but still :D)

Makman2 · 2018-02-22T13:56:02Z

bears/python/DodgyBear.py

+
+    @staticmethod
+    def create_arguments(config_file):
+        return []


You can't pass files to dodgy?

Sadly, no. You cannot specify a path. It just runs for all the .py files in the current directory.
We've discussed this before, I guess.
prospector-dev/dodgy#5

Makman2 · 2018-02-22T13:56:53Z

bears/python/DodgyBear.py

+        Parses JSON stdout call and yields results
+        according to the executable output.
+        :param output:
+            string output from the executable i.e. dodgy


My opinion: These variables and API here are all described in the linter docs already, so I would only document additional parameters that are settings.

Makman2 · 2018-02-22T13:57:06Z

bears/python/DodgyBear.py

+        :param output:
+            string output from the executable i.e. dodgy
+        """
+        print(output)


debugging remains ;)

Yeah, I was trying to catch some stdout when the tests fail. No output is being returned by dodgy. But locally, it works fine.
#2246 (comment)

Makman2 · 2018-02-22T13:57:15Z

bears/python/DodgyBear.py

+        """
+        print(output)
+        issues = json.loads(output)
+        for issue in issues['warnings']:


what about other types of issues?

dodgy just returns a JSON string as output
kinda like this for every type of issue found:

{ "warnings": {.....} }

issues['warnings'] is used for that here.

Makman2 · 2018-02-22T13:58:02Z

I'm wondering why appveyor skips the test... dodgy is a pip package and it gets installed :/

Makman2 · 2018-02-22T13:58:40Z

tests/python/DodgyBearTest.py

+                         'na6d1dBmrQ3ly0B6g+pEf8ObaBriu0Fest0yZhyF8hsgueGu'
+                         'gZFXzjaAu68ib99nP17w111cbej9jUNnyfseykl5ngqrvsP3'
+                         'mGKw739z+p0vIIchHYa3jn6xW8fim/PH4gtcWC8mUXlv6XT '
+                         'random@123')


I think you should use test files :) See other tests that have functions like get_testfile_name/get_testfile_path and load_testfile

same thing - #2246 (comment)

Makman2 · 2018-02-22T13:59:25Z

tests/python/DodgyBearTest.py

+                      'secrets.py', 'ssh_private_key.py', 'ssh_public_key.py']
+        for f in test_files:
+            if f in current:
+                os.remove(f)


yeah properly using test files makes this unnecessary^^

(Btw you should always remove temp files where you created them in a test function, best is using context managers for this, having a tearDown should be rather avoided if possible.)

@Makman2 We did discuss this before.
The problem was that dodgy only runs for files in the current directory. No files can be specified. It doesn't accept absolute/relative paths to a file which should be checked.

So, we cannot use test files here. But rather create test files in the pwd, test dodgy and then remove the created files.

:(

Ahhh I remember now... Yeah sorry it's been a while. Can you maybe execute coala / the bear inside a different working directory? Maybe patching some variables?

Ya, we have a decorator which changes directory.

I've emailed the maintainer to ask them whether we can implement prospector-dev/dodgy#5 for them.

I really don't like dodgy. The regexes included only adhere to a certain kind of checks.
For example: If your file contains PASSWORD=123@xyz, it is detected; if it contains password=123@xyz, it does not. 👎

This - https://github.com/zricethezav/gitleaks

does gitleaks support individual files? if not, it is still not ideal. Create a new issue for gitleaks if you like.

Anyway, dodgy maintainer replied and said they are happy to review PRs for prospector-dev/dodgy#5 .

That gets it usable, and then this PR can be finished.

gitleaks doesn't support individual files, but does accept a path to a directory which dodgy isn't capable of. Although it works only if it's a path to a git repo, and unlike dodgy, it runs checks on the whole commit history.
Also, it is capable of running on remote repositories, and on all of the repositories of a particular user/organization at once.

manankalra · 2018-02-22T15:06:53Z

@Makman2 But the appveyor build passes.
The tests fail.
Because dodgy returns no output.
Locally, it does.

Makman2 · 2018-02-22T20:37:00Z

But the appveyor build passes.

Yeah because the tests are skipped for dodgy :3

The tests fail.
Because dodgy returns no output.

Weird, some bad installation happens maybe in the CI?

manankalra · 2018-02-23T04:38:29Z

Oh 😄
I didn't look at the appveyor jobs before. Also, if you look at the coverage - why are they different under appveyor and circle-ci? And it's 100% here - #2246 (comment) 😞

Weird, some bad installation happens maybe in the CI?

but whenever I synchronize my PR, it gets installed again, right?

Makman2 · 2018-02-23T12:19:04Z

Also, if you look at the coverage - why are they different under appveyor and circle-ci?

Because some tests are skipped on Windows, we don't require 100% coverage there. In the end we
merge code coverage together with codecov (can't see it recently in the CI, but it should be there).

but whenever I synchronize my PR, it gets installed again, right?

Yeah every new PR sync triggers a new build that runs on a clean environment. Still seems dodgy is not available, maybe the old bat vs native exe problem, Windows has problems treating bat files which work cleanly in the cmd (without providing the .bat ending) in their create-process-API.

Makman2 · 2018-02-23T12:20:40Z

maybe the old bat vs native exe problem, Windows has problems treating bat files which work cleanly in the cmd (without providing the .bat ending) in their create-process-API.

But I believe we have fixed that somehow in linter...

gitmate-bot added process/pending review size/S labels Jan 19, 2018

newbazz suggested changes Jan 20, 2018

View reviewed changes

manankalra force-pushed the issue836 branch 2 times, most recently from c56e4a4 to 371dc7a Compare January 20, 2018 22:10

newbazz reviewed Jan 21, 2018

View reviewed changes

newbazz suggested changes Jan 22, 2018

View reviewed changes

manankalra force-pushed the issue836 branch from 371dc7a to a5f38df Compare January 22, 2018 10:26

manankalra force-pushed the issue836 branch from a5f38df to 62ac1ed Compare January 26, 2018 17:03

DodgyBear.py: Add bear implementation

ab6b041

This bear checks Python code for possible dodgy looking values such as secret keys or passwords. Closes coala#836

manankalra force-pushed the issue836 branch from 62ac1ed to ab6b041 Compare January 26, 2018 17:22

newbazz reviewed Jan 30, 2018

View reviewed changes

Makman2 reviewed Feb 22, 2018

View reviewed changes

jayvdb added process/wip and removed process/pending review labels Apr 15, 2018

DodgyBear.py: Add bear implementation #2246

Are you sure you want to change the base?

DodgyBear.py: Add bear implementation #2246

Conversation

manankalra commented Jan 19, 2018 • edited Loading

Checklist

newbazz left a comment

Choose a reason for hiding this comment

manankalra commented Jan 20, 2018

newbazz commented Jan 20, 2018

manankalra commented Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

manankalra Jan 20, 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

newbazz left a comment

Choose a reason for hiding this comment

newbazz left a comment

Choose a reason for hiding this comment

manankalra commented Jan 22, 2018 • edited Loading

newbazz commented Jan 22, 2018

manankalra commented Jan 22, 2018 • edited Loading

newbazz commented Jan 23, 2018

newbazz left a comment

Choose a reason for hiding this comment

manankalra commented Jan 31, 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

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

Makman2 commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Makman2 Feb 22, 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

manankalra Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manankalra commented Feb 22, 2018 • edited Loading

Makman2 commented Feb 22, 2018

manankalra commented Feb 23, 2018 • edited Loading

Makman2 commented Feb 23, 2018 • edited Loading

Makman2 commented Feb 23, 2018

manankalra commented Jan 19, 2018 •

edited

Loading

manankalra commented Jan 20, 2018 •

edited

Loading

manankalra Jan 20, 2018 •

edited

Loading

manankalra commented Jan 22, 2018 •

edited

Loading

manankalra commented Jan 22, 2018 •

edited

Loading

Makman2 Feb 22, 2018 •

edited

Loading

manankalra Apr 18, 2018 •

edited

Loading

manankalra commented Feb 22, 2018 •

edited

Loading

manankalra commented Feb 23, 2018 •

edited

Loading

Makman2 commented Feb 23, 2018 •

edited

Loading