| Message ID | 20180908054224.21856-1-max@max630.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] http-backend: allow empty CONTENT_LENGTH | expand |
From: Max Kirillov <max@max630.net> Subject: http-backend test: make empty CONTENT_LENGTH test more realistic This is a test of smart HTTP, so it should use the smart HTTP endpoints (e.g. /info/refs?service=git-receive-pack), not dumb HTTP (HEAD). Signed-off-by: Max Kirillov <max@max630.net> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Max Kirillov wrote: > Provided more thorough message, also fix test (it did not test > actually the error before) > > There will be more versions later, at least the one which suggested > by Jeff v2 is in "next", and I believe that version should already be sufficient for Git 2.19. Please correct me if I'm wrong. Since v2 is in "next", I think any further refinements are supposed to be incremental patches on top. Here's an example (representing the v2->v3 diff). It's more of an RFC than a serious patch, because: This version of the test doesn't seem to reproduce the bug. When I run the test against the unfixed version of http-backend, it passes. Ideas? Not about this patch: could this test share some infrustructure with t5560-http-backend-noserver.sh? If there were some common shell library that they shared, the tests might be easier to read and write. Thanks, Jonathan t/t5562-http-backend-content-length.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh index f94d01f69e..fceb3d39c1 100755 --- a/t/t5562-http-backend-content-length.sh +++ b/t/t5562-http-backend-content-length.sh @@ -155,8 +155,8 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' ' test_expect_success 'empty CONTENT_LENGTH' ' env \ - QUERY_STRING=/repo.git/HEAD \ - PATH_TRANSLATED="$PWD"/.git/HEAD \ + QUERY_STRING="/repo.git/info/refs?service=git-receive-pack" \ + PATH_TRANSLATED="$PWD"/.git/info/refs \ GIT_HTTP_EXPORT_ALL=TRUE \ REQUEST_METHOD=GET \ CONTENT_LENGTH="" \
On Sun, Sep 09, 2018 at 10:17:48PM -0700, Jonathan Nieder wrote: > From: Max Kirillov <max@max630.net> > Subject: http-backend test: make empty CONTENT_LENGTH test more realistic Thank you, yes, this is what should have left
Max Kirillov wrote: > On Sun, Sep 09, 2018 at 10:17:48PM -0700, Jonathan Nieder wrote: >> From: Max Kirillov <max@max630.net> >> Subject: http-backend test: make empty CONTENT_LENGTH test more realistic > > Thank you, yes, this is what should have left Oh, tying up this loose end: do you know why the test passes without 574c513e8d (http-backend: allow empty CONTENT_LENGTH, 2018-09-10)?
diff --git a/http-backend.c b/http-backend.c index e88d29f62b..a1230d7ead 100644 --- a/http-backend.c +++ b/http-backend.c @@ -353,7 +353,7 @@ static ssize_t get_content_length(void) ssize_t val = -1; const char *str = getenv("CONTENT_LENGTH"); - if (str && !git_parse_ssize_t(str, &val)) + if (str && *str && !git_parse_ssize_t(str, &val)) die("failed to parse CONTENT_LENGTH: %s", str); return val; } diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh index 057dcb85d6..b28c3c4765 100755 --- a/t/t5562-http-backend-content-length.sh +++ b/t/t5562-http-backend-content-length.sh @@ -152,4 +152,15 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' ' grep "fatal:.*CONTENT_LENGTH" err ' +test_expect_success 'empty CONTENT_LENGTH' ' + env \ + QUERY_STRING="/repo.git/info/refs?service=git-receive-pack" \ + PATH_TRANSLATED="$PWD"/.git/info/refs \ + GIT_HTTP_EXPORT_ALL=TRUE \ + REQUEST_METHOD=GET \ + CONTENT_LENGTH="" \ + git http-backend <empty_body >act.out 2>act.err && + verify_http_result "200 OK" +' + test_done
Before 817f7dc223, CONTENT_LENGTH variable was never considered, http-backend was just reading request body from standard input until EOF when it, or a command started by it, needed it. Then it was discovered that some HTTP do not close standard input, instead expecting CGI scripts to obey CONTENT_LENGTH. In 817f7dc223, behavior was changed to consider the CONTENT_LENGTH variable when it is set. Case of unset CONTENT_LENGTH was kept to mean "read until EOF" which is not compliant to the RFC3875 (which treats it as empty body), but practically is used when client uses chunked encoding to submit big request. Case of empty CONTENT_LENGTH has slept through this conditions. Apparently, it is used for GET requests, and RFC3875 does specify that it also means empty body. Current implementation, however, fails to parse it and aborts the request. Fix the case of empty CONTENT_LENGTH to also be treated as "read until EOF". It does not actually matter what does it mean because body is never read anyway, it just should not cause parse error. Add a test for the case. Reported-By: Jelmer Vernooij <jelmer@jelmer.uk> Signed-off-by: Max Kirillov <max@max630.net> --- Provided more thorough message, also fix test (it did not test actually the error before) There will be more versions later, at least the one which suggested by Jeff PS: did I write v2, it should be v3 of course! http-backend.c | 2 +- t/t5562-http-backend-content-length.sh | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-)