Re: [PATCH v4 6/8] test-http-server: pass Git requests to http-backend

[Matthew John Cheetham <mjcheetham@outlook.com>]

On 2022-12-14 15:20, Victoria Dye wrote:

> Matthew John Cheetham via GitGitGadget wrote:
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>>
>> Teach the test-http-sever test helper to forward Git requests to the
>> `git-http-backend`.
>>
>> Introduce a new test script t5556-http-auth.sh that spins up the test
>> HTTP server and attempts an `ls-remote` on the served repository,
>> without any authentication.
>>
>> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
>> ---
>>  t/helper/test-http-server.c |  56 +++++++++++++++++++
>>  t/t5556-http-auth.sh        | 105 ++++++++++++++++++++++++++++++++++++
>>  2 files changed, 161 insertions(+)
>>  create mode 100755 t/t5556-http-auth.sh
>>
>> diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c
>> index 7bde678e264..9f1d6b58067 100644
>> --- a/t/helper/test-http-server.c
>> +++ b/t/helper/test-http-server.c
>> @@ -305,8 +305,64 @@ done:
>>  	return result;
>>  }
>>  
>> +static int is_git_request(struct req *req)
>> +{
>> +	static regex_t *smart_http_regex;
>> +	static int initialized;
>> +
>> +	if (!initialized) {
>> +		smart_http_regex = xmalloc(sizeof(*smart_http_regex));
>> +		if (regcomp(smart_http_regex, "^/(HEAD|info/refs|"
>> +			    "objects/info/[^/]+|git-(upload|receive)-pack)$",
>> +			    REG_EXTENDED)) {
> 
> Could you explain the reasoning behind this regex (e.g., in a comment)? What
> sorts of valid/invalid requests does it represent? Is that the full set of
> requests that are "valid" to Git, or is it a test-specific subset?

Explanatory comment will be added in next iteration. These are the valid Git
endpoints for the dumb and smart HTTP protocols as specified in the tech docs.

>> +			warning("could not compile smart HTTP regex");
>> +			smart_http_regex = NULL;
>> +		}
>> +		initialized = 1;
>> +	}
>> +
>> +	return smart_http_regex &&
>> +		!regexec(smart_http_regex, req->uri_path.buf, 0, NULL, 0);
>> +}
>> +
>> +static enum worker_result do__git(struct req *req, const char *user)
>> +{
>> +	const char *ok = "HTTP/1.1 200 OK\r\n";
>> +	struct child_process cp = CHILD_PROCESS_INIT;
>> +	int res;
>> +
>> +	if (write(1, ok, strlen(ok)) < 0)
>> +		return error(_("could not send '%s'"), ok);
> 
> Is it correct to hardcode the response status to '200 OK'? Even when
> 'http-backend' exits with an error?

We always respond with a 200 OK response even if the http-backend process exits
with an error. This helper is intended only to be used to exercise the HTTP
auth handling in the Git client, and specifically around authentication (not
handled by http-backend).

If we wanted to respond with a more 'valid' HTTP response status then we'd need
to buffer the output of http-backend, wait for and grok the exit status of the
process, then write the HTTP status line followed by the http-backend output.
This is outside of the scope of this test helper's use at time of writing.

Important auth responses (401) we are handling prior to getting to this point.

The above will also be summarised in a comment on the next roll.

>> +
>> +	if (user)
>> +		strvec_pushf(&cp.env, "REMOTE_USER=%s", user);
> 
> I'm guessing that 'user' isn't used until a later patch? I think it might be
> better to not introduce that arg at all until it's needed (it'll put the
> usage of 'user' in context with how its value is determined), rather than
> hardcode it to 'NULL' for now.

Good point!

>> +
>> +	strvec_pushf(&cp.env, "REQUEST_METHOD=%s", req->method);
>> +	strvec_pushf(&cp.env, "PATH_TRANSLATED=%s",
>> +			req->uri_path.buf);
>> +	strvec_push(&cp.env, "SERVER_PROTOCOL=HTTP/1.1");
>> +	if (req->query_args.len)
>> +		strvec_pushf(&cp.env, "QUERY_STRING=%s",
>> +				req->query_args.buf);
>> +	if (req->content_type)
>> +		strvec_pushf(&cp.env, "CONTENT_TYPE=%s",
>> +				req->content_type);
>> +	if (req->content_length >= 0)
>> +		strvec_pushf(&cp.env, "CONTENT_LENGTH=%" PRIdMAX,
>> +				(intmax_t)req->content_length);
>> +	cp.git_cmd = 1;
>> +	strvec_push(&cp.args, "http-backend");
>> +	res = run_command(&cp);
> 
> I'm not super familiar with 'http-backend' but as long as it 1) uses the
> content passed into the environment to parse the request, and 2) writes the
> response to stdout, I think this is right.
> 
>> +	close(1);
>> +	close(0);
>> +	return !!res;
>> +}
>> +
>>  static enum worker_result dispatch(struct req *req)
>>  {
>> +	if (is_git_request(req))
>> +		return do__git(req, NULL);
>> +
>>  	return send_http_error(1, 501, "Not Implemented", -1, NULL,
>>  			       WR_OK | WR_HANGUP);
>>  }
>> diff --git a/t/t5556-http-auth.sh b/t/t5556-http-auth.sh
>> new file mode 100755
>> index 00000000000..78da151f122
>> --- /dev/null
>> +++ b/t/t5556-http-auth.sh
>> @@ -0,0 +1,105 @@
>> +#!/bin/sh
>> +
>> +test_description='test http auth header and credential helper interop'
>> +
>> +. ./test-lib.sh
>> +
>> +test_set_port GIT_TEST_HTTP_PROTOCOL_PORT
>> +
>> +# Setup a repository
>> +#
>> +REPO_DIR="$(pwd)"/repo
> 
> nit: '$TEST_OUTPUT_DIRECTORY' instead of '$(pwd)' is more consistent with
> what I see in other tests. 

