On Mon, Jul 15, 2024 at 7:52 PM Jeff King <peff@xxxxxxxx> wrote: > > On Mon, Jul 15, 2024 at 01:18:52PM -0700, Junio C Hamano wrote: > Yes, I think this is reasonable. You'd also perhaps want to have it set > child->git_cmd as appropriate (though really, I do not think that does > anything except stick "git" into child.args[0], so we could just do that > ourselves). > > I'm actually a little surprised it was not written this way in the first > place. I was too. It seems odd to combine the arguments into a single string early. I was also surprised / didn't realize that 'use_shell' might be ignored. > > Having said that, I do not think you can avoid /bin/sh if your goal > > is "minimal image *to run git*", as there are many things we run, > > starting from the editor and the pager and end-user hooks. The > > credential helper is probably the least of your problems. What's a > > minimum /bin/dash image cost these days? Adding dash is ultimately what we're going to do at least for now. That won't get us a pager or an editor. That's fine. The image will be used in non-interactive environments such as c-i. Not being able to utilize your custom hook that runs awk and sed is one thing. Not being able to build your private repo from github is another. Thanks for your input. On Mon, Jul 15, 2024 at 7:52 PM Jeff King <peff@xxxxxxxx> wrote: > > On Mon, Jul 15, 2024 at 01:18:52PM -0700, Junio C Hamano wrote: > > > Even though the code path starting from start_command() is what > > run_credential_helper() does use, what is run is NOT a simple > > command "/bin/myhelper". It will receive arguments, like > > > > /bin/myhelper erase > > /bin/myhelper get > > /bin/myhelper store > > > > etc., because the caller appends these operation verbs to the value > > of the configuration variable. And as you found out, to tokenize them > > into two, we need shell. > > It is also usually "git myhelper get", and so on (though you can > configure a shell command). > > > We may be able to teach credential.c:credential_do() not to paste > > the operation verb to the command line so early. Instead you could > > teach the function to send the command line and operation verb > > separately down to run_credential_helper() though. That way, we > > might be able to avoid the shell in this particular case. That is, > > if we can > > > > * Have start_command() -> prepare_cmd() -> prepare_shell_cmd() > > codepath to take the usual route _without_ the operation verb > > tucked to the command line, we would get cmd->args.v[] that does > > not rely on the shell; > > > > * Then before the prepared command is executed, if we can somehow > > _append_ to cmd->args.v[] the operation verb (after all, that > > wants to become the argv[1] to the spawned command) before > > start_command() exec's it > > > > then we are done. > > Yes, I think this is reasonable. You'd also perhaps want to have it set > child->git_cmd as appropriate (though really, I do not think that does > anything except stick "git" into child.args[0], so we could just do that > ourselves). > > I'm actually a little surprised it was not written this way in the first > place. In the non-!, non-absolute-path case we are pasting together a > string that will be passed to the shell, and it includes the "helper" > argument without further quoting. I don't think you could smuggle a > semicolon into there (due to our protocol restrictions), but it does > seem like a possible shell injection route. > > I think it probably goes all the way back to my abca927dbe (introduce > credentials API, 2011-12-10). > > > Having said that, I do not think you can avoid /bin/sh if your goal > > is "minimal image *to run git*", as there are many things we run, > > starting from the editor and the pager and end-user hooks. The > > credential helper is probably the least of your problems. What's a > > minimum /bin/dash image cost these days? > > Right. The bigger issue to me is that the credential helper "!" syntax > is defined to the end user as running a shell (and ditto for all of > those other spots). So any platform where we can't run a shell would not > be fully compatible with git on other platforms. > > That may be an OK limitation as long as it is advertised clearly, but > the use of a shell here is not just an implementation detail, but an > intentional user-facing design. > > -Peff
