Please consider requiring opt-in for some of the new subprocess rules

[ofek]

The ones I think should be disabled by default:

• https://bandit.readthedocs.io/en/latest/plugins/b603_subprocess_without_shell_equals_true.html
• https://bandit.readthedocs.io/en/latest/plugins/b604_any_other_function_with_shell_equals_true.html
• https://bandit.readthedocs.io/en/latest/plugins/b606_start_process_with_no_shell.html
• https://bandit.readthedocs.io/en/latest/plugins/b607_start_process_with_partial_path.html

For example, the documented textbook case for S603 is subprocess.check_output(['/bin/ls', '-l']). That is literally perfect usage of subprocess, it cannot be made better other than simply outright banning the use of subprocess.

Pretty much all our repositories fail now with many examples but I'll just give one:

src/ddqa/utils/git.py:61:17: S603 `subprocess` call: check for execution of untrusted input
src/ddqa/utils/git.py:61:17: S607 Starting a process with a partial executable path

    def capture(self, *args) -> str:
        import subprocess

        try:
            process = subprocess.run(
                ['git', *args],
                cwd=str(self.path),
                stdout=subprocess.PIPE,
                stderr=subprocess.STDOUT,
                encoding='utf-8',
                check=True,
            )
        except subprocess.CalledProcessError as e:
            message = f'{str(e)[:-1]}:\n{e.output}'
            raise OSError(message) from None

        return process.stdout

[evanrittenhouse]

A similar subprocess problem appears in #4045 - opt-out's not a bad idea, IMO, but I'll wait for someone else to confirm that it's appropriate.

[ofek]

Has there been any update on this? We're still capping the version to avoid these rules.

[charliermarsh]

Sorry @ofek, I'll make a decision on this today.

[charliermarsh]

A couple observations, to put some of my internal dialogue down on "paper":

1. While these rules do arguably "ban" subprocess calls, I think the intention is that they force you to explicitly allow any uses of subprocess via a deliberate noqa or nosec comment.
2. There are likely projects in which that level of care is useful (or even required), and projects in which it's not.
3. Bandit has a parallel "severity" system, and these are generally low-severity rules. We don't support severities. (I don't know that adding severity support necessarily solves this problem.)
4. We don't have a good mechanism for making these rules "opt-in", and I think it would be confusing if they weren't enabled via --select S, since there's no precedent for that. \cc @MichaReiser
5. Flake8 does have a system for this, in that it doesn't enable any 9XX rules by default. So, e.g., flake8-bugbear's B905 rule isn't enabled with --select B -- you have to explicitly --select B905. So one solution would be to respect 9XX rules, and re-code these to the 900-level.
6. Removing these rules entirely is another option. It does mean that some projects that would like to have them enforced won't be able to enforce them, and it's also not zero-cost, since some users have now upgraded and (e.g.) added noqa pragmas to ignore specific usages. Those pargmas will be automatically removed via RUF100, but just acknowledging that removing these rules (or disabling them even when S is selected) would cause some churn.

Given these observations, I feel a bit torn. I'd probably rather remove these rules than make them opt-in via a special mechanism. Can I ask why you don't ignore these rules, if they're too strict for your project(s)?

P.S. Separately, is B604 in the same league as the others? That one seems the most reasonable to me (and is MEDIUM severity, AFAICT).

[ofek]

Can I ask why you don't ignore these rules, if they're too strict for your project(s)?

I can do that but I opened this issue because I don't think it was known that such calls are effectively banned by default now. If you're now aware and okay with that then feel free to close this!

[charliermarsh]

One solution I am considering here (which goes beyond this specific case) is adding a "pedantic" or "opinionated" rule concept, for rules that require explicit opt-in via select. So, you'd be required to select S603, and the rule wouldn't turn on via select = ["S"].

[evanrittenhouse]

@charliermarsh would that be a new RuleGroup variant?

[charliermarsh]

@evanrittenhouse - Yes. The main downside though is it will be confusing and surprising to users that (e.g.) S603 isn't enabled when select = ["S"]. This is fine for nursery, since it's intentionally experimental. But I'm not sure how to resolve that in general.

[MichaReiser]

What's the main motivation for keeping the same name? Is it so that users can copy-paste their configuration without any changes? Or is it mainly for discoverability (which could be solved by maintaining a mapping table between rules), or are there other reasons?

[charliermarsh]

Do you mean, why keep these rules under Bandit?

[MichaReiser]

Do you mean, why keep these rules under Bandit?

Yes, why keep the S603 name

[charliermarsh]

It may not be the correct decision, but the arguments in favor would be:

1. Discoverability: it's listed with the other Bandit rules, and shares a prefix that people associate with "security".
2. Portability: people can use their existing Flake8 configuration without having to understand that the code has changed. # noqa: S603 also continues to work without issue.