diff --git a/internal/http3/headers.go b/internal/http3/headers.go index 02152742..a66fe1d2 100644 --- a/internal/http3/headers.go +++ b/internal/http3/headers.go @@ -139,6 +139,9 @@ func parseHeaders(decodeFn qpack.DecodeFunc, isRequest bool, sizeLimit int, head if ds.ShouldDump() && len(hdr.Headers) > 0 { ds.DumpResponseHeader([]byte("\r\n")) } + if isRequest && (hdr.Scheme == "http" || hdr.Scheme == "https") && strings.Contains(hdr.Authority, "@") { + return header{}, errors.New("userinfo is not allowed in :authority") + } hdr.ContentLength = -1 if len(contentLengthStr) > 0 { // use ParseUint instead of ParseInt, so that parsing fails on negative values diff --git a/internal/http3/headers_test.go b/internal/http3/headers_test.go index e5facae5..89202e9e 100644 --- a/internal/http3/headers_test.go +++ b/internal/http3/headers_test.go @@ -1,10 +1,36 @@ package http3 import ( + "io" "net/http" "testing" + + "github.com/imroc/req/v3/internal/dump" + "github.com/quic-go/qpack" ) +func TestParseHeadersRejectsUserinfoInAuthority(t *testing.T) { + fields := []qpack.HeaderField{ + {Name: ":method", Value: "GET"}, + {Name: ":scheme", Value: "https"}, + {Name: ":authority", Value: "user@example.com"}, + {Name: ":path", Value: "/"}, + } + decode := func() (qpack.HeaderField, error) { + if len(fields) == 0 { + return qpack.HeaderField{}, io.EOF + } + field := fields[0] + fields = fields[1:] + return field, nil + } + + _, err := parseHeaders(decode, true, 1<<20, nil, dump.Dumpers{}) + if err == nil || err.Error() != "userinfo is not allowed in :authority" { + t.Fatalf("got error %v, want userinfo rejection", err) + } +} + func TestExtractAnnouncedTrailers(t *testing.T) { tests := []struct { name string diff --git a/transfer.go b/transfer.go index 68924d92..28a94c7b 100644 --- a/transfer.go +++ b/transfer.go @@ -827,19 +827,24 @@ var ( doubleCRLF = []byte("\r\n\r\n") ) -func seeUpcomingDoubleCRLF(r *bufio.Reader) bool { +func seeUpcomingDoubleCRLF(r *bufio.Reader) error { for peekSize := 4; ; peekSize++ { // This loop stops when Peek returns an error, // which it does when r's buffer has been filled. buf, err := r.Peek(peekSize) + for i, b := range buf { + if b == '\n' && (i == 0 || buf[i-1] != '\r') { + return errors.New("http: invalid trailer") + } + } if bytes.HasSuffix(buf, doubleCRLF) { - return true + return nil } if err != nil { break } } - return false + return errors.New("http: suspiciously long trailer after chunked body") } var errTrailerEOF = errors.New("http: unexpected EOF reading trailer") @@ -866,8 +871,8 @@ func (b *body) readTrailer() error { // this bufio.textprotoReader. Instead, a hack: we iteratively Peek up // to the bufio.textprotoReader's max size, looking for a double CRLF. // This limits the trailer to the underlying buffer size, typically 4kB. - if !seeUpcomingDoubleCRLF(b.r) { - return errors.New("http: suspiciously long trailer after chunked body") + if err := seeUpcomingDoubleCRLF(b.r); err != nil { + return err } hdr, err := textproto.NewReader(b.r).ReadMIMEHeader() diff --git a/transfer_test.go b/transfer_test.go index b8c203c2..cc258803 100644 --- a/transfer_test.go +++ b/transfer_test.go @@ -2,8 +2,12 @@ package req import ( "bufio" + "fmt" + "io" + "net" "net/http" "strings" + "sync/atomic" "testing" ) @@ -26,3 +30,63 @@ func TestReadTransferClosesResponseWithTransferEncodingAndContentLength(t *testi t.Fatal("response with both Transfer-Encoding and Content-Length did not set Close") } } + +func TestResponseWithBareLFInTrailerClosesConnection(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer ln.Close() + + var connections, requests atomic.Int32 + go func() { + for { + conn, err := ln.Accept() + if err != nil { + return + } + connections.Add(1) + go func() { + defer conn.Close() + r := bufio.NewReader(conn) + for { + req, err := http.ReadRequest(r) + if err != nil { + return + } + req.Body.Close() + if requests.Add(1) == 1 { + fmt.Fprint(conn, "HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\nTrailer: X-One, X-Two\r\n\r\n0\r\nX-One: a\nX-Two: b\r\n\r\n") + } else { + fmt.Fprint(conn, "HTTP/1.1 200 OK\r\nContent-Length: 2\r\n\r\nok") + } + } + }() + } + }() + + tr := NewTransport() + defer tr.CloseIdleConnections() + client := &http.Client{Transport: tr} + resp, err := client.Get("http://" + ln.Addr().String()) + if err != nil { + t.Fatal(err) + } + _, err = io.ReadAll(resp.Body) + resp.Body.Close() + if err == nil || err.Error() != "http: invalid trailer" { + t.Fatalf("got body read error %v, want http: invalid trailer", err) + } + + resp, err = client.Get("http://" + ln.Addr().String()) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + if _, err := io.ReadAll(resp.Body); err != nil { + t.Fatal(err) + } + if got := connections.Load(); got != 2 { + t.Fatalf("got %d connections, want 2; invalid trailer connection was reused", got) + } +}