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

Disallow users to send a message-body and Content-Length header field for 1xx, 204 and 304 responses #6261

Merged

Conversation

Projects
None yet
5 participants
@monkey-mas
Copy link
Contributor

commented Jun 22, 2016

Pull Request Checklist

Fixes

This fix makes sure that Play never sends Content-Length header field for a 204 response.

Purpose

As described in RFC7230 section-3.3.2, sending a Content-Length header field is not allowed in any response with a status code of 1xx (Informational) or 204 (No Content). This requirement was not forced in Play, which means a user can intentionally set a Content-Length header. To follow the RFC section, we need to make sure that we don't send the header field.
With regard to a Content-Length header field in a 304 response, a server May send it as mentioned in RFC7230 section-3.3.2. Thus it'd be better for us to allow users to set the field if they like.
With regard to a Content-Length header field in a 304 response, we don't allow users to do it currently even a server May send it as mentioned in RFC7230 section-3.3.2.

Additionally, as described in RFC7230 section-3.3, 1xx responses don't include a message body. With this commit, a message body isn't sent to meet the requirement.

Background Context

As I've mentioned in Purpose.

References

A server MUST NOT send a Content-Length header field in any response with
a status code of 1xx (Informational) or 204 (No Content). A server MUST NOT send
a Content-Length header field in any 2xx(Successful) response to a CONNECT request
(Section 4.3.6 of [RFC7231]).
A server MAY send a Content-Length header field in a 304 (Not Modified) response to
a conditional GET request (Section 4.1 of [RFC7232]); a server MUST NOT send
Content-Length in such a response unless its field-value equals the decimal number of
octets that would have been sent in the payload body of a 200 (OK) response to
the same request.
All 1xx (Informational), 204 (No Content), and 304 (Not Modified) responses do not
include a message body. All other responses do include a message body, although
the body might be of zero length.

P.S.
play.it.http.NettyScalaResultsHandlingSpec appends a Content-Length: 0 header field
when we explicitly send it, but AkkaHttpIntegrationSpecification does not.

@gmethvin

This comment has been minimized.

Copy link
Member

commented Jun 25, 2016

Hi @monkey-mas,

I think this is something we should issue a warning for, the same way we do if you return a HttpEntity with a content length and also set the Content-Length header. Generally there's not much of a reason to set the Content-Length header directly, so if you're doing that it's likely to be a bug in your code.

@monkey-mas

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2016

Hi @gmethvin,

Generally there's not much of a reason to set the Content-Length header directly, so if you're doing that it's likely to be a bug in your code.

Yeah, most of the cases there isn't. One exception is a 304 response like I've mentioned as it's valid according to RFC7230. AFAIU, Play doesn't send a Content-Length header field if a message-body is empty. This means a user can't send a Content-Length header field with a value of 0 by default even they want. By sending the header field "explicitly" like you can find from the test case, users can achieve it.

With that said, we need to decide whether or not we follow the RFC, and what we expect Play to enforce on behalf of its users.

IMO, it'd be better to achieve what I've explained because 1)Play actually allows to add a header field, and 2)even though it's likely to be a bug to set a Content-Length header field directly, we can do validation to keep users from doing something harmful to clients. I think it's quite fair to assume users might do something wrong and disallow them to do it by using validtaion. WDYT?

@gmethvin

This comment has been minimized.

Copy link
Member

commented Jun 28, 2016

AFAIU, Play doesn't send a Content-Length header field if a message-body is empty. This means a user can't send a Content-Length header field with a value of 0 by default even they want.

Actually by default if you send an Ok response with no body, it will send it with a body of content length 0. And that's fine since 200 OK needs to have a body. Generally it's the Play server's job to decide if a body should be sent for a particular response and either set the content length header or not.

Still, in the case of 204 No Content I don't see what a Content-Length would mean. If there is no content, then there should be no body. So it's very likely a mistake.

@monkey-mas

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2016

Thanks for your reply!

OK, can you please give me your idea to solve this issue, i.e., actual modification to our code if necessary or where we want to stick Play server's job with regard to this problem?

Thanks in advance :)

cancelEntity(result.body)
Future.successful(result.copy(body = HttpEntity.Strict(ByteString.empty, result.body.contentType)))
val _result = {
val r = result.copy(body = HttpEntity.Strict(ByteString.empty, result.body.contentType))

This comment has been minimized.

Copy link
@gmethvin

gmethvin Jun 28, 2016

Member

HttpEntity.NoEntity

This comment has been minimized.

Copy link
@monkey-mas

monkey-mas Jun 29, 2016

Author Contributor

Yeah, sounds reasonable :). we don't have to care about Content-Type as the entity is empty?

