-
Notifications
You must be signed in to change notification settings - Fork 533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Subprocess instead of os #1740
base: master
Are you sure you want to change the base?
Conversation
An quick attempt to silence B605: start_process_with_a_shell
Just tested this one on python3.10 on windows 10. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on 3.11 on linux (rapsberry pi 4 with linux 5.15.65 64-bit)
Can you please explain how is this "safer" ? |
Sorry, I cannot. I'll paste here the complaint and explanation. This rule looks for the spawning of a subprocess using a command shell. This type of subprocess invocation is dangerous as it is vulnerable to various shell injection attacks. Great care should be taken to sanitize all input in order to mitigate this risk. Calls of this type are identified by the use of certain commands which are known to use shells. Example of insecure code:
The subprocess module provides more powerful facilities for spawning new processes and retrieving their results; using that module is preferable to using this function. Example of secure code:
Further Reading The Python Standard Library - subprocess - Frequently Used Arguments |
@Harvie Can you merge this? |
Ping @Harvie |
1 similar comment
Ping @Harvie |
Yeah, i am still not sure whether this is actualy making something safer or just removing actual useful feature, because some automated tool thinks it might be unsafe without understanding context... |
Did'nt notice any feature removal.. if so, we an always revert. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to include the self._onStop call made in sender.py, as well, if incorporated.
i think this will break functionality unless shell=true is provided. When shell=false is provided, it expects array of arguments rather than string. |
An quick attempt to silence B605: start_process_with_a_shell.