submodule: munge paths to submodule git directories

Commit 0383bbb901 (submodule-config: verify submodule names as paths, 2018-04-30) introduced some checks to ensure that submodule names don't include directory traversal components (e.g. "../").

This addresses the vulnerability identified in 0383bbb901 but the root cause is that we use submodule names to construct paths to the submodule's git directory. What we really should do is munge the submodule name before using it to construct a path.

Introduce a function "strbuf_submodule_gitdir()" which callers can use to build a path to a submodule's gitdir. This allows for a single location where we can munge the submodule name (by url encoding it) before using it as part of a path.

Signed-off-by Brandon Williams <[email protected]>

---
Using submodule names as is continues to be not such a good idea. Maybe we could apply something like this to stop using them as is. url encoding seems like the easiest approach, but I've also heard suggestions that would could use the SHA1 of the submodule name.

Any thoughts?

submodule.c +0 -0

submodule.c +1 -1

@@ -1933,9 +1938,29 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
 			goto cleanup;
 		}
 		strbuf_reset(buf);
-		strbuf_git_path(buf, "%s/%s", "modules", sub->name);
+		strbuf_submodule_gitdir(buf, the_repository, sub->name);

submodule.c +6 -0

+	if (!access(buf->buf, F_OK))
+		return;
+
+	strbuf_setlen(buf, modules_len);
+	strbuf_addstr_urlencode(buf, submodule_name, 1);
+}

Introduce a function "strbuf_submodule_gitdir()" which callers can use to build a path to a submodule's gitdir. This allows for a single location where we can munge the submodule name (by url encoding it) before using it as part of a path.

Using submodule names as is continues to be not such a good idea. Maybe we could apply something like this to stop using them as is. url encoding seems like the easiest approach, but I've also heard suggestions that would could use the SHA1 of the submodule name.

Introduce a helper function "submodule_name_to_gitdir()" (and the submodule--helper subcommand "gitdir") which constructs a path to a submodule's gitdir, located in the provided repository's "modules" directory.

This consolidates the logic needed to build up a path into a repository's "modules" directory, abstracting away the fact that submodule git directories are stored in a repository's common gitdir. This makes it easier to adjust how submodules gitdir are stored in the "modules" directory in a future patch.

Quoted 5 lines · Brandon Williams

Introduce a helper function "submodule_name_to_gitdir()" (and the submodule--helper subcommand "gitdir") which constructs a path to a submodule's gitdir, located in the provided repository's "modules" directory.

Makes sense.

Quoted 6 lines

This consolidates the logic needed to build up a path into a repository's "modules" directory, abstracting away the fact that submodule git directories are stored in a repository's common gitdir. This makes it easier to adjust how submodules gitdir are stored in the "modules" directory in a future patch.

and yet, all places that we touch were and still are broken for old-style submodules that have their git directory inside the working tree? Do we need to pay attention to those, too?

git-submodule.sh cmd_update() · line 581

Quoted 13 lines

git-submodule.sh +1 -1

index 8b5ad59bde..053747d290 100755
@@ -577,7 +578,7 @@ cmd_update()
                        die "$(eval_gettext "Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
                fi
 
-               if ! $(git config -f "$(git rev-parse --git-common-dir)/modules/$name/config" core.worktree) 2>/dev/null
+               if ! $(git config -f "$(git submodule--helper gitdir "$name")/config" core.worktree) 2>/dev/null

This will collide with origin/sb/submodule-update-in-c specifically 1c866b9831d (submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-03), but as that removes these lines, it should be easy to resolve the conflict.

Introduce a helper function "submodule_name_to_gitdir()" (and the submodule--helper subcommand "gitdir") which constructs a path to a submodule's gitdir, located in the provided repository's "modules" directory.

This consolidates the logic needed to build up a path into a repository's "modules" directory, abstracting away the fact that submodule git directories are stored in a repository's common gitdir. This makes it easier to adjust how submodules gitdir are stored in the "modules" directory in a future patch.

Signed-off-by Brandon Williams <[email protected]>

---
...

