{"thread":{"id":"63175","subject":"[PATCH] [RFC] shell: allow overriding built-in commands","startedAt":"2025-03-22T10:01:57Z","lastAt":"2025-03-25T22:44:40Z","messageCount":13,"participants":["Ayman Bagabas via GitGitGadget","Elijah Newren","Ayman Bagabas","Chris Torek","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"514856","messageId":"pull.1930.git.git.1742637713157.gitgitgadget@gmail.com","threadId":"63175","inReplyTo":null,"subject":"[PATCH] [RFC] shell: allow overriding built-in commands","fromName":"Ayman Bagabas via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-22T10:01:53Z","receivedAt":"2025-03-22T10:01:57Z","isPatch":true,"sender":{"key":"ayman.bagabas@gmail.com","avatar":"https://gravatar.com/avatar/c7039df0811105496fa9f5d0a3b1b2664925b715b19790a019e9f0efa958ac3d?d=mp&s=160"},"body":"From: Ayman Bagabas \n\nThis patch allows overriding built-in commands by placing a script\nwith the same name under git-shell-commands directory.\n\nThis is useful for users who want to extend the built-in commands\nwithout replacing the original command binary. For instance, a user\nwanting to allow only a subset of users to run the git-receive-pack\ncan override the command with a script that checks the user and\ncalls the original command if the user is allowed.\n\nCC: Junio C Hamano , Jeff King \nCC: Taylor Blau \nSigned-off-by: Ayman Bagabas \n---\n [RFC] shell: allow overriding built-in commands\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1930%2Faymanbagabas%2Fshell-override-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1930/aymanbagabas/shell-override-v1\nPull-Request: https://github.com/git/git/pull/1930\n\n shell.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/shell.c b/shell.c\nindex 76333c80686..3a6d2e8a044 100644\n--- a/shell.c\n+++ b/shell.c\n@@ -194,9 +194,11 @@ int cmd_main(int argc, const char **argv)\n \t\t/* Accept \"git foo\" as if the caller said \"git-foo\". */\n \t\tprog[3] = '-';\n \n+\tcd_to_homedir();\n \tfor (cmd = cmd_list ; cmd->name ; cmd++) {\n \t\tint len = strlen(cmd->name);\n \t\tchar *arg;\n+\t\tchar *full_cmd;\n \t\tif (strncmp(cmd->name, prog, len))\n \t\t\tcontinue;\n \t\targ = NULL;\n@@ -210,10 +212,16 @@ int cmd_main(int argc, const char **argv)\n \t\tdefault:\n \t\t\tcontinue;\n \t\t}\n-\t\treturn cmd->exec(cmd->name, arg);\n+\t\t/* Allow overriding built-in commands */\n+\t\tfull_cmd = make_cmd(cmd->name);\n+\t\tif (!access(full_cmd, F_OK)) {\n+\t\t\tconst char *argv[3] = { cmd->name, arg, NULL };\n+\t\t\treturn execv(full_cmd, (char *const *) argv);\n+\t\t} else {\n+\t\t\treturn cmd->exec(cmd->name, arg);\n+\t\t}\n \t}\n \n-\tcd_to_homedir();\n \tcount = split_cmdline(prog, &user_argv);\n \tif (count >= 0) {\n \t\tif (is_valid_cmd_name(user_argv[0])) {\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n-- \ngitgitgadget\n"},{"id":"514860","messageId":"CABPp-BH9ia_GRNYx+zRCht3n0V=X-uLq2Zg+QWTWCyJfMkeznA@mail.gmail.com","threadId":"63175","inReplyTo":"pull.1930.git.git.1742637713157.gitgitgadget@gmail.com","subject":"Re: [PATCH] [RFC] shell: allow overriding built-in commands","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-22T17:39:33Z","receivedAt":"2025-03-22T17:39:45Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Mar 22, 2025 at 3:02 AM Ayman Bagabas via GitGitGadget\n wrote:\n>\n> From: Ayman Bagabas \n>\n> This patch allows overriding built-in commands by placing a script\n> with the same name under git-shell-commands directory.\n>\n> This is useful for users who want to extend the built-in commands\n> without replacing the original command binary. For instance, a user\n> wanting to allow only a subset of users to run the git-receive-pack\n> can override the command with a script that checks the user and\n> calls the original command if the user is allowed.\n\nSounds like it'd open a window to generating numerous security\nvulnerabilities, break git's own commands that exec another git\nsubprocess (e.g. git-stash), make debugging git bug reports harder,\nand likely break programs that use plumbing commands.\n\nI'd rather we didn't.\n"},{"id":"514861","messageId":"9A950A71-8847-4F1C-A691-5A9CD461FE6C@gmail.com","threadId":"63175","inReplyTo":"CABPp-BH9ia_GRNYx+zRCht3n0V=X-uLq2Zg+QWTWCyJfMkeznA@mail.gmail.com","subject":"Re: [PATCH] [RFC] shell: allow overriding built-in commands","fromName":"Ayman Bagabas","fromEmail":"ayman.bagabas@gmail.com","sentAt":"2025-03-22T18:02:17Z","receivedAt":"2025-03-22T18:02:32Z","isPatch":true,"sender":{"key":"ayman.bagabas@gmail.com","avatar":"https://gravatar.com/avatar/c7039df0811105496fa9f5d0a3b1b2664925b715b19790a019e9f0efa958ac3d?d=mp&s=160"},"body":"\n\n> On Mar 22, 2025, at 8:39 PM, Elijah Newren wrote:\n> \n> On Sat, Mar 22, 2025 at 3:02 AM Ayman Bagabas via GitGitGadget\n> wrote:\n>> \n>> From: Ayman Bagabas \n>> \n>> This patch allows overriding built-in commands by placing a script\n>> with the same name under git-shell-commands directory.\n>> \n>> This is useful for users who want to extend the built-in commands\n>> without replacing the original command binary. For instance, a user\n>> wanting to allow only a subset of users to run the git-receive-pack\n>> can override the command with a script that checks the user and\n>> calls the original command if the user is allowed.\n> \n> Sounds like it'd open a window to generating numerous security\n> vulnerabilities, break git's own commands that exec another git\n> subprocess (e.g. git-stash), make debugging git bug reports harder,\n> and likely break programs that use plumbing commands.\n\nHow so? The security implications are the same as any script\ndefined under git-shell-commands. Git does not handle authentication\nnor authorization and it shouldn't do so, and this can allow repository\nbased authorization to happen using git-shell.\n\nForgive my limited knowledge about git internals but how would this\nbreak git's own commands that exec another git subprocess and\nplumbing commands?\n\n> \n> I'd rather we didn't.\n\n"},{"id":"514862","messageId":"CABPp-BHDF9ufzP6e-YfFbom6XG_QUxr5s=ZGVS-6+zSYm0bBFQ@mail.gmail.com","threadId":"63175","inReplyTo":"9A950A71-8847-4F1C-A691-5A9CD461FE6C@gmail.com","subject":"Re: [PATCH] [RFC] shell: allow overriding built-in commands","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-22T18:26:11Z","receivedAt":"2025-03-22T18:26:23Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Mar 22, 2025 at 11:02 AM Ayman Bagabas wrote:\n>\n> > On Mar 22, 2025, at 8:39 PM, Elijah Newren wrote:\n> >\n> > On Sat, Mar 22, 2025 at 3:02 AM Ayman Bagabas via GitGitGadget\n> > wrote:\n> >>\n> >> From: Ayman Bagabas \n> >>\n> >> This patch allows overriding built-in commands by placing a script\n> >> with the same name under git-shell-commands directory.\n> >>\n> >> This is useful for users who want to extend the built-in commands\n> >> without replacing the original command binary. For instance, a user\n> >> wanting to allow only a subset of users to run the git-receive-pack\n> >> can override the command with a script that checks the user and\n> >> calls the original command if the user is allowed.\n> >\n> > Sounds like it'd open a window to generating numerous security\n> > vulnerabilities, break git's own commands that exec another git\n> > subprocess (e.g. git-stash), make debugging git bug reports harder,\n> > and likely break programs that use plumbing commands.\n>\n> How so? The security implications are the same as any script\n> defined under git-shell-commands. Git does not handle authentication\n> nor authorization and it shouldn't do so, and this can allow repository\n> based authorization to happen using git-shell.\n>\n> Forgive my limited knowledge about git internals but how would this\n> break git's own commands that exec another git subprocess and\n> plumbing commands?\n\nSorry, I wasn't reading closely enough and missed the part where this\nwas specific to `git shell` as opposed to a general ability to\noverride built-ins.\n"},{"id":"514865","messageId":"pull.1930.v2.git.git.1742688740650.gitgitgadget@gmail.com","threadId":"63175","inReplyTo":"pull.1930.git.git.1742637713157.gitgitgadget@gmail.com","subject":"[PATCH v2] shell: allow overriding built-in commands","fromName":"Ayman Bagabas via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-23T00:12:20Z","receivedAt":"2025-03-23T00:12:24Z","isPatch":true,"sender":{"key":"ayman.bagabas@gmail.com","avatar":"https://gravatar.com/avatar/c7039df0811105496fa9f5d0a3b1b2664925b715b19790a019e9f0efa958ac3d?d=mp&s=160"},"body":"From: Ayman Bagabas \n\nThis patch allows overriding the shell built-in commands by placing a\nscript with the same name under git-shell-commands directory.\n\nThis is useful for users who want to extend the shell built-in commands\nwithout replacing the original command binary. For instance, a user\nwanting to allow only a subset of users to run the git-receive-pack can\noverride the command with a script that checks the user and calls the\noriginal command if the user is allowed.\n\nSigned-off-by: Ayman Bagabas \n---\n shell: allow overriding built-in commands\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1930%2Faymanbagabas%2Fshell-override-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1930/aymanbagabas/shell-override-v2\nPull-Request: https://github.com/git/git/pull/1930\n\nRange-diff vs v1:\n\n 1: 4fe18878afa ! 1: 60c6339e790 [RFC] shell: allow overriding built-in commands\n @@ Metadata\n Author: Ayman Bagabas \n \n ## Commit message ##\n - [RFC] shell: allow overriding built-in commands\n + shell: allow overriding built-in commands\n \n - This patch allows overriding built-in commands by placing a script\n - with the same name under git-shell-commands directory.\n + This patch allows overriding the shell built-in commands by placing a\n + script with the same name under git-shell-commands directory.\n \n - This is useful for users who want to extend the built-in commands\n + This is useful for users who want to extend the shell built-in commands\n without replacing the original command binary. For instance, a user\n - wanting to allow only a subset of users to run the git-receive-pack\n - can override the command with a script that checks the user and\n - calls the original command if the user is allowed.\n + wanting to allow only a subset of users to run the git-receive-pack can\n + override the command with a script that checks the user and calls the\n + original command if the user is allowed.\n \n - CC: Junio C Hamano , Jeff King \n - CC: Taylor Blau \n Signed-off-by: Ayman Bagabas \n \n ## shell.c ##\n @@ shell.c: int cmd_main(int argc, const char **argv)\n \t\tdefault:\n \t\t\tcontinue;\n \t\t}\n --\t\treturn cmd->exec(cmd->name, arg);\n +\t\t/* Allow overriding built-in commands */\n +\t\tfull_cmd = make_cmd(cmd->name);\n +\t\tif (!access(full_cmd, F_OK)) {\n +\t\t\tconst char *argv[3] = { cmd->name, arg, NULL };\n +\t\t\treturn execv(full_cmd, (char *const *) argv);\n -+\t\t} else {\n -+\t\t\treturn cmd->exec(cmd->name, arg);\n +\t\t}\n + \t\treturn cmd->exec(cmd->name, arg);\n \t}\n \n -\tcd_to_homedir();\n\n\n shell.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/shell.c b/shell.c\nindex 76333c80686..76c0157e321 100644\n--- a/shell.c\n+++ b/shell.c\n@@ -194,9 +194,11 @@ int cmd_main(int argc, const char **argv)\n \t\t/* Accept \"git foo\" as if the caller said \"git-foo\". */\n \t\tprog[3] = '-';\n \n+\tcd_to_homedir();\n \tfor (cmd = cmd_list ; cmd->name ; cmd++) {\n \t\tint len = strlen(cmd->name);\n \t\tchar *arg;\n+\t\tchar *full_cmd;\n \t\tif (strncmp(cmd->name, prog, len))\n \t\t\tcontinue;\n \t\targ = NULL;\n@@ -210,10 +212,15 @@ int cmd_main(int argc, const char **argv)\n \t\tdefault:\n \t\t\tcontinue;\n \t\t}\n+\t\t/* Allow overriding built-in commands */\n+\t\tfull_cmd = make_cmd(cmd->name);\n+\t\tif (!access(full_cmd, F_OK)) {\n+\t\t\tconst char *argv[3] = { cmd->name, arg, NULL };\n+\t\t\treturn execv(full_cmd, (char *const *) argv);\n+\t\t}\n \t\treturn cmd->exec(cmd->name, arg);\n \t}\n \n-\tcd_to_homedir();\n \tcount = split_cmdline(prog, &user_argv);\n \tif (count >= 0) {\n \t\tif (is_valid_cmd_name(user_argv[0])) {\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n-- \ngitgitgadget\n"},{"id":"514866","messageId":"CAPx1Gvd+0XBVm6N6Vrogczhqy8hARajoQ+zDgaaeKMTcSskmeA@mail.gmail.com","threadId":"63175","inReplyTo":"pull.1930.v2.git.git.1742688740650.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] shell: allow overriding built-in commands","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2025-03-23T01:11:51Z","receivedAt":"2025-03-23T01:12:04Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"I'm not at all sure about any security implications, but aside from that, I\nsuspect this:\n\nOn Sat, Mar 22, 2025 at 5:13 PM Ayman Bagabas via GitGitGadget\n wrote:\n> + if (!access(full_cmd, F_OK)) {\n\nshould use X_OK rather than F_OK.\n\nChris\n"},{"id":"514877","messageId":"11DFC309-0D48-42C8-A683-F5D0C30810D3@gmail.com","threadId":"63175","inReplyTo":"CAPx1Gvd+0XBVm6N6Vrogczhqy8hARajoQ+zDgaaeKMTcSskmeA@mail.gmail.com","subject":"Re: [PATCH v2] shell: allow overriding built-in commands","fromName":"Ayman Bagabas","fromEmail":"ayman.bagabas@gmail.com","sentAt":"2025-03-23T15:05:38Z","receivedAt":"2025-03-23T15:05:53Z","isPatch":true,"sender":{"key":"ayman.bagabas@gmail.com","avatar":"https://gravatar.com/avatar/c7039df0811105496fa9f5d0a3b1b2664925b715b19790a019e9f0efa958ac3d?d=mp&s=160"},"body":"\n\n> On Mar 23, 2025, at 4:11 AM, Chris Torek wrote:\n> \n> I'm not at all sure about any security implications, but aside from that, I\n> suspect this:\n> \n> On Sat, Mar 22, 2025 at 5:13 PM Ayman Bagabas via GitGitGadget\n> wrote:\n>> + if (!access(full_cmd, F_OK)) {\n> \n> should use X_OK rather than F_OK.\n\nGood catch!\n\nShouldn't we also check for both F_OK?\n\n> \n> Chris\n\n"},{"id":"514878","messageId":"CAPx1GvfPUqHkT8MewXyAS5ptO2Cz+Jn1+1w9wa5j-WfO68dfYg@mail.gmail.com","threadId":"63175","inReplyTo":"11DFC309-0D48-42C8-A683-F5D0C30810D3@gmail.com","subject":"Re: [PATCH v2] shell: allow overriding built-in commands","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2025-03-23T15:12:10Z","receivedAt":"2025-03-23T15:12:23Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Sun, Mar 23, 2025 at 8:05 AM Ayman Bagabas wrote:\n> Shouldn't we also check [access()] for both F_OK?\n\nAny success for R_OK, W_OK, or X_OK implies F_OK.\n\nI'm not a fan of access() calls in general for multiple reasons\n(they get suggested for security tests in setuid programs, but\nyou race from \"access seems OK\" to \"actually try to access\"\nagainst anyone who modifies the name-to-underlying-entity\nbinding, so in general the \"right\" test is \"do the operation\nwith the appropriate permissions in place, then see if it\nactually worked\", which is atomic). But in this particular case\nit's probably not that important (we seem to make a lot of\nassumptions about security, or perhaps lack of its importance,\nin git-shell).\n\nChris\n"},{"id":"514879","messageId":"pull.1930.v3.git.git.1742743771108.gitgitgadget@gmail.com","threadId":"63175","inReplyTo":"pull.1930.v2.git.git.1742688740650.gitgitgadget@gmail.com","subject":"[PATCH v3] shell: allow overriding built-in commands","fromName":"Ayman Bagabas via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-23T15:29:30Z","receivedAt":"2025-03-23T15:29:34Z","isPatch":true,"sender":{"key":"ayman.bagabas@gmail.com","avatar":"https://gravatar.com/avatar/c7039df0811105496fa9f5d0a3b1b2664925b715b19790a019e9f0efa958ac3d?d=mp&s=160"},"body":"From: Ayman Bagabas \n\nThis patch allows overriding the shell built-in commands by placing a\nscript with the same name under git-shell-commands directory.\n\nThis is useful for users who want to extend the shell built-in commands\nwithout replacing the original command binary. For instance, a user\nwanting to allow only a subset of users to run the git-receive-pack can\noverride the command with a script that checks the user and calls the\noriginal command if the user is allowed.\n\nSigned-off-by: Ayman Bagabas \n---\n shell: allow overriding built-in commands\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1930%2Faymanbagabas%2Fshell-override-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1930/aymanbagabas/shell-override-v3\nPull-Request: https://github.com/git/git/pull/1930\n\nRange-diff vs v2:\n\n 1: 60c6339e790 ! 1: 7e6996d199e shell: allow overriding built-in commands\n @@ shell.c: int cmd_main(int argc, const char **argv)\n \t\t}\n +\t\t/* Allow overriding built-in commands */\n +\t\tfull_cmd = make_cmd(cmd->name);\n -+\t\tif (!access(full_cmd, F_OK)) {\n ++\t\tif (!access(full_cmd, X_OK)) {\n +\t\t\tconst char *argv[3] = { cmd->name, arg, NULL };\n +\t\t\treturn execv(full_cmd, (char *const *) argv);\n +\t\t}\n\n\n shell.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/shell.c b/shell.c\nindex 76333c80686..8c7f4388bd5 100644\n--- a/shell.c\n+++ b/shell.c\n@@ -194,9 +194,11 @@ int cmd_main(int argc, const char **argv)\n \t\t/* Accept \"git foo\" as if the caller said \"git-foo\". */\n \t\tprog[3] = '-';\n \n+\tcd_to_homedir();\n \tfor (cmd = cmd_list ; cmd->name ; cmd++) {\n \t\tint len = strlen(cmd->name);\n \t\tchar *arg;\n+\t\tchar *full_cmd;\n \t\tif (strncmp(cmd->name, prog, len))\n \t\t\tcontinue;\n \t\targ = NULL;\n@@ -210,10 +212,15 @@ int cmd_main(int argc, const char **argv)\n \t\tdefault:\n \t\t\tcontinue;\n \t\t}\n+\t\t/* Allow overriding built-in commands */\n+\t\tfull_cmd = make_cmd(cmd->name);\n+\t\tif (!access(full_cmd, X_OK)) {\n+\t\t\tconst char *argv[3] = { cmd->name, arg, NULL };\n+\t\t\treturn execv(full_cmd, (char *const *) argv);\n+\t\t}\n \t\treturn cmd->exec(cmd->name, arg);\n \t}\n \n-\tcd_to_homedir();\n \tcount = split_cmdline(prog, &user_argv);\n \tif (count >= 0) {\n \t\tif (is_valid_cmd_name(user_argv[0])) {\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n-- \ngitgitgadget\n"},{"id":"514895","messageId":"20250324032550.GA690093@coredump.intra.peff.net","threadId":"63175","inReplyTo":"pull.1930.v3.git.git.1742743771108.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] shell: allow overriding built-in commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-24T03:25:50Z","receivedAt":"2025-03-24T03:25:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 23, 2025 at 03:29:30PM +0000, Ayman Bagabas via GitGitGadget wrote:\n\n> From: Ayman Bagabas \n> \n> This patch allows overriding the shell built-in commands by placing a\n> script with the same name under git-shell-commands directory.\n> \n> This is useful for users who want to extend the shell built-in commands\n> without replacing the original command binary. For instance, a user\n> wanting to allow only a subset of users to run the git-receive-pack can\n> override the command with a script that checks the user and calls the\n> original command if the user is allowed.\n\nOK. We do not allow users to override normal Git commands with aliases,\netc. But in the case of git-shell, those names are really a well-known\nAPI that a client is using, and this is the only opportunity an admin\nhas to plug in between the client request and Git just running the\ncommand.\n\nSo it seems like a reasonable goal. A more restricted approach might be\nto provide a more formal hook/plugin interface. E.g., to run a hook\nscript with the command name and arguments, and have it return\nsuccess/failure to allow the to proceed.\n\nThat's not quite as flexible (in your approach I could replace what\nupload-pack is doing entirely, cache its output, and so on). But it\nmight be harder for admins to screw up. I dunno.\n\nLet's look at the patch...\n\n> diff --git a/shell.c b/shell.c\n> index 76333c80686..8c7f4388bd5 100644\n> --- a/shell.c\n> +++ b/shell.c\n> @@ -194,9 +194,11 @@ int cmd_main(int argc, const char **argv)\n> \t\t/* Accept \"git foo\" as if the caller said \"git-foo\". */\n> \t\tprog[3] = '-';\n> \n> +\tcd_to_homedir();\n> \tfor (cmd = cmd_list ; cmd->name ; cmd++) {\n\nHmm, so we have moved the cd_to_homedir() call up, which used to happen\nafter this loop. This means that when running a builtin command found in\nthe loop, our working directory will potentially be different now than\nit was before your patch.\n\nThat seems like an unintended side effect. Though I admit I am not sure\nwhy git-shell would be running in anything but the user's homedir in the\nfirst place.\n\n> +\t\tchar *full_cmd;\n> \t\tif (strncmp(cmd->name, prog, len))\n> \t\t\tcontinue;\n> \t\targ = NULL;\n> @@ -210,10 +212,15 @@ int cmd_main(int argc, const char **argv)\n> \t\tdefault:\n> \t\t\tcontinue;\n> \t\t}\n> +\t\t/* Allow overriding built-in commands */\n> +\t\tfull_cmd = make_cmd(cmd->name);\n> +\t\tif (!access(full_cmd, X_OK)) {\n> +\t\t\tconst char *argv[3] = { cmd->name, arg, NULL };\n> +\t\t\treturn execv(full_cmd, (char *const *) argv);\n> +\t\t}\n> \t\treturn cmd->exec(cmd->name, arg);\n\nThis leaks full_cmd if the exec call fails, I'd think?\n\n> +\t\t\tconst char *argv[3] = { cmd->name, arg, NULL };\n> +\t\t\treturn execv(full_cmd, (char *const *) argv);\n\nSo we just stuff \"arg\" into the argv we pass to the script. But isn't it\nsupposed to be a shell command, that could have quoted arguments? For\nuser-defined commands, we call split_cmdline() to get the real array,\nand pass it to the sub-program. For the built-in commands, we seem to\ncheat a little and just assume it is a single string, which we pick\napart with sq_dequote().\n\nBut either way what your patch is doing seems wrong. Your custom\ngit-upload-pack (or whatever) script will get passed the quoted value,\nand have to unquote itself. I guess if that were documented it _could_\nbe the right thing, but it seems rather unfriendly and unlike how the\nother user-defined commands work (and of course it's not actually\ndocumented).\n\nYou also miss out on the option-injection protections from 3ec804490a\n(shell: disallow repo names beginning with dash, 2017-04-29). We skip\nthose for user-defined commands, but I think you'd probably want them\nfor something meant to be a wrapper around the built-in command.\nLikewise the setup_path() magic done by do_generic_cmd().\n\nSo it seems like rather than running execv() ourselves here, this should\nprobably do one of:\n\n a. Break out of the loop, skipping the built-in command, so that we\n can run it as a regular user-defined command.\n\n b. Hook into do_generic_cmd() instead, after we've done our de-quoting\n and checked for option injection.\n\nOf the two, I think (b) is probably the least surprising in terms of\nwhat the wrapper script has to do.\n\nIf this were just a hook that asked \"can we run this command\", then none\nof this would matter. Running it would be a separate step.\n\n-Peff\n"},{"id":"514899","messageId":"xmqqy0wvov5n.fsf@gitster.g","threadId":"63175","inReplyTo":"20250324032550.GA690093@coredump.intra.peff.net","subject":"Re: [PATCH v3] shell: allow overriding built-in commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-24T05:27:32Z","receivedAt":"2025-03-24T05:27:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So it seems like a reasonable goal. A more restricted approach might be\n> to provide a more formal hook/plugin interface. E.g., to run a hook\n> script with the command name and arguments, and have it return\n> success/failure to allow the to proceed.\n>\n> That's not quite as flexible (in your approach I could replace what\n> upload-pack is doing entirely, cache its output, and so on). But it\n> might be harder for admins to screw up. I dunno.\n\nYeah, we usually try not to be overly flexible for that reason, but\ngiven that \"git shell\" is so limited that rewriting its services\nwholesale is not all that much of a deal, I think it is OK.\n\nI however wonder if it is worth admins' time and effort to add\nfeatures to \"git-shell\" using this new facility, or if they are\nbetter off using something more established like gitolite once they\nwant to go fancier beyond what the basic \"git-shell\" offers.\n\n\n"},{"id":"514968","messageId":"20250324202838.GB777700@coredump.intra.peff.net","threadId":"63175","inReplyTo":"xmqqy0wvov5n.fsf@gitster.g","subject":"Re: [PATCH v3] shell: allow overriding built-in commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-24T20:28:38Z","receivedAt":"2025-03-24T20:28:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 23, 2025 at 10:27:32PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > So it seems like a reasonable goal. A more restricted approach might be\n> > to provide a more formal hook/plugin interface. E.g., to run a hook\n> > script with the command name and arguments, and have it return\n> > success/failure to allow the to proceed.\n> >\n> > That's not quite as flexible (in your approach I could replace what\n> > upload-pack is doing entirely, cache its output, and so on). But it\n> > might be harder for admins to screw up. I dunno.\n> \n> Yeah, we usually try not to be overly flexible for that reason, but\n> given that \"git shell\" is so limited that rewriting its services\n> wholesale is not all that much of a deal, I think it is OK.\n> \n> I however wonder if it is worth admins' time and effort to add\n> features to \"git-shell\" using this new facility, or if they are\n> better off using something more established like gitolite once they\n> want to go fancier beyond what the basic \"git-shell\" offers.\n\nYeah, I left my general opinions on git-shell unspoken. ;)\n\nFor features, I think you are probably better off with something like\ngitolite (which I think does have some access control).\n\nFor security, I'd be a little scared of git-shell, just because it's not\nrun all that frequently and we've had issues with it before (e.g.,\ninteger overflows). If you're taking requests from untrusted clients,\nyou're probably better off configuring http service.\n\nI also imagine there may be restricted shell implementations that are\nmore general and more battle-hardened, that you could configure to only\nrun a few commands. But I haven't looked at that space (because again,\nI'd suggest just git-over-http).\n\n-Peff\n"},{"id":"515045","messageId":"FA9BF11C-4237-4A0A-9C06-37F0C4E8B712@gmail.com","threadId":"63175","inReplyTo":"20250324202838.GB777700@coredump.intra.peff.net","subject":"Re: [PATCH v3] shell: allow overriding built-in commands","fromName":"Ayman Bagabas","fromEmail":"ayman.bagabas@gmail.com","sentAt":"2025-03-25T22:44:24Z","receivedAt":"2025-03-25T22:44:40Z","isPatch":true,"sender":{"key":"ayman.bagabas@gmail.com","avatar":"https://gravatar.com/avatar/c7039df0811105496fa9f5d0a3b1b2664925b715b19790a019e9f0efa958ac3d?d=mp&s=160"},"body":"\n\n> On Mar 24, 2025, at 11:28 PM, Jeff King wrote:\n> \n> On Sun, Mar 23, 2025 at 10:27:32PM -0700, Junio C Hamano wrote:\n> \n>> Jeff King writes:\n>> \n>>> So it seems like a reasonable goal. A more restricted approach might be\n>>> to provide a more formal hook/plugin interface. E.g., to run a hook\n>>> script with the command name and arguments, and have it return\n>>> success/failure to allow the to proceed.\n>>> \n>>> That's not quite as flexible (in your approach I could replace what\n>>> upload-pack is doing entirely, cache its output, and so on). But it\n>>> might be harder for admins to screw up. I dunno.\n>> \n>> Yeah, we usually try not to be overly flexible for that reason, but\n>> given that \"git shell\" is so limited that rewriting its services\n>> wholesale is not all that much of a deal, I think it is OK.\n>> \n>> I however wonder if it is worth admins' time and effort to add\n>> features to \"git-shell\" using this new facility, or if they are\n>> better off using something more established like gitolite once they\n>> want to go fancier beyond what the basic \"git-shell\" offers.\n> \n> Yeah, I left my general opinions on git-shell unspoken. ;)\n> \n> For features, I think you are probably better off with something like\n> gitolite (which I think does have some access control).\n\nGitolite is a great software, but it also has its limitations. It couples\nauthentication and authorization in the same system. However, I'm looking\nfor something more flexible that I can plug whatever authentication\nor authorization system to the mix similar to git-http-backend paired with\napache/nginx/h2o/etc.\n\n> \n> For security, I'd be a little scared of git-shell, just because it's not\n> run all that frequently and we've had issues with it before (e.g.,\n> integer overflows). If you're taking requests from untrusted clients,\n> you're probably better off configuring http service.\n\nThat's a fair point. Perhaps writing my own restricted shell might be\nthe best solution for what I'm looking for :/\n\n> \n> I also imagine there may be restricted shell implementations that are\n> more general and more battle-hardened, that you could configure to only\n> run a few commands. But I haven't looked at that space (because again,\n> I'd suggest just git-over-http).\n\nIf you know any general restricted shell implementations please do tell. I'm\nlooking for an SSH solution something pluggable like git-http-backend that\nI can build on top of.\n\nHonestly, git-shell's simplicity is what got my interest at first. The fact that\nit's not secure and not run frequently can change and be improved.\n\n- Ayman\n\n\n"}]}