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

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

[Luke Shumaker]

The superficial aspect of this change is that git-daemon now allows paths
that start with a "~".  Previously, if git-daemon was run with
"--base-path=/srv/git", it was impossible to get it to serve
"/srv/git/~foo/bar.git".  An odd edge-case that was broken.

But from a source-code standpoint, the change is in path.c:enter_repo().  I
have adjusted it to take separate "strict_prefix" and "strict_suffix"
arguments, rather than a single "strict" argument.

I also make it clearer what the purpose of each path buffer is for, by
renaming them to chdir_path and ret_path; chdir_path is the path that we
pass to chdir(); return_path is the path we return to the user.  Using this
nomenclature, we can more easily explain the behavior of the function.
There are 3 DWIM measures that enter_repo() provides: tilde expansion,
suffix guessing, and gitfile expansion; it also trims trailing slashes.
Here is how they are applied to each path:

    +------------------------------+----------------+----------------+
    | Before this commit           | chdir_path     | ret_path       |
    +------------------------------+----------------+----------------+
    | trim trailing slashes        | !strict        | !strict        |
    | tilde expansion              | !strict        | false          |
    | suffix guessing              | !strict        | !strict        |
    | gitfile expansion (< 2.6.3)  | !strict        | false          |
    | gitfile expansion (>= 2.6.3) | true           | strict         |
    +------------------------------+----------------+----------------+
    | With this commit             | chdir_path     | ret_path       |
    +------------------------------+----------------+----------------+
    | trim trailing slashes        | true           | true           |
    | tilde expansion              | !strict_prefix | false          |
    | suffix guessing              | !strict_suffix | !strict_suffix |
    | gitfile expansion            | true           | false          |
    +------------------------------+----------------+----------------+

The separation of "strict" into "strict_prefix" and "strict_suffix" is
necessary for git-daemon because it has separate --strict-paths (affects
prefix and suffix) and --user-path (just prefix) flags that can be toggled
separately.

In the other programs where enter_repo() is called, I continued the
existing behavior of tying the prefix and suffix strictness together
together; though I am not entirely sure that they should all be enabling
tilde expansion.  But for now, their behavior hasn't changed.

Signed-off-by: Luke Shumaker <lukeshu@sbcglobal.net>
---
 builtin/receive-pack.c   |   2 +-
 builtin/upload-archive.c |   2 +-
 cache.h                  |   2 +-
 daemon.c                 |  42 +++++++--------
 http-backend.c           |   2 +-
 path.c                   | 135 +++++++++++++++++++++++++----------------------
 upload-pack.c            |   2 +-
 7 files changed, 96 insertions(+), 91 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 011db00..f430e96 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1860,7 +1860,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 
 	setup_path();
 
-	if (!enter_repo(service_dir, 0))
+	if (!enter_repo(service_dir, 0, 0))
 		die("'%s' does not appear to be a git repository", service_dir);
 
 	git_config(receive_pack_config, NULL);
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 2caedf1..00d4ced 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -25,7 +25,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 	if (argc != 2)
 		usage(upload_archive_usage);
 
-	if (!enter_repo(argv[1], 0))
+	if (!enter_repo(argv[1], 0, 0))
 		die("'%s' does not appear to be a git repository", argv[1]);
 
 	/* put received options in sent_argv[] */
diff --git a/cache.h b/cache.h
index 4cba08e..6380be0 100644
--- a/cache.h
+++ b/cache.h
@@ -1024,7 +1024,7 @@ enum scld_error safe_create_leading_directories_const(const char *path);
 
 int mkdir_in_gitdir(const char *path);
 extern char *expand_user_path(const char *path);
-const char *enter_repo(const char *path, int strict);
+const char *enter_repo(const char *path, int strict_prefix, int strict_suffix);
 static inline int is_absolute_path(const char *path)
 {
 	return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
diff --git a/daemon.c b/daemon.c
index 425aad0..118d337 100644
--- a/daemon.c
+++ b/daemon.c
@@ -170,27 +170,23 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 		return NULL;
 	}
 