Quoted 15 lines · Brandon Williams

Introduce a helper function "submodule_name_to_gitdir()" (and the submodule--helper subcommand "gitdir") which constructs a path to a submodule's gitdir, located in the provided repository's "modules" directory.

This consolidates the logic needed to build up a path into a repository's "modules" directory, abstracting away the fact that submodule git directories are stored in a repository's common gitdir. This makes it easier to adjust how submodules gitdir are stored in the "modules" directory in a future patch.

Signed-off-by Brandon Williams <[email protected]>

---
...

builtin/submodule--helper.c static int connect_gitdir_workingtree(int argc, const char **argv, const char *p · line 2045

Quoted 8 lines

builtin/submodule--helper.c +1 -1

@@ -2018,7 +2039,7 @@ static int connect_gitdir_workingtree(int argc, const char **argv, const char *p
 	name = argv[1];
 	path = argv[2];
 
-	strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
+	submodule_name_to_gitdir(&sb, the_repository, name);
 	sm_gitdir = absolute_pathdup(sb.buf);
 
 	connect_work_tree_and_git_dir(path, sm_gitdir, 0);

This function goes away with 1c866b98 ("submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree", 2018-08-03) in sb/submodule-update-in-c topic. git-submodule.sh has simlar conflicts.

I guess its replacement function does not care as deeply as its predecessor used to about where the submodule's $GIT_DIR is, so the correct resolution may be just to ignore the change made to this caller to the new name-to-gitdir function.

It would have been nicer to see a bit better inter-developer coordination, especially between two who sit practically next to each other ;-)

Thanks.

Commit 0383bbb901 (submodule-config: verify submodule names as paths, 2018-04-30) introduced some checks to ensure that submodule names don't include directory traversal components (e.g. "../").

This addresses the vulnerability identified in 0383bbb901 but the root cause is that we use submodule names to construct paths to the submodule's git directory. What we really should do is munge the submodule name before using it to construct a path.

Teach "submodule_name_to_gitdir()" to munge a submodule's name (by url encoding it) before using it to build a path to the submodule's gitdir.

Quoted 11 lines · Brandon Williams

Commit 0383bbb901 (submodule-config: verify submodule names as paths, 2018-04-30) introduced some checks to ensure that submodule names don't include directory traversal components (e.g. "../").

This addresses the vulnerability identified in 0383bbb901 but the root cause is that we use submodule names to construct paths to the submodule's git directory. What we really should do is munge the submodule name before using it to construct a path.

Teach "submodule_name_to_gitdir()" to munge a submodule's name (by url encoding it) before using it to build a path to the submodule's gitdir.

I like this approach very much, and I think using url encoding is much better than an opaque hash (purely because it makes debugging and inspection saner).

Two thoughts, though:

submodule.c int submodule_to_gitdir(struct strbuf *buf, const char *submodule) · line 1980

Quoted 10 lines

submodule.c +9 -0

+	modules_len = buf->len;
 	strbuf_addstr(buf, submodule_name);
+
+	/*
+	 * If the submodule gitdir already exists using the old-fashioned
+	 * location (which uses the submodule name as-is, without munging it)
+	 * then return that.
+	 */
+	if (!access(buf->buf, F_OK))
+		return;

I think this backwards-compatibility is necessary to avoid pain. But until it goes away, I don't think this is helping the vulnerability from 0383bbb901. Because there the issue was that the submodule name pointed back into the working tree, so this access() would find the untrusted working tree code and say "ah, an old-fashioned name!".

In theory a fresh clone could set a config option for "I only speak use new-style modules". And there could even be a conversion program that moves the modules as appropriate, fixes up the .git files in the working tree, and then sets that config.

In fact, I think that config option _could_ be done by bumping core.repositoryformatversion and then setting extensions.submodulenames to "url" or something. Then you could never run into the confusing case where you have a clone done by a new version of git (using new-style names), but using an old-style version gets confused because it can't find the module directories (instead, it would barf and say "I don't know about that extension").

I don't know if any of that is worth it, though. We already fixed the problem from 0383bbb901. There may be a _different_ "break out of the modules directory" vulnerability, but since we disallow ".." it's hard to see what it would be (the best I could come up with is maybe pointing one module into the interior of another module, but I think you'd have to trouble overwriting anything useful).

And while an old-style version of Git being confused might be annoying, I suspect that bumping the repository version would be even _more_ annoying, because it would hit every command, not just ones that try to touch those submodules.

t/t7400-submodule-basic.sh test_expect_success 'recursive relative submodules stay relative' ' · line 936

Quoted 11 lines

t/t7400-submodule-basic.sh +1 -1

index 2c2c97e144..963693332c 100755
@@ -933,7 +933,7 @@ test_expect_success 'recursive relative submodules stay relative' '
 		cd clone2 &&
 		git submodule update --init --recursive &&
 		echo "gitdir: ../.git/modules/sub3" >./sub3/.git_expect &&
-		echo "gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub" >./sub3/dirdir/subsub/.git_expect
+		echo "gitdir: ../../../.git/modules/sub3/modules/dirdir%2fsubsub" >./sub3/dirdir/subsub/.git_expect

One interesting thing about url-encoding is that it's not one-to-one. This case could also be %2F, which is a different file (on a case-sensitive filesystem). I think "%20" and "+" are similarly interchangeable.

If we were decoding the filenames, that's fine. The round-trip is lossless.

But that's not quite how the new code behaves. We encode the input and then check to see if it matches an encoding we previously performed. So if our urlencode routines ever change, this will subtly break.

I don't know how much it's worth caring about. We're not that likely to change the routines ourself (though certainly a third-party implementation would need to know our exact url-encoding decisions).

Some possible actions:

0. Do nothing, and cross our fingers. ;)

1. Don't use strbuf_addstr_urlencode(), but rather our own munging function which we know will remain stable (or alternatively, a flag to strbuf_addstr_urlencode to get the consistent behavior).

2. Make sure we have tests which cover this, so at least somebody changing the urlencode decisions will see a breakage. Your test here covers the upper/lowercase one, but we might want one that covers "+". (There may be more ambiguous cases, but those are the ones I know about).

3. Rather than check for the existence of names, decode what's actually in the modules/ directory to create an in-memory index of names.

I hesitate to suggest that, because it's obviously way more complicated, and may perform worse if you have a lot of modules (since you have to readdir() and decode the whole directory just to look up one module).

But I think it also gives a more elegant solution to the backwards-compatibility problem, since we could recognize both new and old-style names. There's some ambiguity (e.g., is "foo%2fbar" "foo/bar", or did somebody really have a name with a percent in it?),. but in theory you could respect either name (giving preference to new-style in case of a conflict).

And I think the result would be immune to any directory-escape vulnerabilities, because we'd always start with what actually exists in $GIT_DIR/modules/, which we know _we_ will have written.

Again, I'm not sure if it's worth the effort, but I thought I'd throw it out there.

-Peff

Quoted 8 lines · Jeff King

One interesting thing about url-encoding is that it's not one-to-one. This case could also be %2F, which is a different file (on a case-sensitive filesystem). I think "%20" and "+" are similarly interchangeable.

If we were decoding the filenames, that's fine. The round-trip is lossless.

Quoted 7 lines

But that's not quite how the new code behaves. We encode the input and then check to see if it matches an encoding we previously performed. So if our urlencode routines ever change, this will subtly break.

I don't know how much it's worth caring about. We're not that likely to change the routines ourself (though certainly a third-party implementation would need to know our exact url-encoding decisions).

This is exactly the reason why I wanted to get some opinions on what the best thing to do here would be. I _think_ the best thing would probably be to write a specific routine to do the conversion, and it wouldn't even have to be all that complex. Basically I'm just interested in converting '/' characters so that things no longer behave like nested directories.

Hi,

Quoted 18 lines · Brandon Williams

Quoted 8 lines · Jeff King

One interesting thing about url-encoding is that it's not one-to-one. This case could also be %2F, which is a different file (on a case-sensitive filesystem). I think "%20" and "+" are similarly interchangeable.

If we were decoding the filenames, that's fine. The round-trip is lossless.

Quoted 7 lines

But that's not quite how the new code behaves. We encode the input and then check to see if it matches an encoding we previously performed. So if our urlencode routines ever change, this will subtly break.

I don't know how much it's worth caring about. We're not that likely to change the routines ourself (though certainly a third-party implementation would need to know our exact url-encoding decisions).

Quoted 6 lines

This is exactly the reason why I wanted to get some opinions on what the best thing to do here would be. I _think_ the best thing would probably be to write a specific routine to do the conversion, and it wouldn't even have to be all that complex. Basically I'm just interested in converting '/' characters so that things no longer behave like nested directories.

First of all, I think the behavior with this patch is already much better than the previous status quo. I'm using the patch now and am very happy with it.

Second, what if we store the pathname in config? We already store the URL there:

[submodule "plugins/hooks"]
url = https://gerrit.googlesource.com/plugins/hooks

So we could (as a followup patch) do something like

[submodule "plugins/hooks"]
url = https://gerrit.googlesource.com/plugins/hooks
gitdirname = plugins%2fhooks

and use that for lookups instead of regenerating the directory name. What do you think?

Quoted 6 lines · Jonathan Nieder

Second, what if we store the pathname in config? We already store the URL there:

[submodule "plugins/hooks"]
url = https://gerrit.googlesource.com/plugins/hooks

Quoted 8 lines

So we could (as a followup patch) do something like

[submodule "plugins/hooks"]
url = https://gerrit.googlesource.com/plugins/hooks
gitdirname = plugins%2fhooks

and use that for lookups instead of regenerating the directory name. What do you think?

As I just looked at worktree code, this sounds intriguing for the wrong reason (again), as a user may want to point the gitdirname to a repository that they have already on disk outside the actual superproject. They would be reinventing worktrees in the submodule space. ;-)

