Potential conflicted rules between S602 and S603

[zeshuaro]

After bumping ruff from 0.0.261 to 0.0.262, I'm getting conflicted errors from the rules S602 and S603. See below for the minimal code snippet:

from subprocess import PIPE, Popen

proc = Popen(["/foo/bar"], stdout=PIPE, stderr=PIPE, shell=False)

I have been setting shell=False as per the S602 rule. However, running this snippet with ruff 0.0.262 errors with the S603 rule, which suggests to set shell=True. And if I do that, then the code is failing the S602 rule. Would you be able to check and see if this is intentional?

You can find the GitHub Action logs here with the above error introduced in 0.0.262.

And you can find my ruff config here.

[evanrittenhouse]

Happy to look into this! I'm not able to find any S602/S603 rule, though - here are the S rule codes. Could it be under a different code? I could also look at it if you could provide the human readable name.

[dhruvmanila]

They're here :)

https://github.com/charliermarsh/ruff/blob/cb762f4cad62000e8cd5fa26fec35b228e3bc5d8/crates/ruff/src/rules/flake8_bandit/rules/shell_injection.rs#L252-L253

[evanrittenhouse]

Gotcha - I can submit a fix to make these mutually exclusive if that makes sense? Seems like they only differ on shell truthiness, so I think that's the easiest way to go

@dhruvmanila

[dhruvmanila]

I'm confused between these two as they're the exact opposite of each other. If we try to make them mutually exclusive, then which one to detect first?

I think we should actually look into why these two rules exists in the first place and then decide what the fix should be.

Relevant docs:

• B602: https://bandit.readthedocs.io/en/latest/plugins/b602_subprocess_popen_with_shell_equals_true.html
• B603: https://bandit.readthedocs.io/en/latest/plugins/b603_subprocess_without_shell_equals_true.html

---

(A bit late here so calling it a day :))

[zeshuaro]

I think this Issue on bandit might be related to the behaviour we’re seeing here with ruff?
PyCQA/bandit#333

[evanrittenhouse]

It think B603 is a lower priority than B602:

Because this is a lesser issue than that described in subprocess_popen_with_shell_equals_true a LOW severity warning is reported.

If that's the case, in the case where both are enabled we could defer to checking S602 first - if it's triggered, we could short-circuit the S603 check

[charliermarsh]

Huh, interesting. So I guess we're consistent with Bandit here? And the only workaround within Bandit is to add a # nosec or similar, to allow low-risk subprocess calls?

I suppose we could consider removing S603 altogether. Or, we could modify S603 to only flag calls with dynamic arguments, like variables, and let all-string calls be considered "safe", which seems to be what they suggest in that issue.

[spapanik]

I think that S603 makes sense if (even one of) the arguments are dynamic, but for static arguments, it doesn't offer much

[ofek]

I vote for removal or opt-in since even non-literal statics trigger:

[sys.executable, '-m', 'ruff', 'rule', '--all']

[ThiefMaster]

Yes, this rule (S602) seems completely useless right now. It triggers on a simple subprocess.check_output(['/usr/bin/foo']) which has zero non-hardcoded input...