Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: unexpected 1XX status codes are not handled well #17739

Closed
Lukasa opened this issue Nov 2, 2016 · 20 comments

Comments

Projects
None yet
7 participants
@Lukasa
Copy link

commented Nov 2, 2016

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.7.3 darwin/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/cory/Documents/Go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.7.3/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.7.3/libexec/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/nc/4r8rg3dx3h9b4t4b0bzcv8x40000gn/T/go-build564596711=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

Running the following Python test server:

import socket
import time

document = b'''<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="utf-8">
    <title>title</title>
    <link rel="stylesheet" href="/other/styles.css">
    <script src="/other/action.js"></script>
  </head>
  <body>
    <h1>Hello, world!</h1>
  </body>
</html>
'''

s = socket.socket()
s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
s.bind(('localhost', 8080))
s.listen(5)

while True:
    new_socket, _ = s.accept()
    data = b''

    while not data.endswith(b'\r\n\r\n'):
        data += new_socket.recv(8192)

    new_socket.sendall(
        b'HTTP/1.1 103 Early Hints\r\n'
        b'Content-Length: 0\r\n'
        b'Server: socketserver/1.0.0\r\n'
        b'Link: </other/styles.css>; rel=preload; as=style\r\n'
        b'Link: </other/action.js>; rel=preload; as=script\r\n'
        b'\r\n'
    )
    time.sleep(1)
    new_socket.sendall(
        b'HTTP/1.1 200 OK\r\n'
        b'Server: socketserver/1.0.0\r\n'
        b'Content-Type: text/html\r\n'
        b'Content-Length: %s\r\n'
        b'Link: </other/styles.css>; rel=preload; as=style\r\n'
        b'Link: </other/action.js>; rel=preload; as=script\r\n'
        b'Connection: close\r\n'
        b'\r\n' % len(document)
    )
    new_socket.sendall(document)
    new_socket.close()

I ran the following Go test client against it:

package main

import (
    "fmt"
    "io/ioutil"
    "net/http"
    "log"
)

func request(client *http.Client) {
    resp, err := client.Get("http://localhost:8080/")

    if err != nil {
        log.Fatal(err);
    }
    data, err := ioutil.ReadAll(resp.Body)
    resp.Body.Close()
    if err != nil {
        log.Fatal(err)
    }

    fmt.Printf("Status: %s", resp.Status)
    fmt.Printf("Body: %s", data)
}

func main() {
    transport := &http.Transport{}
    client := &http.Client{Transport: transport}
    request(client)
    request(client)
}

What did you expect to see?

I expected to see a 200 status code and the HTML body from the 200 response.

What did you see instead?

Go reported the 103 status code as final with no body, and did not provide access to the 200 response.

@josharian josharian changed the title net/http doesn't sensibly handle unexpected 1XX status codes net/http: unexpected 1XX status codes are not handled well Nov 3, 2016

@quentinmit quentinmit added this to the Go1.9Maybe milestone Nov 4, 2016

@quentinmit

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

/cc @bradfitz
I think this is too late to change for 1.8, since it's not a regression.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 8, 2016

Yeah, we don't support anything other than "100 Continue". I suppose the Transport could have an optional callback func to handle other 1xx responses.

@Lukasa, which 1xx responses are used in the wild? I've never really seen anything.

@bradfitz bradfitz self-assigned this Nov 8, 2016

@Lukasa

This comment has been minimized.

Copy link
Author

commented Nov 8, 2016

101 is obvious but a special case. 102 is used in WebDAV. Most importantly the HTTPbis is considering specifying a 103 for delivering Link headers early, and there has been push-back on having that response be negotiated like 100 or 101 are. So that would mean that, if specified, Go clients could hit it when communicating with web servers. Not ideal.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 8, 2016

Most importantly the HTTPbis is considering specifying a 103 for delivering Link headers early,

With or without something like Expect: 100-continue?

@Lukasa

