diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index dd75520d..3c833fe7 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -16,7 +16,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v2 with: - go-version: 1.18 + go-version: 1.19 - name: Build run: go build -v ./... diff --git a/.gitignore b/.gitignore new file mode 100644 index 00000000..53cd92eb --- /dev/null +++ b/.gitignore @@ -0,0 +1 @@ +/upstreamrepo diff --git a/README.md b/README.md index 963b0737..f7bd850b 100644 --- a/README.md +++ b/README.md @@ -103,6 +103,9 @@ type TLSConn interface { // HandshakeContext performs an TLS handshake bounded // in time by the given context. HandshakeContext(ctx context.Context) error + + // NetConn returns the underlying net.Conn + NetConn() net.Conn } ``` @@ -219,20 +222,8 @@ minor changes (e.g., updating docs) directly on the `main` branch. (Adapted from refraction-networking/utls instructions.) -- [ ] run the following commands: - -```bash -set -ex -git checkout main -git remote add golang git@github.com:golang/go.git || git fetch golang -git branch -D golang-upstream golang-http-upstream merged-main || true -git fetch golang -git checkout -b golang-upstream go1.18.9 -git subtree split -P src/net/http/ -b golang-http-upstream -git checkout main -git checkout -b merged-main -git merge golang-http-upstream -``` +- [ ] update [UPSTREAM](UPSTREAM), commit the change, and then +run the `./tools/merge.bash` script to merge from upstream; - [ ] make sure you synch [./internal/safefilepath](./internal/safefilepath) with the `./src/internal/safefilepath` of the Go release you're merging from; diff --git a/UPSTREAM b/UPSTREAM new file mode 100644 index 00000000..7ceb20fb --- /dev/null +++ b/UPSTREAM @@ -0,0 +1 @@ +go1.19.4 diff --git a/cgi/host.go b/cgi/host.go index 23afd156..9058c55e 100644 --- a/cgi/host.go +++ b/cgi/host.go @@ -90,10 +90,11 @@ func (h *Handler) stderr() io.Writer { // removeLeadingDuplicates remove leading duplicate in environments. // It's possible to override environment like following. -// cgi.Handler{ -// ... -// Env: []string{"SCRIPT_FILENAME=foo.php"}, -// } +// +// cgi.Handler{ +// ... +// Env: []string{"SCRIPT_FILENAME=foo.php"}, +// } func removeLeadingDuplicates(env []string) (ret []string) { for i, e := range env { found := false diff --git a/client.go b/client.go index 1f69651c..0abcd7f7 100644 --- a/client.go +++ b/client.go @@ -55,7 +55,6 @@ import ( // with the expectation that the Jar will insert those mutated cookies // with the updated values (assuming the origin matches). // If Jar is nil, the initial cookies are forwarded without change. -// type Client struct { // Transport specifies the mechanism by which individual // HTTP requests are made. @@ -425,11 +424,11 @@ func basicAuth(username, password string) string { // the following redirect codes, Get follows the redirect, up to a // maximum of 10 redirects: // -// 301 (Moved Permanently) -// 302 (Found) -// 303 (See Other) -// 307 (Temporary Redirect) -// 308 (Permanent Redirect) +// 301 (Moved Permanently) +// 302 (Found) +// 303 (See Other) +// 307 (Temporary Redirect) +// 308 (Permanent Redirect) // // An error is returned if there were too many redirects or if there // was an HTTP protocol error. A non-2xx response doesn't cause an @@ -454,11 +453,11 @@ func Get(url string) (resp *Response, err error) { // following redirect codes, Get follows the redirect after calling the // Client's CheckRedirect function: // -// 301 (Moved Permanently) -// 302 (Found) -// 303 (See Other) -// 307 (Temporary Redirect) -// 308 (Permanent Redirect) +// 301 (Moved Permanently) +// 302 (Found) +// 303 (See Other) +// 307 (Temporary Redirect) +// 308 (Permanent Redirect) // // An error is returned if the Client's CheckRedirect function fails // or if there was an HTTP protocol error. A non-2xx response doesn't @@ -520,17 +519,6 @@ func redirectBehavior(reqMethod string, resp *Response, ireq *Request) (redirect shouldRedirect = true includeBody = true - // Treat 307 and 308 specially, since they're new in - // Go 1.8, and they also require re-sending the request body. - if resp.Header.Get("Location") == "" { - // 308s have been observed in the wild being served - // without Location headers. Since Go 1.7 and earlier - // didn't follow these codes, just stop here instead - // of returning an error. - // See Issue 17773. - shouldRedirect = false - break - } if ireq.GetBody == nil && ireq.outgoingLength() != 0 { // We had a request body, and 307/308 require // re-sending it, but GetBody is not defined. So just @@ -642,8 +630,10 @@ func (c *Client) do(req *Request) (retres *Response, reterr error) { if len(reqs) > 0 { loc := resp.Header.Get("Location") if loc == "" { - resp.closeBody() - return nil, uerr(fmt.Errorf("%d response missing Location header", resp.StatusCode)) + // While most 3xx responses include a Location, it is not + // required and 3xx responses without a Location have been + // observed in the wild. See issues #17773 and #49281. + return resp, nil } u, err := req.URL.Parse(loc) if err != nil { @@ -901,13 +891,13 @@ func (c *Client) PostForm(url string, data url.Values) (resp *Response, err erro // the following redirect codes, Head follows the redirect, up to a // maximum of 10 redirects: // -// 301 (Moved Permanently) -// 302 (Found) -// 303 (See Other) -// 307 (Temporary Redirect) -// 308 (Permanent Redirect) +// 301 (Moved Permanently) +// 302 (Found) +// 303 (See Other) +// 307 (Temporary Redirect) +// 308 (Permanent Redirect) // -// Head is a wrapper around DefaultClient.Head +// Head is a wrapper around DefaultClient.Head. // // To make a request with a specified context.Context, use NewRequestWithContext // and DefaultClient.Do. @@ -919,11 +909,11 @@ func Head(url string) (resp *Response, err error) { // following redirect codes, Head follows the redirect after calling the // Client's CheckRedirect function: // -// 301 (Moved Permanently) -// 302 (Found) -// 303 (See Other) -// 307 (Temporary Redirect) -// 308 (Permanent Redirect) +// 301 (Moved Permanently) +// 302 (Found) +// 303 (See Other) +// 307 (Temporary Redirect) +// 308 (Permanent Redirect) // // To make a request with a specified context.Context, use NewRequestWithContext // and Client.Do. @@ -952,9 +942,9 @@ func (c *Client) CloseIdleConnections() { } // cancelTimerBody is an io.ReadCloser that wraps rc with two features: -// 1) On Read error or close, the stop func is called. -// 2) On Read failure, if reqDidTimeout is true, the error is wrapped and -// marked as net.Error that hit its timeout. +// 1. On Read error or close, the stop func is called. +// 2. On Read failure, if reqDidTimeout is true, the error is wrapped and +// marked as net.Error that hit its timeout. type cancelTimerBody struct { stop func() // stops the time.Timer waiting to cancel the request rc io.ReadCloser diff --git a/client_test.go b/client_test.go index 718e91b2..4a55f2cd 100644 --- a/client_test.go +++ b/client_test.go @@ -530,27 +530,31 @@ func TestClientRedirectUseResponse(t *testing.T) { } } -// Issue 17773: don't follow a 308 (or 307) if the response doesn't +// Issues 17773 and 49281: don't follow a 3xx if the response doesn't // have a Location header. -func TestClientRedirect308NoLocation(t *testing.T) { - setParallel(t) - defer afterTest(t) - ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) { - w.Header().Set("Foo", "Bar") - w.WriteHeader(308) - })) - defer ts.Close() - c := ts.Client() - res, err := c.Get(ts.URL) - if err != nil { - t.Fatal(err) - } - res.Body.Close() - if res.StatusCode != 308 { - t.Errorf("status = %d; want %d", res.StatusCode, 308) - } - if got := res.Header.Get("Foo"); got != "Bar" { - t.Errorf("Foo header = %q; want Bar", got) +func TestClientRedirectNoLocation(t *testing.T) { + for _, code := range []int{301, 308} { + t.Run(fmt.Sprint(code), func(t *testing.T) { + setParallel(t) + defer afterTest(t) + ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) { + w.Header().Set("Foo", "Bar") + w.WriteHeader(code) + })) + defer ts.Close() + c := ts.Client() + res, err := c.Get(ts.URL) + if err != nil { + t.Fatal(err) + } + res.Body.Close() + if res.StatusCode != code { + t.Errorf("status = %d; want %d", res.StatusCode, code) + } + if got := res.Header.Get("Foo"); got != "Bar" { + t.Errorf("Foo header = %q; want Bar", got) + } + }) } } diff --git a/clientserver_test.go b/clientserver_test.go index bfa218a5..33490e0e 100644 --- a/clientserver_test.go +++ b/clientserver_test.go @@ -9,6 +9,7 @@ package http_test import ( "bytes" "compress/gzip" + "context" "crypto/rand" "crypto/sha1" "crypto/tls" @@ -17,6 +18,7 @@ import ( "io" "log" "net" + "net/textproto" "net/url" "os" "reflect" @@ -30,6 +32,7 @@ import ( . "github.com/ooni/oohttp" "github.com/ooni/oohttp/httptest" + "github.com/ooni/oohttp/httptrace" "github.com/ooni/oohttp/httputil" ) @@ -1617,3 +1620,92 @@ func testIdentityTransferEncoding(t *testing.T, h2 bool) { t.Errorf("got response body = %q; want %q", got, want) } } + +func TestEarlyHintsRequest_h1(t *testing.T) { testEarlyHintsRequest(t, h1Mode) } +func TestEarlyHintsRequest_h2(t *testing.T) { testEarlyHintsRequest(t, h2Mode) } +func testEarlyHintsRequest(t *testing.T, h2 bool) { + defer afterTest(t) + + var wg sync.WaitGroup + wg.Add(1) + cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) { + h := w.Header() + + h.Add("Content-Length", "123") // must be ignored + h.Add("Link", "; rel=preload; as=style") + h.Add("Link", "; rel=preload; as=script") + w.WriteHeader(StatusEarlyHints) + + wg.Wait() + + h.Add("Link", "; rel=preload; as=script") + w.WriteHeader(StatusEarlyHints) + + w.Write([]byte("Hello")) + })) + defer cst.close() + + checkLinkHeaders := func(t *testing.T, expected, got []string) { + t.Helper() + + if len(expected) != len(got) { + t.Errorf("got %d expected %d", len(got), len(expected)) + } + + for i := range expected { + if expected[i] != got[i] { + t.Errorf("got %q expected %q", got[i], expected[i]) + } + } + } + + checkExcludedHeaders := func(t *testing.T, header textproto.MIMEHeader) { + t.Helper() + + for _, h := range []string{"Content-Length", "Transfer-Encoding"} { + if v, ok := header[h]; ok { + t.Errorf("%s is %q; must not be sent", h, v) + } + } + } + + var respCounter uint8 + trace := &httptrace.ClientTrace{ + Got1xxResponse: func(code int, header textproto.MIMEHeader) error { + switch respCounter { + case 0: + checkLinkHeaders(t, []string{"; rel=preload; as=style", "; rel=preload; as=script"}, header["Link"]) + checkExcludedHeaders(t, header) + + wg.Done() + case 1: + checkLinkHeaders(t, []string{"; rel=preload; as=style", "; rel=preload; as=script", "; rel=preload; as=script"}, header["Link"]) + checkExcludedHeaders(t, header) + + default: + t.Error("Unexpected 1xx response") + } + + respCounter++ + + return nil + }, + } + req, _ := NewRequestWithContext(httptrace.WithClientTrace(context.Background(), trace), "GET", cst.ts.URL, nil) + + res, err := cst.c.Do(req) + if err != nil { + t.Fatal(err) + } + defer res.Body.Close() + + checkLinkHeaders(t, []string{"; rel=preload; as=style", "; rel=preload; as=script", "; rel=preload; as=script"}, res.Header["Link"]) + if cl := res.Header.Get("Content-Length"); cl != "123" { + t.Errorf("Content-Length is %q; want 123", cl) + } + + body, _ := io.ReadAll(res.Body) + if string(body) != "Hello" { + t.Errorf("Read body %q; want Hello", body) + } +} diff --git a/cookie.go b/cookie.go index 8f17247c..42b7823b 100644 --- a/cookie.go +++ b/cookie.go @@ -388,11 +388,13 @@ func sanitizeCookieName(n string) string { // sanitizeCookieValue produces a suitable cookie-value from v. // https://tools.ietf.org/html/rfc6265#section-4.1.1 -// cookie-value = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE ) -// cookie-octet = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E -// ; US-ASCII characters excluding CTLs, -// ; whitespace DQUOTE, comma, semicolon, -// ; and backslash +// +// cookie-value = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE ) +// cookie-octet = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E +// ; US-ASCII characters excluding CTLs, +// ; whitespace DQUOTE, comma, semicolon, +// ; and backslash +// // We loosen this as spaces and commas are common in cookie values // but we produce a quoted cookie-value if and only if v contains // commas or spaces. diff --git a/cookiejar/jar.go b/cookiejar/jar.go index af38bcc7..26370a8f 100644 --- a/cookiejar/jar.go +++ b/cookiejar/jar.go @@ -20,9 +20,9 @@ import ( ) // PublicSuffixList provides the public suffix of a domain. For example: -// - the public suffix of "example.com" is "com", -// - the public suffix of "foo1.foo2.foo3.co.uk" is "co.uk", and -// - the public suffix of "bar.pvt.k12.ma.us" is "pvt.k12.ma.us". +// - the public suffix of "example.com" is "com", +// - the public suffix of "foo1.foo2.foo3.co.uk" is "co.uk", and +// - the public suffix of "bar.pvt.k12.ma.us" is "pvt.k12.ma.us". // // Implementations of PublicSuffixList must be safe for concurrent use by // multiple goroutines. @@ -122,7 +122,9 @@ func (e *entry) shouldSend(https bool, host, path string) bool { return e.domainMatch(host) && e.pathMatch(path) && (https || !e.Secure) } -// domainMatch implements "domain-match" of RFC 6265 section 5.1.3. +// domainMatch checks whether e's Domain allows sending e back to host. +// It differs from "domain-match" of RFC 6265 section 5.1.3 because we treat +// a cookie with an IP address in the Domain always as a host cookie. func (e *entry) domainMatch(host string) bool { if e.Domain == host { return true @@ -304,10 +306,8 @@ func canonicalHost(host string) (string, error) { return "", err } } - if strings.HasSuffix(host, ".") { - // Strip trailing dot from fully qualified domain names. - host = host[:len(host)-1] - } + // Strip trailing dot from fully qualified domain names. + host = strings.TrimSuffix(host, ".") encoded, err := toASCII(host) if err != nil { return "", err @@ -458,10 +458,36 @@ func (j *Jar) domainAndType(host, domain string) (string, bool, error) { } if isIP(host) { - // According to RFC 6265 domain-matching includes not being - // an IP address. - // TODO: This might be relaxed as in common browsers. - return "", false, errNoHostname + // RFC 6265 is not super clear here, a sensible interpretation + // is that cookies with an IP address in the domain-attribute + // are allowed. + + // RFC 6265 section 5.2.3 mandates to strip an optional leading + // dot in the domain-attribute before processing the cookie. + // + // Most browsers don't do that for IP addresses, only curl + // version 7.54) and and IE (version 11) do not reject a + // Set-Cookie: a=1; domain=.127.0.0.1 + // This leading dot is optional and serves only as hint for + // humans to indicate that a cookie with "domain=.bbc.co.uk" + // would be sent to every subdomain of bbc.co.uk. + // It just doesn't make sense on IP addresses. + // The other processing and validation steps in RFC 6265 just + // collaps to: + if host != domain { + return "", false, errIllegalDomain + } + + // According to RFC 6265 such cookies should be treated as + // domain cookies. + // As there are no subdomains of an IP address the treatment + // according to RFC 6265 would be exactly the same as that of + // a host-only cookie. Contemporary browsers (and curl) do + // allows such cookies but treat them as host-only cookies. + // So do we as it just doesn't make sense to label them as + // domain cookies when there is no domain; the whole notion of + // domain cookies requires a domain name to be well defined. + return host, true, nil } // From here on: If the cookie is valid, it is a domain cookie (with diff --git a/cookiejar/jar_test.go b/cookiejar/jar_test.go index 3bac19b1..53598dc1 100644 --- a/cookiejar/jar_test.go +++ b/cookiejar/jar_test.go @@ -21,8 +21,9 @@ var tNow = time.Date(2013, 1, 1, 12, 0, 0, 0, time.UTC) // testPSL implements PublicSuffixList with just two rules: "co.uk" // and the default rule "*". // The implementation has two intentional bugs: -// PublicSuffix("www.buggy.psl") == "xy" -// PublicSuffix("www2.buggy.psl") == "com" +// +// PublicSuffix("www.buggy.psl") == "xy" +// PublicSuffix("www2.buggy.psl") == "com" type testPSL struct{} func (testPSL) String() string { @@ -306,8 +307,8 @@ var domainAndTypeTests = [...]struct { {"foo.sso.example.com", "sso.example.com", "sso.example.com", false, nil}, {"bar.co.uk", "bar.co.uk", "bar.co.uk", false, nil}, {"foo.bar.co.uk", ".bar.co.uk", "bar.co.uk", false, nil}, - {"127.0.0.1", "127.0.0.1", "", false, errNoHostname}, - {"2001:4860:0:2001::68", "2001:4860:0:2001::68", "2001:4860:0:2001::68", false, errNoHostname}, + {"127.0.0.1", "127.0.0.1", "127.0.0.1", true, nil}, + {"2001:4860:0:2001::68", "2001:4860:0:2001::68", "2001:4860:0:2001::68", true, nil}, {"www.example.com", ".", "", false, errMalformedDomain}, {"www.example.com", "..", "", false, errMalformedDomain}, {"www.example.com", "other.com", "", false, errIllegalDomain}, @@ -328,7 +329,7 @@ func TestDomainAndType(t *testing.T) { for _, tc := range domainAndTypeTests { domain, hostOnly, err := jar.domainAndType(tc.host, tc.domain) if err != tc.wantErr { - t.Errorf("%q/%q: got %q error, want %q", + t.Errorf("%q/%q: got %q error, want %v", tc.host, tc.domain, err, tc.wantErr) continue } @@ -359,13 +360,13 @@ func mustParseURL(s string) *url.URL { } // jarTest encapsulates the following actions on a jar: -// 1. Perform SetCookies with fromURL and the cookies from setCookies. -// (Done at time tNow + 0 ms.) -// 2. Check that the entries in the jar matches content. -// (Done at time tNow + 1001 ms.) -// 3. For each query in tests: Check that Cookies with toURL yields the -// cookies in want. -// (Query n done at tNow + (n+2)*1001 ms.) +// 1. Perform SetCookies with fromURL and the cookies from setCookies. +// (Done at time tNow + 0 ms.) +// 2. Check that the entries in the jar matches content. +// (Done at time tNow + 1001 ms.) +// 3. For each query in tests: Check that Cookies with toURL yields the +// cookies in want. +// (Query n done at tNow + (n+2)*1001 ms.) type jarTest struct { description string // The description of what this test is supposed to test fromURL string // The full URL of the request from which Set-Cookie headers where received @@ -593,6 +594,21 @@ var basicsTests = [...]jarTest{ "a=1", []query{{"http://192.168.0.10", "a=1"}}, }, + { + "Domain cookies on IP.", + "http://192.168.0.10", + []string{ + "a=1; domain=192.168.0.10", // allowed + "b=2; domain=172.31.9.9", // rejected, can't set cookie for other IP + "c=3; domain=.192.168.0.10", // rejected like in most browsers + }, + "a=1", + []query{ + {"http://192.168.0.10", "a=1"}, + {"http://172.31.9.9", ""}, + {"http://www.fancy.192.168.0.10", ""}, + }, + }, { "Port is ignored #1.", "http://www.host.test/", @@ -927,10 +943,17 @@ var chromiumBasicsTests = [...]jarTest{ { "TestIpAddress #3.", "http://1.2.3.4/foo", - []string{"a=1; domain=1.2.3.4"}, + []string{"a=1; domain=1.2.3.3"}, "", []query{{"http://1.2.3.4/foo", ""}}, }, + { + "TestIpAddress #4.", + "http://1.2.3.4/foo", + []string{"a=1; domain=1.2.3.4"}, + "a=1", + []query{{"http://1.2.3.4/foo", "a=1"}}, + }, { "TestNonDottedAndTLD #2.", "http://com./index.html", diff --git a/doc.go b/doc.go index 2ec72de1..50b6b841 100644 --- a/doc.go +++ b/doc.go @@ -107,6 +107,5 @@ directly and use its ConfigureTransport and/or ConfigureServer functions. Manually configuring HTTP/2 via the golang.org/x/net/http2 package takes precedence over the net/http package's built-in HTTP/2 support. - */ package http diff --git a/export_test.go b/export_test.go index a849327f..205ca83f 100644 --- a/export_test.go +++ b/export_test.go @@ -306,3 +306,12 @@ func ExportCloseTransportConnsAbruptly(tr *Transport) { } tr.idleMu.Unlock() } + +// ResponseWriterConnForTesting returns w's underlying connection, if w +// is a regular *response ResponseWriter. +func ResponseWriterConnForTesting(w ResponseWriter) (c net.Conn, ok bool) { + if r, ok := w.(*response); ok { + return r.conn.rwc, true + } + return nil, false +} diff --git a/fcgi/fcgi_test.go b/fcgi/fcgi_test.go index 3b107665..623deea5 100644 --- a/fcgi/fcgi_test.go +++ b/fcgi/fcgi_test.go @@ -402,16 +402,16 @@ func TestResponseWriterSniffsContentType(t *testing.T) { } } -type signallingNopCloser struct { +type signalingNopCloser struct { io.Reader closed chan bool } -func (*signallingNopCloser) Write(buf []byte) (int, error) { +func (*signalingNopCloser) Write(buf []byte) (int, error) { return len(buf), nil } -func (rc *signallingNopCloser) Close() error { +func (rc *signalingNopCloser) Close() error { close(rc.closed) return nil } @@ -430,7 +430,7 @@ func TestSlowRequest(t *testing.T) { } }(pw) - rc := &signallingNopCloser{pr, make(chan bool)} + rc := &signalingNopCloser{pr, make(chan bool)} handlerDone := make(chan bool) c := newChild(rc, http.HandlerFunc(func( diff --git a/filetransport.go b/filetransport.go index 32126d7e..94684b07 100644 --- a/filetransport.go +++ b/filetransport.go @@ -22,11 +22,11 @@ type fileTransport struct { // The typical use case for NewFileTransport is to register the "file" // protocol with a Transport, as in: // -// t := &http.Transport{} -// t.RegisterProtocol("file", http.NewFileTransport(http.Dir("/"))) -// c := &http.Client{Transport: t} -// res, err := c.Get("file:///etc/passwd") -// ... +// t := &http.Transport{} +// t.RegisterProtocol("file", http.NewFileTransport(http.Dir("/"))) +// c := &http.Client{Transport: t} +// res, err := c.Get("file:///etc/passwd") +// ... func NewFileTransport(fs FileSystem) RoundTripper { return fileTransport{fileHandler{fs}} } diff --git a/fs.go b/fs.go index 40061534..0a94de91 100644 --- a/fs.go +++ b/fs.go @@ -544,6 +544,7 @@ func writeNotModified(w ResponseWriter) { h := w.Header() delete(h, "Content-Type") delete(h, "Content-Length") + delete(h, "Content-Encoding") if h.Get("Etag") != "" { delete(h, "Last-Modified") } @@ -834,12 +835,11 @@ func FS(fsys fs.FS) FileSystem { // To use the operating system's file system implementation, // use http.Dir: // -// http.Handle("/", http.FileServer(http.Dir("/tmp"))) +// http.Handle("/", http.FileServer(http.Dir("/tmp"))) // // To use an fs.FS implementation, use http.FS to convert it: // // http.Handle("/", http.FileServer(http.FS(fsys))) -// func FileServer(root FileSystem) Handler { return &fileHandler{root} } diff --git a/fs_test.go b/fs_test.go index a657bc5a..19a2e3c9 100644 --- a/fs_test.go +++ b/fs_test.go @@ -565,6 +565,60 @@ func testServeFileWithContentEncoding(t *testing.T, h2 bool) { } } +// Tests that ServeFile does not generate representation metadata when +// file has not been modified, as per RFC 7232 section 4.1. +func TestServeFileNotModified_h1(t *testing.T) { testServeFileNotModified(t, h1Mode) } +func TestServeFileNotModified_h2(t *testing.T) { testServeFileNotModified(t, h2Mode) } +func testServeFileNotModified(t *testing.T, h2 bool) { + defer afterTest(t) + cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("Content-Encoding", "foo") + w.Header().Set("Etag", `"123"`) + ServeFile(w, r, "testdata/file") + + // Because the testdata is so small, it would fit in + // both the h1 and h2 Server's write buffers. For h1, + // sendfile is used, though, forcing a header flush at + // the io.Copy. http2 doesn't do a header flush so + // buffers all 11 bytes and then adds its own + // Content-Length. To prevent the Server's + // Content-Length and test ServeFile only, flush here. + w.(Flusher).Flush() + })) + defer cst.close() + req, err := NewRequest("GET", cst.ts.URL, nil) + if err != nil { + t.Fatal(err) + } + req.Header.Set("If-None-Match", `"123"`) + resp, err := cst.c.Do(req) + if err != nil { + t.Fatal(err) + } + b, err := io.ReadAll(resp.Body) + resp.Body.Close() + if err != nil { + t.Fatal("reading Body:", err) + } + if len(b) != 0 { + t.Errorf("non-empty body") + } + if g, e := resp.StatusCode, StatusNotModified; g != e { + t.Errorf("status mismatch: got %d, want %d", g, e) + } + // HTTP1 transport sets ContentLength to 0. + if g, e1, e2 := resp.ContentLength, int64(-1), int64(0); g != e1 && g != e2 { + t.Errorf("Content-Length mismatch: got %d, want %d or %d", g, e1, e2) + } + if resp.Header.Get("Content-Type") != "" { + t.Errorf("Content-Type present, but it should not be") + } + if resp.Header.Get("Content-Encoding") != "" { + t.Errorf("Content-Encoding present, but it should not be") + } +} + func TestServeIndexHtml(t *testing.T) { defer afterTest(t) diff --git a/h2_bundle.go b/h2_bundle.go index f6cfd76e..779dc133 100644 --- a/h2_bundle.go +++ b/h2_bundle.go @@ -30,7 +30,6 @@ import ( "errors" "fmt" "io" - "io/ioutil" "log" "math" mathrand "math/rand" @@ -1294,7 +1293,7 @@ func (e http2headerFieldNameError) Error() string { type http2headerFieldValueError string func (e http2headerFieldValueError) Error() string { - return fmt.Sprintf("invalid header field value %q", string(e)) + return fmt.Sprintf("invalid header field value for %q", string(e)) } var ( @@ -2864,7 +2863,8 @@ func (fr *http2Framer) readMetaFrame(hf *http2HeadersFrame) (*http2MetaHeadersFr fr.debugReadLoggerf("http2: decoded hpack field %+v", hf) } if !httpguts.ValidHeaderFieldValue(hf.Value) { - invalid = http2headerFieldValueError(hf.Value) + // Don't include the value in the error, because it may be sensitive. + invalid = http2headerFieldValueError(hf.Name) } isPseudo := strings.HasPrefix(hf.Name, ":") if isPseudo { @@ -3010,6 +3010,10 @@ func (t *http2Transport) dialTLSWithContext(ctx context.Context, network, addr s return tlsCn, nil } +func http2tlsUnderlyingConn(tc TLSConn) net.Conn { + return tc.NetConn() +} + var http2DebugGoroutines = os.Getenv("DEBUG_HTTP2_GOROUTINES") == "1" type http2goroutineLock uint64 @@ -3579,8 +3583,8 @@ func (s *http2sorter) SortStrings(ss []string) { // validPseudoPath reports whether v is a valid :path pseudo-header // value. It must be either: // -// *) a non-empty string starting with '/' -// *) the string '*', for OPTIONS requests. +// - a non-empty string starting with '/' +// - the string '*', for OPTIONS requests. // // For now this is only used a quick check for deciding when to clean // up Opaque URLs before sending requests from the Transport. @@ -4115,7 +4119,7 @@ func (s *http2Server) ServeConn(c net.Conn, opts *http2ServeConnOpts) { if s.NewWriteScheduler != nil { sc.writeSched = s.NewWriteScheduler() } else { - sc.writeSched = http2NewRandomWriteScheduler() + sc.writeSched = http2NewPriorityWriteScheduler(nil) } // These start at the RFC-specified defaults. If there is a higher @@ -6043,17 +6047,18 @@ type http2requestBody struct { _ http2incomparable stream *http2stream conn *http2serverConn - closed bool // for use by Close only + closeOnce sync.Once // for use by Close only sawEOF bool // for use by Read only pipe *http2pipe // non-nil if we have a HTTP entity message body needsContinue bool // need to send a 100-continue } func (b *http2requestBody) Close() error { - if b.pipe != nil && !b.closed { - b.pipe.BreakWithError(http2errClosedBody) - } - b.closed = true + b.closeOnce.Do(func() { + if b.pipe != nil { + b.pipe.BreakWithError(http2errClosedBody) + } + }) return nil } @@ -6371,8 +6376,7 @@ func http2checkWriteHeaderCode(code int) { // Issue 22880: require valid WriteHeader status codes. // For now we only enforce that it's three digits. // In the future we might block things over 599 (600 and above aren't defined - // at http://httpwg.org/specs/rfc7231.html#status.codes) - // and we might block under 200 (once we have more mature 1xx support). + // at http://httpwg.org/specs/rfc7231.html#status.codes). // But for now any three digits. // // We used to send "HTTP/1.1 000 0" on the wire in responses but there's @@ -6393,13 +6397,41 @@ func (w *http2responseWriter) WriteHeader(code int) { } func (rws *http2responseWriterState) writeHeader(code int) { - if !rws.wroteHeader { - http2checkWriteHeaderCode(code) - rws.wroteHeader = true - rws.status = code - if len(rws.handlerHeader) > 0 { - rws.snapHeader = http2cloneHeader(rws.handlerHeader) + if rws.wroteHeader { + return + } + + http2checkWriteHeaderCode(code) + + // Handle informational headers + if code >= 100 && code <= 199 { + // Per RFC 8297 we must not clear the current header map + h := rws.handlerHeader + + _, cl := h["Content-Length"] + _, te := h["Transfer-Encoding"] + if cl || te { + h = h.Clone() + h.Del("Content-Length") + h.Del("Transfer-Encoding") } + + if rws.conn.writeHeaders(rws.stream, &http2writeResHeaders{ + streamID: rws.stream.id, + httpResCode: code, + h: h, + endStream: rws.handlerDone && !rws.hasTrailers(), + }) != nil { + rws.dirty = true + } + + return + } + + rws.wroteHeader = true + rws.status = code + if len(rws.handlerHeader) > 0 { + rws.snapHeader = http2cloneHeader(rws.handlerHeader) } } @@ -6800,7 +6832,7 @@ type http2Transport struct { DialTLS func(network, addr string, cfg *tls.Config) (net.Conn, error) // TLSClientConfig specifies the TLS configuration to use with - // tls.Client. If nil, the default configuration is used. + // TLSClientFactory. If nil, the default configuration is used. TLSClientConfig *tls.Config // ConnPool optionally specifies an alternate connection pool to use. @@ -7224,12 +7256,14 @@ func (t *http2Transport) RoundTripOpt(req *Request, opt http2RoundTripOpt) (*Res if req, err = http2shouldRetryRequest(req, err); err == nil { // After the first retry, do exponential backoff with 10% jitter. if retry == 0 { + t.vlogf("RoundTrip retrying after failure: %v", err) continue } backoff := float64(uint(1) << (uint(retry) - 1)) backoff += backoff * (0.1 * mathrand.Float64()) select { case <-time.After(time.Second * time.Duration(backoff)): + t.vlogf("RoundTrip retrying after failure: %v", err) continue case <-req.Context().Done(): err = req.Context().Err() @@ -7455,11 +7489,13 @@ func (cc *http2ClientConn) healthCheck() { // trigger the healthCheck again if there is no frame received. ctx, cancel := context.WithTimeout(context.Background(), pingTimeout) defer cancel() + cc.vlogf("http2: Transport sending health check") err := cc.Ping(ctx) if err != nil { + cc.vlogf("http2: Transport health check failure: %v", err) cc.closeForLostPing() - cc.t.connPool().MarkDead(cc) - return + } else { + cc.vlogf("http2: Transport health check success") } } @@ -7630,6 +7666,24 @@ func (cc *http2ClientConn) onIdleTimeout() { cc.closeIfIdle() } +func (cc *http2ClientConn) closeConn() error { + t := time.AfterFunc(250*time.Millisecond, cc.forceCloseConn) + defer t.Stop() + return cc.tconn.Close() +} + +// A tls.Conn.Close can hang for a long time if the peer is unresponsive. +// Try to shut it down more aggressively. +func (cc *http2ClientConn) forceCloseConn() { + tc, ok := cc.tconn.(TLSConn) + if !ok { + return + } + if nc := http2tlsUnderlyingConn(tc); nc != nil { + nc.Close() + } +} + func (cc *http2ClientConn) closeIfIdle() { cc.mu.Lock() if len(cc.streams) > 0 || cc.streamsReserved > 0 { @@ -7644,7 +7698,7 @@ func (cc *http2ClientConn) closeIfIdle() { if http2VerboseLogs { cc.vlogf("http2: Transport closing idle conn %p (forSingleUse=%v, maxStream=%v)", cc, cc.singleUse, nextID-2) } - cc.tconn.Close() + cc.closeConn() } func (cc *http2ClientConn) isDoNotReuseAndIdle() bool { @@ -7661,7 +7715,7 @@ func (cc *http2ClientConn) Shutdown(ctx context.Context) error { return err } // Wait for all in-flight streams to complete or connection to close - done := make(chan error, 1) + done := make(chan struct{}) cancelled := false // guarded by cc.mu go func() { cc.mu.Lock() @@ -7669,7 +7723,7 @@ func (cc *http2ClientConn) Shutdown(ctx context.Context) error { for { if len(cc.streams) == 0 || cc.closed { cc.closed = true - done <- cc.tconn.Close() + close(done) break } if cancelled { @@ -7680,8 +7734,8 @@ func (cc *http2ClientConn) Shutdown(ctx context.Context) error { }() http2shutdownEnterWaitStateHook() select { - case err := <-done: - return err + case <-done: + return cc.closeConn() case <-ctx.Done(): cc.mu.Lock() // Free the goroutine above @@ -7724,9 +7778,9 @@ func (cc *http2ClientConn) closeForError(err error) error { for _, cs := range cc.streams { cs.abortStreamLocked(err) } - defer cc.cond.Broadcast() - defer cc.mu.Unlock() - return cc.tconn.Close() + cc.cond.Broadcast() + cc.mu.Unlock() + return cc.closeConn() } // Close closes the client connection immediately. @@ -8471,7 +8525,8 @@ func (cc *http2ClientConn) encodeHeaders(req *Request, addGzipHeader bool, trail } for _, v := range vv { if !httpguts.ValidHeaderFieldValue(v) { - return nil, fmt.Errorf("invalid HTTP header value %q for header %q", v, k) + // Don't include the value in the error, because it may be sensitive. + return nil, fmt.Errorf("invalid HTTP header value for header %q", k) } } } @@ -8701,7 +8756,7 @@ func (cc *http2ClientConn) forgetStreamID(id uint32) { cc.vlogf("http2: Transport closing idle conn %p (forSingleUse=%v, maxStream=%v)", cc, cc.singleUse, cc.nextStreamID-2) } cc.closed = true - defer cc.tconn.Close() + defer cc.closeConn() } cc.mu.Unlock() @@ -8748,8 +8803,8 @@ func http2isEOFOrNetReadError(err error) bool { func (rl *http2clientConnReadLoop) cleanup() { cc := rl.cc - defer cc.tconn.Close() - defer cc.t.connPool().MarkDead(cc) + cc.t.connPool().MarkDead(cc) + defer cc.closeConn() defer close(cc.readerDone) if cc.idleTimer != nil { @@ -9604,7 +9659,13 @@ func (t *http2Transport) logf(format string, args ...interface{}) { log.Printf(format, args...) } -var http2noBody io.ReadCloser = ioutil.NopCloser(bytes.NewReader(nil)) +var http2noBody io.ReadCloser = http2noBodyReader{} + +type http2noBodyReader struct{} + +func (http2noBodyReader) Close() error { return nil } + +func (http2noBodyReader) Read([]byte) (int, error) { return 0, io.EOF } type http2missingBody struct{} diff --git a/header.go b/header.go index cd180dd3..5f453803 100644 --- a/header.go +++ b/header.go @@ -43,7 +43,8 @@ func (h Header) Set(key, value string) { // Get gets the first value associated with the given key. If // there are no values associated with the key, Get returns "". // It is case insensitive; textproto.CanonicalMIMEHeaderKey is -// used to canonicalize the provided key. To use non-canonical keys, +// used to canonicalize the provided key. Get assumes that all +// keys are stored in canonical form. To use non-canonical keys, // access the map directly. func (h Header) Get(key string) string { return textproto.MIMEHeader(h).Get(key) diff --git a/httptest/recorder.go b/httptest/recorder.go index a8eca6a3..0dd79e9e 100644 --- a/httptest/recorder.go +++ b/httptest/recorder.go @@ -207,18 +207,20 @@ func (rw *ResponseRecorder) Result() *http.Response { if trailers, ok := rw.snapHeader["Trailer"]; ok { res.Trailer = make(http.Header, len(trailers)) for _, k := range trailers { - k = http.CanonicalHeaderKey(k) - if !httpguts.ValidTrailerHeader(k) { - // Ignore since forbidden by RFC 7230, section 4.1.2. - continue + for _, k := range strings.Split(k, ",") { + k = http.CanonicalHeaderKey(textproto.TrimString(k)) + if !httpguts.ValidTrailerHeader(k) { + // Ignore since forbidden by RFC 7230, section 4.1.2. + continue + } + vv, ok := rw.HeaderMap[k] + if !ok { + continue + } + vv2 := make([]string, len(vv)) + copy(vv2, vv) + res.Trailer[k] = vv2 } - vv, ok := rw.HeaderMap[k] - if !ok { - continue - } - vv2 := make([]string, len(vv)) - copy(vv2, vv) - res.Trailer[k] = vv2 } } for k, vv := range rw.HeaderMap { diff --git a/httptest/recorder_test.go b/httptest/recorder_test.go index 773ab0aa..89fb1a35 100644 --- a/httptest/recorder_test.go +++ b/httptest/recorder_test.go @@ -221,8 +221,7 @@ func TestRecorder(t *testing.T) { "Trailer headers are correctly recorded", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Non-Trailer", "correct") - w.Header().Set("Trailer", "Trailer-A") - w.Header().Add("Trailer", "Trailer-B") + w.Header().Set("Trailer", "Trailer-A, Trailer-B") w.Header().Add("Trailer", "Trailer-C") io.WriteString(w, "") w.Header().Set("Non-Trailer", "incorrect") diff --git a/httptest/server.go b/httptest/server.go index c9651f89..71ed67f6 100644 --- a/httptest/server.go +++ b/httptest/server.go @@ -77,7 +77,9 @@ func newLocalListener() net.Listener { // When debugging a particular http server-based test, // this flag lets you run +// // go test -run=BrokenTest -httptest.serve=127.0.0.1:8000 +// // to start the broken server so you can interact with it manually. // We only register this flag if it looks like the caller knows about it // and is trying to use it as we don't want to pollute flags and this diff --git a/httputil/reverseproxy.go b/httputil/reverseproxy.go index 2c768b66..56a8367f 100644 --- a/httputil/reverseproxy.go +++ b/httputil/reverseproxy.go @@ -218,7 +218,18 @@ func (p *ReverseProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) { } ctx := req.Context() - if cn, ok := rw.(http.CloseNotifier); ok { + if ctx.Done() != nil { + // CloseNotifier predates context.Context, and has been + // entirely superseded by it. If the request contains + // a Context that carries a cancellation signal, don't + // bother spinning up a goroutine to watch the CloseNotify + // channel (if any). + // + // If the request Context has a nil Done channel (which + // means it is either context.Background, or a custom + // Context implementation with no cancellation signal), + // then consult the CloseNotifier if available. + } else if cn, ok := rw.(http.CloseNotifier); ok { var cancel context.CancelFunc ctx, cancel = context.WithCancel(ctx) defer cancel() @@ -614,7 +625,6 @@ func (p *ReverseProxy) handleUpgradeResponse(rw http.ResponseWriter, req *http.R go spc.copyToBackend(errc) go spc.copyFromBackend(errc) <-errc - return } // switchProtocolCopier exists so goroutines proxying data back and diff --git a/internal/chunked.go b/internal/chunked.go index 37a72e90..5a174415 100644 --- a/internal/chunked.go +++ b/internal/chunked.go @@ -163,10 +163,11 @@ var semi = []byte(";") // removeChunkExtension removes any chunk-extension from p. // For example, -// "0" => "0" -// "0;token" => "0" -// "0;token=val" => "0" -// `0;token="quoted string"` => "0" +// +// "0" => "0" +// "0;token" => "0" +// "0;token=val" => "0" +// `0;token="quoted string"` => "0" func removeChunkExtension(p []byte) ([]byte, error) { p, _, _ = bytes.Cut(p, semi) // TODO: care about exact syntax of chunk extensions? We're diff --git a/request.go b/request.go index dfbdcaa4..74f3ce7b 100644 --- a/request.go +++ b/request.go @@ -359,7 +359,6 @@ func (r *Request) WithContext(ctx context.Context) *Request { r2 := new(Request) *r2 = *r r2.ctx = ctx - r2.URL = cloneURL(r.URL) // legacy behavior; TODO: try to remove. Issue 23544 return r2 } @@ -480,6 +479,9 @@ func (r *Request) multipartReader(allowMixed bool) (*multipart.Reader, error) { if v == "" { return nil, ErrNotMultipart } + if r.Body == nil { + return nil, errors.New("missing form body") + } d, params, err := mime.ParseMediaType(v) if err != nil || !(d == "multipart/form-data" || allowMixed && d == "multipart/mixed") { return nil, ErrNotMultipart @@ -513,6 +515,7 @@ const defaultUserAgent = "Go-http-client/1.1" // Write writes an HTTP/1.1 request, which is the header and body, in wire format. // This method consults the following fields of the request: +// // Host // URL // Method (defaults to "GET") @@ -736,9 +739,11 @@ func idnaASCII(v string) (string, error) { // into Punycode form, if necessary. // // Ideally we'd clean the Host header according to the spec: -// https://tools.ietf.org/html/rfc7230#section-5.4 (Host = uri-host [ ":" port ]") -// https://tools.ietf.org/html/rfc7230#section-2.7 (uri-host -> rfc3986's host) -// https://tools.ietf.org/html/rfc3986#section-3.2.2 (definition of host) +// +// https://tools.ietf.org/html/rfc7230#section-5.4 (Host = uri-host [ ":" port ]") +// https://tools.ietf.org/html/rfc7230#section-2.7 (uri-host -> rfc3986's host) +// https://tools.ietf.org/html/rfc3986#section-3.2.2 (definition of host) +// // But practically, what we are trying to avoid is the situation in // issue 11206, where a malformed Host header used in the proxy context // would create a bad request. So it is enough to just truncate at the @@ -969,11 +974,13 @@ func parseBasicAuth(auth string) (username, password string, ok bool) { // Basic Authentication with the provided username and password. // // With HTTP Basic Authentication the provided username and password -// are not encrypted. +// are not encrypted. It should generally only be used in an HTTPS +// request. // -// Some protocols may impose additional requirements on pre-escaping the -// username and password. For instance, when used with OAuth2, both arguments -// must be URL encoded first with url.QueryEscape. +// The username may not contain a colon. Some protocols may impose +// additional requirements on pre-escaping the username and +// password. For instance, when used with OAuth2, both arguments must +// be URL encoded first with url.QueryEscape. func (r *Request) SetBasicAuth(username, password string) { r.Header.Set("Authorization", "Basic "+basicAuth(username, password)) } @@ -1119,21 +1126,34 @@ func readRequest(b *bufio.Reader) (req *Request, err error) { // MaxBytesReader is similar to io.LimitReader but is intended for // limiting the size of incoming request bodies. In contrast to // io.LimitReader, MaxBytesReader's result is a ReadCloser, returns a -// non-EOF error for a Read beyond the limit, and closes the -// underlying reader when its Close method is called. +// non-nil error of type *MaxBytesError for a Read beyond the limit, +// and closes the underlying reader when its Close method is called. // // MaxBytesReader prevents clients from accidentally or maliciously -// sending a large request and wasting server resources. +// sending a large request and wasting server resources. If possible, +// it tells the ResponseWriter to close the connection after the limit +// has been reached. func MaxBytesReader(w ResponseWriter, r io.ReadCloser, n int64) io.ReadCloser { if n < 0 { // Treat negative limits as equivalent to 0. n = 0 } - return &maxBytesReader{w: w, r: r, n: n} + return &maxBytesReader{w: w, r: r, i: n, n: n} +} + +// MaxBytesError is returned by MaxBytesReader when its read limit is exceeded. +type MaxBytesError struct { + Limit int64 +} + +func (e *MaxBytesError) Error() string { + // Due to Hyrum's law, this text cannot be changed. + return "http: request body too large" } type maxBytesReader struct { w ResponseWriter r io.ReadCloser // underlying reader + i int64 // max bytes initially, for MaxBytesError n int64 // max bytes remaining err error // sticky error } @@ -1175,7 +1195,7 @@ func (l *maxBytesReader) Read(p []byte) (n int, err error) { if res, ok := l.w.(requestTooLarger); ok { res.requestTooLarge() } - l.err = errors.New("http: request body too large") + l.err = &MaxBytesError{l.i} return n, l.err } diff --git a/request_test.go b/request_test.go index 952935dc..83e2e86b 100644 --- a/request_test.go +++ b/request_test.go @@ -999,23 +999,15 @@ func TestMaxBytesReaderDifferentLimits(t *testing.T) { } } -func TestWithContextDeepCopiesURL(t *testing.T) { +func TestWithContextNilURL(t *testing.T) { req, err := NewRequest("POST", "https://golang.org/", nil) if err != nil { t.Fatal(err) } - reqCopy := req.WithContext(context.Background()) - reqCopy.URL.Scheme = "http" - - firstURL, secondURL := req.URL.String(), reqCopy.URL.String() - if firstURL == secondURL { - t.Errorf("unexpected change to original request's URL") - } - - // And also check we don't crash on nil (Issue 20601) + // Issue 20601 req.URL = nil - reqCopy = req.WithContext(context.Background()) + reqCopy := req.WithContext(context.Background()) if reqCopy.URL != nil { t.Error("expected nil URL in cloned request") } diff --git a/response.go b/response.go index 297394ea..755c6965 100644 --- a/response.go +++ b/response.go @@ -205,8 +205,11 @@ func ReadResponse(r *bufio.Reader, req *Request) (*Response, error) { } // RFC 7234, section 5.4: Should treat +// // Pragma: no-cache +// // like +// // Cache-Control: no-cache func fixPragmaCacheControl(header Header) { if hp, ok := header["Pragma"]; ok && len(hp) > 0 && hp[0] == "no-cache" { @@ -228,24 +231,23 @@ func (r *Response) ProtoAtLeast(major, minor int) bool { // // This method consults the following fields of the response r: // -// StatusCode -// ProtoMajor -// ProtoMinor -// Request.Method -// TransferEncoding -// Trailer -// Body -// ContentLength -// Header, values for non-canonical keys will have unpredictable behavior +// StatusCode +// ProtoMajor +// ProtoMinor +// Request.Method +// TransferEncoding +// Trailer +// Body +// ContentLength +// Header, values for non-canonical keys will have unpredictable behavior // // The Response Body is closed after it is sent. func (r *Response) Write(w io.Writer) error { // Status line text := r.Status if text == "" { - var ok bool - text, ok = statusText[r.StatusCode] - if !ok { + text = StatusText(r.StatusCode) + if text == "" { text = "status code " + strconv.Itoa(r.StatusCode) } } else { diff --git a/serve_test.go b/serve_test.go index 2016eced..23105a45 100644 --- a/serve_test.go +++ b/serve_test.go @@ -2988,6 +2988,13 @@ func testRequestBodyLimit(t *testing.T, h2 bool) { if n != limit { t.Errorf("io.Copy = %d, want %d", n, limit) } + mbErr, ok := err.(*MaxBytesError) + if !ok { + t.Errorf("expected MaxBytesError, got %T", err) + } + if mbErr.Limit != limit { + t.Errorf("MaxBytesError.Limit = %d, want %d", mbErr.Limit, limit) + } })) defer cst.close() @@ -3819,7 +3826,7 @@ func testServerReaderFromOrder(t *testing.T, h2 bool) { // Issue 6157, Issue 6685 func TestCodesPreventingContentTypeAndBody(t *testing.T) { - for _, code := range []int{StatusNotModified, StatusNoContent, StatusContinue} { + for _, code := range []int{StatusNotModified, StatusNoContent} { ht := newHandlerTest(HandlerFunc(func(w ResponseWriter, r *Request) { if r.URL.Path == "/header" { w.Header().Set("Content-Length", "123") @@ -4830,11 +4837,7 @@ func TestServerRequestContextCancel_ConnClose(t *testing.T) { handlerDone := make(chan struct{}) ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) { close(inHandler) - select { - case <-r.Context().Done(): - case <-time.After(3 * time.Second): - t.Errorf("timeout waiting for context to be done") - } + <-r.Context().Done() close(handlerDone) })) defer ts.Close() @@ -4844,18 +4847,9 @@ func TestServerRequestContextCancel_ConnClose(t *testing.T) { } defer c.Close() io.WriteString(c, "GET / HTTP/1.1\r\nHost: foo\r\n\r\n") - select { - case <-inHandler: - case <-time.After(3 * time.Second): - t.Fatalf("timeout waiting to see ServeHTTP get called") - } + <-inHandler c.Close() // this should trigger the context being done - - select { - case <-handlerDone: - case <-time.After(4 * time.Second): - t.Fatalf("timeout waiting to see ServeHTTP exit") - } + <-handlerDone } func TestServerContext_ServerContextKey_h1(t *testing.T) { @@ -5030,10 +5024,11 @@ func benchmarkClientServerParallel(b *testing.B, parallelism int, useTLS bool) { // The client code runs in a subprocess. // // For use like: -// $ go test -c -// $ ./http.test -test.run=XX -test.bench=BenchmarkServer -test.benchtime=15s -test.cpuprofile=http.prof -// $ go tool pprof http.test http.prof -// (pprof) web +// +// $ go test -c +// $ ./http.test -test.run=XX -test.bench=BenchmarkServer -test.benchtime=15s -test.cpuprofile=http.prof +// $ go tool pprof http.test http.prof +// (pprof) web func BenchmarkServer(b *testing.B) { b.ReportAllocs() // Child process mode; @@ -6622,3 +6617,35 @@ func testMaxBytesHandler(t *testing.T, maxSize, requestSize int64) { t.Errorf("expected echo of size %d; got %d", handlerN, buf.Len()) } } + +func TestEarlyHints(t *testing.T) { + ht := newHandlerTest(HandlerFunc(func(w ResponseWriter, r *Request) { + h := w.Header() + h.Add("Link", "; rel=preload; as=style") + h.Add("Link", "; rel=preload; as=script") + w.WriteHeader(StatusEarlyHints) + + h.Add("Link", "; rel=preload; as=script") + w.WriteHeader(StatusEarlyHints) + + w.Write([]byte("stuff")) + })) + + got := ht.rawResponse("GET / HTTP/1.1\nHost: golang.org") + expected := "HTTP/1.1 103 Early Hints\r\nLink: ; rel=preload; as=style\r\nLink: ; rel=preload; as=script\r\n\r\nHTTP/1.1 103 Early Hints\r\nLink: ; rel=preload; as=style\r\nLink: ; rel=preload; as=script\r\nLink: ; rel=preload; as=script\r\n\r\nHTTP/1.1 200 OK\r\nLink: ; rel=preload; as=style\r\nLink: ; rel=preload; as=script\r\nLink: ; rel=preload; as=script\r\nDate: " // dynamic content expected + if !strings.Contains(got, expected) { + t.Errorf("unexpected response; got %q; should start by %q", got, expected) + } +} +func TestProcessing(t *testing.T) { + ht := newHandlerTest(HandlerFunc(func(w ResponseWriter, r *Request) { + w.WriteHeader(StatusProcessing) + w.Write([]byte("stuff")) + })) + + got := ht.rawResponse("GET / HTTP/1.1\nHost: golang.org") + expected := "HTTP/1.1 102 Processing\r\n\r\nHTTP/1.1 200 OK\r\nDate: " // dynamic content expected + if !strings.Contains(got, expected) { + t.Errorf("unexpected response; got %q; should start by %q", got, expected) + } +} diff --git a/server.go b/server.go index 503f9b8b..fa5ca8a5 100644 --- a/server.go +++ b/server.go @@ -97,8 +97,8 @@ type ResponseWriter interface { // Handlers can set HTTP trailers. // // Changing the header map after a call to WriteHeader (or - // Write) has no effect unless the modified headers are - // trailers. + // Write) has no effect unless the HTTP status code was of the + // 1xx class or the modified headers are trailers. // // There are two ways to set Trailers. The preferred way is to // predeclare in the headers which trailers you will later @@ -143,13 +143,18 @@ type ResponseWriter interface { // If WriteHeader is not called explicitly, the first call to Write // will trigger an implicit WriteHeader(http.StatusOK). // Thus explicit calls to WriteHeader are mainly used to - // send error codes. + // send error codes or 1xx informational responses. // // The provided code must be a valid HTTP 1xx-5xx status code. - // Only one header may be written. Go does not currently - // support sending user-defined 1xx informational headers, - // with the exception of 100-continue response header that the - // Server sends automatically when the Request.Body is read. + // Any number of 1xx headers may be written, followed by at most + // one 2xx-5xx header. 1xx headers are sent immediately, but 2xx-5xx + // headers may be buffered. Use the Flusher interface to send + // buffered data. The header map is cleared when 2xx-5xx headers are + // sent, but not with 1xx headers. + // + // The server will automatically send a 100 (Continue) header + // on the first read from the request body if the request has + // an "Expect: 100-continue" header. WriteHeader(statusCode int) } @@ -419,7 +424,7 @@ type response struct { req *Request // request for this response reqBody io.ReadCloser cancelCtx context.CancelFunc // when ServeHTTP exits - wroteHeader bool // reply header has been (logically) written + wroteHeader bool // a non-1xx header has been (logically) written wroteContinue bool // 100 Continue response was written wants10KeepAlive bool // HTTP/1.0 w/ Connection "keep-alive" wantsClose bool // HTTP request has Connection "close" @@ -493,8 +498,9 @@ type response struct { // prior to the headers being written. If the set of trailers is fixed // or known before the header is written, the normal Go trailers mechanism // is preferred: -// https://pkg.go.dev/net/http#ResponseWriter -// https://pkg.go.dev/net/http#example-ResponseWriter-Trailers +// +// https://pkg.go.dev/net/http#ResponseWriter +// https://pkg.go.dev/net/http#example-ResponseWriter-Trailers const TrailerPrefix = "Trailer:" // finalTrailers is called after the Handler exits and returns a non-nil @@ -1098,8 +1104,7 @@ func checkWriteHeaderCode(code int) { // Issue 22880: require valid WriteHeader status codes. // For now we only enforce that it's three digits. // In the future we might block things over 599 (600 and above aren't defined - // at https://httpwg.org/specs/rfc7231.html#status.codes) - // and we might block under 200 (once we have more mature 1xx support). + // at https://httpwg.org/specs/rfc7231.html#status.codes). // But for now any three digits. // // We used to send "HTTP/1.1 000 0" on the wire in responses but there's @@ -1142,6 +1147,26 @@ func (w *response) WriteHeader(code int) { return } checkWriteHeaderCode(code) + + // Handle informational headers + if code >= 100 && code <= 199 { + // Prevent a potential race with an automatically-sent 100 Continue triggered by Request.Body.Read() + if code == 100 && w.canWriteContinue.isSet() { + w.writeContinueMu.Lock() + w.canWriteContinue.setFalse() + w.writeContinueMu.Unlock() + } + + writeStatusLine(w.conn.bufw, w.req.ProtoAtLeast(1, 1), code, w.statusBuf[:]) + + // Per RFC 8297 we must not clear the current header map + w.handlerHeader.WriteSubset(w.conn.bufw, excludedHeadersNoBody) + w.conn.bufw.Write(crlf) + w.conn.bufw.Flush() + + return + } + w.wroteHeader = true w.status = code @@ -1514,7 +1539,7 @@ func writeStatusLine(bw *bufio.Writer, is11 bool, code int, scratch []byte) { } else { bw.WriteString("HTTP/1.0 ") } - if text, ok := statusText[code]; ok { + if text := StatusText(code); text != "" { bw.Write(strconv.AppendInt(scratch[:0], int64(code), 10)) bw.WriteByte(' ') bw.WriteString(text) @@ -1550,14 +1575,14 @@ func (w *response) bodyAllowed() bool { // // The Writers are wired together like: // -// 1. *response (the ResponseWriter) -> -// 2. (*response).w, a *bufio.Writer of bufferBeforeChunkingSize bytes -> -// 3. chunkWriter.Writer (whose writeHeader finalizes Content-Length/Type) -// and which writes the chunk headers, if needed -> -// 4. conn.bufw, a *bufio.Writer of default (4kB) bytes, writing to -> -// 5. checkConnErrorWriter{c}, which notes any non-nil error on Write -// and populates c.werr with it if so, but otherwise writes to -> -// 6. the rwc, the net.Conn. +// 1. *response (the ResponseWriter) -> +// 2. (*response).w, a *bufio.Writer of bufferBeforeChunkingSize bytes -> +// 3. chunkWriter.Writer (whose writeHeader finalizes Content-Length/Type) +// and which writes the chunk headers, if needed -> +// 4. conn.bufw, a *bufio.Writer of default (4kB) bytes, writing to -> +// 5. checkConnErrorWriter{c}, which notes any non-nil error on Write +// and populates c.werr with it if so, but otherwise writes to -> +// 6. the rwc, the net.Conn. // // TODO(bradfitz): short-circuit some of the buffering when the // initial header contains both a Content-Type and Content-Length. @@ -1720,7 +1745,7 @@ type closeWriter interface { var _ closeWriter = (*net.TCPConn)(nil) // closeWrite flushes any outstanding data and sends a FIN packet (if -// client is connected via TCP), signalling that we're done. We then +// client is connected via TCP), signaling that we're done. We then // pause for a bit, hoping the client processes it before any // subsequent RST. // @@ -2100,7 +2125,7 @@ func Error(w ResponseWriter, error string, code int) { func NotFound(w ResponseWriter, r *Request) { Error(w, "404 page not found", StatusNotFound) } // NotFoundHandler returns a simple request handler -// that replies to each request with a ``404 page not found'' reply. +// that replies to each request with a “404 page not found” reply. func NotFoundHandler() Handler { return HandlerFunc(NotFound) } // StripPrefix returns a handler that serves HTTP requests by removing the @@ -2190,7 +2215,7 @@ func Redirect(w ResponseWriter, r *Request, url string, code int) { // Shouldn't send the body for POST or HEAD; that leaves GET. if !hadCT && r.Method == "GET" { - body := "" + statusText[code] + ".\n" + body := "" + StatusText(code) + ".\n" fmt.Fprintln(w, body) } } @@ -2393,7 +2418,7 @@ func (mux *ServeMux) shouldRedirectRLocked(host, path string) bool { // the pattern that will match after following the redirect. // // If there is no registered handler that applies to the request, -// Handler returns a ``page not found'' handler and an empty pattern. +// Handler returns a “page not found” handler and an empty pattern. func (mux *ServeMux) Handler(r *Request) (h Handler, pattern string) { // CONNECT requests are not canonicalized. @@ -2664,6 +2689,8 @@ type Server struct { activeConn map[*conn]struct{} doneChan chan struct{} onShutdown []func() + + listenerGroup sync.WaitGroup } func (s *Server) getDoneChan() <-chan struct{} { @@ -2706,6 +2733,15 @@ func (srv *Server) Close() error { defer srv.mu.Unlock() srv.closeDoneChanLocked() err := srv.closeListenersLocked() + + // Unlock srv.mu while waiting for listenerGroup. + // The group Add and Done calls are made with srv.mu held, + // to avoid adding a new listener in the window between + // us setting inShutdown above and waiting here. + srv.mu.Unlock() + srv.listenerGroup.Wait() + srv.mu.Lock() + for c := range srv.activeConn { c.rwc.Close() delete(srv.activeConn, c) @@ -2752,6 +2788,7 @@ func (srv *Server) Shutdown(ctx context.Context) error { go f() } srv.mu.Unlock() + srv.listenerGroup.Wait() pollIntervalBase := time.Millisecond nextPollInterval := func() time.Duration { @@ -2768,7 +2805,7 @@ func (srv *Server) Shutdown(ctx context.Context) error { timer := time.NewTimer(nextPollInterval()) defer timer.Stop() for { - if srv.closeIdleConns() && srv.numListeners() == 0 { + if srv.closeIdleConns() { return lnerr } select { @@ -2791,12 +2828,6 @@ func (srv *Server) RegisterOnShutdown(f func()) { srv.mu.Unlock() } -func (s *Server) numListeners() int { - s.mu.Lock() - defer s.mu.Unlock() - return len(s.listeners) -} - // closeIdleConns closes all idle connections and reports whether the // server is quiescent. func (s *Server) closeIdleConns() bool { @@ -3131,8 +3162,10 @@ func (s *Server) trackListener(ln *net.Listener, add bool) bool { return false } s.listeners[ln] = struct{}{} + s.listenerGroup.Add(1) } else { delete(s.listeners, ln) + s.listenerGroup.Done() } return true } diff --git a/sniff.go b/sniff.go index 67a7151b..ac18ab97 100644 --- a/sniff.go +++ b/sniff.go @@ -128,11 +128,6 @@ var sniffSignatures = []sniffSig{ // Audio and Video types // Enforce the pattern match ordering as prescribed in // https://mimesniff.spec.whatwg.org/#matching-an-audio-or-video-type-pattern - &maskedSig{ - mask: []byte("\xFF\xFF\xFF\xFF"), - pat: []byte(".snd"), - ct: "audio/basic", - }, &maskedSig{ mask: []byte("\xFF\xFF\xFF\xFF\x00\x00\x00\x00\xFF\xFF\xFF\xFF"), pat: []byte("FORM\x00\x00\x00\x00AIFF"), diff --git a/status.go b/status.go index 286315f6..cd90877e 100644 --- a/status.go +++ b/status.go @@ -7,68 +7,68 @@ package http // HTTP status codes as registered with IANA. // See: https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml const ( - StatusContinue = 100 // RFC 7231, 6.2.1 - StatusSwitchingProtocols = 101 // RFC 7231, 6.2.2 + StatusContinue = 100 // RFC 9110, 15.2.1 + StatusSwitchingProtocols = 101 // RFC 9110, 15.2.2 StatusProcessing = 102 // RFC 2518, 10.1 StatusEarlyHints = 103 // RFC 8297 - StatusOK = 200 // RFC 7231, 6.3.1 - StatusCreated = 201 // RFC 7231, 6.3.2 - StatusAccepted = 202 // RFC 7231, 6.3.3 - StatusNonAuthoritativeInfo = 203 // RFC 7231, 6.3.4 - StatusNoContent = 204 // RFC 7231, 6.3.5 - StatusResetContent = 205 // RFC 7231, 6.3.6 - StatusPartialContent = 206 // RFC 7233, 4.1 + StatusOK = 200 // RFC 9110, 15.3.1 + StatusCreated = 201 // RFC 9110, 15.3.2 + StatusAccepted = 202 // RFC 9110, 15.3.3 + StatusNonAuthoritativeInfo = 203 // RFC 9110, 15.3.4 + StatusNoContent = 204 // RFC 9110, 15.3.5 + StatusResetContent = 205 // RFC 9110, 15.3.6 + StatusPartialContent = 206 // RFC 9110, 15.3.7 StatusMultiStatus = 207 // RFC 4918, 11.1 StatusAlreadyReported = 208 // RFC 5842, 7.1 StatusIMUsed = 226 // RFC 3229, 10.4.1 - StatusMultipleChoices = 300 // RFC 7231, 6.4.1 - StatusMovedPermanently = 301 // RFC 7231, 6.4.2 - StatusFound = 302 // RFC 7231, 6.4.3 - StatusSeeOther = 303 // RFC 7231, 6.4.4 - StatusNotModified = 304 // RFC 7232, 4.1 - StatusUseProxy = 305 // RFC 7231, 6.4.5 - _ = 306 // RFC 7231, 6.4.6 (Unused) - StatusTemporaryRedirect = 307 // RFC 7231, 6.4.7 - StatusPermanentRedirect = 308 // RFC 7538, 3 + StatusMultipleChoices = 300 // RFC 9110, 15.4.1 + StatusMovedPermanently = 301 // RFC 9110, 15.4.2 + StatusFound = 302 // RFC 9110, 15.4.3 + StatusSeeOther = 303 // RFC 9110, 15.4.4 + StatusNotModified = 304 // RFC 9110, 15.4.5 + StatusUseProxy = 305 // RFC 9110, 15.4.6 + _ = 306 // RFC 9110, 15.4.7 (Unused) + StatusTemporaryRedirect = 307 // RFC 9110, 15.4.8 + StatusPermanentRedirect = 308 // RFC 9110, 15.4.9 - StatusBadRequest = 400 // RFC 7231, 6.5.1 - StatusUnauthorized = 401 // RFC 7235, 3.1 - StatusPaymentRequired = 402 // RFC 7231, 6.5.2 - StatusForbidden = 403 // RFC 7231, 6.5.3 - StatusNotFound = 404 // RFC 7231, 6.5.4 - StatusMethodNotAllowed = 405 // RFC 7231, 6.5.5 - StatusNotAcceptable = 406 // RFC 7231, 6.5.6 - StatusProxyAuthRequired = 407 // RFC 7235, 3.2 - StatusRequestTimeout = 408 // RFC 7231, 6.5.7 - StatusConflict = 409 // RFC 7231, 6.5.8 - StatusGone = 410 // RFC 7231, 6.5.9 - StatusLengthRequired = 411 // RFC 7231, 6.5.10 - StatusPreconditionFailed = 412 // RFC 7232, 4.2 - StatusRequestEntityTooLarge = 413 // RFC 7231, 6.5.11 - StatusRequestURITooLong = 414 // RFC 7231, 6.5.12 - StatusUnsupportedMediaType = 415 // RFC 7231, 6.5.13 - StatusRequestedRangeNotSatisfiable = 416 // RFC 7233, 4.4 - StatusExpectationFailed = 417 // RFC 7231, 6.5.14 - StatusTeapot = 418 // RFC 7168, 2.3.3 - StatusMisdirectedRequest = 421 // RFC 7540, 9.1.2 - StatusUnprocessableEntity = 422 // RFC 4918, 11.2 + StatusBadRequest = 400 // RFC 9110, 15.5.1 + StatusUnauthorized = 401 // RFC 9110, 15.5.2 + StatusPaymentRequired = 402 // RFC 9110, 15.5.3 + StatusForbidden = 403 // RFC 9110, 15.5.4 + StatusNotFound = 404 // RFC 9110, 15.5.5 + StatusMethodNotAllowed = 405 // RFC 9110, 15.5.6 + StatusNotAcceptable = 406 // RFC 9110, 15.5.7 + StatusProxyAuthRequired = 407 // RFC 9110, 15.5.8 + StatusRequestTimeout = 408 // RFC 9110, 15.5.9 + StatusConflict = 409 // RFC 9110, 15.5.10 + StatusGone = 410 // RFC 9110, 15.5.11 + StatusLengthRequired = 411 // RFC 9110, 15.5.12 + StatusPreconditionFailed = 412 // RFC 9110, 15.5.13 + StatusRequestEntityTooLarge = 413 // RFC 9110, 15.5.14 + StatusRequestURITooLong = 414 // RFC 9110, 15.5.15 + StatusUnsupportedMediaType = 415 // RFC 9110, 15.5.16 + StatusRequestedRangeNotSatisfiable = 416 // RFC 9110, 15.5.17 + StatusExpectationFailed = 417 // RFC 9110, 15.5.18 + StatusTeapot = 418 // RFC 9110, 15.5.19 (Unused) + StatusMisdirectedRequest = 421 // RFC 9110, 15.5.20 + StatusUnprocessableEntity = 422 // RFC 9110, 15.5.21 StatusLocked = 423 // RFC 4918, 11.3 StatusFailedDependency = 424 // RFC 4918, 11.4 StatusTooEarly = 425 // RFC 8470, 5.2. - StatusUpgradeRequired = 426 // RFC 7231, 6.5.15 + StatusUpgradeRequired = 426 // RFC 9110, 15.5.22 StatusPreconditionRequired = 428 // RFC 6585, 3 StatusTooManyRequests = 429 // RFC 6585, 4 StatusRequestHeaderFieldsTooLarge = 431 // RFC 6585, 5 StatusUnavailableForLegalReasons = 451 // RFC 7725, 3 - StatusInternalServerError = 500 // RFC 7231, 6.6.1 - StatusNotImplemented = 501 // RFC 7231, 6.6.2 - StatusBadGateway = 502 // RFC 7231, 6.6.3 - StatusServiceUnavailable = 503 // RFC 7231, 6.6.4 - StatusGatewayTimeout = 504 // RFC 7231, 6.6.5 - StatusHTTPVersionNotSupported = 505 // RFC 7231, 6.6.6 + StatusInternalServerError = 500 // RFC 9110, 15.6.1 + StatusNotImplemented = 501 // RFC 9110, 15.6.2 + StatusBadGateway = 502 // RFC 9110, 15.6.3 + StatusServiceUnavailable = 503 // RFC 9110, 15.6.4 + StatusGatewayTimeout = 504 // RFC 9110, 15.6.5 + StatusHTTPVersionNotSupported = 505 // RFC 9110, 15.6.6 StatusVariantAlsoNegotiates = 506 // RFC 2295, 8.1 StatusInsufficientStorage = 507 // RFC 4918, 11.5 StatusLoopDetected = 508 // RFC 5842, 7.2 @@ -76,77 +76,135 @@ const ( StatusNetworkAuthenticationRequired = 511 // RFC 6585, 6 ) -var statusText = map[int]string{ - StatusContinue: "Continue", - StatusSwitchingProtocols: "Switching Protocols", - StatusProcessing: "Processing", - StatusEarlyHints: "Early Hints", - - StatusOK: "OK", - StatusCreated: "Created", - StatusAccepted: "Accepted", - StatusNonAuthoritativeInfo: "Non-Authoritative Information", - StatusNoContent: "No Content", - StatusResetContent: "Reset Content", - StatusPartialContent: "Partial Content", - StatusMultiStatus: "Multi-Status", - StatusAlreadyReported: "Already Reported", - StatusIMUsed: "IM Used", - - StatusMultipleChoices: "Multiple Choices", - StatusMovedPermanently: "Moved Permanently", - StatusFound: "Found", - StatusSeeOther: "See Other", - StatusNotModified: "Not Modified", - StatusUseProxy: "Use Proxy", - StatusTemporaryRedirect: "Temporary Redirect", - StatusPermanentRedirect: "Permanent Redirect", - - StatusBadRequest: "Bad Request", - StatusUnauthorized: "Unauthorized", - StatusPaymentRequired: "Payment Required", - StatusForbidden: "Forbidden", - StatusNotFound: "Not Found", - StatusMethodNotAllowed: "Method Not Allowed", - StatusNotAcceptable: "Not Acceptable", - StatusProxyAuthRequired: "Proxy Authentication Required", - StatusRequestTimeout: "Request Timeout", - StatusConflict: "Conflict", - StatusGone: "Gone", - StatusLengthRequired: "Length Required", - StatusPreconditionFailed: "Precondition Failed", - StatusRequestEntityTooLarge: "Request Entity Too Large", - StatusRequestURITooLong: "Request URI Too Long", - StatusUnsupportedMediaType: "Unsupported Media Type", - StatusRequestedRangeNotSatisfiable: "Requested Range Not Satisfiable", - StatusExpectationFailed: "Expectation Failed", - StatusTeapot: "I'm a teapot", - StatusMisdirectedRequest: "Misdirected Request", - StatusUnprocessableEntity: "Unprocessable Entity", - StatusLocked: "Locked", - StatusFailedDependency: "Failed Dependency", - StatusTooEarly: "Too Early", - StatusUpgradeRequired: "Upgrade Required", - StatusPreconditionRequired: "Precondition Required", - StatusTooManyRequests: "Too Many Requests", - StatusRequestHeaderFieldsTooLarge: "Request Header Fields Too Large", - StatusUnavailableForLegalReasons: "Unavailable For Legal Reasons", - - StatusInternalServerError: "Internal Server Error", - StatusNotImplemented: "Not Implemented", - StatusBadGateway: "Bad Gateway", - StatusServiceUnavailable: "Service Unavailable", - StatusGatewayTimeout: "Gateway Timeout", - StatusHTTPVersionNotSupported: "HTTP Version Not Supported", - StatusVariantAlsoNegotiates: "Variant Also Negotiates", - StatusInsufficientStorage: "Insufficient Storage", - StatusLoopDetected: "Loop Detected", - StatusNotExtended: "Not Extended", - StatusNetworkAuthenticationRequired: "Network Authentication Required", -} - // StatusText returns a text for the HTTP status code. It returns the empty // string if the code is unknown. func StatusText(code int) string { - return statusText[code] + switch code { + case StatusContinue: + return "Continue" + case StatusSwitchingProtocols: + return "Switching Protocols" + case StatusProcessing: + return "Processing" + case StatusEarlyHints: + return "Early Hints" + case StatusOK: + return "OK" + case StatusCreated: + return "Created" + case StatusAccepted: + return "Accepted" + case StatusNonAuthoritativeInfo: + return "Non-Authoritative Information" + case StatusNoContent: + return "No Content" + case StatusResetContent: + return "Reset Content" + case StatusPartialContent: + return "Partial Content" + case StatusMultiStatus: + return "Multi-Status" + case StatusAlreadyReported: + return "Already Reported" + case StatusIMUsed: + return "IM Used" + case StatusMultipleChoices: + return "Multiple Choices" + case StatusMovedPermanently: + return "Moved Permanently" + case StatusFound: + return "Found" + case StatusSeeOther: + return "See Other" + case StatusNotModified: + return "Not Modified" + case StatusUseProxy: + return "Use Proxy" + case StatusTemporaryRedirect: + return "Temporary Redirect" + case StatusPermanentRedirect: + return "Permanent Redirect" + case StatusBadRequest: + return "Bad Request" + case StatusUnauthorized: + return "Unauthorized" + case StatusPaymentRequired: + return "Payment Required" + case StatusForbidden: + return "Forbidden" + case StatusNotFound: + return "Not Found" + case StatusMethodNotAllowed: + return "Method Not Allowed" + case StatusNotAcceptable: + return "Not Acceptable" + case StatusProxyAuthRequired: + return "Proxy Authentication Required" + case StatusRequestTimeout: + return "Request Timeout" + case StatusConflict: + return "Conflict" + case StatusGone: + return "Gone" + case StatusLengthRequired: + return "Length Required" + case StatusPreconditionFailed: + return "Precondition Failed" + case StatusRequestEntityTooLarge: + return "Request Entity Too Large" + case StatusRequestURITooLong: + return "Request URI Too Long" + case StatusUnsupportedMediaType: + return "Unsupported Media Type" + case StatusRequestedRangeNotSatisfiable: + return "Requested Range Not Satisfiable" + case StatusExpectationFailed: + return "Expectation Failed" + case StatusTeapot: + return "I'm a teapot" + case StatusMisdirectedRequest: + return "Misdirected Request" + case StatusUnprocessableEntity: + return "Unprocessable Entity" + case StatusLocked: + return "Locked" + case StatusFailedDependency: + return "Failed Dependency" + case StatusTooEarly: + return "Too Early" + case StatusUpgradeRequired: + return "Upgrade Required" + case StatusPreconditionRequired: + return "Precondition Required" + case StatusTooManyRequests: + return "Too Many Requests" + case StatusRequestHeaderFieldsTooLarge: + return "Request Header Fields Too Large" + case StatusUnavailableForLegalReasons: + return "Unavailable For Legal Reasons" + case StatusInternalServerError: + return "Internal Server Error" + case StatusNotImplemented: + return "Not Implemented" + case StatusBadGateway: + return "Bad Gateway" + case StatusServiceUnavailable: + return "Service Unavailable" + case StatusGatewayTimeout: + return "Gateway Timeout" + case StatusHTTPVersionNotSupported: + return "HTTP Version Not Supported" + case StatusVariantAlsoNegotiates: + return "Variant Also Negotiates" + case StatusInsufficientStorage: + return "Insufficient Storage" + case StatusLoopDetected: + return "Loop Detected" + case StatusNotExtended: + return "Not Extended" + case StatusNetworkAuthenticationRequired: + return "Network Authentication Required" + default: + return "" + } } diff --git a/tlsconn.go b/tlsconn.go index ae49a302..f564af73 100644 --- a/tlsconn.go +++ b/tlsconn.go @@ -21,6 +21,9 @@ type TLSConn interface { // HandshakeContext performs an TLS handshake bounded // in time by the given context. HandshakeContext(ctx context.Context) error + + // NetConn returns the underlying net.Conn + NetConn() net.Conn } // TLSClientFactory is the factory used when creating connections diff --git a/tools/compare.bash b/tools/compare.bash new file mode 100755 index 00000000..7d861247 --- /dev/null +++ b/tools/compare.bash @@ -0,0 +1,14 @@ +#!/bin/bash +set -euo pipefail +upstreamrepo=./upstreamrepo +TAG=$(cat UPSTREAM) +test -d $upstreamrepo || git clone git@github.com:golang/go.git $upstreamrepo +( + cd $upstreamrepo + git checkout master + git pull + git checkout $TAG +) +for file in $(cd $upstreamrepo/src/net/http && find . -type f -name \*.go); do + git diff --no-index $upstreamrepo/src/net/http/$file $file || true +done diff --git a/tools/merge.bash b/tools/merge.bash new file mode 100755 index 00000000..33a0727a --- /dev/null +++ b/tools/merge.bash @@ -0,0 +1,11 @@ +#!/bin/bash +set -euxo pipefail +git checkout main +git remote add golang git@github.com:golang/go.git || git fetch golang +git branch -D golang-upstream golang-http-upstream merged-main || true +git fetch golang +git checkout -b golang-upstream $(cat UPSTREAM) +git subtree split -P src/net/http/ -b golang-http-upstream +git checkout main +git checkout -b merged-main +git merge golang-http-upstream diff --git a/transfer.go b/transfer.go index aa65bd22..1af73793 100644 --- a/transfer.go +++ b/transfer.go @@ -196,10 +196,11 @@ func (t *transferWriter) shouldSendChunkedRequestBody() bool { // headers before the pipe is fed data), we need to be careful and bound how // long we wait for it. This delay will only affect users if all the following // are true: -// * the request body blocks -// * the content length is not set (or set to -1) -// * the method doesn't usually have a body (GET, HEAD, DELETE, ...) -// * there is no transfer-encoding=chunked already set. +// - the request body blocks +// - the content length is not set (or set to -1) +// - the method doesn't usually have a body (GET, HEAD, DELETE, ...) +// - there is no transfer-encoding=chunked already set. +// // In other words, this delay will not normally affect anybody, and there // are workarounds if it does. func (t *transferWriter) probeRequestBody() { @@ -421,8 +422,8 @@ func (t *transferWriter) doBodyCopy(dst io.Writer, src io.Reader) (n int64, err // // This function is only intended for use in writeBody. func (t *transferWriter) unwrapBody() io.Reader { - if reflect.TypeOf(t.Body) == nopCloserType { - return reflect.ValueOf(t.Body).Field(0).Interface().(io.Reader) + if r, ok := unwrapNopCloser(t.Body); ok { + return r } if r, ok := t.Body.(*readTrackingBody); ok { r.didRead = true @@ -467,6 +468,7 @@ func bodyAllowedForStatus(status int) bool { var ( suppressedHeaders304 = []string{"Content-Type", "Content-Length", "Transfer-Encoding"} suppressedHeadersNoBody = []string{"Content-Length", "Transfer-Encoding"} + excludedHeadersNoBody = map[string]bool{"Content-Length": true, "Transfer-Encoding": true} ) func suppressedHeaders(status int) []string { @@ -1080,6 +1082,21 @@ func (fr finishAsyncByteRead) Read(p []byte) (n int, err error) { } var nopCloserType = reflect.TypeOf(io.NopCloser(nil)) +var nopCloserWriterToType = reflect.TypeOf(io.NopCloser(struct { + io.Reader + io.WriterTo +}{})) + +// unwrapNopCloser return the underlying reader and true if r is a NopCloser +// else it return false +func unwrapNopCloser(r io.Reader) (underlyingReader io.Reader, isNopCloser bool) { + switch reflect.TypeOf(r) { + case nopCloserType, nopCloserWriterToType: + return reflect.ValueOf(r).Field(0).Interface().(io.Reader), true + default: + return nil, false + } +} // isKnownInMemoryReader reports whether r is a type known to not // block on Read. Its caller uses this as an optional optimization to @@ -1089,8 +1106,8 @@ func isKnownInMemoryReader(r io.Reader) bool { case *bytes.Reader, *bytes.Buffer, *strings.Reader: return true } - if reflect.TypeOf(r) == nopCloserType { - return isKnownInMemoryReader(reflect.ValueOf(r).Field(0).Interface().(io.Reader)) + if r, ok := unwrapNopCloser(r); ok { + return isKnownInMemoryReader(r) } if r, ok := r.(*readTrackingBody); ok { return isKnownInMemoryReader(r.ReadCloser) diff --git a/transfer_test.go b/transfer_test.go index f0c28b26..5e0df896 100644 --- a/transfer_test.go +++ b/transfer_test.go @@ -267,7 +267,7 @@ func TestTransferWriterWriteBodyReaderTypes(t *testing.T) { } if tc.expectedReader != actualReader { - t.Fatalf("got reader %T want %T", actualReader, tc.expectedReader) + t.Fatalf("got reader %s want %s", actualReader, tc.expectedReader) } } diff --git a/transport.go b/transport.go index 0e334cc3..56ee28de 100644 --- a/transport.go +++ b/transport.go @@ -162,7 +162,7 @@ type Transport struct { DialTLS func(network, addr string) (net.Conn, error) // TLSClientConfig specifies the TLS configuration to use with - // tls.Client. + // TLSClientFactory. // If nil, the default configuration is used. // If non-nil, HTTP/2 support may not be enabled by default. TLSClientConfig *tls.Config @@ -529,7 +529,8 @@ func (t *Transport) roundTrip(req *Request) (*Response, error) { for _, v := range vv { if !httpguts.ValidHeaderFieldValue(v) { req.closeBody() - return nil, fmt.Errorf("net/http: invalid header field value %q for key %v", v, k) + // Don't include the value in the error, because it may be sensitive. + return nil, fmt.Errorf("net/http: invalid header field value for %q", k) } } } @@ -1799,7 +1800,6 @@ var _ io.ReaderFrom = (*persistConnWriter)(nil) // socks5://proxy.com|https|foo.com socks5 to proxy, then https to foo.com // https://proxy.com|https|foo.com https to proxy, then CONNECT to foo.com // https://proxy.com|http https to proxy, http to anywhere after that -// type connectMethod struct { _ incomparable proxyURL *url.URL // nil for no proxy, else full proxy URL diff --git a/transport_test.go b/transport_test.go index 69e5e83b..c2520f35 100644 --- a/transport_test.go +++ b/transport_test.go @@ -47,7 +47,7 @@ import ( ) // TODO: test 5 pipelined requests with responses: 1) OK, 2) OK, Connection: Close -// and then verify that the final 2 responses get errors back. +// and then verify that the final 2 responses get errors back. // hostPortHandler writes back the client's "host:port". var hostPortHandler = HandlerFunc(func(w ResponseWriter, r *Request) { @@ -56,6 +56,12 @@ var hostPortHandler = HandlerFunc(func(w ResponseWriter, r *Request) { } w.Header().Set("X-Saw-Close", fmt.Sprint(r.Close)) w.Write([]byte(r.RemoteAddr)) + + // Include the address of the net.Conn in addition to the RemoteAddr, + // in case kernels reuse source ports quickly (see Issue 52450) + if c, ok := ResponseWriterConnForTesting(w); ok { + fmt.Fprintf(w, ", %T %p", c, c) + } }) // testCloseConn is a net.Conn tracked by a testConnSet. @@ -239,6 +245,12 @@ func TestTransportConnectionCloseOnResponse(t *testing.T) { connSet.check(t) } +// TestTransportConnectionCloseOnRequest tests that the Transport's doesn't reuse +// an underlying TCP connection after making an http.Request with Request.Close set. +// +// It tests the behavior by making an HTTP request to a server which +// describes the source source connection it got (remote port number + +// address of its net.Conn). func TestTransportConnectionCloseOnRequest(t *testing.T) { defer afterTest(t) ts := httptest.NewServer(hostPortHandler) @@ -249,7 +261,7 @@ func TestTransportConnectionCloseOnRequest(t *testing.T) { c := ts.Client() tr := c.Transport.(*Transport) tr.Dial = testDial - for _, connectionClose := range []bool{false, true} { + for _, reqClose := range []bool{false, true} { fetch := func(n int) string { req := new(Request) var err error @@ -261,29 +273,37 @@ func TestTransportConnectionCloseOnRequest(t *testing.T) { req.Proto = "HTTP/1.1" req.ProtoMajor = 1 req.ProtoMinor = 1 - req.Close = connectionClose + req.Close = reqClose res, err := c.Do(req) if err != nil { - t.Fatalf("error in connectionClose=%v, req #%d, Do: %v", connectionClose, n, err) + t.Fatalf("error in Request.Close=%v, req #%d, Do: %v", reqClose, n, err) } - if got, want := res.Header.Get("X-Saw-Close"), fmt.Sprint(connectionClose); got != want { - t.Errorf("For connectionClose = %v; handler's X-Saw-Close was %v; want %v", - connectionClose, got, !connectionClose) + if got, want := res.Header.Get("X-Saw-Close"), fmt.Sprint(reqClose); got != want { + t.Errorf("for Request.Close = %v; handler's X-Saw-Close was %v; want %v", + reqClose, got, !reqClose) } body, err := io.ReadAll(res.Body) if err != nil { - t.Fatalf("error in connectionClose=%v, req #%d, ReadAll: %v", connectionClose, n, err) + t.Fatalf("for Request.Close=%v, on request %v/2: ReadAll: %v", reqClose, n, err) } return string(body) } body1 := fetch(1) body2 := fetch(2) - bodiesDiffer := body1 != body2 - if bodiesDiffer != connectionClose { - t.Errorf("error in connectionClose=%v. unexpected bodiesDiffer=%v; body1=%q; body2=%q", - connectionClose, bodiesDiffer, body1, body2) + + got := 1 + if body1 != body2 { + got++ + } + want := 1 + if reqClose { + want = 2 + } + if got != want { + t.Errorf("for Request.Close=%v: server saw %v unique connections, wanted %v\n\nbodies were: %q and %q", + reqClose, got, want, body1, body2) } tr.CloseIdleConnections() @@ -2098,17 +2118,21 @@ func TestTransportConcurrency(t *testing.T) { for req := range reqs { res, err := c.Get(ts.URL + "/?echo=" + req) if err != nil { - t.Errorf("error on req %s: %v", req, err) + if runtime.GOOS == "netbsd" && strings.HasSuffix(err.Error(), ": connection reset by peer") { + // https://go.dev/issue/52168: this test was observed to fail with + // ECONNRESET errors in Dial on various netbsd builders. + t.Logf("error on req %s: %v", req, err) + t.Logf("(see https://go.dev/issue/52168)") + } else { + t.Errorf("error on req %s: %v", req, err) + } wg.Done() continue } all, err := io.ReadAll(res.Body) if err != nil { t.Errorf("read error on req %s: %v", req, err) - wg.Done() - continue - } - if string(all) != req { + } else if string(all) != req { t.Errorf("body of req %s = %q; want %q", req, all, req) } res.Body.Close() @@ -3434,6 +3458,7 @@ func (c writerFuncConn) Write(p []byte) (n int, err error) { return c.write(p) } // - we reused a keep-alive connection // - we haven't yet received any header data // - either we wrote no bytes to the server, or the request is idempotent +// // This automatically prevents an infinite resend loop because we'll run out of // the cached keep-alive connections eventually. func TestRetryRequestsOnError(t *testing.T) { @@ -5698,14 +5723,14 @@ func TestTransportClosesBodyOnInvalidRequests(t *testing.T) { Method: " ", URL: u, }, - wantErr: "invalid method", + wantErr: `invalid method " "`, }, { name: "nil URL", req: &Request{ Method: "GET", }, - wantErr: "nil Request.URL", + wantErr: `nil Request.URL`, }, { name: "invalid header key", @@ -5714,7 +5739,7 @@ func TestTransportClosesBodyOnInvalidRequests(t *testing.T) { Header: Header{"💡": {"emoji"}}, URL: u, }, - wantErr: "invalid header field name", + wantErr: `invalid header field name "💡"`, }, { name: "invalid header value", @@ -5723,7 +5748,7 @@ func TestTransportClosesBodyOnInvalidRequests(t *testing.T) { Header: Header{"key": {"\x19"}}, URL: u, }, - wantErr: "invalid header field value", + wantErr: `invalid header field value for "key"`, }, { name: "non HTTP(s) scheme", @@ -5731,7 +5756,7 @@ func TestTransportClosesBodyOnInvalidRequests(t *testing.T) { Method: "POST", URL: &url.URL{Scheme: "faux"}, }, - wantErr: "unsupported protocol scheme", + wantErr: `unsupported protocol scheme "faux"`, }, { name: "no Host in URL", @@ -5739,7 +5764,7 @@ func TestTransportClosesBodyOnInvalidRequests(t *testing.T) { Method: "POST", URL: &url.URL{Scheme: "http"}, }, - wantErr: "no Host", + wantErr: `no Host in request URL`, }, } @@ -5755,8 +5780,8 @@ func TestTransportClosesBodyOnInvalidRequests(t *testing.T) { if !bc { t.Fatal("Expected body to have been closed") } - if g, w := err.Error(), tt.wantErr; !strings.Contains(g, w) { - t.Fatalf("Error mismatch\n\t%q\ndoes not contain\n\t%q", g, w) + if g, w := err.Error(), tt.wantErr; !strings.HasSuffix(g, w) { + t.Fatalf("Error mismatch: %q does not end with %q", g, w) } }) } @@ -6143,7 +6168,7 @@ func TestCancelRequestWhenSharingConnection(t *testing.T) { r2c := <-reqc cancel() - // Give the cancelation a moment to take effect, and then unblock the first request. + // Give the cancellation a moment to take effect, and then unblock the first request. time.Sleep(1 * time.Millisecond) close(idlec)