Once add `automatic_options=True`, the header of response will generate duplicate properties

[dcalsky]

Should : Access-Control-Allow-Origin →http://example.com

Actual: Access-Control-Allow-Origin →http://example.com, http://example.com

[pejter]

The error seems to be the result of switching from CIDict to CIMultiDict as the latter allows duplicate keys.

[ashleysommer]

Thanks for the report. I knew switching to CIMultiDict would cause problems like this, but I thought I covered all my bases in my testing. I'm struggling to reproduce the problem, does it only happen with automatic_options=True? Can you please submit a minimal snippet of code which reproduces the error?

[1mh0]

Can confirm this issue, my virtualenv:

sanic==0.8.1
Sanic-Cors==0.9.5
multidict==4.4.0

And CORS extension should have automatic_options=True.

Fixed locally by changing this line: https://github.com/ashleysommer/sanic-cors/blob/master/sanic_cors/core.py#L256

To:

resp.headers.update(headers_to_set)

Seems like extend() method will keep duplicate headers, only update() will overwrite them instead.

I know that extend() according to documentation should overwrite existing keys... but it's not actually happening for some reason: https://multidict.readthedocs.io/en/stable/multidict.html#multidict.MultiDict.extend

[ashleysommer]

Thank you @1mh0 for the extra information.

I had already determined that is likely the line which needs chainging. However, I still cannot reproduce the issue myself.

I'm using the same version of sanic and multidict you've mentioned, I've run through the entire sanic-CORS test-suite and example applications with extra debugging and tracing routines introduced into the cors routines, and I simply cannot reproduce a situation in which the same Access-Control-Allow-Origin is added more than once.

The problem with the simple fix you've suggested is that some headers do need more than one value. That is the whole point of moving to the MultiDict. For example the Vary header can (and regularly does) have more than one value, if I use headers.update() here, if a 2nd value is added it would overwrite the first value.

It is the same case with Access-Control-Allow-Origin, sometimes you do want two different values for that header. Changing the line from extend() to update() will remove that capability, with only the last added allow-origin being the one which is set in the headers.

Ideally I'd like to find the source of the problem. What is causing the same Access-Control-Allow-Origin value to be added to the headers more than once? It must have something to do with automatic_options but as I said above, all of my testing, debugging, and tracing cannot reproduce the described behavior.

What I need to be able to properly fix this is a (as minimal as possible) example Sanic application that you can show exhibiting this behavior.

[dcalsky]

from sanic.response import HTTPResponse

app = Sanic()
CORS(app)

@app.middleware('request')
async def handle_options_middleware(req):
    if req.method == 'OPTIONS':
        return HTTPResponse()

This solution should work until the better solution comes out.

[pejter]

From what I was able to deduce the culprit is the OPTIONS route added by automatic_options. It sets CORS headers, but it's still a normal route so the CORS middleware gets applied and adds a second set of headers. I gleaned that just from a brief look at the code, so it might be completely wrong as I haven't done any testing yet, however the fact that the example above works seems to confirm my suspicion.

[ashleysommer]

@dcalsky Are you sure that test cast reproduces the problem for you? I think it might be missing some parts. 1) automatic_options is disabled by default, so its not even using automatic_options as described in the original comment. 2) If automatic_options is turned on, and a client sends an OPTIONS method request, that middleware wouldn't run because the automatic CORS response would be generated and returned before even getting to your code. 3) In its current form, that example does not respond with duplicated Access-Control-Allow-Origin headers.

@pejter Yes, that is the most obvious guess at what is going on, and in fact the very first thing I checked when starting to track down this problem yesterday. There is are two request-context variables that Sanic-CORS can set internally to override the final response-middleware from running. SANIC_CORS_SKIP_RESPONSE_MIDDLEWARE tells the system that something (usually the exception handler response-generator) has determined that the response middleware should not run on this response. SANIC_CORS_EVALUATED is a flag that is set when CORS headers have been added to a response, to signal that Sanic-CORS should not run the headers function again. This flag is used for cases such as when are using both the Sanic-CORS extension, and the decorator. If the decorator has already run the headers function, the extension response middleware will know to not run again.

I believe the problem is this last one. The automatic_options auto-route runs the headers function, and should set SANIC_CORS_EVALUATED and I'm sure at one point in the past it did do this correctly. However looking at it again now, I think at some point the way the automatic-options route works was modified slightly and this flag is no longer set.

It looks like this is actually and easy fix, and I can push out a release with that fix soon.

The main problem I'm facing now is determining why the tests don't reproduce this problem. I spent a long time yesterday stepping through cors response generation on all of the unit tests (some of which use automatic_options=True) and this issue never occurred. It would be great if I could get a test working for this to ensure it doesn't happen again in the future.

[ashleysommer]

Fixed in https://github.com/ashleysommer/sanic-cors/commit/61625e8caafbbc2f7e5cbd38f77949e03aea589d v0.9.6 will be uploaded to pypi after Travis test suite passes.

[pejter]

I can confirm, this fixes it. Here's a quick test case that checks the behaviour:

class AppExtensionOptions(SanicCorsTestCase):
    def setUp(self):
        self.app = Sanic(__name__)
        CORS(self.app, automatic_options=True)

        @self.app.route('/test_automatic')
        def wildcard(request):
            return text('Welcome!')

    def test_double_header(self):
        ''' Only one Origin header should be sent.
        '''
        resp = self._request('options', '/test_automatic')
        assert len(resp.headers.getall('Access-Control-Allow-Origin')) is 1

It's not great, but it's quicker then me adapting your style guide & naming scheme. Also the manual letters definition is unnecessary, because PY3 has string.ascii_lowercase, but that's for a separate PR.

[ashleysommer]

@pejter The test coding style, the naming conventions, and the use of letters all come from the author of Flask-CORS.

Sanic-CORS is a fork of Flask-CORS with as few modifications as possible to get it working on Sanic, and to get all tests passing with Sanic.

That is why there are some weird pre-py3 things going on in the code.