{"thread":{"id":"44320","subject":"[PATCH] daemon, path.c: fix a bug with ~ in repo paths","startedAt":"2016-10-18T15:13:28Z","lastAt":"2016-10-22T03:27:00Z","messageCount":7,"participants":["Luke Shumaker","Junio C Hamano","Duy Nguyen","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"304338","messageId":"20161018150629.23205-1-lukeshu@sbcglobal.net","threadId":"44320","inReplyTo":null,"subject":"[PATCH] daemon, path.c: fix a bug with ~ in repo paths","fromName":"Luke Shumaker","fromEmail":"lukeshu@sbcglobal.net","sentAt":"2016-10-18T15:06:29Z","receivedAt":"2016-10-18T15:13:28Z","isPatch":true,"sender":{"key":"lukeshu@sbcglobal.net","avatar":null},"body":"The superficial aspect of this change is that git-daemon now allows paths\nthat start with a \"~\".  Previously, if git-daemon was run with\n\"--base-path=/srv/git\", it was impossible to get it to serve\n\"/srv/git/~foo/bar.git\".  An odd edge-case that was broken.\n\nBut from a source-code standpoint, the change is in path.c:enter_repo().  I\nhave adjusted it to take separate \"strict_prefix\" and \"strict_suffix\"\narguments, rather than a single \"strict\" argument.\n\nI also make it clearer what the purpose of each path buffer is for, by\nrenaming them to chdir_path and ret_path; chdir_path is the path that we\npass to chdir(); return_path is the path we return to the user.  Using this\nnomenclature, we can more easily explain the behavior of the function.\nThere are 3 DWIM measures that enter_repo() provides: tilde expansion,\nsuffix guessing, and gitfile expansion; it also trims trailing slashes.\nHere is how they are applied to each path:\n\n    +------------------------------+----------------+----------------+\n    | Before this commit           | chdir_path     | ret_path       |\n    +------------------------------+----------------+----------------+\n    | trim trailing slashes        | !strict        | !strict        |\n    | tilde expansion              | !strict        | false          |\n    | suffix guessing              | !strict        | !strict        |\n    | gitfile expansion (< 2.6.3)  | !strict        | false          |\n    | gitfile expansion (>= 2.6.3) | true           | strict         |\n    +------------------------------+----------------+----------------+\n    | With this commit             | chdir_path     | ret_path       |\n    +------------------------------+----------------+----------------+\n    | trim trailing slashes        | true           | true           |\n    | tilde expansion              | !strict_prefix | false          |\n    | suffix guessing              | !strict_suffix | !strict_suffix |\n    | gitfile expansion            | true           | false          |\n    +------------------------------+----------------+----------------+\n\nThe separation of \"strict\" into \"strict_prefix\" and \"strict_suffix\" is\nnecessary for git-daemon because it has separate --strict-paths (affects\nprefix and suffix) and --user-path (just prefix) flags that can be toggled\nseparately.\n\nIn the other programs where enter_repo() is called, I continued the\nexisting behavior of tying the prefix and suffix strictness together\ntogether; though I am not entirely sure that they should all be enabling\ntilde expansion.  But for now, their behavior hasn't changed.\n\nSigned-off-by: Luke Shumaker <lukeshu@sbcglobal.net>\n---\n builtin/receive-pack.c   |   2 +-\n builtin/upload-archive.c |   2 +-\n cache.h                  |   2 +-\n daemon.c                 |  42 +++++++--------\n http-backend.c           |   2 +-\n path.c                   | 135 +++++++++++++++++++++++++----------------------\n upload-pack.c            |   2 +-\n 7 files changed, 96 insertions(+), 91 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 011db00..f430e96 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1860,7 +1860,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \n \tsetup_path();\n \n-\tif (!enter_repo(service_dir, 0))\n+\tif (!enter_repo(service_dir, 0, 0))\n \t\tdie(\"'%s' does not appear to be a git repository\", service_dir);\n \n \tgit_config(receive_pack_config, NULL);\ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 2caedf1..00d4ced 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -25,7 +25,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n \tif (argc != 2)\n \t\tusage(upload_archive_usage);\n \n-\tif (!enter_repo(argv[1], 0))\n+\tif (!enter_repo(argv[1], 0, 0))\n \t\tdie(\"'%s' does not appear to be a git repository\", argv[1]);\n \n \t/* put received options in sent_argv[] */\ndiff --git a/cache.h b/cache.h\nindex 4cba08e..6380be0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1024,7 +1024,7 @@ enum scld_error safe_create_leading_directories_const(const char *path);\n \n int mkdir_in_gitdir(const char *path);\n extern char *expand_user_path(const char *path);\n-const char *enter_repo(const char *path, int strict);\n+const char *enter_repo(const char *path, int strict_prefix, int strict_suffix);\n static inline int is_absolute_path(const char *path)\n {\n \treturn is_dir_sep(path[0]) || has_dos_drive_prefix(path);\ndiff --git a/daemon.c b/daemon.c\nindex 425aad0..118d337 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -170,27 +170,23 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \t\treturn NULL;\n \t}\n \n-\tif (*dir == '~') {\n-\t\tif (!user_path) {\n-\t\t\tlogerror(\"'%s': User-path not allowed\", dir);\n-\t\t\treturn NULL;\n-\t\t}\n-\t\tif (*user_path) {\n-\t\t\t/* Got either \"~alice\" or \"~alice/foo\";\n-\t\t\t * rewrite them to \"~alice/%s\" or\n-\t\t\t * \"~alice/%s/foo\".\n-\t\t\t */\n-\t\t\tint namlen, restlen = strlen(dir);\n-\t\t\tconst char *slash = strchr(dir, '/');\n-\t\t\tif (!slash)\n-\t\t\t\tslash = dir + restlen;\n-\t\t\tnamlen = slash - dir;\n-\t\t\trestlen -= namlen;\n-\t\t\tloginfo(\"userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>\", user_path, dir, namlen, restlen, slash);\n-\t\t\tsnprintf(rpath, PATH_MAX, \"%.*s/%s%.*s\",\n-\t\t\t\t namlen, dir, user_path, restlen, slash);\n-\t\t\tdir = rpath;\n-\t\t}\n+\tif (*dir == '~' && *user_path && user_path[0] != '\\0') {\n+\t\t/* Got either \"~alice\" or \"~alice/foo\";\n+\t\t * rewrite them:\n+\t\t *\n+\t\t * ~alice     -> ~alice/user_dir\n+\t\t * ~alice/foo -> ~alice/user_dir/foo\n+\t\t */\n+\t\tint namlen, restlen = strlen(dir);\n+\t\tconst char *slash = strchr(dir, '/');\n+\t\tif (!slash)\n+\t\t\tslash = dir + restlen;\n+\t\tnamlen = slash - dir;\n+\t\trestlen -= namlen;\n+\t\tloginfo(\"userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>\", user_path, dir, namlen, restlen, slash);\n+\t\tsnprintf(rpath, PATH_MAX, \"%.*s/%s%.*s\",\n+\t\t         namlen, dir, user_path, restlen, slash);\n+\t\tdir = rpath;\n \t}\n \telse if (interpolated_path && hi->saw_extended_args) {\n \t\tstruct strbuf expanded_path = STRBUF_INIT;\n@@ -223,14 +219,14 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \t\tdir = rpath;\n \t}\n \n-\tpath = enter_repo(dir, strict_paths);\n+\tpath = enter_repo(dir, strict_paths || !user_path, strict_paths);\n \tif (!path && base_path && base_path_relaxed) {\n \t\t/*\n \t\t * if we fail and base_path_relaxed is enabled, try without\n \t\t * prefixing the base path\n \t\t */\n \t\tdir = directory;\n-\t\tpath = enter_repo(dir, strict_paths);\n+\t\tpath = enter_repo(dir, strict_paths || !user_path, strict_paths);\n \t}\n \n \tif (!path) {\ndiff --git a/http-backend.c b/http-backend.c\nindex adc8c8c..d71ed81 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -697,7 +697,7 @@ int cmd_main(int argc, const char **argv)\n \t\tnot_found(&hdr, \"Request not supported: '%s'\", dir);\n \n \tsetup_path();\n-\tif (!enter_repo(dir, 0))\n+\tif (!enter_repo(dir, 0, 0))\n \t\tnot_found(&hdr, \"Not a git repository: '%s'\", dir);\n \tif (!getenv(\"GIT_HTTP_EXPORT_ALL\") &&\n \t    access(\"git-daemon-export-ok\", F_OK) )\ndiff --git a/path.c b/path.c\nindex fe3c4d9..636349e 100644\n--- a/path.c\n+++ b/path.c\n@@ -646,96 +646,105 @@ char *expand_user_path(const char *path)\n }\n \n /*\n- * First, one directory to try is determined by the following algorithm.\n+ * chdir() to, and set_git_dir() to the directory found with \"path\", using the\n+ * following algorithm.\n  *\n- * (0) If \"strict\" is given, the path is used as given and no DWIM is\n- *     done. Otherwise:\n- * (1) \"~/path\" to mean path under the running user's home directory;\n- * (2) \"~user/path\" to mean path under named user's home directory;\n- * (3) \"relative/path\" to mean cwd relative directory; or\n- * (4) \"/absolute/path\" to mean absolute directory.\n+ * (0) \"relative/path\" to mean cwd relative directory; or\n+ * (1) \"/absolute/path\" to mean absolute directory.\n+ * (2) trim trailing slashes\n  *\n- * Unless \"strict\" is given, we check \"%s/.git\", \"%s\", \"%s.git/.git\", \"%s.git\"\n- * in this order. We select the first one that is a valid git repository, and\n- * chdir() to it. If none match, or we fail to chdir, we return NULL.\n+ * Unless \"strict_prefix\" is given:\n+ * (3) \"~/path\" to mean path under the running user's home directory;\n+ * (4) \"~user/path\" to mean path under named user's home directory;\n  *\n- * If all goes well, we return the directory we used to chdir() (but\n- * before ~user is expanded), avoiding getcwd() resolving symbolic\n- * links.  User relative paths are also returned as they are given,\n- * except DWIM suffixing.\n+ * Unless \"strict_suffix\" is given:\n+ * (5) check \"%s/.git\", \"%s\", \"%s.git/.git\", \"%s.git\" in this order. We select\n+ *     the first one that is a valid git repository, and chdir() to it. If none\n+ *     match, we return NULL.\n+ *\n+ * And then, unconditionally:\n+ * (6) If the result is a .git file (instead of a directory) that points to a\n+ *     directory elsewhere, follow it.\n+ *\n+ * If all goes well, we return the input path with suffix alteration (steps 2,\n+ * 5, 6) applied, but without prefix alteration (user paths) applied. The\n+ * returned value is a pointer to a static buffer.\n  */\n-const char *enter_repo(const char *path, int strict)\n+const char *enter_repo(const char *path, int strict_prefix, int strict_suffix)\n {\n-\tstatic struct strbuf validated_path = STRBUF_INIT;\n-\tstatic struct strbuf used_path = STRBUF_INIT;\n+\t/* chdir_path is the path we chdir() to */\n+\tstatic struct strbuf chdir_path = STRBUF_INIT;\n+\t/* ret_path is the path we return */\n+\tstatic struct strbuf ret_path = STRBUF_INIT;\n+\n+\tint len;\n+\tconst char *gitfile;\n \n \tif (!path)\n \t\treturn NULL;\n \n-\tif (!strict) {\n+\tlen = strlen(path);\n+\n+\t/* strip trailing slashes */\n+\twhile ((1 < len) && (path[len-1] == '/'))\n+\t\tlen--;\n+\n+\t/*\n+\t * We can handle arbitrary-sized buffers, but this remains as a\n+\t * sanity check on untrusted input.\n+\t */\n+\tif (PATH_MAX <= len)\n+\t\treturn NULL;\n+\n+\tstrbuf_reset(&chdir_path);\n+\tstrbuf_add(&chdir_path, path, len);\n+\tstrbuf_reset(&ret_path);\n+\tstrbuf_add(&ret_path, path, len);\n+\n+\tif (!strict_prefix && chdir_path.buf[0] == '~') {\n+\t\t/* operate only on chdir_path */\n+\t\tchar *newpath = expand_user_path(chdir_path.buf);\n+\t\tif (!newpath)\n+\t\t\treturn NULL;\n+\t\tstrbuf_attach(&chdir_path, newpath, strlen(newpath),\n+\t\t              strlen(newpath));\n+\t}\n+\n+\tif (!strict_suffix) {\n+\t\t/* operate on both chdir_path and ret_path */\n \t\tstatic const char *suffix[] = {\n \t\t\t\"/.git\", \"\", \".git/.git\", \".git\", NULL,\n \t\t};\n-\t\tconst char *gitfile;\n-\t\tint len = strlen(path);\n \t\tint i;\n-\t\twhile ((1 < len) && (path[len-1] == '/'))\n-\t\t\tlen--;\n-\n-\t\t/*\n-\t\t * We can handle arbitrary-sized buffers, but this remains as a\n-\t\t * sanity check on untrusted input.\n-\t\t */\n-\t\tif (PATH_MAX <= len)\n-\t\t\treturn NULL;\n-\n-\t\tstrbuf_reset(&used_path);\n-\t\tstrbuf_reset(&validated_path);\n-\t\tstrbuf_add(&used_path, path, len);\n-\t\tstrbuf_add(&validated_path, path, len);\n-\n-\t\tif (used_path.buf[0] == '~') {\n-\t\t\tchar *newpath = expand_user_path(used_path.buf);\n-\t\t\tif (!newpath)\n-\t\t\t\treturn NULL;\n-\t\t\tstrbuf_attach(&used_path, newpath, strlen(newpath),\n-\t\t\t\t      strlen(newpath));\n-\t\t}\n \t\tfor (i = 0; suffix[i]; i++) {\n \t\t\tstruct stat st;\n-\t\t\tsize_t baselen = used_path.len;\n-\t\t\tstrbuf_addstr(&used_path, suffix[i]);\n-\t\t\tif (!stat(used_path.buf, &st) &&\n+\t\t\tsize_t baselen = chdir_path.len;\n+\t\t\tstrbuf_addstr(&chdir_path, suffix[i]);\n+\t\t\tif (!stat(chdir_path.buf, &st) &&\n \t\t\t    (S_ISREG(st.st_mode) ||\n-\t\t\t    (S_ISDIR(st.st_mode) && is_git_directory(used_path.buf)))) {\n-\t\t\t\tstrbuf_addstr(&validated_path, suffix[i]);\n+\t\t\t     (S_ISDIR(st.st_mode) && is_git_directory(chdir_path.buf)))) {\n+\t\t\t\tstrbuf_addstr(&ret_path, suffix[i]);\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t\tstrbuf_setlen(&used_path, baselen);\n+\t\t\tstrbuf_setlen(&chdir_path, baselen);\n \t\t}\n \t\tif (!suffix[i])\n \t\t\treturn NULL;\n-\t\tgitfile = read_gitfile(used_path.buf);\n-\t\tif (gitfile) {\n-\t\t\tstrbuf_reset(&used_path);\n-\t\t\tstrbuf_addstr(&used_path, gitfile);\n-\t\t}\n-\t\tif (chdir(used_path.buf))\n-\t\t\treturn NULL;\n-\t\tpath = validated_path.buf;\n \t}\n-\telse {\n-\t\tconst char *gitfile = read_gitfile(path);\n-\t\tif (gitfile)\n-\t\t\tpath = gitfile;\n-\t\tif (chdir(path))\n-\t\t\treturn NULL;\n+\n+\tgitfile = read_gitfile(chdir_path.buf);\n+\tif (gitfile) {\n+\t\t/* operate only on chdir_path */\n+\t\tstrbuf_reset(&chdir_path);\n+\t\tstrbuf_addstr(&chdir_path, gitfile);\n \t}\n+\tif (chdir(chdir_path.buf))\n+\t\treturn NULL;\n \n \tif (is_git_directory(\".\")) {\n \t\tset_git_dir(\".\");\n \t\tcheck_repository_format();\n-\t\treturn path;\n+\t\treturn ret_path.buf;\n \t}\n \n \treturn NULL;\ndiff --git a/upload-pack.c b/upload-pack.c\nindex ca7f941..c73b776 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -859,7 +859,7 @@ int cmd_main(int argc, const char **argv)\n \n \tdir = argv[0];\n \n-\tif (!enter_repo(dir, strict))\n+\tif (!enter_repo(dir, strict, strict))\n \t\tdie(\"'%s' does not appear to be a git repository\", dir);\n \n \tgit_config(upload_pack_config, NULL);\n-- \n2.10.0\n\n"},{"id":"304349","messageId":"xmqqvawppote.fsf@gitster.mtv.corp.google.com","threadId":"44320","inReplyTo":"20161018150629.23205-1-lukeshu@sbcglobal.net","subject":"Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-18T17:08:45Z","receivedAt":"2016-10-18T17:09:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Shumaker <lukeshu@sbcglobal.net> writes:\n\n> The superficial aspect of this change is that git-daemon now allows paths\n> that start with a \"~\".  Previously, if git-daemon was run with\n> \"--base-path=/srv/git\", it was impossible to get it to serve\n> \"/srv/git/~foo/bar.git\".\n\nI am not sure I understand what you are saying here.  Do you mean\n\n    I have a path on my server /srv/git/~foo/bar.git; the tilde does\n    not mean anything special--it is just a byte in a valid pathname.\n\n    I want to allow my users to say\n\n\tgit fetch git://my.server/~foo/bar.git\n\n    and fetch from that repository, but \"git daemon\" lacks the way\n    to configure to allow it.\n\nIf that is the case, what happens instead?  Due to the leading\n\"~foo/\" getting noticed as an attempt to use the user-path expansion\nit is not treated as just a literal character?\n\nI am not sure if it is even a bug.  As you can easily lose that\ntilde that appears in front of subdirectory of /srv/git/ or replace\nit with something else (e.g. \"u/\"), this smells like \"Don't do it if\nit hurts\" thing to me.\n\n"},{"id":"304355","messageId":"87pomxildc.wl-lukeshu@sbcglobal.net","threadId":"44320","inReplyTo":"xmqqvawppote.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths","fromName":"Luke Shumaker","fromEmail":"lukeshu@sbcglobal.net","sentAt":"2016-10-18T18:05:03Z","receivedAt":"2016-10-18T18:05:40Z","isPatch":true,"sender":{"key":"lukeshu@sbcglobal.net","avatar":null},"body":"On Tue, 18 Oct 2016 13:08:45 -0400,\nJunio C Hamano wrote:\n> \n> Luke Shumaker <lukeshu@sbcglobal.net> writes:\n> \n> > The superficial aspect of this change is that git-daemon now allows paths\n> > that start with a \"~\".  Previously, if git-daemon was run with\n> > \"--base-path=/srv/git\", it was impossible to get it to serve\n> > \"/srv/git/~foo/bar.git\".\n> \n> I am not sure I understand what you are saying here.  Do you mean\n> \n>     I have a path on my server /srv/git/~foo/bar.git; the tilde does\n>     not mean anything special--it is just a byte in a valid pathname.\n> \n>     I want to allow my users to say\n> \n> \tgit fetch git://my.server/~foo/bar.git\n> \n>     and fetch from that repository, but \"git daemon\" lacks the way\n>     to configure to allow it.\n\nYes, that is what I am saying.\n\n> If that is the case, what happens instead?  Due to the leading\n> \"~foo/\" getting noticed as an attempt to use the user-path expansion\n> it is not treated as just a literal character?\n\nWhat happens instead is\n\n\tif (*dir == '~') {\n\t\tif (!user_path) {\n\t\t\tlogerror(\"'%s': User-path not allowed\", dir);\n\t\t\treturn NULL;\n\t\t}\n\nwhich to the user looks like\n\n\tgit clone git://my.server/~foo/bar.git\n\tCloning into 'bar'...\n\tfatal: remote error: access denied or repository not exported: ~foo/bar.git\n\n> I am not sure if it is even a bug.  As you can easily lose that\n> tilde that appears in front of subdirectory of /srv/git/ or replace\n> it with something else (e.g. \"u/\"), this smells like \"Don't do it if\n> it hurts\" thing to me.\n\nI buy into \"Don't do it if it hurts\", but that doesn't mean it's not a\nbug on an uncommon edge-case.  Note that it doesn't hurt with\ngit-shell or cgit (I haven't checked with gitweb).\n\nMany programs (especially shell scripts) fail to deal with filenames\ncontaining a space.  \"Don't put spaces in filenames if it hurts\".\nIt's still a bug in the program.\n\nSimilarly, `git gui` used to not be able to add a file in a directory\nstarting with '~' (when one clicked the file named \"~foo/bar\", it\nsaid something along the lines of \"/home/~foo/bar is outside\nrepository\"), and one had to use `git add '~foo/bar` directly.\n\"Don't do it if it hurts\"; it was still a bug.\n\n  Aside: one (somewhat silly) non-user reason that I've seen for a\n  directory to start with '~' is that it sorts after all other ASCII\n  characters; it moves the directory to the end of any lists.\n\n-- \nHappy hacking,\n~ Luke Shumaker\n"},{"id":"304385","messageId":"CACsJy8AzWqJ_9JQ_BGnAB3E3GrGSTS40z8umbtO6mQJukWOooQ@mail.gmail.com","threadId":"44320","inReplyTo":"87pomxildc.wl-lukeshu@sbcglobal.net","subject":"Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-10-19T14:12:37Z","receivedAt":"2016-10-19T14:13:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Oct 19, 2016 at 1:05 AM, Luke Shumaker <lukeshu@sbcglobal.net> wrote:\n>> I am not sure if it is even a bug.  As you can easily lose that\n>> tilde that appears in front of subdirectory of /srv/git/ or replace\n>> it with something else (e.g. \"u/\"), this smells like \"Don't do it if\n>> it hurts\" thing to me.\n>\n> I buy into \"Don't do it if it hurts\", but that doesn't mean it's not a\n> bug on an uncommon edge-case.\n\nThe amount of changes is unbelievable for fixing such a rare case\nthough. I wonder if we can just detect this in daemon.c and pass\n\"./~foo/bar\" instead of \"~foo/bar\" to enter_repo() in non-strict mode\nto \"disable\" expand_user_path(). If it works, it's much simpler\nchanges (even though a bit hacky)\n-- \nDuy\n"},{"id":"304616","messageId":"xmqqlgxhd77u.fsf@gitster.mtv.corp.google.com","threadId":"44320","inReplyTo":"CACsJy8AzWqJ_9JQ_BGnAB3E3GrGSTS40z8umbtO6mQJukWOooQ@mail.gmail.com","subject":"Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-21T15:58:45Z","receivedAt":"2016-10-21T15:59:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> The amount of changes is unbelievable for fixing such a rare case\n> though. I wonder if we can just detect this in daemon.c and pass\n> \"./~foo/bar\" instead of \"~foo/bar\" to enter_repo() in non-strict mode\n> to \"disable\" expand_user_path(). If it works, it's much simpler\n> changes (even though a bit hacky)\n\nConceptually, it ought to be updating the code that says \"Does it\nbegin with a tilde?  Then try to do user-path expansion and fail if\nthat is not enabled and if it succeeds use the resulting directory\"\nto \"Is user-path enabled and does it begin with a tilde?  Then try\nto do user-path expansion and if it succeeds use the resulting\ndirectory\".  Compared to that mental model we have with this\ncodepath, I agree that the change does look quite invasive and\nlarge.\n\nIt is OK for a change to be large, as long as the end result is\neasier to read and understand than the original.  I am undecided if\nthat is the case with this patch, though.\n\nAlso I am a bit worried about the change in the semantics.  While it\nis not the best way to achieve this, the server operators could use\nit as a way to add directories whose contents need to be hidden to\ngive them names that begin with a tilde without enabling user-path\nexpansion.  This change may be a new and useful feature for Luke,\nbut to these server operators this change can be a new bug---it is\nprobably a minor new bug as they can work it around by using other\nmeans to hide these directories, though.\n"},{"id":"304635","messageId":"xmqqvawlblxi.fsf@gitster.mtv.corp.google.com","threadId":"44320","inReplyTo":"xmqqlgxhd77u.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-21T18:23:53Z","receivedAt":"2016-10-21T18:24:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> The amount of changes is unbelievable for fixing such a rare case\n>> though. I wonder if we can just detect this in daemon.c and pass\n>> \"./~foo/bar\" instead of \"~foo/bar\" to enter_repo() in non-strict mode\n>> to \"disable\" expand_user_path(). If it works, it's much simpler\n>> changes (even though a bit hacky)\n>\n> Conceptually, it ought to be updating the code that says \"Does it\n> begin with a tilde?  Then try to do user-path expansion and fail if\n> that is not enabled and if it succeeds use the resulting directory\"\n> to \"Is user-path enabled and does it begin with a tilde?  Then try\n> to do user-path expansion and if it succeeds use the resulting\n> directory\".  Compared to that mental model we have with this\n> codepath, I agree that the change does look quite invasive and\n> large.\n>\n> It is OK for a change to be large, as long as the end result is\n> easier to read and understand than the original.  I am undecided if\n> that is the case with this patch, though.\n\nI am still not convinced that this is a bugfix (as opposed to \"add a\nnew feature to please Luke while regressing it for others\"), but the\n\"this looks too invasive\" made me look at the codepath involved.\n\nThere is this code in daemon.c::path_ok() that takes \"dir\" and ends\nup calling enter_repo().\n\n\tif (*dir == '~') {\n\t\tif (!user_path) {\n\t\t\tlogerror(\"'%s': User-path not allowed\", dir);\n\t\t\treturn NULL;\n\t\t}\n\nAt first glance, it ought to be a single-liner\n\n-\tif (*dir == '~') {\n+\tif (user_path && *dir == '~') {\n\nto make 'git://site/~foo.git\" go to \"/var/scm/~foo.git\" when base\npath is set to /var/scm/.  \n\nUnfortunately that is not sufficient.\n\nA request to \"git://<site>/<string>\", depending on <string>, results\nin \"directory\" given to path_ok() in a bit different forms.  Namely,\nconnect.c::parse_connect_url() gives\n\n    URL\t\t\t\tdirectory\n    git://site/nouser.git\t/nouser.git\n    git://site/~nouser.git\t~nouser.git\n\nby special casing \"~user\" syntax (in other words, a pathname that\nbegins with a tilde _is_ special cased, and tilde is not considered\na normal character that can be in a pathname).  Note the lack of\nleading slash in the second one.\n\nBecause that is how the shipped clients work, the daemon needs a bit\nof adjustment, because interpolation and base-path prefix codepaths\nwants to accept only paths that begin with a slash, and relies on\nthe slash when interpolating it in the template or concatenating it\nto the base (e.g. roughly \"sprintf(buf, \"%s%s\", base_path, dir)\").\n\nI came up with the following as a less invasive patch.  There no\nlonger is the ase where \"user-path not allowed\" error is given,\nas treating tilde as just a normal pathname character even when it\nappears at the beginning is the whole point of this change.\n\nAs I said already, I am not sure if this is a good change, though.\n daemon.c              |  9 +++++----\n t/t5570-git-daemon.sh | 11 +++++++++++\n 2 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex afce1b9b7f..e560a535af 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -160,6 +160,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n {\n \tstatic char rpath[PATH_MAX];\n \tstatic char interp_path[PATH_MAX];\n+\tchar tilde_path[PATH_MAX];\n \tconst char *path;\n \tconst char *dir;\n \n@@ -170,11 +171,11 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n \t\treturn NULL;\n \t}\n \n+\tif (!user_path && *dir == '~') {\n+\t\tsnprintf(tilde_path, PATH_MAX, \"/%s\", dir);\n+\t\tdir = tilde_path;\n+\t}\n \tif (*dir == '~') {\n-\t\tif (!user_path) {\n-\t\t\tlogerror(\"'%s': User-path not allowed\", dir);\n-\t\t\treturn NULL;\n-\t\t}\n \t\tif (*user_path) {\n \t\t\t/* Got either \"~alice\" or \"~alice/foo\";\n \t\t\t * rewrite them to \"~alice/%s\" or\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 225a022e8a..b6d2b9a001 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -65,6 +65,17 @@ test_expect_success 'remote detects correct HEAD' '\n \t)\n '\n \n+test_expect_success 'tilde is a normal character without --user-path' '\n+\tnouser=\"~nouser.git\" &&\n+\tnouser_repo=\"$GIT_DAEMON_DOCUMENT_ROOT_PATH/$nouser\" &&\n+\tmkdir \"$nouser_repo\" &&\n+\tgit -C \"$nouser_repo\" --bare init &&\n+\t>\"$nouser_repo/git-daemon-export-ok\" &&\n+\tgit push \"$nouser_repo\" master:master &&\n+\n+\tgit ls-remote \"$GIT_DAEMON_URL/$nouser\"\n+'\n+\n test_expect_success 'prepare pack objects' '\n \tcp -R \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo.git \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n \t(cd \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n"},{"id":"304664","messageId":"20161022032652.nfdj6mcy6mi6anx5@sigill.intra.peff.net","threadId":"44320","inReplyTo":"xmqqvawlblxi.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-22T03:26:53Z","receivedAt":"2016-10-22T03:27:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2016 at 11:23:53AM -0700, Junio C Hamano wrote:\n\n> A request to \"git://<site>/<string>\", depending on <string>, results\n> in \"directory\" given to path_ok() in a bit different forms.  Namely,\n> connect.c::parse_connect_url() gives\n> \n>     URL\t\t\t\tdirectory\n>     git://site/nouser.git\t/nouser.git\n>     git://site/~nouser.git\t~nouser.git\n> \n> by special casing \"~user\" syntax (in other words, a pathname that\n> begins with a tilde _is_ special cased, and tilde is not considered\n> a normal character that can be in a pathname).  Note the lack of\n> leading slash in the second one.\n> \n> Because that is how the shipped clients work, the daemon needs a bit\n> of adjustment, because interpolation and base-path prefix codepaths\n> wants to accept only paths that begin with a slash, and relies on\n> the slash when interpolating it in the template or concatenating it\n> to the base (e.g. roughly \"sprintf(buf, \"%s%s\", base_path, dir)\").\n> \n> I came up with the following as a less invasive patch.  There no\n> longer is the ase where \"user-path not allowed\" error is given,\n> as treating tilde as just a normal pathname character even when it\n> appears at the beginning is the whole point of this change.\n\nThanks for explaining this. It is rather gross, but I think your\nless-invasive patch is the best we could do given the client behavior.\nAnd it's more what I would have expected based on the original problem\ndescription.\n\n> As I said already, I am not sure if this is a good change, though.\n\nI also am on the fence. I'm not sure I understand a compelling reason to\nhave a non-interpolated \"~\" in the repo path name. Sure, it's a\nlimitation, but why does anybody care?\n\n> @@ -170,11 +171,11 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)\n>  \t\treturn NULL;\n>  \t}\n>  \n> +\tif (!user_path && *dir == '~') {\n> +\t\tsnprintf(tilde_path, PATH_MAX, \"/%s\", dir);\n> +\t\tdir = tilde_path;\n> +\t}\n\nI know you are following existing convention in this function to use an\nunchecked snprintf(), but it makes me wonder what kind of mischief a\nclient could get up to by silently truncating via snprintf. This\nfunction is, after all, supposed to be checking the quality of the\nincoming path.\n\nxsnprintf() is probably too blunt a hammer, but:\n\n  if (snprintf(rpath, PATH_MAX, ...) >= PATH_MAX) {\n\tlogerror(\"path too long\");\n\treturn NULL;\n  }\n\nin various places may be appropriate.\n\n-Peff\n"}]}