{"thread":{"id":"57014","subject":"[PATCH] http-backend: give a hint that web browser access is not supported","startedAt":"2021-12-02T00:39:28Z","lastAt":"2021-12-05T23:07:23Z","messageCount":11,"participants":["Jan Engelhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"442838","messageId":"20211202003900.26124-1-jengelh@inai.de","threadId":"57014","inReplyTo":null,"subject":"[PATCH] http-backend: give a hint that web browser access is not supported","fromName":"Jan Engelhardt","fromEmail":"jengelh@inai.de","sentAt":"2021-12-02T00:39:00Z","receivedAt":"2021-12-02T00:39:28Z","isPatch":true,"sender":{"key":"jengelh@inai.de","avatar":"https://avatars.githubusercontent.com/u/8861948?v=4"},"body":"When using a browser to access a URI that is served by http-backend,\nnothing but a blank page is shown. This is not helpful.\n\nEmit the same \"Request not handled\" messages, but to the CGI stream\nat stdout. Use the HTTP REQUEST_URI for this so that filesystem paths\nare not revealed more than necessary. Add a paragraph that browsing\nto http-backend URIs is not something that should normally be done.\n\nSigned-off-by: Jan Engelhardt \n---\nPreviously botched the commit message. not_found is not very nice\nto extend; one can but make a new function.\n\n http-backend.c | 36 +++++++++++++++++++++++++++-----\n t/t5561-http-backend.sh | 46 ++++++++++++++++++++---------------------\n 2 files changed, 54 insertions(+), 28 deletions(-)\n\ndiff --git http-backend.c http-backend.c\nindex 3d6e2ff17f..f7858e9c49 100644\n--- http-backend.c\n+++ http-backend.c\n@@ -139,6 +139,25 @@ static NORETURN void not_found(struct strbuf *hdr, const char *err, ...)\n \texit(0);\n }\n \n+static NORETURN void not_found_2(struct strbuf *hdr, const char *dir,\n+\t\t\t\t const char *pathinfo, const char *err,\n+\t\t\t\t const char *hint)\n+{\n+\thttp_status(hdr, 404, \"Not Found\");\n+\thdr_nocache(hdr);\n+\tstrbuf_add(hdr, \"\\r\\n\", 2);\n+\tif (pathinfo != NULL)\n+\t\tstrbuf_addf(hdr, \"%s: \", pathinfo);\n+\tstrbuf_addf(hdr, \"%s.\\r\\n\", err);\n+\tif (hint != NULL)\n+\t\tstrbuf_addf(hdr, \"%s\\r\\n\", hint);\n+\tend_headers(hdr);\n+\n+\tif (err && *err)\n+\t\tfprintf(stderr, \"%s: %s\\n\", dir, err);\n+\texit(0);\n+}\n+\n __attribute__((format (printf, 2, 3)))\n static NORETURN void forbidden(struct strbuf *hdr, const char *err, ...)\n {\n@@ -736,7 +755,8 @@ static int bad_request(struct strbuf *hdr, const struct service_cmd *c)\n \n int cmd_main(int argc, const char **argv)\n {\n-\tchar *method = getenv(\"REQUEST_METHOD\");\n+\tconst char *method = getenv(\"REQUEST_METHOD\");\n+\tconst char *pathinfo = getenv(\"PATH_INFO\");\n \tconst char *proto_header;\n \tchar *dir;\n \tstruct service_cmd *cmd = NULL;\n@@ -775,15 +795,21 @@ int cmd_main(int argc, const char **argv)\n \t\tregfree(&re);\n \t}\n \n-\tif (!cmd)\n-\t\tnot_found(&hdr, \"Request not supported: '%s'\", dir);\n+\tif (!cmd) {\n+\t\tconst char *hint = \"\";\n+\t\tif (strcmp(method, \"GET\") == 0)\n+\t\t\thint = \"You cannot use a web browser to access \"\n+\t\t\t \"this URL. Only git operations like \"\n+\t\t\t \"clone/ls-remote/etc. will work.\\n\";\n+\t\tnot_found_2(&hdr, dir, pathinfo, \"Request not supported\", hint);\n+\t}\n \n \tsetup_path();\n \tif (!enter_repo(dir, 0))\n-\t\tnot_found(&hdr, \"Not a git repository: '%s'\", dir);\n+\t\tnot_found_2(&hdr, dir, pathinfo, \"Not a git repository\", NULL);\n \tif (!getenv(\"GIT_HTTP_EXPORT_ALL\") &&\n \t access(\"git-daemon-export-ok\", F_OK) )\n-\t\tnot_found(&hdr, \"Repository not exported: '%s'\", dir);\n+\t\tnot_found_2(&hdr, dir, pathinfo, \"Repository not exported\", NULL);\n \n \thttp_config();\n \tmax_request_buffer = git_env_ulong(\"GIT_HTTP_MAX_REQUEST_BUFFER\",\ndiff --git t/t5561-http-backend.sh t/t5561-http-backend.sh\nindex 9c57d84315..d8add36fb4 100755\n--- t/t5561-http-backend.sh\n+++ t/t5561-http-backend.sh\n@@ -44,7 +44,7 @@ grep '^[^#]' >exp < writes:\n\n> http-backend.c | 36 +++++++++++++++++++++++++++-----\n> t/t5561-http-backend.sh | 46 ++++++++++++++++++++---------------------\n> 2 files changed, 54 insertions(+), 28 deletions(-)\n>\n> diff --git http-backend.c http-backend.c\n> index 3d6e2ff17f..f7858e9c49 100644\n> --- http-backend.c\n> +++ http-backend.c\n\nPlease fix your format-patch settings. The comparisons should be\nbetween a/http-backend.c and b/http-backend.c and not between the\nsame path at the top-level.\n\nEverybody uses -p1 patches around here and their tools are set-up to\nhandle -p1 patches.\n\nThanks.\n"},{"id":"442862","messageId":"s6r03p31-o7o7-2142-9oo7-qno483r213s5@vanv.qr","threadId":"57014","inReplyTo":"xmqqee6vwj67.fsf@gitster.g","subject":"RFE: Split diff.noprefix for git-diff and git-format-patch (was: http-backend: give a hint that web browser access is not supported)","fromName":"Jan Engelhardt","fromEmail":"jengelh@inai.de","sentAt":"2021-12-02T10:27:19Z","receivedAt":"2021-12-02T10:27:28Z","isPatch":false,"sender":{"key":"jengelh@inai.de","avatar":"https://avatars.githubusercontent.com/u/8861948?v=4"},"body":"\nOn Thursday 2021-12-02 08:38, Junio C Hamano wrote:\n>\n>> http-backend.c | 36 +++++++++++++++++++++++++++-----\n>> t/t5561-http-backend.sh | 46 ++++++++++++++++++++---------------------\n>> 2 files changed, 54 insertions(+), 28 deletions(-)\n>>\n>> diff --git http-backend.c http-backend.c\n>> index 3d6e2ff17f..f7858e9c49 100644\n>> --- http-backend.c\n>> +++ http-backend.c\n>\n>Please fix your format-patch settings. The comparisons should be\n>between a/http-backend.c and b/http-backend.c and not between the\n>same path at the top-level.\n\nYou are right. But..\n\nIn interactive git-diff invocations, prefixless is the arguably desired mode,\nso as to facilitate xterm copy-and-paste of the pathname (since a/ does not\nexist, you would want to have it in the copypaste operation anywhere).\n\nI can see why git-format-patch would make use of the \"diff.noprefix\"\nconfig directive, but equally, it's a bug that diff.noprefix has such\nbroad implications and that there is no way to distinguish between\ndiff and format-patch.\n"},{"id":"442863","messageId":"20211202102855.23907-1-jengelh@inai.de","threadId":"57014","inReplyTo":"xmqqee6vwj67.fsf@gitster.g","subject":"[PATCH] http-backend: give a hint that web browser access is not supported","fromName":"Jan Engelhardt","fromEmail":"jengelh@inai.de","sentAt":"2021-12-02T10:28:55Z","receivedAt":"2021-12-02T10:29:00Z","isPatch":true,"sender":{"key":"jengelh@inai.de","avatar":"https://avatars.githubusercontent.com/u/8861948?v=4"},"body":"When using a browser to access a URI that is served by http-backend,\nnothing but a blank page is shown. This is not helpful.\n\nEmit the same \"Request not handled\" messages, but to the CGI stream\nat stdout. Use the HTTP REQUEST_URI for this so that filesystem paths\nare not revealed more than necessary. Add a paragraph that browsing\nto http-backend URIs is not something that should normally be done.\n\nSigned-off-by: Jan Engelhardt \n---\nNow as a -p1 patch.\n\n http-backend.c | 36 +++++++++++++++++++++++++++-----\n t/t5561-http-backend.sh | 46 ++++++++++++++++++++---------------------\n 2 files changed, 54 insertions(+), 28 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 3d6e2ff17f..f7858e9c49 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -139,6 +139,25 @@ static NORETURN void not_found(struct strbuf *hdr, const char *err, ...)\n \texit(0);\n }\n \n+static NORETURN void not_found_2(struct strbuf *hdr, const char *dir,\n+\t\t\t\t const char *pathinfo, const char *err,\n+\t\t\t\t const char *hint)\n+{\n+\thttp_status(hdr, 404, \"Not Found\");\n+\thdr_nocache(hdr);\n+\tstrbuf_add(hdr, \"\\r\\n\", 2);\n+\tif (pathinfo != NULL)\n+\t\tstrbuf_addf(hdr, \"%s: \", pathinfo);\n+\tstrbuf_addf(hdr, \"%s.\\r\\n\", err);\n+\tif (hint != NULL)\n+\t\tstrbuf_addf(hdr, \"%s\\r\\n\", hint);\n+\tend_headers(hdr);\n+\n+\tif (err && *err)\n+\t\tfprintf(stderr, \"%s: %s\\n\", dir, err);\n+\texit(0);\n+}\n+\n __attribute__((format (printf, 2, 3)))\n static NORETURN void forbidden(struct strbuf *hdr, const char *err, ...)\n {\n@@ -736,7 +755,8 @@ static int bad_request(struct strbuf *hdr, const struct service_cmd *c)\n \n int cmd_main(int argc, const char **argv)\n {\n-\tchar *method = getenv(\"REQUEST_METHOD\");\n+\tconst char *method = getenv(\"REQUEST_METHOD\");\n+\tconst char *pathinfo = getenv(\"PATH_INFO\");\n \tconst char *proto_header;\n \tchar *dir;\n \tstruct service_cmd *cmd = NULL;\n@@ -775,15 +795,21 @@ int cmd_main(int argc, const char **argv)\n \t\tregfree(&re);\n \t}\n \n-\tif (!cmd)\n-\t\tnot_found(&hdr, \"Request not supported: '%s'\", dir);\n+\tif (!cmd) {\n+\t\tconst char *hint = \"\";\n+\t\tif (strcmp(method, \"GET\") == 0)\n+\t\t\thint = \"You cannot use a web browser to access \"\n+\t\t\t \"this URL. Only git operations like \"\n+\t\t\t \"clone/ls-remote/etc. will work.\\n\";\n+\t\tnot_found_2(&hdr, dir, pathinfo, \"Request not supported\", hint);\n+\t}\n \n \tsetup_path();\n \tif (!enter_repo(dir, 0))\n-\t\tnot_found(&hdr, \"Not a git repository: '%s'\", dir);\n+\t\tnot_found_2(&hdr, dir, pathinfo, \"Not a git repository\", NULL);\n \tif (!getenv(\"GIT_HTTP_EXPORT_ALL\") &&\n \t access(\"git-daemon-export-ok\", F_OK) )\n-\t\tnot_found(&hdr, \"Repository not exported: '%s'\", dir);\n+\t\tnot_found_2(&hdr, dir, pathinfo, \"Repository not exported\", NULL);\n \n \thttp_config();\n \tmax_request_buffer = git_env_ulong(\"GIT_HTTP_MAX_REQUEST_BUFFER\",\ndiff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh\nindex 9c57d84315..d8add36fb4 100755\n--- a/t/t5561-http-backend.sh\n+++ b/t/t5561-http-backend.sh\n@@ -44,7 +44,7 @@ grep '^[^#]' >exp < writes:\n\n> In interactive git-diff invocations, prefixless is the arguably desired mode,\n> so as to facilitate xterm copy-and-paste of the pathname (since a/ does not\n> exist, you would want to have it in the copypaste operation anywhere).\n>\n> I can see why git-format-patch would make use of the \"diff.noprefix\"\n> config directive, but equally, it's a bug that diff.noprefix has such\n> broad implications and that there is no way to distinguish between\n> diff and format-patch.\n\nI do not think it is unthinkable to have \"log.*\" configuration\nvariables that mirror \"diff.*\" configuration variables and have them\noverride the broader \"diff.*\" counterparts, and further add \"format.*\"\nconfiguration variables to do the same as even narrower override.\n\nI do not offhand recall hearing anybody who wanted format.noprefix\nseparately in the past, and I take it a sign that people are happy\nwith paths with prefix in their \"interactive\" invocations. I of\ncourse am among those, as that is most of the diff snippet I send to\nthe list are created when I say \"how about doing it this way\" in my\nresponse and tell \"\\C-u \\M-! git diff --stat -p\" to Emacs to include\nthe output from the command to the message I am composing.\n"},{"id":"443068","messageId":"xmqqee6spz9s.fsf@gitster.g","threadId":"57014","inReplyTo":"20211202102855.23907-1-jengelh@inai.de","subject":"Re: [PATCH] http-backend: give a hint that web browser access is not supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-04T08:09:19Z","receivedAt":"2021-12-04T08:09:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jan Engelhardt writes:\n\n> When using a browser to access a URI that is served by http-backend,\n> nothing but a blank page is shown. This is not helpful.\n>\n> Emit the same \"Request not handled\" messages, but to the CGI stream\n\nPuzzled. Same with what? The closest one in the code without this\npatch is \"Request not supported\" and one call (among the three) to\nthe not_found_2() function does use that string, so perhaps that is\nwhat was meant here?\n\n> +static NORETURN void not_found_2(struct strbuf *hdr, const char *dir,\n> +\t\t\t\t const char *pathinfo, const char *err,\n> +\t\t\t\t const char *hint)\n> +{\n> +\thttp_status(hdr, 404, \"Not Found\");\n> +\thdr_nocache(hdr);\n> +\tstrbuf_add(hdr, \"\\r\\n\", 2);\n> +\tif (pathinfo != NULL)\n> +\t\tstrbuf_addf(hdr, \"%s: \", pathinfo);\n> +\tstrbuf_addf(hdr, \"%s.\\r\\n\", err);\n\nWhat is in \"pathinfo\" parameter? Can it safely be on the left side\nof the colon? I am reading that this part is emitting a series of\nHTTP header lines, so I would understand it if it were producing\nlines like \n\n PATH-INFO: /hello+world\n\nbut it seems that you are instead writing\n\n /hello+world: .\n\nhere.\n\nNotice that the above code already relies on err being non-NULL.\n\n> +\tif (hint != NULL)\n> +\t\tstrbuf_addf(hdr, \"%s\\r\\n\", hint);\n\nLikewise. This just emits a random unstructured string.\n\nBy the way, do not compare pathinfo and hint pointers with != NULL;\nwith \"git grep\" in this file you'll notice no existing code does that.\nJust write\n\n\tif (pathinfo)\n\t\tdo_this();\n\n> +\tend_headers(hdr);\n\nSo here is where the HTTP headers end.\n\nI think the use of internal API in http-backend.c in the new code is\nwrong; I haven't seen how it is used until now, so take this with a\ngrain of salt, though.\n\nDid you actually mean something different, that is:\n\n\tstruct strbuf body = STRBUF_INIT;\n\n\thttp_status(hdr, 404, \"Not Found\");\n\thdr_nocache(hdr);\n \n\t/* stuff pathinfo, err, and hint into \"body\" strbuf ... */\n\tif (pathinfo)\n\t\tstrbuf_addf(&body, \"%s: \", pathinfo);\n\tstrbuf_addf(&body, \"%s.\\r\\n\", err);\n if (hint)\n\t\tstrbuf_addf(&body, \"%s\\r\\n\", hint);\n\n\t/* ... and send it out */\n\tsend_strbuf(hdr, \"text/plain\", &body);\n\tstrbuf_release(&body);\n\nAs end_headers() call emitted the necessary blank line after the\nheader, anything you write to fd #1 after this point will become\nthe body of the HTTP message. And send_strbuf() seems to be a\nhelper that was designed for exactly this kind of usage.\n\n> +\tif (err && *err)\n> +\t\tfprintf(stderr, \"%s: %s\\n\", dir, err);\n\nWe know err is not NULL already, so the first part \"err &&\" is way\ntoo late to be useful safety.\n\nI notice that this is still going to the standard error stream. Is\nthe intention that the remote requestor may get a lightly redacted\nerror message while the log will leave detailed record to help\ndebugging? In that case, I suspect that we may want to rename \"dir\"\nand \"pathinfo\" to make the distinction clearer (my understanding is\nthat the former is the unredacted version and pathinfo is the\nredacted one).\n\nWhy do we need the original not_found()? It seems that there is\nonly one remaining caller, so I think you can make it also use the\nnew not_found_2() with NULL pathinfo and NULL dir (as that existing\ncaller does not need it), and make the caller prepare the error\nstring appropriately.\n\n\tchar *p = git_pathdup(\"%s\", name);\n\tsize_t buf_alloc = 8192;\n\tchar *buf = xmalloc(buf_alloc);\n\tint fd;\n\tstruct stat sb;\n\n\tfd = open(p, O_RDONLY);\n\tif (fd < 0)\n\t\tnot_found(hdr, \"Cannot open '%s': %s\", p, strerror(errno));\n\n'p' is an unredacted one and we can use \"dir\" parameter for it,\nwhile 'name' can be stuffed in the \"pathinfo\" parameter, I guess.\nI wonder if something like this is close enough:\n\n\tnot_found_2(hdr,\n \t p /* sensitive */,\n name /* public */,\n xstrfmt(\"Cannot open (%s)\", strerror(errno)),\n\t\t NULL);\n\nANd if we can get rid of the use of the original not_found(), we\ncould even take its nice name over. \n"},{"id":"443075","messageId":"7r23s082-o3q0-479o-srqn-r45q778s5nq7@vanv.qr","threadId":"57014","inReplyTo":"xmqqee6spz9s.fsf@gitster.g","subject":"Re: [PATCH] http-backend: give a hint that web browser access is not supported","fromName":"Jan Engelhardt","fromEmail":"jengelh@inai.de","sentAt":"2021-12-04T11:09:52Z","receivedAt":"2021-12-04T11:09:56Z","isPatch":true,"sender":{"key":"jengelh@inai.de","avatar":"https://avatars.githubusercontent.com/u/8861948?v=4"},"body":"\nOn Saturday 2021-12-04 09:09, Junio C Hamano wrote:\n\n>Jan Engelhardt writes:\n>\n>> When using a browser to access a URI that is served by http-backend,\n>> nothing but a blank page is shown. This is not helpful.\n>>\n>> Emit the same \"Request not handled\" messages, but to the CGI stream\n>\n>Puzzled. Same with what?\n\n\"Request not handled\" is already sent to stderr, which means it (only)\nshows up in the httpd error log.\n\nSo now we send \"Request not handled\" also to stdout, which is what\nthe browser will see.\n\n>What is in \"pathinfo\" parameter?\n\nIt is getenv(\"PATH_INFO\").\n\n>I think the use of internal API in http-backend.c in the new code is\n>wrong; I haven't seen how it is used until now, so take this with a\n>grain of salt, though.\n>\n>Did you actually mean something different, that is:\n>\n>\tstruct strbuf body = STRBUF_INIT;\n>\n>\thttp_status(hdr, 404, \"Not Found\");\n>\thdr_nocache(hdr);\n> \n>\t/* stuff pathinfo, err, and hint into \"body\" strbuf ... */\n>\tif (pathinfo)\n>\t\tstrbuf_addf(&body, \"%s: \", pathinfo);\n>\tstrbuf_addf(&body, \"%s.\\r\\n\", err);\n> if (hint)\n>\t\tstrbuf_addf(&body, \"%s\\r\\n\", hint);\n>\n>\t/* ... and send it out */\n>\tsend_strbuf(hdr, \"text/plain\", &body);\n>\tstrbuf_release(&body);\n\nYes, that seems more like it. I was not aware of send_strbuf.\n\n>I notice that this is still going to the standard error stream. Is\n>the intention that the remote requestor may get a lightly redacted\n>error message while the log will leave detailed record to help\n>debugging?\n\nYes.\n\n>Why do we need the original not_found()? It seems that there is\n>only one remaining caller\n\nI suppose it can be dissolved.\n\n>ANd if we can get rid of the use of the original not_found(), we\n>could even take its nice name over. \n"},{"id":"443096","messageId":"xmqqtufnonor.fsf@gitster.g","threadId":"57014","inReplyTo":"7r23s082-o3q0-479o-srqn-r45q778s5nq7@vanv.qr","subject":"Re: [PATCH] http-backend: give a hint that web browser access is not supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-05T01:17:08Z","receivedAt":"2021-12-05T01:17:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jan Engelhardt writes:\n\n> On Saturday 2021-12-04 09:09, Junio C Hamano wrote:\n>\n>>Jan Engelhardt writes:\n>>\n>>> When using a browser to access a URI that is served by http-backend,\n>>> nothing but a blank page is shown. This is not helpful.\n>>>\n>>> Emit the same \"Request not handled\" messages, but to the CGI stream\n>>\n>>Puzzled. Same with what?\n>\n> \"Request not handled\" is already sent to stderr, which means it (only)\n> shows up in the httpd error log.\n>\n> So now we send \"Request not handled\" also to stdout, which is what\n> the browser will see.\n>\n>>What is in \"pathinfo\" parameter?\n>\n> It is getenv(\"PATH_INFO\").\n\nThat part I know. The question was what would a typical value of\nthat parameter look like in the context of somebody mistakenly\nvisiting Git smart HTTP endpoint via their browser.\n\nI am basically wondering if it is helping the user enough, or if it\nis sufficient to give just the \"err\" and \"hint\", and nothing else.\n\n> Yes, that seems more like it. I was not aware of send_strbuf.\n\nHeh, I wasn't either. The review of this topic was the first time I\nseriously read any part of that file, and I think I still only read\njust about 20% of it ;-)\n\nAlso, will the real Git clients, which are the primary intended\naudiences this program is trying to talk to, be OK if we suddenly\nstart giving a non-empty 404 page?\n\nIf any implementations of Git HTTP client this program is serving\n(1) uses a 404 response as a cue to decide its next request\n(e.g. there may be some \"try this URL and if it fails, do another\none\" fallback logic), and (2) depends on our 404 response to be\nwithout any body, we'd be breaking the service for our primary\naudience, only to mollify those who visit our HTTP endpoint that\nthey should not be visiting in the first place via the browser,\nwhich would be worse than embarrassing.\n\nThanks.\n"},{"id":"443104","messageId":"34pqs81o-36p6-s416-s791-1onsqo1734oo@vanv.qr","threadId":"57014","inReplyTo":"xmqqtufnonor.fsf@gitster.g","subject":"Re: [PATCH] http-backend: give a hint that web browser access is not supported","fromName":"Jan Engelhardt","fromEmail":"jengelh@inai.de","sentAt":"2021-12-05T10:13:32Z","receivedAt":"2021-12-05T10:13:36Z","isPatch":true,"sender":{"key":"jengelh@inai.de","avatar":"https://avatars.githubusercontent.com/u/8861948?v=4"},"body":"\nOn Sunday 2021-12-05 02:17, Junio C Hamano wrote:\n>>>What is in \"pathinfo\" parameter?\n>> It is getenv(\"PATH_INFO\").\n>\n>That part I know. The question was what would a typical value of\n>that parameter look like in the context of somebody mistakenly\n>visiting Git smart HTTP endpoint via their browser.\n\nAs far as I can tell, it contains the request URI plus index.html resolution;\nhttps://git.inai.de/ reports /index.html while\nhttps://git.inai.de/foo reports /foo (since foo does not exist in the fs).\n\n>I am basically wondering if it is helping the user enough, or if it\n>is sufficient to give just the \"err\" and \"hint\", and nothing else.\n\nI felt that, because ls(1) reports the filename again, e.g.\n\n$ ls x\nls: cannot access 'x': No such file or directory\n\nthat git-http-backend could do the same, especially since\npathinfo isn't just $ENV{REQUEST_URI} again at all times.\n\n>> Yes, that seems more like it. I was not aware of send_strbuf.\n>\n>Heh, I wasn't either. The review of this topic was the first time I\n>seriously read any part of that file, and I think I still only read\n>just about 20% of it ;-)\n>\n>Also, will the real Git clients, which are the primary intended\n>audiences this program is trying to talk to, be OK if we suddenly\n>start giving a non-empty 404 page?\n\nI am confident enough to say yes. It's not like git-http-backend\nreturned anything previously in the 404 case (like JSON or so),\ntherefore clients could not possibly depend on content.\n\n>If any implementations of Git HTTP client this program is serving\n>(1) uses a 404 response as a cue to decide its next request\n>(e.g. there may be some \"try this URL and if it fails, do another\n>one\" fallback logic)\n\nNot sure if they heed Location: headers, but I am not changing\nthat :-)\n"},{"id":"443115","messageId":"xmqq7dcilsig.fsf@gitster.g","threadId":"57014","inReplyTo":"34pqs81o-36p6-s416-s791-1onsqo1734oo@vanv.qr","subject":"Re: [PATCH] http-backend: give a hint that web browser access is not supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-05T20:13:27Z","receivedAt":"2021-12-05T20:13:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jan Engelhardt writes:\n\n>>Also, will the real Git clients, which are the primary intended\n>>audiences this program is trying to talk to, be OK if we suddenly\n>>start giving a non-empty 404 page?\n>\n> I am confident enough to say yes. It's not like git-http-backend\n> returned anything previously in the 404 case (like JSON or so),\n> therefore clients could not possibly depend on content.\n>\n>>If any implementations of Git HTTP client this program is serving\n>>(1) uses a 404 response as a cue to decide its next request\n>>(e.g. there may be some \"try this URL and if it fails, do another\n>>one\" fallback logic)\n>\n> Not sure if they heed Location: headers, but I am not changing\n> that :-)\n\nI was more worried about clients barfing because they depend on\n*not* having content. They parse the status (404) out, and then\nleave the message part untouched---they may not even read the\nmessage in full, and that did not matter because there wasn't\nanything to read and discard. Now we are sending more.\n\nAs long as the leftover bytes would not cause problem with the\naction they take after that step, we would be OK.\n"},{"id":"443119","messageId":"xmqqwnkiirbs.fsf@gitster.g","threadId":"57014","inReplyTo":"xmqq7dcilsig.fsf@gitster.g","subject":"Re: [PATCH] http-backend: give a hint that web browser access is not supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-05T23:07:19Z","receivedAt":"2021-12-05T23:07:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jan Engelhardt writes:\n>\n>>>Also, will the real Git clients, which are the primary intended\n>>>audiences this program is trying to talk to, be OK if we suddenly\n>>>start giving a non-empty 404 page?\n>>\n>> I am confident enough to say yes. It's not like git-http-backend\n>> returned anything previously in the 404 case (like JSON or so),\n>> therefore clients could not possibly depend on content.\n>>\n>>>If any implementations of Git HTTP client this program is serving\n>>>(1) uses a 404 response as a cue to decide its next request\n>>>(e.g. there may be some \"try this URL and if it fails, do another\n>>>one\" fallback logic)\n>>\n>> Not sure if they heed Location: headers, but I am not changing\n>> that :-)\n>\n> I was more worried about clients barfing because they depend on\n> *not* having content. They parse the status (404) out, and then\n> leave the message part untouched---they may not even read the\n> message in full, and that did not matter because there wasn't\n> anything to read and discard. Now we are sending more.\n>\n> As long as the leftover bytes would not cause problem with the\n> action they take after that step, we would be OK.\n\nIn any case, the patch in question seems to fail t5561.\n\n$ cd t && sh t5561-http-backend.sh -i -v\nInitialized empty Git repository in /home/jch/git/t/trash directory.t5561-http-backend/.git/\nchecking prerequisite: NOT_ROOT\n\n...\nok 13 - http.receivepack false\n\nexpecting success of 5561.14 'server request log matches test results': \n\tcheck_access_log exp\n\n--- exp.sorted\t2021-12-05 23:05:09.418684299 +0000\n+++ access.log.sorted\t2021-12-05 23:05:09.422684296 +0000\n@@ -1,33 +1,33 @@\n GET /smart/repo.git/HEAD HTTP/1.1 200\n GET /smart/repo.git/HEAD HTTP/1.1 200\n-GET /smart/repo.git/HEAD HTTP/1.1 403\n+GET /smart/repo.git/HEAD HTTP/1.1 403 -\n GET /smart/repo.git/info/refs HTTP/1.1 200\n GET /smart/repo.git/info/refs HTTP/1.1 200\n-GET /smart/repo.git/info/refs HTTP/1.1 403\n+GET /smart/repo.git/info/refs HTTP/1.1 403 -\n GET /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 200\n-GET /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403\n-GET /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403\n+GET /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403 -\n+GET /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403 -\n GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\n GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\n-GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 403\n+GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 403 -\n GET /smart/repo.git/objects/01/494420155a3f7587b6c26d06a55b1a8bbef2f4 HTTP/1.1 200\n GET /smart/repo.git/objects/01/494420155a3f7587b6c26d06a55b1a8bbef2f4 HTTP/1.1 200\n-GET /smart/repo.git/objects/01/494420155a3f7587b6c26d06a55b1a8bbef2f4 HTTP/1.1 403\n+GET /smart/repo.git/objects/01/494420155a3f7587b6c26d06a55b1a8bbef2f4 HTTP/1.1 403 -\n GET /smart/repo.git/objects/info/alternates HTTP/1.1 200 -\n GET /smart/repo.git/objects/info/alternates HTTP/1.1 200 -\n-GET /smart/repo.git/objects/info/alternates HTTP/1.1 403\n+GET /smart/repo.git/objects/info/alternates HTTP/1.1 403 -\n GET /smart/repo.git/objects/info/http-alternates HTTP/1.1 200 -\n GET /smart/repo.git/objects/info/http-alternates HTTP/1.1 200 -\n-GET /smart/repo.git/objects/info/http-alternates HTTP/1.1 403\n+GET /smart/repo.git/objects/info/http-alternates HTTP/1.1 403 -\n GET /smart/repo.git/objects/info/packs HTTP/1.1 200\n GET /smart/repo.git/objects/info/packs HTTP/1.1 200\n-GET /smart/repo.git/objects/info/packs HTTP/1.1 403\n+GET /smart/repo.git/objects/info/packs HTTP/1.1 403 -\n GET /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.idx HTTP/1.1 200\n GET /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.idx HTTP/1.1 200\n-GET /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.idx HTTP/1.1 403\n+GET /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.idx HTTP/1.1 403 -\n GET /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 200\n GET /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 200\n-GET /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 403\n+GET /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 403 -\n GET /smart/repo.git/refs/heads/main HTTP/1.1 404\n GET /smart_noexport/repo.git/HEAD HTTP/1.1 200\n GET /smart_noexport/repo.git/HEAD HTTP/1.1 404\n@@ -46,8 +46,8 @@\n GET /smart_noexport/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 200\n GET /smart_noexport/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 404\n POST /smart/repo.git/git-receive-pack HTTP/1.1 200 -\n-POST /smart/repo.git/git-receive-pack HTTP/1.1 403\n-POST /smart/repo.git/git-receive-pack HTTP/1.1 403\n+POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -\n+POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -\n POST /smart/repo.git/git-upload-pack HTTP/1.1 200 -\n POST /smart/repo.git/git-upload-pack HTTP/1.1 200 -\n-POST /smart/repo.git/git-upload-pack HTTP/1.1 403\n+POST /smart/repo.git/git-upload-pack HTTP/1.1 403 -\n--- exp\t2021-12-05 23:05:09.410684305 +0000\n+++ access.log.stripped\t2021-12-05 23:05:09.422684296 +0000\n@@ -31,23 +31,23 @@\n GET /smart/repo.git/objects/01/494420155a3f7587b6c26d06a55b1a8bbef2f4 HTTP/1.1 200\n GET /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 200\n GET /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.idx HTTP/1.1 200\n-GET /smart/repo.git/HEAD HTTP/1.1 403\n-GET /smart/repo.git/info/refs HTTP/1.1 403\n-GET /smart/repo.git/objects/info/packs HTTP/1.1 403\n-GET /smart/repo.git/objects/info/alternates HTTP/1.1 403\n-GET /smart/repo.git/objects/info/http-alternates HTTP/1.1 403\n-GET /smart/repo.git/objects/01/494420155a3f7587b6c26d06a55b1a8bbef2f4 HTTP/1.1 403\n-GET /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 403\n-GET /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.idx HTTP/1.1 403\n+GET /smart/repo.git/HEAD HTTP/1.1 403 -\n+GET /smart/repo.git/info/refs HTTP/1.1 403 -\n+GET /smart/repo.git/objects/info/packs HTTP/1.1 403 -\n+GET /smart/repo.git/objects/info/alternates HTTP/1.1 403 -\n+GET /smart/repo.git/objects/info/http-alternates HTTP/1.1 403 -\n+GET /smart/repo.git/objects/01/494420155a3f7587b6c26d06a55b1a8bbef2f4 HTTP/1.1 403 -\n+GET /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.pack HTTP/1.1 403 -\n+GET /smart/repo.git/objects/pack/pack-977dd2d10981235a806ccc52cc769a44e75c889e.idx HTTP/1.1 403 -\n GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\n POST /smart/repo.git/git-upload-pack HTTP/1.1 200 -\n GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\n POST /smart/repo.git/git-upload-pack HTTP/1.1 200 -\n-GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 403\n-POST /smart/repo.git/git-upload-pack HTTP/1.1 403\n-GET /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403\n-POST /smart/repo.git/git-receive-pack HTTP/1.1 403\n+GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 403 -\n+POST /smart/repo.git/git-upload-pack HTTP/1.1 403 -\n+GET /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403 -\n+POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -\n GET /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 200\n POST /smart/repo.git/git-receive-pack HTTP/1.1 200 -\n-GET /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403\n-POST /smart/repo.git/git-receive-pack HTTP/1.1 403\n+GET /smart/repo.git/info/refs?service=git-receive-pack HTTP/1.1 403 -\n+POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -\nnot ok 14 - server request log matches test results\n#\t\n#\t\tcheck_access_log exp\n#\t\n"}]}