-	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
-			 * "~alice/%s/foo".
-			 */
-			int namlen, restlen = strlen(dir);
-			const char *slash = strchr(dir, '/');
-			if (!slash)
-				slash = dir + restlen;
-			namlen = slash - dir;
-			restlen -= namlen;
-			loginfo("userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>", user_path, dir, namlen, restlen, slash);
-			snprintf(rpath, PATH_MAX, "%.*s/%s%.*s",
-				 namlen, dir, user_path, restlen, slash);
-			dir = rpath;
-		}
+	if (*dir == '~' && *user_path && user_path[0] != '\0') {
+		/* Got either "~alice" or "~alice/foo";
+		 * rewrite them:
+		 *
+		 * ~alice     -> ~alice/user_dir
+		 * ~alice/foo -> ~alice/user_dir/foo
+		 */
+		int namlen, restlen = strlen(dir);
+		const char *slash = strchr(dir, '/');
+		if (!slash)
+			slash = dir + restlen;
+		namlen = slash - dir;
+		restlen -= namlen;
+		loginfo("userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>", user_path, dir, namlen, restlen, slash);
+		snprintf(rpath, PATH_MAX, "%.*s/%s%.*s",
+		         namlen, dir, user_path, restlen, slash);
+		dir = rpath;
 	}
 	else if (interpolated_path && hi->saw_extended_args) {
 		struct strbuf expanded_path = STRBUF_INIT;
@@ -223,14 +219,14 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
 		dir = rpath;
 	}
 
-	path = enter_repo(dir, strict_paths);
+	path = enter_repo(dir, strict_paths || !user_path, strict_paths);
 	if (!path && base_path && base_path_relaxed) {
 		/*
 		 * if we fail and base_path_relaxed is enabled, try without
 		 * prefixing the base path
 		 */
 		dir = directory;
-		path = enter_repo(dir, strict_paths);
+		path = enter_repo(dir, strict_paths || !user_path, strict_paths);
 	}
 
 	if (!path) {
diff --git a/http-backend.c b/http-backend.c
index adc8c8c..d71ed81 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -697,7 +697,7 @@ int cmd_main(int argc, const char **argv)
 		not_found(&hdr, "Request not supported: '%s'", dir);
 
 	setup_path();
-	if (!enter_repo(dir, 0))
+	if (!enter_repo(dir, 0, 0))
 		not_found(&hdr, "Not a git repository: '%s'", dir);
 	if (!getenv("GIT_HTTP_EXPORT_ALL") &&
 	    access("git-daemon-export-ok", F_OK) )
diff --git a/path.c b/path.c
index fe3c4d9..636349e 100644
--- a/path.c
+++ b/path.c
@@ -646,96 +646,105 @@ char *expand_user_path(const char *path)
 }
 
 /*
- * First, one directory to try is determined by the following algorithm.
+ * chdir() to, and set_git_dir() to the directory found with "path", using the
+ * following algorithm.
  *
- * (0) If "strict" is given, the path is used as given and no DWIM is
- *     done. Otherwise:
- * (1) "~/path" to mean path under the running user's home directory;
- * (2) "~user/path" to mean path under named user's home directory;
- * (3) "relative/path" to mean cwd relative directory; or
- * (4) "/absolute/path" to mean absolute directory.
+ * (0) "relative/path" to mean cwd relative directory; or
+ * (1) "/absolute/path" to mean absolute directory.
+ * (2) trim trailing slashes
  *
- * Unless "strict" is given, we check "%s/.git", "%s", "%s.git/.git", "%s.git"
- * in this order. We select the first one that is a valid git repository, and
- * chdir() to it. If none match, or we fail to chdir, we return NULL.
+ * Unless "strict_prefix" is given:
+ * (3) "~/path" to mean path under the running user's home directory;
+ * (4) "~user/path" to mean path under named user's home directory;
  *
- * If all goes well, we return the directory we used to chdir() (but
- * before ~user is expanded), avoiding getcwd() resolving symbolic
- * links.  User relative paths are also returned as they are given,
- * except DWIM suffixing.
+ * Unless "strict_suffix" is given:
+ * (5) check "%s/.git", "%s", "%s.git/.git", "%s.git" in this order. We select
+ *     the first one that is a valid git repository, and chdir() to it. If none
+ *     match, we return NULL.
+ *
+ * And then, unconditionally:
+ * (6) If the result is a .git file (instead of a directory) that points to a
+ *     directory elsewhere, follow it.
+ *
+ * If all goes well, we return the input path with suffix alteration (steps 2,
+ * 5, 6) applied, but without prefix alteration (user paths) applied. The
+ * returned value is a pointer to a static buffer.
  */
