threads / patch / 57014

patchhttp-backend: give a hint that web browser access is not supported

Subject: [PATCH] http-backend: give a hint that web browser access is not supported

## tl;dr

11 messages between Dec 2, 2021 and Dec 5, 2021. Diffs are folded; open one to read it.

replies: 10people: 2as markdown or json

Jan Engelhardt· Dec 2, 2021, 00:39 UTC · lore

When using a browser to access a URI that is served by http-backend, nothing but a blank page is shown. This is not helpful.

Emit the same "Request not handled" messages, but to the CGI stream at stdout. Use the HTTP REQUEST_URI for this so that filesystem paths are not revealed more than necessary. Add a paragraph that browsing to http-backend URIs is not something that should normally be done.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
Previously botched the commit message. not_found is not very nice
to extend; one can but make a new function.
 http-backend.c          | 36 +++++++++++++++++++++++++++-----
 t/t5561-http-backend.sh | 46 ++++++++++++++++++++---------------------
 2 files changed, 54 insertions(+), 28 deletions(-)
Show changes to diff +54 −28
diff --git http-backend.c http-backend.c
index 3d6e2ff17f..f7858e9c49 100644
--- http-backend.c
+++ http-backend.c
@@ -139,6 +139,25 @@ static NORETURN void not_found(struct strbuf *hdr, const char *err, ...)
 	exit(0);
 }
 
