"git -c web.browser=w3m help -w help" still kicks firefox

My firefox runs in a different "workspace" than what I usually look at
while working, and I've beem scratching my head why "git help -w help"
doesn't seem to do anything, while I was opening a new tab every time I
tried it.  And then I tried the command on the Subject line, only to end
up with yet another new tab X-<.

I know exactly why this happens---we save the config from the command line
on a list only so that we can apply them in the correct order after items
coming from files, but we do not use the saved values to pass them around
to sub-git invocations.

  8b1fa77 (Allow passing of configuration parameters in the command line, 2010-03-26)

A "trivial fix" would be to pass this info through the execv_git_cmd()
interface by either exporting it via an environment variable or by
modifying the command line options, but I am not sure about the possible
fallouts from such a change.  For example, does "git -c var=value config ..."
work sensibly when what "config" is told to do (say, remove a section)
contradicts with having the named var with a given value?

I am wondering if this is worth fixing it in the first place.

Opinions?  Patches ;-)?

On Mon, Aug 23, 2010 at 11:05:04AM -0700, Junio C Hamano wrote:

A "trivial fix" would be to pass this info through the execv_git_cmd()
interface by either exporting it via an environment variable or by
modifying the command line options, but I am not sure about the possible
fallouts from such a change.  For example, does "git -c var=value config ..."
work sensibly when what "config" is told to do (say, remove a section)
contradicts with having the named var with a given value?

I am wondering if this is worth fixing it in the first place.

IMHO, it needs to be fixed. This bug means "git -c foo=bar X" silently
ignores the new value of "foo" if "X" is an external or a shell script.
For something like "help" it is a minor inconvenience, but I can
certainly see this causing data loss. Just in 30 seconds of grepping, I
see that "git -c mergetool.keepbackup=true mergetool" would be silently
ignored. Oops.

The environment is the only sensible way to pass this down, because we
need to hit not just externals, but things like "git config" invocations
from shell scripts. IOW, "git -c" really is about executing in a
sub-environment that pretends that config is set. Obviously we would
need to quote and unquote when using the environment as a transport (or
do something horrible like making a temporary config file and pointing
at it through the environment).

As for "git config", I would assume that "-c" parameters impact how
config itself behaves, but have no bearing at all on actual
configuration that it writes. I don't know if that is the case now,
though.

-Peff

On Mon, Aug 23, 2010 at 20:05, Junio C Hamano [off-list ref] wrote:

I know exactly why this happens---we save the config from the command line
on a list only so that we can apply them in the correct order after items
coming from files, but we do not use the saved values to pass them around
to sub-git invocations.

 8b1fa77 (Allow passing of configuration parameters in the command line, 2010-03-26)

A "trivial fix" would be to pass this info through the execv_git_cmd()
interface by either exporting it via an environment variable or by
modifying the command line options, but I am not sure about the possible
fallouts from such a change.  For example, does "git -c var=value config ..."
work sensibly when what "config" is told to do (say, remove a section)
contradicts with having the named var with a given value?

I am wondering if this is worth fixing it in the first place.

Opinions?  Patches ;-)?

Maybe it is worth fixing, but on a case-by-case basis?

I mean changing the execv_git_cmd interface (or create a new execv function),
so that it can get the list of config vars to pass down to the callee. A trivial
case of its use would be to just pass the current config (or, more
likely, none).
Or, one could give it its own list of config parameters.

On Mon, Aug 23, 2010 at 02:38:57PM -0400, Jeff King wrote:

The environment is the only sensible way to pass this down, because we
need to hit not just externals, but things like "git config" invocations
from shell scripts. IOW, "git -c" really is about executing in a
sub-environment that pretends that config is set. Obviously we would
need to quote and unquote when using the environment as a transport (or
do something horrible like making a temporary config file and pointing
at it through the environment).

Here's a first attempt. No idea if it has any bad side effects. :)

-- >8 --
Subject: [PATCH] pass "git -c foo=bar" params through environment

Git uses the "-c foo=bar" parameters to set a config
variable for a single git invocation. We currently do this
by making a list in the current process and consulting that
list in git_config.

This works fine for built-ins, but the config changes are
silently ignored by subprocesses, including dashed externals
and invocations to "git config" from shell scripts.

This patch instead puts them in an environment variable
which we consult when looking at config (both internally and
via calls "git config").

Signed-off-by: Jeff King <redacted>
---
 cache.h  |    2 ++
 config.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 git.c    |    2 +-
 3 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index d8c0a98..b76d7b3 100644
