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