This comment has been minimized.

Copy link
Author

commented Nov 8, 2016

I have strongly argued for with, but a couple of folks including Roy Fielding have argued very emphatically for without.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

/cc @tombergan for thoughts.

@tombergan

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

Yeah, I followed the ietf-http-wg thread that @Lukasa is referencing. I had intended to file a bug at the time, but I forgot, and it looks like @Lukasa beat me to it. Here are the relevant messages, with various degrees of curmudgeony:
https://lists.w3.org/Archives/Public/ietf-http-wg/2016OctDec/0327.html
https://lists.w3.org/Archives/Public/ietf-http-wg/2016OctDec/0366.html
https://lists.w3.org/Archives/Public/ietf-http-wg/2016OctDec/0350.html

RFC 7231 is clear that the Go library mishandles 1xx requests:
https://tools.ietf.org/html/rfc7231#section-6.2

A client MUST be able to parse one or more 1xx responses received prior to a final response, even if the client does not expect one. A user agent MAY ignore unexpected 1xx responses.

A proxy MUST forward 1xx responses unless the proxy itself requested the generation of the 1xx response.

Note the proxy case. In order to write a spec-compliant proxy in Go, the proxy needs to forward 1xx responses, which means the proxy must be allowed to call ResponseWrite.WriteHeader(1xx) (or some new API) multiple times.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 20, 2017

Okay, let's fix this in Go 1.10.

We probably want to put some bounds on the number of bytes and/or number of 1xx header messages.

@tombergan

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

@bradfitz, I can think of three ways to allow clients to handle 1xx responses:

  1. add a callback method to http.Transport
  2. add a callback method to http.Request
  3. add a callback to httptrace.ClientTrace.

I don't like (1) because it will be awkward to tie each callback to a request. Slight preference for (3) because that mechanism already exists. Thoughts?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 20, 2017

  1. callback method on Request.Context value?
@tombergan

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

Compared to (3), (4) has the disadvantage that we add a new API, but the advantage that (3) cannot import http types directly (like http.Header) due to an import cycle. So I like (4).

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017

@vfaronov

This comment has been minimized.

Copy link

commented Jan 29, 2018

I think this issue conflates a bigger, easier problem with a smaller, harder problem.

The bigger, easier problem is that the Go HTTP client is completely confused by 1xx responses other than a single 100 (Continue), in clear violation of the protocol, hampering interoperability with RFC 8297 among other things. It can be trivially fixed by replacing if resp.StatusCode == 100 with for resp.StatusCode >= 100 && resp.StatusCode <= 199 in src/net/http/transport.go.

The smaller, harder problem is that the Go HTTP client doesn’t provide an API for 1xx responses, which precludes building a correct HTTP proxy on top of it. But I don’t think this should prevent or delay fixing the bigger, easier problem. As far as I’m aware, the Go distribution doesn’t even include an HTTP proxy (httputil.ReverseProxy is not a proxy in the language of RFC 723x — it’s a gateway).

Would you like me to submit the above fix to transport.go?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

httputil.ReverseProxy is not a proxy in the language of RFC 723x — it’s a gateway

The RFC you linked says:

A "gateway" (a.k.a. "reverse proxy")

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

Would you like me to submit the above fix to transport.go?

If you'd like, but I was also planning on working on this during the Feb/Mar/Apr dev cycle for Go 1.11.

@vfaronov

This comment has been minimized.

Copy link

commented Jan 29, 2018

The RFC you linked says:

A "gateway" (a.k.a. "reverse proxy")

Yes, but it also says:

A "proxy" is a message-forwarding agent that is selected by the client

and elsewhere the RFC 723x series of documents distinguishes clearly between proxies and gateways, so the text quoted by @tombergan should not be taken as applying to gateways.

@gopherbot

This comment has been minimized.

Copy link

commented Jun 6, 2018

Change https://golang.org/cl/116855 mentions this issue: net/http, net/http/httptrace: make Transport support 1xx responses properly

gopherbot pushed a commit that referenced this issue Jun 12, 2018

net/http, net/http/httptrace: make Transport support 1xx responses pr…
…operly

Previously the Transport had good support for 100 Continue responses,
but other 1xx informational responses were returned as-is.

But per https://tools.ietf.org/html/rfc7231#section-6.2:

> A client MUST be able to parse one or more 1xx responses received
> prior to a final response, even if the client does not expect one. A
> user agent MAY ignore unexpected 1xx responses.

We weren't doing that. Instead, we were returning any 1xx that wasn't
100 as the final result.

With this change we instead loop over up to 5 (arbitrary) 1xx
responses until we find the final one, returning an error if there's
more than 5. The limit is just there to guard against malicious
servers and to have _some_ limit.

By default we ignore the 1xx responses, unless the user defines the
new httptrace.ClientTrace.Got1xxResponse hook, which is an expanded
version of the previous ClientTrace.Got100Continue.

Still remaining:

* httputil.ReverseProxy work. (From rfc7231#section-6.2: "A proxy MUST
  forward 1xx responses unless the proxy itself requested the
  generation of the 1xx response."). Which would require:

* Support for an http.Handler to generate 1xx informational responses.

Those can happen later. Fixing the Transport to be resilient to others
using 1xx in the future without negotiation (as is being discussed
with HTTP status 103) is most important for now.

Updates #17739

Change-Id: I55aae8cd978164643fccb9862cd60a230e430486
Reviewed-on: https://go-review.googlesource.com/116855
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

This is fixed enough by CL 116855 above. I'll file separate bugs for the remaining bits.

@gopherbot

This comment has been minimized.

Copy link

commented Jul 12, 2018

Change https://golang.org/cl/123615 mentions this issue: http2: ignore unknown 1xx responses like HTTP/1

gopherbot pushed a commit to golang/net that referenced this issue Jul 12, 2018

http2: ignore unknown 1xx responses like HTTP/1
Updates golang/go#26189 (fixes after vendor into std)
Updates golang/go#17739

Change-Id: I076cdbb57841b7dbbaa764d11240913bc3a8b05d
Reviewed-on: https://go-review.googlesource.com/123615
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Jul 12, 2018

Change https://golang.org/cl/123675 mentions this issue: http2: fix bug in earlier CL 123615

gopherbot pushed a commit to golang/net that referenced this issue Jul 12, 2018

http2: fix bug in earlier CL 123615
I both forgot that this list could contain duplicates, and I had
forgot to run the net/http tests against CL 123615 before mailing it,
which ended up catching this bug.

The diff of this file from the commit before CL 123615 (a45b4ab^ ==
cffdcf6) to this commit is:

    https://gist.github.com/bradfitz/0b7abf8fa421515aed9c4d55ce3a1994

... effectively reverting much of CL 123615 and just moving the 1xx
handling down lower.

Updates golang/go#26189 (fixes after vendor into std)
Updates golang/go#17739

Change-Id: Ib63060b0bb9721883b4b91a983b6e117994faeb9
Reviewed-on: https://go-review.googlesource.com/123675
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Jul 12, 2018

Change https://golang.org/cl/123676 mentions this issue: net/http: update bundled http2

gopherbot pushed a commit that referenced this issue Jul 12, 2018

net/http: update bundled http2
Updates bundled x/net/http2 to git rev d0887baf81f4 for:

    http2: ignore unknown 1xx responses like HTTP/1
    https://golang.org/cl/123615

    http2: fix bug in earlier CL 123615
    https://golang.org/cl/123675

Also along for the ride, but without any effect:

    http2: export a field of an internal type for use by net/http
    https://golang.org/cl/123656

Fixes #26189
Updates #17739

Change-Id: I1955d844d74113efbcbbdaa7d7a7faebb2225b45
Reviewed-on: https://go-review.googlesource.com/123676
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.