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

[Junio C Hamano <gitster@pobox.com>]

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?

> +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.