* [PATCH] http-backend: give a hint that web browser access is not supported
@ 2021-12-02 0:39 Jan Engelhardt
2021-12-02 7:38 ` Junio C Hamano
0 siblings, 1 reply; 11+ messages in thread
From: Jan Engelhardt @ 2021-12-02 0:39 UTC (permalink / raw)
To: git
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(-)
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] http-backend: give a hint that web browser access is not supported
2021-12-02 0:39 [PATCH] http-backend: give a hint that web browser access is not supported Jan Engelhardt
@ 2021-12-02 7:38 ` Junio C Hamano
2021-12-02 10:27 ` RFE: Split diff.noprefix for git-diff and git-format-patch (was: http-backend: give a hint that web browser access is not supported) Jan Engelhardt
2021-12-02 10:28 ` [PATCH] http-backend: give a hint that web browser access is not supported Jan Engelhardt
0 siblings, 2 replies; 11+ messages in thread
From: Junio C Hamano @ 2021-12-02 7:38 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: git
Jan Engelhardt <jengelh@inai.de> writes:
> 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* RFE: Split diff.noprefix for git-diff and git-format-patch (was: http-backend: give a hint that web browser access is not supported)
2021-12-02 7:38 ` Junio C Hamano
@ 2021-12-02 10:27 ` Jan Engelhardt
2021-12-02 17:20 ` RFE: Split diff.noprefix for git-diff and git-format-patch Junio C Hamano
2021-12-02 10:28 ` [PATCH] http-backend: give a hint that web browser access is not supported Jan Engelhardt
1 sibling, 1 reply; 11+ messages in thread
From: Jan Engelhardt @ 2021-12-02 10:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Thursday 2021-12-02 08:38, Junio C Hamano wrote:
>
>> 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: RFE: Split diff.noprefix for git-diff and git-format-patch
2021-12-02 10:27 ` RFE: Split diff.noprefix for git-diff and git-format-patch (was: http-backend: give a hint that web browser access is not supported) Jan Engelhardt
@ 2021-12-02 17:20 ` Junio C Hamano
0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2021-12-02 17:20 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: git
Jan Engelhardt <jengelh@inai.de> writes:
> 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] http-backend: give a hint that web browser access is not supported
2021-12-02 7:38 ` Junio C Hamano
2021-12-02 10:27 ` RFE: Split diff.noprefix for git-diff and git-format-patch (was: http-backend: give a hint that web browser access is not supported) Jan Engelhardt
@ 2021-12-02 10:28 ` Jan Engelhardt
2021-12-04 8:09 ` Junio C Hamano
1 sibling, 1 reply; 11+ messages in thread
From: Jan Engelhardt @ 2021-12-02 10:28 UTC (permalink / raw)
To: git
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(-)
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] http-backend: give a hint that web browser access is not supported
2021-12-02 10:28 ` [PATCH] http-backend: give a hint that web browser access is not supported Jan Engelhardt
@ 2021-12-04 8:09 ` Junio C Hamano
2021-12-04 11:09 ` Jan Engelhardt
0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2021-12-04 8:09 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: git
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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] http-backend: give a hint that web browser access is not supported
2021-12-04 8:09 ` Junio C Hamano
@ 2021-12-04 11:09 ` Jan Engelhardt
2021-12-05 1:17 ` Junio C Hamano
0 siblings, 1 reply; 11+ messages in thread
From: Jan Engelhardt @ 2021-12-04 11:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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").
>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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] http-backend: give a hint that web browser access is not supported
2021-12-04 11:09 ` Jan Engelhardt
@ 2021-12-05 1:17 ` Junio C Hamano
2021-12-05 10:13 ` Jan Engelhardt
0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2021-12-05 1:17 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: git
Jan Engelhardt <jengelh@inai.de> writes:
> 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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] http-backend: give a hint that web browser access is not supported
2021-12-05 1:17 ` Junio C Hamano
@ 2021-12-05 10:13 ` Jan Engelhardt
2021-12-05 20:13 ` Junio C Hamano
0 siblings, 1 reply; 11+ messages in thread
From: Jan Engelhardt @ 2021-12-05 10:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Sunday 2021-12-05 02:17, Junio C Hamano wrote:
>>>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.
>> 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 :-)
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] http-backend: give a hint that web browser access is not supported
2021-12-05 10:13 ` Jan Engelhardt
@ 2021-12-05 20:13 ` Junio C Hamano
2021-12-05 23:07 ` Junio C Hamano
0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2021-12-05 20:13 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: git
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] http-backend: give a hint that web browser access is not supported
2021-12-05 20:13 ` Junio C Hamano
@ 2021-12-05 23:07 ` Junio C Hamano
0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2021-12-05 23:07 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: git
Junio C Hamano <gitster@pobox.com> writes:
> 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
@@ -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
#
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-12-05 23:07 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-02 0:39 [PATCH] http-backend: give a hint that web browser access is not supported Jan Engelhardt
2021-12-02 7:38 ` Junio C Hamano
2021-12-02 10:27 ` RFE: Split diff.noprefix for git-diff and git-format-patch (was: http-backend: give a hint that web browser access is not supported) Jan Engelhardt
2021-12-02 17:20 ` RFE: Split diff.noprefix for git-diff and git-format-patch Junio C Hamano
2021-12-02 10:28 ` [PATCH] http-backend: give a hint that web browser access is not supported Jan Engelhardt
2021-12-04 8:09 ` Junio C Hamano
2021-12-04 11:09 ` Jan Engelhardt
2021-12-05 1:17 ` Junio C Hamano
2021-12-05 10:13 ` Jan Engelhardt
2021-12-05 20:13 ` Junio C Hamano
2021-12-05 23:07 ` Junio C Hamano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.