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

Initial establishment and closing of long-polling transport are missing Content-Type header #2312

Closed
KatriHaapalinna opened this issue Feb 5, 2018 · 6 comments

Comments

@KatriHaapalinna
Copy link
Contributor

Cases where Content-Type header is not included:

  • Initial establishment of the 'long-polling' transport
  • Closing of the transport

The 2nd case has a 0-length response body so perhaps 'Content-Type' doesn't make much sense, but then the return code 204 (No Content) would probably be more idiomatic/accurate than 200, because 204 would indicate more clearly that the body is empty and justify the 'missing' Content-Type.

@jfarcand
Copy link
Member

jfarcand commented Feb 5, 2018

@KatriHaapalinna It the server that decide the status code, not Atmosphere. Which server are you using and why you think we have an issue?

@KatriHaapalinna
Copy link
Contributor Author

The status code is secondary, I just meant to say that in the case of Content-Length:0, Content-Type can/should arguably be empty despite the code being a bit inaccurate.
The main issue is the missing Content-Type in the first request, which has content, but no Content-Type header;

Response headers:
HTTP/1.1 200 OK
Date: Wed, 07 Feb 2018 07:53:58 GMT
X-Atmosphere-first-request: true
X-Atmosphere-tracking-id: ac0f0cb1-b746-464c-84f5-2440266d5323
Expires: -1
Cache-Control: no-store, no-cache, must-revalidate
Pragma: no-cache
Transfer-Encoding: chunked
Server: Jetty(9.2.13.v20150730)

Response content:
45|5ccd624e-8911-4268-b2b2-34ad52251bf4|10000|X|

Is there a reason for not including the Content-Type for this response?

@jfarcand
Copy link
Member

jfarcand commented Feb 7, 2018

@KatriHaapalinna This is coming from Jetty (Atmosphere doesn't deal with that).

@jfarcand jfarcand closed this as completed Feb 7, 2018
@KatriHaapalinna
Copy link
Contributor Author

@jfarcand Are you sure? This issue is present also, when I deploy an app to Tomcat, if that's what you mean.

Why couldn't the Content-Type be added here: https://github.com/Atmosphere/atmosphere/blob/master/modules/runtime/src/main/java/org/atmosphere/runtime/AtmosphereFramework.java#L2229
e.g. res.setHeader("Content-Type", "text/plain; charset=utf-8"); ? At least some headers for the first response are set there, so why couldn't Content-Type be set there too?
Could you please tell me why that doesn't work/isn't advisable, as I don't know the limitations and scope of Atmosphere all that well?

Including Content-Type header is important for compliance with security policies (which increasingly many companies have). The Content-Type header SHOULD be defined for responses that have a payload body, if the type fo the content is known (which it is in this case), as stated in the Hypertext Transfer Protocol standards (RFC7231, HTTP/1.1 Semantics and Content, section 3.1.1.5).

Later in the aforementioned section 3.1.1.5, it is explained that if Content-Type is not set, the server MAY assume the data is application/octet-stream or examine the data to determine the type. This is not desirable, because clients that do so risk drawing wrong conclusions, which could expose additional security risks (e.g. attacks based on MIME-type confusion; XSS).

To sum up, Content-Type header is not strictly required, but very advisable for better security policy compliance, and there are really no reasons not to have it present in the response.

@jfarcand jfarcand reopened this Feb 8, 2018
@jfarcand
Copy link
Member

jfarcand commented Feb 8, 2018

@KatriHaapalinna Good point! Thanks for the explanation. Do you have the cycle to contribute a patch?

KatriHaapalinna added a commit to KatriHaapalinna/atmosphere that referenced this issue Feb 9, 2018
jfarcand added a commit that referenced this issue Feb 27, 2018
Add Content-Type to first response (#2312 )
elmot pushed a commit to vaadin/atmosphere that referenced this issue Mar 6, 2018
@jfarcand
Copy link
Member

jfarcand commented Apr 3, 2018

Fixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants