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

From: Jeff King <peff@peff.net>
Date: Mar 24, 2025, 03:25 UTC
Message-ID: <20250324032550.GA690093@coredump.intra.peff.net>
In-Reply-To: <pull.1930.v3.git.git.1742743771108.gitgitgadget@gmail.com>

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

Show 10 quoted lines

> 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...

Show 10 quoted lines

> 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.

Show 15 quoted lines

> +		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