On Sun, Mar 23, 2025 at 03:29:30PM +0000, Ayman Bagabas via GitGitGadget wrote:
> From: Ayman Bagabas <ayman.bagabas@xxxxxxxxx>
>
> This patch allows overriding the shell built-in commands by placing a
> script with the same name under git-shell-commands directory.
>
> This is useful for users who want to extend the shell built-in commands
> without replacing the original command binary. For instance, a user
> wanting to allow only a subset of users to run the git-receive-pack can
> override the command with a script that checks the user and calls the
> original command if the user is allowed.
OK. We do not allow users to override normal Git commands with aliases,
etc. But in the case of git-shell, those names are really a well-known
API that a client is using, and this is the only opportunity an admin
has to plug in between the client request and Git just running the
command.
So it seems like a reasonable goal. A more restricted approach might be
to provide a more formal hook/plugin interface. E.g., to run a hook
script with the command name and arguments, and have it return
success/failure to allow the to proceed.
That's not quite as flexible (in your approach I could replace what
upload-pack is doing entirely, cache its output, and so on). But it
might be harder for admins to screw up. I dunno.
Let's look at the patch...
> diff --git a/shell.c b/shell.c
> index 76333c80686..8c7f4388bd5 100644
> --- a/shell.c
> +++ b/shell.c
> @@ -194,9 +194,11 @@ int cmd_main(int argc, const char **argv)
> /* Accept "git foo" as if the caller said "git-foo". */
> prog[3] = '-';
>
> + cd_to_homedir();
> for (cmd = cmd_list ; cmd->name ; cmd++) {
Hmm, so we have moved the cd_to_homedir() call up, which used to happen
after this loop. This means that when running a builtin command found in
the loop, our working directory will potentially be different now than
it was before your patch.
That seems like an unintended side effect. Though I admit I am not sure
why git-shell would be running in anything but the user's homedir in the
first place.
> + char *full_cmd;
> if (strncmp(cmd->name, prog, len))
> continue;
> arg = NULL;
> @@ -210,10 +212,15 @@ int cmd_main(int argc, const char **argv)
> default:
> continue;
> }
> + /* Allow overriding built-in commands */
> + full_cmd = make_cmd(cmd->name);
> + if (!access(full_cmd, X_OK)) {
> + const char *argv[3] = { cmd->name, arg, NULL };
> + return execv(full_cmd, (char *const *) argv);
> + }
> return cmd->exec(cmd->name, arg);
This leaks full_cmd if the exec call fails, I'd think?
> + const char *argv[3] = { cmd->name, arg, NULL };
> + return execv(full_cmd, (char *const *) argv);
So we just stuff "arg" into the argv we pass to the script. But isn't it
supposed to be a shell command, that could have quoted arguments? For
user-defined commands, we call split_cmdline() to get the real array,
and pass it to the sub-program. For the built-in commands, we seem to
cheat a little and just assume it is a single string, which we pick
apart with sq_dequote().
But either way what your patch is doing seems wrong. Your custom
git-upload-pack (or whatever) script will get passed the quoted value,
and have to unquote itself. I guess if that were documented it _could_
be the right thing, but it seems rather unfriendly and unlike how the
other user-defined commands work (and of course it's not actually
documented).
You also miss out on the option-injection protections from 3ec804490a
(shell: disallow repo names beginning with dash, 2017-04-29). We skip
those for user-defined commands, but I think you'd probably want them
for something meant to be a wrapper around the built-in command.
Likewise the setup_path() magic done by do_generic_cmd().
So it seems like rather than running execv() ourselves here, this should
probably do one of:
a. Break out of the loop, skipping the built-in command, so that we
can run it as a regular user-defined command.
b. Hook into do_generic_cmd() instead, after we've done our de-quoting
and checked for option injection.
Of the two, I think (b) is probably the least surprising in terms of
what the wrapper script has to do.
If this were just a hook that asked "can we run this command", then none
of this would matter. Running it would be a separate step.
-Peff