This would open up the security hole that we just had, again. So we'd have to make sure that the gitdirname (instead of the now meaningless subsection name) is proof to ../ attacks.

I feel uneasy about this as then the user might come in and move submodules and repoint the gitdirname... to a not url encoded path. Exposing this knob just asks for trouble, no?

What would happen if gitdirname is changed as part of history? (The same problem we have now with changing the subsection name)

Hi,

Quoted 26 lines · Stefan Beller

Quoted 6 lines · Jonathan Nieder

Second, what if we store the pathname in config? We already store the URL there:

[submodule "plugins/hooks"]
url = https://gerrit.googlesource.com/plugins/hooks

Quoted 8 lines

So we could (as a followup patch) do something like

[submodule "plugins/hooks"]
url = https://gerrit.googlesource.com/plugins/hooks
gitdirname = plugins%2fhooks

and use that for lookups instead of regenerating the directory name. What do you think?

As I just looked at worktree code, this sounds intriguing for the wrong reason (again), as a user may want to point the gitdirname to a repository that they have already on disk outside the actual superproject. They would be reinventing worktrees in the submodule space. ;-)

This would open up the security hole that we just had, again. So we'd have to make sure that the gitdirname (instead of the now meaningless subsection name) is proof to ../ attacks.

Quoted 4 lines

