fix(#3577): remove shell=True and convert commands to argument lists to prevent command injection#3578
Conversation
shlokgilda
left a comment
shlokgilda
left a comment
There was a problem hiding this comment.
took a look at this - the fix is solid. nice catch on the shell=True issue.
couple notes from testing:
- git itself actually blocks the injection chars (backticks,
$(),;) in branch names - so the real attack surface was narrower than we initially thought. but|and&are still allowed, which could be exploited withshell=True, so this fix is still the right call. - please confirm the intent behind the switch from
git remote show origin | sedtogit symbolic-ref. - tiny nit:
cmd: listcould becmd: list[str]for better type hints
…nt lists to prevent command injection Signed-off-by: guptapratykshh <[email protected]>
…d strict typing Replaces git symbolic-ref with git remote show origin parsed in Python to ensure accuracy while preventing command injection. Updates type hints. Signed-off-by: guptapratykshh <[email protected]>
guptapratykshh
force-pushed
the
fix/command-injection-facade-worker
branch
from
b1913f4 to
3da58ce
Compare
shlokgilda
left a comment
shlokgilda
left a comment
There was a problem hiding this comment.
PR looks good. Once you create the helper function, I think we are GTG
…code Signed-off-by: guptapratykshh <[email protected]>
guptapratykshh
force-pushed
the
fix/command-injection-facade-worker
branch
from
1ca8aad to
a71e04f
Compare
MoralCode
left a comment
MoralCode
left a comment
There was a problem hiding this comment.
Overall i like the move to the list-based args and not using shell=True. Theres a couple unrelated changes in here though
|
|
||
| getremotedefault = ( | ||
| f"git -C {absolute_path} remote show origin | sed -n '/HEAD branch/s/.*: //p'") | ||
| getremotedefault = ["git", "-C", absolute_path, "symbolic-ref", "refs/remotes/origin/HEAD"] |
There was a problem hiding this comment.
Need to confirm that this is functionally the same
guptapratykshh
force-pushed
the
fix/command-injection-facade-worker
branch
from
b14e877 to
bd88239
Compare
Signed-off-by: guptapratykshh <[email protected]>
guptapratykshh
force-pushed
the
fix/command-injection-facade-worker
branch
from
bd88239 to
8d7d2a3
Compare
sgoggins
added
the
high priority
| return_code_remote, remotedefault = facade_helper.run_git_command( | ||
| return_code_remote, output = facade_helper.run_git_command( |
There was a problem hiding this comment.
this feels like we are maybe duplicating a lot of code here. Can we maybe refactor some of this git access type of stuff into shared functions?
|
Hello! Just wanted to check in to see if you were still interested in helping the maintainers merge this PR. We noticed it has been a little while since this last had activity, and are considering closing it or taking it over if it remains in its current state. Please react to or reply to this to confirm your interest in the next 7 days or let us know if you are no longer interested in this so we can best prioritize everyone's contributions. Thanks! |
MoralCode
added
stale
4 participants
Description
this PR fixes #3577 by removing
shell=Truefrom git command execution and converting all f-string commands to argument lists infacade worker.Signed commits