Disable individual tests

[mikespallino]

This PR allows for disabling specific tests by ID (e.g. B602, B607). It also supports using test name (e.g. subprocess_popen_with_shell_equals_true) as requested in #418 .

the nosec comment behaves nicely with others and can be present anywhere in a string of comments.

If nosec is used without specific tests, it acts as it did before by blanket ignoring all tests for that line.
If nosec is used with specific tests and a test not indicated in the comments is triggered, the line will be caught and reported.

[particledecay]

Would love to see this make it in. It's especially useful as a guide in the code for others, because seeing # nosec in a large codebase likely encourages other developers to simply use the same strategy when bandit complains, rather than skip a single check.

We use the pre-commit framework and have bandit in use with it, so skipping one check on one line is way better than skipping all checks for a single line, or one check for the entire run.

[sbrunner]

Isn't cleaner to have nosec: B602 or as pylint nosec: disable=B602?

[mikespallino]

@sbrunner I think it's probably best to follow your first suggestion and be in line with noqa comments and use a colon. Using a nosec comment is meant for disabling the tests already, so I don't see the need for including the word disabled like pylint. If the comment was actually bandit instead of nosec I could see your point though because we could potentially do multiple things for bandit besides just disabling tests.

@lukehinds or @ericwb thoughts?

[pdunnigan]

+1 request a review on this PR

[naltun]

Devs, please rereview this PR.

Thanks @mikespallino for the patch.

[mjeg]

👍 please add this, it would help us out.

[jenstroeger]

Second this PR, I was looking for this exact feature! @lukehinds @ericwb @ghugo @sigmavirus24 can you please review and merge?

[sigmavirus24]

This is presently not mergeable. I'm happy to review once it's up-to-date

[mikespallino]

Should be ready for review now @sigmavirus24

[sigmavirus24]

Why are we adding these here? We don't report nosec here from what I can see this file, we only use that to exclude those lines from checks

[mikespallino]

I sort of shoehorned this in here because I couldn't think of any existing structures that were better suited for tracking these until we make it to metric collection. The trouble being that we check for test failures in this module, but we report the metrics separately and I needed a way to tie these results over to metrics so that they could be reported. This is complicated by the way that this method is called within BanditNodeVisitor, so here I'm pretty much calculating these metrics on the fly while visiting the AST.

Very open to ideas on changing this around.

[sigmavirus24]

So our tester class has other attributes on it. In all of the visitor functions, we update scores but we could also update metrics too because we have the tester and the metrics gatherer:

bandit/bandit/core/node_visitor.py

Lines 35 to 46 in e0a12a9

	self.tester = b_tester.BanditTester(
	self.testset, self.debug, nosec_lines)
	# in some cases we can't determine a qualified name
	try:
	self.namespace = b_utils.get_module_qualname_from_path(fname)
	except b_utils.InvalidModulePath:
	LOG.info('Unable to find qualified name for module: %s',
	self.fname)
	self.namespace = ""
	LOG.debug('Module qualified name: %s', self.namespace)
	self.metrics = metrics

So if we changed our tester to store these metrics you care about, perhaps in a metrics = {} default dictionary, we could update metrics from there before/after lines like

bandit/bandit/core/node_visitor.py

Line 78 in e0a12a9

self.update_scores(self.tester.run_tests(self.context, 'FunctionDef'))

and add a method to the tester to "zero" them after we've consumed them.

[mikespallino]

I ended up just passing metrics into BanditTester's constructor when we instantiate it in BanditNodeVisitor's constructor so we can just call the metric function there directly, is this okay? I originally did it this way to avoid changing any return types or function signatures.

[sigmavirus24]

So some of these comments I meant to post weeks ago but are still valid I think. Some may be duplicates because the GitHub UI is confusing in this matter and I can't tell how to remove old duplicates

[sigmavirus24]

So our tester class has other attributes on it. In all of the visitor functions, we update scores but we could also update metrics too because we have the tester and the metrics gatherer:

bandit/bandit/core/node_visitor.py

Lines 35 to 46 in e0a12a9

	self.tester = b_tester.BanditTester(
	self.testset, self.debug, nosec_lines)
	# in some cases we can't determine a qualified name
	try:
	self.namespace = b_utils.get_module_qualname_from_path(fname)
	except b_utils.InvalidModulePath:
	LOG.info('Unable to find qualified name for module: %s',
	self.fname)
	self.namespace = ""
	LOG.debug('Module qualified name: %s', self.namespace)
	self.metrics = metrics

So if we changed our tester to store these metrics you care about, perhaps in a metrics = {} default dictionary, we could update metrics from there before/after lines like

bandit/bandit/core/node_visitor.py

Line 78 in e0a12a9

self.update_scores(self.tester.run_tests(self.context, 'FunctionDef'))

and add a method to the tester to "zero" them after we've consumed them.