I feel uneasy about this as then the user might come in and move submodules and repoint the gitdirname... to a not url encoded path. Exposing this knob just asks for trouble, no?

What if we forbid directory separator characters in the gitdirname?

[...]

Quoted 3 lines

What would happen if gitdirname is changed as part of history? (The same problem we have now with changing the subsection name)

In this proposal, it would only be read from config, not from .gitmodules.

Quoted 1 line · Jonathan Nieder

What if we forbid directory separator characters in the gitdirname?

Fine with me, but ideally we'd want to allow sharding the submodules. When you have 1000 submodules
we'd want them not all inside the toplevel "modules/" ?

Quoted 2 lines

In this proposal, it would only be read from config, not from .gitmodules.

Ah good point. That makes sense.

Stepping back a bit regarding the config:

Now that we have the submodule.active or even submodule.<name>.active flags, we do not need (b) any more. So the URL turns into a useless piece of cruft that just is unneeded and might confuse the user.

So maybe I'd want to propose a patch that removes submodule.<name>.url from the config once it is cloned. (I just read up on "submodule sync" again, but that might not even need special care for this new world)

And with all that said, I think if we can avoid having the submodules gitdir in the config, the config would look much cleaner, too.

[...]

All at the cost of recording a little configuration somewhere. If we want to decrease the configuration, we can avoid recording it there in the easy cases (e.g. when name == gitdirname). That's "just" an optimization.

