Add check for zero to xrandom #3040

kinow · 2019-03-24T22:42:04Z

Prevents ZeroDivisionError in xrandom. Adds docstrings and doctests (thought a unittest for it wouldn't be really necessary as it's an example only? We were running doctests anyway for coverage...)

Cheers
Bruno

kinow · 2019-03-24T22:43:56Z

Output of python -m doctest -v lib/cylc/xtriggers/xrandom.py:

Trying:
    xrandom(0, 0)
Expecting:
    (False, {})
ok
Trying:
    import sys
Expecting nothing
ok
Trying:
    mocked_randint = lambda x, y: 10
Expecting nothing
ok
Trying:
    sys.modules[__name__].randint = mocked_randint
Expecting nothing
ok
Trying:
    xrandom(20, 0)
Expecting:
    (False, {})
ok
Trying:
    import sys
Expecting nothing
ok
Trying:
    mocked_randint = lambda x, y: 1
Expecting nothing
ok
Trying:
    sys.modules[__name__].randint = mocked_randint
Expecting nothing
ok
Trying:
    xrandom(1, 0)
Expecting:
    (True, {'COLOR': 'orange', 'SIZE': 'small'})
ok
1 items had no tests:
    xrandom
1 items passed all tests:
   9 tests in xrandom.xrandom
9 tests in 2 items.
9 passed and 0 failed.
Test passed.

kinow · 2019-03-24T22:44:52Z

I think it's not necessary to backport to 7.8.x, but happy to prepare another PR for the other branch if others prefer to include this fix there too.

hjoliver

Looks good, thanks 👍

hjoliver · 2019-03-24T23:04:53Z

@kinow - as a bug fix for existing code, this probably should be back-ported if you don't mind.

hjoliver · 2019-03-24T23:09:03Z

Codacy:

Standard pseudo-random generators are not suitable for security/cryptographic purposes.

I wonder why Codacy sees this as a new issue? (same use of random() before and after this change).

kinow · 2019-03-25T00:00:42Z

@kinow - as a bug fix for existing code, this probably should be back-ported if you don't mind.

Sure thing, pull request coming!

I wonder why Codacy sees this as a new issue? (same use of random() before and after this change).

I suspect they are simply reporting on the old issue, as if it were a new one? We could add a #noqa marker and see if it goes away, or move the new if condition to before the existing one.

hjoliver · 2019-03-25T00:10:43Z

We could add a #noqa marker ...

