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

> Duy Nguyen <pclo...@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 
        (cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git &&

Reply via email to