-const char *enter_repo(const char *path, int strict)
+const char *enter_repo(const char *path, int strict_prefix, int strict_suffix)
 {
-	static struct strbuf validated_path = STRBUF_INIT;
-	static struct strbuf used_path = STRBUF_INIT;
+	/* chdir_path is the path we chdir() to */
+	static struct strbuf chdir_path = STRBUF_INIT;
+	/* ret_path is the path we return */
+	static struct strbuf ret_path = STRBUF_INIT;
+
+	int len;
+	const char *gitfile;
 
 	if (!path)
 		return NULL;
 
-	if (!strict) {
+	len = strlen(path);
+
+	/* strip trailing slashes */
+	while ((1 < len) && (path[len-1] == '/'))
+		len--;
+
+	/*
+	 * We can handle arbitrary-sized buffers, but this remains as a
+	 * sanity check on untrusted input.
+	 */
+	if (PATH_MAX <= len)
+		return NULL;
+
+	strbuf_reset(&chdir_path);
+	strbuf_add(&chdir_path, path, len);
+	strbuf_reset(&ret_path);
+	strbuf_add(&ret_path, path, len);
+
+	if (!strict_prefix && chdir_path.buf[0] == '~') {
+		/* operate only on chdir_path */
+		char *newpath = expand_user_path(chdir_path.buf);
+		if (!newpath)
+			return NULL;
+		strbuf_attach(&chdir_path, newpath, strlen(newpath),
+		              strlen(newpath));
+	}
+
+	if (!strict_suffix) {
+		/* operate on both chdir_path and ret_path */
 		static const char *suffix[] = {
 			"/.git", "", ".git/.git", ".git", NULL,
 		};
-		const char *gitfile;
-		int len = strlen(path);
 		int i;
-		while ((1 < len) && (path[len-1] == '/'))
-			len--;
-
-		/*
-		 * We can handle arbitrary-sized buffers, but this remains as a
-		 * sanity check on untrusted input.
-		 */
-		if (PATH_MAX <= len)
-			return NULL;
-
-		strbuf_reset(&used_path);
-		strbuf_reset(&validated_path);
-		strbuf_add(&used_path, path, len);
-		strbuf_add(&validated_path, path, len);
-
-		if (used_path.buf[0] == '~') {
-			char *newpath = expand_user_path(used_path.buf);
-			if (!newpath)
-				return NULL;
-			strbuf_attach(&used_path, newpath, strlen(newpath),
-				      strlen(newpath));
-		}
 		for (i = 0; suffix[i]; i++) {
 			struct stat st;
-			size_t baselen = used_path.len;
-			strbuf_addstr(&used_path, suffix[i]);
-			if (!stat(used_path.buf, &st) &&
+			size_t baselen = chdir_path.len;
+			strbuf_addstr(&chdir_path, suffix[i]);
+			if (!stat(chdir_path.buf, &st) &&
 			    (S_ISREG(st.st_mode) ||
-			    (S_ISDIR(st.st_mode) && is_git_directory(used_path.buf)))) {
-				strbuf_addstr(&validated_path, suffix[i]);
+			     (S_ISDIR(st.st_mode) && is_git_directory(chdir_path.buf)))) {
+				strbuf_addstr(&ret_path, suffix[i]);
 				break;
 			}
-			strbuf_setlen(&used_path, baselen);
+			strbuf_setlen(&chdir_path, baselen);
 		}
 		if (!suffix[i])
 			return NULL;
-		gitfile = read_gitfile(used_path.buf);
-		if (gitfile) {
-			strbuf_reset(&used_path);
-			strbuf_addstr(&used_path, gitfile);
-		}
-		if (chdir(used_path.buf))
-			return NULL;
-		path = validated_path.buf;
 	}
-	else {
-		const char *gitfile = read_gitfile(path);
-		if (gitfile)
-			path = gitfile;
-		if (chdir(path))
-			return NULL;
+
+	gitfile = read_gitfile(chdir_path.buf);
+	if (gitfile) {
+		/* operate only on chdir_path */
+		strbuf_reset(&chdir_path);
+		strbuf_addstr(&chdir_path, gitfile);
 	}
+	if (chdir(chdir_path.buf))
+		return NULL;
 
 	if (is_git_directory(".")) {
 		set_git_dir(".");
 		check_repository_format();
-		return path;
+		return ret_path.buf;
 	}
 
 	return NULL;
diff --git a/upload-pack.c b/upload-pack.c
index ca7f941..c73b776 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -859,7 +859,7 @@ int cmd_main(int argc, const char **argv)
 
 	dir = argv[0];
 
-	if (!enter_repo(dir, strict))
+	if (!enter_repo(dir, strict, strict))
 		die("'%s' does not appear to be a git repository", dir);
 
 	git_config(upload_pack_config, NULL);
-- 
2.10.0

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

[Junio C Hamano]

Luke Shumaker <lukeshu@sbcglobal.net> writes:

> The superficial aspect of this change is that git-daemon now allows paths
> that start with a "~".  Previously, if git-daemon was run with
> "--base-path=/srv/git", it was impossible to get it to serve
> "/srv/git/~foo/bar.git".

I am not sure I understand what you are saying here.  Do you mean

    I have a path on my server /srv/git/~foo/bar.git; the tilde does
    not mean anything special--it is just a byte in a valid pathname.

    I want to allow my users to say

	git fetch git://my.server/~foo/bar.git

    and fetch from that repository, but "git daemon" lacks the way
    to configure to allow it.

If that is the case, what happens instead?  Due to the leading
"~foo/" getting noticed as an attempt to use the user-path expansion
it is not treated as just a literal character?

I am not sure if it is even a bug.  As you can easily lose that
tilde that appears in front of subdirectory of /srv/git/ or replace
it with something else (e.g. "u/"), this smells like "Don't do it if
it hurts" thing to me.

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

[Luke Shumaker]

On Tue, 18 Oct 2016 13:08:45 -0400,
Junio C Hamano wrote:
> 
> Luke Shumaker <lukeshu@sbcglobal.net> writes:
> 
> > The superficial aspect of this change is that git-daemon now allows paths
> > that start with a "~".  Previously, if git-daemon was run with
> > "--base-path=/srv/git", it was impossible to get it to serve
> > "/srv/git/~foo/bar.git".
> 
> I am not sure I understand what you are saying here.  Do you mean
> 
>     I have a path on my server /srv/git/~foo/bar.git; the tilde does
>     not mean anything special--it is just a byte in a valid pathname.
> 
>     I want to allow my users to say
> 
> 	git fetch git://my.server/~foo/bar.git
> 
>     and fetch from that repository, but "git daemon" lacks the way
>     to configure to allow it.

Yes, that is what I am saying.

> If that is the case, what happens instead?  Due to the leading
> "~foo/" getting noticed as an attempt to use the user-path expansion
> it is not treated as just a literal character?

What happens instead is

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

which to the user looks like

	git clone git://my.server/~foo/bar.git
	Cloning into 'bar'...
	fatal: remote error: access denied or repository not exported: ~foo/bar.git

> I am not sure if it is even a bug.  As you can easily lose that
> tilde that appears in front of subdirectory of /srv/git/ or replace
> it with something else (e.g. "u/"), this smells like "Don't do it if
> it hurts" thing to me.

I buy into "Don't do it if it hurts", but that doesn't mean it's not a
bug on an uncommon edge-case.  Note that it doesn't hurt with
git-shell or cgit (I haven't checked with gitweb).

Many programs (especially shell scripts) fail to deal with filenames
containing a space.  "Don't put spaces in filenames if it hurts".
It's still a bug in the program.

Similarly, `git gui` used to not be able to add a file in a directory
starting with '~' (when one clicked the file named "~foo/bar", it
said something along the lines of "/home/~foo/bar is outside
repository"), and one had to use `git add '~foo/bar` directly.
"Don't do it if it hurts"; it was still a bug.

  Aside: one (somewhat silly) non-user reason that I've seen for a
  directory to start with '~' is that it sorts after all other ASCII
  characters; it moves the directory to the end of any lists.

-- 
Happy hacking,
~ Luke Shumaker

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

[Duy Nguyen]

On Wed, Oct 19, 2016 at 1:05 AM, Luke Shumaker <lukeshu@sbcglobal.net> wrote:
>> I am not sure if it is even a bug.  As you can easily lose that
>> tilde that appears in front of subdirectory of /srv/git/ or replace
>> it with something else (e.g. "u/"), this smells like "Don't do it if
>> it hurts" thing to me.
>
> I buy into "Don't do it if it hurts", but that doesn't mean it's not a
> bug on an uncommon edge-case.

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)
-- 
Duy

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

[Junio C Hamano]

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.

Also I am a bit worried about the change in the semantics.  While it
is not the best way to achieve this, the server operators could use
it as a way to add directories whose contents need to be hidden to
give them names that begin with a tilde without enabling user-path
expansion.  This change may be a new and useful feature for Luke,
but to these server operators this change can be a new bug---it is
probably a minor new bug as they can work it around by using other
means to hide these directories, though.

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

[Junio C Hamano]

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

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

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

[Jeff King]

On Fri, Oct 21, 2016 at 11:23:53AM -0700, Junio C Hamano wrote:

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

Thanks for explaining this. It is rather gross, but I think your
less-invasive patch is the best we could do given the client behavior.
And it's more what I would have expected based on the original problem
description.

> As I said already, I am not sure if this is a good change, though.

I also am on the fence. I'm not sure I understand a compelling reason to
have a non-interpolated "~" in the repo path name. Sure, it's a
limitation, but why does anybody care?

> @@ -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;
> +	}

I know you are following existing convention in this function to use an
unchecked snprintf(), but it makes me wonder what kind of mischief a
client could get up to by silently truncating via snprintf. This
function is, after all, supposed to be checking the quality of the
incoming path.

xsnprintf() is probably too blunt a hammer, but:

  if (snprintf(rpath, PATH_MAX, ...) >= PATH_MAX) {
	logerror("path too long");
	return NULL;
  }

in various places may be appropriate.

-Peff