Can you try that? (or should it be #nosec in this case?). It's clearly not a security issue in this usage!

kinow · 2019-03-25T00:15:25Z

Roger. Before bandit reports:

kinow@kinow-VirtualBox:~/Development/python/workspace/cylc$ bandit lib/cylc/xtriggers/xrandom.py
[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 2.7.15
Run started:2019-03-25 00:12:48.244873

Test results:
>> Issue: [B311:blacklist] Standard pseudo-random generators are not suitable for security/cryptographic purposes.
   Severity: Low   Confidence: High
   Location: lib/cylc/xtriggers/xrandom.py:77
   More Info: https://bandit.readthedocs.io/en/latest/blacklists/blacklist_calls.html#b311-random
76	    results = {}
77	    satisfied = int(percent) != 0 and (1 == randint(1, 100 / int(percent)))
78	    if satisfied:

--------------------------------------------------
>> Issue: [B311:blacklist] Standard pseudo-random generators are not suitable for security/cryptographic purposes.
   Severity: Low   Confidence: High
   Location: lib/cylc/xtriggers/xrandom.py:80
   More Info: https://bandit.readthedocs.io/en/latest/blacklists/blacklist_calls.html#b311-random
79	        results = {
80	            'COLOR': COLORS[randint(0, len(COLORS) - 1)],
81	            'SIZE': SIZES[randint(0, len(SIZES) - 1)]

--------------------------------------------------
>> Issue: [B311:blacklist] Standard pseudo-random generators are not suitable for security/cryptographic purposes.
   Severity: Low   Confidence: High
   Location: lib/cylc/xtriggers/xrandom.py:81
   More Info: https://bandit.readthedocs.io/en/latest/blacklists/blacklist_calls.html#b311-random
80	            'COLOR': COLORS[randint(0, len(COLORS) - 1)],
81	            'SIZE': SIZES[randint(0, len(SIZES) - 1)]
82	        }

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

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

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

With (pushing in a commit in a bit):

diff --git a/lib/cylc/xtriggers/xrandom.py b/lib/cylc/xtriggers/xrandom.py
index ea0c48fa9..1b1b1e995 100644
--- a/lib/cylc/xtriggers/xrandom.py
+++ b/lib/cylc/xtriggers/xrandom.py
@@ -74,10 +74,11 @@ def xrandom(percent, secs=0, _=None, debug=False):
     """
     sleep(float(secs))
     results = {}
-    satisfied = int(percent) != 0 and (1 == randint(1, 100 / int(percent)))
+    satisfied = int(percent) != 0 and \
+                (1 == randint(1, 100 / int(percent)))  # nosec
     if satisfied:
         results = {
-            'COLOR': COLORS[randint(0, len(COLORS) - 1)],
-            'SIZE': SIZES[randint(0, len(SIZES) - 1)]
+            'COLOR': COLORS[randint(0, len(COLORS) - 1)],  # nosec
+            'SIZE': SIZES[randint(0, len(SIZES) - 1)]  # nosec
         }
     return (satisfied, results)

Results in:

[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 2.7.15
Run started:2019-03-25 00:14:16.326919

Test results:
	No issues identified.

Code scanned:
	Total lines of code: 54
	Total lines skipped (#nosec): 5

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

Let's wait and see what Codacy says after the commit is pushed.

kinow · 2019-03-25T00:16:07Z

Oh, and you were right, nosec, not noqa 😁 Thanks!

kinow · 2019-03-25T00:28:52Z

@hjoliver looks it still complains about the added if statement, but not about the security issue with random:

Up to standards. A positive pull request.

If that looks OK I will cherry-pick in the 7.8.x branch and prepare a pull request.

hjoliver · 2019-03-25T00:45:16Z

That's great, plz go ahead with the back-port. Thanks!

hjoliver · 2019-03-25T00:51:44Z

@dwsutherland - you've worked with external triggers recently, so can you provide 2nd review for this? (It'll only take a minute - just a small divide-by-zero bug fix, not really something that requires knowing about xtriggers actually).

lib/cylc/xtriggers/xrandom.py

hjoliver · 2019-03-25T09:50:24Z

How about this (Python 3):

random.random() < int(percent)/100

e.g. true with 20% likelihood, if a random float between 0 and 1 is less than 0.2.

dwsutherland · 2019-03-25T09:52:43Z

How about this (Python 3):
random.random() < int(percent)/100
e.g. true with 20% likelihood, if a random float between 0 and 1 is less than 0.2.

I did have a suggestion above which requires no division:

satisfied =  randint(1, 100) <= int(percent)

or

satisfied =  randint(1, 100) <= int(percent) % 100

Or am I missing something XD

dwsutherland · 2019-03-25T09:54:05Z

It also incorporates the 0

dwsutherland · 2019-03-25T10:03:41Z

Actually I do like your solution @hjoliver .. because if you remove the int() then it would work for decimals 54.35%!

hjoliver · 2019-03-25T10:03:45Z

I missed your suggestion, sorry ... but what's wrong with division?! (percent/100 is safe)

dwsutherland · 2019-03-25T10:07:21Z

I missed your suggestion, sorry ... but what's wrong with division?! (percent/100 is safe)

Yes yours is safe too! (and nothing wrong with non-zero division)

kinow · 2019-03-25T22:09:49Z

Looks like random.random() < int(percent)/100 is safe and fixes this issue and the new one found by @dwsutherland 🎉 updating pull request.

kinow · 2019-03-25T22:24:41Z

Done, ran pycodestyle and bandit before too just to make sure Travis would be happy (except for the flaky tests, of course).

And I think the code looks much nicer too! Reading the line that checks the likelihood is even easier to understand. 👏 great to have more 👀 reviewing the code.

hjoliver · 2019-03-25T23:33:57Z

Yes, shows both how difficult and how critical reviewing is! (I guess didn't bother to read that bit properly because I wrote it myself some time ago and so assumed it was right, haha 😟 ). Thanks again to @dwsutherland for checking properly (I think he has a maths background 🎉 )

hjoliver

Re-approving!

lib/cylc/xtriggers/xrandom.py

kinow · 2019-03-26T03:57:45Z

Both pull requests updated, should be ready for review if Travis-CI doesn't fail (which my indicate it wants another kick).

dwsutherland

LGTM!

hjoliver · 2019-03-26T10:07:34Z

💥

kinow self-assigned this Mar 24, 2019

kinow added the bug Something is wrong :( label Mar 24, 2019

kinow added this to the cylc-8.0a1 milestone Mar 24, 2019

kinow force-pushed the zero-proof-xrandom branch from 97b1cc4 to 350f191 Compare March 24, 2019 22:47

cylc deleted a comment Mar 24, 2019

hjoliver self-requested a review March 24, 2019 23:03

hjoliver approved these changes Mar 24, 2019

View reviewed changes

cylc deleted a comment Mar 25, 2019

kinow force-pushed the zero-proof-xrandom branch from 73d50d9 to 2df86ba Compare March 25, 2019 00:22

kinow mentioned this pull request Mar 25, 2019

Remove unnecessary shebang in Python files #3041

Closed

cylc deleted a comment Mar 25, 2019

hjoliver requested a review from dwsutherland March 25, 2019 00:48

kinow mentioned this pull request Mar 25, 2019

Add check for zero to xrandom (7.8.x backport) #3042

Merged

dwsutherland requested changes Mar 25, 2019

View reviewed changes

lib/cylc/xtriggers/xrandom.py Outdated Show resolved Hide resolved

kinow and others added 3 commits March 26, 2019 11:21

Prevent ZeroDivisionError in xrandom, add doctests

b8b2634

Add docstrings

a6127d8

Add nosec markers for bandit

9a4ff65

kinow force-pushed the zero-proof-xrandom branch from 2df86ba to 9a4ff65 Compare March 25, 2019 22:22

cylc deleted a comment Mar 25, 2019

hjoliver approved these changes Mar 25, 2019

View reviewed changes

hjoliver requested a review from dwsutherland March 25, 2019 23:36

dwsutherland reviewed Mar 26, 2019

View reviewed changes

lib/cylc/xtriggers/xrandom.py Outdated Show resolved Hide resolved

kinow mentioned this pull request Mar 26, 2019

Subprocpool: use SpooledTemporaryFile instead of TemporaryFile [7.8.x] #3028

Merged

Make percent a float

f52e1e2

dwsutherland approved these changes Mar 26, 2019

View reviewed changes

hjoliver merged commit 7c0bb8c into cylc:master Mar 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check for zero to xrandom #3040

Add check for zero to xrandom #3040

kinow commented Mar 24, 2019 •

edited by hjoliver

Loading

kinow commented Mar 24, 2019

kinow commented Mar 24, 2019

hjoliver left a comment

hjoliver commented Mar 24, 2019

hjoliver commented Mar 24, 2019

kinow commented Mar 25, 2019

hjoliver commented Mar 25, 2019

kinow commented Mar 25, 2019

kinow commented Mar 25, 2019 •

edited

Loading

kinow commented Mar 25, 2019

hjoliver commented Mar 25, 2019

hjoliver commented Mar 25, 2019 •

edited

Loading

hjoliver commented Mar 25, 2019

dwsutherland commented Mar 25, 2019

dwsutherland commented Mar 25, 2019

dwsutherland commented Mar 25, 2019 •

edited

Loading

hjoliver commented Mar 25, 2019

dwsutherland commented Mar 25, 2019 •

edited

Loading

kinow commented Mar 25, 2019

kinow commented Mar 25, 2019

hjoliver commented Mar 25, 2019

hjoliver left a comment

kinow commented Mar 26, 2019

dwsutherland left a comment

hjoliver commented Mar 26, 2019

Add check for zero to xrandom #3040

Add check for zero to xrandom #3040

Conversation

kinow commented Mar 24, 2019 • edited by hjoliver Loading

kinow commented Mar 24, 2019

kinow commented Mar 24, 2019

hjoliver left a comment

Choose a reason for hiding this comment

hjoliver commented Mar 24, 2019

hjoliver commented Mar 24, 2019

kinow commented Mar 25, 2019

hjoliver commented Mar 25, 2019

kinow commented Mar 25, 2019

kinow commented Mar 25, 2019 • edited Loading

kinow commented Mar 25, 2019

hjoliver commented Mar 25, 2019

hjoliver commented Mar 25, 2019 • edited Loading

hjoliver commented Mar 25, 2019

dwsutherland commented Mar 25, 2019

dwsutherland commented Mar 25, 2019

dwsutherland commented Mar 25, 2019 • edited Loading

hjoliver commented Mar 25, 2019

dwsutherland commented Mar 25, 2019 • edited Loading

kinow commented Mar 25, 2019

kinow commented Mar 25, 2019

hjoliver commented Mar 25, 2019

hjoliver left a comment

Choose a reason for hiding this comment

kinow commented Mar 26, 2019

dwsutherland left a comment

Choose a reason for hiding this comment

hjoliver commented Mar 26, 2019

kinow commented Mar 24, 2019 •

edited by hjoliver

Loading

kinow commented Mar 25, 2019 •

edited

Loading

hjoliver commented Mar 25, 2019 •

edited

Loading

dwsutherland commented Mar 25, 2019 •

edited

Loading

dwsutherland commented Mar 25, 2019 •

edited

Loading