And then we have the ability later to handle all the edge cases we haven't handled yet today:

- sharding when the number of submodules is too large
- case-insensitive filesystems
- path name length limits
- different sets of filesystem-special characters

Sane?

Quoted 4 lines · Jonathan Nieder

All at the cost of recording a little configuration somewhere. If we want to decrease the configuration, we can avoid recording it there in the easy cases (e.g. when name == gitdirname). That's "just" an optimization.

Sounds good, but gerrit for example would not take advantage of such optimisation as they have slashes in their submodules. :-( I wonder if we can optimize further and keep slashes if there is no conflict (as then name == gitdirname, so it can be optimized).

Quoted 9 lines

And then we have the ability later to handle all the edge cases we haven't handled yet today:

- sharding when the number of submodules is too large
- case-insensitive filesystems
- path name length limits
- different sets of filesystem-special characters

Sane?

I'll keep thinking about it.

FYI: the reduction in configuration was just sent out.

Quoted 11 lines · Stefan Beller

Quoted 4 lines · Jonathan Nieder

All at the cost of recording a little configuration somewhere. If we want to decrease the configuration, we can avoid recording it there in the easy cases (e.g. when name == gitdirname). That's "just" an optimization.

Sounds good, but gerrit for example would not take advantage of such optimisation as they have slashes in their submodules. :-( I wonder if we can optimize further and keep slashes if there is no conflict (as then name == gitdirname, so it can be optimized).

One possibility would be to treat gsub("/", "%2f") as another of the easy cases.

[...]

Quoted 9 lines

Quoted 9 lines

And then we have the ability later to handle all the edge cases we haven't handled yet today:

- sharding when the number of submodules is too large
- case-insensitive filesystems
- path name length limits
- different sets of filesystem-special characters

Sane?

Quoted 2 lines

I'll keep thinking about it.

Thanks.

Quoted 1 line

FYI: the reduction in configuration was just sent out.

https://public-inbox.org/git/[email protected]/
for those following along.

Ciao,
Jonathan

Introduce the config "submodule.<name>.gitdirpath" which is used to indicate where a submodule's gitdir is located inside of a repository's "modules" directory.

Signed-off-by Brandon Williams <[email protected]>

---

Maybe something like this on top? Do you think we should disallow "../" in this config, even though it is a repository local configuration and not shipped in .gitmodules?

So it would be nice, for future-proofing, if we can change the naming scheme later.
...
All at the cost of recording a little configuration somewhere. If we want to decrease the configuration, we can avoid recording it there in the easy cases (e.g. when name == gitdirname). That's "just" an optimization.

And then we have the ability later to handle all the edge cases we haven't handled yet today:

- sharding when the number of submodules is too large
- case-insensitive filesystems
- path name length limits
- different sets of filesystem-special characters

Sane?

Quoted 5 lines

I think this backwards-compatibility is necessary to avoid pain. But until it goes away, I don't think this is helping the vulnerability from 0383bbb901. Because there the issue was that the submodule name pointed back into the working tree, so this access() would find the untrusted working tree code and say "ah, an old-fashioned name!".

[...]

Oh I know that this doesn't help with that vulnerability. As you've said we fix it and now disallow ".." at the submodule-config level so really this path is simply about using what we get out of submodule-config in a more sane manor.

Quoted 8 lines

One interesting thing about url-encoding is that it's not one-to-one. This case could also be %2F, which is a different file (on a case-sensitive filesystem). I think "%20" and "+" are similarly interchangeable.

If we were decoding the filenames, that's fine. The round-trip is lossless.

Quoted 7 lines

But that's not quite how the new code behaves. We encode the input and then check to see if it matches an encoding we previously performed. So if our urlencode routines ever change, this will subtly break.

I don't know how much it's worth caring about. We're not that likely to change the routines ourself (though certainly a third-party implementation would need to know our exact url-encoding decisions).

This is exactly the reason why I wanted to get some opinions on what the best thing to do here would be. I _think_ the best thing would probably be to write a specific routine to do the conversion, and it wouldn't even have to be all that complex. Basically I'm just interested in converting '/' characters so that things no longer behave like nested directories.

Quoted 8 lines

t/t7400-submodule-basic.sh test_expect_success 'recursive relative submodules stay relative' ' · line 936

Quoted 2 lines

t/t7400-submodule-basic.sh +1 -1

-           echo "gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub" >./sub3/dirdir/subsub/.git_expect
+           echo "gitdir: ../../../.git/modules/sub3/modules/dirdir%2fsubsub" >./sub3/dirdir/subsub/.git_expect

One interesting thing about url-encoding is that it's not one-to-one. This case could also be %2F, which is a different file (on a case-sensitive filesystem). I think "%20" and "+" are similarly interchangeable.

Quoted 6 lines

If we were decoding the filenames, that's fine. The round-trip is lossless.

But that's not quite how the new code behaves. We encode the input and then check to see if it matches an encoding we previously performed. So if our urlencode routines ever change, this will subtly break.

This is exactly the reason why I wanted to get some opinions on what the best thing to do here would be. I _think_ the best thing would probably be to write a specific routine to do the conversion, and it wouldn't even have to be all that complex. Basically I'm just interested in converting '/' characters so that things no longer behave like nested directories.

3) (optional) instead of putting it all in modules/, use another directory gitmodules/ for example. this will make sure we can tell if a repository has been converted or is stuck with a setup of a current git.

Quoted 6 lines

This is exactly the reason why I wanted to get some opinions on what the best thing to do here would be. I _think_ the best thing would probably be to write a specific routine to do the conversion, and it wouldn't even have to be all that complex. Basically I'm just interested in converting '/' characters so that things no longer behave like nested directories.

Yeah, then let's just convert '/' with as little overhead as possible.

Quoted 4 lines · Stefan Beller

3) (optional) instead of putting it all in modules/, use another directory gitmodules/ for example. this will make sure we can tell if a repository has been converted or is stuck with a setup of a current git.

