Skip to content

Commit 493a262

Browse files
bradfitzbradfitz
committed
http2: don't send Connection-level headers in Transport
Accept common things that users might try to do to be helpful (managed by net/http anyway, and previously legal or at best ignored), like Connection: close Connection: keep-alive Transfer-Encoding: chunked But reject all other connection-level headers, per http2 spec. The Google GFE enforces this, so we need to filter these before sending, and give users a better error message for the ones we can't safely filter. That is, reject any connection-level header that we don't know the meaning of. This CL also makes "Connection: close" mean the same as Request.Close, and respects that as well, which was previously ignored in http2. Mostly tests. Updates golang/go#14227 Change-Id: I06e20286f71e8416149588e2c6274a3fce68033b Reviewed-on: https://go-review.googlesource.com/19223 Reviewed-by: Andrew Gerrand <adg@golang.org> Reviewed-by: Russ Cox <rsc@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
1 parent 6ccd669 commit 493a262

File tree

2 files changed

+159
-13
lines changed

2 files changed

+159
-13
lines changed

‎http2/transport.go

+44-9
Original file line numberDiff line numberDiff line change
@@ -562,7 +562,27 @@ func (cc *ClientConn) responseHeaderTimeout() time.Duration {
562562
return 0
563563
}
564564

565+
// checkConnHeaders checks whether req has any invalid connection-level headers.
566+
// per RFC 7540 section 8.1.2.2: Connection-Specific Header Fields.
567+
// Certain headers are special-cased as okay but not transmitted later.
568+
func checkConnHeaders(req *http.Request) error {
569+
if v := req.Header.Get("Upgrade"); v != "" {
570+
return errors.New("http2: invalid Upgrade request header")
571+
}
572+
if v := req.Header.Get("Transfer-Encoding"); (v != "" && v != "chunked") || len(req.Header["Transfer-Encoding"]) > 1 {
573+
return errors.New("http2: invalid Transfer-Encoding request header")
574+
}
575+
if v := req.Header.Get("Connection"); (v != "" && v != "close" && v != "keep-alive") || len(req.Header["Connection"]) > 1 {
576+
return errors.New("http2: invalid Connection request header")
577+
}
578+
return nil
579+
}
580+
565581
func (cc *ClientConn) RoundTrip(req *http.Request) (*http.Response, error) {
582+
if err := checkConnHeaders(req); err != nil {
583+
return nil, err
584+
}
585+
566586
trailers, err := commaSeparatedTrailers(req)
567587
if err != nil {
568588
return nil, err
@@ -914,13 +934,24 @@ func (cc *ClientConn) encodeHeaders(req *http.Request, addGzipHeader bool, trail
914934
var didUA bool
915935
for k, vv := range req.Header {
916936
lowKey := strings.ToLower(k)
917-
if lowKey == "host" || lowKey == "content-length" {
937+
switch lowKey {
938+
case "host", "content-length":
939+
// Host is :authority, already sent.
940+
// Content-Length is automatic, set below.
918941
continue
919-
}
920-
if lowKey == "user-agent" {
942+
case "connection", "proxy-connection", "transfer-encoding", "upgrade":
943+
// Per 8.1.2.2 Connection-Specific Header
944+
// Fields, don't send connection-specific
945+
// fields. We deal with these earlier in
946+
// RoundTrip, deciding whether they're
947+
// error-worthy, but we don't want to mutate
948+
// the user's *Request so at this point, just
949+
// skip over them at this point.
950+
continue
951+
case "user-agent":
921952
// Match Go's http1 behavior: at most one
922-
// User-Agent. If set to nil or empty string,
923-
// then omit it. Otherwise if not mentioned,
953+
// User-Agent. If set to nil or empty string,
954+
// then omit it. Otherwise if not mentioned,
924955
// include the default (below).
925956
didUA = true
926957
if len(vv) < 1 {
@@ -1030,8 +1061,9 @@ func (cc *ClientConn) streamByID(id uint32, andRemove bool) *clientStream {
10301061

10311062
// clientConnReadLoop is the state owned by the clientConn's frame-reading readLoop.
10321063
type clientConnReadLoop struct {
1033-
cc *ClientConn
1034-
activeRes map[uint32]*clientStream // keyed by streamID
1064+
cc *ClientConn
1065+
activeRes map[uint32]*clientStream // keyed by streamID
1066+
closeWhenIdle bool
10351067

10361068
hdec *hpack.Decoder
10371069

@@ -1091,7 +1123,7 @@ func (rl *clientConnReadLoop) cleanup() {
10911123

10921124
func (rl *clientConnReadLoop) run() error {
10931125
cc := rl.cc
1094-
closeWhenIdle := cc.t.disableKeepAlives()
1126+
rl.closeWhenIdle = cc.t.disableKeepAlives()
10951127
gotReply := false // ever saw a reply
10961128
for {
10971129
f, err := cc.fr.ReadFrame()
@@ -1140,7 +1172,7 @@ func (rl *clientConnReadLoop) run() error {
11401172
if err != nil {
11411173
return err
11421174
}
1143-
if closeWhenIdle && gotReply && maybeIdle && len(rl.activeRes) == 0 {
1175+
if rl.closeWhenIdle && gotReply && maybeIdle && len(rl.activeRes) == 0 {
11441176
cc.closeIfIdle()
11451177
}
11461178
}
@@ -1407,6 +1439,9 @@ func (rl *clientConnReadLoop) endStream(cs *clientStream) {
14071439
}
14081440
cs.bufPipe.closeWithErrorAndCode(err, code)
14091441
delete(rl.activeRes, cs.ID)
1442+
if cs.req.Close || cs.req.Header.Get("Connection") == "close" {
1443+
rl.closeWhenIdle = true
1444+
}
14101445
}
14111446

14121447
func (cs *clientStream) copyTrailers() {

‎http2/transport_test.go

+115-4
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"net/url"
2121
"os"
2222
"reflect"
23+
"sort"
2324
"strconv"
2425
"strings"
2526
"sync"
@@ -100,8 +101,7 @@ func TestTransport(t *testing.T) {
100101
t.Errorf("Body = %q; want %q", slurp, body)
101102
}
102103
}
103-
104-
func TestTransportReusesConns(t *testing.T) {
104+
func onSameConn(t *testing.T, modReq func(*http.Request)) bool {
105105
st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
106106
io.WriteString(w, r.RemoteAddr)
107107
}, optOnlyServer)
@@ -113,6 +113,7 @@ func TestTransportReusesConns(t *testing.T) {
113113
if err != nil {
114114
t.Fatal(err)
115115
}
116+
modReq(req)
116117
res, err := tr.RoundTrip(req)
117118
if err != nil {
118119
t.Fatal(err)
@@ -130,8 +131,24 @@ func TestTransportReusesConns(t *testing.T) {
130131
}
131132
first := get()
132133
second := get()
133-
if first != second {
134-
t.Errorf("first and second responses were on different connections: %q vs %q", first, second)
134+
return first == second
135+
}
136+
137+
func TestTransportReusesConns(t *testing.T) {
138+
if !onSameConn(t, func(*http.Request) {}) {
139+
t.Errorf("first and second responses were on different connections")
140+
}
141+
}
142+
143+
func TestTransportReusesConn_RequestClose(t *testing.T) {
144+
if onSameConn(t, func(r *http.Request) { r.Close = true }) {
145+
t.Errorf("first and second responses were not on different connections")
146+
}
147+
}
148+
149+
func TestTransportReusesConn_ConnClose(t *testing.T) {
150+
if onSameConn(t, func(r *http.Request) { r.Header.Set("Connection", "close") }) {
151+
t.Errorf("first and second responses were not on different connections")
135152
}
136153
}
137154

@@ -1549,3 +1566,97 @@ func TestTransportDisableCompression(t *testing.T) {
15491566
}
15501567
defer res.Body.Close()
15511568
}
1569+
1570+
// RFC 7540 section 8.1.2.2
1571+
func TestTransportRejectsConnHeaders(t *testing.T) {
1572+
st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
1573+
var got []string
1574+
for k := range r.Header {
1575+
got = append(got, k)
1576+
}
1577+
sort.Strings(got)
1578+
w.Header().Set("Got-Header", strings.Join(got, ","))
1579+
}, optOnlyServer)
1580+
defer st.Close()
1581+
1582+
tr := &Transport{TLSClientConfig: tlsConfigInsecure}
1583+
defer tr.CloseIdleConnections()
1584+
1585+
tests := []struct {
1586+
key string
1587+
value []string
1588+
want string
1589+
}{
1590+
{
1591+
key: "Upgrade",
1592+
value: []string{"anything"},
1593+
want: "ERROR: http2: invalid Upgrade request header",
1594+
},
1595+
{
1596+
key: "Connection",
1597+
value: []string{"foo"},
1598+
want: "ERROR: http2: invalid Connection request header",
1599+
},
1600+
{
1601+
key: "Connection",
1602+
value: []string{"close"},
1603+
want: "Accept-Encoding,User-Agent",
1604+
},
1605+
{
1606+
key: "Connection",
1607+
value: []string{"close", "something-else"},
1608+
want: "ERROR: http2: invalid Connection request header",
1609+
},
1610+
{
1611+
key: "Connection",
1612+
value: []string{"keep-alive"},
1613+
want: "Accept-Encoding,User-Agent",
1614+
},
1615+
{
1616+
key: "Proxy-Connection", // just deleted and ignored
1617+
value: []string{"keep-alive"},
1618+
want: "Accept-Encoding,User-Agent",
1619+
},
1620+
{
1621+
key: "Transfer-Encoding",
1622+
value: []string{""},
1623+
want: "Accept-Encoding,User-Agent",
1624+
},
1625+
{
1626+
key: "Transfer-Encoding",
1627+
value: []string{"foo"},
1628+
want: "ERROR: http2: invalid Transfer-Encoding request header",
1629+
},
1630+
{
1631+
key: "Transfer-Encoding",
1632+
value: []string{"chunked"},
1633+
want: "Accept-Encoding,User-Agent",
1634+
},
1635+
{
1636+
key: "Transfer-Encoding",
1637+
value: []string{"chunked", "other"},
1638+
want: "ERROR: http2: invalid Transfer-Encoding request header",
1639+
},
1640+
{
1641+
key: "Content-Length",
1642+
value: []string{"123"},
1643+
want: "Accept-Encoding,User-Agent",
1644+
},
1645+
}
1646+
1647+
for _, tt := range tests {
1648+
req, _ := http.NewRequest("GET", st.ts.URL, nil)
1649+
req.Header[tt.key] = tt.value
1650+
res, err := tr.RoundTrip(req)
1651+
var got string
1652+
if err != nil {
1653+
got = fmt.Sprintf("ERROR: %v", err)
1654+
} else {
1655+
got = res.Header.Get("Got-Header")
1656+
res.Body.Close()
1657+
}
1658+
if got != tt.want {
1659+
t.Errorf("For key %q, value %q, got = %q; want %q", tt.key, tt.value, got, tt.want)
1660+
}
1661+
}
1662+
}

0 commit comments

Comments
 (0)