This comment has been minimized.

Copy link
@gmethvin

gmethvin Jun 30, 2016

Member

Actually we still want the content type since it will be used afterward. So we could instead use

val r = result.copy(body = HttpEntity.NoEntity.as(result.body.contentType))

but actually what you have is fine. At first glance it looked like using NoEntity would be simpler but it doesn't save you much code.

This comment has been minimized.

Copy link
@monkey-mas

monkey-mas Jun 30, 2016

Author Contributor

Oh, I see. Thanks, will fix it :)

@gmethvin

This comment has been minimized.

Copy link
Member

commented Jun 28, 2016

@monkey-mas I was arguing for issuing a warning if the user has set a Content-Length for a 204, then removing the header. Do you agree? In the context of the original pull request that's all I think needs to be done.

With respect to the 304 response and similar cases where you need to send a Content-Length with no entity, it would be nice if we built the optional Content-Length functionality into a new type of HttpEntity. That would require some API changes but it would be clearer and more type safe than setting the Content-Length header.

@monkey-mas

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2016

@gmethvin

I was arguing for issuing a warning if the user has set a Content-Length for a 204, then removing the header. Do you agree?

Yes! So you mean I just need to log a warning for users, correct?

With respect to the 304 response and similar cases where you need to send a Content-Length with no entity, it would be nice if we built the optional Content-Length functionality into a new type of HttpEntity. That would require some API changes but it would be clearer and more type safe than setting the Content-Length header.

Honestly, I'm not quite sure what you mean we built the optional Content-Length functionality into a new type of HttpEntity. You mean something like this to create a 304 response containing a Content-Length with a value of 0? Can you please explain how it works to solve the problem from the implementation point of view a little bit more?
BTW, I tried a little bit casual solution to handle a 304 response when it's explicitly added by users.

* @param keys Keys to remove from headers
* @return A copy of this result with `keys` removed from its headers.
*/
def removingFromHeaders(keys: String*): Result = {

This comment has been minimized.

Copy link
@gmethvin

gmethvin Jun 30, 2016

Member

Perhaps this should be called withoutHeaders to match withHeaders?

This comment has been minimized.

Copy link
@monkey-mas

monkey-mas Jun 30, 2016

Author Contributor

Yeah, antonym is better I suppose :)

@gmethvin

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

@monkey-mas I think it would be better to discuss and handle these API changes on a separate issue/PR. But what I mean is we create a new type of HttpEntity that holds no data but still has a length. This way there would be no need for a user to deal with the raw header API and we can always use the length from the HttpEntity. This "placeholder" entity could be used in a 304 response or a response to a HEAD request, for example.

In the case of a 304 response, I don't see why you would want to set the Content-Length to 0. The header should either be not included at all, or be set to the length of the content that would have been returned in a 2xx response. So as I understand it the compromise in #3939 was to allow the user to set it if they want to, but otherwise Play won't set it.

r.removingFromHeaders(HeaderNames.CONTENT_LENGTH)
case _ if r.header.headers.contains(HeaderNames.CONTENT_LENGTH) =>
//Make sure a Content-Length header field is set with a value of 0 if it is explicitly sent for a 304 response
r.withHeaders((HeaderNames.CONTENT_LENGTH, "0"))

This comment has been minimized.

Copy link
@gmethvin

gmethvin Jun 30, 2016

Member

We don't actually know that the length of the cached content is 0 here. Not sure why we are overriding the header.

This comment has been minimized.

Copy link
@monkey-mas

monkey-mas Jun 30, 2016

Author Contributor

You're right! I thought a 304 response has no-body indicating its Content-Length is always 0, which is wrong. RFC 7230#section-3.3.2 section actually explains as follows (which you mean the Content-Length should be equal to the length of the cached content?):
a server MUST NOT send Content-Length in such a response unless its field-value equals the decimal number of octets that would have been sent in the payload body of a 200 (OK) response to the same request.

@gmethvin

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

Also, we could do this in NettyModelConversion. That's where our existing logic for handling Content-Length is.

akka-http is supposed to handle more of this stuff for us, but you may have to change the akka-http server as well to make sure it works. Though it's not a major concern since the akka-http server is still experimental.

