From: Omar Polo <op@omarpolo.com>
Subject: Re: dial fix for git-shell
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Fri, 10 Mar 2023 11:36:27 +0100

On 2023/03/08 19:58:48 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> Given your explanation above I am fine with your diff.
> 
> It would be nice to have a test case for a rock'n'roll repo and perhaps
> even one that uses git-shell? Perhaps git-shell would best be done in
> the gotd test suite since we there is more manual setup required for those
> already. We could require yet another user account which uses git-shell
> as its login shell and run some tests against it. I will look into this.

There was an embarassing omission in my previous diff... The
un-escaped path was passed in the argv and caused a bit of headache ^^"

Here's an improved diff that also adds a simple clone test.  it only
ensures we're still quoting it decently for the remote shell, it
doesn't involve git-shell.

diff /home/op/w/got
commit - cee3836880940ba0d1203e0682a43a680132bc27
path + /home/op/w/got
blob - 3325c8994f55721d8588155cdc72b95d11fd2248
file + lib/dial.c
--- lib/dial.c
+++ lib/dial.c
@@ -22,6 +22,7 @@
 
 #include <assert.h>
 #include <err.h>
+#include <limits.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -200,6 +201,47 @@ const struct got_error *
 	return err;
 }
 
+/*
+ * Escape a given path for the shell which will be started by sshd.
+ * In particular, git-shell is known to require single-quote characters
+ * around its repository path argument and will refuse to run otherwise.
+ */
+static const struct got_error *
+escape_path(char *buf, size_t bufsize, const char *path)
+{
+	const char	*p;
+	char		*q;
+
+	p = path;
+	q = buf;
+
+	if (bufsize > 1)
+		*q++ = '\'';
+
+	while (*p != '\0' && (q - buf < bufsize)) {
+		/* git escapes ! too */
+		if (*p != '\'' && *p != '!') {
+			*q++ = *p++;
+			continue;
+		}
+
+		if (q - buf + 4 >= bufsize)
+			break;
+		*q++ = '\'';
+		*q++ = '\\';
+		*q++ = *p++;
+		*q++ = '\'';
+	}
+
+	if (*p == '\0' && (q - buf + 1 < bufsize)) {
+		*q++ = '\'';
+		*q = '\0';
+		return NULL;
+	}
+
+	return got_error_fmt(GOT_ERR_NO_SPACE, "overlong path: %s", path);
+}
+
 const struct got_error *
 got_dial_ssh(pid_t *newpid, int *newfd, const char *host,
     const char *port, const char *path, const char *direction, int verbosity)
@@ -207,12 +249,17 @@ got_dial_ssh(pid_t *newpid, int *newfd, const char *ho
 	const struct got_error *error = NULL;
 	int pid, pfd[2];
 	char cmd[64];
+	char escaped_path[PATH_MAX];
 	const char *argv[11];
 	int i = 0, j;
 
 	*newpid = -1;
 	*newfd = -1;
 
+	error = escape_path(escaped_path, sizeof(escaped_path), path);
+	if (error)
+		return error;
+
 	argv[i++] = GOT_DIAL_PATH_SSH;
 	if (port != NULL) {
 		argv[i++] = "-p";
@@ -228,7 +275,7 @@ got_dial_ssh(pid_t *newpid, int *newfd, const char *ho
 	argv[i++] = "--";
 	argv[i++] = (char *)host;
 	argv[i++] = (char *)cmd;
-	argv[i++] = (char *)path;
+	argv[i++] = (char *)escaped_path;
 	argv[i++] = NULL;
 	assert(i <= nitems(argv));
 
blob - edb0516b38e63ce6695a517988a05d6c9a5266e3
file + regress/cmdline/clone.sh
--- regress/cmdline/clone.sh
+++ regress/cmdline/clone.sh
@@ -130,6 +130,36 @@ test_clone_list() {
 	test_done "$testroot" "$ret"
 }
 
+test_clone_quoting() {
+	local testroot=`test_init clone_basic`
+
+	got log -l0 -p -r "$testroot/repo" > $testroot/log-repo
+
+	(cd "$testroot" && cp -R repo "rock'n roll.git")
+
+	got clone -q "ssh://127.0.0.1/$testroot/rock'n roll.git" \
+		"$testroot/rock-clone"
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got clone failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	got log -l0 -p -r "$testroot/rock-clone" | \
+		sed 's@master, origin/master@master@g' \
+		>$testroot/log-repo-clone
+
+	cmp -s "$testroot/log-repo" "$testroot/log-repo-clone"
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "log -p output of cloned repository differs" >&2
+		diff -u "$testroot/log-repo" "$testroot/log-repo-clone"
+		test_done "$testroot" "$ret"
+	fi
+	test_done "$testroot" "$ret"
+}
+
 test_clone_list() {
 	local testroot=`test_init clone_list`
 	local testurl=ssh://127.0.0.1$testroot
@@ -837,6 +867,7 @@ run_test test_clone_list
 
 test_parseargs "$@"
 run_test test_clone_basic
+run_test test_clone_quoting
 run_test test_clone_list
 run_test test_clone_branch
 run_test test_clone_all