Junio C Hamano <[email protected]> writes:
> Duy Nguyen <[email protected]> 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 &&