@monkey-mas

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2016

@gmethvin
Thanks for your detailed explanation! Now I kind of understand what you mean I suppose :)

I think it would be better to discuss and handle these API changes on a separate issue/PR.

Do you mean we can have a discussion about API changes related to a 304 response in a separate issue/PR? Or changes of both 204 and 304? If you think the modification related to a 204 response is ok to be merged in this PR, then I'll fix #6261 (comment) and tests, and remove 41d5bcb.

@gmethvin

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

@monkey-mas I agree on fixing 204, as discussed in this PR. The other API changes we can discuss elsewhere. Maybe it would be best to create a mailing list thread first and get some feedback from the community.

@monkey-mas

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2016

@gmethvin
All right, I've done additional fix and hope it's ready to be reviewed.

The other API changes we can discuss elsewhere. Maybe it would be best to create a mailing list thread first and get some feedback from the community.

Sure, we can move the topic to mailing list :)

} else if (!mayHaveEntity(result.header.status) && !result.body.isKnownEmpty) {
cancelEntity(result.body)
Future.successful(result.copy(body = HttpEntity.Strict(ByteString.empty, result.body.contentType)))
} else if (mustNotIncludeBody(result.header.status)) {

This comment has been minimized.

Copy link
@gmethvin

gmethvin Jul 1, 2016

Member

Can we move this to NettyModelConversion#convertResult? That is where the rest of the Content-Length logic is. Also I'm not as concerned about the akka-http server here since they generally take responsibility for following the spec than Netty does (and the Play akka-http server is experimental).

This comment has been minimized.

Copy link
@monkey-mas

monkey-mas Jul 6, 2016

Author Contributor

Sure, makes sense to me. Then we can keep ServerResultUtils simple :)

val r = result.copy(body = HttpEntity.Strict(ByteString.empty, result.body.contentType))

result.header.status match {
case Status.NO_CONTENT if r.header.headers.contains(HeaderNames.CONTENT_LENGTH) =>

This comment has been minimized.

Copy link
@gmethvin

gmethvin Jul 1, 2016

Member

Maybe also include 1xx responses?

This comment has been minimized.

Copy link
@monkey-mas

monkey-mas Jul 6, 2016

Author Contributor

Yap 👍

} else if (mustNotIncludeBody(result.header.status)) {

if (!result.body.isKnownEmpty) {
logger.warn(s"Response with a status code of ${result.header.status} must not have a body-message thus the content will be removed.")

This comment has been minimized.

Copy link
@gmethvin

gmethvin Jul 1, 2016

Member

I don't think we should have this warning. We could have an empty body that is not known empty, for example an empty streamed body. The responsibility of this code should just be to cancel the stream so no body is sent.

This comment has been minimized.

Copy link
@monkey-mas

monkey-mas Jul 6, 2016

Author Contributor

Oh, thanks for letting me know. Umm.. I think it's worth letting users know that they've added wrong body data to a 204 response for example but we have some corner case as you explained to me? So, yes it'd be better remove the warning.


result.header.status match {
case Status.NO_CONTENT if r.header.headers.contains(HeaderNames.CONTENT_LENGTH) =>
logger.warn(s"Response with a status code of ${result.header.status} must not have a Content-Length header field thus the field will be removed.")

This comment has been minimized.

Copy link
@gmethvin

gmethvin Jul 1, 2016

Member

This warning could be added as an else case here: https://github.com/playframework/playframework/blob/master/framework/src/play-netty-server/src/main/scala/play/core/server/netty/NettyModelConversion.scala#L207, and I would write something like:

} else if (HttpHeaders.isContentLengthSet(response) && mayNotHaveContentLength(response.header.status)) {
  val manualContentLength = response.headers.get(CONTENT_LENGTH)
  logger.warn(s"Ignoring manual Content-Length ($manualContentLength) since it is not allowed for ${result.header.status} responses.")
  response.headers.remove(CONTENT_LENGTH)
}

This comment has been minimized.

Copy link
@gmethvin

gmethvin Jul 1, 2016

Member

And I think mayHaveContentLength in NettyModelConversion should be called mayHaveEntity, since that's what it's actually doing. It probably should be updated for 1xx responses too.

This comment has been minimized.

Copy link
@monkey-mas

monkey-mas Jul 6, 2016

Author Contributor

Okay, fixed as you suggested :)

@gmethvin

This comment has been minimized.

Copy link
Member

commented Jul 1, 2016

@richdougherty Can you also take a look at this? I'm not that familiar with the Netty code. Seems like both 1xx and 204 responses should be explicitly checked, unless I'm missing something.

@monkey-mas

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2016

Hi @gmethvin,
Updated v2 commit based on your advice. Can you please take a look at v3 commit when you have time?

@@ -64,7 +65,7 @@ object ServerResultUtils {
)
originalErrorResult.copy(header = newHeader)
}
} else if (!mayHaveEntity(result.header.status) && !result.body.isKnownEmpty) {
} else if (mustNotIncludeBody(result.header.status)) {

This comment has been minimized.

Copy link
@schmitch

schmitch Jul 7, 2016

Member

Actually you shouldn't remove !result.body.isKnownEmpty here, since it's unnecessary to create a new HttpEntity if it's already empty.

This comment has been minimized.

Copy link
@monkey-mas

monkey-mas Jul 7, 2016

Author Contributor

oh I see, as we have Future.successful(result.copy(body = HttpEntity.Strict(ByteString.empty, result.body.contentType))) two lines below this condition.
Thanks for your review! Fixed it! :)