I actually kind of like that idea, as it makes the interaction between old and new names much simpler to reason about.

And since old code won't know about the new names anyway, there's in theory no downside. In practice, of course, the encoding may often be a noop, and lazy scripts would continue to work most of the time if you didn't change out the prefix directory. I'm not sure if that is an argument for the scheme (because it will suss out broken scripts more consistently) or against it (because 99% of the time those old scripts would just happen to work).

Quoted 7 lines

Quoted 6 lines

This is exactly the reason why I wanted to get some opinions on what the best thing to do here would be. I _think_ the best thing would probably be to write a specific routine to do the conversion, and it wouldn't even have to be all that complex. Basically I'm just interested in converting '/' characters so that things no longer behave like nested directories.

Quoted 1 line

Yeah, then let's just convert '/' with as little overhead as possible.

Do you care about case-folding issues (e.g., submodules "FOO" and "foo" colliding)?

I'm OK if the answer is "no", but if you do want to deal with it, the time is probably now.

Quoted 2 lines

Do you care about case-folding issues (e.g., submodules "FOO" and "foo" colliding)?

I do. :(

2d84f13dcb6 (config: fix case sensitive subsection names on writing, 2018-08-08) explains the latest episode of case folding with submodules involved.

Quoted 2 lines

I'm OK if the answer is "no", but if you do want to deal with it, the time is probably now.

Good point. But as soon as we start discussing case sensitivity, we are drawn down the rabbit hole of funny file names. (Try naming a submodule "CON1" and obtain it on Windows for example) So we would need to have a file system specific encoding function for submodule names, which sounds like a maintenance night mare.

So if I was thinking in the scheme presented above, we could just have another rule that is

[A-Z] -> _[a-z]

(lowercase capital letters and escape them with an underscore)

Yes, that makes even the capitalized "CON" issues go away. It's not a one-to-one mapping, though ("foo-" and "foo_" map to the same entity).

If we want that, too, I think something like url-encoding is fine, with the caveat that we simply urlencode _more_ things (i.e., anything not in [a-z_]).

Quoted 2 lines

Yes, that makes even the capitalized "CON" issues go away. It's not a one-to-one mapping, though ("foo-" and "foo_" map to the same entity).

foo_ would map to foo__, and foo- would map to something else. (foo- as we do not rewrite dashes, yet?)

Quoted 3 lines

If we want that, too, I think something like url-encoding is fine, with the caveat that we simply urlencode _more_ things (i.e., anything not in [a-z_]).

Yeah I think we need more than url encoding now.

Hi,

Quoted 6 lines · Stefan Beller

Quoted 2 lines

Yes, that makes even the capitalized "CON" issues go away. It's not a one-to-one mapping, though ("foo-" and "foo_" map to the same entity).

foo_ would map to foo__, and foo- would map to something else. (foo- as we do not rewrite dashes, yet?)

Quoted 5 lines

Quoted 3 lines

If we want that, too, I think something like url-encoding is fine, with the caveat that we simply urlencode _more_ things (i.e., anything not in [a-z_]).

Yeah I think we need more than url encoding now.

Can you say more?

Can you spell out for me what problem we're solving with something more custom?

the ability to tweak things later.

Quoted 2 lines

Yes, that makes even the capitalized "CON" issues go away. It's not a one-to-one mapping, though ("foo-" and "foo_" map to the same entity).

foo_ would map to foo__, and foo- would map to something else. (foo- as we do not rewrite dashes, yet?)

[A-Z] -> _[a-z]

Quoted 3 lines

If we want that, too, I think something like url-encoding is fine, with the caveat that we simply urlencode _more_ things (i.e., anything not in [a-z_]).

Yeah I think we need more than url encoding now.

Quoted 1 line · Stefan Beller

Yeah, then let's just convert '/' with as little overhead as possible.

Do you care about case-folding issues (e.g., submodules "FOO" and "foo" colliding)?

I'm OK if the answer is "no", but if you do want to deal with it, the time is probably now.

Quoted 4 lines · Jeff King

Quoted 1 line · Stefan Beller

Yeah, then let's just convert '/' with as little overhead as possible.

Quoted 5 lines

Do you care about case-folding issues (e.g., submodules "FOO" and "foo" colliding)?

I'm OK if the answer is "no", but if you do want to deal with it, the time is probably now.

Have we rejected the config approach?

I really liked the attribute of
not having to solve everything right away. I'm getting scared that we've forgotten that goal.

It mixes well with Stefan's idea of setting up a new .git/submodules/ directory. We could require that everything in .git/submodules/ have configuration (or that everything in that directory either have configuration or be the result of a "very simple" transformation) and that way, all ambiguity goes away.

Part of the definition of "very simple" could be that the submodule name must consist of some whitelisted list of characters (including no uppercase), for example.

Quoted 9 lines · Jonathan Nieder

Quoted 7 lines · Jeff King

Quoted 1 line · Stefan Beller

Yeah, then let's just convert '/' with as little overhead as possible.

Do you care about case-folding issues (e.g., submodules "FOO" and "foo" colliding)?

Quoted 4 lines

Quoted 2 lines

I'm OK if the answer is "no", but if you do want to deal with it, the time is probably now.

Have we rejected the config approach?

I did not reject that approach, but am rather waiting for patches. ;-)