I don't see this? In fact I see more usages of `$(pwd)` than your suggestion.

> Also, if you're creating a repo in its own subdirectory ('repo'), you can
> set 'TEST_NO_CREATE_REPO=1' before importing './test-lib' to avoid creating
> a repo at the root level of the test output dir - it can help avoid
> potential weird/unexpected behavior as a result of being in a repo inside of
> another repo.

However.. after setting `TEST_NO_CREATE_REPO=1` I was getting CI failures
around a missing PWD, so my next iteration uses the `$TRASH_DIRECTORY` variable
explicitly in paths instead :-)

>> +
>> +# Setup some lookback URLs where test-http-server will be listening.
>> +# We will spawn it directly inside the repo directory, so we avoid
>> +# any need to configure directory mappings etc - we only serve this
>> +# repository from the root '/' of the server.
>> +#
>> +HOST_PORT=127.0.0.1:$GIT_TEST_HTTP_PROTOCOL_PORT
>> +ORIGIN_URL=http://$HOST_PORT/
>> +
>> +# The pid-file is created by test-http-server when it starts.
>> +# The server will shutdown if/when we delete it (this is easier than
>> +# killing it by PID).
>> +#
>> +PID_FILE="$(pwd)"/pid-file.pid
>> +SERVER_LOG="$(pwd)"/OUT.server.log
>> +
>> +PATH="$GIT_BUILD_DIR/t/helper/:$PATH" && export PATH
>> +
>> +test_expect_success 'setup repos' '
>> +	test_create_repo "$REPO_DIR" &&
>> +	git -C "$REPO_DIR" branch -M main
>> +'
>> +
>> +stop_http_server () {
>> +	if ! test -f "$PID_FILE"
>> +	then
>> +		return 0
>> +	fi
>> +	#
>> +	# The server will shutdown automatically when we delete the pid-file.
>> +	#
>> +	rm -f "$PID_FILE"
>> +	#
>> +	# Give it a few seconds to shutdown (mainly to completely release the
>> +	# port before the next test start another instance and it attempts to
>> +	# bind to it).
>> +	#
>> +	for k in 0 1 2 3 4
>> +	do
>> +		if grep -q "Starting graceful shutdown" "$SERVER_LOG"
>> +		then
>> +			return 0
>> +		fi
>> +		sleep 1
>> +	done
>> +
>> +	echo "stop_http_server: timeout waiting for server shutdown"
>> +	return 1
>> +}
>> +
>> +start_http_server () {
>> +	#
>> +	# Launch our server into the background in repo_dir.
>> +	#
>> +	(
>> +		cd "$REPO_DIR"
>> +		test-http-server --verbose \
>> +			--listen=127.0.0.1 \
>> +			--port=$GIT_TEST_HTTP_PROTOCOL_PORT \
>> +			--reuseaddr \
>> +			--pid-file="$PID_FILE" \
>> +			"$@" \
>> +			2>"$SERVER_LOG" &
>> +	)
>> +	#
>> +	# Give it a few seconds to get started.
>> +	#
>> +	for k in 0 1 2 3 4
>> +	do
>> +		if test -f "$PID_FILE"
>> +		then
>> +			return 0
>> +		fi
>> +		sleep 1
>> +	done
>> +
>> +	echo "start_http_server: timeout waiting for server startup"
>> +	return 1
>> +}
> 
> These start/stop functions look good to me!
> 
>> +
>> +per_test_cleanup () {
>> +	stop_http_server &&
>> +	rm -f OUT.*
>> +}
>> +
>> +test_expect_success 'http auth anonymous no challenge' '
>> +	test_when_finished "per_test_cleanup" &&
>> +	start_http_server --allow-anonymous &&
> 
> The '--allow-anonymous' option isn't added until patch 7 [1], so the test
> will fail in this patch. I think the easiest way to solve that is to remove
> it here (although I think it's fine to leave the title "anonymous no
> challenge", though), then add it in patch 7. 
> 
> [1] https://lore.kernel.org/git/794256754c1f7d32e438dfb19a05444d423989aa.1670880984.git.gitgitgadget@gmail.com/

Good catch! Will fix.

>> +
>> +	# Attempt to read from a protected repository
>> +	git ls-remote $ORIGIN_URL
>> +'
>> +
>> +test_done
> 

Thanks,
Matthew