@schmitch

This comment has been minimized.

Copy link
Member

commented Jul 7, 2016

According to the one comment I made this looks good to me.

false
case _ =>
true
}

This comment has been minimized.

Copy link
@richdougherty

richdougherty Jul 14, 2016

Member

I noticed there is the same logic in ServerResultUtils. Would you be able to remove this method from NettyModelConversion and call the ServerResultUtils method instead?

private def mayHaveEntity(status: Int) =
status != Status.NO_CONTENT && status != Status.NOT_MODIFIED
private def mustNotIncludeBody(status: Int): Boolean = status match {
case CONTINUE | SWITCHING_PROTOCOLS | NO_CONTENT | NOT_MODIFIED =>

This comment has been minimized.

Copy link
@richdougherty

richdougherty Jul 14, 2016

Member

I haven't checked, but are there tests for the new status codes that you've added?

@richdougherty

This comment has been minimized.

Copy link
Member

commented Jul 14, 2016

LGTM – just a couple of minor comments. :)

@monkey-mas monkey-mas force-pushed the monkey-mas:remove-content-length-header-in-204 branch 4 times, most recently from 824160a to 78d1957 Jul 21, 2016

@monkey-mas

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2016

@richdougherty cc: @gmethvin
Thanks for taking a look! Removed the duplicate function definitions and added test cases for 1xx responses (also modified a test for a 304 response) :) Can you please review again?

P.S.
I found that BasicHttpClient just returns an empty body[1] instead of sending an actual message-body by assuming that a 204 response doesn't send a body for example. (I think there's no tests to check if the returned body is acutally empty) I'll create a new PR and add some tests for ServerResultUtils to check if a message-body is canceld[2] so that we can confirm that an empty entity is returned for 1xx, 204 and 304. What do you think? :)

[1] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-integration-test/src/test/scala/play/it/http/BasicHttpClient.scala#L214-L218
[2] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-server/src/main/scala/play/core/server/common/ServerResultUtils.scala#L67-L68

@monkey-mas monkey-mas changed the title Don't allow users to set a Content-Length header field in a 204 response Disallow users to send a message-body and Content-Length header field for 1xx, 204 and 304 responses Jul 27, 2016

@richdougherty

This comment has been minimized.

Copy link
Member

commented Aug 2, 2016

Hi @monkey-mas, I agree with the idea of adding those tests. :)

@mkurz

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

So can we merge this?
Looks like the tests @monkey-mas and @richdougherty are talking about will be added in a new pull request(?)

@monkey-mas

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2016

@richdougherty Thanks for your review!

@mkurz Yes, will do it by opening a new PR after this PR is merged! That would be better I suppose? ;)

@gmethvin Can you please have a look at the changes and ping me if they're ok? I'll then squash them and modify the commit log :)

@gmethvin

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

@monkey-mas Assuming you'll open a new PR for the tests I think this is good.

@mkurz

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

This could then be backported to 2.5.x as well

Disallow users to send a message-body and Content-Length header field…
… for 1xx, 204 and 304 responses

As described in RFC7230 section-3.3.2[1], sending a Content-Length
header field is not allowed in any response with a status code of
1xx (Informational) or 204 (No Content). This requirement was not forced
in Play, which means a user can intentionally set a Content-Length header
field like you find from this test code[2].