Quoted 7 lines · Jeff King

Quoted 1 line · Stefan Beller

Yeah, then let's just convert '/' with as little overhead as possible.

Do you care about case-folding issues (e.g., submodules "FOO" and "foo" colliding)?

Quoted 2 lines

I'm OK if the answer is "no", but if you do want to deal with it, the time is probably now.

Have we rejected the config approach? I really liked the attribute of not having to solve everything right away. I'm getting scared that we've forgotten that goal.

Teach "submodule_name_to_gitdir()" to munge a submodule's name (by url encoding it) before using it to build a path to the submodule's gitdir.

Commit 0383bbb901 (submodule-config: verify submodule names as paths, 2018-04-30) introduced some checks to ensure that submodule names don't include directory traversal components (e.g. "../").

This addresses the vulnerability identified in 0383bbb901 but the root cause is that we use submodule names to construct paths to the submodule's git directory. What we really should do is munge the submodule name before using it to construct a path.

I've put a summary in https://crbug.com/git/28 to make this easier to pick up where we left off. Summary from there of the upstream review:

1. Using urlencoding to escape the slashes is fine, but what if we want to escape some other character (for example to handle case-insensitive filesystems)?

Proposal: Store the escaping mapping in config[1] so it can be modified it in the future:

[submodule "plugin/hooks"]
gitdirname = plugins%2fhooks

