Returning Response and headers causes duplicate headers

[tonysimpson]

Expected Behavior

from flask import Flask
app = Flask(__name__)
@app.route('/')
def issue():
    return {'test': 'test'}, {'Content-Type': 'test'}

Using curl -v http://127.0.0.1:5000/ to query the view I expect only one Content-Type header > Content-Type: test

Actual Behavior

Duplicate headers are returned

< Content-Type: application/json
< Content-Type: test

Environment

• Python version: 3.8.2
• Flask version: 1.1.2
• Werkzeug version: 1.0.1

Context

This issue also effects responses created with make_response when using a dict or jsonify body + the headers argument with a 'Content-Type':

from flask import Flask, make_response
app = Flask(__name__)
@app.route('/')
def issue():
    return make_response({'test': 'test'}, {'Content-Type': 'test'})

This issue is caused by jsonify adding a 'Content-Type' header then make_response uses extent to add the additional headers, thus leading to the duplicate.

Returning a str/bytes body does not have this problem as no 'Content-Type' is added by flask, if one is missing it is added by werkzeug.

The reason I came across this issue is we have older code which does return json.dumps(data), 200, {'Content-Type': 'application/json+somecustomtype'} and I assumed based on the flask docs that just returning the data and letting flask do the jsonify would be better.

[greyli]

I don't know if we should change this behavior, but if you want to set the default mimetype (e.g. Content-Type value) for JSON response, you should use the JSONIFY_MIMETYPE configuration variable:

app.config['JSONIFY_MIMETYPE'] = 'application/json+somecustomtype'

This will work when you use jsonify() or just return a dict.

[ThiefMaster]

I don't think he wants to change it globally. Since duplicate content-type headers make no sense I think it's reasonable to take the one from the return value

[tonysimpson]

@greyli thanks, we need to support multiple different content-types for JSON in the same app. I think this is pretty common for production APIs that have been around for a while like ours.
@ThiefMaster sounds right, I'll put a PR together.

[davidism]

Before we do anything, I want to understand why headers are extended rather than appended right now. I think I was the last one to touch this code, and either it was already like that or I made a decision about it during refactoring.

If you want to control the headers, instead of using the shortcut, get the response and set the headers directly:

response = jsonify(...)
response.headers.set('Content-Type', 'custom')
return response

[davidism]

This behavior is explicitly documented in app.make_response right now:

If body is a response_class instance, status overwrites the exiting value and headers are extended.

Changing this is not compatible with that. There is no way to change the shortcut to satisfy all use cases. Some headers should be replaced, some should be extended, but Flask doesn't have insight into those semantics.

[davidism]

Looking at the history, returning a dict of headers has used extend since 2012. Before that, all headers were discarded and only the passed headers were used. I'm not seeing a clear cut reason to prefer set over extend, and the previous 2012 change seems intentional.

[pgjones]

Given that,

return "xyz", {"Content-Type": "text/xml"}

returns xml content type despite it actually returning b"xyz", I think that

return {}, {"Content-Type": "text/xml"}

should return a xml content type as well, despite it actually returning b"{}". With application/json+somecustomtype being the actual motivating use case.

[tonysimpson]

@davidism I think you're right, if you use jsonify and headers with a 'Content-Type' that's a programming error and nothing can be done, but I think there is a real issue when returning a dict body.

When a str/bytes/bytearray body is returned any returned headers are passed to to response_class and the logic of werkzeug BaseResponse means that if 'Content-Type' is in the headers it is used, in this case the headers are then set to None and so not extended into the response object headers.

But if the body returned is a dict, jsonify pass a mimetype argument to werkzeug BaseResponse which will set 'Content-Type', then the headers will be extended and if there is a 'Content-Type' the response object will now have two 'Content-Type' headers. I'm suggesting this behaviour is a bug and the dict body case should act similarly to the str case.

From make_response:

        if not isinstance(rv, self.response_class):
            if isinstance(rv, (str, bytes, bytearray)):
                # let the response class set the status and headers instead of
                # waiting to do it manually, so that the class can handle any
                # special logic
                rv = self.response_class(rv, status=status, headers=headers)
                status = headers = None
            elif isinstance(rv, dict):
                rv = jsonify(rv)
... SNIP ...
        # extend existing headers with provided headers
        if headers:
            rv.headers.extend(headers)

[davidism]

If we change it the way you're describing, dict, headers would result in different behavior than jsonify(), headers. In the first case, Flask would detect a dict, call jsonify, then replace headers. In the second case, all Flask would see is a response object, and would extend headers. This doesn't seem correct.

[pgjones]

I think that is understandable as jsonify returns a Response, whereas dict is just a response value.

My corollary is that jsonify is make_response when dict is str. Specifically make_response("xyz"), headers extends the headers whereas "xyz", headers sets the headers.

[davidism]

Returning a dict is just a shortcut for calling jsonify though, so that's not the case.

I definitely recognize there's an issue with the inconsistency. Either the headers should always use extend, or always use set.

I'd be more willing to always use set, because I can think of few headers where extend is actually what's wanted. Most headers, or at least most common use cases, are a single item.

[davidism]

@pgjones @ThiefMaster thoughts on using set/update consistently instead of extend?

[mitsuhiko]

I feel like this might even have been unintentional. The original code constructed the response object with those arguments and in that case it would have overwritten the keys and not extended.

[davidism]

and the previous 2012 change seems intentional.

I feel like this might even have been unintentional.

Yes, I wasn't quite sure why the change used extend, maybe it intended to use update. I think switching to using update / set makes more sense.

[pgjones]

I think switching to update/set makes sense.

[greyli]

I noticed there is a test on duplicate headers support, simply change rv.headers.extend to rv.headers.update will break this test. Since duplicate headers are allowed in HTTP, maybe we only need to make sure the Content-Type is unique? I proposed another PR (#3690) for it.

[davidism]

I think single value headers are more common than multi-line headers, and based on discussion using headers.extend seems unintentional. I'd rather use headers.update consistently, and right now the response, headers case is the odd one out compared to the behavior of the others. If extend is needed, the response object is already available in that case, to be modified as needed.