--- a/cache.h
+++ b/cache.h

@@ -974,7 +974,9 @@ extern int update_server_info(int);
 typedef int (*config_fn_t)(const char *, const char *, void *);
 extern int git_default_config(const char *, const char *, void *);
 extern int git_config_from_file(config_fn_t fn, const char *, void *);
+extern void git_config_push_parameter(const char *text);
 extern int git_config_parse_parameter(const char *text);
+extern int git_config_parse_environment(void);
 extern int git_config_from_parameters(config_fn_t fn, void *data);
 extern int git_config(config_fn_t fn, void *);
 extern int git_parse_ulong(const char *, unsigned long *);

diff --git a/config.c b/config.c
index 7a18bc9..15eabaf 100644
--- a/config.c
+++ b/config.c

@@ -8,6 +8,7 @@
 #include "cache.h"
 #include "exec_cmd.h"
 #include "strbuf.h"
+#include "quote.h"
 
 #define MAXNAME (256)
 

@@ -34,6 +35,19 @@ static void lowercase(char *p)
 		*p = tolower(*p);
 }
 
+void git_config_push_parameter(const char *text)
+{
+	struct strbuf env = STRBUF_INIT;
+	const char *old = getenv("GIT_CONFIG_PARAMETERS");
+	if (old) {
+		strbuf_addstr(&env, old);
+		strbuf_addch(&env, ' ');
+	}
+	sq_quote_buf(&env, text);
+	setenv("GIT_CONFIG_PARAMETERS", env.buf, 1);
+	strbuf_release(&env);
+}
+
 int git_config_parse_parameter(const char *text)
 {
 	struct config_item *ct;

@@ -61,6 +75,37 @@ int git_config_parse_parameter(const char *text)
 	return 0;
 }
 
+int git_config_parse_environment(void) {
+	const char *env = getenv("GIT_CONFIG_PARAMETERS");
+	char *envw;
+	const char **argv = NULL;
+	int nr = 0, alloc = 0;
+	int i;
+
+	if (!env)
+		return 0;
+	/* sq_dequote will write over it */
+	envw = xstrdup(env);
+
+	if (sq_dequote_to_argv(envw, &argv, &nr, &alloc) < 0) {
+		free(envw);
+		return error("bogus format in GIT_CONFIG_PARAMETERS");
+	}
+
+	for (i = 0; i < nr; i++) {
+		if (git_config_parse_parameter(argv[i]) < 0) {
+			error("bogus config parameter: %s", argv[i]);
+			free(argv);
+			free(envw);
+			return -1;
+		}
+	}
+
+	free(argv);
+	free(envw);
+	return 0;
+}
+
 static int get_next_char(void)
 {
 	int c;

@@ -781,7 +826,14 @@ int git_config_global(void)
 
 int git_config_from_parameters(config_fn_t fn, void *data)
 {
+	static int loaded_environment;
 	const struct config_item *ct;
+
+	if (!loaded_environment) {
+		if (git_config_parse_environment() < 0)
+			return -1;
+		loaded_environment = 1;
+	}
 	for (ct = config_parameters; ct; ct = ct->next)
 		if (fn(ct->name, ct->value, data) < 0)
 			return -1;

@@ -820,10 +872,9 @@ int git_config(config_fn_t fn, void *data)
 	}
 	free(repo_config);
 
-	if (config_parameters) {
-		ret += git_config_from_parameters(fn, data);
+	ret += git_config_from_parameters(fn, data);
+	if (config_parameters)
 		found += 1;
-	}
 
 	if (found == 0)
 		return -1;

diff --git a/git.c b/git.c
index 8dda939..681f96c 100644
--- a/git.c
+++ b/git.c

@@ -137,7 +137,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				fprintf(stderr, "-c expects a configuration string\n" );
 				usage(git_usage_string);
 			}
