Merge pull request #37 from ooni/update-to-go-1.19.4

Update to go 1.19.4

See ooni/probe#2273

Here's the diff with the `dev` branch (aka go1.19.4): https://github.com/ooni/oohttp/compare/2125b6b..82e690c

Inline you can see the diff with respect to `main` (aka go1.18.9)

We also include scripts to have more confidence in the merging process robustness.

.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 ./...

.gitignore

@@ -0,0 +1 @@
+/upstreamrepo

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 [email protected]: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;

UPSTREAM

@@ -0,0 +1 @@
+go1.19.4

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

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

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)
+			}
+		})
 	}
 }
 

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", "</style.css>; rel=preload; as=style")
+		h.Add("Link", "</script.js>; rel=preload; as=script")
+		w.WriteHeader(StatusEarlyHints)
+
+		wg.Wait()
+
+		h.Add("Link", "</foo.js>; 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{"</style.css>; rel=preload; as=style", "</script.js>; rel=preload; as=script"}, header["Link"])
+				checkExcludedHeaders(t, header)
+
+				wg.Done()
+			case 1:
+				checkLinkHeaders(t, []string{"</style.css>; rel=preload; as=style", "</script.js>; rel=preload; as=script", "</foo.js>; 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{"</style.css>; rel=preload; as=style", "</script.js>; rel=preload; as=script", "</foo.js>; 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)
+	}
+}

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.