Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

Hi Peff,

Cc:ing Jacob, the author of the CONFIG_DATA_ENVIRONMENT sanitizing code.

On Thu, 28 Apr 2016, Jeff King wrote:

On Thu, Apr 28, 2016 at 12:03:47PM +0200, Johannes Schindelin wrote:

quoted

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 3bd6883..b338f93 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c

@@ -127,7 +127,9 @@ static int module_name(int argc, const char **argv, const char *prefix)
  */
 static int submodule_config_ok(const char *var)
 {
-	if (starts_with(var, "credential."))
+	if (starts_with(var, "credential.") ||
+			(starts_with(var, "http.") &&
+			 ends_with(var, ".extraheader")))
 		return 1;
 	return 0;
 }

Should we consider just white-listing all of "http.*"?

That would help other cases which have come up, like:

  http://thread.gmane.org/gmane.comp.version-control.git/264840

which wants to turn off http.sslverify. That would mean it turns off for
every submodule, too, but if you want to be choosy about your http
variables, you should be using the "http.$URL.sslverify" form, to only
affect specific servers (whether they are in submodules or not).

I considered that, and thought that it might be dangerous, what with me
not vetting carefully which http.* variables are safe to pass on to the
submodules' update and which are not.

So I had a look now, and the most prominent potential problem is the
http.cookieFile setting, which could be reused all of a sudden if we
made my patch more general.

But then, we are talking about the code that filters what gets passed via
the *command-line*. And to be quite honest, I am not sure that we should
actually filter out *any* of these settings.

The commit message that introduced this particular filtering has this
rationale to let only credential.* through:

    GIT_CONFIG_PARAMETERS is special, and we actually do want to
    preserve these settings. However, we do not want to preserve all
    configuration as many things should be left specific to the parent
    project.

    Add a git submodule--helper function, sanitize-config, which shall be
    used to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs
    except a small subset that are known to be safe and necessary.

Dunno. I tried to err on the side of caution... But this sounds maybe a
bit *too* cautious?

Jacob, Junio?

Ciao,
Dscho