+static NORETURN void not_found_2(struct strbuf *hdr, const char *dir,
+				 const char *pathinfo, const char *err,
+				 const char *hint)
+{
+	http_status(hdr, 404, "Not Found");
+	hdr_nocache(hdr);
+	strbuf_add(hdr, "\r\n", 2);
+	if (pathinfo != NULL)
+		strbuf_addf(hdr, "%s: ", pathinfo);
+	strbuf_addf(hdr, "%s.\r\n", err);
+	if (hint != NULL)
+		strbuf_addf(hdr, "%s\r\n", hint);
+	end_headers(hdr);
+
+	if (err && *err)
+		fprintf(stderr, "%s: %s\n", dir, err);
+	exit(0);
+}
+
 __attribute__((format (printf, 2, 3)))
 static NORETURN void forbidden(struct strbuf *hdr, const char *err, ...)
 {
@@ -736,7 +755,8 @@ static int bad_request(struct strbuf *hdr, const struct service_cmd *c)
 
 int cmd_main(int argc, const char **argv)
 {
-	char *method = getenv("REQUEST_METHOD");
+	const char *method = getenv("REQUEST_METHOD");
+	const char *pathinfo = getenv("PATH_INFO");
 	const char *proto_header;
 	char *dir;
 	struct service_cmd *cmd = NULL;
@@ -775,15 +795,21 @@ int cmd_main(int argc, const char **argv)
 		regfree(&re);
 	}
 
-	if (!cmd)
-		not_found(&hdr, "Request not supported: '%s'", dir);
+	if (!cmd) {
+		const char *hint = "";
+		if (strcmp(method, "GET") == 0)
+			hint = "You cannot use a web browser to access "
+			       "this URL. Only git operations like "
+			       "clone/ls-remote/etc. will work.\n";
+		not_found_2(&hdr, dir, pathinfo, "Request not supported", hint);
+	}
 
 	setup_path();
 	if (!enter_repo(dir, 0))
-		not_found(&hdr, "Not a git repository: '%s'", dir);
+		not_found_2(&hdr, dir, pathinfo, "Not a git repository", NULL);
 	if (!getenv("GIT_HTTP_EXPORT_ALL") &&
 	    access("git-daemon-export-ok", F_OK) )
-		not_found(&hdr, "Repository not exported: '%s'", dir);
+		not_found_2(&hdr, dir, pathinfo, "Repository not exported", NULL);
 
 	http_config();
 	max_request_buffer = git_env_ulong("GIT_HTTP_MAX_REQUEST_BUFFER",
diff --git t/t5561-http-backend.sh t/t5561-http-backend.sh
index 9c57d84315..d8add36fb4 100755
--- t/t5561-http-backend.sh
+++ t/t5561-http-backend.sh
@@ -44,7 +44,7 @@ grep '^[^#]' >exp <<EOF
 
 ###  refs/heads/main
 ###
-GET  /smart/repo.git/refs/heads/main HTTP/1.1 404 -
+GET  /smart/repo.git/refs/heads/main HTTP/1.1 404
 
 ###  getanyfile default
 ###
@@ -59,14 +59,14 @@ GET  /smart/repo.git/$IDX_URL HTTP/1.1 200
 
 ###  no git-daemon-export-ok
 ###
-GET  /smart_noexport/repo.git/HEAD HTTP/1.1 404 -
-GET  /smart_noexport/repo.git/info/refs HTTP/1.1 404 -
-GET  /smart_noexport/repo.git/objects/info/packs HTTP/1.1 404 -
-GET  /smart_noexport/repo.git/objects/info/alternates HTTP/1.1 404 -
-GET  /smart_noexport/repo.git/objects/info/http-alternates HTTP/1.1 404 -
-GET  /smart_noexport/repo.git/$LOOSE_URL HTTP/1.1 404 -
-GET  /smart_noexport/repo.git/$PACK_URL HTTP/1.1 404 -
-GET  /smart_noexport/repo.git/$IDX_URL HTTP/1.1 404 -
+GET  /smart_noexport/repo.git/HEAD HTTP/1.1 404
+GET  /smart_noexport/repo.git/info/refs HTTP/1.1 404
+GET  /smart_noexport/repo.git/objects/info/packs HTTP/1.1 404
+GET  /smart_noexport/repo.git/objects/info/alternates HTTP/1.1 404
+GET  /smart_noexport/repo.git/objects/info/http-alternates HTTP/1.1 404
+GET  /smart_noexport/repo.git/$LOOSE_URL HTTP/1.1 404
+GET  /smart_noexport/repo.git/$PACK_URL HTTP/1.1 404
+GET  /smart_noexport/repo.git/$IDX_URL HTTP/1.1 404
 
 ###  git-daemon-export-ok
 ###
@@ -92,14 +92,14 @@ GET  /smart/repo.git/$IDX_URL HTTP/1.1 200
 
 ###  getanyfile false
 ###
-GET  /smart/repo.git/HEAD HTTP/1.1 403 -
-GET  /smart/repo.git/info/refs HTTP/1.1 403 -
-GET  /smart/repo.git/objects/info/packs HTTP/1.1 403 -
-GET  /smart/repo.git/objects/info/alternates HTTP/1.1 403 -
-GET  /smart/repo.git/objects/info/http-alternates HTTP/1.1 403 -
-GET  /smart/repo.git/$LOOSE_URL HTTP/1.1 403 -
-GET  /smart/repo.git/$PACK_URL HTTP/1.1 403 -
-GET  /smart/repo.git/$IDX_URL HTTP/1.1 403 -
+GET  /smart/repo.git/HEAD HTTP/1.1 403
+GET  /smart/repo.git/info/refs HTTP/1.1 403
+GET  /smart/repo.git/objects/info/packs HTTP/1.1 403
+GET  /smart/repo.git/objects/info/alternates HTTP/1.1 403
+GET  /smart/repo.git/objects/info/http-alternates HTTP/1.1 403
+GET  /smart/repo.git/$LOOSE_URL HTTP/1.1 403
+GET  /smart/repo.git/$PACK_URL HTTP/1.1 403
+GET  /smart/repo.git/$IDX_URL HTTP/1.1 403
 
 ###  uploadpack default
 ###
@@ -113,13 +113,13 @@ POST /smart/repo.git/git-upload-pack HTTP/1.1 200 -
 
 ###  uploadpack false
 ###
-GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 403 -
-POST /smart/repo.git/git-upload-pack HTTP/1.1 403 -
+GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 403
+POST /smart/repo.git/git-upload-pack HTTP/1.1 403
 
 ###  receivepack default
 ###
-GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403 -
-POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -
+GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403
+POST /smart/repo.git/git-receive-pack HTTP/1.1 403
 
 ###  receivepack true
 ###
@@ -128,8 +128,8 @@ POST /smart/repo.git/git-receive-pack HTTP/1.1 200 -
 
 ###  receivepack false
 ###
-GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403 -
-POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -
+GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403
+POST /smart/repo.git/git-receive-pack HTTP/1.1 403
 EOF
 test_expect_success 'server request log matches test results' '
 	check_access_log exp

-- 
2.34.0
Junio C Hamano· Dec 2, 2021, 07:38 UTC · re: Jan Engelhardt · lore

Re: [PATCH] http-backend: give a hint that web browser access is not supported

Jan Engelhardt <jengelh@inai.de> writes:
Show 8 quoted lines
>  http-backend.c          | 36 +++++++++++++++++++++++++++-----
>  t/t5561-http-backend.sh | 46 ++++++++++++++++++++---------------------
>  2 files changed, 54 insertions(+), 28 deletions(-)
>
> diff --git http-backend.c http-backend.c
> index 3d6e2ff17f..f7858e9c49 100644
> --- http-backend.c
> +++ http-backend.c

Please fix your format-patch settings. The comparisons should be between a/http-backend.c and b/http-backend.c and not between the same path at the top-level.

Everybody uses -p1 patches around here and their tools are set-up to handle -p1 patches.

Thanks.
Jan Engelhardt· Dec 2, 2021, 10:27 UTC · re: Junio C Hamano · lore

RFE: Split diff.noprefix for git-diff and git-format-patch (was: http-backend: give a hint that web browser access is not supported)

On Thursday 2021-12-02 08:38, Junio C Hamano wrote:
Show 13 quoted lines
>
>>  http-backend.c          | 36 +++++++++++++++++++++++++++-----
>>  t/t5561-http-backend.sh | 46 ++++++++++++++++++++---------------------
>>  2 files changed, 54 insertions(+), 28 deletions(-)
>>
>> diff --git http-backend.c http-backend.c
>> index 3d6e2ff17f..f7858e9c49 100644
>> --- http-backend.c
>> +++ http-backend.c
>
>Please fix your format-patch settings.  The comparisons should be
>between a/http-backend.c and b/http-backend.c and not between the
>same path at the top-level.
You are right. But..

In interactive git-diff invocations, prefixless is the arguably desired mode, so as to facilitate xterm copy-and-paste of the pathname (since a/ does not exist, you would want to have it in the copypaste operation anywhere).

I can see why git-format-patch would make use of the "diff.noprefix" config directive, but equally, it's a bug that diff.noprefix has such broad implications and that there is no way to distinguish between diff and format-patch.

Junio C Hamano· Dec 2, 2021, 17:20 UTC · re: Jan Engelhardt · lore

Re: RFE: Split diff.noprefix for git-diff and git-format-patch

Jan Engelhardt <jengelh@inai.de> writes:
Show 8 quoted lines
> In interactive git-diff invocations, prefixless is the arguably desired mode,
> so as to facilitate xterm copy-and-paste of the pathname (since a/ does not
> exist, you would want to have it in the copypaste operation anywhere).
>
> I can see why git-format-patch would make use of the "diff.noprefix"
> config directive, but equally, it's a bug that diff.noprefix has such
> broad implications and that there is no way to distinguish between
> diff and format-patch.

I do not think it is unthinkable to have "log.*" configuration variables that mirror "diff.*" configuration variables and have them override the broader "diff.*" counterparts, and further add "format.*" configuration variables to do the same as even narrower override.

I do not offhand recall hearing anybody who wanted format.noprefix separately in the past, and I take it a sign that people are happy with paths with prefix in their "interactive" invocations. I of course am among those, as that is most of the diff snippet I send to the list are created when I say "how about doing it this way" in my response and tell "\C-u \M-! git diff --stat -p" to Emacs to include the output from the command to the message I am composing.

Jan Engelhardt· Dec 2, 2021, 10:28 UTC · re: Junio C Hamano · lore

When using a browser to access a URI that is served by http-backend, nothing but a blank page is shown. This is not helpful.

Emit the same "Request not handled" messages, but to the CGI stream at stdout. Use the HTTP REQUEST_URI for this so that filesystem paths are not revealed more than necessary. Add a paragraph that browsing to http-backend URIs is not something that should normally be done.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
Now as a -p1 patch.
 http-backend.c          | 36 +++++++++++++++++++++++++++-----
 t/t5561-http-backend.sh | 46 ++++++++++++++++++++---------------------
 2 files changed, 54 insertions(+), 28 deletions(-)
Show changes to 2 files +54 −28

http-backend.c, t/t5561-http-backend.sh

diff --git a/http-backend.c b/http-backend.c
index 3d6e2ff17f..f7858e9c49 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -139,6 +139,25 @@ static NORETURN void not_found(struct strbuf *hdr, const char *err, ...)
 	exit(0);
 }
 
+static NORETURN void not_found_2(struct strbuf *hdr, const char *dir,
+				 const char *pathinfo, const char *err,
+				 const char *hint)
+{
+	http_status(hdr, 404, "Not Found");
+	hdr_nocache(hdr);
+	strbuf_add(hdr, "\r\n", 2);
+	if (pathinfo != NULL)
+		strbuf_addf(hdr, "%s: ", pathinfo);
+	strbuf_addf(hdr, "%s.\r\n", err);
+	if (hint != NULL)
+		strbuf_addf(hdr, "%s\r\n", hint);
+	end_headers(hdr);
+
+	if (err && *err)
+		fprintf(stderr, "%s: %s\n", dir, err);
+	exit(0);
+}
+
 __attribute__((format (printf, 2, 3)))
 static NORETURN void forbidden(struct strbuf *hdr, const char *err, ...)
 {
@@ -736,7 +755,8 @@ static int bad_request(struct strbuf *hdr, const struct service_cmd *c)
 
 int cmd_main(int argc, const char **argv)
 {
-	char *method = getenv("REQUEST_METHOD");
+	const char *method = getenv("REQUEST_METHOD");
+	const char *pathinfo = getenv("PATH_INFO");
 	const char *proto_header;
 	char *dir;
 	struct service_cmd *cmd = NULL;
@@ -775,15 +795,21 @@ int cmd_main(int argc, const char **argv)
 		regfree(&re);
 	}
 
-	if (!cmd)
-		not_found(&hdr, "Request not supported: '%s'", dir);
+	if (!cmd) {
+		const char *hint = "";
+		if (strcmp(method, "GET") == 0)
+			hint = "You cannot use a web browser to access "
+			       "this URL. Only git operations like "
+			       "clone/ls-remote/etc. will work.\n";
+		not_found_2(&hdr, dir, pathinfo, "Request not supported", hint);
+	}
 
 	setup_path();
 	if (!enter_repo(dir, 0))
-		not_found(&hdr, "Not a git repository: '%s'", dir);
+		not_found_2(&hdr, dir, pathinfo, "Not a git repository", NULL);
 	if (!getenv("GIT_HTTP_EXPORT_ALL") &&
 	    access("git-daemon-export-ok", F_OK) )
-		not_found(&hdr, "Repository not exported: '%s'", dir);
+		not_found_2(&hdr, dir, pathinfo, "Repository not exported", NULL);
 
 	http_config();
 	max_request_buffer = git_env_ulong("GIT_HTTP_MAX_REQUEST_BUFFER",
diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
index 9c57d84315..d8add36fb4 100755
--- a/t/t5561-http-backend.sh
+++ b/t/t5561-http-backend.sh
@@ -44,7 +44,7 @@ grep '^[^#]' >exp <<EOF
 
 ###  refs/heads/main
 ###
-GET  /smart/repo.git/refs/heads/main HTTP/1.1 404 -
+GET  /smart/repo.git/refs/heads/main HTTP/1.1 404
 
 ###  getanyfile default
 ###
@@ -59,14 +59,14 @@ GET  /smart/repo.git/$IDX_URL HTTP/1.1 200
 
 ###  no git-daemon-export-ok
 ###
-GET  /smart_noexport/repo.git/HEAD HTTP/1.1 404 -
-GET  /smart_noexport/repo.git/info/refs HTTP/1.1 404 -
-GET  /smart_noexport/repo.git/objects/info/packs HTTP/1.1 404 -
-GET  /smart_noexport/repo.git/objects/info/alternates HTTP/1.1 404 -
-GET  /smart_noexport/repo.git/objects/info/http-alternates HTTP/1.1 404 -
-GET  /smart_noexport/repo.git/$LOOSE_URL HTTP/1.1 404 -
-GET  /smart_noexport/repo.git/$PACK_URL HTTP/1.1 404 -
-GET  /smart_noexport/repo.git/$IDX_URL HTTP/1.1 404 -
+GET  /smart_noexport/repo.git/HEAD HTTP/1.1 404
+GET  /smart_noexport/repo.git/info/refs HTTP/1.1 404
+GET  /smart_noexport/repo.git/objects/info/packs HTTP/1.1 404
+GET  /smart_noexport/repo.git/objects/info/alternates HTTP/1.1 404
+GET  /smart_noexport/repo.git/objects/info/http-alternates HTTP/1.1 404
+GET  /smart_noexport/repo.git/$LOOSE_URL HTTP/1.1 404
+GET  /smart_noexport/repo.git/$PACK_URL HTTP/1.1 404
+GET  /smart_noexport/repo.git/$IDX_URL HTTP/1.1 404
 
 ###  git-daemon-export-ok
 ###
@@ -92,14 +92,14 @@ GET  /smart/repo.git/$IDX_URL HTTP/1.1 200
 
 ###  getanyfile false
 ###
-GET  /smart/repo.git/HEAD HTTP/1.1 403 -
-GET  /smart/repo.git/info/refs HTTP/1.1 403 -
-GET  /smart/repo.git/objects/info/packs HTTP/1.1 403 -
-GET  /smart/repo.git/objects/info/alternates HTTP/1.1 403 -
-GET  /smart/repo.git/objects/info/http-alternates HTTP/1.1 403 -
-GET  /smart/repo.git/$LOOSE_URL HTTP/1.1 403 -
-GET  /smart/repo.git/$PACK_URL HTTP/1.1 403 -
-GET  /smart/repo.git/$IDX_URL HTTP/1.1 403 -
+GET  /smart/repo.git/HEAD HTTP/1.1 403
+GET  /smart/repo.git/info/refs HTTP/1.1 403
+GET  /smart/repo.git/objects/info/packs HTTP/1.1 403
+GET  /smart/repo.git/objects/info/alternates HTTP/1.1 403
+GET  /smart/repo.git/objects/info/http-alternates HTTP/1.1 403
+GET  /smart/repo.git/$LOOSE_URL HTTP/1.1 403
+GET  /smart/repo.git/$PACK_URL HTTP/1.1 403
+GET  /smart/repo.git/$IDX_URL HTTP/1.1 403
 
 ###  uploadpack default
 ###
@@ -113,13 +113,13 @@ POST /smart/repo.git/git-upload-pack HTTP/1.1 200 -
 
 ###  uploadpack false
 ###
-GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 403 -
-POST /smart/repo.git/git-upload-pack HTTP/1.1 403 -
+GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 403
+POST /smart/repo.git/git-upload-pack HTTP/1.1 403
 
 ###  receivepack default
 ###
-GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403 -
-POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -
+GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403
+POST /smart/repo.git/git-receive-pack HTTP/1.1 403
 
 ###  receivepack true
 ###
@@ -128,8 +128,8 @@ POST /smart/repo.git/git-receive-pack HTTP/1.1 200 -
 
 ###  receivepack false
 ###
-GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403 -
-POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -
+GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403
+POST /smart/repo.git/git-receive-pack HTTP/1.1 403
 EOF
 test_expect_success 'server request log matches test results' '
 	check_access_log exp

-- 
2.34.0
Junio C Hamano· Dec 4, 2021, 08:09 UTC · re: Jan Engelhardt · lore

Re: [PATCH] http-backend: give a hint that web browser access is not supported

Jan Engelhardt <jengelh@inai.de> writes:
> When using a browser to access a URI that is served by http-backend,
> nothing but a blank page is shown. This is not helpful.
>
> Emit the same "Request not handled" messages, but to the CGI stream

Puzzled. Same with what? The closest one in the code without this patch is "Request not supported" and one call (among the three) to the not_found_2() function does use that string, so perhaps that is what was meant here?

Show 10 quoted lines
> +static NORETURN void not_found_2(struct strbuf *hdr, const char *dir,
> +				 const char *pathinfo, const char *err,
> +				 const char *hint)
> +{
> +	http_status(hdr, 404, "Not Found");
> +	hdr_nocache(hdr);
> +	strbuf_add(hdr, "\r\n", 2);
> +	if (pathinfo != NULL)
> +		strbuf_addf(hdr, "%s: ", pathinfo);
> +	strbuf_addf(hdr, "%s.\r\n", err);

What is in "pathinfo" parameter? Can it safely be on the left side of the colon? I am reading that this part is emitting a series of HTTP header lines, so I would understand it if it were producing lines like 

    PATH-INFO: /hello+world
but it seems that you are instead writing
    /hello+world: <error string>.
here.
Notice that the above code already relies on err being non-NULL.
> +	if (hint != NULL)
> +		strbuf_addf(hdr, "%s\r\n", hint);
Likewise.  This just emits a random unstructured string.

By the way, do not compare pathinfo and hint pointers with != NULL; with "git grep" in this file you'll notice no existing code does that. Just write

	if (pathinfo)
		do_this();
> +	end_headers(hdr);
So here is where the HTTP headers end.

I think the use of internal API in http-backend.c in the new code is wrong; I haven't seen how it is used until now, so take this with a grain of salt, though.

Did you actually mean something different, that is:
	struct strbuf body = STRBUF_INIT;
	http_status(hdr, 404, "Not Found");
	hdr_nocache(hdr);
       
	/* stuff pathinfo, err, and hint into "body" strbuf ... */
	if (pathinfo)
		strbuf_addf(&body, "%s: ", pathinfo);
	strbuf_addf(&body, "%s.\r\n", err);
        if (hint)
		strbuf_addf(&body, "%s\r\n", hint);
	/* ... and send it out */
	send_strbuf(hdr, "text/plain", &body);
	strbuf_release(&body);

As end_headers() call emitted the necessary blank line after the header, anything you write to fd #1 after this point will become the body of the HTTP message. And send_strbuf() seems to be a helper that was designed for exactly this kind of usage.

> +	if (err && *err)
> +		fprintf(stderr, "%s: %s\n", dir, err);

We know err is not NULL already, so the first part "err &&" is way too late to be useful safety.

I notice that this is still going to the standard error stream. Is the intention that the remote requestor may get a lightly redacted error message while the log will leave detailed record to help debugging? In that case, I suspect that we may want to rename "dir" and "pathinfo" to make the distinction clearer (my understanding is that the former is the unredacted version and pathinfo is the redacted one).

Why do we need the original not_found()? It seems that there is only one remaining caller, so I think you can make it also use the new not_found_2() with NULL pathinfo and NULL dir (as that existing caller does not need it), and make the caller prepare the error string appropriately.

	char *p = git_pathdup("%s", name);
	size_t buf_alloc = 8192;
	char *buf = xmalloc(buf_alloc);
	int fd;
	struct stat sb;
	fd = open(p, O_RDONLY);
	if (fd < 0)
		not_found(hdr, "Cannot open '%s': %s", p, strerror(errno));

'p' is an unredacted one and we can use "dir" parameter for it, while 'name' can be stuffed in the "pathinfo" parameter, I guess. I wonder if something like this is close enough:

	not_found_2(hdr,
        	    p /* sensitive */,
                    name /* public */,
                    xstrfmt("Cannot open (%s)", strerror(errno)),
		    NULL);

ANd if we can get rid of the use of the original not_found(), we could even take its nice name over.

Jan Engelhardt· Dec 4, 2021, 11:09 UTC · re: Junio C Hamano · lore

Re: [PATCH] http-backend: give a hint that web browser access is not supported

On Saturday 2021-12-04 09:09, Junio C Hamano wrote:
Show 8 quoted lines
>Jan Engelhardt <jengelh@inai.de> writes:
>
>> When using a browser to access a URI that is served by http-backend,
>> nothing but a blank page is shown. This is not helpful.
>>
>> Emit the same "Request not handled" messages, but to the CGI stream
>
>Puzzled.  Same with what?

"Request not handled" is already sent to stderr, which means it (only) shows up in the httpd error log.

So now we send "Request not handled" also to stdout, which is what the browser will see.

>What is in "pathinfo" parameter?
It is getenv("PATH_INFO").
Show 21 quoted lines
>I think the use of internal API in http-backend.c in the new code is
>wrong; I haven't seen how it is used until now, so take this with a
>grain of salt, though.
>
>Did you actually mean something different, that is:
>
>	struct strbuf body = STRBUF_INIT;
>
>	http_status(hdr, 404, "Not Found");
>	hdr_nocache(hdr);
>       
>	/* stuff pathinfo, err, and hint into "body" strbuf ... */
>	if (pathinfo)
>		strbuf_addf(&body, "%s: ", pathinfo);
>	strbuf_addf(&body, "%s.\r\n", err);
>        if (hint)
>		strbuf_addf(&body, "%s\r\n", hint);
>
>	/* ... and send it out */
>	send_strbuf(hdr, "text/plain", &body);
>	strbuf_release(&body);
Yes, that seems more like it. I was not aware of send_strbuf.
>I notice that this is still going to the standard error stream.  Is
>the intention that the remote requestor may get a lightly redacted
>error message while the log will leave detailed record to help
>debugging?
Yes.
>Why do we need the original not_found()?  It seems that there is
>only one remaining caller
I suppose it can be dissolved.
>ANd if we can get rid of the use of the original not_found(), we
>could even take its nice name over.
Junio C Hamano· Dec 5, 2021, 01:17 UTC · re: Jan Engelhardt · lore

Re: [PATCH] http-backend: give a hint that web browser access is not supported

Jan Engelhardt <jengelh@inai.de> writes:
Show 20 quoted lines
> On Saturday 2021-12-04 09:09, Junio C Hamano wrote:
>
>>Jan Engelhardt <jengelh@inai.de> writes:
>>
>>> When using a browser to access a URI that is served by http-backend,
>>> nothing but a blank page is shown. This is not helpful.
>>>
>>> Emit the same "Request not handled" messages, but to the CGI stream
>>
>>Puzzled.  Same with what?
>
> "Request not handled" is already sent to stderr, which means it (only)
> shows up in the httpd error log.
>
> So now we send "Request not handled" also to stdout, which is what
> the browser will see.
>
>>What is in "pathinfo" parameter?
>
> It is getenv("PATH_INFO").

That part I know. The question was what would a typical value of that parameter look like in the context of somebody mistakenly visiting Git smart HTTP endpoint via their browser.

I am basically wondering if it is helping the user enough, or if it is sufficient to give just the "err" and "hint", and nothing else.

> Yes, that seems more like it. I was not aware of send_strbuf.

Heh, I wasn't either. The review of this topic was the first time I seriously read any part of that file, and I think I still only read just about 20% of it ;-)

Also, will the real Git clients, which are the primary intended audiences this program is trying to talk to, be OK if we suddenly start giving a non-empty 404 page?

If any implementations of Git HTTP client this program is serving (1) uses a 404 response as a cue to decide its next request (e.g. there may be some "try this URL and if it fails, do another one" fallback logic), and (2) depends on our 404 response to be without any body, we'd be breaking the service for our primary audience, only to mollify those who visit our HTTP endpoint that they should not be visiting in the first place via the browser, which would be worse than embarrassing.

Thanks.
Jan Engelhardt· Dec 5, 2021, 10:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] http-backend: give a hint that web browser access is not supported

On Sunday 2021-12-05 02:17, Junio C Hamano wrote:
Show 6 quoted lines
>>>What is in "pathinfo" parameter?
>> It is getenv("PATH_INFO").
>
>That part I know.  The question was what would a typical value of
>that parameter look like in the context of somebody mistakenly
>visiting Git smart HTTP endpoint via their browser.

As far as I can tell, it contains the request URI plus index.html resolution; https://git.inai.de/ reports /index.html while https://git.inai.de/foo reports /foo (since foo does not exist in the fs).

>I am basically wondering if it is helping the user enough, or if it
>is sufficient to give just the "err" and "hint", and nothing else.
I felt that, because ls(1) reports the filename again, e.g.

$ ls x ls: cannot access 'x': No such file or directory

that git-http-backend could do the same, especially since pathinfo isn't just $ENV{REQUEST_URI} again at all times.

Show 9 quoted lines
>> Yes, that seems more like it. I was not aware of send_strbuf.
>
>Heh, I wasn't either.  The review of this topic was the first time I
>seriously read any part of that file, and I think I still only read
>just about 20% of it ;-)
>
>Also, will the real Git clients, which are the primary intended
>audiences this program is trying to talk to, be OK if we suddenly
>start giving a non-empty 404 page?

I am confident enough to say yes. It's not like git-http-backend returned anything previously in the 404 case (like JSON or so), therefore clients could not possibly depend on content.

>If any implementations of Git HTTP client this program is serving
>(1) uses a 404 response as a cue to decide its next request
>(e.g. there may be some "try this URL and if it fails, do another
>one" fallback logic)

Not sure if they heed Location: headers, but I am not changing that :-)

Junio C Hamano· Dec 5, 2021, 20:13 UTC · re: Jan Engelhardt · lore

Re: [PATCH] http-backend: give a hint that web browser access is not supported

Jan Engelhardt <jengelh@inai.de> writes:
Show 15 quoted lines
>>Also, will the real Git clients, which are the primary intended
>>audiences this program is trying to talk to, be OK if we suddenly
>>start giving a non-empty 404 page?
>
> I am confident enough to say yes. It's not like git-http-backend
> returned anything previously in the 404 case (like JSON or so),
> therefore clients could not possibly depend on content.
>
>>If any implementations of Git HTTP client this program is serving
>>(1) uses a 404 response as a cue to decide its next request
>>(e.g. there may be some "try this URL and if it fails, do another
>>one" fallback logic)
>
> Not sure if they heed Location: headers, but I am not changing
> that :-)

I was more worried about clients barfing because they depend on *not* having content. They parse the status (404) out, and then leave the message part untouched---they may not even read the message in full, and that did not matter because there wasn't anything to read and discard. Now we are sending more.

As long as the leftover bytes would not cause problem with the action they take after that step, we would be OK.

Junio C Hamano· Dec 5, 2021, 23:07 UTC · re: Junio C Hamano · lore

Re: [PATCH] http-backend: give a hint that web browser access is not supported

Junio C Hamano <gitster@pobox.com> writes:
Show 26 quoted lines
> Jan Engelhardt <jengelh@inai.de> writes:
>
>>>Also, will the real Git clients, which are the primary intended
>>>audiences this program is trying to talk to, be OK if we suddenly
>>>start giving a non-empty 404 page?
>>
>> I am confident enough to say yes. It's not like git-http-backend
>> returned anything previously in the 404 case (like JSON or so),
>> therefore clients could not possibly depend on content.
>>
>>>If any implementations of Git HTTP client this program is serving
>>>(1) uses a 404 response as a cue to decide its next request
>>>(e.g. there may be some "try this URL and if it fails, do another
>>>one" fallback logic)
>>
>> Not sure if they heed Location: headers, but I am not changing
>> that :-)
>
> I was more worried about clients barfing because they depend on
> *not* having content.  They parse the status (404) out, and then
> leave the message part untouched---they may not even read the
> message in full, and that did not matter because there wasn't
> anything to read and discard.  Now we are sending more.
>
> As long as the leftover bytes would not cause problem with the
> action they take after that step, we would be OK.
In any case, the patch in question seems to fail t5561.

$ cd t && sh t5561-http-backend.sh -i -v Initialized empty Git repository in /home/jch/git/t/trash directory.t5561-http-backend/.git/ checking prerequisite: NOT_ROOT

... ok 13 - http.receivepack false

expecting success of 5561.14 'server request log matches test results': 
	check_access_log exp

--- exp.sorted 2021-12-05 23:05:09.418684299 +0000 +++ access.log.sorted 2021-12-05 23:05:09.422684296 +0000

Show changes to diff +28 −28
@@ -1,33 +1,33 @@
 GET  /smart/repo.git/HEAD HTTP/1.1 200
 GET  /smart/repo.git/HEAD HTTP/1.1 200
-GET  /smart/repo.git/HEAD HTTP/1.1 403
+GET  /smart/repo.git/HEAD HTTP/1.1 403 -
 GET  /smart/repo.git/info/refs HTTP/1.1 200
 GET  /smart/repo.git/info/refs HTTP/1.1 200
-GET  /smart/repo.git/info/refs HTTP/1.1 403
+GET  /smart/repo.git/info/refs HTTP/1.1 403 -
 GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 200
-GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403
-GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403
+GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403 -
+GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403 -
 GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
 GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
-GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 403
+GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 403 -
 GET  /smart/repo.git/objects/01/494420155a3f7587b6c26d06a55b1a8bbef2f4 HTTP/1.1 200
 GET  /smart/repo.git/objects/01/494420155a3f7587b6c26d06a55b1a8bbef2f4 HTTP/1.1 200
-GET  /smart/repo.git/objects/01/494420155a3f7587b6c26d06a55b1a8bbef2f4 HTTP/1.1 403
+GET  /smart/repo.git/objects/01/494420155a3f7587b6c26d06a55b1a8bbef2f4 HTTP/1.1 403 -
 GET  /smart/repo.git/objects/info/alternates HTTP/1.1 200 -
 GET  /smart/repo.git/objects/info/alternates HTTP/1.1 200 -
-GET  /smart/repo.git/objects/info/alternates HTTP/1.1 403
+GET  /smart/repo.git/objects/info/alternates HTTP/1.1 403 -
 GET  /smart/repo.git/objects/info/http-alternates HTTP/1.1 200 -
 GET  /smart/repo.git/objects/info/http-alternates HTTP/1.1 200 -
-GET  /smart/repo.git/objects/info/http-alternates HTTP/1.1 403
+GET  /smart/repo.git/objects/info/http-alternates HTTP/1.1 403 -
 GET  /smart/repo.git/objects/info/packs HTTP/1.1 200
 GET  /smart/repo.git/objects/info/packs HTTP/1.1 200
-GET  /smart/repo.git/objects/info/packs HTTP/1.1 403
+GET  /smart/repo.git/objects/info/packs HTTP/1.1 403 -
 GET  /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.idx HTTP/1.1 200
 GET  /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.idx HTTP/1.1 200
-GET  /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.idx HTTP/1.1 403
+GET  /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.idx HTTP/1.1 403 -
 GET  /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 200
 GET  /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 200
-GET  /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 403
+GET  /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 403 -
 GET  /smart/repo.git/refs/heads/main HTTP/1.1 404
 GET  /smart_noexport/repo.git/HEAD HTTP/1.1 200
 GET  /smart_noexport/repo.git/HEAD HTTP/1.1 404
@@ -46,8 +46,8 @@
 GET  /smart_noexport/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 200
 GET  /smart_noexport/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 404
 POST /smart/repo.git/git-receive-pack HTTP/1.1 200 -
-POST /smart/repo.git/git-receive-pack HTTP/1.1 403
-POST /smart/repo.git/git-receive-pack HTTP/1.1 403
+POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -
+POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -
 POST /smart/repo.git/git-upload-pack HTTP/1.1 200 -
 POST /smart/repo.git/git-upload-pack HTTP/1.1 200 -
-POST /smart/repo.git/git-upload-pack HTTP/1.1 403
+POST /smart/repo.git/git-upload-pack HTTP/1.1 403 -
--- exp	2021-12-05 23:05:09.410684305 +0000
+++ access.log.stripped	2021-12-05 23:05:09.422684296 +0000
@@ -31,23 +31,23 @@
 GET  /smart/repo.git/objects/01/494420155a3f7587b6c26d06a55b1a8bbef2f4 HTTP/1.1 200
 GET  /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 200
 GET  /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.idx HTTP/1.1 200
-GET  /smart/repo.git/HEAD HTTP/1.1 403
-GET  /smart/repo.git/info/refs HTTP/1.1 403
-GET  /smart/repo.git/objects/info/packs HTTP/1.1 403
-GET  /smart/repo.git/objects/info/alternates HTTP/1.1 403
-GET  /smart/repo.git/objects/info/http-alternates HTTP/1.1 403
-GET  /smart/repo.git/objects/01/494420155a3f7587b6c26d06a55b1a8bbef2f4 HTTP/1.1 403
-GET  /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 403
-GET  /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.idx HTTP/1.1 403
+GET  /smart/repo.git/HEAD HTTP/1.1 403 -
+GET  /smart/repo.git/info/refs HTTP/1.1 403 -
+GET  /smart/repo.git/objects/info/packs HTTP/1.1 403 -
+GET  /smart/repo.git/objects/info/alternates HTTP/1.1 403 -
+GET  /smart/repo.git/objects/info/http-alternates HTTP/1.1 403 -
+GET  /smart/repo.git/objects/01/494420155a3f7587b6c26d06a55b1a8bbef2f4 HTTP/1.1 403 -
+GET  /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 403 -
+GET  /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.idx HTTP/1.1 403 -
 GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
 POST /smart/repo.git/git-upload-pack HTTP/1.1 200 -
 GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
 POST /smart/repo.git/git-upload-pack HTTP/1.1 200 -
-GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 403
-POST /smart/repo.git/git-upload-pack HTTP/1.1 403
-GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403
-POST /smart/repo.git/git-receive-pack HTTP/1.1 403
+GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 403 -
+POST /smart/repo.git/git-upload-pack HTTP/1.1 403 -
+GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403 -
+POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -
 GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 200
 POST /smart/repo.git/git-receive-pack HTTP/1.1 200 -
-GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403
-POST /smart/repo.git/git-receive-pack HTTP/1.1 403
+GET  /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403 -
+POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -
not ok 14 - server request log matches test results
#	
#		check_access_log exp
#

← back to recent threads