Re: [PATCH v3] shell: allow overriding built-in commands

[Jeff King <peff@peff.net>]

On Sun, Mar 23, 2025 at 03:29:30PM +0000, Ayman Bagabas via GitGitGadget wrote:

> From: Ayman Bagabas <ayman.bagabas@gmail.com>
> 
> 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