diff --git a/caddytest/integration/fastcgi_chunked_test.go b/caddytest/integration/fastcgi_chunked_test.go new file mode 100644 index 000000000..b38ff3f4e --- /dev/null +++ b/caddytest/integration/fastcgi_chunked_test.go @@ -0,0 +1,142 @@ +package integration + +import ( + "fmt" + "io" + "net" + "net/http" + "net/http/fcgi" + "os" + "runtime" + "strings" + "testing" + + "github.com/caddyserver/caddy/v2/caddytest" +) + +// TestFastCGIChunkedRequestBody drives the case from caddyserver/caddy#7386: a +// client sending a body with Transfer-Encoding: chunked through a fastcgi +// reverse_proxy over a unix socket. +// +// FastCGI cannot express a body of unknown length — CGI/1.1 requires +// CONTENT_LENGTH, and php-fpm hangs when it is absent or wrong and the body is +// non-empty — so such a body is only forwardable once it has been buffered in +// full. request_buffers is therefore what decides the outcome, and the default +// of 4096 for this transport is far below a git push or a file upload. +func TestFastCGIChunkedRequestBody(t *testing.T) { + if runtime.GOOS == "windows" { + t.SkipNow() + } + + socketName := tempSocketName(t) + + // Report how many body bytes actually arrived, so a truncated body shows up + // as a failure instead of passing quietly. + ln, err := net.Listen("unix", socketName) + if err != nil { + t.Fatalf("failed to listen on the socket: %s", err) + } + go fcgi.Serve(ln, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { //nolint:errcheck + n, _ := io.Copy(io.Discard, r.Body) + fmt.Fprintf(w, "received %d", n) + })) + t.Cleanup(func() { ln.Close() }) + runtime.Gosched() + + const requestBuffers = 4096 + + tester := caddytest.NewTester(t) + tester.InitServer(fmt.Sprintf(` + { + "admin": {"listen": "localhost:2999"}, + "apps": { + "pki": {"certificate_authorities": {"local": {"install_trust": false}}}, + "http": { + "grace_period": 1, + "servers": { + "srv0": { + "listen": [":18080"], + "routes": [ + { + "match": [{"path": ["/limited"]}], + "handle": [{ + "handler": "reverse_proxy", + "request_buffers": %d, + "transport": {"protocol": "fastcgi"}, + "upstreams": [{"dial": "unix/%s"}] + }] + }, + { + "match": [{"path": ["/unlimited"]}], + "handle": [{ + "handler": "reverse_proxy", + "request_buffers": -1, + "transport": {"protocol": "fastcgi"}, + "upstreams": [{"dial": "unix/%s"}] + }] + } + ] + } + } + } + } + } + `, requestBuffers, socketName, socketName), "json") + + tests := []struct { + name string + path string + size int + wantStatus int + }{ + {name: "small body fits the buffer", path: "/limited", size: 16, wantStatus: http.StatusOK}, + {name: "one below the buffer", path: "/limited", size: requestBuffers - 1, wantStatus: http.StatusOK}, + // The body is buffered up to the limit and no further, so its length + // stays unknown and FastCGI has nothing to put in CONTENT_LENGTH. + {name: "exactly the buffer", path: "/limited", size: requestBuffers, wantStatus: http.StatusLengthRequired}, + {name: "above the buffer", path: "/limited", size: requestBuffers * 4, wantStatus: http.StatusLengthRequired}, + // Unlimited buffering is the remedy: the whole body is read, so its + // length is known and the request goes through at any size. + {name: "above the buffer, buffering unlimited", path: "/unlimited", size: requestBuffers * 4, wantStatus: http.StatusOK}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + body := strings.Repeat("x", tc.size) + // A strings.Reader would let net/http size the body; wrapping it in + // a bare io.Reader is what makes the request chunked. + req, err := http.NewRequest(http.MethodPost, "http://localhost:18080"+tc.path, struct{ io.Reader }{strings.NewReader(body)}) + if err != nil { + t.Fatalf("building request: %s", err) + } + + resp := tester.AssertResponseCode(req, tc.wantStatus) + defer resp.Body.Close() + if tc.wantStatus != http.StatusOK { + return + } + got, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("reading response: %s", err) + } + // Every byte has to reach the upstream, not just a buffer's worth. + if want := fmt.Sprintf("received %d", tc.size); string(got) != want { + t.Errorf("upstream got %q, want %q", got, want) + } + }) + } +} + +// tempSocketName borrows a name inside a valid path to bind a unix socket on. +func tempSocketName(t *testing.T) string { + t.Helper() + f, err := os.CreateTemp("", "*.sock") + if err != nil { + t.Fatalf("failed to create TempFile: %s", err) + } + name := f.Name() + f.Close() + os.Remove(name) + t.Cleanup(func() { os.Remove(name) }) + return name +} diff --git a/modules/caddyhttp/reverseproxy/buffering_test.go b/modules/caddyhttp/reverseproxy/buffering_test.go index 68514814c..6d6e4fe48 100644 --- a/modules/caddyhttp/reverseproxy/buffering_test.go +++ b/modules/caddyhttp/reverseproxy/buffering_test.go @@ -2,6 +2,7 @@ package reverseproxy import ( "io" + "strings" "testing" ) @@ -82,3 +83,41 @@ func TestBuffering(t *testing.T) { }) } } + +// TestBufferingBoundary documents where a buffered body stops being fully +// buffered, which is what decides whether the caller can learn its length and +// set Content-Length. A body the size of the limit counts as partial: io.CopyN +// filling the buffer cannot be told apart from a body with more to come, and +// finding out would mean a blocking read on a stream that may never deliver. +func TestBufferingBoundary(t *testing.T) { + const body = "0123456789" + + for limit := int64(1); limit <= int64(len(body))+2; limit++ { + var h Handler + res, read := h.bufferedBody(io.NopCloser(strings.NewReader(body)), limit) + + // Whatever the limit, the upstream must still receive every byte. + got, err := io.ReadAll(res) + if err != nil { + t.Fatalf("limit %d: reading buffered body: %v", limit, err) + } + if string(got) != body { + t.Errorf("limit %d: body changed: got %q, want %q", limit, got, body) + } + if err := res.Close(); err != nil { + t.Errorf("limit %d: closing buffered body: %v", limit, err) + } + + brc, ok := res.(bodyReadCloser) + if !ok { + t.Fatalf("limit %d: expected a bodyReadCloser", limit) + } + wantFull := limit > int64(len(body)) + if gotFull := brc.body == nil; gotFull != wantFull { + t.Errorf("limit %d: fully buffered = %v, want %v", limit, gotFull, wantFull) + } + if wantFull && read != int64(len(body)) { + t.Errorf("limit %d: read %d bytes, want %d", limit, read, len(body)) + } + } +} diff --git a/modules/caddyhttp/reverseproxy/fastcgi/client.go b/modules/caddyhttp/reverseproxy/fastcgi/client.go index 7811ae234..e8ebf836a 100644 --- a/modules/caddyhttp/reverseproxy/fastcgi/client.go +++ b/modules/caddyhttp/reverseproxy/fastcgi/client.go @@ -26,6 +26,8 @@ package fastcgi import ( "bufio" "bytes" + "errors" + "fmt" "io" "mime/multipart" "net" @@ -125,6 +127,15 @@ const ( // not synchronized because we don't care what the contents are var pad [maxPad]byte +// FastCGI has no way to express a body of unknown length: CONTENT_LENGTH is +// required by CGI/1.1, and php-fpm hangs when it is absent or wrong and the +// body is not empty. A request that arrives without a length therefore has to +// be buffered in full before it can be forwarded. +var ( + errNoContentLength = errors.New("request body has no known length; increase request_buffers so the body can be buffered in full to determine it") + errBadContentLength = errors.New("request body length is not a valid CONTENT_LENGTH") +) + // client implements a FastCGI client, which is a standard for // interfacing external applications with Web servers. type client struct { @@ -138,13 +149,19 @@ type client struct { // Do makes the request and returns an io.Reader that translates the data read // from the FastCGI responder out of FastCGI packets before returning it. func (c *client) Do(p map[string]string, req io.Reader) (r io.Reader, err error) { - // check for CONTENT_LENGTH, since the lack of it or wrong value will cause the backend to hang - if clStr, ok := p["CONTENT_LENGTH"]; !ok { - return nil, caddyhttp.Error(http.StatusLengthRequired, nil) - } else if _, err := strconv.ParseUint(clStr, 10, 64); err != nil { - // stdlib won't return a negative Content-Length, but we check just in case, - // the most likely cause is from a missing content length, which is -1 - return nil, caddyhttp.Error(http.StatusLengthRequired, err) + // check for CONTENT_LENGTH, since the lack of it or wrong value will cause the backend to hang. + // A request with no length of its own (Transfer-Encoding: chunked, or an HTTP/2 or HTTP/3 + // request sent without the header) only acquires one by being buffered in full, so say so: + // the bare status alone has sent people looking for the fault in their backend. + if clStr, ok := p["CONTENT_LENGTH"]; !ok || clStr == "" { + return nil, caddyhttp.Error(http.StatusLengthRequired, errNoContentLength) + } else if l, err := strconv.ParseInt(clStr, 10, 64); err != nil { + return nil, caddyhttp.Error(http.StatusLengthRequired, fmt.Errorf("%w: %w", errBadContentLength, err)) + } else if l < 0 { + // net/http reports -1 for a body whose length it does not know, which is + // what a chunked request looks like by the time it reaches here. This is + // the case operators actually hit, so it gets the actionable message. + return nil, caddyhttp.Error(http.StatusLengthRequired, errNoContentLength) } writer := &streamWriter{c: c} diff --git a/modules/caddyhttp/reverseproxy/fastcgi/contentlength_test.go b/modules/caddyhttp/reverseproxy/fastcgi/contentlength_test.go new file mode 100644 index 000000000..7d871940a --- /dev/null +++ b/modules/caddyhttp/reverseproxy/fastcgi/contentlength_test.go @@ -0,0 +1,134 @@ +// Copyright 2015 Matthew Holt and The Caddy Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package fastcgi + +import ( + "errors" + "net/http" + "strings" + "testing" + + "github.com/caddyserver/caddy/v2/modules/caddyhttp" +) + +// TestDoRejectsUnusableContentLength pins what the client does with a +// CONTENT_LENGTH it cannot use, and which of those cases carries which message. +// +// Only "-1" is reachable through Transport.RoundTrip, which is the shape a +// chunked request takes: Get/Head/Options/Post all set CONTENT_LENGTH before +// Do sees it, from the r.ContentLength that net/http reports as -1 when it has +// no length. The absent and empty cases guard Do's own contract for any future +// caller; an absent key was the one that used to return a 411 with no error at +// all, while an empty value carried strconv's syntax error, true but useless +// to an operator. TestPostRejectsUnknownLengthWithRemedy covers the reachable +// path end to end. +func TestDoRejectsUnusableContentLength(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + env map[string]string + wantErr error + }{ + { + name: "absent", + env: map[string]string{}, + wantErr: errNoContentLength, + }, + { + name: "empty", + env: map[string]string{"CONTENT_LENGTH": ""}, + wantErr: errNoContentLength, + }, + { + // The reported case: what a chunked request becomes by the time it + // reaches here, and the only one production can produce. + name: "negative, as a chunked request arrives", + env: map[string]string{"CONTENT_LENGTH": "-1"}, + wantErr: errNoContentLength, + }, + { + name: "not a number", + env: map[string]string{"CONTENT_LENGTH": "banana"}, + wantErr: errBadContentLength, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + c := &client{} + _, err := c.Do(tc.env, strings.NewReader("body")) + if err == nil { + t.Fatal("expected an error") + } + + var handlerErr caddyhttp.HandlerError + if !errors.As(err, &handlerErr) { + t.Fatalf("error %v is not a caddyhttp.HandlerError", err) + } + if handlerErr.StatusCode != http.StatusLengthRequired { + t.Errorf("status = %d, want %d", handlerErr.StatusCode, http.StatusLengthRequired) + } + // The status alone is what sent people hunting through their + // backend logs, so the cause has to travel with it. + if handlerErr.Err == nil { + t.Fatal("the 411 carries no error to explain it") + } + if !errors.Is(err, tc.wantErr) { + t.Errorf("error %v does not wrap %v", err, tc.wantErr) + } + }) + } +} + +// TestNoContentLengthErrorNamesTheRemedy keeps the message actionable: the +// operator has to learn that buffering is what supplies the missing length. +func TestNoContentLengthErrorNamesTheRemedy(t *testing.T) { + t.Parallel() + + if !strings.Contains(errNoContentLength.Error(), "request_buffers") { + t.Errorf("error %q does not name request_buffers as the remedy", errNoContentLength) + } +} + +// TestPostRejectsUnknownLengthWithRemedy goes through the entry point RoundTrip +// actually uses. Transport.RoundTrip passes r.ContentLength straight to Post, +// and net/http reports -1 for a chunked body, so this is the exact call the +// reporter of #7386 made. Asserting it only against Do would let the message +// rot on a branch production never reaches. +func TestPostRejectsUnknownLengthWithRemedy(t *testing.T) { + t.Parallel() + + c := &client{} + env := map[string]string{} + + _, err := c.Post(env, http.MethodPost, "application/octet-stream", strings.NewReader("body"), -1) + if err == nil { + t.Fatal("expected an error") + } + if !errors.Is(err, errNoContentLength) { + t.Errorf("error %v does not wrap errNoContentLength", err) + } + if !strings.Contains(err.Error(), "request_buffers") { + t.Errorf("the error operators see, %q, does not name the remedy", err) + } + + var handlerErr caddyhttp.HandlerError + if !errors.As(err, &handlerErr) || handlerErr.StatusCode != http.StatusLengthRequired { + t.Errorf("error %v is not a 411 HandlerError", err) + } +}