With regard to a Content-Length header field in a 304 response, we don't
allow users to do it currently even a server May send it as mentioned
in RFC7230 section-3.3.2.

Additionally, as described in RFC7230 section-3.3[3], 1xx responses don't include
a message body. With this commit, a message body isn't sent to meet the requirement.

Then this fix makes sure that Play never sends a `Content-Length` header field
and message-body for 1xx, 204 and 304 responses.

The following is the partial citation from RFC7230 section-3.3.2 and 3.3.
```
A server MUST NOT send a Content-Length header field in any response with
a status code of 1xx (Informational) or 204 (No Content). A server MUST NOT send
a Content-Length header field in any 2xx(Successful) response to a CONNECT request
(Section 4.3.6 of [RFC7231]).
```

```
A server MAY send a Content-Length header field in a 304 (Not Modified) response to
a conditional GET request (Section 4.1 of [RFC7232]); a server MUST NOT send
Content-Length in such a response unless its field-value equals the decimal number of
octets that would have been sent in the payload body of a 200 (OK) response to the same request.
```

```
All 1xx (Informational), 204 (No Content), and 304 (Not Modified)
responses do not include a message body.  All other responses do
include a message body, although the body might be of zero length.
```

[1] https://tools.ietf.org/html/rfc7230#section-3.3.2
[2] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-integration-test/src/test/scala/play/it/http/ScalaResultsHandlingSpec.scala#L327
[3] https://tools.ietf.org/html/rfc7230#section-3.3

@monkey-mas monkey-mas force-pushed the monkey-mas:remove-content-length-header-in-204 branch from 78d1957 to 1c0248c Aug 11, 2016

monkey-mas added a commit to monkey-mas/playframework that referenced this pull request Aug 11, 2016

Disallow users to send a message-body and Content-Length header field…
… for 1xx, 204 and 304 responses (playframework#6261)

As described in RFC7230 section-3.3.2[1], sending a Content-Length
header field is not allowed in any response with a status code of
1xx (Informational) or 204 (No Content). This requirement was not forced
in Play, which means a user can intentionally set a Content-Length header
field like you find from this test code[2].

With regard to a Content-Length header field in a 304 response, we don't
allow users to do it currently even a server May send it as mentioned
in RFC7230 section-3.3.2.

Additionally, as described in RFC7230 section-3.3[3], 1xx responses don't include
a message body. With this commit, a message body isn't sent to meet the requirement.

Then this fix makes sure that Play never sends a `Content-Length` header field
and message-body for 1xx, 204 and 304 responses.

The following is the partial citation from RFC7230 section-3.3.2 and 3.3.
```
A server MUST NOT send a Content-Length header field in any response with
a status code of 1xx (Informational) or 204 (No Content). A server MUST NOT send
a Content-Length header field in any 2xx(Successful) response to a CONNECT request
(Section 4.3.6 of [RFC7231]).
```

```
A server MAY send a Content-Length header field in a 304 (Not Modified) response to
a conditional GET request (Section 4.1 of [RFC7232]); a server MUST NOT send
Content-Length in such a response unless its field-value equals the decimal number of
octets that would have been sent in the payload body of a 200 (OK) response to the same request.
```

```
All 1xx (Informational), 204 (No Content), and 304 (Not Modified)
responses do not include a message body.  All other responses do
include a message body, although the body might be of zero length.
```

[1] https://tools.ietf.org/html/rfc7230#section-3.3.2
[2] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-integration-test/src/test/scala/play/it/http/ScalaResultsHandlingSpec.scala#L327
[3] https://tools.ietf.org/html/rfc7230#section-3.3
@monkey-mas

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2016

@gmethvin Yeah, I promise :) I rebasd the commits and modified the commit message. Though I backported this PR if you think it's not a suitable backport to 2.5.x please close the backport PR (I don't care ;)).

@gmethvin gmethvin merged commit 85ed235 into playframework:master Aug 11, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@monkey-mas monkey-mas deleted the monkey-mas:remove-content-length-header-in-204 branch Aug 12, 2016

schmitch added a commit that referenced this pull request Aug 12, 2016

