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: expect: 100-continue handling is broken in various ways #67555

Open
neild opened this issue May 21, 2024 · 2 comments
Open

net/http: expect: 100-continue handling is broken in various ways #67555

neild opened this issue May 21, 2024 · 2 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@neild
Copy link
Contributor

neild commented May 21, 2024

This is a follow-up to #67382. See that issue for some prior discussion. (That issue is also full of watchflakes reports, so I'm letting it be about the test flakiness. This issue is about the underlying problem.)

An HTTP/1 or HTTP/2 client can send an Expect: 100-continue header in a request to indicate that it might not send the request body until the server asks for it. The client is permitted to send the request body anyway, either immediately or after a delay.

A server may respond to a Expect: 100-continue request with an informational 100 status, indicating that it would like the client to send the request body. A server may also respond with a non-1xx status, without waiting for the request body.

For HTTP/1, a client must send the body for a request or close the connection. If a client sends a Expect: 100-continue request and receives a non-1xx response from the server, it still needs to finish sending the current request's body before sending any further requests on the connection.

For HTTP/2, a client can reset a request's stream and continue to use the connection without sending the body.

This issue is about how our client and server implementations handle the case of a server responding to an Expect: 100-continue request with a non-1xx response.

Client: If we receive a non-1xx response to an Expect: 100-continue request, should we send the request body on the assumption that the server might want it? Or should we assume that receiving response headers from the server indicates that the server didn't need the body? Should we modify this behavior for different response codes? A 200 OK might indicate that the server is going to use the request body, but a 403 Forbidden probably does not. Should we modify this behavior based on the presence or absence of a Connection: close header in the response? (Note that for HTTP/1 requests, if we do not send the request body, we may not reuse the connection for further requests.)

Server: When handling an Expect: 100-continue request, we automatically send a 100 Continue response if the server handler reads from the request body. What should we do if the server handler calls WriteHeader to send a non-1xx response, and then reads from the request body? Should we assume the server handler knows what it's doing, and trust that the client is going to send us a request body? Or should we return an error from the body read?

Our current behavior is inconsistent and occasionally wrong.

The HTTP/1 client does not send the request body if it receives a non-1xx response before ExpectContinueTimeout expires. This can leave the connection in an invalid state. (Server responds with a non-1xx response and attempts to read the request body; client reuses the connection for another request without sending the body.)

The HTTP/2 client always sends the request body after ExpectContinueTimeout, regardless of whether it has received a non-1xx response or not. If the server is trying to read the body, it will eventually get it, but only after a timeout.

Both HTTP/1 and HTTP/2 servers appear to permit reading from a request body even after sending a non-1xx response (and no 100 Continue response).

I'm not certain yet what we should be doing, but at a minimum:

  • The HTTP/1 client should not send any further requests on a connection if it decides not to send a request body.
  • The HTTP/2 client should disable the ExpectContinueTimeout timer after receiving a non-1xx response. It should either send or not send the request body, but it shouldn't keep delaying.
  • We should make an intentional decision on whether a server handler reading from a Expect: 100-continue request body after sending response headers gets an error or not.
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/587155 mentions this issue: net/http: disable flaky 100-continue tests

gopherbot pushed a commit that referenced this issue May 21, 2024
Disable three 100-continue tests that aren't exercising the
intended behavior because they don't set ExpectContinueTimeout.
The tests are flaky right now; setting ExpectContinueTimeout
makes them consistently fail.

Set ExpectContinueTimeout and t.Skip the tests for now.

Fixes #67382
For #67555

Change-Id: I459a19a927e14af03881e89c73d20c93cf0da43e
Reviewed-on: https://go-review.googlesource.com/c/go/+/587155
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@cagedmantis cagedmantis added this to the Go1.23 milestone May 22, 2024
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 22, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/591255 mentions this issue: net/http: send body or close connection on expect-100-continue requests

gopherbot pushed a commit that referenced this issue Jun 6, 2024
When sending a request with an "Expect: 100-continue" header,
we must send the request body before sending any further requests
on the connection.

When receiving a non-1xx response to an "Expect: 100-continue" request,
send the request body if the connection isn't being closed after
processing the response. In other words, if either the request
or response contains a "Connection: close" header, then skip sending
the request body (because the connection will not be used for
further requests), but otherwise send it.

Correct a comment on the server-side Expect: 100-continue handling
that implied sending the request body is optional. It isn't.

For #67555

Change-Id: Ia2f12091bee697771087f32ac347509ec5922d54
Reviewed-on: https://go-review.googlesource.com/c/go/+/591255
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants