threads / discuss / 24837

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

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

## tl;dr

11 messages between Aug 23, 2010 and Jul 1, 2024.

replies: 10people: 5as markdown or json

Junio C Hamano· Aug 23, 2010, 18:05 UTC · lore

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 ;-)?
Jeff King· Aug 23, 2010, 18:38 UTC · re: Junio C Hamano · lore

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

On Mon, Aug 23, 2010 at 11:05:04AM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> 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
Jeff King· Aug 23, 2010, 19:16 UTC · re: Jeff King · lore

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

On Mon, Aug 23, 2010 at 02:38:57PM -0400, Jeff King wrote:
Show 7 quoted lines
> 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 <peff@peff.net>
---
 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
Jonathan Nieder· Aug 24, 2010, 06:41 UTC · re: Jeff King · lore

[PATCH 2/1] do not pass "git -c foo=bar" params to transport helpers

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 <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
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
Jeff King· Aug 24, 2010, 14:14 UTC · re: Jonathan Nieder · lore

Re: [PATCH 2/1] do not pass "git -c foo=bar" params to transport helpers

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
Eric Raible· Aug 24, 2010, 19:07 UTC · re: Jonathan Nieder · lore

Re: [PATCH 2/1] do not pass &quot;git -c foo=bar&quot; params to transport helpers

Jonathan Nieder <jrnieder <at> gmail.com> writes:
Show 7 quoted lines
>  #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"
Junio C Hamano· Jul 1, 2024, 20:42 UTC · re: Jonathan Nieder · lore

Re: [PATCH 2/1] do not pass "git -c foo=bar" params to transport helpers

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 12 quoted lines
> 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 <peff@peff.net>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Jeff King wrote:
>
>> Here's a first attempt. No idea if it has any bad side effects. :)
>
> Here's the transport boundary.
FYI: there is a follow-up discussion recently.
https://lore.kernel.org/git/20240701181916.GD3199@coredump.intra.peff.net/
Alex Riesen· Aug 23, 2010, 19:02 UTC · re: Junio C Hamano · lore

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

On Mon, Aug 23, 2010 at 20:05, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> 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.

Jeff King· Aug 23, 2010, 20:33 UTC · re: Alex Riesen · lore

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

On Mon, Aug 23, 2010 at 09:02:36PM +0200, Alex Riesen wrote:
Show 7 quoted lines
> 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
Jonathan Nieder· Aug 24, 2010, 05:01 UTC · re: Jeff King · lore

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

Jeff King wrote:
Show 7 quoted lines
> 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.

Jeff King· Aug 24, 2010, 14:12 UTC · re: Jonathan Nieder · lore

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

On Tue, Aug 24, 2010 at 12:01:27AM -0500, Jonathan Nieder wrote:
Show 18 quoted lines
> > 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

← back to recent threads