Problem Statement

From classes:

Pick two scanners appropriate for the language you used in Lab 1b: https://github.com/mre/awesome-static-analysis

Run them, and compare the results. Your deliverable should be a short write up of the results they produced and whether your code has those issues (true positives) and which issues you think are false positives, and why.

Scanners

We decided to run with three scanners to hit a few different areas, namely: Linting, Security and Complexity. The three we chose were pylint, bandit, and radon, respectively.

pylint

Output

$ pylint croptool/
No config file found, using default configuration
************* Module croptool.crop
C:  1, 0: Missing module docstring (missing-docstring)
C: 23, 0: Missing class docstring (missing-docstring)
C: 28, 4: Missing method docstring (missing-docstring)
C: 28, 4: Class method has_direction should have 'cls' as first argument (bad-classmethod-argument)
C: 32, 0: Missing class docstring (missing-docstring)
C: 37, 4: Missing method docstring (missing-docstring)
C: 49, 4: Missing method docstring (missing-docstring)
C:  4, 0: standard import "from enum import Enum" should be placed before "from PIL import Image" (wrong-import-order)
************* Module croptool.test.test_crop
C:  1, 0: Missing module docstring (missing-docstring)
C:  9, 0: Missing class docstring (missing-docstring)
C: 10, 4: Method name "test_crop_with_explicit_destination" doesn't conform to snake_case naming style (invalid-name)
C: 10, 4: Missing method docstring (missing-docstring)
C: 17, 8: Variable name "x" doesn't conform to snake_case naming style (invalid-name)
C: 17,11: Variable name "y" doesn't conform to snake_case naming style (invalid-name)
C: 21, 4: Missing method docstring (missing-docstring)
C: 30, 4: Missing method docstring (missing-docstring)
C: 37, 8: Variable name "x" doesn't conform to snake_case naming style (invalid-name)
C: 37,11: Variable name "y" doesn't conform to snake_case naming style (invalid-name)
C: 41, 4: Missing method docstring (missing-docstring)
C: 46, 4: Missing method docstring (missing-docstring)
C:  5, 0: third party import "from click.testing import CliRunner" should be placed before "from croptool.cli import crop" (wrong-import-order)
C:  6, 0: third party import "from PIL import Image" should be placed before "from croptool.cli import crop" (wrong-import-order)
************* Module croptool.cli.crop
C:  1, 0: Missing module docstring (missing-docstring)

-----------------------------------
Your code has been rated at 6.93/10

Discussion

We agree with these results, as the majority of the problems are style-based (docstrings, import orders, variable naming). This would be something we would include in our CI process and could have corrected these mistakes earlier to follow best practices (or the relevant PEPs for a Python project). Another alternative is to use a Python auto-styler/formatter such as black as part of a Git commit hook to avoid some of the purely stylistic reports (such as wrong-import-order). Looking further into:

C: 10, 4: Method name "test_crop_with_explicit_destination" doesn't conform to snake_case naming style (invalid-name)

This seemed odd that it didn't "conform to snake_case" but in actuality it is just pylint expecting method names to be 30 char or less.

bandit

Output

$ bandit -r croptool/
[main]	INFO	profile include tests: None
[main]	INFO	profile exclude tests: None
[main]	INFO	cli include tests: None
[main]	INFO	cli exclude tests: None
[main]	INFO	running on Python 3.7.3
Run started:2019-07-04 00:08:21.285046

Test results:
>> Issue: [B108:hardcoded_tmp_directory] Probable insecure usage of temp file/directory.
   Severity: Medium   Confidence: Medium
   Location: croptool/test/test_crop.py:13
   More Info: https://bandit.readthedocs.io/en/latest/plugins/b108_hardcoded_tmp_directory.html
12	        raw_image = os.path.join(os.path.dirname(__file__), "../resources/longcrop.png")
13	        out_image = "/tmp/test_output.png"
14	        runner.invoke(crop.cli, ["square", "-s", raw_image, "-d", out_image])

--------------------------------------------------
>> Issue: [B108:hardcoded_tmp_directory] Probable insecure usage of temp file/directory.
   Severity: Medium   Confidence: Medium
   Location: croptool/test/test_crop.py:33
   More Info: https://bandit.readthedocs.io/en/latest/plugins/b108_hardcoded_tmp_directory.html
32	        raw_image = os.path.join(os.path.dirname(__file__), "../resources/rectangle.png")
33	        out_image = "/tmp/test_output.png"
34	        runner.invoke(crop.cli, ["square", "-s", raw_image, "-d", out_image])

--------------------------------------------------

Code scanned:
	Total lines of code: 105
	Total lines skipped (#nosec): 0

Run metrics:
	Total issues (by severity):
		Undefined: 0.0
		Low: 0.0
		Medium: 2.0
		High: 0.0
	Total issues (by confidence):
		Undefined: 0.0
		Low: 0.0
		Medium: 2.0
		High: 0.0
Files skipped (0):

Discussion

This was the tool we were most impressed with as it's not something we had an eye for from the get-go. We agree with the issue (B108) in that we shouldn't be using a hardcoded temporary file path, since it could lead to incorrect loading if somewhere to mount over it or otherwise cause problems in the code because that path may not be what you expect. A reasonable alternative would be to use standard library modules for accessing this information (for example, in Python, Lib/tempfile.py for the tempfile.TemporaryDirectory function).

radon

Output

$ radon cc croptool/
croptool/crop.py
    F 7:0 square - A
    M 37:4 ImageBox.center_crop - A
    M 49:4 ImageBox.transpose - A
    C 32:0 ImageBox - A
    C 23:0 Directions - A
    M 27:4 Directions.has_direction - A
    M 33:4 ImageBox.__init__ - A
croptool/test/test_crop.py
    M 21:4 TestCrop.test_cant_save_new_image - A
    C 9:0 TestCrop - A
    M 10:4 TestCrop.test_crop_with_explicit_destination - A
    M 30:4 TestCrop.test_crop_with_transposed_image - A
    M 41:4 TestCrop.test_valid_direction - A
    M 46:4 TestCrop.test_invalid_direction - A
croptool/cli/crop.py
    F 5:0 cli - A
    F 11:0 square - A

Discussion

radon doesn't think this code is terribly complex and we agree. It is nice however that we could use this tool to enforce minimum grading in our CI and catch merges that would increase complexity (and thereby potentially decrease performance) for further review.