On 2023/03/08 16:54:57 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> On Wed, Mar 08, 2023 at 04:01:20PM +0100, Omar Polo wrote:
> > do we really need to quote all these characters?  Since we're wrapping
> > everything in _single_ quotes I thought we only needed to quote \ and
> > the single quote itself.
> 
> I don't know. Are there rules in POSIX we can refer to?
> 
> I took the quoting rules from /usr/bin/locale. 
> I presume it makes it safe to run `eval` on locale's output (though
> I'd have to ask guenther@ for specifics). In any case, given that
> the remote server might be spawning a regular unix shell I think it
> makes sense to quote this argument by default, such that a bad path
> argument accidentally produced by some script will not cause problems.
> (Disregarding people trying to play jokes by removing this safe-guard
> and sending garbage on purpose, then it becomes a server-side problem).

The major difference is that locale use double quotes, git-shell
requires single.  Single quotes are easier to work with, since you
just need to escape an inner ' character.

I was wrong in my initial message, \ can't be used to escape the
single quoting:

	$ /bin/echo '\'
	\

(was tricked by how the ksh' echo builtin process some \c sequences.)

Furthermore, your escape_path routine will produce wrong results since
it's thought to be used to quote inside double quotes string and not
single ones.

Consider:

	$ /bin/echo '\$foo'
	\$foo
	$ /bin/echo "\$foo"
	$foo

> In case of gotsh/gotd we don't care, and we also don't really care
> whether any of these characters work in a gotd.conf repository path.
> Some of them probably won't ever work because of parse.y limitations?

unless you're willing to accept a patch to add some way to add fancy
characters in gotd.conf I guess this won't bite us.  I don't know how
many peoples have git repos with path containing control codes...

> One problem could be that someone out there is using some characters in
> a path (perhaps ~ could appear somehwere) and someone else wants to use
> 'got clone' with the resulting URL. In that case they can always use Git as
> a workaround, or we can adjust our quoting as such problems get uncovered.
> Should we provide an option to turn the quoting off, just in case?

I don't think it is worth it.  Quoting inside single quotes *should*
be simple and I don't think we need to provide an option, env variable
or even a compile-time toggle, nor having folks using git for this use
case.

> > Then I took a look at git: in quote.c:sq_quote_buf quotes ' and !
> > which I find admittedly confusing.  it is the function used in
> > connect.c:git_connect to prepare the ssh connection.
> 
> No idea.
> Even just the single-quote requirement of git-shell is silly to begin with.
> git-shell should be doing its own escaping, assuming arbitrary input, not
> require the input to be already quoted somehow.

it's probably just a safe-guard in case someone switches back from
git-shell to a regular shell?  Just wondering.

How about diff below?

the question now becomes why git escapes ! too, any idea?  I skimmed
thru csh(1) and it seem to imply that single-quotes behaves like in
sh(1).  maybe some other popular shell has some quirks?

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";