polling2 icon indicating copy to clipboard operation
polling2 copied to clipboard

Don't use assert for validation

Open tucked opened this issue 3 years ago • 1 comments

https://github.com/ddmee/polling2/blob/b9244a1e8a1aa8fb4e034a1cac1cdd06a680f266/polling2.py#L166-L171

$ bandit polling2.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 3.8.10
[node_visitor]	WARNING	Unable to find qualified name for module: polling2.py
Run started:2022-10-14 22:09:04.481341

Test results:
>> Issue: [B101:assert_used] Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
   Severity: Low   Confidence: High
   CWE: CWE-703 (https://cwe.mitre.org/data/definitions/703.html)
   Location: polling2.py:166:4
   More Info: https://bandit.readthedocs.io/en/1.7.4/plugins/b101_assert_used.html
165	
166	    assert (timeout is not None or max_tries is not None) or poll_forever, \
167	        ('You did not specify a maximum number of tries or a timeout. Without either of these set, the polling '
168	         'function will poll forever. If this is the behavior you want, pass "poll_forever=True"')
169	
170	    assert not ((timeout is not None or max_tries is not None) and poll_forever), \

--------------------------------------------------
>> Issue: [B101:assert_used] Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
   Severity: Low   Confidence: High
   CWE: CWE-703 (https://cwe.mitre.org/data/definitions/703.html)
   Location: polling2.py:170:4
   More Info: https://bandit.readthedocs.io/en/1.7.4/plugins/b101_assert_used.html
169	
170	    assert not ((timeout is not None or max_tries is not None) and poll_forever), \
171	        'You cannot specify both the option to poll_forever and max_tries/timeout.'
172	
173	    kwargs = kwargs or dict()

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

Code scanned:
	Total lines of code: 161
	Total lines skipped (#nosec): 0
	Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

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

Consider raising ValueError instead.

tucked avatar Oct 15 '22 00:10 tucked

hi @tucked thanks for raising this, I have raise a PR for this that should fix this issue. I'm going to still use assertionerror's in-case anyone is catching those. Though I think that's probably not likely, but you never know.

However, I'll use if statements instead of assert as that fixes bandit's complaint because if statemetns won't be optimised away in particularly runtime configurations.

Hope this satisfies you? If you want, have a look or comment on the PR. Cheers, Donal

ddmee avatar Oct 15 '22 16:10 ddmee