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

Jan Engelhardt <jengelh@inai.de> writes:

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?

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.

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();

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.

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.