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

Don't include CONTENT_TYPE/CONTENT_LENGTH in environ if absent in headers #1308

Merged
merged 3 commits into from
May 21, 2018
Merged

Don't include CONTENT_TYPE/CONTENT_LENGTH in environ if absent in headers #1308

merged 3 commits into from
May 21, 2018

Conversation

thusoy
Copy link
Contributor

@thusoy thusoy commented May 18, 2018

This has been reported earlier in #1049 and #1159, but closed since it's valid behavior according to the WSGI spec. It is however inconsistent with how most of the wsgi servers handle it.

With the following test application I tested this for gunicorn, gevent, twisted and uwsgi:

# test_app.py
from werkzeug.wrappers import Response

def application(environ, start_response):
    content_type = environ.get('CONTENT_TYPE')
    response = Response(repr(content_type) + '\n')
    return response(environ, start_response)
Test case Outcome
gunicorn test_app None
gevent.WSGIServer.serve_forever() None
twistd -n web test_app.application ''
uwsgi --http :8000 --wsgi-file test_app.py None

Of the four tested, only Twisted handles the environ the way werkzeug does, the others will experience inconsistencies between their test and production environments.

We had trouble reproducing a production error in test because of this as it's currently non-trivial to test a missing content-type header in flask. If absent by default it's easy to test both absent and empty header values, as you can send an empty string in the headers to get the old behavior.

thusoy added 2 commits May 18, 2018 13:53
This makes the test environment consistent with the rest.
@thusoy thusoy changed the title Missing content type Don't include CONTENT_TYPE/CONTENT_LENGTH in environ if absent in headers May 18, 2018
@lepture
Copy link
Contributor

lepture commented May 19, 2018

It makes sense to me since those common production ready servers behave like this.

@davidism
Copy link
Member

I updated this to only omit the headers if they're None, which seems more sensible in terms of being able to test different values.

@davidism davidism merged commit 84718b4 into pallets:master May 21, 2018
@thusoy thusoy deleted the missing-content-type branch May 21, 2018 21:07
@davidism davidism added this to the 0.15 milestone Dec 9, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants