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

2016-10-18 Thread Luke Shumaker
On Tue, 18 Oct 2016 13:08:45 -0400,
Junio C Hamano wrote:
> 
> Luke Shumaker <luke...@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


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

2016-10-18 Thread 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 <luke...@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(