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: HTTP server should respond with 100 Continue for zero-length "Expect: 100-continue" requests #16799

Closed
ghost opened this issue Aug 19, 2016 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@ghost
Copy link

ghost commented Aug 19, 2016

  1. What version of Go are you using (go version)?
    go1.6.3 linux/amd64
  2. What operating system and processor architecture are you using (go env)?
    GOARCH="amd64"
    GOOS="linux"
  3. What did you do?
    Make an HTTP request with the headers Expect: 100-continue and Content-Length: 0 to an http.Server. The server handler reads the (zero-length) response body and responds with a success status.
  4. What did you expect to see?
    A 100 Continue response on the wire, followed by a 200 OK.
  5. What did you see instead?
    Only a 200 OK response.

RFC 2616 says the following:

Upon receiving a request which includes an Expect request-header field with the "100-continue" expectation, an origin server MUST either respond with 100 (Continue) status and continue to read from the input stream, or respond with a final status code. The origin server MUST NOT wait for the request body before sending the 100 (Continue) response. If it responds with a final status code, it MAY close the transport connection or it MAY continue to read and discard the rest of the request. It MUST NOT perform the requested method if it returns a final status code.

The last sentence implies that, when an Expect: 100-continue request header is present and the request was successfully handled by the server, it's incorrect behavior for the server to send a 200 OK without a preceding 100 Continue.

http.Server does not exhibit the correct behavior with zero-length requests.

@ghost ghost changed the title HTTP server should respond with 100 Continue for zero-length requests HTTP server should respond with 100 Continue for zero-length "Expect: 100-continue" requests Aug 19, 2016
@bradfitz
Copy link
Contributor

Wow, that's a bizarre edge case.

But the RFC you cite is deprecated. The replacement seems like it's clarified the rules here:

https://tools.ietf.org/html/rfc7231#section-5.1.1

 A server MAY omit sending a 100 (Continue) response if it has
 already received some or all of the message body for the
 corresponding request, or if the framing indicates that there is
 no message body.

 A server that sends a 100 (Continue) response MUST ultimately send
 a final status code, once the message body is received and
 processed, unless the connection is closed prematurely.

 A server that responds with a final status code before reading the
 entire message body SHOULD indicate in that response whether it
 intends to close the connection or continue reading and discarding
 the request message (see Section 6.6 of [RFC7230]).

On a first skim, I'm not seeing the requirement in RFC 7231.

A client sending 100-continue for a 0-lengthed body is dumb, too. The documented intent of 100-continue is for clients who are sending large bodies.

I'm not sure this is worth fixing, or that Go is even out of compliance.

Do you see otherwise in RFC 7231?

@ghost
Copy link
Author

ghost commented Aug 19, 2016

RFC 7230 does seem to make a distinction between a zero-length body and no body:

A user agent SHOULD send a Content-Length in a request message when no Transfer-Encoding is sent and the request method defines a meaning for an enclosed payload body. For example, a Content-Length header field is normally sent in a POST request even when the value is 0 (indicating an empty payload body). A user agent SHOULD NOT send a Content-Length header field when the request message does not contain a payload body and the method semantics do not anticipate such a body.

By that, if a request explicitly has a Content-Length: 0 header, it does not have "framing indicat[ing] that there is no message body", but rather has a payload which is zero-length. As for whether the server "has already received some or all of the message body for the corresponding request", I'm not sure a server can be said to have received "some or all" of a zero-length payload, since definitionally it can't have read any of the payload. Thus, I don't think RFC 7231 exempts the server from sending the 100 Continue for zero-length requests.

That's being pedantic, but I think it's the right interpretation. I do agree with you that the client code is being dumb in either case, but I need to work with that dumb client code, and the http package does not provide any mechanism for changing this behavior from outside.

The fix is to remove the second condition from this if-statement. It shouldn't impact existing servers. If you agree that it's a small enough change and worthwhile, I'll submit a patch.

@bradfitz
Copy link
Contributor

The fix is to remove the second condition from this if-statement. It shouldn't impact existing servers. If you agree that it's a small enough change and worthwhile, I'll submit a patch.

Ignoring the RFC pedantry for a second, I don't even think that's enough of a fix. Consider how Go's 100-continue actually works: we don't send 100-continue to the peer unless/until the ServeHTTP handler does a Read from the Request.Body.

But if the request body is 0 bytes, it's very likely that either/both:

a) the user will look at Request.ContentLength == 0 and never do a Read at all,
b) the Reader type provided to the ServeHTTP handler will be limited to 0 bytes or a dummy noBodyReader value.

Most likely (a), though.

If this is important, you'd want to send the 100-continue to the peer unconditional on whether the ServeHTTP handler ever looks at RequestBody.

So the fix is a little bit more involved.

But writing a test is usually 10-50x more code than the fix itself, and a test is required for all HTTP changes too, otherwise it would just regress, and then there's no point fixing it in the first place. (Sorry, little digression, knee-jerking at the description of the fix as a "trivial (one-line) patch" from the email thread about this, since it's always more than 1 line)

Anyway, if you want to send that in, feel free.

@bradfitz bradfitz changed the title HTTP server should respond with 100 Continue for zero-length "Expect: 100-continue" requests net/http: HTTP server should respond with 100 Continue for zero-length "Expect: 100-continue" requests Aug 19, 2016
@bradfitz bradfitz added this to the Go1.8Maybe milestone Aug 19, 2016
@bradfitz
Copy link
Contributor

Paging @mnot for HTTP pedantry.

Mark, if an HTTP client sends "Expect: 100-continue" and also "Content-Length: 0", what should (or MUST NOT) a server do?

@mnot
Copy link

mnot commented Aug 19, 2016

A server MAY omit sending a 100 (Continue) response if it has already received some or all of the message body for the corresponding request, or if the framing indicates that there is no message body

That includes a zero-length body when the client has said it's sending zero bytes.

@ghost
Copy link
Author

ghost commented Aug 20, 2016

Sounds alright by me then. Still, I would like it if server handlers had more control over expectations. I'll play around with the code a bit and see if I can come up with something nice that won't break anything.

@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 10, 2016
@rsc
Copy link
Contributor

rsc commented Oct 20, 2016

@bradfitz, based on mnot's response, it sounds like the server is already behaving properly. Can we close this?

@golang golang locked and limited conversation to collaborators Oct 25, 2017
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

5 participants