openzim / _python-bootstrap

Sample openZIM Python project bootstrap
1 stars 1 forks source link

Remove S603 `subprocess call: check for execution of untrusted input` #18

Closed benoit74 closed 11 months ago

benoit74 commented 11 months ago

We are wondering wether we should remove S603 subprocess call: check for execution of untrusted input from our standard set of rules.

I have mixed feelings.

It looks like the idea of the developers of S603 (and somehow S602) rules (and don't find anymore where I read this) is that there must be a very special attention paid to these process execution, hence the fact that it will always raise on error.

While I understand the purpose, I know that we will not really pay extra attention to #noqa : S603, at least not more than to subprocess.run or subprocess.popen.

So from my point of view, we should disable S603 from our set of rules.

rgaudin commented 11 months ago

I agree with your analysis: subprocess are always a last resort and nobody wants to use it so whenever we use it, I think we pay attention. Codefactor is also complaining about non-absolute paths so we trick it with /usr/bin/env because in our cases, we usually want users to be able to overload the external tools we use.

Unless I misread, S603 and S604 makes it impossible to run subprocess without triggering one of them anyway.

benoit74 commented 11 months ago

Yes, this is what I've read regarding S603.

I would disable only S603, other S6xx are regarding other topics that could probably be fixed to only raise a S603 (which would be ignored).