use safe local envvars for subprocess stuff #170
Conversation
|
@guykisel, thanks for your PR! By analyzing the annotation information on this pull request, we identified @v3n, @vsiakka and @raphaelcastaneda to be potential reviewers |
| shell = True | ||
| local_env = {} | ||
| safe_envvars = [ | ||
| b'PATH', b'PYTHONPATH', b'GOPATH', b'SystemRoot', |
There was a problem hiding this comment.
Looks like this could balloon into many language + tool aware exceptions. Can I get the context on what this is trying to solve?
There was a problem hiding this comment.
the idea here is to reduce risk when running potentially untrusted subprocesses by providing the absolute minimum necessary environment variables. i'm also going to refactor the way the token is being passed in the bot to make this safer.
There was a problem hiding this comment.
Ah, if this is about security then the gains from doing this are barely worth it. Processes can just look at the environment variables on their parent if they're feeling malicious. If you want to run code without trusting it you must sandbox it / jail it using OS functionality or machine separation.
There was a problem hiding this comment.
Yeah, that's a really good point about the per language ballooning. Running the comment portion in a separate container would be an option, but then you'd need to think about authorization to be able to use the comment service... how deep would that rabbit hole go?
@theTechnoWeenie does this look right to you?