Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 21, 2016, 18:23 UTC
Message-ID: <xmqqvawlblxi.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqlgxhd77u.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 20 quoted lines

> Duy Nguyen <pclouds@gmail.com> writes:
>
>> The amount of changes is unbelievable for fixing such a rare case
>> though. I wonder if we can just detect this in daemon.c and pass
>> "./~foo/bar" instead of "~foo/bar" to enter_repo() in non-strict mode
>> to "disable" expand_user_path(). If it works, it's much simpler
>> changes (even though a bit hacky)
>
> Conceptually, it ought to be updating the code that says "Does it
> begin with a tilde?  Then try to do user-path expansion and fail if
> that is not enabled and if it succeeds use the resulting directory"
> to "Is user-path enabled and does it begin with a tilde?  Then try
> to do user-path expansion and if it succeeds use the resulting
> directory".  Compared to that mental model we have with this
> codepath, I agree that the change does look quite invasive and
> large.
>
> It is OK for a change to be large, as long as the end result is
> easier to read and understand than the original.  I am undecided if
> that is the case with this patch, though.

I am still not convinced that this is a bugfix (as opposed to "add a new feature to please Luke while regressing it for others"), but the "this looks too invasive" made me look at the codepath involved.

There is this code in daemon.c::path_ok() that takes "dir" and ends up calling enter_repo().

	if (*dir == '~') {
		if (!user_path) {
			logerror("'%s': User-path not allowed", dir);
			return NULL;
		}

At first glance, it ought to be a single-liner

- if (*dir == '~') { + if (user_path && *dir == '~') {

to make 'git://site/~foo.git" go to "/var/scm/~foo.git" when base path is set to /var/scm/.

Unfortunately that is not sufficient.

A request to "git://<site>/<string>", depending on <string>, results in "directory" given to path_ok() in a bit different forms. Namely, connect.c::parse_connect_url() gives

    URL				directory
    git://site/nouser.git	/nouser.git
    git://site/~nouser.git	~nouser.git

by special casing "~user" syntax (in other words, a pathname that begins with a tilde _is_ special cased, and tilde is not considered a normal character that can be in a pathname). Note the lack of leading slash in the second one.

Because that is how the shipped clients work, the daemon needs a bit of adjustment, because interpolation and base-path prefix codepaths wants to accept only paths that begin with a slash, and relies on the slash when interpolating it in the template or concatenating it to the base (e.g. roughly "sprintf(buf, "%s%s", base_path, dir)").

I came up with the following as a less invasive patch. There no longer is the ase where "user-path not allowed" error is given, as treating tilde as just a normal pathname character even when it appears at the beginning is the whole point of this change.

As I said already, I am not sure if this is a good change, though.
 daemon.c              |  9 +++++----
 t/t5570-git-daemon.sh | 11 +++++++++++
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index afce1b9b7f..e560a535af 100644
--- a/daemon.c
+++ b/daemon.c
@@ -160,6 +160,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 {
 	static char rpath[PATH_MAX];
 	static char interp_path[PATH_MAX];
+	char tilde_path[PATH_MAX];
 	const char *path;
 	const char *dir;
 
@@ -170,11 +171,11 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 		return NULL;
 	}
 
+	if (!user_path && *dir == '~') {
+		snprintf(tilde_path, PATH_MAX, "/%s", dir);
+		dir = tilde_path;
+	}
 	if (*dir == '~') {
-		if (!user_path) {
-			logerror("'%s': User-path not allowed", dir);
-			return NULL;
-		}
 		if (*user_path) {
 			/* Got either "~alice" or "~alice/foo";
 			 * rewrite them to "~alice/%s" or
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 225a022e8a..b6d2b9a001 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -65,6 +65,17 @@ test_expect_success 'remote detects correct HEAD' '
 	)
 '
 
+test_expect_success 'tilde is a normal character without --user-path' '
+	nouser="~nouser.git" &&
+	nouser_repo="$GIT_DAEMON_DOCUMENT_ROOT_PATH/$nouser" &&
+	mkdir "$nouser_repo" &&
+	git -C "$nouser_repo" --bare init &&
+	>"$nouser_repo/git-daemon-export-ok" &&
+	git push "$nouser_repo" master:master &&
+
+	git ls-remote "$GIT_DAEMON_URL/$nouser"
+'
+
 test_expect_success 'prepare pack objects' '
 	cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&
 	(cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&