Skip to content
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

Add security checks #1385

Merged
merged 2 commits into from
Dec 17, 2020
Merged

Add security checks #1385

merged 2 commits into from
Dec 17, 2020

Conversation

dmentipl
Copy link
Contributor

@dmentipl dmentipl commented Dec 7, 2020

Addresses #1382

Run bandit and safety as part of CI. Bandit finds common security issues in Python code. Safety checks installed dependencies for know security vulnerabilities.

If bandit/safety find any issues the tests will not fail. However, the output is written to the log (in GitHub Actions) for inspection.

Testing:

  • Ran tests and they passed OK
  • Added new tests for the new feature(s)

CLA

  • If a new developer, signed up to CLA

@codecov
Copy link

codecov bot commented Dec 7, 2020

Codecov Report

Merging #1385 (e676700) into master (ecbe0da) will increase coverage by 0.10%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1385      +/-   ##
==========================================
+ Coverage   96.19%   96.29%   +0.10%     
==========================================
  Files          86       89       +3     
  Lines        7301     7476     +175     
==========================================
+ Hits         7023     7199     +176     
+ Misses        278      277       -1     
Impacted Files Coverage Δ
improver/metadata/probabilistic.py 100.00% <0.00%> (ø)
improver/wxcode/weather_symbols.py 98.48% <0.00%> (ø)
improver/wxcode/wxcode_decision_tree.py 100.00% <0.00%> (ø)
improver/synthetic_data/set_up_test_cubes.py 100.00% <0.00%> (ø)
improver/wxcode/wxcode_decision_tree_global.py 100.00% <0.00%> (ø)
improver/calibration/reliability_calibration.py 100.00% <0.00%> (ø)
...semble_copula_coupling/ensemble_copula_coupling.py 99.01% <0.00%> (ø)
improver/convection.py
improver/calculate_sleet_prob.py
improver/precipitation_type/convection.py 100.00% <0.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecbe0da...e676700. Read the comment docs.

Copy link
Contributor

@tjtg tjtg left a comment

Choose a reason for hiding this comment

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

All looks good.
Bandit has found a few assert statements in non-test code. I'll create another issue to tackle those.

@tjtg tjtg requested a review from benfitzpatrick December 7, 2020 23:13
@dmentipl dmentipl mentioned this pull request Dec 15, 2020
1 task
@tjtg tjtg added the good first review This pull request is simpler to review label Dec 15, 2020
@benfitzpatrick
Copy link
Contributor

Thank you @dmentipl! Looks good. I'm half wondering if bandit should just fail - what do you think?

@dmentipl
Copy link
Contributor Author

No worries, @benfitzpatrick!

We certainly could fail with bandit. However, it does pick up things like using assert, which is a low severity issue. We could add the -ll flag which reports on "medium" severity and higher, e.g. ignoring use of assert.

What do you and @tjtg think?

@tjtg
Copy link
Contributor

tjtg commented Dec 16, 2020

I'm half wondering if bandit should just fail - what do you think?

We should make it so that bandit fails if it identifies issues, but not right now.
There are a few existing issues identified by bandit (see #1386) and those existing issues would cause all CI jobs to be marked as failing until they get addressed. The flags to only fail on medium or high severity issues won't help here as one of the currently identified issues is high severity.

@tjtg tjtg merged commit b1ae691 into metoppv:master Dec 17, 2020
MoseleyS pushed a commit to MoseleyS/improver that referenced this pull request Aug 22, 2024
* Add safety and bandit to tests

* Add name to CONTRIBUTING.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first review This pull request is simpler to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants