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
Closed

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

Lukasa opened this issue Nov 2, 2016 · 20 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@Lukasa
Copy link

Lukasa 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 the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 4, 2016
@quentinmit quentinmit added this to the Go1.9Maybe milestone Nov 4, 2016
@quentinmit
Copy link
Contributor

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

@bradfitz
Copy link
Contributor

bradfitz 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
Copy link
Author

Lukasa 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
Copy link
Contributor

bradfitz 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
Copy link
Author

Lukasa 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
Copy link
Contributor

/cc @tombergan for thoughts.

@tombergan
Copy link
Contributor

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
Copy link
Contributor

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.

@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 20, 2017
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 20, 2017
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9Maybe Jul 20, 2017
@tombergan
Copy link
Contributor

@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
Copy link
Contributor

  1. callback method on Request.Context value?

@tombergan
Copy link
Contributor

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

@vfaronov
Copy link

vfaronov 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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

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
Copy link

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
…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
Copy link
Contributor

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

@gopherbot
Copy link

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
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
Copy link

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
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
Copy link

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
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>
@golang golang locked and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants