git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] Make http-backend REMOTE_USER configurable

From
Jeff King <peff@peff.net>
Date
Mar 30, 2012, 07:01 UTC
Message-ID
<20120330070130.GA30656@sigill.intra.peff.net>
In-Reply-To
<7vk422q2ho.fsf@alter.siamese.dyndns.org>
On Thu, Mar 29, 2012 at 06:52:35PM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> It would regress for somebody who is running the CGI program while
> exporting these environment variables pointing at himself and relying on
> the fact that these are canceled by REMOTE_USER/ADDR (perhaps a web-based
> editor can write into some repository and commits made by that editor
> takes the ident information from COMMITTER variables, while another part
> of the webserver takes a push by spawning the http backend???).
> 
> Which is very unlikely.
Yeah, I agree that is not worth worrying too much about.
> But I think this is the right thing to do, even though it is not related
> to the issue William wanted to address with his patch.
> 
> Care to sign it off?

Updated patch is below. In addition to a commit message and signoff, I tweaked two things:

  1. The original I posted failed to update the free() calls at the end
     of the function (and since we now have a variable number of env
     variables, we can't just free all elements). I ended up just
     converting this to use argv_array, which handles the memory
     management for us and is way shorter and easier to read.
  2. I added tests, both for the default behavior and for the new one.
     The "127.0.0.1" is hard-coded in the expected output (but comes
     from apache's setting of REMOTE_ADDR). I think this is OK. The
     lib-httpd setup code always listens on 127.0.0.1, so short of some
     insane loopback routing setup, the REMOTE_ADDR should also be
     127.0.0.1.
-- >8 --
Subject: http-backend: respect existing GIT_COMMITTER_* variables

The http-backend program sets default GIT_COMMITTER_NAME and GIT_COMMITTER_EMAIL variables based on the REMOTE_USER and REMOTE_ADDR variables provided by the webserver. However, it unconditionally overwrites any existing GIT_COMMITTER variables, which may have been customized by site-specific code in the webserver (or in a script wrapping http-backend).

Let's leave those variables intact if they already exist, assuming that any such configuration was intentional. There is a slight chance of a regression if somebody has set GIT_COMMITTER_* for the entire webserver, not intending it to leak through http-backend. We could protect against this by passing the information in alternate variables. However, it seems unlikely that anyone will care about that regression, and there is value in the simplicity of using the common variable names that are used elsewhere in git.

While we're tweaking the environment-handling in http-backend, let's switch it to use argv_array to handle the list of variables. That makes the memory management much simpler.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-backend.c          |   22 +++++++++-------------
 t/lib-httpd/apache.conf |    7 +++++++
 t/t5541-http-push.sh    |   21 +++++++++++++++++++++
 3 files changed, 37 insertions(+), 13 deletions(-)
diff --git a/http-backend.c b/http-backend.c
index 869d515..f50e77f 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -7,6 +7,7 @@
 #include "run-command.h"
 #include "string-list.h"
 #include "url.h"
+#include "argv-array.h"
 
 static const char content_type[] = "Content-Type";
 static const char content_length[] = "Content-Length";
@@ -317,8 +318,7 @@ static void run_service(const char **argv)
 	const char *encoding = getenv("HTTP_CONTENT_ENCODING");
 	const char *user = getenv("REMOTE_USER");
 	const char *host = getenv("REMOTE_ADDR");
-	char *env[3];
-	struct strbuf buf = STRBUF_INIT;
+	struct argv_array env = ARGV_ARRAY_INIT;
 	int gzipped_request = 0;
 	struct child_process cld;
 
@@ -332,17 +332,15 @@ static void run_service(const char **argv)
 	if (!host || !*host)
 		host = "(none)";
 
-	memset(&env, 0, sizeof(env));
-	strbuf_addf(&buf, "GIT_COMMITTER_NAME=%s", user);
-	env[0] = strbuf_detach(&buf, NULL);
-
-	strbuf_addf(&buf, "GIT_COMMITTER_EMAIL=%s@http.%s", user, host);
-	env[1] = strbuf_detach(&buf, NULL);
-	env[2] = NULL;
+	if (!getenv("GIT_COMMITTER_NAME"))
+		argv_array_pushf(&env, "GIT_COMMITTER_NAME=%s", user);
+	if (!getenv("GIT_COMMITTER_EMAIL"))
+		argv_array_pushf(&env, "GIT_COMMITTER_EMAIL=%s@http.%s",
+				 user, host);
 
 	memset(&cld, 0, sizeof(cld));
 	cld.argv = argv;
-	cld.env = (const char *const *)env;
+	cld.env = env.argv;
 	if (gzipped_request)
 		cld.in = -1;
 	cld.git_cmd = 1;
@@ -357,9 +355,7 @@ static void run_service(const char **argv)
 
 	if (finish_command(&cld))
 		exit(1);
-	free(env[0]);
-	free(env[1]);
-	strbuf_release(&buf);
+	argv_array_clear(&env);
 }
 
 static int show_text_ref(const char *name, const unsigned char *sha1,
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 3c12b05..de3762e 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -52,8 +52,15 @@ Alias /auth/ www/auth/
 <Location /smart_noexport/>
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
 </Location>
+<Location /smart_custom_env/>
+	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+	SetEnv GIT_HTTP_EXPORT_ALL
+	SetEnv GIT_COMMITTER_NAME "Custom User"
+	SetEnv GIT_COMMITTER_EMAIL custom@example.com
+</Location>
 ScriptAlias /smart/ ${GIT_EXEC_PATH}/git-http-backend/
 ScriptAlias /smart_noexport/ ${GIT_EXEC_PATH}/git-http-backend/
+ScriptAlias /smart_custom_env/ ${GIT_EXEC_PATH}/git-http-backend/
 <Directory ${GIT_EXEC_PATH}>
 	Options None
 </Directory>
diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index cc6f081..d7964c7 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -30,6 +30,7 @@ test_expect_success 'setup remote repository' '
 	git clone --bare test_repo test_repo.git &&
 	cd test_repo.git &&
 	git config http.receivepack true &&
+	git config core.logallrefupdates true &&
 	ORIG_HEAD=$(git rev-parse --verify HEAD) &&
 	cd - &&
 	mv test_repo.git "$HTTPD_DOCUMENT_ROOT_PATH"
@@ -222,5 +223,25 @@ test_expect_success TTY 'quiet push' '
 	test_cmp /dev/null output
 '
 
+test_expect_success 'http push gives sane defaults to reflog' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	test_commit reflog-test &&
+	git push "$HTTPD_URL"/smart/test_repo.git &&
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" \
+		log -g -1 --format="%gn <%ge>" >actual &&
+	echo "anonymous <anonymous@http.127.0.0.1>" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'http push respects GIT_COMMITTER_* in reflog' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	test_commit custom-reflog-test &&
+	git push "$HTTPD_URL"/smart_custom_env/test_repo.git &&
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" \
+		log -g -1 --format="%gn <%ge>" >actual &&
+	echo "Custom User <custom@example.com>" >expect &&
+	test_cmp expect actual
+'
+
 stop_httpd
 test_done

-- 
1.7.9.5.7.g11b89
Previous: William Strecker-KelloggNext: Junio C Hamano
Message 7 of 8 in “Make http-backend REMOTE_USER configurable”
  1. Make http-backend REMOTE_USER configurableWilliam Strecker-Kellogg, Mar 29, 2012
  2. Junio C HamanoMar 29, 2012
  3. Jeff KingMar 29, 2012
  4. Jeff KingMar 29, 2012
  5. Junio C HamanoMar 30, 2012
  6. William Strecker-KelloggMar 30, 2012
  7. Jeff KingMar 30, 2012
  8. Junio C HamanoMar 30, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.