[Backport 2.5.x] Disallow users to send a message-body and Content-Le…
…ngth header field for 1xx, 204 and 304 responses (#6261) (#6409)

* Disallow users to send a message-body and Content-Length header field for 1xx, 204 and 304 responses (#6261)

As described in RFC7230 section-3.3.2[1], sending a Content-Length
header field is not allowed in any response with a status code of
1xx (Informational) or 204 (No Content). This requirement was not forced
in Play, which means a user can intentionally set a Content-Length header
field like you find from this test code[2].

With regard to a Content-Length header field in a 304 response, we don't
allow users to do it currently even a server May send it as mentioned
in RFC7230 section-3.3.2.

Additionally, as described in RFC7230 section-3.3[3], 1xx responses don't include
a message body. With this commit, a message body isn't sent to meet the requirement.

Then this fix makes sure that Play never sends a `Content-Length` header field
and message-body for 1xx, 204 and 304 responses.

The following is the partial citation from RFC7230 section-3.3.2 and 3.3.
```
A server MUST NOT send a Content-Length header field in any response with
a status code of 1xx (Informational) or 204 (No Content). A server MUST NOT send
a Content-Length header field in any 2xx(Successful) response to a CONNECT request
(Section 4.3.6 of [RFC7231]).
```

```
A server MAY send a Content-Length header field in a 304 (Not Modified) response to
a conditional GET request (Section 4.1 of [RFC7232]); a server MUST NOT send
Content-Length in such a response unless its field-value equals the decimal number of
octets that would have been sent in the payload body of a 200 (OK) response to the same request.
```

```
All 1xx (Informational), 204 (No Content), and 304 (Not Modified)
responses do not include a message body.  All other responses do
include a message body, although the body might be of zero length.
```

[1] https://tools.ietf.org/html/rfc7230#section-3.3.2
[2] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-integration-test/src/test/scala/play/it/http/ScalaResultsHandlingSpec.scala#L327
[3] https://tools.ietf.org/html/rfc7230#section-3.3

* Fix broken binary-compatibility caused by Results.{Continue, SwitchingProtocols}

As we can't add methods to trait if we want to maintain binary-compatibility,
I avoided adding these vals to Results.scala.

* Remove unnecessary imports

monkey-mas added a commit to monkey-mas/playframework that referenced this pull request Aug 14, 2016

Add tests of ServerResultUtils#validateResult to make sure an empty b…
…ody is sent for 1xx, 204 and 304 responses

Purpose
Currently there's no tests to check if the returned body of 1xx, 204 and 304 responses
is empty, which is required by RFC7230. To make sure that no entity is sent to a client,
we add unit tests of ServerResultUtils#validateResult[1] that takes responsibility for
cancelling a body if necessary.

Background Context
PR(playframework#6261)[2] was merged to meet the requirement of RFC7230. BasicHttpClient is used for
the integration tests to check if an empty body is returned for 1xx, 204 and 304 responses.
These integration tests, however, don't guarantee that those responses return an empty body
as our BasicHttpClient just returns an empty body instead of sending an actual body.[3]

References
PR(playframework#6261)[2]
PR Comment[4]

[1] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-server/src/main/scala/play/core/server/common/ServerResultUtils.scala#L67-L68
[2] playframework#6261
[3] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-integration-test/src/test/scala/play/it/http/BasicHttpClient.scala#L214-L218
[4] playframework#6261 (comment)

monkey-mas added a commit to monkey-mas/playframework that referenced this pull request Aug 14, 2016

Add tests of ServerResultUtils#validateResult to make sure an empty b…
…ody is sent for 1xx, 204 and 304 responses

Purpose
Currently there's no tests to check if the returned body of 1xx, 204 and 304 responses
is empty, which is required by RFC7230. To make sure that no entity is sent to a client,
we add unit tests of ServerResultUtils#validateResult[1] that takes responsibility for
cancelling a body if necessary.

Background Context
PR(playframework#6261)[2] was merged to meet the requirement of RFC7230. BasicHttpClient is used for
the integration tests to check if an empty body is returned for 1xx, 204 and 304 responses.
These integration tests, however, don't guarantee that those responses return an empty body
as our BasicHttpClient just returns an empty body instead of sending an actual body.[3]

References
PR(playframework#6261)[2]
PR Comment[4]

[1] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-server/src/main/scala/play/core/server/common/ServerResultUtils.scala#L67-L68
[2] playframework#6261
[3] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-integration-test/src/test/scala/play/it/http/BasicHttpClient.scala#L214-L218
[4] playframework#6261 (comment)

monkey-mas added a commit to monkey-mas/playframework that referenced this pull request Aug 16, 2016

Add tests of ServerResultUtils#validateResult to make sure an empty b…
…ody is sent for 1xx, 204 and 304 responses

Purpose
Currently there's no tests to check if the returned body of 1xx, 204 and 304 responses
is empty, which is required by RFC7230. To make sure that no entity is sent to a client,
we add unit tests of ServerResultUtils#validateResult[1] that takes responsibility for
cancelling a body if necessary.

Background Context
PR(playframework#6261)[2] was merged to meet the requirement of RFC7230. BasicHttpClient is used for
the integration tests to check if an empty body is returned for 1xx, 204 and 304 responses.
These integration tests, however, don't guarantee that those responses return an empty body
as our BasicHttpClient just returns an empty body instead of sending an actual body.[3]

References
PR(playframework#6261)[2]
PR Comment[4]

[1] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-server/src/main/scala/play/core/server/common/ServerResultUtils.scala#L67-L68
[2] playframework#6261
[3] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-integration-test/src/test/scala/play/it/http/BasicHttpClient.scala#L214-L218
[4] playframework#6261 (comment)

gmethvin added a commit that referenced this pull request Aug 16, 2016

Add tests of ServerResultUtils#validateResult to make sure an empty b…
…ody is sent for 1xx, 204 and 304 responses (#6436)

Purpose
Currently there's no tests to check if the returned body of 1xx, 204 and 304 responses
is empty, which is required by RFC7230. To make sure that no entity is sent to a client,
we add unit tests of ServerResultUtils#validateResult[1] that takes responsibility for
cancelling a body if necessary.

Background Context
PR(#6261)[2] was merged to meet the requirement of RFC7230. BasicHttpClient is used for
the integration tests to check if an empty body is returned for 1xx, 204 and 304 responses.
These integration tests, however, don't guarantee that those responses return an empty body
as our BasicHttpClient just returns an empty body instead of sending an actual body.[3]

References
PR(#6261)[2]
PR Comment[4]

[1] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-server/src/main/scala/play/core/server/common/ServerResultUtils.scala#L67-L68
[2] #6261
[3] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-integration-test/src/test/scala/play/it/http/BasicHttpClient.scala#L214-L218
[4] #6261 (comment)

monkey-mas added a commit to monkey-mas/playframework that referenced this pull request Aug 17, 2016

Add tests of ServerResultUtils#validateResult to make sure an empty b…
…ody is sent for 1xx, 204 and 304 responses

Purpose
Currently there's no tests to check if the returned body of 1xx, 204 and 304 responses
is empty, which is required by RFC7230. To make sure that no entity is sent to a client,
we add unit tests of ServerResultUtils#validateResult[1] that takes responsibility for
cancelling a body if necessary.

Background Context
PR(playframework#6261)[2] was merged to meet the requirement of RFC7230. BasicHttpClient is used for
the integration tests to check if an empty body is returned for 1xx, 204 and 304 responses.
These integration tests, however, don't guarantee that those responses return an empty body
as our BasicHttpClient just returns an empty body instead of sending an actual body.[3]

References
PR(playframework#6261)[2]
PR Comment[4]

[1] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-server/src/main/scala/play/core/server/common/ServerResultUtils.scala#L67-L68
[2] playframework#6261
[3] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-integration-test/src/test/scala/play/it/http/BasicHttpClient.scala#L214-L218
[4] playframework#6261 (comment)

mkurz added a commit that referenced this pull request Aug 17, 2016

Add tests of ServerResultUtils#validateResult to make sure an empty b…
…ody is sent for 1xx, 204 and 304 responses (#6453)

Purpose
Currently there's no tests to check if the returned body of 1xx, 204 and 304 responses
is empty, which is required by RFC7230. To make sure that no entity is sent to a client,
we add unit tests of ServerResultUtils#validateResult[1] that takes responsibility for
cancelling a body if necessary.

Background Context
PR(#6261)[2] was merged to meet the requirement of RFC7230. BasicHttpClient is used for
the integration tests to check if an empty body is returned for 1xx, 204 and 304 responses.
These integration tests, however, don't guarantee that those responses return an empty body
as our BasicHttpClient just returns an empty body instead of sending an actual body.[3]

References
PR(#6261)[2]
PR Comment[4]

[1] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-server/src/main/scala/play/core/server/common/ServerResultUtils.scala#L67-L68
[2] #6261
[3] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-integration-test/src/test/scala/play/it/http/BasicHttpClient.scala#L214-L218
[4] #6261 (comment)

wsargent pushed a commit to wsargent/playframework that referenced this pull request Aug 29, 2016

Add tests of ServerResultUtils#validateResult to make sure an empty b…
…ody is sent for 1xx, 204 and 304 responses (playframework#6436)

Purpose
Currently there's no tests to check if the returned body of 1xx, 204 and 304 responses
is empty, which is required by RFC7230. To make sure that no entity is sent to a client,
we add unit tests of ServerResultUtils#validateResult[1] that takes responsibility for
cancelling a body if necessary.

Background Context
PR(playframework#6261)[2] was merged to meet the requirement of RFC7230. BasicHttpClient is used for
the integration tests to check if an empty body is returned for 1xx, 204 and 304 responses.
These integration tests, however, don't guarantee that those responses return an empty body
as our BasicHttpClient just returns an empty body instead of sending an actual body.[3]

References
PR(playframework#6261)[2]
PR Comment[4]

[1] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-server/src/main/scala/play/core/server/common/ServerResultUtils.scala#L67-L68
[2] playframework#6261
[3] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-integration-test/src/test/scala/play/it/http/BasicHttpClient.scala#L214-L218
[4] playframework#6261 (comment)

wsargent added a commit to wsargent/playframework that referenced this pull request Oct 25, 2016

Disallow users to send a message-body and Content-Length header field…
… for 1xx, 204 and 304 responses (playframework#6261)

As described in RFC7230 section-3.3.2[1], sending a Content-Length
header field is not allowed in any response with a status code of
1xx (Informational) or 204 (No Content). This requirement was not forced
in Play, which means a user can intentionally set a Content-Length header
field like you find from this test code[2].

With regard to a Content-Length header field in a 304 response, we don't
allow users to do it currently even a server May send it as mentioned
in RFC7230 section-3.3.2.

Additionally, as described in RFC7230 section-3.3[3], 1xx responses don't include
a message body. With this commit, a message body isn't sent to meet the requirement.

Then this fix makes sure that Play never sends a `Content-Length` header field
and message-body for 1xx, 204 and 304 responses.

The following is the partial citation from RFC7230 section-3.3.2 and 3.3.
```
A server MUST NOT send a Content-Length header field in any response with
a status code of 1xx (Informational) or 204 (No Content). A server MUST NOT send
a Content-Length header field in any 2xx(Successful) response to a CONNECT request
(Section 4.3.6 of [RFC7231]).
```

```
A server MAY send a Content-Length header field in a 304 (Not Modified) response to
a conditional GET request (Section 4.1 of [RFC7232]); a server MUST NOT send
Content-Length in such a response unless its field-value equals the decimal number of
octets that would have been sent in the payload body of a 200 (OK) response to the same request.
```

```
All 1xx (Informational), 204 (No Content), and 304 (Not Modified)
responses do not include a message body.  All other responses do
include a message body, although the body might be of zero length.
```

[1] https://tools.ietf.org/html/rfc7230#section-3.3.2
[2] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-integration-test/src/test/scala/play/it/http/ScalaResultsHandlingSpec.scala#L327
[3] https://tools.ietf.org/html/rfc7230#section-3.3

wsargent added a commit to wsargent/playframework that referenced this pull request Oct 25, 2016

Add tests of ServerResultUtils#validateResult to make sure an empty b…
…ody is sent for 1xx, 204 and 304 responses (playframework#6436)

Purpose
Currently there's no tests to check if the returned body of 1xx, 204 and 304 responses
is empty, which is required by RFC7230. To make sure that no entity is sent to a client,
we add unit tests of ServerResultUtils#validateResult[1] that takes responsibility for
cancelling a body if necessary.

Background Context
PR(playframework#6261)[2] was merged to meet the requirement of RFC7230. BasicHttpClient is used for
the integration tests to check if an empty body is returned for 1xx, 204 and 304 responses.
These integration tests, however, don't guarantee that those responses return an empty body
as our BasicHttpClient just returns an empty body instead of sending an actual body.[3]

References
PR(playframework#6261)[2]
PR Comment[4]

[1] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-server/src/main/scala/play/core/server/common/ServerResultUtils.scala#L67-L68
[2] playframework#6261
[3] https://github.com/playframework/playframework/blob/2.5.4/framework/src/play-integration-test/src/test/scala/play/it/http/BasicHttpClient.scala#L214-L218
[4] playframework#6261 (comment)
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.