-			git_config_parse_parameter((*argv)[1]);
+			git_config_push_parameter((*argv)[1]);
 			(*argv)++;
 			(*argc)--;
 		} else {

-- 
1.7.2.2.418.g55566.dirty

On Mon, Aug 23, 2010 at 09:02:36PM +0200, Alex Riesen wrote:

Maybe it is worth fixing, but on a case-by-case basis?

I mean changing the execv_git_cmd interface (or create a new execv
function), so that it can get the list of config vars to pass down to
the callee. A trivial case of its use would be to just pass the
current config (or, more likely, none).  Or, one could give it its own
list of config parameters.

I don't think that is enough. We don't necessarily know which config
options will be relevant to exec'd processes. We could be running some
user-defined command that calls a bunch of other git commands. Or a
hook, for that matter.

Which does bring up one interesting boundary. If I run:

  git -c receive.denyDeletes=false git push

what should happen? Obviously with cross-server communication the
environment won't get passed. I am inclined to say that even for local
cases, receive-pack should clear the string. Certainly for the sake of
consistency between local and remote transports, but it may also be a
security issue (in most cases, no, since you would have to be exec'ing
receive-pack directly, and you are clearly already running an arbitrary
command, but I can see somebody perhaps crossing a setuid boundary with
receive-pack).

-Peff

Jeff King wrote:

Which does bring up one interesting boundary. If I run:

  git -c receive.denyDeletes=false git push

what should happen? Obviously with cross-server communication the
environment won't get passed. I am inclined to say that even for local
cases, receive-pack should clear the string.

Sticky.  I agree with you that that would follow the principle of
least surprise.

On the other hand if I use

	git push --receive-pack='git -c receive.denyDeletes=false receive-pack'

then I would expect it to work.  I don't think this is a security
problem because I already could have set the remote $GIT_CONFIG just
as easily.

Like $GIT_CONFIG, $GIT_CONFIG_PARAMETERS needs to be suppressed by
"git push" and its cousins when running local transport helpers to
imitate remote transport well.

Noticed-by: Jeff King [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
---
Jeff King wrote:

Here's a first attempt. No idea if it has any bad side effects. :)

Here's the transport boundary.

 cache.h              |    3 ++-
 config.c             |    8 ++++----
 environment.c        |    1 +
 t/t5400-send-pack.sh |   23 +++++++++++++++++++++++
 4 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index 6f50ea8..181a305 100644
--- a/cache.h
+++ b/cache.h

@@ -379,6 +379,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define GRAFT_ENVIRONMENT "GIT_GRAFT_FILE"
 #define TEMPLATE_DIR_ENVIRONMENT "GIT_TEMPLATE_DIR"
 #define CONFIG_ENVIRONMENT "GIT_CONFIG"
+#define CONFIG_DATA_ENVIRONMENT "GIT_CONFIG_PARAMETERS"
 #define EXEC_PATH_ENVIRONMENT "GIT_EXEC_PATH"
 #define CEILING_DIRECTORIES_ENVIRONMENT "GIT_CEILING_DIRECTORIES"
 #define NO_REPLACE_OBJECTS_ENVIRONMENT "GIT_NO_REPLACE_OBJECTS"

@@ -397,7 +398,7 @@ static inline enum object_type object_type(unsigned int mode)
  * environment creation or simple walk of the list.
  * The number of non-NULL entries is available as a macro.
  */
-#define LOCAL_REPO_ENV_SIZE 8
+#define LOCAL_REPO_ENV_SIZE 9
 extern const char *const local_repo_env[LOCAL_REPO_ENV_SIZE + 1];
 
 extern int is_bare_repository_cfg;

diff --git a/config.c b/config.c
index e08e32b..c2c995f 100644
--- a/config.c
+++ b/config.c

@@ -38,13 +38,13 @@ static void lowercase(char *p)
 void git_config_push_parameter(const char *text)
 {
 	struct strbuf env = STRBUF_INIT;
-	const char *old = getenv("GIT_CONFIG_PARAMETERS");
+	const char *old = getenv(CONFIG_DATA_ENVIRONMENT);
 	if (old) {
 		strbuf_addstr(&env, old);
 		strbuf_addch(&env, ' ');
 	}
 	sq_quote_buf(&env, text);
-	setenv("GIT_CONFIG_PARAMETERS", env.buf, 1);
+	setenv(CONFIG_DATA_ENVIRONMENT, env.buf, 1);
 	strbuf_release(&env);
 }
 

@@ -76,7 +76,7 @@ int git_config_parse_parameter(const char *text)
 }
 
 int git_config_parse_environment(void) {
-	const char *env = getenv("GIT_CONFIG_PARAMETERS");
+	const char *env = getenv(CONFIG_DATA_ENVIRONMENT);
 	char *envw;
 	const char **argv = NULL;
 	int nr = 0, alloc = 0;

@@ -89,7 +89,7 @@ int git_config_parse_environment(void) {
 
 	if (sq_dequote_to_argv(envw, &argv, &nr, &alloc) < 0) {
 		free(envw);
-		return error("bogus format in GIT_CONFIG_PARAMETERS");
+		return error("bogus format in " CONFIG_DATA_ENVIRONMENT);
 	}
 
 	for (i = 0; i < nr; i++) {

diff --git a/environment.c b/environment.c
index 83d38d3..a199f63 100644
--- a/environment.c
+++ b/environment.c

@@ -72,6 +72,7 @@ static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;
 const char * const local_repo_env[LOCAL_REPO_ENV_SIZE + 1] = {
 	ALTERNATE_DB_ENVIRONMENT,
 	CONFIG_ENVIRONMENT,
+	CONFIG_DATA_ENVIRONMENT,
 	DB_ENVIRONMENT,
 	GIT_DIR_ENVIRONMENT,
 	GIT_WORK_TREE_ENVIRONMENT,

diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index c718253..5bcf0b8 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh

@@ -94,6 +94,29 @@ test_expect_success 'refuse deleting push with denyDeletes' '
 	test_must_fail git send-pack ./victim :extra master
 '
 
+test_expect_success 'cannot override denyDeletes with git -c send-pack' '
+	(
+		cd victim &&
+		test_might_fail git branch -D extra &&
+		git config receive.denyDeletes true &&
+		git branch extra master
+	) &&
+	test_must_fail git -c receive.denyDeletes=false \
+					send-pack ./victim :extra master
+'
+
+test_expect_success 'override denyDeletes with git -c receive-pack' '
+	(
+		cd victim &&
+		test_might_fail git branch -D extra &&
+		git config receive.denyDeletes true &&
+		git branch extra master
+	) &&
+	git send-pack \
+		--receive-pack="git -c receive.denyDeletes=false receive-pack" \
+		./victim :extra master
+'
+
 test_expect_success 'denyNonFastforwards trumps --force' '
 	(
 	    cd victim &&

-- 
1.7.2.2

On Tue, Aug 24, 2010 at 12:01:27AM -0500, Jonathan Nieder wrote:

quoted

Which does bring up one interesting boundary. If I run:

  git -c receive.denyDeletes=false git push

what should happen? Obviously with cross-server communication the
environment won't get passed. I am inclined to say that even for local
cases, receive-pack should clear the string.

Sticky.  I agree with you that that would follow the principle of
least surprise.

On the other hand if I use

	git push --receive-pack='git -c receive.denyDeletes=false receive-pack'

then I would expect it to work.  I don't think this is a security
problem because I already could have set the remote $GIT_CONFIG just
as easily.

Yeah, I think you are right. Anybody who was trying to cross a setuid
boundary with receive-pack is already screwed unless they are cleansing
the environment. And I would hope that any such cleansing would be
allow-known-good, so the new variable would be blocked along with
GIT_CONFIG.

So I doubt we are making anything worse, security-wise. I do think we
should still remove the variable in the local transport for the sake of
least surprise, and I agree that your example above should work.

-Peff

On Tue, Aug 24, 2010 at 01:41:14AM -0500, Jonathan Nieder wrote:

Like $GIT_CONFIG, $GIT_CONFIG_PARAMETERS needs to be suppressed by
"git push" and its cousins when running local transport helpers to
imitate remote transport well.

Thanks, this looks good to me.

Though arguably these bits:

+#define CONFIG_DATA_ENVIRONMENT "GIT_CONFIG_PARAMETERS"
[...]
-	const char *old = getenv("GIT_CONFIG_PARAMETERS");
+	const char *old = getenv(CONFIG_DATA_ENVIRONMENT);

Should be squashed into the original patch. :)

-Peff

Jonathan Nieder <jrnieder <at> gmail.com> writes:

 #define GRAFT_ENVIRONMENT "GIT_GRAFT_FILE"
 #define TEMPLATE_DIR_ENVIRONMENT "GIT_TEMPLATE_DIR"
 #define CONFIG_ENVIRONMENT "GIT_CONFIG"
+#define CONFIG_DATA_ENVIRONMENT "GIT_CONFIG_PARAMETERS"
 #define EXEC_PATH_ENVIRONMENT "GIT_EXEC_PATH"
 #define CEILING_DIRECTORIES_ENVIRONMENT "GIT_CEILING_DIRECTORIES"
 #define NO_REPLACE_OBJECTS_ENVIRONMENT "GIT_NO_REPLACE_OBJECTS"

Given that the pattern is:
#define foo_ENVIRONMENT "GIT_foo"

Your addition should be:
#define CONFIG_PARAMETERS_ENVIRONMENT "GIT_CONFIG_PARAMETERS"

Not only that, but the first one should be:

#define GRAFT_FILE_ENVIRONMENT "GIT_GRAFT_FILE"