2. The urlencoded name could conflict with a submodule that has % in its name in an existing clone created by an older version of Git.

Proposal: Put submodules in a new .git/submodules/ directory instead of .git/modules/.

3. These gitdirname settings can clutter up .git/config.

Proposal: For the "easy" cases (e.g. submodule name consisting of [a-z]*), allow omitting the gitdirname setting.

Is that a fair summary? Are there concerns from the review that I forgot, or would a new version of the series that addresses those three problems put us in good shape?

Quoted 10 lines · Jonathan Nieder

I've put a summary in https://crbug.com/git/28 to make this easier to pick up where we left off. Summary from there of the upstream review:

1. Using urlencoding to escape the slashes is fine, but what if we want to escape some other character (for example to handle case-insensitive filesystems)?

Proposal: Store the escaping mapping in config[1] so it can be modified it in the future:

Quoted 2 lines

[submodule "plugin/hooks"]
gitdirname = plugins%2fhooks

I think it might be worth dealing with case-sensitivity _now_, since we know it's a problem. That doesn't make the problem of "what if we want to change the mapping later" go away, but it does make it a lot less likely to come up.

Quoted 6 lines

2. The urlencoded name could conflict with a submodule that has % in its name in an existing clone created by an older version of Git.

Proposal: Put submodules in a new .git/submodules/ directory instead of .git/modules/.

This proposal is orthogonal to (1), right? I.e., if we store the mapping then that is what tells us we're using the mapped name.

Quoted 4 lines

3. These gitdirname settings can clutter up .git/config.

Proposal: For the "easy" cases (e.g. submodule name consisting of [a-z]*), allow omitting the gitdirname setting.

Not having thought about it too hard, I suspect that may open back up corner cases with respect to backwards compatibility and ambiguity.

Are you worried about human-readable clutter? I.e., that .git/config becomes hard to read? If so, then:

- I doubt this is any worse than the existing tracking-branch config.

- it might be reasonable to store it in .git/submodule-config, and make sure that .git/config contains a single "[include]path = submodule-config" line. I've been tempted to do that for tracking-branch config.

Or are you worried about the cost of parsing those entries? Basically every git command parses config linearly at least once; this normally isn't noticeable, but at some size it becomes a problem. I have no idea what that size is.

If so, then I think we'd want submodule config in its own file but _without_ an include from the normal config file. That would break compatibility with anything that tries to use "git config submodule.foo.path", etc.

That's all just musing. I'm actually not really convinced it's a problem.

Quoted 4 lines

Is that a fair summary? Are there concerns from the review that I forgot, or would a new version of the series that addresses those three problems put us in good shape?

I don't really have a strong opinion either way. I still think the one-way transformation that the patch uses is less elegant than a real encode/decode round-trip (i.e., what I discussed in [1]). But I admit to not having thought through all of the details of the encode/decode thing, and certainly have not written the code.

[1] http://public-inbox.org/git/[email protected]/