{"thread":{"id":"49065","subject":"[RFC] submodule: munge paths to submodule git directories","startedAt":"2018-08-07T23:06:44Z","lastAt":"2019-01-17T17:57:22Z","messageCount":40,"participants":["Brandon Williams","Jonathan Nieder","Junio C Hamano","Stefan Beller","Jeff King","Aaron Schrab"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"354778","messageId":"20180807230637.247200-1-bmwill@google.com","threadId":"49065","inReplyTo":null,"subject":"[RFC] submodule: munge paths to submodule git directories","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-08-07T23:06:37Z","receivedAt":"2018-08-07T23:06:44Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Commit 0383bbb901 (submodule-config: verify submodule names as paths,\n2018-04-30) introduced some checks to ensure that submodule names don't\ninclude directory traversal components (e.g. \"../\").\n\nThis addresses the vulnerability identified in 0383bbb901 but the root\ncause is that we use submodule names to construct paths to the\nsubmodule's git directory. What we really should do is munge the\nsubmodule name before using it to construct a path.\n\nIntroduce a function \"strbuf_submodule_gitdir()\" which callers can use\nto build a path to a submodule's gitdir. This allows for a single\nlocation where we can munge the submodule name (by url encoding it)\nbefore using it as part of a path.\n\nSigned-off-by: Brandon Williams \n---\n\nUsing submodule names as is continues to be not such a good idea. Maybe\nwe could apply something like this to stop using them as is. url\nencoding seems like the easiest approach, but I've also heard\nsuggestions that would could use the SHA1 of the submodule name.\n\nAny thoughts?\n\n builtin/submodule--helper.c | 10 ++++--\n dir.c | 2 +-\n repository.c | 3 +-\n submodule.c | 57 +++++++++++++++++++++++---------\n submodule.h | 3 ++\n t/t7400-submodule-basic.sh | 2 +-\n t/t7406-submodule-update.sh | 21 ++++--------\n t/t7410-submodule-checkout-to.sh | 6 ++--\n 8 files changed, 65 insertions(+), 39 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex bd250ca216..37b7353167 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1122,18 +1122,24 @@ static int add_possible_reference_from_superproject(\n \t * standard layout with .git/(modules/)+/objects\n \t */\n \tif (ends_with(alt->path, \"/objects\")) {\n+\t\tstruct repository alternate;\n \t\tchar *sm_alternate;\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstruct strbuf err = STRBUF_INIT;\n \t\tstrbuf_add(&sb, alt->path, strlen(alt->path) - strlen(\"objects\"));\n \n+\t\trepo_init(&alternate, sb.buf, NULL);\n+\n \t\t/*\n \t\t * We need to end the new path with '/' to mark it as a dir,\n \t\t * otherwise a submodule name containing '/' will be broken\n \t\t * as the last part of a missing submodule reference would\n \t\t * be taken as a file name.\n \t\t */\n-\t\tstrbuf_addf(&sb, \"modules/%s/\", sas->submodule_name);\n+\t\tstrbuf_reset(&sb);\n+\t\tstrbuf_submodule_gitdir(&sb, &alternate, sas->submodule_name);\n+\t\tstrbuf_addch(&sb, '/');\n+\t\trepo_clear(&alternate);\n \n \t\tsm_alternate = compute_alternate_path(sb.buf, &err);\n \t\tif (sm_alternate) {\n@@ -1246,7 +1252,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_submodule_helper_usage,\n \t\t\t\t module_clone_options);\n \n-\tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n+\tstrbuf_submodule_gitdir(&sb, the_repository, name);\n \tsm_gitdir = absolute_pathdup(sb.buf);\n \tstrbuf_reset(&sb);\n \ndiff --git a/dir.c b/dir.c\nindex fe9bf58e4c..3463a5e0a5 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -3052,7 +3052,7 @@ static void connect_wt_gitdir_in_nested(const char *sub_worktree,\n \t\tstrbuf_reset(&sub_wt);\n \t\tstrbuf_reset(&sub_gd);\n \t\tstrbuf_addf(&sub_wt, \"%s/%s\", sub_worktree, sub->path);\n-\t\tstrbuf_addf(&sub_gd, \"%s/modules/%s\", sub_gitdir, sub->name);\n+\t\tstrbuf_submodule_gitdir(&sub_gd, &subrepo, sub->name);\n \n \t\tconnect_work_tree_and_git_dir(sub_wt.buf, sub_gd.buf, 1);\n \t}\ndiff --git a/repository.c b/repository.c\nindex 02fe884603..15fabbd08d 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -194,8 +194,7 @@ int repo_submodule_init(struct repository *submodule,\n \t\t * submodule would not have a worktree.\n \t\t */\n \t\tstrbuf_reset(&gitdir);\n-\t\tstrbuf_repo_git_path(&gitdir, superproject,\n-\t\t\t\t \"modules/%s\", sub->name);\n+\t\tstrbuf_submodule_gitdir(&gitdir, superproject, sub->name);\n \n \t\tif (repo_init(submodule, gitdir.buf, NULL)) {\n \t\t\tret = -1;\ndiff --git a/submodule.c b/submodule.c\nindex 939d6870ec..1d571845e8 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1625,20 +1625,22 @@ int submodule_move_head(const char *path,\n \t\t\t\tabsorb_git_dir_into_superproject(\"\", path,\n \t\t\t\t\tABSORB_GITDIR_RECURSE_SUBMODULES);\n \t\t} else {\n-\t\t\tchar *gitdir = xstrfmt(\"%s/modules/%s\",\n-\t\t\t\t get_git_common_dir(), sub->name);\n-\t\t\tconnect_work_tree_and_git_dir(path, gitdir, 0);\n-\t\t\tfree(gitdir);\n+\t\t\tstruct strbuf gitdir = STRBUF_INIT;\n+\t\t\tstrbuf_submodule_gitdir(&gitdir, the_repository,\n+\t\t\t\t\t\tsub->name);\n+\t\t\tconnect_work_tree_and_git_dir(path, gitdir.buf, 0);\n+\t\t\tstrbuf_release(&gitdir);\n \n \t\t\t/* make sure the index is clean as well */\n \t\t\tsubmodule_reset_index(path);\n \t\t}\n \n \t\tif (old_head && (flags & SUBMODULE_MOVE_HEAD_FORCE)) {\n-\t\t\tchar *gitdir = xstrfmt(\"%s/modules/%s\",\n-\t\t\t\t get_git_common_dir(), sub->name);\n-\t\t\tconnect_work_tree_and_git_dir(path, gitdir, 1);\n-\t\t\tfree(gitdir);\n+\t\t\tstruct strbuf gitdir = STRBUF_INIT;\n+\t\t\tstrbuf_submodule_gitdir(&gitdir, the_repository,\n+\t\t\t\t\t\tsub->name);\n+\t\t\tconnect_work_tree_and_git_dir(path, gitdir.buf, 1);\n+\t\t\tstrbuf_release(&gitdir);\n \t\t}\n \t}\n \n@@ -1711,7 +1713,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,\n \t\t\t\t\t\t const char *path)\n {\n \tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n-\tconst char *new_git_dir;\n+\tstruct strbuf new_gitdir = STRBUF_INIT;\n \tconst struct submodule *sub;\n \n \tif (submodule_uses_worktrees(path))\n@@ -1729,10 +1731,10 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,\n \tif (!sub)\n \t\tdie(_(\"could not lookup name for submodule '%s'\"), path);\n \n-\tnew_git_dir = git_path(\"modules/%s\", sub->name);\n-\tif (safe_create_leading_directories_const(new_git_dir) < 0)\n-\t\tdie(_(\"could not create directory '%s'\"), new_git_dir);\n-\treal_new_git_dir = real_pathdup(new_git_dir, 1);\n+\tstrbuf_submodule_gitdir(&new_gitdir, the_repository, sub->name);\n+\tif (safe_create_leading_directories_const(new_gitdir.buf) < 0)\n+\t\tdie(_(\"could not create directory '%s'\"), new_gitdir.buf);\n+\treal_new_git_dir = real_pathdup(new_gitdir.buf, 1);\n \n \tfprintf(stderr, _(\"Migrating git directory of '%s%s' from\\n'%s' to\\n'%s'\\n\"),\n \t\tget_super_prefix_or_empty(), path,\n@@ -1743,6 +1745,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,\n \tfree(old_git_dir);\n \tfree(real_old_git_dir);\n \tfree(real_new_git_dir);\n+\tstrbuf_release(&new_gitdir);\n }\n \n /*\n@@ -1763,6 +1766,7 @@ void absorb_git_dir_into_superproject(const char *prefix,\n \t/* Not populated? */\n \tif (!sub_git_dir) {\n \t\tconst struct submodule *sub;\n+\t\tstruct strbuf sub_gitdir = STRBUF_INIT;\n \n \t\tif (err_code == READ_GITFILE_ERR_STAT_FAILED) {\n \t\t\t/* unpopulated as expected */\n@@ -1784,8 +1788,9 @@ void absorb_git_dir_into_superproject(const char *prefix,\n \t\tsub = submodule_from_path(the_repository, &null_oid, path);\n \t\tif (!sub)\n \t\t\tdie(_(\"could not lookup name for submodule '%s'\"), path);\n-\t\tconnect_work_tree_and_git_dir(path,\n-\t\t\tgit_path(\"modules/%s\", sub->name), 0);\n+\t\tstrbuf_submodule_gitdir(&sub_gitdir, the_repository, sub->name);\n+\t\tconnect_work_tree_and_git_dir(path, sub_gitdir.buf, 0);\n+\t\tstrbuf_release(&sub_gitdir);\n \t} else {\n \t\t/* Is it already absorbed into the superprojects git dir? */\n \t\tchar *real_sub_git_dir = real_pathdup(sub_git_dir, 1);\n@@ -1933,9 +1938,29 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)\n \t\t\tgoto cleanup;\n \t\t}\n \t\tstrbuf_reset(buf);\n-\t\tstrbuf_git_path(buf, \"%s/%s\", \"modules\", sub->name);\n+\t\tstrbuf_submodule_gitdir(buf, the_repository, sub->name);\n \t}\n \n cleanup:\n \treturn ret;\n }\n+\n+void strbuf_submodule_gitdir(struct strbuf *buf, struct repository *r,\n+\t\t\t const char *submodule_name)\n+{\n+\tint modules_len;\n+\n+\tstrbuf_git_common_path(buf, r, \"modules/\");\n+\tmodules_len = buf->len;\n+\tstrbuf_addstr(buf, submodule_name);\n+\n+\t/*\n+\t * If the submodule gitdir already exists using the old location then\n+\t * return that.\n+\t */\n+\tif (!access(buf->buf, F_OK))\n+\t\treturn;\n+\n+\tstrbuf_setlen(buf, modules_len);\n+\tstrbuf_addstr_urlencode(buf, submodule_name, 1);\n+}\ndiff --git a/submodule.h b/submodule.h\nindex 7856b8a0b3..b56f89740d 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -114,6 +114,9 @@ extern int push_unpushed_submodules(struct oid_array *commits,\n */\n int submodule_to_gitdir(struct strbuf *buf, const char *submodule);\n \n+void strbuf_submodule_gitdir(struct strbuf *buf, struct repository *r,\n+\t\t\t const char *submodule_name);\n+\n #define SUBMODULE_MOVE_HEAD_DRY_RUN (1<<0)\n #define SUBMODULE_MOVE_HEAD_FORCE (1<<1)\n extern int submodule_move_head(const char *path,\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 812db137b8..fce164484d 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -932,7 +932,7 @@ test_expect_success 'recursive relative submodules stay relative' '\n \t\tcd clone2 &&\n \t\tgit submodule update --init --recursive &&\n \t\techo \"gitdir: ../.git/modules/sub3\" >./sub3/.git_expect &&\n-\t\techo \"gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub\" >./sub3/dirdir/subsub/.git_expect\n+\t\techo \"gitdir: ../../../.git/modules/sub3/modules/dirdir%2fsubsub\" >./sub3/dirdir/subsub/.git_expect\n \t) &&\n \ttest_cmp clone2/sub3/.git_expect clone2/sub3/.git &&\n \ttest_cmp clone2/sub3/dirdir/subsub/.git_expect clone2/sub3/dirdir/subsub/.git\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 9e0d31700e..c4e94c168d 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -777,12 +777,8 @@ test_expect_success 'submodule add places git-dir in superprojects git-dir' '\n \t(cd super &&\n \t mkdir deeper &&\n \t git submodule add ../submodule deeper/submodule &&\n-\t (cd deeper/submodule &&\n-\t git log > ../../expected\n-\t ) &&\n-\t (cd .git/modules/deeper/submodule &&\n-\t git log > ../../../../actual\n-\t ) &&\n+\t git -C deeper/submodule log >expected &&\n+\t git -C .git/modules/deeper%2fsubmodule log >actual &&\n \t test_cmp actual expected\n \t)\n '\n@@ -795,12 +791,9 @@ test_expect_success 'submodule update places git-dir in superprojects git-dir' '\n \t(cd super2 &&\n \t git submodule init deeper/submodule &&\n \t git submodule update &&\n-\t (cd deeper/submodule &&\n-\t git log > ../../expected\n-\t ) &&\n-\t (cd .git/modules/deeper/submodule &&\n-\t git log > ../../../../actual\n-\t ) &&\n+\n+\t git -C deeper/submodule log >expected &&\n+\t git -C .git/modules/deeper%2fsubmodule log >actual &&\n \t test_cmp actual expected\n \t)\n '\n@@ -815,9 +808,7 @@ test_expect_success 'submodule add places git-dir in superprojects git-dir recur\n \t git commit -m \"added subsubmodule\" &&\n \t git push origin :\n \t ) &&\n-\t (cd .git/modules/deeper/submodule/modules/subsubmodule &&\n-\t git log > ../../../../../actual\n-\t ) &&\n+\t git -C .git/modules/deeper%2fsubmodule/modules/subsubmodule log >actual &&\n \t git add deeper/submodule &&\n \t git commit -m \"update submodule\" &&\n \t git push origin : &&\ndiff --git a/t/t7410-submodule-checkout-to.sh b/t/t7410-submodule-checkout-to.sh\nindex 1acef32647..c408e9010a 100755\n--- a/t/t7410-submodule-checkout-to.sh\n+++ b/t/t7410-submodule-checkout-to.sh\n@@ -35,8 +35,10 @@ test_expect_success 'checkout main' \\\n (cd clone/main &&\n \tgit worktree add \"$base_path/default_checkout/main\" \"$rev1_hash_main\")'\n \n-test_expect_failure 'can see submodule diffs just after checkout' \\\n- '(cd default_checkout/main && git diff --submodule master\"^!\" | grep \"file1 updated\")'\n+test_expect_success 'can see submodule diffs just after checkout' '\n+\tgit -C default_checkout/main diff --submodule master\"^!\" >out &&\n+\tgrep \"file1 updated\" out\n+'\n \n test_expect_success 'checkout main and initialize independed clones' \\\n 'mkdir fully_cloned_submodule &&\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354780","messageId":"20180807232524.GB249457@aiede.svl.corp.google.com","threadId":"49065","inReplyTo":"20180807230637.247200-1-bmwill@google.com","subject":"Re: [RFC] submodule: munge paths to submodule git directories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-07T23:25:24Z","receivedAt":"2018-08-07T23:25:29Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nBrandon Williams wrote:\n\n> Commit 0383bbb901 (submodule-config: verify submodule names as paths,\n> 2018-04-30) introduced some checks to ensure that submodule names don't\n> include directory traversal components (e.g. \"../\").\n>\n> This addresses the vulnerability identified in 0383bbb901 but the root\n> cause is that we use submodule names to construct paths to the\n> submodule's git directory. What we really should do is munge the\n> submodule name before using it to construct a path.\n>\n> Introduce a function \"strbuf_submodule_gitdir()\" which callers can use\n> to build a path to a submodule's gitdir. This allows for a single\n> location where we can munge the submodule name (by url encoding it)\n> before using it as part of a path.\n>\n> Signed-off-by: Brandon Williams \n> ---\n> Using submodule names as is continues to be not such a good idea. Maybe\n> we could apply something like this to stop using them as is. url\n> encoding seems like the easiest approach, but I've also heard\n> suggestions that would could use the SHA1 of the submodule name.\n>\n> Any thoughts?\n\nI like this idea. It avoids the security and complexity problems of\nfunny nested directories, while still making the submodule git dirs\neasy to find.\n\nThe current behavior has been particularly a problem in practice when\nsubmodule names are nested:\n\n\t[submodule \"a\"]\n\t\turl = https://www.example.com/a\n\t\tpath = a/1\n\n\t[submodule \"a/b\"]\n\t\turl = https://www.example.com/a/b\n\t\tpath = a/2\n\nWe don't enforce any constraint on submodule names to prevent that,\nbut it causes hard to diagnose errors at clone time:\n\n\tfatal: not a git repository: superproject/a/1/../../.git/modules/a\n\tUnable to fetch in submodule path 'a/1'\n\tfatal: not a git repository: superproject/a/1/../../.git/modules/a\n\tfatal: not a git repository: superproject/a/1/../../.git/modules/a\n\tfatal: not a git repository: superproject/a/1/../../.git/modules/a\n\tFetched in submodule 'a/1', but it did not contain 55ca6286e3e4f4fba5d0448333fa99fc5a404a73. Direct fetching of that commit failed.\n\nbecause the fetch in .git/modules/a is interfered with by\n.git/modules/a/b.\n\n[...]\n> --- a/submodule.c\n> +++ b/submodule.c\n[...]\n> @@ -1933,9 +1938,29 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)\n> \t\t\tgoto cleanup;\n> \t\t}\n> \t\tstrbuf_reset(buf);\n> -\t\tstrbuf_git_path(buf, \"%s/%s\", \"modules\", sub->name);\n> +\t\tstrbuf_submodule_gitdir(buf, the_repository, sub->name);\n> \t}\n> \n> cleanup:\n> \treturn ret;\n> }\n> +\n> +void strbuf_submodule_gitdir(struct strbuf *buf, struct repository *r,\n> +\t\t\t const char *submodule_name)\n> +{\n> +\tint modules_len;\n\nnit: size_t\n\n> +\n> +\tstrbuf_git_common_path(buf, r, \"modules/\");\n> +\tmodules_len = buf->len;\n> +\tstrbuf_addstr(buf, submodule_name);\n> +\n> +\t/*\n> +\t * If the submodule gitdir already exists using the old location then\n> +\t * return that.\n> +\t */\n\nnit: \"old-fashioned location\" or something. Maybe the function could\nuse an API comment describing what's going on (that there are two\nnaming conventions and we try first the old, then the new).\n\nShould we validate the submodule_name here when accessing following the old\nconvention?\n\n> +\tif (!access(buf->buf, F_OK))\n> +\t\treturn;\n> +\n> +\tstrbuf_setlen(buf, modules_len);\n> +\tstrbuf_addstr_urlencode(buf, submodule_name, 1);\n> +}\n[...]\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -932,7 +932,7 @@ test_expect_success 'recursive relative submodules stay relative' '\n> \t\tcd clone2 &&\n> \t\tgit submodule update --init --recursive &&\n> \t\techo \"gitdir: ../.git/modules/sub3\" >./sub3/.git_expect &&\n> -\t\techo \"gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub\" >./sub3/dirdir/subsub/.git_expect\n> +\t\techo \"gitdir: ../../../.git/modules/sub3/modules/dirdir%2fsubsub\" >./sub3/dirdir/subsub/.git_expect\n> \t) &&\n> \ttest_cmp clone2/sub3/.git_expect clone2/sub3/.git &&\n> \ttest_cmp clone2/sub3/dirdir/subsub/.git_expect clone2/sub3/dirdir/subsub/.git\n\nSensible.\n\nCan there be a test of the compatibility code as well? (I mean a test\nthat manually sets up a submodule in .git/modules/dirdir/subsub and\nensures that it gets reused.)\n\nI'll apply this, experiment with it, and report back. Thanks for\nwriting it.\n\nSincerely,\nJonathan\n"},{"id":"354783","messageId":"xmqq36vpk94c.fsf@gitster-ct.c.googlers.com","threadId":"49065","inReplyTo":"20180807230637.247200-1-bmwill@google.com","subject":"Re: [RFC] submodule: munge paths to submodule git directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-08T00:14:11Z","receivedAt":"2018-08-08T00:14:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> Introduce a function \"strbuf_submodule_gitdir()\" which callers can use\n> to build a path to a submodule's gitdir. This allows for a single\n> location where we can munge the submodule name (by url encoding it)\n> before using it as part of a path.\n\nI am not sure about the name with \"strbuf_\" prefix; it is as bad as\nusing hungarian notation for variable names.\n\nThere probably are some existing offenders, but it is merely an\nimplementation detail (or a function signature) that the returned\nvalue is communicated using a strbuf (contrast it with things like\nstrbuf_add() that is _about_ doing something to a strbuf), and in\nthe longer term I prefer to see them lose \"strbuf_\" from their names\nand optionally use the same number of bytes to describe what they do\nmore clearly. For this particular case, \"submodule\" and \"gitdir\"\nare sufficient to signal what the function is about, I think, so the\n\"optionally use...\" is not necessary---instead we get a name that is\nshorte to type and to remember.\n\n> Using submodule names as is continues to be not such a good idea. Maybe\n> we could apply something like this to stop using them as is. url\n> encoding seems like the easiest approach, but I've also heard\n> suggestions that would could use the SHA1 of the submodule name.\n\nBeing human readable is a good trait to keep when possible. \n\nWhen you have two submodules with vastly different names\n(e.g. \"hello\" and \"bye\"), and for some reason you need to go in to\n.gitmodules and manually fix their entries up, \"hash of name\" does\nnot help you avoid mistakes (hashing \"hello\" and hashing \"helo\"\nwould give a name as different as hashing \"bye\", so when you see\n[module \"hel$something\"] in .gitmodules, you would know that entry\nis not about the \"bye\" module, but \"hello\" module, even if you do\nnot remember exactly if the module you want to manipulate was called\n\"hello\" or \"helo\"). The same discussion applies against UUID.\n"},{"id":"354945","messageId":"20180808223323.79989-1-bmwill@google.com","threadId":"49065","inReplyTo":"20180807230637.247200-1-bmwill@google.com","subject":"[PATCH 0/2] munge submodule names","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-08-08T22:33:21Z","receivedAt":"2018-08-08T22:33:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Here's a more polished series taking into account some of the feedback\non the RFC. As Junio pointed out URL encoding makes the directories\nmuch more human readable, but I'm open to other ideas if we don't think\nURL encoding is the right thing to do.\n\nBrandon Williams (2):\n submodule: create helper to build paths to submodule gitdirs\n submodule: munge paths to submodule git directories\n\n builtin/submodule--helper.c | 28 +++++++++++--\n dir.c | 2 +-\n git-submodule.sh | 7 ++--\n repository.c | 3 +-\n submodule.c | 67 ++++++++++++++++++++++----------\n submodule.h | 7 ++++\n t/t7400-submodule-basic.sh | 32 ++++++++++++++-\n t/t7406-submodule-update.sh | 21 +++-------\n t/t7410-submodule-checkout-to.sh | 6 ++-\n 9 files changed, 126 insertions(+), 47 deletions(-)\n\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354946","messageId":"20180808223323.79989-2-bmwill@google.com","threadId":"49065","inReplyTo":"20180808223323.79989-1-bmwill@google.com","subject":"[PATCH 1/2] submodule: create helper to build paths to submodule gitdirs","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-08-08T22:33:22Z","receivedAt":"2018-08-08T22:33:34Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Introduce a helper function \"submodule_name_to_gitdir()\" (and the\nsubmodule--helper subcommand \"gitdir\") which constructs a path to a\nsubmodule's gitdir, located in the provided repository's \"modules\"\ndirectory.\n\nThis consolidates the logic needed to build up a path into a\nrepository's \"modules\" directory, abstracting away the fact that\nsubmodule git directories are stored in a repository's common gitdir.\nThis makes it easier to adjust how submodules gitdir are stored in the\n\"modules\" directory in a future patch.\n\nSigned-off-by: Brandon Williams \n---\n builtin/submodule--helper.c | 28 +++++++++++++++--\n dir.c | 2 +-\n git-submodule.sh | 7 +++--\n repository.c | 3 +-\n submodule.c | 53 ++++++++++++++++++++------------\n submodule.h | 7 +++++\n t/t7410-submodule-checkout-to.sh | 6 ++--\n 7 files changed, 75 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex a3c4564c6c..5bfd2d0be9 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -906,6 +906,21 @@ static int module_name(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int module_gitdir(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct strbuf gitdir = STRBUF_INIT;\n+\n+\tif (argc != 2)\n+\t\tusage(_(\"git submodule--helper gitdir \"));\n+\n+\tsubmodule_name_to_gitdir(&gitdir, the_repository, argv[1]);\n+\n+\tprintf(\"%s\\n\", gitdir.buf);\n+\n+\tstrbuf_release(&gitdir);\n+\treturn 0;\n+}\n+\n struct sync_cb {\n \tconst char *prefix;\n \tunsigned int flags;\n@@ -1268,18 +1283,24 @@ static int add_possible_reference_from_superproject(\n \t * standard layout with .git/(modules/)+/objects\n \t */\n \tif (ends_with(alt->path, \"/objects\")) {\n+\t\tstruct repository alternate;\n \t\tchar *sm_alternate;\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstruct strbuf err = STRBUF_INIT;\n \t\tstrbuf_add(&sb, alt->path, strlen(alt->path) - strlen(\"objects\"));\n \n+\t\trepo_init(&alternate, sb.buf, NULL);\n+\n \t\t/*\n \t\t * We need to end the new path with '/' to mark it as a dir,\n \t\t * otherwise a submodule name containing '/' will be broken\n \t\t * as the last part of a missing submodule reference would\n \t\t * be taken as a file name.\n \t\t */\n-\t\tstrbuf_addf(&sb, \"modules/%s/\", sas->submodule_name);\n+\t\tstrbuf_reset(&sb);\n+\t\tsubmodule_name_to_gitdir(&sb, &alternate, sas->submodule_name);\n+\t\tstrbuf_addch(&sb, '/');\n+\t\trepo_clear(&alternate);\n \n \t\tsm_alternate = compute_alternate_path(sb.buf, &err);\n \t\tif (sm_alternate) {\n@@ -1392,7 +1413,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_submodule_helper_usage,\n \t\t\t\t module_clone_options);\n \n-\tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n+\tsubmodule_name_to_gitdir(&sb, the_repository, name);\n \tsm_gitdir = absolute_pathdup(sb.buf);\n \tstrbuf_reset(&sb);\n \n@@ -2018,7 +2039,7 @@ static int connect_gitdir_workingtree(int argc, const char **argv, const char *p\n \tname = argv[1];\n \tpath = argv[2];\n \n-\tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n+\tsubmodule_name_to_gitdir(&sb, the_repository, name);\n \tsm_gitdir = absolute_pathdup(sb.buf);\n \n \tconnect_work_tree_and_git_dir(path, sm_gitdir, 0);\n@@ -2040,6 +2061,7 @@ struct cmd_struct {\n static struct cmd_struct commands[] = {\n \t{\"list\", module_list, 0},\n \t{\"name\", module_name, 0},\n+\t{\"gitdir\", module_gitdir, 0},\n \t{\"clone\", module_clone, 0},\n \t{\"update-clone\", update_clone, 0},\n \t{\"connect-gitdir-workingtree\", connect_gitdir_workingtree, 0},\ndiff --git a/dir.c b/dir.c\nindex 21e6f2520a..7a9827ea4b 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -3053,7 +3053,7 @@ static void connect_wt_gitdir_in_nested(const char *sub_worktree,\n \t\tstrbuf_reset(&sub_wt);\n \t\tstrbuf_reset(&sub_gd);\n \t\tstrbuf_addf(&sub_wt, \"%s/%s\", sub_worktree, sub->path);\n-\t\tstrbuf_addf(&sub_gd, \"%s/modules/%s\", sub_gitdir, sub->name);\n+\t\tsubmodule_name_to_gitdir(&sub_gd, &subrepo, sub->name);\n \n \t\tconnect_work_tree_and_git_dir(sub_wt.buf, sub_gd.buf, 1);\n \t}\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 8b5ad59bde..053747d290 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -252,12 +252,13 @@ Use -f if you really want to add it.\" >&2\n \t\tfi\n \n \telse\n-\t\tif test -d \".git/modules/$sm_name\"\n+\t\tsm_gitdir=\"$(git submodule--helper gitdir \"$sm_name\")\"\n+\t\tif test -d \"$sm_gitdir\"\n \t\tthen\n \t\t\tif test -z \"$force\"\n \t\t\tthen\n \t\t\t\teval_gettextln >&2 \"A git directory for '\\$sm_name' is found locally with remote(s):\"\n-\t\t\t\tGIT_DIR=\".git/modules/$sm_name\" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^,\" \", -e s,' (fetch)',, >&2\n+\t\t\t\tGIT_DIR=\"$sm_gitdir\" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^,\" \", -e s,' (fetch)',, >&2\n \t\t\t\tdie \"$(eval_gettextln \"\\\n If you want to reuse this local git directory instead of cloning again from\n \\$realrepo\n@@ -577,7 +578,7 @@ cmd_update()\n \t\t\tdie \"$(eval_gettext \"Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n \t\tfi\n \n-\t\tif ! $(git config -f \"$(git rev-parse --git-common-dir)/modules/$name/config\" core.worktree) 2>/dev/null\n+\t\tif ! $(git config -f \"$(git submodule--helper gitdir \"$name\")/config\" core.worktree) 2>/dev/null\n \t\tthen\n \t\t\tgit submodule--helper connect-gitdir-workingtree \"$name\" \"$sm_path\"\n \t\tfi\ndiff --git a/repository.c b/repository.c\nindex 5dd1486718..9da3c9d4b7 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -198,8 +198,7 @@ int repo_submodule_init(struct repository *submodule,\n \t\t * submodule would not have a worktree.\n \t\t */\n \t\tstrbuf_reset(&gitdir);\n-\t\tstrbuf_repo_git_path(&gitdir, superproject,\n-\t\t\t\t \"modules/%s\", sub->name);\n+\t\tsubmodule_name_to_gitdir(&gitdir, superproject, sub->name);\n \n \t\tif (repo_init(submodule, gitdir.buf, NULL)) {\n \t\t\tret = -1;\ndiff --git a/submodule.c b/submodule.c\nindex 6e14547e9e..24eced34e7 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1536,14 +1536,15 @@ int bad_to_remove_submodule(const char *path, unsigned flags)\n \n void submodule_unset_core_worktree(const struct submodule *sub)\n {\n-\tchar *config_path = xstrfmt(\"%s/modules/%s/config\",\n-\t\t\t\t get_git_common_dir(), sub->name);\n+\tstruct strbuf config_path = STRBUF_INIT;\n+\tsubmodule_name_to_gitdir(&config_path, the_repository, sub->name);\n+\tstrbuf_addstr(&config_path, \"/config\");\n \n-\tif (git_config_set_in_file_gently(config_path, \"core.worktree\", NULL))\n+\tif (git_config_set_in_file_gently(config_path.buf, \"core.worktree\", NULL))\n \t\twarning(_(\"Could not unset core.worktree setting in submodule '%s'\"),\n \t\t\t sub->path);\n \n-\tfree(config_path);\n+\tstrbuf_release(&config_path);\n }\n \n static const char *get_super_prefix_or_empty(void)\n@@ -1639,20 +1640,22 @@ int submodule_move_head(const char *path,\n \t\t\t\tabsorb_git_dir_into_superproject(\"\", path,\n \t\t\t\t\tABSORB_GITDIR_RECURSE_SUBMODULES);\n \t\t} else {\n-\t\t\tchar *gitdir = xstrfmt(\"%s/modules/%s\",\n-\t\t\t\t get_git_common_dir(), sub->name);\n-\t\t\tconnect_work_tree_and_git_dir(path, gitdir, 0);\n-\t\t\tfree(gitdir);\n+\t\t\tstruct strbuf gitdir = STRBUF_INIT;\n+\t\t\tsubmodule_name_to_gitdir(&gitdir, the_repository,\n+\t\t\t\t\t\t sub->name);\n+\t\t\tconnect_work_tree_and_git_dir(path, gitdir.buf, 0);\n+\t\t\tstrbuf_release(&gitdir);\n \n \t\t\t/* make sure the index is clean as well */\n \t\t\tsubmodule_reset_index(path);\n \t\t}\n \n \t\tif (old_head && (flags & SUBMODULE_MOVE_HEAD_FORCE)) {\n-\t\t\tchar *gitdir = xstrfmt(\"%s/modules/%s\",\n-\t\t\t\t get_git_common_dir(), sub->name);\n-\t\t\tconnect_work_tree_and_git_dir(path, gitdir, 1);\n-\t\t\tfree(gitdir);\n+\t\t\tstruct strbuf gitdir = STRBUF_INIT;\n+\t\t\tsubmodule_name_to_gitdir(&gitdir, the_repository,\n+\t\t\t\t\t\t sub->name);\n+\t\t\tconnect_work_tree_and_git_dir(path, gitdir.buf, 1);\n+\t\t\tstrbuf_release(&gitdir);\n \t\t}\n \t}\n \n@@ -1727,7 +1730,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,\n \t\t\t\t\t\t const char *path)\n {\n \tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n-\tconst char *new_git_dir;\n+\tstruct strbuf new_gitdir = STRBUF_INIT;\n \tconst struct submodule *sub;\n \n \tif (submodule_uses_worktrees(path))\n@@ -1745,10 +1748,10 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,\n \tif (!sub)\n \t\tdie(_(\"could not lookup name for submodule '%s'\"), path);\n \n-\tnew_git_dir = git_path(\"modules/%s\", sub->name);\n-\tif (safe_create_leading_directories_const(new_git_dir) < 0)\n-\t\tdie(_(\"could not create directory '%s'\"), new_git_dir);\n-\treal_new_git_dir = real_pathdup(new_git_dir, 1);\n+\tsubmodule_name_to_gitdir(&new_gitdir, the_repository, sub->name);\n+\tif (safe_create_leading_directories_const(new_gitdir.buf) < 0)\n+\t\tdie(_(\"could not create directory '%s'\"), new_gitdir.buf);\n+\treal_new_git_dir = real_pathdup(new_gitdir.buf, 1);\n \n \tfprintf(stderr, _(\"Migrating git directory of '%s%s' from\\n'%s' to\\n'%s'\\n\"),\n \t\tget_super_prefix_or_empty(), path,\n@@ -1759,6 +1762,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,\n \tfree(old_git_dir);\n \tfree(real_old_git_dir);\n \tfree(real_new_git_dir);\n+\tstrbuf_release(&new_gitdir);\n }\n \n /*\n@@ -1779,6 +1783,7 @@ void absorb_git_dir_into_superproject(const char *prefix,\n \t/* Not populated? */\n \tif (!sub_git_dir) {\n \t\tconst struct submodule *sub;\n+\t\tstruct strbuf sub_gitdir = STRBUF_INIT;\n \n \t\tif (err_code == READ_GITFILE_ERR_STAT_FAILED) {\n \t\t\t/* unpopulated as expected */\n@@ -1800,8 +1805,9 @@ void absorb_git_dir_into_superproject(const char *prefix,\n \t\tsub = submodule_from_path(the_repository, &null_oid, path);\n \t\tif (!sub)\n \t\t\tdie(_(\"could not lookup name for submodule '%s'\"), path);\n-\t\tconnect_work_tree_and_git_dir(path,\n-\t\t\tgit_path(\"modules/%s\", sub->name), 0);\n+\t\tsubmodule_name_to_gitdir(&sub_gitdir, the_repository, sub->name);\n+\t\tconnect_work_tree_and_git_dir(path, sub_gitdir.buf, 0);\n+\t\tstrbuf_release(&sub_gitdir);\n \t} else {\n \t\t/* Is it already absorbed into the superprojects git dir? */\n \t\tchar *real_sub_git_dir = real_pathdup(sub_git_dir, 1);\n@@ -1949,9 +1955,16 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)\n \t\t\tgoto cleanup;\n \t\t}\n \t\tstrbuf_reset(buf);\n-\t\tstrbuf_git_path(buf, \"%s/%s\", \"modules\", sub->name);\n+\t\tsubmodule_name_to_gitdir(buf, the_repository, sub->name);\n \t}\n \n cleanup:\n \treturn ret;\n }\n+\n+void submodule_name_to_gitdir(struct strbuf *buf, struct repository *r,\n+\t\t\t const char *submodule_name)\n+{\n+\tstrbuf_git_common_path(buf, r, \"modules/\");\n+\tstrbuf_addstr(buf, submodule_name);\n+}\ndiff --git a/submodule.h b/submodule.h\nindex 4644683e6c..0de9410dda 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -114,6 +114,13 @@ extern int push_unpushed_submodules(struct oid_array *commits,\n */\n int submodule_to_gitdir(struct strbuf *buf, const char *submodule);\n \n+/*\n+ * Given a submodule name, create a path to where the submodule's gitdir lives\n+ * inside of the provided repository's 'modules' directory.\n+ */\n+void submodule_name_to_gitdir(struct strbuf *buf, struct repository *r,\n+\t\t\t const char *submodule_name);\n+\n #define SUBMODULE_MOVE_HEAD_DRY_RUN (1<<0)\n #define SUBMODULE_MOVE_HEAD_FORCE (1<<1)\n extern int submodule_move_head(const char *path,\ndiff --git a/t/t7410-submodule-checkout-to.sh b/t/t7410-submodule-checkout-to.sh\nindex 1acef32647..c408e9010a 100755\n--- a/t/t7410-submodule-checkout-to.sh\n+++ b/t/t7410-submodule-checkout-to.sh\n@@ -35,8 +35,10 @@ test_expect_success 'checkout main' \\\n (cd clone/main &&\n \tgit worktree add \"$base_path/default_checkout/main\" \"$rev1_hash_main\")'\n \n-test_expect_failure 'can see submodule diffs just after checkout' \\\n- '(cd default_checkout/main && git diff --submodule master\"^!\" | grep \"file1 updated\")'\n+test_expect_success 'can see submodule diffs just after checkout' '\n+\tgit -C default_checkout/main diff --submodule master\"^!\" >out &&\n+\tgrep \"file1 updated\" out\n+'\n \n test_expect_success 'checkout main and initialize independed clones' \\\n 'mkdir fully_cloned_submodule &&\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354947","messageId":"20180808223323.79989-3-bmwill@google.com","threadId":"49065","inReplyTo":"20180808223323.79989-1-bmwill@google.com","subject":"[PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-08-08T22:33:23Z","receivedAt":"2018-08-08T22:33:35Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Commit 0383bbb901 (submodule-config: verify submodule names as paths,\n2018-04-30) introduced some checks to ensure that submodule names don't\ninclude directory traversal components (e.g. \"../\").\n\nThis addresses the vulnerability identified in 0383bbb901 but the root\ncause is that we use submodule names to construct paths to the\nsubmodule's git directory. What we really should do is munge the\nsubmodule name before using it to construct a path.\n\nTeach \"submodule_name_to_gitdir()\" to munge a submodule's name (by url\nencoding it) before using it to build a path to the submodule's gitdir.\n\nSigned-off-by: Brandon Williams \n---\n submodule.c | 14 ++++++++++++++\n t/t7400-submodule-basic.sh | 32 +++++++++++++++++++++++++++++++-\n t/t7406-submodule-update.sh | 21 ++++++---------------\n 3 files changed, 51 insertions(+), 16 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 24eced34e7..4854d88ce8 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1965,6 +1965,20 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)\n void submodule_name_to_gitdir(struct strbuf *buf, struct repository *r,\n \t\t\t const char *submodule_name)\n {\n+\tsize_t modules_len;\n+\n \tstrbuf_git_common_path(buf, r, \"modules/\");\n+\tmodules_len = buf->len;\n \tstrbuf_addstr(buf, submodule_name);\n+\n+\t/*\n+\t * If the submodule gitdir already exists using the old-fashioned\n+\t * location (which uses the submodule name as-is, without munging it)\n+\t * then return that.\n+\t */\n+\tif (!access(buf->buf, F_OK))\n+\t\treturn;\n+\n+\tstrbuf_setlen(buf, modules_len);\n+\tstrbuf_addstr_urlencode(buf, submodule_name, 1);\n }\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 2c2c97e144..963693332c 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -933,7 +933,7 @@ test_expect_success 'recursive relative submodules stay relative' '\n \t\tcd clone2 &&\n \t\tgit submodule update --init --recursive &&\n \t\techo \"gitdir: ../.git/modules/sub3\" >./sub3/.git_expect &&\n-\t\techo \"gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub\" >./sub3/dirdir/subsub/.git_expect\n+\t\techo \"gitdir: ../../../.git/modules/sub3/modules/dirdir%2fsubsub\" >./sub3/dirdir/subsub/.git_expect\n \t) &&\n \ttest_cmp clone2/sub3/.git_expect clone2/sub3/.git &&\n \ttest_cmp clone2/sub3/dirdir/subsub/.git_expect clone2/sub3/dirdir/subsub/.git\n@@ -1324,4 +1324,34 @@ test_expect_success 'recursive clone respects -q' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'resolve submodule gitdir in superprojects modules directory' '\n+\ttest_when_finished \"rm -rf superproject submodule\" &&\n+\n+\t# Create a superproject with a submodule which contains a \"/\"\n+\ttest_create_repo submodule &&\n+\ttest_commit -C submodule one &&\n+\ttest_create_repo superproject &&\n+\tgit -C superproject submodule add ../submodule sub/module &&\n+\tgit -C superproject commit -m \"add submodule\" &&\n+\n+\t# \"/\" characters in submodule names are properly urlencoded before\n+\t# being used to construct a path to the submodules gitdir.\n+\tcat >expect <<-EOF &&\n+\t$(git -C superproject rev-parse --git-common-dir)/modules/sub%2fmodule\n+\tEOF\n+\tgit -C superproject submodule--helper gitdir \"sub/module\" >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_path_is_dir \"superproject/.git/modules/sub%2fmodule\" &&\n+\n+\t# Test the old-fashioned way of storing submodules in the\n+\t# \"modules\" directory by directly renaming the submodules gitdir\n+\tmkdir superproject/.git/modules/sub/ &&\n+\tmv superproject/.git/modules/sub%2fmodule superproject/.git/modules/sub/module &&\n+\tcat >expect <<-EOF &&\n+\t$(git -C superproject rev-parse --git-common-dir)/modules/sub/module\n+\tEOF\n+\tgit -C superproject submodule--helper gitdir \"sub/module\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex f604ef7a72..fb744c5c39 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -777,12 +777,8 @@ test_expect_success 'submodule add places git-dir in superprojects git-dir' '\n \t(cd super &&\n \t mkdir deeper &&\n \t git submodule add ../submodule deeper/submodule &&\n-\t (cd deeper/submodule &&\n-\t git log > ../../expected\n-\t ) &&\n-\t (cd .git/modules/deeper/submodule &&\n-\t git log > ../../../../actual\n-\t ) &&\n+\t git -C deeper/submodule log >expected &&\n+\t git -C .git/modules/deeper%2fsubmodule log >actual &&\n \t test_cmp actual expected\n \t)\n '\n@@ -795,12 +791,9 @@ test_expect_success 'submodule update places git-dir in superprojects git-dir' '\n \t(cd super2 &&\n \t git submodule init deeper/submodule &&\n \t git submodule update &&\n-\t (cd deeper/submodule &&\n-\t git log > ../../expected\n-\t ) &&\n-\t (cd .git/modules/deeper/submodule &&\n-\t git log > ../../../../actual\n-\t ) &&\n+\n+\t git -C deeper/submodule log >expected &&\n+\t git -C .git/modules/deeper%2fsubmodule log >actual &&\n \t test_cmp actual expected\n \t)\n '\n@@ -815,9 +808,7 @@ test_expect_success 'submodule add places git-dir in superprojects git-dir recur\n \t git commit -m \"added subsubmodule\" &&\n \t git push origin :\n \t ) &&\n-\t (cd .git/modules/deeper/submodule/modules/subsubmodule &&\n-\t git log > ../../../../../actual\n-\t ) &&\n+\t git -C .git/modules/deeper%2fsubmodule/modules/subsubmodule log >actual &&\n \t git add deeper/submodule &&\n \t git commit -m \"update submodule\" &&\n \t git push origin : &&\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354959","messageId":"CAGZ79kYvM5hxbe9ZCuFt=Cgv9W0mmdwdFGJz6+DdhPv4UbEXjQ@mail.gmail.com","threadId":"49065","inReplyTo":"20180808223323.79989-2-bmwill@google.com","subject":"Re: [PATCH 1/2] submodule: create helper to build paths to submodule gitdirs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-08T23:21:20Z","receivedAt":"2018-08-08T23:21:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 8, 2018 at 3:33 PM Brandon Williams wrote:\n>\n> Introduce a helper function \"submodule_name_to_gitdir()\" (and the\n> submodule--helper subcommand \"gitdir\") which constructs a path to a\n> submodule's gitdir, located in the provided repository's \"modules\"\n> directory.\n\nMakes sense.\n\n>\n> This consolidates the logic needed to build up a path into a\n> repository's \"modules\" directory, abstracting away the fact that\n> submodule git directories are stored in a repository's common gitdir.\n> This makes it easier to adjust how submodules gitdir are stored in the\n> \"modules\" directory in a future patch.\n\nand yet, all places that we touch were and still are broken for old-style\nsubmodules that have their git directory inside the working tree?\nDo we need to pay attention to those, too?\n\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 8b5ad59bde..053747d290 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n\n> @@ -577,7 +578,7 @@ cmd_update()\n> die \"$(eval_gettext \"Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n> fi\n>\n> - if ! $(git config -f \"$(git rev-parse --git-common-dir)/modules/$name/config\" core.worktree) 2>/dev/null\n> + if ! $(git config -f \"$(git submodule--helper gitdir \"$name\")/config\" core.worktree) 2>/dev/null\n\nThis will collide with origin/sb/submodule-update-in-c specifically\n1c866b9831d (submodule--helper: replace connect-gitdir-workingtree\nby ensure-core-worktree, 2018-08-03), but as that removes these lines,\nit should be easy to resolve the conflict.\n"},{"id":"354963","messageId":"20180809004548.GA219777@google.com","threadId":"49065","inReplyTo":"CAGZ79kYvM5hxbe9ZCuFt=Cgv9W0mmdwdFGJz6+DdhPv4UbEXjQ@mail.gmail.com","subject":"Re: [PATCH 1/2] submodule: create helper to build paths to submodule gitdirs","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-08-09T00:45:48Z","receivedAt":"2018-08-09T00:45:52Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/08, Stefan Beller wrote:\n> On Wed, Aug 8, 2018 at 3:33 PM Brandon Williams wrote:\n> >\n> > Introduce a helper function \"submodule_name_to_gitdir()\" (and the\n> > submodule--helper subcommand \"gitdir\") which constructs a path to a\n> > submodule's gitdir, located in the provided repository's \"modules\"\n> > directory.\n> \n> Makes sense.\n> \n> >\n> > This consolidates the logic needed to build up a path into a\n> > repository's \"modules\" directory, abstracting away the fact that\n> > submodule git directories are stored in a repository's common gitdir.\n> > This makes it easier to adjust how submodules gitdir are stored in the\n> > \"modules\" directory in a future patch.\n> \n> and yet, all places that we touch were and still are broken for old-style\n> submodules that have their git directory inside the working tree?\n> Do we need to pay attention to those, too?\n\nThis series only tries to address the issues with submodules stored in\n$GITDIR/modules/ and places in our codebase that explicitly reference\nsubmodules stored there.\n\nFor those old-old-style submodules, wouldn't the absorb submodule\nfunctions handle that migration?\n\n> \n> \n> > diff --git a/git-submodule.sh b/git-submodule.sh\n> > index 8b5ad59bde..053747d290 100755\n> > --- a/git-submodule.sh\n> > +++ b/git-submodule.sh\n> \n> > @@ -577,7 +578,7 @@ cmd_update()\n> > die \"$(eval_gettext \"Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n> > fi\n> >\n> > - if ! $(git config -f \"$(git rev-parse --git-common-dir)/modules/$name/config\" core.worktree) 2>/dev/null\n> > + if ! $(git config -f \"$(git submodule--helper gitdir \"$name\")/config\" core.worktree) 2>/dev/null\n> \n> This will collide with origin/sb/submodule-update-in-c specifically\n> 1c866b9831d (submodule--helper: replace connect-gitdir-workingtree\n> by ensure-core-worktree, 2018-08-03), but as that removes these lines,\n> it should be easy to resolve the conflict.\n\n-- \nBrandon Williams\n"},{"id":"355040","messageId":"20180809212602.GA11342@sigill.intra.peff.net","threadId":"49065","inReplyTo":"20180808223323.79989-3-bmwill@google.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-09T21:26:02Z","receivedAt":"2018-08-09T21:26:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 08, 2018 at 03:33:23PM -0700, Brandon Williams wrote:\n\n> Commit 0383bbb901 (submodule-config: verify submodule names as paths,\n> 2018-04-30) introduced some checks to ensure that submodule names don't\n> include directory traversal components (e.g. \"../\").\n> \n> This addresses the vulnerability identified in 0383bbb901 but the root\n> cause is that we use submodule names to construct paths to the\n> submodule's git directory. What we really should do is munge the\n> submodule name before using it to construct a path.\n> \n> Teach \"submodule_name_to_gitdir()\" to munge a submodule's name (by url\n> encoding it) before using it to build a path to the submodule's gitdir.\n\nI like this approach very much, and I think using url encoding is much\nbetter than an opaque hash (purely because it makes debugging and\ninspection saner).\n\nTwo thoughts, though:\n\n> +\tmodules_len = buf->len;\n> \tstrbuf_addstr(buf, submodule_name);\n> +\n> +\t/*\n> +\t * If the submodule gitdir already exists using the old-fashioned\n> +\t * location (which uses the submodule name as-is, without munging it)\n> +\t * then return that.\n> +\t */\n> +\tif (!access(buf->buf, F_OK))\n> +\t\treturn;\n\nI think this backwards-compatibility is necessary to avoid pain. But\nuntil it goes away, I don't think this is helping the vulnerability from\n0383bbb901. Because there the issue was that the submodule name pointed\nback into the working tree, so this access() would find the untrusted\nworking tree code and say \"ah, an old-fashioned name!\".\n\nIn theory a fresh clone could set a config option for \"I only speak\nuse new-style modules\". And there could even be a conversion program\nthat moves the modules as appropriate, fixes up the .git files in the\nworking tree, and then sets that config.\n\nIn fact, I think that config option _could_ be done by bumping\ncore.repositoryformatversion and then setting extensions.submodulenames\nto \"url\" or something. Then you could never run into the confusing case\nwhere you have a clone done by a new version of git (using new-style\nnames), but using an old-style version gets confused because it can't\nfind the module directories (instead, it would barf and say \"I don't\nknow about that extension\").\n\nI don't know if any of that is worth it, though. We already fixed the\nproblem from 0383bbb901. There may be a _different_ \"break out of the\nmodules directory\" vulnerability, but since we disallow \"..\" it's hard\nto see what it would be (the best I could come up with is maybe pointing\none module into the interior of another module, but I think you'd have\nto trouble overwriting anything useful).\n\nAnd while an old-style version of Git being confused might be annoying,\nI suspect that bumping the repository version would be even _more_\nannoying, because it would hit every command, not just ones that try to\ntouch those submodules.\n\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 2c2c97e144..963693332c 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -933,7 +933,7 @@ test_expect_success 'recursive relative submodules stay relative' '\n> \t\tcd clone2 &&\n> \t\tgit submodule update --init --recursive &&\n> \t\techo \"gitdir: ../.git/modules/sub3\" >./sub3/.git_expect &&\n> -\t\techo \"gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub\" >./sub3/dirdir/subsub/.git_expect\n> +\t\techo \"gitdir: ../../../.git/modules/sub3/modules/dirdir%2fsubsub\" >./sub3/dirdir/subsub/.git_expect\n\nOne interesting thing about url-encoding is that it's not one-to-one.\nThis case could also be %2F, which is a different file (on a\ncase-sensitive filesystem). I think \"%20\" and \"+\" are similarly\ninterchangeable.\n\nIf we were decoding the filenames, that's fine. The round-trip is\nlossless.\n\nBut that's not quite how the new code behaves. We encode the input and\nthen check to see if it matches an encoding we previously performed. So\nif our urlencode routines ever change, this will subtly break.\n\nI don't know how much it's worth caring about. We're not that likely to\nchange the routines ourself (though certainly a third-party\nimplementation would need to know our exact url-encoding decisions).\n\nSome possible actions:\n\n 0. Do nothing, and cross our fingers. ;)\n\n 1. Don't use strbuf_addstr_urlencode(), but rather our own munging\n function which we know will remain stable (or alternatively, a flag\n to strbuf_addstr_urlencode to get the consistent behavior).\n\n 2. Make sure we have tests which cover this, so at least somebody\n changing the urlencode decisions will see a breakage. Your test here\n covers the upper/lowercase one, but we might want one that covers\n \"+\". (There may be more ambiguous cases, but those are the ones I\n know about).\n\n 3. Rather than check for the existence of names, decode what's actually\n in the modules/ directory to create an in-memory index of names.\n\n I hesitate to suggest that, because it's obviously way more\n complicated, and may perform worse if you have a lot of modules\n (since you have to readdir() and decode the whole directory just to\n look up one module).\n\n But I think it also gives a more elegant solution to the\n backwards-compatibility problem, since we could recognize both new\n and old-style names. There's some ambiguity (e.g., is \"foo%2fbar\"\n \"foo/bar\", or did somebody really have a name with a percent in\n it?),. but in theory you could respect either name (giving\n preference to new-style in case of a conflict).\n\n And I think the result would be immune to any directory-escape\n vulnerabilities, because we'd always start with what actually exists\n in $GIT_DIR/modules/, which we know _we_ will have written.\n\n Again, I'm not sure if it's worth the effort, but I thought I'd\n throw it out there.\n\n-Peff\n"},{"id":"355160","messageId":"xmqqpnyp7vzv.fsf@gitster-ct.c.googlers.com","threadId":"49065","inReplyTo":"20180808223323.79989-2-bmwill@google.com","subject":"Re: [PATCH 1/2] submodule: create helper to build paths to submodule gitdirs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-10T21:27:32Z","receivedAt":"2018-08-10T21:27:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> Introduce a helper function \"submodule_name_to_gitdir()\" (and the\n> submodule--helper subcommand \"gitdir\") which constructs a path to a\n> submodule's gitdir, located in the provided repository's \"modules\"\n> directory.\n>\n> This consolidates the logic needed to build up a path into a\n> repository's \"modules\" directory, abstracting away the fact that\n> submodule git directories are stored in a repository's common gitdir.\n> This makes it easier to adjust how submodules gitdir are stored in the\n> \"modules\" directory in a future patch.\n>\n> Signed-off-by: Brandon Williams \n> ---\n> ...\n> @@ -2018,7 +2039,7 @@ static int connect_gitdir_workingtree(int argc, const char **argv, const char *p\n> \tname = argv[1];\n> \tpath = argv[2];\n> \n> -\tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n> +\tsubmodule_name_to_gitdir(&sb, the_repository, name);\n> \tsm_gitdir = absolute_pathdup(sb.buf);\n> \n> \tconnect_work_tree_and_git_dir(path, sm_gitdir, 0);\n\nThis function goes away with 1c866b98 (\"submodule--helper: replace\nconnect-gitdir-workingtree by ensure-core-worktree\", 2018-08-03) in\nsb/submodule-update-in-c topic. git-submodule.sh has simlar\nconflicts.\n\nI guess its replacement function does not care as deeply as its\npredecessor used to about where the submodule's $GIT_DIR is, so the\ncorrect resolution may be just to ignore the change made to this\ncaller to the new name-to-gitdir function.\n\nIt would have been nicer to see a bit better inter-developer\ncoordination, especially between two who sit practically next to\neach other ;-)\n\nThanks.\n"},{"id":"355164","messageId":"20180810214559.GA211322@google.com","threadId":"49065","inReplyTo":"xmqqpnyp7vzv.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] submodule: create helper to build paths to submodule gitdirs","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-08-10T21:45:59Z","receivedAt":"2018-08-10T21:46:04Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/10, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > Introduce a helper function \"submodule_name_to_gitdir()\" (and the\n> > submodule--helper subcommand \"gitdir\") which constructs a path to a\n> > submodule's gitdir, located in the provided repository's \"modules\"\n> > directory.\n> >\n> > This consolidates the logic needed to build up a path into a\n> > repository's \"modules\" directory, abstracting away the fact that\n> > submodule git directories are stored in a repository's common gitdir.\n> > This makes it easier to adjust how submodules gitdir are stored in the\n> > \"modules\" directory in a future patch.\n> >\n> > Signed-off-by: Brandon Williams \n> > ---\n> > ...\n> > @@ -2018,7 +2039,7 @@ static int connect_gitdir_workingtree(int argc, const char **argv, const char *p\n> > \tname = argv[1];\n> > \tpath = argv[2];\n> > \n> > -\tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n> > +\tsubmodule_name_to_gitdir(&sb, the_repository, name);\n> > \tsm_gitdir = absolute_pathdup(sb.buf);\n> > \n> > \tconnect_work_tree_and_git_dir(path, sm_gitdir, 0);\n> \n> This function goes away with 1c866b98 (\"submodule--helper: replace\n> connect-gitdir-workingtree by ensure-core-worktree\", 2018-08-03) in\n> sb/submodule-update-in-c topic. git-submodule.sh has simlar\n> conflicts.\n> \n> I guess its replacement function does not care as deeply as its\n> predecessor used to about where the submodule's $GIT_DIR is, so the\n> correct resolution may be just to ignore the change made to this\n> caller to the new name-to-gitdir function.\n\nWell that patch still cares about where the gitdir is except it\ninitializes a \"struct repository\" for the submodule and then builds a\npath to the config using:\n\n cfg_file = xstrfmt(\"%s/config\", subrepo.gitdir);\n\nhmm...I didn't get a chance to look at that series but that line looks\nwrong. It probably should be more like:\n\n cfg_file = repo_git_path(&subrepo, \"config\");\n\nI'll go comment on that series.\n\n> \n> It would have been nicer to see a bit better inter-developer\n> coordination, especially between two who sit practically next to\n> each other ;-)\n> \n> Thanks.\n\n-- \nBrandon Williams\n"},{"id":"355570","messageId":"20180814180406.GA86804@google.com","threadId":"49065","inReplyTo":"20180809212602.GA11342@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-08-14T18:04:06Z","receivedAt":"2018-08-14T18:04:11Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/09, Jeff King wrote:\n> On Wed, Aug 08, 2018 at 03:33:23PM -0700, Brandon Williams wrote:\n> \n> > Commit 0383bbb901 (submodule-config: verify submodule names as paths,\n> > 2018-04-30) introduced some checks to ensure that submodule names don't\n> > include directory traversal components (e.g. \"../\").\n> > \n> > This addresses the vulnerability identified in 0383bbb901 but the root\n> > cause is that we use submodule names to construct paths to the\n> > submodule's git directory. What we really should do is munge the\n> > submodule name before using it to construct a path.\n> > \n> > Teach \"submodule_name_to_gitdir()\" to munge a submodule's name (by url\n> > encoding it) before using it to build a path to the submodule's gitdir.\n> \n> I like this approach very much, and I think using url encoding is much\n> better than an opaque hash (purely because it makes debugging and\n> inspection saner).\n> \n> Two thoughts, though:\n> \n> > +\tmodules_len = buf->len;\n> > \tstrbuf_addstr(buf, submodule_name);\n> > +\n> > +\t/*\n> > +\t * If the submodule gitdir already exists using the old-fashioned\n> > +\t * location (which uses the submodule name as-is, without munging it)\n> > +\t * then return that.\n> > +\t */\n> > +\tif (!access(buf->buf, F_OK))\n> > +\t\treturn;\n> \n> I think this backwards-compatibility is necessary to avoid pain. But\n> until it goes away, I don't think this is helping the vulnerability from\n> 0383bbb901. Because there the issue was that the submodule name pointed\n> back into the working tree, so this access() would find the untrusted\n> working tree code and say \"ah, an old-fashioned name!\".\n> \n> In theory a fresh clone could set a config option for \"I only speak\n> use new-style modules\". And there could even be a conversion program\n> that moves the modules as appropriate, fixes up the .git files in the\n> working tree, and then sets that config.\n> \n> In fact, I think that config option _could_ be done by bumping\n> core.repositoryformatversion and then setting extensions.submodulenames\n> to \"url\" or something. Then you could never run into the confusing case\n> where you have a clone done by a new version of git (using new-style\n> names), but using an old-style version gets confused because it can't\n> find the module directories (instead, it would barf and say \"I don't\n> know about that extension\").\n> \n> I don't know if any of that is worth it, though. We already fixed the\n> problem from 0383bbb901. There may be a _different_ \"break out of the\n> modules directory\" vulnerability, but since we disallow \"..\" it's hard\n> to see what it would be (the best I could come up with is maybe pointing\n> one module into the interior of another module, but I think you'd have\n> to trouble overwriting anything useful).\n> \n> And while an old-style version of Git being confused might be annoying,\n> I suspect that bumping the repository version would be even _more_\n> annoying, because it would hit every command, not just ones that try to\n> touch those submodules.\n\nOh I know that this doesn't help with that vulnerability. As you've\nsaid we fix it and now disallow \"..\" at the submodule-config level so\nreally this path is simply about using what we get out of\nsubmodule-config in a more sane manor.\n\n> \n> > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> > index 2c2c97e144..963693332c 100755\n> > --- a/t/t7400-submodule-basic.sh\n> > +++ b/t/t7400-submodule-basic.sh\n> > @@ -933,7 +933,7 @@ test_expect_success 'recursive relative submodules stay relative' '\n> > \t\tcd clone2 &&\n> > \t\tgit submodule update --init --recursive &&\n> > \t\techo \"gitdir: ../.git/modules/sub3\" >./sub3/.git_expect &&\n> > -\t\techo \"gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub\" >./sub3/dirdir/subsub/.git_expect\n> > +\t\techo \"gitdir: ../../../.git/modules/sub3/modules/dirdir%2fsubsub\" >./sub3/dirdir/subsub/.git_expect\n> \n> One interesting thing about url-encoding is that it's not one-to-one.\n> This case could also be %2F, which is a different file (on a\n> case-sensitive filesystem). I think \"%20\" and \"+\" are similarly\n> interchangeable.\n> \n> If we were decoding the filenames, that's fine. The round-trip is\n> lossless.\n> \n> But that's not quite how the new code behaves. We encode the input and\n> then check to see if it matches an encoding we previously performed. So\n> if our urlencode routines ever change, this will subtly break.\n> \n> I don't know how much it's worth caring about. We're not that likely to\n> change the routines ourself (though certainly a third-party\n> implementation would need to know our exact url-encoding decisions).\n\nThis is exactly the reason why I wanted to get some opinions on what the\nbest thing to do here would be. I _think_ the best thing would probably\nbe to write a specific routine to do the conversion, and it wouldn't\neven have to be all that complex. Basically I'm just interested in\nconverting '/' characters so that things no longer behave like\nnested directories.\n\n> \n> Some possible actions:\n> \n> 0. Do nothing, and cross our fingers. ;)\n> \n> 1. Don't use strbuf_addstr_urlencode(), but rather our own munging\n> function which we know will remain stable (or alternatively, a flag\n> to strbuf_addstr_urlencode to get the consistent behavior).\n> \n> 2. Make sure we have tests which cover this, so at least somebody\n> changing the urlencode decisions will see a breakage. Your test here\n> covers the upper/lowercase one, but we might want one that covers\n> \"+\". (There may be more ambiguous cases, but those are the ones I\n> know about).\n> \n> 3. Rather than check for the existence of names, decode what's actually\n> in the modules/ directory to create an in-memory index of names.\n> \n> I hesitate to suggest that, because it's obviously way more\n> complicated, and may perform worse if you have a lot of modules\n> (since you have to readdir() and decode the whole directory just to\n> look up one module).\n> \n> But I think it also gives a more elegant solution to the\n> backwards-compatibility problem, since we could recognize both new\n> and old-style names. There's some ambiguity (e.g., is \"foo%2fbar\"\n> \"foo/bar\", or did somebody really have a name with a percent in\n> it?),. but in theory you could respect either name (giving\n> preference to new-style in case of a conflict).\n> \n> And I think the result would be immune to any directory-escape\n> vulnerabilities, because we'd always start with what actually exists\n> in $GIT_DIR/modules/, which we know _we_ will have written.\n> \n> Again, I'm not sure if it's worth the effort, but I thought I'd\n> throw it out there.\n> \n> -Peff\n\n-- \nBrandon Williams\n"},{"id":"355592","messageId":"20180814185743.GE142615@aiede.svl.corp.google.com","threadId":"49065","inReplyTo":"20180814180406.GA86804@google.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-14T18:57:43Z","receivedAt":"2018-08-14T18:57:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nBrandon Williams wrote:\n> On 08/09, Jeff King wrote:\n\n>> One interesting thing about url-encoding is that it's not one-to-one.\n>> This case could also be %2F, which is a different file (on a\n>> case-sensitive filesystem). I think \"%20\" and \"+\" are similarly\n>> interchangeable.\n>>\n>> If we were decoding the filenames, that's fine. The round-trip is\n>> lossless.\n>>\n>> But that's not quite how the new code behaves. We encode the input and\n>> then check to see if it matches an encoding we previously performed. So\n>> if our urlencode routines ever change, this will subtly break.\n>>\n>> I don't know how much it's worth caring about. We're not that likely to\n>> change the routines ourself (though certainly a third-party\n>> implementation would need to know our exact url-encoding decisions).\n>\n> This is exactly the reason why I wanted to get some opinions on what the\n> best thing to do here would be. I _think_ the best thing would probably\n> be to write a specific routine to do the conversion, and it wouldn't\n> even have to be all that complex. Basically I'm just interested in\n> converting '/' characters so that things no longer behave like\n> nested directories.\n\nFirst of all, I think the behavior with this patch is already much\nbetter than the previous status quo. I'm using the patch now and am\nvery happy with it.\n\nSecond, what if we store the pathname in config? We already store the\nURL there:\n\n\t[submodule \"plugins/hooks\"]\n\t\turl = https://gerrit.googlesource.com/plugins/hooks\n\nSo we could (as a followup patch) do something like\n\n\t[submodule \"plugins/hooks\"]\n\t\turl = https://gerrit.googlesource.com/plugins/hooks\n\t\tgitdirname = plugins%2fhooks\n\nand use that for lookups instead of regenerating the directory name.\nWhat do you think?\n\nThanks,\nJonathan\n"},{"id":"355593","messageId":"20180814185759.GA28452@sigill.intra.peff.net","threadId":"49065","inReplyTo":"20180814180406.GA86804@google.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-14T18:58:00Z","receivedAt":"2018-08-14T18:58:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 14, 2018 at 11:04:06AM -0700, Brandon Williams wrote:\n\n> > I think this backwards-compatibility is necessary to avoid pain. But\n> > until it goes away, I don't think this is helping the vulnerability from\n> > 0383bbb901. Because there the issue was that the submodule name pointed\n> > back into the working tree, so this access() would find the untrusted\n> > working tree code and say \"ah, an old-fashioned name!\".\n> [...]\n> \n> Oh I know that this doesn't help with that vulnerability. As you've\n> said we fix it and now disallow \"..\" at the submodule-config level so\n> really this path is simply about using what we get out of\n> submodule-config in a more sane manor.\n\nOK, I'm alright with that as long as we are all on the same page. I\nthink I mistook \"this addresses the vulnerability\" from your commit\nmessage the wrong way. I took it as \"this patch\", but reading it again,\nyou simply mean \"the '..' handling we already did\".\n\nI do think eventually dropping this back-compatibility could save us\nfrom another directory-escape problem, but it's hard to justify the\nreal-world pain for a hypothetical benefit. Maybe in a few years we\ncould get rid of it in a major version bump.\n\n> > One interesting thing about url-encoding is that it's not one-to-one.\n> > This case could also be %2F, which is a different file (on a\n> > case-sensitive filesystem). I think \"%20\" and \"+\" are similarly\n> > interchangeable.\n> > \n> > If we were decoding the filenames, that's fine. The round-trip is\n> > lossless.\n> > \n> > But that's not quite how the new code behaves. We encode the input and\n> > then check to see if it matches an encoding we previously performed. So\n> > if our urlencode routines ever change, this will subtly break.\n> > \n> > I don't know how much it's worth caring about. We're not that likely to\n> > change the routines ourself (though certainly a third-party\n> > implementation would need to know our exact url-encoding decisions).\n> \n> This is exactly the reason why I wanted to get some opinions on what the\n> best thing to do here would be. I _think_ the best thing would probably\n> be to write a specific routine to do the conversion, and it wouldn't\n> even have to be all that complex. Basically I'm just interested in\n> converting '/' characters so that things no longer behave like\n> nested directories.\n\nI think we benefit from catching names that would trigger filesystem\ncase-folding, too. If I have submodules with names \"foo\" and \"FOO\", we\nwould not want to confuse them (or at least we should confuse them\nequally on all platforms). I doubt you can do anything malicious, but it\nmight simply be annoying.\n\nThat implies to me using a custom function (even if its encoded form\nends up being understandable as url-encoding).\n\n-Peff\n"},{"id":"355623","messageId":"CAGZ79kZUq5jPqyb=B1ppEi1QhNGmhLXeV6vPn8ouR=YGEN32pg@mail.gmail.com","threadId":"49065","inReplyTo":"20180814185743.GE142615@aiede.svl.corp.google.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-14T21:08:01Z","receivedAt":"2018-08-14T21:08:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 14, 2018 at 11:57 AM Jonathan Nieder wrote:\n>\n> Hi,\n>\n> Brandon Williams wrote:\n> > On 08/09, Jeff King wrote:\n>\n> >> One interesting thing about url-encoding is that it's not one-to-one.\n> >> This case could also be %2F, which is a different file (on a\n> >> case-sensitive filesystem). I think \"%20\" and \"+\" are similarly\n> >> interchangeable.\n> >>\n> >> If we were decoding the filenames, that's fine. The round-trip is\n> >> lossless.\n> >>\n> >> But that's not quite how the new code behaves. We encode the input and\n> >> then check to see if it matches an encoding we previously performed. So\n> >> if our urlencode routines ever change, this will subtly break.\n> >>\n> >> I don't know how much it's worth caring about. We're not that likely to\n> >> change the routines ourself (though certainly a third-party\n> >> implementation would need to know our exact url-encoding decisions).\n> >\n> > This is exactly the reason why I wanted to get some opinions on what the\n> > best thing to do here would be. I _think_ the best thing would probably\n> > be to write a specific routine to do the conversion, and it wouldn't\n> > even have to be all that complex. Basically I'm just interested in\n> > converting '/' characters so that things no longer behave like\n> > nested directories.\n>\n> First of all, I think the behavior with this patch is already much\n> better than the previous status quo. I'm using the patch now and am\n> very happy with it.\n>\n> Second, what if we store the pathname in config? We already store the\n> URL there:\n>\n> [submodule \"plugins/hooks\"]\n> url = https://gerrit.googlesource.com/plugins/hooks\n>\n> So we could (as a followup patch) do something like\n>\n> [submodule \"plugins/hooks\"]\n> url = https://gerrit.googlesource.com/plugins/hooks\n> gitdirname = plugins%2fhooks\n>\n> and use that for lookups instead of regenerating the directory name.\n> What do you think?\n\nAs I just looked at worktree code, this sounds intriguing for the wrong\nreason (again), as a user may want to point the gitdirname to a repository\nthat they have already on disk outside the actual superproject. They\nwould be reinventing worktrees in the submodule space. ;-)\n\nThis would open up the security hole that we just had, again.\nSo we'd have to make sure that the gitdirname (instead of the\nnow meaningless subsection name) is proof to ../ attacks.\n\nI feel uneasy about this as then the user might come in\nand move submodules and repoint the gitdirname...\nto a not url encoded path. Exposing this knob just\nasks for trouble, no?\n\nOn the other hand, the only requirement for the \"name\" is\nnow uniqueness, and that is implied with subsections,\nso I guess it looks elegant.\n\nWhat would happen if gitdirname is changed as part of\nhistory? (The same problem we have now with changing\nthe subsection name)\n\nThe more I think about it the less appealing this is, but it looks\nelegant.\n\nStefan\n"},{"id":"355626","messageId":"20180814211211.GF142615@aiede.svl.corp.google.com","threadId":"49065","inReplyTo":"CAGZ79kZUq5jPqyb=B1ppEi1QhNGmhLXeV6vPn8ouR=YGEN32pg@mail.gmail.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-14T21:12:11Z","receivedAt":"2018-08-14T21:12:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n> On Tue, Aug 14, 2018 at 11:57 AM Jonathan Nieder wrote:\n\n>> Second, what if we store the pathname in config? We already store the\n>> URL there:\n>>\n>> [submodule \"plugins/hooks\"]\n>> url = https://gerrit.googlesource.com/plugins/hooks\n>>\n>> So we could (as a followup patch) do something like\n>>\n>> [submodule \"plugins/hooks\"]\n>> url = https://gerrit.googlesource.com/plugins/hooks\n>> gitdirname = plugins%2fhooks\n>>\n>> and use that for lookups instead of regenerating the directory name.\n>> What do you think?\n>\n> As I just looked at worktree code, this sounds intriguing for the wrong\n> reason (again), as a user may want to point the gitdirname to a repository\n> that they have already on disk outside the actual superproject. They\n> would be reinventing worktrees in the submodule space. ;-)\n>\n> This would open up the security hole that we just had, again.\n> So we'd have to make sure that the gitdirname (instead of the\n> now meaningless subsection name) is proof to ../ attacks.\n>\n> I feel uneasy about this as then the user might come in\n> and move submodules and repoint the gitdirname...\n> to a not url encoded path. Exposing this knob just\n> asks for trouble, no?\n\nWhat if we forbid directory separator characters in the gitdirname?\n\n[...]\n> What would happen if gitdirname is changed as part of\n> history? (The same problem we have now with changing\n> the subsection name)\n\nIn this proposal, it would only be read from config, not from\n.gitmodules.\n\nThanks,\nJonathan\n"},{"id":"355636","messageId":"CAGZ79kYfoK9hfXM2-VMAZLPpqBOFQYKtyYuYJb8twzz6Oz5ymQ@mail.gmail.com","threadId":"49065","inReplyTo":"20180814211211.GF142615@aiede.svl.corp.google.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-14T22:34:18Z","receivedAt":"2018-08-14T22:34:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 14, 2018 at 2:12 PM Jonathan Nieder wrote:\n>\n> Hi,\n>\n> Stefan Beller wrote:\n> > On Tue, Aug 14, 2018 at 11:57 AM Jonathan Nieder wrote:\n>\n> >> Second, what if we store the pathname in config? We already store the\n> >> URL there:\n> >>\n> >> [submodule \"plugins/hooks\"]\n> >> url = https://gerrit.googlesource.com/plugins/hooks\n> >>\n> >> So we could (as a followup patch) do something like\n> >>\n> >> [submodule \"plugins/hooks\"]\n> >> url = https://gerrit.googlesource.com/plugins/hooks\n> >> gitdirname = plugins%2fhooks\n> >>\n> >> and use that for lookups instead of regenerating the directory name.\n> >> What do you think?\n> >\n> > As I just looked at worktree code, this sounds intriguing for the wrong\n> > reason (again), as a user may want to point the gitdirname to a repository\n> > that they have already on disk outside the actual superproject. They\n> > would be reinventing worktrees in the submodule space. ;-)\n> >\n> > This would open up the security hole that we just had, again.\n> > So we'd have to make sure that the gitdirname (instead of the\n> > now meaningless subsection name) is proof to ../ attacks.\n> >\n> > I feel uneasy about this as then the user might come in\n> > and move submodules and repoint the gitdirname...\n> > to a not url encoded path. Exposing this knob just\n> > asks for trouble, no?\n>\n> What if we forbid directory separator characters in the gitdirname?\n\nFine with me, but ideally we'd want to allow sharding the\nsubmodules. When you have 1000 submodules\nwe'd want them not all inside the toplevel \"modules/\" ?\nUp to now we could just wave hands and claim the user\n(who is clearly experienced with submodules as they\nuse so many of them) would shard it properly.\n\nWith this scheme we loose the ability to shard.\n\n> [...]\n> > What would happen if gitdirname is changed as part of\n> > history? (The same problem we have now with changing\n> > the subsection name)\n>\n> In this proposal, it would only be read from config, not from\n> .gitmodules.\n\nAh good point. That makes sense.\n\nStepping back a bit regarding the config:\nWhen I clone gerrit (or any repo using submodules)\n\n$ git clone --recurse-submodules \\\n https://gerrit.googlesource.com/gerrit g2\n[...]\n$ cat g2/.git/config\n[submodule]\n active = .\n[submodule \"plugins/codemirror-editor\"]\n url = https://gerrit.googlesource.com/plugins/codemirror-editor\n[... more urls to follow...]\n\nOriginally we have had the url in the config, (a) that we can change\nthe URLs after the \"git submodule init\" and \"git submodule update\"\nstep that actually clones the submodule if not present and much more\nimportantly (b) to know which submodule \"was initialized/active\".\n\nNow that we have the submodule.active or even\nsubmodule..active flags, we do not need (b) any more.\nSo the URL turns into a useless piece of cruft that just is unneeded\nand might confuse the user.\n\nSo maybe I'd want to propose a patch that removes\nsubmodule..url from the config once it is cloned.\n(I just read up on \"submodule sync\" again, but that might not\neven need special care for this new world)\n\nAnd with all that said, I think if we can avoid having the submodules\ngitdir in the config, the config would look much cleaner, too.\n\nBut maybe that is the wrong thing to optimize for. ;-)\nIt just demonstrates that we'd have a submodule specific\nthing again in the config.\n\nSo my preference would be to do a similar thing as\nurl-encoding as that solves the issue of slashes and\npotentially of case sensitivity (e.g. encode upper case A\nas lower case with underscore _a)\n\nHowever the transition worries me, as it transitions\nwithin the same namespace. Back then when we\ntransferred from the .git dir inside the submodules\nworking tree to the embedded version in the superprojects\n.git dir, there was no overlap, and any potential directory\nin .git/modules/ that was already there, was highly\nunusual, so asking the user for help is the reasonable\nthing to do.\nBut now we might run into issues that has overlap between\nold(name as is) and new (urlencoded) world.\n\nSo maybe we also want to transition from\n\n modules/\n\nto\n\n submodules/)>\n\nThanks,\nStefan\n"},{"id":"355787","messageId":"20180816001939.GA31703@pug.qqx.org","threadId":"49065","inReplyTo":"20180808223323.79989-3-bmwill@google.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Aaron Schrab","fromEmail":"aaron@schrab.com","sentAt":"2018-08-16T00:19:39Z","receivedAt":"2018-08-16T00:19:41Z","isPatch":true,"sender":{"key":"aaron@schrab.com","avatar":"https://avatars.githubusercontent.com/u/39620?v=4"},"body":"At 15:33 -0700 08 Aug 2018, Brandon Williams wrote:\n>Teach \"submodule_name_to_gitdir()\" to munge a submodule's name (by url\n>encoding it) before using it to build a path to the submodule's gitdir.\n\nSeems like this will be a problem if it results in names that exceed \nNAME_MAX? On common systems that's 255, so it's probably not going to be \ncommon; but it certainly could for some repositories.\n"},{"id":"355796","messageId":"20180816023446.GA127655@aiede.svl.corp.google.com","threadId":"49065","inReplyTo":"CAGZ79kYfoK9hfXM2-VMAZLPpqBOFQYKtyYuYJb8twzz6Oz5ymQ@mail.gmail.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-16T02:34:46Z","receivedAt":"2018-08-16T02:34:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi again,\n\nStefan Beller wrote:\n> On Tue, Aug 14, 2018 at 2:12 PM Jonathan Nieder wrote:\n\n>> What if we forbid directory separator characters in the gitdirname?\n>\n> Fine with me, but ideally we'd want to allow sharding the\n> submodules. When you have 1000 submodules\n> we'd want them not all inside the toplevel \"modules/\" ?\n\nThat's a good reason to permit slashes in the gitdirname.\n\nIf I understood the rest of your reply correctly, your worry was about\ndangerous gitdirname values in .gitmodules. I never had any wish to\nread them from there anyway, so this worry hopefully goes away.\n\n[...]\n>> In this proposal, it would only be read from config, not from\n>> .gitmodules.\n>\n> Ah good point. That makes sense.\n>\n> Stepping back a bit regarding the config:\n[...]\n> Now that we have the submodule.active or even\n> submodule..active flags, we do not need (b) any more.\n> So the URL turns into a useless piece of cruft that just is unneeded\n> and might confuse the user.\n>\n> So maybe I'd want to propose a patch that removes\n> submodule..url from the config once it is cloned.\n> (I just read up on \"submodule sync\" again, but that might not\n> even need special care for this new world)\n>\n> And with all that said, I think if we can avoid having the submodules\n> gitdir in the config, the config would look much cleaner, too.\n\nYes, I understand and agree with this.\n\nI should further spell out my motivation with this gitdirname\nsuggestion. The issue that some people have mentioned in this thread\nis that urlencoding might not be perfect --- it's pretty close to\nperfect, but it's likely we'll come up with some unanticipated needs\nlater (like sharding) that it doesn't solve. Solving those all right\nnow would not necessarily be wise, since the thing about unanticipated\nneeds is that you never know in advance what they will be. ;-)\n\nSo it would be nice, for future-proofing, if we can change the naming\nscheme later.\n\nAs a bonus, that would also make interoperability with other\nimplementations easier. For example, suppose we mess up in JGit and\nurlencode a different set of characters than Git does. Then a mixed\nGit + JGit installation would have this subtle bug of the submodule\n.git directory not being reused when I switch to and from and branch\nnot containing that submodule, in some circumstances. That sounds\ndifficult to support.\n\nWhereas if we have a gitdirname configuration variable, then JGit and\nlibgit2 and go-git do not have to match the naming scheme Git chooses.\nThey can try, but if one gets it subtly wrong then that is okay\nbecause the submodule's directory name is right there and easy to look\nup.\n\nAll at the cost of recording a little configuration somewhere. If we\nwant to decrease the configuration, we can avoid recording it there in\nthe easy cases (e.g. when name == gitdirname). That's \"just\" an\noptimization.\n\nAnd then we have the ability later to handle all the edge cases we\nhaven't handled yet today:\n\n- sharding when the number of submodules is too large\n- case-insensitive filesystems\n- path name length limits\n- different sets of filesystem-special characters\n\nSane?\n\nThanks,\nJonathan\n"},{"id":"355798","messageId":"CAGZ79kamoPjX_yWYABLoyTh8jqAPV4iVX0r46q=41B12zku=tg@mail.gmail.com","threadId":"49065","inReplyTo":"20180816023446.GA127655@aiede.svl.corp.google.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-16T02:39:42Z","receivedAt":"2018-08-16T02:39:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> [...]\n\nall good reasons; ship it :-)\n\n> All at the cost of recording a little configuration somewhere. If we\n> want to decrease the configuration, we can avoid recording it there in\n> the easy cases (e.g. when name == gitdirname). That's \"just\" an\n> optimization.\n\nSounds good, but gerrit for example would not take advantage of such\noptimisation as they have slashes in their submodules. :-(\nI wonder if we can optimize further and keep slashes if there is\nno conflict (as then name == gitdirname, so it can be optimized).\n\n> And then we have the ability later to handle all the edge cases we\n> haven't handled yet today:\n>\n> - sharding when the number of submodules is too large\n> - case-insensitive filesystems\n> - path name length limits\n> - different sets of filesystem-special characters\n>\n> Sane?\n\nI'll keep thinking about it.\n\nFYI: the reduction in configuration was just sent out.\n\nThanks,\nStefan\n"},{"id":"355799","messageId":"20180816024733.GB127655@aiede.svl.corp.google.com","threadId":"49065","inReplyTo":"CAGZ79kamoPjX_yWYABLoyTh8jqAPV4iVX0r46q=41B12zku=tg@mail.gmail.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-16T02:47:33Z","receivedAt":"2018-08-16T02:47:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n> Jonathan Nieder wrote:\n\n>> All at the cost of recording a little configuration somewhere. If we\n>> want to decrease the configuration, we can avoid recording it there in\n>> the easy cases (e.g. when name == gitdirname). That's \"just\" an\n>> optimization.\n>\n> Sounds good, but gerrit for example would not take advantage of such\n> optimisation as they have slashes in their submodules. :-(\n> I wonder if we can optimize further and keep slashes if there is\n> no conflict (as then name == gitdirname, so it can be optimized).\n\nOne possibility would be to treat gsub(\"/\", \"%2f\") as another of the\neasy cases.\n\n[...]\n>> And then we have the ability later to handle all the edge cases we\n>> haven't handled yet today:\n>>\n>> - sharding when the number of submodules is too large\n>> - case-insensitive filesystems\n>> - path name length limits\n>> - different sets of filesystem-special characters\n>>\n>> Sane?\n>\n> I'll keep thinking about it.\n\nThanks.\n\n> FYI: the reduction in configuration was just sent out.\n\nhttps://public-inbox.org/git/20180816023100.161626-1-sbeller@google.com/\nfor those following along.\n\nCiao,\nJonathan\n"},{"id":"355836","messageId":"xmqqh8jugxk4.fsf@gitster-ct.c.googlers.com","threadId":"49065","inReplyTo":"20180816023446.GA127655@aiede.svl.corp.google.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-16T15:07:07Z","receivedAt":"2018-08-16T15:07:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> So it would be nice, for future-proofing, if we can change the naming\n> scheme later.\n> ...\n> All at the cost of recording a little configuration somewhere. If we\n> want to decrease the configuration, we can avoid recording it there in\n> the easy cases (e.g. when name == gitdirname). That's \"just\" an\n> optimization.\n>\n> And then we have the ability later to handle all the edge cases we\n> haven't handled yet today:\n>\n> - sharding when the number of submodules is too large\n> - case-insensitive filesystems\n> - path name length limits\n> - different sets of filesystem-special characters\n>\n> Sane?\n\nYup.\n\n"},{"id":"355856","messageId":"20180816173412.GA212462@google.com","threadId":"49065","inReplyTo":"20180816024733.GB127655@aiede.svl.corp.google.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-08-16T17:34:12Z","receivedAt":"2018-08-16T17:34:17Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/15, Jonathan Nieder wrote:\n> Stefan Beller wrote:\n> > Jonathan Nieder wrote:\n> \n> >> All at the cost of recording a little configuration somewhere. If we\n> >> want to decrease the configuration, we can avoid recording it there in\n> >> the easy cases (e.g. when name == gitdirname). That's \"just\" an\n> >> optimization.\n> >\n> > Sounds good, but gerrit for example would not take advantage of such\n> > optimisation as they have slashes in their submodules. :-(\n> > I wonder if we can optimize further and keep slashes if there is\n> > no conflict (as then name == gitdirname, so it can be optimized).\n> \n> One possibility would be to treat gsub(\"/\", \"%2f\") as another of the\n> easy cases.\n> \n> [...]\n> >> And then we have the ability later to handle all the edge cases we\n> >> haven't handled yet today:\n> >>\n> >> - sharding when the number of submodules is too large\n> >> - case-insensitive filesystems\n> >> - path name length limits\n> >> - different sets of filesystem-special characters\n> >>\n> >> Sane?\n\nSeems like a sensible thing to do. Let me work up some patches to\nimplement this using config primarily and these other schemes as\nfallbacks.\n\n> >\n> > I'll keep thinking about it.\n> \n> Thanks.\n> \n> > FYI: the reduction in configuration was just sent out.\n> \n> https://public-inbox.org/git/20180816023100.161626-1-sbeller@google.com/\n> for those following along.\n> \n> Ciao,\n> Jonathan\n\n-- \nBrandon Williams\n"},{"id":"355866","messageId":"20180816181940.46114-1-bmwill@google.com","threadId":"49065","inReplyTo":"20180816024733.GB127655@aiede.svl.corp.google.com","subject":"[PATCH] submodule: add config for where gitdirs are located","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-08-16T18:19:40Z","receivedAt":"2018-08-16T18:19:54Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Introduce the config \"submodule..gitdirpath\" which is used to\nindicate where a submodule's gitdir is located inside of a repository's\n\"modules\" directory.\n\nSigned-off-by: Brandon Williams \n---\n\nMaybe something like this on top? Do you think we should disallow \"../\"\nin this config, even though it is a repository local configuration and\nnot shipped in .gitmodules?\n\n submodule.c | 13 ++++++++++++-\n t/t7400-submodule-basic.sh | 10 ++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 4854d88ce8..0cb00a9f24 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1966,16 +1966,27 @@ void submodule_name_to_gitdir(struct strbuf *buf, struct repository *r,\n \t\t\t const char *submodule_name)\n {\n \tsize_t modules_len;\n+\tchar *key;\n+\tchar *gitdir_path;\n \n \tstrbuf_git_common_path(buf, r, \"modules/\");\n \tmodules_len = buf->len;\n-\tstrbuf_addstr(buf, submodule_name);\n+\n+\tkey = xstrfmt(\"submodule.%s.gitdirpath\", submodule_name);\n+\tif (!repo_config_get_string(r, key, &gitdir_path)) {\n+\t\tstrbuf_addstr(buf, gitdir_path);\n+\t\tfree(key);\n+\t\tfree(gitdir_path);\n+\t\treturn;\n+\t}\n+\tfree(key);\n \n \t/*\n \t * If the submodule gitdir already exists using the old-fashioned\n \t * location (which uses the submodule name as-is, without munging it)\n \t * then return that.\n \t */\n+\tstrbuf_addstr(buf, submodule_name);\n \tif (!access(buf->buf, F_OK))\n \t\treturn;\n \ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 963693332c..1555329a2f 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1351,6 +1351,16 @@ test_expect_success 'resolve submodule gitdir in superprojects modules directory\n \t$(git -C superproject rev-parse --git-common-dir)/modules/sub/module\n \tEOF\n \tgit -C superproject submodule--helper gitdir \"sub/module\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Test using \"submodule..gitdirpath\" config for where the submodules\n+\t# gitdir is located inside the superprojecs \"modules\" directory\n+\tmv superproject/.git/modules/sub/module superproject/.git/modules/submodule &&\n+\tcat >expect <<-EOF &&\n+\t$(git -C superproject rev-parse --git-common-dir)/modules/submodule\n+\tEOF\n+\tgit -C superproject config \"submodule.sub/module.gitdirpath\" \"submodule\" &&\n+\tgit -C superproject submodule--helper gitdir \"sub/module\" >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.18.0.865.gffc8e1a3cd6-goog\n\n"},{"id":"356144","messageId":"xmqq1sasbsra.fsf@gitster-ct.c.googlers.com","threadId":"49065","inReplyTo":"20180816181940.46114-1-bmwill@google.com","subject":"Re: [PATCH] submodule: add config for where gitdirs are located","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-20T22:03:21Z","receivedAt":"2018-08-20T22:03:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> Introduce the config \"submodule..gitdirpath\" which is used to\n> indicate where a submodule's gitdir is located inside of a repository's\n> \"modules\" directory.\n>\n> Signed-off-by: Brandon Williams \n> ---\n>\n> Maybe something like this on top? Do you think we should disallow\n> \"../\" in this config, even though it is a repository local\n> configuration and not shipped in .gitmodules?\n\nSounds sensible to start strict and loosen later if/when necessary.\n\nIf we disallow \"../\", shouldn't we also reject an absolute path\n(meaning, you can only specify a subdirectory of $GIT_DIR/modules/\nin the super-project)?\n"},{"id":"356770","messageId":"CAGZ79kaLXcTeeM9AKvXi7X8WMd+vcyCM5n-Nz2igHkGJdXbSfg@mail.gmail.com","threadId":"49065","inReplyTo":"20180814180406.GA86804@google.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-28T21:35:25Z","receivedAt":"2018-08-28T21:35:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> > > - echo \"gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub\" >./sub3/dirdir/subsub/.git_expect\n> > > + echo \"gitdir: ../../../.git/modules/sub3/modules/dirdir%2fsubsub\" >./sub3/dirdir/subsub/.git_expect\n> >\n> > One interesting thing about url-encoding is that it's not one-to-one.\n> > This case could also be %2F, which is a different file (on a\n> > case-sensitive filesystem). I think \"%20\" and \"+\" are similarly\n> > interchangeable.\n> >\n> > If we were decoding the filenames, that's fine. The round-trip is\n> > lossless.\n> >\n> > But that's not quite how the new code behaves. We encode the input and\n> > then check to see if it matches an encoding we previously performed. So\n> > if our urlencode routines ever change, this will subtly break.\n\nAnd this is the problem:\na) we have a 'complicated' encoding here, which must never change\nb) the \"encode and check if it matches\", will produce ugly code going forward,\n as it tries to differentiate between submodules named \"url_encoded(a)\"\n and \"a\" (e.g. \"a%20b\" and \"a b\" would conflict and we have to resolve\n the conflict, although those two names are perfectly fine as they do not\n have the original problem of having slashes)\n\nHence I would propose a simpler encoding:\n\n1) / -> _ ( replace a slash by an underscore)\n2) _ -> __ (replace any underscore by 2 underscores, this is just the\n escaping mechanism to differentiate a/b and a_b)\n\n3) (optional) instead of putting it all in modules/, use another\ndirectory gitmodules/\n for example. this will make sure we can tell if a repository has\nbeen converted\n or is stuck with a setup of a current git.\n\n> This is exactly the reason why I wanted to get some opinions on what the\n> best thing to do here would be. I _think_ the best thing would probably\n> be to write a specific routine to do the conversion, and it wouldn't\n> even have to be all that complex. Basically I'm just interested in\n> converting '/' characters so that things no longer behave like\n> nested directories.\n\nYeah, then let's just convert '/' with as little overhead as possible.\n\nThanks,\nStefan\n"},{"id":"356813","messageId":"20180829052519.GA17253@sigill.intra.peff.net","threadId":"49065","inReplyTo":"CAGZ79kaLXcTeeM9AKvXi7X8WMd+vcyCM5n-Nz2igHkGJdXbSfg@mail.gmail.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-29T05:25:19Z","receivedAt":"2018-08-29T05:25:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 28, 2018 at 02:35:25PM -0700, Stefan Beller wrote:\n\n> 3) (optional) instead of putting it all in modules/, use another\n> directory gitmodules/ for example. this will make sure we can tell\n> if a repository has been converted or is stuck with a setup of a\n> current git.\n\nI actually kind of like that idea, as it makes the interaction between\nold and new names much simpler to reason about.\n\nAnd since old code won't know about the new names anyway, there's in\ntheory no downside. In practice, of course, the encoding may often be a\nnoop, and lazy scripts would continue to work most of the time if you\ndidn't change out the prefix directory. I'm not sure if that is an\nargument for the scheme (because it will suss out broken scripts more\nconsistently) or against it (because 99% of the time those old scripts\nwould just happen to work).\n\n> > This is exactly the reason why I wanted to get some opinions on what the\n> > best thing to do here would be. I _think_ the best thing would probably\n> > be to write a specific routine to do the conversion, and it wouldn't\n> > even have to be all that complex. Basically I'm just interested in\n> > converting '/' characters so that things no longer behave like\n> > nested directories.\n> \n> Yeah, then let's just convert '/' with as little overhead as possible.\n\nDo you care about case-folding issues (e.g., submodules \"FOO\" and \"foo\"\ncolliding)?\n\nI'm OK if the answer is \"no\", but if you do want to deal with it, the\ntime is probably now.\n\n-Peff\n"},{"id":"356865","messageId":"CAGZ79kZv4BjRq=kq_1UeT2Kn38OZwYFgnMsTe6X_WP41=hBtSQ@mail.gmail.com","threadId":"49065","inReplyTo":"20180829052519.GA17253@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-29T18:10:51Z","receivedAt":"2018-08-29T18:11:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 28, 2018 at 10:25 PM Jeff King wrote:\n>\n> On Tue, Aug 28, 2018 at 02:35:25PM -0700, Stefan Beller wrote:\n>\n> > 3) (optional) instead of putting it all in modules/, use another\n> > directory gitmodules/ for example. this will make sure we can tell\n> > if a repository has been converted or is stuck with a setup of a\n> > current git.\n>\n> I actually kind of like that idea, as it makes the interaction between\n> old and new names much simpler to reason about.\n>\n> And since old code won't know about the new names anyway, there's in\n> theory no downside. In practice, of course, the encoding may often be a\n> noop, and lazy scripts would continue to work most of the time if you\n> didn't change out the prefix directory. I'm not sure if that is an\n> argument for the scheme (because it will suss out broken scripts more\n> consistently) or against it (because 99% of the time those old scripts\n> would just happen to work).\n>\n> > > This is exactly the reason why I wanted to get some opinions on what the\n> > > best thing to do here would be. I _think_ the best thing would probably\n> > > be to write a specific routine to do the conversion, and it wouldn't\n> > > even have to be all that complex. Basically I'm just interested in\n> > > converting '/' characters so that things no longer behave like\n> > > nested directories.\n> >\n> > Yeah, then let's just convert '/' with as little overhead as possible.\n>\n> Do you care about case-folding issues (e.g., submodules \"FOO\" and \"foo\"\n> colliding)?\n\nI do. :(\n\n2d84f13dcb6 (config: fix case sensitive subsection names on writing, 2018-08-08)\nexplains the latest episode of case folding with submodules involved.\n\n> I'm OK if the answer is \"no\", but if you do want to deal with it, the\n> time is probably now.\n\nGood point. But as soon as we start discussing case sensitivity, we\nare drawn down the rabbit hole of funny file names. (Try naming\na submodule \"CON1\" and obtain it on Windows for example)\nSo we would need to have a file system specific encoding function for\nsubmodule names, which sounds like a maintenance night mare.\n\nThe CON1 example shows that URL encoding may not be enough\non Windows and we'd have to extend the encoding if we care about\nFS issues.\n\nAnother example would be \"a\" and \"a\\b\" which would be a mess\nin Windows as the '\\' would work as a dir separator whereas these\ntwo names were ok on linux. This would be fixed with url encoding.\n\nURL encoding would not fix the case-folding issue that you\nmentioned above.\n\nSo if I was thinking in the scheme presented above, we could just\nhave another rule that is\n\n [A-Z] -> _[a-z]\n\n(lowercase capital letters and escape them with an underscore)\n\nBut with that rule added, we are inventing a really complicated\nencoding scheme already.\n"},{"id":"356883","messageId":"20180829210348.GA29880@sigill.intra.peff.net","threadId":"49065","inReplyTo":"CAGZ79kZv4BjRq=kq_1UeT2Kn38OZwYFgnMsTe6X_WP41=hBtSQ@mail.gmail.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-29T21:03:48Z","receivedAt":"2018-08-29T21:03:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 29, 2018 at 11:10:51AM -0700, Stefan Beller wrote:\n\n> > Do you care about case-folding issues (e.g., submodules \"FOO\" and \"foo\"\n> > colliding)?\n> \n> I do. :(\n> \n> 2d84f13dcb6 (config: fix case sensitive subsection names on writing, 2018-08-08)\n> explains the latest episode of case folding with submodules involved.\n> \n> > I'm OK if the answer is \"no\", but if you do want to deal with it, the\n> > time is probably now.\n> \n> Good point. But as soon as we start discussing case sensitivity, we\n> are drawn down the rabbit hole of funny file names. (Try naming\n> a submodule \"CON1\" and obtain it on Windows for example)\n> So we would need to have a file system specific encoding function for\n> submodule names, which sounds like a maintenance night mare.\n\nHmph. I'd hoped that simply escaping metacharacters and doing some\nobvious case-folding would be enough. And I think that would cover most\naccidental cases. But yeah, Windows reserved names are basically\nindistinguishable from reasonable names. They'd probably need\nspecial-cased.\n\nOTOH, I'm not sure how we handle those for entries in the actual tree.\nPoking around git-for-windows/git, I think it uses the magic \"\\\\?\"\nmarker to tell the OS to interpret the name literally.\n\nSo I wonder if it might be sufficient to just deal with the more obvious\nfolding issues. Or as you noted, if we just choose lowercase names as\nthe normalized form, that might also be enough. :)\n\n> So if I was thinking in the scheme presented above, we could just\n> have another rule that is\n> \n> [A-Z] -> _[a-z]\n> \n> (lowercase capital letters and escape them with an underscore)\n\nYes, that makes even the capitalized \"CON\" issues go away. It's not a\none-to-one mapping, though (\"foo-\" and \"foo_\" map to the same entity).\n\nIf we want that, too, I think something like url-encoding is fine, with\nthe caveat that we simply urlencode _more_ things (i.e., anything not in\n[a-z_]).\n\n-Peff\n"},{"id":"356885","messageId":"20180829210913.GF7547@aiede.svl.corp.google.com","threadId":"49065","inReplyTo":"20180829052519.GA17253@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-29T21:09:13Z","receivedAt":"2018-08-29T21:09:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Tue, Aug 28, 2018 at 02:35:25PM -0700, Stefan Beller wrote:\n\n>> Yeah, then let's just convert '/' with as little overhead as possible.\n>\n> Do you care about case-folding issues (e.g., submodules \"FOO\" and \"foo\"\n> colliding)?\n>\n> I'm OK if the answer is \"no\", but if you do want to deal with it, the\n> time is probably now.\n\nHave we rejected the config approach? I really liked the attribute of\nnot having to solve everything right away. I'm getting scared that\nwe've forgotten that goal.\n\nIt mixes well with Stefan's idea of setting up a new .git/submodules/\ndirectory. We could require that everything in .git/submodules/ have\nconfiguration (or that everything in that directory either have\nconfiguration or be the result of a \"very simple\" transformation) and\nthat way, all ambiguity goes away.\n\nPart of the definition of \"very simple\" could be that the submodule\nname must consist of some whitelisted list of characters (including no\nuppercase), for example.\n\nThanks,\nJonathan\n"},{"id":"356886","messageId":"CAGZ79kYJTWROYSGjEbdVBsEAkWkNE4QVCiPVfuMf75d13fXN6A@mail.gmail.com","threadId":"49065","inReplyTo":"20180829210348.GA29880@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-29T21:10:37Z","receivedAt":"2018-08-29T21:10:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Yes, that makes even the capitalized \"CON\" issues go away. It's not a\n> one-to-one mapping, though (\"foo-\" and \"foo_\" map to the same entity).\n\nfoo_ would map to foo__, and foo- would map to something else.\n(foo- as we do not rewrite dashes, yet?)\n\n>\n> If we want that, too, I think something like url-encoding is fine, with\n> the caveat that we simply urlencode _more_ things (i.e., anything not in\n> [a-z_]).\n\nYeah I think we need more than url encoding now.\n"},{"id":"356887","messageId":"CAGZ79kafLRXag0DBmw57sJ0WdTUEckCejKFz0j6UJVNdG7_UDA@mail.gmail.com","threadId":"49065","inReplyTo":"20180829210913.GF7547@aiede.svl.corp.google.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-29T21:14:30Z","receivedAt":"2018-08-29T21:14:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 29, 2018 at 2:09 PM Jonathan Nieder wrote:\n>\n> Jeff King wrote:\n> > On Tue, Aug 28, 2018 at 02:35:25PM -0700, Stefan Beller wrote:\n>\n> >> Yeah, then let's just convert '/' with as little overhead as possible.\n> >\n> > Do you care about case-folding issues (e.g., submodules \"FOO\" and \"foo\"\n> > colliding)?\n> >\n> > I'm OK if the answer is \"no\", but if you do want to deal with it, the\n> > time is probably now.\n>\n> Have we rejected the config approach?\n\nI did not reject that approach, but am rather waiting for patches. ;-)\n\n> I really liked the attribute of\n> not having to solve everything right away. I'm getting scared that\n> we've forgotten that goal.\n\nEh, sorry for side tracking this issue.\n\nI am just under the impression that the URL encoding is not particularly\ngood for our use case as it solves just one out of many things, whereas\nthe one thing (having no slashes) can also be solved in an easier way.\n\n> It mixes well with Stefan's idea of setting up a new .git/submodules/\n> directory. We could require that everything in .git/submodules/ have\n> configuration (or that everything in that directory either have\n> configuration or be the result of a \"very simple\" transformation) and\n> that way, all ambiguity goes away.\n\nI would not want to have a world where we require that config, but I\nwould agree to the latter, hence we would need to discuss \"very simple\".\nI guess that are 2 or 3 rules at most.\n\n> Part of the definition of \"very simple\" could be that the submodule\n> name must consist of some whitelisted list of characters (including no\n> uppercase), for example.\n\nGood catch!\n\nThanks for chiming in,\nStefan\n"},{"id":"356888","messageId":"20180829211802.GG7547@aiede.svl.corp.google.com","threadId":"49065","inReplyTo":"CAGZ79kYJTWROYSGjEbdVBsEAkWkNE4QVCiPVfuMf75d13fXN6A@mail.gmail.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-29T21:18:02Z","receivedAt":"2018-08-29T21:18:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n>> Yes, that makes even the capitalized \"CON\" issues go away. It's not a\n>> one-to-one mapping, though (\"foo-\" and \"foo_\" map to the same entity).\n>\n> foo_ would map to foo__, and foo- would map to something else.\n> (foo- as we do not rewrite dashes, yet?)\n>\n>> If we want that, too, I think something like url-encoding is fine, with\n>> the caveat that we simply urlencode _more_ things (i.e., anything not in\n>> [a-z_]).\n>\n> Yeah I think we need more than url encoding now.\n\nCan you say more? Perhaps my expectations have been poisoned by tools\nlike dpkg-buildpackage that use urlencode. As far as I can tell, it\nworks fine.\n\nMoreover, urlencode has some attributes that make it a good potential\nfit: it's intuitive, it's unambiguous (yes, it's one-to-many, but at\nleast it's not many-to-many), and people know how to deal with it from\ntheir lives using browsers. Can you spell out for me what problem\nwe're solving with something more custom?\n\nStepping back, I am very worried about any design that doesn't give us\nthe ability to tweak things later. See [1] and [2] for more on that\nsubject.\n\nThanks,\nJonathan\n\n[1] https://public-inbox.org/git/20180816023446.GA127655@aiede.svl.corp.google.com/\n[2] https://public-inbox.org/git/20180829210913.GF7547@aiede.svl.corp.google.com/\n"},{"id":"356890","messageId":"20180829212504.GA72254@google.com","threadId":"49065","inReplyTo":"CAGZ79kafLRXag0DBmw57sJ0WdTUEckCejKFz0j6UJVNdG7_UDA@mail.gmail.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-08-29T21:25:04Z","receivedAt":"2018-08-29T21:25:09Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/29, Stefan Beller wrote:\n> On Wed, Aug 29, 2018 at 2:09 PM Jonathan Nieder wrote:\n> >\n> > Jeff King wrote:\n> > > On Tue, Aug 28, 2018 at 02:35:25PM -0700, Stefan Beller wrote:\n> >\n> > >> Yeah, then let's just convert '/' with as little overhead as possible.\n> > >\n> > > Do you care about case-folding issues (e.g., submodules \"FOO\" and \"foo\"\n> > > colliding)?\n> > >\n> > > I'm OK if the answer is \"no\", but if you do want to deal with it, the\n> > > time is probably now.\n> >\n> > Have we rejected the config approach?\n> \n> I did not reject that approach, but am rather waiting for patches. ;-)\n\nNote I did send out a patch using this approach, so no need to wait any\nlonger! :D\n\nhttps://public-inbox.org/git/20180816181940.46114-1-bmwill@google.com/\n\n-- \nBrandon Williams\n"},{"id":"356891","messageId":"CAGZ79kYnbjaPoWdda0SM_-_X77mVyYC7JO61OV8nm2yj3Q1OvQ@mail.gmail.com","threadId":"49065","inReplyTo":"20180829211802.GG7547@aiede.svl.corp.google.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-29T21:27:43Z","receivedAt":"2018-08-29T21:27:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 29, 2018 at 2:18 PM Jonathan Nieder wrote:\n>\n> Hi,\n>\n> Stefan Beller wrote:\n>\n> >> Yes, that makes even the capitalized \"CON\" issues go away. It's not a\n> >> one-to-one mapping, though (\"foo-\" and \"foo_\" map to the same entity).\n> >\n> > foo_ would map to foo__, and foo- would map to something else.\n> > (foo- as we do not rewrite dashes, yet?)\n> >\n> >> If we want that, too, I think something like url-encoding is fine, with\n> >> the caveat that we simply urlencode _more_ things (i.e., anything not in\n> >> [a-z_]).\n> >\n> > Yeah I think we need more than url encoding now.\n>\n> Can you say more?\n\nhttps://public-inbox.org/git/CAGZ79kZv4BjRq=kq_1UeT2Kn38OZwYFgnMsTe6X_WP41=hBtSQ@mail.gmail.com/\n\n> Can you spell out for me what problem we're solving with something more custom?\n\ncase sensitivity for example.\n\n> the ability to tweak things later.\n\nThat is unrelated to the choice of encoding, but more related to\nhttps://public-inbox.org/git/20180816181940.46114-1-bmwill@google.com/\n"},{"id":"356892","messageId":"20180829213012.GA32400@sigill.intra.peff.net","threadId":"49065","inReplyTo":"CAGZ79kYJTWROYSGjEbdVBsEAkWkNE4QVCiPVfuMf75d13fXN6A@mail.gmail.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-29T21:30:12Z","receivedAt":"2018-08-29T21:30:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 29, 2018 at 02:10:37PM -0700, Stefan Beller wrote:\n\n> > Yes, that makes even the capitalized \"CON\" issues go away. It's not a\n> > one-to-one mapping, though (\"foo-\" and \"foo_\" map to the same entity).\n> \n> foo_ would map to foo__, and foo- would map to something else.\n> (foo- as we do not rewrite dashes, yet?)\n\nAh, OK, I took your:\n\n> [A-Z] -> _[a-z]\n\nto mean \"A-Z becomes a-z, and everything else becomes underscore\".\n\nIf you mean a real one-to-one mapping that allows a-z and only a few\nsafe metacharacters, then yeah, that's what I was thinking, too.\n\n> > If we want that, too, I think something like url-encoding is fine, with\n> > the caveat that we simply urlencode _more_ things (i.e., anything not in\n> > [a-z_]).\n> \n> Yeah I think we need more than url encoding now.\n\nIf you take \"url encoding\" to only be the mechanical transformation of\nquoting, not the set of _what_ gets quoting, we can still stick with it.\nWe don't need to, but it's probably no worse than inventing our own\nset of quoting rules.\n\n-Peff\n"},{"id":"356893","messageId":"20180829213217.GB32400@sigill.intra.peff.net","threadId":"49065","inReplyTo":"20180829210913.GF7547@aiede.svl.corp.google.com","subject":"Re: [PATCH 2/2] submodule: munge paths to submodule git directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-29T21:32:17Z","receivedAt":"2018-08-29T21:32:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 29, 2018 at 02:09:13PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> > On Tue, Aug 28, 2018 at 02:35:25PM -0700, Stefan Beller wrote:\n> \n> >> Yeah, then let's just convert '/' with as little overhead as possible.\n> >\n> > Do you care about case-folding issues (e.g., submodules \"FOO\" and \"foo\"\n> > colliding)?\n> >\n> > I'm OK if the answer is \"no\", but if you do want to deal with it, the\n> > time is probably now.\n> \n> Have we rejected the config approach? I really liked the attribute of\n> not having to solve everything right away. I'm getting scared that\n> we've forgotten that goal.\n\nI personally have no problem with that approach, but I also haven't\nthought that hard about it (I was mostly ignoring the discussion since\nit seemed like submodule-interested folks, but I happened to see what\nlooked like a potentially bad idea cc'd to me ;) ).\n\n-Peff\n"},{"id":"366698","messageId":"20190115012507.GK162110@google.com","threadId":"49065","inReplyTo":"20180807230637.247200-1-bmwill@google.com","subject":"Re: [RFC] submodule: munge paths to submodule git directories","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-01-15T01:25:07Z","receivedAt":"2019-01-15T01:25:12Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nIn August, 2018, Brandon Williams wrote:\n\n> Commit 0383bbb901 (submodule-config: verify submodule names as paths,\n> 2018-04-30) introduced some checks to ensure that submodule names don't\n> include directory traversal components (e.g. \"../\").\n>\n> This addresses the vulnerability identified in 0383bbb901 but the root\n> cause is that we use submodule names to construct paths to the\n> submodule's git directory. What we really should do is munge the\n> submodule name before using it to construct a path.\n\nThanks again for this. I liked the proposal enough to run Git with\npatches implementing it for a while. That said, there were some\nunaddressed comments in the review.\n\nI've put a summary in https://crbug.com/git/28 to make this easier to\npick up where we left off. Summary from there of the upstream review:\n\n1. Using urlencoding to escape the slashes is fine, but what if we\n want to escape some other character (for example to handle\n case-insensitive filesystems)?\n\n Proposal: Store the escaping mapping in config[1] so it can be\n modified it in the future:\n\n\t[submodule \"plugin/hooks\"]\n\t\tgitdirname = plugins%2fhooks\n\n2. The urlencoded name could conflict with a submodule that has % in\n its name in an existing clone created by an older version of Git.\n\n Proposal: Put submodules in a new .git/submodules/ directory\n instead of .git/modules/.\n\n3. These gitdirname settings can clutter up .git/config.\n\n Proposal: For the \"easy\" cases (e.g. submodule name consisting of\n [a-z]*), allow omitting the gitdirname setting.\n\nIs that a fair summary? Are there concerns from the review that I\nforgot, or would a new version of the series that addresses those\nthree problems put us in good shape?\n\nThanks,\nJonathan\n\n[1] https://public-inbox.org/git/20180816181940.46114-1-bmwill@google.com/\n"},{"id":"367005","messageId":"20190117173216.GB27667@sigill.intra.peff.net","threadId":"49065","inReplyTo":"20190115012507.GK162110@google.com","subject":"Re: [RFC] submodule: munge paths to submodule git directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-17T17:32:17Z","receivedAt":"2019-01-17T17:32:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 14, 2019 at 05:25:07PM -0800, Jonathan Nieder wrote:\n\n> I've put a summary in https://crbug.com/git/28 to make this easier to\n> pick up where we left off. Summary from there of the upstream review:\n> \n> 1. Using urlencoding to escape the slashes is fine, but what if we\n> want to escape some other character (for example to handle\n> case-insensitive filesystems)?\n> \n> Proposal: Store the escaping mapping in config[1] so it can be\n> modified it in the future:\n> \n> \t[submodule \"plugin/hooks\"]\n> \t\tgitdirname = plugins%2fhooks\n\nI think it might be worth dealing with case-sensitivity _now_, since we\nknow it's a problem. That doesn't make the problem of \"what if we want\nto change the mapping later\" go away, but it does make it a lot less\nlikely to come up.\n\n> 2. The urlencoded name could conflict with a submodule that has % in\n> its name in an existing clone created by an older version of Git.\n> \n> Proposal: Put submodules in a new .git/submodules/ directory\n> instead of .git/modules/.\n\nThis proposal is orthogonal to (1), right? I.e., if we store the mapping\nthen that is what tells us we're using the mapped name.\n\n> 3. These gitdirname settings can clutter up .git/config.\n> \n> Proposal: For the \"easy\" cases (e.g. submodule name consisting of\n> [a-z]*), allow omitting the gitdirname setting.\n\nNot having thought about it too hard, I suspect that may open back up\ncorner cases with respect to backwards compatibility and ambiguity.\n\nAre you worried about human-readable clutter? I.e., that .git/config\nbecomes hard to read? If so, then:\n\n - I doubt this is any worse than the existing tracking-branch config.\n\n - it might be reasonable to store it in .git/submodule-config, and\n make sure that .git/config contains a single \"[include]path =\n submodule-config\" line. I've been tempted to do that for\n tracking-branch config.\n\nOr are you worried about the cost of parsing those entries? Basically\nevery git command parses config linearly at least once; this normally\nisn't noticeable, but at some size it becomes a problem. I have no idea\nwhat that size is.\n\nIf so, then I think we'd want submodule config in its own file but\n_without_ an include from the normal config file. That would break\ncompatibility with anything that tries to use \"git config\nsubmodule.foo.path\", etc.\n\nThat's all just musing. I'm actually not really convinced it's a\nproblem.\n\n> Is that a fair summary? Are there concerns from the review that I\n> forgot, or would a new version of the series that addresses those\n> three problems put us in good shape?\n\nI don't really have a strong opinion either way. I still think the\none-way transformation that the patch uses is less elegant than a real\nencode/decode round-trip (i.e., what I discussed in [1]). But I admit to\nnot having thought through all of the details of the encode/decode\nthing, and certainly have not written the code.\n\n[1] http://public-inbox.org/git/20180809212602.GA11342@sigill.intra.peff.net/\n"},{"id":"367006","messageId":"CAGZ79kbofESBSTHbDdPmeZgb2Pwz=8FVmtXG6x376utMyS0vqA@mail.gmail.com","threadId":"49065","inReplyTo":"20190117173216.GB27667@sigill.intra.peff.net","subject":"Re: [RFC] submodule: munge paths to submodule git directories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-17T17:57:07Z","receivedAt":"2019-01-17T17:57:22Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 17, 2019 at 9:32 AM Jeff King wrote:\n>\n> On Mon, Jan 14, 2019 at 05:25:07PM -0800, Jonathan Nieder wrote:\n>\n> > I've put a summary in https://crbug.com/git/28 to make this easier to\n> > pick up where we left off. Summary from there of the upstream review:\n> >\n> > 1. Using urlencoding to escape the slashes is fine, but what if we\n> > want to escape some other character (for example to handle\n> > case-insensitive filesystems)?\n> >\n> > Proposal: Store the escaping mapping in config[1] so it can be\n> > modified it in the future:\n> >\n> > [submodule \"plugin/hooks\"]\n> > gitdirname = plugins%2fhooks\n>\n> I think it might be worth dealing with case-sensitivity _now_, since we\n> know it's a problem. That doesn't make the problem of \"what if we want\n> to change the mapping later\" go away, but it does make it a lot less\n> likely to come up.\n\nMakes sense.\n\n>\n> > 2. The urlencoded name could conflict with a submodule that has % in\n> > its name in an existing clone created by an older version of Git.\n> >\n> > Proposal: Put submodules in a new .git/submodules/ directory\n> > instead of .git/modules/.\n>\n> This proposal is orthogonal to (1), right? I.e., if we store the mapping\n> then that is what tells us we're using the mapped name.\n\nTechnically true, but it allows for easier implementation:\nnow we have 2 distinct namespaces, such that we can avoid\ndouble booking easier:\n\n Consider 2 submodules \"a b\" and \"a%20b\".\n\nWithout (2), (1) is hard to explain as the first might have been encoded\nto a%20b or there might have been the second put in place from a\ncurrent (old) version of Git. So we'd have to reason about these corner cases.\n\nWith (2) in place, we'd only ever have the second in a place \"a%2520b\"\n(if I am to trust https://www.urlencoder.org/)\n\n> > 3. These gitdirname settings can clutter up .git/config.\n> >\n> > Proposal: For the \"easy\" cases (e.g. submodule name consisting of\n> > [a-z]*), allow omitting the gitdirname setting.\n>\n> Not having thought about it too hard, I suspect that may open back up\n> corner cases with respect to backwards compatibility and ambiguity.\n>\n> Are you worried about human-readable clutter? I.e., that .git/config\n> becomes hard to read? If so, then:\n>\n> - I doubt this is any worse than the existing tracking-branch config.\n>\n> - it might be reasonable to store it in .git/submodule-config, and\n> make sure that .git/config contains a single \"[include]path =\n> submodule-config\" line. I've been tempted to do that for\n> tracking-branch config.\n>\n> Or are you worried about the cost of parsing those entries? Basically\n> every git command parses config linearly at least once; this normally\n> isn't noticeable, but at some size it becomes a problem. I have no idea\n> what that size is.\n>\n> If so, then I think we'd want submodule config in its own file but\n> _without_ an include from the normal config file. That would break\n> compatibility with anything that tries to use \"git config\n> submodule.foo.path\", etc.\n>\n> That's all just musing. I'm actually not really convinced it's a\n> problem.\n\nok, we can deal with the problem once it arises.\n\n>\n> > Is that a fair summary? Are there concerns from the review that I\n> > forgot, or would a new version of the series that addresses those\n> > three problems put us in good shape?\n>\n> I don't really have a strong opinion either way. I still think the\n> one-way transformation that the patch uses is less elegant than a real\n> encode/decode round-trip (i.e., what I discussed in [1]). But I admit to\n> not having thought through all of the details of the encode/decode\n> thing, and certainly have not written the code.\n\nThe suggestion of adding at least a test for url encoding (2. from your mail)\nis sensible.\n\nStefan\n\n>\n> [1] http://public-inbox.org/git/20180809212602.GA11342@sigill.intra.peff.net/\n"}]}