| Message ID | 20240612115028.1169183-4-cmn@dwim.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Report rejections over HTTP when the remote rejects during the transfer | expand |
On Wed, Jun 12, 2024 at 01:50:27PM +0200, Carlos Martín Nieto wrote: > This rejection requires us to make sure we handle this kind of error > correctly rather than throw away the report in remote-curl and end up > with "Everything up-to-date" due to the lack of report. > > Signed-off-by: Carlos Martín Nieto <cmn@dwim.me> > --- > t/t5546-receive-limits.sh | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) I haven't looked at the CGI stuff to know whether or not this behavior can be reasonably emulated without the new Python script you wrote, but a couple of small things in the meantime... This patch's subject line states that it is modifying script t5541, but the patch modifies t5546. I think the latter is correct, and there is just a typo in the subject line, but wanted to make sure I pointed it out regardless. More importantly, this test fails after applying through this patch, but not 4/4. After applying the final patch, it looks like it is still failing for me. I figure that I am probably holding it wrong, but regardless, here is the error message I see on my machine: --- 8< --- + git init /home/ttaylorr/src/git/t/trash directory.t5546-receive-limits/httpd/www/error_too_large Initialized empty Git repository in /home/ttaylorr/src/git/t/trash directory.t5546-receive-limits/httpd/www/error_too_large/.git/ + git -C /home/ttaylorr/src/git/t/trash directory.t5546-receive-limits/httpd/www/error_too_large config receive.maxInputSize 128 + test-tool genrandom foo 10485760 + git add large-file + test_commit large-file + local notick= + local echo=echo + local append= + local author= + local signoff= + local indir= + local tag=light + test 1 != 0 + break + indir= + local file=large-file.t + test -n + echo large-file + git add -- large-file.t + test -z + test_tick + test -z set + test_tick=1112912053 + GIT_COMMITTER_DATE=1112912053 -0700 + GIT_AUTHOR_DATE=1112912053 -0700 + export GIT_COMMITTER_DATE GIT_AUTHOR_DATE + git commit -m large-file [master 03a3078] large-file Author: A U Thor <author@example.com> 2 files changed, 1 insertion(+) create mode 100644 large-file create mode 100644 large-file.t + git tag large-file + test_must_fail git push --porcelain http://127.0.0.1:5546/error_too_large HEAD:refs/tags/will-fail + _test_ok= + test_must_fail_acceptable git push --porcelain http://127.0.0.1:5546/error_too_large HEAD:refs/tags/will-fail + test git = env + return 0 + git push --porcelain http://127.0.0.1:5546/error_too_large HEAD:refs/tags/will-fail Enumerating objects: 8, done. Counting objects: 100% (8/8), done. Delta compression using up to 20 threads Compressing objects: 100% (6/6), done. error: RPC failed; curl 55 Send failure: Broken pipe send-pack: unexpected disconnect while reading sideband packet Writing objects: 100% (8/8), 10.00 MiB | 34.04 MiB/s, done. Total 8 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0) fatal: the remote end hung up unexpectedly fatal: the remote end hung up unexpectedly error: failed to push some refs to 'http://127.0.0.1:5546/error_too_large' + exit_code=1 + test 1 -eq 0 + test_match_signal 13 1 + test 1 = 141 + test 1 = 269 + return 1 + test 1 -gt 129 + test 1 -eq 127 + test 1 -eq 126 + return 0 + test_must_fail git -C /home/ttaylorr/src/git/t/trash directory.t5546-receive-limits/httpd/www/error_too_large rev-parse --verify refs/tags/will-fail + _test_ok= + test_must_fail_acceptable git -C /home/ttaylorr/src/git/t/trash directory.t5546-receive-limits/httpd/www/error_too_large rev-parse --verify refs/tags/will-fail + test git = env + return 0 + git -C /home/ttaylorr/src/git/t/trash directory.t5546-receive-limits/httpd/www/error_too_large rev-parse --verify refs/tags/will-fail fatal: Needed a single revision + exit_code=128 + test 128 -eq 0 + test_match_signal 13 128 + test 128 = 141 + test 128 = 269 + return 1 + test 128 -gt 129 + test 128 -eq 127 + test 128 -eq 126 + return 0 + cat + test_cmp expect actual + test 2 -ne 2 + eval diff -u "$@" + diff -u expect actual --- expect 2024-06-12 21:48:50.005929827 +0000 +++ actual 2024-06-12 21:48:49.677930723 +0000 @@ -1,3 +0,0 @@ -To http://127.0.0.1:5546/error_too_large -! HEAD:refs/tags/will-fail [remote rejected] (unpacker error) -Done error: last command exited with $?=1 not ok 18 - reject too-large push over HTTP --- >8 --- Thanks, Taylor
On Wed, Jun 12, 2024 at 01:50:27PM +0200, Carlos Martín Nieto wrote: > diff --git a/t/t5546-receive-limits.sh b/t/t5546-receive-limits.sh > index 9fc9ba552f1..ccbdf3945ab 100755 > --- a/t/t5546-receive-limits.sh > +++ b/t/t5546-receive-limits.sh > @@ -5,6 +5,11 @@ test_description='check receive input limits' > TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > + > +ROOT_PATH="$PWD" I don't think this ROOT_PATH is doing anything? > +. "$TEST_DIRECTORY"/lib-httpd.sh > +start_serve_git Since you're adding to an existing script, these should go near the bottom (or possibly even in their own script). If we don't have apache or curl support, then loading lib-httpd.sh at all will cause us to bail from the test script immediately. So we'll never run these existing tests at all on such platforms, even though we could (and do now). -Peff
On Wed, Jun 12, 2024 at 01:50:27PM +0200, Carlos Martín Nieto wrote: > +test_expect_success 'reject too-large push over HTTP' ' > + git init "$HTTPD_DOCUMENT_ROOT_PATH/error_too_large" && > + git -C "$HTTPD_DOCUMENT_ROOT_PATH/error_too_large" config receive.maxInputSize 128 && > + test-tool genrandom foo $((10*1024*1024)) >large-file && > + git add large-file && > + test_commit large-file && > + test_must_fail git push --porcelain \ > + $GIT_SERVE_URL/error_too_large \ > + HEAD:refs/tags/will-fail >actual && > + test_must_fail git -C "$HTTPD_DOCUMENT_ROOT_PATH/error_too_large" \ > + rev-parse --verify refs/tags/will-fail && > + cat >expect <<-EOF && > + To $GIT_SERVE_URL/error_too_large > + ! HEAD:refs/tags/will-fail [remote rejected] (unpacker error) > + Done > + EOF > + test_cmp expect actual > +' This test fails for me (even with the fix in the next patch) with: Exception occurred during processing of request from ('127.0.0.1', 47480) Traceback (most recent call last): File "/usr/lib/python3.11/socketserver.py", line 317, in _handle_request_noblock self.process_request(request, client_address) File "/usr/lib/python3.11/socketserver.py", line 348, in process_request self.finish_request(request, client_address) File "/usr/lib/python3.11/socketserver.py", line 361, in finish_request self.RequestHandlerClass(request, client_address, self) File "/home/peff/compile/git/t/lib-httpd/serve-git.py", line 35, in __init__ super().__init__(*args, **kwargs) File "/usr/lib/python3.11/socketserver.py", line 755, in __init__ self.handle() File "/usr/lib/python3.11/http/server.py", line 436, in handle self.handle_one_request() File "/usr/lib/python3.11/http/server.py", line 424, in handle_one_request method() File "/home/peff/compile/git/t/lib-httpd/serve-git.py", line 117, in do_GET self.do_receive_pack(responder, gitdir, True, protocol) File "/home/peff/compile/git/t/lib-httpd/serve-git.py", line 243, in do_receive_pack self._run_command(resp, 'receive-pack', gitdir, advertisement, protocol) File "/home/peff/compile/git/t/lib-httpd/serve-git.py", line 192, in _run_command with subprocess.Popen(argv, stdin=stdin, stdout=subprocess.PIPE, cwd=gitdir, env=env) as proc: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3.11/subprocess.py", line 1026, in __init__ self._execute_child(args, executable, preexec_fn, close_fds, File "/usr/lib/python3.11/subprocess.py", line 1955, in _execute_child raise child_exception_type(errno_num, err_msg, err_filename) FileNotFoundError: [Errno 2] No such file or directory: 'git' which seems...odd. It should be finding "git" in bin-wrappers/, but I think it is using a restricted/vanilla path. If I put "git" into /usr/bin, it stops complaining (but obviously this is very wrong, as we are not running the Git we're trying to test!). Ah, I think I see it. You set up an environment for the Popen like: env = {k:v for k, v in os.environ.items() if k.startswith('GIT_')} if protocol is not None: env['GIT_PROTOCOL'] = protocol so it does not contain $PATH (nor other possibly useful things!), so presumably a fallback $PATH is used. I think you'd want to start with "env = os.environ.copy()" and then modify it from there. -Peff
diff --git a/t/t5546-receive-limits.sh b/t/t5546-receive-limits.sh index 9fc9ba552f1..ccbdf3945ab 100755 --- a/t/t5546-receive-limits.sh +++ b/t/t5546-receive-limits.sh @@ -5,6 +5,11 @@ test_description='check receive input limits' TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh + +ROOT_PATH="$PWD" +. "$TEST_DIRECTORY"/lib-httpd.sh +start_serve_git + # Let's run tests with different unpack limits: 1 and 10000 # When the limit is 1, `git receive-pack` will call `git index-pack`. # When the limit is 10000, `git receive-pack` will call `git unpack-objects`. @@ -83,4 +88,23 @@ test_expect_success "create known-size (1024 bytes) commit" ' test_pack_input_limit index test_pack_input_limit unpack +test_expect_success 'reject too-large push over HTTP' ' + git init "$HTTPD_DOCUMENT_ROOT_PATH/error_too_large" && + git -C "$HTTPD_DOCUMENT_ROOT_PATH/error_too_large" config receive.maxInputSize 128 && + test-tool genrandom foo $((10*1024*1024)) >large-file && + git add large-file && + test_commit large-file && + test_must_fail git push --porcelain \ + $GIT_SERVE_URL/error_too_large \ + HEAD:refs/tags/will-fail >actual && + test_must_fail git -C "$HTTPD_DOCUMENT_ROOT_PATH/error_too_large" \ + rev-parse --verify refs/tags/will-fail && + cat >expect <<-EOF && + To $GIT_SERVE_URL/error_too_large + ! HEAD:refs/tags/will-fail [remote rejected] (unpacker error) + Done + EOF + test_cmp expect actual +' + test_done
This rejection requires us to make sure we handle this kind of error correctly rather than throw away the report in remote-curl and end up with "Everything up-to-date" due to the lack of report. Signed-off-by: Carlos Martín Nieto <cmn@dwim.me> --- t/t5546-receive-limits.sh | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)