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

ResponseEmitter should not remove Content-Type and Content-Length when body is empty due to HEAD being used #2924

Closed
proton-ab opened this issue Jan 18, 2020 · 5 comments · Fixed by #2925
Labels

Comments

@proton-ab
Copy link
Contributor

ResponseEmitter's function emit performs check against isResponseEmpty and in such case removes headers Content-Type and Content-Length. This however has effect of removing these headers when HEAD request is sent (as the body in such case won't be set, it will be always empty). RFC2616 specifies:

The Content-Type entity-header field indicates the media type of the
entity-body sent to the recipient or, in the case of the HEAD method,
the media type that would have been sent had the request been a GET.

The Content-Length entity-header field indicates the size of the
entity-body, in decimal number of OCTETs, sent to the recipient or,
in the case of the HEAD method, the size of the entity-body that
would have been sent had the request been a GET.

Additionally, some web servers might get wrong idea and try to insert Content-Type as detected, resulting in no Content-Length header and Content-Type being set as whatever web server defaults to (for nginx this would be text/html on default configuration)

@proton-ab
Copy link
Contributor Author

In addition I think we should either expose some way for user to disable this behavior, get rid of it completely or check for some common headers like X-Accel-Redirect. Right now, if X-Accel-Redirect is used to stream body without Content-Disposition it will end up with empty Content-Type unless user performs some trickery such as $response->write('Performing internal redirect...');

@l0gicgate
Copy link
Member

I'm not opposed to changing the behavior of the ResponseEmitter. I agree that it should be controllable though so you're not bound to the default behavior that we agree on.

@proton-ab
Copy link
Contributor Author

I believe the default should be not to touch these headers even if body is empty.

Is there any specific reason why we would want to remove them even if user specifically set them? I think most web servers will try to guess Content-Type anyway if it's not present already, which defeats the point of us trying to remove it anyway. Couple that with various reasons as to why user might want to explicitly set these headers on specific response (such as X-Accel-Redirect) it seems to be more trouble than it's worth.

Will not oppose some way to enable back the old behavior however I'd like to understand in what situations it might be welcome considering HEAD should return them unaltered.

@l0gicgate
Copy link
Member

Are you going to create a pull request for this?

@proton-ab
Copy link
Contributor Author

I've made simple PR at #2925 which removes the check since I could not come with any situation where having ResponseEmitter automatically remove them would be desired behavior.

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

Successfully merging a pull request may close this issue.

2 participants